Skip to content

fix: prevent irreversible context loss when compaction archive write fails #754

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.

Sign up for GitHub

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

Merged
zmanian merged 2 commits into from
Mar 9, 2026
Merged

fix: prevent irreversible context loss when compaction archive write fails #754

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ea3bd9
fix(compaction): preserve turns when archival write fails
pikaxinge Mar 9, 2026
fe4a8ef
style: cargo fmt
zmanian Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 104 additions & 25 deletions src/agent/compaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,27 +103,26 @@ impl ContextCompactor {
// Generate summary
let summary = self.generate_summary(&to_summarize).await?;

// Write to workspace if available
let summary_written = if let Some(ws) = workspace {
// Write to workspace if available.
// If archival fails, preserve turns to avoid context loss.
let (summary_written, turns_removed) = if let Some(ws) = workspace {
Comment on lines 103 to +108
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In compact_with_summary, the summary is generated via an LLM call before attempting the workspace append. With the new behavior, a workspace write failure preserves turns (no truncation), which means this LLM call becomes pure cost/latency in the failure path and could repeat on subsequent compaction attempts while the workspace remains unhealthy. Consider adding a cheap workspace writability/migrations check before calling generate_summary(), or returning an error/explicit status when archival fails so callers can avoid retry loops and/or switch strategies without repeatedly invoking the LLM.

Copilot uses AI. Check for mistakes.
match self.write_summary_to_workspace(ws, &summary).await {
Ok(()) => true,
Ok(()) => {
thread.truncate_turns(keep_recent);
(true, turns_to_remove)
}
Err(e) => {
tracing::warn!(
"Compaction summary write failed (turns will still be truncated): {}",
e
);
false
tracing::warn!("Compaction summary write failed (turns preserved): {}", e);
(false, 0)
}
}
} else {
false
thread.truncate_turns(keep_recent);
(false, turns_to_remove)
};

// Truncate thread
thread.truncate_turns(keep_recent);

Ok(CompactionPartial {
turns_removed: turns_to_remove,
turns_removed,
summary_written,
summary: Some(summary),
})
Comment on lines +108 to 128
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While the logic is correct, this block can be refactored to be more linear and less repetitive, improving readability. Using an early return on write failure can simplify the control flow.

        let summary_written = if let Some(ws) = workspace {
            if let Err(e) = self.write_summary_to_workspace(ws, &summary).await {
                tracing::warn!("Compaction summary write failed (turns preserved): {}", e);
                // On write failure, don't truncate. Return early with summary but no turns removed.
                return Ok(CompactionPartial {
                    turns_removed: 0,
                    summary_written: false,
                    summary: Some(summary),
                });
            }
            true // Summary was written successfully.
        } else {
            false // No workspace, so no summary written.
        };

        // Truncate turns if summary was successfully written, or if no workspace was provided.
        thread.truncate_turns(keep_recent);

        Ok(CompactionPartial {
            turns_removed: turns_to_remove,
            summary_written,
            summary: Some(summary),
        })

Expand Down Expand Up @@ -165,23 +164,20 @@ impl ContextCompactor {
// Format turns for storage
let content = format_turns_for_storage(old_turns);

// Write to workspace
let written = match self.write_context_to_workspace(ws, &content).await {
Ok(()) => true,
// Write to workspace. If archival fails, preserve turns.
let (written, turns_removed) = match self.write_context_to_workspace(ws, &content).await {
Ok(()) => {
thread.truncate_turns(keep_recent);
(true, turns_to_remove)
}
Err(e) => {
tracing::warn!(
"Compaction context write failed (turns will still be truncated): {}",
e
);
false
tracing::warn!("Compaction context write failed (turns preserved): {}", e);
(false, 0)
}
Comment on lines +167 to 176
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When write_context_to_workspace fails, compact_to_workspace now returns Ok with turns_removed = 0 and leaves the thread unmodified. This makes the operation effectively a no-op (aside from formatting work) and may lead to repeated compaction attempts without any token reduction while storage remains unavailable. Consider propagating the error (while still preserving turns), or extending the result with an explicit archival-failed/compaction-skipped signal so callers can notify the user and/or choose a fallback strategy intentionally.

Copilot uses AI. Check for mistakes.
};

// Truncate
thread.truncate_turns(keep_recent);

Ok(CompactionPartial {
turns_removed: turns_to_remove,
turns_removed,
summary_written: written,
summary: None,
})
Comment on lines +167 to 183
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to compact_with_summary, this block can be refactored for better readability and to reduce complexity. Using an early return on write failure simplifies the control flow.

Suggested change
// Write to workspace. If archival fails, preserve turns.
let (written, turns_removed) = match self.write_context_to_workspace(ws, &content).await {
Ok(()) => {
thread.truncate_turns(keep_recent);
(true, turns_to_remove)
}
Err(e) => {
tracing::warn!(
"Compaction context write failed (turns will still be truncated): {}",
e
);
false
tracing::warn!("Compaction context write failed (turns preserved): {}", e);
(false, 0)
}
};
// Truncate
thread.truncate_turns(keep_recent);
Ok(CompactionPartial {
turns_removed: turns_to_remove,
turns_removed,
summary_written: written,
summary: None,
})
// Write to workspace. If archival fails, preserve turns.
if let Err(e) = self.write_context_to_workspace(ws, &content).await {
tracing::warn!("Compaction context write failed (turns preserved): {}", e);
return Ok(CompactionPartial::empty());
}
// Truncate since write was successful.
thread.truncate_turns(keep_recent);
Ok(CompactionPartial {
turns_removed: turns_to_remove,
summary_written: true,
summary: None,
})

Expand Down Expand Up @@ -362,6 +358,19 @@ mod tests {
thread
}

#[cfg(feature = "libsql")]
async fn make_unmigrated_workspace() -> crate::workspace::Workspace {
use crate::db::Database;
use crate::db::libsql::LibSqlBackend;

// Intentionally skip migrations so workspace append operations fail.
let backend = LibSqlBackend::new_memory()
.await
.expect("should create in-memory libsql backend");
let db: Arc<dyn Database> = Arc::new(backend);
crate::workspace::Workspace::new_with_db("compaction-test", db)
}

// ------------------------------------------------------------------
// 1. compact_truncate keeps last N turns
// ------------------------------------------------------------------
Expand Down Expand Up @@ -560,6 +569,43 @@ mod tests {
assert_eq!(llm.calls(), 0);
}

#[cfg(feature = "libsql")]
#[tokio::test]
async fn test_compact_with_summary_preserves_turns_when_workspace_write_fails() {
let llm = Arc::new(StubLlm::new("summary"));
let compactor = make_compactor(llm.clone());
let mut thread = make_thread(8);
let original_inputs: Vec<String> =
thread.turns.iter().map(|t| t.user_input.clone()).collect();
let workspace = make_unmigrated_workspace().await;

let result = compactor
.compact(
&mut thread,
CompactionStrategy::Summarize { keep_recent: 3 },
Some(&workspace),
)
.await
.expect("compact should succeed even when workspace write fails");

// On archival failure, no turns should be removed.
assert_eq!(thread.turns.len(), 8);
assert_eq!(
thread
.turns
.iter()
.map(|t| t.user_input.as_str())
.collect::<Vec<_>>(),
original_inputs
.iter()
.map(|s| s.as_str())
.collect::<Vec<_>>()
);
assert_eq!(result.turns_removed, 0);
assert!(!result.summary_written);
assert_eq!(llm.calls(), 1);
}

// ------------------------------------------------------------------
// 7. compact_to_workspace without workspace falls back to truncation
// ------------------------------------------------------------------
Expand Down Expand Up @@ -608,6 +654,39 @@ mod tests {
assert_eq!(result.turns_removed, 0);
}

#[cfg(feature = "libsql")]
#[tokio::test]
async fn test_compact_to_workspace_preserves_turns_when_workspace_write_fails() {
let llm = Arc::new(StubLlm::new("unused"));
let compactor = make_compactor(llm.clone());
let mut thread = make_thread(20);
let original_inputs: Vec<String> =
thread.turns.iter().map(|t| t.user_input.clone()).collect();
let workspace = make_unmigrated_workspace().await;

let result = compactor
.compact(&mut thread, CompactionStrategy::MoveToWorkspace, Some(&workspace))
.await
.expect("compact should succeed even when workspace write fails");

// On archival failure, no turns should be removed.
assert_eq!(thread.turns.len(), 20);
assert_eq!(
thread
.turns
.iter()
.map(|t| t.user_input.as_str())
.collect::<Vec<_>>(),
original_inputs
.iter()
.map(|s| s.as_str())
.collect::<Vec<_>>()
);
assert_eq!(result.turns_removed, 0);
assert!(!result.summary_written);
assert_eq!(llm.calls(), 0);
}

// ------------------------------------------------------------------
// 9. format_turns_for_storage includes tool calls
// ------------------------------------------------------------------
Expand Down
Loading