Fix/timeout and reconnect - #543
Open
Adekunes wants to merge 4 commits into
Open
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>
test_cloud_bootstrap_on_headless_server failed on any box that exports BU_CDP_URL (a running local Chrome debug endpoint): run.main()'s bootstrap guard reads process env directly, so the ambient value made _explicit_cdp_configured() true and suppressed the start_remote_daemon call the test asserts on. Add an autouse fixture that clears BROWSER_USE_API_KEY, BU_AUTOSPAWN, BU_CDP_URL and BU_CDP_WS before each test, so every test starts from a clean slate and sets only the vars it needs. Also hardens the non-cloud tests, which would otherwise fire the real start_remote_daemon on a box exporting BU_AUTOSPAWN + key. Source logic was correct; only the test's env assumption was stale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tests were being run via `uv run --with pytest pytest`, which layers pytest into a fresh ephemeral overlay venv on every cold uv cache (~2.8s) instead of the persistent project env (~0.5s). Declaring pytest in the default `dev` group lets `uv run pytest ...` resolve it from the synced project env, so repeat runs stay sub-second. uv.lock is gitignored, so only pyproject changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Skill review passedReviewed 1 file(s) — no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 7 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/daemon.py">
<violation number="1" location="src/browser_harness/daemon.py:309">
P2: If Chrome disappears while a JavaScript dialog is open, the reconnected daemon can keep reporting that stale dialog and make `page_info()` skip evaluation on the new page. Dialog state should be reset or revalidated whenever the CDP client is replaced.</violation>
<violation number="2" location="src/browser_harness/daemon.py:425">
P1: After reconnect, an explicitly targeted command can execute in the wrong tab or frame because this line replaces its stale session with the daemon's newly attached default session. Reattach the requested target or fail the retry when the request supplied an explicit session rather than silently changing its execution target.</violation>
<violation number="3" location="src/browser_harness/daemon.py:427">
P1: A dropped websocket does not prove that CDP failed to execute the request, so this retry can apply side-effecting commands twice; for example, typing, clicks, navigation, JavaScript mutations, or tab creation may be duplicated. Reconnect without retrying unknown-outcome commands, or restrict retries to operations with an idempotency guarantee.</violation>
</file>
<file name="src/browser_harness/helpers.py">
<violation number="1" location="src/browser_harness/helpers.py:49">
P2: A Chrome restart can still make the original helper call fail despite the new reconnect path: daemon recovery can take 30 seconds of discovery plus attach/retry work, while `_send` waits only 30 seconds. Reserving additional client budget for reconnect, or coordinating the client deadline with daemon recovery, would let the transparent retry complete.</violation>
<violation number="2" location="src/browser_harness/helpers.py:58">
P2: Invalid timeout values now make browser operations fail before sending the request: `BH_CMD_TIMEOUT=-1`, `nan`, or `inf`, and `cdp(..., _timeout=-1)`, are not rejected or normalized. Validating a finite positive value before `settimeout` would preserve the default for malformed configuration or produce a clear argument error.</violation>
<violation number="3" location="src/browser_harness/helpers.py:70">
P2: Helpers with shorter timeouts can now exceed those timeouts when a daemon response stalls: for example, `wait_for_element(timeout=10)` may block for 30 seconds in its first `js()` call. Forwarding the remaining deadline to each CDP call, or clamping the socket timeout to it, would keep the public wait helpers bounded.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| 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)} |
Contributor
There was a problem hiding this comment.
P1: A dropped websocket does not prove that CDP failed to execute the request, so this retry can apply side-effecting commands twice; for example, typing, clicks, navigation, JavaScript mutations, or tab creation may be duplicated. Reconnect without retrying unknown-outcome commands, or restrict retries to operations with an idempotency guarantee.
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 websocket does not prove that CDP failed to execute the request, so this retry can apply side-effecting commands twice; for example, typing, clicks, navigation, JavaScript mutations, or tab creation may be duplicated. Reconnect without retrying unknown-outcome commands, or restrict retries to operations with an idempotency guarantee.</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>
| 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 |
Contributor
There was a problem hiding this comment.
P1: After reconnect, an explicitly targeted command can execute in the wrong tab or frame because this line replaces its stale session with the daemon's newly attached default session. Reattach the requested target or fail the retry when the request supplied an explicit session rather than silently changing its execution target.
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>After reconnect, an explicitly targeted command can execute in the wrong tab or frame because this line replaces its stale session with the daemon's newly attached default session. Reattach the requested target or fail the retry when the request supplied an explicit session rather than silently changing its execution target.</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>
| return | ||
| log("CDP websocket dropped — reconnecting") | ||
| await _silent(failed_cdp.stop()) | ||
| await self._connect_cdp() |
Contributor
There was a problem hiding this comment.
P2: If Chrome disappears while a JavaScript dialog is open, the reconnected daemon can keep reporting that stale dialog and make page_info() skip evaluation on the new page. Dialog state should be reset or revalidated whenever the CDP client is replaced.
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>If Chrome disappears while a JavaScript dialog is open, the reconnected daemon can keep reporting that stale dialog and make `page_info()` skip evaluation on the new page. Dialog state should be reset or revalidated whenever the CDP client is replaced.</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>
| v = os.environ.get("BH_CMD_TIMEOUT") | ||
| if v: | ||
| try: | ||
| return float(v) |
Contributor
There was a problem hiding this comment.
P2: Invalid timeout values now make browser operations fail before sending the request: BH_CMD_TIMEOUT=-1, nan, or inf, and cdp(..., _timeout=-1), are not rejected or normalized. Validating a finite positive value before settimeout would preserve the default for malformed configuration or produce a clear argument error.
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 58:
<comment>Invalid timeout values now make browser operations fail before sending the request: `BH_CMD_TIMEOUT=-1`, `nan`, or `inf`, and `cdp(..., _timeout=-1)`, are not rejected or normalized. Validating a finite positive value before `settimeout` would preserve the default for malformed configuration or produce a clear argument error.</comment>
<file context>
@@ -38,20 +38,49 @@ def _load_env_file(p):
+ v = os.environ.get("BH_CMD_TIMEOUT")
+ if v:
+ try:
+ return float(v)
+ except ValueError:
+ pass
</file context>
| # both the connect AND read timeout), which hard-capped every operation at 5s | ||
| # and turned any slow-but-valid call into a failure. | ||
| _CONNECT_TIMEOUT = 5.0 | ||
| _DEFAULT_CMD_TIMEOUT = 30.0 |
Contributor
There was a problem hiding this comment.
P2: A Chrome restart can still make the original helper call fail despite the new reconnect path: daemon recovery can take 30 seconds of discovery plus attach/retry work, while _send waits only 30 seconds. Reserving additional client budget for reconnect, or coordinating the client deadline with daemon recovery, would let the transparent retry complete.
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 49:
<comment>A Chrome restart can still make the original helper call fail despite the new reconnect path: daemon recovery can take 30 seconds of discovery plus attach/retry work, while `_send` waits only 30 seconds. Reserving additional client budget for reconnect, or coordinating the client deadline with daemon recovery, would let the transparent retry complete.</comment>
<file context>
@@ -38,20 +38,49 @@ def _load_env_file(p):
+# both the connect AND read timeout), which hard-capped every operation at 5s
+# and turned any slow-but-valid call into a failure.
+_CONNECT_TIMEOUT = 5.0
+_DEFAULT_CMD_TIMEOUT = 30.0
+_SCREENSHOT_CMD_TIMEOUT = 60.0
+
</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) |
Contributor
There was a problem hiding this comment.
P2: Helpers with shorter timeouts can now exceed those timeouts when a daemon response stalls: for example, wait_for_element(timeout=10) may block for 30 seconds in its first js() call. Forwarding the remaining deadline to each CDP call, or clamping the socket timeout to it, would keep the public wait helpers bounded.
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>Helpers with shorter timeouts can now exceed those timeouts when a daemon response stalls: for example, `wait_for_element(timeout=10)` may block for 30 seconds in its first `js()` call. Forwarding the remaining deadline to each CDP call, or clamping the socket timeout to it, would keep the public wait helpers bounded.</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>
_has_return_statement flagged any return token, so a return inside a nested function or arrow callback (e.g. arr.map(x => { return x*2 })) wrongly triggered IIFE wrapping in js(). The wrapper evaluated to undefined and surfaced to the caller as a silent None.
Track paren/brace nesting to decide whether each { opens a function body, and treat a return as top-level only when it sits outside every function/arrow body. Drop the fragile startswith("(") heuristic in js(). Add unit tests covering nested vs top-level returns.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
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:204">
P2: Valid JavaScript regex literals can corrupt the new nesting stacks, causing a return inside a top-level `if` block to be treated as nested and preventing `js()` from wrapping it. For example, `if (/\(/.test(x)) { return 1 }` is classified as having no top-level return, so evaluation fails with a top-level-return syntax error; regex-aware lexing (or a parser) would preserve the intended wrapping.</violation>
<violation number="2" location="src/browser_harness/helpers.py:204">
P2: `js("if /*...*/ (ok) { return value; }")` now leaves a top-level `return` unwrapped and CDP rejects it as an illegal return. Track the preceding code token while lexing (or make the backward scan skip comments) before classifying `(` as a control statement.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| return True | ||
| i += 6; continue | ||
| if ch == "(": | ||
| paren_ctl.append(_word_before(expression, i) in _CONTROL_KEYWORDS) |
Contributor
There was a problem hiding this comment.
P2: Valid JavaScript regex literals can corrupt the new nesting stacks, causing a return inside a top-level if block to be treated as nested and preventing js() from wrapping it. For example, if (/\(/.test(x)) { return 1 } is classified as having no top-level return, so evaluation fails with a top-level-return syntax error; regex-aware lexing (or a parser) would preserve the intended wrapping.
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 204:
<comment>Valid JavaScript regex literals can corrupt the new nesting stacks, causing a return inside a top-level `if` block to be treated as nested and preventing `js()` from wrapping it. For example, `if (/\(/.test(x)) { return 1 }` is classified as having no top-level return, so evaluation fails with a top-level-return syntax error; regex-aware lexing (or a parser) would preserve the intended wrapping.</comment>
<file context>
@@ -165,7 +197,20 @@ def _has_return_statement(expression):
+ return True
+ i += 6; continue
+ if ch == "(":
+ paren_ctl.append(_word_before(expression, i) in _CONTROL_KEYWORDS)
+ elif ch == ")":
+ ctl = paren_ctl.pop() if paren_ctl else False
</file context>
| return True | ||
| i += 6; continue | ||
| if ch == "(": | ||
| paren_ctl.append(_word_before(expression, i) in _CONTROL_KEYWORDS) |
Contributor
There was a problem hiding this comment.
P2: js("if /*...*/ (ok) { return value; }") now leaves a top-level return unwrapped and CDP rejects it as an illegal return. Track the preceding code token while lexing (or make the backward scan skip comments) before classifying ( as a control statement.
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 204:
<comment>`js("if /*...*/ (ok) { return value; }")` now leaves a top-level `return` unwrapped and CDP rejects it as an illegal return. Track the preceding code token while lexing (or make the backward scan skip comments) before classifying `(` as a control statement.</comment>
<file context>
@@ -165,7 +197,20 @@ def _has_return_statement(expression):
+ return True
+ i += 6; continue
+ if ch == "(":
+ paren_ctl.append(_word_before(expression, i) in _CONTROL_KEYWORDS)
+ elif ch == ")":
+ ctl = paren_ctl.pop() if paren_ctl else False
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Split socket connect and per-call command timeouts and added auto-reconnect for dropped CDP websockets, so long-running calls succeed and sessions recover without manual reload. Also fixed js() to only wrap snippets with a top-level return, preventing undefined results from nested callbacks.
Bug Fixes
Dependencies
pytestto thedevdependency group for faster local test runs.Written for commit c40af8d. Summary will update on new commits.