fix(cli): a disk that fills mid-unzip fails the browser install at once instead of hanging (#6013)

* fix(cli): a disk that fills mid-unzip fails the browser install at once instead of hanging

* test(cli): the unzip tests hold on Windows, and a cancel stops unzip

packages/cli/src/browser/manager.test.ts

@@ -21,9 +21,17 @@
  * `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 { basename, dirname, join, relative, sep } from "node:path";
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { managedChromeVersion } from "./manager.js";
 
@@ -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<boolean>;
   } = {},
 ) {
   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,65 @@ 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(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 });
+        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 { 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: the disk filled up while unpacking Chrome.");
+      },
+    });
+    try {
+      const { ensureBrowser } = await import("./manager.js");
+      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 });
+    }
+  });
 });
 
 // Regression guard for HF#2103: `hyperframes render` hung forever on macOS

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<PuppeteerBrowsers> {
   try {
@@ -707,7 +709,7 @@ async function downloadBrowser(options?: EnsureBrowserOptions): Promise<BrowserR
         `${freeMb} MB free at ${CACHE_ROOT_DIR}. Free disk space, then retry.`,
     );
   }
-  const { Browser, detectBrowserPlatform, install } = await loadPuppeteerBrowsers();
+  const { Browser, Cache, detectBrowserPlatform, install } = await loadPuppeteerBrowsers();
 
   const platform = detectBrowserPlatform();
   if (!platform) {
@@ -719,12 +721,13 @@ async function downloadBrowser(options?: EnsureBrowserOptions): Promise<BrowserR
   const clearStaging = () => 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<BrowserR
           if (debug.enabled) debug(line);
         };
       },
-    });
+    };
+    const unzipMayAskOnInput = process.platform !== "win32";
+    if (unzipMayAskOnInput) {
+      const archive = await install({ ...installOptions, unpack: false });
+      const into = new Cache(stagingDir).installationDir(installOptions.browser, platform, buildId);
+      await unzipArchive(archive, into);
+    }
+    return install(installOptions);
   };
 
   try {

packages/cli/src/browser/unzipArchive.ts

@@ -0,0 +1,30 @@
+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 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<boolean> {
+  // 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], {
+      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 });
+  }
+}

packages/cli/src/browser/unzipFallback.test.ts

@@ -1,16 +1,27 @@
-import { existsSync, mkdtempSync, rmSync } 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";
 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 +58,110 @@ 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<void>((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: the disk filled up while unpacking Chrome. Free disk space, then retry.",
+      );
+    } finally {
+      rmSync(root, { recursive: true, force: true });
+    }
+  },
+  5_000,
+);
+
+it.skipIf(process.platform === "win32")(
+  "runs unzip in the caller's process group, so cancelling the setup worker stops it",
+  async () => {
+    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 });
+    }
+  },
+);

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", () => {

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);
       }
     };