From ee77018cb3a413806da4e635d4bba65aef95d820 Mon Sep 17 00:00:00 2001 From: Rory Ford Date: Sun, 12 Jul 2026 19:26:21 +1000 Subject: [PATCH] fix(agent): keep credential-pool entries when terminal-OAuth quarantine save fails xAI, Codex, and Nous terminal-OAuth-refresh paths evicted the in-memory pool entry and persisted even when the auth-store clear/save raised, leaving the pool diverged from on-disk auth.json. Gate the eviction and persist behind a `cleared` flag that is only set once the auth-store write path completes without error, and raise the failure log level from debug to warning so it's visible. Co-Authored-By: Claude Opus 4.8 (1M context) --- agent/credential_pool.py | 91 ++++++++-------- tests/agent/test_credential_pool.py | 157 ++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 41 deletions(-) diff --git a/agent/credential_pool.py b/agent/credential_pool.py index 9d5d81b2386f9..0497b55f373a2 100644 --- a/agent/credential_pool.py +++ b/agent/credential_pool.py @@ -1142,6 +1142,7 @@ def _refresh_entry_impl( logger.debug( "xAI OAuth refresh token is terminally invalid; clearing local token state" ) + cleared = False try: with _auth_store_lock(): auth_store = _load_auth_store() @@ -1165,21 +1166,23 @@ def _refresh_entry_impl( } _save_provider_state(auth_store, "xai-oauth", state) _save_auth_store(auth_store) + cleared = True except Exception as clear_exc: - logger.debug( + logger.warning( "Failed to clear terminal xAI OAuth state: %s", clear_exc ) - removed_ids = [ - item.id for item in self._entries - if item.source == "device_code" - ] - self._entries = [ - item for item in self._entries - if item.source != "device_code" - ] - if self._current_id == entry.id: - self._current_id = None - self._persist(removed_ids=removed_ids) + if cleared: + removed_ids = [ + item.id for item in self._entries + if item.source == "device_code" + ] + self._entries = [ + item for item in self._entries + if item.source != "device_code" + ] + if self._current_id == entry.id: + self._current_id = None + self._persist(removed_ids=removed_ids) return None # For openai-codex: same race as xAI/nous — another Hermes process # may have consumed the refresh token between our proactive sync @@ -1212,6 +1215,7 @@ def _refresh_entry_impl( logger.debug( "Codex OAuth refresh token is terminally invalid; clearing local token state" ) + cleared = False try: with _auth_store_lock(): auth_store = _load_auth_store() @@ -1235,21 +1239,23 @@ def _refresh_entry_impl( } _save_provider_state(auth_store, "openai-codex", state) _save_auth_store(auth_store) + cleared = True except Exception as clear_exc: - logger.debug( + logger.warning( "Failed to clear terminal Codex OAuth state: %s", clear_exc ) - removed_ids = [ - item.id for item in self._entries - if item.source == "device_code" - ] - self._entries = [ - item for item in self._entries - if item.source != "device_code" - ] - if self._current_id == entry.id: - self._current_id = None - self._persist(removed_ids=removed_ids) + if cleared: + removed_ids = [ + item.id for item in self._entries + if item.source == "device_code" + ] + self._entries = [ + item for item in self._entries + if item.source != "device_code" + ] + if self._current_id == entry.id: + self._current_id = None + self._persist(removed_ids=removed_ids) return None # For nous: another process may have consumed the refresh token # between our proactive sync and the HTTP call. Re-sync from @@ -1273,6 +1279,7 @@ def _refresh_entry_impl( return updated if auth_mod._is_terminal_nous_refresh_error(exc): logger.debug("Nous refresh token is terminally invalid; clearing local token state") + cleared = False try: with _auth_store_lock(): auth_store = _load_auth_store() @@ -1299,24 +1306,26 @@ def _refresh_entry_impl( ) _save_provider_state(auth_store, "nous", state) _save_auth_store(auth_store) + cleared = True except Exception as clear_exc: - logger.debug("Failed to clear terminal Nous OAuth state: %s", clear_exc) - - singleton_sources = { - auth_mod.NOUS_DEVICE_CODE_SOURCE, - f"manual:{auth_mod.NOUS_DEVICE_CODE_SOURCE}", - } - removed_ids = [ - item.id for item in self._entries - if item.source in singleton_sources - ] - self._entries = [ - item for item in self._entries - if item.source not in singleton_sources - ] - if self._current_id == entry.id: - self._current_id = None - self._persist(removed_ids=removed_ids) + logger.warning("Failed to clear terminal Nous OAuth state: %s", clear_exc) + + if cleared: + singleton_sources = { + auth_mod.NOUS_DEVICE_CODE_SOURCE, + f"manual:{auth_mod.NOUS_DEVICE_CODE_SOURCE}", + } + removed_ids = [ + item.id for item in self._entries + if item.source in singleton_sources + ] + self._entries = [ + item for item in self._entries + if item.source not in singleton_sources + ] + if self._current_id == entry.id: + self._current_id = None + self._persist(removed_ids=removed_ids) return None self._mark_exhausted(entry, None) return None diff --git a/tests/agent/test_credential_pool.py b/tests/agent/test_credential_pool.py index ae28580d85866..b19bd21747f9b 100644 --- a/tests/agent/test_credential_pool.py +++ b/tests/agent/test_credential_pool.py @@ -1440,6 +1440,69 @@ def _terminal_refresh_failure(*_args, **_kwargs): assert refresh_calls["count"] == 1 +def test_nous_pool_terminal_refresh_keeps_entries_when_auth_store_save_fails(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) + monkeypatch.setenv("HERMES_SHARED_AUTH_DIR", str(tmp_path / "shared")) + _write_auth_store( + tmp_path, + { + "version": 1, + "active_provider": "nous", + "providers": { + "nous": { + "portal_base_url": "https://portal.example.com", + "inference_base_url": "https://inference.example.com/v1", + "client_id": "hermes-cli", + "token_type": "Bearer", + "scope": "inference:invoke", + "access_token": "access-token", + "refresh_token": "refresh-token", + "expires_at": "2026-03-24T12:00:00+00:00", + "agent_key": "agent-key", + "agent_key_expires_at": "2026-03-24T13:30:00+00:00", + } + }, + }, + ) + + import agent.credential_pool as credential_pool_mod + from agent.credential_pool import load_pool + from hermes_cli import auth as auth_mod + from hermes_cli.auth import AuthError + + def _terminal_refresh_failure(*_args, **_kwargs): + raise AuthError( + "Refresh session has been revoked", + provider="nous", + code="invalid_grant", + relogin_required=True, + ) + + pool = load_pool("nous") + selected = pool.select() + assert selected is not None + assert selected.source == "device_code" + + monkeypatch.setattr(auth_mod, "resolve_nous_runtime_credentials", _terminal_refresh_failure) + + def _save_failure(*_args, **_kwargs): + raise OSError("disk full") + + monkeypatch.setattr(credential_pool_mod, "_save_auth_store", _save_failure) + + assert pool.try_refresh_current() is None + + # Quarantine save failed: the entry must stay in the pool so that auth.json + # and the pool remain consistent (both still hold the revoked token). + assert [entry.source for entry in pool.entries()] == ["device_code"] + + # auth.json tokens must be untouched. + auth_payload = json.loads((tmp_path / "hermes" / "auth.json").read_text()) + nous_state = auth_payload["providers"]["nous"] + assert nous_state.get("refresh_token") == "refresh-token" + assert nous_state.get("access_token") == "access-token" + + def test_load_pool_removes_nous_device_code_when_singleton_quarantined(tmp_path, monkeypatch): monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) _write_auth_store( @@ -2873,6 +2936,53 @@ def _terminal_refresh_failure(*_args, **_kwargs): assert refresh_calls["count"] == 1 +def test_xai_oauth_terminal_refresh_keeps_entries_when_auth_store_save_fails( + tmp_path, monkeypatch +): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) + monkeypatch.delenv("XAI_API_KEY", raising=False) + monkeypatch.delenv("XAI_OAUTH_ACCESS_TOKEN", raising=False) + + _write_auth_store(tmp_path, _xai_auth_store("old-access-token", "old-refresh-token")) + + import agent.credential_pool as credential_pool_mod + from agent.credential_pool import load_pool + import hermes_cli.auth as auth_mod + from hermes_cli.auth import AuthError + + pool = load_pool("xai-oauth") + selected = pool.select() + assert selected is not None + assert selected.source == "device_code" + + def _terminal_refresh_failure(*_args, **_kwargs): + raise AuthError( + "Refresh session has been revoked", + provider="xai-oauth", + code="xai_refresh_failed", + relogin_required=True, + ) + + monkeypatch.setattr(auth_mod, "refresh_xai_oauth_pure", _terminal_refresh_failure) + + def _save_failure(*_args, **_kwargs): + raise OSError("disk full") + + monkeypatch.setattr(credential_pool_mod, "_save_auth_store", _save_failure) + + assert pool.try_refresh_current() is None + + # Quarantine save failed: the entry must stay in the pool so that auth.json + # and the pool remain consistent (both still hold the revoked token). + assert [entry.source for entry in pool.entries()] == ["device_code"] + + # auth.json tokens must be untouched. + auth_payload = json.loads((tmp_path / "hermes" / "auth.json").read_text()) + tokens = auth_payload["providers"]["xai-oauth"].get("tokens", {}) + assert tokens.get("access_token") == "old-access-token" + assert tokens.get("refresh_token") == "old-refresh-token" + + def test_xai_oauth_nonterminal_refresh_does_not_quarantine(tmp_path, monkeypatch): monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) monkeypatch.delenv("XAI_API_KEY", raising=False) @@ -3014,6 +3124,53 @@ def _terminal_refresh_failure(*_args, **_kwargs): assert refresh_calls["count"] == 1 +def test_codex_oauth_terminal_refresh_keeps_entries_when_auth_store_save_fails( + tmp_path, monkeypatch +): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) + monkeypatch.delenv("OPENAI_API_KEY", raising=False) + monkeypatch.delenv("CODEX_OAUTH_ACCESS_TOKEN", raising=False) + + _write_auth_store(tmp_path, _codex_auth_store("old-access-token", "old-refresh-token")) + + import agent.credential_pool as credential_pool_mod + from agent.credential_pool import load_pool + import hermes_cli.auth as auth_mod + from hermes_cli.auth import AuthError + + pool = load_pool("openai-codex") + selected = pool.select() + assert selected is not None + assert selected.source == "device_code" + + def _terminal_refresh_failure(*_args, **_kwargs): + raise AuthError( + "Refresh session has been revoked", + provider="openai-codex", + code="codex_refresh_failed", + relogin_required=True, + ) + + monkeypatch.setattr(auth_mod, "refresh_codex_oauth_pure", _terminal_refresh_failure) + + def _save_failure(*_args, **_kwargs): + raise OSError("disk full") + + monkeypatch.setattr(credential_pool_mod, "_save_auth_store", _save_failure) + + assert pool.try_refresh_current() is None + + # Quarantine save failed: the entry must stay in the pool so that auth.json + # and the pool remain consistent (both still hold the revoked token). + assert [entry.source for entry in pool.entries()] == ["device_code"] + + # auth.json tokens must be untouched. + auth_payload = json.loads((tmp_path / "hermes" / "auth.json").read_text()) + tokens = auth_payload["providers"]["openai-codex"].get("tokens", {}) + assert tokens.get("access_token") == "old-access-token" + assert tokens.get("refresh_token") == "old-refresh-token" + + def test_codex_oauth_nonterminal_refresh_does_not_quarantine(tmp_path, monkeypatch): monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes")) monkeypatch.delenv("OPENAI_API_KEY", raising=False)