Repository navigation
fix(cli): a disk that fills mid-unzip fails the browser install at once instead of hanging - #6013
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 151 of thoseThe gate passes. Quarantined, measured but not gated (0) |
terencechotai2-commits
left a comment
There was a problem hiding this comment.
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
- The new manager tests assume POSIX, and they run on the Windows lane. The
engine-clilane runs--filter @hyperframes/clitests (.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 pinsprocess.platform. On Windows,unzipMayAskOnInputis false (manager.ts:744), so the mocked unzip that throws never runs.install()then succeeds, and the test gets "promise resolved instead of rejecting" plusinstallcalled. Reproduced on Linux by pinningprocess.platformtowin32in that test:AssertionError: promise resolved "{ …(2) }" instead of rejecting. - The linux and darwin cases of "on $platform, unzips the download itself first" check
intoagainst/\.chrome-staging-[^/]+\/chrome-headless-shell\/linux-[^/]+$/. On a real Windows runnerpath.joingives\separators, so the regex can't match. This one is from reading the code; I didn't run it on Windows. - Fix: pin
process.platformto"linux"in the disk-full test, the same way theit.eachdoes, orskipIf(win32). Build the expected path withjoin(...), or match on[\\/]. - CI at
84394360:Tests on windows-latest: engine-cliwas cancelled at the 20-minute job limit during "Run engine-cli package tests", so the aggregateTests on windows-latestis 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.
- "a disk that fills mid-unzip fails the install at once…" (
Non-blocking
- Low: a cancelled render now leaves unzip running for a few seconds.
unzipArchivegoes throughrunCancellableProcess, which spawns withdetached: trueon 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'sexecFileunzip 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 aslinux-152.0.7977.30/appeared in staging. On this branch,unzip -q -o … -d …/linux-152.0.7977.30was still running 3 s afterensureBrowserrejected with "user cancelled", and staging held 285 MB. On base6cb9ce096the 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
runCancellableProcesstake adetachedoption; - or keep the library's call and do
execFile(...).child.stdin?.end().
- run unzip without
- Repro: the real
- 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. - Nit: depends on the parent folder existing. Info-ZIP
-dcreates only the last directory level. Unzipping to a dir two levels deep under a missing parent exits 2 at once (checked). The library doesmkdir(folderPath, { recursive: true })before its unzip. This works today becauseinstall({ unpack: false })already createdbrowserRoot, where the archive lives. AnmkdirSync(folderPath, { recursive: true })inunzipArchivewould 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 realensureBrowser({ force: true })against the real CDN, Chrome 152.0.7977.30 linux64.- Branch:
FAIL 8940 mswith the message above. The cache root was empty afterwards (no staging, no lock, nochrome/), 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.lockleft behind.
- Branch:
- 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.
unzipArchivesurfaces stderr, so the one-time re-download still triggers. On retry,clearStagingremoves the archive, soinstall({ unpack: false })downloads it again instead of reusing the bad one. install({ unpack: false })and the secondinstall()(3.2.2install.js).- With
unpack: falseit returnsbrowserRoot/<buildId>-<file>.zip. - Our unzip writes to
Cache.installationDir(...), which is the sameoutputPaththe library uses. - The second call takes the "folder exists" branch: it checks the executable (and throws
IncompleteInstallationErrorif it is missing), runsrunSetup, and unlinks the archive infinally. DefaultProvidermakes no network calls on the second call.- A
.metadatafile is only written forbuildIdAliasor 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 secondensureBrowser()returnedsource: "cache"at the same path. No.zipremained, the binary is-rwxr-xr-x, and--versionprintsGoogle Chrome for Testing 152.0.7977.30.
- With
- Tool parity. 3.2.2
extractZipWithClirunsunzip -oon every non-win32 platform, macOS included. The branch runs the same Info-ZIP with the same-o, plus-qand input closed. Symlink, exec-bit and path handling are therefore unchanged. I also tested Info-ZIP 6.00 directly:../xand/xentries 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 throughinstall(). - Linux arm64 (apt) is unchanged.
ensureLinuxArmBrowserreturns before this code runs. - No unzip on PATH. ENOENT returns
false, andinstall()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
outputPathcan exist. clearStaginginfinallyremoves partial extracts.assertExecutableUnpackedstill runs.
- Output buffer.
-qkeeps stdout tiny, so the 1 MBmaxBufferByteslimit isn't a concern. - Tests fail without the fix.
- Reverting
manager.tsto base: the linux and darwin "unzips first" tests and "a disk that fills mid-unzip…" fail (3 failed / 69 passed). - Switching
unzipArchivetoexecFile(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.
- Reverting
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 -rmwith 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
8439436 to
0147b70
Compare
|
Thanks, all five answered at Blocking
Non-blocking
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
left a comment
There was a problem hiding this comment.
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
- Blocker: the new manager tests assumed POSIX. Fixed.
- The disk-full test now pins
process.platformtolinuxandarchtox64, and restores both infinally(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 withjointhe same way the mockedCache.installationDirbuildsinto. They also checkbasename(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/shstand-in scripts,chmod 0o755,ps -o pgid=,:-joinedPATH) sit only in the skipped-on-Windows cases.ps -o pgid= -palso works on macOS.
- The disk-full test now pins
- Cancel left unzip running. Fixed.
runCancellableProcesstakes adetachedoption (default unchanged:process.platform !== "win32"), andunzipArchivepassesdetached: 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), notkill(-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 oncelinux-152.0.7977.30/appeared in staging. It rejected with "user cancelled", andpgrep unzipwas empty both right after exit and 3 s later. Staging held only the 115 MB archive, the same as base6cb9ce096in 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 passesdetached, so they keepdetached: trueandprocessGroup: trueon POSIX, exactly as before. On win32,terminateProcessTreeignoresprocessGroup(taskkill /T), so nothing changes there either.
- 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 incause.- 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_PATHhint. - 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:
unzipArchiveon the garbage and truncated zips from round 1 still throws unzip's "End-of-central-directory" stderr, andisCorruptArchiveErroristruefor both.
- 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
- Nit:
unzip -donly 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 leafinstallationDiris not left empty forinstall()to mistake for a broken install. The new test covers that (unzipFallback.test.ts:156).
Blocking
None.
Non-blocking
- 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
runCancellableProcesscallers' 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: falseinunzipArchive: 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: truefor a non-detached child: the cancellableProcess cancel test times out with the child alive; - ignore the
detachedoption: both process-group tests fail; - revert
manager.tsto main: the linux and darwin "unzips first" tests and the disk-full test fail (3/68).
- drop
- Real-CDN repros under
unshare -rmwith tmpfs HOME, Chrome 152.0.7977.30 linux64:- disk fills mid-unzip (530 MB tmpfs, 300 MB filler once staging appears):
FAIL 3651 mswith 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", thensource: "cache"at the same path. No.zipremained, the binary is-rwxr-xr-x, and--versionprintsGoogle Chrome for Testing 152.0.7977.30.
- disk fills mid-unzip (530 MB tmpfs, 300 MB filler once staging appears):
- 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
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 throughexecFile, which leaves that input open, so the answer never comes: the first render hangs for good while holdingthe 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])):install()runs it)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_PATHhint, and the staging folder is removed as before.How
browser/unzipArchive.ts:unzip -q -o <archive> -d <dir>through the existingrunCancellableProcess(input already closed). No unzip binary: returns false, and
install()unpacks with its own fallback.browser/manager.ts:install({ unpack: false })to download,unzipArchiveintoCache.installationDir(...)of the staging folder, theninstall(). 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 realinstall()(our unzip lands whereinstall()looks, it then returns the browser and the archive is gone); a stand-inunzipthat asks thequestion and reads its input fails at once; no
unzipon PATH returns false.manager.test.ts: linux and darwin unzip first, win32 does not; a disk that fills mid-unzip fails the installwithout calling
install()again and leaves no staging folder.full-disk test times out), no missing-binary fallback.