diff --git a/hermes_cli/web_routers/git.py b/hermes_cli/web_routers/git.py index a8bbe0ea5d..f24f839909 100644 --- a/hermes_cli/web_routers/git.py +++ b/hermes_cli/web_routers/git.py @@ -15,6 +15,7 @@ from typing import Optional from fastapi import APIRouter, HTTPException from hermes_cli import web_git as _web_git +from hermes_cli._subprocess_compat import bounded_probe_run from hermes_cli.web_deps import late from hermes_cli.web_server_files import _fs_path from hermes_cli.web_models import ( @@ -56,6 +57,7 @@ async def git_status_route(path: str): # only to users who aren't already authenticated. _GH_AUTH_TTL_S = 300.0 _gh_auth_cache: Optional[tuple] = None # (monotonic_ts, payload) +_gh_auth_probe_task: Optional[asyncio.Task] = None def _probe_gh_auth() -> dict: @@ -64,25 +66,33 @@ def _probe_gh_auth() -> dict: return {"available": False, "authenticated": False} try: # Exits 0 when at least one host is logged in; DEVNULL stdin guards against any prompt. - proc = subprocess.run( - [gh, "auth", "status"], - stdin=subprocess.DEVNULL, - capture_output=True, - timeout=10, - ) - return {"available": True, "authenticated": proc.returncode == 0} + proc = bounded_probe_run([gh, "auth", "status"], timeout=10) + return {"available": True, "authenticated": bool(proc and proc.returncode == 0)} except Exception: return {"available": True, "authenticated": False} +def _clear_gh_auth_probe_task(completed_task: asyncio.Task) -> None: + """Release a completed shared probe even if all requesters disconnected.""" + global _gh_auth_probe_task + if _gh_auth_probe_task is completed_task: + _gh_auth_probe_task = None + + @router.get("/api/git/gh-auth") async def gh_auth_status_route(refresh: bool = False): """``{"available", "authenticated"}`` for the `gh` CLI; cached 5 min (``refresh=true`` bypasses so the pill withdraws right after a login).""" - global _gh_auth_cache + global _gh_auth_cache, _gh_auth_probe_task if not refresh and _gh_auth_cache and time.monotonic() - _gh_auth_cache[0] < _GH_AUTH_TTL_S: return _gh_auth_cache[1] - payload = await asyncio.to_thread(_probe_gh_auth) + if _gh_auth_probe_task is None: + _gh_auth_probe_task = asyncio.create_task(asyncio.to_thread(_probe_gh_auth)) + _gh_auth_probe_task.add_done_callback(_clear_gh_auth_probe_task) + probe_task = _gh_auth_probe_task + # Shield the shared probe: disconnecting one requester must not cancel the + # probe that other refreshes/cache misses are awaiting. + payload = await asyncio.shield(probe_task) _gh_auth_cache = (time.monotonic(), payload) return payload diff --git a/tests/hermes_cli/test_web_server_git.py b/tests/hermes_cli/test_web_server_git.py index 752f76e74d..8e4164e523 100644 --- a/tests/hermes_cli/test_web_server_git.py +++ b/tests/hermes_cli/test_web_server_git.py @@ -1,14 +1,88 @@ +import asyncio import subprocess +import threading from pathlib import Path import pytest from hermes_cli import web_server +from hermes_cli.web_routers import git as git_router pytest.importorskip("starlette.testclient") from starlette.testclient import TestClient +@pytest.fixture(autouse=True) +def reset_gh_auth_probe_state(): + previous_cache = git_router._gh_auth_cache + previous_task = git_router._gh_auth_probe_task + git_router._gh_auth_cache = None + git_router._gh_auth_probe_task = None + try: + yield + finally: + git_router._gh_auth_cache = previous_cache + git_router._gh_auth_probe_task = previous_task + + +def test_gh_auth_probe_uses_bounded_process_probe(monkeypatch): + monkeypatch.setattr(git_router.shutil, "which", lambda _: "/usr/bin/gh") + calls = [] + + def bounded(argv, *, timeout): + calls.append((argv, timeout)) + return None # The bounded helper returns None after timeout/tree cleanup. + + monkeypatch.setattr(git_router, "bounded_probe_run", bounded) + + assert git_router._probe_gh_auth() == {"available": True, "authenticated": False} + assert calls == [(["/usr/bin/gh", "auth", "status"], 10)] + + +def test_gh_auth_concurrent_refreshes_share_one_probe(monkeypatch): + started = threading.Event() + release = threading.Event() + calls = 0 + + def probe(): + nonlocal calls + calls += 1 + started.set() + assert release.wait(timeout=1) + return {"available": True, "authenticated": True} + + monkeypatch.setattr(git_router, "_probe_gh_auth", probe) + + async def exercise(): + first = asyncio.create_task(git_router.gh_auth_status_route(refresh=True)) + while not started.is_set(): + await asyncio.sleep(0) + second = asyncio.create_task(git_router.gh_auth_status_route(refresh=True)) + await asyncio.sleep(0) + assert calls == 1 + release.set() + return await asyncio.gather(first, second) + + assert asyncio.run(exercise()) == [ + {"available": True, "authenticated": True}, + {"available": True, "authenticated": True}, + ] + assert calls == 1 + + +def test_gh_auth_returns_fresh_cache_without_probing(monkeypatch): + git_router._gh_auth_cache = (git_router.time.monotonic(), {"available": True, "authenticated": True}) + monkeypatch.setattr(git_router, "_probe_gh_auth", lambda: pytest.fail("cache miss")) + + assert asyncio.run(git_router.gh_auth_status_route()) == {"available": True, "authenticated": True} + + +def test_gh_auth_reports_unavailable_when_gh_is_missing(monkeypatch): + monkeypatch.setattr(git_router.shutil, "which", lambda _: None) + + assert git_router._probe_gh_auth() == {"available": False, "authenticated": False} + + @pytest.fixture def client(): previous = getattr(web_server.app.state, "auth_required", None)