Skip to content

Commit b7857bc

Browse files
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 9e46d3e commit b7857bc

20 files changed

Lines changed: 1714 additions & 352 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: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ vi.mock("../core/task-persistence", async (importOriginal) => {
3838
saveTaskMessages: vi.fn().mockResolvedValue(undefined),
3939
}
4040
})
41+
vi.mock("../core/task-persistence/delegationMeta", () => ({
42+
readDelegationMeta: vi.fn().mockResolvedValue(null),
43+
saveDelegationMeta: vi.fn().mockResolvedValue(undefined),
44+
}))
4145

4246
import { ClineProvider } from "../core/webview/ClineProvider"
4347
import { readTaskMessages } from "../core/task-persistence/taskMessages"
@@ -149,6 +153,7 @@ describe("History resume delegation - parent metadata transitions", () => {
149153
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
150154
}),
151155
updateTaskHistory: vi.fn().mockResolvedValue([]),
156+
log: vi.fn(),
152157
} as unknown as ClineProvider
153158

154159
// Start with existing messages in history
@@ -232,6 +237,7 @@ describe("History resume delegation - parent metadata transitions", () => {
232237
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
233238
}),
234239
updateTaskHistory: vi.fn().mockResolvedValue([]),
240+
log: vi.fn(),
235241
} as unknown as ClineProvider
236242

237243
// Include an assistant message with new_task tool_use to exercise the tool_result path
@@ -320,6 +326,7 @@ describe("History resume delegation - parent metadata transitions", () => {
320326
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
321327
}),
322328
updateTaskHistory: vi.fn().mockResolvedValue([]),
329+
log: vi.fn(),
323330
} as unknown as ClineProvider
324331

325332
// No assistant tool_use in history
@@ -555,6 +562,7 @@ describe("History resume delegation - parent metadata transitions", () => {
555562
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
556563
}),
557564
updateTaskHistory: vi.fn().mockResolvedValue([]),
565+
log: vi.fn(),
558566
} as unknown as ClineProvider
559567

560568
vi.mocked(readTaskMessages).mockResolvedValue([])
@@ -754,6 +762,7 @@ describe("History resume delegation - parent metadata transitions", () => {
754762
overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined),
755763
}),
756764
updateTaskHistory: vi.fn().mockResolvedValue([]),
765+
log: vi.fn(),
757766
} as unknown as ClineProvider
758767

759768
// 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

@@ -938,6 +938,15 @@ export async function presentAssistantMessage(cline: Task) {
938938
// locked.
939939
cline.presentAssistantMessageLocked = false
940940

941+
// Early exit if task was aborted/abandoned during tool execution (e.g., new_task delegation).
942+
// Prevents unhandled promise rejections from recursive calls hitting the abort check.
943+
if (cline.abort || cline.abandoned) {
944+
if (cline.didCompleteReadingStream) {
945+
cline.userMessageContentReady = true
946+
}
947+
return
948+
}
949+
941950
// NOTE: When tool is rejected, iterator stream is interrupted and it waits
942951
// for `userMessageContentReady` to be true. Future calls to present will
943952
// skip execution since `didRejectTool` and iterate until `contentIndex` is
@@ -965,7 +974,11 @@ export async function presentAssistantMessage(cline: Task) {
965974
if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) {
966975
// There are already more content blocks to stream, so we'll call
967976
// this function ourselves.
968-
presentAssistantMessage(cline)
977+
presentAssistantMessage(cline).catch((err) => {
978+
if (!cline.abort) {
979+
console.error("[presentAssistantMessage] Unhandled error:", err)
980+
}
981+
})
969982
return
970983
} else {
971984
// CRITICAL FIX: If we're out of bounds and the stream is complete, set userMessageContentReady
@@ -978,7 +991,11 @@ export async function presentAssistantMessage(cline: Task) {
978991

979992
// Block is partial, but the read stream may have finished.
980993
if (cline.presentAssistantMessageHasPendingUpdates) {
981-
presentAssistantMessage(cline)
994+
presentAssistantMessage(cline).catch((err) => {
995+
if (!cline.abort) {
996+
console.error("[presentAssistantMessage] Unhandled error:", err)
997+
}
998+
})
982999
}
9831000
}
9841001

0 commit comments

Comments
 (0)