Skip to content

fix(auth): return exhausted credential key instead of empty string on pool exhaustion (#40960) - #42475

Closed
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/40960-credential-pool-exhaustion
Closed

fix(auth): return exhausted credential key instead of empty string on pool exhaustion (#40960)#42475
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/40960-credential-pool-exhaustion

Conversation

@Tranquil-Flow

Copy link
Copy Markdown
Contributor

Problem: When all credential pool entries are in exhaustion cooldown, peek() returns None and the auth resolver returns an empty string as the API key. This causes the upstream provider to return a misleading 401 Unauthorized instead of the real 429 Rate Limited or 402 Payment Required error.

Fix: When peek() returns None (all entries exhausted), iterate over pool._entries and return an exhausted entry's key anyway. The upstream API will then return the real error with the correct status code and message.

Testing: 3 new regression tests in TestCredentialPoolExhaustionFallback:

  • Exhausted pool returns key instead of empty string
  • Prefers runtime_api_key over access_token
  • Truly empty pool (no entries) still returns empty string

All 11 tests in the file pass.

Closes #40960

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification comment (automated review — no issues found)

Reviewed the diff: when peek() returns None (all entries in cooldown), the new else branch iterates _entries to return any usable key. This ensures the upstream API returns the real 429/402 error with quota info, instead of a misleading 401 Unauthorized from an empty key.

Findings: Clean implementation. The _entries access is an internal detail but acceptable since _resolve_api_key_provider_secret is itself an internal function. Test coverage is solid — three scenarios (exhausted → returns key, prefers runtime_api_key, truly empty pool → empty string).

No issues found.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard labels Jun 9, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused credential-pool investigation. This is now superseded by current main.

Automated hermes-sweeper review evidence:

  • 8041be795419ee00890bbc6efd075c1459684acd (fix(model): keep configured provider authoritative) added the fail-closed guard in hermes_cli/runtime_provider.py:2010-2023.
  • hermes_cli/runtime_provider.py:2016 now raises AuthError(code="missing_api_key") when no usable API key resolves, so Hermes does not construct a runtime that sends the empty key described in this PR.
  • Gateway setup catches that resolution error and tries configured fallback providers in gateway/run.py:1857-1871.

The current behavior avoids the misleading empty-key upstream request without bypassing credential-pool cooldown selection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] credential pool exhaustion causes misleading 401 instead of real 429/402

4 participants