Skip to content

fix(windows): _process_alive misreports process state - #150

Open
tradewithmeai wants to merge 1 commit into
elara-labs:mainfrom
tradewithmeai:fix/windows-process-alive
Open

fix(windows): _process_alive misreports process state#150
tradewithmeai wants to merge 1 commit into
elara-labs:mainfrom
tradewithmeai:fix/windows-process-alive

Conversation

@tradewithmeai

Copy link
Copy Markdown

Fixes #149.

What's wrong

_process_alive() uses os.kill(pid, 0), the POSIX existence idiom. On Windows os.kill is implemented over OpenProcess rather than signals, and the function returns the wrong answer in two different ways — measured on Windows 11 / CPython 3.13:

PID state old behaviour correct
never existed raises OSError(WinError 87) — propagates out of a status check False
recently exited returns True False
0 / negative returns True False

The middle row is the reason this is worth fixing. A Windows process handle stays openable while any handle to it remains, so os.kill(pid, 0) on a just-exited PID doesn't raise — cce services status reports a crashed service as healthy, with nothing to notice. The WinError 87 crash is the loud symptom; this is the quiet one.

Non-positive PIDs slipped through too: on POSIX os.kill(0, 0) signals the caller's whole process group and returns cleanly, so pid=0 reported alive.

The change

psutil>=5.9 is already a core dependency and already imported by config.py, so this uses it rather than hand-rolling a ctypes OpenProcess + WaitForSingleObject path:

if pid <= 0:
    return False
try:
    return psutil.Process(pid).status() != psutil.STATUS_ZOMBIE
except psutil.NoSuchProcess:
    return False
except psutil.AccessDenied:
    return True   # exists, owned by another user — preserves the old PermissionError branch

Zombies are excluded deliberately: on POSIX a reaped-but-not-collected child still "exists" while not running, which is the same wrong answer in a different shape.

Tests

Three added alongside the existing max-PID case:

  • an actually-exited child reports dead — the case the current suite cannot reach. test_process_alive_dead_pid uses PID 2**22, beyond the max PID on Linux/macOS, so it only exercises "never existed"; the silent wrong answer lives in the exited-process path.
  • a known-running child reports alive — positive control, so the fix can't pass by returning False for everything.
  • 0 and negative PIDs report dead.

Verified by mutation rather than by watching them pass: against the previous implementation 5 tests in this file fail; against the fix all 23 pass.

CI

This also turns tests/test_cli_smoke.py::test_services green — it was failing for the same root cause.

One thing worth flagging separately: ci.yml runs the Windows job with || true, so both of these failures were being swallowed rather than surfaced. That mask was hiding a real defect, not runner flakiness. Removing it is out of scope here, but you may want to once Windows is green.

os.kill(pid, 0) is the POSIX idiom for "does this process exist", but it does
not carry to Windows, where os.kill is implemented over OpenProcess rather
than signals. Measured on Windows 11 / CPython 3.13, the old implementation
had two distinct faults:

  1. A PID that never existed raised OSError(WinError 87, "The parameter is
     incorrect") rather than ProcessLookupError, so it matched neither except
     clause and propagated out of what is only a status check.

  2. A recently-exited PID did not raise at all. A Windows process handle
     stays openable while any handle to it remains, so os.kill(pid, 0)
     succeeded and the function returned True — reporting a dead service as
     running.

The second is the more damaging of the two: it is silent, and it makes
`cce services status` claim a crashed service is healthy. The first at least
announces itself.

Non-positive PIDs were also accepted. On POSIX os.kill(0, 0) signals the
caller's entire process group and returns cleanly, so pid=0 reported "alive".

psutil is already a core dependency (pyproject.toml) and is already imported
by config.py, so use it instead of hand-rolling a ctypes OpenProcess path.
Zombies are excluded deliberately: on POSIX a reaped-but-not-collected child
still "exists" while not running, which is fault 2 wearing a different hat.

Tests: three added alongside the existing max-PID case —

  - an actually-exited child reports dead (the case the max-PID test cannot
    reach, and the one that was silently wrong)
  - a known-running child reports alive (positive control)
  - 0 and negative PIDs report dead

Verified by mutation: against the previous implementation 5 of the tests in
this file fail; against the fix all 23 pass. This also turns
tests/test_cli_smoke.py::test_services green, which was failing for the same
reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows: _process_alive() reports dead services as running, and crashes on unknown PIDs

1 participant