Skip to content

fix(pdf): serialize PDFium rendering across parser instances - #5952

Merged
qin-ctx merged 1 commit into
mainfrom
fix/pdfium-render-lock
Oct 12, 2026
Merged

qin-ctx merged 1 commit into
mainfrom
fix/pdfium-render-lock

Conversation

@qin-ctx

@qin-ctx qin-ctx commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Description

Concurrent local PDF imports can call PDFium from multiple threads, even though PDFium is not thread-safe across documents.

For example, when two PDFs each contain an image, both imports previously could enter PDFium rendering at the same time. With this change, the image renders run serially under one shared lock, and each import still receives its PNG output.

Human Involvement

  • A human participated in the implementation or review loop
  • This PR was generated entirely by AI agents without human participation in the loop

Related Issue

Related to #5879; addresses the unsafe PDFium concurrency path only.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • Protect pdfplumber.to_image() with a module-level threading.Lock shared across parser instances. This call covers PDFium document opening, rendering, and closing.
  • Keep the lock scoped to rendering; text/table extraction and PNG encoding retain their existing execution paths.
  • Update the existing PNG extraction test to exercise two parser instances concurrently against a renderer that rejects overlapping calls.

Testing

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Updated one existing test; no new test cases or test files were added. The updated test fails without the lock and passes with it.

  • python -m pytest tests/parse/test_pdf_bookmark_extraction.py -q --no-cov --disable-warnings: 21 passed.
  • ruff check openviking/parse/parsers/pdf.py tests/parse/test_pdf_bookmark_extraction.py: passed.
  • ruff format --check openviking/parse/parsers/pdf.py tests/parse/test_pdf_bookmark_extraction.py: passed.
  • git diff --check: passed.

Checklist

  • My code follows the project's coding style
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Not applicable.

Additional Notes

This change does not alter queue replay behavior or provide process-level crash containment. The original Windows native crash in #5879 has not been reproduced locally; the regression test verifies serialization across parser instances.

Guard PDFium rendering with a shared thread lock to respect its process-wide thread-safety constraint. Extend the existing PNG extraction test to cover concurrent parser instances.

Related to #5879.
@qin-ctx
qin-ctx merged commit 92285e5 into main Oct 12, 2026
9 checks passed
@qin-ctx
qin-ctx deleted the fix/pdfium-render-lock branch October 12, 2026 13:48
q2000s pushed a commit to q2000s/OpenViking that referenced this pull request Oct 12, 2026
main advanced 510 commits while this branch was stacked on volcengine#4725. Resolved a
single import-order conflict in pdf.py (main added \	hreading\ for the PDFium
render lock in volcengine#5952; this branch added \	empfile\), keeping both.

No behavioural change from the resolution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants