fetch: do not touch JSCells in the Response Weak finalizer during GC sweep#32729
Conversation
…sweep FetchTasklet::on_response_finalize runs from WeakRefOwner::finalize inside WeakBlock::sweep (MutatorState::Sweeping). It called ignore_remaining_response_body, which in turn called ResumableSink::detach_js() to clear the sink wrapper's cached ondrain/oncancel/stream slots. Those generated *SetCachedValue helpers uncheckedDowncast<JSResumableFetchSink>(...) -> JSCell::classInfo(), and any classInfo() call while the mutator is sweeping trips the validateIsNotSweeping assertion in assert builds (and is latent heap corruption otherwise, since the cell's structure may already have been swept). Gate detach_js() and clear_stream_handlers() on a from_finalizer flag. When called from the Weak finalizer only native state is touched; the JS-side detach still happens later from clear_sink() in deinit(), which runs as an event-loop task outside the sweep. Also correct the detach_js() doc comment that claimed it was safe from finalizers.
|
Updated 10:21 PM PT - Jun 25th, 2026
❌ @robobun, your commit 0889b05 has 3 failures in
The baseline build contains instructions not available on Static scan violations
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughFetch response cleanup now distinguishes normal cancel paths from GC-finalizer paths. A new regression test exercises repeated GC sweeps on an unconsumed streaming-upload response and checks that the process completes successfully. ChangesFinalizer-safe fetch response cleanup
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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-response-finalizer-sweep.test.ts`:
- Line 113: Remove the custom per-test timeout from the fetch response finalizer
sweep test; the issue is the explicit timeout on the test wrapper in
fetch-response-finalizer-sweep.test.ts. Update the test definition around the
collectContinuously-driven case to rely on Bun’s default timeout, and if the
test is too slow, reduce the iteration workload inside the test instead of
extending the timeout.
🪄 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: 4dffdd43-37e9-424c-8315-2cacc4cad6d9
📒 Files selected for processing (3)
src/runtime/webcore/ResumableSink.rssrc/runtime/webcore/fetch/FetchTasklet.rstest/js/web/fetch/fetch-response-finalizer-sweep.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix looks correct and well-tested, but this touches GC-finalizer lifecycle code in the fetch path with subtle reasoning about deferred cleanup (skipping detach_js/clear_stream_handlers during sweep and relying on clear_sink() in deinit() to catch up), so it's worth a human pass from someone familiar with the FetchTasklet/ResumableSink ownership model.
Extended reasoning...
Overview
This PR fixes a JSC assertion crash (validateIsNotSweeping) found by the syscall fault-injection fuzzer. FetchTasklet::on_response_finalize runs as a WeakRefOwner::finalize callback inside WeakBlock::sweep, and previously called ignore_remaining_response_body() which in turn invoked ResumableSink::detach_js() (writes cached-value slots → uncheckedDowncast → JSCell::classInfo() + write barrier) and clear_stream_handlers() (reaches ReadableStreamTag__tagged). Touching any JSCell while MutatorState == Sweeping is forbidden.
The fix threads a from_finalizer: bool through ignore_remaining_response_body. The two finalizer call sites in on_response_finalize pass true (skip the cell-touching cleanup); the event-loop call site on_stream_cancelled_callback passes false (unchanged). A doc comment on ResumableSink::detach_js is corrected to drop the prior claim of finalizer safety. A new regression test reproduces the crash under BUN_JSC_collectContinuously=1.
Security risks
None. This is an internal GC-lifecycle correctness fix; no auth, crypto, input parsing, or trust-boundary changes.
Level of scrutiny
High. The diff is small (~30 LOC of logic plus comments and a test), but it changes behavior inside a JSC Weak finalizer in a hot runtime path. The correctness argument depends on non-local invariants:
- Skipping
clear_stream_handlers()is safe in the finalizer path because the guard aton_response_finalize(line 2460) already returns early whenreadable_stream_ref.has()is true, soclear_stream_handlers()would have been a no-op anyway (readable_stream_ref.get()→ None). The PR description acknowledges this is defensive ("guarded today, but one boolean away"). - Skipping
detach_js()relies onclear_sink()indeinit()(line 492, scheduled as an event-loopConcurrentTask) eventually running outside sweep to perform the deferred detach so the sink wrapper's cachedondrainclosure becomes collectible. This holds as long as the tasklet's normal teardown path is reached, which the PR description asserts.
These look correct to me, but the reasoning spans the FetchTasklet refcount/teardown lifecycle and the ResumableSink ownership model — exactly the kind of thing where a maintainer who knows this code (Jarred/Ciro per coderabbit's suggestion) should confirm there's no path where the deferred cleanup is skipped, leaking the reader/stream graph.
Other factors
- The regression test is solid: it was verified to fail (assertion abort, exit 134) without the fix and pass with it;
fetch-backpressure.test.ts(thefrom_finalizer=falsepath) passes unchanged. - The CodeRabbit timeout nit was reasonably rebutted with precedent and resolved.
- The only finding from the bug-hunting pass is a pre-existing inverted comment label (
Scenario 2bvs2a) on a line adjacent to the change — cosmetic only. - All three call sites of
ignore_remaining_response_bodyare accounted for in the diff.
|
CI status on #64829: every test lane that has completed is green, including all 20 The one hard failure is The remaining annotation is the Ready for review; needs a maintainer to either retry the musl verify-baseline job or confirm it's a known infra issue. |
2 participants
Crash
Backtrace (from a release-asan build with asserts):
Found by the syscall fault-injection fuzzer's client-side grammar scenario (fetch/node:http with abort + transient errno on the client socket). Reproduces ~4/5 under
BUN_JSC_collectContinuously=1.Cause
FetchTasklet::on_response_finalizeis theWeakRefOwner<FetchResponse>::finalizecallback and runs insideWeakBlock::sweepwhileMutatorState == Sweeping. When the response body isLockedwithout a pending promise or stream it callsignore_remaining_response_body(), which called:ResumableSink::detach_js(): writes the sink wrapper's cachedondrain/oncancel/streamslots via the generatedResumableFetchSinkPrototype__*SetCachedValuehelpers. Each doesuncheckedDowncast<JSResumableFetchSink>(thisValue), which reachesJSCell::classInfo()and then issues a write barrier on the wrapper cell.clear_stream_handlers(): reachesReadableStreamTag__tagged->object->inherits<JSReadableStream>()(guarded today, but one boolean away).Calling
classInfo()on any cell while the mutator is sweeping is forbidden: the cell'sStructuremay already have been swept. Assert builds catch it; release builds corrupt the heap.Fix
Thread a
from_finalizerflag throughignore_remaining_response_body. Whentrue(theon_response_finalizecaller) skipdetach_js()andclear_stream_handlers(); only native state is touched. The sink's JS-side detach still happens fromclear_sink()inFetchTasklet::deinit(), which runs as an event-loopConcurrentTaskoutside any sweep, so nothing leaks.The
on_stream_cancelled_callbackcaller (reader.cancel(), runs from JS on the event loop) passesfalseand keeps the immediate detach.Also corrects the
ResumableSink::detach_jsdoc comment that claimed finalizer safety.Verification
New test at
test/js/web/fetch/fetch-response-finalizer-sweep.test.ts: a child process underBUN_JSC_collectContinuously=1does 12 iterations offetch()with a user-constructedReadableStreambody (so the sink takes the JS route with a Strongjs_this) against a raw TCP server that sends headers + a partial chunked body and never terminates it, then drops theResponseunconsumed and runsBun.gc(true).Without the fix (
bun bd, src/ stashed):With the fix:
stdout: "ok",exitCode: 0.test/js/web/fetch/fetch-backpressure.test.ts(exercises theon_stream_cancelled_callbackpath) passes unchanged.