fix(tokenizer): drain consumed tokens in DecodeStream to bound memory#1638
fix(tokenizer): drain consumed tokens in DecodeStream to bound memory#1638slin1237 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 1 hour, 19 minutes, and 18 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesBounded token history in DecodeStream
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request bounds the memory usage of DecodeStream by draining consumed tokens below the prefix window after each successful step and rebasing the internal offsets. It also updates the documentation and existing tests to reflect this sliding-window behavior, and adds a new integration test to verify that the retained history remains bounded over many steps while still producing correct decoded output. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Clean, correct fix. The drain logic is safe — tokens before prefix_offset are provably never accessed again by step() or flush(). Offset rebasing preserves all invariants. No production callers of tokens() are affected by the semantic change. Tests cover both boundedness and output correctness over 1000 steps.
DecodeStream::step pushed every token into all_token_ids and never drained it, so retained history grew unbounded for the lifetime of a stream regardless of generation length. Drain tokens below the prefix window after each successful step and rebase prefix_offset/read_offset, mirroring decode_step/Sequence. The decode windows are preserved, so incremental output is unchanged; the public tokens() accessor now returns the retained sliding window. Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
ad879fc to
7dad7d3
Compare
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
Description
Problem
DecodeStream::stepappends every generated token to its internalall_token_idsbuffer but never drains it. The two offsets (prefix_offset,read_offset) advance, yet the consumed tokens belowprefix_offsetare kept for the entire lifetime of the stream. For a long-lived streaming response this is unbounded memory growth per stream — the buffer grows by one entry per generated token even though only the small prefix window is ever read by subsequentdecode()calls.This mirrors a class of issue already solved elsewhere in the crate: the
Decoder::decode_stepdefault andSequenceboth drain consumed tokens to keep memory bounded;DecodeStreamwas the outlier.Solution
After each successful step (the branch that emits a chunk and advances the offsets), drain the tokens below the prefix window and rebase both offsets:
The active decode windows (
prefix_offset..read_offsetandread_offset..) are preserved exactly, so incremental output andflush()behavior are unchanged. Only tokens that can no longer affect any future decode are removed. The publictokens()accessor now returns the retained sliding window rather than full history (documented), matchingSequence::token_ids().Changes
crates/tokenizer/src/stream.rs: drain consumed tokens belowprefix_offsetand rebase offsets after a successful step; updatetokens()doc to reflect the sliding-window semantics. MadeINITIAL_INCREMENTAL_DETOKENIZATION_OFFSETpub(crate)so the test can assert against the real window size.crates/tokenizer/src/tests.rs: addedtest_decode_stream_history_bounded(1000 steps, asserts the retained buffer stays within the detokenization window and that accumulated output equals a full decode); updated two existingtokens()assertions for the new draining behavior.crates/tokenizer/README.md: note that consumed tokens are drained so retained history stays bounded.Test Plan
Scenario: stream 1000 tokens through a single
DecodeStreamand assert (a) the retained token buffer never exceeds the detokenization window (INITIAL_INCREMENTAL_DETOKENIZATION_OFFSET + 1) regardless of total tokens, and (b) accumulated incremental output still equals a full decode of all tokens. Correctness is additionally guarded end-to-end by the existing real-tokenizer integration teststest_decode_streamandtest_long_sequence_incremental_decode_with_prefill, which compare accumulatedstep()output against the original prompt.Gate (sccache disabled, scoped to the changed crate
llm-tokenizer):From the codebase audit: crates/tokenizer/src/stream.rs:48 — DecodeStream never drains all_token_ids: unbounded memory per stream.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Bug Fixes
Documentation
Tests