Derive Nous keepalive tick from the issued credential lifetime
Follow-up to the interval fix: ticking faster narrowed the gap but did not close it, and the hardcoded interval was wrong for short-lifetime accounts. Two independent problems, both needed: 1. Lifetime is not a constant. Installs have been observed issuing ~3594s and ~899s (see #35752, which reports expires_in: 899 while this account reports 3594). Any hardcoded interval is right for one and wrong for the other. The tick now derives from the lifetime the server actually issued, capped by the configured interval and floored at 60s. The access token and the invoke agent key carry separate lifetimes, so the shorter one governs. 2. Ticking faster alone never closes the gap. The refresh only fires once a credential is within ACCESS_TOKEN_REFRESH_SKEW_SECONDS (120s) of expiry, so a tick spaced wider than that window steps straight over it. With a 900s tick against a 3594s lifetime the last tick before expiry still saw 894s remaining, declined to refresh, and the credential died 6s before the next one. The keepalive now asks "will this outlive my next tick?" (tick + skew) rather than reusing the request path's bare skew. Measured against both observed lifetimes: lifetime 3594s: was never refreshed proactively; now refreshes 900s early lifetime 899s: was never refreshed proactively; now refreshes 227s early The lifetime is re-read every pass rather than cached, since it can change when the account, plan, or server-side policy does -- exactly the case the keepalive exists to cover. min_access_ttl_seconds is threaded through as an optional parameter, so the request path keeps its existing 120s behaviour and only the keepalive widens the window.
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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 (
|
||||
|
||||
Reference in New Issue
Block a user