From 4c76101142ba2b453d4167801f7c16f3a1485c9b Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:37:51 +0200 Subject: [PATCH 1/9] fix: add document_timeout to PdfPipelineOptions (C1) Docling's native document_timeout is the only mechanism that can interrupt processing inside a blocked thread (OCR, table extraction). Without it, asyncio.wait_for cannot stop a frozen conversion. Configurable via DOCUMENT_TIMEOUT env var (default: 120s). Closes #57 (C1) --- document-parser/infra/local_converter.py | 2 ++ document-parser/infra/settings.py | 2 ++ document-parser/tests/test_pipeline_options.py | 1 + document-parser/tests/test_settings.py | 3 +++ 4 files changed, 8 insertions(+) diff --git a/document-parser/infra/local_converter.py b/document-parser/infra/local_converter.py index ba8875d..b469de1 100644 --- a/document-parser/infra/local_converter.py +++ b/document-parser/infra/local_converter.py @@ -42,6 +42,7 @@ from domain.value_objects import ( PageElement, ) from infra.bbox import to_topleft_list +from infra.settings import settings logger = logging.getLogger(__name__) @@ -102,6 +103,7 @@ def _build_docling_converter(options: ConversionOptions) -> DoclingConverter: generate_page_images=options.generate_page_images, generate_picture_images=options.generate_picture_images, images_scale=options.images_scale, + document_timeout=settings.document_timeout, ) return DoclingConverter( diff --git a/document-parser/infra/settings.py b/document-parser/infra/settings.py index 13ed454..b9f4b96 100644 --- a/document-parser/infra/settings.py +++ b/document-parser/infra/settings.py @@ -14,6 +14,7 @@ class Settings: docling_serve_url: str = "http://localhost:5001" docling_serve_api_key: str | None = None conversion_timeout: int = 900 + document_timeout: float = 120.0 # Docling-level per-document timeout (seconds) max_concurrent_analyses: int = 3 max_page_count: int = 0 # 0 = unlimited upload_dir: str = "./uploads" @@ -33,6 +34,7 @@ class Settings: docling_serve_url=os.environ.get("DOCLING_SERVE_URL", "http://localhost:5001"), docling_serve_api_key=os.environ.get("DOCLING_SERVE_API_KEY"), conversion_timeout=int(os.environ.get("CONVERSION_TIMEOUT", "900")), + document_timeout=float(os.environ.get("DOCUMENT_TIMEOUT", "120.0")), max_concurrent_analyses=int(os.environ.get("MAX_CONCURRENT_ANALYSES", "3")), max_page_count=int(os.environ.get("MAX_PAGE_COUNT", "0")), upload_dir=os.environ.get("UPLOAD_DIR", "./uploads"), diff --git a/document-parser/tests/test_pipeline_options.py b/document-parser/tests/test_pipeline_options.py index 2ce01f6..b52fdf4 100644 --- a/document-parser/tests/test_pipeline_options.py +++ b/document-parser/tests/test_pipeline_options.py @@ -53,6 +53,7 @@ class TestBuildConverter: assert opts.generate_page_images is False assert opts.generate_picture_images is False assert opts.images_scale == 1.0 + assert opts.document_timeout is not None def test_ocr_disabled(self): conv = build_converter(ConversionOptions(do_ocr=False)) diff --git a/document-parser/tests/test_settings.py b/document-parser/tests/test_settings.py index a6f600c..1b8837c 100644 --- a/document-parser/tests/test_settings.py +++ b/document-parser/tests/test_settings.py @@ -14,6 +14,7 @@ class TestSettingsDefaults: assert s.docling_serve_url == "http://localhost:5001" assert s.docling_serve_api_key is None assert s.conversion_timeout == 900 + assert s.document_timeout == 120.0 assert s.max_page_count == 0 assert s.upload_dir == "./uploads" assert s.db_path == "./data/docling_studio.db" @@ -36,6 +37,7 @@ class TestSettingsFromEnv: monkeypatch.setenv("DOCLING_SERVE_URL", "http://serve:9000") monkeypatch.setenv("DOCLING_SERVE_API_KEY", "secret-key") monkeypatch.setenv("CONVERSION_TIMEOUT", "120") + monkeypatch.setenv("DOCUMENT_TIMEOUT", "60.0") monkeypatch.setenv("MAX_PAGE_COUNT", "20") monkeypatch.setenv("UPLOAD_DIR", "/data/uploads") monkeypatch.setenv("DB_PATH", "/data/test.db") @@ -49,6 +51,7 @@ class TestSettingsFromEnv: assert s.docling_serve_url == "http://serve:9000" assert s.docling_serve_api_key == "secret-key" assert s.conversion_timeout == 120 + assert s.document_timeout == 60.0 assert s.max_page_count == 20 assert s.upload_dir == "/data/uploads" assert s.db_path == "/data/test.db" From c0fb1287183f6bf5ade6fbe015cfbc4c7773c435 Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:38:17 +0200 Subject: [PATCH 2/9] fix: replace _converter_lock with timeout-based acquisition (C2) A frozen conversion holding the lock indefinitely blocks all subsequent jobs. Using lock.acquire(timeout=300) fails fast with a clear error instead of waiting forever. Ref #57 (C2) --- document-parser/infra/local_converter.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/document-parser/infra/local_converter.py b/document-parser/infra/local_converter.py index b469de1..763b90d 100644 --- a/document-parser/infra/local_converter.py +++ b/document-parser/infra/local_converter.py @@ -46,8 +46,10 @@ from infra.settings import settings logger = logging.getLogger(__name__) -# Thread lock — DoclingConverter is not thread-safe +# Thread lock — DoclingConverter is not thread-safe. +# Uses a timeout to prevent a frozen conversion from blocking all others. _converter_lock = threading.Lock() +_LOCK_TIMEOUT = 300 # seconds — fail fast rather than wait forever # US Letter page dimensions (points) — fallback when page size is unknown _DEFAULT_PAGE_WIDTH = 612.0 @@ -212,9 +214,17 @@ def _process_content_item( def _convert_sync(file_path: str, options: ConversionOptions) -> ConversionResult: - with _converter_lock: + acquired = _converter_lock.acquire(timeout=_LOCK_TIMEOUT) + if not acquired: + raise TimeoutError( + f"Could not acquire converter lock within {_LOCK_TIMEOUT}s — " + "a previous conversion may be frozen" + ) + try: conv = _select_converter(options) result = conv.convert(file_path) + finally: + _converter_lock.release() doc = result.document page_count = len(doc.pages) From 6327b13614f592299d3f7f0ec583269137af4fd5 Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:39:20 +0200 Subject: [PATCH 3/9] fix: pass max_num_pages and max_file_size to Docling convert (C3) Defense-in-depth: even if upload validation passes, Docling itself now enforces page count and file size limits. Configurable via MAX_PAGE_COUNT and MAX_FILE_SIZE env vars (0 = unlimited). Ref #57 (C3) --- document-parser/infra/local_converter.py | 7 ++++++- document-parser/infra/settings.py | 4 +++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/document-parser/infra/local_converter.py b/document-parser/infra/local_converter.py index 763b90d..8d3eb75 100644 --- a/document-parser/infra/local_converter.py +++ b/document-parser/infra/local_converter.py @@ -222,7 +222,12 @@ def _convert_sync(file_path: str, options: ConversionOptions) -> ConversionResul ) try: conv = _select_converter(options) - result = conv.convert(file_path) + kwargs: dict = {} + if settings.max_page_count > 0: + kwargs["max_num_pages"] = settings.max_page_count + if settings.max_file_size > 0: + kwargs["max_file_size"] = settings.max_file_size + result = conv.convert(file_path, **kwargs) finally: _converter_lock.release() diff --git a/document-parser/infra/settings.py b/document-parser/infra/settings.py index b9f4b96..92c68b0 100644 --- a/document-parser/infra/settings.py +++ b/document-parser/infra/settings.py @@ -16,7 +16,8 @@ class Settings: conversion_timeout: int = 900 document_timeout: float = 120.0 # Docling-level per-document timeout (seconds) max_concurrent_analyses: int = 3 - max_page_count: int = 0 # 0 = unlimited + max_page_count: int = 0 # 0 = unlimited (upload validation) + max_file_size: int = 0 # 0 = unlimited (Docling-level, bytes) upload_dir: str = "./uploads" db_path: str = "./data/docling_studio.db" cors_origins: list[str] = field( @@ -37,6 +38,7 @@ class Settings: document_timeout=float(os.environ.get("DOCUMENT_TIMEOUT", "120.0")), max_concurrent_analyses=int(os.environ.get("MAX_CONCURRENT_ANALYSES", "3")), max_page_count=int(os.environ.get("MAX_PAGE_COUNT", "0")), + max_file_size=int(os.environ.get("MAX_FILE_SIZE", "0")), upload_dir=os.environ.get("UPLOAD_DIR", "./uploads"), db_path=os.environ.get("DB_PATH", "./data/docling_studio.db"), cors_origins=[o.strip() for o in cors_raw.split(",")], From f58b563a133a63b4328802eed14c0ff1a3479fed Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:40:11 +0200 Subject: [PATCH 4/9] fix: make default table_mode configurable via DEFAULT_TABLE_MODE (H1) TableFormerMode.ACCURATE is very expensive on CPU (~3-5x slower). The default can now be set to "fast" on resource-constrained environments (HF Spaces) via the DEFAULT_TABLE_MODE env var. User-specified table_mode in the request still takes precedence. Ref #57 (H1) --- document-parser/infra/settings.py | 2 ++ document-parser/services/analysis_service.py | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/document-parser/infra/settings.py b/document-parser/infra/settings.py index 92c68b0..ebb596c 100644 --- a/document-parser/infra/settings.py +++ b/document-parser/infra/settings.py @@ -16,6 +16,7 @@ class Settings: conversion_timeout: int = 900 document_timeout: float = 120.0 # Docling-level per-document timeout (seconds) max_concurrent_analyses: int = 3 + default_table_mode: str = "accurate" # "accurate" or "fast" max_page_count: int = 0 # 0 = unlimited (upload validation) max_file_size: int = 0 # 0 = unlimited (Docling-level, bytes) upload_dir: str = "./uploads" @@ -37,6 +38,7 @@ class Settings: conversion_timeout=int(os.environ.get("CONVERSION_TIMEOUT", "900")), document_timeout=float(os.environ.get("DOCUMENT_TIMEOUT", "120.0")), max_concurrent_analyses=int(os.environ.get("MAX_CONCURRENT_ANALYSES", "3")), + default_table_mode=os.environ.get("DEFAULT_TABLE_MODE", "accurate"), max_page_count=int(os.environ.get("MAX_PAGE_COUNT", "0")), max_file_size=int(os.environ.get("MAX_FILE_SIZE", "0")), upload_dir=os.environ.get("UPLOAD_DIR", "./uploads"), diff --git a/document-parser/services/analysis_service.py b/document-parser/services/analysis_service.py index 93c556f..4b62eaa 100644 --- a/document-parser/services/analysis_service.py +++ b/document-parser/services/analysis_service.py @@ -15,6 +15,7 @@ from typing import TYPE_CHECKING from domain.models import AnalysisJob, AnalysisStatus from domain.value_objects import ChunkingOptions, ChunkResult, ConversionOptions, ConversionResult +from infra.settings import settings if TYPE_CHECKING: from domain.ports import DocumentChunker, DocumentConverter @@ -151,7 +152,10 @@ class AnalysisService: await analysis_repo.update_status(job) logger.info("Analysis started: %s (file: %s)", job_id, filename) - options = ConversionOptions(**(pipeline_options or {})) + opts_dict = pipeline_options or {} + if "table_mode" not in opts_dict: + opts_dict = {**opts_dict, "table_mode": settings.default_table_mode} + options = ConversionOptions(**opts_dict) result: ConversionResult = await asyncio.wait_for( self._converter.convert(file_path, options), From 4b1a15f49a1b20713529c4c6318610f489821b0e Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:41:00 +0200 Subject: [PATCH 5/9] fix: add retry tolerance to frontend polling (H3) A single transient network error (502, timeout) no longer kills the polling loop. The frontend now tolerates up to 3 consecutive errors before abandoning. Successful fetches reset the counter. Also aligns frontend polling timeout (15 min) with backend timeout. Ref #57 (H3, M5) --- frontend/src/features/analysis/store.test.ts | 34 +++++++++++++++++++- frontend/src/features/analysis/store.ts | 17 +++++++--- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/frontend/src/features/analysis/store.test.ts b/frontend/src/features/analysis/store.test.ts index 6c6fb15..4806d00 100644 --- a/frontend/src/features/analysis/store.test.ts +++ b/frontend/src/features/analysis/store.test.ts @@ -154,17 +154,49 @@ describe('useAnalysisStore', () => { expect(store.currentAnalysis.status).toBe('FAILED') }) - it('polling stops on fetch error', async () => { + it('polling retries on transient errors and stops after MAX_POLL_RETRIES', async () => { const job = { id: 'j1', status: 'PENDING', documentId: 'd1' } api.createAnalysis.mockResolvedValue(job) api.fetchAnalysis.mockRejectedValue(new Error('network')) + vi.spyOn(console, 'warn').mockImplementation(() => {}) vi.spyOn(console, 'error').mockImplementation(() => {}) const store = useAnalysisStore() await store.run('d1') + // First two errors: still polling await vi.advanceTimersByTimeAsync(2000) + expect(store.running).toBe(true) + await vi.advanceTimersByTimeAsync(2000) + expect(store.running).toBe(true) + // Third error: stops polling + await vi.advanceTimersByTimeAsync(2000) + expect(store.running).toBe(false) + }) + + it('polling resets error count on successful fetch', async () => { + const job = { id: 'j1', status: 'PENDING', documentId: 'd1' } + api.createAnalysis.mockResolvedValue(job) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + + // Fail once, succeed, fail once — should NOT stop polling + api.fetchAnalysis + .mockRejectedValueOnce(new Error('network')) + .mockResolvedValueOnce({ ...job, status: 'RUNNING' }) + .mockRejectedValueOnce(new Error('network')) + .mockResolvedValueOnce({ ...job, status: 'COMPLETED' }) + + const store = useAnalysisStore() + await store.run('d1') + + await vi.advanceTimersByTimeAsync(2000) // error 1 + expect(store.running).toBe(true) + await vi.advanceTimersByTimeAsync(2000) // success — resets counter + expect(store.running).toBe(true) + await vi.advanceTimersByTimeAsync(2000) // error 1 again + expect(store.running).toBe(true) + await vi.advanceTimersByTimeAsync(2000) // success — COMPLETED expect(store.running).toBe(false) }) }) diff --git a/frontend/src/features/analysis/store.ts b/frontend/src/features/analysis/store.ts index 8891cb7..6c509a6 100644 --- a/frontend/src/features/analysis/store.ts +++ b/frontend/src/features/analysis/store.ts @@ -10,7 +10,8 @@ export const useAnalysisStore = defineStore('analysis', () => { const error = ref(null) const pollingInterval = ref | null>(null) const pollingTimeout = ref | null>(null) - const MAX_POLLING_DURATION = 10 * 60 * 1000 // 10 minutes + const MAX_POLLING_DURATION = 15 * 60 * 1000 // 15 minutes — aligned with backend timeout + const MAX_POLL_RETRIES = 3 const currentPages = computed(() => { if (!currentAnalysis.value?.pagesJson) return [] @@ -87,9 +88,11 @@ export const useAnalysisStore = defineStore('analysis', () => { function startPolling(id: string): void { stopPolling() + let consecutiveErrors = 0 pollingInterval.value = setInterval(async () => { try { const updated = await api.fetchAnalysis(id) + consecutiveErrors = 0 currentAnalysis.value = updated const idx = analyses.value.findIndex((a) => a.id === id) if (idx !== -1) analyses.value[idx] = updated @@ -98,10 +101,14 @@ export const useAnalysisStore = defineStore('analysis', () => { running.value = false } } catch (e) { - error.value = (e as Error).message || 'Polling error' - console.error('Polling error', e) - stopPolling() - running.value = false + consecutiveErrors++ + console.warn(`Polling error (${consecutiveErrors}/${MAX_POLL_RETRIES})`, e) + if (consecutiveErrors >= MAX_POLL_RETRIES) { + error.value = (e as Error).message || 'Polling error' + console.error('Polling abandoned after retries', e) + stopPolling() + running.value = false + } } }, 2000) pollingTimeout.value = setTimeout(() => { From f89dc51661ec36c0b40e88dae85c13ea338700c4 Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:41:20 +0200 Subject: [PATCH 6/9] fix: reset _default_converter on init failure (H5) If the lazy-init of the default converter fails (e.g. model download error), the singleton was left as None but subsequent calls would not retry. Now the failed state is cleared so the next request retries. Ref #57 (H5) --- document-parser/infra/local_converter.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/document-parser/infra/local_converter.py b/document-parser/infra/local_converter.py index 8d3eb75..a126e7b 100644 --- a/document-parser/infra/local_converter.py +++ b/document-parser/infra/local_converter.py @@ -118,7 +118,11 @@ def _build_docling_converter(options: ConversionOptions) -> DoclingConverter: def _get_default_converter() -> DoclingConverter: global _default_converter if _default_converter is None: - _default_converter = _build_docling_converter(ConversionOptions()) + try: + _default_converter = _build_docling_converter(ConversionOptions()) + except Exception: + _default_converter = None + raise return _default_converter From c281b6c5510064fdacbfb59ca780a88388b918eb Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:41:50 +0200 Subject: [PATCH 7/9] fix: classify pipeline errors into user-friendly messages (M1) Raw PyTorch/Docling stack traces are no longer shown to the user. Common failures (missing compiler, OOM, lock contention, corrupted document) are mapped to actionable messages. Unknown errors are truncated to 200 chars. Ref #57 (M1) --- document-parser/services/analysis_service.py | 28 +++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/document-parser/services/analysis_service.py b/document-parser/services/analysis_service.py index 4b62eaa..f630e95 100644 --- a/document-parser/services/analysis_service.py +++ b/document-parser/services/analysis_service.py @@ -191,7 +191,33 @@ class AnalysisService: except Exception as e: logger.exception("Analysis failed: %s", job_id) - await _mark_failed(job_id, str(e)) + await _mark_failed(job_id, _classify_error(e)) + + +def _classify_error(exc: Exception) -> str: + """Return a user-friendly error message based on the exception type/content.""" + msg = str(exc).lower() + + if "invalidcxxcompiler" in msg or "no working c++ compiler" in msg: + return "Missing C++ compiler — set TORCHDYNAMO_DISABLE=1 to work around this" + + if "out of memory" in msg or "oom" in msg: + return "Out of memory — try a smaller document or disable table structure analysis" + + if "could not acquire converter lock" in msg: + return "Server busy — a previous conversion is still running. Please retry later" + + if "pipeline" in msg and "failed" in msg: + return "Document processing failed — the document may be corrupted or unsupported" + + if "timeout" in msg: + return "Processing took too long — try with fewer pages or simpler options" + + # Fallback: truncate raw error to something reasonable + raw = str(exc) + if len(raw) > 200: + raw = raw[:200] + "…" + return raw _background_tasks: set[asyncio.Task] = set() From 6177452de16d55434e31e5d73a94a5e341c1fcd6 Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 14:58:26 +0200 Subject: [PATCH 8/9] test: add robustness tests for pipeline failure scenarios 25 tests covering all backend robustness fixes: - TestClassifyError (9): user-friendly error message mapping - TestDefaultTableMode (4): settings.default_table_mode injection - TestDocumentTimeout (2): document_timeout wired into pipeline - TestConverterLockTimeout (3): lock timeout + release guarantees - TestConvertSyncLimits (4): max_num_pages/max_file_size forwarding - TestGetDefaultConverterReset (3): converter singleton reset on failure Ref #57 --- document-parser/tests/test_robustness.py | 377 +++++++++++++++++++++++ 1 file changed, 377 insertions(+) create mode 100644 document-parser/tests/test_robustness.py diff --git a/document-parser/tests/test_robustness.py b/document-parser/tests/test_robustness.py new file mode 100644 index 0000000..9e33b29 --- /dev/null +++ b/document-parser/tests/test_robustness.py @@ -0,0 +1,377 @@ +"""Robustness tests — verify pipeline failure scenarios and protective mechanisms. + +Tests cover: document_timeout (C1), lock timeout (C2), convert limits (C3), +default table_mode (H1), converter reset on failure (H5), error classification (M1). +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from domain.models import AnalysisJob +from domain.value_objects import ConversionOptions, ConversionResult, PageDetail +from infra.settings import Settings +from services.analysis_service import AnalysisService, _classify_error + +# --------------------------------------------------------------------------- +# M1 — _classify_error: user-friendly error messages +# --------------------------------------------------------------------------- + + +class TestClassifyError: + """Verify _classify_error maps exceptions to user-friendly messages.""" + + def test_cxx_compiler_error(self): + exc = RuntimeError("InvalidCxxCompiler: No working C++ compiler found") + assert "Missing C++ compiler" in _classify_error(exc) + + def test_no_working_compiler(self): + exc = RuntimeError("no working c++ compiler found in torch") + assert "Missing C++ compiler" in _classify_error(exc) + + def test_out_of_memory(self): + exc = MemoryError("Out of memory allocating tensor") + assert "Out of memory" in _classify_error(exc) + + def test_oom_shorthand(self): + exc = RuntimeError("OOM during inference on page 5") + assert "Out of memory" in _classify_error(exc) + + def test_lock_timeout(self): + exc = TimeoutError("Could not acquire converter lock within 300s") + assert "Server busy" in _classify_error(exc) + + def test_pipeline_failed(self): + exc = RuntimeError("Pipeline StandardPdfPipeline failed on page 3") + assert "Document processing failed" in _classify_error(exc) + + def test_timeout_generic(self): + exc = TimeoutError("timeout exceeded while processing") + assert "Processing took too long" in _classify_error(exc) + + def test_unknown_short_error(self): + exc = ValueError("something weird happened") + assert _classify_error(exc) == "something weird happened" + + def test_unknown_long_error_truncated(self): + long_msg = "x" * 300 + exc = ValueError(long_msg) + result = _classify_error(exc) + assert len(result) <= 201 + assert result.endswith("…") + + +# --------------------------------------------------------------------------- +# H1 — default table_mode injection +# --------------------------------------------------------------------------- + + +class TestDefaultTableMode: + """Verify _run_analysis_inner injects settings.default_table_mode.""" + + @pytest.fixture + def mock_job(self): + return AnalysisJob(id="j1", document_id="d1", document_filename="test.pdf") + + def _make_result(self) -> ConversionResult: + return ConversionResult( + page_count=1, + content_markdown="# Test", + content_html="

Test

", + pages=[PageDetail(page_number=1, width=612.0, height=792.0)], + ) + + @patch("services.analysis_service.analysis_repo") + @patch("services.analysis_service.document_repo") + @pytest.mark.asyncio + async def test_default_table_mode_injected_when_missing( + self, mock_doc_repo, mock_analysis_repo, mock_job + ): + mock_analysis_repo.find_by_id = AsyncMock(return_value=mock_job) + mock_analysis_repo.update_status = AsyncMock() + mock_doc_repo.update_page_count = AsyncMock() + + mock_converter = AsyncMock() + mock_converter.convert.return_value = self._make_result() + + svc = AnalysisService(converter=mock_converter) + await svc._run_analysis("j1", "/tmp/test.pdf", "test.pdf", {}) + + opts = mock_converter.convert.call_args[0][1] + assert opts.table_mode == "accurate" + + @patch("services.analysis_service.analysis_repo") + @patch("services.analysis_service.document_repo") + @pytest.mark.asyncio + async def test_default_table_mode_injected_when_none( + self, mock_doc_repo, mock_analysis_repo, mock_job + ): + mock_analysis_repo.find_by_id = AsyncMock(return_value=mock_job) + mock_analysis_repo.update_status = AsyncMock() + mock_doc_repo.update_page_count = AsyncMock() + + mock_converter = AsyncMock() + mock_converter.convert.return_value = self._make_result() + + svc = AnalysisService(converter=mock_converter) + await svc._run_analysis("j1", "/tmp/test.pdf", "test.pdf", None) + + opts = mock_converter.convert.call_args[0][1] + assert opts.table_mode == "accurate" + + @patch("services.analysis_service.analysis_repo") + @patch("services.analysis_service.document_repo") + @pytest.mark.asyncio + async def test_user_table_mode_preserved(self, mock_doc_repo, mock_analysis_repo, mock_job): + mock_analysis_repo.find_by_id = AsyncMock(return_value=mock_job) + mock_analysis_repo.update_status = AsyncMock() + mock_doc_repo.update_page_count = AsyncMock() + + mock_converter = AsyncMock() + mock_converter.convert.return_value = self._make_result() + + svc = AnalysisService(converter=mock_converter) + await svc._run_analysis("j1", "/tmp/test.pdf", "test.pdf", {"table_mode": "fast"}) + + opts = mock_converter.convert.call_args[0][1] + assert opts.table_mode == "fast" + + @patch("services.analysis_service.settings", Settings(default_table_mode="fast")) + @patch("services.analysis_service.analysis_repo") + @patch("services.analysis_service.document_repo") + @pytest.mark.asyncio + async def test_custom_default_from_settings(self, mock_doc_repo, mock_analysis_repo, mock_job): + mock_analysis_repo.find_by_id = AsyncMock(return_value=mock_job) + mock_analysis_repo.update_status = AsyncMock() + mock_doc_repo.update_page_count = AsyncMock() + + mock_converter = AsyncMock() + mock_converter.convert.return_value = self._make_result() + + svc = AnalysisService(converter=mock_converter) + await svc._run_analysis("j1", "/tmp/test.pdf", "test.pdf", {}) + + opts = mock_converter.convert.call_args[0][1] + assert opts.table_mode == "fast" + + +# --------------------------------------------------------------------------- +# Docling-dependent tests — skip if docling is not installed +# --------------------------------------------------------------------------- + +docling = pytest.importorskip("docling", reason="docling library not installed") + +from docling.datamodel.base_models import InputFormat # noqa: E402 + +from infra.local_converter import ( # noqa: E402 + _build_docling_converter as build_converter, +) +from infra.local_converter import ( # noqa: E402 + _convert_sync as convert_sync, +) +from infra.local_converter import ( # noqa: E402 + _get_default_converter as get_default_converter, +) + +# --------------------------------------------------------------------------- +# C1 — document_timeout in PdfPipelineOptions +# --------------------------------------------------------------------------- + + +class TestDocumentTimeout: + """Verify document_timeout from settings is wired into PdfPipelineOptions.""" + + def _get_pipeline_options(self, converter): + fmt_opt = converter.format_to_options[InputFormat.PDF] + return fmt_opt.pipeline_options + + def test_document_timeout_from_settings(self): + conv = build_converter(ConversionOptions()) + opts = self._get_pipeline_options(conv) + assert opts.document_timeout == 120.0 + + @patch("infra.local_converter.settings", Settings(document_timeout=45.0)) + def test_custom_document_timeout(self): + conv = build_converter(ConversionOptions()) + opts = self._get_pipeline_options(conv) + assert opts.document_timeout == 45.0 + + +# --------------------------------------------------------------------------- +# C2 — converter lock timeout +# --------------------------------------------------------------------------- + + +def _mock_docling_result(): + """Create a minimal mock that satisfies _convert_sync's result processing.""" + mock_result = MagicMock() + mock_result.document.pages = {} + mock_result.document.iterate_items.return_value = [] + mock_result.document.export_to_markdown.return_value = "" + mock_result.document.export_to_html.return_value = "" + mock_result.document.export_to_dict.return_value = {} + return mock_result + + +class TestConverterLockTimeout: + """Verify _convert_sync raises TimeoutError when lock cannot be acquired.""" + + @patch("infra.local_converter._converter_lock") + def test_lock_timeout_raises(self, mock_lock): + mock_lock.acquire.return_value = False + + with pytest.raises(TimeoutError, match="Could not acquire converter lock"): + convert_sync("/tmp/test.pdf", ConversionOptions()) + + mock_lock.release.assert_not_called() + + @patch("infra.local_converter._select_converter") + @patch("infra.local_converter._converter_lock") + def test_lock_released_on_success(self, mock_lock, mock_select): + mock_lock.acquire.return_value = True + mock_conv = MagicMock() + mock_conv.convert.return_value = _mock_docling_result() + mock_select.return_value = mock_conv + + convert_sync("/tmp/test.pdf", ConversionOptions()) + + mock_lock.release.assert_called_once() + + @patch("infra.local_converter._select_converter") + @patch("infra.local_converter._converter_lock") + def test_lock_released_on_error(self, mock_lock, mock_select): + mock_lock.acquire.return_value = True + mock_conv = MagicMock() + mock_conv.convert.side_effect = RuntimeError("boom") + mock_select.return_value = mock_conv + + with pytest.raises(RuntimeError, match="boom"): + convert_sync("/tmp/test.pdf", ConversionOptions()) + + mock_lock.release.assert_called_once() + + +# --------------------------------------------------------------------------- +# C3 — max_num_pages and max_file_size forwarding +# --------------------------------------------------------------------------- + + +class TestConvertSyncLimits: + """Verify _convert_sync forwards limit settings to conv.convert().""" + + @patch("infra.local_converter._select_converter") + @patch("infra.local_converter._converter_lock") + @patch("infra.local_converter.settings", Settings(max_page_count=10, max_file_size=0)) + def test_max_page_count_forwarded(self, mock_lock, mock_select): + mock_lock.acquire.return_value = True + mock_conv = MagicMock() + mock_conv.convert.return_value = _mock_docling_result() + mock_select.return_value = mock_conv + + convert_sync("/tmp/test.pdf", ConversionOptions()) + + kwargs = mock_conv.convert.call_args.kwargs + assert kwargs["max_num_pages"] == 10 + assert "max_file_size" not in kwargs + + @patch("infra.local_converter._select_converter") + @patch("infra.local_converter._converter_lock") + @patch("infra.local_converter.settings", Settings(max_page_count=0, max_file_size=5_000_000)) + def test_max_file_size_forwarded(self, mock_lock, mock_select): + mock_lock.acquire.return_value = True + mock_conv = MagicMock() + mock_conv.convert.return_value = _mock_docling_result() + mock_select.return_value = mock_conv + + convert_sync("/tmp/test.pdf", ConversionOptions()) + + kwargs = mock_conv.convert.call_args.kwargs + assert kwargs["max_file_size"] == 5_000_000 + assert "max_num_pages" not in kwargs + + @patch("infra.local_converter._select_converter") + @patch("infra.local_converter._converter_lock") + @patch( + "infra.local_converter.settings", + Settings(max_page_count=20, max_file_size=10_000_000), + ) + def test_both_limits_forwarded(self, mock_lock, mock_select): + mock_lock.acquire.return_value = True + mock_conv = MagicMock() + mock_conv.convert.return_value = _mock_docling_result() + mock_select.return_value = mock_conv + + convert_sync("/tmp/test.pdf", ConversionOptions()) + + kwargs = mock_conv.convert.call_args.kwargs + assert kwargs["max_num_pages"] == 20 + assert kwargs["max_file_size"] == 10_000_000 + + @patch("infra.local_converter._select_converter") + @patch("infra.local_converter._converter_lock") + @patch("infra.local_converter.settings", Settings(max_page_count=0, max_file_size=0)) + def test_zero_limits_not_forwarded(self, mock_lock, mock_select): + mock_lock.acquire.return_value = True + mock_conv = MagicMock() + mock_conv.convert.return_value = _mock_docling_result() + mock_select.return_value = mock_conv + + convert_sync("/tmp/test.pdf", ConversionOptions()) + + kwargs = mock_conv.convert.call_args.kwargs + assert "max_num_pages" not in kwargs + assert "max_file_size" not in kwargs + + +# --------------------------------------------------------------------------- +# H5 — _default_converter reset on init failure +# --------------------------------------------------------------------------- + + +class TestGetDefaultConverterReset: + """Verify _get_default_converter resets on failure and retries on next call.""" + + @pytest.fixture(autouse=True) + def reset_default_converter(self): + import infra.local_converter as mod + + original = mod._default_converter + mod._default_converter = None + yield + mod._default_converter = original + + @patch("infra.local_converter._build_docling_converter") + def test_init_failure_resets_to_none(self, mock_build): + import infra.local_converter as mod + + mock_build.side_effect = RuntimeError("torch not found") + + with pytest.raises(RuntimeError, match="torch not found"): + get_default_converter() + + assert mod._default_converter is None + + @patch("infra.local_converter._build_docling_converter") + def test_retry_after_failure_succeeds(self, mock_build): + mock_conv = MagicMock() + mock_build.side_effect = [RuntimeError("torch not found"), mock_conv] + + with pytest.raises(RuntimeError): + get_default_converter() + + result = get_default_converter() + assert result is mock_conv + assert mock_build.call_count == 2 + + @patch("infra.local_converter._build_docling_converter") + def test_success_caches_converter(self, mock_build): + mock_conv = MagicMock() + mock_build.return_value = mock_conv + + result1 = get_default_converter() + result2 = get_default_converter() + + assert result1 is result2 is mock_conv + mock_build.assert_called_once() From f04e5369eff3ab322582796eaeff4340716a465b Mon Sep 17 00:00:00 2001 From: Pier-Jean Malandrino Date: Tue, 7 Apr 2026 15:39:02 +0200 Subject: [PATCH 9/9] style: use single import style for infra.local_converter in tests Use `import infra.local_converter as lc_mod` consistently instead of mixing `import` and `from ... import` for the same module. Addresses CodeQL review comment on PR #58. --- document-parser/tests/test_robustness.py | 26 ++++++++---------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/document-parser/tests/test_robustness.py b/document-parser/tests/test_robustness.py index 9e33b29..0f48f75 100644 --- a/document-parser/tests/test_robustness.py +++ b/document-parser/tests/test_robustness.py @@ -165,15 +165,11 @@ docling = pytest.importorskip("docling", reason="docling library not installed") from docling.datamodel.base_models import InputFormat # noqa: E402 -from infra.local_converter import ( # noqa: E402 - _build_docling_converter as build_converter, -) -from infra.local_converter import ( # noqa: E402 - _convert_sync as convert_sync, -) -from infra.local_converter import ( # noqa: E402 - _get_default_converter as get_default_converter, -) +import infra.local_converter as lc_mod # noqa: E402 + +build_converter = lc_mod._build_docling_converter +convert_sync = lc_mod._convert_sync +get_default_converter = lc_mod._get_default_converter # --------------------------------------------------------------------------- # C1 — document_timeout in PdfPipelineOptions @@ -335,23 +331,19 @@ class TestGetDefaultConverterReset: @pytest.fixture(autouse=True) def reset_default_converter(self): - import infra.local_converter as mod - - original = mod._default_converter - mod._default_converter = None + original = lc_mod._default_converter + lc_mod._default_converter = None yield - mod._default_converter = original + lc_mod._default_converter = original @patch("infra.local_converter._build_docling_converter") def test_init_failure_resets_to_none(self, mock_build): - import infra.local_converter as mod - mock_build.side_effect = RuntimeError("torch not found") with pytest.raises(RuntimeError, match="torch not found"): get_default_converter() - assert mod._default_converter is None + assert lc_mod._default_converter is None @patch("infra.local_converter._build_docling_converter") def test_retry_after_failure_succeeds(self, mock_build):