fix(parser): bound OCR worker init so offline scanned PDFs degrade, never hang (#298) - #311
Conversation
…ever hang When eng.traineddata is uncached AND OCR_LANG_PATH is unset, tesseract.js fetches the model from a CDN on first use. #290 already handles the fetch *rejection* (fail-fast -> pdf-ocr-unusable). The unhandled case was a connection accepted but never answered: createWorker never settles and the whole parse job hangs. Race worker init against a configurable timer (OCR_INIT_TIMEOUT_MS, default 30s). On timeout, reject with a typed ParserError -- which the existing applyOcrIfNeeded catch degrades to a pdf-ocr-unusable warning -- and terminate any late-resolving worker so no Tesseract process leaks. Wrapping is centralized at the init boundary, preserving the #290 fail-fast. A createWorker DI seam makes the stall unit-testable with no network or real traineddata. ADR-039 records the offline-OCR provisioning strategy: vendored traineddata via OCR_LANG_PATH, never a production CDN dependency. Closes #298 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds two complementary offline OCR safety mechanisms: a configurable worker-initialization timeout ( ChangesOffline OCR provisioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Invariant 2 (a rejecting worker init must keep degrading to a pdf-ocr-unusable warning, never throw) was only pinned at the recognizePdfPages surface (-> ParserError). Add the parsePdf-boundary assertion mirroring the offline `TypeError: fetch failed` case, so all three #298 invariants are pinned at the parser boundary, not internals. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…top OCR worker leak PR #311's bounded init timeout stops the offline hang but not a worker LEAK that a Codex (GPT-5.5) adversarial review surfaced: tesseract.js v7 spawns the worker thread synchronously BEFORE fetching eng.traineddata, so in the black-hole stall (connection accepted, never answered) createWorker never settles, terminateLater() never fires, and the already-spawned worker leaks. The timeout can't terminate a worker it has no handle to. Timeout and pre-flight are therefore complementary, not substitutes. Add an opt-in strict mode (OCR_REQUIRE_LOCAL_TRAINEDDATA, default false) that pre-flights local traineddata BEFORE spawning: if eng.traineddata is absent (probing langPath + cache for .traineddata and .traineddata.gz, matching tesseract.js v7's resolution), refuse via ParserError -> pdf-ocr-unusable WITHOUT invoking the worker factory. Default false preserves the convenient networked-dev behavior (CDN fetch on first run, bounded by the timeout); production sets it true and provisions local data. New DI seams (requireLocalTraineddata, hasLocalTraineddata) keep it fully offline-testable. Plumbed config -> ParseOptions -> PdfOcrOptions; env via z.stringbool. ADR-039 corrected: pre-flight is adopted as complementary to the timeout, with the filename contract pinned by a test; credits the PR #311 review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
#325) * docs(readme): sync capabilities to last month of merged PRs Reflect shipped work in the README's "Included Today", "API Surface", and MCP tool table, validated against the merged diffs and current main: - PDF ingest (text-layer + OCR + font-encoding recovery) accepted by POST /parse (#287, #290, #311) - coordination / E&O report + submittal register (#241, #269, #277, #282, #283, #284) and article-role tagging (#273) - onboarding pipeline: library import, editability review/override, reclassify, finalize/reopen, open-comments (#243, #247, #248, #249, #272) - spec/project soft-delete + restore (#257, #313), document concurrency (#197), revision/addendum manual rendering (#221), numbering profiles (#317, #322) - add missing MCP tools get_numbering_profile, submittal_register, open_comments_report; document GET /docs (Scalar) (#213, #285) - add Example Client pointer to examples/web_ui_demo (#225) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(roadmap): move shipped work to done; re-date to 2026-07-01 Reconcile the roadmap with merged reality (was stamped 2026-06-17). Moved from planned/in-progress to Included, each validated against the diff: - PDF ingest (#287, #290, #311) — remove from "Later" - deep paragraph nesting pr6/pr7 (#215) - revision nomenclature (#216) + revision/addendum manual rendering (#221) — the two "Near Term" Phase 2e items are done - coordination / E&O report, required-sections, article-role, submittal register (#239, #241, #269, #273, #277, #282, #283, #284) — new "Coordination and Semantics" section; removed "coordination report" from planned Phase 4 - onboarding APIs (#243, #247, #248, #249, #272) — API done; UI remains planned - soft-delete/withdraw (#257, #313), section-number format (#266, #271), external-content associations (#242), structural numbering profiles (#317) Kept as planned (foundation only): header/footer composition (#222, #314) and keynote surfacing (#315) — DB/AST exist, no resolution/render/export yet. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(architecture): reflect merged structural changes Update the architecture spec for shipped work, validated against the diffs and current schema/routes: - Tech Stack + Data Flow: Parse — PDF text-layer (unpdf/pdfjs-dist) + OCR (tesseract.js/@napi-rs/canvas) path and numberingProfileId override (#287, #290, #311, #317; ADR-034, ADR-039) - DB schema — specs.onboarding_status/withdrawn_at, projects.section_number_format /deleted_at/deleted_by, paragraphs.source_facts/classification/ editability_override; "Additional tables" summary for editing_conventions, paragraph_associations, required_sections, keynotes, header_footer_configs, numbering_profiles, revision_nomenclature_profiles (foundation-only tables flagged) (ADR-021/022/023/028/031/032; #187, #242) - new Coordination Report / E&O section (finding vocabulary) and Document Concurrency section (locks/optimistic/lifecycle) (#197, #241, #269, #277, #282, #283, #284; ADR-018, ADR-033/035/036/037) - AST meta.articleRole (#273, ADR-033); API-surface note pointing at the CI-enforced openapi.yaml + GET /docs; refreshed MCP tool list Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Why
The PDF OCR fallback (#246/#290) initializes the Tesseract worker via
Tesseract.createWorker('eng', 1, …). When the model cache is empty andOCR_LANG_PATHis unset,tesseract.jsfetcheseng.traineddatafrom a CDN onfirst use. Offline, two failure modes exist:
TypeError: fetch failed) → cleanpdf-ocr-unusablewarning.but never responds,
createWorkerneither resolves nor rejects, and the firstscanned-PDF parse job hangs forever — wedging a Piscina worker thread.
There was no bounded init timeout on the production OCR path.
What
initManagedRecognizerraces workercreation against a configurable timer. On timeout it rejects with a typed
ParserError, which the existingapplyOcrIfNeededcatch degrades to apdf-ocr-unusablewarning — so a stall now completes within a bounded timeinstead of hanging. A late-resolving worker is terminated (
terminateLater) sono Tesseract process leaks.
OCR_INIT_TIMEOUT_MS(Zod-validated, positive int, default30000), threaded
parse-worker → ParseOptions → PdfOcrOptions.initTimeoutMs.Documented in
.env.example.boundary, so a rejecting init still surfaces a
ParserError→pdf-ocr-unusable.PdfOcrOptions.createWorker(mirrors the existingrenderPageAsImage/recognizeseams) makes the stall unit-testable bysimulating a never-resolving init — no network, no real traineddata.
Design decisions
The issue floated three options (bounded init timeout, local traineddata,
pre-flight check). ADR-039 records the call:
every "model not readily available" case (absent, unreadable, or hung) with
one mechanism. It is the only thing added to the hot path.
must not depend on a CDN: vendor
eng.traineddata(Apache-2.0) and pointOCR_LANG_PATHat it. The timeout is the safety net for when that is missed.The ~15 MB model is an ops artifact and is not committed to the repo.
resolution logic, risk drifting from it, and still not cover a path that exists
but whose fetch stalls — which the timeout already covers.
See
docs/adr/039-offline-ocr-provisioning.mdfor the full Context/Decision/Consequences, building on ADR-034.
Testing
pnpm lintclean (eslint + tsc + prettier)pnpm test— 1185 unit tests pass (withDATABASE_URL/NODE_ENVset, as CI does)ocr: worker init stall degrades to pdf-ocr-unusable within timeout, never hangs(parsePdf boundary) + focusedrecognizePdfPagestests for ParserError-on-stall, feat(parser): PDF OCR fallback + font-encoding recovery (#246) #290 fail-fast preservation, and no-leak late-worker termination — all offline, no real traineddataSPECR_OCR_E2E=1) unaffected — the bounded-timeout tests do not require network or real traineddata🤖 Co-authored by Claude Opus 4.8 (1M context). Closes #298.
Summary by CodeRabbit