mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-08-28 05:27:24 +00:00
fix: resolve race condition in new_task delegation that loses parent task history (#11331)
* 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. * docs: clarify Task.start() only handles new tasks, not history resume
This commit is contained in:
parent
62a0106ce0
commit
7c58f29975
5 changed files with 140 additions and 4 deletions
|
|
@ -95,6 +95,9 @@ export interface CreateTaskOptions {
|
|||
initialTodos?: TodoItem[]
|
||||
/** Initial status for the task's history item (e.g., "active" for child tasks) */
|
||||
initialStatus?: "active" | "delegated" | "completed"
|
||||
/** Whether to start the task loop immediately (default: true).
|
||||
* When false, the caller must invoke `task.start()` manually. */
|
||||
startTask?: boolean
|
||||
}
|
||||
|
||||
export enum TaskStatus {
|
||||
|
|
|
|||
|
|
@ -9,9 +9,10 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
|
|||
const providerEmit = vi.fn()
|
||||
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
|
||||
|
||||
const childStart = vi.fn()
|
||||
const updateTaskHistory = vi.fn()
|
||||
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
|
||||
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1" })
|
||||
const createTask = vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart })
|
||||
const handleModeSwitch = vi.fn().mockResolvedValue(undefined)
|
||||
const getTaskWithId = vi.fn().mockImplementation(async (id: string) => {
|
||||
if (id === "parent-1") {
|
||||
|
|
@ -62,10 +63,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
|
|||
|
||||
// Invariant: parent closed before child creation
|
||||
expect(removeClineFromStack).toHaveBeenCalledTimes(1)
|
||||
// Child task is created with initialStatus: "active" to avoid race conditions
|
||||
// Child task is created with startTask: false and initialStatus: "active"
|
||||
expect(createTask).toHaveBeenCalledWith("Do something", undefined, parentTask, {
|
||||
initialTodos: [],
|
||||
initialStatus: "active",
|
||||
startTask: false,
|
||||
})
|
||||
|
||||
// Metadata persistence - parent gets "delegated" status (child status is set at creation via initialStatus)
|
||||
|
|
@ -83,10 +85,61 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
|
|||
}),
|
||||
)
|
||||
|
||||
// child.start() must be called AFTER parent metadata is persisted
|
||||
expect(childStart).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Event emission (provider-level)
|
||||
expect(providerEmit).toHaveBeenCalledWith(RooCodeEventName.TaskDelegated, "parent-1", "child-1")
|
||||
|
||||
// Mode switch
|
||||
expect(handleModeSwitch).toHaveBeenCalledWith("code")
|
||||
})
|
||||
|
||||
it("calls child.start() only after parent metadata is persisted (no race condition)", async () => {
|
||||
const callOrder: string[] = []
|
||||
|
||||
const parentTask = { taskId: "parent-1", emit: vi.fn() } as any
|
||||
const childStart = vi.fn(() => callOrder.push("child.start"))
|
||||
|
||||
const updateTaskHistory = vi.fn(async () => {
|
||||
callOrder.push("updateTaskHistory")
|
||||
})
|
||||
const removeClineFromStack = vi.fn().mockResolvedValue(undefined)
|
||||
const createTask = vi.fn(async () => {
|
||||
callOrder.push("createTask")
|
||||
return { taskId: "child-1", start: childStart }
|
||||
})
|
||||
const handleModeSwitch = vi.fn().mockResolvedValue(undefined)
|
||||
const getTaskWithId = vi.fn().mockResolvedValue({
|
||||
historyItem: {
|
||||
id: "parent-1",
|
||||
task: "Parent",
|
||||
tokensIn: 0,
|
||||
tokensOut: 0,
|
||||
totalCost: 0,
|
||||
childIds: [],
|
||||
},
|
||||
})
|
||||
|
||||
const provider = {
|
||||
emit: vi.fn(),
|
||||
getCurrentTask: vi.fn(() => parentTask),
|
||||
removeClineFromStack,
|
||||
createTask,
|
||||
getTaskWithId,
|
||||
updateTaskHistory,
|
||||
handleModeSwitch,
|
||||
log: vi.fn(),
|
||||
} as unknown as ClineProvider
|
||||
|
||||
await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, {
|
||||
parentTaskId: "parent-1",
|
||||
message: "Do something",
|
||||
initialTodos: [],
|
||||
mode: "code",
|
||||
})
|
||||
|
||||
// Verify ordering: createTask → updateTaskHistory → child.start
|
||||
expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"])
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -516,6 +516,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
didAlreadyUseTool = false
|
||||
didToolFailInCurrentTurn = false
|
||||
didCompleteReadingStream = false
|
||||
private _started = false
|
||||
// No streaming parser is required.
|
||||
assistantMessageParser?: undefined
|
||||
private providerProfileChangeListener?: (config: { name: string; provider?: string }) => void
|
||||
|
|
@ -720,6 +721,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
onCreated?.(this)
|
||||
|
||||
if (startTask) {
|
||||
this._started = true
|
||||
if (task || images) {
|
||||
this.startTask(task, images)
|
||||
} else if (historyItem) {
|
||||
|
|
@ -2071,6 +2073,30 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Manually start a **new** task when it was created with `startTask: false`.
|
||||
*
|
||||
* This fires `startTask` as a background async operation for the
|
||||
* `task/images` code-path only. It does **not** handle the
|
||||
* `historyItem` resume path (use the constructor with `startTask: true`
|
||||
* for that). The primary use-case is in the delegation flow where the
|
||||
* parent's metadata must be persisted to globalState **before** the
|
||||
* child task begins writing its own history (avoiding a read-modify-write
|
||||
* race on globalState).
|
||||
*/
|
||||
public start(): void {
|
||||
if (this._started) {
|
||||
return
|
||||
}
|
||||
this._started = true
|
||||
|
||||
const { task, images } = this.metadata
|
||||
|
||||
if (task || images) {
|
||||
this.startTask(task ?? undefined, images ?? undefined)
|
||||
}
|
||||
}
|
||||
|
||||
private async startTask(task?: string, images?: string[]): Promise<void> {
|
||||
try {
|
||||
if (this.enableBridge) {
|
||||
|
|
|
|||
|
|
@ -1801,6 +1801,49 @@ describe("Cline", () => {
|
|||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe("start()", () => {
|
||||
it("should be a no-op if the task was already started in the constructor", () => {
|
||||
const task = new Task({
|
||||
provider: mockProvider,
|
||||
apiConfiguration: mockApiConfig,
|
||||
task: "test task",
|
||||
startTask: false,
|
||||
})
|
||||
|
||||
// Manually trigger start
|
||||
const startTaskSpy = vi.spyOn(task as any, "startTask").mockImplementation(async () => {})
|
||||
task.start()
|
||||
|
||||
expect(startTaskSpy).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Calling start() again should be a no-op
|
||||
task.start()
|
||||
expect(startTaskSpy).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it("should not call startTask if already started via constructor", () => {
|
||||
// Create a task that starts immediately (startTask defaults to true)
|
||||
// but mock startTask to prevent actual execution
|
||||
const startTaskSpy = vi.spyOn(Task.prototype as any, "startTask").mockImplementation(async () => {})
|
||||
|
||||
const task = new Task({
|
||||
provider: mockProvider,
|
||||
apiConfiguration: mockApiConfig,
|
||||
task: "test task",
|
||||
startTask: true,
|
||||
})
|
||||
|
||||
// startTask was called by the constructor
|
||||
expect(startTaskSpy).toHaveBeenCalledTimes(1)
|
||||
|
||||
// Calling start() should be a no-op since _started is already true
|
||||
task.start()
|
||||
expect(startTaskSpy).toHaveBeenCalledTimes(1)
|
||||
|
||||
startTaskSpy.mockRestore()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe("Queued message processing after condense", () => {
|
||||
|
|
|
|||
|
|
@ -3296,12 +3296,20 @@ export class ClineProvider
|
|||
// 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.
|
||||
//
|
||||
// Pass startTask: false to prevent the child from beginning its task loop
|
||||
// (and writing to globalState via saveClineMessages → updateTaskHistory)
|
||||
// before we persist the parent's delegation metadata in step 5.
|
||||
// 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,
|
||||
})
|
||||
|
||||
// 5) Persist parent delegation metadata
|
||||
// 5) Persist parent delegation metadata BEFORE the child starts writing.
|
||||
try {
|
||||
const { historyItem } = await this.getTaskWithId(parentTaskId)
|
||||
const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId]))
|
||||
|
|
@ -3321,7 +3329,10 @@ export class ClineProvider
|
|||
)
|
||||
}
|
||||
|
||||
// 6) Emit TaskDelegated (provider-level)
|
||||
// 6) Start the child task now that parent metadata is safely persisted.
|
||||
child.start()
|
||||
|
||||
// 7) Emit TaskDelegated (provider-level)
|
||||
try {
|
||||
this.emit(RooCodeEventName.TaskDelegated, parentTaskId, child.taskId)
|
||||
} catch {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue