Repository navigation
Bun.serve: end the request when an HTTP/2 or HTTP/3 stream closes after a streamed response - #42844
Conversation
…er a streamed response On HTTP/2 and HTTP/3 the stream is freed once both sides finish, and uWS reports that through onAborted even after a complete response. When the response sink ended the response from the writable callback, that report arrived before the promise reaction of the body stream ran. on_abort only dropped `resp`, and the reaction then found no response and returned, so nothing released the base ref of the request. The RequestContext leaked, server.pendingRequests never returned to 0, and a graceful server.stop() never resolved. on_abort now ends the request in that case, through end_already_responded_stream().
…he never-settling pump The MUX arm added in the previous commit can reject a parked request-body read and drain microtasks. That ran with the event loop scope count at zero, so a nested enter()/exit() inside the rejection would have run a checkpoint in the middle of the frame. Take the scope before both arms, as the rest of the function already assumed. Add the case the arm exists for: a direct stream whose pull() never settles after it ended the response from a later microtask. Its promise reaction never runs, so the stream close is the only notification left. An end inside the first pull() finishes the response before the stream is attached, which takes a different path, so the test ends it from a later microtask. Correct two comments that described the old behaviour.
StatusReproduced, fixed, and verified on this branch. How I reproduced it. Scale. One h2c connection, a client-chosen 4-byte initial window, 200 sequential requests: 200 of 200 leak before the fix and survive the connection close. 0 after. Not fixed here. HTTP/1.1 leaks one context per request when a body-stream pump never settles after it ended the response. It has no per-request teardown notification to hook, so it needs the sink to tell its context when it ends a response. That is a larger change and a follow-up, described in the PR body. Tests: 8 new cases, all failing on main and passing here. |
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesStream cleanup
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The multiplexed-stream cleanup change has no identified merge-blocking issue in the reviewed scope. 🚥 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.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding (which is the pre-existing HTTP/1 sibling, not a regression from this diff), I also checked the new on_abort MUX arm for re-entrancy: the event-loop scope guard is now taken before that arm so its microtask drain runs under the held count, and the case where the transport frees the stream from inside do_render_stream's drain_microtasks() lands on the existing resp.get().is_none() check and discard_stream_after_abort, with the sink's res already nulled. This is still a subtle request-lifetime change across H2/H3 teardown, so a human look at the ordering around end_already_responded_stream() releasing the base ref is worthwhile.
Extended reasoning...
The diff is a one-branch change in on_abort (/home/claude/bun/src/runtime/server/RequestContext.rs): the MUX && has_responded() arm now nulls the sink's raw res copy and calls end_already_responded_stream() (whose debug_assert!(!MUX) is removed), with the enter_event_loop_scope_without_checkpoint guard hoisted above it. The rest is comment updates in Http3Context.h and streams.rs plus new H2/H3 tests. The three candidates the hunt investigated (JS teardown running inside the transport's stream-free loop, event-loop scope coverage, and base-ref release mid-do_render_stream) were ruled out; I re-read the do_render_stream post-drain path and the hoisted guard to confirm the stated reasons hold. The only surviving finding is the pre-existing HTTP/1 never-settling-pump leak the PR description itself names as a follow-up, so nothing in the diff is wrong on its face, but the change moves a base-ref release into a transport callback that fires while other frames may still hold the context, which merits human review rather than an automated approve.
…e, and test the nested case The arm can run while do_render_stream is still on the stack: an abrupt server.stop(true) inside pull() closes the connection, and the stream teardown reports before the response finishes attaching. discard_stream_after_abort then cleans up the sink the arm left behind, and its finalize() reads `res` unless the sink is done. Drop the handle the way the sink's own abort() does, so it never holds a freed one, and say that in the comment. Add the case as a test. It fails on main with pendingRequests 1.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Keep the facts a reader needs (who reaches end_already_responded_stream on each transport, why an H2/H3 reaction must not, why the event loop scope precedes both arms) and drop the rest.
|
Updated 8:09 PM PT - Sep 15th, 2026
✅ @robobun, your commit 82b98a8aba8c50e91a3b154319711aafe8983232 passed in 🧪 To try this PR locally: bunx bun-pr 42844That installs a local version of the PR into your bun-42844 --bun |
Review round: two commitsThe nested ordering, now executed rather than argued. Both reviews asked for a human look at the case where The arm ends the request, On Comment length. HTTP/1.1 is answered in the review thread. It has the same never-settling shape, it still leaks after this change, and the fix belongs in its own diff. |
There was a problem hiding this comment.
No new blocking issues. 1 optional suggestion (a nit or a note on pre-existing code) was found and not posted. Nothing in this review needs a push before merging.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
…continue cases The request-context leak that this shape hit over HTTP/3 is fixed on main (#42844), so the case no longer has to read the request body first. Both shapes stay.
…t when pull() returns (#42957) ### Problem - Over HTTP/1.1, a direct stream whose `pull()` calls `end()` from a later microtask and then never returns keeps its request forever. `server.pendingRequests` climbs and a graceful `server.stop()` never resolves. #42844 fixed HTTP/2 and HTTP/3 only. - The cause is `readDirectStream` (`src/jsc/bindings/webcore/streams/BunStreamSource.cpp:758`). For an async `pull()`, the sink's owner got a promise that settles when `pull()` settles. A sync `pull()` got one that settles when the controller closes. - Each owner waited the same way: a `fetch()` upload and `HTMLRewriter` never finished. `bytes()` and `arrayBuffer()` wait in a different function (#42950). ### Fix - An async `pull()` now gets the same close promise. It settles at `end()`, `close()`, `close(error)`, or a peer abort. The `pull()` reactions only close a controller that `pull()` left open. - The close promise is rejected as handled. `server.stop(true)` in the microtask drain after the attach rejects it before the request subscribes. Main exits with code 1 there for a sync `pull()`. - Correct because `end()` and `close()` detach the controller first. Nothing `pull()` does later reaches the sink. - Verified: `test/js/bun/http/serve-direct-readable-stream.test.ts` (19 cases fail on main), `test/js/workerd/html-rewriter.test.js` (2). Other suites: Notes. Self-reviewed: 6 concerns raised, 6 addressed. ### Background - A direct stream (`type: "direct"`) calls `pull(controller)` once. The controller writes straight into a native sink, such as the `Bun.serve` response sink. - The sink's owner (for `Bun.serve`, the `RequestContext`) gets a promise from `assignToStream`. When it settles, the owner tears the sink down and releases itself. - `end()` and `close()` detach the controller. Then `directStreamOnClose` settles the close promise. <details><summary>Notes</summary> - Origin: a fuzz-ledger note, no user report. The shape needs a `pull()` that outlives its own `end()`. Repro, one file: ```js const server = Bun.serve({ port: 0, fetch: () => new Response(new ReadableStream({ type: "direct", async pull(c) { c.write("streamed"); await Promise.resolve(); c.end(); await new Promise(() => {}); } })) }); for (let i = 0; i < 3; i++) console.log(await (await fetch(server.url)).text(), server.pendingRequests); await server.stop(); console.log("stopped", server.pendingRequests); ``` 1.4.3 and main a8e4e90: `streamed 1`, `streamed 2`, `streamed 3`, then `stop()` never resolves. This branch: `streamed 0` three times, then `stopped 0`. - The same stream shape on 1.4.3: a `fetch()` upload hangs when `end()` comes in the first tick of `pull()` or later. `HTMLRewriter.transform()` hangs when `end()` comes after an await. `text()`, `blob()`, reader loops, `Bun.write` and `Bun.spawn({ stdin })` already settle at `end()`. - Not fixed here: `consumeDirectStreamToArrayBuffer` (`BunStreamConsumers.cpp:1114`) also returns a promise derived from an async `pull()`. So `Response.bytes()`, `Response.arrayBuffer()`, `Bun.readableStreamToBytes` and `Bun.readableStreamToArrayBuffer` still wait for `pull()` to return. #42950 fixes that function. The contract matrix names those consumers in `waitsForPullToReturn` and skips only the three `pullNeverReturns` shapes for them. The PR that lands second removes that set. - Why this layer and not a notification from the sink to its `RequestContext`, which the notes of #42844 proposed. That fixes only `Bun.serve`. The sink would call its owner from inside `end_from_js(&mut self)`, and the owner frees the sink when its last ref goes, so the release needs a deferral. The close promise already is that deferred notification, and a sync `pull()` uses it today. The `on_abort` arm from #42844 stays: it covers a response that the sink ends from a native callback (`onWritable`), where HTTP/2 and HTTP/3 free the stream before the reaction runs. - What changes for an async `pull()`. A clean `end()` or `close()`: the owner resolves at the call, not when `pull()` settles. `close(error)`: the owner rejects with the error at the call. A peer abort while `pull()` is pending: the owner's promise rejects with the abort reason then, as it already did for a sync `pull()`. Before, it resolved when `pull()` settled. `Bun.serve` has reclaimed its claim by then, and `fetch()` checks its aborted flag. A `pull()` that closes the controller in its first tick: `readDirectStream` returns `undefined`, or a rejected and handled promise for `close(error)`, exactly as for a sync `pull()`. - What does not change. `pull()` that resolves without `close()`/`end()` ends the controller and then resolves the owner. `pull()` that rejects while the controller is open closes it with the reason and rejects the owner with the raw reason. An `end()` that throws in that implicit close rejects the owner. A rejection after a clean close is dropped. - Visible consequence: a graceful `stop()` no longer waits for work that `pull()` does after `end()`. The response is complete at that point, and HTTP/2 and HTTP/3 already release the request there since #42844. - Tests that pinned the old behaviour as a precondition (from #39510). "closing the connection after the request was released" and "requestIP()/timeout() after the response completed and the client left" sampled `pendingWhileParked: 1` while `pull()` was parked after `close()`. They now pin 0. "stop(true) after the response completed still releases the parked request" opened the gate before it awaited the stop. It is now "... does not wait for the parked pull()" and samples the stop with the gate shut. - `live_resp()` and `ended_response` stay. The window they guard (the sink ended the response, the context is still alive) is now one microtask checkpoint, not "until `pull()` settles". - #36567 once changed `readDirectStream` for the aborted-request case and reverted it over the abort-then-reject path. #39743 solved that case in `on_abort`. `serve-stream-reject-flush-leak.test.ts`, the test that change broke, passes here. - The new backpressure cases use one buffered write below the high water mark plus `end()` in the same tick. That makes a single `tryEnd()` of 32 MB to a client that does not read, so `end()` parks on the drain. The request then waits for the drain only. These two cases are skipped on Windows. It takes the first write of a response whole, whatever its size (probed up to 250 MB on main), so one `tryEnd()` never parks there. - Suites run on the debug ASAN build, all green except where noted: `serve-direct-readable-stream` (169), `html-rewriter` (186), `html-rewriter-leak`, `streams.test.js` (596), `serve-http2` (93), `serve-http3` (72), `serve-http2-lifecycle`, `serve-protocols`, `serve-stream-body-error`, `serve-error-handler-stream`, `serve-stream-reject-flush-leak`, `serve-pending-promise-abort-leak`, `serve-async-stream-client-abort`, `serve-response-gc-backpressure-abort`, `serve-response-stream-sink-leak`, `serve-body-leak`, `serve-reused-response`, `async-iterator-stream`, `bun-server`, `body-stream` (9086), `body.test`, `fetch-abort-stream-body`, `body-stream-excess`, `fetch-gzip`, `fetch-http3-client`, `fetch-http3-adversarial`, `spawn-stdin-readable-stream`, `spawn.test`, `node-stream`, `web-stream-state`, `s3.test`, `bun-install-streaming-extract`. `serve.test`, `fetch.test`, `fetch.stream.test`, `bun-write.test`, `streams-leak` and `AsyncLocalStorage.test` have failures in this container. The same tests fail on a build of main, so they are not from this change. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/workerd/html-rewriter.test.js, test/js/web/streams/streams.test.js, test/js/bun/http/serve-direct-readable-stream.test.ts <!-- robobun:evidence:end -->
2 participants
Problem
RequestContext.pendingRequestsstays up and a gracefulstop()hangs.on_abort(RequestContext.rs:1468) seeshas_responded()and only dropsresp. The body stream's reaction then finds noresp, so nothing releases the base ref.Fix
on_abortnow ends the request there, throughend_already_responded_stream(): the HTTP/1 path for a response the sink ended. That branch can reject a parked request-body read, so the event loop scope now covers both arms.respalso releases the base ref. The late reaction finds the request ended, and no-ops.serve-http3.test.tsandserve-http2.test.tsfail on main, pass here. Other suites: Notes.Background
RequestContextholds a base ref whilerespis set. Each end path clearsrespand releases it.HTTPServerWritable) writes a streamed body and can end the uWS response itself, for example once flow control reopens. The context hears of that from the stream's reaction.onAborted, also after a complete response. It is the last call onresp.Notes
Direct leak of 15 byte(s)fromRequestContext::on_buffered_body_chunk(RequestContext.rs:4249).test/js/bun/http/serve-http3.test.ts: all tests pass, then LeakSanitizer reports the buffered request body (request-content, 15 bytes). The pool slot of the context is not visible to LeakSanitizer. Only the buffered body bytes are, so the report needs a request body. The leak itself does not: a plain GET leaks the context too.ReadableStream, a directReadableStream, and an async generator. A string body does not use the sink and does not leak (checked on main).Bun.serve({ tls, http3: true, fetch: () => new Response(new ReadableStream({ async pull(c) { c.enqueue(bytes); c.close(); } })) }), onefetch(..., { protocol: "http3" }), then readserver.pendingRequests. Before: 1, andawait server.stop()hangs. After: 0, andstop()resolves in a few ms.on_abortfiring whiledo_render_streamis still on the stack.server.stop(true)insidepull()after the sink ended the response drives it. The debug log showsdoRenderStream -> onAbort -> endAlreadyRespondedStream -> aborted while attaching the stream -> deinit, so the arm ends the request,discard_stream_after_abortcleans up the sink the arm left behind, and the context is freed only after the outer frame unwinds. Clean under ASAN, and a latecontroller.write()after the teardown still fails with the normal closed-sink error. It leaks on main (pendingRequests1) and is the ninth new case.wrapper.sink.res = Nonein that arm: the sink outlives the call on this path and itsfinalize()readsresunless the sink is done. I could not build a case that dereferences the freed handle, because the JS wrapper rejects a call on a closed controller first, and removing the line changed nothing under ASAN. It is kept because it matches what the sink's ownabort()does (streams.rs:1974) and keeps the guard local instead of resting on a cross-file invariant.pendingRequests200, 201Responseobjects retained afterBun.gc(true), still 200 after the socket closes, gracefulstop()hangs. On this branch: 0, 1, 0, andstop()resolves.http2andhttp3both default to false, so only a server that opts in is affected.pendingRequestsat 64 on main and at 0 with this change. It is clean under ASAN.EventLoop::drain_microtasks_with_globalof the kind already inVirtualMachine.rs, and the drain-ordering question in the same helper. The last two are helper-wide and already reachable on main over HTTP/1.onBufferedBodyChunk 0 true), so with Bun.serve: release the request body slot once the body is complete #39660 the 15 bytes are freed and LeakSanitizer goes quiet. TheRequestContextstill leaks andserver.stop()still hangs. The two changes do not overlap. The new tests assert onserver.pendingRequestsandserver.stop(), so they fail on main with or without Bun.serve: release the request body slot once the body is complete #39660.respalso releases the base ref". Four sites clearrespon main.detach_response(): each of its 12 callers ends inderef()orRequestContextRef::adopt, or isdeinitat refcount 0.end_already_responded_stream():take(), thenderef(). The WebSocket upgrade (server_body.rs:2012):set(None), thenreclaim_promise_cell()andderef(). Thehas_responded()branch ofon_abort(RequestContext.rs:1471on main):set(None), thenreturn. It was the one exception.discard_stream_after_abortalready relies on the rule.handle_resolve_stream): when the pump promise never settles it never runs. A direct stream whosepull()awaits forever after an asynchronouscontroller.end()leaks on main and is fixed here for H2/H3 (the last two new cases). The end has to come from a later microtask: an end inside the firstpull()leaves the response already finished when the stream is attached, which takes a different path and does not leak.pendingRequests1, 2, 3, gracefulstop()hangs, measured on this branch). An HTTP/1 socket is not freed per request, so there is no equivalent notification to hook. That needs the sink to tell its context when it ends a response, which is a larger change and a follow-up.qenc-hdl: not all 0 bytes of encoder stream written out; 1 bytes buffered,stream: stashed 49 bytes of header block,stream: still sending headers: no writing allowed. The next write pass sends the block and calls the writable callback, where the sink ends the response.service_streamsthen callson_closein the sameprocess_conns. A handler that answers from a timer writes directly, so it does not hit this. A second request on a warm connection does not hit it either.Http2Response::tryEndreturns false when the stream window is too small. The sink then finishes fromonWritable, reached fromepilogue -> pump -> drainWritablein the socket event that carried the WINDOW_UPDATE.sweepConnectionin the same epilogue callsonAborted. The new test drives this with the raw frame client:SETTINGS_INITIAL_WINDOW_SIZE = 4, then oneWINDOW_UPDATE.drain_quic_if_necessary(HTTP/3). So a response that the sink ends from JS is always detached by the reaction first. Only an end from a native callback loses the race.wrapper.sink.res = None: the sink keeps a raw copy ofresp. If a frame up the stack keeps the context alive, the sink survives this call, andHTTPServerWritable::start()readsreswithout adonecheck. The abort path clears it for the same reason.req.signaldoes not fire for a stream that closes after a complete response. A rejection of the body stream that arrives after the stream closed is still not reported on HTTP/2 and HTTP/3.serve-http3(69),serve-http2(91),serve-http2-lifecycle(23),serve-http2-protocol(201),serve-protocols,fetch-http3-client,fetch-http3-cold-post,fetch-http3-adversarial,serve-direct-readable-stream,serve-async-stream-client-abort,serve-pending-promise-abort-leak,serve-response-gc-backpressure-abort,serve-response-stream-sink-leak,serve-stream-reject-flush-leak,serve-stream-body-error,serve-error-handler-stream,async-iterator-stream. The new cases also pass withdetect_leaks=1and the CI LeakSanitizer options.[human-review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file