admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(vector): address PR review round 15 — concurrency, pagination, stale coercion

Browse Source 
Defer publication of `_qdrant_client` until after the in-lock backfill +
payload-index migration awaits complete. The fast-path check at the top of
`get_qdrant_client` reads the singleton without holding the init lock, so
publishing the constructed-but-unmigrated client let concurrent fast-path
callers fire filtered searches before `_ensure_payload_indexes` ran —
producing HTTP 400 ("Index required but not found") on Qdrant Cloud strict
mode. Local `provisional` is now used for every await inside the lock; the
global is assigned exactly once, last.

Replace the five hand-rolled `scroll(..., limit=10000)` calls in
`vector/scanner.py` (notes / files / news / deck-cards deletion tracking,
plus the timestamp scroll) with a single paginated `_scroll_all_points`
helper. The previous single-page cap silently dropped deletion-tracking
points beyond the first 10 k for any user past that threshold. Pagination
follows Qdrant's documented contract (loop until `next_page_offset is
None`) with a fixed per-page `_DELETION_TRACKING_PAGE_SIZE = 1024`.

Extract `_create_one_payload_index` from `_ensure_payload_indexes` to drop
its cognitive complexity below the SonarQube limit (17 → ≤ 15) without
losing the per-field error-containment rationale; every comment is
preserved verbatim on the helper.

Drop the stale `SearchResult.id` `int | str` comment and the redundant
`str(d)` coercion in `_verify_news_items` — the contract has been
str-only since the producer-side stringification landed earlier in this
PR.

Fix eight `doc_id=<int>` test calls in `test_chunk_context_offset_gate.py`
that violated the `doc_id: str` signature of `get_chunk_with_context`,
plus align `_make_result` in `test_verification.py` to coerce `id=str(...)`
matching the production contract — and update 30+ assertions from int
sets (`{1, 2, 3}`) to str sets (`{"1", "2", "3"}`) so the tests now model
the post-PR `SearchResult.id: str` reality end-to-end. Previously these
were masked by the `str(d)` coercion now removed from production.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-10 13:49:17 +02:00
co-authored by Claude Opus 4.7
parent c5020d9629
commit 68506f96c5
5 changed files with 220 additions and 143 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/vector/qdrant_client.py
+99 -65
View File
@@ -71,6 +71,71 @@ _qdrant_client: AsyncQdrantClient | None = None
_qdrant_init_lock: anyio.Lock | None = None
async def _create_one_payload_index(
client: AsyncQdrantClient,
collection_name: str,
field: str,
schema_type: PayloadSchemaType,
) -> bool:
"""Create one payload index with per-field error containment.
Returns True on success or benign 400 schema-conflict (caller treats as
indexed). Returns False if the field should be added to the caller's
failed-fields list. Never re-raises: the singleton in
``get_qdrant_client`` is already assigned by the time this runs, so
propagating a network blip would leave the process holding a usable
client with the migration silently incomplete.
"""
try:
await client.create_payload_index(
collection_name=collection_name,
field_name=field,
field_schema=schema_type,
wait=True,
)
logger.info("Created %s payload index on '%s'", schema_type.name, field)
return True
except UnexpectedResponse as e:
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). 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
)
return True
logger.error(
"Unexpected error creating payload index on '%s' (status %s): %s",
field,
e.status_code,
body_text,
)
return False
except Exception:
# Raw network / timeout failures (httpx.ConnectError,
# asyncio.TimeoutError, etc.) reach here — outside the HTTP-status
# taxonomy that UnexpectedResponse covers. Same containment
# rationale as above: one transient failure on one field must not
# skip the rest, and the singleton in get_qdrant_client is already
# assigned by this point so re-raising would leave the process
# holding a usable client with the migration silently incomplete.
logger.error(
"Network error creating payload index on '%s'; "
"field will remain unindexed until next successful restart",
field,
exc_info=True,
)
return False
async def _ensure_payload_indexes(
client: AsyncQdrantClient,
collection_name: str,
@@ -82,13 +147,9 @@ async def _ensure_payload_indexes(
schema type (KEYWORD for string fields, BOOL for ``is_placeholder``,
INTEGER for ``chunk_index``). Skips fields that are already in
``existing_schema`` so routine restarts make no Qdrant write round-trips
and emit no INFO log lines. Schema conflicts (a pre-existing index with
a different type) still surface as a 400 — log loudly so operators can
intervene, but keep going so the remaining fields still get indexed.
The same per-field error containment applies to raw network errors
(e.g. ``httpx.ConnectError`` from a transient Qdrant unavailability):
log at ERROR with ``exc_info`` and continue, so a single transient
failure on one field does not skip the rest.
and emit no INFO log lines. Per-field error handling (schema conflicts,
network errors) lives in ``_create_one_payload_index``; this loop is
flat so a single transient failure on one field does not skip the rest.
Args:
client: Qdrant client instance.
@@ -117,60 +178,17 @@ async def _ensure_payload_indexes(
)
return
existing_schema = collection_info.payload_schema or {}
failed_fields: list[str] = []
failed_fields: list[str] = []
for field, schema_type in _PAYLOAD_INDEX_FIELDS.items():
if field in existing_schema:
# Index already present — silent skip. Logging here on every
# restart would be noise that hides the genuinely interesting
# "first-time creation" line below.
# "first-time creation" line in _create_one_payload_index.
continue
try:
await client.create_payload_index(
collection_name=collection_name,
field_name=field,
field_schema=schema_type,
wait=True,
)
logger.info("Created %s payload index on '%s'", schema_type.name, field)
except UnexpectedResponse as e:
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). 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
)
else:
logger.error(
"Unexpected error creating payload index on '%s' (status %s): %s",
field,
e.status_code,
body_text,
)
failed_fields.append(field)
except Exception:
# Raw network / timeout failures (httpx.ConnectError,
# asyncio.TimeoutError, etc.) reach here — outside the HTTP-status
# taxonomy that UnexpectedResponse covers. Same containment
# rationale as above: one transient failure on one field must not
# skip the rest, and the singleton in get_qdrant_client is already
# assigned by this point so re-raising would leave the process
# holding a usable client with the migration silently incomplete.
logger.error(
"Network error creating payload index on '%s'; "
"field will remain unindexed until next successful restart",
field,
exc_info=True,
)
if not await _create_one_payload_index(
client, collection_name, field, schema_type
):
failed_fields.append(field)
# A single per-field ERROR line is easy to miss in startup noise. Surface
@@ -467,11 +485,21 @@ async def get_qdrant_client() -> AsyncQdrantClient:
if _qdrant_client is None:
settings = get_settings()
# Build the client into a local ``provisional`` and only publish
# it to the global ``_qdrant_client`` after the migration awaits
# below have all completed. The fast-path check at the top of
# this function reads ``_qdrant_client`` without the lock, so
# publishing the constructed-but-unmigrated client would let a
# concurrent caller short-circuit the lock and fire a filtered
# search before ``_ensure_payload_indexes`` runs — that search
# would 400 with "Index required but not found".
provisional: AsyncQdrantClient
# Detect mode and initialize client accordingly
if settings.qdrant_url:
# Network mode
logger.info(f"Using Qdrant network mode: {settings.qdrant_url}")
_qdrant_client = AsyncQdrantClient(
provisional = AsyncQdrantClient(
url=settings.qdrant_url,
api_key=settings.qdrant_api_key,
timeout=30,
@@ -480,17 +508,17 @@ async def get_qdrant_client() -> AsyncQdrantClient:
# Local mode (either :memory: or persistent path)
if settings.qdrant_location == ":memory:":
logger.info("Using Qdrant in-memory mode: :memory:")
_qdrant_client = AsyncQdrantClient(":memory:")
provisional = AsyncQdrantClient(":memory:")
else:
# Persistent local mode - use path parameter
logger.info(
f"Using Qdrant persistent mode: {settings.qdrant_location}"
)
_qdrant_client = AsyncQdrantClient(path=settings.qdrant_location)
provisional = AsyncQdrantClient(path=settings.qdrant_location)
else:
# Should not happen due to __post_init__ validation, but handle gracefully
logger.warning("No Qdrant mode configured, defaulting to :memory:")
_qdrant_client = AsyncQdrantClient(":memory:")
provisional = AsyncQdrantClient(":memory:")
# Get collection name (auto-generated from deployment ID + model)
collection_name = settings.get_collection_name()
@@ -505,7 +533,7 @@ async def get_qdrant_client() -> AsyncQdrantClient:
# Explicitly check if collection exists
logger.debug(f"Checking if collection '{collection_name}' exists...")
collections = await _qdrant_client.get_collections()
collections = await provisional.get_collections()
collection_names = [c.name for c in collections.collections]
if collection_name in collection_names:
@@ -513,7 +541,7 @@ async def get_qdrant_client() -> AsyncQdrantClient:
logger.debug(
f"Collection '{collection_name}' found, validating dimensions..."
)
collection_info = await _qdrant_client.get_collection(collection_name)
collection_info = await provisional.get_collection(collection_name)
# Handle both named vectors (dict) and legacy single vector
vectors = collection_info.config.params.vectors
if isinstance(vectors, dict):
@@ -551,10 +579,10 @@ async def get_qdrant_client() -> AsyncQdrantClient:
# 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
provisional, collection_name, expected_dimension
)
await _ensure_payload_indexes(
_qdrant_client,
provisional,
collection_name,
existing_schema=collection_info.payload_schema or {},
)
@@ -566,7 +594,7 @@ async def get_qdrant_client() -> AsyncQdrantClient:
f"Collection '{collection_name}' not found, creating with "
f"dimension={expected_dimension}, model={embedding_model}..."
)
await _qdrant_client.create_collection(
await provisional.create_collection(
collection_name=collection_name,
vectors_config={
"dense": VectorParams(
@@ -601,9 +629,15 @@ async def get_qdrant_client() -> AsyncQdrantClient:
# implicit auto-indexes, etc.) worth investigating before
# suppressing.
await _ensure_payload_indexes(
_qdrant_client, collection_name, existing_schema={}
provisional, collection_name, existing_schema={}
)
# Publish only after the migration awaits completed. From this
# point on, fast-path callers may short-circuit the lock and
# use the client; every payload index they could filter on now
# exists.
_qdrant_client = provisional
# Lock released. ``_qdrant_client`` is guaranteed non-None here:
# either the fast path returned earlier, the lock-protected branch
# set it, or a sibling waiter set it before we got the lock.
nextcloud_mcp_server/vector/scanner.py
+70 -30
View File
@@ -13,7 +13,8 @@ from email.utils import parsedate_to_datetime
import anyio
from anyio.abc import TaskStatus
from anyio.streams.memory import MemoryObjectSendStream
from qdrant_client.models import FieldCondition, Filter, MatchValue
from qdrant_client import AsyncQdrantClient
from qdrant_client.models import FieldCondition, Filter, MatchValue, Record
from nextcloud_mcp_server.client import NextcloudClient
from nextcloud_mcp_server.client.news import NewsItemType
@@ -43,6 +44,49 @@ INDEXED_DOC_TYPES: frozenset[str] = frozenset(
)
# Page size for paginated deletion-tracking scrolls. Chosen to keep per-page
# memory bounded while making the round-trip count manageable in the typical
# < 100 k point per (user_id, doc_type) case. The previous single-page
# ``limit=10_000`` silently truncated deletion sets for any user past the
# cap, so anything indexed beyond the first 10 k was never reconciled.
_DELETION_TRACKING_PAGE_SIZE: int = 1024
async def _scroll_all_points(
qdrant_client: AsyncQdrantClient,
*,
collection_name: str,
scroll_filter: Filter,
payload_fields: list[str],
page_size: int = _DELETION_TRACKING_PAGE_SIZE,
) -> list[Record]:
"""Scroll every point matching the filter, paginating until exhausted.
Replaces the prior single-page ``limit=10_000`` calls that silently
dropped points beyond the first page. Pagination follows Qdrant's
documented contract: ``scroll`` returns ``(points, next_page_offset)``
and ``next_page_offset`` is ``None`` once the cursor reaches the end.
Errors propagate to the caller — the scanner's outer ``try`` already
handles them by skipping the deletion-tracking pass for this scan
(worse: extra-scan latency; never: bad data).
"""
all_points: list[Record] = []
offset = None
while True:
points, offset = await qdrant_client.scroll(
collection_name=collection_name,
scroll_filter=scroll_filter,
with_payload=payload_fields,
with_vectors=False,
limit=page_size,
offset=offset,
)
all_points.extend(points)
if offset is None:
break
return all_points
@dataclass
class DocumentTask:
"""Document task for processing queue."""
@@ -79,8 +123,11 @@ async def get_last_indexed_timestamp(user_id: str) -> int | None:
try:
qdrant_client = await get_qdrant_client()
# Query for user's notes, ordered by indexed_at descending, limit 1
scroll_result = await qdrant_client.scroll(
# Scroll across every indexed note for this user — paginated so users
# with > 10 k indexed notes still produce a correct max (the prior
# single-page ``limit=10_000`` would have silently undercounted).
points = await _scroll_all_points(
qdrant_client,
collection_name=get_settings().get_collection_name(),
scroll_filter=Filter(
must=[
@@ -88,19 +135,16 @@ async def get_last_indexed_timestamp(user_id: str) -> int | None:
FieldCondition(key="doc_type", match=MatchValue(value="note")),
]
),
with_payload=["indexed_at"],
with_vectors=False,
limit=10000, # Get all to find max
payload_fields=["indexed_at"],
)
# Find max indexed_at across all results
num_points = len(scroll_result[0]) if scroll_result[0] else 0
num_points = len(points)
logger.info(f"Found {num_points} indexed notes in Qdrant for user {user_id}")
if scroll_result[0]:
if points:
timestamps = [
point.payload.get("indexed_at", 0)
for point in scroll_result[0]
for point in points
if point.payload is not None
]
max_timestamp = max(timestamps) if timestamps else 0
@@ -220,7 +264,8 @@ async def scan_user_documents(
indexed_doc_ids = set()
if not initial_sync:
assert qdrant_client is not None # narrow for the type checker
scroll_result = await qdrant_client.scroll(
points = await _scroll_all_points(
qdrant_client,
collection_name=get_settings().get_collection_name(),
scroll_filter=Filter(
must=[
@@ -228,14 +273,12 @@ async def scan_user_documents(
FieldCondition(key="doc_type", match=MatchValue(value="note")),
]
),
with_payload=["doc_id"],
with_vectors=False,
limit=10000,
payload_fields=["doc_id"],
)
indexed_doc_ids = {
str(point.payload["doc_id"])
for point in (scroll_result[0] or [])
for point in points
if point.payload is not None and "doc_id" in point.payload
}
@@ -394,7 +437,8 @@ async def scan_user_documents(
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(
points = await _scroll_all_points(
qdrant_client,
collection_name=settings.get_collection_name(),
scroll_filter=Filter(
must=[
@@ -402,14 +446,12 @@ async def scan_user_documents(
FieldCondition(key="doc_type", match=MatchValue(value="file")),
]
),
limit=10000, # Reasonable limit for file count
with_payload=["doc_id"],
with_vectors=False,
payload_fields=["doc_id"],
)
indexed_file_ids = {
str(point.payload["doc_id"])
for point in (file_scroll_result[0] or [])
for point in points
if point.payload is not None and "doc_id" in point.payload
}
@@ -675,7 +717,8 @@ async def scan_news_items(
indexed_item_ids: set[str] = set()
if not initial_sync:
qdrant_client = await get_qdrant_client()
scroll_result = await qdrant_client.scroll(
points = await _scroll_all_points(
qdrant_client,
collection_name=settings.get_collection_name(),
scroll_filter=Filter(
must=[
@@ -683,13 +726,11 @@ async def scan_news_items(
FieldCondition(key="doc_type", match=MatchValue(value="news_item")),
]
),
with_payload=["doc_id"],
with_vectors=False,
limit=10000,
payload_fields=["doc_id"],
)
indexed_item_ids = {
str(point.payload["doc_id"])
for point in (scroll_result[0] or [])
for point in points
if point.payload is not None and "doc_id" in point.payload
}
logger.debug(f"Found {len(indexed_item_ids)} indexed news items in Qdrant")
@@ -854,7 +895,8 @@ async def scan_deck_cards(
indexed_card_ids: set[str] = set()
if not initial_sync:
qdrant_client = await get_qdrant_client()
scroll_result = await qdrant_client.scroll(
points = await _scroll_all_points(
qdrant_client,
collection_name=settings.get_collection_name(),
scroll_filter=Filter(
must=[
@@ -862,13 +904,11 @@ async def scan_deck_cards(
FieldCondition(key="doc_type", match=MatchValue(value="deck_card")),
]
),
with_payload=["doc_id"],
with_vectors=False,
limit=10000,
payload_fields=["doc_id"],
)
indexed_card_ids = {
str(point.payload["doc_id"])
for point in (scroll_result[0] or [])
for point in points
if point.payload is not None and "doc_id" in point.payload
}
logger.debug(f"Found {len(indexed_card_ids)} indexed deck cards in Qdrant")