AbortSignal.timeout: keep the timer armed when the signal loses its observers - #37666
Conversation
…bservers eventListenersDidChange() cancelled the native timeout timer as soon as the signal had no abort listeners, native callbacks, pending activity, algorithms or dependent signals left. A signal the program still holds then never aborts: signal.aborted stays false after the deadline, throwIfAborted() never throws, and listeners or AbortSignal.any() dependents attached later never fire. Adding a listener for an unrelated event type was enough to trigger it, as was Bun.spawn() releasing the signal when its child exited. The cancel was added when the timer held a ref on the signal and was the only way to free an unobserved one. Since the wrapper became the sole owner, collecting an unobserved signal frees the timer through ~AbortSignal(), so the timer now lives until the signal aborts or is destroyed, as in Node and the browsers.
|
Updated 2:38 AM PT - Aug 12th, 2026
✅ @robobun, your commit 88adbf084fa3666a712b80327996ffd43a7f6a62 passed in 🧪 To try this PR locally: bunx bun-pr 37666That installs a local version of the PR into your bun-37666 --bun |
|
Status: reproduced on bun 1.4.0 with the snippet in the description ( |
|
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 (2)
💤 Files with no reviewable changes (1)
Walkthrough
ChangesAbortSignal timeout behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is small and the reasoning is convincingly traced through #28756/#28761/#35849, but it deletes a timer-cancel that was originally added to prevent a leak in GC-sensitive JSC lifetime code — a maintainer familiar with the AbortSignal timeout history should confirm the argument that #35849 made it fully redundant.
What was reviewed:
- Confirmed
~AbortSignal()andmarkAborted()are now the onlycancelTimer()call sites, and the timer's raw back-pointer cannot outlive the signal (destructor cancels first). - Verified
isReachableFromOpaqueRootsgates onhasActiveTimeoutTimer() && hasTimeoutObserver(), so an unobserved signal's wrapper stays collectible and the #28756 leak path is still handled by GC. - Checked the new tests: the fence-signal pattern keeps the signal under test unobserved and orders on the timer heap rather than wall-clock;
describe.concurrentand the shell-based spawn keep them fast; Windows/POSIX both covered.
Extended reasoning...
Overview
The PR deletes an eager cancelTimer() branch from AbortSignal::eventListenersDidChange() in src/jsc/bindings/webcore/AbortSignal.cpp and adds a comment at the timeout() factory documenting that only markAborted() and ~AbortSignal() stop the timer. Six new tests in test/js/web/abort/abort.test.ts cover four listener-churn shapes, re-observation after churn (listener, AbortSignal.any, throwIfAborted), and the Bun.spawn native-callback release path.
Security risks
None identified. This is a Web API correctness fix; no untrusted input parsing, auth, or privilege boundary is touched.
Level of scrutiny
High. This is native JSC binding code that manages the lifetime of a heap-allocated timer holding a raw back-pointer to a refcounted C++ object whose JS wrapper's reachability is decided by a custom isReachableFromOpaqueRoots. Per the repo's review guidance, native memory-safety changes are the most-blocked category, and "before deleting odd-looking code, git-blame why it was written — it is usually load-bearing." The PR does that git-blame work (the branch dates from #28761 as the fix for the #28756 leak, and #35849 later removed the timer's self-ref that made the branch necessary), and I verified against the current JSAbortSignalCustom.cpp that reachability now hinges on hasTimeoutObserver() — so an unobserved signal is collectible and ~AbortSignal() frees the timer. I also verified the timer doesn't ref the signal (Timeout.signal in src/jsc/AbortSignal.rs is documented as a raw non-owning back-pointer), so keeping it armed cannot pin the signal.
Other factors
The tests are well-constructed: they wait on a second, longer AbortSignal.timeout fence rather than sleep, keeping the signal under test unobserved and ordering on the timer heap instead of wall-clock; they cover the full variant matrix including the "unrelated event type" case that isn't even a removal; the spawn test picks a shell over bunExe() for a fast exit and branches on isWindows. The PR description reports the #28756 regression test and the surrounding leak/GC-root suites still pass and gives before/after RSS numbers. That said, the correctness of removing this branch rests on a chain of prior refactors to GC-sensitive lifetime code, which is exactly where a second pair of eyes from someone who reviewed #35849 is worth the small extra latency.
|
One more data point for the "is the GC path alone enough" question, since that is the load-bearing claim here: the state this change leaves a churned signal in (timer armed, observer count zero, wrapper the only owner) is already reachable on main. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
We should still cancel the timer, just only if the JS wrapper has been finalized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@Jarred-Sumner I went through what "cancel once the wrapper is finalized" would do with the code that holds a C++ timeout signal today, and I don't think there is a safe point for it yet: Request hands the signal back out after its wrapper is gone. const req = new Request("http://localhost/", { signal: AbortSignal.timeout(50) });
Bun.gc(true); // heapStats(): the signal's wrapper is gone, the Request keeps the C++ signal
await Bun.sleep(150);
req.signal.aborted; // true today (1.4.0 and this branch); false if the timer is cancelled when the wrapper is finalizedCancelling in Every other holder drops its ref in the same call as its observer. Gating the old branch on If you still want the cancel, the prerequisite is making Request (and |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes AbortSignal timer lifetime relative to GC (deleting the eager cancel that #28761 added for #28756), and there's an open question to a maintainer about whether to also pin the Request-held signal wrapper here or in a follow-up, a human look is still warranted.
What was reviewed:
- The deletion in
eventListenersDidChange()leaves timer cleanup tomarkAborted()and~AbortSignal()only; verified those two sites both callcancelTimer(). - Checked that the "timer armed, observer count zero" state is already reachable on main via
releaseSourceObserverCounts()/decrementPendingActivityCount(), so this isn't a new lifetime shape. - New tests: fence-signal pattern keeps the signal under test unobserved; the spawn test branches on
isWindows; the Request/heapStats test runs in a subprocess and drains stdout/stderr/exited concurrently.
Extended reasoning...
Overview
The src change is a pure 8-line deletion in AbortSignal::eventListenersDidChange() (src/jsc/bindings/webcore/AbortSignal.cpp), removing the branch that eagerly cancelled the native timer when m_timeoutObserverCount dropped to zero. The test change adds a describe.concurrent block to test/js/web/abort/abort.test.ts covering four listener-churn variants, re-observation after churn, a Bun.spawn() native-callback release, and a Request-held-signal wrapper-collection case.
Security risks
None. This is Web API spec-conformance for AbortSignal.timeout(); no auth, crypto, untrusted input parsing, or privilege boundaries are involved.
Level of scrutiny
High. The deleted branch was introduced in #28761 specifically to fix the #28756 leak, and this PR's correctness rests on a full lifetime analysis: that #35849's removal of the timer→signal self-ref made the branch redundant (GC of the wrapper now runs ~AbortSignal() which frees the timer), and that no native holder can keep the C++ signal alive past its wrapper in a way that would leak the timer. The author's analysis is thorough and the #28756 regression test reportedly still passes, but per REVIEW.md this is exactly the "delete odd-looking code → git-blame why it was written → it is usually load-bearing" case where a maintainer should confirm the lifetime claims. This is JSC GC / memory-safety territory, the most-blocked review category.
Other factors
The author's last comment is addressed to a maintainer with an open design question — whether to also make Request pin the signal's wrapper (so a future finalize-time cancel becomes safe) in this PR or a follow-up. The PR is stated as ready-as-is ("the deletion plus the tests"), but the question signals expected maintainer engagement before merge. The comment-cop bot's flags have been addressed (the AbortSignal.cpp comment is back to its main-branch text; the src diff is now deletion-only). The tests follow harness conventions (bunEnv/bunExe, concurrent pipe draining, isWindows branching, subprocess isolation for heapStats), and the fence-signal pattern avoids wall-clock sleeps for the churn cases.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The 8-line deletion in eventListenersDidChange() is well-reasoned and the test coverage is thorough, but since this touches AbortSignal timer lifetime / GC reachability and there's an open design question to a maintainer (whether to also cancel on wrapper finalization, and whether Request should pin the wrapper), a human look is still worthwhile.
What was reviewed:
- Traced timer cancellation to its two remaining call sites (
markAborted(),~AbortSignal()) — no path leaves a timer armed past signal destruction. - Checked the #28756 leak scenario: with the #35849 ownership model, an unobserved wrapper is collectible and
~AbortSignal()frees the timer; the regression test is reported passing. - The "timer armed, observer count zero" state is already reachable on main via
releaseSourceObserverCounts()/decrementPendingActivityCount(), so this doesn't introduce a new state. - Tests: fence pattern keeps the signal-under-test unobserved; churn matrix, native consumers (fs.watch, Bun.spawn), and the Request/heapStats subprocess case all look sound.
Extended reasoning...
Overview
The source change is an 8-line deletion in AbortSignal::eventListenersDidChange() (src/jsc/bindings/webcore/AbortSignal.cpp) that removes eager timer cancellation when a timeout signal's observer count reaches zero. The rest of the diff is ~160 lines of new tests in test/js/web/abort/abort.test.ts covering listener churn, native-callback release (fs.watch, Bun.spawn), re-observation after churn, and a Request-held signal whose wrapper is GC'd before the deadline.
Security risks
None. This is a Web API correctness/lifetime fix; no auth, crypto, untrusted input parsing, or permission surface is touched.
Level of scrutiny
High. Although the source diff is a pure deletion, it changes when a native timer is freed relative to JSC GC of the AbortSignal wrapper. The repo's review guidance flags GC lifetime and reference-count balance as the most-blocked category. The PR description does trace the full history (#28761 introduced the branch when the timer held a self-ref; #35849 removed the self-ref and made the wrapper the sole owner, so ~AbortSignal() now frees the timer on collection), and confirms the #28756 leak regression test still passes. The analysis of why a wrapper-finalization cancel is not safe (Request re-wraps the native signal after the original wrapper is collected) is careful and now pinned by a test. I did not find a hole in the reasoning.
Other factors
- There is an open, unanswered question to a maintainer about whether the eager cancel should instead move to wrapper finalization (with Request changed to pin the wrapper). The author explicitly offered to do that here or as a follow-up. This is a design decision a maintainer should sign off on before merge.
- The comment-cop bot feedback was addressed (8f6d130 reverts the comment to its main-branch text; the src diff is now just the deletion).
- Test quality is good:
describe.concurrent, a fence pattern that avoids observing the signal under test,test.eachover the churn matrix, subprocess isolation for the heapStats case with a tolerance for conservative-scan stragglers, and per-platform spawn args for Windows. The spawn test's 500ms deadline for a shell to exit is generous but is the one place a very loaded CI runner could theoretically race.
Given the GC-sensitive subject matter and the outstanding maintainer question, deferring rather than auto-approving.
… it unfired (#37846) ### Problem - An `AbortSignal.timeout()` signal with an abort listener stays in the heap for the rest of the process once its timer is dropped by `useRealTimers()` / `clearAllTimers()` or by the `bun test --isolate` file swap. In the repro in the original description, 500 such signals survive `Bun.gc(true)` on main. - Under `--isolate` this pins the retired file's whole global object graph: 8 files that each leave one such signal pending climb to 8 live globals, where the other leaked-handle fixtures plateau at 1 to 3. - Cause: the GC keeps an observed timeout signal alive for as long as the signal itself says it has a timer, and only the signal's `cancelTimer()` clears that. Both paths above pulled the timer out of the heap without going through the signal, so it kept reporting a timer that could never fire. - The only remaining thing that clears it is the signal's destructor, which needs the GC to collect the signal first: a cycle. ### Fix - Every place the heap drops an `AbortSignal.timeout` timer unfired (fake-heap drain, `--isolate` swap, VM teardown, and the generation-stale exit when a stale timer comes due) now hands it back to the signal's `cancelTimer()`, which clears the flag and frees the timer. - Correct because it is the same path a firing timer already takes, and it produces the state these callers already wanted: an inert signal that never aborts and is collectable like any other. Dropped timers still never fire, and the existing `--isolate` "does not fire in next file" test still passes. - Verification: new fake-timers tests (the `useRealTimers()` and `clearAllTimers()` cases fail on main with 500 survivors) and two new `--isolate` fixtures that hit 8 live globals on main and plateau with the fix. Teardown relies on an existing worker-terminate test; the related timer and abort suites pass under ASAN. - Found while reviewing #37666; independent of it. ### Background - `AbortSignal.timeout(n)` aborts with `TimeoutError` after n ms. So that `AbortSignal.timeout(n).addEventListener("abort", cb)` works with no JS reference held, the GC keeps the wrapper alive while the signal has both a pending timer and an observer (`JSAbortSignalOwner::isReachableFromOpaqueRoots`). "Pending timer" means the C++ signal's `m_timeout` is non-null. - The native timer is a box owned by the C++ `AbortSignal` and linked as a node into bun's timer heap, with a back-pointer to the signal. Firing runs through the signal (`markAborted()` -> `cancelTimer()`), which clears `m_timeout` and frees the box; unlinking the heap node alone leaves `m_timeout` set. - bun:test fake timers hold timers in a separate fake heap that only advances on `advanceTimersByTime` and friends; `useRealTimers()` / `clearAllTimers()` empty it, and a timer emptied that way is meant to never fire. - `bun test --isolate` runs each file in a fresh global and, at the swap (and at VM teardown), cancels every timer still in either heap so a previous file's callbacks cannot run against the new global. Timers also carry a generation number so a stale one that comes due anyway is skipped. <details> <summary>Original description</summary> ### What An `AbortSignal.timeout()` signal that has an abort listener (or any other observer) stays pinned in the heap for the rest of the process once its timer is dropped by `useRealTimers()` / `clearAllTimers()` or by the `bun test --isolate` file swap. Under `--isolate` that pins the retired file's whole global object graph, one global per file that leaves such a signal pending. ```ts import { heapStats } from "bun:jsc"; import { test, vi } from "bun:test"; test("repro", () => { vi.useFakeTimers(); for (let i = 0; i < 500; i++) AbortSignal.timeout(1_000_000).addEventListener("abort", () => {}); vi.useRealTimers(); Bun.gc(true); console.log(heapStats().objectTypeCounts.AbortSignal); // 500 on main; 0 (give or take a stack-conservative one) with this PR }); ``` `--isolate` variant: 8 files that each do `AbortSignal.timeout(3_600_000).addEventListener("abort", () => {})` at module scope and count `GlobalObject` cells after `Bun.gc(true)`. On main the count climbs 1, 2, ..., 8; the existing `setTimeout` / `Bun.serve` / `fs.watch` fixtures in the same describe block plateau at 1 to 3, and so does this one with the fix. ### Why `JSAbortSignalOwner::isReachableFromOpaqueRoots` keeps a timeout signal's wrapper alive while `hasActiveTimeoutTimer() && hasTimeoutObserver()`. That is what makes `AbortSignal.timeout(n).addEventListener("abort", cb)` work with no JS reference to the signal, and it is only correct for as long as the timer can actually fire. `hasActiveTimeoutTimer()` is `m_timeout != nullptr`, and `m_timeout` is only cleared by `AbortSignal::cancelTimer()` (called from `markAborted()` and `~AbortSignal()`). Two places on the Rust side took the timer away without going through the signal, so the signal kept claiming a timer it no longer had: - `FakeTimers::clear()` (`useRealTimers()`, `clearAllTimers()`) popped every node out of the fake heap and only released `TimeoutObject` pins; `AbortSignalTimeout` nodes were just dropped. - `All::cancel_all_timeout_objects` (the `--isolate` swap and VM teardown) unlinked `AbortSignalTimeout` nodes and nulled their back-pointer. After either, the timer can never fire, `m_timeout` is still set, the observer is still there, and the wrapper (plus its listeners, plus whatever they close over) is unreachable for GC until the process exits. `~AbortSignal()` is the only remaining thing that would clear `m_timeout`, and it only runs once the wrapper is collected: a cycle through the GC's reachability rule. The behavior these paths want is unchanged: a timer dropped by `useRealTimers()` never fires (same as a `setTimeout` created under fake timers, see the first test in `fake-timers.test.ts`), and `--isolate` must not fire a previous file's signal in the next file (`test/cli/test/isolation.test.ts`, "leaked AbortSignal.timeout does not fire in next file", still passes). The signal should simply end up as an inert, never-aborting signal that is collectable like any other, which is exactly the state `cancelTimer()` produces. ### How - `AbortSignal::cancelTimer()` becomes public, exposed to Rust as `WebCore__AbortSignal__cancelTimer`. - New `Timeout::discard(this)` in `src/jsc/AbortSignal.rs`: asserts the box is the one its signal owns and calls `cancelTimer()` on the signal, which clears `m_timeout` and frees the box via the existing `AbortSignal__Timeout__deinit` (which also unlinks the node if it is still in a heap). This is the same path a firing timer takes through `markAborted()`, so nothing new has to be kept in sync. - `FakeTimers::clear()` now collects `AbortSignalTimeout` nodes alongside the `TimeoutObject` pins (small `ClearedTimers` struct, released by both callers after the `FakeTimers` borrow ends, same reason `release_heap_pin` was already deferred) and discards them. - `cancel_all_timeout_objects` discards the signal timeouts instead of unlinking them and nulling `signal`; nulling is unnecessary now that the box is gone. At VM teardown this also means `~AbortSignal()` later finds `m_timeout` already cleared. - The generation-stale early return in `Timeout::run` (a timer from a retired `--isolate` file that somehow comes due) discards too, instead of leaving the same pinned state behind. In practice the swap's `cancel_all_timeout_objects` handles these first; this just makes the remaining exit consistent. ### Tests `test/js/bun/test/fake-timers/fake-timers.test.ts`, new `AbortSignal.timeout` block: - control: 500 observed signals stay alive while the fake heap holds their timers (`getTimerCount() === 500`) - `useRealTimers()` releases them; `clearAllTimers()` releases them (both fail on main with 500 survivors) - a signal the program still holds is left unaborted and usable (`AbortSignal.any`, `addEventListener`) after its fake timer is dropped - `advanceTimersByTime` still aborts the signal with `TimeoutError` `test/cli/test/isolation.test.ts`, "collects globals pinned by leaked handles": two new fixtures, an observed `AbortSignal.timeout` left pending in the real heap and one left pending in the fake heap of a file that never called `useRealTimers()` (both reach 8 live globals on main, plateau with the fix). VM teardown is covered by the existing `test/js/web/workers/worker-terminate-funnels.test.ts` (armed, observed `AbortSignal.timeout` in a terminated worker), which passes on the ASAN debug build along with `timer-gc-roots.test.ts`, `abort.test.ts`, `test-timers.test.ts`, the whole `fake-timers/` directory and `isolation.test.ts`. Also ran worker terminate x3 and `BUN_DESTRUCT_VM_ON_EXIT=1` (real and fake heap) with pending observed timeouts under ASAN by hand, clean. Found while reviewing #37666; independent of it (that PR only removes the eager cancel in `eventListenersDidChange`, and this one does not touch that function). </details>
2 participants
Problem
AbortSignal.timeout()never fires if the signal loses its observers before the deadline: add then remove an abort listener, set then clearonabort, close thefs.watch()it was passed to, let aBun.spawn()child exit, or even add a listener for an unrelated event. Repro in the original below.abortedstaysfalse,reasonstaysundefined,throwIfAborted()never throws, and listeners orAbortSignal.any([s])dependents added later never fire. Node and the browsers reportaborted: truewith aTimeoutErrorin every case.AbortSignal.timeout()+util.aborted()causes unbounded memory growth #28756). Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC #35849 removed that ref, so since then the branch only freed the timer slightly before the next GC would.Fix
AbortSignal.timeout()+util.aborted()causes unbounded memory growth #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.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:http2was affected too and was checked by hand only.Background
AbortSignal.timeout(ms)must abort with aTimeoutErroroncemspasses, 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.node:fsescaped only because of the order they release things in.Requestis the exception: it holds a native ref and re-wraps the signal each timerequest.signalis 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.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
Original description
AbortSignal.timeout()stops working as soon as the signal's observer count touches zero while the program still holds the signal:The same happens after
s.onabort = fn; s.onabort = null, after a listener registered with{ signal: controller.signal }is removed bycontroller.abort(), afterawait Bun.spawn({ cmd, signal: s }).exited(the subprocess releases its native callback when the child exits), and even afters.addEventListener("unrelated", fn)with nothing removed at all. Once it has happened,s.reasonstaysundefined,s.throwIfAborted()never throws, and abort listeners orAbortSignal.any([s])dependents attached afterwards never fire. Node printstruewith aTimeoutErrorreason for every one of these, and so do the browsers.Cause
AbortSignal::eventListenersDidChange()insrc/jsc/bindings/webcore/AbortSignal.cppended withIt runs on every listener add or remove, and
m_timeoutObserverCountis 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 readaborted/reason, callthrowIfAborted(), 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), whichJSAbortSignalOwner::isReachableFromOpaqueRootsalready 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 untilrequest.signalis 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 throughrequest.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,isReachableFromOpaqueRootskeeps it alive only whilehasTimeoutObserver(), 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__createinserts 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()(anAbortSignal.any()dependent aborting) anddecrementPendingActivityCount()(fetch finishing) drop the count without going througheventListenersDidChange(), and the existingtimer-gc-roots.test.tscase "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.tspasses.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 plusfs.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()throwssignal.reason), and the asynchronousBun.spawn()release when the child exits. A further test constructs Requests around timeout signals, checks withheapStats()that the signal wrappers were collected before the deadline, and then readsrequest.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 infetch-leak.test.ts,test/js/bun/spawn/spawn-signal.test.ts,test/js/node/util/test-aborted.test.ts, the--isolateleaked-timeout test intest/cli/test/isolation.test.ts, and Node'stest-abortsignal-any.mjs.Native consumers affected on the unfixed build, from going through each
add_listener/listensite and its release order:Bun.spawn/spawnSync(Subprocess::clear_abort_signaldrops pending activity first, then the callback),fs.watch(detach()removes the callback; closing the watcher left the signal stuck), andnode:http2session.request(headers, { signal })(the stream'sSignalRefdrop; 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,TimeoutErrorwith the fix, same as Node) and goes through the samecleanNativeBindings()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 thatspawnSync({ 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(),Responseandnode:fsreadFile/writeFilewere not affected: they drop their callback before their pending-activity count (or hold only pending activity), so the counter never reached zero insideeventListenersDidChange(). With this change the release order of native consumers no longer matters.