Skip to content

fix(fallback): benched credentials must not disqualify a chain entry - #79840

Open
rodrigogs wants to merge 1 commit into
NousResearch:mainfrom
rodrigogs:pr/fallback-benched-credential
Open

fix(fallback): benched credentials must not disqualify a chain entry#79840
rodrigogs wants to merge 1 commit into
NousResearch:mainfrom
rodrigogs:pr/fallback-benched-credential

Conversation

@rodrigogs

Copy link
Copy Markdown

Problem

A healthy provider silently vanished from the fallback chain, leaving a free-tier model as the last resort for 47 minutes.

When DeepSeek returned 402 Insufficient Balance, credential_pool benched the key for an hour (EXHAUSTED_TTL_DEFAULT_SECONDS; 402 is billing by definition, so it correctly keeps the full bench). resolve_provider_client then returned None — which at the call site in try_activate_fallback is indistinguishable from a provider the user never configured. Two things followed:

1. A false log message. The chain logged:

Fallback to deepseek failed: provider not configured

DeepSeek was the primary provider in config.yaml and had served 195 calls earlier the same day. The message sent diagnosis toward config and credentials when the real cause was a billing bench with a known expiry.

2. Permanent suppression from a temporary condition. The entry was added to _unavailable_fallback_keys, a memo cleared only when fallback_providers content changes (gateway/run.py:_apply_fallback_chain_to_agent). Credentials normally arrive via hermes auth (writes auth.json) or a new env var — neither touches config.yaml — so a one-hour bench removed the entry for the entire life of the cached agent.

Reproduced deterministically by setting the pool entry to last_status=exhausted, last_error_code=402, last_status_at=now-7s:

pool.has_credentials=True  pool.has_available=False
_select_pool_entry -> (True, None)      # pool present, nothing selectable
resolve_provider_client -> None         # logged as "provider not configured"

Fix

credential_pool already separates the two cases — has_credentials() means the user configured something, has_available() means one is usable right now. Use it:

  • _fallback_provider_benched_until() returns None for genuinely unconfigured (memoize, as before), or the epoch time the next credential re-enters rotation for a benched one (skip this turn, stay retryable). An unreadable pool is treated as unconfigured, preserving previous behavior.
  • Benched entries log honestly and surface the wait: Fallback to deepseek skipped: configured, but all credentials are in cooldown (retry in 59m). Staying retryable for later turns.
  • _unavailable_fallback_keys gains a 10-minute TTL (UNAVAILABLE_FALLBACK_RETRY_SECONDS). An unconfigured provider still is not re-probed on every activation, but the memo can no longer outlive the condition that created it. The container stays a set so every existing key in unavailable check — including tests/run_agent/test_nous_fallback_unavailable.py — is unaffected; stamps live in a side table.

Impact

The affected chain had 6 entries but only 3 distinct credentials (zai ×2 sharing GLM_API_KEY, openai-codex ×2 sharing one device_code, nous). With deepseek wrongly dropped, the only pool not already exhausted was the free tier, which then ran 467 API calls and produced a 517-iteration tool loop (separate fix in #79839).

Redundancy that collapses under pressure into "the worst model is the only survivor" is worth guarding: this fix keeps a temporarily-benched good provider in the running instead of retiring it.

Verification

  • Reproduction above now classifies as BENCHED (retryable) (retry in 59m) and is not memoized.
  • New tests/run_agent/test_fallback_benched_credential.py: benched entry stays retryable; unconfigured entry still memoized; memo expires so a provider configured mid-uptime is reconsidered; memo still short-circuits within its window.
  • pytest -k "fallback or failover or credential_pool or guardrail or chat_completion" → 960 passed, 4 skipped. ruff clean.

(One unrelated pre-existing failure in tests/tools/test_web_tools_config.py::TestParallelClientConfig reproduces on a clean checkout — order-dependent state leakage, untouched here.)

@rodrigogs

Copy link
Copy Markdown
Author

Hi — heads-up that CI has never actually run on this PR: every workflow run (CI + Docker Build) since it was opened ends in action_required with zero jobs, and the run page says 'This workflow is awaiting approval from a maintainer'. The branch is mergeable with no conflicts and doesn't touch any workflow files. Could a maintainer approve the workflow runs (or enable CI for outside collaborators)? Happy to address anything the checks find.

@rodrigogs
rodrigogs force-pushed the pr/fallback-benched-credential branch 2 times, most recently from acf0d5f to dab42b7 Compare August 23, 2026 23:09
@rodrigogs

Copy link
Copy Markdown
Author

Rebased onto current upstream/main (0a171fffe); new head dab42b78f84c.

Effectively zero drift on the touched file, which is worth stating precisely because 492 commits sounds like
a lot: agent/chat_completion_helpers.py is byte-identical between this branch's old base (4a5b6dd45)
and current upstream/main — verified by md5 of git show on both sides. The commits that show up in
git log upstream/main -- agent/chat_completion_helpers.py are all ancestors of the merge base, not new work.
Every symbol the PR depends on still exists unchanged.

Verification on the new head: tests/run_agent/test_fallback_benched_credential.py4 passed.

This repository does not run CI on pull requests from forks, so the checks tab stays empty and protect-main's required All required checks pass context never reports — which is why this PR shows mergeable: true with mergeStateStatus: BLOCKED. Approving the workflow run (or landing it on the strength of the local evidence) is all that is left from my side.

Two defects made a healthy provider vanish from the fallback chain, leaving a
free model as the last resort for 47 minutes.

When DeepSeek returned 402 Insufficient Balance, credential_pool benched the key
for an hour (EXHAUSTED_TTL_DEFAULT_SECONDS). resolve_provider_client then
returned None, which at the call site is indistinguishable from a provider the
user never configured. Two consequences followed:

1. The chain logged "Fallback to deepseek failed: provider not configured" — a
   false statement that sent diagnosis toward config and credentials when the
   real cause was a billing bench with a known expiry. credential_pool already
   separates the cases: has_credentials() means configured, has_available()
   means usable right now. Use that, and report the remaining cooldown.

2. The entry was added to _unavailable_fallback_keys, a memo cleared only by a
   fallback_providers content edit. Credentials normally arrive via `hermes auth`
   (auth.json) or a new env var, neither of which touches config.yaml, so a
   one-hour bench removed the entry for the entire life of the cached agent.
   Give the memo a 10-minute TTL: an unconfigured provider still is not
   re-probed every activation, but the memo can no longer outlive the condition
   that created it.

Observed impact: with deepseek dropped from a 6-entry chain that collapses into
3 distinct credentials (zai, openai-codex, nous), the only pool not already
exhausted was the free tier, which then ran 467 API calls and produced a
517-iteration tool loop.
@rodrigogs
rodrigogs force-pushed the pr/fallback-benched-credential branch from dab42b7 to d8b5cd8 Compare August 24, 2026 15:10
@rodrigogs

Copy link
Copy Markdown
Author

Force-pushed a metadata-only fix so the contributor attribution check can pass.

.github/workflows/contributor-check.yml exits 1 on any commit-author email that has no file under
contributors/emails/, and ci.yaml calls it, so it feeds the required All required checks pass context.
This branch carried a placeholder author identity from a misconfigured local git config, which the check
would have rejected the moment a maintainer approved the workflow runs.

Every commit's author is now Rodrigo Gomes <2362425+rodrigogs@users.noreply.github.com> — the account's
GitHub noreply address, which the check auto-resolves via its +…@users.noreply.github.com rule, so no
mapping file is needed. Author dates are preserved, and the tree is byte-identical: git diff <old-head> <new-head> is empty, so nothing about the change under review moved and the verification I posted earlier
still stands.

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 area/billing Account usage, credit usage, billing (cross-cutting) comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists provider/deepseek DeepSeek API type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants