diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index f3256bd1432..a78c41b7c06 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -387,6 +387,7 @@ 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 provider = { contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, @@ -411,7 +412,7 @@ describe("History resume delegation - parent metadata transitions", () => { overwriteClineMessages: vi.fn().mockResolvedValue(undefined), overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), }), - updateTaskHistory: vi.fn().mockResolvedValue([]), + updateTaskHistory, } as unknown as ClineProvider vi.mocked(readTaskMessages).mockResolvedValue([]) @@ -433,6 +434,92 @@ describe("History resume delegation - parent metadata transitions", () => { const resumedIdx = emitSpy.mock.calls.findIndex((c) => c[0] === RooCodeEventName.TaskDelegationResumed) 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] + const completedEmitCallOrder = emitSpy.mock.invocationCallOrder[completedIdx] + expect(parentUpdateCallOrder).toBeLessThan(completedEmitCallOrder) + }) + + it("reopenParentFromDelegation continues when overwrite operations fail and still resumes/emits (RPD-06)", async () => { + const emitSpy = vi.fn() + const parentInstance = { + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockRejectedValue(new Error("ui overwrite failed")), + overwriteApiConversationHistory: vi.fn().mockRejectedValue(new Error("api overwrite failed")), + } + + const provider = { + 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, + }, + } + }), + emit: emitSpy, + getCurrentTask: vi.fn(() => ({ taskId: "child-rpd06" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), + updateTaskHistory: vi.fn().mockResolvedValue([]), + } as unknown as ClineProvider + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-rpd06", + childTaskId: "child-rpd06", + completionResultSummary: "Subtask finished despite overwrite failures", + }), + ).resolves.toBeUndefined() + + expect(parentInstance.overwriteClineMessages).toHaveBeenCalledTimes(1) + expect(parentInstance.overwriteApiConversationHistory).toHaveBeenCalledTimes(1) + expect(parentInstance.resumeAfterDelegation).toHaveBeenCalledTimes(1) + + expect(emitSpy).toHaveBeenCalledWith( + RooCodeEventName.TaskDelegationCompleted, + "parent-rpd06", + "child-rpd06", + "Subtask finished despite overwrite failures", + ) + expect(emitSpy).toHaveBeenCalledWith(RooCodeEventName.TaskDelegationResumed, "parent-rpd06", "child-rpd06") + + const completedIdx = emitSpy.mock.calls.findIndex((c) => c[0] === RooCodeEventName.TaskDelegationCompleted) + const resumedIdx = emitSpy.mock.calls.findIndex((c) => c[0] === RooCodeEventName.TaskDelegationResumed) + expect(completedIdx).toBeGreaterThanOrEqual(0) + expect(resumedIdx).toBeGreaterThan(completedIdx) }) it("reopenParentFromDelegation does NOT emit TaskPaused or TaskUnpaused (new flow only)", async () => { @@ -480,6 +567,162 @@ describe("History resume delegation - parent metadata transitions", () => { expect(eventNames).not.toContain(RooCodeEventName.TaskSpawned) }) + it("reopenParentFromDelegation skips child close when current task differs and still reopens parent (RPD-02)", async () => { + const parentInstance = { + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + } + + const updateTaskHistory = vi.fn().mockResolvedValue([]) + const removeClineFromStack = vi.fn().mockResolvedValue(undefined) + const createTaskWithHistoryItem = vi.fn().mockResolvedValue(parentInstance) + + const provider = { + 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, + }, + } + }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "different-open-task" })), + removeClineFromStack, + createTaskWithHistoryItem, + updateTaskHistory, + } as unknown as ClineProvider + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-rpd02", + childTaskId: "child-rpd02", + completionResultSummary: "Child done without being current", + }) + + expect(removeClineFromStack).not.toHaveBeenCalled() + expect(updateTaskHistory).toHaveBeenCalledWith( + expect.objectContaining({ + id: "child-rpd02", + status: "completed", + }), + ) + expect(createTaskWithHistoryItem).toHaveBeenCalledWith( + expect.objectContaining({ + id: "parent-rpd02", + status: "active", + completedByChildId: "child-rpd02", + }), + { startTask: false }, + ) + 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), + } + + const updateTaskHistory = vi.fn().mockImplementation(async (historyItem: { id?: string }) => { + if (historyItem.id === "child-rpd04") { + throw new Error("child status persist failed") + } + return [] + }) + + const provider = { + 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, + getCurrentTask: vi.fn(() => ({ taskId: "child-rpd04" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue(parentInstance), + updateTaskHistory, + } as unknown as ClineProvider + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-rpd04", + childTaskId: "child-rpd04", + completionResultSummary: "Child completion with persistence failure", + }), + ).resolves.toBeUndefined() + + 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") + }) + it("handles empty history gracefully when injecting synthetic messages", async () => { const provider = { contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index fc15a8dd5c3..62848a9b0c6 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -149,6 +149,7 @@ export class ClineProvider private currentWorkspacePath: string | undefined private recentTasksCache?: string[] + private taskHistoryWriteLock: Promise = Promise.resolve() private pendingOperations: Map = new Map() private static readonly PENDING_OPERATION_TIMEOUT_MS = 30000 // 30 seconds @@ -1803,10 +1804,12 @@ export class ClineProvider } // Delete all tasks from state in one batch - const taskHistory = this.getGlobalState("taskHistory") ?? [] - const updatedTaskHistory = taskHistory.filter((task) => !allIdsToDelete.includes(task.id)) - await this.updateGlobalState("taskHistory", updatedTaskHistory) - this.recentTasksCache = undefined + await this.withTaskHistoryLock(async () => { + const taskHistory = this.getGlobalState("taskHistory") ?? [] + const updatedTaskHistory = taskHistory.filter((task) => !allIdsToDelete.includes(task.id)) + await this.updateGlobalState("taskHistory", updatedTaskHistory) + this.recentTasksCache = undefined + }) // Delete associated shadow repositories or branches and task directories const globalStorageDir = this.contextProxy.globalStorageUri.fsPath @@ -1847,10 +1850,12 @@ export class ClineProvider } async deleteTaskFromState(id: string) { - const taskHistory = this.getGlobalState("taskHistory") ?? [] - const updatedTaskHistory = taskHistory.filter((task) => task.id !== id) - await this.updateGlobalState("taskHistory", updatedTaskHistory) - this.recentTasksCache = undefined + await this.withTaskHistoryLock(async () => { + const taskHistory = this.getGlobalState("taskHistory") ?? [] + const updatedTaskHistory = taskHistory.filter((task) => task.id !== id) + await this.updateGlobalState("taskHistory", updatedTaskHistory) + this.recentTasksCache = undefined + }) await this.postStateToWebview() } @@ -2511,6 +2516,19 @@ export class ClineProvider } } + /** + * Serializes all read-modify-write operations on taskHistory to prevent + * concurrent interleaving that can cause entries to vanish. + */ + private withTaskHistoryLock(fn: () => Promise): Promise { + const result = this.taskHistoryWriteLock.then(fn, fn) // run even if previous write errored + this.taskHistoryWriteLock = result.then( + () => {}, + () => {}, + ) // swallow for chain continuity + return result + } + /** * Updates a task in the task history and optionally broadcasts the updated history to the webview. * @param item The history item to update or add @@ -2518,34 +2536,36 @@ export class ClineProvider * @returns The updated task history array */ async updateTaskHistory(item: HistoryItem, options: { broadcast?: boolean } = {}): Promise { - const { broadcast = true } = options - const history = (this.getGlobalState("taskHistory") as HistoryItem[] | undefined) || [] - const existingItemIndex = history.findIndex((h) => h.id === item.id) - const wasExisting = existingItemIndex !== -1 - - if (wasExisting) { - // Preserve existing metadata (e.g., delegation fields) unless explicitly overwritten. - // This prevents loss of status/awaitingChildId/delegatedToId when tasks are reopened, - // terminated, or when routine message persistence occurs. - history[existingItemIndex] = { - ...history[existingItemIndex], - ...item, + return this.withTaskHistoryLock(async () => { + const { broadcast = true } = options + const history = (this.getGlobalState("taskHistory") as HistoryItem[] | undefined) || [] + const existingItemIndex = history.findIndex((h) => h.id === item.id) + const wasExisting = existingItemIndex !== -1 + + if (wasExisting) { + // Preserve existing metadata (e.g., delegation fields) unless explicitly overwritten. + // This prevents loss of status/awaitingChildId/delegatedToId when tasks are reopened, + // terminated, or when routine message persistence occurs. + history[existingItemIndex] = { + ...history[existingItemIndex], + ...item, + } + } else { + history.push(item) } - } else { - history.push(item) - } - await this.updateGlobalState("taskHistory", history) - this.recentTasksCache = undefined + await this.updateGlobalState("taskHistory", history) + this.recentTasksCache = undefined - // Broadcast the updated history to the webview if requested. - // Prefer per-item updates to avoid repeatedly cloning/sending the full history. - if (broadcast && this.isViewLaunched) { - const updatedItem = wasExisting ? history[existingItemIndex] : item - await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedItem }) - } + // Broadcast the updated history to the webview if requested. + // Prefer per-item updates to avoid repeatedly cloning/sending the full history. + if (broadcast && this.isViewLaunched) { + const updatedItem = wasExisting ? history[existingItemIndex] : item + await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedItem }) + } - return history + return history + }) } /** @@ -3416,7 +3436,19 @@ export class ClineProvider await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) - // 3) Update child metadata to "completed" status + // 3) Close child instance if still open (single-open-task invariant). + // This MUST happen BEFORE updating the child's status to "completed" because + // removeClineFromStack() → abortTask(true) → saveClineMessages() writes + // the historyItem with initialStatus (typically "active"), which would + // overwrite a "completed" status set earlier. + const current = this.getCurrentTask() + if (current?.taskId === childTaskId) { + await this.removeClineFromStack() + } + + // 4) 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 3. try { const { historyItem: childHistory } = await this.getTaskWithId(childTaskId) await this.updateTaskHistory({ @@ -3431,7 +3463,7 @@ export class ClineProvider ) } - // 4) Update parent metadata and persist BEFORE emitting completion event + // 5) Update parent metadata and persist BEFORE emitting completion event const childIds = Array.from(new Set([...(historyItem.childIds ?? []), childTaskId])) const updatedHistory: typeof historyItem = { ...historyItem, @@ -3443,19 +3475,13 @@ export class ClineProvider } await this.updateTaskHistory(updatedHistory) - // 5) Emit TaskDelegationCompleted (provider-level) + // 6) Emit TaskDelegationCompleted (provider-level) try { this.emit(RooCodeEventName.TaskDelegationCompleted, parentTaskId, childTaskId, completionResultSummary) } catch { // non-fatal } - // 6) Close child instance if still open (single-open-task invariant) - const current = this.getCurrentTask() - if (current?.taskId === childTaskId) { - await this.removeClineFromStack() - } - // 7) Reopen the parent from history as the sole active task (restores saved mode) // IMPORTANT: startTask=false to suppress resume-from-history ask scheduling const parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { startTask: false }) diff --git a/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts b/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts index e0f1d2dc29a..aefed797443 100644 --- a/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.taskHistory.spec.ts @@ -420,6 +420,74 @@ describe("ClineProvider Task History Synchronization", () => { expect(taskHistoryItemUpdatedCalls.length).toBe(0) }) + it("preserves delegated metadata on partial update unless explicitly overwritten (UTH-02)", async () => { + await provider.resolveWebviewView(mockWebviewView) + provider.isViewLaunched = true + + const initial = createHistoryItem({ + id: "task-delegated-metadata", + task: "Delegated task", + status: "delegated", + delegatedToId: "child-1", + awaitingChildId: "child-1", + childIds: ["child-1"], + }) + + await provider.updateTaskHistory(initial, { broadcast: false }) + + // Partial update intentionally omits delegated metadata fields. + const partialUpdate: HistoryItem = { + ...createHistoryItem({ id: "task-delegated-metadata", task: "Delegated task (updated)" }), + status: "active", + } + + const updatedHistory = await provider.updateTaskHistory(partialUpdate, { broadcast: false }) + const updatedItem = updatedHistory.find((item) => item.id === "task-delegated-metadata") + + expect(updatedItem).toBeDefined() + expect(updatedItem?.status).toBe("active") + expect(updatedItem?.delegatedToId).toBe("child-1") + expect(updatedItem?.awaitingChildId).toBe("child-1") + expect(updatedItem?.childIds).toEqual(["child-1"]) + }) + + it("invalidates recentTasksCache on updateTaskHistory (UTH-04)", async () => { + const workspace = provider.cwd + const tsBase = Date.now() + + await provider.updateTaskHistory( + createHistoryItem({ + id: "cache-seed", + task: "Cache seed", + workspace, + ts: tsBase, + }), + { broadcast: false }, + ) + + const initialRecent = provider.getRecentTasks() + expect(initialRecent).toContain("cache-seed") + + // Prime cache and verify internal cache is set. + expect((provider as unknown as { recentTasksCache?: string[] }).recentTasksCache).toEqual(initialRecent) + + await provider.updateTaskHistory( + createHistoryItem({ + id: "cache-new", + task: "Cache new", + workspace, + ts: tsBase + 1, + }), + { broadcast: false }, + ) + + // Direct assertion for invalidation side-effect. + expect((provider as unknown as { recentTasksCache?: string[] }).recentTasksCache).toBeUndefined() + + const recomputedRecent = provider.getRecentTasks() + expect(recomputedRecent).toContain("cache-new") + }) + it("updates existing task in history", async () => { await provider.resolveWebviewView(mockWebviewView) provider.isViewLaunched = true @@ -597,4 +665,97 @@ describe("ClineProvider Task History Synchronization", () => { expect(state.taskHistory.some((item: HistoryItem) => item.workspace === "/different/workspace")).toBe(true) }) }) + + describe("taskHistory write lock (mutex)", () => { + it("serializes concurrent updateTaskHistory calls so no entries are lost", async () => { + await provider.resolveWebviewView(mockWebviewView) + + // Fire 5 concurrent updateTaskHistory calls + const items = Array.from({ length: 5 }, (_, i) => + createHistoryItem({ id: `concurrent-${i}`, task: `Task ${i}` }), + ) + + await Promise.all(items.map((item) => provider.updateTaskHistory(item, { broadcast: false }))) + + // All 5 entries must survive + const history = (provider as any).contextProxy.getGlobalState("taskHistory") as HistoryItem[] + const ids = history.map((h: HistoryItem) => h.id) + for (const item of items) { + expect(ids).toContain(item.id) + } + expect(history.length).toBe(5) + }) + + it("serializes concurrent update and deleteTaskFromState so they don't corrupt each other", async () => { + await provider.resolveWebviewView(mockWebviewView) + + // Seed with two items + const keep = createHistoryItem({ id: "keep-me", task: "Keep" }) + const remove = createHistoryItem({ id: "remove-me", task: "Remove" }) + await provider.updateTaskHistory(keep, { broadcast: false }) + await provider.updateTaskHistory(remove, { broadcast: false }) + + // Concurrently: add a new item AND delete "remove-me" + const newItem = createHistoryItem({ id: "new-item", task: "New" }) + await Promise.all([ + provider.updateTaskHistory(newItem, { broadcast: false }), + provider.deleteTaskFromState("remove-me"), + ]) + + const history = (provider as any).contextProxy.getGlobalState("taskHistory") as HistoryItem[] + const ids = history.map((h: HistoryItem) => h.id) + expect(ids).toContain("keep-me") + expect(ids).toContain("new-item") + expect(ids).not.toContain("remove-me") + }) + + it("does not block subsequent writes when a previous write errors", async () => { + await provider.resolveWebviewView(mockWebviewView) + + // Temporarily make updateGlobalState throw + const origUpdateGlobalState = (provider as any).updateGlobalState.bind(provider) + let callCount = 0 + ;(provider as any).updateGlobalState = vi.fn().mockImplementation((...args: unknown[]) => { + callCount++ + if (callCount === 1) { + return Promise.reject(new Error("simulated write failure")) + } + return origUpdateGlobalState(...args) + }) + + // First call should fail + const item1 = createHistoryItem({ id: "fail-item", task: "Fail" }) + await expect(provider.updateTaskHistory(item1, { broadcast: false })).rejects.toThrow( + "simulated write failure", + ) + + // Second call should still succeed (lock not stuck) + const item2 = createHistoryItem({ id: "ok-item", task: "OK" }) + const result = await provider.updateTaskHistory(item2, { broadcast: false }) + expect(result.some((h) => h.id === "ok-item")).toBe(true) + }) + + it("serializes concurrent updates to the same item preserving the last write", async () => { + await provider.resolveWebviewView(mockWebviewView) + + const base = createHistoryItem({ id: "race-item", task: "Original" }) + await provider.updateTaskHistory(base, { broadcast: false }) + + // Fire two concurrent updates to the same item + await Promise.all([ + provider.updateTaskHistory(createHistoryItem({ id: "race-item", task: "Original", tokensIn: 111 }), { + broadcast: false, + }), + provider.updateTaskHistory(createHistoryItem({ id: "race-item", task: "Original", tokensIn: 222 }), { + broadcast: false, + }), + ]) + + const history = (provider as any).contextProxy.getGlobalState("taskHistory") as HistoryItem[] + const item = history.find((h: HistoryItem) => h.id === "race-item") + expect(item).toBeDefined() + // The second write (tokensIn: 222) should be the last one since writes are serialized + expect(item!.tokensIn).toBe(222) + }) + }) })