diff --git a/agent/auxiliary_client.py b/agent/auxiliary_client.py index 81a59c6612..6194a82e29 100644 --- a/agent/auxiliary_client.py +++ b/agent/auxiliary_client.py @@ -5035,6 +5035,14 @@ def _is_invalid_aux_response_error(exc: Exception) -> bool: ) +# Auxiliary tasks that sit on a user-visible critical path. A same-provider +# retry after a full-budget timeout costs another whole ``timeout`` window +# before the fallback chain is reached, so these skip it and fall through +# immediately. Fast blips (a streaming-close or a 5xx) still retry, since +# those are cheap. See issue #54465 for the compression case. +_TIMEOUT_NO_RETRY_TASKS = frozenset({"compression", "vision"}) + + def _evict_cached_clients(provider: str) -> None: """Drop cached auxiliary clients for a provider so fresh creds are used.""" normalized = _normalize_aux_provider(provider) @@ -10622,14 +10630,17 @@ def _call_llm_impl( if not _is_transient_transport_error(transient_err): raise # Compression is on the critical preflight path: a user cannot - # continue or resume an oversized session until it compacts. A - # same-provider retry on a timeout means another full ``timeout``- - # long wall-clock block before the except-chain below can fall - # back — doubling the user-visible stall (issue #54465). Skip the - # same-provider retry for compression on a full-budget timeout and + # continue or resume an oversized session until it compacts. + # Vision is on the interactive path: the turn holding the image + # cannot answer, and because turns are serialised the following + # user messages stall behind it. In both cases a same-provider + # retry on a timeout means another full ``timeout``-long + # wall-clock block before the except-chain below can fall back — + # doubling the user-visible stall (issue #54465). Skip the + # same-provider retry for those tasks on a full-budget timeout and # fall straight through to provider/model fallback; fast blips (a # streaming-close or a 5xx) still retry, since those are cheap. - if task == "compression" and _is_timeout_error(transient_err): + if task in _TIMEOUT_NO_RETRY_TASKS and _is_timeout_error(transient_err): # A fast first-token fail (dead stream detected within the # 60s no-progress window, zero output seen) is cheap — take # the normal same-provider retry chain first; the provider @@ -10639,9 +10650,9 @@ def _call_llm_impl( # same provider doubles the user-visible stall (#54465). if "no-progress timeout" not in str(transient_err): logger.info( - "Auxiliary compression: timeout on the critical path; " + "Auxiliary %s: timeout on the critical path; " "skipping same-provider retry and falling back: %s", - transient_err, + task, transient_err, ) raise _max_transient_retries = _transient_retry_count() @@ -11459,14 +11470,14 @@ async def _async_call_llm_impl( except Exception as transient_err: if not _is_transient_transport_error(transient_err): raise - # See call_llm(): compression is on the critical preflight path, - # so skip the same-provider retry on a full-budget timeout and - # fall straight through to fallback (issue #54465). - if task == "compression" and _is_timeout_error(transient_err): + # See call_llm(): compression and vision both sit on a critical + # path, so skip the same-provider retry on a full-budget timeout + # and fall straight through to fallback (issue #54465). + if task in _TIMEOUT_NO_RETRY_TASKS and _is_timeout_error(transient_err): logger.info( - "Auxiliary compression (async): timeout on the critical " + "Auxiliary %s (async): timeout on the critical " "path; skipping same-provider retry and falling back: %s", - transient_err, + task, transient_err, ) raise logger.info( diff --git a/tests/agent/test_auxiliary_client.py b/tests/agent/test_auxiliary_client.py index bd970dd972..931b96c69a 100644 --- a/tests/agent/test_auxiliary_client.py +++ b/tests/agent/test_auxiliary_client.py @@ -2118,6 +2118,70 @@ class TestTransientTransportRetry: assert primary.chat.completions.create.call_count == 1 assert fb_client.chat.completions.create.call_count == 1 + def test_vision_skips_same_provider_retry_on_timeout(self): + """Vision is on the interactive critical path: the turn holding the + image cannot answer, and because turns are serialised the following + user messages stall behind it. A full-budget timeout must therefore + fall straight through to fallback rather than spending a second + ``timeout`` window on the same provider (same reasoning as #54465). + """ + class _Timeout(Exception): + pass + _Timeout.__name__ = "APITimeoutError" + + primary = MagicMock() + primary.base_url = "https://openrouter.ai/api/v1" + primary.chat.completions.create.side_effect = _Timeout("Request timed out.") + + fb_client = MagicMock() + fb_client.base_url = "https://api.openai.com/v1" + fb_client.chat.completions.create.return_value = {"fallback": True} + + p1, p2, p3 = self._patches(primary) + with ( + p1, p2, p3, + # Vision resolves its client through resolve_vision_provider_client(), + # not _get_cached_client(); the retry block under test is shared. + patch( + "agent.auxiliary_client.resolve_vision_provider_client", + return_value=("openrouter", primary, "some-model"), + ), + patch( + "agent.auxiliary_client._try_configured_fallback_chain", + return_value=(None, None, ""), + ), + patch( + "agent.auxiliary_client._try_main_agent_model_fallback", + return_value=(fb_client, "fb-model", "openai"), + ), + ): + result = call_llm(task="vision", messages=[{"role": "user", "content": "hi"}]) + assert result == {"fallback": True} + assert primary.chat.completions.create.call_count == 1 + assert fb_client.chat.completions.create.call_count == 1 + + def test_non_critical_task_still_retries_same_provider_on_timeout(self): + """The skip is scoped to critical-path tasks. Everything else keeps the + existing one-shot same-provider retry, so this is not a blanket change. + """ + class _Timeout(Exception): + pass + _Timeout.__name__ = "APITimeoutError" + + primary = MagicMock() + primary.base_url = "https://openrouter.ai/api/v1" + primary.chat.completions.create.side_effect = [ + _Timeout("Request timed out."), + {"retried": True}, + ] + + p1, p2, p3 = self._patches(primary) + with p1, p2, p3: + result = call_llm(task="title", messages=[{"role": "user", "content": "hi"}]) + assert result == {"retried": True} + # Same provider was retried once — unchanged behaviour off the critical path. + assert primary.chat.completions.create.call_count == 2 + 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=, not None). This