From fe38d50833ed9f68967cf972b43fa4dac47db0e6 Mon Sep 17 00:00:00 2001 From: liuhao1024 Date: Sun, 7 Jun 2026 07:04:33 +0800 Subject: [PATCH] fix(tools): read browser.command_timeout in Camofox HTTP client The Camofox browser backend hardcoded a 30s HTTP timeout via _DEFAULT_TIMEOUT, ignoring the user's browser.command_timeout config. The main browser_tool path already reads this config via _get_command_timeout(). This commit adds an equivalent _get_command_timeout() to browser_camofox.py that reads browser.command_timeout from config with caching, and switches all HTTP helper methods (_post, _get, _get_raw, _delete) to use it as the default timeout. Fixes #40843 --- tests/tools/test_browser_camofox_timeout.py | 69 +++++++++++++++++++++ tools/browser_camofox.py | 50 ++++++++++++--- 2 files changed, 112 insertions(+), 7 deletions(-) create mode 100644 tests/tools/test_browser_camofox_timeout.py diff --git a/tests/tools/test_browser_camofox_timeout.py b/tests/tools/test_browser_camofox_timeout.py new file mode 100644 index 00000000000..c209eba7112 --- /dev/null +++ b/tests/tools/test_browser_camofox_timeout.py @@ -0,0 +1,69 @@ +"""Tests for browser_camofox._get_command_timeout — config-driven timeout.""" +from unittest.mock import MagicMock, patch + +import pytest + + +class TestCamofoxCommandTimeout: + """Verify that the Camofox HTTP backend reads browser.command_timeout.""" + + def test_default_is_30(self): + """When config has no browser.command_timeout, default to 30s.""" + from tools.browser_camofox import _get_command_timeout + + # Clear cache + import tools.browser_camofox as mod + mod._cmd_timeout_resolved = False + mod._cached_cmd_timeout = None + + with patch("tools.browser_camofox.read_raw_config", return_value={}): + assert _get_command_timeout() == 30 + + def test_reads_from_config(self): + """Read browser.command_timeout from config.yaml.""" + from tools.browser_camofox import _get_command_timeout + + import tools.browser_camofox as mod + mod._cmd_timeout_resolved = False + mod._cached_cmd_timeout = None + + cfg = {"browser": {"command_timeout": 90}} + with patch("tools.browser_camofox.read_raw_config", return_value=cfg): + assert _get_command_timeout() == 90 + + def test_floor_at_5s(self): + """Config values below 5 are clamped to 5.""" + from tools.browser_camofox import _get_command_timeout + + import tools.browser_camofox as mod + mod._cmd_timeout_resolved = False + mod._cached_cmd_timeout = None + + cfg = {"browser": {"command_timeout": 1}} + with patch("tools.browser_camofox.read_raw_config", return_value=cfg): + assert _get_command_timeout() == 5 + + def test_cached_after_first_call(self): + """Config is read only once; subsequent calls use cached value.""" + from tools.browser_camofox import _get_command_timeout + + import tools.browser_camofox as mod + mod._cmd_timeout_resolved = False + mod._cached_cmd_timeout = None + + mock_read = MagicMock(return_value={"browser": {"command_timeout": 45}}) + with patch("tools.browser_camofox.read_raw_config", mock_read): + _get_command_timeout() + _get_command_timeout() + mock_read.assert_called_once() + + def test_config_read_error_falls_back(self): + """If config read raises, fall back to 30s.""" + from tools.browser_camofox import _get_command_timeout + + import tools.browser_camofox as mod + mod._cmd_timeout_resolved = False + mod._cached_cmd_timeout = None + + with patch("tools.browser_camofox.read_raw_config", side_effect=Exception("no config")): + assert _get_command_timeout() == 30 diff --git a/tools/browser_camofox.py b/tools/browser_camofox.py index 717e0420d06..e28a86e2e08 100644 --- a/tools/browser_camofox.py +++ b/tools/browser_camofox.py @@ -36,7 +36,7 @@ from urllib.parse import SplitResult, urlsplit, urlunsplit import requests -from hermes_cli.config import cfg_get, load_config +from hermes_cli.config import cfg_get, load_config, read_raw_config from tools.browser_camofox_state import get_camofox_identity from tools.registry import tool_error @@ -46,11 +46,39 @@ logger = logging.getLogger(__name__) # Configuration # --------------------------------------------------------------------------- -_DEFAULT_TIMEOUT = 30 # seconds per HTTP request +_DEFAULT_TIMEOUT = 30 # fallback when config is unreadable _SNAPSHOT_MAX_CHARS = 80_000 # camofox paginates at this limit _vnc_url: Optional[str] = None # cached from /health response _vnc_url_checked = False # only probe once per process +# Cached command timeout from config (resolved lazily, like browser_tool) +_cached_cmd_timeout: Optional[int] = None +_cmd_timeout_resolved = False + + +def _get_command_timeout() -> int: + """Return ``browser.command_timeout`` from config, falling back to 30s. + + Mirrors :func:`tools.browser_tool._get_command_timeout` so both the + local browser path and the Camofox path honour the same config knob. + Result is cached after the first call. + """ + global _cached_cmd_timeout, _cmd_timeout_resolved + if _cmd_timeout_resolved: + return _cached_cmd_timeout # type: ignore[return-value] + + _cmd_timeout_resolved = True + result = _DEFAULT_TIMEOUT + try: + cfg = read_raw_config() + val = cfg_get(cfg, "browser", "command_timeout") + if val is not None: + result = max(int(val), 5) # floor at 5s + except Exception as exc: + logger.debug("Could not read browser.command_timeout: %s", exc) + _cached_cmd_timeout = result + return result + def _auth_headers() -> Dict[str, str]: """Return Authorization header when CAMOFOX_API_KEY is set.""" @@ -356,7 +384,7 @@ def _ensure_tab(task_id: Optional[str], url: str = "about:blank") -> Dict[str, A "listItemId": session["session_key"], "url": url, }, - timeout=_DEFAULT_TIMEOUT, + timeout=_get_command_timeout(), headers=_auth_headers(), ) resp.raise_for_status() @@ -393,32 +421,40 @@ def camofox_soft_cleanup(task_id: Optional[str] = None) -> bool: # HTTP helpers # --------------------------------------------------------------------------- -def _post(path: str, body: dict, timeout: int = _DEFAULT_TIMEOUT) -> dict: +def _post(path: str, body: dict, timeout: Optional[int] = None) -> dict: """POST JSON to camofox and return parsed response.""" + if timeout is None: + timeout = _get_command_timeout() url = f"{get_camofox_url()}{path}" resp = requests.post(url, json=body, timeout=timeout, headers=_auth_headers()) resp.raise_for_status() return resp.json() -def _get(path: str, params: dict = None, timeout: int = _DEFAULT_TIMEOUT) -> dict: +def _get(path: str, params: dict = None, timeout: Optional[int] = None) -> dict: """GET from camofox and return parsed response.""" + if timeout is None: + timeout = _get_command_timeout() url = f"{get_camofox_url()}{path}" resp = requests.get(url, params=params, timeout=timeout, headers=_auth_headers()) resp.raise_for_status() return resp.json() -def _get_raw(path: str, params: dict = None, timeout: int = _DEFAULT_TIMEOUT) -> requests.Response: +def _get_raw(path: str, params: dict = None, timeout: Optional[int] = None) -> requests.Response: """GET from camofox and return raw response (for binary data).""" + if timeout is None: + timeout = _get_command_timeout() url = f"{get_camofox_url()}{path}" resp = requests.get(url, params=params, timeout=timeout, headers=_auth_headers()) resp.raise_for_status() return resp -def _delete(path: str, body: dict = None, timeout: int = _DEFAULT_TIMEOUT) -> dict: +def _delete(path: str, body: dict = None, timeout: Optional[int] = None) -> dict: """DELETE to camofox and return parsed response.""" + if timeout is None: + timeout = _get_command_timeout() url = f"{get_camofox_url()}{path}" resp = requests.delete(url, json=body, timeout=timeout, headers=_auth_headers()) resp.raise_for_status()