From adcc10a86bfd18540e5ac014abe6b5a546141ba8 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Wed, 6 Aug 2025 19:19:13 -0500 Subject: [PATCH] fix: address PR review feedback for task cancellation - Add proper resource disposal in resetToResumableState() to prevent memory leaks - Add JSDoc documentation for skipSave parameter in abortTask() - Add error handling for resetToResumableState() and ask() operations - Extract resumption logic into dedicated handleUserCancellationResume() method - Add 30-second timeout safety net for cancellation in ClineProvider These changes ensure proper cleanup of resources (browsers, terminals) during task cancellation while maintaining the no-rerender behavior. --- src/core/task/Task.ts | 75 ++++++++++++++++++++++++++++--- src/core/webview/ClineProvider.ts | 21 ++++++++- 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 18828a3088..5d3f3c4743 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1489,7 +1489,13 @@ export class Task extends EventEmitter implements TaskLike { } } -public async abortTask(isAbandoned = false, skipSave = false) { +/** + * 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. @@ -1521,6 +1527,9 @@ public async abortTask(isAbandoned = false, skipSave = false) { /** * 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() { console.log(`[subtasks] resetting task ${this.taskId}.${this.instanceId} to resumable state`) @@ -1553,7 +1562,6 @@ public async abortTask(isAbandoned = false, skipSave = false) { this.askResponse = undefined this.askResponseText = undefined this.askResponseImages = undefined - this.blockingAsk = undefined // Reset parser if exists if (this.assistantMessageParser) { @@ -1566,9 +1574,27 @@ public async abortTask(isAbandoned = false, skipSave = false) { await this.diffViewProvider.reset() } - // The task is now ready to be resumed - // The API request status has already been updated by abortStream - // We don't add the resume_task message here because ask() will add it + // Dispose of resources that could accumulate if tasks are repeatedly cancelled + // These will be recreated as needed when the task resumes + try { + // 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 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 @@ -2771,6 +2797,45 @@ if (this.abort) { } } + /** + * 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 - true if the task should end, false to continue + */ + private async handleUserCancellationResume(): Promise { + 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() { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 61b2f0465e..44570a37c9 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1282,8 +1282,27 @@ export class ClineProvider // Just set the abort flag - the task will handle its own resumption cline.abort = true + // 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((resolve) => { + 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) + }) + // The task's streaming loop will detect the abort flag and handle the resumption - // No need to wait or do anything else here + // The timeout ensures we don't wait indefinitely } async updateCustomInstructions(instructions?: string) {