diff --git a/tests/tools/test_browser_orphan_reaper.py b/tests/tools/test_browser_orphan_reaper.py index beed82e836248..9edd72021fbd6 100644 --- a/tests/tools/test_browser_orphan_reaper.py +++ b/tests/tools/test_browser_orphan_reaper.py @@ -2,7 +2,7 @@ daemons whose Python parent exited without cleaning up.""" import os -from unittest.mock import patch +from unittest.mock import patch, MagicMock import pytest @@ -181,6 +181,170 @@ def test_corrupt_pid_file_is_cleaned(self, fake_tmpdir): assert not d.exists() +class TestOrphanedChromiumCleanup: + """Tests for _kill_orphaned_chromium_processes() — kills Chromium + processes left behind when the agent-browser daemon dies before cleanup. + + When the daemon (Node process) crashes or is SIGKILLed, its Chromium + children are reparented to PID 1 and the normal tree-kill path (which + walks children of the daemon PID) never reaches them. The reaper + discovers the dead daemon PID, removes the socket dir, and calls + _kill_orphaned_chromium_processes to scan for surviving orphaned + Chromium processes by matching PPID == 1 + the ``agent-browser-chrome-`` + cmdline pattern. + """ + + def test_dead_daemon_triggers_chromium_scan(self, fake_tmpdir): + """When the daemon PID is dead, orphaned Chromium is scanned + killed.""" + from tools.browser_tool import _reap_orphaned_browser_sessions + + d = _make_socket_dir(fake_tmpdir, "h_dead123456", pid=999999999) + + chromium_killed = [] + + def mock_kill_chromium(session_name): + chromium_killed.append(session_name) + return 0 + + with patch("gateway.status._pid_exists", return_value=False), \ + patch("tools.browser_tool._kill_orphaned_chromium_processes", + side_effect=mock_kill_chromium): + _reap_orphaned_browser_sessions() + + assert len(chromium_killed) == 1 + assert chromium_killed[0] == "h_dead123456" + assert not d.exists() + + def test_dead_daemons_trigger_chromium_scan_only_once(self, fake_tmpdir): + """Global Chromium scanning happens once per reaper invocation.""" + from tools.browser_tool import _reap_orphaned_browser_sessions + + first = _make_socket_dir(fake_tmpdir, "h_dead_first", pid=10001) + second = _make_socket_dir(fake_tmpdir, "h_dead_second", pid=10002) + chromium_scans = [] + + def mock_kill_chromium(session_name): + chromium_scans.append(session_name) + return 0 + + with patch("gateway.status._pid_exists", return_value=False), \ + patch("tools.browser_tool._kill_orphaned_chromium_processes", + side_effect=mock_kill_chromium): + _reap_orphaned_browser_sessions() + + assert len(chromium_scans) == 1 + assert set(chromium_scans) <= {"h_dead_first", "h_dead_second"} + assert not first.exists() + assert not second.exists() + + def test_alive_daemon_does_not_trigger_chromium_scan(self, fake_tmpdir): + """When the daemon PID is alive, Chromium scan is NOT triggered + (the tree-kill path handles it).""" + from tools.browser_tool import _reap_orphaned_browser_sessions + + d = _make_socket_dir(fake_tmpdir, "h_alive1234567", pid=12345) + + chromium_killed = [] + + def mock_kill_chromium(session_name): + chromium_killed.append(session_name) + return 0 + + with patch("gateway.status._pid_exists", return_value=True), \ + patch("tools.browser_tool._verify_reapable_browser_daemon", return_value=True), \ + patch("tools.process_registry.ProcessRegistry._terminate_host_pid"), \ + patch("tools.browser_tool._kill_orphaned_chromium_processes", + side_effect=mock_kill_chromium): + _reap_orphaned_browser_sessions() + + assert len(chromium_killed) == 0 + + def test_kill_orphaned_chromium_skips_non_orphan_active_session(self, fake_tmpdir): + """_kill_orphaned_chromium_processes only kills Chromium whose PPID + is 1 (reparented to init = true orphan). It must NOT kill: + + - Chromium whose parent is a still-running daemon (active session) + - Non-agent-browser Chromium (user-installed Chrome, even if PPID=1) + """ + from tools.browser_tool import _kill_orphaned_chromium_processes + + # 1. orphaned agent-browser Chromium (PPID=1) → should terminate + orphan_proc = MagicMock() + orphan_proc.info = { + "pid": 2001, "name": "chrome", "cmdline": [ + "/usr/bin/chromium", "--headless", + "--user-data-dir=/tmp/agent-browser-chrome-aaaa1111", + ], "ppid": 1, + } + # 2. active session Chromium (PPID=daemon) → must NOT terminate + live_proc = MagicMock() + live_proc.info = { + "pid": 2002, "name": "chrome", "cmdline": [ + "/usr/bin/chromium", "--headless", + "--user-data-dir=/tmp/agent-browser-chrome-bbbb2222", + ], "ppid": 99999, + } + # 3. user-installed Chrome (PPID=1 but no agent-browser pattern) → must NOT terminate + user_chrome = MagicMock() + user_chrome.info = { + "pid": 2003, "name": "chrome", "cmdline": [ + "/usr/bin/google-chrome", "--user-data-dir=/home/user/.config/chrome", + ], "ppid": 1, + } + + terminated_pids = [] + + def fake_terminate(pid): + terminated_pids.append(pid) + + with patch("psutil.process_iter", + return_value=[orphan_proc, live_proc, user_chrome]), \ + patch( + "tools.process_registry.ProcessRegistry._terminate_host_pid", + side_effect=fake_terminate, + ): + result = _kill_orphaned_chromium_processes("h_test1234") + + assert result == 1 # only the orphan root was selected + assert terminated_pids == [2001] + orphan_proc.terminate.assert_not_called() + live_proc.terminate.assert_not_called() + user_chrome.terminate.assert_not_called() + + def test_kill_orphaned_chromium_uses_full_process_tree_termination( + self, fake_tmpdir + ): + """An orphan Chromium root is terminated together with its descendants. + + The process registry owns the cross-platform tree-kill and escalation + behavior. The browser orphan scanner must delegate to it rather than + terminating only the Chromium root object. + """ + from tools.browser_tool import _kill_orphaned_chromium_processes + + orphan_proc = MagicMock() + orphan_proc.info = { + "pid": 2001, + "name": "chrome", + "cmdline": [ + "/usr/bin/chromium", + "--headless", + "--user-data-dir=/tmp/agent-browser-chrome-aaaa1111", + ], + "ppid": 1, + } + + with patch("psutil.process_iter", return_value=[orphan_proc]), \ + patch( + "tools.process_registry.ProcessRegistry._terminate_host_pid" + ) as terminate_tree: + result = _kill_orphaned_chromium_processes("h_test1234") + + assert result == 1 + terminate_tree.assert_called_once_with(2001) + orphan_proc.terminate.assert_not_called() + + class TestOwnerPidCrossProcess: """Tests for owner_pid-based cross-process safe reaping. diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 82c248a3f715a..2d534806ce103 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -1626,6 +1626,83 @@ def _verify_reapable_browser_daemon(daemon_pid: int, socket_dir: str, return True +def _kill_orphaned_chromium_processes(session_name: str) -> int: + """Kill Chromium processes orphaned by a dead agent-browser daemon. + + When the agent-browser daemon (a Node process) dies before its owner + cleans up, Chromium child processes it spawned are reparented to PID 1 + and left running indefinitely — consuming CPU and memory with no one to + shut them down. The daemon's ``.pid`` file points at the dead Node + process, so the reaper's normal tree-kill path (which walks children of + the *daemon* PID) never reaches them. + + We cannot map Chromium's ``--user-data-dir`` UUID back to a specific + session's socket directory, so this function targets **all** orphaned + agent-browser Chromium processes (PPID == 1 + ``agent-browser-chrome-`` + cmdline pattern), not just those from ``session_name``. This is safe + because any agent-browser Chromium with PPID 1 is a true orphan whose + daemon is gone — leaving it running is the same resource leak this fix + addresses. Active sessions are unaffected: their Chromium still has + the live daemon as parent (PPID != 1). + + Security: the ``agent-browser-chrome-`` prefix is specific to + agent-browser-spawned Chromium and distinct from user-installed + Chrome/Chromium. The PPID==1 check ensures we only kill true orphans, + not Chromium children of a still-running daemon. Same-user only + (psutil can only signal same-user processes). + + Args: + session_name: Used for logging context only; the actual kill + criteria are orphaned parent + agent-browser cmdline pattern. + + Returns the number of orphaned Chromium roots handed to the process-tree + terminator. + """ + try: + import psutil + except ImportError: + logger.warning( + "Cannot scan for orphaned Chromium (session %s): psutil unavailable", + session_name) + return 0 + + killed = 0 + from tools.process_registry import ProcessRegistry + + for proc in psutil.process_iter(["pid", "name", "cmdline", "ppid"]): + try: + cmdline = " ".join(proc.info["cmdline"] or []) + # Match agent-browser-spawned Chromium by its user-data-dir pattern. + if "agent-browser-chrome-" not in cmdline: + continue + if "--user-data-dir=" not in cmdline: + continue + # Only kill true orphans: Chromium whose parent (the daemon) + # has died, causing reparenting to PID 1 (init). Chromium + # belonging to a *live* daemon still has the daemon as its + # parent and must not be touched. + if proc.info["ppid"] != 1: + continue + + # Chromium is a process tree. Reuse the shared termination path + # so descendants (renderer, GPU, etc.) receive the same + # SIGTERM→SIGKILL escalation as the root instead of surviving as + # newly orphaned processes when the root is force-killed. + ProcessRegistry._terminate_host_pid(proc.info["pid"]) + killed += 1 + logger.info( + "Reaped orphaned Chromium process tree rooted at PID %d " + "(session %s, daemon gone)", + proc.info["pid"], session_name) + except (psutil.NoSuchProcess, psutil.AccessDenied, psutil.ZombieProcess): + continue + except Exception as exc: + logger.debug("Error scanning process %d for orphaned Chromium: %s", + getattr(proc, 'pid', -1), exc) + + return killed + + def _reap_orphaned_browser_sessions(): """Scan for orphaned agent-browser daemon processes from previous runs. @@ -1671,6 +1748,7 @@ def _reap_orphaned_browser_sessions(): } reaped = 0 + chromium_scan_done = False for socket_dir in socket_dirs: dir_name = os.path.basename(socket_dir) # dir_name is "agent-browser-{session_name}" @@ -1714,10 +1792,19 @@ def _reap_orphaned_browser_sessions(): shutil.rmtree(socket_dir, ignore_errors=True) continue - # Check if the daemon is still alive. ``os.kill(pid, 0)`` on Windows + # Check if the daemon is still alive. ``os.kill(pid, 0)`` on Windows # is NOT a no-op — use the handle-based existence check. from gateway.status import _pid_exists if not _pid_exists(daemon_pid): + # The daemon is gone, but it may have left behind Chromium children. + # Once the daemon dies unexpectedly, those Chromium processes are + # reparented to PID 1. We cannot map Chromium's --user-data-dir UUID + # back to this socket directory, so only terminate agent-browser + # Chromium processes that are already orphaned. Active sessions still + # have a live daemon parent and are skipped. + if not chromium_scan_done: + chromium_scan_done = True + _kill_orphaned_chromium_processes(session_name) shutil.rmtree(socket_dir, ignore_errors=True) continue