Skip to content

feat(api): credential pool with rotation on auth / rate-limit failures - #780

Closed
kevincodex1 wants to merge 1 commit into
Twigpine:mainfrom
kevincodex1:feat/credential-pool-rotation
Closed

kevincodex1 wants to merge 1 commit into
Twigpine:mainfrom
kevincodex1:feat/credential-pool-rotation

Conversation

@kevincodex1

Copy link
Copy Markdown
Member

Heavy users of OpenAI-compatible providers (OpenRouter, Groq, Together, self-hosted, personal-plus-team key mixes) frequently hit quota or auth failures on individual keys, which today surfaces as a hard error to the user. Adds transparent multi-key rotation.

New env var: OPENAI_API_KEYS (plural, comma-separated) takes precedence over OPENAI_API_KEY. For convenience, OPENAI_API_KEY itself is now also treated as a comma-separated list — no config migration needed for users who want rotation without adopting the new var.

Behavior:

  • Round-robin across healthy keys; single-key pools preserve prior behavior.
  • On 401/403: permanently evict the offending key from the pool for this request lifetime.
  • On 429: 30s cooldown on the offending key; rotate to next healthy key.
  • On success: clears any cooldown on the used key.
  • All keys exhausted: falls back to least-recently-failed key so the caller still gets a meaningful error rather than silent failure.

Max attempts per request = max(existing policy, pool.size), so rotation doesn't starve existing GitHub 429 retry behavior and single-key pools continue one-shot.

Auth header rebuilt per attempt via applyAuthHeaders() helper; Gemini ADC and keyless-local paths remain untouched (pool size 0 → same headers as before).

@kevincodex1
kevincodex1 force-pushed the feat/credential-pool-rotation branch from 6aa92c7 to 4bf3239 Compare April 20, 2026 07:20
Heavy users of OpenAI-compatible providers (OpenRouter, Groq, Together,
self-hosted, personal-plus-team key mixes) frequently hit quota or auth
failures on individual keys, which today surfaces as a hard error to
the user. Adds transparent multi-key rotation.

New env var: OPENAI_API_KEYS (plural, comma-separated) takes precedence
over OPENAI_API_KEY. For convenience, OPENAI_API_KEY itself is now also
treated as a comma-separated list — no config migration needed for users
who want rotation without adopting the new var.

Behavior:
- Round-robin across healthy keys; single-key pools preserve prior behavior.
- On 401/403: permanently evict the offending key from the pool for this
  request lifetime.
- On 429: 30s cooldown on the offending key; rotate to next healthy key.
- On success: clears any cooldown on the used key.
- All keys exhausted: falls back to least-recently-failed key so the caller
  still gets a meaningful error rather than silent failure.

Max attempts per request = max(existing policy, pool.size), so rotation
doesn't starve existing GitHub 429 retry behavior and single-key pools
continue one-shot.

Auth header rebuilt per attempt via applyAuthHeaders() helper; Gemini ADC
and keyless-local paths remain untouched (pool size 0 → same headers as
before).

Co-Authored-By: OpenClaude <openclaude@gitlawb.com>

@auriti auriti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-engineered feature. The credential pool module is clean, well-tested (135 lines of tests covering all edge cases), and the shim integration is minimal.

Positive:

  • credentialPool.ts is a standalone module with no external dependencies — easy to test and reason about
  • Round-robin with degraded fallback (least-recently-failed) is the right strategy — never hard-fails
  • OPENAI_API_KEY treated as comma-separated for backward compatibility — zero migration needed
  • OPENAI_API_KEYS (plural) takes precedence — clean upgrade path
  • applyAuthHeaders helper centralizes auth header management — cleaner than the previous inline logic
  • maxAttempts = Math.max(baseAttempts, credentialPool.size) preserves single-key one-shot behavior
  • 4 integration tests in openaiShim.test.ts covering 429 rotation, 401 rotation, comma-separated key, and single-key backward compat

Issues:

1. (Blocker) Pool is recreated on every _doOpenAIRequest call.

createCredentialPool(parseKeyList(keyListRaw)) runs inside the request method, which is called for every API request. The pool (and its cooldown/eviction state) is thrown away after each request. For rotation to work across a multi-turn session, the pool should be created once and stored as an instance field on OpenAIShimMessages:

// In constructor or as lazy getter:
private _credentialPool?: CredentialPool
private get credentialPool(): CredentialPool {
  if (!this._credentialPool) {
    const raw = this.providerOverride?.apiKey
      ?? process.env.OPENAI_API_KEYS
      ?? process.env.OPENAI_API_KEY ?? ''
    this._credentialPool = createCredentialPool(parseKeyList(raw))
  }
  return this._credentialPool
}

2. 403 treated as permanent eviction may be too aggressive. OpenRouter returns 403 for content policy violations, not bad keys. Consider treating 403 as cooldown (like 429) rather than permanent eviction.

3. response.text().catch(() => {}) on rotation — consuming the body before retry is correct, but the empty catch swallows useful error info. Consider logging at debug level.

Minor:

  • findByToken uses linear scan — fine for 2-5 keys, but a Map would be O(1)
  • Consider a CLAUDE_CODE_CREDENTIAL_POOL_COOLDOWN_MS env var for power users

Overall: strong implementation. Issue #1 is the only blocker — without it, rotation state resets on every API call.

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes for a blocking implementation issue:

  • src/services/api/openaiShim.ts: the credential pool is still created inside _doOpenAIRequest with createCredentialPool(parseKeyList(...)), so cooldown/eviction state is recreated on every API request. That means auth/rate-limit history does not survive across turns, which defeats the rotation feature.

Residual gap: I still do not see coverage for Azure api-key auth or 403 classification.

@gnanam1990 gnanam1990 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean pool with sensible rotation / eviction / cooldown semantics and a good degraded-pick fallback so callers never get a silent null. Integration into the shim request loop is minimal and the tests exercise 401 / 429 rotation end-to-end. Two small notes for follow-up: (1) 403 is currently classed as permanent auth-eviction, but some providers use 403 for recoverable quota — worth reconsidering. (2) The silent comma-split of OPENAI_API_KEY could surprise users with comma-containing keys; worth a note in the docs. LGTM.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [P1] Keep credential pool state across API requests
    src/services/api/openaiShim.ts:1439
    The pool is created inside _doOpenAIRequest, so every messages.create call starts from a fresh cursor with no prior cooldown or eviction state. That means a key that just returned 429 or 401 is immediately retried as the first key on the next turn/request, which defeats the feature for the long-running sessions this PR is meant to help. The current tests only prove rotation within one request; please keep the pool on the OpenAIShimMessages instance (or another client-lifetime cache keyed by the effective key list/provider override) and add a sequential-request test showing a failed key is skipped on the next API call.

@jatmn

jatmn commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

closing to be replaced by #1706

@jatmn jatmn closed this Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants