Stop HMR websocket frames from aborting or crashing the dev server - #33196
Conversation
|
Updated 4:43 AM PT - Sep 11th, 2026
❌ @robobun, your commit 03f585b has 1 failures in 🧪 To try this PR locally: bunx bun-pr 33196That installs a local version of the PR into your bun-33196 --bun |
WalkthroughUpdates HmrSocket.rs to use compile-time feature gating for hook bookkeeping and removes a duplicate-frame assert. Adds a Bun test file that drives HMR websocket frames against a dev server and checks close behavior and server liveness. ChangesHMR websocket protocol
Sequence Diagram(s)sequenceDiagram
participant Test
participant DevServer
participant HmrSocket
Test->>DevServer: spawn process, watchDevServer parses PORT
Test->>HmrSocket: connectHmr websocket to /_bun/hmr
Test->>HmrSocket: send "sM" then "n/"
HmrSocket->>Test: frame ids ["V","n"]
Test->>DevServer: fetch / (expect 200)
Test->>HmrSocket: send "H" then "H" again
HmrSocket->>Test: websocket close
Test->>DevServer: release bundle gate, pending request resolves 200
Estimated code review effort: Medium Related issues: None found in the provided summary. Related PRs: None found in the provided summary. Suggested labels: bake, dev-server, tests Suggested reviewers: None found in the provided summary. 🎨 Finishing touches 🧠 Learnings used: None found in the provided summary. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hmr-socket-protocol.test.ts`:
- Around line 119-171: The test currently awaits bundleEntered without any
failure path, so it can hang indefinitely if the bundle never reaches the plugin
hook. Update the hmr-socket-protocol test around bundleEntered to reject on dev
server/proc failure events such as error, close, abort, or process exit,
following the same pattern used by gotSetUrlResponse and closed. Make sure the
promise used in the fetch/onLoad gate is wired to fail fast with a useful
message instead of relying on the default timeout.
🪄 Autofix (Beta)
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: b8e637c3-3d62-450e-8c55-4eb73a2bcb7c
📒 Files selected for processing (2)
src/runtime/bake/DevServer/HmrSocket.rstest/bake/hmr-socket-protocol.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — thanks for also folding in the memory-counter fix in f2be9dd.
Extended reasoning...
Overview
Two files changed. In src/runtime/bake/DevServer/HmrSocket.rs: (1) the on-subscribe and on-unsubscribe visualizer hooks now gate on cfg!(feature = "bake_debugging_features") instead of the runtime feature_flags::BAKE_DEBUGGING_FEATURES const, aligning them with every other gate in the visualizer subsystem; (2) the debug_assert!(false) in the TestingBatchEvents::EnableAfterBundle arm is removed, keeping the existing ws.close(); (3) the pre-existing copy-paste I flagged (memory-visualizer timer disarm checking the incremental counter) is fixed in f2be9dd. test/bake/hmr-socket-protocol.test.ts is a new file with two concurrent tests exercising both frames.
Security risks
None introduced — the change is a strict hardening. The HMR websocket accepts frames from any client that can reach the dev server, and this PR ensures two such frames can no longer reach an assert and kill the process on assertion-enabled builds. The gate change makes the visualizer hook less reachable (the cargo feature is never enabled), and the assert removal converts an abort-on-untrusted-input into the existing socket close. No new attack surface, credentials, or filesystem/network reach.
Level of scrutiny
Low-to-moderate. This is dev-server-only code (never runs outside development: { hmr: true }), and each of the three Rust changes is mechanical: aligning one gate with its siblings, deleting an assert on peer input while keeping the handling, and fixing a wrong-variable copy-paste inside a block that is dead in every shipped build. The PR description traces each predicate to its counterparts elsewhere in the subsystem and explains why the alternative fix (flipping the assert's predicate) would be wrong.
Other factors
The one CodeRabbit comment (wire bundleEntered to a failure path) was addressed in 4fb260d and marked resolved. My prior inline note about the wrong counter — explicitly non-blocking and pre-existing — was addressed in f2be9dd. The new tests follow harness conventions well: tempDir + using, port: 0, subprocess pipes drained concurrently, awaited conditions instead of sleeps (the in-flight bundle is held deterministically via a plugin onLoad gated on a local server), and every awaited promise has a rejection path wired to process exit or socket close so failures surface with the dev server's stderr rather than hanging. No outstanding review comments remain.
There was a problem hiding this comment.
LGTM — minimal, well-reasoned gate alignment and assert removal; both prior review notes addressed.
Extended reasoning...
Overview
Two files: src/runtime/bake/DevServer/HmrSocket.rs (~15 net lines, mostly comments) and a new test file test/bake/hmr-socket-protocol.test.ts. The Rust change (1) swaps two feature_flags::BAKE_DEBUGGING_FEATURES gates for cfg!(feature = "bake_debugging_features") so the on-subscribe/unsubscribe hooks use the same predicate as the visualizer machinery they drive, (2) removes a debug_assert!(false) on the duplicate-H websocket-input path (keeping the existing ws.close()), and (3) fixes the pre-existing wrong-counter check I flagged in my earlier inline comment. The now-unused feature_flags import is dropped.
Security risks
None introduced; the change strictly reduces DoS surface. Before, any client that could open a WebSocket to the dev server could abort an assertion-enabled build with a two-byte frame. The affected code is development-tooling-only (HMR socket), the visualizer paths remain compile-time dead (the cargo feature is never enabled), and release builds were already unaffected since debug_assert! compiles out.
Level of scrutiny
Low-to-medium. The Rust diff introduces no new logic — it aligns one predicate with every other gate in the same subsystem and deletes an assert on peer-controlled input, which is exactly the repo guideline ("a panic on user input is a DoS"). The PR description explicitly justifies why the alternative fix (changing the assert's predicate instead) would be wrong: it would arm memory_visualizer_timer whose callback body is compiled out under the same feature. The watcher path's sibling debug_assert!(false) is intentionally left because it guards a real internal invariant, not wire input.
Other factors
The new tests are well-constructed per repo conventions: tempDir/bunEnv/bunExe, port: 0, test.concurrent, using/await using cleanup, stdout/stderr drained concurrently, and the "bundle in flight" precondition is made deterministic via a plugin that blocks on a controlled fetch (no sleeps). Both prior review threads — CodeRabbit's bundleEntered failure-path wiring and my wrong-counter note — are resolved in the current diff (4fb260d, f2be9dd). The bug-hunting system found nothing on this revision.
There was a problem hiding this comment.
LGTM — small, well-justified defensive fix with solid deterministic test coverage; the one inline note is pre-existing and non-blocking.
Extended reasoning...
Overview
Two files touched. In src/runtime/bake/DevServer/HmrSocket.rs: (1) the on-subscribe and on-unsubscribe visualizer hooks now gate on cfg!(feature = "bake_debugging_features") instead of the runtime const feature_flags::BAKE_DEBUGGING_FEATURES, aligning with the ~30 other gates on the same subsystem in DevServer.rs; (2) the debug_assert!(false) in the TestingBatchEvents::EnableAfterBundle arm is removed, keeping the existing ws.close(); (3) the pre-existing memory/incremental counter mix-up I flagged last run is fixed; (4) the now-unused feature_flags import is dropped. test/bake/hmr-socket-protocol.test.ts is new and covers both frames deterministically (the second holds a bundle open via a plugin onLoad gated on an in-process fetch — awaited, no sleeps).
Security risks
None introduced. The change is strictly defensive: it stops two wire-reachable debug_assert! aborts on the dev-server HMR websocket. The cfg! alignment turns a block that was live on debug/canary builds into dead code everywhere (matching the rest of the visualizer machinery, which is compiled out under the same never-enabled cargo feature), so if anything this reduces reachable surface. Removing an assert on network input follows the repo's own "user-reachable failures are recoverable errors, never panics" rule.
Level of scrutiny
Low-to-moderate. The Rust delta is ~4 substantive lines, each mechanical: a predicate swap verified against the callee's own assert (DevServer.rs:5753), and dropping a debug_assert!(false) on peer input while keeping the close. This is dev-server-only (/_bun/hmr), not a production runtime hot path. I verified the cargo feature is declared in src/runtime/Cargo.toml but never enabled, and that the old const evaluates to IS_CANARY || IS_DEBUG — so the PR's root-cause analysis is correct.
Other factors
All prior review feedback is addressed and resolved: my counter-mismatch note (f2be9dd) and CodeRabbit's bundleEntered fail-fast wiring (4fb260d). The tests follow repo conventions closely (tempDir, bunEnv/bunExe, port: 0, test.concurrent, awaited conditions with failure paths wired to reject, using/await using for cleanup, exit-code-independent liveness assertions). The one inline finding posted this run is on the adjacent Enabled arm — pre-existing, requires filesystem + HTTP cooperation beyond this PR's "any WebSocket client" scope, and is explicitly noted as non-blocking follow-up material.
There was a problem hiding this comment.
LGTM — the SetUrl guard and its test in 1f052e8 cover the last wire-reachable assert I flagged; all prior feedback is addressed.
Extended reasoning...
Overview
Touches src/runtime/bake/DevServer/HmrSocket.rs (~15 changed lines) and adds test/bake/hmr-socket-protocol.test.ts (~250 lines, new). The Rust changes are all defensive tightening of the HMR websocket message handler: (1) align the on-subscribe/on-unsubscribe gate with the cfg!(feature = "bake_debugging_features") predicate the rest of the visualizer machinery already uses, (2) drop a debug_assert!(false) reachable from a duplicate H frame in favor of the ws.close() already there, (3) reject non-absolute SetUrl patterns before they reach FrameworkRouter::match_slow's debug_assert!(path[0] == b'/'), and (4) fix a copy-paste counter check in the (feature-gated, currently dead) memory-visualizer unsubscribe path. The unused feature_flags import is dropped.
Security risks
None. The HMR socket is a local dev-server endpoint; the changes strictly add input validation and remove peer-triggerable asserts. All modified paths only diverge from prior behavior on debug_assertions builds — release builds already compiled every affected assert out. No auth, crypto, filesystem, or privilege boundaries are touched.
Level of scrutiny
Low-to-moderate. The dev server is a complex subsystem, but each individual edit here is mechanical and locally verifiable: a gate-predicate alignment (matching every other gate in the file's callees), an assert deletion that leaves the existing ws.close() handling in place, a one-line input guard mirroring the pattern used by every sibling arm, and a one-identifier typo fix in dead code. The blast radius is bounded to assertion-enabled builds of the dev server.
Other factors
I left three inline comments on earlier revisions. Two were fixed in f2be9dd (counter typo) and 1f052e8 (SetUrl guard + new test.concurrent.each(["n", "nfoo"]) case). The third — the adjacent Enabled-arm race with ensure_route_is_bundled — was explicitly and reasonably deferred: it requires filesystem writes plus HTTP cooperation (outside this PR's "any WebSocket client" scope), and fixing it needs a NextBundle shape change plus its own harness test; I marked it non-blocking myself. CodeRabbit's one comment (wire bundleEntered to reject on process exit) was addressed in 4fb260d. The new tests follow harness conventions closely (tempDir, bunEnv/bunExe, await using on the subprocess, concurrent pipe draining, awaited conditions instead of sleeps, failure paths wired to reject with dev-server stderr). No CODEOWNERS entry covers these paths, and the current bug-hunting pass found nothing.
|
Status. This comment used to describe an earlier revision of the branch (older file paths, and a All three reproduce on
CI on 03f585b (114237): the build is final at 180 of 181 jobs passed, and the diff is green. No Independent pre-merge verification. A fuzz lane drove
Two dev-server aborts they confirmed are still present on that same test-merge build are not in this diff. |
The HMR websocket at /_bun/hmr accepts frames from any client that can reach the dev server. Three frames drove invalid state: - A duplicate `H` (testing-batch) frame before a pending batch activates hit the TestingBatchEvents::EnableAfterBundle arm's debug_assert!(false). The ws.close() already there is the handling. - `SetUrl` passed the peer-supplied pattern into FrameworkRouter::match_slow, whose second line is debug_assert!(path[0] == b'/'). "n" indexes an empty slice inside that assert and "nfoo" fails it. Close the socket instead, like the sibling arms do for their malformed input. - Releasing a batch with `H` called start_async_bundle unconditionally, but a request for a route that is not bundled yet starts a bundle without consulting the batch. That broke start_async_bundle's debug_assert!(current_bundle.is_none()), and on a release build, where the assert is compiled out, the second call overwrote the in-flight CurrentBundle and freed the arena its parse tasks were still reading (a multi-thread segfault). Hold the batch in a new ReleaseAfterBundle state and release it from finalize_bundle_cleanup once no bundle is running. Also fix the memory-visualizer unsubscribe checking the incremental visualizer's counter, a copy-paste from the arm above it.
1f052e8 to
d510625
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
…rops (#43995) ### Problem - A `/_bun/hmr` socket that drops a topic still receives it. After `sh`, then `s`, three edits give `["u","u","u"]`, not `[]`. - With no other `h` subscriber, the next bundle aborts a debug build: `panic: assertion failed: ref_count > 0`. A release build leaks the source map entry. - The cause is at `src/runtime/bake/dev_server/hmr_socket.rs:149`. The `else if` that calls `ws.unsubscribe` repeats the condition of its `if`. No user reported this. ### Fix - The condition becomes `!new_bits.contains(bit) && self.subscriptions.contains(bit)`. - This is correct because the handler already replaces the whole set. It calls `on_unsubscribe` for the dropped bits and overwrites `subscriptions`. - Verified: a new case in `test/bake/hmr-socket-protocol.test.ts` fails on main and passes here. Also ran `test/bake/dev/hot.test.ts`. - Self-reviewed: 10 concerns raised, 9 addressed. Deferred: a clippy lint for this pattern. ### Background - `/_bun/hmr` is the WebSocket of the dev server. The frame `s` plus topic letters replaces the topic set of a socket. Topic `h` delivers hot updates (`u`). - uWS (the WebSocket library) holds the subscriptions and publishes. `HmrSocket.subscriptions` is the dev server's copy. `finalize_bundle` counts source map references by the copy. - No other design was weighed: the cause and the fix are on one line. This is the unsubscribe half of #37878. It merges cleanly with #42547. ### Downsides - A client that dropped a topic no longer gets its frames. No client in the tree sends a second subscribe frame. - `HmrSocket::on_message` grows by 37 bytes (4945 to 4982, linux-x64 release). The file size does not change. <details><summary>Notes</summary> **Reach** - The bug is in all builds, stable included. The Zig version of the handler had the same two conditions (`src/bake/DevServer/HmrSocket.zig`, lines 74 and 96, before #30412). - No client in the tree sends a second subscribe frame. `hmr-runtime-client.ts` sends `she` once, `hmr-runtime-error.ts` sends `se` once, the test harness sends `sr` once. The bug affects hand-written clients and tests. - There is no issue and no user report. The ground for the change is `REVIEW.md`: a failure that network bytes can reach must not be a panic. **Conditions for the abort** - The build has debug assertions (debug, ASAN). - A socket dropped `h`, and no other socket has the `h` bit. A browser tab sends `she`, so a connected tab prevents the abort. The extra frames remain. - The bundle has a source map that the store does not hold yet. - An edit starts such a bundle. The first request for a page that is not bundled yet starts one too. The new test uses that. ``` panic: assertion failed: ref_count > 0 SourceMapStore::put_or_increment_ref_count src/runtime/bake/dev_server/source_map_store.rs:467 finalize_bundle src/runtime/bake/DevServer.rs:4578 ``` **Script for the extra frames** ```js import { mkdtempSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; const dir = mkdtempSync(join(tmpdir(), "elseif-")); process.chdir(dir); writeFileSync(join(dir, "a.html"), `<!doctype html><html><body><script type="module" src="./a.ts"></script></body></html>`); writeFileSync(join(dir, "a.ts"), `console.log("v0");`); const html = (await import(join(dir, "a.html"))).default; const srv = Bun.serve({ routes: { "/a": html }, development: true, port: 0, hostname: "127.0.0.1" }); await (await fetch(`http://127.0.0.1:${srv.port}/a`)).text(); const spin = async ms => { const end = Date.now() + ms; while (Date.now() < end) await new Promise(r => setImmediate(r)); }; const ids = []; const ws = new WebSocket(`ws://127.0.0.1:${srv.port}/_bun/hmr`); ws.binaryType = "arraybuffer"; const ready = Promise.withResolvers(); ws.onmessage = e => { const id = String.fromCharCode(new Uint8Array(e.data)[0]); if (id === "V") { ws.send("sh"); ws.send("s"); ready.resolve(); } else ids.push(id); }; ws.onclose = () => ids.push("<closed>"); await ready.promise; await spin(300); for (let i = 1; i <= 3; i++) { writeFileSync(join(dir, "a.ts"), `console.log("v${i}");`); await spin(700); } console.log("frames after the socket dropped every topic:", JSON.stringify(ids)); process.exit(0); ``` | Build | Result | | --- | --- | | 1.4.2+744846f84 (released) | `["u","u","u"]` | | main 29d9638, release build, canary config | `["u","u","u"]` | | main 29d9638, debug build, one socket | `panic: assertion failed: ref_count > 0` | | this branch, debug build | `[]` | | this branch, debug build, `shr` then `sr` | `["r","r","r"]` (the kept topic still arrives) | Control: a socket that never sends `sh` gets `[]` on each build. **Source map entries on a release build** - In a release build the assert is compiled out. `put_or_increment_ref_count` stores the entry with `ref_count` 0. The only removal path is `unref_at_index`, and no socket owns the entry. - Measurement: one socket, 20 edits, each with a new source map. After the socket closed, I requested the source map URL of each hot update. | Build | Socket sends | Hot updates received | Source maps still served after close | | --- | --- | --- | --- | | 1.4.2+744846f84 (released) | `sh`, then `s` | 20 | 20 | | 1.4.2+744846f84 (released) | `sh` | 20 | 0 | | main 29d9638, release build | `sh`, then `s` | 20 | 20 | | main 29d9638, release build | `sh` | 20 | 0 | | this branch, debug build | `sh`, then `s` | 0 | 0 | **The test** - One socket subscribes to four topic sets in turn: `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. - A SetUrl round trip (`n` plus 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. - The test uses no file watcher and no second socket. An earlier draft used both and failed 2 of 1033 runs under load, because two sockets have no order between them. - Result on main, release build: the `r` step 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. | Setup, debug build of this branch, linux-x64 | Runs | Failures | | --- | --- | --- | | The new case alone | 60 | 0 | | 6 loops of the file beside 15 parallel workers that run other test files | 219 | 0 | | Before 1cede56, the same two setups | 60 and 353 | 0 | | Before 1cede56, 6 copies of the new case in one runner, 4 pinned CPUs, 2 parallel workers beside it | 492 | 0 | - The test is not run on Windows or macOS before this PR. CI is the first run there. - Each awaited promise rejects on its own failure, on the process exit and on the socket close. Each rejection carries the stderr of the dev server. - After a panic, the dev server sends nothing more until the process exits. The crash handler of a debug build needs about 4 s for that. With the default 5 s timeout on a loaded machine, the test can then report a timeout and not the panic text. CI uses 90 s (270 s on the ASAN lane). **Cost for a client that never drops a topic** - Each subscribe frame evaluates the new condition for each of the 6 topics. There is no allocation and no syscall on that path. - `ws.unsubscribe` runs only for a topic that the frame drops. - Sizes are from `nm -S` and `size -A` on `bun-profile`, release builds of 29d9638 and of this branch. File size: 80995912 bytes for both. `bloaty` is not installed. **Possible follow-up: a lint for this pattern** - The clippy lint `same_functions_in_if_condition` reports an `else if` that repeats the call of its `if`. The default lint `ifs_same_cond` does not look at conditions with calls. - `cargo clippy --workspace --no-deps --keep-going -- -D clippy::same_functions_in_if_condition` gives one error on main (this line) and no error with this change (clippy 0.1.100, linux-x64). - The lint is not in this PR. A change to `[workspace.lints.clippy]` changes the rustc flags of each workspace crate, so each crate builds again once. **Relation to other PRs** - #37878 had this change and closed with no maintainer objection, in favor of #39488. Its other half (the `on_unsubscribe` counter) is on main through #33196. - #39488 has the same condition (`hmr_socket.rs:133` on its head) but has conflicts since 2026-08-28. - #40255 rewrites this handler and keeps the old condition. It must take the new condition on its next rebase. - #42547 makes the memory visualizer timer publish an `M` frame each second. A socket that dropped `M` then keeps receiving that frame while another socket holds `M`. This change stops that. A test merge of the two heads (this branch and 95ee86a) has no conflict. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bake/hmr-socket-protocol.test.ts bun test v1.4.3 (367d939) test/bake/hmr-socket-protocol.test.ts: (pass) a duplicate testing-batch frame (H) during an in-flight bundle closes the socket instead of aborting [880.24ms] (pass) a SetUrl frame without a leading slash ("nfoo") closes the socket instead of aborting [776.41ms] (pass) a SetUrl frame without a leading slash ("n") closes the socket instead of aborting [1021.87ms] (pass) releasing a testing batch while another bundle is in flight defers it [1042.66ms] 407 | /** Awaits `promise`. Its failure, a process exit, or a socket close rejects with the dev server's stderr. */ 408 | async function orFail<T>(promise: Promise<T>) { 409 | try { 410 | return await Promise.race([promise, failed.promise]); 411 | } catch (e) { 412 | throw new Error(`${(e as Error).message}\n--- dev server stderr ---\n${dev.stderr()}`, { cause: e }); ^ error: hmr websocket closed (code 1006, reason "Connection ended") --- dev server stderr --- Bundled page in 119ms ... (truncated) release without fix: 1 FAILED bun test v1.4.3-canary.1 (abc36e727) test/bake/hmr-socket-protocol.test.ts: (pass) a SetUrl frame without a leading slash ("nfoo") closes the socket instead of aborting [37.14ms] (pass) a SetUrl frame without a leading slash ("n") closes the socket instead of aborting [41.24ms] (pass) a duplicate testing-batch frame (H) during an in-flight bundle closes the socket instead of aborting [52.96ms] 446 | // Frames that arrive before this reply belong to the previous topic set. 447 | await roundTrip(); 448 | await orFail(fetch(`http://127.0.0.1:${port}${page}`).then(response => response.text())); 449 | delivered.push({ topics, ids: [...new Set(await roundTrip())].sort() }); 450 | } 451 | expect(delivered).toEqual([ ^ error: expect(received).toEqual(expected) [ { "ids": [ "r", "u", ], "topics": "hr", }, { "ids": [ "r", + "u", ], "topics": "r", }, { - "ids": [], + "ids": [ + "r", + "u", + ], "topics": "", }, { "ids": [ "r", "u", ], "topics": "hr", }, ] - Expected ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bake/hmr-socket-protocol.test.ts bun test v1.4.3 (367d939) test/bake/hmr-socket-protocol.test.ts: (pass) a SetUrl frame without a leading slash ("nfoo") closes the socket instead of aborting [874.43ms] (pass) releasing a testing batch while another bundle is in flight defers it [1016.05ms] (pass) a subscribe frame that drops a topic stops delivery of that topic [1140.59ms] (pass) a SetUrl frame without a leading slash ("n") closes the socket instead of aborting [1316.42ms] (pass) a duplicate testing-batch frame (H) during an in-flight bundle closes the socket instead of aborting [1595.68ms] 5 pass 0 fail 9 expect() calls Ran 5 tests across 1 file. [4.40s] __F:0:S:0 release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 1306ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/57] gen generated_host_exports.rs generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 248 extern-C blocks audited [2/56] rustc bun_csrf [3/56] rustc bun_s3_signing [4/56] rustc bun_exe_format [5/56] rustc bun_threading [6/56] rustc bun_dotenv [7/56] rustc bun_uws_sys [8/56] rustc bun_uws [9/56] rustc bun_libarchive [10/56] rustc bun_sql [11/56] rustc bun_watcher [12/56] rustc bun_io [13/56] rustc bun_event_loop [14/56] rustc bun_crash_handler [15/56] rustc bun_md [16/56] rustc bun_spawn [17/56] rustc bun_patch [18/56] rustc bun_ast [19/56] rustc bun_install_types [20/56] rustc bun_resolve_builtins [21/56] rustc bun_options_types [22/56] rustc bun_api [23/56] rustc bun_http [24/56] rustc bun_parsers [25/56] rustc bun_sourcemap [26/56] cxx obj/src/jsc/bindings/BunProcess.cpp.o [27/56] rustc bun_js_printer [28/56] rustc bun_react_compiler [29/56] rustc bun_ini [30/56] rustc bun_css [31/56] rustc bun_js_parser [32/56] rustc bun_resolver [33/56] rustc ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/bake/dev_server/hmr_socket.rs | 5 +- test/bake/hmr-socket-protocol.test.ts | 116 ++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 4 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/bake/dev_server/hmr_socket.rs 1 1 33 test/bake/hmr-socket-protocol.test.ts 3 5 31 ``` </details> <!-- robobun:evidence:end -->
2 participants
Problem
/_bun/hmr, which any client that can reach the dev server may send, drove invalid state. A duplicateH(testing-batch) frame hit theTestingBatchEvents::EnableAfterBundlearm'sdebug_assert!(false). ASetUrlpattern that does not start with/reachedFrameworkRouter::match_slow'sdebug_assert!(path[0] == b'/'):ws.send("n")indexes an empty slice inside that assert,ws.send("nfoo")fails it.Hwhile an unrelated bundle was in flight calledstart_async_bundle, whose first line isdebug_assert!(self.current_bundle.is_none())(DevServer.rs:3111). On a release build that assert is compiled out, and the assignment drops the in-flightCurrentBundle(itsBundleV2, and throughDrop for MimallocArenaanmi_heap_destroyof its arena) while that bundle's thread-pool tasks are still running. Since 1.4.1 that crashes on several threads at once:Fix
H: drop the assert and keep thews.close()that was already there.SetUrl: close the socket on a pattern that does not start with/, like the sibling arms do for their malformed input.match_slow's assert is a correct contract for its HTTP callers, whose request paths always start with/; the HMR socket is the one caller feeding it peer bytes.TestingBatchEvents::ReleaseAfterBundlestate and release it fromfinalize_bundle_cleanuponce no bundle is running, so the harness still gets its bundle. A furtherHin that state is a protocol violation and closes the socket.test/bake/hmr-socket-protocol.test.ts, 4 tests. All 4 fail againstbun bdwithout thesrc/diff and pass with it.Background
/_bun/hmris the dev server's hot-reload websocket.HmrSocket::on_messageswitches on the first byte of each frame, so each arm validates its own payload.Hframe drives the bake test harness's batching: the first turns batching on, a later one releases the files collected since as a single bundle.CurrentBundleowns the arena that the bundler's parse tasks, which run on the thread pool, read from.ensure_route_is_bundled, which starts a bundle without consultingtesting_batch_events. That is how a bundle comes to be in flight between twoHframes.Notes
Rebased onto current
main. The branch was 1983 commits behind and the file had moved fromsrc/runtime/bake/DevServer/HmrSocket.rstosrc/runtime/bake/dev_server/hmr_socket.rs, so the PR had gone conflicting; it is now a clean diff againstmainand all three bugs were re-confirmed present there.An earlier revision of this PR also gated the visualizer on-subscribe hooks on
cfg!(feature = "bake_debugging_features")to stopsMreachingemit_memory_visualizer_message'sdebug_assert!(cfg!(feature = ...)). That fix is dropped:mainhas since deleted the assert and removed the cargo feature fromsrc/runtime/Cargo.tomlentirely, so there is nothing left to gate.While re-checking
sMagainst currentmainI found a separate, still-open crash that this PR does not touch:sM, then ~1s so the 1-second timer fires, thensto unsubscribe givespanic: assertion failed: self.root == v, the intrusive timer heap'sremove()on a node the drain had already popped. The cleanup that emptiedemit_memory_visualizer_message_timertook away thestate = FIRED+ re-insert that kept the node's bookkeeping honest. It reproduces on unmodifiedmainwith this diff stashed, and fixing it needs a decision about what remains of the memory-visualizer feature, so it is filed separately rather than folded in here.The third test keeps every step condition-based rather than timed: a bundler plugin parks the
/twobundle on a fetch the test controls, the batch steps wait on ther0/r1synchronization frames, and the release step waits for the socket close that a furtherHtriggers, which is what proves the firstHwas handled while the bundle was still held.The duplicate-
HandSetUrlarms were reported by review on an earlier revision of this PR; the batch-release segfault came with a runnable reproduction from the fuzz lane, and that reproduction now exits 0 with the server still answering, 3/3.Release dating, from the fuzz lane's runs of the same frames: 1.4.0 keeps serving, while 1.4.1, 1.4.2 and canary segfault 3/3 each. The dev-server side of this did not change in that window.
start_async_bundleis identical betweenbun-v1.4.0andbun-v1.4.1apart from an unrelated inspector stringderef, andsrc/bun_alloc/MimallocArena.rsis byte-identical, so 1.4.0 already destroys the in-flight bundle's arena and gets away with it: a use-after-free that happens to read intact bytes. Of the two changes first suspected, #40478 only touchesStaticRouteresponse refs (neitherCurrentBundlenorBundleV2has a refcounted field for it to affect), and #40640 moves out-of-root path text out of the per-bundle arena into a process-lifetime store, which leaves less dangling, not more. What did change on this path is the allocator: mimalloc moved from6a14aee2to6a64e1bain that window, including #40138, which replaced themi_heap_deleteandmi_heap_destroyteardown with a protocol that detaches, claims and frees a heap's pages even when another thread can still reach them. That is the likeliest reason a silent use-after-free became a reliable fault. It is inferred from the diffs, not bisected; running the reproduction against one Bun commit with the old and the new mimalloc pin would settle it. Either way the defect is the secondstart_async_bundle, which is as old as theEnabledarm. 1.4.1 made it visible.[human-review] gate passed · iteration 10 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 10
evidence per changed file