Round-2 review on PR #893:
- Add test_generate_retries_on_connection_error (generate() shares the transient
retry; guards the decorator against accidental removal).
- Add test_process_document_records_drop_on_exhausted_retries: drives
process_document to retry-exhaustion and asserts record_ingest_dropped is
called once with the classified reason (processor-level coverage, not just the
_drop_reason unit).
- Note in _drop_reason that a multi-failure group is labelled by its first leaf
(best-effort, no "mixed" bucket).
SonarCloud: the quality gate was failing on new_security_hotspots_reviewed
(S5332 "use https") from http:// URLs in the test _req() helpers — switched to
https:// (mirrors commit 98c9d58e).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
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>
