Hold one Allow popup per Chrome run instead of churning it

[johnblythe]

Fixes #631.

Chrome 144+ charges one "Allow remote debugging?" approval per new CDP connection, so the daemon's single held connection is what makes approval one-time per Chrome run. Four bugs were churning that connection, so users got asked repeatedly (details and repro sketches in #631). This PR makes the invariant hold:

Changes

daemon.py

• LOCAL_HANDSHAKE_TIMEOUT is now BH_ALLOW_TIMEOUT (default 600s, was 45s). The daemon parks its one WS handshake on the popup, so this is how long the popup stays clickable. Generous by default because every expiry costs a fresh popup on the next attempt.
• __main__ now refuses to start when a sibling daemon is mid-connect (fresh pid file whose process is alive but not answering pings, i.e. parked on the popup). Previously a second invocation during the handshake window spawned a duplicate daemon: second popup, truncated log, clobbered pid file.

admin.py

• New _parked_pid(): detects a daemon that is alive but not serving IPC yet. Freshness-bounded (pid files older than the popup window never count) so a reused PID can't wedge spawning.
• New _join_parked_daemon(): invocations that find a parked daemon wait for the one click instead of spawning. The permission-blocked error it raises tells callers the popup is still on screen and that retries attach to the same pending connection.
• ensure_daemon no longer kills a daemon that is parked on the popup when wait expires; it leaves it holding the popup and tells the caller to retry after approval. Only a daemon that actually gave up (handshake window expired) triggers a fresh spawn.
• restart_daemon can now terminate a parked daemon (previously invisible to it, leaving an orphan holding a stale popup). The pid is only signaled when the pid file is fresh AND the process command line proves it is a browser_harness.daemon; pid numbers alone are still never trusted, preserving the existing PID-reuse guarantees.
• The staleness probe retries with growing timeouts (3s/6s/10s) instead of 2x3s. A dead CDP WS still restarts fast (it answers each probe instantly with an error), while a slow-but-alive Chrome gets patience instead of paying a restart popup.
• New _allow_action() centralizes the platform-specific approval hint (mac-approve on macOS, click Allow elsewhere) used by the existing hint and the new messages.

SKILL.md

• Gotcha updated with the new contract: a pending popup is held ~10 minutes, retries attach to it, and agents must not restart_daemon() while one is pending.

Interaction with mac-approve

Complementary: mac-approve removes the click on macOS with Accessibility granted; this PR guarantees there is only ever one pending popup for it (or the user) to accept, on every platform, with no extra permissions.

Tests

10 new unit tests in tests/unit/test_admin.py covering parked-pid detection (freshness bound, dead process), join semantics (comes-alive, still-pending raise, parked-daemon-exited fallthrough), no-spawn-while-parked, and the cmdline-verified parked kill (including the no-proof negative case). Full suite: 133 passed.

🤖 Generated with Claude Code

---

Summary by cubic

Stops churn of Chrome’s “Allow remote debugging?” popup by holding a single pending CDP handshake and making all invocations join it. Previously, retries spawned new daemons and new popups; now they wait on the same approval and reconnect without extra prompts.

• Holds the pending popup for BH_ALLOW_TIMEOUT (default 600s; was 45s). If approval is still pending when ensure_daemon() wait expires, it raises permission-blocked and leaves the daemon parked.
• ensure_daemon() joins a parked daemon instead of spawning; staleness probes use 3s/6s/10s timeouts to avoid restarting a busy-but-alive Chrome.
• restart_daemon() can terminate a parked daemon after verifying its command line; it never signals a PID without proof (PID reuse remains guarded).
• The daemon refuses to start if a sibling is mid-connect (prevents a second popup, log truncation, and PID-file clobber).
• Adds platform-specific approval hints via _allow_action(); SKILL.md documents the new contract.
• Tests cover parked detection, join semantics, “no spawn while parked”, and safe termination.

Migration

• Treat permission-blocked as “the popup is on screen.” Instruct users to click Allow (or run browser-harness mac-approve on macOS), then retry the same command.
• Do not call restart_daemon() while approval is pending; that dismisses the popup and forces a new one. Only use it for a truly dead connection.

^{Written for commit d523068. Summary will update on new commits.}

[browser-harness-review]

✅ Skill review passed

Reviewed 1 file(s) — no findings.

[cubic-dev-ai]

6 issues found across 4 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/browser_harness/admin.py">

<violation number="1" location="src/browser_harness/admin.py:441">
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 resp` and 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.</violation>

<violation number="2" location="src/browser_harness/admin.py:641">
P1: On Windows, `restart_daemon` can never terminate a parked daemon because `_harness_daemon_cmdline` returns `False` for `win32`. Add a Windows command-line identity check before signaling; otherwise restart removes the PID file but leaves the handshake alive, and the next retry starts a second daemon and popup.</violation>
</file>

<file name="src/browser_harness/daemon.py">

<violation number="1" location="src/browser_harness/daemon.py:725">
P1: When two daemon processes start before either writes `PID`, both checks return no parked daemon, so both can create duplicate Allow popups. Protect the parked check and PID creation with an atomic lock or exclusive file claim.</violation>

<violation number="2" location="src/browser_harness/daemon.py:726">
P2: When a remote daemon is still connecting, this guard makes a concurrent child exit, but `ensure_daemon` joins parked daemons only in local mode. Add a remote startup-join path or limit this guard to local Chrome so concurrent remote starts do not fail.</violation>

<violation number="3" location="src/browser_harness/daemon.py:726">
P1: When a crashed daemon’s fresh PID is reused by another live process, `_parked_pid` treats that process as parked and this guard refuses every new daemon. Verify daemon identity before treating a fresh PID file as a pending handshake.</violation>
</file>

<file name="tests/unit/test_admin.py">

<violation number="1" location="tests/unit/test_admin.py:742">
P3: The three `_join_parked_daemon` tests (`...returns_true_once_daemon_comes_alive`, `...raises_permission_blocked_while_popup_pending`, `...falls_through_when_parked_daemon_exits`) don't patch `admin.time.sleep` or `admin.time.time`, so each runs real wall-clock waits (`wait=10.0`, `wait=0.4`, and 0.3s sleep steps). The other tests in this file patch `admin.time.sleep` (see the restart test). Patch the time functions so these unit tests run deterministically and avoid ~1.5s of injected wall-clock delay in the suite.</violation>
</file>

<sub>Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: On Windows, restart_daemon can never terminate a parked daemon because _harness_daemon_cmdline returns False for win32. Add a Windows command-line identity check before signaling; otherwise restart removes the PID file but leaves the handshake alive, and the next retry starts a second daemon and popup.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/admin.py, line 641:

<comment>On Windows, `restart_daemon` can never terminate a parked daemon because `_harness_daemon_cmdline` returns `False` for `win32`. Add a Windows command-line identity check before signaling; otherwise restart removes the PID file but leaves the handshake alive, and the next retry starts a second daemon and popup.</comment>

<file context>
@@ -526,6 +630,20 @@ def restart_daemon(name=None):
+        # 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)
</file context>

[cubic-dev-ai]

P1: When a crashed daemon’s fresh PID is reused by another live process, _parked_pid treats that process as parked and this guard refuses every new daemon. Verify daemon identity before treating a fresh PID file as a pending handshake.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/daemon.py, line 726:

<comment>When a crashed daemon’s fresh PID is reused by another live process, `_parked_pid` treats that process as parked and this guard refuses every new daemon. Verify daemon identity before treating a fresh PID file as a pending handshake.</comment>

<file context>
@@ -714,6 +721,15 @@ def already_running():
         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
</file context>

[cubic-dev-ai]

P1: When two daemon processes start before either writes PID, both checks return no parked daemon, so both can create duplicate Allow popups. Protect the parked check and PID creation with an atomic lock or exclusive file claim.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/daemon.py, line 725:

<comment>When two daemon processes start before either writes `PID`, both checks return no parked daemon, so both can create duplicate Allow popups. Protect the parked check and PID creation with an atomic lock or exclusive file claim.</comment>

<file context>
@@ -714,6 +721,15 @@ def 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
</file context>

[cubic-dev-ai]

P2: When a remote daemon is still connecting, this guard makes a concurrent child exit, but ensure_daemon joins parked daemons only in local mode. Add a remote startup-join path or limit this guard to local Chrome so concurrent remote starts do not fail.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/daemon.py, line 726:

<comment>When a remote daemon is still connecting, this guard makes a concurrent child exit, but `ensure_daemon` joins parked daemons only in local mode. Add a remote startup-join path or limit this guard to local Chrome so concurrent remote starts do not fail.</comment>

<file context>
@@ -714,6 +721,15 @@ def already_running():
         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
</file context>

[cubic-dev-ai]

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 resp and 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

Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/admin.py, line 441:

<comment>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 resp` and 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.</comment>

<file context>
@@ -345,18 +433,25 @@ def ensure_daemon(wait=60.0, name=None, env=None):
+        # 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:
</file context>

[cubic-dev-ai]

P3: The three _join_parked_daemon tests (...returns_true_once_daemon_comes_alive, ...raises_permission_blocked_while_popup_pending, ...falls_through_when_parked_daemon_exits) don't patch admin.time.sleep or admin.time.time, so each runs real wall-clock waits (wait=10.0, wait=0.4, and 0.3s sleep steps). The other tests in this file patch admin.time.sleep (see the restart test). Patch the time functions so these unit tests run deterministically and avoid ~1.5s of injected wall-clock delay in the suite.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/test_admin.py, line 742:

<comment>The three `_join_parked_daemon` tests (`...returns_true_once_daemon_comes_alive`, `...raises_permission_blocked_while_popup_pending`, `...falls_through_when_parked_daemon_exits`) don't patch `admin.time.sleep` or `admin.time.time`, so each runs real wall-clock waits (`wait=10.0`, `wait=0.4`, and 0.3s sleep steps). The other tests in this file patch `admin.time.sleep` (see the restart test). Patch the time functions so these unit tests run deterministically and avoid ~1.5s of injected wall-clock delay in the suite.</comment>

<file context>
@@ -686,3 +686,127 @@ def test_process_start_time_returns_none_for_invalid_pid():
+    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)
</file context>