fetch: error the response body stream when a fully-buffered response is aborted

[robobun]

Follow-up to #32659 / #32662.

Problem

When a fetch() response body is fully received before the user touches response.body, aborting via AbortController is a no-op on the body. A reader drains the full buffered bytes and ends with { done: true }; arrayBuffer() / text() resolve with the content. The backing store is only released when the Response becomes unreachable and the NewSource<ByteBlobLoader> wrapper is finalized.

const ac = new AbortController();
const res = await fetch(url, { signal: ac.signal });
await setImmediate();      // body arrives with headers
const r = res.body.getReader();
await r.read();            // { done: false, value: <all bytes> }
ac.abort();
await r.read();            // { done: true }   (Node: rejects AbortError)

Cause

FetchTasklet owns the only abort-signal listener, and clear_abort_signal() runs as part of its teardown once the last body chunk has been delivered. The Response itself has no connection to the signal, so aborting after teardown has nothing to dispatch to. response.body on the now-InternalBlob value builds a NewSource<ByteBlobLoader> whose Option<StoreRef> holds a +1 on the whole body until the source is cancelled or finalized.

#32662 fixes the still-streaming (ByteStream) case, where the tasklet's listener is still attached.

Fix

Give the Response its own abort-signal listener, attached in FetchTasklet::on_resolve and detached in Response::destroy. On abort it errors any existing body stream (rejecting pending reads and releasing the native source) and replaces the body with the abort reason so later body consumers reject.

To error an existing native-backed stream (rather than close it), a ReadableStream__error FFI export is added that drives webStreamControllerError; ReadableStream::error() calls it and then done() to release the source.

The listener takes a +1 ref and a pending-activity count on the AbortSignal, both released in Drop. The pending-activity count is required for AbortSignal.timeout(): cancel_all_timeout_objects (pre-destructOnExit teardown) already releases the extra ref that timeout() took, so dropping the listener in the exit sweep must not also trigger eventListenersDidChange's last-observer deref() on a signal whose m_timeout is still set. This matches FetchTasklet::clear_abort_signal.

Why this is correct

The Fetch spec's "abort a fetch" step 4 reads: if response's body is non-null and is readable, error response's body with error. Node (undici) follows this regardless of whether the body has finished arriving. After this change Bun matches Node on every observed case:

case	Node	Bun before	Bun after
abort then .body.getReader().read()	rejects AbortError	{ value: <bytes> }	rejects AbortError
.body.getReader().read(), abort, read again	rejects AbortError	{ done: true }	rejects AbortError
abort then arrayBuffer()	rejects AbortError	resolves	rejects AbortError
abort(customReason) then read	rejects customReason	{ value: <bytes> }	rejects customReason

Tests

• test/js/web/fetch/fetch-abort-stream-body.test.ts: abort() errors a fully-buffered fetch response body covers all four cases above.
• test/js/web/fetch/fetch-leak.test.ts: fetch Response's abort-signal listener does not leak the AbortSignal checks via heapStats that the listener's +1 on the signal is released after GC, and exercises the exit-teardown path with a pending AbortSignal.timeout() attached.

Existing fetch-abort-*, fetch-response-finalizer-sweep, fetch-tls-abortsignal-timeout, serve-plugins-dev-server, response, body, and body-stream suites pass unchanged.

---

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

[coderabbitai]

Walkthrough

Changes

The PR adds native and Rust ReadableStream.error plumbing, retains an abort listener on resolved fetch responses, propagates abort reasons to response bodies, prevents late buffered data from overwriting terminal state, and adds regression coverage for abort rejection and AbortSignal cleanup.

Fetch abort propagation

Layer / File(s)	Summary
ReadableStream error plumbing src/jsc/bindings/webcore/streams/WebStreamsExports.cpp, src/runtime/webcore/ReadableStream.rs	Adds the ReadableStream__error FFI entry point and corresponding Rust method, which errors the stream and finalizes it.
Response abort listener lifecycle src/runtime/webcore/Response.rs	Stores a native abort listener on each response, errors eligible body streams with the abort reason, and removes references during teardown.
Fetch wiring and regression coverage src/runtime/webcore/fetch/FetchTasklet.rs, test/js/web/fetch/fetch-abort-stream-body.test.ts, test/js/web/fetch/fetch-leak.test.ts	Attaches abort signals to resolved responses, guards terminal buffered bodies from late resolution, and tests abort rejection, custom reasons, and signal reference cleanup.

Possibly related PRs

• oven-sh/bun#32627: Adds related Web Streams controller-error plumbing used by abort-signal handling.
• oven-sh/bun#32662: Updates related fetch and ReadableStream abort/error handling for buffered response bodies.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly summarizes the core change: aborting a fully buffered fetch response now errors the body stream.
Description check	✅ Passed	The description covers the change and verification, though it uses custom headings instead of the template's exact section names.

---

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

[robobun]

^{Updated 7:13 AM PT - Jul 28th, 2026}

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

---

🧪   To try this PR locally:

bunx bun-pr 35093

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

bun-35093 --bun

[claude]

No bugs found; earlier findings (R-2 aliased &mut BodyValue, missing heapStats leak coverage, ASAN timeout) are all addressed. Deferring to a human because this adds a new native-refcount lifetime edge (Response → AbortSignal via BodyAbortListener) whose correctness depends on the pending_activity_ref / cancel_all_timeout_objects / eventListenersDidChange teardown ordering — worth a maintainer eye on that specific interaction.

What was reviewed:

• BodyAbortListener::drop matches FetchTasklet::clear_abort_signal order (clean_native_bindings → pending_activity_unref → unref); attach_abort_signal matches the tasklet's setup order.
• on_abort re-derives get_body_value() per statement (no held &mut across get_body_readable_stream/readable.error()); copies response/global out of *ctx and brackets with ref_/unref so re-entrant native_response unref cannot free the box mid-call.
• ReadableStream__error mirrors ReadableStream__cancelWithReason's catch-scope pattern; webStreamControllerError on an already-errored/closed stream is a no-op, so double-fire (tasklet listener + response listener both attached mid-stream) is benign.
• Leak test uses heapStats().objectTypeCounts.AbortSignal in a subprocess with the isASAN ? 30_000 : 5_000 timeout, and exercises exit-teardown with a pending AbortSignal.timeout().

Extended reasoning...

Overview

Adds a Response-owned native AbortSignal listener so aborting after the fetch body is fully buffered errors the body stream and releases the store, per Fetch spec "abort a fetch" step 4. Touches: a new C++ FFI export ReadableStream__error + Rust ReadableStream::error(); new BodyAbortListener struct on Response with Drop, an unsafe extern "C" callback, and an attach_abort_signal unsafe fn taking *mut Response; a call site in FetchTasklet::on_resolve; behavior tests in fetch-abort-stream-body.test.ts and a heapStats leak test in fetch-leak.test.ts.

Security risks

None identified — no user-controlled parsing, no auth/crypto/permission surfaces. The change is internal lifetime management on the JS thread.

Level of scrutiny

High. This is the memory-safety category REVIEW.md flags as most-blocked: a new intrusive-refcount edge (AbortSignal::ref_/unref + pending_activity_ref/unref), a raw *mut Response backref stored in a heap box, an FFI callback that runs JS and must survive re-entrant teardown, and GC-finalizer / exit-sweep ordering (the cancel_all_timeout_objects justification for taking pending_activity_ref). The pattern does mirror FetchTasklet::clear_abort_signal and other in-tree AbortSignal listener owners (subprocess, RequestContext), which reduces risk, but the specific claim that pending_activity_ref is required to avoid a double-unref of a timeout signal during destructOnExit is subtle enough that someone who owns that machinery should confirm it.

Other factors

Three prior review rounds' findings are all resolved: the R-2 aliased-&mut BodyValue was fixed by re-deriving per statement (7733604); the leak-claim now has a heapStats subprocess test; the ASAN timeout was added (786fdb5) and behavior assertions moved next to existing buffered-body abort coverage. Tests cover the four Node-compat cases from the PR table plus custom-reason passthrough plus the exit-teardown path with a pending AbortSignal.timeout(). on_resolve runs synchronously on the JS thread after the tasklet's own abort check, so add_listener won't fire the callback synchronously here; even if it did, the box's fields are populated before registration and on_abort never reads response.abort_listener, so ordering is safe. The double-listener window (tasklet + response both attached while streaming) errors the stream twice at worst, which webStreamControllerError no-ops on the second call, and to_error_instance is guarded by the BodyValue::Error exclusion.

[Jarred-Sumner]

Is there a simpler & more reliable fix?

[robobun]

I considered a few alternatives; here's why I landed on the listener.

Lazy check (no listener): store Option<AbortSignalRef> on the Response and check signal.aborted() at the top of to_readable_stream / on_pull / body consumers. No callback, so no refcount edge beyond the plain ref()/Drop::unref. The problem is the common path: a small body comes back as one IntoArrayAndDone chunk, so after the first read() the JS stream is already Closed and on_pull is never called again. abort() after that point has nothing to hook into, and the ByteBlobLoader store is still held (it only clears in on_cancel/deinit). So the lazy check fixes res.arrayBuffer() but not the reader path, which is the leak the issue is about.

Keep the FetchTasklet listener alive longer: don't detach in clear_data; instead keep the tasklet alive until on_response_finalize and have abort_listener error the body. That means every fetch({signal}) pins the whole FetchTasklet (http, buffers, Strongs) for the Response's lifetime, and it moves the same detach into the Weak-finalizer path anyway. More retained state, same teardown shape.

The listener as written is the FetchTasklet::clear_abort_signal pattern verbatim (ref_ + pending_activity_ref + add_listener on setup, clean_native_bindings + pending_activity_unref + unref on teardown), just owned by the Response instead of the tasklet. The pending_activity_ref is what makes it safe with AbortSignal.timeout() at exit, and the tasklet already depends on it for the same reason.

One real simplification I can offer: drop ReadableStream__error and the readable.error() call. on_abort then just calls to_error_instance, which cancel()s the stream. The store is released (fixes the leak) and arrayBuffer()/text() reject, but a reader that already has the stream sees {done: true} on its next read() instead of rejecting. That diverges from Node but is arguably spec-compliant (the stream is Closed, not Readable, so step 4 doesn't apply). It removes the new FFI and one moving piece from on_abort. Happy to go that way if the Node-match on that specific read is not worth it.

[Jarred-Sumner]

Can we use a better RAII type than manually incrementing/decrementing a ref count?

[robobun]

Switched to AbortSignalRef (the existing ExternalShared<AbortSignal>) for the signal ref, so unref is field-drop instead of explicit; and a scopeguard for the Response keepalive in on_abort. The one pair that stays explicit is pending_activity_ref/_unref (no existing RAII wrapper; it's just the pair bracketing the listener box's lifetime). 475bd90.

[claude]

Beyond the inline nit, two candidates were examined and ruled out: (1) on_abort double-processing a Locked body — readable.error() transitions the JS stream but does not itself flip BodyValue, so the trailing to_error_instance is the single state write; (2) ReadableStream__error being a no-op on a NativePending stream — get_body_readable_stream returns None for that case (no cached stream, body not Locked), so to_error_instance alone handles it and later .body access builds an already-errored stream.

Extended reasoning...

This run's remaining finding is a narrow race (queued success task clobbering the Error body variant) and is a nit — the primary observable is now correct. The two ruled-out candidates above were the other concerns raised against the current on_abort shape after the R-2 re-derive fix in 7733604 and the AbortSignalRef/scopeguard refactor in 475bd90. Deferring rather than approving because the change adds a new Response→AbortSignal lifetime edge with paired ref/pending_activity_ref bookkeeping and a re-entrant JS call inside a native abort callback, and the author's Jul 22 comment leaves an open design choice (keep ReadableStream__error for Node parity vs. drop it and let the second read see {done:true}) that a maintainer should weigh in on.

[claude]

No further issues found — the four earlier review rounds (R-2 aliasing in on_abort, heapStats leak coverage, ASAN timeout, on_body_received clobber guard) are all addressed. Deferring to a human because this adds a new refcount/lifetime edge (Response → AbortSignal via BodyAbortListener, with pending_activity_ref ordering against cancel_all_timeout_objects) and the author left an open design question about dropping ReadableStream__error in favor of cancel-only.

What was reviewed:

• BodyAbortListener box address stability across add_listener → abort_listener.set → Drop::clean_native_bindings — heap-stable, ctx pointer matches.
• on_abort re-entrancy: Response::ref_ + scopeguard keeps the box's owner live across readable.error(); get_body_value() borrows are now statement-scoped.
• Teardown pattern matches FetchTasklet::clear_abort_signal (clean_native_bindings → pending_activity_unref → field-drop unref); Response::destroy clears the listener.
• on_body_received early-return when body ≠ Locked — scheduled_response_buffer take moved after the guard so bytes aren't dropped on the floor for the still-Locked path only.

Extended reasoning...

Overview

Adds a Response-owned AbortSignal listener so aborting a fetch() after the body is fully buffered still errors the body stream (Fetch spec §abort-a-fetch step 4). New ReadableStream__error FFI + Rust wrapper; new BodyAbortListener struct holding an AbortSignalRef (+1) and a raw *mut Response backref, installed in FetchTasklet::on_resolve and torn down in Response::destroy. on_body_received gains a guard so a queued success callback can't overwrite an Error body the abort listener installed. Tests cover the four behavior cases and a heapStats-based AbortSignal leak check with an AbortSignal.timeout() exit-teardown probe.

Security risks

None. This is fetch-abort semantics; no untrusted-input parsing, auth, or crypto.

Level of scrutiny

High. This is native memory-safety code in the most-blocked review category: intrusive C++ refcounting (ref_/unref/pending_activity_ref), a raw-pointer backref whose validity depends on the box living inside its referent, JS re-entrancy inside a native abort callback, and Drop ordering that interacts with cancel_all_timeout_objects during destructOnExit. The pattern is copied verbatim from FetchTasklet::clear_abort_signal, which reduces risk, but a maintainer familiar with the AbortSignal/timeout teardown path should confirm the pending_activity_ref bracketing is the right shape here too.

Other factors

• Four prior review rounds; every finding addressed with a commit and the author's explanations are precise.
• The author's design-alternatives comment offers to drop ReadableStream__error (cancel-only, {done:true} instead of reject on the second-read case) — that's a Node-compat vs. simplicity tradeoff a maintainer should decide.
• #[repr(C)] Response gains a trailing field; C++ only holds it as an opaque m_ctx pointer, so no layout mirror to update.
• Tests follow harness conventions (subprocess, isASAN timeout branch, heapStats baseline+delta with slack).

[robobun]

CI status for bb33c55: the diff is green on every lane that ran, including x64-asan. Build #84083's only non-flaky red is darwin-14-x64-test-bun exiting 1 with no test-failure annotation (runner issue); earlier builds #78472 / #78555 / #78612 were the same with the darwin-14-aarch64 lane expiring. All other test failures across those builds are retry-passes in unrelated areas (complex-workspace, fastutf8stream-reopen, webview-chrome, inspect, no-orphans, fetch-keepalive, fetch-http2-client, etc.). The new tests in fetch-abort-stream-body.test.ts and fetch-leak.test.ts passed on every completed lane.

[claude]

No issues found on this pass — all four prior findings (the R-2 get_body_value() overlapping borrow, the missing heapStats leak assertion, the ASAN timeout, the on_body_received race guard, and its buffer_reset ordering) are addressed in the current diff. That said, this adds a new Response→AbortSignal refcount edge with a ParentRef backref, re-entrancy handling in on_abort, and a new FFI export, so it's worth a human sign-off (Jarred already weighed in on the pointer type).

What was reviewed:

• BodyAbortListener::on_abort re-entrancy: fields copied out before JS runs; ref_/scopeguard keeps the Response alive across readable.error(); get_body_value() re-derived per statement.
• Drop ordering: clean_native_bindings before pending_activity_unref; AbortSignalRef field-drop releases the +1; destroy() sets abort_listener to None.
• on_body_received guard now sits above buffer_reset.set(false) so the defer still resets the buffer on the abort-raced early return.
• ReadableStream__error mirrors the __cancel exception-scope pattern; leak test uses heapStats in a subprocess with the ASAN timeout.

Extended reasoning...

Overview

Adds a Response-owned AbortSignal listener so aborting after the body is fully buffered still errors the stream (Fetch spec "abort a fetch" step 4). Touches Response.rs (new BodyAbortListener struct + Drop, new abort_listener field, attach_abort_signal, destroy teardown), FetchTasklet.rs (attach in on_resolve; early-return guard in on_body_received so a queued success callback can't overwrite an Error body), ReadableStream.rs + WebStreamsExports.cpp (new ReadableStream__error FFI + error() method), and two test files.

Security risks

None user-facing. The risk surface is memory safety: a new intrusive-refcount edge (AbortSignalRef + pending_activity_ref/_unref), a parent backref (ParentRef<Response>), and a callback that can re-enter JS and reach Response::unref. All of these were iterated on across five commits in response to review.

Level of scrutiny

High. This is native GC-adjacent lifetime code — REVIEW.md's most-blocked category. The on_abort path copies state out of a box that could otherwise be freed mid-call, holds a scopeguard keepalive on the Response, runs webStreamControllerError (which rejects user promises and can synchronously re-enter host fns), then mutates the body. The pending_activity_ref pairing exists specifically for the AbortSignal.timeout() exit-teardown ordering, mirroring FetchTasklet::clear_abort_signal. These are the right patterns, but they warrant a maintainer's eyes.

Other factors

• Jarred already engaged (asked for an RAII pointer type; addressed with ParentRef in 850c348) but has not approved.
• Five fix commits landed in response to review; every thread is resolved.
• CI on 2878c52 was green on all completed lanes per robobun; b296b3d is the current head with a build in progress.
• Tests cover the four behavioral cases against Node, plus a heapStats-based AbortSignal leak check and the AbortSignal.timeout() exit-teardown path.

[zenprocess]

Correction (2026-07-29). This comment originally presented a cross-binary RSS table claiming this PR's build "still leaks ~77 KB/req" on a held path and that "only the cancel path is fixed." Both claims were unsupported and I have removed the table. Two defects in my harness, found on re-audit:

1. The held arm pushed every Response into a retained array in all three cases, so ~71 KiB of the ~73–77 KB/req figure was deliberately-retained live data, not a leak. Real cross-build excess was 2–6 KB.
2. The runs called bare fetch(url) with no AbortSignal, so attach_abort_signal never ran and this PR's BodyAbortListener was never constructed in any measurement. The build labelled "fetch: error the response body stream when a fully-buffered response is aborted #35093" was also 1.4.0-canary (~1266 commits past v1.3.14, spanning the Zig→Rust webcore port), so its deltas are not attributable to this PR.

Apologies for the noise on your PR — the numbers should not have been posted in that form.

---

What I can still report, corrected

We are a Bun-based multi-provider AI proxy (tombii/better-ccflare, leak tracked in #273).

Re-reading this PR properly: the fix is broader than its title suggests. BodyAbortListener is attached unconditionally in FetchTasklet::on_resolve, and on_abort handles the Locked/ByteStream case, so it covers streaming bodies as well as fully-buffered ones. I originally assumed a buffered-only scope and that was wrong.

Why it happens not to help our case — offered as a data point about a real-world consumer, not as a criticism of the fix:

Our retention comes from responses that are discarded without ever being aborted. On a failover or retry we obtain a Response, decide not to forward it, and drop the reference; the body is never read, cancelled, or aborted. Our only AbortController guards time-to-first-byte and its timeout is cleared the moment fetch() resolves on headers, so it cannot fire during body lifetime, and client disconnect propagates via reader.cancel() rather than abort(). self.abort_signal() is therefore None on those requests and the listener is never attached.

So for this specific shape the buffer stays owned by the Response until GC finalisation, which is orthogonal to what this PR changes. That is not a gap in the PR — the abort semantics it implements look correct and match the Fetch spec's "abort a fetch" step 4.

Our mitigation is application-side: explicitly draining discarded bodies in 16 KiB chunks, which closes the source deterministically instead of waiting for finalisation. Within-build on Bun 1.3.2 that measured 142.7 → 18.8 KB/req, and ~2.6x faster than await arrayBuffer() since it never materialises the whole body.

If a "response dropped without abort or read" case is considered in scope for a future change, we would be glad to test it — we have the workload and can measure against a canary build. Happy to share the (now-fixed) harness if useful.

[Jarred-Sumner]

Thanks — reproduced locally with a similar harness (500 sequential fetches of a 73,015-byte body from a local server, RSS delta / N, 10 interleaved rounds, medians below). One row your table is missing changes the read: a plain canary of main without this PR.

case	1.3.2	1.3.14	main canary (d54984555)	this PR (b296b3d5)
await body.cancel()	93.1 KB/req	89.3	9.4	7.6
ac.abort() after resolve	61.6	37.2	47.5	7.7

The body.cancel() improvement is already on main — that one isn't this PR. What this PR adds is the abort() row.

[robobun]

Thanks for the measurements. Agreed on attribution: this PR only touches the ac.abort() path; the body.cancel() improvement on main is the existing on_cancel → clear_data() path (plus whatever GC-pressure work landed separately). The PR body's "cause" section already frames it that way (reader.cancel() already releases the store; abort() did not), so I'll leave the body as-is unless you'd like the table added.

[Jarred-Sumner]

@robobun fix conflicts