From b97ac23228418c68dfef91150817ad1e60c48e3f Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 23:39:37 +0200 Subject: [PATCH] =?UTF-8?q?fix(vector):=20address=20PR=20review=20round=20?= =?UTF-8?q?4=20=E2=80=94=20backfill=20resilience=20+=20degraded-mode=20doc?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove three stale `# Use numeric file ID` / `# Pass file path` comments in scanner.py. file_id is already normalized to str() above each call site, so the inline comments mislead readers. - Wrap `_backfill_doc_id_to_string` scroll loop + sentinel upsert in try/except Exception. The qdrant_client singleton is assigned before this migration runs, so a transient scroll failure was leaving the process holding a usable client with int payloads permanently unbackfilled until the next restart. Catch broadly, log ERROR with exc_info, and return without writing the sentinel — next process restart retries from scratch. - Note `:memory:` mode behavior near the sentinel constants so future readers don't read the every-start scroll as a bug. - Document the two degraded-migration ERROR log signals in docs/configuration.md so operators know when a clean restart is required to recover indexing. - Add unit test asserting scroll-time exceptions are logged and swallowed without writing the sentinel. Closes round-4 review feedback on PR #773. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/configuration.md | 17 +++ nextcloud_mcp_server/vector/qdrant_client.py | 143 +++++++++++-------- nextcloud_mcp_server/vector/scanner.py | 10 +- tests/unit/vector/test_qdrant_client.py | 31 ++++ 4 files changed, 133 insertions(+), 68 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index bd7dd87f..8ca27842 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -355,6 +355,23 @@ Both steps emit INFO-level log lines so operators can track progress. > caught early. Either widen the public model's `id` field or convert the > id at the verifier layer. +> **Degraded-migration signals:** both startup steps swallow non-fatal +> failures so the server still starts, but each leaves a distinct ERROR +> log line that operators should treat as a "restart needed" signal: +> +> - `Unexpected error creating payload index on '' (status 5xx)` — +> the index was not created. Searches filtering on that field will keep +> returning HTTP 400 (`Index required but not found`) until a subsequent +> restart succeeds in creating it. +> - `doc_id backfill failed on ''; will retry on next restart` — +> the migration sentinel was not written. Legacy integer `doc_id` +> payloads remain invisible to the keyword index in the meantime; the +> scroll re-runs from scratch on the next process start. +> +> Neither prevents the server from accepting requests, but both indicate +> that vector search is operating in a degraded state on the affected +> collection until the next clean restart. + #### Explicit Override Set `QDRANT_COLLECTION` to use a specific collection name: diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index bf676923..6209bc42 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -29,7 +29,9 @@ _KEYWORD_PAYLOAD_FIELDS: tuple[str, ...] = ("doc_id", "user_id", "doc_type") # doc_id". Written after a successful pass of _backfill_doc_id_to_string so # subsequent restarts can short-circuit the O(N) scroll. Carries no # user_id/doc_id/doc_type, so production search filters (which always -# require user_id) never see it. +# require user_id) never see it. In :memory: mode the sentinel does not +# survive a restart — the scroll runs every start, but is a no-op against +# an empty in-memory collection. _DOC_ID_BACKFILL_SENTINEL_ID: str = "00000000-0000-0000-0000-d0c1d0d1d0c1" _DOC_ID_BACKFILL_SENTINEL_PAYLOAD: dict[str, str] = {"_migration_marker": "doc_id_v1"} @@ -139,73 +141,88 @@ async def _backfill_doc_id_to_string( next_offset = None batch_size = 256 - while True: - points, next_offset = await client.scroll( - collection_name=collection_name, - limit=batch_size, - offset=next_offset, - with_payload=["doc_id"], - with_vectors=False, - ) - if not points: - break - - # Group by stringified value so points sharing a doc_id (one document - # → many chunks) collapse into a single set_payload call. Point IDs - # can be int/str/UUID, so widen the value type to satisfy the qdrant - # client's PointsSelector signature without re-spelling the union. - by_value: dict[str, list[Any]] = {} - for point in points: - scanned += 1 - # Qdrant client typing allows None payload even when with_payload - # was requested; defensive default so the type checker is happy. - payload = point.payload or {} - value = payload.get("doc_id") - if value is None or isinstance(value, str): - continue - by_value.setdefault(str(value), []).append(point.id) - - for str_val, point_ids in by_value.items(): - # wait=True is required because _ensure_keyword_payload_indexes - # runs immediately after this function (see get_qdrant_client - # near the call site) and only indexes committed data — - # fire-and-forget writes would leave int payloads invisible - # to KEYWORD filters. - await client.set_payload( + # A transient Qdrant failure mid-scroll (network blip, timeout) must not + # crash startup. The singleton in get_qdrant_client is already assigned + # by the time this runs, so re-raising here would leave the process in + # a half-initialized state where the next call returns the cached + # client and skips this migration entirely. Catch broadly, log with + # exc_info, and return without writing the sentinel — the next process + # restart will retry from scratch. + try: + while True: + points, next_offset = await client.scroll( collection_name=collection_name, - payload={"doc_id": str_val}, - points=point_ids, - wait=True, + limit=batch_size, + offset=next_offset, + with_payload=["doc_id"], + with_vectors=False, ) - rewritten += len(point_ids) + if not points: + break - if next_offset is None: - break + # Group by stringified value so points sharing a doc_id (one document + # → many chunks) collapse into a single set_payload call. Point IDs + # can be int/str/UUID, so widen the value type to satisfy the qdrant + # client's PointsSelector signature without re-spelling the union. + by_value: dict[str, list[Any]] = {} + for point in points: + scanned += 1 + # Qdrant client typing allows None payload even when with_payload + # was requested; defensive default so the type checker is happy. + payload = point.payload or {} + value = payload.get("doc_id") + if value is None or isinstance(value, str): + continue + by_value.setdefault(str(value), []).append(point.id) - # Write the sentinel after a successful scroll so a future restart can - # short-circuit. Empty sparse vector mirrors the placeholder.py - # convention (vector/placeholder.py); zero dense vector is fine - # because the sentinel never participates in a search (no user_id / - # doc_id / doc_type payload to match). - sentinel_point = PointStruct( - id=_DOC_ID_BACKFILL_SENTINEL_ID, - vector={ - "dense": [0.0] * dimension, - "sparse": models.SparseVector(indices=[], values=[]), - }, - payload=dict(_DOC_ID_BACKFILL_SENTINEL_PAYLOAD), - ) - await client.upsert( - collection_name=collection_name, - points=[sentinel_point], - wait=True, - ) + for str_val, point_ids in by_value.items(): + # wait=True is required because _ensure_keyword_payload_indexes + # runs immediately after this function (see get_qdrant_client + # near the call site) and only indexes committed data — + # fire-and-forget writes would leave int payloads invisible + # to KEYWORD filters. + await client.set_payload( + collection_name=collection_name, + payload={"doc_id": str_val}, + points=point_ids, + wait=True, + ) + rewritten += len(point_ids) - logger.info( - "doc_id backfill complete: rewrote %d/%d payloads from int to str", - rewritten, - scanned, - ) + if next_offset is None: + break + + # Write the sentinel after a successful scroll so a future restart can + # short-circuit. Empty sparse vector mirrors the placeholder.py + # convention (vector/placeholder.py); zero dense vector is fine + # because the sentinel never participates in a search (no user_id / + # doc_id / doc_type payload to match). + sentinel_point = PointStruct( + id=_DOC_ID_BACKFILL_SENTINEL_ID, + vector={ + "dense": [0.0] * dimension, + "sparse": models.SparseVector(indices=[], values=[]), + }, + payload=dict(_DOC_ID_BACKFILL_SENTINEL_PAYLOAD), + ) + await client.upsert( + collection_name=collection_name, + points=[sentinel_point], + wait=True, + ) + + logger.info( + "doc_id backfill complete: rewrote %d/%d payloads from int to str", + rewritten, + scanned, + ) + except Exception: + logger.error( + "doc_id backfill failed on '%s'; will retry on next restart", + collection_name, + exc_info=True, + ) + return async def get_qdrant_client() -> AsyncQdrantClient: diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 6a5a3758..c483019b 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -484,11 +484,11 @@ async def scan_user_documents( await send_stream.send( DocumentTask( user_id=user_id, - doc_id=file_id, # Use numeric file ID + doc_id=file_id, doc_type="file", operation="index", modified_at=modified_at, - file_path=file_path, # Pass file path for content retrieval + file_path=file_path, ) ) file_queued += 1 @@ -547,11 +547,11 @@ async def scan_user_documents( await send_stream.send( DocumentTask( user_id=user_id, - doc_id=file_id, # Use numeric file ID + doc_id=file_id, doc_type="file", operation="index", modified_at=modified_at, - file_path=file_path, # Pass file path for content retrieval + file_path=file_path, ) ) file_queued += 1 @@ -581,7 +581,7 @@ async def scan_user_documents( await send_stream.send( DocumentTask( user_id=user_id, - doc_id=file_id, # Use numeric file ID + doc_id=file_id, doc_type="file", operation="delete", modified_at=0, diff --git a/tests/unit/vector/test_qdrant_client.py b/tests/unit/vector/test_qdrant_client.py index 92314034..332d0bc6 100644 --- a/tests/unit/vector/test_qdrant_client.py +++ b/tests/unit/vector/test_qdrant_client.py @@ -387,3 +387,34 @@ async def test_backfill_handles_payload_with_explicit_none_doc_id(mocker): points=[2], wait=True, ) + + +@pytest.mark.unit +async def test_backfill_logs_and_returns_when_scroll_raises(mocker, caplog): + """A scroll-time exception is logged and swallowed; sentinel is not written. + + The singleton client in get_qdrant_client is already assigned by the + time _backfill_doc_id_to_string runs, so re-raising here would leave + the process holding a usable client with the migration silently + skipped on every subsequent call. Catching, logging, and returning + without writing the sentinel preserves retry-on-next-restart behavior. + """ + client = mocker.AsyncMock() + client.retrieve.return_value = [] # No sentinel — backfill must run + client.scroll.side_effect = RuntimeError("boom") + + with caplog.at_level("ERROR", logger="nextcloud_mcp_server.vector.qdrant_client"): + await _backfill_doc_id_to_string( + client, "test-collection", _backfill_dimension() + ) + + # No sentinel written — next process restart will retry from scratch. + client.upsert.assert_not_awaited() + client.set_payload.assert_not_awaited() + errors = [r for r in caplog.records if r.levelname == "ERROR"] + assert len(errors) == 1 + assert "doc_id backfill failed" in errors[0].getMessage() + assert "test-collection" in errors[0].getMessage() + # exc_info=True attaches the original exception to the log record. + assert errors[0].exc_info is not None + assert errors[0].exc_info[0] is RuntimeError