fix(browser): make the real-profile attach daemon reapable (#100855, salvage #103284)

The `hermes-real-profile` agent-browser daemon (the attach lane for consented
real-profile browsing) ran with plain `_build_browser_env()`: no
`AGENT_BROWSER_SOCKET_DIR`, so it lived in agent-browser's default dir and no
reap path could see it. A wedged daemon + headless Chrome survived 47h across
two gateway restarts (#100855), and on macOS the genuine Chrome binary it held
made "Chrome won't open" for the user.

Give the attach lane the same contract every other lane already has:
`_prepare_session_socket_dir()` (per-session socket dir + `owner_pid` claim)
and `_agent_browser_command_env()`, and add the named dir to the orphan
reaper's scan. The existing `_reap_socket_dir` then applies its owner-liveness
and start-time-fingerprint rules unchanged; the daemon is listed as tracked so
the untracked-idle escape hatch never fires under a live user (per-task `rp_*`
sessions drive it over `--cdp`, so its own dir shows no activity). The daemon-side idle
timeout is NOT inherited: Chrome is launched by Hermes, not the daemon, so a
self-exiting daemon would leave Chrome holding the copy dir while the next
attach re-runs the snapshot overlay over it.

When a reaped daemon's Chrome (Hermes-launched, own process group) still
holds the copy dir, `_real_profile_cdp` re-attaches to it instead of running
the snapshot overlay over a live profile. DevToolsActivePort outlives a
crashed Chrome and its port can be recycled, so the file's browser id must
match `/json/version` before it is trusted; an attach failure on a live
Chrome fails closed rather than overlaying.

Tests: attach/get/close commands carry the reaper-visible socket dir and
owner_pid and no idle timeout; a dead-owner real-profile daemon is reaped by
`_reap_orphaned_browser_sessions`; a surviving Chrome is re-attached, never
overlaid (all red on main).
This commit is contained in:
jango
2026-09-06 17:22:45 +00:00
committed by kshitij
parent 57f05e2142
commit ed406f929d
4 changed files with 116 additions and 3 deletions
+23
View File
@@ -111,6 +111,29 @@ class TestReapOrphanedBrowserSessions:
assert 12345 in terminate_calls
assert not d.exists()
def test_real_profile_attach_daemon_is_reaped_when_owner_is_dead(self, fake_tmpdir):
"""#100855: the shared ``hermes-real-profile`` attach daemon is not ``<prefix>_<hex>``
named, so the reaper's glob never saw it and a wedged daemon + headless Chrome outlived
gateway restarts. Same owner-liveness rule as every other lane: dead owner => reaped."""
import tools.browser_tool as bt
from tools.browser_tool_lifecycle import _reap_orphaned_browser_sessions
d = _make_socket_dir(fake_tmpdir, bt._REAL_PROFILE_SESSION, pid=4242, owner_pid=99999)
terminate_calls = []
def _pid_exists(pid):
return pid == 4242 # daemon alive, owning hermes gone
with patch("gateway.status._pid_exists", side_effect=_pid_exists), \
patch("gateway.status.get_process_start_time", return_value=777), \
patch("tools.browser_tool_lifecycle._verify_reapable_browser_daemon", return_value=True), \
patch("tools.process_registry.ProcessRegistry._terminate_host_pid",
side_effect=lambda pid, expected_start=None: terminate_calls.append(pid)):
_reap_orphaned_browser_sessions()
assert terminate_calls == [4242]
assert not d.exists()
def test_unfingerprintable_daemon_is_refused(self, fake_tmpdir):
"""No start-time fingerprint -> the kill is refused (fail closed).
+37
View File
@@ -275,6 +275,7 @@ class TestRealProfileCdpLaunch:
def fake_run(argv, **kw):
captured["argv"] = argv
captured["env"] = kw["env"]
return proc
class FakeChrome:
@@ -295,6 +296,7 @@ class TestRealProfileCdpLaunch:
side_effect=[None, "http://127.0.0.1:41000"]), \
patch.object(bt_install, "_find_agent_browser", return_value="/usr/bin/agent-browser"), \
patch.object(bt.subprocess, "run", side_effect=fake_run), \
patch.object(bt, "_socket_safe_tmpdir", return_value=str(tmp_path)), \
patch.object(bt_cloud, "_is_headed_mode", return_value=False):
bt_real_profile._real_profile_cdp()
# The chrome launch itself is headless (no window, no focus steal).
@@ -303,6 +305,12 @@ class TestRealProfileCdpLaunch:
assert "--headless" not in captured["argv"]
assert "--profile" not in captured["argv"]
assert "--cdp" in captured["argv"]
# #100855: the attach daemon lives in a reaper-visible socket dir claimed by this
# process, and never self-terminates (Chrome is ours, not the daemon's).
socket_dir = captured["env"]["AGENT_BROWSER_SOCKET_DIR"]
assert socket_dir == str(tmp_path / f"agent-browser-{bt._REAL_PROFILE_SESSION}")
assert (tmp_path / f"agent-browser-{bt._REAL_PROFILE_SESSION}" / f"{bt._REAL_PROFILE_SESSION}.owner_pid").read_text() == str(os.getpid())
assert "AGENT_BROWSER_IDLE_TIMEOUT_MS" not in captured["env"]
self._reset()
def test_reuses_only_session_on_our_copy_dir(self, tmp_path):
@@ -339,6 +347,35 @@ class TestRealProfileCdpLaunch:
assert cdp == "http://127.0.0.1:41000"
self._reset()
@pytest.mark.parametrize("live_browser_id", ["/devtools/browser/x", "/devtools/browser/other"])
def test_reattaches_to_surviving_chrome_instead_of_overlaying_its_profile(self, tmp_path, live_browser_id):
"""The attach daemon of a crashed owner gets reaped, but its Chrome (Hermes-launched,
own session) survives holding the copy dir: re-attach, never re-run the snapshot.
A DevToolsActivePort left by a crash whose port was recycled by ANOTHER CDP server
(browser id mismatch) must not be attached to; the normal launch path runs."""
import tools.browser_tool as bt
self._reset()
(tmp_path / "DevToolsActivePort").write_text("41000\n/devtools/browser/x\n")
version = Mock()
version.json.return_value = {"webSocketDebuggerUrl": f"ws://127.0.0.1:41000{live_browser_id}"}
with patch.object(bt_cloud, "_use_real_profile", return_value=True), \
patch("hermes_cli.browser_connect.detect_default_chromium", return_value="chrome"), \
patch("hermes_cli.browser_connect.real_profile_copy_dir", return_value=str(tmp_path)), \
patch("hermes_cli.browser_connect.snapshot_real_profile", return_value=(None, "boom")) as snapshot, \
patch("requests.get", return_value=version), \
patch.object(bt_real_profile, "_agent_browser_get_cdp", return_value=None), \
patch.object(bt_real_profile, "_attach_agent_browser_to_real_profile",
return_value=("http://127.0.0.1:41000", None)) as attach:
cdp, err = bt_real_profile._real_profile_cdp()
if live_browser_id == "/devtools/browser/x":
assert (cdp, err) == ("http://127.0.0.1:41000", None)
attach.assert_called_once_with(41000, str(tmp_path))
snapshot.assert_not_called()
else:
attach.assert_not_called()
snapshot.assert_called_once()
self._reset()
def test_cdp_on_data_dir_matches_devtoolsactiveport(self, tmp_path):
(tmp_path / "DevToolsActivePort").write_text("41000\n/devtools/browser/x\n")
assert bt_real_profile._cdp_on_data_dir("http://127.0.0.1:41000", str(tmp_path))
+7 -1
View File
@@ -360,13 +360,19 @@ def _reap_orphaned_browser_sessions():
tmpdir = _bt._socket_safe_tmpdir()
socket_dirs = []
for prefix in ("agent-browser-h_*", "agent-browser-cdp_*", "agent-browser-hermes_*"):
# The shared real-profile attach daemon is named, not ``<prefix>_<hex>``; list it explicitly.
for prefix in ("agent-browser-h_*", "agent-browser-cdp_*", "agent-browser-hermes_*",
f"agent-browser-{_bt._REAL_PROFILE_SESSION}"):
socket_dirs += glob.glob(os.path.join(tmpdir, prefix))
if not socket_dirs:
return
with _bt._cleanup_lock:
tracked_names = {info.get("session_name") for info in _bt._active_sessions.values() if info.get("session_name")}
# Browsing on the shared real-profile daemon runs through per-task ``rp_*`` sessions
# (``--cdp``), so its own dir never shows activity; the idle escape hatch would misfire
# under a live user. Owner liveness alone gates it — a dead owner still gets reaped.
tracked_names.add(_bt._REAL_PROFILE_SESSION)
reaped = 0
for socket_dir in socket_dirs:
+49 -2
View File
@@ -36,6 +36,18 @@ def _cdp_http_ready(http_cdp: str) -> bool:
return _cdp_ready(http_cdp, timeout=1.0)
def _real_profile_daemon_env() -> dict:
"""Reaper-visible socket dir + ``owner_pid`` claim like every other lane (agent-browser's
default dir is invisible to the reaper — #100855). The daemon-side idle timeout is dropped:
Chrome is launched by Hermes, not the daemon, so a self-exiting daemon would leave Chrome
holding the copy dir under the next snapshot overlay."""
_bt = _origin()
socket_dir = _session._prepare_session_socket_dir(_bt._REAL_PROFILE_SESSION)
env = _session._agent_browser_command_env(socket_dir)
env.pop("AGENT_BROWSER_IDLE_TIMEOUT_MS", None)
return env
def _agent_browser_session_cmd(session_name: str, *cmd: str, log_label: str) -> Optional[subprocess.CompletedProcess]:
"""Run ``agent-browser --session <name> <cmd...>``; None when agent-browser is missing or the run fails."""
_bt = _origin()
@@ -46,7 +58,7 @@ def _agent_browser_session_cmd(session_name: str, *cmd: str, log_label: str) ->
try:
return subprocess.run([*_session._agent_browser_argv(browser_cmd), "--session", session_name, *cmd],
capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=15,
env=_bt._build_browser_env(), stdin=subprocess.DEVNULL)
env=_real_profile_daemon_env(), stdin=subprocess.DEVNULL)
except (subprocess.SubprocessError, OSError) as e:
_bt.logger.debug("real-profile %s failed: %s", log_label, e)
return None
@@ -68,6 +80,26 @@ def _read_devtools_port(data_dir: str) -> Optional[str]:
return None
def _surviving_chrome_cdp(data_dir: str) -> Optional[str]:
"""HTTP CDP root of a Chrome still running on ``data_dir``, or None. ``DevToolsActivePort``
outlives a crashed Chrome and its port can be recycled by another local CDP server, so the
file's browser id (line 2) must match what ``/json/version`` reports before it is trusted."""
try:
with open(os.path.join(data_dir, "DevToolsActivePort"), encoding="utf-8") as fh:
port, browser_path = fh.readline().strip(), fh.readline().strip()
except OSError:
return None
if not port.isdigit() or not browser_path.startswith("/devtools/browser/"):
return None
http_cdp = f"http://127.0.0.1:{port}"
try:
import requests
ws_url = str(requests.get(f"{http_cdp}/json/version", timeout=2).json().get("webSocketDebuggerUrl") or "")
except Exception:
return None
return http_cdp if ws_url.endswith(browser_path) else None
def _cdp_on_data_dir(http_cdp: str, data_dir: str) -> bool:
"""True when the CDP endpoint's browser runs on ``data_dir`` (DevToolsActivePort match proves it
is our profile copy, not a throwaway temp dir a raced/stale launch fell back to)."""
@@ -169,7 +201,7 @@ def _attach_agent_browser_to_real_profile(port: int, copy_dir: str) -> Tuple[Opt
"--cdp", str(port), "open", "about:blank"]
try:
proc = subprocess.run(argv, capture_output=True, text=True, encoding="utf-8", errors="replace",
timeout=_bt._get_open_command_timeout(first_open=True), env=_bt._build_browser_env(),
timeout=_bt._get_open_command_timeout(first_open=True), env=_real_profile_daemon_env(),
stdin=subprocess.DEVNULL)
except subprocess.TimeoutExpired:
return None, _RP + "the real-profile browser took too long to start. Retry, or turn the toggle off."
@@ -219,6 +251,9 @@ def _real_profile_cdp() -> tuple:
with _bt._real_profile_cdp_lock:
cached = _bt._real_profile_cdp_cache.get("cdp")
if cached and _cdp_http_ready(cached):
# Re-claim the shared daemon's socket dir so the orphan reaper's idle clock sees
# this process still using it (a cache hit never runs a daemon command).
_session._prepare_session_socket_dir(_bt._REAL_PROFILE_SESSION)
return cached, None
_bt._real_profile_cdp_cache.pop("cdp", None)
@@ -237,6 +272,18 @@ def _real_profile_cdp() -> tuple:
return existing, None
if existing: # stale/wrong-dir session: close it so nothing holds the dir open
_agent_browser_close_session(_bt._REAL_PROFILE_SESSION)
# A Chrome from an earlier hermes process can still hold the copy dir after its attach
# daemon was reaped (that owner died). Re-attach to it rather than overlay a live profile;
# if the daemon cannot attach, fail closed — never snapshot over an open profile. Not ours
# to terminate (no Popen handle): it lives until the user closes it, by design.
surviving = _surviving_chrome_cdp(copy_dir)
if surviving:
cdp, err = _attach_agent_browser_to_real_profile(int(surviving.rsplit(":", 1)[1]), copy_dir)
if not cdp:
return None, err
_bt._real_profile_cdp_cache["cdp"] = cdp
_bt.logger.info("real-profile: re-attached to surviving Chrome at %s (%s)", cdp, copy_dir)
return cdp, None
copy_dir, err = snapshot_real_profile(browser)
if err or not copy_dir: