Fix: change model retry strategy

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):

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