-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: prevent subtask restart on cancellation #8505
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 |
|---|---|---|
|
|
@@ -146,6 +146,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| readonly rootTaskId?: string | ||
| readonly parentTaskId?: string | ||
| childTaskId?: string | ||
| wasSubtaskCancelled?: boolean | ||
|
|
||
| readonly instanceId: string | ||
| readonly metadata: TaskMetadata | ||
|
|
@@ -1649,32 +1650,55 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| }) | ||
| } | ||
|
|
||
| public async completeSubtask(lastMessage: string) { | ||
| public async completeSubtask(lastMessage: string, wasCancelled: boolean = false) { | ||
| this.isPaused = false | ||
| this.childTaskId = undefined | ||
| this.wasSubtaskCancelled = wasCancelled | ||
|
|
||
| this.emit(RooCodeEventName.TaskUnpaused, this.taskId) | ||
|
|
||
| // Fake an answer from the subtask that it has completed running and | ||
| // this is the result of what it has done add the message to the chat | ||
| // history and to the webview ui. | ||
| try { | ||
| await this.say("subtask_result", lastMessage) | ||
| // Only add the subtask result if it wasn't cancelled | ||
| if (!wasCancelled) { | ||
| // Fake an answer from the subtask that it has completed running and | ||
| // this is the result of what it has done add the message to the chat | ||
| // history and to the webview ui. | ||
| try { | ||
| await this.say("subtask_result", lastMessage) | ||
|
|
||
| await this.addToApiConversationHistory({ | ||
| role: "user", | ||
| content: [{ type: "text", text: `[new_task completed] Result: ${lastMessage}` }], | ||
| }) | ||
| await this.addToApiConversationHistory({ | ||
| role: "user", | ||
| content: [{ type: "text", text: `[new_task completed] Result: ${lastMessage}` }], | ||
| }) | ||
|
|
||
| // Set skipPrevResponseIdOnce to ensure the next API call sends the full conversation | ||
| // including the subtask result, not just from before the subtask was created | ||
| this.skipPrevResponseIdOnce = true | ||
| } catch (error) { | ||
| this.providerRef | ||
| .deref() | ||
| ?.log(`Error failed to add reply from subtask into conversation of parent task, error: ${error}`) | ||
| // Set skipPrevResponseIdOnce to ensure the next API call sends the full conversation | ||
| // including the subtask result, not just from before the subtask was created | ||
| this.skipPrevResponseIdOnce = true | ||
| } catch (error) { | ||
| this.providerRef | ||
| .deref() | ||
| ?.log(`Error failed to add reply from subtask into conversation of parent task, error: ${error}`) | ||
|
|
||
| throw error | ||
| throw error | ||
| } | ||
| } else { | ||
| // If the subtask was cancelled, add a message indicating that | ||
| try { | ||
| await this.say("subtask_result", "Subtask was cancelled by user") | ||
|
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. P3: Hardcoded user-facing string duplicated across files (also passed from ClineProvider.cancelTask). Consider centralizing or localizing via the existing i18n utilities (t("…")) to keep copy consistent and translatable. |
||
|
|
||
| await this.addToApiConversationHistory({ | ||
| role: "user", | ||
| content: [{ type: "text", text: `[new_task cancelled] The subtask was cancelled by the user.` }], | ||
| }) | ||
|
|
||
| // Set skipPrevResponseIdOnce to ensure the next API call sends the full conversation | ||
| this.skipPrevResponseIdOnce = true | ||
| } catch (error) { | ||
| this.providerRef | ||
| .deref() | ||
| ?.log(`Error failed to add cancellation message to parent task, error: ${error}`) | ||
|
|
||
| throw error | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -481,13 +481,13 @@ export class ClineProvider | |
| // exists). | ||
| // This is used when a subtask is finished and the parent task needs to be | ||
| // resumed. | ||
| async finishSubTask(lastMessage: string) { | ||
| async finishSubTask(lastMessage: string, wasCancelled: boolean = false) { | ||
| // Remove the last cline instance from the stack (this is the finished | ||
| // subtask). | ||
| await this.removeClineFromStack() | ||
| // Resume the last cline instance in the stack (if it exists - this is | ||
| // the 'parent' calling task). | ||
| await this.getCurrentTask()?.completeSubtask(lastMessage) | ||
| await this.getCurrentTask()?.completeSubtask(lastMessage, wasCancelled) | ||
| } | ||
| // Pending Edit Operations Management | ||
|
|
||
|
|
@@ -2582,6 +2582,44 @@ export class ClineProvider | |
|
|
||
| console.log(`[cancelTask] cancelling task ${task.taskId}.${task.instanceId}`) | ||
|
|
||
| // Check if this is a subtask | ||
| const isSubtask = task.parentTask !== undefined | ||
|
|
||
| if (isSubtask) { | ||
| // For subtasks, we handle cancellation differently to prevent automatic restart | ||
| console.log(`[cancelTask] Cancelling subtask ${task.taskId}.${task.instanceId}`) | ||
|
|
||
| // Mark this as a user-initiated cancellation | ||
| task.abortReason = "user_cancelled" | ||
|
|
||
| // Begin abort (non-blocking) | ||
| task.abortTask() | ||
|
|
||
| // Mark as abandoned to prevent residual activity | ||
| task.abandoned = true | ||
|
|
||
| // Wait for the task to finish aborting | ||
| await pWaitFor( | ||
| () => | ||
|
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. P1: Race condition and potential undefined access in the wait predicate. The predicate calls getCurrentTask() multiple times; the stack can change between calls, making a later call return undefined and then property access (e.g. .isStreaming) can throw. Snapshot the current task once per evaluation. Example: const predicate = () => { |
||
| this.getCurrentTask()! === undefined || | ||
| this.getCurrentTask()!.isStreaming === false || | ||
| this.getCurrentTask()!.didFinishAbortingStream || | ||
| this.getCurrentTask()!.isWaitingForFirstChunk, | ||
| { | ||
| timeout: 3_000, | ||
| }, | ||
| ).catch(() => { | ||
| console.error("Failed to abort subtask") | ||
| }) | ||
|
|
||
| // Notify the parent task that the subtask was cancelled | ||
| await this.finishSubTask("Subtask was cancelled by user", true) | ||
|
|
||
| // Don't rehydrate - let the parent task handle what to do next | ||
| return | ||
| } | ||
|
|
||
| // For non-subtasks, use the original cancellation logic with rehydration | ||
| const { historyItem, uiMessagesFilePath } = await this.getTaskWithId(task.taskId) | ||
|
|
||
| // Preserve parent and root task information for history item. | ||
|
|
||
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.
P3: New field
wasSubtaskCancelledis written but not read anywhere. Consider either (a) using it to adjust parent control flow/logging/telemetry, or (b) deferring adding this state until it is consumed to avoid drift.