fetch, Bun.spawn: do not root the AbortSignal they were given

[dylan-conway]

What does this PR do?

Fixes a leak: a fetch() Response that is reachable only from an abort listener on the signal it was fetched with is never collected, and neither is the signal or anything the listener closes over.

const controller = new AbortController();
const response = await fetch(url, { signal: controller.signal });
controller.signal.addEventListener("abort", () => console.log(response.status));
// drop controller and response: both stay alive forever

A signal with an abort listener and a pending-activity count is a GC root, and two things held that count for as long as the Response lived, which closes the cycle signal → listener → Response → count:

• the Response's own abort listener (added in fetch: error the response body stream when a fully-buffered response is aborted #35093 so that abort still errors a fully-buffered body), until the Response was finalized;
• the fetch task, for as long as it follows the signal (AbortHandle::follow, shared with Bun.spawn). A body past the receive high-water mark that is never read keeps the fetch in flight until the Response is finalized.

Both counts are removed. Neither was what kept anything working: each holder keeps a reference to the native signal, and the native callback it registers already counts as an abort listener and as a timeout observer, which is what keeps an AbortSignal.timeout() or AbortSignal.any() signal alive while it can still fire. A controller roots its own signal. Native abort delivery does not go through the JS wrapper. Server request signals, which native code aborts, keep their count.

How did you verify your code works?

• fetch-leak.test.ts: responses held only by their signal's listener, with a small read body, a small unread body and a 1 MB unread body. On 1.4.2 it leaves 49 Response and 49 AbortSignal cells after GC; with this change it passes. With only the Response change, the 1 MB shape still leaked 40 of 40.
• fetch-leak.test.ts: responses outlive the wrapper of the signal they were fetched with (controller dropped, responses held): the wrappers are collected (21 cells on 1.4.2), and the bodies and clones still read.
• fetch-abort-stream-body.test.ts: after forced GCs, abort still reaches an in-flight fetch() whose signal nothing else references, through the two routes that need a JS wrapper to stay alive: inline any([timeout]) (nothing native holds the timeout source), and an inline timeout signal with a JS listener that must still run.
• spawn-signal.test.ts: after forced GCs, an inline any([timeout]) still kills the child.

The last two pass before and after the change. All of fetch-abort-stream-body.test.ts and spawn-signal.test.ts, and the signal tests in fetch-leak.test.ts, pass 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: c06c6d8d-d1f1-4c0b-bf87-00dfe489fd7b

📥 Commits

Reviewing files that changed from the base of the PR and between 572d568 and eaaa8d9.

📒 Files selected for processing (2)

• test/js/bun/spawn/spawn-signal.test.ts
• test/js/web/fetch/fetch-abort-stream-body.test.ts

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

---

Walkthrough

The runtime no longer changes abort-signal pending activity during abort listener lifecycle operations. Regression tests cover fetch collection, response use, fetch abortion, and child-process termination after garbage collection.

Changes

Abort listener garbage collection

Layer / File(s)	Summary
Remove abort listener activity bookkeeping src/runtime/webcore/Response.rs, src/jsc/ScriptExecutionContext.rs	Response and AbortHandle no longer update abort-signal pending activity. Listener registration and native cleanup remain unchanged.
Validate fetch collection and response use test/js/web/fetch/fetch-leak.test.ts	Subprocess tests trigger repeated garbage collection, check bounded Response and AbortSignal counts, and verify that original and cloned responses remain readable.
Validate abort behavior after collection test/js/web/fetch/fetch-abort-stream-body.test.ts, test/js/bun/spawn/spawn-signal.test.ts	Tests verify that unreferenced signals still abort pending fetches with the expected errors and terminate a child process with SIGTERM.

Suggested reviewers: robobun

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to eaaa8

The change removes obsolete abort-signal activity bookkeeping while preserving fetch and child-process abort behavior after garbage collection. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: preventing fetch and Bun.spawn from rooting the provided AbortSignal.
Description check	✅ Passed	The description includes both required sections. It explains the memory leak, the implementation changes, and the verification tests with sufficient technical detail.
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.

---

_{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 finding, I also checked the wrapper-collected-before-Response case: BodyAbortListener.signal is an AbortSignalRef (WTF RefCounted +1, src/jsc/AbortSignal.rs:161), so the native signal outlives its JS wrapper and clean_native_bindings in Drop always targets a live object. FetchTasklet never took pending activity on the signal, so in-flight aborts are unaffected, and no stale pending-activity comments remain in Response.rs.

Extended reasoning...

A confirmed finding is already attached inline, so this note only records what else was examined. The PR description flags uncertainty about the signal's JS wrapper being collected while the Response is alive; the native AbortSignal is intrusively refcounted and the listener's AbortSignalRef holds a +1 until the Response drops the box, so the Drop path (clean_native_bindings) cannot touch freed memory. The only other pending_activity_* users in src/ are server request signals, node:fs, WebSocket, and ScriptExecutionContext; none are on the fetch client path, so removing the count changes nothing for in-flight requests. No comment in Response.rs still references the removed pending-activity count.

[claude]

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

The new push dropping the AbortHandle pending-activity pair in src/jsc/ScriptExecutionContext.rs covers the in-flight-fetch shape from the earlier thread. Beyond the inline finding, I also checked the #4517 regression shapes against JSAbortSignalCustom.cpp: an unreferenced AbortSignal.timeout with a JS listener stays rooted through m_timeoutObserverCount (native callbacks count as abort listeners in eventListenersDidChange), and AbortSignal.any stays rooted via alive source signals, so neither depends on the removed count. The remaining pending_activity_ref sites (server Request signal in RequestContext.rs/server_body.rs, node_fs watchers, ServerWebSocket) root signals that have no JS-side owner, so they are a different class and correctly left alone.

Extended reasoning...

The diff removes four lines: the pending_activity_ref/pending_activity_unref pairs on AbortHandle::follow/unfollow and on BodyAbortListener. The native AbortSignalRef (ref_()/opaque_ref) still keeps the C++ signal alive and add_listener/clean_native_bindings remain balanced, so only the JS-wrapper GC-root side effect changes. I traced the wrapper-reachability rules in JSAbortSignalOwner::isReachableFromOpaqueRoots to confirm the shapes that originally motivated the pending-activity root (#4517) remain rooted by other counters: AbortSignal::eventListenersDidChange bumps m_timeoutObserverCount whenever native callbacks are non-empty, so a timeout signal with a native fetch/spawn listener is rooted via "Has Observed Timeout" until it fires, and a dependent signal with a live source is rooted via hasAliveSourceSignals. A plain AbortController signal whose controller is dropped cannot be aborted by anyone, so losing its wrapper is harmless. The other pending_activity_ref callers left in tree root server-side Request signals and fs watchers, which have no AbortController wrapper to act as an opaque root, so they are not the same class as this fix. The one inline finding is a nit on test strength; I have nothing further to add on the tests.

[claude]

I reviewed the latest push and didn't find any bugs; both points from my earlier inline comments (the AbortHandle pending-activity pair for in-flight fetches, and test variants whose abort route actually needs a live wrapper) are addressed by 572d568 and eaaa8d9. Because this deletes GC roots on a wrapper-lifetime path, a maintainer look is still worthwhile.

What was reviewed:

• Traced JSAbortSignalOwner::isReachableFromOpaqueRoots against the two removed pairs: a native callback flips hasAbortEventListener and bumps m_timeoutObserverCount, and addDependentSignal bumps the source's count, so AbortSignal.timeout() and any([...]) chains stay rooted without pending activity; a plain controller signal is rooted by its controller or cannot fire.
• Checked the remaining pending_activity_ref holders: server request signals are natively aborted and keep their count; node_fs releases on operation completion, so it is not the same unbounded shape.
• Both ref/unref pairs were removed together; no remaining unref is unbalanced.
• Could not execute the new tests here (no debug build available in this checkout); the leak assertions are heapStats-based with a bounded GC poll rather than a sleep.

Extended reasoning...

Overview

The PR deletes four lines: the pending_activity_ref()/pending_activity_unref() pair in BodyAbortListener (src/runtime/webcore/Response.rs, attach_abort_signal / Drop) and the same pair in AbortHandle::follow / unfollow (src/jsc/ScriptExecutionContext.rs), shared by the fetch tasklet and Bun.spawn. Both holders retain their native AbortSignalRef and their registered native callback. Tests add two heapStats().objectTypeCounts leak regressions in fetch-leak.test.ts (including the 1 MB unread-body shape that the first commit alone did not fix), and one "still fires after forced GC" test each in fetch-abort-stream-body.test.ts and spawn-signal.test.ts.

Security risks

None specific to this change: no input parsing, auth, or crypto paths. The risk class is memory/lifetime: a use-after-free would need the native signal to be freed while a listener is registered, but each holder keeps a native ref for as long as the callback is registered, and clean_native_bindings is called before that ref is dropped on every exit (Drop, unfollow, disarm). The opposite failure — a timeout or dependent signal whose wrapper is collected before it can fire — is what I traced in JSAbortSignalCustom.cpp and AbortSignal.cpp: addNativeCallback -> eventListenersDidChange sets HasAbortEventListener and increments m_timeoutObserverCount; AbortSignal::any calls addDependentSignal which increments the source's observer count, so a timeout() source is reachable via hasActiveTimeoutTimer() && hasTimeoutObserver() and the dependent via hasAbortEventListener() && isDependent() && hasAliveSourceSignals(). The pending-activity branch (#4517) is thus redundant for these holders.

Level of scrutiny

Moderate-high despite the tiny diff. REVIEW.md says not to silently delete existing Strong refs / pending-activity counts, and the justification here rests on a chain of C++ invariants rather than anything local to the diff. I verified the chain from source, but a maintainer familiar with the AbortSignal rooting history (#4517, #35093) should confirm there is no native aborter of a plain, controller-less signal outside the server request path — I found none via WebCore__AbortSignal__signal callers, but that is a negative search.

Other factors

Sibling pending_activity_ref sites (server/mod.rs, server_body.rs, RequestContext.rs, ServerWebSocket.rs, node_fs.rs) were checked: the server ones are for natively-aborted request signals and are deliberately kept per the PR description; node_fs holds the count only for the duration of a single readFile/writeFile and releases on completion, so it is not an unbounded leak. The tests use bounded GC polling loops with setImmediate yields rather than sleeps, drain stdout/stderr concurrently, and assert output before the exit code. No debug build exists in this checkout, so I did not execute the tests. The prior ruled-out nit (spawn test hangs to the runner timeout if abort never fires) remains a nit only.