fix(readers): reject chunk overlap that prevents forward progress - #1354
Open
linhongyu510 wants to merge 1 commit into
Open
linhongyu510 wants to merge 1 commit into
linhongyu510 wants to merge 1 commit into
Conversation
`chunk_pdf` and `chunk_code_text` advance their buffers with `buffer[chunk_chars - overlap:]`. When overlap equals chunk_chars, the slice starts at zero, the buffer never shrinks, and both functions loop forever. The configuration reaches these functions directly through `ParsingSettings.reader_config`, which is an unconstrained `dict[str, Any]`, so there is no earlier validation. With chunk_chars=100 and overlap=100, both real functions failed to return within 20 seconds; with overlap=50 they returned 99 chunks. Validate the shared chunking contract in all three chunkers: - chunk_chars must be positive (zero is the documented no-chunking sentinel and is handled by read_doc before dispatch) - overlap must be non-negative and strictly less than chunk_chars Validating `chunk_text` too keeps the same reader_config semantics across PDF, text, and code inputs instead of letting an invalid configuration hang only some file types. The parameterized regression tests cover all three chunkers and four invalid boundaries: equal overlap, larger overlap, negative overlap, and zero size.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What hangs
chunk_pdfandchunk_code_textadvance their buffers with the same expression:When
overlap == chunk_chars, the slice starts at zero. The buffer never shrinks, so bothwhile len(buffer) > chunk_charsloops run forever.ParsingSettings.reader_configis an unconstraineddict[str, Any]and is passed directly into these functions byread_doc, so no earlier validation prevents this configuration.Reproduction with the real functions
I ran each function in its own spawned process and used a parent-side timeout so a hung child could not stall the probe itself. The same 5,000-character input and
chunk_chars=100were used for the control and boundary cases:This is not merely a slow edge case: the step is exactly zero, so no iteration can make progress.
Fix
Add one shared chunk-parameter validator:
chunk_chars > 00 <= overlap < chunk_charsand call it from
chunk_pdf,chunk_text, andchunk_code_text.The infinite loop exists in the PDF/Office/image and code paths.
chunk_textdoes not use the same shrinking loop, but validating it too is intentional: the samereader_configis dispatched by file extension, and an invalid configuration should not hang PDFs/code while being silently accepted for.txt/.htmlfiles.chunk_chars=0remains supported through its documented meaning:read_dochandles it before dispatch and returns the full document without calling a chunker.The code fails fast with an actionable
ValueError; it does not silently clamp or reinterpret the user's configuration.Tests
One parameterized regression test covers all three chunkers and four invalid boundaries:
That is 12 explicit cases. The test has a 5-second timeout so a reintroduction of the zero-step loop cannot hang CI indefinitely.
Reverse verification:
readers.pyrestored to baseline, the six non-hanging invalid cases (larger/negative overlap across all three chunkers) all fail withDID NOT RAISE ValueError.Verification
Full
tests/test_paperqa.py, compared against an unmodified checkout in the same environment:The failure/error sets are identical. They are existing environment-dependent tests requiring LLM credentials or unavailable Office parsing dependencies; this change adds exactly 12 passing tests and no new failures.
I also attempted the complete pre-commit collection. Its first run spent over ten minutes installing the many isolated hook environments, so I stopped that setup rather than reporting it as passed. The code-relevant gates from that configuration (Ruff, Black, and the local mypy hook) were run directly with the pinned versions and passed.
git diff --checkis clean anduv.lockis unchanged.AI disclosure
This change was prepared with AI assistance. The defect was identified from the zero-progress slice, reproduced using the real chunkers under isolated process timeouts, and verified against the repository's tests and quality gates by the contributor.