diff --git a/openhands-tools/openhands/tools/browser_use/impl.py b/openhands-tools/openhands/tools/browser_use/impl.py index 0eb7dc3120..4099623f43 100644 --- a/openhands-tools/openhands/tools/browser_use/impl.py +++ b/openhands-tools/openhands/tools/browser_use/impl.py @@ -2,6 +2,7 @@ from __future__ import annotations +import builtins import functools import json import logging @@ -26,7 +27,10 @@ BrowserObservation, ) from openhands.tools.browser_use.server import CustomBrowserUseServer -from openhands.tools.utils.timeout import TimeoutError, run_with_timeout +from openhands.tools.utils.timeout import ( + TimeoutError as ToolTimeoutError, + run_with_timeout, +) F = TypeVar("F", bound=Callable[..., Coroutine[Any, Any, Any]]) @@ -86,6 +90,24 @@ async def wrapper(self: BrowserToolExecutor, *args: Any, **kwargs: Any) -> Any: logger = get_logger(__name__) +DEFAULT_BROWSER_ACTION_TIMEOUT_SECONDS = 300.0 + + +def _format_browser_operation_error( + error: BaseException, timeout_seconds: float | None = None +) -> str: + if error_detail := str(error).strip(): + pass + elif isinstance(error, builtins.TimeoutError): + error_detail = ( + f"Operation timed out after {int(timeout_seconds)} seconds" + if timeout_seconds is not None + else "Operation timed out" + ) + else: + error_detail = error.__class__.__name__ + return f"Browser operation failed: {error_detail}" + def _install_chromium() -> bool: """Attempt to install Chromium via uvx playwright install.""" @@ -139,6 +161,7 @@ class BrowserToolExecutor(ToolExecutor[BrowserAction, BrowserObservation]): _initialized: bool _async_executor: AsyncExecutor _cleanup_initiated: bool + _action_timeout_seconds: float def check_chromium_available(self) -> str | None: """Check if a Chromium/Chrome binary is available. @@ -229,6 +252,7 @@ def __init__( allowed_domains: list[str] | None = None, session_timeout_minutes: int = 30, init_timeout_seconds: int = 30, + action_timeout_seconds: float = DEFAULT_BROWSER_ACTION_TIMEOUT_SECONDS, full_output_save_dir: str | None = None, inject_scripts: list[str] | None = None, **config, @@ -240,6 +264,7 @@ def __init__( allowed_domains: List of allowed domains for browser operations session_timeout_minutes: Browser session timeout in minutes init_timeout_seconds: Timeout for browser initialization in seconds + action_timeout_seconds: Timeout for each browser action in seconds full_output_save_dir: Absolute path to directory to save full output logs and files, used when truncation is needed. inject_scripts: List of JavaScript code strings to inject into every @@ -286,15 +311,19 @@ def init_logic(): try: run_with_timeout(init_logic, init_timeout_seconds) - except TimeoutError: + except ToolTimeoutError: raise Exception( f"Browser tool initialization timed out after {init_timeout_seconds}s" ) + if action_timeout_seconds <= 0: + raise ValueError("action_timeout_seconds must be greater than 0") + self.full_output_save_dir: str | None = full_output_save_dir self._initialized = False self._async_executor = AsyncExecutor() self._cleanup_initiated = False + self._action_timeout_seconds = action_timeout_seconds def __call__( self, @@ -302,9 +331,20 @@ def __call__( conversation: LocalConversation | None = None, # noqa: ARG002 ): """Submit an action to run in the background loop and wait for result.""" - return self._async_executor.run_async( - self._execute_action, action, timeout=300.0 - ) + try: + return self._async_executor.run_async( + self._execute_action, + action, + timeout=self._action_timeout_seconds, + ) + 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, + ) async def _execute_action(self, action): """Execute browser action asynchronously.""" @@ -372,8 +412,8 @@ async def _execute_action(self, action): is_error=False, full_output_save_dir=self.full_output_save_dir, ) - except Exception as e: - error_msg = f"Browser operation failed: {str(e)}" + except Exception as error: + error_msg = _format_browser_operation_error(error) logging.error(error_msg, exc_info=True) return BrowserObservation.from_text( text=error_msg, diff --git a/tests/tools/browser_use/test_browser_executor.py b/tests/tools/browser_use/test_browser_executor.py index 342e350ce9..8c6a57693f 100644 --- a/tests/tools/browser_use/test_browser_executor.py +++ b/tests/tools/browser_use/test_browser_executor.py @@ -1,14 +1,28 @@ """Tests for BrowserToolExecutor integration logic.""" +import asyncio +import builtins +import threading +import time +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from types import SimpleNamespace +from typing import Any, cast from unittest.mock import AsyncMock, patch +from urllib.request import urlopen +import pytest + +from openhands.sdk.utils.async_executor import AsyncExecutor from openhands.tools.browser_use.definition import ( BrowserClickAction, BrowserGetStateAction, BrowserNavigateAction, BrowserObservation, ) -from openhands.tools.browser_use.impl import BrowserToolExecutor +from openhands.tools.browser_use.impl import ( + DEFAULT_BROWSER_ACTION_TIMEOUT_SECONDS, + BrowserToolExecutor, +) from .conftest import ( assert_browser_observation_error, @@ -16,6 +30,69 @@ ) +class _ThreadedSlowServer(ThreadingHTTPServer): + daemon_threads = True + + +class SlowServiceBrowserExecutor(BrowserToolExecutor): + """Minimal browser executor that blocks on a live HTTP request.""" + + def __init__(self, action_timeout_seconds: float): + self._server = cast(Any, SimpleNamespace(_is_recording=False)) + self._config = {} + self._initialized = True + self._async_executor = AsyncExecutor() + self._cleanup_initiated = False + self._action_timeout_seconds = action_timeout_seconds + self.full_output_save_dir = None + + async def navigate(self, url: str, new_tab: bool = False) -> str: + del new_tab + return await asyncio.to_thread(self._fetch_url, url) + + def close(self) -> None: + return + + @staticmethod + def _fetch_url(url: str) -> str: + with urlopen(url, timeout=30) as response: + return response.read().decode() + + +@pytest.fixture +def slow_service(): + """Serve an endpoint that stays pending long enough to trigger a timeout.""" + request_started = threading.Event() + + class SlowHandler(BaseHTTPRequestHandler): + def do_GET(self): # noqa: N802 + request_started.set() + time.sleep(5) + body = b"slow response" + self.send_response(200) + self.send_header("Content-Type", "text/plain; charset=utf-8") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, format, *args): # noqa: A003 + _ = (format, args) + return + + server = _ThreadedSlowServer(("127.0.0.1", 0), SlowHandler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + + try: + host = server.server_address[0] + port = server.server_address[1] + yield f"http://{host}:{port}", request_started + finally: + server.shutdown() + thread.join(timeout=5) + server.server_close() + + def test_browser_executor_initialization(): """Test that BrowserToolExecutor initializes correctly.""" executor = BrowserToolExecutor() @@ -25,6 +102,7 @@ def test_browser_executor_initialization(): assert executor._initialized is False assert executor._server is not None assert executor._async_executor is not None + assert executor._action_timeout_seconds == DEFAULT_BROWSER_ACTION_TIMEOUT_SECONDS def test_browser_executor_config_passing(): @@ -33,12 +111,27 @@ def test_browser_executor_config_passing(): session_timeout_minutes=60, headless=False, allowed_domains=["example.com", "test.com"], + action_timeout_seconds=12.5, custom_param="value", ) assert executor._config["headless"] is False assert executor._config["allowed_domains"] == ["example.com", "test.com"] assert executor._config["custom_param"] == "value" + assert executor._action_timeout_seconds == 12.5 + + +def test_browser_executor_rejects_non_positive_action_timeout(): + """Test that BrowserToolExecutor validates action timeouts.""" + with patch("openhands.tools.browser_use.impl.run_with_timeout"): + with patch.object(BrowserToolExecutor, "_ensure_chromium_available"): + with patch("openhands.tools.browser_use.impl.CustomBrowserUseServer"): + with patch("openhands.tools.browser_use.impl.AsyncExecutor"): + with pytest.raises( + ValueError, + match="action_timeout_seconds must be greater than 0", + ): + BrowserToolExecutor(action_timeout_seconds=0) @patch("openhands.tools.browser_use.impl.BrowserToolExecutor.navigate") @@ -123,6 +216,37 @@ def test_browser_executor_async_execution(mock_browser_executor): mock_execute.assert_called_once_with(action) +def test_browser_executor_timeout_wrapping_live_service(slow_service): + """Test that a live slow service timeout becomes a BrowserObservation.""" + slow_url, request_started = slow_service + executor = SlowServiceBrowserExecutor(action_timeout_seconds=1) + + try: + result = executor(BrowserNavigateAction(url=slow_url)) + finally: + executor.close() + + assert request_started.wait(timeout=1), "The slow service was never queried" + assert_browser_observation_error(result, "Browser operation failed") + assert "timed out after 1 seconds" in result.text + + +def test_browser_executor_timeout_wrapping(mock_browser_executor): + """Test that browser action timeouts return BrowserObservation errors.""" + mock_browser_executor._action_timeout_seconds = 7 + + with patch.object( + mock_browser_executor._async_executor, + "run_async", + side_effect=builtins.TimeoutError(), + ): + action = BrowserNavigateAction(url="https://example.com") + result = mock_browser_executor(action) + + assert_browser_observation_error(result, "Browser operation failed") + assert "timed out after 7 seconds" in result.text + + async def test_browser_executor_initialization_lazy(mock_browser_executor): """Test that browser session initialization is lazy.""" assert mock_browser_executor._initialized is False