fix(vector): address PR review round 4 — backfill resilience + degraded-mode docs
- 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
02744a50e0
commit
b97ac23228
@@ -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
|
> caught early. Either widen the public model's `id` field or convert the
|
||||||
> id at the verifier layer.
|
> 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 '<field>' (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 '<collection>'; 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
|
#### Explicit Override
|
||||||
|
|
||||||
Set `QDRANT_COLLECTION` to use a specific collection name:
|
Set `QDRANT_COLLECTION` to use a specific collection name:
|
||||||
|
|||||||
@@ -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
|
# 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
|
# subsequent restarts can short-circuit the O(N) scroll. Carries no
|
||||||
# user_id/doc_id/doc_type, so production search filters (which always
|
# 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_ID: str = "00000000-0000-0000-0000-d0c1d0d1d0c1"
|
||||||
_DOC_ID_BACKFILL_SENTINEL_PAYLOAD: dict[str, str] = {"_migration_marker": "doc_id_v1"}
|
_DOC_ID_BACKFILL_SENTINEL_PAYLOAD: dict[str, str] = {"_migration_marker": "doc_id_v1"}
|
||||||
|
|
||||||
@@ -139,6 +141,14 @@ async def _backfill_doc_id_to_string(
|
|||||||
next_offset = None
|
next_offset = None
|
||||||
batch_size = 256
|
batch_size = 256
|
||||||
|
|
||||||
|
# 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:
|
while True:
|
||||||
points, next_offset = await client.scroll(
|
points, next_offset = await client.scroll(
|
||||||
collection_name=collection_name,
|
collection_name=collection_name,
|
||||||
@@ -206,6 +216,13 @@ async def _backfill_doc_id_to_string(
|
|||||||
rewritten,
|
rewritten,
|
||||||
scanned,
|
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:
|
async def get_qdrant_client() -> AsyncQdrantClient:
|
||||||
|
|||||||
@@ -484,11 +484,11 @@ async def scan_user_documents(
|
|||||||
await send_stream.send(
|
await send_stream.send(
|
||||||
DocumentTask(
|
DocumentTask(
|
||||||
user_id=user_id,
|
user_id=user_id,
|
||||||
doc_id=file_id, # Use numeric file ID
|
doc_id=file_id,
|
||||||
doc_type="file",
|
doc_type="file",
|
||||||
operation="index",
|
operation="index",
|
||||||
modified_at=modified_at,
|
modified_at=modified_at,
|
||||||
file_path=file_path, # Pass file path for content retrieval
|
file_path=file_path,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
file_queued += 1
|
file_queued += 1
|
||||||
@@ -547,11 +547,11 @@ async def scan_user_documents(
|
|||||||
await send_stream.send(
|
await send_stream.send(
|
||||||
DocumentTask(
|
DocumentTask(
|
||||||
user_id=user_id,
|
user_id=user_id,
|
||||||
doc_id=file_id, # Use numeric file ID
|
doc_id=file_id,
|
||||||
doc_type="file",
|
doc_type="file",
|
||||||
operation="index",
|
operation="index",
|
||||||
modified_at=modified_at,
|
modified_at=modified_at,
|
||||||
file_path=file_path, # Pass file path for content retrieval
|
file_path=file_path,
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
file_queued += 1
|
file_queued += 1
|
||||||
@@ -581,7 +581,7 @@ async def scan_user_documents(
|
|||||||
await send_stream.send(
|
await send_stream.send(
|
||||||
DocumentTask(
|
DocumentTask(
|
||||||
user_id=user_id,
|
user_id=user_id,
|
||||||
doc_id=file_id, # Use numeric file ID
|
doc_id=file_id,
|
||||||
doc_type="file",
|
doc_type="file",
|
||||||
operation="delete",
|
operation="delete",
|
||||||
modified_at=0,
|
modified_at=0,
|
||||||
|
|||||||
@@ -387,3 +387,34 @@ async def test_backfill_handles_payload_with_explicit_none_doc_id(mocker):
|
|||||||
points=[2],
|
points=[2],
|
||||||
wait=True,
|
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
|
||||||
|
|||||||
Reference in New Issue
Block a user