From 6c99906ed4218eca9882a731355a86add1d92507 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 05:39:23 +0200 Subject: [PATCH] fix(vector): nested-group drop classification + review/Sonar fixes (#893) Round-1 review on PR #893: - _drop_reason now descends through nested ExceptionGroups to the first leaf (was single-level), so a doubly-wrapped cause isn't mislabelled "other"; added a nested-group test. Commented why both the httpx and openai isinstance branches exist (raw Nextcloud-API errors vs SDK-wrapped variants). - Documented that generate() intentionally shares the broadened transient retry (RAG sampling path), with the worst-case latency note. - Added a docstring note to process_document on how the provider-level retry (5x) layers over the outer loop (3x in-process / 1x procrastinate). - Added test_embed_batch_retries_on_connection_error for the batch path. - Renamed test_retry_reraises_non_rate_limit_immediately -> test_retry_reraises_when_predicate_returns_false (it tests the predicate, not a specific status). SonarCloud: - S5708 (BLOCKER) on the helper's dynamic `except exception_type`: the type is constrained to BaseException/tuple by the signature; suppressed with a justified NOSONAR. - S7503 (async without await) in the embed-retry test: use AsyncMock side_effect instead of a hand-rolled async function. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/providers/_retry.py | 5 ++- nextcloud_mcp_server/providers/openai.py | 4 +++ nextcloud_mcp_server/vector/processor.py | 19 ++++++++--- tests/unit/providers/test_openai.py | 42 ++++++++++++++++++------ tests/unit/providers/test_retry.py | 4 +-- tests/unit/test_processor_drop_reason.py | 9 +++++ 6 files changed, 66 insertions(+), 17 deletions(-) diff --git a/nextcloud_mcp_server/providers/_retry.py b/nextcloud_mcp_server/providers/_retry.py index fbeb9fee..e563cc3b 100644 --- a/nextcloud_mcp_server/providers/_retry.py +++ b/nextcloud_mcp_server/providers/_retry.py @@ -56,7 +56,10 @@ def retry_on_transient( for attempt in range(1, MAX_RETRIES + 1): try: return await func(*args, **kwargs) - except exception_type as e: + # exception_type is constrained by the signature to a + # BaseException subclass or a tuple of them; the dynamic catch is + # the whole point of this reusable helper. + except exception_type as e: # NOSONAR(S5708) if not should_retry(e): raise last_error = e diff --git a/nextcloud_mcp_server/providers/openai.py b/nextcloud_mcp_server/providers/openai.py index 3fb04fe3..1ed73af1 100644 --- a/nextcloud_mcp_server/providers/openai.py +++ b/nextcloud_mcp_server/providers/openai.py @@ -283,6 +283,10 @@ class OpenAIProvider(Provider): ) return self._dimension + # Transient retry intentionally covers generation too (RAG sampling path): + # a pod rollover breaks generation as readily as embedding. Worst case adds + # ~30s (5 attempts, 2s→60s backoff) to an interactive call hitting a + # sustained connection issue, which is preferable to a hard failure. @_retry_transient async def generate(self, prompt: str, max_tokens: int = 500) -> str: """ diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index c721b79e..fd2cf847 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -64,14 +64,18 @@ def _drop_reason(exc: BaseException) -> str: Distinguishes the transient backend-pod-rollover causes (connection / timeout — the ones provider-level retry should now ride through, card 309) from persistent faults, so ``astrolabe_vector_ingest_dropped_total`` is - alertable per cause. Unwraps a single ExceptionGroup leaf. Best-effort: + alertable per cause. Descends through nested ExceptionGroups to the first + leaf so a doubly-wrapped cause isn't mislabelled ``other``. Best-effort: unknown causes fall back to ``other``. """ - # An anyio task group can wrap the real cause; classify the first leaf. - if isinstance(exc, BaseExceptionGroup) and exc.exceptions: + # An anyio task group can wrap the real cause (and nest groups when sub-tasks + # use their own groups); descend to the first concrete leaf. + while isinstance(exc, BaseExceptionGroup) and exc.exceptions: exc = exc.exceptions[0] - # httpx transport errors (raised by the OpenAI/gateway client underneath). + # Raw httpx transport errors from direct Nextcloud API calls (the nc_client + # uses httpx directly); the openai checks below catch the SDK-wrapped + # variants of the same failure modes. if isinstance(exc, httpx.TimeoutException): return "timeout" if isinstance(exc, httpx.ConnectError): @@ -387,6 +391,13 @@ async def process_document( (3) suits the in-process SQLite pool, which has no durable retry. The procrastinate worker passes ``1`` so durable retry is owned by the queue (and survives worker crashes), avoiding compounding 3×N retries. + + Retry layering: the embedding provider adds its own transient retry (5 + attempts, 2s→60s backoff — card 309) *inside* each of these attempts. On the + in-process path (max_retries=3) a sustained outage therefore costs up to + 5×3=15 provider calls (~90s wall-clock) before the document is dropped and + re-picked on the next scan; the procrastinate path (max_retries=1) caps it at + one outer attempt (~30s) and defers. Don't stack a third retry layer here. """ start_time = time.time() diff --git a/tests/unit/providers/test_openai.py b/tests/unit/providers/test_openai.py index 4dfed9f2..afa3cc83 100644 --- a/tests/unit/providers/test_openai.py +++ b/tests/unit/providers/test_openai.py @@ -396,19 +396,41 @@ async def test_embed_retries_on_connection_error(mock_openai_client, monkeypatch mock_response = MagicMock() mock_response.data = [mock_embedding_data] - calls = {"n": 0} - - async def _flaky(*args, **kwargs): - calls["n"] += 1 - if calls["n"] == 1: - raise APIConnectionError(request=_req()) - return mock_response - - mock_openai_client.embeddings.create = _flaky + # First call raises a transient connection error, second succeeds. + create = AsyncMock(side_effect=[APIConnectionError(request=_req()), mock_response]) + mock_openai_client.embeddings.create = create provider = OpenAIProvider( api_key="test-key", embedding_model="text-embedding-3-small" ) result = await provider.embed("hello") assert result == [0.1, 0.2, 0.3] - assert calls["n"] == 2 # one failure, one success + assert create.await_count == 2 # one failure, one success + + +@pytest.mark.unit +async def test_embed_batch_retries_on_connection_error(mock_openai_client, monkeypatch): + """The batch path (`_embed_batch_request`) shares the transient retry too.""" + from openai import APIConnectionError + + from nextcloud_mcp_server.providers import _retry + + monkeypatch.setattr(_retry.anyio, "sleep", AsyncMock(return_value=None)) + + data = MagicMock() + data.embedding = [0.4, 0.5, 0.6] + data.index = 0 + mock_response = MagicMock() + mock_response.data = [data] + mock_response.usage.total_tokens = 7 + + create = AsyncMock(side_effect=[APIConnectionError(request=_req()), mock_response]) + mock_openai_client.embeddings.create = create + provider = OpenAIProvider( + api_key="test-key", embedding_model="text-embedding-3-small" + ) + + embeddings, tokens = await provider.embed_batch_with_usage(["text"]) + assert embeddings == [[0.4, 0.5, 0.6]] + assert tokens == 7 + assert create.await_count == 2 diff --git a/tests/unit/providers/test_retry.py b/tests/unit/providers/test_retry.py index 79f98ff0..bfa6c74d 100644 --- a/tests/unit/providers/test_retry.py +++ b/tests/unit/providers/test_retry.py @@ -39,8 +39,8 @@ async def test_retry_succeeds_after_429(): @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.""" +async def test_retry_reraises_when_predicate_returns_false(): + """An error the predicate rejects is re-raised on first hit (no retry).""" calls = {"n": 0} @_retry.retry_on_transient(_FakeError, should_retry=lambda e: e.status_code == 429) diff --git a/tests/unit/test_processor_drop_reason.py b/tests/unit/test_processor_drop_reason.py index c03ff359..75074ebe 100644 --- a/tests/unit/test_processor_drop_reason.py +++ b/tests/unit/test_processor_drop_reason.py @@ -59,6 +59,15 @@ def test_exception_group_unwraps_to_leaf(): assert processor._drop_reason(group) == "connection" +@pytest.mark.unit +def test_nested_exception_group_descends_to_leaf(): + """A doubly-wrapped group must still classify by its leaf, not 'other'.""" + nested = BaseExceptionGroup( + "outer", [BaseExceptionGroup("inner", [httpx.ReadTimeout("slow")])] + ) + assert processor._drop_reason(nested) == "timeout" + + @pytest.mark.unit def test_qdrant_namespace_classified(): from qdrant_client.http.exceptions import UnexpectedResponse