fix(task-persistence): add cross-instance lock for concurrent task history updates (#920)

src/__tests__/history-resume-delegation.spec.ts

@@ -39,6 +39,12 @@ vi.mock("../core/task-persistence", async (importOriginal) => {
 		saveTaskMessages: vi.fn().mockResolvedValue(undefined),
 	}
 })
+vi.mock("../core/task-persistence/TaskHistoryLock", () => ({
+	taskHistoryLock: {
+		withLock: vi.fn(async (_globalStoragePath: string, fn: () => Promise<unknown>) => fn()),
+		reset: vi.fn(),
+	},
+}))
 
 import { ClineProvider } from "../core/webview/ClineProvider"
 import { readTaskMessages } from "../core/task-persistence/taskMessages"
@@ -923,6 +929,72 @@ describe("History resume delegation - parent metadata transitions", () => {
 		expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting"))
 	})
 
+	it("reopenParentFromDelegation revalidates the locked parent snapshot before committing completion metadata", async () => {
+		const logSpy = vi.fn()
+		const saveTaskMessagesMock = vi.mocked(saveTaskMessages)
+		const saveApiMessagesMock = vi.mocked(saveApiMessages)
+		const createTaskWithHistoryItem = vi.fn()
+		const parentItem = {
+			id: "parent-locked-guard",
+			status: "delegated",
+			awaitingChildId: "child-locked-guard",
+			childIds: ["child-locked-guard"],
+			ts: 100,
+			task: "Parent locked guard",
+			tokensIn: 0,
+			tokensOut: 0,
+			totalCost: 0,
+		}
+		const childItem = {
+			id: "child-locked-guard",
+			status: "active",
+			ts: 101,
+			task: "Child locked guard",
+			tokensIn: 0,
+			tokensOut: 0,
+			totalCost: 0,
+		}
+		const atomicUpdatePair = vi.fn(
+			async (
+				_firstId: string,
+				_secondId: string,
+				firstUpdater: (h: HistoryItem) => HistoryItem,
+				secondUpdater: (h: HistoryItem) => HistoryItem,
+			) => {
+				firstUpdater(childItem as HistoryItem)
+				secondUpdater({ ...parentItem, status: "active", awaitingChildId: undefined } as HistoryItem)
+				return []
+			},
+		)
+
+		const provider = makeProviderStub({
+			contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
+			getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }),
+			emit: vi.fn(),
+			log: logSpy,
+			getCurrentTask: vi.fn(() => null),
+			removeClineFromStack: vi.fn(),
+			createTaskWithHistoryItem,
+			taskHistoryStore: { atomicUpdatePair, get: vi.fn() },
+		} as any)
+		vi.mocked(readTaskMessages).mockResolvedValue([])
+		vi.mocked(readApiMessages).mockResolvedValue([])
+
+		await expect(
+			(ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, {
+				parentTaskId: "parent-locked-guard",
+				childTaskId: "child-locked-guard",
+				completionResultSummary: "should not commit",
+			}),
+		).resolves.toBe(false)
+
+		expect(atomicUpdatePair).toHaveBeenCalled()
+		expect(createTaskWithHistoryItem).not.toHaveBeenCalled()
+		expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting"))
+		expect(saveTaskMessagesMock).toHaveBeenCalled()
+		expect(saveApiMessagesMock).toHaveBeenCalled()
+	})
+
 	it("serializes delegation transitions and continues after a rejected predecessor", async () => {
 		const provider = makeProviderStub({} as any) as any
 		const calls: string[] = []

src/__tests__/provider-delegation.spec.ts

@@ -3,6 +3,13 @@
 import { describe, it, expect, vi } from "vitest"
 import type { HistoryItem } from "@roo-code/types"
 import { RooCodeEventName } from "@roo-code/types"
+vi.mock("../core/task-persistence/TaskHistoryLock", () => ({
+	taskHistoryLock: {
+		withLock: vi.fn(async (_globalStoragePath: string, fn: () => Promise<unknown>) => fn()),
+		reset: vi.fn(),
+	},
+}))
+
 import { ClineProvider } from "../core/webview/ClineProvider"
 
 const parentHistoryItem: HistoryItem = {
@@ -53,6 +60,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
 		const taskHistoryStore = makeStoreStub()
 
 		const provider = {
+			contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
 			emit: providerEmit,
 			getCurrentTask: vi.fn(() => parentTask),
 			removeClineFromStack,
@@ -117,6 +125,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
 		})
 
 		const provider = {
+			contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
 			emit: vi.fn(),
 			getCurrentTask: vi.fn(() => parentTask),
 			removeClineFromStack: vi.fn().mockResolvedValue(undefined),
@@ -150,6 +159,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
 		})
 
 		const provider = {
+			contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
 			emit: vi.fn(),
 			getCurrentTask: vi.fn(() => parentTask),
 			removeClineFromStack: vi.fn().mockResolvedValue(undefined),
@@ -191,6 +201,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
 		})
 
 		const provider = {
+			contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
 			emit: vi.fn(),
 			getCurrentTask: vi.fn(() => parentTask),
 			removeClineFromStack,
@@ -236,6 +247,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => {
 		})
 
 		const provider = {
+			contextProxy: { globalStorageUri: { fsPath: "/tmp" } },
 			emit: vi.fn(),
 			getCurrentTask,
 			removeClineFromStack,

src/core/task-persistence/TaskHistoryLock.ts

@@ -0,0 +1,91 @@
+import * as fs from "fs/promises"
+import * as path from "path"
+import * as lockfile from "proper-lockfile"
+
+import { GlobalFileNames } from "../../shared/globalFileNames"
+import { getStorageBasePath } from "../../utils/storage"
+
+/**
+ * Cross-process lock for task history mutations.
+ *
+ * Multiple `ClineProvider` instances may live in separate extension-host
+ * processes (for example VS Code windows) while sharing the same task history
+ * storage. Each process has its own `TaskHistoryStore`, so an in-memory mutex is
+ * not sufficient. This lock serializes mutations by taking an exclusive advisory
+ * lock on the shared `tasks/_history.lock` file.
+ */
+export class TaskHistoryLock {
+	private queue: Promise<unknown> = Promise.resolve()
+
+	/**
+	 * Acquires the shared task-history lock and executes `fn` while holding it.
+	 *
+	 * The lock file is scoped to the effective storage root (including custom
+	 * storage path resolution) so all windows/processes targeting the same history
+	 * store contend on the same file.
+	 */
+	async withLock<T>(globalStoragePath: string, fn: () => Promise<T>): Promise<T> {
+		const result = this.queue.then(
+			async () => {
+				const lockFilePath = await this.getLockFilePath(globalStoragePath)
+				return this.runWithFileLock(lockFilePath, fn)
+			},
+			async () => {
+				const lockFilePath = await this.getLockFilePath(globalStoragePath)
+				return this.runWithFileLock(lockFilePath, fn)
+			},
+		)
+
+		this.queue = result.then(
+			() => undefined,
+			() => undefined,
+		)
+
+		return result
+	}
+
+	/**
+	 * Clears in-process queues. File locks held by other processes are not affected.
+	 */
+	reset(): void {
+		this.queue = Promise.resolve()
+	}
+
+	async getLockFilePath(globalStoragePath: string): Promise<string> {
+		const basePath = await getStorageBasePath(globalStoragePath)
+		const tasksDir = path.join(basePath, "tasks")
+		await fs.mkdir(tasksDir, { recursive: true })
+		return path.join(tasksDir, GlobalFileNames.historyLock)
+	}
+
+	private async runWithFileLock<T>(lockFilePath: string, fn: () => Promise<T>): Promise<T> {
+		let releaseLock: (() => Promise<void>) | undefined
+
+		try {
+			releaseLock = await lockfile.lock(lockFilePath, {
+				stale: 31000,
+				update: 10000,
+				realpath: false,
+				retries: {
+					retries: 36,
+					factor: 1,
+					minTimeout: 1000,
+					maxTimeout: 1000,
+				},
+				onCompromised: (err) => {
+					console.error(`[TaskHistoryLock] Lock at ${lockFilePath} was compromised:`, err)
+					throw err
+				},
+			})
+
+			return await fn()
+		} finally {
+			if (releaseLock) {
+				await releaseLock()
+			}
+		}
+	}
+}
+
+// Singleton instance shared across all ClineProvider instances in this process.
+export const taskHistoryLock = new TaskHistoryLock()