From 7033c64393939f2b5f59fed3d9f122460cdb0cdb Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 2 Jun 2026 23:52:16 +0200 Subject: [PATCH] fix(search): address PR #834 re-review (403/404 coverage + docs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/configuration.md | 25 +++++++++++++++++++++---- tests/unit/search/test_verification.py | 20 +++++++++++++++++--- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 81a719bb..1b4dd23c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -675,10 +675,27 @@ aware of: an excluded folder) drops out of results immediately rather than waiting for 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; configure `VECTOR_SYNC_PDF_TAG` to change the tag name. **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. + search; configure `VECTOR_SYNC_PDF_TAG` to change the tag name. The `file` + verifier's latency therefore scales with **both** the `Depth: infinity` folder + expansion **and** the `EXCLUDED_TAGS` lookup: that lookup fans out ~2 WebDAV + 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 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 diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index bcc6a8af..ab198096 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -529,10 +529,24 @@ async def test_verify_files_excluded_path_drops(mocker): @pytest.mark.unit async def test_verify_files_tag_fetch_failure_keeps_all(mocker): - """If the tag REPORT itself fails (HTTP or otherwise), keep every file - result (fail-open) — never silently shrink results on a backend blip.""" + """If the tag REPORT itself fails, keep every file result (fail-open) — + 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) - 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) result = await _verify_files( client,