mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-10-06 06:52:20 +02:00
fix(browser): stop treating a missing cmdline as proof the daemon exited
_terminate_owned_daemon() read /proc/<pid>/cmdline and, on FileNotFoundError, unlinked the pid file on the stated assumption that "the daemon may have exited". Off Linux that file is always missing, so the branch always fired: the pid file of a live daemon was deleted and the daemon itself never killed. _owned_daemon_exists() swallowed the same error and therefore always returned False, which is precisely the state its own docstring warns about, since a close against an unrecognised session can bootstrap a fresh daemon and wait on its browser indefinitely. Demonstrated on macOS before the change: a pid file holding a live pid is removed by _terminate_owned_daemon() and _owned_daemon_exists() reports False. After it, the file survives and the daemon is reported present. _process_command_line() now returns None for "this host cannot tell" and _process_is_alive() answers the separate question of whether the pid exists. Without procfs we decline to kill a process we cannot confirm is ours, and we only forget a pid file once the pid is genuinely gone. Linux behaviour is unchanged: the command-line identity check still gates both paths.
This commit is contained in:
@@ -125,6 +125,37 @@ def _browser_pid_file_candidates(
|
||||
# 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 (_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. Signal 0 checks without delivering."""
|
||||
|
||||
try:
|
||||
os.kill(pid, 0)
|
||||
except ProcessLookupError:
|
||||
return False
|
||||
except PermissionError:
|
||||
# Alive, owned by somebody else.
|
||||
return True
|
||||
except OSError:
|
||||
return False
|
||||
return True
|
||||
|
||||
_SCHOLARLY_METADATA_CUE_RE = re.compile(
|
||||
r"\b(?:accept(?:ed|ance)?|publish(?:ed|ing|cation)?|venue|conference|"
|
||||
r"journal|proceedings|doi)\b",
|
||||
@@ -2359,16 +2390,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 +2428,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
|
||||
|
||||
@@ -1845,3 +1845,81 @@ def test_terminate_owned_chrome_kills_only_this_runtimes_profile(
|
||||
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(web_tools, "_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(web_tools, "_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(web_tools, "_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(web_tools, "_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(web_tools, "_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
|
||||
|
||||
Reference in New Issue
Block a user