mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-29 18:46:59 +00:00
fix(browser): stop stale cdp_url from stalling every startup by 10+ seconds
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.
This commit is contained in:
parent
c63be0daf7
commit
731aa0ccc9
6 changed files with 82 additions and 29 deletions
|
|
@ -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()
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue