Round-4 review on PR #893 (no blockers, minor items):
- Document why Mistral's _is_transient is SDK-level only (429/5xx): a bare
connection drop the SDK surfaces as httpx/ConnectionError isn't an SDKError
and isn't retried here by design — the pod-rollover target is the gateway
(OpenAI-compatible) path, which does cover connection errors.
- Include the last error (%r) in the retry helper's "not resolved after N
attempts" error log.
- Add test_mistral_embed_batch_retries_on_5xx (batch path parity with embed()).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 review on PR #893:
- record_qdrant_operation("upsert","error") now fires only when the exhausted
retry was actually a Qdrant failure (reason=="qdrant"); an embed/connection
failure exhausts retries before Qdrant is called, so attributing it to
mcp_qdrant_operations_total{error} inflated that signal. The cause is still
captured by record_ingest_dropped.
- Add test_mistral_embed_retries_on_5xx: exercises the full Mistral retry path
(5xx SDKError then success), not just the predicate.
- Add test_generate_does_not_retry_on_bad_request: generate() fast-fails on a
permanent 4xx.
- Move astrolabe_vector_ingest_dropped_total's definition into the astrolabe_
pipeline-metrics block (was in the mcp_ section).
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>
embeddings_queries now records the embedding request's token count (the unit
upstream providers bill on) instead of an operation count, and fires on the
indexing path too. Previously only semantic search recorded it (value=1), so a
re-indexing run produced no embeddings_queries events at all — only pages_chunks.
- Provider layer: additive embed_with_usage / embed_batch_with_usage surface the
per-request token count (Mistral/OpenAI usage.total_tokens, Bedrock Titan
inputTextTokenCount, Ollama prompt_eval_count); a char-based estimate is the
fallback (Simple, and any provider/response without a token field). Gateway and
EmbeddingService forward through. The count travels as a return value / a
per-request SearchAlgorithm attribute — never on the singleton — so concurrent
indexing + search can't mis-attribute bills.
- Indexing (vector/processor.py): records embeddings_queries (value=batch tokens)
alongside the existing pages_chunks event.
- Search (server/semantic.py): value is now the query embedding's token count,
relayed from BM25HybridSearchAlgorithm via query_token_count.
The astrolabe_embeddings_queries Stripe meter (sum aggregation) now sums tokens
with no CP/Terraform change. The meter "queries"->tokens naming/unit
clarification (homelab-terraform #254) + CP rollup/portal copy is a follow-up.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- test_registry.py: stub `mistralai.client.Mistral` in
`test_registry_mistral_wins_over_ollama`, mirroring the sibling
picker test, so the test doesn't depend on the SDK accepting
arbitrary keys.
- openai.py: convert remaining f-string `logger.info(...)` calls to
lazy `%s` formatting, aligning with the pattern in mistral.py and
the repo's logging convention.
- test_mistral.py: add four tests covering the defensive RuntimeError
guards in `embed()` and `_embed_batch_request()` — empty
response.data, single null embedding, batch null embedding, and
count-mismatch.
- docs/configuration.md: add `AWS_ACCESS_KEY_ID` and
`AWS_SECRET_ACCESS_KEY` rows to the env-var reference table; they
were already mentioned in prose but missing from the table.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _retry.py: replace `assert last_error is not None` with explicit
`if last_error is None: raise RuntimeError(...)` so the original
rate-limit error is preserved under `python -O`.
- openai.py: drop the `_retry_factory` alias chain; rename the bound
decorator to `_retry_429` to match the pattern in mistral.py.
- mistral.py: comment the imports so future reviewers understand why
`from mistralai.client import …` is the canonical path on 2.x (no
top-level `__init__.py`; no `mistralai.models` subpackage either).
- docs/configuration.md: add `OPENAI_GENERATION_MODEL` and
`OLLAMA_GENERATION_MODEL` rows to the env-var reference table.
- test_mistral.py: add direct unit test for the `_is_rate_limit`
predicate (429 → True, 500 → False, missing-attr → False).
- test_registry.py: stub `mistralai.client.Mistral` in the registry
picker test, mirroring the Ollama sibling, so the test doesn't
depend on the SDK accepting arbitrary keys.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Adds a hosted Mistral embedding option (mistral-embed, 1024-dim) alongside
the existing Bedrock / OpenAI / Ollama / Simple providers. Implementation
mirrors OpenAIProvider: lazy dimension detection with a known-models lookup,
chunked batch requests, defensive index sort, and a 429-aware retry decorator.
In the same change, ProviderRegistry switches from os.getenv to the
dynaconf-backed Settings dataclass so all five providers share a single
configuration path. config.py gains the previously-uncovered Bedrock keys,
the new Mistral keys, the missing OPENAI_GENERATION_MODEL /
OLLAMA_GENERATION_MODEL, and SIMPLE_EMBEDDING_DIMENSION.
Auto-detection priority: Bedrock → OpenAI → Mistral → Ollama → Simple.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
