fix(runner): never let a hung check hang the machine - #18
Merged
Conversation
execute() drained the child's stdout through a pipe. Every descendant inherits the pipe's write end, so communicate() blocks until *all* of them exit: killing the direct child (uv) left pytest and its workers holding it, and ckdn waited on EOF forever while the orphans burned CPU. Interrupting produced an empty run directory, because artifacts were only written after the subprocess returned. - Stream stdout/stderr straight into full.log. No pipe, no deadlock, and partial evidence survives an interrupt. - Start the child in its own process group (POSIX session / Windows CREATE_NEW_PROCESS_GROUP) and terminate the whole tree on timeout, on SIGINT, and on any other path: TERM -> grace -> KILL. - rc=130 plus an additive `interrupted: true` digest field. Interruption outranks every other signal in reconcile, so partial evidence can never be read as fail or parse_mismatch. ckdn.digest/2 is unchanged. - Alias and --all sequences stop on interrupt; the CLI exits 130 instead of a traceback. - Serialize runs per (runs_dir, check): a second concurrent run of the same check is refused, stale locks reclaimed. .locks is bookkeeping and is never listed or pruned as a run. 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 |
`os.kill(pid, 0)` cannot answer "is this pid alive?" on Windows: a dead pid raises a bare OSError (WinError 87), not ProcessLookupError, so _pid_alive read every stale lock as live. A check whose run died uncleanly could then never be run again in that workspace — the lock was permanent. Ask the kernel directly instead (OpenProcess + GetExitCodeProcess), and pin both directions with a test that runs on every OS. The test helper now shares that liveness check rather than assuming POSIX signal semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The existing tree test had the child sleep alongside its grandchild, so it never exercised the sharpest case: a child that exits *immediately* and leaves a grandchild holding the inherited stdout. That is the pure pipe-EOF deadlock — waiting on the child is not enough. Measured on the old implementation the call returned after 30.1s (the grandchild's lifetime); on the new one, 0.08s. The test bounds it at 10s. Also pin the zero-byte lock: a crash between creating the lock file and writing the pid into it leaves an unreadable holder, which must be read as stale rather than wedging the check with no process to blame. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The assertions already passed on Windows; only the grandchild cleanup raised AttributeError. os.kill maps any non-CTRL signal to TerminateProcess there, so SIGTERM is the portable choice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A run killed with SIGKILL executes no cleanup, so its process tree can outlive it. Nothing can prevent that — but the next run of the same check can say so instead of staying silent: reclaiming a stale lock now records a note that the previous run did not exit cleanly and may have left processes behind. Deliberately advisory, and deliberately not a kill: - it describes the *previous* run, so it never changes this run's status (a green run stays green); - ckdn stops nothing on its own. Only its own pid is ever written to the lock, never the child's process group, and a recycled pid would make an automatic kill land on an unrelated process. Killing on ambiguous evidence is worse than leaving an orphan; - when the lock is unreadable the note says "an earlier ckdn" rather than inventing a holder. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Process lifecycle: - terminate_tree escalated by watching the direct child, so a wrapper that dies promptly on SIGTERM ended the wait and SIGKILL was never sent. A TERM-immune grandchild survived the run while the digest claimed the tree had been terminated — a false statement in evidence, in the exact class of the original incident. Escalation now watches the process group, and the group is terminated on every path including a clean exit. - Popen.wait(None) blocks in WaitForSingleObject(INFINITE) on Windows, where KeyboardInterrupt is only raised between bytecodes: Ctrl-C was ignored outright for any check without a timeout, strictly worse than before this branch. The wait polls now, and the interrupt test drives a real interrupt_main instead of monkeypatching wait. - A second Ctrl-C in the grace window, or one during parsing or the evidence write, escaped the handlers and left a run directory with only full.log. Locks are kernel file locks (flock / msvcrt.locking) instead of a pid file. The old protocol handed one check to two runs through three races, could not see a second thread of ckdn's own process (how the MCP server runs checks), wedged a check permanently on a pid too wide for a C int (OverflowError is not an OSError), and turned a failed unlink into a permanent false "did not exit cleanly" note. Releasing empties the record rather than deleting it. Evidence and contracts: - meta's log_sha256/log_bytes cover full.log's real bytes; they were taken over decoded text, which collapses CRLF, so an external sha256 disagreed for almost any Windows tool's output. - A timeout reconciles to error, as the status model already documented. - Interruption outranks an earlier red member in an alias, and the aggregate carries the marker; it exited 1 while the CHANGELOG promised 130. - ckdn baseline refuses an interrupted or untrusted run instead of silently overwriting the accepted findings and exiting 0. - prune skips run directories without a digest, so retiring one check's runs cannot delete another check's run mid-write. - AppError is a message with exit 2 on every command, not a traceback and exit 1 — the code that means "this check is red". - The CHANGELOG no longer claims ckdn.digest/2 is unchanged (the packaged schemas are closed, so a pinned 1.2.0 validator rejects the new fields), and the docs state what termination actually guarantees instead of promising that nothing outlives a run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two Windows-only breaks in the remediation's tests: - signal.SIGKILL does not exist there, and graceful-then-forceful escalation is POSIX signal semantics in the first place — Windows terminates the tree unconditionally. The SIGTERM-immunity test is POSIX-only with that reason. - msvcrt.locking is a *mandatory* lock, so reading the lock file through a second handle while it is held raises PermissionError. The recorded holder is now proven end-to-end instead: another process takes the lock, is killed, and the next acquire must name it. The file is only read after release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A uv venv's python.exe on Windows is a trampoline: the parent sees the launcher's pid while os.getpid() inside is the interpreter's, so asserting on Popen.pid compared two different processes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Lock file names are injective. Sanitizing mapped every unsafe character to `_`, so `py.test` and `py_test` shared one lock: each refused to start while the *other* ran, and reported the other as a run that did not exit cleanly. The readable part stays; a digest of the original name makes it unique. - `latest` is published by rename. Unlink-then-create left a window with no pointer at all, and two runs finishing together could both unlink and race to create — the loser falling back to the marker, leaving two pointers that disagreed. Note LATEST_LINK and LATEST_FILE are the same path on a case-insensitive filesystem, so clearing a stale marker checks `is_file`. - Windows `_pid_alive` reported ACCESS_DENIED as dead, although that is the one answer proving the process exists. The POSIX branch has always said alive for the same case. - The interrupt test waited a fixed 1.5s before interrupting, which is a coin flip on a loaded runner. It now waits for the child's own output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
orenlab
force-pushed
the
fix/process-lifecycle
branch
from
July 22, 2026 15:21
bd60437 to
4328da3
Compare
Renaming the symlink into place fails on Windows with ACCESS_DENIED: os.replace cannot replace a *directory*, and a run-dir symlink is one. Worse, `LATEST` and `latest` are a single path on a case-insensitive filesystem, so the marker fallback then tried to replace that same directory with a file and failed too. Symlinks there need privilege anyway and the marker is already the documented Windows pointer, so publish only the marker on Windows. POSIX keeps the atomic symlink swap, which is where the concurrent-update race this fixes can actually happen. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Windows publishes the marker by design now, so "no marker file" was only ever the POSIX half of the invariant. What must hold everywhere is that one form is present and never two that could disagree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An interrupt during parsing marked the outcome interrupted but kept the command's own exit code, so a single `ckdn run` produced a digest saying `"rc": 0, "interrupted": true` and exited 1 — while the status model and the CHANGELOG entry directly above it promise 130. Alias and --all were already right, which is why it hid. The command's own code is not lost: the run's notes record it. Two related gaps from the same review: - The interrupt-tolerant step now builds the documents as well as writing them. A Ctrl-C inside reconcile or build_digest abandoned a run directory that had a log and no digest — the incident's symptom, one stage earlier. - Its retry holds SIGINT off instead of racing the next keypress, so two fast Ctrl-C cannot lose the evidence either. A worker thread cannot set handlers, so there it degrades to a plain retry. The parse-interrupt test asserted only `interrupted`, which is precisely why the exit-code contract broke unnoticed; it now pins rc and exit_code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
82 lines of incident narrative down to 17: one line per user-visible change. Mechanism, rationale and trade-offs live in the docs and the commit messages. What a reader needs on upgrade stays — rc=130, the new `interrupted` field, and re-exporting the closed schemas. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Found by running ckdn over codeclone's 4422-test suite: every interrupt cost the full five seconds and finished with SIGKILL, even when the tool had already exited. The direct child stays a zombie until reaped, and a zombie is still a member of its process group, so `killpg(pgid, 0)` kept answering "alive" for the whole grace window. The escalation therefore fired every time — SIGKILLing tools precisely to avoid which the grace period exists. The termination poll now reaps the child as it goes. ckdn's own suite dropped from ~25s to ~10s, which is the same bug seen from the inside. Measured after the fix, interrupting the codeclone suite: 0.07s where the tree stops on SIGTERM, and 5.1s in the phases where it does not — there the SIGKILL is doing its job, and nothing survives either way. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Its docstring said the run "exits 130 either way", which holds only when the interrupt already happened during execute or parsing. A *first* Ctrl-C landing inside the finalize step of a check that ran to completion is absorbed, and the run reports what the check did — a green one still exits 0. That is the right trade: by then the work is done and only the paperwork is left, and a finished result is worth more than a keypress that arrived after it. But it has to be stated, not glossed. Behaviour unchanged; the absorbed case is now pinned by a test, so the documented trade-off is the tested one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
orenlab
added a commit
that referenced
this pull request
Jul 23, 2026
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
added a commit
that referenced
this pull request
Jul 23, 2026
* docs(status-model): name the Windows termination limits 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> * fix(runner): stop the tree on Windows the way POSIX does 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> * test(runner): prove the Windows job object is really used 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> * fix(runner): watch the job, not the wrapper, when stopping on Windows 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> * fix(_win32): close the review's tail - 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> * chore: tidy the tail so nothing needs a follow-up - 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> * fix(_win32): close the job even when Ctrl-C lands in the grace 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> --------- 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.
The incident
A
ckdn runon a heavy check hung the machine — twice. The reported symptoms were orphaned processes and empty run directories at 14:44 and 14:55.The root cause is one level deeper than orphan cleanup.
execute()read the child's output through a pipe, and a pipe's write end is inherited by every descendant.communicate()waits for EOF, which arrives only when all holders exit. Killing the direct child (uv) leftpytestand its workers holding the write end, so ckdn waited forever while the orphans kept burning CPU. Because artifacts were written only after the subprocess returned, interrupting left a run directory with no log, no meta, no digest — the two empty directories.The runner's own docstring promised "a digest must exist for every attempt". The code broke that promise on exactly the paths where evidence matters most.
The fix
stdout/stderrstream straight intofull.log. No pipe means no descendant can hold the parent hostage, and partial evidence survives an interrupt.CREATE_NEW_PROCESS_GROUP), and the whole tree is terminated on timeout, onSIGINT, and on any other path:SIGTERM→ 5s grace →SIGKILL. Afinallyguard makes this hold even on an unexpected exception.rc=130plus an additiveinterrupted: truedigest field — a reason, exactly liketimed_out.ckdn.digest/2is unchanged and older consumers still just seeerror. Interruption outranks every other signal in reconcile, so partial evidence can never be misread asfailorparse_mismatch.--allruns don't start the next check; the CLI exits130instead of printing a traceback.(runs_dir, check). A second concurrent run of the same check is refused rather than doubling load on the same tools; stale locks are reclaimed..locksis bookkeeping and is never listed or pruned as a run.os.kill(pid, 0)cannot answer "is this pid alive?" there — a dead pid raises a bareOSError(WinError 87), notProcessLookupError— so every stale lock read as live and a check whose run died uncleanly could never be run again in that workspace. Liveness now goes throughOpenProcess+GetExitCodeProcess.SIGKILLexecutes no cleanup, so this is the only honest signal left. It is strictly advisory — it describes the previous run and never changes this run's status — and ckdn stops nothing on its own. Only its own pid is ever written to the lock, never the child's process group, and a recycled pid would make an automatic kill land on an unrelated process; killing on ambiguous evidence is worse than leaving an orphan.Verification
An end-to-end gate drives the real CLI as a subprocess under hard time bounds and inspects what lands on disk — nothing is taken from ckdn's own reporting.
rc=124in 3.1s, grandchild deadSIGINTleaves complete evidencerc=130in 0.28s;digest.json+meta.json+full.log;status=error,interrupted: trueSIGKILLof ckdn / a zero-byte lockThe third row is measured, not asserted: on the old implementation the call returned after 30.1s (exactly the surviving grandchild's lifetime) against 0.08s now. That case is pinned by a new test bounded at 10s, so the deadlock cannot come back unnoticed.
257 tests pass.
ruff,mypy,ty, and the docs build are clean, on Linux and on Windows.Known limitation, by design
kill -9on ckdn itself executes no cleanup, so its process tree survives. There is no portable "die with my parent" (PR_SET_PDEATHSIGis Linux-only). Reaping it automatically was rejected deliberately: the lock would have to record a pgid, and a recycled pid would make ckdn kill an unrelated process group. The run lock is still reclaimed, so a rerun is never blocked — and it now warns instead of staying silent.Release note
1.3.0 was merged to
mainbut never tagged and never published to PyPI, so no user has ever seen it. The changelog entries are folded into[1.3.0]rather than shipped as a follow-up patch — that way no published version of ckdn ever contains this defect.🤖 Generated with Claude Code