Bun.serve: cancel the piped upstream fetch when the client aborts while backpressured - #36700
Bun.serve: cancel the piped upstream fetch when the client aborts while backpressured#36700robobun wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 3 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 (3)
Comment |
|
Rebased onto main 5ad56ed at 9d70b74 (see #36700 (comment)). Fail-before on main: Build #88706 on 96b99d4: |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the duplicate flag for #35552: that PR applies the same |
|
Updated 3:05 AM PT - Aug 4th, 2026
❌ @robobun, your commit 96b99d4 has some failures in 🧪 To try this PR locally: bunx bun-pr 36700That installs a local version of the PR into your bun-36700 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but this touches the intrusive refcount lifecycle on Bun.serve's abort path, so it warrants a human look at the ref balance.
What was reviewed:
- Traced the sink-install
ref_()(RequestContext.rs:3227) against the newderef()inon_abortand theRequestContextRefinend_chunk— the.take()in both plusbyte_stream = Noneinend_chunkkeeps them mutually exclusive, no double-deref. cancel_from_sinksetssink = Nonebefore closing the producer, so it can't re-enterend_chunk;response_body_readable_stream_refkeeps the ByteStream alive across theBackRefuse and itsdeinit()is idempotent for the fall-through intofinalize_without_deinit._ref(line 1385) holds one ref across the whole body, sothis.deref()at 1441 can't drop the count to zero mid-function.resp.reset_timeout()matches the uWSonWritablecontract;respis the live handle uWS just passed in and the aborted check runs first.
Extended reasoning...
Overview
Three changes to src/runtime/server/RequestContext.rs plus a new subprocess-fixture test:
on_abort: whenbyte_streamis set,.take()it, callcancel_from_sink(None), deinitresponse_body_readable_stream_ref, andderef()the sink-install ref before falling through to the existingis_dead_request()tail.end_chunk: clearthis.byte_stream = Noneso the field precisely tracks whether the sink-install ref is outstanding.on_writable_byte_stream: callresp.reset_timeout()beforeresume(), since uWS zeroes the socket timeout before invoking the writable callback andresume()only posts a task rather than writing synchronously.
Security risks
None identified. This is resource-lifecycle cleanup on client disconnect; no parsing of untrusted input, no auth/crypto surface. The failure mode being fixed is a resource leak (parked upstream fetch + RequestContext until idle timeout), not a safety violation.
Level of scrutiny
High. This is the intrusive-refcount teardown path for every Bun.serve request that pipes a native ByteStream body. REVIEW.md flags "reference counts provably balanced on every terminal path" as the most-blocked category. I traced the balance and believe it's correct:
- The sink-install ref is taken exactly once at RequestContext.rs:3227 alongside
byte_stream = Some(...). - It was previously released only via
end_chunk'sRequestContextRef. The newon_abortbranch releases it on the abort path instead; both sites.take()/ null the field, so at most one runs. finalize_without_deinitstill handles a residualbyte_streamviabyte_stream_unpipewithout deref'ing (correct — it runs when the count is already zero).cancel_from_sinkdetachesSinkHandlefirst, so the producer close cannot re-enterwrite_chunk/end_chunkon this context.readable_stream::Strong::deinitis idempotent, so the fall-through intofinalize_without_deinitcalling it again is safe.
That said, someone who owns the #36087 SinkHandle rewrite should confirm this is the intended abort contract for SinkHandle::ServerResponse — in particular that cancel_from_sink's producer.close(None) cannot run JS that would need any_js_calls.set(true) here.
Other factors
- ASAN debug build passes the new test; the fail-before evidence on main f91d5c9 shows
cancelled: falseas expected. - The
release without fixevidence used a canary at1498d7b(pre-#36087) where pulls hit the cap — that's the system-bun check, not main; the release-with-fix log was truncated before showing the test result, so the release-mode assertionpulls < CAP_CHUNKS/2hasn't been visibly confirmed on a fast release build. - CI build #87084 is still in progress; the one failure so far (
bun-upgrade.test.tson Windows aarch64) is unrelated. - #35552 is a stale duplicate on pre-#36087 code; this PR supersedes it.
There was a problem hiding this comment.
Beyond the inline nits, I traced the new on_abort branch for a double-deref of the sink-install ref: cancel_from_sink() clears ByteStream::sink before anything else so end_chunk cannot fire afterward, and the new end_chunk line nulls this.byte_stream so on_abort skips the branch after normal completion — the pairing looks balanced under ASAN. Deferring to a human for the RequestContext ref-count/abort-path change and to reconcile with #35552.
Extended reasoning...
The core fix — taking byte_stream, calling cancel_from_sink(), deiniting response_body_readable_stream_ref, and dropping the sink-install ref inside on_abort — is ref-counting on the abort path of RequestContext, which REVIEW.md flags as the most-blocked category. I verified the ref taken at do_render_with_body (line 3228) is now released on exactly one of end_chunk or the new on_abort branch (each clears byte_stream and cancel_from_sink nulls the ByteStream's sink handle first), and that _ref at the top of on_abort keeps this alive across the new this.deref(). That said, this is production-critical server lifecycle code with a duplicate open PR (#35552) to reconcile, so it warrants a human sign-off rather than bot approval.
There was a problem hiding this comment.
Both earlier nits are addressed in afc89c6 and I didn't find anything further, but this adds a deref() on the client-abort teardown path in RequestContext, so it's worth a human look at the ref-count balance.
What was reviewed:
- Traced the sink-install ref (
ref_()at theSource::Bytesinstall site) to its two release sites —end_chunkand the newon_abortblock — and confirmed they're mutually exclusive:end_chunknow nullsbyte_stream, andcancel_from_sink()clearssinkbefore anything that could re-enter, soend_chunkcan't be reached after the abort path derefs. - Checked that
this.deref()at the new site can't drop the last ref before the fall-throughis_dead_request()read —_refstill holds one at that point. response_body_readable_stream_ref.deinit()is idempotent, so the second call fromfinalize_without_deiniton the same abort path is fine.
Extended reasoning...
Overview
The PR adds a 6-line block to RequestContext::on_abort that handles the case where this.byte_stream is set (the native SinkHandle::ServerResponse path used when Bun.serve returns a fetch() Response directly). It calls cancel_from_sink() on the ByteStream, deinits the held response_body_readable_stream_ref, and releases the sink-install ref taken in do_render_with_body. A one-line change to end_chunk clears this.byte_stream so the field tracks whether that ref is still outstanding, keeping the two release sites mutually exclusive. A new subprocess-based test reproduces the parked-upstream leak and asserts both cancellation and that the proxy child stayed alive.
Security risks
None identified. This is resource-lifecycle cleanup on the server abort path; no parsing, auth, or trust-boundary changes.
Level of scrutiny
High. Per REVIEW.md this is the most-blocked category — ref-counting and teardown on an abort path in native server code. The change is small and the mechanism is clearly explained, but a mistake here is a UAF or a leak that only reproduces under load. I traced the ref balance and re-entrancy and it looks correct: cancel_from_sink sets sink = None before running any callbacks, so sink.end()/end_chunk cannot be reached from inside it; byte_stream.take() and the new end_chunk null keep the two deref() sites exclusive; and the scope-exit _ref guard in on_abort guarantees at least one ref remains across the fall-through to is_dead_request()/finalize_without_deinit(). Still, a maintainer who owns the RequestContext lifecycle should confirm this matches the intended ownership model post-#36087.
Other factors
Both nits from the previous pass were addressed in afc89c6 (the redundant reset_timeout() was reverted; the test now asserts proxy.exitCode/signalCode are null so a crash-on-abort would fail it). The author reports fail-before on main and pass-after, and that the sibling serve-async-stream-client-abort.test.ts still passes. The bug hunter found nothing new this run. There's an open near-duplicate (#35552) targeting the pre-#36087 code that should be closed if this lands.
|
@robobun is this still needed? if yes rebase else close |
…le backpressured Follow-up to #36087, carrying over the abort-path fix from the now-closed #35547 that the SinkHandle rewrite did not pick up. on_abort handles this.sink (the JS-stream sink path) but not this.byte_stream (the native SinkHandle::ServerResponse path). When a client aborts while the ByteStream sink is paused for backpressure, the upstream FetchTasklet stays in Paused with no wake path and no cancellation, and the ref taken when the sink was installed is never released (end_chunk only runs via sink.end(), which requires a drain that never comes on a closed socket). on_abort now calls cancel_from_sink() on the held ByteStream (detaches the sink and closes the producer, aborting the upstream fetch), deinits the Strong, and drops the sink-install ref. end_chunk clears this.byte_stream so the field tracks whether that ref is still held, keeping the two release sites mutually exclusive.
afc89c6 to
9d70b74
Compare
|
Still needed on current main (5ad56ed): Rebased onto 5ad56ed in 9d70b74 (branch reset + force-push). The structure changed since the original commit (Cell-wrapped fields,
Fail-before/pass-after verified locally; |
There was a problem hiding this comment.
All three prior review points are addressed in 96b99d4; no new findings on the rebased version. Deferring to a human for the final look since this is ref-counting in Bun.serve's abort path.
What was reviewed:
- Traced the sink-install ref (line 3272
ref_()) to its two release sites —end_chunk'sRequestContextRef::adoptand the newon_abortderef()— and confirmed they're mutually exclusive:end_chunkclearsbyte_streambefore any re-entrant call, andcancel_from_sinkclearsSinkHandlefirst soend_chunkcannot fire afteron_abort. - Checked
readable_stream::Strong::deinit()is idempotent (*self = Empty), so the seconddeinitinfinalize_without_deinitafter the newon_abortblock is a no-op, not a double-free. - Test now asserts proxy
exitCode/signalCodeare null, so a crash-on-abort would fail it.
Extended reasoning...
Overview
The PR adds an 8-line branch to RequestContext::on_abort that mirrors the existing this.sink handling for the this.byte_stream case (native SinkHandle::ServerResponse path used when a handler returns fetch(upstream) directly). It calls cancel_from_sink() on the held ByteStream, deinits the response body's ReadableStreamStrong, and drops the sink-install ref taken at do_render_with_body line 3272. A one-line change in end_chunk moves byte_stream.set(None) to the top so the field tracks whether that ref is still outstanding, and the now-dead conditional set(None) in the error branch was removed per my last review comment. The test spawns a proxy child, stalls a raw-socket client to force backpressure, destroys the socket, and polls for the upstream ReadableStream.cancel() to fire — with post-assertion liveness checks on the proxy child.
Security risks
None identified. This is teardown/resource-release on client disconnect; no new input parsing, no auth surface.
Level of scrutiny
High — this is manual ref-counting in Bun.serve's abort path, the most-blocked category per REVIEW.md. I traced the ref balance:
do_render_with_bodytakes +1 (this.ref_(), line 3272) and storesbyte_stream.- Normal completion:
end_chunkadopts that +1 viaRequestContextRef::adopt(line 3392), and now clearsbyte_streamat line 3396 so any lateron_abortskips the new block. - Abort while parked:
on_aborttakesbyte_stream(line 1499) andderef()s (line 1504).cancel_from_sinksetsSinkHandle::Nonebefore callingon_cancel/producerclose(), so the ByteStream cannot reachsink.end()→end_chunkafterwards. finalize_without_deinit(whichon_abortmay fall through to) seesbyte_stream == Noneand itsresponse_body_readable_stream_ref.deinit()is idempotent.
The pre-existing _ref = RequestContextRef::adopt at on_abort line 1447 is unchanged and accounts for a separate ref; the new deref() releases only the sink-install ref. I did not find a path where both release sites fire on the same ref.
Other factors
This PR has been through three review rounds from me (redundant reset_timeout, missing proxy-liveness assertion, dead byte_stream.set(None)), all addressed and resolved. Jarred asked whether it's still needed a few hours ago and the author rebased onto current main in response, so a maintainer is already engaged. Given the ref-counting sensitivity and active maintainer attention, deferring rather than approving.
2 participants
Follow-up to #36087, carrying over the abort-path fix from the now-closed #35547 that the SinkHandle rewrite did not pick up.
Repro
Upstream streams a 64 MiB body. Client pauses after the first chunk so the proxy's
write_chunkreturnsBackpressureand theByteStreamparks withsink_paused = true. Client then destroys the socket.On main (f91d5c9): the upstream
ReadableStream'scancel()never fires; pulls stay at whatever count they reached when backpressure engaged, and the proxy'sRequestContextplus the upstream FetchTasklet/socket stay parked until the upstream server's idle timeout.Cause
on_aborthandlesthis.sink(the JS-stream sink path) but notthis.byte_stream(the nativeSinkHandle::ServerResponsepath). Theelsebranch'sresponse_body_stream()returnsNonebecause the body was already set toUsed. The sink-install ref taken indo_render_with_bodyis only dropped inend_chunk, which is reached viasink.end()fromByteStream::resume(), andresume()only runs fromon_writable_byte_stream, which never fires on a closed socket.Fix
on_abort: whenbyte_streamis set, callcancel_from_sink()on it (detaches the sink and closes the producer, which aborts the upstream fetch), deinit the heldresponse_body_readable_stream_ref, and drop the sink-install ref.end_chunk: clearthis.byte_streamso the field tracks whether that ref is still held, keeping the two release sites mutually exclusive.Verification
test/js/bun/http/serve-fetch-body-abort-backpressure.test.tsstalls a raw-socket client against a proxy child, destroys the socket while the upstream is held paused, and asserts the upstreamReadableStream.cancel()fires and the proxy child is still alive. On main it fails with{ cancelled: false, pulls: ~22 }; with the fix it passes in ~1s.[review] gate passed · iteration 17 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 17
evidence per changed file