diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 7b2eabdd63..6d420c8cbb 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -41,18 +41,21 @@ 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` | -| Pending-action settlement | `TaskHistoryStore.clearPendingActionIfMatching` compare-and-clear in the rejected-delegation settlement path (#1714) | -| 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` | +| Pending-action settlement | `TaskHistoryStore.clearPendingActionIfMatching` compare-and-clear in the rejected-delegation settlement path (#1714) | +| `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) | +| `heartbeat(child)` / `expireHeartbeat(child)` | persisted `lastActivityAt` liveness heartbeat fresh vs stale past `LIVE_CHILD_MTIME_THRESHOLD_MS` (production: the owning session's throttled heartbeat across the child's whole active lifetime — long streaming turns, long tool calls, and ask-idle periods alike — shared predicate `isDelegatedChildLive`) | +| 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 two abstract booleans per slot recording whether an active child's session shows life: one for ownership by another window (recent history-file mtime) and one for a fresh persisted `lastActivityAt` heartbeat from the owning session. 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, a delegated parent whose active child heartbeats through a long streaming turn (stale mtime, fresh `lastActivityAt`) likewise surviving unchanged, and a stale-mtime, stale-heartbeat (crash-orphan) active child being repaired to `interrupted` with the parent returned to `active` only through `reconcileStartup`. Each task slot can also hold one of two pending `create_subtask` actions. A `stage` action mirrors `setPendingTaskAction` overwrite semantics, delegation clears the action its request carried, completion clears the child's action only when its event carries the matching action ID, and a `settle-rejected` action models the settlement that follows an authoritative delegation rejection (#1714). Production settles through the typed `LifecycleTransitionError` from the shared guards: the provider calls the disk-authoritative `TaskHistoryStore.clearPendingActionIfMatching` compare-and-clear under the per-file lock, then propagates the original rejection. Six named witnesses must remain reachable: settlement from an interrupted record after rejection, settlement through a successful active delegation, unrelated-action preservation during completion, stale-action protection where a settlement targeting one action ID leaves a replacement action intact, matching-ID completion clearing, and replacement-ID completion preservation. A mismatched pending-action request keeps its production behavior: the atomic update throws before any transition, and no settlement runs. @@ -140,8 +143,9 @@ 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. A rejected delegation settles only the exact matching pending `create_subtask` action. Settlement preserves status, lineage, and accounting, never clears a replacement or different-kind action, and never mutates a completed record. The settlement compare-and-clear reads the persisted record under the per-file lock, so it never clears from a stale host cache. -9. A completion clears the completing child's pending action only when the completion event carries the exact matching action ID. A completion with no action ID or a different ID preserves the pending action. +8. No transition may clear a delegated parent's link to a child that is active and live, where live means either marked live-elsewhere (`ModelState.liveElsewhere` true, recent history-file mtime) or heartbeat-alive (`ModelState.heartbeatAlive` true, fresh persisted `lastActivityAt`); reconciliation repairs the link only when both flags are false. This encodes the PR #1495 cross-window misrepair bug class — which broke delegation links so subtask completion could not return to the parent — extended by the liveness-heartbeat fix so a child streaming a long turn (minutes without any other history-file write) is likewise preserved. +9. A rejected delegation settles only the exact matching pending `create_subtask` action. Settlement preserves status, lineage, and accounting, never clears a replacement or different-kind action, and never mutates a completed record. The settlement compare-and-clear reads the persisted record under the per-file lock, so it never clears from a stale host cache. +10. A completion clears the completing child's pending action only when the completion event carries the exact matching action ID. A completion with no action ID or a different ID preserves the pending action. The completion persistence checker additionally enforces: diff --git a/packages/types/src/history.ts b/packages/types/src/history.ts index 5d9671842e..4cbcfba010 100644 --- a/packages/types/src/history.ts +++ b/packages/types/src/history.ts @@ -48,6 +48,7 @@ export const historyItemSchema = z.object({ awaitingChildId: z.string().optional(), // Child currently awaited (set when delegated) completedByChildId: z.string().optional(), // Child that completed and resumed this parent completionResultSummary: z.string().optional(), // Summary from completed child + lastActivityAt: z.number().optional(), // Liveness heartbeat: last time the owning session persisted activity pendingAction: pendingTaskActionSchema.optional(), }) diff --git a/scripts/check-task-lifecycle.ts b/scripts/check-task-lifecycle.ts index bf05ad68e9..44cb18ea96 100644 --- a/scripts/check-task-lifecycle.ts +++ b/scripts/check-task-lifecycle.ts @@ -12,7 +12,35 @@ 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 signals. Production decides whether an + * active child awaited by a delegated parent belongs to another live window + * through two independent freshness checks against a 5-minute threshold + * (`TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS`, shared predicate + * `isDelegatedChildLive` in `src/core/task-persistence/taskLifecycle.ts`): + * + * - `liveElsewhere[id]` models the child's history-file mtime: true when the + * mtime is "recent" (the child is owned by another window that is actively + * persisting it), false when it is "stale" or the file is unreadable. + * - `heartbeatAlive[id]` models the persisted `lastActivityAt` heartbeat: true + * when the owning session heartbeated within the threshold (Task's throttled + * heartbeat runs across the child's whole active lifetime — long streaming + * turns, long tool calls, and ask-idle periods that write nothing else to + * the history file), false when the heartbeat is absent or stale. + * + * The model never reads wall-clock time. Reconciliation treats the child as + * live when EITHER signal is true and repairs it to interrupted only when + * BOTH are false (the crash-orphan case). + */ +type LivenessMap = Record + +interface ModelState { + tasks: TaskMap + liveElsewhere: LivenessMap + heartbeatAlive: LivenessMap +} interface Transition { name: string @@ -36,51 +64,110 @@ interface WitnessContext { const MAX_DEPTH = 12 const MAX_STATES = 10_000 const actionIds = ["action-1", "action-2"] as const -const expectedActions = ["delegate", "interrupt", "complete", "abandon", "stage", "settle-rejected"] as const +const expectedActions = [ + "delegate", + "interrupt", + "complete", + "abandon", + "stage", + "settle-rejected", + "markLiveElsewhere", + "expireLiveElsewhere", + "heartbeat", + "expireHeartbeat", + "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 heartbeat companion of the same guard: a child whose history + // file mtime is stale (no recent persistence write) but whose owning session + // heartbeats a fresh `lastActivityAt` — the long-streaming-turn case — must + // survive reconciliation with its delegation link intact. + "heartbeat-protected-child-preserved": (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] && + state.heartbeatAlive[childId] + ) + }, + // Proves the repair half of the same reconciliation outcome still works: a + // non-live (crash-orphan) active child — stale mtime AND stale/absent + // heartbeat — 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"] && + !state.heartbeatAlive["child-a"] + ) + }, } satisfies Record boolean> const semanticWitnesses = { "interrupted-pending-delegation-settled": ({ prev, next, transition }: WitnessContext) => transition.settlement !== undefined && - prev[transition.settlement.taskId]?.status === "interrupted" && - prev[transition.settlement.taskId]?.pendingAction?.actionId === transition.settlement.actionId && - next[transition.settlement.taskId]?.pendingAction === undefined, + prev.tasks[transition.settlement.taskId]?.status === "interrupted" && + prev.tasks[transition.settlement.taskId]?.pendingAction?.actionId === transition.settlement.actionId && + next.tasks[transition.settlement.taskId]?.pendingAction === undefined, "successful-active-delegation-settlement": ({ prev, next, transition }: WitnessContext) => transition.delegation !== undefined && - prev[transition.delegation.parentId]?.status === "active" && - prev[transition.delegation.parentId]?.pendingAction?.kind === "create_subtask" && - next[transition.delegation.parentId]?.pendingAction === undefined, + prev.tasks[transition.delegation.parentId]?.status === "active" && + prev.tasks[transition.delegation.parentId]?.pendingAction?.kind === "create_subtask" && + next.tasks[transition.delegation.parentId]?.pendingAction === undefined, "completion-preserves-unrelated-pending-action": ({ prev, next, transition }: WitnessContext) => { if (transition.completion === undefined) return false - const beforeAction = prev[transition.completion.childId]?.pendingAction + const beforeAction = prev.tasks[transition.completion.childId]?.pendingAction return ( beforeAction !== undefined && - next[transition.completion.childId]?.status === "completed" && - canonicalTask(next[transition.completion.childId]?.pendingAction) === canonicalTask(beforeAction) + next.tasks[transition.completion.childId]?.status === "completed" && + canonicalTask(next.tasks[transition.completion.childId]?.pendingAction) === canonicalTask(beforeAction) ) }, "stale-settlement-rejected": ({ prev, next, transition }: WitnessContext) => { if (transition.settlement === undefined) return false - const beforeAction = prev[transition.settlement.taskId]?.pendingAction + const beforeAction = prev.tasks[transition.settlement.taskId]?.pendingAction return ( beforeAction?.kind === "create_subtask" && beforeAction.actionId !== transition.settlement.actionId && - next[transition.settlement.taskId]?.pendingAction?.actionId === beforeAction.actionId + next.tasks[transition.settlement.taskId]?.pendingAction?.actionId === beforeAction.actionId ) }, "matching-completion-clears-pending-action": ({ prev, next, transition }: WitnessContext) => { if (transition.completion?.pendingActionId === undefined) return false - const beforeAction = prev[transition.completion.childId]?.pendingAction - const after = next[transition.completion.childId] + const beforeAction = prev.tasks[transition.completion.childId]?.pendingAction + const after = next.tasks[transition.completion.childId] return ( beforeAction?.kind === "create_subtask" && beforeAction.actionId === transition.completion.pendingActionId && @@ -90,8 +177,8 @@ const semanticWitnesses = { }, "replacement-completion-preserves-pending-action": ({ prev, next, transition }: WitnessContext) => { if (transition.completion?.pendingActionId === undefined) return false - const beforeAction = prev[transition.completion.childId]?.pendingAction - const after = next[transition.completion.childId] + const beforeAction = prev.tasks[transition.completion.childId]?.pendingAction + const after = next.tasks[transition.completion.childId] return ( beforeAction?.kind === "create_subtask" && beforeAction.actionId !== transition.completion.pendingActionId && @@ -129,27 +216,37 @@ function createSubtaskAction(actionId: (typeof actionIds)[number]): PendingTaskA } 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 }, + heartbeatAlive: { 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, heartbeatAlive: state.heartbeatAlive } } 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 - const awaitedStatus = parent.awaitingChildId ? state[parent.awaitingChildId as TaskId]?.status : undefined + // 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 + + const awaitedStatus = parent.awaitingChildId + ? state.tasks[parent.awaitingChildId as TaskId]?.status + : undefined const delegationValid = parent.status === "active" || (parent.status === "delegated" && awaitedStatus === "interrupted") for (const childId of taskIds) { - if (childId === parentId || state[childId]) continue + if (childId === parentId || state.tasks[childId]) continue if (!delegationValid) continue const delegated = { ...delegateTaskToChild(parent, childId, awaitedStatus), pendingAction: undefined } result.push({ @@ -179,11 +276,21 @@ 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. The same holds for a heartbeat-alive child: the owning session + // is mid-turn and its completion can only arrive after the heartbeat + // expires (production clears the heartbeat timer when the stream ends). + // `checkTransitionInvariants` re-proves universally that no reachable + // action clears the parent's link while the child is active and live + // (either signal). + if (state.liveElsewhere[childId] || state.heartbeatAlive[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) }) @@ -221,13 +328,103 @@ 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 by the OR of the two + // abstract liveness signals (mirroring the shared production predicate + // `isDelegatedChildLive`): + // - stale mtime AND stale/absent heartbeat (not live) → 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 OR fresh heartbeat (live) → skip: the pre-fix bug repaired + // exactly these children, 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] || state.heartbeatAlive[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 }, heartbeatAlive: state.heartbeatAlive }, + }) + } else { + result.push({ + name: `expireLiveElsewhere(${id2})`, + next: { tasks: state.tasks, liveElsewhere: { ...state.liveElsewhere, [id2]: false }, heartbeatAlive: state.heartbeatAlive }, + }) + } + } + + // Model actions for the abstract heartbeat flag: `heartbeat` represents the + // owning session persisting a fresh `lastActivityAt` while it streams a long + // turn (production: Task's throttled liveness heartbeat), and + // `expireHeartbeat` represents the heartbeat going stale — the session + // stopped heartbeating (turn finished, task disposed, or owner crashed) + // without another window taking over persistence. Same child-slot + // restriction as the mtime flag: only active delegated children carry a + // liveness dimension in this bug class. + for (const id of taskIds) { + const current = state.tasks[id] + if (current?.status !== "active" || !current.parentTaskId) continue + const id2 = id as TaskId + if (!state.heartbeatAlive[id2]) { + result.push({ + name: `heartbeat(${id2})`, + next: { tasks: state.tasks, liveElsewhere: state.liveElsewhere, heartbeatAlive: { ...state.heartbeatAlive, [id2]: true } }, + }) + } else { + result.push({ + name: `expireHeartbeat(${id2})`, + next: { tasks: state.tasks, liveElsewhere: state.liveElsewhere, heartbeatAlive: { ...state.heartbeatAlive, [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") { @@ -235,7 +432,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`) } @@ -247,7 +444,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`) } @@ -261,14 +458,18 @@ 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]), + taskIds.map((id) => state.heartbeatAlive[id]), + ]) } function formatCounterexample(message: string, trace: TraceStep[]): string { @@ -289,17 +490,37 @@ 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, extended by the + // liveness-heartbeat fix): no transition may clear a delegated parent's + // link to a child that is active AND live — where live means either a + // recent history-file mtime (`liveElsewhere`) or a fresh persisted + // `lastActivityAt` heartbeat (`heartbeatAlive`). Pre-fix, startup + // reconciliation repaired exactly these children; the shared production + // predicate `isDelegatedChildLive` 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 + // 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] || previous.heartbeatAlive[childId])) { + if (after?.status !== "delegated" || after.awaitingChildId !== childId) { + violations.push(`${id}: ${transition.name} cleared delegation to active live child ${childId}`) + } + } } } const settlement = transition.settlement if (settlement) { - const before = previous[settlement.taskId] - const after = transition.next[settlement.taskId] + const before = previous.tasks[settlement.taskId] + const after = transition.next.tasks[settlement.taskId] const beforeAction = before?.pendingAction const afterAction = after?.pendingAction @@ -324,8 +545,8 @@ function checkTransitionInvariants(previous: ModelState, transition: Transition) const completion = transition.completion if (completion) { - const beforeAction = previous[completion.childId]?.pendingAction - const afterAction = transition.next[completion.childId]?.pendingAction + const beforeAction = previous.tasks[completion.childId]?.pendingAction + const afterAction = transition.next.tasks[completion.childId]?.pendingAction if (afterAction && canonicalTask(afterAction) !== canonicalTask(beforeAction)) { violations.push(`${completion.childId}: completion after ${transition.name} replaced a pending action`) @@ -341,7 +562,14 @@ function canonicalTask(value: unknown): string { return JSON.stringify(value ?? null) } -function runModelCheck(): number { +interface ModelCheckResult { + states: number + reachedActions: ReadonlySet + reachedLandmarks: ReadonlySet + reachedWitnesses: ReadonlySet +} + +function runModelCheck(): ModelCheckResult { const start = initialState() const queue: Array<{ state: ModelState; trace: TraceStep[] }> = [ { state: start, trace: [{ action: "initial", state: start }] }, @@ -405,7 +633,7 @@ function runModelCheck(): number { `Task lifecycle exploration reached depth ${MAX_DEPTH} with an unseen successor (${unexploredSuccessor.name}); increase the depth bound`, ) } - return visited.size + return { states: visited.size, reachedActions, reachedLandmarks, reachedWitnesses } } function runRepresentativeScenarios(): void { @@ -432,6 +660,39 @@ function runRepresentativeScenarios(): void { assert.equal(interruptedCompletion.child.status, "completed") assert.equal(interruptedCompletion.parent.status, "active") + // Heartbeat-only liveness (stale mtime, fresh `lastActivityAt`) — the + // whole-lifetime heartbeat (long streaming turns, long tool calls, and + // ask-idle periods alike): production's shared `isDelegatedChildLive` + // predicate treats the child as live, so reconciliation must be an identity + // transition and the delegation link survives. + { + const state: ModelState = { + tasks: { parent: delegated, "child-a": childA, "child-b": undefined }, + liveElsewhere: { parent: false, "child-a": false, "child-b": false }, + heartbeatAlive: { parent: false, "child-a": true, "child-b": false }, + } + const reconcile = transitions(state).filter((t) => t.name.startsWith("reconcileStartup")) + assert.equal(reconcile.length, 1) + assert.equal(reconcile[0]!.next, state) + } + + // Both signals stale — the genuine crash orphan: the startup repair must + // still run (child → interrupted, parent → active, both pointers cleared). + { + const state: ModelState = { + tasks: { parent: delegated, "child-a": childA, "child-b": undefined }, + liveElsewhere: { parent: false, "child-a": false, "child-b": false }, + heartbeatAlive: { parent: false, "child-a": false, "child-b": false }, + } + const reconcile = transitions(state).filter((t) => t.name.startsWith("reconcileStartup")) + assert.equal(reconcile.length, 1) + const repaired = reconcile[0]!.next + assert.equal(repaired.tasks["child-a"]?.status, "interrupted") + assert.equal(repaired.tasks.parent?.status, "active") + assert.equal(repaired.tasks.parent?.awaitingChildId, undefined) + assert.equal(repaired.tasks.parent?.delegatedToId, undefined) + } + const rejectedParent: HistoryItem = { ...parent, status: "interrupted", @@ -453,7 +714,7 @@ function runRepresentativeScenarios(): void { } runRepresentativeScenarios() -const checkedStates = runModelCheck() +const result = runModelCheck() console.log( - `Task lifecycle model check passed: ${checkedStates} reachable states, ${expectedActions.length}/${expectedActions.length} actions reachable, ${Object.keys(semanticLandmarks).length}/${Object.keys(semanticLandmarks).length} landmarks reached, ${Object.keys(semanticWitnesses).length}/${Object.keys(semanticWitnesses).length} semantic witnesses reached, depth <= ${MAX_DEPTH}, ${taskIds.length} task slots`, + `Task lifecycle model check passed: ${result.states} reachable states, ${result.reachedActions.size}/${expectedActions.length} actions reachable, ${result.reachedLandmarks.size}/${Object.keys(semanticLandmarks).length} landmarks reached, ${result.reachedWitnesses.size}/${Object.keys(semanticWitnesses).length} semantic witnesses reached, depth <= ${MAX_DEPTH}, ${taskIds.length} task slots`, ) diff --git a/src/__tests__/ClineProvider.delegation.spec.ts b/src/__tests__/ClineProvider.delegation.spec.ts index 68f39c6517..78d62e92f9 100644 --- a/src/__tests__/ClineProvider.delegation.spec.ts +++ b/src/__tests__/ClineProvider.delegation.spec.ts @@ -31,6 +31,8 @@ function makeStoreStub( return [] }), get: vi.fn().mockReturnValue(undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), ...overrides, } } @@ -787,6 +789,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { current = updater(current) return [current] }), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), clearPendingActionIfMatching: vi.fn(async (_taskId: string, actionId: string) => { if (current.pendingAction?.kind === "create_subtask" && current.pendingAction.actionId === actionId) { current = { ...current, pendingAction: undefined } @@ -874,6 +878,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { get: vi.fn(() => interruptedParent), atomicReadAndUpdate, clearPendingActionIfMatching, + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), }, } as unknown as ClineProvider @@ -938,6 +944,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { get: vi.fn(() => interruptedParent), atomicReadAndUpdate, clearPendingActionIfMatching, + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), }, } as unknown as ClineProvider @@ -1003,6 +1011,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { get: vi.fn(() => interruptedParent), atomicReadAndUpdate, clearPendingActionIfMatching, + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), }, } as unknown as ClineProvider @@ -1070,6 +1080,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { return [] }), clearPendingActionIfMatching, + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), }, } as unknown as ClineProvider @@ -1137,6 +1149,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { return [] }), clearPendingActionIfMatching, + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), }, } as unknown as ClineProvider @@ -1221,6 +1235,8 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { }), clearPendingActionIfMatching, deleteMany: vi.fn().mockResolvedValue(undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), }, } as unknown as ClineProvider diff --git a/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts index 852e2f5a67..c047597535 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -5,7 +5,12 @@ import { type Task } from "../../core/task/Task" type ProviderStubFields = { cancelledDelegationChildIds?: Set log?: ReturnType - taskHistoryStore?: { get: (id: string) => unknown; invalidate?: (id: string) => Promise } + taskHistoryStore?: { + get: (id: string) => unknown + invalidate?: (id: string) => Promise + markLocallyActive?: (taskId: string) => void + markLocallyInactive?: (taskId: string) => void + } taskScheduler?: { schedule: (task: Task, run: () => Promise) => Promise } taskRegistry?: TaskRegistry clineStack?: Task[] @@ -38,6 +43,8 @@ export function makeProviderStub(stub: T): ClineProvider { s.log ??= vi.fn() s.taskHistoryStore ??= { get: () => undefined } s.taskHistoryStore.invalidate ??= async () => {} + s.taskHistoryStore.markLocallyActive ??= () => {} + s.taskHistoryStore.markLocallyInactive ??= () => {} s.taskScheduler ??= { schedule: async (_task, run) => run() } // Convert legacy clineStack array into a TaskRegistry diff --git a/src/__tests__/single-open-invariant.spec.ts b/src/__tests__/single-open-invariant.spec.ts index 13fc4bc6fb..f0f64c900a 100644 --- a/src/__tests__/single-open-invariant.spec.ts +++ b/src/__tests__/single-open-invariant.spec.ts @@ -81,7 +81,11 @@ 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(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -130,6 +134,11 @@ describe("Single-open-task invariant", () => { const provider = { taskRegistry: registry2, taskScheduler: new TaskScheduler(), + taskHistoryStore: { + get: vi.fn(() => undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + }, setValues: vi.fn(), getState: vi.fn().mockResolvedValue({ apiConfiguration: { apiProvider: providerIdentifiers.anthropic, consecutiveMistakeLimit: 0 }, @@ -169,7 +178,11 @@ 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(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -244,7 +257,11 @@ 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(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) @@ -320,7 +337,11 @@ 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(), + markLocallyInactive: vi.fn(), + }, evictCurrentTask, addClineToStack: vi.fn().mockImplementation(async (task: Task) => registry.push(task)), log: vi.fn(), @@ -387,7 +408,11 @@ 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(), + markLocallyInactive: vi.fn(), + }, markDelegatedChildInterrupted: vi.fn().mockResolvedValue(undefined), get evictCurrentTask() { return privateClineProvider.evictCurrentTask.bind(this) diff --git a/src/__tests__/task-run-dispatch.spec.ts b/src/__tests__/task-run-dispatch.spec.ts index 90af5a519d..2e2e090f6f 100644 --- a/src/__tests__/task-run-dispatch.spec.ts +++ b/src/__tests__/task-run-dispatch.spec.ts @@ -18,6 +18,7 @@ type Runnable = { resumeTaskFromHistory: () => Promise startTask: (task?: string, images?: string[]) => Promise startIdleTelemetryCheck: () => void + startLivenessHeartbeat: () => void } function makeRunnable(overrides: Partial = {}): Runnable & { run(): Promise } { @@ -29,6 +30,7 @@ function makeRunnable(overrides: Partial = {}): Runnable & { run(): Pr resumeTaskFromHistory: vi.fn().mockResolvedValue(undefined), startTask: vi.fn().mockResolvedValue(undefined), startIdleTelemetryCheck: vi.fn(), + startLivenessHeartbeat: vi.fn(), ...overrides, } // Bind the real run() implementation from Task.prototype to our stand-in. diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 346d697d9a..2fecea6c1b 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -10,7 +10,13 @@ import { GlobalFileNames } from "../../shared/globalFileNames" import { LOCK_STALE_MS, withFileLock } from "../../utils/fileLock" import { safeWriteJson } from "../../utils/safeWriteJson" import { getStorageBasePath } from "../../utils/storage" -import { assertValidTransition, settleRejectedCreateSubtaskAction, type HistoryItemStatus } from "./taskLifecycle" +import { + assertValidTransition, + isDelegatedChildLive, + isLivenessSignalFresh, + settleRejectedCreateSubtaskAction, + type HistoryItemStatus, +} from "./taskLifecycle" import { computeHistoryDelta, DeltaRejectedError, mergeHistoryDelta } from "./taskStoreConcurrency" export { assertValidTransition, type HistoryItemStatus } from "./taskLifecycle" @@ -24,6 +30,23 @@ function mergeWithDisk(delta: Partial): (existing: unknown, incomin return (existing, incoming) => mergeHistoryDelta(existing, incoming as HistoryItem, delta) } +/** + * Control-flow signal: a liveness heartbeat merged under the per-file + * advisory lock found the disk record absent, non-active, or already current. + * The beat must skip (never recreate or rewrite), so the merge aborts the + * write by throwing; `recordTaskActivity` catches this class and stays quiet. + */ +class HeartbeatSkipError extends Error {} + +/** + * Control-flow signal: an administrative repair write re-validated its + * preconditions under the target file's advisory lock (via a safeWriteJson + * merge callback) and found a peer had changed the record mid-repair. The + * write aborts by throwing; the repair paths catch this class, leave the + * durable intent in place, and report "not repaired". + */ +class RepairAbortedError extends Error {} + /** * Durable intent for the one repair that spans an active delegated child and * its parent. Task files remain authoritative; this file only records the @@ -86,6 +109,33 @@ 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() + /** + * Ids whose local ownership was explicitly released (Task.dispose, + * scheduler rejection, delegation rollback) AFTER an active-status write + * had claimed them. `trackLocalSessionOwnership` must not re-add these from + * late teardown writes (an in-flight heartbeat beat or final save that + * preserves "active" status): the session is gone, and a ghost re-claim + * would suppress in-window orphan repair for the window's lifetime. An + * explicit `markLocallyActive` (a new session's eager claim) clears the + * entry, so resumes and re-delegations re-claim normally; any non-active + * status write clears it too (the record then proves no live session). + */ + private readonly releasedLocalOwnershipIds = new Set() private disposed = false /** @@ -98,6 +148,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 @@ -251,6 +308,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() @@ -276,6 +339,8 @@ export class TaskHistoryStore { return this.withLock(async () => { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) // Remove per-task file (best-effort) try { @@ -304,6 +369,8 @@ export class TaskHistoryStore { for (const taskId of taskIds) { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) // Remove per-task file (best-effort) try { @@ -391,6 +458,10 @@ export class TaskHistoryStore { if (!liveIds.has(taskId)) { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + // The record is gone (e.g. removed by a peer window), so any + // local-ownership claim for it is stale too. + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) } } }) @@ -399,8 +470,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 @@ -465,29 +537,104 @@ export class TaskHistoryStore { const child = byId.get(item.awaitingChildId) if (!child) { - await this.upsertCore( - { - ...item, - status: "active", - awaitingChildId: undefined, - delegatedToId: undefined, - }, - { skipTransitionCheck: true }, - ) - console.warn( - `[TaskHistoryStore] Reconciled orphaned delegation: task ${item.id} → active (child ${item.awaitingChildId} not found)`, - ) + // A brand-new delegation persists the parent as "delegated" + // BEFORE the child's first history write (and before the + // scheduler admits the child run). Repairing immediately in + // that gap severs a healthy link, so two grace signals apply: + // 1. an eager local ownership claim for the awaited child id + // (installed by the delegating window) means a session in + // THIS window is still starting the child — skip; + // 2. a freshly written parent record means the delegation + // transition itself just landed (including from a peer + // window this store cannot claim for) — skip while the + // parent's own history file is fresh. + // A genuinely orphaned delegation outlives both signals: the + // claim is released with the session and the parent file goes + // quiet, so the next pass repairs it. An unreadable/absent + // parent mtime stays conservative and repairs immediately. + if (this.locallyActiveTaskIds.has(item.awaitingChildId)) { + console.warn(`[TaskHistoryStore] Skipping repair for delegation ${item.id}: child ${item.awaitingChildId} claimed by a live session in this window`) + continue + } + const parentMtimeMs = await this.getChildFileMtimeMs(item.id) + if ( + parentMtimeMs !== undefined && + isLivenessSignalFresh( + parentMtimeMs, + Date.now(), + TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS, + ) + ) { + console.warn(`[TaskHistoryStore] Skipping repair for delegation ${item.id}: parent record freshly written (child ${item.awaitingChildId} may still be starting)`) + continue + } + const repairedParent: HistoryItem = { ...item, status: "active", awaitingChildId: undefined, delegatedToId: undefined } + // The repair write re-validates under the parent's advisory lock + // (same cross-process guard as the active-child repair): the parent + // must still be delegated to the same child, and the child's file + // must still be absent — a peer that landed the child record in the + // gap wins. + const childFilePath = await this.getTaskFilePath(item.awaitingChildId!) + try { + await this.writeAdministrativeRepair(repairedParent, (existing) => + this.assertMissingChildRepairPreconditions(existing, childFilePath, item), + ) + } catch (error) { + if (!(error instanceof RepairAbortedError)) { + throw error + } + console.warn(`[TaskHistoryStore] Aborting repair for delegation ${item.id}: repair raced a peer write`) + continue + } + this.cache.set(repairedParent.id, repairedParent) + if (this.onWrite) { + await this.onWrite(this.getAll()) + } + console.warn(`[TaskHistoryStore] Reconciled orphaned delegation: task ${item.id} → active (child ${item.awaitingChildId} not found)`) repairsInThisPass++ } else if ((child.status ?? "active") === "active" && persistedActiveIds.has(child.id)) { + // Cross-instance liveness guard: a child whose history file was written + // recently, or whose owning session heartbeats a fresh `lastActivityAt`, + // is owned by a live window — not a crash orphan. + const mtimeMs = await this.getChildFileMtimeMs(child.id) + const isLiveElsewhere = isDelegatedChildLive( + child, + Date.now(), + TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS, + mtimeMs, + ) + if (isLiveElsewhere) { + console.warn( + `[TaskHistoryStore] Skipping repair for live child ${child.id} ` + + `(${this.describeLivenessSignal(child, mtimeMs)}) — owned by another window`, + ) + 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 // administrative recovery, not a runtime delegation transition. - await this.repairActiveDelegation(item, child) - console.warn( - `[TaskHistoryStore] Reconciled orphaned active child: child ${child.id} → interrupted, task ${item.id} → active`, - ) - repairsInThisPass++ + const repaired = await this.repairActiveDelegation(item, child) + if (repaired) { + console.warn( + `[TaskHistoryStore] Reconciled orphaned active child: child ${child.id} → interrupted, task ${item.id} → active`, + ) + repairsInThisPass++ + } } else if (child.status === "completed") { await this.upsertCore( { @@ -522,11 +669,141 @@ 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") { + // A released id must not be re-claimed by late teardown writes from + // its disposed session; only an explicit markLocallyActive clears + // the release (see releasedLocalOwnershipIds). + if (!this.releasedLocalOwnershipIds.has(written.id)) { + this.locallyActiveTaskIds.add(written.id) + } + } else { + this.locallyActiveTaskIds.delete(written.id) + this.releasedLocalOwnershipIds.delete(written.id) + } + } + + /** + * 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. + * The eager claim also clears any prior ownership release, so a resumed or + * re-delegated session re-claims an id its predecessor released at dispose. + */ + public markLocallyActive(taskId: string): void { + this.releasedLocalOwnershipIds.delete(taskId) + this.locallyActiveTaskIds.add(taskId) + } + + /** + * Release a task id claimed by `markLocallyActive` when its session did not + * start (preparation failure, scheduler rejection, startTask disabled) or + * has been torn down (Task.dispose). Re-running reconciliation for the id + * is safe: without local ownership the periodic pass treats it like any + * other persisted record. The release is remembered so a late active-status + * write from the dead session cannot ghost-reclaim the id. + */ + public markLocallyInactive(taskId: string): void { + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.add(taskId) + } + + /** + * Persist a liveness heartbeat for a live task session: bump the record's + * `lastActivityAt` so a delegated child quiet for minutes while running a + * long tool call, streaming a long turn, or awaiting a user ask is still + * recognized as alive by reconciliation in another window or after an + * extension-host restart. + * + * Callers are expected to throttle (see Task's liveness heartbeat, one + * beat per minute over the whole active lifetime). The write is scoped to + * the periodic refresh path: a record whose cached status is not active + * (or is unknown) writes NOTHING — a completed, interrupted, or delegated + * task must not have history_item.json and globalState rewritten every + * minute while it lingers before disposal. Under the per-file advisory + * lock the merge re-validates the disk record and NEVER recreates a file + * another host deleted (a heartbeat is a refresh, not a creation — the + * normal task-start write path still creates files). Best-effort by + * design: a missing record, a non-active status, or a transient write + * failure never rejects — a heartbeat must never disturb the turn it + * reports on. + */ + public async recordTaskActivity(taskId: string, at: number = Date.now()): Promise { + try { + const cached = this.cache.get(taskId) + if (!cached || (cached.status ?? "active") !== "active") { + return + } + await this.withLock(async () => { + const filePath = await this.getTaskFilePath(taskId) + let written: HistoryItem | undefined + try { + await safeWriteJson(filePath, cached, { + merge: (existing) => { + if (!existing || typeof existing !== "object" || !("id" in existing)) { + // Confirmed absent under the lock: a peer deleted the + // record. A heartbeat must never recreate it — that + // ghost would carry fresh liveness signals and + // suppress orphan repair. Drop the stale cache and + // claim with it. + this.cache.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) + throw new HeartbeatSkipError(`record ${taskId} deleted by another host`) + } + const disk = existing as HistoryItem + if ((disk.status ?? "active") !== "active") { + this.cache.set(taskId, disk) + throw new HeartbeatSkipError(`record ${taskId} no longer active on disk`) + } + if (disk.lastActivityAt === at) { + this.cache.set(taskId, disk) + throw new HeartbeatSkipError(`record ${taskId} already at ${at}`) + } + written = { ...disk, lastActivityAt: at } + return written + }, + }) + } catch (error) { + if (error instanceof HeartbeatSkipError) { + if (error.message.includes("deleted by another host")) { + console.warn(`[TaskHistoryStore] Skipping liveness heartbeat for ${taskId}: ${error.message}`) + } + return + } + throw error + } + if (written) { + this.cache.set(taskId, written) + if (this.onWrite) { + await this.onWrite(this.getAll()) + } + } + }) + } catch (error) { + console.warn(`[TaskHistoryStore] Failed to record activity heartbeat for ${taskId}:`, error) + } + } + /** * 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 @@ -562,18 +839,59 @@ 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. 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 = isDelegatedChildLive( + child, + Date.now(), + TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS, + mtimeMs, + ) + if (isLiveElsewhere) { + await this.quarantineDelegationRepairIntent( + intent, + `child live in another window (${this.describeLivenessSignal(child, mtimeMs)})`, + ) + return + } + } + const repairedChild = childAtTarget ? child : { ...child, status: intent.target.childStatus } const repairedParent = parentMatchesTargetState ? parent - : { - ...parent, - status: intent.target.parentStatus, - awaitingChildId: undefined, - delegatedToId: undefined, - } + : { ...parent, status: intent.target.parentStatus, awaitingChildId: undefined, delegatedToId: undefined } - if (!childAtTarget) await this.writeTaskFile(repairedChild) - if (!parentMatchesTargetState) await this.writeTaskFile(repairedParent) + // Same cross-process guard as the in-pass repair: each write + // re-validates its preconditions under the target file's advisory + // lock, so a peer write that landed after the pre-write liveness + // check is observed and aborts the stale replay (the journal stays + // for a later pass, and replay skips sides already at target). + try { + if (!childAtTarget) { + await this.writeAdministrativeRepair(repairedChild, (existing, filePath) => this.assertChildRepairPreconditions(existing, filePath, child.id)) + } + if (!parentMatchesTargetState) { + await this.writeAdministrativeRepair(repairedParent, (existing) => this.assertParentRepairPreconditions(existing, child.id)) + } + } catch (error) { + if (error instanceof RepairAbortedError) { + // A peer changed the records between the pre-write liveness + // check and the lock. The replay is stale: quarantine the + // intent rather than stomping the peer's write. + await this.quarantineDelegationRepairIntent(intent, `repair raced a peer write (${error.message})`) + return + } + throw error + } this.cache.set(repairedChild.id, repairedChild) this.cache.set(repairedParent.id, repairedParent) @@ -589,8 +907,11 @@ export class TaskHistoryStore { /** * Start and complete a guarded active-child repair while already holding the * store lock. The intent is durable before either task file is touched. + * Returns false when a last-moment liveness re-check (see + * `applyDelegationRepairIntent`) shows the child is alive again — no write + * happened and the durable intent is removed. */ - private async repairActiveDelegation(parent: HistoryItem, child: HistoryItem): Promise { + private async repairActiveDelegation(parent: HistoryItem, child: HistoryItem): Promise { const intent: DelegationRepairIntent = { version: 1, operationId: crypto.randomUUID(), @@ -612,24 +933,54 @@ export class TaskHistoryStore { } await this.writeDelegationRepairIntent(intent) - await this.applyDelegationRepairIntent(intent, child, parent) + return this.applyDelegationRepairIntent(intent, child, parent) } private async applyDelegationRepairIntent( intent: DelegationRepairIntent, child: HistoryItem, parent: HistoryItem, - ): Promise { - const repairedChild = { ...child, status: intent.target.childStatus } - const repairedParent = { - ...parent, - status: intent.target.parentStatus, - awaitingChildId: undefined, - delegatedToId: undefined, + ): Promise { + // Pre-write liveness re-check (cheap early-out): the repair decision at + // the call site came from an unlocked stat, and a peer window can resume + // the child while the intent journal was being written. + if (this.locallyActiveTaskIds.has(child.id)) { + await this.removeDelegationRepairIntent() + console.warn(`[TaskHistoryStore] Aborting repair for child ${child.id}: claimed by a live session in this window`) + return false + } + const mtimeMs = await this.getChildFileMtimeMs(child.id) + if (isDelegatedChildLive(child, Date.now(), TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS, mtimeMs)) { + await this.removeDelegationRepairIntent() + console.warn(`[TaskHistoryStore] Aborting repair for live child ${child.id} (${this.describeLivenessSignal(child, mtimeMs)}) — owned by another window`) + return false } - await this.writeTaskFile(repairedChild) - await this.writeTaskFile(repairedParent) + const repairedChild = { ...child, status: intent.target.childStatus } + const repairedParent = { ...parent, status: intent.target.parentStatus, awaitingChildId: undefined, delegatedToId: undefined } + + // The authoritative writes carry an under-lock re-validation: the merge + // callback runs inside the target file's advisory lock (the same + // cross-process guard pattern as clearPendingActionIfMatching), so a + // peer write that lands in the gap before the lock is observed and the + // stale repair aborts instead of stomping it. An abort after the child + // write has landed keeps the durable intent: replay skips files already + // at target and completes only the missing side. + let childWritten = false + try { + await this.writeAdministrativeRepair(repairedChild, (existing, filePath) => this.assertChildRepairPreconditions(existing, filePath, child.id)) + childWritten = true + await this.writeAdministrativeRepair(repairedParent, (existing) => this.assertParentRepairPreconditions(existing, child.id)) + } catch (error) { + if (error instanceof RepairAbortedError) { + console.warn(`[TaskHistoryStore] Aborting repair for child ${child.id}: ${error.message}`) + if (childWritten) { + this.cache.set(repairedChild.id, repairedChild) + } + return false + } + throw error + } this.cache.set(repairedChild.id, repairedChild) this.cache.set(repairedParent.id, repairedParent) @@ -638,6 +989,99 @@ export class TaskHistoryStore { await this.onWrite(this.getAll()) } await this.removeDelegationRepairIntent() + return true + } + + /** + * Write one administrative repair record with its precondition re-validated + * UNDER the target file's advisory lock: the `guard` runs inside + * safeWriteJson's merge callback, which reads the current disk record after + * the lock is acquired. Throwing `RepairAbortedError` aborts the write + * (nothing is committed); any other error propagates. Returns true when the + * write committed. + */ + private async writeAdministrativeRepair( + target: HistoryItem, + guard: (existing: unknown, filePath: string) => void, + ): Promise { + const filePath = await this.getTaskFilePath(target.id) + await safeWriteJson(filePath, target, { + merge: (existing) => { + guard(existing, filePath) + return target + }, + }) + // No cache update here: multi-file repairs must publish cache state + // atomically after ALL writes commit (see applyDelegationRepairIntent). + } + + /** + * Under-lock preconditions for the child half of an orphan repair. A peer + * write in the gap before the advisory lock is observed here and aborts: + * - a child record whose status is no longer active was transitioned by a + * peer (resume/completion/abandonment); + * - a fresh child mtime means a peer is persisting the child again; + * - an in-window ownership claim landed mid-repair. + * An absent or unreadable record/mtime PROCEEDS, matching the established + * conservative-repair contract (undefined mtime is not evidence of life). + */ + private assertChildRepairPreconditions(existing: unknown, filePath: string, childId: string): void { + if (existing && typeof existing === "object" && "id" in existing) { + const disk = existing as HistoryItem + if ((disk.status ?? "active") !== "active") { + throw new RepairAbortedError(`child status already ${disk.status ?? "active"}`) + } + } + const mtimeMs = this.statChildFileMtimeMsSync(filePath) + if (mtimeMs !== undefined && isLivenessSignalFresh(mtimeMs, Date.now(), TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS)) { + throw new RepairAbortedError("child mtime became fresh under lock (peer resumed)") + } + if (this.locallyActiveTaskIds.has(childId)) { + throw new RepairAbortedError("child claimed by this window during repair") + } + } + + /** + * Synchronous mtime read used by the under-lock repair guards. Extracted so + * tests can inject the same deterministic ages as the async + * `getChildFileMtimeMs` probe. Returns undefined when the file is absent or + * unreadable (no evidence of life either way). + */ + private statChildFileMtimeMsSync(filePath: string): number | undefined { + try { + return fsSync.statSync(filePath).mtimeMs + } catch { + return undefined + } + } + + /** + * Under-lock preconditions for the parent half of an orphan repair: the + * record must still exist and still be delegated to the same child (a peer + * may have completed, abandoned, or re-delegated it in the gap). + */ + private assertParentRepairPreconditions(existing: unknown, childId: string): void { + if (!existing || typeof existing !== "object" || !("id" in existing)) { + throw new RepairAbortedError("parent record deleted under lock") + } + const disk = existing as HistoryItem + if (disk.status !== "delegated" || disk.awaitingChildId !== childId) { + throw new RepairAbortedError(`parent no longer delegated to child (status ${disk.status ?? "active"})`) + } + } + + /** + * Under-lock preconditions for the missing-child repair: the parent must + * still be delegated to the same (still absent) child. A child file that + * appears under the lock means a peer landed the record in the gap — the + * awaited child now exists, so severing the link would orphan a live child. + */ + private assertMissingChildRepairPreconditions(existing: unknown, childFilePath: string, item: HistoryItem): void { + this.assertParentRepairPreconditions(existing, item.awaitingChildId!) + if (this.statChildFileMtimeMsSync(childFilePath) !== undefined) { + throw new RepairAbortedError("child record appeared under lock (peer landed it)") + } + // Still absent — proceed. } private matchesDelegationRepairParentPreconditions(intent: DelegationRepairIntent, parent: HistoryItem): boolean { @@ -766,19 +1210,44 @@ export class TaskHistoryStore { /** * Invalidate a single task's cache entry (re-read from disk on next access). + * Only a CONFIRMED absence (ENOENT) drops the cache entry together with any + * local ownership claim: a session cannot own a record that no longer + * exists. Transient read errors (EBUSY/EACCES/...) and malformed content + * keep both the cache entry and the claim — readTaskFile maps every failure + * to null, but a live session's ownership must not be dropped because ONE + * locked or corrupt read could not prove absence. This mirrors the + * cross-instance convention in `getChildFileMtimeMs` (transient stat + * failure = evidence of life). */ async invalidate(taskId: string): Promise { return this.withLock(async () => { + let raw: string try { - const item = await this.readTaskFile(taskId) - if (item) { - this.cache.set(taskId, item) - } else { + raw = await fs.readFile(await this.getTaskFilePath(taskId), "utf8") + } catch (error) { + if (this.isFileNotFoundError(error)) { + // Confirmed absent: the record no longer exists, so the + // cache entry and any local ownership claim are stale. this.cache.delete(taskId) + this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) + } + // Any other read error keeps everything: the next access retries. + return + } + let item: HistoryItem | null = null + try { + const parsed: unknown = JSON.parse(raw) + if (parsed && typeof parsed === "object" && "id" in parsed && (parsed as HistoryItem).id) { + item = parsed as HistoryItem } - this.taskFileMtimes.delete(taskId) } catch { - this.cache.delete(taskId) + // Malformed content: keep the cache entry and the claim (see above). + } + if (item) { + this.cache.set(taskId, item) + this.taskFileMtimes.delete(taskId) } }) } @@ -949,6 +1418,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) { @@ -964,10 +1440,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 ────────────────────────────── /** @@ -1058,12 +1578,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) { @@ -1114,6 +1637,8 @@ export class TaskHistoryStore { missingDiskRecord = true this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) throw new Error( `[TaskHistoryStore] clearPendingActionIfMatching: task ${taskId} not found in cache`, ) @@ -1122,12 +1647,15 @@ export class TaskHistoryStore { // task-history schema (#1726). Settlement must not // rewrite malformed data and must not clear the action // on a record persisted under a different task ID, so - // both cases drop the stale cache entry and fail - // closed without touching the disk record. + // both cases drop the stale cache entry (and any local + // ownership claim — the record no longer exists) and + // fail closed without touching the disk record. const parsed = historyItemSchema.safeParse(existing) if (!parsed.success) { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) throw new Error( `[TaskHistoryStore] clearPendingActionIfMatching: task ${taskId} has an invalid disk record`, ) @@ -1135,6 +1663,8 @@ export class TaskHistoryStore { if (parsed.data.id !== taskId) { this.cache.delete(taskId) this.taskFileMtimes.delete(taskId) + this.locallyActiveTaskIds.delete(taskId) + this.releasedLocalOwnershipIds.delete(taskId) throw new Error( `[TaskHistoryStore] clearPendingActionIfMatching: task ${taskId} has a disk record with mismatched id ${parsed.data.id}`, ) @@ -1192,4 +1722,60 @@ 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 + * 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 { + const filePath = await this.getTaskFilePath(childId) + const stat = await fs.stat(filePath) + return stat.mtimeMs + } catch (error) { + // 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 + } + } + + /** + * Renders the signal that keeps a delegated child from being repaired as a + * crash orphan: the freshest of the history-file mtime and the persisted + * `lastActivityAt` heartbeat. Called only when `isDelegatedChildLive` held, + * so at least one signal is fresh; the mtime label wins when the file mtime + * is itself fresh, preserving the historical skip-log shape. + */ + private describeLivenessSignal(child: HistoryItem, mtimeMs: number | undefined): string { + const now = Date.now() + if (mtimeMs !== undefined && isLivenessSignalFresh(mtimeMs, now, TaskHistoryStore.LIVE_CHILD_MTIME_THRESHOLD_MS)) { + return `mtime ${Math.round((now - mtimeMs) / 1000)}s ago` + } + return `heartbeat ${Math.round((now - (child.lastActivityAt ?? 0)) / 1000)}s ago` + } } diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index e37fd1a25e..44e51beb6f 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -7,23 +7,59 @@ import * as os from "os" import type { HistoryItem } from "@roo-code/types" import { GlobalFileNames } from "../../../shared/globalFileNames" +import { LOCK_STALE_MS } from "../../../utils/fileLock" import { TaskHistoryStore, assertValidTransition } from "../TaskHistoryStore" vi.mock("../../../utils/storage", () => ({ getStorageBasePath: vi.fn().mockImplementation((defaultPath: string) => defaultPath), })) -const writeJson = async (filePath: string, data: unknown): Promise => { +const writeJson = async ( + filePath: string, + data: unknown, + options?: { merge?: (existing: unknown, incoming: unknown) => unknown }, +): Promise => { await fs.mkdir(path.dirname(filePath), { recursive: true }) - await fs.writeFile(filePath, JSON.stringify(data, null, "\t"), "utf8") + let outgoing = data + if (options?.merge) { + // Mirror safeWriteJson's contract: read the current file and let the + // caller's merge decide what is written (a throwing merge aborts the + // write). Missing or unparseable content reads as null. + let existing: unknown = null + try { + existing = JSON.parse(await fs.readFile(filePath, "utf8")) + } catch { + existing = null + } + outgoing = options.merge(existing, data) + } + await fs.writeFile(filePath, JSON.stringify(outgoing, null, "\t"), "utf8") } 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) +// 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)}`, @@ -59,6 +95,99 @@ 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}.`, + ) +} + +// Under-lock mtime probe companion: the round-3 repair guards re-read the +// child mtime synchronously UNDER the per-file advisory lock +// (`statChildFileMtimeMsSync`). When a test injects an async +// `getChildFileMtimeMs` age, the sync probe must inject the SAME age: +// otherwise the under-lock precondition re-check reads the real (fresh) seeded +// mtime and aborts the repair the test is exercising. Restored per describe +// alongside `mtimeSpy`. +let syncMtimeSpy: { mockRestore(): void } | undefined + +function spySyncMtimeProbe( + impl: (filePath: string, real: (fp: string) => number | undefined) => number | undefined, +): void { + const probe = TaskHistoryStore.prototype as unknown as { + statChildFileMtimeMsSync: (filePath: string) => number | undefined + } + const original = probe.statChildFileMtimeMsSync + syncMtimeSpy = vi.spyOn(probe, "statChildFileMtimeMsSync").mockImplementation((fp: string) => + impl(fp, (realFp) => original.call(probe, realFp)), + ) +} + +/** + * 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 // ───────────────────────────────────────────────────────────────────────────── @@ -151,14 +280,64 @@ 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 + * 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) + } + + /** + * 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) + 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), + ) + spySyncMtimeProbe((fp, real) => (fp === filePath ? FIXED_NOW - ageMs : real(fp))) } beforeEach(async () => { @@ -166,7 +345,117 @@ describe("TaskHistoryStore reconcileDelegationState", () => { 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 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 { + getChildFileMtimeMs: (childId: string) => Promise + } + + expect(await internals.getChildFileMtimeMs("missing-mtime-child")).toBeUndefined() + + const child = makeItem({ id: "present-mtime-child", status: "active" }) + 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() + } + }) + + 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 + syncMtimeSpy?.mockRestore() + syncMtimeSpy = undefined safeWriteJsonMock.mockImplementation(writeJson) for (const disposable of disposables) disposable.dispose() disposables.clear() @@ -175,7 +464,11 @@ 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]) + // A genuinely orphaned delegation: the parent record stopped being + // written long ago, so the brand-new-delegation grace check must NOT + // protect it (a freshly written parent record would be skipped instead). + await markStaleMtime("parent-1") await store.initialize() @@ -185,6 +478,277 @@ describe("TaskHistoryStore reconcileDelegationState", () => { expect(repaired?.delegatedToId).toBeUndefined() }) + it("skips repairing a brand-new delegation whose child record has not landed yet (fresh parent record)", async () => { + // The delegating window persists the parent BEFORE the child's first + // history write (and before the scheduler admits the child run). A + // startup pass or periodic tick in that gap must not sever the healthy + // link: the freshly written parent record is the grace signal. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const parent = makeItem({ id: "parent-fresh", status: "delegated", awaitingChildId: "child-starting" }) + await seedItems(tmpDir, [parent]) + // Parent file was just written by seedItems → mtime is fresh. + + await store.initialize() + + expect(store.get("parent-fresh")?.status).toBe("delegated") + expect(store.get("parent-fresh")?.awaitingChildId).toBe("child-starting") + const persistedParent = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", "parent-fresh", "history_item.json"), "utf8"), + ) as HistoryItem + expect(persistedParent.status).toBe("delegated") + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("parent record freshly written")) + warnSpy.mockRestore() + }) + + it("skips while the awaited child id is claimed by this window and repairs after the claim is released", async () => { + // The delegating window claims the child id eagerly at creation; the + // missing-child repair must honor that claim and only repair once the + // claim is gone (session ended without the child ever being written). + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const parent = makeItem({ id: "parent-claimed", status: "delegated", awaitingChildId: "child-claimed" }) + await seedItems(tmpDir, [parent]) + await markStaleMtime("parent-claimed") + + store.markLocallyActive("child-claimed") + await store.initialize() + expect(store.get("parent-claimed")?.status).toBe("delegated") + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("claimed by a live session in this window")) + + // Session gone, claim released, parent record long quiet → orphan repair. + store.markLocallyInactive("child-claimed") + await store.initialize() + expect(store.get("parent-claimed")?.status).toBe("active") + expect(store.get("parent-claimed")?.awaitingChildId).toBeUndefined() + warnSpy.mockRestore() + }) + + it("aborts the orphan repair when the child goes live between the liveness stat and the repair write (lock-time re-check)", async () => { + // TOCTOU coverage: the orphan decision is taken from an unlocked stat; + // a peer window resuming the child in the gap (fresh writes → fresh + // mtime) must win. The re-check inside the repair path sees the fresh + // second stat, writes nothing, and removes the just-written intent. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const child = makeItem({ + id: "child-toctou", + status: "active", + parentTaskId: "parent-toctou", + rootTaskId: "parent-toctou", + }) + const parent = makeItem({ + id: "parent-toctou", + status: "delegated", + awaitingChildId: "child-toctou", + delegatedToId: "child-toctou", + childIds: ["child-toctou"], + }) + await seedItems(tmpDir, [parent, child]) + await markStaleMtime("child-toctou") + + // First stat (the orphan decision): stale. Second stat (the lock-time + // re-check immediately before the repair write): fresh — the peer + // resumed the child in the gap. + const probe = TaskHistoryStore.prototype as unknown as { + getChildFileMtimeMs: (childId: string) => Promise + } + const original = probe.getChildFileMtimeMs + let calls = 0 + mtimeSpy = vi + .spyOn(probe, "getChildFileMtimeMs") + .mockImplementation((childId: string) => { + if (childId !== "child-toctou") { + return original.call(store, childId) + } + calls += 1 + return calls === 1 + ? Promise.resolve(Date.now() - 10 * 60 * 1000) + : Promise.resolve(Date.now()) + }) + + await store.initialize() + + // No repair: link preserved in cache and on disk. + expect(store.get("child-toctou")?.status).toBe("active") + expect(store.get("parent-toctou")?.status).toBe("delegated") + expect(store.get("parent-toctou")?.awaitingChildId).toBe("child-toctou") + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Aborting repair for live child child-toctou")) + expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("Reconciled orphaned active child")) + + // The durable intent was removed, not left for a stale replay. + await expect(fs.access(path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent))).rejects.toThrow() + + // The child record on disk is untouched. + const persistedChild = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", "child-toctou", GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedChild.status).toBe("active") + + warnSpy.mockRestore() + }) + + it("invalidate keeps the ownership claim on a transient read failure and drops it only on confirmed absence", async () => { + // readTaskFile maps every failure to null; invalidate() must not let one + // transient Windows-style read (EBUSY/EISDIR/...) drop a LIVE session's + // ownership claim — only confirmed absence (ENOENT) may. + const item = makeItem({ id: "transient-read-claim", status: "active" }) + await store.upsert(item) + const internals = store as unknown as { locallyActiveTaskIds: Set } + expect(internals.locallyActiveTaskIds.has("transient-read-claim")).toBe(true) + const filePath = path.join(tmpDir, "tasks", "transient-read-claim", GlobalFileNames.historyItem) + // A directory at the file path makes readFile fail with EISDIR (a + // non-ENOENT read error) without removing the record. + await fs.rm(filePath) + await fs.mkdir(filePath) + await store.invalidate("transient-read-claim") + expect(internals.locallyActiveTaskIds.has("transient-read-claim")).toBe(true) + // Confirmed absence drops the claim. + await fs.rmdir(filePath) + await store.invalidate("transient-read-claim") + expect(internals.locallyActiveTaskIds.has("transient-read-claim")).toBe(false) + }) + + it("a liveness beat never recreates a record another host deleted (no ghost resurrection)", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const item = makeItem({ id: "ghost-beat", status: "active" }) + await store.upsert(item) + const filePath = path.join(tmpDir, "tasks", "ghost-beat", GlobalFileNames.historyItem) + await expect(fs.access(filePath)).resolves.toBeUndefined() + // Peer deletes the record; the stale cache entry still says active. + await fs.rm(filePath) + const callsBefore = safeWriteJsonMock.mock.calls.length + await store.recordTaskActivity("ghost-beat") + // The beat attempted the locked write but the merge aborted it: the + // file must stay deleted, and the stale cache/claim went with it. + expect(safeWriteJsonMock.mock.calls.length).toBe(callsBefore + 1) + await expect(fs.access(filePath)).rejects.toThrow() + expect(store.get("ghost-beat")).toBeUndefined() + // A later beat has no cached record to refresh and writes nothing. + await store.recordTaskActivity("ghost-beat") + expect(safeWriteJsonMock.mock.calls.length).toBe(callsBefore + 1) + await expect(fs.access(filePath)).rejects.toThrow() + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping liveness heartbeat for ghost-beat")) + warnSpy.mockRestore() + }) + + it("a liveness beat writes nothing for a non-active task", async () => { + const item = makeItem({ id: "quiet-completed", status: "active" }) + await store.upsert(item) + await store.upsert({ ...item, status: "completed" }) + const filePath = path.join(tmpDir, "tasks", "quiet-completed", GlobalFileNames.historyItem) + const before = await fs.readFile(filePath, "utf8") + const callsBefore = safeWriteJsonMock.mock.calls.length + await store.recordTaskActivity("quiet-completed") + // No write of any kind: history_item.json and globalState stay untouched + // for records that merely linger before disposal. + expect(safeWriteJsonMock.mock.calls.length).toBe(callsBefore) + expect(await fs.readFile(filePath, "utf8")).toBe(before) + const persisted = JSON.parse(before) as HistoryItem + expect(persisted.lastActivityAt).toBeUndefined() + }) + + it("a dispose-style ownership release is not re-claimed by a late active-status teardown write", async () => { + const item = makeItem({ id: "ghost-claim", status: "active" }) + await store.upsert(item) + const internals = store as unknown as { + locallyActiveTaskIds: Set + releasedLocalOwnershipIds: Set + } + expect(internals.locallyActiveTaskIds.has("ghost-claim")).toBe(true) + // Task.dispose releases the claim after stopping the heartbeat. + store.markLocallyInactive("ghost-claim") + expect(internals.locallyActiveTaskIds.has("ghost-claim")).toBe(false) + expect(internals.releasedLocalOwnershipIds.has("ghost-claim")).toBe(true) + // A late teardown write preserving "active" (an in-flight beat or the + // abort path's final save) must not ghost-reclaim the id. + await store.upsert({ ...item }) + expect(internals.locallyActiveTaskIds.has("ghost-claim")).toBe(false) + // A new session's explicit eager claim clears the release and re-claims. + store.markLocallyActive("ghost-claim") + expect(internals.releasedLocalOwnershipIds.has("ghost-claim")).toBe(false) + await store.upsert({ ...item }) + expect(internals.locallyActiveTaskIds.has("ghost-claim")).toBe(true) + }) + + it("aborts the orphan repair under the lock when the child's disk record shows a peer transition", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const child = makeItem({ + id: "child-lock-toctou", + status: "active", + parentTaskId: "parent-lock-toctou", + rootTaskId: "parent-lock-toctou", + }) + const parent = makeItem({ + id: "parent-lock-toctou", + status: "delegated", + awaitingChildId: child.id, + delegatedToId: child.id, + childIds: [child.id], + }) + await seedItems(tmpDir, [parent, child]) + // Fresh seed mtimes: the startup pass treats the child as live and + // leaves the pair untouched, so the cache still says "active" when + // the peer transition below lands on disk. + await store.initialize() + // A peer transitions the child on disk WITHOUT this store's cache + // knowing. Only the under-lock status guard can catch this: the mtime + // is aged so the freshness check passes. + const childFilePath = path.join(tmpDir, "tasks", "child-lock-toctou", GlobalFileNames.historyItem) + await fs.writeFile(childFilePath, JSON.stringify({ ...child, status: "interrupted" }, null, "\t")) + await markStaleMtime("child-lock-toctou") + const internals = store as unknown as { + repairActiveDelegation: (parent: HistoryItem, child: HistoryItem) => Promise + } + const repaired = await internals.repairActiveDelegation( + store.get("parent-lock-toctou")!, + store.get("child-lock-toctou")!, + ) + expect(repaired).toBe(false) + expect(store.get("parent-lock-toctou")?.status).toBe("delegated") + const persistedParent = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", "parent-lock-toctou", GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedParent.status).toBe("delegated") + // The durable intent stays for a later pass; nothing was stomped. + await expect(fs.access(path.join(tmpDir, "tasks", GlobalFileNames.delegationRepairIntent))).resolves.toBeUndefined() + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Aborting repair for child child-lock-toctou")) + warnSpy.mockRestore() + }) + + it("aborts the missing-child repair under the lock when the child's file appears (peer landed it)", async () => { + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const parent = makeItem({ + id: "parent-mc-lock", + status: "delegated", + awaitingChildId: "child-mc-lock", + delegatedToId: "child-mc-lock", + childIds: ["child-mc-lock"], + }) + await store.upsert(parent) + await markStaleMtime("parent-mc-lock") + // The child record lands on disk (a peer) while this store's cache has + // no child entry — the repair must re-verify absence under the lock. + const childDir = path.join(tmpDir, "tasks", "child-mc-lock") + await fs.mkdir(childDir, { recursive: true }) + await fs.writeFile( + path.join(childDir, GlobalFileNames.historyItem), + JSON.stringify( + makeItem({ id: "child-mc-lock", status: "active", parentTaskId: "parent-mc-lock", rootTaskId: "parent-mc-lock" }), + null, + "\t", + ), + ) + const internals = store as unknown as { + reconcileDelegationStateCore: (persistedActiveIds: ReadonlySet) => Promise + } + await internals.reconcileDelegationStateCore(new Set(["child-mc-lock"])) + expect(store.get("parent-mc-lock")?.status).toBe("delegated") + const persistedParent = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", "parent-mc-lock", GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedParent.status).toBe("delegated") + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Aborting repair for delegation parent-mc-lock")) + warnSpy.mockRestore() + }) + it("repairs interrupted handoff: delegated parent with completed child → active", async () => { const child = makeItem({ id: "child-2", @@ -197,7 +761,7 @@ describe("TaskHistoryStore reconcileDelegationState", () => { awaitingChildId: "child-2", delegatedToId: "child-2", }) - await seedItems([parent, child]) + await seedItems(tmpDir, [parent, child]) await store.initialize() @@ -212,7 +776,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() @@ -235,7 +799,8 @@ 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() @@ -275,6 +840,423 @@ describe("TaskHistoryStore reconcileDelegationState", () => { expect(persistedParent.delegatedToId).toBeUndefined() }) + 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 → 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) + }) + + it("skips repair for active child with recent mtime (live in another window)", async () => { + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + 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(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. + 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") + + // 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", + 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(tmpDir, [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() + + // 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("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 { + // The under-lock guard's sync probe must see the same undefined signal. + spySyncMtimeProbe((fp, real) => + fp === path.join(tmpDir, "tasks", "child-undef-mtime", GlobalFileNames.historyItem) ? undefined : real(fp), + ) + 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(tmpDir, [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 + // 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(tmpDir, [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, "warn").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(tmpDir, [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 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, "warn").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(tmpDir, [parent, child]) + await setChildMtimeAge("child-epoch-mtime", FIXED_NOW - 1_000) // store observes mtime 1970-01-01T00:00:01.000Z + + 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. 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, "warn").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(tmpDir, [parent, child]) + await setChildMtimeAge("child-future-mtime", -100_000) // store observes a mtime 100s in the future + + 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, "warn").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(tmpDir, [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 () => { const child = makeItem({ id: "child-implicit-active", @@ -287,7 +1269,8 @@ 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() @@ -310,7 +1293,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( @@ -347,7 +1330,8 @@ 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)) throw new Error("fault before child write") @@ -384,7 +1368,8 @@ 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)) throw new Error("fault before parent write") @@ -417,7 +1402,8 @@ 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( new TaskHistoryStore(tmpDir, { onWrite: vi.fn().mockRejectedValue(new Error("fault before cleanup")) }), @@ -447,7 +1433,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, @@ -473,9 +1459,12 @@ 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 + // 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 { @@ -489,7 +1478,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 })) @@ -509,7 +1498,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))) @@ -538,7 +1527,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))) @@ -571,7 +1560,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( @@ -602,58 +1591,231 @@ describe("TaskHistoryStore reconcileDelegationState", () => { ).toBe(true) }) - 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 + 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-5", + id: "parent-replay-live", status: "delegated", - delegatedToId: "stale-child", - awaitingChildId: "", + awaitingChildId: child.id, + delegatedToId: child.id, }) - await seedItems([parent]) - - await store.initialize() - - const repaired = store.get("parent-5") - expect(repaired?.status).toBe("active") - expect(repaired?.delegatedToId).toBeUndefined() - // Fix #4: falsy awaitingChildId must also be cleared - expect(repaired?.awaitingChildId).toBeUndefined() - }) + 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))) - 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]) + // 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() - expect(store.get("task-active")?.status).toBe("active") - expect(store.get("task-completed")?.status).toBe("completed") - }) + // 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) - it("repairs multiple delegated parents in a single initialize()", async () => { - 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]) + 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) - await store.initialize() + // 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. The reason + // names the actual liveness signal (fresh mtime here), not a hardcoded label. + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("child live in another window (mtime")) - expect(store.get("parent-a")?.status).toBe("active") - expect(store.get("parent-b")?.status).toBe("active") + warnSpy.mockRestore() }) - it("repairs an orphaned link in a chained delegation without repairing its grandparent", async () => { - // C doesn't exist (orphaned). B is delegated waiting for C → repaired to active. - // A sees B as delegated in the persisted startup snapshot and remains delegated. - const parentA = makeItem({ id: "parent-a-chain", status: "delegated", awaitingChildId: "parent-b-chain" }) - const parentB = makeItem({ - id: "parent-b-chain", + 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: "missing-child-chain", + awaitingChildId: child.id, + delegatedToId: child.id, }) - await seedItems([parentA, parentB]) + 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))) + 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(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))) + 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 () => { + // 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 + } + mtimeSpy = vi.spyOn(probe, "getChildFileMtimeMs").mockResolvedValue(undefined) + // The under-lock guard's sync probe must see the same unreadable signal. + spySyncMtimeProbe(() => 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(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))) + + 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({ + id: "parent-5", + status: "delegated", + delegatedToId: "stale-child", + awaitingChildId: "", + }) + await seedItems(tmpDir, [parent]) + + await store.initialize() + + const repaired = store.get("parent-5") + expect(repaired?.status).toBe("active") + expect(repaired?.delegatedToId).toBeUndefined() + // Fix #4: falsy awaitingChildId must also be cleared + expect(repaired?.awaitingChildId).toBeUndefined() + }) + + 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(tmpDir, [active, completed]) + + await store.initialize() + + expect(store.get("task-active")?.status).toBe("active") + expect(store.get("task-completed")?.status).toBe("completed") + }) + + it("repairs multiple delegated parents in a single initialize()", async () => { + 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(tmpDir, [childA, parentA, parentB]) + // parentB hits the missing-child repair path; age its record so it reads + // as a genuinely orphaned delegation rather than a brand-new one. + await markStaleMtime("parent-b") + + await store.initialize() + + expect(store.get("parent-a")?.status).toBe("active") + expect(store.get("parent-b")?.status).toBe("active") + }) + + it("repairs an orphaned link in a chained delegation without repairing its grandparent", async () => { + // C doesn't exist (orphaned). B is delegated waiting for C → repaired to active. + // A sees B as delegated in the persisted startup snapshot and remains delegated. + const parentA = makeItem({ id: "parent-a-chain", status: "delegated", awaitingChildId: "parent-b-chain" }) + const parentB = makeItem({ + id: "parent-b-chain", + status: "delegated", + awaitingChildId: "missing-child-chain", + }) + await seedItems(tmpDir, [parentA, parentB]) + // B hits the missing-child repair path; age its record so the + // brand-new-delegation grace check does not protect a stale orphan. + await markStaleMtime("parent-b-chain") await store.initialize() @@ -688,9 +1850,12 @@ 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 + // the replay/startup liveness guards would skip the repair entirely. + await markStaleMtime(child.id) await store.initialize() @@ -737,7 +1902,8 @@ 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() const afterFirstParent = { ...store.get(parent.id) } @@ -759,7 +1925,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") } @@ -780,7 +1946,10 @@ 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]) + // Aged so the delegation reads as a genuine orphan rather than a + // brand-new delegation (fresh parent record → grace skip). + await markStaleMtime("parent-log") await store.initialize() @@ -796,7 +1965,10 @@ 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]) + // The missing-child repair requires a genuinely old delegation: a freshly + // written parent record gets the brand-new-delegation grace instead. + await markStaleMtime("parent-onwrite") await store.initialize() @@ -807,6 +1979,82 @@ describe("TaskHistoryStore reconcileDelegationState", () => { const repaired = lastCall.find((i) => i.id === "parent-onwrite") expect(repaired?.status).toBe("active") }) + + it("skips repair for an active child with a stale mtime but a fresh lastActivityAt heartbeat (long streaming turn)", async () => { + // The delegated child's owning session streams a turn for minutes without + // writing anything else to its history file, so the file mtime goes stale. + // Only the throttled liveness heartbeat (lastActivityAt) proves the + // session is alive — and this fresh store (extension-host restart, no + // local ownership claims) must accept that signal and keep the link. + const logSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const child = makeItem({ + id: "child-hb-live", + status: "active", + parentTaskId: "parent-hb-live", + rootTaskId: "parent-hb-live", + lastActivityAt: Date.now() - 60_000, + }) + const parent = makeItem({ + id: "parent-hb-live", + status: "delegated", + awaitingChildId: "child-hb-live", + delegatedToId: "child-hb-live", + childIds: ["child-hb-live"], + }) + await seedItems(tmpDir, [parent, child]) + await markStaleMtime("child-hb-live") + + await store.initialize() + + expect(store.get("child-hb-live")?.status).toBe("active") + const preservedParent = store.get("parent-hb-live") + expect(preservedParent?.status).toBe("delegated") + expect(preservedParent?.awaitingChildId).toBe("child-hb-live") + expect(preservedParent?.delegatedToId).toBe("child-hb-live") + + const persistedChild = JSON.parse( + await fs.readFile(path.join(tmpDir, "tasks", "child-hb-live", GlobalFileNames.historyItem), "utf8"), + ) as HistoryItem + expect(persistedChild.status).toBe("active") + expect(persistedChild.lastActivityAt).toBe(child.lastActivityAt) + + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child child-hb-live")) + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("(heartbeat 60s ago)")) + expect(logSpy).not.toHaveBeenCalledWith(expect.stringContaining("Reconciled orphaned active child")) + + logSpy.mockRestore() + }) + + it("repairs an active child whose lastActivityAt heartbeat is also stale (genuine crash orphan)", async () => { + // The owning window heartbeated for a while, then crashed. A heartbeat + // past the liveness threshold is not evidence of life: with both signals + // stale the child must be repaired to interrupted exactly as before the + // heartbeat mechanism existed. + const child = makeItem({ + id: "child-hb-stale", + status: "active", + parentTaskId: "parent-hb-stale", + rootTaskId: "parent-hb-stale", + lastActivityAt: Date.now() - 10 * 60 * 1000, + }) + const parent = makeItem({ + id: "parent-hb-stale", + status: "delegated", + awaitingChildId: "child-hb-stale", + delegatedToId: "child-hb-stale", + childIds: ["child-hb-stale"], + }) + await seedItems(tmpDir, [parent, child]) + await markStaleMtime("child-hb-stale") + + await store.initialize() + + expect(store.get("child-hb-stale")).toMatchObject({ id: "child-hb-stale", status: "interrupted" }) + const repairedParent = store.get("parent-hb-stale") + expect(repairedParent).toMatchObject({ id: "parent-hb-stale", status: "active" }) + expect(repairedParent?.awaitingChildId).toBeUndefined() + expect(repairedParent?.delegatedToId).toBeUndefined() + }) }) // ───────────────────────────────────────────────────────────────────────────── @@ -865,16 +2113,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) @@ -888,7 +2126,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() @@ -906,7 +2144,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() @@ -923,7 +2161,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() @@ -934,7 +2172,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() @@ -947,7 +2185,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() @@ -968,7 +2206,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() @@ -979,7 +2217,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() @@ -1016,3 +2254,1344 @@ 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" + + /** + * 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), + ) + const childFilePath = path.join(tmpDir, "tasks", CHILD_ID, GlobalFileNames.historyItem) + spySyncMtimeProbe((fp, real) => (fp === childFilePath ? Date.now() - childAgeMs : real(fp))) + } + + 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 + syncMtimeSpy?.mockRestore() + syncMtimeSpy = 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(tmpDir, [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) + // 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. + 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 quiet-but-alive child across the periodic tick while its lastActivityAt heartbeat stays fresh", async () => { + // Pins the whole-lifetime liveness contract the tick relies on: a child + // running a long tool call or awaiting a user ask writes nothing to its + // history file for minutes (stale mtime), but its owning session keeps + // heartbeating a fresh `lastActivityAt` (Task's liveness heartbeat now + // covers the whole active lifetime, not only streaming). The tick must + // treat the heartbeat as life exactly like the startup pass. + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + const child = makeItem({ + id: CHILD_ID, + status: "active", + parentTaskId: PARENT_ID, + rootTaskId: PARENT_ID, + // Fresh at startup; the tick runs one RECONCILE_INTERVAL_MS later, + // far below the 5-minute liveness threshold, so it stays fresh. + lastActivityAt: Date.now() - 60_000, + }) + const parent = makeItem({ + id: PARENT_ID, + status: "delegated", + awaitingChildId: CHILD_ID, + delegatedToId: CHILD_ID, + childIds: [CHILD_ID], + }) + await seedItems(tmpDir, [parent, child]) + + const s = (store = new TaskHistoryStore(tmpDir)) + useTickClock() + // The child is owned elsewhere and quiet: mtime is long stale at both + // the startup pass and the tick; only the heartbeat says alive. + installChildAgeInjector() + childAgeMs = 10 * 60 * 1000 + await s.initialize() + + // Startup pass: stale mtime but fresh heartbeat → skip (same guard). + expect(s.get(CHILD_ID)?.status).toBe("active") + expect(s.get(PARENT_ID)?.status).toBe("delegated") + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("Skipping repair for live child child-tick")) + + // The owning session keeps beating once a minute (Task's liveness + // interval across the whole active lifetime), so when the tick lands + // exactly one RECONCILE_INTERVAL_MS (= the 5-minute threshold) after + // startup, the last beat is 60s old — strictly fresh. A static seed + // would sit exactly AT the threshold and read stale. + for (let i = 0; i < 5; i++) { + await vi.advanceTimersByTimeAsync(60_000) + if (i < 4) { + await s.recordTaskActivity(CHILD_ID) + } + } + + const timerState = s as unknown as { reconcileTimer: ReturnType | null } + const timerBeforeTick = timerState.reconcileTimer + await flushUntil(() => timerState.reconcileTimer !== timerBeforeTick, { + label: "periodic tick completed without repairing the heartbeat-alive 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") + expect(s.get(PARENT_ID)?.awaitingChildId).toBe(CHILD_ID) + expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("Reconciled orphaned active child")) + expect(errorSpy).not.toHaveBeenCalled() + + 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), + ) + + // 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 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") + 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, "warn").mockImplementation(() => {}) + const [parent, child] = makeDelegatedPair() + await seedItems(tmpDir, [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) + // 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") + 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(tmpDir, [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(() => {}) + + try { + 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" }), + ) + + // 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() + } + }) +}) + +// ───────────────────────────────────────────────────────────────────────────── +// 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 + + /** + * 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), + ) + const childFilePath = path.join(tmpDir, "tasks", childId, GlobalFileNames.historyItem) + spySyncMtimeProbe((fp, real) => (fp === childFilePath ? Date.now() - 10 * 60 * 1000 : real(fp))) + } + + 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 + syncMtimeSpy?.mockRestore() + syncMtimeSpy = 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(tmpDir, [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(tmpDir, [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 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.) + 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(tmpDir, [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) + // 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 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") + 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("reconcile() drops a peer-removed task id from locallyActiveTaskIds (CodeRabbit review 657db15e)", async () => { + // CodeRabbit finding on PR #1495: the reconcile() eviction loop removed the id + // from cache and taskFileMtimes but leaked it in locallyActiveTaskIds. Locally own + // an active task via a runtime write, remove its history file through the peer path + // (fs.rm of the task dir, NOT store.delete()), run reconcile(), and assert the id + // is gone from the ownership set along with its cache entry. + const s = (store = new TaskHistoryStore(tmpDir)) + await s.initialize() + await s.upsert(makeItem({ id: "evict-own", status: "active" })) + await s.upsert(makeItem({ id: "evict-keep", status: "active" })) + expect(ownedIds(s).has("evict-own")).toBe(true) + expect(ownedIds(s).has("evict-keep")).toBe(true) + + // A peer window removes the task directory; reconcile() must evict the cache + // entry AND the stale local-ownership claim. + await fs.rm(path.join(tmpDir, "tasks", "evict-own"), { recursive: true, force: true }) + await s.reconcile() + + expect(s.get("evict-own")).toBeUndefined() + expect(ownedIds(s).has("evict-own")).toBe(false) + // Untouched task keeps its ownership. + expect(s.get("evict-keep")).toBeDefined() + expect(ownedIds(s).has("evict-keep")).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("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 + // 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 + // 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(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))) + + // 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) + }) + // The under-lock guard's sync probe must mirror the same sequencing: + // the replay's child write re-checks exactly-threshold (NOT live -> + // proceed); any later sync read (startup reconcile) sees recent. + let syncCalls = 0 + spySyncMtimeProbe((fp, real) => { + if (!fp.endsWith(path.join(child.id, GlobalFileNames.historyItem))) { + return real(fp) + } + syncCalls++ + return syncCalls === 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(tmpDir, [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() + }) + + 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. + // + // 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() + + 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) + + 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") + // 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 () => { + // 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() + } + }) + + 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/task-persistence/__tests__/taskLifecycle.spec.ts b/src/core/task-persistence/__tests__/taskLifecycle.spec.ts index 5d3129e023..015278c079 100644 --- a/src/core/task-persistence/__tests__/taskLifecycle.spec.ts +++ b/src/core/task-persistence/__tests__/taskLifecycle.spec.ts @@ -5,6 +5,8 @@ import { completeDelegatedChild, delegateTaskToChild, interruptDelegatedChild, + isDelegatedChildLive, + isLivenessSignalFresh, LifecycleTransitionError, settleRejectedCreateSubtaskAction, } from "../taskLifecycle" @@ -105,6 +107,54 @@ describe("task lifecycle transitions", () => { }) }) +describe("isLivenessSignalFresh", () => { + const NOW = 1_756_886_400_000 + const THRESHOLD = 5 * 60 * 1000 + + it("treats a timestamp inside the threshold as fresh", () => { + expect(isLivenessSignalFresh(NOW - THRESHOLD + 1, NOW, THRESHOLD)).toBe(true) + }) + + it("treats a timestamp exactly at the threshold as stale (strict '<' boundary)", () => { + expect(isLivenessSignalFresh(NOW - THRESHOLD, NOW, THRESHOLD)).toBe(false) + }) + + it("treats an absent timestamp as never fresh", () => { + expect(isLivenessSignalFresh(undefined, NOW, THRESHOLD)).toBe(false) + }) + + it("treats a future timestamp (clock skew, transient-stat sentinel) as fresh", () => { + expect(isLivenessSignalFresh(NOW + THRESHOLD, NOW, THRESHOLD)).toBe(true) + }) +}) + +describe("isDelegatedChildLive", () => { + const NOW = 1_756_886_400_000 + const THRESHOLD = 5 * 60 * 1000 + + it("is live when only the history-file mtime is fresh", () => { + expect(isDelegatedChildLive({}, NOW, THRESHOLD, NOW - 60_000)).toBe(true) + }) + + it("is live when only the persisted heartbeat is fresh (long streaming turn)", () => { + expect(isDelegatedChildLive({ lastActivityAt: NOW - 60_000 }, NOW, THRESHOLD, NOW - 10 * 60 * 1000)).toBe(true) + }) + + it("is live when only the heartbeat is fresh and the file mtime is unreadable", () => { + expect(isDelegatedChildLive({ lastActivityAt: NOW - 60_000 }, NOW, THRESHOLD, undefined)).toBe(true) + }) + + it("is not live when both signals are stale (genuine crash orphan)", () => { + expect( + isDelegatedChildLive({ lastActivityAt: NOW - 10 * 60 * 1000 }, NOW, THRESHOLD, NOW - 10 * 60 * 1000), + ).toBe(false) + }) + + it("is not live when both signals are absent", () => { + expect(isDelegatedChildLive({}, NOW, THRESHOLD, undefined)).toBe(false) + }) +}) + describe("settleRejectedCreateSubtaskAction", () => { const createSubtaskAction = { kind: "create_subtask" as const, diff --git a/src/core/task-persistence/taskLifecycle.ts b/src/core/task-persistence/taskLifecycle.ts index b8a5e571f0..20d1474128 100644 --- a/src/core/task-persistence/taskLifecycle.ts +++ b/src/core/task-persistence/taskLifecycle.ts @@ -130,3 +130,42 @@ export function abandonDelegatedChild( }, } } + +/** + * Returns true when a liveness timestamp is fresh relative to `now`: present + * and less than `thresholdMs` old. Timestamps in the future (clock skew, or + * the future-shifted sentinel used for transient stat failures) count as + * fresh because `now - timestamp` is negative. An absent timestamp is never + * fresh — callers must treat "no signal" as "not evidence of life". + */ +export function isLivenessSignalFresh(timestamp: number | undefined, now: number, thresholdMs: number): boolean { + return timestamp !== undefined && now - timestamp < thresholdMs +} + +/** + * Cross-instance liveness decision for an active delegated child, owned by + * the shared reconciliation guard in `TaskHistoryStore` (startup/periodic + * orphan repair and repair-intent replay). + * + * A child is live when EITHER signal is fresh within `thresholdMs`: + * - its history file mtime (`fileMtimeMs`) — another window is persisting it; + * - its persisted `lastActivityAt` heartbeat — the owning session is running + * a long turn that legitimately writes nothing else to the history file + * (minutes of model streaming between saves). + * + * Requiring both signals to be stale before repair is what keeps a live + * child streamed by another window (or by this window before an + * extension-host restart wiped the local ownership claim) from being + * misjudged as a crash orphan and cut out of its delegation link. + */ +export function isDelegatedChildLive( + child: Pick, + now: number, + thresholdMs: number, + fileMtimeMs?: number, +): boolean { + return ( + isLivenessSignalFresh(fileMtimeMs, now, thresholdMs) || + isLivenessSignalFresh(child.lastActivityAt, now, thresholdMs) + ) +} diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 4de2b84590..dc2044b60b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -65,6 +65,7 @@ import { CloudService } from "@roo-code/cloud" import { ApiHandler, ApiHandlerCreateMessageMetadata, buildApiHandler } from "../../api" import { ApiStream, GroundingSource } from "../../api/transform/stream" import { maybeRemoveImageBlocks } from "../../api/transform/image-cleaning" +import { type ReasoningDetail } from "../../api/transform/openai-format" import { OutputTokenLimitError } from "../../api/providers/utils/output-token-limit-error" // shared @@ -269,6 +270,75 @@ type AssistantMessagePersistenceCancellation = { resolve: () => void } +/** + * OpenAI Responses API reasoning summary element (e.g. `{ type: "summary_text", text }`). + * Derived from the installed `openai` SDK types rather than restated locally. + */ +type ReasoningSummaryItem = NonNullable[number] + +/** + * Reasoning content block stored at the head of an assistant message's `content` array by + * OpenAI-family providers. It is not part of the Anthropic `ContentBlockParam` union, so the + * conversation-history builder handles it as a parallel block variant. + */ +type ReasoningContentBlockParam = { + type: "reasoning" + id?: string + summary?: ReasoningSummaryItem[] + encrypted_content?: string + text?: string +} + +/** A `ReasoningContentBlockParam` whose encrypted payload is confirmed present. */ +type EncryptedReasoningContentBlockParam = ReasoningContentBlockParam & { encrypted_content: string } + +function asEncryptedReasoningContentBlockParam( + block: { type?: string } | undefined, +): EncryptedReasoningContentBlockParam | undefined { + if (!block || block.type !== "reasoning") { + return undefined + } + const candidate = block as ReasoningContentBlockParam + return typeof candidate.encrypted_content === "string" ? (candidate as EncryptedReasoningContentBlockParam) : undefined +} + +function asPlainTextReasoningContentBlockParam( + block: { type?: string } | undefined, +): (ReasoningContentBlockParam & { text: string }) | undefined { + if (!block || block.type !== "reasoning") { + return undefined + } + const candidate = block as ReasoningContentBlockParam + return typeof candidate.text === "string" ? (candidate as ReasoningContentBlockParam & { text: string }) : undefined +} + +type ReasoningItemForRequest = { + type: "reasoning" + encrypted_content: string + id?: string + summary?: ReasoningSummaryItem[] +} + +/** Assistant message carrying OpenRouter-style reasoning details (Gemini 3, etc.). */ +type MessageParamWithReasoningDetails = Anthropic.Messages.MessageParam & { + reasoning_details?: ReasoningDetail[] +} + +/** Entry shape produced by `buildCleanConversationHistory`: regular messages plus the two reasoning variants. */ +type CleanConversationHistoryEntry = + | Anthropic.Messages.MessageParam + | ReasoningItemForRequest + | MessageParamWithReasoningDetails + +/** + * Error shape providers surface during retries: an optional HTTP status plus an optional + * Google-RPC-style `errorDetails` array (e.g. the RetryInfo entry sent on HTTP 429). + */ +interface BackoffApiError extends Error { + status?: number + errorDetails?: { "@type"?: string; retryDelay?: string }[] +} + export class PendingActionSettlementError extends Error { constructor(message: string, options?: ErrorOptions) { super(message, options) @@ -479,6 +549,20 @@ export class Task extends EventEmitter implements TaskLike { private disposalPromise?: Promise private diffReversionPromise: Promise = Promise.resolve() + /** + * Delegated-child liveness heartbeat. While a child task streams a turn its + * history file can legitimately go quiet for minutes (no saves between long + * model generations), which used to let startup/periodic reconciliation in + * another window — or this window after an extension-host restart, before + * any local-ownership claim — misjudge the live child as a crash orphan and + * repair it to `interrupted`, severing the delegation link. A throttled + * `lastActivityAt` write is the durable signal that the owning session is + * still alive. One minute keeps the signal comfortably inside the store's + * 5-minute liveness threshold even with jittered ticks. + */ + private static readonly LIVENESS_HEARTBEAT_INTERVAL_MS = 60 * 1000 + private livenessHeartbeatInterval?: NodeJS.Timeout + // Checkpoints enableCheckpoints: boolean checkpointTimeout: number @@ -766,16 +850,22 @@ export class Task extends EventEmitter implements TaskLike { if (startTask) { this._started = true this.startIdleTelemetryCheck() + // Validate BEFORE starting the liveness heartbeat: the throw branch + // is unreachable in production (the constructor guard above fires + // first), but if it ever did fire it must not leak the unref'd + // heartbeat interval. + if (!task && !images && !historyItem) { + throw new Error("Either historyItem or task/images must be provided") + } + this.startLivenessHeartbeat() if (task || images) { void this.startTask(task, images).catch((error) => { console.error("[Task#constructor] startTask failed:", error) }) - } else if (historyItem) { + } else { void this.resumeTaskFromHistory().catch((error) => { console.error("[Task#constructor] resumeTaskFromHistory failed:", error) }) - } else { - throw new Error("Either historyItem or task/images must be provided") } } } @@ -2767,6 +2857,7 @@ export class Task extends EventEmitter implements TaskLike { } this._started = true this.startIdleTelemetryCheck() + this.startLivenessHeartbeat() const { task, images } = this.metadata @@ -2792,6 +2883,7 @@ export class Task extends EventEmitter implements TaskLike { } this._started = true this.startIdleTelemetryCheck() + this.startLivenessHeartbeat() const { task, images } = this.metadata @@ -3336,6 +3428,22 @@ export class Task extends EventEmitter implements TaskLike { console.error("Error flushing shutdown telemetry:", error) } + // A disposed task must stop claiming liveness: a trailing heartbeat + // would keep reconciliation from repairing a genuinely orphaned child. + this.stopLivenessHeartbeat() + + // Release the local session ownership claim: a disposed task no longer + // runs in this window, and a stale `locallyActiveTaskIds` entry would + // silently exclude the id from in-window orphan repair for the window's + // lifetime. A non-active status write already drops the claim; teardown + // paths that write no status are covered here. Best-effort, like the + // heartbeat: disposal must never fail on the store. + try { + this.providerRef.deref()?.taskHistoryStore.markLocallyInactive(this.taskId) + } catch (error) { + console.warn(`[Task#dispose] Failed to release local ownership for task ${this.taskId}:`, error) + } + // A task being disposed is no longer serving requests: set the same // cancellation state `abortTask()` sets, synchronously before the aborts // below, so the request-construction guard (`abort || abandoned` in @@ -3428,7 +3536,7 @@ export class Task extends EventEmitter implements TaskLike { throw new Error("Provider not available") } - const child = await (provider as any).delegateParentAndOpenChild({ + const child = await provider.delegateParentAndOpenChild({ parentTaskId: this.taskId, message, initialTodos, @@ -3468,6 +3576,13 @@ export class Task extends EventEmitter implements TaskLike { this.isInitialized = true this.emit(RooCodeEventName.TaskActive, this.taskId) + // This is a launch entry like start()/run(): a nested-delegation resume + // re-activates a delegated parent that may itself be an awaited child, + // and the whole-lifetime liveness heartbeat must cover it from this + // point on. Idempotent — a task that never stopped heartbeating keeps + // its interval. + this.startLivenessHeartbeat() + // Load conversation history if not already loaded if (this.apiConversationHistory.length === 0) { this.hydrateApiConversationHistory(await this.getSavedApiConversationHistory()) @@ -3975,7 +4090,7 @@ export class Task extends EventEmitter implements TaskLike { } // Store the ID for native protocol - ;(partialToolUse as any).id = event.id + partialToolUse.id = event.id // Add to content and present this.assistantMessageContent.push(partialToolUse) @@ -3995,7 +4110,7 @@ export class Task extends EventEmitter implements TaskLike { const toolUseIndex = this.streamingToolCallIndices.get(event.id) if (toolUseIndex !== undefined) { // Store the ID for native protocol - ;(partialToolUse as any).id = event.id + partialToolUse.id = event.id // Update the existing tool use with new partial data this.assistantMessageContent[toolUseIndex] = partialToolUse @@ -4388,7 +4503,7 @@ export class Task extends EventEmitter implements TaskLike { if (finalToolUse) { // Store the tool call ID - ;(finalToolUse as any).id = event.id + finalToolUse.id = event.id // Get the index and replace partial with final if (toolUseIndex !== undefined) { @@ -4422,7 +4537,7 @@ export class Task extends EventEmitter implements TaskLike { existingToolUse.nativeArgs = undefined existingToolUse.params = {} // Ensure it has the ID for native protocol - ;(existingToolUse as any).id = event.id + existingToolUse.id = event.id } // Clean up tracking @@ -4921,10 +5036,15 @@ export class Task extends EventEmitter implements TaskLike { })() } - private getCurrentProfileId(state: any): string { + private getCurrentProfileId( + state: Pick | undefined, + ): string { return ( - state?.listApiConfigMeta?.find((profile: any) => profile.name === state?.currentApiConfigName)?.id ?? - "default" + state?.listApiConfigMeta?.find( + // Stryker disable next-line OptionalChaining: equivalent mutant — the find callback only + // runs when state is non-nullish, so removing the inner `?.` cannot change behavior. + (profile) => profile.name === state?.currentApiConfigName, + )?.id ?? "default" ) } @@ -5609,7 +5729,7 @@ export class Task extends EventEmitter implements TaskLike { } // Shared exponential backoff for retries (first-chunk and mid-stream) - private async backoffAndAnnounce(retryAttempt: number, error: any): Promise { + private async backoffAndAnnounce(retryAttempt: number, error: BackoffApiError): Promise { try { const state = await this.providerRef.deref()?.getState() const baseDelay = state?.requestDelaySeconds || 5 @@ -5631,7 +5751,7 @@ export class Task extends EventEmitter implements TaskLike { // Prefer RetryInfo on 429 if present if (error?.status === 429) { const retryInfo = error?.errorDetails?.find( - (d: any) => d["@type"] === "type.googleapis.com/google.rpc.RetryInfo", + (d) => d["@type"] === "type.googleapis.com/google.rpc.RetryInfo", ) const match = retryInfo?.retryDelay?.match?.(/^(\d+)s$/) if (match) { @@ -5692,17 +5812,8 @@ export class Task extends EventEmitter implements TaskLike { private buildCleanConversationHistory( messages: ApiMessage[], requestModelInfo: ModelInfo, - ): Array< - Anthropic.Messages.MessageParam | { type: "reasoning"; encrypted_content: string; id?: string; summary?: any[] } - > { - type ReasoningItemForRequest = { - type: "reasoning" - encrypted_content: string - id?: string - summary?: any[] - } - - const cleanConversationHistory: (Anthropic.Messages.MessageParam | ReasoningItemForRequest)[] = [] + ): CleanConversationHistoryEntry[] { + const cleanConversationHistory: CleanConversationHistoryEntry[] = [] for (const msg of messages) { // Standalone reasoning: send encrypted, skip plain text @@ -5751,26 +5862,22 @@ export class Task extends EventEmitter implements TaskLike { role: "assistant", content: assistantContent, reasoning_details: msgWithDetails.reasoning_details, - } as any) + }) continue } // Embedded reasoning: encrypted (send) or plain text (skip) - const hasEncryptedReasoning = - first && (first as any).type === "reasoning" && typeof (first as any).encrypted_content === "string" - const hasPlainTextReasoning = - first && (first as any).type === "reasoning" && typeof (first as any).text === "string" - - if (hasEncryptedReasoning) { - const reasoningBlock = first as any + const encryptedReasoning = asEncryptedReasoningContentBlockParam(first) + const plainTextReasoning = asPlainTextReasoningContentBlockParam(first) + if (encryptedReasoning) { // Send as separate reasoning item (OpenAI Native) cleanConversationHistory.push({ type: "reasoning", - summary: reasoningBlock.summary ?? [], - encrypted_content: reasoningBlock.encrypted_content, - ...(reasoningBlock.id ? { id: reasoningBlock.id } : {}), + summary: encryptedReasoning.summary ?? [], + encrypted_content: encryptedReasoning.encrypted_content, + ...(encryptedReasoning.id ? { id: encryptedReasoning.id } : {}), }) // Send assistant message without reasoning @@ -5790,7 +5897,7 @@ export class Task extends EventEmitter implements TaskLike { } satisfies Anthropic.Messages.MessageParam) continue - } else if (hasPlainTextReasoning) { + } else if (plainTextReasoning) { // Check if the model's preserveReasoning flag is set, resolved from // the request's threaded model snapshot (same per-request source as // the prompt and tool arrays) rather than a fresh getModel() re-read, @@ -5942,6 +6049,66 @@ export class Task extends EventEmitter implements TaskLike { this.idleTelemetryCheckInterval?.unref?.() } + /** + * Start the delegated-child liveness heartbeat for the task's whole active + * lifetime. No-op for non-child tasks: only an active child awaited by a + * delegated parent is at risk of reconcile orphan-repair, so standalone + * tasks never pay the write cost. Idempotent — a second call while a + * heartbeat is already running leaves the existing interval in place. + * + * The heartbeat intentionally outlives individual streaming rounds: a + * quiet-but-alive child that is running a long tool call or awaiting a + * user ask writes nothing to its history file for minutes, and both the + * peer-window startup pass and the periodic delegation tick would + * otherwise read stale mtime AND stale/absent `lastActivityAt` and + * misrepair the live link (the #1495 bug class). + */ + private startLivenessHeartbeat(): void { + if (this.livenessHeartbeatInterval !== undefined || !this.parentTaskId) { + return + } + this.livenessHeartbeatInterval = setInterval(() => { + void this.recordLivenessHeartbeat() + }, Task.LIVENESS_HEARTBEAT_INTERVAL_MS) + // Don't hold the process open just for this timer. + this.livenessHeartbeatInterval.unref?.() + } + + /** + * Stop the liveness heartbeat. Safe to call when none is running. + */ + private stopLivenessHeartbeat(): void { + if (this.livenessHeartbeatInterval !== undefined) { + clearInterval(this.livenessHeartbeatInterval) + this.livenessHeartbeatInterval = undefined + } + } + + /** + * One heartbeat beat: persist `lastActivityAt` for this task so + * reconciliation sees the session as alive. Skipped once the task was + * cancelled/abandoned — a stale trailing beat must not claim life the + * session no longer has. The store additionally refuses to bump a record + * whose persisted status is not active, so a completed/interrupted child + * is never falsely claimed even before disposal stops this interval. + * Failures are logged, never thrown: a heartbeat must never disturb the + * turn it reports on. + */ + private async recordLivenessHeartbeat(): Promise { + if (this.abort || this.abandoned) { + return + } + const provider = this.providerRef.deref() + if (!provider) { + return + } + try { + await provider.taskHistoryStore.recordTaskActivity(this.taskId) + } catch (error) { + console.warn(`[Task#${this.taskId}] Failed to persist liveness heartbeat:`, error) + } + } + // Getters public get taskStatus(): TaskStatus { diff --git a/src/core/task/__tests__/Task.backoffAndAnnounce.retryInfo.spec.ts b/src/core/task/__tests__/Task.backoffAndAnnounce.retryInfo.spec.ts new file mode 100644 index 0000000000..b3c0b16c5c --- /dev/null +++ b/src/core/task/__tests__/Task.backoffAndAnnounce.retryInfo.spec.ts @@ -0,0 +1,134 @@ +// npx vitest run core/task/__tests__/Task.backoffAndAnnounce.retryInfo.spec.ts + +import { describe, expect, it, vi } from "vitest" + +import { createRateLimitClock } from "../RateLimitClock" +import { Task } from "../Task" + +// Resolve every countdown tick instantly so the observable behavior (the say() +// call sequence) is asserted without real sleeps. Same mock as reasoning-preservation. +vi.mock("delay", () => ({ + __esModule: true, + default: vi.fn().mockResolvedValue(undefined), +})) + +/** + * Structural twin of the private `BackoffApiError` interface in Task.ts: an Error with + * the optional HTTP status and Google-RPC-style errorDetails array (RetryInfo on 429). + * Kept local and exact so the call through the bracket-notation seam type-checks + * against the private parameter without `as any`. + */ +type RetryApiError = Error & { + status?: number + errorDetails?: { "@type"?: string; retryDelay?: string }[] +} + +const RETRY_INFO_TYPE = "type.googleapis.com/google.rpc.RetryInfo" + +/** + * `Task.backoffAndAnnounce` is private; the specs drive it through the same + * bracket-notation prototype seam used by `ask-allowlist-cwd.spec.ts` (no production + * visibility change). The countdown duration is observable: `say("api_req_retry_delayed", + * "\n\nN", undefined, true)` is called once + * per remaining second starting at N = finalDelay, followed by a final non-partial say. + * Asserting the exact first timer value and total call count pins the RetryInfo + * extraction predicate on Task.ts L4931-L4933 (REQ-005 gate: NoCoverage region R4). + */ +function buildTask(): { task: Task; sayMock: ReturnType } { + const task = Object.create(Task.prototype) as Task + task["abort"] = false + task["rateLimitClock"] = createRateLimitClock() + const sayMock = vi.fn().mockResolvedValue(undefined) + task.say = sayMock + // A double assertion is unavoidable here: `providerRef` is a `WeakRef`, + // and the stub is neither a `WeakRef` nor a whole `ClineProvider`. Constructing + // either would drag in the extension host, when `backoffAndAnnounce` only ever calls + // `deref()` and `getState()` on it. getState() returns no requestDelaySeconds, so the + // base backoff is the documented 5 seconds. + task["providerRef"] = { deref: () => ({ getState: async () => ({}) }) } as unknown as Task["providerRef"] + return { task, sayMock } +} + +/** Countdown text stamped with the seconds remaining on each partial say(). */ +function timerOf(call: unknown[]): string | undefined { + const text = call[1] + return typeof text === "string" ? text : undefined +} + +describe("Task.backoffAndAnnounce RetryInfo extraction", () => { + it("uses the provider RetryInfo delay (seconds + 1) on a 429 instead of the exponential backoff", async () => { + const { task, sayMock } = buildTask() + const error: RetryApiError = Object.assign(new Error("rate limited"), { + status: 429, + errorDetails: [{ "@type": RETRY_INFO_TYPE, retryDelay: "7s" }], + }) + + await task["backoffAndAnnounce"](0, error) + + // Correct code: RetryInfo found → exponentialDelay = 7 + 1 = 8 (beats the + // default baseDelay*2^0 = 5). Predicate mutants that fail the lookup + // (ArrowFunction→undefined, condition→false, flipped ===, either string + // literal→"") fall back to 5 and break these assertions. + const calls = sayMock.mock.calls + expect(calls).toHaveLength(9) // 8 countdown ticks + 1 final non-partial + expect(calls[0]).toEqual([ + "api_req_retry_delayed", + "429\nrate limited\n8", + undefined, + true, + ]) + expect(calls[7][1]).toContain("1") + expect(calls[8]).toEqual(["api_req_retry_delayed", "429\nrate limited\n", undefined, false]) + }) + + it("finds the RetryInfo entry even when it is not the first errorDetail", async () => { + const { task, sayMock } = buildTask() + const error: RetryApiError = Object.assign(new Error("rate limited"), { + status: 429, + errorDetails: [ + { "@type": "type.googleapis.com/google.rpc.ErrorInfo", reason: "rateLimitExceeded" }, + { "@type": RETRY_INFO_TYPE, retryDelay: "7s" }, + ], + }) + + await task["backoffAndAnnounce"](0, error) + + // A predicate mutated to `() => true` would stop at the first detail (which has + // no retryDelay) and use the default 5-second backoff; the correct strict + // equality skips it and still extracts 7s → first timer 8. + expect(timerOf(sayMock.mock.calls[0])).toContain("8") + }) + + it("falls back to the default exponential backoff when no errorDetail matches RetryInfo", async () => { + const { task, sayMock } = buildTask() + const error: RetryApiError = Object.assign(new Error("rate limited"), { + status: 429, + errorDetails: [{ "@type": "type.googleapis.com/google.rpc.ErrorInfo", reason: "quota" }], + }) + + await task["backoffAndAnnounce"](0, error) + + // No RetryInfo → ceil(baseDelay 5 * 2^0) = 5 → 5 countdown ticks + final say. + // A condition-mutated-to-true predicate would match the ErrorInfo entry here; + // it has no retryDelay, so this case alone cannot kill it, but it pins the + // no-match behavior the equality/string mutants must invert to survive. + const calls = sayMock.mock.calls + expect(calls).toHaveLength(6) + expect(calls[0][1]).toContain("5") + expect(calls[5]).toEqual(["api_req_retry_delayed", "429\nrate limited\n", undefined, false]) + }) + + it("ignores RetryInfo entirely for non-429 statuses", async () => { + const { task, sayMock } = buildTask() + const error: RetryApiError = Object.assign(new Error("server error"), { + status: 500, + errorDetails: [{ "@type": RETRY_INFO_TYPE, retryDelay: "7s" }], + }) + + await task["backoffAndAnnounce"](0, error) + + // 500 → the 429 gate never calls the predicate → default backoff of 5. + expect(sayMock.mock.calls).toHaveLength(6) + expect(timerOf(sayMock.mock.calls[0])).toContain("5") + }) +}) diff --git a/src/core/task/__tests__/Task.buildCleanConversationHistory.reasoning.spec.ts b/src/core/task/__tests__/Task.buildCleanConversationHistory.reasoning.spec.ts new file mode 100644 index 0000000000..e6b2c3c4bc --- /dev/null +++ b/src/core/task/__tests__/Task.buildCleanConversationHistory.reasoning.spec.ts @@ -0,0 +1,311 @@ +// npx vitest run core/task/__tests__/Task.buildCleanConversationHistory.reasoning.spec.ts + +import { describe, expect, it, vi } from "vitest" + +import type { ModelInfo } from "@roo-code/types" + +import type { ApiMessage } from "../../task-persistence" +import { Task } from "../Task" + +/** + * Focused specs for `Task.buildCleanConversationHistory` and its two narrowing + * helpers (`asEncryptedReasoningContentBlockParam` / `asPlainTextReasoningContentBlockParam`). + * The REQ-005 mutation-gate run flagged this region (Task.ts L226-L248 helpers and the + * L5045-L5102 branch selection) as NoCoverage: no spec had ever driven an assistant + * message whose first content block is a reasoning variant. Every expectation below is + * an exact deep-equality on the rebuilt request so any flipped guard, dropped `??` + * fallback, id-spread change, or branch swap inverts at least one assertion. + */ + +/** + * The reasoning content-block variant is deliberately outside the Anthropic + * `ContentBlockParam` union (that is exactly why `Task.ts` narrows it with the two + * type-guard helpers), and production writes it through the same out-of-band storage + * path (`prepareApiConversationMessage`). The `unknown`-mediated assertion here is the + * only way to carry that intentional out-of-band shape through a test fixture without + * `as any`. + */ +const asApiMessage = (message: Record): ApiMessage => message as unknown as ApiMessage + +/** + * `Task.buildCleanConversationHistory` reads `preserveReasoning` from the + * threaded `requestModelInfo` argument in the plain-text branch. A minimal + * typed double is attached through `unknown` because `ApiHandler` is a large + * class surface; this mirrors the bracket-notation seam used by + * `ask-allowlist-cwd.spec.ts` without changing production visibility. + */ +function buildTask(modelOverrides: Partial = {}): Task { + const task = Object.create(Task.prototype) as Task + const info: ModelInfo = { contextWindow: 16000, supportsPromptCache: true, ...modelOverrides } + task.api = { getModel: () => ({ id: "test-model", info }) } as unknown as Task["api"] + return task +} + +/** + * `Task.buildCleanConversationHistory` resolves `preserveReasoning` from the + * request's threaded model snapshot — the same `info` object this double + * serves through `getModel()`, so passing it as the second argument preserves + * the exact semantics these fixtures were built against. + */ +function buildHistory(task: Task, messages: ApiMessage[]) { + return task["buildCleanConversationHistory"](messages, task.api.getModel().info) +} + +describe("Task.buildCleanConversationHistory: encrypted reasoning first block", () => { + it("splits an assistant message into a reasoning item (with summary/id) plus the stripped message", () => { + const task = buildTask() + const messages = [ + asApiMessage({ role: "user", content: [{ type: "text", text: "question" }] }), + asApiMessage({ + role: "assistant", + content: [ + { + type: "reasoning", + encrypted_content: "enc-1", + id: "rs_1", + summary: [{ type: "summary_text", text: "gist" }], + }, + { type: "text", text: "answer" }, + ], + }), + ] + + // Exact match: the reasoning item must carry the real summary array (a dropped + // `?? []` fallback yields `summary: undefined`; an id-spread mutation drops or + // invents `id`), and the assistant entry must contain only the remaining block's + // text (rest.length === 1 collapse; any branch mutant leaks the raw array here). + expect(buildHistory(task, messages)).toEqual([ + { role: "user", content: [{ type: "text", text: "question" }] }, + { + type: "reasoning", + summary: [{ type: "summary_text", text: "gist" }], + encrypted_content: "enc-1", + id: "rs_1", + }, + { role: "assistant", content: "answer" }, + ]) + }) + + it("defaults the summary to an empty array and omits the id when the block has neither", () => { + const task = buildTask() + const messages = [ + asApiMessage({ + role: "assistant", + content: [ + { type: "reasoning", encrypted_content: "enc-2" }, + { type: "text", text: "answer" }, + { type: "text", text: "tail" }, + ], + }), + ] + + // `summary` must be `[]` (mutants: `??`→`&&` produces `undefined`; + // array-declaration produces a placeholder), and no `id` key at all + // (ObjectLiteral mutant on the conditional spread would add one). + // `toEqual` ignores `undefined`-valued keys, so the key-absence claim is + // pinned separately via `not.toHaveProperty` — a mutant that flattens the + // conditional spread to an unconditional `id: encryptedReasoning.id` + // would pass the deep-equal but fails the property check. + const history = buildHistory(task, messages) + expect(history[0]).not.toHaveProperty("id") + expect(history).toEqual([ + { type: "reasoning", summary: [], encrypted_content: "enc-2" }, + { + role: "assistant", + content: [ + { type: "text", text: "answer" }, + { type: "text", text: "tail" }, + ], + }, + ]) + }) + + it("collapses to empty-string content when the reasoning block is the only content", () => { + const task = buildTask() + const messages = [ + asApiMessage({ role: "assistant", content: [{ type: "reasoning", encrypted_content: "enc-3" }] }), + ] + + // Same id-absence pin as above: the conditionally-spread `id` must not + // appear as an `undefined`-valued key that `toEqual` would ignore. + const history = buildHistory(task, messages) + expect(history[0]).not.toHaveProperty("id") + expect(history).toEqual([ + { type: "reasoning", summary: [], encrypted_content: "enc-3" }, + { role: "assistant", content: "" }, + ]) + }) +}) + +describe("Task.buildCleanConversationHistory: plain-text reasoning first block", () => { + const plainFirstAssistant = (): ApiMessage[] => [ + asApiMessage({ + role: "assistant", + content: [ + { type: "reasoning", text: "visible reasoning" }, + { type: "text", text: "answer" }, + ], + }), + ] + + it("strips the reasoning block when the model does not preserve reasoning", () => { + const task = buildTask({ preserveReasoning: false }) + + expect(buildHistory(task, plainFirstAssistant())).toEqual([{ role: "assistant", content: "answer" }]) + }) + + it("keeps the full content array when the model preserves reasoning", () => { + const task = buildTask({ preserveReasoning: true }) + + // The exact-match array must still contain the reasoning block; a branch mutant + // (e.g. `preserveReasoning === true` flipped) strips it and breaks this. + expect(buildHistory(task, plainFirstAssistant())).toEqual([ + { + role: "assistant", + content: [ + { type: "reasoning", text: "visible reasoning" }, + { type: "text", text: "answer" }, + ], + }, + ]) + }) + + it("treats a missing preserveReasoning flag as false (reasoning stripped)", () => { + const task = buildTask() + + expect(buildHistory(task, plainFirstAssistant())).toEqual([{ role: "assistant", content: "answer" }]) + }) + + it("collapses to empty-string content when the reasoning block is the only content", () => { + const task = buildTask() + const messages = [asApiMessage({ role: "assistant", content: [{ type: "reasoning", text: "solo" }] })] + + expect(buildHistory(task, messages)).toEqual([{ role: "assistant", content: "" }]) + }) +}) + +describe("Task.buildCleanConversationHistory: reasoning-block guard rejection", () => { + it("passes a non-reasoning first block through untouched", () => { + const task = buildTask() + const messages = [ + asApiMessage({ role: "assistant", content: [{ type: "text", text: "plain" }] }), + asApiMessage({ role: "user", content: "next question" }), + ] + + // Default path forwards the stored content verbatim (array stays an array, + // string stays a string): both guards must have returned undefined. + expect(buildHistory(task, messages)).toEqual([ + { role: "assistant", content: [{ type: "text", text: "plain" }] }, + { role: "user", content: "next question" }, + ]) + }) + + it("rejects a non-reasoning first block that carries a stray encrypted_content payload", () => { + const task = buildTask() + const messages = [ + asApiMessage({ + role: "assistant", + content: [{ type: "text", text: "answer", encrypted_content: "stray" }], + }), + ] + + // This pins the type leg of `asEncryptedReasoningContentBlockParam`: if the + // guard is skipped (ConditionalExpression→false mutant), the payload check + // alone would classify the block as encrypted reasoning and split the message + // into a reasoning item + stripped content. Real code passes it through. + expect(buildHistory(task, messages)).toEqual([ + { role: "assistant", content: [{ type: "text", text: "answer", encrypted_content: "stray" }] }, + ]) + }) + + it("passes a reasoning-typed block without a string payload through untouched", () => { + const task = buildTask() + const messages = [ + asApiMessage({ + role: "assistant", + content: [ + { type: "reasoning", summary: [{ type: "summary_text", text: "gist" }] }, + { type: "text", text: "answer" }, + ], + }), + ] + + // Neither `encrypted_content` nor `text` is a string, so both narrowing + // helpers must return undefined and the message keeps its raw content. + expect(buildHistory(task, messages)).toEqual([ + { + role: "assistant", + content: [ + { type: "reasoning", summary: [{ type: "summary_text", text: "gist" }] }, + { type: "text", text: "answer" }, + ], + }, + ]) + }) + + it("passes an assistant message with empty content through untouched", () => { + const task = buildTask() + const messages = [asApiMessage({ role: "assistant", content: [] })] + + // `first` is undefined here, exercising the `!block` leg of both guards. + expect(buildHistory(task, messages)).toEqual([{ role: "assistant", content: [] }]) + }) +}) + +describe("Task.buildCleanConversationHistory: standalone reasoning messages", () => { + it("forwards a standalone encrypted reasoning item and drops plain-text-only ones", () => { + const task = buildTask() + const messages = [ + asApiMessage({ + type: "reasoning", + encrypted_content: "enc-standalone", + id: "rs_s", + summary: [{ type: "summary_text", text: "gist" }], + }), + asApiMessage({ role: "user", content: [{ type: "text", text: "after" }] }), + // No encrypted_content: standalone reasoning is skipped entirely. + asApiMessage({ type: "reasoning", text: "plain standalone" }), + ] + + expect(buildHistory(task, messages)).toEqual([ + { + type: "reasoning", + summary: [{ type: "summary_text", text: "gist" }], + encrypted_content: "enc-standalone", + id: "rs_s", + }, + { role: "user", content: [{ type: "text", text: "after" }] }, + ]) + }) + + it("omits the id key on a standalone reasoning item without one", () => { + const task = buildTask() + const history = buildHistory(task, [asApiMessage({ type: "reasoning", encrypted_content: "enc" })]) + + // Key absence must be pinned explicitly: `toEqual` treats `{ id: undefined }` + // as equal to `{}`, so the standalone item's `...(msg.id ? { id: msg.id } : {})` + // spread needs the property check to kill an unconditional-spread mutant. + expect(history[0]).not.toHaveProperty("id") + expect(history).toEqual([{ type: "reasoning", encrypted_content: "enc" }]) + }) +}) + +describe("Task.buildCleanConversationHistory: reasoning_details (OpenRouter style)", () => { + it("rebuilds the assistant message with reasoning_details and collapsed text content", () => { + const task = buildTask() + const reasoningDetails = [{ id: "resp-1", type: "reasoning", text: "od reasoning" }] + const messages = [ + asApiMessage({ + role: "assistant", + content: [{ type: "text", text: "answer" }], + reasoning_details: reasoningDetails, + }), + ] + + // contentArray is a single text block → collapsed to the string; the embedded + // reasoning guards must never see this message (the `continue` skips them). + expect(buildHistory(task, messages)).toEqual([ + { role: "assistant", content: "answer", reasoning_details: reasoningDetails }, + ]) + }) +}) diff --git a/src/core/task/__tests__/Task.getCurrentProfileId.spec.ts b/src/core/task/__tests__/Task.getCurrentProfileId.spec.ts new file mode 100644 index 0000000000..1a0ada9900 --- /dev/null +++ b/src/core/task/__tests__/Task.getCurrentProfileId.spec.ts @@ -0,0 +1,63 @@ +// npx vitest run core/task/__tests__/Task.getCurrentProfileId.spec.ts + +import { describe, expect, it } from "vitest" + +import type { ExtensionState } from "@roo-code/types" + +import { Task } from "../Task" + +type ProfileState = Pick + +/** + * The method is a pure projection of provider-profile state, so the specs drive it + * directly through the bracket-notation prototype seam the existing Task specs use + * (see `ask-allowlist-cwd.spec.ts`) instead of going through `startNewTask`-driven + * flows that would couple the assertion to unrelated provider machinery. The mutation + * gate (REQ-005 run 3) flagged the *return value* as covered-but-never-asserted: these + * specs pin the exact id in every branch (match, name mismatch, missing list, and + * `undefined` state), which kills the `??`/optional-chaining/predicate/default-string + * mutants on the single body line. + */ +function getCurrentProfileId(state: ProfileState | undefined): string { + const task = Object.create(Task.prototype) as Task + return task["getCurrentProfileId"](state) +} + +describe("Task.getCurrentProfileId", () => { + it("returns the id of the profile whose name matches currentApiConfigName", () => { + const state: ProfileState = { + currentApiConfigName: "alpha", + listApiConfigMeta: [ + { id: "prof-beta", name: "beta" }, + { id: "prof-alpha", name: "alpha" }, + ], + } + + expect(getCurrentProfileId(state)).toBe("prof-alpha") + }) + + it("falls back to the default profile id when no profile name matches", () => { + const state: ProfileState = { + currentApiConfigName: "gamma", + listApiConfigMeta: [{ id: "prof-alpha", name: "alpha" }], + } + + expect(getCurrentProfileId(state)).toBe("default") + }) + + it("falls back to the default profile id when the profile list is missing", () => { + const state: ProfileState = { + currentApiConfigName: "alpha", + listApiConfigMeta: undefined, + } + + expect(getCurrentProfileId(state)).toBe("default") + }) + + it("returns the default profile id without throwing when state is undefined", () => { + // Covers both the `state?.` optional chaining and the `?? "default"` fallback: + // a mutant that strips the chaining throws here, and a mutant that replaces the + // default literal (or flips `??` to `&&`) returns something other than "default". + expect(getCurrentProfileId(undefined)).toBe("default") + }) +}) diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 109eee8dc0..f150737ce4 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -127,6 +127,9 @@ vi.mock("../../task-persistence", async (importOriginal) => { deleteMany: vi.fn().mockResolvedValue(undefined), reconcile: vi.fn().mockResolvedValue(undefined), initialized: Promise.resolve(), + recordTaskActivity: vi.fn().mockResolvedValue(undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), } }), } @@ -2468,4 +2471,112 @@ describe("Task persistence", () => { expect(task.userMessageContent).toEqual([]) }) }) + + describe("delegated-child liveness heartbeat and dispose ownership release", () => { + it("beats for the whole active lifetime, including while NOT streaming (ask-idle or long tool call)", async () => { + const recordSpy = vi + .spyOn(mockProvider.taskHistoryStore, "recordTaskActivity") + .mockResolvedValue(undefined) + // No task/images on either construction: start() then only starts the + // liveness machinery, never the (heavy) task loop — the narrow surface + // this test needs. + const parent = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, startTask: false }) + const child = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + parentTask: parent, + startTask: false, + }) + + vi.useFakeTimers() + try { + child.start() + // The regression gate: the pre-fix beat was suppressed unless + // isStreaming, so a child awaiting a user ask or executing a long + // tool call showed BOTH liveness signals stale and could be + // misrepaired by the peer startup pass or the periodic tick. + expect(child.isStreaming).toBe(false) + + await vi.advanceTimersByTimeAsync(60_000) + expect(recordSpy).toHaveBeenCalledWith(child.taskId) + + // Cancellation silences the beat without waiting for disposal. + child.abort = true + recordSpy.mockClear() + await vi.advanceTimersByTimeAsync(120_000) + expect(recordSpy).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it("never starts a heartbeat for standalone tasks", async () => { + const recordSpy = vi + .spyOn(mockProvider.taskHistoryStore, "recordTaskActivity") + .mockResolvedValue(undefined) + const standalone = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + startTask: false, + }) + + vi.useFakeTimers() + try { + standalone.start() + await vi.advanceTimersByTimeAsync(120_000) + expect(recordSpy).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it("resumeAfterDelegation starts the liveness heartbeat (nested-delegation resume entry)", async () => { + // resumeAfterDelegation ends by continuing the full task loop; stub the + // loop and the history save so this test exercises only the resume + // prologue. Fake timers keep the started interval from leaking. + const parent = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, startTask: false }) + const child = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + parentTask: parent, + startTask: false, + }) + const access = child as unknown as { + initiateTaskLoop: (userContent: unknown[]) => Promise + saveApiConversationHistory: (merge?: boolean) => Promise + livenessHeartbeatInterval: ReturnType | undefined + } + access.initiateTaskLoop = vi.fn().mockResolvedValue(undefined) + access.saveApiConversationHistory = vi.fn().mockResolvedValue(true) + // Pre-seed history so the hydrate path is skipped and the + // environment-details prologue has a user message to annotate. + child.apiConversationHistory.push({ + role: "user", + content: [{ type: "text", text: "parent question" }], + }) + + vi.useFakeTimers() + try { + expect(access.livenessHeartbeatInterval).toBeUndefined() + await child.resumeAfterDelegation() + expect(access.livenessHeartbeatInterval).toBeDefined() + } finally { + vi.useRealTimers() + } + }) + + it("dispose releases the local session ownership claim", async () => { + const releaseSpy = vi.spyOn(mockProvider.taskHistoryStore, "markLocallyInactive") + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + + await task.dispose() + + expect(releaseSpy).toHaveBeenCalledWith(task.taskId) + }) + }) }) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 07772816b4..1a425ceeb1 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -81,6 +81,7 @@ import { WebviewMessage } from "../../shared/WebviewMessage" import { EMBEDDING_MODEL_PROFILES } from "../../shared/embeddingModels" import { ProfileValidator } from "../../shared/ProfileValidator" +import type { DiagnosticData } from "../../integrations/editor/EditorUtils" import { Terminal } from "../../integrations/terminal/Terminal" import { downloadTask, getTaskFileName } from "../../integrations/misc/export-markdown" import { resolveDefaultSaveUri, saveLastExportPath } from "../../utils/export" @@ -499,7 +500,14 @@ export class ClineProvider event: K, listener: (...args: TaskProviderEvents[K]) => void | Promise, ): this { - return super.on(event, listener as any) + // @types/node types the listener slot of a generic-K call as a deferred conditional + // (`K extends keyof T ? ... : never`) that TS will not resolve while K stays generic, + // so even an event-map-shaped listener is rejected against the base method here. + // Asserting the method to the base class's untyped-map signature (`EventEmitter["on"]`, + // whose listener slot is Node's own `(...args: any[]) => void` fallback) is the + // minimal type-only workaround; the assertion is erased at compile time, so the + // emitted runtime call remains exactly `super.on(event, listener)`. + return (super.on as EventEmitter["on"])(event, listener) as this } /** @@ -509,7 +517,9 @@ export class ClineProvider event: K, listener: (...args: TaskProviderEvents[K]) => void | Promise, ): this { - return super.off(event, listener as any) + // See the `on` override above for why the assertion through the base signature is + // required; runtime behavior is unchanged (type assertion only). + return (super.off as EventEmitter["off"])(event, listener) as this } /** @@ -953,7 +963,7 @@ export class ClineProvider public async handleCodeAction( command: CodeActionId, promptType: CodeActionName, - params: Record, + params: Record, ): Promise { // Capture telemetry for code action usage TelemetryService.instance.captureCodeActionUsed(promptType) @@ -979,7 +989,7 @@ export class ClineProvider public static async handleTerminalAction( command: TerminalActionId, promptType: TerminalActionPromptType, - params: Record, + params: Record, ): Promise { TelemetryService.instance.captureCodeActionUsed(promptType) @@ -1362,57 +1372,83 @@ export class ClineProvider diffFuzzyThreshold, }) - 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}`, - ) - } + // 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) + + // 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 onError + // hook (which also runs the failed-task cleanup for settlement + // rejections). 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 - // Remove event listeners from the old task - const cleanupFunctions = this.taskEventListeners.get(oldTask) - if (cleanupFunctions) { - cleanupFunctions.forEach((cleanup) => cleanup()) - this.taskEventListeners.delete(oldTask) - } + 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}`, + ) + } - // Replace in-place: preserves stack index and current pointer - this.taskRegistry.replace(oldTask.taskId, task) - } + // Remove event listeners from the old task + const cleanupFunctions = this.taskEventListeners.get(oldTask) + if (cleanupFunctions) { + cleanupFunctions.forEach((cleanup) => cleanup()) + this.taskEventListeners.delete(oldTask) + } - task.emit(RooCodeEventName.TaskFocused) + // Replace in-place: preserves stack index and current pointer + this.taskRegistry.replace(oldTask.taskId, task) + } - // Perform preparation tasks and set up event listeners - await this.performPreparationTasks(task) + task.emit(RooCodeEventName.TaskFocused) - this.log( - `[createTaskWithHistoryItem] rehydrated task ${task.taskId}.${task.instanceId} in-place (flicker-free)`, - ) + // Perform preparation tasks and set up event listeners + await this.performPreparationTasks(task) - if (options?.startTask !== false) { - scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem", undefined, (error) => - this.cleanupFailedHistoryTask(task, error), + this.log( + `[createTaskWithHistoryItem] rehydrated task ${task.taskId}.${task.instanceId} in-place (flicker-free)`, ) - } - } else { - await this.addClineToStack(task) - this.log( - `[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, - ) + if (options?.startTask !== false) { + scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem", undefined, async (error) => { this.taskHistoryStore.markLocallyInactive(task.taskId); await this.cleanupFailedHistoryTask(task, error) }) + } + } else { + await this.addClineToStack(task) - if (options?.startTask !== false) { - scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem", undefined, (error) => - this.cleanupFailedHistoryTask(task, error), + this.log( + `[createTaskWithHistoryItem] ${task.parentTask ? "child" : "parent"} task ${task.taskId}.${task.instanceId} instantiated`, ) + + if (options?.startTask !== false) { + scheduleTask(this.taskScheduler, task, "createTaskWithHistoryItem", undefined, async (error) => { this.taskHistoryStore.markLocallyInactive(task.taskId); await this.cleanupFailedHistoryTask(task, error) }) + } } + } 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 @@ -1737,7 +1773,10 @@ export class ClineProvider } // Only update the task's mode after successful persistence. - ;(task as any)._taskMode = newMode + // `_taskMode` is private on Task; bracket access is the AGENTS.md-sanctioned + // escape hatch for provider-side mutation and emits the same property write as + // the previous `(task as any)._taskMode = newMode`, so runtime behavior is unchanged. + task["_taskMode"] = newMode } catch (error) { // If persistence fails, log the error but don't update the in-memory state. this.log( @@ -1855,7 +1894,7 @@ export class ClineProvider task.updateApiConfiguration(providerSettings) } else { // No rebuild needed, just sync apiConfiguration - ;(task as any).apiConfiguration = providerSettings + task.apiConfiguration = providerSettings } } @@ -3435,8 +3474,14 @@ export class ClineProvider }) await this.addClineToStack(task) + // Claim local session ownership before scheduling so the periodic + // delegation pass cannot treat this not-yet-started task (e.g. a child + // awaited by a delegated parent) as an orphan while the scheduler queues + // the run. Released on scheduler rejection; a later non-active status + // write or dispose also drops the claim. + this.taskHistoryStore.markLocallyActive(task.taskId) if (options.startTask !== false) { - scheduleTask(this.taskScheduler, task, "createTask") + scheduleTask(this.taskScheduler, task, "createTask", undefined, () => this.taskHistoryStore.markLocallyInactive(task.taskId)) } this.log( @@ -3909,9 +3954,8 @@ export class ClineProvider // Non-fatal: proceed with child creation even if parent cleanup had issues } - // 4) Bind the child directly to the delegating task's local provider - // context. Delegation never mutates shared profile/global state. - // Create child as sole active (parent reference preserved for lineage) + // 4) Create child as sole active, bound to the delegating task's local + // provider context (parent reference preserved for lineage) // Pass initialStatus: "active" to ensure the child task's historyItem is created // with status from the start, avoiding race conditions where the task might // call attempt_completion before status is persisted separately. @@ -3922,7 +3966,7 @@ export class ClineProvider // Without this, the child's fire-and-forget startTask() races with step 5, // and the last writer to globalState overwrites the other's changes— // causing the parent's delegation fields to be lost. - const child = await this.createTask(message, undefined, parent as any, { + const child = await this.createTask(message, undefined, parent, { initialTodos, initialStatus: "active", startTask: false, @@ -3967,6 +4011,19 @@ export class ClineProvider } } } catch (err) { + // The child will never start: release the ownership claim createTask + // installed for it (the rollback delete also drops it, but that path + // is best-effort). Best-effort like every cleanup in this catch: a + // minimal store must not mask the original rollback error. + try { + this.taskHistoryStore.markLocallyInactive(child.taskId) + } catch (releaseError) { + this.log( + `[delegateParentAndOpenChild] Failed to release child ${child.taskId} ownership claim during rollback: ${ + (releaseError as Error)?.message ?? String(releaseError) + }`, + ) + } this.log( `[delegateParentAndOpenChild] Failed to persist parent metadata for ${parentTaskId} -> ${child.taskId}: ${ (err as Error)?.message ?? String(err) @@ -4039,7 +4096,9 @@ export class ClineProvider } // 6) Start the child task now that parent metadata is safely persisted. - scheduleTask(this.taskScheduler, child, "delegateParentAndOpenChild") + // The child's ownership claim was installed by createTask in step 4; + // release it only if the scheduler rejects the run. + scheduleTask(this.taskScheduler, child, "delegateParentAndOpenChild", undefined, () => this.taskHistoryStore.markLocallyInactive(child.taskId)) // 7) Emit TaskDelegated (provider-level) try { @@ -4353,7 +4412,16 @@ export class ClineProvider schedulerAdmitted = true admitContinuation() const { runPromise } = await continuation - if (!runPromise) return + if (!runPromise) { + // Admitted, but the continuation declined to resume (the + // parent was cancelled/abandoned or its persisted state + // changed while queued). The eager claim from + // createTaskWithHistoryItem would otherwise linger on a + // task that never runs, excluding the id from orphan + // reconciliation for the life of this window. + this.taskHistoryStore.markLocallyInactive(parentTaskId) + return + } try { await runPromise try { @@ -4368,13 +4436,32 @@ export class ClineProvider throw error } }) - .then(admitContinuation, (error) => { - admitContinuation() - console.error( - `[${ClineProvider.prototype.reopenParentFromDelegation.name}] taskScheduler.schedule failed:`, - error, - ) - }) + .then( + () => { + admitContinuation() + if (!schedulerAdmitted) { + // schedule() resolved without ever invoking the callback + // (the parent was aborted/abandoned while waiting for the + // permit): no resume will run, so release the eager claim. + this.taskHistoryStore.markLocallyInactive(parentTaskId) + } + }, + (error) => { + admitContinuation() + if (!schedulerAdmitted) { + // schedule() rejected before the callback ever ran (the + // permit wait was cancelled): no resume will run, so + // release the eager claim. An admitted resume that later + // fails keeps its claim — its session still lives in this + // window — and that error still reaches this handler. + this.taskHistoryStore.markLocallyInactive(parentTaskId) + } + console.error( + `[${ClineProvider.prototype.reopenParentFromDelegation.name}] taskScheduler.schedule failed:`, + error, + ) + }, + ) } this.cancelledDelegationChildIds.delete(childTaskId) 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 5625beaa54..a2381b17cb 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,8 @@ vi.mock("../../task-persistence", async (importOriginal) => { delete: vi.fn().mockResolvedValue(undefined), deleteMany: vi.fn().mockResolvedValue(undefined), migrateFromGlobalState: vi.fn().mockResolvedValue(undefined), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), } }), readApiMessages: vi.fn().mockResolvedValue([]), 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..6132d9bf9a --- /dev/null +++ b/src/core/webview/__tests__/ClineProvider.markLocallyActive.spec.ts @@ -0,0 +1,1141 @@ +// 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), +// plus the reopenParentFromDelegation continuation-scheduling paths where the +// parent is claimed through createTaskWithHistoryItem(startTask:false) but the +// scheduler never admits the resumed run (latest CodeRabbit review). +// +// 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 { RooCodeEventName } from "@roo-code/types" +import { ClineProvider } from "../ClineProvider" +import { TaskRegistry } from "../../task/TaskRegistry" +import { type Task } from "../../task/Task" +import { LifecycleTransitionError } from "../../task-persistence/taskLifecycle" +import { providerIdentifiers } from "@roo-code/types/provider-identifiers" + +type HistoryItemLike = Parameters[0] + +type PrivateClineProviderMethods = { + createTaskWithHistoryItem: ( + this: unknown, + historyItem: HistoryItemLike, + options?: { startTask?: boolean }, + ) => ReturnType + createTask: ( + this: unknown, + text?: string, + images?: string[], + parentTask?: Task, + options?: { startTask?: boolean }, + ) => Promise + runDelegationTransition: (this: unknown, parentTaskId: string, fn: () => Promise) => Promise + delegateParentAndOpenChild: ( + this: unknown, + params: { + parentTaskId: string + message: string + initialTodos: unknown[] + mode: string + pendingActionId?: string + }, + ) => Promise + reopenParentFromDelegation: ( + this: unknown, + params: { + parentTaskId: string + childTaskId: string + completionResultSummary: string + pendingActionId?: string + }, + ) => Promise +} + +const privateClineProvider = ClineProvider.prototype as unknown as PrivateClineProviderMethods + +// 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" + public parentTask?: unknown + public abort = false + public abandoned = false + public abortTask = vi.fn().mockResolvedValue(undefined) + constructor(opts: { historyItem?: { id: string }; parentTask?: unknown; onCreated?: (t: TaskStub) => void }) { + // 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) + } + run() { + return Promise.resolve() + } + on() {} + off() {} + emit() {} + // Surfaces used by reopenParentFromDelegation's step 8 + continuation body. + public overwriteClineMessages = vi.fn().mockResolvedValue(undefined) + public overwriteApiConversationHistory = vi.fn().mockResolvedValue(undefined) + public resumeAfterDelegation = vi.fn().mockResolvedValue(undefined) + public static instanceCount = 0 + } + return TaskStub +}) + +vi.mock("../../task/Task", () => ({ + Task: TaskStub, +})) + +// reopenParentFromDelegation reads/writes per-task message files through these +// module functions. Echo the passed messages back (production returns the +// saved array) so the continuation tests never touch the filesystem. +vi.mock("../../task-persistence", async (importOriginal) => { + const mod = await importOriginal() + return { + ...mod, + readApiMessages: vi.fn().mockResolvedValue([]), + saveApiMessages: vi + .fn() + .mockImplementation(({ messages }: { messages: unknown[] }) => Promise.resolve(messages)), + saveTaskMessages: vi + .fn() + .mockImplementation(({ messages }: { messages: unknown[] }) => Promise.resolve(messages)), + } +}) +vi.mock("../../task-persistence/taskMessages", async (importOriginal) => { + const mod = await importOriginal() + return { ...mod, readTaskMessages: vi.fn().mockResolvedValue([]) } +}) + +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 + markLocallyInactive: MockFn + invalidate: MockFn + atomicUpdatePair: MockFn + atomicReadAndUpdate: MockFn + clearPendingActionIfMatching: MockFn +} + +type ProviderStubObject = { + historyTaskCreationQueue: Promise + getCurrentTask: MockFn + getTaskWithId: MockFn + cancelledDelegationChildIds: Set + taskCreationCallback?: (task: unknown) => void + runDelegationTransition: (parentTaskId: string, fn: () => Promise) => Promise + // Wired after the literal in makeProvider because the wrapper must be bound to + // the stub object itself; optional so the literal typechecks before wiring. + createTaskWithHistoryItem?: (historyItem: HistoryItemLike, options?: { startTask?: boolean }) => Promise + createTask?: (text?: string, images?: string[], parentTask?: Task, options?: { startTask?: boolean }) => Promise + /** Satisfies the ClineProvider structural interface for `createTask` without being invoked by it. */ + setValues?: MockFn + taskRegistry: TaskRegistry + taskHistoryStore: OwnershipStore + evictCurrentTask: MockFn + removeClineFromStack: MockFn + addClineToStack: MockFn + deleteTaskWithId: MockFn + performPreparationTasks: MockFn + taskScheduler: { schedule: MockFn } + taskEventListeners: Map void>> + log: MockFn + emit: 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(), + invalidate: vi.fn().mockResolvedValue(undefined), + atomicUpdatePair: vi.fn(), + atomicReadAndUpdate: vi.fn().mockResolvedValue(undefined), + clearPendingActionIfMatching: vi.fn(), + } +} + +function makeProvider(store: OwnershipStore, overrides: Partial = {}): ProviderStubObject { + const registry = new TaskRegistry() + const provider: ProviderStubObject = { + historyTaskCreationQueue: Promise.resolve(), + getCurrentTask: vi.fn((...args: unknown[]) => (registry.current as undefined | Task) && registry.current), + getTaskWithId: vi.fn((id: string) => + Promise.resolve({ + historyItem: { + id, + number: 1, + ts: Date.now(), + task: "test task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/tmp", + }, + }), + ), + cancelledDelegationChildIds: new Set(), + runDelegationTransition: (parentTaskId: string, fn: () => Promise): Promise => + privateClineProvider.runDelegationTransition(parentTaskId, fn), + taskRegistry: registry, + taskHistoryStore: store, + evictCurrentTask: vi.fn().mockResolvedValue(undefined), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + addClineToStack: vi.fn().mockResolvedValue(undefined), + deleteTaskWithId: 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(), + emit: 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, + organizationAllowList: { allowAll: true }, + }), + getPendingEditOperation: vi.fn().mockReturnValue(undefined), + clearPendingEditOperation: vi.fn(), + postStateToWebview: vi.fn().mockResolvedValue(undefined), + context: { + extension: { packageJSON: {} }, + globalStorageUri: { fsPath: "/tmp" }, + workspaceState: { get: vi.fn().mockReturnValue(false) }, + }, + contextProxy: { + extensionUri: {}, + globalStorageUri: { fsPath: "/tmp" }, + getValue: vi.fn(), + setValue: vi.fn(), + setProviderSettings: vi.fn(), + getProviderSettings: vi.fn(() => ({})), + }, + ...overrides, + } + // The public wrapper reads this.historyTaskCreationQueue and routes to the + // Unlocked prototype method with `this`, so it must be bound to the stub object. + provider.createTaskWithHistoryItem = (historyItem, options) => + privateClineProvider.createTaskWithHistoryItem.call(provider, historyItem, options) + // delegateParentAndOpenChildUnlocked calls this.createTask internally; bind + // the real prototype method so the child-construction claim is exercised too. + provider.createTask = (text?: string, images?: string[], parentTask?: Task, options?: { startTask?: boolean }) => + privateClineProvider.createTask.call(provider, text, images, parentTask, options) + return provider +} + +function makeHistoryItem(id: string, extra: Partial = {}): HistoryItemLike { + return { + id, + number: 1, + ts: Date.now(), + task: "test task", + tokensIn: 0, + 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 = {}, + { seedListeners = true }: { seedListeners?: boolean } = {}, +) { + 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) + 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 { + // 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) + // 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 () => { + 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) + // 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 () => { + 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")) + + // 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 () => { + 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")) + + // 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("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 + // `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() + }) + + 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 claims ownership before scheduling and releases it when the scheduler rejects", async () => { + // The claim closes the orphan-repair window between stacking the task and + // its first active status write (the periodic delegation pass must never + // treat a not-yet-started child as an orphan); the scheduler-rejection + // hook rolls the claim back so the id is not excluded from in-window + // orphan repair for the window's lifetime. + const store = makeStore() + const provider = makeProvider(store, { + taskScheduler: { schedule: vi.fn().mockRejectedValue(new Error("permit failed")) }, + }) + + const task = await privateClineProvider.createTask.call(provider, "hello") + + // 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) + expect(store.markLocallyActive).toHaveBeenCalledWith(task.taskId) + + await flushMicrotasks() + // The rejection hook ran after the catch logged: claim released exactly once. + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + expect(store.markLocallyInactive).toHaveBeenCalledWith(task.taskId) + + // 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" }), + ) + }) + + it("hookless createTask keeps the ownership claim when the scheduler admits the run", async () => { + // A started task legitimately owns its id until a non-active status + // write or dispose drops the claim — the scheduler path must not + // release it on success. + const store = makeStore() + const provider = makeProvider(store, { + taskScheduler: { + schedule: vi.fn().mockImplementation(async (_task: unknown, run: () => Promise) => run()), + }, + }) + + const task = await privateClineProvider.createTask.call(provider, "hello") + + expect(task).toBeInstanceOf(TaskStub) + expect(store.markLocallyActive).toHaveBeenCalledWith(task.taskId) + await flushMicrotasks() + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) + + it("hookless createTask with startTask:false claims ownership for the caller-scheduled start", async () => { + // The delegation handoff creates its child with startTask:false and + // starts it in a later step; the claim installed here is what protects + // the brand-new delegation from the periodic pass in the meantime. + const store = makeStore() + const provider = makeProvider(store) + + const task = await privateClineProvider.createTask.call(provider, "hello", undefined, undefined, { + startTask: false, + }) + + expect(task).toBeInstanceOf(TaskStub) + expect(provider.taskScheduler.schedule).not.toHaveBeenCalled() + expect(store.markLocallyActive).toHaveBeenCalledWith(task.taskId) + await flushMicrotasks() + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) +}) + +describe("ClineProvider delegateParentAndOpenChild child ownership claim", () => { + let consoleErrorSpy: ReturnType + + beforeEach(() => { + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + }) + + afterEach(() => { + consoleErrorSpy.mockRestore() + }) + + /** + * Parent surface read by delegateParentAndOpenChildUnlocked: identity, + * execution-context selectors, and the pre-disposal history flush. Mode + * resolves to the requested mode so the handoff selector takes the + * no-profile-swap path. + */ + function makeParentStub(taskId: string) { + return { + taskId, + instanceId: "parent-inst", + apiConfiguration: { apiProvider: providerIdentifiers.anthropic }, + getTaskApiConfigName: vi.fn().mockResolvedValue("profile-1"), + getTaskMode: vi.fn().mockResolvedValue("code"), + flushPendingToolResultsToHistory: vi.fn().mockResolvedValue(true), + } + } + + async function delegate(store: OwnershipStore, provider: ReturnType) { + return privateClineProvider.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "do the child work", + initialTodos: [], + mode: "code", + }) + } + + it("claims the child before the parent delegation write and keeps the claim when the scheduler admits the run", async () => { + const store = makeStore() + const parent = makeParentStub("parent-1") + const provider = makeProvider(store, { + getCurrentTask: vi.fn(() => parent), + }) + + const child = await delegate(store, provider) + + expect(child).toBeInstanceOf(TaskStub) + // Claim installed by createTask (step 4) before the parent was persisted + // as delegated (step 5) — a tick in that gap must see the claim. + expect(store.markLocallyActive).toHaveBeenCalledWith(child.taskId) + expect(provider.taskScheduler.schedule).toHaveBeenCalledTimes(1) + await flushMicrotasks() + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) + + it("releases the child claim when the scheduler rejects the delegated run", async () => { + const store = makeStore() + const parent = makeParentStub("parent-1") + const provider = makeProvider(store, { + getCurrentTask: vi.fn(() => parent), + taskScheduler: { schedule: vi.fn().mockRejectedValue(new Error("permit failed")) }, + }) + + const child = await delegate(store, provider) + + expect(child).toBeInstanceOf(TaskStub) + expect(store.markLocallyActive).toHaveBeenCalledWith(child.taskId) + await flushMicrotasks() + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + expect(store.markLocallyInactive).toHaveBeenCalledWith(child.taskId) + }) + + it("releases the child claim when the parent delegation write rejects the transition", async () => { + // Rollback path: the child never starts, so the claim from createTask + // must not leak (it would exclude the deleted child id from in-window + // orphan repair for the window's lifetime). The rejected delegation + // settles its pending action and reports settlement failure, so the + // parent-restore block is skipped and the assertion stays focused on + // the child claim release. + const store = makeStore() + store.get = vi.fn((id: string) => + id === "parent-1" + ? { status: "interrupted", pendingAction: { kind: "create_subtask", actionId: "action-1" } } + : undefined, + ) + store.atomicReadAndUpdate = vi.fn().mockRejectedValue(new LifecycleTransitionError("rejected delegation")) + store.clearPendingActionIfMatching = vi.fn().mockResolvedValue({ + status: "interrupted", + pendingAction: { kind: "create_subtask", actionId: "action-1" }, + }) + const parent = makeParentStub("parent-1") + const provider = makeProvider(store, { + getCurrentTask: vi.fn(() => parent), + }) + + await expect( + privateClineProvider.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "do the child work", + initialTodos: [], + mode: "code", + pendingActionId: "action-1", + }), + ).rejects.toThrow("rejected delegation") + + expect(store.markLocallyActive).toHaveBeenCalled() + // The settlement ran disk-authoritatively before the error propagated. + expect(store.clearPendingActionIfMatching).toHaveBeenCalledWith("parent-1", "action-1") + await flushMicrotasks() + expect(store.markLocallyInactive).toHaveBeenCalled() + }) +}) + +describe("ClineProvider reopenParentFromDelegation continuation scheduling ownership release", () => { + let consoleErrorSpy: ReturnType + + beforeEach(() => { + consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + }) + + afterEach(() => { + consoleErrorSpy.mockRestore() + }) + + // A store double whose atomicUpdatePair applies the updaters for real, so the + // production completeDelegatedChild reducer produces the parent's `active` + // record that the continuation body re-reads through get()/invalidate(). + function makeDelegationStore(seed: { child: HistoryItemLike; parent: HistoryItemLike }): OwnershipStore { + const items = new Map([ + [seed.child.id, structuredClone(seed.child)], + [seed.parent.id, structuredClone(seed.parent)], + ]) + return { + get: vi.fn((id: string) => items.get(id)), + markLocallyActive: vi.fn(), + markLocallyInactive: vi.fn(), + invalidate: vi.fn().mockResolvedValue(undefined), + atomicUpdatePair: vi.fn( + async ( + firstId: string, + secondId: string, + firstUpdater: (current: HistoryItemLike) => HistoryItemLike, + secondUpdater: (current: HistoryItemLike) => HistoryItemLike, + ) => { + const first = items.get(firstId) + const second = items.get(secondId) + if (!first || !second) { + throw new Error(`atomicUpdatePair: ${first ? secondId : firstId} not found`) + } + items.set(firstId, firstUpdater(structuredClone(first))) + items.set(secondId, secondUpdater(structuredClone(second))) + return [] + }, + ), + atomicReadAndUpdate: vi.fn( + async (taskId: string, updater: (current: HistoryItemLike) => HistoryItemLike) => { + const current = items.get(taskId) + if (!current) { + throw new Error(`atomicReadAndUpdate: ${taskId} not found`) + } + const next = updater(structuredClone(current)) + items.set(taskId, next) + return [] + }, + ), + clearPendingActionIfMatching: vi.fn(), + } + } + + function makeDelegationHistory(parentId: string, childId: string) { + const parent = makeHistoryItem(parentId, { + status: "delegated", + awaitingChildId: childId, + delegatedToId: childId, + childIds: [childId], + }) + const child = makeHistoryItem(childId, { + status: "active", + parentTaskId: parentId, + }) + return { parent, child } + } + + // Captures the scheduled run callback and returns a promise the test settles + // manually, mirroring production where schedule() settles only after run(). + function makeCapturingSchedule() { + let runTask: (() => Promise) | undefined + let resolveSchedule!: (value?: unknown) => void + let rejectSchedule!: (reason?: unknown) => void + const settled = new Promise((resolve, reject) => { + resolveSchedule = resolve + rejectSchedule = reject + }) + const schedule = vi.fn((_task: unknown, run: () => Promise) => { + runTask = run + return settled + }) + return { + schedule, + getRunTask: () => runTask, + resolveSchedule: () => resolveSchedule(), + rejectSchedule: (reason: unknown) => rejectSchedule(reason), + } + } + + function makeReopenFixture(schedule: MockFn) { + const ids = { parentId: "parent-1", childId: "child-1" } + const { parent, child } = makeDelegationHistory(ids.parentId, ids.childId) + const store = makeDelegationStore({ child, parent }) + let created: InstanceType | undefined + const provider = makeProvider(store, { + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parent }), + taskCreationCallback: (task) => { + created = task as InstanceType + }, + }) + // The stack branch's addClineToStack must actually install the created task so + // the continuation body's `currentTask === parentInstance` check sees it. + provider.addClineToStack = vi.fn((task: unknown) => { + provider.taskRegistry.push(task as unknown as Task) + return Promise.resolve() + }) + provider.taskScheduler.schedule = schedule + return { + ids, + store, + provider, + getCreated: () => { + if (!created) throw new Error("taskCreationCallback was never invoked") + return created + }, + } + } + + it("releases the parent's eager claim when the scheduler rejects before the continuation is ever admitted", async () => { + // permit wait cancelled while the continuation was queued: schedule() rejects + // without invoking its callback, so resumeAfterDelegation never runs and the + // eager claim from createTaskWithHistoryItem(startTask:false) must be rolled back. + const schedule = vi.fn().mockRejectedValue(new Error("permit wait cancelled")) + const { ids, store, provider, getCreated } = makeReopenFixture(schedule) + + const result = await privateClineProvider.reopenParentFromDelegation.call(provider, { + parentTaskId: ids.parentId, + childTaskId: ids.childId, + completionResultSummary: "subtask done", + }) + + expect(result).toBe(true) + expect(schedule).toHaveBeenCalledTimes(1) + // Claimed once by step 7, released once by the rejection handler. + expect(store.markLocallyActive).toHaveBeenCalledTimes(1) + expect(store.markLocallyActive).toHaveBeenCalledWith(ids.parentId) + await vi.waitFor(() => expect(store.markLocallyInactive).toHaveBeenCalledWith(ids.parentId)) + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + expect(getCreated().resumeAfterDelegation).not.toHaveBeenCalled() + // The rejection still reaches the tagged console.error diagnostic. + await vi.waitFor(() => + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[reopenParentFromDelegation] taskScheduler.schedule failed:", + expect.objectContaining({ message: "permit wait cancelled" }), + ), + ) + }) + + it("releases the parent's eager claim when the scheduler resolves without ever invoking the continuation (aborted/abandoned while waiting)", async () => { + // TaskScheduler.schedule returns early — permit acquired but task.abort/abandoned + // set while queued — resolving WITHOUT calling run(). The claim must still be + // released even though nothing rejected. + const schedule = vi.fn().mockResolvedValue(undefined) + const { ids, store, provider, getCreated } = makeReopenFixture(schedule) + + const result = await privateClineProvider.reopenParentFromDelegation.call(provider, { + parentTaskId: ids.parentId, + childTaskId: ids.childId, + completionResultSummary: "subtask done", + }) + + expect(result).toBe(true) + expect(store.markLocallyActive).toHaveBeenCalledWith(ids.parentId) + await vi.waitFor(() => expect(store.markLocallyInactive).toHaveBeenCalledWith(ids.parentId)) + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + expect(getCreated().resumeAfterDelegation).not.toHaveBeenCalled() + // Resolution-without-admission is not an error: the tagged console.error stays silent. + await flushMicrotasks() + expect(consoleErrorSpy).not.toHaveBeenCalled() + }) + + it("releases the parent's eager claim when the admitted continuation declines to resume (stale parent)", async () => { + // The scheduler admits the continuation, but the body's staleness guard fires + // (parent abandoned between scheduling and admission) and returns no runPromise. + const capture = makeCapturingSchedule() + const { ids, store, provider, getCreated } = makeReopenFixture(capture.schedule) + + const result = await privateClineProvider.reopenParentFromDelegation.call(provider, { + parentTaskId: ids.parentId, + childTaskId: ids.childId, + completionResultSummary: "subtask done", + }) + expect(result).toBe(true) + expect(store.markLocallyInactive).not.toHaveBeenCalled() + + // Make the parent stale BEFORE invoking the admitted run. + getCreated().abandoned = true + await capture.getRunTask()!() + + expect(getCreated().resumeAfterDelegation).not.toHaveBeenCalled() + expect(provider.log).toHaveBeenCalledWith( + `[reopenParentFromDelegation] Skipping stale parent continuation for ${ids.parentId} after child ${ids.childId}`, + ) + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + expect(store.markLocallyInactive).toHaveBeenCalledWith(ids.parentId) + + // The schedule promise settling afterwards must not release a second time. + capture.resolveSchedule() + await flushMicrotasks() + expect(store.markLocallyInactive).toHaveBeenCalledTimes(1) + }) + + it("keeps the claim when the admitted continuation resumes the parent to completion", async () => { + const capture = makeCapturingSchedule() + const { ids, store, provider, getCreated } = makeReopenFixture(capture.schedule) + + const result = await privateClineProvider.reopenParentFromDelegation.call(provider, { + parentTaskId: ids.parentId, + childTaskId: ids.childId, + completionResultSummary: "subtask done", + }) + expect(result).toBe(true) + + await capture.getRunTask()!() + capture.resolveSchedule() + await flushMicrotasks() + + expect(getCreated().resumeAfterDelegation).toHaveBeenCalledTimes(1) + expect(provider.emit).toHaveBeenCalledWith( + RooCodeEventName.TaskDelegationCompleted, + ids.parentId, + ids.childId, + "subtask done", + ) + expect(provider.emit).toHaveBeenCalledWith(RooCodeEventName.TaskDelegationResumed, ids.parentId, ids.childId) + expect(store.markLocallyActive).toHaveBeenCalledWith(ids.parentId) + expect(store.markLocallyInactive).not.toHaveBeenCalled() + }) + + it("keeps the claim and still reports the error when the admitted resume itself fails", async () => { + // CodeRabbit: rejection propagation from an ADMITTED resume must be preserved — + // the error reaches the tagged console.error handler, but the claim is NOT + // released because the parent session still exists in this window. + const capture = makeCapturingSchedule() + const { ids, store, provider, getCreated } = makeReopenFixture(capture.schedule) + + const result = await privateClineProvider.reopenParentFromDelegation.call(provider, { + parentTaskId: ids.parentId, + childTaskId: ids.childId, + completionResultSummary: "subtask done", + }) + expect(result).toBe(true) + + getCreated().resumeAfterDelegation.mockRejectedValue(new Error("resume blew up")) + await expect(capture.getRunTask()!()).rejects.toThrow("resume blew up") + capture.rejectSchedule(new Error("resume blew up")) + await flushMicrotasks() + + expect(store.markLocallyInactive).not.toHaveBeenCalled() + expect(consoleErrorSpy).toHaveBeenCalledWith( + "[reopenParentFromDelegation] taskScheduler.schedule failed:", + expect.objectContaining({ message: "resume blew up" }), + ) + }) +}) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 908159f7ab..8944785a0c 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -794,11 +794,6 @@ "count": 2 } }, - "core/task/Task.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 17 - } - }, "core/task/__tests__/Task.dispose.test.ts": { "@typescript-eslint/no-explicit-any": { "count": 3 @@ -1024,11 +1019,6 @@ "count": 2 } }, - "core/webview/ClineProvider.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 7 - } - }, "core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts": { "@typescript-eslint/no-explicit-any": { "count": 34