diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index eebf3a98..6da74196 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -931,6 +931,12 @@ class Settings: Returns: Active embedding model name """ + # NOTE: there is intentionally no "gateway" branch here. When + # EMBEDDING_PROVIDER=gateway this falls through to the underlying + # provider's model (used for the Qdrant collection name), whereas + # get_embedding_provider_family() short-circuits to the gateway-routed + # family. Keep that asymmetry in mind before joining metrics/labels + # derived from these two methods. if ( self.aws_region or self.bedrock_embedding_model diff --git a/nextcloud_mcp_server/document_processors/registry.py b/nextcloud_mcp_server/document_processors/registry.py index 8f434afa..545513de 100644 --- a/nextcloud_mcp_server/document_processors/registry.py +++ b/nextcloud_mcp_server/document_processors/registry.py @@ -3,7 +3,7 @@ import logging import time from collections.abc import Awaitable, Callable -from typing import Any, Optional +from typing import Any from nextcloud_mcp_server.observability.metrics import record_document_parse from nextcloud_mcp_server.observability.tracing import trace_operation @@ -72,7 +72,7 @@ class ProcessorRegistry: len(processor.supported_mime_types), ) - def get_processor(self, name: str) -> Optional[DocumentProcessor]: + def get_processor(self, name: str) -> DocumentProcessor | None: """Get a processor by name. Args: @@ -85,7 +85,7 @@ class ProcessorRegistry: return self._processors[name][0] return None - def find_processor(self, content_type: str) -> Optional[DocumentProcessor]: + def find_processor(self, content_type: str) -> DocumentProcessor | None: """Find the first processor that supports the given MIME type. Processors are checked in priority order (highest priority first). @@ -117,12 +117,12 @@ class ProcessorRegistry: self, content: bytes, content_type: str, - filename: Optional[str] = None, - processor_name: Optional[str] = None, - options: Optional[dict[str, Any]] = None, - progress_callback: Optional[ - Callable[[float, Optional[float], Optional[str]], Awaitable[None]] - ] = None, + filename: str | None = None, + processor_name: str | None = None, + options: dict[str, Any] | None = None, + progress_callback: ( + Callable[[float, float | None, str | None], Awaitable[None]] | None + ) = None, ) -> ProcessingResult: """Process a document using available processors. @@ -183,6 +183,7 @@ class ProcessorRegistry: "byte_size": byte_size, "escalated": False, }, + record_exception=True, ) as span: try: result = await processor.process( @@ -197,6 +198,22 @@ class ProcessorRegistry: byte_size=byte_size, status="error", ) + # Structured error signal for Loki (the processor logs the + # traceback; this adds the aggregatable fields). The span + # records the exception itself via record_exception=True. + logger.warning( + "Parse failed for %s with '%s' after %.2fs", + filename or "", + processor.name, + duration, + extra={ + "processor": processor.name, + "tier": tier, + "byte_size": byte_size, + "duration_ms": round(duration * 1000, 1), + "status": "error", + }, + ) raise duration = time.time() - start_time diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index dd207af0..658b785e 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -11,7 +11,6 @@ from typing import Any, cast import anyio from anyio.abc import TaskStatus from anyio.streams.memory import MemoryObjectReceiveStream -from httpx import HTTPStatusError from qdrant_client.models import FieldCondition, Filter, MatchValue, PointStruct from nextcloud_mcp_server.acl_hash import compute_acl_hash @@ -246,7 +245,7 @@ async def process_document(doc_task: DocumentTask, nc_client: NextcloudClient): ) return # Success - except (HTTPStatusError, Exception) as e: + except Exception as e: if attempt < max_retries - 1: logger.warning( "Retry %s/%s for %s_%s: %s", @@ -289,9 +288,14 @@ async def process_document(doc_task: DocumentTask, nc_client: NextcloudClient): except Exception: # Single processing-error call site: catches exhausted-retry # re-raises, delete failures, and setup errors (get_qdrant_client / - # get_settings) — each counted exactly once. + # get_settings) — each counted exactly once. A failed delete is not + # an indexing event either, so doc_type is omitted for deletes to + # keep them out of astrolabe_documents_indexed_total. duration = time.time() - start_time - record_vector_sync_processing(duration, "error", doc_type=doc_task.doc_type) + indexed_doc_type = ( + None if doc_task.operation == "delete" else doc_task.doc_type + ) + record_vector_sync_processing(duration, "error", doc_type=indexed_doc_type) raise diff --git a/tests/unit/test_document_parse_metrics.py b/tests/unit/test_document_parse_metrics.py index d1d82c96..be0cbf61 100644 --- a/tests/unit/test_document_parse_metrics.py +++ b/tests/unit/test_document_parse_metrics.py @@ -192,6 +192,14 @@ class TestParseMetricHelpers: def test_error_does_not_increment_throughput(self, metric_sample): labels = {"processor": "uttest-error", "tier": "fast"} + # Snapshot before — counters are global singletons, so assert the delta + # rather than an absolute value (consistent with the success test). + before_pages = metric_sample("astrolabe_document_pages_processed_total", labels) + before_chars = metric_sample("astrolabe_document_chars_processed_total", labels) + before_total = metric_sample( + "astrolabe_document_parse_total", {**labels, "status": "error"} + ) + record_document_parse( "uttest-error", "fast", @@ -201,16 +209,17 @@ class TestParseMetricHelpers: byte_size=10, status="error", ) + # Error parses count the attempt + duration, but NOT pages/chars/bytes. assert metric_sample( "astrolabe_document_pages_processed_total", labels - ) == pytest.approx(0.0) + ) == pytest.approx(before_pages) assert metric_sample( "astrolabe_document_chars_processed_total", labels - ) == pytest.approx(0.0) + ) == pytest.approx(before_chars) assert metric_sample( "astrolabe_document_parse_total", {**labels, "status": "error"} - ) == pytest.approx(1.0) + ) == pytest.approx(before_total + 1) def test_record_document_chunks(self, metric_sample): labels = {"doc_type": "uttest-chunks"} @@ -313,3 +322,31 @@ class TestProcessDocumentMetricCounting: assert metric_sample( "mcp_vector_sync_documents_processed_total", processed_labels ) == pytest.approx(before_processed + 1) + + async def test_failed_delete_is_processed_but_not_indexed(self, metric_sample): + # A *failed* delete also must not touch astrolabe_documents_indexed_total + # (the outer except gates doc_type on operation != "delete"). + task = DocumentTask( + user_id="u", doc_id="3", doc_type="note", operation="delete", modified_at=0 + ) + indexed_labels = {"source": "note", "status": "error"} + processed_labels = {"status": "error"} + before_indexed = metric_sample( + "astrolabe_documents_indexed_total", indexed_labels + ) + before_processed = metric_sample( + "mcp_vector_sync_documents_processed_total", processed_labels + ) + + qmock = MagicMock() + qmock.delete = AsyncMock(side_effect=RuntimeError("boom")) + with patch.object(proc, "get_qdrant_client", new=AsyncMock(return_value=qmock)): + with pytest.raises(RuntimeError): + await proc.process_document(task, MagicMock()) + + assert metric_sample( + "astrolabe_documents_indexed_total", indexed_labels + ) == pytest.approx(before_indexed) + assert metric_sample( + "mcp_vector_sync_documents_processed_total", processed_labels + ) == pytest.approx(before_processed + 1)