Skip to content

fetch: release the in-flight request slot while a response body is paused for backpressure - #35272

Open
robobun wants to merge 2 commits into
mainfrom
farm/19e3b468/fetch-unread-response-wedge
Open

fetch: release the in-flight request slot while a response body is paused for backpressure#35272
robobun wants to merge 2 commits into
mainfrom
farm/19e3b468/fetch-unread-response-wedge

Conversation

@robobun

@robobun robobun commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Problem

An unread, still-reachable Response (body larger than the ~64 KB that arrives with the headers) permanently occupies one BUN_CONFIG_MAX_HTTP_REQUESTS slot. At the default cap of 256, the 257th fetch(), and every later fetch() to any origin (including a host never contacted before), queues forever with no timeout, no error, and no diagnostic.

Common shapes that hit it: response caches keeping Response objects, "collect all responses then process", pollers pushing res into an array after checking res.ok. The slot is only reclaimed when the retained Response is GC'd (its finalizer schedules an Ignore resume). Node/undici never wedges: it holds 300 sockets and every probe answers in under 40 ms.

const hold = [];
for (let i = 0; i < 300; i++) hold.push(await fetch(`${O1}/x${i}`)); // headers seen, body never read
await fetch(`${O2}/probe`);  // HANGS forever, even though O2 was never contacted

Deterministic 3/3 on both 1.4.0-canary.1+5b98630ac and release-asan main (e383be4); lowering BUN_CONFIG_MAX_HTTP_REQUESTS to 64 moves the wedge point to 64.

Cause

FetchTasklet::callback transitions body_receive_mode from AutoPause to Paused after the first body chunk lands in scheduled_response_buffer, and maybe_pause_receive then parks the socket. If JS never touches .body / .arrayBuffer() / etc., nothing ever flips the mode back, so the socket stays parked and the request never reaches the !has_more terminal callback that decrements ACTIVE_REQUESTS_COUNT.

Fix

A socket parked for JS-side backpressure is not doing I/O, so it should not count against the active-request cap:

  • maybe_pause_receive decrements ACTIVE_REQUESTS_COUNT when it parks the socket and records that it did so on client.flags.released_active_slot.
  • resume_receive re-increments and clears the flag. A resume can briefly push the count above max; drain_events only gates admission of new requests on it, so an already-in-flight resume is never blocked.
  • The terminal callback in on_async_http_callback_raw skips its own decrement when the flag is still set (request ended while paused: abort, peer close, or GC-driven Ignore drain).

Only FetchTasklet wires up body_receive_mode (via Signals::to_with_backpressure), so bun install, S3, and other HTTP-client users never enter maybe_pause_receive and are unaffected. HTTP/2 and HTTP/3 do not use this HTTP/1.1 per-socket pause path.

Relation to #33007

That PR makes the cap per-origin. It would stop the cross-origin starvation in the repro above (probe to O2 would work), but 300 unread Responses to one origin would still wedge that origin. This change is orthogonal: the slot is released regardless of which origin holds it.

Verification

New test in test/js/web/fetch/fetch-backpressure.test.ts spawns a child with BUN_CONFIG_MAX_HTTP_REQUESTS=4, retains 12 unread Responses from origin A, and asserts:

  • the retain loop never wedges (wedgedAt: 0, retained: 12);
  • a subsequent probe to origin A completes;
  • a subsequent probe to origin B (never contacted) completes;
  • draining one retained Response via .arrayBuffer() still returns the full body (resume path re-acquires and completes).

Without the src/ change, wedgedAt is 5 and both probes report false.

Also re-ran fetch-abort-queued, fetch-keepalive, client-fetch, body-stream (9086 tests), and the rest of fetch-backpressure (including the peer-RST/FIN-while-paused cases, which exercise the "terminates while slot already released" branch). No regressions; the only failures reproduce identically without this diff under debug+ASAN.


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

…used for backpressure

A retained Response whose body has not been read pauses the underlying
socket after the first body chunk (receive-side backpressure), but kept
counting against BUN_CONFIG_MAX_HTTP_REQUESTS. Holding 256 such
Responses permanently wedged every later fetch(), including to origins
never contacted before, with no timeout or diagnostic. The slot was
only reclaimed when GC finalized the Response and scheduled an Ignore
resume.

A parked socket is not doing I/O, so it should not count against the
active-request cap. maybe_pause_receive now decrements the counter and
records that it did so on the client; resume_receive re-increments, and
the terminal callback skips its own decrement when the request ends
while still paused (abort/close/GC). Resumes can briefly push the count
above the cap, which only gates admission of new requests.

Only fetch() wires up body_receive_mode (Signals::to_with_backpressure),
so bun install, S3, and other HTTP client users never enter
maybe_pause_receive and are unaffected.
@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 32847cef-1352-4bf7-8a03-0f8c7ffac5a8

📥 Commits

Reviewing files that changed from the base of the PR and between 892b1da and f9193bc.

📒 Files selected for processing (3)
  • src/http/AsyncHTTP.rs
  • src/http/lib.rs
  • test/js/web/fetch/fetch-backpressure.test.ts

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

@github-actions

Copy link
Copy Markdown
Contributor

Found 1 issue this PR may fix:

  1. HTTPS requests hanging (regression 1.2.23 -> 1.3.x) #26066 - Reports HTTPS fetch() requests hanging indefinitely after ~400-500 successful requests (AWS SDK calls), with no timeout or error — matches the exact symptom of HTTP/1.1 request slot exhaustion from unread response bodies permanently holding slots.

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

Fixes #26066

🤖 Generated with Claude Code

@robobun

robobun commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Re #26066: that issue was opened 2026-01-14 against 1.3.3, but the receive-side backpressure pause this PR adjusts landed in #29831 on 2026-06-25, so the original report there has a different cause. Some of the later comments on that thread may be hitting this after #29831 shipped, but I can't verify that from here, so I'm not adding a Fixes link.

@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.

I didn't find any bugs, but this changes what BUN_CONFIG_MAX_HTTP_REQUESTS actually caps (from open connections to actively-reading connections — paused sockets now unbounded), and it interacts with #33007's per-origin work, so a maintainer should sign off on the design.

What was reviewed:

  • Counter balance across all paths (start/pause/resume/terminal, terminal-while-paused) — every combination nets to zero.
  • drain_events admission uses >= comparisons only, so resume_receive briefly overshooting max is harmless.
  • maybe_pause_receive runs inside uws_loop.tick(); process_events calls drain_events unconditionally each iteration, so no explicit wakeup is needed.
  • client.flags is never reset on the HTTP-thread copy mid-request (only sync_progress_from copies it to the JS-thread real, which is not the copy the terminal callback reads).
Extended reasoning...

Overview

The PR releases the global ACTIVE_REQUESTS_COUNT slot when maybe_pause_receive parks an HTTP/1.1 socket for JS-side backpressure, and re-acquires it in resume_receive. A new client.flags.released_active_slot bit tracks whether the slot is currently released so the terminal callback in on_async_http_callback_raw can skip its own decrement when a request ends while still paused. Touches src/http/lib.rs (pause/resume), src/http/AsyncHTTP.rs (terminal path + flag read), and adds a subprocess test in fetch-backpressure.test.ts.

Security risks

None identified. This is internal resource-accounting for the fetch client; no parsing of untrusted input, no auth/crypto surface. The one resource-exhaustion angle is that paused sockets are now unbounded by the cap — that's a design trade-off (matches Node/undici behavior per the description) rather than a vulnerability, but worth a maintainer's eye.

Level of scrutiny

High. This is core HTTP client concurrency control. The counter is a process-global atomic that gates every fetch/install/S3 request; an accounting error would either deadlock (over-decrement gone negative → debug_assert in debug, silent wrap in release) or leak slots. I traced every increment/decrement pairing:

  • never-paused: +1 (on_start) / −1 (terminal) → 0
  • pause→resume→terminal: +1 / −1 / +1 / −1 → 0
  • pause→terminal-while-paused (abort/peer-close/GC-Ignore): +1 / −1 / skip → 0
  • multiple pause/resume cycles: each cycle is idempotently guarded by the flag

I also verified the flag lives on client.flags (not state.flags), so state.reset() on redirect/retry does not clear it; and that no code path assigns Flags::default() to client.flags mid-request on the HTTP-thread copy. The done() keep-alive path (lib.rs:4308) clears state.flags.receive_paused without touching the new flag, but that path is only reachable after the body completes, which requires a prior resume_receive (which clears the flag) — maybe_pause_receive is only called when has_more.

Other factors

The change is small and the accounting looks correct, but it redefines what the cap means: previously it bounded open in-flight requests (≈ sockets for HTTP/1.1); now it bounds only requests actively doing I/O, and an arbitrary number of paused sockets can be held open. That may be exactly the intended semantics (the description argues it, and Node behaves this way), but it's a policy call that overlaps with #33007 and warrants a human decision rather than a bot approval. The test follows harness conventions (subprocess with bunEnv spread, drains all pipes concurrently, asserts stderr/stdout before exitCode) and is deterministic per the description.

@robobun

robobun commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:33 AM PT - Jul 23rd, 2026

@robobun, your commit f9193bc2b4def880ae97ecefd9fa0fb0ddefcdbd passed in Build #78656! 🎉


🧪   To try this PR locally:

bunx bun-pr 35272

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

bun-35272 --bun

@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.

I didn't find bugs in the counter accounting, but this changes what MAX_SIMULTANEOUS_REQUESTS bounds — from "in-flight requests" to "requests actively doing I/O" — so a human should sign off on the design.

What was reviewed:

  • Counter balance across start/pause/resume/terminal, including pause→resume→pause and terminal-while-paused (abort/RST/GC) — all net to zero; the flag is read before callback.run and the dealloc.
  • sync_progress_from copies client.flags (incl. the new bit) to the JS-thread real, but that copy never re-enters on_start/terminal, so no double-count.
  • drain_events only gates admission on the count, so a resume overshooting max doesn't block; the tick loop shape means the freed slot is observed on the next iteration.
Extended reasoning...

Overview

Adds a released_active_slot flag to HTTPClient::Flags and uses it to decrement ACTIVE_REQUESTS_COUNT when maybe_pause_receive parks an HTTP/1.1 socket for JS-side backpressure, re-increment in resume_receive, and skip the terminal decrement in on_async_http_callback_raw when the request ends while still paused. Adds a subprocess test that caps BUN_CONFIG_MAX_HTTP_REQUESTS=4, retains 12 unread Responses, and asserts later fetches to both origins complete and a retained body still drains.

Security risks

None direct. The change loosens a concurrency cap: previously N paused Responses wedged fetch; now they release their slots, so an unbounded number of retained Responses translates to an unbounded number of open sockets/fds instead of a hang. That's closer to Node/undici semantics and is arguably strictly better than a silent process-wide wedge, but it does mean BUN_CONFIG_MAX_HTTP_REQUESTS no longer bounds total open HTTP/1.1 connections — only those actively reading.

Level of scrutiny

High. This is a semantic change to a global resource limiter in the HTTP client hot path, touches an atomic counter accessed from the HTTP thread, and interacts with a related open PR (#33007, per-origin caps). The mechanics are small and the accounting checks out on every path I traced (start +1; pause −1 with flag set; resume +1 with flag cleared; terminal −1 only if flag clear), but whether "paused ⇒ not counted" is the right invariant — vs. e.g. an idle timeout on paused bodies, or bounding paused sockets separately — is a maintainer call.

Other factors

  • Only FetchTasklet sets body_receive_mode, so bun install/S3 don't reach maybe_pause_receive; H2/H3 have their own resume_receive_by_http_id paths that don't touch this flag, consistent with the PR's claim that this is H1-only.
  • client.flags is never bulk-reset between pause and terminal on the HTTP-thread clone (only sync_progress_from copies it outward to real), so the flag survives to the terminal read.
  • The new test follows harness conventions (subprocess, bunEnv spread, concurrent pipe drain, exit-code asserted last, ASAN stderr filtered) and asserts exact values.
  • CI is still building; no results yet.

@robobun

robobun commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the diff is green. The new test passed on every lane across both build 78648 and the retrigger build 78656, with zero fetch/http failures on either.

Remaining red on 78656 is unrelated infra/flake:

  • darwin-14-aarch64-test-bun: job Expired waiting for an agent (no tests ran).
  • Every test-level failure in the annotations is tagged flaky (passed on retry): complex-workspace, test-fs-promises-file-handle-readFile, in-process-cron, spawn, test-fastutf8stream-reopen, require-cache.

The three non-retried failures from 78648 (bake/deinitialization ASAN UAF, two worker_threads JSC !exception() asserts on debian-13-x64-asan) did not reappear on the retrigger; bake/deinitialization also shows up on main build 78481.

Ready for review.

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.

2 participants