fix(cli): re-download the browser when the cached archive is corrupt [P0] - #1982
Merged
Merged
Conversation
A partially-downloaded or interrupted chrome-headless-shell archive left in the cache makes @puppeteer/browsers' install() throw "invalid end-of-central-directory" during extraction. That error propagated out of the browser check and hard-blocked the render, forcing users onto the fallback renderer until they manually cleared the cache — a recurring Windows failure. Detect the corrupt-archive extraction error (isCorruptArchiveError), clear the cache to drop the bad archive, and retry the download exactly once; non-corrupt errors and a second corruption still propagate (no infinite retry). The pure predicate and the recovery wrapper are unit-tested.
miguel-heygen
marked this pull request as ready for review
July 7, 2026 19:04
vanceingalls
reviewed
Jul 7, 2026
vanceingalls
left a comment
Collaborator
There was a problem hiding this comment.
R1 — #1982 at 83e89063e4a26ac9c97d386581db532bb4adfa66
🟢 LGTM. Clean self-heal, bounded retry, real unit coverage. One required-check blocker + two small nits.
Correctness
isCorruptArchiveErrormatches the six known truncated/corrupt-zip signatures with a lowercase substring check and explicitly rejects network errors (ECONNRESET,ENOENT,socket hang up). Predicate lives at pure-fn-level — the test that pins the boundary is doing real work.installWithCorruptArchiveRecoveryis a single-retry wrapper: on corrupt →onRecover→clearCache→ one retry; on non-corrupt → propagate unchanged; on second corrupt → propagate (no infinite loop). All three branches unit-tested. Order ofonRecover(beforeclearCache) is fine — the warn is future-tense.CACHE_DIR = ~/.cache/hyperframes/chrome(verified at head) is our dedicated subdirectory, sormSync(recursive: true, force: true)here doesn't clobber~/.cache/puppeteer/chrome-headless-shellor any user data outside our cache.normalizeErrorMessagecorrectly imported from../utils/errorMessage.js; embedded in the warn with original case, lowercased inside the predicate. Consistent.- Only fires when
install()throws during extraction. A silent-truncation failure (install succeeds but binary is broken) isn't in scope here — that's a different failure mode and belongs elsewhere.
Blocker
Semantic PR titleCI check FAILED on the current head (SUCCESS on the older SHA at 06:32, FAILURE at 19:02 after the title got[P0]prefixed). Same as #2025. Drop the[P0]from the title or move it to a label. Required-check-shaped — block merge until it's green.
Nits (non-blocking)
msg.includes("corrupted")is broad — one-word substring, could false-positive on unrelated errors that happen to say "corrupted." The tradeoff is right (miss-a-variant hard-blocks the render; false-positive triggers a rare re-download), but consider tightening to"file is corrupted"/"archive is corrupted"if you want to be defensive.- Cache scope on recovery is the whole
CACHE_DIR, not the failing archive subdir. Fine given the cache is dedicated and single-tenant, but a more surgicalrmon just thechrome-headless-shell/<version>/<platform>folder would preserve any manually-placed browsers a user has staged. Micro-nit — the current scope matches howclearBrowser()already treats this dir.
R1 by Via
Collaborator
Author
|
@via thanks. Title fixed the same way (priority tag moved to the end). One extra note: I dug into the 'already-stuck box' case (archive gone, empty install dir) and it's already handled outside this predicate — the resolve path returns a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A partially-downloaded or interrupted
chrome-headless-shellarchive left in the cache makes@puppeteer/browsers'install()throwinvalid end-of-central-directoryduring extraction (a zip whose central directory is truncated/missing). That error propagated straight out ofdownloadBrowser→ the browser check, hard-blocking the render with no recovery. Users had to manually clear the cache to render again; several fell back to the deterministic local-frame renderer instead. This is a recurring Windows failure.Fix
In
packages/cli/src/browser/manager.ts, detect the corrupt-archive extraction error and self-heal:isCorruptArchiveError(err)— pure predicate matching truncated/corrupt-zip signatures (end of central directory,not a zip,invalid or corrupt,unexpected end of,corrupted), while not matching network errors (ECONNRESET, etc.).installWithCorruptArchiveRecovery(runInstall, clearCache, onRecover)— runs the install; on a corrupt-archive error it clears the cache (dropping the bad archive) and retries the download exactly once. Non-corruption errors propagate unchanged, and a second corruption propagates too (no infinite retry).downloadBrowserwires the two together, clearingCACHE_DIRand warning before the single retry.Network errors and genuine platform problems are unaffected — only a corrupt cached archive triggers the clear-and-redownload.
Verification
packages/cli/src/browser/manager.test.ts— 6 new assertions (21 total pass): predicate matches corrupt signatures / rejects network errors; recovery clears cache + retries once then succeeds; a non-corruption error propagates without clearing; a second corruption propagates (no infinite loop).