Repository navigation
Conversation
…is aborted DeferredRequest::abort() dropped the SavedRequest without calling deinit(), so the RequestContext refcount that prepare_and_save_js_request_context seeded never reached zero. On the client-disconnect path RequestContext::on_abort's own RequestContextRef drop balances one of the two refs, and the orphaned one left pending_requests stuck, preventing DevServer deinit. Also release the same ref in on_saved_request's defensive 503 early return for the Saved variant (unreachable today but would leak if the Finalized-wrapper gate ever fires).
|
Status: ready for review. Three same-class sites covered ( CI: Reproduced with: bun bd test test/bake/dev/bundle.test.ts -t "client disconnect on a deferred framework route" |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe server now explicitly deinitializes saved requests on abort, argument-computation errors, and finalized-server dispatch. A regression test covers client disconnects during deferred framework-route bundling. Deferred request cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline test-race nit, I traced the refcount accounting for both changed sites and it checks out: at DeferredRequest::abort() the count is 2 (initial +1 from prepare_and_save_js_request_context, +1 from defer_request's ctx.ref_() at DevServer.rs:2212), so saved.deinit() + RequestContext::on_abort's RequestContextRef drop reaches zero — same pattern __deinit and the failed-bundle drain (DevServer.rs:4641-4643) already use. For the on_saved_request 503 early-return, the only Saved caller (drain loop at DevServer.rs:4843-4857) already arms scopeguard::defer! { ctx.deref() }, so saved.deinit() there is the second of two required derefs, not a double-free.
Extended reasoning...
Verified the two-site fix balances refs on every terminal path: abort → 2→0 via saved.deinit + on_abort's RequestContextRef; 503 gate → 2→0 via saved.deinit + caller scopeguard. StrongOptional::deinit-then-Drop is an existing idiom at DevServer.rs:3094 and :4643. Deferring because this is refcount-lifecycle code and the test has the flagged ordering gap.
…e gate Await the local close event and round-trip a plain route so the server's event loop has processed the FIN (and DeferredRequest::abort has run) before the plugin gate is released. Without this the plugin's poll could in principle observe the unblock file before the server handled the disconnect, letting the drain loop take the normal ServerHandler path and masking the regression.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/bake/DevServer.rs:3119-3125— Same-class miss:on_framework_request_with_bundle(DevServer.rs:2671) doescompute_arguments_for_framework_request(...)?while holdingreq: SavedRequestUnionby value — onErrthe?early-returns andreqdrops withoutsaved.deinit(), leaking the sameprepare_and_save_js_request_context+1 this PR fixes here and inon_saved_request. The drain-loop caller'sscopeguard::defer! { ctx.deref() }only balances thedefer_request+1, sopending_requestssticks. Trigger is JS OOM during args construction (same reachability tier as the 503 gate this PR patches), but per REVIEW.md "fix the whole class in the same PR" it should get the sameif let SavedRequestUnion::Saved(mut saved) = req { saved.deinit() }guard — or better, move the release into aDrop/scopeguard so no early-return can skip it.Extended reasoning...
What the bug is
This PR fixes two sites where a
SavedRequestis dropped withoutdeinit(), leaking theRequestContextrefcount it owns:DeferredRequest::abort()andon_saved_request'srespond_stopped_503early-return. A third same-class site is not covered:on_framework_request_with_bundle(DevServer.rs:2607-2692) receivesreq: SavedRequestUnionby value and, at line 2671, calls:let args = unsafe { Self::compute_arguments_for_framework_request(...) }?;
compute_arguments_for_framework_requestreturnsJsResult<FrameworkRequestArgs>and has multiple internal?sites on JS-allocation calls (create_utf8_for_jsat 2472,create_empty_arrayat 2499,put_indexat 2514/2528,transfer_to_jsat 2514/2531,to_jsat 2559,generate_css_js_arrayat 2579). If any of them returnsErr— reachable at least on JS OOM or a termination trap — the?at 2671 early-returns fromon_framework_request_with_bundleandreqis dropped by fall-through.The specific code path
The only caller that passes
SavedRequestUnion::Savedis the drain loop at DevServer.rs:4841-4860:Handler::ServerHandler(saved) => { let response = saved.response; let ctx = saved.ctx; scopeguard::defer! { ctx.deref() }; // balances defer_request's ctx.ref_() dev.on_framework_request_with_bundle( req.route_bundle_index, SavedRequestUnion::Saved(saved), // moves the initial +1 in response, )? }
At this point
ctxholds two refs: the initial +1 fromprepare_and_save_js_request_context(owned bysaved) and the +1 fromdefer_request'sctx.ref_()at DevServer.rs:2212 (balanced by thescopeguard::defer!). Whenon_framework_request_with_bundlereturnsErr,savedhas already been moved in and drops there; the caller's scopeguard fires and drops the count 2→1. Nothing releases the remaining +1.Why existing code doesn't prevent it
SavedRequesthas noDropimpl — only an explicitdeinit()(mod.rs:4037). Itsjs_request: StrongOptionalreleases viaDrop, butctx: AnyRequestContextisCopy, so plain drop is a no-op for the refcount — exactly the mechanism this PR's own comment at DevServer.rs:3119-3125 documents. On the success path this ref is consumed byon_saved_requesthandingctxinto theRequestContextlifecycle (on_response→deinit), but the?-error path bypasseson_saved_requestentirely. The drain loop is the first request to a route, so thecached_*lookups incompute_arguments_for_framework_requestare allNoneand every fallible?site is live. The other caller at DevServer.rs:1877 passesSavedRequestUnion::Stack, which does not own a ctx ref, so only the drain-loop path is affected.Step-by-step proof
- Client requests a framework route whose bundle is not yet built →
prepare_and_save_js_request_contextcreatesRequestContextatref_count=1,defer_requestbumps to 2 and storesHandler::ServerHandler(saved). - Bundle finishes; drain loop takes
saved(owning the initial +1), armsscopeguard::defer! { ctx.deref() }for thedefer_request+1, and callson_framework_request_with_bundle(SavedRequestUnion::Saved(saved), ...). - Inside,
compute_arguments_for_framework_requesthits e.g. JS OOM increate_empty_array→Err. ?at line 2671 returnsErrfromon_framework_request_with_bundle;req(holdingsaved) drops with nodeinit()→ctxref not released.- Back in the drain loop, the scopeguard fires:
ref_count2→1. The?propagates out of the drain loop. ref_countis stuck at 1;on_request_complete()never runs;pending_requestsnever decrements;deinit_if_we_can()never frees the server.
Impact
Identical to the bug this PR fixes: a leaked
RequestContextpool slot and apending_requestscount that never reaches zero, so the server never deinits on graceful stop. The trigger (JS OOM / termination trap during args construction) is in the same reachability tier as therespond_stopped_503gate this PR patches — the PR description itself calls that gate "documented-unreachable today" but fixes it anyway for the class.How to fix
Mirror the
on_saved_requestfix — either wrap the?site:let args = match unsafe { Self::compute_arguments_for_framework_request(...) } { Ok(a) => a, Err(e) => { if let SavedRequestUnion::Saved(mut saved) = req { saved.deinit(); } return Err(e); } };
or (preferable, given three sites now need the same manual call and the comment-cop bot is already flagging the paragraph-long justifications) move the
ctx.deref()into aDropimpl / scopeguard onSavedRequestso no early-return can skip it. Per REVIEW.md "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep)", this sibling belongs in the same change. - Client requests a framework route whose bundle is not yet built →
Same class as the other two sites: the ? at DevServer.rs:2671 drops req (and its Saved payload) without deinit() if args construction fails, leaking the prepare_and_save_js_request_context +1. The drain-loop scopeguard only balances the defer_request +1.
There was a problem hiding this comment.
I didn't find any bugs, but this is refcount balancing in RequestContext lifecycle across bake and server, with an explicitly deferred design question (impl Drop for SavedRequest) and acknowledged same-class sites (on_plugins_rejected / finalize_bundle OOM) left for follow-up — worth a human look.
What was reviewed:
DeferredRequest::abort()→saved.deinit(): verified__deinit()and the drain loop seeHandler::Abortedafterward, so no double-deref;on_abort'sRequestContextRefguard still holds a ref whilectx.deref()runs (2→1, then 1→0).on_framework_request_with_bundleerror path andon_saved_request503 gate: the addedsaved.deinit()pairs with the drain-loopscopeguard::defer!{ ctx.deref() }to release both refs;StrongOptional::deinit()is idempotent with itsDrop(handle.take()).- Test: prior sync and try/finally nits are addressed in the current diff; poll deadline scales with
WAIT_MULTIPLIER.
Extended reasoning...
Overview
Three-site SavedRequest::deinit() fix for a leaked RequestContext refcount when a deferred bake framework-route request is aborted or errors before reaching on_saved_request's normal path. Touches src/runtime/bake/DevServer.rs (DeferredRequest::abort, on_framework_request_with_bundle), src/runtime/server/mod.rs (on_saved_request 503 early-return), and adds a bake dev test that gates bundling with an onLoad plugin, aborts a raw-TCP request mid-bundle, and relies on the harness's gracefulExit() deinit-count check.
Security risks
None. This is internal refcount bookkeeping on the dev-server request path; no parsing of untrusted input, no auth/crypto.
Level of scrutiny
High. AnyRequestContext is a Copy tagged pointer whose refcount is balanced manually across bake's deferred-request pool, the server's on_abort RequestContextRef guard, and the drain-loop scopeguard. I traced each new deinit() against the existing paths: after abort() swaps self.handler to Aborted, both __deinit() (via deref_()) and the finalize_bundle drain loop match Handler::Aborted and skip, so the added deref is the only one for the SavedRequest-owned +1; on_abort's guard releases the caller's +1. On the compute_arguments_for_framework_request error path, the drain-loop scopeguard captured ctx by Copy before the move, so its deref (defer_request +1) plus the new saved.deinit() (initial +1) balance. StrongOptional::deinit() uses handle.take(), so the subsequent Drop is a no-op — no double-destroy of the JS strong.
Other factors
The PR body explicitly defers the structurally-right fix (impl Drop for SavedRequest) because it intersects the bundle-failure double-deinit() pattern at DevServer.rs:4636-4643 and the separately-tracked on_plugins_rejected path — a maintainer should weigh in on whether to land this targeted patch or fold it into that refactor. My two prior inline nits (destroy/unblock synchronization, socket try/finally) are both addressed in the current diff. The test looks deterministic now (awaits close, round-trips a plain harness route before releasing the gate, poll deadline scaled by WAIT_MULTIPLIER). Given this is manual refcount work in the most-blocked review category, deferring rather than auto-approving.
There was a problem hiding this comment.
Both earlier nits are addressed and I have nothing further on the test. The three saved.deinit() additions trace correctly against the SavedRequest.ctx +1 and the defer_request ctx.ref_() +1; the two other abort() callers (finalize_bundle OOM defer and the plugins-rejected drain) go from leaking both refs to leaking one, which matches the follow-up scoping in the PR body. Deferring to a maintainer only for sign-off on that scoping and the deferred impl Drop for SavedRequest refactor.
What was reviewed:
- Traced ctx refcount on the abort path (initial +1 via
SavedRequest, +1 fromdefer_requestline 2212) —saved.deinit()+on_abort'sRequestContextRefguard reach 0;__deinit()seesAbortedso no double-deref. - Checked the
on_framework_request_with_bundleerror arm against the drain-loopscopeguard::defer! { ctx.deref() }at DevServer.rs:4858 — two derefs balance two refs. - Confirmed the bundle-failure double-
deinit()at DevServer.rs:4647 +__deinit()is untouched. StrongOptional::deinit()idempotent withDrop, sosaved.deinit()followed by scope-exit is safe.
Extended reasoning...
Overview
Three-site refcount fix in the bake dev server: DeferredRequest::abort(), on_framework_request_with_bundle's error return, and on_saved_request's finalized-server 503 gate now call SavedRequest::deinit() (releases js_request strong + ctx.deref()) where they previously dropped the struct without releasing the Copy AnyRequestContext ref. Plus a bake-harness regression test that gates bundling with an onLoad plugin, aborts a raw-TCP client mid-bundle, and relies on the harness gracefulExit() deinit-count assertion.
Security risks
None. Dev-server request-lifecycle refcounting; no parsing of untrusted input, no auth/crypto surface.
Level of scrutiny
High — this is native refcount management, REVIEW.md's most-blocked category. I traced each changed site against the two refs on ctx (the SavedRequest-owned +1 and the defer_request ctx.ref_() +1 at DevServer.rs:2212):
abort()(DevServer.rs:3129):saved.deinit()releases theSavedRequest+1;RequestContext::on_abort'sRequestContextRefguard releases the other; the later drain-loop__deinit()seesHandler::Abortedand does nothing. Net 2 derefs, no double-free. The two non-on_abortcallers ofabort()(DevServer.rs:3856, 6157 — the OOM cleanup defer and the plugins-rejected drain) previously leaked both refs and now leak one; the PR body scopes those to a follow-up alongside theimpl Droprefactor.on_framework_request_with_bundleerror arm (DevServer.rs:2673-2679):saved.deinit()releases theSavedRequest+1; the drain-loop caller'sscopeguard::defer! { ctx.deref() }at 4858 releases thedefer_request+1.Stackvariant has no owned ctx, so theif let Savedis right.on_saved_request503 gate (mod.rs:896): documented-unreachable defensive path; matches the other two.- The existing bundle-failure double-
deinit()pattern at DevServer.rs:4647 (explicitsaved.deinit()+__deinit()viaderef_()scopeguard) is not touched, andStrongOptional::deinit()is idempotent with itsDrop, so no new double-free there.
Other factors
Both of my earlier test nits (destroy/unblock ordering, socket try/finally) and the comment-cop paragraph-length flags were addressed; all threads are resolved. The test now round-trips a plain harness route after await once(sock, "close") so the server has processed the FIN before the plugin gate opens, and the poll deadline scales with WAIT_MULTIPLIER. The Rust change matches the existing __deinit() pattern at DevServer.rs:3104 exactly. I'm deferring rather than approving only because the PR explicitly scopes out two sibling leak sites and a structural Drop refactor for a follow-up — a maintainer familiar with the DeferredRequest pool should confirm that scoping.
This branch has not been deployed
1 participant
Problem
A framework-route request that is deferred while its bundle is building (stored as a
SavedRequestinsideDeferredRequest::ServerHandler) leaks itsRequestContextwhen the client disconnects mid-bundle. The server'spending_requestsnever returns to zero, sodeinit_if_we_can()never frees theNewServerand the bake harness graceful-exit check fails with:Repro
Cause
SavedRequestcarries anAnyRequestContext(aCopytagged pointer), so RustDropdoes nothing for the ref it owns;SavedRequest::deinit()exists to release it but three sites dropped the struct without calling it:DeferredRequest::abort()just diddrop(saved)afterset_signal_aborted. On client disconnectRequestContext::on_abortruns this callback and then drops one ref via its ownRequestContextRefguard, leaving the count at 1; the drain loop later seesHandler::Abortedand skips the entry.on_saved_request's defensiverespond_stopped_503early-return forSavedRequestUnion::Saved(documented-unreachable today but would leak if it ever fires).on_framework_request_with_bundle's?oncompute_arguments_for_framework_requestdropsreqon error before reachingon_saved_request; the drain-loop scopeguard only balances thedefer_request+1.Fix
Call
saved.deinit()at all three sites.SavedRequest::deinit()releases thejs_requeststrong (idempotent withStrongOptional::Drop) andctx.deref(); withRequestContext::on_abort's existingRequestContextRefdrop the count reaches zero on the client-disconnect path,on_request_complete()runs, andpending_requestsdecrements.Why not
impl Drop for SavedRequest?A
Dropthat callsself.ctx.deref()is the structurally right shape and would make future early-returns safe by construction. I kept this PR to targeteddeinit()calls because the existing bundle-failure drain (DevServer.rs:4636-4643) andDeferredRequest::__deinit()rely on double-deinit()to balance both the initial anddefer_requestrefs; moving toDropchanges that pattern and theon_plugins_rejected/ finalize_bundle OOM paths (which still leave thedefer_request+1 unbalanced and never write a response, tracked separately) belong in the same refactor. Happy to follow up with theDrop+take_ctx()change across all sites.Test
test/bake/dev/bundle.test.tsgains a test that uses anonLoadplugin to hold bundling open deterministically, opens a raw TCP request to/, waits for the plugin to signal it has been entered (request is deferred), destroys the socket, round-trips a plain harness route so the server has processed the FIN, then unblocks bundling and fetches again. The bake harnessgracefulExit()assertsgetDevServerDeinitCount()increments afterstop(true); without the fix that check throws.Also re-ran
test/bake/deinitialization.test.tsandtest/bake/dev/plugins.test.tswith the change.