Merge pull request #17 from o3LL/fix/procfs-pid-file-liveness

fix(browser): stop treating a missing cmdline as proof the daemon exited

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(
+            except (OSError, ValueError):
-                    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()
                 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(
+            except (OSError, ValueError):
-                    b"\0", b" "
+                continue
-                ).decode("utf-8", errors="replace")
+            command_line = _process_command_line(pid)
-            except (FileNotFoundError, OSError, UnicodeError, ValueError):
+            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

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]

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))