diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index e4707ee0a9..075d21474f 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -7,12 +7,23 @@ import deepEqual from "fast-deep-equal" import type { HistoryItem } from "@roo-code/types" import { GlobalFileNames } from "../../shared/globalFileNames" -import { safeWriteJson } from "../../utils/safeWriteJson" +import { LOCK_STALE_MS, safeWriteJson } from "../../utils/safeWriteJson" import { getStorageBasePath } from "../../utils/storage" /** Valid status values for a task's HistoryItem. */ export type HistoryItemStatus = NonNullable +export class DeltaRejectedError extends Error { + constructor( + public readonly taskId: string, + public readonly diskStatus: HistoryItemStatus, + public readonly attemptedStatus: HistoryItemStatus, + ) { + super(`Delta rejected for task ${taskId}: disk status ${diskStatus} rejects transition to ${attemptedStatus}`) + this.name = "DeltaRejectedError" + } +} + const VALID_TRANSITIONS: Record = { active: ["delegated", "completed", "interrupted"], delegated: ["active"], @@ -34,12 +45,30 @@ export function assertValidTransition(from: HistoryItemStatus | undefined, to: H } /** - * Index file format for fast startup reads. + * Build a `safeWriteJson` merge callback that applies only `delta` to the + * current disk state, preserving fields written by another process. */ -interface HistoryIndex { - version: number - updatedAt: number - entries: HistoryItem[] +function mergeWithDisk(delta: Partial): (existing: unknown, incoming: unknown) => unknown { + return (existing, incoming) => { + if (!existing || typeof existing !== "object" || !("id" in existing)) { + return incoming + } + const disk = existing as HistoryItem + if (delta.status !== undefined) { + const diskStatus: HistoryItemStatus = disk.status ?? "active" + if (delta.status !== diskStatus) { + const validTargets = VALID_TRANSITIONS[diskStatus] + if (!validTargets?.includes(delta.status as HistoryItemStatus)) { + throw new DeltaRejectedError(disk.id, diskStatus, delta.status as HistoryItemStatus) + } + } + } + const merged = { ...disk, ...delta } + if (delta.childIds && disk.childIds) { + merged.childIds = [...new Set([...disk.childIds, ...delta.childIds])] + } + return merged + } } /** @@ -75,12 +104,14 @@ interface DelegationRepairIntent { * * Each task's HistoryItem is stored as an individual JSON file in its * existing task directory (`globalStorage/tasks//history_item.json`). - * A single index file (`globalStorage/tasks/_index.json`) is maintained - * as a cache for fast list reads at startup. + * There is no shared index file. Reads scan the task directories. * - * Cross-process safety comes from `safeWriteJson`'s `proper-lockfile` - * on per-task file writes. Within a single extension host process, - * an in-process write lock serializes mutations. + * Cross-process safety for per-task files comes from `safeWriteJson`'s + * `proper-lockfile` with a `merge` callback: each write reads the + * current file under the advisory lock and merges incoming fields, so + * a concurrent writer's changes are preserved rather than silently + * dropped. Within a single extension host process, an in-process write + * lock serializes mutations. */ /** * Options for TaskHistoryStore constructor. @@ -100,7 +131,6 @@ export class TaskHistoryStore { private cache: Map = new Map() private taskFileMtimes: Map = new Map() private writeLock: Promise = Promise.resolve() - private indexWriteTimer: ReturnType | null = null private fsWatcher: fsSync.FSWatcher | null = null private reconcileTimer: ReturnType | null = null private disposed = false @@ -112,9 +142,6 @@ export class TaskHistoryStore { public readonly initialized: Promise private resolveInitialized!: () => void - /** Debounce window for index writes in milliseconds. */ - private static readonly INDEX_WRITE_DEBOUNCE_MS = 2000 - /** Periodic reconciliation interval in milliseconds. */ private static readonly RECONCILE_INTERVAL_MS = 5 * 60 * 1000 @@ -129,37 +156,34 @@ export class TaskHistoryStore { // ────────────────────────────── Lifecycle ────────────────────────────── /** - * Load index, reconcile if needed, start watchers. + * Scan task files, reconcile delegation state, start watchers. */ async initialize(): Promise { try { const tasksDir = await this.getTasksDir() await fs.mkdir(tasksDir, { recursive: true }) - // 1. Load existing index into the cache - await this.loadIndex() - - // 2. Reconcile cache against actual task directories on disk + // 1. Scan task directories to populate the cache await this.reconcile({ forceRefresh: true }) // Capture which active tasks were present in persisted state before replay can // change any statuses. Reconciliation must not treat a replay-repaired parent // as an orphaned active child in the same startup pass. const persistedActiveIds = this.getPersistedActiveIds() - // 3. Complete any two-record repair interrupted after its intent was durable. + // 2. Complete any two-record repair interrupted after its intent was durable. try { await this.replayDelegationRepairIntent() } catch (error) { console.error("[TaskHistoryStore] Failed to replay delegation repair intent:", error) } - // 4. Repair delegation inconsistencies left by a previous crash + // 3. Repair delegation inconsistencies left by a previous crash await this.reconcileDelegationState(persistedActiveIds) - // 5. Start fs.watch for cross-instance reactivity + // 4. Start fs.watch for cross-instance reactivity this.startWatcher() - // 6. Start periodic reconciliation as a defensive fallback + // 5. Start periodic reconciliation as a defensive fallback this.startPeriodicReconciliation() } finally { // Mark initialization as complete so callers awaiting `initialized` can proceed @@ -173,11 +197,6 @@ export class TaskHistoryStore { dispose(): void { this.disposed = true - if (this.indexWriteTimer) { - clearTimeout(this.indexWriteTimer) - this.indexWriteTimer = null - } - if (this.reconcileTimer) { clearTimeout(this.reconcileTimer) this.reconcileTimer = null @@ -187,11 +206,6 @@ export class TaskHistoryStore { this.fsWatcher.close() this.fsWatcher = null } - - // Synchronously flush the index (best-effort) - this.flushIndex().catch((err) => { - console.error("[TaskHistoryStore] Error flushing index on dispose:", err) - }) } // ────────────────────────────── Reads ────────────────────────────── @@ -222,8 +236,8 @@ export class TaskHistoryStore { /** * Insert or update a history item. * - * Writes the per-task file immediately (source of truth), - * updates the in-memory Map, and schedules a debounced index write. + * Writes the per-task file immediately (source of truth) + * and updates the in-memory cache. */ async upsert(item: HistoryItem): Promise { return this.withLock(() => this.upsertCore(item)) @@ -250,20 +264,40 @@ export class TaskHistoryStore { if (!options.skipTransitionCheck && existing && item.status !== undefined) { const normalizedExisting: HistoryItemStatus = existing.status ?? "active" if (item.status !== normalizedExisting) { - assertValidTransition(existing.status, item.status) + try { + assertValidTransition(existing.status, item.status) + } catch (cacheError) { + // Cache may be stale from a peer write. Re-read disk + // under the store lock before rejecting the transition. + const diskItem = await this.readTaskFile(item.id) + if (!diskItem) { + throw cacheError + } + assertValidTransition(diskItem.status, item.status) + } } } // Merge: preserve existing metadata unless explicitly overwritten const merged = existing ? { ...existing, ...item } : item - // Write per-task file (source of truth) - await this.writeTaskFile(merged) + const delta = existing ? this.buildDelta(item.id, existing, item) : { ...item } + let written: HistoryItem + try { + written = await this.writeTaskFile(merged, delta) + } catch (error) { + if (error instanceof DeltaRejectedError) { + const diskItem = await this.readTaskFile(item.id) + if (diskItem) { + this.cache.set(item.id, diskItem) + } + throw error + } + throw error + } - // Update in-memory cache - this.cache.set(merged.id, merged) - // Schedule debounced index write - this.scheduleIndexWrite() + // Update in-memory cache with what was actually persisted + this.cache.set(written.id, written) const all = this.getAll() @@ -291,8 +325,6 @@ export class TaskHistoryStore { // File may already be deleted } - this.scheduleIndexWrite() - // Call onWrite callback inside the lock for serialized write-through if (this.onWrite) { await this.onWrite(this.getAll()) @@ -317,8 +349,6 @@ export class TaskHistoryStore { } } - this.scheduleIndexWrite() - // Call onWrite callback inside the lock for serialized write-through if (this.onWrite) { await this.onWrite(this.getAll()) @@ -329,7 +359,7 @@ export class TaskHistoryStore { // ────────────────────────────── Reconciliation ────────────────────────────── /** - * Scan task directories vs index and fix any drift. + * Scan task directories and fix any drift between disk and cache. * * - Tasks on disk but missing from cache: read and add * - Tasks in cache but missing from disk: remove @@ -346,20 +376,18 @@ export class TaskHistoryStore { return // tasks dir doesn't exist yet } - // Filter out the index file and hidden files + // Filter out hidden and reserved names const taskDirNames = dirEntries.filter((name) => !name.startsWith("_") && !name.startsWith(".")) const onDiskIds = new Set(taskDirNames) const cacheIds = new Set(this.cache.keys()) - let changed = false + const liveIds = new Set() - // Task files are authoritative during startup. Later watcher and periodic - // reconciliations use mtime change detection to avoid rewriting the index when - // nothing changed on disk. for (const taskId of onDiskIds) { try { const taskFilePath = await this.getTaskFilePath(taskId) const { mtimeMs } = await fs.stat(taskFilePath) + liveIds.add(taskId) if ( !options.forceRefresh && this.cache.has(taskId) && @@ -369,31 +397,37 @@ export class TaskHistoryStore { } const item = await this.readTaskFile(taskId) - if (item) { + if (item?.id === taskId) { const previous = this.cache.get(taskId) this.taskFileMtimes.set(taskId, mtimeMs) if (!deepEqual(previous, item)) { this.cache.set(taskId, item) - changed = true } } } catch { - // Corrupted or missing file, skip + // File may be temporarily absent during a peer's atomic + // rename window in safeWriteJson. The advisory lock is + // held for the entire write, so its presence means a + // write is in progress — keep the task live. + try { + const lockPath = (await this.getTaskFilePath(taskId)) + ".lock" + const lockStat = await fs.stat(lockPath) + if (Date.now() - lockStat.mtimeMs < LOCK_STALE_MS) { + liveIds.add(taskId) + } + } catch { + // No lock file — file is genuinely absent + } } } - // Tasks in cache but not on disk: remove from cache + // Evict tasks whose history_item.json no longer exists for (const taskId of cacheIds) { - if (!onDiskIds.has(taskId)) { + if (!liveIds.has(taskId)) { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) - changed = true } } - - if (changed) { - this.scheduleIndexWrite() - } }) } @@ -428,7 +462,7 @@ export class TaskHistoryStore { * Reconcile delegation state while the store lock is already held. * * Callers that do not hold the lock must use `reconcileDelegationState()`. - * Migration uses this core method so its cache/file/index updates and the + * Migration uses this core method so its cache/file updates and the * follow-up repair remain one serialized operation without re-entering the * non-reentrant lock. */ @@ -584,12 +618,6 @@ export class TaskHistoryStore { await this.onWrite(this.getAll()) } await this.removeDelegationRepairIntent() - // Task files are authoritative and the intent is the recovery journal. - // Clean up the journal before scheduling the derived index: a crash after - // cleanup but before the index write is safe because startup rebuilds the - // index from task files, while the reverse ordering could make the index - // appear durable before recovery metadata is settled. - this.scheduleIndexWrite() }) } @@ -645,9 +673,6 @@ export class TaskHistoryStore { await this.onWrite(this.getAll()) } await this.removeDelegationRepairIntent() - // The index is derived state; keep the intent until authoritative task-file - // writes and write-through have completed, then schedule the index update. - this.scheduleIndexWrite() } private matchesDelegationRepairParentPreconditions(intent: DelegationRepairIntent, parent: HistoryItem): boolean { @@ -845,96 +870,52 @@ export class TaskHistoryStore { } } - // Write the index - await this.writeIndex() - // Repair any delegation inconsistencies introduced by the migrated entries. // Run the lock-free core because migration already holds the store lock. await this.reconcileDelegationStateCore(this.getPersistedActiveIds()) }) } - // ────────────────────────────── Private: Index management ────────────────────────────── - - /** - * Load the `_index.json` file into the in-memory cache. - */ - private async loadIndex(): Promise { - const indexPath = await this.getIndexPath() - - try { - const raw = await fs.readFile(indexPath, "utf8") - const index: HistoryIndex = JSON.parse(raw) - - if (index.version === 1 && Array.isArray(index.entries)) { - for (const entry of index.entries) { - if (entry.id) { - this.cache.set(entry.id, entry) - } - } - } - } catch { - // Index doesn't exist or is corrupted; cache stays empty. - // Reconciliation will rebuild it from per-task files. - } - } - - /** - * Write the full index to disk. - */ - private async writeIndex(): Promise { - const indexPath = await this.getIndexPath() - const index: HistoryIndex = { - version: 1, - updatedAt: Date.now(), - entries: this.getAll(), - } - - await safeWriteJson(indexPath, index) - } + // ────────────────────────────── Private: Per-task file I/O ────────────────────────────── /** - * Schedule a debounced index write. + * Return only the fields in `incoming` that differ from `cached`. */ - private scheduleIndexWrite(): void { - if (this.disposed) { - return - } - - if (this.indexWriteTimer) { - clearTimeout(this.indexWriteTimer) - } - - this.indexWriteTimer = setTimeout(async () => { - this.indexWriteTimer = null - try { - await this.writeIndex() - } catch (err) { - console.error("[TaskHistoryStore] Failed to write index:", err) - } - }, TaskHistoryStore.INDEX_WRITE_DEBOUNCE_MS) + private computeDelta(cached: HistoryItem, incoming: Partial): Partial { + return Object.fromEntries( + Object.entries(incoming).filter(([k, v]) => !deepEqual(v, (cached as Record)[k])), + ) as Partial } - /** - * Force an immediate index write (called on dispose/shutdown). - */ - async flushIndex(): Promise { - if (this.indexWriteTimer) { - clearTimeout(this.indexWriteTimer) - this.indexWriteTimer = null - } - - await this.writeIndex() + private buildDelta(id: string, cached: HistoryItem, incoming: Partial): Partial { + return { id, ...this.computeDelta(cached, incoming) } } - // ────────────────────────────── Private: Per-task file I/O ────────────────────────────── - /** * Write a HistoryItem to its per-task `history_item.json` file. + * + * When `delta` is provided, the merge callback applies only the + * delta to the current disk state, so fields written by another + * process are preserved. Without a delta the full item is written + * as-is (used by administrative repair paths that are authoritative). */ - private async writeTaskFile(item: HistoryItem): Promise { + private async writeTaskFile(item: HistoryItem, delta?: Partial): Promise { const filePath = await this.getTaskFilePath(item.id) - await safeWriteJson(filePath, item) + if (delta) { + let written: HistoryItem = item + const mergeFn = mergeWithDisk(delta) + await safeWriteJson(filePath, item, { + merge: (existing, incoming) => { + const result = mergeFn(existing, incoming) + written = result as HistoryItem + return result + }, + }) + return written + } else { + await safeWriteJson(filePath, item) + return item + } } /** @@ -1055,10 +1036,11 @@ 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. + * Update two related HistoryItems within a single in-process lock acquisition. + * Both updaters run synchronously (no I/O, no lock re-entry). Both writes + * complete before the lock releases, so no in-process reader can observe an + * intermediate state. Cross-process atomicity is NOT guaranteed — each + * writeTaskFile call acquires and releases its own advisory file lock. * * @throws If either task ID is not present in the cache. */ @@ -1105,16 +1087,21 @@ export class TaskHistoryStore { 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) + const writtenFirst = await this.writeTaskFile(mergedFirst, this.buildDelta(firstId, first, updatedFirst)) + let writtenSecond: HistoryItem + try { + writtenSecond = await this.writeTaskFile(mergedSecond, this.buildDelta(secondId, second, updatedSecond)) + } catch (error) { + // First record is committed on disk. Update cache so it + // reflects disk state before propagating the error. + this.cache.set(firstId, writtenFirst) + throw error + } - // Both disk writes succeeded — now update the cache atomically. - this.cache.set(firstId, mergedFirst) - this.cache.set(secondId, mergedSecond) + // Both disk writes succeeded — now update the cache. + this.cache.set(firstId, writtenFirst) + this.cache.set(secondId, writtenSecond) - this.scheduleIndexWrite() const all = this.getAll() if (this.onWrite) { await this.onWrite(all) @@ -1155,12 +1142,4 @@ export class TaskHistoryStore { const tasksDir = await this.getTasksDir() return path.join(tasksDir, taskId, GlobalFileNames.historyItem) } - - /** - * Get the path to the `_index.json` file. - */ - private async getIndexPath(): Promise { - const tasksDir = await this.getTasksDir() - return path.join(tasksDir, GlobalFileNames.historyIndex) - } } diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.crossInstance.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.crossInstance.spec.ts index e5166c478c..cef9874e5f 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.crossInstance.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.crossInstance.spec.ts @@ -6,7 +6,7 @@ import * as os from "os" import type { HistoryItem } from "@roo-code/types" -import { TaskHistoryStore } from "../TaskHistoryStore" +import { TaskHistoryStore, DeltaRejectedError } from "../TaskHistoryStore" import { GlobalFileNames } from "../../../shared/globalFileNames" vi.mock("../../../utils/storage", () => ({ @@ -15,12 +15,30 @@ vi.mock("../../../utils/storage", () => ({ }), })) -// Mock safeWriteJson to use plain fs writes in tests (avoids proper-lockfile issues) +// Mock safeWriteJson to use plain fs writes but honor the merge callback. vi.mock("../../../utils/safeWriteJson", () => ({ - safeWriteJson: vi.fn().mockImplementation(async (filePath: string, data: any) => { - await fs.mkdir(path.dirname(filePath), { recursive: true }) - await fs.writeFile(filePath, JSON.stringify(data, null, "\t"), "utf8") - }), + safeWriteJson: vi + .fn() + .mockImplementation( + async ( + filePath: string, + data: unknown, + options?: { merge?: (existing: unknown, incoming: unknown) => unknown }, + ) => { + await fs.mkdir(path.dirname(filePath), { recursive: true }) + if (options?.merge) { + let existing: unknown = null + try { + const raw = await fs.readFile(filePath, "utf8") + existing = JSON.parse(raw) + } catch { + // File does not exist or is corrupt + } + data = options.merge(existing, data) + } + await fs.writeFile(filePath, JSON.stringify(data, null, "\t"), "utf8") + }, + ), })) function makeHistoryItem(overrides: Partial = {}): HistoryItem { @@ -124,6 +142,27 @@ describe("TaskHistoryStore cross-instance safety", () => { expect(storeB.get("shared-task")).toBeUndefined() }) + it("delete by instance A is detected even when the task directory remains", async () => { + await storeA.initialize() + await storeB.initialize() + + const item = makeHistoryItem({ id: "file-only-delete" }) + await storeA.upsert(item) + await storeB.reconcile() + + expect(storeB.get("file-only-delete")).toBeDefined() + + // delete() unlinks history_item.json but leaves the task directory. + await storeA.delete("file-only-delete") + + // Directory still exists (other files like ui_messages.json may remain). + const taskDir = path.join(tmpDir, "tasks", "file-only-delete") + await expect(fs.access(taskDir)).resolves.toBeUndefined() + + await storeB.reconcile() + expect(storeB.get("file-only-delete")).toBeUndefined() + }) + it("per-task file updates by one instance are visible to another after invalidation", async () => { await storeA.initialize() await storeB.initialize() @@ -164,4 +203,107 @@ describe("TaskHistoryStore cross-instance safety", () => { expect(storeA.getAll().length).toBe(10) expect(storeB.getAll().length).toBe(10) }) + + /** + * Host B completes a task on disk while host A's cache still has it + * active. Host A's next save updates only totalCost (a full-object + * upsert — the realistic production shape). The diff-delta merge + * preserves B's status because status did not change in A's cache. + */ + it("per-task diff-delta preserves a peer's status change on full-object upsert", async () => { + await storeA.initialize() + + // Base item with an explicit status — mirrors real production items. + const base = makeHistoryItem({ id: "shared-task", status: "active", totalCost: 0.01, ts: 1000 }) + await storeA.upsert(base) + + // Host B completes the task on disk; A's cache still has "active". + const filePath = path.join(tmpDir, "tasks", "shared-task", GlobalFileNames.historyItem) + const onDisk = JSON.parse(await fs.readFile(filePath, "utf8")) + onDisk.status = "completed" + onDisk.completionResultSummary = "done by host B" + await fs.writeFile(filePath, JSON.stringify(onDisk), "utf8") + + // Host A does a full-object upsert (the realistic path — spread the + // cached item and change one field). The cached item has status: "active". + await storeA.upsert({ ...storeA.get("shared-task")!, totalCost: 9.99 }) + + const final = JSON.parse(await fs.readFile(filePath, "utf8")) as HistoryItem + expect(final.totalCost).toBe(9.99) + // Status is preserved from disk because A's delta does not include + // status — it was unchanged relative to A's cache. + expect(final.status).toBe("completed") + expect(final.completionResultSummary).toBe("done by host B") + + // Cache reflects the caller's totalCost change and the peer's status. + expect(storeA.get("shared-task")!.totalCost).toBe(9.99) + expect(storeA.get("shared-task")!.status).toBe("completed") + expect(storeA.get("shared-task")!.completionResultSummary).toBe("done by host B") + }) + + /** + * Regression: a stale host whose cache says "active" tries to write + * status: "delegated" after a peer already wrote "completed" to disk. + * The merge must reject the entire delta (including companion fields) + * to prevent an internally-inconsistent record. + */ + it("merge rejects an invalid status transition against disk and throws DeltaRejectedError", async () => { + await storeA.initialize() + + const base = makeHistoryItem({ id: "guarded-task", status: "active", totalCost: 0.01, ts: 1000 }) + await storeA.upsert(base) + + // Peer writes terminal "completed" directly to disk. + const filePath = path.join(tmpDir, "tasks", "guarded-task", GlobalFileNames.historyItem) + const onDisk = JSON.parse(await fs.readFile(filePath, "utf8")) + onDisk.status = "completed" + onDisk.completionResultSummary = "done by peer" + await fs.writeFile(filePath, JSON.stringify(onDisk), "utf8") + + // Host A's cache still has "active". It tries to delegate (active → delegated + // passes the cache check, but completed → delegated is invalid on disk). + const staleItem = storeA.get("guarded-task")! + await expect( + storeA.upsert({ + ...staleItem, + status: "delegated", + awaitingChildId: "child-99", + delegatedToId: "child-99", + }), + ).rejects.toThrow(DeltaRejectedError) + + const final = JSON.parse(await fs.readFile(filePath, "utf8")) as HistoryItem + // Terminal status must survive — disk is untouched. + expect(final.status).toBe("completed") + expect(final.completionResultSummary).toBe("done by peer") + // Companion fields from the rejected delta must NOT be applied. + expect(final.awaitingChildId).toBeUndefined() + expect(final.delegatedToId).toBeUndefined() + + // Cache must reflect the disk state, not the stale delta. + expect(storeA.get("guarded-task")!.status).toBe("completed") + }) + + /** + * When both hosts change the same field, the last writer wins. + * This is expected — true conflict resolution requires application + * semantics that a generic merge cannot provide. + */ + it("same-field changes from both hosts are last-writer-wins", async () => { + await storeA.initialize() + await storeB.initialize() + + const base = makeHistoryItem({ id: "shared-task", status: "active", totalCost: 0.01, ts: 1000 }) + await storeA.upsert(base) + await storeB.reconcile() + + // Both hosts change totalCost. + await storeA.upsert({ ...storeA.get("shared-task")!, totalCost: 1.0 }) + await storeB.upsert({ ...storeB.get("shared-task")!, totalCost: 2.0 }) + + const filePath = path.join(tmpDir, "tasks", "shared-task", GlobalFileNames.historyItem) + const final = JSON.parse(await fs.readFile(filePath, "utf8")) as HistoryItem + // B wrote last, so B's value wins. + expect(final.totalCost).toBe(2.0) + }) }) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index e788b5d96a..e37fd1a25e 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -465,7 +465,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { await expect(fs.access(intentPath)).rejects.toThrow() }) - it("does not schedule the derived index before repair-intent cleanup succeeds", async () => { + it("removes the repair-intent file after successful replay", async () => { const child = makeItem({ id: "child-deferred-index", status: "active", parentTaskId: "parent-deferred-index" }) const parent = makeItem({ id: "parent-deferred-index", @@ -478,23 +478,12 @@ describe("TaskHistoryStore reconcileDelegationState", () => { await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) await store.reconcile({ forceRefresh: true }) - const events: string[] = [] const storeInternals = store as unknown as { - scheduleIndexWrite: () => void - removeDelegationRepairIntent: () => Promise replayDelegationRepairIntent: () => Promise } - vi.spyOn(storeInternals, "removeDelegationRepairIntent").mockImplementation(async () => { - events.push("cleanup") - await fs.unlink(intentPath) - }) - vi.spyOn(storeInternals, "scheduleIndexWrite").mockImplementation(() => { - events.push("schedule") - }) await storeInternals.replayDelegationRepairIntent() - expect(events).toEqual(["cleanup", "schedule"]) await expect(fs.access(intentPath)).rejects.toThrow() }) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts index 3188e9c505..8ce80e096d 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts @@ -57,29 +57,19 @@ describe("TaskHistoryStore", () => { expect(store.getAll()).toEqual([]) }) - it("initializes from existing index file", async () => { + it("initializes from existing per-task files", async () => { const tasksDir = path.join(tmpDir, "tasks") await fs.mkdir(tasksDir, { recursive: true }) const item1 = makeHistoryItem({ id: "task-1", ts: 1000 }) const item2 = makeHistoryItem({ id: "task-2", ts: 2000 }) - // Create task directories so reconciliation doesn't remove them await fs.mkdir(path.join(tasksDir, "task-1"), { recursive: true }) await fs.mkdir(path.join(tasksDir, "task-2"), { recursive: true }) - // Write per-task files await fs.writeFile(path.join(tasksDir, "task-1", GlobalFileNames.historyItem), JSON.stringify(item1)) await fs.writeFile(path.join(tasksDir, "task-2", GlobalFileNames.historyItem), JSON.stringify(item2)) - // Write index - const index = { - version: 1, - updatedAt: Date.now(), - entries: [item1, item2], - } - await fs.writeFile(path.join(tasksDir, GlobalFileNames.historyIndex), JSON.stringify(index)) - await store.initialize() expect(store.getAll()).toHaveLength(2) @@ -374,7 +364,7 @@ describe("TaskHistoryStore", () => { expect(store.get("idem-task")).toBeDefined() }) - it("serializes migration cache and index updates behind the store lock", async () => { + it("serializes migration cache updates behind the store lock", async () => { const tasksDir = path.join(tmpDir, "tasks") const migrated = makeHistoryItem({ id: "migration-locked" }) const concurrent = makeHistoryItem({ id: "migration-concurrent" }) @@ -389,12 +379,17 @@ describe("TaskHistoryStore", () => { const migrationWriteStarted = new Promise((resolve) => { signalMigrationWriteStarted = resolve }) - const storeInternals = store as unknown as { writeIndex: () => Promise } - const originalWriteIndex = storeInternals.writeIndex.bind(store) - vi.spyOn(storeInternals, "writeIndex").mockImplementation(async () => { - signalMigrationWriteStarted() - await migrationWriteCanFinish - return originalWriteIndex() + + const { safeWriteJson: mockSafeWriteJson } = await import("../../../utils/safeWriteJson") + const originalImpl = vi.mocked(mockSafeWriteJson).getMockImplementation()! + let firstCall = true + vi.mocked(mockSafeWriteJson).mockImplementation(async (...args) => { + if (firstCall) { + firstCall = false + signalMigrationWriteStarted() + await migrationWriteCanFinish + } + return originalImpl(...args) }) const migration = store.migrateFromGlobalState([migrated]) @@ -407,45 +402,6 @@ describe("TaskHistoryStore", () => { expect(store.get(migrated.id)).toEqual(migrated) expect(store.get(concurrent.id)).toEqual(concurrent) - await store.flushIndex() - const index = JSON.parse(await fs.readFile(path.join(tasksDir, GlobalFileNames.historyIndex), "utf8")) as { - entries: HistoryItem[] - } - expect(index.entries.map((entry) => entry.id)).toEqual(expect.arrayContaining([migrated.id, concurrent.id])) - }) - }) - - describe("flushIndex()", () => { - it("writes index to disk on flush", async () => { - await store.initialize() - - await store.upsert(makeHistoryItem({ id: "flush-task" })) - await store.flushIndex() - - const indexPath = path.join(tmpDir, "tasks", GlobalFileNames.historyIndex) - const raw = await fs.readFile(indexPath, "utf8") - const index = JSON.parse(raw) - - expect(index.version).toBe(1) - expect(index.entries).toHaveLength(1) - expect(index.entries[0].id).toBe("flush-task") - }) - }) - - describe("dispose()", () => { - it("flushes index on dispose", async () => { - await store.initialize() - - await store.upsert(makeHistoryItem({ id: "dispose-task" })) - store.dispose() - - // Give the flush a moment to complete - await new Promise((resolve) => setTimeout(resolve, 100)) - - const indexPath = path.join(tmpDir, "tasks", GlobalFileNames.historyIndex) - const raw = await fs.readFile(indexPath, "utf8") - const index = JSON.parse(raw) - expect(index.entries).toHaveLength(1) }) }) @@ -748,8 +704,9 @@ describe("TaskHistoryStore", () => { 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") + // First record's cache IS updated (it was committed to disk). + // Second record's cache is unchanged (write never completed). + expect(store.get("child-partial")?.status).toBe("completed") expect(store.get("parent-partial")?.status).toBe("delegated") }) diff --git a/src/core/webview/__tests__/webviewMessageHandler.importRooHistory.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.importRooHistory.spec.ts index df85ff1df4..1f23e353c6 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.importRooHistory.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.importRooHistory.spec.ts @@ -52,7 +52,6 @@ describe("webviewMessageHandler - importRooHistory", () => { taskHistoryStore: { invalidateAll: ReturnType reconcile: ReturnType - flushIndex: ReturnType } postMessageToWebview: ReturnType postStateToWebview: ReturnType @@ -71,7 +70,6 @@ describe("webviewMessageHandler - importRooHistory", () => { taskHistoryStore: { invalidateAll: vi.fn(), reconcile: vi.fn().mockResolvedValue(undefined), - flushIndex: vi.fn().mockResolvedValue(undefined), }, postMessageToWebview: vi.fn().mockResolvedValue(undefined), postStateToWebview: vi.fn().mockResolvedValue(undefined), @@ -106,7 +104,7 @@ describe("webviewMessageHandler - importRooHistory", () => { expect(importRooTaskHistoryMock).toHaveBeenCalledWith("/mock/storage", expect.any(Function)) expect(mockProvider.taskHistoryStore.invalidateAll).toHaveBeenCalledTimes(1) expect(mockProvider.taskHistoryStore.reconcile).toHaveBeenCalledTimes(1) - expect(mockProvider.taskHistoryStore.flushIndex).toHaveBeenCalledTimes(1) + expect(mockProvider.postStateToWebview).toHaveBeenCalledTimes(1) expect(mockProvider.postMessageToWebview).toHaveBeenNthCalledWith(1, { type: "rooHistoryImportProgress", @@ -189,7 +187,7 @@ describe("webviewMessageHandler - importRooHistory", () => { expect(importRooTaskHistoryMock).toHaveBeenCalledWith("/mock/storage", expect.any(Function)) expect(mockProvider.taskHistoryStore.invalidateAll).not.toHaveBeenCalled() expect(mockProvider.taskHistoryStore.reconcile).not.toHaveBeenCalled() - expect(mockProvider.taskHistoryStore.flushIndex).not.toHaveBeenCalled() + expect(mockProvider.postStateToWebview).not.toHaveBeenCalled() expect(mockProvider.postMessageToWebview).toHaveBeenNthCalledWith(2, { type: "rooHistoryImportProgress", @@ -222,7 +220,7 @@ describe("webviewMessageHandler - importRooHistory", () => { // after a partial-copy failure still reconciles the store. expect(mockProvider.taskHistoryStore.invalidateAll).toHaveBeenCalledTimes(1) expect(mockProvider.taskHistoryStore.reconcile).toHaveBeenCalledTimes(1) - expect(mockProvider.taskHistoryStore.flushIndex).toHaveBeenCalledTimes(1) + expect(mockProvider.postStateToWebview).toHaveBeenCalledTimes(1) expect(vscode.window.showWarningMessage).toHaveBeenCalledWith( "common:warnings.rooHistoryImport.alreadyImported", @@ -237,7 +235,7 @@ describe("webviewMessageHandler - importRooHistory", () => { expect(mockProvider.taskHistoryStore.invalidateAll).not.toHaveBeenCalled() expect(mockProvider.taskHistoryStore.reconcile).not.toHaveBeenCalled() - expect(mockProvider.taskHistoryStore.flushIndex).not.toHaveBeenCalled() + expect(mockProvider.postStateToWebview).not.toHaveBeenCalled() expect(mockProvider.log).toHaveBeenCalledWith("[importRooHistory] failed: permission denied") expect(mockProvider.postMessageToWebview).toHaveBeenNthCalledWith(2, { diff --git a/src/core/webview/webviewMessageHandler.ts b/src/core/webview/webviewMessageHandler.ts index b481e2ec1d..648269c1b5 100644 --- a/src/core/webview/webviewMessageHandler.ts +++ b/src/core/webview/webviewMessageHandler.ts @@ -1003,7 +1003,6 @@ export const webviewMessageHandler = async ( // so a retry after a partial-copy failure still reconciles the store. await provider.taskHistoryStore.invalidateAll() await provider.taskHistoryStore.reconcile() - await provider.taskHistoryStore.flushIndex() await provider.postStateToWebview() await provider.postMessageToWebview({ type: "rooHistoryImportProgress", diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index f790fba436..f405adc8df 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -779,11 +779,6 @@ "count": 4 } }, - "core/task-persistence/__tests__/TaskHistoryStore.crossInstance.spec.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 1 - } - }, "core/task-persistence/__tests__/TaskHistoryStore.spec.ts": { "@typescript-eslint/no-explicit-any": { "count": 4 diff --git a/src/shared/globalFileNames.ts b/src/shared/globalFileNames.ts index 7bfe18f4bc..9f15a06319 100644 --- a/src/shared/globalFileNames.ts +++ b/src/shared/globalFileNames.ts @@ -5,6 +5,5 @@ export const GlobalFileNames = { customModes: "custom_modes.yaml", taskMetadata: "task_metadata.json", historyItem: "history_item.json", - historyIndex: "_index.json", delegationRepairIntent: "_delegation_repair_intent.json", } diff --git a/src/utils/__tests__/safeWriteJson.test.ts b/src/utils/__tests__/safeWriteJson.test.ts index e060de4a31..79d08678a0 100644 --- a/src/utils/__tests__/safeWriteJson.test.ts +++ b/src/utils/__tests__/safeWriteJson.test.ts @@ -468,4 +468,78 @@ describe("safeWriteJson", () => { consoleErrorSpy.mockRestore() }) + + // Merge option tests + test("should merge incoming data with existing file content when merge callback is provided", async () => { + const initial = { a: 1, b: 2 } + await safeWriteJson(currentTestFilePath, initial) + + const incoming = { b: 3, c: 4 } + await safeWriteJson(currentTestFilePath, incoming, { + merge: (existing, data) => ({ + ...(existing as Record), + ...(data as Record), + }), + }) + + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual({ a: 1, b: 3, c: 4 }) + }) + + test("should pass null to merge callback when file does not exist", async () => { + const newFilePath = path.join(tempDir, "nonexistent.json") + const mergeFn = vi.fn((existing, incoming) => incoming) + + await safeWriteJson(newFilePath, { value: 42 }, { merge: mergeFn }) + + expect(mergeFn).toHaveBeenCalledWith(null, { value: 42 }) + const content = await readFileContent(newFilePath) + expect(content).toEqual({ value: 42 }) + }) + + test("should propagate non-ENOENT read errors during merge instead of silently losing data", async () => { + const initial = { a: 1, b: 2 } + await safeWriteJson(currentTestFilePath, initial) + + const eio = Object.assign(new Error("I/O error"), { code: "EIO" }) + vi.mocked(fs.readFile).mockRejectedValueOnce(eio) + + await expect( + safeWriteJson( + currentTestFilePath, + { b: 99 }, + { + merge: (existing, incoming) => ({ + ...(existing as Record), + ...(incoming as Record), + }), + }, + ), + ).rejects.toThrow("I/O error") + + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual({ a: 1, b: 2 }) + }) + + test("should treat corrupt JSON as null during merge", async () => { + await fs.writeFile(currentTestFilePath, "not valid json", "utf8") + + const mergeFn = vi.fn((_existing, incoming) => incoming) + await safeWriteJson(currentTestFilePath, { value: 1 }, { merge: mergeFn }) + + expect(mergeFn).toHaveBeenCalledWith(null, { value: 1 }) + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual({ value: 1 }) + }) + + test("should write incoming data directly when no merge callback is provided", async () => { + const initial = { a: 1, b: 2 } + await safeWriteJson(currentTestFilePath, initial) + + const replacement = { c: 3 } + await safeWriteJson(currentTestFilePath, replacement) + + const content = await readFileContent(currentTestFilePath) + expect(content).toEqual({ c: 3 }) + }) }) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index c32dd92ce5..957a0bb20f 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -15,6 +15,16 @@ export interface SafeWriteJsonOptions { * @default false */ prettyPrint?: boolean + + /** + * When provided, the current file is read under the advisory lock + * and passed to this function along with the incoming data. The + * return value replaces `data` for the write. This turns a blind + * overwrite into an atomic read-modify-write, preventing cross-process + * lost updates. `existing` is null when the file does not exist or + * cannot be parsed. + */ + merge?: (existing: unknown, incoming: unknown) => unknown } /** @@ -54,7 +64,7 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso // Acquire the lock before any file operations try { releaseLock = await lockfile.lock(absoluteFilePath, { - stale: 31000, // Stale after 31 seconds + stale: LOCK_STALE_MS, update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long realpath: false, // the file may not exist yet, which is acceptable retries: { @@ -83,6 +93,23 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso let actualTempBackupFilePath: string | null = null try { + // If a merge callback was provided, read the current file under the lock + // and let the caller merge before we write. Must be inside try/finally + // so a throwing merge still releases the lock. + if (options?.merge) { + let existing: unknown = null + try { + existing = JSON.parse(await fs.readFile(absoluteFilePath, "utf8")) + } catch (error: unknown) { + const code = + error && typeof error === "object" && "code" in error ? (error as { code: string }).code : undefined + if (!(error instanceof SyntaxError) && code !== "ENOENT") { + throw error + } + } + data = options.merge(existing, data) + } + // Step 1: Write data to a new temporary file. actualTempNewFilePath = path.join( path.dirname(absoluteFilePath), @@ -220,4 +247,6 @@ async function _streamDataToFile(targetPath: string, data: any, prettyPrint = fa }) } +export const LOCK_STALE_MS = 31_000 + export { safeWriteJson }