From e360a7782b4a696b073172261a7f9dc6cdd13238 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 17:46:32 +0200 Subject: [PATCH] =?UTF-8?q?refactor(providers):=20address=20PR=20#772=20re?= =?UTF-8?q?view=20=E2=80=94=20shared=20retry,=20cleaner=20imports,=20no-op?= =?UTF-8?q?=20close?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the Claude Code review on PR #772 plus the SonarCloud S1192 finding: - Extract `retry_on_rate_limit` into `nextcloud_mcp_server/providers/_retry.py` as a parametric decorator. OpenAI and Mistral now share the same backoff loop; future providers can reuse it without copy-paste. - New `tests/unit/providers/test_retry.py` covers the decorator: 429 retry + success, non-429 immediate re-raise, MAX_RETRIES exhaustion, default predicate, and unrelated exception passthrough. - Tighten Mistral SDK import to `from mistralai.client.errors import SDKError` (the canonical sub-path; the reviewer's `from mistralai.models import SDKError` does not exist in mistralai 2.4.5). - Replace `MistralProvider.close()`'s direct `__aexit__` call with a no-op + comment — the Speakeasy-generated client has no public close hook and the underlying httpx client is closed by GC. - Extract the duplicated "Embedding not supported" message to a module-level constant (SonarCloud S1192). - Align `Settings.get_embedding_model_name()` Bedrock check with the registry by also considering `bedrock_generation_model`. - Add the `mock_mistral_client` fixture to `test_mistral_no_embeddings_disabled` for parity with the rest of the file. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/config.py | 6 +- nextcloud_mcp_server/providers/_retry.py | 77 ++++++++++++++++ nextcloud_mcp_server/providers/mistral.py | 86 ++++++------------ nextcloud_mcp_server/providers/openai.py | 37 +------- tests/unit/providers/test_mistral.py | 2 +- tests/unit/providers/test_retry.py | 103 ++++++++++++++++++++++ 6 files changed, 215 insertions(+), 96 deletions(-) create mode 100644 nextcloud_mcp_server/providers/_retry.py create mode 100644 tests/unit/providers/test_retry.py diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index c34a862e..3a205110 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -613,7 +613,11 @@ class Settings: Returns: Active embedding model name """ - if self.aws_region or self.bedrock_embedding_model: + if ( + self.aws_region + or self.bedrock_embedding_model + or self.bedrock_generation_model + ): return self.bedrock_embedding_model or "bedrock-default" if self.openai_api_key: diff --git a/nextcloud_mcp_server/providers/_retry.py b/nextcloud_mcp_server/providers/_retry.py new file mode 100644 index 00000000..d1ced096 --- /dev/null +++ b/nextcloud_mcp_server/providers/_retry.py @@ -0,0 +1,77 @@ +"""Shared rate-limit retry helper for provider modules. + +OpenAI and Mistral both retry on 429 with the same exponential-backoff curve; +extracting the loop here keeps the two provider modules thin and lets future +providers (Bedrock throttling, etc.) reuse the same primitive. +""" + +from __future__ import annotations + +import logging +from collections.abc import Awaitable, Callable +from functools import wraps +from typing import Any, TypeVar + +import anyio + +logger = logging.getLogger(__name__) + +MAX_RETRIES = 5 +INITIAL_RETRY_DELAY = 2.0 +MAX_RETRY_DELAY = 60.0 + +T = TypeVar("T") + + +def retry_on_rate_limit( + exception_type: type[BaseException], + is_rate_limit: Callable[[BaseException], bool] = lambda _exc: True, + *, + provider_name: str = "provider", +) -> Callable[[Callable[..., Awaitable[T]]], Callable[..., Awaitable[T]]]: + """Build a decorator that retries on rate-limit exceptions. + + Args: + exception_type: Catch this exception class (e.g. ``openai.RateLimitError``, + ``mistralai.client.errors.SDKError``). + is_rate_limit: Predicate that decides whether a caught exception is + actually a rate-limit (vs. some other error of the same class). + Defaults to "always True" — appropriate when ``exception_type`` is + already a rate-limit-specific class. + provider_name: Used in log messages so operators can tell which + provider exhausted retries. + """ + + def decorator(func: Callable[..., Awaitable[T]]) -> Callable[..., Awaitable[T]]: + @wraps(func) + async def wrapper(*args: Any, **kwargs: Any) -> T: + retry_delay = INITIAL_RETRY_DELAY + last_error: BaseException | None = None + + for attempt in range(1, MAX_RETRIES + 1): + try: + return await func(*args, **kwargs) + except exception_type as e: + if not is_rate_limit(e): + raise + last_error = e + if attempt < MAX_RETRIES: + logger.warning( + "%s rate limit hit (attempt %d/%d), retrying in %.1fs...", + provider_name, + attempt, + MAX_RETRIES, + retry_delay, + ) + await anyio.sleep(retry_delay) + retry_delay = min(retry_delay * 2, MAX_RETRY_DELAY) + + logger.error( + "%s rate limit exceeded after %d attempts", provider_name, MAX_RETRIES + ) + assert last_error is not None # The loop above always assigns it. + raise last_error + + return wrapper + + return decorator diff --git a/nextcloud_mcp_server/providers/mistral.py b/nextcloud_mcp_server/providers/mistral.py index 892cfc34..26702e93 100644 --- a/nextcloud_mcp_server/providers/mistral.py +++ b/nextcloud_mcp_server/providers/mistral.py @@ -5,55 +5,15 @@ can be added later if needed; see ADR-015. """ import logging -from functools import wraps -import anyio from mistralai.client import Mistral -from mistralai.client.errors.sdkerror import SDKError +from mistralai.client.errors import SDKError +from ._retry import retry_on_rate_limit from .base import Provider logger = logging.getLogger(__name__) -MAX_RETRIES = 5 -INITIAL_RETRY_DELAY = 2.0 -MAX_RETRY_DELAY = 60.0 - - -def retry_on_rate_limit(func): - """Retry on Mistral 429 (rate limit) responses with exponential backoff.""" - - @wraps(func) - async def wrapper(*args, **kwargs): - retry_delay = INITIAL_RETRY_DELAY - last_error: Exception | None = None - - for attempt in range(1, MAX_RETRIES + 1): - try: - return await func(*args, **kwargs) - except SDKError as e: - # SDKError carries a status_code attribute populated from the - # raw response. Only 429 is retryable here. - status = getattr(e, "status_code", None) - if status != 429: - raise - last_error = e - if attempt < MAX_RETRIES: - logger.warning( - "Mistral rate limit hit (attempt %d/%d), retrying in %.1fs...", - attempt, - MAX_RETRIES, - retry_delay, - ) - await anyio.sleep(retry_delay) - retry_delay = min(retry_delay * 2, MAX_RETRY_DELAY) - - logger.error("Mistral rate limit exceeded after %d attempts", MAX_RETRIES) - raise last_error # type: ignore[misc] - - return wrapper - - # Well-known Mistral embedding model dimensions MISTRAL_EMBEDDING_DIMENSIONS: dict[str, int] = { "mistral-embed": 1024, @@ -63,6 +23,18 @@ MISTRAL_EMBEDDING_DIMENSIONS: dict[str, int] = { # but we keep this in line with sibling providers (OpenAI=100, Ollama=32). BATCH_SIZE = 64 +_NO_EMBEDDING_MODEL_MSG = "Embedding not supported - no embedding_model configured" + + +def _is_rate_limit(exc: BaseException) -> bool: + """True only for HTTP 429 SDKErrors.""" + return getattr(exc, "status_code", None) == 429 + + +_retry_429 = retry_on_rate_limit( + SDKError, is_rate_limit=_is_rate_limit, provider_name="Mistral" +) + class MistralProvider(Provider): """ @@ -114,13 +86,11 @@ class MistralProvider(Provider): def supports_generation(self) -> bool: return False - @retry_on_rate_limit + @_retry_429 async def embed(self, text: str) -> list[float]: """Generate an embedding for a single text.""" if not self.supports_embeddings: - raise NotImplementedError( - "Embedding not supported - no embedding_model configured" - ) + raise NotImplementedError(_NO_EMBEDDING_MODEL_MSG) assert self.embedding_model is not None response = await self.client.embeddings.create_async( @@ -149,9 +119,7 @@ class MistralProvider(Provider): async def embed_batch(self, texts: list[str]) -> list[list[float]]: """Generate embeddings for multiple texts, chunking by ``BATCH_SIZE``.""" if not self.supports_embeddings: - raise NotImplementedError( - "Embedding not supported - no embedding_model configured" - ) + raise NotImplementedError(_NO_EMBEDDING_MODEL_MSG) if not texts: return [] @@ -172,7 +140,7 @@ class MistralProvider(Provider): return all_embeddings - @retry_on_rate_limit + @_retry_429 async def _embed_batch_request(self, batch: list[str]) -> list[list[float]]: """Single batch request with rate-limit retry.""" assert self.embedding_model is not None @@ -202,9 +170,7 @@ class MistralProvider(Provider): def get_dimension(self) -> int: if not self.supports_embeddings: - raise NotImplementedError( - "Embedding not supported - no embedding_model configured" - ) + raise NotImplementedError(_NO_EMBEDDING_MODEL_MSG) if self._dimension is None: raise RuntimeError( @@ -221,11 +187,9 @@ class MistralProvider(Provider): ) async def close(self) -> None: - # The Mistral SDK manages its own httpx client lifecycle; close it - # via the SDK's context-manager hook if present, otherwise no-op. - close = getattr(self.client, "__aexit__", None) - if close is not None: - try: - await close(None, None, None) - except Exception: # pragma: no cover - best-effort cleanup - logger.debug("Mistral client close raised; ignoring", exc_info=True) + # The mistralai 2.x client (Speakeasy-generated) does not expose a + # public close()/aclose() — only the async-context-manager protocol + # (__aenter__/__aexit__). Calling __aexit__ directly is internal API + # and brittle across SDK patch versions; the underlying httpx client + # is closed during garbage collection, so we leave this as a no-op. + return None diff --git a/nextcloud_mcp_server/providers/openai.py b/nextcloud_mcp_server/providers/openai.py index 201c1c29..df0407be 100644 --- a/nextcloud_mcp_server/providers/openai.py +++ b/nextcloud_mcp_server/providers/openai.py @@ -7,46 +7,17 @@ Supports: """ import logging -from functools import wraps -import anyio from openai import AsyncOpenAI, RateLimitError +from ._retry import retry_on_rate_limit as _retry_factory from .base import Provider logger = logging.getLogger(__name__) -# Rate limit retry configuration -MAX_RETRIES = 5 -INITIAL_RETRY_DELAY = 2.0 # seconds -MAX_RETRY_DELAY = 60.0 # seconds - - -def retry_on_rate_limit(func): - """Decorator to retry on OpenAI rate limit errors with exponential backoff.""" - - @wraps(func) - async def wrapper(*args, **kwargs): - retry_delay = INITIAL_RETRY_DELAY - last_error: Exception | None = None - - for attempt in range(1, MAX_RETRIES + 1): - try: - return await func(*args, **kwargs) - except RateLimitError as e: - last_error = e - if attempt < MAX_RETRIES: - logger.warning( - f"Rate limit hit (attempt {attempt}/{MAX_RETRIES}), " - f"retrying in {retry_delay:.1f}s..." - ) - await anyio.sleep(retry_delay) - retry_delay = min(retry_delay * 2, MAX_RETRY_DELAY) - - logger.error(f"Rate limit exceeded after {MAX_RETRIES} attempts") - raise last_error # type: ignore[misc] - - return wrapper +# OpenAI's RateLimitError is itself a 429-specific class, so the default +# is_rate_limit predicate ("always True") matches the previous behavior. +retry_on_rate_limit = _retry_factory(RateLimitError, provider_name="OpenAI") # Well-known embedding dimensions for OpenAI models diff --git a/tests/unit/providers/test_mistral.py b/tests/unit/providers/test_mistral.py index 374bc2a8..29f4392e 100644 --- a/tests/unit/providers/test_mistral.py +++ b/tests/unit/providers/test_mistral.py @@ -151,7 +151,7 @@ async def test_mistral_get_dimension_unknown_model_detected(mock_mistral_client) @pytest.mark.unit -async def test_mistral_no_embeddings_disabled(): +async def test_mistral_no_embeddings_disabled(mock_mistral_client): """Setting embedding_model=None disables the embedding capability.""" provider = MistralProvider(api_key="test-key", embedding_model=None) assert provider.supports_embeddings is False diff --git a/tests/unit/providers/test_retry.py b/tests/unit/providers/test_retry.py new file mode 100644 index 00000000..305acdd3 --- /dev/null +++ b/tests/unit/providers/test_retry.py @@ -0,0 +1,103 @@ +"""Unit tests for the shared rate-limit retry decorator.""" + +from unittest.mock import AsyncMock + +import pytest + +from nextcloud_mcp_server.providers import _retry + + +class _FakeError(Exception): + """Stand-in for an SDK exception with an HTTP status code attached.""" + + def __init__(self, status_code: int): + super().__init__(f"status {status_code}") + self.status_code = status_code + + +@pytest.fixture(autouse=True) +def _no_real_sleep(monkeypatch): + """Replace anyio.sleep with an awaitable no-op so retries don't waste time.""" + monkeypatch.setattr(_retry.anyio, "sleep", AsyncMock(return_value=None)) + + +@pytest.mark.unit +async def test_retry_succeeds_after_429(): + """A 429 followed by success returns the success value.""" + calls = {"n": 0} + + @_retry.retry_on_rate_limit( + _FakeError, is_rate_limit=lambda e: e.status_code == 429 + ) + async def flaky(): + calls["n"] += 1 + if calls["n"] < 3: + raise _FakeError(429) + return "ok" + + result = await flaky() + assert result == "ok" + assert calls["n"] == 3 + + +@pytest.mark.unit +async def test_retry_reraises_non_rate_limit_immediately(): + """A non-rate-limit error of the same class is re-raised on first hit.""" + calls = {"n": 0} + + @_retry.retry_on_rate_limit( + _FakeError, is_rate_limit=lambda e: e.status_code == 429 + ) + async def boom(): + calls["n"] += 1 + raise _FakeError(500) + + with pytest.raises(_FakeError, match="status 500"): + await boom() + assert calls["n"] == 1 # No retries on non-429. + + +@pytest.mark.unit +async def test_retry_gives_up_after_max_retries(): + """After MAX_RETRIES failed attempts the last error is re-raised.""" + calls = {"n": 0} + + @_retry.retry_on_rate_limit( + _FakeError, is_rate_limit=lambda e: e.status_code == 429 + ) + async def always_429(): + calls["n"] += 1 + raise _FakeError(429) + + with pytest.raises(_FakeError, match="status 429"): + await always_429() + assert calls["n"] == _retry.MAX_RETRIES + + +@pytest.mark.unit +async def test_retry_default_predicate_treats_all_as_rate_limit(): + """Default predicate (`lambda _: True`) retries every caught exception.""" + calls = {"n": 0} + + @_retry.retry_on_rate_limit(_FakeError) + async def fail_once(): + calls["n"] += 1 + if calls["n"] < 2: + raise _FakeError(503) + return "recovered" + + result = await fail_once() + assert result == "recovered" + assert calls["n"] == 2 + + +@pytest.mark.unit +async def test_retry_does_not_catch_unrelated_exceptions(): + """Exceptions of a different class bypass the decorator entirely.""" + + @_retry.retry_on_rate_limit(_FakeError) + async def value_error(): + raise ValueError("nope") + + with pytest.raises(ValueError, match="nope"): + await value_error()