From d523068bfbb66ff13817e845d68d931c39bb20f5 Mon Sep 17 00:00:00 2001 From: johnblythe Date: Thu, 20 Aug 2026 10:12:14 -0400 Subject: [PATCH] Hold one Allow popup per Chrome run instead of churning it Chrome 144+ charges one 'Allow remote debugging?' popup per new CDP connection, so the daemon's single held connection is meant to make the click one-time per Chrome run. Four bugs broke that: 1. ensure_daemon killed the daemon parked on the popup after its wait expired and told callers to retry -- each retry spawned a new daemon and a new popup. restart_daemon could not even see a parked daemon (no IPC yet), so it orphaned the old popup while stacking a new one. 2. LOCAL_HANDSHAKE_TIMEOUT was 45s -- the popup expired before a user who was not watching Chrome could click it. 3. A parked daemon cannot answer pings, so concurrent invocations passed already_running() and spawned sibling daemons: a second popup, plus the sibling truncated the first daemon's log and clobbered its pid file. 4. The staleness probe gave a live daemon two 3s tries before killing it -- a busy Chrome tripped it and paid a popup for nothing. Now the daemon holds a pending popup for BH_ALLOW_TIMEOUT (default 600s); invocations that find a parked daemon join it and wait for the one click instead of spawning; a parked daemon is left running when ensure_daemon's wait expires (errors tell callers the popup is still on screen and to retry, not restart); restart_daemon can kill a parked daemon after verifying its command line (pid numbers alone are never trusted); and the staleness probe retries with growing timeouts so a dead WS still restarts fast (it fails instantly) while a slow Chrome gets patience. Co-Authored-By: Claude Fable 5 --- SKILL.md | 2 +- src/browser_harness/admin.py | 140 +++++++++++++++++++++++++++++++--- src/browser_harness/daemon.py | 20 ++++- tests/unit/test_admin.py | 124 ++++++++++++++++++++++++++++++ 4 files changed, 272 insertions(+), 14 deletions(-) diff --git a/SKILL.md b/SKILL.md index 102e71bc..7180fc5a 100644 --- a/SKILL.md +++ b/SKILL.md @@ -171,7 +171,7 @@ If you get stuck on a browser mechanic, check https://github.com/browser-use/bro ## Gotchas - `chrome://inspect/#remote-debugging` must be enabled for local Chrome control. -- On macOS, if Chrome shows an "Allow remote debugging?" popup, run `browser-harness mac-approve`. Do not poll in a loop — the daemon holds one connection. +- On macOS, if Chrome shows an "Allow remote debugging?" popup, run `browser-harness mac-approve`; elsewhere ask the user to click Allow. Chrome charges one popup per new CDP connection, and the daemon's single held connection is what makes approval one-time per Chrome run. A pending popup is held open for ~10 minutes (`BH_ALLOW_TIMEOUT`), and retries attach to that same pending connection — a `permission-blocked` error saying the popup is still on screen means exactly that: approve it, then retry the same command. Never call `restart_daemon()` while a popup is pending; that dismisses it and forces a fresh one. Reserve `restart_daemon()` for a genuinely dead connection (e.g. `no close frame received or sent`), and expect the reconnect to cost one approval. - Omnibox popups are not real work tabs. - CDP target order is not Chrome's visible tab-strip order. - `BU_CDP_URL` is an HTTP DevTools endpoint; the daemon resolves it to WebSocket. diff --git a/src/browser_harness/admin.py b/src/browser_harness/admin.py index 3b893039..19b02e5e 100644 --- a/src/browser_harness/admin.py +++ b/src/browser_harness/admin.py @@ -125,6 +125,12 @@ def _load_env_file(p): _load_env() NAME = os.environ.get("BU_NAME", "default") +# Mirrors daemon.py's LOCAL_HANDSHAKE_TIMEOUT (same env var): how long a daemon +# parks its CDP opening handshake on Chrome's "Allow remote debugging?" popup. +try: + _ALLOW_POPUP_TIMEOUT = int(os.environ.get("BH_ALLOW_TIMEOUT", "600")) +except ValueError: + _ALLOW_POPUP_TIMEOUT = 600 BU_API = "https://api.browser-use.com/api/v3" PYPI_JSON = "https://pypi.org/pypi/browser-harness/json" VERSION_CACHE = paths.config_dir() / "version-cache.json" @@ -337,6 +343,88 @@ def run_doctor_fix_snap(): return 0 +def _parked_pid(name=None): + """PID of a daemon that is alive but not serving IPC yet — parked on the CDP + opening handshake, keeping Chrome's "Allow remote debugging?" popup on screen. + + None when no such daemon exists. A parked daemon holds the one pending + connection whose popup the user must click; spawning a second daemon stacks + a second popup and dismiss-races the first, so callers treat parked as + "join and wait", never "restart". Only fresh pid files count (a parked + daemon is by definition younger than the handshake window) — an ancient pid + file whose number was reused by an unrelated process must not wedge spawns.""" + p = ipc.pid_path(name or NAME) + try: + age = time.time() - p.stat().st_mtime + pid = int(p.read_text(encoding="utf-8").strip()) + except (OSError, ValueError): + return None + if pid <= 0 or age > _ALLOW_POPUP_TIMEOUT + 120: + return None + return pid if _process_start_time(pid) is not None else None + + +def _harness_daemon_cmdline(pid): + """True when PID's command line is a browser-harness daemon. + + Identity check for signaling a parked daemon: it has no IPC yet, so the + identify()-based verification restart_daemon normally requires can't run. + The command line is the next-strongest evidence that the pid file's number + still belongs to us and not to a PID-reuse victim.""" + try: + if sys.platform.startswith("linux"): + raw = Path(f"/proc/{pid}/cmdline").read_bytes().replace(b"\x00", b" ").decode("utf-8", "replace") + elif sys.platform == "darwin": + raw = subprocess.check_output( + ["ps", "-o", "command=", "-p", str(pid)], + stderr=subprocess.DEVNULL, timeout=2, + ).decode("utf-8", "replace") + else: + return False + except Exception: + return False + return "browser_harness.daemon" in raw + + +def _allow_action(): + """How the pending Allow popup gets accepted on this platform.""" + if sys.platform == "darwin": + return "run `browser-harness mac-approve` in another shell or click Allow" + return "click Allow" + + +def _join_parked_daemon(name, wait): + """When a daemon is already parked on the Allow popup, wait for the click + instead of spawning a second daemon (which would stack a second popup). + + True once the parked daemon comes alive; False when none is parked or the + parked one exited (caller proceeds to spawn); raises permission-blocked + when the popup is still pending after `wait` seconds.""" + if not _parked_pid(name): + return False + print(f'browser-harness: Chrome\'s "Allow remote debugging?" popup is already on screen — {_allow_action()} to continue.', file=sys.stderr) + deadline = time.time() + wait + parked_check_at = 0.0 + while time.time() < deadline: + if daemon_alive(name): + return True + now = time.time() + if now >= parked_check_at: + if not _parked_pid(name): + # Parked daemon gave up (handshake window expired) or crashed — + # let the caller spawn a fresh one (one new popup). + return False + parked_check_at = now + 2 + time.sleep(0.3) + raise RuntimeError( + "permission-blocked: Chrome's 'Allow remote debugging?' popup has not been accepted yet. " + f"It is STILL ON SCREEN and its connection is being held open -- {_allow_action()}, " + "then retry this command (the retry attaches to the same pending connection; no new " + "popup appears). Do NOT call restart_daemon(); that dismisses the pending popup and " + "forces a fresh one." + ) + + def ensure_daemon(wait=60.0, name=None, env=None): """Idempotent. Self-heals stale daemon, closed Chrome (launches it), cold Chrome, and missing Allow on chrome://inspect.""" @@ -345,18 +433,25 @@ def ensure_daemon(wait=60.0, name=None, env=None): # CDP WS to Chrome is dead — probe with a real CDP call and require "result". # Must go through ipc.connect so this works on Windows (TCP loopback) too; # raw AF_UNIX here would fail on every warm call and churn the daemon. - for last in (False, True): + # Chrome 144+ prices every new CDP connection at one Allow click, so a live + # daemon is only declared stale after patient probing: a dead CDP WS answers + # each probe instantly with {"error": ...} (restart stays fast), while a + # slow-but-alive Chrome only times out — it gets growing budgets before we + # pay a restart (= another popup) for what was just a busy browser. + probe_timeouts = (3.0, 6.0, 10.0) + for i, timeout in enumerate(probe_timeouts): try: - s, token = ipc.connect(name or NAME, timeout=3.0) + s, token = ipc.connect(name or NAME, timeout=timeout) resp = ipc.request(s, token, {"method": "Target.getTargets", "params": {}}) if "result" in resp: return except Exception: pass - if not last: time.sleep(0.5) + if i < len(probe_timeouts) - 1: time.sleep(0.5) restart_daemon(name) - import subprocess, sys local = _is_local_chrome_mode(env) + if local and _join_parked_daemon(name, wait): + return launched_browser = False opened_inspect = False for _ in range(3): @@ -378,22 +473,31 @@ def ensure_daemon(wait=60.0, name=None, env=None): if daemon_alive(name): return if p.poll() is not None: break if not hinted and time.time() - spawned > 2 and (_log_tail(name) or "").startswith("handshake-wait"): - action = ( - "run `browser-harness mac-approve` in another shell or click Allow" - if sys.platform == "darwin" - else "click Allow" - ) print( - f'browser-harness: Chrome is asking "Allow remote debugging?" — {action} to continue.', + f'browser-harness: Chrome is asking "Allow remote debugging?" — {_allow_action()} to continue.', file=sys.stderr, ) hinted = True time.sleep(0.2) msg = _log_tail(name) or "" if local and msg.startswith("handshake-wait"): + if p.poll() is None or _parked_pid(name) is not None: + # The daemon is still parked on the WS handshake, holding the + # Allow popup on screen. Leave it running: killing it here (the + # old behavior) dismissed the popup mid-click and cost a fresh + # popup on every retry. Retries join it via _join_parked_daemon. + raise RuntimeError( + "permission-blocked: Chrome's 'Allow remote debugging?' popup has not been accepted yet. " + f"It is STILL ON SCREEN and its connection is being held open -- {_allow_action()}, " + "then retry this command (the retry attaches to the same pending connection; no new " + "popup appears). Do NOT call restart_daemon(); that dismisses the pending popup and " + "forces a fresh one." + ) restart_daemon(name) raise RuntimeError( - "permission-blocked: Chrome's Allow popup was not clicked in time -- wait for the user to click Allow, then retry." + "permission-blocked: Chrome's Allow popup expired before it was clicked " + f"(the daemon holds it open for BH_ALLOW_TIMEOUT={_ALLOW_POPUP_TIMEOUT}s) -- " + f"retry to show a fresh popup, then {_allow_action()}." ) if local and _needs_chrome_permission_popup(msg): print('browser-harness: Chrome is asking "Allow remote debugging?". Click Allow in Chrome, then retry browser work.', file=sys.stderr) @@ -526,6 +630,20 @@ def restart_daemon(name=None): except (ProcessLookupError, OSError, SystemError, OverflowError): pass + if daemon_pid is None and not daemon_alive: + # A parked daemon (mid CDP handshake, holding Chrome's Allow popup) has + # no IPC yet, so identify()/ping() can't see it. Left alone it would + # survive this "restart" as an orphan holding a stale popup while the + # next spawn raises a second one. Signal it only when the pid file is + # fresh AND the process's command line proves it is a harness daemon — + # the pid file's number alone is never trusted (PID reuse). + parked = _parked_pid(name) + if parked is not None and _harness_daemon_cmdline(parked): + try: + os.kill(parked, signal.SIGTERM) + except (ProcessLookupError, OSError, SystemError, OverflowError): + pass + ipc.cleanup_endpoint(name) try: os.unlink(pid_path) diff --git a/src/browser_harness/daemon.py b/src/browser_harness/daemon.py index 3783ca4e..202ea7d3 100644 --- a/src/browser_harness/daemon.py +++ b/src/browser_harness/daemon.py @@ -88,8 +88,15 @@ def profile_dirs(system=None): BU_API = "https://api.browser-use.com/api/v3" REMOTE_ID = os.environ.get("BU_BROWSER_ID") BROWSER_KIND = "cloud" if REMOTE_ID else ("cdp" if (os.environ.get("BU_CDP_WS") or os.environ.get("BU_CDP_URL")) else "local") -# Chrome 144+ shows a per-connection popup. Keep popup open enough to click. -LOCAL_HANDSHAKE_TIMEOUT = 45 +# Chrome 144+ shows a per-connection popup. The daemon parks its single WS +# handshake on that popup, so this is how long the popup stays clickable before +# the daemon gives up. Generous by default — the user may not be looking at +# Chrome — because every expiry costs a fresh popup on the next attempt. +# Override with BH_ALLOW_TIMEOUT (seconds); admin.py reads the same variable. +try: + LOCAL_HANDSHAKE_TIMEOUT = int(os.environ.get("BH_ALLOW_TIMEOUT", "600")) +except ValueError: + LOCAL_HANDSHAKE_TIMEOUT = 600 # How long get_ws_url() keeps waiting for DevToolsActivePort before giving up NO_TOGGLE_GRACE = 3 TOGGLE_BOOT_GRACE = 12 @@ -714,6 +721,15 @@ def already_running(): if already_running(): print(f"daemon already running on {SOCK}", file=sys.stderr) sys.exit(0) + from .admin import _parked_pid + _parked = _parked_pid(NAME) + if _parked is not None and _parked != os.getpid(): + # A sibling daemon is mid-connect — it can't answer pings yet because it + # is parked on Chrome's Allow popup. Starting a second daemon here would + # raise a second popup, truncate the sibling's log, and clobber its pid + # file. Exit instead; ensure_daemon joins the sibling's pending popup. + print(f"daemon {_parked} is already connecting (waiting on Chrome's Allow popup)", file=sys.stderr) + sys.exit(0) open(LOG, "w").close() open(PID, "w").write(str(os.getpid())) try: diff --git a/tests/unit/test_admin.py b/tests/unit/test_admin.py index 804ed7bc..55eaacc6 100644 --- a/tests/unit/test_admin.py +++ b/tests/unit/test_admin.py @@ -686,3 +686,127 @@ def test_process_start_time_returns_none_for_invalid_pid(): ) # 2**31 - 1 is the largest pid_t; in practice no live process at that PID. assert admin._process_start_time((1 << 31) - 1) is None + + +# --- parked-daemon handling: one Allow popup per Chrome run --- + +def test_parked_pid_returns_live_pid_from_fresh_pid_file(monkeypatch, tmp_path): + import os as _os + pid_path = tmp_path / "default.pid" + pid_path.write_text(str(_os.getpid())) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + + assert admin._parked_pid("default") == _os.getpid() + + +def test_parked_pid_ignores_stale_pid_file(monkeypatch, tmp_path): + """An ancient pid file (older than the popup window) must not count as + parked even when its number matches a live process — that's the PID-reuse + wedge guard.""" + import os as _os + pid_path = tmp_path / "default.pid" + pid_path.write_text(str(_os.getpid())) + old = admin.time.time() - (admin._ALLOW_POPUP_TIMEOUT + 121) + _os.utime(pid_path, (old, old)) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + + assert admin._parked_pid("default") is None + + +def test_parked_pid_returns_none_for_dead_process(monkeypatch, tmp_path): + pid_path = tmp_path / "default.pid" + pid_path.write_text(str((1 << 31) - 1)) # no live process at pid_t max + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + + assert admin._parked_pid("default") is None + + +def test_join_parked_daemon_returns_false_when_nothing_parked(monkeypatch): + monkeypatch.setattr(admin, "_parked_pid", lambda name: None) + + assert admin._join_parked_daemon("default", wait=5.0) is False + + +def test_join_parked_daemon_returns_true_once_daemon_comes_alive(monkeypatch): + alive = iter([False, False, True]) + monkeypatch.setattr(admin, "_parked_pid", lambda name: 4242) + monkeypatch.setattr(admin, "daemon_alive", lambda name: next(alive)) + + assert admin._join_parked_daemon("default", wait=10.0) is True + + +def test_join_parked_daemon_raises_permission_blocked_while_popup_pending(monkeypatch): + monkeypatch.setattr(admin, "_parked_pid", lambda name: 4242) + monkeypatch.setattr(admin, "daemon_alive", lambda name: False) + + with pytest.raises(RuntimeError) as exc: + admin._join_parked_daemon("default", wait=0.4) + assert "permission-blocked" in str(exc.value) + assert "STILL ON SCREEN" in str(exc.value) + + +def test_join_parked_daemon_falls_through_when_parked_daemon_exits(monkeypatch): + """If the parked daemon gives up mid-wait, the caller must get False so it + can spawn a fresh daemon (one new popup) instead of raising forever.""" + parked = iter([4242, None]) + monkeypatch.setattr(admin, "_parked_pid", lambda name: next(parked, None)) + monkeypatch.setattr(admin, "daemon_alive", lambda name: False) + + assert admin._join_parked_daemon("default", wait=10.0) is False + + +def test_ensure_daemon_joins_parked_daemon_instead_of_spawning(monkeypatch): + """A second invocation while a daemon is parked on the Allow popup must + wait on that daemon, never spawn a sibling (= second popup).""" + monkeypatch.setattr(admin, "daemon_alive", lambda name=None: False) + monkeypatch.setattr(admin, "_join_parked_daemon", lambda name, wait: True) + monkeypatch.setattr( + admin.subprocess, "Popen", + lambda *a, **k: (_ for _ in ()).throw(AssertionError("spawned a second daemon while one was parked")), + ) + + admin.ensure_daemon(name="default") + + +def test_restart_daemon_sigterms_parked_daemon_with_verified_cmdline(monkeypatch, tmp_path): + """A parked daemon has no IPC, so identify()/ping() can't see it. It must + still be killed on restart — but only after its command line proves the + pid file's number is really a harness daemon.""" + import signal + pid_path = tmp_path / "default.pid" + pid_path.write_text("4242") + + kill_calls = [] + monkeypatch.setattr(admin.os, "kill", lambda pid, sig: kill_calls.append((pid, sig))) + monkeypatch.setattr(admin.ipc, "identify", lambda name, timeout=5.0: None) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: False) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", lambda name: None) + monkeypatch.setattr(admin, "_parked_pid", lambda name: 4242) + monkeypatch.setattr(admin, "_harness_daemon_cmdline", lambda pid: True) + + admin.restart_daemon("default") + + assert kill_calls == [(4242, signal.SIGTERM)] + assert not pid_path.exists() + + +def test_restart_daemon_never_signals_parked_pid_without_cmdline_proof(monkeypatch, tmp_path): + """Same setup, but the command line does not match a harness daemon — the + PID was likely reused, so no signal may fire.""" + pid_path = tmp_path / "default.pid" + pid_path.write_text("4242") + + kill_calls = [] + monkeypatch.setattr(admin.os, "kill", lambda pid, sig: kill_calls.append((pid, sig))) + monkeypatch.setattr(admin.ipc, "identify", lambda name, timeout=5.0: None) + monkeypatch.setattr(admin.ipc, "ping", lambda name, timeout=1.0: False) + monkeypatch.setattr(admin.ipc, "pid_path", lambda name: pid_path) + monkeypatch.setattr(admin.ipc, "cleanup_endpoint", lambda name: None) + monkeypatch.setattr(admin, "_parked_pid", lambda name: 4242) + monkeypatch.setattr(admin, "_harness_daemon_cmdline", lambda pid: False) + + admin.restart_daemon("default") + + assert kill_calls == [] + assert not pid_path.exists()