Skip to content

http: couple fetch() receive backpressure to JS body consumption (h1/h2/h3) - #29831

Merged
Jarred-Sumner merged 16 commits into
mainfrom
farm/0a9cea98/h2-window-update-backpressure
Jun 26, 2026
Merged

http: couple fetch() receive backpressure to JS body consumption (h1/h2/h3)#29831
Jarred-Sumner merged 16 commits into
mainfrom
farm/0a9cea98/h2-window-update-backpressure

Conversation

@robobun

@robobun robobun commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Stop the fetch() HTTP thread from buffering an entire response body when nothing on the JS side is reading it. After the first body chunk is delivered to JS, the transport pauses until a consumer pulls; buffered consumers (.arrayBuffer()/.text()/etc.) opt out so they still receive the body in one go.

Fixes #28035.

Mechanism

One AtomicU8 and one callback:

  • signals::Store.body_receive_mode: AtomicU8 carries a BodyReceiveMode (AutoPause / Paused / BufferAll / Ignore). FetchTasklet::callback (HTTP thread) CASes AutoPause -> Paused whenever it has just appended body bytes to scheduled_response_buffer with has_more; the transport reads signals.is_receive_paused() after the callback returns. on_start_buffering (.arrayBuffer()/.text()/etc.) sets the terminal BufferAll; ignore_remaining_response_body sets Ignore.
  • NewSource<ByteStream>.drain_handlerByteStream::signal_drained() fires it whenever bytes leave for a JS consumer (on_pull empties the buffer, drain(), the pipe / buffer-action arms of on_data, the pending-reader fulfilment path). The handler CASes Paused -> AutoPause and, if it won, enqueues a resume to the HTTP thread. on_start_streaming also CASes Paused -> AutoPause when it drains scheduled_response_buffer into a newly-created stream, so the resume it schedules actually un-pauses the socket.

A chunk delivered to a stream that has no reader lock, no pipe, and no buffer action flips to BufferAll so an abandoned body still completes and the tasklet's Strong on the stream source is released. poll_ref is unref'd once the fetch() promise resolves and re-ref'd when a body consumer attaches, so an unread Response does not keep the event loop alive while paused.

handle_response_body{,_chunked}_from_multiple_packets now flush to the progress callback on every read instead of waiting for is_done || ResponseBodyStreaming. The libdeflate one-shot path was only ever taken by _from_single_packet (whole body arrived with the headers), which is unchanged.

Per transport

  • HTTP/1.1: maybe_pause_receive after callback.run() calls socket.pause_stream() and clears the idle timeout; the keep-alive release path resumes before pooling so a paused socket is never handed back. Upgraded (101) connections and proxy tunnels are excluded.
  • HTTP/2: replenish_window skips the per-stream WINDOW_UPDATE while is_receive_paused(); the connection-level window stays receipt-based so siblings aren't starved. resume_receive_by_http_id re-runs replenish_window.
  • HTTP/3: on_stream_data calls want_read(false) after deliver() when paused so lsquic withholds MAX_STREAM_DATA; resume re-enables it.

The schedule_response_body_drain queue is folded into the new schedule_receive_resume queue (the resume handler does resume_receive() then drain_response_body()). Signals::is_empty() had no callers and is removed.

Not affected

bun install and S3 wire Signals without body_receive_mode and use their own callbacks, so the pause is never set for them. _from_multiple_packets flushing per-read is what they already got via response_body_streaming = true.

Tests

test/js/web/fetch/fetch-backpressure.test.ts — 40 tests:

  • stalled getReader() / pipeTo() / for await / unread Response — for each of h1, h1-chunked, h1-gzip, h1-tls, h2, h3: a subprocess reads the first chunk, sleeps until RSS settles, then drains to the full body. RSS is reported but not bounded (JIT warmup + mimalloc chunks + TLS dylib faulting exceed the 16 MiB body on several CI hosts); these prove the resume path does not deadlock.
  • server stops writing (h1/h1-chunked/h1-tls) — a 1 GiB in-process server's write() count plateaus while the reader is stalled, then reaches the full body once it drains. This is the platform-independent proof of the h1 pause.
  • buffered consumers are not throttledres.{arrayBuffer,bytes,text,blob}(), res.body.{bytes,text,blob,json}(), and Bun.readableStreamTo{ArrayBuffer,Bytes,Text,Blob,Array}(res.body) each receive the full body without intervention.
  • streaming shapesreader.cancel() then the abandoned body drains and a second keep-alive fetch completes; res.body.tee() both branches drain; two sequential keep-alive responses each drain fully.

Also fixes an unrelated is_final_chunk = true in the _from_single_packet chunked needs-more-data arm (it was finalising the decompressor mid-stream).

@coderabbitai

coderabbitai Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

This PR introduces per-reader byte-consumption tracking and backpressure control across HTTP/2, HTTP/1.1, and HTTP/3 fetch response streams. When JS code drains fetch response bodies via ReadableStream, consumed bytes are reported back to the HTTP transport layer, enabling sockets and protocol streams to pause and resume reads based on buffered body thresholds.

Changes

Fetch Response Stream Backpressure

Layer / File(s) Summary
Data Shape & Callback Wiring
src/bun.js/webcore/Body.zig, src/bun.js/webcore/ReadableStream.zig, src/bun.js/webcore/ByteStream.zig, src/http/Signals.zig
Added PendingValue.onStreamConsumed and NewSource drain_handler/drain_ctx fields; added internal ByteStream.didDrain helper and used it in multiple delivery paths; added Signals.body_consumption_tracked atomic flag.
Core Fetch Tasklet Wiring
src/bun.js/webcore/fetch/FetchTasklet.zig
Wired onStreamConsumedCallback into toBodyValue() for Body.Value.Locked; arm/disarm signal_store.body_consumption_tracked when streaming is enabled or ignored; clear Source drain callbacks in stream teardown.
HTTP Thread Queue & Posting
src/http/HTTPThread.zig
Added queued_response_body_consumed queue, ConsumeMessage type, scheduleResponseBodyConsumed(...) to coalesce/queue consumption deltas, and drainQueuedHTTPResponseBodyConsumed() invoked from drainEvents.
HTTP Core Receive Pausing
src/http.zig, src/http/InternalState.zig
Added receive_body_high_water/receive_body_low_water constants; InternalState.outstanding_body_bytes and InternalStateFlags.receive_paused; implemented maybePauseReceive and updated onData/consume logic to pause/resume socket reads based on outstanding buffered body bytes and body_consumption_tracked.
HTTP/2 Consumption Semantics
src/http/h2_client/Stream.zig, src/http/h2_client/ClientSession.zig
Renamed/clarified unacked_bytes semantics, added consumed_bytes to Stream, and added consumeResponseBodyByHttpId to advance per-stream consumption and trigger replenishWindow() under consumption-tracked semantics.
HTTP/3 QUIC Read Control
src/http/h3_client/Stream.zig, src/http/h3_client/ClientSession.zig, src/http/h3_client/ClientContext.zig, src/deps/uws/quic/Stream.zig, src/http/H3Client.zig, src/http/h3_client/callbacks.zig
H3 Stream: added outstanding_body_bytes and read_paused; callbacks increment outstanding bytes and global body_bytes_received; added consumeResponseBodyByHttpId at session/context levels to decrement and resume reads; added Stream.wantRead() wrapper binding to control lsquic read callbacks; exposed H3 metrics.
Byte-Delivery Notifications
src/bun.js/webcore/ByteStream.zig
Ensure drain notifications are emitted when bytes are delivered to JS (pipes, buffer actions, pending buffer copies, onPull, and drain() path), suppressing zero-length reports.
Testing & Metrics
test/js/web/fetch/fetch-backpressure.test.ts, src/js/internal-for-testing.ts
Added comprehensive tests exercising HTTP/2 per-stream WINDOW_UPDATE, HTTP/1.1 socket-read pausing, and HTTP/3 lsquic wantRead pausing; extended fetchH3Internals.liveCounts to include bodyBytesReceived.

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and specifically describes the main change: coupling fetch() receive backpressure to JS body consumption across HTTP/1, HTTP/2, and HTTP/3 transports, which aligns with the comprehensive changes shown in the raw summary.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The description explains the change and includes test verification, though it uses custom subsections instead of the exact template headings.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 5:35 PM PT - Jun 25th, 2026

@Jarred-Sumner, your commit a1e272f has 3 failures in Build #64736 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 29831

That installs a local version of the PR into your bun-29831 executable, so you can run:

bun-29831 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 3 issues this PR may fix:

  1. fetch().body piped through TransformStream does not propagate backpressure #28035 - fetch().body piped through TransformStream does not propagate backpressure; the PR's drain callbacks and per-stream WINDOW_UPDATE gating directly address this for HTTP/2 connections
  2. Bun.write writing a Response from a fetch leaks memory #10686 - Bun.write writing a Response from fetch leaks memory because the response body is fully buffered; gating HTTP/2 flow control on JS consumption would bound memory
  3. Memory leak when downloading file (both @google-cloud/storage and Bun's S3) #20487 - Large file downloads via fetch (GCS/S3) cause unbounded RSS growth; per-stream receive-window gating prevents the server from sending faster than JS consumes over HTTP/2

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #28035
Fixes #10686
Fixes #20487

🤖 Generated with Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/bun.js/webcore/fetch/FetchTasklet.zig (1)

922-930: ⚠️ Potential issue | 🟠 Major

Don't clear the drain callback before the final chunk is consumed.

clearStreamCancelHandler() now removes drain_handler, but onBodyReceived(...has_more = false) calls this before handing the last buffered bytes to ByteStream.onData(). That means the tail bytes can never report actual JS consumption, so the HTTP/2 stream stays under-credited at end-of-body.

🔧 Suggested fix
 fn clearStreamCancelHandler(this: *FetchTasklet) void {
     if (this.readable_stream_ref.get(this.global_this)) |readable| {
         if (readable.ptr == .Bytes) {
             const source = readable.ptr.Bytes.parent();
             source.cancel_handler = null;
             source.cancel_ctx = null;
-            source.drain_handler = null;
-            source.drain_ctx = null;
         }
     }
 }

Keep drain teardown separate so the final buffered bytes can still trigger didDrain().

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/webcore/fetch/FetchTasklet.zig` around lines 922 - 930, The
function clearStreamCancelHandler currently clears drain_handler and drain_ctx
prematurely, which prevents the final chunk from reporting consumption
correctly. Modify clearStreamCancelHandler in FetchTasklet to only clear
cancel_handler and cancel_ctx, keeping drain_handler and drain_ctx intact.
Implement a separate method to clear drain callbacks after the last buffered
bytes are fully consumed, ensuring proper crediting of the HTTP/2 stream in
onBodyReceived.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/http/h2_client/ClientSession.zig`:
- Around line 355-363: The current consumeResponseBodyByHttpId updates
per-stream stream.consumed_bytes with JS-delivered bytes and then calls
replenishWindow(), which lets stale decompressed-byte surplus drive
WINDOW_UPDATEs; modify consumeResponseBodyByHttpId (and corresponding logic used
by replenishWindow) to limit carried consumed balance to at most the
wire-received bytes for that stream or track a separate wire_consumed counter:
either add a stream.wire_consumed field and increment that by the actual
received DATA/WIRE bytes (and use that in replenishWindow/when deciding
WINDOW_UPDATEs) or clamp stream.consumed_bytes to the lesser of the existing
consumed balance and the total_wire_received - already_windowed_wire_consumed
before calling replenishWindow; update any places that read
stream.consumed_bytes (e.g., replenishWindow, the WINDOW_UPDATE computation) to
use the new wire-aware value so decompressing reads cannot overspend window
credits.

In `@test/js/web/fetch/fetch-http2-backpressure.test.ts`:
- Around line 9-13: Move the test cases from fetch-http2-backpressure.test.ts
into the existing fetch-http2-client.test.ts file instead of a new file; if they
must run serially, place those tests outside the existing describe.concurrent
block (e.g. put them in a top-level describe or a separate describe that is not
.concurrent) so they don’t run concurrently with the heavy TLS/debug-build
tests; ensure you import any helpers used by the tests into
fetch-http2-client.test.ts and remove the standalone
fetch-http2-backpressure.test.ts to keep tests co-located with fetch HTTP/2
specs.

---

Outside diff comments:
In `@src/bun.js/webcore/fetch/FetchTasklet.zig`:
- Around line 922-930: The function clearStreamCancelHandler currently clears
drain_handler and drain_ctx prematurely, which prevents the final chunk from
reporting consumption correctly. Modify clearStreamCancelHandler in FetchTasklet
to only clear cancel_handler and cancel_ctx, keeping drain_handler and drain_ctx
intact. Implement a separate method to clear drain callbacks after the last
buffered bytes are fully consumed, ensuring proper crediting of the HTTP/2
stream in onBodyReceived.
🪄 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: fdef9a93-7969-48fe-95b0-f2e4605bfa45

📥 Commits

Reviewing files that changed from the base of the PR and between 4d615e8 and dc050be.

📒 Files selected for processing (8)
  • src/bun.js/webcore/Body.zig
  • src/bun.js/webcore/ByteStream.zig
  • src/bun.js/webcore/ReadableStream.zig
  • src/bun.js/webcore/fetch/FetchTasklet.zig
  • src/http/HTTPThread.zig
  • src/http/h2_client/ClientSession.zig
  • src/http/h2_client/Stream.zig
  • test/js/web/fetch/fetch-http2-backpressure.test.ts

Comment thread src/http/h2_client/ClientSession.zig Outdated
Comment thread test/js/web/fetch/fetch-http2-backpressure.test.ts Outdated
@robobun

robobun commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator Author

Status: Jarred has taken the branch over for landing. Current head ff2fbf3 ("fetch: pause after the first delivered body chunk instead of at a 64K threshold") on top of his f9a8a7f squash/rewrite; CI build #64365 running. He's iterating actively (64352/64354 canceled by his subsequent pushes), so I'm not pushing to avoid stepping on maintainer work.

#64354 triage (Jarred's previous head 94e6dfa, canceled)

29 annotations, dominated by bun install/registry-download tests across many lanes — bun-install-streaming-extract ("extracts a drip-fed tarball correctly", "decompressed size exceeds the limit"), bun-install{,-registry,-patch,-proxy}, bun-info, every migration/*, bundler_{npm,compile} ReactSSR, integration/esbuild, bun-run-dir ("download dependency"), cli/init, shell/ls node_modules, bake/dev/*. All of these go through the HTTP client to download from a registry, so the pattern pointed at the non-streaming HTTP response path (the package-manager download path doesn't set up a JS reader, so body_consumption_tracked stays unset). ff2fbf3 appears to address this.

Prior state through 85f77c9 (before the force-push)

CI #64213 (85f77c9) final triage: 1 hard-failed job (darwin-14-aarch64), one error annotation — test/js/web/fetch/fetch-leak.test.ts › should not leak using readable stream (RSS at iter 250 = 53.3 MB, final = 62.9 MB, 9.6 MB delta vs the test's 5 MB allowance). Cross-branch: the same test hit 11/60 recent non-this-PR builds as a retried-and-passed warning, predominantly on darwin-14-aarch64. Build 58083 (bf070b0) had passed fully. 28 review threads resolved.

What was in the diff (85f77c9 vs main): ~725 lines across 19 src/**/*.rs files + internal-for-testing.ts + 3 test files — h1 maybe_pause_receive/consume_response_body (4 MiB/1 MiB watermarks, keep-alive-pool safety, idle-timer handling incl. the on_data/on_writable guards), coalescing HTTPThread consume queue, h2 per-stream WINDOW_UPDATE gated on min(consumed, unacked), h3 want_read(false) past high water, ByteStream.did_drain + Body.on_stream_consumed + FetchTasklet arm/disarm plumbing, fetchInternals/fetchH3Internals testing counters.

Comment thread src/http/h2_client/ClientSession.zig Outdated
Comment thread src/runtime/webcore/Body.zig Outdated
@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

@robobun fix the comments

Comment thread src/runtime/webcore/ByteStream.zig Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three of my earlier findings are addressed and I found nothing new on c28c8c0, but this reworks h2 per-stream flow control across the JS↔HTTP thread boundary with a new body_consumption_tracked signal — worth a human pass before merge.

Extended reasoning...

Overview

This PR changes how the experimental HTTP/2 fetch client credits the per-stream receive window: instead of sending WINDOW_UPDATE on receipt, it now gates per-stream credit on bytes actually drained by the JS ReadableStream reader. The plumbing spans ByteStream/ReadableStream.NewSource (new drain_handler), Body.Value (toReadableStream and tee() wiring), FetchTasklet (new onStreamConsumed callback + body_consumption_tracked signal management in onStartStreaming/ignoreRemainingResponseBody), HTTPThread (new scheduleResponseBodyConsumed queue + drain), Signals (new atomic), and ClientSession/Stream (new consumed_bytes counter + reworked replenishWindow). Three new wire-level tests cover stalled reader, draining reader, and reader.cancel() fallback.

Security risks

No new attack surface for untrusted input — the change governs outbound WINDOW_UPDATE based on local reader behaviour. The risk profile is the inverse: getting it wrong stalls or leaks streams. My earlier reviews found three such cases (S3/abandoned-body deadlock, res.clone() deadlock, pre-buffered drain() floor) and all three are now fixed; the body_consumption_tracked signal was introduced specifically so paths that don't report consumption (S3, ignored bodies) degrade to receipt-based instead of deadlocking.

Level of scrutiny

High. This is production-critical networking with cross-thread atomics, and the correctness depends on every path that flips body_consumption_tracked also keeping consumed_bytes fed (or disarming the signal). The fact that three real bugs surfaced across two review rounds before reaching the current state confirms it's subtle enough to warrant a maintainer's eyes on the final design — particularly the ignoreRemainingResponseBody saturate-then-disarm sequence and whether any other ByteStream delivery path was missed.

Other factors

CI on the latest commit shows a timeout in fetch-http2-client.test.ts on x64-asan, which the author has documented as a pre-existing shard-7 flake also seen on unrelated PRs; the new fetch-http2-backpressure.test.ts is not in that shard. Jarred is already engaged on the thread. All CodeRabbit and claude inline threads are resolved.

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

@robobun make this PR work for http/1 and http/3. Write extensive tests. Merge main. Make this good.

@robobun robobun changed the title http: couple per-stream h2 WINDOW_UPDATE to JS body consumption http: couple fetch() receive backpressure to JS body consumption (h1/h2/h3) May 2, 2026
Comment thread src/bun.js/webcore/fetch/FetchTasklet.zig Outdated
Comment thread src/http.zig Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/bun.js/webcore/fetch/FetchTasklet.zig (1)

894-910: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Seed backpressure with the bytes that were already buffered before streaming started.

When this path returns .owned, Line 907 hands the prebuffered body chunk to the new ByteStream, but the HTTP thread never gets told that those bytes are now outstanding against the JS reader. Their later didDrain callbacks will subtract from 0, so h1/h2/h3 can still admit another full high-water window on top of whatever was already sitting in scheduled_response_buffer when res.body was first touched. Please enqueue that initial buffered length when arming body-consumption tracking.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/webcore/fetch/FetchTasklet.zig` around lines 894 - 910, The
prebuffered bytes in this.scheduled_response_buffer.list are returned as an
owned list (via scheduled_response_buffer.toManaged) but the HTTP thread’s
backpressure accounting isn't informed, so enqueue the initial buffered length
into the body-consumption tracking before returning .owned; in FetchTasklet.zig
update the code path that returns .owned (referencing scheduled_response_buffer,
toManaged, and scheduled_response_buffer.list) to call the existing enqueue/arm
function used for body-consumption (or directly increment the outstanding-bytes
counter / invoke didDrain bookkeeping) with scheduled_response_buffer.items.len
(or the byte length equivalent) so the HTTP layers are aware those bytes are
outstanding to the JS reader.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/bun.js/webcore/fetch/FetchTasklet.zig`:
- Around line 875-881: The current logic arms body_consumption_tracked in the
path that constructs a drain handler for Body.toReadableStream and Body.tee,
buttee() clones share a single transport receive window so per-branch reads can
incorrectly report shared bytes consumed; update the logic in FetchTasklet.zig
to detect when the Body was produced via Body.tee (or when a shared tee source
exists) and either disable/skip calling scheduleResponseBodyConsumed/from arming
signal_store.body_consumption_tracked for tee clones, or move the consumption
accounting to the shared tee source so only the slowest branch advances
transport credit; specifically adjust the code paths that install the
drain_handler and the call sites of scheduleResponseBodyConsumed and
signal_store.body_consumption_tracked.store(true, .release) (also apply same
change to the analogous block around lines 952-956) so that per-clone drain
events do not prematurely reopen transport reads for a shared tee source.

In `@test/js/web/fetch/fetch-backpressure.test.ts`:
- Around line 535-541: The settle() helper returns after only one unchanged
100ms sample; update it so it waits for two consecutive identical samples 100ms
apart before returning to match the comment and ensure wantRead(false) actually
took effect. Specifically, in settle() use received() and track two successive
samples (e.g., prev and curr) taken with a 100ms await between them and only
break/return when curr === prev; reference the settle() function and the
received() counter so the loop enforces two stable samples.

---

Outside diff comments:
In `@src/bun.js/webcore/fetch/FetchTasklet.zig`:
- Around line 894-910: The prebuffered bytes in
this.scheduled_response_buffer.list are returned as an owned list (via
scheduled_response_buffer.toManaged) but the HTTP thread’s backpressure
accounting isn't informed, so enqueue the initial buffered length into the
body-consumption tracking before returning .owned; in FetchTasklet.zig update
the code path that returns .owned (referencing scheduled_response_buffer,
toManaged, and scheduled_response_buffer.list) to call the existing enqueue/arm
function used for body-consumption (or directly increment the outstanding-bytes
counter / invoke didDrain bookkeeping) with scheduled_response_buffer.items.len
(or the byte length equivalent) so the HTTP layers are aware those bytes are
outstanding to the JS reader.
🪄 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: c20a4ad0-251d-4e28-99e8-08f373953653

📥 Commits

Reviewing files that changed from the base of the PR and between 6aa5782 and f35a1b5.

📒 Files selected for processing (13)
  • src/bun.js/webcore/ReadableStream.zig
  • src/bun.js/webcore/fetch/FetchTasklet.zig
  • src/deps/uws/quic/Stream.zig
  • src/http.zig
  • src/http/H3Client.zig
  • src/http/HTTPThread.zig
  • src/http/InternalState.zig
  • src/http/h3_client/ClientContext.zig
  • src/http/h3_client/ClientSession.zig
  • src/http/h3_client/Stream.zig
  • src/http/h3_client/callbacks.zig
  • src/js/internal-for-testing.ts
  • test/js/web/fetch/fetch-backpressure.test.ts

Comment thread src/runtime/webcore/fetch/FetchTasklet.zig Outdated
Comment thread test/js/web/fetch/fetch-backpressure.test.ts Outdated
Comment thread src/http/InternalState.zig Outdated
Comment thread src/http/http.zig Outdated
Comment thread src/bun.js/webcore/ByteStream.zig Outdated
Comment thread test/js/web/fetch/fetch-backpressure.test.ts Outdated
Comment thread src/http/Signals.zig Outdated
Comment thread test/js/web/fetch/fetch-backpressure.test.ts Outdated
Comment thread src/http.zig Outdated
Comment thread src/http.zig Outdated
Comment thread src/runtime/webcore/ByteStream.zig Outdated
Comment thread src/http/http.zig Outdated
Comment thread test/js/web/fetch/fetch-backpressure.test.ts Outdated
Comment thread test/js/web/fetch/fetch-backpressure.test.ts Outdated
Comment thread src/http/Signals.zig Outdated
Comment thread src/runtime/webcore/fetch/FetchTasklet.zig Outdated
@robobun

robobun commented May 13, 2026

Copy link
Copy Markdown
Collaborator Author

This also fixes #22514 ("Out of memory while copying request body" panic in ByteStream.onData) — same root cause, a stalled res.body.getReader() lets the HTTP client buffer the full response in ByteStream.buffer until allocation fails.

Verified locally on the HTTP/1.1 path with a firehose server + stalled reader: without this change RSS climbs ~1.3 GB/s and panics; with it the socket pauses at the high-water mark and RSS stays flat, then resumes and delivers intact data once the reader drains.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review is now clean — all 23 prior inline findings resolved and nothing new on this pass. Given the scope (cross-thread flow-control wiring across h1/h2/h3, socket pause/resume around the keep-alive pool, plus the acknowledged h3 decompression-currency follow-up), this one's worth a human look before merge.

Extended reasoning...

Overview

This PR couples fetch() receive-side flow control to JS body consumption across HTTP/1.1, HTTP/2, and HTTP/3. It adds a did_drain hook to ByteStream, a new body_consumption_tracked signal, a coalescing JS→HTTP-thread consume queue in HTTPThread, and per-transport gating: h2 per-stream WINDOW_UPDATE clamped to min(consumed, unacked), h1 us_socket_pause/resume around 4 MiB / 1 MiB watermarks with idle-timer suspension and a defensive resume before keep-alive pool release, and h3 lsquic_stream_wantread toggling. ~715 lines across 19 .rs files plus a 808-line test file and two timeout bumps. The feature was originally written in Zig and re-implemented in Rust after main switched runtimes.

Security risks

None identified. No auth/crypto/permissions surface; the change is internal flow-control accounting. The new fetchInternals.h1BackpressureCounts() / fetchH3Internals.liveCounts().bodyBytesReceived are read-only process-wide counters exposed via bun:internal-for-testing.

Level of scrutiny

High. This is production-critical HTTP client code with several subtle invariants: cross-thread atomic signal ordering (arm/disarm + sentinel), uSockets repeat-recv interaction with pause_stream(), keep-alive pool hand-off of a possibly-paused socket, and byte-counting currency (post-dechunk/post-decompress vs. wire). The review history bears this out — eight distinct correctness issues were found and fixed across multiple rounds (S3/abandoned-body deadlock, tee() wiring, drain()/toBufferedValue floors, chunked-framing and Content-Encoding currency mismatches, idle-timer re-arm during repeat-recv, resume-counter symmetry on the close branch). All are resolved, but the density of edge cases argues for a maintainer pass on the final shape.

Other factors

  • Jarred is already engaged (requested the h1/h3 extension and asked robobun to address comments) but hasn't signed off on the post-Rust-port revision.
  • The author explicitly deferred one known issue to a follow-up (h3 outstanding_body_bytes is pre-decompression; the gzip-on-incompressible floor is theoretically reachable at multi-GiB over h3).
  • CI is currently blocked by darwin-x64 agent infra (build-cpp succeeded then buildkite-agent ENOENT on artifact upload), not by this diff; a maintainer retrigger is needed regardless.
  • Test coverage is thorough (12 tests across protocols + keep-alive regression + two pipeThrough tests for #28035), and adjacent suites are reported green.

Jarred-Sumner and others added 12 commits June 25, 2026 15:57
…enum

The HTTP-thread callback now reads one AtomicU8 (AutoPause / BufferAll /
Ignore) instead of a plain bool + an AtomicBool. Ignore being atomic
also closes the pre-existing cross-thread read of ignore_data without
the FetchTasklet mutex.
Polls RSS until stable for 60ms rather than measuring after a fixed
sleep, so slow hosts wait until the server has actually flooded instead
of vacuously passing.
…tion

on_start_buffering had the same load-mode/store-paused race that
ignore_remaining_response_body guarded — a concurrent callback past its
mode load can pause after the JS-thread swap, leaving arrayBuffer()
with no signal_drained to self-heal. set_body_receive_mode now does the
unconditional store(false)+schedule for both transitions.
The transport now reads `signals.is_receive_paused()` (mode == Paused);
the callback transitions AutoPause→Paused via CAS, JS transitions
Paused→AutoPause (signal_drained) or →BufferAll/Ignore via CAS/swap.
A single atomic removes the load-mode/store-paused interleaving the
review flagged — the CAS either commits the pause or fails because JS
already moved to a terminal mode.
A queued resume that arrives while body_receive_mode is still Paused
(e.g. on_start_streaming flushing body_out_str before the reader pulls)
should not briefly un-pause the socket. The keep-alive release path
un-pauses unconditionally inline since the socket is leaving this
request regardless of mode.
Only fetch wires body_receive_mode (via Store::to_with_backpressure());
install manifest fetches and S3 non-download ops use Store::to() which
leaves it None, so they keep the once-at-end callback their handlers
expect. Restores the report_progress gate in _from_multiple_packets
and adds body_receive_mode.is_some() as the fetch opt-in.
When the ByteStream has no lock, no pipe, and no buffer_action after a
chunk is delivered, there is nothing to apply backpressure against.
Flip to BufferAll so the body completes and the FetchTasklet releases
its Strong on the stream source. Fixes native-source-onclose-leak
(releaseLock without cancel left the transport paused forever and the
source wrapper pinned).

Also:
- skip maybe_pause_receive for upgraded (101) connections; the duplex
  protocol needs continuous reads (fetch.upgrade.test.ts).
- call on_start_buffering from ValueBufferer before materialising the
  stream so pipe consumers (HTMLRewriter) never pause.
- unref poll_ref once the fetch() promise resolves and re-ref when a
  body consumer attaches, so an unread Response no longer keeps the
  process alive indefinitely (unref-fixture-2.ts).
- restore the JSC_BORROW doc on NewSource.global_this.
- fetch-backpressure.test.ts: destroy tracked h2 sockets on dispose
  (Http2SecureServer.close() otherwise waits forever); tighten the
  keep-alive cancel and sequential-drain assertions.
…bound on macOS/ASAN

on_writable's RequestStage::Body arm re-armed the idle timer that
maybe_pause_receive had cleared, so a full-duplex request with a
stalled reader would spuriously time out once the upload finished.
The matching guard already exists on the on_data arms.

fetch-backpressure.test.ts: subprocess RSS captures JIT warmup and
lazy dylib faulting (boringssl for TLS) which on macOS and ASAN can
exceed the 16 MiB body. The in-process 'server stops writing' tests
prove the h1 pause without that noise, so skip the RSS bound there.
…GiB body for server-stops

ByteStream::on_data's pending path now signals drained even when the
delivered chunk overflowed the reader's view, so a releaseLock() in
the await continuation still gets one more on_body_received to run
drop_backpressure_if_unobserved with the stream unlocked.

fetch-backpressure.test.ts: the subprocess RSS delta captures JIT
warmup, mimalloc chunking and TLS dylib faulting, which exceeds the
16 MiB body on many CI lanes (pre-existing on c050a12); drop that
assertion and rely on the in-process 'server stops writing' tests as
the proof of the h1 pause. Raise that test's body to 1 GiB since some
CI hosts have loopback tcp_rmem+tcp_wmem autotune approaching 256 MiB.
…y_receive_mode gate in single-packet chunked -2 arm

on_start_streaming_http_response_body_callback drains
scheduled_response_buffer into the new ByteStream but left
body_receive_mode at Paused, so the resume it schedules was a no-op
on the HTTP thread. A reader whose first on_pull found the drained
buffer smaller than its view returned Pending without ever
signalling, leaving the socket paused past a server FIN
(fetch.stream 'can handle socket close' on darwin).

Also mirror the body_receive_mode.is_some() clause in
_from_single_packet's chunked -2 arm for consistency with the
_from_multiple_packets sibling.
A FIN/RST that arrives while the read poll is paused is only observable
through the poll, so resume_receive returning early on
is_closed_or_has_error() stranded the request: no on_end/on_close ever
fired and reader.read() stayed pending forever with the idle timeout
disabled. Only a closed socket skips the re-arm now; an errored or
shut-down one is resumed so the normal readable/EOF/error dispatch
delivers the failure.

Also stop the POSIX repeat-recv loop when on_data paused the socket,
matching the Windows arm, so us_socket_pause from a data handler is
honored within the same readable event instead of pulling the rest of
the kernel buffer.
@Jarred-Sumner
Jarred-Sumner force-pushed the farm/0a9cea98/h2-window-update-backpressure branch from dd47c91 to a1e272f Compare June 26, 2026 00:00
Comment thread src/http/HTTPThread.rs
resume_receive_by_http_id can reach fail_all() via flush() returning
Err, which tears down the session when its internal ref_scope guard
drops; the subsequent drain_response_body_by_http_id would then
dereference freed memory. The trigger is currently unreachable
(us_socket_write clamps negatives to 0 on both TCP and SSL), so this
is defense-in-depth matching the single-call sibling dispatch loops.
@Jarred-Sumner
Jarred-Sumner merged commit 81dfd59 into main Jun 26, 2026
75 of 77 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/0a9cea98/h2-window-update-backpressure branch June 26, 2026 01:27
robobun added a commit that referenced this pull request Jul 25, 2026
…complete

The receive-backpressure change in #29831 made fetch pause the transport
after the first body chunk unless a consumer calls on_start_buffering.
Bun.write's file-destination Locked arm registered on_receive_value and
overwrote locked.task without ever signalling the producer, so the
transport parked at ~128 KB and resolve() never fired. The awaited
promise hung forever and no file was created.

Call on_start_buffering with the original producer task before replacing
locked.task, mirroring ValueBufferer.
robobun added a commit that referenced this pull request Jul 25, 2026
…complete

The receive-backpressure change in #29831 made fetch pause the transport
after the first body chunk unless a consumer calls on_start_buffering.
Bun.write's file-destination Locked arm registered on_receive_value and
overwrote locked.task without ever signalling the producer, so the
transport parked at ~128 KB and resolve() never fired. The awaited
promise hung forever and no file was created.

Call on_start_buffering with the original producer task before replacing
locked.task, mirroring ValueBufferer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fetch().body piped through TransformStream does not propagate backpressure

3 participants