fix(auth): serialize Codex OAuth pool refresh under the auth-store lock - #240
Conversation
|
Review Complete Files Reviewed: 3 By Severity:
One low-severity issue: the Codex OAuth credential pool refresh path computes a user-configured timeout but fails to pass it through to the underlying HTTP call, leaving the default 20s timeout in effect regardless of configuration. Files Reviewed (3 files) |
There was a problem hiding this comment.
Risk: 🟢 Low (18/100) — 1 low finding · 109 LOC across 3 files
Findings Summary
finding-001: Timeout not propagated in Codex pool refresh (low, confidence 85)
File: agent/credential_pool.py:989
The PR adds a cross-process lock for Codex OAuth token refresh in _refresh_entry, computing refresh_timeout_seconds from HERMES_CODEX_REFRESH_TIMEOUT_SECONDS and correctly sizing the lock timeout to refresh_timeout_seconds + 5. However, the call to _refresh_entry_impl does not pass this timeout through, and refresh_codex_oauth_pure always uses the default 20s. When a user increases HERMES_CODEX_REFRESH_TIMEOUT_SECONDS for slow networks, the lock waits up to the extended duration while the HTTP POST still times out at 20s, defeating the purpose of the configuration.
Fix: Thread refresh_timeout_seconds through _refresh_entry_impl to refresh_codex_oauth_pure.
Additional Observations (below confidence threshold, not in review)
- xAI OAuth lacks cross-process lock (confidence 78): The xAI refresh path in the pool lacks equivalent lock serialization despite also using single-use refresh tokens, leaving a race window.
- Lock held unnecessarily for non-device_code entries (confidence 72): The auth-store lock gates only on provider type, holding lock during HTTP POST for non-device_code entries that don't interact with auth.json.
| entry = synced | ||
| if not force and not self._entry_needs_refresh(entry): | ||
| return entry | ||
| return self._refresh_entry_impl(entry, force=force) |
There was a problem hiding this comment.
🟢 Codex pool refresh computes timeout but does not propagate it to refresh_codex_oauth_pure (bug)
In agent/credential_pool.py, the new Codex OAuth lock path in _refresh_entry reads HERMES_CODEX_REFRESH_TIMEOUT_SECONDS to compute refresh_timeout_seconds (line 976) and correctly sizes the lock timeout to refresh_timeout_seconds + 5 (line 979-982). However, it calls _refresh_entry_impl (line 989) without passing this timeout through. The _refresh_entry_impl method (line 992) does not accept a timeout parameter, and its openai-codex branch (line 1030) calls auth_mod.refresh_codex_oauth_pure() using the default timeout_seconds=20.0. The singleton refresh path in hermes_cli/auth.py (resolve_codex_runtime_credentials → _refresh_codex_auth_tokens) correctly passes timeout_seconds through. When a user increases HERMES_CODEX_REFRESH_TIMEOUT_SECONDS for slow networks, the pool-held lock waits for the full extended duration but the HTTP call still times out at 20s, defeating the purpose of the configuration.
💡 Suggestion: Thread refresh_timeout_seconds through to _refresh_entry_impl and on to refresh_codex_oauth_pure. Add a *, timeout_seconds: float = 20.0 parameter to _refresh_entry_impl and pass it to the refresh_codex_oauth_pure call at line 1030. Update both call sites (lines 989 and 990) to pass the appropriate timeout value.
| return self._refresh_entry_impl(entry, force=force) | |
| return self._refresh_entry_impl(entry, force=force, timeout_seconds=refresh_timeout_seconds) |
📋 Prompt for AI Agents
In agent/credential_pool.py: (1) At line 989, change return self._refresh_entry_impl(entry, force=force) to return self._refresh_entry_impl(entry, force=force, timeout_seconds=refresh_timeout_seconds). (2) At line 992, change the signature to def _refresh_entry_impl(self, entry: PooledCredential, *, force: bool, timeout_seconds: float = 20.0) -> Optional[PooledCredential]:. (3) At line 1030-1032, change refreshed = auth_mod.refresh_codex_oauth_pure(entry.access_token, entry.refresh_token,) to refreshed = auth_mod.refresh_codex_oauth_pure(entry.access_token, entry.refresh_token, timeout_seconds=timeout_seconds,). (4) Line 990's non-codex fallthrough call already passes the default 20.0 which is correct for other providers.
Summary
Two concurrent Hermes processes can no longer spend the same single-use Codex OAuth refresh token, so the
refresh_token_reused/invalid_grantrace on the credential-pool refresh path is closed.Root cause:
CredentialPool._refresh_entrysynced Codex tokens fromauth.jsonand then POSTed the refresh token to OpenAI's token endpoint without holding the cross-process auth-store lock across the whole read → POST → write-back sequence. Codex refresh tokens are single-use, so two processes could both adopt the same on-disk token and both POST it — the loser gotrefresh_token_reused. (The non-poolresolve_codex_runtime_credentials()path already serialized under this lock; the pool path did not.)Changes
agent/credential_pool.py: wrap the Codex OAuth branch of_refresh_entryin the existing reentrant, cross-process_auth_store_lock(extended timeout =max(AUTH_LOCK_TIMEOUT_SECONDS, refresh_timeout + 5)— the same patternresolve_codex_runtime_credentials()uses). A waiter blocks on the flock; once inside, the in-lock re-sync adopts the rotated token the winner persisted and skips its own POST.hermes_cli/auth.py: sendUser-Agent: hermes-cli/<version>on the Codex refresh request.tests/agent/test_credential_pool_oauth_writethrough.py: invariant test assertingrefresh_codex_oauth_pureis only ever called while the auth-store lock is held.Validation
refresh_token_reusedevery run (3/3 fail)tests/agent/test_credential_pool*.py,tests/hermes_cli/test_auth_codex*.pyruff,py_compileE2E was two real Python processes hitting a real local HTTP token endpoint that rejects any refresh token it has already consumed — the flip-the-fix run (lock reverted) reproduced the reported failure deterministically.
Credit
Supersedes NousResearch#34820 by @cooper-oai, which identified this concurrent-refresh reuse race. This ships the narrow lock-serialization fix (plus the UA header) using the existing shared auth-store lock, without the separate Codex auth-store partition (
_codex_auth_store_lock/_codex_auth_file_path/ refresh-owner enforcement) — the "stop auto-importing~/.codextokens" and singleton-refresh-under-lock pieces of that PR are already onmain.Infographic
Mirror-of: NousResearch#56233
NousResearch#56233