fix(cli): wait for stalled download cleanup (#2835)

## 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

- [x] Unit test updated to reproduce the Windows locked-file lifecycle on Linux
- [x] Focused regression test (20 consecutive runs)
- [x] Full CLI suite: 164 files passed, 1 skipped; 2,162 tests passed, 2 skipped
- [x] CLI typecheck
- [x] Full workspace build
- [x] Changed-file oxlint, oxfmt, Fallow, tracked-artifact check, and pre-commit hooks
- [ ] CI green on Linux, macOS, and Windows
This commit is contained in:
Miguel Ángel
2026-07-28 01:12:15 +02:00
committed by GitHub
2 changed files with 35 additions and 1 deletions
+23
View File
@@ -8,7 +8,16 @@ import type { ClientRequest, IncomingMessage } from "node:http";
import { afterEach, describe, expect, it, vi } from "vitest";
import { downloadFile } from "./download.js";
const { unlinkSyncMock } = vi.hoisted(() => ({
unlinkSyncMock: vi.fn(),
}));
vi.mock("node:https", () => ({ get: vi.fn() }));
vi.mock("node:fs", async (importOriginal) => {
const actual = await importOriginal<typeof import("node:fs")>();
unlinkSyncMock.mockImplementation(actual.unlinkSync);
return { ...actual, unlinkSync: unlinkSyncMock };
});
const mockGet = vi.mocked(httpsGet);
const tempDirs: string[] = [];
@@ -20,6 +29,17 @@ afterEach(() => {
describe("downloadFile", () => {
it("rejects an idle response and removes the partial file", async () => {
const actualFs = await vi.importActual<typeof import("node:fs")>("node:fs");
let responseClosed = false;
unlinkSyncMock.mockImplementation((path) => {
if (!responseClosed) {
const error = new Error("file is locked");
Object.assign(error, { code: "EBUSY" });
throw error;
}
actualFs.unlinkSync(path);
});
mockGet.mockImplementation(((_url: string, callback: (response: IncomingMessage) => void) => {
const response = new PassThrough() as PassThrough & {
statusCode: number;
@@ -27,6 +47,9 @@ describe("downloadFile", () => {
};
response.statusCode = 200;
response.headers = {};
response.once("close", () => {
responseClosed = true;
});
class FakeRequest extends EventEmitter {
private timeout: ReturnType<typeof setTimeout> | undefined;
+12 -1
View File
@@ -1,5 +1,6 @@
import { createWriteStream, renameSync, unlinkSync } from "node:fs";
import { get as httpsGet } from "node:https";
import type { IncomingMessage } from "node:http";
import { pipeline } from "node:stream/promises";
const DEFAULT_DOWNLOAD_TIMEOUT_MS = 30_000;
@@ -31,7 +32,11 @@ export function downloadFile(
const timeoutMs = options.timeoutMs ?? DEFAULT_DOWNLOAD_TIMEOUT_MS;
return new Promise((resolve, reject) => {
const follow = (u: string) => {
let activeResponse: IncomingMessage | undefined;
let responsePipelineStarted = false;
let requestError: Error | undefined;
const request = httpsGet(u, (res) => {
activeResponse = res;
if (res.statusCode === 301 || res.statusCode === 302) {
const location = res.headers.location;
if (location) {
@@ -47,6 +52,7 @@ export function downloadFile(
return;
}
const file = createWriteStream(tmp);
responsePipelineStarted = true;
pipeline(res, file)
.then(() => {
renameSync(tmp, dest);
@@ -54,13 +60,18 @@ export function downloadFile(
})
.catch((err) => {
removePartialFile(tmp);
reject(err);
reject(requestError ?? err);
});
});
request.setTimeout(timeoutMs, () => {
request.destroy(new Error(`Download timed out after ${timeoutMs}ms`));
});
request.on("error", (err) => {
if (responsePipelineStarted) {
requestError = err;
activeResponse?.destroy(err);
return;
}
removePartialFile(tmp);
reject(err);
});