Move WTFTimer out of the shared timer heap to fix a cross-thread race - #33131
Conversation
|
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:
WalkthroughThe timer runtime moves heap and epoch state behind a guarded ChangesTimer heap locking refactor and race coverage
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 3:27 PM PT - Jul 14th, 2026
❌ @robobun, your commit f0fa434 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33131That installs a local version of the PR into your bun-33131 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
I looked at all three before adding any #18181 is plausibly this bug, but I cannot prove it, so I am not closing it. Its stack is the exact code path this PR serializes: the event loop's pre-poll timer pass ( #30418 is not this bug. That crash is #24033 is not this bug either. The fault is inside the user's JS callback ( Leaving the description as is; if a maintainer wants #18181 linked, the cross-reference from this comment is there. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Can the code be restructured to avoid unsafe instead of adding more unsafe?
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find issues, but this restructures lock acquisition and node-state bookkeeping in the core event-loop timer heap with subtle cross-thread and re-entrancy reasoning — worth a human pass on the lock/unlock placement and the broadened InHeap::None no-op in remove_lock_held.
Extended reasoning...
Overview
This PR modifies src/runtime/timer/mod.rs and src/runtime/timer/WTFTimer.rs to close a data race on the intrusive pairing heap that backs every runtime timer (setTimeout/setInterval, the JSC GC RunLoop timer, the date-header timer, the event-loop-delay monitor, etc.). All::get_timeout now takes All.lock around its peek/pop loop and releases it only around EventLoopTimer::fire; both pop sites (get_timeout and All::next) now write in_heap = None / state = FIRED under the lock; WTFTimer::fire no longer writes state itself; remove_lock_held now treats InHeap::None as a silent no-op instead of debug_assert!(false); and the Windows on_uv_timer callback now locks around ensure_uv_timer. A new debug-only test fixture drives GC against a timer chain to exercise the path.
Security risks
None in the auth/crypto/permission sense. The risk class here is memory safety and liveness: this is unsafe Rust with raw-pointer heap manipulation and manual (non-RAII) lock/unlock. A misplaced unlock would deadlock the event loop (the Mutex is non-recursive and fire re-enters All); a missed lock would leave the original race; and the InHeap::None early-return now silently swallows a state that previously asserted, which could mask future bookkeeping bugs in callers other than the documented WTFTimer::cancel race.
Level of scrutiny
High. This is the scheduling core of the runtime — a regression here affects every timer in every Bun process. The control flow in get_timeout was non-trivially restructured (the inner match became a break Some((min_next, now)) loop with post-loop branching), and All::next now writes state = FIRED for all popped nodes, not just WTFTimer, which depends on the claim that every fire handler already treats a popped node as FIRED. The reasoning in the PR description is thorough and the changes look correct to me, but the unlock-around-fire pattern, the cross-thread re-arm window between pop and fire, and the interaction with the Windows insert_lock_held → ensure_uv_timer path (already called under lock) deserve a reviewer who knows this subsystem.
Other factors
The author explicitly notes the race cannot be deterministically reproduced from JS, so the new test is a smoke test for the code path rather than a fail-before regression. CI is still building. No prior human review on this PR. Given the concurrency subtlety, the unsafe-Rust surface, and the breadth of timers routed through these two functions, I'm deferring rather than approving.
|
Done in d748590. The lock no longer goes through a raw pointer anywhere.
The detach marking moved into Net, the diff now removes one more One thing from the automated review while I was at it: instead of dropping the |
There was a problem hiding this comment.
I didn't find any bugs, but this reworks lock ordering and node-state invariants on the cross-thread timer heap that every setTimeout and the GC scheduler share, so it warrants a careful human pass.
Extended reasoning...
Overview
This PR changes src/runtime/timer/mod.rs and src/runtime/timer/WTFTimer.rs to close a data race on the intrusive pairing-heap that backs all runtime timers. All::get_timeout now takes All.lock around its peek/pop (via a new pop_due_wtf_timer helper and HeapHead enum) and releases it around fire(); TimerHeap::delete_min now writes in_heap = None and state = FIRED on the popped node; WTFTimer::fire no longer rewrites state; remove_lock_held now no-ops on an already-detached WTFTimer node instead of asserting; and Windows' on_uv_timer re-arm now locks via a new rearm_uv_timer. A debug-only regression test and fixture are added.
Security risks
None in the conventional sense — no parsing, auth, or external input handling. The risk class here is memory safety / heap corruption: a wrong lock placement or a missed state transition in this code can corrupt the intrusive heap that schedules every timer in the process, which is exactly the failure mode the PR is fixing.
Level of scrutiny
High. This is core event-loop scheduling code on the hot path of every setTimeout/setInterval and the JSC GC scheduler, with cross-thread mutation, a non-recursive mutex that must be released around re-entrant fire(), and TOCTOU windows between unlocked state == ACTIVE checks and locked heap operations. The reasoning in the description is detailed and looks sound, but the correctness depends on subtle invariants (e.g., that delete_min's new unconditional state = FIRED write is compatible with every caller, including the three FakeTimers call sites that also go through TimerHeap::delete_min), and the author notes the underlying race cannot be deterministically reproduced from JS.
Other factors
The bug-hunting pass found nothing, the existing timer test suite reportedly passes, and the new test exercises the lock placement (a misplaced lock deadlocks the debug mutex). Still, given the blast radius — silent heap corruption in release builds if any of the new state-machine edges are wrong — and the cross-cutting change to delete_min semantics, a maintainer who knows this subsystem should sign off.
|
On the one concrete question in the automated review, the
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/timers/setTimeout.test.js`:
- Around line 559-568: The test in setTimeout.test.js is too strict about
stderr, since filtering only the ASAN warning still leaves other benign
debug/ASAN output that can make the empty-string assertion flaky. Update the
assertion in the relevant test case to keep validating the important success
signals via stdout, exitCode, and signal, and relax stderr handling so it only
checks for absence of fatal/error markers instead of requiring stderr to be
exactly empty. Use the existing stderrLines handling in the test as the place to
adjust this expectation.
🪄 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: 14471e5d-38d5-411b-a1d8-c69fec61d1fe
📒 Files selected for processing (4)
src/runtime/timer/WTFTimer.rssrc/runtime/timer/mod.rstest/js/web/timers/setTimeout.test.jstest/js/web/timers/timer-heap-gc-fixture.ts
|
When I opened this PR I wrote that I could not build a standalone JS reproducer for this race and doubted one exists. That was wrong. I had only traced the collector-thread writer, whose re-arms take the zero-delay imminent path and skip the heap. The writer that makes this reachable from ordinary code is
This push adds Both tests live in |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/web/timers/timer-heap-atomics-fixture.ts`:
- Around line 1-18: Trim the header comment in timer-heap-atomics-fixture.ts
down to at most 3 lines while preserving only the durable invariant. Keep the
essential purpose of the fixture around Atomics.waitAsync, Atomics.notify, and
the cross-thread timer-heap interaction, but remove the detailed call-chain,
crash-mode, and debugging explanation from the inline comment. Leave the deeper
rationale for the PR description and keep the comment style aligned with the
repo guideline.
In `@test/js/web/timers/timer-heap-race.test.ts`:
- Around line 25-30: The runFixture() helper is discarding child stderr, which
hides useful crash diagnostics in failure diffs. Update runFixture() so it
returns stderr alongside stdout, signal, and exitCode, and keep it in the
asserted result object with a non-strict matcher to preserve ASAN/assert output
without making the test brittle. Use the existing runFixture() flow in
timer-heap-race.test.ts so all crash-oriented fixture checks include stderr in
their assertions.
- Around line 1-6: Trim the oversized header comments in timer-heap-race.test.ts
to fit the repo’s 3-line inline-comment limit while preserving only the durable
invariant note. Shorten the top-of-file comment near the timer heap regression
tests and the other header block referenced by the review, keeping the essential
references to src/runtime/timer, All.lock, and WTFTimer behavior. Move any
longer rationale or debugging context out of the comment and into the PR
description instead.
🪄 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: be20181b-443f-409c-bfa8-8dd296e59856
📒 Files selected for processing (2)
test/js/web/timers/timer-heap-atomics-fixture.tstest/js/web/timers/timer-heap-race.test.ts
There was a problem hiding this comment.
I didn't find any issues, but this reworks lock acquisition and detach-state invariants on the cross-thread intrusive timer heap that schedules every runtime timer — the lock-ordering (acquire in pop_due_wtf_timer, release before re-entrant fire) and the new InHeap::None early-return in remove_lock_held are subtle enough to warrant a human pass.
Extended reasoning...
Overview
This PR fixes a data race in src/runtime/timer/mod.rs where All::get_timeout peeked and popped the intrusive pairing-heap without holding All.lock, while WTFTimer__update/WTFTimer__cancel mutate the same heap from JSC's collector thread under that lock. The fix factors the locked peek/pop into pop_due_wtf_timer (returning a new HeapHead enum), moves the in_heap = None / state = FIRED detach marking into TimerHeap::delete_min itself, removes a now-redundant unlocked state write in WTFTimer::fire, makes remove_lock_held tolerate an already-popped WTFTimer node, and adds a locked rearm_uv_timer wrapper for the Windows path. Three new test files (two fixtures + one test) exercise the GC and Atomics.waitAsync cross-thread paths.
Security risks
None identified. This is internal scheduler synchronization with no user-controlled input, auth, crypto, or external I/O surface.
Level of scrutiny
High. This is core runtime infrastructure: the heap that schedules every setTimeout/setInterval/setImmediate and every JSC RunLoop timer. The change involves cross-thread lock discipline on an intrusive data structure with raw-pointer node manipulation in unsafe Rust, and the correctness argument hinges on subtle ordering (lock released before fire() because the non-recursive Mutex would deadlock on re-entry; state/in_heap written under lock so a racing update sees a consistent view). The PR description is exceptionally thorough and the refactor mirrors the existing All::next pattern, but a wrong move here corrupts the heap silently in release builds. The author also notes the race cannot be deterministically reproduced from JS, so the new tests are regression guards on the code path rather than proof of the fix.
Other factors
- The
remove_lock_heldchange replaces a harddebug_assert!(false)with a tag-narrowed assert plus early return — this is a behavioural relaxation that a human should confirm cannot mask a real double-remove on non-WTFTimer paths. TimerHeap::delete_minnow writesstate = FIREDfor all callers includingFakeTimers; the author verified the fake-timer suite passes, but it broadens the function's contract.- CI shows musl build failures on the latest commit (likely infra given the
ci: retriggercommit, but unconfirmed). - No prior review from me on this PR; the only resolved thread (CodeRabbit's stderr-assertion nit) was addressed in 664034a.
|
Independent corroboration: a fuzz run against current main (ffea69a) hit this same race as an AddressSanitizer heap-use-after-free, a READ of size 8 of timer-heap memory on a Worker thread, under an None of it changes the fix. It does settle the question of whether the debug assertion was just conservative: the unlocked pop reads freed memory in release builds too. |
There was a problem hiding this comment.
Both earlier nits are addressed and the automated pass found nothing further, but this restructures locking on the event loop's per-tick timer heap (cross-thread WTFTimer races, fire-outside-the-lock, the delete_min state contract, and the relaxed remove_lock_held arm), so it needs a maintainer's eyes.
Extended reasoning...
Overview
This PR fixes a data race in All::get_timeout (src/runtime/timer/mod.rs), the event loop's pre-poll pass that computes the epoll/kqueue timeout and fires due WTFTimers. Before, it peeked/popped the intrusive pairing heap without All.lock, while WTFTimer__cancel / WTFTimer__update mutate the same heap from other threads (Atomics.notify, JSC's collector). The fix:
- moves the locked peek/pop into a new
pop_due_wtf_timer(&mut self) -> HeapHeadhelper and fires outside the lock (the Mutex is non-recursive and firing re-entersAll::update/remove); - has
TimerHeap::delete_minmark the popped nodein_heap = None/state = FIREDunder the lock, and removes the now-redundant (and racy)state = FIREDwrite fromWTFTimer::fire; - makes
remove_lock_held'sInHeap::Nonearm a no-op for the WTFTimer tag (the only tag whosestate == ACTIVEgate runs off-thread and can lose the race to the pop), keeping the debug assertion for every other tag; - wraps the Windows
on_uv_timerre-arm peek in the same lock viarearm_uv_timer; - drops a now-dead
in_heap = Nonewrite inFakeTimers::clear(and itsInHeapimport); - adds two stress fixtures and a regression test (
timer-heap-race.test.ts).
Security risks
None in the conventional sense (no auth/crypto/input parsing). The change is memory-safety relevant: the bug it fixes is a real heap-use-after-free in release builds. The fix narrows unsafe surface (one fewer net unsafe block per the author's accounting) and the new raw-pointer writes in delete_min are on a node the function just unlinked under the lock.
Level of scrutiny
High. get_timeout runs on every event-loop iteration of every Bun process; the change touches lock ordering, cross-thread state-machine semantics (ACTIVE/FIRED/CANCELLED), and intrusive-heap invariants shared with All::next, drain_timers, and the fake-timer pop sites. The argument for removing WTFTimer::fire's state = FIRED write (it would clobber a concurrent re-arm's ACTIVE between the pop releasing the lock and fire() running) is sound on inspection but is exactly the kind of reasoning a maintainer who owns this subsystem should confirm. Likewise the relaxed InHeap::None arm in remove_lock_held: returning early without writing state = CANCELLED is justified in the comment, but it changes a long-standing invariant.
Other factors
- The PR is bot-authored with no human review yet on the timeline.
- CI shows musl build failures on the latest commit (likely infra, but unverified).
- The author ran the timer/abort/fake-timer suites and the new fixture both with and without the fix; the Atomics fixture reproduces the crash reliably on an unfixed debug build.
- All prior review feedback (CodeRabbit's stderr/comment-trim nits and my two inline nits) has been addressed; nothing outstanding.
- The behavior of
get_timeoutis preserved for the non-WTFTimer-due / not-yet-due / empty-heap cases via theHeapHeadenum, including thespec = 0branch when a non-WTFTimer is already due.
Given the subsystem and the subtlety of the lock/state reasoning, this should not be auto-approved.
|
Final CI status, since the remaining red is not actionable from this diff. Everything that exercises this change is green: every The three failures on the final build (67198) are unrelated to the diff:
Nothing further to push from my side. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
This is terrible code. Think about how to make the data structure better and try again.
|
Done in f52d292. You were right; naming the actual defect made the fix smaller. The defect was the data structure: Now the mutex owns the heaps.
Nothing takes or releases the timer lock by hand anywhere, and the diff against main is net negative (431 deletions, 330 insertions). Re-verified both ways: with |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/test_runner/timers/FakeTimers.rs`:
- Around line 122-126: The invariant comment in FakeTimers should be shortened
to fit the repo’s 3-line limit without losing the key ownership and locking
note. Compress the documentation on the drain path around the heap guard and
`TimerObjectInternals::release_heap_pin`, keeping only the essential point that
the caller releases the heap ref and JS pin after the guard drops because
`timer::All::remove` re-locks the heaps. Apply the same trimming to the other
long comment block referenced in the review so both stay within the
comment-length guideline.
In `@src/runtime/timer/mod.rs`:
- Around line 796-803: WTFTimer::update currently re-arms the timer through
ensure_uv_timer(), which can lazily use TLS even when called off the owning JS
thread; replace that path so insert/update do not touch the libuv handle from a
foreign thread. Route the rearm through the owning JS thread or a thread-safe
wakeup/dispatch path, and keep the loop/handle access constrained to the timer’s
registered loop rather than relying on debug_assert! affinity checks.
🪄 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: cd98da5a-5dd6-40d2-9360-430a5d5ccf87
📒 Files selected for processing (3)
src/runtime/test_runner/timers/FakeTimers.rssrc/runtime/timer/mod.rssrc/runtime/timer/timer_object_internals.rs
|
Can we simplify this by not relying on the timer heap for the WTF Timer? It can be special cased. All other timers are same thread. This timer shouldn't involve the fake timers list either. |
robobun
changed the title
|
Done in 823e4ff.
I rewrote the title and description, since they described the previous approach. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Delete all the code comments. Simplify further.
|
Done in 3bab0e3. Every comment the branch added is gone. The one-line SAFETY notes on unsafe blocks stay because the workspace sets Two more deletions fell out:
Net against the previous revision: 37 insertions, 249 deletions. |
|
CI status for f0fa434 (Buildkite build 73001, finished): clippy, Format, and Lint are green. The only failing test is Older buildsBuild 71634 (f8fe07b, before the second rebase) had one failure: |
|
Independent confirmation that this race is also the cause of an open high-volume timer crash in Sentry: BUN-3K4E, 205 events since 2026-06-18, 27 in the last 24 hours, macOS aarch64 dominant, still occurring on the newest 1.4.0 canaries. Its decoded stack is: i.e. a It does not survive this PR's heap corruption. Against an otherwise unmodified assert!(
s.this_value.get().is_strong() || !s.this_value.get().is_not_empty(),
"BUG(fire-entry): JS timer fired while this_value was an unprotected weak JSValue (id={}, kind={}, state={})",
...
);and commented out the two heap
So this change also fixes BUN-3K4E. A line in the description would let that Sentry issue be resolved on merge. One nit on the Sentry side: it marks this as "not present on 1.3.x", but the Zig Two smaller things this surfaced, neither of which should hold this up:
|
3bab0e3 to
b406f31
Compare
WTF::RunLoop::TimerBase is the only timer WebKit starts and stops from threads other than the owning run loop's: Atomics.notify on thread B re-arms thread A's GC JSRunLoopTimer and cancels thread A's Atomics.waitAsync timeout, and ~ArrayBufferContents does the same from whichever thread drops the last SharedArrayBuffer reference. Keeping those nodes in the same pairing heap as setTimeout was the only reason All.lock existed, and get_timeout's pre-poll pass peeked and popped that shared heap without taking it, racing the locked Intrusive::remove from those other threads and corrupting the heap links. Evict the one cross-thread client instead of locking everything: - All.wtf_timers (Guarded<TimerHeap>) holds only WTFTimer nodes, reached through All::wtf_arm / All::wtf_disarm from any thread. - All.lock is deleted. insert/remove/update, the regular heap, the fake timers, and the epoch are single threaded again (asserted), so setTimeout no longer takes a mutex per call. The fake-timer lock helper and its assert machinery existed only because of that shared lock, so they are deleted too. - get_timeout no longer pops and fires inline: it drains the due WTF timers through their own mutex, then returns min(wtf, regular, quic). - drain_timers drains the WTF heap first, keeping the firing paths on an inactive loop and on Windows. - WTFTimer.lock (the per-instance mutex) is deleted; wtf_timers owns everything it guarded, and update never took it anyway. - ensure_uv_timer folds both heaps into the libuv deadline and is now only reachable from the owning thread.
The braces were there to scope a guard that no longer exists; nothing inside has a drop, and the three vectors it fills are declared outside.
f8fe07b to
f0fa434
Compare
There was a problem hiding this comment.
No issues found in this pass, but this restructures cross-thread synchronization for the event-loop timer heap (deletes All.lock, adds wtf_timers: Guarded<TimerHeap>, rewrites get_timeout/drain_timers/next) — worth a human look at the final shape.
Reviewed: the wtf_arm/wtf_disarm guard scope vs. the unlocked fire() — the residual deinit-during-fire window is pre-existing and currently unreachable per the dispatchAfter self-cycle analysis in the thread.
Checked ensure_uv_timer now folds both heaps and is only reached on the owning thread; the pre-existing cross-thread TLS hazard is not widened.
Checked in_heap is never set for WTF nodes (wtf_arm doesn't write it) and remove() debug-asserts on WTFTimer tag, so the two heaps cannot cross.
Verified the FakeTimers lock-helper deletion is safe: every remaining access is JS-thread-only, asserted via assert_js_thread().
Extended reasoning...
Overview
This PR fixes a cross-thread data race on the event-loop timer heap by moving WTFTimer (JSC's RunLoop::TimerBase backing — GC scheduler timers and Atomics.waitAsync timeouts) out of the shared All.timers pairing heap into its own Guarded<TimerHeap>. The shared All.lock mutex and per-instance WTFTimer.lock are deleted; the regular heap, fake-timer heap, and epoch become single-threaded again with a debug_assert! on the owning thread id. get_timeout is rewritten from a pop-and-fire loop to a drain-WTF-then-min(wtf, regular, quic) computation. FakeTimers loses its timers_lock_guard()/assert_locked() machinery. ~486 lines changed in mod.rs, ~180 net-removed in FakeTimers.rs, plus two new stress-test fixtures.
Security risks
None in the traditional sense — no user-input parsing, auth, or crypto. The risk class here is memory safety: this is unsafe Rust manipulating an intrusive pairing heap across threads via raw pointers, and the bug it fixes is a heap-use-after-free reachable from ordinary JS (Atomics.waitAsync + Atomics.notify). The fix's own correctness depends on the invariant that state == ACTIVE ⇔ linked in wtf_timers, enforced under the guard, and that the guard is dropped before every re-entrant fire().
Level of scrutiny
High. This is core event-loop concurrency infrastructure with pervasive unsafe, and it went through three architectural revisions (lock get_timeout → Guarded<Heaps> → separate heap). The final design was chosen at a maintainer's explicit direction, but the maintainer has not yet reviewed the implementation of that direction. Several subtle concerns were raised and analyzed in the thread (the residual deinit-during-fire window, the newly-unlocked seconds_until_timer read, the wtf_now reuse in drain_timers) — each has a reasoned response, but they are the kind of tradeoff a maintainer should sign off on rather than a bot.
Other factors
The PR has a working regression test that fails on main (SIGABRT / ASan UAF) and passes with the fix, plus a Sentry crash (BUN-3K4E, 205 events) traced to the same corruption. CI is green modulo unrelated infrastructure failures. All prior inline review comments (mine and CodeRabbit's) are resolved. The bug-hunting system found nothing this run. Net diff is negative (-437/+448 with substantial deletions of lock scaffolding). Still, the combination of scope, unsafe density, and cross-thread lifetime reasoning puts this well outside the auto-approve bar.
|
Heads up: the |
* upstream/main: (57 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (70 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ... # Conflicts: # test/js/bun/websocket/websocket-server.test.ts
…loop's last tick (#34293) `test/js/web/timers/timer-heap-race.test.ts` went red on the x64-asan lane after #33131 landed (build 73570): ``` SUMMARY: AddressSanitizer: 1280 byte(s) leaked in 40 allocation(s). ConcurrentTask::create src/event_loop/ConcurrentTask.rs:319 Bun__queueJSCDeferredWorkTaskConcurrently src/jsc/JSCScheduler.rs:64 Bun::JSCTaskScheduler::onScheduleWorkSoon JSCTaskScheduler.cpp:54 JSC::DeferredWorkTimer::scheduleWorkSoon DeferredWorkTimer.cpp:235 JSC::Waiter::cancelAndClear WaiterListManager.cpp:298 JSC::WaiterListManager::unregister(JSC::VM*) WaiterListManager.cpp:310 JSC::VM::~VM() VM.cpp:591 WebWorker__teardownJSCVM Worker.cpp:676 ``` The leak is pre-existing; #33131's new cross-thread `Atomics.waitAsync` fixture is the first test that terminates a worker with pending async waiters on a SharedArrayBuffer the parent keeps alive. ### Cause Worker `shutdown()` drains the concurrent task queue once (`release_queued_tasks_for_shutdown`), then calls `WebWorker__teardownJSCVM`, which ends in `~VM()`. `~VM()` runs `WaiterListManager::unregister(this)`, and for every pending `Atomics.waitAsync` ticket that reaches `Waiter::cancelAndClear` → `DeferredWorkTimer::scheduleWorkSoon` → our `onScheduleWorkSoon` hook. The hook allocates a `JSCDeferredWorkTask` and a `ConcurrentTask` and enqueues them into the worker's concurrent queue, which was just drained for the last time. When the worker's `VirtualMachine` box is raw-`dealloc`'d, both become unreachable. The same path is reachable from the final `collectNow` via `JSFinalizationRegistry::finalizeUnconditionally`. A second, narrower leak: a cross-thread `Atomics.notify` that lands between the worker's last tick and `teardownJSCVM` enqueues a `JSCDeferredWorkTask` that `release_queued_tasks_for_shutdown` forwards into `self.tasks`. `__bun_release_task_at_shutdown` had no arm for that tag, so it was re-queued, and `EventLoop::deinit` re-queued it once more into a freshly allocated `LinearFifo` buffer that leaked on worker dealloc. ### Fix - `JSCTaskScheduler` gets an `std::atomic<bool> m_isShuttingDown`, set at the start of `WebWorker__teardownJSCVM` and `Zig__GlobalObject__destructOnExit` (mirroring the existing `ctx->markTerminating()`). `onScheduleWorkSoon` and `onAddPendingWork` drop the work once it's set; `onScheduleWorkSoon` also balances the `onAddPendingWork` ref via `onCancelPendingWork`. - `__bun_release_task_at_shutdown` gains a `JSCDeferredWorkTask` arm that deletes the job via a new `Bun__deleteDeferredWorkTask` FFI. This runs before JSC teardown, so `~Ref<TicketData>` and the captured `Task` lambda release against a live VM. ### Test `timer-heap-atomics-teardown-fixture.ts` terminates a worker with 32 pending `Atomics.waitAsync` tickets on a parent-owned SAB, a few of them notified cross-thread first, under `detect_leaks=1`. Without the fix LSan reports ~29 `ConcurrentTask` allocations from `WaiterListManager::unregister` and SIGABRTs; with it the fixture exits clean. The original race fixture is also 0/10 failures under the CI env (was ~1/5 on a debug build and 1/1 on release-asan). ### Overlap with #34270 \#34270 adds the same `m_isShuttingDown`/`onScheduleWorkSoon` gate (there named `m_isTerminating`) while fixing a separate `FinalizationRegistry` assert, but without the `__bun_release_task_at_shutdown` arm the race fixture still fails ~1/10 on that branch. Whichever lands first, the other is a small rebase over `JSCTaskScheduler.{h,cpp}`. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 3 · 9 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 1 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/timers/timer-heap-race.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (4d9b926) test/js/web/timers/timer-heap-race.test.ts: (pass) timer heap survives cross-thread Atomics.waitAsync timeout cancellation [3806.16ms] (pass) timer heap stays consistent while GC re-arms the RunLoop timer [2408.11ms] 51 | ASAN_OPTIONS: "allow_user_segv_handler=1:disable_coredump=0:detect_leaks=1:abort_on_error=1", 52 | LSAN_OPTIONS: `malloc_context_size=30:print_suppressions=0:suppressions=${path.join(import.meta.dir, "..", "..", "..", "leaksan.supp")}`, 53 | }); 54 | // LSan writes its leak report to stderr and SIGABRTs; stdout holds the 55 | // fixture's own OK line either way, so assert exitCode/signal explicitly. 56 | expect({ stdout, stderr, signal, exitCode }).toEqual({ ... (truncated) release without fix: 2 skipped bun test v1.4.0-canary.1 (1498d7b) test/js/web/timers/timer-heap-race.test.ts: (pass) timer heap survives cross-thread Atomics.waitAsync timeout cancellation [3042.35ms] (skip) timer heap stays consistent while GC re-arms the RunLoop timer (skip) terminating a worker with pending Atomics.waitAsync tickets does not leak deferred-work tasks 1 pass 2 skip 0 fail 1 expect() calls Ran 3 tests across 1 file. [3.20s] __F:0:S:2 ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/timers/timer-heap-race.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (4d9b926) test/js/web/timers/timer-heap-race.test.ts: (pass) timer heap survives cross-thread Atomics.waitAsync timeout cancellation [3805.27ms] (pass) timer heap stays consistent while GC re-arms the RunLoop timer [2481.63ms] (pass) terminating a worker with pending Atomics.waitAsync tickets does not leak deferred-work tasks [7403.00ms] 3 pass 0 fail 3 expect() calls Ran 3 tests across 1 file. [15.73s] __F:0:S:0 release with fix: 2 skipped $ bun scripts/build.ts --profile=release info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 4d9b926 features (none) 22 deps, 106 codegen, 1168 objects in 891ms ninja: Entering directory `/workspace/bun/build/release' [1/1231] install /workspace/bun bun install v1.4.0-canary.1 (1498d7b) Checked 124 installs across 170 packages (no changes) [13.00ms] [2/1231] install /workspace/bun/packages/bun-error bun install v1.4.0-canary.1 (1498d7b) Checked 1 install across 2 packages (no changes) [3.00ms] [3/1231] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (1498d7b) Checked 129 installs across 147 packages (no changes) [11.00ms] [4/1231] gen ErrorCode+*.h [5/1231] fetch zlib [zlib] up to date [6/1231] gen bindgenv2 [7/1231] fetch picohttpparser [picohttpparser] up to date [8/1231] gen .bind ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/VirtualMachine.rs | 8 +++ src/jsc/bindings/JSCTaskScheduler.cpp | 77 ++++++++++++++++++---- src/jsc/bindings/JSCTaskScheduler.h | 13 ++++ src/jsc/bindings/ZigGlobalObject.cpp | 2 + src/jsc/bindings/webcore/Worker.cpp | 5 ++ src/jsc/web_worker.rs | 8 +++ src/runtime/dispatch.rs | 15 +++++ .../timers/timer-heap-atomics-teardown-fixture.ts | 32 +++++++++ test/js/web/timers/timer-heap-race.test.ts | 25 ++++++- 9 files changed, 171 insertions(+), 14 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 3 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/VirtualMachine.rs 5 2 0 src/jsc/bindings/JSCTaskScheduler.cpp 7 10 0 src/jsc/bindings/JSCTaskScheduler.h 5 4 0 src/jsc/bindings/ZigGlobalObject.cpp 1 1 0 src/jsc/bindings/webcore/Worker.cpp 1 1 0 src/jsc/web_worker.rs 5 4 0 src/runtime/dispatch.rs 2 1 0 …st/js/web/timers/timer-heap-atomics-teardown-fixture.ts 0 2 0 test/js/web/timers/timer-heap-race.test.ts 2 3 0 ``` </details> <!-- robobun:evidence:end -->
|
The first test in |
2 participants
WTF::RunLoop::TimerBase(JSC's GC scheduler timers, and the timer behind anAtomics.waitAsynctimeout) is started and stopped from threads other than the one that owns its run loop:Atomics.notifyon thread B re-arms thread A'sJSRunLoopTimerand cancels thread A'swaitAsynctimeout, and~ArrayBufferContentsdoes the same from whichever thread drops the lastSharedArrayBufferreference.Those timers shared one pairing heap (
All.timers) withsetTimeoutand every other same-thread timer.All.lockexisted only because of them, andget_timeout's pre-poll pass peeked and popped that shared heap without taking it, racing the lockedIntrusive::removefrom the other threads. In a debug build that tripsand in a release build the check compiles out, so
removewalks the corrupted links silently.Fix
Rather than locking the shared heap, take the one cross-thread client out of it:
All.wtf_timers(aGuarded<TimerHeap>) holds onlyWTFTimernodes, reached throughAll::wtf_armandAll::wtf_disarmfrom any thread.All.lockis deleted.insert,remove,update, the regular heap, the fake timers, and the epoch are single threaded again, asserted with adebug_assert!on the owning thread id, sosetTimeoutno longer takes a mutex per call. The fake-timer lock helper and itsassert_lockedmachinery existed only because of that shared lock, so they are deleted too.get_timeoutno longer pops and fires inline. It drains the dueWTFTimers through their own mutex (dropping the guard across eachfire, which can synchronously re-enter it), then returnsmin(wtf, regular, quic).drain_timersdrains theWTFTimerheap first, preserving both existing firing paths: the pre-poll one only runs when the uws loop is active, and on Windowsdrain_timersruns fromon_uv_timer.WTFTimer.lock(the per-instance mutex) is deleted;wtf_timersowns everything it guarded, andupdatenever took it anyway.ensure_uv_timerfolds both heaps into the libuv deadline and is now only reachable from the owning thread, which also removes the cross-thread TLS hazard it had.WTFTimernever enters the fake-timer heap. Its tag already returnedfalsefromallow_fake_timers(), but it can no longer reach that branch at all.Reproducer
test/js/web/timers/timer-heap-race.test.tswithtimer-heap-atomics-fixture.ts: four threads each arm batches of shortAtomics.waitAsynctimeouts while the othersAtomics.notifythem, alongsidesetTimeoutchurn. Under a debug build ofmainthis aborts within a few seconds with the assertion above, or with an ASanheap-use-after-freeinIntrusive::removeatsrc/io/heap.rs:178. With this change it runs to completion.A second fixture drives
Bun.gc(true)in asetTimeoutloop so the per-VMJSRunLoopTimeris re-armed and popped repeatedly on the owning thread.Rebased onto #33359, #33623, #33896, #34009
Squashed to one commit and rebased; the earlier commits in the branch history were the two rejected approaches (locking
get_timeout, thenGuarded<Heaps>).One conflict had semantic content: #33896 switched the regular heap's clock reads in
get_timeoutandnextfromAllowMockedTimetoForceRealTimebecause every node that lands there is armed in real-time units.WTFTimeris one of those tags, and its new separate heap is the same case, so theForceRealTimechange is also applied indrain_due_wtf_timers. The #33359 hunk (skipuv_timer.startwhen already due sooner) and the #33623 hunks (set_wall_ms,clear_wall,Bun__FakeTimers__setSystemTime) applied without semantic interaction.#34009 added a
now_out: &mut Option<Timespec>out-parameter toget_timeoutso its caller can reuse the monotonic clock read; the lazymaybe_nowin the rewritten body becomes a reborrow of that out-parameter, anddrain_due_wtf_timersfills it the same way the old loop did.Verification
Three
setTimeout doesn't leak when X is called inside its own callbacktests intest/js/web/timers/fail under debug + ASan on this machine, and did so identically with an unmodifiedsrc/at the previous merge base: their fixtures widen the RSS threshold only whenprocess.execPath.includes("bun-asan"), which is never true for thebun-debugbinary name.[review] gate passed · iteration 11 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 11
evidence per changed file