diff --git a/src/agent_tools/web_tools.py b/src/agent_tools/web_tools.py index 84bce8d5c..4a04b8b10 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 import platform_compat from src.constants import MAX_OUTPUT_CHARS PDF_EXTRACT_MAX_BYTES = 80_000_000 @@ -123,7 +124,42 @@ def _browser_pid_file_candidates( # Linux exposes one command line per pid under /proc; macOS and Windows do not. # Kept as a module attribute so the procfs-dependent paths stay testable on a # host that has no procfs, and on one that does. -_PROC_ROOT = Path("/proc") + + +def _process_command_line(pid: int) -> str | None: + """Command line of a running process, or ``None`` when it cannot be read. + + ``None`` means "this host cannot tell", not "the process is gone". Off + Linux there is no procfs to read a command line from, so callers must not + treat it as proof that the process exited. + """ + + try: + return (platform_compat.PROC_ROOT / str(pid) / "cmdline").read_bytes().replace( + b"\0", b" " + ).decode("utf-8", errors="replace") + except (OSError, UnicodeError): + return None + + +def _process_is_alive(pid: int) -> bool: + """Whether a pid currently exists. + + 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 platform_compat.pid_alive(pid) _SCHOLARLY_METADATA_CUE_RE = re.compile( r"\b(?:accept(?:ed|ance)?|publish(?:ed|ing|cation)?|venue|conference|" @@ -2322,14 +2358,14 @@ class PrivateBrowserTool: except OSError: return profile_prefix = str(tmpdir / "agent-browser-chrome-") - if not _PROC_ROOT.is_dir(): + if not platform_compat.has_procfs(): # Without procfs there is no way to match a reparented Chrome by # its command line, and the sweep is an optimisation rather than a # correctness requirement. Leave those trees to the daemon's own # lifecycle instead of failing the whole shutdown path. return pids: list[int] = [] - for entry in _PROC_ROOT.iterdir(): + for entry in platform_compat.PROC_ROOT.iterdir(): if not entry.name.isdigit(): continue try: @@ -2359,16 +2395,18 @@ class PrivateBrowserTool: for pid_file in pid_files: try: pid = int(pid_file.read_text().strip()) - command_line = (Path("/proc") / str(pid) / "cmdline").read_bytes().replace( - b"\0", b" " - ).decode("utf-8", errors="replace") - except FileNotFoundError: - # The daemon may have exited between writing its pid file and - # this cleanup pass. The exact file is still ours to remove. - with contextlib.suppress(FileNotFoundError, PermissionError, OSError): - pid_file.unlink() + except (OSError, ValueError): continue - except (OSError, UnicodeError, ValueError): + command_line = _process_command_line(pid) + if command_line is None: + # Either the daemon exited between writing its pid file and + # this pass, or this host has no procfs to ask. Only the first + # justifies forgetting the pid file. Without procfs we cannot + # confirm the process is ours, so we neither kill it nor drop + # the record that would let a later pass find it. + if not _process_is_alive(pid): + with contextlib.suppress(FileNotFoundError, PermissionError, OSError): + pid_file.unlink() continue if "agent-browser" in command_line: with contextlib.suppress(ProcessLookupError, PermissionError, OSError): @@ -2395,10 +2433,17 @@ class PrivateBrowserTool: for pid_file in _browser_pid_file_candidates(runtime_dir, namespace, session_id): try: pid = int(pid_file.read_text().strip()) - command_line = (Path("/proc") / str(pid) / "cmdline").read_bytes().replace( - b"\0", b" " - ).decode("utf-8", errors="replace") - except (FileNotFoundError, OSError, UnicodeError, ValueError): + except (OSError, ValueError): + continue + command_line = _process_command_line(pid) + if command_line is None: + # Without procfs we can only tell that something with this pid + # is alive, not that it is agent-browser. The pid file is our + # own namespaced one, so treat a live pid as a match: answering + # "no daemon" here is what lets `close` bootstrap a fresh one + # and wait on its browser forever. + if _process_is_alive(pid): + return True continue if "agent-browser" in command_line: return True diff --git a/tests/test_private_browser_tool.py b/tests/test_private_browser_tool.py index 34fbce363..79c761e60 100644 --- a/tests/test_private_browser_tool.py +++ b/tests/test_private_browser_tool.py @@ -4,6 +4,7 @@ import base64 import json from pathlib import Path +from core import platform_compat import src.agent_tools.web_tools as web_tools from src.agent_tools.web_tools import ( PrivateBrowserTool, @@ -1809,7 +1810,7 @@ def test_terminate_owned_chrome_skips_the_sweep_without_procfs( """macOS and Windows have no /proc; shutdown must degrade, not raise.""" missing = tmp_path / "no-procfs" - monkeypatch.setattr(web_tools, "_PROC_ROOT", missing) + monkeypatch.setattr(platform_compat, "PROC_ROOT", missing) def _unexpected_iterdir(*args, **kwargs): raise AssertionError("the pid sweep must not run without procfs") @@ -1838,10 +1839,112 @@ def test_terminate_owned_chrome_kills_only_this_runtimes_profile( _write_pid("202", "chrome --user-data-dir=/Users/someone/Library/Chrome") (proc / "self").mkdir() - monkeypatch.setattr(web_tools, "_PROC_ROOT", proc) + monkeypatch.setattr(platform_compat, "PROC_ROOT", proc) killed: list[int] = [] monkeypatch.setattr(web_tools.os, "kill", lambda pid, sig: killed.append(pid)) PrivateBrowserTool._terminate_owned_chrome({"TMPDIR": str(tmpdir)}) assert killed == [101] + + +def _pid_file_for(tmp_path, monkeypatch, namespace, session, pid): + """Write a pid file where the daemon helpers will look for it.""" + monkeypatch.setenv("XDG_RUNTIME_DIR", str(tmp_path)) + monkeypatch.setenv("ODYSSEUS_BROWSER_NAMESPACE", namespace) + candidates = web_tools._browser_pid_file_candidates(tmp_path, namespace, session) + target = candidates[0] + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(str(pid)) + return target + + +def test_live_daemon_pid_file_survives_a_host_without_procfs( + monkeypatch, tmp_path +) -> None: + """Off Linux a missing cmdline is not evidence the daemon exited.""" + + monkeypatch.setattr(platform_compat, "PROC_ROOT", tmp_path / "no-procfs") + monkeypatch.setattr(web_tools, "_process_is_alive", lambda pid: True) + killed: list[int] = [] + monkeypatch.setattr(web_tools.os, "kill", lambda pid, sig: killed.append(pid)) + pid_file = _pid_file_for(tmp_path, monkeypatch, "clawmm-test", "session-1", 4321) + + PrivateBrowserTool._terminate_owned_daemon({}, "session-1") + + assert pid_file.exists(), "a live daemon's pid file must not be removed" + assert killed == [], "an unverified process must not be killed" + + +def test_dead_daemon_pid_file_is_removed_without_procfs(monkeypatch, tmp_path) -> None: + """A pid that no longer exists is the one case that justifies forgetting it.""" + + monkeypatch.setattr(platform_compat, "PROC_ROOT", tmp_path / "no-procfs") + monkeypatch.setattr(web_tools, "_process_is_alive", lambda pid: False) + pid_file = _pid_file_for(tmp_path, monkeypatch, "clawmm-test", "session-2", 4322) + + PrivateBrowserTool._terminate_owned_daemon({}, "session-2") + + assert not pid_file.exists() + + +def test_owned_daemon_is_detected_from_a_live_pid_without_procfs( + monkeypatch, tmp_path +) -> None: + """Answering "no daemon" here is what lets close bootstrap a fresh one.""" + + monkeypatch.setattr(platform_compat, "PROC_ROOT", tmp_path / "no-procfs") + monkeypatch.setattr(web_tools, "_process_is_alive", lambda pid: True) + _pid_file_for(tmp_path, monkeypatch, "clawmm-test", "session-3", 4323) + + assert PrivateBrowserTool._owned_daemon_exists({}, "session-3") is True + + +def test_owned_daemon_absent_when_the_pid_is_gone(monkeypatch, tmp_path) -> None: + monkeypatch.setattr(platform_compat, "PROC_ROOT", tmp_path / "no-procfs") + monkeypatch.setattr(web_tools, "_process_is_alive", lambda pid: False) + _pid_file_for(tmp_path, monkeypatch, "clawmm-test", "session-4", 4324) + + assert PrivateBrowserTool._owned_daemon_exists({}, "session-4") is False + + +def test_procfs_host_still_matches_on_the_command_line(monkeypatch, tmp_path) -> None: + """With procfs present the identity check stays exact, not pid-liveness.""" + + proc = tmp_path / "proc" + (proc / "5555").mkdir(parents=True) + (proc / "5555" / "cmdline").write_bytes(b"node\0agent-browser\0--serve") + (proc / "6666").mkdir(parents=True) + (proc / "6666" / "cmdline").write_bytes(b"some\0other\0process") + monkeypatch.setattr(platform_compat, "PROC_ROOT", proc) + monkeypatch.setattr(web_tools, "_process_is_alive", lambda pid: True) + + _pid_file_for(tmp_path, monkeypatch, "clawmm-test", "session-5", 5555) + assert PrivateBrowserTool._owned_daemon_exists({}, "session-5") is True + + _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( + platform_compat, "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] diff --git a/tests/test_runtime_behavior_regressions.py b/tests/test_runtime_behavior_regressions.py index 84ca3ad15..9563bb177 100644 --- a/tests/test_runtime_behavior_regressions.py +++ b/tests/test_runtime_behavior_regressions.py @@ -16,6 +16,7 @@ import json import pytest +from core import platform_compat import src.agent_loop as al import src.agent_tools.web_tools as al_web @@ -259,7 +260,7 @@ def test_chrome_sweep_kills_only_this_runtimes_profile(monkeypatch, tmp_path): _pid("303", "chrome --user-data-dir=/tmp/other-worktree/agent-browser-chrome-x") (proc / "self").mkdir() - monkeypatch.setattr(al_web, "_PROC_ROOT", proc) + monkeypatch.setattr(platform_compat, "PROC_ROOT", proc) killed = [] monkeypatch.setattr(al_web.os, "kill", lambda pid, sig: killed.append(pid))