fix(tools): return browser timeout as observation

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,25 +311,40 @@ 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,
         action: BrowserAction,
         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,

tests/tools/browser_use/test_browser_executor.py

@@ -1,21 +1,98 @@
 """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,
     assert_browser_observation_success,
 )
 
 
+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