fix(auth): surface real upstream 429 when all pool entries exhausted (#40960) - #57137
Conversation
…ousResearch#40960) When all credential pool entries are in exhaustion cooldown (e.g. after a 429 quota-exhausted response from the upstream provider), _resolve_api_key_provider_secret() silently returned an empty api_key. This caused the API request to fail with a misleading 401 Unauthorized error, hiding the real cause (a 429 quota message from the provider). Fix: after pool.peek() returns None, iterate pool._entries and return the first usable key from an exhausted entry, with a warning log. The upstream API then returns its real 429 with the quota-reset timestamp, giving users actionable information about when their quota will recover. Closes NousResearch#40960
Duplicate of #40961 - same fix in |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the empty-key path. The premise is still present on current main: hermes_cli/auth.py:595-609 returns an empty key when pool.peek() has no available entry.
Problems
- The new loop at
hermes_cli/auth.py:611reads every raw entry, includingSTATUS_DEADcredentials.agent/credential_pool.py:1483-1516deliberately excludes dead entries and entries inside exhaustion cooldowns; returning either can still produce the misleading 401 this change aims to avoid. - Reissuing a request with a cooldown entry conflicts with the current recovery policy:
run_agent.py:293-315says retrying an exhausted quota burns retry budget, and the documented pool flow falls through to a configured fallback after all keys are exhausted (website/docs/user-guide/features/credential-pools.md:24-40). - No regression test exercises
peek() is Nonewith exhausted/dead entries; the current fallback tests only cover a healthy pool entry (tests/tools/test_credential_pool_env_fallback.py:181-250).
Suggested changes
- Avoid raw
_entriesaccess; route the intended all-exhausted behavior through a pool API that preserves dead-entry and cooldown semantics. - Add focused exhausted/dead-pool regression coverage after agreeing on the intended fallback or diagnostic behavior.
This is an automated hermes-sweeper review.
| try: | ||
| for exhausted_entry in pool._entries: | ||
| ex_key = ( | ||
| getattr(exhausted_entry, "access_token", "") |
There was a problem hiding this comment.
This raw iteration includes STATUS_DEAD entries as well as cooldown entries. CredentialPool._available_entries() intentionally excludes dead credentials (agent/credential_pool.py:1483-1516); returning one here can send a permanently invalid key and retain the same 401 failure.
Summary
Fix issue #40960: when all credential pool entries are in exhaustion cooldown (e.g. after a 429 quota-exhausted response),
_resolve_api_key_provider_secret()silently returned an emptyapi_key. This caused the API request to fail with a misleading 401 Unauthorized, hiding the real cause (a 429 quota message from the provider).Root Cause
hermes_cli/auth.pyL593-605: whenpool.peek()returnsNone(all entries in cooldown), the code returns an empty string forapi_keyinstead of falling back to an exhausted entry.Fix
After
peek()returnsNone, iteratepool._entriesand return the first usable key with a warning log. The upstream API then returns its real 429 with the quota-reset timestamp, giving users actionable information.Test Plan
ast.parse()passesRelated