-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Hold one Allow popup per Chrome run instead of churning it #632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: On Windows, Prompt for AI agents |
||
| try: | ||
| os.kill(parked, signal.SIGTERM) | ||
| except (ProcessLookupError, OSError, SystemError, OverflowError): | ||
| pass | ||
|
|
||
| ipc.cleanup_endpoint(name) | ||
| try: | ||
| os.unlink(pid_path) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: When two daemon processes start before either writes Prompt for AI agents |
||
| if _parked is not None and _parked != os.getpid(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: When a crashed daemon’s fresh PID is reused by another live process, Prompt for AI agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When a remote daemon is still connecting, this guard makes a concurrent child exit, but Prompt for AI agents |
||
| # 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: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The three Prompt for AI agents |
||
| 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() | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: The new probe loop never short-circuits on a definitive dead-CDPS error. When the daemon replies with
{"error": ...}(the dead-connection case the new comment says answers 'instantly'),"result" not in respand no exception is raised, so the loop still runs all three probes (3+6+10s plus sleeps = ~20s) before restarting. That contradicts the stated 'restart stays fast' intent and regresses cold restarts from ~6s to ~20s. Break out of the loop when the response contains an error.Prompt for AI agents