-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: resolve timestamp collision in condense + rewind scenario #8299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Make summary timestamp unique by using keepMessages[0].ts - 1 to avoid collision - Change checkpoint restore filter from < to <= to include target message - Add regression test for condense + rewind scenario Fixes #8295
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review complete: I debated myself about a one-millisecond universe where timestamps collide and still demanded better tests.
| // Use keepMessages[0].ts - 1, but ensure we don't go before the first message | ||
| const firstMessageTs = firstMessage.ts || Date.now() | ||
| const firstKeptTs = keepMessages[0].ts || Date.now() | ||
| const summaryTs = Math.max(firstMessageTs + 1, firstKeptTs - 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Potential timestamp collision persists when firstKeptTs - firstMessageTs ≤ 1. In that case, Math.max(firstMessageTs + 1, firstKeptTs - 1) can evaluate to firstKeptTs, reintroducing the duplicate timestamp we’re trying to avoid. Consider ensuring summaryTs < firstKeptTs in all cases and staying within the known window.
| const summaryTs = Math.max(firstMessageTs + 1, firstKeptTs - 1) | |
| const firstMessageTs = firstMessage.ts ?? 0 | |
| const firstKeptTs = keepMessages[0].ts ?? Number.MAX_SAFE_INTEGER | |
| // Guarantee summaryTs is strictly before the first kept message; if there's no room (≤1ms gap), | |
| // fall back to firstMessageTs to avoid colliding with firstKeptTs. | |
| const summaryTs = (firstKeptTs - 1 > firstMessageTs) ? (firstKeptTs - 1) : firstMessageTs |
|
|
||
| // Make the summary timestamp unique to avoid collision with the first kept message | ||
| // Use keepMessages[0].ts - 1, but ensure we don't go before the first message | ||
| const firstMessageTs = firstMessage.ts || Date.now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] Using Date.now() as a fallback can place summaryTs outside the historical range if messages have older timestamps, potentially disrupting chronology. Prefer a deterministic bound within the known window (e.g., clamp relative to firstMessageTs and strictly less than firstKeptTs) so ordering remains stable regardless of wall-clock time.
| return { ...response, cost, error } | ||
| } | ||
|
|
||
| // Make the summary timestamp unique to avoid collision with the first kept message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P3] Comment/code intent mismatch: the correctness goal is to avoid collision with the first kept message (summaryTs < firstKeptTs), not specifically to avoid going before the first message. Consider updating the comment to clearly state the invariant (strictly before firstKeptTs, clamped to the known window).
|
|
||
| // With the fix, checkpoint restore should use <= instead of < | ||
| // This ensures message 8 is kept | ||
| const filteredApiHistory = task.apiConversationHistory.filter((m) => !m.ts || m.ts <= msg8Ts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] The regression test validates m.ts ≤ ts filtering but doesn’t exercise checkpointRestore(...), which also truncates cline messages and posts UI state. Consider invoking checkpointRestore(task, { ts: msg8Ts, commitHash, mode: "restore" }) with a mocked CheckpointService and asserting both apiConversationHistory and clineMessages to cover the full integration path.
Summary
This PR fixes the bug where rewinding after manually condensing context would incorrectly lose the expected intervening message history, leaving only the initial message and new messages.
Problem
As detailed by @hannesrudolph in #8295, the issue was caused by:
Solution
Implemented the two low-risk fixes suggested:
Unique summary timestamp (src/core/condense/index.ts)
keepMessages[0].tstokeepMessages[0].ts - 1Inclusive checkpoint restore (src/core/checkpoints/index.ts)
m.ts < tstom.ts <= tsTesting
Verification
The fix has been validated to:
Fixes #8295
Important
Fixes timestamp collision in context condensing and rewinding by ensuring unique summary timestamps and inclusive checkpoint restore filtering.
keepMessages[0].ts - 1insummarizeConversation()inindex.ts.m.ts < tstom.ts <= tsincheckpointRestore()inindex.ts.Task.spec.tsto verify message preservation up to rewind target.This description was created by
for 0071fe5. You can customize this summary. It will automatically update as commits are pushed.