Skip to content

test: tolerate one conservative-GC survivor in the aborted body stream check - #42190

Open
robobun wants to merge 3 commits into
mainfrom
robobun/d89d4802/serve-abort-stream-asan-survivor
Open

robobun wants to merge 3 commits into
mainfrom
robobun/d89d4802/serve-abort-stream-asan-survivor

Conversation

@robobun

@robobun robobun commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • test/js/bun/http/serve-pending-promise-abort-leak.test.ts, both releases the body stream it held cases, fail on the debian 13 x64-asan lane: expect(alive).toBe(0), Received: 1. Builds 113711, 113722, 113732, 113727 (3 of 3 attempts), and once on alpine x64 release (110257).
  • It is not a leak. The survivor is always the last stream. A heap snapshot shows its cell with no incoming edge and no roots entry. At conservative root gathering, its address is on the main thread stack once: rbp-0xa60 in the live JSC::runInternalMicrotask frame under the test's own await continuation.

Fix

  • Loop while more than one stream is alive and assert alive <= 1, on every build (a release lane hit it too). The leak that Bun.serve: release the body stream of a Response whose client aborted mid-stream #41080 fixed kept every stream: with release_body_stream disabled in finalize_without_deinit, the test reports Received: 8.
  • Verified: release-asan build, whole file, 3 runs each: 25 pass 2 fail before, 27 pass after. bun bd test: 27 pass.

Background

  • JSC scans the machine stack conservatively: a stack word that matches a cell address keeps that cell. sanitizeStack zeroes only dead stack below the stack pointer, so a stale value in a live caller frame survives each Bun.gc(true) from under it.
  • An await continuation is a microtask and runs inside runInternalMicrotask. One drain earlier, the pump for the last response ran at that frame position and wrote the stream there.
  • ASAN enlarges frames and stops slot reuse, so there the next call never overwrites that slot. Other builds usually do.
Notes

Main does not run the x64-asan test lane, so the failure only shows on PR builds.

Repro: bun run build:asan, then ./build/release-asan/bun-asan test test/js/bun/http/serve-pending-promise-abort-leak.test.ts. The whole file fails 3 of 3 (25 pass, 2 fail). -t "releases the body stream" alone passes. FOO=1 in the environment makes the whole file pass. The microtask that pumped the last response is what left the address in the frame: the stale code addresses around the slot are that response's teardown (see below).

Heap snapshot, taken with generateHeapSnapshotForDebugging() from bun:jsc after the 20 GC rounds, with each stream tagged by a property at creation so no deref() is needed to find it:

[diag] kind=sync alive indices: 7
[diag] ReadableStream-class nodes in snapshot: 3; total nodes 15182; total roots 446
[diag] index 7: ReadableStream#450 cell=0x7bb7b1d10700 roots=[] incoming=[]
[diag] cell 0x7bb7b1d10700: 1 hit(s) in [stack]: 0x7ffd3e0c91d0 (top-0xae30)

The async case is the same (index 7, roots=[] incoming=[], one stack hit at the same stack address).

Stack word, from gdb as the parent (startup-with-shell off, LINES/COLUMNS unset, suspend signals passed through), with a breakpoint on JSC::MachineThreads::gatherConservativeRoots armed by the test right before one more Bun.gc(true) from the same continuation. Frame-pointer walk of the main thread at that point, newest first (GC frames 0 to 16 elided):

#17 JSC__VM__runGC + 324
#18 bun_runtime::hw_exports::bindgen_bunobject_dispatch_gc + 234
#19 bindgen_BunObject_jsGc + 696
#20 0x7283b20141b8 (JIT)
#21 op_call_ignore_result_return_location
#22 llint_call_javascript + 6
#23 JSC::asyncFunctionGeneratorBodyCall(...)            rbp=0x7ffcc6f06db0
#24 JSC::runInternalMicrotask(...)                      rbp=0x7ffcc6f07cd0
#25 JSC::MicrotaskQueue::drainWithUseCallOnEachMicrotask(...) + 1500
#26 JSC::VM::drainMicrotasks() + 2166
#27 Bun::jsFunctionDrainMicrotaskQueue(...) + 42
#28-#30 JS (processTicksAndRejections), llint_call_javascript
#31-#34 JSC::Interpreter::executeCall / JSC::call
#35 Bun::JSNextTickQueue::drain(...) + 1272
#36 Zig::GlobalObject::drainMicrotasks() + 853
#37 bun_jsc::event_loop::EventLoop::drain_microtasks_with_global
#38 bun_jsc::event_loop::EventLoop::exit
#39 bun_runtime::timer::...::TimerObjectInternals::fire      (the Bun.sleep(1) timer)
#40 __bun_fire_timer
#41 bun_runtime::timer::All::drain_timers
#42 bun_runtime::jsc_hooks::auto_tick
...
victim 0x7283f3d10700: 1 aligned hit(s) in live stack [rsp=0x7ffcc6f00dd0, top=0x7ffcc6f13000): 0x7ffcc6f07270
  hit 0x7ffcc6f07270 lies in frame #24 (JSC::runInternalMicrotask): rbp=0x7ffcc6f07cd0, slot = rbp-0xa60

Stale code addresses next to that slot (symbolized): WebCore::JSReadableHTTPResponseSinkController__endImpl, RequestContext::end_request_streaming, uWS::HttpContext<false>::onClose, us_internal_socket_close_raw, a return address inside JSC::runInternalMicrotask, and __interceptor_free / __sanitizer::BufferedStackTrace::UnwindImpl. That is the teardown of the last aborted response.

#41409 adds a retainer report to this test and lists the file as flaky because the cause was not known then. This is the answer that report was meant to find. #40539 rewrites the file for speed and does not touch this assertion's meaning.


[auto-merge] gate passed · iteration 0 · 1 files touched

passes on PR (with fix)
Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/js/bun/http/serve-pending-promise-abort-leak.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/http/serve-pending-promise-abort-leak.test.ts
bun test v1.4.3 (5f554969b)

test/js/bun/http/serve-pending-promise-abort-leak.test.ts:
(pass) RequestContext is freed when client aborts before Promise<Response> settles (http2: false) [2636.71ms]
(pass) RequestContext is freed when client aborts before Promise<Response> settles (http2: true) [2174.72ms]
(pass) Promise<Response> still works normally when not aborted [34.15ms]
(pass) resolve() inside abort handler is handled safely [33.72ms]
(pass) streaming 413 detaches the response so a late resolve/reject is a no-op [7332.69ms]
(pass) chunked request body consumed as a ReadableStream is capped at maxRequestBodySize [481.38ms]
(pass) client abort frees the context even while the resolve function stays reachable [42.78ms]
(pass) client abort while a direct stream pull() is parked frees the context and rejects a pending req.text() read [51.95ms]
(pass) client abort while a direct stream pull() is parked frees the context and rejects a pending for await (req.body) read [39.60ms]
(pass) client abort while a direct stream pull() is parked frees the context and rejects a pending req.textStream() read [39.15ms]
(pass) pendingRequests drops when the client aborts a parked direct-stream pull(), and the late pull() settle is a no-op [109.16ms]
(pass) client abort of a streaming Response releases the body stream it held (sync handler) [123.05ms]
(pass) client abort of a streaming Response releases the body stream it held (async handler) [100.93ms]
(pass) releasing a parked pull() after the abort tore down the context and the server is a no-op (stop-then-abort) [349.74ms]
(pass) releasing a parked pull() after the abort tore down the context and the server is a no-op (abort-then-stop) [338.88ms]
(pass) async server.upgrade() frees the context while the handler promise stays parked [68.48ms]
(pass) 413 on a chunked upload frees th
... (truncated)
Exit: 0
diff hotspot
test/js/bun/http/serve-pending-promise-abort-leak.test.ts | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

gate history · 3 passed · 0 rejected · iteration 0

evidence per changed file
file                                                      reads  edits  tests
…st/js/bun/http/serve-pending-promise-abort-leak.test.ts      5      4     14

…m check

The "client abort of a streaming Response releases the body stream it
held" cases assert that all 8 streams are collected. On the x64 ASAN
lane one stream, always the last, survives: its address is left in the
live JSC::runInternalMicrotask frame that runs the test's own await
continuation, and the conservative stack scan keeps it. A heap snapshot
shows the cell with no incoming edge and no root.

The leak this test guards (#41080) kept every stream, so assert
alive <= 1 instead of 0. With the release in finalize_without_deinit
disabled the test still fails with 8 of 8 alive.
@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: ec31e067-03e7-4b53-af71-490cee2dcf0b

📥 Commits

Reviewing files that changed from the base of the PR and between 4ff9193 and b79a636.

📒 Files selected for processing (1)
  • test/js/bun/http/serve-pending-promise-abort-leak.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


Walkthrough

The streaming-response garbage-collection test now permits one surviving stream reference after repeated collection attempts. It still fails when more than one stream remains.

Changes

Streaming response cleanup

Layer / File(s) Summary
Stream cleanup assertion
test/js/bun/http/serve-pending-promise-abort-leak.test.ts
The cleanup loop retries while more than one stream remains and asserts that at most one stream survives.

Suggested reviewers: jarred-sumner

Merge Risk: ⚪ Minimal · up to b79a6

This test-only adjustment accommodates one conservatively retained stream without masking the actual leak; the affected tests pass and no production behavior changes.

🚥 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 and concisely describes the test change: allowing one conservative-GC survivor in the aborted body-stream check.
Description check ✅ Passed The description explains the problem, fix, technical cause, and verification results. It does not use the template headings exactly, but it includes the required change summary and verification inform…

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

@robobun

robobun commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:21 AM PT - Sep 10th, 2026

✅ @robobun, your commit 2690c08dca893eef83a55495ea33de0d2de17257 passed in Build #113789! 🎉


🧪   To try this PR locally:

bunx bun-pr 42190

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

bun-42190 --bun

@robobun

robobun commented Sep 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. Test-only change, head 2690c08.

Reproduced with bun run build:asan and the whole file: ./build/release-asan/bun-asan test test/js/bun/http/serve-pending-promise-abort-leak.test.ts gives 25 pass, 2 fail (Received: 1) on 3 of 3 runs before this change and 27 pass on 3 of 3 runs after it. bun bd test on the file passes (27). With the body stream release in finalize_without_deinit disabled, the changed assertion still fails with Received: 8.

Review: two optional suggestions, both answered and resolved. The tolerance stays unconditional (build 110257 hit it on an alpine x64 release lane), and the conservative-root attribution carries the two proofs CLAUDE.md asks for (heap snapshot with no retainer, gdb frame that owns the stack word), both in the PR body.

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

@robobun

robobun commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed b79a636: the code comment now says the one-survivor tolerance applies on every build, since build 110257 (alpine x64 release) hit the same Received: 1. No code change beyond the comment. The review thread is answered and resolved.

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

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

@robobun

robobun commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed 2690c08: the test comment is now three lines and points here for the proof. Both review threads are answered and resolved. No change to the assertion.

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

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator Author

Independent check on main at 3d40e50. This change still fixes the failure, and the failure now blocks most PRs.

Impact. Of the last 110 failed builds (2026-09-11 07:00 to 2026-09-12 04:00 UTC), 87 carry the error annotation for this file on debian 13 x64-asan. That lane runs on PRs only, so main stays green. The first PR builds that fail have merge base 5f55496 (#42002, which moved the WebKit pin). Builds based on the commit before it (5eba206) pass. No retainer changed there. The frame layout of JSC::runInternalMicrotask moved.

Repro. bun run build:asan, then the whole file with ./build/release-asan/bun-asan test:

  • main: fails 12 of 12 runs (Received: 1, always index 7).
  • main plus this diff: passes 20 of 20 runs (12 plain, 8 with the CI runner environment: BUN_GARBAGE_COLLECTOR_LEVEL=1, BUN_JSC_randomIntegrityAuditRate=1.0, BUN_JSC_validateExceptionChecks=1).
  • this diff with release_body_stream disabled in finalize_without_deinit: fails with Received: 8, so the test still catches the leak from Bun.serve: release the body stream of a Response whose client aborted mid-stream #41080.

Same stack word. The heap snapshot shows the stream cell with no incoming edge and no roots entry. In gdb, at JSC__VM__runGC for one more Bun.gc(true) from the same continuation, the cell address is in the live stack once: sp+0xd30, which is rbp-0xac0 in the JSC::runInternalMicrotask frame (that frame is 3856 bytes on this build). The words next to it are stale frame records of the last response's teardown (JSReadableHTTPResponseSinkController__endImpl, blob::Any::detach).

Two more probes:

  • Writing 0 to that one stack word in gdb, then continuing, lets the next Bun.gc(true) collect the stream. The alive list goes from [7] to []. Without the write it stays [7]. So that word is the only thing that holds the cell.
  • With nine requests and not eight, only index 8 survives (16 of 16 test runs). Indexes 0 to 7 are always collected. Each teardown overwrites the word that the previous teardown left.

@robobun

robobun commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator Author

Fresh numbers from CI, read 2026-09-16 03:30Z. The failure is frequent again.

This change still covers all 26 failures, because each one has exactly one survivor.

@robobun

robobun commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

Rate of this failure on PR builds since #42775

Source: the Buildkite annotations of 219 finished PR builds, 115781 to 116305 (2026-09-15 03:02Z to 2026-09-16 04:41Z). A build counts when one of the two releases the body stream it held cases fails on an attempt. "Red" means the file failed on every retry.

Build commit Builds With this failure Red
does not contain acb2be0 (#42775) 173 19 (11%) 6 (3%)
contains acb2be0 (#42775) 46 34 (74%) 27 (59%)

This branch has not been deployed

No deployments
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.

1 participant