Repository navigation
Bun.serve: settle the request body stream when the response ends after req.clone() - #42017
Conversation
…r req.clone() A handler that called req.clone() and returned without reading either body left the request body's native ByteStream with an unsettled pull. The pull promise stays protected until the producer settles it, and it roots both tee branches, the tee reader and every controller, so each such request leaked about 7 KB that no GC could reclaim. end_request_streaming() only reached the ByteStream through the body Value. clone() re-points Locked.readable at a tee branch, so the abort went to a branch with no reader (a no-op) and the early return skipped the context's own reference to the source, which finalize then dropped without erroring it. The context now always errors the ByteStream it feeds when request streaming ends, whatever the body Value looks like. A read pending on either branch rejects with the same AbortError an un-cloned body read gets, instead of never settling.
- end_request_streaming only errors the ByteStream when it has not already received its last chunk, so the ordinary req.body paths where to_error_instance errored the same stream do not deliver a second error. - The leak test reports the fixture's stderr and exit code together when it fails instead of asserting stderr is empty. - New test: a read parked on the clone's branch also rejects when the client disconnects while the handler is still parked.
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 11 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 10:40 AM PT - Sep 8th, 2026
❌ @robobun, your commit 759d592 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 42017That installs a local version of the PR into your bun-42017 --bun |
|
Status: ready for review. All review threads are resolved. How I reproduced it, on release // bun leak.mjs
const N = 20000;
const server = Bun.serve({ port: 0, hostname: "127.0.0.1",
fetch(req) { req.clone(); return new Response("k"); } });
const rss = () => (process.memoryUsage().rss / 1048576) | 0;
Bun.gc(true); const base = rss();
for (let i = 1; i <= N; i++) {
await (await fetch(server.url, { method: "POST", body: "x".repeat(100) })).text();
if (i % 4000 === 0) { Bun.gc(true); await Bun.sleep(30); Bun.gc(true); console.log(i, "+" + (rss() - base) + "MB"); }
}
server.stop(true); process.exit(0);
Tests: three new cases in CI (build 113032 on 759d592): the new tests and every |
![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 nit, I also checked whether the ? on body.to_error_instance(...) at RequestContext.rs:2423 can now leak request_body_readable_stream_ref (since finalize_without_deinit no longer deinits it directly) — to_error_instance with a CommonAbortReason builds a DOMException without entering user JS, so that early-exit path is not reachable in practice, and the callers that discard the result continue teardown regardless.
Extended reasoning...
The one confirmed finding is a test-hygiene nit (unbounded poll loop). The additional concern worth recording for a human reviewer is the interaction between the removed .deinit() in finalize_without_deinit and the ? early-return inside end_request_streaming: if to_error_instance could fail, the strong stream ref would be neither errored nor released on that path. This was examined and ruled out because the AbortReason(ConnectionClosed) branch constructs the error without calling into user-observable JS, so the JsResult cannot be Err here; and even on the discarded-Err path, finalize_without_deinit continues past the call. The core lifecycle change — moving the release of request_body_readable_stream_ref from finalize_without_deinit into end_request_streaming and always settling the ByteStream — touches request-teardown ordering in Bun.serve, so a human look is still appropriate; deferring rather than approving.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Pushed two follow-ups: a65d3c3 bounds the handler-reached poll in the client-disconnect test and disposes the client socket with |
…nstead of ending it (#42125) ### Problem - After `req.clone()` in `Bun.serve`, or `res.clone()` on a `fetch()` response, a reader on the original body ends with `{ done: true }` when the body fails mid-stream. 100 KB of an announced 160 KB reads as complete. The clone, and an un-cloned body, reject. - Cause: `Body::Value::to_error_instance` (`src/runtime/webcore/Body.rs:1366`) errors a native `ByteStream` but cancels any other stream. After `clone()` the body holds a tee branch, and cancel closes it. ### Fix - `to_error_instance` calls `ReadableStream::error()` with the body's error instead. Every caller is a producer that reports a failed body, so none wants a clean end. - The streamed `maxRequestBodySize` arm in `RequestContext.rs` now rejects through the body first, as the buffering arm does. Otherwise the original's branch got `The connection was closed.` and the clone got `Request body exceeded maxRequestBodySize`. - Self-reviewed: 2 concerns raised, 1 addressed (the parity fix above), 1 declined (see Notes). - Verified: six new tests in `test/js/web/fetch/body-clone.test.ts`. The three original-side tests fail on main `b5ba14b6` and pass here. Neighbouring suites stay green (list in Notes). ### Background - A streamed body is `Value::Locked` and holds a `ReadableStream`. For an incoming request or a fetch response its source is a native `ByteStream`. - `clone()` tees that stream: the body keeps branch 1, the clone gets branch 2. When the body fails, the server or client errors the source. The tee forwards that to both branches, one microtask after `to_error_instance` ran on branch 1. - `cancel()` is the consumer-side end: pending reads resolve `done: true`. `error()` is the producer-side failure: pending reads reject. The fetch abort listener already uses it (#35093). <details><summary>Notes</summary> - Found while re-checking the `req.clone()` abort work from #42017. That PR settles the native source so the clone's branch rejects. The original's branch still went through the `abort()` arm here. - Callers of `to_error_instance`: `RequestContext::end_request_streaming` and the two `maxRequestBodySize` arms, `FetchTasklet` on failure, the fetch `AbortSignal` listener in `Response.rs`, `HTMLRewriter` `fail()`. - Repro on main `b5ba14b6` (release and debug+ASAN), 102400 of 163840 bytes delivered, then the peer goes away. `Bun.serve` + `req.clone()`, reader on `req.body`: `done:true` at 102400. `fetch()` + `res.clone()`, reader on `res.body`: `done:true` at 102400. Reading the clone instead, or not cloning: rejects (`AbortError: The connection was closed.` / `ECONNRESET`). With the fix all of them reject. A `fetch()` aborted through its `AbortSignal` already rejected on both sides (the abort listener errors the branch itself). - The six tests: serve client disconnect, serve chunked upload over `maxRequestBodySize`, fetch server disconnect, each for the original and the clone. The clone-side cells pass on main too and pin the symmetry. - In the streamed cap arm the byte stream is errored through the held ref only when the body did not already reach it (`has_received_last_chunk`), the same guard `end_request_streaming` uses since #42017, so the un-cloned path does not see a second error. - `HTMLRewriter`: `transform(res).clone()` with a failing input already rejected on both sides before this change, because `fail()` errors the output `ByteStream` directly and the tee forwards it. No change there. - Declined review concern: rename `ReadableStream::abort()` (which cancels) at its three remaining callers. It stays. Those callers (`FetchTasklet::start_request_stream` on an already-aborted signal, `RequestContext::on_abort` for the response body, `FileSink::handle_reject_stream`) are consumers cancelling their source, which is what cancel is for. - Erroring a tee branch from outside the tee is safe: `readableStreamDefaultControllerEnqueue`/`Close` check `CanCloseOrEnqueue` and `readableStreamDefaultControllerError` returns early on a non-readable stream, so the tee's later chunk, close, and error steps for that branch are no-ops. `ReadableStream__error` goes through `webStreamControllerError`, which is a no-op on a closed or errored stream. - On the server disconnect path the original and the clone reject with two distinct `AbortError` objects (one from the body error, one from the source error through the tee). That was already true for a pending `.text()` on the original versus a read on the clone. - Suites run on the debug+ASAN build: body-clone (85), html-rewriter (180), serve-body-leak (15), http-server-chunking (13), serve-http2-lifecycle (23), fetch-file-upload (11), fetch-abort-stream-body, body-mixin-errors, textstream-wpt, serve-pending-promise-abort-leak (27), regression/22353, `serve.test.ts -t "request body|streaming|abort|clone|maxRequestBodySize"`. - Local-only failures seen while running neighbouring suites, identical on the unfixed release build in this container: `fetch.stream.test.ts` "Content-Length response works (multiple parts)" (5 s timeouts when the eight variants run concurrently under debug+ASAN, 0.8 s alone), `express-memory-leak.test.ts` (20 s budget, the body-less variant alone takes 19.9 s here), and `fetch.test.ts` "abort should work even if the socket was closed before the redirect" (connects to `[::]`, which this container's egress proxy refuses). </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/fetch/body-clone.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Problem
Bun.servehandler that callsreq.clone()and reads neither body leaks about 7.3 KB per request, independent of body size. Full GC keeps it.protect()ed pull promise of the request body'sByteStream.end_request_streaming(src/runtime/server/RequestContext.rs) reached that stream only through the bodyValue, andclone()re-pointsLocked.readableat a tee branch. It aborted that reader-less branch (a no-op) and returned before erroring the source the context holds inrequest_body_readable_stream_ref.finalize_without_deinitthen dropped that ref silently.Fix
end_request_streamingrejects aLockedbody as before, then errors and releases theByteStreambehindrequest_body_readable_stream_refwhenever that stream has not already ended.finalize_without_deinitleaves the ref to that call.AbortErroran un-cloned read already gets.test/js/web/fetch/body-clone.test.tsfail on 1.4.3 and withsrc/reverted, and pass with the fix.Background
req.body,clone()andtextStream()wrap an incoming body in a nativeByteStreamsource that theRequestContextfeeds from the socket.protect()ed until the producer settles it, and it roots the whole tee.clone()tees that stream and each branch pulls at once. A synchronous handler's response ends before uWS delivers the body bytes, anddetach_responsestops reading them. So onlyend_request_streamingcan settle that pull.Notes
has_received_last_chunkguard keeps the non-clone paths byte-identical. There,to_error_instancereaches the sameByteStreamthrough the bodyValueand errors it first, and the context still holds its ref.ByteStream::on_datadoes tolerate a repeatedAbortReason(thedonearm returns early, a storedErris a plain enum value), but the fix does not want to depend on that.req.clone()only:4000:+40MB 8000:+69MB 12000:+97MB 16000:+123MB 20000:+149MB => ~7811 B/request.no-clone,clone-consume-cloneandclone-consume-originalplateau.heapStats()per leaked request: +3ReadableStream, +3ReadableStreamDefaultController, +1ReadableStreamDefaultReader, +1BytesInternalReadableStreamSource, +1NativeStreamSourceAdapter, +1ReadRequest, +1StreamTeeState, +1Uint8Array, +3Promise, +3FullPromiseReaction;protectedObjectTypeCounts: +1Promise, +1Uint8Array. That graph is exactly what the protected pull promise reaches.detach_response()clears the body handler. The bytes are never buffered; only the tee machinery is retained.req.body.tee()done by hand did not leak: the bodyValuestill pointed at the native stream, soto_error_instanceerrored theByteStreamdirectly. Only the native clone paths (Request.prototype.clone,BunRequestclone, with or without.bodyobserved first) re-point the body at a branch. The first new test covers all three.clone().text()started in the handler, for a body the client never sends: it rejects when the response ends first, and when the client disconnects while the handler is still parked. Before, both stayed pending forever.req.text()without a clone already rejects in both cases.finalize_without_deinitis the first place that sees the held ref whenon_aborttakes theis_dead_request()shortcut with aUsed(textStream()) body. Dropping the ref there left that read pending too; it now goes through the same erroring path thirty lines later.Response.clone()with nothing read does not leak (checked separately, 500 iterations, zero retained streams).test/js/web/fetch/body-clone.test.ts(65 pass),test/js/bun/http/serve-body-leak.test.ts(15 pass, HTTP/1 and HTTP/2),test/js/bun/http/serve.test.ts(295 pass; 2 failures that also fail on the release binary in this container: root port range,/bun:infoloopback),serve-http2-lifecycle.test.ts,serve-pending-promise-abort-leak.test.ts,bun-serve-routes.test.ts,bun-serve-body-json-async.test.ts,test/js/web/fetch/body.test.ts,body-stream.test.ts,body-mixin-errors.test.ts,wpt/textstream-wpt.test.ts,fetch-abort-stream-body.test.ts.[human-review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file