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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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)."
|
||||
)
|
||||
+9
-1
@@ -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},
|
||||
},
|
||||
}
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user