Repository navigation
Conversation
…p wakeup The watcher arm of the run loop called on_before_exit() after every return from tick_possibly_forever(), which returns on every wakeup: its one second bounded wait, a signal, a ready one-shot poll. Dispatch only after the loop actually ran work and drained, or after a reload bumped hot_reload_counter (an entry with nothing to transpile off-thread is re-evaluated inside the wakeup's own tick without the loop ever being alive), plus once for the initial evaluation.
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the semantics of beforeExit dispatch in the core --hot/--watch run loop and interacts with several nearby open PRs (#38638, #38206, #34657, #38613), a human look would still be worthwhile.
What was reviewed:
- The gating logic in
run_command.rs: initial-eval / drained-work /hot_reload_counter-moved each set the flag; spurious wakeups (bounded wait, signal, unref'd poll) do not. hot_reload_counteris a plainu32read and written only on the JS thread (incremented inVirtualMachine::reload), so no atomics needed.- Confirmed the re-arm case (a
beforeExitlistener that schedules work) still re-dispatches: the scheduled work makesis_event_loop_alive()true on the next iteration, which re-enters the while body and sets the flag. - Test uses
forEachLine/tempDir/writeHotFileAtomicSyncfrom harness, awaits each line rather than sleeping, and thewhile (line === "start")loop absorbs coalesced watcher events.
Extended reasoning...
Overview
This PR gates vm.on_before_exit() in the watcher arm of Run::start (src/runtime/cli/run_command.rs) so that beforeExit fires once per drain of the event loop rather than once per return from tick_possibly_forever(). Two new flags track whether a drain occurred: dispatch_before_exit (set on initial entry and whenever the while is_event_loop_alive() body runs) and a snapshot of hot_reload_counter (to catch reloads that complete entirely inside a single wakeup without the loop ever being alive). A test in test/cli/hot/hot.test.ts covers both --hot and --watch, using SIGUSR2 as a wakeup that must not re-emit and a file save as a reload that must emit exactly once more.
Security risks
None. This is control flow around event dispatch in the run loop; no untrusted input, parsing, allocation, or privilege boundaries are involved.
Level of scrutiny
High. The watcher arm of Run::start is the core run loop for --hot and --watch — every long-running dev session goes through it. The change alters when a Node-compat lifecycle event fires, which is user-observable and something applications may depend on (cleanup handlers, graceful-shutdown hooks). The PR description is unusually thorough (mechanism traced, USE_SYSTEM_BUN=1 failure confirmed, --rerun-each=20 pass on ASAN, the counter-check verified load-bearing by removing it), and the logic reads correctly to me: signal/bounded-wait/unref'd-poll wakeups skip straight to tick_possibly_forever(), while any real work (loop-alive body ran, or the reload counter moved) re-arms the dispatch. The re-arm-from-listener case is preserved because scheduled work makes the loop alive on the next iteration.
Other factors
The PR itself notes four nearby open PRs touching the same line or the counters it reads, and describes how they compose. That merge-ordering judgment, plus the fact that this is a semantic change to a Node lifecycle event in the hottest dev-mode code path, is exactly what a maintainer should weigh in on. The bug hunting system found nothing; I checked the thread-safety of hot_reload_counter (single-threaded, JS-thread-only), the re-arm path, and the test's handling of coalesced watcher events, and found no issues. Deferring so a maintainer can confirm the intended beforeExit semantics under watch modes and the composition with the adjacent PRs.
Problem
bun --hotandbun --watch,process.on("beforeExit", ...)fires again about once per second while the script is idle, and in a tight loop whenever some fd the loop polls is ready. Plainbunand node (with or without--watch) emit it once when the loop drains. Reproduces on 1.4.0 and main (probes below).Run::start(src/runtime/cli/run_command.rs, theloopafter// core run-loop) callsvm.on_before_exit()on every iteration, and one iteration is one return fromtick_possibly_forever()(src/jsc/event_loop.rs), which returns on every wakeup: its one second bounded wait, a signal, a ready one-shot poll. Nothing checked whether the loop had done any work since the previous dispatch.tick_possibly_forever()uses is what makes the idle case visible.Fix
beforeExitonly when something drained: once for the initial evaluation, again after itswhile vm.is_event_loop_alive()body ran (the loop had work and ran out of it), and again whenvm.hot_reload_countermoved (a reload happened). A wakeup that did none of these goes straight back to parking.transpile_filein src/runtime/jsc_hooks.rs skips the concurrent store foris_main), so a reload of an entry without imports is evaluated entirely inside the tick of the wakeup that delivered the reload task, and the loop is never alive for it. Verified by building without the counter check: the--hottest below then fails because the reloaded generation never gets itsbeforeExit.VirtualMachine::on_before_exit(). A listener that schedules a timer once still getsstart, beforeExit, timer, beforeExitunder--hot, the same as plainbun. Two reloads that land in the same tick drain together and are announced once.report_exception_in_hot_reloaded_module_if_needed(), which reports a reloaded generation's rejection, retries a deferred reload and re-arms the watcher on the entry) still runs on every iteration; only the dispatch is gated.--hotand--watchvariants of "emits beforeExit once per generation, not once per event loop wakeup". The wake source isSIGUSR2: a signal listener runs without keeping the loop alive, so after eachsignalline nothing else may be printed. The test then saves the entry and expects the new generation to printstartand exactly one morebeforeExit. Signals are what make the test POSIX-only; the run loop is shared by all platforms.USE_SYSTEM_BUN=1, and the previous debug build): both variants fail within 100ms withexpected "signal", received "beforeExit". Fixed: pass, 40/40 with--rerun-each=20on the debug (ASAN) build.beforeExittests in test/js/node/process/process.test.js.console.write()(aFileSinkwrite + flush) inside abeforeExitlistener still re-fires forever, with or without a watcher, because the flushed write leaves its poll registered and ref'd, so the loop reads as alive andon_before_exit()'s own re-dispatch rule fires; that is aFileSinkkeep-alive bug being fixed separately. Once it is, this change is what stops the same scenario from looping under--hot(the poll still wakes the loop, but no longer counts as a drain).on_before_exit()call withon_before_exit_with(...)(the call simply moves inside the newif), hot/watch: keep driving timers and surviving errors after an unhandled error #38206 and hot: reset unhandled-error state on reload so timers recover after a failed reload #34657 change the error counters the dispatch is already guarded on, hot: replace a generation that is parked on a top-level await #38613 changes when a reload defers.Background
beforeExitis node's "the event loop is empty" event: emitted when the loop drains; if a listener schedules more work, the loop runs again and the event is emitted again when it drains; if not, the process exits.VirtualMachine::on_before_exit()implements exactly that (dispatch, drain, re-dispatch only if the drain did work).--hotor--watchthe process must not exit, soRun::starttakes a different arm: drive the loop whileis_event_loop_alive(), dispatchbeforeExit, then park intick_possibly_forever()until something wakes the loop.--hothandles a file change by posting a task that re-evaluates the entry in the same process (one "generation" per evaluation;hot_reload_counteris incremented by each reload, andglobalThisandprocesslisteners survive it);--watchre-executes the process, so each generation starts from the top of this function.is_event_loop_alive()counts ref'd handles, queued tasks and in-flight module loads. Signal listeners and unref'd handles do not count, so their callbacks run on a wakeup without the loop ever being alive.Probes (1.4.0, linux x64)