fetch: deliver response body bytes as a borrowed slice; reserve Content-Length for buffered consumers - #36570
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces caller-owned HTTP response buffers with internal decoded storage and callback body ownership. It updates HTTP consumers and call sites. Fetch buffering adds capped preallocation and cleanup rules. Tests measure RSS for a 129 MiB ChangesHTTP response ownership and buffering
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Overlaps with the swap in #31676. That PR also right-sizes the gzip libdeflate fast path in |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/webcore/fetch/FetchTasklet.rs:2446-2453— Thereserve_exactruns for every non-Ignore receive mode, including streaming (res.body/getReader()). Afteron_start_streaming_http_response_body_callbacktakesscheduled_response_buffer(leaving cap=0), the next HTTP-thread callback swaps and reserves ~Content-Length again, and theon_body_receivedscopeguard'sreset()isVec::clear()so that capacity is held for the rest of the download. Streaming a 200 MiB body now keeps a ~200 MiB scratch allocation resident for the whole stream (plus the over-capacityDrainResult::OwnedVec) — gate the reservation on the buffered path (e.g. skip oncehttp_.enable_response_body_streaming()has fired).Extended reasoning...
What the bug is
The new pre-reservation in
FetchTasklet::callbackfires on theelsebranch at line 2428, which covers every non-IgnoreBodyReceiveMode—AutoPause,Paused, andBufferAllalike. The streaming consumer path (res.body/getReader()) runs underAutoPause(on_start_streaming_http_response_body_callbacktransitionsPaused → AutoPauseat line 1597), so it hits the reserve too. That means a caller who chose streaming precisely to avoid holding the whole body in memory now gets a Content-Length-sized scratch allocation held for the entire download.Code path
Step-by-step for a 200 MiB
Content-Lengthbody consumed viares.body.getReader():- First HTTP-thread callback (headers + first packets arrive, before the user has chosen buffered vs streaming):
scheduled.list.capacity() == 0→ swap the incoming packet Vec in →body_sizeisContentLength(200 MiB), which is> scheduled.capacity()→try_reserve_exactgrowsscheduled_response_bufferto ~200 MiB. - User touches
res.body→on_start_streaming_http_response_body_callback(FetchTasklet.rs:1615-1622):scheduled_response_bufferis non-empty, socore::mem::takehands the 200 MiB-capacity Vec (containing only a few packets oflen) toDrainResult::Owned, and leavesscheduled_response_bufferat capacity 0. - Next HTTP-thread callback:
scheduled.list.capacity() == 0again → swap the ~64 KB packet in →200 MiB > ~64 KB→try_reserve_exactreserves ~200 MiB again. - JS-thread
on_body_received(line 719,has_morebranch): borrowsscheduled_response_buffer.list.as_slice()forbytes.on_data(), then the scopeguard at lines 655-660 calls(*scheduled_buf).reset().MutableString::reset()(src/bun_core/string/MutableString.rs:252) isself.list.clear(), which retains capacity. Soscheduled_response_bufferstays at ~200 MiB capacity for every subsequent packet until the stream ends.
body_sizestaysContentLength(n)on every callback (line 2404 re-reads it fromresult.body_size, whichhttp/lib.rs to_result()recomputes from the parsed header each time), so step 3's condition keeps being true whenever capacity is 0.Why nothing prevents it
- The reserve is not gated on
BodyReceiveMode::BufferAllor onhttp_.response_body_streaming— onlyIgnoreis special-cased (line 2413). on_start_buffering_callback(the.arrayBuffer()path) does not takescheduled_response_buffer, so the double-reservation in step 3 is specific to the streaming drain callback, whichcore::mem::takes it.- The
on_body_receivedreset usesclear(), not= MutableString::default(), so the second reservation persists.
Impact
Before this PR, streaming a 200 MiB Content-Length body kept
scheduled_response_bufferat roughly the max coalesced packet burst (~64 KB, bounded byAutoPause). After this PR, it holds a ~200 MiB (capped at 256 MiB) allocation for the entire download — plus the over-capacityDrainResult::OwnedVec parked inreader.context.bufferfrom step 2. That is a straight regression of the memory profile for the API users reach for on large bodies specifically to avoid this. The PR title says "peak at ~1x payload" for the buffered path; the streaming path went from ~0x to ~1x.Fix
Gate the
try_reserve_exacton the buffered path — e.g. only reserve when the receive mode isBufferAll, or skip it oncehttp_.enable_response_body_streaming()has been called /self.is_waiting_bodyand streaming has attached. The swap-instead-of-copy is fine for both modes; only the Content-Length reservation needs the gate. - First HTTP-thread callback (headers + first packets arrive, before the user has chosen buffered vs streaming):
|
Good catch on the streaming regression. Gated the |
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/web/fetch/fetch-leak.test.ts`:
- Around line 967-1028: Expand the fetch memory regression coverage around the
existing large-body test to exercise the sibling consumers bytes() and text()
with method-appropriate RSS assertions, and add a controlled case where
Content-Length exceeds the 256 MiB cap. Replace the current explanatory comments
with the relevant issue URL, or remove them if no issue URL exists, while
preserving the existing arrayBuffer() behavior and test setup.
🪄 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: d295f165-9911-4933-b80c-36b7c894e2e9
📒 Files selected for processing (3)
src/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-buffer-peak-fixture.tstest/js/web/fetch/fetch-leak.test.ts
There was a problem hiding this comment.
Beyond the inline finding, I also checked: the core::mem::swap is safe w.r.t. the HTTP client's body_out_str — it's a *mut MutableString pointing at the response_buffer field, so swapping the field contents leaves the pointer valid and the client writes into the (now-empty) swapped-in Vec on the next packet. The !success branch still clears response_buffer on every path (reset if has_more, drop-capacity otherwise). Deferring to a human on the overlap with #31676 and the un-branched 1.5x RSS threshold in the new test.
Extended reasoning...
The inline nit (BufferAll gate not excluding the unobserved-ReadableStream setter at drop_backpressure_if_unobserved) is real — MutableString::reset() is Vec::clear(), so the reserved capacity would sit idle at len≈0 for the whole download on that path. It's bounded (256 MiB cap) and the triggering pattern is unusual, so it's correctly filed as a nit rather than a blocker. Separately confirmed the swap doesn't invalidate body_out_str (field-address aliasing, not heap-pointer) and that the !success reset is preserved. This is a hot-path change with an overlapping open PR and an RSS-threshold test that isn't branched on isASAN/isDebug, so a maintainer should look.
…ffer()/bytes() peak at ~1x payload FetchTasklet::callback fires per socket read (signals.body_receive_mode is always set for fetch), so scheduled_response_buffer accumulates the body via extend_from_slice and grows by amortized doubling. The resulting over-capacity Vec is what the ArrayBuffer/Uint8Array adopts, and the intermediate reallocations push peak RSS to ~2-3x the payload. At the handoff, swap the first chunk into scheduled instead of copying, then reserve_exact the known Content-Length once (capped at 256 MiB) so the buffer grows to the final size in one step. On completion drop response_buffer's capacity instead of clearing. 129 MiB body, RSS after await res.arrayBuffer(): release linux: 377 -> 139 MiB over baseline debug+ASAN linux: 420 -> 139 MiB over baseline
Streaming consumers drain scheduled_response_buffer per chunk; reserving the full body there would hold a body-sized scratch allocation for the whole download.
…mode drop_backpressure_if_unobserved also sets BufferAll while still delivering per chunk to a ByteStream; reserving there would hold an idle body-sized allocation in scheduled_response_buffer. Track the actual buffered-consumer path with an AtomicBool written by on_start_buffering_callback.
4f61bf5 to
a8b1e0a
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/runtime/webcore/fetch/FetchTasklet.rs:2450-2458— Theis_buffering_bodygate still doesn't exclude every path where a ByteStream is attached:ValueBufferer::buffer_locked_body_value(Body.rs:2582-2585, reached vianew HTMLRewriter().transform(await fetch(url))) callson_start_buffering_callback(settingis_buffering_body = true) and then immediately materializes a ByteStream viato_readable_stream()→on_readable_stream_available. From thereon_body_receiveddelivers each chunk to the ByteStream andreset()s (i.e.clear()s, capacity retained)scheduled_response_buffer, so this reserve grows it tomin(Content-Length, 256 MiB)and it sits idle at len≈0 for the whole download — the same regression thedrop_backpressure_if_unobservedcomment flagged, now reached through the flag its fix introduced. The predicate the reserve actually needs is "no ByteStream attached" (e.g. also requireself.readable_stream_refunset, or haveon_readable_stream_available/on_start_streamingclearis_buffering_body).Extended reasoning...
What the bug is
Commit 4f61bf5 replaced the
BufferAllgate with a newis_buffering_body: AtomicBoolon the assumption that onlyon_start_buffering_callbacksets it, and that firing implies the body will accumulate inscheduled_response_buffer. That does hold for the primary caller (Body.rs:413-417, the.arrayBuffer()/.text()/.bytes()path) and forBun.write(file, response)(Blob.rs, which setson_receive_valueand never materializes a stream). Buton_start_bufferinghas a third caller:ValueBufferer::buffer_locked_body_valueat Body.rs:2582-2585, and that one does materialize a ByteStream immediately after setting the flag.Step-by-step trace
For
new HTMLRewriter().on(...).transform(await fetch(url))on a freshResponsewithContent-Length: 129 MiB:- html_rewriter.rs:724-751 →
get_body_readable_stream()returnsNone(Body.rs:1671-1687 only reads the JS cache andlocked.readable, both unset for a fresh fetchResponse) →ValueBufferer::run(value, None)→buffer_locked_body_value(value, None). - Body.rs:2482-2497:
locked.readableisNoneandowned_readable_streamisNone→readable_stream = None, fall through. - Body.rs:2579:
locked.task.is_some()(the FetchTasklet ptr, set at FetchTasklet.rs:1731) → true. - Body.rs:2582-2585:
on_start_buffering(task)=FetchTasklet::on_start_buffering_callback→is_buffering_body.store(true)andset_receive_mode_terminal(BufferAll). - Body.rs:2588-2590:
value.to_readable_stream()→locked_to_native_stream→ callson_start_streaming_http_response_body_callback(FetchTasklet.rs:1620mem::takesscheduled_response_buffer, leaving it at capacity 0) and thenon_readable_stream_available(Body.rs:917-918 → FetchTasklet.rs:1581 setsthis.readable_stream_ref). - Body.rs:2596: recursive call now finds
locked.readableset →Source::Bytesbranch (2519) → setsbyte_stream.pipe. - Next HTTP-thread
callback()at line 2438:scheduled.list.capacity() == 0→ swap; thenis_buffering_body == trueandContentLength(129 MiB)and129 MiB > cap→try_reserve_exactgrowsscheduled_response_bufferto ~129 MiB (bounded bySCHEDULED_PRERESERVE_MAX = 256 MiB). - Every subsequent JS-thread
on_body_receivedat FetchTasklet.rs:717-736 seesreadable_stream_refset → delivers the chunk to the ByteStream viatemporary_chunk, returns withoutbuffer_reset.set(false), so the scopeguard at line 663 runs(*scheduled_buf).reset()=Vec::clear()(MutableString.rs:252-254) — len → 0, capacity retained.
Why the gate doesn't prevent it
is_buffering_bodywas introduced to mean "on_start_buffering_callbackfired", but that callback does not uniquely imply "body accumulates inscheduled_response_buffer": one of its three callers immediately materializes a ByteStream and routes chunks there instead. Nothing clearsis_buffering_bodywhenreadable_stream_refis later set. The predicate the reserve actually needs — as the earlier review comment already suggested — is "no ByteStream attached".Impact
On main,
scheduled_response_buffercapacity in this path stays at ~one socket read (a few tens of KB); with this PR it grows tomin(Content-Length, 256 MiB)and sits idle at len≈0 for the entire download while the ValueBufferer'sstream_bufferindependently accumulates the same bytes. ForHTMLRewriter().transform(await fetch(url))on a large body that is a per-fetch memory regression of up to 256 MiB versus main — exactly the failure mode the earlierdrop_backpressure_if_unobservedreview identified, reached through a different setter of the very flag that comment's fix introduced. Since the PR's whole purpose is reducing peak memory, regressing it on a real user-facing path should block.Fix
Either tighten the predicate to also require no ByteStream is attached:
if task_ref.is_buffering_body.load(Ordering::Acquire) && task_ref.readable_stream_ref.get().is_none() {
or clear the flag when a stream is attached (in
on_readable_stream_availableoron_start_streaming_http_response_body_callback):this.is_buffering_body.store(false, Ordering::Release);
The second option also handles the case where the ByteStream is attached mid-download after buffering has begun.
- html_rewriter.rs:724-751 →
ValueBufferer (HTMLRewriter.transform) calls on_start_buffering then immediately materializes a ByteStream; without this the reserve would hold an idle body-sized capacity on scheduled_response_buffer while the stream drains it per chunk.
|
@robobun two things before review:
After the rebase, please re-run and post the two measurements this PR is judged on, since the slice-passing rework changed the receive path after the last numbers were taken (those were on
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/fetch/FetchTasklet.rs (1)
2600-2623: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAvoid reserving before adopting an owned terminal chunk.
When
body_ownedis the first non-empty chunk,scheduled.listis empty. Lines 2608-2614 first allocate the capped Content-Length capacity. Line 2622 then replaces that vector withbody_owned. Both allocations are live during the assignment, which can restore a roughly 2× payload peak for a single terminal callback.Skip the reservation when the direct adoption path applies.
Proposed fix
let scheduled = &mut task_ref.scheduled_response_buffer; + let can_adopt_owned_body = + body.is_empty() && !body_owned.is_empty() && scheduled.list.is_empty(); - if task_ref.is_buffering_body.load(Ordering::Acquire) { + if !can_adopt_owned_body + && task_ref.is_buffering_body.load(Ordering::Acquire) + {Add a regression case for a response that completes in one callback with a large
Content-Length.🤖 Prompt for 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. In `@src/runtime/webcore/fetch/FetchTasklet.rs` around lines 2600 - 2623, Update the reservation guard in the success path around scheduled_response_buffer so Content-Length pre-reservation is skipped when body_owned is non-empty and scheduled.list is empty, allowing the terminal chunk to be adopted directly without a second allocation. Preserve reservation behavior for other buffering cases, and add a regression test covering a large Content-Length response completed in one callback.
🤖 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.
Outside diff comments:
In `@src/runtime/webcore/fetch/FetchTasklet.rs`:
- Around line 2600-2623: Update the reservation guard in the success path around
scheduled_response_buffer so Content-Length pre-reservation is skipped when
body_owned is non-empty and scheduled.list is empty, allowing the terminal chunk
to be adopted directly without a second allocation. Preserve reservation
behavior for other buffering cases, and add a regression test covering a large
Content-Length response completed in one callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92709fdd-d2a1-43d3-a3ab-7aff01bb2e2a
📒 Files selected for processing (7)
src/http/AsyncHTTP.rssrc/http/lib.rssrc/install/NetworkTask.rssrc/runtime/cli/run_command.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/simple_request.rs
… on single-callback terminal
|
Updated 3:07 PM PT - Jul 31st, 2026
✅ @robobun, your commit 637d54b6b61ffa50cdd672ab01b9e1c8d1a59607 passed in 🧪 To try this PR locally: bunx bun-pr 36570That installs a local version of the PR into your bun-36570 --bun |
|
Clippy fixed in 7ba280e. Branch is already based on f68e504 ( Buffered peak (
|
| body | canary | this PR |
|---|---|---|
| 128 MiB | 2.23x | 1.07x |
| 256 MiB | 2.34x | 1.04x |
| 384 MiB | 2.79x | 1.02x |
All under the 1.15x target, including 384 MiB (past the 256 MiB reserve cap; the cap only bounds how far ahead of received bytes the reservation runs, so larger bodies still grow tight).
Streaming window (CONCURRENCY=50 MB=256 fetch-to-fetch)
linux-x64 container, same binaries:
| peakRssMB | elapsedMs | MB/stream | |
|---|---|---|---|
| release canary 1498d7b | 13858 | 22454 | ~277 |
| this PR (debug+ASAN) | 802 | 53292 | ~9.5 |
Window flat vs payload (802 MB at MB=64 too). Debug+ASAN is ~2.4x slower, which is the expected debug-build overhead, not a throughput regression in the hot path.
I can't reach bun-fuzz from this container (not on the farm-tailscale routes I have), so no release-build apples-to-apples yet. Once the linux-x64 release artifact lands from build 86444 I'll pull it and re-run here, or if you can kick ~/run50.sh on bun-fuzz against this head that's the cleaner number.
#36588) Shrinks the `node:http` proxy memory window (`pipeline(Readable.fromWeb((await fetch(upstream)).body), res)`) by keeping the upstream socket paused while `ByteStream` still has spilled bytes. ## Repro ``` MB=64 CONCURRENCY=50 STALL=0 <bin> bench/fetch/streaming-backpressure.mjs download-proxy ``` | | peak RSS | elapsed | |---|---|---| | main (`df49a6e1c`, after #36570) | 277 MB | 4.13 s | | this PR | **226 MB** | **3.92 s** | | node 26.3 | 204 MB | 5.80 s | At `CONCURRENCY=40 MB=32` (the shape the RSS test uses): main 241 MB, this PR 197 MB. ## Cause `ByteStream::on_data` called `signal_drained()` after filling a pending pull view even when the tail of the chunk spilled into `buffer`. That resumed the upstream socket with data still queued; the next recv landed with no reader waiting (`pipeline` was paused on `res.write() == false`) and went straight to `ByteStream.buffer`. Instrumented on main at C=50, `on_data` hit the no-pending-pull append path 1686 times; with this change, zero. `FetchTasklet.scheduled_response_buffer` growth from the same pattern was already capped by #36570 (which drops the staging allocation once it passes `DECODED_BODY_RETAIN_CAP`), so the `ByteStream.buffer` overflow is what remains. ## Fix `on_data` only calls `signal_drained()` when the whole chunk fit the pull view. When it spilled, the next `on_pull` signals once it drains `buffer`, so the producer stays paused until the reader has caught up. ## Tests `test/js/web/fetch/fetch-backpressure.test.ts` gains a `Readable.fromWeb` stalled-pipe drain test (all lanes) and a release-Linux-only RSS bound for the `download-proxy` shape (fails on main with peak ~241 MB, passes here at ~197 MB). ASAN quarantine dominates the per-conn buffer delta, so the RSS bound is skipped under debug/ASAN; the drain test covers the path there. <!-- 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/web/fetch/fetch-backpressure.test.ts <!-- robobun:evidence:end -->
Two changes to the fetch response-body path, together bringing buffered
.arrayBuffer()peak to ~1x payload and the 50-concurrent streaming window from ~78 to ~10 MB/stream.1. Buffered consumers (
.arrayBuffer()/.bytes()/.text()) peak at ~1x payloadFetchTasklet::callbackfires per socket read, soscheduled_response_bufferaccumulated the body viaextend_from_slicedoubling, leaving up to ~2x over-capacity on the Vec the ArrayBuffer adopts and spiking RSS to ~2-3x. A newis_buffering_bodyflag (set byon_start_buffering_callback, cleared under the mutex when a ByteStream attaches) gates a one-timereserve_exact(Content-Length)(capped at 256 MiB) so the buffer grows once.2. Body bytes delivered as
&[u8];response_bufferintermediate removedPreviously every caller supplied a
*mut MutableStringsink that the HTTP client wrote decoded body bytes into, then the callback copied that into its own destination andreset()both buffers (retaining their high-water capacity for the connection's life). Under 50-way contention those retained capacities dominate the per-stream window.InternalStatenow ownsdecoded_body: MutableString(was aNonNull<MutableString>back-pointer). Chunked/decompress write there; uncompressed Content-Length appends the recv-buffer slice.HTTPClientResult.bodyis&'a [u8](wasOption<&'a mut MutableString>).send_progress_update_*liftsdecoded_bodyonto the stack, hands outdecoded_body.list.as_slice(), and after the callback returns re-seats it if capacity is under 512 KiB (else drops it).AsyncHTTP::init/init_syncdrop theresponse_bufferparameter;FetchTaskletandS3HttpDownloadStreamingTaskdrop theirresponse_bufferfield entirely and copyresult.bodystraight into their destination under their own mutex.NetworkTask,S3HttpSimpleTask,RemoteImageDownload,send_sync) keep a localresponse_bufferandextend_from_slice(result.body)in their callback.FetchTasklet::on_body_receivedreleasesscheduled_response_buffercapacity above 512 KiB after each streaming drain instead ofclear().(Debug is ~3x slower as expected; release numbers to follow from CI artifacts.)
Net: 16 files, +~230/-~440.
Verification
body.test.ts(448),fetch-backpressure.test.tsh1/h2 (20),fetch-redirect.test.ts(30), andbun-install.test.ts(178; 14 network-gated fails match release) all pass.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/fetch-leak.test.ts