fix(desktop): Models/Providers settings no longer hang 20s when gh CLI is signed out
/api/model/options probed Copilot auth via `gh auth token` four separate times per payload build. When gh has no credential store for the backend's HOME (fresh profile, desktop-spawned backend, CI), each probe blocks its full 5s subprocess timeout on keyring/D-Bus, so every open of the Desktop Models or Providers settings page took 20s — past the renderer's 15s IPC budget, painting 'Error invoking remote method hermes:api: Timed out'. Fix: cache the gh-CLI probe result (hit or miss) for 5 minutes with an invalidation hook, feed gh stdin=DEVNULL, and disable gh interactive prompts/update notifier in the probe env. Measured on the failing profile: 20.5s -> 5.3s cold (one bounded probe), 0.03s warm.
This commit is contained in:
@@ -142,6 +142,26 @@ def _gh_cli_candidates() -> list[str]:
|
||||
return candidates
|
||||
|
||||
|
||||
# ``gh auth token`` result cache. The probe shells out to the gh CLI, and when
|
||||
# gh has no credential store for this HOME (fresh profile, desktop-spawned
|
||||
# backend, CI) it can block for its full 5s subprocess timeout — on keyring /
|
||||
# D-Bus prompts rather than returning immediately. Provider inventory builds
|
||||
# (``/api/model/options``, ``hermes tools``) probe Copilot auth several times
|
||||
# per request, so an uncached miss turns one settings-page load into a 4×5s
|
||||
# stall that exceeds the Desktop renderer's 15s IPC budget and paints an error
|
||||
# (observed Aug 2026: Models/Providers settings pages timing out on every
|
||||
# open). Successes and failures are both cached; a short TTL keeps a freshly
|
||||
# run ``gh auth login`` discoverable without restarting the backend.
|
||||
_GH_CLI_TOKEN_CACHE_TTL_SECONDS = 300.0
|
||||
_gh_cli_token_cache: tuple[float, Optional[str]] | None = None
|
||||
|
||||
|
||||
def _invalidate_gh_cli_token_cache() -> None:
|
||||
"""Reset the ``gh auth token`` probe cache (used by tests and re-auth flows)."""
|
||||
global _gh_cli_token_cache
|
||||
_gh_cli_token_cache = None
|
||||
|
||||
|
||||
def _try_gh_cli_token() -> Optional[str]:
|
||||
"""Return a token from ``gh auth token`` when the GitHub CLI is available.
|
||||
|
||||
@@ -149,12 +169,34 @@ def _try_gh_cli_token() -> Optional[str]:
|
||||
correct host's token. Also strips GITHUB_TOKEN / GH_TOKEN from the
|
||||
subprocess environment so ``gh`` reads from its own credential store
|
||||
(hosts.yml) instead of just echoing the env var back.
|
||||
|
||||
The result (including a miss) is cached for a short TTL — see the cache
|
||||
comment above. Callers that just re-authenticated can call
|
||||
``_invalidate_gh_cli_token_cache()`` to re-probe immediately.
|
||||
"""
|
||||
global _gh_cli_token_cache
|
||||
|
||||
now = time.monotonic()
|
||||
if _gh_cli_token_cache is not None:
|
||||
cached_at, cached_token = _gh_cli_token_cache
|
||||
if now - cached_at < _GH_CLI_TOKEN_CACHE_TTL_SECONDS:
|
||||
return cached_token
|
||||
|
||||
token = _probe_gh_cli_token()
|
||||
_gh_cli_token_cache = (now, token)
|
||||
return token
|
||||
|
||||
|
||||
def _probe_gh_cli_token() -> Optional[str]:
|
||||
"""Uncached ``gh auth token`` subprocess probe (see ``_try_gh_cli_token``)."""
|
||||
hostname = os.getenv("COPILOT_GH_HOST", "").strip()
|
||||
|
||||
# Build a clean env so gh doesn't short-circuit on GITHUB_TOKEN / GH_TOKEN
|
||||
clean_env = {k: v for k, v in os.environ.items()
|
||||
if k not in {"GITHUB_TOKEN", "GH_TOKEN"}}
|
||||
# Never let gh open an interactive prompt from a backend process.
|
||||
clean_env.setdefault("GH_PROMPT_DISABLED", "1")
|
||||
clean_env.setdefault("GH_NO_UPDATE_NOTIFIER", "1")
|
||||
|
||||
_popen_kwargs = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {}
|
||||
for gh_path in _gh_cli_candidates():
|
||||
@@ -168,6 +210,7 @@ def _try_gh_cli_token() -> Optional[str]:
|
||||
text=True, encoding='utf-8', errors='replace',
|
||||
timeout=5,
|
||||
env=clean_env,
|
||||
stdin=subprocess.DEVNULL,
|
||||
**_popen_kwargs,
|
||||
)
|
||||
except (FileNotFoundError, subprocess.TimeoutExpired) as exc:
|
||||
|
||||
@@ -341,6 +341,9 @@ class TestResolveApiKeyProviderCredentials:
|
||||
|
||||
|
||||
def test_try_gh_cli_token_uses_homebrew_path_when_not_on_path(self, monkeypatch):
|
||||
from hermes_cli.copilot_auth import _invalidate_gh_cli_token_cache
|
||||
|
||||
_invalidate_gh_cli_token_cache()
|
||||
monkeypatch.setattr("hermes_cli.copilot_auth.shutil.which", lambda command: None)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.copilot_auth.os.path.isfile",
|
||||
|
||||
@@ -72,6 +72,61 @@ class TestResolveToken:
|
||||
mock_cli.assert_not_called()
|
||||
|
||||
|
||||
class TestGhCliTokenCache:
|
||||
"""The gh-CLI probe result is cached — a miss must not re-spawn gh.
|
||||
|
||||
Regression: /api/model/options ran `gh auth token` four times per build;
|
||||
with no gh credential store each probe blocked its full 5s timeout, so the
|
||||
Desktop Models/Providers settings pages took 20s per open and exceeded the
|
||||
renderer's 15s IPC budget (Aug 2026 desktop audit).
|
||||
"""
|
||||
|
||||
def _reset(self):
|
||||
from hermes_cli.copilot_auth import _invalidate_gh_cli_token_cache
|
||||
_invalidate_gh_cli_token_cache()
|
||||
|
||||
def test_miss_is_cached_and_probe_runs_once(self):
|
||||
from hermes_cli import copilot_auth
|
||||
self._reset()
|
||||
with patch.object(copilot_auth, "_probe_gh_cli_token", return_value=None) as probe:
|
||||
assert copilot_auth._try_gh_cli_token() is None
|
||||
assert copilot_auth._try_gh_cli_token() is None
|
||||
assert copilot_auth._try_gh_cli_token() is None
|
||||
assert probe.call_count == 1
|
||||
self._reset()
|
||||
|
||||
def test_hit_is_cached(self):
|
||||
from hermes_cli import copilot_auth
|
||||
self._reset()
|
||||
with patch.object(copilot_auth, "_probe_gh_cli_token", return_value="gho_cached") as probe:
|
||||
assert copilot_auth._try_gh_cli_token() == "gho_cached"
|
||||
assert copilot_auth._try_gh_cli_token() == "gho_cached"
|
||||
assert probe.call_count == 1
|
||||
self._reset()
|
||||
|
||||
def test_ttl_expiry_reprobes(self, monkeypatch):
|
||||
from hermes_cli import copilot_auth
|
||||
self._reset()
|
||||
clock = {"now": 1000.0}
|
||||
monkeypatch.setattr(copilot_auth.time, "monotonic", lambda: clock["now"])
|
||||
with patch.object(copilot_auth, "_probe_gh_cli_token", return_value=None) as probe:
|
||||
copilot_auth._try_gh_cli_token()
|
||||
clock["now"] += copilot_auth._GH_CLI_TOKEN_CACHE_TTL_SECONDS + 1
|
||||
copilot_auth._try_gh_cli_token()
|
||||
assert probe.call_count == 2
|
||||
self._reset()
|
||||
|
||||
def test_invalidate_forces_reprobe(self):
|
||||
from hermes_cli import copilot_auth
|
||||
self._reset()
|
||||
with patch.object(copilot_auth, "_probe_gh_cli_token", return_value=None) as probe:
|
||||
copilot_auth._try_gh_cli_token()
|
||||
copilot_auth._invalidate_gh_cli_token_cache()
|
||||
copilot_auth._try_gh_cli_token()
|
||||
assert probe.call_count == 2
|
||||
self._reset()
|
||||
|
||||
|
||||
class TestRequestHeaders:
|
||||
"""Copilot API header generation."""
|
||||
|
||||
|
||||
Reference in New Issue
Block a user