From 792767b344942e52976f9a5599e2e2463374a40c Mon Sep 17 00:00:00 2001 From: ciregenz Date: Wed, 3 Jun 2026 15:13:31 -0700 Subject: [PATCH] [eric] browser: bound each smart-wait probe so a hung tab can't inherit the 30s command timeout --- backend/apps/agents/browser/browser_wait.py | 48 ++++++++++++++++++--- backend/tests/test_browser_wait.py | 47 ++++++++++++++++++++ 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/backend/apps/agents/browser/browser_wait.py b/backend/apps/agents/browser/browser_wait.py index 79cf524b..46f2527e 100644 --- a/backend/apps/agents/browser/browser_wait.py +++ b/backend/apps/agents/browser/browser_wait.py @@ -23,8 +23,11 @@ logic is a pure function we can hammer with tests. import asyncio import json +import logging import time +logger = logging.getLogger(__name__) + # One probe: is the document complete, and how long since the last network # resource finished/started? Returns a JSON string (BrowserEvaluate hands back # string results verbatim). @@ -38,6 +41,14 @@ PROBE_JS = ( _QUIET_WINDOW_MS = 400 # network must be silent this long to count as settled _FLOOR_MS = 250 # never return before this (a momentary gap isn't 'settled') _POLL_MS = 150 +# A healthy probe is tens of ms. A busy-but-fine SPA (heavy main-thread work mid- +# hydration) can occasionally block longer, so a slow probe is NOT proof of death, +# it's just a reason to stop THIS wait early instead of inheriting the 30s command +# timeout. We bound each probe at this, and after a few consecutive non-responses +# we surface hung=True as a SIGNAL (the loop folds it into a cross-command streak +# and only then acts), never as a unilateral abort from a single wait. +_PROBE_TIMEOUT_S = 2.5 +_MAX_PROBE_TIMEOUTS = 3 def decide_stop(ready, quiet_ms, elapsed_ms, @@ -51,14 +62,19 @@ def decide_stop(ready, quiet_ms, elapsed_ms, async def smart_wait(execute_fn, browser_id, tab_id, max_ms, *, poll_ms=_POLL_MS, floor_ms=_FLOOR_MS, - quiet_window_ms=_QUIET_WINDOW_MS) -> dict: + quiet_window_ms=_QUIET_WINDOW_MS, + probe_timeout_s=_PROBE_TIMEOUT_S) -> dict: """Wait up to `max_ms`, returning early once the page settles. `execute_fn` is an async (tool, params, browser_id, tab_id) -> result|None (None = the run - was cancelled). Never raises into the caller.""" + was cancelled). Never raises into the caller. If the page stops responding to + probes (hung tab), returns fast with hung=True so the caller can bail instead + of blocking on the underlying long command timeout.""" max_ms = max(100, min(int(max_ms or 1000), 10000)) start = time.monotonic() settled = False + hung = False last_url = "" + probe_timeouts = 0 def _elapsed(): return (time.monotonic() - start) * 1000 @@ -67,7 +83,25 @@ async def smart_wait(execute_fn, browser_id, tab_id, max_ms, *, await asyncio.sleep(min(poll_ms, max(0, max_ms - _elapsed())) / 1000) if _elapsed() >= max_ms: break - res = await execute_fn("BrowserEvaluate", {"expression": PROBE_JS}, browser_id, tab_id) + # Bound each probe so a wedged tab can't make us inherit the 30s command + # timeout. A timeout is a not-responding signal (not a verdict): count + # consecutive ones and surface hung only after the threshold; any non- + # timeout error is a different problem, treated as 'keep waiting'. + try: + res = await asyncio.wait_for( + execute_fn("BrowserEvaluate", {"expression": PROBE_JS}, browser_id, tab_id), + timeout=probe_timeout_s, + ) + except asyncio.TimeoutError: + probe_timeouts += 1 + if probe_timeouts >= _MAX_PROBE_TIMEOUTS: + hung = True + break + continue + except Exception as e: + logger.debug(f"[smart-wait] probe error (not a timeout): {e}") + continue + probe_timeouts = 0 # a response resets the streak (busy != dead) if res is None: # cancelled mid-wait break last_url = res.get("url") or last_url @@ -83,7 +117,11 @@ async def smart_wait(execute_fn, browser_id, tab_id, max_ms, *, break waited = round(_elapsed()) - text = f"Waited {waited}ms ({'page settled' if settled else 'reached cap'})." + state = "page settled" if settled else ("page not responding" if hung else "reached cap") + text = f"Waited {waited}ms ({state})." + if hung: + text += " The page or tab appears unresponsive." if last_url: text += f" Current URL: {last_url}" - return {"text": text, "url": last_url, "settled": settled, "waited_ms": waited} + return {"text": text, "url": last_url, "settled": settled, "hung": hung, + "waited_ms": waited, **({"error": "page unresponsive"} if hung else {})} diff --git a/backend/tests/test_browser_wait.py b/backend/tests/test_browser_wait.py index 6ccd38d0..1ff26ee4 100644 --- a/backend/tests/test_browser_wait.py +++ b/backend/tests/test_browser_wait.py @@ -7,7 +7,9 @@ cap, (4) keeps waiting through a still-loading SPA, (5) survives a cancel or a mid-navigation probe error. Edge-case-complete on purpose. """ +import asyncio import json +import time import pytest @@ -53,6 +55,19 @@ class FakeExec: return r +class HangingExec: + """Simulates a wedged tab: every probe blocks far longer than the probe + timeout (like the underlying 30s command timeout on a hung page).""" + def __init__(self, block_s=5.0): + self.block_s = block_s + self.calls = 0 + + async def __call__(self, tool, params, bid, tid): + self.calls += 1 + await asyncio.sleep(self.block_s) + return _probe(False, 0) + + @pytest.mark.asyncio async def test_returns_early_once_settled(): # first probe: still loading; second: settled -> should stop well under the cap @@ -105,3 +120,35 @@ async def test_garbage_probe_text_does_not_crash(): ex = FakeExec([{"text": "not json", "url": "u"}, _probe(True, 999)]) out = await bw.smart_wait(ex, "b", "", 3000, poll_ms=15, floor_ms=15, quiet_window_ms=50) assert out["settled"] is True + + +@pytest.mark.asyncio +async def test_hung_tab_returns_fast_not_after_the_full_command_timeout(): + # THE bug from the 20-min loop: a wedged tab made each 'wait' block ~30s. + # Now each probe is bounded, so after a couple of timeouts it returns hung, + # in a few seconds, NOT 30s+, regardless of how long the command would block. + ex = HangingExec(block_s=30.0) # mimic the 30s command timeout + t0 = time.monotonic() + out = await bw.smart_wait(ex, "b", "", 8000, poll_ms=20, probe_timeout_s=0.3) + elapsed = time.monotonic() - t0 + assert out["hung"] is True, "a non-responding tab must be flagged hung" + assert out.get("error") == "page unresponsive" + assert elapsed < 3.0, f"hung wait must return fast, took {elapsed:.1f}s" + # it bailed after the timeout threshold, not after burning the whole cap + assert ex.calls <= bw._MAX_PROBE_TIMEOUTS + + +@pytest.mark.asyncio +async def test_a_single_slow_probe_then_settle_is_not_flagged_hung(): + # one slow probe (under the threshold count) shouldn't trip 'hung'; it recovers + class _OneSlow: + def __init__(self): self.n = 0 + async def __call__(self, *a): + self.n += 1 + if self.n == 1: + await asyncio.sleep(0.5) # one slow poll + return _probe(False, 0) + return _probe(True, 999) + out = await bw.smart_wait(_OneSlow(), "b", "", 5000, poll_ms=10, floor_ms=10, + quiet_window_ms=50, probe_timeout_s=0.2) + assert out["hung"] is False and out["settled"] is True