serve: trace handler callbacks from the wrapper instead of rooting them as Strong - #32215
Closed
alii wants to merge 48 commits into
Closed
serve: trace handler callbacks from the wrapper instead of rooting them as Strong#32215alii wants to merge 48 commits into
alii wants to merge 48 commits into
Conversation
Per-route handlers are stored as WriteBarriers reachable only via the Server JS wrapper. stop() previously downgraded js_value immediately, so a late keep-alive request after stop+drop+GC would hit js_value_assert_alive() with a Finalized ref and panic. Downgrade inside the deinit_if_we_can idle predicate instead so the wrapper stays rooted until pending_requests/listener/active websockets are all clear. The websocket close path does not yet call deinit_if_we_can; the next commit threads an AnyServer backref through Handler so the last close can trigger it.
The previous commit moved the JsRef downgrade into deinit_if_we_can's idle predicate, but nothing calls that when the last websocket closes after a graceful stop. Thread an AnyServer backref through Handler so on_close can trigger it; move the live-socket count onto NewServer (where reload's context swap can no longer reset it). on_close also copies the close handler to a stack local before sig.signal() so a GC between the test and the call cannot collect it.
…per finalize Idle keep-alive sockets are not counted in pending_requests, so the wrapper can downgrade and be collected while one such socket can still deliver another request. js_value_assert_alive() panics on Finalized; the dispatch entry points now check first and close the connection with 503 instead.
JsRef::Weak holds a raw JSValue, not a JSC::Weak: try_get() can return the address of a dead-but-unswept cell, so the Finalized→503 check alone leaves a window where dispatch reads an unrooted handler shadow. Closing idle connections at stop() removes the late-request source; in-flight requests are not idle and drain normally. The on_open error-path websocket-close accounting already runs after run_error_callback as of 65c76a2, so the live-socket count stays nonzero across that read; no further change needed there.
JsRef::Weak holds a raw JSValue: try_get() on Weak returns the address even when the cell is dead-but-unswept. Gating on Strong means trampolines refuse the moment the server goes idle (downgrade) rather than only after the wrapper destructor has run.
…et second request
Contributor
|
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:
WalkthroughRefactors server handler storage from Strong references to JSValue write-barrier slots, defers async-context wrapping to serve/reload, expands cached js_gc_* accessors, gates dispatch on wrapper strong-root (503 fallback), rewires websocket back-references and lifetime accounting, and adds GC/liveness tests. ChangesServer Handler Storage and GC Refactoring
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
|
Updated 8:10 AM PT - Jul 9th, 2026
❌ @robobun, your commit b9275c7 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32215That installs a local version of the PR into your bun-32215 --bun |
Per-route handlers already use this pattern (ServerRouteList held in m_routeList). Extend to fetch/error/nodeHTTPRequest/clientError/ws handlers and the all-closed promise so the native↔JS cycle becomes all-JS-heap once the corresponding Strong/gcProtect roots are dropped in a follow-up. The unused JSServerWebSocket m_socket slot is repurposed to hold the server wrapper so a connected websocket keeps it (and its handler slots) reachable. No behavior change yet — the slots are declared but not written.
The slots are populated alongside the existing routeList write after ptr_to_js. ServerConfig still holds Strong handles; the slot writes are additive until the Strong fields are dropped in a follow-up. wsHandlers, allClosedPromise and onClientError are written from their respective set sites in later commits.
with_async_context_if_needed wraps each callback in a fresh function;
storing the original websocket: {...} object would not root the
wrappers (the H2FrameParser bug a5af485 fixed). Match rdr-impl.
The unused m_socket slot is repurposed. Handler gains an AnyServer backref (same field PR2 adds for the on_close trigger) so init can reach the server wrapper. With the handler callbacks moving to wrapper slots, this edge keeps the server wrapper — and its m_ws* handler slots — reachable while any websocket is connected.
The wrapper slot is now the sole GC root for fetch/error/nodeHTTP handlers. ServerConfig holds the raw JSValue as a shadow for hot-path dispatch reads; reload writes both the shadow and the slot.
heapStats wrapper-count assertions for the cycle (handler closing over server is collected; control case unchanged), websocket-connected liveness, reload swap, and the AsyncLocalStorage init-window collectContinuously stress.
Both branches added the same `server: Option<AnyServer>` field at different struct positions; the rebase auto-merge kept both. Keep one, merge the doc comment to cover both uses (m_server slot tracing and live-socket accounting).
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 Drop never released it. One Route per HTML route per dev-server leaked. Zig RouteBundle.deinit:112 does the deref; the Rust port missed it.
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
This was referenced Jun 12, 2026
alii
force-pushed
the
ali/serve-handlers-gc-traced-v2
branch
from
June 12, 2026 20:47
7b7022c to
7984ef9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/server/ServerConfig.rs (1)
1276-1299: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRoot these callbacks through the parse→slot-write gap.
arg.get_truthy(...)can run a getter and return a fresh closure. After these assignments, the callback only lives inargs.on_*as a rawJSValue, but this function still does more JS-visible work afterward (for example thetlsreads/parsing on Lines 1329-1388). That creates a GC window where getter-produced handlers are untraced beforeserve_with!/on_reload_from_ziginstalls the wrapper slot, so the callback can be reclaimed before it ever becomes the rooted handler.Keep a temporary root for these values until the wrapper slot write completes, then drop that temporary owner once the WriteBarrier slot becomes the long-lived root. As per coding guidelines, "Root or copy every JSValue held beyond the current call" and "Anything that can run user JS can synchronously free your state."
🤖 Prompt for 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. In `@src/runtime/server/ServerConfig.rs` around lines 1276 - 1299, The getters (arg.get_truthy) can produce fresh closures that are only stored in args.on_error / args.on_node_http_request / args.on_request, creating a GC window before the serve_with! / on_reload_from_zig WriteBarrier slot is written; to fix, create temporary rooted holders for each callback returned by arg.get_truthy (e.g., local Root/Rooted/JSValueRoot variables) immediately after retrieval and use those temporaries when validating and assigning, keep the temporaries alive until after the wrapper slot write (the serve_with! / on_reload_from_zig installation), then move the value into the long‑lived WriteBarrier slot and drop the temporary roots; ensure this pattern is applied for all places using arg.get_truthy in this function so no getter-produced handler can be reclaimed prematurely.Source: Coding guidelines
🤖 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/api/BunObject.rs`:
- Around line 1651-1683: The wrapper JS object (obj) must be rooted before
allocating wrapped callbacks or calling write_ws_handler_slots to prevent GC
from collecting the transient local JSValue during with_async_context_if_needed;
move the call to server_ref.js_value.set_strong(obj, global_object) to occur
before the on_request/on_error/on_node_http_request wrapping and before
server_ref.write_ws_handler_slots(obj, global_object) (and then update any
assignments that expect the wrapper to already be rooted), keeping the rest of
the logic (using with_async_context_if_needed and js_gc_*_set helpers)
unchanged.
In `@src/runtime/server/mod.rs`:
- Around line 2992-2995: The current write_ws_handler_slots in mod.rs returns
early when self.config.websocket is None, leaving the m_wsOn* WriteBarrier slots
on the wrapper still rooted; update write_ws_handler_slots to clear all
websocket-related GC slots on the wrapper when self.config.websocket is removed
— explicitly set/clear m_wsOnOpen, m_wsOnClose, m_wsOnError, m_wsOnMessage (and
any other m_wsOn* WriteBarrier fields) to an empty/null JSValue or equivalent so
the wrapper no longer traces old callbacks; keep the existing logic when
websocket is Some(...) but ensure the else branch performs the clearing to drop
references to server-capturing closures.
In `@src/runtime/server/server_body.rs`:
- Around line 2178-2189: The branch handling on_node_http_request must normalize
undefined/null to JSValue::ZERO (like on_request/on_error do) before wrapping:
check new_config.on_node_http_request for undefined or null and treat those as
JSValue::ZERO, then only call with_async_context_if_needed on non-empty callable
values; update the value passed to js_gc_on_node_http_request_set and to
self.config.on_node_http_request accordingly (use the same normalization logic
used by the on_request/on_error branches so with_async_context_if_needed and
later !is_empty() checks never receive non-callable undefined/null values).
- Around line 2199-2212: The branch that processes new_config.websocket can
leave previous websocket handler roots active when the new ws is rejected by the
on_message/on_open gate; update the logic in the block handling
new_config.websocket.take() so that if the new ws is not adopted you explicitly
clear existing roots: set self.config.websocket = None (or replace with an empty
value) and call write_ws_handler_slots(global) or another routine to null out
the wrapper's wsOn* slots so the old BackRef/global roots are unbound; ensure
you still drop or release the BackRef on the rejected ws and maintain the
existing use of handler.flags and write_ws_handler_slots when a ws is adopted.
In `@test/js/bun/http/bun-server.test.ts`:
- Around line 1524-1549: The test creates servers with Bun.serve(...) and delays
calling server.stop(), which can leak resources if an earlier await fails;
immediately register cleanup by either using a using/await using pattern with
the returned server or wrap the server creation in try/finally and call await
server.stop() in finally before any awaits (also ensure you await the Promise
returned by server.stop() in the `/after-stop` case); update all instances
referencing server, stop(), and responsePromise (and the websocket cases at the
noted ranges) to ensure stop() is scheduled/awaited before the first await so
tests remain hermetic.
---
Outside diff comments:
In `@src/runtime/server/ServerConfig.rs`:
- Around line 1276-1299: The getters (arg.get_truthy) can produce fresh closures
that are only stored in args.on_error / args.on_node_http_request /
args.on_request, creating a GC window before the serve_with! /
on_reload_from_zig WriteBarrier slot is written; to fix, create temporary rooted
holders for each callback returned by arg.get_truthy (e.g., local
Root/Rooted/JSValueRoot variables) immediately after retrieval and use those
temporaries when validating and assigning, keep the temporaries alive until
after the wrapper slot write (the serve_with! / on_reload_from_zig
installation), then move the value into the long‑lived WriteBarrier slot and
drop the temporary roots; ensure this pattern is applied for all places using
arg.get_truthy in this function so no getter-produced handler can be reclaimed
prematurely.
🪄 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: ad657845-5390-4895-8c68-2fc1cbfacf70
📒 Files selected for processing (12)
src/runtime/api/BunObject.rssrc/runtime/bake/DevServer.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/ServerWebSocket.rssrc/runtime/server/WebSocketServerContext.rssrc/runtime/server/mod.rssrc/runtime/server/server.classes.tssrc/runtime/server/server_body.rstest/bake/deinitialization.test.tstest/bake/fixtures/deinitialization/test.tstest/js/bun/http/bun-server.test.ts
…xy in ws-close trigger
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- replace DEINIT_RUNNING bitflag with a Cell<bool> field; the scopeguard's raw *mut self.flags was invalidated under Stacked Borrows by the &mut self reborrows in the body. Clear inline at the tail (no early returns) instead of holding a borrow across them. - write_ws_handler_slots now takes Option<JSValue> and is called unconditionally on reload, matching wrap_handler_slot's contract so the ws shadows get async-context-wrapped even when the wrapper is gone. Collapse the None/Some branches to one 7-setter list. - bind let-Some(server_js)=js_value_for_dispatch() at each trampoline gate and reuse it instead of re-matching via js_value_assert_alive(). Adds the gate to on_saved_request (defensive — pending_requests currently blocks the downgrade) and notes why on_web_socket_upgrade's id==0 gate is kept despite the inner re-check. - unify on_reload_from_zig's three liveness spellings to js_value_for_dispatch(); fix its stale "declined to adopt" doc. - clear ws.handler.server alongside .app in stop_listening / deinit_if_we_can teardown. - on_drain: copy handler.on_drain to a stack local once, matching the read-once invariant the other five ws callbacks use. - rename AnyServer::js_value_if_strong -> js_value_for_dispatch so a grep for the gate finds every site. - fold the hand-expanded js_gc_route_list_set into cached_value_set_dispatch\! with the other 11 slots.
The on_close defers reach on_websocket_closed through handler.server, so wiping it before app.close() stranded the live-socket count and the idle pass never saw it drained. Fixup for the previous commit.
…503 rig - drop the redundant serverWeak.deref()?.stop(true) in the ws GC-tracing test so it actually exercises the new on_websocket_closed → deinit_if_we_can auto-downgrade instead of masking it. - extract the shared raw-HTTP/1.1 nextResponse parser + socket plumbing + hold-release-GC protocol from the two "late keep-alive 503" tests into a runLateKeepAlive503 helper parameterized on the server-create snippet.
…ia assert_alive ws.close/terminate read handler.server after websocket().end()/.close(), which re-enters on_close and may run a user handler that calls stop(true) (clearing handler.server) — copy it first so the count compensation always fires. on_reload_from_zig is reached via a host_fn (wrapper on JS stack) or the hot_map path (removed before downgrade), so the wrapper is alive even when js_value is Weak; using for_dispatch() there skipped the slot writes after stop+reload, leaving the new handlers unrooted in the heap shadow. Also: trim stale WeakRef test prose and the deinit_running Cell doc.
…t_alive prose The handler-slot writes in on_reload_from_zig switched to assert_alive but the routeList slot write below still re-derived via for_dispatch (None on Weak). reload_static_routes keeps for_dispatch — that path is async and can fire post-downgrade.
Member
Author
|
@robobun adopt |
Collaborator
|
✅ Diff is green. Latest: b9275c7 (re-entrant-close test now captures target ws in CI red across the last five builds is unrelated Windows infra, rotating per run:
Ready for a maintainer to merge. |
Every caller passes Some(live_wrapper); the None arm was dead after beaddea switched reload to js_value_assert_alive(). Drop the Option wrapper and the vestigial 'wrapper already collected' doc clauses. Also shorten the heapStats drain sleep (50ms -> 10ms) and give the subprocess GC-polling tests a 15s budget to match the existing collectContinuously cases; they were brushing the 5s default under debug+ASAN.
server_body.rs on_web_socket_upgrade: take main's request_object_ptr provenance refactor (#33311) and keep this branch's server_js (already resolved via js_value_for_dispatch above). bun-server.test.ts imports: main dropped bunRun; keep this branch's normalizeBunSnapshot.
…ocket.close to_slice_or_null on the reason arg can run user toString(), which may re-entrantly call ws.close(). The inner call sets closed=true and runs the on_websocket_closed compensation this PR added; without re-checking, the outer call then decremented the count again (N->N-2 with >=2 sockets). Re-check the guard after coercion so only one decrement runs. Test observes server.pendingWebSockets: 2->1 with the re-check, 2->0 without it (and 2->2 on the released binary, which had no compensation path).
…open() order Server-side open() ordering for two same-tick connects is poll-order dependent across platforms; only c1 sends "do-close", so capture its peer there instead.
Collaborator
|
Superseded by #34346, which landed the same approach for every server-level callback. Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A handler that closes over
serverforms a cycle the GC can't see through becauseServerConfig/NewServer/Handlerhold it as aStrong/gcProtectroot:box → Strong(handler) → closure env → wrapper → m_ctx → box. The wrapper never finalizes and the box is never freed. Per-route handlers already avoid this viaServerRouteList'sWriteBarriervector — a route handler closing overserveris collectable today. This applies the same to the 11 server-level callbacks.Handler storage —
server.classes.tsvalues:gainsonRequest/onError/onNodeHTTPRequest/onClientError/7×wsOn*/allClosedPromise(H2FrameParser already ships with 18). Native fields stay rawJSValueshadows for hot-path reads.JSServerWebSocket's unusedm_socketslot becomesm_serverso a connected websocket keeps the server wrapper reachable.with_async_context_if_neededis deferred fromfrom_jsto slot-write so the wrapped function is rooted by the slot the moment it exists.WebSocketServerHandler::protect/unprotectare deleted (never balanced on stop today).Wrapper liveness (the existing route-handler post-stop bug) —
js_value.downgrade()moves fromstop()intodeinit_if_we_can()'s idle predicate; the live-websocket count moves ontoNewServerand on_close callsdeinit_if_we_can()via aHandler.serverbackref; dispatch trampolines refuse with 503 oncejs_valueis no longer strong.Also:
DevServer::Dropderefshtml_bundle(ZigRouteBundle.deinit:112; the Rust port missed it).Tests: handler closing over
serveris collected after stop (heapStats; fails on the released binary), reload swap, ws-connected liveness, AsyncLocalStorage init-window stress,BUN_JSC_collectContinuously=1stress, late keep-alive request after stop+GC doesn't crash.Supersedes #32086.
no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/http/bun-server.test.ts