From 6456c35675dfbfdfc25ec5346e52ed8a8de1c5ef Mon Sep 17 00:00:00 2001 From: ajayr Date: Tue, 28 Jul 2026 00:56:04 +0100 Subject: [PATCH] fix(browserless): accept the `timeout` config key and harden coercion (#4519) `browserless` reads `cfg["timeout_s"]`, while its sibling web providers `crawl4ai` and `jina_ai` read `cfg["timeout"]`. Tool configs allow extra fields, so the unrecognised spelling is dropped without a diagnostic: someone adapting one provider's config snippet for another silently gets the 30s default instead of the timeout they set. (Observed in the other direction, on a deployment whose crawl4ai entry carried `timeout_s`.) Accept both keys, preferring the documented `timeout_s` when both are present. While adding coverage, two pre-existing bugs in the same three lines surfaced, both already guarded in crawl4ai/jina_ai but not here: - `timeout_s: "30s"` (or any non-numeric string) raised ValueError out of `float(raw)` during tool construction rather than falling back. - `timeout_s: off` -- YAML parses that as `False`, and `float(False)` is `0.0`, so every request timed out immediately against a healthy server. `_coerce_timeout` now mirrors the sibling providers: booleans and unparsable strings fall back to the default, with a warning for the string case. Tests: five cases in tests/test_browserless_client.py covering both keys, the precedence order, and both coercion bugs. Verified red before the fix (3 of 5 fail) and green after. Co-authored-by: Claude Opus 5 --- .../deerflow/community/browserless/tools.py | 38 ++++++++++++++++++- backend/tests/test_browserless_client.py | 30 +++++++++++++++ config.example.yaml | 2 +- 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/backend/packages/harness/deerflow/community/browserless/tools.py b/backend/packages/harness/deerflow/community/browserless/tools.py index f5829c84b..7fd981c94 100644 --- a/backend/packages/harness/deerflow/community/browserless/tools.py +++ b/backend/packages/harness/deerflow/community/browserless/tools.py @@ -44,6 +44,41 @@ def _get_tool_config(tool_name: str) -> dict | None: return extras if extras is not None else {} +def _coerce_timeout(value: object, default: float) -> float: + """Coerce a config timeout into seconds, falling back to ``default`` on bad input. + + Mirrors ``crawl4ai._coerce_timeout`` / ``jina_ai._coerce_timeout``: booleans and + non-numeric strings fall back to the default, so ``timeout_s: off`` (YAML ``False``) + does not become ``0.0`` and time out every request against a healthy server, and a + typo'd value does not raise out of tool construction. + """ + if isinstance(value, bool): + return default + if isinstance(value, (int, float)): + return float(value) + if isinstance(value, str): + try: + return float(value) + except ValueError: + logger.warning("Browserless: invalid timeout %r in config; using %ss", value, default) + return default + + +def _resolve_timeout(cfg: dict, default: float) -> float: + """Read the timeout, accepting this provider's key and the sibling providers' key. + + ``browserless`` documents ``timeout_s`` while ``crawl4ai`` and ``jina_ai`` read + ``timeout``. An unrecognised key is dropped silently by the extra-fields tool config, + so someone adapting another provider's snippet got the default with no diagnostic. + Accept both, preferring the documented ``timeout_s`` when both are present. + """ + if "timeout_s" in cfg: + return _coerce_timeout(cfg["timeout_s"], default) + if "timeout" in cfg: + return _coerce_timeout(cfg["timeout"], default) + return default + + def _get_browserless_client(tool_name: str = "web_fetch") -> BrowserlessClient: cfg = _get_tool_config(tool_name) base_url = "http://localhost:3032" @@ -52,8 +87,7 @@ def _get_browserless_client(tool_name: str = "web_fetch") -> BrowserlessClient: if cfg is not None: base_url = cfg.get("base_url", base_url) token = cfg.get("token", token) - raw = cfg.get("timeout_s", timeout_s) - timeout_s = float(raw) if not isinstance(raw, float) else raw + timeout_s = _resolve_timeout(cfg, timeout_s) return BrowserlessClient(base_url=base_url, token=token, timeout_s=timeout_s) diff --git a/backend/tests/test_browserless_client.py b/backend/tests/test_browserless_client.py index fe6ebcdfd..a2f80b8ea 100644 --- a/backend/tests/test_browserless_client.py +++ b/backend/tests/test_browserless_client.py @@ -319,6 +319,36 @@ class TestBrowserlessTools: assert client.token == "env-token" + def _client_with_config(self, cfg: dict): + with patch("deerflow.community.browserless.tools._get_tool_config") as mock_cfg: + mock_cfg.return_value = cfg + with patch.dict("os.environ", {}, clear=True): + return tools._get_browserless_client("web_capture") + + async def test_timeout_s_config_key_is_honored(self): + """The documented `timeout_s` key keeps working (back-compat).""" + assert self._client_with_config({"timeout_s": 45}).timeout_s == 45.0 + + async def test_timeout_config_key_is_honored(self): + """`timeout` is accepted too: it is what crawl4ai and jina_ai read. + + Copying that spelling across providers previously produced a silently + ignored key and the 30s default. + """ + assert self._client_with_config({"timeout": 45}).timeout_s == 45.0 + + async def test_timeout_s_wins_when_both_keys_are_present(self): + """`timeout_s` is this provider's documented key, so it takes precedence.""" + assert self._client_with_config({"timeout_s": 45, "timeout": 10}).timeout_s == 45.0 + + async def test_invalid_timeout_falls_back_to_default(self): + """A non-numeric timeout must not crash tool construction.""" + assert self._client_with_config({"timeout_s": "not-a-number"}).timeout_s == 30.0 + + async def test_boolean_timeout_falls_back_to_default(self): + """YAML `timeout_s: off` parses as False; 0.0 would time out every request.""" + assert self._client_with_config({"timeout_s": False}).timeout_s == 30.0 + @patch("deerflow.community.browserless.tools._get_browserless_client") async def test_web_fetch_tool_success(self, mock_get_client): """web_fetch_tool successfully fetches and extracts content.""" diff --git a/config.example.yaml b/config.example.yaml index a7ab5a555..95d3a1f40 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -796,7 +796,7 @@ tools: # use: deerflow.community.browserless.tools:web_capture_tool # base_url: http://localhost:3032 # Browserless instance URL (Docker: http://browserless:3000) # # token: $BROWSERLESS_TOKEN # Required for Browserless Cloud; optional for self-hosted - # timeout_s: 30 # Request timeout in seconds + # timeout_s: 30 # Request timeout in seconds (`timeout` also accepted) # output_format: png # png, jpeg, or webp # full_page: true # Capture entire page instead of viewport only # viewport_width: 1280