diff --git a/nanobot/providers/fallback_provider.py b/nanobot/providers/fallback_provider.py index a425fd507..e348f28f9 100644 --- a/nanobot/providers/fallback_provider.py +++ b/nanobot/providers/fallback_provider.py @@ -4,11 +4,13 @@ from __future__ import annotations +import asyncio import time from collections.abc import Awaitable, Callable from dataclasses import replace from typing import Any +import httpx from loguru import logger from nanobot.providers.base import ( @@ -59,6 +61,7 @@ _AUTHENTICATION_ERROR_TOKENS = ( "access_denied", "account_deactivated", "organization_deactivated", + "not logged in", ) _NON_FALLBACK_ERROR_KINDS = frozenset({ "content_filter", @@ -428,7 +431,14 @@ class FallbackProvider(LLMProvider): if self._primary_available(): primary_was_attempted = True - response = await call(self._primary, kwargs) + response, primary_exception = await self._call_provider( + call, self._primary, kwargs + ) + if primary_exception is not None: + logger.warning( + "Primary model '{}' raised {} before responding", + primary_model, type(primary_exception).__name__, + ) if response.finish_reason != "error": self._primary_failures = 0 self._primary_tripped_at = None @@ -457,7 +467,7 @@ class FallbackProvider(LLMProvider): if not self._should_fallback(response): logger.warning( - "Primary model '{}' returned non-fallbackable error: {}", + "Primary model '{}' failed with non-fallbackable error: {}", primary_model, (response.content or "")[:120], ) @@ -544,7 +554,14 @@ class FallbackProvider(LLMProvider): fallback_kwargs.pop("reasoning_effort", None) else: fallback_kwargs["reasoning_effort"] = fallback.reasoning_effort - fallback_response = await call(fallback_provider, fallback_kwargs) + fallback_response, fallback_exception = await self._call_provider( + call, fallback_provider, fallback_kwargs + ) + if fallback_exception is not None: + logger.warning( + "Fallback '{}' raised {}", + fallback_model, type(fallback_exception).__name__, + ) if fallback_response.finish_reason != "error": # Do not publish a model switch merely because a fallback was @@ -593,6 +610,57 @@ class FallbackProvider(LLMProvider): error_should_retry=True, ) + @staticmethod + async def _call_provider( + call: Callable[[LLMProvider, dict[str, Any]], Awaitable[LLMResponse]], + provider: LLMProvider, + kwargs: dict[str, Any], + ) -> tuple[LLMResponse, Exception | None]: + """Turn provider exceptions into error responses without swallowing cancellation.""" + try: + return await call(provider, kwargs), None + except asyncio.CancelledError: + raise + except Exception as exc: + error_name = type(exc).__name__.lower() + error_kind: str | None = None + error_should_retry: bool | None = None + if isinstance(exc, (httpx.TimeoutException, asyncio.TimeoutError)): + error_kind = "timeout" + error_should_retry = True + elif isinstance(exc, (httpx.NetworkError, httpx.TransportError, ConnectionError)): + error_kind = "connection" + error_should_retry = True + elif any( + token in error_name + for token in ("auth", "credential", "permissiondenied", "unauthor") + ): + error_kind = "authentication" + elif "ratelimit" in error_name or "throttl" in error_name: + error_kind = "rate_limit" + error_should_retry = True + elif "server" in error_name or "internal" in error_name: + error_kind = "server_error" + error_should_retry = True + + response = getattr(exc, "response", None) + raw_status = getattr(exc, "status_code", None) + if raw_status is None and response is not None: + raw_status = getattr(response, "status_code", None) + try: + error_status_code = int(raw_status) if raw_status is not None else None + except (TypeError, ValueError): + error_status_code = None + + detail = str(exc).strip() or type(exc).__name__ + return LLMResponse( + content=f"Error calling LLM: {detail}", + finish_reason="error", + error_status_code=error_status_code, + error_kind=error_kind, + error_should_retry=error_should_retry, + ), exc + async def _notify_fallback_model(self, model: str) -> None: if self._fallback_model_observer is None: return diff --git a/tests/agent/test_runner_fallback.py b/tests/agent/test_runner_fallback.py index ee2805bec..4f850c852 100644 --- a/tests/agent/test_runner_fallback.py +++ b/tests/agent/test_runner_fallback.py @@ -2,9 +2,11 @@ from __future__ import annotations +import asyncio from typing import Any from unittest.mock import AsyncMock, MagicMock, patch +import httpx import pytest from loguru import logger @@ -347,6 +349,190 @@ class TestNoFallbackWhenPrimarySucceeds: factory.assert_not_called() +class _RaisingProvider(LLMProvider): + """Provider whose chat/chat_stream raise, like an auth/setup failure.""" + + def __init__(self, name: str = "raiser", exc: BaseException | None = None): + super().__init__() + self.name = name + self._exc = exc if exc is not None else RuntimeError("GitHub Copilot is not logged in.") + + def get_default_model(self) -> str: + return f"{self.name}/model" + + async def chat(self, **kwargs: Any) -> LLMResponse: + raise self._exc + + async def chat_stream(self, **kwargs: Any) -> LLMResponse: + raise self._exc + + +class _StreamingThenRaisingProvider(_RaisingProvider): + async def chat_stream(self, **kwargs: Any) -> LLMResponse: + on_content_delta = kwargs.get("on_content_delta") + if on_content_delta: + await on_content_delta("partial") + raise self._exc + + +class TestFallbackWhenPrimaryRaises: + @pytest.mark.parametrize( + "exc", + [TimeoutError(), httpx.ReadTimeout(""), httpx.ConnectError("")], + ids=["asyncio-timeout", "httpx-timeout", "connection"], + ) + @pytest.mark.asyncio + async def test_transient_exception_triggers_fallback(self, exc: Exception) -> None: + """Transient exceptions remain eligible even when their messages are empty.""" + primary = _RaisingProvider("primary", exc) + fallback = _FakeProvider("fallback", _make_response("fallback ok")) + factory = MagicMock(return_value=fallback) + + fb = FallbackProvider( + primary=primary, + fallback_presets=[_fallback("fallback-a")], + provider_factory=factory, + ) + + result = await fb.chat(messages=[{"role": "user", "content": "hi"}], model="primary-model") + + assert result.content == "fallback ok" + assert result.finish_reason == "stop" + factory.assert_called_once_with(_fallback("fallback-a")) + + @pytest.mark.asyncio + async def test_primary_exception_triggers_fallback(self) -> None: + """A primary whose chat() raises must not abort failover. + + GitHubCopilotProvider.chat refreshes its token before the request and + raises when not logged in; the exception must be treated as a + fallbackable primary error, not swallow the whole fallback chain. + """ + primary = _RaisingProvider("primary") + fallback = _FakeProvider("fallback", _make_response("fallback ok")) + factory = MagicMock(return_value=fallback) + + fb = FallbackProvider( + primary=primary, + fallback_presets=[_fallback("fallback-a")], + provider_factory=factory, + ) + + result = await fb.chat(messages=[{"role": "user", "content": "hi"}], model="primary-model") + assert result.content == "fallback ok" + assert result.finish_reason == "stop" + factory.assert_called_once_with(_fallback("fallback-a")) + + @pytest.mark.parametrize( + "exc", + [ + ValueError("unexpected response shape"), + PermissionError("local provider file access failed"), + ], + ids=["value-error", "local-permission-error"], + ) + @pytest.mark.asyncio + async def test_non_fallbackable_primary_exception_does_not_trigger_fallback( + self, exc: Exception, + ) -> None: + """An unrelated provider bug must not silently switch models.""" + primary = _RaisingProvider("primary", exc) + factory = MagicMock( + return_value=_FakeProvider("fallback", _make_response("fallback")) + ) + fb = FallbackProvider( + primary=primary, + fallback_presets=[_fallback("fallback-a")], + provider_factory=factory, + ) + + result = await fb.chat( + messages=[{"role": "user", "content": "hi"}], + model="primary-model", + ) + + assert result.finish_reason == "error" + assert result.content is not None + assert str(exc) in result.content + factory.assert_not_called() + + @pytest.mark.asyncio + async def test_fallback_exception_advances_to_next_fallback(self) -> None: + """A fallback whose chat() raises must advance to the next fallback.""" + primary = _FakeProvider("primary", _error_response()) + raising_fb = _RaisingProvider("fb1") + ok_fb = _FakeProvider("fb2", _make_response("second fallback ok")) + factory = MagicMock(side_effect=[raising_fb, ok_fb]) + + fb = FallbackProvider( + primary=primary, + fallback_presets=[_fallback("fallback-a"), _fallback("fallback-b")], + provider_factory=factory, + ) + + result = await fb.chat(messages=[{"role": "user", "content": "hi"}], model="primary-model") + assert result.content == "second fallback ok" + assert result.finish_reason == "stop" + + @pytest.mark.asyncio + async def test_primary_exception_stream_triggers_fallback(self) -> None: + """Streaming path: a raising primary still fails over when nothing streamed.""" + primary = _RaisingProvider("primary") + fallback = _FakeProvider("fallback", _make_response("fallback ok")) + factory = MagicMock(return_value=fallback) + + fb = FallbackProvider( + primary=primary, + fallback_presets=[_fallback("fallback-a")], + provider_factory=factory, + ) + + result = await fb.chat_stream(messages=[{"role": "user", "content": "hi"}], model="primary-model") + assert result.content == "fallback ok" + assert result.finish_reason == "stop" + + @pytest.mark.asyncio + async def test_primary_exception_after_streaming_does_not_duplicate_output(self) -> None: + """Once content was emitted, an exception must not start a replacement stream.""" + primary = _StreamingThenRaisingProvider("primary") + factory = MagicMock(return_value=_FakeProvider("fallback", _make_response("duplicate"))) + deltas: list[str] = [] + + fb = FallbackProvider( + primary=primary, + fallback_presets=[_fallback("fallback-a")], + provider_factory=factory, + ) + + async def collect_delta(text: str) -> None: + deltas.append(text) + + result = await fb.chat_stream( + messages=[{"role": "user", "content": "hi"}], + model="primary-model", + on_content_delta=collect_delta, + ) + + assert deltas == ["partial"] + assert result.finish_reason == "error" + factory.assert_not_called() + + @pytest.mark.asyncio + async def test_primary_cancellation_is_not_converted_to_failover(self) -> None: + """Task cancellation must retain asyncio cancellation semantics.""" + primary = _RaisingProvider("primary", asyncio.CancelledError()) + factory = MagicMock(return_value=_FakeProvider("fallback", _make_response("fallback"))) + fb = FallbackProvider( + primary=primary, + fallback_presets=[_fallback("fallback-a")], + provider_factory=factory, + ) + + with pytest.raises(asyncio.CancelledError): + await fb.chat(messages=[{"role": "user", "content": "hi"}]) + + factory.assert_not_called() + class TestFallbackOnPrimaryError: @pytest.mark.asyncio async def test_first_fallback_succeeds(self) -> None: