From c6c8c74c70e0a5cfd6691d417a4a15ed397b33ae Mon Sep 17 00:00:00 2001 From: olopez25 Date: Mon, 17 Aug 2026 07:35:29 -0400 Subject: [PATCH] Move the keepalive interval to config.yaml and tighten the schedule guard Addresses review feedback on #84928. The tick interval was exposed as HERMES_NOUS_KEEPALIVE_INTERVAL_SECONDS. AGENTS.md reserves .env for credentials and puts behavioural thresholds in config.yaml, so the knob moves to `nous.keepalive_interval_seconds`, following the existing `vertex:` section's precedent for non-secret provider settings. The env var is dropped rather than bridged: it was never released, so nothing depends on it. Adding a key to a new section is handled by the deep-merge, so no _config_version bump is required. test_keepalive_interval_fits_inside_the_token_lifetime asserted `900 < 899 * 4 - 120`, which is true for any realistic interval and could never fail. It also tested the wrong value: the configured constant is only a ceiling, while the schedule that ships is the derived tick. Replaced with an assertion over the derived tick for each observed lifetime, which does fail if the derivation constants regress -- verified against both TICKS_PER_LIFETIME=1 and MIN_INTERVAL_SECONDS=5000. Also adds coverage for an unreadable config.yaml, which must fall back to the module default rather than take the keepalive thread down. pytest tests/hermes_cli/test_nous_auth_keepalive.py -> 9 passed Co-Authored-By: Claude Opus 5 --- hermes_cli/config_defaults.py | 9 ++++ hermes_cli/nous_auth_keepalive.py | 34 ++++++++++--- tests/hermes_cli/test_nous_auth_keepalive.py | 53 +++++++++++++++----- 3 files changed, 76 insertions(+), 20 deletions(-) diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index 6cfc67692f..08ab1789e8 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -4065,6 +4065,15 @@ DEFAULT_CONFIG = { # settings are non-secret routing config and live here. Both are bridged to # the VERTEX_PROJECT_ID / VERTEX_REGION env vars the adapter reads, so an # explicit env var still wins over config.yaml. + "nous": { + # Upper bound on the Nous auth keepalive tick, in seconds. The tick + # actually used derives from the credential lifetime the server issued + # and is capped by this value, so lowering it makes the keepalive more + # frequent while raising it has no effect below the derived tick. + # 0 disables the keepalive thread entirely. + "keepalive_interval_seconds": 900, + }, + "vertex": { # GCP project ID. Empty → use the project_id embedded in the service # account JSON (or ADC-resolved project). diff --git a/hermes_cli/nous_auth_keepalive.py b/hermes_cli/nous_auth_keepalive.py index 3bf28d2a78..f86acc705c 100644 --- a/hermes_cli/nous_auth_keepalive.py +++ b/hermes_cli/nous_auth_keepalive.py @@ -43,7 +43,7 @@ NOUS_AUTH_KEEPALIVE_MIN_INTERVAL_SECONDS = 60 # 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" +NOUS_AUTH_KEEPALIVE_INTERVAL_CONFIG_KEY = "keepalive_interval_seconds" _keepalive_lock = threading.Lock() _keepalive_stop = threading.Event() @@ -59,12 +59,30 @@ def _timeout_seconds(value: Optional[float]) -> float: return 15.0 +def _nous_config() -> dict: + """Return the ``nous:`` section of config.yaml, or {} on any failure. + + Imported lazily: this module is loaded by the gateway and the web server + during startup, and the config loader pulls in a wider dependency graph + than the keepalive itself needs. + """ + try: + from hermes_cli.config import load_config + + section = load_config().get("nous") + return section if isinstance(section, dict) else {} + except Exception: + return {} + + def _interval_seconds(value: Optional[int]) -> int: """Resolve the keepalive tick interval. - Explicit argument wins, then the environment override, then the module - default. A non-positive result disables the keepalive thread entirely, - which is the documented way to turn it off. + Explicit argument wins, then ``nous.keepalive_interval_seconds`` in + config.yaml, then the module default. This is a behavioural threshold + rather than a credential, so it lives in config.yaml and not in .env. + A non-positive result disables the keepalive thread entirely, which is + the documented way to turn it off. """ if value is not None: try: @@ -72,15 +90,15 @@ def _interval_seconds(value: Optional[int]) -> int: except (TypeError, ValueError): return NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS - raw = os.getenv(NOUS_AUTH_KEEPALIVE_INTERVAL_ENV) - if raw is None or not raw.strip(): + raw = _nous_config().get(NOUS_AUTH_KEEPALIVE_INTERVAL_CONFIG_KEY) + if raw is None or (isinstance(raw, str) and not raw.strip()): return NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS try: return int(float(raw)) except (TypeError, ValueError): logger.warning( - "Ignoring invalid %s=%r; using %ds", - NOUS_AUTH_KEEPALIVE_INTERVAL_ENV, + "Ignoring invalid nous.%s=%r; using %ds", + NOUS_AUTH_KEEPALIVE_INTERVAL_CONFIG_KEY, raw, NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS, ) diff --git a/tests/hermes_cli/test_nous_auth_keepalive.py b/tests/hermes_cli/test_nous_auth_keepalive.py index 6006ab41fb..97becdbd45 100644 --- a/tests/hermes_cli/test_nous_auth_keepalive.py +++ b/tests/hermes_cli/test_nous_auth_keepalive.py @@ -5,16 +5,27 @@ from hermes_cli.auth import ACCESS_TOKEN_REFRESH_SKEW_SECONDS OBSERVED_LIFETIMES_SECONDS = (3594, 899) -def test_keepalive_interval_fits_inside_the_token_lifetime(): - """The tick must land before the credential rolls over. +def test_resolved_tick_fits_inside_the_token_lifetime(): + """The tick actually used 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. + + This asserts on the derived tick rather than the configured constant, + because the constant is only a ceiling -- the schedule that ships is + whatever the derivation produces. It therefore fails if the derivation + constants regress (a lower TICKS_PER_LIFETIME or a higher + MIN_INTERVAL_SECONDS both break it), which a bare inequality against the + default interval cannot catch. """ - assert ( - keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS - < min(OBSERVED_LIFETIMES_SECONDS) * 4 - ACCESS_TOKEN_REFRESH_SKEW_SECONDS - ) + for lifetime in OBSERVED_LIFETIMES_SECONDS: + tick = keepalive._tick_seconds( + keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS, lifetime + ) + assert tick < lifetime - ACCESS_TOKEN_REFRESH_SKEW_SECONDS, ( + f"tick={tick}s leaves no room to refresh inside a {lifetime}s " + f"lifetime (skew={ACCESS_TOKEN_REFRESH_SKEW_SECONDS}s)" + ) def test_refresh_always_fires_before_expiry_for_observed_lifetimes(): @@ -98,30 +109,48 @@ def test_observed_lifetime_takes_the_shorter_credential(monkeypatch): def test_interval_precedence_and_disable(monkeypatch): - monkeypatch.delenv(keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_ENV, raising=False) + def _config(section): + monkeypatch.setattr(keepalive, "_nous_config", lambda: section) + + # An absent section leaves the module default in place. + _config({}) assert ( keepalive._interval_seconds(None) == keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS ) - monkeypatch.setenv(keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_ENV, "600") + _config({keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_CONFIG_KEY: 600}) assert keepalive._interval_seconds(None) == 600 - # An explicit argument still outranks the environment. + # An explicit argument still outranks config.yaml. assert keepalive._interval_seconds(300) == 300 - # A malformed override falls back to the default rather than disabling. - monkeypatch.setenv(keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_ENV, "not-a-number") + # A malformed value falls back to the default rather than disabling. + _config({keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_CONFIG_KEY: "not-a-number"}) assert ( keepalive._interval_seconds(None) == keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS ) # Zero remains the documented way to turn the keepalive off. - monkeypatch.setenv(keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_ENV, "0") + _config({keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_CONFIG_KEY: 0}) assert keepalive._interval_seconds(None) == 0 assert keepalive.start_nous_auth_keepalive() is None +def test_interval_survives_an_unreadable_config(monkeypatch): + """A broken config.yaml must not take the keepalive thread down with it.""" + + def _boom(): + raise RuntimeError("config.yaml is unreadable") + + monkeypatch.setattr("hermes_cli.config.load_config", _boom) + assert keepalive._nous_config() == {} + assert ( + keepalive._interval_seconds(None) + == keepalive.NOUS_AUTH_KEEPALIVE_INTERVAL_SECONDS + ) + + def test_keepalive_refreshes_stale_pool_entry(monkeypatch): class _Entry: access_token = "pooled-access-token"