Repository navigation
[miniflare] Recover from a partially downloaded Chrome install - #15206
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 66896bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
✅ All changesets look good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
|
Review posted to PR #15206. Summary of what I flagged:
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
petebacondarwin
marked this pull request as ready for review
August 15, 2026 14:52
workers-devprod
requested review from
a team and
dario-piotrowicz
and removed request for
a team
August 15, 2026 14:52
Contributor
|
Codeowners approval required for this PR:
Show detailed file reviewers |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
|
@petebacondarwin Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
petebacondarwin
force-pushed
the
fix/miniflare-chrome-install-race
branch
from
August 15, 2026 16:06
1f00721 to
3a04bc1
Compare
`@puppeteer/browsers` considers an install present as soon as the directory and executable exist. Chrome-for-Testing archives extract alphabetically, so an interrupted install leaves `chrome.exe` in place while `resources.pak` is still missing, satisfying `install()` while producing a Chrome that dies on startup with `Failed to load ...\resources.pak`. Every later launch reused that directory, so one interrupted download poisoned the cache until it was deleted by hand. Two things made this near-certain on Windows CI. The browser spec allowed 20s per test, nowhere near enough for a cold ~150 MB download, so the first test lost that race and abandoned the install mid-extraction. And the Chrome cache was never populated: `actions/cache` saves from a post step declaring `post-if: success()`, so a failing job skipped the save, and since `test-and-check.yml` runs only on `pull_request`/`merge_group` nothing is ever written to `refs/heads/main` for a PR to restore from. - Mark an install only once Chrome has actually launched from it, and on a startup failure clear and re-download an unmarked install once. Installs Chrome has started from before are left alone so real launch bugs surface. - Only treat pre-banner failures as evidence about the install. Once Chrome has printed its DevTools endpoint it has loaded the resources a partial download would lack, so a readiness-probe timeout or a profile directory that cannot be created must not cost a 150 MB re-download. - Share one download between overlapping installs, record a successful launch before the awaited marker write, and version each install directory, so a failing launch never deletes an install a concurrent session has just proven or replaced. - Wait for Chrome to exit before clearing. A crashed Chrome holds handles on files inside its install directory on Windows, which made removal fail. An unclearable install is reported on the error itself, since Miniflare logs to a no-op by default. - Give each launch attempt its own profile directory, so the retry cannot have it deleted by the previous attempt's unawaited cleanup. - Download Chrome in a `beforeAll` with a 10 minute budget so no test races the download, and broaden the retry condition to cover the launch failure. - Pair `cache/restore` with an explicit `cache/save`, gated on the completion marker so a partial install is never cached.
petebacondarwin
force-pushed
the
fix/miniflare-chrome-install-race
branch
from
August 15, 2026 18:02
df7532e to
03b53bb
Compare
…cache It shares its Chrome cache key with test-and-check.yml, which now saves only once a marker proves Chrome launched from the install. Cache keys are write-once, so an unvalidated save here could claim the key and the validated save would never replace it. Restore only.
2 of 5 tasks
dario-piotrowicz
approved these changes
Aug 16, 2026
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
Member
There was a problem hiding this comment.
The changes here look good to me (although I didn't spend as much time reviewing as I would have liked), I left a few minor comments.
One thing concerns me a bit is the amount of complexity that we're adding here for such a simple feature... 😖 but I trust your judgement on this Pete of course 🙂
workers-devprod
approved these changes
Aug 16, 2026
workers-devprod
left a comment
workers-devprod
left a comment
Contributor
There was a problem hiding this comment.
Codeowners reviews satisfied
`install.ts` was written as though it might be loaded from outside Miniflare, so it hand-rolled a structural `InstallLog` rather than naming `Log`, and exported types and functions nothing else refers to. Everything it coordinates now lives in this plugin, and the module is not re-exported from `plugins/index.ts`, so its only callers are the sibling `index.ts` and its own spec. - `InstallLog` is `Pick<Log, "warn" | "debug">`, matching `BrowserProcess` in `process.ts`. The comment justifying the structural type went with it: the standalone script it described does not exist. - `ensureBrowserInstalled` hands back the installation as a small handle carrying `markVerified()` and `discard()`. Both need the generation of the directory the caller was given, captured before it launches, so making them methods removes both the `getInstallGeneration` export and the chance of taking that token too late. - `DiscardResult` discriminates on `outcome` alone rather than on `cleared` plus `reason`, which lets the caller test the two failing outcomes directly instead of `!cleared && reason !== "superseded"`. - Inline the single-use option and result types, and unexport the four functions the handle now covers. Ten exported names become two. - Rename `installs.verified` to `installs.verifiedDirs` and reword its comment, since it holds directories rather than installations. The discard tests now hold handles instead of generation numbers, which is what they were always modelling: two sessions racing over one install.
dario-piotrowicz
approved these changes
Aug 16, 2026
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…rror When a bad install cannot be removed, the error told the developer to delete the directory by hand but dropped the reason Chrome would not start, which is the part that says whether the install is really at fault. `cause` cannot carry it: the loopback replies with `e?.stack ?? String(e)`, so only the message and stack reach the caller, and Miniflare logs to a no-op by default. Both failures therefore go in the message — the removal error inline after "could not be removed", and Chrome's own message at the end, behind the instruction that matters. `cause` keeps the removal error so `formatError` still chains it wherever the log is real. Reported by Devin. Its other point, that the message no longer matches the spec's retry condition, does not hold: every acquire failure is wrapped as "Failed to launch local browser via miniflare loopback (/browser/launch)", which matches that condition whatever the inner message says.
Merged
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
3 participants
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.
Fixes the Windows CI failure in
packages/miniflare/test/plugins/browser/index.spec.ts(Failed to launch the browser process!/Failed to load ...\resources.pak).This reproduces on
mainand is unrelated to any single PR — see runs 94802577128, 94729853512, 94741594483.Root cause
@puppeteer/browsersconsiders an install present as soon as the directory and executable exist. Chrome-for-Testing archives extract alphabetically, so an interrupted install leaveschrome.exein place whileresources.pakis still missing — satisfyinginstall()while producing a Chrome that dies on startup. Every later launch reused that directory, so a single interrupted download poisoned the cache permanently.Two things made this near-certain on Windows CI:
actions/cachesaves from a post step declaringpost-if: success(), so any job failure skipped the save — and sincetest-and-check.ymlonly runs onpull_request/merge_group, nothing is ever written torefs/heads/main, the one ref every PR can restore from. So every PR got a cold cache, every time, and each cold run could re-poison it.Existing mitigations missed it:
CORRUPTED_CACHE_ERROR_PATTERNonly matched "executable is missing", and the retry condition only matched the timeout — so it stopped retrying once the failure became an assertion.Changes
browser-rendering/install.ts; overlapping installs now share one download instead of racing to populate the same directory.beforeAllwith a 10 minute budget, so no test races the download.cache/restorewith an explicitcache/save, gated on the completion marker so a partial install is never cached.test-and-check-other-node.ymlrestore-only. It shares this cache key, and keys are write-once, so leaving it on the combinedactions/cachewould let it publish an unvalidated Chrome that the marker-gated save could then never replace.Evidence
Measured with a temporary soak workflow (14 Windows jobs per run, since removed), comparing this branch against the same commit with the runtime fix reverted.
Corrupt install — delete
resources.pak, keepchrome.exe, exactly reproducing the poisoned cache:The fixed runs log
...which it has never successfully started from; clearing it, then re-download. Baseline fails with the exact CI error. An earlier iteration of the fix scored 1/2 here, which is what surfaced the Windows file-handle bug fixed above.Cold cache — first test in the file, against its 20s timeout:
Both variants passed 10/10 in the soak: on an idle runner the download beats the timeout. The baseline still leaves as little as 4.2s of headroom on the good case, while real CI runs this suite alongside two other packages.
Cold cache, on real CI. The soak could never make the cold case actually fail. Real CI just did, and it produced a clean controlled comparison. The Windows cache entry has since expired, so both of the following jobs logged
Cache not found for input keys: chrome-Windows-f12b2a29…and downloaded Chrome cold, on the same OS, at the same time, from the same base:test/plugins/browser/index.spec.tsTest timed out in 20000ms#15209 is an unrelated test-output-noise PR that touches none of this code, so it shows what
maindoes today on a cold cache: the first test blows its 20s budget mid-download, and the remaining tests then failexpect(text.includes("sessionId")).toBe(true)against a Chrome that never started — the exact cascade this PR fixes. It is the soak's 58–79%-of-budget figure tipping past 100% on a runner that is not idle.This also means the failure is no longer merely latent: it is reproducing on
mainnow that the cache has lapsed.