Bun.serve: release the body stream of a Response whose client aborted mid-stream - #41080
Conversation
… mid-stream Since #39743, on_abort reclaims the stream pump's promise cell and frees the context at once. The pump's settle reaction then finds no context, so handle_resolve_stream/handle_reject_stream, which detach the stream from the Response body and mark the body used, never run for an aborted streaming response. The body kept its strong ref on the stream. When the stream can reach the Response (hono's streamSSE keeps a WeakMap from the body stream to its Context, which holds the Response), that ref closes a cycle through a GC root: every client disconnect from an SSE endpoint leaked the Response, the Request, the stream, its queued chunk, and the handler closure. Do that cleanup in finalize_without_deinit, the teardown every context goes through, so a settle that arrives after the context is gone has nothing left to release.
|
Reproduced with hono 4.12.28 A hono-free repro: a handler that returns |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. Walkthrough
ChangesResponse stream cleanup
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized change releases aborted response body streams and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, cause, fix, correctness, verification, regression tests, and known unrelated failures. It does not use the exact template headings, but it provides the required change summary and verification details. Comment |
| if let Some(resp) = self.response_mut() { | ||
| if let Some(stream) = resp.get_body_readable_stream() { | ||
| stream.value.ensure_still_alive(); | ||
| resp.detach_readable_stream(global_this); |
There was a problem hiding this comment.
Does this execute JavaScript? Do we need to check mutator state to avoid entering JavaScript? And, do we need to potentially drain microtasks if not?
There was a problem hiding this comment.
No JS runs here: a slot read (m_stream, the pure ReadableStreamTag__taggedStream), a write barrier plus a Strong release (detach_readable_stream), a native NewSource::cancel (stream.done(), not ReadableStream::cancel, so the source's JS cancel() is not invoked; the sink abort already ran it), and a PendingValue drop. Same four calls as handle_reject_stream after sink teardown. Every path into finalize_without_deinit is an event-loop path (uWS callback, promise reaction, reclaim_promise_cell, SavedRequest::deinit, DeferredDerefTask), never a GC sweep, and this function already fires request.signal listeners below. Nothing here settles a promise, so there is no new microtask to drain. Details in the PR comment.
|
No JavaScript runs in this block. Each call is a slot read or write, or a native handle release:
These are the same four calls Mutator state: every path into Microtasks: the block settles no promise and enqueues nothing, so there is nothing new to drain. The callers that can run JS here already drain ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it adds cleanup to finalize_without_deinit — a multi-caller teardown path in the Bun.serve request context lifecycle — a human look would still be worthwhile.
What was reviewed:
- The new detach block mirrors the existing
handle_resolve_streamcleanup (RequestContext.rs:2912-2921) exactly, and is placed beforeresponse_jsvalue.unprotect()/response_weakrefclear, soresponse_mut()is still valid there. - Idempotence of the added block: on repeat calls
response_weakrefis alreadyEMPTYsoresponse_mut()returnsNone; theLocked → Usedreplacement is gated so non-stream bodies are untouched. - The new leak test: WeakRef + bounded
Bun.gc(true)poll (accepted pattern),port: 0,using server, awaitsstopAndAssertDrained(a real condition) rather than sleeping, and covers both sync and async handler shapes.
Extended reasoning...
Overview
The PR fixes a memory leak where a client abort of a streaming Response leaves the Body::Value::Locked's readable::Strong on the ReadableStream forever, because on_abort reclaims the promise cell so the pump's later handle_resolve_stream/handle_reject_stream reactions become no-ops and never detach the body's stream. The fix adds a small block to finalize_without_deinit in src/runtime/server/RequestContext.rs that detaches the body's readable stream and replaces a Locked body value with Used, releasing the Strong. A parameterized GC-collection test is appended to test/js/bun/http/serve-pending-promise-abort-leak.test.ts reproducing the hono-style stream → Response WeakMap cycle for both sync and async handlers.
Security risks
None. This is a lifecycle/GC-root cleanup change with no user-input parsing, no auth/crypto, and no new external surface.
Level of scrutiny
High. Per the repository's own review guidance, native memory safety — specifically ref/deref balance across every terminal path (success, error, cancellation, finalize) — is the most-blocked review category. finalize_without_deinit is explicitly documented as reachable multiple times and from more than one teardown route (both the abort sink path via later deref, and non-sink paths). The added code is a copy of the existing handle_resolve_stream body-detach block (lines 2912-2921), which is reassuring, and it is placed while response_jsvalue is still protected and before response_weakref is cleared, so response_mut() is guaranteed valid. It is also idempotent (subsequent calls see response_weakref == EMPTY), and the Locked-only gate means non-streaming success paths are unaffected. Still, changes to this function's behavior fan out across every context teardown, so a maintainer familiar with the RequestContext lifecycle should confirm no non-abort caller of finalize_without_deinit relied on the body remaining Locked afterward.
Other factors
The PR description is unusually thorough: it names the exact regressing PR, cites the specific line where the Strong is stored (do_render_stream), and provides heap-snapshot evidence (retained stream under StrongRootBlock) as required by CLAUDE.md rule #15. The test follows the accepted leak-test pattern (WeakRef + bounded Bun.gc(true) poll loop with sleep(1), port: 0, using disposal, awaits server.stop() as the observable drained condition). The bug hunter ran to dry_streak with zero findings and no ruled-out candidates. No CODEOWNERS entry covers the changed paths. Given all of that, this is close to approvable, but the teardown-path sensitivity tips it to a human confirmation.
…41130) ### Problem - The `mordant` job fails on every PR since 2026-09-01: one `generic_body_not_generic` finding over the baseline, named as `src/runtime/server/RequestContext.rs:4439` (`get_remote_socket_info`). The baseline holds 8, the file has 9. Mordant names the last finding in the file. - The new one is the block #41080 added to `finalize_without_deinit`, a copy of the block in `handle_reject_stream`. It uses no type parameter and is compiled eight times. #41082 wrote the baseline ten minutes before #41080 merged. ### Fix - Move the block into `release_body_stream`, a free `#[inline(never)]` function called from both places. One copy instead of sixteen. - No behavior change: the statements and their order are the same. - The file drops from nine findings to seven, so `mordant-baseline.toml` goes from 8 to 7. Checked with the pinned mordant: 7 reports nothing over, 6 reports one over. - Verified: seven stream, abort and leak serve tests (85 pass, listed in Notes), `serve.test.ts`, and the `mordant` job on this PR. ### Background - `RequestContext<ThisServer, SSL, DEBUG, MUX>` is the per-request state of `Bun.serve`. It has eight monomorphizations. Every method body is compiled once per instantiation. - `generic_body_not_generic` is a mordant lint. It flags a region of a generic function that uses no type parameter (30 MIR statements here) and asks for a non-generic function that takes what the region reads. - `mordant-baseline.toml` is a ratchet: per lint and file, the number of findings that predate the job. A file over its count fails the job. A fixed finding lowers the entry. - #40423 carries f0b5a3b, which hoists `get_remote_socket_info` itself. It touches other lines, so both can land. <details><summary>Notes</summary> No `test/` change. The diff moves two identical blocks into one function and changes no statement, so no test can fail before it and pass after it. The regression check for this PR is the `mordant` job itself: red on main and on every open PR, green here. The behavior of the moved block is pinned by the existing tests listed below, in particular `serve-pending-promise-abort-leak.test.ts` (#41080, the `finalize_without_deinit` caller) and `serve-stream-reject-flush-leak.test.ts` (the `handle_reject_stream` caller). Tests run with the debug build: `test/js/bun/http/serve-pending-promise-abort-leak.test.ts`, `serve-stream-reject-flush-leak.test.ts`, `serve-async-stream-client-abort.test.ts`, `serve-response-stream-sink-leak.test.ts`, `serve-stream-body-error.test.ts`, `serve-error-handler-stream.test.ts`, `serve-body-leak.test.ts`: 85 pass. `serve.test.ts` on this container: 295 pass, 2 fail (`root range port` and `/bun:info to loopback clients`). Both fail on main here too; #41080 noted the same two. How the cause was found: with the pinned mordant (`cargo dylint --all -p bun_runtime`), reverting only the `RequestContext.rs` hunk of e5a18d5 leaves nothing over the baseline. With the baseline line commented out and `--cap-lints warn`, the nine findings in the file are `cancel_unread_body`, `render_missing_invalid_response`, `finalize_without_deinit` (30 of 373 statements, the #41080 block), `do_sendfile`, `handle_reject_stream` (30 of 246 statements, the same block), `render_metadata`, `do_write_status`, `on_buffered_body_chunk` and `get_remote_socket_info`. Mordant reports the findings past the baseline count in file order, which is why the job names the last one. With this PR and f0b5a3b both on main, the file has six findings under a baseline of seven. </details>
|
The sync and async 'releases the body stream' tests from this PR fail in about 1 of 250 CI builds with one of eight streams alive (builds 109108 and 110257). The retainer could not be found or reproduced locally. #41409 makes the test print the retainer chain on the next failure and lists the file for a retry. |
3 participants
Problem
Responseleaves its body holding a strong GC ref on theReadableStream. hono'sstreamSSEmaps that stream back to itsContext, which holds theResponse, so the ref closes a cycle through a GC root: every SSE disconnect leaks theResponse, theRequest, the stream, and the handler closure. bun 1.4.0 is clean; every canary since Bun.serve: tear down an aborted request's context at abort instead of waiting for GC #39743 (2026-08-20) leaks.on_abortreclaims the stream pump's promise cell and frees the context (RequestContext.rs:1516). The pump's settle reaction then finds no context, sohandle_resolve_stream/handle_reject_streamnever detach the stream from the body. TheStrongthatdo_render_streamstored there (RequestContext.rs:2239) is never released.Fix
finalize_without_deinitdetaches the body stream from theResponseand marks the body used, as the settle handlers do. Every context teardown goes through it.cancel(), nothing reads it any more. Only the body's hold on it remained.test/js/bun/http/serve-pending-promise-abort-leak.test.ts, 2 new tests (sync and async handler); without the fix 8 of 8 streams stay alive. Alsoserve.test.ts,bun-server.test.ts, and theserve-*stream, abort, and leak files.Background
Responseis sent by a pump whose promise the context subscribes to through aNativePromiseContextcell. The settle handlers tear the sink down and clean the body.on_abortreclaim that cell so an aborted request is freed at abort time instead of at GC. The settle still runs later, as a no-op.Body::Value::Lockedholds areadable_stream::Strongon the stream, a GC root until the body is detached or theResponseis collected.Notes
Found while following up on #41011: with the SSE loops now ending promptly, the reporter's app churned reconnects faster and the growth became obvious. Measured with hono 4.12.28 and an async middleware: 600 disconnects add about 17,000 live objects (about 28 per disconnect) that survive
Bun.gc(true), on the canary before #41011 (a6c4cc2) and after it (2bf11c5) alike; bun 1.4.0, 1.3.14 and 1.3.0 stay flat. A heap snapshot shows the retainedReadableStreamunder aStrongRootBlockand theContextunder(GC roots)(the ephemeron value), with#resand#rawRequesthanging off it.The minimal repro needs the stream to reach the
Response; without that, theResponseis collected and its body drops theStrong. So a plainnew Response(stream)handler does not show it, and hono does.With a sync handler hono returns the
Responsedirectly and the leak did not reproduce in the hono harness, but the minimal repro leaks for both sync and async handlers. Both shapes are in the test.Known failures on this container that also fail on main:
serve.test.tsroot range port and/bun:infoloopback, andbun-server.test.tsIPv6 listen.