diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index c9baf084..6cd583a7 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -373,12 +373,15 @@ async def get_chunk_with_context( chunk_text = await _get_chunk_by_index_from_qdrant( user_id, doc_id, doc_type, chunk_index ) - # Skip the offset fallback for files when the indexed chunk_index - # lookup already ran: chunk_start/end_offset aren't indexed in Qdrant - # Cloud strict mode, so the call returns 400 and surfaces a misleading - # logger.error. The file fast-fail below correctly handles the miss - # without it. - skip_offset_lookup = chunk_index is not None and doc_type == "file" + # When chunk_index is available, treat the indexed lookup as canonical + # for every doc_type. A miss means the chunk is genuinely absent, not + # "fall back to the unindexed slow path". chunk_start/end_offset aren't + # in _PAYLOAD_INDEX_FIELDS, so the offset filter 400s in Qdrant Cloud + # strict mode and surfaces a misleading logger.error. Legacy data + # without chunk_index (pre-cbcoutinho/astrolabe#75) still hits the + # offset path and degrades to a None chunk with a WARNING; that's the + # same behavior get_chunk_bbox_and_page_from_qdrant already documents. + skip_offset_lookup = chunk_index is not None if chunk_text is None and not skip_offset_lookup: chunk_text = await _get_chunk_from_qdrant( user_id, doc_id, doc_type, chunk_start, chunk_end diff --git a/nextcloud_mcp_server/vector/qdrant_client.py b/nextcloud_mcp_server/vector/qdrant_client.py index 5162cbb4..c4876bc4 100644 --- a/nextcloud_mcp_server/vector/qdrant_client.py +++ b/nextcloud_mcp_server/vector/qdrant_client.py @@ -132,9 +132,14 @@ async def _ensure_payload_indexes( body = getattr(e, "content", b"") or b"" body_text = body.decode("utf-8", errors="replace") # 400 is the expected schema-conflict path (index already exists - # with a different type). 5xx is unexpected — keep the loop going - # so the remaining fields still get attempted, but log at error - # so operators see it. + # with a different type). Verified for Qdrant OSS, where an + # idempotent re-create against a matching schema returns 200; if + # Qdrant Cloud diverges and returns 400 for benign re-creates, + # the WARNING below will fire on every restart against an + # already-indexed collection — read the response body before + # treating that as a real schema conflict. 5xx is unexpected — + # keep the loop going so the remaining fields still get + # attempted, but log at error so operators see it. if e.status_code == 400: logger.warning( "Schema conflict on payload index '%s': %s", field, body_text @@ -534,7 +539,10 @@ async def get_qdrant_client() -> AsyncQdrantClient: # payload-index work. Backfill before creating the index so the # index covers every point. Pass the already-fetched # collection_info.payload_schema through to avoid a redundant - # get_collection round-trip on every restart. + # get_collection round-trip on every restart — safe because + # _backfill_doc_id_to_string only rewrites payload *values*, + # never schema or indexes, so the snapshot remains accurate + # across the backfill call. await _backfill_doc_id_to_string( _qdrant_client, collection_name, expected_dimension ) @@ -575,8 +583,16 @@ async def get_qdrant_client() -> AsyncQdrantClient: f" Distance: COSINE\n" f"Background sync will index all documents with dense + sparse vectors." ) - # Freshly created collection has no payload schema yet; pass {} - # explicitly to skip the otherwise-redundant get_collection call. + # Freshly created collection has no payload schema yet; pass + # {} explicitly to skip the otherwise-redundant + # get_collection call. Every field in _PAYLOAD_INDEX_FIELDS + # then goes through create_payload_index; on a brand-new + # collection none of them exist yet, so the WARNING in the + # 400-handler should *never* fire on this path. If it does + # on Qdrant Cloud first-start, that points at a + # deployment-level issue (race with a concurrent creator, + # implicit auto-indexes, etc.) worth investigating before + # suppressing. await _ensure_payload_indexes( _qdrant_client, collection_name, existing_schema={} ) diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 6322411e..2dccdb3f 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -210,10 +210,16 @@ async def scan_user_documents( ) # For deletion tracking, get all doc_ids in Qdrant (for incremental sync) - # Note: We no longer bulk-query indexed_at, instead check per-document + # Note: We no longer bulk-query indexed_at, instead check per-document. + # Hoisted to function scope so the file-scroll block below doesn't + # depend on a name bound inside the notes-scroll block; future + # refactors that add an early return between the two blocks would + # otherwise hit an UnboundLocalError. get_qdrant_client is a + # singleton call, so the cost is identical. + qdrant_client = await get_qdrant_client() if not initial_sync else None indexed_doc_ids = set() if not initial_sync: - qdrant_client = await get_qdrant_client() + assert qdrant_client is not None # narrow for the type checker scroll_result = await qdrant_client.scroll( collection_name=get_settings().get_collection_name(), scroll_filter=Filter( @@ -387,6 +393,7 @@ async def scan_user_documents( # Get indexed file IDs from Qdrant (for deletion tracking) indexed_file_ids = set() if not initial_sync: + assert qdrant_client is not None # narrow for the type checker file_scroll_result = await qdrant_client.scroll( collection_name=settings.get_collection_name(), scroll_filter=Filter( diff --git a/tests/unit/test_chunk_context_offset_gate.py b/tests/unit/test_chunk_context_offset_gate.py index d46dc69d..8c7f85cc 100644 --- a/tests/unit/test_chunk_context_offset_gate.py +++ b/tests/unit/test_chunk_context_offset_gate.py @@ -28,8 +28,12 @@ def mock_nc_client() -> MagicMock: class TestOffsetFallbackGate: - """When chunk_index is provided AND doc_type=='file', the offset fallback - must be skipped — see PR #767 review (🟡 spurious Qdrant error log). + """When chunk_index is provided, the offset fallback must be skipped for + every doc_type. The original gate was file-only (PR #767 review, 🟡 + spurious Qdrant error log); PR #773 round 11 broadened it to all + doc_types because chunk_start/end_offset aren't in + ``_PAYLOAD_INDEX_FIELDS`` and 400 in Qdrant Cloud strict mode for + notes / deck cards / news items the same way they do for files. """ async def test_file_with_chunk_index_skips_offset_fallback_on_miss( @@ -64,11 +68,15 @@ class TestOffsetFallbackGate: mock_indexed.assert_awaited_once() mock_offset.assert_not_awaited() - async def test_note_with_chunk_index_still_uses_offset_fallback( + async def test_note_with_chunk_index_skips_offset_fallback_on_miss( self, mock_nc_client ): - """Notes/deck cards keep the offset fallback (cheap, useful for legacy - data): the gate is file-specific. + """Notes (and other non-file doc_types) trust the indexed + chunk_index lookup as canonical too. A miss means absent — don't + hit the unindexed offset path that 400s in Qdrant Cloud strict + mode. Legacy data without chunk_index still uses the offset path + via the chunk_index=None branch (see + test_file_without_chunk_index_uses_offset_fallback). """ with ( patch.object( @@ -81,7 +89,7 @@ class TestOffsetFallbackGate: context_module, "_get_chunk_from_qdrant", new_callable=AsyncMock, - return_value=None, + return_value="should-not-be-returned", ) as mock_offset, patch.object( context_module, @@ -102,7 +110,7 @@ class TestOffsetFallbackGate: ) mock_indexed.assert_awaited_once() - mock_offset.assert_awaited_once() + mock_offset.assert_not_awaited() async def test_file_without_chunk_index_uses_offset_fallback(self, mock_nc_client): """Files with no chunk_index supplied still use the offset path —