fix: guard SIGTERM grace period in shutdown on Windows (#14302) - #14316
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a Windows-only crash in Dune’s shutdown path introduced by the SIGTERM-grace-period change in #14170, by avoiding Unix-only process waiting APIs on Windows.
Changes:
- Skip the SIGTERM grace-period pre-phase on Windows and send SIGKILL directly in
kill_and_wait_for_all_processes. - Add a changelog entry documenting the Windows crash fix (#14302).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/dune_scheduler/scheduler.ml |
Guards the SIGTERM/grace-period polling loop so Windows doesn’t call Process_watcher.wait_unix; uses direct SIGKILL instead. |
doc/changes/fixed/14302.md |
Documents the Windows shutdown crash fix and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Alizter
requested changes
Apr 24, 2026
Alizter
left a comment
Collaborator
There was a problem hiding this comment.
I don't think this requires a changelog since this is a regression introduced before we release 3.23 and is blocking the release.
I've tested the patch on Windows before and after and it appears to work correctly.
Fixes ocaml#14302. PR ocaml#14170 added a SIGTERM-before-SIGKILL grace period to [kill_and_wait_for_all_processes]. The new polling loop calls [Process_watcher.wait_unix] unconditionally, which on Windows routes through [Proc.wait] and raises: Code_error "wait4 not available on windows" Since SIGTERM is not meaningful on Windows anyway (as PR ocaml#14170 itself noted), skip the grace-period pre-phase there and send SIGKILL directly. The main drain loop already handles the Windows case correctly: job exits are pushed to [jobs_completed] by the [run_win32] polling thread and decrement [pending_jobs] when popped by [Event.Queue.next]. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
force-pushed
the
robinbb-issue-14302-windows-signals
branch
from
April 24, 2026 17:14
ec8aafd to
9944251
Compare
Collaborator
Author
|
Dropped the changelog entry (commit 9944251). Thanks for the Windows validation. |
Alizter
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #14302.
PR #14170 added a SIGTERM-before-SIGKILL grace period to
kill_and_wait_for_all_processes. The new polling loop callsProcess_watcher.wait_unixunconditionally, which on Windows routes throughProc.waitand raisesCode_error "wait4 not available on windows", crashingdune on Ctrl+C.
Fix
Since SIGTERM is not meaningful on Windows anyway — as PR #14170 itself noted
("On Windows, where SIGTERM is not meaningful, behavior is unchanged") — skip
the grace-period pre-phase on Windows and send SIGKILL directly. The existing
main drain loop already handles the Windows case correctly: job exits are
pushed to
jobs_completedby therun_win32polling thread and decrementpending_jobswhen popped byEvent.Queue.next(seesrc/dune_scheduler/event.ml:222and:187-191).Changelog
Added
doc/changes/fixed/14302.md.