From cd348b32334b4c6fc3c0c5d4dc31fa56705fac96 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 17 Jun 2026 19:17:38 +0200 Subject: [PATCH] fix(vector): clear dead-letter marker on delete, treat oversize as terminal Addresses round-1 review on PR #920: - Delete path now clears the file's dead-letter marker after release_document_for_user (whose principal-based filter misses the user-agnostic, principal-less marker), preventing orphan-marker accumulation for dead-lettered-then-deleted files. - Oversize PDFs (rejected by the pre-parse size guard, no pipeline_tier stamped) are now treated as terminal regardless of failing_tier -- no tier can parse an oversize file -- so they dead-letter instead of falling to the legacy per-user mark on the inline path. - Gate the success-path clear on a non-empty etag (an etag-less file can never have a marker, mirroring is_dead_lettered's early return). - dead_letter.py: payload typed dict[str, Any] (CLAUDE.md). Tests: oversize-terminal dead-letter and delete-path marker clear. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/vector/dead_letter.py | 3 +- nextcloud_mcp_server/vector/processor.py | 21 ++++++-- .../unit/vector/test_processor_dead_letter.py | 51 +++++++++++++++++++ 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/nextcloud_mcp_server/vector/dead_letter.py b/nextcloud_mcp_server/vector/dead_letter.py index eca389de..245c08d4 100644 --- a/nextcloud_mcp_server/vector/dead_letter.py +++ b/nextcloud_mcp_server/vector/dead_letter.py @@ -30,6 +30,7 @@ lookup degrades to "process normally", a failed write is logged, not raised. import logging import time import uuid +from typing import Any from qdrant_client import models from qdrant_client.models import FieldCondition, Filter, MatchValue, PointStruct @@ -88,7 +89,7 @@ async def mark_dead_letter( settings = get_settings() dimension = get_embedding_service().get_dimension() - payload: dict = { + payload: dict[str, Any] = { "doc_id": doc_id, "doc_type": doc_type, "is_placeholder": True, diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 39ee8389..e394da6f 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -626,6 +626,13 @@ async def process_document( await release_document_for_user( doc_task.doc_id, doc_task.doc_type, doc_task.user_id ) + # Drop any dead-letter marker for the file too: release only + # removes it when the last reader leaves (its filter misses the + # user-agnostic, principal-less marker), so without this a + # dead-lettered-then-deleted file would leave an orphan marker + # accumulating in Qdrant. Only files are ever dead-lettered. + if doc_task.doc_type == "file": + await clear_dead_letter(doc_task.doc_id, doc_task.doc_type) logger.info( "Deleted %s_%s for %s", doc_task.doc_type, @@ -1052,8 +1059,14 @@ async def _index_document( failing_tier = tier or result.metadata.get( "pipeline_tier", TIER_LADDER[0] ) + # An oversize PDF is rejected by the pre-parse size guard + # before any tier runs (no pipeline_tier stamped on the inline + # path) and no tier can ever parse it, so it is terminal + # regardless of failing_tier. Otherwise, terminal == no higher + # tier can run. terminal = ( - registry.next_available_tier(failing_tier, settings) is None + reason == "oversize" + or registry.next_available_tier(failing_tier, settings) is None ) if terminal: # No higher tier can run (e.g. structured timed out with @@ -1594,9 +1607,9 @@ async def _index_document( # A successful (re-)index supersedes any prior terminal failure: clear a # stale dead-letter marker (e.g. the file was fixed/replaced, or a new # escalation tier finally parsed it) so it isn't left behind. Only files are - # ever dead-lettered (the mark lives in the file branch), so skip the extra - # Qdrant round-trip for the other doc types on the hot indexing path. - if doc_task.doc_type == "file": + # ever dead-lettered, and only with a non-empty etag (is_dead_lettered + # early-returns without one), so skip the extra Qdrant round-trip otherwise. + if doc_task.doc_type == "file" and doc_task.etag: await clear_dead_letter(doc_task.doc_id, doc_task.doc_type) # Delete placeholder before writing real vectors diff --git a/tests/unit/vector/test_processor_dead_letter.py b/tests/unit/vector/test_processor_dead_letter.py index a00dbcbc..d1e9b81b 100644 --- a/tests/unit/vector/test_processor_dead_letter.py +++ b/tests/unit/vector/test_processor_dead_letter.py @@ -139,3 +139,54 @@ async def test_non_terminal_failure_keeps_legacy_mark(mocker): spies.update_ph.assert_awaited_once() # legacy per-user failed mark spies.mark.assert_not_awaited() # NOT dead-lettered (structured can still run) spies.dead_metric.assert_not_called() + + +async def test_oversize_failure_dead_letters_regardless_of_tier(mocker): + """An oversize PDF is terminal at any tier (no tier can parse it) -> dead-letter + even though a higher tier (structured) is nominally available above 'fast'.""" + spies = _patch_common(mocker, ocr_enabled=False) + mocker.patch.object( + processor, + "_parse_pdf_tier", + AsyncMock( + return_value=ProcessingResult( + text="", + metadata={"parse_failed_reason": "oversize"}, + processor="size_guard", + success=False, + error="PDF exceeds size cap", + ) + ), + ) + + result = await processor._index_document( + _file_task(), _nc_client(), MagicMock(), tier="fast" + ) + + assert result is False + spies.mark.assert_awaited_once() + assert spies.mark.await_args.args[4] == "oversize" # reason + spies.dead_metric.assert_called_once_with("oversize") + spies.update_ph.assert_not_awaited() + + +async def test_delete_clears_dead_letter_marker(mocker): + """Deleting a file must also drop its dead-letter marker, else a + dead-lettered-then-deleted file leaves an orphan accumulating in Qdrant + (release_document_for_user's filter misses the user-agnostic marker).""" + mocker.patch.object(processor, "get_qdrant_client", AsyncMock()) + mocker.patch.object(processor, "release_document_for_user", AsyncMock()) + clear = mocker.patch.object(processor, "clear_dead_letter", AsyncMock()) + + task = DocumentTask( + user_id="Demo-User", + doc_id="520189", + doc_type="file", + operation="delete", + modified_at=0, + file_path="/Plans/big.pdf", + etag="etag-1", + ) + await processor.process_document(task, MagicMock(), max_retries=1) + + clear.assert_awaited_once_with("520189", "file")