Windows: daemon stop must taskkill the whole process tree (/T)#53
Open
danielhertz1999-bit wants to merge 2 commits into
Open
Windows: daemon stop must taskkill the whole process tree (/T)#53danielhertz1999-bit wants to merge 2 commits into
danielhertz1999-bit wants to merge 2 commits into
Conversation
`iai_mcp daemon stop` on Windows could leave a child interpreter alive and still holding the hippo lock. The stop path sent os.kill(pid, SIGINT) -- which on Windows is not a signal but a TerminateProcess against the parent process only. That orphans any children the daemon spawned, and because the parent then exits immediately the subsequent poll loop returned before the `taskkill /F /PID` escalation (itself childless) ever ran. Replace the SIGINT-then-escalate dance with a single `taskkill /F /T /PID`, which terminates the daemon together with its child processes as one tree while that tree is still intact. schtasks /End still runs first. POSIX (macOS/Linux) stop paths are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Windows stop path had no coverage (existing stop tests are macOS/Linux or @skipif-SIGKILL). The new test drives cmd_daemon_stop with platform=Windows and asserts it issues taskkill /F /T /PID (tree-kill) after schtasks /End and never falls back to os.kill, which orphans the child tree. Verified it fails against the pre-fix source. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
17 tasks
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.
Problem
iai_mcp daemon stopon Windows can leave a child process alive and still holding the hippo DB lock, so a subsequent restart wedges.The Windows stop path did:
Two problems compound:
os.kill(pid, SIGINT)is not a signal — Python maps any non-CTRL_*value toTerminateProcess, which hard-kills the parent process only. Any child the daemon spawned (a worker interpreter, a maturin build, etc.) is orphaned and keeps running — and keeps its handle on~/.iai-mcp/hippo/.lock.TerminateProcesskills the parent immediately, the poll loop sees the pid gone and returns before thetaskkillescalation runs. And even when it did run,taskkill /F /PID(no/T) only kills that one pid — by then childless.Net effect: a "stopped" daemon can leave an orphan holding the lock, which then blocks the next daemon from acquiring it.
Fix
Replace the SIGINT-then-escalate sequence with a single
taskkill /F /T /PID <pid>./Tterminates the daemon together with its child process tree, while that tree is still intact.schtasks /Endstill runs first. This is a hard kill, but the old path was already a hard kill on Windows (TerminateProcess) — no graceful-shutdown behavior is lost, and the tree is now handled correctly.POSIX (macOS/Linux) stop paths are untouched.
Test
The Windows stop path previously had zero coverage (existing stop tests are macOS/Linux, or
@skipifno-SIGKILL). The new test drivescmd_daemon_stopwithplatform.system() == "Windows"and asserts it issuestaskkill /F /T /PIDafterschtasks /End, and never callsos.kill(which would orphan the tree). Verified both ways: passes on the fix, fails against the pre-fix source (which shows theos.kill(SIGINT)+taskkill /F /PIDsequence).