AbortSignal.any(): keep a dependent's listeners only while a source can abort

[dylan-conway]

What does this PR do?

Fixes a leak: an AbortSignal.any() dependent signal whose abort listener holds its source's controller is never collected, and neither are its sources, the listener, or anything the listener closes over.

const controller = new AbortController();
AbortSignal.any([controller.signal]).addEventListener("abort", () => controller.abort());
// nothing references any of this anymore, and none of it is ever collected

JSAbortSignalOwner::isReachableFromOpaqueRoots kept a dependent signal with an abort listener alive for as long as any of its sources' native objects existed. Here the source exists only because the dependent is alive (dependent → listener → controller → source), so the condition holds forever.

Now the dependent is kept alive only while one of its sources can still abort: its AbortController is reachable (the controller's wrapper adds its signal as an opaque root), its timeout is armed, or native code holds pending activity on it. hasAliveSourceSignals() has no other caller and is removed.

The check runs on GC marker threads. It iterates the source set without pruning it (pruning releases thread-affine weak pointers) and reads the sources; the JS thread is suspended while marking constraints execute (worldShouldBeSuspended(CollectorPhase::Fixpoint)), so no source is freed during the read.

How did you verify your code works?

Four new tests in test/js/web/abort/abort.test.ts:

• the leak: 100 such dependents. On 1.4.2 it leaves 202 AbortSignal and 102 AbortController cells after GC; with this change it passes.
• an unreferenced dependent (nested any([any([signal])])) with a listener still fires after forced GCs when its reachable controller aborts.
• an unreferenced dependent of AbortSignal.timeout() with a listener still fires after forced GCs.
• an unreferenced dependent of a Bun.serve request's signal with a listener still fires after forced GCs when the client disconnects. With the pending-activity clause removed, this one times out.

The last three pass before and after. abort-controller-gc-reason.test.ts (which covers the marker-thread assertion from #32785) passes on the debug build.

[coderabbitai]

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: d44eee06-f70e-4fbc-bad6-3339120f9e8d

📥 Commits

Reviewing files that changed from the base of the PR and between 0ab6fe2 and 8607555.

📒 Files selected for processing (2)

• src/jsc/bindings/webcore/JSAbortSignalCustom.cpp
• test/js/web/abort/abort.test.ts

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

---

Walkthrough

The change removes hasAliveSourceSignals() and updates GC reachability to inspect source signals without mutation. Four regression tests cover collection, abort propagation, timeout propagation, and server-request abort propagation for dependent AbortSignal.any() signals.

Changes

AbortSignal reachability

Layer / File(s)	Summary
Read-only AbortSignal reachability src/jsc/bindings/webcore/AbortSignal.h, src/jsc/bindings/webcore/JSAbortSignalCustom.cpp	Removes the public hasAliveSourceSignals() method. GC reachability now checks source signals for active timeouts, pending activity, and opaque roots without mutation.
AbortSignal.any garbage-collection regressions test/js/web/abort/abort.test.ts	Adds tests for collection of dependent signals and controllers, controller abort propagation, timeout propagation with TimeoutError, and server-request abort propagation with AbortError.

Suggested reviewers: robobun

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 86075

The inspected reachability paths and regression coverage are consistent, with no actionable merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: AbortSignal.any() dependents retain listeners only while a source can still abort.
Description check	✅ Passed	The description includes both required sections. It explains the garbage-collection fix and documents verification with four regression tests plus the debug-build assertion test.

---

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

[claude]

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

Beyond the inline findings, I also checked whether visitor.containsOpaqueRoot(&source) misses a JS-abortable source reachable only through a nested any() chain — it does not: AbortSignal::addSourceSignal flattens dependents, so m_sourceSignals only ever holds leaf signals (controller, timeout, or native-held), each covered by one of the four clauses. The const iteration over the WeakListHashSet also skips dead entries without pruning, so it is no more mutating on the marker thread than the begin() != end() probe it replaces.

Extended reasoning...

Findings are already posted inline, so this body only records what else was examined. The main refactor-polarity risk (old: any alive source roots the dependent; new: only a source that can still abort) hinges on whether the four clauses cover every leaf source. Nested any() cannot produce a dependent-as-source because addSourceSignal in src/jsc/bindings/webcore/AbortSignal.cpp:141-148 recursively adds the nested dependent's sources instead, so a source is always a controller signal (opaque root from JSAbortController), a timeout signal (hasActiveTimeoutTimer), or a native-held signal (hasPendingActivity). The const-range iteration over WeakListHashSet only reads and skips null weak references, matching the thread-affinity constraint the removed hasAliveSourceSignals() comment described. The inline findings (missing native-held test variant, the { signal }-only dependent gap, and the never-true isFiringEventListeners clause) mean a human should still look before merging.

One verified lower-impact observation (a convention, logging or cleanup point) was not posted.

[claude]

🟣 pre-existing, not blocking: pre-existing: a dependent signal used only as the { signal } option of addEventListener is collected while its controller is still reachable, so aborting never removes that listener. JSAbortSignalCustom.cpp:49 runs the new source check only when hasAbortEventListener() is set. { signal } registers an abort algorithm through addAlgorithm (EventTarget.cpp:88) and holds the signal weakly, so the gate is false and line 77 finds no opaque root. Fix: for dependents, gate on anything that observes the abort (listeners, algorithms, abort algorithms, pending activity), e.g. hasTimeoutObserver() which already counts all of these, then run the source loop.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Extended reasoning...

The PR keeps a dependent while a source can abort, but only for dependents that have an abort event listener; the same reachable-controller case fails for a dependent whose only observer is an algorithm, exactly as on the base branch.
User code: const c = new AbortController(); target.addEventListener("ping", handler, { signal: AbortSignal.any([c.signal]) }); with c and target kept, the dependent dropped.
EventTarget.cpp:75-95: the option calls options.signal->addAlgorithm(...) and stores WeakPtr { *options.signal } on the RegisteredEventListener; no Ref, no wrapper root, no abort event listener.
GC: JSAbortSignalCustom.cpp:42 not aborted; line 43 no timer; line 49 hasAbortEventListener() is false (AbortSignal.cpp:289-300 only counts abort listeners and native callbacks), so the loop at line 56 and the pending-activity check at line 66 are skipped; line 77 containsOpaqueRoot(&abortSignal) is false because only JSAbortController roots its own signal (JSAbortController.cpp:234), not a dependent.
The wrapper is the sole owner (AbortSignal.cpp:90-94 returns a Ref held by the…

Verification: pre-existing (same route fails on the base branch; the PR rewrites the dependent branch directly under the gate but leaves the gate) — trigger: a non-aborted AbortSignal.any() dependent whose only observer is an abort algorithm (e.g. passed as { signal } to addEventListener) and whose JS wrapper is otherwise unreferenced, while its source's AbortController stays reachable. Mechanism…

[dylan-conway]

Reproduces on 1.4.2 (the listener still runs after abort()). This PR does not touch that gate and does not change the behaviour, so it is left for its own change.

[robobun]

^{Updated 12:53 PM PT - Sep 17th, 2026}

@dylan-conway, your commit 8607555 is building: #117210

[claude]

I re-reviewed after 0ab6fe2 and found no bugs; the follow-up commit addresses what I raised inline earlier. Because this changes GC keep-alive semantics evaluated on marker threads, a human look is still worthwhile before merging.

What was reviewed:

• Nested any() in the two-level test: addSourceSignal flattens a dependent source into its own sources, so the outer dependent's only source is controller.signal, which containsOpaqueRoot(&source) matches (AbortController::opaqueRoot() is root(&signal()), the same AbortSignal*).
• Native holders that can abort a signal (server request signals in server_body.rs/mod.rs, the WebSocket-upgrade signal released in ServerWebSocket.rs after ConnectionClosed) all take pending_activity_ref() for as long as they can abort, so the hasPendingActivity() clause covers them; the lazily created Request.signal never aborts natively, so it is correctly collectable.
• Marker-thread reads: pendingActivityCount is atomic, hasActiveTimeoutTimer()/const iteration are the same reads the previous hasAliveSourceSignals() and line 43 already performed; nothing new mutates the set.
• I could not build or run the new tests in this environment (no WebKit sources in the checkout), so CI results should be checked for the debug/ASAN lanes.

Extended reasoning...

Overview

The PR narrows JSAbortSignalOwner::isReachableFromOpaqueRoots in src/jsc/bindings/webcore/JSAbortSignalCustom.cpp so a dependent AbortSignal.any() signal with an abort listener is kept alive only while one of its (flattened) source signals can still abort: the source has an armed timeout timer, native pending activity, or is an opaque root (added by JSAbortController::visitChildrenImpl via GenerateAddOpaqueRoot=signal). AbortSignal::hasAliveSourceSignals() is deleted from AbortSignal.h since its only caller is gone. Four tests are added to test/js/web/abort/abort.test.ts: a subprocess heap-count leak regression and three keep-alive behavioral tests (reachable controller through nested any(), AbortSignal.timeout(), and a Bun.serve request signal). The second commit (0ab6fe2) dropped the isFiringEventListeners() clause and added the server-request test, addressing my two earlier inline comments; the third earlier comment was explicitly pre-existing and non-blocking.

Security risks

None: no untrusted input parsing, no auth or crypto, no new allocation. The risk class is GC correctness (premature collection would silently drop an abort listener; over-retention is the leak being fixed), not security.

Level of scrutiny

High, because this is reachability logic evaluated on GC marker threads and the failure mode of getting it wrong (a dependent silently collected while its source can still abort) would not crash but would lose listeners. I traced each clause: AbortSignal::addSourceSignal recurses into a dependent source's own sources, so nested any() chains always bottom out at non-dependent signals, which is why the two-level test does not need a transitive check. AbortController::opaqueRoot() returns root(&signal()), i.e. the AbortSignal* address, matching containsOpaqueRoot(&source). Every Rust site that creates a natively abortable signal (server_body.rs:3018/3298, server/mod.rs:827) calls pending_activity_ref(), and the WebSocket upgrade path in ServerWebSocket.rs only calls pending_activity_unref() after signaling ConnectionClosed, so the hasPendingActivity() clause covers the native-held case; Request.rs:693's lazily created signal is never aborted natively. The AbortSignal.timeout() case relies on the source wrapper being kept by hasActiveTimeoutTimer() && hasTimeoutObserver() (dependents count as observers via addDependentSignal), so the source and dependent keep each other only until the timer fires. Thread affinity: pendingActivityCount is std::atomic, and the const iteration plus m_timeout read are the same non-mutating reads the removed helper and existing line 43 already did on the same thread.

Other factors

The bug hunt exited on dry_streak with no findings. I could not compile the change or run the tests here (the checkout has no vendor/ WebKit tree), so I cannot independently confirm the const AbortSignal&/void* conversion in the loop compiles or that the tests pass on debug/ASAN; the author reports a debug-build run. Given the subtlety of GC keep-alive rules and the inability to execute the tests locally, a maintainer look is the right additional step rather than an automated approval.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[claude]

Code review found no issues

No high-confidence issues detected in this change.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

[dylan-conway]

Closing. The retention this changes is what the DOM spec requires (§3.2.1: a non-aborted dependent signal with abort listeners must not be collected while its source signals is non-empty), and Node 26 and WebKit behave the same way: the same script keeps 1000 of 1000 alive on Node. Bun should not differ here.