Skip to content

Commit fc06118

Browse files
committed
fix(memory): clear summary on partial rewind too; propagate prune errors
Per-commit review of #60 surfaced two issues: 1. Partial truncation left session_summaries in place, but a rewind discards the turns the summary was derived from — leaving stale long-term memory of content the user edited/rolled back. Clear session_summaries (and session_metadata) on any real truncation, partial or full, so the next reflection rescans from a consistent state. The no-op path (keep >= 1 but the thread has fewer user messages) is left untouched. Note this diverges from Clear { keep_last > 0 } on purpose: compaction trims the OLD head so the (recent-derived) summary stays valid, whereas a rewind trims the RECENT tail and invalidates it. 2. The tool_result_cache / metadata prune used `let _ =` and swallowed errors; a failed prune inside the transaction would still commit and leave the thread inconsistent. Propagate with `?`. Test now also covers summary invalidation (insert a summary, rewind, assert gone). 6/6 memory tests green; clippy clean on memory.rs.
1 parent 2a147e7 commit fc06118

1 file changed

Lines changed: 59 additions & 21 deletions

File tree

src/memory.rs

Lines changed: 59 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -489,8 +489,10 @@ pub enum MemoryMessage {
489489
/// - `keep_user_messages == 0`: delete the entire thread (equivalent to
490490
/// `Clear { keep_last: 0 }`).
491491
///
492-
/// Deleted tool-result cache rows and reflection metadata that pointed
493-
/// into the removed range are pruned in the same transaction.
492+
/// On any real truncation the thread's `session_summaries` and
493+
/// `session_metadata` are cleared (a rewind invalidates the reflection
494+
/// derived from the discarded turns), and `tool_result_cache` rows for the
495+
/// dropped tool_call_ids are pruned — all in the same transaction.
494496
TruncateAfterUserMessage {
495497
thread_id: String,
496498
keep_user_messages: usize,
@@ -1086,33 +1088,31 @@ impl ActorLogic<MemoryMessage> for SqliteMemoryActor {
10861088
};
10871089

10881090
for tcid in &dropped_tool_call_ids {
1089-
let _ = tx.execute(
1091+
tx.execute(
10901092
"DELETE FROM tool_result_cache WHERE tool_call_id = ?1",
10911093
params![tcid],
1092-
);
1094+
)
1095+
.map_err(|e| e.to_string())?;
10931096
}
10941097

1095-
// Reflection metadata may point at a now-deleted message.
1096-
// Drop the stale pointer (or the whole row on full delete)
1097-
// so the next reflection pass rescans from a valid state.
1098-
if keep_user_messages == 0 {
1099-
// Full wipe — match Clear { keep_last: 0 } exactly,
1100-
// including the thread's summary so no orphan remains.
1101-
let _ = tx.execute(
1098+
// A rewind discards turns the thread's reflection/summary
1099+
// were derived from, so on any real truncation (partial or
1100+
// full) clear both — the next reflection rescans from a
1101+
// consistent state instead of "remembering" deleted turns.
1102+
// The no-op path (keep >= 1 but the thread has fewer user
1103+
// messages than requested) touches neither. Errors here
1104+
// would leave the thread inconsistent, so propagate.
1105+
if cutoff.is_some() || keep_user_messages == 0 {
1106+
tx.execute(
11021107
"DELETE FROM session_metadata WHERE thread_id = ?1",
11031108
params![thread_id],
1104-
);
1105-
let _ = tx.execute(
1109+
)
1110+
.map_err(|e| e.to_string())?;
1111+
tx.execute(
11061112
"DELETE FROM session_summaries WHERE thread_id = ?1",
11071113
params![thread_id],
1108-
);
1109-
} else if let Some(cutoff_id) = cutoff {
1110-
let _ = tx.execute(
1111-
"DELETE FROM session_metadata
1112-
WHERE thread_id = ?1
1113-
AND COALESCE(last_reflection_msg_id, -1) > ?2",
1114-
params![thread_id, cutoff_id],
1115-
);
1114+
)
1115+
.map_err(|e| e.to_string())?;
11161116
}
11171117

11181118
tx.commit().map_err(|e| e.to_string())?;
@@ -2486,6 +2486,20 @@ mod root_thread_id_tests {
24862486
.expect("send metadata");
24872487
rx.await.expect("reply").expect("metadata ok");
24882488

2489+
// Add a thread summary — a rewind must invalidate it (it was derived
2490+
// from the turns being discarded).
2491+
let (tx, rx) = tokio::sync::oneshot::channel();
2492+
node.send_packet(MemoryMessage::AddSummary {
2493+
thread_id: thread_id.to_string(),
2494+
summary: "stale summary".to_string(),
2495+
key_info: "k".to_string(),
2496+
knowledge_gaps: "g".to_string(),
2497+
reply: SharedReply::new(tx),
2498+
})
2499+
.await
2500+
.expect("send summary");
2501+
rx.await.expect("reply").expect("summary ok");
2502+
24892503
// Sanity: cache is present before the rewind.
24902504
let (tx, rx) = tokio::sync::oneshot::channel();
24912505
node.send_packet(MemoryMessage::FetchToolResult {
@@ -2499,6 +2513,18 @@ mod root_thread_id_tests {
24992513
Some("full tool result".to_string())
25002514
);
25012515

2516+
// Sanity: summary is present before the rewind.
2517+
let (tx, rx) = tokio::sync::oneshot::channel();
2518+
node.send_packet(MemoryMessage::GetRecentSummaries {
2519+
thread_id: thread_id.to_string(),
2520+
limit: 10,
2521+
reply: SharedReply::new(tx),
2522+
})
2523+
.await
2524+
.expect("send get-summaries");
2525+
let summaries = rx.await.expect("reply").expect("summaries ok");
2526+
assert_eq!(summaries.len(), 1);
2527+
25022528
// Rewind to u1 — drops ids 2..4.
25032529
let (tx, rx) = tokio::sync::oneshot::channel();
25042530
node.send_packet(MemoryMessage::TruncateAfterUserMessage {
@@ -2531,5 +2557,17 @@ mod root_thread_id_tests {
25312557
.expect("send get-metadata");
25322558
let (last_id, _time) = rx.await.expect("reply").expect("metadata ok");
25332559
assert_eq!(last_id, None);
2560+
2561+
// Summary was derived from the discarded turns → cleared.
2562+
let (tx, rx) = tokio::sync::oneshot::channel();
2563+
node.send_packet(MemoryMessage::GetRecentSummaries {
2564+
thread_id: thread_id.to_string(),
2565+
limit: 10,
2566+
reply: SharedReply::new(tx),
2567+
})
2568+
.await
2569+
.expect("send get-summaries");
2570+
let summaries = rx.await.expect("reply").expect("summaries ok");
2571+
assert!(summaries.is_empty());
25342572
}
25352573
}

0 commit comments

Comments
 (0)