fix(review): close doc via try/finally; don't count parse failures as indexed
Address PR #852 review: - pymupdf.py: the metadata `doc` was only closed on the PdfParseFailed and success paths, so a failure in `_extract_metadata`/`mkdir`/`get_settings` leaked it. `doc` is only needed for metadata + page_count (the heavy parse works from `content` bytes in the worker), so open it, read metadata, and close it immediately under try/finally; drop the two later doc.close() calls. - processor.py: a permanent parse failure early-returned from `_index_document`, after which `process_document` still recorded record_qdrant_operation("upsert", "success") + record_vector_sync_processing(success) -- counting an OOM/timeout bomb as astrolabe_documents_indexed_total{status="success"}. `_index_document` now returns False on that path and the caller skips the success metrics (the failure is already recorded via document_parse_failed_total + the registry's document_parse_total{error}). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
7db8d3e301
commit
7ec116a3c7
@@ -237,7 +237,15 @@ async def process_document(
|
||||
|
||||
for attempt in range(max_retries):
|
||||
try:
|
||||
await _index_document(doc_task, nc_client, qdrant_client)
|
||||
indexed = await _index_document(doc_task, nc_client, qdrant_client)
|
||||
|
||||
# A permanent parse failure returns False: it was already
|
||||
# recorded (document_parse_failed_total + the registry's
|
||||
# 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.
|
||||
if indexed is False:
|
||||
return
|
||||
|
||||
# Record successful processing metrics
|
||||
duration = time.time() - start_time
|
||||
@@ -534,8 +542,8 @@ async def _index_document(
|
||||
# on a pathological PDF) returns success=False rather than
|
||||
# raising -- there is nothing to index and retrying would just
|
||||
# fail again. Mark the placeholder "failed" so the scanner stops
|
||||
# re-queuing it (until the file changes) and return without
|
||||
# indexing empty content.
|
||||
# re-queuing it (until the file changes) and return False so the
|
||||
# caller skips the success metrics (it was not indexed).
|
||||
if not result.success:
|
||||
reason = result.metadata.get("parse_failed_reason", "error")
|
||||
record_document_parse_failed(reason)
|
||||
@@ -561,7 +569,7 @@ async def _index_document(
|
||||
doc_task.doc_id,
|
||||
exc_info=True,
|
||||
)
|
||||
return
|
||||
return False
|
||||
|
||||
content = result.text
|
||||
file_metadata = result.metadata
|
||||
|
||||
Reference in New Issue
Block a user