From 64ea5c8631141b072ac2e4d11db730b41561f53d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 05:36:47 +0200 Subject: [PATCH] fix(document): apply OCR timeout to Mistral backend + review/Sonar fixes (#892) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../document_processors/ocr.py | 20 ++++-- .../document_processors/registry.py | 8 ++- tests/unit/test_config.py | 8 +-- tests/unit/test_ocr_processor.py | 61 ++++++++++++------- 4 files changed, 63 insertions(+), 34 deletions(-) diff --git a/nextcloud_mcp_server/document_processors/ocr.py b/nextcloud_mcp_server/document_processors/ocr.py index ee3fc064..4c90beba 100644 --- a/nextcloud_mcp_server/document_processors/ocr.py +++ b/nextcloud_mcp_server/document_processors/ocr.py @@ -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) diff --git a/nextcloud_mcp_server/document_processors/registry.py b/nextcloud_mcp_server/document_processors/registry.py index a4e84dee..b6c181a5 100644 --- a/nextcloud_mcp_server/document_processors/registry.py +++ b/nextcloud_mcp_server/document_processors/registry.py @@ -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) diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index c8ada4ec..492e85a0 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -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.""" diff --git a/tests/unit/test_ocr_processor.py b/tests/unit/test_ocr_processor.py index fdcdc1cb..a09844d0 100644 --- a/tests/unit/test_ocr_processor.py +++ b/tests/unit/test_ocr_processor.py @@ -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")