fix(memory): correct off-by-one in memory flush cycle dedup#26145
fix(memory): correct off-by-one in memory flush cycle dedup#26145drvoss wants to merge 1 commit into
Conversation
| if (memoryCompactionCompleted) { | ||
| const nextCount = await incrementCompactionCount({ | ||
| await incrementCompactionCount({ | ||
| sessionEntry: activeSessionEntry, | ||
| sessionStore: activeSessionStore, | ||
| sessionKey: params.sessionKey, | ||
| storePath: params.storePath, | ||
| }); | ||
| if (typeof nextCount === "number") { | ||
| memoryFlushCompactionCount = nextCount; | ||
| } | ||
| // Do not update memoryFlushCompactionCount here: keeping it at the | ||
| // pre-flush compactionCount ensures shouldRunMemoryFlush sees a mismatch | ||
| // on the next turn and runs again if tokens are still high. | ||
| } |
There was a problem hiding this comment.
Integration test not updated for new behavior
The fix correctly stops updating memoryFlushCompactionCount to the post-increment value, but the integration test in agent-runner.runreplyagent.test.ts:1618 still asserts the old behavior:
expect(stored[sessionKey].memoryFlushCompactionCount).toBe(2);
With this fix, when compaction completes (bumping compactionCount from 1 → 2), memoryFlushCompactionCount should remain at 1 (the pre-flush value). The assertion at line 1618 should be updated to .toBe(1) to match the intended behavior. As-is, the test "increments compaction count when flush compaction completes" will fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/agent-runner-memory.ts
Line: 139-149
Comment:
**Integration test not updated for new behavior**
The fix correctly stops updating `memoryFlushCompactionCount` to the post-increment value, but the integration test in `agent-runner.runreplyagent.test.ts:1618` still asserts the old behavior:
```
expect(stored[sessionKey].memoryFlushCompactionCount).toBe(2);
```
With this fix, when compaction completes (bumping `compactionCount` from 1 → 2), `memoryFlushCompactionCount` should remain at `1` (the pre-flush value). The assertion at line 1618 should be updated to `.toBe(1)` to match the intended behavior. As-is, the test "increments compaction count when flush compaction completes" will fail.
How can I resolve this? If you propose a fix, please make it concise.1af1a73 to
97c171c
Compare
89db58e to
0e5eee9
Compare
0e5eee9 to
fb76e31
Compare
c64bdc4 to
8e30857
Compare
|
This pull request has been automatically marked as stale due to inactivity. |
|
Updating this PR to keep it active. The fix is still valid and ready for review. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8e30857 to
70bd99f
Compare
|
Rebased onto current main with a clean single-commit branch. Removes the post-increment memoryFlushCompactionCount = nextCount update to fix the off-by-one in flush cycle dedup. |
|
Closing to reduce active PR count. Will rebase and resubmit if still needed. |
Summary
The memory flush cycle had an off-by-one bug: after flushing triggered a compaction, memoryFlushCompactionCount was updated to the new compaction count, making the dedup check skip every other flush cycle.
The fix keeps memoryFlushCompactionCount at the pre-flush value so the next cycle correctly detects a mismatch and runs the flush.
Change Type
Scope
Linked Issue
Relates to #12590
Security Impact
No security impact. Memory management logic only.
Human Verification
AI-Assisted
This PR was prepared with AI assistance.
Greptile Summary
Corrected the memory flush deduplication logic in
agent-runner-memory.ts:139-148by removing the update tomemoryFlushCompactionCountafter a flush-triggered compaction. Previously, when a flush triggered a compaction, the code updatedmemoryFlushCompactionCountto the post-increment value, causingshouldRunMemoryFlushto see matching counts on the next cycle and skip the flush—resulting in flushes running only every other eligible cycle.The fix keeps
memoryFlushCompactionCountat the pre-flush value so the next cycle correctly detects a mismatch betweencompactionCountandmemoryFlushCompactionCount, ensuring flushes run on every eligible cycle when tokens remain high.Changes:
nextCountreturn value and the conditional update tomemoryFlushCompactionCountmemoryFlushCompactionCountshould not be updatedreply-state.test.ts:322-334to verify the flush-triggered compaction scenariosetSessionRuntimeModelmock inrun.skill-filter.test.ts:116(unrelated test maintenance)Confidence Score: 5/5
memoryFlushCompactionCountat the pre-flush value ensures the deduplication check works correctly on subsequent cycles. The new test case validates the exact scenario that was broken, and the change is minimal and well-documented with an explanatory comment.Last reviewed commit: c64bdc4