fix: harden delegated subtask cancellation

Author: edelauna

apps/vscode-e2e/src/fixtures/subtasks.ts

@@ -6,15 +6,24 @@ import { toolResultContains } from "./tool-result"
 const SUBTASK_PARENT_MARKER = "SUBTASK_PARENT_CANCELLATION_SMOKE"
 const SUBTASK_CHILD_MARKER = "SUBTASK_CHILD_CALCULATOR_SMOKE"
 
-const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.`
+export const SUBTASK_CHILD_PROMPT = `${SUBTASK_CHILD_MARKER}: Ask the user exactly this follow-up question: What is the square root of 81? After the user answers, complete with only the answer.`
 export const SUBTASK_PARENT_PROMPT = `${SUBTASK_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_CHILD_PROMPT}" Do not answer directly.`
 export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9"
-const INTERRUPTED_TOOL_RESULT = "Task was interrupted before this tool call could be completed."
+
+const requestContains = (req: ChatCompletionRequest, expected: string[]) => {
+	const rawRequest = JSON.stringify(req)
+	return expected.every((text) => rawRequest.includes(text))
+}
 
 const completionAfterAnswer = (followupId: string, completionId: string) => ({
 	match: {
-		toolCallId: followupId,
-		predicate: (req: ChatCompletionRequest) => toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]),
+		predicate: (req: ChatCompletionRequest) =>
+			toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]) ||
+			requestContains(req, [followupId, SUBTASK_CHILD_FOLLOWUP_ANSWER]) ||
+			requestContains(req, [
+				SUBTASK_CHILD_MARKER,
+				`<user_message>\\n${SUBTASK_CHILD_FOLLOWUP_ANSWER}\\n</user_message>`,
+			]),
 	},
 	response: {
 		toolCalls: [
@@ -64,31 +73,8 @@ export function addSubtaskFixtures(mock: InstanceType<typeof LLMock>) {
 		},
 	})
 
-	mock.addFixture({
-		match: {
-			toolCallId: "call_subtasks_child_followup_001",
-			predicate: (req) => toolResultContains(req, "call_subtasks_child_followup_001", [INTERRUPTED_TOOL_RESULT]),
-		},
-		response: {
-			toolCalls: [
-				{
-					name: "ask_followup_question",
-					arguments: JSON.stringify({
-						question: "What is the square root of 81?",
-						follow_up: [{ text: SUBTASK_CHILD_FOLLOWUP_ANSWER }],
-					}),
-					id: "call_subtasks_child_followup_resume_002",
-				},
-			],
-		},
-	})
-
 	mock.addFixture(completionAfterAnswer("call_subtasks_child_followup_001", "call_subtasks_child_completion_002"))
 
-	mock.addFixture(
-		completionAfterAnswer("call_subtasks_child_followup_resume_002", "call_subtasks_child_completion_resume_003"),
-	)
-
 	mock.addFixture({
 		match: {
 			toolCallId: "call_subtasks_parent_new_task_001",

apps/vscode-e2e/src/suite/subtasks.test.ts

@@ -3,13 +3,13 @@ import * as assert from "assert"
 import { RooCodeEventName, type ClineMessage } from "@roo-code/types"
 
 import { setDefaultSuiteTimeout } from "./test-utils"
-import { sleep, waitFor, waitUntilCompleted } from "./utils"
+import { waitFor, waitUntilCompleted } from "./utils"
 import { SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_PARENT_PROMPT } from "../fixtures/subtasks"
 
 suite("Roo Code Subtasks", function () {
 	setDefaultSuiteTimeout(this)
 
-	test("Should handle subtask cancellation and resumption correctly", async () => {
+	test("Should keep parent paused after subtask cancellation", async () => {
 		const api = globalThis.api
 		const asks: Record<string, ClineMessage[]> = {}
 		const messages: Record<string, ClineMessage[]> = {}
@@ -78,13 +78,9 @@ suite("Roo Code Subtasks", function () {
 				() => asks[spawnedTaskId!]?.some(({ type, ask }) => type === "ask" && ask === "followup") ?? false,
 			)
 			const cancelledChildTaskId = spawnedTaskId!
-			const delegatedFollowupCount =
-				asks[cancelledChildTaskId]?.filter(({ type, ask }) => type === "ask" && ask === "followup").length ?? 0
 
 			await api.cancelCurrentTask()
 
-			await sleep(2_000)
-
 			assert.ok(
 				messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") ===
 					undefined,
@@ -101,38 +97,40 @@ suite("Roo Code Subtasks", function () {
 					asks[cancelledChildTaskId]?.some(({ type, ask }) => type === "ask" && ask === "resume_task") ??
 					false,
 			)
-			await api.approveCurrentAsk()
-			await waitForStage(
-				"wait for resumed child followup ask",
-				() =>
-					(asks[cancelledChildTaskId]?.filter(({ type, ask }) => type === "ask" && ask === "followup")
-						.length ?? 0) > delegatedFollowupCount,
-			)
-			await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER)
-			await waitUntilCompleted({ api, taskId: cancelledChildTaskId })
 
+			const resumedChildTaskId = await waitUntilCompleted({
+				api,
+				start: async () => {
+					await api.sendMessage(SUBTASK_CHILD_FOLLOWUP_ANSWER)
+					return cancelledChildTaskId
+				},
+			})
+
+			assert.strictEqual(
+				resumedChildTaskId,
+				cancelledChildTaskId,
+				"Cancelled child task should be resumed in place",
+			)
 			assert.strictEqual(
-				findErrorText(cancelledChildTaskId),
+				findErrorText(resumedChildTaskId),
 				undefined,
-				"Cancelled child should not emit an error",
+				"Resumed child task should not emit an error",
 			)
 			assert.strictEqual(
-				findCompletionText(cancelledChildTaskId),
+				findCompletionText(resumedChildTaskId),
 				"9",
-				"Cancelled child should complete with `9`",
+				"Resumed child task should complete with `9`",
 			)
 			assert.strictEqual(
 				api.getCurrentTaskStack().at(-1),
 				cancelledChildTaskId,
-				"Cancelled child should stay active after resuming from cancellation",
+				"Cancelled child task should remain the active completed task",
 			)
 
-			await sleep(2_000)
-
 			assert.ok(
 				messages[parentTaskId]?.find(({ type, text }) => type === "say" && text === "Parent task resumed") ===
 					undefined,
-				"Parent task should not have resumed after subtask cancellation",
+				"Parent task should not have resumed after the cancelled child completed",
 			)
 
 			await api.clearCurrentTask()

src/core/assistant-message/presentAssistantMessage.ts

@@ -60,7 +60,7 @@ import { sanitizeToolUseId } from "../../utils/tool-id"
 
 export async function presentAssistantMessage(cline: Task) {
 	if (cline.abort) {
-		throw new Error(`[Task#presentAssistantMessage] task ${cline.taskId}.${cline.instanceId} aborted`)
+		return
 	}
 
 	if (cline.presentAssistantMessageLocked) {

src/core/webview/ClineProvider.ts

@@ -114,13 +114,11 @@ export type ClineProviderEvents = {
 	clineCreated: [cline: Task]
 }
 
-type DelegationLockHost = {
-	delegationTransitionLocks?: Map<string, Promise<void>>
-}
-
-function runDelegationTransition<T>(host: DelegationLockHost, parentTaskId: string, fn: () => Promise<T>): Promise<T> {
-	host.delegationTransitionLocks ??= new Map()
-	const locks = host.delegationTransitionLocks
+function runDelegationTransition<T>(
+	locks: Map<string, Promise<void>>,
+	parentTaskId: string,
+	fn: () => Promise<T>,
+): Promise<T> {
 	const previous = locks.get(parentTaskId) ?? Promise.resolve()
 	const current = previous.then(fn, fn)
 	const tail = current.then(
@@ -153,7 +151,7 @@ export class ClineProvider
 	private webviewDisposables: vscode.Disposable[] = []
 	private view?: vscode.WebviewView | vscode.WebviewPanel
 	private clineStack: Task[] = []
-	private delegationTransitionLocks = new Map<string, Promise<void>>()
+	private delegationTransitionLocks?: Map<string, Promise<void>>
 	private cancelledDelegationChildIds = new Set<string>()
 	private codeIndexStatusSubscription?: vscode.Disposable
 	private codeIndexManager?: CodeIndexManager
@@ -173,6 +171,11 @@ export class ClineProvider
 	private globalStateWriteThroughTimer: ReturnType<typeof setTimeout> | null = null
 	private static readonly GLOBAL_STATE_WRITE_THROUGH_DEBOUNCE_MS = 5000 // 5 seconds
 	private static readonly PENDING_OPERATION_TIMEOUT_MS = 30000 // 30 seconds
+
+	private runDelegationTransition<T>(parentTaskId: string, fn: () => Promise<T>): Promise<T> {
+		this.delegationTransitionLocks ??= new Map()
+		return runDelegationTransition(this.delegationTransitionLocks, parentTaskId, fn)
+	}
 	private readonly pendingEditOperations: PendingEditOperationStore
 
 	private cloudOrganizationsCache: CloudOrganizationMembership[] | null = null
@@ -509,7 +512,7 @@ export class ClineProvider
 			// child and will update the parent to point at the new child.
 			if (parentTaskId && childTaskId && !options?.skipDelegationRepair) {
 				try {
-					await runDelegationTransition(this as unknown as DelegationLockHost, parentTaskId, async () => {
+					await ClineProvider.prototype.runDelegationTransition.call(this, parentTaskId, async () => {
 						const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId)
 
 						if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === childTaskId) {
@@ -2995,7 +2998,7 @@ export class ClineProvider
 
 		if (task.parentTaskId) {
 			try {
-				await runDelegationTransition(this as unknown as DelegationLockHost, task.parentTaskId, async () => {
+				await ClineProvider.prototype.runDelegationTransition.call(this, task.parentTaskId, async () => {
 					const { historyItem: parentHistory } = await this.getTaskWithId(task.parentTaskId!)
 
 					if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) {
@@ -3014,12 +3017,26 @@ export class ClineProvider
 					}
 				})
 			} catch (error) {
-				// Fail closed: if we cannot prove the parent was detached, keep the
-				// rehydrated child disconnected from runtime parent links and prevent
-				// this in-process child completion from reopening the parent later.
+				// Fail closed: if we cannot prove the parent was detached, make the
+				// rehydrated child standalone so later completions cannot reopen a
+				// stale delegated parent, even after a provider reload.
 				parentTask = undefined
 				rootTask = undefined
 				this.cancelledDelegationChildIds.add(task.taskId)
+				historyItem = {
+					...historyItem,
+					parentTaskId: undefined,
+					rootTaskId: undefined,
+				}
+				try {
+					await this.updateTaskHistory(historyItem)
+				} catch (historyError) {
+					this.log(
+						`[cancelTask] Failed to persist standalone child state for ${task.taskId}: ${
+							historyError instanceof Error ? historyError.message : String(historyError)
+						}`,
+					)
+				}
 				this.log(
 					`[cancelTask] Failed to detach delegated parent for ${task.taskId}: ${
 						error instanceof Error ? error.message : String(error)
@@ -3382,7 +3399,7 @@ export class ClineProvider
 		completionResultSummary: string
 	}): Promise<boolean> {
 		const { parentTaskId, childTaskId, completionResultSummary } = params
-		return await runDelegationTransition(this as unknown as DelegationLockHost, parentTaskId, async () => {
+		return await (ClineProvider.prototype.runDelegationTransition.call(this, parentTaskId, async () => {
 			const globalStoragePath = this.contextProxy.globalStorageUri.fsPath
 
 			// 1) Load parent from history and current persisted messages
@@ -3591,7 +3608,7 @@ export class ClineProvider
 
 			;(this.cancelledDelegationChildIds as Set<string> | undefined)?.delete(childTaskId)
 			return true
-		})
+		}) as Promise<boolean>)
 	}
 
 	/**

src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts

@@ -435,6 +435,7 @@ describe("ClineProvider flicker-free cancel", () => {
 			throw new Error(`unexpected task lookup: ${id}`)
 		}) as any
 
+		const updateTaskHistorySpy = vi.spyOn(provider, "updateTaskHistory").mockResolvedValue([])
 		const createTaskWithHistoryItemSpy = vi
 			.spyOn(provider, "createTaskWithHistoryItem")
 			.mockResolvedValue(undefined as any)
@@ -444,11 +445,18 @@ describe("ClineProvider flicker-free cancel", () => {
 		expect(mockOutputChannel.appendLine).toHaveBeenCalledWith(
 			expect.stringContaining("[cancelTask] Failed to detach delegated parent for child-1: parent lookup failed"),
 		)
+		expect(updateTaskHistorySpy).toHaveBeenCalledWith(
+			expect.objectContaining({
+				id: "child-1",
+				parentTaskId: undefined,
+				rootTaskId: undefined,
+			}),
+		)
 		expect(createTaskWithHistoryItemSpy).toHaveBeenCalledWith(
 			expect.objectContaining({
 				id: "child-1",
-				parentTaskId: "parent-1",
-				rootTaskId: "root-1",
+				parentTaskId: undefined,
+				rootTaskId: undefined,
 				parentTask: undefined,
 				rootTask: undefined,
 			}),