fix(cli): wait for stalled download cleanup

[miguel-heygen]

What

Fix Windows cleanup ordering for timed-out CLI downloads so downloadFile() rejects only after the active response pipeline has closed and the .tmp file has been removed.

Why

Main CI run 30299854376 failed in packages/cli/src/utils/download.test.ts because the timeout path rejected from the request error handler while the response pipeline still held the temporary file open. Unix permits unlinking an open file, but Windows reports the file as locked; that cleanup error was swallowed and the promise settled while the partial file still existed.

The race was introduced by #2415 / 75eedf5cc1, which added the inactivity timeout and separate request-error cleanup path.

How

• Track whether a response pipeline owns the temporary file.
• When the request errors after streaming has begun, destroy the active response and let pipeline() finish tearing down the writer.
• Preserve the original request/timeout error, then remove the partial file before rejecting.
• Make the regression deterministic on every OS by simulating Windows EBUSY until the response closes.

Test plan

• Unit test updated to reproduce the Windows locked-file lifecycle on Linux
• Focused regression test (20 consecutive runs)
• Full CLI suite: 164 files passed, 1 skipped; 2,162 tests passed, 2 skipped
• CLI typecheck
• Full workspace build
• Changed-file oxlint, oxfmt, Fallow, tracked-artifact check, and pre-commit hooks
• CI green on Linux, macOS, and Windows

[vanceingalls]

Reviewed at 0bf33cb117b111e89ee8d031d565c291aabf9160.

Verdict

APPROVE.

Minimal, targeted fix for a legitimate Windows race. The pre-existing bug: pipeline() on Unix could unlink the tmp file while the response pipeline still held it open (Windows returns EBUSY), and the request-error path settled the promise while removePartialFile was silently failing. Fix reroutes: when the request errors AFTER responsePipelineStarted, the handler destroys the response instead of unlinking, and lets pipeline().catch handle the tmp cleanup once the writer has torn down. Original error preserved via requestError sentinel.

Fix-internal remaining-silent-X audit

Traced every path the timeout / request-error / redirect / pipeline-error can take:

• Timeout while streaming — request.setTimeout fires request.destroy(new Error("Download timed out")) → request.on("error") fires → responsePipelineStarted === true → requestError = err; activeResponse.destroy(err) → response emits close → pipeline's finished sees error → pipeline .catch → removePartialFile(tmp) succeeds (writer released) → reject(requestError ?? err) preserves the timeout error. Correct.
• Request error before response — DNS/TLS failure. responsePipelineStarted === false → takes the else branch → removePartialFile(tmp) on a nonexistent file (safe, removePartialFile swallows), reject(err). Unchanged from pre-existing.
• Redirect (301/302) — activeResponse = res is set BEFORE the redirect check, so activeResponse briefly points at the 3xx response. But responsePipelineStarted stays false because the pipeline hasn't started, and the redirect calls res.resume(); follow(location) which correctly drains the old response. If a subsequent request errors during redirect (before new response arrives), the else branch fires with activeResponse pointing at the drained 3xx response — but the else branch doesn't touch activeResponse, so no harm. Fine.
• Pipeline success then late request error — .then runs renameSync(tmp, dest); resolve(), then a late request error hits request.on("error"), sets requestError = err; activeResponse?.destroy(err). Promise is already resolved, destroy on a closed response is a no-op. Safe.
• renameSync throws in .then — falls into .catch(err) → removePartialFile(tmp) (already renamed, removePartialFile swallows ENOENT) → reject(requestError ?? err) — but requestError is undefined since no request error fired, so reject(err) with the rename failure. Correct.
• Non-200, non-redirect status — res.resume(); removePartialFile(tmp); reject(new Error("HTTP N")). Unchanged. Fine.

No silent-X paths reintroduced.

Test-symmetry audit

Test uses vi.hoisted for the unlinkSyncMock so it's available before vi.mock("node:fs") runs. The mock's implementation gates EBUSY on a responseClosed latch flipped by response.once("close", ...). This is the correct shape:

1. Test-shape asymmetry: mockGet triggers a timeout via request.setTimeout fired manually (existing test scaffolding). Request destroys → response destroys via new code path → response emits close → responseClosed = true. Now pipeline .catch runs and calls removePartialFile → unlinkSync mock checks responseClosed === true → delegates to real unlinkSync. Correct sequencing.
2. Deterministic on every OS: the mock throws EBUSY deterministically until close fires, regardless of the underlying platform. On Linux without the fix, unlinkSync would succeed prematurely and mask the race; the mock forces the failure surface even where the real filesystem is permissive. This is the right shape of test — asymmetric input (not-yet-closed vs. closed) discriminates the fix from a fix that only handles the symmetric case.

Only nit: the test asserts partial-file cleanup indirectly via mock call ordering. An additional assertion like expect(existsSync(tmp)).toBe(false) after the reject would harden the shape against a future edit that stops calling removePartialFile in .catch. Not blocking.

Standards lens re-run

Mechanical grep on the added lines in both files:

• Bare as T (excluding as unknown as, as const): 0
• Non-null !. / ![ / !;: 0
• .message without instanceof Error guard: 0
• Angle-bracket cast <T>value: 0

Clean.

Non-blocking observations

1. Object.assign(error, { code: "EBUSY" }) in the test is idiomatic but reads a touch stringly-typed; a class BusyError extends Error { code = "EBUSY"; } shape would be more discoverable. Not this PR's scope.
2. activeResponse is declared inside the follow closure, so each redirect gets its own reference. Nice — matches the pre-existing pattern of scoping per-request state.
3. The requestError ?? err fallback preserves ordering (request error takes precedence over pipeline error). Since the pipeline error is downstream of the request error (destroyed response causes pipeline failure), this is the correct precedence — user sees "timed out" not "response destroyed."

Peer state

No prior reviews at this head. mergeStateStatus is BLOCKED (needs approval + CI). Awaiting Linux/macOS/Windows CI results per your test-plan checklist.

Stamped.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at 0bf33cb117b111e89ee8d031d565c291aabf9160. Via already did an exhaustive path-by-path audit + stamped — layering one cross-cutting observation on top.

Sibling parity check. The CLI has two independent download paths:

• packages/cli/src/utils/download.ts (this PR's target — HTTPS via node:https + node:stream/promises pipeline)
• packages/cli/src/cloud/download.ts (fetch-based streaming for presigned S3 URLs, used from hyperframes cloud get)

The sibling cloud/download.ts never had this race because it structures cleanup differently: a try around the for-await loop feeds errored = true in the catch, and the finally block awaits closeFile(file) (:137-151) — which itself awaits file.end(cb) — BEFORE the unlinkSync(destPath). So on cloud/download.ts, the writer is guaranteed torn down before the unlink, which is the same invariant this PR's fix now establishes for utils/download.ts via the response-destroy-then-pipeline-catch route.

Different mechanics, same invariant: don't unlink while a writer still holds the fd. Miguel's fix brings utils/download.ts to parity with the existing sibling pattern. If a future refactor consolidates these into one download primitive, both sites now support the merge.

Non-blocking harden: the test could pin the tmp-cleanup shape directly with expect(existsSync(tmp)).toBe(false) after the reject — Via called this out too. Not required for this PR.

LGTM from my side — leaving as a comment. Approval's already Via's; nothing to add there.

— Review by Rames D Jusso

[miguel-heygen]

Merge activity

• Jul 27, 11:12 PM UTC: @miguel-heygen merged this pull request with Graphite.