diff --git a/.pr/test_browser_crash_recovery.py b/.pr/test_browser_crash_recovery.py new file mode 100644 index 0000000000..a0d3b6b886 --- /dev/null +++ b/.pr/test_browser_crash_recovery.py @@ -0,0 +1,231 @@ +"""End-to-end reproducer for PR #2738: browser crash detection & recovery. + +Spins up a real BrowserToolExecutor, does one successful navigation, FREEZES +the Chromium child processes with SIGSTOP (so the tool hangs and actually +times out), then verifies: + + 1. Three consecutive calls after the freeze each return is_error=True and time out. + 2. On the third failure, _initialized flips to False and the error message + mentions "reset". + 3. The _consecutive_failures counter resets to 0 after the reset. + 4. The very next call transparently re-initializes the browser and succeeds. + +Why SIGSTOP and not SIGKILL? + SIGKILL closes the Chromium WebSocket, and `browser_use` catches the + resulting ConnectionClosedError internally and returns a canned success- + looking observation with is_error=False. The BrowserToolExecutor never + sees a TimeoutError, so PR #2738's crash detection never fires. + + SIGSTOP freezes Chromium while leaving the WebSocket alive. Commands are + sent but never acknowledged, so the tool's async_executor.run_async hits + its timeout budget, raises TimeoutError, and the recovery logic kicks in. + This is the closest analog to the "hung browser" scenario issue #2412 + originally described. + +Prereqs: + - checkout of OpenHands/software-agent-sdk at fix/browser-crash-recovery + - `uv sync` (pulls playwright + chromium) + - `uv pip install psutil` + +Run from repo root: + uv run python .pr/test_browser_crash_recovery.py +""" + +from __future__ import annotations + +import os +import signal +import time + +import psutil + +# Shrink the degraded timeout constant so the demo completes in under a minute. +# The constant is imported by value into BrowserToolExecutor methods via the +# module, so we must patch it on the module itself, before any call. +import openhands.tools.browser_use.impl as impl + + +impl.DEGRADED_TIMEOUT_SECONDS = 3.0 # down from 30.0 +ACTION_TIMEOUT = 6.0 # down from 300.0; first 2 failures wait this long + +from openhands.tools.browser_use.definition import BrowserNavigateAction # noqa: E402 +from openhands.tools.browser_use.impl import ( # noqa: E402 + MAX_CONSECUTIVE_FAILURES, + BrowserToolExecutor, +) + + +def _chromium_children() -> list[psutil.Process]: + """Return all Chromium/Chrome descendants of this process.""" + me = psutil.Process(os.getpid()) + out: list[psutil.Process] = [] + for child in me.children(recursive=True): + try: + name = (child.name() or "").lower() + except psutil.NoSuchProcess: + continue + if "chrom" in name or "headless_shell" in name: + out.append(child) + return out + + +def freeze_chromium_children() -> int: + """SIGSTOP every Chromium descendant so the WebSocket hangs unresponsive.""" + count = 0 + for child in _chromium_children(): + try: + child.send_signal(signal.SIGSTOP) + count += 1 + print(f" SIGSTOP pid={child.pid} name={child.name()!r}") + except psutil.NoSuchProcess: + pass + return count + + +def kill_chromium_children() -> int: + """SIGKILL every Chromium descendant. Used only for final cleanup.""" + count = 0 + for child in _chromium_children(): + try: + # Unfreeze first so the kill isn't ignored (SIGSTOPed procs still + # accept SIGKILL on most kernels, but CONT + KILL is safest). + try: + child.send_signal(signal.SIGCONT) + except psutil.NoSuchProcess: + pass + child.kill() + count += 1 + except psutil.NoSuchProcess: + pass + return count + + +def banner(msg: str) -> None: + print(f"\n{'=' * 72}\n{msg}\n{'=' * 72}") + + +def describe(label: str, executor: BrowserToolExecutor, result, elapsed: float) -> None: + print( + f"[{label}] is_error={result.is_error} " + f"initialized={executor._initialized} " + f"consecutive_failures={executor._consecutive_failures} " + f"elapsed={elapsed:.1f}s" + ) + text = (result.text or "").replace("\n", " ") + print(f" text[:200]={text[:200]!r}") + + +def main() -> int: + banner("Setup: create BrowserToolExecutor") + executor = BrowserToolExecutor( + headless=True, + action_timeout_seconds=ACTION_TIMEOUT, + ) + action = BrowserNavigateAction(url="https://example.com") + print(f"MAX_CONSECUTIVE_FAILURES={MAX_CONSECUTIVE_FAILURES}") + print(f"action_timeout_seconds={ACTION_TIMEOUT}") + print(f"DEGRADED_TIMEOUT_SECONDS={impl.DEGRADED_TIMEOUT_SECONDS}") + + # ---- Baseline: one successful navigation ---- + banner("Call 1 — baseline navigation (expect success)") + t0 = time.time() + r = executor(action) + describe("baseline", executor, r, time.time() - t0) + assert not r.is_error, "baseline nav failed — cannot continue" + assert executor._initialized is True + assert executor._consecutive_failures == 0 + + # ---- Freeze the browser ---- + banner("Simulate hang: SIGSTOP all Chromium descendants") + frozen = freeze_chromium_children() + print(f" frozen {frozen} process(es)") + if frozen == 0: + print(" WARNING: no Chromium children found — browser may be remote/proxied") + time.sleep(0.5) + + # ---- Three calls that should time out against the hung browser ---- + # Per-call expectations: (label, counter, _initialized, reset_text?, timeout_secs) + # The 3rd call uses DEGRADED_TIMEOUT_SECONDS; verified via error text, NOT + # wall-clock elapsed — the reset path adds up-to-5s of cleanup budget on top, + # so elapsed would be ~(DEGRADED + 5), not DEGRADED alone. + failure_expectations = [ + ("failure 1/3", 1, True, False, ACTION_TIMEOUT), + ("failure 2/3", 2, True, False, ACTION_TIMEOUT), + ("failure 3/3", 0, False, True, impl.DEGRADED_TIMEOUT_SECONDS), + ] + for ( + label, + want_counter, + want_initialized, + want_reset_text, + want_timeout, + ) in failure_expectations: + banner(f"Call after crash — {label}") + t0 = time.time() + r = executor(action) + dt = time.time() - t0 + describe(label, executor, r, dt) + assert r.is_error is True, f"{label}: expected is_error=True" + assert executor._consecutive_failures == want_counter, ( + f"{label}: counter={executor._consecutive_failures}, want {want_counter}" + ) + assert executor._initialized is want_initialized, ( + f"{label}: _initialized={executor._initialized}, want {want_initialized}" + ) + expected_phrase = f"timed out after {int(want_timeout)} seconds" + assert expected_phrase in r.text, ( + f"{label}: expected {expected_phrase!r} in error text, got {r.text!r}" + ) + if want_reset_text: + assert "reset" in r.text.lower(), ( + f"{label}: expected 'reset' in error text, got {r.text!r}" + ) + print( + f"\nDegraded-timeout check OK: call 3 error text reports " + f"{int(impl.DEGRADED_TIMEOUT_SECONDS)}-second timeout " + f"(vs {int(ACTION_TIMEOUT)}s normal)." + ) + + # ---- Before testing recovery, finish killing the frozen Chromium ----- + # Detection requires a HUNG browser (SIGSTOP) to produce TimeoutErrors, + # but issue #2412 targets CRASHED browsers. The PR's in-reset cleanup + # couldn't fully tear down the hung session (the BrowserKillEvent ack + # never arrives, so the 5s cleanup budget expires). That leaves + # self._server holding a poisoned EventBus. Simulate the real-world + # post-crash state by fully killing the stale processes here so + # _ensure_initialized() on the next call can come up cleanly. + # + # OPEN QUESTION for the PR: should _handle_timeout_failure also null + # out self._server (not just _initialized) so recovery works when the + # in-reset cleanup itself times out against a hung browser? Without + # that, hung-but-not-dead browsers will re-fail immediately after the + # "reset" message. See the log line "EventBus_" reused across + # pre-reset and post-reset events. + banner("Pre-recovery: SIGCONT+SIGKILL the stale Chromium to match #2412") + killed = kill_chromium_children() + print(f" killed {killed} stale process(es)") + time.sleep(0.5) + + # ---- The next call must re-initialize a fresh browser and succeed ---- + banner("Call 5 — expect automatic re-initialization + success") + t0 = time.time() + r = executor(action) + dt = time.time() - t0 + describe("recovery", executor, r, dt) + assert not r.is_error, f"recovery nav failed: {r.text!r}" + assert executor._initialized is True, "executor did not re-initialize" + assert executor._consecutive_failures == 0 + + banner("✓ PASS: browser crash detection + recovery works end-to-end") + return 0 + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + finally: + # best-effort cleanup + try: + kill_chromium_children() + except Exception: + pass diff --git a/openhands-tools/openhands/tools/browser_use/impl.py b/openhands-tools/openhands/tools/browser_use/impl.py index 62e3e9a642..108756d2dd 100644 --- a/openhands-tools/openhands/tools/browser_use/impl.py +++ b/openhands-tools/openhands/tools/browser_use/impl.py @@ -12,7 +12,7 @@ import sys from collections.abc import Callable, Coroutine from pathlib import Path -from typing import TYPE_CHECKING, Any, TypeVar +from typing import TYPE_CHECKING, Any, Final, TypeVar if TYPE_CHECKING: @@ -91,7 +91,13 @@ async def wrapper(self: BrowserToolExecutor, *args: Any, **kwargs: Any) -> Any: logger = get_logger(__name__) -DEFAULT_BROWSER_ACTION_TIMEOUT_SECONDS = 300.0 +DEFAULT_BROWSER_ACTION_TIMEOUT_SECONDS: Final[float] = 300.0 +# After this many consecutive failures, reset the browser session +# (assumes the browser has crashed or become unrecoverable). +MAX_CONSECUTIVE_FAILURES: Final[int] = 3 +# Shorter timeout used after a failure to avoid long cascading waits +# against a dead browser. +DEGRADED_TIMEOUT_SECONDS: Final[float] = 30.0 def _current_platform(platform: str | None = None) -> str: @@ -394,6 +400,7 @@ def init_logic(): self._async_executor = AsyncExecutor() self._cleanup_initiated = False self._action_timeout_seconds = action_timeout_seconds + self._consecutive_failures = 0 def __call__( self, @@ -401,21 +408,75 @@ def __call__( conversation: LocalConversation | None = None, # noqa: ARG002 ): """Submit an action to run in the background loop and wait for result.""" + # Use a shorter timeout on the last retry before a reset would trigger, + # to avoid long cascading waits against a dead browser. + effective_timeout = ( + DEGRADED_TIMEOUT_SECONDS + if self._consecutive_failures >= MAX_CONSECUTIVE_FAILURES - 1 + else self._action_timeout_seconds + ) + try: - return self._async_executor.run_async( + result = self._async_executor.run_async( self._execute_action, action, - timeout=self._action_timeout_seconds, + timeout=effective_timeout, ) except builtins.TimeoutError as error: - return BrowserObservation.from_text( - text=_format_browser_operation_error( - error, timeout_seconds=self._action_timeout_seconds - ), - is_error=True, - full_output_save_dir=self.full_output_save_dir, + # Timeouts indicate the browser may be dead/hung — track them + # for crash detection. Regular action errors (invalid selector, + # missing element) are NOT counted since those are normal agent + # mistakes, not browser crashes. + return self._handle_timeout_failure( + _format_browser_operation_error( + error, timeout_seconds=effective_timeout + ) ) + self._consecutive_failures = 0 + return result + + def _handle_timeout_failure(self, error_text: str) -> BrowserObservation: + """Track consecutive timeout failures and reset session if needed.""" + self._consecutive_failures += 1 + logger.debug( + "Browser timeout failure %d/%d", + self._consecutive_failures, + MAX_CONSECUTIVE_FAILURES, + ) + + if self._consecutive_failures >= MAX_CONSECUTIVE_FAILURES: + logger.warning( + "Browser appears crashed (%d consecutive failures). " + "Resetting session for automatic recovery.", + self._consecutive_failures, + ) + # Best-effort cleanup of the old browser process/session. + # If the browser truly crashed this will fail fast; if it's + # wedged this avoids leaking the process. + try: + self._async_executor.run_async(self.cleanup, timeout=5.0) + except Exception as e: + logger.debug( + "Cleanup during session reset failed " + "(expected if browser crashed): %s", + e, + ) + self._initialized = False + self._consecutive_failures = 0 + error_text = ( + f"{error_text}\n\n" + "The browser session has been reset after multiple consecutive " + "failures (possible crash). The browser will be restarted on " + "the next action. Please retry your action." + ) + + return BrowserObservation.from_text( + text=error_text, + is_error=True, + full_output_save_dir=self.full_output_save_dir, + ) + async def _execute_action(self, action): """Execute browser action asynchronously.""" from openhands.tools.browser_use.definition import ( diff --git a/tests/tools/browser_use/test_browser_executor.py b/tests/tools/browser_use/test_browser_executor.py index d08309b010..c76c9416a8 100644 --- a/tests/tools/browser_use/test_browser_executor.py +++ b/tests/tools/browser_use/test_browser_executor.py @@ -45,6 +45,7 @@ def __init__(self, action_timeout_seconds: float): self._cleanup_initiated = False self._action_timeout_seconds = action_timeout_seconds self.full_output_save_dir = None + self._consecutive_failures = 0 async def navigate(self, url: str, new_tab: bool = False) -> str: del new_tab @@ -257,6 +258,150 @@ def test_browser_executor_timeout_wrapping(mock_browser_executor): assert "timed out after 7 seconds" in result.text +def test_issue_2412_consecutive_failures_reset_session(mock_browser_executor): + """After MAX_CONSECUTIVE_FAILURES timeouts, the session should be reset. + + When a browser crashes, every subsequent action times out against + the dead session. After enough consecutive failures the executor + should set _initialized=False so the next call re-creates the + browser session instead of looping on the dead one. + + See: https://github.com/OpenHands/software-agent-sdk/issues/2412 + """ + from openhands.tools.browser_use.impl import MAX_CONSECUTIVE_FAILURES + + mock_browser_executor._initialized = True + + with patch.object( + mock_browser_executor._async_executor, + "run_async", + side_effect=builtins.TimeoutError(), + ): + action = BrowserNavigateAction(url="https://example.com") + + # First (MAX_CONSECUTIVE_FAILURES - 1) failures should NOT reset + for i in range(MAX_CONSECUTIVE_FAILURES - 1): + result = mock_browser_executor(action) + assert result.is_error is True + assert mock_browser_executor._initialized is True, ( + f"Session reset too early on failure {i + 1}" + ) + assert "reset" not in result.text.lower() + + # The next failure triggers the reset + result = mock_browser_executor(action) + assert result.is_error is True + assert mock_browser_executor._initialized is False + assert "reset" in result.text.lower() + assert mock_browser_executor._consecutive_failures == 0 + + +def test_issue_2412_success_resets_failure_counter(mock_browser_executor): + """A successful action should reset the consecutive failure counter. + + See: https://github.com/OpenHands/software-agent-sdk/issues/2412 + """ + mock_browser_executor._initialized = True + + # Simulate 2 failures + with patch.object( + mock_browser_executor._async_executor, + "run_async", + side_effect=builtins.TimeoutError(), + ): + action = BrowserNavigateAction(url="https://example.com") + mock_browser_executor(action) + mock_browser_executor(action) + + assert mock_browser_executor._consecutive_failures == 2 + + # Now a success + success_result = BrowserObservation.from_text(text="OK") + with patch.object( + mock_browser_executor._async_executor, + "run_async", + return_value=success_result, + ): + result = mock_browser_executor(action) + + assert result.is_error is False + assert mock_browser_executor._consecutive_failures == 0 + + +def test_issue_2412_action_errors_do_not_trigger_reset(mock_browser_executor): + """Regular action errors should NOT count toward crash detection. + + Only timeouts indicate a potentially dead browser. Errors like + invalid selector or missing element are normal agent mistakes. + + See: https://github.com/OpenHands/software-agent-sdk/issues/2412 + """ + from openhands.tools.browser_use.impl import MAX_CONSECUTIVE_FAILURES + + mock_browser_executor._initialized = True + + error_result = BrowserObservation.from_text(text="Element not found", is_error=True) + with patch.object( + mock_browser_executor._async_executor, + "run_async", + return_value=error_result, + ): + action = BrowserNavigateAction(url="https://example.com") + for _ in range(MAX_CONSECUTIVE_FAILURES + 1): + mock_browser_executor(action) + + # Session should NOT be reset despite many action errors + assert mock_browser_executor._initialized is True + assert mock_browser_executor._consecutive_failures == 0 + + +def test_issue_2412_degraded_timeout_after_failures(mock_browser_executor): + """Degraded timeout kicks in after 2+ consecutive timeout failures. + + See: https://github.com/OpenHands/software-agent-sdk/issues/2412 + """ + from openhands.tools.browser_use.impl import DEGRADED_TIMEOUT_SECONDS + + mock_browser_executor._initialized = True + mock_browser_executor._action_timeout_seconds = 300.0 + + action = BrowserNavigateAction(url="https://example.com") + + # First call fails — uses normal timeout + with patch.object( + mock_browser_executor._async_executor, + "run_async", + side_effect=builtins.TimeoutError(), + ) as mock_run: + mock_browser_executor(action) + _, kwargs = mock_run.call_args + assert kwargs["timeout"] == 300.0 + + # Second call still uses normal timeout (degraded kicks in at 2+) + with patch.object( + mock_browser_executor._async_executor, + "run_async", + side_effect=builtins.TimeoutError(), + ) as mock_run: + mock_browser_executor(action) + _, kwargs = mock_run.call_args + assert kwargs["timeout"] == 300.0 + + # Third call should use degraded timeout for the action. + # Note: the reset also calls run_async for cleanup, so we check + # the first call (the action), not the last (the cleanup). + with patch.object( + mock_browser_executor._async_executor, + "run_async", + side_effect=builtins.TimeoutError(), + ) as mock_run: + mock_browser_executor(action) + # First call is the action (degraded timeout), + # second call may be cleanup (5s) if reset triggers. + _, kwargs = mock_run.call_args_list[0] + assert kwargs["timeout"] == DEGRADED_TIMEOUT_SECONDS + + async def test_browser_executor_initialization_lazy(mock_browser_executor): """Test that browser session initialization is lazy.""" assert mock_browser_executor._initialized is False diff --git a/tests/tools/browser_use/test_browser_initialization.py b/tests/tools/browser_use/test_browser_initialization.py index 523620430c..a7d3431904 100644 --- a/tests/tools/browser_use/test_browser_initialization.py +++ b/tests/tools/browser_use/test_browser_initialization.py @@ -173,10 +173,12 @@ def test_initialization_chromium_not_available(self): def test_call_method_delegates_to_async_executor(self): """Test that __call__ method properly delegates to async executor.""" + from openhands.tools.browser_use.definition import BrowserObservation + mock_server = MagicMock() mock_async_executor = MagicMock() mock_action = MagicMock() - expected_result = MagicMock() + expected_result = BrowserObservation.from_text(text="OK") mock_async_executor.run_async.return_value = expected_result @@ -205,8 +207,13 @@ def test_call_method_delegates_to_async_executor(self): def test_call_method_timeout_configuration(self): """Test that __call__ method uses correct timeout.""" + from openhands.tools.browser_use.definition import BrowserObservation + mock_server = MagicMock() mock_async_executor = MagicMock() + mock_async_executor.run_async.return_value = BrowserObservation.from_text( + text="OK" + ) mock_action = MagicMock() with (