fix(browser): cross-platform reaper for Chromium orphaned by abnormal daemon death - #70002
fix(browser): cross-platform reaper for Chromium orphaned by abnormal daemon death#70002sablea wants to merge 1 commit into
Conversation
…platform) When the agent-browser daemon dies without shutting Chromium down (OOM kill / crash / SIGKILL — the common failure mode on memory-starved hosts), the browser is cut loose and keeps running: on POSIX it reparents to init, on Windows its PPID is invalidated. Reproduced live on Linux — SIGKILL the daemon after a successful navigate and the full 12-process Chromium tree survives session cleanup indefinitely. Every existing cleanup path goes blind in that state: * _cleanup_single_browser_session kills via the daemon PID's child tree — _terminate_host_pid returns silently when that PID is already dead — then removes the socket dir, destroying the only pid breadcrumb. * _reap_orphaned_browser_sessions only globs daemon socket dirs (agent-browser-h_*/cdp_*/hermes_*), never the browser's agent-browser-chrome-* user-data dir. On a long-running gateway this accumulates orphaned Chromium processes: 7 chrome processes surviving 6 days on a 2 GB Ubuntu VPS (the NousResearch#32047 symptom; also the macOS case in NousResearch#17388, closed not-planned). Relationship to NousResearch#43577: that open PR reaps the same orphans but is gated Windows-only (`if os.name != "nt": return 0`) on the assumption that "POSIX SIGTERM cascades reliably." The live repro above disproves that assumption — the cascade needs a live process to walk the tree, which an abnormally-killed daemon cannot do (POSIX does not signal children when a parent dies). This change detects orphans cross-platform via psutil (already a hard dependency) instead of WMIC, and reaps via the existing ProcessRegistry._terminate_host_pid (taskkill /T /F on Windows, psutil tree-walk on POSIX), so a single reaper needs no per-OS branch. The POSIX path is verified live; the Windows path is by construction only (cross-platform primitives + the existing taskkill path) and not yet verified on Windows. Fix: sweep for main Chromium processes whose cmdline carries an agent-browser-chrome-* --user-data-dir but whose parent is no longer a live agent-browser daemon; tree-kill them and drop the stale profile dir. The sweep runs on every cleanup-thread tick and from the startup/atexit orphan reap — including when no socket dirs remain, which is exactly the orphan's state — so a long session self-heals instead of leaking until restart. Daemon-owned browsers and foreign Chrome installs are untouched (covered by unit tests). Refs: NousResearch#17388, NousResearch#32047, NousResearch#43577
Related to #62615, #60152, #64383, and #43577: this patch uses a cross-platform cmdline/parent-based sweep after abnormal daemon death, while the existing proposals use profile-directory, socket-session, ownership, or Windows-specific mechanisms. Maintainers should select a canonical orphan-reaper design. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the already-dead-daemon case; current main still returns without any Chromium scan when no socket directory remains (tools/browser_tool.py:1787-1788), so this addresses a real gap.
Problems
tools/browser_tool.py:1856-1859treats any process with anagent-browser-chrome-*argument and a non-agent-browserimmediate parent as garbage. It does not verify process identity or bind the process/profile to a known Hermes daemon. Current main explicitly requires identity plus session binding before tree-killing from predictable temporary-path state (tools/browser_tool.py:1672-1751) to avoid arbitrary-process DoS.- The new tests cover only null and
agent-browser-named parents. They do not cover a matching profile argument without verified Hermes ownership.
Suggested changes
- Preserve or derive durable session provenance and fail closed until a candidate is tied to a known dead agent-browser daemon; then add a negative ownership test before tree-killing.
- Add platform integration coverage for the parent topology rather than relying only on mocked parent names.
Automated hermes-sweeper review.
| parent = proc.parent() | ||
| if parent is not None and "agent-browser" in (parent.name() or ""): | ||
| continue # daemon alive and owning this browser — not an orphan | ||
| orphans.append((proc, user_data_dir)) |
There was a problem hiding this comment.
This appends any process with the matching argument unless its immediate parent name contains agent-browser. Current main requires both process identity and binding to a specific session before a tree kill (_verify_reapable_browser_daemon), because predictable temporary-path state otherwise permits same-user arbitrary-process DoS. Please require durable evidence that this profile/process belongs to a known dead Hermes daemon, and add a negative test for a matching command line without that association.
What does this PR do?
Reaps orphaned Chromium processes that accumulate when the agent-browser daemon dies abnormally — OOM kill / crash / SIGKILL, the common failure mode on memory-starved hosts. A single
psutil-based reaper that works on both POSIX and Windows.The leak
agent-browser launches Chromium with a
--user-data-dir=<tmp>/agent-browser-chrome-*profile. When the daemon is killed without a clean shutdown, the browser is cut loose and keeps running — on POSIX it reparents to init, on Windows its PPID is invalidated — and every existing cleanup path then goes blind:_cleanup_single_browser_sessionkills via the daemon PID's child tree;_terminate_host_pidreturns silently once that PID is dead, and the socket dir (the only pid breadcrumb) is then removed._reap_orphaned_browser_sessionsonly globs daemon socket dirs (agent-browser-h_*/cdp_*/hermes_*), never the browser'sagent-browser-chrome-*user-data dir.After that nothing can find the browser again, so it runs until reboot.
Motivating case: on a 2 GB RAM Ubuntu VPS running a long-lived gateway, 7 Chromium processes survived 6 days (daemon dead, socket dirs gone,
agent-browser-chrome-*still under/tmp) until killed by hand. On a box that small the orphans themselves make the next OOM kill — and the next orphan — more likely.Relationship to #43577
#43577 already tackles this exact leak and reaps orphans correctly on Windows. It's currently scoped to Windows (
if os.name != "nt": return 0), on the reasoning that POSIX's SIGTERM cascade handles the Unix side.While testing on Linux I found the POSIX side isn't actually covered: the cascade only fires while a live process walks the tree, so a daemon that's killed abnormally (OOM / SIGKILL) leaves its whole Chromium tree behind — POSIX doesn't signal a process's children when it dies. Repro:
kill -9the daemon after a successful navigate and the full 12-process tree survives session cleanup and the inactivity reaper indefinitely (logs below).Rather than add a second platform-specific reaper, this PR detects orphans cross-platform via
psutil(already a hard dependency) and reaps through the existingProcessRegistry._terminate_host_pid(taskkill /T /Fon Windows, psutil tree-walk on POSIX), so one code path needs no per-OS branch:wmicpsutil.process_iterproc.cmdline()proc.parent()(dead/Noneon all OSes)_terminate_host_pidScope note / verification: the POSIX path is verified live (repro below). The Windows path is by construction — it reuses
psutilplus the existingtaskkillprimitive — but I haven't verified it on a real Windows host. So this could either (a) supersede #43577 as a unified cross-platform reaper, or (b) be trimmed to POSIX-only to land alongside #43577's tested Windows path. Happy to go whichever way the maintainers and @bighamx prefer — glad to coordinate.Related Issue
Fixes #32047. Also covers the macOS case in #17388 (closed not-planned; process-tree cleanup exists, but no call chain reaches a browser whose daemon PID is already gone).
Type of Change
Changes Made
tools/browser_tool.py: add_find_orphaned_chrome_processes()(cross-platformpsutilscan matching theagent-browser-chrome-*user-data-dir signature; skips--type=helpers and browsers whose parent is a live agent-browser daemon) and_reap_orphaned_chrome_processes()(tree-kill viaProcessRegistry._terminate_host_pid+ stale profile-dir removal). Wire the sweep into the cleanup-thread tick and both branches of_reap_orphaned_browser_sessions(normal path + empty-socket-dir early return).tests/tools/test_browser_cleanup.py: 4 regression tests — orphan reaped (+ profile dir removed), daemon-owned browser left alone,--type=helpers / foreign Chrome installs skipped, sweep not short-circuited by the empty-socket-dir return.How to Test
browser_navigateto a page, thenkill -9theagent-browserdaemon. The full Chromium tree (12+ processes) survives session cleanup and the inactivity reaper forever; the socket dir is removed so nothing can find it again.agent-browser-chrome-*user-data dir.pytest tests/tools/test_browser_cleanup.py -q→ 10 passed.Checklist
Code
pytest tests/ -qand all tests pass — the full suite is red onmainindependently of this PR (e.g. tip commit fix(desktop): place steer messages before redirected replies #69739 shows failing CI), in unrelated modules. This PR's own tests pass (pytest tests/tools/test_browser_cleanup.py -q→ 10 passed) and no browser/chrome/cleanup test is in the suite's failure set.Documentation & Housekeeping
cli-config.yaml.example— N/A (no config keys)CONTRIBUTING.md/AGENTS.md— N/AScreenshots / Logs
Without the fix (daemon SIGKILLed at t≈2s; reaper ran at t≈62s):
With the fix (same scenario):