Release Bun.serve handler Strongs once the server is idle (native↔JS cycle leak) - #32086
Release Bun.serve handler Strongs once the server is idle (native↔JS cycle leak)#32086alii wants to merge 14 commits into
Conversation
A stopped Bun.serve server's NewServer box is freed in NewServer::deinit, which only runs after the JS Server wrapper finalizes. But the server's config holds Strong handles to the user's fetch/error/node-http/websocket handlers, and a handler defined in a scope that closes over the JS Server value (the common pattern) forms a native↔JS cycle the GC cannot see through: box → Strong(handler) → handler's parent lexical environment → server wrapper → m_ctx → box so the wrapper never finalizes, schedule_deinit never runs, and the box (with the entire config it owns) is never freed. Release the handler Strongs once the server is idle (listener gone, in-flight count zero, no live websockets), gated by a new HANDLERS_RELEASED flag so the websocket-handler unprotect runs at most once. The deinitialization fixture is also tightened to assert what it always intended: every JS Server wrapper actually collects (via heapStats counts before/after), not just that the embedded dev server's deinit counter ticks. The fixture had its own retainer too — globalThis.callback (the plugin↔test bridge) was never cleared after each case.
|
Updated 10:57 PM PT - Jun 11th, 2026
❌ @robobun, your commit 569470a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32086That installs a local version of the PR into your bun-32086 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces guarded handler release during idle server shutdown to break native↔JS reference cycles, updates DevServer teardown for intrusive refcount cleanup, and adds comprehensive GC verification and regression tests for websocket handler protection lifecycle. ChangesServer shutdown, handler release, and GC verification
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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/fixtures/deinitialization/test.ts`:
- Around line 74-79: The cleanup that clears the GC root stored in
globalThis.callback must be moved into a finally block so it runs even if main()
throws; wrap the await main() call and any setup in try { await main(); ... }
finally { globalThis.callback = undefined; } (reference the main() invocation
and the globalThis.callback reset) to ensure the global is always cleared before
heap assertions.
🪄 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: 0833842e-e1df-43e9-9ee0-f870d9d9a216
📒 Files selected for processing (2)
src/runtime/server/mod.rstest/bake/fixtures/deinitialization/test.ts
|
@robobun adopt |
|
✅ Reproduced the asan failure locally (leaked |
Restore src/runtime/bake/DevServer/HmrSocket.rs and test/cli/inspect/BunFrontendDevServer.test.ts to their state on main. The branch accidentally re-added the duplicate clientNavigated inspector notify and deleted the guard test, both from #32081.
Once the idle release lets NewServer boxes actually free, LSan reports the html_bundle::Route objects created for served HTML routes: get_or_put_route_bundle takes an intrusive ref when it creates a RouteBundle, but the raw html_bundle pointer has no Drop and DevServer's Drop never released it. Deref it in the same loop that already releases client_bundle and cached_response, matching Zig RouteBundle.deinit. reload() on a stopped server would unprotect the old websocket context's handlers a second time (the idle release already did), which strips another server's protection of the same function values. Skip the unprotect when HANDLERS_RELEASED is set and clear the flag once a new protected context is installed. Tests: protected-AsyncFunction counts prove the idle release and the reload guard; the deinitialization fixture clears its GC root in a finally block; the spawning wrapper gets a 60s budget since the child suite exceeds the 5s default under ASAN.
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 `@src/runtime/server/server_body.rs`:
- Around line 2204-2206: The HANDLERS_RELEASED flag is only cleared in the
websocket-adoption path via self.flags.remove(ServerFlags::HANDLERS_RELEASED),
but new handlers can also be installed earlier (the
on_request/on_error/on_node_http_request install path), so update the code to
clear/reset HANDLERS_RELEASED whenever the method installs any new handler
Strong (not just in the websocket branch). Locate the handler-installing logic
(the block that registers on_request, on_error, on_node_http_request) and add a
call to self.flags.remove(ServerFlags::HANDLERS_RELEASED) right after those
installations (or factor into a helper used by both the websocket-adoption
branch and the earlier install path) so all newly installed handlers are treated
consistently and won't be skipped by the idle deinit pass.
🪄 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: 8d30ad34-5b8c-41c6-a918-fdf1845b01a4
📒 Files selected for processing (8)
src/runtime/bake/DevServer.rssrc/runtime/bake/DevServer/HmrSocket.rssrc/runtime/bake/dev_server/route_bundle.rssrc/runtime/server/server_body.rstest/bake/deinitialization.test.tstest/bake/fixtures/deinitialization/test.tstest/cli/inspect/BunFrontendDevServer.test.tstest/js/bun/websocket/websocket-server-reload-leak.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/bake/DevServer/HmrSocket.rs
The Strong releases in the idle pass are idempotent, so run them on every pass instead of gating them behind HANDLERS_RELEASED; the flag now guards only the counted websocket unprotect. on_reload_from_zig re-runs the idle pass at the end, so handlers installed by reloading a stopped server are released immediately instead of pinning the wrapper; it is a no-op while the server is listening or has work in flight.
…ndlers A graceful stop() leaves accepted keep-alive connections open, and one can deliver a request after the idle pass released the handler Strongs: the uws on_request and node:http dispatch trampolines then called a zero JSValue (assertion on asan, segfault at 0x0 in release). Both trampolines are registered only when the corresponding handler exists, so a missing handler means the server is fully stopped: answer 503 and close the connection before any request state is created.
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this bundles four interacting lifecycle fixes in the Bun.serve hot path (handler Strong release, counted gcProtect gating, reload-after-stop guard, 503 on late keep-alive requests) plus a DevServer intrusive-refcount fix and a user-visible behavior change at the edges — worth a human pass on the invariants.
Extended reasoning...
Overview
This PR fixes a native↔JS reference cycle that prevents stopped Bun.serve servers from being freed. It touches:
src/runtime/server/mod.rs: addsServerFlags::HANDLERS_RELEASED, releases config handlerStrongs and websocket gcProtects indeinit_if_we_can, and short-circuits the uwson_request/ node:http trampolines to 503+close when handlers are gone.src/runtime/server/server_body.rs: guardson_reload_from_zig's websocket swap against double-unprotect when the old context was already released, clears the flag on new-context install, and re-runs the idle pass at the end.src/runtime/bake/DevServer.rs/route_bundle.rs: derefs the intrusivehtml_bundleref inDrop(previously leaked oneRouteper HTML route per server).HmrSocket.rs+BunFrontendDevServer.test.ts: restores #32081, which the branch had accidentally reverted.- Five test files with new GC/heapStats-based regression coverage.
Security risks
None identified. The 503 short-circuit on released handlers is a safe degradation; no auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
High. This is core server lifecycle code with subtle invariants:
- The distinction between idempotent
Option<Strong>releases (run unconditionally) vs. countedgcProtect/unprotect(gated byHANDLERS_RELEASED) is correct but delicate — getting it wrong either reinstates the leak or strips another server's protection of shared handler values. - The 503 guard in
on_request/on_node_http_request_with_upgrade_ctxis new code in the request hot path and changes user-visible behavior (late keep-alive requests afterstop()now 503 instead of being served;server.fetch()after idle now rejects). self.deinit_if_we_can()at the tail ofon_reload_from_zigruns on every reload — the PR asserts it's a no-op while listening, which depends on the idle gate inside that function holding.- The DevServer
Dropchange is an unsafe intrusive-refcount deref on a raw pointer.
Other factors
- The PR description is exceptionally detailed and the test coverage is strong (each fix has a regression test that fails on the unfixed build).
- Both CodeRabbit findings were addressed; the second exchange (re: where to clear
HANDLERS_RELEASED) led to a cleaner design in 590f5c3. - CI has one failure (
test/cli/init/init.test.tson macOS aarch64) that appears unrelated. - This is four distinct fixes plus a branch-cleanup in one PR; the interactions between them (especially fix 1 unmasking fix 2, and fix 1 requiring fixes 3 and 4) are well-explained but increase review surface.
Given the hot-path placement, unsafe refcount management, and intentional behavior change, this should get a human look despite no bugs being found.
ServerWebSocket holds a BackRef into the inline config.websocket.handler storage, so sockets opened before a reload dispatch through whichever context the swap installs. The fresh context started its active_connections at zero, which made has_active_web_sockets() report an idle server while those sockets were still connected: with a graceful stop before or after the reload, deinit_if_we_can then unprotected the newly installed handlers and the next message or close on a surviving socket could call a collected JSValue. Copy the old count into the new context so the idle release waits for the sockets to actually close.
The stopped-server guard covered on_request and the node:http trampoline but not the per-route dispatch paths: the H1 on_user_route_request, the generic on_user_route_request_for used by H3, and upgrade_web_socket_user_route. A late request to a declared route on a surviving keep-alive connection was still served by a stopped server, and once the released wrapper is collected the same path panics in js_value_assert_alive. All three now answer 503 and close the connection when HANDLERS_RELEASED is set (the handler presence checks used by the other trampolines do not work here since routes-only servers run with on_request unset).
There was a problem hiding this comment.
Both issues I flagged earlier are addressed (the active_connections carry-over in 46a7cb5 and the user-route trampoline guards in e4f5804), and I have nothing further — but this reworks the server's GC/lifetime teardown and changes post-stop() dispatch behavior, so it's worth a human pass before merge.
Extended reasoning...
Overview
Breaks a native↔JS reference cycle that prevented stopped Bun.serve instances from ever being freed when a handler closure captured the Server value. The fix releases the config's Strong/gcProtect handler references in deinit_if_we_can once the server is fully idle, with a HANDLERS_RELEASED flag gating the non-idempotent websocket unprotect. Five interlocking parts: the idle release itself, an html_bundle::Route deref the release unmasked in DevServer::Drop, a reload-after-stop double-unprotect guard, carrying active_connections across the websocket context swap, and 503/close guards on every uWS dispatch trampoline so late requests on surviving keep-alive connections can't reach released handlers or a finalized wrapper. Substantial new test coverage (heap-stat assertions, protected-AsyncFunction counts, raw-socket keep-alive regressions for both catch-all and declared routes, both stop↔reload orderings with a live websocket).
Security risks
None identified. No auth, crypto, or permission surfaces touched. The new 503 path is strictly more restrictive than before (refuses dispatch where the old code would have called into a released/possibly-collected handler).
Level of scrutiny
High. This is core Bun.serve lifecycle code with subtle GC/refcount/protect-count invariants, and review on this PR already surfaced two real bugs (a use-after-free window from the lost active_connections count, and unguarded user-route trampolines) that required follow-up commits. It also changes user-observable behavior: server.fetch() on a stopped-idle server now rejects, and late keep-alive requests get a 503 instead of being served. The PR description argues nothing depends on the old behavior, which seems right, but that's a judgment call for a maintainer.
Other factors
All prior review threads (CodeRabbit's two and my two) are resolved with code changes plus targeted tests that fail on the unfixed build. CI is reported green on all lanes including ASAN/LSan at the current head. The change is well-reasoned and well-tested; I'm deferring purely on complexity/criticality, not on any open concern.
|
@robobun please fix my review |
|
I cannot see a review from you on this PR, only the bot reviews (all resolved). If you left line comments, the review is probably still pending; pending reviews are visible only to their author until submitted. Once you submit it I will pick the comments up and address them. |
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Max-effort review pass. robobun's e4f5804 already addressed the missing user-route/upgrade guards while this was being written, so those are dropped here. [1] is the remaining blocker — the active_connections carry correctly delays the idle release while websockets are live, but nothing triggers it once the count drains to zero. [hot-path] is a design suggestion: with five entry points now guarding per-request, swapping the uws callbacks at idle-release would cover all of them with zero hot-path additions.
alii
left a comment
alii
left a comment
There was a problem hiding this comment.
Max-effort review pass. robobun's e4f5804 already addressed the missing user-route/upgrade guards while this was being written, so those are dropped here. [1] is the remaining blocker — the active_connections carry correctly delays the idle release while websockets are live, but nothing triggers it once the count drains to zero. [hot-path] is a design suggestion: with five entry points now guarding per-request, swapping the uws callbacks at idle-release would cover all of them with zero hot-path additions.
|
@robobun here |
|
Got it now, working through the review. [1] is real: the close path had no way to re-run the idle pass. Fixing it per [6] (count moved onto NewServer, handler gets a type-erased server backref set in set_routes, close drains trigger deinit_if_we_can), plus [7] short-circuit, [3] message, and the cleanups. Tests including the post-close probes are running; will push shortly. |
The active_connections carry in 46a7cb5 delayed the idle release while sockets were live, but nothing re-ran deinit_if_we_can once the count drained to zero, so a graceful stop with a connected websocket still leaked the cycle. Restructured per review: - The live-socket count moves onto NewServer (next to pending_requests, which has_active_web_sockets mirrors), so a reload's context swap can no longer reset it and the carry is deleted. - Handler gains a type-erased AnyServer backref, set in set_routes alongside app. ServerWebSocket open/close route their accounting through it, and the close that drains the last socket runs deinit_if_we_can. Skipped while TERMINATED: the abrupt path drains sockets synchronously from inside stop_listening (which holds &mut self) and stop() runs the idle pass itself right after. - on_reload_from_zig short-circuits on a fully idle stopped server instead of registering routes and releasing them via a trailing idle pass; HANDLERS_RELEASED is now terminal and the reload flag dance is gone. - fetch() on a stopped idle server rejects with a message naming the stopped state instead of claiming the fetch handler is missing. - Cleanups: reject_stopped helper for the five dispatch guards, one canonical guard comment, merged websocket blocks in the release path, dead unwrap_or(ZERO) defensiveness removed, fixture baseline captured in beforeAll with a shared drain helper, websocket probe tests share runProbe and the two live-socket orderings are one test.each with post-close assertions that the release actually fired.
ws.close() and ws.terminate() set the closed flag before end()/close() re-enters on_close, whose accounting is skipped for already-closed sockets, so the live-socket count never drained for server-side closes and the idle release could never run on such servers. Both methods now balance the count themselves, mirroring the on_open error path. The drain trigger was gated on TERMINATED, which is permanent: a stop(true) issued from inside a websocket close handler decrements only after stop() returns, and the trigger then skipped forever. The gate is now a transient WEBSOCKETS_DRAINING flag held only across the abrupt app.close() drain inside stop_listening, so the calling socket's deferred decrement still runs the idle pass.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
- The comments should not be there
- This is dangerous code and I'm wondering if we can avoid doing this? How do we lean more on GC to keep things alive than strong? Can we refactor to make it visit each child of the server and keep things alive that way, instead of relying on being smart enough to release things at the right time?
Cut the review-added commentary down to SAFETY lines and short why notes. Handler::unprotect now zeroes the seven callback slots after dropping their protections. A ws.close() inside a message handler can drain the last socket of a stopped server and run the release while the dispatch is still on the stack; its tail error path then reads the handler fields, and an unrooted cell there is a use after free once GC runs. Zeroed slots make the existing empty checks fire instead, so the throw surfaces through the default uncaught reporter.
|
Stripped the comments down to SAFETY lines and short why notes in 9cb0bdb. On the design question: agreed, and yes, the GC-visitation model works here. The per-route callbacks already use it: they live on the wrapper's
What survives from this PR regardless: the It is a bigger change: |
…ving idleness The idle predicate is not stable after the release: static and file routes dispatch without JS and bump pending_requests with no released guard, so a late request on a surviving keep-alive connection makes a released server transiently non-idle. A reload() in that window used to bypass the short-circuit and install websocket handler protections that the idle pass (gated on the terminal flag) would never release.
The late request on the surviving keep-alive connection never reaches the router there, so the pending window the race needs cannot open. The test asserts that precondition and was failing with it on both Windows lanes; the reload short-circuit it covers is platform-independent.
|
@Jarred-Sumner Replacing with #32215 |
3 participants
A stopped
Bun.serveserver is never freed if any of its user-supplied handler callbacks closes over the JSServervalue, the common pattern. The bake decoupling work in #32078 surfaced this on the x64-asan lane (test/bake/deinitialization.test.ts), but it is a pre-existing leak independent of that stack.Mechanism.
NewServer::deinit(which frees the box) only runs after the JSServerwrapper finalizes. ButServerConfigholdsStronghandles to the user'sfetch/error/nodeHTTPRequesthandlers (and gcProtects on the websocket handlers), and a handler defined in a scope that closes over the JSServervalue forms a cycle the GC cannot see through:stop()downgradesjs_value(the box's handle to the wrapper) but never releases the config-side handles, so the wrapper never finalizes and the box is never freed.Fix, part 1: release handler refs once the server is idle. In
deinit_if_we_can, alongside the existingdev_server.take(), release every config callback handle once the server is idle (listener gone, in-flight count zero, no live websockets): the threeOption<Strong>fields,on_clienterror, and the websocket context's gcProtects. The protect-counted websocket unprotect is gated by a newServerFlags::HANDLERS_RELEASEDso it runs at most once across the multipledeinit_if_we_cancallers.Fix, part 2: the leak that releasing the boxes unmasked. With the boxes actually freed, the asan lane then reported the
html_bundle::Routeobjects created for served HTML routes:get_or_put_route_bundletakes an intrusive ref on the route when it creates aRouteBundle, with a comment claimingRouteBundle::deinitreleases it. That deinit does not exist in the Rust port: the field is a raw pointer with no Drop, andDevServer'sDropreleasedclient_bundleandcached_responsebut nothtml_bundle(the Zig reference,RouteBundle.zig:112, doeshtml.html_bundle.deref()). One request through an HTML route therefore leaked oneRouteper server. Exactly the 4 fixture cases withsendAnyRequests: trueleaked. Fixed by deref'inghtml_bundlein the same teardown loop, and the stale comments now describe the real ownership.Fix, part 3: reload on a stopped server.
server.reload()has no stopped-server guard, and its websocket swap unconditionally unprotected the old context. After the idle release, that second unprotect would strip another server's protection of the same function values (gcProtect is counted per value, and handler objects can be shared between servers). The swap now skips the unprotect whenHANDLERS_RELEASEDis set and clears the flag once the new protected context is installed. The idempotentStrongreleases run on every idle pass (the flag gates only the counted websocket unprotect), andon_reload_from_zigre-runs the idle pass at the end: handlers installed by reloading a stopped server can never be invoked and are released immediately instead of reinstating the cycle. The extra pass is a no-op while the server is listening or has work in flight.Fix, part 4: live websockets defer the release, and their last close triggers it.
config.websocketis inline storage and eachServerWebSocketholds aBackRefinto it, so sockets opened before areload()dispatch through whichever context the swap installs, while the fresh context restartedactive_connectionsat zero: the idle pass could then unprotect handlers that surviving sockets still invoke (found by review). And even with the count preserved, nothing re-ran the idle pass once the last socket closed, so the cycle persisted for graceful stops with connected websockets (also found by review). Reworked structurally: the live-socket count lives onNewServernext topending_requests(a reload cannot reset it),Handlercarries a type-erasedAnyServerbackref set inset_routes, and the close that drains the last socket runsdeinit_if_we_can(skipped only while the transient WEBSOCKETS_DRAINING flag is held across the abruptapp.close()drain insidestop_listening, where&mut selfis live andstop()runs the idle pass itself; a socket whose close handler calledstop(true)decrements afterstop()returns with the flag cleared, so its drain still triggers). Server-sidews.close()/ws.terminate()balance the count themselves, since the re-entranton_closethey dispatch skips its accounting for already-closed sockets.HANDLERS_RELEASEDis terminal:on_reload_from_zigshort-circuits once the flag is set instead of installing handlers that could never run, which also removes the reload flag dance. The short-circuit tests the flag rather than re-deriving idleness: static/file routes dispatch without JS and bumppending_requestswith no released guard, so a backpressured static response on a surviving keep-alive connection would otherwise let areload()slip through and install protections the flag-gated idle pass never releases (found by review; a regression test drives the window with a paused raw socket). Onetest.eachcovers both stop/reload orderings with a connected websocket, asserting the handlers stay protected while the socket lives, still serve messages, and release once it closes.Known behavior edge:
server.fetch()on a stopped idle server rejects with "fetch() cannot be used after the server has been stopped" (previously it dispatched the handler; the message distinguishes the stopped state from a server that never had a fetch handler).Fix, part 5: late requests on surviving connections. A graceful
stop()closes only the listener; an already-accepted keep-alive connection can deliver one more request after the idle release. The uwson_requestand node:http dispatch trampolines called a zeroJSValuein that window (ASSERTION FAILED: Cannot call function with JSValue zeroon the asan lane viatest/regression/issue/server-stop-with-pending-requests.test.ts, segfault at address 0x0 on Windows vianode-http-uaf). Both trampolines are registered only when the corresponding handler exists, so a missing handler means the server is fully stopped: they now answer 503 and close the connection before any request state is created. The per-route dispatch paths (on_user_route_request, the genericon_user_route_request_forused by H3, andupgrade_web_socket_user_route) get the same guard onHANDLERS_RELEASED, since routes-only servers legitimately run withon_requestunset and the route list lives on the JS wrapper, which may already be collected by the time a late request arrives. Two regression tests drive the catch-all and declared-route paths deterministically over a raw socket (both fail on the unfixed build, where the stopped server serves the late request).Known behavior edge: a late request on a surviving keep-alive connection gets a 503 and connection close instead of being served by the stopped server. No test or documented behavior relies on the old behavior.
Branch cleanup. The original branch accidentally reverted #32081 (re-added the duplicate
clientNavigatedinspector notify and deleted its guard test); both files are restored to main.Tests.
test/bake/fixtures/deinitialization/test.tsasserts what it always intended: every JSServerwrapper actually collects (heapStats().objectTypeCountsbefore/after), not just that the embedded dev server's deinit counter ticks. The fixture's own retainer (globalThis.callback) is now cleared in afinally. The spawning wrapper gets a 60s budget; the child suite exceeds the 5s default under ASAN.websocket-server-reload-leak.test.tsgains three tests usingheapStats().protectedObjectTypeCounts.AsyncFunction: the idle release drops the websocket handler protections to baseline (fails on the unfixed build), a stopped server's reload releases the newly installed handler protections (fails on the unfixed build), and a stopped server's reload leaves a second server's shared handler protections intact (fails if the reload guard is removed).detect_leaks=1plustest/leaksan.supp): the fixture aborts with theRouteleak before the fix and runs clean after.serve.test.ts,websocket-server.test.ts,node-http.test.ts, andBunFrontendDevServer.test.tspass (the handful of remaining failures reproduce on the released binary in the same container: IPv6, privileged ports, proxy egress).This lands ahead of #32077/#32078 so each remaining decoupling PR is green on the asan lane independently. It supersedes the suppression approach in the (closed) #32084.