timer: drive the real heap from the real clock under fake timers - #33896
Conversation
All::drain_timers and All::get_timeout compared self.timers (the opt-out-of-fake-timers heap: GC controller, WTFTimer, bun:test timeouts, StatWatcherScheduler) against Timespec::now(AllowMockedTime). Every node in that heap is armed with ForceRealTime, so once jest.advanceTimersByTime pushed the mocked clock past CLOCK_MONOTONIC uptime the nodes looked permanently overdue, and any that re-armed on fire (GcRepeating, WTFTimer) were re-inserted still 'overdue' and the drain loop spun at 100% CPU. The GcRepeating case is new to #33359 (the GC timers used to be kernel timerfds and fired on real time regardless); the WTFTimer case was latent before that. StatWatcherScheduler is the one allow_fake_timers()==false tag that still armed with AllowMockedTime; flip it so its deadlines are in the same units as the heap they live in.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
I don't think this closes #26037. That report is against 1.3.6 (January), six months before #33359 put the GC timers on the heap, so the spin fixed here can't be what it hit. The repro there never calls
|
|
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)
WalkthroughChangesTimer deadline computation and due-time checks now use real-time clocks when mocked timers are active. New subprocess-based regression tests verify that Timer clock behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Jarred-Sumner
left a comment
There was a problem hiding this comment.
@robobun there hsould be two timer heaps - fake timer heap and real timer heap. the timers that support mocking can be moved around. the timers that don't support mocking cannot be moved. thats how it should work.
|
That's what this change lands on. The two heaps are already there:
When fake timers aren't installed the mode argument is a no-op, so the only behavior change is inside a One thing this PR doesn't touch: |
There was a problem hiding this comment.
Thanks for dropping the tmpdir write — the fixture is clean now. No bugs found, but since this touches drain_timers/get_timeout on every event-loop tick, I'd like a human to confirm the invariant that everything in self.timers is armed in real-clock units.
Extended reasoning...
Overview
Three enum-variant swaps (TimespecMockMode::AllowMockedTime → ForceRealTime) across two files:
src/runtime/timer/mod.rs:All::get_timeoutandAll::next(the pop helper fordrain_timers) now read the real monotonic clock when comparing againstself.timers.src/runtime/node/node_fs_stat_watcher.rs:StatWatcherScheduler::set_timernow arms its deadline withForceRealTime, matching the heap it lives in.
Plus a subprocess regression test that pushes mocked time ~11 years past uptime and awaits real I/O; pre-fix the child spins at 100% CPU.
Security risks
None. No untrusted input, no auth/crypto/permissions surface. Pure internal clock-source selection.
Level of scrutiny
High. drain_timers and get_timeout run on every event-loop tick for every Bun process. The change is provably a no-op when fake timers are not installed (verified in src/bun_core/util.rs:5282-5285 — ForceRealTime skips the mock_time::get() branch and both fall through to the real monotonic read). Inside a useFakeTimers() window, correctness depends on the invariant that every node in self.timers was armed with a real-clock deadline. The PR description enumerates the arm sites (GC controller, WTFTimer, bun:test timeouts, EventLoopDelayMonitor, StatWatcherScheduler) and this PR fixes the one outlier, but that invariant spans multiple files and the fake-timer routing in insert_lock_held — worth a human familiar with #33359 confirming.
The author also flags a deliberate non-change: mockable timers armed before useFakeTimers() stay in the real heap and now continue on real time (previously compared against mocked time). Argued to match Jest, but it's a behavior change a maintainer should sign off on.
Other factors
- Bug-hunting system found nothing.
- My prior inline comment (tmpdir leak in the fixture) was addressed in 53c511c — the fixture now reads
import.meta.pathand writes nothing. - Verification is thorough:
fake-timers/(61 pass),node/timers/(20 pass),fs.watchFile.test.ts(9 pass),rust:check-all(10/10). - Test follows harness conventions (concurrent pipe drain,
signalCodenull-check for hang-guard, stdout asserted before exitCode). - No CODEOWNERS entry for these paths.
…#33131) `WTF::RunLoop::TimerBase` (JSC's GC scheduler timers, and the timer behind an `Atomics.waitAsync` timeout) is started and stopped from threads other than the one that owns its run loop: `Atomics.notify` on thread B re-arms thread A's `JSRunLoopTimer` and cancels thread A's `waitAsync` timeout, and `~ArrayBufferContents` does the same from whichever thread drops the last `SharedArrayBuffer` reference. Those timers shared one pairing heap (`All.timers`) with `setTimeout` and every other same-thread timer. `All.lock` existed only because of them, and `get_timeout`'s pre-poll pass peeked and popped that shared heap without taking it, racing the locked `Intrusive::remove` from the other threads. In a debug build that trips ``` assertion failed: self.root == v (src/io/heap.rs:165, Intrusive::remove) ``` and in a release build the check compiles out, so `remove` walks the corrupted links silently. ### Fix Rather than locking the shared heap, take the one cross-thread client out of it: - `All.wtf_timers` (a `Guarded<TimerHeap>`) holds only `WTFTimer` nodes, reached through `All::wtf_arm` and `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 with a `debug_assert!` on the owning thread id, so `setTimeout` no longer takes a mutex per call. The fake-timer lock helper and its `assert_locked` 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 `WTFTimer`s through their own mutex (dropping the guard across each `fire`, which can synchronously re-enter it), then returns `min(wtf, regular, quic)`. - `drain_timers` drains the `WTFTimer` heap first, preserving both existing firing paths: the pre-poll one only runs when the uws loop is active, and on Windows `drain_timers` runs from `on_uv_timer`. - `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, which also removes the cross-thread TLS hazard it had. `WTFTimer` never enters the fake-timer heap. Its tag already returned `false` from `allow_fake_timers()`, but it can no longer reach that branch at all. ### Reproducer `test/js/web/timers/timer-heap-race.test.ts` with `timer-heap-atomics-fixture.ts`: four threads each arm batches of short `Atomics.waitAsync` timeouts while the others `Atomics.notify` them, alongside `setTimeout` churn. Under a debug build of `main` this aborts within a few seconds with the assertion above, or with an ASan `heap-use-after-free` in `Intrusive::remove` at `src/io/heap.rs:178`. With this change it runs to completion. A second fixture drives `Bun.gc(true)` in a `setTimeout` loop so the per-VM `JSRunLoopTimer` is 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`, then `Guarded<Heaps>`). One conflict had semantic content: #33896 switched the regular heap's clock reads in `get_timeout` and `next` from `AllowMockedTime` to `ForceRealTime` because every node that lands there is armed in real-time units. `WTFTimer` is one of those tags, and its new separate heap is the same case, so the `ForceRealTime` change is also applied in `drain_due_wtf_timers`. The #33359 hunk (skip `uv_timer.start` when 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 to `get_timeout` so its caller can reuse the monotonic clock read; the lazy `maybe_now` in the rewritten body becomes a reborrow of that out-parameter, and `drain_due_wtf_timers` fills it the same way the old loop did. ### Verification ``` bun bd test test/js/web/timers/timer-heap-race.test.ts # 2 pass bun bd test test/js/bun/test/test-timers.test.ts # #33896's suite, all pass bun bd test test/js/bun/test/fake-timers/fake-timers.test.ts # #33623's suite, all pass ``` Three `setTimeout doesn't leak when X is called inside its own callback` tests in `test/js/web/timers/` fail under debug + ASan on this machine, and did so identically with an unmodified `src/` at the previous merge base: their fixtures widen the RSS threshold only when `process.execPath.includes("bun-asan")`, which is never true for the `bun-debug` binary name. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 11 · 10 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 (f0fa434) test/js/web/timers/timer-heap-race.test.ts: 19 | 20 | return { stdout, stderr, signal: proc.signalCode, exitCode }; 21 | } 22 | 23 | it("timer heap survives cross-thread Atomics.waitAsync timeout cancellation", async () => { 24 | expect(await runFixture("timer-heap-atomics-fixture.ts")).toEqual({ ^ error: expect(received).toEqual(expected) { - "exitCode": 0, - "signal": null, - "stderr": Any<String>, + "exitCode": 134, + "signal": "SIGABRT", + "stderr": + "============================================================ + Bun Debug v1.4.0 (f0fa434) Linux x64 + Linux Kernel v6.17.0 | glibc v2.41 + CPU: sse42 popcnt avx avx2 avx51 ... (truncated) release without fix: 1 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 [3036.31ms] (skip) timer heap stays consistent while GC re-arms the RunLoop timer 1 pass 1 skip 0 fail 1 expect() calls Ran 2 tests across 1 file. [3.21s] __F:0:S:1 ``` </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 (f0fa434) test/js/web/timers/timer-heap-race.test.ts: (pass) timer heap survives cross-thread Atomics.waitAsync timeout cancellation [3787.70ms] (pass) timer heap stays consistent while GC re-arms the RunLoop timer [2478.37ms] 2 pass 0 fail 2 expect() calls Ran 2 tests across 1 file. [8.23s] __F:0:S:0 release with fix: 1 skipped $ bun scripts/build.ts --profile=release info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu error: could not download file from 'https://static.rust-lang.org/dist/2026-05-06/channel-rust-nightly.toml' to '/root/.rustup/tmp/pg_7uy9hcvyywudd_file.toml': error downloading file: error sending request for url (https://static.rust-lang.org/dist/2026-05-06/channel-rust-nightly.toml): client error (Connect): tls handshake eof [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision f0fa434 features (none) 22 deps, 106 codegen, 1168 objects in 667ms 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) [10.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) [1.00ms] [3/1231] gen bindgenv2 [4/1231] install /workspace/bun/src/node-fallbacks bun install v1.4.0-canary.1 (1498d7b) Checked 129 installs across 147 packages (no changes) [ ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/hw_exports.rs | 4 +- src/runtime/jsc_hooks.rs | 12 +- src/runtime/test_runner/timers/FakeTimers.rs | 183 ++-------- src/runtime/timer/Timer.rs | 6 +- src/runtime/timer/WTFTimer.rs | 58 +--- src/runtime/timer/mod.rs | 414 ++++++++++++----------- src/runtime/timer/timer_object_internals.rs | 8 +- test/js/web/timers/timer-heap-atomics-fixture.ts | 68 ++++ test/js/web/timers/timer-heap-gc-fixture.ts | 17 + test/js/web/timers/timer-heap-race.test.ts | 43 +++ 10 files changed, 411 insertions(+), 402 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 11 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/hw_exports.rs 2 3 0 src/runtime/jsc_hooks.rs 2 2 0 src/runtime/test_runner/timers/FakeTimers.rs 9 16 0 src/runtime/timer/Timer.rs 1 1 0 src/runtime/timer/WTFTimer.rs 3 13 0 src/runtime/timer/mod.rs 24 59 0 src/runtime/timer/timer_object_internals.rs 4 3 0 test/js/web/timers/timer-heap-atomics-fixture.ts 2 4 0 test/js/web/timers/timer-heap-gc-fixture.ts 2 3 0 test/js/web/timers/timer-heap-race.test.ts 3 6 0 ``` </details> <!-- robobun:evidence:end -->
Follow-up to #33359 (review).
Repro
Any
jest.advanceTimersByTimethat pushes the mocked monotonic clock past the machine'sCLOCK_MONOTONICuptime, followed by a real I/O await, spinsAll::drain_timersforever. Deterministic on a fresh CI container; on a developer machine it needs an advance larger than uptime.Cause
All::drain_timersandAll::get_timeoutcomparedself.timersagainstTimespec::now(AllowMockedTime). That heap holds onlyallow_fake_timers()==falsenodes (GC controller,WTFTimer,bun:testtimeouts,StatWatcherScheduler) plus anything armed beforeuseFakeTimers(), and every one of those arms its deadline withForceRealTime. Once the mocked clock exceeds real uptime, every node looks overdue; any that re-arm on fire (GcRepeatingunconditionally,WTFTimerwhen JSC's GC scheduler has more work) are re-inserted atreal_uptime + interval, still less than the cached mockednow, and the drain loop never returns.The
GcRepeatingcase is new to #33359 (the GC timers were kernel timerfds before and fired on real time regardless of fake timers). TheWTFTimercase was latent before that. Reproduces withBUN_GC_TIMER_DISABLE=1too, so the fix has to be at the comparison layer, not inGarbageCollectionController::arm().Fix
drain_timers::next()andget_timeoutreadForceRealTimeforself.timers. The fake heap is already walked separately byadvanceTimersByTime, so the two heaps are now ticked against their own clocks. No behavior change when fake timers are not installed (AllowMockedTime == ForceRealTimethen).StatWatcherScheduler::set_timerwas the oneallow_fake_timers()==falsetag that still armed withAllowMockedTime; flipped toForceRealTimeso its deadlines are in the same units as the heap they live in. The Windows path (ensure_uv_timer) already usedForceRealTime.Verification
test/js/bun/test/test-timers.test.tsspawns abun testchild that advances mocked time by ~11 years and then awaits file I/O inside the fake-timer window. Without this change the child spins and is killed by the 20s spawn timeout; with it the child exits in ~35ms. Also rantest/js/bun/test/fake-timers/(61 pass),test/js/node/timers/(20 pass),test/js/node/watch/fs.watchFile.test.ts(9 pass), andrust:check-all(10/10).