Repository navigation
Conversation
|
Updated 2:38 PM PT - Sep 16th, 2026
✅ Your commit f8c5cab has passed in 🧪 To try this PR locally: bunx bun-pr 42968That installs a local version of the PR into your bun-42968 --bun |
|
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 (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesWindows wakeup and hot reload
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The hot-reload wakeup behavior handles queued work consistently across both event-loop paths, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the new per-tick wakeup() in src/runtime/jsc_hooks.rs can busy-loop on Windows when has_pending_immediate stays set — it mirrors the unix path, which already zeroes the poll timeout under the same condition, so this is parity rather than a new spin. I also checked the state.is_null() branch of auto_tick: it never reached the removed early wakeup either, so no wakeup is lost there.
Extended reasoning...
Two findings are already posted inline (the debug/ASAN cost of the 1000-module hot test, and the sibling --hot run-loop path that bypasses auto_tick_active after an unhandled error). This note only records the concerns examined beyond them: the unconditional per-tick wakeup() matches the existing unix zero-timeout behavior under the identical has_pending_immediate predicate, and the null-state branch of auto_tick was not covered by the removed early wakeup, so the move does not drop a wakeup on that path. The change itself is small and Windows-only, but since it touches the event-loop tick in cfg-gated code that CI does not type-check on this lane, a human look remains worthwhile.
There was a problem hiding this comment.
I re-reviewed the latest revision (f8c5cab) and found no new bugs; the follow-up commit addressed the earlier test-size/timeout concern (600 modules, finite 60s bound on debug). Since the fix is Windows-only event-loop wakeup logic that can't be exercised from this Linux checkout, the Windows CI lane is the real validation and a maintainer look at src/runtime/jsc_hooks.rs is still worthwhile.
What was reviewed:
- The wakeup now sits after the
state.is_null()early return in bothauto_tickandauto_tick_active— that path polls withtick_without_idle(non-blocking), so dropping the wakeup there loses nothing. - Widening the condition to
has_pending_tasks()mirrors the unix zero-timeout branch intimer::All::get_timeoutexactly;us_wakeup_loopis auv_async_send, so it is idempotent and only shortens the poll the same way unix already does. - The new test only fails on Windows (unix already zeroes the timeout on pending tasks), so
USE_SYSTEM_BUN=1can't be checked here;stderr: "inherit"remains, which is cosmetic.
Extended reasoning...
Overview
The PR moves the Windows-only el.wakeup() call in auto_tick and auto_tick_active (src/runtime/jsc_hooks.rs) from immediately after promote_yield_tasks to the block that computes has_pending_immediate, and widens the condition from has_yielded_tasks || !immediate_tasks.is_empty() to also include el.has_pending_tasks(). It adds one test to test/cli/hot/hot.test.ts that rewrites 600 imported modules under --hot and waits for the second reload. The second commit (f8c5cab) shrank the fixture from 1000 to 600 files and replaced the Infinity debug timeout with 60s, addressing the substance of the prior inline finding.
Security risks
None. The change alters only when the JS thread's libuv poll is woken; no user input, parsing, auth, or crypto paths are involved. The test is hermetic (tempDir, no network).
Level of scrutiny
Moderate. The diff is small and the reasoning is sound: timer::All::get_timeout in src/runtime/timer/mod.rs discards has_pending_immediate on non-unix, so on Windows a task left in el.tasks after a tick that hit CONCURRENT_REFILLS_PER_TICK (or a HotReloadTask early return) had nothing to shorten the uv_run. Calling us_wakeup_loop (uv_async_send) before us_loop_run is the correct equivalent of a zero poll timeout, and it is idempotent, so the widened condition cannot spin any harder than the unix path already does. Relocating the call past the state.is_null() early return is safe because that branch uses tick_without_idle, which never blocks. However, the change is #[cfg(windows)]-gated and cannot be compiled or exercised from this Linux checkout, and it touches the core run-loop drive path; Windows CI and a maintainer familiar with the libuv integration are the appropriate final check.
Other factors
The new test exercises the scenario only on Windows; on unix the has_pending_tasks() term already zeroes the timeout, so the test passes both with and without the fix there, which is expected for a platform-specific regression test. The test still inherits stderr rather than piping it; that is cosmetic and was noted previously. The pre-existing sibling in tick_possibly_forever (raised inline in the prior run) is unchanged and remains out of scope for this PR.
Problem
--hotprocess whose watcher reports more than about 136 changed files at once stops reloading. The entry point never re-evaluates until the next file event. Onegit checkoutthat touches that many imported files is enough.tick_turn(src/jsc/event_loop.rs) runs such a task, returns early without a microtask drain, and stops after 8 refills with the rest still inel.tasks. On unixhas_pending_immediatethen zeroes the poll timeout. On Windowsget_timeout(src/runtime/timer/mod.rs) ignores that flag, and the wakeup inauto_tickandauto_tick_active(src/runtime/jsc_hooks.rs) only covered yielded and immediate tasks. Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 addedhas_pending_tasks()to the unix side and not to the Windows wakeup.Fix
auto_tickandauto_tick_activenow callwakeup()on Windows whenhas_pending_immediateis set, the same condition that zeroes the unix timeout. The check moves next to that computation, so it also sees tasks queued by the GC timer that runs in between.test/cli/hot/hot.test.ts"--hot reloads after hundreds of imported files change at once". With the change a Windows debug build passes 7 of 7 in about 3 s. Without it the same build reloads after a stall of 3 to 12 s in most runs and timed out at 5 s in 2 of 2 early runs, so the test is a regression check, not a deterministic proof (Notes). Alsotest/cli/watch/watch.test.tsandtest/js/node/timers/node-timers.test.tson Windows.Background
EventLoop::tick_turndrainsel.tasks, refills from the cross-thread queue up to 8 times, then drains microtasks. AHotReloadTaskreturnsEarlyReturnfromrun_task, which skips the microtask drain and counts as one task, so one tick runs at most 9 of them.auto_tick_active, which polls the platform loop with a timeout fromtimer::All::get_timeout. On unix that timeout is zero when tasks are pending. On Windows the poll isuv_run, and bun wakes it throughuv_async_sendinstead.Notes
Found while working on #42951. The first symptom was one
DEBUG: Reloading...followed by silence, the JS thread idle inuv__pollwith 1119 tasks inel.tasks. Limiting the resync to 129 events (17 tasks) worked and 256 (32 tasks) hung, which located the refill cap and the missing wakeup.The deterministic reproduction is the overflow resync in #42951: hundreds of reload tasks posted while the JS thread is inside a reload, with no later post to wake it. The test here writes 600 imported modules in one loop under
--hotand waits for the second run. The writes arrive over a few batches, each post wakes the loop once, and libuv coalesces the wakeups, so a debug build of main usually drains the backlog after a stall of several seconds instead of hanging. Sizes from 300 to 1000 files and a second batch written during the first reload were tried on the unfixed build. None hangs every time. The test keeps the batch shape with a finite bound on debug builds.Suites on Windows (debug build of this branch):
test/cli/hot/hot.test.tsexcept the two tests that hang on every Windows debug build of main ("renamed() into place" and "sourcemap loading", see #42938),test/cli/watch/watch.test.ts(6 pass, 7 skip),test/js/node/timers/node-timers.test.ts(23 pass).no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/hot/hot.test.ts