diff --git a/apps/vscode-e2e/src/fixtures/search-files.ts b/apps/vscode-e2e/src/fixtures/search-files.ts index 3c1318c299..b653e199c0 100644 --- a/apps/vscode-e2e/src/fixtures/search-files.ts +++ b/apps/vscode-e2e/src/fixtures/search-files.ts @@ -89,7 +89,7 @@ export function addSearchFilesResultFixtures(mock: InstanceType) toolName: "search_files", arguments: '{"path":"search-files-tool-fixture","regex":"nonExistentPattern12345"}', toolCallId: "call_search_files_no_match_001", - expected: ["No results found"], + expected: ["Found 0 results."], result: "No matches were found for `nonExistentPattern12345` in the search fixture directory.", id: "call_search_files_no_match_002", }, diff --git a/apps/vscode-e2e/src/fixtures/subtasks.ts b/apps/vscode-e2e/src/fixtures/subtasks.ts index f5dfc813df..73cc4a8ff8 100644 --- a/apps/vscode-e2e/src/fixtures/subtasks.ts +++ b/apps/vscode-e2e/src/fixtures/subtasks.ts @@ -14,6 +14,10 @@ const SUBTASK_FAST_CHILD_MARKER = "SUBTASK_CHILD_IMMEDIATE_COMPLETION" const SUBTASK_XPROFILE_PARENT_MARKER = "SUBTASK_PARENT_CROSS_PROFILE" const SUBTASK_XPROFILE_SAME_CHILD_MARKER = "SUBTASK_CHILD_SAME_PROFILE" const SUBTASK_XPROFILE_DIFFERENT_CHILD_MARKER = "SUBTASK_CHILD_DIFFERENT_PROFILE" +const SUBTASK_FANOUT_PARENT_MARKER = "SUBTASK_PARENT_FANOUT_CONCURRENT" +const SUBTASK_FANOUT_CHILD_MARKER = "SUBTASK_CHILD_FANOUT_CONCURRENT" +const SUBTASK_FANOUT_XPROFILE_PARENT_MARKER = "SUBTASK_PARENT_FANOUT_CROSS_PROFILE" +const SUBTASK_FANOUT_XPROFILE_CHILD_MARKER = "SUBTASK_CHILD_FANOUT_CROSS_PROFILE" 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.` @@ -21,6 +25,16 @@ export const SUBTASK_CHILD_FOLLOWUP_ANSWER = "9" export const SUBTASK_FAST_CHILD_RESULT = "Fast child completed" const SUBTASK_FAST_CHILD_PROMPT = `${SUBTASK_FAST_CHILD_MARKER}: Complete immediately with the exact result "${SUBTASK_FAST_CHILD_RESULT}".` export const SUBTASK_FAST_PARENT_PROMPT = `${SUBTASK_FAST_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FAST_CHILD_PROMPT}" Do not answer directly.` +export const SUBTASK_FANOUT_PARENT_FOLLOWUP = "Parent fan-out is still active?" +export const SUBTASK_FANOUT_CHILD_RESULT = "Fan-out child completed" +const SUBTASK_FANOUT_CHILD_PROMPT = `${SUBTASK_FANOUT_CHILD_MARKER}: Complete with the exact result "${SUBTASK_FANOUT_CHILD_RESULT}".` +export const SUBTASK_FANOUT_PARENT_PROMPT = `${SUBTASK_FANOUT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FANOUT_CHILD_PROMPT}" After delegation, ask the user exactly this follow-up question: ${SUBTASK_FANOUT_PARENT_FOLLOWUP}` +export const SUBTASK_FANOUT_XPROFILE_PARENT_MODEL = "openai/gpt-4.1" +export const SUBTASK_FANOUT_XPROFILE_CHILD_MODEL = "openai/gpt-4.1-mini" +export const SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP = "Parent cross-profile fan-out is still isolated?" +export const SUBTASK_FANOUT_XPROFILE_CHILD_RESULT = "Fan-out cross-profile child completed" +const SUBTASK_FANOUT_XPROFILE_CHILD_PROMPT = `${SUBTASK_FANOUT_XPROFILE_CHILD_MARKER}: Complete with the exact result "${SUBTASK_FANOUT_XPROFILE_CHILD_RESULT}".` +export const SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT = `${SUBTASK_FANOUT_XPROFILE_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_FANOUT_XPROFILE_CHILD_PROMPT}" After delegation, ask the user exactly this follow-up question: ${SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP}` const SUBTASK_INTERRUPT_CHILD_PROMPT = `${SUBTASK_INTERRUPT_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_INTERRUPT_PARENT_PROMPT = `${SUBTASK_INTERRUPT_PARENT_MARKER}: Use the new_task tool exactly once. Create an ask-mode subtask with this exact message: "${SUBTASK_INTERRUPT_CHILD_PROMPT}" Do not answer directly. When the subtask returns, complete with the exact result "Interrupted parent resumed".` @@ -135,6 +149,120 @@ export function addSubtaskFixtures(mock: InstanceType) { }, }) + mock.addFixture({ + match: { + userMessage: new RegExp(SUBTASK_FANOUT_PARENT_MARKER), + sequenceIndex: 0, + }, + response: { + toolCalls: [ + { + name: "new_task", + arguments: JSON.stringify({ + mode: "ask", + message: SUBTASK_FANOUT_CHILD_PROMPT, + }), + id: "call_subtasks_fanout_parent_new_task_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + lastUserMessageContains(req, SUBTASK_FANOUT_CHILD_MARKER) && + !requestContains(req, [SUBTASK_FANOUT_PARENT_MARKER]), + }, + latency: 15_000, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_FANOUT_CHILD_RESULT }), + id: "call_subtasks_fanout_child_completion_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + requestContains(req, [SUBTASK_FANOUT_PARENT_MARKER, "Delegated to child task"]) && + !requestContains(req, [SUBTASK_RESULT_INJECTION]), + }, + response: { + toolCalls: [ + { + name: "ask_followup_question", + arguments: JSON.stringify({ + question: SUBTASK_FANOUT_PARENT_FOLLOWUP, + follow_up: [{ text: "continue" }], + }), + id: "call_subtasks_fanout_parent_followup_003", + }, + ], + }, + }) + + mock.addFixture({ + match: { + userMessage: new RegExp(SUBTASK_FANOUT_XPROFILE_PARENT_MARKER), + sequenceIndex: 0, + }, + response: { + toolCalls: [ + { + name: "new_task", + arguments: JSON.stringify({ + mode: "ask", + message: SUBTASK_FANOUT_XPROFILE_CHILD_PROMPT, + }), + id: "call_subtasks_fanout_xprofile_parent_new_task_001", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + lastUserMessageContains(req, SUBTASK_FANOUT_XPROFILE_CHILD_MARKER) && + !requestContains(req, [SUBTASK_FANOUT_XPROFILE_PARENT_MARKER]), + }, + latency: 15_000, + response: { + toolCalls: [ + { + name: "attempt_completion", + arguments: JSON.stringify({ result: SUBTASK_FANOUT_XPROFILE_CHILD_RESULT }), + id: "call_subtasks_fanout_xprofile_child_completion_002", + }, + ], + }, + }) + + mock.addFixture({ + match: { + predicate: (req: ChatCompletionRequest) => + requestContains(req, [SUBTASK_FANOUT_XPROFILE_PARENT_MARKER, "Delegated to child task"]) && + !requestContains(req, [SUBTASK_RESULT_INJECTION]), + }, + response: { + toolCalls: [ + { + name: "ask_followup_question", + arguments: JSON.stringify({ + question: SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP, + follow_up: [{ text: "continue" }], + }), + id: "call_subtasks_fanout_xprofile_parent_followup_003", + }, + ], + }, + }) + // The parent prompt embeds SUBTASK_FAST_CHILD_MARKER verbatim, so parent-resume turns // can also match a bare substring check (same collision class as #561). Exclude the // parent marker so those turns fall through to the parent-resume fixture below. diff --git a/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts b/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts index 2fa9fc25b6..14d9293ff8 100644 --- a/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts +++ b/apps/vscode-e2e/src/fixtures/terminal-reuse-shell-race.ts @@ -4,10 +4,19 @@ import { toolResultContains } from "./tool-result" export function addTerminalReuseShellRaceFixtures(mock: InstanceType) { // First command completes — model issues a second command on the same terminal. - // With the temp-script fix, both commands now deliver real output. mock.addFixture({ match: { - predicate: (req) => toolResultContains(req, "call_terminal_reuse_001", ["first", "Exit code: 0"]), + predicate: (req) => { + const messages = Array.isArray(req?.messages) ? req.messages : [] + const lastToolMsg = messages.filter((message) => message?.role === "tool").at(-1) + + return ( + lastToolMsg?.tool_call_id === "call_terminal_reuse_001" && + toolResultContains(req, "call_terminal_reuse_001", [ + "Command was submitted in the VS Code terminal", + ]) + ) + }, }, response: { toolCalls: [ @@ -25,7 +34,17 @@ export function addTerminalReuseShellRaceFixtures(mock: InstanceType toolResultContains(req, "call_terminal_reuse_002", ["second", "Exit code: 0"]), + predicate: (req) => { + const messages = Array.isArray(req?.messages) ? req.messages : [] + const lastToolMsg = messages.filter((message) => message?.role === "tool").at(-1) + + return ( + lastToolMsg?.tool_call_id === "call_terminal_reuse_002" && + toolResultContains(req, "call_terminal_reuse_002", [ + "Command was submitted in the VS Code terminal", + ]) + ) + }, }, response: { toolCalls: [ diff --git a/apps/vscode-e2e/src/suite/subtasks.test.ts b/apps/vscode-e2e/src/suite/subtasks.test.ts index 9e1d0e83cb..ad40c4a0ba 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -19,6 +19,13 @@ import { SUBTASK_API_HANG_RESUME_MESSAGE, SUBTASK_CHILD_FOLLOWUP_ANSWER, SUBTASK_FAST_CHILD_RESULT, + SUBTASK_FANOUT_PARENT_FOLLOWUP, + SUBTASK_FANOUT_PARENT_PROMPT, + SUBTASK_FANOUT_XPROFILE_CHILD_MODEL, + SUBTASK_FANOUT_XPROFILE_CHILD_RESULT, + SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP, + SUBTASK_FANOUT_XPROFILE_PARENT_MODEL, + SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT, SUBTASK_FAST_PARENT_PROMPT, SUBTASK_INTERRUPT_CHILD_FOLLOWUP_ANSWER, SUBTASK_INTERRUPT_PARENT_PROMPT, @@ -34,6 +41,7 @@ type AimockMessageContent = string | Array<{ type?: string; text?: string }> type AimockJournalEntry = { body?: { + model?: string messages?: Array<{ role?: string content?: AimockMessageContent @@ -49,24 +57,55 @@ const messageContentText = (content?: AimockMessageContent) => { return content?.map((part) => part.text ?? "").join("") ?? "" } +const requestUserText = (entry: AimockJournalEntry) => + (entry.body?.messages ?? []) + .filter((message) => message.role === "user") + .map((message) => messageContentText(message.content)) + .join("") + const waitForAimockRequestContaining = async (expectedText: string, excludeText?: string) => { + await waitForAimockRequest((entry) => { + const messages = entry.body?.messages + if (!messages) return false + const entryText = messages.map((m) => messageContentText(m.content)).join("") + if (excludeText && entryText.includes(excludeText)) return false + return messages.some( + (message) => message.role === "user" && messageContentText(message.content).includes(expectedText), + ) + }) +} + +const waitForAimockRequest = async (matches: (entry: AimockJournalEntry) => boolean) => { + let latestEntries: AimockJournalEntry[] = [] + + try { + await waitFor(async () => { + latestEntries = await readAimockJournal() + return latestEntries.some(matches) + }) + } catch { + assertAimockRequest(latestEntries, matches) + } +} + +const readAimockJournal = async () => { const aimockUrl = process.env.AIMOCK_URL assert.ok(aimockUrl, "AIMOCK_URL must be set for aimock journal assertions") - await waitFor(async () => { - const response = await fetch(`${aimockUrl}/__aimock/journal`) - const entries = (await response.json()) as AimockJournalEntry[] - - return entries.some((entry) => { - const messages = entry.body?.messages - if (!messages) return false - const entryText = messages.map((m) => messageContentText(m.content)).join("") - if (excludeText && entryText.includes(excludeText)) return false - return messages.some( - (message) => message.role === "user" && messageContentText(message.content).includes(expectedText), - ) - }) - }) + const response = await fetch(`${aimockUrl}/__aimock/journal`) + return (await response.json()) as AimockJournalEntry[] +} + +const assertAimockRequest = (entries: AimockJournalEntry[], matches: (entry: AimockJournalEntry) => boolean) => { + if (entries.some(matches)) return + + const summary = entries.map((entry) => ({ + model: entry.body?.model, + userText: entry.body?.messages + ?.filter((message) => message.role === "user") + .map((message) => messageContentText(message.content).slice(0, 180)), + })) + assert.fail(`Expected aimock request was not found. Requests: ${JSON.stringify(summary, null, 2)}`) } suite("Roo Code Subtasks", function () { @@ -129,6 +168,191 @@ suite("Roo Code Subtasks", function () { } }) + test("fan-out keeps parent executing while child request is in flight", async () => { + const api = globalThis.api + const asks: Record = {} + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } + } + + api.on(RooCodeEventName.Message, messageHandler) + + try { + api.setTaskSchedulerMaxConcurrency(2) + + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "ask", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_FANOUT_PARENT_PROMPT, + }) + + let childTaskId: string | undefined + await waitFor(() => { + const stack = api.getCurrentTaskStack() + const current = stack.at(-1) + if (current && current !== parentTaskId) { + childTaskId = current + return stack.includes(parentTaskId) + } + return false + }) + + await waitFor(() => + (asks[parentTaskId] ?? []).some( + ({ ask, text }) => ask === "followup" && text?.includes(SUBTASK_FANOUT_PARENT_FOLLOWUP), + ), + ) + + const stack = api.getCurrentTaskStack() + assert.ok(stack.includes(parentTaskId), "Fan-out parent should remain in the live task stack") + assert.ok(stack.includes(childTaskId!), "Fan-out child should remain in the live task stack") + assert.strictEqual(stack.at(-1), childTaskId, "Child should remain the focused task while parent runs") + + const parentHistory = await api.getTaskHistoryItem(parentTaskId) + assert.strictEqual(parentHistory?.status, "delegated", "Fan-out parent history should stay delegated") + assert.strictEqual( + parentHistory?.awaitingChildId, + childTaskId, + "Fan-out parent history should point at the running child", + ) + assert.strictEqual( + parentHistory?.delegatedToId, + childTaskId, + "Fan-out parent history should record the delegated child", + ) + } finally { + api.off(RooCodeEventName.Message, messageHandler) + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } + api.setTaskSchedulerMaxConcurrency(1) + await waitFor(() => api.getCurrentTaskStack().length === 0).catch(() => {}) + } + }) + + test("fan-out keeps parent API config isolated when child switches to a different saved profile", async () => { + const api = globalThis.api + const asks: Record = {} + + const messageHandler = ({ taskId, message }: { taskId: string; message: ClineMessage }) => { + if (message.type === "ask") { + asks[taskId] = asks[taskId] || [] + asks[taskId].push(message) + } + } + + api.on(RooCodeEventName.Message, messageHandler) + + const aimockUrl = process.env.AIMOCK_URL + const parentProfile = { + apiProvider: "openrouter" as const, + openRouterApiKey: "mock-key", + openRouterModelId: SUBTASK_FANOUT_XPROFILE_PARENT_MODEL, + rateLimitSeconds: 0, + ...(aimockUrl && { openRouterBaseUrl: `${aimockUrl}/v1` }), + } + const childProfile = { + ...parentProfile, + openRouterModelId: SUBTASK_FANOUT_XPROFILE_CHILD_MODEL, + } + const priorModeApiConfigs = api.getConfiguration().modeApiConfigs ?? {} + const parentProfileId = await api.upsertProfile("subtask-fanout-parent-profile", parentProfile, true) + const childProfileId = await api.upsertProfile("subtask-fanout-child-profile", childProfile, false) + assert.ok(parentProfileId, "Failed to create parent profile") + assert.ok(childProfileId, "Failed to create child profile") + await api.setConfiguration({ + modeApiConfigs: { + ...priorModeApiConfigs, + code: parentProfileId, + ask: childProfileId, + }, + }) + + try { + api.setTaskSchedulerMaxConcurrency(2) + + const parentTaskId = await api.startNewTask({ + configuration: { + mode: "code", + alwaysAllowModeSwitch: true, + alwaysAllowSubtasks: true, + autoApprovalEnabled: true, + enableCheckpoints: false, + }, + text: SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT, + }) + + let childTaskId: string | undefined + await waitFor(() => { + const stack = api.getCurrentTaskStack() + const current = stack.at(-1) + if (current && current !== parentTaskId) { + childTaskId = current + return stack.includes(parentTaskId) + } + return false + }) + + await waitFor(() => + (asks[parentTaskId] ?? []).some( + ({ ask, text }) => ask === "followup" && text?.includes(SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP), + ), + ) + + assert.ok( + (asks[parentTaskId] ?? []).some( + ({ ask, text }) => ask === "followup" && text?.includes(SUBTASK_FANOUT_XPROFILE_PARENT_FOLLOWUP), + ), + "Parent should keep running and ask its follow-up using its own profile", + ) + + const stack = api.getCurrentTaskStack() + assert.ok(stack.includes(parentTaskId), "Fan-out parent should remain in the live task stack") + assert.ok(stack.includes(childTaskId!), "Fan-out child should remain in the live task stack") + assert.strictEqual(stack.at(-1), childTaskId, "Child should remain the focused task while parent runs") + + await waitForAimockRequest((entry) => { + const text = requestUserText(entry) + const userMessageCount = (entry.body?.messages ?? []).filter( + (message) => message.role === "user", + ).length + return ( + entry.body?.model === SUBTASK_FANOUT_XPROFILE_PARENT_MODEL && + text.includes(SUBTASK_FANOUT_XPROFILE_PARENT_PROMPT) && + userMessageCount > 1 + ) + }) + await waitForAimockRequest( + (entry) => + entry.body?.model === SUBTASK_FANOUT_XPROFILE_CHILD_MODEL && + (entry.body.messages ?? []).some( + (message) => + message.role === "user" && + messageContentText(message.content).includes(SUBTASK_FANOUT_XPROFILE_CHILD_RESULT), + ), + ) + } finally { + api.off(RooCodeEventName.Message, messageHandler) + while (api.getCurrentTaskStack().length > 0) { + await api.clearCurrentTask() + } + api.setTaskSchedulerMaxConcurrency(1) + await waitFor(() => api.getCurrentTaskStack().length === 0).catch(() => {}) + await api.setConfiguration({ modeApiConfigs: priorModeApiConfigs }) + await api.deleteProfile("subtask-fanout-child-profile").catch(() => {}) + await api.deleteProfile("subtask-fanout-parent-profile").catch(() => {}) + } + }) + // Smoke: child completing normally must resume the parent task. test("child task returns to parent after normal completion", async () => { const api = globalThis.api diff --git a/packages/types/src/api.ts b/packages/types/src/api.ts index 89e9c8bc2b..07a730e6a1 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -56,6 +56,11 @@ export interface RooCodeAPI extends EventEmitter { * @returns An array of task IDs. */ getCurrentTaskStack(): string[] + /** + * Sets the TaskScheduler concurrency for extension-host tests. + * Intended for test/integration harnesses that need to exercise fan-out. + */ + setTaskSchedulerMaxConcurrency(maxConcurrency: number): void /** * Clears the current task. */ diff --git a/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts index 59dde33933..346abf73ac 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -13,12 +13,20 @@ type ProviderStubFields = { runDelegationTransition?: unknown removeClineFromStack?: unknown evictCurrentTask?: unknown + restoreParentOrReleasePermit?: unknown } type PrivateProviderMethods = { runDelegationTransition: (this: unknown, ...args: unknown[]) => unknown removeClineFromStack: (this: unknown, ...args: unknown[]) => unknown evictCurrentTask: (this: unknown, ...args: unknown[]) => unknown + restoreParentOrReleasePermit: (this: unknown, ...args: unknown[]) => unknown +} + +export function bindRestoreParentOrReleasePermit(provider: ClineProvider): void { + const s = provider as unknown as ProviderStubFields + const proto = ClineProvider.prototype as unknown as PrivateProviderMethods + s.restoreParentOrReleasePermit = proto.restoreParentOrReleasePermit.bind(s) } /** @@ -51,5 +59,8 @@ export function makeProviderStub(stub: T): ClineProvider { s.runDelegationTransition ??= proto.runDelegationTransition.bind(s) s.removeClineFromStack ??= proto.removeClineFromStack.bind(s) s.evictCurrentTask ??= proto.evictCurrentTask.bind(s) + if (!s.restoreParentOrReleasePermit) { + bindRestoreParentOrReleasePermit(s as unknown as ClineProvider) + } return s as unknown as ClineProvider } diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 0154027753..d3bd639931 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -5,6 +5,7 @@ import type { HistoryItem } from "@roo-code/types" import { RooCodeEventName } from "@roo-code/types" import { ClineProvider } from "../core/webview/ClineProvider" import { TaskScheduler } from "../core/task/TaskScheduler" +import { bindRestoreParentOrReleasePermit } from "./helpers/provider-stub" const parentHistoryItem: HistoryItem = { id: "parent-1", @@ -318,6 +319,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindRestoreParentOrReleasePermit(provider) await expect( (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { @@ -369,7 +371,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { isViewLaunched: false, recentTasksCache: undefined, taskHistoryStore, + // Rollback looks up the just-created child by id to decide whether it + // still needs evicting, independent of current focus. + taskRegistry: { getById: vi.fn((id: string) => (id === "child-1" ? child : undefined)) }, } as unknown as ClineProvider + bindRestoreParentOrReleasePermit(provider) await expect( (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { @@ -381,8 +387,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { ).rejects.toThrow(persistError) expect(childRun).not.toHaveBeenCalled() + // 1st call (step 3): closes the parent to enforce the single-open invariant. expect(removeClineFromStack).toHaveBeenNthCalledWith(1) - expect(removeClineFromStack).toHaveBeenNthCalledWith(2) + // 2nd call (rollback): evicts the just-created child by id, regardless of + // current focus — see Story 3.2b fan-out rollback fix. + expect(removeClineFromStack).toHaveBeenNthCalledWith(2, "child-1") expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistoryItem) }) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 8f69a3a0d4..f789ee7020 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1591,7 +1591,10 @@ export class Task extends EventEmitter implements TaskLike { // Get condensing configuration const state = await this.providerRef.deref()?.getState() const customCondensingPrompt = state?.customSupportPrompts?.CONDENSE - const { mode, apiConfiguration } = state ?? {} + // mode/apiConfiguration come from this task's own fields, not shared + // provider state — see getSystemPrompt() for why. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration const { contextTokens: prevContextTokens } = this.getTokenUsage() @@ -3780,16 +3783,16 @@ export class Task extends EventEmitter implements TaskLike { const state = await this.providerRef.deref()?.getState() - const { - mode, - customModes, - customModePrompts, - customInstructions, - experiments, - language, - apiConfiguration, - enableSubfolderRules, - } = state ?? {} + const { customModes, customModePrompts, customInstructions, experiments, language, enableSubfolderRules } = + state ?? {} + + // mode and apiConfiguration are read from this task's own fields, not the + // shared provider state — provider.getState().mode/apiConfiguration reflect + // whichever task is currently focused, which during delegation fan-out can + // be a different, concurrently-running task. Reading global state here would + // leak the focused task's mode/config into this task's own system prompt. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration return await (async () => { const provider = this.providerRef.deref() @@ -3839,7 +3842,11 @@ export class Task extends EventEmitter implements TaskLike { private async handleContextWindowExceededError(): Promise { const state = await this.providerRef.deref()?.getState() - const { profileThresholds = {}, mode, apiConfiguration } = state ?? {} + const { profileThresholds = {} } = state ?? {} + // mode/apiConfiguration come from this task's own fields, not shared + // provider state — see getSystemPrompt() for why. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration const { contextTokens } = this.getTokenUsage() const modelInfo = this.api.getModel().info @@ -3978,9 +3985,10 @@ export class Task extends EventEmitter implements TaskLike { * the `api_req_rate_limit_wait` say type (not an error). */ private async maybeWaitForProviderRateLimit(retryAttempt: number): Promise { - const state = await this.providerRef.deref()?.getState() - const rateLimitSeconds = - state?.apiConfiguration?.rateLimitSeconds ?? this.apiConfiguration?.rateLimitSeconds ?? 0 + // This task's own apiConfiguration takes precedence over shared provider + // state, which during delegation fan-out reflects whichever task is + // currently focused — see getSystemPrompt() for the general rationale. + const rateLimitSeconds = this.apiConfiguration?.rateLimitSeconds ?? 0 const lastRequestTime = this.rateLimitClock.getLastRequestTime() if (rateLimitSeconds <= 0 || !lastRequestTime) { @@ -4013,14 +4021,16 @@ export class Task extends EventEmitter implements TaskLike { const state = await this.providerRef.deref()?.getState() const { - apiConfiguration, autoApprovalEnabled, requestDelaySeconds, - mode, autoCondenseContext = true, autoCondenseContextPercent = 100, profileThresholds = {}, } = state ?? {} + // mode/apiConfiguration come from this task's own fields, not shared + // provider state — see getSystemPrompt() for why. + const mode = await this.getTaskMode() + const apiConfiguration = this.apiConfiguration // Get condensing configuration for automatic triggers. const customCondensingPrompt = state?.customSupportPrompts?.CONDENSE @@ -4432,7 +4442,7 @@ export class Task extends EventEmitter implements TaskLike { // Respect provider rate limit window let rateLimitDelay = 0 - const rateLimit = (state?.apiConfiguration ?? this.apiConfiguration)?.rateLimitSeconds || 0 + const rateLimit = this.apiConfiguration?.rateLimitSeconds || 0 const lastRequestTime = this.rateLimitClock.getLastRequestTime() if (lastRequestTime && rateLimit > 0) { const elapsed = performance.now() - lastRequestTime diff --git a/src/core/task/TaskScheduler.ts b/src/core/task/TaskScheduler.ts index 7431fb76ad..b0cbdcf979 100644 --- a/src/core/task/TaskScheduler.ts +++ b/src/core/task/TaskScheduler.ts @@ -10,8 +10,10 @@ import { type Task } from "./Task" */ export class TaskScheduler { private readonly sem: TaskSemaphore + readonly maxConcurrency: number constructor(maxConcurrency = 1) { + this.maxConcurrency = maxConcurrency this.sem = new TaskSemaphore(maxConcurrency) } @@ -19,6 +21,26 @@ export class TaskScheduler { return this.sem.waiting } + /** Number of permits not currently held by a running task. */ + get available(): number { + return this.sem.available + } + + /** + * Reserve a permit only if one is immediately free, without queueing. + * Returns a release function on success, or `undefined` if none was free. + * + * Use this (not `available > 0` followed later by `schedule()`) when a + * caller needs to make an irreversible decision — e.g. keeping a parent + * task alive for fan-out — based on whether a child can actually run + * concurrently. Checking `available` and then `await`-ing other work + * before calling `schedule()` leaves a window where another caller can + * consume the last permit; reserving it immediately closes that window. + */ + async tryReserve(): Promise<(() => void) | undefined> { + return this.sem.tryAcquire() + } + /** * Acquire a permit for `task`, call `run()`, and release on completion. * @@ -30,7 +52,19 @@ export class TaskScheduler { * without calling `run()`. */ async schedule(task: Task, run: () => Promise): Promise { - const release = await this.sem.acquire() + return this.runWithRelease(await this.sem.acquire(), task, run) + } + + /** + * Run `task` using a permit already obtained via `tryReserve()`, instead of + * acquiring a new one. Same abort/abandon and release-on-completion + * semantics as `schedule()`. + */ + async runWithReservation(release: () => void, task: Task, run: () => Promise): Promise { + return this.runWithRelease(release, task, run) + } + + private async runWithRelease(release: () => void, task: Task, run: () => Promise): Promise { if (task.abort || task.abandoned) { release() return diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index d9f7240c5c..56718ceb71 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -16,6 +16,7 @@ import { import { TelemetryService } from "@roo-code/telemetry" import { Task } from "../Task" +import { SYSTEM_PROMPT } from "../../prompts/system" import { createRateLimitClock } from "../RateLimitClock" import { summarizeConversation } from "../../condense" import { ClineProvider } from "../../webview/ClineProvider" @@ -213,6 +214,15 @@ vi.mock("../../condense", async (importOriginal) => { }), } }) +// Spy on SYSTEM_PROMPT's mode argument without changing its behavior for tests +// that mock getSystemPrompt() at a higher level and never reach this call. +vi.mock("../../prompts/system", async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + SYSTEM_PROMPT: vi.fn(actual.SYSTEM_PROMPT), + } +}) // Mock storagePathManager to prevent dynamic import issues. vi.mock("../../../utils/storage", () => ({ getTaskDirectoryPath: vi @@ -441,6 +451,68 @@ describe("Cline", () => { }) }) + describe("getSystemPrompt mode/apiConfiguration isolation", () => { + // Regression test for delegation fan-out (Story 3.2b): a parent task must + // keep using its own mode and apiConfiguration even while shared provider + // state has moved on to reflect a concurrently-running child (e.g. during + // delegateParentAndOpenChild's fan-out window, before the child registers + // itself as current). Reading provider.getState().mode/apiConfiguration + // directly — instead of the task's own fields — would leak the child's + // mode/config into the parent's next system prompt. + it("uses the task's own mode, not provider.getState().mode, when they diverge", async () => { + mockProvider.getState = vi.fn().mockResolvedValue({ mode: "architect", mcpEnabled: false }) + + const cline = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + await cline.getTaskMode() + + // Simulate a concurrently-running child having switched shared provider + // state to a different mode (what handleModeSwitch does today). + mockProvider.getState = vi.fn().mockResolvedValue({ mode: "code", mcpEnabled: false }) + vi.mocked(SYSTEM_PROMPT).mockResolvedValueOnce("mock system prompt") + + await getTaskTestAccess(cline).getSystemPrompt() + + expect(cline.taskMode).toBe("architect") + const [, , , , , modeArg] = vi.mocked(SYSTEM_PROMPT).mock.calls.at(-1)! + expect(modeArg).toBe("architect") + }) + + it("uses the task's own apiConfiguration, not provider.getState().apiConfiguration, when they diverge", async () => { + const parentApiConfig: ProviderSettings = { + ...mockApiConfig, + todoListEnabled: true, + } + + const cline = new Task({ + provider: mockProvider, + apiConfiguration: parentApiConfig, + task: "test task", + startTask: false, + }) + await cline.getTaskMode() + + // Simulate shared provider state now reflecting a different task's + // (the child's) apiConfiguration — opposite todoListEnabled value so + // a leak is directly observable in SYSTEM_PROMPT's settings argument. + mockProvider.getState = vi.fn().mockResolvedValue({ + mode: "code", + mcpEnabled: false, + apiConfiguration: { ...mockApiConfig, todoListEnabled: false }, + }) + vi.mocked(SYSTEM_PROMPT).mockResolvedValueOnce("mock system prompt") + + await getTaskTestAccess(cline).getSystemPrompt() + + const [, , , , , , , , , , , , settingsArg] = vi.mocked(SYSTEM_PROMPT).mock.calls.at(-1)! + expect((settingsArg as { todoListEnabled?: boolean }).todoListEnabled).toBe(true) + }) + }) + describe("sayAndCreateMissingParamError", () => { it("surfaces a localized error notice and returns the missing-parameter tool error for both relPath branches", async () => { const cline = new Task({ @@ -530,6 +602,57 @@ describe("Cline", () => { expect(Object.keys(cleanConversationHistory[0]!)).toEqual(["role", "content"]) }) + it("uses task-local mode and apiConfiguration in request metadata when provider state diverges", async () => { + const taskApiConfiguration = { + ...mockApiConfig, + apiProvider: providerIdentifiers.gemini, + } as ProviderSettings + + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + mode: "ask", + apiConfiguration: taskApiConfiguration, + autoApprovalEnabled: true, + requestDelaySeconds: 0, + }) + + const cline = new Task({ + provider: mockProvider, + apiConfiguration: taskApiConfiguration, + task: "test task", + startTask: false, + }) + await cline.getTaskMode() + vi.spyOn(getTaskTestAccess(cline), "getSystemPrompt").mockResolvedValue("mock system prompt") + vi.spyOn(cline.api, "getModel").mockReturnValue({ + id: requireDefined(mockApiConfig.apiModelId), + info: { contextWindow: 200000, maxTokens: 4096 } as ModelInfo, + }) + + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + mode: "code", + apiConfiguration: { + ...mockApiConfig, + apiProvider: providerIdentifiers.anthropic, + }, + autoApprovalEnabled: true, + requestDelaySeconds: 0, + }) + + const mockStream = (async function* () { + yield { type: "text", text: "response" } as ApiStreamChunk + })() + const createMessageSpy = vi.spyOn(cline.api, "createMessage").mockReturnValue(mockStream) + cline.apiConversationHistory = [ + { role: "user", content: [{ type: "text", text: "test message" }], ts: Date.now() }, + ] + + await cline.attemptApiRequest(0).next() + + const [, , metadata] = requireDefined(createMessageSpy.mock.calls[0]) + expect(metadata?.mode).toBe("ask") + expect(metadata?.allowedFunctionNames).toBeDefined() + }) + it("should shape image blocks for API compatibility before request construction", async () => { const conversationHistory = [ { @@ -681,7 +804,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() { @@ -700,7 +823,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() { @@ -772,7 +895,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() {}, @@ -788,7 +911,7 @@ describe("Cline", () => { async return() { return { done: true, value: undefined } }, - async throw(e: any) { + async throw(e: unknown) { throw e }, async [Symbol.asyncDispose]() {}, @@ -820,6 +943,82 @@ describe("Cline", () => { expect(clock.getLastRequestTime()).toBeDefined() }) + it("uses the task's own rate limit for retry backoff when provider state belongs to another task", async () => { + const clock = createRateLimitClock() + const cline = new Task({ + provider: mockProvider, + apiConfiguration: { + ...mockApiConfig, + rateLimitSeconds: 4, + }, + task: "test task", + startTask: false, + rateLimitClock: clock, + }) + vi.spyOn(getTaskTestAccess(cline), "getSystemPrompt").mockResolvedValue("mock system prompt") + + const mockDelay = vi.fn().mockResolvedValue(undefined) + vi.spyOn(await import("delay"), "default").mockImplementation(mockDelay) + + const mockError = new Error("API Error") + const mockFailedStream = { + // eslint-disable-next-line require-yield + async *[Symbol.asyncIterator]() { + throw mockError + }, + async next() { + throw mockError + }, + async return() { + return { done: true, value: undefined } + }, + async throw(e: any) { + throw e + }, + async [Symbol.asyncDispose]() {}, + } as AsyncGenerator + + const mockSuccessStream = { + async *[Symbol.asyncIterator]() { + yield { type: "text", text: "Success" } + }, + async next() { + return { done: true, value: { type: "text", text: "Success" } } + }, + async return() { + return { done: true, value: undefined } + }, + async throw(e: any) { + throw e + }, + async [Symbol.asyncDispose]() {}, + } as AsyncGenerator + + let firstAttempt = true + vi.spyOn(cline.api, "createMessage").mockImplementation(() => { + if (firstAttempt) { + firstAttempt = false + return mockFailedStream + } + return mockSuccessStream + }) + const providerState = await mockProvider.getState() + vi.spyOn(mockProvider, "getState").mockResolvedValue({ + ...providerState, + apiConfiguration: { + ...mockApiConfig, + rateLimitSeconds: 10, + }, + autoApprovalEnabled: true, + requestDelaySeconds: 1, + }) + + const iterator = cline.attemptApiRequest(0) + await iterator.next() + + expect(mockDelay).toHaveBeenCalledTimes(4) + }) + it("should not apply retry delay twice", async () => { const cline = new Task({ provider: mockProvider, diff --git a/src/core/task/__tests__/delegation-concurrent.spec.ts b/src/core/task/__tests__/delegation-concurrent.spec.ts new file mode 100644 index 0000000000..b0d8b7d851 --- /dev/null +++ b/src/core/task/__tests__/delegation-concurrent.spec.ts @@ -0,0 +1,457 @@ +// npx vitest run src/core/task/__tests__/delegation-concurrent.spec.ts + +import * as vscode from "vscode" +import { describe, it, expect, vi, beforeEach } from "vitest" +import type { HistoryItem } from "@roo-code/types" + +import { ClineProvider } from "../../webview/ClineProvider" +import { TaskScheduler } from "../TaskScheduler" +import { TaskRegistry } from "../TaskRegistry" +import { type Task } from "../Task" +import { makeProviderStub } from "../../../__tests__/helpers/provider-stub" + +function makeParent(overrides: Record = {}): Task { + return { + taskId: "parent-1", + clineMessages: [] as unknown[], + flushPendingToolResultsToHistory: vi.fn().mockResolvedValue(true), + retrySaveApiConversationHistory: vi.fn().mockResolvedValue(true), + abort: false, + abandoned: false, + ...overrides, + } as unknown as Task +} + +function makeChild(overrides: Record = {}): Task { + return { + taskId: "child-1", + clineMessages: [] as unknown[], + abort: false, + abandoned: false, + run: vi.fn().mockResolvedValue(undefined), + ...overrides, + } as unknown as Task +} + +/** + * Real createTask() pushes the child onto taskRegistry (which also focuses + * it) before returning. Mirror that here so the fan-out branch has a real + * registry entry to operate on, same as production. + */ +function makeCreateTaskMock(provider: ClineProvider, child: Task) { + return vi.fn().mockImplementation(async () => { + ;(provider as unknown as { taskRegistry: { push: (t: Task) => void } }).taskRegistry.push(child) + return child + }) +} + +function baseStubFields(parent: Task) { + const atomicReadAndUpdate = vi.fn(async (_id: string, updater: (h: HistoryItem) => HistoryItem) => + updater({ id: "parent-1", status: "active", childIds: [] } as unknown as HistoryItem), + ) + return { + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getCurrentTask: vi.fn(() => parent), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + taskHistoryStore: { + get: vi.fn(() => undefined), + atomicReadAndUpdate, + }, + isViewLaunched: false, + emit: vi.fn(), + deleteTaskWithId: vi.fn().mockResolvedValue(undefined), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: { id: "parent-1" } }), + createTaskWithHistoryItem: vi.fn().mockResolvedValue(parent), + } +} + +function getRunningTasks(provider: ClineProvider): Task[] { + return ClineProvider.prototype.getRunningTasks.call(provider) +} + +function callDelegate(provider: ClineProvider) { + return ( + ClineProvider.prototype as unknown as { + delegateParentAndOpenChild: (params: { + parentTaskId: string + message: string + initialTodos: never[] + mode: string + }) => Promise + } + ).delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "do work", + initialTodos: [], + mode: "code", + }) +} + +describe("delegateParentAndOpenChild — fan-out (Story 3.2b)", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("maxConcurrency=1: removeClineFromStack is still called — existing behavior fully preserved", async () => { + const parent = makeParent() + const child = makeChild() + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const createTask = vi.fn().mockResolvedValue(child) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack, + createTask, + }) + + await callDelegate(provider) + + expect(removeClineFromStack).toHaveBeenCalledTimes(1) + }) + + it("maxConcurrency=2 with a free permit: parent stays active, fan-out path is taken", async () => { + const parent = makeParent() + const child = makeChild() + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(2), + removeClineFromStack, + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + + // Parent must not be suspended. + expect(removeClineFromStack).not.toHaveBeenCalled() + // UI focus moves to the child, parent remains in the registry. + const registry = (provider as unknown as { taskRegistry: TaskRegistry }).taskRegistry + expect(registry.current?.taskId).toBe("child-1") + expect(registry.getById("parent-1")).toBeDefined() + }) + + it("maxConcurrency=2 but no free permit: falls back to the suspending (maxConcurrency=1) path", async () => { + const parent = makeParent() + const child = makeChild() + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const createTask = vi.fn().mockResolvedValue(child) + + const scheduler = new TaskScheduler(2) + // Occupy both permits so none are free for fan-out. Don't await — + // the occupying "run" functions never resolve on purpose. + void scheduler.schedule(makeParent({ taskId: "occupant-1" }), () => new Promise(() => {})) + void scheduler.schedule(makeParent({ taskId: "occupant-2" }), () => new Promise(() => {})) + // Poll the deterministic signal instead of assuming a fixed microtask + // depth for sem.acquire() to settle. + for (let i = 0; i < 10 && scheduler.available > 0; i++) { + await Promise.resolve() + } + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack, + createTask, + }) + + await callDelegate(provider) + + expect(removeClineFromStack).toHaveBeenCalledTimes(1) + }) + + it("createTask() throws in fan-out: the reserved permit is released, not leaked", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const scheduler = new TaskScheduler(2) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + }) + + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + // Permit must have been released, not leaked — a subsequent reservation + // attempt must succeed immediately. + expect(scheduler.available).toBe(2) + const release = await scheduler.tryReserve() + expect(release).toBeDefined() + }) + + it("handleModeSwitch() throws in fan-out: delegation aborts and releases the reserved permit before child creation", async () => { + const parent = makeParent() + const modeSwitchError = new Error("Provider profile mutation timed out") + const scheduler = new TaskScheduler(2) + const createTask = vi.fn() + + const provider = makeProviderStub({ + ...baseStubFields(parent), + handleModeSwitch: vi.fn().mockRejectedValue(modeSwitchError), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask, + }) + + await expect(callDelegate(provider)).rejects.toThrow(modeSwitchError) + + expect(createTask).not.toHaveBeenCalled() + expect(scheduler.available).toBe(2) + }) + + it("createTask() throws in the non-fan-out path: the evicted parent is restored", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const createTaskWithHistoryItem = vi.fn().mockResolvedValue(parent) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + createTaskWithHistoryItem, + }) + + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + expect(createTaskWithHistoryItem).toHaveBeenCalledWith({ id: "parent-1" }) + }) + + it("createTask() throws in the non-fan-out path: parent restore retries once after a restore failure", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const restoreError = new Error("restore failed once") + const createTaskWithHistoryItem = vi.fn().mockRejectedValueOnce(restoreError).mockResolvedValueOnce(parent) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + createTaskWithHistoryItem, + }) + + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + expect(createTaskWithHistoryItem).toHaveBeenCalledTimes(2) + expect(createTaskWithHistoryItem).toHaveBeenNthCalledWith(1, { id: "parent-1" }) + expect(createTaskWithHistoryItem).toHaveBeenNthCalledWith(2, { id: "parent-1" }) + }) + + it("createTask() throws in the non-fan-out path: parent restore reports an error after retry exhaustion", async () => { + const parent = makeParent() + const createTaskError = new Error("createTask boom") + const createTaskWithHistoryItem = vi.fn().mockRejectedValue(new Error("restore keeps failing")) + const showErrorMessage = vi.spyOn(vscode.window, "showErrorMessage").mockResolvedValue(undefined) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockRejectedValue(createTaskError), + createTaskWithHistoryItem, + }) + + try { + await expect(callDelegate(provider)).rejects.toThrow(createTaskError) + + expect(createTaskWithHistoryItem).toHaveBeenCalledTimes(2) + expect(showErrorMessage).toHaveBeenCalledWith( + "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", + ) + } finally { + showErrorMessage.mockRestore() + } + }) + + it("fan-out path: both parent and child are tracked in the registry with no shared clineMessages reference", async () => { + const parent = makeParent({ clineMessages: [{ text: "parent msg" }] }) + const child = makeChild({ clineMessages: [{ text: "child msg" }] }) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(2), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + + const running = getRunningTasks(provider).map((t) => t.taskId) + expect(running).toContain("parent-1") + expect(running).toContain("child-1") + expect(parent.clineMessages).not.toBe(child.clineMessages) + }) + + it("fan-out path: the child's run() actually starts concurrently with the parent, not just the registry bookkeeping", async () => { + const parent = makeParent() + let parentResolve!: () => void + // The parent's own request loop is "in flight" (never resolves during + // this test) — provided as run() the way ClineProvider's real scheduler + // invocation for the parent would use it, to prove the child does not + // wait for the parent to finish. + const parentRun = vi.fn(() => new Promise((res) => (parentResolve = res))) + + let childStarted = false + let childResolve!: () => void + const child = makeChild({ + run: vi.fn().mockImplementation(() => { + childStarted = true + return new Promise((res) => (childResolve = res)) + }), + }) + + const scheduler = new TaskScheduler(2) + // Occupy one permit with the "parent's own request loop" so only one + // permit remains — exactly the scenario fan-out is meant to handle. + void scheduler.schedule(parent, parentRun) + for (let i = 0; i < 10 && scheduler.available > 1; i++) { + await Promise.resolve() + } + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: scheduler, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + // Poll the deterministic signal (the child's run() having actually been + // invoked) instead of assuming a fixed microtask depth. + for (let i = 0; i < 10 && !childStarted; i++) { + await Promise.resolve() + } + + expect(childStarted).toBe(true) + expect(child.run).toHaveBeenCalledTimes(1) + + childResolve() + parentResolve() + }) + + it("fan-out path: persisted parent history status is still 'delegated' (awaiting child), independent of the in-memory registry", async () => { + // Fan-out only changes whether the parent Task instance stays alive in + // TaskRegistry — it does not change what delegateParentAndOpenChild + // persists to TaskHistoryStore. The parent's HistoryItem status becomes + // "delegated" (awaiting the child) in both the fan-out and suspending + // paths; "both recorded as active simultaneously" is not the intended + // semantics here, even though the parent Task keeps running in memory. + const parent = makeParent() + const child = makeChild() + let persistedParent: HistoryItem | undefined + + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent], + taskScheduler: new TaskScheduler(2), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + taskHistoryStore: { + get: vi.fn(() => undefined), + atomicReadAndUpdate: vi.fn(async (_id: string, updater: (h: HistoryItem) => HistoryItem) => { + persistedParent = updater({ + id: "parent-1", + status: "active", + childIds: [], + } as unknown as HistoryItem) + return persistedParent + }), + }, + }) + Object.assign(provider, { createTask: makeCreateTaskMock(provider, child) }) + + await callDelegate(provider) + + expect(persistedParent?.status).toBe("delegated") + expect(persistedParent?.awaitingChildId).toBe("child-1") + // But the parent Task instance itself is still live and running in the registry. + expect(getRunningTasks(provider).map((t) => t.taskId)).toContain("parent-1") + }) + + it("permit is reserved atomically at the fan-out decision — a concurrent delegation cannot steal the last free permit", async () => { + // maxConcurrency=2, one permit already held by an unrelated running task, + // leaving exactly one free — the contested permit. + const scheduler = new TaskScheduler(2) + void scheduler.schedule(makeParent({ taskId: "occupant" }), () => new Promise(() => {})) + await Promise.resolve() + expect(scheduler.available).toBe(1) + + // Two reservation attempts race for the single remaining permit. Because + // tryReserve() has no await before the underlying semaphore decrements + // its count, only one can win even though both observe available === 1 + // beforehand. + const [first, second] = await Promise.all([scheduler.tryReserve(), scheduler.tryReserve()]) + + const wins = [first, second].filter((r) => r !== undefined) + expect(wins).toHaveLength(1) + expect(scheduler.available).toBe(0) + }) + + it("getRunningTasks() exposes all concurrently active tasks", () => { + const parent = makeParent() + const child = makeChild() + const provider = makeProviderStub({ + ...baseStubFields(parent), + tasks: [parent, child], + taskScheduler: new TaskScheduler(2), + }) + + expect(getRunningTasks(provider).map((t) => t.taskId)).toEqual(["parent-1", "child-1"]) + }) +}) + +describe("ClineProvider.setTaskSchedulerMaxConcurrency()", () => { + function setMaxConcurrency(provider: ClineProvider, maxConcurrency: number): void { + ClineProvider.prototype.setTaskSchedulerMaxConcurrency.call(provider, maxConcurrency) + } + + it("replaces the scheduler with one at the new maxConcurrency when no tasks are active", () => { + const cancelQueued = vi.fn() + const provider = makeProviderStub({ + tasks: [], + taskScheduler: { cancelQueued, maxConcurrency: 1 } as unknown as TaskScheduler, + }) + + setMaxConcurrency(provider, 2) + + expect(cancelQueued).toHaveBeenCalledTimes(1) + expect((provider as unknown as { taskScheduler: TaskScheduler }).taskScheduler.maxConcurrency).toBe(2) + }) + + it("throws and leaves the scheduler untouched if a task is active", () => { + const cancelQueued = vi.fn() + const originalScheduler = { cancelQueued, maxConcurrency: 1 } as unknown as TaskScheduler + const provider = makeProviderStub({ + tasks: [makeParent()], + taskScheduler: originalScheduler, + }) + + expect(() => setMaxConcurrency(provider, 2)).toThrow("Cannot change task scheduler concurrency") + expect(cancelQueued).not.toHaveBeenCalled() + expect((provider as unknown as { taskScheduler: TaskScheduler }).taskScheduler).toBe(originalScheduler) + }) + + it("rejects non-positive-integer values", () => { + const provider = makeProviderStub({ + tasks: [], + taskScheduler: new TaskScheduler(1), + }) + + expect(() => setMaxConcurrency(provider, 0)).toThrow("must be a positive integer") + expect(() => setMaxConcurrency(provider, 1.5)).toThrow("must be a positive integer") + expect(() => setMaxConcurrency(provider, -1)).toThrow("must be a positive integer") + }) +}) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 2ee92edebc..65ea9803fa 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -160,6 +160,13 @@ function scheduleTask(scheduler: TaskScheduler, task: Task, source: string): voi .catch((error) => console.error(`[${source}] taskScheduler.schedule failed:`, error)) } +/** Run `task` using a permit already reserved via `TaskScheduler.tryReserve()`. */ +function runReservedTask(scheduler: TaskScheduler, release: () => void, task: Task, source: string): void { + void scheduler + .runWithReservation(release, task, () => task.run()) + .catch((error) => console.error(`[${source}] taskScheduler.runWithReservation failed:`, error)) +} + export class ClineProvider extends EventEmitter implements vscode.WebviewViewProvider, TelemetryPropertiesProvider, TaskProviderLike @@ -196,11 +203,37 @@ export class ClineProvider private globalStateWriteThroughTimer: ReturnType | 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 providerProfileMutationQueue = Promise.resolve() private runDelegationTransition(parentTaskId: string, fn: () => Promise): Promise { this.delegationTransitionLocks ??= new Map() return runDelegationTransition(this.delegationTransitionLocks, parentTaskId, fn) } + + private enqueueProviderProfileMutation(fn: () => Promise): Promise { + const run = this.providerProfileMutationQueue.then(fn, fn) + const callerResult = this.withProviderProfileMutationTimeout(run) + this.providerProfileMutationQueue = run.then( + () => undefined, + () => undefined, + ) + return callerResult + } + + private withProviderProfileMutationTimeout(operation: Promise): Promise { + let timeoutId: ReturnType | undefined + const timeout = new Promise((_, reject) => { + timeoutId = setTimeout(() => { + reject(new Error("Provider profile mutation timed out")) + }, ClineProvider.PENDING_OPERATION_TIMEOUT_MS) + }) + + return Promise.race([operation, timeout]).finally(() => { + if (timeoutId) { + clearTimeout(timeoutId) + } + }) + } private readonly pendingEditOperations: PendingEditOperationStore private cloudOrganizationsCache: CloudOrganizationMembership[] | null = null @@ -505,13 +538,17 @@ export class ClineProvider // Removes and destroys the top Cline instance (the current finished task), // activating the previous one (resuming the parent task). - async removeClineFromStack() { + // + // Pass `taskId` to evict a specific task regardless of focus (e.g. rollback + // cleanup of a just-created child after focus has already moved elsewhere). + // Defaults to the current task, preserving prior behavior. + async removeClineFromStack(taskId?: string) { if (this.taskRegistry.length === 0) { return } - // Remove the focused Cline instance from the stack. - let task = this.taskRegistry.current + const targetId = taskId ?? this.taskRegistry.current?.taskId + let task = targetId ? this.taskRegistry.getById(targetId) : undefined if (task) { task = this.taskRegistry.remove(task.taskId) } @@ -639,6 +676,17 @@ export class ClineProvider return this.taskRegistry.taskIds } + public setTaskSchedulerMaxConcurrency(maxConcurrency: number): void { + if (!Number.isInteger(maxConcurrency) || maxConcurrency < 1) { + throw new Error(`maxConcurrency must be a positive integer, got ${maxConcurrency}`) + } + if (this.taskRegistry.length > 0) { + throw new Error("Cannot change task scheduler concurrency while tasks are active") + } + this.taskScheduler.cancelQueued() + this.taskScheduler = new TaskScheduler(maxConcurrency) + } + // Pending Edit Operations Management /** @@ -1506,9 +1554,18 @@ export class ClineProvider /** * Handle switching to a new mode, including updating the associated API configuration * @param newMode The mode to switch to + * @param targetTask The task whose in-memory `_taskMode` should be updated to `newMode`. + * Defaults to the current task. Pass `null` to skip the per-task mutation and only + * apply the global mode/API-config side effects — used by delegation fan-out, where + * the "current" task is still the parent (which must keep its own mode) even though + * `newMode` is the child's requested mode. */ - public async handleModeSwitch(newMode: Mode) { - const task = this.getCurrentTask() + public async handleModeSwitch(newMode: Mode, targetTask: Task | null | undefined = this.getCurrentTask()) { + return this.enqueueProviderProfileMutation(() => this.handleModeSwitchUnlocked(newMode, targetTask)) + } + + private async handleModeSwitchUnlocked(newMode: Mode, targetTask: Task | null | undefined): Promise { + const task = targetTask if (task) { TelemetryService.instance.captureModeSwitch(task.taskId, newMode) @@ -1545,7 +1602,14 @@ export class ClineProvider // If workspace lock is on, keep the current API config — don't load mode-specific config const lockApiConfigAcrossModes = this.context.workspaceState.get("lockApiConfigAcrossModes", false) if (lockApiConfigAcrossModes) { - await this.postStateToWebview() + // Skip in fan-out (targetTask === null): postStateToWebview() reads + // getCurrentTask(), which is still the live parent here (the child + // doesn't exist yet) — pushing now would flash the parent's + // clineMessages/taskId to the webview as "current". The child's own + // registration moments later triggers the correct push. + if (targetTask !== null) { + await this.postStateToWebview() + } return } @@ -1571,7 +1635,18 @@ export class ClineProvider const hasActualSettings = !!fullProfile.apiProvider if (hasActualSettings) { - await this.activateProviderProfile({ name: profile.name }) + // targetTask === null means this call came from delegation fan-out + // (see handleModeSwitch's targetTask doc) — the parent is still + // "current" here and must not have its live API handler rebuilt + // to the child's mode's saved profile. + if (targetTask === null) { + await this.activateProviderProfileUnlocked( + { name: profile.name }, + { skipCurrentTaskRebuild: true }, + ) + } else { + await this.activateProviderProfileUnlocked({ name: profile.name }) + } } else { // The task will continue with the current/default configuration. } @@ -1591,7 +1666,9 @@ export class ClineProvider } } - await this.postStateToWebview() + if (targetTask !== null) { + await this.postStateToWebview() + } } // Provider Profile Management @@ -1607,8 +1684,9 @@ export class ClineProvider */ private updateTaskApiHandlerIfNeeded( providerSettings: ProviderSettings, - options: { forceRebuild?: boolean } = {}, + options: { forceRebuild?: boolean; skipCurrentTaskRebuild?: boolean } = {}, ): void { + if (options.skipCurrentTaskRebuild) return const task = this.getCurrentTask() if (!task) return @@ -1724,7 +1802,13 @@ export class ClineProvider await this.postStateToWebview() } - private async persistStickyProviderProfileToCurrentTask(apiConfigName: string): Promise { + private async persistStickyProviderProfileToCurrentTask( + apiConfigName: string, + options: { skipCurrentTaskRebuild?: boolean } = {}, + ): Promise { + if (options.skipCurrentTaskRebuild) { + return + } const task = this.getCurrentTask() if (!task) { return @@ -1754,12 +1838,35 @@ export class ClineProvider async activateProviderProfile( args: { name: string } | { id: string }, - options?: { persistModeConfig?: boolean; persistTaskHistory?: boolean }, + options?: { + persistModeConfig?: boolean + persistTaskHistory?: boolean + /** + * Skip rebuilding/mutating the current task's API handler and sticky + * profile. Used by delegation fan-out mode switches, where the + * "current" task is still the live parent (the child doesn't exist + * yet) and must not have its API configuration swapped to the + * child's mode's saved profile. + */ + skipCurrentTaskRebuild?: boolean + }, ) { + return this.enqueueProviderProfileMutation(() => this.activateProviderProfileUnlocked(args, options)) + } + + private async activateProviderProfileUnlocked( + args: { name: string } | { id: string }, + options?: { + persistModeConfig?: boolean + persistTaskHistory?: boolean + skipCurrentTaskRebuild?: boolean + }, + ): Promise { const { name, id, ...providerSettings } = await this.providerSettingsManager.activateProfile(args) const persistModeConfig = options?.persistModeConfig ?? true const persistTaskHistory = options?.persistTaskHistory ?? true + const skipCurrentTaskRebuild = options?.skipCurrentTaskRebuild ?? false // See `upsertProviderProfile` for a description of what this is doing. await Promise.all([ @@ -1775,17 +1882,27 @@ export class ClineProvider } // Change the provider for the current task. - this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true }) + this.updateTaskApiHandlerIfNeeded(providerSettings, { forceRebuild: true, skipCurrentTaskRebuild }) // Update the current task's sticky provider profile, unless this activation is // being used purely as a non-persisting restoration (e.g., reopening a task from history). if (persistTaskHistory) { - await this.persistStickyProviderProfileToCurrentTask(name) + await this.persistStickyProviderProfileToCurrentTask(name, { skipCurrentTaskRebuild }) } - await this.postStateToWebview() + // Same fan-out hazard as handleModeSwitch: postStateToWebview() reads + // getCurrentTask(), which is still the live parent while skipCurrentTaskRebuild + // is set, so pushing here would flash the parent's clineMessages/taskId to + // the webview as "current" a moment before the child registers itself. + if (!skipCurrentTaskRebuild) { + await this.postStateToWebview() + } - if (providerSettings.apiProvider) { + // In fan-out, ProviderProfileChanged would be received by the still-running + // parent task and make it reload the child's provider settings through its + // own listener. The child is created from the updated provider state, so no + // live-task event is needed for that path. + if (providerSettings.apiProvider && !skipCurrentTaskRebuild) { this.emit(RooCodeEventName.ProviderProfileChanged, { name, provider: providerSettings.apiProvider }) } } @@ -3049,6 +3166,11 @@ export class ClineProvider return this.taskRegistry.current } + /** All tasks concurrently active in the registry (parent(s) plus any fanned-out children). */ + public getRunningTasks(): Task[] { + return this.taskRegistry.getRunning() + } + private logWebviewHiddenDiagnostics(): void { const task = this.getCurrentTask() if (!task || task.abort || task.abandoned) { @@ -3514,13 +3636,56 @@ export class ClineProvider return this.currentWorkspacePath || getWorkspacePath() } + /** + * Undo the "parent kept running" (fan-out) or "parent evicted" (non-fan-out) + * side effect from step 3 of `delegateParentAndOpenChild`, for failures that + * happen before a child exists to attach lineage to. Shared by the + * `createTask()` failure path and the metadata-persistence failure path. + */ + private async restoreParentOrReleasePermit( + parentTaskId: string, + fanOut: boolean, + childReservedRelease: (() => void) | undefined, + ): Promise { + if (!fanOut) { + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + } catch (firstRollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback, retrying once: ${ + (firstRollbackError as Error)?.message ?? String(firstRollbackError) + }`, + ) + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + } catch (rollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback retry: ${ + (rollbackError as Error)?.message ?? String(rollbackError) + }`, + ) + vscode.window.showErrorMessage( + "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", + ) + } + } + } else { + // The child never reached step 6, so the reserved permit must be + // released here or it leaks for the lifetime of the scheduler. + childReservedRelease?.() + } + } + /** * Delegate parent task and open child task. * - * - Enforce single-open invariant + * - Enforce single-open invariant, unless fan-out (maxConcurrency > 1 with a + * free permit) keeps the parent running alongside the child * - Persist parent delegation metadata * - Emit TaskDelegated (task-level; API forwards to provider/bridge) - * - Create child as sole active and switch mode to child's mode + * - Create and focus child, preserving parent reference for lineage, and switch mode to child's mode */ public async delegateParentAndOpenChild(params: { parentTaskId: string @@ -3576,11 +3741,28 @@ export class ClineProvider ) } - // 3) Enforce single-open invariant by closing/disposing the parent first - // This ensures we never have >1 tasks open at any time during delegation. + // 3) Enforce single-open invariant by closing/disposing the parent first — + // unless fan-out is enabled (maxConcurrency > 1) AND a permit can be + // reserved for the child right now, in which case the parent stays + // active on the registry and runs concurrently with the child. This + // path is only reachable via explicit TaskScheduler configuration; at + // the default maxConcurrency=1 it can never be taken. + // + // The permit is reserved here — not just checked via `available > 0` — + // because several awaits follow before the child actually starts + // running (mode switch, task creation, history persistence). Checking + // availability without reserving would leave a window where another + // delegation could consume the last permit, leaving the parent live + // but the child queued rather than concurrent. childReservedRelease is + // handed to the scheduler in step 6 instead of it acquiring its own. // Await abort completion to ensure clean disposal and prevent unhandled rejections. + const childReservedRelease = + this.taskScheduler.maxConcurrency > 1 ? await this.taskScheduler.tryReserve() : undefined + const fanOut = childReservedRelease !== undefined try { - await this.removeClineFromStack() + if (!fanOut) { + await this.removeClineFromStack() + } } catch (error) { this.log( `[delegateParentAndOpenChild] Error during parent disposal (non-fatal): ${ @@ -3594,17 +3776,32 @@ export class ClineProvider // This ensures the child's system prompt and configuration are based on the correct mode. // The mode switch must happen before createTask() because the Task constructor // initializes its mode from provider.getState() during initializeTaskMode(). + // In fan-out, the parent is still `getCurrentTask()` at this point (it hasn't been + // removed and the child doesn't exist yet) — pass `null` so handleModeSwitch applies + // the global mode/API-config side effects for the child's benefit without stomping + // the still-running parent's own `_taskMode`. + const requestedMode = mode as Mode try { - await this.handleModeSwitch(mode as any) + if (fanOut) { + await this.handleModeSwitch(requestedMode, null) + } else { + await this.handleModeSwitch(requestedMode) + } } catch (e) { this.log( `[delegateParentAndOpenChild] handleModeSwitch failed for mode '${mode}': ${ (e as Error)?.message ?? String(e) }`, ) + if (fanOut) { + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) + throw e + } } - // 4) Create child as sole active (parent reference preserved for lineage) + // 4) Create and focus child, preserving parent reference for lineage. + // In the non-fan-out path the parent was already removed above, so the + // child is the sole active task; in fan-out both remain active. // Pass initialStatus: "active" to ensure the child task's historyItem is created // with status from the start, avoiding race conditions where the task might // call attempt_completion before status is persisted separately. @@ -3615,11 +3812,29 @@ export class ClineProvider // Without this, the child's fire-and-forget startTask() races with step 5, // and the last writer to globalState overwrites the other's changes— // causing the parent's delegation fields to be lost. - const child = await this.createTask(message, undefined, parent as any, { - initialTodos, - initialStatus: "active", - startTask: false, - }) + let child: Task + try { + child = await this.createTask(message, undefined, parent as any, { + initialTodos, + initialStatus: "active", + startTask: false, + }) + } catch (err) { + this.log( + `[delegateParentAndOpenChild] createTask failed for parent ${parentTaskId}: ${ + (err as Error)?.message ?? String(err) + }`, + ) + // No child was created, so there is no lineage to unwind — just undo + // step 3's parent-eviction (or release the reserved permit in fan-out). + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) + throw err + } + + // createTask() -> addClineToStack() -> taskRegistry.push() already focuses + // the child. In the fan-out case the parent remains in the registry + // alongside it instead of being evicted, so both are now tracked with the + // child focused. // 5) Persist parent delegation metadata BEFORE the child starts writing. // atomicReadAndUpdate reads from the in-memory cache and writes back within a @@ -3683,10 +3898,11 @@ export class ClineProvider }`, ) try { - // Only pop the stack if the child we just created is still on top. - // A concurrent delegation could have pushed another child since we created ours. - if (this.getCurrentTask()?.taskId === child.taskId) { - await this.removeClineFromStack() + // Evict the child by id regardless of current focus — in fan-out (or if a + // concurrent delegation shifted focus), the child we just created may no + // longer be `current`, but it must still be removed from the registry. + if (this.taskRegistry.getById(child.taskId)) { + await this.removeClineFromStack(child.taskId) } } catch (cleanupError) { this.log( @@ -3704,21 +3920,22 @@ export class ClineProvider }`, ) } - try { - const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - await this.createTaskWithHistoryItem(parentHistory) - } catch (rollbackError) { - this.log( - `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback: ${ - (rollbackError as Error)?.message ?? String(rollbackError) - }`, - ) - } + // In the fan-out case the parent was never removed from the registry + // (it kept running throughout), so it must not be re-created here — + // doing so would push a duplicate parent Task instance. If the child was + // still focused, removeClineFromStack() above already re-focused the + // registry onto the parent (TaskRegistry.remove only reassigns focus + // when the removed task was current). + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) throw err } // 6) Start the child task now that parent metadata is safely persisted. - scheduleTask(this.taskScheduler, child, "delegateParentAndOpenChild") + if (childReservedRelease) { + runReservedTask(this.taskScheduler, childReservedRelease, child, "delegateParentAndOpenChild") + } else { + scheduleTask(this.taskScheduler, child, "delegateParentAndOpenChild") + } // 7) Emit TaskDelegated (provider-level) try { diff --git a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts index 3c777d0fc0..fa02ff6b5d 100644 --- a/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts @@ -3,9 +3,10 @@ import * as vscode from "vscode" import { TelemetryService } from "@roo-code/telemetry" -import { getModelId } from "@roo-code/types" +import { getModelId, RooCodeEventName } from "@roo-code/types" import { ContextProxy } from "../../config/ContextProxy" +import type { Mode } from "../../../shared/modes" import { Task, TaskOptions } from "../../task/Task" import { ClineProvider } from "../ClineProvider" @@ -235,6 +236,7 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { { name: "test-config", id: "test-id", apiProvider: "openrouter", modelId: "openai/gpt-4" }, ]), setModeConfig: vi.fn(), + getModeConfigId: vi.fn().mockResolvedValue(undefined), activateProfile: vi.fn().mockResolvedValue({ name: "test-config", id: "test-id", @@ -410,6 +412,158 @@ describe("ClineProvider - API Handler Rebuild Guard", () => { }) describe("activateProviderProfile", () => { + test("serializes provider profile mutations without interleaving", async () => { + const events: string[] = [] + let resolveFirst!: () => void + + provider["providerSettingsManager"].activateProfile = vi + .fn() + .mockImplementationOnce(async () => { + events.push("first:start") + await new Promise((resolve) => { + resolveFirst = resolve + }) + events.push("first:end") + return { + name: "first-profile", + id: "first-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + } + }) + .mockImplementationOnce(async () => { + events.push("second:start") + return { + name: "second-profile", + id: "second-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + } + }) + + const first = provider.activateProviderProfile({ name: "first-profile" }) + const second = provider.activateProviderProfile({ name: "second-profile" }) + + await Promise.resolve() + expect(events).toEqual(["first:start"]) + + resolveFirst() + await first + await second + + expect(events).toEqual(["first:start", "first:end", "second:start"]) + }) + + test("provider profile mutation rejection does not poison later queued mutations", async () => { + const firstError = new Error("first profile failed") + + provider["providerSettingsManager"].activateProfile = vi + .fn() + .mockRejectedValueOnce(firstError) + .mockResolvedValueOnce({ + name: "second-profile", + id: "second-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + }) + + await expect(provider.activateProviderProfile({ name: "first-profile" })).rejects.toThrow(firstError) + await expect(provider.activateProviderProfile({ name: "second-profile" })).resolves.toBeUndefined() + }) + + test("provider profile mutation timeout does not advance the queue until the timed-out mutation settles", async () => { + vi.useFakeTimers() + try { + const events: string[] = [] + let resolveFirst!: () => void + provider["providerSettingsManager"].activateProfile = vi + .fn() + .mockImplementationOnce(async () => { + events.push("first:start") + await new Promise((resolve) => { + resolveFirst = resolve + }) + events.push("first:end") + return { + name: "stuck-profile", + id: "stuck-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + } + }) + .mockImplementationOnce(async () => { + events.push("second:start") + return { + name: "second-profile", + id: "second-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + } + }) + + const first = provider.activateProviderProfile({ name: "stuck-profile" }) + const firstResult = first.catch((error: unknown) => error) + await Promise.resolve() + expect(events).toEqual(["first:start"]) + + await vi.advanceTimersByTimeAsync(30_000) + + expect(await firstResult).toEqual(new Error("Provider profile mutation timed out")) + const second = provider.activateProviderProfile({ name: "second-profile" }) + await Promise.resolve() + expect(events).toEqual(["first:start"]) + + resolveFirst() + await second + expect(events).toEqual(["first:start", "first:end", "second:start"]) + } finally { + vi.useRealTimers() + } + }) + + test("skipCurrentTaskRebuild does not rebuild current task or emit profile-change event", async () => { + const mockTask = new Task({ + ...defaultTaskOptions, + apiConfiguration: { + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + }, + }) + await provider.addClineToStack(mockTask) + + provider["providerSettingsManager"].activateProfile = vi.fn().mockResolvedValue({ + name: "ask-profile", + id: "ask-id", + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4.1-mini", + }) + const emitSpy = vi.spyOn(provider, "emit") + + await provider.activateProviderProfile({ name: "ask-profile" }, { skipCurrentTaskRebuild: true }) + + expect(mockTask.updateApiConfiguration).not.toHaveBeenCalled() + expect(emitSpy).not.toHaveBeenCalledWith( + RooCodeEventName.ProviderProfileChanged, + expect.objectContaining({ name: "ask-profile" }), + ) + }) + + test("fan-out mode switch does not post the current parent state when no saved profile is activated", async () => { + const mockTask = new Task({ + ...defaultTaskOptions, + apiConfiguration: { + apiProvider: "openrouter", + openRouterModelId: "openai/gpt-4", + }, + }) + await provider.addClineToStack(mockTask) + const postStateSpy = vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + + await provider.handleModeSwitch("ask" as Mode, null) + + expect(postStateSpy).not.toHaveBeenCalled() + }) + test("calls updateApiConfiguration when provider/model unchanged but settings differ (explicit profile switch)", async () => { const mockTask = new Task({ ...defaultTaskOptions, diff --git a/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts b/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts index 49d181ed36..07e6b82a64 100644 --- a/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts @@ -374,14 +374,15 @@ describe("ClineProvider - Lock API Config Across Modes", () => { apiProvider: "anthropic", }) - const activateProviderProfileSpy = vi - .spyOn(provider, "activateProviderProfile") - .mockResolvedValue(undefined) + const activateProfileSpy = vi.spyOn(provider.providerSettingsManager, "activateProfile").mockResolvedValue({ + name: "architect-profile", + apiProvider: "anthropic", + }) await provider.handleModeSwitch("architect") expect(getModeConfigIdSpy).toHaveBeenCalledWith("architect") - expect(activateProviderProfileSpy).toHaveBeenCalledWith({ name: "architect-profile" }) + expect(activateProfileSpy).toHaveBeenCalledWith({ name: "architect-profile" }) }) }) }) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 608e190d04..8e4d671079 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -856,7 +856,7 @@ }, "core/task/__tests__/Task.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 31 + "count": 29 } }, "core/task/__tests__/Task.sticky-profile-race.spec.ts": { @@ -1076,7 +1076,7 @@ }, "core/webview/ClineProvider.ts": { "@typescript-eslint/no-explicit-any": { - "count": 12 + "count": 11 } }, "core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts": { diff --git a/src/extension/api.ts b/src/extension/api.ts index 2d0d5a6975..af68d24b5a 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -23,6 +23,7 @@ import { import { IpcServer } from "@roo-code/ipc" import { Package } from "../shared/package" +import type { Mode } from "../shared/modes" import { ClineProvider } from "../core/webview/ClineProvider" import { Terminal } from "../integrations/terminal/Terminal" import { TerminalRegistry } from "../integrations/terminal/TerminalRegistry" @@ -253,6 +254,10 @@ export class API extends EventEmitter implements RooCodeAPI { return this.sidebarProvider.getCurrentTaskStack() } + public setTaskSchedulerMaxConcurrency(maxConcurrency: number): void { + this.sidebarProvider.setTaskSchedulerMaxConcurrency(maxConcurrency) + } + public async clearCurrentTask(_lastMessage?: string) { // Legacy finishSubTask removed; clear current by closing active task instance. await this.sidebarProvider.evictCurrentTask() @@ -505,6 +510,13 @@ export class API extends EventEmitter implements RooCodeAPI { public async setConfiguration(values: RooCodeSettings) { await this.sidebarProvider.contextProxy.setValues(values) await this.sidebarProvider.providerSettingsManager.saveConfig(values.currentApiConfigName || "default", values) + if (values.modeApiConfigs) { + await Promise.all( + Object.entries(values.modeApiConfigs).map(([mode, configId]) => + this.sidebarProvider.providerSettingsManager.setModeConfig(mode as Mode, configId), + ), + ) + } await this.sidebarProvider.postStateToWebview() } diff --git a/src/utils/TaskSemaphore.ts b/src/utils/TaskSemaphore.ts index 15db11c2e1..188d4a98ba 100644 --- a/src/utils/TaskSemaphore.ts +++ b/src/utils/TaskSemaphore.ts @@ -36,6 +36,26 @@ export class TaskSemaphore { return this._waiting } + /** + * Reserve a permit only if one is immediately free; never queues. + * + * `async-mutex`'s `Semaphore.acquire()` resolves its permit synchronously + * (inside the Promise executor, before `acquire()` returns) whenever a + * permit is free at call time — the queue is only used when none is + * free. So `isLocked()` (== false means a permit is free) followed + * immediately by `acquire()`, with no `await` between them, cannot race: + * nothing else runs on the event loop in that gap. If `isLocked()` was + * false, the following `acquire()` is guaranteed not to queue. + * + * Returns `undefined` without side effects if no permit was free. + */ + async tryAcquire(): Promise<(() => void) | undefined> { + if (this.sem.isLocked()) { + return undefined + } + return this.acquire() + } + async acquire(): Promise<() => void> { // Only count as waiting if the permit won't be granted immediately. const willQueue = this.sem.isLocked()