Skip to content
This repository was archived by the owner on May 15, 2026. It is now read-only.

Commit cd96fa6

Browse files
committed
fix: harden delegation lifecycle against race conditions with per-task metadata, mutual-exclusion guards, and multi-layer failure recovery (#11379)
* fix: race conditions in subtask delegation system Comprehensive fix for race conditions and error handling gaps in the subtask delegation system. Addresses multiple failure modes that could leave parent tasks permanently stuck in 'delegated' status, causing nested subtasks to hang. Key fixes: - Remove initialStatus from taskMetadata rebuild (eliminates status overwrites) - Persist delegation metadata to per-task files (resolves globalState eviction) - Add delegationInProgress mutex guard (prevents concurrent delegation ops) - TOCTOU race fixes with fresh re-reads before writes - Abort-aware pWaitFor predicate (prevents false 60s timeout on user input) - Remove silent .catch(() => {}) — all errors now logged unless task is aborting - Single-attempt delegation with parent repair on failure (no retry band-aids) - Cancel debouncedEmitTokenUsage in dispose() (prevents zombie callbacks) - new_task isolation truncation for parallel tool calls * fix: write all 6 delegation fields in every saveDelegationMeta call site * fix: align delegation tests with single-attempt implementation (no retry)
1 parent 766cfc3 commit cd96fa6

20 files changed

Lines changed: 2028 additions & 334 deletions

packages/types/src/task.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,6 @@ export interface CreateTaskOptions {
9393
consecutiveMistakeLimit?: number
9494
experiments?: Record<string, boolean>
9595
initialTodos?: TodoItem[]
96-
/** Initial status for the task's history item (e.g., "active" for child tasks) */
97-
initialStatus?: "active" | "delegated" | "completed"
9896
/** Whether to start the task loop immediately (default: true).
9997
* When false, the caller must invoke `task.start()` manually. */
10098
startTask?: boolean

src/__tests__/history-resume-delegation.spec.ts

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,18 @@ vi.mock("vscode", () => {
2929
vi.mock("../core/task-persistence/taskMessages", () => ({
3030
readTaskMessages: vi.fn().mockResolvedValue([]),
3131
}))
32-
vi.mock("../core/task-persistence", () => ({
33-
readApiMessages: vi.fn().mockResolvedValue([]),
34-
saveApiMessages: vi.fn().mockResolvedValue(undefined),
35-
saveTaskMessages: vi.fn().mockResolvedValue(undefined),
32+
vi.mock("../core/task-persistence", async (importOriginal) => {
33+
const actual = await importOriginal<typeof import("../core/task-persistence")>()
34+
return {
35+
...actual,
36+
readRooMessages: vi.fn().mockResolvedValue([]),
37+
saveRooMessages: vi.fn().mockResolvedValue(undefined),
38+
saveTaskMessages: vi.fn().mockResolvedValue(undefined),
39+
}
40+
})
41+
vi.mock("../core/task-persistence/delegationMeta", () => ({
42+
readDelegationMeta: vi.fn().mockResolvedValue(null),
43+
saveDelegationMeta: vi.fn().mockResolvedValue(undefined),
3644
}))
3745

3846
import { ClineProvider } from "../core/webview/ClineProvider"
@@ -145,6 +153,7 @@ describe("History resume delegation - parent metadata transitions", () => {
145153
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
146154
}),
147155
updateTaskHistory: vi.fn().mockResolvedValue([]),
156+
log: vi.fn(),
148157
} as unknown as ClineProvider
149158

150159
// Start with existing messages in history
@@ -228,6 +237,7 @@ describe("History resume delegation - parent metadata transitions", () => {
228237
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
229238
}),
230239
updateTaskHistory: vi.fn().mockResolvedValue([]),
240+
log: vi.fn(),
231241
} as unknown as ClineProvider
232242

233243
// Include an assistant message with new_task tool_use to exercise the tool_result path
@@ -314,6 +324,7 @@ describe("History resume delegation - parent metadata transitions", () => {
314324
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
315325
}),
316326
updateTaskHistory: vi.fn().mockResolvedValue([]),
327+
log: vi.fn(),
317328
} as unknown as ClineProvider
318329

319330
// No assistant tool_use in history
@@ -549,6 +560,7 @@ describe("History resume delegation - parent metadata transitions", () => {
549560
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
550561
}),
551562
updateTaskHistory: vi.fn().mockResolvedValue([]),
563+
log: vi.fn(),
552564
} as unknown as ClineProvider
553565

554566
vi.mocked(readTaskMessages).mockResolvedValue([])
@@ -748,6 +760,7 @@ describe("History resume delegation - parent metadata transitions", () => {
748760
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
749761
}),
750762
updateTaskHistory: vi.fn().mockResolvedValue([]),
763+
log: vi.fn(),
751764
} as unknown as ClineProvider
752765

753766
// Mock read failures or empty returns

src/__tests__/provider-delegation.spec.ts

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,20 @@
22

33
import { describe, it, expect, vi } from "vitest"
44
import { RooCodeEventName } from "@roo-code/types"
5+
6+
vi.mock("../core/task-persistence/delegationMeta", () => ({
7+
readDelegationMeta: vi.fn().mockResolvedValue(null),
8+
saveDelegationMeta: vi.fn().mockResolvedValue(undefined),
9+
}))
10+
511
import { ClineProvider } from "../core/webview/ClineProvider"
612

713
describe("ClineProvider.delegateParentAndOpenChild()", () => {
814
it("persists parent delegation metadata and emits TaskDelegated", async () => {
915
const providerEmit = vi.fn()
1016
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
1117

12-
const childStart = vi.fn()
18+
const childStart = vi.fn().mockResolvedValue(undefined)
1319
const updateTaskHistory = vi.fn()
1420
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
1521
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart })
@@ -48,6 +54,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
4854
updateTaskHistory,
4955
handleModeSwitch,
5056
log: vi.fn(),
57+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
5158
} as unknown as ClineProvider
5259

5360
const params = {
@@ -63,18 +70,26 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
6370

6471
// Invariant: parent closed before child creation
6572
expect(removeClineFromStack).toHaveBeenCalledTimes(1)
66-
// Child task is created with startTask: false and initialStatus: "active"
73+
// Child task is created with startTask: false
6774
expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, {
6875
initialTodos: [],
69-
initialStatus: "active",
7076
startTask: false,
7177
})
7278

73-
// Metadata persistence - parent gets "delegated" status (child status is set at creation via initialStatus)
74-
expect(updateTaskHistory).toHaveBeenCalledTimes(1)
79+
// Metadata persistence - child gets "active" status, parent gets "delegated" status
80+
expect(updateTaskHistory).toHaveBeenCalledTimes(2)
7581

76-
// Parent set to "delegated"
77-
const parentSaved = updateTaskHistory.mock.calls[0][0]
82+
// Child set to "active" (first call)
83+
const childSaved = updateTaskHistory.mock.calls[0][0]
84+
expect(childSaved).toEqual(
85+
expect.objectContaining({
86+
id: "child-1",
87+
status: "active",
88+
}),
89+
)
90+
91+
// Parent set to "delegated" (second call)
92+
const parentSaved = updateTaskHistory.mock.calls[1][0]
7893
expect(parentSaved).toEqual(
7994
expect.objectContaining({
8095
id: "parent-1",
@@ -99,7 +114,10 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
99114
const callOrder: string[] = []
100115

101116
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
102-
const childStart = vi.fn(() => callOrder.push("child.start"))
117+
const childStart = vi.fn(() => {
118+
callOrder.push("child.start")
119+
return Promise.resolve()
120+
})
103121

104122
const updateTaskHistory = vi.fn(async () => {
105123
callOrder.push("updateTaskHistory")
@@ -130,6 +148,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
130148
updateTaskHistory,
131149
handleModeSwitch,
132150
log: vi.fn(),
151+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
133152
} as unknown as ClineProvider
134153

135154
await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, {
@@ -139,7 +158,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
139158
mode: "code",
140159
})
141160

142-
// Verify ordering: createTask → updateTaskHistory → child.start
143-
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"])
161+
// Verify ordering: createTask → updateTaskHistory (child) → updateTaskHistory (parent) → child.start
162+
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "updateTaskHistory", "child.start"])
144163
})
145164
})

src/__tests__/removeClineFromStack-delegation.spec.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
// npx vitest run __tests__/removeClineFromStack-delegation.spec.ts
22

33
import { describe, it, expect, vi } from "vitest"
4+
5+
vi.mock("../core/task-persistence/delegationMeta", () => ({
6+
readDelegationMeta: vi.fn().mockResolvedValue(null),
7+
saveDelegationMeta: vi.fn().mockResolvedValue(undefined),
8+
}))
9+
410
import { ClineProvider } from "../core/webview/ClineProvider"
511

612
describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
@@ -38,6 +44,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
3844
log: vi.fn(),
3945
getTaskWithId,
4046
updateTaskHistory,
47+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
4148
}
4249

4350
return { provider, childTask, updateTaskHistory, getTaskWithId }
@@ -183,6 +190,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
183190
log: vi.fn(),
184191
getTaskWithId: vi.fn(),
185192
updateTaskHistory: vi.fn(),
193+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
186194
}
187195

188196
// Should not throw
@@ -263,6 +271,7 @@ describe("ClineProvider.removeClineFromStack() delegation awareness", () => {
263271
log: vi.fn(),
264272
getTaskWithId,
265273
updateTaskHistory,
274+
contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
266275
}
267276

268277
// Simulate what delegateParentAndOpenChild does: pop B with skipDelegationRepair

src/core/assistant-message/presentAssistantMessage.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ import { sanitizeToolUseId } from "../../utils/tool-id"
6060
*/
6161

6262
export async function presentAssistantMessage(cline: Task) {
63-
if (cline.abort) {
63+
if (cline.abort || cline.abandoned) {
6464
throw new Error(`[Task#presentAssistantMessage] task ${cline.taskId}.${cline.instanceId} aborted`)
6565
}
6666

@@ -972,6 +972,15 @@ export async function presentAssistantMessage(cline: Task) {
972972
// locked.
973973
cline.presentAssistantMessageLocked = false
974974

975+
// Early exit if task was aborted/abandoned during tool execution (e.g., new_task delegation).
976+
// Prevents unhandled promise rejections from recursive calls hitting the abort check.
977+
if (cline.abort || cline.abandoned) {
978+
if (cline.didCompleteReadingStream) {
979+
cline.userMessageContentReady = true
980+
}
981+
return
982+
}
983+
975984
// NOTE: When tool is rejected, iterator stream is interrupted and it waits
976985
// for `userMessageContentReady` to be true. Future calls to present will
977986
// skip execution since `didRejectTool` and iterate until `contentIndex` is
@@ -999,7 +1008,11 @@ export async function presentAssistantMessage(cline: Task) {
9991008
if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) {
10001009
// There are already more content blocks to stream, so we'll call
10011010
// this function ourselves.
1002-
presentAssistantMessage(cline)
1011+
presentAssistantMessage(cline).catch((err) => {
1012+
if (!cline.abort) {
1013+
console.error("[presentAssistantMessage] Unhandled error:", err)
1014+
}
1015+
})
10031016
return
10041017
} else {
10051018
// CRITICAL FIX: If we're out of bounds and the stream is complete, set userMessageContentReady
@@ -1012,7 +1025,11 @@ export async function presentAssistantMessage(cline: Task) {
10121025

10131026
// Block is partial, but the read stream may have finished.
10141027
if (cline.presentAssistantMessageHasPendingUpdates) {
1015-
presentAssistantMessage(cline)
1028+
presentAssistantMessage(cline).catch((err) => {
1029+
if (!cline.abort) {
1030+
console.error("[presentAssistantMessage] Unhandled error:", err)
1031+
}
1032+
})
10161033
}
10171034
}
10181035

0 commit comments

Comments
 (0)