fetch: release the buffered response body when a streaming response is aborted#32662
fetch: release the buffered response body when a streaming response is aborted#32662robobun wants to merge 2 commits into
Conversation
…s aborted Aborting an in-flight streaming fetch() left the already-received but unconsumed response body in the ByteStream's off-heap buffer. reader.cancel() frees those bytes via on_cancel, but the error path the abort and connection-failure paths take only stored the error and never released the buffer. While the response ReadableStream stayed referenced the bytes were reclaimed only by GC finalization (which never runs while the stream is reachable), so RSS grew proportionally to the un-consumed body on every abort. Release the buffer in ByteStream::append's error arm, matching on_cancel, finalize, and to_buffered_value: an errored stream discards its queued chunks, so the reader observes the error instead of the buffered bytes. Closes #32659
|
Updated 11:16 PM PT - Jun 23rd, 2026
❌ @autofix-ci[bot], your commit e825d93 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32662That installs a local version of the PR into your bun-32662 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
Walkthrough
ChangesFetch abort body memory leak fix
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/fetch/fetch-abort-stream-leak-fixture.js`:
- Around line 25-27: The abort in fetch-abort-stream-leak-fixture.js is
currently gated only by Bun.sleep, which makes the test flaky and
timing-dependent. Update the fixture to wait for an observable buffered-body
condition before calling ac.abort(), using the existing fetch/stream setup in
the test instead of a fixed delay so the abort happens only after unread
response bytes are actually accumulating.
🪄 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: 120f271c-233c-44fb-94e6-7bcdac022264
📒 Files selected for processing (3)
src/runtime/webcore/ByteStream.rstest/js/web/fetch/fetch-abort-stream-leak-fixture.jstest/js/web/fetch/fetch-leak.test.ts
| // Let the in-flight body buffer up client-side, then abort mid-stream. | ||
| await Bun.sleep(15); | ||
| ac.abort(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wait for a real buffered-body condition before aborting.
await Bun.sleep(15) is the only thing making unread response bytes accumulate before ac.abort(). On a slow CI worker that window can elapse before any meaningful backlog exists, so the regression may pass on an unfixed build without exercising the leak. Please gate the abort on an observable server-side/client-side condition instead of wall-clock time.
As per coding guidelines, "Do not write flaky tests" and "await the actual condition to be met rather than testing time passing."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/web/fetch/fetch-abort-stream-leak-fixture.js` around lines 25 - 27,
The abort in fetch-abort-stream-leak-fixture.js is currently gated only by
Bun.sleep, which makes the test flaky and timing-dependent. Update the fixture
to wait for an observable buffered-body condition before calling ac.abort(),
using the existing fetch/stream setup in the test instead of a fixed delay so
the abort happens only after unread response bytes are actually accumulating.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/fetch/fetch-leak.test.ts`:
- Line 847: Remove the explicit per-test timeout from the fetch leak test so it
follows the repo policy and Bun’s default timeout handling. Update the test
definition around the fetch-leak case in fetch-leak.test.ts to no longer pass
120_000, and keep the existing test logic unchanged.
🪄 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: 481faf25-2686-495e-aa21-eb539e4d6c55
📒 Files selected for processing (1)
test/js/web/fetch/fetch-leak.test.ts
| expect(stdout).toContain("held=50"); | ||
| expect(stderr).not.toContain("LEAK"); | ||
| expect(exitCode).toBe(0); | ||
| }, 120_000); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the explicit test timeout override.
Line 847 adds 120_000 as a per-test timeout; this can hide hangs and conflicts with repo test policy.
Suggested fix
-test("aborting an in-flight streaming response frees the buffered body", async () => {
+test("aborting an in-flight streaming response frees the buffered body", async () => {
// ...
-}, 120_000);
+});As per coding guidelines: “CRITICAL: Do not set a timeout on tests. Bun already has timeouts.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, 120_000); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/js/web/fetch/fetch-leak.test.ts` at line 847, Remove the explicit
per-test timeout from the fetch leak test so it follows the repo policy and
Bun’s default timeout handling. Update the test definition around the fetch-leak
case in fetch-leak.test.ts to no longer pass 120_000, and keep the existing test
logic unchanged.
Source: Coding guidelines
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Regression (https://github.com/oven-sh/bun/issues/32659): aborting an | ||
| // in-flight streaming fetch() left the already-received-but-unconsumed response | ||
| // body in the ByteStream's off-heap buffer. reader.cancel() frees it via | ||
| // on_cancel, but the abort path only errored the stream and never released the | ||
| // bytes, so a still-referenced response stream retained the body until GC | ||
| // finalization (which never runs while the stream is reachable) and RSS grew | ||
| // without bound. The client runs in a child process so only its own retention | ||
| // is measured; the stalling server keeps every response mid-stream at abort. |
There was a problem hiding this comment.
🟡 Per CLAUDE.md (line 241: "Regression tests get exactly one comment: the issue URL" and rule 13: "Keep code comments to 3 lines max", added in f91c27b right before this PR), this 8-line header — and the 5-line header in fetch-abort-stream-leak-fixture.js — should be trimmed. The bug-history narration is already in the PR description; collapsing this to // https://github.com/oven-sh/bun/issues/32659 would suffice.
Extended reasoning...
What
CLAUDE.md establishes two relevant conventions for this PR:
- Line 241: "no bug history — that belongs in the PR description. Regression tests get exactly one comment: the issue URL."
- Rule 13 (line 270): "Keep code comments to 3 lines max." This rule was added in commit f91c27b (PR CLAUDE.md: limit code comments to 3 lines #32658), the commit immediately preceding this PR on
main, so this PR is the first one it applies to.
This PR adds two comments that exceed those limits:
test/js/web/fetch/fetch-leak.test.ts:797-804— an 8-line header above the new regression test. It includes the issue URL but adds 7 lines narrating the cause ("reader.cancel() frees it via on_cancel, but the abort path only errored the stream…"), the symptom, and the test-design rationale.test/js/web/fetch/fetch-abort-stream-leak-fixture.js:1-5— a 5-line file header that re-explains what the fixture exercises and why responses are pinned.
The 3-line doc comment on release_buffer() in ByteStream.rs is exactly 3 lines and compliant.
Why this is worth flagging
The 8-line test header is essentially the PR description re-stated inline — exactly the "bug history" that line 241 says "belongs in the PR description." The PR description already covers cause/fix/verification in detail, so the inline copy is redundant and will drift the next time this code path changes.
There is a counter-argument: the surrounding tests in fetch-leak.test.ts all carry similarly long explanatory headers (e.g., the FetchTasklet/HiveRef tests have 8–12-line comments), so the new code matches local file style. However, those comments predate rule 13; this PR is the first one written after the rule landed, and the regression-test-comment convention (line 241) predates this PR entirely.
Step-by-step
git logshows f91c27b ("CLAUDE.md: limit code comments to 3 lines") landed onmainimmediately before 54c99af (this PR's commit).- CLAUDE.md line 270 reads verbatim: "Keep code comments to 3 lines max."
- CLAUDE.md line 241 reads verbatim: "Regression tests get exactly one comment: the issue URL."
fetch-leak.test.ts:797-804is 8 contiguous//lines → violates both rules.fetch-abort-stream-leak-fixture.js:1-5is 5 contiguous//lines → violates rule 13.
Impact
None functional — pure style/documentation. Hence nit.
Fix
Collapse the test header to:
// https://github.com/oven-sh/bun/issues/32659and trim the fixture header to ≤3 lines (or drop it — the test name + issue link already say what's being exercised; the "keep every response + reader alive" rationale is also stated at the held declaration on line 12).
| /// Release the queued-but-unread body bytes. An aborted/errored/cancelled | ||
| /// stream never delivers them, so free the capacity now instead of holding | ||
| /// it off-heap until GC finalization. | ||
| pub(crate) fn release_buffer(&self) { | ||
| if self.buffer.get().capacity() > 0 { | ||
| self.buffer.with_mut(|b| { | ||
| b.clear(); | ||
| b.shrink_to_fit(); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 nit: release_buffer() is extracted as a helper but only called from the one new site — the byte-identical inline if capacity > 0 { clear(); shrink_to_fit(); } pattern remains right below in on_cancel() (lines 479-484) and again in finalize(), plus near-identical copies in on_pull(), to_buffered_value(), and the on_data buffer_action error arm. Consider either calling the helper at those parallel sites too, or just inlining the 3 lines at the new site to match the surrounding code — a single-call-site helper defined directly above an inline copy of its own body reads as inconsistent.
Extended reasoning...
What this is. The PR introduces a release_buffer() helper at lines 464-474 whose body is the capacity-guarded clear() + shrink_to_fit() pattern, and calls it from exactly one place: the new error arm in append(). Meanwhile the identical inline pattern still appears at multiple other sites in the same file — most notably on_cancel() at lines 479-484, which is literally the next function after the helper definition, and finalize() at ~519-524 (both byte-identical including the capacity guard). Near-identical unguarded copies (bare clear(); shrink_to_fit(); inside with_mut) also remain in on_pull()'s done branch (~432), the on_data() buffer_action reject path (~199), and to_buffered_value()'s err branch (~599).
Step-by-step. Reading the file top-to-bottom after this diff:
- Line 464:
pub(crate) fn release_buffer(&self)is defined —if self.buffer.get().capacity() > 0 { self.buffer.with_mut(|b| { b.clear(); b.shrink_to_fit(); }); }. - Line 476:
on_cancel()begins, and at lines 479-484 contains exactlyif self.buffer.get().capacity() > 0 { self.buffer.with_mut(|b| { b.clear(); b.shrink_to_fit(); }); }— verbatim the helper's body, five lines below the helper. finalize()repeats it again verbatim.- Three more sites do the unguarded
clear(); shrink_to_fit();variant.
So a reader sees a named helper, then immediately sees its body open-coded in the adjacent function, which raises the question of whether the inline copies were meant to differ.
Why nothing prevents it. This is purely a code-organization concern; nothing in the compiler or tests would flag it. The author chose to extract a helper rather than inline the 3-line fix at the new site — which signals intent to deduplicate — but the diff stops short of applying it to the existing copies. CLAUDE.md:240 says "the second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site"; the same paragraph also cautions against riding file-wide standardization on a focused bugfix, and the other sites are pre-existing rather than new in this diff. So the rule's letter doesn't strictly apply, but the resulting state — a helper with one call site sitting right next to inline copies of itself — is the awkward middle ground.
Impact. Zero runtime impact. All sites do the same thing whether inlined or via the helper; this is a readability/maintenance nit only and should not block the PR.
How to fix. Two equally valid options:
- Inline at the new site (smallest diff): drop the
release_buffer()helper and write the 3 lines directly inappend()'s error arm, matching the style ofon_cancel/finalize/to_buffered_valuewhich the PR description already cites as the precedent. - Use the helper everywhere (cleaner end state): replace the inline copies in
on_cancel()andfinalize()(exact matches) withself.release_buffer(), and optionally the unguarded sites inon_pull(),on_data()'s buffer_action reject path, andto_buffered_value()as well (the capacity guard is a no-op there sinceshrink_to_fiton an empty Vec is already a no-op).
Either is fine; the current half-way state is the only thing worth flagging.
| ITERATIONS: "50", | ||
| MAX_GROWTH_MB: "250", | ||
| // Measure real retention, not ASan's freed-memory quarantine. | ||
| ASAN_OPTIONS: "quarantine_size_mb=0:thread_local_quarantine_size_kb=0", |
There was a problem hiding this comment.
🟡 This overwrites bunEnv.ASAN_OPTIONS (which harness.ts sets to allow_user_segv_handler=1:disable_coredump=0 under ASAN) instead of appending to it. The other two ASAN_OPTIONS overrides in this same file preserve the harness value via [bunEnv.ASAN_OPTIONS, "quarantine_size_mb=0"].filter(Boolean).join(":") — this one should follow the same pattern.
Extended reasoning...
What the issue is
test/harness.ts:87 sets bunEnv.ASAN_OPTIONS ??= "allow_user_segv_handler=1:disable_coredump=0" when running under bun-asan, and CI (.buildkite/ci.mjs) further sets it to include detect_leaks=0. The new test spreads ...bunEnv and then overwrites ASAN_OPTIONS with the literal string "quarantine_size_mb=0:thread_local_quarantine_size_kb=0", dropping the harness-supplied options entirely.
Why it matters
allow_user_segv_handler=1 lets Bun/JSC install its own SIGSEGV handler under ASAN. ZigGlobalObject.cpp checks ASAN_OPTIONS for this flag to decide whether to enable the WASM fault signal handler, and disable_coredump=0 / detect_leaks=0 are CI-tuned defaults. Losing them means the child process runs with a different ASAN configuration than every other test in the suite. In practice this fixture (a simple fetch loop with no WASM) is unlikely to fault, so this is more of a consistency/maintenance concern than a hard failure — but it diverges from the convention established right here in this file.
Established pattern in this file
Two existing tests in fetch-leak.test.ts already do this correctly:
- The
fetch({proxy})leak test:ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "quarantine_size_mb=0"].filter(Boolean).join(":"),
- The
fetch(file://...)leak test uses the identical pattern.
And test/js/node/tls/node-tls-connect.test.ts:557 carries an explicit comment: "Preserve the harness ASAN options (bunEnv sets allow_user_segv_handler / disable_coredump)".
Step-by-step proof
- Run under
bun-asan.isASANis true, soharness.ts:87executesbunEnv.ASAN_OPTIONS ??= "allow_user_segv_handler=1:disable_coredump=0". - The new test builds
env: { ...bunEnv, ..., ASAN_OPTIONS: "quarantine_size_mb=0:thread_local_quarantine_size_kb=0" }. - Object spread is left-to-right, so the literal
ASAN_OPTIONSkey wins over the spread one. - The spawned child sees
ASAN_OPTIONS=quarantine_size_mb=0:thread_local_quarantine_size_kb=0only —allow_user_segv_handler=1anddisable_coredump=0are gone. - Compare to the proxy/file:// tests in the same file, whose children see
allow_user_segv_handler=1:disable_coredump=0:quarantine_size_mb=0.
Fix
ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "quarantine_size_mb=0:thread_local_quarantine_size_kb=0"]
.filter(Boolean)
.join(":"),One-line change, matches the file's own convention.
|
After deeper investigation I don't recommend merging this as-is. Summary of what I found, for whoever picks this up: The reported repro is already fixed on current A narrower abort-vs-cancel asymmetry remains, but only when the response stays referenced. If the Root cause: the fetch Why the committed change does not fix it: it frees the buffer in A complete fix needs the response body (or the Happy to implement once there's agreement on the shape. Leaving this open rather than shipping a risky core-lifecycle change for an edge case when the reported issue is already fixed. |
1 participant
Fixes #32659.
Aborting an in-flight streaming
fetch()viaAbortControllerretained the already-received-but-unconsumed response body in off-heap memory for as long as the response stream stayed referenced.reader.cancel()did not leak. RSS grew proportionally to the un-consumed body on every abort, which OOM-killed long-running proxies that abort their upstream fetch on client disconnect.Cause
The response body streams into a native
ByteStreamwhosebuffer: Vec<u8>holds received-but-unread bytes. That buffer is released byByteStream::on_cancel(thereader.cancel()path), byfinalize(GC), and byto_buffered_value(the.text()/.arrayBuffer()path). The abort path, however, reaches the stream throughByteStream::on_data(StreamResult::Err(..))(the HTTP thread's terminal failure callback errors the stream), andappend's error arm only stored the error:It never touched
self.buffer. So the un-consumed bytes were reclaimed only by GC finalization, which never runs while the reader/stream is still reachable. The leak is off-heap (invisible toheapStats()) andBun.gc(true)cannot reclaim it.Fix
Release the buffered bytes in that error arm, consistent with
on_cancel/finalize/to_buffered_value. An errored ReadableStream discards its queued chunks per spec, so the reader observes the error rather than the buffered bytes.Verification
New regression test in
test/js/web/fetch/fetch-leak.test.ts: a child process aborts 50 in-flight streaming fetches (~8 MB each, served by a stalling server so every abort lands mid-stream) while keeping each response + reader referenced so GC cannot mask the leak, then asserts RSS growth stays under 250 MB.Minimal repro from the issue (serial, un-pinned) is already flat on current main; pinning the stream exposes the retained buffer
test/js/web/fetch/fetch.stream.test.ts(110),test/js/web/streams/streams.test.js(72), and the fetch abort tests pass unchanged.