win: high-resolution event-loop timer via waitable timer + IOCP - #34834
Conversation
On an idle Windows box the GetQueuedCompletionStatusEx timeout rounds to the ~15.6ms system tick, so setTimeout(cb, 1) and Bun.sleep(1) fire ~15ms late unless another process has raised the tick rate. Patch libuv to arm a CREATE_WAITABLE_TIMER_HIGH_RESOLUTION waitable timer for the poll deadline and associate it with the loop's IOCP via NtAssociateWaitCompletionPacket (Win10 1803+). The kernel posts a NULL-overlapped completion when it fires, which uv__poll already treats as a pure wakeup, and GQCS blocks with INFINITE so its coarse ms timeout never applies. On older Windows, fall back to timeBeginPeriod(1). Before: setTimeout(1) median ~15.5ms on Windows After: setTimeout(1) median ~1.5ms on Windows
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
If a later uv_loop_init step failed (timer_heap alloc, wq_mutex init, async init, loops_add), the hrtimer / wait-completion-packet handles leaked through the fail_* unwind path. Move the call to just before the successful return so no new cleanup label is needed.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesWindows libuv polling adds high-resolution wakeups for short deadlines on supported systems, preserves the existing fallback path, manages per-loop timer resources, wires the patch into builds, and adds a 1 ms timer regression test. Windows high-resolution timer support
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: 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 `@patches/libuv/win-hrtimer.patch`:
- Around line 147-152: Replace TIMER_ALL_ACCESS with SYNCHRONIZE |
TIMER_MODIFY_STATE in both CreateWaitableTimerExW calls: the capability probe at
patches/libuv/win-hrtimer.patch lines 147-152 and the per-loop call in
uv__hrtimer_init at lines 170-175. No other access rights or timer behavior
should change.
- Around line 137-142: Shorten the uv__init probe comment at
patches/libuv/win-hrtimer.patch lines 137-142 to three lines or fewer, retaining
only a concise summary and referring to the patch header for details. Also
shorten the uv__poll arm-timer comment at lines 215-220 to three lines or fewer;
both sites require direct comment-only changes.
🪄 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: 33555de0-b50e-41f2-90d8-938051920943
📒 Files selected for processing (3)
patches/libuv/win-hrtimer.patchscripts/build/deps/libuv.tstest/js/web/timers/setTimeout.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…r in test - CreateWaitableTimerExW: TIMER_ALL_ACCESS -> SYNCHRONIZE|TIMER_QUERY_STATE| TIMER_MODIFY_STATE (matches Go's createHighResTimer). - Trim the uv__init probe and uv__poll arm comments to three lines; the patch header carries the full rationale. - Test: filter 'WARNING: ASAN interferes' from stderr before asserting empty, matching the neighboring subprocess tests.
… unused param - Only arm the high-res waitable timer for deadlines <= 100ms. Bun.serve's 1s date-header / 4s sweep timers (and any far-away user timer) fall through to GQCS's own ms timeout, so a pure HTTP workload pays zero extra syscalls per poll. Measured Bun.serve hello-world throughput on Windows debug was already within noise of main (11.4k vs 11.4k req/s) before this guard; with it, the arm path is unreachable there. - NtCancelWaitCompletionPacket with RemoveSignaledPacket=TRUE to match Go's netpollQueueTimer and flush a stale queued packet instead of eating an extra empty iteration. - Drop the unused uv_loop_t* parameter from uv__hrtimer_init.
There was a problem hiding this comment.
No further findings — the earlier nits (init ordering, stderr filter, unused param, access mask) are all addressed. Deferring to a human because this rewires libuv's uv__poll hot path on Windows around undocumented Nt*WaitCompletionPacket syscalls and adds a process-wide timeBeginPeriod(1) fallback; a maintainer should sign off on those design choices and the stale-packet / manual-reset-timer edge cases.
What was reviewed:
timeout - 1 < 100on unsignedDWORD— wraps at 0 and INFINITE, so the arm range is exactly [1, 100] as intended.- Init/close pairing:
uv__hrtimer_initnow runs afteruv__loops_add, anduv__loop_closecancels + closes both handles;lfieldsis calloc'd so the NULL guards are sound. - Stale completion from a prior arm when the next poll skips the hrtimer branch — dequeues as a NULL-overlapped spurious wakeup, which
uv__pollalready ignores, so at worst one extra loop tick. - Test: median-of-50 with an 8 ms bound cleanly separates ~15.6 ms (before) from ~1-2 ms (after);
min >= 1guards against firing early.
Extended reasoning...
Overview
The PR adds a new libuv patch (patches/libuv/win-hrtimer.patch) that changes how uv__poll in src/win/core.c blocks for a deadline on Windows: instead of relying on GetQueuedCompletionStatusEx's millisecond timeout (which quantizes to the ~15.6 ms system tick), it arms a CREATE_WAITABLE_TIMER_HIGH_RESOLUTION waitable timer and associates it with the loop's IOCP via the undocumented NtAssociateWaitCompletionPacket, then blocks with INFINITE. On pre-1803 Windows it falls back to a one-shot timeBeginPeriod(1). Two struct fields are added to uv__loop_internal_fields_s, three ntdll function pointers are dynamically loaded, and winmm.lib is pulled in via #pragma comment. The build-script change registers the patch; the test change adds a subprocess median-latency assertion.
Security risks
Low. The new NT calls take no user-controlled input — all arguments are process-owned handles or constants. Access masks were already narrowed to SYNCHRONIZE | TIMER_QUERY_STATE | TIMER_MODIFY_STATE per earlier feedback. No parsing, no external data.
Level of scrutiny
High. uv__poll is the single blocking call at the heart of every Windows Bun process; a subtle mistake here is a hang or a busy-loop for every user. The mechanism relies on three undocumented ntdll exports whose exact semantics (re-association after cancel, RemoveSignaledPacket=TRUE behavior when the packet is already queued, interaction with a manual-reset timer that was left signaled from a prior iteration) are inferred from Go's runtime rather than a spec. The timeBeginPeriod(1) fallback is a process-global (and on older Windows, system-global) timer-resolution change with power implications — Node deliberately does not do this, so it's a design call a maintainer should ratify. The 100 ms arm threshold is a heuristic backed by one hello-world benchmark. None of that is a bug I can point to; it's exactly the kind of judgment a human owner of the Windows event loop should make.
Other factors
All five prior review threads (mine and CodeRabbit's) are resolved in commits 4a5a6b6 / 2345ba5 / f83da13. I traced the one non-obvious correctness edge I could find — a completion packet armed on iteration N whose next iteration has timeout outside [1, 100] so the NtCancelWaitCompletionPacket(TRUE) is skipped — and it degrades to a single NULL-overlapped spurious wakeup that the existing dequeue loop discards, so no hang. The SetWaitableTimer call reactivates the manual-reset timer each arm, and NtCancel(..., TRUE) is issued before re-associate, matching Go's sequence. The failure-closed shape (any arm/associate failure leaves timeout at its original ms value) is correct. Given the criticality of the code path and the design decisions involved, this is a defer rather than an approve.
|
CI build #76320: 285/286 jobs passed. The one red job is |
NtCancelWaitCompletionPacket can return STATUS_PENDING when the packet is mid-delivery (timer fired, kernel is inserting it into the IOCP queue). Re-associate in that window fails with STATUS_INVALID_PARAMETER_1 anyway, so skip SetWaitableTimer + associate and fall through to the GQCS ms timeout for this iteration. Matches Go's netpollQueueTimer. Previously this was already failure-closed via the NT_SUCCESS check on associate; the explicit guard just saves two syscalls and documents the edge case.
…en set If the timer fires between SetWaitableTimer and the associate, the kernel posts the packet immediately and sets AlreadySignaled. The packet is in the queue so GQCS(INFINITE) would return immediately anyway, but timeout=0 makes that explicit. Matches Go's netpollQueueTimer.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/web/timers/setTimeout.test.js (1)
534-539: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winShorten this test comment to three lines. Preserve the subprocess/median rationale, but move the extended explanation out of source.
As per coding guidelines: “Keep code comments to three lines or fewer.”
🤖 Prompt for 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. In `@test/js/web/timers/setTimeout.test.js` around lines 534 - 539, Shorten the comment in the setTimeout(1) test to three lines or fewer while preserving that it runs in a subprocess and uses the median to avoid resolution changes and isolated scheduler hiccups affecting the assertion; remove the extended Windows timer explanation from the source comment.Source: Coding guidelines
🤖 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/web/timers/setTimeout.test.js`:
- Around line 561-565: Remove the exact-empty stderr assertion following the
filteredStderr construction in the subprocess test. Continue draining and
filtering stderr, including the existing ASAN warning handling, but do not
require filteredStderr to equal an empty string.
---
Outside diff comments:
In `@test/js/web/timers/setTimeout.test.js`:
- Around line 534-539: Shorten the comment in the setTimeout(1) test to three
lines or fewer while preserving that it runs in a subprocess and uses the median
to avoid resolution changes and isolated scheduler hiccups affecting the
assertion; remove the extended Windows timer explanation from the source
comment.
🪄 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: ce3d7c11-93a3-4494-937c-0c46cfb8c159
📒 Files selected for processing (2)
patches/libuv/win-hrtimer.patchtest/js/web/timers/setTimeout.test.js
On pre-Win10-1803 the high-res probe fails and uv__poll keeps the plain GQCS ms timeout, i.e. behavior is unchanged from before this patch. Removes the winmm link and the process-global tick-rate side effect.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/libuv/win-hrtimer.patch (1)
206-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the timeout range explicit.
Line 206 relies on subtraction/underflow to exclude zero and the infinite sentinel. Use explicit bounds so zero, finite deadlines, and unset values remain deliberate; the Windows poll timeout is a
DWORD. (raw.githubusercontent.com)Proposed fix
- if (timeout - 1 < 100 && lfields->hrtimer != NULL) { + if (lfields->hrtimer != NULL && timeout >= 1 && timeout <= 100) {As per coding guidelines: “Enumerate input spaces deliberately, distinguishing empty, zero, and unset.”
🤖 Prompt for 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. In `@patches/libuv/win-hrtimer.patch` at line 206, Update the timeout condition near the hrtimer check to use explicit DWORD bounds instead of timeout - 1 arithmetic: distinguish zero, finite timeout values below 100, and the unset/infinite sentinel deliberately. Preserve the lfields->hrtimer != NULL requirement while ensuring underflow cannot influence the decision.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@patches/libuv/win-hrtimer.patch`:
- Line 206: Update the timeout condition near the hrtimer check to use explicit
DWORD bounds instead of timeout - 1 arithmetic: distinguish zero, finite timeout
values below 100, and the unset/infinite sentinel deliberately. Preserve the
lfields->hrtimer != NULL requirement while ensuring underflow cannot influence
the decision.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 359edd84-93df-468e-92df-e469f37f8c12
📒 Files selected for processing (2)
patches/libuv/win-hrtimer.patchscripts/build/deps/libuv.ts
There was a problem hiding this comment.
No issues found in this pass — all three earlier findings (init-failure HANDLE leak, stderr assertion, dead loop param) are addressed and CI is green on Windows. Deferring to a human because this rewires uv__poll's blocking semantics (GQCS now waits with INFINITE when the hrtimer arms) using undocumented Nt*WaitCompletionPacket APIs — the design looks sound and matches Go's runtime, but it's the Windows event-loop hot path and worth a maintainer's eyes on the 100ms threshold and the STATUS_PENDING / AlreadySignaled handling.
What was reviewed
uv__hrtimer_initordering afteruv__loops_add— no fallible work follows, so no leak on init failure.uv__pollarm path: cancel → SetWaitableTimer → associate short-circuits to the original ms wait on any failure, so a bad status can't hang GQCS atINFINITE.uv__loop_closereleases both handles;lfieldsis calloc'd so the NULL checks are safe on the fallback path.- Test: subprocess isolation, median-of-50, filtered-stderr pattern matches neighboring tests; 8ms bound cleanly separates 15.6ms from ~1.5ms.
Extended reasoning...
Overview
This PR adds a vendored libuv patch (patches/libuv/win-hrtimer.patch) that changes how uv__poll blocks on Windows when the next timer deadline is ≤100ms. Instead of passing that timeout to GetQueuedCompletionStatusEx (which rounds to the ~15.6ms system tick), it arms a CREATE_WAITABLE_TIMER_HIGH_RESOLUTION waitable timer, associates it with the loop's IOCP via NtAssociateWaitCompletionPacket, and blocks GQCS with INFINITE. The kernel posts a NULL-overlapped completion when the timer fires, which the existing dequeue loop already treats as a pure wakeup. It also wires the patch into scripts/build/deps/libuv.ts and adds a subprocess regression test in setTimeout.test.js.
Security risks
None identified. The new code runs entirely inside the process against handles it created itself; the Nt*WaitCompletionPacket functions are dynamically probed and the whole path degrades to the pre-existing GQCS ms wait if any probe fails. Access rights on the waitable timer were narrowed to SYNCHRONIZE | TIMER_QUERY_STATE | TIMER_MODIFY_STATE per earlier review. No user-controlled input reaches this path.
Level of scrutiny
High. uv__poll is the single blocking call in Bun's Windows event loop — every timer, socket, file I/O, and subprocess completion flows through it. The change replaces a bounded ms wait with INFINITE on the assumption that the waitable-timer completion will always arrive. The patch handles the known edge cases (STATUS_PENDING on cancel → skip arm this round; AlreadySignaled → timeout = 0; any step failing → keep original ms timeout), and Go's runtime uses the same recipe, but this is exactly the kind of change where a subtle kernel-behavior misunderstanding manifests as a rare production hang. The 100ms cutoff is also a design choice (precision vs 3 syscalls/poll) that a maintainer should sign off on.
Other factors
All prior review feedback (mine and CodeRabbit's) has been addressed and the threads are resolved. CI build #76320 passed 285/286 with the one red job on darwin (this patch is Windows-only, gated by enabled: cfg => cfg.windows). The test is well-constructed — subprocess isolation avoids in-process tick-rate contamination, median-of-50 tolerates CI jitter, and the 8ms bound sits well between the before (15.6ms) and after (~1.5ms) values. The approach has strong prior art (golang/go#44343). I'm deferring not because I found a defect but because the blast radius of uv__poll warrants human sign-off.
…imer_init uv__init() is now unchanged from upstream. uv__hrtimer_init() gates on the dynamically-loaded pNt*WaitCompletionPacket function pointers (matching libuv's convention for optional ntdll APIs) and on CreateWaitableTimerExW succeeding with the HIGH_RESOLUTION flag. Removes the uv__have_hrtimer static and the duplicated CreateWaitableTimerExW call.
uv__loop_close now calls uv__hrtimer_close(lfields) instead of inlining the handle cleanup, matching the init/close pairing convention.
Arm the high-res waitable timer for any finite nonzero deadline. The 100ms threshold was a heuristic and meant a 110ms timer could fire up to ~15ms late; the measured per-poll syscall overhead without the gate was already noise vs main (11,484 vs 11,434 req/s on Bun.serve hello-world).
oven-sh/libuv#9 landed the high-res poll timeouts on the bun branch, so drop the local patch file and bump LIBUV_COMMIT. This also pulls in the intervening Windows fs correctness fixes on that branch (oven-sh/libuv#7 and #8).
…tries by build The previous wording attributed aarch64 solo variance to #34834, which is a Windows-only libuv change. Solo aarch64 >20s was observed in CI retry attempts (builds 85866, 85400) but via a different mechanism than Windows.
…ng (#36478) Fixes `test/js/node/test/parallel/test-fs-read-stream-pos.js` going red on main (seen on builds 84352, 84676, 85400, 85630, 85664, 85692, 85720-85866 across win2019, alpine/ubuntu/debian aarch64, and debian x64-asan). ### Cause The test's exit path is a pure timing race: a `setInterval(append, 1)` writer must land a write between two consecutive `ReadStream` preads within a single stream instance (partial chunk → another `'data'` before `'end'`). Upstream ships it with a 90-second safety timer for exactly that reason. In addition, each stream's `'data'` handler is wrapped in `common.mustCallAtLeast(1)`, so any stream that observes zero bytes fails the process on exit. Two recent PRs together pushed both failure modes over the runner's 20 s ceiling: * #34834 raised Windows event-loop timer resolution from ~15 ms to ~1 ms. With the old resolution the writer and reader both fired on the same ~15 ms tick and each stream covered ~1 write (so the race hit in ≤25 cycles); with 1 ms resolution each stream covers ~10 writes, the partial chunk is always last, and the per-stream hit rate drops to ~1/500. 30 solo runs on a Windows box: 1 s–40 s, max 40 s, 5 runs >20 s. * #36175 removed the `cpuCount: 2` clamp on the linux/windows test agents, taking the parallel-safe phase from width 1 to width 3. Running alongside `test-fs-read-stream-fd-leak.js` (50× `createReadStream` at 2 ms) starves the 1 ms appender long enough for a later stream to be created with `start == EOF`, producing zero `'data'` events and failing `mustCallAtLeast` with "Mismatched function calls. Expected at least 1, actual 0." (e.g. build 85400 debian aarch64, attempts 1–2). Neither changes what the test is actually checking (ReadStream position tracking while a file is being appended, nodejs/node#33940), which has never failed here; all observed reds are the race not being reached inside the runner's 20 s window, or the per-stream `mustCallAtLeast` tripping under I/O contention. ### Change In `scripts/runner.node.mjs`: * `isParallelSafeTest` now returns `false` for this file, so it runs in the serial phase. With no I/O neighbours the 1 ms writer keeps its cadence and every stream sees at least one write, so the `mustCallAtLeast` failure cannot occur. * `getNodeParallelTestTimeout` gives it 120 s, so on the occasions the race does not hit within 90 s the test's own safety timer fires and the process exits 0 (Node's python runner also allows 120 s per `test/parallel` file). Same assertions and code path; only the runner's scheduling of this file changes. ### Verification * Windows x64 solo, 30 consecutive runs with the release canary: 30/30 pass, times 1 s–40 s (median ~6 s), none reached the 90 s safety timer, none exited non-zero. * `node scripts/runner.node.mjs --include js/node/test/parallel/test-fs-read-stream-pos.js` on linux: file is scheduled in the serial phase (not under "Running N parallel-safe tests"), passes. * `bun test test/internal/parallel-allowlist.test.ts`: 2/2 pass. The event-loop ordering difference (Bun drains queued thread-pool completions before yielding to due timers, Node yields to the timer phase between poll iterations) is a pre-existing behaviour not introduced by either of the two PRs above; changing it is out of scope for unbreaking this test. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · build/CI scripts only; test-proof not applicable <!-- robobun:evidence:end -->
2 participants
What
On an idle Windows box the
GetQueuedCompletionStatusExtimeout rounds to the ~15.6 ms system clock tick, sosetTimeout(cb, 1)andBun.sleep(1)fire ~15 ms late unless another process happens to have raised the tick rate (the "works when Spotify is open" heisenbug). Node.js has the same behavior.How
Bumps libuv to oven-sh/libuv@9687330, which landed oven-sh/libuv#9 (same approach Go's runtime uses, golang/go#44343):
uv__winapi_init, dynamically loadNtCreateWaitCompletionPacket/NtAssociateWaitCompletionPacket/NtCancelWaitCompletionPacketfrom ntdll.CREATE_WAITABLE_TIMER_HIGH_RESOLUTIONwaitable timer + wait-completion-packet and stash them onuv__loop_internal_fields_s(Win10 1803+ / Server 2019+; on older Windows the handles stay NULL anduv__pollkeeps its original GQCS ms wait, so behavior is unchanged).uv__poll, whentimeout > 0: arm the waitable timer for the deadline, associate it with the loop's IOCP, and wait in GQCS withINFINITE. When the timer fires, the kernel posts a completion withlpOverlapped == NULL, which the existing dequeue loop already treats as a pure wakeup.The bump also pulls in the intervening Windows fs correctness fixes on the
bunbranch (oven-sh/libuv#7, #8).Numbers (Windows Server 2019, idle)
setTimeout(cb,1)Bun.sleep(1)setTimeout(cb,5)setInterval(16)Bun.servehello-world throughput on Windows debug (oha, 10s, 50 concurrent): 11,484 req/s on this branch vs 11,434 req/s on main (noise).The added test in
setTimeout.test.jsmeasures the median of 50setTimeout(1)samples in a subprocess and asserts it's under 8 ms (before: 15.6 ms; after: ~1.5 ms).Fixes #16714
Fixes #26965
no test proof · iteration 1 · Platform-specific test-only change; deferring to CI.