From f330d03109f4d8061c05186ecdefb7c50447aa2a Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Wed, 21 Oct 2026 03:53:42 -0400 Subject: [PATCH 1/2] fix(cli): a disk that fills mid-unzip fails the browser install at once instead of hanging --- packages/cli/src/browser/manager.test.ts | 74 ++++++++++++++++++- packages/cli/src/browser/manager.ts | 20 +++-- packages/cli/src/browser/unzipArchive.ts | 16 ++++ .../cli/src/browser/unzipFallback.test.ts | 68 ++++++++++++++++- 4 files changed, 171 insertions(+), 7 deletions(-) create mode 100644 packages/cli/src/browser/unzipArchive.ts diff --git a/packages/cli/src/browser/manager.test.ts b/packages/cli/src/browser/manager.test.ts index b94afa9802f..ad54df8497d 100644 --- a/packages/cli/src/browser/manager.test.ts +++ b/packages/cli/src/browser/manager.test.ts @@ -21,7 +21,15 @@ * `background-removal/manager.test.ts`) so we don't touch the real * `HOME` cache. */ -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + rmSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join, relative, sep } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -200,6 +208,7 @@ function installFsMocks({ interface InstallMockOptions { buildId: string; cacheDir: string; + unpack?: boolean; logger?: (prefix: string) => ((...args: unknown[]) => void) | undefined; } @@ -218,12 +227,14 @@ function installPuppeteerBrowsersMock( installImpl?: ( options: InstallMockOptions, ) => Promise<{ executablePath: string; path?: string }>; + unzip?: (archivePath: string, folderPath: string) => Promise; } = {}, ) { const impl = opts.installImpl ?? (async () => opts.installResult ?? { executablePath: HF_BINARY }); // Fixtures name the binary where it lands in HF_CACHE; install() really writes it under its own cacheDir. const stagedInstall = async (options: InstallMockOptions) => { + if (options.unpack === false) return join(options.cacheDir, "archive.zip"); const result = await impl(options); if (result.path || !result.executablePath.startsWith(HF_CACHE + sep)) return result; const rel = relative(HF_CACHE, result.executablePath); @@ -232,8 +243,15 @@ function installPuppeteerBrowsersMock( mockedPaths?.add(path).add(executablePath); return { executablePath, path }; }; + vi.doMock("./unzipArchive.js", () => ({ unzipArchive: opts.unzip ?? vi.fn(async () => true) })); vi.doMock("@puppeteer/browsers", () => ({ Browser: { CHROMEHEADLESSSHELL: "chrome-headless-shell" }, + Cache: class { + constructor(private readonly dir: string) {} + installationDir(browser: string, platform: string, buildId: string) { + return join(this.dir, browser, `${platform}-${buildId}`); + } + }, detectBrowserPlatform: () => opts.browserPlatform ?? "linux", getInstalledBrowsers: opts.installedInHfCacheError ? vi.fn().mockRejectedValue(opts.installedInHfCacheError) @@ -1484,6 +1502,60 @@ describe("downloadBrowser — too little disk for the browser", () => { rmSync(home, { recursive: true, force: true }); } }); + + it.each([ + { platform: "linux", unzipsFirst: true }, + { platform: "darwin", unzipsFirst: true }, + { platform: "win32", unzipsFirst: false }, + ])( + "on $platform, unzips the download itself first: $unzipsFirst", + async ({ platform, unzipsFirst }) => { + const home = useRealCacheHome(); + const { platform: origPlatform, arch: origArch } = process; + Object.defineProperty(process, "platform", { value: platform, configurable: true }); + Object.defineProperty(process, "arch", { value: "x64", configurable: true }); + const unzip = vi.fn(async () => true); + installPuppeteerBrowsersMock({ + unzip, + installImpl: async ({ cacheDir, buildId }) => + writeStagedInstall(cacheDir, buildId, "fresh"), + }); + try { + const { ensureBrowser } = await import("./manager.js"); + await ensureBrowser({ force: true }); + if (!unzipsFirst) return expect(unzip).not.toHaveBeenCalled(); + const [archive, into] = unzip.mock.calls[0] as unknown as [string, string]; + expect(dirname(archive)).toBe(dirname(dirname(into))); + expect(into).toMatch(/\.chrome-staging-[^/]+\/chrome-headless-shell\/linux-[^/]+$/); + } finally { + Object.defineProperty(process, "platform", { value: origPlatform, configurable: true }); + Object.defineProperty(process, "arch", { value: origArch, configurable: true }); + rmSync(home, { recursive: true, force: true }); + } + }, + ); + + it("a disk that fills mid-unzip fails the install at once and leaves no staged copy", async () => { + const home = useRealCacheHome(); + const install = vi.fn(async ({ cacheDir, buildId }: InstallMockOptions) => + writeStagedInstall(cacheDir, buildId, "fresh"), + ); + installPuppeteerBrowsersMock({ + installImpl: install, + unzip: async () => { + throw new Error("Extraction failed: write error (disk full?). Continue? (y/n/^C)"); + }, + }); + try { + const { ensureBrowser } = await import("./manager.js"); + await expect(ensureBrowser({ force: true })).rejects.toThrow("write error (disk full?)"); + expect(install).not.toHaveBeenCalled(); + const root = join(home, ".cache", "hyperframes"); + expect(readdirSync(root).filter((name) => name.startsWith(".chrome-staging-"))).toEqual([]); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }); }); // Regression guard for HF#2103: `hyperframes render` hung forever on macOS diff --git a/packages/cli/src/browser/manager.ts b/packages/cli/src/browser/manager.ts index 7468b05e137..61b7f5b5301 100644 --- a/packages/cli/src/browser/manager.ts +++ b/packages/cli/src/browser/manager.ts @@ -23,8 +23,10 @@ import { import { getFreeDiskMb } from "../telemetry/system.js"; import { normalizeErrorMessage } from "../utils/errorMessage.js"; import { runRenderSetupWorker } from "../utils/cancellableProcess.js"; +import { unzipArchive } from "./unzipArchive.js"; type PuppeteerBrowsers = typeof import("@puppeteer/browsers"); +type InstallOptions = import("@puppeteer/browsers").InstallOptions; async function loadPuppeteerBrowsers(): Promise { try { @@ -707,7 +709,7 @@ async function downloadBrowser(options?: EnsureBrowserOptions): Promise rmSync(stagingDir, { recursive: true, force: true }); // install() only debug-logs each failed unzip tool before trying the next one; keep those lines. let unzipLog: string[] = []; - const runInstall = () => { + const buildId = managedChromeVersion(); + const runInstall = async () => { unzipLog = []; - return install({ + const installOptions: InstallOptions & { unpack?: true } = { cacheDir: stagingDir, browser: Browser.CHROMEHEADLESSSHELL, - buildId: managedChromeVersion(), + buildId, platform, downloadProgressCallback: options?.onProgress, logger: (prefix) => { @@ -737,7 +740,14 @@ async function downloadBrowser(options?: EnsureBrowserOptions): Promise { + try { + await runCancellableProcess("unzip", ["-q", "-o", archivePath, "-d", folderPath]); + return true; + } catch (err) { + if ((err as NodeJS.ErrnoException).code === "ENOENT") return false; + const said = (err as { stderr?: string }).stderr?.trim(); + throw new Error(`Extraction failed: ${said || (err as Error).message}`, { cause: err }); + } +} diff --git a/packages/cli/src/browser/unzipFallback.test.ts b/packages/cli/src/browser/unzipFallback.test.ts index 6e1d0999a00..200e3c6c569 100644 --- a/packages/cli/src/browser/unzipFallback.test.ts +++ b/packages/cli/src/browser/unzipFallback.test.ts @@ -1,16 +1,18 @@ -import { existsSync, mkdtempSync, rmSync } from "node:fs"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { createServer } from "node:http"; import type { AddressInfo } from "node:net"; import { tmpdir } from "node:os"; import { join, relative } from "node:path"; import { Browser, + Cache, computeExecutablePath, detectBrowserPlatform, install, } from "@puppeteer/browsers"; import AdmZip from "adm-zip"; import { afterEach, expect, it, vi } from "vitest"; +import { unzipArchive } from "./unzipArchive.js"; afterEach(() => { vi.unstubAllEnvs(); @@ -47,3 +49,67 @@ it.skipIf(process.platform === "win32")( } }, ); + +it.skipIf(process.platform === "win32")( + "unzips the download itself, and install() then finds the browser and drops the archive", + async () => { + const root = mkdtempSync(join(tmpdir(), "hf-unzip-own-")); + const platform = detectBrowserPlatform(); + if (!platform) throw new Error("unsupported test platform"); + const cacheDir = join(root, "cache"); + const options = { cacheDir, browser: Browser.CHROMEHEADLESSSHELL, buildId: "1.0.0", platform }; + const executablePath = computeExecutablePath(options); + const into = new Cache(cacheDir).installationDir(options.browser, platform, options.buildId); + const archive = new AdmZip(); + archive.addFile(relative(into, executablePath), Buffer.from("browser")); + const zipBytes = archive.toBuffer(); + const server = createServer((_request, response) => response.end(zipBytes)); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + try { + const baseUrl = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; + + const archivePath = await install({ ...options, baseUrl, unpack: false }); + expect(await unzipArchive(archivePath, into)).toBe(true); + expect(existsSync(executablePath)).toBe(true); + const installed = await install({ ...options, baseUrl }); + + expect(installed.executablePath).toBe(executablePath); + expect(existsSync(archivePath)).toBe(false); + } finally { + server.close(); + rmSync(root, { recursive: true, force: true }); + } + }, +); + +it.skipIf(process.platform === "win32")( + "a full disk fails the unzip at once instead of waiting on its Continue? question", + async () => { + const root = mkdtempSync(join(tmpdir(), "hf-unzip-full-")); + const bin = join(root, "bin"); + mkdirSync(bin); + // What unzip does on a write error: ask, wait for an answer on its input, then give up. + writeFileSync( + join(bin, "unzip"), + "#!/bin/sh\nprintf 'write error (disk full?). Continue? (y/n/^C) ' >&2\nread answer\nexit 50\n", + ); + chmodSync(join(bin, "unzip"), 0o755); + vi.stubEnv("PATH", `${bin}:/usr/bin:/bin`); + try { + await expect(unzipArchive(join(root, "a.zip"), join(root, "out"))).rejects.toThrow( + "Extraction failed: write error (disk full?)", + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, + 5_000, +); + +it.skipIf(process.platform === "win32")( + "says there is no unzip command, so install() unpacks it itself", + async () => { + vi.stubEnv("PATH", join(tmpdir(), "hf-no-such-bin")); + expect(await unzipArchive("a.zip", "out")).toBe(false); + }, +); From 0147b70823ab5ea55936487ffd1952ac2f849f75 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Wed, 21 Oct 2026 05:27:26 -0400 Subject: [PATCH 2/2] test(cli): the unzip tests hold on Windows, and a cancel stops unzip --- packages/cli/src/browser/manager.test.ts | 15 +++-- packages/cli/src/browser/unzipArchive.ts | 20 +++++- .../cli/src/browser/unzipFallback.test.ts | 62 +++++++++++++++++-- .../cli/src/utils/cancellableProcess.test.ts | 32 ++++++++++ packages/cli/src/utils/cancellableProcess.ts | 6 +- 5 files changed, 120 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/browser/manager.test.ts b/packages/cli/src/browser/manager.test.ts index ad54df8497d..1e4463ae94c 100644 --- a/packages/cli/src/browser/manager.test.ts +++ b/packages/cli/src/browser/manager.test.ts @@ -31,7 +31,7 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { dirname, join, relative, sep } from "node:path"; +import { basename, dirname, join, relative, sep } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { managedChromeVersion } from "./manager.js"; @@ -1525,8 +1525,8 @@ describe("downloadBrowser — too little disk for the browser", () => { await ensureBrowser({ force: true }); if (!unzipsFirst) return expect(unzip).not.toHaveBeenCalled(); const [archive, into] = unzip.mock.calls[0] as unknown as [string, string]; - expect(dirname(archive)).toBe(dirname(dirname(into))); - expect(into).toMatch(/\.chrome-staging-[^/]+\/chrome-headless-shell\/linux-[^/]+$/); + expect(into).toBe(stagedVersionDir(dirname(archive), managedChromeVersion())); + expect(basename(dirname(archive))).toMatch(/^\.chrome-staging-/); } finally { Object.defineProperty(process, "platform", { value: origPlatform, configurable: true }); Object.defineProperty(process, "arch", { value: origArch, configurable: true }); @@ -1537,22 +1537,27 @@ describe("downloadBrowser — too little disk for the browser", () => { it("a disk that fills mid-unzip fails the install at once and leaves no staged copy", async () => { const home = useRealCacheHome(); + const { platform: origPlatform, arch: origArch } = process; + Object.defineProperty(process, "platform", { value: "linux", configurable: true }); + Object.defineProperty(process, "arch", { value: "x64", configurable: true }); const install = vi.fn(async ({ cacheDir, buildId }: InstallMockOptions) => writeStagedInstall(cacheDir, buildId, "fresh"), ); installPuppeteerBrowsersMock({ installImpl: install, unzip: async () => { - throw new Error("Extraction failed: write error (disk full?). Continue? (y/n/^C)"); + throw new Error("Extraction failed: the disk filled up while unpacking Chrome."); }, }); try { const { ensureBrowser } = await import("./manager.js"); - await expect(ensureBrowser({ force: true })).rejects.toThrow("write error (disk full?)"); + await expect(ensureBrowser({ force: true })).rejects.toThrow("the disk filled up"); expect(install).not.toHaveBeenCalled(); const root = join(home, ".cache", "hyperframes"); expect(readdirSync(root).filter((name) => name.startsWith(".chrome-staging-"))).toEqual([]); } finally { + Object.defineProperty(process, "platform", { value: origPlatform, configurable: true }); + Object.defineProperty(process, "arch", { value: origArch, configurable: true }); rmSync(home, { recursive: true, force: true }); } }); diff --git a/packages/cli/src/browser/unzipArchive.ts b/packages/cli/src/browser/unzipArchive.ts index c7af12086be..21ae9f57917 100644 --- a/packages/cli/src/browser/unzipArchive.ts +++ b/packages/cli/src/browser/unzipArchive.ts @@ -1,15 +1,29 @@ +import { mkdir } from "node:fs/promises"; +import { dirname } from "node:path"; import { runCancellableProcess } from "../utils/cancellableProcess.js"; +const UNZIP_DISK_FULL_STATUS = 50; + /** - * Unzip with its input closed. On a write error (a full disk) unzip asks "Continue? (y/n)" and waits on its input, - * which `@puppeteer/browsers` leaves open, so its own unzip hangs forever. False when there is no unzip binary. + * Unzip with its input closed: on a full disk unzip asks "Continue? (y/n)" on the input `@puppeteer/browsers` leaves + * open, and hangs. Not detached, so a cancel of the setup worker stops it. False when there is no unzip binary. */ export async function unzipArchive(archivePath: string, folderPath: string): Promise { + // unzip -d creates only the last level; an empty folderPath would make install() report a broken install. + await mkdir(dirname(folderPath), { recursive: true }); try { - await runCancellableProcess("unzip", ["-q", "-o", archivePath, "-d", folderPath]); + await runCancellableProcess("unzip", ["-q", "-o", archivePath, "-d", folderPath], { + detached: false, + }); return true; } catch (err) { if ((err as NodeJS.ErrnoException).code === "ENOENT") return false; + if ((err as { status?: number }).status === UNZIP_DISK_FULL_STATUS) { + throw new Error( + "Extraction failed: the disk filled up while unpacking Chrome. Free disk space, then retry.", + { cause: err }, + ); + } const said = (err as { stderr?: string }).stderr?.trim(); throw new Error(`Extraction failed: ${said || (err as Error).message}`, { cause: err }); } diff --git a/packages/cli/src/browser/unzipFallback.test.ts b/packages/cli/src/browser/unzipFallback.test.ts index 200e3c6c569..8ec773e5a16 100644 --- a/packages/cli/src/browser/unzipFallback.test.ts +++ b/packages/cli/src/browser/unzipFallback.test.ts @@ -1,4 +1,13 @@ -import { chmodSync, existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { + chmodSync, + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; import { createServer } from "node:http"; import type { AddressInfo } from "node:net"; import { tmpdir } from "node:os"; @@ -97,7 +106,7 @@ it.skipIf(process.platform === "win32")( vi.stubEnv("PATH", `${bin}:/usr/bin:/bin`); try { await expect(unzipArchive(join(root, "a.zip"), join(root, "out"))).rejects.toThrow( - "Extraction failed: write error (disk full?)", + "Extraction failed: the disk filled up while unpacking Chrome. Free disk space, then retry.", ); } finally { rmSync(root, { recursive: true, force: true }); @@ -107,9 +116,52 @@ it.skipIf(process.platform === "win32")( ); it.skipIf(process.platform === "win32")( - "says there is no unzip command, so install() unpacks it itself", + "runs unzip in the caller's process group, so cancelling the setup worker stops it", async () => { - vi.stubEnv("PATH", join(tmpdir(), "hf-no-such-bin")); - expect(await unzipArchive("a.zip", "out")).toBe(false); + const root = mkdtempSync(join(tmpdir(), "hf-unzip-group-")); + const bin = join(root, "bin"); + mkdirSync(bin); + writeFileSync(join(bin, "unzip"), `#!/bin/sh\nps -o pgid= -p $$ > "${join(root, "pgid")}"\n`); + chmodSync(join(bin, "unzip"), 0o755); + vi.stubEnv("PATH", `${bin}:/usr/bin:/bin`); + try { + expect(await unzipArchive(join(root, "a.zip"), join(root, "out"))).toBe(true); + const callerGroup = execFileSync("ps", ["-o", "pgid=", "-p", String(process.pid)], { + encoding: "utf8", + }); + expect(readFileSync(join(root, "pgid"), "utf8").trim()).toBe(callerGroup.trim()); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, +); + +it.skipIf(process.platform === "win32")( + "unzips into a folder whose parents do not exist yet", + async () => { + const root = mkdtempSync(join(tmpdir(), "hf-unzip-deep-")); + const archive = new AdmZip(); + archive.addFile("chrome", Buffer.from("browser")); + archive.writeZip(join(root, "a.zip")); + try { + expect(await unzipArchive(join(root, "a.zip"), join(root, "one", "two"))).toBe(true); + expect(existsSync(join(root, "one", "two", "chrome"))).toBe(true); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, +); + +it.skipIf(process.platform === "win32")( + "says there is no unzip command, and leaves no empty folder for install() to mistake for a broken install", + async () => { + const root = mkdtempSync(join(tmpdir(), "hf-unzip-none-")); + vi.stubEnv("PATH", join(root, "empty-bin")); + try { + expect(await unzipArchive(join(root, "a.zip"), join(root, "one", "two"))).toBe(false); + expect(existsSync(join(root, "one", "two"))).toBe(false); + } finally { + rmSync(root, { recursive: true, force: true }); + } }, ); diff --git a/packages/cli/src/utils/cancellableProcess.test.ts b/packages/cli/src/utils/cancellableProcess.test.ts index 92a937c8b74..5161085b14b 100644 --- a/packages/cli/src/utils/cancellableProcess.test.ts +++ b/packages/cli/src/utils/cancellableProcess.test.ts @@ -1,3 +1,4 @@ +import { execFileSync } from "node:child_process"; import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -7,6 +8,10 @@ import { testProcessIsAlive, waitForTestCondition } from "./processTestUtils.js" const IS_POSIX = process.platform !== "win32"; +function processGroupOf(pid: number): string { + return execFileSync("ps", ["-o", "pgid=", "-p", String(pid)], { encoding: "utf8" }).trim(); +} + describe.skipIf(!IS_POSIX)("cancellable process tree teardown", () => { it("reaps a SIGTERM-resistant grandchild after the setup root exits", async () => { const testDir = mkdtempSync(join(tmpdir(), "hyperframes-cancellable-process-")); @@ -78,6 +83,33 @@ describe.skipIf(!IS_POSIX)("cancellable process tree teardown", () => { rmSync(testDir, { recursive: true, force: true }); } }, 5_000); + + it("keeps a child it does not detach in the caller's process group, and a cancel still stops it", async () => { + const controller = new AbortController(); + const abortReason = new Error("cancel unzip"); + let childPid: number | undefined; + try { + const run = runCancellableProcess( + process.execPath, + ["--eval", "setTimeout(() => undefined, 10_000)"], + { + signal: controller.signal, + detached: false, + onSpawn: (pid) => { + childPid = pid; + }, + }, + ); + expect(processGroupOf(childPid!)).toBe(processGroupOf(process.pid)); + + controller.abort(abortReason); + await expect(run).rejects.toBe(abortReason); + + expect(testProcessIsAlive(childPid!)).toBe(false); + } finally { + if (childPid && testProcessIsAlive(childPid)) process.kill(childPid, "SIGKILL"); + } + }, 5_000); }); describe("render setup worker failure", () => { diff --git a/packages/cli/src/utils/cancellableProcess.ts b/packages/cli/src/utils/cancellableProcess.ts index 6916e15d28a..515eb01a147 100644 --- a/packages/cli/src/utils/cancellableProcess.ts +++ b/packages/cli/src/utils/cancellableProcess.ts @@ -14,6 +14,7 @@ export interface CancellableProcessOptions { maxBufferBytes?: number; cwd?: string; env?: NodeJS.ProcessEnv; + detached?: boolean; onSpawn?: (pid: number) => void; } @@ -56,6 +57,7 @@ export function runCancellableProcess( const signal = options.signal; signal?.throwIfAborted(); + const detached = options.detached ?? process.platform !== "win32"; return new Promise((resolve, reject) => { let child: ChildProcess; try { @@ -64,7 +66,7 @@ export function runCancellableProcess( env: options.env, stdio: ["ignore", "pipe", "pipe"], windowsHide: true, - detached: process.platform !== "win32", + detached, }; child = spawn(command, [...args], spawnOptions); } catch (error) { @@ -102,7 +104,7 @@ export function runCancellableProcess( terminationCode = code; terminationSignal = "SIGTERM"; if (processTreePid) { - terminationTask = terminateProcessTree(processTreePid, { child, processGroup: true }); + terminationTask = terminateProcessTree(processTreePid, { child, processGroup: detached }); void terminationTask.catch(() => undefined); } };