Conversation
jest.runAllTimers() drained the fake-timer heap with an unbounded loop, so any self-re-arming timer (setInterval, recursive setTimeout, Bun.cron since it started honoring the mocked clock in #33623) made it spin at 100% CPU forever. Jest bounds this drain at timerLimit (default 100000) and throws 'Aborting after running N timers, assuming an infinite loop!' when timers still remain. This matches that behavior and wires the limit through useFakeTimers({ timerLimit }). The cron.mdx fake-timers section is updated to stop pointing users at runAllTimers() for cron jobs.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
Updated 5:41 AM PT - Jul 16th, 2026
❌ @robobun, your commit acb2bf2 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34325That installs a local version of the PR into your bun-34325 --bun |
|
Status: diff is ready; needs maintainer review/merge.
Repro on main (hangs at 100% CPU, never returns): bun -e 'const {jest}=require("bun:test"); jest.useFakeTimers(); setInterval(()=>{},10); jest.runAllTimers()' |
![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 any correctness issues beyond the cosmetic nit already flagged inline. Deferring because this adds user-facing API surface (timerLimit option) and changes runAllTimers() behavior from hang → throw, so it's worth a maintainer glance even though it's straight Jest-compat.
Checked: the bounded loop reads timer_limit once before draining and re-peeks after the loop, so a chain of exactly limit timers does not throw (covered by test). The unsafe per-thread timer::All accesses follow the same one-statement-borrow pattern as execute_until/execute_next. options_value.get(global, "timerLimit")? returns None for undefined, so { timerLimit: undefined } correctly falls through to the default.
Extended reasoning...
Overview
Caps jest.runAllTimers() at timerLimit iterations (default 100000, matching Jest) and throws Jest's "assuming an infinite loop" error if timers remain, replacing an unbounded while loop that spins forever on any self-re-arming timer. Touches src/runtime/test_runner/timers/FakeTimers.rs (new timer_limit: u32 field, option parsing, bounded drain), packages/bun-types/test.d.ts (type for the new option), docs/runtime/cron.mdx (steer users away from runAllTimers() with cron), and adds subprocess-based tests.
Security risks
None. Test-runner-only code path (bun:test fake timers), no network/filesystem/auth surface. The new option is validated (finite, ≥1, ≤u32::MAX) before the as u32 cast so there's no UB from out-of-range float-to-int.
Level of scrutiny
Moderate. It's a small, self-contained fix that matches Jest's documented semantics exactly (same option name, same default, same error message), and the native-side change follows the file's existing unsafe { (*timer_all()).fake_timers... } idiom without introducing new borrow patterns. But it does two things that push it past auto-approve for me: (1) it adds a new user-facing option to useFakeTimers(), and (2) it changes observable behavior — code that previously hung now throws. Both are strictly improvements and Jest-compat, but API surface additions in the test runner are the kind of thing a maintainer should sign off on.
Other factors
- Test coverage is thorough: three re-arming variants (setInterval, recursive setTimeout, Bun.cron) run in subprocesses with a spawn-side timeout as the fail-before guard, boundary cases (chain of exactly
limitand below), and option validation. Tests follow the harness conventions (bunExe/bunEnv, concurrent drain of stdout/stderr/exited, stdout asserted before exitCode,await usingfor the process). - I previously left a cosmetic nit (fractional
timerLimitis truncated rather than rejected despite the "positive integer" message) that hasn't been addressed; it's non-blocking. - CI was still building at last check per robobun.
There was a problem hiding this comment.
LGTM — the fractional timerLimit nit is addressed in 13337b8 (n.trunc() != n + 1.5 in the validation test).
Checked the execute_all_timers loop bound and post-loop peek() against the boundary test (chain of exactly timerLimit timers does not throw); the off-by-one is handled correctly. The timer_limit field defaults to 0 via #[derive(Default)] but is always written by activate() before run_all_timers can read it (guarded by error_unless_fake_timers). Subprocess tests drain pipes concurrently and assert signalCode: null so a pre-fix hang fails observably.
Extended reasoning...
Overview
Bounds jest.runAllTimers() at timerLimit iterations (default 100000, matching Jest's modern fake timers) and throws Jest's exact "assuming an infinite loop" message if timers remain. Touches src/runtime/test_runner/timers/FakeTimers.rs (new timer_limit field, bounded loop in execute_all_timers, option parsing/validation in use_fake_timers), packages/bun-types/test.d.ts (adds timerLimit to the options type), docs/runtime/cron.mdx (steers cron users to advanceTimersByTime/runOnlyPendingTimers), and adds subprocess tests to fake-timers.test.ts.
Security risks
None. Fake timers are test-runner-only surface; the change replaces an unbounded loop with a bounded one, which strictly reduces DoS-by-hang risk. Input validation on timerLimit clamps to [1, u32::MAX], rejects non-finite and non-integral values before the as u32 cast, so no truncation/wrap surprises.
Level of scrutiny
Moderate — native Rust with unsafe raw-pointer access to the per-thread timer::All, but every new unsafe block follows the identical pattern already used throughout the file (single-statement borrow, ends before re-entering fire). The behavioral change is a strict improvement (previously-infinite loop now capped) and mirrors Jest's documented semantics, so compat risk is low.
Other factors
- My earlier nit (non-integral
timerLimitsilently truncated) was fixed in 13337b8 and the thread is resolved; no other outstanding comments. - Test coverage is thorough per CLAUDE.md conventions: three re-arm variants (setInterval, recursive setTimeout,
Bun.cron) each in a subprocess with a spawn-side timeout as the fail-before guard,signalCode: nullasserted, concurrent pipe drain, boundary cases at and below the limit, and validation for 0/-1/1.5/NaN/Infinity/string/object plusundefinedand1accepted. - Traced the boundary: with
timerLimit: 50and a 50-long self-terminating chain, thefor _ in 0..50loop fires all 50, the last one doesn't re-arm, and the post-looppeek().is_none()returnsOk(())— matches the test. timer_limitdefaults to 0 under#[derive(Default)]butrun_all_timersrequiresis_active()(viaerror_unless_fake_timers) andactivate()always writes it, so it's never read as 0.
There was a problem hiding this comment.
The earlier nit is addressed and I found no further issues, but packages/bun-types/test.d.ts is CODEOWNER-owned so leaving this for a human sign-off.
Checked: execute_all_timers boundary — exactly-limit timers doesn't throw (post-loop peek() handles it).
Checked: timer_limit defaulting to 0 via #[derive(Default)] is unreachable — error_unless_fake_timers gates the read, activate() always sets it.
Checked: timerLimit parsing mirrors the existing now option's get() pattern; undefined returns None so the default applies.
Checked: subprocess tests drain both pipes concurrently and use spawn-side timeout as the fail-before guard.
Extended reasoning...
Overview
Bounds jest.runAllTimers() at timerLimit iterations (default 100000, matching Jest's modern fake timers) so self-re-arming timers throw instead of spinning forever. Touches FakeTimers.rs (new timer_limit field, bounded loop in execute_all_timers, option parsing in use_fake_timers), adds the timerLimit type to test.d.ts, updates the cron docs to steer away from runAllTimers(), and adds subprocess-isolated tests for setInterval / recursive setTimeout / Bun.cron plus boundary and validation cases.
Security risks
None. Test-runner-only surface; the new option is validated (finite, ≥1, ≤u32::MAX, integral) before the as u32 cast. No untrusted-input parsing beyond a single number from a user's own test code.
Level of scrutiny
Moderate. It's a focused behavioral fix (unbounded → bounded loop) plus a small new user-facing option that follows Jest's spec. The Rust change reuses the existing unsafe per-thread timer_all() pattern from neighboring code and doesn't add new lifetime hazards. The prior nit (fractional timerLimit silently truncated) was fixed in 13337b8 with a n.trunc() != n check and a test case for 1.5.
Other factors
I'm not approving because packages/bun-types/test.d.ts falls under /packages/bun-types/ @alii in CODEOWNERS. The type change itself is trivial (one optional documented field), but per policy CODEOWNER-covered files get a human review. The Rust and test changes look correct on their own; boundary handling, default reset on each useFakeTimers() call, and error propagation via ? in run_all_timers all check out.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
…llback 1ms later (#42728) ### Problem - Under `jest.useFakeTimers()`, `advanceTimersByTime()` and `runOnlyPendingTimers()` never return when a timer callback arms the next timer with no delay: an `AbortSignal.timeout(0)` armed again by its own `abort` listener, or a `while (!done) await Bun.sleep(0)` loop. Bun spins inside one host call, so no test timeout fires. Fuzz-found, no user report. - `FakeTimers::execute_until` (`src/runtime/test_runner/timers/FakeTimers.rs:274`) fires every timer due at or before its target. Such a timer lands on the current fake instant, so it is due again. ### Fix - While a fake timer's callback runs, a timer armed with no delay gets 1ms (`FakeTimers::min_delay_ms`). Both arm sites that accept 0 use it: `AbortSignal::Timeout::init` (new `RuntimeHooks` slot) and `TimerObjectInternals::reschedule` (`Bun.sleep`). - `@sinonjs/fake-timers` has this rule (`addTimer`: `delay || (duringTick ? 1 : 0)`). Each re-armed timer is 1ms later, so the drain ends. Real timers and interval re-arms do not change. - Verified: `test/js/bun/test/fake-timers/fake-timers.test.ts` (11 new cases, 8 fail without the fix). Also the rest of that directory, `test-timers`, `sleep`, cron, `web/abort`. - Self-reviewed: 15 concerns raised, 9 addressed, 5 needed no change, 1 rejected. ### Background - The `jest` timer controls pop due timers from a fake heap and run each through `FakeTimers::fire`, which first sets the fake clock. - `setTimeout(fn, 0)` is always 1ms. Only `AbortSignal.timeout(0)` and `Bun.sleep(0)` arm with no delay. - `RuntimeHooks` is the function table through which `bun_jsc` (`AbortSignal`) calls up into `bun_runtime` (the timer heap). - With no event loop task on the stack (the first tests of a file), `fire` also drains the callback's microtasks. The `Bun.sleep(0)` loop re-arms there. <details><summary>Notes</summary> Repro (the bound keeps it from hanging): ```js const { jest } = Bun.jest(); jest.useFakeTimers(); let fires = 0; const arm = () => AbortSignal.timeout(0).addEventListener("abort", () => { if (++fires < 20000) arm(); }); arm(); jest.advanceTimersByTime(1); // same with jest.runOnlyPendingTimers() console.log(fires, performance.now()); ``` | | 1.4.3 | this PR | lolex 5.1.2, `setTimeout(fn, 0)` re-armed | |---|---|---|---| | `advanceTimersByTime(1)` / `tick(1)` | 20000 fires, all at 0 | 2 fires, at 0 and 1 | 2 fires, at 0 and 1 | | `runOnlyPendingTimers()` / `runToLast()` | 20000 fires, all at 0 | 1 fire, at 0 | 1 fire, at 0 | | `advanceTimersToNextTimer()` twice / `next()` twice | fires at 0 and 0 | fires at 0 and 1 | fires at 0 and 1 | The `Bun.sleep(0)` loop, as the first test of a file: `setTimeout(() => (done = true), 3)`, then `(async () => { while (!done) await Bun.sleep(0); })()`, then `advanceTimersByTime(5)`. 1.4.3 polls forever at instant 0. This PR polls at 0, 0, 1, 2 and returns with the clock at 5. After a test that awaited a real timer, the controls run inside an event loop task and do not drain microtasks between timers. There the loop never spun, and it does not change. Behavior changes beyond the hang. Each equals what `setTimeout(fn, 0)` already does in Bun and in Jest: - An `AbortSignal.timeout(0)` that a fake timer callback arms fires 1ms after that callback. Before, it fired at the same fake instant, inside the same `advanceTimersByTime()` call. - A `Bun.sleep(0)` (any value below 1) that a fake timer callback calls resolves 1ms after that callback. - `advanceTimersToNextTimer()` moves the clock to the re-armed timer, 1ms later. Before, the clock stayed. The rule is on the requested delay, and not on the deadline. A first version compared the deadline with the fake clock in `All::insert`. That also moved a `setInterval(fn, 10)` whose callback calls `advanceTimersByTime(10)` (its next deadline equals the clock) from 10, 20, 30 to 10, 21, 31, and an `_idleStart` write that lands on the clock. Neither is a zero delay. Two of the new tests hold the interval case. `firing` is a depth counter and not a flag. A listener can call a nested timer control, which fires another timer and returns into the listener. A flag is clear from then on, and a zero-delay timer that the listener arms afterwards spins the outer drain again. One of the new tests covers this. The count spans the whole `EventLoopTimer::fire` call, which includes the microtask drain on return. Timer control calls that still do not return, and where they stand: - `runAllTimers()` with a timer that always re-arms (`setInterval`, `Bun.cron`, the repro above). Unbounded by design, in Jest too. Jest stops at `timerLimit`. #34325 added that cap and is closed. This PR does not change `runAllTimers()`. `tick()` has no loop limit in Jest or sinon, so a count cap on `execute_until` is not the right shape. - A callback that writes `_idleStart` so that a timer is due at the current instant, in a cycle. That is an explicit absolute deadline. Not changed. - The test timeout cannot interrupt any synchronous spin: #30598, #21277. That is the rejected review concern. It is a backstop for all of these, and a separate change. Related PRs. None of them is a prerequisite. Each pair needs a textual merge only: - #42045 (never move the fake clock backwards) changes `fire()` and `advance_timers_by_time`. Its tests arm no zero-delay timer, so this change cannot move their values. - #38740 (per-VM fake clock) moves the clock into `FakeTimers`. It also covers a callback that installs the fake clock again. - #41571 adds an `execute_until(now)` step to `advanceTimersToNextTimer()`. Without this rule that step has the same hang. - #40187 (timer: remove unsafe) rewrites the timer module. `node:test` `mock.timers` (#32631) is a separate implementation in `src/js/internal/test_runner/mock_timers.ts`. It replaces `AbortSignal.timeout` with its own timers and has its own `tick()`. This PR does not touch it. Suites run on the debug ASAN build: `test/js/bun/test/fake-timers/` (the sinon port gives the same pass, fail and todo sets with `--todo` as 1.4.3), `test/js/bun/test/test-timers.test.ts`, `test/js/bun/util/sleep.test.ts`, `test/js/bun/cron/in-process-cron.test.ts`, `test/js/bun/cron/cron-local-time.test.ts`, `test/regression/issue/26284.test.ts`, `test/regression/issue/25869.test.ts`, `test/js/third_party/jsonwebtoken/`, `test/js/web/abort/`, the fake timers case of `test/cli/test/isolation.test.ts`, `test/internal/source-lints/`, and `cargo clippy -p bun_jsc -p bun_runtime`. `test/js/web/timers/setTimeout.test.js`, `setInterval.test.js` and `timer-gc-roots.test.ts` have 5 failures on my machine under the debug ASAN build (RSS thresholds and one 5s timeout). The same 5 fail with `src/` at `main`. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/test/fake-timers/fake-timers.test.ts bun test v1.4.3 (09bb546) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [25.32ms] (pass) advanceTimersToNextTimer > one setTimeout [9.05ms] (pass) advanceTimersToNextTimer > setInterval [8.07ms] (pass) advanceTimersToNextTimer > sorted timeouts [13.08ms] (pass) advanceTimersToNextTimer > alternating intervals [9.95ms] (pass) advanceTimersByTime > setInterval [6.35ms] (pass) advanceTimersByTime > advanceTimersByTime(NaN) throws and does not move the clock [5.91ms] (pass) advanceTimersByTime > advanceTimersByTime(-1) throws and does not move the clock [1.24ms] (pass) advanceTimersByTime > advanceTimersByTime(Infinity) throws and does not move the clock [0.91ms] (pass) advanceTimersByTime > advanceTimersByTime(4294967296) throws and does not move the clock [1.47ms] (pass) runOnlyPendingTimers > two setIntervals [7.85ms] (pass) runAllTimers > two setIntervals [9.60ms] (pass) getTimerCount > returns correct count of pending timers [8.65ms] (pass) getTimerCount > throw ... (truncated) release without fix: 8 FAILED bun test v1.4.3-canary.1 (09bb546) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [10.41ms] (pass) advanceTimersToNextTimer > one setTimeout [0.14ms] (pass) advanceTimersToNextTimer > setInterval [0.09ms] (pass) advanceTimersToNextTimer > sorted timeouts [0.11ms] (pass) advanceTimersToNextTimer > alternating intervals [0.08ms] (pass) advanceTimersByTime > setInterval [0.07ms] (pass) advanceTimersByTime > advanceTimersByTime(NaN) throws and does not move the clock [0.10ms] (pass) advanceTimersByTime > advanceTimersByTime(-1) throws and does not move the clock (pass) advanceTimersByTime > advanceTimersByTime(Infinity) throws and does not move the clock (pass) advanceTimersByTime > advanceTimersByTime(4294967296) throws and does not move the clock (pass) runOnlyPendingTimers > two setIntervals [0.13ms] (pass) runAllTimers > two setIntervals [0.07ms] (pass) getTimerCount > returns correct count of pending timers [0.06ms] (pass) getTimerCount > throws error if fake timers not active [0.02ms] (pass) clearAllTimers > clears all pending timers [0.05ms] (pass) clearAllTimers > throws error if fake timers not active [0.02ms] (pass) AbortSignal.timeout ... (truncated) ``` </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/pr_gate.xml" test/js/bun/test/fake-timers/fake-timers.test.ts bun test v1.4.3 (09bb546) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [26.39ms] (pass) advanceTimersToNextTimer > one setTimeout [9.05ms] (pass) advanceTimersToNextTimer > setInterval [8.23ms] (pass) advanceTimersToNextTimer > sorted timeouts [13.11ms] (pass) advanceTimersToNextTimer > alternating intervals [10.00ms] (pass) advanceTimersByTime > setInterval [6.67ms] (pass) advanceTimersByTime > advanceTimersByTime(NaN) throws and does not move the clock [6.56ms] (pass) advanceTimersByTime > advanceTimersByTime(-1) throws and does not move the clock [1.29ms] (pass) advanceTimersByTime > advanceTimersByTime(Infinity) throws and does not move the clock [0.91ms] (pass) advanceTimersByTime > advanceTimersByTime(4294967296) throws and does not move the clock [1.56ms] (pass) runOnlyPendingTimers > two setIntervals [8.18ms] (pass) runAllTimers > two setIntervals [9.79ms] (pass) getTimerCount > returns correct count of pending timers [8.87ms] (pass) getTimerCount > thro ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision b23c2c8 features baseline 23 deps, 131 codegen, 1176 objects in 692ms ninja: Entering directory `/workspace/bun/build/release' [1/1248] install /workspace/bun bun install v1.4.3-canary.1 (09bb546) Checked 22 installs across 61 packages (no changes) [14.00ms] [2/1248] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (09bb546) Checked 1 install across 2 packages (no changes) [1.00ms] [3/1248] gen ErrorCode+*.h [4/1248] gen bindgenv2 [5/1248] install /workspace/bun/src/node-fallbacks bun install v1.4.3-canary.1 (09bb546) Checked 111 installs across 104 packages (no changes) [4.00ms] [6/1248] fetch zlib [zlib] up to date [7/1248] fetch tinycc [tinycc] up to date [8/1247] fetch libjpeg-turbo [libjpeg-turbo] up to date [9/1220] gen node-fallbacks/react-refresh.js Bundled 1 module in 7ms react-refresh.js 4.81 KB (entry point) [10/1220] gen .bind.ts → GeneratedBindings.cpp [11/1220] gen bake.{client,server,error}.js -> bake.client.js, bake.server ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/AbortSignal.rs | 1 + src/jsc/VirtualMachine.rs | 9 ++ src/runtime/jsc_hooks.rs | 11 ++ src/runtime/test_runner/timers/FakeTimers.rs | 13 ++ src/runtime/timer/timer_object_internals.rs | 5 +- test/js/bun/test/fake-timers/fake-timers.test.ts | 189 ++++++++++++++++++++++- 6 files changed, 226 insertions(+), 2 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/AbortSignal.rs 2 2 24 src/jsc/VirtualMachine.rs 4 5 23 src/runtime/jsc_hooks.rs 2 3 23 src/runtime/test_runner/timers/FakeTimers.rs 3 7 22 src/runtime/timer/timer_object_internals.rs 3 1 22 test/js/bun/test/fake-timers/fake-timers.test.ts 5 5 22 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…llback 1ms later (oven-sh#42728) ### Problem - Under `jest.useFakeTimers()`, `advanceTimersByTime()` and `runOnlyPendingTimers()` never return when a timer callback arms the next timer with no delay: an `AbortSignal.timeout(0)` armed again by its own `abort` listener, or a `while (!done) await Bun.sleep(0)` loop. Bun spins inside one host call, so no test timeout fires. Fuzz-found, no user report. - `FakeTimers::execute_until` (`src/runtime/test_runner/timers/FakeTimers.rs:274`) fires every timer due at or before its target. Such a timer lands on the current fake instant, so it is due again. ### Fix - While a fake timer's callback runs, a timer armed with no delay gets 1ms (`FakeTimers::min_delay_ms`). Both arm sites that accept 0 use it: `AbortSignal::Timeout::init` (new `RuntimeHooks` slot) and `TimerObjectInternals::reschedule` (`Bun.sleep`). - `@sinonjs/fake-timers` has this rule (`addTimer`: `delay || (duringTick ? 1 : 0)`). Each re-armed timer is 1ms later, so the drain ends. Real timers and interval re-arms do not change. - Verified: `test/js/bun/test/fake-timers/fake-timers.test.ts` (11 new cases, 8 fail without the fix). Also the rest of that directory, `test-timers`, `sleep`, cron, `web/abort`. - Self-reviewed: 15 concerns raised, 9 addressed, 5 needed no change, 1 rejected. ### Background - The `jest` timer controls pop due timers from a fake heap and run each through `FakeTimers::fire`, which first sets the fake clock. - `setTimeout(fn, 0)` is always 1ms. Only `AbortSignal.timeout(0)` and `Bun.sleep(0)` arm with no delay. - `RuntimeHooks` is the function table through which `bun_jsc` (`AbortSignal`) calls up into `bun_runtime` (the timer heap). - With no event loop task on the stack (the first tests of a file), `fire` also drains the callback's microtasks. The `Bun.sleep(0)` loop re-arms there. <details><summary>Notes</summary> Repro (the bound keeps it from hanging): ```js const { jest } = Bun.jest(); jest.useFakeTimers(); let fires = 0; const arm = () => AbortSignal.timeout(0).addEventListener("abort", () => { if (++fires < 20000) arm(); }); arm(); jest.advanceTimersByTime(1); // same with jest.runOnlyPendingTimers() console.log(fires, performance.now()); ``` | | 1.4.3 | this PR | lolex 5.1.2, `setTimeout(fn, 0)` re-armed | |---|---|---|---| | `advanceTimersByTime(1)` / `tick(1)` | 20000 fires, all at 0 | 2 fires, at 0 and 1 | 2 fires, at 0 and 1 | | `runOnlyPendingTimers()` / `runToLast()` | 20000 fires, all at 0 | 1 fire, at 0 | 1 fire, at 0 | | `advanceTimersToNextTimer()` twice / `next()` twice | fires at 0 and 0 | fires at 0 and 1 | fires at 0 and 1 | The `Bun.sleep(0)` loop, as the first test of a file: `setTimeout(() => (done = true), 3)`, then `(async () => { while (!done) await Bun.sleep(0); })()`, then `advanceTimersByTime(5)`. 1.4.3 polls forever at instant 0. This PR polls at 0, 0, 1, 2 and returns with the clock at 5. After a test that awaited a real timer, the controls run inside an event loop task and do not drain microtasks between timers. There the loop never spun, and it does not change. Behavior changes beyond the hang. Each equals what `setTimeout(fn, 0)` already does in Bun and in Jest: - An `AbortSignal.timeout(0)` that a fake timer callback arms fires 1ms after that callback. Before, it fired at the same fake instant, inside the same `advanceTimersByTime()` call. - A `Bun.sleep(0)` (any value below 1) that a fake timer callback calls resolves 1ms after that callback. - `advanceTimersToNextTimer()` moves the clock to the re-armed timer, 1ms later. Before, the clock stayed. The rule is on the requested delay, and not on the deadline. A first version compared the deadline with the fake clock in `All::insert`. That also moved a `setInterval(fn, 10)` whose callback calls `advanceTimersByTime(10)` (its next deadline equals the clock) from 10, 20, 30 to 10, 21, 31, and an `_idleStart` write that lands on the clock. Neither is a zero delay. Two of the new tests hold the interval case. `firing` is a depth counter and not a flag. A listener can call a nested timer control, which fires another timer and returns into the listener. A flag is clear from then on, and a zero-delay timer that the listener arms afterwards spins the outer drain again. One of the new tests covers this. The count spans the whole `EventLoopTimer::fire` call, which includes the microtask drain on return. Timer control calls that still do not return, and where they stand: - `runAllTimers()` with a timer that always re-arms (`setInterval`, `Bun.cron`, the repro above). Unbounded by design, in Jest too. Jest stops at `timerLimit`. oven-sh#34325 added that cap and is closed. This PR does not change `runAllTimers()`. `tick()` has no loop limit in Jest or sinon, so a count cap on `execute_until` is not the right shape. - A callback that writes `_idleStart` so that a timer is due at the current instant, in a cycle. That is an explicit absolute deadline. Not changed. - The test timeout cannot interrupt any synchronous spin: oven-sh#30598, oven-sh#21277. That is the rejected review concern. It is a backstop for all of these, and a separate change. Related PRs. None of them is a prerequisite. Each pair needs a textual merge only: - oven-sh#42045 (never move the fake clock backwards) changes `fire()` and `advance_timers_by_time`. Its tests arm no zero-delay timer, so this change cannot move their values. - oven-sh#38740 (per-VM fake clock) moves the clock into `FakeTimers`. It also covers a callback that installs the fake clock again. - oven-sh#41571 adds an `execute_until(now)` step to `advanceTimersToNextTimer()`. Without this rule that step has the same hang. - oven-sh#40187 (timer: remove unsafe) rewrites the timer module. `node:test` `mock.timers` (oven-sh#32631) is a separate implementation in `src/js/internal/test_runner/mock_timers.ts`. It replaces `AbortSignal.timeout` with its own timers and has its own `tick()`. This PR does not touch it. Suites run on the debug ASAN build: `test/js/bun/test/fake-timers/` (the sinon port gives the same pass, fail and todo sets with `--todo` as 1.4.3), `test/js/bun/test/test-timers.test.ts`, `test/js/bun/util/sleep.test.ts`, `test/js/bun/cron/in-process-cron.test.ts`, `test/js/bun/cron/cron-local-time.test.ts`, `test/regression/issue/26284.test.ts`, `test/regression/issue/25869.test.ts`, `test/js/third_party/jsonwebtoken/`, `test/js/web/abort/`, the fake timers case of `test/cli/test/isolation.test.ts`, `test/internal/source-lints/`, and `cargo clippy -p bun_jsc -p bun_runtime`. `test/js/web/timers/setTimeout.test.js`, `setInterval.test.js` and `timer-gc-roots.test.ts` have 5 failures on my machine under the debug ASAN build (RSS thresholds and one 5s timeout). The same 5 fail with `src/` at `main`. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 6 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/test/fake-timers/fake-timers.test.ts bun test v1.4.3 (09bb546) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [25.32ms] (pass) advanceTimersToNextTimer > one setTimeout [9.05ms] (pass) advanceTimersToNextTimer > setInterval [8.07ms] (pass) advanceTimersToNextTimer > sorted timeouts [13.08ms] (pass) advanceTimersToNextTimer > alternating intervals [9.95ms] (pass) advanceTimersByTime > setInterval [6.35ms] (pass) advanceTimersByTime > advanceTimersByTime(NaN) throws and does not move the clock [5.91ms] (pass) advanceTimersByTime > advanceTimersByTime(-1) throws and does not move the clock [1.24ms] (pass) advanceTimersByTime > advanceTimersByTime(Infinity) throws and does not move the clock [0.91ms] (pass) advanceTimersByTime > advanceTimersByTime(4294967296) throws and does not move the clock [1.47ms] (pass) runOnlyPendingTimers > two setIntervals [7.85ms] (pass) runAllTimers > two setIntervals [9.60ms] (pass) getTimerCount > returns correct count of pending timers [8.65ms] (pass) getTimerCount > throw ... (truncated) release without fix: 8 FAILED bun test v1.4.3-canary.1 (09bb546) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [10.41ms] (pass) advanceTimersToNextTimer > one setTimeout [0.14ms] (pass) advanceTimersToNextTimer > setInterval [0.09ms] (pass) advanceTimersToNextTimer > sorted timeouts [0.11ms] (pass) advanceTimersToNextTimer > alternating intervals [0.08ms] (pass) advanceTimersByTime > setInterval [0.07ms] (pass) advanceTimersByTime > advanceTimersByTime(NaN) throws and does not move the clock [0.10ms] (pass) advanceTimersByTime > advanceTimersByTime(-1) throws and does not move the clock (pass) advanceTimersByTime > advanceTimersByTime(Infinity) throws and does not move the clock (pass) advanceTimersByTime > advanceTimersByTime(4294967296) throws and does not move the clock (pass) runOnlyPendingTimers > two setIntervals [0.13ms] (pass) runAllTimers > two setIntervals [0.07ms] (pass) getTimerCount > returns correct count of pending timers [0.06ms] (pass) getTimerCount > throws error if fake timers not active [0.02ms] (pass) clearAllTimers > clears all pending timers [0.05ms] (pass) clearAllTimers > throws error if fake timers not active [0.02ms] (pass) AbortSignal.timeout ... (truncated) ``` </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/pr_gate.xml" test/js/bun/test/fake-timers/fake-timers.test.ts bun test v1.4.3 (09bb546) test/js/bun/test/fake-timers/fake-timers.test.ts: (pass) fake timers [26.39ms] (pass) advanceTimersToNextTimer > one setTimeout [9.05ms] (pass) advanceTimersToNextTimer > setInterval [8.23ms] (pass) advanceTimersToNextTimer > sorted timeouts [13.11ms] (pass) advanceTimersToNextTimer > alternating intervals [10.00ms] (pass) advanceTimersByTime > setInterval [6.67ms] (pass) advanceTimersByTime > advanceTimersByTime(NaN) throws and does not move the clock [6.56ms] (pass) advanceTimersByTime > advanceTimersByTime(-1) throws and does not move the clock [1.29ms] (pass) advanceTimersByTime > advanceTimersByTime(Infinity) throws and does not move the clock [0.91ms] (pass) advanceTimersByTime > advanceTimersByTime(4294967296) throws and does not move the clock [1.56ms] (pass) runOnlyPendingTimers > two setIntervals [8.18ms] (pass) runAllTimers > two setIntervals [9.79ms] (pass) getTimerCount > returns correct count of pending timers [8.87ms] (pass) getTimerCount > thro ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision b23c2c8 features baseline 23 deps, 131 codegen, 1176 objects in 692ms ninja: Entering directory `/workspace/bun/build/release' [1/1248] install /workspace/bun bun install v1.4.3-canary.1 (09bb546) Checked 22 installs across 61 packages (no changes) [14.00ms] [2/1248] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (09bb546) Checked 1 install across 2 packages (no changes) [1.00ms] [3/1248] gen ErrorCode+*.h [4/1248] gen bindgenv2 [5/1248] install /workspace/bun/src/node-fallbacks bun install v1.4.3-canary.1 (09bb546) Checked 111 installs across 104 packages (no changes) [4.00ms] [6/1248] fetch zlib [zlib] up to date [7/1248] fetch tinycc [tinycc] up to date [8/1247] fetch libjpeg-turbo [libjpeg-turbo] up to date [9/1220] gen node-fallbacks/react-refresh.js Bundled 1 module in 7ms react-refresh.js 4.81 KB (entry point) [10/1220] gen .bind.ts → GeneratedBindings.cpp [11/1220] gen bake.{client,server,error}.js -> bake.client.js, bake.server ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/AbortSignal.rs | 1 + src/jsc/VirtualMachine.rs | 9 ++ src/runtime/jsc_hooks.rs | 11 ++ src/runtime/test_runner/timers/FakeTimers.rs | 13 ++ src/runtime/timer/timer_object_internals.rs | 5 +- test/js/bun/test/fake-timers/fake-timers.test.ts | 189 ++++++++++++++++++++++- 6 files changed, 226 insertions(+), 2 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/AbortSignal.rs 2 2 24 src/jsc/VirtualMachine.rs 4 5 23 src/runtime/jsc_hooks.rs 2 3 23 src/runtime/test_runner/timers/FakeTimers.rs 3 7 22 src/runtime/timer/timer_object_internals.rs 3 1 22 test/js/bun/test/fake-timers/fake-timers.test.ts 5 5 22 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
1 participant
What
jest.runAllTimers()drained the fake-timer heap with an unboundedwhileloop:so any timer that re-arms itself from its own callback makes it spin at 100% CPU forever with no yield point (the per-test timeout cannot interrupt it). This has always been reachable via plain
setInterval, and since #33623Bun.cronparticipates in fake timers and hits the same loop.docs/runtime/cron.mdxcurrently tells users to userunAllTimers()with cron, which walks them straight into the hang.Repro
Fix
Match Jest: bound the drain at
timerLimititerations and, if timers still remain afterwards, throwAborting after running N timers, assuming an infinite loop!. The limit defaults to 100000 and is configurable viajest.useFakeTimers({ timerLimit })(same option name and default as Jest's modern fake timers).runOnlyPendingTimers()is unaffected; it was already bounded.Also updates
docs/runtime/cron.mdxto recommendadvanceTimersByTime()/runOnlyPendingTimers()for cron instead ofrunAllTimers(), and adds thetimerLimitfield to theuseFakeTimerstype.Verification
New tests in
test/js/bun/test/fake-timers/fake-timers.test.tsspawn a subprocess for each ofsetInterval, recursivesetTimeout, andBun.cronwithtimerLimit: 50, and assert the thrown message and fire count. Without thesrc/change they spin until the spawn/ test timeout and fail; with it they throw after 50 fires. Boundary cases (chain of exactlytimerLimittimers does not throw) andtimerLimitoption validation are covered.