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) <noreply@anthropic.com>

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

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

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.
+    # An anyio task group can wrap the real cause (and nest groups when sub-tasks
-    if isinstance(exc, BaseExceptionGroup) and exc.exceptions:
+    # 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()
 

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}
+    # First call raises a transient connection error, second succeeds.
-
+    create = AsyncMock(side_effect=[APIConnectionError(request=_req()), mock_response])
-    async def _flaky(*args, **kwargs):
+    mock_openai_client.embeddings.create = create
-        calls["n"] += 1
-        if calls["n"] == 1:
-            raise APIConnectionError(request=_req())
-        return mock_response
-
-    mock_openai_client.embeddings.create = _flaky
     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

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():
+async def test_retry_reraises_when_predicate_returns_false():
-    """A non-rate-limit error of the same class is re-raised on first hit."""
+    """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)

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