Skip to content

fix: prevent conversation history corruption when cancelling during response #8154

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

Closed
wants to merge 1 commit into from
Closed

fix: prevent conversation history corruption when cancelling during response #8154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 15 additions & 0 deletions src/core/task-persistence/apiMessages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,21 @@ export async function saveApiMessages({
taskId: string
globalStoragePath: string
}) {
// Validate messages array to prevent saving empty arrays that could corrupt history
if (!messages || !Array.isArray(messages)) {
Copy link
Contributor Author

@roomote roomote bot Sep 18, 2025

Choose a reason for hiding this comment

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

The validation logic here is nearly identical to saveTaskMessages. Could we extract this to a shared validation utility function to reduce duplication?

console.error(
`[Roo-Debug] saveApiMessages: Invalid messages provided for taskId: ${taskId}. Messages must be an array.`,
)
throw new Error("Invalid messages: must be an array")
}

// Warn if saving empty array but allow it for new tasks
if (messages.length === 0) {
console.warn(
`[Roo-Debug] saveApiMessages: Saving empty messages array for taskId: ${taskId}. This may be intentional for new tasks.`,
)
}

const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory)
await safeWriteJson(filePath, messages)
Expand Down
15 changes: 15 additions & 0 deletions src/core/task-persistence/taskMessages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,21 @@ export type SaveTaskMessagesOptions = {
}

export async function saveTaskMessages({ messages, taskId, globalStoragePath }: SaveTaskMessagesOptions) {
// Validate messages array to prevent saving empty arrays that could corrupt history
if (!messages || !Array.isArray(messages)) {
Copy link
Contributor Author

@roomote roomote bot Sep 18, 2025

Choose a reason for hiding this comment

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

This duplicates the validation from apiMessages.ts. Consider extracting to a shared function.

console.error(
`[Roo-Debug] saveTaskMessages: Invalid messages provided for taskId: ${taskId}. Messages must be an array.`,
)
throw new Error("Invalid messages: must be an array")
}

// Warn if saving empty array but allow it for new tasks
if (messages.length === 0) {
console.warn(
`[Roo-Debug] saveTaskMessages: Saving empty messages array for taskId: ${taskId}. This may be intentional for new tasks.`,
)
}

const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
const filePath = path.join(taskDir, GlobalFileNames.uiMessages)
await safeWriteJson(filePath, messages)
Expand Down
34 changes: 31 additions & 3 deletions src/core/task/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {

private async saveApiConversationHistory() {
try {
// Validate that we have messages before saving to prevent corruption
if (!this.apiConversationHistory || this.apiConversationHistory.length === 0) {
console.warn(
Copy link
Contributor Author

@roomote roomote bot Sep 18, 2025

Choose a reason for hiding this comment

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

Is this error handling approach intentional? The persistence layer throws errors for invalid input, but here we silently return. Consider making the validation behavior consistent across layers - either always throw for invalid data or always handle gracefully with logging.

`[Task#saveApiConversationHistory] Skipping save of empty API conversation history for task ${this.taskId}`,
)
return
}

await saveApiMessages({
messages: this.apiConversationHistory,
taskId: this.taskId,
Expand Down Expand Up @@ -658,6 +666,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {

private async saveClineMessages() {
try {
// Validate that we have messages before saving to prevent corruption
if (!this.clineMessages || this.clineMessages.length === 0) {
console.warn(`[Task#saveClineMessages] Skipping save of empty Cline messages for task ${this.taskId}`)
Copy link
Contributor Author

@roomote roomote bot Sep 18, 2025

Choose a reason for hiding this comment

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

Same inconsistency here - we warn and return while the persistence layer throws. Should we standardize the approach?

return
}

await saveTaskMessages({
messages: this.clineMessages,
taskId: this.taskId,
Expand Down Expand Up @@ -1504,10 +1518,24 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error)
// Don't rethrow - we want abort to always succeed
}
// Save the countdown message in the automatic retry or other content.

// Only save messages if they are not empty to prevent corruption
// This prevents the race condition where cancelling during streaming
// could save an empty array and wipe the conversation history
try {
// Save the countdown message in the automatic retry or other content.
await this.saveClineMessages()
// Check if we have valid messages before saving
// Don't save if messages are empty or if we're in the middle of streaming
if (this.clineMessages && this.clineMessages.length > 0 && !this.isStreaming) {
await this.saveClineMessages()
} else if (this.isStreaming) {
console.log(
Copy link
Contributor Author

@roomote roomote bot Sep 18, 2025

Choose a reason for hiding this comment

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

Minor: Using console.log here but console.warn in the save methods above. Should we use consistent log levels based on severity? Perhaps console.info for informational skips?

`[Task#abortTask] Skipping message save during active streaming for task ${this.taskId}.${this.instanceId}`,
)
} else {
console.log(
`[Task#abortTask] Skipping message save due to empty messages for task ${this.taskId}.${this.instanceId}`,
)
}
} catch (error) {
console.error(`Error saving messages during abort for task ${this.taskId}.${this.instanceId}:`, error)
}
Expand Down
Loading