fix(auth): preserve concurrently-added credentials on pool rewrite (#19566) - #53896
Conversation
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-argument-type |
1 |
unresolved-attribute |
1 |
First entries
tests/agent/test_credential_pool.py:3097: [invalid-argument-type] invalid-argument-type: Argument to function `write_credential_pool` is incorrect: Expected `list[dict[str, Any]]`, found `dict[str, Any]`
tests/agent/test_credential_pool.py:3087: [unresolved-attribute] unresolved-attribute: Object of type `dict[str, Any]` has no attribute `append`
✅ Fixed issues: none
Unchanged: 6064 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
Salvage of #19608 (@konsisumer), cherry-picked onto current |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Thorough fix for credential pool concurrent-write race condition (#19566). The core issue: between loading the in-memory credential snapshot and persisting it, another process may add a credential — the later persist drops the concurrent addition.
The fix:
write_credential_poolnow re-reads the on-disk pool under the same lock and merges entries present on disk but missing from the caller's entriesremoved_idsparameter prevents the merge from resurrecting intentionally-removed credentials- Both
load_pooland_persisttrack and propagate removed IDs - Multiple call sites updated (3 terminal auth failure paths + expiry pruning + load_pool normalization)
Well-documented with clear rationale for the merge pattern. No concerns. Ready to merge.
Reviewed by Hermes Agent
Summary
Credentials added by
hermes auth addnow survive a concurrent rotation/exhaustion rewrite by another Hermes process. Previouslywrite_credential_poolblind-overwrote the on-disk pool with the caller's in-memory snapshot, so a long-running gateway that rotated on a 429 would silently erase any credential added since it started.Root cause: lost-update race. Process A loads the pool → Process B (
auth add) appends a credential and writes → Process A marks a key exhausted and persists its stale snapshot, dropping B's addition. No error, no log line.Salvage of #19608 by @konsisumer, cherry-picked onto current
main(his branch was ~2500 commits stale). Fixes #19566.Changes
hermes_cli/auth.py::write_credential_pool: re-reads the on-disk pool inside the existing_auth_store_lockand merges entries present on disk but absent from the caller's snapshot (read-modify-write). Newremoved_idskwarg lets callers signal intentional removals so the merge doesn't resurrect them.agent/credential_pool.py: plumbsremoved_idsthrough every prune-then-persist site — the three terminal-auth quarantine paths (xAIloopback_pkce, Codexdevice_code, Nous singleton),_available_entriesexpiry prune,remove_index, and theload_poolnormalization/prune diff.scripts/release.py: AUTHOR_MAP entry for konsisumer.Validation
remove_indexthen later writeHERMES_HOME: reproduced the exact field scenario (brendatoridropped on old blind-overwrite, preserved with the fix); confirmedremove_indexnot undone by merge.tests/agent/test_credential_pool.py: 81 passed (includes the 2 new regression teststest_persist_preserves_concurrent_disk_only_entry+test_remove_index_does_not_resurrect_via_disk_merge).mainand passes here.Two community reporters independently field-verified #19608 against
origin/main(entry dropped on main, preserved on the branch).Note: #19566 was previously auto-closed as resolved by #33744 — that PR fixes a different bug (device-code token-content sync on re-auth, a 401 loop) and never touches this write race.
Infographic