diff --git a/headroom/_subprocess.py b/headroom/_subprocess.py index e3adf9be4..f563826ab 100644 --- a/headroom/_subprocess.py +++ b/headroom/_subprocess.py @@ -1,7 +1,37 @@ +import os import subprocess as _sp from typing import Any +def pid_alive(pid: int) -> bool: + """Return True if ``pid`` names a live process (Windows-safe). + + ``os.kill(pid, 0)`` is the usual Unix liveness probe, but on Windows it can + fail against a detached/stale/invalid PID with ``WinError 87`` ("The + parameter is incorrect"). CPython sometimes surfaces that as a + ``SystemError`` rather than an ``OSError``; since ``SystemError`` is not an + ``OSError`` subclass, a bare ``except OSError`` lets it escape and crash the + caller — and in the detached-agent path it took down the supervised proxy + (issue #1544). Prefer ``psutil.pid_exists`` and treat ``SystemError`` as + "not alive". + """ + if pid <= 0: + return False # non-positive PIDs are never valid liveness targets + try: + import psutil # type: ignore[import-untyped] # optional dep, already used elsewhere + + return bool(psutil.pid_exists(pid)) + except Exception: + pass + try: + os.kill(pid, 0) + except PermissionError: + return True # exists but owned by another user + except (ProcessLookupError, OSError, SystemError): + return False + return True + + def run(*args: Any, **kwargs: Any) -> _sp.CompletedProcess: if kwargs.get("text") or kwargs.get("universal_newlines"): kwargs.setdefault("encoding", "utf-8") diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index 07b42e6ed..0daf7a432 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -33,7 +33,7 @@ from collections.abc import Callable from pathlib import Path from typing import Any, cast -from headroom._subprocess import run +from headroom._subprocess import pid_alive, run # Fix Windows cp1252 encoding — box-drawing characters require UTF-8 if sys.platform == "win32" and hasattr(sys.stdout, "buffer"): @@ -3057,27 +3057,12 @@ def _unregister_proxy_client(port: int) -> None: def _pid_alive(pid: int) -> bool: - """Return True if ``pid`` names a live process.""" - if pid <= 0: - return False # non-positive PIDs are never valid client markers - try: - import psutil # type: ignore[import-untyped] # optional dep, already used elsewhere + """Return True if ``pid`` names a live process. - return bool(psutil.pid_exists(pid)) - except Exception: - pass - try: - os.kill(pid, 0) - except PermissionError: - return True # exists but owned by another user - except (ProcessLookupError, OSError, SystemError): - # On Windows, os.kill against a stale/invalid PID can fail with WinError - # 87 ("The parameter is incorrect"); CPython sometimes surfaces this as a - # SystemError rather than an OSError. SystemError is not an OSError - # subclass, so a bare `except OSError` lets it escape and crash cleanup(), - # leaving the shared proxy running. - return False - return True + Thin wrapper over the shared Windows-safe helper so the marker-cleanup path + and the install/runtime status path use one liveness probe (see #1544). + """ + return pid_alive(pid) def _marker_pid_reused(marker: Path, pid: int) -> bool: diff --git a/headroom/install/runtime.py b/headroom/install/runtime.py index ec0d3f08f..02a979fea 100644 --- a/headroom/install/runtime.py +++ b/headroom/install/runtime.py @@ -13,7 +13,7 @@ from contextlib import contextmanager from pathlib import Path from typing import Any, cast -from headroom._subprocess import run +from headroom._subprocess import pid_alive, run from .health import probe_ready from .models import DeploymentManifest, InstallPreset, RuntimeKind @@ -321,7 +321,8 @@ def stop_runtime(manifest: DeploymentManifest) -> None: return try: os.kill(pid, signal.SIGTERM) - except OSError: + except (OSError, SystemError): + # SystemError covers the Windows WinError 87 surfacing described in #1544. pass _clear_pid(manifest.profile) @@ -351,8 +352,7 @@ def runtime_status(manifest: DeploymentManifest) -> str: pid = _read_pid(manifest.profile) if pid is None: return "stopped" - try: - os.kill(pid, 0) - except OSError: - return "stopped" - return "running" + # Windows-safe liveness probe: a bare os.kill(pid, 0) here raised WinError 87 + # as a SystemError against the detached agent, crashing status and taking the + # live proxy down with it (#1544). + return "running" if pid_alive(pid) else "stopped" diff --git a/tests/test_install/test_runtime.py b/tests/test_install/test_runtime.py index 6ad3dac83..d0c751711 100644 --- a/tests/test_install/test_runtime.py +++ b/tests/test_install/test_runtime.py @@ -4,6 +4,7 @@ import os import signal import subprocess import sys +import types from pathlib import Path from headroom.install.models import DeploymentManifest, InstallPreset @@ -466,9 +467,7 @@ def test_start_stop_wait_and_runtime_status_branches(monkeypatch, tmp_path: Path assert runtime_status(python_manifest) == "stopped" _write_pid("default", 125) - monkeypatch.setattr( - "headroom.install.runtime.os.kill", lambda pid, sig: (_ for _ in ()).throw(OSError()) - ) + monkeypatch.setattr("headroom.install.runtime.pid_alive", lambda pid: False) assert runtime_status(python_manifest) == "stopped" @@ -530,7 +529,7 @@ def test_runtime_status_reads_container_and_pid_state(monkeypatch, tmp_path: Pat pid_file = tmp_path / ".headroom" / "deploy" / "default" / "runner.pid" pid_file.parent.mkdir(parents=True) pid_file.write_text("123", encoding="utf-8") - monkeypatch.setattr("headroom.install.runtime.os.kill", lambda pid, sig: None) + monkeypatch.setattr("headroom.install.runtime.pid_alive", lambda pid: True) python_manifest = DeploymentManifest( profile="default", preset="persistent-service", @@ -544,3 +543,56 @@ def test_runtime_status_reads_container_and_pid_state(monkeypatch, tmp_path: Pat backend="anthropic", ) assert runtime_status(python_manifest) == "running" + + +def _python_service_manifest() -> DeploymentManifest: + return DeploymentManifest( + profile="default", + preset="persistent-service", + runtime_kind="python", + supervisor_kind="service", + scope="user", + provider_mode="manual", + targets=[], + port=8787, + host="127.0.0.1", + backend="anthropic", + ) + + +def test_runtime_status_reports_live_pid_without_terminating(monkeypatch, tmp_path: Path) -> None: + """#1544: status on a live detached PID stays 'running' and never signals it.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + pid_file = tmp_path / ".headroom" / "deploy" / "default" / "runner.pid" + pid_file.parent.mkdir(parents=True) + pid_file.write_text("25212", encoding="utf-8") + + def fail_kill(pid: int, sig: int) -> None: + raise AssertionError(f"status must not signal the live proxy (pid={pid}, sig={sig})") + + monkeypatch.setattr("headroom.install.runtime.os.kill", fail_kill) + monkeypatch.setattr("headroom.install.runtime.pid_alive", lambda pid: True) + + assert runtime_status(_python_service_manifest()) == "running" + assert pid_file.exists() # status left the deployment untouched + + +def test_runtime_status_survives_winerror87_systemerror(monkeypatch, tmp_path: Path) -> None: + """#1544: a WinError 87 SystemError from the liveness probe yields 'stopped', not a crash.""" + monkeypatch.setattr(Path, "home", lambda: tmp_path) + pid_file = tmp_path / ".headroom" / "deploy" / "default" / "runner.pid" + pid_file.parent.mkdir(parents=True) + pid_file.write_text("25212", encoding="utf-8") + + # Force the psutil fast-path to bail so the os.kill fallback runs... + fake_psutil = types.SimpleNamespace( + pid_exists=lambda pid: (_ for _ in ()).throw(RuntimeError("no psutil")) + ) + monkeypatch.setitem(sys.modules, "psutil", fake_psutil) + # ...where Windows surfaces WinError 87 as a SystemError, not an OSError. + monkeypatch.setattr( + "headroom._subprocess.os.kill", + lambda pid, sig: (_ for _ in ()).throw(SystemError("WinError 87")), + ) + + assert runtime_status(_python_service_manifest()) == "stopped" diff --git a/tests/test_pid_alive.py b/tests/test_pid_alive.py new file mode 100644 index 000000000..db866e169 --- /dev/null +++ b/tests/test_pid_alive.py @@ -0,0 +1,50 @@ +"""Regression tests for the Windows-safe PID liveness helper (#1544).""" + +from __future__ import annotations + +import sys +import types + +from headroom._subprocess import pid_alive + + +def test_pid_alive_rejects_non_positive() -> None: + assert pid_alive(0) is False + assert pid_alive(-1) is False + + +def test_pid_alive_prefers_psutil_without_signalling(monkeypatch) -> None: + monkeypatch.setitem(sys.modules, "psutil", types.SimpleNamespace(pid_exists=lambda pid: True)) + + def boom(pid: int, sig: int) -> None: + raise AssertionError("os.kill must not run when psutil answers") + + monkeypatch.setattr("headroom._subprocess.os.kill", boom) + assert pid_alive(4321) is True + + +def test_pid_alive_systemerror_is_not_alive(monkeypatch) -> None: + """WinError 87 surfaces as SystemError on Windows; it must read as 'not alive', not crash.""" + monkeypatch.setitem( + sys.modules, + "psutil", + types.SimpleNamespace(pid_exists=lambda pid: (_ for _ in ()).throw(RuntimeError())), + ) + monkeypatch.setattr( + "headroom._subprocess.os.kill", + lambda pid, sig: (_ for _ in ()).throw(SystemError("WinError 87")), + ) + assert pid_alive(4321) is False + + +def test_pid_alive_only_uses_signal_zero(monkeypatch) -> None: + """The liveness probe must never send a real (terminating) signal.""" + monkeypatch.setitem( + sys.modules, + "psutil", + types.SimpleNamespace(pid_exists=lambda pid: (_ for _ in ()).throw(RuntimeError())), + ) + sent: list[int] = [] + monkeypatch.setattr("headroom._subprocess.os.kill", lambda pid, sig: sent.append(sig)) + assert pid_alive(4321) is True + assert sent == [0]