Conversation
A page load fetches the HTML, then the client bundle, then opens the HMR socket and subscribes to hot updates. A rebuild that finishes between the last two steps publishes its hot update to nobody, and nothing told the page afterwards that its bundle was stale. A truncate-then-write save (open(O_TRUNC), write ~15ms later) of a module without an import.meta.hot.accept boundary hits this window: the first build (empty file) reloads the page, the second build lands while the page reloads, and the page keeps running the empty module forever. The client now always sends the init frame with its bundle generation after subscribing. The server replies with a new FullReload frame when no route bundle has that generation any more, and the client reloads.
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Comment |
|
Status: ready for review. Reproduced two ways before the fix, both on 1.4.3-canary (f42e980):
CI (build 113107): 180 of 181 jobs passed, the new tests passed on every lane. The one red job is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is well-scoped and the tests are solid, but since it adds a protocol message and changes i to fire on every route (shifting source-map refs from weak-with-sweep to socket-held for framework routes), a quick look from someone with bake dev-server context would be worthwhile.
What was reviewed:
- The
Inithandler'sroute_bundles.iter().any(...)scan — no unwraps,let _on send, runs afterSubscribeso the race window is closed as described. - Client change:
iis now unconditional; confirmedfullReload()andsendBufferedare already in scope and thebun:loadDatacleanup is preserved. - Tests:
hmrHandshakewiresoncloseto reject, asserts exact frame arrays for both stale and current generations; the proxy test usesport: 0,await using, andPromise.withResolversgating instead of sleeps. Noted the helper sendsibeforen(opposite of the real client) so thenreply bounds the collected frames — the second test covers the real client order end-to-end.
Extended reasoning...
Overview
This PR fixes an HMR staleness race in the bake dev server: a page that fetches its client bundle, then has the route rebuilt before its /_bun/hmr socket subscribes, would never receive the missed hot_update and stay stale forever. The fix adds MessageId::FullReload (b'R') in src/runtime/bake/dev_server/mod.rs, has the Init handler in hmr_socket.rs compare the client-reported generation against every route_bundle.client_script_generation and reply R on mismatch, makes hmr-runtime-client.ts send i<generation> unconditionally after Subscribe/set_url and handle full_reload by calling fullReload(), and adds two tests to test/bake/dev/html.test.ts (a raw-socket handshake replay asserting ["V","R","n"] vs ["V","n"], and a proxy that parks the WS upgrade until after a rebuild to verify the real client reloads).
Security risks
None. This is a local dev-server hot-reload signal — a single-byte binary frame over the existing /_bun/hmr WebSocket that triggers location.reload(). No auth, crypto, filesystem, or network-boundary parsing is touched; the generation is already parsed and bounds-checked by the pre-existing msg.len() != 9 / hex-decode guards above the added lines.
Level of scrutiny
Moderate. The Rust change is ~12 lines with no new unsafe, no allocation, and no new error paths — it reuses the dev reference already obtained for the source-map upgrade and does a linear scan over route bundles. The client change is small but has a documented side effect: i now fires for framework routes too, which upgrades their source-map weak ref to a socket-held ref instead of letting it expire on the sweep timer. The PR description calls this out and it looks benign (released on socket close), but that's the one behavioral shift I'd want a bake maintainer to nod at rather than rubber-stamp.
Other factors
Test quality is high by the repo's own bar: exact toEqual frame assertions covering both the stale and current case, onclose wired to reject, no sleeps or timing assumptions (all sequencing via Promise.withResolvers), port: 0, await using for the proxy and client, and placement in the existing test/bake/dev/html.test.ts. The hmrHandshake helper deliberately sends i before n (inverting the real client's order) so the n reply serves as a barrier proving i was handled — the PR notes the server-side check is order-independent by design, and the second proxy-based test exercises the real client's actual order. No CODEOWNERS cover these paths, no prior reviewer objections exist on the timeline, and the bug-hunt exited on dry_streak with nothing found.
This branch has not been deployed
2 participants
Problem
open(O_TRUNC), thenwrite~15 ms later) of a module with noimport.meta.hot.acceptboundary can leave the connected dev-server page permanently stale. It keeps running the bundle built from the empty intermediate file (TypeError: x is not a function). A fresh tab is correct.finalize_bundlepublisheshot_updateto current subscribers only, and the handshake never told the page its bundle was outdated.Fix
i(init) frame withconfig.generationafter it subscribes. TheInithandler (dev_server/hmr_socket.rs) replies with a newMessageId::FullReload(R) frame when no route bundle has that generation any more, and the client reloads.on_js_requestonly serves a current generation and onlyinvalidate_client_bundleretires one. The check runs afterSubscribeon the same socket, so each rebuild reaches the page as this reply or as ahot_update.test/bake/dev/html.test.ts(two new tests, both fail on 1.4.3-canary), plus the rest oftest/bake/.Background
client_script_generationis a randomu32per route bundle, regenerated byinvalidate_client_bundlewhen a build changes code the route reaches. It is part of the script URL and isconfig.generationin the bundle.on_js_requestanswers an outdated script URL with alocation.reload()stub.iframe already existed for source-map ref counting, on HTML routes only.Notes
cp, shell>redirection and several editors save this way. A page load is three steps: fetch the HTML, fetch the client bundle it names (/_bun/client/{name}-{rbi}{generation}.js), open/_bun/hmrand sendshe(subscribe),n(set_url),i(init).TypeError: import_f2.f2 is not a function), silently wrong values (the empty module has no exports), and a blank document whenindex.htmlitself was saved this way. All three are the page running the bundle of build 1 and never hearing about build 2./_bun/hmrand sendshe,n/,iA. The server answersV,nand nothing else. With this change it also answersR.test/bakehappy-dom client and a realopen(O_TRUNC)/ sleep /writesweep over gaps of 0 to 100 ms: 1.4.3-canary gets stuck at the 8 ms gap on 3 of 3 runs (#outkeeps the value from the empty module). With this change every gap converges, through one extra reload when the stale bundle was loaded. That sweep is timing-dependent, so the committed tests force the window instead: one replays the handshake on a raw socket, the other puts a proxy in front of the dev server that parks the page's/_bun/hmrupgrade until the rebuild has landed.dev.route_bundlesinstead of reading only the socket'sactive_route. It then does not depend on the order ofnandi, and does not misfire whenlocation.pathnamemaps to a different route bundle than the one whose script the page loaded (apushStatebefore the socket connected). A false "current" verdict needs au32collision between two route bundles.SourceMapStorealready keys route bundles by the generation alone (generation << 32).sandiof one handshake, the page receives both thehot_updateandRand reloads once more than needed. The outcome is still a current page.ion every page instead of HTML routes only means the source-map weak ref of a framework route's bundle is upgraded to a socket-held ref (released on close) instead of expiring on the sweep timer.hmr-runtime-error.ts) reloads only when aneframe empties its error list, and can miss theeframe of the build that fixed the error; it has no bundle generation and needs its own resync (tracked separately). A Bake framework route whose server code is rebuilt in the same window gets noReither, because a server-only rebuild does not callinvalidate_client_bundle; the page keeps the SSR output it was served until the next change.test/bake/dev-and-prod.test.ts("hmr handles rapid consecutive edits", test(bake): re-write rapid-edits sentinel across full reloads #33981) re-writes its sentinel file on every reload because of this exact window. With this change the page would recover throughRanyway. Trimming that workaround is a follow-up; it needs a Windows run.test/js/bun/http/bun-serve-html.test.ts,test/cli/inspect/BunFrontendDevServer.test.tsandtest/bake/deinitialization.test.ts.src/runtime/bake/generated.ts(the TypeScriptMessageIdenum) is generated fromdev_server/mod.rsbysrc/codegen/bake-codegen.ts, so the new variant needs no manual TS edit.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file