fix(search): address PR #834 re-review (403/404 coverage + docs)

Resolve the blocking + important findings from the claude bot's re-review:

- test (blocking): pin the file verifier's fail-open contract for definitive
  403/404 on the tag REPORT, not just transient 503/429. A disabled systemtags
  endpoint commonly 403s; unlike the per-access verifiers (where 403/404 = drop),
  the batch file verifier must keep all results since the whole set hinges on one
  REPORT. Adds _http_error(403)/_http_error(404) to
  test_verify_files_tag_fetch_failure_keeps_all and documents the asymmetry.
- docs (important): migration caveat — if vector-index was created as
  user_visible=False (manual occ tag:add, or pre-release), an owner's tag won't
  surface in a recipient's REPORT and shared-file results are silently dropped
  after upgrade. Note that the MCP server's get_or_create_tag defaults to
  user_visible=True, and how to verify/fix an existing tag.
- docs (important): note the file verifier's latency scales with both the
  Depth:infinity folder expansion and the EXCLUDED_TAGS lookup (~2 WebDAV calls
  per excluded tag, fanned out under one slot); suggest lowering
  VERIFICATION_CONCURRENCY for large excluded-tag lists / deeply tagged trees.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-06-02 23:52:16 +02:00
co-authored by Claude Opus 4.8
parent 8cae7d1708
commit 7033c64393
2 changed files with 38 additions and 7 deletions
+21 -4
View File
@@ -675,10 +675,27 @@ aware of:
an excluded folder) drops out of results immediately rather than waiting for an excluded folder) drops out of results immediately rather than waiting for
the scanner sweep. The REPORT expands tagged folders via a `Depth: infinity` the scanner sweep. The REPORT expands tagged folders via a `Depth: infinity`
SEARCH, so deployments that tag whole directory trees pay that walk once per SEARCH, so deployments that tag whole directory trees pay that walk once per
search; configure `VECTOR_SYNC_PDF_TAG` to change the tag name. **Shared search; configure `VECTOR_SYNC_PDF_TAG` to change the tag name. The `file`
files**: a file an owner tagged and shared with the searcher only survives verifier's latency therefore scales with **both** the `Depth: infinity` folder
verification if the owner's (userVisible) tag surfaces in the *searcher's* expansion **and** the `EXCLUDED_TAGS` lookup: that lookup fans out ~2 WebDAV
tag REPORT. calls (1 PROPFIND + 1 REPORT) *per excluded tag*, concurrently, while holding
a single verification slot — so a deployment with a long `EXCLUDED_TAGS` list
and/or deeply tagged trees issues many parallel Nextcloud requests per search.
Operators in that situation may want to **lower `VERIFICATION_CONCURRENCY`** so
the file verifier's internal fan-out does not overwhelm the backend.
- **Shared files**: a file an owner tagged and shared with the searcher only
survives verification if the owner's **`userVisible`** tag surfaces in the
*searcher's* tag REPORT. The MCP server's own tag-creation path
(`WebDAVClient.get_or_create_tag`) defaults to `user_visible=True`, so tags it
creates are fine. **Migration caveat**: if the `vector-index` tag was created
some other way — manually via `occ tag:add … --user-visible=false`, or in a
deployment predating this release — it may be `user_visible=False` (the
Nextcloud default for system-managed tags). In that case an owner's tag will
**not** surface in a recipient's systemtag REPORT, so every shared-file result
is *silently dropped* for recipients after upgrading — no error, just a
narrower result set. Verify the tag's visibility (Administration → *Collaborative
tags*, or `occ tag:list`) and, if it is not user-visible, recreate it as
user-visible so shared search keeps working.
- **Eviction**: when verification finds a definitive miss (a 404 / 403, or — for - **Eviction**: when verification finds a definitive miss (a 404 / 403, or — for
files — absence from the tag set), the corresponding Qdrant points are deleted files — absence from the tag set), the corresponding Qdrant points are deleted
in the background on a lifespan-owned task group — fire-and-forget, does in the background on a lifespan-owned task group — fire-and-forget, does
+17 -3
View File
@@ -529,10 +529,24 @@ async def test_verify_files_excluded_path_drops(mocker):
@pytest.mark.unit @pytest.mark.unit
async def test_verify_files_tag_fetch_failure_keeps_all(mocker): async def test_verify_files_tag_fetch_failure_keeps_all(mocker):
"""If the tag REPORT itself fails (HTTP or otherwise), keep every file """If the tag REPORT itself fails, keep every file result (fail-open) —
result (fail-open) — never silently shrink results on a backend blip.""" never silently shrink results on a backend blip.
Unlike the per-access verifiers (notes/deck/news), where a definitive
403/404 is the DROP signal, the file verifier fails open on *every* HTTP
error — including 403/404. The whole result set hinges on one batch REPORT,
so a disabled systemtags endpoint (commonly 403) must not nuke all file
results; the next query re-verifies. 403 and 404 are pinned here alongside
the transient 503/429 to lock that contract against regression.
"""
_patch_excluded(mocker) _patch_excluded(mocker)
for exc in (_http_error(503), _http_error(429), RuntimeError("dav blew up")): for exc in (
_http_error(403),
_http_error(404),
_http_error(503),
_http_error(429),
RuntimeError("dav blew up"),
):
client = _file_client(mocker, find_side_effect=exc) client = _file_client(mocker, find_side_effect=exc)
result = await _verify_files( result = await _verify_files(
client, client,