Skip to content

[miniflare] Improve error diagnostics in the Browser Run binding worker#13971

Merged
petebacondarwin merged 1 commit into
mainfrom
pbd/miniflare/flaky-browser-run-tests
May 20, 2026
Merged

[miniflare] Improve error diagnostics in the Browser Run binding worker#13971
petebacondarwin merged 1 commit into
mainfrom
pbd/miniflare/flaky-browser-run-tests

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented May 19, 2026

No specific issue — this is a developer-experience improvement motivated by a recurring CI flake in Tests (Windows, packages-and-tools) where the underlying error from the local Browser Run binding is hidden behind an opaque SyntaxError.

What

When the local Browser Run binding fails to reach an upstream — for example when Chrome fails to launch and miniflare's loopback /browser/launch endpoint returns a 500 with a stack-trace text body — the binding worker calls response.json() on the non-JSON body and throws a cryptic:

SyntaxError: Unexpected token "I", "Internal "... is not valid JSON
    at JSON.parse (<anonymous>)
    at parseJSONFromBytes (node:internal/deps/undici/...)

The actual upstream error message (e.g. Chrome readiness probe at ... timed out after 5000ms from the recently-added waitForBrowserReady probe) is discarded by .json() before the SyntaxError is thrown. This makes both local-dev failures and CI flakes nearly impossible to diagnose from logs alone.

How

Add a parseJsonResponse<T>(resp, context) helper to binding.worker.ts that:

  1. Reads the response body as text.
  2. If !resp.ok, throws Error("${context}: upstream returned ${status} ${statusText}\n${truncatedBody}").
  3. Otherwise tries JSON.parse(text). If it throws, rethrows with the same shape plus the original SyntaxError chained via cause.
  4. Truncates bodies to 2000 chars with a ... (truncated, N bytes total) suffix to avoid overwhelming CI logs.

Wire it into the three loopback JSON-parsing sites in the binding worker:

  • BrowserRenderingRouter.#acquireSession()/browser/launch (the Chrome-failed-to-launch path).
  • BrowserRenderingRouter.#getActiveSessions()/browser/sessionIds.
  • The same method's per-DO session-info read.

The internal setSessionInfoRoute req.json() (line 172) is left alone — input is always JSON from #acquireSession() itself.

Test plan

  • All 21 existing miniflare browser tests still pass locally on macOS: pnpm -F miniflare test:ci test/plugins/browser/index.spec.ts
  • pnpm -F miniflare check:type
  • pnpm check:lint
  • pnpm check:format
  • validate-changesets.ts

No new tests added — the helper is exercised by the existing passing tests on the happy path, and the failure path only matters when an upstream is broken (which is the diagnostic case we're improving, not a behavior we want to assert).

What this looks like after the next CI flake

Before:

SyntaxError: Unexpected token "I", "Internal "... is not valid JSON
    at JSON.parse (<anonymous>)
    at parseJSONFromBytes (...)

After:

Failed to launch local browser via miniflare loopback (/browser/launch): upstream returned 500 Internal Server Error
Error: Chrome readiness probe at http://127.0.0.1:53198/json/version timed out after 5000ms
    at waitForBrowserReady (.../packages/miniflare/src/plugins/browser-rendering/index.ts:286:8)
    ...

…which immediately tells us whether bumping waitForBrowserReady's 5s timeout is the right next fix, or whether something else is going on.


  • Tests
    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because: this is a pure-diagnostic change. The new helper has no behavioural effect on the happy path (all 21 existing browser-binding tests still pass), and the failure path it improves only fires when an upstream is broken — there's no stable error response to assert against, just the format of the rethrown error.
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal diagnostic improvement to miniflare's embedded browser-binding worker. No public API or user-facing string changes.

Open in Devin Review

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 19, 2026

🦋 Changeset detected

Latest commit: bb6eb87

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
miniflare Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch
wrangler Patch

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

@workers-devprod workers-devprod requested review from a team and ascorbic and removed request for a team May 19, 2026 14:41
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented May 19, 2026

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

Small nit — text.length is the character count, not the byte count. The suffix says "bytes total" which is misleading for multi-byte content. Not a blocker since the diagnostic is for developer consumption only, but worth fixing while you're here.

Comment thread packages/miniflare/src/workers/browser-rendering/binding.worker.ts
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 19, 2026

I'm Bonk, and I've done a quick review of your PR.

This PR adds a parseJsonResponse helper to the browser-rendering binding worker that reads response bodies as text first, then attempts JSON parsing — surfacing the actual upstream error message instead of an opaque SyntaxError when the response isn't valid JSON.

  1. Minor inaccuracy in truncation suffixtext.length gives UTF-16 code unit count, not byte count, but the message says "bytes total". Posted a suggestion to say "chars" instead.

Otherwise the change is clean and well-scoped. The helper is straightforward, the three call sites are correctly wired up, and the req.json() on line 172 is correctly left alone as noted in the PR description.

github run

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 19, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13971

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13971

miniflare

npm i https://pkg.pr.new/miniflare@13971

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13971

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13971

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13971

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13971

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13971

@cloudflare/workers-utils

npm i https://pkg.pr.new/@cloudflare/workers-utils@13971

wrangler

npm i https://pkg.pr.new/wrangler@13971

commit: bb6eb87

Copy link
Copy Markdown
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Blocked on #13972 but looks good

Copy link
Copy Markdown
Contributor

@workers-devprod workers-devprod left a comment

Choose a reason for hiding this comment

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

Codeowners reviews satisfied

@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk May 19, 2026
@petebacondarwin petebacondarwin force-pushed the pbd/miniflare/flaky-browser-run-tests branch from 977e531 to 0d90ea8 Compare May 19, 2026 15:14
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 19, 2026

✅ All changesets look good

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

Force-pushed to rebase onto current main. Two unrelated failures in the previous CI run:

  1. Checks job failedunstable-get-miniflare-worker-options.test.ts had a stale ./helpers/run-in-tmp import after [wrangler] Read OAuth state from disk lazily so env auth takes priority #13954 removed the local wrapper. Already fixed on main by [wrangler] Fix broken runInTempDir import in unstable-get-miniflare-worker-options test #13972, which this rebase picks up.

  2. Tests (Linux/Windows/macOS, packages-and-tools) all failed on one testMiniflare: HTTPS fetches using browser CA certificates (packages/miniflare/test/index.spec.ts:1579). The test does an outbound https://workers.cloudflare.com/cf.json fetch from inside a worker and asserts res.ok === true. Same assertion failure across all three OS jobs simultaneously strongly suggests a transient connectivity blip to workers.cloudflare.com, not anything in this PR. Will see if it reproduces on the rerun.

@petebacondarwin
Copy link
Copy Markdown
Contributor Author

Update on the rebased CI run (26106486025):

Good news on the actual browser-run tests: All 21 tests in packages/miniflare/test/plugins/browser/index.spec.ts passed on Windows this time (✓ test/plugins/browser/index.spec.ts (21 tests | 1 skipped) 240530ms). The @puppeteer/browsers cache-corruption failure from the previous run that this PR's diagnostic exposed (The browser folder ... exists but the executable ... is missing) was a one-off flake on a specific runner, not consistently reproducible. The diagnostic improvement is still valuable — it gave us a concrete root cause to investigate, and is needed for the next time it does fire — but no recovery code change is needed in this PR.

Remaining CI blocker: All three packages-and-tools jobs still fail on Miniflare: HTTPS fetches using browser CA certificates in test/index.spec.ts:1579. Root cause: the test fetches https://workers.cloudflare.com/cf.json, which now permanently 301s to https://www.cloudflare.com/cf.json which returns 404 not found :(. This is broken on main, not caused by this PR, and is blocking every PR.

Opened #13973 to fix the broken test by pointing it at https://example.com/ (a stable IANA-maintained endpoint). Once that lands I'll rebase this PR on top of it and CI should be green.

When the local Browser Run binding failed to reach an upstream — for
example when Chrome failed to launch and miniflare's loopback
`/browser/launch` endpoint returned a 500 with a stack-trace text
body — the binding worker would call `response.json()` on the non-JSON
body and throw an opaque `SyntaxError: Unexpected token X, "..." is not
valid JSON`. The actual upstream error message (e.g. `Chrome readiness
probe at ... timed out after 5000ms`) was discarded.

Add a `parseJsonResponse` helper to the binding worker that reads the
body as text first, surfaces the HTTP status and (truncated) body
content in the thrown error, and chains the original `SyntaxError` via
`cause` when the body was a 2xx response that didn't parse as JSON.
Wire it into the three loopback JSON-parsing sites in the binding
worker (`/browser/launch`, `/browser/sessionIds`, and per-DO
session-info reads).

This makes both local-dev failures and CI test flakes self-diagnosing
without requiring a debugger.
@petebacondarwin petebacondarwin force-pushed the pbd/miniflare/flaky-browser-run-tests branch from 0d90ea8 to bb6eb87 Compare May 19, 2026 15:58
@petebacondarwin petebacondarwin merged commit 59cd880 into main May 20, 2026
54 checks passed
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk May 20, 2026
@petebacondarwin petebacondarwin deleted the pbd/miniflare/flaky-browser-run-tests branch May 20, 2026 05:54
petebacondarwin added a commit that referenced this pull request May 20, 2026
Browser Run tests in `packages/miniflare/test/plugins/browser/index.spec.ts`
and the `fixtures/browser-run` fixture call into `@puppeteer/browsers`
to ensure Chrome is downloaded into the global Wrangler cache. Every
CI run currently re-downloads ~150 MB of Chrome from scratch because
the cache directory is per-runner-instance and not shared between runs.

Add an `actions/cache@v4` step keyed on the OS + the Chrome version
hardcoded in `packages/miniflare/src/index.ts` (`126.0.6478.182")`,
restoring/saving `~/.cache/.wrangler/chrome` (Linux),
`~/Library/Caches/.wrangler/chrome` (macOS), and
`~/AppData/Local/xdg.cache/.wrangler/chrome` (Windows).

Benefits:
- Cuts ~150 MB and the associated download time off cold CI runs.
- Reduces the surface area for the intermittent partial-extraction
  race that surfaces as `The browser folder (...) exists but the
  executable (...) is missing` (see #13971 for the diagnostic that
  exposed this, #13980 for the in-process recovery layer). When the
  cache is warm and the binary is already extracted, this race can't
  fire at all because `install()` short-circuits.

The cache step runs for the `packages-and-tools` suite on all three
OSes and for the `fixtures` suite on macOS + Windows (the Browser Run
fixture is excluded on Ubuntu because of AppArmor).

When the Chrome version in `packages/miniflare/src/index.ts` changes,
the cache key here needs to be bumped manually. A miss only triggers a
fresh download — no functional impact.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants