From 5f68516936a6a1b9e1f661ea8ff6b28cff6ea978 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Thu, 18 Sep 2025 17:35:53 +0000 Subject: [PATCH] fix: prevent empty history corruption during cancel/resume operations - Make resume tolerant of empty API conversation history - Standardize reads using helper functions instead of direct JSON.parse - Add transaction helper for safe read-modify-write operations - Guard against unintentional empty writes with allowEmpty flag This fixes the issue where canceling during model's thinking/streaming phase could leave the API history file as an empty array ([]), causing the chat to lock when resuming. Fixes #8153 and #5333 --- src/core/task-persistence/apiMessages.ts | 43 +++++++++++++++++++ src/core/task/Task.ts | 21 ++++++++- src/core/webview/ClineProvider.ts | 6 ++- .../webviewMessageHandler.edit.spec.ts | 23 +++++----- src/core/webview/webviewMessageHandler.ts | 2 + 5 files changed, 82 insertions(+), 13 deletions(-) diff --git a/src/core/task-persistence/apiMessages.ts b/src/core/task-persistence/apiMessages.ts index f846aaf13f..204eb71599 100644 --- a/src/core/task-persistence/apiMessages.ts +++ b/src/core/task-persistence/apiMessages.ts @@ -81,3 +81,46 @@ export async function saveApiMessages({ const filePath = path.join(taskDir, GlobalFileNames.apiConversationHistory) await safeWriteJson(filePath, messages) } + +/** + * Transaction helper for safe read-modify-write operations on API messages. + * Ensures atomic updates by reading, modifying, and writing under a conceptual lock. + * + * @param taskId - The task ID + * @param globalStoragePath - The global storage path + * @param updater - A pure function that takes the current messages and returns the updated messages + * @param options - Optional configuration + * @returns The updated messages + */ +export async function transactApiMessages({ + taskId, + globalStoragePath, + updater, + options = {}, +}: { + taskId: string + globalStoragePath: string + updater: (messages: ApiMessage[]) => ApiMessage[] + options?: { + allowEmpty?: boolean + } +}): Promise { + // Read current state + const currentMessages = await readApiMessages({ taskId, globalStoragePath }) + + // Apply the pure updater function + const updatedMessages = updater(currentMessages) + + // Guard against unintentional empty writes + if (updatedMessages.length === 0 && currentMessages.length > 0 && !options.allowEmpty) { + console.warn( + `[transactApiMessages] Preventing empty write for taskId: ${taskId}. Current has ${currentMessages.length} messages. Use allowEmpty: true to force.`, + ) + return currentMessages // Return unchanged + } + + // Commit the changes + await saveApiMessages({ messages: updatedMessages, taskId, globalStoragePath }) + + return updatedMessages +} diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index cf16df8dcc..4ecde0b2e8 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -582,7 +582,16 @@ export class Task extends EventEmitter implements TaskLike { await this.saveApiConversationHistory() } - async overwriteApiConversationHistory(newHistory: ApiMessage[]) { + async overwriteApiConversationHistory(newHistory: ApiMessage[], allowEmpty: boolean = false) { + // Guard against unintentional empty writes + if (newHistory.length === 0 && this.apiConversationHistory.length > 0 && !allowEmpty) { + console.warn( + `[Task#overwriteApiConversationHistory] Preventing empty write for taskId: ${this.taskId}. ` + + `Current has ${this.apiConversationHistory.length} messages. Use allowEmpty: true to force.`, + ) + return // Don't overwrite with empty array unless explicitly allowed + } + this.apiConversationHistory = newHistory await this.saveApiConversationHistory() } @@ -1436,7 +1445,15 @@ export class Task extends EventEmitter implements TaskLike { throw new Error("Unexpected: Last message is not a user or assistant message") } } else { - throw new Error("Unexpected: No existing API conversation history") + // Handle empty API conversation history gracefully instead of throwing + // This prevents the "chat locks until reopen" failure mode + this.say( + "text", + "[TASK RESUMPTION] Previous conversation history was empty. Starting with a fresh baseline.", + ) + // Initialize with empty arrays to allow the task to continue + modifiedApiConversationHistory = [] + modifiedOldUserContent = [] } let newUserContent: Anthropic.Messages.ContentBlockParam[] = [...modifiedOldUserContent] diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 9abddc6d96..0d32baaaf2 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -918,8 +918,10 @@ export class ClineProvider await task.overwriteClineMessages(task.clineMessages.slice(0, messageIndex)) if (apiConversationHistoryIndex !== -1) { + // Allow empty writes for edit operations after checkpoint restoration await task.overwriteApiConversationHistory( task.apiConversationHistory.slice(0, apiConversationHistoryIndex), + true, // allowEmpty: true for edit operations ) } @@ -1453,6 +1455,7 @@ export class ClineProvider if (historyItem) { const { getTaskDirectoryPath } = await import("../../utils/storage") + const { readApiMessages } = await import("../task-persistence/apiMessages") const globalStoragePath = this.contextProxy.globalStorageUri.fsPath const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id) const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory) @@ -1460,7 +1463,8 @@ export class ClineProvider const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath) if (fileExists) { - const apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8")) + // Use the helper reader for unified behavior/logging instead of direct JSON.parse + const apiConversationHistory = await readApiMessages({ taskId: id, globalStoragePath }) return { historyItem, diff --git a/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts index d467f5cd92..63349b4b0b 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts @@ -135,7 +135,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { ) // API history should be truncated from first message at/after edited timestamp (fallback) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) it("should preserve messages before the edited message when message not in API history", async () => { @@ -197,13 +197,16 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { ]) // API history should be truncated from the first API message at/after the edited timestamp (fallback) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([ - { - ts: earlierMessageTs, - role: "user", - content: [{ type: "text", text: "Earlier message" }], - }, - ]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith( + [ + { + ts: earlierMessageTs, + role: "user", + content: [{ type: "text", text: "Earlier message" }], + }, + ], + true, + ) }) it("should not use fallback when exact apiConversationHistoryIndex is found", async () => { @@ -248,7 +251,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { // Both should be truncated at index 0 expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([]) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) it("should handle case where no API messages match timestamp criteria", async () => { @@ -385,6 +388,6 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => { expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([]) // API history should be truncated from first message at/after edited timestamp (fallback) - expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([]) + expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([], true) }) }) diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index accb66f6e9..55e2ca1a88 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -108,8 +108,10 @@ export const webviewMessageHandler = async ( await currentCline.overwriteClineMessages(currentCline.clineMessages.slice(0, messageIndex)) if (apiConversationHistoryIndex !== -1) { + // Allow empty writes for edit/delete operations await currentCline.overwriteApiConversationHistory( currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex), + true, // allowEmpty: true for edit/delete operations ) } }