bun:test: keep runtime-internal timeouts out of the fake timer heap - #37946
Conversation
Tag::allow_fake_timers was a denylist, so every timer the runtime arms for itself (Bun.spawn timeout, Postgres/MySQL/Valkey connection, idle, lifetime and reconnect timers, UpgradedDuplex and named pipe timeouts, the c-ares poll timer, the Date header timer, dev server sweeps) went into the fake heap while jest.useFakeTimers() was active. There they were counted by getTimerCount(), only fired through advanceTimersByTime(), and were silently disarmed for good when useRealTimers() or clearAllTimers() drained the heap: the child was never killed, the connection attempt never timed out. Make the set an allowlist of the timers a program schedules itself (setTimeout/setInterval, AbortSignal.timeout(), Bun.cron), and arm every other owner with ForceRealTime, since the real heap is drained against the real clock. spawnSync's blocking wait compares against the real clock for the same reason, and only consults an AbortSignal.timeout() deadline that lives in the real heap. Bun.cron is the one remaining non-JS timer the fake heap can drop. Stop a dropped job like stop() would, instead of leaving it holding the event loop open for a timer that can never fire.
WalkthroughChangesThe change restricts fake timers to timeout objects, abort-signal timeouts, and cron jobs. Runtime-managed timers use real time. Spawn timeout checks exclude mocked deadlines. Cron jobs removed from the fake heap receive explicit cleanup. Tests cover spawn, SQL, Redis, and cron behavior. Fake timer scope and cleanup
Spawn timeout real-clock handling
Runtime-managed real-time timers
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reproduced on the released 1.4.0 binary with the spawn, spawnSync (Bun and Overlaps with #37935 and #37940, which each handle one of the owners covered here; see the PR description. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Note for reviewers: the Repro on 1.4.0import { expect, jest, test } from "bun:test";
import { execSync } from "node:child_process";
test("execSync({ timeout }) under fake timers", () => {
jest.useFakeTimers();
try {
let err: any;
try {
execSync("sleep 2", { timeout: 300, stdio: "ignore" });
} catch (e) {
err = e;
}
expect(err?.code).toBe("ETIMEDOUT"); // undefined: returned normally after ~2005 ms
} finally {
jest.useRealTimers();
}
}); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/bun/test/fake-timers/fake-timers.test.ts`:
- Around line 283-292: Update the sleepingChild workload to launch Bun with -e
and execute Bun.sleep(3000) instead of relying on the external sleep executable.
Preserve the existing timeout, killSignal, environment, and output settings so
the child remains alive beyond the timeout and exercises the intended path
portably.
- Around line 278-421: Add fake-timer regression tests covering socket timeout
behavior for both UpgradedDuplex and WindowsNamedPipe, verifying each timeout
expires using real time while fake timers are active and after useRealTimers().
Gate the WindowsNamedPipe coverage to Windows, and follow the existing
runtime-timeout test patterns and assertions.
🪄 Autofix
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: 60de6905-6458-4728-9d24-f9f69974f4ba
📒 Files selected for processing (16)
src/event_loop/EventLoopTimer.rssrc/event_loop/SpawnSyncEventLoop.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/cron.rssrc/runtime/bake/dev_server/hmr_socket.rssrc/runtime/bake/dev_server/source_map_store.rssrc/runtime/dns_jsc/dns.rssrc/runtime/socket/UpgradedDuplex.rssrc/runtime/socket/WindowsNamedPipe.rssrc/runtime/test_runner/timers/FakeTimers.rssrc/runtime/timer/Timer.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rstest/js/bun/test/fake-timers/fake-timers.test.ts
|
The spawnSync part of this PR is the same change as That branch has two tests this PR does not, both confined to test files, so they cherry-pick cleanly on top of this one:
|
![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 it inverts the fake-timer admission rule from opt-out to allowlist across ~15 timer owners, changes user-observable behavior (advanceTimersByTime no longer fires runtime timeouts), and overlaps with #37935/#37940, a human look would still be worthwhile.
What was reviewed:
- Every tag moved off the allowlist now arms with
ForceRealTime— includingValkeyConnectionReconnect, which sharesRefCountedTimer::armwith the connection timer. ImmediateObjectreturningfalsefrom the new allowlist is a no-op: immediates go throughenqueue_immediate_task, neverAll::insert.CronJob::stop_dropped_from_fake_heap→self_stop→stop_internalskips the heap remove becauseclear()already setstate = CANCELLED; no double-remove.- The spawnSync
in_heap == Regulargate correctly excludes both fake-heap and unlinked (None)AbortSignal.timeoutdeadlines.
Extended reasoning...
Overview
This PR inverts Tag::allow_fake_timers() in src/event_loop/EventLoopTimer.rs from an opt-out list (everything mockable except a handful of internal tags) to an explicit allowlist (TimeoutObject, AbortSignalTimeout, CronJob). Every timer owner that consequently moves to the real heap has its deadline computation flipped from AllowMockedTime to ForceRealTime (subprocess, Postgres, MySQL, Valkey, UpgradedDuplex, WindowsNamedPipe, c-ares poll, Date header, dev server sweep/visualizer). spawnSync's tick loop and its AbortSignal.timeout deadline probe are moved to the real clock, with a new in_heap == Regular filter. FakeTimers::clear gains a CronJob release arm and turns the wildcard into a debug_assert!. Nine new tests cover spawn/spawnSync (Bun and node:child_process), Postgres/MySQL/Redis connection timeouts, and Bun.cron under useRealTimers()/clearAllTimers().
Security risks
None identified. No auth, crypto, or untrusted-input parsing is touched; the change is which clock a set of internal deadlines are computed against and which heap they land in.
Level of scrutiny
Medium-high. The core mechanism is small and well-argued, but it is a semantic redesign of the fake-timer contract that touches 16 files across the event loop, timer subsystem, spawn, three SQL drivers, DNS, sockets, and the dev server. It deliberately changes user-observable behavior (advanceTimersByTime no longer fires SQL/Redis/subprocess timeouts) and the PR itself flags overlap with two sibling PRs (#37935, #37940) that will need rebase coordination. That combination — cross-cutting behavior change plus PR-ordering concerns — is exactly what a maintainer should sign off on rather than an automated approval.
Other factors
I traced the tag list against the diff: every owner leaving the allowlist has its arming clock flipped, including ValkeyConnectionReconnect (shares RefCountedTimer::arm) and both dev-server timers. ImmediateObject now returns false from allow_fake_timers() but is unaffected because immediates are enqueued via enqueue_immediate_task, not All::insert. The new CronJob release path is safe against double-remove: clear() sets state = CANCELLED and in_heap = None before self_stop runs, so stop_internal's state == ACTIVE guard skips the heap remove. The comment-cop and CodeRabbit threads on the timeline are all resolved (comments trimmed in 4d03cbd, sleeping child made portable, UpgradedDuplex/WindowsNamedPipe test request withdrawn). Test coverage is solid and the PR shows the 9 new cases failing on the unfixed build. No correctness issues found; deferring purely on scope and the design/coordination decision.
|
Heads up: #37936 moves the memory visualizer arm site (the |
Before #37946 the Date header timer was enrolled in jest.useFakeTimers(): accepting one connection under fake timers made jest.getTimerCount() report 1 with no user timers, and jest.runAllTimers() never returned because the timer re-armed itself into the fake heap on every pop while a keep-alive connection was open. #37946 fixed the enrollment; these tests pin the Bun.serve symptom, which its own tests do not exercise.
#37946 moved the resolver's 1s poll timer (Tag::DNSResolver) out of the fake timer heap and onto the real clock, but added no DNS coverage. These tests pin that behavior: an in-flight query is not counted by getTimerCount() and still times out after useRealTimers(), runAllTimers() returns while a query is in flight, and the timer is armed against the real clock even when the mocked clock has been pushed years ahead. All of them fail on bun 1.4.0 (getTimerCount() is 1, the query never settles, runAllTimers() spins until killed) and pass on main.
2 participants
Problem
jest.useFakeTimers()captures the runtime's own safety timeouts. With fake timers active,Bun.spawn({ timeout })never kills the child,new SQL({ connectionTimeout })/new RedisClient(url, { connectionTimeout })never time out, andjest.getTimerCount()counts these timers as if the test had created them.jest.useRealTimers()(andjest.clearAllTimers()) then drains the fake heap and disarms them permanently: a child spawned withtimeout: 200under fake timers runs to completion after the test is back on real time, and a Postgres connection attempt stays pending forever. The owner still believes it is armed, so nothing re-arms it.Bun.spawnSync({ timeout })under fake timers has the same problem without the heap: its wait loop compared the deadline against the mocked clock, which cannot advance while the call blocks.Tag::allow_fake_timers()insrc/event_loop/EventLoopTimer.rswas an opt-out list, so every owner not on it (SubprocessTimeout, the Postgres/MySQL/Valkey connection, idle, lifetime and reconnect timers,UpgradedDuplex,WindowsNamedPipe,DNSResolver,DateHeaderTimer, the dev server timers) was routed into the fake heap byAll::insert(src/runtime/timer/mod.rs), and each of them computed its deadline withAllowMockedTimeto match. The fake timers PR left this list as a TODO ("should subprocess timeout? probably not").FakeTimers::clear()(src/runtime/test_runner/timers/FakeTimers.rs) only toldTimeoutObjectandAbortSignal.timeout()owners that their timer was gone; every other popped node was just markedCANCELLED.Fix
allow_fake_timers()becomes an allowlist of the timers a program schedules itself:setTimeout/setInterval(TimeoutObject),AbortSignal.timeout(), andBun.cron()(documented as honoring fake timers). Everything else stays in the real heap and keeps running on the real clock, which is also what Jest does (its fake timers never reach Node's internal timeouts).ForceRealTime(subprocess, Postgres, MySQL, Valkey,UpgradedDuplex,WindowsNamedPipe, c-ares poll timer, Date header timer, dev server sweep / visualizer). This is required, not optional: the real heap is drained against the real clock (All::next), so a mocked deadline there would be due immediately. It is the same convention the existing opt-outs (BunTest,GcRepeating,StatWatcherScheduler,QuicEndpoint,DnsSdConnection) already follow.spawnSynccompares itstimeoutagainst the real clock too (js_bun_spawn_bindings.rs,SpawnSyncEventLoop.rs), and only consults anAbortSignal.timeout()deadline that lives in the real heap; one in the fake heap is on the mocked clock and cannot fire during a blocking call anyway. Behavior without fake timers is unchanged.Bun.cronis now the only non-JS owner the fake heap can drop, soFakeTimers::clear()collects dropped jobs and stops them the waystop()would (CronJob::stop_dropped_from_fake_heap), matching what happens to a droppedsetInterval. Before, a job created under fake timers and thenuseRealTimers()kept the process alive forever with a timer that could never fire. The fallback arm is now adebug_assert!, so adding a tag to the allowlist without a release path fails loudly in debug builds.advanceTimersByTime()no longer fires runtime-internal timeouts (e.g. a SQLconnectionTimeout); they elapse in real time. No test or doc relied on that; the cron tests that do rely on mocking are unaffected. Not changed here: the asyncnode:child_processtimeoutoption is implemented with a JSsetTimeoutinsrc/js/node/child_process.ts, so it is still faked like any othersetTimeout.test/js/bun/test/fake-timers/fake-timers.test.ts(newruntime timeouts are not fake timersandBun.cron() job dropped from the fake heapblocks, 9 tests, includingnode:child_process.spawnSync({ timeout }), which hands its timeout toBun.spawnSync). All 9 fail on the unfixed build (getTimerCount()is 1 or 2, spawn/spawnSync children exit normally, the cron child hangs until killed) and pass with the fix.test/js/bun/test/fake-timers/sinonjs,test-timers.test.ts,test/js/bun/cron/in-process-cron.test.tsandcron-local-time.test.ts,spawn-signal.test.ts(coversspawnSync+AbortSignal.timeout),spawnSync.test.ts,spawn-maxbuf.test.ts,spawnsync-isolated-event-loop.test.ts,spawnsync-no-microtask-drain.test.ts,test/js/sql/sql-connect-error-reporting.test.ts, the fake-timer cases oftest/cli/test/isolation.test.ts,test/regression/issue/25869.test.ts,26284.test.ts.cargo clippyonbun_event_loop,bun_runtime,bun_sql_jscis clean, including--target x86_64-pc-windows-msvcfor theWindowsNamedPipechange.allow_fake_timers().Background
timer::Allkeeps two intrusive heaps ofEventLoopTimernodes. The event loop drainstimersagainstCLOCK_MONOTONIC. Whilejest.useFakeTimers()is active,All::insertdiverts nodes whoseTagallows it intofake_timers.timers, which only thejest.*functions drain: each pop sets the mocked clock to the node's deadline and fires it;useRealTimers()/clearAllTimers()pop everything without firing.Tagsays which struct embeds a node (aSubprocess, aPostgresSQLConnection, aTimeoutObject, ...);__bun_fire_timerinsrc/runtime/dispatch.rsrecovers the owner from it. Owners detect their own cancellation by checkingstate == ACTIVEbefore removing, which is why a foreign cancellation is silent rather than a crash.Timespec::now(AllowMockedTime)returns the fake-timers clock when one is installed;ForceRealTimealways reads the monotonic clock. A deadline must be computed with the clock of the heap it is inserted into.FakeTimers::clear()returns the owners of the popped nodes so they can be released after the heap borrow ends (TimeoutObjectdrops its heap ref,AbortSignal.timeout()is cancelled through its signal, and now aCronJobis stopped); a node that is only unlinked leaves its owner holding event-loop refs for a timer that will never fire.spawnSyncruns the child on an isolated mini event loop and blocks the JS thread in a tick loop, so nojest.advanceTimersByTime()can run while it waits; the only clock that can make its timeout elapse is the real one.Repro on the released binary (1.4.0)
The SQL variant from the report (a
Bun.listenthat accepts and never answers,connectionTimeout: 1, let the connection get established, thenuseRealTimers()) stays pending past 4 s; with the fix it rejects withERR_POSTGRES_CONNECTION_TIMEOUTafter ~1 s.[review] gate passed · iteration 1 · 16 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file