fix(llmloop): scope async memory compression to each RunPerFile conversation#10
fix(llmloop): scope async memory compression to each RunPerFile conversation#10chethanuk wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughBackground compression bookkeeping moved from shared ChangesConversation compression isolation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RunPerFile
participant addNextMessage
participant compressionState
participant compressionJob
RunPerFile->>compressionState: create per-conversation state
RunPerFile->>addNextMessage: pass compressionState
addNextMessage->>compressionState: check pendingJob
addNextMessage->>compressionJob: trigger async compression
compressionJob->>compressionState: update completed or failed job
addNextMessage->>compressionState: apply or cancel pending compression
RunPerFile->>compressionState: cancel pending job on conversation exit
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the background memory compression in the LLM tool-use loop to be scoped per conversation (within RunPerFile) rather than shared globally across the Runner session. This is done by introducing a compressionState struct to manage the pending job and mutex, ensuring cross-file isolation and preventing race conditions during concurrent file reviews. Additionally, async compression is now triggered only after all message appends are completed to avoid immediate cancellation. Feedback on the PR suggests adding a defensive check job.snapshotLen <= len(*messages) before applying rebuilt messages to prevent potential out-of-bounds panics if the message history is modified or shrunk while a background job is in flight.
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.
| r.compressionMu.Lock() | ||
| if r.pendingJob == job && job.rebuilt != nil { | ||
| st.mu.Lock() | ||
| if st.pendingJob == job && job.rebuilt != nil { |
There was a problem hiding this comment.
To ensure robust defensive programming, we should add a guard to verify that job.snapshotLen <= len(*messages) before attempting to apply the rebuilt messages. Although messages is currently only expected to grow during the active conversation loop, if any future refactoring or error-handling path shrinks the message history while a background compression job is in flight, job.snapshotLen > len(*messages) could cause a slice bounds out-of-range panic when executing (*messages)[job.snapshotLen:], or lead to inconsistent history state by restoring previously deleted messages.
| if st.pendingJob == job && job.rebuilt != nil { | |
| if st.pendingJob == job && job.rebuilt != nil && job.snapshotLen <= len(*messages) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/llmloop/loop.go`:
- Around line 444-446: Update the compression boundary logic around
CountMessagesTokens and the related finalCount handling so an exact warnLimit
value triggers synchronous compression before the stop decision, while
preserving asynchronous compression for values above the threshold. Add a
regression test covering finalCount == warnLimit and verify compression is
attempted before the function returns false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9f0c5c1-5cf5-4667-9832-d3049c23a269
📒 Files selected for processing (3)
internal/llmloop/compression.gointernal/llmloop/loop.gointernal/llmloop/runner_test.go
| if CountMessagesTokens(*messages) > warnLimit { | ||
| r.cancelPendingCompression(st) | ||
| *messages, _ = r.runCompression(ctx, *messages, filePath) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compress at the exact warning threshold before stopping.
When finalCount == warnLimit, synchronous compression is skipped, async compression is skipped, and the function returns false. This contradicts the contract that stopping occurs only after compression cannot bring the conversation below the threshold.
Proposed boundary fix
- if CountMessagesTokens(*messages) > warnLimit {
+ if CountMessagesTokens(*messages) >= warnLimit {
r.cancelPendingCompression(st)
*messages, _ = r.runCompression(ctx, *messages, filePath)
}
...
- if finalCount > warnLimit {
+ if finalCount >= warnLimit {
r.cancelPendingCompression(st)
*messages, _ = r.runCompression(ctx, *messages, filePath)Please add an exact-warnLimit regression case.
Also applies to: 461-473
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/llmloop/loop.go` around lines 444 - 446, Update the compression
boundary logic around CountMessagesTokens and the related finalCount handling so
an exact warnLimit value triggers synchronous compression before the stop
decision, while preserving asynchronous compression for values above the
threshold. Add a regression test covering finalCount == warnLimit and verify
compression is attempted before the function returns false.
…rsation The Runner is shared by all concurrent per-file review goroutines, but it held a single compressionMu/pendingJob slot for async memory compression. With concurrency > 1 that shared slot caused four defects (alibaba#384): 1. Cross-file apply: tryApplyPendingCompression had no owner check, so file B could splice file A's rebuilt history into its own messages. 2. Cross-file cancel/replace: the warning-threshold paths canceled whichever file's job happened to be pending, surfacing spurious "context canceled" errors at the gateway; triggerAsyncCompression overwrote the slot unconditionally, wasting the superseded request. 3. Same-call start-then-cancel: the soft-threshold trigger fired before the new messages were appended, so an append that crossed the warning threshold canceled the job started microseconds earlier. 4. The pendingJob == nil fast-path read in addNextMessage was unlocked. Fix: move the bookkeeping into a compressionState owned by each RunPerFile call and thread it through trigger/apply/cancel. The nil-pending gate is now an atomic check-and-set inside triggerAsyncCompression under st.mu, and the async trigger moved to the end of addNextMessage, gated on the post-append count sitting strictly between the soft and warning thresholds. RunPerFile defers a cancel so no job outlives its conversation. Aggregate token counters and warnings stay Runner-level. Intentional behavior deltas, all strictly safer: the async snapshot now includes the just-appended round; an in-flight job is canceled when RunPerFile returns instead of running up to 5 minutes orphaned; no async job starts when the call is about to return false. Verified: new regression tests (cross-file isolation via a channel-gated fake client, owner-only summary apply with post-snapshot suffix preserved, no start-then-cancel in one update, 4 concurrent RunPerFile calls under -race); all existing compression tests updated to the per-conversation API; full suite green with -race; coverage 81.1%. E2E: 2-file concurrent review against a local OpenAI-compatible stub with compression exercised — zero "context canceled", both files done. Fixes alibaba#384
b634d96 to
47a9222
Compare
1 participant
fix(llmloop): scope async memory compression to each RunPerFile conversation
Problem
One
Runnerserves a whole review session, and bothinternal/agentandinternal/scanfan out one goroutine per file over it (default concurrency 8). Async memory compression state, however, lived on the Runner as a single shared slot (compressionMu+pendingJob). As analyzed in alibaba#384 (and confirmed by the maintainer), that shared slot breaks per-conversation semantics once concurrency > 1.Root cause — four concrete defects
tryApplyPendingCompressionhad no owner check: file B could consume file A's completed job, splicing[frozen + A's summary + A's active rounds]into B's message history. A review-correctness bug, not just wasted work.context canceled(rendered as an HTTP 500 by some gateways);triggerAsyncCompressionalso overwrote the slot unconditionally, burning the superseded request.addNextMessagecall canceled the job it had started microseconds earlier (the cancellation-within-milliseconds seen in the issue).pendingJob == nilfast-path read inaddNextMessagewas unlocked.Fix
compressionState { mu sync.Mutex; pendingJob *compressionJob }, created perRunPerFilecall and threaded throughtriggerAsyncCompression/tryApplyPendingCompression/cancelPendingCompression. Isolation holds by construction: a conversation physically cannot see another conversation's job. The state is GC'd with the call — no cleanup maps, no keying by file path (which would collide if the same path is reviewed twice).triggerAsyncCompressionas an atomic check-and-set underst.mu(also fixes 4).addNextMessage, gated on the post-append count sitting strictly between the soft (60%) and warning (80%) thresholds — start-then-cancel within one update is now structurally impossible, and no job starts when the call is about to return false (also fixes 3).RunPerFiledeferscancelPendingCompression(st), so an in-flight job is aborted when its conversation ends instead of running up to 5 minutes orphaned.internal/agentandinternal/scanare untouched.Intentional behavior deltas (all strictly safer): the async snapshot now includes the just-appended round (strictly more history summarized); in-flight jobs are canceled at conversation end; no async job starts on a failing update.
Tests
New regression tests (channel-synchronized, no sleeps):
TestCompression_CrossFileIsolation└ SummaryAppliesOnlyToOwningConversation(subtest)TestAddNextMessage_NoStartThenCancelSameCallTestRunPerFile_ConcurrentFilesCompression_RaceRunPerFilecalls on one Runner crossing both thresholds repeatedly; asserts-racecleanliness end to end (defect 4)The RED baseline was demonstrated on unmodified
main: a gated-client test showed file B's cancel destroying file A's pending job (file A's job should still be pending after file B's cancel).All seven existing compression unit tests were updated mechanically to the per-conversation API; the remaining compression edge cases (empty template no-op, LLM error → messages unchanged, empty summary → unchanged, snapshot-suffix preservation, fence stripping) are unchanged and still green.
E2E
Mirroring the issue's setup: 2-file review at
--concurrency 2against a local OpenAI-compatible stub, with responses sized so each conversation crosses the soft threshold (async compression) and the warning threshold (sync compression). Result: both filesreview_item_done,memory_compression_taskrecords present for each file, and zerocontext canceled/Memory compression failedin stdout or session records.Fixes alibaba#384
Summary by CodeRabbit
Bug Fixes
Tests