fix(credential_pool): lock the public pool query surface (#62614 salvage) - #70154
Merged
Conversation
teknium1
force-pushed
the
salvage/62614-pool-query-locks
branch
from
July 23, 2026 15:39
17222c0 to
e90659d
Compare
Contributor
૮ >ﻌ< ა ci reviewran on 7a4acc3 all good! |
alt-glitch
added
type/bug
…rent/entries `has_available()`, `peek()`, `current()` and `entries()` read (and, via `_available_entries()`, mutate and persist) `self._entries` without holding `self._lock`, while every other entry point — `select()`, `mark_exhausted_and_rotate()`, `acquire_lease()`, `try_refresh_current()` — guards the exact same access with the lock. `_available_entries()` is not read-only: it prunes aged-out DEAD manual entries (rebinding `self._entries` at the prune step) and calls `_persist()` (writes auth.json). The gateway runs platform adapters in threads and cron runs jobs in a ThreadPoolExecutor, so a status probe via `has_available()` or `peek()` can race a concurrent `select()`/rotation: torn iteration of `self._entries`, interleaved auth.json writes, or a lost token rotation. Fix: take `self._lock` in all four query methods. Because the lock is non-reentrant and `peek()` composes `current()` + `_available_entries()`, add a lock-free `_current_unlocked()` helper and route the already-locked internal callers (`_select_unlocked`, `mark_exhausted_and_rotate`, `_try_refresh_current_unlocked`) through it to avoid self-deadlock. Added regression tests: a no-deadlock check (peek re-entrancy) and a lock-held-blocks-the-call check for each of the four methods.
… pool surface Follow-up to review feedback: - Acquire self._lock in the remaining public pool-state methods: has_credentials, reset_statuses, remove_index, resolve_target, and add_entry. All of them read or rebind self._entries (and the mutating ones persist auth.json), so they now hold the same lock as select() and the query methods. None are called from within the lock, so no unlocked helpers are needed. - Make the blocking test deterministic: an instrumented lock records the acquire attempt, and the test first waits for the worker to actually reach self._lock before asserting it blocks. Previously an unlocked method could pass if the worker thread was scheduled late. - Extend the lock test matrix to all nine public methods; the five newly locked ones fail the test without this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ranch Follow-up to the #62614 salvage: try_refresh_matching (added by the #69843 salvage after this PR's base) calls self.current() while already holding the now-locking non-reentrant pool lock — a guaranteed deadlock that git merges silently (no textual conflict). Use _current_unlocked() and cover the method in the no-deadlock test.
teknium1
force-pushed
the
salvage/62614-pool-query-locks
branch
from
July 23, 2026 16:22
e90659d to
7a4acc3
Compare
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 #62614 by @solyanviktor-star: every public
CredentialPoolstate method now runs underself._lock. Previouslyhas_available/peek/current/entries/has_credentials(plus the management surface:reset_statuses/remove_index/resolve_target/add_entry) read — and inhas_available's case pruned and persisted — shared pool state with no lock, racing againstselect/mark_exhausted_and_rotate/ refresh, which all mutate under the lock. Gateway and auxiliary threads call these queries concurrently with rotation on every turn.The lock is a non-reentrant
threading.Lock, so the fix introduces_current_unlocked()and reroutes every in-lock caller — one lock boundary, unlocked helpers inside.The merge hazard we caught (and fixed)
The PR predates our #69843 salvage, which added
try_refresh_matching()— it callsself.current()insidewith self._lock:. Git merges the two changes with zero textual conflict, but the merged tree deadlocks the momenttry_refresh_matchingruns with no/unmatched hint (live callers: the 401 recovery path andtools/xai_http.py). Fixed with_current_unlocked()and covered in the no-deadlock test so the class of bug can't silently return.Changes
agent/credential_pool.py(contributor, 2 commits cherry-picked): lock acquisition in the 4 query methods + 5 management methods;_current_unlocked()helper; all in-lockcurrent()callers rerouted.agent/credential_pool.py(follow-up):try_refresh_matchingno-hint branch uses_current_unlocked()— the silent-merge deadlock.self._lockbefore asserting it blocks — no spurious passes) + no-deadlock smoke test. Follow-up:try_refresh_matchingadded to the no-deadlock test.Validation
test_credential_pool.py,test_credential_pool_routing.py,test_credential_pool_interrupt.py,test_codex_xai_oauth_recovery.py,test_anthropic_adapter.py,test_auth_codex_quota_probe.py).try_refresh_matchingcompletes in <5s on a background thread (hangs forever pre-follow-up on the merged tree); (2) 4 reader threads + 1 rotator hammering the pool for 3s — no hang, no exceptions.Closes #62614's scope.
Credit
Locking design, both commits, and the instrumented test matrix by @solyanviktor-star (#62614), cherry-picked with authorship preserved. The
try_refresh_matchingdeadlock fix (a post-PR merge hazard) added on top.Infographic