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 30d3852b0a..f9cb49279c 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".` @@ -38,6 +52,7 @@ export const SUBTASK_API_HANG_PARENT_RESULT = "API hang parent resumed" // Correctness depends on no flat `latency` (fixture or LLMock default) being set on that // fixture — a flat latency would apply to every chunk after the first, not just the ttft. export const SUBTASK_API_HANG_RESPONSE_LATENCY_MS = 15_000 +export const SUBTASK_FANOUT_CHILD_DELAY_MS = 15_000 // Abandon-subtask scenario (#559) — separate markers to avoid sequenceIndex collisions with the // interrupted-child-resumes tests above, which exhaust the sequence count for INTERRUPT markers. @@ -141,6 +156,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]), + }, + streamingProfile: { ttft: SUBTASK_FANOUT_CHILD_DELAY_MS }, + 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]), + }, + streamingProfile: { ttft: SUBTASK_FANOUT_CHILD_DELAY_MS }, + 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 02d3dfe487..ee8336137e 100644 --- a/apps/vscode-e2e/src/suite/subtasks.test.ts +++ b/apps/vscode-e2e/src/suite/subtasks.test.ts @@ -20,6 +20,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, @@ -36,6 +43,7 @@ type AimockMessageContent = string | Array<{ type?: string; text?: string }> type AimockJournalEntry = { timestamp?: number body?: { + model?: string messages?: Array<{ role?: string content?: AimockMessageContent @@ -51,6 +59,12 @@ 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 fetchAimockJournal = async () => { const aimockUrl = process.env.AIMOCK_URL assert.ok(aimockUrl, "AIMOCK_URL must be set for aimock journal assertions") @@ -59,6 +73,8 @@ const fetchAimockJournal = async () => { return (await response.json()) as AimockJournalEntry[] } +const readAimockJournal = fetchAimockJournal + const findAimockRequest = (entries: AimockJournalEntry[], expectedText: string, excludeText?: string) => entries.find((entry) => { const messages = entry.body?.messages @@ -86,6 +102,29 @@ const waitForAimockRequestContaining = async ( return matchedAt } +const assertAimockRequest = (entries: AimockJournalEntry[], matches: (entry: AimockJournalEntry) => boolean) => { + if (entries.some(matches)) return + + const summary = entries.map((entry) => ({ + model: entry.body?.model, + userText: requestUserText(entry).slice(0, 180), + })) + assert.fail(`Expected aimock request was not found. Requests: ${JSON.stringify(summary, null, 2)}`) +} + +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) + } +} + // Grace period after the delayed window for aimock to flush the stream's remaining chunks to // the dead socket. 500ms is an empirical margin for that flush plus socket teardown; if this // suite becomes flaky again on slow CI runners, widen this value first. @@ -174,6 +213,206 @@ 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), + ), + ) + + const parentHistory = await api.getTaskHistoryItem(parentTaskId) + assert.strictEqual(parentHistory?.mode, "code", "Parent task mode should remain isolated") + assert.strictEqual( + parentHistory?.apiConfigName, + "subtask-fanout-parent-profile", + "Parent task profile should remain isolated", + ) + const childHistory = await api.getTaskHistoryItem(childTaskId!) + assert.strictEqual(childHistory?.mode, "ask", "Child task should retain its requested mode") + assert.strictEqual( + childHistory?.apiConfigName, + "subtask-fanout-child-profile", + "Child task should retain its resolved profile", + ) + } 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..ca4b62dea7 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -13,12 +13,40 @@ type ProviderStubFields = { runDelegationTransition?: unknown removeClineFromStack?: unknown evictCurrentTask?: unknown + restoreParentOrReleasePermit?: unknown + handleModeSwitchForChild?: unknown + handleModeSwitch?: (newMode: string, targetTask?: unknown) => Promise } 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 + handleModeSwitchForChild: (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) +} + +export function bindHandleModeSwitchForChild(provider: ClineProvider): void { + const s = provider as unknown as ProviderStubFields + s.handleModeSwitchForChild = async (newMode: string, targetTask?: unknown) => { + if (targetTask === undefined) { + await s.handleModeSwitch?.(newMode) + } else { + await s.handleModeSwitch?.(newMode, targetTask) + } + return { apiConfiguration: {}, mode: newMode, apiConfigName: "default" } + } +} + +export function bindDelegationMethods(provider: ClineProvider): void { + bindRestoreParentOrReleasePermit(provider) + bindHandleModeSwitchForChild(provider) } /** @@ -51,5 +79,11 @@ 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) + } + if (!s.handleModeSwitchForChild) { + bindHandleModeSwitchForChild(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..8e5d5aae39 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 { bindDelegationMethods } from "./helpers/provider-stub" const parentHistoryItem: HistoryItem = { id: "parent-1", @@ -43,6 +44,18 @@ const makeParentTask = () => }) as any describe("ClineProvider.delegateParentAndOpenChild()", () => { + it("forwards an explicit target task through the typed child mode-switch binder", async () => { + const handleModeSwitch = vi.fn().mockResolvedValue(undefined) + const provider = { handleModeSwitch } as unknown as ClineProvider + bindDelegationMethods(provider) + const targetTask = makeParentTask() + + const snapshot = await provider.handleModeSwitchForChild("architect", targetTask) + + expect(handleModeSwitch).toHaveBeenCalledWith("architect", targetTask) + expect(snapshot).toMatchObject({ mode: "architect" }) + }) + it("persists parent delegation metadata via atomicReadAndUpdate and emits TaskDelegated", async () => { const providerEmit = vi.fn() const parentTask = makeParentTask() @@ -65,6 +78,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindDelegationMethods(provider) const child = await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { parentTaskId: "parent-1", @@ -80,11 +94,22 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(removeClineFromStack).toHaveBeenCalledTimes(1) // Child task created with startTask: false and initialStatus: "active" - expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, { - initialTodos: [], - initialStatus: "active", - startTask: false, - }) + expect(createTask).toHaveBeenCalledWith( + "Do something", + undefined, + parentTask, + { + initialTodos: [], + initialStatus: "active", + startTask: false, + }, + {}, + { + apiConfiguration: {}, + mode: "code", + apiConfigName: "default", + }, + ) // Delegation metadata written via atomicReadAndUpdate with correct taskId expect(taskHistoryStore.atomicReadAndUpdate).toHaveBeenCalledTimes(1) @@ -132,6 +157,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindDelegationMethods(provider) await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { parentTaskId: "parent-1", @@ -166,6 +192,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindDelegationMethods(provider) await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { parentTaskId: "parent-1", @@ -207,6 +234,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindDelegationMethods(provider) await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { parentTaskId: "parent-1", @@ -254,6 +282,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindDelegationMethods(provider) await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { parentTaskId: "parent-1", @@ -318,6 +347,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { recentTasksCache: undefined, taskHistoryStore, } as unknown as ClineProvider + bindDelegationMethods(provider) await expect( (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { @@ -369,7 +399,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 + bindDelegationMethods(provider) await expect( (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { @@ -381,8 +415,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 f55078b6ff..a60395c154 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -145,6 +145,8 @@ const MAX_CONTEXT_WINDOW_RETRIES = 3 // Maximum retries for context window error export interface TaskOptions extends CreateTaskOptions { provider: ClineProvider apiConfiguration: ProviderSettings + /** Immutable mode/profile values captured by the provider before task construction. */ + startupSnapshot?: TaskStartupSnapshot enableCheckpoints?: boolean checkpointTimeout?: number consecutiveMistakeLimit?: number @@ -165,6 +167,12 @@ export interface TaskOptions extends CreateTaskOptions { diffFuzzyThreshold?: number } +export interface TaskStartupSnapshot { + apiConfiguration: ProviderSettings + mode: string + apiConfigName?: string +} + export class Task extends EventEmitter implements TaskLike { readonly taskId: string readonly rootTaskId?: string @@ -479,6 +487,7 @@ export class Task extends EventEmitter implements TaskLike { initialStatus, rateLimitClock, diffFuzzyThreshold, + startupSnapshot, }: TaskOptions) { super() @@ -527,12 +536,15 @@ export class Task extends EventEmitter implements TaskLike { console.error("Failed to initialize RooIgnoreController:", error) }) - this.apiConfiguration = apiConfiguration + this.apiConfiguration = startupSnapshot?.apiConfiguration ?? apiConfiguration this.api = buildApiHandler(this.apiConfiguration) this.rateLimitClock = rateLimitClock ?? createRateLimitClock() this.autoApprovalHandler = new AutoApprovalHandler() - this.consecutiveMistakeLimit = consecutiveMistakeLimit ?? DEFAULT_CONSECUTIVE_MISTAKE_LIMIT + this.consecutiveMistakeLimit = + startupSnapshot?.apiConfiguration.consecutiveMistakeLimit ?? + consecutiveMistakeLimit ?? + DEFAULT_CONSECUTIVE_MISTAKE_LIMIT this.providerRef = new WeakRef(provider) this.globalStoragePath = provider.context.globalStorageUri.fsPath this.diffViewProvider = new DiffViewProvider(this.cwd, this) @@ -552,6 +564,12 @@ export class Task extends EventEmitter implements TaskLike { this.taskModeReady = Promise.resolve() this.taskApiConfigReady = Promise.resolve() TelemetryService.instance.captureTaskRestarted(this.taskId) + } else if (startupSnapshot) { + this._taskMode = startupSnapshot.mode + this._taskApiConfigName = startupSnapshot.apiConfigName ?? "default" + this.taskModeReady = Promise.resolve() + this.taskApiConfigReady = Promise.resolve() + TelemetryService.instance.captureTaskCreated(this.taskId) } else { // For new tasks, don't set the mode/apiConfigName yet - wait for async initialization. this._taskMode = undefined @@ -4046,7 +4064,7 @@ export class Task extends EventEmitter implements TaskLike { autoCondenseContextPercent = 100, profileThresholds = {}, } = state ?? {} - // Use task-local values, not provider state, to prevent cross-task configuration leaks. + // Use task-local mode/apiConfiguration values, not shared provider state, to prevent cross-task configuration leaks. const mode = await this.getTaskMode() const apiConfiguration = this.apiConfiguration diff --git a/src/core/task/TaskScheduler.ts b/src/core/task/TaskScheduler.ts index 7431fb76ad..a036d933ac 100644 --- a/src/core/task/TaskScheduler.ts +++ b/src/core/task/TaskScheduler.ts @@ -10,8 +10,13 @@ import { type Task } from "./Task" */ export class TaskScheduler { private readonly sem: TaskSemaphore + readonly maxConcurrency: number constructor(maxConcurrency = 1) { + if (!Number.isInteger(maxConcurrency) || maxConcurrency < 1) { + throw new Error(`maxConcurrency must be a positive integer, got ${maxConcurrency}`) + } + this.maxConcurrency = maxConcurrency this.sem = new TaskSemaphore(maxConcurrency) } @@ -19,6 +24,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 +55,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 60bc2f3192..d106724417 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -60,6 +60,13 @@ function requireDefined(value: T | null | undefined): T { return value } +async function getProviderStateWithOverrides( + provider: ClineProvider, + overrides: Partial, +): Promise { + return { ...(await provider.getState()), ...overrides } +} + // Mock delay before any imports that might use it vi.mock("delay", () => ({ __esModule: true, @@ -472,6 +479,48 @@ describe("Cline", () => { expect(cline.diffStrategy).toBeDefined() }) + it("uses startupSnapshot configuration, mode, profile, and mistake-limit precedence", async () => { + const startupApiConfiguration: ProviderSettings = { + ...mockApiConfig, + apiModelId: "snapshot-model", + consecutiveMistakeLimit: 11, + } + const cline = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + consecutiveMistakeLimit: 5, + task: "test task", + startTask: false, + startupSnapshot: { + apiConfiguration: startupApiConfiguration, + mode: "architect", + apiConfigName: "child-profile", + }, + }) + + expect(cline.apiConfiguration).toEqual(startupApiConfiguration) + expect(cline.consecutiveMistakeLimit).toBe(11) + expect(await cline.getTaskMode()).toBe("architect") + expect(await cline.getTaskApiConfigName()).toBe("child-profile") + }) + + it("falls back to the constructor mistake limit when the snapshot has none", async () => { + const cline = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + consecutiveMistakeLimit: 7, + task: "test task", + startTask: false, + startupSnapshot: { + apiConfiguration: { ...mockApiConfig }, + mode: "code", + }, + }) + + expect(cline.consecutiveMistakeLimit).toBe(7) + await expect(cline.getTaskApiConfigName()).resolves.toBe("default") + }) + it("should use default consecutiveMistakeLimit when not provided", () => { const cline = new Task({ provider: mockProvider, @@ -727,6 +776,61 @@ 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( + await getProviderStateWithOverrides(mockProvider, { + 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( + await getProviderStateWithOverrides(mockProvider, { + 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 = [ { diff --git a/src/core/task/__tests__/TaskScheduler.spec.ts b/src/core/task/__tests__/TaskScheduler.spec.ts index 9a0a4f5701..bb63e0bafe 100644 --- a/src/core/task/__tests__/TaskScheduler.spec.ts +++ b/src/core/task/__tests__/TaskScheduler.spec.ts @@ -13,6 +13,38 @@ describe("TaskScheduler", () => { expect(ran).toBe(true) }) + it("tryReserve() never queues and reserved execution releases on success, error, and abort", async () => { + const scheduler = new TaskScheduler(1) + const release = await scheduler.tryReserve() + expect(release).toBeDefined() + expect(scheduler.available).toBe(0) + expect(await scheduler.tryReserve()).toBeUndefined() + + await scheduler.runWithReservation(release!, stubTask(), async () => {}) + expect(scheduler.available).toBe(1) + + const errorRelease = await scheduler.tryReserve() + await expect( + scheduler.runWithReservation(errorRelease!, stubTask(), async () => { + throw new Error("reserved boom") + }), + ).rejects.toThrow("reserved boom") + expect(scheduler.available).toBe(1) + + const abortRelease = await scheduler.tryReserve() + const abortedTask = { abort: true, abandoned: false } as unknown as Task + await scheduler.runWithReservation(abortRelease!, abortedTask, async () => { + throw new Error("must not run") + }) + expect(scheduler.available).toBe(1) + }) + + it("rejects invalid maxConcurrency values", () => { + expect(() => new TaskScheduler(0)).toThrow("must be a positive integer") + expect(() => new TaskScheduler(1.5)).toThrow("must be a positive integer") + expect(() => new TaskScheduler(-1)).toThrow("must be a positive integer") + }) + it("queues a second task at maxConcurrency=1 until the first completes", async () => { const scheduler = new TaskScheduler(1) const order: number[] = [] 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..1cf961d566 --- /dev/null +++ b/src/core/task/__tests__/delegation-concurrent.spec.ts @@ -0,0 +1,490 @@ +// 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({ + apiConfiguration: {}, + mode: "code", + apiConfigName: "default", + }), + handleModeSwitchForChild: vi.fn().mockResolvedValue({ + apiConfiguration: {}, + mode: "code", + apiConfigName: "default", + }), + 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), + handleModeSwitchForChild: 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("handleModeSwitch() throws in the non-fan-out path: the evicted parent is restored", async () => { + const parent = makeParent() + const modeSwitchError = new Error("mode switch failed") + const createTask = vi.fn() + const createTaskWithHistoryItem = vi.fn().mockResolvedValue(parent) + + const provider = makeProviderStub({ + ...baseStubFields(parent), + handleModeSwitch: vi.fn().mockRejectedValue(modeSwitchError), + handleModeSwitchForChild: vi.fn().mockRejectedValue(modeSwitchError), + tasks: [parent], + taskScheduler: new TaskScheduler(1), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask, + createTaskWithHistoryItem, + }) + + await expect(callDelegate(provider)).rejects.toThrow(modeSwitchError) + + expect(createTask).not.toHaveBeenCalled() + 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 206d6ca611..892a9e5d93 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -102,7 +102,7 @@ import { forceFullModelDetailsLoad, hasLoadedFullDetails } from "../../api/provi import { ContextProxy } from "../config/ContextProxy" import { ProviderSettingsManager } from "../config/ProviderSettingsManager" import { CustomModesManager } from "../config/CustomModesManager" -import { Task } from "../task/Task" +import { Task, type TaskStartupSnapshot } from "../task/Task" import { webviewMessageHandler } from "./webviewMessageHandler" import type { ClineMessage, TodoItem } from "@roo-code/types" @@ -161,6 +161,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 @@ -562,13 +569,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) } @@ -696,6 +707,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 /** @@ -1566,7 +1588,25 @@ export class ClineProvider * @param targetTask The task whose in-memory mode should be updated. Defaults to the * current task. Pass null to apply only global mode/profile effects for a pending child. */ - public async handleModeSwitch(newMode: Mode, targetTask: Task | null | undefined = this.getCurrentTask()) { + public async handleModeSwitch( + newMode: Mode, + targetTask: Task | null | undefined = this.getCurrentTask(), + ): Promise { + await this.handleModeSwitchAndGetStartupSnapshot(newMode, targetTask) + } + + public async handleModeSwitchForChild(newMode: Mode, targetTask?: Task | null): Promise { + const startupSnapshot = await this.handleModeSwitchAndGetStartupSnapshot(newMode, targetTask) + if (!startupSnapshot) { + throw new Error(`Unable to capture startup snapshot for mode '${newMode}'`) + } + return startupSnapshot + } + + private handleModeSwitchAndGetStartupSnapshot( + newMode: Mode, + targetTask: Task | null | undefined = this.getCurrentTask(), + ): Promise { return this.enqueueProviderProfileMutation((signal) => this.handleModeSwitchUnlocked(newMode, targetTask, signal), ) @@ -1576,7 +1616,7 @@ export class ClineProvider newMode: Mode, targetTask: Task | null | undefined, signal?: AbortSignal, - ): Promise { + ): Promise { const task = targetTask if (task) { @@ -1617,7 +1657,11 @@ export class ClineProvider if (targetTask !== null) { await this.postStateToWebview() } - return + return { + apiConfiguration: structuredClone(this.contextProxy.getProviderSettings()), + mode: newMode, + apiConfigName: this.getGlobalState("currentApiConfigName"), + } } if (signal?.aborted) return @@ -1651,6 +1695,20 @@ export class ClineProvider targetTask === null ? { skipCurrentTaskRebuild: true } : undefined, signal, ) + + if (signal?.aborted) return + + const startupSnapshot: TaskStartupSnapshot = { + apiConfiguration: fullProfile, + mode: newMode, + apiConfigName: fullProfile.name, + } + + if (targetTask !== null) { + await this.postStateToWebview() + } + + return startupSnapshot } else { // The task will continue with the current/default configuration. } @@ -1673,6 +1731,12 @@ export class ClineProvider if (targetTask !== null) { await this.postStateToWebview() } + + return { + apiConfiguration: structuredClone(this.contextProxy.getProviderSettings()), + mode: newMode, + apiConfigName: this.getGlobalState("currentApiConfigName"), + } } // Provider Profile Management @@ -3168,6 +3232,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) { @@ -3240,6 +3309,7 @@ export class ClineProvider parentTask?: Task, options: CreateTaskOptions = {}, configuration: RooCodeSettings = {}, + startupSnapshot?: TaskStartupSnapshot, ): Promise { if (configuration) { await this.setValues(configuration) @@ -3282,13 +3352,14 @@ export class ClineProvider } const { - apiConfiguration, + apiConfiguration: stateApiConfiguration, enableCheckpoints, checkpointTimeout, experiments, organizationAllowList, diffFuzzyThreshold, } = await this.getState() + const apiConfiguration = startupSnapshot?.apiConfiguration ?? stateApiConfiguration // Single-open-task invariant: always enforce for user-initiated top-level tasks. if (!parentTask) { @@ -3304,6 +3375,7 @@ export class ClineProvider const task = new Task({ provider: this, apiConfiguration, + startupSnapshot, enableCheckpoints, checkpointTimeout, consecutiveMistakeLimit: apiConfiguration.consecutiveMistakeLimit, @@ -3633,13 +3705,50 @@ 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) { + const maxAttempts = 2 + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + return + } catch (rollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback (attempt ${attempt}/${maxAttempts}): ${ + (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 @@ -3695,11 +3804,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): ${ @@ -3713,17 +3839,34 @@ 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 + let startupSnapshot: TaskStartupSnapshot | undefined try { - await this.handleModeSwitch(mode as any) + startupSnapshot = await this.handleModeSwitchForChild(requestedMode, fanOut ? null : undefined) } catch (e) { this.log( `[delegateParentAndOpenChild] handleModeSwitch failed for mode '${mode}': ${ (e as Error)?.message ?? String(e) }`, ) + // Mode switching happens before child creation. If it fails, do not + // leave a fan-out reservation held or leave the parent evicted in the + // serial path with no task to resume it. + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) + throw e + } + if (!startupSnapshot) { + await this.restoreParentOrReleasePermit(parentTaskId, fanOut, childReservedRelease) + throw new Error(`[delegateParentAndOpenChild] No startup snapshot for mode '${mode}'`) } - // 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. @@ -3734,11 +3877,36 @@ 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, + { + initialTodos, + initialStatus: "active", + startTask: false, + }, + {}, + startupSnapshot, + ) + } 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 @@ -3802,10 +3970,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( @@ -3823,21 +3992,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.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 00f848bec4..81a6daccc9 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -25,7 +25,7 @@ import { defaultModeSlug } from "../../../shared/modes" import { experimentDefault } from "../../../shared/experiments" import { setTtsEnabled } from "../../../utils/tts" import { ContextProxy } from "../../config/ContextProxy" -import { Task, TaskOptions } from "../../task/Task" +import { Task, TaskOptions, type TaskStartupSnapshot } from "../../task/Task" import { safeWriteJson } from "../../../utils/safeWriteJson" import { ClineProvider } from "../ClineProvider" @@ -1851,6 +1851,44 @@ describe("ClineProvider", () => { expect(mockPostMessage).toHaveBeenCalledWith(expect.objectContaining({ type: "state" })) }) + it("returns a child startup snapshot without posting parent state when targetTask is null", async () => { + const profile = { + name: "child-profile", + id: "child-profile-id", + apiProvider: "anthropic" as const, + apiModelId: "child-model", + } + const fullProfile = { ...profile, apiKey: "child-key" } + ;(provider as unknown as { providerSettingsManager: Record }).providerSettingsManager = { + getModeConfigId: vi.fn().mockResolvedValue(profile.id), + listConfig: vi.fn().mockResolvedValue([profile]), + getProfile: vi.fn().mockResolvedValue(fullProfile), + activateProfile: vi.fn().mockResolvedValue(fullProfile), + setModeConfig: vi.fn(), + } + const postStateSpy = vi.spyOn(provider, "postStateToWebview").mockResolvedValue(undefined) + + const snapshot = await provider.handleModeSwitchForChild("architect", null) + + expect(snapshot).toEqual({ + apiConfiguration: fullProfile, + mode: "architect", + apiConfigName: "child-profile", + } satisfies TaskStartupSnapshot) + expect(postStateSpy).not.toHaveBeenCalled() + }) + + it("fails defensively when a child startup snapshot cannot be captured", async () => { + const providerWithSnapshotSeam = provider as unknown as { + handleModeSwitchAndGetStartupSnapshot: (mode: string, targetTask?: null) => Promise + } + vi.spyOn(providerWithSnapshotSeam, "handleModeSwitchAndGetStartupSnapshot").mockResolvedValue(undefined) + + await expect(provider.handleModeSwitchForChild("architect", null)).rejects.toThrow( + "Unable to capture startup snapshot for mode 'architect'", + ) + }) + test("saves current config when switching to mode without config", async () => { ;(provider as any).providerSettingsManager = { getModeConfigId: vi.fn().mockResolvedValue(undefined), diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 53d5ba4441..a3acede2d0 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1051,7 +1051,7 @@ }, "core/webview/ClineProvider.ts": { "@typescript-eslint/no-explicit-any": { - "count": 12 + "count": 10 } }, "core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts": { diff --git a/src/extension/__tests__/api-task-conversation-history-length.spec.ts b/src/extension/__tests__/api-task-conversation-history-length.spec.ts index 4cfd9bbe4b..c356e19a46 100644 --- a/src/extension/__tests__/api-task-conversation-history-length.spec.ts +++ b/src/extension/__tests__/api-task-conversation-history-length.spec.ts @@ -23,6 +23,7 @@ describe("API#getTaskApiConversationHistoryLength", () => { mockProvider = { context: {} as vscode.ExtensionContext, getTaskWithId: mockGetTaskWithId, + setTaskSchedulerMaxConcurrency: vi.fn(), on: vi.fn(), } as unknown as ClineProvider @@ -42,4 +43,10 @@ describe("API#getTaskApiConversationHistoryLength", () => { await expect(api.getTaskApiConversationHistoryLength("missing-task")).resolves.toBe(0) }) + + it("forwards the test-only scheduler concurrency setting to the provider", () => { + api.setTaskSchedulerMaxConcurrency(2) + + expect(mockProvider.setTaskSchedulerMaxConcurrency).toHaveBeenCalledWith(2) + }) }) diff --git a/src/extension/api.ts b/src/extension/api.ts index b57dc89b74..af68d24b5a 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -254,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() 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() diff --git a/src/utils/__tests__/TaskSemaphore.spec.ts b/src/utils/__tests__/TaskSemaphore.spec.ts index 8c8873b025..306ebb123a 100644 --- a/src/utils/__tests__/TaskSemaphore.spec.ts +++ b/src/utils/__tests__/TaskSemaphore.spec.ts @@ -9,6 +9,44 @@ describe("TaskSemaphore", () => { release() }) + it("tryAcquire() reserves an immediately available permit without queueing", async () => { + const sem = new TaskSemaphore(1) + const release = await sem.tryAcquire() + + expect(release).toBeDefined() + expect(sem.available).toBe(0) + expect(sem.waiting).toBe(0) + + const queued = sem.acquire() + await Promise.resolve() + const unavailable = await sem.tryAcquire() + expect(unavailable).toBeUndefined() + expect(sem.waiting).toBe(1) + + release!() + ;(await queued)() + }) + + it("tryAcquire() reserves a permit while a queued waiter is being handed one", async () => { + const sem = new TaskSemaphore(2) + const release1 = await sem.acquire() + const release2 = await sem.acquire() + const queued = sem.acquire() + + expect(sem.waiting).toBe(1) + release1() + release2() + + const reserved = await sem.tryAcquire() + expect(reserved).toBeDefined() + + const queuedRelease = await queued + queuedRelease() + reserved!() + expect(sem.available).toBe(2) + expect(sem.waiting).toBe(0) + }) + it("second acquire() queues when no permits remain; resolves after release", async () => { const sem = new TaskSemaphore(1) const release1 = await sem.acquire()