Skip to content

bake test harness: check the error overlay once instead of polling for a second per connect and write - #37866

Open
robobun wants to merge 4 commits into
mainfrom
farm/86650e60/bake-harness-overlay-check
Open

bake test harness: check the error overlay once instead of polling for a second per connect and write#37866
robobun wants to merge 4 commits into
mainfrom
farm/86650e60/bake-harness-overlay-check

Conversation

@robobun

@robobun robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • The test/bake dev tests are slow for no reason: dev/bundle.test.ts takes ~43s for 21 tests on main, and bundle, css and hot sit at 41-56s on every CI lane.
  • Cause: after every client connect, and after every write for every connected client, the harness polled for an error overlay 5 x 200ms and only stopped early when one was visible, so the common case (no errors expected, none present) always paid the full second.
  • That second was also hiding ordering bugs in the harness: the client fixture acked a build on the errors frame, before the update behind it was applied; a window abandoned by a reload still acked, and the new window acked again; connect waited for a log line rather than the page-loaded ack. Removing the poll alone made one css and one ssg test fail.
  • Split out of test(bake): run bundle.test.ts against fewer dev servers and assert served output #37827, which left this lever for a separate PR; bake test harness: ack hot updates on bun:afterUpdate instead of sleeping 550ms per write #34691 removed the other fixed sleeps on this path.

Fix

  • The overlay is checked once; the short poll is kept only when the caller expects errors. This is enough because build errors are on the page before the harness asks: the error page renders the overlay before opening its socket, and after a write the errors frame arrives before the update frame the fixture acks. Only a runtime error arriving within a second of a test's last connect or write is no longer caught, and no test relies on that.
  • The fixture now acks exactly once per build per client: pages running the HMR runtime ack the update frame, not the errors frame, and acks carry a window generation, so after a reload only the new window acks, once it has connected. A reload the test did not allow now rejects the pending write.
  • Connect and hard reload wait for that ack instead of a log line; a failed overlay check no longer leaves the write batch open; the one test that asserts DOM right after a server-side route reload now waits for the DOM to change.
  • Verification: test infrastructure only, no src/ change. Of three new hot tests, two fail 3/3 with the matching fixture hunk reverted; all of test/bake passes with the release binary (189/189), and with debug+ASAN apart from production.test.ts cases that also time out on main; the whole directory went from 277s to 138s in single runs on a shared box.

Background

  • Bake is bun's dev server with hot module reloading. test/bake drives it through a harness: dev.write() edits a file and waits for the rebuild, dev.client() loads a page in a child process running happy-dom (the client fixture).
  • The fixture acks to the harness over IPC (received-hmr-event) when a page has connected and its stylesheets loaded, and when it has applied a build. dev.client() and dev.write() resolve on that ack, so ack timing is the tests' only synchronization with the page.
  • Per build, the dev server's HMR socket sends an errors frame ("e") and then a hot update frame ("u"); while a page running the HMR runtime is connected a build always ends with "u". The page served for a route that fails to bundle subscribes to errors only.
  • The error overlay is the <bun-hmr> element. Build errors reach it as part of the frames above; runtime errors are first sent to /_bun/report_error and rendered afterwards, which is why only they can show up late.
  • A hot update to a module that does not accept itself (import.meta.hot.accept()) falls back to a full page reload; in the fixture that creates a new window while the old one may still be finishing the update.
Original description

What

Client.expectErrorOverlay() in test/bake/bake-harness.ts polled bun-hmr visibility up to 5 times with Bun.sleep(200) in between and only stopped early when an overlay was visible. The harness calls it with an empty list on every dev.client() in dev mode and, for every connected client, at the end of every dev.write() / patch() / delete() that does not pass errors: null. So the common case, no errors expected and none present, paid the full second every time. On main (USE_SYSTEM_BUN=1, this machine) bundle.test.ts makes 32 such calls and takes ~43s for 21 tests; a connect-only case is ~1.5s of which ~1s is this poll, and each write with a client attached adds ~1.07s. test/expected-durations.json shows the same 41-56s for bundle, css and hot on the release, ASAN, musl and Windows lanes, which is what fixed sleeps look like. #37827 restructures bundle.test.ts itself and leaves this harness lever for a separate PR; this is that PR. #34691 removed the other fixed sleeps on this path.

Why one check is enough

Build errors are on the page before the harness can ask about them:

  • On connect, a route with bundling failures is served the error page (hmr-runtime-error.ts), which renders the overlay synchronously before it opens the socket the harness waits for.
  • After a write, finalize_bundle (DevServer.rs) publishes the errors frame before the hot update frame on the same socket, and the harness only gets to the check after the fixture acked the build.

Only runtime errors reach the overlay later (onRuntimeError awaits /_bun/report_error before rendering), so the short poll is kept for callers that pass a non-empty errors list, where it already returned early on success. A synchronous throw during module evaluation still exits the fixture through console.error, which dev.client() / dev.write() report. What is given up: an asynchronous runtime error that surfaces within a second of the last connect or write of a test, with no later write to observe it, is no longer caught. No test relies on that; a passing test cannot.

What the sleep was hiding

Removing it made css.test.ts "changing html file with link tag works" and ssg-pages-router.test.ts "hot reload on page changes" fail, and reading client-fixture.mjs against finalize_bundle turned up two more ordering problems the poll had absorbed. Each is replaced with the signal it was standing in for:

  1. Errors frame acked too early (client-fixture.mjs). The fixture acked "e" frames immediately. While a page running the HMR runtime is connected the dev server always ends the build with a "u" frame after the "e" (will_hear_hot_update in finalize_bundle), so a write that fixed an error resolved when the overlay disappeared, before the update behind it had been applied; a second ack followed later. Pages with the runtime now ack only "u" (via bun:afterUpdate as before, or the next tick when no script was queued); the error page, which is only subscribed to errors, still acks "e". One ack per build per client.
  2. Reloaded windows kept acking (client-fixture.mjs). After location.reload() the abandoned window still ran the rest of replaceModules and acked from bun:afterUpdate (or from its pending immediate), and the new window acked again on connect. The harness resolved the write on the first one, before the new page existed, and the second one was a straggler that could satisfy the next write's wait as soon as it was registered; the poll used to absorb it. Acks now carry the window generation: reset() (called for every reload request, permitted or not) invalidates the current window, so only the live window acks, and a build that reloads the page is acked once, by the new window after it connected. This also replaces the isUpdating immediate cancellation. A reload the test did not allow now rejects the pending dev.write() via the existing exit handler instead of acking and failing later.
  3. Connect waited for the log line, not the ack (bake-harness.ts). dev.client() and Client.hardReload() returned once [Bun] Hot-module-reloading socket connected was printed. The fixture's own ack for a page load is sent after its stylesheets have loaded (checkCSSLoaded), and hardReload() could even match the line left over from an earlier reload. Both now go through Client.waitForPageLoad(), which registers for the ack before the load starts and keeps the line wait for its timeout and exit diagnostics. This is the css-13 failure.
  4. Server-side route reloads are applied asynchronously. For the React framework, dev.write() resolves when the client received the route reload; the framework then fetches the RSC payload and React commits it from its scheduler (a setTimeout(0) under happy-dom), so asserting the DOM right after the write was only passing because of the second of slack. The one test that does this (ssg-4) now uses the new Client.expectElemText(), which resolves from a MutationObserver on the document (React can replace <html> rather than patch it when the reload lands during hydration, which is why the observer is on the document and not documentElement; with it on documentElement the test still failed about 1 in 10 runs).
  5. Dev.batchChanges() left batchingChanges set when the overlay check threw, so any later write in the same test ran unsynchronized. The dispose body now clears it in a finally, which the new dev.write rejection test depends on.

expectErrorOverlay itself is restructured so both branches share the single get-errors round trip; expect(actual).toEqual([...expected].sort()) covers the empty case as before.

Tests

test/bake/dev/hot.test.ts, next to the existing ack-timing test from #34691 (which no longer needs errors: null, since the default path no longer sleeps):

  • dev.client rejects on a build error the test did not expect (single connect check still detects the error page).
  • dev.write rejects on an unexpected build error, and the write that fixes it resolves only after the fixed module's 200ms top-level await has run. With the fixture's old unconditional "e" ack this fails 3/3 with Received: "initial".
  • dev.write that triggers a full reload resolves only after the new page has run. Without the generation check this fails 3/3 with Received: undefined.

The two rejection tests capture the error with runWithErrorPromise rather than expect(...).rejects: the first CI run hung both of them on the two Windows lanes only, and bisecting on a Windows box showed that expect(promise).rejects / .resolves does not deliver a child's IPC messages while it waits, even with the unmodified harness from main (a working write wrapped in .resolves hangs the same way), and dev.client() / dev.write() resolve from the fixture's IPC acks. That is a bun:test bug on Windows, reported separately with a standalone repro; with the matcher avoided, hot.test.ts passes on Windows (13 pass, 1 todo, 10s).

This is a test-infrastructure change with no src/ diff, so there is no fail-before build to show; the checks above were done by reverting the corresponding fixture hunks. If #37863 lands first, these belong in the harness.test.ts it adds.

Verification

Whole test/bake (24 files): 186/186 on main, 189/189 here with the release binary; with a debug + ASAN build everything passes except the same production.test.ts cases that also exceed their 5s default timeout on main in this container. ssg-4 passed 25/25 in a loop after the expectElemText change. Single runs on a shared box, release binary, so the numbers move around by a few seconds between runs:

file main this PR
dev/bundle.test.ts 43.6s 16.6s (11.0s on a quieter run)
dev/css.test.ts 53.7s 17.2s
dev/hot.test.ts 53.3s (11 tests) 15.1s (14 tests)
dev/html.test.ts 15.8s 5.0s
dev/esm.test.ts 14.1s 7.5s
dev/react-spa.test.ts 14.0s 5.3s
dev/ssg-pages-router.test.ts 38.3s 21.5s
all of test/bake 277s 138s

test/expected-durations.json is left for the scheduled job to regenerate. Local runs were done under a throwaway uid because root's inotify instance budget on this host is shared with other containers and intermittently makes every dev server fail to start with EMFILE; that affects main identically and is unrelated to this change.

…nnect and write

expectErrorOverlay([]) polled the overlay 5 times with 200ms sleeps and only
stopped early when an overlay was visible, so every dev.client() and every
dev.write()/patch()/delete() with a client connected paid about a second in
the common no-error case. Build errors are on the page by the time the check
runs, so check once; keep the short poll only when errors are expected, since
runtime errors are remapped through /_bun/report_error before they render.

The sleep was also covering for synchronization the harness did not do:

- client-fixture.mjs acked the errors frame immediately, but the dev server
  always follows it with a hot update while an HMR page is connected, so a
  write that fixed an error resolved before the update was applied. Pages
  running the HMR runtime now ack only the hot update.
- A page that called location.reload() kept acking (bun:afterUpdate from the
  abandoned window, then the new window on connect), leaving a straggler ack
  for the next write. Acks are now tied to a window generation; a reloaded
  window's acks are dropped and the new window acks once it has connected.
- dev.client() and Client.hardReload() returned on the "socket connected"
  log line; they now also wait for the fixture's connect ack, which is sent
  after stylesheets have loaded.
- A failed expectErrorOverlay left Dev.batchingChanges set, so later writes
  in the same test went unsynchronized.
- ssg-pages-router "hot reload on page changes" asserted the DOM right after
  a server-side route reload, which the framework applies asynchronously;
  it now waits for the DOM via the new Client.expectElemText().

hot.test.ts gains tests for the unexpected-error rejections and for the
write/reload ack ordering the fixture now guarantees.
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The bake test harness now coordinates page-load completion through HMR and client acknowledgements. The client fixture tracks window generations and defers acknowledgements until updates execute. Tests cover build errors, recovery, reload completion, and asynchronous DOM updates.

Changes

Bake harness synchronization

Layer / File(s) Summary
Generation-aware client acknowledgements
test/bake/client-fixture.mjs
The fixture scopes acknowledgements to window generations and handles HMR, JavaScript, and CSS completion frames.
Page-load and overlay synchronization
test/bake/bake-harness.ts
The harness waits for HMR and client page-load signals, restores batching state after failures, waits for DOM text, and validates error overlays.
HMR error and reload regression coverage
test/bake/dev/hot.test.ts, test/bake/dev/ssg-pages-router.test.ts, test/bake/dev-and-prod.test.ts
Tests cover unexpected build errors, recovery, delayed execution, fallback reloads, asynchronous DOM updates, and updated HMR event semantics.

Possibly related PRs

  • oven-sh/bun#37878: Both PRs modify HMR coverage in test/bake/dev/hot.test.ts, but address different behavior.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the primary change: reducing error-overlay polling in the bake test harness.
Description check ✅ Passed The description explains the problem, fix, synchronization changes, tests, and verification results in substantial detail.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 3:05 PM PT - Aug 12th, 2026

@robobun, your commit c31a9b2 has 2 failures in Build #93428 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 37866

That installs a local version of the PR into your bun-37866 executable, so you can run:

bun-37866 --bun

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced the fixed cost on main with USE_SYSTEM_BUN=1 bun test test/bake/dev/bundle.test.ts (43.3s for 21 tests, connect-only cases ~1.5s each, "commonjs forms" 9.0s; 32 expectErrorOverlay([]) calls at ~1s each). With this branch the same file takes 11-17s depending on machine load, and all of test/bake goes from 277s to 138s with the release binary; the full directory also passes with a debug + ASAN build.

The PR description lists the four synchronization gaps the poll had been covering (errors-frame ack, acks from reloaded windows, connect returning before stylesheets loaded, and the asynchronous route reload in ssg-4) and the tests that pin each of them. Nothing outside test/bake is touched, so there is no fail-before build for this one; the fixture hunks were verified by reverting them individually against the new tests.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this and didn't find any bugs. It's a substantive rework of the bake harness's cross-process ack/synchronization logic (window-generation invalidation, which HMR frames ack, connect now waits on the IPC ack rather than just the log line), so a human look and green CI across all lanes would still be worthwhile before landing.

What was reviewed:

  • expectErrorOverlay restructure — no-errors path now checks once, errors-expected path still polls; both branches share one get-errors round trip and the empty case is covered by toEqual([...errors].sort()).
  • waitForPageLoad registration ordering — listener is attached synchronously before the load starts in both dev.client() and hardReload(), so the ack cannot be missed; the retry loop in dev.client() still surfaces real build errors via expectErrorOverlay (verified against the new "dev.client rejects on build error" test).
  • client-fixture.mjs generation gating — reset() bumps the counter and createWindow captures it, so an abandoned window's bun:afterUpdate / immediate / CSS-check acks are all dropped; the "e"-vs-"u" ack condition matches the described finalize_bundle frame ordering.
  • batchChanges dispose now clears batchingChanges in finally, which the new rejecting-write test depends on.
Extended reasoning...

Overview

This PR removes a fixed ~1s poll from Client.expectErrorOverlay() in the bake test harness and replaces the timing slack it was providing with explicit synchronization. Four files touched, all under test/bake/, no src/ changes:

  • bake-harness.ts: expectErrorOverlay checks once (polls only when errors are expected), new waitForPageLoad() waits for both the connect log line and the fixture's IPC ack, new expectElemText() uses a MutationObserver for framework-driven async DOM updates, and batchChanges dispose is wrapped in try/finally so a throwing overlay check doesn't leave batchingChanges set.
  • client-fixture.mjs: adds a windowGeneration counter so acks from an abandoned window are dropped after a reload, and changes which WS frame acks a build (HMR-runtime pages ack only "u", the error page acks "e") so a fix-after-error write resolves on the update rather than the errors-cleared frame.
  • hot.test.ts: three new harness-behavior tests and drops the now-unneeded errors: null from the existing ack-timing test.
  • ssg-pages-router.test.ts: switches one assertion to expectElemText to wait for React's async commit.

Security risks

None. This is test-harness code with no production surface; no auth, crypto, or untrusted-input handling is touched.

Level of scrutiny

Medium-high. Although it is test-only, the harness is load-bearing for the entire test/bake suite (24 files, ~189 tests), and the change replaces a forgiving fixed sleep with tight ordering assumptions about HMR frame sequencing (finalize_bundle publishes "e" before "u", will_hear_hot_update guarantees a trailing "u" for runtime pages) and cross-process IPC ack timing. If any of those assumptions don't hold on a particular platform (Windows watcher/IPC ordering, macOS), it would flake the whole suite rather than one test. This is not a simple/mechanical change.

Other factors

  • The PR description is unusually thorough — each removed sleep is traced to the specific signal it was standing in for, and each new test's fail-before was checked by reverting the corresponding fixture hunk.
  • The full test/bake suite was reported green (189/189) on the release binary and on debug+ASAN modulo pre-existing production.test.ts timeouts; ssg-4 was looped 25× after the expectElemText change.
  • I traced listener-registration ordering for waitForPageLoad in both call sites and confirmed the ack cannot arrive before the once handler is attached; I also confirmed the dev.client() retry loop still lets genuine build errors through (they surface at expectErrorOverlay([]), not waitForPageLoad, so they aren't retried away).
  • CI (#93330) was still building at review time, and given the cross-platform timing sensitivity a human should confirm all lanes are green before merging.

On Windows, expect(promise).rejects / .resolves does not deliver a child's
IPC messages while it waits on the promise, and dev.client() / dev.write()
resolve from the client fixture's IPC acks, so the two new tests hung there.
Use runWithErrorPromise instead.
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

The first CI run failed only in hot.test.ts, only on the two Windows lanes: the two new rejection tests hung at dev.write() / the test timeout, and the reload test that follows failed as collateral of the timeout's process cleanup. Reproduced on a Windows machine and bisected: the trigger is wrapping the harness call in expect(...).rejects. On Windows that matcher (and .resolves) does not deliver a child's IPC messages while it waits, and dev.write() resolves from the client fixture's IPC ack; the same hang happens with the unmodified harness and fixture from main, and even for a working write wrapped in .resolves, so it is independent of this PR's changes. Reported separately with a standalone Bun.spawn + expect().resolves repro.

a73c091 switches the two tests to runWithErrorPromise; hot.test.ts now passes on Windows locally (13 pass, 1 todo, 10s) and on Linux. The remaining failures in build 93330 (child_process_ipc_handle, inspect-error-leak, sourcetextmodule-leak, test-cluster-shared-leak, ctrl-c, solc) all passed on retry or alone and are unrelated to test/bake.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this and found no bugs. It's a well-reasoned but non-trivial rework of the bake harness's write/connect synchronization (window-generation acks, "e" vs "u" frame ordering, waitForPageLoad, expectElemText), so a maintainer familiar with finalize_bundle's frame ordering should give it a look before it becomes the foundation for the whole test/bake suite.

What was reviewed:

  • expectErrorOverlay restructure — empty-errors path now single check, non-empty still polls; both branches share one get-errors round trip and suppressInteractivePrompt is restored in finally.
  • client-fixture.mjs ack changes — generation counter drops stale-window acks (bumped in both reset() and createWindow()); HMR-runtime pages ack only "u", error page still acks "e"; matches the will_hear_hot_update gate in DevServer.rs.
  • batchChanges dispose now clears batchingChanges in finally, which the new "rejects then continues" test depends on.
  • New tests use runWithErrorPromise (exists in test/harness.ts) instead of .rejects, per the recent Windows IPC-delivery fix.
Extended reasoning...

Overview

Test-infrastructure-only change to test/bake/ (no src/ diff). Removes the fixed 5×200ms poll in Client.expectErrorOverlay for the no-errors-expected case and replaces the four synchronization gaps that poll had been absorbing with real signals: (1) fixture acks the last frame of a build ("u" for HMR-runtime pages, "e" for the error page) instead of both; (2) a per-window generation counter drops acks from abandoned windows after location.reload(); (3) dev.client()/hardReload() now wait for the fixture's page-load ack (post-checkCSSLoaded) via new waitForPageLoad(), not just the log line; (4) new expectElemText uses a document-level MutationObserver for the one test that reads DOM after an async React route-reload commit. Also wraps batchChanges dispose in try/finally so a rejected write doesn't leave the harness batching. Three new hot.test.ts cases pin each fixture change; ssg-4 switches to expectElemText.

Security risks

None. Pure test-harness code; no auth, crypto, network exposure, or user-facing surface.

Level of scrutiny

Medium-high. While it's test-only and can't break the runtime, it rewires the synchronization primitives that every test/bake dev test's dev.write()/dev.client() depends on. The core assumption — that finalize_bundle always publishes a "u" frame after "e" whenever a HotUpdate subscriber is connected — is the linchpin of the "ack only 'u'" change; I checked will_hear_hot_update in src/runtime/bake/DevServer.rs and it gates the hot-update publish on num_subscribers(HmrTopic::HotUpdate) > 0, which matches the PR's claim, but a maintainer who owns that path should confirm it holds for every build outcome (e.g., a build that only clears errors with no client-graph delta). The PR also explicitly gives up detecting an async runtime error that surfaces within ~1s of the last write with no later write to observe it — a small semantic change to what the harness catches that a human should sign off on.

Other factors

  • Bug hunter found nothing; my own read turned up no issues.
  • The one other consumer of received-hmr-event outside the harness (test/bake/dev-and-prod.test.ts) uses it as an idempotent rewrite trigger, so fewer emissions per build is harmless there (its comment about "every 'u'/'e' frame" is now slightly stale but not load-bearing).
  • waitForPageLoad() in the dev.client() path registers its received-hmr-event listener after the subprocess spawns, but the gap is a handful of synchronous statements vs. Node startup + happy-dom import + fetch + bundle eval + WS connect, and waitForLine still scans buffered output for its half; not a practical race, and 189/189 across release and debug+ASAN supports that.
  • Verification is thorough (full test/bake on release and debug+ASAN, ssg-4 25/25 in a loop), and each fixture hunk was fail-tested by reversion against its new test.

This does not meet the "simple, mechanical, or obvious" bar for auto-approval — it's a careful synchronization refactor with cross-platform flake risk if any ordering assumption is off.

…t.ts

The fixture now sends it when a page's socket has connected and once per
applied hot update, not for every errors frame.
@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Two follow-ups from the review above:

  • The "every build that sends e also sends u" invariant, stated against the code: the errors frame is only published from index_failures() (DevServer.rs, the two publish(HmrTopic::Errors, ...) calls), whose only caller is finalize_bundle, which after that point has no early return other than ? error propagation before the unconditional if will_hear_hot_update { ...; publish(HmrTopic::HotUpdate, ...) } block. That block does not depend on whether the client graph changed: with nothing to send it still publishes the empty payload (route lists terminated, CSS count 0), which is exactly what the failing write in the new dev.write rejects... test receives, and what I captured with the fixture's --verbose-web-sockets on both Linux and Windows: e then a 17-byte u. A change that produces no bundle at all (the ResultDidNotBundle / SeenFiles-only paths) publishes neither frame, and the harness resolves those waits from the sync frame alone, as before. An error page is only subscribed to e, so it acks e, also as before.

  • dev-and-prod.test.ts still works with fewer acks (it needs an event after a reconnect and after each applied update, and it passed in the release and debug runs), but its comment did describe the old per-frame behaviour; f0dbb55 updates it. That push also re-runs CI, since build 93388 failed before running anything: five build-bun jobs could not download c-ares, mimalloc, lol-html and the WebKit tarballs from GitHub.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/bake/bake-harness.ts`:
- Around line 917-932: Update waitForPageLoad to clean up both the
received-hmr-event and exit listeners when output.waitForLine or the combined
wait fails. Ensure the failure path removes onAck and onExit and settles the
acked promise, while preserving the existing success and client-exit cleanup
behavior.

In `@test/bake/client-fixture.mjs`:
- Around line 131-140: Update the pending build acknowledgement logic in the HMR
event-handling block to capture queued-script state per update rather than
relying on the shared hmrScriptQueued flag. Ensure each deferred callback checks
the state associated with its own frame, so a later CSS-only update cannot
acknowledge a preceding JavaScript update before its Blob evaluates.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5e397fa5-87b8-441a-b271-90a53cc9529d

📥 Commits

Reviewing files that changed from the base of the PR and between 9a543cc and f0dbb55.

📒 Files selected for processing (5)
  • test/bake/bake-harness.ts
  • test/bake/client-fixture.mjs
  • test/bake/dev-and-prod.test.ts
  • test/bake/dev/hot.test.ts
  • test/bake/dev/ssg-pages-router.test.ts

Comment thread test/bake/bake-harness.ts
Comment thread test/bake/client-fixture.mjs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR and didn't find any bugs. Because it reworks the ack/synchronization contract between the bake harness, the client fixture, and the dev server's HMR frame ordering — subtle logic that underpins ~24 test files — a human familiar with the bake/HMR subsystem should confirm the stated invariants before merge.

What was reviewed:

  • expectErrorOverlay restructure: single-check path for empty errors is sound given the fixture now acks builds only after the runtime handled the errors frame; poll retained for the non-empty case.
  • Window-generation tracking in client-fixture.mjs: reset() bumps the generation for both permitted and denied reloads, and createWindow captures its own generation, so stale windows cannot ack — checked against the reload-triggers-write test.
  • waitForPageLoad listener cleanup (c31a9b2) covers the line-wait timeout path; batchChanges now clears batchingChanges in finally, which the new rejection test depends on.
  • expectElemText MutationObserver on document (not documentElement) with timeout cleanup on both paths.
Extended reasoning...

Overview

Test-infrastructure only (no src/ diff). The core change removes a fixed 1-second poll in Client.expectErrorOverlay() that ran on every dev.client() and every dev.write() with a connected client, cutting test/bake wall-clock roughly in half. Removing the sleep exposed four synchronization gaps the poll had been absorbing, each replaced with the actual signal it was standing in for: (1) the fixture now acks a build once, on the last frame the page receives (u for HMR pages, e for the error page), so a write that fixes an error resolves only after the update behind it applied; (2) acks carry a window generation so abandoned-then-reloaded windows cannot satisfy a later write's wait; (3) dev.client() / hardReload() wait for the fixture's page-load ack (sent after stylesheets loaded) rather than just the log line; (4) a new expectElemText() helper polls via MutationObserver for framework-side async DOM commits. Three new tests in hot.test.ts pin each fix, and ssg-pages-router.test.ts uses the new helper for the one test that was reading the DOM before React committed.

Security risks

None. This is test harness code that spawns local subprocesses and talks to a local dev server over IPC and WebSocket. No auth, crypto, or user-facing surface touched.

Level of scrutiny

Medium-high. Although test-only, this reworks the synchronization contract that every bake dev-server test relies on. The correctness argument depends on invariants about finalize_bundle's frame publication order in DevServer.rs (errors frame before hot-update frame, and will_hear_hot_update always sending u while an HMR page is subscribed). The PR description traces these through the code and the author verified them with --verbose-web-sockets on both Linux and Windows, but a subtle mismatch would surface as intermittent flakiness across the whole test/bake directory rather than a clean failure. That, plus the fact that the previous 1s poll was masking four separate ordering bugs, suggests a maintainer who owns bake/HMR should sanity-check the invariants.

Other factors

  • Both CodeRabbit findings were resolved: the waitForPageLoad listener-cleanup nit was applied in c31a9b2; the per-frame-ack concern was reasoned through (harness serializes builds via batchChanges, so overlapping frames cannot occur) and withdrawn.
  • The Windows expect().rejects IPC-delivery hang was worked around with runWithErrorPromise and reported separately; the workaround is commented at the call site.
  • Full test/bake verified on release and debug+ASAN; ssg-4 passed 25/25 in a loop after the observer fix.
  • Remaining CI failures in the latest build (complex-workspace install, Android/musl build-bun) are unrelated to test/bake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant