diff --git a/tests/tools/test_browser_orphan_reaper.py b/tests/tools/test_browser_orphan_reaper.py index 692a4be738..429863b739 100644 --- a/tests/tools/test_browser_orphan_reaper.py +++ b/tests/tools/test_browser_orphan_reaper.py @@ -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 ``_`` + 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). diff --git a/tests/tools/test_browser_real_profile.py b/tests/tools/test_browser_real_profile.py index 8483a46e7f..8266174984 100644 --- a/tests/tools/test_browser_real_profile.py +++ b/tests/tools/test_browser_real_profile.py @@ -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)) diff --git a/tools/browser_tool_lifecycle.py b/tools/browser_tool_lifecycle.py index 4e8451a6b2..1a67476422 100644 --- a/tools/browser_tool_lifecycle.py +++ b/tools/browser_tool_lifecycle.py @@ -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 ``_``; 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: diff --git a/tools/browser_tool_real_profile.py b/tools/browser_tool_real_profile.py index 4d2624a948..103bc7a64c 100644 --- a/tools/browser_tool_real_profile.py +++ b/tools/browser_tool_real_profile.py @@ -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 ``; 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: