-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent empty history corruption during cancel/resume operations #8160
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<ApiMessage[]> { | ||
| // 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.`, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I notice we're using different log levels ('warn' vs 'debug') for similar operations. Would it make sense to standardize these? For example, all successful operations could use 'debug' while warnings/errors use 'warn' or 'error'. |
||
| ) | ||
| return currentMessages // Return unchanged | ||
| } | ||
|
|
||
| // Commit the changes | ||
| await saveApiMessages({ messages: updatedMessages, taskId, globalStoragePath }) | ||
|
|
||
| return updatedMessages | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this transaction helper is a good step forward, there's still a small race condition window between the read and write operations. Have you considered implementing file locking or using a more robust transaction mechanism like a write-ahead log? This could provide stronger guarantees against concurrent modifications. |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -582,7 +582,16 @@ export class Task extends EventEmitter<TaskEvents> 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<TaskEvents> 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 = [] | ||||||||||||||||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The empty history fallback is solid defensive programming. Could we enhance this with a recovery mechanism that checks for backup files? Something like:
Suggested change
|
||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| let newUserContent: Anthropic.Messages.ContentBlockParam[] = [...modifiedOldUserContent] | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good use of the allowEmpty flag here for intentional deletions. Consider adding an inline comment explaining why this specific operation needs to allow empty writes, to help future maintainers understand the design decision. |
||
| ) | ||
| } | ||
| } | ||
|
|
||
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.
This transactApiMessages function would benefit from comprehensive JSDoc comments explaining the transaction semantics and the guarantees it provides. The current JSDoc is good but could be expanded to mention the race condition limitation: