From b736bf199b236c778bf2f13bb3e59f020bb47813 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 3 Jun 2026 00:23:55 +0200 Subject: [PATCH] perf(search): skip exclusion lookup on empty tag set; fix semaphore comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the two important findings from the claude bot's latest re-review: - _verify_files: skip get_excluded_file_paths entirely when the tag REPORT returns no files. An empty `tagged` yields an empty `tagged_ids` regardless of exclusions, so the lookup's 2xlen(EXCLUDED_TAGS) WebDAV fan-out is wasted work in the common "this tag matched nothing" case. The per-result loop still runs, so malformed doc_ids are still kept (fail-open) β€” pinned by a new test (test_verify_files_empty_tag_set_skips_exclusion_lookup), which also asserts the exclusion lookup is never awaited. - Rewrite the semaphore comment: it claimed "the slot bounds them", but the slot only caps concurrent *searches* β€” get_excluded_file_paths internally spawns a task group issuing 2xlen(EXCLUDED_TAGS) concurrent WebDAV calls, so live Nextcloud connections can exceed VERIFICATION_CONCURRENCY. Comment now says so and points at configuration.md. The third 🟑 (sequential dir expansion in find_files_by_tag) is pre-existing and flagged by the reviewer as a follow-up, not part of this PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/search/verification.py | 45 ++++++++++++++------- tests/unit/search/test_verification.py | 24 +++++++++++ 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/nextcloud_mcp_server/search/verification.py b/nextcloud_mcp_server/search/verification.py index 0bd36c32..1192d5fb 100644 --- a/nextcloud_mcp_server/search/verification.py +++ b/nextcloud_mcp_server/search/verification.py @@ -174,11 +174,20 @@ async def _verify_files( tag_name = get_settings().vector_sync_pdf_tag - # Two batch fetches per search held under a single semaphore slot (same - # backpressure rationale as _verify_news_items): the tagged-file REPORT - # (plus optional Depth:infinity folder expansion) and the EXCLUDED_TAGS - # lookup (get_excluded_file_paths, itself ~2Γ—N concurrent calls). Both are - # batched once per search, not once per result, and the slot bounds them. + # One semaphore slot is held for both Nextcloud round-trips: the tagged-file + # REPORT (plus optional Depth:infinity folder expansion) and β€” only when the + # REPORT returned files β€” the EXCLUDED_TAGS lookup. Both are batched once per + # search, not once per result (same backpressure rationale as + # _verify_news_items). + # + # The slot caps how many *searches* verify files concurrently, but it does + # NOT bound the fan-out *within* one verification: get_excluded_file_paths + # internally spawns a task group issuing 2Γ—len(EXCLUDED_TAGS) concurrent + # WebDAV calls (1 PROPFIND + 1 REPORT per excluded tag), so the live + # Nextcloud connection count can exceed VERIFICATION_CONCURRENCY when + # excluded tags are configured. See configuration.md β†’ "Files caveat" for + # the latency/tuning guidance. + # # The pure-Python intersection that builds tagged_ids/accessible runs # *outside* the slot β€” it needs no Nextcloud round-trip (mirrors the # post-fetch present_ids build in _verify_news_items). @@ -213,15 +222,23 @@ async def _verify_files( # Exclusion wins: a tagged file under an EXCLUDED_TAGS folder must not # surface, matching the scanner's defense-in-depth filter. A failure # here degrades to "no exclusion" rather than dropping legitimate hits. - try: - excluded_paths = await get_excluded_file_paths(client.webdav) - except Exception as e: - logger.warning( - "EXCLUDED_TAGS lookup failed during verification (%s); " - "proceeding without exclusion filter", - e, - ) - excluded_paths = set() + # + # Skip the lookup entirely when the tag REPORT returned nothing: an empty + # `tagged` yields an empty `tagged_ids` regardless of the exclusion set, + # so the lookup's 2Γ—len(EXCLUDED_TAGS) WebDAV fan-out cannot change the + # outcome β€” avoid it in the common "this tag matched nothing" case. The + # per-result loop below still runs, so malformed doc_ids are still kept + # (fail-open), exactly as when `tagged` is non-empty. + excluded_paths: set[str] = set() + if tagged: + try: + excluded_paths = await get_excluded_file_paths(client.webdav) + except Exception as e: + logger.warning( + "EXCLUDED_TAGS lookup failed during verification (%s); " + "proceeding without exclusion filter", + e, + ) tagged_ids: set[str] = set() for f in tagged: diff --git a/tests/unit/search/test_verification.py b/tests/unit/search/test_verification.py index ab198096..bc79925f 100644 --- a/tests/unit/search/test_verification.py +++ b/tests/unit/search/test_verification.py @@ -504,6 +504,30 @@ async def test_verify_files_deleted_drops(mocker): assert result == set() +@pytest.mark.unit +async def test_verify_files_empty_tag_set_skips_exclusion_lookup(mocker): + """When the tag REPORT returns no files, the EXCLUDED_TAGS lookup is skipped + entirely: an empty tagged set drops every valid-id result regardless of + exclusions, so the lookup's 2xN WebDAV fan-out is wasted work. Malformed + doc_ids are still kept (fail-open), exactly as on the non-empty path.""" + excluded = _patch_excluded(mocker, {"Secret"}) + client = _file_client(mocker, tagged=[]) + + result = await _verify_files( + client, + [ + _make_result(123, doc_type="file"), + _make_result("not-a-file-id", doc_type="file"), + ], + _sem(), + ) + + # Valid id absent from the (empty) tagged set β†’ dropped; malformed id kept. + assert result == {"not-a-file-id"} + # The optimization: no exclusion fan-out when there is nothing to filter. + excluded.assert_not_awaited() + + @pytest.mark.unit async def test_verify_files_excluded_path_drops(mocker): """A tagged file under an EXCLUDED_TAGS folder must not surface β€” exclusion