Fix CDP reconnect when Chrome has zero open tabs#17
Conversation
connect_over_cdp throws Browser.setDownloadBehavior instead of returning an empty contexts list when no tabs are open, so the empty-contexts fallback never ran. Wrap the first connect attempt too, and prefer Playwright's bundled Chromium in browser-service since it supports the CDP multi-context calls system Chrome rejects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reusing the same profile dir doesn't carry the session over when bin/browser-service switches CHROME_BIN to Playwright's Chromium — operators hit "not logged in" until they log in again in the new window. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
install.sh's persona sync unconditionally re-ran on every reinstall (defaulting yes interactively, always running non-interactively), overwriting an already-configured persona.json. Now it detects an existing, non-example persona.json and keeps it by default. outreach-upgrade also hardcoded CHROME_BIN to system Chrome before calling browser-service install, undoing this branch's preference for Playwright's Chromium on every upgrade. It now lets resolve_chrome pick. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| if self._browser.contexts | ||
| else await self._browser.new_context() | ||
| ) | ||
| self._ctx = self._browser.contexts[0] |
There was a problem hiding this comment.
Possible IndexError if contexts is still empty after the retry.
self._browser.contexts[0] has no guard. Trace: if connect_over_cdp throws (line 656), we open a blank tab and reconnect (661-662); we then fall into if not self._browser.contexts (665) unconditionally and, if still empty, open another blank tab and reconnect again (671-672) — but there's no check after that. If the CDP HTTP /json/new call races with Playwright's context enumeration (tab created but not yet visible to the fresh CDP connection, or something closes it in between), contexts can still be [] here and this raises IndexError, whereas the old ... else await self._browser.new_context() fallback would have degraded gracefully instead of crashing.
| self._browser = await self._pw.chromium.connect_over_cdp(self.cdp_url) | ||
| try: | ||
| self._browser = await self._pw.chromium.connect_over_cdp(self.cdp_url) | ||
| except Error: |
There was a problem hiding this comment.
except Error: is broader than the documented zero-tabs case.
Playwright's Error is the base exception for basically all CDP failures — wrong port, Chrome not listening, profile lock, auth issues. All of these now silently trigger an extra _open_blank_tab HTTP round-trip (with its own 5s timeout) plus a blind reconnect before the real failure surfaces, instead of failing fast with the original, more diagnostic error. Worth narrowing to the specific zero-tabs signature (e.g. matching the Browser.setDownloadBehavior message mentioned in the CHANGELOG) so unrelated connection failures aren't delayed and re-reported as a different exception type from _open_blank_tab's own urlopen call.
| self._browser = await self._pw.chromium.connect_over_cdp(self.cdp_url) | ||
| self._is_attached = True | ||
|
|
||
| if not self._browser.contexts: |
There was a problem hiding this comment.
Duplicated retry logic — same three-line recovery sequence written twice.
The except Error branch (656-662) and this if not self._browser.contexts branch (665-672) both do: call _open_blank_tab, then connect_over_cdp again. Same recipe, different trigger. Worth collapsing into one small retry loop (e.g. for _ in range(2): try connect; if contexts: break; except Error: pass; open_blank_tab()) so a future fix (backoff, retry limit, better error message) only has to be applied once instead of risking the two copies drifting apart.
| empty_browser = MagicMock(contexts=[]) | ||
| populated_browser = MagicMock(contexts=[ctx]) | ||
| pw_chromium = MagicMock() | ||
| pw_chromium.connect_over_cdp = AsyncMock( |
There was a problem hiding this comment.
The actual root-cause path (connect_over_cdp raising) is never tested.
Per the CHANGELOG, the real bug is that connect_over_cdp itself throws when there are zero open tabs (not that it returns an empty contexts list). But this test's side_effect=[empty_browser, populated_browser] makes connect_over_cdp return successfully both times — it exercises the if not self._browser.contexts: branch (browser.py:665), not the except Error: branch (browser.py:656) that the CHANGELOG says is the actual fix. Consider adding a case where connect_over_cdp's first call raises (e.g. side_effect=[Error("..."), fake_browser]) to cover that branch directly.
| local persona="${REPO_ROOT}/outreach/config/persona.json" | ||
| local example="${REPO_ROOT}/outreach/config/persona.json.example" | ||
| [[ -f "${persona}" ]] || return 1 | ||
| [[ -f "${example}" ]] && cmp -s "${persona}" "${example}" && return 1 |
There was a problem hiding this comment.
No corruption/validity check before keeping an existing persona.json.
persona_already_configured only compares against persona.json.example via cmp. If a previous install was interrupted (Ctrl-C, disk full) leaving a truncated/invalid JSON file, this still returns "configured" (it differs from the example), and a non-interactive reinstall now permanently keeps the corrupt file (return 0 at line 666) instead of the old behavior of always re-syncing. Might be worth a quick validity check (e.g. python -c "import json; json.load(open(...))") before treating it as configured.
| # Let browser-service resolve_chrome pick the binary (Playwright's | ||
| # Chromium first, then system Chrome) instead of forcing system Chrome | ||
| # here — forcing it would undo that preference on every upgrade. | ||
| if OUTREACH_REPO_ROOT="$REPO_ROOT" "$BROWSER_SVC" install; then |
There was a problem hiding this comment.
Removed guard for "no Chrome binary found at all" changes the failure message, not just the Chrome-choice logic.
The old code only called browser-service install when a chrome_bin was actually found; now it's called unconditionally. On a machine with no system Chrome and no Playwright Chromium installed yet, resolve_chrome fails inside browser-service, this if takes the else branch, and prints "skipped (no launchd/systemd)" (line 169) — which is the wrong diagnostic; the real cause is "no Chrome binary found," unrelated to launchd/systemd, and will misdirect anyone troubleshooting from this log line.
| # multi-context calls (Browser.setDownloadBehavior, Target.createBrowserContext) | ||
| # that a real Google Chrome / system Chromium install rejects — using it here | ||
| # avoids that class of CDP error entirely instead of working around it. | ||
| command -v uv >/dev/null 2>&1 || return 1 |
There was a problem hiding this comment.
resolve_playwright_chromium spawns a full uv run + Python + Playwright-import subprocess with no caching.
This runs on every resolve_chrome call, i.e. every install/daemon/start invocation — including launchd/systemd KeepAlive respawns. A Chrome crash-loop now also crash-loops this subprocess each cycle. Since the resolved path only changes when Playwright's browser is (re)installed, worth caching it (e.g. a file next to REPO_ROOT invalidated by playwright version, or just resolving once at install time and relying on the CHROME_BIN already baked into the plist/unit).
| # ── Browser wrapper ─────────────────────────────────────────────────────────── | ||
|
|
||
|
|
||
| def _open_blank_tab(cdp_url: str) -> None: |
There was a problem hiding this comment.
Reimplements a workaround that already exists in install.sh.
install.sh's open_linkedin_tab_in_cdp (install.sh:498-503) already does the identical PUT-then-GET-fallback to {cdp}/json/new to work around the same Chrome CDP quirk. Now there are two independent implementations (bash + Python) of the same workaround to keep in sync if Chrome's CDP behavior changes again.
| req = urllib.request.Request(f"{cdp_url}/json/new", method="PUT") | ||
| try: | ||
| urllib.request.urlopen(req, timeout=5).close() | ||
| except urllib.error.URLError: |
There was a problem hiding this comment.
PUT succeeding server-side but the response read timing out still triggers a second (GET) tab-open.
except urllib.error.URLError catches more than "PUT unsupported" — a slow CDP endpoint that creates the tab but doesn't respond within the 5s timeout also lands here, and the GET retry then opens a second blank tab. Since about: URLs are in _pick_tab's skip list, neither leftover blank tab gets reused, so duplicate about:blank tabs can accumulate across attach cycles on a slow/loaded machine.
|
Fresh installs never get the Playwright-Chromium preference this PR adds.
Only |
resolve_chrome already warns "Chrome / Chromium not found" when no binary is available; the old message here blamed missing launchd/ systemd regardless of the actual cause, misdirecting troubleshooting.
persona_already_configured only compared against the example template, so a truncated/invalid file left by an interrupted prior install would be kept forever on a non-interactive reinstall instead of re-synced.
launch_chrome_cdp force-exported CHROME_BIN to system Chrome before calling bin/browser-service install, so resolve_chrome's new Playwright-Chromium preference (added for this same CDP bug) never took effect on a first install — only on bin/outreach-upgrade, which already dropped this forcing. Fresh installs now get the same preference upgrades do.
resolve_chrome runs on every install/daemon/start invocation, including launchd KeepAlive respawns, and resolve_playwright_chromium was paying a full `uv run` + Playwright-import subprocess each time with no caching — turning a Chrome crash-loop into a subprocess crash-loop too. Cache the resolved path in ~/.ebase and revalidate with -x so a Playwright upgrade (new versioned install dir) invalidates it naturally.
Catching the broad URLError meant a slow-but-successful PUT (timeout reading the response) also triggered the GET fallback, opening a second blank tab. Narrow the fallback to HTTPError (an actual "this doesn't work" response, e.g. 404/405 on older Chrome) so a timeout or connection failure propagates instead of risking a duplicate tab.
_open_blank_tab and install.sh's open_linkedin_tab_in_cdp both PUT-then- GET-fallback to Chrome's /json/new endpoint. Can't share the implementation across bash and Python (install.sh runs before there's a project env to call into), so just flag the duplication for future maintainers touching either one.
Catching the base playwright.Error swallowed every connect_over_cdp failure — wrong port, Chrome not running, auth issues — and silently padded each with a doomed retry (open a tab, reconnect, fail again) before the real error surfaced. Match on the known zero-tab error text instead and re-raise anything else immediately. Also closes the test-coverage gap this left: no test exercised the except branch itself (connect_over_cdp raising), only the separate empty-contexts branch.
If opening a blank tab via CDP and reconnecting still left contexts empty (a plausible race between the HTTP tab-open and Playwright's context enumeration), _attach indexed into an empty list and crashed with a bare IndexError. Raise a RuntimeError that names the actual failure instead.
The except-Error branch and the empty-contexts branch each did the same "open a blank tab via CDP, then reconnect" recovery, just on different triggers — two copies to keep in sync. Extract _connect_with_zero_tab_retry() so both paths share one retry loop with one attempt limit and one error message.
1 participant
Summary
LinkedInBrowser._attachnow handles the case whereconnect_over_cdpitself throwsBrowser.setDownloadBehavior: Browser context management is not supportedwhen Chrome has zero open tabs, instead of only handling an emptycontextslist after a successful connectbin/browser-servicenow prefers Playwright's bundled Chromium ("Chrome for Testing") over system Chrome, which avoids this class of CDP error entirelyTest plan
uv run pytest testing/tests/test_browser_attach.pypassesscrape_profile, confirmed the fix path via the CDP HTTP fallback🤖 Generated with Claude Code