fix(credential-pool): stop lost-update cooldown erasure and unmatched-hint wrong-key quarantine (#65844 salvage) - #70147
Merged
Merged
Conversation
teknium1
force-pushed
the
salvage/65844-cooldown-races
branch
from
July 23, 2026 15:30
1752cbc to
c1b46e7
Compare
Contributor
૮ >ﻌ< ა ci reviewran on 78fbca6 all good! |
… quarantine Two related races in credential-pool cooldown state: 1. Lost update across processes: write_credential_pool merged only entries missing from the caller's snapshot; for entries present on both sides the caller's in-memory copy won wholesale. A process holding a snapshot taken before another process marked a key exhausted would, on its next persist (e.g. a round-robin rotation), write the key back as healthy — erasing the cooldown so every process resumes hammering a rate-limited key. Merge status fields by last_status_at recency: adopt the on-disk status only when it is strictly newer AND still binding (DEAD, or EXHAUSTED with an unexpired cooldown), and never onto re-authed (token-changed) entries, so legitimate expiry-clears and fresh logins are preserved. 2. Wrong-key quarantine: when mark_exhausted_and_rotate received an api_key_hint that matched no entry, it fell through to current()/_select_unlocked() — on a freshly loaded pool that selects the NEXT healthy key and benches it for the full cooldown TTL, punishing an innocent credential. When a hint is provided but unmatched, rotate without marking anything instead of guessing. Includes regression tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up to the #65844 salvage: the new anthropic pool test must stub read_claude_code_credentials like the sibling tests, otherwise a dev machine's live claude_code singleton seeds a third entry and the no-benching assertion fails outside CI.
teknium1
force-pushed
the
salvage/65844-cooldown-races
branch
from
July 23, 2026 16:13
c1b46e7 to
78fbca6
Compare
alt-glitch
added
type/bug
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Salvage of #65844 by @drleadflow: two races in credential-pool cooldown state are closed — (1) a cross-process lost update where one process's stale in-memory snapshot, persisted later, erased another process's just-written exhausted/dead cooldown from auth.json, so every process resumed hammering a rate-limited key; (2) an unmatched
api_key_hintinmark_exhausted_and_rotatefell through tocurrent()/_select_unlocked()and benched an innocent healthy credential for the full cooldown TTL.Both premises verified live on current main (post-#69553/#69843/#69494):
write_credential_poolmerged only entries missing from the caller's snapshot — same-ID entries were overwritten wholesale — and the unmatched-hint fall-through still guessed.Changes
hermes_cli/auth.py(contributor commit, cherry-picked):_merge_disk_cooldown_state()in thewrite_credential_poolpath — adopts the on-disk status only when it is strictly newer (last_status_at) AND still binding (DEAD, or EXHAUSTED with an unexpired cooldown). Token-changed entries are exempt so re-auths never get the old cooldown resurrected; expired cooldowns are not resurrected so the pool's own expiry-clear survives. Best-effort try/except.agent/credential_pool.py(contributor): unmatched hint → rotate to a fresh selection without marking anything, with an INFO log.read_claude_code_credentialslike its siblings, so a dev machine's live~/.claudecreds can't seed a third entry and break the assertion outside CI.Validation
test_credential_pool.py,test_auth_profile_fallback.py,test_credential_pool_routing.py,test_auth_codex_quota_probe.py,test_auth_codex_provider.py).Semantically compatible with #69494's
clear_codex_pool_quota_cooldowns(a cleared status is None, never re-adopted) and orthogonal to #69553/#69843 (hint matching; this covers unmatched fall-through + persistence). Closes #65844's scope. Known residual (from the original review):proxy/adapters/xai.pystill calls rotation hint-less — follow-up material.Credit
Both fixes and all 5 regression tests by @drleadflow (#65844), cherry-picked with authorship preserved. Test-isolation fix added on top.
Infographic