Locate compaction prefix boundary positionally to avoid dropping messages#524
Open
PratikDhanave wants to merge 1 commit into
Open
Locate compaction prefix boundary positionally to avoid dropping messages#524PratikDhanave wants to merge 1 commit into
PratikDhanave wants to merge 1 commit into
Conversation
…ages MessageIndex.Update located the end of the already-processed prefix by searching backward for the last message whose CONTENT equals lastProcessedMessage. When a message repeats (e.g. a user re-sending "continue", or any empty-content message), that latched the boundary onto a later duplicate, so the turns appended since the previous update were silently dropped from the index. Because the index is persisted across turns (state.MessageGroups), the loss compounds. Check the expected position (processedMessageCount-1) first, where lastProcessedMessage sits when nothing earlier was trimmed, and fall back to the backward search only when the prefix was actually trimmed — preserving the existing trim-tolerance / rebuild behavior. Adds a black-box test (repeated "continue") that drops messages before this change and passes after.
There was a problem hiding this comment.
Pull request overview
This PR fixes an incremental indexing bug in MessageIndex.Update where repeated message content could cause the “already-processed prefix” boundary to latch onto a later duplicate, silently dropping newly appended turns from the compaction index across updates.
Changes:
- Update
MessageIndex.Updateto prefer a positional boundary check before falling back to a backward search forlastProcessedMessage. - Add a regression test that reproduces the repeated-content drop (“continue” repeated across turns) and validates the fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| agent/compaction/index.go | Adjusts how the processed-prefix boundary is located to avoid dropping newly appended messages when content repeats. |
| agent/compaction/index_test.go | Adds a regression test to ensure repeated content across Update calls doesn’t drop appended turns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
80
to
+84
| processedMessageCount := index.includedRawMessageCount() | ||
| if index.lastProcessedMessage != nil && len(messages) >= processedMessageCount && messageContentEqual(messages[len(messages)-1], index.lastProcessedMessage) { | ||
| return | ||
| } | ||
|
|
||
| // Locate where the already-processed prefix ends. lastProcessedMessage sits | ||
| // at index processedMessageCount-1 when nothing earlier was trimmed, so check | ||
| // that expected position first. Matching only the last occurrence of its | ||
| // content would latch the boundary onto a later duplicate when a message is |
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.
Summary
MessageIndex.Update(agent/compaction/index.go) locates the end of the already-processed prefix by searching backward for the last message whose content equalslastProcessedMessage. When message content repeats, that latches the boundary onto a later duplicate, so the turns appended since the previous update are silently dropped from the index.Trigger (verified):
[user "hi", assistant "hello", user "continue"]→ indexed (3 messages).[user "hi", assistant "hello", user "continue", assistant "sure", user "continue"]— the user re-sent"continue".Updatefinds the last"continue"(index 4) as the boundary and appends nothing after it → the index has 3 messages, not 5."sure"and the new"continue"are lost.Message IDs are empty for
message.NewText, so the boundary match falls back to role+content equality (equality.go), and two empty-content messages also compare equal — so this fires for any repeated short message ("yes","ok","continue") or empty-content messages. The index is persisted viastate.MessageGroups, so the divergence compounds across turns.Fix
Check the expected position (
processedMessageCount - 1, wherelastProcessedMessagesits when nothing earlier was trimmed) first; fall back to the backward search only when that position doesn't match (i.e. the prefix was trimmed). This fixes the repeated-content drop while preserving the existing trim-tolerance / rebuild behavior.Public API
No exported symbols change.
Tests
Adds
TestMessageIndex_Update_RepeatedContentDoesNotDropMessages(black-box, inindex_test.go): a repeated"continue"across twoUpdatecalls. It reports[hi hello continue](dropped) before this change and the full[hi hello continue sure continue]after. Full./agent/compactionsuite passes (trim-tolerance and other behavior unaffected).