From 731aa0ccc9d0968bd61a0c6cee7911aa787c58b2 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sun, 26 Jul 2026 19:53:13 -0700 Subject: [PATCH] fix(browser): stop stale cdp_url from stalling every startup by 10+ seconds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tool-schema assembly at CLI/Desktop startup runs the browser-family check_fns (browser, browser_cdp, browser_dialog, browser_vision). Each of those gates called _get_cdp_override(), which resolves the configured endpoint over HTTP (GET /json/version, timeout=10) — so a *stale* browser.cdp_url pointing at a dead debug browser cost ~7 serial blocking socket connects before the banner rendered. Measured on a real Windows install with a dead http://[::1]:9222 config: 15.1s of an 18s launch, with no warning or error — just mystery slowness. The value is easy to leave behind: /browser connect writes a session-scoped env override, but 'hermes config set browser.cdp_url' persists forever while the debug Chrome it pointed at dies on the next browser restart. Split the helper: - _get_cdp_override_raw() — returns the configured value (env var or config.yaml) with zero network I/O. Used by every is-it-configured gate: check_browser_requirements, _browser_cdp_check, _is_local_mode, _is_local_backend, _navigation_session_key, _should_inject_engine (via _is_local_mode), and the hermes doctor chromium-skip check. - _get_cdp_override() — unchanged contract (raw + /json/version resolution), now only called on paths that are about to connect: session creation and the dialog-supervisor attach. This follows the existing rule in check_browser_requirements ('do not execute agent-browser --version here') and the browser.manage status path, which already banned _get_cdp_override for exactly this reason (test_browser_manage_status_does_not_call_get_cdp_override): schema assembly must not perform blocking I/O. A/B on the same machine, same dead endpoint: get_tool_definitions() 15.08s unpatched -> 1.89s patched, with browser_cdp/browser_dialog still advertised (gate now keys off configuration, not reachability — matching the documented lazy-supervisor contract in _browser_dialog_check). Adds a regression test asserting the browser_cdp check_fn never touches the network. --- hermes_cli/doctor.py | 4 +- tests/tools/test_browser_cdp_tool.py | 23 +++++-- tests/tools/test_browser_hybrid_routing.py | 2 +- tests/tools/test_browser_lightpanda.py | 2 +- tools/browser_cdp_tool.py | 7 ++- tools/browser_tool.py | 73 ++++++++++++++++------ 6 files changed, 82 insertions(+), 29 deletions(-) diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 9fd241630ae0..c5581045ec5f 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -1848,7 +1848,7 @@ def run_doctor(args): _chromium_installed, _is_camofox_mode, _get_cloud_provider, - _get_cdp_override, + _get_cdp_override_raw, _using_lightpanda_engine, ) except Exception: @@ -1861,7 +1861,7 @@ def run_doctor(args): # Lightpanda all bypass the local Chromium requirement. skip_chromium_check = ( _is_camofox_mode() - or bool(_get_cdp_override()) + or bool(_get_cdp_override_raw()) or _get_cloud_provider() is not None or _using_lightpanda_engine() ) diff --git a/tests/tools/test_browser_cdp_tool.py b/tests/tools/test_browser_cdp_tool.py index 0c0b16e9b198..19ef3c24b730 100644 --- a/tests/tools/test_browser_cdp_tool.py +++ b/tests/tools/test_browser_cdp_tool.py @@ -561,21 +561,36 @@ def test_check_fn_false_when_no_cdp_url(monkeypatch): import tools.browser_tool as bt monkeypatch.setattr(bt, "check_browser_requirements", lambda: True) - monkeypatch.setattr(bt, "_get_cdp_override", lambda: "") + monkeypatch.setattr(bt, "_get_cdp_override_raw", lambda: "") assert browser_cdp_tool._browser_cdp_check() is False def test_check_fn_true_when_cdp_url_set(monkeypatch): - """Gate opens as soon as a CDP URL is resolvable.""" + """Gate opens as soon as a CDP URL is configured (no network resolution).""" import tools.browser_tool as bt monkeypatch.setattr(bt, "check_browser_requirements", lambda: True) monkeypatch.setattr( - bt, "_get_cdp_override", lambda: "ws://localhost:9222/devtools/browser/x" + bt, "_get_cdp_override_raw", lambda: "ws://localhost:9222/devtools/browser/x" ) assert browser_cdp_tool._browser_cdp_check() is True +def test_check_fn_does_not_probe_network(monkeypatch): + """The availability gate must never hit the network: a stale/unreachable + configured endpoint used to cost multiple blocking HTTP probes at every + CLI/Desktop startup (tool-schema assembly), stalling launch by 10+ s.""" + import tools.browser_tool as bt + + def _boom(*a, **k): # pragma: no cover — the assertion is that it's unused + raise AssertionError("check_fn must not perform network I/O") + + monkeypatch.setattr(bt, "check_browser_requirements", lambda: True) + monkeypatch.setattr(bt.requests, "get", _boom) + monkeypatch.setenv("BROWSER_CDP_URL", "http://127.0.0.1:9222") + assert browser_cdp_tool._browser_cdp_check() is True + + def test_check_fn_false_when_browser_requirements_fail(monkeypatch): """Even with a CDP URL, gate closes if the overall browser toolset is unavailable (e.g. agent-browser not installed).""" @@ -583,6 +598,6 @@ def test_check_fn_false_when_browser_requirements_fail(monkeypatch): monkeypatch.setattr(bt, "check_browser_requirements", lambda: False) monkeypatch.setattr( - bt, "_get_cdp_override", lambda: "ws://localhost:9222/devtools/browser/x" + bt, "_get_cdp_override_raw", lambda: "ws://localhost:9222/devtools/browser/x" ) assert browser_cdp_tool._browser_cdp_check() is False diff --git a/tests/tools/test_browser_hybrid_routing.py b/tests/tools/test_browser_hybrid_routing.py index 667ec22a7fbb..9b883ffcd491 100644 --- a/tests/tools/test_browser_hybrid_routing.py +++ b/tests/tools/test_browser_hybrid_routing.py @@ -91,7 +91,7 @@ class TestNavigationSessionKey: def test_cdp_override_stays_on_bare_task_id(self, monkeypatch): """A user-supplied CDP endpoint owns the whole session — no hybrid.""" monkeypatch.setattr(browser_tool, "_get_cloud_provider", lambda: Mock()) - monkeypatch.setattr(browser_tool, "_get_cdp_override", lambda: "ws://localhost:9222") + monkeypatch.setattr(browser_tool, "_get_cdp_override_raw", lambda: "ws://localhost:9222") key = browser_tool._navigation_session_key("default", "http://localhost:3000/") assert key == "default" diff --git a/tests/tools/test_browser_lightpanda.py b/tests/tools/test_browser_lightpanda.py index dabfc5d1bd70..d1660b5da8a9 100644 --- a/tests/tools/test_browser_lightpanda.py +++ b/tests/tools/test_browser_lightpanda.py @@ -127,7 +127,7 @@ class TestShouldInjectEngine: def test_no_inject_with_cdp_override(self): from tools.browser_tool import _should_inject_engine with patch("tools.browser_tool._is_camofox_mode", return_value=False), \ - patch("tools.browser_tool._get_cdp_override", return_value="ws://localhost:9222"): + patch("tools.browser_tool._get_cdp_override_raw", return_value="ws://localhost:9222"): assert _should_inject_engine("lightpanda") is False def test_no_inject_with_cloud_provider(self): diff --git a/tools/browser_cdp_tool.py b/tools/browser_cdp_tool.py index 2df9a1660f23..eccd8f8fc1c5 100644 --- a/tools/browser_cdp_tool.py +++ b/tools/browser_cdp_tool.py @@ -653,7 +653,7 @@ def _browser_cdp_check() -> bool: """ try: from tools.browser_tool import ( # type: ignore[import-not-found] - _get_cdp_override, + _get_cdp_override_raw, check_browser_requirements, ) except ImportError as exc: # pragma: no cover — defensive @@ -661,7 +661,10 @@ def _browser_cdp_check() -> bool: return False if not check_browser_requirements(): return False - return bool(_get_cdp_override()) + # Raw (no-I/O) gate: check_fns run during tool-schema assembly at every + # startup; resolving the endpoint over HTTP here would block launch when + # the configured endpoint is stale/unreachable. + return bool(_get_cdp_override_raw()) registry.register( diff --git a/tools/browser_tool.py b/tools/browser_tool.py index c8cf75157008..7a712c875657 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -457,6 +457,45 @@ def _resolve_cdp_override(cdp_url: str) -> str: return raw +def _get_cdp_override_raw() -> str: + """Return the *configured* CDP override without any network I/O. + + Precedence is: + 1. ``BROWSER_CDP_URL`` env var (live override from ``/browser connect``) + 2. ``browser.cdp_url`` in config.yaml (persistent config) + + This is the availability-check variant: callers that only need to know + *whether* a CDP override is configured (tool ``check_fn`` gates, + ``_is_local_mode`` / ``_is_local_backend`` routing decisions, + ``hermes doctor``) MUST use this instead of :func:`_get_cdp_override`. + + Rationale: ``_get_cdp_override`` resolves the endpoint over HTTP + (``/json/version`` discovery, 10s timeout). Tool-schema assembly runs at + every CLI/Desktop startup and probes several browser-family check_fns; + when a *stale* ``browser.cdp_url`` points at a dead endpoint (the debug + Chrome it referenced is long gone), each check blocked on a failing + socket connect and startup stalled for 10+ seconds before the banner — + with no error, just mystery slowness. Same principle as the existing + "do not execute ``agent-browser --version`` here" rule in + ``check_browser_requirements``: no side effects during schema build. + """ + env_override = os.environ.get("BROWSER_CDP_URL", "").strip() + if env_override: + return env_override + + try: + from hermes_cli.config import read_raw_config + + cfg = read_raw_config() + browser_cfg = cfg.get("browser", {}) + if isinstance(browser_cfg, dict): + return str(browser_cfg.get("cdp_url", "") or "").strip() + except Exception as e: + logger.debug("Could not read browser.cdp_url from config: %s", e) + + return "" + + def _get_cdp_override() -> str: """Return a normalized CDP URL override, or empty string. @@ -467,22 +506,16 @@ def _get_cdp_override() -> str: When either is set, we skip both Browserbase and the local headless launcher and connect directly to the supplied Chrome DevTools Protocol endpoint. + + NOTE: resolution may perform an HTTP ``/json/version`` discovery request. + Only call this on paths that are about to *connect* (session creation, + supervisor attach). Pure is-it-configured gates must use + :func:`_get_cdp_override_raw`. """ - env_override = os.environ.get("BROWSER_CDP_URL", "").strip() - if env_override: - return _resolve_cdp_override(env_override) - - try: - from hermes_cli.config import read_raw_config - - cfg = read_raw_config() - browser_cfg = cfg.get("browser", {}) - if isinstance(browser_cfg, dict): - return _resolve_cdp_override(str(browser_cfg.get("cdp_url", "") or "")) - except Exception as e: - logger.debug("Could not read browser.cdp_url from config: %s", e) - - return "" + raw = _get_cdp_override_raw() + if not raw: + return "" + return _resolve_cdp_override(raw) def _get_dialog_policy_config() -> Tuple[str, float]: @@ -789,7 +822,7 @@ def _termux_browser_install_error() -> str: def _is_local_mode() -> bool: """Return True when the browser tool will use a local browser backend.""" - if _get_cdp_override(): + if _get_cdp_override_raw(): return False return _get_cloud_provider() is None @@ -821,7 +854,7 @@ def _is_local_backend() -> bool: # config (both via _get_cdp_override(), and both now suppress camofox in # browser_camofox.py). _is_local_mode() already treats any CDP override as # non-local; keep the two helpers in agreement. - if _get_cdp_override(): + if _get_cdp_override_raw(): return False if _is_camofox_mode(): return True @@ -1313,7 +1346,7 @@ def _navigation_session_key(task_id: str, url: str) -> str: """ if task_id is None: task_id = "default" - if _get_cdp_override(): + if _get_cdp_override_raw(): return task_id if _is_camofox_mode(): return task_id @@ -4740,7 +4773,9 @@ def check_browser_requirements() -> bool: # CDP override mode can connect to an existing remote/local browser endpoint # without requiring the local agent-browser binary on PATH. - if _get_cdp_override(): + # Raw (no-I/O) check: this runs during tool-schema assembly at startup, + # where a stale endpoint must not cost a blocking HTTP probe. + if _get_cdp_override_raw(): return True # The agent-browser CLI is required for local launch and cloud-provider flows.