Claude finished @Yeraze's task —— View job

PR Review: fix(backupRoutes): async backup existence check + headersSent guards

• Read changed files
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage

This PR is well-targeted and fixes two real bugs that would surface under concurrent or error-path conditions. The changes are minimal and focused. Here are my findings:

Correctness / Bugs

archive.on('error') handler fires after finalize() rejects — both paths may execute

In backupRoutes.ts:219–229 and backupRoutes.ts:245–256, if archive.finalize() rejects, the outer catch block runs. However, archive.on('error') may also fire for the same underlying failure (archiver emits error and then the returned promise rejects). With the current code both code paths are guarded by !res.headersSent, so if headers weren't sent yet, only the first responder wins (the second call to res.status(500).json(...) would throw "headers already sent"). If headers were sent, both call res.end() — calling res.end() twice is harmless in Node.js HTTP but worth noting.

A more robust pattern would be to track whether an error response has been sent with a local flag:

let errorHandled = false;
archive.on('error', err => {
  if (errorHandled) return;
  errorHandled = true;
  // existing guard logic...
});
// and set errorHandled = true in the catch block too

This is a minor robustness concern — the headersSent guard prevents the actual crash — but the double-res.end() edge case is worth hardening.

archive.on('error') fires asynchronously but finalize() returns a Promise

The await archive.finalize() on line 242 resolves/rejects after the archive is done. If archive.finalize() throws, control jumps to catch. If archive.on('error') fires while the async iteration inside finalize() is happening, that error event fires synchronously on the event emitter before the Promise rejects. In practice the guard prevents an unhandled error, but see point above.

Code Quality

Ordering: headers sent before the audit log, but audit log is before archive.pipe(res) (lines 232–240)

The audit log call at line 232 happens before archive.pipe(res) at line 240 — but the existence check and header-setting happen at lines 204–212. This is intentional (log the download intent before streaming) and acceptable, but if the stream errors after pipe, the audit log will have recorded a download that was incomplete. This is a pre-existing design choice, not a regression.

Dynamic import('archiver') on every request

Lines 199–201 use a dynamic import('archiver') inside the request handler on every download request. Node.js caches ESM dynamic imports after the first load, so this is effectively free after warm-up, but the type cast is somewhat fragile. A top-level import (or a module-level cached import) would be cleaner and make the intent explicit. This is pre-existing from the #3523 refactor and not introduced here.

Security

Filename validation is solid. The dirname regex ^\d{4}-\d{2}-\d{2}_\d{6}$ (line 193) and YAML filename regex ^[a-zA-Z0-9\-_]+\.yaml$ (line 91) correctly prevent directory traversal — no issues here.

fsp.access uses the default F_OK flag — this only checks existence, not readability. If the backup directory exists but is not readable by the process, archiver will error mid-stream (caught by the new archive.on('error') guard), which is acceptable behavior. Using fsp.access(backupPath, fs.constants.R_OK) would give a cleaner 404 vs 500 distinction, but this is a minor improvement.

Performance

The async fsp.access replacement (line 205) correctly removes the synchronous fs.existsSync from the request path. This is the main fix and it's done correctly.

Test Coverage

The two new tests are well-constructed:

• 404 path (backupRoutes.test.ts:177–185) — correctly exercises the fsp.access rejection → 404 flow by returning a non-existent path from getBackupPath.
• Happy path (backupRoutes.test.ts:187–202) — uses a real temp directory and real archiver (not mocked), which is the right approach for streaming response tests. The finally block ensures cleanup even on test failure. Checking Content-Type, Content-Disposition, and the audit log call provides good coverage.

Missing: error path test for the headersSent guard

There's no test that exercises the archive.on('error') branch or the catch block when headers have already been sent. Testing this is admittedly hard (you'd need to mock archiver to emit an error mid-stream), but the absence means the guard code itself is uncovered. This is acceptable for now given the complexity of mocking archiver internals.

Summary

The two fixes are correct and solve the stated issues. The headersSent guard is the right pattern here. Minor robustness improvement available (double-error-handler flag), and the R_OK access check would be a slight improvement. Test coverage is good for the happy path and 404 path. Overall this is a solid, focused PR — approve with minor nits.