diff --git a/docs/rfc/0013-consent-prompt-lifecycle.md b/docs/rfc/0013-consent-prompt-lifecycle.md new file mode 100644 index 0000000..2e14da0 --- /dev/null +++ b/docs/rfc/0013-consent-prompt-lifecycle.md @@ -0,0 +1,158 @@ +# RFC-0013: Surviving Chrome's consent prompt + +- **Status:** Draft +- **Priority:** P0 +- **Area:** connection +- **Depends on:** the spawn serialisation in #17 (necessary, not sufficient) + +## Summary + +Stop `chrome-cdp` from wedging the user's browser on the one-time "Allow remote debugging?" prompt. +Three changes: wait for the consent it asks for instead of abandoning it, detect the pending state instead of reporting a generic failure, and prefer the connection path that never prompts. + +## What happened + +A user's Chrome froze with the consent dialog on screen and no button responding. +It was reproduced deliberately afterwards, and the reproduction contradicted the first diagnosis. + +The initial theory was **stacked prompts**: several `chrome-cdp` processes had started at once, each found no daemon, each spawned one, and each spawned daemon attached to Chrome and raised its own prompt. +That much is real, and #17 fixes it — eight concurrent callers produced eight daemons before the fix and one after. + +But the controlled reproduction showed a **single** prompt wedges Chrome just as thoroughly. +Serialising the spawn is necessary and not sufficient. + +### What the reproduction established + +Measured against a real Chrome made debug-enabled through the `chrome://inspect/#remote-debugging` toggle: + +| Probe | While consent is pending | +|-------|--------------------------| +| TCP connect to `127.0.0.1:9222` | succeeds immediately | +| `GET /json/version` | 404 in under a millisecond | +| WebSocket upgrade to the browser endpoint | **hangs — never completes, never refuses** | + +The hang is the mechanism. +Chrome does not reject the connection while it waits for the user; it holds the upgrade open and says nothing. +So the client has no error to classify — only silence — which is why the failure surfaces as an undifferentiated timeout. + +Three consequences followed, each independently a defect: + +1. **The daemon abandons the prompt it raised.** + `chrome.Connect` dials, the upgrade hangs, the dial times out in about ten seconds, and the daemon writes its error and exits. + The modal is left on screen with nothing behind it. + Clicking Allow then grants consent to a connection that no longer exists. +2. **Ten seconds is not a human timescale for a dialog that may be invisible.** + The prompt is browser-modal and can sit behind the window. + A user who has not clicked it has usually not *seen* it, and by the time they do, the process that asked is gone. +3. **`doctor` reports readiness it never verified.** + It reads the `DevToolsActivePort` file and reports "debug endpoint reachable — Path B attach ready", handing back a `ws://` URL. + It does not probe. + The one command whose job is to answer "can I connect?" answered yes while every connection was hanging. + +A fourth observation is recorded because it cost time: **`/json/version` returning 404 is not a consent signal.** +It returns 404 in this connection mode whether or not consent has been granted — the toggle path exposes the WebSocket without the HTTP JSON API. +An early reading of that 404 as "consent pending" was wrong, and any detection built on it would be too. + +## User stories + +**US-1 — Do not freeze my browser.** +As a user, I want a tool that asks for consent to still be there when I answer, so that a prompt I did not see immediately does not leave my browser unusable. +*Acceptance:* a prompt answered minutes after it appears results in a working connection, not an orphaned dialog. + +**US-2 — Tell me what is happening.** +As a user staring at an unresponsive browser, I want to be told a consent prompt is pending and where to find it, so that I know this is a dialog and not a crash. +*Acceptance:* while the upgrade is hanging, the CLI reports a distinct pending state naming the prompt, not a generic connection failure. + +**US-3 — Do not ask at all when you do not have to.** +As a user, I want to be steered to the launch flag that skips consent entirely, so that routine use never involves a modal. +*Acceptance:* when no usable endpoint exists, the CLI recommends `--remote-debugging-port` before it recommends the toggle that prompts. + +**US-4 — Recover without losing my tabs.** +As a user whose browser is already wedged, I want to be told the actual remedy, so that I am not left force-quitting on a guess. +*Acceptance:* the failure message names the recovery and does not imply the browser has crashed. + +**US-5 — One prompt, not many.** +As a user running several commands at once, I want at most one consent request. +*Acceptance:* covered by #17; this RFC keeps it true. + +## Proposed changes + +### 1. Wait for the consent + +The daemon's initial connect must outlive the dialog. +Replace the single short dial with a bounded wait — proposal: `consent_timeout`, default **120s** — during which the daemon stays alive and keeps the pending upgrade open. + +Distinguishing the pending state from a dead endpoint is what makes a long wait safe. +A refused TCP connection is a real failure and must stay fast; a hanging *upgrade* against an *open* port is the consent signature and is the only case that earns the long wait. + +### 2. Report the pending state + +Add `CodeConsentPending = "consent_pending"` mapped to the existing connection exit code, so a caller can branch on it without a new number. + +While waiting, the CLI says a consent prompt is pending, that it is browser-modal and may be behind the window, and that Chrome will accept no other input until it is answered. +That last clause is the part a user cannot deduce, and is why a frozen browser reads as a crash. + +### 3. Make `doctor` probe + +`doctor` must attempt the upgrade rather than trusting the port file, and report one of: no endpoint, consent pending, or ready. +A diagnostic that reports readiness without testing it is worse than no diagnostic, because it sends the user looking somewhere else. + +Probing is itself a connection request, so `doctor` must reuse a live daemon when one exists rather than raising a prompt of its own. + +### 4. Prefer the path that never prompts + +When no usable endpoint exists, recommend relaunching Chrome with `--remote-debugging-port=9222` **first**, and the `chrome://inspect` toggle second with a note that it prompts on every fresh attach. +The toggle is currently presented as the primary route, which routes every new user through the failure this RFC exists to remove. + +## Verification scenarios + +**VS-1 — A hanging upgrade is classified as consent pending, not as a timeout.** +Given a listener that accepts TCP and never completes the WebSocket upgrade, when the daemon connects, then it reports `consent_pending` and stays alive rather than exiting. + +**VS-2 — A refused endpoint still fails fast.** +Given a closed port, when the daemon connects, then it fails within a second or two with `connection_failed`, not after the consent timeout. + +**VS-3 — Consent answered late still works.** +Given a listener that completes the upgrade after 30 seconds, when the daemon connects, then the connection succeeds and no prompt is orphaned. + +**VS-4 — The consent wait is bounded.** +Given a listener that never completes the upgrade, when `consent_timeout` elapses, then the daemon exits with a message naming the prompt and the recovery. + +**VS-5 — `doctor` distinguishes all three states.** +Table over: no endpoint, open-but-hanging, and ready — each reported distinctly, and the ready case verified by a completed upgrade rather than by the port file alone. + +**VS-6 — `doctor` does not raise its own prompt.** +Given a running daemon, when `doctor` runs, then it answers through the daemon and initiates no new connection. + +**VS-7 — Concurrency stays at one prompt.** +The guard from #17, restated here so this RFC's changes cannot regress it. + +## Test plan + +The pending state is a **local listener that accepts and stalls**, so almost all of this is testable with `net.Listen` and no browser at all — which matters, because the manual reproduction wedged a real browser twice and must not be the regression test. + +- **Pure/stub (`-short`):** VS-1 through VS-5 against hand-built listeners — refusing, stalling, and completing-after-a-delay. + This is where the classification logic belongs. +- **Daemon:** VS-6 and VS-7 against the existing socket harness. +- **Live Chrome:** none. + Consent cannot be granted programmatically, and a test that needs a human click is not a test. + +Note for anyone extending this: a long `t.TempDir()` path breaks a Unix socket bind on darwin — `sun_path` caps near 104 bytes and the directory embeds the test name, which fails with a bare `bind: invalid argument`. + +## Out of scope + +- Granting or suppressing consent programmatically. + It is a deliberate user decision and the tool should not try to route around it. +- Anything about Chrome's own modal behaviour, which is not ours to change. +- Windows: the project ships linux and darwin only. + +## Open questions + +1. Should the CLI **refuse to raise a prompt at all** unless the user opts in — erroring with "relaunch with `--remote-debugging-port`" instead? + Safest, but it removes the zero-config path that makes the tool pleasant on first use. + **Recommendation:** keep the prompt, fix the waiting, and lead with the flag in the docs. +2. Is 120s the right consent timeout? + Long enough for a hidden dialog, short enough that a genuinely dead endpoint is not mistaken for a slow human. + **Recommendation:** 120s, as a config key so it can be argued with. +3. Should `doctor` be able to probe *without* a daemon, accepting that it may raise a prompt? + **Recommendation:** yes, but say so before doing it, since the user ran a diagnostic and did not ask to connect. diff --git a/docs/rfc/README.md b/docs/rfc/README.md index f61b5d7..31e227b 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -40,6 +40,7 @@ The ordering below follows that: make the interaction surface complete, then mak | [0010](0010-page-reading-ergonomics.md) | Page-reading ergonomics: `text --article`, `eval --await` | P2 | reading | Draft | | [0011](0011-session-recording.md) | Session recording: `record` and GIF export | P2 | capture | Draft | | [0012](0012-domain-allowlist.md) | Domain allow-list: bounding what the CLI may drive | P2 | safety | Draft | +| [0013](0013-consent-prompt-lifecycle.md) | Surviving Chrome's consent prompt | P0 | connection | Draft | ## Dependency graph diff --git a/internal/daemon/lifecycle.go b/internal/daemon/lifecycle.go index 0118289..b23ac8c 100644 --- a/internal/daemon/lifecycle.go +++ b/internal/daemon/lifecycle.go @@ -96,16 +96,38 @@ func Ensure(sockPath, exePath string, env []string) (*Client, error) { if c := TryConnect(sockPath); c != nil { return c, nil } + + // From here on, exactly one process at a time. Concurrent invocations that + // all find no daemon would otherwise each spawn one, and each spawned daemon + // attaches to Chrome — which raises a SEPARATE browser-modal "Allow remote + // debugging?" prompt. Several stacked prompts is not a slower version of + // one: the visible dialog need not be the one holding input, so the whole + // browser looks frozen with no button that responds. The daemon exists so + // that prompt happens once per session; nothing was making the FIRST attach + // single-file. + // + // The unlinks below are the other half. Outside the lock they can delete a + // socket a sibling daemon has just bound, orphaning a live daemon that no + // client can ever reach. + unlock, err := lockSpawn(sockPath) + if err != nil { + return nil, err + } + defer unlock() + + // Re-check under the lock: while we waited, the holder may have started the + // daemon we were about to duplicate. This is what makes N callers converge + // on one daemon and one prompt. + if c := TryConnect(sockPath); c != nil { + return c, nil + } + _ = os.Remove(sockPath) // clear a stale socket file _ = os.Remove(sockPath + ".err") // and a stale error, so we only read THIS spawn's - cmd := exec.Command(exePath, "__daemon", sockPath) - cmd.Env = env - cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} // detach into its own session - if err := cmd.Start(); err != nil { - return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot start daemon: " + err.Error()} + if err := spawnDaemon(exePath, sockPath, env); err != nil { + return nil, err } - _ = cmd.Process.Release() for range 100 { // up to ~10s for the first Allow-dialog click time.Sleep(100 * time.Millisecond) @@ -118,7 +140,43 @@ func Ensure(sockPath, exePath string, env []string) (*Client, error) { return nil, decodeConnectErr(data) } } - return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "daemon did not start within 10s — did you click Allow in Chrome?"} + return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "daemon did not start within 10s — Chrome may be waiting on its \"Allow remote debugging?\" prompt; it can hide behind the window, and until it is answered Chrome accepts no other input"} +} + +// spawnDaemon starts the detached daemon process. It is a variable so a test can +// substitute a spawn it can count, without a real Chrome or a real binary. +var spawnDaemon = func(exePath, sockPath string, env []string) error { + cmd := exec.Command(exePath, "__daemon", sockPath) + cmd.Env = env + cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} // detach into its own session + if err := cmd.Start(); err != nil { + return &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot start daemon: " + err.Error()} + } + _ = cmd.Process.Release() + return nil +} + +// lockSpawn takes an exclusive advisory lock covering the spawn-and-wait for one +// socket path, and returns the release. The lock file is never removed: unlinking +// it would let a later caller lock a different inode and defeat the exclusion. +// +// The wait is deliberately unbounded. The holder may be waiting out a consent +// prompt the user has not clicked yet, and blocking behind it is the correct +// outcome — spawning our own would add another prompt to the pile, which is the +// failure this exists to prevent. +func lockSpawn(sockPath string) (func(), error) { + f, err := os.OpenFile(sockPath+".lock", os.O_CREATE|os.O_RDWR, 0o600) + if err != nil { + return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot open the daemon spawn lock: " + err.Error()} + } + if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil { + _ = f.Close() + return nil, &chrome.ConnectError{Code: result.CodeDaemon, Message: "cannot take the daemon spawn lock: " + err.Error()} + } + return func() { + _ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN) + _ = f.Close() + }, nil } // RunDaemon connects Chrome and serves sockPath until idle or stopped. Used by diff --git a/internal/daemon/spawn_test.go b/internal/daemon/spawn_test.go new file mode 100644 index 0000000..73724cf --- /dev/null +++ b/internal/daemon/spawn_test.go @@ -0,0 +1,132 @@ +package daemon + +import ( + "net" + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" +) + +// TestEnsureSpawnsOneDaemonUnderConcurrency is the guard for a failure that took +// down a user's whole browser. +// +// Ensure used to check for a running daemon, find none, and spawn one — with no +// exclusion. Several chrome-cdp processes starting at once therefore each found +// nothing and each spawned a daemon, and every spawned daemon attaches to Chrome, +// raising its own browser-modal "Allow remote debugging?" prompt. Stacked prompts +// are not a slower version of one prompt: the visible dialog need not be the one +// holding input, so Chrome looks frozen with no button that responds. +// +// The unlink was the other half of it. Outside a lock, a late caller's +// os.Remove(sockPath) can delete a socket a sibling daemon has just bound, +// orphaning a live daemon nothing can reach. +func TestEnsureSpawnsOneDaemonUnderConcurrency(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + + var spawns atomic.Int32 + restore := swapSpawn(func(_, sockPath string, _ []string) error { + spawns.Add(1) + // Behave like the real daemon: bind the socket, a moment later, so the + // window between spawning and being connectable is real rather than + // instantaneous. Every caller must still converge on this one listener. + go func() { + time.Sleep(150 * time.Millisecond) + ln, err := net.Listen("unix", sockPath) + if err != nil { + t.Errorf("fake daemon could not bind %s: %v", sockPath, err) + return + } + t.Cleanup(func() { _ = ln.Close() }) + for { + c, err := ln.Accept() + if err != nil { + return + } + _ = c.Close() + } + }() + return nil + }) + defer restore() + + const callers = 8 + var wg sync.WaitGroup + errs := make([]error, callers) + clients := make([]*Client, callers) + for i := range callers { + wg.Add(1) + go func() { + defer wg.Done() + clients[i], errs[i] = Ensure(sock, "unused", nil) + }() + } + wg.Wait() + + for i, err := range errs { + if err != nil { + t.Errorf("caller %d: Ensure failed: %v", i, err) + } else if clients[i] == nil { + t.Errorf("caller %d: got a nil client and no error", i) + } + } + if got := spawns.Load(); got != 1 { + t.Fatalf("%d daemons were spawned for %d concurrent callers, want exactly 1 — "+ + "each spawn attaches to Chrome and raises its own consent prompt", got, callers) + } +} + +// TestEnsureReusesARunningDaemon pins the fast path: an already-listening socket +// is connected to without taking the lock or spawning anything. +func TestEnsureReusesARunningDaemon(t *testing.T) { + sock := filepath.Join(shortTempDir(t), "d.sock") + ln, err := net.Listen("unix", sock) + if err != nil { + t.Fatalf("listen: %v", err) + } + defer ln.Close() + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + _ = c.Close() + } + }() + + var spawns atomic.Int32 + restore := swapSpawn(func(string, string, []string) error { + spawns.Add(1) + return nil + }) + defer restore() + + if _, err := Ensure(sock, "unused", nil); err != nil { + t.Fatalf("Ensure against a live daemon: %v", err) + } + if got := spawns.Load(); got != 0 { + t.Errorf("spawned %d daemons while one was already running, want 0", got) + } +} + +// shortTempDir returns a temp dir with a SHORT path. A Unix socket address is +// capped near 104 bytes on darwin, and t.TempDir() embeds the test's name — long +// enough here to fail the bind with a bare "invalid argument". +func shortTempDir(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "cdpd") + if err != nil { + t.Fatalf("MkdirTemp: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + return dir +} + +func swapSpawn(fn func(exePath, sockPath string, env []string) error) func() { + prev := spawnDaemon + spawnDaemon = fn + return func() { spawnDaemon = prev } +} diff --git a/skills/drive-chrome-cdp/SKILL.md b/skills/drive-chrome-cdp/SKILL.md index 56c45ca..eb2b57f 100644 --- a/skills/drive-chrome-cdp/SKILL.md +++ b/skills/drive-chrome-cdp/SKILL.md @@ -23,6 +23,8 @@ Because it drives the real profile, live logins are reused: **type no credential `chrome-cdp daemon status --json` shows it; `--no-daemon` bypasses it. 3. **Avoid re-triggering the consent prompt.** A fresh attach (the first command after `daemon stop`, or after a Chrome restart) re-shows Chrome's "Allow remote debugging?" prompt; if it isn't clicked it can wedge Chrome. + The prompt is **browser-modal**, so an unanswered one freezes the whole browser, not just the tab — and it can hide behind the window. + Answer it before assuming Chrome has crashed. Keep the daemon alive — don't `daemon stop` mid-session. If a command returns `connection_failed`, its message now says whether to click the Allow prompt (it can hide behind the window) or restart Chrome. To skip the prompt entirely, have the user launch Chrome with `--remote-debugging-port=9222` (e.g. `open -a "Google Chrome" --args --remote-debugging-port=9222`).