From 20546ffdfeadcfa2a877b6ddbcbd36964f23ed51 Mon Sep 17 00:00:00 2001 From: jinglun010 Date: Wed, 8 Jul 2026 17:10:00 +0800 Subject: [PATCH] perf(cold-start): mitigate ~14s GIL stall during backend init (#60800) Three fixes for the Desktop/TUI cold-start stall where the event loop is blocked for ~14s between HERMES_BACKEND_READY and the first prompt (#60800): 1. copilot_auth: skip subprocess fallback when any Copilot env var is explicitly set (even if invalid). The user expressed token intent via env var; silently substituting a CLI token is surprising and the subprocess adds up to 5s on Windows. 2. tui_gateway/ws: run resolve_skin() via asyncio.to_thread so config loading + skin engine init do not block the WS read loop during the cold-start RPC burst. 3. web_server: extend _warm_gateway_module to pre-import the heavy module chains (auth, copilot_auth, runtime_provider, skin_engine, inventory, model_switch) that the first WS connection + RPC burst would otherwise import on the loop thread. These trigger .pyc compilation and Defender scans on Windows (15-30s per the existing comment) and were not covered by the original gateway-only warm. Tests: 5 new tests in test_cold_start_gil_stall.py + 2 new tests in test_copilot_auth.py. All 36 copilot_auth tests + 16 ws/web_server tests pass. --- hermes_cli/copilot_auth.py | 15 +- hermes_cli/web_server.py | 38 ++++- tests/hermes_cli/test_copilot_auth.py | 31 ++++ .../tui_gateway/test_cold_start_gil_stall.py | 144 ++++++++++++++++++ tui_gateway/ws.py | 10 +- 5 files changed, 232 insertions(+), 6 deletions(-) create mode 100644 tests/tui_gateway/test_cold_start_gil_stall.py diff --git a/hermes_cli/copilot_auth.py b/hermes_cli/copilot_auth.py index 1216254fea5d9..d101288c02dfd 100644 --- a/hermes_cli/copilot_auth.py +++ b/hermes_cli/copilot_auth.py @@ -79,9 +79,11 @@ def resolve_copilot_token() -> tuple[str, str]: Raises ValueError if only a classic PAT is available. """ # 1. Check env vars in priority order + any_env_var_set = False for env_var in COPILOT_ENV_VARS: val = os.getenv(env_var, "").strip() if val: + any_env_var_set = True valid, msg = validate_copilot_token(val) if not valid: logger.warning( @@ -90,7 +92,18 @@ def resolve_copilot_token() -> tuple[str, str]: continue return val, env_var - # 2. Fall back to gh auth token + # 2. Fall back to gh auth token — but ONLY when no Copilot env var was + # explicitly set. When the user exported GITHUB_TOKEN (even an + # unsupported classic PAT), their intent is to use *that* token, not + # to silently substitute one from the gh CLI credential store. + # Skipping the subprocess here also avoids a slow `gh auth token` + # call (up to 5s timeout on Windows) on every cold start that scans + # Copilot auth state — a measurable contributor to the ~14s + # cold-start stall (#60800). The user can run `copilot login` or + # set a supported token (gho_*/github_pat_*/ghu_) explicitly. + if any_env_var_set: + return "", "" + token = _try_gh_cli_token() if token: valid, msg = validate_copilot_token(token) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 30c86e9e90a54..333995b2d2767 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -151,10 +151,40 @@ def _start_desktop_cron_ticker(stop_event: "threading.Event", interval: int = 60 def _warm_gateway_module() -> None: - try: - import hermes_cli.gateway # noqa: F401 - except Exception: - pass + """Pre-import heavy modules so the event loop is not stalled on first use. + + On a cold Windows install, importing these module chains triggers .pyc + compilation and Defender real-time scans that can stall the event loop + for 15-30s. The original fix (pre-#60800) only warmed + ``hermes_cli.gateway``. But the first WS connection and its initial + RPC burst (``setup.status``, ``setup.runtime_check``, + ``gateway.ready``→``resolve_skin``) pull in several *other* heavy + chains that were still imported on the loop thread, contributing to + the ~14s cold-start stall (#60800). Warm them all here so the cost + is paid in a worker thread while the server socket is already open. + """ + for mod in ( + "hermes_cli.gateway", + # setup.status / setup.runtime_check resolve provider auth state, + # which imports copilot_auth (→ subprocess module) and scans + # credential files. First import is noticeably slow on Windows. + "hermes_cli.auth", + "hermes_cli.copilot_auth", + "hermes_cli.runtime_provider", + # resolve_skin() reads config + initialises the skin engine. + # Even though handle_ws now calls it via asyncio.to_thread + # (see tui_gateway/ws.py), warming it here avoids the first-call + # import cost inside that thread. + "hermes_cli.skin_engine", + # model.options / picker context — parses provider catalogs and + # the models.dev cache on first use. + "hermes_cli.inventory", + "hermes_cli.model_switch", + ): + try: + __import__(mod) + except Exception: + pass def _resolve_restart_drain_timeout() -> float: diff --git a/tests/hermes_cli/test_copilot_auth.py b/tests/hermes_cli/test_copilot_auth.py index b658584f30295..5a44a5a038af9 100644 --- a/tests/hermes_cli/test_copilot_auth.py +++ b/tests/hermes_cli/test_copilot_auth.py @@ -106,6 +106,37 @@ def test_no_token_returns_empty(self, monkeypatch): assert token == "" assert source == "" + def test_invalid_env_var_skips_gh_cli_fallback(self, monkeypatch): + """When an env var is set but holds an unsupported classic PAT, + resolve_copilot_token must NOT fall back to ``gh auth token``. + + The user explicitly exported a token; silently substituting one + from the gh CLI credential store is surprising and the subprocess + call adds up to 5s of latency on Windows cold starts (#60800). + Only fall back to the CLI when NO Copilot env var is set at all. + """ + from hermes_cli.copilot_auth import resolve_copilot_token + monkeypatch.delenv("COPILOT_GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GH_TOKEN", raising=False) + monkeypatch.setenv("GITHUB_TOKEN", "ghp_classic_pat_nope") + with patch("hermes_cli.copilot_auth._try_gh_cli_token") as mock_cli: + token, source = resolve_copilot_token() + assert token == "" + assert source == "" + mock_cli.assert_not_called() + + def test_all_env_vars_invalid_skips_gh_cli_fallback(self, monkeypatch): + """All three env vars set to classic PATs → no gh CLI call.""" + from hermes_cli.copilot_auth import resolve_copilot_token + monkeypatch.setenv("COPILOT_GITHUB_TOKEN", "ghp_one") + monkeypatch.setenv("GH_TOKEN", "ghp_two") + monkeypatch.setenv("GITHUB_TOKEN", "ghp_three") + with patch("hermes_cli.copilot_auth._try_gh_cli_token") as mock_cli: + token, source = resolve_copilot_token() + assert token == "" + assert source == "" + mock_cli.assert_not_called() + class TestRequestHeaders: """Copilot API header generation.""" diff --git a/tests/tui_gateway/test_cold_start_gil_stall.py b/tests/tui_gateway/test_cold_start_gil_stall.py new file mode 100644 index 0000000000000..29601e8d05ef9 --- /dev/null +++ b/tests/tui_gateway/test_cold_start_gil_stall.py @@ -0,0 +1,144 @@ +"""Tests for cold-start GIL stall mitigations (#60800). + +The Desktop/TUI cold start could stall the event loop for ~14s because +synchronous CPU-bound work ran on the loop thread during the window +between ``HERMES_BACKEND_READY`` and the first prompt. Three fixes: + +1. ``copilot_auth.resolve_copilot_token`` skips the ``gh auth token`` + subprocess when a Copilot env var is explicitly set (even if invalid). +2. ``tui_gateway.ws.handle_ws`` runs ``resolve_skin()`` via + ``asyncio.to_thread`` so the loop is not blocked by config/skin init. +3. ``web_server._warm_gateway_module`` pre-imports the heavy module + chains that the first WS connection + RPC burst would otherwise + import on the loop thread. +""" + +import asyncio +import inspect +import sys +from unittest.mock import patch, MagicMock + +import pytest + + +# ─── Fix 1: copilot_auth skips gh CLI when env var is set ────────────── + + +class TestCopilotAuthSkipsGhCli: + """resolve_copilot_token must not call _try_gh_cli_token when any + Copilot env var is set, even if the token is an unsupported classic PAT. + + See test_copilot_auth.py::TestResolveToken for the full env-var-priority + suite; these tests focus on the #60800 cold-start regression — the + gh CLI subprocess adds up to 5s on Windows and should not fire when + the user already expressed token intent via an env var. + """ + + def test_invalid_env_var_skips_gh_cli(self, monkeypatch): + from hermes_cli.copilot_auth import resolve_copilot_token + + monkeypatch.delenv("COPILOT_GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GH_TOKEN", raising=False) + monkeypatch.setenv("GITHUB_TOKEN", "ghp_classic_pat_nope") + with patch("hermes_cli.copilot_auth._try_gh_cli_token") as mock_cli: + token, source = resolve_copilot_token() + assert token == "" + assert source == "" + mock_cli.assert_not_called() + + def test_valid_env_var_skips_gh_cli(self, monkeypatch): + """A valid token in an env var should return immediately — no CLI.""" + from hermes_cli.copilot_auth import resolve_copilot_token + + monkeypatch.setenv("GITHUB_TOKEN", "gho_valid_oauth_token") + with patch("hermes_cli.copilot_auth._try_gh_cli_token") as mock_cli: + token, source = resolve_copilot_token() + assert token == "gho_valid_oauth_token" + assert source == "GITHUB_TOKEN" + mock_cli.assert_not_called() + + def test_no_env_vars_falls_back_to_gh_cli(self, monkeypatch): + """When NO env var is set, the gh CLI fallback must still fire.""" + from hermes_cli.copilot_auth import resolve_copilot_token + + monkeypatch.delenv("COPILOT_GITHUB_TOKEN", raising=False) + monkeypatch.delenv("GH_TOKEN", raising=False) + monkeypatch.delenv("GITHUB_TOKEN", raising=False) + with patch( + "hermes_cli.copilot_auth._try_gh_cli_token", + return_value="gho_from_cli", + ) as mock_cli: + token, source = resolve_copilot_token() + assert token == "gho_from_cli" + assert source == "gh auth token" + mock_cli.assert_called_once() + + +# ─── Fix 2: resolve_skin runs via to_thread in handle_ws ─────────────── + + +def test_handle_ws_uses_to_thread_for_resolve_skin(): + """handle_ws must call resolve_skin through asyncio.to_thread, not + inline on the event loop thread (#60800). + + We verify by inspecting the source of handle_ws — the call to + ``server.resolve_skin`` must be wrapped in ``asyncio.to_thread``. + A regression that reverts to inline ``resolve_skin()`` would fail + this assertion. + """ + import tui_gateway.ws as ws_mod + + source = inspect.getsource(ws_mod.handle_ws) + assert "asyncio.to_thread" in source, ( + "handle_ws must call resolve_skin via asyncio.to_thread to avoid " + "blocking the event loop during cold start (#60800)." + ) + assert "resolve_skin" in source + + +# ─── Fix 3: _warm_gateway_module pre-imports heavy chains ────────────── + + +def test_warm_gateway_module_imports_cold_start_chains(): + """_warm_gateway_module must pre-import the module chains that the + first WS connection + RPC burst would otherwise import on the loop + thread (#60800). Each of these chains involves .pyc compilation, + Defender scans, or heavy transitive imports that stall the loop. + + We verify by patching __import__ to record which modules were + requested, then assert the cold-start-critical modules are present. + """ + import hermes_cli.web_server as web_server_mod + + # The set of modules that MUST be warmed — these are imported on the + # first WS connection / RPC burst and are heavy enough to stall the + # loop on Windows cold starts. + required = { + "hermes_cli.gateway", + "hermes_cli.auth", + "hermes_cli.copilot_auth", + "hermes_cli.runtime_provider", + "hermes_cli.skin_engine", + "hermes_cli.inventory", + "hermes_cli.model_switch", + } + + imported = [] + real_import = __import__ + + def tracking_import(name, *args, **kwargs): + imported.append(name) + # Don't actually import — we only care about what was requested. + # Raise ImportError to let _warm_gateway_module's except pass. + raise ImportError(f"tracking stub for {name}") + + with patch("builtins.__import__", tracking_import): + web_server_mod._warm_gateway_module() + + imported_set = set(imported) + missing = required - imported_set + assert not missing, ( + f"_warm_gateway_module did not pre-import cold-start-critical " + f"modules: {missing}. These must be warmed in a background thread " + f"to avoid stalling the event loop (#60800)." + ) diff --git a/tui_gateway/ws.py b/tui_gateway/ws.py index 2ab4798df1205..6921e355021eb 100644 --- a/tui_gateway/ws.py +++ b/tui_gateway/ws.py @@ -316,13 +316,21 @@ async def handle_ws(ws: Any) -> None: thread_name="tui-ws-mcp-discovery", ) + # resolve_skin() reads config + initializes the skin engine — + # synchronous I/O + CPU work that should not block the event loop + # during the cold-start window. Run it in the thread pool so the + # WS read loop stays free to drain the frontend's initial RPC + # burst (setup.status, session.list, ...) without a stall + # (#60800). The skin payload is small (a dict of strings/arrays), + # so the to_thread overhead is negligible. + skin_payload = await asyncio.to_thread(server.resolve_skin) ready_ok = await transport.write_async( { "jsonrpc": "2.0", "method": "event", "params": { "type": "gateway.ready", - "payload": {"skin": server.resolve_skin()}, + "payload": {"skin": skin_payload}, }, } )