fix(vector): dead-letter terminally-failed documents to stop multi-user re-queue loop
A pathological PDF (a 206-page ChronoScan scan with ~3400 JBIG2/JPX images)
jammed a tenant's structured ingest worker in an infinite reprocess loop,
re-burning a 120s pymupdf4llm parse (and occasionally OOM-racing the 2Gi pod)
every few minutes.
Root cause: the per-user placeholder "failed" mark could not stop the loop. The
placeholder point ID is user-agnostic (uuid5("file:<doc_id>:placeholder")) but
the scanner's freshness gate, query, and status update all filter by user_id.
For a file visible to several users the single shared placeholder's user_id is
overwritten by whoever scanned last, so every other user's scan sees "no record"
and re-queues -- an N-user ping-pong that never honours the failed status.
Fix: when a parse fails terminally (no higher escalation tier available, e.g.
structured with OCR off) record a durable, content-addressed, user-agnostic
dead-letter marker (mirrors vector/sharing_state.py). The scanner consults it
tenant-wide for every user and skips re-queuing until the content (etag) OR the
escalation-tier set (tiers_sig -- e.g. OCR enabled) changes, so the document is
attempted once per content-version instead of forever.
- new vector/dead_letter.py: mark/is/clear, content-addressed marker carrying
is_placeholder=True (inherits search exclusion) + dead_letter=True
- escalation.escalation_tiers_signature(settings): retry-on-tier-change key
- processor: dead-letter terminal failures, clear on successful (re-)index
- scanner: user-agnostic is_dead_lettered skip beside claim_existing_index
- placeholder: exempt dead_letter markers from the orphan sweep (durability)
- metrics: astrolabe_document_dead_lettered_total{reason}
Deck #349.
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
d969526613
commit
8c9339501e
@@ -0,0 +1,42 @@
|
||||
"""Unit tests for the escalation-tier signature used by dead-letter keying.
|
||||
|
||||
``escalation_tiers_signature`` fingerprints the runtime escalation config so a
|
||||
dead-lettered document becomes retryable when a new tier appears (e.g. an
|
||||
operator enables OCR). It must be settings-derived (role-independent) and must
|
||||
change when OCR is toggled.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.document_processors.escalation import (
|
||||
escalation_tiers_signature,
|
||||
)
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
def _settings(*, ocr: bool, engine: str = "pypdfium2") -> SimpleNamespace:
|
||||
return SimpleNamespace(document_ocr_enabled=ocr, document_tier1_engine=engine)
|
||||
|
||||
|
||||
def test_signature_is_stable_for_same_config() -> None:
|
||||
assert escalation_tiers_signature(
|
||||
_settings(ocr=False)
|
||||
) == escalation_tiers_signature(_settings(ocr=False))
|
||||
|
||||
|
||||
def test_enabling_ocr_changes_signature() -> None:
|
||||
# Enabling OCR adds an escalation tier -> previously dead-lettered docs retry.
|
||||
assert escalation_tiers_signature(
|
||||
_settings(ocr=False)
|
||||
) != escalation_tiers_signature(_settings(ocr=True))
|
||||
|
||||
|
||||
def test_tier1_engine_change_changes_signature() -> None:
|
||||
assert escalation_tiers_signature(
|
||||
_settings(ocr=False, engine="pypdfium2")
|
||||
) != escalation_tiers_signature(_settings(ocr=False, engine="pymupdf"))
|
||||
@@ -0,0 +1,170 @@
|
||||
"""Unit tests for content-addressed dead-letter markers.
|
||||
|
||||
Covers vector/dead_letter.py: the durable, user-agnostic terminal-failure marker
|
||||
that stops a multi-user shared file (whose single placeholder's user_id is
|
||||
overwritten by the last scanner) from being re-queued forever. A marker is keyed
|
||||
by content (``etag``) + escalation config (``tiers_sig``); a scan skips only while
|
||||
both still match, so a content change or a newly-available tier (e.g. OCR enabled)
|
||||
makes the document retryable again.
|
||||
|
||||
Qdrant is reached via ``get_qdrant_client``/``get_settings``/``get_embedding_service``,
|
||||
all monkeypatched here so the logic runs without a live Qdrant.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock
|
||||
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.vector import dead_letter as dl
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
_COLLECTION = "test_collection"
|
||||
|
||||
|
||||
class _Settings:
|
||||
def get_collection_name(self) -> str:
|
||||
return _COLLECTION
|
||||
|
||||
|
||||
def _point(payload: dict) -> SimpleNamespace:
|
||||
"""Stand-in for a qdrant_client Record (only id/payload are read)."""
|
||||
return SimpleNamespace(id="pt", payload=payload)
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def client(monkeypatch) -> AsyncMock:
|
||||
"""An AsyncMock Qdrant client wired into dead_letter, plus stub deps.
|
||||
|
||||
``scroll`` defaults to "no points"; individual tests override
|
||||
``client.scroll.return_value``/``side_effect``.
|
||||
"""
|
||||
qc = AsyncMock()
|
||||
qc.scroll.return_value = ([], None)
|
||||
monkeypatch.setattr(dl, "get_qdrant_client", AsyncMock(return_value=qc))
|
||||
monkeypatch.setattr(dl, "get_settings", lambda: _Settings())
|
||||
monkeypatch.setattr(
|
||||
dl, "get_embedding_service", lambda: SimpleNamespace(get_dimension=lambda: 4)
|
||||
)
|
||||
return qc
|
||||
|
||||
|
||||
def _must_conditions(flt) -> dict:
|
||||
"""Map FieldCondition key -> matched value for a Filter's ``must`` clause."""
|
||||
out = {}
|
||||
for c in flt.must or []:
|
||||
key = getattr(c, "key", None)
|
||||
match = getattr(c, "match", None)
|
||||
out[key] = getattr(match, "value", None)
|
||||
return out
|
||||
|
||||
|
||||
class TestDeadLetterId:
|
||||
def test_user_agnostic_and_distinct_from_placeholder(self) -> None:
|
||||
from nextcloud_mcp_server.vector.placeholder import _generate_placeholder_id
|
||||
|
||||
dl_id = dl._generate_dead_letter_id("file", "520189")
|
||||
# Deterministic + user-agnostic (depends only on doc_type:doc_id).
|
||||
assert dl_id == dl._generate_dead_letter_id("file", "520189")
|
||||
# Never collides with the in-flight placeholder for the same document.
|
||||
assert dl_id != _generate_placeholder_id("file", "520189")
|
||||
|
||||
|
||||
class TestMarkDeadLetter:
|
||||
async def test_upserts_content_addressed_marker(self, client) -> None:
|
||||
await dl.mark_dead_letter(
|
||||
"520189",
|
||||
"file",
|
||||
"etag-1",
|
||||
"ocr=0;t1=pypdfium2",
|
||||
"timeout",
|
||||
file_path="/Plans/big.pdf",
|
||||
)
|
||||
client.upsert.assert_awaited_once()
|
||||
point = client.upsert.await_args.kwargs["points"][0]
|
||||
assert point.id == dl._generate_dead_letter_id("file", "520189")
|
||||
payload = point.payload
|
||||
assert payload["is_placeholder"] is True
|
||||
assert payload[dl.DEAD_LETTER_KEY] is True
|
||||
assert payload["etag"] == "etag-1"
|
||||
assert payload["tiers_sig"] == "ocr=0;t1=pypdfium2"
|
||||
assert payload["reason"] == "timeout"
|
||||
assert payload["doc_id"] == "520189"
|
||||
assert payload["file_path"] == "/Plans/big.pdf"
|
||||
|
||||
async def test_failure_is_swallowed(self, client) -> None:
|
||||
client.upsert.side_effect = RuntimeError("qdrant down")
|
||||
# Best-effort: a write failure must not propagate (would crash the worker).
|
||||
await dl.mark_dead_letter("1", "file", "e", "sig", "oom")
|
||||
|
||||
|
||||
class TestIsDeadLettered:
|
||||
def _marker(self, etag: str, tiers_sig: str) -> SimpleNamespace:
|
||||
return _point(
|
||||
{
|
||||
"doc_id": "520189",
|
||||
dl.DEAD_LETTER_KEY: True,
|
||||
"etag": etag,
|
||||
"tiers_sig": tiers_sig,
|
||||
}
|
||||
)
|
||||
|
||||
async def test_true_when_etag_and_sig_match(self, client) -> None:
|
||||
client.scroll.return_value = ([self._marker("e1", "sig1")], None)
|
||||
assert await dl.is_dead_lettered("520189", "file", "e1", "sig1") is True
|
||||
|
||||
async def test_false_on_etag_change(self, client) -> None:
|
||||
client.scroll.return_value = ([self._marker("e1", "sig1")], None)
|
||||
# File content changed -> retryable.
|
||||
assert await dl.is_dead_lettered("520189", "file", "e2", "sig1") is False
|
||||
|
||||
async def test_false_on_tiers_sig_change(self, client) -> None:
|
||||
client.scroll.return_value = ([self._marker("e1", "ocr=0;t1=pypdfium2")], None)
|
||||
# OCR just enabled -> a new escalation tier exists -> retryable.
|
||||
assert (
|
||||
await dl.is_dead_lettered("520189", "file", "e1", "ocr=1;t1=pypdfium2")
|
||||
is False
|
||||
)
|
||||
|
||||
async def test_false_when_no_marker(self, client) -> None:
|
||||
client.scroll.return_value = ([], None)
|
||||
assert await dl.is_dead_lettered("520189", "file", "e1", "sig1") is False
|
||||
|
||||
async def test_false_on_empty_etag(self, client) -> None:
|
||||
# Cannot content-address without an etag; never dead-lettered.
|
||||
assert await dl.is_dead_lettered("520189", "file", "", "sig1") is False
|
||||
client.scroll.assert_not_awaited()
|
||||
|
||||
async def test_false_on_qdrant_error(self, client) -> None:
|
||||
client.scroll.side_effect = RuntimeError("qdrant down")
|
||||
# Degrade to "process normally" rather than aborting the scan.
|
||||
assert await dl.is_dead_lettered("520189", "file", "e1", "sig1") is False
|
||||
|
||||
async def test_filter_is_user_agnostic(self, client) -> None:
|
||||
client.scroll.return_value = ([self._marker("e1", "sig1")], None)
|
||||
await dl.is_dead_lettered("520189", "file", "e1", "sig1")
|
||||
flt = client.scroll.await_args.kwargs["scroll_filter"]
|
||||
conds = _must_conditions(flt)
|
||||
assert conds == {
|
||||
"doc_id": "520189",
|
||||
"doc_type": "file",
|
||||
dl.DEAD_LETTER_KEY: True,
|
||||
}
|
||||
assert "user_id" not in conds
|
||||
|
||||
|
||||
class TestClearDeadLetter:
|
||||
async def test_deletes_by_marker_filter(self, client) -> None:
|
||||
await dl.clear_dead_letter("520189", "file")
|
||||
client.delete.assert_awaited_once()
|
||||
flt = client.delete.await_args.kwargs["points_selector"]
|
||||
conds = _must_conditions(flt)
|
||||
assert conds[dl.DEAD_LETTER_KEY] is True
|
||||
assert conds["doc_id"] == "520189"
|
||||
|
||||
async def test_failure_is_swallowed(self, client) -> None:
|
||||
client.delete.side_effect = RuntimeError("qdrant down")
|
||||
await dl.clear_dead_letter("520189", "file")
|
||||
@@ -110,6 +110,26 @@ async def test_keeps_own_pod_placeholders(monkeypatch):
|
||||
client.delete.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_keeps_dead_letter_markers(monkeypatch):
|
||||
"""Dead-letter markers (vector/dead_letter.py) reuse is_placeholder=True for
|
||||
the search exclusion but are DURABLE terminal-state records, not in-flight
|
||||
placeholders. They carry a foreign/absent instance_id, so without the
|
||||
carve-out the sweep would delete them on every Pod restart and the
|
||||
dead-lettered document would loop again. They MUST be kept."""
|
||||
monkeypatch.setattr(placeholder_module, "_INSTANCE_ID", "pod-new")
|
||||
marker = SimpleNamespace(
|
||||
id="dl-1",
|
||||
payload={"is_placeholder": True, "dead_letter": True, "instance_id": "pod-old"},
|
||||
)
|
||||
client = _make_client([([marker], None)])
|
||||
|
||||
swept, kept = await sweep_orphan_placeholders(client, "nextcloud_content")
|
||||
|
||||
assert (swept, kept) == (0, 1)
|
||||
client.delete.assert_not_awaited()
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
async def test_noop_when_no_placeholders_exist(monkeypatch):
|
||||
"""Cold-boot tenant with an empty collection — sweep does NOT
|
||||
|
||||
@@ -0,0 +1,141 @@
|
||||
"""Unit tests for the processor's terminal-parse-failure dead-lettering.
|
||||
|
||||
When a PDF parse fails permanently (isolated-worker timeout/OOM) and the failing
|
||||
tier has NO higher escalation tier available (e.g. ``structured`` with OCR off),
|
||||
``_index_document`` records a durable, content-addressed dead-letter marker
|
||||
instead of the per-user ``status="failed"`` placeholder mark — the latter could
|
||||
not stop the multi-user re-queue loop. A failure that still has a higher tier
|
||||
keeps the legacy per-user mark.
|
||||
|
||||
The real ``ProcessorRegistry`` singleton is used so the terminal decision
|
||||
(``next_available_tier``) is exercised faithfully; only the parse itself, the
|
||||
content fetch, and the Qdrant side-effects are mocked.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from nextcloud_mcp_server.document_processors.base import ProcessingResult
|
||||
from nextcloud_mcp_server.vector import processor
|
||||
from nextcloud_mcp_server.vector.scanner import DocumentTask
|
||||
|
||||
pytestmark = pytest.mark.unit
|
||||
|
||||
|
||||
def _settings(*, ocr_enabled: bool) -> SimpleNamespace:
|
||||
return SimpleNamespace(
|
||||
document_ocr_enabled=ocr_enabled,
|
||||
document_tier1_engine="pypdfium2",
|
||||
get_collection_name=lambda: "c",
|
||||
)
|
||||
|
||||
|
||||
def _file_task() -> DocumentTask:
|
||||
return DocumentTask(
|
||||
user_id="Demo-User",
|
||||
doc_id="520189",
|
||||
doc_type="file",
|
||||
operation="index",
|
||||
modified_at=0,
|
||||
file_path="/Plans/big.pdf",
|
||||
etag="etag-1",
|
||||
)
|
||||
|
||||
|
||||
def _nc_client() -> MagicMock:
|
||||
# MagicMock (typed Any) keeps the pre-commit ty-check happy where the real
|
||||
# signature wants a NextcloudClient -- matching the other processor tests.
|
||||
nc = MagicMock()
|
||||
nc.webdav.read_file = AsyncMock(return_value=(b"%PDF-1.4", "application/pdf"))
|
||||
return nc
|
||||
|
||||
|
||||
def _patch_common(mocker, *, ocr_enabled: bool):
|
||||
"""Patch the shared seams; returns the spies for assertions."""
|
||||
mocker.patch.object(
|
||||
processor, "get_settings", lambda: _settings(ocr_enabled=ocr_enabled)
|
||||
)
|
||||
# Never a tenant-wide dedup hit (file was never indexed).
|
||||
mocker.patch.object(
|
||||
processor, "claim_existing_index", AsyncMock(return_value=False)
|
||||
)
|
||||
spies = SimpleNamespace(
|
||||
mark=mocker.patch.object(processor, "mark_dead_letter", AsyncMock()),
|
||||
dead_metric=mocker.patch.object(processor, "record_document_dead_lettered"),
|
||||
delete_ph=mocker.patch.object(
|
||||
processor, "delete_placeholder_point", AsyncMock()
|
||||
),
|
||||
update_ph=mocker.patch.object(
|
||||
processor, "update_placeholder_status", AsyncMock()
|
||||
),
|
||||
)
|
||||
return spies
|
||||
|
||||
|
||||
async def test_terminal_failure_dead_letters(mocker):
|
||||
"""structured tier fails + OCR off (no higher tier) -> dead-letter, not mark."""
|
||||
spies = _patch_common(mocker, ocr_enabled=False)
|
||||
# The per-tier worker runs the structured tier and the parse times out.
|
||||
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)",
|
||||
)
|
||||
),
|
||||
)
|
||||
|
||||
result = await processor._index_document(
|
||||
_file_task(), _nc_client(), MagicMock(), tier="structured"
|
||||
)
|
||||
|
||||
assert result is False
|
||||
spies.mark.assert_awaited_once()
|
||||
# Marker is content-addressed with this etag + the OCR-off tiers signature.
|
||||
args = spies.mark.await_args.args
|
||||
assert args[0] == "520189" and args[1] == "file"
|
||||
assert args[2] == "etag-1" # etag
|
||||
assert "ocr=0" in args[3] # tiers_sig
|
||||
assert args[4] == "timeout" # reason
|
||||
spies.dead_metric.assert_called_once_with("timeout")
|
||||
spies.delete_ph.assert_awaited_once() # volatile placeholder dropped
|
||||
spies.update_ph.assert_not_awaited() # NOT the legacy per-user failed mark
|
||||
|
||||
|
||||
async def test_non_terminal_failure_keeps_legacy_mark(mocker):
|
||||
"""fast tier fails while structured is still available -> legacy failed mark."""
|
||||
spies = _patch_common(mocker, ocr_enabled=False)
|
||||
mocker.patch.object(
|
||||
processor,
|
||||
"_parse_pdf_tier",
|
||||
AsyncMock(
|
||||
return_value=ProcessingResult(
|
||||
text="",
|
||||
metadata={"parse_failed_reason": "error", "pipeline_tier": "fast"},
|
||||
processor="pypdfium2",
|
||||
success=False,
|
||||
error="isolated parse failed (error)",
|
||||
)
|
||||
),
|
||||
)
|
||||
|
||||
result = await processor._index_document(
|
||||
_file_task(), _nc_client(), MagicMock(), tier="fast"
|
||||
)
|
||||
|
||||
assert result is False
|
||||
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()
|
||||
Reference in New Issue
Block a user