fetch: abort the request when an unread Response is GC'd instead of draining the body by robobun · Pull Request #35809 · oven-sh/bun

robobun · 2026-07-25T21:25:04Z

Problem

fetch() → read status/headers → drop the Response without reading or cancelling its body: the Response's Weak finalizer resumes the paused transport and discards the entire remaining body at line rate so the socket can be returned to the keep-alive pool. Verified with 64 KB, 1 MB, 32 MB, 64 MB Content-Length bodies and a 1 GB chunked stream: 100% of every body is pulled after GC, RSS stays flat (the bytes are discarded, not buffered), and the next fetch() to the same origin reuses the connection. redirect: 'manual' → drop pulls the redirect response's body identically.

Operator cost for a health-checker that reads headers only: 20 × 32 MB endpoints costs 640 MB of download over 12 pooled connections in Bun vs ~85 MB (20 × kernel window) over 33 short connections in Node.

Node/undici destroys the socket on GC instead: the transfer stalls at the kernel send window (~2-4 MB), the socket is closed, and the next request opens a fresh connection.

import net from 'node:net';
const MB = 1 << 20, SZ = +(process.argv[2] || 64), CH = Buffer.alloc(65536, 0x61), conns = [];
const srv = net.createServer(s => {
  const c = { w: 0 }; conns.push(c);
  s.on('close', () => c.closed = true); s.on('error', () => {});
  s.once('data', () => {
    s.write(`HTTP/1.1 200 OK\r\nContent-Length: ${SZ * MB}\r\n\r\n`);
    let sent = 0;
    const push = () => {
      while (sent < SZ * MB && !s.destroyed) {
        sent += 65536; c.w += 65536;
        if (!s.write(CH)) return void s.once('drain', push);
      }
    };
    push();
  });
});
srv.listen(0, '127.0.0.1', async () => {
  const base = `http://127.0.0.1:${srv.address().port}`;
  let r = await fetch(`${base}/big`); console.log('status', r.status, '-> drop'); r = null;
  for (let i = 0; i < 12; i++) { await new Promise(x => setTimeout(x, 500)); new Array(2000).fill(0).map((_, j) => ({ j })); }
  await (await fetch(`${base}/small`)).text();
  console.log('origin pushed', (conns[0].w / MB).toFixed(1), 'MB of', SZ, '; conns opened', conns.length);
  process.exit(0);
});

Before: origin pushed 64.0 MB of 64 ; conns opened 1
Node and after: origin pushed 2.6 MB of 64 ; conns opened 2

Cause

FetchTasklet::on_response_finalize (the JSC Weak finalizer on the Response wrapper) handles the "body never touched" case by calling ignore_remaining_response_body, which sets BodyReceiveMode::Ignore and calls schedule_receive_resume() + enable_response_body_streaming(). The HTTP thread un-pauses the socket, and with the receive mode set to Ignore FetchTasklet::callback drops every chunk on arrival and never re-pauses, so the whole body streams through and the socket is then pooled.

reader.cancel() / res.body.cancel() already close the connection via abort_task() since #33231; the GC/drop path still drained.

Fix

on_response_finalize now calls abort_task() before ignore_remaining_response_body(true), the same abort-then-ignore shape as on_stream_cancelled_callback. abort_task() sets aborted = true and enqueues a shutdown on the HTTP thread; ignore_remaining_response_body already skips schedule_receive_resume() / enable_response_body_streaming() when aborted is set, so the paused transport is never resumed and the socket is closed instead of drained.

abort_task() only touches atomics, the AsyncTaskTracker (no-op without a debugger, and only C++ inspector state with one), and the cross-thread shutdown queue; it does not touch JSCells, so it is safe from a Weak finalizer during WeakBlock::sweep. fetch-response-finalizer-sweep.test.ts (runs under collectContinuously) still passes.

Verification

New tests in test/js/web/fetch/fetch-backpressure.test.ts (fetch() abandoned Response body (content-length|chunked)): a raw net origin that counts bytes written serves a 64 MB body; the client reads headers, drops the Response, forces GC, and waits for either the server to observe a close or to finish writing the whole body. Asserts {outcome: 'closed', conns: 2} and that the server wrote well under half of 64 MB; a follow-up fetch() to the same origin opens a fresh connection and completes.

Without the src/ change: {outcome: 'drained', conns: 1} and the server writes the full 64 MB.

Existing fetch-response-finalizer-sweep, fetch-stream-cancel-leak, 33227, fetch-keepalive, fetch-abort-* suites pass unchanged.

no test proof · iteration 2 · 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

…raining the body When a fetch() Response is dropped without reading or cancelling its body, the Response's Weak finalizer (on_response_finalize) called ignore_remaining_response_body, which flips the receive mode to Ignore and resumes the paused transport so the HTTP thread reads-and-discards the rest of the body and returns the socket to the keep-alive pool. For the common "just read the headers" pattern this downloads 100% of every body at line rate (verified up to 1 GB) before pooling the socket. undici destroys the socket on GC instead, so the server stops at the kernel send window and a follow-up request opens a fresh connection. The finalizer now calls abort_task() before ignore_remaining_response_body, exactly as on_stream_cancelled_callback already does for reader.cancel()/ body.cancel() (#33231). abort_task() only touches atomics and the HTTP-thread shutdown queue, so it is safe from a JSC Weak finalizer during sweep; the tracker.did_cancel() call is a no-op without a debugger attached and reads only C++ inspector state with one.

coderabbitai · 2026-07-25T21:25:12Z

Warning

Review limit reached

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

Next review available in: 4 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: ab0f26e8-3283-4f9d-8d3f-8036c670169a

📥 Commits

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

📒 Files selected for processing (2)

src/runtime/webcore/fetch/FetchTasklet.rs
test/js/web/fetch/fetch-backpressure.test.ts

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

robobun · 2026-07-25T21:25:14Z

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

❌ @robobun, your commit d610112 has 1 failures in Build #81836 (All Failures):

step failed outside runner - exit 1 on 🪟 aarch64 - build-bun
step failed outside runner - exit 1 on 🐧 x64-asan - build-bun

🧪 To try this PR locally:

bunx bun-pr 35809

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

bun-35809 --bun

…loopback kernel buffers Addresses review: some CI hosts have tcp_rmem[2]+tcp_wmem[2] approaching 256 MiB, so a 64 MiB body can be fully written into kernel buffers before socket.write() returns false and the server races to settle('drained') before the client's close arrives. 1 GiB matches the neighboring test in this file. Bytes are now counted per-connection and sampled before the second fetch so the assertion measures only the abandoned connection.

claude

Both earlier rounds of feedback are addressed and I don't see anything further, but this is a user-visible change to fetch keep-alive semantics (GC'd Response now closes the socket instead of draining-and-pooling it) inside a JSC Weak finalizer, so a maintainer should sign off.

What was reviewed:

has_live_promise refactor preserves the exact old branch conditions (None / Some(empty) → abort+ignore, Some(live) → keep loading); only the added abort_task() is new.
abort_task() from the finalizer: atomic swap + tracker.did_cancel (early-returns on id == 0) + schedule_shutdown; matches the shape already used by on_stream_cancelled_callback, and ignore_remaining_response_body already skips resume/streaming when aborted is set.
Test: 1 GiB body and per-connection written now match the neighboring backpressure test's kernel-buffer reasoning; settled is first-resolve-wins so a post-close drain on conn 1 can't flip the outcome.

Extended reasoning...

Overview

Two files: src/runtime/webcore/fetch/FetchTasklet.rs changes on_response_finalize (the JSC Weak finalizer on the fetch Response wrapper) so that when the Response is collected with no consumer attached, it calls abort_task() before ignore_remaining_response_body(true) — closing the socket instead of resuming the paused transport and draining the entire body to return it to the keep-alive pool. The branch structure is also refactored from nested if let Some(promise) into a single has_live_promise boolean, which I verified is behavior-equivalent to the old conditions apart from the new abort_task() call. test/js/web/fetch/fetch-backpressure.test.ts adds a subprocess test (content-length + chunked variants) with a raw net origin that asserts the server observes a close and a follow-up fetch opens a fresh connection.

Security risks

None identified. This narrows behavior toward closing connections earlier rather than reading more from the network. No untrusted-input parsing, auth, or crypto is touched.

Level of scrutiny

High. This runs inside a JSC Weak finalizer during WeakBlock::sweep, where touching JSCells is forbidden — the surrounding code already documents this constraint and the PR is careful to stay within it (abort_task() only does an atomic swap, an AsyncTaskTracker::did_cancel that early-returns on id == 0, and a cross-thread shutdown enqueue). The change also mirrors the existing on_stream_cancelled_callback (abort_task() + ignore_remaining_response_body), and ignore_remaining_response_body already gates schedule_receive_resume() / enable_response_body_streaming() on !aborted, so ordering the abort first correctly suppresses the drain. That said, finalizer-path changes in fetch are exactly the kind of thing a maintainer should look at.

Other factors

This is a deliberate behavior change: previously an abandoned Response drained and the socket was pooled; now the socket is closed and the next request reconnects. The PR argues (with a repro) that this matches Node/undici and avoids pulling potentially-large bodies for header-only patterns. That trade-off (bandwidth vs. connection reuse) is a product decision worth a human ack.
My two prior inline comments were both addressed: the test now uses a 1 GiB body with per-connection byte tracking sampled before the second fetch (fc5eaf7), and the numbered-scenario comment framing was replaced with per-branch one-liners (17a0ef0, further condensed in 492adeb/2ef3d9d6). The comment-cop bot's remaining pings are on lines that are now one-liners.
CI build #81603 was still in progress at the time of the last timeline update; the PR description notes the new tests are platform-sensitive and defers to CI for coverage.

claude

No bugs found this pass; my earlier comments (64 MiB vs kernel-buffer flake, shared written counter, stale scenario-numbered comments) are addressed. Deferring to a human because this is a user-visible behavioral change to fetch() — GC'd Responses now close the socket instead of drain-and-pool — and abort_task() is newly reachable from inside a JSC Weak finalizer.

Checked: abort_task() from the finalizer touches only the aborted atomic, AsyncTaskTracker::did_cancel (early-returns when id == 0), and the HTTP-thread shutdown queue — no JSCell access. ignore_remaining_response_body already gates schedule_receive_resume/enable_response_body_streaming on !aborted, so the paused transport is not resumed. The has_live_promise refactor is behavior-equivalent to the old two-arm shape for which cases fall through to ignore. Test now uses 1 GiB + per-connection counters and awaits an observable close/drain rather than a byte threshold.

Extended reasoning...

Overview

Two files: src/runtime/webcore/fetch/FetchTasklet.rs changes on_response_finalize (the JSC Weak finalizer on the Response wrapper) to call abort_task() before ignore_remaining_response_body(true) when the body has no live consumer, mirroring on_stream_cancelled_callback. Net effect: dropping a Response without reading or cancelling its body closes the socket instead of resuming the paused transport and draining the full body so the socket can be pooled. test/js/web/fetch/fetch-backpressure.test.ts adds a subprocess test for both content-length and chunked encodings that asserts the server observes a close (not a full drain) and that a follow-up fetch opens a fresh connection.

Security risks

None identified. No new input parsing, no auth/TLS/crypto changes. The change narrows what happens on GC (close socket vs. keep reading), which if anything reduces bandwidth exposure.

Level of scrutiny

Medium-high. The Rust diff is small (~20 lines net) and reuses the exact abort-then-ignore shape already shipping for reader.cancel() since #33231. But two aspects warrant a maintainer look:

Design trade-off: this swaps keep-alive connection reuse for not draining unbounded bodies when the user drops a Response. The PR argues (with numbers) that this matches Node/undici and is the right call for the header-only-check pattern, but it's a user-visible fetch semantics change.
Weak-finalizer safety: abort_task() is now reachable from WeakBlock::sweep. I traced it — signal_store.aborted.swap is atomic, tracker.did_cancel early-returns when no debugger is attached (id == 0) and otherwise calls Debugger__didCancelAsyncCall (C++ inspector state, no JSCell access per the PR), and schedule_shutdown is a cross-thread enqueue. The existing from_finalizer guards in ignore_remaining_response_body still apply. fetch-response-finalizer-sweep.test.ts under collectContinuously reportedly passes. This looks correct, but new-code-in-a-GC-finalizer is exactly the class REVIEW.md flags for extra care.

Other factors

My two earlier inline comments (64 MiB body vs. documented ~256 MiB kernel buffers → false drained; shared written counter across connections; contradictory scenario-numbered comments) were addressed in fc5eaf7 / 17a0ef0 / 492adeb / 2ef3d9d. The test now uses 1 GiB, per-connection written, samples before the second fetch, and asserts {outcome: 'closed', drained: false, conns: 2, second: 'ok'} — the assertion can fail (pre-fix produces {outcome: 'drained', conns: 1}).
The has_live_promise inversion is equivalent to the old Some(empty) → ignore / None → ignore / Some(live) → keep loading structure; only the added abort_task() changes behavior.
The comment-cop bot fired repeatedly on paragraph-length comments; the author condensed them across the last three commits and the current comments are one-liners.
CI for the latest commit is still building per robobun's status comment, so there's no green build to point at yet.

On alpine 3.23 x64 the Response was not collected within 5 Bun.gc(true) cycles so await settled hung and the test timed out at 30s. Hold a WeakRef, drop the local explicitly, and loop GC+poll up to 200 iterations until the server observes a close or a full drain. If neither happens, report whether the Response was collected so the failure is diagnostic.

claude

No new issues found — my earlier feedback on the test's kernel-buffer sizing and the scenario-numbered comments has been addressed. Deferring to a human because this changes user-visible connection-pooling semantics from inside a JSC Weak finalizer, which is worth a maintainer's eyes.

Checked that the has_live_promise refactor is behavior-equivalent to the old two-arm match for the keep-loading case, and that ignore_remaining_response_body already gates schedule_receive_resume/enable_response_body_streaming on aborted so the abort-then-ignore ordering does what the description says. Traced abort_task() for finalizer safety: tracker.did_cancel early-returns on id == 0 and Debugger__didCancelAsyncCall early-returns without an agent, so the no-debugger path touches only atomics and the HTTP-thread shutdown queue. The test now uses the 1 GiB body and per-connection byte counters, and d610112 replaced the fixed GC loop with a poll on settled.

Extended reasoning...

Overview

The PR changes FetchTasklet::on_response_finalize — the JSC Weak finalizer that fires when a Response wrapper is collected — so that a Response dropped without its body being read or cancelled now calls abort_task() before ignore_remaining_response_body(true), closing the socket instead of resuming the paused transport and draining the entire body to return it to the keep-alive pool. The logic was also refactored from a nested if let Some(promise) into a single has_live_promise early-return; I verified the three cases (Some(live) → keep loading, Some(empty) → abort+ignore, None → abort+ignore) map exactly onto the old arms with the abort added. A new pair of subprocess tests in fetch-backpressure.test.ts covers content-length and chunked encodings.

Security risks

None identified. This is an outbound-client behavior change (close vs. drain on GC) with no untrusted-input parsing. The finalizer-safety question is a memory-safety concern rather than a security one.

Level of scrutiny

High. The change runs during WeakBlock::sweep, where touching JSCells is forbidden — the file already carries a from_finalizer flag on ignore_remaining_response_body for exactly that reason, and abort_task() is now newly reachable from that context. I traced it: the atomic aborted swap and http_thread().schedule_shutdown are heap/queue only; tracker.did_cancel short-circuits on id == 0 (Debugger.rs:716) and Debugger__didCancelAsyncCall short-circuits without an agent (BunDebugger.cpp:1029), so the no-debugger path is JSCell-free. The debugger-attached path goes into InspectorDebuggerAgent::didCancelAsyncCall, which the PR description asserts is inspector-state-only; I didn't independently verify that WebKit path, and a maintainer familiar with the inspector should confirm. The change also alters observable semantics — sockets are no longer pooled after a header-only fetch — which is intentional (matching undici) but is a product decision.

Other factors

Both of my earlier inline comments were addressed: the test now uses a 1 GiB body and per-connection written counters (fc5eaf7), and the numbered-scenario comment framing was replaced with per-branch descriptions (17a0ef0 and follow-ups). The comment-cop bot fired repeatedly and the comments were trimmed in response. The latest commit (d610112) replaced the fixed 5-GC loop with a 200-iteration poll on settled plus a WeakRef fallback that distinguishes not-collected from settle-timeout, which is a reasonable de-flake. ignore_remaining_response_body reads aborted before checking set_receive_mode_terminal and gates both the resume and enable_response_body_streaming on it (FetchTasklet.rs:1808-1819), so the abort-then-ignore ordering in both this finalizer and on_stream_cancelled_callback produces the intended no-resume behavior. Given the finalizer context, the cross-thread shutdown, and the connection-pool semantics change, this warrants a human sign-off even though I found nothing wrong.

robobun · 2026-07-26T02:02:16Z

CI status on build 81836: the new fetch() abandoned Response body tests passed on every test lane that ran (ubuntu-aarch64, debian-aarch64, alpine-aarch64, windows-x64, darwin-aarch64, darwin-14-aarch64). Remaining red is unrelated to this diff:

linux-x64 build-bun, linux-x64-musl build-bun, freebsd-x64 build-bun timed out and windows-aarch64 build-bun failed outside the runner (build infra), which blocked all x64/windows-aarch64 test shards.
test-fastutf8stream-reopen.js, test-fs-read-stream-pos.js, no-orphans.test.ts, fs.watch.test.ts, 20144.test.ts, and fetch.stream.test.ts (Verdaccio port collision) are all flaky on main.

The alpine-x64 timeout seen in build 81734 was addressed in d610112 (GC-poll loop with WeakRef probe instead of a fixed 5 cycles); that lane was blocked by the build timeout in 81836 so did not re-run.

The diff is ready for review.

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

Closing in favour of #39690. Both this PR and #35819 fix the same unbounded drain in the same arm of on_response_finalize, and both conflict with main since #39590. #39690 keeps the bounded drain of #35819 rather than the unconditional abort here: a body under 256 KiB still completes, so its keep-alive connection is reused, and only a longer or unbounded body is aborted. Its tests cover the cases from here as well (Content-Length and chunked bodies are aborted, and a later request still completes).

robobun assigned Jarred-Sumner Jul 25, 2026

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

[autofix.ci] apply automated fixes

4c234f9

github-actions Bot reviewed Jul 25, 2026

View reviewed changes