fix(browser): kill orphaned Chromium processes when daemon is dead - #60152
fix(browser): kill orphaned Chromium processes when daemon is dead#60152nj1i6t6 wants to merge 5 commits into
Conversation
When the agent-browser daemon (Node process) dies before cleanup — via SIGKILL, crash, or OOM — its Chromium child processes are reparented to PID 1 and left running indefinitely. The orphan reaper discovers the dead daemon PID and removes the socket directory, but never terminates the orphaned Chromium, causing steady resource accumulation. In production (long-running gateway, 10 days uptime) this accumulated 371 orphaned Chromium processes consuming ~20% CPU and ~14 GB RSS. Fix: when the reaper finds a dead daemon, call the new _kill_orphaned_chromium_for_session() to scan for Chromium processes whose --user-data-dir matches the agent-browser-chrome-* pattern and terminate them (SIGTERM → 3s grace → SIGKILL) before removing the socket dir. Security: the agent-browser-chrome- prefix is specific to agent-browser-spawned Chromium and distinct from user-installed Chrome/Chromium. psutil only signals same-user processes. Tests: two new tests verify (1) dead daemon triggers the Chromium scan, and (2) alive daemon does NOT trigger it (tree-kill path handles that case). All 28 existing orphan-reaper tests still pass.
There was a problem hiding this comment.
Pull request overview
This PR addresses a production resource-leak scenario in the browser tool orphan reaper: when the agent-browser daemon (Node) dies unexpectedly, its Chromium children can remain running and accumulate CPU/RSS. The change adds a Chromium-cleanup path when the reaper detects a dead daemon PID.
Changes:
- Added
_kill_orphaned_chromium_for_session()and wired it into_reap_orphaned_browser_sessions()when the daemon PID is dead. - Added tests to assert the reaper calls (or does not call) the Chromium cleanup hook based on daemon liveness.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/browser_tool.py | Adds orphaned-Chromium cleanup and calls it when a dead daemon PID is detected. |
| tests/tools/test_browser_orphan_reaper.py | Adds tests for reaper behavior around the new Chromium cleanup path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for proc in psutil.process_iter(["pid", "name", "cmdline"]): | ||
| try: | ||
| cmdline = " ".join(proc.info["cmdline"] or []) | ||
| if "agent-browser-chrome-" in cmdline and "--user-data-dir=" in cmdline: | ||
| proc.terminate() | ||
| terminated_procs.append(proc) | ||
| killed += 1 | ||
| logger.info( | ||
| "Killed orphaned Chromium PID %d (session %s, daemon gone)", | ||
| proc.info["pid"], session_name) |
| 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(socket_dir, 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_for_session", | ||
| side_effect=mock_kill_chromium): | ||
| _reap_orphaned_browser_sessions() | ||
|
|
||
| assert len(chromium_killed) == 0 | ||
|
|
||
|
|
Related: orphan-reaper family — #31231 (merged; POSIX+Windows tree-kill while the daemon is alive) and #43577 (open; Windows second-pass reaper for the dead-daemon case). This PR covers the POSIX dead-daemon case that main's reaper currently skips (it only removes the socket dir and never kills the reparented Chromium children). Distinct mechanism, not a duplicate. |
Address Copilot review feedback on PR NousResearch#60152: 1. Cross-session kill risk: _kill_orphaned_chromium_for_session() previously matched ANY Chromium whose cmdline contained "agent-browser-chrome-" regardless of which session it belonged to. When multiple browser sessions are active and one daemon dies, this could terminate Chromium belonging to other live sessions. Fix: added PPID==1 check. When a daemon dies, its Chromium children are reparented to init (PID 1). Chromium belonging to a *live* daemon still has the daemon as its parent. This precisely distinguishes true orphans from active sessions without needing a socket_dir→UUID mapping (which doesn't exist — agent-browser generates Chromium user-data-dir UUIDs independently of socket dir names). 2. Added test_only_reparented_chromium_is_killed: mocks two Chromium processes (one PPID=1 orphan, one PPID=99999 live), verifies only the orphan is terminated.
Changes in response to Copilot review feedback: 1. Cross-session safety: add PPID==1 check to ensure only true orphan Chromium (reparented to init after daemon death) is killed. Active sessions' Chromium still has the live daemon as parent and is skipped. 2. Rename _kill_orphaned_chromium_for_session(socket_dir, session_name) → _kill_orphaned_chromium_processes(session_name). Remove unused socket_dir parameter that could not be mapped to Chromium's --user-data-dir UUID. 3. Fix inaccurate comment claiming --user-data-dir references the socket directory. Explain that we cannot map Chromium's UUID to a session, so we target all orphaned agent-browser Chromium (PPID==1), not just the current session's. 4. Add test with 3 fake processes: orphaned agent-browser Chromium (PPID=1, killed), active session Chromium (PPID=daemon, skipped), and user-installed Chrome (PPID=1 but no agent-browser pattern, skipped).
| 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 |
| logger.info( | ||
| "Killed orphaned Chromium PID %d (session %s, daemon gone)", | ||
| proc.info["pid"], session_name) |
|
Reproduced on a Linux systemd gateway (Hermes v0.18.2 / 2026.7.7.2) under real parallel-subagent browser use. Observed sequence:
A relevant integration detail: Additional contributing paths found locally:
I applied this PR's three commits locally together with the aborted-turn cleanup from #39897 and pinned This is a strong production confirmation of the Linux dead-daemon/reparented-Chromium failure mode. A browser-session concurrency cap and session-lifetime ownership (rather than process-wide gateway PID ownership) would be useful complementary fixes. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the dead-daemon case; current main still has the gap at tools/browser_tool.py:1720-1722, where a dead daemon only causes socket-directory removal.
Problems
- The new helper selects PPID-1 Chromium roots (
tools/browser_tool.py:1685) but its escalation only waits for and kills those root objects (:1701-1704). It does not snapshot or signal their descendants. CurrentProcessRegistry._terminate_host_pidexplicitly handles that full-tree invariant attools/process_registry.py:627-672; commit8cfcbd327dfc65dbc073d0ba002dbff7a61f7713documents whywait_procsis insufficient for parent/child trees. - The new test covers root candidate filtering, but not root-plus-child teardown or the escalation path (
tests/tools/test_browser_orphan_reaper.py:248-289).
Suggested changes
- Reuse the full-tree termination path after selecting an orphaned Chromium root, and add a regression asserting a SIGTERM-ignoring root and its children are all reaped.
This is an automated hermes-sweeper review.
| gone, alive = psutil.wait_procs(terminated_procs, timeout=3) | ||
| for proc in alive: | ||
| try: | ||
| proc.kill() |
There was a problem hiding this comment.
This escalates only the PPID-1 root selected above; its renderer/GPU descendants are never snapshotted or signalled and can be reparented when the root is force-killed. Please use the existing full-tree termination path (ProcessRegistry._terminate_host_pid) or otherwise reap the complete descendant tree; 8cfcbd327dfc65dbc073d0ba002dbff7a61f7713 added that invariant after wait_procs proved unreliable for process trees.
| # 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. | ||
| _kill_orphaned_chromium_processes(session_name) |
| killed = 0 | ||
|
|
||
| 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. | ||
| from tools.process_registry import ProcessRegistry | ||
| ProcessRegistry._terminate_host_pid(proc.info["pid"]) |
| with patch("tools.browser_tool._kill_orphaned_chromium_processes", | ||
| side_effect=mock_kill_chromium): | ||
| _reap_orphaned_browser_sessions() |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tools/browser_tool.py:1678
- The orphan check
proc.info["ppid"] != 1assumes POSIX-style reparenting to PID 1. On Windows, a child process typically keeps the dead parent PID (it is not reparented), so this will skip orphaned Chromium and the leak will persist. Also, the current cmdline check can matchagent-browser-chrome-outside the--user-data-dirvalue; parsing that flag reduces false positives.
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:
|
|
||
|
|
||
| class TestOwnerPidCrossProcess: |
Problem
When the
agent-browserdaemon (Node process) dies before cleanup — via SIGKILL, crash, or OOM — its Chromium child processes are reparented to PID 1 and left running indefinitely.The orphan reaper (
_reap_orphaned_browser_sessions) discovers the dead daemon PID and removes the socket directory, but never terminates the orphaned Chromium processes, causing steady resource accumulation.Observed in production
A long-running Hermes gateway (10 days uptime, gateway mode) accumulated 371 orphaned Chromium processes:
Root cause
In
_reap_orphaned_browser_sessions(), when the daemon PID is found dead:The tree-kill path (
ProcessRegistry._terminate_host_pid(daemon_pid)) only runs when the daemon PID is alive — it walks the process tree from the daemon downward. When the daemon is already dead, Chromium children (reparented to PID 1) are unreachable via tree-kill.Fix
Added
_kill_orphaned_chromium_for_session()— called when the reaper finds a dead daemon, before removing the socket directory. It:psutil.process_iter()for Chromium instances whose command line containsagent-browser-chrome-and--user-data-dir=SIGTERM(viaproc.terminate())SIGKILLfor any survivorsSecurity
agent-browser-chrome-prefix is specific to agent-browser-spawned Chromium — distinct from user-installed Chrome/Chromium profilespsutilonly signals same-user processes (same security boundary as the existing reaper)psutilis already a hard dependency)Tests
Two new tests in
TestOrphanedChromiumCleanup:test_dead_daemon_triggers_chromium_scan_kill_orphaned_chromium_for_sessionis called + socket dir removedtest_alive_daemon_does_not_trigger_chromium_scanAll 28 existing orphan-reaper tests continue to pass.