refactor(search): address PR #750 round 6 review feedback
Closes out the remaining nits flagged in the round-6 review. Critical: - _verify_files contract comment now enumerates all None-return cases (404 + malformed PROPFIND XML) and documents the false-eviction trade-off; self-healing via re-indexing recovers - int(r.id) cast at the SemanticSearchResult boundary now raises a TypeError with explicit doc_type/value context instead of bubbling up as an opaque "Search failed: ..." McpError Design observations: - nc_semantic_search_answer docstring documents the per-note round-trip cost from the post-verification race guard - News verification latency hint added to configuration.md - SemanticSearchResponse exposes verified_count + dropped_count so short result pages on high-ghost-density indexes are distinguishable from genuine scarcity. verify_search_results now returns (kept, dropped_count); production caller and tests updated Minor: - Comment clarifies the .get() fallback in verify_search_results is defensive only (run_verifier always populates the entry) - Eviction task-group guard narrowed from except Exception to except RuntimeError (the only documented failure mode of TaskGroup.start_soon on a closed group) - Indexer logs a warning when a deck_card task is missing board_id/stack_id, surfacing data-quality issues at index time rather than at verification time - New unit test covers the news verifier's non-numeric-id fail-open path (one bad doc_id keeps the entire batch) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
ffcca23a7b
commit
e8df6003c5
@@ -498,9 +498,12 @@ aware of:
|
||||
verifier issues a single `news.get_items(batch_size=-1, get_read=True)` call
|
||||
per search that contains any news result, then intersects locally. The
|
||||
payload is **unbounded** — for users with very large feed backlogs this can
|
||||
dominate verification latency. Disabling News in the indexer or running with
|
||||
a smaller backlog mitigates this; per-item paginated verification is tracked
|
||||
as a future improvement.
|
||||
dominate verification latency. As a rough guide on a healthy LAN connection:
|
||||
a typical purged backlog (1k–5k items) returns in ~200–500 ms; very large
|
||||
backlogs (>20k items) can exceed 2 s and become the dominant cost of any
|
||||
search that surfaces news results. Disabling News in the indexer or running
|
||||
with a smaller backlog mitigates this; per-item paginated verification is
|
||||
tracked as a future improvement.
|
||||
- **Eviction**: when verification finds a definitive miss (404 / 403), the
|
||||
corresponding Qdrant points are deleted in the background on a lifespan-owned
|
||||
task group — fire-and-forget, does **not** block the search response.
|
||||
|
||||
@@ -80,6 +80,22 @@ class SemanticSearchResponse(BaseResponse):
|
||||
search_method: str = Field(
|
||||
default="semantic", description="Search method used (semantic or hybrid)"
|
||||
)
|
||||
verified_count: int = Field(
|
||||
default=0,
|
||||
description=(
|
||||
"Number of unique documents that passed verify-on-read access "
|
||||
"checks (ADR-019). Equals len(results) before trimming to limit."
|
||||
),
|
||||
)
|
||||
dropped_count: int = Field(
|
||||
default=0,
|
||||
description=(
|
||||
"Number of unique documents dropped as ghost records during "
|
||||
"verify-on-read (ADR-019). A short result page (len(results) < "
|
||||
"limit) combined with a non-zero dropped_count indicates ghost "
|
||||
"density rather than scarcity of relevant content."
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
class SamplingSearchResponse(BaseResponse):
|
||||
|
||||
@@ -139,11 +139,25 @@ async def _verify_files(
|
||||
try:
|
||||
info = await client.webdav.get_file_info(file_path)
|
||||
if info is None:
|
||||
# Contract: WebDAVClient.get_file_info returns None on 404
|
||||
# and raises HTTPStatusError on 403/5xx/network. If that
|
||||
# contract changes (e.g. a future refactor that raises 404
|
||||
# like other client methods), the `except HTTPStatusError`
|
||||
# block below already handles it via _is_definitive_404_or_403.
|
||||
# Contract: WebDAVClient.get_file_info returns None in two
|
||||
# cases — (1) HTTP 404, and (2) a malformed PROPFIND XML
|
||||
# response (missing <d:response>, <d:propstat>, or <d:prop>
|
||||
# — see client/webdav.py). Both are treated as
|
||||
# "inaccessible" and trigger eviction.
|
||||
#
|
||||
# Trade-off: a malformed response from a brittle backend
|
||||
# could cause a *false* eviction. We accept that risk in
|
||||
# exchange for correctness on real 404s — the index
|
||||
# self-heals via re-indexing on the next scan, and
|
||||
# malformed responses are exceedingly rare in practice.
|
||||
# Distinguishing the two cases would require widening
|
||||
# get_file_info's return contract; deferred to a future
|
||||
# change if false evictions become observable.
|
||||
#
|
||||
# If the contract ever changes (e.g. 404 raises
|
||||
# HTTPStatusError like other client methods), the
|
||||
# `except HTTPStatusError` block below already handles
|
||||
# it via _is_definitive_404_or_403.
|
||||
return
|
||||
accessible.add(doc_id)
|
||||
except HTTPStatusError as e:
|
||||
@@ -355,7 +369,7 @@ async def verify_search_results(
|
||||
evict_on_missing: bool = True,
|
||||
max_concurrent: int | None = None,
|
||||
eviction_task_group: TaskGroup | None = None,
|
||||
) -> list[SearchResult]:
|
||||
) -> tuple[list[SearchResult], int]:
|
||||
"""Filter search results to those the user can currently access.
|
||||
|
||||
Deduplicates by ``(doc_id, doc_type)`` before verifying, so multiple
|
||||
@@ -386,10 +400,13 @@ async def verify_search_results(
|
||||
from FastMCP tools.
|
||||
|
||||
Returns:
|
||||
Filtered list preserving the original order.
|
||||
Tuple of ``(kept_results, dropped_count)`` where ``kept_results`` is
|
||||
the filtered list preserving the original order and ``dropped_count``
|
||||
is the number of unique ``(doc_id, doc_type)`` pairs that failed
|
||||
verification (ghost records).
|
||||
"""
|
||||
if not results:
|
||||
return results
|
||||
return results, 0
|
||||
|
||||
user_id: str = client.username
|
||||
|
||||
@@ -443,6 +460,9 @@ async def verify_search_results(
|
||||
# Compute (doc_id, doc_type) pairs that failed verification
|
||||
inaccessible: set[tuple[int | str, str]] = set()
|
||||
for doc_type, id_to_result in by_type.items():
|
||||
# The .get() default is defensive only — run_verifier always populates
|
||||
# accessible_by_type[doc_type], either with the verifier's result or
|
||||
# with all ids on verifier crash (fail-open).
|
||||
accessible = accessible_by_type.get(doc_type, set(id_to_result.keys()))
|
||||
for doc_id in id_to_result.keys():
|
||||
if doc_id not in accessible:
|
||||
@@ -492,11 +512,11 @@ async def verify_search_results(
|
||||
# best-effort: the next query re-verifies and re-attempts.
|
||||
try:
|
||||
eviction_task_group.start_soon(evict, doc_id, doc_type)
|
||||
except Exception:
|
||||
except RuntimeError:
|
||||
logger.debug("Eviction task group closed; will retry on next query")
|
||||
else:
|
||||
async with anyio.create_task_group() as tg:
|
||||
for doc_id, doc_type in inaccessible:
|
||||
tg.start_soon(evict, doc_id, doc_type)
|
||||
|
||||
return kept
|
||||
return kept, len(inaccessible)
|
||||
|
||||
@@ -172,11 +172,12 @@ def configure_semantic_tools(mcp: FastMCP):
|
||||
eviction_task_group = getattr(
|
||||
ctx.request_context.lifespan_context, "eviction_task_group", None
|
||||
)
|
||||
verified_results = await verify_search_results(
|
||||
verified_results, dropped_count = await verify_search_results(
|
||||
client,
|
||||
all_results,
|
||||
eviction_task_group=eviction_task_group,
|
||||
)
|
||||
verified_count = len(verified_results)
|
||||
search_results = verified_results[:limit]
|
||||
|
||||
# Convert SearchResult objects to SemanticSearchResult for response.
|
||||
@@ -188,9 +189,24 @@ def configure_semantic_tools(mcp: FastMCP):
|
||||
# public API.
|
||||
results = []
|
||||
for r in search_results:
|
||||
try:
|
||||
narrowed_id = int(r.id)
|
||||
except (TypeError, ValueError) as e:
|
||||
# Re-raise with explicit context so the outer handler logs
|
||||
# something operators can act on (the generic "Search
|
||||
# failed: invalid literal for int()" is opaque).
|
||||
raise TypeError(
|
||||
f"SemanticSearchResult.id must be int-convertible, "
|
||||
f"got {r.id!r} (type={type(r.id).__name__}) for "
|
||||
f"doc_type={r.doc_type!r}. This indicates a doc_type "
|
||||
f"with non-numeric ids has been indexed but the "
|
||||
f"public response model has not been widened. Add "
|
||||
f"the doc_type to the SemanticSearchResult.id type "
|
||||
f"or convert at the verifier layer."
|
||||
) from e
|
||||
results.append(
|
||||
SemanticSearchResult(
|
||||
id=int(r.id),
|
||||
id=narrowed_id,
|
||||
doc_type=r.doc_type,
|
||||
title=r.title,
|
||||
category=r.metadata.get("category", "") if r.metadata else "",
|
||||
@@ -304,6 +320,8 @@ def configure_semantic_tools(mcp: FastMCP):
|
||||
query=query,
|
||||
total_found=len(results),
|
||||
search_method=f"bm25_hybrid_{fusion}",
|
||||
verified_count=verified_count,
|
||||
dropped_count=dropped_count,
|
||||
)
|
||||
|
||||
except ValueError as e:
|
||||
@@ -382,6 +400,14 @@ def configure_semantic_tools(mcp: FastMCP):
|
||||
Note: Requires MCP client to support sampling. If sampling is unavailable,
|
||||
the tool gracefully degrades to returning documents with an explanation.
|
||||
The client may prompt the user to approve the sampling request.
|
||||
|
||||
Latency profile: For each note in the result page, this tool fetches
|
||||
the full note body via ``client.notes.get_note`` after upstream
|
||||
verify-on-read has already round-tripped to the same endpoint as a
|
||||
race guard (ADR-019). Expect one additional Nextcloud round-trip per
|
||||
note result; raising ``limit`` above the default of 5 amplifies this
|
||||
cost roughly linearly. File / news / deck results do not pay this
|
||||
cost — they reuse the verified excerpt.
|
||||
"""
|
||||
# 1. Retrieve relevant documents via existing semantic search
|
||||
search_response = await nc_semantic_search(
|
||||
|
||||
@@ -658,6 +658,23 @@ async def _index_document(
|
||||
indexed_at = int(time.time())
|
||||
points = []
|
||||
|
||||
# Surface deck card data quality issues at indexing time rather than
|
||||
# only at verification time (where _verify_deck_cards falls through to
|
||||
# legacy-data pass-through when board_id/stack_id are missing). This is
|
||||
# logged once per document — not per chunk — to avoid log spam.
|
||||
if doc_task.doc_type == "deck_card":
|
||||
missing_deck_fields = [
|
||||
field for field in ("board_id", "stack_id") if not file_metadata.get(field)
|
||||
]
|
||||
if missing_deck_fields:
|
||||
logger.warning(
|
||||
"Indexing deck_card %s for user %s with missing metadata: %s; "
|
||||
"verification will fall back to legacy-data pass-through",
|
||||
doc_task.doc_id,
|
||||
doc_task.user_id,
|
||||
missing_deck_fields,
|
||||
)
|
||||
|
||||
for i, (chunk, dense_emb, sparse_emb) in enumerate(
|
||||
zip(chunks, dense_embeddings, sparse_embeddings)
|
||||
):
|
||||
|
||||
@@ -58,9 +58,10 @@ async def test_verify_keeps_accessible_note(
|
||||
note_id = temporary_note["id"]
|
||||
results = [_result_for_note(note_id)]
|
||||
|
||||
kept = await verify_search_results(nc_client, results)
|
||||
kept, dropped_count = await verify_search_results(nc_client, results)
|
||||
|
||||
assert [r.id for r in kept] == [note_id]
|
||||
assert dropped_count == 0
|
||||
spy_evict.assert_not_awaited()
|
||||
|
||||
|
||||
@@ -96,9 +97,12 @@ async def test_verify_drops_deleted_note_and_schedules_eviction(
|
||||
await nc_client.notes.get_note(note_id)
|
||||
assert exc_info.value.response.status_code == 404
|
||||
|
||||
kept = await verify_search_results(nc_client, [_result_for_note(note_id)])
|
||||
kept, dropped_count = await verify_search_results(
|
||||
nc_client, [_result_for_note(note_id)]
|
||||
)
|
||||
|
||||
assert kept == [], "deleted note must not pass verification"
|
||||
assert dropped_count == 1
|
||||
spy_evict.assert_awaited_once_with(note_id, "note", nc_client.username)
|
||||
|
||||
|
||||
@@ -126,9 +130,10 @@ async def test_verify_mixed_accessible_and_deleted(
|
||||
_result_for_note(accessible_id),
|
||||
_result_for_note(ghost_id),
|
||||
]
|
||||
kept = await verify_search_results(nc_client, results)
|
||||
kept, dropped_count = await verify_search_results(nc_client, results)
|
||||
|
||||
assert [r.id for r in kept] == [accessible_id]
|
||||
assert dropped_count == 1
|
||||
spy_evict.assert_awaited_once_with(ghost_id, "note", nc_client.username)
|
||||
|
||||
|
||||
@@ -158,9 +163,10 @@ async def test_verify_dedupes_chunks_of_same_document(
|
||||
for i in range(3)
|
||||
]
|
||||
|
||||
kept = await verify_search_results(nc_client, results)
|
||||
kept, dropped_count = await verify_search_results(nc_client, results)
|
||||
|
||||
# All three chunks kept (they're all from the same accessible note)
|
||||
assert len(kept) == 3
|
||||
assert dropped_count == 0
|
||||
# ...but verification only fetched the note ONCE
|
||||
assert spy_get_note.await_count == 1
|
||||
|
||||
@@ -266,6 +266,33 @@ async def test_verify_news_items_transient_keeps_all(mocker):
|
||||
assert result == {1, 2, 3}
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_verify_news_items_non_numeric_id_keeps_all(mocker):
|
||||
"""One non-numeric doc_id triggers fail-open for the WHOLE result set.
|
||||
|
||||
The intersection logic in `_verify_news_items` tries `int(d)` for each
|
||||
incoming doc_id; a single non-numeric value (e.g. ``"abc"``) raises
|
||||
ValueError inside the intersection loop. The except block must catch it
|
||||
and return the full input set (fail open) rather than dropping anything.
|
||||
This is intentional v1 behaviour — surfacing one bad id should not
|
||||
drop legitimate adjacent results.
|
||||
"""
|
||||
news_client = SimpleNamespace(
|
||||
get_items=mocker.AsyncMock(return_value=[{"id": 10}, {"id": 20}])
|
||||
)
|
||||
client = SimpleNamespace(news=news_client, username="alice")
|
||||
|
||||
doc_ids: list[int | str] = [10, 20, "abc"]
|
||||
result = await _verify_news_items(
|
||||
client,
|
||||
[_make_result(d, doc_type="news_item") for d in doc_ids],
|
||||
_sem(),
|
||||
)
|
||||
|
||||
# Fail-open: every requested id is preserved, INCLUDING the bad one.
|
||||
assert result == {10, 20, "abc"}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# File verifier
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -449,7 +476,7 @@ async def test_verify_deck_cards_missing_metadata_keeps_unverified(mocker):
|
||||
@pytest.mark.unit
|
||||
async def test_verify_search_results_empty_input_passthrough():
|
||||
client = SimpleNamespace(username="alice")
|
||||
assert await verify_search_results(client, []) == []
|
||||
assert await verify_search_results(client, []) == ([], 0)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -466,9 +493,10 @@ async def test_verify_search_results_dedupes_chunks_per_document(mocker):
|
||||
]
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
kept = await verify_search_results(client, results)
|
||||
kept, dropped_count = await verify_search_results(client, results)
|
||||
|
||||
assert len(kept) == 3 # all kept, all reference the same accessible doc
|
||||
assert dropped_count == 0
|
||||
spy.assert_awaited_once()
|
||||
# Verifier received exactly one SearchResult (the deduplicated representative)
|
||||
args, _kwargs = spy.call_args
|
||||
@@ -494,9 +522,10 @@ async def test_verify_search_results_drops_inaccessible_and_evicts(mocker):
|
||||
]
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
kept = await verify_search_results(client, results)
|
||||
kept, dropped_count = await verify_search_results(client, results)
|
||||
|
||||
assert [r.id for r in kept] == [1]
|
||||
assert dropped_count == 1
|
||||
spy_evict.assert_awaited_once_with(99, "note", "alice")
|
||||
|
||||
|
||||
@@ -530,9 +559,12 @@ async def test_verify_search_results_fire_and_forget_eviction(mocker):
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
async with anyio.create_task_group() as tg:
|
||||
kept = await verify_search_results(client, results, eviction_task_group=tg)
|
||||
kept, dropped_count = await verify_search_results(
|
||||
client, results, eviction_task_group=tg
|
||||
)
|
||||
# 1. Search response was returned …
|
||||
assert kept == []
|
||||
assert dropped_count == 1
|
||||
# 2. … even though eviction has started but not finished.
|
||||
await eviction_started.wait()
|
||||
assert not eviction_completed.is_set()
|
||||
@@ -554,9 +586,12 @@ async def test_verify_search_results_no_eviction_when_disabled(mocker):
|
||||
results = [_make_result(7, doc_type="note")]
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
kept = await verify_search_results(client, results, evict_on_missing=False)
|
||||
kept, dropped_count = await verify_search_results(
|
||||
client, results, evict_on_missing=False
|
||||
)
|
||||
|
||||
assert kept == []
|
||||
assert dropped_count == 1
|
||||
spy_evict.assert_not_awaited()
|
||||
|
||||
|
||||
@@ -575,9 +610,10 @@ async def test_verify_search_results_unknown_doc_type_passes_through(mocker, cap
|
||||
results = [_make_result(1, doc_type="calendar")]
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
kept = await verify_search_results(client, results)
|
||||
kept, dropped_count = await verify_search_results(client, results)
|
||||
|
||||
assert len(kept) == 1
|
||||
assert dropped_count == 0
|
||||
spy_evict.assert_not_awaited()
|
||||
|
||||
|
||||
@@ -595,9 +631,10 @@ async def test_verify_search_results_verifier_blowup_keeps_all(mocker):
|
||||
]
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
kept = await verify_search_results(client, results)
|
||||
kept, dropped_count = await verify_search_results(client, results)
|
||||
|
||||
assert [r.id for r in kept] == [1, 2]
|
||||
assert dropped_count == 0 # fail-open: nothing dropped
|
||||
spy_evict.assert_not_awaited()
|
||||
|
||||
|
||||
@@ -615,9 +652,10 @@ async def test_verify_search_results_preserves_order(mocker):
|
||||
]
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
kept = await verify_search_results(client, results)
|
||||
kept, dropped_count = await verify_search_results(client, results)
|
||||
|
||||
assert [r.id for r in kept] == [1, 3]
|
||||
assert dropped_count == 1
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -633,8 +671,11 @@ async def test_verify_search_results_eviction_failure_does_not_propagate(mocker)
|
||||
|
||||
client = SimpleNamespace(username="alice")
|
||||
# Should NOT raise
|
||||
kept = await verify_search_results(client, [_make_result(1, doc_type="note")])
|
||||
kept, dropped_count = await verify_search_results(
|
||||
client, [_make_result(1, doc_type="note")]
|
||||
)
|
||||
assert kept == []
|
||||
assert dropped_count == 1
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@@ -656,9 +697,10 @@ async def test_verify_search_results_dispatches_per_doc_type_concurrently(mocker
|
||||
]
|
||||
client = SimpleNamespace(username="alice")
|
||||
|
||||
kept = await verify_search_results(client, results)
|
||||
kept, dropped_count = await verify_search_results(client, results)
|
||||
|
||||
assert {(r.id, r.doc_type) for r in kept} == {(1, "note"), (500, "file")}
|
||||
assert dropped_count == 1
|
||||
note_verifier.assert_awaited_once()
|
||||
file_verifier.assert_awaited_once()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user