fetch: cap the GC-triggered drain of an abandoned Response body by robobun · Pull Request #35819 · oven-sh/bun

robobun · 2026-07-25T21:59:51Z

What

When a fetch() Response is dropped without its body being read, the Weak finalizer on the JS wrapper (FetchTasklet::on_response_finalize, scenario 3: Locked body, no stream, no promise) previously flipped the transport to BodyReceiveMode::Ignore and resumed it with no upper bound, so the HTTP thread read the entire remaining body just to discard it and then pooled the connection.

Reproduction

// server advertises Content-Length: 33554432
const res = await fetch(url);
// drop without reading res.body
Bun.gc(true);
// stock bun: origin sends all 32 MiB and the connection is reused

The new test's byte-counting origin shows bodyWritten = 33554432 and the server socket left open on current main; node (undici) destroys the connection after a few MiB.

Fix

Capped drain in the finalizer (FetchTasklet.rs): on_response_finalize now computes the remaining Content-Length and only drains when it is bounded and at most 256 KiB (undici's Readable.dump limit is 128 KiB). Larger or chunked/unknown bodies set signal_store.aborted and schedule_shutdown before calling ignore_remaining_response_body(true), so the existing aborted guard there skips the resume/enable-streaming and the socket is closed. The close arm is the finalizer-safe subset of abort_task (it does not call tracker.did_cancel, which would touch a JSCell during sweep).
Absolute deadline on the discard-drain (http/lib.rs, Signals.rs): for the small-body drain that remains, resume_receive arms a 5 s socket timeout once when the receive mode is Ignore, and on_data no longer re-arms the idle timer per read in that mode. Previously each body read reset the 300 s idle timer, so a trickling origin could keep the pool slot indefinitely.

Verification

test/js/web/fetch/fetch-response-abandoned-drain.test.ts spawns a byte-counting net origin that writes the body in 16 KiB chunks with drain backpressure, fetches and drops the Response, forces GC, and reports (bodyWritten, open|closed):

body	before	after
64 KiB	65536, open	65536, open
32 MiB	33554432, open	~2.8 MiB (one loopback send window), closed

The 64 KiB row guards the pool-reuse path for small bodies. fetch-response-finalizer-sweep and fetch-stream-cancel-leak still pass.

no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch-response-abandoned-drain.test.ts

When a fetch() Response is dropped without its body being read, the Weak finalizer on the JS wrapper (FetchTasklet::on_response_finalize, scenario 3) previously flipped the transport to BodyReceiveMode::Ignore and resumed it with no bound, so the HTTP thread read the entire remaining body just to discard it and then pooled the connection. A 32 MiB response abandoned this way was drained in full. on_response_finalize now checks the remaining Content-Length and only drains when it is bounded and under 256 KiB (undici's Readable.dump limit is 128 KiB). Larger or chunked/unknown bodies set the aborted signal and schedule a shutdown before entering Ignore, so ignore_remaining_response_body does not re-arm the read and the socket is closed instead of drained. The close arm avoids tracker.did_cancel so no JSCell is touched during the sweep. For the small-body drain that remains, the idle timeout is no longer re-armed per read while in Ignore mode: resume_receive arms a 5 s timer once and on_data leaves it alone, making it an absolute deadline for the discard-drain rather than a per-read budget that a trickling origin could keep resetting.

coderabbitai · 2026-07-25T21:59:58Z

Warning

Review limit reached

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

Next review available in: 19 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: 68b5f460-02a2-4d85-9577-937073051f50

📥 Commits

Reviewing files that changed from the base of the PR and between 916492f and f81d23d.

📒 Files selected for processing (4)

src/http/Signals.rs
src/http/lib.rs
src/runtime/webcore/fetch/FetchTasklet.rs
test/js/web/fetch/fetch-response-abandoned-drain.test.ts

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

robobun · 2026-07-25T22:03:05Z

^{Updated 11:45 PM PT - Jul 25th, 2026}

❌ @robobun, your commit f81d23d has some failures in Build #82079 (All Failures)

🧪 To try this PR locally:

bunx bun-pr 35819

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

bun-35819 --bun

github-actions · 2026-07-25T22:04:45Z

Found 1 issue this PR may fix:

Doing a large amount of fetch requests results in a memory leak #20912 - Web crawler doing thousands of fetch requests leaks memory until crashing at 2.46GB RSS; crash stack is in the HTTP thread's SSL socket handling, exactly where the unbounded discard-drain runs on abandoned response bodies

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

Fixes #20912

🤖 Generated with Claude Code

github-actions · 2026-07-25T22:07:47Z

This PR may be a duplicate of:

fetch: abort the request when an unread Response is GC'd instead of draining the body #35809 - Also changes on_response_finalize to handle GC'd unread Response bodies, but always aborts instead of capping the drain

🤖 Generated with Claude Code

robobun · 2026-07-25T22:08:47Z

Related: #35809 touches the same on_response_finalize path but always aborts. This PR keeps the drain-to-reuse behavior for small bodies (≤ 256 KiB remaining, mirroring undici's Readable.dump limit) and only closes beyond that, so the 64 KiB / header-only case still pools the connection. It also avoids tracker.did_cancel in the close arm so the finalizer touches no JSCell, and adds a 5 s absolute deadline on the small-body drain so a trickling origin cannot hold the slot.

Addresses review on the abandoned-body drain cap: - body_size is written by the HTTP-thread callback() under self.mutex; reading it on the JS thread during the Weak finalizer without the lock is a data race on a two-word enum. Lock around the read, matching on_progress_update / on_start_streaming. - Content-Length is the wire (post-encoding) byte count while scheduled_response_buffer holds decompressed bytes; subtracting them mixes units. Compare n directly against the cap instead. - on_data's proxy_tunnel arm now matches the Body/BodyChunk arms and stops re-arming the idle timeout while in Ignore mode; the resume_receive comment notes that tunnels never reach the 5 s arm.

claude

The three findings from the earlier pass — unlocked body_size read, decompressed-vs-wire unit mismatch in the drain heuristic, and the unguarded proxy-tunnel set_timeout arm — look addressed in f4bdff3 / 9d481e4. Beyond the two inline nits, I also checked whether the new self.mutex.lock() in abandon_response_body_from_finalizer can recursively re-enter the same non-recursive bun_threading::Mutex while on_progress_update holds it on the JS thread — examined and ruled out.

Extended reasoning...

The author addressed all three findings from the previous review: the finalizer now locks self.mutex around the body_size read, the drain heuristic compares the raw Content-Length against the 256 KiB cap without subtracting the decompressed buffer, and the proxy-tunnel arm in on_data is now guarded by !is_receive_ignored(). The two remaining findings this run are nits (test-coverage precision on the 64 KiB case, and the disable_timeout interaction with the 5 s drain deadline — neither a regression). The added mutex lock in the Weak finalizer prompted a check for same-thread recursive re-entry via on_progress_update → JS allocation → GC sweep, since bun_threading::Mutex::lock is documented UB when already held by the caller; that path was examined and ruled out. Not approving: this is finalizer-safety and cross-thread-lifecycle code in the fetch hot path, and there is a competing design in #35809 (always-abort vs. capped-drain) that a maintainer should weigh.

…-entrancy deadlock) The lock added in f4bdff3 deadlocks when the Weak finalizer fires for the same tasklet whose on_progress_update already holds self.mutex: on_body_received delivering the final chunk clears readable_stream_ref and then calls bytes.on_data(terminal), which allocates and can GC; the Response is collected, on_response_finalize reaches scenario 3 (body still Locked, stream ref gone, no promise), and abandon_response_body_from_finalizer tried to lock the held mutex. Reproduces as a 100 s timeout in fetch-leak.test.ts fixture #2 on release builds. Reading body_size without the lock is sound here: scenario 2b/3 is only reached with the transport Paused (HTTP thread not in callback()) or with the body already delivered (HTTP thread finished), so there is no concurrent writer. Also let on_timeout close an Ignore-mode drain even under disable_timeout, so the 5 s deadline applies to fetch({timeout:false}) once the user-visible promise has resolved.

…ritable re-arm The finalizer must not take self.mutex (JS-thread re-entrancy deadlock), but the header_progress path can deliver headers with an empty body buffer, leaving the mode at AutoPause and the socket un-paused; callback #2 on the HTTP thread can then be assigning body_size while the finalizer reads it. Mirror the Content-Length into an AtomicUsize that callback() writes and abandon_response_body_from_finalizer reads, so no multi-word enum is accessed across threads without a lock. Also guard on_writable's RequestStage::Body arm the same way as the on_data arms so a still-uploading request body cannot re-arm the idle timeout over the 5 s Ignore-mode drain deadline.

robobun · 2026-07-26T03:31:40Z

Build 81981 and the retrigger 82079:

fetch-leak.test.ts (including fixture #2, which the f4bdff3 mutex lock previously hung) and the new fetch-response-abandoned-drain.test.ts passed on every lane that ran tests.
Both builds have most build-cpp jobs stuck in scheduled for over an hour; the dependent build-bun jobs time out waiting, so the downstream x64 test shards are waiting_failed without having run. This is CI queue backlog, not this diff.
test/cli/run/no-orphans.test.ts is flaky on darwin 14 x64 (unrelated process-reaping test).

No fetch-related failures on any lane that ran. The diff is ready.

robobun · 2026-08-19T20:40:19Z

Closing in favour of #39690, which carries this design forward on top of #39590. This branch conflicts with main since #39590 reworked on_response_finalize. #39690 keeps the bounded drain (a body over 256 KiB, or without a length, is aborted; a shorter one still completes and its connection is reused), but makes the decision in callback() on the HTTP thread, where the body size is already known. That removes the atomic copy of the Content-Length and also keeps the connection for short chunked bodies. The 5 s drain deadline is not carried over, see the notes in #39690. The test from this PR passes on that branch.

robobun assigned Jarred-Sumner Jul 25, 2026

github-actions Bot added the claude label Jul 25, 2026

claude Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread src/runtime/webcore/fetch/FetchTasklet.rs Outdated

Comment thread src/runtime/webcore/fetch/FetchTasklet.rs Outdated

Comment thread src/http/lib.rs