admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(providers): address PR #772 review — shared retry, cleaner imports, no-op close

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-08 17:46:32 +02:00
co-authored by Claude Opus 4.7
parent cf59663d9e
commit e360a7782b
6 changed files with 215 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/config.py
+5 -1
View File
@@ -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:
nextcloud_mcp_server/providers/_retry.py
+77
View File
@@ -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
nextcloud_mcp_server/providers/mistral.py
+25 -61
View File
@@ -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
nextcloud_mcp_server/providers/openai.py
+4 -33
View File
@@ -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
tests/unit/providers/test_mistral.py
+1 -1
View File
@@ -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
tests/unit/providers/test_retry.py
+103
View File
@@ -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()