Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
179 changes: 159 additions & 20 deletions src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

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 skipSave parameter to clarify when it should be used:

console.log(`[subtasks] aborting task ${this.taskId}.${this.instanceId}`)

// Will stop any autonomously running promises.
if (isAbandoned) {
Expand All @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 urlContentFetcher, browserSession, or terminalProcess. These could accumulate if tasks are repeatedly cancelled and resumed.

Consider adding cleanup for these resources:

Copy link
Collaborator

Choose a reason for hiding this comment

The 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" } = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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()
}
}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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() {
Expand Down
52 changes: 22 additions & 30 deletions src/core/webview/ClineProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
Expand Down