From 93f0f4f881d504293ddfa2da7a9fe694115cb3d4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 4 Jun 2026 22:20:06 +0200 Subject: [PATCH] fix(review): type timeout as float; document worker reuse + identity check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #852 round 3 (all 🟡, no blockers): - config: DOCUMENT_PARSE_TIMEOUT_SECONDS is now float (default 120.0) so a fractional value is honoured rather than silently stored in an int field; matches anyio.move_on_after's float seconds. - _isolation: comment that a clean rlimit MemoryError leaves the worker alive in anyio's pool (vs the SIGKILL/BrokenWorkerProcess path that respawns) -- acceptable since RLIMIT_AS caps virtual address space, not RSS. - processor: note the `if indexed is False` is a deliberate identity check -- a successful index (incl. dedup hit) returns None and must not be mistaken for a parse failure. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/config.py | 6 ++++-- nextcloud_mcp_server/document_processors/_isolation.py | 5 +++++ nextcloud_mcp_server/vector/processor.py | 3 +++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index a2bb3a27..08a79081 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -134,7 +134,7 @@ _DEFAULTS: dict[str, Any] = { "document_chunk_overlap": 200, # PDF parse isolation (OOM guard) "document_pdf_graphics_limit": 5000, - "document_parse_timeout_seconds": 120, + "document_parse_timeout_seconds": 120.0, "document_parse_mem_limit_mb": 1536, # Observability "metrics_enabled": True, @@ -716,7 +716,9 @@ class Settings: # >=1 -- pymupdf4llm treats 0 as "no cap", which re-exposes the OOM. document_pdf_graphics_limit: int = 5000 # wall-clock cap per parse; the worker subprocess is killed on timeout. - document_parse_timeout_seconds: int = 120 + # float so a fractional DOCUMENT_PARSE_TIMEOUT_SECONDS is honoured, matching + # anyio.move_on_after's float seconds. + document_parse_timeout_seconds: float = 120.0 # RLIMIT_AS in the parse subprocess (below the pod limit). Applied once per # worker for its lifetime, so changing it needs a pod restart. document_parse_mem_limit_mb: int = 1536 diff --git a/nextcloud_mcp_server/document_processors/_isolation.py b/nextcloud_mcp_server/document_processors/_isolation.py index f452124f..f030bbe4 100644 --- a/nextcloud_mcp_server/document_processors/_isolation.py +++ b/nextcloud_mcp_server/document_processors/_isolation.py @@ -115,6 +115,11 @@ async def run_isolated_pdf_parse( cancellable=True, ) except MemoryError as e: + # A clean rlimit breach: the worker raised MemoryError and stays + # ALIVE in anyio's pool (unlike the BrokenWorkerProcess/SIGKILL path, + # which spawns a fresh worker). Its heap may be slightly fragmented + # for the next document. Acceptable: RLIMIT_AS caps virtual address + # space (not RSS), so practical fragmentation risk is low. raise PdfParseFailed("oom", str(e)) from e except BrokenWorkerProcess as e: # Worker died without a clean exception (e.g. SIGKILL from the OS OOM diff --git a/nextcloud_mcp_server/vector/processor.py b/nextcloud_mcp_server/vector/processor.py index 6a825662..73a8c0c2 100644 --- a/nextcloud_mcp_server/vector/processor.py +++ b/nextcloud_mcp_server/vector/processor.py @@ -244,6 +244,9 @@ async def process_document( # document_parse_total{error}) and the placeholder marked # "failed". It is not an indexing event and not retryable, so # don't count it as a successful upsert/indexed document. + # Identity check, not `if not indexed`: a successful index + # (including a dedup hit) returns None, which must NOT be + # treated as a parse failure. if indexed is False: return