fetch: hand off buffered response bodies instead of copying them - #31676
fetch: hand off buffered response bodies instead of copying them#31676alii wants to merge 11 commits into
Conversation
Two full-body copies on the buffered fetch response path are avoidable: - FetchTasklet::callback copied the accumulated response bytes into scheduled_response_buffer on every delivery. For the final delivery into an empty scheduled buffer - the common buffered case, where one callback carries the complete body - swap the two vectors instead. Intermediate streaming chunks keep the copy so response_buffer retains its capacity for reuse, and close-delimited bodies that deliver across multiple callbacks keep appending. - The libdeflate gzip path decompressed bodies up to the shared scratch buffer's size (512KB) into that scratch and then copied the result into the response buffer. Use the gzip ISIZE trailer to reserve the exact size and decompress directly into the response buffer for all sizes (the >512KB branch already worked this way). The reservation stays capped at 32MB; a trailer too small for the actual data makes decompress_to_vec fail with InsufficientSpace and fall through to the existing streaming slow path, and the multi-chunk paths still disable this fast path before any accumulation happens. Differential-tested against the previous behavior: honest gzip (small/large), multi-member, corrupted ISIZE/CRC, truncated streams, empty gzip, uncompressed, close-delimited multi-callback bodies, and streamed chunked consumption all produce byte-identical results or the same error codes.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughTightens libdeflate gzip fast-path eligibility and acceptance, optimizes fetch callback buffering to avoid copies on final responses, and adds deterministic tests covering gzip corruption, truncation, and ISIZE trailer edge cases. ChangesGzip decompression and fetch response handling
Suggested reviewers:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-gzip.test.ts`:
- Around line 328-330: The test currently uses expect(async () => { await (await
fetch(server.url)).arrayBuffer(); }).toThrow(), which doesn't catch Promise
rejections; replace it with an async-rejection assertion such as awaiting the
rejected promise: await expect(fetch(server.url).then(r =>
r.arrayBuffer())).rejects.toThrow(); to ensure the arrayBuffer() rejection is
actually asserted (referencing fetch, arrayBuffer, and server.url in the test).
🪄 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: 7019b29c-cc2a-48eb-b60b-49a9ed8c60fa
📒 Files selected for processing (3)
src/http/InternalState.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-gzip.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun adopt |
|
✅ Adopted; current state:
CI on build 61640: all 13 red jobs are build steps that expired waiting for agents (whole-fleet backlog during the window; nothing ran and failed). A Buildkite rebuild once agents recover is all that's needed. Awaiting maintainer judgment on the complexity-vs-win tradeoff Jarred raised. |
…lback
The error assertions now await the full fetch+read chain with
.rejects.toThrow("ZlibError") — the rejection surfaces from fetch()
itself for fully-buffered bodies, which the arrayBuffer()-only form
would leak.
Rename isize-corrupt to isize-oversized (its ~4.28 GB trailer is
rejected by the 32 MB cap and never enters the exact-size reservation)
and add isize-undersized, whose in-cap but too-small trailer makes
decompress_to_vec fail with InsufficientSpace and fall through to the
streaming slow path.
Bun.listen({ hostname: "localhost" }) can bind only ::1 while fetch()
resolves localhost to 127.0.0.1, making the test fail with
ConnectionRefused depending on the environment's resolver ordering.
Pin both sides to the same loopback address.
21a84d7 to
8a8dcd7
Compare
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
I think the memory lifetime of all the fetch stuff is very confusing right now and I am worried about increasing the complexity.
The reason for the cloning here, originally, was worries about memory fragmentation and cross-thread memory reclamation being worse. That did get better in mimalloc v3. But I would be curious about what RSS looks like when run for ~5 minutes in a release build with a request & response throughput heavy benchmark for both small responses and larger responses both with gzip and without
There was a problem hiding this comment.
No issues found and prior feedback is addressed, but this swaps buffer ownership in the fetch() hot path and widens how attacker-controlled gzip ISIZE trailers gate direct decompression — worth a human sign-off given every buffered response flows through here.
Extended reasoning...
Overview
Two production changes plus tests:
FetchTasklet::callback(src/runtime/webcore/fetch/FetchTasklet.rs): when the final HTTP-thread delivery lands in an emptyscheduled_response_buffer,mem::swapthe twoVec<u8>s instead of copying. The post-swapresponse_buffer.reset()becomes a no-op on the swapped-in empty Vec; both sides remain under the existing tasklet mutex.InternalState::decompress_bytes(src/http/InternalState.rs): the libdeflate direct-decompress branch now triggers for any ISIZE in(0, 32MB)rather than only(shared_buffer.len(), 32MB). Failure (InsufficientSpace/BadData) still falls through to the streaming slow path via the existingbreak 'libdeflate.- New edge-case tests in fetch-gzip.test.ts covering honest/empty/corrupted-trailer/truncated streams, plus a pre-existing localhost→127.0.0.1 flake fix.
Security risks
The ISIZE trailer is attacker-controlled. The 32MB reservation cap is unchanged and the new > 0 lower bound prevents zero-size reservations. A lying ISIZE either exceeds the cap (skips the branch) or under-reserves (libdeflate fails, falls through to zlib). This is the same safety envelope as the pre-existing >512KB branch, just applied more broadly — but it does mean the direct-into-caller-buffer path now runs for essentially every buffered gzip response rather than only large ones, so any latent issue in that path gets much wider exposure.
Level of scrutiny
High. FetchTasklet::callback runs for every fetch() response, and the swap changes which allocation each MutableString owns across the HTTP-thread → JS-thread handoff. The reasoning looks sound (final delivery, empty target, under mutex, InternalState.body_out_str still points at the same MutableString struct whose inner Vec was swapped), and the PR was differential-tested for byte-identical output — but this is exactly the kind of ownership change where a maintainer familiar with the tasklet lifecycle should confirm no path reads response_buffer after the swap expecting the body bytes.
Other factors
Both prior review threads (CodeRabbit's async-rejection assertion and my ISIZE-byte-offset coverage note) were addressed in 4adc39b and are resolved. CI build #60009 reports failures that robobun attributes to unrelated lanes (docker-build-on-macOS, sql, terminal, autobahn). A maintainer (alii) is already engaged via @robobun adopt. No CODEOWNERS entries cover these files.
|
Re the possible-duplicate flag for #31126: they're complementary, not duplicates. #31126 removes the per-chunk copy on the streaming path ( |
|
Ran the RSS soak you asked for. Setup: release builds of this PR vs its merge-base (only the two
RSS: no divergence attributable to the PR. Start/end/peak all land inside the baseline's own run-to-run envelope for every scenario, slopes oscillate around zero for both builds (sawtooth reclamation, not monotonic growth), and the longest run — 10 minutes of sustained 2.9 GB/s plain 1MB on the PR — ends below where it started (64 MB) with a 117 MB high-water mark, inside the baseline's 115–131 range. The one scary-looking cell (plain-1MB r1 PR: +5.6 MB/min, HWM 154) did not reproduce in the rerun or the 10-minute soak; the baseline produced a similar outlier peak (131) in its own rerun, so I read both as container noise. Throughput: gzip is where the PR moves — +36% (2KB), +19% (256KB), +35% (1MB) in these runs, consistent with the direct-decompress path removing the scratch-buffer copy. Plain is equivalent within noise (r1 −6%/−16%, r2 +5%/+5% — the swap only saves one memcpy there). Caveats worth stating: the gzip payloads are synthetic/highly compressible, so those scenarios are decompression-heavy but light on wire I/O; and this container's variance means the plain-throughput deltas shouldn't be read as anything but "no change". Raw per-run JSON (all 15 runs): happy to paste if useful. On the cross-thread reclamation worry specifically: the handed-off buffer (allocated on the HTTP thread, freed on the JS thread after consumption) is the same cross-thread free the copied |
There was a problem hiding this comment.
Thanks for addressing the ISIZE test-coverage feedback — the split into isize-oversized/isize-undersized now exercises each reservation-branch exit precisely. I didn't find any correctness issues in this revision, but deferring to a human since this touches cross-thread buffer ownership in the core fetch path and widens the attacker-influenced ISIZE allocation branch, and the sustained-RSS benchmark results are still pending.
Extended reasoning...
Overview
This PR removes one to two full-body copies from buffered fetch() responses via two changes: (1) in FetchTasklet::callback, swap response_buffer ↔ scheduled_response_buffer on the final delivery into an empty scheduled buffer instead of copying; (2) in InternalState::decompress_bytes, widen the libdeflate direct-decompress branch from estimated_size > shared_buffer.len() to estimated_size > 0, so all sub-32MB gzip bodies decompress straight into the caller's buffer rather than via the shared scratch buffer + copy. New tests in fetch-gzip.test.ts pin honest/empty/corrupted-trailer/truncated behavior.
Security risks
The ISIZE trailer is attacker-controlled, and this change makes the direct-allocation branch fire for any nonzero ISIZE under 32 MB (previously only when ISIZE exceeded the ~512KB shared buffer). The 32 MB cap is unchanged and the fallback on InsufficientSpace/BadData is preserved, so I don't see a new DoS or memory-safety vector — but it does change how often and how much we allocate based on untrusted input, which deserves human eyes. The buffer swap is under the existing tasklet mutex and the post-swap response_buffer.reset() is harmless on the now-empty vector.
Level of scrutiny
High. Both touched paths are in the hot, every-request fetch pipeline: one alters cross-thread buffer ownership semantics (HTTP thread → JS thread handoff), the other alters allocation sizing driven by untrusted gzip trailers. The reasoning in the PR description is careful and the differential testing is thorough, but this is exactly the kind of change where a maintainer familiar with the response_buffer reuse lifecycle and the libdeflate/zlib fallback contract should sign off.
Other factors
My earlier inline comment (the original isize-corrupt case skipped the reservation branch entirely) was fully addressed — the test now has separate oversized/undersized cases with an accurate comment describing each exit path. CodeRabbit's async-assertion nit was also resolved. Two things keep this from auto-approval: robobun is still running the sustained-RSS benchmark that was requested, and CI flags an unexplained ~1 MB binary-size increase on every aarch64 target for what is a very small source diff — both worth a human glance before merge.
|
Two quick notes on the open review points: the sustained-RSS benchmark is done — results in #31676 (comment) (no RSS divergence over 5–10 min soaks; gzip +19–36% throughput, plain unchanged). And the "~1 MB aarch64 binary-size increase" was measured on the pre-merge build (#60010, since canceled), whose canary baseline had drifted — main picked up a WebKit upgrade after this branch forked, which a ~30-line Rust diff can't account for. The current build on the merged-with-main head re-measures against an aligned baseline. |
…-swap # Conflicts: # test/js/web/fetch/fetch-gzip.test.ts
…ling data The reservation fast path reads the buffer's last 4 bytes as the gzip ISIZE trailer. With trailing data after the stream those bytes are attacker-chosen: a tiny body could reserve up to the 32 MB cap, and the oversized Vec is later adopted as-is into JS objects whose GC accounting sees only len - the capacity is invisible to the collector and accumulates across requests. Treat a decode that does not consume the whole buffer as a miss for the fast path (the streaming slow path already handles trailing data the same way released versions do), and shrink grossly oversized reservations - capacity more than twice len with at least 64 KB of excess - before the buffer leaves the HTTP layer. Tests: trailing junk spelling a huge ISIZE must decode the real body with bounded RSS across 64 requests; gzip with trailing data decodes without corruption, matching released behavior.
The shrink added in 01dbffb sat inside the read == buffer.len() branch, where libdeflate has just verified the trailer equals the output length, so capacity never grossly exceeds len there. The lying-trailer case it was written for takes the other exit: Success with trailing data skips the shrink, the slow path decodes into the same Vec without reallocating, and the swap hands up to 32 MB of capacity to JS behind a tiny body. The reservation's pages are never touched, so RSS cannot see it; VmSize grows ~34 MB per held response. Clear the partial output and drop oversized capacity on that exit instead, and keep the success-path shrink for capacity carried over from a reused connection. The subprocess test now measures the VmSize delta on Linux (fails at 2198 MB without this fix, 517 MB with it) and drains stderr so a child failure surfaces the real error instead of an empty-JSON parse error.
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-gzip.test.ts`:
- Around line 363-366: The test binds the server with raw.listen(0) which can
pick an IPv6 unspecified address; update the listen call to bind explicitly to
127.0.0.1 (e.g., raw.listen(0, "127.0.0.1", ...)) so the subsequent fetch to
http://127.0.0.1:${port}/ connects reliably; keep the rest of the logic that
reads port via raw.address() and the fetch invocation unchanged.
🪄 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: 86be8c6b-7d0c-4489-b500-c2d8e37caff5
📒 Files selected for processing (2)
src/http/InternalState.rstest/js/web/fetch/fetch-gzip.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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-gzip.test.ts`:
- Around line 363-366: The test binds the server with raw.listen(0) which can
pick an IPv6 unspecified address; update the listen call to bind explicitly to
127.0.0.1 (e.g., raw.listen(0, "127.0.0.1", ...)) so the subsequent fetch to
http://127.0.0.1:${port}/ connects reliably; keep the rest of the logic that
reads port via raw.address() and the fetch invocation unchanged.
🪄 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: 86be8c6b-7d0c-4489-b500-c2d8e37caff5
📒 Files selected for processing (2)
src/http/InternalState.rstest/js/web/fetch/fetch-gzip.test.ts
🛑 Comments failed to post (1)
test/js/web/fetch/fetch-gzip.test.ts (1)
363-366:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBind the raw server to
127.0.0.1explicitly.This test fetches
127.0.0.1, butraw.listen(0)can bind the unspecified IPv6 address on some hosts. That makes the case intermittently fail withECONNREFUSEDfor the same reason called out on Line 201.Proposed fix
- await new Promise<void>(resolve => raw.listen(0, () => resolve())); + await new Promise<void>(resolve => raw.listen(0, "127.0.0.1", () => resolve()));🤖 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 `@test/js/web/fetch/fetch-gzip.test.ts` around lines 363 - 366, The test binds the server with raw.listen(0) which can pick an IPv6 unspecified address; update the listen call to bind explicitly to 127.0.0.1 (e.g., raw.listen(0, "127.0.0.1", ...)) so the subsequent fetch to http://127.0.0.1:${port}/ connects reliably; keep the rest of the logic that reads port via raw.address() and the fetch invocation unchanged.
A bare listen(0) can bind only the IPv6 unspecified address on some hosts while the test fetches 127.0.0.1, the same mismatch already pinned for the TCP-server test in this file.
|
@robobun fix review comments |
|
@alii every review comment visible via the API is addressed at head (9c791c8), and all 7 inline threads are resolved:
If you're seeing comments beyond these, they may be sitting in a pending review that hasn't been submitted, since nothing else shows up on the reviews, review-comments, or threads endpoints. Happy to pick them up once they're visible. |
3 participants
What does this PR do?
Removes one to two full-body copies from every buffered
fetch()response.1. Buffer handoff in
FetchTasklet::callback. Each HTTP-thread delivery copied the accumulated response bytes intoscheduled_response_buffer. For the final delivery into an empty scheduled buffer — the common buffered case, where one callback carries the complete body — the two vectors are now swapped instead. Intermediate streaming chunks keep the copy (soresponse_bufferretains its capacity for reuse across chunks), and close-delimited bodies that deliver across multiple callbacks keep appending. Both sides of the handoff stay under the existing tasklet mutex.2. Direct gzip decompression for all sizes. The libdeflate path decompressed bodies up to 512KB (decompressed) into the HTTP thread's shared scratch buffer and then copied the result into the response buffer. The
>512KBbranch already used the gzip ISIZE trailer to reserve the exact size and decompress directly into the response buffer; this extends that to all sizes. Safety unchanged:decompress_to_vecfail withInsufficientSpaceand fall through to the existing streaming slow path (and a corrupted ISIZE is an integrity error both libdeflate and zlib reject — unchanged);is_libdeflate_fast_path_disabledbefore any accumulation, so this branch only ever sees whole bodies with an empty output buffer.Measured (release builds of the same commit, local server, fixed-work loop, macOS arm64; medians of 2 runs)
fetch().arrayBuffer())The 256KB-class gzip response is the case that moves most — exactly the newly-covered range (was scratch + copy, now direct).
Correctness
Differential-tested against the unpatched build — byte-identical results or identical error codes for: honest gzip (small/large), multi-member streams, corrupted ISIZE, corrupted CRC, truncated streams, empty gzip, uncompressed bodies, close-delimited multi-callback bodies, and streamed chunked consumption.
New behavior pins in
test/js/web/fetch/fetch-gzip.test.ts(pass on released Bun and this PR): exact decode for honest/empty streams,ZlibErrorrejection for corrupted-ISIZE/CRC/truncated streams — the trailer cases also exercise the exact-size-reservation fallback.Local suites on the patched debug build:
fetch.test.ts,fetch-gzip.test.ts,fetch.stream.test.ts,body.test.ts,body-stream.test.ts— 9,909 pass; the 3 failures are environmental (two external-network TLS tests and one whose C helper server cannot start in this environment; all three fail identically on released Bun here).