admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(document): apply OCR timeout to Mistral backend + review/Sonar fixes (#892)

Browse Source 
Round-1 review on PR #892:
- Wire DOCUMENT_OCR_TIMEOUT_SECONDS into _MistralOcrBackend too (was
  gateway-only): wrap process_async in anyio.fail_after so the SDK-managed
  client honours the setting; on expiry it fails fast as a clean parse error.
  Test added.
- Tighten the misleading "honoured without a restart" comment — per-call
  get_settings() is for test monkeypatching; a live change still needs a
  restart since the backend is cached for the pod lifetime.
- Comment the size guard's two intentional gaps: an explicit processor_name
  override bypasses it, and the early return skips the parse-duration histogram.

SonarCloud (new-code smells in the added tests):
- S1244 float-equality asserts → pytest.approx (test_config.py, test_ocr_processor.py).
- S1186/S7503: rewrite the gateway-timeout test with mocker AsyncMock/MagicMock
  instead of a hand-rolled fake client (no empty method, no async-without-await).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-06-11 05:36:47 +02:00
co-authored by Claude Opus 4.8
parent 523e4cb7b5
commit 64ea5c8631
4 changed files with 63 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/document_processors/ocr.py
+14 -6
View File
@@ -95,8 +95,9 @@ class _GatewayOcrBackend(_OcrBackend):
"document_b64": base64.b64encode(content).decode("ascii"),
"mime_type": mime_type,
}
# Resolve the timeout per call (get_settings builds fresh, so a test or
# tenant override is honoured without a restart).
# Resolved per call (get_settings builds fresh) so test monkeypatching is
# honoured; a live tenant change still needs a restart because the backend
# instance itself is cached for the pod's lifetime.
ocr_timeout = get_settings().document_ocr_timeout_seconds
async with httpx.AsyncClient(
timeout=httpx.Timeout(ocr_timeout, connect=_OCR_CONNECT_TIMEOUT_SECONDS)
@@ -125,10 +126,17 @@ class _MistralOcrBackend(_OcrBackend):
data_url = (
f"data:{mime_type};base64,{base64.b64encode(content).decode('ascii')}"
)
resp = await self._client.ocr.process_async(
model=self._model,
document={"type": "document_url", "document_url": data_url},
)
# Apply DOCUMENT_OCR_TIMEOUT_SECONDS uniformly with the gateway backend.
# The Mistral SDK manages its own httpx client, so wrap the call in an
# anyio cancel-scope timeout rather than threading a per-request timeout
# through the SDK; on expiry this raises TimeoutError, which the
# OcrProcessor turns into a clean parse failure.
ocr_timeout = get_settings().document_ocr_timeout_seconds
with anyio.fail_after(ocr_timeout):
resp = await self._client.ocr.process_async(
model=self._model,
document={"type": "document_url", "document_url": data_url},
)
pages = [(p.index, p.markdown or "") for p in (resp.pages or [])]
return _pages_to_text(pages)
nextcloud_mcp_server/document_processors/registry.py
+7 -1
View File
@@ -205,7 +205,13 @@ class ProcessorRegistry:
# Pre-parse size guard: a pathologically large PDF (e.g. a 42 MB scanned
# DUDE) burns the OCR timeout for 0 chars. Fail fast with an explicit
# reason so the caller marks the placeholder "failed" instead of
# retrying. 0 disables the cap.
# retrying. 0 disables the cap. This lives on the auto-tiered path only:
# an explicit processor_name="ocr" override (registry.process) bypasses
# _process_pdf entirely and is intentionally not size-gated (power-user
# escape hatch). Returning here also skips _run_processor, so the
# rejection is counted on astrolabe_document_parse_failed_total{oversize}
# (via vector/processor.py) but deliberately not on the parse-duration
# histogram -- there is no parse to time.
max_pdf_mb = settings.document_max_pdf_size_mb
if max_pdf_mb > 0 and len(content) > max_pdf_mb * 1024 * 1024:
size_mb = len(content) / (1024 * 1024)
tests/unit/test_config.py
+4 -4
View File
@@ -219,17 +219,17 @@ class TestChunkConfigValidation:
Guards the _DEFAULTS-key-must-match-env-var footgun: a mismatch would
leave the override silently ignored.
"""
assert Settings().document_ocr_timeout_seconds == 180.0
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 == 45.0
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 == 50.0
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 == 12.5
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."""
tests/unit/test_ocr_processor.py
+38 -23
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
@@ -131,32 +132,25 @@ async def test_processor_backend_error_returns_success_false(monkeypatch):
assert r.metadata["parse_failed_reason"] == "error"
async def test_gateway_backend_uses_configured_timeout(monkeypatch):
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] = {}
class _FakeResponse:
def raise_for_status(self):
pass
def _make_client(*args, **kwargs):
captured["timeout"] = kwargs.get("timeout")
return client
def json(self):
return {"pages": [{"index": 0, "markdown": "ok"}]}
class _FakeClient:
def __init__(self, *, timeout=None, **kw):
captured["timeout"] = timeout
async def __aenter__(self):
return self
async def __aexit__(self, *exc):
return False
async def post(self, url, json=None, headers=None):
return _FakeResponse()
monkeypatch.setattr(ocr.httpx, "AsyncClient", _FakeClient)
monkeypatch.setattr(ocr.httpx, "AsyncClient", _make_client)
monkeypatch.setattr(
ocr, "get_settings", lambda: _settings(document_ocr_timeout_seconds=42.0)
)
@@ -165,5 +159,26 @@ async def test_gateway_backend_uses_configured_timeout(monkeypatch):
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 == 42.0
assert captured["timeout"].connect == 10.0
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")