fix(auth): a deliberate reset must outrank a binding cooldown - #90318
fix(auth): a deliberate reset must outrank a binding cooldown#90318rodrigogs wants to merge 2 commits into
Conversation
|
Confirmed on a live multi-account OpenAI Codex setup. Before this patch, I tested commit |
7d13286 to
d075be7
Compare
|
Thanks for taking the time to reproduce this on a real multi-account setup — that Worth underlining one detail from your report for whoever gates this: you cleared The branch is currently |
`hermes auth reset <provider>` printed "Reset status on N credentials" and left the pool exactly as it was. Measured on a live install: an entry benched with 402 / Insufficient Balance kept `last_status: exhausted` and the same `last_status_at`, read back in a fresh process immediately after the reset, and the CLI went on refusing the provider with "No usable credentials found" long after the account had been topped up. Two causes, one per file. write_credential_pool keeps a newer on-disk cooldown over the caller's snapshot so one process cannot resurrect a key another has just rate-limited. That merge compares `last_status_at`, and clearing sets it to None, which parses as epoch 0 — older than any real timestamp. So a deliberate operator reset was indistinguishable from the stale snapshot the merge exists to reject, and the cooldown was copied straight back over the cleared fields. The failure profile made it hard to see: `_merge_disk_cooldown_state` returns early for a cooldown that has already expired, so a reset appeared to work whenever it did not matter and silently did nothing whenever it did. reset_statuses also never cleared `failure_reason`. It lives in `extra` rather than as a dataclass field, so `replace()` could not reach it, and an entry came out of a reset with no status and no error code but still classified `billing` — which `hermes auth list` renders as though it were current. The fix threads the caller's intent through, mirroring the `removed_ids` parameter that already exists one concern over for the same reason: an id in `status_cleared_ids` says "I have seen the newer status and I am dropping it", as opposed to "I have not seen it". Anything that does not declare the intent keeps the old, protective behaviour. Tests: three in tests/agent/test_credential_pool.py. The cooldown in the two regression tests is deliberately recent, because an expired one passes with or without the fix. Verified by reverting the source change with the tests kept — both regression tests fail and the third keeps passing, so it is known to pin the concurrency guard rather than the new flag. The guard test exists because "skip the merge always" would have made these two pass while reintroducing the lost update the merge was written to prevent. 204 passed, 2 skipped across tests/agent/test_credential_pool.py and tests/hermes_cli/test_auth*.py.
d075be7 to
3a24755
Compare
|
Rebased onto current No semantic drift. One cosmetic tidy is folded into the existing commit rather than added as a whitespace-only commit: the new Verification on the new head: This repository does not run CI on pull requests from forks, so the checks tab stays empty and |
|
A few repo-specific unblockers after the successful rebase, to avoid another CI round trip:
The live multi-account OpenAI Codex evidence remains positive on the rebased semantics: the deliberate reset survives a fresh read, no OAuth token changes, and the recovered account completes a real |
|
Thanks for the thorough live validation @benperry6 — the multi-account reproduction and the quota-restored recovery path are exactly the scenarios the fix targets. Both unblockers are done:
The implementation itself is unchanged — |
Problem
hermes auth reset <provider>reports success and changes nothing — but only while the cooldown it is meant to clear is still in force.Observed on a live install after topping up a DeepSeek account. The key was healthy (a direct
POST /chat/completionswith the pooled token answered200, and/user/balancereportedtotal_balance "19.92"), yet every agent invocation still refused the provider:Read back from
auth.jsonin a fresh process immediately after the reset,last_status,last_status_atandlast_error_codewere byte-identical to their pre-reset values.1. The concurrency guard cannot tell a reset from a stale snapshot.
write_credential_poolmerges on-disk status over the caller's entries via_merge_disk_cooldown_state, so one process cannot resurrect a key another has just benched. That merge adopts the disk copy when it is strictly more recent bylast_status_at.reset_statusesclearslast_status_attoNone, which_parse_absolute_timestampyields as0.0— older than any real timestamp. So a deliberate operator reset is by construction the losing side of that comparison, and the cooldown is copied straight back over the cleared fields.2. The failure profile hides it.
_merge_disk_cooldown_statereturns early when the on-disk cooldown has already expired (until <= time.time()). So the command works whenever clearing was unnecessary, and silently does nothing whenever it was the reason you ran it. A test that resets an expired cooldown passes either way, which is why the existing coverage —test_query_method_acquires_lock, which only asserts thatreset_statusestakes the lock — never saw this.3.
failure_reasonwas never cleared at all. It is persisted through_EXTRA_KEYSand lives inPooledCredential.extra, not as a dataclass field, soreplace()could not reach it. Entries came out of a reset with no status and no error code but still classifiedbilling, whichhermes auth listrenders as though it were current.Fix
Thread the caller's intent through, mirroring the
removed_idsparameter that already exists one concern over for the same reason — "do not resurrect this from disk, I removed it on purpose":write_credential_pooltakesstatus_cleared_ids. Entries whose id is in that set skip_merge_disk_cooldown_stateentirely. An id in the set means "I have seen the newer status and I am dropping it", as opposed to "I have not seen it".CredentialPool._persistforwardsstatus_cleared_ids;reset_statusespasses the ids it cleared.reset_statusesalso clearsfailure_reason(viaextra) and treats its presence as a reason to clear, so an entry carrying only a stale classification is not skipped.Every caller that does not declare the intent keeps the previous, protective behaviour unchanged.
Verification
has_available()isTruefrom a freshload_pool.tests/agent/test_credential_pool.py. The cooldown in the two regression tests is deliberately 5 seconds old, because an expired one passes with or without the fix:test_reset_statuses_clears_a_cooldown_that_is_still_binding— asserts against what reached disk, not the in-memory object, which was correct all along.test_reset_statuses_clears_the_classified_failure_reasontest_a_persist_without_declared_intent_still_cannot_erase_a_cooldown— the guard this fix is scoped against. Without it, "skip the merge always" would make the other two pass while reintroducing the lost update_merge_disk_cooldown_stateexists to prevent.pytest tests/agent/test_credential_pool.py tests/hermes_cli/test_auth*.py→ 204 passed, 2 skipped.ruffclean on the changed files.The wider
-k "credential_pool or auth or fallback or failover"selection reports 27 failed on a cleanmainworktree (3638 passed) and the same 27 on this branch, with identical test names — intests/tools/test_mcp_oauth.py,test_mcp_dashboard_oauth.py,test_terminal_tool_requirements.py,test_web_tools_config.pyandtests/docker/. None of them importcredential_poolorhermes_cli/auth.py. The three MCP OAuth ones fail in isolation too; the rest only inside the large selection, i.e. the order-dependent state leakage already noted in #79840.Context
Same subsystem as #79840, which fixed a benched credential being mistaken for an unconfigured provider. That one made a cooldown recoverable automatically; this one makes it clearable deliberately.