fix(runner): stop the process tree on Windows the way POSIX does - #19
Merged
Conversation
The page promised SIGTERM, a grace period and then SIGKILL, and listed where that guarantee stops — without mentioning that none of it holds on Windows. There the tree is killed forcefully at once, so a tool gets none of the grace a POSIX run gives it to finish its report, and taskkill's parent-link walk misses a re-parented grandchild. The same timeout can therefore produce a different digest on the two platforms, which matters for a tool that sells byte-identical digests across operating systems. Stated as a third limit rather than left for a user to discover. Closing the gap in code needs a Windows job object; the claim had to stop overreaching either way. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeClone Review✅ Passed · Health 94/100 (A) · Baseline Review snapshot
Review focus
Security Surfaces are report-only capability inventory, not vulnerability claims. Generated by CodeClone |
Windows had neither half of the contract the status model describes. `taskkill /T /F` is unconditional, so a tool got none of the grace a POSIX run gives it to finish writing its report — the same `timeout` could yield a completed report on one platform and "report not found" on the other, for a tool whose whole claim is byte-identical digests across operating systems. And `taskkill` walks parent links, so a grandchild whose parent had already exited was simply missed. Now: `CTRL_BREAK` to the child's process group, the same grace period, then a job object. The child is assigned to the job at spawn, so the tree is held by membership rather than by parentage — and the job is created with KILL_ON_JOB_CLOSE, so it dies with ckdn's handle even when ckdn is killed outright and none of our cleanup runs. That closes the `kill -9` gap on Windows, which nothing portable closes on POSIX. Every ctypes step is best effort: a failure anywhere falls back to the taskkill path this replaces, so the worst case is today's behaviour. I cannot exercise this locally — Windows CI is the only verifier, and the POSIX path is unchanged and still covered by its own tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every ctypes step falls back to taskkill, so a job that is never created passes every other test exactly like one that works — green CI would look identical either way, which is not evidence of anything. This exercises the real calls end to end: create the job, assign a live child, drop the handle, and assert KILL_ON_JOB_CLOSE took the child with it. That is also the mechanism that stops a killed ckdn leaking its tree, so it is the part most worth pinning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review found I had transplanted the sequence from POSIX but not the predicate. Three blockers, all in the same shape: - The grace loop polled the direct child. A wrapper like `uv` dies on CTRL_BREAK in milliseconds, so the wait ended at once and the job close took the real tool mid-write — a zero-length grace, and exactly the bug #18 fixed on POSIX. It now polls the job's ActiveProcesses. - `break_group`'s result was discarded, so where there is no console to deliver it — an MCP host, a service — every stop paid a full dead grace. - The job block sat inside the region whose OSError means "the command never started": a failure there lost a live child, and a Ctrl-C there left `execute` with no outcome and the run with no digest. It attaches after the handle is bound, and `execute` now has its own KI handler. The Win32 layer moves to `ckdn/_win32.py`. Ten helpers were each repeating a platform guard and both ctypes imports so type checkers would narrow the platform on the Linux job — and that preamble was itself the block clone the gate rejected. A module imported only on Windows needs none of them; the checkers are told not to report on it instead. Docs and CHANGELOG stop overclaiming: `kill -9` cover is conditional on a job existing, descendants created before the child joins the job are named as a limit, and CTRL_BREAK is described as an opportunity — its default handler calls ExitProcess, so only a tool with its own handler gets a window. The CHANGELOG compares against 1.2.0 rather than an unreleased intermediate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
orenlab
changed the title
- The status model said the child is "started detached", which reads as a recipe rather than a description. Use the runner's own wording: detached into a process group on POSIX, held in a job on Windows. - `break_group` claimed the event reaches "it and its descendants". It reaches the direct child's group; a descendant that created a group of its own does not get it — what holds that one is the job. - `pid_alive` said an exit-259 misread "only delays a lock reclaim". Locks stopped depending on pid liveness when they became file locks; the real cost is a delayed taskkill in the jobless branch. - The POSIX-only skip reason still said Windows terminates unconditionally via taskkill /F, which this branch made untrue. - The mechanism test leaked the job handle when its assign assertion failed. - `kernel32()` is cached and declares its return types once. A fresh binding per call left every call site to remember the incantation, and forgetting it truncates a 64-bit handle silently — the next helper added here would have broken quietly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Two unreachable suppresses go: `Popen.wait` does not raise ValueError, and the `delattr` is guarded by the `getattr` on the line above it — a swallow there could only ever hide a real bug. - `int()`/`bool()` that operate on values already Python ints and bools go too. The ones sitting directly on a ctypes return stay, and the module docstring now says why: the type checkers do not read `_win32`, so those casts are the only thing holding its annotations honest. - The CHANGELOG said only the real-symlink test stays POSIX-only. It is three: symlinks, and two that assert POSIX signal semantics whose Windows counterparts are tested on their own terms. - 1.3.0 is dated the day it actually ships. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The grace is exactly where an impatient second Ctrl-C arrives — the whole reason `_terminate_absorbing_interrupts` exists — and returning through that exception skipped the close: the handle leaked, KILL_ON_JOB_CLOSE never fired, and the retry with grace=0 found no job and fell back to taskkill's parent walk. That loses precisely the re-parented descendants the job was introduced to hold. Detaching the record still guards against a double close; a `finally` is what guarantees the single one happens. Two more from the same review: - An unanswerable `job_active` fell back to the direct child, which is the zero-length grace the predicate was written to remove. It now counts as alive: the cost of being wrong that way is waiting out a grace nobody needed, rather than killing a tool mid-write. - The seam test proved console delivery, not job membership — its victim could have died from CTRL_BREAK with no job at all. It now picks one neither fallback can explain: an orphaned grandchild that ignores CTRL_BREAK, so if it dies, the job took it. `terminate_tree` no longer claims to never raise; a Ctrl-C in the grace propagates, and the tree is down before it does. Co-Authored-By: Claude Opus 4.8 <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.
Not docs-only. An earlier revision of this PR said so; that was wrong and is corrected here — the branch carries the Windows process-containment implementation.
Why
The status model promises
SIGTERM→ grace →SIGKILLand lists where that guarantee stops. On Windows none of it held:taskkill /T /Fis unconditional and walks parent links, so a tool got no grace at all and a grandchild whose parent had already exited was missed. The sametimeoutcould yield a completed report on POSIX and "report not found" on Windows — a different digest, for a tool whose contract is byte-identical digests across operating systems.What
CTRL_BREAK→ grace → job object, mirroring the POSIX sequence. The child joins a job created withKILL_ON_JOB_CLOSE, so the tree is held by membership rather than parentage, and dies with ckdn's handle even when ckdn is killed outright.The Win32 layer lives in
ckdn/_win32.py: ten helpers were each repeating a platform guard and both ctypes imports purely so the type checkers narrow the platform on the Linux job, and that preamble was itself a block clone. A module imported only on Windows needs none of them.Fixed after review
The first implementation transplanted the POSIX sequence but not its predicate, which produced three defects of one shape:
uvdies onCTRL_BREAKin milliseconds, so the wait ended immediately and closing the job took the real tool mid-write — a zero-length grace, and precisely the bug fix(runner): never let a hung check hang the machine #18 fixed on POSIX. Now polls the job'sActiveProcesses.CTRL_BREAK— an MCP host, a service — every stop paid a full dead grace period.OSErrorthere lost a live child (nothing was bound, so nothing terminated it); aCtrl-Cthere leftexecutewith no outcome and the run with no digest — the incident's own symptom. The job is attached after the handle is bound, andexecutehas its ownKeyboardInterrupthandler.Claims corrected
kill -9cover on Windows is conditional on a job having been created — the layer is best effort and falls back totaskkill.Popencannot start suspended, so this is documented, not closed.CTRL_BREAKis an opportunity, not a promise: its default handler callsExitProcess, so only a tool that installs its own handler gets a shutdown window.Verification
272 passed / 3 skipped,
ruff/mypy/tyclean, CodeCloneaccepted—improved, health +1, gate would pass. The Windows path has two tests: one drives the Win32 calls end to end, the other drives the_spawn→ attach →terminate_treeseam, so deleting the attach or the close from production fails the suite rather than silently falling back.Windows CI is the only verifier for that path; I cannot execute it locally.
🤖 Generated with Claude Code