From 5ce2394adb8c340c1762270e9462660cd43e8e41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o?= Date: Wed, 30 Sep 2026 17:13:29 +0200 Subject: [PATCH] fix(browser): probe pid liveness through the platform-safe helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _process_is_alive used os.kill(pid, 0). That probe is POSIX-only: CPython's Windows os.kill calls TerminateProcess(handle, sig) for any signal other than CTRL_C/CTRL_BREAK, so it terminates the process it is asked about. This function is only reached when there is no procfs to read a command line from, which is exactly the macOS and Windows case the rest of this change exists to handle. core/platform_compat.py already owns that probe and documents the hazard; its module docstring asks callers to import from there rather than spell a POSIX-only call out locally. Delegate to it. pid_alive answers False where os.kill raises PermissionError — a live process owned by another user. Both call sites want that reading: the sweep only unlinks a pid file it wrote itself, and a pid it cannot confirm is not the daemon it is looking for. --- src/agent_tools/web_tools.py | 28 +++++++++++++++++----------- tests/test_private_browser_tool.py | 24 ++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/src/agent_tools/web_tools.py b/src/agent_tools/web_tools.py index 8c8a98621..6e974c47a 100644 --- a/src/agent_tools/web_tools.py +++ b/src/agent_tools/web_tools.py @@ -18,6 +18,7 @@ import urllib.request from pathlib import Path from typing import Dict, Any +from core.platform_compat import pid_alive from src.constants import MAX_OUTPUT_CHARS PDF_EXTRACT_MAX_BYTES = 80_000_000 @@ -143,18 +144,23 @@ def _process_command_line(pid: int) -> str | None: def _process_is_alive(pid: int) -> bool: - """Whether a pid currently exists. Signal 0 checks without delivering.""" + """Whether a pid currently exists. - try: - os.kill(pid, 0) - except ProcessLookupError: - return False - except PermissionError: - # Alive, owned by somebody else. - return True - except OSError: - return False - return True + Delegates to ``core.platform_compat.pid_alive`` rather than probing with + ``os.kill(pid, 0)`` directly. That probe is POSIX-only: CPython's Windows + ``os.kill`` calls ``TerminateProcess(handle, sig)`` for any signal other + than CTRL_C / CTRL_BREAK, so it would *kill* the daemon it is asked about. + Windows is also where there is no procfs, which is precisely when this + function gets called at all. + + ``pid_alive`` reads False for a pid that ``os.kill`` reports with + ``PermissionError`` — a live process owned by another user. Neither caller + here wants a different answer: the sweep only unlinks a pid file it wrote + itself, and treating somebody else's pid as "not our daemon" is the safe + reading in both. + """ + + return pid_alive(pid) _SCHOLARLY_METADATA_CUE_RE = re.compile( r"\b(?:accept(?:ed|ance)?|publish(?:ed|ing|cation)?|venue|conference|" diff --git a/tests/test_private_browser_tool.py b/tests/test_private_browser_tool.py index 1e003d4c5..7df8db9fe 100644 --- a/tests/test_private_browser_tool.py +++ b/tests/test_private_browser_tool.py @@ -1923,3 +1923,27 @@ def test_procfs_host_still_matches_on_the_command_line(monkeypatch, tmp_path) -> _pid_file_for(tmp_path, monkeypatch, "clawmm-test", "session-6", 6666) assert PrivateBrowserTool._owned_daemon_exists({}, "session-6") is False + + +def test_liveness_probe_goes_through_the_platform_safe_helper(monkeypatch) -> None: + """The no-procfs path must not reach a bare ``os.kill(pid, 0)``. + + CPython's Windows ``os.kill`` calls ``TerminateProcess(handle, sig)`` for + any signal other than CTRL_C / CTRL_BREAK, so probing liveness with signal + 0 terminates the process it asks about — and the only hosts that reach this + probe are the ones with no procfs, Windows among them. + ``core.platform_compat.pid_alive`` is the tree's platform-safe answer. + """ + + asked: list[int] = [] + monkeypatch.setattr( + web_tools, "pid_alive", lambda pid: asked.append(pid) or True + ) + monkeypatch.setattr( + web_tools.os, + "kill", + lambda *a, **kw: pytest.fail("os.kill must not be used to probe liveness"), + ) + + assert web_tools._process_is_alive(4242) is True + assert asked == [4242]