fix(timer): use monotonic QPC for Timespec::now on Windows - #34779
Conversation
The timer heap inserts and drains deadlines against Timespec::now(). On unix this reads CLOCK_MONOTONIC, but on Windows it fell through to time::nano_timestamp() (GetSystemTimePreciseAsFileTime), which is wall-clock. An NTP correction, user clock change, or VM-suspend step would stall every pending timer (backward step) or fire them all at once (forward step), and the uv_timer wakeup was armed against libuv's monotonic clock while the heap used wall time. Route the Windows arm through the existing clock_gettime_monotonic QPC shim in c-bindings.cpp so the heap shares a clock with libuv (uv_hrtime), uSockets' sweep (io::Loop::update_timespec) and WTF::MonotonicTime::now(). All Timespec::now callers were audited: every site uses the value as a delta or for ordering against another Timespec::now-derived deadline, so changing the Windows epoch from 1970 to boot is safe. Also drop the dead calibrated-TSC src/perf/hw_timer.rs (zero callers) and fix the stale doc comment on Timespec::now.
|
Verified on Windows x64: Before (canary After (debug build of this branch,
Earlier CI runs had only unrelated failures: debian x64-asan |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesMonotonic timer clock
Hardware timer module removal
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/bun_core/util.rs`:
- Around line 2321-2324: Handle the nonzero status from clock_gettime_monotonic
by exposing it as a typed Rust error rather than discarding it, then update the
call sites in src/bun_core/util.rs lines 5303-5311 and src/io/lib.rs lines
1096-1101 to propagate that error or apply an explicit fallback before
constructing Timespec; preserve successful timestamp behavior.
In `@test/js/web/timers/setTimeout.test.js`:
- Around line 535-537: Update the regression-test comment near the
setTimeout/setInterval monotonic-clock assertion to remove the explanatory prose
and retain only the tracked issue URL, following repository comment guidelines.
- Around line 534-542: Update the “timer heap clock is monotonic, not
wall-clock” test to read timerInternals.timerClockMs() twice around a bounded
event and assert the second reading is greater than or equal to the first. Keep
the existing positive and boot-relative wall-clock checks separate, without
relying on Date.now() to establish monotonicity.
🪄 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: b1dffba4-55f9-42dd-86c8-b381baa2c3e3
📒 Files selected for processing (4)
src/bun_core/util.rssrc/perf/hw_timer.rssrc/perf/lib.rstest/js/web/timers/setTimeout.test.js
💤 Files with no reviewable changes (2)
- src/perf/hw_timer.rs
- src/perf/lib.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nits, I also checked: the #[cfg(not(unix))] → #[cfg(windows)] narrowing on now_real() drops no supported target (Bun only ships linux/macos/windows); and hw_timer::read_counter has zero remaining references repo-wide, so the deletion is safe.
Extended reasoning...
Two nit-level findings are already posted inline. This note just records what else was examined and ruled out: (1) the cfg narrowing from not(unix) to windows could in principle leave a platform with no now_real() body, but Bun's supported targets are linux/macos/windows only, so nothing is dropped; (2) grepped all of src/ for hw_timer and read_counter — no hits, so the dead-file deletion is clean; (3) timerInternals.timerClockMs is an existing binding already exercised by test-timers-ordering.js, so the new test's import is not novel surface. Not approving because this changes the clock source underlying every Windows timer and the PR's own caller audit missed at least one absolute-value consumer — worth a human look.
…t strengthening - c-bindings.cpp: make ticksPerSec a C++11 thread-safe static since Timespec::now() now calls clock_gettime_monotonic from multiple threads (Worker timer heaps, install threadpool). QueryPerformanceFrequency is documented to always succeed on Windows XP+, so the ENOTSUP path is unreachable but kept for belt-and-suspenders. - server/mod.rs: notify_server_started now sends wall-clock milli_timestamp_allow_mocked_time() to match its notify_server_stopped sibling, so DevTools receives start/stop on the same clock base on every platform (was already mismatched on unix). - setTimeout.test.js: add a second clock read and assert it is non-decreasing.
|
Re the find-issues suggestions above: I tested all three and none are fixed by this PR.
Not adding |
Align notify_server_started with its notify_server_stopped sibling on both axes: wall-clock (vs monotonic) and real (vs fake-timers-mocked). The inspector timestamps reach a real DevTools UI, not test assertions, so respecting fake-timers here has no benefit and would disagree with the stop event.
There was a problem hiding this comment.
All prior review findings are addressed and the b3f41ce pass found nothing new, but this changes the clock base for the Windows timer heap (every setTimeout/setInterval/AbortSignal.timeout deadline), so a maintainer should sign off.
Checked this round: notify_server_started/notify_server_stopped now both send plain milli_timestamp() (same clock, same mocked-ness); the C++ ticksPerSec init is a proper C++11 thread-safe static; no remaining references to hw_timer/read_counter anywhere in src/; the #[cfg(not(unix))] → #[cfg(windows)] narrowing leaves no supported target uncovered.
Extended reasoning...
Overview
Six files: the Windows arm of Timespec::now_real() in src/bun_core/util.rs now calls the QPC clock_gettime_monotonic shim instead of wall-clock nano_timestamp(); the shim in src/jsc/bindings/c-bindings.cpp gets a C++11 thread-safe static for ticksPerSec; src/runtime/server/mod.rs:3964 switches the inspector startTime to milli_timestamp() to match its notify_server_stopped sibling; the dead calibrated-TSC src/perf/hw_timer.rs module is deleted and de-registered from src/perf/lib.rs; and test/js/web/timers/setTimeout.test.js gains a monotonic + boot-relative assertion via timerInternals.timerClockMs().
Security risks
None. This is a clock-source selection change with no user-controlled input, no parsing, no auth/crypto surface.
Level of scrutiny
High. Timespec::now() is the reference clock for the timer heap (src/runtime/timer/mod.rs) and is read from dozens of call sites across install, DNS cache, lifecycle scripts, debugger wait loops, and profiler filenames. Changing its epoch on Windows from Unix-1970 to boot is correct (aligns with the unix CLOCK_MONOTONIC behavior and with libuv's uv_hrtime that arms the wakeup), but any missed absolute-value consumer would silently misbehave. My earlier pass found one such consumer (inspector startTime) that the PR's caller audit had missed; it's now fixed, and I found no others this round.
Other factors
All three of my prior inline findings are addressed and resolved: the ticksPerSec lazy-init race (fixed with a lambda-initialized magic static in a793f86), the inspector start/stop clock mismatch (a793f86), and the mocked-vs-real asymmetry between the sibling notifiers (b3f41ce — both now use plain milli_timestamp(), verified at mod.rs:3964/3978). CodeRabbit's QPF-failure concern was correctly declined (documented never-fails on XP+; debug_assert! matches src/io/lib.rs). I re-grepped src/ for hw_timer/read_counter — zero hits, deletion is safe. The #[cfg(not(unix))] → #[cfg(windows)] narrowing is fine since every Bun target is unix or windows. The new test's t1 < Date.now()/2 bound gives ~28 years of uptime headroom, well beyond any CI runner.
Deferring rather than approving because this is a platform-wide behavioral change to core timer infrastructure (critical code path per the approval guidelines), and Windows CI for b3f41ce (build #76075) is the actual proof — I can't run it here.
QueryPerformanceFrequency is documented to always succeed on Windows XP and later, so the nonzero-return path was dead. Change the shim to void and update both Rust extern declarations and call sites accordingly.
There was a problem hiding this comment.
I didn't find any issues this pass — all three concerns from my earlier reviews (thread-safe QPF init, inspector startTime clock, mocked-vs-real milli_timestamp sibling mismatch) are addressed, and the void-return signature change in 5b5512d is reflected at both Rust extern sites. Deferring to a human because this changes the timer-heap clock epoch for every setTimeout/setInterval on Windows and Windows CI hasn't reported yet.
Checked: #[cfg(unix)]/#[cfg(windows)] arms of now_real() are exhaustive for supported targets; hw_timer/read_counter have zero remaining references; notify_server_started/_stopped now agree on both the wall-clock and mocked axes; the < Date.now()/2 test bound holds for any uptime under ~28 years so it can't false-negative.
Extended reasoning...
Overview
Routes Windows Timespec::now_real() through the existing clock_gettime_monotonic QPC shim so the timer heap uses the same monotonic clock as libuv/uSockets/WTF instead of wall-clock GetSystemTimePreciseAsFileTime. Follow-up commits: made the shim's ticksPerSec a C++11 thread-safe static (a793f86), switched the inspector startTime to plain milli_timestamp() to match its _stopped sibling (b3f41ce), and dropped the now-unreachable ENOTSUP path by making the shim void-returning with both Rust extern decls updated (5b5512d). Also deletes the unused src/perf/hw_timer.rs and adds a timerClockMs() regression test.
Security risks
None. No user input, auth, crypto, or network parsing touched.
Level of scrutiny
High — this changes the clock epoch for every timer-heap insert/drain on Windows from 1970 to boot. The change itself is small and well-reasoned, but the caller audit already had one miss (inspector startTime) that surfaced in review, and the test evidence defers to Windows CI. A human should confirm CI is green on Windows before merge.
Other factors
All prior review threads (mine and CodeRabbit's) are resolved. Verified this pass: the int → void FFI signature change is consistent across c-bindings.cpp, src/bun_core/util.rs, and src/io/lib.rs; hw_timer has no remaining references anywhere in src/; the two #[cfg] arms in now_real() leave exactly one tail expression per supported target; and the new test's < Date.now()/2 bound (~28 years of uptime) correctly discriminates the pre-fix wall-clock reading without false positives.
2 participants
Problem
The timer heap inserts and drains
setTimeout/setInterval/AbortSignal.timeoutdeadlines againstTimespec::now()(src/runtime/timer/mod.rs). On unix this readsCLOCK_MONOTONIC, but on WindowsTimespec::now_real()fell through totime::nano_timestamp(), which isGetSystemTimePreciseAsFileTime(wall clock):So an NTP correction, manual clock change, or VM-suspend/restore step would stall every pending timer (backward step) or fire them all at once (forward step). The
uv_timerwakeup is also armed against libuv's monotonic clock while the heap used wall time, so the two could disagree.Fix
Route the Windows arm of
Timespec::now_real()through the existingclock_gettime_monotonicQPC shim inc-bindings.cpp. This is the same monotonic clock that libuv (uv_hrtime), uSockets' sweep (io::Loop::update_timespec),performance.now()(std::time::Instant) andWTF::MonotonicTime::now()already use.After:
Caller audit
All
Timespec::now/ms_from_now/since_nowcall sites were checked. Every site uses the value as a delta or for ordering against anotherTimespec::now-derived deadline (timer heap inserts, elapsed-time logging, debugger wait loops, DNS cache TTL comparison), so changing the Windows epoch from 1970 to boot is safe. Absolute-value uses:{timestamp}.{pid}): already boot-relative on unix, so Windows now matches.InspectorHTTPServerAgent::notify_server_started(src/runtime/server/mod.rs): was sendingTimespec::now().ms()while its siblingnotify_server_stoppedsends wall-clockmilli_timestamp(). Switched tomilli_timestamp()so DevTools receives start/stop on the same clock on every platform.Cleanup
c-bindings.cpp:clock_gettime_monotonic'sstatic LARGE_INTEGER ticksPerSecis now a C++11 thread-safe static (lambda initializer). This PR makes the shim reachable from multiple threads (Worker timer heaps, install threadpool) for the first time; the previous unsynchronized lazy-init was a benign-in-practice data race.src/perf/hw_timer.rs(zero callers; docstring referenced anow_ns()that was never ported).Timespec::nowwith an accurate one.Testing
Added a test asserting
timerInternals.timerClockMs()is non-decreasing and< Date.now() / 2(boot-relative vs epoch). Verified it fails on Windows canary and passes with this change on both Windows (a793f869b) and Linux. FullsetTimeout.test.jssuite passes on Windows debug.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/timers/setTimeout.test.js