perf(search): skip exclusion lookup on empty tag set; fix semaphore comment
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
7033c64393
commit
b736bf199b
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user