fix(runtime): real SIGINT/SIGBREAK/SIGHUP + process.kill on Windows#6631
Conversation
|
Warning Review limit reached
Next review available in: 14 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWindows console control events now feed process-signal listeners, and ChangesWindows process signals
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ConsoleControlHandler
participant WinSignalSlot
participant JSMainThread
ConsoleControlHandler->>WinSignalSlot: increment pending signal count
ConsoleControlHandler->>JSMainThread: notify main thread
JSMainThread->>WinSignalSlot: drain pending signals
sequenceDiagram
participant js_process_kill
participant win_process_kill
participant WindowsProcessAPI
js_process_kill->>win_process_kill: pass pid and signal
win_process_kill->>WindowsProcessAPI: probe or terminate process
WindowsProcessAPI-->>win_process_kill: return Win32 status
win_process_kill-->>js_process_kill: return Node errno code
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
f8bd710 to
b5a9974
Compare
Fixes two Windows defects from the 2026-07-18 audit of
crates/perry-runtime/src/os/signal.rs:process.on('SIGINT'|...)was a#[cfg(not(unix))]no-op — no console control handler was ever installed, so Ctrl+C never reached JS and graceful shutdown was impossible on Windows.process.kill(pid, sig)silently discarded its arguments and returnedtrue.Mechanism
Signal delivery mirrors the Unix structure in the same file (per-signal
pending/listenersatomics + main-thread drain):SetConsoleCtrlHandlerinstallswin_console_ctrl_handleron first listener registration (set_process_signal_listener_count > 0for a console-mapped signal). It stays installed for the process lifetime; a per-slot listener-count check inside the handler restores default behavior when the last listener is removed (no unregister, so there is no install/remove race).pendingatomic and callsjs_notify_main_thread()(atomics + condvar, documented safe from any thread) — no JS heap access, no JS calls. The existing platform-neutral drain (js_process_signal_drain→take_pending_process_signals()→emit_process_event) then fires the JS listeners on the main thread with its thread-local arena, exactly as on Unix.has_active_process_signal_listeners()gains the same pending-gated keep-alive as Unix, so a delivered signal holds the loop open until its callbacks run, while mere listener registration stays ref-neutral.Node/libuv parity mapping:
CTRL_C_EVENT'SIGINT'CTRL_BREAK_EVENT'SIGBREAK'CTRL_CLOSE_EVENT'SIGHUP'(handler thread then parks, libuv-style, so the main thread gets the full ~5s grace window instead of dying the instant the handler returns)'SIGTERM'registerable but source-less (Node allows it; it never fires from the console)Handler TRUE/FALSE semantics: returns TRUE (handled) iff at least one JS listener is registered for the mapped signal at that moment; otherwise FALSE so default termination is preserved — plain Ctrl+C on a program with no listener still kills it, and removing the last listener restores default behavior.
process.kill(pid, sig)mirrors libuv'suv_kill(src/win/process.c), via a JS-free testable corewin_process_kill:sig 0→ existence probe:OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION)+GetExitCodeProcess; onlySTILL_ACTIVEcounts as alive (an exited process whose pid is pinned by an open handle reportsESRCH).SIGHUP/SIGINT/SIGQUIT/SIGKILL/SIGTERM→OpenProcess(PROCESS_TERMINATE)+TerminateProcess(h, 1), including libuv's already-exited special case (ERROR_ACCESS_DENIED+!STILL_ACTIVE→ESRCH).0..NSIG→ENOSYS; out of range →EINVAL.pid 0targets the current process via the pseudo-handle (noCloseHandle); real handles are always closed.throw_kill_error_code):Error("kill <CODE>")with.code/.syscall = "kill"—ESRCHfor a nonexistent pid,EPERMfor access denied.Win32 access uses the crate's existing binding style:
windows-sys 0.61(added theWin32_System_Threadingfeature;Win32_System_Console/Win32_Foundationwere already enabled).Tests
#[cfg(windows)]unit tests inos/signal.rs(run on this Windows 11 x64 / MSVC box viacargo test -p perry-runtime --release signal):win_kill_terminates_child_and_sig0_tracks_liveness— spawnscmd /c pause, probes alive (sig 0 → Ok), SIGTERM-kills it, asserts exit code 1, then asserts sig-0 and terminate both reportESRCHwhile theChildhandle pins the pid (no pid-reuse flake window).win_kill_reports_esrch_for_nonexistent_pid,win_kill_rejects_unsupported_signals(ENOSYS/EINVAL).win_console_ctrl_handler_semantics— drives the control handler directly: FALSE with no listeners (for all mapped + unmapped events), TRUE with a SIGINT listener, exactly one pendingSIGINTdrained viatake_pending_process_signals(), keep-alive gate flips accordingly, and removing the last listener restores FALSE + drops undelivered pendings.win_sigterm_registration_is_accepted_and_inert.A live
GenerateConsoleCtrlEventround-trip is deliberately not a unit test: group 0 would hit every process sharing the test console (cargo, the shell), and a fresh process group needs a compiled child binary — too flaky for CI.Manual verification
Live console delivery (not exercised by unit tests; needs a perry-compiled child so the ctrl event lands in our handler, in its own process group so nothing else on the console is hit):
cargo run --release -- sigtest.ts -o sigtest.exegot SIGINT, exits 43; comment out the SIGINT listener → Ctrl+C terminates immediately (handler returns FALSE, default preserved).Step 1–3 were not run end-to-end for this PR (they need a full compiler build on a box where main's pre-existing Windows breakage is patched); the handler's mapping/return/marshal logic is covered deterministically by the unit tests above.
Notes
ExitProcessbreakage (fixed by fix(runtime): declare ExitProcess as never-returning on Windows (build break) #6609) was applied locally only and is not part of this PR.Summary by CodeRabbit