bake DevServer: remove unsafe from DevServer.rs and dev_server/* - #40255
Jarred-Sumner wants to merge 12 commits into
Conversation
|
Updated 7:32 AM PT - Sep 8th, 2026
✅ @Jarred-Sumner, your commit 408cbf5be0c0aa74b09d0b148b74bf9f65108542 passed in 🧪 To try this PR locally: bunx bun-pr 40255That installs a local version of the PR into your bun-40255 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks — c5cbe4c addresses the stale arena-lifetime comment nit; the earlier drop-order and duplicated-doc-comment findings were fixed in aedac40. This automated pass found no further issues.
Given the scope (38 files, ~5k lines net, a new ownership model for DevServer via JsCell/OwnedThis, new cross-thread primitives like ThreadBound/HotReloadShared, refcounted DeferredRequest/HmrSocket, and FFI signature changes), a human pass is still warranted.
What was reviewed
BundleSetupdestructuring drop order on thestart_async_bundleerror paths — now bindsheapfirst sobv2drops before its arena.finalize_bundlephasing: user JS (server HMR patch, framework handlers, abort handlers) runs betweenwith_mutsections, not under a DevServer borrow; failed-bundle request contexts are released after the error page is written.HmrSocket/DeferredRequestrefcount balance: connection map + uws each hold a ref,on_close/detach_from_dev_serverrelease;AdditionalOnAbortCallbackholds the secondDeferredRequestref.ThreadBoundSend/Sync justification andHotReloadSharedslot protocol vs. the oldWatcherAtomics.
Extended reasoning...
Overview
This PR is the bake DevServer instalment of the ongoing zero-unsafe programme (stacked on #40214). It replaces the Box<DevServer> + raw-pointer container_of / per-field reborrow pattern with a JsCell<DevServer> in an OwnedThis<DevServerCell> owned by NewServer, threaded through uws routes/sockets/deferred-requests/bundler as ThisPtr/BackRef and entered via short with_mut sections. The IncrementalGraph sibling access moves from container_of to an explicit GraphRef { g, s: GraphSiblings } split borrow; the watcher-thread → JS-thread hot-reload channel becomes Arc<HotReloadShared> + ThreadBound<DevServerCell> + VmHandle::post_boxed. New reusable primitives land in bun_ptr (ThreadBound), bun_uws_sys (BodyReader, WebSocketHandlerRef, upgrade_ref), bun_jsc (HeadersRef::create, VmHandle::post_boxed), bun_alloc (default_arena()), and bun_watcher (WatcherHandler trait, shutdown_boxed). Two C++ FFI signatures change to take {ptr,len} structs. BundleOptions::framework moves from Option<&'a Framework> to Option<Arc<Framework>>. Residual: one unsafe block in bundle_borrows (documented; needs a bundler-side change to BundleV2's primary-transpiler borrow).
Security risks
No new attack surface. The Host/Origin guards on /_bun/* routes are preserved (is_allowed_dev_host / is_allowed_dev_origin still gate dev_route). The InspectorBunFrontendDevServerAgent__notifyBundleStart change from transferToWTFString to toWTFString is a copy-vs-move on trigger-file names, not user-controlled input. The FFI struct changes (BunStringSlice, BakeSourceMapJSON) are ABI-equivalent {ptr,len} pairs matching bun_core::ffi::FfiSlice.
Level of scrutiny
High. This is a structural rewrite of the dev server's ownership and re-entrancy model in memory-safety-critical code: refcounted sockets/requests with multiple holders, cross-thread state (ThreadBound with hand-written Send/Sync), explicit phasing so user JS never runs under a JsCell borrow, and one remaining lifetime-erasing unsafe block whose SAFETY comment admits the exclusivity invariant is not locally established. The PR description documents four pre-existing bugs fixed in passing (Drop-time next-bundle request handling, synchronous plugin-load parking, double-processed hot-reload event, double-deinit on the failure branch), each of which is a behaviour change worth a maintainer's eye. The diff is ~640KB across 38 files.
Other factors
Two prior automated findings were addressed (aedac40: BundleSetup destructuring order + duplicated doc comment; c5cbe4c: stale arena-lifetime comment in bake_body.rs). This run's bug hunt found nothing further. The PR reports the full test/bake/* and test/cli/hot/* suites passing under debug+ASAN plus a hand-driven fixture, and clippy/rust-check-all clean across targets — but there are no new tests in the diff for the four in-passing bug fixes, and the re-entrancy phasing (JS runs between with_mut sections) is the kind of invariant that benefits from a maintainer confirming every call site. Not a candidate for auto-approval.
1a1d590 to
c608a6a
Compare
896ad7f to
93aa1f4
Compare
There was a problem hiding this comment.
I reviewed the follow-up commits and this pass found no further issues — the earlier drop-order and stale-comment findings are all addressed. Given the scope (~14k-line ownership/re-entrancy refactor across DevServer, uws, watcher, and bundler), a human pass is still worthwhile.
What was reviewed:
JsCell/with_mutphasing so JS (framework handler, plugin load, HMR patch, aborts) runs between borrows — checkeddev_route,ensure_route_is_bundled,finalize_bundle,start_async_bundlefor re-entrancy under the new split.- Ref-count balance for
DeferredRequest/HmrSocket/BodyReader/upgrade_refon abort, close, and Drop paths;BundleSetupdestructuring drop order now bindsheapfirst. - Watcher-thread
HotReloadShared/ThreadBoundprotocol andshutdown_boxed;notifyBundleStart/BakeLoadServerHmrPatchWithSourceMapFFI now take slice structs (C++ side matches, no ownership change).
Extended reasoning...
Overview
This PR is the DevServer instalment of the ongoing zero-unsafe programme: it replaces the raw-pointer / container_of / MaybeUninit ownership model in src/runtime/bake/DevServer.rs (197 → 2 unsafe blocks) and dev_server/* (→ 0) with JsCell<DevServer> inside an OwnedThis<DevServerCell>, BackRef/ThisPtr handles for uws routes, HMR sockets, deferred requests, bundler dispatch and hot-reload tasks, and a phased with_mut structure so user JS never executes under a live &mut DevServer. It introduces ThreadBound<T>, bun_uws::BodyReader, WebSocketHandlerRef, Response::upgrade_ref, HeadersRef::create, default_arena(), VmHandle::post_boxed, Watcher::init_with_handler/shutdown_boxed, and reshapes IncrementalGraph sibling access into an explicit GraphRef split-borrow. Two C++ FFI signatures switch from ptr+len to FfiSlice structs. Roughly 14k diff lines across 38 files, stacked on #40214.
Security risks
No new attack surface. The Host/Origin guards on /_bun/* routes and the HMR websocket upgrade are preserved verbatim in dev_route / on_websocket_upgrade. The notifyBundleStart change from transferToWTFString to toWTFString is deliberate (Rust side now sends borrowed &[BunString] instead of transferring ownership). The BakeLoadServerHmrPatchWithSourceMap ownership contract is unchanged — Box::leak on the Rust side, C++ provider frees.
Level of scrutiny
High. This is a large refactor of memory-safety-critical, cross-thread, GC-adjacent code: ref-counted request lifecycles across abort paths, watcher-thread ↔ JS-thread event handoff, uws socket userdata ownership, and the one remaining unsafe (bundle_borrows) that erases the transpiler/heap lifetime for BundleV2<'static>. The PR description documents the residual and several pre-existing bugs fixed in passing (double-processed first hot-reload event, next-bundle requests not aborted on Drop, plugin-state reset race). These are exactly the changes REVIEW.md flags as needing careful review of every terminal path.
Other factors
The author has addressed every prior finding from this bot (drop-order regression in BundleSetup destructuring, duplicated finalize_bundle doc comment, stale arena-lifetime comment in bake_body.rs). The current bug-hunt pass came back clean. Test coverage is stated as extensive (all of test/bake/dev/* under debug+ASAN, hand-driven HMR/error/recovery/stop-during-bundle scenarios, clippy, cross-target check), but the sheer number of restructured control-flow paths and new primitives — plus the acknowledged non-exclusive &'static mut Transpiler in bundle_borrows — put this well outside auto-approval territory.
|
5b23aae: release builds compile again ( |
There was a problem hiding this comment.
I reviewed this revision and the bug hunting system found no issues — the earlier findings (BundleSetup drop order, duplicated/stale doc comments) have all been addressed, including the WebSocketHandlerRef doc link in a52e562. Because this is a large ownership/lifecycle refactor across 38 files (new DevServerCell/JsCell entry model, ThreadBound, WebSocketHandlerRef/BodyReader primitives, watcher-thread handoff, deferred-request refcounting) with one documented residual unsafe block, a human look is still worthwhile.
What was reviewed:
start_async_bundle/finalize_bundlephasing so JS never runs under awith_mutborrow; drop order ofheapvsbv2on error paths.HmrSocket/DeferredRequestref balance acrossupgrade_ref,on_close,detach_from_dev_server, and the abort paths.HotReloadSharedtriple-buffer +ThreadBound<DevServerCell>— watcher thread never dereferences the JS-thread cell.InspectorBunFrontendDevServerAgentFFI change fromtransferToWTFStringtotoWTFString(caller now owns/drops theVec<BunString>).
Extended reasoning...
Overview
This PR is the DevServer instalment of the ongoing "remove unsafe" programme (stacked on #40214). It replaces the Box<MaybeUninit<DevServer>> + container_of/raw-pointer-reborrow pattern with a DevServerCell(JsCell<DevServer>) owned by NewServer, and threads ThisPtr/BackRef/RefPtr handles through every re-entry point (uws routes, HMR sockets, deferred requests, the bundler's DevServerHandle, body readers, hot-reload tasks). It also introduces several new cross-crate primitives — bun_ptr::ThreadBound, bun_uws_sys::{BodyReader, WebSocketHandlerRef}, Response::upgrade_ref, VmHandle::post_boxed, Watcher::init_with_handler/shutdown_boxed, bun_alloc::default_arena() — and reshapes finalize_bundle/start_async_bundle/ensure_route_is_bundled into phased with_mut sections so user JS runs between borrows rather than under them. IncrementalGraph sibling access moves from container_of to an explicit GraphRef split borrow. 38 files, ~640 KB of diff; lifecycle.rs deleted.
Security risks
None identified. The Host/Origin guards on dev-server routes are preserved unchanged (is_allowed_dev_host/is_allowed_dev_origin still gate every handler in dev_route and the WebSocket upgrade). No new externally-reachable surface, no auth/crypto changes, no untrusted-input parsing changes beyond mechanical reshaping of the existing body readers.
Level of scrutiny
High. This is a wholesale rewrite of ownership, borrowing, and refcount lifecycle in a subsystem where the previous design's invariants were held together by ~200 hand-audited unsafe blocks. The new design trades those for JsCell reentrancy checks, BackRef holder obligations, and explicit RefPtr balance — all of which are easier to reason about but still require a reviewer to trace each holder's lifetime against the cell it points into (HMR sockets vs Drop for DevServer, deferred requests vs abort callbacks, the watcher thread's Arc<HotReloadShared> vs shutdown_boxed). It also introduces new unsafe impl Send/Sync on ThreadBound, changes an FFI signature (notifyBundleStart now borrows rather than transfers strings), and leaves one documented residual unsafe (bundle_borrows) whose SAFETY comment admits the exclusive-borrow invariant is not actually established. This is exactly the "memory safety (the most-blocked category)" territory in REVIEW.md.
Other factors
Three prior automated review rounds surfaced a real drop-order regression and several doc-comment issues; the author fixed each promptly. The current bug-hunting pass found nothing. The PR description reports the full test/bake suite plus hot-reload/watch tests passing under debug+ASAN, a hand-driven multi-client HMR fixture, clippy on all touched crates, and rust-check-all on windows-msvc + apple-darwin. That is strong evidence of correctness, but the scale of the change, the new primitives it introduces for downstream use, the stacked base branch, and the explicitly-documented residual unsafe all warrant a human maintainer's sign-off rather than an automated approval.
7a1fcdf to
a18f3ae
Compare
a52e562 to
3c0d0fa
Compare
93bac3d to
084d026
Compare
3c0d0fa to
6ea5167
Compare
084d026 to
4f359b9
Compare
6ea5167 to
4974f10
Compare
bfaf540 to
955d4d7
Compare
DevServer lives in a DevServerCell (JsCell) owned by the server as OwnedThis; uws routes, HMR sockets, deferred requests, the bundler handle and hot-reload tasks reach it through ThisPtr/BackRef + with_mut. HmrSocket and DeferredRequest are CellRefCounted heap objects; the watcher-thread hand-off is an Arc<HotReloadShared> with Guarded event slots and a boxed HotReloadTask; IncrementalGraph methods that touch sibling DevServer state run on a GraphRef split-borrow instead of container_of; body readers own their handler; C++ entry points are HOST_EXPORTs.
finalize_bundle is driven on the DevServerCell in phases (graph update, server patch load, hot update, serve), with the framework handler, aborted-request signal handlers and microtask checkpoints between the with_mut sections; start_async_bundle enqueues entry points (plugin onResolve) between borrows and takes entry-point resolve failures back from the bundler instead of a re-entrant dispatch; plugin loading in ensure_route_is_bundled happens between borrows. Also: abort pending next-bundle requests on drop, keep the hot-reload slot lock off the graph work, DevServerHandle::from_owner(BackRef), HeadersRef::create(&[..]).
…olver log settings when taking entry-point failures
…efore its arena; dedupe finalize_bundle doc
…iber count before removing the timer
e072e16 to
4ac0750
Compare
…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 -->
This branch has not been deployed
2 participants
What
Same programme as #40055 … #40252. Stacked on #40214 (base branch
claude/server-zero-unsafe; retarget to main once that lands).src/runtime/bake/DevServer.rs197 → 2 (one all-safe fnextern block + one real block, below);dev_server/{mod, hmr_socket, incremental_graph, error_report_request, source_map_store, assets}.rs→ 0,inspector_agent.rs4 → 1 (extern block),lifecycle.rsdeleted.DevServerlives inDevServerCell(JsCell<DevServer>)owned byNewServerasOwnedThis<DevServerCell>; uws routes (method_this/any_this/ws_this),HmrSocket(CellRefCounted,BackRef<DevServerCell>),DeferredRequest(CellRefCounted; the list and the request-context abort slot each hold aRefPtr), the bundler handle (DevServerHandle::from_owner(BackRef<_, Root>)), body readers and the hot-reload task all enter throughThisPtr/BackRef+ shortwith_mutsections.handleRequest,Bun.servefetch fallthrough, plugin loading /onResolve, server HMR patch evaluation, abort handlers of aborted deferred requests, microtask checkpoints and finalRequestContextreleases all happen betweenwith_mutsections (dev_route, theensure_route_is_bundledstep loop, phasedfinalize_bundle, splitstart_async_bundle,on_plugins_resolved/rejected,bundle_new_route).start_from_bake_dev_serverreturns entry-point resolve failures instead of dispatchinghandle_parse_task_failurere-entrantly.bun_watcher::WatcherHandlertrait +FileUpdateBatch,Arc<HotReloadShared>(Guarded<HotReloadFiles>per slot, unchanged atomics protocol,ThreadBound<DevServerCell>for the JS side), events posted asBox<HotReloadTask>viaVmHandle::post_boxed(same primitives as fs.watch/fs.watchFile: remove unsafe from the watcher modules #40200). IncrementalGraph sibling access is aGraphRef { g, s: GraphSiblings }split borrow instead of container_of.bun_uws_sys::BodyReader<H: BodyReaderHandler>(owned; box is the on_data/on_aborted userdata, reclaimed once),WebSocketHandlerRef: AnyRefCounted(ref guard per callback; socket's ref released afteron_close),Response::upgrade_ref(RefPtr<D>);bun_jsc::HeadersRef::create(global, names, values, buf)/bun_http_jsc::to_fetch_headers_ref;bun_alloc::default_arena();Watcher::{init_with_handler, shutdown_boxed};Bake__bundleNewRouteJSFunctionImpl/Bake__getNewRouteParamsJSFunctionImplareHOST_EXPORTs; C++BakeLoadServerHmrPatchWithSourceMap/notifyBundleStarttake{ptr,len}structs.Residual (1 block, needs a bundler change):
DevServer::bundle_borrows—BundleV2<'a>wants&'a mut Transpiler<'a>+&'a Arenaunder one'athat must be'statichere and isn't exclusive (the dev server keeps using its resolver mid-bundle, like the bundler's existingclient_transpiler/ssr_transpilerNonNulls). Fix is forBundleV2to take its primary transpiler as aBackReflike the other two; left for a bundler PR. Trade-off: transpiler init-time define/AST data now comes fromdefault_arena()instead of the per-server arena, so a few KB per DevServer aren't freed on stop.Pre-existing bugs fixed in passing:
Drop for DevServerreleased next-bundle HTML requests without ending their responses / unhookingon_aborted(now aborted first); when plugin loading settled synchronously the old flow resetplugin_statetoPendingand parked the request on a bundle nothing would start;drain_hot_reload_eventsprocessed the first event twice; the failure branch relied on a doubleSavedRequest::deinit()to release the request context (kept explicit: the second release happens after the error page is written).Testing
Debug+ASAN,
--timeout 60000: all oftest/bake/dev/{bundle,html,css,esm,hot,sourcemap,server-sourcemap,incremental-graph-edge-deletion,react-spa,plugins,request-cookies,response-to-bake-response,ssg-pages-router,react-response,vfile,import-meta-inline(+negative),harness,stress,ecosystem,production},test/bake/{deinitialization,serve-plugins-dev-server,framework-router,dev-and-prod},bun-serve-html{,-build-holds-server,-entry,-hot-reload-drop,-manifest},test/cli/hot/{hot,watch,watch-many-dirs}— pass, no ASAN output (bun-serve-html-405's LSAN case fails identically on a main debug build; symbolizer). Hand-driven fixture (HTML + TS + CSS): initial page/bundle/css/sourcemap, 5 HMR clients, edit → hot update, syntax error → error payload + 500 page → recovery, delete + restore an imported CSS file, 50 rapid edits, close 3 clients mid-bundle, origin/host guards,server.stop()during a bundle — exit 0. clippy clean on bun_runtime/bun_uws_sys/bun_watcher/bun_ptr/bun_alloc/bun_bundler/bun_jsc/bun_http_jsc/bun_uws;rust-check-allwindows-msvc + apple-darwin pass.