fix(vector): normalize doc_id to str + add Qdrant keyword payload indexes
Production was logging two cascading classes of Qdrant errors against the
welcomed-malamute deployment:
1. HTTP 400 — "Bad request: Index required but not found for \"doc_id\" of
one of the following types: [keyword]". The collection was created via
create_collection() with no payload indexes, so any FieldCondition
filter on doc_id failed at the Qdrant layer (placeholder writes/reads,
eviction, search context lookups).
2. Compounding the missing index, producers wrote a mix of int and str
doc_ids: webhook_parser stringified node_id, scanner stringified note
IDs, news IDs, and deck card IDs — but the file scanner passed the
numeric file_id through unchanged. A keyword index would not have
covered both kinds even if it had existed.
This change:
- Normalizes doc_id to str at every producer site (scanner.py:459,
DocumentTask.doc_id, indexed_*_ids reads from Qdrant).
- Tightens str|int annotations to str across placeholder.py,
eviction.py, search/verification.py, search/context.py,
SearchResult.id, and the auth/api visualization endpoints.
- Defensive str() coercion on doc_id reads in semantic.py /
bm25_hybrid.py / vector/visualization.py for the transition window
before the backfill runs.
- Adds an idempotent startup migration in get_qdrant_client():
- _ensure_keyword_payload_indexes creates KEYWORD indexes for
doc_id, user_id, and doc_type (tolerates "already exists" 400s).
- _backfill_doc_id_to_string scrolls the collection once and rewrites
int doc_ids to str. Skipped after a quick sample shows no legacy
int payloads.
- Public API preserved: SemanticSearchResult.id stays int via explicit
int(r.id) narrowing in server/semantic.py — surfaces a TypeError with
actionable context if a future doc_type ships non-numeric ids.
- Documents the startup migration in docs/configuration.md.
Tests: 11 new unit tests in tests/unit/vector/test_qdrant_client.py
covering happy path / already-exists / unrelated-400 for the index
helpers, and sample-skip / mixed-batch rewrite / payload=None edge cases
for the backfill. 889 unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
0690378915
commit
719b3b5034
@@ -18,7 +18,7 @@ logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
async def delete_document_points(
|
||||
doc_id: str | int,
|
||||
doc_id: str,
|
||||
doc_type: str,
|
||||
user_id: str,
|
||||
) -> None:
|
||||
@@ -29,7 +29,7 @@ async def delete_document_points(
|
||||
not present — Qdrant returns successfully with zero points affected.
|
||||
|
||||
Args:
|
||||
doc_id: Document ID (int for notes/files/cards/news, str otherwise)
|
||||
doc_id: Document ID (str — keyword-indexed in Qdrant payload)
|
||||
doc_type: Document type (note, file, deck_card, news_item)
|
||||
user_id: Owner of the points being evicted
|
||||
|
||||
|
||||
@@ -31,7 +31,7 @@ from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def _generate_placeholder_id(doc_type: str, doc_id: str | int) -> str:
|
||||
def _generate_placeholder_id(doc_type: str, doc_id: str) -> str:
|
||||
"""Generate deterministic UUID for placeholder point.
|
||||
|
||||
Args:
|
||||
@@ -46,7 +46,7 @@ def _generate_placeholder_id(doc_type: str, doc_id: str | int) -> str:
|
||||
|
||||
|
||||
async def write_placeholder_point(
|
||||
doc_id: str | int,
|
||||
doc_id: str,
|
||||
doc_type: str,
|
||||
user_id: str,
|
||||
modified_at: int,
|
||||
@@ -60,7 +60,7 @@ async def write_placeholder_point(
|
||||
processing completes.
|
||||
|
||||
Args:
|
||||
doc_id: Document ID (int for notes/files)
|
||||
doc_id: Document ID (always str — see DocumentTask)
|
||||
doc_type: Document type (note, file, etc.)
|
||||
user_id: User ID who owns the document
|
||||
modified_at: Document modification timestamp
|
||||
@@ -135,7 +135,7 @@ async def write_placeholder_point(
|
||||
|
||||
|
||||
async def query_document_metadata(
|
||||
doc_id: str | int,
|
||||
doc_id: str,
|
||||
doc_type: str,
|
||||
user_id: str,
|
||||
) -> dict | None:
|
||||
@@ -185,7 +185,7 @@ async def query_document_metadata(
|
||||
|
||||
|
||||
async def delete_placeholder_point(
|
||||
doc_id: str | int,
|
||||
doc_id: str,
|
||||
doc_type: str,
|
||||
user_id: str,
|
||||
) -> None:
|
||||
@@ -230,7 +230,7 @@ async def delete_placeholder_point(
|
||||
|
||||
|
||||
async def update_placeholder_status(
|
||||
doc_id: str | int,
|
||||
doc_id: str,
|
||||
doc_type: str,
|
||||
user_id: str,
|
||||
status: str,
|
||||
|
||||
@@ -3,7 +3,8 @@
|
||||
import logging
|
||||
|
||||
from qdrant_client import AsyncQdrantClient, models
|
||||
from qdrant_client.models import Distance, VectorParams
|
||||
from qdrant_client.http.exceptions import UnexpectedResponse
|
||||
from qdrant_client.models import Distance, PayloadSchemaType, VectorParams
|
||||
|
||||
from nextcloud_mcp_server.config import get_settings
|
||||
from nextcloud_mcp_server.embedding import get_embedding_service
|
||||
@@ -11,10 +12,137 @@ from nextcloud_mcp_server.embedding import get_embedding_service
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
# Payload fields filtered by exact-match in scanner/processor/placeholder/eviction.
|
||||
# Qdrant requires a payload index for any field used in a FieldCondition; without
|
||||
# one, queries fail with HTTP 400 ("Index required but not found"). All three
|
||||
# carry string values after producer normalization, so a KEYWORD index is the
|
||||
# correct schema (see ADR notes in commit message).
|
||||
_KEYWORD_PAYLOAD_FIELDS: tuple[str, ...] = ("doc_id", "user_id", "doc_type")
|
||||
|
||||
# Singleton instance
|
||||
_qdrant_client: AsyncQdrantClient | None = None
|
||||
|
||||
|
||||
async def _ensure_keyword_payload_indexes(
|
||||
client: AsyncQdrantClient, collection_name: str
|
||||
) -> None:
|
||||
"""Create KEYWORD payload indexes for fields used in exact-match filters.
|
||||
|
||||
Idempotent: tolerates 'already exists' errors so it can run on every
|
||||
startup against existing collections.
|
||||
"""
|
||||
for field in _KEYWORD_PAYLOAD_FIELDS:
|
||||
try:
|
||||
await client.create_payload_index(
|
||||
collection_name=collection_name,
|
||||
field_name=field,
|
||||
field_schema=PayloadSchemaType.KEYWORD,
|
||||
wait=True,
|
||||
)
|
||||
logger.info("Created KEYWORD payload index on '%s'", field)
|
||||
except UnexpectedResponse as e:
|
||||
# Qdrant returns 400 if the index already exists with a different
|
||||
# schema, or simply succeeds if it already matches. Treat
|
||||
# already-exists as benign; surface schema conflicts loudly.
|
||||
body = getattr(e, "content", b"") or b""
|
||||
body_text = body.decode("utf-8", errors="replace")
|
||||
if "already exists" in body_text.lower():
|
||||
logger.debug("Payload index on '%s' already exists", field)
|
||||
else:
|
||||
logger.warning(
|
||||
"Failed to create payload index on '%s': %s", field, body_text
|
||||
)
|
||||
|
||||
|
||||
async def _has_int_doc_id_sample(
|
||||
client: AsyncQdrantClient, collection_name: str, sample_size: int = 256
|
||||
) -> bool:
|
||||
"""Quick sample to decide whether the full backfill scroll is needed.
|
||||
|
||||
Reading the first batch is cheap; if all sampled doc_ids are already str
|
||||
(the steady-state on healthy collections), we skip the full pass.
|
||||
"""
|
||||
points, _ = await client.scroll(
|
||||
collection_name=collection_name,
|
||||
limit=sample_size,
|
||||
with_payload=["doc_id"],
|
||||
with_vectors=False,
|
||||
)
|
||||
for point in points:
|
||||
payload = point.payload or {}
|
||||
value = payload.get("doc_id")
|
||||
if value is not None and not isinstance(value, str):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
async def _backfill_doc_id_to_string(
|
||||
client: AsyncQdrantClient, collection_name: str
|
||||
) -> None:
|
||||
"""Rewrite legacy integer doc_id payloads to strings.
|
||||
|
||||
Producers now uniformly write str(doc_id), but historical points may carry
|
||||
int values from before normalization. A KEYWORD index does not match int
|
||||
payloads, so any leftover int doc_ids would be silently invisible to
|
||||
filters. Scroll all points and convert in-place. Idempotent.
|
||||
|
||||
Skipped when the first sample batch already contains only str doc_ids.
|
||||
"""
|
||||
if not await _has_int_doc_id_sample(client, collection_name):
|
||||
logger.debug(
|
||||
"doc_id backfill: sample shows no legacy int payloads; skipping full scan"
|
||||
)
|
||||
return
|
||||
|
||||
logger.info(
|
||||
"Running doc_id backfill on '%s' (this may take a moment for large collections)",
|
||||
collection_name,
|
||||
)
|
||||
|
||||
rewritten = 0
|
||||
scanned = 0
|
||||
# Qdrant scroll returns next_offset as PointId | None — keep it untyped here
|
||||
# so the qdrant client's full union (UUID/int/str/PointId) flows through.
|
||||
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
|
||||
|
||||
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
|
||||
await client.set_payload(
|
||||
collection_name=collection_name,
|
||||
payload={"doc_id": str(value)},
|
||||
points=[point.id],
|
||||
wait=False,
|
||||
)
|
||||
rewritten += 1
|
||||
|
||||
if next_offset is None:
|
||||
break
|
||||
|
||||
logger.info(
|
||||
"doc_id backfill complete: rewrote %d/%d payloads from int to str",
|
||||
rewritten,
|
||||
scanned,
|
||||
)
|
||||
|
||||
|
||||
async def get_qdrant_client() -> AsyncQdrantClient:
|
||||
"""
|
||||
Get singleton Qdrant client instance.
|
||||
@@ -110,6 +238,12 @@ async def get_qdrant_client() -> AsyncQdrantClient:
|
||||
f"(dimension={actual_dimension}, model={settings.get_embedding_model_name()})"
|
||||
)
|
||||
|
||||
# Existing collections may pre-date the doc_id normalization /
|
||||
# payload-index work. Backfill before creating the index so the
|
||||
# index covers every point.
|
||||
await _backfill_doc_id_to_string(_qdrant_client, collection_name)
|
||||
await _ensure_keyword_payload_indexes(_qdrant_client, collection_name)
|
||||
|
||||
else:
|
||||
# Collection doesn't exist - create it
|
||||
embedding_model = settings.get_embedding_model_name()
|
||||
@@ -141,5 +275,6 @@ async def get_qdrant_client() -> AsyncQdrantClient:
|
||||
f" Distance: COSINE\n"
|
||||
f"Background sync will index all documents with dense + sparse vectors."
|
||||
)
|
||||
await _ensure_keyword_payload_indexes(_qdrant_client, collection_name)
|
||||
|
||||
return _qdrant_client
|
||||
|
||||
@@ -48,7 +48,7 @@ class DocumentTask:
|
||||
"""Document task for processing queue."""
|
||||
|
||||
user_id: str
|
||||
doc_id: int | str # int for files/notes, str for legacy
|
||||
doc_id: str # Always str — see vector/qdrant_client.py keyword index
|
||||
doc_type: str # "note", "file", "calendar"
|
||||
operation: str # "index" or "delete"
|
||||
modified_at: int
|
||||
@@ -228,7 +228,7 @@ async def scan_user_documents(
|
||||
)
|
||||
|
||||
indexed_doc_ids = {
|
||||
point.payload["doc_id"]
|
||||
str(point.payload["doc_id"])
|
||||
for point in (scroll_result[0] or [])
|
||||
if point.payload is not None
|
||||
}
|
||||
@@ -401,7 +401,7 @@ async def scan_user_documents(
|
||||
)
|
||||
|
||||
indexed_file_ids = {
|
||||
point.payload["doc_id"]
|
||||
str(point.payload["doc_id"])
|
||||
for point in (file_scroll_result[0] or [])
|
||||
if point.payload is not None
|
||||
}
|
||||
@@ -456,7 +456,9 @@ async def scan_user_documents(
|
||||
for file_info in tagged_files:
|
||||
# Files are already filtered by MIME type in find_files_by_tag()
|
||||
file_count += 1
|
||||
file_id = file_info["id"] # Use numeric file ID, not path
|
||||
# Normalize file ID to str — Qdrant doc_id payload is keyword-indexed
|
||||
# and producers across doc_types must agree on a single type.
|
||||
file_id = str(file_info["id"])
|
||||
file_path = file_info["path"] # Keep path for logging
|
||||
nextcloud_file_ids.add(file_id)
|
||||
|
||||
@@ -679,7 +681,7 @@ async def scan_news_items(
|
||||
limit=10000,
|
||||
)
|
||||
indexed_item_ids = {
|
||||
point.payload["doc_id"]
|
||||
str(point.payload["doc_id"])
|
||||
for point in (scroll_result[0] or [])
|
||||
if point.payload is not None
|
||||
}
|
||||
@@ -858,7 +860,7 @@ async def scan_deck_cards(
|
||||
limit=10000,
|
||||
)
|
||||
indexed_card_ids = {
|
||||
point.payload["doc_id"]
|
||||
str(point.payload["doc_id"])
|
||||
for point in (scroll_result[0] or [])
|
||||
if point.payload is not None
|
||||
}
|
||||
|
||||
@@ -70,7 +70,10 @@ async def compute_pca_coordinates(
|
||||
vector = point.vector
|
||||
|
||||
if vector is not None and point.payload:
|
||||
doc_id = point.payload.get("doc_id")
|
||||
# SearchResult.id is str; coerce payload doc_id to match so the
|
||||
# tuple lookup below succeeds even on legacy int-typed payloads.
|
||||
raw_doc_id = point.payload.get("doc_id")
|
||||
doc_id = None if raw_doc_id is None else str(raw_doc_id)
|
||||
chunk_start = point.payload.get("chunk_start_offset")
|
||||
chunk_end = point.payload.get("chunk_end_offset")
|
||||
chunk_key = (doc_id, chunk_start, chunk_end)
|
||||
|
||||
Reference in New Issue
Block a user