From 033366004eb40f7c193ed111f2dae0621e9725bf Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Mon, 22 Jun 2026 19:53:48 -0400 Subject: [PATCH 1/3] fix(delegation): serialize delegateParentAndOpenChild read-modify-write atomically --- src/__tests__/delegation-concurrent.spec.ts | 143 +++++++++++ src/__tests__/provider-delegation.spec.ts | 222 +++++++++++------- src/core/task-persistence/TaskHistoryStore.ts | 62 +++-- src/core/webview/ClineProvider.ts | 29 ++- 4 files changed, 344 insertions(+), 112 deletions(-) create mode 100644 src/__tests__/delegation-concurrent.spec.ts diff --git a/src/__tests__/delegation-concurrent.spec.ts b/src/__tests__/delegation-concurrent.spec.ts new file mode 100644 index 0000000000..7c07cae1b6 --- /dev/null +++ b/src/__tests__/delegation-concurrent.spec.ts @@ -0,0 +1,143 @@ +// npx vitest run __tests__/delegation-concurrent.spec.ts + +import { describe, it, expect, vi, beforeEach } from "vitest" +import type { HistoryItem } from "@roo-code/types" + +vi.mock("fs/promises", () => ({ + mkdir: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })), + readdir: vi.fn().mockResolvedValue([]), + unlink: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("fs", () => ({ + default: { + watch: vi.fn().mockReturnValue({ on: vi.fn(), close: vi.fn() }), + existsSync: vi.fn().mockReturnValue(false), + }, + watch: vi.fn().mockReturnValue({ on: vi.fn(), close: vi.fn() }), + existsSync: vi.fn().mockReturnValue(false), +})) + +vi.mock("../utils/safeWriteJson", () => ({ + safeWriteJson: vi.fn().mockResolvedValue(undefined), +})) + +vi.mock("../utils/storage", () => ({ + getStorageBasePath: vi.fn().mockResolvedValue("/tmp/test-storage"), +})) + +import { TaskHistoryStore } from "../core/task-persistence/TaskHistoryStore" + +const makeItem = (id: string, overrides: Partial = {}): HistoryItem => + ({ + id, + ts: Date.now(), + task: "test task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + status: "active", + mode: "code", + workspace: "/tmp", + ...overrides, + }) as HistoryItem + +describe("TaskHistoryStore.atomicReadAndUpdate", () => { + let store: TaskHistoryStore + + beforeEach(() => { + vi.clearAllMocks() + store = new TaskHistoryStore("/tmp/test-storage") + }) + + it("serializes concurrent operations — second caller reads the state written by the first", async () => { + // Seed the cache with an item that has no childIds yet. + const item = makeItem("parent-task", { childIds: [] }) + ;(store as any).cache.set(item.id, item) + + // Two concurrent delegations each append their child ID. + // Because they are serialized by the lock, the second caller must + // read the cache state that the first caller wrote — not the original. + const delegation1 = store.atomicReadAndUpdate("parent-task", (current) => ({ + ...current, + childIds: [...(current.childIds ?? []), "child-A"], + })) + + const delegation2 = store.atomicReadAndUpdate("parent-task", (current) => ({ + ...current, + childIds: [...(current.childIds ?? []), "child-B"], + })) + + await Promise.all([delegation1, delegation2]) + + // Both child IDs must be present: delegation2 saw delegation1's write. + const final = (store as any).cache.get("parent-task") as HistoryItem + expect(final.childIds).toContain("child-A") + expect(final.childIds).toContain("child-B") + }) + + it("two concurrent delegations produce consistent HistoryItem state (full field set)", async () => { + const item = makeItem("parent-task", { status: "active", childIds: [] }) + ;(store as any).cache.set(item.id, item) + + // Each delegation sets awaitingChildId and appends to childIds. + const delegation1 = store.atomicReadAndUpdate("parent-task", (historyItem) => { + const childIds = Array.from(new Set([...(historyItem.childIds ?? []), "child-A"])) + return { + ...historyItem, + status: "delegated", + delegatedToId: "child-A", + awaitingChildId: "child-A", + childIds, + } + }) + + const delegation2 = store.atomicReadAndUpdate("parent-task", (historyItem) => { + const childIds = Array.from(new Set([...(historyItem.childIds ?? []), "child-B"])) + return { + ...historyItem, + status: "delegated", + delegatedToId: "child-B", + awaitingChildId: "child-B", + childIds, + } + }) + + await Promise.all([delegation1, delegation2]) + + const final = (store as any).cache.get("parent-task") as HistoryItem + // Both child IDs present — neither write clobbered the other's childIds. + expect(final.childIds).toContain("child-A") + expect(final.childIds).toContain("child-B") + expect(["active", "delegated"]).toContain(final.status) + }) + + it("completes without deadlock — updater is pure and does not re-acquire the lock", async () => { + const item = makeItem("task-1", { childIds: [] }) + ;(store as any).cache.set(item.id, item) + + // With the typed (taskId, updater) API, the updater is synchronous and + // cannot call upsert/withLock — no re-entrancy, no deadlock. + const result = await Promise.race([ + store + .atomicReadAndUpdate("task-1", (current) => ({ + ...current, + childIds: [...(current.childIds ?? []), "child-1"], + })) + .then(() => "completed"), + new Promise((resolve) => setTimeout(() => resolve("deadlocked"), 100)), + ]) + + expect(result).toBe("completed") + + const final = (store as any).cache.get("task-1") as HistoryItem + expect(final.childIds).toContain("child-1") + }) + + it("throws if the task ID is not in the cache", async () => { + await expect( + store.atomicReadAndUpdate("nonexistent-task", (current) => ({ ...current, status: "delegated" })), + ).rejects.toThrow("nonexistent-task") + }) +}) diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index bd4519b970..fa427af44b 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -1,127 +1,180 @@ // npx vitest run __tests__/provider-delegation.spec.ts import { describe, it, expect, vi } from "vitest" +import type { HistoryItem } from "@roo-code/types" import { RooCodeEventName } from "@roo-code/types" import { ClineProvider } from "../core/webview/ClineProvider" +const parentHistoryItem: HistoryItem = { + id: "parent-1", + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + childIds: [], +} as unknown as HistoryItem + +/** Minimal taskHistoryStore stub whose atomicReadAndUpdate calls the updater with the parent item. */ +function makeStoreStub( + overrides: Partial<{ atomicReadAndUpdate: ReturnType; get: ReturnType }> = {}, +) { + return { + atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (h: HistoryItem) => HistoryItem) => { + updater(parentHistoryItem) + return [] + }), + get: vi.fn().mockReturnValue(undefined), + ...overrides, + } +} + describe("ClineProvider.delegateParentAndOpenChild()", () => { - it("persists parent delegation metadata and emits TaskDelegated", async () => { + it("persists parent delegation metadata via atomicReadAndUpdate and emits TaskDelegated", async () => { 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", start: childStart }) const handleModeSwitch = vi.fn().mockResolvedValue(undefined) - const getTaskWithId = vi.fn().mockImplementation(async (id: string) => { - if (id === "parent-1") { - return { - historyItem: { - id: "parent-1", - task: "Parent", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - childIds: [], - }, - } - } - // child-1 - return { - historyItem: { - id: "child-1", - task: "Do something", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - }, - } - }) + const taskHistoryStore = makeStoreStub() const provider = { emit: providerEmit, getCurrentTask: vi.fn(() => parentTask), removeClineFromStack, createTask, - getTaskWithId, - updateTaskHistory, handleModeSwitch, log: vi.fn(), + isViewLaunched: false, + recentTasksCache: undefined, + taskHistoryStore, } as unknown as ClineProvider - const params = { + const child = await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { parentTaskId: "parent-1", message: "Do something", initialTodos: [], mode: "code", - } - - const child = await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, params) + }) expect(child.taskId).toBe("child-1") // Invariant: parent closed before child creation expect(removeClineFromStack).toHaveBeenCalledTimes(1) - // Child task is created with startTask: false and initialStatus: "active" + + // Child task 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) - expect(updateTaskHistory).toHaveBeenCalledTimes(1) - - // Parent set to "delegated" - const parentSaved = updateTaskHistory.mock.calls[0][0] - expect(parentSaved).toEqual( - expect.objectContaining({ - id: "parent-1", - status: "delegated", - delegatedToId: "child-1", - awaitingChildId: "child-1", - childIds: expect.arrayContaining(["child-1"]), - }), - ) + // Delegation metadata written via atomicReadAndUpdate with correct taskId + expect(taskHistoryStore.atomicReadAndUpdate).toHaveBeenCalledTimes(1) + const [calledTaskId, updater] = taskHistoryStore.atomicReadAndUpdate.mock.calls[0] + expect(calledTaskId).toBe("parent-1") - // child.start() must be called AFTER parent metadata is persisted + // The updater must produce the correct delegation fields + const result = updater(parentHistoryItem) + expect(result).toMatchObject({ + id: "parent-1", + status: "delegated", + delegatedToId: "child-1", + awaitingChildId: "child-1", + childIds: expect.arrayContaining(["child-1"]), + }) + + // child.start() called AFTER parent metadata is persisted expect(childStart).toHaveBeenCalledTimes(1) - // Event emission (provider-level) + // Provider-level event 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[] = [] + it("posts taskHistoryItemUpdated to the webview when isViewLaunched is true", async () => { + const updatedParent = { ...parentHistoryItem, status: "delegated" } as HistoryItem + const postMessageToWebview = vi.fn().mockResolvedValue(undefined) + const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const taskHistoryStore = makeStoreStub({ + get: vi.fn().mockReturnValue(updatedParent), + }) - const parentTask = { - taskId: "parent-1", + const provider = { emit: vi.fn(), - } as any - const childStart = vi.fn(() => callOrder.push("child.start")) + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: vi.fn() }), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + postMessageToWebview, + log: vi.fn(), + isViewLaunched: true, + recentTasksCache: undefined, + taskHistoryStore, + } as unknown as ClineProvider + + await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + }) + + expect(postMessageToWebview).toHaveBeenCalledWith({ + type: "taskHistoryItemUpdated", + taskHistoryItem: updatedParent, + }) + }) + + it("skips postMessageToWebview when isViewLaunched is true but store returns undefined", async () => { + const postMessageToWebview = vi.fn().mockResolvedValue(undefined) + const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const taskHistoryStore = makeStoreStub({ + get: vi.fn().mockReturnValue(undefined), + }) - const updateTaskHistory = vi.fn(async () => { - callOrder.push("updateTaskHistory") + const provider = { + emit: vi.fn(), + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: vi.fn() }), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + postMessageToWebview, + log: vi.fn(), + isViewLaunched: true, + recentTasksCache: undefined, + taskHistoryStore, + } as unknown as ClineProvider + + await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", }) + + expect(postMessageToWebview).not.toHaveBeenCalled() + }) + + it("calls child.start() only after atomicReadAndUpdate completes (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 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 taskHistoryStore = makeStoreStub({ + atomicReadAndUpdate: vi.fn(async (_taskId: string, _updater: (h: HistoryItem) => HistoryItem) => { + callOrder.push("atomicReadAndUpdate") + return [] + }), }) const provider = { @@ -129,10 +182,11 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { getCurrentTask: vi.fn(() => parentTask), removeClineFromStack, createTask, - getTaskWithId, - updateTaskHistory, handleModeSwitch, log: vi.fn(), + isViewLaunched: false, + recentTasksCache: undefined, + taskHistoryStore, } as unknown as ClineProvider await (ClineProvider.prototype as any).delegateParentAndOpenChild.call(provider, { @@ -142,40 +196,36 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { mode: "code", }) - // Verify ordering: createTask → updateTaskHistory → child.start - expect(callOrder).toEqual(["createTask", "updateTaskHistory", "child.start"]) + // createTask → atomicReadAndUpdate → child.start: lock must release before start + expect(callOrder).toEqual(["createTask", "atomicReadAndUpdate", "child.start"]) }) - it("rolls back the paused child and restores the parent when metadata persistence fails", async () => { + it("rolls back the paused child and restores the parent when atomicReadAndUpdate fails", async () => { const persistError = new Error("parent metadata persist failed") - const parentHistory = { - id: "parent-1", - task: "Parent", - tokensIn: 0, - tokensOut: 0, - totalCost: 0, - mode: "code", - } - const parentTask = { - taskId: "parent-1", - emit: vi.fn(), - } as any + const parentTask = { taskId: "parent-1", emit: vi.fn() } as any const childStart = vi.fn() const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const deleteTaskWithId = vi.fn().mockResolvedValue(undefined) const createTaskWithHistoryItem = vi.fn().mockResolvedValue(undefined) + const getTaskWithId = vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }) + + const taskHistoryStore = makeStoreStub({ + atomicReadAndUpdate: vi.fn().mockRejectedValue(persistError), + }) const provider = { emit: vi.fn(), getCurrentTask: vi.fn(() => parentTask), removeClineFromStack, createTask: vi.fn().mockResolvedValue({ taskId: "child-1", start: childStart }), - getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistory }), - updateTaskHistory: vi.fn().mockRejectedValue(persistError), + getTaskWithId, handleModeSwitch: vi.fn().mockResolvedValue(undefined), deleteTaskWithId, createTaskWithHistoryItem, log: vi.fn(), + isViewLaunched: false, + recentTasksCache: undefined, + taskHistoryStore, } as unknown as ClineProvider await expect( @@ -191,6 +241,6 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { skipDelegationRepair: true }) expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { skipDelegationRepair: true }) expect(deleteTaskWithId).toHaveBeenCalledWith("child-1", false) - expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistory) + expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistoryItem) }) }) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 4157d8b9fb..c6142b5051 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -158,30 +158,36 @@ export class TaskHistoryStore { * updates the in-memory Map, and schedules a debounced index write. */ async upsert(item: HistoryItem): Promise { - return this.withLock(async () => { - const existing = this.cache.get(item.id) + return this.withLock(() => this._upsertUnlocked(item)) + } - // Merge: preserve existing metadata unless explicitly overwritten - const merged = existing ? { ...existing, ...item } : item + /** + * Upsert body executed without acquiring the lock. + * Must only be called from within a `withLock` callback. + */ + private async _upsertUnlocked(item: HistoryItem): Promise { + const existing = this.cache.get(item.id) - // Write per-task file (source of truth) - await this.writeTaskFile(merged) + // Merge: preserve existing metadata unless explicitly overwritten + const merged = existing ? { ...existing, ...item } : item - // Update in-memory cache - this.cache.set(merged.id, merged) + // Write per-task file (source of truth) + await this.writeTaskFile(merged) - // Schedule debounced index write - this.scheduleIndexWrite() + // Update in-memory cache + this.cache.set(merged.id, merged) - const all = this.getAll() + // Schedule debounced index write + this.scheduleIndexWrite() - // Call onWrite callback inside the lock for serialized write-through - if (this.onWrite) { - await this.onWrite(all) - } + const all = this.getAll() - return all - }) + // Call onWrite callback inside the lock for serialized write-through + if (this.onWrite) { + await this.onWrite(all) + } + + return all } /** @@ -529,6 +535,28 @@ export class TaskHistoryStore { }, TaskHistoryStore.RECONCILE_INTERVAL_MS) } + // ────────────────────────────── Atomic read-modify-write ────────────────────────────── + + /** + * Read a HistoryItem from the in-memory cache and write back an updated version, + * all within a single lock acquisition so no concurrent writer can interleave + * between the read and the write. + * + * The `updater` receives the current cached item and must return the new item + * synchronously. It must not perform I/O or acquire any other lock. + * + * @throws If the task ID is not present in the cache. + */ + public atomicReadAndUpdate(taskId: string, updater: (current: HistoryItem) => HistoryItem): Promise { + return this.withLock(async () => { + const current = this.cache.get(taskId) + if (!current) { + throw new Error(`[TaskHistoryStore] atomicReadAndUpdate: task ${taskId} not found in cache`) + } + return this._upsertUnlocked(updater(current)) + }) + } + // ────────────────────────────── Private: Write lock ────────────────────────────── /** diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index b2bb73e982..b03fac9015 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -3491,17 +3491,28 @@ export class ClineProvider }) // 5) Persist parent delegation metadata BEFORE the child starts writing. + // atomicReadAndUpdate reads from the in-memory cache and writes back within a + // single lock acquisition — no concurrent writer can slip between the read and + // write, and the pure updater cannot re-enter the lock (no deadlock). + // Broadcast and cache invalidation happen outside the lock after it releases. try { - const { historyItem } = await this.getTaskWithId(parentTaskId) - const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId])) - const updatedHistory: typeof historyItem = { - ...historyItem, - status: "delegated", - delegatedToId: child.taskId, - awaitingChildId: child.taskId, - childIds, + await this.taskHistoryStore.atomicReadAndUpdate(parentTaskId, (historyItem) => { + const childIds = Array.from(new Set([...(historyItem.childIds ?? []), child.taskId])) + return { + ...historyItem, + status: "delegated", + delegatedToId: child.taskId, + awaitingChildId: child.taskId, + childIds, + } + }) + this.recentTasksCache = undefined + if (this.isViewLaunched) { + const updatedItem = this.taskHistoryStore.get(parentTaskId) + if (updatedItem) { + await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedItem }) + } } - await this.updateTaskHistory(updatedHistory) } catch (err) { this.log( `[delegateParentAndOpenChild] Failed to persist parent metadata for ${parentTaskId} -> ${child.taskId}: ${ From 92801976496b187cd4edd1cff4d331bc60e9160e Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Tue, 23 Jun 2026 09:37:31 -0400 Subject: [PATCH 2/3] refactor(TaskHistoryStore): coderabbit feedback --- src/__tests__/delegation-concurrent.spec.ts | 6 ++++- src/__tests__/provider-delegation.spec.ts | 23 +++++++++++++++---- src/core/task-persistence/TaskHistoryStore.ts | 10 +++++++- 3 files changed, 32 insertions(+), 7 deletions(-) diff --git a/src/__tests__/delegation-concurrent.spec.ts b/src/__tests__/delegation-concurrent.spec.ts index 7c07cae1b6..40d9b49ee5 100644 --- a/src/__tests__/delegation-concurrent.spec.ts +++ b/src/__tests__/delegation-concurrent.spec.ts @@ -110,7 +110,11 @@ describe("TaskHistoryStore.atomicReadAndUpdate", () => { // Both child IDs present — neither write clobbered the other's childIds. expect(final.childIds).toContain("child-A") expect(final.childIds).toContain("child-B") - expect(["active", "delegated"]).toContain(final.status) + // The last writer wins on scalar fields; whichever child ran second is authoritative. + expect(final.status).toBe("delegated") + expect(["child-A", "child-B"]).toContain(final.awaitingChildId) + expect(final.delegatedToId).toBe(final.awaitingChildId) + expect(final.childIds).toContain(final.awaitingChildId) }) it("completes without deadlock — updater is pure and does not re-acquire the lock", async () => { diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index fa427af44b..6c0eeef3c5 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -28,10 +28,23 @@ function makeStoreStub( } } +/** + * Parent task double with the methods delegateParentAndOpenChild reads from + * `parent`. Without flushPendingToolResultsToHistory the method hits its + * non-fatal flush-error branch and never reaches the happy delegation path. + */ +const makeParentTask = () => + ({ + taskId: "parent-1", + emit: vi.fn(), + flushPendingToolResultsToHistory: vi.fn().mockResolvedValue(true), + retrySaveApiConversationHistory: vi.fn(), + }) as any + describe("ClineProvider.delegateParentAndOpenChild()", () => { it("persists parent delegation metadata via atomicReadAndUpdate and emits TaskDelegated", async () => { const providerEmit = vi.fn() - const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const parentTask = makeParentTask() const childStart = vi.fn() const removeClineFromStack = vi.fn().mockResolvedValue(undefined) @@ -98,7 +111,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { it("posts taskHistoryItemUpdated to the webview when isViewLaunched is true", async () => { const updatedParent = { ...parentHistoryItem, status: "delegated" } as HistoryItem const postMessageToWebview = vi.fn().mockResolvedValue(undefined) - const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const parentTask = makeParentTask() const taskHistoryStore = makeStoreStub({ get: vi.fn().mockReturnValue(updatedParent), }) @@ -131,7 +144,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { it("skips postMessageToWebview when isViewLaunched is true but store returns undefined", async () => { const postMessageToWebview = vi.fn().mockResolvedValue(undefined) - const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const parentTask = makeParentTask() const taskHistoryStore = makeStoreStub({ get: vi.fn().mockReturnValue(undefined), }) @@ -162,7 +175,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { it("calls child.start() only after atomicReadAndUpdate completes (no race condition)", async () => { const callOrder: string[] = [] - const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const parentTask = makeParentTask() const childStart = vi.fn(() => callOrder.push("child.start")) const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const createTask = vi.fn(async () => { @@ -202,7 +215,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { it("rolls back the paused child and restores the parent when atomicReadAndUpdate fails", async () => { const persistError = new Error("parent metadata persist failed") - const parentTask = { taskId: "parent-1", emit: vi.fn() } as any + const parentTask = makeParentTask() const childStart = vi.fn() const removeClineFromStack = vi.fn().mockResolvedValue(undefined) const deleteTaskWithId = vi.fn().mockResolvedValue(undefined) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index c6142b5051..9d056f81f5 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -553,7 +553,15 @@ export class TaskHistoryStore { if (!current) { throw new Error(`[TaskHistoryStore] atomicReadAndUpdate: task ${taskId} not found in cache`) } - return this._upsertUnlocked(updater(current)) + // Deep-copy so a mutating updater cannot alter cached state before persistence. + const snapshot = JSON.parse(JSON.stringify(current)) as HistoryItem + const updated = updater(snapshot) + if (updated.id !== taskId) { + throw new Error( + `[TaskHistoryStore] atomicReadAndUpdate: updater changed task id from ${taskId} to ${updated.id}`, + ) + } + return this._upsertUnlocked(updated) }) } From c8477f41ffc10fdd9a64f5ca68c2f2485d300a27 Mon Sep 17 00:00:00 2001 From: Elliott de Launay Date: Thu, 25 Jun 2026 13:13:35 +0000 Subject: [PATCH 3/3] refactor(TaskHistoryStore): use structuredClone instead of hacky JSON.stringigy and parse methods --- src/core/task-persistence/TaskHistoryStore.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 9d056f81f5..a65d0e866c 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -554,7 +554,7 @@ export class TaskHistoryStore { throw new Error(`[TaskHistoryStore] atomicReadAndUpdate: task ${taskId} not found in cache`) } // Deep-copy so a mutating updater cannot alter cached state before persistence. - const snapshot = JSON.parse(JSON.stringify(current)) as HistoryItem + const snapshot = structuredClone(current) const updated = updater(snapshot) if (updated.id !== taskId) { throw new Error(