diff --git a/hermes_cli/copilot_auth.py b/hermes_cli/copilot_auth.py index 0f1ec8c034..ac6bc78a77 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 d3d3ca2249..1fb3e61316 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -169,10 +169,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 d696806a36..1023bbfa95 100644 --- a/tests/hermes_cli/test_copilot_auth.py +++ b/tests/hermes_cli/test_copilot_auth.py @@ -40,6 +40,37 @@ class TestResolveToken: with pytest.raises(ValueError, match="classic PAT"): resolve_copilot_token() + 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 0000000000..29601e8d05 --- /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 b3b6c4bcc1..073c4ac149 100644 --- a/tui_gateway/ws.py +++ b/tui_gateway/ws.py @@ -303,6 +303,14 @@ async def handle_ws(ws: Any) -> None: transport = WSTransport(ws, asyncio.get_running_loop(), peer=peer) + # 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", @@ -312,7 +320,7 @@ async def handle_ws(ws: Any) -> None: # change_events: this backend broadcasts pet.changed / # cron.changed / sessions.changed, so clients can demote # their legacy polls to slow backstops. - "payload": {"skin": server.resolve_skin(), "change_events": True}, + "payload": {"skin": skin_payload, "change_events": True}, }, } )