Fix memory leak in AbortSignal.timeout() when listeners are removed - #28761
Conversation
|
Updated 1:04 AM PT - Apr 2nd, 2026
❌ @robobun, your commit 6ec5872 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 28761That installs a local version of the PR into your bun-28761 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTimeout-created AbortSignal reference release was centralized: Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/webcore/AbortSignal.cpp`:
- Around line 270-282: The cleanup condition after setting hasListeners must
also account for abort algorithms still registered; update the conditional in
AbortSignal (around where hasListeners, m_timeout, m_algorithms, and
hasPendingActivity() are checked) to require m_abortAlgorithms.isEmpty() as well
(i.e., add && m_abortAlgorithms.isEmpty()), or alternatively modify
addAbortAlgorithmToSignal to increment the pendingActivityCount (to match
removeAbortAlgorithmFromSignal) so abort algorithms prevent timer cancellation;
ensure you update the logic near AbortSignal::timeout(), cancelTimer(), and
deref() so the ref balance and notification semantics remain correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d16b99cf-8f4f-4d7b-b395-8b667c72602f
📥 Commits
Reviewing files that changed from the base of the PR and between 3ed4186 and 68cdc9f6f8b4268537f1205406c487030f11f0c5.
📒 Files selected for processing (3)
src/bun.js/bindings/AbortSignal.zigsrc/bun.js/bindings/webcore/AbortSignal.cpptest/regression/issue/28756.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/webcore/AbortSignal.cpp`:
- Around line 273-286: Extract the unobserved-timeout cleanup block into a
shared helper (e.g., checkAndCancelUnobservedTimeout or
cleanupUnobservedTimeout) that performs the same condition checks (m_timeout,
!aborted(), m_algorithms.isEmpty(), !hasPendingActivity(),
m_dependentSignals.isEmptyIgnoringNullReferences()) and, while holding
m_abortAlgorithmsLock, tests m_abortAlgorithms.isEmpty() then calls
cancelTimer() and deref() to balance the timeout ref; replace the inline block
in eventListenersDidChange() with a call to this helper, and also call the
helper from removeAlgorithm() and from removeAbortAlgorithmFromSignal() after
releasing m_abortAlgorithmsLock in that code path so the timeout ref is cleaned
up when observers are removed via those methods.
- Around line 281-285: The code calls deref() while holding
m_abortAlgorithmsLock via Locker in AbortSignal::timeout(), which can destroy
*this while the locker is still active; to prevent use-after-free, create a
self-reference (e.g., Ref<AbortSignal> protectedThis{*this};) immediately before
acquiring the Locker so the object stays alive, then acquire the Locker {
m_abortAlgorithmsLock }, perform the isEmpty() check, call cancelTimer()/deref()
as before, and let protectedThis go out of scope after the Locker is destroyed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8db13a04-8602-4f2f-9164-21d1037d5a80
📥 Commits
Reviewing files that changed from the base of the PR and between 68cdc9f6f8b4268537f1205406c487030f11f0c5 and af70b82eedcf9b30d6088cf07b6d3ab1e190ba2f.
📒 Files selected for processing (1)
src/bun.js/bindings/webcore/AbortSignal.cpp
dbe312f to
7e30cfc
Compare
4793b78 to
a2b5f6a
Compare
5834357 to
99b37e8
Compare
040bebe to
7f82890
Compare
When AbortSignal.timeout() creates a timer, it takes an extra ref() on the signal to keep the C++ object alive. The matching unref() only happened in Timeout::dispatch() when the timer actually fires. If all listeners were removed before the timer fires (e.g. via util.aborted()'s FinalizationRegistry cleanup), the timer and signal leaked for the full timeout duration. Two fixes: - eventListenersDidChange(): when a timeout signal loses all observers, cancel the timer and release the extra ref immediately. - signalAbort(): when a timeout signal is aborted via any path, release the extra ref after abort steps complete. This consolidates the unref from Zig Timeout.dispatch() into C++ signalAbort() for all paths. Closes #28756
7f82890 to
6ec5872
Compare
…ven-sh#28761) ## Problem `AbortSignal.timeout()` + `util.aborted()` causes unbounded memory growth (~50 MB/second in the repro). RSS grows indefinitely until segfault. Closes oven-sh#28756 ## Root Cause `AbortSignal::timeout()` takes an extra `ref()` on the C++ signal to keep it alive while the timer is pending. The matching `unref()` only happens in `Timeout::dispatch()` when the timer actually fires. Two paths never released this ref: 1. **All listeners removed before timer fires**: `util.aborted()` uses a `FinalizationRegistry` that removes the event listener when the resource is GC'd. With no listeners left, there's nothing to notify — but the timer + signal stay alive for the full timeout duration (up to 11.5 days in the repro). 2. **Signal aborted via non-timer path**: When a timeout signal is aborted through `AbortSignal.any()` or manual abort, `cancelTimer()` frees the timer struct but never releases the extra ref. ## Fix - **`eventListenersDidChange()`**: When a timeout signal loses all observers (no JS listeners, no native callbacks, no algorithms), cancel the timer and release the extra ref immediately. - **`signalAbort()`**: When a timeout signal is aborted via any path, release the extra ref after all abort steps complete. - **`Timeout.dispatch()`** (Zig): Remove the `unref()` call since `signalAbort()` now handles it for all paths. ## Verification ``` # System bun (unfixed) — FAILS with 128 MB growth: USE_SYSTEM_BUN=1 bun test test/regression/issue/28756.test.ts # Debug build (fixed) — PASSES with ~3 MB growth: bun bd test test/regression/issue/28756.test.ts ``` Also verified: `AbortSignal.timeout()` still fires correctly, `AbortSignal.any()` with timeout works, and `fetch()` with timeout signal aborts properly. Co-authored-by: robobun <robobun@users.noreply.github.com>
…ven-sh#28761) ## Problem `AbortSignal.timeout()` + `util.aborted()` causes unbounded memory growth (~50 MB/second in the repro). RSS grows indefinitely until segfault. Closes oven-sh#28756 ## Root Cause `AbortSignal::timeout()` takes an extra `ref()` on the C++ signal to keep it alive while the timer is pending. The matching `unref()` only happens in `Timeout::dispatch()` when the timer actually fires. Two paths never released this ref: 1. **All listeners removed before timer fires**: `util.aborted()` uses a `FinalizationRegistry` that removes the event listener when the resource is GC'd. With no listeners left, there's nothing to notify — but the timer + signal stay alive for the full timeout duration (up to 11.5 days in the repro). 2. **Signal aborted via non-timer path**: When a timeout signal is aborted through `AbortSignal.any()` or manual abort, `cancelTimer()` frees the timer struct but never releases the extra ref. ## Fix - **`eventListenersDidChange()`**: When a timeout signal loses all observers (no JS listeners, no native callbacks, no algorithms), cancel the timer and release the extra ref immediately. - **`signalAbort()`**: When a timeout signal is aborted via any path, release the extra ref after all abort steps complete. - **`Timeout.dispatch()`** (Zig): Remove the `unref()` call since `signalAbort()` now handles it for all paths. ## Verification ``` # System bun (unfixed) — FAILS with 128 MB growth: USE_SYSTEM_BUN=1 bun test test/regression/issue/28756.test.ts # Debug build (fixed) — PASSES with ~3 MB growth: bun bd test test/regression/issue/28756.test.ts ``` Also verified: `AbortSignal.timeout()` still fires correctly, `AbortSignal.any()` with timeout works, and `fetch()` with timeout signal aborts properly. Co-authored-by: robobun <robobun@users.noreply.github.com>
…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>
Problem
AbortSignal.timeout()+util.aborted()causes unbounded memory growth (~50 MB/second in the repro). RSS grows indefinitely until segfault.Closes #28756
Root Cause
AbortSignal::timeout()takes an extraref()on the C++ signal to keep it alive while the timer is pending. The matchingunref()only happens inTimeout::dispatch()when the timer actually fires. Two paths never released this ref:All listeners removed before timer fires:
util.aborted()uses aFinalizationRegistrythat removes the event listener when the resource is GC'd. With no listeners left, there's nothing to notify — but the timer + signal stay alive for the full timeout duration (up to 11.5 days in the repro).Signal aborted via non-timer path: When a timeout signal is aborted through
AbortSignal.any()or manual abort,cancelTimer()frees the timer struct but never releases the extra ref.Fix
eventListenersDidChange(): When a timeout signal loses all observers (no JS listeners, no native callbacks, no algorithms), cancel the timer and release the extra ref immediately.signalAbort(): When a timeout signal is aborted via any path, release the extra ref after all abort steps complete.Timeout.dispatch()(Zig): Remove theunref()call sincesignalAbort()now handles it for all paths.Verification
Also verified:
AbortSignal.timeout()still fires correctly,AbortSignal.any()with timeout works, andfetch()with timeout signal aborts properly.