diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index 587ae917a6..3d77e4358f 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest" import { RooCodeEventName } from "@roo-code/types" +import type { HistoryItem } from "@roo-code/types" /* vscode mock for Task/Provider imports */ vi.mock("vscode", () => { @@ -40,6 +41,39 @@ import { readTaskMessages } from "../core/task-persistence/taskMessages" import { readApiMessages, saveApiMessages, saveTaskMessages } from "../core/task-persistence" import { makeProviderStub } from "./helpers/provider-stub" +/** + * Create a minimal taskHistoryStore stub whose atomicUpdatePair calls both updaters + * with the provided items and resolves, simulating the happy-path atomic write. + */ +function makeTaskHistoryStoreStub( + childItem: Record, + parentItem: Record, + overrides: { atomicUpdatePair?: ReturnType } = {}, +) { + const itemMap = new Map>([ + [childItem.id!, childItem], + [parentItem.id!, parentItem], + ]) + + const atomicUpdatePair = vi.fn( + async ( + firstId: string, + secondId: string, + firstUpdater: (h: HistoryItem) => HistoryItem, + secondUpdater: (h: HistoryItem) => HistoryItem, + ) => { + firstUpdater(itemMap.get(firstId) as HistoryItem) + secondUpdater(itemMap.get(secondId) as HistoryItem) + return [] + }, + ) + + return { + atomicUpdatePair: overrides.atomicUpdatePair ?? atomicUpdatePair, + get: vi.fn((id: string) => itemMap.get(id)), + } +} + describe("History resume delegation - parent metadata transitions", () => { beforeEach(() => { vi.clearAllMocks() @@ -47,24 +81,23 @@ describe("History resume delegation - parent metadata transitions", () => { it("reopenParentFromDelegation accepts an active parent awaiting the returning child", async () => { const providerEmit = vi.fn() - const getTaskWithId = vi.fn().mockResolvedValue({ - historyItem: { - id: "parent-1", - status: "active", - delegatedToId: "child-1", - awaitingChildId: "child-1", - childIds: ["child-1"], - ts: Date.now(), - task: "Parent task", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - mode: "code", - workspace: "/tmp", - }, - }) + const parentHistoryItem = { + id: "parent-1", + status: "active", + delegatedToId: "child-1", + awaitingChildId: "child-1", + childIds: ["child-1"], + ts: Date.now(), + task: "Parent task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + mode: "code", + workspace: "/tmp", + } + const getTaskWithId = vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }) - const updateTaskHistory = vi.fn().mockResolvedValue([]) + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-1", status: "active" }, parentHistoryItem) const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const createTaskWithHistoryItem = vi.fn().mockResolvedValue({ taskId: "parent-1", @@ -79,10 +112,9 @@ describe("History resume delegation - parent metadata transitions", () => { getCurrentTask: vi.fn(() => ({ taskId: "child-1" })), removeClineFromStack, createTaskWithHistoryItem, - updateTaskHistory, + taskHistoryStore, } as any) - // Mock persistence reads to return empty arrays vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -92,22 +124,31 @@ describe("History resume delegation - parent metadata transitions", () => { completionResultSummary: "Child done", }) - // Assert: metadata updated BEFORE createTaskWithHistoryItem - expect(updateTaskHistory).toHaveBeenCalledWith( - expect.objectContaining({ - id: "parent-1", - status: "active", - completedByChildId: "child-1", - completionResultSummary: "Child done", - awaitingChildId: undefined, - childIds: ["child-1"], - }), - ) + // atomicUpdatePair called with child first, parent second + expect(taskHistoryStore.atomicUpdatePair).toHaveBeenCalledTimes(1) + const [firstId, secondId, firstUpdater, secondUpdater] = taskHistoryStore.atomicUpdatePair.mock.calls[0] + expect(firstId).toBe("child-1") + expect(secondId).toBe("parent-1") + + // Verify child updater produces completed status + const updatedChild = firstUpdater({ id: "child-1", status: "active" } as HistoryItem) + expect(updatedChild.status).toBe("completed") + + // Verify parent updater produces active status with correct fields + const updatedParent = secondUpdater(parentHistoryItem as HistoryItem) + expect(updatedParent).toMatchObject({ + id: "parent-1", + status: "active", + completedByChildId: "child-1", + completionResultSummary: "Child done", + awaitingChildId: undefined, + childIds: ["child-1"], + }) - // Verify call ordering: updateTaskHistory before createTaskWithHistoryItem - const updateCall = updateTaskHistory.mock.invocationCallOrder[0] + // atomicUpdatePair must happen before createTaskWithHistoryItem + const atomicCall = taskHistoryStore.atomicUpdatePair.mock.invocationCallOrder[0] const createCall = createTaskWithHistoryItem.mock.invocationCallOrder[0] - expect(updateCall).toBeLessThan(createCall) + expect(atomicCall).toBeLessThan(createCall) // Verify child closed and parent reopened with updated metadata expect(removeClineFromStack).toHaveBeenCalledTimes(1) @@ -122,21 +163,21 @@ describe("History resume delegation - parent metadata transitions", () => { }) it("reopenParentFromDelegation injects subtask_result into both UI and API histories", async () => { + const parentItem = { + id: "p1", + status: "delegated", + awaitingChildId: "c1", + childIds: [], + ts: 100, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c1", status: "active" }, parentItem) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/storage" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "p1", - status: "delegated", - awaitingChildId: "c1", - childIds: [], - ts: 100, - task: "Parent", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "c1" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), @@ -146,7 +187,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteClineMessages: vi.fn().mockResolvedValue(undefined), overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), - updateTaskHistory: vi.fn().mockResolvedValue([]), + taskHistoryStore, } as any) // Start with existing messages in history @@ -205,21 +246,21 @@ describe("History resume delegation - parent metadata transitions", () => { }) it("reopenParentFromDelegation injects tool_result when new_task tool_use exists in API history", async () => { + const parentItem = { + id: "p-tool", + status: "delegated", + awaitingChildId: "c-tool", + childIds: [], + ts: 100, + task: "Parent with tool_use", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c-tool", status: "active" }, parentItem) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/storage" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "p-tool", - status: "delegated", - awaitingChildId: "c-tool", - childIds: [], - ts: 100, - task: "Parent with tool_use", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "c-tool" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), @@ -229,7 +270,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteClineMessages: vi.fn().mockResolvedValue(undefined), overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), - updateTaskHistory: vi.fn().mockResolvedValue([]), + taskHistoryStore, } as any) // Include an assistant message with new_task tool_use to exercise the tool_result path @@ -291,21 +332,21 @@ describe("History resume delegation - parent metadata transitions", () => { }) it("reopenParentFromDelegation injects plain text when no new_task tool_use exists in API history", async () => { + const parentItem = { + id: "p-no-tool", + status: "delegated", + awaitingChildId: "c-no-tool", + childIds: [], + ts: 100, + task: "Parent without tool_use", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c-no-tool", status: "active" }, parentItem) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/storage" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "p-no-tool", - status: "delegated", - awaitingChildId: "c-no-tool", - childIds: [], - ts: 100, - task: "Parent without tool_use", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "c-no-tool" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), @@ -315,7 +356,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteClineMessages: vi.fn().mockResolvedValue(undefined), overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), - updateTaskHistory: vi.fn().mockResolvedValue([]), + taskHistoryStore, } as any) // No assistant tool_use in history @@ -351,26 +392,26 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), } + const parentItem = { + id: "parent-2", + status: "delegated", + awaitingChildId: "child-2", + childIds: [], + ts: 200, + task: "P", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-2", status: "active" }, parentItem) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "parent-2", - status: "delegated", - awaitingChildId: "child-2", - childIds: [], - ts: 200, - task: "P", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "child-2" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), - updateTaskHistory: vi.fn().mockResolvedValue([]), + taskHistoryStore, } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -389,23 +430,22 @@ describe("History resume delegation - parent metadata transitions", () => { it("reopenParentFromDelegation emits events in correct order: TaskDelegationCompleted → TaskDelegationResumed", async () => { const emitSpy = vi.fn() - const updateTaskHistory = vi.fn().mockResolvedValue([]) + const parentItem = { + id: "p3", + status: "delegated", + awaitingChildId: "c3", + childIds: [], + ts: 300, + task: "P3", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c3", status: "active" }, parentItem) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "p3", - status: "delegated", - awaitingChildId: "c3", - childIds: [], - ts: 300, - task: "P3", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: emitSpy, getCurrentTask: vi.fn(() => ({ taskId: "c3" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), @@ -414,7 +454,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteClineMessages: vi.fn().mockResolvedValue(undefined), overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), - updateTaskHistory, + taskHistoryStore, } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -437,16 +477,10 @@ describe("History resume delegation - parent metadata transitions", () => { expect(completedIdx).toBeGreaterThanOrEqual(0) expect(resumedIdx).toBeGreaterThan(completedIdx) - // RPD-05: verify parent metadata persistence happens before TaskDelegationCompleted emit - const parentUpdateCallIdx = updateTaskHistory.mock.calls.findIndex((call) => { - const item = call[0] as { id?: string; status?: string } | undefined - return item?.id === "p3" && item.status === "active" - }) - expect(parentUpdateCallIdx).toBeGreaterThanOrEqual(0) - - const parentUpdateCallOrder = updateTaskHistory.mock.invocationCallOrder[parentUpdateCallIdx] + // RPD-05: atomicUpdatePair must be called before TaskDelegationCompleted + const atomicCallOrder = taskHistoryStore.atomicUpdatePair.mock.invocationCallOrder[0] const completedEmitCallOrder = emitSpy.mock.invocationCallOrder[completedIdx] - expect(parentUpdateCallOrder).toBeLessThan(completedEmitCallOrder) + expect(atomicCallOrder).toBeLessThan(completedEmitCallOrder) }) it("reopenParentFromDelegation continues when overwrite operations fail and still resumes/emits (RPD-06)", async () => { @@ -457,42 +491,27 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockRejectedValue(new Error("api overwrite failed")), } + const parentItem = { + id: "parent-rpd06", + status: "delegated", + awaitingChildId: "child-rpd06", + childIds: ["child-rpd06"], + ts: 800, + task: "Parent RPD-06", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-rpd06", status: "active" }, parentItem) + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockImplementation(async (id: string) => { - if (id === "parent-rpd06") { - return { - historyItem: { - id: "parent-rpd06", - status: "delegated", - awaitingChildId: "child-rpd06", - childIds: ["child-rpd06"], - ts: 800, - task: "Parent RPD-06", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - } - } - - return { - historyItem: { - id: "child-rpd06", - status: "active", - ts: 801, - task: "Child RPD-06", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - } - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: emitSpy, getCurrentTask: vi.fn(() => ({ taskId: "child-rpd06" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), - updateTaskHistory: vi.fn().mockResolvedValue([]), + taskHistoryStore, } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -526,22 +545,22 @@ describe("History resume delegation - parent metadata transitions", () => { it("reopenParentFromDelegation does NOT emit TaskPaused or TaskUnpaused (new flow only)", async () => { const emitSpy = vi.fn() + const parentItem = { + id: "p4", + status: "delegated", + awaitingChildId: "c4", + childIds: [], + ts: 400, + task: "P4", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c4", status: "active" }, parentItem) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "p4", - status: "delegated", - awaitingChildId: "c4", - childIds: [], - ts: 400, - task: "P4", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: emitSpy, getCurrentTask: vi.fn(() => ({ taskId: "c4" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), @@ -550,7 +569,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteClineMessages: vi.fn().mockResolvedValue(undefined), overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), - updateTaskHistory: vi.fn().mockResolvedValue([]), + taskHistoryStore, } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -576,45 +595,29 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), } - const updateTaskHistory = vi.fn().mockResolvedValue([]) + const parentItem = { + id: "parent-rpd02", + status: "delegated", + awaitingChildId: "child-rpd02", + childIds: ["child-rpd02"], + ts: 600, + task: "Parent RPD-02", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-rpd02", status: "active" }, parentItem) const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const createTaskWithHistoryItem = vi.fn().mockResolvedValue(parentInstance) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockImplementation(async (id: string) => { - if (id === "parent-rpd02") { - return { - historyItem: { - id: "parent-rpd02", - status: "delegated", - awaitingChildId: "child-rpd02", - childIds: ["child-rpd02"], - ts: 600, - task: "Parent RPD-02", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - } - } - return { - historyItem: { - id: "child-rpd02", - status: "active", - ts: 601, - task: "Child RPD-02", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - } - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "different-open-task" })), removeClineFromStack, createTaskWithHistoryItem, - updateTaskHistory, + taskHistoryStore, } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -627,12 +630,17 @@ describe("History resume delegation - parent metadata transitions", () => { }) expect(removeClineFromStack).not.toHaveBeenCalled() - expect(updateTaskHistory).toHaveBeenCalledWith( - expect.objectContaining({ - id: "child-rpd02", - status: "completed", - }), - ) + + // Verify atomicUpdatePair called with child first (completed) and parent second (active) + expect(taskHistoryStore.atomicUpdatePair).toHaveBeenCalledTimes(1) + const [firstId, secondId, firstUpdater, secondUpdater] = taskHistoryStore.atomicUpdatePair.mock.calls[0] + expect(firstId).toBe("child-rpd02") + expect(secondId).toBe("parent-rpd02") + const updatedChild = firstUpdater({ id: "child-rpd02", status: "active" } as HistoryItem) + expect(updatedChild.status).toBe("completed") + const updatedParent = secondUpdater(parentItem as HistoryItem) + expect(updatedParent).toMatchObject({ id: "parent-rpd02", status: "active", completedByChildId: "child-rpd02" }) + expect(createTaskWithHistoryItem).toHaveBeenCalledWith( expect.objectContaining({ id: "parent-rpd02", @@ -644,110 +652,80 @@ describe("History resume delegation - parent metadata transitions", () => { expect(parentInstance.resumeAfterDelegation).toHaveBeenCalledTimes(1) }) - it("reopenParentFromDelegation logs child status persistence failure and continues reopen flow (RPD-04)", async () => { - const logSpy = vi.fn() - const emitSpy = vi.fn() - const parentInstance = { - resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), - overwriteClineMessages: vi.fn().mockResolvedValue(undefined), - overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + it("reopenParentFromDelegation propagates atomicUpdatePair failure — parent not reopened (RPD-04)", async () => { + const parentItem = { + id: "parent-rpd04", + status: "delegated", + awaitingChildId: "child-rpd04", + childIds: ["child-rpd04"], + ts: 700, + task: "Parent RPD-04", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, } - - const updateTaskHistory = vi.fn().mockImplementation(async (historyItem: { id?: string }) => { - if (historyItem.id === "child-rpd04") { - throw new Error("child status persist failed") - } - return [] + const persistError = new Error("atomic pair write failed") + const atomicUpdatePair = vi.fn().mockRejectedValue(persistError) + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-rpd04", status: "active" }, parentItem, { + atomicUpdatePair, }) + const createTaskWithHistoryItem = vi.fn() const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockImplementation(async (id: string) => { - if (id === "parent-rpd04") { - return { - historyItem: { - id: "parent-rpd04", - status: "delegated", - awaitingChildId: "child-rpd04", - childIds: ["child-rpd04"], - ts: 700, - task: "Parent RPD-04", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - } - } - return { - historyItem: { - id: "child-rpd04", - status: "active", - ts: 701, - task: "Child RPD-04", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - } - }), - emit: emitSpy, - log: logSpy, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + emit: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "child-rpd04" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), - createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), - updateTaskHistory, + createTaskWithHistoryItem, + taskHistoryStore, } as any) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) + // Failure propagates — child is closed (step 4 already ran) but parent is NOT reopened await expect( (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { parentTaskId: "parent-rpd04", childTaskId: "child-rpd04", completionResultSummary: "Child completion with persistence failure", }), - ).resolves.toBe(true) + ).rejects.toThrow(persistError) - expect(logSpy).toHaveBeenCalledWith( - expect.stringContaining( - "[reopenParentFromDelegation] Failed to persist child completed status for child-rpd04:", - ), - ) - expect(updateTaskHistory).toHaveBeenCalledWith( - expect.objectContaining({ - id: "parent-rpd04", - status: "active", - completedByChildId: "child-rpd04", - }), - ) - expect(parentInstance.resumeAfterDelegation).toHaveBeenCalledTimes(1) - expect(emitSpy).toHaveBeenCalledWith(RooCodeEventName.TaskDelegationResumed, "parent-rpd04", "child-rpd04") + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() }) - it("reopenParentFromDelegation keeps the child open when parent metadata persistence fails", async () => { + it("reopenParentFromDelegation aborts parent reopen when all persistence paths fail (RPD-05)", async () => { const persistError = new Error("parent status persist failed") const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const createTaskWithHistoryItem = vi.fn() + const parentItem = { + id: "parent-rpd05", + status: "delegated", + awaitingChildId: "child-rpd05", + childIds: ["child-rpd05"], + ts: 800, + task: "Parent RPD-05", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + // atomicUpdatePair failure aborts the reopen flow. + const atomicUpdatePair = vi.fn().mockRejectedValue(persistError) + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-rpd05", status: "active" }, parentItem, { + atomicUpdatePair, + }) + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "parent-rpd05", - status: "delegated", - awaitingChildId: "child-rpd05", - childIds: ["child-rpd05"], - ts: 800, - task: "Parent RPD-05", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: vi.fn(), + log: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "child-rpd05" })), removeClineFromStack, createTaskWithHistoryItem, + taskHistoryStore, updateTaskHistory: vi.fn().mockRejectedValue(persistError), } as any) @@ -762,26 +740,27 @@ describe("History resume delegation - parent metadata transitions", () => { }), ).rejects.toThrow(persistError) - expect(removeClineFromStack).not.toHaveBeenCalled() + // Child is closed before the atomic write (new ordering) — child closed, parent not reopened + expect(removeClineFromStack).toHaveBeenCalledTimes(1) expect(createTaskWithHistoryItem).not.toHaveBeenCalled() }) it("handles empty history gracefully when injecting synthetic messages", async () => { + const parentItem = { + id: "p5", + status: "delegated", + awaitingChildId: "c5", + childIds: [], + ts: 500, + task: "P5", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c5", status: "active" }, parentItem) const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { - id: "p5", - status: "delegated", - awaitingChildId: "c5", - childIds: [], - ts: 500, - task: "P5", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - }), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), emit: vi.fn(), getCurrentTask: vi.fn(() => ({ taskId: "c5" })), removeClineFromStack: vi.fn().mockResolvedValue(undefined), @@ -790,7 +769,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteClineMessages: vi.fn().mockResolvedValue(undefined), overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), - updateTaskHistory: vi.fn().mockResolvedValue([]), + taskHistoryStore, } as any) // Mock read failures or empty returns @@ -830,7 +809,7 @@ describe("History resume delegation - parent metadata transitions", () => { it("reopenParentFromDelegation aborts when parent is already active (stale-delegation guard)", async () => { const logSpy = vi.fn() - const updateTaskHistory = vi.fn() + const atomicUpdatePair = vi.fn() const saveTaskMessagesMock = vi.mocked(saveTaskMessages) const saveApiMessagesMock = vi.mocked(saveApiMessages) @@ -843,7 +822,7 @@ describe("History resume delegation - parent metadata transitions", () => { getCurrentTask: vi.fn(() => null), removeClineFromStack: vi.fn(), createTaskWithHistoryItem: vi.fn(), - updateTaskHistory, + taskHistoryStore: { atomicUpdatePair, get: vi.fn() }, } as any) const providerActive = makeProvider({ @@ -860,13 +839,13 @@ describe("History resume delegation - parent metadata transitions", () => { ).resolves.toBe(false) expect(saveTaskMessagesMock).not.toHaveBeenCalled() expect(saveApiMessagesMock).not.toHaveBeenCalled() - expect(updateTaskHistory).not.toHaveBeenCalled() + expect(atomicUpdatePair).not.toHaveBeenCalled() expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) }) it("reopenParentFromDelegation aborts when in-process cancellation failed closed", async () => { const logSpy = vi.fn() - const updateTaskHistory = vi.fn() + const atomicUpdatePair = vi.fn() const saveTaskMessagesMock = vi.mocked(saveTaskMessages) const saveApiMessagesMock = vi.mocked(saveApiMessages) const provider = makeProviderStub({ @@ -883,7 +862,7 @@ describe("History resume delegation - parent metadata transitions", () => { getCurrentTask: vi.fn(() => null), removeClineFromStack: vi.fn(), createTaskWithHistoryItem: vi.fn(), - updateTaskHistory, + taskHistoryStore: { atomicUpdatePair, get: vi.fn() }, cancelledDelegationChildIds: new Set(["child-guard"]), } as any) @@ -897,13 +876,13 @@ describe("History resume delegation - parent metadata transitions", () => { expect(saveTaskMessagesMock).not.toHaveBeenCalled() expect(saveApiMessagesMock).not.toHaveBeenCalled() - expect(updateTaskHistory).not.toHaveBeenCalled() + expect(atomicUpdatePair).not.toHaveBeenCalled() expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) }) it("reopenParentFromDelegation aborts when parent awaits a different child (stale-delegation guard)", async () => { const logSpy = vi.fn() - const updateTaskHistory = vi.fn() + const atomicUpdatePair = vi.fn() const saveTaskMessagesMock = vi.mocked(saveTaskMessages) const saveApiMessagesMock = vi.mocked(saveApiMessages) @@ -916,7 +895,7 @@ describe("History resume delegation - parent metadata transitions", () => { getCurrentTask: vi.fn(() => null), removeClineFromStack: vi.fn(), createTaskWithHistoryItem: vi.fn(), - updateTaskHistory, + taskHistoryStore: { atomicUpdatePair, get: vi.fn() }, } as any) const providerWrongChild = makeProvider({ @@ -933,7 +912,7 @@ describe("History resume delegation - parent metadata transitions", () => { ).resolves.toBe(false) expect(saveTaskMessagesMock).not.toHaveBeenCalled() expect(saveApiMessagesMock).not.toHaveBeenCalled() - expect(updateTaskHistory).not.toHaveBeenCalled() + expect(atomicUpdatePair).not.toHaveBeenCalled() expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) }) @@ -961,4 +940,192 @@ describe("History resume delegation - parent metadata transitions", () => { await expect(second).resolves.toBe("done") expect(calls).toEqual(["first", "second"]) }) + + it("reopenParentFromDelegation posts taskHistoryItemUpdated for both records when view is launched", async () => { + const childItem = { id: "c-webview", status: "active" } + const parentItem = { + id: "p-webview", + status: "delegated", + awaitingChildId: "c-webview", + childIds: [], + ts: 100, + task: "Parent webview", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + + // After atomicUpdatePair resolves, get() returns the merged committed items. + const updatedChild = { ...childItem, status: "completed" } + const updatedParent = { + ...parentItem, + status: "active", + awaitingChildId: undefined, + completedByChildId: "c-webview", + } + const itemMap = new Map([ + ["c-webview", updatedChild], + ["p-webview", updatedParent], + ]) + const taskHistoryStore = { + atomicUpdatePair: vi.fn(async (_fId: string, _sId: string, fU: (h: any) => any, sU: (h: any) => any) => { + fU(childItem) + sU(parentItem) + return [] + }), + get: vi.fn((id: string) => itemMap.get(id)), + } + + const postMessageToWebview = vi.fn().mockResolvedValue(undefined) + + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + emit: vi.fn(), + isViewLaunched: true, + getCurrentTask: vi.fn(() => ({ taskId: "c-webview" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + }), + taskHistoryStore, + postMessageToWebview, + } as any) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "p-webview", + childTaskId: "c-webview", + completionResultSummary: "Done", + }) + + // Must post taskHistoryItemUpdated for both the child (completed) and parent (active) + const webviewCalls = postMessageToWebview.mock.calls.filter((c) => c[0]?.type === "taskHistoryItemUpdated") + expect(webviewCalls).toHaveLength(2) + + const childMsg = webviewCalls.find((c) => c[0].taskHistoryItem.id === "c-webview") + expect(childMsg?.[0].taskHistoryItem).toMatchObject({ id: "c-webview", status: "completed" }) + + const parentMsg = webviewCalls.find((c) => c[0].taskHistoryItem.id === "p-webview") + expect(parentMsg?.[0].taskHistoryItem).toMatchObject({ id: "p-webview", status: "active" }) + + // Both must be sent after atomicUpdatePair (verified by call order) + const atomicOrder = taskHistoryStore.atomicUpdatePair.mock.invocationCallOrder[0] + for (const call of webviewCalls) { + const msgOrder = + postMessageToWebview.mock.invocationCallOrder[postMessageToWebview.mock.calls.indexOf(call)] + expect(atomicOrder).toBeLessThan(msgOrder) + } + }) + + it("reopenParentFromDelegation does NOT post taskHistoryItemUpdated when view is not launched", async () => { + const childItem = { id: "c-noview", status: "active" } + const parentItem = { + id: "p-noview", + status: "delegated", + awaitingChildId: "c-noview", + childIds: [], + ts: 100, + task: "Parent no-view", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub(childItem, parentItem) + const postMessageToWebview = vi.fn().mockResolvedValue(undefined) + + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + emit: vi.fn(), + isViewLaunched: false, + getCurrentTask: vi.fn(() => ({ taskId: "c-noview" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + }), + taskHistoryStore, + postMessageToWebview, + } as any) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "p-noview", + childTaskId: "c-noview", + completionResultSummary: "Done", + }) + + const webviewCalls = postMessageToWebview.mock.calls.filter((c) => c[0]?.type === "taskHistoryItemUpdated") + expect(webviewCalls).toHaveLength(0) + }) + + describe("atomicUpdatePair handoff correctness", () => { + it("after reopenParentFromDelegation, child is completed and parent is active atomically", async () => { + const parentItem = { + id: "p-handoff", + status: "delegated", + awaitingChildId: "c-handoff", + childIds: [], + ts: 100, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const childItem = { id: "c-handoff", status: "active" } + + let capturedChildResult: HistoryItem | undefined + let capturedParentResult: HistoryItem | undefined + + const atomicUpdatePair = vi.fn( + async ( + firstId: string, + secondId: string, + firstUpdater: (h: HistoryItem) => HistoryItem, + secondUpdater: (h: HistoryItem) => HistoryItem, + ) => { + // Both updaters must be applied atomically + capturedChildResult = firstUpdater(childItem as unknown as HistoryItem) + capturedParentResult = secondUpdater(parentItem as unknown as HistoryItem) + return [] + }, + ) + const taskHistoryStore = { atomicUpdatePair, get: vi.fn() } + + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "c-handoff" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + }), + taskHistoryStore, + } as any) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "p-handoff", + childTaskId: "c-handoff", + completionResultSummary: "Done", + }) + + // After atomicUpdatePair: child completed AND parent active — never one without the other + expect(capturedChildResult?.status).toBe("completed") + expect(capturedParentResult?.status).toBe("active") + expect(capturedParentResult?.awaitingChildId).toBeUndefined() + expect(capturedParentResult?.completedByChildId).toBe("c-handoff") + }) + }) }) diff --git a/src/__tests__/nested-delegation-resume.spec.ts b/src/__tests__/nested-delegation-resume.spec.ts index 9f78ba14bb..b905453303 100644 --- a/src/__tests__/nested-delegation-resume.spec.ts +++ b/src/__tests__/nested-delegation-resume.spec.ts @@ -149,6 +149,25 @@ describe("Nested delegation resume (A → B → C)", () => { return Object.values(historyIndex) }) + const taskHistoryStore = { + atomicUpdatePair: vi.fn( + async ( + firstId: string, + secondId: string, + firstUpdater: (h: any) => any, + secondUpdater: (h: any) => any, + ) => { + // Apply both updaters and persist to historyIndex atomically + const updatedFirst = firstUpdater(historyIndex[firstId]) + const updatedSecond = secondUpdater(historyIndex[secondId]) + historyIndex[firstId] = updatedFirst + historyIndex[secondId] = updatedSecond + return Object.values(historyIndex) + }, + ), + get: vi.fn((id: string) => historyIndex[id]), + } + const provider = makeProviderStub({ contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, getTaskWithId, @@ -157,6 +176,7 @@ describe("Nested delegation resume (A → B → C)", () => { removeClineFromStack, createTaskWithHistoryItem, updateTaskHistory, + taskHistoryStore, // Wire through provider method so attemptCompletionTool can call it reopenParentFromDelegation: vi.fn(async (params: any) => { return await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, params) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index a65d0e866c..a584cce955 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -166,16 +166,7 @@ export class TaskHistoryStore { * Must only be called from within a `withLock` callback. */ private async _upsertUnlocked(item: HistoryItem): Promise { - const existing = this.cache.get(item.id) - - // Merge: preserve existing metadata unless explicitly overwritten - const merged = existing ? { ...existing, ...item } : item - - // Write per-task file (source of truth) - await this.writeTaskFile(merged) - - // Update in-memory cache - this.cache.set(merged.id, merged) + await this.upsertCore(item) // Schedule debounced index write this.scheduleIndexWrite() @@ -565,6 +556,73 @@ export class TaskHistoryStore { }) } + /** + * Atomically update two related HistoryItems within a single lock acquisition. + * Both updaters run synchronously (no I/O, no lock re-entry). Both writes are + * committed before the lock releases — no concurrent writer can observe an + * intermediate state. + * + * @throws If either task ID is not present in the cache. + */ + public atomicUpdatePair( + firstId: string, + secondId: string, + firstUpdater: (current: HistoryItem) => HistoryItem, + secondUpdater: (current: HistoryItem) => HistoryItem, + ): Promise { + return this.withLock(async () => { + const first = this.cache.get(firstId) + if (!first) throw new Error(`[TaskHistoryStore] atomicUpdatePair: ${firstId} not found`) + const second = this.cache.get(secondId) + if (!second) throw new Error(`[TaskHistoryStore] atomicUpdatePair: ${secondId} not found`) + + const updatedFirst = firstUpdater(structuredClone(first)) + const updatedSecond = secondUpdater(structuredClone(second)) + + if (updatedFirst.id !== firstId) { + throw new Error( + `[TaskHistoryStore] atomicUpdatePair: first updater changed id from ${firstId} to ${updatedFirst.id}`, + ) + } + if (updatedSecond.id !== secondId) { + throw new Error( + `[TaskHistoryStore] atomicUpdatePair: second updater changed id from ${secondId} to ${updatedSecond.id}`, + ) + } + + // Merge with existing cache entries before writing, mirroring upsertCore. + const mergedFirst = { ...first, ...updatedFirst } + const mergedSecond = { ...second, ...updatedSecond } + + // Write both files before touching the cache so readers never observe a + // half-updated in-memory state between the two await points. + await this.writeTaskFile(mergedFirst) + await this.writeTaskFile(mergedSecond) + + // Both disk writes succeeded — now update the cache atomically. + this.cache.set(firstId, mergedFirst) + this.cache.set(secondId, mergedSecond) + + this.scheduleIndexWrite() + const all = this.getAll() + if (this.onWrite) { + await this.onWrite(all) + } + return all + }) + } + + /** + * Write a single item to disk and update the cache without triggering onWrite + * or scheduling an index write. Must only be called from within a withLock callback. + */ + private async upsertCore(item: HistoryItem): Promise { + const existing = this.cache.get(item.id) + const merged = existing ? { ...existing, ...item } : item + await this.writeTaskFile(merged) + this.cache.set(merged.id, merged) + } + // ────────────────────────────── Private: Write lock ────────────────────────────── /** diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts index ebf00d4954..53fabf838a 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts @@ -441,4 +441,205 @@ describe("TaskHistoryStore", () => { expect(store.get("gone-task")).toBeUndefined() }) }) + + describe("atomicUpdatePair()", () => { + it("updates both records and both files are written before lock releases", async () => { + await store.initialize() + + const child = makeHistoryItem({ id: "child-pair", status: "active", ts: 1000 }) + const parent = makeHistoryItem({ id: "parent-pair", status: "delegated", ts: 2000 }) + await store.upsert(child) + await store.upsert(parent) + + await store.atomicUpdatePair( + "child-pair", + "parent-pair", + (c) => ({ ...c, status: "completed" as const }), + (p) => ({ ...p, status: "active" as const, awaitingChildId: undefined }), + ) + + expect(store.get("child-pair")?.status).toBe("completed") + expect(store.get("parent-pair")?.status).toBe("active") + expect(store.get("parent-pair")?.awaitingChildId).toBeUndefined() + + // Both per-task files must be persisted + const childFile = path.join(tmpDir, "tasks", "child-pair", GlobalFileNames.historyItem) + const parentFile = path.join(tmpDir, "tasks", "parent-pair", GlobalFileNames.historyItem) + const childDisk = JSON.parse(await fs.readFile(childFile, "utf8")) + const parentDisk = JSON.parse(await fs.readFile(parentFile, "utf8")) + expect(childDisk.status).toBe("completed") + expect(parentDisk.status).toBe("active") + }) + + it("a concurrent upsert queued during the pair write sees completed state of both records", async () => { + await store.initialize() + + const child = makeHistoryItem({ id: "child-concurrent", status: "active", ts: 1000 }) + const parent = makeHistoryItem({ id: "parent-concurrent", status: "delegated", ts: 2000 }) + await store.upsert(child) + await store.upsert(parent) + + // Queue atomicUpdatePair and a follow-up upsert concurrently + const pairPromise = store.atomicUpdatePair( + "child-concurrent", + "parent-concurrent", + (c) => ({ ...c, status: "completed" as const }), + (p) => ({ ...p, status: "active" as const }), + ) + + // This upsert is queued while the pair lock is held — it must see the final state + const followUp = store.upsert({ ...parent, status: "active" as const, tokensOut: 999 }) + + await pairPromise + await followUp + + // Both pair updates landed; follow-up should have merged on top + expect(store.get("child-concurrent")?.status).toBe("completed") + expect(store.get("parent-concurrent")?.status).toBe("active") + expect(store.get("parent-concurrent")?.tokensOut).toBe(999) + }) + + it("throws when first updater returns a different id", async () => { + await store.initialize() + + const child = makeHistoryItem({ id: "child-id-check" }) + const parent = makeHistoryItem({ id: "parent-id-check" }) + await store.upsert(child) + await store.upsert(parent) + + await expect( + store.atomicUpdatePair( + "child-id-check", + "parent-id-check", + (c) => ({ ...c, id: "wrong-id" }), + (p) => p, + ), + ).rejects.toThrow( + "[TaskHistoryStore] atomicUpdatePair: first updater changed id from child-id-check to wrong-id", + ) + }) + + it("throws when second updater returns a different id", async () => { + await store.initialize() + + const child = makeHistoryItem({ id: "child-id-check2" }) + const parent = makeHistoryItem({ id: "parent-id-check2" }) + await store.upsert(child) + await store.upsert(parent) + + await expect( + store.atomicUpdatePair( + "child-id-check2", + "parent-id-check2", + (c) => c, + (p) => ({ ...p, id: "wrong-id" }), + ), + ).rejects.toThrow( + "[TaskHistoryStore] atomicUpdatePair: second updater changed id from parent-id-check2 to wrong-id", + ) + }) + + it("throws when first task ID is not in cache", async () => { + await store.initialize() + + const parent = makeHistoryItem({ id: "parent-missing-child" }) + await store.upsert(parent) + + await expect( + store.atomicUpdatePair( + "nonexistent-child", + "parent-missing-child", + (c) => c, + (p) => p, + ), + ).rejects.toThrow("[TaskHistoryStore] atomicUpdatePair: nonexistent-child not found") + }) + + it("throws when second task ID is not in cache", async () => { + await store.initialize() + + const child = makeHistoryItem({ id: "child-missing-parent" }) + await store.upsert(child) + + await expect( + store.atomicUpdatePair( + "child-missing-parent", + "nonexistent-parent", + (c) => c, + (p) => p, + ), + ).rejects.toThrow("[TaskHistoryStore] atomicUpdatePair: nonexistent-parent not found") + }) + + it("partial failure: first file write lands, second throws — error surfaces; cache unchanged (known disk/cache divergence)", async () => { + await store.initialize() + + const child = makeHistoryItem({ id: "child-partial", status: "active", ts: 1000 }) + const parent = makeHistoryItem({ id: "parent-partial", status: "delegated", ts: 2000 }) + await store.upsert(child) + await store.upsert(parent) + + let writeCallCount = 0 + const storeAny = store as any + const originalWriteTaskFile = storeAny.writeTaskFile.bind(store) + vi.spyOn(storeAny, "writeTaskFile").mockImplementation(async (...args: unknown[]) => { + writeCallCount++ + if (writeCallCount === 2) throw new Error("second writeTaskFile failed") + return originalWriteTaskFile(...args) + }) + + await expect( + store.atomicUpdatePair( + "child-partial", + "parent-partial", + (c) => ({ ...c, status: "completed" as const }), + (p) => ({ ...p, status: "active" as const }), + ), + ).rejects.toThrow("second writeTaskFile failed") + + // First file write landed on disk (known limitation — no rollback) + const childFile = path.join(tmpDir, "tasks", "child-partial", GlobalFileNames.historyItem) + const childDisk = JSON.parse(await fs.readFile(childFile, "utf8")) + expect(childDisk.status).toBe("completed") + + // Second file write did not land + const parentFile = path.join(tmpDir, "tasks", "parent-partial", GlobalFileNames.historyItem) + const parentDisk = JSON.parse(await fs.readFile(parentFile, "utf8")) + expect(parentDisk.status).toBe("delegated") + + // Cache was NOT updated (cache set is deferred until after both writes succeed) + expect(store.get("child-partial")?.status).toBe("active") + expect(store.get("parent-partial")?.status).toBe("delegated") + }) + + it("invokes onWrite callback once with both records updated", async () => { + const onWrite = vi.fn().mockResolvedValue(undefined) + const storeWithCallback = new TaskHistoryStore(tmpDir, { onWrite }) + await storeWithCallback.initialize() + + const child = makeHistoryItem({ id: "child-onwrite", status: "active" }) + const parent = makeHistoryItem({ id: "parent-onwrite", status: "delegated" }) + await storeWithCallback.upsert(child) + await storeWithCallback.upsert(parent) + + onWrite.mockClear() + + await storeWithCallback.atomicUpdatePair( + "child-onwrite", + "parent-onwrite", + (c) => ({ ...c, status: "completed" as const }), + (p) => ({ ...p, status: "active" as const }), + ) + + // onWrite called exactly once (not once per record) + expect(onWrite).toHaveBeenCalledTimes(1) + const writtenItems = onWrite.mock.calls[0][0] as HistoryItem[] + const childWritten = writtenItems.find((i) => i.id === "child-onwrite") + const parentWritten = writtenItems.find((i) => i.id === "parent-onwrite") + expect(childWritten?.status).toBe("completed") + expect(parentWritten?.status).toBe("active") + + storeWithCallback.dispose() + }) + }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 4e7ddf5973..51cf646e8a 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3746,44 +3746,51 @@ export class ClineProvider await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) - // 3) Persist parent metadata before closing the child. If persistence fails, - // the delegated child remains active and can retry completion. - const childIds = Array.from(new Set([...(historyItem.childIds ?? []), childTaskId])) - const updatedHistory: typeof historyItem = { - ...historyItem, - status: "active", - completedByChildId: childTaskId, - completionResultSummary, - awaitingChildId: undefined, - childIds, - } - await this.updateTaskHistory(updatedHistory) - // 4) Close child instance if still open (single-open-task invariant). - // This MUST happen BEFORE updating the child's status to "completed" because + // This MUST happen BEFORE marking the child "completed" because // removeClineFromStack() → abortTask(true) → saveClineMessages() writes // the historyItem with initialStatus (typically "active"), which would - // overwrite a "completed" status set earlier. + // overwrite a "completed" status set later. const current = this.getCurrentTask() if (current?.taskId === childTaskId) { await this.removeClineFromStack({ skipDelegationRepair: true }) } - // 5) Update child metadata to "completed" status. - // This runs after the abort so it overwrites the stale "active" status - // that saveClineMessages() may have written during step 4. - try { - const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) - await this.updateTaskHistory({ - ...childHistory, - status: "completed", - }) - } catch (err) { - this.log( - `[reopenParentFromDelegation] Failed to persist child completed status for ${childTaskId}: ${ - (err as Error)?.message ?? String(err) - }`, - ) + // 3+5) Atomically mark child completed and parent active in one lock acquisition. + // No intermediate state is ever persisted — no sentinel needed. + // Build the parent update inside the updater from the locked snapshot so + // any concurrent write that landed between step 1 and the lock acquisition + // is preserved rather than silently overwritten. + let updatedHistory!: typeof historyItem + await this.taskHistoryStore.atomicUpdatePair( + childTaskId, + parentTaskId, + (child) => ({ ...child, status: "completed" as const }), + (parent) => { + const childIds = Array.from(new Set([...(parent.childIds ?? []), childTaskId])) + updatedHistory = { + ...parent, + status: "active" as const, + completedByChildId: childTaskId, + completionResultSummary, + awaitingChildId: undefined, + childIds, + } + return updatedHistory + }, + ) + this.recentTasksCache = undefined + + // Notify the webview of both updated items so its in-memory history stays current. + if (this.isViewLaunched) { + const updatedChild = this.taskHistoryStore.get(childTaskId) + const updatedParent = this.taskHistoryStore.get(parentTaskId) + if (updatedChild) { + await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedChild }) + } + if (updatedParent) { + await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedParent }) + } } // 6) Emit TaskDelegationCompleted (provider-level)