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]