diff --git a/hermes_cli/nous_auth_keepalive.py b/hermes_cli/nous_auth_keepalive.py index b4adb3a20f..3bf28d2a78 100644 --- a/hermes_cli/nous_auth_keepalive.py +++ b/hermes_cli/nous_auth_keepalive.py @@ -19,14 +19,29 @@ from hermes_cli.auth import ( logger = logging.getLogger(__name__) -# Nous Portal access tokens carry a one-hour lifetime, and the refresh only -# fires once a token is within ACCESS_TOKEN_REFRESH_SKEW_SECONDS (120s) of -# expiry. A tick interval must therefore be comfortably below -# 3600 - 120 = 3480s, or the hour rolls over untouched between ticks and the -# next inference call pays a 401 plus a re-auth round trip. The previous -# 6-hour interval could only ever land inside the 2-minute refresh window by -# coincidence, so in practice every hour expired reactively. +# Two things have to line up for the keepalive to actually keep anything alive. +# +# 1. The tick has to be frequent enough to see the credential before it dies. +# Nous credential lifetimes are not fixed: this varies by account, and +# installs have been observed at both ~3594s and ~899s. The tick therefore +# derives from the lifetime the server actually issued rather than assuming +# an hour, capped by the configured interval and floored so a pathological +# lifetime cannot spin the thread. +# +# 2. The refresh has to fire while the tick can still act on it. The refresh +# only triggers once a credential is within a skew window of expiry, so a +# tick spaced wider than that window steps straight over it. The keepalive +# widens the window to "will this credential outlive my next tick?" instead +# of the request-path default of 120s. Without this, ticking faster only +# narrows the gap; it never closes it. +# +# The original 6-hour tick against a one-hour credential failed both tests, so +# in practice every hour expired reactively into a 401 plus a re-auth retry. NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS = 15 * 60 +NOUS_AUTH_KEEPALIVE_MIN_INTERVAL_SECONDS = 60 +# Ticks per credential lifetime. Four keeps the refresh comfortably ahead of +# expiry without making the thread chatty. +NOUS_AUTH_KEEPALIVE_TICKS_PER_LIFETIME = 4 NOUS_AUTH_KEEPALIVE_INITIAL_DELAY_SECONDS = 60 NOUS_AUTH_KEEPALIVE_INTERVAL_ENV = "HERMES_NOUS_KEEPALIVE_INTERVAL_SECONDS" @@ -72,6 +87,46 @@ def _interval_seconds(value: Optional[int]) -> int: return NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS +def _observed_lifetime_seconds() -> Optional[int]: + """Lifetime the server issued for the current Nous credentials, in seconds. + + Both the access token and the invoke agent key carry their own lifetime and + they are not always equal, so the shorter one governs. Returns None when no + usable value is stored, in which case the caller keeps its configured tick. + """ + state = get_provider_auth_state("nous") or {} + lifetimes = [] + for key in ("expires_in", "agent_key_expires_in"): + try: + value = int(float(state.get(key))) + except (TypeError, ValueError): + continue + if value > 0: + lifetimes.append(value) + return min(lifetimes) if lifetimes else None + + +def _tick_seconds(configured_interval: int, lifetime: Optional[int]) -> int: + """Tick fast enough to refresh several times per credential lifetime.""" + if not lifetime or lifetime <= 0: + return configured_interval + derived = lifetime // NOUS_AUTH_KEEPALIVE_TICKS_PER_LIFETIME + return max( + NOUS_AUTH_KEEPALIVE_MIN_INTERVAL_SECONDS, + min(configured_interval, derived), + ) + + +def _refresh_horizon_seconds(tick_seconds: int, floor_seconds: int) -> int: + """How much life a credential needs to be left alone this tick. + + A credential that will not survive until the next tick has to be refreshed + now, because nothing will look at it again before it expires. Hence + tick + skew rather than the request path's bare skew. + """ + return max(floor_seconds, tick_seconds + ACCESS_TOKEN_REFRESH_SKEW_SECONDS) + + def _entry_state(entry: object) -> dict: return { "agent_key": getattr(entry, "agent_key", None), @@ -83,6 +138,7 @@ def _entry_state(entry: object) -> dict: def _refresh_selected_pool_entry( *, min_key_ttl_seconds: int, + min_access_ttl_seconds: Optional[int] = None, ) -> Optional[bool]: """Refresh the current Nous credential pool entry when it is stale. @@ -109,9 +165,11 @@ def _refresh_selected_pool_entry( if entry is None: return False + if min_access_ttl_seconds is None: + min_access_ttl_seconds = ACCESS_TOKEN_REFRESH_SKEW_SECONDS access_expiring = _is_expiring( getattr(entry, "expires_at", None), - ACCESS_TOKEN_REFRESH_SKEW_SECONDS, + min_access_ttl_seconds, ) key_usable = _agent_key_is_usable(_entry_state(entry), min_key_ttl_seconds) if access_expiring or not key_usable: @@ -127,6 +185,7 @@ def _refresh_selected_pool_entry( def refresh_nous_auth_keepalive_once( *, min_key_ttl_seconds: int = NOUS_INVOKE_JWT_MIN_TTL_SECONDS, + min_access_ttl_seconds: Optional[int] = None, timeout_seconds: Optional[float] = None, ) -> bool: """Refresh Nous auth once if credentials are configured.""" @@ -134,6 +193,7 @@ def refresh_nous_auth_keepalive_once( pool_result = _refresh_selected_pool_entry( min_key_ttl_seconds=min_key_ttl_seconds, + min_access_ttl_seconds=min_access_ttl_seconds, ) if pool_result is not None: return pool_result @@ -171,11 +231,17 @@ def _keepalive_loop( return while not stop_event.is_set(): + # Re-read each pass: the lifetime can change when the account, plan, or + # server-side policy does, and a keepalive that caches it would go stale + # in exactly the case it exists to cover. + tick = _tick_seconds(interval_seconds, _observed_lifetime_seconds()) + horizon = _refresh_horizon_seconds(tick, min_key_ttl_seconds) refresh_nous_auth_keepalive_once( - min_key_ttl_seconds=min_key_ttl_seconds, + min_key_ttl_seconds=horizon, + min_access_ttl_seconds=horizon, timeout_seconds=timeout_seconds, ) - stop_event.wait(interval_seconds) + stop_event.wait(tick) def start_nous_auth_keepalive( diff --git a/tests/hermes_cli/test_nous_auth_keepalive.py b/tests/hermes_cli/test_nous_auth_keepalive.py index 8703e6887a..6006ab41fb 100644 --- a/tests/hermes_cli/test_nous_auth_keepalive.py +++ b/tests/hermes_cli/test_nous_auth_keepalive.py @@ -1,21 +1,102 @@ from hermes_cli import nous_auth_keepalive as keepalive from hermes_cli.auth import ACCESS_TOKEN_REFRESH_SKEW_SECONDS -NOUS_ACCESS_TOKEN_TTL_SECONDS = 3600 +# Both lifetimes have been observed on real installs. +OBSERVED_LIFETIMES_SECONDS = (3594, 899) def test_keepalive_interval_fits_inside_the_token_lifetime(): - """The tick must land before the hour rolls over, or refresh stays reactive. + """The tick must land before the credential rolls over. A tick at or above TTL - skew can miss the refresh window entirely, which is what made every hour expire into a 401 plus a re-auth round trip. """ assert ( keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS - < NOUS_ACCESS_TOKEN_TTL_SECONDS - ACCESS_TOKEN_REFRESH_SKEW_SECONDS + < min(OBSERVED_LIFETIMES_SECONDS) * 4 - ACCESS_TOKEN_REFRESH_SKEW_SECONDS ) +def test_refresh_always_fires_before_expiry_for_observed_lifetimes(): + """Simulate the tick schedule and assert no credential expires unrefreshed. + + This is the property that actually matters: for every lifetime, some tick + must decide to refresh while the credential is still valid. Ticking faster + alone does not guarantee it -- the refresh horizon has to cover the gap + between ticks too. + """ + for lifetime in OBSERVED_LIFETIMES_SECONDS: + tick = keepalive._tick_seconds( + keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS, lifetime + ) + horizon = keepalive._refresh_horizon_seconds( + tick, keepalive.NOUS_INVOKE_JWT_MIN_TTL_SECONDS + ) + + # Walk the ticks and find the first one that refreshes. + refreshed_at = None + elapsed = 0 + while elapsed <= lifetime: + if lifetime - elapsed <= horizon: + refreshed_at = elapsed + break + elapsed += tick + + assert refreshed_at is not None, f"never refreshed for lifetime={lifetime}" + assert refreshed_at < lifetime, ( + f"refresh at {refreshed_at}s came at/after expiry {lifetime}s " + f"(tick={tick}, horizon={horizon})" + ) + + +def test_tick_derives_from_observed_lifetime(): + default = keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS + + # A short-lived credential pulls the tick down below the configured default. + assert keepalive._tick_seconds(default, 899) == 899 // 4 + + # A long-lived one is still capped by the configured interval. + assert keepalive._tick_seconds(default, 86400) == default + + # A missing or nonsensical lifetime leaves the configured tick alone. + assert keepalive._tick_seconds(default, None) == default + assert keepalive._tick_seconds(default, 0) == default + + # A pathological lifetime cannot spin the thread. + assert ( + keepalive._tick_seconds(default, 4) + == keepalive.NOUS_AUTH_KEEPALIVE_MIN_INTERVAL_SECONDS + ) + + +def test_refresh_horizon_covers_the_gap_between_ticks(): + # A credential that will not survive until the next tick refreshes now. + assert keepalive._refresh_horizon_seconds(900, 120) == 900 + ( + ACCESS_TOKEN_REFRESH_SKEW_SECONDS + ) + # The caller's floor still applies when ticks are very frequent. + assert keepalive._refresh_horizon_seconds(10, 5000) == 5000 + + +def test_observed_lifetime_takes_the_shorter_credential(monkeypatch): + monkeypatch.setattr( + keepalive, + "get_provider_auth_state", + lambda provider: {"expires_in": 3594, "agent_key_expires_in": 899}, + ) + assert keepalive._observed_lifetime_seconds() == 899 + + monkeypatch.setattr(keepalive, "get_provider_auth_state", lambda provider: {}) + assert keepalive._observed_lifetime_seconds() is None + + monkeypatch.setattr( + keepalive, + "get_provider_auth_state", + lambda provider: {"expires_in": "nonsense"}, + ) + assert keepalive._observed_lifetime_seconds() is None + + def test_interval_precedence_and_disable(monkeypatch): monkeypatch.delenv(keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_ENV, raising=False) assert (