From b275e7d3aea48fc4c19c508ea13cf6a5cd87628b Mon Sep 17 00:00:00 2001 From: PRATHAMESH75 Date: Sat, 20 Jun 2026 19:54:44 +0530 Subject: [PATCH] feat: retry + timeout + uniform errors for LLM calls (closes #28) LLMClient issued a single attempt per call, so a provider 429 / timeout / connection blip propagated as a raw SDK exception. Centralize resilient outbound handling for every LLMClient-based agent (report, ontology, tools). - _request_with_retries: retry transient provider errors (RateLimitError, APITimeoutError, APIConnectionError, InternalServerError) with exponential backoff; non-transient errors (bad request, auth) raise immediately. - Per-request timeout passed to the OpenAI client (primary + fallback). - Order: primary (with retries) -> backup model (with retries) -> give up. - LLMError: a single, actionable, user-facing failure surface raised when all attempts are exhausted. Subclasses RuntimeError and embeds the original detail + model, so existing catchers and the fallback tests still work. - Config: LLM_MAX_RETRIES, LLM_RETRY_INITIAL_DELAY, LLM_RETRY_MAX_DELAY, LLM_TIMEOUT; documented in .env.example. - tests/test_llm_retry.py: retry-then-succeed, exhausted->LLMError with actionable message, non-transient not retried, transient-then-fallback, timeout class membership. Note: HTTP-side rate limiting (incoming) is already wired via flask-limiter on the routes; this covers the outbound provider direction. Stacked on #20/#15. Co-Authored-By: PRATHAMESH75 --- .env.example | 7 ++ backend/app/config.py | 8 +++ backend/app/utils/llm_client.py | 82 ++++++++++++++++++++--- backend/tests/test_llm_retry.py | 111 ++++++++++++++++++++++++++++++++ 4 files changed, 199 insertions(+), 9 deletions(-) create mode 100644 backend/tests/test_llm_retry.py diff --git a/.env.example b/.env.example index 3bd7bf1..c4fa22a 100644 --- a/.env.example +++ b/.env.example @@ -47,6 +47,13 @@ ZEP_API_KEY=your_zep_api_key_here # LLM_CACHE_DIR= # default: backend/uploads/llm_cache # LLM_CACHE_TTL=0 # seconds before an entry expires; 0 = never +# Resilience for outbound LLM calls: retry transient provider errors +# (429 / timeout / connection / 5xx) with exponential backoff, with a per-request timeout. +# LLM_MAX_RETRIES=3 +# LLM_RETRY_INITIAL_DELAY=1.0 # seconds; doubles each retry +# LLM_RETRY_MAX_DELAY=30.0 # cap on the per-retry wait +# LLM_TIMEOUT=60.0 # seconds before a single request times out + # API key that callers must supply in X-Api-Key or Authorization: Bearer headers. # Leave empty (default) for open access — suitable for localhost development. # For public / self-hosted deployments set a strong random value: diff --git a/backend/app/config.py b/backend/app/config.py index cb63ea0..1c28acd 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -64,6 +64,14 @@ class Config: ) LLM_CACHE_TTL = int(os.environ.get('LLM_CACHE_TTL', '0')) # seconds; 0 = no expiry + # Resilience for outbound LLM calls (issue #28): retry transient provider + # errors (429 / timeout / connection / 5xx) with exponential backoff, and + # bound every request with a timeout. + LLM_MAX_RETRIES = int(os.environ.get('LLM_MAX_RETRIES', '3')) + LLM_RETRY_INITIAL_DELAY = float(os.environ.get('LLM_RETRY_INITIAL_DELAY', '1.0')) + LLM_RETRY_MAX_DELAY = float(os.environ.get('LLM_RETRY_MAX_DELAY', '30.0')) + LLM_TIMEOUT = float(os.environ.get('LLM_TIMEOUT', '60.0')) + ZEP_API_KEY = os.environ.get('ZEP_API_KEY') APP_API_KEY = os.environ.get('APP_API_KEY', '').strip() diff --git a/backend/app/utils/llm_client.py b/backend/app/utils/llm_client.py index 818571f..860eae8 100644 --- a/backend/app/utils/llm_client.py +++ b/backend/app/utils/llm_client.py @@ -3,6 +3,7 @@ Unified calls using the OpenAI format """ +import time from typing import Optional, Dict, Any, List, Iterable from openai import OpenAI @@ -12,9 +13,33 @@ logger = get_logger('mirofish.llm_client') +# Provider errors worth retrying: rate limits (429), timeouts, connection +# blips, and 5xx. Imported defensively so the module still loads if the openai +# SDK surface changes. +try: + from openai import ( + RateLimitError, + APITimeoutError, + APIConnectionError, + InternalServerError, + ) + _TRANSIENT_LLM_ERRORS = ( + RateLimitError, APITimeoutError, APIConnectionError, InternalServerError, + ) +except ImportError: # pragma: no cover - depends on openai version + _TRANSIENT_LLM_ERRORS = () + + +class LLMError(RuntimeError): + """Uniform, user-facing failure surface for outbound LLM calls. + + Raised after retries (and fallback, if configured) are exhausted, so callers + handle one predictable exception instead of provider-specific SDK errors. + """ + class LLMClient: - """LLM client with optional per-task model and backup-model fallback.""" + """LLM client with per-task model, backup-model fallback, and retries.""" def __init__( self, @@ -34,7 +59,8 @@ def __init__( self.client = OpenAI( api_key=self.api_key, - base_url=self.base_url + base_url=self.base_url, + timeout=Config.LLM_TIMEOUT, ) # Optional backup model engaged only when a primary call raises. It may @@ -63,25 +89,63 @@ def _get_fallback_client(self) -> Optional[OpenAI]: if self._fallback_client is None: self._fallback_client = OpenAI( api_key=self.fallback_api_key, - base_url=self.fallback_base_url + base_url=self.fallback_base_url, + timeout=Config.LLM_TIMEOUT, ) return self._fallback_client + def _request_with_retries(self, client: OpenAI, model: str, kwargs: Dict[str, Any]) -> str: + """Call one model, retrying transient provider errors with backoff. + + Non-transient errors (bad request, auth, etc.) raise immediately — no + point retrying those. + """ + delay = Config.LLM_RETRY_INITIAL_DELAY + for attempt in range(Config.LLM_MAX_RETRIES + 1): + try: + response = client.chat.completions.create(model=model, **kwargs) + return response.choices[0].message.content + except _TRANSIENT_LLM_ERRORS as exc: + if attempt >= Config.LLM_MAX_RETRIES: + raise + wait = min(delay, Config.LLM_RETRY_MAX_DELAY) + logger.warning( + "Transient LLM error on '%s' (attempt %d/%d): %s; retrying in %.1fs", + model, attempt + 1, Config.LLM_MAX_RETRIES, exc, wait + ) + time.sleep(wait) + delay *= 2 + def _create(self, kwargs: Dict[str, Any]) -> str: - """Call the primary model, falling back to the backup model on failure.""" + """Call the primary model (with retries), then the backup, then give up. + + Always raises :class:`LLMError` on terminal failure so callers see one + clear, actionable error rather than a provider-specific SDK exception. + """ try: - response = self.client.chat.completions.create(model=self.model, **kwargs) - return response.choices[0].message.content + return self._request_with_retries(self.client, self.model, kwargs) except Exception as primary_error: fallback = self._get_fallback_client() if fallback is None: - raise + raise LLMError(self._failure_message(self.model, primary_error)) from primary_error logger.warning( "Primary model '%s' failed (%s); falling back to '%s'", self.model, primary_error, self.fallback_model ) - response = fallback.chat.completions.create(model=self.fallback_model, **kwargs) - return response.choices[0].message.content + try: + return self._request_with_retries(fallback, self.fallback_model, kwargs) + except Exception as fallback_error: + raise LLMError( + self._failure_message(self.fallback_model, fallback_error) + ) from fallback_error + + @staticmethod + def _failure_message(model: str, error: Exception) -> str: + return ( + f"The language model request failed (model '{model}'): {error}. " + "Please retry shortly; if this persists, check your LLM provider " + "status, API key, and rate limits." + ) def chat( self, diff --git a/backend/tests/test_llm_retry.py b/backend/tests/test_llm_retry.py new file mode 100644 index 0000000..1a1eea7 --- /dev/null +++ b/backend/tests/test_llm_retry.py @@ -0,0 +1,111 @@ +"""Tests for outbound LLM retry/backoff + uniform error surface (issue #28).""" + +import pytest + +import app.utils.llm_client as llm_mod +from app.config import Config +from app.utils.llm_client import LLMClient, LLMError, _TRANSIENT_LLM_ERRORS + + +class _Transient(Exception): + """Stand-in transient provider error.""" + + +class _FakeMessage: + def __init__(self, content): + self.content = content + + +class _FakeChoice: + def __init__(self, content): + self.message = _FakeMessage(content) + self.finish_reason = "stop" + + +class _FakeCompletion: + def __init__(self, content): + self.choices = [_FakeChoice(content)] + + +class _FlakyClient: + """Raises ``error`` for the first ``fail_times`` calls, then returns content.""" + + def __init__(self, fail_times, error, content="ok"): + self.fail_times = fail_times + self.error = error + self.content = content + self.attempts = 0 + + class _Completions: + def create(_self, **kwargs): + self.attempts += 1 + if self.attempts <= self.fail_times: + raise self.error + return _FakeCompletion(self.content) + + class _Chat: + completions = _Completions() + + self.chat = _Chat() + + +@pytest.fixture(autouse=True) +def _fast_retries(monkeypatch): + # Treat _Transient as retryable and make backoff instant. + monkeypatch.setattr(llm_mod, "_TRANSIENT_LLM_ERRORS", (_Transient,)) + monkeypatch.setattr(llm_mod.time, "sleep", lambda *_: None) + monkeypatch.setattr(Config, "LLM_API_KEY", "test-key") + monkeypatch.setattr(Config, "LLM_MAX_RETRIES", 3) + monkeypatch.setattr(Config, "LLM_RETRY_INITIAL_DELAY", 0.0) + + +def _client(**kwargs): + return LLMClient(api_key="test-key", model="primary-model", **kwargs) + + +def test_retries_transient_then_succeeds(): + client = _client() + flaky = _FlakyClient(fail_times=2, error=_Transient("429")) + client.client = flaky + + assert client.chat([{"role": "user", "content": "hi"}]) == "ok" + assert flaky.attempts == 3 # 2 failures + 1 success + + +def test_exhausted_retries_raise_llm_error(): + client = _client() + flaky = _FlakyClient(fail_times=99, error=_Transient("still 429")) + client.client = flaky + + with pytest.raises(LLMError) as exc: + client.chat([{"role": "user", "content": "hi"}]) + assert flaky.attempts == Config.LLM_MAX_RETRIES + 1 # initial try + retries + # Uniform, actionable message that surfaces the model and original detail. + assert "primary-model" in str(exc.value) + assert "still 429" in str(exc.value) + + +def test_non_transient_error_is_not_retried(): + client = _client() + flaky = _FlakyClient(fail_times=99, error=ValueError("bad request")) + client.client = flaky + + with pytest.raises(LLMError): + client.chat([{"role": "user", "content": "hi"}]) + assert flaky.attempts == 1 # no retries on non-transient errors + + +def test_transient_on_primary_then_fallback_succeeds(): + client = _client(fallback_model="backup-model") + client.client = _FlakyClient(fail_times=99, error=_Transient("primary 429")) + backup = _FlakyClient(fail_times=0, error=_Transient("unused"), content="from backup") + client._fallback_client = backup + + assert client.chat([{"role": "user", "content": "hi"}]) == "from backup" + assert backup.attempts == 1 + + +def test_timeout_error_class_is_transient(): + from openai import APITimeoutError + # Guards against an SDK change silently dropping timeouts from the retry set. + assert APITimeoutError in _TRANSIENT_LLM_ERRORS