Repository navigation
bake: unsubscribe the HMR socket from topics that a subscribe frame drops - #43995
Conversation
…rops The unsubscribe branch of the Subscribe handler repeated the condition of the subscribe branch, so it never ran. A socket that sent a topic set without a topic it had before kept the uWS subscription for that topic and still received its frames, while HmrSocket.subscriptions said that it was unsubscribed.
|
Status: ready for review. How I reproduced the bug:
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. WalkthroughThe HMR socket now unsubscribes from topics removed from the updated subscription set. A new integration test changes the subscribed topics and checks which published frames the socket receives. ChangesHMR topic subscriptions
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Removed HMR topics no longer receive their frames, and the added protocol test checks that behavior across subscription changes. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — the one-line condition flip is the intended !new && old unsubscribe, and the existing on_unsubscribe(!new_bits & self.subscriptions) call on the next line already balances the visualizer counters and timer, so the new branch needs no matching hooks. The inline note on the test's fetch error path is a diagnostics nit, not a correctness concern.
Extended reasoning...
The change flips a duplicated else if condition in the Subscribe frame handler of src/runtime/bake/dev_server/hmr_socket.rs so uWS is actually unsubscribed from dropped topics, deletes the stale "likely a bug" comment, and adds one test.concurrent case to test/bake/hmr-socket-protocol.test.ts. It touches the dev-server HMR WebSocket (peer bytes), but only makes an existing unsubscribe call reachable; no auth, crypto, or parsing surface changes. I confirmed on_unsubscribe at hmr_socket.rs:295 already decrements emit_incremental_visualizer_events/emit_memory_visualizer_events and removes the memory-visualizer timer for dropped bits, so debug-feature bookkeeping is balanced without additional code in the new branch. The fix is a one-line, self-evidently correct change with a regression test wired to reject on server exit; the single surviving finding concerns test failure diagnostics only.
The page request and its body read now reject with their own error, and they race the process exit and the socket close. Each rejection carries the stderr of the dev server. Before, a failed request waited for the process exit, so a request that failed while the dev server stayed alive ended as a timeout with no cause.
|
Updated 1:33 PM PT - Sep 25th, 2026
✅ @robobun, your commit 1cede5667f7d58100ebd662367c6421677c022ca passed in 🧪 To try this PR locally: bunx bun-pr 43995That installs a local version of the PR into your bun-43995 --bun |
There was a problem hiding this comment.
LGTM — the one-line polarity fix is correct, and the follow-up commit resolves the fetch-error propagation I raised earlier (orFail now races each awaited step against failed and rethrows the fetch's own error with stderr attached).
What was reviewed:
- The inverted
else ifat src/runtime/bake/dev_server/hmr_socket.rs:149 now matches the dropped-bit set thaton_unsubscribe(!new_bits & self.subscriptions)on line 153 already accounted for, so uWS and dev-server bookkeeping agree; grepped for the same pattern elsewhere in src/ — only this site. - Checked the test's SetUrl round-trip barrier against vendored uWS:
WebSocket::senddrains the subscriber's pending topic publishes before the direct frame, and unsubscribing the last topic drains before freeing the subscriber, so thenreply cannot overtake earlier published frames. - Test conventions:
tempDir,port: 0,test.concurrent,await using proc/using hmrregistered before assertions, stdout/stderr drained concurrently, process exit and socket close both reject the awaited promise, exacttoEqualon the delivered frame sets.
Extended reasoning...
The change touches one condition in the HMR socket subscribe-frame handler (src/runtime/bake/dev_server/hmr_socket.rs) and adds one concurrent test case to the existing test/bake/hmr-socket-protocol.test.ts. It handles peer-supplied WebSocket bytes but only flips which bit-set difference triggers ws.unsubscribe, with no new parsing, allocation, or trust boundary. The fix is self-evidently correct against the neighboring on_unsubscribe call, the new test exercises both the drop and re-subscribe transitions, the prior inline finding was addressed by the second commit, and neither file is covered by CODEOWNERS. No third-party review requested changes.
Problem
/_bun/hmrsocket that drops a topic still receives it. Aftersh, thens, three edits give["u","u","u"], not[].hsubscriber, the next bundle aborts a debug build:panic: assertion failed: ref_count > 0. A release build leaks the source map entry.src/runtime/bake/dev_server/hmr_socket.rs:149. Theelse ifthat callsws.unsubscriberepeats the condition of itsif. No user reported this.Fix
!new_bits.contains(bit) && self.subscriptions.contains(bit).on_unsubscribefor the dropped bits and overwritessubscriptions.test/bake/hmr-socket-protocol.test.tsfails on main and passes here. Also rantest/bake/dev/hot.test.ts.Background
/_bun/hmris the WebSocket of the dev server. The framesplus topic letters replaces the topic set of a socket. Topichdelivers hot updates (u).HmrSocket.subscriptionsis the dev server's copy.finalize_bundlecounts source map references by the copy.Downsides
HmrSocket::on_messagegrows by 37 bytes (4945 to 4982, linux-x64 release). The file size does not change.Notes
Reach
src/bake/DevServer/HmrSocket.zig, lines 74 and 96, before Rewrite Bun in Rust #30412).hmr-runtime-client.tssendssheonce,hmr-runtime-error.tssendsseonce, the test harness sendssronce. The bug affects hand-written clients and tests.REVIEW.md: a failure that network bytes can reach must not be a panic.Conditions for the abort
h, and no other socket has thehbit. A browser tab sendsshe, so a connected tab prevents the abort. The extra frames remain.Script for the extra frames
["u","u","u"]["u","u","u"]panic: assertion failed: ref_count > 0[]shrthensr["r","r","r"](the kept topic still arrives)Control: a socket that never sends
shgets[]on each build.Source map entries on a release build
put_or_increment_ref_countstores the entry withref_count0. The only removal path isunref_at_index, and no socket owns the entry.sh, thensshsh, thensshsh, thensThe test
hr,r, none,hr. Each step requests a page that is not bundled yet. That starts exactly one bundle, and the response arrives after the dev server published the frames of that bundle.nplus a route) opens and closes the frame window of a step. The server answers on the same socket, after each frame that it published to that socket before.rstep receives["r","u"]and the step with no topic receives["r","u"]. Result on main, debug build: the dev server aborts with the panic above.Cost for a client that never drops a topic
ws.unsubscriberuns only for a topic that the frame drops.nm -Sandsize -Aonbun-profile, release builds of 29d9638 and of this branch. File size: 80995912 bytes for both.bloatyis not installed.Possible follow-up: a lint for this pattern
same_functions_in_if_conditionreports anelse ifthat repeats the call of itsif. The default lintifs_same_conddoes not look at conditions with calls.cargo clippy --workspace --no-deps --keep-going -- -D clippy::same_functions_in_if_conditiongives one error on main (this line) and no error with this change (clippy 0.1.100, linux-x64).[workspace.lints.clippy]changes the rustc flags of each workspace crate, so each crate builds again once.Relation to other PRs
on_unsubscribecounter) is on main through Stop HMR websocket frames from aborting or crashing the dev server #33196.hmr_socket.rs:133on its head) but has conflicts since 2026-08-28.Mframe each second. A socket that droppedMthen keeps receiving that frame while another socket holdsM. This change stops that. A test merge of the two heads (this branch and 95ee86a) has no conflict.[human-review] gate passed · iteration 0 · 2 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