Skip to content

Commit 33c377b

Browse files
daniel-lxsdaniel-lxs
committed
fix: resolve race condition in new_task delegation that loses parent task history
When delegateParentAndOpenChild creates a child task via createTask(), the Task constructor fires startTask() as a fire-and-forget async call. The child immediately begins its task loop and eventually calls saveClineMessages() → updateTaskHistory(), which reads globalState, modifies it, and writes back. Meanwhile, delegateParentAndOpenChild persists the parent's delegation metadata (status: 'delegated', delegatedToId, awaitingChildId, childIds) via a separate updateTaskHistory() call AFTER createTask() returns. These two concurrent read-modify-write operations on globalState race: the last writer wins, overwriting the other's changes. When the child's write lands last, the parent's delegation fields are lost, making the parent task unresumable when the child finishes. Fix: create the child task with startTask: false, persist the parent's delegation metadata first, then manually call child.start(). This ensures the parent metadata is safely in globalState before the child begins writing.
1 parent 2789bab commit 33c377b

File tree

5 files changed

+138
-4
lines changed

5 files changed

+138
-4
lines changed

‎packages/types/src/task.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,9 @@ export interface CreateTaskOptions {
9595
initialTodos?: TodoItem[]
9696
/** Initial status for the task's history item (e.g., "active" for child tasks) */
9797
initialStatus?: "active" | "delegated" | "completed"
98+
/** Whether to start the task loop immediately (default: true).
99+
* When false, the caller must invoke `task.start()` manually. */
100+
startTask?: boolean
98101
}
99102

100103
export enum TaskStatus {

‎src/__tests__/provider-delegation.spec.ts‎

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
99
const providerEmit = vi.fn()
1010
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
1111

12+
const childStart = vi.fn()
1213
const updateTaskHistory = vi.fn()
1314
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
14-
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1" })
15+
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart })
1516
const handleModeSwitch = vi.fn().mockResolvedValue(undefined)
1617
const getTaskWithId = vi.fn().mockImplementation(async (id: string) => {
1718
if (id === "parent-1") {
@@ -62,10 +63,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
6263

6364
// Invariant: parent closed before child creation
6465
expect(removeClineFromStack).toHaveBeenCalledTimes(1)
65-
// Child task is created with initialStatus: "active" to avoid race conditions
66+
// Child task is created with startTask: false and initialStatus: "active"
6667
expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, {
6768
initialTodos: [],
6869
initialStatus: "active",
70+
startTask: false,
6971
})
7072

7173
// Metadata persistence - parent gets "delegated" status (child status is set at creation via initialStatus)
@@ -83,10 +85,61 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
8385
}),
8486
)
8587

88+
// child.start() must be called AFTER parent metadata is persisted
89+
expect(childStart).toHaveBeenCalledTimes(1)
90+
8691
// Event emission (provider-level)
8792
expect(providerEmit).toHaveBeenCalledWith(RooCodeEventName.TaskDelegated, "parent-1", "child-1")
8893

8994
// Mode switch
9095
expect(handleModeSwitch).toHaveBeenCalledWith("code")
9196
})
97+
98+
it("calls child.start() only after parent metadata is persisted (no race condition)", async () => {
99+
const callOrder: string[] = []
100+
101+
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
102+
const childStart = vi.fn(() => callOrder.push("child.start"))
103+
104+
const updateTaskHistory = vi.fn(async () => {
105+
callOrder.push("updateTaskHistory")
106+
})
107+
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
108+
const createTask = vi.fn(async () => {
109+
callOrder.push("createTask")
110+
return { taskId: "child-1", start: childStart }
111+
})
112+
const handleModeSwitch = vi.fn().mockResolvedValue(undefined)
113+
const getTaskWithId = vi.fn().mockResolvedValue({
114+
historyItem: {
115+
id: "parent-1",
116+
task: "Parent",
117+
tokensIn: 0,
118+
tokensOut: 0,
119+
totalCost: 0,
120+
childIds: [],
121+
},
122+
})
123+
124+
const provider = {
125+
emit: vi.fn(),
126+
getCurrentTask: vi.fn(() => parentTask),
127+
removeClineFromStack,
128+
createTask,
129+
getTaskWithId,
130+
updateTaskHistory,
131+
handleModeSwitch,
132+
log: vi.fn(),
133+
} as unknown as ClineProvider
134+
135+
await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, {
136+
parentTaskId: "parent-1",
137+
message: "Do something",
138+
initialTodos: [],
139+
mode: "code",
140+
})
141+
142+
// Verify ordering: createTask → updateTaskHistory → child.start
143+
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"])
144+
})
92145
})

‎src/core/task/Task.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,6 +515,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
515515
didAlreadyUseTool = false
516516
didToolFailInCurrentTurn = false
517517
didCompleteReadingStream = false
518+
private _started = false
518519
// No streaming parser is required.
519520
assistantMessageParser?: undefined
520521
private providerProfileChangeListener?: (config: { name: string; provider?: string }) => void
@@ -719,6 +720,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
719720
onCreated?.(this)
720721

721722
if (startTask) {
723+
this._started = true
722724
if (task || images) {
723725
this.startTask(task, images)
724726
} else if (historyItem) {
@@ -2066,6 +2068,28 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
20662068
}
20672069
}
20682070

2071+
/**
2072+
* Manually start the task when it was created with `startTask: false`.
2073+
*
2074+
* This fires `startTask` as a background async operation, identical to
2075+
* what the constructor does when `startTask` is `true`. The primary
2076+
* use-case is in the delegation flow where the parent's metadata must
2077+
* be persisted to globalState **before** the child task begins writing
2078+
* its own history (avoiding a read-modify-write race on globalState).
2079+
*/
2080+
public start(): void {
2081+
if (this._started) {
2082+
return
2083+
}
2084+
this._started = true
2085+
2086+
const { task, images } = this.metadata
2087+
2088+
if (task || images) {
2089+
this.startTask(task ?? undefined, images ?? undefined)
2090+
}
2091+
}
2092+
20692093
private async startTask(task?: string, images?: string[]): Promise<void> {
20702094
try {
20712095
if (this.enableBridge) {

‎src/core/task/__tests__/Task.spec.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1801,6 +1801,49 @@ describe("Cline", () => {
18011801
})
18021802
})
18031803
})
1804+
1805+
describe("start()", () => {
1806+
it("should be a no-op if the task was already started in the constructor", () => {
1807+
const task = new Task({
1808+
provider: mockProvider,
1809+
apiConfiguration: mockApiConfig,
1810+
task: "test task",
1811+
startTask: false,
1812+
})
1813+
1814+
// Manually trigger start
1815+
const startTaskSpy = vi.spyOn(task as any, "startTask").mockImplementation(async () => {})
1816+
task.start()
1817+
1818+
expect(startTaskSpy).toHaveBeenCalledTimes(1)
1819+
1820+
// Calling start() again should be a no-op
1821+
task.start()
1822+
expect(startTaskSpy).toHaveBeenCalledTimes(1)
1823+
})
1824+
1825+
it("should not call startTask if already started via constructor", () => {
1826+
// Create a task that starts immediately (startTask defaults to true)
1827+
// but mock startTask to prevent actual execution
1828+
const startTaskSpy = vi.spyOn(Task.prototype as any, "startTask").mockImplementation(async () => {})
1829+
1830+
const task = new Task({
1831+
provider: mockProvider,
1832+
apiConfiguration: mockApiConfig,
1833+
task: "test task",
1834+
startTask: true,
1835+
})
1836+
1837+
// startTask was called by the constructor
1838+
expect(startTaskSpy).toHaveBeenCalledTimes(1)
1839+
1840+
// Calling start() should be a no-op since _started is already true
1841+
task.start()
1842+
expect(startTaskSpy).toHaveBeenCalledTimes(1)
1843+
1844+
startTaskSpy.mockRestore()
1845+
})
1846+
})
18041847
})
18051848

18061849
describe("Queued message processing after condense", () => {

‎src/core/webview/ClineProvider.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3253,12 +3253,20 @@ export class ClineProvider
32533253
// Pass initialStatus: "active" to ensure the child task's historyItem is created
32543254
// with status from the start, avoiding race conditions where the task might
32553255
// call attempt_completion before status is persisted separately.
3256+
//
3257+
// Pass startTask: false to prevent the child from beginning its task loop
3258+
// (and writing to globalState via saveClineMessages → updateTaskHistory)
3259+
// before we persist the parent's delegation metadata in step 5.
3260+
// Without this, the child's fire-and-forget startTask() races with step 5,
3261+
// and the last writer to globalState overwrites the other's changes—
3262+
// causing the parent's delegation fields to be lost.
32563263
const child = await this.createTask(message, undefined, parent as any, {
32573264
initialTodos,
32583265
initialStatus: "active",
3266+
startTask: false,
32593267
})
32603268

3261-
// 5) Persist parent delegation metadata
3269+
// 5) Persist parent delegation metadata BEFORE the child starts writing.
32623270
try {
32633271
const { historyItem } = await this.getTaskWithId(parentTaskId)
32643272
const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId]))
@@ -3278,7 +3286,10 @@ export class ClineProvider
32783286
)
32793287
}
32803288

3281-
// 6) Emit TaskDelegated (provider-level)
3289+
// 6) Start the child task now that parent metadata is safely persisted.
3290+
child.start()
3291+
3292+
// 7) Emit TaskDelegated (provider-level)
32823293
try {
32833294
this.emit(RooCodeEventName.TaskDelegated, parentTaskId, child.taskId)
32843295
} catch {

0 commit comments

Comments
 (0)