Skip to content

Commit 4cad931

Browse files
committed
fix: atomic initialStatus for child tasks to prevent race conditions
- Add initialStatus option to CreateTaskOptions to set status at historyItem creation - Child tasks now created with initialStatus: 'active' to ensure status exists before attempt_completion - AttemptCompletionTool requires explicit 'active' status for delegation (prevents undefined/corrupted states) - Reorder child→completed status update before parent metadata update in reopenParentFromDelegation - ChatView shows 'Start New Task' for completed subtasks instead of misleading 'Resume' - Remove unused 'aborted' status from history enum
1 parent 97f48c8 commit 4cad931

File tree

9 files changed

+173
-49
lines changed

9 files changed

+173
-49
lines changed

packages/types/src/history.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ export const historyItemSchema = z.object({
1919
size: z.number().optional(),
2020
workspace: z.string().optional(),
2121
mode: z.string().optional(),
22-
status: z.enum(["active", "completed", "aborted", "delegated"]).optional(),
22+
status: z.enum(["active", "completed", "delegated"]).optional(),
2323
delegatedToId: z.string().optional(), // Last child this parent delegated to
2424
childIds: z.array(z.string()).optional(), // All children spawned by this task
2525
awaitingChildId: z.string().optional(), // Child currently awaited (set when delegated)

packages/types/src/task.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,8 @@ export interface CreateTaskOptions {
9595
consecutiveMistakeLimit?: number
9696
experiments?: Record<string, boolean>
9797
initialTodos?: TodoItem[]
98+
/** Initial status for the task's history item (e.g., "active" for child tasks) */
99+
initialStatus?: "active" | "delegated" | "completed"
98100
}
99101

100102
export enum TaskStatus {

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ describe("Nested delegation resume (A → B → C)", () => {
7272
delegatedToId: "B",
7373
awaitingChildId: "B",
7474
childIds: ["B"],
75+
parentTaskId: undefined,
7576
ts: 1,
7677
task: "Task A",
7778
tokensIn: 0,
@@ -86,6 +87,7 @@ describe("Nested delegation resume (A → B → C)", () => {
8687
delegatedToId: "C",
8788
awaitingChildId: "C",
8889
childIds: ["C"],
90+
parentTaskId: "A",
8991
ts: 2,
9092
task: "Task B",
9193
tokensIn: 0,
@@ -94,6 +96,18 @@ describe("Nested delegation resume (A → B → C)", () => {
9496
mode: "code",
9597
workspace: "/tmp",
9698
},
99+
C: {
100+
id: "C",
101+
status: "active",
102+
parentTaskId: "B",
103+
ts: 3,
104+
task: "Task C",
105+
tokensIn: 0,
106+
tokensOut: 0,
107+
totalCost: 0,
108+
mode: "code",
109+
workspace: "/tmp",
110+
},
97111
}
98112

99113
const emitSpy = vi.fn()

src/__tests__/provider-delegation.spec.ts

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,29 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
1313
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
1414
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1" })
1515
const handleModeSwitch = vi.fn().mockResolvedValue(undefined)
16-
const getTaskWithId = vi.fn().mockResolvedValue({
17-
historyItem: {
18-
id: "parent-1",
19-
task: "Parent",
20-
tokensIn: 0,
21-
tokensOut: 0,
22-
totalCost: 0,
23-
childIds: [],
24-
},
16+
const getTaskWithId = vi.fn().mockImplementation(async (id: string) => {
17+
if (id === "parent-1") {
18+
return {
19+
historyItem: {
20+
id: "parent-1",
21+
task: "Parent",
22+
tokensIn: 0,
23+
tokensOut: 0,
24+
totalCost: 0,
25+
childIds: [],
26+
},
27+
}
28+
}
29+
// child-1
30+
return {
31+
historyItem: {
32+
id: "child-1",
33+
task: "Do something",
34+
tokensIn: 0,
35+
tokensOut: 0,
36+
totalCost: 0,
37+
},
38+
}
2539
})
2640

2741
const provider = {
@@ -48,12 +62,18 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
4862

4963
// Invariant: parent closed before child creation
5064
expect(removeClineFromStack).toHaveBeenCalledTimes(1)
51-
expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, { initialTodos: [] })
65+
// Child task is created with initialStatus: "active" to avoid race conditions
66+
expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, {
67+
initialTodos: [],
68+
initialStatus: "active",
69+
})
5270

53-
// Metadata persistence
71+
// Metadata persistence - parent gets "delegated" status (child status is set at creation via initialStatus)
5472
expect(updateTaskHistory).toHaveBeenCalledTimes(1)
55-
const saved = updateTaskHistory.mock.calls[0][0]
56-
expect(saved).toEqual(
73+
74+
// Parent set to "delegated"
75+
const parentSaved = updateTaskHistory.mock.calls[0][0]
76+
expect(parentSaved).toEqual(
5777
expect.objectContaining({
5878
id: "parent-1",
5979
status: "delegated",

src/core/task-persistence/taskMetadata.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ export type TaskMetadataOptions = {
2121
globalStoragePath: string
2222
workspace: string
2323
mode?: string
24+
/** Initial status for the task (e.g., "active" for child tasks) */
25+
initialStatus?: "active" | "delegated" | "completed"
2426
}
2527

2628
export async function taskMetadata({
@@ -32,6 +34,7 @@ export async function taskMetadata({
3234
globalStoragePath,
3335
workspace,
3436
mode,
37+
initialStatus,
3538
}: TaskMetadataOptions) {
3639
const taskDir = await getTaskDirectoryPath(globalStoragePath, id)
3740

@@ -84,6 +87,9 @@ export async function taskMetadata({
8487
}
8588

8689
// Create historyItem once with pre-calculated values.
90+
// initialStatus is included when provided (e.g., "active" for child tasks)
91+
// to ensure the status is set from the very first save, avoiding race conditions
92+
// where attempt_completion might run before a separate status update.
8793
const historyItem: HistoryItem = {
8894
id,
8995
rootTaskId,
@@ -101,6 +107,7 @@ export async function taskMetadata({
101107
size: taskDirSize,
102108
workspace,
103109
mode,
110+
...(initialStatus && { status: initialStatus }),
104111
}
105112

106113
return { historyItem, tokenUsage }

src/core/task/Task.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,8 @@ export interface TaskOptions extends CreateTaskOptions {
150150
onCreated?: (task: Task) => void
151151
initialTodos?: TodoItem[]
152152
workspacePath?: string
153+
/** Initial status for the task's history item (e.g., "active" for child tasks) */
154+
initialStatus?: "active" | "delegated" | "completed"
153155
}
154156

155157
export class Task extends EventEmitter<TaskEvents> implements TaskLike {
@@ -321,6 +323,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
321323
// Cloud Sync Tracking
322324
private cloudSyncedMessageTimestamps: Set<number> = new Set()
323325

326+
// Initial status for the task's history item (set at creation time to avoid race conditions)
327+
private readonly initialStatus?: "active" | "delegated" | "completed"
328+
324329
constructor({
325330
provider,
326331
apiConfiguration,
@@ -341,6 +346,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
341346
onCreated,
342347
initialTodos,
343348
workspacePath,
349+
initialStatus,
344350
}: TaskOptions) {
345351
super()
346352

@@ -427,6 +433,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
427433

428434
this.parentTask = parentTask
429435
this.taskNumber = taskNumber
436+
this.initialStatus = initialStatus
430437

431438
// Store the task's mode when it's created.
432439
// For history items, use the stored mode; for new tasks, we'll set it
@@ -868,6 +875,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
868875
globalStoragePath: this.globalStoragePath,
869876
workspace: this.cwd,
870877
mode: this._taskMode || defaultModeSlug, // Use the task's own mode, not the current provider mode.
878+
initialStatus: this.initialStatus,
871879
})
872880

873881
if (hasTokenUsageChanged(tokenUsage, this.tokenUsageSnapshot)) {

src/core/tools/AttemptCompletionTool.ts

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -96,12 +96,14 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
9696
if (provider) {
9797
try {
9898
const { historyItem } = await provider.getTaskWithId(task.taskId)
99-
if (historyItem?.status === "completed") {
99+
const status = historyItem?.status
100+
101+
if (status === "completed") {
100102
// Subtask already completed - skip delegation flow entirely
101103
// Fall through to normal completion ask flow below (outside this if block)
102104
// This shows the user the completion result and waits for acceptance
103105
// without injecting another tool_result to the parent
104-
} else {
106+
} else if (status === "active") {
105107
// Normal subtask completion - do delegation
106108
const delegated = await this.delegateToParent(
107109
task,
@@ -111,17 +113,23 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
111113
pushToolResult,
112114
)
113115
if (delegated) return
116+
} else {
117+
// Unexpected status (undefined or "delegated") - log error and skip delegation
118+
// undefined indicates a bug in status persistence during child creation
119+
// "delegated" would mean this child has its own grandchild pending (shouldn't reach attempt_completion)
120+
console.error(
121+
`[AttemptCompletionTool] Unexpected child task status "${status}" for task ${task.taskId}. ` +
122+
`Expected "active" or "completed". Skipping delegation to prevent data corruption.`,
123+
)
124+
// Fall through to normal completion ask flow
114125
}
115-
} catch {
116-
// If we can't get the history, proceed with normal delegation flow
117-
const delegated = await this.delegateToParent(
118-
task,
119-
result,
120-
provider,
121-
askFinishSubTaskApproval,
122-
pushToolResult,
126+
} catch (err) {
127+
// If we can't get the history, log error and skip delegation
128+
console.error(
129+
`[AttemptCompletionTool] Failed to get history for task ${task.taskId}: ${(err as Error)?.message ?? String(err)}. ` +
130+
`Skipping delegation.`,
123131
)
124-
if (delegated) return
132+
// Fall through to normal completion ask flow
125133
}
126134
}
127135
}

src/core/webview/ClineProvider.ts

Lines changed: 32 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -933,6 +933,8 @@ export class ClineProvider
933933
onCreated: this.taskCreationCallback,
934934
startTask: options?.startTask ?? true,
935935
enableBridge: BridgeOrchestrator.isEnabled(cloudUserInfo, taskSyncEnabled),
936+
// Preserve the status from the history item to avoid overwriting it when the task saves messages
937+
initialStatus: historyItem.status,
936938
})
937939

938940
if (isRehydratingCurrentTask) {
@@ -3017,9 +3019,15 @@ export class ClineProvider
30173019
}
30183020

30193021
// 4) Create child as sole active (parent reference preserved for lineage)
3020-
const child = await this.createTask(message, undefined, parent as any, { initialTodos })
3022+
// Pass initialStatus: "active" to ensure the child task's historyItem is created
3023+
// with status from the start, avoiding race conditions where the task might
3024+
// call attempt_completion before status is persisted separately.
3025+
const child = await this.createTask(message, undefined, parent as any, {
3026+
initialTodos,
3027+
initialStatus: "active",
3028+
})
30213029

3022-
// 4) Persist parent delegation metadata
3030+
// 5) Persist parent delegation metadata
30233031
try {
30243032
const { historyItem } = await this.getTaskWithId(parentTaskId)
30253033
const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId]))
@@ -3039,7 +3047,7 @@ export class ClineProvider
30393047
)
30403048
}
30413049

3042-
// 5) Emit TaskDelegated (provider-level)
3050+
// 6) Emit TaskDelegated (provider-level)
30433051
try {
30443052
this.emit(RooCodeEventName.TaskDelegated, parentTaskId, child.taskId)
30453053
} catch {
@@ -3164,7 +3172,22 @@ export class ClineProvider
31643172

31653173
await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath })
31663174

3167-
// 3) Update parent metadata and persist BEFORE emitting completion event
3175+
// 3) Update child metadata to "completed" status
3176+
try {
3177+
const { historyItem: childHistory } = await this.getTaskWithId(childTaskId)
3178+
await this.updateTaskHistory({
3179+
...childHistory,
3180+
status: "completed",
3181+
})
3182+
} catch (err) {
3183+
this.log(
3184+
`[reopenParentFromDelegation] Failed to persist child completed status for ${childTaskId}: ${
3185+
(err as Error)?.message ?? String(err)
3186+
}`,
3187+
)
3188+
}
3189+
3190+
// 4) Update parent metadata and persist BEFORE emitting completion event
31683191
const childIds = Array.from(new Set([...(historyItem.childIds ?? []), childTaskId]))
31693192
const updatedHistory: typeof historyItem = {
31703193
...historyItem,
@@ -3176,35 +3199,24 @@ export class ClineProvider
31763199
}
31773200
await this.updateTaskHistory(updatedHistory)
31783201

3179-
// 4) Emit TaskDelegationCompleted (provider-level)
3202+
// 5) Emit TaskDelegationCompleted (provider-level)
31803203
try {
31813204
this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary)
31823205
} catch {
31833206
// non-fatal
31843207
}
31853208

3186-
// 5) Close child instance if still open (single-open-task invariant)
3209+
// 6) Close child instance if still open (single-open-task invariant)
31873210
const current = this.getCurrentTask()
31883211
if (current?.taskId === childTaskId) {
31893212
await this.removeClineFromStack()
31903213
}
31913214

3192-
// 5b) Mark child subtask as "completed" to prevent duplicate tool_result on revisit
3193-
try {
3194-
const { historyItem: childHistory } = await this.getTaskWithId(childTaskId)
3195-
await this.updateTaskHistory({
3196-
...childHistory,
3197-
status: "completed",
3198-
})
3199-
} catch {
3200-
// non-fatal: child history may not exist for some edge cases
3201-
}
3202-
3203-
// 6) Reopen the parent from history as the sole active task (restores saved mode)
3215+
// 7) Reopen the parent from history as the sole active task (restores saved mode)
32043216
// IMPORTANT: startTask=false to suppress resume-from-history ask scheduling
32053217
const parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { startTask: false })
32063218

3207-
// 7) Inject restored histories into the in-memory instance before resuming
3219+
// 8) Inject restored histories into the in-memory instance before resuming
32083220
if (parentInstance) {
32093221
try {
32103222
await parentInstance.overwriteClineMessages(parentClineMessages)
@@ -3221,7 +3233,7 @@ export class ClineProvider
32213233
await parentInstance.resumeAfterDelegation()
32223234
}
32233235

3224-
// 8) Emit TaskDelegationResumed (provider-level)
3236+
// 9) Emit TaskDelegationResumed (provider-level)
32253237
try {
32263238
this.emit(RooCodeEventName.TaskDelegationResumed, parentTaskId, childTaskId)
32273239
} catch {

0 commit comments

Comments
 (0)