fix(vector): retry transient embed errors so a pod rollover drops 0 docs
From card 309 (OHR-Bench smoke-test triage): during a backend-pod rollover the
embedding endpoint was briefly unreachable, and openai.APIConnectionError /
ConnectError propagated unretried (the provider only retried 429). Documents
exhausted the 3 in-process retries and were dropped for that scan cycle.
Broaden the provider-level retry to the transient set -- APIConnectionError,
APITimeoutError, 429, and 5xx -- on the existing exponential backoff (2s->60s,
5 attempts), so a few seconds of retry rides through the rollover. Permanent
4xx (auth, bad request) still re-raise immediately. Generalize the shared
_retry helper (retry_on_rate_limit -> retry_on_transient, predicate renamed to
should_retry, accurate log label) with a back-compat alias; Mistral gets 429+5xx
for parity. The production gateway path inherits this via GatewayProvider, which
delegates to the decorated OpenAIProvider methods.
Add astrolabe_vector_ingest_dropped_total{reason}, incremented when a document
exhausts retries, classified (connection|timeout|rate_limit|server|qdrant|other)
by _drop_reason so the embed-drop rate is alertable per cause. Dropped docs are
NOT marked failed, so the next full scan re-picks them (re-queue via scan loop).
Refs: Deck board 12 card 309 (AC #1 no permanently-dropped docs; embed-drop
metric for AC #5).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
457c115ef4
commit
258ee96f4c
@@ -9,7 +9,7 @@ from nextcloud_mcp_server.providers.mistral import (
|
||||
BATCH_SIZE,
|
||||
MISTRAL_EMBEDDING_DIMENSIONS,
|
||||
MistralProvider,
|
||||
_is_rate_limit,
|
||||
_is_transient,
|
||||
)
|
||||
|
||||
|
||||
@@ -318,14 +318,17 @@ async def test_mistral_embed_with_usage_single(mock_mistral_client):
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_mistral_is_rate_limit_predicate():
|
||||
"""_is_rate_limit returns True only for SDKErrors with status_code == 429."""
|
||||
def test_mistral_is_transient_predicate():
|
||||
"""_is_transient retries 429 (rate limit) and 5xx (server/transient) SDKErrors."""
|
||||
err_429 = MagicMock(spec=SDKError)
|
||||
err_429.status_code = 429
|
||||
err_500 = MagicMock(spec=SDKError)
|
||||
err_500.status_code = 500
|
||||
err_400 = MagicMock(spec=SDKError)
|
||||
err_400.status_code = 400
|
||||
|
||||
assert _is_rate_limit(err_429) is True
|
||||
assert _is_rate_limit(err_500) is False
|
||||
assert _is_transient(err_429) is True
|
||||
assert _is_transient(err_500) is True # broadened to 5xx (card 309)
|
||||
assert _is_transient(err_400) is False # permanent client error
|
||||
# ValueError has no status_code attr → getattr returns None → False.
|
||||
assert _is_rate_limit(ValueError()) is False
|
||||
assert _is_transient(ValueError()) is False
|
||||
|
||||
@@ -330,3 +330,85 @@ async def test_openai_close(mock_openai_client):
|
||||
|
||||
await provider.close()
|
||||
mock_openai_client.close.assert_called_once()
|
||||
|
||||
|
||||
# --- transient-error retry (card 309) ----------------------------------------
|
||||
|
||||
|
||||
def _req():
|
||||
import httpx
|
||||
|
||||
return httpx.Request("POST", "http://gw/v1/embeddings")
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_is_transient_classifies_retryable_errors():
|
||||
"""Connection / timeout / 429 / 5xx are transient; 4xx and others are not."""
|
||||
import httpx
|
||||
from openai import (
|
||||
APIConnectionError,
|
||||
APITimeoutError,
|
||||
BadRequestError,
|
||||
InternalServerError,
|
||||
RateLimitError,
|
||||
)
|
||||
|
||||
from nextcloud_mcp_server.providers.openai import _is_transient
|
||||
|
||||
req = _req()
|
||||
assert _is_transient(APIConnectionError(request=req)) is True
|
||||
assert _is_transient(APITimeoutError(request=req)) is True
|
||||
assert (
|
||||
_is_transient(
|
||||
RateLimitError("rl", response=httpx.Response(429, request=req), body=None)
|
||||
)
|
||||
is True
|
||||
)
|
||||
assert (
|
||||
_is_transient(
|
||||
InternalServerError(
|
||||
"boom", response=httpx.Response(500, request=req), body=None
|
||||
)
|
||||
)
|
||||
is True
|
||||
)
|
||||
# Permanent client errors must NOT be retried.
|
||||
assert (
|
||||
_is_transient(
|
||||
BadRequestError("bad", response=httpx.Response(400, request=req), body=None)
|
||||
)
|
||||
is False
|
||||
)
|
||||
assert _is_transient(ValueError("unrelated")) is False
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_embed_retries_on_connection_error(mock_openai_client, monkeypatch):
|
||||
"""A transient APIConnectionError (pod rollover) is retried, not dropped."""
|
||||
from openai import APIConnectionError
|
||||
|
||||
from nextcloud_mcp_server.providers import _retry
|
||||
|
||||
monkeypatch.setattr(_retry.anyio, "sleep", AsyncMock(return_value=None))
|
||||
|
||||
mock_embedding_data = MagicMock()
|
||||
mock_embedding_data.embedding = [0.1, 0.2, 0.3]
|
||||
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
|
||||
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
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
"""Unit tests for the shared rate-limit retry decorator."""
|
||||
"""Unit tests for the shared transient-error retry decorator."""
|
||||
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
@@ -26,9 +26,7 @@ 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
|
||||
)
|
||||
@_retry.retry_on_transient(_FakeError, should_retry=lambda e: e.status_code == 429)
|
||||
async def flaky():
|
||||
calls["n"] += 1
|
||||
if calls["n"] < 3:
|
||||
@@ -45,9 +43,7 @@ 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
|
||||
)
|
||||
@_retry.retry_on_transient(_FakeError, should_retry=lambda e: e.status_code == 429)
|
||||
async def boom():
|
||||
calls["n"] += 1
|
||||
raise _FakeError(500)
|
||||
@@ -62,9 +58,7 @@ 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
|
||||
)
|
||||
@_retry.retry_on_transient(_FakeError, should_retry=lambda e: e.status_code == 429)
|
||||
async def always_429():
|
||||
calls["n"] += 1
|
||||
raise _FakeError(429)
|
||||
@@ -79,7 +73,7 @@ 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)
|
||||
@_retry.retry_on_transient(_FakeError)
|
||||
async def fail_once():
|
||||
calls["n"] += 1
|
||||
if calls["n"] < 2:
|
||||
@@ -95,9 +89,37 @@ async def test_retry_default_predicate_treats_all_as_rate_limit():
|
||||
async def test_retry_does_not_catch_unrelated_exceptions():
|
||||
"""Exceptions of a different class bypass the decorator entirely."""
|
||||
|
||||
@_retry.retry_on_rate_limit(_FakeError)
|
||||
@_retry.retry_on_transient(_FakeError)
|
||||
async def value_error():
|
||||
raise ValueError("nope")
|
||||
|
||||
with pytest.raises(ValueError, match="nope"):
|
||||
await value_error()
|
||||
|
||||
|
||||
class _ConnError(Exception):
|
||||
pass
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_retry_accepts_tuple_of_exception_types():
|
||||
"""A tuple of exception classes is caught (the OpenAI transient set shape)."""
|
||||
calls = {"n": 0}
|
||||
|
||||
@_retry.retry_on_transient((_FakeError, _ConnError))
|
||||
async def flaky():
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 1:
|
||||
raise _ConnError("dropped")
|
||||
if calls["n"] == 2:
|
||||
raise _FakeError(503)
|
||||
return "ok"
|
||||
|
||||
assert await flaky() == "ok"
|
||||
assert calls["n"] == 3
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_retry_on_rate_limit_is_backcompat_alias():
|
||||
"""The old name still resolves to the generalized helper."""
|
||||
assert _retry.retry_on_rate_limit is _retry.retry_on_transient
|
||||
|
||||
@@ -0,0 +1,72 @@
|
||||
"""Unit tests for the embed-drop classifier (card 309).
|
||||
|
||||
``processor._drop_reason`` maps a terminal indexing failure to a metric label
|
||||
so the transient backend-pod-rollover causes (connection / timeout) are
|
||||
alertable on ``astrolabe_vector_ingest_dropped_total`` distinctly from
|
||||
persistent faults.
|
||||
"""
|
||||
|
||||
import httpx
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.vector import processor
|
||||
|
||||
|
||||
def _req() -> httpx.Request:
|
||||
return httpx.Request("POST", "http://gw/v1/embeddings")
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_httpx_connect_and_timeout_classified():
|
||||
assert processor._drop_reason(httpx.ConnectError("refused")) == "connection"
|
||||
assert processor._drop_reason(httpx.ReadTimeout("slow")) == "timeout"
|
||||
assert processor._drop_reason(httpx.ConnectTimeout("slow")) == "timeout"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_openai_errors_classified():
|
||||
from openai import (
|
||||
APIConnectionError,
|
||||
APITimeoutError,
|
||||
InternalServerError,
|
||||
RateLimitError,
|
||||
)
|
||||
|
||||
req = _req()
|
||||
assert processor._drop_reason(APIConnectionError(request=req)) == "connection"
|
||||
assert processor._drop_reason(APITimeoutError(request=req)) == "timeout"
|
||||
assert (
|
||||
processor._drop_reason(
|
||||
RateLimitError("rl", response=httpx.Response(429, request=req), body=None)
|
||||
)
|
||||
== "rate_limit"
|
||||
)
|
||||
assert (
|
||||
processor._drop_reason(
|
||||
InternalServerError(
|
||||
"boom", response=httpx.Response(503, request=req), body=None
|
||||
)
|
||||
)
|
||||
== "server"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_exception_group_unwraps_to_leaf():
|
||||
group = BaseExceptionGroup(
|
||||
"unhandled errors in a TaskGroup", [httpx.ConnectError("refused")]
|
||||
)
|
||||
assert processor._drop_reason(group) == "connection"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_qdrant_namespace_classified():
|
||||
from qdrant_client.http.exceptions import UnexpectedResponse
|
||||
|
||||
exc = UnexpectedResponse(500, "err", b"", headers=None)
|
||||
assert processor._drop_reason(exc) == "qdrant"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_unknown_error_falls_back_to_other():
|
||||
assert processor._drop_reason(ValueError("nope")) == "other"
|
||||
Reference in New Issue
Block a user