fix(auth): preserve concurrently-added credentials on pool rewrite - #19608
fix(auth): preserve concurrently-added credentials on pool rewrite#19608konsisumer wants to merge 1 commit into
Conversation
|
Rebased onto current origin/main and resolved the auth/credential-pool conflicts within the original PR scope. The stale-write merge now also respects current-main intentional removal paths (manual DEAD pruning and OAuth quarantine), so concurrent disk-only credentials are preserved without resurrecting entries that were deliberately removed. Verification:
|
f5855c2 to
4ee0fae
Compare
|
Verified: the What this fixes: When two Hermes processes load the credential pool concurrently, process A's rotation/exhaustion rewrite drops credentials that process B added between A's Correctness checks:
No issues found. |
|
Validated this PR against a real-world credential-loss incident (details in #19566): a long-running gateway's rotation persist erased a credential added 6 minutes earlier by
This fixes a silent-data-loss path that's very visible to anyone running gateway + TUI concurrently. +1 for review. |
|
Merged via #53896. Your commit was cherry-picked onto current main with your authorship preserved in git log (8b4c29f, der@konsi.org). Thanks for the clean read-modify-write fix and the regression tests — both landed as-is. |
Fixes a stale-write race where a long-running Hermes process could drop credentials added by another process during credential rotation/exhaustion (issue #19566).
What changed and why
hermes_cli/auth.py::write_credential_poolnow re-reads the on-disk pool inside the existing_auth_store_lockand merges any provider entries present on disk but missing from the caller's snapshot. This is the read-modify-write pattern requested in the issue.removed_idskeyword lets callers signal entries they intentionally dropped, so the merge does not resurrect them.agent/credential_pool.py::CredentialPool.remove_indexpasses the removed entry's id throughremoved_ids.agent/credential_pool.py::load_poolrecords the disk ids before seeding/pruning and passes the diff asremoved_ids, so stale env-seeded entries pruned at load do not come back via the merge.tests/agent/test_credential_pool.py: one reproduces the lost-update scenario from the issue (writer A loads[A,B], writer B addsC, writer A rotates and persists; final pool must still containC), and one guardsremove_indexagainst being undone by the merge.The rotation rewrite path (
_persist) is the originally reported trigger but the same race exists foradd_entryand other in-memory mutators; doing the merge insidewrite_credential_poolcovers all callers without requiring each one to know about the on-disk state.How to test
pytest tests/agent/test_credential_pool.py -q(52 passed locally, including the two new regression tests).test_persist_preserves_concurrent_disk_only_entryfails onmain(cred-C is dropped) and passes after this change.What platforms tested on
Fixes #19566