Conversation
runAllTimers() drained the fake timer heap with an unbounded loop. A timer
that re-arms itself from its own callback (setInterval, a recursive
setTimeout, Bun.cron) keeps the heap non-empty, so the call never returned
and no test timeout could interrupt it.
Like Jest, run at most timerLimit timers per call and throw "Aborting after
running N timers, assuming an infinite loop!" if timers remain. The limit
defaults to 100000. useFakeTimers({ timerLimit }) sets it.
|
Status: ready for review. Reproduced on canary 1.4.3 ( import { test, jest } from "bun:test";
test("runAllTimers on an infinitely self-rescheduling timer", () => {
jest.useFakeTimers();
const re = () => { setTimeout(re, 1); };
setTimeout(re, 1);
try { jest.runAllTimers(); } catch (e) { console.log("guard THREW: " + e.message); }
jest.useRealTimers();
}, 5000);With this branch it prints
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughChangesFake timers now accept a validated Fake timer API and guidance
Runtime enforcement
Validation
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The timer-limit API’s documented behavior and validation remain consistent, with no actionable merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because packages/bun-types/test.d.ts is CODEOWNER-owned, a human sign-off is still needed.
What was reviewed:
execute_all_timersloop bound and the exactly-at-limit edge —peek()after the loop returnsNone, so no throw when the heap drains on the last iteration.timerLimitvalidation — compares inf64before theu32cast; NaN/±Infinity/0/negatives/non-integers/> u32::MAXall rejected, andJSValue::getmapsundefinedtoNoneso{ timerLimit: undefined }falls through to the default.nowparsing after theelse if→ nested-ifrestructure — semantics unchanged, both options are now read independently.- Tests: added to the existing file,
test.eachfor the invalid-value and self-re-arming matrices, the 100k-default case gated onisDebug || isASAN, and the give-up guard so an unfixed build fails the assertion instead of hanging.
Extended reasoning...
Overview
This PR caps jest.runAllTimers() / vi.runAllTimers() at a configurable timerLimit (default 100,000, matching Jest) so self-re-arming timers throw instead of spinning forever. It touches src/runtime/test_runner/timers/FakeTimers.rs (loop bound + option parsing, ~40 net lines), packages/bun-types/test.d.ts (adds timerLimit?: number with JSDoc), the bun-types fixture, test/js/bun/test/fake-timers/fake-timers.test.ts (17 new cases), and docs/runtime/cron.mdx.
Security risks
None. This is test-runner-only fake-timer control; the only user input is a numeric option that is fully range-validated in the wide type before casting to u32. No network, filesystem, auth, or crypto paths are involved.
Level of scrutiny
Moderate. The Rust change is small and follows the neighboring patterns exactly — the two new unsafe blocks mirror the existing timer_all() dereferences with matching SAFETY comments, and the borrow ends before any timer callback can re-enter. I traced execute_next to confirm false means the heap is empty, verified JSValue::get returns None for undefined so an explicit timerLimit: undefined uses the default, and checked that the now refactor is behavior-preserving. The error message text matches Jest's @ sinonjs/fake-timers message. The #[derive(Default)] gives timer_limit: 0, but activate() always sets it before execute_all_timers can be reached.
Other factors
Test coverage is thorough and follows repo conventions: added to the existing fake-timers.test.ts, uses test.each for matrices, combined .toEqual assertions, and gates the 100k-timer default case behind isDebug || isASAN. The self-re-arming tests give up after 1000 fires so an unfixed build fails the toThrow assertion in milliseconds rather than hanging — satisfying the "fails for the right reason without spinning" requirement. The .d.ts change is straightforward, but packages/bun-types/ is owned by a CODEOWNER per .github/CODEOWNERS, so approval should wait for that sign-off.
This branch has not been deployed
2 participants
Problem
jest.runAllTimers()never returns when a timer re-arms itself on every fire (setInterval, a recursivesetTimeout,Bun.cron).bun testspins at 100% CPU, and no test timeout can interrupt one native call.FakeTimers::execute_all_timers(src/runtime/test_runner/timers/FakeTimers.rs):while Self::execute_next(global)? {}has no bound.Aborting after running 100000 timers, assuming an infinite loop!here. Fuzz-found, no user report.Fix
execute_all_timersfires at mosttimer_limittimers. If timers remain, it throws Jest's message.useFakeTimers({ timerLimit })sets it, with Jest's option name and default. A value that is not a positive integer throws.docs/runtime/cron.mdxno longer tells cron users to callrunAllTimers(). A cron job always re-arms.test/js/bun/test/fake-timers/fake-timers.test.ts(17 new cases, 14 fail on canary 1.4.3). Alsotest-timers.test.ts,in-process-cron.test.ts,bun-types.test.ts.Background
jest.useFakeTimers()putssetTimeout,setInterval,AbortSignal.timeoutandBun.crontimers on a separate heap.runAllTimers()fires the earliest one until the heap is empty.advanceTimersByTime()andrunOnlyPendingTimers()use another loop,execute_until. It still spins when a zero-delay timer re-arms itself (AbortSignal.timeout(0)). bun:test fake timers: schedule a zero-delay timer armed by a timer callback 1ms later #42728 fixes that with Jest's 1 ms floor.node:httpserver keeps its own 30 ssetIntervalin the fake heap. ThererunAllTimers()hung before and now throws. bun:test: keep built-in modules' own timers out of fake timers (internal/timers) #37987 takes such timers out of the fake heap.Notes
b99371011), killed by an external timeout after 15 s:guard THREW: Aborting after running 100000 timers, assuming an infinite loop!.@jest/fake-timerspassesloopLimit: fakeTimersConfig.timerLimit || 100_000to@sinonjs/fake-timers, andclock.runAll()throws the message above. The unbounded loop came over unchanged from the first fake timers PR (Add fake timers for bun:test #23764), which ported sinon's loop-limit tests asdescribe.todo.--timeoutinterrupt such loops. The cap is still useful with it: it fails in milliseconds with Jest's message and a stack at therunAllTimers()call, and it works outsidebun test(bun -e, top-level code).node:httpinteraction:_http_server.tssetupConnectionsTrackingarmssetInterval(checkConnections, 30_000)onlistening. WithtimerLimit: 50and a listening server,runAllTimers()throws after 50 sweeps. The same script on canary 1.4.3 never returns. Jest under Node does not see that timer, because Node's internals do not use the faked globals.timerLimit: 5: a re-armedAbortSignal.timeout(0)fired 20000 times inside oneadvanceTimersByTime(1)call and inside onerunOnlyPendingTimers()call.runAllTimers()threw after 5.setTimeout(fn, 0)is not affected, becausesetTimeoutfloors 0 to 1 ms.toThrowassertion in milliseconds. It does not spin. An earlier draft ran each case in a child process with a spawn timeout. On a build without the cap the 5 s test timeout fired first, the runner exited, and the children kept spinning as orphans.timerLimit. For thenode:httptimer the limit does not help, because that timer never stops.useFakeTimers()call resets the limit to the default, as a new JestuseFakeTimers()call does.timerLimit(0,nullandNaNmean the default,"10"coerces). This PR rejects a value that is not a positive integer, like the existingnowvalidation.{ timerLimit: undefined }means the default.@sinonjs/fake-timersrunAll()also throws when the chain ends exactly at the limit. This PR returns normally in that case.loopLimitis 10000.viandjestshare these functions in Bun, so both use Jest's 100000.runAllTimersAsync()on top ofexecute_all_timers, so it gets the same cap.cargo fmtdoes not reachFakeTimers.rs(a#[path]module inside an inlinemod), so the new lines follow rustfmt style by hand.[human-review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file