fix: per-call CDP timeout + auto-reconnect on dropped websocket#508
fix: per-call CDP timeout + auto-reconnect on dropped websocket#508Rselectronic wants to merge 1 commit into
Conversation
Two failure modes that made the harness feel slow and flaky: browser-use#1 Every helper->daemon call was hard-capped at 5s. ipc.connect() sets the connect AND read timeout to one value, and _send passed 5.0, so any single CDP command that legitimately took longer (screenshots, heavy DOM reads, awaited promises, slow navigations) raised socket.timeout and killed the script. Split the two: keep a short 5s connect budget, add a separate command read budget (default 30s, screenshots 60s) tunable via BH_CMD_TIMEOUT and per-call cdp(_timeout=)/js(timeout=). browser-use#2 The daemon built its CDP websocket once and never rebuilt it. If the socket dropped (Chrome closed/restarted, remote hiccup) every subsequent call returned an error until a manual `browser-harness --reload` — a zombie daemon that still answered ping. Added _conn_dead() detection plus a locked _reconnect() that rebuilds the client and retries the failing call once; for local Chrome it re-resolves the live websocket so it also recovers from a Chrome restart. Both verified live against Chrome (8s call now succeeds; kill+restart Chrome mid-session self-heals) and covered by tests/unit/test_reconnect.py. Full suite: 112 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Skill review passedReviewed 1 file(s) — no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 547dc0e157
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -245,18 +290,27 @@ async def start(self): | |||
| ) | |||
| raise RuntimeError(f"CDP WS handshake failed: {e} -- click Allow in Chrome if prompted, then retry") | |||
| await self.attach_first_page() | |||
There was a problem hiding this comment.
When _reconnect() calls _connect_cdp() after a transient websocket drop while Chrome is still running, this unconditional attach_first_page() discards the daemon's tracked self.target_id. If the agent had switched to a second tab, the retry below and future default-session calls can run against whichever page Target.getTargets returns first instead of the selected tab; reattach/activate the previous target when it still exists, or fail rather than silently switching context.
Useful? React with 👍 / 👎.
| except Exception as re: | ||
| log(f"reconnect failed: {re}") | ||
| return {"error": f"{msg} -- CDP reconnect failed: {re}"} | ||
| retry_sid = None if method.startswith("Target.") else self.session |
There was a problem hiding this comment.
When the failed request carried an explicit session_id (for example js(..., target_id=iframe_target(...))), reconnecting invalidates that session, but retrying every non-Target.* command with self.session moves the operation into the newly attached top-level page. A Runtime.evaluate/DOM call intended for a cross-origin iframe can therefore return or mutate the wrong document silently; if the original target cannot be reattached, surface an error instead of changing the session.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 5 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/helpers.py">
<violation number="1" location="src/browser_harness/helpers.py:70">
P3: Invalid numeric timeout settings make every CDP call fail at `settimeout` (and `0` switches the socket to nonblocking mode). Validate finite positive values; fall back for invalid `BH_CMD_TIMEOUT` and raise a clear error for invalid per-call overrides.</violation>
</file>
<file name="src/browser_harness/daemon.py">
<violation number="1" location="src/browser_harness/daemon.py:309">
P2: After a websocket drop, `_connect_cdp()` unconditionally calls `attach_first_page()` which overwrites `self.target_id` and `self.session` with whichever page `Target.getTargets` returns first. If the agent had previously switched to a specific tab, the reconnect silently moves the session to a potentially different page. Consider saving the previous `target_id` before calling `_connect_cdp()` and attempting to reattach to it (e.g. via `Target.activateTarget` + `Target.attachToTarget`) if it still exists, falling back to `attach_first_page()` only if it's gone.</violation>
<violation number="2" location="src/browser_harness/daemon.py:309">
P2: After a Chrome restart, `page_info()` can report a dialog from the discarded CDP session indefinitely. Clear or revalidate `self.dialog` during rebuild so stale dialog state is not exposed to the new session.</violation>
<violation number="3" location="src/browser_harness/daemon.py:309">
P2: When local Chrome is still restarting, reconnect freezes the daemon event loop for up to 30 seconds, stalling every IPC handler. Resolve `get_ws_url()` off-loop with `asyncio.to_thread` before starting the async client.</violation>
<violation number="4" location="src/browser_harness/daemon.py:419">
P1: Concurrent failed requests can tear down the client a first handler just rebuilt, making either retry fail. Capture the CDP instance before the initial send and pass that saved instance to `_reconnect`.</violation>
<violation number="5" location="src/browser_harness/daemon.py:425">
P1: `retry_sid` unconditionally uses `self.session` for non-Target calls, but when the original request carried a different `session_id` (e.g., an iframe target obtained via `Target.attachToTarget`), this silently redirects the retried command to the top-level page. A `Runtime.evaluate` intended for a cross-origin iframe would execute against (and potentially mutate) the wrong document. If the original `sid` differs from `self.session`, it's safer to surface an error (the iframe session is invalid after reconnect) rather than silently switching context.</violation>
<violation number="6" location="src/browser_harness/daemon.py:427">
P1: A dropped reply can replay a command Chrome already executed, so clicks, navigation, or `Runtime.evaluate` side effects may run twice. Reconnect transport but return a retryable error, or retry only an explicit allowlist of idempotent commands.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| except Exception as re: | ||
| log(f"reconnect failed: {re}") | ||
| return {"error": f"{msg} -- CDP reconnect failed: {re}"} | ||
| retry_sid = None if method.startswith("Target.") else self.session |
There was a problem hiding this comment.
P1: retry_sid unconditionally uses self.session for non-Target calls, but when the original request carried a different session_id (e.g., an iframe target obtained via Target.attachToTarget), this silently redirects the retried command to the top-level page. A Runtime.evaluate intended for a cross-origin iframe would execute against (and potentially mutate) the wrong document. If the original sid differs from self.session, it's safer to surface an error (the iframe session is invalid after reconnect) rather than silently switching context.
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 425:
<comment>`retry_sid` unconditionally uses `self.session` for non-Target calls, but when the original request carried a different `session_id` (e.g., an iframe target obtained via `Target.attachToTarget`), this silently redirects the retried command to the top-level page. A `Runtime.evaluate` intended for a cross-origin iframe would execute against (and potentially mutate) the wrong document. If the original `sid` differs from `self.session`, it's safer to surface an error (the iframe session is invalid after reconnect) rather than silently switching context.</comment>
<file context>
@@ -352,7 +406,27 @@ async def disable_old():
+ except Exception as re:
+ log(f"reconnect failed: {re}")
+ return {"error": f"{msg} -- CDP reconnect failed: {re}"}
+ retry_sid = None if method.startswith("Target.") else self.session
+ try:
+ return {"result": await self.cdp.send_raw(method, params, session_id=retry_sid)}
</file context>
| # prior session id is void, so a non-Target call runs on the | ||
| # freshly attached default session rather than the stale id the | ||
| # caller computed client-side. | ||
| failed = self.cdp |
There was a problem hiding this comment.
P1: Concurrent failed requests can tear down the client a first handler just rebuilt, making either retry fail. Capture the CDP instance before the initial send and pass that saved instance to _reconnect.
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 419:
<comment>Concurrent failed requests can tear down the client a first handler just rebuilt, making either retry fail. Capture the CDP instance before the initial send and pass that saved instance to `_reconnect`.</comment>
<file context>
@@ -352,7 +406,27 @@ async def disable_old():
+ # prior session id is void, so a non-Target call runs on the
+ # freshly attached default session rather than the stale id the
+ # caller computed client-side.
+ failed = self.cdp
+ try:
+ await self._reconnect(failed)
</file context>
| return {"error": f"{msg} -- CDP reconnect failed: {re}"} | ||
| retry_sid = None if method.startswith("Target.") else self.session | ||
| try: | ||
| return {"result": await self.cdp.send_raw(method, params, session_id=retry_sid)} |
There was a problem hiding this comment.
P1: A dropped reply can replay a command Chrome already executed, so clicks, navigation, or Runtime.evaluate side effects may run twice. Reconnect transport but return a retryable error, or retry only an explicit allowlist of idempotent commands.
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 427:
<comment>A dropped reply can replay a command Chrome already executed, so clicks, navigation, or `Runtime.evaluate` side effects may run twice. Reconnect transport but return a retryable error, or retry only an explicit allowlist of idempotent commands.</comment>
<file context>
@@ -352,7 +406,27 @@ async def disable_old():
+ return {"error": f"{msg} -- CDP reconnect failed: {re}"}
+ retry_sid = None if method.startswith("Target.") else self.session
+ try:
+ return {"result": await self.cdp.send_raw(method, params, session_id=retry_sid)}
+ except Exception as e3:
+ return {"error": str(e3)}
</file context>
| return | ||
| log("CDP websocket dropped — reconnecting") | ||
| await _silent(failed_cdp.stop()) | ||
| await self._connect_cdp() |
There was a problem hiding this comment.
P2: After a websocket drop, _connect_cdp() unconditionally calls attach_first_page() which overwrites self.target_id and self.session with whichever page Target.getTargets returns first. If the agent had previously switched to a specific tab, the reconnect silently moves the session to a potentially different page. Consider saving the previous target_id before calling _connect_cdp() and attempting to reattach to it (e.g. via Target.activateTarget + Target.attachToTarget) if it still exists, falling back to attach_first_page() only if it's gone.
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 309:
<comment>After a websocket drop, `_connect_cdp()` unconditionally calls `attach_first_page()` which overwrites `self.target_id` and `self.session` with whichever page `Target.getTargets` returns first. If the agent had previously switched to a specific tab, the reconnect silently moves the session to a potentially different page. Consider saving the previous `target_id` before calling `_connect_cdp()` and attempting to reattach to it (e.g. via `Target.activateTarget` + `Target.attachToTarget`) if it still exists, falling back to `attach_first_page()` only if it's gone.</comment>
<file context>
@@ -245,18 +290,27 @@ async def start(self):
+ return
+ log("CDP websocket dropped — reconnecting")
+ await _silent(failed_cdp.stop())
+ await self._connect_cdp()
+
+ async def start(self):
</file context>
| return | ||
| log("CDP websocket dropped — reconnecting") | ||
| await _silent(failed_cdp.stop()) | ||
| await self._connect_cdp() |
There was a problem hiding this comment.
P2: After a Chrome restart, page_info() can report a dialog from the discarded CDP session indefinitely. Clear or revalidate self.dialog during rebuild so stale dialog state is not exposed to the new session.
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 309:
<comment>After a Chrome restart, `page_info()` can report a dialog from the discarded CDP session indefinitely. Clear or revalidate `self.dialog` during rebuild so stale dialog state is not exposed to the new session.</comment>
<file context>
@@ -245,18 +290,27 @@ async def start(self):
+ return
+ log("CDP websocket dropped — reconnecting")
+ await _silent(failed_cdp.stop())
+ await self._connect_cdp()
+
+ async def start(self):
</file context>
| return | ||
| log("CDP websocket dropped — reconnecting") | ||
| await _silent(failed_cdp.stop()) | ||
| await self._connect_cdp() |
There was a problem hiding this comment.
P2: When local Chrome is still restarting, reconnect freezes the daemon event loop for up to 30 seconds, stalling every IPC handler. Resolve get_ws_url() off-loop with asyncio.to_thread before starting the async client.
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 309:
<comment>When local Chrome is still restarting, reconnect freezes the daemon event loop for up to 30 seconds, stalling every IPC handler. Resolve `get_ws_url()` off-loop with `asyncio.to_thread` before starting the async client.</comment>
<file context>
@@ -245,18 +290,27 @@ async def start(self):
+ return
+ log("CDP websocket dropped — reconnecting")
+ await _silent(failed_cdp.stop())
+ await self._connect_cdp()
+
+ async def start(self):
</file context>
| # settimeout, the reply had to arrive within _CONNECT_TIMEOUT. | ||
| c, token = ipc.connect(NAME, timeout=_CONNECT_TIMEOUT) | ||
| try: | ||
| c.settimeout(_cmd_timeout() if timeout is None else timeout) |
There was a problem hiding this comment.
P3: Invalid numeric timeout settings make every CDP call fail at settimeout (and 0 switches the socket to nonblocking mode). Validate finite positive values; fall back for invalid BH_CMD_TIMEOUT and raise a clear error for invalid per-call overrides.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/helpers.py, line 70:
<comment>Invalid numeric timeout settings make every CDP call fail at `settimeout` (and `0` switches the socket to nonblocking mode). Validate finite positive values; fall back for invalid `BH_CMD_TIMEOUT` and raise a clear error for invalid per-call overrides.</comment>
<file context>
@@ -38,20 +38,49 @@ def _load_env_file(p):
+ # settimeout, the reply had to arrive within _CONNECT_TIMEOUT.
+ c, token = ipc.connect(NAME, timeout=_CONNECT_TIMEOUT)
try:
+ c.settimeout(_cmd_timeout() if timeout is None else timeout)
r = ipc.request(c, token, req)
finally:
</file context>
1 participant
Two failure modes that made the harness feel slow and flaky in daily use.
#1 — Every helper→daemon call was hard-capped at 5s
ipc.connect()sets the connect and read timeout to a single value, and_sendpassed5.0. So any single CDP command that legitimately took longer — screenshots on a large/retina page, heavyDOM.getDocument, awaited promises, slow navigations — raisedsocket.timeoutand killed the script.Fix: split the two budgets. Keep a short 5s connect budget; add a separate command read budget (default 30s, screenshots 60s), tunable via
BH_CMD_TIMEOUTand per-callcdp(_timeout=)/js(timeout=).#2 — Daemon never rebuilt a dropped websocket
The CDP client was built once in
start(). If the websocket dropped (Chrome closed/restarted, remote hiccup), every subsequent call returned an error until a manualbrowser-harness --reload— a zombie daemon that still answeredping.Fix:
_conn_dead()detection + a locked_reconnect()that rebuilds the client and retries the failing call once. For a local Chrome,get_ws_url()re-resolves the live websocket, so it also recovers from a Chrome restart on the same port. Only if the rebuild itself fails (e.g. a remoteBU_CDP_WSwhose endpoint is gone) is the error surfaced.Verification
BH_CMD_TIMEOUT=2fails fast at 2s.Connection lost→reconnecting→attached→ OK).tests/unit/test_reconnect.py(5 tests). Full suite: 112 passed.Notes
BH_CMD_TIMEOUT.send_rawstill has no server-side timeout; the client read budget is the only bound. Out of scope here.🤖 Generated with Claude Code
Summary by cubic
Split the connect vs command timeouts to remove the 5s hard cap, and added automatic reconnect when the CDP websocket drops. Long CDP calls now succeed, and the daemon self-heals after Chrome restarts or transient drops.
BH_CMD_TIMEOUT; override per call withcdp(_timeout=)orjs(timeout=).get_ws_url()re-resolves the live socket; if a remote endpoint is gone, the error is returned.Written for commit 547dc0e. Summary will update on new commits.