Merge remote-tracking branch 'origin/master' into fix/309-embed-resilience

# Conflicts:
#	nextcloud_mcp_server/vector/processor.py
This commit is contained in:
Chris Coutinho
2026-06-11 10:46:57 +02:00
19 changed files with 837 additions and 48 deletions
+145 -1
View File
@@ -1,11 +1,12 @@
"""Tests for CLI options using Click's testing utilities."""
import os
from types import SimpleNamespace
import pytest
from click.testing import CliRunner
from nextcloud_mcp_server.cli import run
from nextcloud_mcp_server.cli import _init_worker_observability, run, worker
@pytest.fixture
@@ -324,3 +325,146 @@ def test_stdio_calls_get_stdio_mcp(runner, clean_env, monkeypatch):
assert result.exit_code == 0, result.output
assert called_with.get("transport") == "stdio"
assert called_with.get("enabled_apps") is None
# ---------------------------------------------------------------------------
# Ingest worker observability bootstrap (Deck #310 / #175)
# ---------------------------------------------------------------------------
def _fake_settings(**overrides):
"""A lightweight settings stand-in for the worker observability helper.
The helper only reads attributes, so a SimpleNamespace avoids running the
real Settings.__post_init__ validation/derivation.
"""
base = dict(
ingest_queue="postgres", # for realism / worker() gating; unused by the helper
log_format="json",
log_level="INFO",
log_include_trace_context=True,
metrics_enabled=True,
metrics_port=9090,
otel_exporter_otlp_endpoint=None,
otel_service_name="nextcloud-mcp-server",
otel_exporter_verify_ssl=False,
otel_traces_sampler_arg=1.0,
)
base.update(overrides)
return SimpleNamespace(**base)
@pytest.fixture
def patched_observability(monkeypatch):
"""Patch the worker's observability entrypoints and record their kwargs."""
calls: dict[str, dict] = {}
monkeypatch.setattr(
"nextcloud_mcp_server.cli.setup_logging",
lambda **kw: calls.__setitem__("logging", kw),
)
monkeypatch.setattr(
"nextcloud_mcp_server.cli.setup_metrics",
lambda **kw: calls.__setitem__("metrics", kw),
)
monkeypatch.setattr(
"nextcloud_mcp_server.cli.setup_tracing",
lambda **kw: calls.__setitem__("tracing", kw),
)
return calls
def test_init_worker_observability_configures_logging(patched_observability):
"""Worker initializes structured logging from settings (AC: JSON logs)."""
_init_worker_observability(_fake_settings())
assert patched_observability["logging"] == {
"log_format": "json",
"log_level": "INFO",
"include_trace_context": True,
}
def test_init_worker_observability_starts_metrics_when_enabled(patched_observability):
"""Worker starts the Prometheus server on the configured port (AC: /metrics)."""
_init_worker_observability(_fake_settings(metrics_port=9123))
assert patched_observability["metrics"] == {"port": 9123}
def test_init_worker_observability_skips_metrics_when_disabled(patched_observability):
"""METRICS_ENABLED=false leaves the worker without a metrics server."""
_init_worker_observability(_fake_settings(metrics_enabled=False))
assert "metrics" not in patched_observability
# Logging is still configured regardless of the metrics toggle.
assert "logging" in patched_observability
def test_init_worker_observability_sets_up_tracing_when_endpoint(
patched_observability,
):
"""An OTLP endpoint enables tracing so worker spans (parse/embed) export."""
_init_worker_observability(
_fake_settings(
otel_exporter_otlp_endpoint="https://otel:4317",
otel_traces_sampler_arg=0.5,
)
)
assert patched_observability["tracing"] == {
"service_name": "nextcloud-mcp-server",
"otlp_endpoint": "https://otel:4317",
"otlp_verify_ssl": False,
"sampling_rate": 0.5,
}
def test_init_worker_observability_skips_tracing_without_endpoint(
patched_observability,
):
"""No OTLP endpoint → tracing stays disabled (matches API pod behavior)."""
_init_worker_observability(_fake_settings(otel_exporter_otlp_endpoint=None))
assert "tracing" not in patched_observability
def test_worker_initializes_observability_on_postgres_queue(runner, monkeypatch):
"""The worker command wires up observability once config is runnable."""
monkeypatch.setattr(
"nextcloud_mcp_server.cli.get_settings",
lambda: _fake_settings(ingest_queue="postgres"),
)
called = {}
def fake_init(settings):
called["settings"] = settings
# Stop before the procrastinate/worker machinery.
raise SystemExit(0)
monkeypatch.setattr(
"nextcloud_mcp_server.cli._init_worker_observability", fake_init
)
result = runner.invoke(worker, [])
assert result.exit_code == 0, result.output
assert called.get("settings") is not None
def test_worker_rejects_non_postgres_queue_before_observability(runner, monkeypatch):
"""A non-postgres queue fails fast, before any metrics server is started."""
monkeypatch.setattr(
"nextcloud_mcp_server.cli.get_settings",
lambda: _fake_settings(ingest_queue="memory"),
)
called = {}
monkeypatch.setattr(
"nextcloud_mcp_server.cli._init_worker_observability",
lambda settings: called.setdefault("init", True),
)
result = runner.invoke(worker, [])
assert result.exit_code != 0
assert "INGEST_QUEUE=postgres" in result.output
assert "init" not in called
+130
View File
@@ -510,3 +510,133 @@ def test_parse_search_response_decodes_non_ascii_paths(mocker):
assert results[0]["href"] == "/remote.php/dav/files/testuser/学生邮箱/report.pdf"
# name comes from <d:displayname>, which is not URL-encoded; sanity-check it.
assert results[0]["name"] == "report.pdf"
def _request_url(mock_http_client) -> str:
"""Positional URL passed to the underlying httpx ``request`` call."""
return mock_http_client.request.call_args[0][1]
@pytest.mark.unit
@pytest.mark.parametrize(
"path, expected",
[
("", "/remote.php/dav/files/testuser/"),
("/Documents/notes.txt", "/remote.php/dav/files/testuser/Documents/notes.txt"),
("Documents/notes.txt", "/remote.php/dav/files/testuser/Documents/notes.txt"),
("a/b #1.pdf", "/remote.php/dav/files/testuser/a/b%20%231.pdf"),
("law/x, y z.pdf", "/remote.php/dav/files/testuser/law/x%2C%20y%20%20z.pdf"),
(
"学生邮箱/r.pdf",
"/remote.php/dav/files/testuser/%E5%AD%A6%E7%94%9F%E9%82%AE%E7%AE%B1/r.pdf",
),
],
)
def test_webdav_path_encoding(path, expected):
"""_webdav_path encodes the decoded caller path once, preserving '/', and
strips a leading slash. Every caller-path builder routes through this, so
it is the single source of truth for their encoding."""
client = WebDAVClient(AsyncMock(), "testuser")
assert client._webdav_path(path) == expected
@pytest.mark.unit
def test_encode_dav_path_encodes_exactly_once():
"""Pins the decoded-input precondition: a literal '%' becomes '%25', so an
already-encoded path passed in error would double-encode (caught here)."""
from nextcloud_mcp_server.client.webdav import _encode_dav_path
assert _encode_dav_path("already%20encoded.pdf") == "already%2520encoded.pdf"
@pytest.mark.unit
async def test_read_file_encodes_special_chars(mocker):
"""read_file must percent-encode '#', commas, and spaces in the path (card 309).
Paths arrive already URL-decoded from PROPFIND/REPORT, so an unencoded '#'
reaches httpx as a URL fragment and silently truncates the request → 404 on
valid files (e.g. OHR-Bench law filenames). The outgoing request path must be
percent-encoded.
"""
mock_http_client = AsyncMock()
client = WebDAVClient(mock_http_client, "testuser")
mock_response = AsyncMock()
mock_response.content = b"%PDF-1.4 data"
mock_response.headers = {"content-type": "application/pdf"}
mock_response.raise_for_status = mocker.Mock()
mock_http_client.request = AsyncMock(return_value=mock_response)
# Name with a '#', a comma, a double space and a trailing space before ".pdf".
await client.read_file("law/ADMA BioManufacturing, LLC - Amendment #2 .pdf")
url = _request_url(mock_http_client)
assert url.startswith("/remote.php/dav/files/testuser/")
# The hazardous characters are encoded; path separators are preserved.
assert "%23" in url # '#'
assert "%2C" in url # ','
assert "%20" in url # space
assert "#" not in url
assert ", " not in url
assert "/law/" in url
@pytest.mark.unit
async def test_read_file_ascii_path_unchanged(mocker):
"""A plain ASCII path must pass through unchanged (no spurious encoding)."""
mock_http_client = AsyncMock()
client = WebDAVClient(mock_http_client, "testuser")
mock_response = AsyncMock()
mock_response.content = b"data"
mock_response.headers = {"content-type": "text/plain"}
mock_response.raise_for_status = mocker.Mock()
mock_http_client.request = AsyncMock(return_value=mock_response)
await client.read_file("Documents/notes.txt")
assert (
_request_url(mock_http_client)
== "/remote.php/dav/files/testuser/Documents/notes.txt"
)
@pytest.mark.unit
async def test_move_resource_encodes_destination_header(mocker):
"""The MOVE Destination header must be percent-encoded too (card 309)."""
mock_http_client = AsyncMock()
client = WebDAVClient(mock_http_client, "testuser")
mock_response = AsyncMock()
mock_response.status_code = 201
mock_response.raise_for_status = mocker.Mock()
mock_http_client.request = AsyncMock(return_value=mock_response)
await client.move_resource("a/old.pdf", "b/new #1.pdf")
call = mock_http_client.request.call_args
# Source is the request path; destination is the header.
assert call[0][1] == "/remote.php/dav/files/testuser/a/old.pdf"
destination = call.kwargs["headers"]["Destination"]
assert "%23" in destination
assert "#" not in destination
@pytest.mark.unit
async def test_copy_resource_encodes_destination_header(mocker):
"""The COPY Destination header must be percent-encoded too (card 309)."""
mock_http_client = AsyncMock()
client = WebDAVClient(mock_http_client, "testuser")
mock_response = AsyncMock()
mock_response.status_code = 201
mock_response.raise_for_status = mocker.Mock()
mock_http_client.request = AsyncMock(return_value=mock_response)
await client.copy_resource("a/old.pdf", "b/new #1.pdf")
call = mock_http_client.request.call_args
assert call[0][1] == "/remote.php/dav/files/testuser/a/old.pdf"
destination = call.kwargs["headers"]["Destination"]
assert "%23" in destination
assert "#" not in destination
+34
View File
@@ -213,6 +213,24 @@ class TestChunkConfigValidation:
_reload_config()
assert get_settings().document_chunk_page_aware is False
def test_ocr_timeout_default_and_env_override(self):
"""document_ocr_timeout_seconds defaults to 180 and reads its env var.
Guards the _DEFAULTS-key-must-match-env-var footgun: a mismatch would
leave the override silently ignored.
"""
assert Settings().document_ocr_timeout_seconds == pytest.approx(180.0)
with patch.dict(os.environ, {"DOCUMENT_OCR_TIMEOUT_SECONDS": "45"}, clear=True):
_reload_config()
assert get_settings().document_ocr_timeout_seconds == pytest.approx(45.0)
def test_max_pdf_size_default_and_env_override(self):
"""document_max_pdf_size_mb defaults to 50 and reads its env var."""
assert Settings().document_max_pdf_size_mb == pytest.approx(50.0)
with patch.dict(os.environ, {"DOCUMENT_MAX_PDF_SIZE_MB": "12.5"}, clear=True):
_reload_config()
assert get_settings().document_max_pdf_size_mb == pytest.approx(12.5)
def test_valid_chunk_settings(self):
"""Test valid chunk size and overlap configuration."""
settings = Settings(
@@ -491,6 +509,22 @@ class TestDynaconfValidators:
with pytest.raises(ValidationError, match="DOCUMENT_CHUNK_SIZE"):
_reload_config()
@patch.dict(os.environ, {"DOCUMENT_OCR_TIMEOUT_SECONDS": "0"}, clear=True)
def test_ocr_timeout_zero_rejected(self):
"""DOCUMENT_OCR_TIMEOUT_SECONDS=0 fails the gte=1 validator."""
from dynaconf import ValidationError
with pytest.raises(ValidationError, match="DOCUMENT_OCR_TIMEOUT_SECONDS"):
_reload_config()
@patch.dict(os.environ, {"DOCUMENT_MAX_PDF_SIZE_MB": "-1"}, clear=True)
def test_max_pdf_size_negative_rejected(self):
"""DOCUMENT_MAX_PDF_SIZE_MB=-1 fails the gte=0 validator (0 = disabled)."""
from dynaconf import ValidationError
with pytest.raises(ValidationError, match="DOCUMENT_MAX_PDF_SIZE_MB"):
_reload_config()
@patch.dict(os.environ, {"METRICS_PORT": "8080"}, clear=True)
def test_valid_metrics_port(self):
"""Test valid METRICS_PORT passes validation."""
+90
View File
@@ -3,6 +3,7 @@
from types import SimpleNamespace
from typing import Any
import anyio
import pytest
from nextcloud_mcp_server.document_processors import ocr
@@ -14,6 +15,7 @@ def _settings(**kw) -> Any: # a Settings stand-in (only the read fields matter)
base = dict(
document_ocr_provider="auto",
document_ocr_model="mistral/mistral-ocr-latest",
document_ocr_timeout_seconds=180.0,
embedding_gateway_url=None,
embedding_gateway_client_id=None,
embedding_gateway_client_secret=None,
@@ -128,3 +130,91 @@ async def test_processor_backend_error_returns_success_false(monkeypatch):
r = await ocr.OcrProcessor().process(b"%PDF-1.7", "application/pdf")
assert r.success is False
assert r.metadata["parse_failed_reason"] == "error"
async def test_processor_timeout_returns_timeout_reason(monkeypatch):
"""A backend TimeoutError gets its own reason bucket (not 'error')."""
class _TimeoutBackend:
async def ocr(self, content, mime_type):
raise TimeoutError
monkeypatch.setattr(
ocr, "get_settings", lambda: _settings(document_ocr_timeout_seconds=5.0)
)
monkeypatch.setattr(ocr, "build_ocr_backend", lambda s: _TimeoutBackend())
r = await ocr.OcrProcessor().process(b"%PDF-1.7", "application/pdf")
assert r.success is False
assert r.metadata["parse_failed_reason"] == "timeout"
assert "timed out" in r.error
async def test_gateway_httpx_timeout_maps_to_timeout_reason(monkeypatch):
"""A gateway httpx.ReadTimeout (not a builtin TimeoutError) must still map to
parse_failed_reason='timeout', not 'error'."""
import httpx
class _HttpxTimeoutBackend:
async def ocr(self, content, mime_type):
raise httpx.ReadTimeout("read timed out")
monkeypatch.setattr(
ocr, "get_settings", lambda: _settings(document_ocr_timeout_seconds=5.0)
)
monkeypatch.setattr(ocr, "build_ocr_backend", lambda s: _HttpxTimeoutBackend())
r = await ocr.OcrProcessor().process(b"%PDF-1.7", "application/pdf")
assert r.success is False
assert r.metadata["parse_failed_reason"] == "timeout"
assert "timed out" in r.error
async def test_gateway_backend_uses_configured_timeout(mocker, monkeypatch):
"""The gateway OCR call must use DOCUMENT_OCR_TIMEOUT_SECONDS (resolved per
call), not the old hardcoded 180s constant."""
resp = mocker.Mock()
resp.raise_for_status = mocker.Mock()
resp.json = mocker.Mock(return_value={"pages": [{"index": 0, "markdown": "ok"}]})
client = mocker.MagicMock()
client.__aenter__ = mocker.AsyncMock(return_value=client)
client.__aexit__ = mocker.AsyncMock(return_value=False)
client.post = mocker.AsyncMock(return_value=resp)
captured: dict[str, Any] = {}
def _make_client(*args, **kwargs):
captured["timeout"] = kwargs.get("timeout")
return client
monkeypatch.setattr(ocr.httpx, "AsyncClient", _make_client)
monkeypatch.setattr(
ocr, "get_settings", lambda: _settings(document_ocr_timeout_seconds=42.0)
)
backend = ocr._GatewayOcrBackend("https://gw", "mistral/mistral-ocr-latest")
await backend.ocr(b"%PDF-1.7", "application/pdf")
# httpx.Timeout(42.0, connect=10.0): the read/overall budget is the setting.
assert captured["timeout"].read == pytest.approx(42.0)
assert captured["timeout"].connect == pytest.approx(10.0)
async def test_mistral_backend_applies_timeout(mocker, monkeypatch):
"""The Mistral backend wraps process_async in DOCUMENT_OCR_TIMEOUT_SECONDS,
so a slow OCR call fails fast instead of hanging on the SDK default."""
monkeypatch.setattr(
ocr, "get_settings", lambda: _settings(document_ocr_timeout_seconds=0.01)
)
# Bypass the SDK constructor; only the two attributes ocr() reads matter.
backend = ocr._MistralOcrBackend.__new__(ocr._MistralOcrBackend)
backend._model = "mistral-ocr-latest"
async def _slow(*args, **kwargs):
await anyio.sleep(1.0)
backend._client = mocker.MagicMock()
backend._client.ocr.process_async = _slow
with pytest.raises(TimeoutError):
await backend.ocr(b"%PDF-1.7", "application/pdf")
+41
View File
@@ -74,6 +74,9 @@ class _Settings:
page_fraction=0.5,
min_page_chars=16,
detect_scanned=False,
# Guard off by default so existing tiering tests are unaffected; tests
# that exercise the size guard pass an explicit cap.
max_pdf_size_mb=0.0,
):
self.document_tier1_engine = engine
self.document_classify_enabled = classify
@@ -82,6 +85,7 @@ class _Settings:
self.document_ocr_page_fraction = page_fraction
self.document_ocr_min_page_chars = min_page_chars
self.document_ocr_detect_scanned = detect_scanned
self.document_max_pdf_size_mb = max_pdf_size_mb
def _registry(*procs: tuple[DocumentProcessor, int]) -> ProcessorRegistry:
@@ -98,6 +102,43 @@ async def test_pdf_routes_to_fast_tier(monkeypatch):
assert res.processor == "fast"
async def test_oversize_pdf_fails_fast_without_parsing(monkeypatch):
"""A PDF over the size cap must fail fast as 'oversize' before any tier runs."""
monkeypatch.setattr(
reg_mod, "get_settings", lambda: _Settings(max_pdf_size_mb=0.001)
)
fast = _Fake("fast", "fast")
ran = False
orig = fast.process
async def _tracking(*a, **k):
nonlocal ran
ran = True
return await orig(*a, **k)
fast.process = _tracking # type: ignore[method-assign]
r = _registry((fast, 20))
# ~2 KB > 0.001 MB (~1 KB) cap.
res = await r.process(b"%PDF-1.7" + b"0" * 2048, "application/pdf", "big.pdf")
assert res.success is False
assert res.metadata["parse_failed_reason"] == "oversize"
assert res.processor == "size_guard"
assert ran is False, "size guard must short-circuit before the fast tier runs"
async def test_under_cap_pdf_still_parses(monkeypatch):
"""A PDF under the cap is unaffected by the guard."""
monkeypatch.setattr(
reg_mod, "get_settings", lambda: _Settings(max_pdf_size_mb=10.0)
)
r = _registry((_Fake("fast", "fast"), 20))
res = await r.process(b"%PDF-1.7", "application/pdf")
assert res.success is True
assert res.processor == "fast"
async def test_engine_rollback_uses_structured(monkeypatch):
monkeypatch.setattr(reg_mod, "get_settings", lambda: _Settings(engine="pymupdf"))
r = _registry((_Fake("fast", "fast"), 20), (_Fake("structured", "structured"), 10))
+44
View File
@@ -0,0 +1,44 @@
"""Unit tests for vector-sync error formatting (card 309)."""
import httpx
import pytest
from nextcloud_mcp_server.vector._errors import format_exception_group
@pytest.mark.unit
def test_format_plain_exception_returns_repr():
exc = httpx.ConnectError("Connection error")
assert format_exception_group(exc) == repr(exc)
@pytest.mark.unit
def test_format_exception_group_names_leaf_cause():
"""A single-child group must surface the real ConnectError, not the group's
useless 'unhandled errors in a TaskGroup' default message."""
leaf = httpx.ConnectError("Connection error")
group = BaseExceptionGroup("unhandled errors in a TaskGroup", [leaf])
formatted = format_exception_group(group)
assert "ConnectError" in formatted
# Assert the full leaf repr survives, not just the type name -- guards a
# future format change that kept the type but dropped the message.
assert repr(leaf) in formatted
assert "unhandled errors in a TaskGroup" not in formatted
assert "1 sub-exception" in formatted
@pytest.mark.unit
def test_format_nested_exception_group_flattens_all_leaves():
inner = BaseExceptionGroup(
"inner", [ValueError("bad value"), httpx.ConnectError("conn")]
)
outer = BaseExceptionGroup("outer", [inner, RuntimeError("boom")])
formatted = format_exception_group(outer)
assert "ValueError" in formatted
assert "ConnectError" in formatted
assert "RuntimeError" in formatted
assert "3 sub-exceptions" in formatted
+50
View File
@@ -0,0 +1,50 @@
"""Regression test for processor_task's exception handler (card 309 / PR #891).
If ``receive_stream.receive()`` raises something other than
``TimeoutError``/``EndOfStream`` before any document is bound, the broad
``except`` handler must not crash on an unbound ``doc_task`` name.
"""
from unittest.mock import MagicMock
import anyio
import pytest
from nextcloud_mcp_server.vector.processor import processor_task
class _ReceiveBoomThenEnd:
"""First receive() raises a non-Timeout error (no doc_task bound yet); the
second ends the stream so the loop exits."""
def __init__(self, shutdown: anyio.Event):
self._calls = 0
self._shutdown = shutdown
async def receive(self):
self._calls += 1
if self._calls == 1:
raise RuntimeError("transport blew up before any document")
self._shutdown.set()
raise anyio.EndOfStream
def statistics(self): # pragma: no cover - not reached on the error path
return MagicMock(current_buffer_used=0)
@pytest.mark.unit
async def test_processor_task_receive_error_does_not_raise_unbound(caplog):
shutdown = anyio.Event()
stream = _ReceiveBoomThenEnd(shutdown)
# Must complete without a NameError leaking out of the except handler.
with caplog.at_level("ERROR", logger="nextcloud_mcp_server.vector.processor"):
await processor_task(
worker_id=0,
receive_stream=stream, # type: ignore[arg-type]
shutdown_event=shutdown,
nc_client=MagicMock(),
user_id="alice",
)
assert any("RuntimeError" in rec.message for rec in caplog.records)