Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC - #35849
Merged
Merged
Conversation
…imeout at wrapper GC Every setTimeout/setInterval/setImmediate previously took a JSC strong handle (HandleSet node) on its Timeout/Immediate wrapper so the cached callback and arguments survived until fire. JSC's "Sh" strong-handle marking constraint walks the whole HandleSet on every collection including eden, so N armed timers added O(N) work to every eden GC: 1e6 idle armed timers drove eden pauses from ~0.3 ms to ~18 ms and heapStats().protectedObjectCount from 3 to 1000003. Replace the per-timer Strong with a slot in a per-VM segmented root table. A new JSTimerRootSegment JSCell holds 4096 WriteBarrier<Unknown> slots; segments are rooted via one Strong each (~N/4096 handles total). A barriered slot store dirties only that segment, so eden collections skip segments untouched since the last full GC. TimerObjectInternals keeps this_value as a weak JsRef for retrieval and records the slot in root_slot; arm/disarm replace the previous set_strong/downgrade sites. AbortSignal.timeout() also held an extra ref on the native AbortSignal so it outlived its JS wrapper until the timer fired, forming a cycle: the signal owns m_timeout and only ~AbortSignal() cancels it, so a dropped listener-less signal leaked its native timer and ~440 B of AbortSignal until the deadline. Drop the extra ref and its compensating derefs; the JS wrapper (kept alive via isReachableFromOpaqueRoots while an abort listener is registered) is the sole owner. Collecting the wrapper runs ~AbortSignal() -> cancelTimer() and frees the native timer. signalAbort() takes a Ref protectedThis to stay valid while dependent-signal abort steps re-enter JS.
Contributor
WalkthroughChangesThe PR adds segmented GC-root storage for timer wrappers, integrates it across timer lifecycle operations, and updates Timer rooting and AbortSignal lifetime
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
Author
Collaborator
|
Can't the segments be a linked list. And then we keep 4096 of slack and a bitset. And we only need the head to be rooted on the JSGobalObject |
Collaborator
Author
|
Yes, that's cleaner. Reworking to:
Pushing shortly. |
Collaborator
|
No we can use a WTF::BitSet with size 4096. And then we mark which ones are used within it that way. And when all go unused we add it to a separate GC-visited free list on the global object. If that free list already exists and is not null, then instead of setting to next, we clear it and it eventually GC's |
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
Collaborator
Author
|
Got it. Reworking to:
Will also add "Helps with #27365" to the PR body (that report has a separate module-registry component, so not claiming a full fix). |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
…pties Per review: instead of one Strong per segment, segments now form a singly-linked list via WriteBarrier<JSTimerRootSegment> m_next with the head in ZigGlobalObject::m_timerRootSegmentHead (visited by GlobalObject::visitChildren), so no HandleSet strong handle is held for any segment. Each segment tracks occupancy in a WTF::BitSet<4096>. When a segment's last slot is cleared it is unlinked and parked in m_timerRootSegmentFree (one spare segment of slack); if that slot is already taken the segment is left unreachable and GC reclaims it. Bun__TimerRootSegment__acquire reuses the spare before allocating. TimerObjectInternals now stores the (segment cell pointer, in-segment index) pair in root_slot: Cell<RootSlot>, so reclaiming a segment cannot invalidate any live handle. The Rust-side RootTable is reduced to an allocation cursor plus a teardown flag.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Comment on lines
152
to
156
| fn drop(&mut self) { | ||
| let ctx = core::ptr::from_mut(self).cast::<c_void>(); | ||
| self.signal.clean_native_bindings(ctx); | ||
| // Suppresses `eventListenersDidChange`'s timeout-signal deref; `cancel_all_timeout_objects` may already own it. | ||
| self.signal.pending_activity_unref(); | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Nit: c9abf29 swept the three HandleSet/HandleSlot references in JSRef.rs (per earlier review round), but a repo-wide grep -rn 'HandleSlot\|HandleSet' src/**/*.rs shows five more sibling comments describing bun_jsc::Strong's backing that this PR replaced: Response.rs:913 (this file, in the diff), FetchTasklet.rs:399-401/512, dispatch.rs:1182, server/mod.rs:4057-4075, and VirtualMachine.rs:1594. The substantive JS-thread-only claims remain true; only the parenthetical mechanism is stale (the WeakSet half of the FetchTasklet/VirtualMachine comments is still accurate — this PR doesn't touch Weak handles). Separately, the ServerAllConnectionsClosedTask impl Drop at server/mod.rs:4054-4068 is now redundant — Impl::destroy in Strong.rs (added by this PR) performs the identical is_shutting_down() check internally and skips Bun__StrongRef__delete, so JSPromiseStrong::Drop is safe past shutdown regardless. Per REVIEW.md "Fix the whole class in the same PR" and "Delete dead code in the same PR that makes it dead — guards a new validator makes redundant".
Extended reasoning...
What the finding is
Commit c9abf29 swept the three HandleSet/HandleSlot references in src/jsc/JSRef.rs to say "StrongRootBlock slot" / "block slot", addressing the earlier review round. But grep -rn 'HandleSlot\|HandleSet' src/**/*.rs shows five more sibling comments describing bun_jsc::Strong's backing storage — which this PR replaced with StrongRootBlock — that were not swept:
src/runtime/webcore/Response.rs:913— "JsRef— assignment drops theStrongarm (HandleSlot freed)".JsRef::Strongwrapsbun_jsc::strong::Optional. This file is in the PR diff (the removed comment at line ~155).src/runtime/webcore/fetch/FetchTasklet.rs:399-401,512— "reach into the VM's HandleSet from this (HTTP) thread" / "the JSCStrong/Weakfields touch the VM's HandleSet/WeakSet". TheStrongfields arebun_jsc::Strong; the JS-thread-only claim remains true (the block bitset/count are not thread-safe), but theHandleSethalf of the parenthetical is stale. TheWeakSethalf is still accurate — this PR does not change Weak handles.src/runtime/dispatch.rs:1182— "destroy()resetsJSPromiseStrong(touches the JSC HandleSet)".JSPromiseStrong=js_promise::Strongwrapscrate::strong::Optional(JSPromise.rs:6,70-72).src/runtime/server/mod.rs:4057,4060,4075— threeHandleSetmentions describing whyServerAllConnectionsClosedTask::DropManuallyDrops itsJSPromiseStrongat shutdown.src/jsc/VirtualMachine.rs:1594— "JSCStrong/Weakhandles against a live HandleSet" (same caveat:Weakhalf still accurate).
The unrelated hit at h2/connection.rs:649 (HandleSettingsFrame, an nghttp2 function name) is correctly excluded.
Why this is in scope
This is exactly the same class as the two prior review rounds on this PR: comment #19 (Strong.rs, fixed in 8043b1a) and comment #22 (JSRef.rs, fixed in c9abf29). Per REVIEW.md "Fix the whole class in the same PR — grep for every sibling site sharing the pattern (same-class sites are ONE concern, not scope creep)", a repo-wide grep after the second sweep would have caught these. The recent commit b3ba56c ("Sweep stale AbortSignal self-ref/cycle comments in Response/jsc_hooks/FakeTimers") shows the author is already sweeping stale comments repo-wide for this PR, and Response.rs is in the diff.
The redundant guard at server/mod.rs:4054-4068
The impl Drop for ServerAllConnectionsClosedTask block reads:
impl Drop for ServerAllConnectionsClosedTask {
fn drop(&mut self) {
// The owned `Box` may be reclaimed by `EventLoop::deinit()` *after*
// `~VM` has already torn down the JSC `HandleSet`. `JSPromiseStrong`'s
// own `Drop` would dereference the freed slot ...
if jsc::VirtualMachine::get().is_shutting_down() {
let _ = std::mem::ManuallyDrop::new(std::mem::take(&mut self.promise));
}
}
}But this PR's Impl::destroy in Strong.rs now performs the identical check internally:
match crate::virtual_machine::VirtualMachine::get_or_null() {
Some(vm) => {
if unsafe { (*vm).is_shutting_down() } {
return;
}
}
...
}
unsafe { Bun__StrongRef__delete(this.as_ptr()) };JSPromiseStrong (js_promise::Strong) wraps crate::strong::Optional (JSPromise.rs:6), whose Drop calls Impl::destroy. So dropping a JSPromiseStrong past shutdown is now a no-op regardless of whether the caller ManuallyDrops it — the is_shutting_down() gate in Impl::destroy skips Bun__StrongRef__delete and returns early ("the handle carries no allocation, so skipping the slot release is the whole of teardown"). Per REVIEW.md "Delete dead code in the same PR that makes it dead — guards a new validator makes redundant", this impl Drop block (and its stale HandleSet comment, and the "The custom Drop impl above keeps the late free from UAFing the freed HandleSet" line at 4074-4075) can go.
Step-by-step proof
grep -rn 'HandleSlot\|HandleSet' src/**/*.rsreturns the eight lines listed above (plus the unrelated h2 hit).src/jsc/JSPromise.rs:6readsuse crate::strong::Optional as JscStrong;and line 70-72 definespub struct Strong { strong: JscStrong }— soJSPromiseStrongis backed bybun_jsc::strong::Optional.src/jsc/Strong.rsimpl Drop for OptionalcallsImpl::destroy(r).- This PR's
Impl::destroymatches onVirtualMachine::get_or_null()and returns early whenis_shutting_down()— the exact same predicateServerAllConnectionsClosedTask::dropchecks beforeManuallyDrop. - Therefore, on the shutdown path: with the guard,
self.promiseis leaked intoManuallyDropandBun__StrongRef__deleteis never called; without the guard,self.promise'sDropruns →Optional::drop→Impl::destroy→is_shutting_down()is true → return early,Bun__StrongRef__deleteis never called. Identical behavior.
Impact
None at runtime — comment-only drift plus one now-redundant guard. The substantive claims (JS-thread-only drop, Drop releases the handle) remain true; only the parenthetical mechanism names are stale.
Fix
Sweep the five files' HandleSet/HandleSlot references to "StrongRootBlock slot" / "block slot" (matching Strong.rs/JSRef.rs), keeping the WeakSet half of the FetchTasklet/VirtualMachine comments as-is. Delete the impl Drop for ServerAllConnectionsClosedTask block at server/mod.rs:4054-4068 and update the schedule() doc comment at 4071-4075 accordingly.
Comment on lines
+60
to
73
| extern "C" JSC::EncodedJSValue Bun__StrongRef__get(StrongRefImpl* _Nonnull ref) | ||
| { | ||
| return JSC::JSValue::encode(decodeStrongRefBlock(ref)->read(decodeStrongRefIndex(ref))); | ||
| } | ||
|
|
||
| extern "C" void Bun__StrongRef__set(StrongRefImpl* _Nonnull ref, JSC::JSGlobalObject* globalObject, JSC::EncodedJSValue encodedValue) | ||
| { | ||
| decodeStrongRefBlock(ref)->write(JSC::getVM(globalObject), decodeStrongRefIndex(ref), JSC::JSValue::decode(encodedValue)); | ||
| } | ||
|
|
||
| extern "C" void Bun__StrongRef__clear(StrongRefImpl* _Nonnull ref) | ||
| { | ||
| decodeStrongRefBlock(ref)->clearValue(decodeStrongRefIndex(ref)); | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Nit: Bun__StrongRef__get (StrongRef.cpp:60, StrongRef.h:12) and Bun__StrongRef__clear (StrongRef.cpp:70, StrongRef.h:14) are dead code — commit c9abf29 inlined Impl::get/Impl::clear on the Rust side via slot_ptr() and removed Bun__StrongRef__clear from the Rust extern block, and no C++ site calls either (Bun::StrongRef = std::unique_ptr<StrongRefImpl, StrongRefDeleter> only touches new/delete). StrongRootBlock::clearValue() (StrongRootBlock.h:87) is transitively dead — its only caller is Bun__StrongRef__clear. Per REVIEW.md "Delete dead code in the same PR that makes it dead … public items escape dead-code lints", drop both functions, their header declarations, and clearValue() (and the stale Bun__StrongRef__clear mention in the StrongRootBlock.h:86 comment).
Extended reasoning...
What the finding is
This PR rewrites the Bun__StrongRef__* FFI surface in src/jsc/bindings/StrongRef.cpp and adds Bun__StrongRef__get alongside the rewritten Bun__StrongRef__clear. But commit c9abf29 ("StrongRootBlock: inline get/clear on the Rust side via slot-pointer encoding; hot-path tuning") — a later commit in this same PR — moved both operations into Rust so they read/write the slot pointer directly without an FFI hop, and removed Bun__StrongRef__clear from the Rust extern block. The two C++ definitions were left behind.
Step-by-step proof
- Rust extern block (
src/jsc/Strong.rs:249-253): declares onlyBun__StrongRef__delete,Bun__StrongRef__new, andBun__StrongRef__set. The diff shows- safe fn Bun__StrongRef__clear(this: &Impl);removed.Bun__StrongRef__getwas never declared on the Rust side in this PR. - Rust
Impl::get(Strong.rs:184-189): reads the slot viaunsafe { (*Self::slot_ptr(this)).encode() }— no FFI call. - Rust
Impl::clear(Strong.rs:197-201): writes the slot viaunsafe { (Self::slot_ptr(this) as *mut i64).write(0) }— no FFI call. The diff shows- Bun__StrongRef__clear(Impl::opaque_ref(this.as_ptr()));removed. - Repo-wide grep for
Bun__StrongRef__get|Bun__StrongRef__clear: only hits are the definitions (StrongRef.cpp:60, 70), the declarations (StrongRef.h:12, 14), and one comment mention (StrongRootBlock.h:86). Zero call sites. - C++ consumer (
StrongRef.h:18-25):Bun::StrongRefisstd::unique_ptr<StrongRefImpl, StrongRefDeleter>; the deleter callsBun__StrongRef__deleteandExternTraits<Bun::StrongRef>::convertToExterncalls.release(). Neithergetnorclearis used from C++. - No codegen references: grep of
src/codegen/for either symbol returns nothing. clearValue()(StrongRootBlock.h:87-92): its only caller isBun__StrongRef__clear(StrongRef.cpp:72), so it is transitively dead.
Why existing tooling doesn't catch this
extern "C" functions have external linkage and are exported from the object file, so the C++ compiler cannot see they are unused — this is exactly the case REVIEW.md calls out under "Delete dead code in the same PR that makes it dead": "Public items escape dead-code lints — grep for callers manually." The Rust dead-code lint likewise cannot see across the FFI boundary. Both functions were made dead by a commit inside this PR (c9abf29), so the PR itself is the one that should delete them.
Impact
None at runtime — dead code adds a few hundred bytes of object code and nothing else. This is purely a code-hygiene finding: leaving Bun__StrongRef__get/clear in place invites a future reader to assume they are the FFI entry points (they are declared right next to the live new/set/delete in StrongRef.h), and StrongRootBlock::clearValue() documents an invariant ("keep the slot occupied so a later write() reuses it without touching the bitset") that nothing exercises.
Fix
Delete:
Bun__StrongRef__getdefinition (StrongRef.cpp:60-63) and declaration (StrongRef.h:12).Bun__StrongRef__cleardefinition (StrongRef.cpp:70-73) and declaration (StrongRef.h:14).StrongRootBlock::clearValue()(StrongRootBlock.h:85-92), including the comment on line 86 that referencesBun__StrongRef__clear.
StrongRootBlock::read() (StrongRootBlock.h:69-73) is also only used by the dead Bun__StrongRef__get, so it can go too if you want to be thorough — though it is trivial enough that keeping it as a public accessor is defensible.
hughescr
added a commit
to hughescr/bun
that referenced
this pull request
Jul 31, 2026
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/jsc/bindings/BunDebugger.cpp
hughescr
added a commit
to hughescr/bun
that referenced
this pull request
Jul 31, 2026
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
hughescr
added a commit
to hughescr/bun
that referenced
this pull request
Jul 31, 2026
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/js/internal/debugger.ts
hughescr
added a commit
to hughescr/bun
that referenced
this pull request
Jul 31, 2026
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 1, 2026
…before the wrapper exists (#36574) Fixes a flake of `test/js/web/fetch/fetch.stream.test.ts` on aarch64: ``` TypeError: null is not an object (evaluating 'err.name') ✗ fetch() with streaming > should be able to fail properly when reading from readable stream with timeout -1 ``` Seen on main (build 86266, darwin aarch64) and ~15 unrelated PR branches since build ~85697. Went red on every retry in build 86380. ## Cause `AbortSignal::abort()` with no argument creates its default `DOMException` and stores it in `m_reason`, a `JSValueInWrappedObject` that holds cell values in a `JSC::Weak`: ```cpp if (reason.isUndefined()) reason = toJS(&globalObject, &globalObject, DOMException::create(ExceptionCode::AbortError)); return adoptRef(*new AbortSignal(&context, Aborted::Yes, reason)); // m_reason(reason) -> setWeakly ``` At that point there is no JS wrapper to own it. The caller's `toJSNewlyCreated()` allocates the `JSAbortSignal` wrapper next, and if that allocation triggers a GC the `DOMException` has no root (the `reason` local is out of scope in release builds) so it is collected and the weak clears. From then on `WebCore__AbortSignal__abortReason` returned `jsNull()`, and `fetch()`'s pre-aborted fast path (#33950, `src/runtime/webcore/fetch.rs:1634`) rejected the promise with `null`. A user-supplied `reason` is safe: the binding's `EnsureStillAliveScope argument0` roots it through the wrapper allocation. The window has existed since #33950 landed; it started being hit in CI after #35356 and #35849 shifted GC scheduling. Debug builds do not reproduce it because `-O0` leaves the `reason` local on the stack for the conservative scanner. ## Repro ```console $ BUN_JSC_collectContinuously=1 bun -e ' let n=0; for (let i=0;i<200000;i++) await fetch("http://127.0.0.1:1/",{signal:AbortSignal.abort()}).catch(e => { if (e===null) n++ }); console.log(n)' 79 ``` ## Fix `AbortSignal::abort()` records `CommonAbortReason::UserAbort` for the default case and leaves `m_reason` undefined, the same deferral `AbortController.abort()` and `AbortSignal.timeout()` already use. `jsReason()` materializes (and caches with a write barrier on the wrapper) the `DOMException` on first access, which is always after the wrapper exists. `toJS(CommonAbortReason::UserAbort)` produces the same `DOMException("The operation was aborted.", "AbortError")` the eager path did. The places that read `m_reason` raw now go through `jsReason()` so they see the deferred value: - `throwIfAborted()` (would have thrown `undefined`) - `AbortSignal::any()`'s already-aborted-source branch (would have set the result's reason to `undefined`) - `WebCore__AbortSignal__addListener`'s already-aborted branch - the Rust `abort_reason()` callers in `fetch.rs` / `h2_frame_parser.rs` / `node_fs_watcher.rs` now call the new `js_reason()` wrapper `AbortSignal::abort_reason()` (and its C++ shim `WebCore__AbortSignal__abortReason` / `headers.h` decl) are removed: those were the only callers, and after the deferral the raw-weak read is a footgun. ## Verification New test in `test/js/web/abort/abort-controller-gc-reason.test.ts` spawns a child with `BUN_JSC_collectContinuously=1` and asserts 10k `fetch({signal: AbortSignal.abort()})` rejections, 10k `throwIfAborted()`, and 10k `AbortSignal.any([AbortSignal.abort()]).reason` all observe an `AbortError` `DOMException` with the same identity as `signal.reason`. ``` release main: bad=10 first=fetch rejection null (fail) release fix: bad=0 (pass) ``` Debug+ASAN does not reproduce the fail-before (the local survives on the stack at `-O0`); the release build does. `test/js/web/fetch/fetch.stream.test.ts`, `test/js/web/abort/abort.test.ts`, the `fs.watch` abort tests and `timer-gc-roots.test.ts` all pass; `rust:check-all` passes on all 10 targets. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/fetch/fetch.stream.test.ts test/js/web/abort/abort-controller-gc-reason.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This was referenced Aug 3, 2026
This was referenced Aug 5, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Aug 12, 2026
…bservers (#37666) ### Problem - `AbortSignal.timeout()` never fires if the signal loses its observers before the deadline: add then remove an abort listener, set then clear `onabort`, close the `fs.watch()` it was passed to, let a `Bun.spawn()` child exit, or even add a listener for an unrelated event. Repro in the original below. - Once stuck, `aborted` stays `false`, `reason` stays `undefined`, `throwIfAborted()` never throws, and listeners or `AbortSignal.any([s])` dependents added later never fire. Node and the browsers report `aborted: true` with a `TimeoutError` in every case. - Cause: the native timer was cancelled as a side effect of listener bookkeeping whenever the observer count read zero. No observers does not mean unreachable; the program still holds the signal, and the DOM spec says a timeout signal aborts for as long as it exists. - The cancel dates from #28761, when the timer held a ref on the signal and this was the only way to reclaim an unobserved signal (#28756). #35849 removed that ref, so since then the branch only freed the timer slightly before the next GC would. ### Fix - Delete the eager cancel. The timer is now stopped in exactly two places: when the signal aborts and when the signal is destroyed. The observer count itself is unchanged and still drives GC. - Why it is correct: a churned signal now ends up in the same state as one nobody ever touched (timer armed, zero observers, wrapper the only owner), which already worked. The timer never held the event loop open, so leaving it armed cannot keep a process alive. - The #28756 memory case still holds: removing the last listener leaves the wrapper collectible and GC frees the signal and timer together. That regression test still passes; measured growth was 36.0 MB before and 36.7 MB after. - Verification: new tests for the listener churn variants, `fs.watch()`, `Bun.spawn()`, re-observation after churn, and a Request whose signal wrapper was collected before the deadline. Seven fail on the unfixed build and on bun 1.4.0 and pass with the change; CI is green on every lane. `node:http2` was affected too and was checked by hand only. ### Background - `AbortSignal.timeout(ms)` must abort with a `TimeoutError` once `ms` passes, whether or not anything is listening. Bun backs it with a native timer that does not keep the event loop alive, like Node's unref'd timer. - Timeout observer count: the C++ signal counts abort listeners plus native consumers (spawn, fs.watch, http2, fetch) currently holding it. Its job is GC only: while it is nonzero, the signal's JS wrapper is kept alive so the timeout still has someone to notify. - Ownership: since #35849 the JS wrapper is the sole owner of the C++ signal, and collecting the wrapper runs the C++ destructor, which frees the timer. GC, not listener removal, is what reclaims an unobserved timeout signal. - Native consumers register an observer while they hold the signal and release it when done, so a subprocess exiting or a watcher closing dropped the count to zero through the same path as removing a JS listener. fetch and `node:fs` escaped only because of the order they release things in. - `Request` is the exception: it holds a native ref and re-wraps the signal each time `request.signal` is read, so the wrapper can be collected while the timeout is still observable. That is why the timer has to outlive the wrapper and only stop when the signal itself dies. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/abort/abort.test.ts <!-- robobun:evidence:end --> <details> <summary>Original description</summary> `AbortSignal.timeout()` stops working as soon as the signal's observer count touches zero while the program still holds the signal: ```js const s = AbortSignal.timeout(50); const f = () => {}; s.addEventListener("abort", f); s.removeEventListener("abort", f); await Bun.sleep(150); console.log(s.aborted); // bun 1.4.0: false node 26.3.0: true ``` The same happens after `s.onabort = fn; s.onabort = null`, after a listener registered with `{ signal: controller.signal }` is removed by `controller.abort()`, after `await Bun.spawn({ cmd, signal: s }).exited` (the subprocess releases its native callback when the child exits), and even after `s.addEventListener("unrelated", fn)` with nothing removed at all. Once it has happened, `s.reason` stays `undefined`, `s.throwIfAborted()` never throws, and abort listeners or `AbortSignal.any([s])` dependents attached afterwards never fire. Node prints `true` with a `TimeoutError` reason for every one of these, and so do the browsers. ## Cause `AbortSignal::eventListenersDidChange()` in `src/jsc/bindings/webcore/AbortSignal.cpp` ended with ```cpp if (m_timeout && !aborted() && !hasTimeoutObserver()) cancelTimer(); ``` It runs on every listener add or remove, and `m_timeoutObserverCount` is zero for any timeout signal nobody is listening to, so the native timer was freed as a side effect of ordinary listener bookkeeping. "No observers" is not "unreachable": the program can still read `aborted`/`reason`, call `throwIfAborted()`, or hand the signal to something later, and https://dom.spec.whatwg.org/#dom-abortsignal-timeout requires the signal to abort after the timeout for as long as it exists. Listener presence only matters for GC (that is the "strong reference while it has abort listeners" clause), which `JSAbortSignalOwner::isReachableFromOpaqueRoots` already implements separately. ## Fix Delete that branch. The timer is now stopped in exactly two places: `markAborted()` (the signal aborted, by the timer or otherwise) and `~AbortSignal()` (nothing references the signal any more). A signal that went through listener churn therefore behaves exactly like one that was never touched, which already worked. Cancelling later instead, once the JS wrapper has been finalized, is not safe with the current holders either. `new Request(url, { signal })` keeps only a native ref (`Request.rs:1343`), nothing pins the signal's wrapper until `request.signal` is first read, and the getter re-wraps the C++ signal (`Request.rs:727`), so the wrapper is routinely collected while the timeout is still observable through `request.signal`; that works today and is now covered by a test. Every other native holder (FetchTasklet, Response's BodyAbortListener, Subprocess, the http2 SignalRef, node_fs ReadFile/WriteFile, fs.watch) registers an observer for as long as it holds the signal, which keeps the wrapper alive, and releases the observer and the ref in the same call, so for those the wrapper is only finalized when it holds the last ref and `~AbortSignal()` frees the timer at that moment anyway. Why this is the right fix rather than a narrower one: the cancel dates from #28761, when the timer held a ref on the signal and cancelling on the last listener removal was the only way to free an unobserved `AbortSignal.timeout()` before its deadline (#28756). #35849 removed that self-ref: the JS wrapper is the sole owner, `isReachableFromOpaqueRoots` keeps it alive only while `hasTimeoutObserver()`, and collecting the wrapper runs `~AbortSignal()` which frees the timer. With that in place the branch could only free the timer slightly earlier than the next GC would, and it is observably wrong. The observer counter itself is unchanged; it still drives GC reachability. The timer does not hold the event loop open (it never did; `AbortSignal__Timeout__create` inserts it without a ref, the same as Node's unref'd timer), so keeping it armed cannot keep a process alive. The resulting lifetime is the same as Node's: its timer is cleared when the signal aborts or when the signal is collected, and never because listeners were removed. The state this leaves a churned signal in (timer armed, observer count zero, wrapper the only owner) is already reachable on main: `releaseSourceObserverCounts()` (an `AbortSignal.any()` dependent aborting) and `decrementPendingActivityCount()` (fetch finishing) drop the count without going through `eventListenersDidChange()`, and the existing `timer-gc-roots.test.ts` case "AbortSignal.any([timeout, controller.signal]).abort releases the timeout" checks that GC alone reclaims the signals and their 600 s timers from that state. This change only routes the listener and native-callback paths into the same state. The #28756 scenario still reclaims memory: removing the last listener makes the wrapper collectible, and the next GC frees the signal and its timer together. Running that test's script under the debug build gives 36.0 MB growth without this change and 36.7 MB with it (the number is dominated by ASAN quarantine; 202 signals remain live in both cases, which are the test's 200 warm-up signals that keep a listener), and `test/regression/issue/28756.test.ts` passes. ## Verification New tests in `test/js/web/abort/abort.test.ts` (`AbortSignal.timeout() still fires after its observers go away`): the four listener-churn variants above plus `fs.watch(dir, { signal }).close()` (a native consumer releasing its callback synchronously), re-observation after churn (a listener added afterwards fires, `AbortSignal.any([signal])` aborts, `throwIfAborted()` throws `signal.reason`), and the asynchronous `Bun.spawn()` release when the child exits. A further test constructs Requests around timeout signals, checks with `heapStats()` that the signal wrappers were collected before the deadline, and then reads `request.signal.aborted`; it passes on 1.4.0 as well and pins the Request case described above. Each waits on a second timeout signal armed afterwards with a longer delay, which sits behind the signal under test in the timer heap, so the signal under test stays unobserved and the tests do not depend on wall-clock timing (the spawn case needs the child to exit before a 500 ms deadline; a shell exits in about 1 ms). The seven churn/fs.watch/spawn tests fail on the unfixed debug build (`aborted: false`, `reason: undefined`) and on bun 1.4.0, and pass with the change (repeated runs locally with `--rerun-each`; CI is green on every lane, including Windows and ASAN). Also passing with the change: `test/regression/issue/28756.test.ts`, `test/js/web/timers/timer-gc-roots.test.ts`, `test/js/web/abort/*`, `test/js/web/streams/pipeTo-signal-leak.test.ts`, `pipeTo-shutdown-gc.test.ts`, `test/js/web/fetch/fetch-tls-abortsignal-timeout.test.ts`, the abort-signal tests in `fetch-leak.test.ts`, `test/js/bun/spawn/spawn-signal.test.ts`, `test/js/node/util/test-aborted.test.ts`, the `--isolate` leaked-timeout test in `test/cli/test/isolation.test.ts`, and Node's `test-abortsignal-any.mjs`. Native consumers affected on the unfixed build, from going through each `add_listener`/`listen` site and its release order: `Bun.spawn`/`spawnSync` (`Subprocess::clear_abort_signal` drops pending activity first, then the callback), `fs.watch` (`detach()` removes the callback; closing the watcher left the signal stuck), and `node:http2` `session.request(headers, { signal })` (the stream's `SignalRef` drop; a signal reused after a stream closed never fired). spawn and fs.watch are tested; http2 was verified by hand (stuck on 1.4.0, `TimeoutError` with the fix, same as Node) and goes through the same `cleanNativeBindings()` entry point, but an http2 round trip takes over a second on a debug build, so it did not get a test of its own. A side effect of the same bug was that `spawnSync({ signal })` ignored the deadline of a signal that had lost its observers, since it reads the armed timer's deadline (`js_bun_spawn_bindings.rs`); that now works too. `fetch()`, `Response` and `node:fs` `readFile`/`writeFile` were not affected: they drop their callback before their pending-activity count (or hold only pending activity), so the counter never reached zero inside `eventListenersDidChange()`. With this change the release order of native consumers no longer matters. </details>
robobun
added a commit
that referenced
this pull request
Aug 19, 2026
JSC now stores Strong<> slots in StrongBlocks owned by a per-heap StrongSet (upstream ff64aee116d4, which also deletes HandleSet). Allocate bun_jsc::Strong slots from that set instead of the StrongRootBlock cells added in #35849: Bun__StrongRef__new returns the slot, Rust reads and writes it directly (the set is scanned unconditionally, so stores need no barrier), and Bun__StrongRef__delete returns it. Bun__StrongRef__set goes away. Delete StrongRootBlock, its iso subspace, the per-VM block list on JSVMClientData and the "Srb" marking constraint. heapStats() and getProtectedObjects() go back to JSC's own protected-cell walk, which covers StrongSet slots. The AbortSignal.timeout changes from #35849 are unaffected. Requires a WebKit that contains the StrongSet refactor.
robobun
added a commit
that referenced
this pull request
Aug 19, 2026
JSC now stores Strong<> slots in StrongBlocks owned by a per-heap StrongSet (upstream ff64aee116d4, which also deletes HandleSet). Allocate bun_jsc::Strong slots from that set instead of the StrongRootBlock cells added in #35849: Bun__StrongRef__new returns the slot, Rust reads and writes it directly (the set is scanned unconditionally, so stores need no barrier), and Bun__StrongRef__delete returns it. Bun__StrongRef__set goes away. Delete StrongRootBlock, its iso subspace, the per-VM block list on JSVMClientData and the "Srb" marking constraint. heapStats() and getProtectedObjects() go back to JSC's own protected-cell walk, which covers StrongSet slots. The AbortSignal.timeout changes from #35849 are unaffected. Requires a WebKit that contains the StrongSet refactor.
8 tasks
robobun
added a commit
that referenced
this pull request
Aug 21, 2026
JSC now stores Strong<> slots in StrongBlocks owned by a per-heap StrongSet (upstream ff64aee116d4, which also deletes HandleSet). Allocate bun_jsc::Strong slots from that set instead of the StrongRootBlock cells added in #35849: Bun__StrongRef__new returns the slot, Rust reads and writes it directly (the set is scanned unconditionally, so stores need no barrier), and Bun__StrongRef__delete returns it. Bun__StrongRef__set goes away. Delete StrongRootBlock, its iso subspace, the per-VM block list on JSVMClientData and the "Srb" marking constraint. heapStats() and getProtectedObjects() go back to JSC's own protected-cell walk, which covers StrongSet slots. The AbortSignal.timeout changes from #35849 are unaffected. Requires a WebKit that contains the StrongSet refactor.
robobun
added a commit
that referenced
this pull request
Aug 24, 2026
JSC now stores Strong<> slots in StrongBlocks owned by a per-heap StrongSet (upstream ff64aee116d4, which also deletes HandleSet). Allocate bun_jsc::Strong slots from that set instead of the StrongRootBlock cells added in #35849: Bun__StrongRef__new returns the slot, Rust reads and writes it directly (the set is scanned unconditionally, so stores need no barrier), and Bun__StrongRef__delete returns it. Bun__StrongRef__set goes away. Delete StrongRootBlock, its iso subspace, the per-VM block list on JSVMClientData and the "Srb" marking constraint. heapStats() and getProtectedObjects() go back to JSC's own protected-cell walk, which covers StrongSet slots. The AbortSignal.timeout changes from #35849 are unaffected. Requires a WebKit that contains the StrongSet refactor.
robobun
added a commit
that referenced
this pull request
Aug 26, 2026
JSC now stores Strong<> slots in StrongBlocks owned by a per-heap StrongSet (upstream ff64aee116d4, which also deletes HandleSet). Allocate bun_jsc::Strong slots from that set instead of the StrongRootBlock cells added in #35849: Bun__StrongRef__new returns the slot, Rust reads and writes it directly (the set is scanned unconditionally, so stores need no barrier), and Bun__StrongRef__delete returns it. Bun__StrongRef__set goes away. Delete StrongRootBlock, its iso subspace, the per-VM block list on JSVMClientData and the "Srb" marking constraint. heapStats() and getProtectedObjects() go back to JSC's own protected-cell walk, which covers StrongSet slots. The AbortSignal.timeout changes from #35849 are unaffected. Requires a WebKit that contains the StrongSet refactor.
Jarred-Sumner
added a commit
that referenced
this pull request
Aug 30, 2026
…destructors (#40904) ## Bug Running `test/js/sql` in one debug process intermittently aborts with ``` ASSERTION FAILED: vm().currentThreadIsHoldingAPILock() => vm().heap.mutatorState() != MutatorState::Sweeping JavaScriptCore/runtime/JSCell.cpp(179) : bool JSC::JSCell::validateIsNotSweeping() const ``` Backtrace (debug build, `Bun.gc(true)` after dropping a few thousand resolved MySQL queries): ``` JSC::JSCell::validateIsNotSweeping JSC::JSCell::classInfo JSC::validateCell<Bun::StrongRootBlock*> WriteBarrierBase<Bun::StrongRootBlock>::setMayBeNull Bun::StrongRootBlock::setNext Bun::StrongRootBlock::release Bun__StrongRef__delete <bun_jsc::strong::Optional as Drop>::drop drop_in_place<bun_sql_jsc::shared::cached_structure::CachedStructure> drop_in_place<bun_sql_jsc::mysql::my_sql_statement::MySQLStatement> <MySQLStatement as CellRefCounted>::deref drop_in_place<bun_sql_jsc::mysql::js_my_sql_query::JSMySQLQuery> host_fn_finalize_ref_counted<JSMySQLQuery> MySQLQueryClass__finalize WebCore::JSMySQLQuery::~JSMySQLQuery JSC::MarkedBlock::Handle::specializedSweep<...> JSC::Heap::sweepSynchronously JSC::Heap::collectNow ``` The same path is reachable through `JSMySQLConnection` finalize -> `MySQLConnection.statements` drop, and through `PostgresSQLQuery`/`PostgresSQLConnection` finalize -> `PostgresSQLStatement` drop (the regression test uses the Postgres path against a mock server). ## Root cause `MySQLStatement` / `PostgresSQLStatement` cache the result-row `Structure` in a `bun_jsc::Strong`, and the statement's last ref is dropped from a JS wrapper's finalizer, i.e. from a `JSCell` destructor while JSC is sweeping. #35849 moved `bun_jsc::Strong` from HandleSet nodes to slots in `StrongRootBlock` cells and explicitly intended "a `bun_jsc::Strong` dropped from a sweep-time finalizer" to work, but when a delete empties a block, `StrongRootBlock::release()` unlinked it by walking and rewriting `WriteBarrier<StrongRootBlock> m_next` on *sibling* blocks: a write barrier plus (under `GC_VALIDATION`) `validateCell -> classInfo()`, which JSC does not allow mid-sweep. It only fires when a delete happens to empty a whole 960-slot block inside a destructor, hence intermittent. Severity, stated plainly: the sibling blocks are always live (the "Srb" constraint roots every linked block), so in release builds the stray barrier between two marked cells is benign; what actually fails is the debug/ASAN-assertions `validateIsNotSweeping` check. This is a GC-hygiene fix (do not touch other cells from a destructor), not a release memory-safety bug. ## Fix Make the block list linkage raw pointers instead of `WriteBarrier`s: `m_next` becomes `StrongRootBlock*`, `visitChildren` no longer appends it, and the "Srb" marking constraint walks the list and `appendUnbarriered`s every block (plus the parked spare and the Structure) instead of only the head. `acquire()`/`release()`/`Bun__StrongRef__delete` are now plain loads and stores, so they are safe from a sweep-time destructor, and empty blocks are still released immediately as before (no deferred reclamation state). Why this is sound: the constraint is `GreyedByExecution`, so it re-runs on every return to Fixpoint (world stopped) until marking converges; every block the mutator linked, relinked from the spare slot, or parked is appended by the final execution, and no inter-block edge is needed for liveness any more. Eden behaviour is unchanged per block: an old block reads as marked and is skipped, and slot stores still barrier the block cell so dirtied blocks are re-visited via the remembered set. Cost: the constraint body goes from three appends to one `isMarked` check per block (960 handles each) per Fixpoint execution. Why not hang the Structure off a wrapper cell as a `WriteBarrier<Structure>` instead: a statement is refcounted and shared between the connection's prepared-statement map and every query wrapper that ran it, and it can outlive any one of those wrappers, so there is no single owning cell to visit it from without turning statements into cells. More importantly the allocator promised finalizer-time drops for every `bun_jsc::Strong` user, so that is the layer that was wrong. ## Test `test/js/sql/postgres-statement-structure-gc.fixture.ts` (driven from `test/js/web/timers/timer-gc-roots.test.ts`, the StrongRootBlock test file): a mock Postgres server from `wire-frames.ts` answers 2048 pipelined simple queries; each query's statement caches a Structure; the resolved queries are dropped together and collected, so their finalizers release >2 blocks' worth of Strongs mid-sweep. Asserts the Structures were rooted while held and are gone afterwards, and that the process exits cleanly. On a debug build before this change the fixture aborts with the assertion above (verified); after it prints `{"count":2048,"protectedWhileHeld":2048,"protectedAfter":0}`. A release build does not compile the assertion, so this test only has signal on debug/ASAN-assertion builds. Also adds a `pgMockServer()` helper to `wire-frames.ts` and moves the two existing mock-server leak fixtures onto it. Ran locally on the debug build: `test/js/sql` (Docker mysql_plain / mysql_native_password / mysql_tls / postgres_plain via the compose harness), `test/js/web/timers/timer-gc-roots.test.ts`, `test/js/web/abort/abort.test.ts`, `test/js/valkey/valkey.test.ts` + `unit/`. The only failures are pre-existing on main's debug build and unrelated ("should not timeout in long results" 10 s cap, RSS-threshold fixtures keyed on the executable name, unsymbolized LSAN in valkey-gc).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Back every
bun_jsc::Stronghandle with a slot in aStrongRootBlockJSCell (960WriteBarrier<Unknown>slots per block) instead of one HandleSet node per handle, and drop theAbortSignal.timeout()self-ref so the native signal/timer is freed when the JS wrapper is collected.Why
JSC's
"Sh"strong-handle marking constraint walks the whole HandleSet on every collection including eden. With one HandleSet node per armed timer/fetch/subprocess/etc., N livebun_jsc::Stronghandles add O(N) work to every eden pause.Separately,
AbortSignal::timeout()tooksignal->ref()so the native timer pinned the C++ signal; only~AbortSignal()cancels the timer, so a droppedAbortSignal.timeout(big)leaked both until the deadline.Helps with #27365.
How
StrongRootBlock(src/jsc/bindings/StrongRootBlock.{h,cpp}, new): aJSCellwithcapacity = 960WriteBarrier<Unknown>slots plus aWTF::BitSet<960>occupancy map,unsigned m_occupiedCount, andWriteBarrier<StrongRootBlock> m_next. Active blocks form a singly-linked list; one spare empty block is parked as a free-list head, further empties are unlinked and reclaimed by GC.visitChildrenappendsm_nextplus the 960 slots; a per-slot write fires the barrier on the block cell, so eden only re-visits blocks actually touched since the last full GC.Per-VM storage and rooting (
src/jsc/bindings/BunClientData.{h,cpp}):JSVMClientDatagains rawStrongRootBlock* m_strongRootBlockHead/Free/CursorandStructure* m_strongRootBlockStructure. They are rooted by aSimpleMarkingConstraint("Srb",GreyedByExecution) registered inJSVMClientData::create, so the block list is per-VM rather than per-global (ShadowRealm / node:vm /bun test --isolateall share one list with no transfer step). The constraint body is threeappendUnbarrieredcalls, O(1) per collection; see the comment inBunClientData.cppfor the full eden/full/concurrency reasoning againstMarkingConstraintSet/CollectorPhase.m_strongRootBlockCursorremembers the last block with room (set on acquire and on every delete), soBun__StrongRef__newis oneisFull()compare + onefindFreeSlot()in the common case.bun_jsc::StrongFFI (src/jsc/bindings/StrongRef.cpp,src/jsc/Strong.rs): the opaque handle is(&m_slots[index]) | (index << 48), no heap allocation (relies on the 48-bit VA invariant JSValue NaN-boxing already assumes). Rust'sImpl::get/clearread/write the slot through the low-48-bit pointer with no FFI, matching the old HandleSlot direct-deref fast path;set/deleterecoverblock = slot - index*8 - offsetof(m_slots)in C++.Impl::destroyskips the FFI call onceVirtualMachine.is_shutting_down, which is set before bothZig__GlobalObject__destructOnExitandWebWorker__teardownJSCVMreach their finalcollectNow, so abun_jsc::Strongdropped from a sweep-time finalizer ordeinit_runtime_stateafter~VMnever touches a dead block cell.heapStats()/getProtectedObjects()(src/jsc/modules/BunJSCModule.h): both walkclientData(vm)->m_strongRootBlockHeadand merge occupied slots intoprotectedObjectTypeCounts/protectedObjectCount/ the returned array, so those stay user-visible.AbortSignal.timeoutlifetime (src/jsc/bindings/webcore/AbortSignal.{h,cpp},JSAbortSignalCustom.cpp,src/jsc/AbortSignal.rs,src/runtime/timer/mod.rs): drop thesignal->ref()self-pin; the nativeTimeoutholds a raw back-pointer and~AbortSignal()cancels it.JSAbortSignal::isReachableFromOpaqueRootskeeps the wrapper alive whilehasActiveTimeoutTimer() && hasTimeoutObserver();hasTimeoutObserver()reads a singlestd::atomic<uint32_t> m_timeoutObserverCountbumped by event-listener presence,addAlgorithm/addAbortAlgorithmToSignal, dependent signals, andincrementPendingActivityCount, so the GC-thread read is lock-free. BothsignalAbort()overloads takeRef protectedThisbefore calling into JS.Benchmark (release, Linux x64, canary
9a568e7d6vs this branchc9abf29d3)Eden GC pause with 1M armed timers held (allocation churn to drive eden;
BUN_JSC_logGC=1, last 20 pauses averaged):protectedObjectCountobjectTypeCounts.StrongRootBlockAbortSignal.timeout(600000)dropped, 50k × 20 rounds:AbortSignal.timeoutcreation (bench/snippets/abort-signal.mjs):AbortSignal.timeout(1000)Per-handle throughput (
setTimeout+clearTimeout):bench/snippets/set-timeout.mjs(20M)So: eden visiting is ~8x cheaper,
AbortSignal.timeoutno longer leaks, per-Strongcreate/drop is ~17-30% more expensive (still 3-7M ops/s), RSS neutral.Tests
test/js/web/timers/timer-gc-roots.test.ts(new, 9 tests):heapStats()still reports protected Timeout counts andobjectTypeCounts.StrongRootBlocksetTimeout/setInterval/setImmediatecallbacks stay reachable across GC while armedAbortSignal.timeoutwithout listeners frees its native timer (RSS bounded)AbortSignal.any([timeout, controller.signal])+controller.abort()releases the timeoutany()source / passed asaddEventListener({signal})still fire after GC[review] gate passed · iteration 12 · 25 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 12
evidence per changed file