Repository navigation
Conversation
…r the response finishes A request-body read that was still pending when the response finished was force-rejected with `AbortError: The connection was closed.`, even when the body had fully arrived, the client had not aborted, `req.signal.aborted` was false, and the keep-alive connection went on to serve the next request. Any "respond 202 now, ingest the upload in the background" handler silently lost its body, with an error indistinguishable from a real client disconnect. Response completion is not request completion. uWS's `markDone()` cleared `inStream`, so no further body bytes could reach the handler, and `end_request_streaming()` then rejected the pending consumer on every response-finalize path. Add an opt-in `keepRequestBodyOnDone` to uWS that keeps the request-body and abort handlers armed past `markDone()`. Bun sets it when a consumer asks for the body (`.text()`, `.json()`, `req.body`, ...) while the body is still arriving, and parks the request context when the response finishes first: the body keeps flowing to its consumer, and the context is released once the last chunk is delivered. The body read is still rejected with the connection-closed AbortError when the peer actually goes away mid-upload, which the now-armed abort handler reports. Delivering a chunk runs user JS, which drains microtasks and can observe a socket close, so the body callback holds a ref across the delivery.
|
Updated 10:23 PM PT - Jul 6th, 2026
❌ @robobun, your commit dc6da69 has some failures in 🧪 To try this PR locally: bunx bun-pr 33524That installs a local version of the PR into your bun-33524 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Warning Review limit reached
Next review available in: 35 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 (2)
WalkthroughThis PR adds a ChangesKeep Request Body On Done
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Checked #13237 against this branch: it does not fix it, so I'm leaving the That repro async fetch(req) {
await Bun.write("test.txt", new Response(req.body)); // hangs here
return new Response("ok"); // never gets this far
}It still hangs on this branch: The body stream is alive and unconsumed the whole time the handler is blocked; the hang is on the |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…uest body Three fixes from review, all on the parking path: `clearOnWritableAndAborted()` nulls `onAborted` unconditionally, and the end/tryEnd/sendTerminatingChunk shims all call it, so it undid markDone()'s `keepRequestBodyOnDone` preservation. A parked context was left with no abort handler on the socket: a peer that disconnected mid-upload was never reported, the pending read never settled, and the context leaked. The sync-handler tests missed it because `to_async()` re-arms `onAborted` right after the response renders; an async handler renders from a later microtask and nothing re-arms. Honour `keepRequestBodyOnDone` there too, and cover sync / async / streaming-response handlers in the tests. `park_for_request_body_drain()` read `resp` through `should_close_connection()` before checking `has_body_abort_handler()`, which is the only proof the socket is still alive. `end_already_responded_stream()` documents that `resp` may already be freed, so the guards have to run in the other order. uws disarms the idle timeout when it delivers the body's last chunk, expecting the response on its way out to re-arm it. A parked response already went out, so nothing did: the keep-alive socket was left with no timeout and stuck at `isIdle = false`, so neither the idle sweep nor closeIdleConnections() could reap it. Re-arm in `finish_request_body_drain()`, and restore `isIdle` when the body is no longer kept and the response is done. Also wire the test client's error/close events to reject its pending waiters, so a server-side regression fails with the socket error rather than a timeout.
|
Pushed 198bf77 addressing all four review findings. Two of them were real bugs in the parking path, and one of those was a genuine miss in my test coverage, so summarising here:
My tests missed it because they only used a sync handler, where Idle timeout + Guard ordering in Verification
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/http/serve-request-body-after-response.test.ts`:
- Around line 4-8: The top block comment is too long for the repo’s 3-line
limit; shorten the explanation in the request-body test while keeping the same
rationale. Update the comment near serve-request-body-after-response so it is
compressed into at most three lines, and apply the same trimming to the other
long comment referenced in the review, preserving only the essential context
about the pending read and live keep-alive upload behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e04ef7a1-404f-48c5-ad89-fca28adf2951
📒 Files selected for processing (8)
packages/bun-uws/src/HttpContext.hpackages/bun-uws/src/HttpResponse.hpackages/bun-uws/src/HttpResponseData.hsrc/runtime/server/RequestContext.rssrc/uws_sys/Response.rssrc/uws_sys/libuwsockets.cpptest/js/bun/http/serve-request-body-after-response.test.tstest/js/bun/http/serve.test.ts
Finishing a parked request body restores isIdle via setKeepRequestBodyOnDone, and that can run mid-segment: when the body's last chunk and a pipelined next request arrive together, the drain sets isIdle = true and then the next request's lambda sets HTTP_RESPONSE_PENDING without clearing it. A following closeIdleConnections() then closes the socket out from under the in-flight request, whose later render corks a freed socket (heap-use-after-free in us_socket_group). The same window already existed for a sync-handled request pipelined ahead of an async one via markDone(). Clear isIdle where HTTP_RESPONSE_PENDING is set: a just-routed request is in-flight by definition, so closeIdleConnections() must not see it as idle. Verified with an ASan repro (body tail + pipelined GET in one segment, then closeIdleConnections during the async handler's window). A hermetic bun:test is omitted: the bug requires the tail and next request to land in a single socket read, and the test runner's event loop perturbs that coalescing, so the test would pass with and without the fix.
CI status: green diff, flaky unrelated lanesThe last two CI runs failed on entirely different, non-overlapping sets of tests, which is the signature of infrastructure/timing flake rather than a fault in this diff (a real regression fails the same test across runs):
None touch the
I've used my one re-roll already, so I'm not going to keep pushing
Ready for a maintainer to re-run the flaky lanes or merge. |
…r req.clone() (#42017) ### Problem - A `Bun.serve` handler that calls `req.clone()` and reads neither body leaks about 7.3 KB per request, independent of body size. Full GC keeps it. - The root is the `protect()`ed pull promise of the request body's `ByteStream`. `end_request_streaming` (`src/runtime/server/RequestContext.rs`) reached that stream only through the body `Value`, and `clone()` re-points `Locked.readable` at a tee branch. It aborted that reader-less branch (a no-op) and returned before erroring the source the context holds in `request_body_readable_stream_ref`. `finalize_without_deinit` then dropped that ref silently. ### Fix - `end_request_streaming` rejects a `Locked` body as before, then errors and releases the `ByteStream` behind `request_body_readable_stream_ref` whenever that stream has not already ended. `finalize_without_deinit` leaves the ref to that call. - Correct because the context is the stream's only producer and nothing feeds it once request streaming ends. Erroring the source settles the parked pull and errors both branches, so a pending clone read rejects with the `AbortError` an un-cloned read already gets. - Self-reviewed: 2 concerns raised, 2 addressed (a guard so the non-clone paths do not deliver a second error, and a test for the client-abort path). - Verified: three new tests in `test/js/web/fetch/body-clone.test.ts` fail on 1.4.3 and with `src/` reverted, and pass with the fix. ### Background - `req.body`, `clone()` and `textStream()` wrap an incoming body in a native `ByteStream` source that the `RequestContext` feeds from the socket. - A pull with nothing buffered parks. Its promise stays `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, and `detach_response` stops reading them. So only `end_request_streaming` can settle that pull. <details><summary>Notes</summary> - The `has_received_last_chunk` guard keeps the non-clone paths byte-identical. There, `to_error_instance` reaches the same `ByteStream` through the body `Value` and errors it first, and the context still holds its ref. `ByteStream::on_data` does tolerate a repeated `AbortReason` (the `done` arm returns early, a stored `Err` is a plain enum value), but the fix does not want to depend on that. - Repro from the report on release 1.4.3, 20k requests, `req.clone()` only: `4000:+40MB 8000:+69MB 12000:+97MB 16000:+123MB 20000:+149MB => ~7811 B/request`. `no-clone`, `clone-consume-clone` and `clone-consume-original` plateau. `heapStats()` per leaked request: +3 `ReadableStream`, +3 `ReadableStreamDefaultController`, +1 `ReadableStreamDefaultReader`, +1 `BytesInternalReadableStreamSource`, +1 `NativeStreamSourceAdapter`, +1 `ReadRequest`, +1 `StreamTeeState`, +1 `Uint8Array`, +3 `Promise`, +3 `FullPromiseReaction`; `protectedObjectTypeCounts`: +1 `Promise`, +1 `Uint8Array`. That graph is exactly what the protected pull promise reaches. - Why the body size does not matter: the response of a synchronous handler ends inside uWS's request callback, before uWS delivers the body bytes of the same packet, and `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 body `Value` still pointed at the native stream, so `to_error_instance` errored the `ByteStream` directly. Only the native clone paths (`Request.prototype.clone`, `BunRequest` clone, with or without `.body` observed first) re-point the body at a branch. The first new test covers all three. - Two tests pin the observable behaviour of a `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_deinit` is the first place that sees the held ref when `on_abort` takes the `is_dead_request()` shortcut with a `Used` (`textStream()`) body. Dropping the ref there left that read pending too; it now goes through the same erroring path thirty lines later. - The fetch client's `Response.clone()` with nothing read does not leak (checked separately, 500 iterations, zero retained streams). - Suites run on the debug ASAN build: `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:info` loopback), `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`. - Related open PRs that touch the same function but not this bug: #33524 (keep delivering a late body after the response), #39660 (release the body slot once complete). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: BUILD FAILED (no junit output) $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/web/fetch/body-clone.test.ts ninja: Entering directory `/workspace/bun/build/debug' [1/162] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 242 extern-C blocks audited [2/162] gen cpp.rs (cppbind) [2/162] cargo bun_runtime → libbun_runtime.a FAILED: rust-target/x86_64-unknown-linux-gnu/debug/libbun_runtime.a /workspace/bun/build/release/bun /workspace/bun/scripts/build/stream.ts rust --console --cwd=/workspace/bun --env=CARGO_TERM_COLOR=always --env=BUN_CODEGEN_DIR=/workspace/bun/build/debug/codegen --env=CC=/usr/lib/llvm-21/bin/clang --env=CXX=/usr/lib/llvm-21/bin/clang++ --env=AR=/usr/lib/llvm-21/bin/llvm-ar --env=CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_LINKER=/usr/lib/llvm-21/bin/clang++ --env=CARGO_HOME=/root/.cargo --env=RUSTUP_HOME=/root/.rustup --env=RUSTUP_TOOLCHAIN=nightly-2026-07-20 --env=CARGO_PROFILE_RELEASE_LTO=off --env=CARGO_PROFILE_RELEASE_CODEGEN_UNITS=16 --env=CARGO_PROFILE_RELEASE_DEBUG_ASSERTIONS=true --env=CARGO_ENCODED_RUSTFLAGS='-Crelocation-mod ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (427e0a0) test/js/web/fetch/body-clone.test.ts: (pass) Request with streaming body can be cloned [0.30ms] (pass) Response with streaming body can be cloned [0.15ms] (pass) Request with large streaming body can be cloned [5.63ms] (pass) Request with large streaming body can be cloned (pull) [4.43ms] (pass) Response with chunked streaming body can be cloned [30.88ms] (pass) Request with streaming body can be cloned multiple times [0.33ms] (pass) Request with string body can be cloned [0.11ms] (pass) Response with string body can be cloned [0.07ms] (pass) Request with ArrayBuffer body can be cloned [0.15ms] (pass) Response with ArrayBuffer body can be cloned [0.09ms] (pass) Request with Uint8Array body can be cloned [0.11ms] (pass) Response with Uint8Array body can be cloned [0.07ms] (pass) Request with mixed body types can be cloned [0.30ms] (pass) Response with mixed body types can be cloned [0.21ms] (pass) Request with non-ASCII string body can be cloned [0.10ms] (pass) Response with non-ASCII string body can be cloned [0.06ms] (pass) Request with streaming non-ASCII body can be cloned [0.12ms] (pass) Response with streaming non-ASCII bod ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/web/fetch/body-clone.test.ts bun test v1.4.3 (f42e980) test/js/web/fetch/body-clone.test.ts: (pass) Request with streaming body can be cloned [37.67ms] (pass) Response with streaming body can be cloned [12.72ms] (pass) Request with large streaming body can be cloned [25.39ms] (pass) Request with large streaming body can be cloned (pull) [32.80ms] (pass) Response with chunked streaming body can be cloned [51.79ms] (pass) Request with streaming body can be cloned multiple times [14.25ms] (pass) Request with string body can be cloned [8.76ms] (pass) Response with string body can be cloned [7.99ms] (pass) Request with ArrayBuffer body can be cloned [11.92ms] (pass) Response with ArrayBuffer body can be cloned [9.39ms] (pass) Request with Uint8Array body can be cloned [9.30ms] (pass) Response with Uint8Array body can be cloned [8.50ms] (pass) Request with mixed body types can be cloned [22.29ms] (pass) Response with mixed body types can be cloned [20.63ms] (pass) Request with non-ASCII string body can be cloned [7.49ms] (pass) Res ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 812ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/123] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 242 extern-C blocks audited [2/123] gen cpp.rs (cppbind) [2/123] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno) �[1m�[92m Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr) �[1m�[92m Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys) �[1m�[92m Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety) �[1m�[92m Compiling�[0m bun_base64 v0.0.0 (/workspace/bun/src/base64) �[1m�[92m Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys) �[1m�[92m Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys) �[1m�[92m Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd) �[1m�[92m Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp) �[1m�[92m Compiling�[0m bun_brotli v0.0.0 (/workspace/bun/src/ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/server/RequestContext.rs | 49 ++++++------ test/js/web/fetch/body-clone.test.ts | 141 +++++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+), 26 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/server/RequestContext.rs 9 5 19 test/js/web/fetch/body-clone.test.ts 6 6 19 ``` </details> <!-- robobun:evidence:end -->
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-06, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
Repro
Before:
After:
The body had fully arrived, the client never aborted,
req.signal.abortedisfalse, and the socket went on to serve the next keep-alive request. The "respond 202 immediately, ingest the upload in the background" pattern lost the body every time, with an error indistinguishable from a real client disconnect.Cause
Response completion was treated as request completion.
HttpResponseData::markDone()(run byend()/tryEnd()) clearsinStream, so no further request-body bytes can ever reach the handler once the response is out.RequestContext::end_request_streaming()then rejects a still-Lockedrequest body on every response-finalize path, unconditionally, withCommonAbortReason::ConnectionClosed.A pending read therefore only resolved when the last body chunk happened to land before the response finalized — which is why the same
req.text()resolves fine behind a streaming response that is still open when the bytes arrive.Fix
HttpResponseData::keepRequestBodyOnDone. When set,markDone()leavesinStreamandonAbortedarmed instead of clearing them. It is reset per request inHttpContext::onData, and once the parser sees the body's last chunk, so a stale flag can never keep handlers pointing at a freed context.RequestContextsets it (and armsonAborted) the moment a consumer asks for the body —.text(),.json(),req.body, … — while the body is still arriving.respand the body handler, skipsend_request_streaming(), and hands its base ref to the body callback. The last chunk releases it;detach_response()disarms everything on every other exit.AbortError: The connection was closed.— now reported by the abort handler that parking keeps armed, rather than guessed at from response completion.Delivering a chunk runs user JS, which drains microtasks and can observe a socket close (and drop the last ref), so the body callback holds a ref across the delivery. Without it the new path is an ASan use-after-poison.
HTTP/3 is unchanged: the QUIC stream carries the body and is torn down with the response, so parking is gated on
!HTTP3.Verification
test/js/bun/http/serve-request-body-after-response.test.tscovers the body arriving in the same packet as the headers, arriving after the response was sent, a streamedreq.body, and a client that disconnects mid-upload (still rejects, andserver.pendingRequestsreturns to 0 in all cases). The first three fail onmainwithAbortError: The connection was closed.serve.test.ts's "should resolve pending promise if requested ended with pending read" asserted the old AbortError; it now asserts the read resolves with the bytes. Its point — that the promise settles rather than hanging — is unchanged.Suites run against the debug+ASan build (failures identical to
main/ the 1.4.0 release binary)test/js/bun/http/serve.test.tsrequestIP, root-range port,bun:infoloopback,#6583)test/js/bun/http/(dir)main's debug buildtest/js/node/http/mainand this branchtest/js/bun/websocket/Not covered here
bun's
node:httpserver has the same defect with a different face: the identical pattern emitsendwith zerodataevents (real node delivers the bytes). That one additionally needs theIncomingMessagereadable to be allowed to flow afterres.end()—NodeHTTPResponsedecides "nobody is reading the body" synchronously insideend(), before_read()has run on itsnextTick— so it is a separate change on top of thekeepRequestBodyOnDonemechanism this PR adds. Happy to do it as a follow-up.