Fix watch status on Windows (#72)#73
Conversation
IsRunning probed process liveness with os.Process.Signal(0), which is unsupported on Windows: Signal returns an error for any signal other than Kill, so IsRunning always reported false and `watch status` said "Watch daemon not running" even with a live PID in .codemap/watch.pid. Move liveness detection behind a platform-specific processAlive: - Unix keeps the Signal(0) probe. - Windows opens the process and checks GetExitCodeProcess for STILL_ACTIVE. Also make Stop fall back to Process.Kill when SIGTERM is unsupported (Windows), so `watch stop` actually terminates the daemon there. Adds a host-runnable processAlive test covering live, dead, and invalid PIDs. Fixes #72 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR fixes codemap watch status on Windows by replacing the Unix-only liveness probe (Signal(0)) with a platform-specific processAlive(pid) implementation, and improves watch stop behavior on Windows where SIGTERM is unsupported.
Changes:
- Replace
IsRunning’s liveness check with a new OS-specificprocessAlive(pid)helper (Unix:Signal(0), Windows:OpenProcess+GetExitCodeProcess). - Add a new unit test covering
processAlivefor live, exited, and invalid PIDs. - Update
Stopto fall back to a hard kill whenSIGTERMis unsupported (intended for Windows).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| watch/state.go | Switches IsRunning to platform-specific liveness and adjusts Stop behavior for Windows. |
| watch/state_test.go | Adds a new processAlive test to validate live/dead PID detection. |
| watch/process_windows.go | Implements Windows liveness detection using Win32 process APIs via syscall. |
| watch/process_unix.go | Implements non-Windows liveness detection via Signal(0). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Spawn a short-lived process, wait for it to exit, then confirm it is dead. | ||
| cmd := exec.Command("go", "version") | ||
| if err := cmd.Start(); err != nil { | ||
| t.Skipf("cannot spawn helper process: %v", err) | ||
| } | ||
| pid := cmd.Process.Pid | ||
| _ = cmd.Wait() |
| if err := proc.Signal(syscall.SIGTERM); err != nil { | ||
| return err | ||
| // Windows does not support SIGTERM; fall back to a hard kill so | ||
| // `watch stop` actually terminates the daemon there. | ||
| if killErr := proc.Kill(); killErr != nil { | ||
| return killErr |
- Stop: use a platform-specific terminateProcess (SIGTERM on Unix, Kill on Windows) instead of a blanket Kill-on-any-SIGTERM-error fallback, so Unix no longer masks ESRCH/EPERM. - Test: re-exec the test binary with `-test.run=^$` for the exited-PID case instead of depending on `go` being on PATH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @copilot — both addressed in the latest commit:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abc5b9de17
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // terminateProcess stops the daemon. Windows has no SIGTERM, so Kill (which | ||
| // maps to TerminateProcess) is the correct mechanism. | ||
| func terminateProcess(proc *os.Process) error { |
There was a problem hiding this comment.
When .codemap/watch.pid is stale and its PID has been reused on Windows, the new liveness check lets runWatchSubcommand("stop") proceed for any active process, and this Kill terminates it even though processCommandLine still cannot verify that the PID belongs to a codemap daemon for this root. This is a new Windows failure mode from making IsRunning succeed there, so please verify daemon ownership before killing or refuse to stop when ownership cannot be established.
Useful? React with 👍 / 👎.
Revert the Windows Kill fallback in Stop. It let `watch stop` terminate any live process matching a stale .codemap/watch.pid — on Windows the PID could have been reused by an unrelated process, and processCommandLine cannot yet verify daemon ownership there (P2 flagged by Codex). Stop returns to its prior behavior (SIGTERM, error on Windows), so this PR is scoped to the reported issue: fixing `watch status` via the new platform-specific processAlive. Safe Windows `stop` (with PID-ownership verification) is left as tracked follow-up. This also resolves the earlier Copilot note that the blanket Kill fallback masked ESRCH/EPERM on Unix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @chatgpt-codex-connector — good catch on the PID-reuse risk. I've reverted the Windows This PR is now scoped to the reported issue — fixing |
Summary
Testing
No commit or new PR was created because there were no code changes to commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45968bc1ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := syscall.GetExitCodeProcess(h, &code); err != nil { | ||
| return false | ||
| } | ||
| return code == stillActive |
There was a problem hiding this comment.
When .codemap/watch.pid is stale and Windows has reused that PID for an unrelated live process, this returns true even though processCommandLine is still unsupported on Windows and no repo/daemon ownership is verified. runWatchSubcommand("start") checks watchIsRunning(absRoot) before spawning, so that stale file makes codemap watch start refuse to start a real daemon, while status can report running/stale state until the user deletes the PID file. Fresh evidence: Stop is now non-destructive, but start/status still rely only on this liveness result.
Useful? React with 👍 / 👎.
Fixes #72.
Problem
On Windows,
codemap watch startleaves a live daemon with a matching.codemap/watch.pid, butcodemap watch statusimmediately reportsWatch daemon not running.Root cause
IsRunningprobed liveness withos.Process.Signal(syscall.Signal(0)). On Windows,os.Process.Signalreturns "not supported by windows" for any signal exceptKill, so signal 0 always errored →IsRunningalways returnedfalse. It's a Unix idiom that never worked on Windows.Stop(SIGTERM) had the same latent problem.Fix
processAlive(pid):Signal(0)probe.GetExitCodeProcessforSTILL_ACTIVE(stdlibsyscallonly, no new dependency).Stopfalls back toProcess.Kill()whenSIGTERMis unsupported, sowatch stopalso works on Windows.processAlivetest (live / exited / invalid PIDs).Verification
GOOS=windows go buildandgo vetclean — the actual target platform.watchtests pass, including the existing status test and the new liveness test.I couldn't run this on real Windows from my environment — @ScottRosenberg2, if you're able to try a build from this branch (Win11/Scoop), confirmation would be appreciated.
🤖 Generated with Claude Code