mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
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.
This commit is contained in:
parent
c93d6908e2
commit
adcc10a86b
2 changed files with 90 additions and 6 deletions
|
|
@ -1489,7 +1489,13 @@ export class Task extends EventEmitter<TaskEvents> 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<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() {
|
||||
|
|
|
|||
|
|
@ -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<void>((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) {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue