fix(rag): guarantee forward progress in chunkText - #83
Open
Agnik47 wants to merge 1 commit into
Open
Conversation
`chunkText` advanced its window with `start = breakPoint + 1 - overlap`,
clamped only by `if (start < 0) start = 0`. Nothing kept that ahead of the
previous `start`. The half-chunk floor (`start + chunkSize * 0.5`) was
applied to the `". "` and `"\n"` candidates but not re-checked after the
`lastIndexOf(" ", end)` fallback — which is exactly the branch that yields
a break point close to `start`.
With the shipped CHUNK_SIZE 1600 / CHUNK_OVERLAP 320, any break point in
`(start, start + 319]` sends the next `start` backwards, and the clamp
parks it at 0 forever. Text carrying a long unbroken token — a URL, a
base64 blob, minified JSON, or CJK with no ASCII spaces — hangs the loop
while pushing a fresh sliver chunk every iteration, so the process spins
*and* grows until the heap is exhausted. `chunkText` runs inside
`RAGProvider.ingest` under `ConcurrentExecutor` with no timeout, and
ingest is checkpointed per session, so the hang reproduces at the same
session on every resume and the run can never make progress.
Apply the half-chunk floor to the word fallback too, so a degenerate break
point falls through to `breakPoint = end` and yields a genuine full-size
chunk instead of a sliver. Then clamp the step forward with
`Math.max(breakPoint + 1 - overlap, start + 1)`, which keeps the loop
terminating even for caller-supplied sizes where the overlap exceeds the
step. Normal prose is unaffected: its break points already clear the floor,
so the step is unchanged and chunks still end on sentence boundaries.
Export `chunkText` and cover it: the pathological input from the report,
full-size chunks instead of slivers, source coverage, an overlap larger
than the chunk step, and the existing sentence-boundary and overlap
behaviour. Note that on the unfixed code these hang rather than fail — the
loop is synchronous, so a per-test timeout cannot interrupt it.
Fixes supermemoryai#69
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XFXnxCW2dLbbC2Hi7Pi92A
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.
Fixes #69
The bug
chunkText(src/providers/rag/index.ts) advances its window withstart = breakPoint + 1 - overlap, clamped only byif (start < 0) start = 0. Nothing keeps that value ahead of the previousstart.The half-chunk floor (
start + chunkSize * 0.5) guards the". "and"\n"candidates, but it is not re-checked after thelastIndexOf(" ", end)fallback — which is precisely the branch that produces a break point close tostart. The only remaining guard there isbreakPoint <= start, so with the shippedCHUNK_SIZE = 1600/CHUNK_OVERLAP = 320any break point in(start, start + 319]pushes the nextstartbackwards, and the clamp parks it at0forever.Reproduced against
main— one early space followed by an unbroken run:startnever leaves0, and each iteration pushes a fresh ~10-character slice, so the process spins and grows until the heap is exhausted.Any ingested session containing a long unbroken token hits this: a URL, a base64 image, a stack trace, a minified payload, or CJK text (
lastIndexOf(" ")is whitespace-based).chunkTextruns insideRAGProvider.ingestunderConcurrentExecutorwith no timeout, and ingest is checkpointed per session — so the hang reproduces at the same session on every resume and the run can never make progress.The fix
Two changes, both in the loop:
breakPoint = end, producing a genuine full-size chunk instead of a sliver. (The droppedbreakPoint <= startclauses were redundant —breakPoint <= startalready impliesbreakPoint < start + chunkSize * 0.5.)start = Math.max(breakPoint + 1 - overlap, start + 1). The floor above already keeps the step positive at the default sizes; this keeps the loop terminating for caller-supplied sizes whereoverlapexceeds the step, and makes thestart < 0clamp unnecessary.Normal prose is unaffected. Its break points already clear the floor (
>= start + 800), so the step is>= start + 481andMath.maxpicks the unchanged value — chunks still end on sentence boundaries with the same overlap.Verification
bun test— 8 new tests pass insrc/providers/rag/chunking.test.ts: the reported input, full-size chunks rather than slivers, source coverage, an overlap larger than the chunk step, plus the existing sentence-boundary and overlap behaviour as regression guards.chunkSize1–2000,overlap0–2000, alphabets mixing spaces, periods, newlines, unbroken runs and CJK) all terminate with content preserved. Every one of these can hang onmain.tsc --noEmitclean forsrc/providers/rag.One note on the tests: against the unfixed code they hang rather than fail. The loop is synchronous, so bun's per-test timeout cannot interrupt it — that is the bug, not a flaw in the tests.
chunkTextis now exported so it can be tested directly; it is otherwise unchanged in signature and behaviour. The diff is scoped to the loop — I left the file's pre-existingprettier --checkwarning (an over-longlogger.infoline, present onmain) alone.