fix(agent): keep pool entries when terminal-OAuth quarantine save fails

[roryford]

Problem

The three terminal-OAuth-error handlers in agent/credential_pool.py — xAI, Codex, and Nous — share an identical structure: when a refresh token is terminally revoked, they (a) clear/quarantine the token in auth.json inside a try/except that only logs at debug, then (b) unconditionally remove the matching entries from the in-memory pool and persist.

If step (a) fails — the auth-store flock has a 15s timeout and is contended by the refresh path itself; the save path does O_EXCL open + fsync + atomic replace, all of which can raise — the pool is wiped while auth.json still holds the revoked token. On the next load_pool(), _seed_from_singletons re-seeds the dead credential and the agent loops on terminal auth failures with no path to the clean "re-login required" state.

This is exactly the re-seeding loop the quarantine logic (#27911, #28118, #28116) was added to prevent — it fails back into it when its own I/O fails.

Fix

Gate the pool-entry removal on the quarantine actually completing: a cleared flag is set when the try block finishes without exception (including the no-op case where the store's refresh token doesn't match the entry's), and on exception the handler now logs at warning and leaves the pool entry in place. Pool and auth.json stay consistent either way, and the next refresh attempt retries the quarantine. Same minimal change applied to all three provider handlers.

Tests

Added to tests/agent/test_credential_pool.py, one per provider: monkeypatch _save_auth_store to raise OSError, trigger a terminal refresh error, assert the pool entry survives and auth.json is untouched. All three new tests fail against pre-fix code (verified via stash round-trip) and pass with the fix. Existing success-path quarantine tests pass unchanged — 89 total in the touched files; ruff clean.

🤖 Generated with Claude Code

[liuhao1024]

Verification: OAuth quarantine save-failure guard looks correct.

Applied security checklist to this credential-pool PR:

1. Defense-in-depth preserved: The cleared flag ensures that when _save_auth_store raises (e.g. disk full), the in-memory pool entries remain consistent with on-disk auth.json — both still hold the revoked token. Previously, the entry was unconditionally evicted from the pool while auth.json still contained it, creating a silent divergence.

2. Fail-closed on save failure: When the auth store save fails, the pool entry stays and return None still fires, so the caller sees the terminal failure and doesn't retry the same entry. The entry will be retried on the next refresh cycle, which is the correct behavior — the token may still be valid on disk.

3. Three providers covered consistently: xAI OAuth, Codex OAuth, and Nous device-code all receive the same cleared = False → try → cleared = True → if cleared: evict pattern. No provider is left with the old unconditional-evict path.

4. Log level upgrade from debug to warning on save failures is appropriate — a disk-full condition during credential cleanup is operationally relevant and shouldn't be hidden at debug level.

5. Test coverage: Three dedicated tests (test_nous_pool_terminal_refresh_keeps_entries_when_auth_store_save_fails, test_xai_oauth_terminal_refresh_keeps_entries_when_auth_store_save_fails, test_codex_oauth_terminal_refresh_keeps_entries_when_auth_store_save_fails) verify the invariant: entry stays in pool, auth.json tokens are untouched. The tests also verify that the return None still fires (caller sees terminal failure).

Clean — no findings.

[tonydwb]

Good fix for terminal OAuth quarantine save failures. The _refresh_entry was deleting pool entries even when the auth store clear failed (cleared=False). Now entries are only removed if the clear actually succeeded. Also upgrades debug logs to warning for better visibility. No issues found.

[roryford]

Superseded by #63131, which re-implements this credential-pool fix on current main (this branch was ~3740 commits stale and the surrounding code had been restructured). The replacement is dynamically verified (fails on unfixed main, passes with the fix) and full-suite-clean vs baseline. Closing in favor of #63131.