fix(credential-pool): clear exhaustion state on key rotation (#22622)
* fix(credential-pool): clear exhaustion state on key rotation When a user rotates an API key (e.g. via `hermes setup` after hitting a rate limit), _upsert_entry updates the access_token on the existing pool entry but preserves the stale last_status=exhausted from the old key. On the next session the pool finds the entry, sees it exhausted, and returns no usable credentials — even though the new key is valid. Fix: when access_token changes on an existing entry, reset last_status, last_error_code, last_error_reason, last_error_message, and last_error_reset_at. The exhaustion state belongs to the old key, not the new one. * chore: add pasevin@gmail.com to AUTHOR_MAP * fix: clear last_status_at on key rotation, remove unused pytest import Address review feedback from teknium1 on PR #22622: - Add last_status_at=None to the reset block (matches all other token-sync reset paths in credential_pool.py) - Assert last_status_at is None in the regression test - Remove unused pytest import flagged by ruff + ty
This commit is contained in:
committed by
GitHub
parent
3fa318a50c
commit
fe859a1f55
@@ -2300,6 +2300,11 @@ def _upsert_entry(entries: List[PooledCredential], provider: str, source: str, p
|
||||
field_updates = {}
|
||||
extra_updates = {}
|
||||
_field_names = {f.name for f in fields(existing)}
|
||||
token_changed = (
|
||||
"access_token" in payload
|
||||
and payload["access_token"] is not None
|
||||
and payload["access_token"] != existing.access_token
|
||||
)
|
||||
for key, value in payload.items():
|
||||
if key in {"id", "priority"} or value is None:
|
||||
continue
|
||||
@@ -2311,6 +2316,15 @@ def _upsert_entry(entries: List[PooledCredential], provider: str, source: str, p
|
||||
elif key in _EXTRA_KEYS:
|
||||
if existing.extra.get(key) != value:
|
||||
extra_updates[key] = value
|
||||
# When the credential token itself changes (key rotation), clear any
|
||||
# exhaustion/error state — the old status is stale for the new key.
|
||||
if token_changed and existing.last_status is not None:
|
||||
field_updates["last_status"] = None
|
||||
field_updates["last_status_at"] = None
|
||||
field_updates["last_error_code"] = None
|
||||
field_updates["last_error_reason"] = None
|
||||
field_updates["last_error_message"] = None
|
||||
field_updates["last_error_reset_at"] = None
|
||||
if field_updates or extra_updates:
|
||||
if extra_updates:
|
||||
field_updates["extra"] = {**existing.extra, **extra_updates}
|
||||
|
||||
@@ -1577,6 +1577,7 @@ LEGACY_AUTHOR_MAP = {
|
||||
"17683456+wanazhar@users.noreply.github.com": "wanazhar",
|
||||
"26782336+cixuuz@users.noreply.github.com": "cixuuz",
|
||||
"aleksandr.pasevin@openzeppelin.com": "pasevin",
|
||||
"pasevin@gmail.com": "pasevin",
|
||||
"ubuntu@localhost.localdomain": "holynn-q",
|
||||
"holynn@placeholder.local": "holynn-q",
|
||||
"agent@hermes.local": "jacdevos",
|
||||
|
||||
@@ -0,0 +1,106 @@
|
||||
"""Tests for credential pool upsert — key rotation clears exhaustion state."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
|
||||
|
||||
def _write_auth_store(tmp_path, payload: dict) -> None:
|
||||
hermes_home = tmp_path / "hermes"
|
||||
hermes_home.mkdir(parents=True, exist_ok=True)
|
||||
(hermes_home / "auth.json").write_text(json.dumps(payload, indent=2))
|
||||
|
||||
|
||||
def test_key_rotation_clears_exhausted_status(tmp_path, monkeypatch):
|
||||
"""Replacing an exhausted API key via _upsert_entry resets last_status.
|
||||
|
||||
Regression: `hermes setup` saves a new OPENROUTER_API_KEY to .env, which
|
||||
triggers _seed_from_env → _upsert_entry. If the existing pool entry was
|
||||
marked exhausted (e.g. from a rate-limit on the old key), the stale status
|
||||
was preserved on the new key — making the pool appear unusable even though
|
||||
a fresh valid key was present.
|
||||
"""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes"))
|
||||
_write_auth_store(
|
||||
tmp_path,
|
||||
{
|
||||
"version": 1,
|
||||
"credential_pool": {
|
||||
"openrouter": [
|
||||
{
|
||||
"id": "cred-1",
|
||||
"label": "OPENROUTER_API_KEY",
|
||||
"auth_type": "api_key",
|
||||
"priority": 0,
|
||||
"source": "env:OPENROUTER_API_KEY",
|
||||
"access_token": "old-key",
|
||||
"last_status": "exhausted",
|
||||
"last_status_at": 1000.0,
|
||||
"last_error_code": 429,
|
||||
"last_error_reason": "rate_limit",
|
||||
"last_error_message": "Too many requests",
|
||||
"last_error_reset_at": 2000.0,
|
||||
}
|
||||
]
|
||||
},
|
||||
},
|
||||
)
|
||||
|
||||
# Simulate the user rotating their key (new value in env)
|
||||
monkeypatch.setenv("OPENROUTER_API_KEY", "new-rotated-key")
|
||||
|
||||
from agent.credential_pool import load_pool
|
||||
|
||||
pool = load_pool("openrouter")
|
||||
entry = pool.select()
|
||||
|
||||
assert entry is not None, "Pool should have a usable entry after key rotation"
|
||||
assert entry.access_token == "new-rotated-key"
|
||||
assert entry.last_status is None, "last_status should be cleared after key rotation"
|
||||
assert entry.last_status_at is None
|
||||
assert entry.last_error_code is None
|
||||
assert entry.last_error_reason is None
|
||||
assert entry.last_error_message is None
|
||||
assert entry.last_error_reset_at is None
|
||||
|
||||
|
||||
def test_same_key_preserves_exhausted_status(tmp_path, monkeypatch):
|
||||
"""If the key has NOT changed, _upsert_entry does not clear exhaustion state."""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes"))
|
||||
|
||||
from agent.credential_pool import PooledCredential, _upsert_entry
|
||||
|
||||
existing = PooledCredential.from_dict(
|
||||
"openrouter",
|
||||
{
|
||||
"id": "cred-1",
|
||||
"label": "OPENROUTER_API_KEY",
|
||||
"auth_type": "api_key",
|
||||
"priority": 0,
|
||||
"source": "env:OPENROUTER_API_KEY",
|
||||
"access_token": "same-key",
|
||||
"last_status": "exhausted",
|
||||
"last_status_at": 1000.0,
|
||||
"last_error_code": 429,
|
||||
"last_error_reason": "rate_limit",
|
||||
"last_error_message": "Too many requests",
|
||||
"last_error_reset_at": 2000.0,
|
||||
},
|
||||
)
|
||||
entries = [existing]
|
||||
|
||||
# Upsert with the same token — should NOT clear exhaustion
|
||||
_upsert_entry(
|
||||
entries,
|
||||
"openrouter",
|
||||
"env:OPENROUTER_API_KEY",
|
||||
{
|
||||
"source": "env:OPENROUTER_API_KEY",
|
||||
"auth_type": "api_key",
|
||||
"access_token": "same-key",
|
||||
},
|
||||
)
|
||||
|
||||
assert entries[0].last_status == "exhausted", (
|
||||
"last_status should not be cleared when the key is unchanged"
|
||||
)
|
||||
Reference in New Issue
Block a user