-
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
Conversation
- Added wasSubtaskCancelled flag to Task class to track cancelled subtasks - Modified completeSubtask to accept wasCancelled parameter - Updated cancelTask to detect subtasks and handle them differently - Subtasks now notify parent of cancellation instead of triggering rehydration - Added comprehensive tests for subtask cancellation behavior Fixes #8504
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.
Self-reviewing my own code like a deterministic robot grading its dreams.
|
|
||
| // Wait for the task to finish aborting | ||
| await pWaitFor( | ||
| () => |
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.
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 = () => {
const current = this.getCurrentTask();
return !current || current.isStreaming === false || current.didFinishAbortingStream || current.isWaitingForFirstChunk;
};
| readonly rootTaskId?: string | ||
| readonly parentTaskId?: string | ||
| childTaskId?: string | ||
| wasSubtaskCancelled?: boolean |
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 wasSubtaskCancelled is 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.
| } else { | ||
| // If the subtask was cancelled, add a message indicating that | ||
| try { | ||
| await this.say("subtask_result", "Subtask was cancelled by user") |
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: 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.
Description
This PR fixes an issue where cancelling a subtask would immediately trigger it to restart instead of properly cancelling it.
Problem
When a user cancels a subtask, the parent task immediately restarts it because it doesn't know the subtask was cancelled by the user. This creates a frustrating experience where subtasks cannot be properly cancelled.
Solution
The fix introduces a mechanism to track when a subtask is cancelled by the user and communicate this to the parent task:
Changes
Testing
Related Issues
Fixes #8504
Important
Fix subtask cancellation to prevent automatic restart and notify parent task, with tests added for verification.
wasSubtaskCancelledinTaskand notifies the parent task without restarting the subtask.completeSubtask()inTask.tsupdated to handlewasCancelledparameter.finishSubTask()inClineProvider.tsupdated to passwasCancelledflag.ClineProvider.subtask-cancel.spec.tswith tests for subtask cancellation behavior.ClineProvider.ts.This description was created by
for 5ab3c4d. You can customize this summary. It will automatically update as commits are pushed.