mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-27 17:58:07 +00:00
fix(auxiliary): keep provider-wide skip for auth/payment failures in fallback chain
Follow-up to the same-provider fallback fix: narrowing the configured-chain skip to the exact failed model is only correct for model-specific failures. Auth (401) and payment (402) errors are provider-wide — every model on the provider shares the same broken credentials/account — so trying a sibling model can't recover and merely burns another doomed request before the aux task fails. Worse, returning that sibling client bypasses the main-agent-model safety net that a provider-wide skip would have reached. Only forward failed_model to _try_configured_fallback_chain for model-specific failures (timeout, connection, rate limit, model-incompatible, invalid response). Auth/payment keep failed_model=None (whole-provider skip), preserving the pre-existing safety-net behaviour for credential/billing failures. Adds an integration test that a timeout forwards the failed model, and updates the payment-error test to assert failed_model=None. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
e8f8b34b0c
commit
83f20e07e0
2 changed files with 81 additions and 17 deletions
|
|
@ -4335,18 +4335,23 @@ def _try_configured_fallback_chain(
|
|||
``base_url``, and ``api_key`` are optional.
|
||||
|
||||
``failed_model`` narrows the skip check to the exact (provider, model)
|
||||
pair that just failed, rather than the whole provider. Without it (the
|
||||
"no client could be built" callers, where credentials/auth are broken
|
||||
for the whole provider regardless of model) every entry sharing that
|
||||
provider is skipped, same as before. With it (the runtime request-error
|
||||
callers), a chain that intentionally lists several models under the
|
||||
same provider — e.g. two more NVIDIA NIM models after the primary NIM
|
||||
model times out — is no longer skipped wholesale; only the entry that
|
||||
is an exact match for what just failed is skipped, so the chain can
|
||||
still serve the request from the same provider instead of jumping
|
||||
straight to the main-agent-model safety net. See issue where an NVIDIA
|
||||
NIM timeout on the primary compression model fell through to the main
|
||||
Codex model instead of trying the two other configured NIM fallbacks.
|
||||
pair that just failed, rather than the whole provider. Without it every
|
||||
entry sharing the failed provider is skipped (the original behaviour).
|
||||
Callers pass it only when a sibling model on the same provider could
|
||||
plausibly recover:
|
||||
|
||||
- Model-specific runtime failures (timeout, connection, rate limit,
|
||||
model-incompatible, invalid response) pass ``failed_model`` so a
|
||||
chain that intentionally lists several models under the same provider
|
||||
— e.g. two more NVIDIA NIM models after the primary NIM model times
|
||||
out — is not skipped wholesale. Only the exact model that failed is
|
||||
skipped; the siblings still run instead of jumping straight to the
|
||||
main-agent-model safety net.
|
||||
- Provider-wide failures (auth 401, payment 402) and "no client could
|
||||
be built" callers leave ``failed_model`` as None, keeping the whole
|
||||
provider skipped — the shared credentials/account behind every model
|
||||
on that provider are broken, so a sibling can't help and the
|
||||
main-agent-model safety net should be reached instead.
|
||||
|
||||
Returns:
|
||||
(client, model, provider_label) or (None, None, "") if no fallback.
|
||||
|
|
@ -8099,6 +8104,15 @@ def call_llm(
|
|||
logger.info("Auxiliary %s: %s on %s (%s), trying fallback",
|
||||
task or "call", reason, resolved_provider, first_err)
|
||||
|
||||
# Narrow the configured-chain skip to the exact model that
|
||||
# failed ONLY for model-specific failures. Auth (401) and
|
||||
# payment (402) errors are provider-wide — the credentials or
|
||||
# account behind every model on that provider are the same — so
|
||||
# a sibling model can't recover; keep skipping the whole
|
||||
# provider so the main-agent-model safety net is still reached.
|
||||
_chain_failed_model = (
|
||||
None if reason in ("auth error", "payment error") else final_model
|
||||
)
|
||||
# Fallback order (#26882, #26803):
|
||||
# 1. User-configured fallback_chain (per-task) if set
|
||||
# 2. For auto: top-level main fallback_providers/fallback_model
|
||||
|
|
@ -8108,7 +8122,7 @@ def call_llm(
|
|||
if is_auto:
|
||||
fb_client, fb_model, fb_label = _try_configured_fallback_chain(
|
||||
task, resolved_provider or "auto", reason=reason,
|
||||
failed_model=final_model)
|
||||
failed_model=_chain_failed_model)
|
||||
if fb_client is None:
|
||||
fb_client, fb_model, fb_label = _try_main_fallback_chain(
|
||||
task, resolved_provider or "auto", reason=reason)
|
||||
|
|
@ -8118,7 +8132,7 @@ def call_llm(
|
|||
else:
|
||||
fb_client, fb_model, fb_label = _try_configured_fallback_chain(
|
||||
task, resolved_provider or "auto", reason=reason,
|
||||
failed_model=final_model)
|
||||
failed_model=_chain_failed_model)
|
||||
if fb_client is None:
|
||||
fb_client, fb_model, fb_label = _try_main_agent_model_fallback(
|
||||
resolved_provider, task, reason=reason)
|
||||
|
|
@ -8635,6 +8649,15 @@ async def async_call_llm(
|
|||
logger.info("Auxiliary %s (async): %s on %s (%s), trying fallback",
|
||||
task or "call", reason, resolved_provider, first_err)
|
||||
|
||||
# Narrow the configured-chain skip to the exact model that
|
||||
# failed ONLY for model-specific failures. Auth (401) and
|
||||
# payment (402) errors are provider-wide — the credentials or
|
||||
# account behind every model on that provider are the same — so
|
||||
# a sibling model can't recover; keep skipping the whole
|
||||
# provider so the main-agent-model safety net is still reached.
|
||||
_chain_failed_model = (
|
||||
None if reason in ("auth error", "payment error") else final_model
|
||||
)
|
||||
# Fallback order (#26882, #26803):
|
||||
# 1. User-configured fallback_chain (per-task) if set
|
||||
# 2. For auto: top-level main fallback_providers/fallback_model
|
||||
|
|
@ -8644,7 +8667,7 @@ async def async_call_llm(
|
|||
if is_auto:
|
||||
fb_client, fb_model, fb_label = _try_configured_fallback_chain(
|
||||
task, resolved_provider or "auto", reason=reason,
|
||||
failed_model=final_model)
|
||||
failed_model=_chain_failed_model)
|
||||
if fb_client is None:
|
||||
fb_client, fb_model, fb_label = _try_main_fallback_chain(
|
||||
task, resolved_provider or "auto", reason=reason)
|
||||
|
|
@ -8654,7 +8677,7 @@ async def async_call_llm(
|
|||
else:
|
||||
fb_client, fb_model, fb_label = _try_configured_fallback_chain(
|
||||
task, resolved_provider or "auto", reason=reason,
|
||||
failed_model=final_model)
|
||||
failed_model=_chain_failed_model)
|
||||
if fb_client is None:
|
||||
fb_client, fb_model, fb_label = _try_main_agent_model_fallback(
|
||||
resolved_provider, task, reason=reason)
|
||||
|
|
|
|||
|
|
@ -2920,9 +2920,12 @@ class TestAuxiliaryFallbackLayering:
|
|||
)
|
||||
|
||||
assert main_chain_client.chat.completions.create.called
|
||||
# Payment errors are provider-wide, so the configured chain is
|
||||
# asked to skip the whole provider (failed_model=None), not just
|
||||
# the failed model — a sibling model can't recover from a 402.
|
||||
mock_task_chain.assert_called_once_with(
|
||||
"title_generation", "auto", reason="payment error",
|
||||
failed_model="qwen/qwen3.5-122b-a10b")
|
||||
failed_model=None)
|
||||
mock_main_chain.assert_called_once_with(
|
||||
"title_generation", "auto", reason="payment error")
|
||||
mock_builtin_chain.assert_not_called()
|
||||
|
|
@ -3360,6 +3363,44 @@ class TestTransientTransportRetry:
|
|||
assert primary.chat.completions.create.call_count == 1
|
||||
assert fb_client.chat.completions.create.call_count == 1
|
||||
|
||||
def test_timeout_forwards_failed_model_to_configured_chain(self):
|
||||
"""A timeout is model-specific, so call_llm must forward the failed
|
||||
model to the configured chain (failed_model=<model>, not None). This
|
||||
lets a same-provider sibling in the chain be tried instead of the
|
||||
whole provider being skipped — the exact NVIDIA NIM bug's trigger.
|
||||
"""
|
||||
class _Timeout(Exception):
|
||||
pass
|
||||
_Timeout.__name__ = "APITimeoutError"
|
||||
|
||||
primary = MagicMock()
|
||||
primary.base_url = "https://integrate.api.nvidia.com/v1"
|
||||
primary.chat.completions.create.side_effect = _Timeout("Request timed out.")
|
||||
|
||||
fb_client = MagicMock()
|
||||
fb_client.base_url = "https://integrate.api.nvidia.com/v1"
|
||||
fb_client.chat.completions.create.return_value = {"fallback": True}
|
||||
|
||||
p1, p2, p3 = self._patches(primary)
|
||||
with (
|
||||
p1, p2, p3,
|
||||
patch(
|
||||
"agent.auxiliary_client._try_configured_fallback_chain",
|
||||
return_value=(fb_client, "sibling-model", "fallback_chain[0](openrouter)"),
|
||||
) as mock_chain,
|
||||
patch(
|
||||
"agent.auxiliary_client._try_main_agent_model_fallback",
|
||||
return_value=(None, None, ""),
|
||||
),
|
||||
):
|
||||
result = call_llm(task="compression", messages=[{"role": "user", "content": "hi"}])
|
||||
assert result == {"fallback": True}
|
||||
_, kwargs = mock_chain.call_args
|
||||
assert kwargs.get("failed_model") == "some-model", (
|
||||
"A timeout is model-specific — the failed model must be forwarded "
|
||||
"so a same-provider sibling can be tried, not skipped wholesale."
|
||||
)
|
||||
|
||||
def test_non_compression_still_retries_same_provider_on_timeout(self):
|
||||
"""The timeout skip is scoped to compression only; other auxiliary
|
||||
tasks keep the single same-provider transient retry.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue