-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: resume parent task when subtask gets interrupted #7781
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 |
|---|---|---|
|
|
@@ -1228,6 +1228,12 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| } | ||
| } | ||
|
|
||
| // Check if this task was paused waiting for a subtask that got interrupted | ||
| // If so, handle the interrupted subtask properly | ||
| if (this.isPaused && this.childTaskId) { | ||
| await this.handleInterruptedSubtask() | ||
| } | ||
|
|
||
| const modifiedClineMessages = await this.getSavedClineMessages() | ||
|
|
||
| // Check for any stored GPT-5 response IDs in the message history. | ||
|
|
@@ -1657,6 +1663,55 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handle the case when a subtask was interrupted (e.g., due to 5-hour limit) | ||
| * and the parent task needs to be resumed without the subtask result. | ||
| */ | ||
| public async handleInterruptedSubtask() { | ||
|
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. I notice this method appears to be duplicated in the diff. Is this intentional, or did the same method get added twice? We should have only one definition of handleInterruptedSubtask().
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. This critical functionality lacks test coverage. Could we add unit tests to verify that:
|
||
| if (!this.isPaused || !this.childTaskId) { | ||
| return // Not waiting for a subtask | ||
| } | ||
|
|
||
| const provider = this.providerRef.deref() | ||
| provider?.log( | ||
| `[handleInterruptedSubtask] Handling interrupted subtask ${this.childTaskId} for parent task ${this.taskId}`, | ||
| ) | ||
|
|
||
| // Clear the paused state | ||
| this.isPaused = false | ||
| const interruptedChildId = this.childTaskId | ||
| this.childTaskId = undefined | ||
|
|
||
| // Clear any pause interval | ||
| if (this.pauseInterval) { | ||
| clearInterval(this.pauseInterval) | ||
| this.pauseInterval = undefined | ||
| } | ||
|
|
||
| // Emit event to update UI | ||
| this.emit(RooCodeEventName.TaskUnpaused, this.taskId) | ||
|
|
||
| // Add a message to the conversation history indicating the subtask was interrupted | ||
| try { | ||
| await this.say("subtask_result", `[Subtask ${interruptedChildId} was interrupted and could not complete]`) | ||
|
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. Consider extracting this message format to a constant or configuration for better maintainability. We might want to reuse this format elsewhere or make it configurable. |
||
|
|
||
| await this.addToApiConversationHistory({ | ||
| role: "user", | ||
| content: [ | ||
| { | ||
| type: "text", | ||
| text: `[new_task interrupted] The subtask was interrupted before completion. Please continue with the main task.`, | ||
| }, | ||
| ], | ||
| }) | ||
|
|
||
| // Set skipPrevResponseIdOnce to ensure continuity | ||
| this.skipPrevResponseIdOnce = true | ||
| } catch (error) { | ||
| provider?.log(`Error handling interrupted subtask: ${error}`) | ||
|
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. The error is only logged but not propagated. Is silent failure the intended behavior here? Should we consider rethrowing the error or at least emitting an error event? |
||
| } | ||
| } | ||
|
|
||
| // Task Loop | ||
|
|
||
| private async initiateTaskLoop(userContent: Anthropic.Messages.ContentBlockParam[]): Promise<void> { | ||
|
|
||
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.
Could there be a race condition here? What happens if the child task completes between checking this.isPaused && this.childTaskId and calling handleInterruptedSubtask()? Should we consider using a lock or atomic operation?