From 2f8875e736e340c3c8cc31ac3d0ce3f89458bc5d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 11 Jun 2026 06:12:21 +0200 Subject: [PATCH] fix(document): timeout reason bucket + Sonar https hotspot (#892 round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review on PR #892: - OcrProcessor.process now catches TimeoutError separately and returns parse_failed_reason="timeout" with a populated message ("OCR timed out after Ns"), instead of conflating timeouts with API errors under "error" and logging an empty suffix. Lets dashboards tell a too-low timeout from a failing provider. Test added. - Add validator-rejection tests for DOCUMENT_OCR_TIMEOUT_SECONDS=0 (gte=1) and DOCUMENT_MAX_PDF_SIZE_MB=-1 (gte=0), matching the existing validator-test pattern. - Comment the _Settings test fixture's max_pdf_size_mb=0.0 default. SonarCloud: quality gate was failing on new_security_hotspots_reviewed (S5332 "use https") from an http:// URL in the new gateway-timeout test — switched to https:// (mirrors commit 98c9d58e). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../document_processors/ocr.py | 16 ++++++++++++++++ tests/unit/test_config.py | 16 ++++++++++++++++ tests/unit/test_ocr_processor.py | 19 ++++++++++++++++++- tests/unit/test_registry_tiering.py | 2 ++ 4 files changed, 52 insertions(+), 1 deletion(-) diff --git a/nextcloud_mcp_server/document_processors/ocr.py b/nextcloud_mcp_server/document_processors/ocr.py index 4c90beba..7f08cb32 100644 --- a/nextcloud_mcp_server/document_processors/ocr.py +++ b/nextcloud_mcp_server/document_processors/ocr.py @@ -264,6 +264,22 @@ class OcrProcessor(DocumentProcessor): text, boundaries = await backend.ocr( content, content_type.split(";")[0].strip().lower() ) + except TimeoutError: + # anyio.fail_after / httpx read-timeout raise TimeoutError with an + # empty message; give it its own reason bucket and a useful log so a + # too-low DOCUMENT_OCR_TIMEOUT_SECONDS is distinguishable from a + # provider that's actually erroring. + timeout = settings.document_ocr_timeout_seconds + logger.warning( + "OCR timed out for %s after %.1fs", filename or "", timeout + ) + return ProcessingResult( + text="", + metadata={"parse_failed_reason": "timeout"}, + processor=self.name, + success=False, + error=f"OCR timed out after {timeout:.1f}s", + ) except Exception as e: logger.warning("OCR failed for %s: %s", filename or "", e) return ProcessingResult( diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 492e85a0..2f7cc371 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -509,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.""" diff --git a/tests/unit/test_ocr_processor.py b/tests/unit/test_ocr_processor.py index a09844d0..d13a42a9 100644 --- a/tests/unit/test_ocr_processor.py +++ b/tests/unit/test_ocr_processor.py @@ -132,6 +132,23 @@ async def test_processor_backend_error_returns_success_false(monkeypatch): 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_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.""" @@ -155,7 +172,7 @@ async def test_gateway_backend_uses_configured_timeout(mocker, monkeypatch): ocr, "get_settings", lambda: _settings(document_ocr_timeout_seconds=42.0) ) - backend = ocr._GatewayOcrBackend("http://gw", "mistral/mistral-ocr-latest") + 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. diff --git a/tests/unit/test_registry_tiering.py b/tests/unit/test_registry_tiering.py index 818b0cad..c111763d 100644 --- a/tests/unit/test_registry_tiering.py +++ b/tests/unit/test_registry_tiering.py @@ -74,6 +74,8 @@ 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