diff --git a/application/llm/base.py b/application/llm/base.py index f6448d50..98fb98a5 100644 --- a/application/llm/base.py +++ b/application/llm/base.py @@ -136,32 +136,22 @@ class BaseLLM(ABC): return args_dict return {k: v for k, v in args_dict.items() if v is not None} - @staticmethod - def _is_non_retriable_client_error(exc: BaseException) -> bool: - """4xx errors mean the request itself is malformed — retrying with - a different model fails identically and doubles the work. Only - transient/5xx/connection errors should trigger fallback.""" - try: - from google.genai.errors import ClientError as _GenaiClientError - - if isinstance(exc, _GenaiClientError): - return True - except ImportError: - pass - for attr in ("status_code", "code", "http_status"): - v = getattr(exc, attr, None) - if isinstance(v, int) and 400 <= v < 500: - return True - resp = getattr(exc, "response", None) - v = getattr(resp, "status_code", None) - return isinstance(v, int) and 400 <= v < 500 - def _execute_with_fallback( self, method_name: str, decorators: list, *args, **kwargs ): """ Execute method with fallback support. + Any error raised by the primary model triggers a single attempt on + the fallback model, when one is configured. There is no error + classification: 5xx/transient failures are obviously recoverable on a + different model, and for client-side (4xx) errors — including rate + limits (429) and provider-specific payload rejections — the one extra + attempt is cheap insurance, since a second provider often accepts what + the first refused. ``GeneratorExit``/cancellation are ``BaseException`` + subclasses and so bypass this handler (no fallback on client + disconnect), which is intentional. + Args: method_name: Name of the raw method ('_raw_gen' or '_raw_gen_stream') decorators: List of decorators to apply @@ -185,12 +175,6 @@ class BaseLLM(ABC): try: return decorated_method() except Exception as e: - if self._is_non_retriable_client_error(e): - logger.error( - f"Primary LLM failed with non-retriable client error; " - f"skipping fallback: {str(e)}" - ) - raise if not self.fallback_llm: logger.error(f"Primary LLM failed and no fallback configured: {str(e)}") raise @@ -210,13 +194,7 @@ class BaseLLM(ABC): try: return fallback_method(fallback, *args, **fallback_kwargs) except Exception as e2: - if self._is_non_retriable_client_error(e2): - logger.error( - f"Fallback LLM failed with non-retriable client " - f"error; giving up: {str(e2)}" - ) - else: - logger.error(f"Fallback LLM also failed; giving up: {str(e2)}") + logger.error(f"Fallback LLM also failed; giving up: {str(e2)}") raise def _stream_with_fallback( @@ -233,12 +211,6 @@ class BaseLLM(ABC): try: yield from decorated_method() except Exception as e: - if self._is_non_retriable_client_error(e): - logger.error( - f"Primary LLM failed mid-stream with non-retriable client " - f"error; skipping fallback: {str(e)}" - ) - raise if not self.fallback_llm: logger.error( f"Primary LLM failed and no fallback configured: {str(e)}" @@ -270,15 +242,9 @@ class BaseLLM(ABC): try: yield from fallback_method(fallback, *args, **fallback_kwargs) except Exception as e2: - if self._is_non_retriable_client_error(e2): - logger.error( - f"Fallback LLM failed mid-stream with non-retriable " - f"client error; giving up: {str(e2)}" - ) - else: - logger.error( - f"Fallback LLM also failed mid-stream; giving up: {str(e2)}" - ) + logger.error( + f"Fallback LLM also failed mid-stream; giving up: {str(e2)}" + ) raise def gen(self, model, messages, stream=False, tools=None, *args, **kwargs): diff --git a/tests/llm/test_base.py b/tests/llm/test_base.py index 6fbe0f96..d19a967d 100644 --- a/tests/llm/test_base.py +++ b/tests/llm/test_base.py @@ -367,54 +367,39 @@ class _ClientErrorLLM(BaseLLM): @pytest.mark.unit -class TestNonRetriableClientError: +class TestClientErrorFallback: + """Any primary error triggers a single fallback attempt. - def test_helper_detects_4xx_status_code(self): - assert BaseLLM._is_non_retriable_client_error(_StatusError(400)) - assert BaseLLM._is_non_retriable_client_error(_StatusError(404)) - assert BaseLLM._is_non_retriable_client_error(_StatusError(429)) - - def test_helper_passes_5xx_through(self): - # 5xx and connection errors should still trigger fallback. - assert not BaseLLM._is_non_retriable_client_error(_StatusError(500)) - assert not BaseLLM._is_non_retriable_client_error(_StatusError(503)) - assert not BaseLLM._is_non_retriable_client_error(RuntimeError("oops")) - - def test_helper_detects_genai_client_error(self): - try: - from google.genai.errors import ClientError - except ImportError: - pytest.skip("google-genai not installed") - # ClientError(code, response_json, response=None) - exc = ClientError(400, {"error": {"message": "bad", "code": 400}}, None) - assert BaseLLM._is_non_retriable_client_error(exc) - - def test_helper_detects_response_status_code(self): - exc = RuntimeError("wrapped") - exc.response = type("R", (), {"status_code": 401})() - assert BaseLLM._is_non_retriable_client_error(exc) + There is no error-class gate: 4xx (including 429 rate limits and + provider-specific payload rejections) and Gemini ``ClientError`` now + fall back just like 5xx/transient failures. Client disconnect + (``GeneratorExit``) is a ``BaseException`` and still bypasses fallback. + """ + # 4xx that used to be force-skipped; each should now reach the fallback. + @pytest.mark.parametrize("status_code", [400, 401, 403, 404, 413, 422, 429]) @patch("application.llm.base.gen_cache", lambda f: f) @patch("application.llm.base.gen_token_usage", lambda f: f) - def test_4xx_skips_fallback(self): + def test_4xx_now_falls_back(self, status_code): fallback = FallbackLLM(model_id="fallback-model") - llm = _ClientErrorLLM(status_code=400) + llm = _ClientErrorLLM(status_code=status_code) llm._fallback_llm = fallback - with pytest.raises(_StatusError): - llm.gen(model="m", messages=[]) - assert not fallback.gen_called + result = llm.gen(model="m", messages=[]) + assert result == "fallback_result" + assert fallback.gen_called + @pytest.mark.parametrize("status_code", [400, 429]) @patch("application.llm.base.stream_cache", lambda f: f) @patch("application.llm.base.stream_token_usage", lambda f: f) - def test_4xx_skips_stream_fallback(self): + def test_4xx_now_falls_back_stream(self, status_code): fallback = FallbackLLM(model_id="fallback-model") - llm = _ClientErrorLLM(status_code=400) + llm = _ClientErrorLLM(status_code=status_code) llm._fallback_llm = fallback - with pytest.raises(_StatusError): - list(llm.gen_stream(model="m", messages=[])) - assert not fallback.gen_stream_called + result = list(llm.gen_stream(model="m", messages=[])) + assert "fallback_chunk" in result + assert fallback.gen_stream_called @patch("application.llm.base.gen_cache", lambda f: f) @patch("application.llm.base.gen_token_usage", lambda f: f) @@ -427,6 +412,50 @@ class TestNonRetriableClientError: assert result == "fallback_result" assert fallback.gen_called + @patch("application.llm.base.gen_cache", lambda f: f) + @patch("application.llm.base.gen_token_usage", lambda f: f) + def test_genai_client_error_now_falls_back(self): + """A Gemini ClientError (4xx) is no longer force-skipped.""" + try: + from google.genai.errors import ClientError + except ImportError: + pytest.skip("google-genai not installed") + + class _GenaiErrorLLM(BaseLLM): + def _raw_gen(self, baseself, model, messages, stream=False, tools=None, **kw): + raise ClientError(400, {"error": {"message": "bad", "code": 400}}, None) + + def _raw_gen_stream(self, baseself, model, messages, stream=True, tools=None, **kw): + raise ClientError(400, {"error": {"message": "bad", "code": 400}}, None) + yield # unreachable; makes this a generator + + fallback = FallbackLLM(model_id="fallback-model") + llm = _GenaiErrorLLM() + llm._fallback_llm = fallback + + result = llm.gen(model="m", messages=[]) + assert result == "fallback_result" + assert fallback.gen_called + + @patch("application.llm.base.gen_cache", lambda f: f) + @patch("application.llm.base.gen_token_usage", lambda f: f) + def test_fallback_failure_propagates(self): + """When the fallback also fails, its error propagates to the caller.""" + + class _FailingFallback(BaseLLM): + def _raw_gen(self, baseself, model, messages, stream=False, tools=None, **kw): + raise _StatusError(400, "fallback_failed") + + def _raw_gen_stream(self, baseself, model, messages, stream=True, tools=None, **kw): + raise _StatusError(400, "fallback_failed") + yield # unreachable; makes this a generator + + llm = _ClientErrorLLM(status_code=429) + llm._fallback_llm = _FailingFallback(model_id="fb") + + with pytest.raises(_StatusError, match="fallback_failed"): + llm.gen(model="m", messages=[]) + # --------------------------------------------------------------------------- # fallback_llm property: backup model resolution