Repository navigation
fix(*): ask Windows whether a pid is alive instead of terminating it - #458
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new helper uses the documented Windows process-handle contract correctly: it requests only SYNCHRONIZE, treats a zero-timeout WAIT_TIMEOUT as alive, treats a signaled process handle as exited, preserves access-denied processes as alive, and always closes an opened handle. The POSIX path preserves the prior behavior, and keeping the CLI wrapper preserves the existing caller and test patch points.
I covered the full diff, AGENTS.md/CLAUDE.md and CONTEXT-MAP.md constraints, both CLI callers, the related upgrade-helper implementation and history, backward compatibility, test integrity, and package-boundary implications. I found no weakened tests or architectural violation.
Verification I ran: uv run pytest tests/test_utils_pid.py tests/test_cli_serve_commands.py -q -> 80 passed in 10.29s. The currently failing GitHub checks point only at untouched web-search files, so they do not identify a defect in this patch.
os.kill(pid, 0) is the POSIX existence probe. On Windows CPython implements os.kill as TerminateProcess, so signal 0 is not a probe there at all, and it fails in two directions depending on what the caller may do to the target: - a process the caller can terminate, which is any child of it, is killed and the call returns cleanly, so the probe reports "alive" about a process it has just destroyed; - a process it cannot, such as the detached `raven web` supervisor, raises OSError with ERROR_INVALID_PARAMETER, so the probe reports "dead" about a process that is running. The second is why `raven web --stop` did nothing on Windows. _read_web_state and _read_serve_pid both answered None, so _stop_resident signalled neither the supervisor nor the gateway, and printed "nothing to stop" while the engine went on serving the page. raven/utils/pid.py holds the platform question now. Windows asks OpenProcess plus a zero-timeout WaitForSingleObject, the pair raven/updates/upgrade.py already uses to watch a parent exit: a handle that fails to open for any reason but ERROR_ACCESS_DENIED is a dead process, and one that opens and then times out is a live one. The wait rather than GetExitCodeProcess because a process whose real exit code is 259 cannot be told apart from STILL_ACTIVE. _pid_alive keeps its name, since _gateway_page imports it and the stop tests patch it; only the answer moves. The test drives real subprocesses. Survival is asserted by a wait that must time out, not by a poll taken straight after the probe: termination is asynchronous, and the immediate read still says "running" for a process already on its way down. Forcing the POSIX branch on every platform fails two of the four cases. Four other sites carry the same probe and are left for their own change. The sharpest is raven/agent/tools/background_exec.py, which reaches it on the Windows branch specifically, against the agent's own background tasks. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
0af0be5 to
eb9c857
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision is the previously reviewed patch rebased onto the latest main; git range-diff reports the patch as unchanged. I rechecked the full diff and callers against the updated base, including repository rules, compatibility, test integrity, and architecture constraints, and found no new issue.
Verification: uv run pytest tests/test_utils_pid.py tests/test_cli_serve_commands.py -q -> 80 passed in 7.32s. Current lint and completed platform checks are green; the unit shards are still running.
The PR diff-coverage gate reads 50 percent on this branch: twenty of the forty changed lines are the Windows probe, and the runner that measures coverage is Linux, where none of them can execute. Most of those lines are not platform binding at all, they are the decision the binding feeds: which refusal means the process is there, which wait result means it is running, and that the handle is closed on every path. _win32_api now holds the ctypes declarations, which cannot load off Windows and carry no logic, and _alive_windows holds the decision and asks for the pair. A fake kernel32 then drives all four cases anywhere, including the access-denied one no test could reach before: it needs a process owned by another account. The two POSIX branches are covered the same way, by refusing the signal rather than by finding a process that would. The real subprocess tests stay as they are and remain the ones that catch the original defect; a mocked probe would not have. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new revision cleanly isolates the platform binding from the already-reviewed liveness decision. The fake API exercises access denial, missing processes, both wait outcomes, and handle cleanup, while the real subprocess test remains unchanged, so coverage improved without weakening the regression test. I rechecked the full diff, callers, repository rules, compatibility, history, test integrity, and architecture constraints.
Verification: uv run pytest tests/test_utils_pid.py tests/test_cli_serve_commands.py -q -> 87 passed in 6.66s. The completed lint and platform checks are green; unit shards are still running.
|
**Not a blocker -- the shipped probe is correct, and I verified it. This is about one line of the new test file that records a value and never asserts it, so a specific class of future edit to the Windows branch would ship green.
self.opened = (access, inherit, pid)and no test in the file ever reads assert kernel32.waited == (99, 0), "the wait has to be the zero-timeout one, or the probe blocks"
assert kernel32.closed == [99], "an opened handle leaks unless every path closes it"Two of the three calls are pinned; the third is recorded and dropped. That matters more here than it normally would, because this fake is the entire coverage of the Windows decision -- the file says so itself at :66-70 ("the coverage platform has no kernel32 at all, so the decision is asked here"), and no CI runner executes What I measuredAll via
The control is the important half: it dies, so the suite really does reach The survivor is not equivalent. On Windows a probe that asks about its own process answers "alive" for every pid forever, so One line closes it, in the file's own idiom: assert kernel32.opened == (_SYNCHRONIZE, False, 4321), "the probe must ask about the caller's pid, with no more access than SYNCHRONIZE"I have no Windows host; the consequences above are read from the Win32 semantics this module itself encodes at What held up
|
…458) ## Summary `os.kill(pid, 0)` is the POSIX existence probe. On Windows, CPython implements `os.kill` as `TerminateProcess`, so signal 0 is not a probe there at all, and it fails in two directions depending on what the caller may do to the target: - a process the caller can terminate, which is any child of it, is killed and the call returns cleanly, so the probe reports "alive" about a process it has just destroyed; - a process it cannot, such as the detached `raven web` supervisor, raises `OSError` with `ERROR_INVALID_PARAMETER`, so the probe reports "dead" about a process that is running. The second is why `raven web --stop` did nothing on Windows. `_read_web_state` and `_read_serve_pid` both answered `None`, so `_stop_resident` signalled neither the supervisor nor the gateway, and printed "nothing to stop" while the engine went on serving the page. `raven/utils/pid.py` holds the platform question now. Windows asks `OpenProcess` plus a zero-timeout `WaitForSingleObject`, the pair `raven/updates/upgrade.py` already uses to watch a parent exit: a handle that fails to open for any reason but `ERROR_ACCESS_DENIED` is a dead process, and one that opens and then times out is a live one. The wait rather than `GetExitCodeProcess`, because a process whose real exit code is 259 cannot be told apart from `STILL_ACTIVE`. `_pid_alive` keeps its name in `serve_commands`, since `_gateway_page` imports it and the stop tests patch it; only the answer moves. Four other call sites carry the same probe and are left for their own change. The sharpest is `raven/agent/tools/background_exec.py`, which reaches it on the Windows branch specifically, against the agent's own background tasks. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Run on Windows 11, in a clean worktree cut from `main`: - `uv run pytest tests/test_utils_pid.py tests/test_cli_serve_commands.py -q`: 77 passed, 3 failed. - the same 3 failures reproduce on `main` without this change (`test_the_state_file_stays_owner_only`, `test_sigterm_removes_the_state_file`, `test_sigterm_during_the_prologue_still_removes_the_state_file`): they are pre-existing Windows gaps in the supervisor tests, unrelated to this PR, and this branch adds no new failure. Baseline on `main` is 73 passed, 3 failed; the 4 extra passes are the new `tests/test_utils_pid.py`. The new test drives real subprocesses. Survival is asserted by a wait that must time out, not by a poll taken straight after the probe: termination is asynchronous, and the immediate read still says "running" for a process already on its way down. Forcing the POSIX branch on every platform fails two of the four cases. - [x] Relevant tests pass locally - [ ] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk POSIX behaviour is unchanged: the helper keeps calling `os.kill(pid, 0)` there. On Windows, `raven web --stop` starts working and stops silently terminating child processes it only meant to look at, which is the point of the change. Rollback is reverting to the inline `os.kill(pid, 0)` in `raven/cli/serve_commands.py` and dropping `raven/utils/pid.py`. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues N/A --------- Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Summary
os.kill(pid, 0)is the POSIX existence probe. On Windows, CPython implementsos.killasTerminateProcess, so signal 0 is not a probe there at all, and it failsin two directions depending on what the caller may do to the target:
returns cleanly, so the probe reports "alive" about a process it has just destroyed;
raven websupervisor, raisesOSErrorwith
ERROR_INVALID_PARAMETER, so the probe reports "dead" about a process that isrunning.
The second is why
raven web --stopdid nothing on Windows._read_web_stateand_read_serve_pidboth answeredNone, so_stop_residentsignalled neither thesupervisor nor the gateway, and printed "nothing to stop" while the engine went on
serving the page.
raven/utils/pid.pyholds the platform question now. Windows asksOpenProcessplusa zero-timeout
WaitForSingleObject, the pairraven/updates/upgrade.pyalready usesto watch a parent exit: a handle that fails to open for any reason but
ERROR_ACCESS_DENIEDis a dead process, and one that opens and then times out is alive one. The wait rather than
GetExitCodeProcess, because a process whose real exitcode is 259 cannot be told apart from
STILL_ACTIVE._pid_alivekeeps its name inserve_commands, since_gateway_pageimports it andthe stop tests patch it; only the answer moves.
Four other call sites carry the same probe and are left for their own change. The
sharpest is
raven/agent/tools/background_exec.py, which reaches it on the Windowsbranch specifically, against the agent's own background tasks.
Type
Verification
Run on Windows 11, in a clean worktree cut from
main:uv run pytest tests/test_utils_pid.py tests/test_cli_serve_commands.py -q:77 passed, 3 failed.
mainwithout this change(
test_the_state_file_stays_owner_only,test_sigterm_removes_the_state_file,test_sigterm_during_the_prologue_still_removes_the_state_file): they arepre-existing Windows gaps in the supervisor tests, unrelated to this PR, and this
branch adds no new failure. Baseline on
mainis 73 passed, 3 failed; the 4 extrapasses are the new
tests/test_utils_pid.py.The new test drives real subprocesses. Survival is asserted by a wait that must time
out, not by a poll taken straight after the probe: termination is asynchronous, and
the immediate read still says "running" for a process already on its way down. Forcing
the POSIX branch on every platform fails two of the four cases.
Risk
POSIX behaviour is unchanged: the helper keeps calling
os.kill(pid, 0)there. OnWindows,
raven web --stopstarts working and stops silently terminating childprocesses it only meant to look at, which is the point of the change.
Rollback is reverting to the inline
os.kill(pid, 0)inraven/cli/serve_commands.pyand dropping
raven/utils/pid.py.Related Issues
N/A