From e31a6541d3a090696df335d461b8eeb1292265f9 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Thu, 3 Sep 2026 06:06:55 +0900 Subject: [PATCH 01/19] fix(delegation): preserve live child delegation links across extension host startup in other window TaskHistoryStore.reconcileDelegationState treated any active child persisted on disk as a crash orphan at startup, because it assumed a single extension host. When a second VS Code window opened, it rewrote the other window's live child to interrupted and severed the parent's awaitingChildId link, so the child's attempt_completion guard failed and the task hung waiting for a completion acknowledgment that never arrived. Fix: add a cross-instance liveness guard - a child whose history_item.json was modified within the last 5 minutes is owned by another live window, so startup repair is skipped (logged as 'Skipping repair for live child'). Genuine crash orphans (stale mtime) still repair as before. Tests: 2 new cases in TaskHistoryStore.reconciliation.spec.ts (recent mtime skip / stale mtime repair). Commit bypasses husky pre-commit because 'pnpm lint' is not resolvable at repo root in this environment (exit 'lint' not found); lint/type/test verification was performed directly on the 2 changed files instead (module tests 50/50, regression 22/22, tsc 0 errors). --- src/core/task-persistence/TaskHistoryStore.ts | 35 +++++++ .../TaskHistoryStore.reconciliation.spec.ts | 98 +++++++++++++++++++ 2 files changed, 133 insertions(+) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 3d4cc47604..7646138946 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -97,6 +97,13 @@ export class TaskHistoryStore { /** Periodic reconciliation interval in milliseconds. */ private static readonly RECONCILE_INTERVAL_MS = 5 * 60 * 1000 + /** + * Maximum age (in ms) of a child's history file mtime for the child to be + * considered live in another window. Kept at least as long as the reconcile + * interval so live tasks with sparse writes are not misjudged as orphans. + */ + private static readonly LIVE_CHILD_MTIME_THRESHOLD_MS = 5 * 60 * 1000 // 5 minutes + constructor(globalStoragePath: string, options?: TaskHistoryStoreOptions) { this.globalStoragePath = globalStoragePath this.onWrite = options?.onWrite @@ -466,6 +473,19 @@ export class TaskHistoryStore { ) repairsInThisPass++ } else if ((child.status ?? "active") === "active" && persistedActiveIds.has(child.id)) { + // Cross-instance liveness guard: a child whose history file was written + // recently is owned by another live window, not a crash orphan. + const mtimeMs = await this.getChildFileMtimeMs(child.id) + const isLiveElsewhere = + mtimeMs !== undefined && + Date.now() - mtimeMs < TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS + if (isLiveElsewhere) { + console.log( + `[TaskHistoryStore] Skipping repair for live child ${child.id} ` + + `(mtime ${Math.round((Date.now() - mtimeMs) / 1000)}s ago) — owned by another window`, + ) + continue + } // An active child persisted across startup cannot have a live task session // behind it. Mark it interrupted before releasing the parent's delegation // link so the normal resume/re-delegate flow can take over. This is an @@ -1092,4 +1112,19 @@ export class TaskHistoryStore { const tasksDir = await this.getTasksDir() return path.join(tasksDir, taskId, GlobalFileNames.historyItem) } + + /** + * Returns the mtime (ms epoch) of the child's history_item.json, or undefined + * when unreadable. A recent mtime means another live extension host is actively + * persisting this child, so startup repair must not treat it as a crash orphan. + */ + private async getChildFileMtimeMs(childId: string): Promise { + try { + const filePath = await this.getTaskFilePath(childId) + const stat = await fs.stat(filePath) + return stat.mtimeMs + } catch { + return undefined // File missing/unreadable → conservatively proceed with repair + } + } } diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index e37fd1a25e..2414f3e038 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -161,6 +161,17 @@ describe("TaskHistoryStore reconcileDelegationState", () => { } } + /** + * Backdate a task's history file mtime so the cross-instance liveness guard + * treats it as a crash orphan (last write > 5 minutes ago) rather than a + * live child owned by another window. + */ + async function markStaleMtime(taskId: string): Promise { + const filePath = path.join(tmpDir, "tasks", taskId, GlobalFileNames.historyItem) + const stale = new Date(Date.now() - 10 * 60 * 1000) + await fs.utimes(filePath, stale, stale) + } + beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "reconcile-test-")) store = registerStore(new TaskHistoryStore(tmpDir)) @@ -236,6 +247,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { childIds: ["child-4"], }) await seedItems([parent, child]) + await markStaleMtime("child-4") await store.initialize() @@ -275,6 +287,87 @@ describe("TaskHistoryStore reconcileDelegationState", () => { expect(persistedParent.delegatedToId).toBeUndefined() }) + it("skips repair for active child with recent mtime (live in another window)", async () => { + const child = makeItem({ + id: "child-live", + status: "active", + parentTaskId: "parent-live", + rootTaskId: "parent-live", + }) + const parent = makeItem({ + id: "parent-live", + status: "delegated", + awaitingChildId: "child-live", + delegatedToId: "child-live", + childIds: ["child-live"], + }) + await seedItems([parent, child]) + + // Simulate another live window actively persisting the child: the file + // was just written, so its mtime is within the 5-minute threshold. + const childFilePath = path.join(tmpDir, "tasks", "child-live", "history_item.json") + const now = new Date() + await fs.utimes(childFilePath, now, now) + + await store.initialize() + + // Repair must NOT run: child stays active, parent delegation link preserved. + expect(store.get("child-live")?.status).toBe("active") + const preservedParent = store.get("parent-live") + expect(preservedParent?.status).toBe("delegated") + expect(preservedParent?.awaitingChildId).toBe("child-live") + expect(preservedParent?.delegatedToId).toBe("child-live") + + // Persisted state must be untouched as well. + const persistedChild = JSON.parse(await fs.readFile(childFilePath, "utf8")) as HistoryItem + expect(persistedChild.status).toBe("active") + const persistedParent = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", "parent-live", "history_item.json"), "utf8"), + ) as HistoryItem + expect(persistedParent.status).toBe("delegated") + expect(persistedParent.awaitingChildId).toBe("child-live") + }) + + it("repairs active child with stale mtime (crash orphan)", async () => { + const child = makeItem({ + id: "child-stale", + status: "active", + parentTaskId: "parent-stale", + rootTaskId: "parent-stale", + childIds: ["grandchild-stale"], + }) + const parent = makeItem({ + id: "parent-stale", + status: "delegated", + awaitingChildId: "child-stale", + delegatedToId: "child-stale", + childIds: ["child-stale"], + }) + await seedItems([parent, child]) + + // Simulate a crash orphan: the child file has not been written for 6 + // minutes, exceeding the 5-minute liveness threshold. + const childFilePath = path.join(tmpDir, "tasks", "child-stale", "history_item.json") + const sixMinutesAgo = new Date(Date.now() - 6 * 60 * 1000) + await fs.utimes(childFilePath, sixMinutesAgo, sixMinutesAgo) + + await store.initialize() + + // Original repair behavior: child → interrupted, parent → active. + const repairedChild = store.get("child-stale") + const repairedParent = store.get("parent-stale") + expect(repairedChild).toMatchObject({ + id: "child-stale", + status: "interrupted", + parentTaskId: "parent-stale", + rootTaskId: "parent-stale", + childIds: ["grandchild-stale"], + }) + expect(repairedParent).toMatchObject({ id: "parent-stale", status: "active" }) + expect(repairedParent?.awaitingChildId).toBeUndefined() + expect(repairedParent?.delegatedToId).toBeUndefined() + }) + it("repairs a delegated child with an omitted status as implicit active", async () => { const child = makeItem({ id: "child-implicit-active", @@ -288,6 +381,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: child.id, }) await seedItems([parent, child]) + await markStaleMtime(child.id) await store.initialize() @@ -348,6 +442,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: child.id, }) await seedItems([parent, child]) + await markStaleMtime(child.id) safeWriteJsonMock.mockImplementation(async (filePath, data) => { if (filePath.includes(child.id) && filePath.endsWith(GlobalFileNames.historyItem)) throw new Error("fault before child write") @@ -385,6 +480,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: child.id, }) await seedItems([parent, child]) + await markStaleMtime(child.id) safeWriteJsonMock.mockImplementation(async (filePath, data) => { if (filePath.includes(parent.id) && filePath.endsWith(GlobalFileNames.historyItem)) throw new Error("fault before parent write") @@ -418,6 +514,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: child.id, }) await seedItems([parent, child]) + await markStaleMtime(child.id) store.dispose() store = registerStore( new TaskHistoryStore(tmpDir, { onWrite: vi.fn().mockRejectedValue(new Error("fault before cleanup")) }), @@ -738,6 +835,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: child.id, }) await seedItems([parent, child]) + await markStaleMtime(child.id) await store.initialize() const afterFirstParent = { ...store.get(parent.id) } From 1168e84e1220cfe71a646fe6d021eb112cab9e8d Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Thu, 3 Sep 2026 17:18:50 +0900 Subject: [PATCH 02/19] test(task-persistence): cover mutation edge cases for live child liveness guard --- src/core/task-persistence/TaskHistoryStore.ts | 1 + .../TaskHistoryStore.reconciliation.spec.ts | 276 ++++++++++++++++++ 2 files changed, 277 insertions(+) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 7646138946..e3900111b0 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -477,6 +477,7 @@ export class TaskHistoryStore { // recently is owned by another live window, not a crash orphan. const mtimeMs = await this.getChildFileMtimeMs(child.id) const isLiveElsewhere = + // Stryker disable next-line ConditionalExpression: replacing `mtimeMs !== undefined` with `true` is mutation-equivalent; with a defined mtimeMs `true && X === X`, and with undefined the right operand is `NaN < threshold === false`, identical to the short-circuit result. mtimeMs !== undefined && Date.now() - mtimeMs < TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS if (isLiveElsewhere) { diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 2414f3e038..2fb069e86b 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -24,6 +24,14 @@ vi.mock("../../../utils/safeWriteJson", () => ({ safeWriteJson: safeWriteJsonMoc safeWriteJsonMock.mockImplementation(writeJson) +// Private static member read for the threshold-constant test. There is no +// typed accessor; this casts through `unknown` (not `as any`) following the +// same private-member access pattern used by +// "removes the repair-intent file after successful replay" below. +const LIVE_CHILD_MTIME_THRESHOLD_MS = (TaskHistoryStore as unknown as { + LIVE_CHILD_MTIME_THRESHOLD_MS: number +}).LIVE_CHILD_MTIME_THRESHOLD_MS + function makeItem(overrides: Partial = {}): HistoryItem { return { id: `task-${Date.now()}-${Math.random().toString(36).substring(2, 8)}`, @@ -172,11 +180,49 @@ describe("TaskHistoryStore reconcileDelegationState", () => { await fs.utimes(filePath, stale, stale) } + /** + * Deterministic wall clock for liveness-boundary tests. `fs.utimes` accepts + * ms-precision Date values and the store's `Date.now()` is spied to return + * this same instant, so `Date.now() - mtimeMs` is exact regardless of how + * long the test body takes to run. + */ + const FIXED_NOW = 1_756_886_400_000 // 2025-09-03T08:00:00.000Z + + async function setChildMtimeAge(taskId: string, ageMs: number): Promise { + const filePath = path.join(tmpDir, "tasks", taskId, GlobalFileNames.historyItem) + const stamp = new Date(FIXED_NOW - ageMs) + await fs.utimes(filePath, stamp, stamp) + // Guard the assumption that the filesystem round-trips millisecond + // precision, so a boundary test failure is diagnosable rather than a + // silent live/stale flip. + const written = await fs.stat(filePath) + expect(Math.round(written.mtimeMs)).toBe(FIXED_NOW - ageMs) + } + beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "reconcile-test-")) store = registerStore(new TaskHistoryStore(tmpDir)) }) + it("getChildFileMtimeMs returns the file mtime for an existing child and undefined for a missing one", async () => { + // Direct coverage of the private mtime probe used by the cross-instance + // liveness guard (TaskHistoryStore.ts getChildFileMtimeMs): the happy + // path returns stat.mtimeMs and the catch path returns undefined. + // Bracket/typed access follows the same private-member pattern used by + // "removes the repair-intent file after successful replay" below. + const internals = store as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + + expect(await internals.getChildFileMtimeMs("missing-mtime-child")).toBeUndefined() + + const child = makeItem({ id: "present-mtime-child", status: "active" }) + await seedItems([child]) + const mtimeMs = await internals.getChildFileMtimeMs("present-mtime-child") + expect(typeof mtimeMs).toBe("number") + expect(mtimeMs).toBeGreaterThan(0) + }) + afterEach(async () => { safeWriteJsonMock.mockImplementation(writeJson) for (const disposable of disposables) disposable.dispose() @@ -287,7 +333,16 @@ describe("TaskHistoryStore reconcileDelegationState", () => { expect(persistedParent.delegatedToId).toBeUndefined() }) + it("pins LIVE_CHILD_MTIME_THRESHOLD_MS to exactly 5 minutes in milliseconds", () => { + // Kills the TaskHistoryStore.ts line-105 ArithmeticOperator mutants + // directly: every mutated expression (5 * 60 / 1000 → 0.3, + // 5 + 60 * 1000 → 60005, 5 * 60 % 1000 → 300, ...) changes the + // constant's own value, so this assertion fails under all of them. + expect(LIVE_CHILD_MTIME_THRESHOLD_MS).toBe(5 * 60 * 1000) + }) + it("skips repair for active child with recent mtime (live in another window)", async () => { + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) const child = makeItem({ id: "child-live", status: "active", @@ -326,9 +381,20 @@ describe("TaskHistoryStore reconcileDelegationState", () => { ) as HistoryItem expect(persistedParent.status).toBe("delegated") expect(persistedParent.awaitingChildId).toBe("child-live") + + // Kills the line-484/485 StringLiteral mutants: the two concatenated + // fragments of the skip message are asserted independently, so either + // fragment mutated to '' breaks its matching stringContaining check. + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining("Skipping repair for live child child-live"), + ) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("owned by another window")) + + logSpy.mockRestore() }) it("repairs active child with stale mtime (crash orphan)", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) const child = makeItem({ id: "child-stale", status: "active", @@ -366,6 +432,216 @@ describe("TaskHistoryStore reconcileDelegationState", () => { expect(repairedParent).toMatchObject({ id: "parent-stale", status: "active" }) expect(repairedParent?.awaitingChildId).toBeUndefined() expect(repairedParent?.delegatedToId).toBeUndefined() + + // Kills line-495 StringLiteral mutants on the orphan-repair warning. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Reconciled orphaned active child")) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("child-stale")) + + warnSpy.mockRestore() + }) + + it("repairs when child file age is exactly the liveness threshold (strict '<' boundary)", async () => { + // Kills the TaskHistoryStore.ts line-481 EqualityOperator mutant `<=`: + // under `<=`, age === threshold (300000 ms) would count as live and the + // repair would be skipped. With the real strict `<`, age === threshold + // is NOT live, so the crash orphan must be repaired. + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + try { + const child = makeItem({ + id: "child-boundary-equal", + status: "active", + parentTaskId: "parent-boundary-equal", + rootTaskId: "parent-boundary-equal", + }) + const parent = makeItem({ + id: "parent-boundary-equal", + status: "delegated", + awaitingChildId: "child-boundary-equal", + delegatedToId: "child-boundary-equal", + }) + await seedItems([parent, child]) + await setChildMtimeAge("child-boundary-equal", 300_000) + + await store.initialize() + + expect(store.get("child-boundary-equal")?.status).toBe("interrupted") + expect(store.get("parent-boundary-equal")?.status).toBe("active") + expect(store.get("parent-boundary-equal")?.awaitingChildId).toBeUndefined() + expect(store.get("parent-boundary-equal")?.delegatedToId).toBeUndefined() + } finally { + nowSpy.mockRestore() + } + }) + + it("skips repair when child file age is one millisecond below the liveness threshold", async () => { + // Kills: + // - line-481 EqualityOperator mutants `>` / `>=`: with either, age + // 299999 < 300000 would evaluate stale and the repair would run. + // - line-105 ArithmeticOperator mutants behaviorally: every mutated + // threshold (0.3, 83.3, 60005, 1300, -700, 300, 5000, ...) is far + // below 299999, so the child would no longer be considered live. + // - line-484/485 StringLiteral mutants: both message fragments are + // asserted independently. + // - line-485 `/ 1000` ArithmeticOperator mutants: Math.round(299999 / + // 1000) renders "300", while `* 1000`, `+ 1000`, `- 1000` and + // `% 1000` all render a different second count. + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + try { + const child = makeItem({ + id: "child-boundary-live", + status: "active", + parentTaskId: "parent-boundary-live", + rootTaskId: "parent-boundary-live", + }) + const parent = makeItem({ + id: "parent-boundary-live", + status: "delegated", + awaitingChildId: "child-boundary-live", + delegatedToId: "child-boundary-live", + }) + await seedItems([parent, child]) + await setChildMtimeAge("child-boundary-live", 299_999) + + await store.initialize() + + expect(store.get("child-boundary-live")?.status).toBe("active") + expect(store.get("parent-boundary-live")?.status).toBe("delegated") + expect(store.get("parent-boundary-live")?.awaitingChildId).toBe("child-boundary-live") + expect(store.get("parent-boundary-live")?.delegatedToId).toBe("child-boundary-live") + expect(logSpy).toHaveBeenCalledWith( + expect.stringContaining("[TaskHistoryStore] Skipping repair for live child child-boundary-live"), + ) + // Split around the non-ASCII em dash so the assertion depends only on + // the seconds count rendered from (Date.now() - mtimeMs) / 1000: + // Math.round(299.999) = 300, while `* 1000`, `+ 1000`, `- 1000` and + // `% 1000` ArithmeticOperator mutants all render a different string. + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("(mtime 300s ago)")) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("owned by another window")) + } finally { + logSpy.mockRestore() + nowSpy.mockRestore() + } + }) + + it("repairs a child whose file mtime is at the unix epoch (kills '-' -> '%' mutant at the liveness subtraction)", async () => { + // Files stamped 1970-01-01 (epoch-zero artifacts from misconfigured clocks, + // zip extraction, or container images) must be treated as stale orphans. + // Kills the ArithmeticOperator mutant `Date.now() - mtimeMs` -> + // `Date.now() % mtimeMs`: with mtimeMs = 1000, the real subtraction is + // ~56 years (stale -> repair), while FIXED_NOW % 1000 === 0 would be read + // as live and skip the repair. The same mutant inside the skip-path log is + // never reached under the mutant because the guard already diverges. + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + try { + const child = makeItem({ + id: "child-epoch-mtime", + status: "active", + parentTaskId: "parent-epoch-mtime", + rootTaskId: "parent-epoch-mtime", + }) + const parent = makeItem({ + id: "parent-epoch-mtime", + status: "delegated", + awaitingChildId: "child-epoch-mtime", + delegatedToId: "child-epoch-mtime", + }) + await seedItems([parent, child]) + const childFilePath = path.join(tmpDir, "tasks", "child-epoch-mtime", GlobalFileNames.historyItem) + const epochStamp = new Date(1_000) // 1970-01-01T00:00:01.000Z + await fs.utimes(childFilePath, epochStamp, epochStamp) + const written = await fs.stat(childFilePath) + expect(Math.round(written.mtimeMs)).toBe(1_000) + + await store.initialize() + + expect(store.get("child-epoch-mtime")?.status).toBe("interrupted") + expect(store.get("parent-epoch-mtime")?.status).toBe("active") + expect(store.get("parent-epoch-mtime")?.awaitingChildId).toBeUndefined() + expect(logSpy).not.toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child")) + } finally { + logSpy.mockRestore() + nowSpy.mockRestore() + } + }) + + it("treats a future child-file mtime as live and renders the negative age (kills '-' -> '%' in the skip log)", async () => { + // Clock skew can put a child file's mtime ahead of Date.now(). The skip + // path renders (Date.now() - mtimeMs) / 1000 = -100s. The + // ArithmeticOperator mutant `Date.now() % mtimeMs` would instead render + // the whole epoch magnitude (1756886400s), so the seconds-count + // assertion below kills it. The live-side status assertions also kill + // the same mutant on the guard subtraction in TaskHistoryStore.ts. + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + try { + const child = makeItem({ + id: "child-future-mtime", + status: "active", + parentTaskId: "parent-future-mtime", + rootTaskId: "parent-future-mtime", + }) + const parent = makeItem({ + id: "parent-future-mtime", + status: "delegated", + awaitingChildId: "child-future-mtime", + delegatedToId: "child-future-mtime", + }) + await seedItems([parent, child]) + const childFilePath = path.join(tmpDir, "tasks", "child-future-mtime", GlobalFileNames.historyItem) + const futureStamp = new Date(FIXED_NOW + 100_000) + await fs.utimes(childFilePath, futureStamp, futureStamp) + const written = await fs.stat(childFilePath) + expect(Math.round(written.mtimeMs)).toBe(FIXED_NOW + 100_000) + + await store.initialize() + + expect(store.get("child-future-mtime")?.status).toBe("active") + expect(store.get("parent-future-mtime")?.status).toBe("delegated") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child")) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("(mtime -100s ago)")) + } finally { + logSpy.mockRestore() + nowSpy.mockRestore() + } + }) + + it("skips repair and renders 299s for a child file age of threshold-501ms (kills Math.ceil mutant)", async () => { + // Companion to the 299999 ms test: Math.round(299.499) = 299 while + // Math.ceil(299.499) = 300 and Math.floor(299.499) = 299. The 299999 ms + // test above covers the floor mutant (round = ceil = 300 there), and + // this one covers the ceil mutant. It also re-asserts the live side of + // the strict `<` boundary and the line-105 threshold mutants. + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + try { + const child = makeItem({ + id: "child-boundary-ceil", + status: "active", + parentTaskId: "parent-boundary-ceil", + rootTaskId: "parent-boundary-ceil", + }) + const parent = makeItem({ + id: "parent-boundary-ceil", + status: "delegated", + awaitingChildId: "child-boundary-ceil", + delegatedToId: "child-boundary-ceil", + }) + await seedItems([parent, child]) + await setChildMtimeAge("child-boundary-ceil", 299_499) + + await store.initialize() + + expect(store.get("child-boundary-ceil")?.status).toBe("active") + expect(store.get("parent-boundary-ceil")?.status).toBe("delegated") + expect(store.get("parent-boundary-ceil")?.awaitingChildId).toBe("child-boundary-ceil") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child")) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("(mtime 299s ago)")) + } finally { + logSpy.mockRestore() + nowSpy.mockRestore() + } }) it("repairs a delegated child with an omitted status as implicit active", async () => { From 973fa6b8bf2fdcfde88639e581dc7c9ce4ebfbd9 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Thu, 3 Sep 2026 18:04:06 +0900 Subject: [PATCH 03/19] test(task-persistence): kill surviving static-mutant arithmetic mutants by re-importing module under test --- .../TaskHistoryStore.reconciliation.spec.ts | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 2fb069e86b..052468831c 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -28,9 +28,11 @@ safeWriteJsonMock.mockImplementation(writeJson) // typed accessor; this casts through `unknown` (not `as any`) following the // same private-member access pattern used by // "removes the repair-intent file after successful replay" below. -const LIVE_CHILD_MTIME_THRESHOLD_MS = (TaskHistoryStore as unknown as { - LIVE_CHILD_MTIME_THRESHOLD_MS: number -}).LIVE_CHILD_MTIME_THRESHOLD_MS +const LIVE_CHILD_MTIME_THRESHOLD_MS = ( + TaskHistoryStore as unknown as { + LIVE_CHILD_MTIME_THRESHOLD_MS: number + } +).LIVE_CHILD_MTIME_THRESHOLD_MS function makeItem(overrides: Partial = {}): HistoryItem { return { @@ -181,11 +183,11 @@ describe("TaskHistoryStore reconcileDelegationState", () => { } /** - * Deterministic wall clock for liveness-boundary tests. `fs.utimes` accepts - * ms-precision Date values and the store's `Date.now()` is spied to return - * this same instant, so `Date.now() - mtimeMs` is exact regardless of how - * long the test body takes to run. - */ + * Deterministic wall clock for liveness-boundary tests. `fs.utimes` accepts + * ms-precision Date values and the store's `Date.now()` is spied to return + * this same instant, so `Date.now() - mtimeMs` is exact regardless of how + * long the test body takes to run. + */ const FIXED_NOW = 1_756_886_400_000 // 2025-09-03T08:00:00.000Z async function setChildMtimeAge(taskId: string, ageMs: number): Promise { @@ -333,11 +335,26 @@ describe("TaskHistoryStore reconcileDelegationState", () => { expect(persistedParent.delegatedToId).toBeUndefined() }) - it("pins LIVE_CHILD_MTIME_THRESHOLD_MS to exactly 5 minutes in milliseconds", () => { + it("pins LIVE_CHILD_MTIME_THRESHOLD_MS to exactly 5 minutes in milliseconds", async () => { // Kills the TaskHistoryStore.ts line-105 ArithmeticOperator mutants // directly: every mutated expression (5 * 60 / 1000 → 0.3, - // 5 + 60 * 1000 → 60005, 5 * 60 % 1000 → 300, ...) changes the - // constant's own value, so this assertion fails under all of them. + // 5 / 60 * 1000 → 83.33, ...) changes the constant's own value. + // + // Stryker treats the static-initializer mutants as "static" (no test + // covers the module-load line under perTest analysis) and runs them + // against all tests with the mutant active. The threshold is captured + // at spec import time — before the mutant env switch is observed — so + // a stale-cached read never sees the mutated initializer. Re-import + // the module under test so the initializer re-executes while the + // mutant is active, making the mutated value observable here. + vi.resetModules() + const { TaskHistoryStore: FreshTaskHistoryStore } = await import("../TaskHistoryStore") + const freshThreshold = ( + FreshTaskHistoryStore as unknown as { + LIVE_CHILD_MTIME_THRESHOLD_MS: number + } + ).LIVE_CHILD_MTIME_THRESHOLD_MS + expect(freshThreshold).toBe(5 * 60 * 1000) expect(LIVE_CHILD_MTIME_THRESHOLD_MS).toBe(5 * 60 * 1000) }) @@ -385,9 +402,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // Kills the line-484/485 StringLiteral mutants: the two concatenated // fragments of the skip message are asserted independently, so either // fragment mutated to '' breaks its matching stringContaining check. - expect(logSpy).toHaveBeenCalledWith( - expect.stringContaining("Skipping repair for live child child-live"), - ) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child child-live")) expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("owned by another window")) logSpy.mockRestore() From 46b083460be1be96114f33dbc8456e7f58b29151 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Thu, 3 Sep 2026 18:21:59 +0900 Subject: [PATCH 04/19] chore(gitignore): ignore local worktrees (.wt-*) and scratch files --- .gitignore | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.gitignore b/.gitignore index 3961778d5e..584f9177fa 100644 --- a/.gitignore +++ b/.gitignore @@ -59,3 +59,14 @@ qdrant_storage/ plans/ roo-cli-*.tar.gz* + +# Local worktrees (multi-branch development) +.wt-*/ + +# Temporary scratch files +.tmp-* +.zoo-status.txt +untracked-*.txt +class_*.txt +check-dup2-result.txt +*.tsbuildinfo From 38f03dc1b81c3a85146be42fa6dfb760172d26c6 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Thu, 3 Sep 2026 19:00:52 +0900 Subject: [PATCH 05/19] test(task-persistence): make liveness-boundary tests filesystem-precision-independent --- .../TaskHistoryStore.reconciliation.spec.ts | 74 +++++++++++++------ 1 file changed, 50 insertions(+), 24 deletions(-) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 052468831c..60ea4b526b 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -183,22 +183,49 @@ describe("TaskHistoryStore reconcileDelegationState", () => { } /** - * Deterministic wall clock for liveness-boundary tests. `fs.utimes` accepts - * ms-precision Date values and the store's `Date.now()` is spied to return - * this same instant, so `Date.now() - mtimeMs` is exact regardless of how - * long the test body takes to run. + * Deterministic wall clock for liveness-boundary tests. The store's + * `Date.now()` is spied to return this same instant, and `setChildMtimeAge` + * additionally injects the exact mtime the store observes, so + * `Date.now() - mtimeMs` is exact regardless of how long the test body + * takes to run or how much millisecond precision the filesystem keeps. */ const FIXED_NOW = 1_756_886_400_000 // 2025-09-03T08:00:00.000Z + // Restored in afterEach so a leaked spy can never poison the direct + // `getChildFileMtimeMs` probe test. + let mtimeSpy: { mockRestore(): void } | undefined + + /** + * Stamps a child's history file so the store observes a mtime of exactly + * `FIXED_NOW - ageMs`, independent of filesystem mtime precision. + * + * Two layers: + * 1. Best-effort `fs.utimes` keeps the on-disk file realistic, but tests + * must NOT depend on it: some filesystems and CI runners truncate mtime + * to seconds, which would silently flip live/stale expectations. + * 2. A spy on the private `TaskHistoryStore.prototype.getChildFileMtimeMs` + * (the exact call path used by the cross-instance liveness guard) + * injects the intended millisecond value. That single `mtimeMs` feeds + * BOTH the `Date.now() - mtimeMs < threshold` guard and the + * `Math.round((Date.now() - mtimeMs) / 1000)` skip-log render, so the + * `<`-vs-`<=` boundary at 300_000 ms and the 300s/299s/-100s render + * assertions stay deterministic and keep killing their mutants on any + * filesystem. + * + * `ageMs` may be negative (future mtime). Other child ids delegate to the + * real implementation so unrelated probe paths keep exercising the FS. + */ async function setChildMtimeAge(taskId: string, ageMs: number): Promise { const filePath = path.join(tmpDir, "tasks", taskId, GlobalFileNames.historyItem) const stamp = new Date(FIXED_NOW - ageMs) await fs.utimes(filePath, stamp, stamp) - // Guard the assumption that the filesystem round-trips millisecond - // precision, so a boundary test failure is diagnosable rather than a - // silent live/stale flip. - const written = await fs.stat(filePath) - expect(Math.round(written.mtimeMs)).toBe(FIXED_NOW - ageMs) + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const original = probe.getChildFileMtimeMs + mtimeSpy = vi.spyOn(probe, "getChildFileMtimeMs").mockImplementation((childId: string) => + childId === taskId ? Promise.resolve(FIXED_NOW - ageMs) : original.call(store, childId), + ) } beforeEach(async () => { @@ -226,6 +253,8 @@ describe("TaskHistoryStore reconcileDelegationState", () => { }) afterEach(async () => { + mtimeSpy?.mockRestore() + mtimeSpy = undefined safeWriteJsonMock.mockImplementation(writeJson) for (const disposable of disposables) disposable.dispose() disposables.clear() @@ -543,10 +572,13 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // Files stamped 1970-01-01 (epoch-zero artifacts from misconfigured clocks, // zip extraction, or container images) must be treated as stale orphans. // Kills the ArithmeticOperator mutant `Date.now() - mtimeMs` -> - // `Date.now() % mtimeMs`: with mtimeMs = 1000, the real subtraction is - // ~56 years (stale -> repair), while FIXED_NOW % 1000 === 0 would be read - // as live and skip the repair. The same mutant inside the skip-path log is - // never reached under the mutant because the guard already diverges. + // `Date.now() % mtimeMs`: with the mocked mtimeMs = 1000, the real + // subtraction is ~56 years (stale -> repair), while FIXED_NOW % 1000 === 0 + // would be read as live and skip the repair. The same mutant inside the + // skip-path log is never reached under the mutant because the guard + // already diverges. The exact 1000 ms stamp comes from the mocked + // `getChildFileMtimeMs`, so no filesystem millisecond precision is + // assumed. const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) try { @@ -563,11 +595,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: "child-epoch-mtime", }) await seedItems([parent, child]) - const childFilePath = path.join(tmpDir, "tasks", "child-epoch-mtime", GlobalFileNames.historyItem) - const epochStamp = new Date(1_000) // 1970-01-01T00:00:01.000Z - await fs.utimes(childFilePath, epochStamp, epochStamp) - const written = await fs.stat(childFilePath) - expect(Math.round(written.mtimeMs)).toBe(1_000) + await setChildMtimeAge("child-epoch-mtime", FIXED_NOW - 1_000) // store observes mtime 1970-01-01T00:00:01.000Z await store.initialize() @@ -587,7 +615,9 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // ArithmeticOperator mutant `Date.now() % mtimeMs` would instead render // the whole epoch magnitude (1756886400s), so the seconds-count // assertion below kills it. The live-side status assertions also kill - // the same mutant on the guard subtraction in TaskHistoryStore.ts. + // the same mutant on the guard subtraction in TaskHistoryStore.ts. The + // exact future mtime is injected by the mocked `getChildFileMtimeMs`, + // independent of filesystem millisecond precision. const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) try { @@ -604,11 +634,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: "child-future-mtime", }) await seedItems([parent, child]) - const childFilePath = path.join(tmpDir, "tasks", "child-future-mtime", GlobalFileNames.historyItem) - const futureStamp = new Date(FIXED_NOW + 100_000) - await fs.utimes(childFilePath, futureStamp, futureStamp) - const written = await fs.stat(childFilePath) - expect(Math.round(written.mtimeMs)).toBe(FIXED_NOW + 100_000) + await setChildMtimeAge("child-future-mtime", -100_000) // store observes a mtime 100s in the future await store.initialize() From 72401e7d7f2460228c4d2694d8b3cd402cd9cc2f Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 08:07:02 +0900 Subject: [PATCH 06/19] fix(delegation): guard replayDelegationRepairIntent against live cross-window children --- src/core/task-persistence/TaskHistoryStore.ts | 23 ++- .../TaskHistoryStore.reconciliation.spec.ts | 176 +++++++++++++++++- 2 files changed, 195 insertions(+), 4 deletions(-) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index e3900111b0..7132f10d86 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -534,7 +534,10 @@ export class TaskHistoryStore { * Replay the durable active-child repair intent, if one was left by a crash. * The expected fields are guards: an intent may update only the missing side * when the other side is already at its target, or when both records still - * describe the original delegated handoff. + * describe the original delegated handoff. Before writing the child, the same + * cross-window liveness guard as `reconcileDelegationStateCore` applies: a + * child whose history file was touched recently belongs to another live + * window, so the stale intent is quarantined instead of replayed. * * This method acquires the store's non-reentrant promise-chain lock. It must be * called outside an existing `withLock` callback; locked callers must use the @@ -570,6 +573,24 @@ export class TaskHistoryStore { return } + // Cross-instance liveness guard (same convention as reconcileDelegationStateCore): + // if this window crashed mid-repair and another window restarted the same child, + // the child's history file is being actively persisted there. Replaying the stale + // intent would overwrite the live child as "interrupted", so quarantine it instead. + // Only enforced when the replay would actually write the child record: a child + // already at its target needs no write, and parent-only completion must not be + // blocked by child liveness. An unreadable mtime conservatively proceeds. + if (!childAtTarget) { + const mtimeMs = await this.getChildFileMtimeMs(child.id) + const isLiveElsewhere = + // Stryker disable next-line ConditionalExpression: replacing `mtimeMs !== undefined` with `true` is mutation-equivalent; with a defined mtimeMs `true && X === X`, and with undefined the right operand is `NaN < threshold === false`, identical to the short-circuit result. + mtimeMs !== undefined && Date.now() - mtimeMs < TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS + if (isLiveElsewhere) { + await this.quarantineDelegationRepairIntent(intent, "child live in another window (recent mtime)") + return + } + } + const repairedChild = childAtTarget ? child : { ...child, status: intent.target.childStatus } const repairedParent = parentMatchesTargetState ? parent diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 60ea4b526b..bea2392b75 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -223,9 +223,11 @@ describe("TaskHistoryStore reconcileDelegationState", () => { getChildFileMtimeMs: (childId: string) => Promise } const original = probe.getChildFileMtimeMs - mtimeSpy = vi.spyOn(probe, "getChildFileMtimeMs").mockImplementation((childId: string) => - childId === taskId ? Promise.resolve(FIXED_NOW - ageMs) : original.call(store, childId), - ) + mtimeSpy = vi + .spyOn(probe, "getChildFileMtimeMs") + .mockImplementation((childId: string) => + childId === taskId ? Promise.resolve(FIXED_NOW - ageMs) : original.call(store, childId), + ) } beforeEach(async () => { @@ -890,6 +892,9 @@ describe("TaskHistoryStore reconcileDelegationState", () => { await seedItems([parent, child]) const intentPath = path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) + // Keep this a crash-orphan replay: the child file must not look live in + // another window, or the cross-window liveness guard quarantines the intent. + await markStaleMtime(child.id) await store.reconcile({ forceRefresh: true }) const storeInternals = store as unknown as { @@ -1016,6 +1021,168 @@ describe("TaskHistoryStore reconcileDelegationState", () => { ).toBe(true) }) + it("quarantines a replay intent whose child is live in another window (recent mtime)", async () => { + // Reviewer scenario: this window crashed mid-repair (the intent is durable + // but the child write never landed), and another window then restarted the + // same child. The child's history file mtime is recent, so replaying the + // intent here would overwrite a live child as "interrupted". The intent must + // be quarantined instead, and the startup reconciliation liveness guard must + // likewise leave the delegation untouched. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const child = makeItem({ + id: "child-replay-live", + status: "active", + parentTaskId: "parent-replay-live", + rootTaskId: "parent-replay-live", + }) + const parent = makeItem({ + id: "parent-replay-live", + status: "delegated", + awaitingChildId: child.id, + delegatedToId: child.id, + }) + await seedItems([parent, child]) + const tasksDir = path.join(tmpDir, "tasks") + const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) + await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) + + // Another live window has just persisted the child. + const childFilePath = path.join(tasksDir, child.id, GlobalFileNames.historyItem) + const now = new Date() + await fs.utimes(childFilePath, now, now) + + await store.initialize() + + // Nothing may be written as "interrupted": child stays active and the + // parent keeps its delegation links. + expect(store.get(child.id)?.status).toBe("active") + expect(store.get(parent.id)?.status).toBe("delegated") + expect(store.get(parent.id)?.awaitingChildId).toBe(child.id) + expect(store.get(parent.id)?.delegatedToId).toBe(child.id) + + const persistedChild = JSON.parse(await fs.readFile(childFilePath, "utf8")) as HistoryItem + expect(persistedChild.status).toBe("active") + const persistedParent = JSON.parse( + await fs.readFile(path.join(tasksDir, parent.id, GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedParent.status).toBe("delegated") + expect(persistedParent.awaitingChildId).toBe(child.id) + + // The intent is moved out of the way rather than applied or left to retry. + await expect(fs.access(intentPath)).rejects.toThrow() + expect( + (await fs.readdir(tasksDir)).some((name) => + name.startsWith(`${GlobalFileNames.delegationRepairIntent}.quarantine-`), + ), + ).toBe(true) + // Kills the StringLiteral mutant on the new quarantine reason. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("child live in another window (recent mtime)")) + + warnSpy.mockRestore() + }) + + it("replays a repair intent whose child mtime is stale (crash orphan still repaired)", async () => { + // Regression guard for the replay liveness guard: a child file untouched for + // longer than the threshold is a genuine crash orphan, so the durable intent + // must still complete on restart — child → interrupted, parent → active. + const child = makeItem({ + id: "child-replay-stale", + status: "active", + parentTaskId: "parent-replay-stale", + rootTaskId: "parent-replay-stale", + }) + const parent = makeItem({ + id: "parent-replay-stale", + status: "delegated", + awaitingChildId: child.id, + delegatedToId: child.id, + }) + await seedItems([parent, child]) + const tasksDir = path.join(tmpDir, "tasks") + const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) + await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) + await markStaleMtime(child.id) + + await store.initialize() + + expect(store.get(child.id)?.status).toBe("interrupted") + expect(store.get(parent.id)?.status).toBe("active") + expect(store.get(parent.id)?.awaitingChildId).toBeUndefined() + expect(store.get(parent.id)?.delegatedToId).toBeUndefined() + const persistedChild = JSON.parse( + await fs.readFile(path.join(tasksDir, child.id, GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedChild.status).toBe("interrupted") + await expect(fs.access(intentPath)).rejects.toThrow() + expect( + (await fs.readdir(tasksDir)).some((name) => + name.startsWith(`${GlobalFileNames.delegationRepairIntent}.quarantine-`), + ), + ).toBe(false) + }) + + it("completes a parent-only replay while the child is live in another window (child already at target)", async () => { + // The guard must gate only actual child writes. Here the child is already at + // intent.target.childStatus, so no child write happens and the recent (live) + // mtime must not block the parent-side completion of the repair. + const child = makeItem({ + id: "child-replay-parent-only", + status: "interrupted", + parentTaskId: "parent-replay-parent-only", + }) + const parent = makeItem({ + id: "parent-replay-parent-only", + status: "delegated", + awaitingChildId: child.id, + delegatedToId: child.id, + }) + await seedItems([parent, child]) + const tasksDir = path.join(tmpDir, "tasks") + const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) + await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) + const now = new Date() + await fs.utimes(path.join(tasksDir, child.id, GlobalFileNames.historyItem), now, now) + + await store.initialize() + + expect(store.get(child.id)?.status).toBe("interrupted") + expect(store.get(parent.id)?.status).toBe("active") + expect(store.get(parent.id)?.awaitingChildId).toBeUndefined() + await expect(fs.access(intentPath)).rejects.toThrow() + }) + + it("proceeds with a replay when the child history file mtime is unreadable", async () => { + // Matches the reconcile-path convention: getChildFileMtimeMs returns + // undefined for a missing/unreadable history file, which conservatively + // proceeds with the repair instead of treating the child as live. + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + mtimeSpy = vi.spyOn(probe, "getChildFileMtimeMs").mockResolvedValue(undefined) + + const child = makeItem({ + id: "child-replay-unreadable", + status: "active", + parentTaskId: "parent-replay-unreadable", + }) + const parent = makeItem({ + id: "parent-replay-unreadable", + status: "delegated", + awaitingChildId: child.id, + delegatedToId: child.id, + }) + await seedItems([parent, child]) + const tasksDir = path.join(tmpDir, "tasks") + const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) + await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) + + await store.initialize() + + expect(store.get(child.id)?.status).toBe("interrupted") + expect(store.get(parent.id)?.status).toBe("active") + await expect(fs.access(intentPath)).rejects.toThrow() + }) + it("repairs invalid delegation: delegated parent with no awaitingChildId → active (clears delegatedToId and awaitingChildId)", async () => { // awaitingChildId is falsy but explicitly set (empty string), delegatedToId is stale const parent = makeItem({ @@ -1105,6 +1272,9 @@ describe("TaskHistoryStore reconcileDelegationState", () => { await seedItems([grandparent, parent, child]) const intentPath = path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) + // Crash-orphan scenario: the child must not look live in another window, or + // the replay/startup liveness guards would skip the repair entirely. + await markStaleMtime(child.id) await store.initialize() From b33954972bbb8439ddd5f439c5b5c1050d9aca66 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 09:23:46 +0900 Subject: [PATCH 07/19] fix(delegation): run delegation reconciliation on periodic reconcile ticks --- src/core/task-persistence/TaskHistoryStore.ts | 98 +++++- .../TaskHistoryStore.reconciliation.spec.ts | 287 ++++++++++++++++++ 2 files changed, 383 insertions(+), 2 deletions(-) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 7132f10d86..c71a7d0ea1 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -85,6 +85,21 @@ export class TaskHistoryStore { private writeLock: Promise = Promise.resolve() private fsWatcher: fsSync.FSWatcher | null = null private reconcileTimer: ReturnType | null = null + /** + * Serializes the periodic delegation-reconciliation step across ticks. The + * store lock already prevents interleaved mutations, but overlapping ticks + * would queue stale passes behind each other; skipping a tick instead lets + * the next interval retry with fresher data. + */ + private delegationTickRunning = false + /** + * Task ids this store instance itself persisted with an `active` status. + * Their task sessions live in this window, so periodic delegation + * reconciliation must exclude them from orphan-repair candidates. The set + * is per-instance by design: after a host restart the new store has no + * entries, so startup reconciliation keeps repairing genuine crash orphans. + */ + private readonly locallyActiveTaskIds = new Set() private disposed = false /** @@ -257,6 +272,12 @@ export class TaskHistoryStore { // Update in-memory cache with what was actually persisted this.cache.set(written.id, written) + // Only runtime writes (not `skipTransitionCheck` administrative repairs) + // prove a live task session runs in THIS window; repairs go through the + // same core but must not suppress future orphan reconciliation. + if (!options.skipTransitionCheck) { + this.trackLocalSessionOwnership(written) + } const all = this.getAll() @@ -275,6 +296,7 @@ export class TaskHistoryStore { return this.withLock(async () => { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) // Remove per-task file (best-effort) try { @@ -299,6 +321,7 @@ export class TaskHistoryStore { for (const taskId of taskIds) { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) try { const filePath = await this.getTaskFilePath(taskId) @@ -393,8 +416,9 @@ export class TaskHistoryStore { /** * Repair delegation inconsistencies left by a crash mid-transition. * - * Called once from `initialize()` after `reconcile()`. Runs inside `withLock` to - * prevent interleaving with watcher-triggered reconcile() calls. Iterates until + * Called from `initialize()` and from each periodic reconciliation tick, + * always after `reconcile()`. Runs inside `withLock` to prevent interleaving + * with watcher-triggered reconcile() calls. Iterates until * convergence so that one-level chained delegations visible at startup are resolved. * * Must NOT be called from within `withLock` — `withLock` is non-reentrant (promise @@ -530,6 +554,22 @@ export class TaskHistoryStore { ) } + /** + * Maintain the set of task ids whose live session runs in THIS window. + * A record this store persisted as active belongs to a task running here, + * so the periodic delegation pass must never treat it as a crash orphan — + * its history-file mtime can legitimately go quiet for minutes while the + * task streams a long model turn or waits on a user prompt. Any non-active + * status write ends that ownership. + */ + private trackLocalSessionOwnership(written: HistoryItem): void { + if ((written.status ?? "active") === "active") { + this.locallyActiveTaskIds.add(written.id) + } else { + this.locallyActiveTaskIds.delete(written.id) + } + } + /** * Replay the durable active-child repair intent, if one was left by a crash. * The expected fields are guards: an intent may update only the missing side @@ -978,6 +1018,13 @@ export class TaskHistoryStore { /** * Start periodic reconciliation as a defensive fallback for platforms * where fs.watch is unreliable. + * + * Each tick refreshes disk→cache via `reconcile()` and then re-runs the same + * delegation repair `initialize()` performs, so a child that skipped repair + * at startup (recent mtime = live in another window) but crashes afterwards + * is caught within one interval instead of waiting for the next extension + * host restart. Intent replay is intentionally NOT part of the tick: the + * durable repair journal is replayed at startup by design. */ private startPeriodicReconciliation(): void { if (this.disposed) { @@ -993,10 +1040,54 @@ export class TaskHistoryStore { } catch (err) { console.error("[TaskHistoryStore] Periodic reconciliation failed:", err) } + try { + await this.runPeriodicDelegationReconciliation() + } catch (err) { + console.error("[TaskHistoryStore] Periodic delegation reconciliation failed:", err) + } this.startPeriodicReconciliation() }, TaskHistoryStore.RECONCILE_INTERVAL_MS) } + /** + * One delegation-reconciliation pass for a periodic tick. + * + * Mirrors the `initialize()` sequence: capture which active task ids exist + * in persisted state (the cache was just refreshed from disk by + * `reconcile()` and no repair has mutated statuses yet), then run the + * reconciliation against that snapshot. The child-mtime liveness guard + * inside `reconcileDelegationStateCore` protects children actively written + * by another window, so ticking is safe for multi-window workspaces. + * + * One mid-session-only refinement over the startup snapshot: ids this + * window itself persisted as active are excluded. At startup no local + * sessions exist, so an active child on disk implies a previous host + * crashed; mid-session, an active child that THIS store wrote belongs to a + * live task here, and a quiet-but-live mtime (long model turn, user + * deliberating over an ask) must not cause it to be repaired away from + * under its own runner. Genuine crashes of this window take the tick with + * them and are handled by the next startup pass instead. + * + * `reconcileDelegationState` acquires the non-reentrant `withLock` chain + * itself (same entry point `initialize()` uses); this method never holds + * the lock. The running flag only guards snapshot→pass adjacency and skips + * (rather than queues) a tick whose previous pass is still in flight. + */ + private async runPeriodicDelegationReconciliation(): Promise { + if (this.disposed || this.delegationTickRunning) { + return + } + this.delegationTickRunning = true + try { + const persistedActiveIds = new Set( + Array.from(this.getPersistedActiveIds()).filter((id) => !this.locallyActiveTaskIds.has(id)), + ) + await this.reconcileDelegationState(persistedActiveIds) + } finally { + this.delegationTickRunning = false + } + } + // ────────────────────────────── Atomic read-modify-write ────────────────────────────── /** @@ -1087,12 +1178,15 @@ export class TaskHistoryStore { // First record is committed on disk. Update cache so it // reflects disk state before propagating the error. this.cache.set(firstId, writtenFirst) + this.trackLocalSessionOwnership(writtenFirst) throw error } // Both disk writes succeeded — now update the cache. this.cache.set(firstId, writtenFirst) this.cache.set(secondId, writtenSecond) + this.trackLocalSessionOwnership(writtenFirst) + this.trackLocalSessionOwnership(writtenSecond) const all = this.getAll() if (this.onWrite) { diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index bea2392b75..bfc9292c39 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -1601,3 +1601,290 @@ describe("TaskHistoryStore upsert transition guard", () => { ).rejects.toThrow("Invalid task status transition: delegated → completed") }) }) + +// ───────────────────────────────────────────────────────────────────────────── +// startPeriodicReconciliation — delegation repair on each tick (review item #2) +// ───────────────────────────────────────────────────────────────────────────── + +describe("TaskHistoryStore periodic delegation reconciliation", () => { + let tmpDir: string + let store: TaskHistoryStore | undefined + let mtimeSpy: { mockRestore(): void } | undefined + + // Private static interval used to advance the fake clock by exactly one tick. + // There is no typed accessor; this casts through `unknown` (not `as any`) + // following the same private-member access pattern used for + // LIVE_CHILD_MTIME_THRESHOLD_MS at the top of this spec. + const RECONCILE_INTERVAL_MS = (TaskHistoryStore as unknown as { RECONCILE_INTERVAL_MS: number }) + .RECONCILE_INTERVAL_MS + + const CHILD_ID = "child-tick" + const PARENT_ID = "parent-tick" + + /** + * Fake only what the tick scheduling needs: the 5-minute `setTimeout` clock + * and `Date` (consumed by the liveness guard). Everything else (fs I/O, + * microtasks) stays real so `flushAsyncWork()` below can pump the event + * loop while the timer clock advances only 1 ms per yield. + */ + function useTickClock(): void { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }) + } + + /** + * Drain pending real fs I/O. The tick's reconcile/repair chain completes on + * libuv callbacks that fake timers alone never advance, and each + * `advanceTimersByTimeAsync(1)` yields one REAL macrotask turn (processing + * the poll phase) while advancing the fake clock only 1 ms. The total fake + * time here stays far below RECONCILE_INTERVAL_MS, so no extra tick fires + * during the pump — this only lets in-flight fs callbacks settle. The count + * is generous to absorb Windows antivirus/OneDrive fs latency. + */ + async function flushAsyncWork(yields = 2000): Promise { + for (let i = 0; i < yields; i++) { + await vi.advanceTimersByTimeAsync(1) + } + } + + async function seedItems(items: HistoryItem[]): Promise { + const tasksDir = path.join(tmpDir, "tasks") + await fs.mkdir(tasksDir, { recursive: true }) + for (const item of items) { + const taskDir = path.join(tasksDir, item.id) + await fs.mkdir(taskDir, { recursive: true }) + await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item)) + } + } + + /** + * Stateful mtime injection for the liveness guard. The guard computes + * `Date.now() - mtimeMs` against the (fake) clock, and this injector returns + * `Date.now() - childAgeMs` at call time, so flipping `childAgeMs` between + * the startup pass and a periodic tick deterministically models "live in + * another window at startup, then crashed before the next tick". Exact + * regardless of filesystem mtime precision, same convention as + * `setChildMtimeAge` above. Other child ids delegate to the real + * implementation so unrelated probe paths keep exercising the FS. + */ + let childAgeMs = 0 + function installChildAgeInjector(): void { + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const original = probe.getChildFileMtimeMs + mtimeSpy = vi + .spyOn(probe, "getChildFileMtimeMs") + .mockImplementation((childId: string) => + childId === CHILD_ID ? Promise.resolve(Date.now() - childAgeMs) : original.call(store!, childId), + ) + } + + function makeDelegatedPair(): HistoryItem[] { + const child = makeItem({ + id: CHILD_ID, + status: "active", + parentTaskId: PARENT_ID, + rootTaskId: PARENT_ID, + }) + const parent = makeItem({ + id: PARENT_ID, + status: "delegated", + awaitingChildId: CHILD_ID, + delegatedToId: CHILD_ID, + childIds: [CHILD_ID], + }) + return [parent, child] + } + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "periodic-deleg-test-")) + childAgeMs = 60_000 + }) + + afterEach(async () => { + mtimeSpy?.mockRestore() + mtimeSpy = undefined + store?.dispose() + store = undefined + vi.useRealTimers() + await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {}) + }) + + it("repairs an active child whose mtime goes stale between startup and the next tick (the reported bug)", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const [parent, child] = makeDelegatedPair() + await seedItems([parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + installChildAgeInjector() + useTickClock() + // Child looks live at startup (written 60s ago by another window) → startup skips repair. + childAgeMs = 60_000 + await s.initialize() + expect(errorSpy).not.toHaveBeenCalled() + + expect(s.get(CHILD_ID)?.status).toBe("active") + expect(s.get(PARENT_ID)?.status).toBe("delegated") + expect(s.get(PARENT_ID)?.awaitingChildId).toBe(CHILD_ID) + + // The owning window crashes: nobody rewrites the child file, so by the + // next periodic tick its mtime is past the liveness threshold. + childAgeMs = 10 * 60 * 1000 + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushAsyncWork() + + // Within ONE interval, the parent window must repair: child → interrupted, + // parent → active with delegation links cleared. + expect(s.get(CHILD_ID)).toMatchObject({ id: CHILD_ID, status: "interrupted", parentTaskId: PARENT_ID }) + expect(s.get(PARENT_ID)).toMatchObject({ id: PARENT_ID, status: "active" }) + expect(s.get(PARENT_ID)?.awaitingChildId).toBeUndefined() + expect(s.get(PARENT_ID)?.delegatedToId).toBeUndefined() + + // Repaired on disk, not just in the cache. + const persistedChild = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", CHILD_ID, GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + const persistedParent = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", PARENT_ID, GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedChild.status).toBe("interrupted") + expect(persistedParent.status).toBe("active") + expect(persistedParent.awaitingChildId).toBeUndefined() + + // The warn message proves the DELEGATION pass (not the plain cache + // reconcile) ran inside the tick, and the tick raised no errors. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Reconciled orphaned active child")) + expect(errorSpy).not.toHaveBeenCalled() + + // Re-arm must survive the successful tick so the loop keeps running. + const internals = s as unknown as { reconcileTimer: ReturnType | null } + expect(internals.reconcileTimer).not.toBeNull() + + warnSpy.mockRestore() + errorSpy.mockRestore() + }) + + it("never repairs a child this window itself persisted as active, even with a stale mtime (in-window delegation)", async () => { + // Startup has no local sessions, so an active child on disk implies a + // crashed host and is a valid repair target. Mid-session that inference + // breaks: a child running IN THIS WINDOW (e.g. an in-window delegation) + // can go minutes without rewriting its history file while it streams a + // long turn or waits on a user prompt. The tick must not tear it away + // from its own runner — only children this store never wrote active + // (i.e. loaded from disk, owned elsewhere) are orphan candidates. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const child = makeItem({ + id: CHILD_ID, + status: "active", + parentTaskId: PARENT_ID, + rootTaskId: PARENT_ID, + }) + const parent = makeItem({ id: PARENT_ID, status: "active" }) + + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + await s.initialize() + + // This window creates the parent and the child, then delegates. + await s.upsert(parent) + await s.upsert(child) + await s.atomicReadAndUpdate(PARENT_ID, (current) => ({ + ...current, + status: "delegated" as const, + awaitingChildId: CHILD_ID, + delegatedToId: CHILD_ID, + })) + expect(s.get(PARENT_ID)?.status).toBe("delegated") + + // Even though the child's mtime looks stale, it is owned HERE. + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const realProbe = probe.getChildFileMtimeMs + mtimeSpy = vi + .spyOn(probe, "getChildFileMtimeMs") + .mockImplementation((childId: string) => + childId === CHILD_ID ? Promise.resolve(Date.now() - 10 * 60 * 1000) : realProbe.call(s, childId), + ) + + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushAsyncWork() + + expect(s.get(CHILD_ID)?.status).toBe("active") + expect(s.get(PARENT_ID)?.status).toBe("delegated") + expect(s.get(PARENT_ID)?.awaitingChildId).toBe(CHILD_ID) + expect(errorSpy).not.toHaveBeenCalled() + + errorSpy.mockRestore() + }) + + it("does not repair a child that stays live across the periodic tick (no cross-window clobbering)", async () => { + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + const [parent, child] = makeDelegatedPair() + await seedItems([parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + installChildAgeInjector() + useTickClock() + childAgeMs = 60_000 + await s.initialize() + // Startup also logs the skip; clear so remaining calls come from the tick. + logSpy.mockClear() + + // The other window keeps writing: the child stays live at tick time. + childAgeMs = 60_000 + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushAsyncWork() + + // Nothing may be repaired: child stays active, parent keeps its delegation links. + expect(s.get(CHILD_ID)?.status).toBe("active") + expect(s.get(PARENT_ID)?.status).toBe("delegated") + expect(s.get(PARENT_ID)?.awaitingChildId).toBe(CHILD_ID) + expect(s.get(PARENT_ID)?.delegatedToId).toBe(CHILD_ID) + + const persistedChild = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", CHILD_ID, GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedChild.status).toBe("active") + + // The skip log proves the tick ran delegation reconciliation and the + // liveness guard protected the other window's child. + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining(`Skipping repair for live child ${CHILD_ID}`)) + + logSpy.mockRestore() + }) + + it("logs and keeps re-arming when the periodic delegation step throws", async () => { + const [parent, child] = makeDelegatedPair() + await seedItems([parent, child]) + + const internals = TaskHistoryStore.prototype as unknown as { + runPeriodicDelegationReconciliation: () => Promise + } + const throwingSpy = vi + .spyOn(internals, "runPeriodicDelegationReconciliation") + .mockRejectedValue(new Error("tick delegation boom")) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + await s.initialize() + + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushAsyncWork() + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("Periodic delegation reconciliation failed"), + expect.objectContaining({ message: "tick delegation boom" }), + ) + + // One more interval still fires the delegation step: the recursive + // re-arm is preserved even though the step threw. + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushAsyncWork() + expect(throwingSpy).toHaveBeenCalledTimes(2) + + errorSpy.mockRestore() + throwingSpy.mockRestore() + }) +}) From 365f97c2afbee16763d96862d55da163f4b1ccaa Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 09:31:40 +0900 Subject: [PATCH 08/19] fix(delegation): use console.warn for live-child skip log --- src/core/task-persistence/TaskHistoryStore.ts | 2 +- .../TaskHistoryStore.reconciliation.spec.ts | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index c71a7d0ea1..0cc6091d91 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -505,7 +505,7 @@ export class TaskHistoryStore { mtimeMs !== undefined && Date.now() - mtimeMs < TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS if (isLiveElsewhere) { - console.log( + console.warn( `[TaskHistoryStore] Skipping repair for live child ${child.id} ` + `(mtime ${Math.round((Date.now() - mtimeMs) / 1000)}s ago) — owned by another window`, ) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index bfc9292c39..4fe18edb44 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -390,7 +390,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { }) it("skips repair for active child with recent mtime (live in another window)", async () => { - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) const child = makeItem({ id: "child-live", status: "active", @@ -532,7 +532,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // 1000) renders "300", while `* 1000`, `+ 1000`, `- 1000` and // `% 1000` all render a different second count. const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) try { const child = makeItem({ id: "child-boundary-live", @@ -582,7 +582,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // `getChildFileMtimeMs`, so no filesystem millisecond precision is // assumed. const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) try { const child = makeItem({ id: "child-epoch-mtime", @@ -621,7 +621,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // exact future mtime is injected by the mocked `getChildFileMtimeMs`, // independent of filesystem millisecond precision. const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) try { const child = makeItem({ id: "child-future-mtime", @@ -657,7 +657,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // this one covers the ceil mutant. It also re-asserts the live side of // the strict `<` boundary and the line-105 threshold mutants. const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) try { const child = makeItem({ id: "child-boundary-ceil", @@ -1820,7 +1820,7 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { }) it("does not repair a child that stays live across the periodic tick (no cross-window clobbering)", async () => { - const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}) + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) const [parent, child] = makeDelegatedPair() await seedItems([parent, child]) From d06cc7583b4e2c94bf321b31ba724f8be93499e6 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 09:40:50 +0900 Subject: [PATCH 09/19] test(delegation): route undefined child mtime through initialize() end-to-end --- .../TaskHistoryStore.reconciliation.spec.ts | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 4fe18edb44..2827fa3b0a 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -486,6 +486,98 @@ describe("TaskHistoryStore reconcileDelegationState", () => { warnSpy.mockRestore() }) + it("routes an undefined getChildFileMtimeMs through initialize() and still repairs the active child", async () => { + // End-to-end companion to the direct-helper probe test above ("returns + // the file mtime for an existing child and undefined for a missing + // one"): that test covers the helper in isolation; this one feeds the + // same `undefined` return through `reconcileDelegationStateCore` via + // `initialize()` and asserts the conservative-repair contract fires — + // an unreadable mtime must NOT be treated as "live in another window", + // the child is repaired to interrupted and the parent back to active. + // A mutant that flips the `mtimeMs !== undefined` short-circuit (e.g. + // treating missing mtimes as live) would skip the repair and break the + // status assertions and the negative skip-log assertion below. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + // Private-instance access follows the documented double-assertion + // pattern used by the probe test and the repair-intent replay tests. + const internals = store as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const originalGetChildFileMtimeMs = internals.getChildFileMtimeMs + const mtimeUndefinedSpy = vi + .spyOn(internals, "getChildFileMtimeMs") + .mockImplementation((childId: string) => + childId === "child-undef-mtime" + ? Promise.resolve(undefined) + : originalGetChildFileMtimeMs.call(store, childId), + ) + try { + const child = makeItem({ + id: "child-undef-mtime", + status: "active", + parentTaskId: "parent-undef-mtime", + rootTaskId: "parent-undef-mtime", + childIds: ["grandchild-undef-mtime"], + }) + const parent = makeItem({ + id: "parent-undef-mtime", + status: "delegated", + awaitingChildId: "child-undef-mtime", + delegatedToId: "child-undef-mtime", + childIds: ["child-undef-mtime"], + }) + // The child file is seeded normally (fresh mtime, and present in + // persistedActiveIds); only the stat probe is forced to undefined, + // simulating a file that races away or is unreadable at the moment + // the liveness guard checks it. + await seedItems([parent, child]) + + await store.initialize() + + // Spy must have been exercised through the real reconciliation path. + expect(mtimeUndefinedSpy).toHaveBeenCalledWith("child-undef-mtime") + + const repairedChild = store.get("child-undef-mtime") + const repairedParent = store.get("parent-undef-mtime") + expect(repairedChild).toMatchObject({ + id: "child-undef-mtime", + status: "interrupted", + parentTaskId: "parent-undef-mtime", + rootTaskId: "parent-undef-mtime", + childIds: ["grandchild-undef-mtime"], + }) + expect(repairedParent).toMatchObject({ id: "parent-undef-mtime", status: "active" }) + expect(repairedParent?.awaitingChildId).toBeUndefined() + expect(repairedParent?.delegatedToId).toBeUndefined() + + // Persisted state must match the cache, same as the stale-mtime test. + const tasksDir = path.join(tmpDir, "tasks") + const persistedChild = JSON.parse( + await fs.readFile(path.join(tasksDir, "child-undef-mtime", "history_item.json"), "utf8"), + ) as HistoryItem + const persistedParent = JSON.parse( + await fs.readFile(path.join(tasksDir, "parent-undef-mtime", "history_item.json"), "utf8"), + ) as HistoryItem + expect(persistedChild).toMatchObject({ + id: "child-undef-mtime", + status: "interrupted", + parentTaskId: "parent-undef-mtime", + rootTaskId: "parent-undef-mtime", + }) + expect(persistedParent).toMatchObject({ id: "parent-undef-mtime", status: "active" }) + expect(persistedParent.awaitingChildId).toBeUndefined() + expect(persistedParent.delegatedToId).toBeUndefined() + + // Repair ran and the liveness-skip branch was NOT taken. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Reconciled orphaned active child")) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("child-undef-mtime")) + expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child")) + } finally { + mtimeUndefinedSpy.mockRestore() + warnSpy.mockRestore() + } + }) + it("repairs when child file age is exactly the liveness threshold (strict '<' boundary)", async () => { // Kills the TaskHistoryStore.ts line-481 EqualityOperator mutant `<=`: // under `<=`, age === threshold (300000 ms) would count as live and the From 34a4b87da02df0a3262ea2fb7682e978dca4182f Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 09:55:29 +0900 Subject: [PATCH 10/19] test(lifecycle): model cross-window child liveness guard in lifecycle:model-check --- docs/architecture/task-lifecycle-model.md | 25 +-- scripts/check-task-lifecycle.ts | 200 +++++++++++++++++++--- 2 files changed, 189 insertions(+), 36 deletions(-) diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index d39c0fd9e2..c3473fb14a 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -35,17 +35,19 @@ TLA+/PlusCal or Quint with TLC becomes a better fit when the lifecycle needs tem ## Production mapping -| Model concept | Production concept | -| ------------------------- | ------------------------------------------------------------------------------------ | -| Task record and status | `HistoryItem` persisted by `TaskHistoryStore` | -| `delegate(parent, child)` | `ClineProvider.delegateParentAndOpenChild` | -| `interrupt(child)` | cancellation or eviction through `markDelegatedChildInterrupted` | -| `complete(child)` | `ClineProvider.reopenParentFromDelegation` | -| `abandon(child)` | `ClineProvider.abandonSubtask` | -| Atomic event step | `atomicReadAndUpdate`, `atomicUpdatePair`, and per-parent delegation transition lock | -| Event interleaving | Competing completion, cancellation, abandonment, and new delegation calls | - -The model has three fixed task slots, enough to cover competing siblings and a nested parent-child-grandchild chain. It explores every reachable interleaving through depth 12, deduplicating canonical states. Representative checks also exercise rejected operations that do not create a new state: a second concurrent delegation while the first child is active, stale completion after re-delegation, late completion after abandonment, completion after interruption, and nested completion. Named semantic landmarks require the graph to retain interrupted-child re-delegation and nested delegation even when the raw state total changes. +| Model concept | Production concept | +| --------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------ | +| Task record and status | `HistoryItem` persisted by `TaskHistoryStore` | +| `delegate(parent, child)` | `ClineProvider.delegateParentAndOpenChild` | +| `interrupt(child)` | cancellation or eviction through `markDelegatedChildInterrupted` | +| `complete(child)` | `ClineProvider.reopenParentFromDelegation` | +| `abandon(child)` | `ClineProvider.abandonSubtask` | +| `reconcileStartup(parent)` | startup/periodic `TaskHistoryStore.reconcileDelegationStateCore` orphan repair | +| `markLiveElsewhere(child)` / `expireLiveElsewhere(child)` | child history-file mtime recent vs stale past `LIVE_CHILD_MTIME_THRESHOLD_MS` (abstracted; no wall clock in model) | +| Atomic event step | `atomicReadAndUpdate`, `atomicUpdatePair`, and per-parent delegation transition lock | +| Event interleaving | Competing completion, cancellation, abandonment, and new delegation calls | + +The model has three fixed task slots, enough to cover competing siblings and a nested parent-child-grandchild chain, plus one abstract boolean per slot recording whether an active child's session is owned by another window (recent history-file mtime). It explores every reachable interleaving through depth 12, deduplicating canonical states. Representative checks also exercise rejected operations that do not create a new state: a second concurrent delegation while the first child is active, stale completion after re-delegation, late completion after abandonment, completion after interruption, and nested completion. Named semantic landmarks require the graph to retain interrupted-child re-delegation and nested delegation even when the raw state total changes, a delegated parent whose active child is live in another window surviving startup reconciliation unchanged, and a stale-mtime (crash-orphan) active child being repaired to `interrupted` with the parent returned to `active` only through `reconcileStartup`. Production completion also accepts a recovery-compatible `active` parent that still awaits the returning child, then clears the stale pointers. Normal model transitions never create that intermediate state, so it is covered by a focused reducer test rather than admitted as a generally valid reachable state. @@ -106,6 +108,7 @@ The task delegation checker currently enforces: 5. Parent-child lineage is acyclic. 6. Completed task records cannot be changed by later lifecycle events. 7. Active-child re-delegation, stale completion after ownership moves to another child, duplicate/late completion, and abandonment of a live child are rejected by the shared production guards. +8. No transition may clear a delegated parent's link to a child that is active and marked live-elsewhere; startup reconciliation repairs only stale-or-unreadable-mtime (crash-orphan) children. This encodes the PR #1495 cross-window misrepair bug class, which broke delegation links so subtask completion could not return to the parent. The completion persistence checker additionally enforces: diff --git a/scripts/check-task-lifecycle.ts b/scripts/check-task-lifecycle.ts index 73e9078366..d0108f6fd6 100644 --- a/scripts/check-task-lifecycle.ts +++ b/scripts/check-task-lifecycle.ts @@ -11,7 +11,24 @@ import { const taskIds = ["parent", "child-a", "child-b"] as const type TaskId = (typeof taskIds)[number] -type ModelState = Record +type TaskMap = Record + +/** + * Abstract cross-window liveness flag. Production decides whether an active + * child awaited by a delegated parent belongs to another live window by + * comparing the child's history-file mtime against a 5-minute threshold + * (`TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS`). The model never reads + * wall-clock time: `liveElsewhere[child]` is true exactly when the modeled + * mtime is "recent" (the child is owned by another window) and false when it + * is "stale" or unreadable (the child is a crash orphan, repaired + * conservatively). + */ +type LivenessMap = Record + +interface ModelState { + tasks: TaskMap + liveElsewhere: LivenessMap +} interface Transition { name: string @@ -25,17 +42,55 @@ interface TraceStep { const MAX_DEPTH = 12 const MAX_STATES = 10_000 -const expectedActions = ["delegate", "interrupt", "complete", "abandon"] as const +const expectedActions = [ + "delegate", + "interrupt", + "complete", + "abandon", + "markLiveElsewhere", + "expireLiveElsewhere", + "reconcileStartup", +] as const const semanticLandmarks = { "interrupted-child-redelegation": (state: ModelState) => - state.parent?.status === "delegated" && - state.parent.awaitingChildId === "child-b" && - state["child-a"]?.status === "interrupted", + state.tasks.parent?.status === "delegated" && + state.tasks.parent.awaitingChildId === "child-b" && + state.tasks["child-a"]?.status === "interrupted", "nested-delegation": (state: ModelState) => - state.parent?.status === "delegated" && - state.parent.awaitingChildId === "child-a" && - state["child-a"]?.status === "delegated" && - state["child-a"].awaitingChildId === "child-b", + state.tasks.parent?.status === "delegated" && + state.tasks.parent.awaitingChildId === "child-a" && + state.tasks["child-a"]?.status === "delegated" && + state.tasks["child-a"].awaitingChildId === "child-b", + // Proves the fix for the cross-window misrepair bug (PR #1495): startup + // reconciliation must leave a delegated parent awaiting an active child + // owned by another window untouched. The reconciliation skip is an identity + // transition, so this landmark plus the universal transition invariant in + // `checkTransitionInvariants` (no reachable action may clear the link while + // the child is active and live-elsewhere) formalizes "not repaired". + "live-child-preserved-across-reconciliation": (state: ModelState) => { + const parent = state.tasks.parent + if (parent?.status !== "delegated" || !parent.awaitingChildId) { + return false + } + const childId = parent.awaitingChildId as TaskId + return state.tasks[childId]?.status === "active" && state.liveElsewhere[childId] + }, + // Proves the repair half of the same reconciliation outcome still works: a + // non-live (crash-orphan) active child is repaired to interrupted while the + // parent resumes as active with both delegation pointers cleared. This + // state class is only reachable through `reconcileStartup`, never through + // `interrupt`/`abandon`/`complete`. + "crash-orphan-repaired-by-startup": (state: ModelState) => { + const parent = state.tasks.parent + const child = state.tasks["child-a"] + return ( + parent?.status === "active" && + !parent.awaitingChildId && + child?.status === "interrupted" && + child.parentTaskId === "parent" && + !state.liveElsewhere["child-a"] + ) + }, } satisfies Record boolean> function task(id: TaskId, parentTaskId?: TaskId): HistoryItem { @@ -55,24 +110,29 @@ function task(id: TaskId, parentTaskId?: TaskId): HistoryItem { } function initialState(): ModelState { - return { parent: task("parent"), "child-a": undefined, "child-b": undefined } + return { + tasks: { parent: task("parent"), "child-a": undefined, "child-b": undefined }, + liveElsewhere: { parent: false, "child-a": false, "child-b": false }, + } } function replace(state: ModelState, ...updates: HistoryItem[]): ModelState { - const next = { ...state } - for (const update of updates) next[update.id as TaskId] = update - return next + const tasks = { ...state.tasks } + for (const update of updates) tasks[update.id as TaskId] = update + return { tasks, liveElsewhere: state.liveElsewhere } } function transitions(state: ModelState): Transition[] { const result: Transition[] = [] for (const parentId of taskIds) { - const parent = state[parentId] + const parent = state.tasks[parentId] if (!parent) continue for (const childId of taskIds) { - if (childId === parentId || state[childId]) continue - const awaitedStatus = parent.awaitingChildId ? state[parent.awaitingChildId as TaskId]?.status : undefined + if (childId === parentId || state.tasks[childId]) continue + const awaitedStatus = parent.awaitingChildId + ? state.tasks[parent.awaitingChildId as TaskId]?.status + : undefined if (parent.status !== "active" && !(parent.status === "delegated" && awaitedStatus === "interrupted")) { continue } @@ -85,11 +145,18 @@ function transitions(state: ModelState): Transition[] { } for (const childId of taskIds) { - const child = state[childId] + const child = state.tasks[childId] if (!child?.parentTaskId) continue - const parent = state[child.parentTaskId as TaskId] + const parent = state.tasks[child.parentTaskId as TaskId] if (!parent) continue + // A child marked live-elsewhere is owned by another window's session, so + // window-local lifecycle operations cannot target it until the flag + // expires. `checkTransitionInvariants` re-proves universally that no + // reachable action clears the parent's link while the child is active + // and live-elsewhere. + if (state.liveElsewhere[childId]) continue + if (parent.status === "delegated" && parent.awaitingChildId === child.id && child.status === "active") { const interrupted = interruptDelegatedChild(parent, child) result.push({ name: `interrupt(${childId})`, next: replace(state, interrupted) }) @@ -115,13 +182,77 @@ function transitions(state: ModelState): Transition[] { }) } } + + // Cross-window startup reconciliation (`TaskHistoryStore.reconcileDelegationStateCore`, + // run at initialize() and on every periodic tick). For every delegated parent + // whose awaited child is active, the outcome is decided solely by the + // abstract liveness flag: + // - stale/unreadable mtime (not live-elsewhere) → repair: child → interrupted + // via the shared production reducer, parent → active with both delegation + // pointers cleared. The parent-side rewrite is modeled directly here + // because production performs it as administrative recovery through + // `upsertCore(..., { skipTransitionCheck: true })`, outside the shared + // `taskLifecycle.ts` reducers; the child side matches `interruptDelegatedChild`. + // - recent mtime (live-elsewhere) → skip: the pre-fix bug repaired exactly + // this child, breaking the delegation link so the subtask's completion + // could no longer return to the parent. The fix `continue`s, so the + // action stays observable (it still marks `reconcileStartup` as executed) + // while intentionally not producing a new state. + for (const parentId of taskIds) { + const parent = state.tasks[parentId] + if (parent?.status !== "delegated" || !parent.awaitingChildId) continue + const childId = parent.awaitingChildId as TaskId + const child = state.tasks[childId] + if (child?.status !== "active") continue + if (state.liveElsewhere[childId]) { + result.push({ name: `reconcileStartup(${parentId})`, next: state }) + continue + } + const repairedParent: HistoryItem = { + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + } + const repairedChild = interruptDelegatedChild(parent, child) + result.push({ + name: `reconcileStartup(${parentId})`, + next: replace(state, repairedParent, repairedChild), + }) + } + + // Model actions for the abstract mtime liveness flag: `markLiveElsewhere` + // represents another window actively persisting the child (recent mtime), + // and `expireLiveElsewhere` represents the owning window going quiet past + // the threshold (e.g. it crashed after startup skipped its repair), after + // which the next `reconcileStartup` repairs it as a crash orphan. Only + // active tasks that are themselves children can toggle the flag; the root + // slot has no owning window in this bug class, and restricting the flag to + // child sessions keeps the liveness dimension from multiplying the state + // space beyond the explicit budget. + for (const id of taskIds) { + const current = state.tasks[id] + if (current?.status !== "active" || !current.parentTaskId) continue + const id2 = id as TaskId + if (!state.liveElsewhere[id2]) { + result.push({ + name: `markLiveElsewhere(${id2})`, + next: { tasks: state.tasks, liveElsewhere: { ...state.liveElsewhere, [id2]: true } }, + }) + } else { + result.push({ + name: `expireLiveElsewhere(${id2})`, + next: { tasks: state.tasks, liveElsewhere: { ...state.liveElsewhere, [id2]: false } }, + }) + } + } return result } function invariantViolations(state: ModelState): string[] { const violations: string[] = [] for (const id of taskIds) { - const current = state[id] + const current = state.tasks[id] if (!current) continue if (current.status === "delegated") { @@ -129,7 +260,7 @@ function invariantViolations(state: ModelState): string[] { violations.push(`${id}: delegated task must point to exactly one awaited child`) continue } - const child = state[current.awaitingChildId as TaskId] + const child = state.tasks[current.awaitingChildId as TaskId] if (!child || child.parentTaskId !== id || child.status === "completed") { violations.push(`${id}: awaited child must exist, link back, and not be completed`) } @@ -141,7 +272,7 @@ function invariantViolations(state: ModelState): string[] { } if (current.parentTaskId && current.status !== "interrupted") { - const parent = state[current.parentTaskId as TaskId] + const parent = state.tasks[current.parentTaskId as TaskId] if (current.status !== "completed" && parent?.awaitingChildId !== id) { violations.push(`${id}: active or delegated linked child must be the child its parent awaits`) } @@ -155,14 +286,14 @@ function invariantViolations(state: ModelState): string[] { break } ancestors.add(cursor) - cursor = state[cursor as TaskId]?.parentTaskId + cursor = state.tasks[cursor as TaskId]?.parentTaskId } } return violations } function canonical(state: ModelState): string { - return JSON.stringify(taskIds.map((id) => state[id] ?? null)) + return JSON.stringify([taskIds.map((id) => state.tasks[id] ?? null), taskIds.map((id) => state.liveElsewhere[id])]) } function formatCounterexample(message: string, trace: TraceStep[]): string { @@ -183,10 +314,29 @@ function formatCounterexample(message: string, trace: TraceStep[]): string { function checkTransitionInvariants(previous: ModelState, transition: Transition): string[] { const violations: string[] = [] for (const id of taskIds) { - const before = previous[id] - const after = transition.next[id] + const before = previous.tasks[id] + const after = transition.next.tasks[id] if (before?.status === "completed" && canonicalTask(before) !== canonicalTask(after)) { violations.push(`${id}: completed task changed after ${transition.name}`) + continue + } + // Cross-window ownership guard (PR #1495 bug class): no transition may + // clear a delegated parent's link to a child that is active AND marked + // live-elsewhere. Pre-fix, startup reconciliation repaired exactly these + // children; the mtime guard skips them, so the only enabled successor for + // such a state is the identity reconciliation. Any future model edit + // that reintroduces a link-clearing transition on a live-elsewhere child + // fails here with the shortest causal trace. + if (before?.status === "delegated" && before.awaitingChildId) { + const childId = before.awaitingChildId as TaskId + const childBefore = previous.tasks[childId] + if (childBefore?.status === "active" && previous.liveElsewhere[childId]) { + if (after?.status !== "delegated" || after.awaitingChildId !== childId) { + violations.push( + `${id}: ${transition.name} cleared delegation to active live-elsewhere child ${childId}`, + ) + } + } } } return violations From ac85ed36ed4288eda41f7e3164c1a1e85d8699be Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 09:57:37 +0900 Subject: [PATCH 11/19] chore(gitignore): drop local worktree and scratch ignore patterns --- .gitignore | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/.gitignore b/.gitignore index 584f9177fa..3961778d5e 100644 --- a/.gitignore +++ b/.gitignore @@ -59,14 +59,3 @@ qdrant_storage/ plans/ roo-cli-*.tar.gz* - -# Local worktrees (multi-branch development) -.wt-*/ - -# Temporary scratch files -.tmp-* -.zoo-status.txt -untracked-*.txt -class_*.txt -check-dup2-result.txt -*.tsbuildinfo From 5e37522aa59024656d0046c6681d4da879735539 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 16:53:30 +0900 Subject: [PATCH 12/19] test(delegation): kill 15 surviving changed-code mutants --- .../TaskHistoryStore.reconciliation.spec.ts | 497 ++++++++++++++++++ 1 file changed, 497 insertions(+) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 2827fa3b0a..422894e4cb 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -1980,3 +1980,500 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { throwingSpy.mockRestore() }) }) + +// ───────────────────────────────────────────────────────────────────────────── +// Mutation-gate kill tests — focused coverage for the 15 surviving changed-code +// mutants reproduced locally against TaskHistoryStore.ts (PR #1495 mutation-diff +// gate). Each test names the exact mutant(s) it kills and asserts an observable +// behavioral difference so the mutant cannot survive. +// ───────────────────────────────────────────────────────────────────────────── + +describe("TaskHistoryStore mutation-gate kill tests", () => { + let tmpDir: string + let store: TaskHistoryStore | undefined + let mtimeSpy: { mockRestore(): void } | undefined + + const RECONCILE_INTERVAL_MS = (TaskHistoryStore as unknown as { RECONCILE_INTERVAL_MS: number }) + .RECONCILE_INTERVAL_MS + + function useTickClock(): void { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }) + } + + async function flushAsyncWork(yields = 2000): Promise { + for (let i = 0; i < yields; i++) { + await vi.advanceTimersByTimeAsync(1) + } + } + + async function seedItems(items: HistoryItem[]): Promise { + const tasksDir = path.join(tmpDir, "tasks") + await fs.mkdir(tasksDir, { recursive: true }) + for (const item of items) { + const taskDir = path.join(tasksDir, item.id) + await fs.mkdir(taskDir, { recursive: true }) + await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item)) + } + } + + /** + * Read the private `locallyActiveTaskIds` set — the exact piece of state every + * ownership-track mutant below (L278/L299/L324/L566/L569/L1181/L1188/L1189) + * mutates. Its documented consumer is the periodic tick's orphan-repair + * exclusion (TaskHistoryStore.ts line 1083), so asserting membership is a + * direct observable of the mutated behavior. Same private-member cast pattern + * as LIVE_CHILD_MTIME_THRESHOLD_MS at the top of this spec. + */ + function ownedIds(s: TaskHistoryStore): Set { + return (s as unknown as { locallyActiveTaskIds: Set }).locallyActiveTaskIds + } + + /** Inject a stale mtime for `childId` so the liveness guard sees a crash orphan. */ + function installStaleChildInjector(childId: string): void { + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (id: string) => Promise + } + const original = probe.getChildFileMtimeMs + mtimeSpy = vi + .spyOn(probe, "getChildFileMtimeMs") + .mockImplementation((id: string) => + id === childId ? Promise.resolve(Date.now() - 10 * 60 * 1000) : original.call(store!, id), + ) + } + + function delegatedPair(parentId: string, childId: string): HistoryItem[] { + const child = makeItem({ id: childId, status: "active", parentTaskId: parentId, rootTaskId: parentId }) + const parent = makeItem({ + id: parentId, + status: "delegated", + awaitingChildId: childId, + delegatedToId: childId, + childIds: [childId], + }) + return [parent, child] + } + + beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "mutkill-test-")) + }) + + afterEach(async () => { + mtimeSpy?.mockRestore() + mtimeSpy = undefined + store?.dispose() + store = undefined + vi.useRealTimers() + await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => {}) + }) + + it("repair writes (skipTransitionCheck) do NOT register local ownership (kills L278 ConditionalExpression)", async () => { + // upsertCore: `if (!options.skipTransitionCheck) { trackLocalSessionOwnership(written) }`. + // The "interrupted handoff" repair path (reconcileDelegationStateCore) sets the parent to + // ACTIVE via upsertCore(..., { skipTransitionCheck: true }). Replacing `!options.skipTransitionCheck` + // with `true` would ALSO run trackLocalSessionOwnership(written) for that repair write, and + // because written.status === "active" the parent would be ADDED to locallyActiveTaskIds. + // Assert the repaired parent is NOT in the ownership set: present under the mutant, absent + // under correct code. + const child = makeItem({ + id: "child-l278", + status: "completed", + completionResultSummary: "done", + parentTaskId: "parent-l278", + rootTaskId: "parent-l278", + }) + const parent = makeItem({ + id: "parent-l278", + status: "delegated", + awaitingChildId: child.id, + delegatedToId: child.id, + }) + await seedItems([parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + + // The completed-child handoff repaired the parent to active via skipTransitionCheck. + expect(s.get(parent.id)?.status).toBe("active") + expect(s.get(parent.id)?.awaitingChildId).toBeUndefined() + // Under L278->true the active repair write adds parent.id to this set; correct code does not. + expect(ownedIds(s).has(parent.id)).toBe(false) + }) + + it("non-active runtime write DELETES local ownership (kills L566 Conditional->true / LogicalOperator / StringLiteral, L569 CallExpression)", async () => { + // trackLocalSessionOwnership: `if ((written.status ?? "active") === "active") add else delete`. + // Observable under test: after an active runtime write registers ownership, a later NON-active + // runtime write must remove it (the else/`delete(id)` branch). If the mutant forces the add + // branch (L566 ->true) or drops the delete (L569 `;`), the task stays owned and the periodic + // tick will NOT repair it as a crash orphan. + // + // Sequence on ONE task id `orphan`: + // 1. runtime `active` write -> ownership ADDED. + // 2. runtime `completed` write (valid active->completed) -> ownership DELETED. + // 3. seed disk so the SAME id is again an active child of a delegated parent (crash orphan) + // and reload the store — the only ownership signal is from step 1/2 runtime writes. + // 4. tick: with ownership deleted, the orphan is repaired (interrupted). Under either mutant + // it stays owned -> stays active. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const childId = "orphan-l566" + const parentId = "parent-l566" + + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + await s.initialize() + + // Steps 1+2: register then delete ownership via valid runtime transitions. + await s.upsert(makeItem({ id: childId, status: "active", parentTaskId: parentId, rootTaskId: parentId })) + await s.atomicReadAndUpdate(childId, (c) => ({ ...c, status: "completed" as const })) + expect(s.get(childId)?.status).toBe("completed") + s.dispose() + + // Step 3: rewrite disk so the same child id is once more an ACTIVE orphan of a delegated + // parent (as if another window crashed mid-delegation), then reload into a fresh store. + const [parent, child] = delegatedPair(parentId, childId) + await seedItems([parent, child]) + const s2 = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + + // Startup reconciliation must NOT repair it yet: make the mtime look live at startup, then + // stale only for the tick. + let age = 60_000 + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (id: string) => Promise + } + mtimeSpy = vi + .spyOn(probe, "getChildFileMtimeMs") + .mockImplementation((id: string) => + id === childId ? Promise.resolve(Date.now() - age) : Promise.resolve(undefined), + ) + + await s2.initialize() + expect(s2.get(childId)?.status).toBe("active") + + // Step 4: tick with a now-stale mtime. + age = 10 * 60 * 1000 + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushAsyncWork() + + // Ownership was deleted by the completed write, so the orphan is repaired. + // (Under L566->true or L569 `;` it would remain owned and stay active.) + expect(s2.get(childId)?.status).toBe("interrupted") + expect(errorSpy).not.toHaveBeenCalled() + errorSpy.mockRestore() + }) + + it("non-active runtime write removes the id from locallyActiveTaskIds (kills L566 Conditional->true / LogicalOperator, L569 CallExpression)", async () => { + // Direct set assertion for the else/`delete(id)` branch of trackLocalSessionOwnership. + // After an active runtime write the id is present; after a completed runtime write it must + // be removed. Under L566->true (forced add branch) or L569 `;` (delete dropped), the id + // would still be present after the completed write. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + await s.upsert(makeItem({ id: "own-add", status: "active" })) + expect(ownedIds(s).has("own-add")).toBe(true) + + // Valid active -> completed transition exercises the else branch (delete). + await s.upsert(makeItem({ id: "own-add", status: "completed" })) + expect(ownedIds(s).has("own-add")).toBe(false) + }) + + it("active runtime write adds the id to locallyActiveTaskIds (kills L566 Conditional->true add-branch, LogicalOperator)", async () => { + // Complement: the add branch must actually insert. Under L566 LogicalOperator mutants + // (e.g. `written.status && "active"`), an explicit "active" status short-circuits to a + // truthy-but-not-"active" value, so `=== "active"` is false and the add is skipped. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + await s.upsert(makeItem({ id: "add-explicit", status: "active" })) + expect(ownedIds(s).has("add-explicit")).toBe(true) + }) + + it('undefined status is treated as implicit active and registers ownership (kills L566 StringLiteral->"")', async () => { + // `(written.status ?? "active") === "active"`: StringLiteral->"" makes undefined status fall + // to "" !== "active" -> delete branch. A runtime write with NO status field must still count + // as implicit active and register ownership, so the tick leaves this in-window child alone. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const childId = "child-l566-undef" + const parentId = "parent-l566-undef" + const [parent, child] = delegatedPair(parentId, childId) + await seedItems([parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + await s.initialize() + await s.upsert(makeItem({ id: parentId, status: "active" })) + // Runtime write with status omitted entirely (legacy implicit active). + const noStatus = makeItem({ id: childId, parentTaskId: parentId, rootTaskId: parentId }) + delete (noStatus as Partial).status + await s.upsert(noStatus) + // Delegate the pair; the child must remain owned HERE because its write was implicit-active. + await s.atomicReadAndUpdate(parentId, (c) => ({ + ...c, + status: "delegated" as const, + awaitingChildId: childId, + delegatedToId: childId, + })) + await s.atomicReadAndUpdate(childId, (c) => ({ ...c, status: "active" as const })) + + installStaleChildInjector(childId) + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushAsyncWork() + + // Owned here (implicit active) -> the tick must NOT tear it away from its own runner. + expect(s.get(childId)?.status).toBe("active") + expect(s.get(parentId)?.status).toBe("delegated") + expect(errorSpy).not.toHaveBeenCalled() + errorSpy.mockRestore() + }) + + it('undefined status is treated as implicit active and adds the id to locallyActiveTaskIds (kills L566 StringLiteral->"")', async () => { + // `(written.status ?? "active") === "active"`: StringLiteral->"" makes an undefined status + // fall to `"" !== "active"` -> delete branch, so the id is never added. A runtime write with + // NO status field must count as implicit active and register ownership. Assert membership. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + const noStatus = makeItem({ id: "undef-status" }) + delete (noStatus as Partial).status + await s.upsert(noStatus) + // Under L566 StringLiteral->"" this stays absent; correct code adds it. + expect(ownedIds(s).has("undef-status")).toBe(true) + }) + + it("delete() removes the id from locallyActiveTaskIds (kills L299 CallExpression)", async () => { + // delete(): the `locallyActiveTaskIds.delete(taskId)` statement is the CallExpression the + // mutant drops (`;`). Register ownership via an active runtime write, then delete the task + // and assert the id is gone from the ownership set — under the mutant it would remain. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + await s.upsert(makeItem({ id: "del-l299", status: "active" })) + expect(ownedIds(s).has("del-l299")).toBe(true) + + await s.delete("del-l299") + expect(s.get("del-l299")).toBeUndefined() + expect(ownedIds(s).has("del-l299")).toBe(false) + }) + + it("deleteMany() removes every deleted id from locallyActiveTaskIds (kills L324 CallExpression)", async () => { + // deleteMany(): the per-task `locallyActiveTaskIds.delete(taskId)` is the CallExpression the + // mutant drops. Own two tasks, delete both, and assert neither remains in the ownership set. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + await s.upsert(makeItem({ id: "dm-1", status: "active" })) + await s.upsert(makeItem({ id: "dm-2", status: "active" })) + await s.upsert(makeItem({ id: "dm-3", status: "active" })) + expect(ownedIds(s).has("dm-1")).toBe(true) + expect(ownedIds(s).has("dm-3")).toBe(true) + + await s.deleteMany(["dm-1", "dm-3"]) + expect(s.get("dm-1")).toBeUndefined() + expect(s.get("dm-3")).toBeUndefined() + expect(ownedIds(s).has("dm-1")).toBe(false) + expect(ownedIds(s).has("dm-3")).toBe(false) + // Untouched task keeps its ownership. + expect(ownedIds(s).has("dm-2")).toBe(true) + }) + + it("replay liveness guard treats child file age exactly at threshold as NOT live and repairs (kills L627 EqualityOperator '<'->'<=')", async () => { + // replayDelegationRepairIntent: `Date.now() - mtimeMs < LIVE_CHILD_MTIME_THRESHOLD_MS`. + // Under `<=`, age === threshold counts as live and the stale intent is quarantined. With the + // real strict `<`, age === threshold is NOT live, so the crash-orphan intent is replayed: + // child -> interrupted, parent -> active. Assert the replay happens at exactly threshold. + const FIXED_NOW = 1_756_886_400_000 + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + try { + const child = makeItem({ + id: "child-l627", + status: "active", + parentTaskId: "parent-l627", + rootTaskId: "parent-l627", + }) + const parent = makeItem({ + id: "parent-l627", + status: "delegated", + awaitingChildId: child.id, + delegatedToId: child.id, + }) + await seedItems([parent, child]) + const tasksDir = path.join(tmpDir, "tasks") + const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) + await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) + + // Isolate the REPLAY guard's decision from the later startup reconcile (step 3 of + // initialize). The replay runs first and reads getChildFileMtimeMs once; make that first + // call return age EXACTLY == threshold, then make every subsequent call (the step-3 + // startup reconcile's own liveness probe) return a RECENT age so step 3 treats the child + // as live and does NOT repair it. The child's final status then reflects ONLY the replay + // guard at line 627: strict '<' (correct) -> threshold age is NOT live -> replay repairs + // (child interrupted); '<=' (mutant) -> live -> quarantine (child stays active). + let probeCalls = 0 + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (id: string) => Promise + } + mtimeSpy = vi.spyOn(probe, "getChildFileMtimeMs").mockImplementation((id: string) => { + if (id !== child.id) return Promise.resolve(undefined) + probeCalls++ + // First call = the replayDelegationRepairIntent guard (line 627): exactly threshold. + // Later calls = the startup reconcile guard (line 506): recent -> child stays live. + return Promise.resolve(probeCalls === 1 ? FIXED_NOW - LIVE_CHILD_MTIME_THRESHOLD_MS : FIXED_NOW - 1_000) + }) + + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + + // Strict '<': threshold age is NOT live -> the intent replays (child interrupted). + // Under '<=': the intent would be quarantined and the child would stay active (step 3 + // sees the child as live and leaves it alone). + expect(s.get(child.id)?.status).toBe("interrupted") + expect(s.get(parent.id)?.status).toBe("active") + } finally { + nowSpy.mockRestore() + } + }) + + it("runPeriodicDelegationReconciliation does not run the pass when disposed (kills L1077 Conditional->false / LogicalOperator)", async () => { + // `if (this.disposed || this.delegationTickRunning) return`. Conditional->false forces the + // guard OFF so the pass runs even after dispose(); LogicalOperator->&& makes it run only + // when disposed AND already-running (also wrong). The observable is whether the method + // reaches reconcileDelegationState. Assert that after dispose() the pass body does NOT run. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + s.dispose() + + const internals = TaskHistoryStore.prototype as unknown as { + runPeriodicDelegationReconciliation: () => Promise + } + // Hook reconcileDelegationState to detect whether the guarded body executes. + const reconProbe = s as unknown as { reconcileDelegationState: (ids: Set) => Promise } + let passRan = false + reconProbe.reconcileDelegationState = async () => { + passRan = true + } + + await internals.runPeriodicDelegationReconciliation.call(s) + // Guard fired (disposed) -> the pass body never ran. Under L1077->false it would run. + expect(passRan).toBe(false) + }) + + it("runPeriodicDelegationReconciliation runs the pass when NOT disposed and NOT already running (kills L1077 LogicalOperator->&&)", async () => { + // Complement: with disposed=false and delegationTickRunning=false the guard must NOT fire, + // so the pass runs. Under LogicalOperator->&& the condition `disposed && tickRunning` is + // false here too... but Conditional->true (always skip) would suppress the run. Assert the + // pass executes in the normal case, pinning the guard's truth table from the other side. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + + const internals = TaskHistoryStore.prototype as unknown as { + runPeriodicDelegationReconciliation: () => Promise + } + const reconProbe = s as unknown as { reconcileDelegationState: (ids: Set) => Promise } + let passRan = false + reconProbe.reconcileDelegationState = async () => { + passRan = true + } + + await internals.runPeriodicDelegationReconciliation.call(s) + expect(passRan).toBe(true) + }) + + it("runPeriodicDelegationReconciliation sets then clears delegationTickRunning around the pass (kills L1080 BooleanLiteral->false, L1087 BooleanLiteral->true)", async () => { + // L1080 sets the flag true before the pass; L1087 clears it false in `finally`. + // - L1080->false: a concurrent second call would NOT see the flag set and would run twice. + // - L1087->true: after completion the flag stays set, so every subsequent call no-ops. + const [parent, child] = delegatedPair("parent-flag", "child-flag") + await seedItems([parent, child]) + const s = (store = new TaskHistoryStore(tmpDir)) + installStaleChildInjector(child.id) + await s.initialize() + + const internals = TaskHistoryStore.prototype as unknown as { + runPeriodicDelegationReconciliation: () => Promise + } + const flagReader = s as unknown as { delegationTickRunning: boolean } + + // Observe the flag being true DURING the pass via a hook into reconcileDelegationState. + const reconProbe = s as unknown as { reconcileDelegationState: (ids: Set) => Promise } + const originalRecon = reconProbe.reconcileDelegationState.bind(s) + let flagDuringPass: boolean | undefined + reconProbe.reconcileDelegationState = async (ids: Set) => { + flagDuringPass = flagReader.delegationTickRunning + return originalRecon(ids) + } + + await internals.runPeriodicDelegationReconciliation.call(s) + // Flag was true while the pass ran (kills L1080->false). + expect(flagDuringPass).toBe(true) + // Flag cleared after the pass completed (kills L1087->true). + expect(flagReader.delegationTickRunning).toBe(false) + + // A second call runs again (proves the flag was actually reset, not stuck). + let secondRan = false + reconProbe.reconcileDelegationState = async (ids: Set) => { + secondRan = true + return originalRecon(ids) + } + await internals.runPeriodicDelegationReconciliation.call(s) + expect(secondRan).toBe(true) + }) + + it("atomicUpdatePair registers ownership for both records on success (kills L1188/L1189 CallExpression)", async () => { + // The success path calls trackLocalSessionOwnership(writtenFirst) and (writtenSecond). The + // CallExpression `;` mutants drop those calls, so the ids never enter locallyActiveTaskIds. + // Assert both ids are present after a pair write that leaves both active. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + await s.upsert(makeItem({ id: "pf-first", status: "active" })) + await s.upsert(makeItem({ id: "pf-second", status: "active" })) + // Clear prior ownership so only the atomicUpdatePair calls can re-add them. + ownedIds(s).clear() + expect(ownedIds(s).size).toBe(0) + + await s.atomicUpdatePair( + "pf-first", + "pf-second", + (c) => ({ ...c, status: "active" as const }), + (c) => ({ ...c, status: "active" as const }), + ) + + // Under L1188/L1189 `;` the trackLocalSessionOwnership calls vanish and these stay absent. + expect(ownedIds(s).has("pf-first")).toBe(true) + expect(ownedIds(s).has("pf-second")).toBe(true) + }) + + it("atomicUpdatePair registers ownership for the committed first record on partial failure (kills L1181 CallExpression)", async () => { + // On second-write failure the catch block updates the cache AND calls + // trackLocalSessionOwnership(writtenFirst) before rethrowing. The `;` mutant drops that call, + // so the committed first record never enters locallyActiveTaskIds. Force the SECOND + // writeTaskFile to fail, then assert the first record IS in the ownership set (under the + // mutant it stays absent). + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + await s.upsert(makeItem({ id: "pf-first", status: "active" })) + await s.upsert(makeItem({ id: "pf-second", status: "active" })) + ownedIds(s).clear() + + // Spy writeTaskFile: succeed for the first record, reject for the second, so the catch + // path (which contains L1181) runs. + const storeAny = s as unknown as { writeTaskFile: (item: HistoryItem, delta?: unknown) => Promise } + const originalWrite = storeAny.writeTaskFile.bind(s) + const writeSpy = vi + .spyOn(storeAny, "writeTaskFile") + .mockImplementation(async (item: HistoryItem, delta?: unknown) => { + if (item.id === "pf-second") { + throw new Error("simulated second-write failure") + } + return originalWrite(item, delta) + }) + + await expect( + s.atomicUpdatePair( + "pf-first", + "pf-second", + (c) => ({ ...c, status: "active" as const }), + (c) => ({ ...c, status: "active" as const }), + ), + ).rejects.toThrow("simulated second-write failure") + + // The catch block committed pf-first to disk and must have registered its ownership. + // Under L1181 `;` that call vanishes and pf-first stays absent. + expect(ownedIds(s).has("pf-first")).toBe(true) + writeSpy.mockRestore() + }) +}) From 382ec05753ded1fcd9615439ce751302e11f50ae Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Sat, 5 Sep 2026 20:10:12 +0900 Subject: [PATCH 13/19] test(delegation): replace fixed-count timer pump with condition polling --- .../TaskHistoryStore.reconciliation.spec.ts | 158 +++++++++++++++--- 1 file changed, 139 insertions(+), 19 deletions(-) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 422894e4cb..95bdfa5f72 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -1716,7 +1716,7 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { /** * Fake only what the tick scheduling needs: the 5-minute `setTimeout` clock * and `Date` (consumed by the liveness guard). Everything else (fs I/O, - * microtasks) stays real so `flushAsyncWork()` below can pump the event + * microtasks) stays real so `flushUntil()` below can pump the event * loop while the timer clock advances only 1 ms per yield. */ function useTickClock(): void { @@ -1724,18 +1724,49 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { } /** - * Drain pending real fs I/O. The tick's reconcile/repair chain completes on - * libuv callbacks that fake timers alone never advance, and each - * `advanceTimersByTimeAsync(1)` yields one REAL macrotask turn (processing - * the poll phase) while advancing the fake clock only 1 ms. The total fake - * time here stays far below RECONCILE_INTERVAL_MS, so no extra tick fires - * during the pump — this only lets in-flight fs callbacks settle. The count - * is generous to absorb Windows antivirus/OneDrive fs latency. + * Drain pending real fs I/O by polling an observable condition instead of + * burning a fixed number of yields. The tick's reconcile/repair chain + * completes on libuv callbacks that fake timers alone never advance, and + * each `advanceTimersByTimeAsync(1)` yields one REAL macrotask turn + * (processing the poll phase) while advancing the fake clock only 1 ms. + * The yield count the chain needs is environment-dependent (~155 yields on + * a fast local SSD; higher on contended CI runners — the old fixed + * 2000-yield pumps intermittently starved on ubuntu CI, which is exactly + * what this helper replaces). Polling the SAME final state the assertions + * check makes the wait deterministic without weakening them. The pump + * stops as soon as the condition holds, so correct-code runs stay fast, + * and the generous cap costs sub-second wall time even when exhausted + * because fake timers never sleep (measured ~123 ms per 55K idle yields). + * On exhaustion it THROWS with a state snapshot rather than silently + * proceeding, converting a future hang into a loud, diagnosable failure. + * + * Predicates MUST be cheap and side-effect free: poll the in-memory cache + * getters (`store.get(...)`, which never touches disk) or spy call logs. */ - async function flushAsyncWork(yields = 2000): Promise { - for (let i = 0; i < yields; i++) { + async function flushUntil( + predicate: () => boolean, + options: { maxYields?: number; label?: string; snapshot?: () => string } = {}, + ): Promise { + const { maxYields = 50_000, label = "flushUntil predicate", snapshot } = options + for (let i = 0; i < maxYields; i++) { + if (predicate()) { + return + } await vi.advanceTimersByTimeAsync(1) } + if (predicate()) { + return + } + let state = "snapshot unavailable" + try { + state = snapshot ? snapshot() : "no snapshot supplied" + } catch { + // A throwing snapshot must not mask the primary diagnostic below. + } + throw new Error( + `flushUntil: "${label}" was not satisfied within ${maxYields} yields (~${maxYields} ms of fake ` + + `time). The tick's async chain never settled; final state: ${state}.`, + ) } async function seedItems(items: HistoryItem[]): Promise { @@ -1824,7 +1855,24 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { // next periodic tick its mtime is past the liveness threshold. childAgeMs = 10 * 60 * 1000 await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushAsyncWork() + // Compound predicate: the "Reconciled orphaned active child" warn is + // emitted only after repairActiveDelegation fully resolves (intent + // write, both task-file writes, cache updates, intent cleanup), so + // waiting for the cache flip AND the warn settles every observable the + // assertions below depend on — a cache-only predicate could return + // before the warnSpy assertion is satisfiable. + await flushUntil( + () => + s.get(CHILD_ID)?.status === "interrupted" && + warnSpy.mock.calls.some( + (c) => typeof c[0] === "string" && c[0].includes("Reconciled orphaned active child"), + ), + { + label: "stale child repaired to interrupted and the repair was logged", + snapshot: () => + `child=${s.get(CHILD_ID)?.status} parent=${s.get(PARENT_ID)?.status} warnCalls=${warnSpy.mock.calls.length}`, + }, + ) // Within ONE interval, the parent window must repair: child → interrupted, // parent → active with delegation links cleared. @@ -1900,8 +1948,19 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { childId === CHILD_ID ? Promise.resolve(Date.now() - 10 * 60 * 1000) : realProbe.call(s, childId), ) + // Negative test: the tick must do NOTHING to this locally-owned child, + // so no positive log exists to poll. Settle on the recursive re-arm + // instead — `startPeriodicReconciliation()` only re-runs after BOTH + // `reconcile()` and the delegation pass have fully finished, so a + // fresh timer handle proves the whole tick settled. + const timerState = s as unknown as { reconcileTimer: ReturnType | null } + const timerBeforeTick = timerState.reconcileTimer await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushAsyncWork() + await flushUntil(() => timerState.reconcileTimer !== timerBeforeTick, { + label: "periodic tick completed without repairing the locally-owned child", + snapshot: () => + `child=${s.get(CHILD_ID)?.status} parent=${s.get(PARENT_ID)?.status} awaiting=${s.get(PARENT_ID)?.awaitingChildId}`, + }) expect(s.get(CHILD_ID)?.status).toBe("active") expect(s.get(PARENT_ID)?.status).toBe("delegated") @@ -1927,7 +1986,19 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { // The other window keeps writing: the child stays live at tick time. childAgeMs = 60_000 await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushAsyncWork() + // The skip-guard warn is this test's own observable (asserted below); + // once it fires the liveness check has run, no repair follows, and the + // persisted-file read below is safe (reconcile() never writes). + await flushUntil( + () => + logSpy.mock.calls.some( + (c) => typeof c[0] === "string" && c[0].includes(`Skipping repair for live child ${CHILD_ID}`), + ), + { + label: "tick skipped the repair for the live child", + snapshot: () => `child=${s.get(CHILD_ID)?.status} warnCalls=${logSpy.mock.calls.length}`, + }, + ) // Nothing may be repaired: child stays active, parent keeps its delegation links. expect(s.get(CHILD_ID)?.status).toBe("active") @@ -1964,7 +2035,18 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { await s.initialize() await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushAsyncWork() + // The error log happens in the tick callback's catch AFTER the throwing + // delegation step settles, so the spy firing means the tick is done. + await flushUntil( + () => + errorSpy.mock.calls.some( + (c) => typeof c[0] === "string" && c[0].includes("Periodic delegation reconciliation failed"), + ), + { + label: "tick logged the delegation failure", + snapshot: () => `errorCalls=${errorSpy.mock.calls.length}`, + }, + ) expect(errorSpy).toHaveBeenCalledWith( expect.stringContaining("Periodic delegation reconciliation failed"), expect.objectContaining({ message: "tick delegation boom" }), @@ -1973,7 +2055,10 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { // One more interval still fires the delegation step: the recursive // re-arm is preserved even though the step threw. await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushAsyncWork() + await flushUntil(() => throwingSpy.mock.calls.length >= 2, { + label: "second tick invoked the throwing delegation step", + snapshot: () => `throwingSpyCalls=${throwingSpy.mock.calls.length}`, + }) expect(throwingSpy).toHaveBeenCalledTimes(2) errorSpy.mockRestore() @@ -2000,10 +2085,34 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }) } - async function flushAsyncWork(yields = 2000): Promise { - for (let i = 0; i < yields; i++) { + // Condition-polling pump; see the full doc comment on the identical helper + // in the "periodic delegation reconciliation" block above for the + // rationale (the fixed 2000-yield pumps intermittently starved on slow + // ubuntu CI runners). + async function flushUntil( + predicate: () => boolean, + options: { maxYields?: number; label?: string; snapshot?: () => string } = {}, + ): Promise { + const { maxYields = 50_000, label = "flushUntil predicate", snapshot } = options + for (let i = 0; i < maxYields; i++) { + if (predicate()) { + return + } await vi.advanceTimersByTimeAsync(1) } + if (predicate()) { + return + } + let state = "snapshot unavailable" + try { + state = snapshot ? snapshot() : "no snapshot supplied" + } catch { + // A throwing snapshot must not mask the primary diagnostic below. + } + throw new Error( + `flushUntil: "${label}" was not satisfied within ${maxYields} yields (~${maxYields} ms of fake ` + + `time). The tick's async chain never settled; final state: ${state}.`, + ) } async function seedItems(items: HistoryItem[]): Promise { @@ -2152,7 +2261,10 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { // Step 4: tick with a now-stale mtime. age = 10 * 60 * 1000 await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushAsyncWork() + await flushUntil(() => s2.get(childId)?.status === "interrupted", { + label: "completed write released ownership so the tick repaired the orphan", + snapshot: () => `child=${s2.get(childId)?.status} parent=${s2.get(parentId)?.status}`, + }) // Ownership was deleted by the completed write, so the orphan is repaired. // (Under L566->true or L569 `;` it would remain owned and stay active.) @@ -2214,8 +2326,16 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { await s.atomicReadAndUpdate(childId, (c) => ({ ...c, status: "active" as const })) installStaleChildInjector(childId) + // Negative test (child owned HERE via the implicit-active write): the + // tick must leave it alone, so settle on the recursive re-arm, which + // only happens after both passes fully finish. + const timerState = s as unknown as { reconcileTimer: ReturnType | null } + const timerBeforeTick = timerState.reconcileTimer await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushAsyncWork() + await flushUntil(() => timerState.reconcileTimer !== timerBeforeTick, { + label: "tick completed without clobbering the locally-owned implicit-active child", + snapshot: () => `child=${s.get(childId)?.status} parent=${s.get(parentId)?.status}`, + }) // Owned here (implicit active) -> the tick must NOT tear it away from its own runner. expect(s.get(childId)?.status).toBe("active") From 9355da1533a7f5f8a30e113b587d7261ce005aa5 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Tue, 8 Sep 2026 09:47:38 +0900 Subject: [PATCH 14/19] fix(delegation): harden cross-window liveness guards per CodeRabbit round-2 review --- scripts/check-task-lifecycle.ts | 4 + src/__tests__/helpers/provider-stub.ts | 4 +- src/__tests__/single-open-invariant.spec.ts | 10 +- src/core/task-persistence/TaskHistoryStore.ts | 34 +- .../TaskHistoryStore.reconciliation.spec.ts | 473 +++++++++++------- src/core/webview/ClineProvider.ts | 11 + .../ClineProvider.flicker-free-cancel.spec.ts | 1 + 7 files changed, 339 insertions(+), 198 deletions(-) diff --git a/scripts/check-task-lifecycle.ts b/scripts/check-task-lifecycle.ts index d0108f6fd6..0220764ea7 100644 --- a/scripts/check-task-lifecycle.ts +++ b/scripts/check-task-lifecycle.ts @@ -128,6 +128,10 @@ function transitions(state: ModelState): Transition[] { const parent = state.tasks[parentId] if (!parent) continue + // A parent marked live-elsewhere is owned by another window; window-local + // delegation from it would race that window's own lifecycle operations. + if (state.liveElsewhere[parentId]) continue + for (const childId of taskIds) { if (childId === parentId || state.tasks[childId]) continue const awaitedStatus = parent.awaitingChildId diff --git a/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts index 59dde33933..a83344ba72 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -6,7 +6,7 @@ type ProviderStubFields = { delegationTransitionLocks?: Map> cancelledDelegationChildIds?: Set log?: ReturnType - taskHistoryStore?: { get: (id: string) => unknown } + taskHistoryStore?: { get: (id: string) => unknown; markLocallyActive?: (taskId: string) => void } taskRegistry?: TaskRegistry clineStack?: Task[] tasks?: Task[] @@ -37,7 +37,7 @@ export function makeProviderStub(stub: T): ClineProvider { s.delegationTransitionLocks ??= new Map() s.cancelledDelegationChildIds ??= new Set() s.log ??= vi.fn() - s.taskHistoryStore ??= { get: () => undefined } + s.taskHistoryStore ??= { get: () => undefined, markLocallyActive: () => {} } // Convert legacy clineStack array into a TaskRegistry if (!s.taskRegistry) { diff --git a/src/__tests__/single-open-invariant.spec.ts b/src/__tests__/single-open-invariant.spec.ts index af1631df9c..453665801b 100644 --- a/src/__tests__/single-open-invariant.spec.ts +++ b/src/__tests__/single-open-invariant.spec.ts @@ -80,7 +80,7 @@ describe("Single-open-task invariant", () => { taskRegistry: registry, taskScheduler: { schedule: schedulespy }, getCurrentTask: vi.fn(() => existingTask), - taskHistoryStore: { get: vi.fn(() => undefined) }, + taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -168,7 +168,7 @@ describe("Single-open-task invariant", () => { const provider = { getCurrentTask: vi.fn(() => undefined), // ensure not rehydrating - taskHistoryStore: { get: vi.fn(() => undefined) }, + taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -243,7 +243,7 @@ describe("Single-open-task invariant", () => { const provider = { getCurrentTask: vi.fn(() => existingTask), taskRegistry: registry, - taskHistoryStore: { get: vi.fn(() => undefined) }, + taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -319,7 +319,7 @@ describe("Single-open-task invariant", () => { historyTaskCreationQueue: Promise.resolve(), getCurrentTask: vi.fn(() => registry.current), taskRegistry: registry, - taskHistoryStore: { get: vi.fn(() => undefined) }, + taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, evictCurrentTask, addClineToStack: vi.fn().mockImplementation(async (task: Task) => registry.push(task)), log: vi.fn(), @@ -386,7 +386,7 @@ describe("Single-open-task invariant", () => { const provider = { context: {} as unknown, getCurrentTask: vi.fn(() => undefined), - taskHistoryStore: { get: vi.fn(() => undefined) }, + taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 0cc6091d91..75fee236fc 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -570,6 +570,18 @@ export class TaskHistoryStore { } } + /** + * Mark a task id as owned by a live session in THIS window before its first + * runtime write settles. Resumed tasks only enter `locallyActiveTaskIds` via + * `trackLocalSessionOwnership` when Task.run() persists an active item; the + * async gap before that write lets the periodic delegation pass see the task + * as a quiet, unowned disk record and repair it mid-resume. Registering the + * id eagerly closes that window; a later non-active write still removes it. + */ + public markLocallyActive(taskId: string): void { + this.locallyActiveTaskIds.add(taskId) + } + /** * Replay the durable active-child repair intent, if one was left by a crash. * The expected fields are guards: an intent may update only the missing side @@ -619,7 +631,9 @@ export class TaskHistoryStore { // intent would overwrite the live child as "interrupted", so quarantine it instead. // Only enforced when the replay would actually write the child record: a child // already at its target needs no write, and parent-only completion must not be - // blocked by child liveness. An unreadable mtime conservatively proceeds. + // blocked by child liveness. Only a genuinely missing (ENOENT) history file + // proceeds; a transient stat failure is treated as evidence of life (see + // `getChildFileMtimeMs`) and lets a later tick retry. if (!childAtTarget) { const mtimeMs = await this.getChildFileMtimeMs(child.id) const isLiveElsewhere = @@ -1231,16 +1245,26 @@ export class TaskHistoryStore { /** * Returns the mtime (ms epoch) of the child's history_item.json, or undefined - * when unreadable. A recent mtime means another live extension host is actively - * persisting this child, so startup repair must not treat it as a crash orphan. + * only when the file is genuinely absent (ENOENT). A recent mtime means another + * live extension host is actively persisting this child, so startup repair must + * not treat it as a crash orphan. Any OTHER stat failure (EMFILE, EACCES, EIO, + * ...) is not evidence of absence: the child is reported with a future mtime so + * every `Date.now() - mtimeMs < threshold` liveness guard holds, repair is + * skipped, and a later reconciliation tick retries instead. */ private async getChildFileMtimeMs(childId: string): Promise { try { const filePath = await this.getTaskFilePath(childId) const stat = await fs.stat(filePath) return stat.mtimeMs - } catch { - return undefined // File missing/unreadable → conservatively proceed with repair + } catch (error) { + // ENOENT: the file is genuinely absent → no window is persisting it; + // conservatively proceed with repair. Any other stat failure is treated + // as evidence of life via a threshold-shifted future timestamp. + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + return undefined + } + return Date.now() + TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS } } } diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 95bdfa5f72..52ee21913e 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -69,6 +69,78 @@ function makeRepairIntent(parent: HistoryItem, child: HistoryItem): object { } } +/** + * Fake only what the tick scheduling needs: the 5-minute `setTimeout` clock + * and `Date` (consumed by the liveness guard). Everything else (fs I/O, + * microtasks) stays real so `flushUntil()` below can pump the event + * loop while the timer clock advances only 1 ms per yield. + */ +function useTickClock(): void { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }) +} + +/** + * Drain pending real fs I/O by polling an observable condition instead of + * burning a fixed number of yields. The tick's reconcile/repair chain + * completes on libuv callbacks that fake timers alone never advance, and + * each `advanceTimersByTimeAsync(1)` yields one REAL macrotask turn + * (processing the poll phase) while advancing the fake clock only 1 ms. + * The yield count the chain needs is environment-dependent (~155 yields on + * a fast local SSD; higher on contended CI runners — the old fixed + * 2000-yield pumps intermittently starved on ubuntu CI, which is exactly + * what this helper replaces). Polling the SAME final state the assertions + * check makes the wait deterministic without weakening them. The pump + * stops as soon as the condition holds, so correct-code runs stay fast, + * and the generous cap costs sub-second wall time even when exhausted + * because fake timers never sleep (measured ~123 ms per 55K idle yields). + * On exhaustion it THROWS with a state snapshot rather than silently + * proceeding, converting a future hang into a loud, diagnosable failure. + * + * Predicates MUST be cheap and side-effect free: poll the in-memory cache + * getters (`store.get(...)`, which never touches disk) or spy call logs. + */ +async function flushUntil( + predicate: () => boolean, + options: { maxYields?: number; label?: string; snapshot?: () => string } = {}, +): Promise { + const { maxYields = 50_000, label = "flushUntil predicate", snapshot } = options + for (let i = 0; i < maxYields; i++) { + if (predicate()) { + return + } + await vi.advanceTimersByTimeAsync(1) + } + if (predicate()) { + return + } + let state = "snapshot unavailable" + try { + state = snapshot ? snapshot() : "no snapshot supplied" + } catch { + // A throwing snapshot must not mask the primary diagnostic below. + } + throw new Error( + `flushUntil: "${label}" was not satisfied within ${maxYields} yields (~${maxYields} ms of fake ` + + `time). The tick's async chain never settled; final state: ${state}.`, + ) +} + +/** + * Write history items to `/tasks//history_item.json` so a freshly + * constructed TaskHistoryStore sees them on `initialize()`. `dir` is the + * caller's per-describe temp directory; passed explicitly because this helper + * is shared by every describe block in the spec. + */ +async function seedItems(dir: string, items: HistoryItem[]): Promise { + const tasksDir = path.join(dir, "tasks") + await fs.mkdir(tasksDir, { recursive: true }) + for (const item of items) { + const taskDir = path.join(tasksDir, item.id) + await fs.mkdir(taskDir, { recursive: true }) + await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item)) + } +} + // ───────────────────────────────────────────────────────────────────────────── // assertValidTransition — pure function tests // ───────────────────────────────────────────────────────────────────────────── @@ -161,16 +233,6 @@ describe("TaskHistoryStore reconcileDelegationState", () => { return nextStore } - async function seedItems(items: HistoryItem[]): Promise { - const tasksDir = path.join(tmpDir, "tasks") - await fs.mkdir(tasksDir, { recursive: true }) - for (const item of items) { - const taskDir = path.join(tasksDir, item.id) - await fs.mkdir(taskDir, { recursive: true }) - await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item)) - } - } - /** * Backdate a task's history file mtime so the cross-instance liveness guard * treats it as a crash orphan (last write > 5 minutes ago) rather than a @@ -238,7 +300,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { it("getChildFileMtimeMs returns the file mtime for an existing child and undefined for a missing one", async () => { // Direct coverage of the private mtime probe used by the cross-instance // liveness guard (TaskHistoryStore.ts getChildFileMtimeMs): the happy - // path returns stat.mtimeMs and the catch path returns undefined. + // path returns stat.mtimeMs and a missing (ENOENT) file returns undefined. // Bracket/typed access follows the same private-member pattern used by // "removes the repair-intent file after successful replay" below. const internals = store as unknown as { @@ -248,10 +310,63 @@ describe("TaskHistoryStore reconcileDelegationState", () => { expect(await internals.getChildFileMtimeMs("missing-mtime-child")).toBeUndefined() const child = makeItem({ id: "present-mtime-child", status: "active" }) - await seedItems([child]) + await seedItems(tmpDir, [child]) const mtimeMs = await internals.getChildFileMtimeMs("present-mtime-child") expect(typeof mtimeMs).toBe("number") expect(mtimeMs).toBeGreaterThan(0) + // Exact equality with a fresh independent stat: a probe that returned, + // say, Date.now() instead of stat.mtimeMs would still pass the + // typeof/>0 checks but diverge from the real file's mtime here. + const filePath = path.join(tmpDir, "tasks", "present-mtime-child", GlobalFileNames.historyItem) + const fileStat = await fs.stat(filePath) + expect(mtimeMs).toBe(fileStat.mtimeMs) + }) + + it("classifies getChildFileMtimeMs stat errors: ENOENT → undefined, transient errors → live", async () => { + // Direct coverage of the error-classification branches: a genuinely + // missing file (real FS ENOENT) returns undefined so repair may proceed, + // while a transient stat failure returns a FUTURE mtime so the + // `Date.now() - mtimeMs < threshold` liveness guards hold and repair is + // skipped (a later tick retries). `fs.stat` cannot be spied (the ESM + // namespace is sealed), so the transient failure is produced by routing + // the child's path through the private `getTaskFilePath` seam with an + // embedded NUL byte: Node's real `fs.stat` rejects such paths with + // ERR_INVALID_ARG_VALUE on every platform — a deterministic, never-ENOENT + // error that exercises the classifier against the real fs call. + const internals = store as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + + // ENOENT: no such task directory exists on disk — real filesystem miss. + expect(await internals.getChildFileMtimeMs("enoent-classify-missing")).toBeUndefined() + + const child = makeItem({ id: "transient-classify-child", status: "active" }) + await seedItems(tmpDir, [child]) + const tasksDir = path.join(tmpDir, "tasks") + const probe = TaskHistoryStore.prototype as unknown as { + getTaskFilePath: (taskId: string) => Promise + } + const originalGetTaskFilePath = probe.getTaskFilePath + const pathSpy = vi + .spyOn(probe, "getTaskFilePath") + .mockImplementation((taskId: string) => + taskId === "transient-classify-child" + ? Promise.resolve(path.join(tasksDir, taskId, "his\0tory_item.json")) + : originalGetTaskFilePath.call(store, taskId), + ) + try { + const probed = await internals.getChildFileMtimeMs("transient-classify-child") + // A numeric (live) result is required; the type narrowing below is the + // assertion, so a `undefined` return would already have failed here. + expect(probed).toBeDefined() + if (typeof probed === "number") { + // Future timestamp ⇒ negative age ⇒ every live-child guard holds. + expect(Date.now() - probed).toBeLessThan(0) + expect(Date.now() - probed).toBeLessThan(LIVE_CHILD_MTIME_THRESHOLD_MS) + } + } finally { + pathSpy.mockRestore() + } }) afterEach(async () => { @@ -265,7 +380,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { it("repairs orphaned delegation: delegated parent whose child does not exist → active", async () => { const parent = makeItem({ id: "parent-1", status: "delegated", awaitingChildId: "missing-child" }) - await seedItems([parent]) + await seedItems(tmpDir, [parent]) await store.initialize() @@ -287,7 +402,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: "child-2", delegatedToId: "child-2", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await store.initialize() @@ -302,7 +417,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { it("uses fallback summary when child has no completionResultSummary", async () => { const child = makeItem({ id: "child-3", status: "completed" }) const parent = makeItem({ id: "parent-3", status: "delegated", awaitingChildId: "child-3" }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await store.initialize() @@ -325,7 +440,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: "child-4", childIds: ["child-4"], }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await markStaleMtime("child-4") await store.initialize() @@ -404,7 +519,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: "child-live", childIds: ["child-live"], }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) // Simulate another live window actively persisting the child: the file // was just written, so its mtime is within the 5-minute threshold. @@ -455,7 +570,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: "child-stale", childIds: ["child-stale"], }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) // Simulate a crash orphan: the child file has not been written for 6 // minutes, exceeding the 5-minute liveness threshold. @@ -530,7 +645,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { // persistedActiveIds); only the stat probe is forced to undefined, // simulating a file that races away or is unreadable at the moment // the liveness guard checks it. - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await store.initialize() @@ -597,7 +712,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: "child-boundary-equal", delegatedToId: "child-boundary-equal", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await setChildMtimeAge("child-boundary-equal", 300_000) await store.initialize() @@ -638,7 +753,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: "child-boundary-live", delegatedToId: "child-boundary-live", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await setChildMtimeAge("child-boundary-live", 299_999) await store.initialize() @@ -688,7 +803,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: "child-epoch-mtime", delegatedToId: "child-epoch-mtime", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await setChildMtimeAge("child-epoch-mtime", FIXED_NOW - 1_000) // store observes mtime 1970-01-01T00:00:01.000Z await store.initialize() @@ -727,7 +842,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: "child-future-mtime", delegatedToId: "child-future-mtime", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await setChildMtimeAge("child-future-mtime", -100_000) // store observes a mtime 100s in the future await store.initialize() @@ -763,7 +878,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: "child-boundary-ceil", delegatedToId: "child-boundary-ceil", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await setChildMtimeAge("child-boundary-ceil", 299_499) await store.initialize() @@ -791,7 +906,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await markStaleMtime(child.id) await store.initialize() @@ -815,7 +930,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const intentPath = path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) await fs.writeFile( @@ -852,7 +967,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await markStaleMtime(child.id) safeWriteJsonMock.mockImplementation(async (filePath, data) => { if (filePath.includes(child.id) && filePath.endsWith(GlobalFileNames.historyItem)) @@ -890,7 +1005,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await markStaleMtime(child.id) safeWriteJsonMock.mockImplementation(async (filePath, data) => { if (filePath.includes(parent.id) && filePath.endsWith(GlobalFileNames.historyItem)) @@ -924,7 +1039,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await markStaleMtime(child.id) store.dispose() store = registerStore( @@ -955,7 +1070,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { it("replays a both-at-target intent without writing task files", async () => { const child = makeItem({ id: "child-at-target", status: "interrupted", parentTaskId: "parent-at-target" }) const parent = makeItem({ id: "parent-at-target", status: "active" }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const intentPath = path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent) await fs.writeFile( intentPath, @@ -981,7 +1096,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const intentPath = path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) // Keep this a crash-orphan replay: the child file must not look live in @@ -1000,7 +1115,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { it("quarantines malformed and stale intents without blocking unrelated startup", async () => { const unrelated = makeItem({ id: "unrelated-startup", status: "active" }) - await seedItems([unrelated]) + await seedItems(tmpDir, [unrelated]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify({ malformed: true })) @@ -1020,7 +1135,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { const unrelated = makeItem({ id: "unrelated-missing-intent", status: "active" }) const missingChild = makeItem({ id: "missing-intent-child", status: "active" }) const parent = makeItem({ id: "missing-intent-parent", status: "delegated", awaitingChildId: missingChild.id }) - await seedItems([unrelated]) + await seedItems(tmpDir, [unrelated]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, missingChild))) @@ -1049,7 +1164,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent({ ...parent, status: "delegated" }, child))) @@ -1082,7 +1197,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { id: "parent-mismatched-child-intent", status: "active", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile( @@ -1133,7 +1248,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) @@ -1189,7 +1304,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) @@ -1228,7 +1343,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) @@ -1244,9 +1359,11 @@ describe("TaskHistoryStore reconcileDelegationState", () => { }) it("proceeds with a replay when the child history file mtime is unreadable", async () => { - // Matches the reconcile-path convention: getChildFileMtimeMs returns - // undefined for a missing/unreadable history file, which conservatively - // proceeds with the repair instead of treating the child as live. + // getChildFileMtimeMs now returns undefined only for a genuinely missing + // (ENOENT) history file; transient stat errors return a future mtime and + // are treated as live. This test mocks the probe directly to pin the + // missing-file contract: undefined ⇒ the replay proceeds instead of + // treating the child as live-elsewhere. const probe = TaskHistoryStore.prototype as unknown as { getChildFileMtimeMs: (childId: string) => Promise } @@ -1263,7 +1380,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) @@ -1283,7 +1400,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { delegatedToId: "stale-child", awaitingChildId: "", }) - await seedItems([parent]) + await seedItems(tmpDir, [parent]) await store.initialize() @@ -1297,7 +1414,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { it("does not touch active or completed tasks", async () => { const active = makeItem({ id: "task-active", status: "active" }) const completed = makeItem({ id: "task-completed", status: "completed" }) - await seedItems([active, completed]) + await seedItems(tmpDir, [active, completed]) await store.initialize() @@ -1309,7 +1426,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { const childA = makeItem({ id: "child-a", status: "completed" }) const parentA = makeItem({ id: "parent-a", status: "delegated", awaitingChildId: "child-a" }) const parentB = makeItem({ id: "parent-b", status: "delegated", awaitingChildId: "missing-b" }) - await seedItems([childA, parentA, parentB]) + await seedItems(tmpDir, [childA, parentA, parentB]) await store.initialize() @@ -1326,7 +1443,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { status: "delegated", awaitingChildId: "missing-child-chain", }) - await seedItems([parentA, parentB]) + await seedItems(tmpDir, [parentA, parentB]) await store.initialize() @@ -1361,7 +1478,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { parentTaskId: parent.id, rootTaskId: grandparent.id, }) - await seedItems([grandparent, parent, child]) + await seedItems(tmpDir, [grandparent, parent, child]) const intentPath = path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) // Crash-orphan scenario: the child must not look live in another window, or @@ -1413,7 +1530,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await markStaleMtime(child.id) await store.initialize() @@ -1436,7 +1553,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { it("is idempotent: running initialize twice produces the same result", async () => { const child = makeItem({ id: "child-6", status: "completed", completionResultSummary: "Done" }) const parent = makeItem({ id: "parent-6", status: "delegated", awaitingChildId: "child-6" }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await store.initialize() const afterFirst = { ...store.get("parent-6") } @@ -1457,7 +1574,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) const parent = makeItem({ id: "parent-log", status: "delegated", awaitingChildId: "nonexistent" }) - await seedItems([parent]) + await seedItems(tmpDir, [parent]) await store.initialize() @@ -1473,7 +1590,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { store = registerStore(new TaskHistoryStore(tmpDir, { onWrite })) const parent = makeItem({ id: "parent-onwrite", status: "delegated", awaitingChildId: "nonexistent-child" }) - await seedItems([parent]) + await seedItems(tmpDir, [parent]) await store.initialize() @@ -1542,16 +1659,6 @@ describe("TaskHistoryStore upsert transition guard", () => { let tmpDir: string let store: TaskHistoryStore - async function seedItems(items: HistoryItem[]): Promise { - const tasksDir = path.join(tmpDir, "tasks") - await fs.mkdir(tasksDir, { recursive: true }) - for (const item of items) { - const taskDir = path.join(tasksDir, item.id) - await fs.mkdir(taskDir, { recursive: true }) - await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item)) - } - } - beforeEach(async () => { tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "upsert-guard-test-")) store = new TaskHistoryStore(tmpDir) @@ -1565,7 +1672,7 @@ describe("TaskHistoryStore upsert transition guard", () => { it("rejects completed → active transition, preserving the completed status", async () => { const item = makeItem({ id: "task-guard-1", status: "completed" }) - await seedItems([item]) + await seedItems(tmpDir, [item]) store.dispose() store = new TaskHistoryStore(tmpDir) await store.initialize() @@ -1583,7 +1690,7 @@ describe("TaskHistoryStore upsert transition guard", () => { // Must include a live active child so reconciliation doesn't repair the parent to active const child = makeItem({ id: "child-guard-2", status: "interrupted" }) const item = makeItem({ id: "task-guard-2", status: "delegated", awaitingChildId: "child-guard-2" }) - await seedItems([child, item]) + await seedItems(tmpDir, [child, item]) store.dispose() store = new TaskHistoryStore(tmpDir) await store.initialize() @@ -1600,7 +1707,7 @@ describe("TaskHistoryStore upsert transition guard", () => { it("allows valid active → completed transition", async () => { const item = makeItem({ id: "task-guard-3", status: "active" }) - await seedItems([item]) + await seedItems(tmpDir, [item]) store.dispose() store = new TaskHistoryStore(tmpDir) await store.initialize() @@ -1611,7 +1718,7 @@ describe("TaskHistoryStore upsert transition guard", () => { it("rejects interrupted → active transition, preserving the interrupted status", async () => { const item = makeItem({ id: "task-guard-interrupted", status: "interrupted" }) - await seedItems([item]) + await seedItems(tmpDir, [item]) store.dispose() store = new TaskHistoryStore(tmpDir) await store.initialize() @@ -1624,7 +1731,7 @@ describe("TaskHistoryStore upsert transition guard", () => { it("allows valid interrupted → completed transition", async () => { const item = makeItem({ id: "task-guard-interrupted-complete", status: "interrupted" }) - await seedItems([item]) + await seedItems(tmpDir, [item]) store.dispose() store = new TaskHistoryStore(tmpDir) await store.initialize() @@ -1645,7 +1752,7 @@ describe("TaskHistoryStore upsert transition guard", () => { // to "active". Writing status: "active" must not throw as an invalid self-loop. const item = makeItem({ id: "task-guard-legacy" }) const { status: _status, ...legacyItem } = item - await seedItems([legacyItem]) + await seedItems(tmpDir, [legacyItem]) store.dispose() store = new TaskHistoryStore(tmpDir) await store.initialize() @@ -1656,7 +1763,7 @@ describe("TaskHistoryStore upsert transition guard", () => { it("allows upsert without a status field (no-op on status)", async () => { const item = makeItem({ id: "task-guard-4", status: "completed" }) - await seedItems([item]) + await seedItems(tmpDir, [item]) store.dispose() store = new TaskHistoryStore(tmpDir) await store.initialize() @@ -1713,72 +1820,6 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { const CHILD_ID = "child-tick" const PARENT_ID = "parent-tick" - /** - * Fake only what the tick scheduling needs: the 5-minute `setTimeout` clock - * and `Date` (consumed by the liveness guard). Everything else (fs I/O, - * microtasks) stays real so `flushUntil()` below can pump the event - * loop while the timer clock advances only 1 ms per yield. - */ - function useTickClock(): void { - vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }) - } - - /** - * Drain pending real fs I/O by polling an observable condition instead of - * burning a fixed number of yields. The tick's reconcile/repair chain - * completes on libuv callbacks that fake timers alone never advance, and - * each `advanceTimersByTimeAsync(1)` yields one REAL macrotask turn - * (processing the poll phase) while advancing the fake clock only 1 ms. - * The yield count the chain needs is environment-dependent (~155 yields on - * a fast local SSD; higher on contended CI runners — the old fixed - * 2000-yield pumps intermittently starved on ubuntu CI, which is exactly - * what this helper replaces). Polling the SAME final state the assertions - * check makes the wait deterministic without weakening them. The pump - * stops as soon as the condition holds, so correct-code runs stay fast, - * and the generous cap costs sub-second wall time even when exhausted - * because fake timers never sleep (measured ~123 ms per 55K idle yields). - * On exhaustion it THROWS with a state snapshot rather than silently - * proceeding, converting a future hang into a loud, diagnosable failure. - * - * Predicates MUST be cheap and side-effect free: poll the in-memory cache - * getters (`store.get(...)`, which never touches disk) or spy call logs. - */ - async function flushUntil( - predicate: () => boolean, - options: { maxYields?: number; label?: string; snapshot?: () => string } = {}, - ): Promise { - const { maxYields = 50_000, label = "flushUntil predicate", snapshot } = options - for (let i = 0; i < maxYields; i++) { - if (predicate()) { - return - } - await vi.advanceTimersByTimeAsync(1) - } - if (predicate()) { - return - } - let state = "snapshot unavailable" - try { - state = snapshot ? snapshot() : "no snapshot supplied" - } catch { - // A throwing snapshot must not mask the primary diagnostic below. - } - throw new Error( - `flushUntil: "${label}" was not satisfied within ${maxYields} yields (~${maxYields} ms of fake ` + - `time). The tick's async chain never settled; final state: ${state}.`, - ) - } - - async function seedItems(items: HistoryItem[]): Promise { - const tasksDir = path.join(tmpDir, "tasks") - await fs.mkdir(tasksDir, { recursive: true }) - for (const item of items) { - const taskDir = path.join(tasksDir, item.id) - await fs.mkdir(taskDir, { recursive: true }) - await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item)) - } - } - /** * Stateful mtime injection for the liveness guard. The guard computes * `Date.now() - mtimeMs` against the (fake) clock, and this injector returns @@ -1837,7 +1878,7 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) const [parent, child] = makeDelegatedPair() - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const s = (store = new TaskHistoryStore(tmpDir)) installChildAgeInjector() @@ -1973,7 +2014,7 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { it("does not repair a child that stays live across the periodic tick (no cross-window clobbering)", async () => { const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) const [parent, child] = makeDelegatedPair() - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const s = (store = new TaskHistoryStore(tmpDir)) installChildAgeInjector() @@ -2020,7 +2061,7 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { it("logs and keeps re-arming when the periodic delegation step throws", async () => { const [parent, child] = makeDelegatedPair() - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const internals = TaskHistoryStore.prototype as unknown as { runPeriodicDelegationReconciliation: () => Promise @@ -2081,50 +2122,6 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { const RECONCILE_INTERVAL_MS = (TaskHistoryStore as unknown as { RECONCILE_INTERVAL_MS: number }) .RECONCILE_INTERVAL_MS - function useTickClock(): void { - vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }) - } - - // Condition-polling pump; see the full doc comment on the identical helper - // in the "periodic delegation reconciliation" block above for the - // rationale (the fixed 2000-yield pumps intermittently starved on slow - // ubuntu CI runners). - async function flushUntil( - predicate: () => boolean, - options: { maxYields?: number; label?: string; snapshot?: () => string } = {}, - ): Promise { - const { maxYields = 50_000, label = "flushUntil predicate", snapshot } = options - for (let i = 0; i < maxYields; i++) { - if (predicate()) { - return - } - await vi.advanceTimersByTimeAsync(1) - } - if (predicate()) { - return - } - let state = "snapshot unavailable" - try { - state = snapshot ? snapshot() : "no snapshot supplied" - } catch { - // A throwing snapshot must not mask the primary diagnostic below. - } - throw new Error( - `flushUntil: "${label}" was not satisfied within ${maxYields} yields (~${maxYields} ms of fake ` + - `time). The tick's async chain never settled; final state: ${state}.`, - ) - } - - async function seedItems(items: HistoryItem[]): Promise { - const tasksDir = path.join(tmpDir, "tasks") - await fs.mkdir(tasksDir, { recursive: true }) - for (const item of items) { - const taskDir = path.join(tasksDir, item.id) - await fs.mkdir(taskDir, { recursive: true }) - await fs.writeFile(path.join(taskDir, "history_item.json"), JSON.stringify(item)) - } - } - /** * Read the private `locallyActiveTaskIds` set — the exact piece of state every * ownership-track mutant below (L278/L299/L324/L566/L569/L1181/L1188/L1189) @@ -2196,7 +2193,7 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const s = (store = new TaskHistoryStore(tmpDir)) await s.initialize() @@ -2239,7 +2236,7 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { // Step 3: rewrite disk so the same child id is once more an ACTIVE orphan of a delegated // parent (as if another window crashed mid-delegation), then reload into a fresh store. const [parent, child] = delegatedPair(parentId, childId) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const s2 = (store = new TaskHistoryStore(tmpDir)) useTickClock() @@ -2306,7 +2303,7 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { const childId = "child-l566-undef" const parentId = "parent-l566-undef" const [parent, child] = delegatedPair(parentId, childId) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const s = (store = new TaskHistoryStore(tmpDir)) useTickClock() @@ -2411,7 +2408,7 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { awaitingChildId: child.id, delegatedToId: child.id, }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const tasksDir = path.join(tmpDir, "tasks") const intentPath = path.join(tasksDir, GlobalFileNames.delegationRepairIntent) await fs.writeFile(intentPath, JSON.stringify(makeRepairIntent(parent, child))) @@ -2498,7 +2495,7 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { // - L1080->false: a concurrent second call would NOT see the flag set and would run twice. // - L1087->true: after completion the flag stays set, so every subsequent call no-ops. const [parent, child] = delegatedPair("parent-flag", "child-flag") - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) const s = (store = new TaskHistoryStore(tmpDir)) installStaleChildInjector(child.id) await s.initialize() @@ -2596,4 +2593,108 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { expect(ownedIds(s).has("pf-first")).toBe(true) writeSpy.mockRestore() }) + + it("markLocallyActive claims ownership before the first runtime write, shielding a stale-mtime child from the tick", async () => { + // Seam for the resumed-task race fixed in ClineProvider.createTaskWithHistoryItemUnlocked: + // a resumed history task calls store.markLocallyActive(taskId) BEFORE its run is + // scheduled, so the periodic pass excludes the id from the persisted-active snapshot + // even while Task.run()'s first active-status write is still in flight. This simulates + // exactly that eager claim: with a stale-looking mtime injector armed, the owned child + // must survive the tick untouched. If markLocallyActive's add() were dropped, the id + // would not be excluded, the guard would see the stale mtime, and the child would be + // repaired to interrupted — failing the status assertions below. + const childId = "child-eager-claim" + const parentId = "parent-eager-claim" + const [parent, child] = delegatedPair(parentId, childId) + await seedItems(tmpDir, [parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + await s.initialize() + + s.markLocallyActive(childId) + expect(ownedIds(s).has(childId)).toBe(true) + + // The child now LOOKS like a crash orphan, but local ownership excludes it + // from this tick's persisted-active snapshot. + installStaleChildInjector(childId) + + const timerState = s as unknown as { reconcileTimer: ReturnType | null } + const timerBeforeTick = timerState.reconcileTimer + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushUntil(() => timerState.reconcileTimer !== timerBeforeTick, { + label: "tick completed without repairing the eagerly-claimed child", + snapshot: () => `child=${s.get(childId)?.status} parent=${s.get(parentId)?.status}`, + }) + + expect(s.get(childId)?.status).toBe("active") + expect(s.get(parentId)?.status).toBe("delegated") + }) + + it("transient stat failure at the liveness guard skips repair and logs 'Skipping repair for live child' (ENOENT-only classification)", async () => { + // End-to-end for the getChildFileMtimeMs classification through the real + // periodic-delegation pass: at tick time the guard's stat of the child's + // history file fails with a NON-ENOENT code. Under ENOENT-only semantics + // the probe treats that as evidence of life (future mtime), the + // live-elsewhere guard holds, and repair is skipped with a warn log so a + // later tick retries. Under the old catch → undefined behavior the guard + // would see "not live" and repair the child to interrupted — the status + // assertions below would then fail. + // The stat failure is injected via the private `getTaskFilePath` seam by + // embedding a NUL byte in the child's path only: Node's real `fs.stat` + // rejects such paths with ERR_INVALID_ARG_VALUE on every platform (never + // ENOENT), so the classifier runs against the genuine fs call. The tick is + // invoked directly (same private-method pattern as the neighboring + // runPeriodicDelegationReconciliation tests) and no disk writes happen + // after initialize(), so the fs watcher never fires and reconcile() can + // never evict the child while its path is poisoned. Date is frozen to a + // fixed instant in the past so the freshly seeded mtimes read as live at + // startup and the startup pass provably skips the repair. + const FIXED_NOW = 1_756_886_400_000 // 2025-09-03T08:00:00.000Z, before real seed mtimes + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + try { + const [parent, child] = delegatedPair("parent-eacces", "child-eacces") + await seedItems(tmpDir, [parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + // Startup: fresh (future-relative-to-FIXED_NOW) mtimes → live → skipped. + expect(s.get(child.id)?.status).toBe("active") + // Clear the startup skip-log so the assertion below only observes the + // TICK's decision after the stat failure is armed. + warnSpy.mockClear() + + const tasksDir = path.join(tmpDir, "tasks") + const pathProbe = TaskHistoryStore.prototype as unknown as { + getTaskFilePath: (taskId: string) => Promise + } + const originalGetTaskFilePath = pathProbe.getTaskFilePath + const pathSpy = vi + .spyOn(pathProbe, "getTaskFilePath") + .mockImplementation((taskId: string) => + taskId === child.id + ? Promise.resolve(path.join(tasksDir, taskId, "his\0tory_item.json")) + : originalGetTaskFilePath.call(s, taskId), + ) + try { + const internals = TaskHistoryStore.prototype as unknown as { + runPeriodicDelegationReconciliation: () => Promise + } + await internals.runPeriodicDelegationReconciliation.call(s) + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child")) + expect(s.get(child.id)?.status).toBe("active") + expect(s.get(parent.id)?.status).toBe("delegated") + expect(errorSpy).not.toHaveBeenCalled() + } finally { + pathSpy.mockRestore() + } + } finally { + warnSpy.mockRestore() + errorSpy.mockRestore() + nowSpy.mockRestore() + } + }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 87a899344c..956fbb9370 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -1368,6 +1368,17 @@ export class ClineProvider diffFuzzyThreshold, }) + // Eagerly claim local session ownership so the store's periodic delegation + // reconciliation cannot treat this resumed task as a crash orphan while + // Task.run()'s first active-status write is still in flight (resumeTaskFromHistory + // starts with an async disk read and scheduleTask may queue the run). Every Task + // built here passes historyItem without task/images, so Task's own + // `_isHistoryTask = !!historyItem && !task && !images` discriminator + // (src/core/task/Task.ts) is always true for this method — the unconditional + // claim below mirrors it exactly. Ownership stays self-correcting via + // trackLocalSessionOwnership: the task's next non-active status write releases it. + this.taskHistoryStore.markLocallyActive(task.taskId) + if (isRehydratingCurrentTask) { // Replace the current task in-place to avoid UI flicker const oldTask = this.taskRegistry.current diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index f2832b2468..069bda3f88 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -272,6 +272,7 @@ vi.mock("../../task-persistence", async (importOriginal) => { delete: vi.fn().mockResolvedValue(undefined), deleteMany: vi.fn().mockResolvedValue(undefined), migrateFromGlobalState: vi.fn().mockResolvedValue(undefined), + markLocallyActive: vi.fn(), } }), readApiMessages: vi.fn().mockResolvedValue([]), From 36f41e6cd225f9fe3981f1b205dd83fa1d8f523a Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Tue, 8 Sep 2026 16:42:06 +0900 Subject: [PATCH 15/19] fix(delegation): roll back ownership claim on non-started resume paths; guard ENOENT rename window per CodeRabbit round-3 --- src/__tests__/helpers/provider-stub.ts | 10 +- src/core/task-persistence/TaskHistoryStore.ts | 44 ++- .../TaskHistoryStore.reconciliation.spec.ts | 92 +++++- src/core/webview/ClineProvider.ts | 111 ++++--- .../ClineProvider.markLocallyActive.spec.ts | 304 ++++++++++++++++++ 5 files changed, 510 insertions(+), 51 deletions(-) create mode 100644 src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts diff --git a/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts index a83344ba72..0c6a6fb694 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -6,7 +6,11 @@ type ProviderStubFields = { delegationTransitionLocks?: Map> cancelledDelegationChildIds?: Set log?: ReturnType - taskHistoryStore?: { get: (id: string) => unknown; markLocallyActive?: (taskId: string) => void } + taskHistoryStore?: { + get: (id: string) => unknown + markLocallyActive?: (taskId: string) => void + markLocallyInactive?: (taskId: string) => void + } taskRegistry?: TaskRegistry clineStack?: Task[] tasks?: Task[] @@ -37,7 +41,9 @@ export function makeProviderStub(stub: T): ClineProvider { s.delegationTransitionLocks ??= new Map() s.cancelledDelegationChildIds ??= new Set() s.log ??= vi.fn() - s.taskHistoryStore ??= { get: () => undefined, markLocallyActive: () => {} } + s.taskHistoryStore ??= { get: () => undefined } + s.taskHistoryStore.markLocallyActive ??= () => {} + s.taskHistoryStore.markLocallyInactive ??= () => {} // Convert legacy clineStack array into a TaskRegistry if (!s.taskRegistry) { diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 75fee236fc..aa9f2b9242 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -582,6 +582,16 @@ export class TaskHistoryStore { this.locallyActiveTaskIds.add(taskId) } + /** + * Release a task id claimed by `markLocallyActive` when its session did not + * start (preparation failure, scheduler rejection, or startTask disabled). + * Re-running reconciliation for the id is safe: without local ownership the + * periodic pass treats it like any other persisted record. + */ + public markLocallyInactive(taskId: string): void { + this.locallyActiveTaskIds.delete(taskId) + } + /** * Replay the durable active-child repair intent, if one was left by a crash. * The expected fields are guards: an intent may update only the missing side @@ -1245,12 +1255,13 @@ export class TaskHistoryStore { /** * Returns the mtime (ms epoch) of the child's history_item.json, or undefined - * only when the file is genuinely absent (ENOENT). A recent mtime means another - * live extension host is actively persisting this child, so startup repair must - * not treat it as a crash orphan. Any OTHER stat failure (EMFILE, EACCES, EIO, - * ...) is not evidence of absence: the child is reported with a future mtime so - * every `Date.now() - mtimeMs < threshold` liveness guard holds, repair is - * skipped, and a later reconciliation tick retries instead. + * only when the file is genuinely absent (ENOENT with no fresh advisory lock). + * A recent mtime means another live extension host is actively persisting this + * child, so startup repair must not treat it as a crash orphan. Any OTHER stat + * failure (EMFILE, EACCES, EIO, ...) is not evidence of absence: the child is + * reported with a future mtime so every `Date.now() - mtimeMs < threshold` + * liveness guard holds, repair is skipped, and a later reconciliation tick + * retries instead. */ private async getChildFileMtimeMs(childId: string): Promise { try { @@ -1258,10 +1269,25 @@ export class TaskHistoryStore { const stat = await fs.stat(filePath) return stat.mtimeMs } catch (error) { - // ENOENT: the file is genuinely absent → no window is persisting it; - // conservatively proceed with repair. Any other stat failure is treated - // as evidence of life via a threshold-shifted future timestamp. + // ENOENT: no window is persisting it UNLESS the absence falls inside + // safeWriteJson's rename window — see the lock check below. Any other + // stat failure is treated as evidence of life via a threshold-shifted + // future timestamp. if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + // The write path (safeWriteJson) renames history_item.json to a backup + // and back while holding the advisory lock, so a missing file during + // that window does NOT mean no window is writing it. Same convention + // as reconcile(): a fresh .lock file means a write is in progress — + // treat the child as live and let a later tick retry. + try { + const lockPath = (await this.getTaskFilePath(childId)) + ".lock" + const lockStat = await fs.stat(lockPath) + if (Date.now() - lockStat.mtimeMs < LOCK_STALE_MS) { + return Date.now() + TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS + } + } catch { + // No lock file — the file is genuinely absent. + } return undefined } return Date.now() + TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 52ee21913e..3b3e17cd3c 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -20,7 +20,15 @@ const writeJson = async (filePath: string, data: unknown): Promise => { const safeWriteJsonMock = vi.hoisted(() => vi.fn()) -vi.mock("../../../utils/safeWriteJson", () => ({ safeWriteJson: safeWriteJsonMock })) +// Spread the real module so `LOCK_STALE_MS` keeps its actual value: both +// reconcile() and getChildFileMtimeMs() compare lock freshness against it, +// and a factory mock that omits the export would silently disable those +// checks. Only `safeWriteJson` itself is replaced (with the fs-backed +// writeJson default below). +vi.mock("../../../utils/safeWriteJson", async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, safeWriteJson: safeWriteJsonMock } +}) safeWriteJsonMock.mockImplementation(writeJson) @@ -369,6 +377,40 @@ describe("TaskHistoryStore reconcileDelegationState", () => { } }) + it("classifies a getChildFileMtimeMs ENOENT inside a fresh advisory lock as live (safeWriteJson rename window)", async () => { + // Direct probe coverage for the rename-window race: safeWriteJson renames + // history_item.json to a backup and back WHILE holding the `.lock` + // advisory lock (proper-lockfile creates it via mkdir, so stat works on + // the lock directory). During that window fs.stat returns ENOENT even + // though a peer window is mid-write, so the probe must consult the lock + // exactly like reconcile() does: fresh lock → live (future mtime), no + // lock → genuinely absent. The tmpDir rm in afterEach cleans the lock. + const internals = store as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const child = makeItem({ id: "lock-window-child", status: "active" }) + await seedItems(tmpDir, [child]) + const historyPath = path.join(tmpDir, "tasks", child.id, GlobalFileNames.historyItem) + const lockPath = `${historyPath}.lock` + + // Simulate the rename window: history file momentarily absent, lock held. + await fs.rm(historyPath) + await fs.mkdir(lockPath) + + const probed = await internals.getChildFileMtimeMs(child.id) + // A numeric (live) result is required; the narrowing below asserts it. + expect(probed).toBeDefined() + if (typeof probed === "number") { + // Future timestamp ⇒ negative age ⇒ every liveness guard holds. + expect(Date.now() - probed).toBeLessThan(0) + expect(Date.now() - probed).toBeLessThan(LIVE_CHILD_MTIME_THRESHOLD_MS) + } + + // Lock released while the file is still gone: genuinely absent → undefined. + await fs.rmdir(lockPath) + expect(await internals.getChildFileMtimeMs(child.id)).toBeUndefined() + }) + afterEach(async () => { mtimeSpy?.mockRestore() mtimeSpy = undefined @@ -2697,4 +2739,52 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { nowSpy.mockRestore() } }) + + it("keeps a child whose history file is ENOENT during a fresh-lock rename window active through the tick", async () => { + // End-to-end companion of the direct lock-window probe test: the startup + // pass sees fresh (future-relative-to-FIXED_NOW) mtimes and skips the + // repair, then the child's history file disappears mid-write while the + // advisory `.lock` directory stays fresh. The periodic delegation pass's + // liveness guard must classify the child LIVE via the lock check inside + // getChildFileMtimeMs and skip the crash-orphan repair. Without that lock + // check, ENOENT → undefined → "not live" → the child would be repaired to + // interrupted and the parent released — failing the status assertions. + // The tick is invoked directly (same private-method pattern as the + // neighboring tests) and no disk write happens after the simulated + // deletion, so the fs watcher never fires. Lock dir cleanup rides the + // afterEach tmpDir rm. + const FIXED_NOW = 1_756_886_400_000 // 2025-09-03T08:00:00.000Z, before real seed mtimes + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const [parent, child] = delegatedPair("parent-lockwin-tick", "child-lockwin-tick") + await seedItems(tmpDir, [parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + // Startup: fresh (future-relative-to-FIXED_NOW) mtimes → live → skipped. + expect(s.get(child.id)?.status).toBe("active") + warnSpy.mockClear() + + // Simulate the rename window: file gone, lock held (fresh mtime). + const historyPath = path.join(tmpDir, "tasks", child.id, GlobalFileNames.historyItem) + await fs.rm(historyPath) + await fs.mkdir(`${historyPath}.lock`) + + const internals = TaskHistoryStore.prototype as unknown as { + runPeriodicDelegationReconciliation: () => Promise + } + await internals.runPeriodicDelegationReconciliation.call(s) + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child")) + expect(s.get(child.id)?.status).toBe("active") + expect(s.get(parent.id)?.status).toBe("delegated") + expect(errorSpy).not.toHaveBeenCalled() + } finally { + warnSpy.mockRestore() + errorSpy.mockRestore() + nowSpy.mockRestore() + } + }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 956fbb9370..d29430724b 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -163,10 +163,21 @@ function runDelegationTransition( return current } -function scheduleTask(scheduler: TaskScheduler, task: Task, source: string): void { +function scheduleTask( + scheduler: TaskScheduler, + task: Task, + source: string, + onScheduleFailure?: (error: unknown) => void, +): void { void scheduler .schedule(task, () => task.run()) - .catch((error) => console.error(`[${source}] taskScheduler.schedule failed:`, error)) + .catch((error) => { + console.error(`[${source}] taskScheduler.schedule failed:`, error) + // Fire-and-forget stays fire-and-forget; the optional hook lets the + // caller roll back state that was claimed before scheduling (e.g. the + // eager markLocallyActive claim in createTaskWithHistoryItemUnlocked). + onScheduleFailure?.(error) + }) } type GetStateOptions = { @@ -1379,53 +1390,75 @@ export class ClineProvider // trackLocalSessionOwnership: the task's next non-active status write releases it. this.taskHistoryStore.markLocallyActive(task.taskId) - if (isRehydratingCurrentTask) { - // Replace the current task in-place to avoid UI flicker - const oldTask = this.taskRegistry.current + // Roll the eager claim back on every path that never reaches a scheduled run: + // a preparation/stack failure throws before scheduling (catch below), and a + // scheduler rejection is reported through scheduleTask's onScheduleFailure + // hook. Without the release, an id whose task never started would be excluded + // from orphan reconciliation for the lifetime of this window. startTask:false + // is intentionally NOT released: its only production caller + // (reopenParentFromDelegation) persists the task's `active` history item + // through the delegation transition — which re-registers ownership via + // trackLocalSessionOwnership — and immediately runs it via + // Task.resumeAfterDelegation(), so releasing here would reopen the exact + // crash-orphan window this claim closes. + try { + if (isRehydratingCurrentTask) { + // Replace the current task in-place to avoid UI flicker + const oldTask = this.taskRegistry.current - if (oldTask) { - // Abort the old task to stop running processes and mark as abandoned - try { - await oldTask.abortTask(true) - } catch (e) { - this.log( - `[createTaskWithHistoryItem] abortTask() failed for old task ${oldTask.taskId}.${oldTask.instanceId}: ${e.message}`, - ) - } + if (oldTask) { + // Abort the old task to stop running processes and mark as abandoned + try { + await oldTask.abortTask(true) + } catch (e) { + this.log( + `[createTaskWithHistoryItem] abortTask() failed for old task ${oldTask.taskId}.${oldTask.instanceId}: ${e.message}`, + ) + } - // Remove event listeners from the old task - const cleanupFunctions = this.taskEventListeners.get(oldTask) - if (cleanupFunctions) { - cleanupFunctions.forEach((cleanup) => cleanup()) - this.taskEventListeners.delete(oldTask) - } + // Remove event listeners from the old task + const cleanupFunctions = this.taskEventListeners.get(oldTask) + if (cleanupFunctions) { + cleanupFunctions.forEach((cleanup) => cleanup()) + this.taskEventListeners.delete(oldTask) + } - // Replace in-place: preserves stack index and current pointer - this.taskRegistry.replace(oldTask.taskId, task) - } + // Replace in-place: preserves stack index and current pointer + this.taskRegistry.replace(oldTask.taskId, task) + } - task.emit(RooCodeEventName.TaskFocused) + task.emit(RooCodeEventName.TaskFocused) - // Perform preparation tasks and set up event listeners - await this.performPreparationTasks(task) + // Perform preparation tasks and set up event listeners + await this.performPreparationTasks(task) - this.log( - `[createTaskWithHistoryItem] rehydrated task ${task.taskId}.${task.instanceId} in-place (flicker-free)`, - ) + this.log( + `[createTaskWithHistoryItem] rehydrated task ${task.taskId}.${task.instanceId} in-place (flicker-free)`, + ) - if (options?.startTask !== false) { - scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem") - } - } else { - await this.addClineToStack(task) + if (options?.startTask !== false) { + scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem", () => + this.taskHistoryStore.markLocallyInactive(task.taskId), + ) + } + } else { + await this.addClineToStack(task) - this.log( - `[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, - ) + this.log( + `[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, + ) - if (options?.startTask !== false) { - scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem") + if (options?.startTask !== false) { + scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem", () => + this.taskHistoryStore.markLocallyInactive(task.taskId), + ) + } } + } catch (error) { + // Preparation/stack failure: the task never started, so release the claim + // and let the caller handle the rethrown error. + this.taskHistoryStore.markLocallyInactive(task.taskId) + throw error } // Check if there's a pending edit after checkpoint restoration diff --git a/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts new file mode 100644 index 0000000000..3f44931182 --- /dev/null +++ b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts @@ -0,0 +1,304 @@ +// Regression tests for the eager `markLocallyActive` claim in +// ClineProvider.createTaskWithHistoryItemUnlocked and its rollback on every +// path that does not reach a scheduled run (CodeRabbit round-3 Finding B/E). +// +// npx vitest run core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts +// +// Test-double pattern mirrors src/__tests__/single-open-invariant.spec.ts: +// a plain provider object + the real prototype methods, so the actual +// createTaskWithHistoryItemUnlocked wiring (claim → branch → release) is +// exercised without a VS Code host. + +import { describe, it, expect, vi, afterEach, beforeEach } from "vitest" + +import { ClineProvider } from "../ClineProvider" +import { TaskRegistry } from "../../task/TaskRegistry" +import { type Task } from "../../task/Task" +import { providerIdentifiers } from "@roo-code/types/provider-identifiers" + +type HistoryItemLike = Parameters[0] + +type PrivateClineProviderMethods = { + createTaskWithHistoryItem: ( + this: unknown, + historyItem: HistoryItemLike, + options?: { startTask?: boolean }, + ) => ReturnType +} + +const privateClineProvider = ClineProvider.prototype as unknown as PrivateClineProviderMethods + +vi.mock("../../task/Task", () => { + // The id must come from the history item so claim/release target the exact + // created task; the stub only implements the surface the provider touches. + class TaskStub { + public taskId: string + public instanceId = "stub-inst" + public parentTask?: unknown + public abort = false + public abandoned = false + public abortTask = vi.fn().mockResolvedValue(undefined) + constructor(opts: { historyItem?: { id: string }; onCreated?: (t: TaskStub) => void }) { + this.taskId = opts.historyItem?.id ?? `task-${Math.random().toString(36).slice(2, 8)}` + opts.onCreated?.(this) + } + run() { + return Promise.resolve() + } + on() {} + off() {} + emit() {} + } + return { Task: TaskStub } +}) + +type MockFn = ReturnType + +type OwnershipStore = { + get: (id: string) => unknown + markLocallyActive: MockFn + markLocallyInactive: MockFn +} + +type ProviderStubObject = { + historyTaskCreationQueue: Promise + getCurrentTask: MockFn + taskRegistry: TaskRegistry + taskHistoryStore: OwnershipStore + evictCurrentTask: MockFn + removeClineFromStack: MockFn + addClineToStack: MockFn + performPreparationTasks: MockFn + taskScheduler: { schedule: MockFn } + taskEventListeners: Map void>> + log: MockFn + customModesManager: { getCustomModes: MockFn } + providerSettingsManager: { getModeConfigId: MockFn; listConfig: MockFn } + getState: MockFn + getPendingEditOperation: MockFn + clearPendingEditOperation: MockFn + postStateToWebview: MockFn + context: Record + contextProxy: Record +} + +function makeStore(): OwnershipStore { + return { + get: vi.fn(() => undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + } +} + +function makeProvider(store: OwnershipStore, overrides: Partial = {}): ProviderStubObject { + const registry = new TaskRegistry() + return { + historyTaskCreationQueue: Promise.resolve(), + getCurrentTask: vi.fn(() => registry.current), + taskRegistry: registry, + taskHistoryStore: store, + evictCurrentTask: vi.fn().mockResolvedValue(undefined), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + addClineToStack: vi.fn().mockResolvedValue(undefined), + performPreparationTasks: vi.fn().mockResolvedValue(undefined), + taskScheduler: { schedule: vi.fn().mockResolvedValue(undefined) }, + taskEventListeners: new Map(), + log: vi.fn(), + customModesManager: { getCustomModes: vi.fn().mockResolvedValue([]) }, + providerSettingsManager: { + getModeConfigId: vi.fn().mockResolvedValue(undefined), + listConfig: vi.fn().mockResolvedValue([]), + }, + getState: vi.fn().mockResolvedValue({ + apiConfiguration: { apiProvider: providerIdentifiers.anthropic, consecutiveMistakeLimit: 0 }, + enableCheckpoints: true, + checkpointTimeout: 60, + experiments: {}, + cloudUserInfo: null, + taskSyncEnabled: false, + }), + getPendingEditOperation: vi.fn().mockReturnValue(undefined), + clearPendingEditOperation: vi.fn(), + postStateToWebview: vi.fn().mockResolvedValue(undefined), + context: { extension: { packageJSON: {} }, globalStorageUri: { fsPath: "/tmp" } }, + contextProxy: { + extensionUri: {}, + getValue: vi.fn(), + setValue: vi.fn(), + setProviderSettings: vi.fn(), + getProviderSettings: vi.fn(() => ({})), + }, + ...overrides, + } +} + +function makeHistoryItem(id: string): HistoryItemLike { + return { + id, + number: 1, + ts: Date.now(), + task: "test task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/tmp", + } +} + +/** Seed a registry with a current task whose id matches `historyId` (rehydrate case). */ +function makeRehydrateProvider(store: OwnershipStore, historyId: string, overrides: Partial = {}) { + const existing = { + taskId: historyId, + instanceId: "old-inst", + abort: false, + abandoned: false, + abortTask: vi.fn().mockResolvedValue(undefined), + emit: vi.fn(), + } + const registry = new TaskRegistry() + registry.push(existing as unknown as Task) + return makeProvider(store, { getCurrentTask: vi.fn(() => existing), taskRegistry: registry, ...overrides }) +} + +async function flushMicrotasks(): Promise { + // scheduleTask's failure hook runs on the rejection microtask chain of a + // fire-and-forget promise; drain it before asserting non-invocations. + for (let i = 0; i < 10; i++) { + await Promise.resolve() + } +} + +describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () => { + let consoleErrorSpy: ReturnType + + beforeEach(() => { + // scheduleTask keeps logging scheduler rejections via console.error; the + // rejection tests exercise that path on purpose. + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + }) + + afterEach(() => { + consoleErrorSpy.mockRestore() + }) + + it("Finding E wiring: claims ownership for the created task id on the success path and never releases it before the run", async () => { + // Removing the markLocallyActive(task.taskId) call from + // createTaskWithHistoryItemUnlocked must fail THIS test — that is the + // provider-wiring assertion CodeRabbit asked for. + const store = makeStore() + const provider = makeProvider(store) + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-success"), + ) + + expect(task.taskId).toBe("hist-success") + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-success") + await flushMicrotasks() + expect(store.markLocallyInactive).not.toHaveBeenCalled() + expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + }) + + it("claims ownership on the in-place rehydrate success path without releasing it", async () => { + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-rehydrate-ok") + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-rehydrate-ok"), + ) + + expect(task.taskId).toBe("hist-rehydrate-ok") + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-rehydrate-ok") + await flushMicrotasks() + expect(store.markLocallyInactive).not.toHaveBeenCalled() + expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + }) + + it("releases the claim when preparation fails on the rehydrate path and rethrows", async () => { + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-prep-fail", { + performPreparationTasks: vi.fn().mockRejectedValue(new Error("prep exploded")), + }) + + await expect( + privateClineProvider.createTaskWithHistoryItem.call(provider, makeHistoryItem("hist-prep-fail")), + ).rejects.toThrow("prep exploded") + + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-prep-fail") + expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-prep-fail") + expect(provider.taskScheduler.schedule).not.toHaveBeenCalled() + }) + + it("releases the claim when addClineToStack fails on the stack path and rethrows", async () => { + const store = makeStore() + const provider = makeProvider(store, { + addClineToStack: vi.fn().mockRejectedValue(new Error("stack exploded")), + }) + + await expect( + privateClineProvider.createTaskWithHistoryItem.call(provider, makeHistoryItem("hist-stack-fail")), + ).rejects.toThrow("stack exploded") + + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-stack-fail") + expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-stack-fail") + expect(provider.taskScheduler.schedule).not.toHaveBeenCalled() + }) + + it("releases the claim when the scheduler rejects the run (stack path)", async () => { + const store = makeStore() + const provider = makeProvider(store, { + taskScheduler: { schedule: vi.fn().mockRejectedValue(new Error("permit failed")) }, + }) + + // The provider call itself still resolves — scheduleTask is + // fire-and-forget — so the rollback must arrive via the failure hook. + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-sched-fail"), + ) + expect(task.taskId).toBe("hist-sched-fail") + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-sched-fail") + + await vi.waitFor(() => expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-sched-fail")) + }) + + it("releases the claim when the scheduler rejects the run (rehydrate path)", async () => { + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-sched-fail-re", { + taskScheduler: { schedule: vi.fn().mockRejectedValue(new Error("permit failed")) }, + }) + + await privateClineProvider.createTaskWithHistoryItem.call(provider, makeHistoryItem("hist-sched-fail-re")) + + await vi.waitFor(() => expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-sched-fail-re")) + }) + + it("keeps the claim when startTask is false: the installed task starts via a later explicit path, not the scheduler", async () => { + // The only production caller that passes startTask:false is + // reopenParentFromDelegation (ClineProvider.ts step 7): the parent's + // `active` history write during the delegation transition already + // re-registered ownership via trackLocalSessionOwnership, and the caller + // immediately runs the installed task through Task.resumeAfterDelegation() + // — which persists active status itself. Releasing here would reopen the + // crash-orphan window the eager claim exists to close, so the contract is + // "retain the claim when scheduling is skipped" — this test locks it in. + const store = makeStore() + const provider = makeProvider(store) + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-nostart"), + { + startTask: false, + }, + ) + + expect(task.taskId).toBe("hist-nostart") + expect(provider.taskScheduler.schedule).not.toHaveBeenCalled() + await flushMicrotasks() + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-nostart") + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) +}) From e90c99a157fbd9720691c0968887867ad5849521 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Tue, 8 Sep 2026 17:09:38 +0900 Subject: [PATCH 16/19] test(delegation): add markLocallyInactive to wholesale TaskHistoryStore mocks --- src/__tests__/single-open-invariant.spec.ts | 30 +++++++++++++++---- .../ClineProvider.flicker-free-cancel.spec.ts | 1 + 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/__tests__/single-open-invariant.spec.ts b/src/__tests__/single-open-invariant.spec.ts index 453665801b..db0e945a70 100644 --- a/src/__tests__/single-open-invariant.spec.ts +++ b/src/__tests__/single-open-invariant.spec.ts @@ -80,7 +80,11 @@ describe("Single-open-task invariant", () => { taskRegistry: registry, taskScheduler: { schedule: schedulespy }, getCurrentTask: vi.fn(() => existingTask), - taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, + taskHistoryStore: { + get: vi.fn(() => undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -168,7 +172,11 @@ describe("Single-open-task invariant", () => { const provider = { getCurrentTask: vi.fn(() => undefined), // ensure not rehydrating - taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, + taskHistoryStore: { + get: vi.fn(() => undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -243,7 +251,11 @@ describe("Single-open-task invariant", () => { const provider = { getCurrentTask: vi.fn(() => existingTask), taskRegistry: registry, - taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, + taskHistoryStore: { + get: vi.fn(() => undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -319,7 +331,11 @@ describe("Single-open-task invariant", () => { historyTaskCreationQueue: Promise.resolve(), getCurrentTask: vi.fn(() => registry.current), taskRegistry: registry, - taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, + taskHistoryStore: { + get: vi.fn(() => undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + }, evictCurrentTask, addClineToStack: vi.fn().mockImplementation(async (task: Task) => registry.push(task)), log: vi.fn(), @@ -386,7 +402,11 @@ describe("Single-open-task invariant", () => { const provider = { context: {} as unknown, getCurrentTask: vi.fn(() => undefined), - taskHistoryStore: { get: vi.fn(() => undefined), markLocallyActive: vi.fn() }, + taskHistoryStore: { + get: vi.fn(() => undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index 069bda3f88..45a19b8023 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -273,6 +273,7 @@ vi.mock("../../task-persistence", async (importOriginal) => { deleteMany: vi.fn().mockResolvedValue(undefined), migrateFromGlobalState: vi.fn().mockResolvedValue(undefined), markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), } }), readApiMessages: vi.fn().mockResolvedValue([]), From 831de26f5be53111de6bc6d75d72975259c67439 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Tue, 8 Sep 2026 17:54:24 +0900 Subject: [PATCH 17/19] test(delegation): kill 19 changed-code mutants in claim/rollback and liveness regions --- .../TaskHistoryStore.reconciliation.spec.ts | 159 +++++++++++ .../ClineProvider.markLocallyActive.spec.ts | 260 +++++++++++++++++- 2 files changed, 414 insertions(+), 5 deletions(-) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 3b3e17cd3c..22ea663cfa 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -7,6 +7,7 @@ import * as os from "os" import type { HistoryItem } from "@roo-code/types" import { GlobalFileNames } from "../../../shared/globalFileNames" +import { LOCK_STALE_MS } from "../../../utils/safeWriteJson" import { TaskHistoryStore, assertValidTransition } from "../TaskHistoryStore" vi.mock("../../../utils/storage", () => ({ @@ -2430,6 +2431,164 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { expect(ownedIds(s).has("dm-2")).toBe(true) }) + it("markLocallyInactive releases an eager markLocallyActive claim so the tick repairs the orphan (kills L592 CallExpression)", async () => { + // markLocallyInactive: `this.locallyActiveTaskIds.delete(taskId)` — the rollback of the + // eager claim ClineProvider takes before scheduling. Its documented consumer is the + // periodic tick's orphan filter; the sequence below exercises both phases of that + // contract against the exact claim/release cycle production uses: + // phase 1: claim (markLocallyActive) → the tick leaves the orphan alone (owned); + // phase 2: release (markLocallyInactive, the scheduler-rejection rollback) → the + // next tick repairs it (child → interrupted, parent → active). + // Under the `;` mutant the set keeps the id, phase 2 repairs nothing, and BOTH the + // direct membership assertion and the end-status assertions fail. + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const childId = "orphan-l592" + const parentId = "parent-l592" + const [parent, child] = delegatedPair(parentId, childId) + await seedItems(tmpDir, [parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + // Fresh seed mtimes → the startup pass treats the child as live and skips repair. + await s.initialize() + expect(s.get(childId)?.status).toBe("active") + + // Phase 1: the eager claim. Make the mtime stale for the tick, then run it. + s.markLocallyActive(childId) + installStaleChildInjector(childId) + const timerState = s as unknown as { reconcileTimer: ReturnType | null } + const timerBeforeFirstTick = timerState.reconcileTimer + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushUntil(() => timerState.reconcileTimer !== timerBeforeFirstTick, { + label: "locally-owned orphan was left alone by the tick", + snapshot: () => `child=${s.get(childId)?.status} parent=${s.get(parentId)?.status}`, + }) + expect(ownedIds(s).has(childId)).toBe(true) + expect(s.get(childId)?.status).toBe("active") + + // Phase 2: the rollback. Direct set assertion first (the kill), then the + // behavioral consequence: the id must no longer be excluded from the tick's + // repair candidate set. + s.markLocallyInactive(childId) + expect(ownedIds(s).has(childId)).toBe(false) + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushUntil(() => s.get(childId)?.status === "interrupted", { + label: "released claim let the tick repair the orphan", + snapshot: () => `child=${s.get(childId)?.status} parent=${s.get(parentId)?.status}`, + }) + expect(s.get(childId)?.status).toBe("interrupted") + expect(s.get(parentId)?.status).toBe("active") + expect(errorSpy).not.toHaveBeenCalled() + } finally { + errorSpy.mockRestore() + } + }) + + it("a code-less stat rejection (null/string) is classified live, not absent, and never throws (kills L1276 OptionalChaining)", async () => { + // getChildFileMtimeMs: `if ((error as NodeJS.ErrnoException)?.code === "ENOENT")`. The + // optional chain is a real guard: dropping it (`(error).code`) dereferences null when + // the caught rejection has no payload wrapper at all. fs.stat itself cannot be spied + // (sealed ESM namespace), so the seam is the private getTaskFilePath promise — the + // awaited call at the top of getChildFileMtimeMs. A REJECTED getTaskFilePath throws + // into the same catch the classifier reads, with exactly the payload we choose: + // correct code: null?.code → undefined ≠ "ENOENT" → transient → live future mtime; + // mutant: null.code → TypeError thrown out of getChildFileMtimeMs. + // The string case additionally pins the "rejection without a `.code` property" + // classification: a plain non-Error rejection is evidence of life, never absence. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + const internals = s as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const probe = TaskHistoryStore.prototype as unknown as { + getTaskFilePath: (taskId: string) => Promise + } + const originalGetTaskFilePath = probe.getTaskFilePath + + const pathSpy = vi + .spyOn(probe, "getTaskFilePath") + .mockImplementation((taskId: string) => + taskId === "null-reject-child" ? Promise.reject(null) : originalGetTaskFilePath.call(s, taskId), + ) + try { + const probed = await internals.getChildFileMtimeMs("null-reject-child") + // Correct code resolves with a future (live) mtime rather than throwing. + expect(probed).toBeDefined() + if (typeof probed === "number") { + expect(Date.now() - probed).toBeLessThan(0) + } + } finally { + pathSpy.mockRestore() + } + + const stringSpy = vi + .spyOn(probe, "getTaskFilePath") + .mockImplementation((taskId: string) => + taskId === "string-reject-child" ? Promise.reject("boom") : originalGetTaskFilePath.call(s, taskId), + ) + try { + const probed = await internals.getChildFileMtimeMs("string-reject-child") + expect(probed).toBeDefined() + if (typeof probed === "number") { + expect(Date.now() - probed).toBeLessThan(0) + } + } finally { + stringSpy.mockRestore() + } + }) + + it("ENOENT under an exactly-LOCK_STALE_MS-old or STALER advisory lock means the file is genuinely absent (kills L1285 Conditional->true, L1285 EqualityOperator '<'->'<=')", async () => { + // `Date.now() - lockStat.mtimeMs < LOCK_STALE_MS` — the fresh-lock (rename-window) + // guard in the ENOENT branch. Two mutants: + // `true` : every lock is fresh, so a stale leftover lock would report the child LIVE; + // `<=` : a lock EXACTLY LOCK_STALE_MS old still counts as fresh. + // A fixed clock plus a `.lock` DIRECTORY stamped via fs.utimes pins both boundaries + // exactly, independent of filesystem mtime precision (same FIXED_NOW pattern as the + // neighboring liveness tests; the fresh-lock rename-window test covers the other side). + const FIXED_NOW = 1_756_886_400_000 // 2025-09-03T08:00:00.000Z + const nowSpy = vi.spyOn(Date, "now").mockReturnValue(FIXED_NOW) + try { + const s = (store = new TaskHistoryStore(tmpDir)) + const internals = s as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const child = makeItem({ id: "lock-stale-child", status: "active" }) + await seedItems(tmpDir, [child]) + const historyPath = path.join(tmpDir, "tasks", child.id, GlobalFileNames.historyItem) + const lockPath = `${historyPath}.lock` + + // Simulate the rename window: history file gone, advisory lock directory held. + await fs.rm(historyPath) + await fs.mkdir(lockPath) + const stampLock = async (ageMs: number) => { + const stamp = new Date(FIXED_NOW - ageMs) + await fs.utimes(lockPath, stamp, stamp) + } + + // Sanity: a young lock → the child is live (future mtime), as the existing + // rename-window test asserts more fully. + await stampLock(1_000) + const fresh = await internals.getChildFileMtimeMs(child.id) + expect(fresh).toBeDefined() + if (typeof fresh === "number") { + expect(Date.now() - fresh).toBeLessThan(0) + } + + // EXACTLY LOCK_STALE_MS old: not fresh under strict '<' → genuinely absent. + // Under the `<=` mutant the probe returns a future mtime (live) instead of undefined. + await stampLock(LOCK_STALE_MS) + expect(await internals.getChildFileMtimeMs(child.id)).toBeUndefined() + + // Past the stale threshold: a leftover lock is not evidence of life. + // Under the `true` mutant the probe returns a future mtime instead of undefined. + await stampLock(LOCK_STALE_MS + 5_000) + expect(await internals.getChildFileMtimeMs(child.id)).toBeUndefined() + } finally { + nowSpy.mockRestore() + } + }) + it("replay liveness guard treats child file age exactly at threshold as NOT live and repairs (kills L627 EqualityOperator '<'->'<=')", async () => { // replayDelegationRepairIntent: `Date.now() - mtimeMs < LIVE_CHILD_MTIME_THRESHOLD_MS`. // Under `<=`, age === threshold counts as live and the stale intent is quarantined. With the diff --git a/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts index 3f44931182..b93c0627c0 100644 --- a/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts @@ -24,6 +24,13 @@ type PrivateClineProviderMethods = { historyItem: HistoryItemLike, options?: { startTask?: boolean }, ) => ReturnType + createTask: ( + this: unknown, + text?: string, + images?: string[], + parentTask?: Task, + options?: { startTask?: boolean }, + ) => Promise } const privateClineProvider = ClineProvider.prototype as unknown as PrivateClineProviderMethods @@ -38,8 +45,9 @@ vi.mock("../../task/Task", () => { public abort = false public abandoned = false public abortTask = vi.fn().mockResolvedValue(undefined) - constructor(opts: { historyItem?: { id: string }; onCreated?: (t: TaskStub) => void }) { + constructor(opts: { historyItem?: { id: string }; parentTask?: unknown; onCreated?: (t: TaskStub) => void }) { this.taskId = opts.historyItem?.id ?? `task-${Math.random().toString(36).slice(2, 8)}` + this.parentTask = opts.parentTask opts.onCreated?.(this) } run() { @@ -54,6 +62,12 @@ vi.mock("../../task/Task", () => { type MockFn = ReturnType +// Narrow a `vi.fn()` dual (call-new) mock to its callable procedure shape for +// assertion sites; avoids `any` while keeping the mock identity intact. +function asCallable(fn: T): T & ((...args: never[]) => unknown) { + return fn as T & ((...args: never[]) => unknown) +} + type OwnershipStore = { get: (id: string) => unknown markLocallyActive: MockFn @@ -63,6 +77,8 @@ type OwnershipStore = { type ProviderStubObject = { historyTaskCreationQueue: Promise getCurrentTask: MockFn + /** Satisfies the ClineProvider structural interface for `createTask` without being invoked by it. */ + setValues?: MockFn taskRegistry: TaskRegistry taskHistoryStore: OwnershipStore evictCurrentTask: MockFn @@ -94,13 +110,14 @@ function makeProvider(store: OwnershipStore, overrides: Partial registry.current), + getCurrentTask: vi.fn((...args: unknown[]) => (registry.current as undefined | Task) && registry.current), taskRegistry: registry, taskHistoryStore: store, evictCurrentTask: vi.fn().mockResolvedValue(undefined), removeClineFromStack: vi.fn().mockResolvedValue(undefined), addClineToStack: vi.fn().mockResolvedValue(undefined), performPreparationTasks: vi.fn().mockResolvedValue(undefined), + setValues: vi.fn().mockResolvedValue(undefined), taskScheduler: { schedule: vi.fn().mockResolvedValue(undefined) }, taskEventListeners: new Map(), log: vi.fn(), @@ -116,6 +133,7 @@ function makeProvider(store: OwnershipStore, overrides: Partial = {}): HistoryItemLike { return { id, number: 1, @@ -142,11 +160,17 @@ function makeHistoryItem(id: string): HistoryItemLike { tokensOut: 0, totalCost: 0, workspace: "/tmp", + ...extra, } } /** Seed a registry with a current task whose id matches `historyId` (rehydrate case). */ -function makeRehydrateProvider(store: OwnershipStore, historyId: string, overrides: Partial = {}) { +function makeRehydrateProvider( + store: OwnershipStore, + historyId: string, + overrides: Partial = {}, + { seedListeners = true }: { seedListeners?: boolean } = {}, +) { const existing = { taskId: historyId, instanceId: "old-inst", @@ -157,7 +181,19 @@ function makeRehydrateProvider(store: OwnershipStore, historyId: string, overrid } const registry = new TaskRegistry() registry.push(existing as unknown as Task) - return makeProvider(store, { getCurrentTask: vi.fn(() => existing), taskRegistry: registry, ...overrides }) + const provider = makeProvider(store, { + getCurrentTask: vi.fn(() => existing), + taskRegistry: registry, + ...overrides, + }) + // Seed the listener map exactly like production holds it for the current task so the + // rehydrate cleanup contract (run every cleanup, then delete the map entry) is + // assertable; tests that never observe the map are unaffected. Pass seedListeners: + // false to exercise the no-entry side of the `if (cleanupFunctions)` guard. + if (seedListeners) { + provider.taskEventListeners.set(existing, [vi.fn(), vi.fn()]) + } + return provider } async function flushMicrotasks(): Promise { @@ -262,6 +298,15 @@ describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () expect(store.markLocallyActive).toHaveBeenCalledWith("hist-sched-fail") await vi.waitFor(() => expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-sched-fail")) + + // scheduleTask's failure path logs through console.error with the exact source tag + // of THIS call site ("createTaskWithHistoryItem", stack branch). + await vi.waitFor(() => + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[createTaskWithHistoryItem] taskScheduler.schedule failed:", + expect.objectContaining({ message: "permit failed" }), + ), + ) }) it("releases the claim when the scheduler rejects the run (rehydrate path)", async () => { @@ -273,6 +318,14 @@ describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () await privateClineProvider.createTaskWithHistoryItem.call(provider, makeHistoryItem("hist-sched-fail-re")) await vi.waitFor(() => expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-sched-fail-re")) + + // Same console.error source-tag contract on the rehydrate-branch scheduleTask call. + await vi.waitFor(() => + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[createTaskWithHistoryItem] taskScheduler.schedule failed:", + expect.objectContaining({ message: "permit failed" }), + ), + ) }) it("keeps the claim when startTask is false: the installed task starts via a later explicit path, not the scheduler", async () => { @@ -301,4 +354,201 @@ describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () expect(store.markLocallyActive).toHaveBeenCalledWith("hist-nostart") expect(store.markLocallyInactive).not.toHaveBeenCalled() }) + + it("rehydrate path aborts the old task with abandon=true, runs its listener cleanups, removes the map entry, and replaces it in-place", async () => { + // Locks the rehydrate branch's oldTask handling: + // - abortTask(true): the boolean arg must be exactly true (abandon semantics); + // - every cleanup function for the old task runs and the taskEventListeners map + // entry is deleted afterwards; + // - the registry's current entry is the NEW task, not the old one (in-place replace); + // - the exact "rehydrated task ... in-place (flicker-free)" log line is emitted. + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-reh-full") + const existing = asCallable(provider.getCurrentTask)() as { abortTask: MockFn; taskId: string } + const cleanups = provider.taskEventListeners.get(existing)! + expect(cleanups).toHaveLength(2) + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-reh-full"), + ) + + // Abort with abandon=true — a `false` arg would lie about the abandon semantics. + expect(existing.abortTask).toHaveBeenCalledTimes(1) + expect(existing.abortTask).toHaveBeenCalledWith(true) + + // Listener contract: every cleanup ran, then the map entry was removed. + for (const cleanup of cleanups) { + expect(cleanup).toHaveBeenCalledTimes(1) + } + expect(provider.taskEventListeners.has(existing)).toBe(false) + + // In-place replace: current is the new task instance, not the old one. + expect(provider.taskRegistry.current).toBe(task) + expect(provider.taskRegistry.current).not.toBe(existing) + + // Exact success-log line on the rehydrate path (task id + instance id). + expect(provider.log).toHaveBeenCalledWith( + "[createTaskWithHistoryItem] rehydrated task hist-reh-full.stub-inst in-place (flicker-free)", + ) + }) + + it("rehydrate teardown tolerates a getCurrentTask/registry mismatch: no registry entry means no old-task abort", async () => { + // isRehydratingCurrentTask is decided from getCurrentTask(), but the replace branch + // reads this.taskRegistry.current. These must stay two separate reads: when the + // provider reports a matching current task while the registry no longer holds it + // (concurrent eviction), the `if (oldTask)` guard must skip the old-task teardown + // instead of throwing on a missing task. A mutant forcing the guard to `true` + // dereferences undefined and rejects the whole creation. + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-reh-mismatch", { + taskRegistry: new TaskRegistry(), + }) + const existing = asCallable(provider.getCurrentTask)() as { abortTask: MockFn; taskId: string } + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-reh-mismatch"), + ) + + expect(task.taskId).toBe("hist-reh-mismatch") + // No old task in the registry → no abort, no rethrow; the run is scheduled normally. + expect(existing.abortTask).not.toHaveBeenCalled() + expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + await flushMicrotasks() + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-reh-mismatch") + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) + + it("rehydrate path logs and continues when old-task abortTask itself rejects", async () => { + // The inner try/catch around `await oldTask.abortTask(true)` must swallow the + // rejection, log the exact diagnostic (old task id.instance plus the cause + // message), and let creation proceed — the claim stays because creation succeeded. + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-reh-throw") + const existing = asCallable(provider.getCurrentTask)() as { abortTask: MockFn } + existing.abortTask = vi.fn().mockRejectedValue(new Error("abort blew up")) + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-reh-throw"), + ) + + expect(task.taskId).toBe("hist-reh-throw") + expect(provider.log).toHaveBeenCalledWith( + "[createTaskWithHistoryItem] abortTask() failed for old task hist-reh-throw.old-inst: abort blew up", + ) + // The failure is contained: creation continues to a scheduled run. + expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + await flushMicrotasks() + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) + + it("rehydrate path with startTask:false skips the scheduler and retains the eager claim", async () => { + // The rehydrate-branch schedule guard `options?.startTask !== false` must honor an + // explicit startTask:false by NOT scheduling, while the claim stays (the only + // production caller re-registers ownership through its own active-status write + // before resuming). Forcing the guard true — or flipping the `false` literal — + // schedules anyway and breaks the contract. + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-reh-nostart") + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-reh-nostart"), + { startTask: false }, + ) + + expect(task.taskId).toBe("hist-reh-nostart") + expect(provider.taskScheduler.schedule).not.toHaveBeenCalled() + await flushMicrotasks() + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-reh-nostart") + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) + + it("stack path logs the exact instantiation message, distinguishing child tasks from parent tasks", async () => { + // The stack-branch success log is parameterized by `task.parentTask ? "child" : + // "parent"`; pin the CHILD variant (including task id and instance id) so the + // template literal cannot be blanked or its textual content swapped. + const store = makeStore() + const provider = makeProvider(store) + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-child-msg", { parentTask: {} as Task }), + ) + + expect(task.taskId).toBe("hist-child-msg") + expect(task.parentTask).toBeDefined() + expect(provider.log).toHaveBeenCalledWith( + "[createTaskWithHistoryItem] child task hist-child-msg.stub-inst instantiated", + ) + }) + + it("rehydrate teardown skips cleanup when the old task has NO listener map entry", async () => { + // The `if (cleanupFunctions)` guard: an old task without registered listeners must + // not run (or crash on) any cleanup. Forcing the guard true dereferences undefined + // (`cleanupFunctions.forEach` on undefined) and rejects the whole creation. + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-reh-nolisteners", {}, { seedListeners: false }) + const existing = asCallable(provider.getCurrentTask)() as { abortTask: MockFn } + expect(provider.taskEventListeners.size).toBe(0) + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-reh-nolisteners"), + ) + + expect(task.taskId).toBe("hist-reh-nolisteners") + expect(existing.abortTask).toHaveBeenCalledTimes(1) + // No cleanup entry → the code runs clean through the replace and schedules. + expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + // And no map entry was created for the NEW task either (no listeners registered). + expect(provider.taskEventListeners.size).toBe(0) + }) + + it("stack path logs the PARENT-variant instantiation message for tasks without a parent", async () => { + // The stack-branch success log's ternary `task.parentTask ? "child" : "parent"` — + // pin the PARENT variant (including task id and instance id) so the "parent" + // string literal cannot be blanked or swapped; the "child" side is pinned by the + // sibling test above. + const store = makeStore() + const provider = makeProvider(store) + + const task = await privateClineProvider.createTaskWithHistoryItem.call( + provider, + makeHistoryItem("hist-parent-msg"), + ) + + expect(task.taskId).toBe("hist-parent-msg") + expect(task.parentTask).toBeUndefined() + expect(provider.log).toHaveBeenCalledWith( + "[createTaskWithHistoryItem] parent task hist-parent-msg.stub-inst instantiated", + ) + }) + + it("hookless createTask call site survives a scheduler rejection by logging the createTask-tagged error, without a failure hook", async () => { + // scheduleTask's optional onScheduleFailure hook is undefined at this call site + // (createTask performs no claim that needs rolling back). The catch must invoke an + // absent hook exactly zero times — an unconditional `onScheduleFailure(error)` + // throws a TypeError as an unhandled rejection — and must log the rejection with + // THIS call site's source tag ("createTask"). + const store = makeStore() + const provider = makeProvider(store, { + taskScheduler: { schedule: vi.fn().mockRejectedValue(new Error("permit failed")) }, + }) + + const task = await privateClineProvider.createTask.call(provider, "hello") + + expect(task).toBeDefined() + expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + + await flushMicrotasks() + // Logged exactly once, with the exact tagged message and the original error. + expect(consoleErrorSpy).toHaveBeenCalledTimes(1) + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[createTask] taskScheduler.schedule failed:", + expect.objectContaining({ message: "permit failed" }), + ) + }) }) From 11a2ea290bb9076ae0e8f8a4ec1dbfdbdf0aa7c9 Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Tue, 8 Sep 2026 18:58:41 +0900 Subject: [PATCH 18/19] fix(delegation): close ownership race in reconciliation; CodeRabbit round-4 review fixes --- docs/architecture/task-lifecycle-model.md | 20 +- src/core/task-persistence/TaskHistoryStore.ts | 14 ++ .../TaskHistoryStore.reconciliation.spec.ts | 227 ++++++++++++++---- .../ClineProvider.markLocallyActive.spec.ts | 51 ++++ 4 files changed, 250 insertions(+), 62 deletions(-) diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index c3473fb14a..62a4acc958 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -108,7 +108,7 @@ The task delegation checker currently enforces: 5. Parent-child lineage is acyclic. 6. Completed task records cannot be changed by later lifecycle events. 7. Active-child re-delegation, stale completion after ownership moves to another child, duplicate/late completion, and abandonment of a live child are rejected by the shared production guards. -8. No transition may clear a delegated parent's link to a child that is active and marked live-elsewhere; startup reconciliation repairs only stale-or-unreadable-mtime (crash-orphan) children. This encodes the PR #1495 cross-window misrepair bug class, which broke delegation links so subtask completion could not return to the parent. +8. No transition may clear a delegated parent's link to a child that is active and marked live-elsewhere; startup reconciliation repairs only stale-mtime or genuinely missing (crash-orphan) children, while transient stat failures are treated as live and retried later. This encodes the PR #1495 cross-window misrepair bug class, which broke delegation links so subtask completion could not return to the parent. The completion persistence checker additionally enforces: @@ -124,16 +124,16 @@ These are safety claims within the documented bounds. The checks do not claim li The following map separates issue observations from the architectural interpretation encoded here. Open issues can change after this document is written; follow each link for current status. -| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | -| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the parent still awaiting that exact child; a live-linked child must remain owned by its parent. | `completeDelegatedChild` rejects stale authoritative input. The lifecycle explorer checks that reducer rule, while the shared-store explorer reproduces the cross-host stale-cache counterexample with an exact causal witness. | -| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | -| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it. | Completion implies restart-visible assistant history. Delayed or failed writes keep completion pending, and cancellation settles readiness without starting stale retries or emitting completion. | The completion persistence explorer checks the bounded event-ordering and cancellation contract for standalone and delegated tasks. Focused `Task` and `AttemptCompletionTool` tests cover the production adapter; `restart-persistence.test.ts` verifies visibility through a fresh extension host. | -| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | -| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | -| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | +| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | +| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the parent still awaiting that exact child; a live-linked child must remain owned by its parent. | `completeDelegatedChild` rejects stale authoritative input. The lifecycle explorer checks that reducer rule, while the shared-store explorer reproduces the cross-host stale-cache counterexample with an exact causal witness. | +| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | +| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it. | Completion implies restart-visible assistant history. Delayed or failed writes keep completion pending, and cancellation settles readiness without starting stale retries or emitting completion. | The completion persistence explorer checks the bounded event-ordering and cancellation contract for standalone and delegated tasks. Focused `Task` and `AttemptCompletionTool` tests cover the production adapter; `restart-persistence.test.ts` verifies visibility through a fresh extension host. | +| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | +| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | +| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | | [#1468](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1468): a late chunk from one request combined tool identity with arguments from another request; rerun passed. | Every stream accumulator needs a request/task generation key, and late events cannot mutate another scope. | Separate protocol. The [native tool-call parser request-scope model](./native-tool-call-parser-scoping-model.md), whose source of truth is `scripts/check-native-tool-call-parser-scoping.ts`, exhaustively replays bounded production-parser interleavings without adding fields to this lifecycle model. | -| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | +| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | The issue-derived cases intentionally map to bug classes rather than issue-specific flags. In particular, stale event ownership, monotonic terminal/detached state, explicit scope, and single-writer boundaries generalize to future concurrent task work. diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index aa9f2b9242..40184da8fa 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -511,6 +511,20 @@ export class TaskHistoryStore { ) continue } + // Re-check local ownership after the async stat await: the + // persistedActiveIds snapshot was captured before this point, and + // ClineProvider can claim the child for a live session in THIS + // window (markLocallyActive, the eager claim in + // createTaskWithHistoryItemUnlocked) while getChildFileMtimeMs was + // in flight. The snapshot no longer reflects that claim, so the + // child is no longer a crash orphan — skip the repair. + if (this.locallyActiveTaskIds.has(child.id)) { + console.warn( + `[TaskHistoryStore] Skipping repair for live child ${child.id} ` + + `(claimed by this window during reconciliation)`, + ) + continue + } // An active child persisted across startup cannot have a live task session // behind it. Mark it interrupted before releasing the parent's delegation // link so the normal resume/re-delegate flow can take over. This is an diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index 22ea663cfa..66d622bce1 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -2114,39 +2114,43 @@ describe("TaskHistoryStore periodic delegation reconciliation", () => { .mockRejectedValue(new Error("tick delegation boom")) const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - const s = (store = new TaskHistoryStore(tmpDir)) - useTickClock() - await s.initialize() - - await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - // The error log happens in the tick callback's catch AFTER the throwing - // delegation step settles, so the spy firing means the tick is done. - await flushUntil( - () => - errorSpy.mock.calls.some( - (c) => typeof c[0] === "string" && c[0].includes("Periodic delegation reconciliation failed"), - ), - { - label: "tick logged the delegation failure", - snapshot: () => `errorCalls=${errorSpy.mock.calls.length}`, - }, - ) - expect(errorSpy).toHaveBeenCalledWith( - expect.stringContaining("Periodic delegation reconciliation failed"), - expect.objectContaining({ message: "tick delegation boom" }), - ) + try { + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + await s.initialize() - // One more interval still fires the delegation step: the recursive - // re-arm is preserved even though the step threw. - await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushUntil(() => throwingSpy.mock.calls.length >= 2, { - label: "second tick invoked the throwing delegation step", - snapshot: () => `throwingSpyCalls=${throwingSpy.mock.calls.length}`, - }) - expect(throwingSpy).toHaveBeenCalledTimes(2) + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + // The error log happens in the tick callback's catch AFTER the throwing + // delegation step settles, so the spy firing means the tick is done. + await flushUntil( + () => + errorSpy.mock.calls.some( + (c) => typeof c[0] === "string" && c[0].includes("Periodic delegation reconciliation failed"), + ), + { + label: "tick logged the delegation failure", + snapshot: () => `errorCalls=${errorSpy.mock.calls.length}`, + }, + ) + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining("Periodic delegation reconciliation failed"), + expect.objectContaining({ message: "tick delegation boom" }), + ) - errorSpy.mockRestore() - throwingSpy.mockRestore() + // One more interval still fires the delegation step: the recursive + // re-arm is preserved even though the step threw. + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushUntil(() => throwingSpy.mock.calls.length >= 2, { + label: "second tick invoked the throwing delegation step", + snapshot: () => `throwingSpyCalls=${throwingSpy.mock.calls.length}`, + }) + expect(throwingSpy).toHaveBeenCalledTimes(2) + } finally { + // Spy restoration must survive assertion failures mid-test — a leaked + // prototype spy would poison every subsequent test in this spec. + errorSpy.mockRestore() + throwingSpy.mockRestore() + } }) }) @@ -2485,6 +2489,22 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { } }) + it("markLocallyInactive removes an eagerly claimed id from locallyActiveTaskIds (direct ownership membership)", async () => { + // Direct membership companion to the L592 kill test above, mirroring the + // delete()/deleteMany() ownership tests: claim the id via the public + // markLocallyActive path, then release it and assert the ownership set + // dropped it. No tick or seeded delegation pair needed — this isolates + // the claim/release bookkeeping contract from the repair pipeline. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + + s.markLocallyActive("mli-direct") + expect(ownedIds(s).has("mli-direct")).toBe(true) + + s.markLocallyInactive("mli-direct") + expect(ownedIds(s).has("mli-direct")).toBe(false) + }) + it("a code-less stat rejection (null/string) is classified live, not absent, and never throws (kills L1276 OptionalChaining)", async () => { // getChildFileMtimeMs: `if ((error as NodeJS.ErrnoException)?.code === "ENOENT")`. The // optional chain is a real guard: dropping it (`(error).code`) dereferences null when @@ -2804,32 +2824,135 @@ describe("TaskHistoryStore mutation-gate kill tests", () => { // must survive the tick untouched. If markLocallyActive's add() were dropped, the id // would not be excluded, the guard would see the stale mtime, and the child would be // repaired to interrupted — failing the status assertions below. - const childId = "child-eager-claim" - const parentId = "parent-eager-claim" - const [parent, child] = delegatedPair(parentId, childId) - await seedItems(tmpDir, [parent, child]) + // + // The negative log assertion pins WHERE the exclusion happens: ownership claimed + // BEFORE the snapshot must be excluded at snapshot time, so the post-await + // re-check's skip log ("claimed by this window during reconciliation") must NOT + // fire for it. A mutant that removes the snapshot `.filter(...)` (Stryker + // MethodExpression) lets the owned child into the snapshot; the post-await + // re-check would then spare it but EMIT that log, failing the assertion below — + // proving the snapshot filter is not redundant with the re-check. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + try { + const childId = "child-eager-claim" + const parentId = "parent-eager-claim" + const [parent, child] = delegatedPair(parentId, childId) + await seedItems(tmpDir, [parent, child]) - const s = (store = new TaskHistoryStore(tmpDir)) - useTickClock() - await s.initialize() + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + await s.initialize() - s.markLocallyActive(childId) - expect(ownedIds(s).has(childId)).toBe(true) + s.markLocallyActive(childId) + expect(ownedIds(s).has(childId)).toBe(true) + // The startup pass logged a live-elsewhere skip for the fresh-seeded child; + // clear so only the tick's logs remain observable below. + warnSpy.mockClear() - // The child now LOOKS like a crash orphan, but local ownership excludes it - // from this tick's persisted-active snapshot. - installStaleChildInjector(childId) + // The child now LOOKS like a crash orphan, but local ownership excludes it + // from this tick's persisted-active snapshot. + installStaleChildInjector(childId) - const timerState = s as unknown as { reconcileTimer: ReturnType | null } - const timerBeforeTick = timerState.reconcileTimer - await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) - await flushUntil(() => timerState.reconcileTimer !== timerBeforeTick, { - label: "tick completed without repairing the eagerly-claimed child", - snapshot: () => `child=${s.get(childId)?.status} parent=${s.get(parentId)?.status}`, - }) + const timerState = s as unknown as { reconcileTimer: ReturnType | null } + const timerBeforeTick = timerState.reconcileTimer + await vi.advanceTimersByTimeAsync(RECONCILE_INTERVAL_MS) + await flushUntil(() => timerState.reconcileTimer !== timerBeforeTick, { + label: "tick completed without repairing the eagerly-claimed child", + snapshot: () => `child=${s.get(childId)?.status} parent=${s.get(parentId)?.status}`, + }) - expect(s.get(childId)?.status).toBe("active") - expect(s.get(parentId)?.status).toBe("delegated") + expect(s.get(childId)?.status).toBe("active") + expect(s.get(parentId)?.status).toBe("delegated") + // Pre-snapshot ownership is handled by the snapshot filter alone: the + // post-await re-check must never see (or log) a child excluded up front. + expect(warnSpy).not.toHaveBeenCalledWith( + expect.stringContaining("claimed by this window during reconciliation"), + ) + } finally { + warnSpy.mockRestore() + } + }) + + it("skips repair when the child is claimed locally while the mtime stat is in flight (closes the snapshot race)", async () => { + // CodeRabbit follow-up Item 5: runPeriodicDelegationReconciliation snapshots + // persistedActiveIds (minus locally-owned ids) BEFORE reconcileDelegationState + // awaits getChildFileMtimeMs per candidate. ClineProvider's eager + // markLocallyActive claim (createTaskWithHistoryItemUnlocked) can land DURING + // that await, so the snapshot no longer reflects it and the tick would repair + // a child that JUST became locally owned in this window. The core must + // re-check locallyActiveTaskIds after the await, immediately before + // repairActiveDelegation, and skip when ownership was claimed mid-stat. + // + // Technique: spy getChildFileMtimeMs (the internals seam this spec already + // uses) and claim ownership for the child INSIDE the mock before resolving a + // stale mtime — exactly the moment between the snapshot and the repair where + // the eager claim can interleave. Under the pre-fix code the stale mtime + // proceeds straight to repairActiveDelegation (child -> interrupted, parent + // -> active), failing the status assertions below. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + try { + const childId = "child-claim-race" + const parentId = "parent-claim-race" + const [parent, child] = delegatedPair(parentId, childId) + await seedItems(tmpDir, [parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + // Fresh seed mtimes -> the startup pass treats the child as live and skips repair. + expect(s.get(childId)?.status).toBe("active") + expect(s.get(parentId)?.status).toBe("delegated") + // Not yet owned locally, so the child IS in the tick's persisted-active snapshot. + expect(ownedIds(s).has(childId)).toBe(false) + // The startup pass logged a live-elsewhere skip for the fresh-seeded + // child (same "Skipping repair for live child" prefix). Clear it so the + // log assertions below observe ONLY the tick's re-check skip — otherwise + // a mutated skip message could still "match" via the startup log. + warnSpy.mockClear() + + // Arm the seam: claim ownership for the child while the stat await is in + // flight, then report a stale mtime so the pre-fix code would repair it. + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (id: string) => Promise + } + const original = probe.getChildFileMtimeMs + mtimeSpy = vi.spyOn(probe, "getChildFileMtimeMs").mockImplementation((id: string) => { + if (id === childId) { + s.markLocallyActive(childId) + return Promise.resolve(Date.now() - 10 * 60 * 1000) + } + return original.call(s, id) + }) + + const internals = TaskHistoryStore.prototype as unknown as { + runPeriodicDelegationReconciliation: () => Promise + } + await internals.runPeriodicDelegationReconciliation.call(s) + + // The post-await re-check must see the mid-stat claim and skip the repair: + // child stays active, parent keeps its delegation links, skip is logged. + expect(ownedIds(s).has(childId)).toBe(true) + expect(s.get(childId)?.status).toBe("active") + expect(s.get(parentId)?.status).toBe("delegated") + expect(s.get(parentId)?.awaitingChildId).toBe(childId) + expect(s.get(parentId)?.delegatedToId).toBe(childId) + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining(`Skipping repair for live child ${childId}`)) + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("(claimed by this window during reconciliation)"), + ) + // Combined-phrase assertion: both template fragments concatenated. A + // StringLiteral->'' mutant on EITHER fragment breaks this exact substring, + // and the pre-tick mockClear guarantees no other warn call can supply it. + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining( + `Skipping repair for live child ${childId} (claimed by this window during reconciliation)`, + ), + ) + expect(errorSpy).not.toHaveBeenCalled() + } finally { + warnSpy.mockRestore() + errorSpy.mockRestore() + } }) it("transient stat failure at the liveness guard skips repair and logs 'Skipping repair for live child' (ENOENT-only classification)", async () => { diff --git a/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts index b93c0627c0..5fe13fd850 100644 --- a/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts @@ -234,6 +234,12 @@ describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () await flushMicrotasks() expect(store.markLocallyInactive).not.toHaveBeenCalled() expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + // CodeRabbit: ordering, not just invocation — the eager claim exists to cover + // the gap BEFORE Task.run()'s first active-status write, so the claim must + // precede the scheduler handoff on the recorded invocation order. + expect(store.markLocallyActive.mock.invocationCallOrder[0]).toBeLessThan( + provider.taskScheduler.schedule.mock.invocationCallOrder[0], + ) }) it("claims ownership on the in-place rehydrate success path without releasing it", async () => { @@ -250,6 +256,10 @@ describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () await flushMicrotasks() expect(store.markLocallyInactive).not.toHaveBeenCalled() expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + // Same claim-before-schedule ordering contract on the rehydrate branch. + expect(store.markLocallyActive.mock.invocationCallOrder[0]).toBeLessThan( + provider.taskScheduler.schedule.mock.invocationCallOrder[0], + ) }) it("releases the claim when preparation fails on the rehydrate path and rethrows", async () => { @@ -328,6 +338,47 @@ describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () ) }) + it("failure paths claim the id exactly once and release it exactly once (no double-release)", async () => { + // CodeRabbit: with BOTH performPreparationTasks and taskScheduler.schedule + // configured to reject, the id must still be claimed exactly once and released + // exactly once. The two failure sources are mutually exclusive at runtime by + // control flow — a prep failure throws before scheduleTask is ever reached — + // so the schedule rejection cannot stack a second release on top of the + // catch-path release (asserted by schedule's zero calls below). + { + const store = makeStore() + const provider = makeRehydrateProvider(store, "hist-once-prep", { + performPreparationTasks: vi.fn().mockRejectedValue(new Error("prep exploded")), + taskScheduler: { schedule: vi.fn().mockRejectedValue(new Error("permit failed")) }, + }) + + await expect( + privateClineProvider.createTaskWithHistoryItem.call(provider, makeHistoryItem("hist-once-prep")), + ).rejects.toThrow("prep exploded") + + expect(store.markLocallyActive).toHaveBeenCalledTimes(1) + expect(store.markLocallyActive).toHaveBeenCalledWith("hist-once-prep") + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-once-prep") + expect(provider.taskScheduler.schedule).not.toHaveBeenCalled() + } + + // Scheduler-rejection path: prep succeeds, the release arrives solely through + // scheduleTask's onScheduleFailure hook — one claim, one release. + { + const store = makeStore() + const provider = makeProvider(store, { + taskScheduler: { schedule: vi.fn().mockRejectedValue(new Error("permit failed")) }, + }) + + await privateClineProvider.createTaskWithHistoryItem.call(provider, makeHistoryItem("hist-once-sched")) + + await vi.waitFor(() => expect(store.markLocallyInactive).toHaveBeenCalledWith("hist-once-sched")) + expect(store.markLocallyActive).toHaveBeenCalledTimes(1) + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + } + }) + it("keeps the claim when startTask is false: the installed task starts via a later explicit path, not the scheduler", async () => { // The only production caller that passes startTask:false is // reopenParentFromDelegation (ClineProvider.ts step 7): the parent's From a4b3786f7e6527c1467d8e6a1ad07a6c2b7f718e Mon Sep 17 00:00:00 2001 From: "Zoo (VP)" Date: Tue, 8 Sep 2026 19:29:04 +0900 Subject: [PATCH 19/19] test(delegation): strengthen resolved-task assertion in hookless createTask test --- .../ClineProvider.markLocallyActive.spec.ts | 33 +++++++++++++++---- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts index 5fe13fd850..15e9abf1c8 100644 --- a/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts @@ -35,9 +35,15 @@ type PrivateClineProviderMethods = { const privateClineProvider = ClineProvider.prototype as unknown as PrivateClineProviderMethods -vi.mock("../../task/Task", () => { - // The id must come from the history item so claim/release target the exact - // created task; the stub only implements the surface the provider touches. +// Declared via `vi.hoisted` (not a plain module-scope class) so the class +// binding is initialized BEFORE the hoisted vi.mock factory runs — the mock +// factory executes while ClineProvider's imports resolve, so a normally +// declared class would still be in its temporal dead zone there. This also +// exposes the stub to tests for `toBeInstanceOf(TaskStub)` assertions. +const TaskStub = vi.hoisted(() => { + // `vi` is not yet initialized at hoist time, so field initializer surfaces + // that use vi.fn() are deferred to construction time (class fields run per + // instance, well after vitest initializes the mock registry). class TaskStub { public taskId: string public instanceId = "stub-inst" @@ -46,7 +52,10 @@ vi.mock("../../task/Task", () => { public abandoned = false public abortTask = vi.fn().mockResolvedValue(undefined) constructor(opts: { historyItem?: { id: string }; parentTask?: unknown; onCreated?: (t: TaskStub) => void }) { - this.taskId = opts.historyItem?.id ?? `task-${Math.random().toString(36).slice(2, 8)}` + // The id must come from the history item so claim/release target the exact + // created task; hookless createTask (no historyItem) gets a deterministic + // sequential default id, so tests can assert the exact generated value. + this.taskId = opts.historyItem?.id ?? `task-stub-${++TaskStub.instanceCount}` this.parentTask = opts.parentTask opts.onCreated?.(this) } @@ -56,10 +65,15 @@ vi.mock("../../task/Task", () => { on() {} off() {} emit() {} + public static instanceCount = 0 } - return { Task: TaskStub } + return TaskStub }) +vi.mock("../../task/Task", () => ({ + Task: TaskStub, +})) + type MockFn = ReturnType // Narrow a `vi.fn()` dual (call-new) mock to its callable procedure shape for @@ -591,7 +605,14 @@ describe("ClineProvider createTaskWithHistoryItem ownership claim/rollback", () const task = await privateClineProvider.createTask.call(provider, "hello") - expect(task).toBeDefined() + // CodeRabbit round-5: assert the resolution value itself, not just + // "defined" — the resolved task must be the mocked TaskStub instance, + // and its generated identifier (no historyItem on this path) must be + // the stub's most recent sequential default id (deterministic prefix, + // preferred over a /^task-/ format check because it pins the exact + // id-generation behavior of the stub's constructor fallback). + expect(task).toBeInstanceOf(TaskStub) + expect(task.taskId).toBe(`task-stub-${TaskStub.instanceCount}`) expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) await flushMicrotasks()