Skip to content

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

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/cli-unzip-input-closed
Oct 21, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
fix/cli-unzip-input-closed

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

On macOS and Linux the CLI now unzips the browser download itself, with unzip's input closed, then lets
@puppeteer/browsers' install() finish (it finds the unpacked folder, runs its setup and deletes the archive).
Windows keeps the library's own path (tar.exe or PowerShell, which never ask).

Why

Follow-up to #6006. When the disk fills while the browser unpacks, unzip prints
write error (disk full?). Continue? (y/n/^C) and waits for an answer on its input. install() runs unzip through
execFile, which leaves that input open, so the answer never comes: the first render hangs for good while holding
the install lock. #6006 refuses up front when there is clearly too little room; this covers the disk that fills
mid-unzip (another process writing, or a tight margin).

Reproduced on a 20 MB RAM disk with a 60 MB zip (execFile("unzip", ["-o", zip, "-d", dir])):

unzip's input result
left open (as install() runs it) still waiting after 15 s, killed
closed (this PR) exits 50 in about 0.1 s, write error (disk full?)

The failure then reads Failed to install chrome-headless-shell …: Extraction failed: … write error (disk full?)
with the usual HYPERFRAMES_BROWSER_PATH hint, and the staging folder is removed as before.

How

  • browser/unzipArchive.ts: unzip -q -o <archive> -d <dir> through the existing runCancellableProcess
    (input already closed). No unzip binary: returns false, and install() unpacks with its own fallback.
  • browser/manager.ts: install({ unpack: false }) to download, unzipArchive into
    Cache.installationDir(...) of the staging folder, then install(). A corrupt archive still reads as corrupt
    (unzip's own words) and still gets one re-download.

Tests

  • unzipFallback.test.ts, real processes: the composition against the real install() (our unzip lands where
    install() looks, it then returns the browser and the archive is gone); a stand-in unzip that asks the
    question and reads its input fails at once; no unzip on PATH returns false.
  • manager.test.ts: linux and darwin unzip first, win32 does not; a disk that fills mid-unzip fails the install
    without calling install() again and leaves no staging folder.
  • Each was broken on purpose and failed: no own unzip, own unzip on Windows too, input left open (the
    full-disk test times out), no missing-binary fallback.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 151 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@terencechotai2-commits terencechotai2-commits left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes at 84394360.

The fix itself works. I reproduced the hang I reported on #6006 with the real Chrome download. On a disk that fills partway through the unzip, main hangs while holding the install lock. This branch fails in about 9 s with unzip's own "write error (disk full?)" and the HYPERFRAMES_BROWSER_PATH hint, and leaves nothing behind. The happy path still installs a working browser where the cache lookup expects it, and the archive is deleted afterwards.

What blocks is the tests: two of the new manager.test.ts cases can't pass on Windows, and the required Tests on windows-latest check is red at this head. Small test-only fix.

Blocking

  1. The new manager tests assume POSIX, and they run on the Windows lane. The engine-cli lane runs --filter @hyperframes/cli tests (.github/workflows/windows-render.yml:444-446). Two problems:
    • "a disk that fills mid-unzip fails the install at once…" (packages/cli/src/browser/manager.test.ts, the last case in "downloadBrowser — too little disk") never pins process.platform. On Windows, unzipMayAskOnInput is false (manager.ts:744), so the mocked unzip that throws never runs. install() then succeeds, and the test gets "promise resolved instead of rejecting" plus install called. Reproduced on Linux by pinning process.platform to win32 in that test: AssertionError: promise resolved "{ …(2) }" instead of rejecting.
    • The linux and darwin cases of "on $platform, unzips the download itself first" check into against /\.chrome-staging-[^/]+\/chrome-headless-shell\/linux-[^/]+$/. On a real Windows runner path.join gives \ separators, so the regex can't match. This one is from reading the code; I didn't run it on Windows.
    • Fix: pin process.platform to "linux" in the disk-full test, the same way the it.each does, or skipIf(win32). Build the expected path with join(...), or match on [\\/].
    • CI at 84394360: Tests on windows-latest: engine-cli was cancelled at the 20-minute job limit during "Run engine-cli package tests", so the aggregate Tests on windows-latest is failure. The last six other runs of that lane finished in 10-15 min. The log isn't retrievable ("log not found"), so it's unverified whether this PR's tests caused the timeout. The failing test above is enough either way.

Non-blocking

  1. Low: a cancelled render now leaves unzip running for a few seconds. unzipArchive goes through runCancellableProcess, which spawns with detached: true on POSIX (packages/cli/src/utils/cancellableProcess.ts:67). That puts unzip in its own process group. A render cancel kills the setup worker's process group, so unzip is no longer part of it and keeps running. Before this PR, the library's execFile unzip sat in the worker's group and died with it.
    • Repro: the real ensureBrowser({ force: true, signal }) on a 1 GB tmpfs, aborted as soon as linux-152.0.7977.30/ appeared in staging. On this branch, unzip -q -o … -d …/linux-152.0.7977.30 was still running 3 s after ensureBrowser rejected with "user cancelled", and staging held 285 MB. On base 6cb9ce096 the same run showed no unzip after exit, with a 114 MB archive left in staging.
    • Effect: a few more seconds of CPU and disk writes after the user cancelled, with the lock already released. I tried to make the next run's sweep fail: rmSync(staging) against a live unzip passed 5/5, and unzip then exits 2 into the deleted dir. So I have no failing follow-on scenario. The sweep racing a live unzip is unverified beyond that.
    • Fix (pick one):
      • run unzip without detached, i.e. spawn("unzip", args, { stdio: ["ignore", "pipe", "pipe"] });
      • or let runCancellableProcess take a detached option;
      • or keep the library's call and do execFile(...).child.stdin?.end().
  2. Nit: the error message shows unzip's question. The failure reads Extraction failed: …/chrome-headless-shell: write error (disk full?). Continue? (y/n/^C) \nwarning: … is probably truncated. A user sees a prompt they can't answer. Mapping unzip exit 50 to "The disk filled up while unpacking Chrome (N MB free at ~/.cache/hyperframes). Free disk space, then retry." would read like the up-front #6006 message. packages/cli/src/browser/unzipArchive.ts:13-14.
  3. Nit: depends on the parent folder existing. Info-ZIP -d creates only the last directory level. Unzipping to a dir two levels deep under a missing parent exits 2 at once (checked). The library does mkdir(folderPath, { recursive: true }) before its unzip. This works today because install({ unpack: false }) already created browserRoot, where the archive lives. An mkdirSync(folderPath, { recursive: true }) in unzipArchive would make that independent of the archive's location.

Checked and fine

  • Disk fills mid-unzip (real repro). Setup: unshare -rm, 530 MB tmpfs as HOME/TMPDIR. That passes the 512 MB floor from #6006. A filler wrote 300 MB as soon as .chrome-staging-* appeared, leaving room for the 114 MB archive but not the 260 MB unpacked tree. The test ran the real ensureBrowser({ force: true }) against the real CDN, Chrome 152.0.7977.30 linux64.
    • Branch: FAIL 8940 ms with the message above. The cache root was empty afterwards (no staging, no lock, no chrome/), and no unzip process remained.
    • Base 6cb9ce096 (manager.ts reverted): still running at 60 s and killed. The disk was at 100%, with .chrome-staging-* and .chrome.install.lock left behind.
  • Not mistaken for a corrupt archive. The disk-full stderr ("probably truncated") does not match isCorruptArchiveError, so there is no second download. Both the real repro and the new manager test confirm it.
  • Corrupt archives still recover. Truncated and garbage zips make unzip print "End-of-central-directory signature not found" on stderr with exit 9. unzipArchive surfaces stderr, so the one-time re-download still triggers. On retry, clearStaging removes the archive, so install({ unpack: false }) downloads it again instead of reusing the bad one.
  • install({ unpack: false }) and the second install() (3.2.2 install.js).
    • With unpack: false it returns browserRoot/<buildId>-<file>.zip.
    • Our unzip writes to Cache.installationDir(...), which is the same outputPath the library uses.
    • The second call takes the "folder exists" branch: it checks the executable (and throws IncompleteInstallationError if it is missing), runs runSetup, and unlinks the archive in finally.
    • DefaultProvider makes no network calls on the second call.
    • A .metadata file is only written for buildIdAlias or non-default providers, and we use neither, so skipping the library's unpack loses nothing.
    • Real end-to-end on a 1 GB tmpfs: source: "download", then a second ensureBrowser() returned source: "cache" at the same path. No .zip remained, the binary is -rwxr-xr-x, and --version prints Google Chrome for Testing 152.0.7977.30.
  • Tool parity. 3.2.2 extractZipWithCli runs unzip -o on every non-win32 platform, macOS included. The branch runs the same Info-ZIP with the same -o, plus -q and input closed. Symlink, exec-bit and path handling are therefore unchanged. I also tested Info-ZIP 6.00 directly:
    • ../x and /x entries are stripped into the destination;
    • a write through a symlinked entry is refused ("exists but is not directory");
    • the 0755 bit is kept;
    • there is no -: flag.
  • Windows is unchanged. process.platform !== "win32" gates the new path (manager.ts:744), so tar.exe/PowerShell still run through install().
  • Linux arm64 (apt) is unchanged. ensureLinuxArmBrowser returns before this code runs.
  • No unzip on PATH. ENOENT returns false, and install() then falls back to its own chain (unzip, then yauzl), the same as before.
  • Interaction with #6006.
    • removeInstallLeftovers() and the 512 MB floor still run first, under the lock.
    • Staging is a fresh UUID dir, so no stale outputPath can exist.
    • clearStaging in finally removes partial extracts.
    • assertExecutableUnpacked still runs.
  • Output buffer. -q keeps stdout tiny, so the 1 MB maxBufferBytes limit isn't a concern.
  • Tests fail without the fix.
    • Reverting manager.ts to base: the linux and darwin "unzips first" tests and "a disk that fills mid-unzip…" fail (3 failed / 69 passed).
    • Switching unzipArchive to execFile (input left open): the stand-in-unzip test times out at 5 s.
    • The composition test runs the real install() against a local HTTP server, not a mock.

Checks run

  • env -u NODE_ENV npx vitest run src/browser/unzipFallback.test.ts src/browser/manager.test.ts: 72/72 pass.
  • The revert and mutation checks above.
  • npx tsc --noEmit -p packages/cli, after building parsers, lint and core: clean.
  • Real-CDN repros under unshare -rm with tmpfs HOME, on this branch and on base:
    • disk fills mid-unzip;
    • happy path;
    • abort mid-extract.
  • Info-ZIP probes: zip-slip, symlink, exec bit, truncated or garbage zip, missing parent dir.
  • Simulated the disk-full manager test under process.platform = "win32": fails as described.
  • Required CI at 84394360: 10 of 11 green. Tests on windows-latest is failure, because engine-cli was cancelled at the 20 min timeout.

— tai2

@miguel-heygen
miguel-heygen force-pushed the fix/cli-unzip-input-closed branch from 8439436 to 0147b70 Compare October 21, 2026 09:27
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks, all five answered at 0147b708 (rebased on main).

Blocking

  1. The disk-full test now pins process.platform/arch to linux/x64 and restores them. Forcing it to win32 reproduces your failure (promise resolved … instead of rejecting).
  2. The it.each now compares into with stagedVersionDir(dirname(archive), managedChromeVersion()), built with join, so it holds on Windows separators. Pointing the manager at a wrong folder turns both cases red.

Non-blocking

  1. Cancel now stops unzip. runCancellableProcess takes detached (the default is unchanged), and its kill uses the process group only when the child was detached. unzipArchive passes detached: false. One correction to the risk you named: killing a non-detached child "by group" doesn't hit the parent's group. kill(-pid) targets a group that doesn't exist, so the cancel just never ends. A new test pins both halves: a non-detached child shares the caller's process group, and a cancel still kills it.
  2. Exit 50 now reads "the disk filled up while unpacking Chrome. Free disk space, then retry." Other failures keep unzip's stderr, so a corrupt archive still triggers the one re-download.
  3. unzipArchive creates the parent of the target, not the target itself. With no unzip on PATH, an empty target folder would make install() throw IncompleteInstallationError (3.2.2 install.js, the "folder exists but executable missing" branch). A test covers both: a real unzip into a two-level missing folder, and no folder left behind when there is no unzip.

Each fix was broken once on purpose and its test went red. 77/77 across the three files, 3 runs in a row.

@terencechotai2-commits terencechotai2-commits left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 0147b708.

Round 2. All four round-1 points are fixed: the Windows test blocker, the cancel note and both nits. The branch was rebased onto 1d422a2fa (main), so the 8439436..head diff also carries the v0.8.182 release and #6016. I read only the packages/cli delta. I re-ran the real-CDN repros on this head: a disk that fills mid-unzip now fails in 3.7 s with plain words, a cancel leaves no unzip behind, and the happy path still installs a working browser.

All 11 required checks are green at this head. That includes Tests on windows-latest: its engine-cli lane passed, and the package-test step took 7m17s, in line with the 7-10 min of other runs today.

Round-1 points

  1. Blocker: the new manager tests assumed POSIX. Fixed.
    • The disk-full test now pins process.platform to linux and arch to x64, and restores both in finally (packages/cli/src/browser/manager.test.ts:1538).
    • The linux and darwin cases of "unzips the download itself first" now compare against stagedVersionDir(dirname(archive), managedChromeVersion()), which is built with join the same way the mocked Cache.installationDir builds into. They also check basename(dirname(archive)) against ^\.chrome-staging-, so no separator literal is left (manager.test.ts:1528-1529).
    • Simulated a Windows host: I copied each test file with process.platform = "win32" forced before collection.
      • manager "too little disk" block: 8/8 pass, including the linux, darwin and win32 cases and the disk-full test. In round 1 the disk-full test failed with "promise resolved instead of rejecting".
      • unzipFallback.test.ts: all 6 cases skip (skipIf(win32)).
      • cancellableProcess.test.ts: both process-group tests skip (describe.skipIf(!IS_POSIX)), and the worker-failure test passes.
    • Other POSIX assumptions in the new tests (/bin/sh stand-in scripts, chmod 0o755, ps -o pgid=, :-joined PATH) sit only in the skipped-on-Windows cases. ps -o pgid= -p also works on macOS.
  2. Cancel left unzip running. Fixed.
    • runCancellableProcess takes a detached option (default unchanged: process.platform !== "win32"), and unzipArchive passes detached: false (cancellableProcess.ts:60, unzipArchive.ts:15-17). unzip now stays in the setup worker's process group, which the render cancel kills.
    • Termination follows the same flag: processGroup: detached (cancellableProcess.ts:107). A non-detached child is killed via its pid tree (terminatePidTree), not kill(-pid). Without that change, kill(-pid) on a non-leader would hit ESRCH, and the "group exited" check would return at once with the child still alive.
    • Real repro, run twice: the round-1 ensureBrowser({ force: true, signal }) on a 1 GB tmpfs, aborted once linux-152.0.7977.30/ appeared in staging. It rejected with "user cancelled", and pgrep unzip was empty both right after exit and 3 s later. Staging held only the 115 MB archive, the same as base 6cb9ce096 in round 1, and the next run's sweep removes it.
    • Other callers: snapshot.ts:244/339, ffmpeg.ts:45, preflight.ts:84/211/266, renderCancellation.fixture.ts, and runRenderSetupWorker (browser, lint and orphan-cleanup through manager.ts:597, render/execute.ts:270, preview.ts:619, studioServer.ts:568, orphanCleanup.ts:25). None of them passes detached, so they keep detached: true and processGroup: true on POSIX, exactly as before. On win32, terminateProcessTree ignores processGroup (taskkill /T), so nothing changes there either.
  3. Nit: error showed unzip's "Continue? (y/n/^C)". Fixed. Exit 50 maps to "Extraction failed: the disk filled up while unpacking Chrome. Free disk space, then retry." (unzipArchive.ts:21-26), and unzip's own error stays in cause.
    • The real repro confirms that Info-ZIP exits 50 with its input closed. The full message a user sees is "Failed to install chrome-headless-shell 152.0.7977.30: Extraction failed: the disk filled up while unpacking Chrome. Free disk space, then retry.", followed by the HYPERFRAMES_BROWSER_PATH hint.
    • It is not a corrupt archive to isCorruptArchiveError, so there is no second download. There was one download and no "re-downloading" warning.
    • Corrupt archives still recover: unzipArchive on the garbage and truncated zips from round 1 still throws unzip's "End-of-central-directory" stderr, and isCorruptArchiveError is true for both.
  4. Nit: unzip -d only creates the last level. Fixed. mkdir(dirname(folderPath), { recursive: true }) runs before unzip (unzipArchive.ts:13). Only the parent is created, so when there is no unzip binary, the leaf installationDir is not left empty for install() to mistake for a broken install. The new test covers that (unzipFallback.test.ts:156).

Blocking

None.

Non-blocking

  1. Nit, optional: the disk-full message doesn't say which disk. The up-front #6006 message names N MB free at ~/.cache/hyperframes. This one says only "the disk filled up". Adding the cache root (CACHE_ROOT_DIR), or the free MB at failure, would tell someone with several volumes where to clear space. Fine to leave as is.

Checks run

  • env -u NODE_ENV npx vitest run src/browser/unzipFallback.test.ts src/browser/manager.test.ts src/utils/cancellableProcess.test.ts src/utils/processTree.test.ts: 79/79 pass.
  • Other runCancellableProcess callers' tests (cancellableProcess.killFailure, renderCancellation.process, renderCancellation, preflight, ffmpeg, snapshot): 115 pass, 16 skipped.
  • npx tsc --noEmit -p packages/cli, after building parsers, lint, studio-server, core, engine and producer: clean.
  • Windows simulation (process.platform = "win32" forced before collection) of the three changed test files: results as above.
  • Mutations, one source change at a time. Each one made a test fail:
    • drop detached: false in unzipArchive: the process-group test fails;
    • drop the mkdir: the missing-parents test fails;
    • drop the exit-50 mapping: the full-disk test fails;
    • keep processGroup: true for a non-detached child: the cancellableProcess cancel test times out with the child alive;
    • ignore the detached option: both process-group tests fail;
    • revert manager.ts to main: the linux and darwin "unzips first" tests and the disk-full test fail (3/68).
  • Real-CDN repros under unshare -rm with tmpfs HOME, Chrome 152.0.7977.30 linux64:
    • disk fills mid-unzip (530 MB tmpfs, 300 MB filler once staging appears): FAIL 3651 ms with the plain message, empty cache root, no unzip process left;
    • abort mid-extract (twice): no unzip right after exit or 3 s later;
    • happy path: source: "download", then source: "cache" at the same path. No .zip remained, the binary is -rwxr-xr-x, and --version prints Google Chrome for Testing 152.0.7977.30.
  • Corrupt-archive classification on garbage and truncated zips.
  • Required CI at 0147b708, all success: Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest (engine-cli, core, studio-1, studio-2), Studio and player captures, Test reachability, Studio: edit accuracy gate (5/5 shards).

— tai2

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 21, 2026
Merged via the queue into main with commit 84f7965 Oct 21, 2026
67 checks passed
@miguel-heygen
miguel-heygen deleted the fix/cli-unzip-input-closed branch October 21, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants