diff --git a/nextcloud_mcp_server/vector/dead_letter.py b/nextcloud_mcp_server/vector/dead_letter.py index 245c08d4..7bbcbdbc 100644 --- a/nextcloud_mcp_server/vector/dead_letter.py +++ b/nextcloud_mcp_server/vector/dead_letter.py @@ -25,6 +25,16 @@ filter, plus ``dead_letter=True`` so the orphan-placeholder sweep and the scanne can tell it apart from a volatile in-flight placeholder. Mirrors the fail-safe philosophy of ``sharing_state``: a Qdrant error never aborts ingest — a failed lookup degrades to "process normally", a failed write is logged, not raised. + +TODO(deck-349): a marker for a file that is dead-lettered and *then* deleted from +Nextcloud can be orphaned. The processor's delete path clears it, but the +scanner's grace-period deletion tracking only sees a file via its real indexed +points (filtered by ``user_id``); a dead-lettered file has only this +user-agnostic marker, so its disappearance enqueues no delete task and the marker +is never reached. Not a correctness issue (search excludes ``is_placeholder=True`` +and the etag check means a stale marker never blocks new content), but it +accumulates. A dedicated marker sweep or a TTL payload field would close it — +tracked as a follow-up, not done here. """ import logging @@ -57,11 +67,17 @@ def _generate_dead_letter_id(doc_type: str, doc_id: str) -> str: def _dead_letter_filter(doc_id: str, doc_type: str) -> Filter: - """Match the dead-letter marker for one document (tenant-wide, no user_id).""" + """Match the dead-letter marker for one document (tenant-wide, no user_id). + + Includes ``is_placeholder=True`` (redundant with ``dead_letter=True``, which + nothing else sets) so Qdrant can lean on the existing ``is_placeholder`` + payload index. + """ return Filter( must=[ FieldCondition(key="doc_id", match=MatchValue(value=doc_id)), FieldCondition(key="doc_type", match=MatchValue(value=doc_type)), + FieldCondition(key="is_placeholder", match=MatchValue(value=True)), FieldCondition(key=DEAD_LETTER_KEY, match=MatchValue(value=True)), ] ) diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index e394da6f..c6f90526 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -1068,7 +1068,7 @@ async def _index_document( reason == "oversize" or registry.next_available_tier(failing_tier, settings) is None ) - if terminal: + if terminal and doc_task.etag: # No higher tier can run (e.g. structured timed out with # OCR off), so retrying just re-burns the same failing # parse. Dead-letter the document tenant-wide @@ -1079,11 +1079,13 @@ async def _index_document( # ping-pong the per-user "failed" mark could not: a file # shared by N users has ONE user-agnostic placeholder # whose user_id is overwritten by the last scanner, so - # every other user re-queued it forever. + # every other user re-queued it forever. Requires an etag + # to content-address the marker; without one (rare) we + # fall back to the legacy per-user mark below. await mark_dead_letter( doc_task.doc_id, doc_task.doc_type, - doc_task.etag or "", + doc_task.etag, escalation_tiers_signature(settings), reason, file_path=file_path, @@ -1112,10 +1114,10 @@ async def _index_document( exc_info=True, ) else: - # A higher tier exists; parse failures don't escalate to - # it today, so keep the legacy per-user "failed" - # placeholder mark (not dead-lettered -- a future change - # may route the failure to that tier). + # Either a higher tier exists (parse failures don't + # escalate to it today) or there's no etag to + # content-address a dead-letter marker. Keep the legacy + # per-user "failed" placeholder mark. logger.warning( "Permanent parse failure for %s (reason=%s); marking " "failed and skipping index", diff --git a/tests/unit/vector/test_dead_letter.py b/tests/unit/vector/test_dead_letter.py index 6defa720..6529bba0 100644 --- a/tests/unit/vector/test_dead_letter.py +++ b/tests/unit/vector/test_dead_letter.py @@ -151,6 +151,7 @@ class TestIsDeadLettered: assert conds == { "doc_id": "520189", "doc_type": "file", + "is_placeholder": True, dl.DEAD_LETTER_KEY: True, } assert "user_id" not in conds diff --git a/tests/unit/vector/test_processor_dead_letter.py b/tests/unit/vector/test_processor_dead_letter.py index d1e9b81b..afea278d 100644 --- a/tests/unit/vector/test_processor_dead_letter.py +++ b/tests/unit/vector/test_processor_dead_letter.py @@ -170,6 +170,38 @@ async def test_oversize_failure_dead_letters_regardless_of_tier(mocker): spies.update_ph.assert_not_awaited() +async def test_terminal_failure_without_etag_uses_legacy_mark(mocker): + """A terminal failure with no etag can't be content-addressed, so fall back to + the legacy per-user placeholder mark instead of writing an unmatchable marker.""" + spies = _patch_common(mocker, ocr_enabled=False) + mocker.patch.object( + processor, + "_parse_pdf_tier", + AsyncMock( + return_value=ProcessingResult( + text="", + metadata={ + "parse_failed_reason": "timeout", + "pipeline_tier": "structured", + }, + processor="pymupdf", + success=False, + error="isolated parse failed (timeout)", + ) + ), + ) + task = _file_task() + task.etag = None # no content key + + result = await processor._index_document( + task, _nc_client(), MagicMock(), tier="structured" + ) + + assert result is False + spies.mark.assert_not_awaited() # no unmatchable marker written + spies.update_ph.assert_awaited_once() # legacy fallback + + 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