Repository navigation
feat: Windows support via Job Objects - #5
Merged
Merged
Conversation
On Windows only qsv.exe itself was killed on a timeout or cancellation; everything it started survived. Each run is now assigned to its own Job Object (ctypes, no new dependency): CTRL_BREAK_EVENT to its console process group first, then TerminateJobObject after kill_grace. Children cannot leave the job, unlike a POSIX process group. - _process.py: a _ProcessTree per run (POSIX session or Windows job) with interrupt/kill/close; the teardown logic is now shared. - client.py: keep SYSTEMROOT with inherit_env=False on Windows. - Tests: the fake qsv is a .py behind a .cmd wrapper on Windows; process liveness uses GetExitCodeProcess (os.kill(pid, 0) terminates there); escaped-grandchild tests assert the job killed it. - CI: full suite on Windows for Python 3.10-3.14; lint also runs mypy --platform win32. - Docs: README platform notes, CHANGELOG, Windows classifier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addresses two findings of roborev review 5163: - A second Ctrl-C during the sync grace wait raised out of _terminate_sync before the forced kill, leaving a qsv that ignores SIGTERM / CTRL_BREAK_EVENT running (all platforms). The kill now runs in a finally, followed by a bounded reap; run_sync also closes output pipes an escaping exception would have left open. - Before Python 3.13, communicate() on Windows writes stdin synchronously before checking its timeout, so a child that stops reading could block a run forever. On Windows, stdin is now fed from a thread. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addresses the remaining finding of roborev review 5163. A child qsv started between CreateProcess and AssignProcessToJobObject landed outside the job; if it also ignored CTRL_BREAK_EVENT and held the output pipes, the timeout could block on Windows' pipe reader threads until it exited. qsv is now created with CREATE_SUSPENDED, assigned to the job, and only then resumed (its main thread found through a Toolhelp snapshot, since Popen closes the thread handle). If resuming fails, the process is killed and the run raises OSError rather than leaving it suspended. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Addresses roborev reviews 5164 and 5165: a BaseException raised between spawning qsv and entering the protected block (Ctrl-C during _ProcessTree setup, or Thread.start() failing for the Windows stdin feeder) bypassed teardown, and on Windows could leave qsv suspended. - _ProcessTree (Windows) kills and closes its job on any BaseException during setup, not just a failed resume. - run_sync / run_async catch any BaseException from setup, kill the leader, reap it and close its pipes before re-raising. - The stdin feeder starts inside the protected block and detaches proc.stdin only after its thread started; run_sync's finally now also closes stdin. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On Windows the interrupting _ProcessTree stand-in replaced the code that resumes the suspended qsv, so the fake never ran and never wrote the pidfile the tests waited for. Record the pid handed to the setup and check that instead, which covers a suspended (Windows) or running (POSIX) leader alike. _wait_reaped falls back to _wait_dead on Windows, which has no waitpid/WNOHANG. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
1 participant
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
On Windows, a timeout or cancellation used to kill only
qsv.exe; anything it started kept running. Each run now goes in its own Job Object, called throughctypes, so there's still no runtime dependency:CTRL_BREAK_EVENTto the run's console process group (the polite stop, like SIGTERM)kill_grace,TerminateJobObject, which kills every process in the jobChild processes can't leave the job, unlike a POSIX process group, which
setsidescapes.Changes
_process.py: a per-run_ProcessTree(a POSIX session, or a Windows Job Object) withinterrupt/kill/close. The timeout, cancellation and KeyboardInterrupt teardown logic is now shared across platforms.client.py: withinherit_env=Falseon Windows,SYSTEMROOTis still passed on, because many programs fail to start without it..pyscript behind a.cmdwrapper. Process liveness usesGetExitCodeProcess, becauseos.kill(pid, 0)terminates the process on Windows. The escaped-grandchild tests (grandchild in its own console process group, out ofCTRL_BREAK_EVENT's reach) assert that the Job Object killed it. The symlink..test is POSIX-only, because Win32 collapses..before following symlinks.mypy --platform win32.Operating System :: Microsoft :: Windowsclassifier.Hardening from review
CREATE_SUSPENDED, assigned to its job, and only then resumed. Its main thread is found through a Toolhelp snapshot, becausePopencloses the thread handle. Nothing qsv starts can land outside the job. If resuming fails, the process is killed and the run raisesOSError.communicate()wrote stdin synchronously before checking its timeout, so a child that stopped reading could hang the run. stdin is now fed from a thread.Test plan
ruff,mypy, andmypy --platform win32are clean🤖 Generated with Claude Code