-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: eliminate UI rerender during task cancellation #6781
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 |
|---|---|---|
|
|
@@ -1489,8 +1489,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| } | ||
| } | ||
|
|
||
| public async abortTask(isAbandoned = false) { | ||
| // Aborting task | ||
| /** | ||
| * Aborts the current task and optionally saves messages. | ||
| * | ||
| * @param isAbandoned - If true, marks the task as abandoned (no cleanup needed) | ||
| * @param skipSave - If true, skips saving messages (used during user cancellation when messages are already saved) | ||
| */ | ||
| public async abortTask(isAbandoned = false, skipSave = false) { | ||
| console.log(`[subtasks] aborting task ${this.taskId}.${this.instanceId}`) | ||
|
|
||
| // Will stop any autonomously running promises. | ||
| if (isAbandoned) { | ||
|
|
@@ -1506,12 +1512,118 @@ 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 not skipping (e.g., during user cancellation where messages are already saved) | ||
| if (!skipSave) { | ||
| try { | ||
| // Save the countdown message in the automatic retry or other content. | ||
| await this.saveClineMessages() | ||
| } catch (error) { | ||
| console.error(`Error saving messages during abort for task ${this.taskId}.${this.instanceId}:`, error) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Reset the task to a resumable state without recreating the instance. | ||
| * This is used when canceling a task to avoid unnecessary rerenders. | ||
| * | ||
| * IMPORTANT: This method cleans up resources to prevent memory leaks | ||
| * while preserving the task instance for resumption. | ||
| */ | ||
| public async resetToResumableState() { | ||
|
Contributor
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. Potential memory leak concern: This method resets many properties but doesn't dispose of resources like Consider adding cleanup for these resources:
Collaborator
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 feel nervous about this logic getting out of sync with logic in other parts of the codebase (like the "// Reset streaming state" logic a couple hundred lines down). Think there's any way to DRY it up so we don't forget to add things? |
||
| console.log(`[subtasks] resetting task ${this.taskId}.${this.instanceId} to resumable state`) | ||
|
|
||
| // Reset abort flags | ||
| this.abort = false | ||
| this.abandoned = false | ||
|
|
||
| // Reset streaming state | ||
| this.isStreaming = false | ||
| this.isWaitingForFirstChunk = false | ||
| this.didFinishAbortingStream = true | ||
| this.didCompleteReadingStream = false | ||
|
|
||
| // Centralized streaming reset to avoid duplication | ||
| await this.resetStreamingState({ resetDiffView: "ifEditing" }) | ||
|
|
||
| // Reset API state | ||
| this.consecutiveMistakeCount = 0 | ||
|
|
||
| // Reset ask response state to allow new messages | ||
| this.askResponse = undefined | ||
| this.askResponseText = undefined | ||
| this.askResponseImages = undefined | ||
|
|
||
| // Dispose of resources that could accumulate if tasks are repeatedly cancelled | ||
| // These will be recreated as needed when the task resumes | ||
| try { | ||
| // Save the countdown message in the automatic retry or other content. | ||
| await this.saveClineMessages() | ||
| // Close browser sessions to free memory and browser processes | ||
| if (this.urlContentFetcher) { | ||
| this.urlContentFetcher.closeBrowser() | ||
| } | ||
| if (this.browserSession) { | ||
| this.browserSession.closeBrowser() | ||
| } | ||
|
|
||
| // Release any terminals associated with this task | ||
| // They will be recreated if needed when the task resumes | ||
| if (this.terminalProcess) { | ||
| this.terminalProcess.abort() | ||
| this.terminalProcess = undefined | ||
| } | ||
| } catch (error) { | ||
| console.error(`Error saving messages during abort for task ${this.taskId}.${this.instanceId}:`, error) | ||
| console.error(`Error disposing resources during resetToResumableState: ${error}`) | ||
| // Continue even if resource cleanup fails | ||
| } | ||
|
|
||
| // Keep messages and history intact for resumption | ||
| // The task is now ready to be resumed without recreation | ||
| } | ||
| /** | ||
| * Reset all streaming-related state in a single place to avoid duplication. | ||
| * | ||
| * This centralizes the "Reset streaming state" logic that previously existed | ||
| * in multiple locations (e.g., within recursivelyMakeClineRequests and | ||
| * resetToResumableState). Keeping it here prevents the two call sites from | ||
| * drifting out of sync as fields evolve. | ||
| * | ||
| * Behavior: | ||
| * - Clears transient streaming buffers and flags | ||
| * - Resets the assistant message parser | ||
| * - Optionally resets the diff view (always, only-if-editing, or never) | ||
| * | ||
| * Note: | ||
| * - This method intentionally does NOT touch abort/abandoned flags or | ||
| * high-level control flags like isStreaming/isWaitingForFirstChunk. | ||
| * Those are managed by their respective flows. | ||
| */ | ||
| private async resetStreamingState(options: { resetDiffView?: "always" | "ifEditing" | "never" } = {}) { | ||
|
Contributor
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. Inconsistency in parameter naming: The comment describes the resetDiffView option as accepting 'always, only-if-editing, or never', but the code defines it as 'always', 'ifEditing', or 'never'. Consider updating the comment or the parameter type for consistency. |
||
| // Clear streaming content and state | ||
| this.currentStreamingContentIndex = 0 | ||
| this.currentStreamingDidCheckpoint = false | ||
| this.assistantMessageContent = [] | ||
| this.userMessageContent = [] | ||
| this.userMessageContentReady = false | ||
| this.didRejectTool = false | ||
| this.didAlreadyUseTool = false | ||
| this.presentAssistantMessageLocked = false | ||
| this.presentAssistantMessageHasPendingUpdates = false | ||
| this.didCompleteReadingStream = false | ||
|
|
||
| // Reset parser if exists | ||
| if (this.assistantMessageParser) { | ||
| this.assistantMessageParser.reset() | ||
| } | ||
|
|
||
| // Optionally reset the diff view | ||
| const mode = options.resetDiffView ?? "never" | ||
| if (mode !== "never" && this.diffViewProvider) { | ||
| if (mode === "always") { | ||
| await this.diffViewProvider.reset() | ||
| } else if (mode === "ifEditing" && this.diffViewProvider.isEditing) { | ||
| await this.diffViewProvider.reset() | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1778,20 +1890,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| this.didFinishAbortingStream = true | ||
| } | ||
|
|
||
| // Reset streaming state. | ||
| this.currentStreamingContentIndex = 0 | ||
| this.currentStreamingDidCheckpoint = false | ||
| this.assistantMessageContent = [] | ||
| this.didCompleteReadingStream = false | ||
| this.userMessageContent = [] | ||
| this.userMessageContentReady = false | ||
| this.didRejectTool = false | ||
| this.didAlreadyUseTool = false | ||
| this.presentAssistantMessageLocked = false | ||
| this.presentAssistantMessageHasPendingUpdates = false | ||
| this.assistantMessageParser.reset() | ||
|
|
||
| await this.diffViewProvider.reset() | ||
| // Reset streaming state using centralized helper to keep logic in sync | ||
| await this.resetStreamingState({ resetDiffView: "always" }) | ||
|
|
||
| // Yields only if the first chunk is successful, otherwise will | ||
| // allow the user to retry the request (most likely due to rate | ||
|
|
@@ -2712,6 +2812,45 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handles the resumption flow after a user cancels a task. | ||
| * This method resets the task state, shows the resume prompt, | ||
| * and continues with new user input if provided. | ||
| * | ||
| * @returns Promise<boolean> - true if the task should end, false to continue | ||
| */ | ||
| private async handleUserCancellationResume(): Promise<boolean> { | ||
| try { | ||
| // Reset the task to a resumable state | ||
| this.abort = false | ||
| await this.resetToResumableState() | ||
|
|
||
| // Show the resume prompt | ||
| const { response, text, images } = await this.ask("resume_task") | ||
|
|
||
| if (response === "messageResponse") { | ||
| await this.say("user_feedback", text, images) | ||
| // Continue with the new user input | ||
| const newUserContent: Anthropic.Messages.ContentBlockParam[] = [] | ||
| if (text) { | ||
| newUserContent.push({ type: "text", text }) | ||
| } | ||
| if (images && images.length > 0) { | ||
| newUserContent.push(...formatResponse.imageBlocks(images)) | ||
| } | ||
| // Recursively continue with the new content | ||
| return await this.recursivelyMakeClineRequests(newUserContent) | ||
| } | ||
| // If not messageResponse, the task will end | ||
| return true | ||
| } catch (error) { | ||
| // If there's an error during resumption, log it and end the task | ||
| console.error(`Error during user cancellation resume: ${error}`) | ||
| // Re-throw to maintain existing error handling behavior | ||
| throw error | ||
| } | ||
| } | ||
|
|
||
| // Getters | ||
|
|
||
| public get cwd() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1279,38 +1279,30 @@ export class ClineProvider | |
|
|
||
| console.log(`[subtasks] cancelling task ${cline.taskId}.${cline.instanceId}`) | ||
|
|
||
| const { historyItem } = await this.getTaskWithId(cline.taskId) | ||
| // Preserve parent and root task information for history item. | ||
| const rootTask = cline.rootTask | ||
| const parentTask = cline.parentTask | ||
|
|
||
| cline.abortTask() | ||
|
|
||
| await pWaitFor( | ||
| () => | ||
| this.getCurrentTask()! === undefined || | ||
| this.getCurrentTask()!.isStreaming === false || | ||
| this.getCurrentTask()!.didFinishAbortingStream || | ||
| // If only the first chunk is processed, then there's no | ||
| // need to wait for graceful abort (closes edits, browser, | ||
| // etc). | ||
| this.getCurrentTask()!.isWaitingForFirstChunk, | ||
| { | ||
| timeout: 3_000, | ||
| }, | ||
| ).catch(() => { | ||
| console.error("Failed to abort task") | ||
| // Just set the abort flag - the task will handle its own resumption | ||
| cline.abort = true | ||
|
Contributor
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 simplified cancellation removes the timeout and abandonment logic. While cleaner, this could potentially leave tasks hanging if the streaming loop doesn't detect the abort flag quickly. Is this intentional? You might want to keep a timeout as a safety net: |
||
|
|
||
| // Add a timeout safety net to ensure the task doesn't hang indefinitely | ||
| // If the task doesn't respond to cancellation within 30 seconds, force abort it | ||
| const timeoutMs = 30000 // 30 seconds | ||
| const timeoutPromise = new Promise<void>((resolve) => { | ||
|
Contributor
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 timeout safety net in cancelTask is defined but not awaited. Consider using Promise.race or otherwise awaiting the timeout promise to ensure the forced abort occurs reliably if the task doesn’t respond to cancellation. |
||
| setTimeout(async () => { | ||
| // Check if the task is still in an aborted state | ||
| if (cline.abort && !cline.abandoned) { | ||
| console.log( | ||
| `[subtasks] task ${cline.taskId}.${cline.instanceId} did not respond to cancellation within ${timeoutMs}ms, forcing abort`, | ||
| ) | ||
| // Force abandon the task to ensure cleanup | ||
| cline.abandoned = true | ||
| // Remove it from the stack | ||
| await this.removeClineFromStack() | ||
| resolve() | ||
| } | ||
| }, timeoutMs) | ||
| }) | ||
|
|
||
| if (this.getCurrentTask()) { | ||
| // 'abandoned' will prevent this Cline instance from affecting | ||
| // future Cline instances. This may happen if its hanging on a | ||
| // streaming request. | ||
| this.getCurrentTask()!.abandoned = true | ||
| } | ||
|
|
||
| // Clears task again, so we need to abortTask manually above. | ||
| await this.createTaskWithHistoryItem({ ...historyItem, rootTask, parentTask }) | ||
| // The task's streaming loop will detect the abort flag and handle the resumption | ||
| // The timeout ensures we don't wait indefinitely | ||
| } | ||
|
|
||
| async updateCustomInstructions(instructions?: string) { | ||
|
|
||
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.
Consider adding JSDoc documentation for the new
skipSaveparameter to clarify when it should be used: