diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 588ffd5204..67aba20ede 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -23,15 +23,20 @@ TLA+/PlusCal or Quint with TLC becomes a better fit when the lifecycle needs tem ## Production mapping -| Model concept | Production concept | -| ------------------------- | ------------------------------------------------------------------------------------ | -| Task record and status | `HistoryItem` persisted by `TaskHistoryStore` | -| `delegate(parent, child)` | `ClineProvider.delegateParentAndOpenChild` | -| `interrupt(child)` | cancellation or eviction through `markDelegatedChildInterrupted` | -| `complete(child)` | `ClineProvider.reopenParentFromDelegation` | -| `abandon(child)` | `ClineProvider.abandonSubtask` | -| Atomic event step | `atomicReadAndUpdate`, `atomicUpdatePair`, and per-parent delegation transition lock | -| Event interleaving | Competing completion, cancellation, abandonment, and new delegation calls | +| 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` | +| Parent refresh and transition lock | `TaskHistoryStore.withTaskFileLock(parentTaskId, ...)` refreshes the authoritative parent under its cross-process file lock; `runDelegationTransition` also serializes one provider's parent transitions | +| Result conversations | `saveTaskMessages` and `saveApiMessages`, using pre-images restored by `restoreConversationFiles` | +| Completion records | Parent-first `atomicUpdatePair(parentTaskId, childTaskId, ...)` with `firstDiskGuard`, exact-child reducer checks, and guarded pre-images | +| Finite live handoff | `whileFirstFileLocked` removes C without another save, creates the resumed parent without starting it, and projects the persisted conversations into that instance | +| Record compensation | `rollbackBothOnCallbackFailure` restores the guarded child and parent pre-images and republishes write-through state | +| Live-task compensation | `runLockedDelegationTransition` invokes `afterUnlockError` to remove a partially installed parent and recreate C after the file-locked transition fails | +| 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. @@ -52,15 +57,19 @@ The same `pnpm lifecycle:model-check` command also runs a second bounded explore There is no production record version or compare-and-swap token today. The model therefore does not invent one. It universally checks host-mutex and file-lock ownership, whole-file delta rejection, disk-field preservation, `childIds` union, and pair write order. Six scenarios, including distinct-task writes from #920 and a second-write pair failure, and all seven phases (`read`, `prepare`, `revalidate`, `commit`, `refresh`, `reject`, and `fail`) must remain reachable without exceeding the state/depth budgets. Positive semantic landmarks additionally require a stale cache beside newer disk state, the first pair write committed while the second is pending, and the same committed prefix retained after the second write fails. -Two desired properties are currently false and remain issue-keyed shortest-witness ratchets rather than silently allowed assertion failures: +Two desired properties remain false in the deliberately generic shared-store abstraction and stay issue-keyed shortest-witness ratchets rather than silently allowed assertion failures: -- [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): an old completion can commit after a newer handoff and clear it because disk revalidation checks status legality, not exact-child ownership. +- [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the retained historical completion path can commit after a newer handoff and clear it because generic disk revalidation checks status legality, not exact-child ownership. The protocol-specific fixed model below covers the production guard and lock added for this case. - [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): after abandonment and cache refresh, a stale live-task save can preserve the new interrupted status while restoring old lineage fields. CI fails if either exact causal witness or violation class changes, a witness disappears without being promoted to a universal invariant, a named semantic landmark or modeled phase becomes unreachable, a new safety violation appears, or exploration truncates. Raw reachable-state totals are printed as diagnostics, not used as ratchets: harmless representation changes can alter them without weakening protocol coverage. The known-unsafe witnesses currently compare exact shortest action sequences. This is intentionally simple and reviewable, but brittle to harmless action renames or serialization refactors. A causal partial-order comparator would reduce that brittleness but would add a second trace-equivalence protocol to maintain. Until that complexity is justified, update an exact witness only after confirming the terminal violation class and required causal ordering are unchanged. +The script then runs a protocol-specific explorer separately in historical unsafe and fixed modes. It projects hosts A and B, old child C, replacement D, the parent transition lock, UI and API result conversations, parent/C/D records, and the finite live handoff. Its explicit steps cover scheduling C's stale completion; completion begin; both conversation writes; both record writes; C removal; parent installation; callback failure; record and conversation compensation; C restoration; and release. The competing B path acquires the parent lock, interrupts C, writes D, changes the parent to await D, installs D, and releases. Unsafe mode intentionally models the former behavior that continued from stale C state without honoring B's parent lock or rechecking exact-child ownership. Fixed mode models `withTaskFileLock` refreshing the authoritative parent and rejecting stale C before any completion write. + +Both runs use `HANDOFF_MAX_DEPTH = 20` and a 25,000-state budget, fail on an unseen successor at the depth frontier, and print state count and maximum reached depth without ratcheting either raw count. Fixed mode checks that every active linked delegated child is the exact child awaited by its parent, established D ownership is monotonic at later lock-free observations, and no partial conversation/record/live bundle is observable without the parent lock. The only coherent observable bundles are original C ownership, completed C with both result conversations and the resumed parent, D ownership, or the exact compensated C pre-image. Partial states are permitted under the lock, and a landmark requires one to be reached. Additional landmarks require a stale completion scheduled after D ownership, stale completion rejection, and successful callback compensation. Unsafe mode retains an exact issue-keyed #1469 witness; fixed mode must exhaust with zero errors. + `TaskHistoryStore.realConcurrency.spec.ts` complements the abstract interleavings with one synchronized integration smoke check through the real `proper-lockfile` and filesystem rename path; broader VS Code E2E remains reserved for restart and extension-host behavior. ## Invariants @@ -75,22 +84,22 @@ The checker currently enforces: 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. -These are safety claims within the documented bounds. The check does not claim liveness, fairness, crash consistency, filesystem-lock correctness, API history correctness, or exhaustive coverage of arbitrary task counts. It also does not distinguish a delayed pre-interruption completion from a legitimate post-resume completion for the same child ID; that requires a persisted attempt/generation token before it can become a sound invariant. +These are safety claims within the documented bounds. The check does not claim liveness or fairness, crash consistency, safety when record or conversation compensation itself fails, consistency for arbitrary filesystem readers that ignore the advisory lock, filesystem-lock implementation correctness, API provider acceptance of the projected history, or exhaustive coverage of arbitrary task counts. It also does not distinguish a delayed pre-interruption completion from a legitimate post-resume completion for the same child ID; that requires a persisted attempt/generation token before it can become a sound invariant. ## Open-issue traceability The following map separates issue observations from the architectural interpretation encoded here. Open issues can change after this document is written; follow each link for current status. -| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | -| ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the parent still awaiting that exact child; a live-linked child must remain owned by its parent. | `completeDelegatedChild` rejects stale authoritative input. The lifecycle explorer checks that reducer rule, while the shared-store explorer reproduces the cross-host stale-cache counterexample with an exact causal witness. | -| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | -| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it, while an Alloy abstraction permits the ordering. | A completion/readiness contract must define whether completion implies restart visibility. This is a liveness/durability boundary, not only a `HistoryItem` safety transition. | Not claimed by this checker. Add a controlled persistence barrier test after the contract decision; move to temporal model checking if eventual readiness and failure handling become protocol guarantees. | -| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | -| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | -| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | -| [#1468](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1468): a late chunk from one request combined tool identity with arguments from another request; rerun passed. | Every stream accumulator needs a request/task generation key, and late events cannot mutate another scope. | Separate protocol. It warrants a parser-scope model or deterministic interleaving test, not an unrelated field in the delegation model. | -| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | +| Issue and directly observed evidence | Derived protocol rule | Production transition and current check | +| ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| [#1469](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1469): the issue report states that a barrier-controlled two-host run reproduced an old child completion clearing a newer handoff 25/25 times. | Completion is conditional on the authoritative parent still awaiting that exact child, and completion and replacement must use the same parent lock through their finite handoffs. | The generic shared-store explorer retains its unchanged historical stale-cache witness. The protocol explorer separately retains an exact unsafe completion/redelegation witness, while fixed mode exhaustively checks authoritative refresh, stale-C rejection before writes, lock-scoped bundle coherence, and D ownership preservation within its bounds. | +| [#1021](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1021): an in-flight `saveClineMessages` can restore parent/root IDs after abandonment cleared them. | Detachment should be monotonic: later lifecycle work must not reattach an abandoned child. | `abandonDelegatedChild` clears both sides. The shared-store explorer proves the detach commit occurs, then reproduces a refreshed-cache delta that preserves interrupted status while restoring stale live-task lineage. | +| [#1453](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1453), under user report [#1279](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1279): CI observed `TaskCompleted` before restart-visible API history once; 120 local repetitions did not reproduce it, while an Alloy abstraction permits the ordering. | A completion/readiness contract must define whether completion implies restart visibility. This is a liveness/durability boundary, not only a `HistoryItem` safety transition. | Not claimed by this checker. Add a controlled persistence barrier test after the contract decision; move to temporal model checking if eventual readiness and failure handling become protocol guarantees. | +| [#921](https://github.com/Zoo-Code-Org/Zoo-Code/issues/921): delegation across parallel tabs lacks coverage for different view-local mode/profile state. | Delegation must bind an explicit immutable execution-context snapshot rather than read whichever view is focused later. | The persisted ownership transition is covered; mode/profile snapshot isolation is outside this state model and belongs in a production adapter/model-based test. | +| [#920](https://github.com/Zoo-Code-Org/Zoo-Code/issues/920): issue analysis identifies a missing cross-instance history-update test and potential lost writes. | Distinct task writes must not overwrite one another, and same-task conflicts need an explicit merge/ownership rule. | The shared-store explorer checks distinct-task writes and same-record independent deltas. Cross-instance store tests retain production API coverage, and the synchronized real-filesystem smoke test exercises the actual lock/write path without claiming exhaustive filesystem proof. | +| [#369](https://github.com/Zoo-Code-Org/Zoo-Code/issues/369) and [#372](https://github.com/Zoo-Code-Org/Zoo-Code/issues/372): planned fan-out keeps a parent live while a child runs and requires completion routing by explicit parent ID, single-writer result readiness, permit release, and orphan cleanup. | Persisted `delegated` status is ownership, not proof that the parent instance is suspended. Completion must route by IDs; scheduler resources and live-instance state need separate invariants. | Nested and sibling lifecycle ownership are covered. Scheduler permits, live/suspended parent selection, orphan cancellation, and single-writer message readiness must be added when fan-out lands; they should not be folded into `HistoryItem` fields prematurely. | +| [#1468](https://github.com/Zoo-Code-Org/Zoo-Code/issues/1468): a late chunk from one request combined tool identity with arguments from another request; rerun passed. | Every stream accumulator needs a request/task generation key, and late events cannot mutate another scope. | Separate protocol. It warrants a parser-scope model or deterministic interleaving test, not an unrelated field in the delegation model. | +| [#612](https://github.com/Zoo-Code-Org/Zoo-Code/issues/612): the CLI copied a status union and omitted `interrupted`. | Lifecycle vocabulary should have one type owner. | `HistoryItemStatus` is derived from `HistoryItem`, and production/checker transitions share `taskLifecycle.ts`; consumers should import rather than copy the union. | The issue-derived cases intentionally map to bug classes rather than issue-specific flags. In particular, stale event ownership, monotonic terminal/detached state, explicit scope, and single-writer boundaries generalize to future concurrent task work. diff --git a/scripts/check-task-store-concurrency.ts b/scripts/check-task-store-concurrency.ts index cf6f5f7d64..3c18ad887b 100644 --- a/scripts/check-task-store-concurrency.ts +++ b/scripts/check-task-store-concurrency.ts @@ -721,3 +721,484 @@ if (missingLandmarks.length) { console.log( `Shared-store model check passed: ${totalStates} states, ${scenarios.length} scenarios, ${commonInvariantNames.length} invariants, ${expectedPhases.length}/${expectedPhases.length} phases reachable, ${Object.keys(semanticLandmarks).length}/${Object.keys(semanticLandmarks).length} landmarks reached`, ) + +type HandoffMode = "unsafe" | "fixed" +type HandoffLockOwner = "A" | "B" +type CompletionPhase = + | "idle" + | "scheduled" + | "begun" + | "ui-written" + | "api-written" + | "parent-record-written" + | "c-record-written" + | "c-removed" + | "parent-installed" + | "callback-failed" + | "records-compensated" + | "conversations-compensated" + | "c-restored" + | "rejected" + | "done" + | "failed" +type ReplacementPhase = "idle" | "locked" | "c-interrupted" | "d-written" | "parent-written" | "d-installed" | "done" +type ParentRecordState = "awaiting-c" | "awaiting-d" | "completed-c" +type CRecordState = "active" | "interrupted" | "completed" +type DRecordState = "missing" | "active" +type ConversationState = "original" | "c-result" +type LiveHandoffState = "c" | "none" | "d" | "parent" + +interface HandoffState { + mode: HandoffMode + lockOwner?: HandoffLockOwner + completionPhase: CompletionPhase + replacementPhase: ReplacementPhase + scheduledOwnership?: "c" | "d" + uiConversation: ConversationState + apiConversation: ConversationState + parentRecord: ParentRecordState + cRecord: CRecordState + dRecord: DRecordState + live: LiveHandoffState + dOwnershipEstablished: boolean + compensationCompleted: boolean +} + +interface HandoffTraceStep { + action: string + state: HandoffState +} + +const HANDOFF_MAX_DEPTH = 20 +const HANDOFF_MAX_STATES = 25_000 +const handoffMechanicalInvariantNames = [ + "parent transition lock ownership", + "completion phase lock discipline", + "replacement phase lock discipline", +] as const +const handoffFixedInvariantNames = [ + ...handoffMechanicalInvariantNames, + "active linked child exact ownership", + "replacement ownership monotonicity", + "lock-free handoff bundle coherence", +] as const +const unsafeHandoffLandmarks = { + "internal partial handoff while locked": (state: HandoffState) => + state.lockOwner !== undefined && !isCoherentHandoff(state), + "stale schedule after D ownership": (state: HandoffState) => + state.dOwnershipEstablished && state.completionPhase === "scheduled" && state.scheduledOwnership === "d", + "successful callback compensation": (state: HandoffState) => state.compensationCompleted, +} satisfies Record boolean> +const fixedHandoffLandmarks = { + ...unsafeHandoffLandmarks, + "stale completion rejection": (state: HandoffState) => state.completionPhase === "rejected", +} satisfies Record boolean> +const expectedUnsafe1469Actions = [ + "handoff.stale-completion.schedule", + "handoff.unsafe-completion.begin", + "handoff.completion.write-UI", + "handoff.completion.write-API", + "handoff.B.acquire-parent-lock", + "handoff.B.interrupt-C", + "handoff.B.write-D", + "handoff.B.write-parent-awaiting-D", + "handoff.B.install-D", + "handoff.B.release", + "handoff.completion.write-parent-record", + "handoff.completion.write-C-record", + "handoff.completion.remove-C", + "handoff.completion.install-parent", + "handoff.completion.release", +] as const + +function initialHandoffState(mode: HandoffMode): HandoffState { + return { + mode, + completionPhase: "idle", + replacementPhase: "idle", + uiConversation: "original", + apiConversation: "original", + parentRecord: "awaiting-c", + cRecord: "active", + dRecord: "missing", + live: "c", + dOwnershipEstablished: false, + compensationCompleted: false, + } +} + +function handoffTransition( + state: HandoffState, + action: string, + mutate: (next: HandoffState) => void, +): HandoffTraceStep { + const next = clone(state) + mutate(next) + return { action, state: next } +} + +function nextHandoffSteps(state: HandoffState): HandoffTraceStep[] { + const steps: HandoffTraceStep[] = [] + + if (state.completionPhase === "idle") { + steps.push( + handoffTransition(state, "handoff.stale-completion.schedule", (next) => { + next.completionPhase = "scheduled" + next.scheduledOwnership = next.parentRecord === "awaiting-d" ? "d" : "c" + }), + ) + } else if (state.completionPhase === "scheduled") { + if (state.mode === "unsafe") { + steps.push( + handoffTransition(state, "handoff.unsafe-completion.begin", (next) => { + next.completionPhase = "begun" + }), + ) + } else if (!state.lockOwner) { + steps.push( + handoffTransition(state, "handoff.fixed-completion.begin", (next) => { + next.lockOwner = "A" + // withTaskFileLock refreshes the authoritative parent before this exact-child guard. + next.completionPhase = + next.parentRecord === "awaiting-c" && next.cRecord !== "completed" ? "begun" : "rejected" + }), + ) + } + } else if (state.completionPhase === "begun") { + steps.push( + handoffTransition(state, "handoff.completion.write-UI", (next) => { + next.uiConversation = "c-result" + next.completionPhase = "ui-written" + }), + ) + } else if (state.completionPhase === "ui-written") { + steps.push( + handoffTransition(state, "handoff.completion.write-API", (next) => { + next.apiConversation = "c-result" + next.completionPhase = "api-written" + }), + ) + } else if (state.completionPhase === "api-written") { + steps.push( + handoffTransition(state, "handoff.completion.write-parent-record", (next) => { + next.parentRecord = "completed-c" + next.completionPhase = "parent-record-written" + }), + ) + } else if (state.completionPhase === "parent-record-written") { + steps.push( + handoffTransition(state, "handoff.completion.write-C-record", (next) => { + next.cRecord = "completed" + next.completionPhase = "c-record-written" + }), + ) + } else if (state.completionPhase === "c-record-written") { + steps.push( + handoffTransition(state, "handoff.completion.remove-C", (next) => { + if (next.live === "c") next.live = "none" + next.completionPhase = "c-removed" + }), + ) + } else if (state.completionPhase === "c-removed") { + steps.push( + handoffTransition(state, "handoff.completion.install-parent", (next) => { + next.live = "parent" + next.completionPhase = "parent-installed" + }), + handoffTransition(state, "handoff.completion.callback-fail", (next) => { + next.completionPhase = "callback-failed" + }), + ) + } else if (state.completionPhase === "parent-installed") { + steps.push( + handoffTransition(state, "handoff.completion.release", (next) => { + if (next.mode === "fixed") delete next.lockOwner + next.completionPhase = "done" + }), + ) + } else if (state.completionPhase === "callback-failed") { + steps.push( + handoffTransition(state, "handoff.completion.compensate-records", (next) => { + next.parentRecord = "awaiting-c" + next.cRecord = "active" + next.completionPhase = "records-compensated" + }), + ) + } else if (state.completionPhase === "records-compensated") { + steps.push( + handoffTransition(state, "handoff.completion.compensate-conversations", (next) => { + next.uiConversation = "original" + next.apiConversation = "original" + next.completionPhase = "conversations-compensated" + }), + ) + } else if (state.completionPhase === "conversations-compensated") { + steps.push( + handoffTransition(state, "handoff.completion.restore-C", (next) => { + next.live = "c" + next.completionPhase = "c-restored" + }), + ) + } else if (state.completionPhase === "c-restored") { + steps.push( + handoffTransition(state, "handoff.completion.release", (next) => { + if (next.mode === "fixed") delete next.lockOwner + next.completionPhase = "failed" + next.compensationCompleted = true + }), + ) + } else if (state.completionPhase === "rejected") { + steps.push( + handoffTransition(state, "handoff.completion.release", (next) => { + delete next.lockOwner + next.completionPhase = "done" + }), + ) + } + + if ( + state.replacementPhase === "idle" && + !state.lockOwner && + state.parentRecord === "awaiting-c" && + state.cRecord === "active" + ) { + steps.push( + handoffTransition(state, "handoff.B.acquire-parent-lock", (next) => { + next.lockOwner = "B" + next.replacementPhase = "locked" + }), + ) + } else if (state.replacementPhase === "locked") { + steps.push( + handoffTransition(state, "handoff.B.interrupt-C", (next) => { + next.cRecord = "interrupted" + if (next.live === "c") next.live = "none" + next.replacementPhase = "c-interrupted" + }), + ) + } else if (state.replacementPhase === "c-interrupted") { + steps.push( + handoffTransition(state, "handoff.B.write-D", (next) => { + next.dRecord = "active" + next.replacementPhase = "d-written" + }), + ) + } else if (state.replacementPhase === "d-written") { + steps.push( + handoffTransition(state, "handoff.B.write-parent-awaiting-D", (next) => { + next.parentRecord = "awaiting-d" + next.replacementPhase = "parent-written" + }), + ) + } else if (state.replacementPhase === "parent-written") { + steps.push( + handoffTransition(state, "handoff.B.install-D", (next) => { + next.live = "d" + next.replacementPhase = "d-installed" + }), + ) + } else if (state.replacementPhase === "d-installed") { + steps.push( + handoffTransition(state, "handoff.B.release", (next) => { + delete next.lockOwner + next.replacementPhase = "done" + if (next.parentRecord === "awaiting-d" && next.dRecord === "active" && next.live === "d") { + next.dOwnershipEstablished = true + } + }), + ) + } + + return steps +} + +function isOriginalCOwnership(state: HandoffState): boolean { + return ( + state.uiConversation === "original" && + state.apiConversation === "original" && + state.parentRecord === "awaiting-c" && + state.cRecord === "active" && + state.dRecord === "missing" && + state.live === "c" + ) +} + +function isCompletedCOwnership(state: HandoffState): boolean { + return ( + state.uiConversation === "c-result" && + state.apiConversation === "c-result" && + state.parentRecord === "completed-c" && + state.cRecord === "completed" && + state.dRecord === "missing" && + state.live === "parent" + ) +} + +function isDOwnership(state: HandoffState): boolean { + return ( + state.uiConversation === "original" && + state.apiConversation === "original" && + state.parentRecord === "awaiting-d" && + state.cRecord === "interrupted" && + state.dRecord === "active" && + state.live === "d" + ) +} + +function isCoherentHandoff(state: HandoffState): boolean { + return isOriginalCOwnership(state) || isCompletedCOwnership(state) || isDOwnership(state) +} + +function handoffMechanicalViolations(state: HandoffState): string[] { + const violations: string[] = [] + const replacementHoldsLock = ["locked", "c-interrupted", "d-written", "parent-written", "d-installed"].includes( + state.replacementPhase, + ) + const completionHoldsLock = [ + "begun", + "ui-written", + "api-written", + "parent-record-written", + "c-record-written", + "c-removed", + "parent-installed", + "callback-failed", + "records-compensated", + "conversations-compensated", + "c-restored", + "rejected", + ].includes(state.completionPhase) + + if (replacementHoldsLock !== (state.lockOwner === "B")) { + violations.push("B replacement phase and parent transition lock ownership disagree") + } + if (state.mode === "fixed" && completionHoldsLock !== (state.lockOwner === "A")) { + violations.push("fixed completion phase and parent transition lock ownership disagree") + } + if (state.mode === "unsafe" && state.lockOwner === "A") { + violations.push("unsafe completion unexpectedly acquired the parent transition lock") + } + return violations +} + +function fixedHandoffViolations(state: HandoffState): string[] { + const violations = handoffMechanicalViolations(state) + if (state.lockOwner) return violations + + if (state.cRecord === "active" && state.parentRecord !== "awaiting-c") { + violations.push("active linked C is not the exact child awaited by the delegated parent") + } + if (state.dRecord === "active" && state.parentRecord !== "awaiting-d") { + violations.push("active linked D is not the exact child awaited by the delegated parent") + } + if ( + state.dOwnershipEstablished && + (state.parentRecord !== "awaiting-d" || state.dRecord !== "active" || state.live !== "d") + ) { + violations.push("established D ownership was not preserved") + } + if (!isCoherentHandoff(state)) violations.push("a partial handoff bundle is observable without the parent lock") + return violations +} + +function isUnsafe1469Violation(state: HandoffState): boolean { + return ( + state.mode === "unsafe" && + state.completionPhase === "done" && + state.replacementPhase === "done" && + state.scheduledOwnership === "c" && + state.dOwnershipEstablished && + state.parentRecord === "completed-c" && + state.dRecord === "active" + ) +} + +function formatHandoffTrace(message: string, trace: HandoffTraceStep[]): string { + return [ + message, + `Bounds: depth=${HANDOFF_MAX_DEPTH}, states=${HANDOFF_MAX_STATES}`, + ...trace.map((step, index) => `${index}. ${step.action}\n${JSON.stringify(step.state, null, 2)}`), + ].join("\n") +} + +function runHandoffExplorer(mode: HandoffMode): { + states: number + maxDepth: number + errors: number + landmarks: Set + witness?: HandoffTraceStep[] +} { + const start = initialHandoffState(mode) + const queue: Array<{ state: HandoffState; trace: HandoffTraceStep[] }> = [ + { state: start, trace: [{ action: "initial", state: start }] }, + ] + const visited = new Set([canonical(start)]) + const frontier: HandoffState[] = [] + const landmarks = new Set() + const landmarkPredicates = mode === "fixed" ? fixedHandoffLandmarks : unsafeHandoffLandmarks + let maxDepth = 0 + let witness: HandoffTraceStep[] | undefined + + for (let index = 0; index < queue.length; index++) { + const node = queue[index]! + const depth = node.trace.length - 1 + maxDepth = Math.max(maxDepth, depth) + for (const [name, predicate] of Object.entries(landmarkPredicates)) { + if (predicate(node.state)) landmarks.add(name) + } + const violations = + mode === "fixed" ? fixedHandoffViolations(node.state) : handoffMechanicalViolations(node.state) + if (violations.length) { + throw new Error( + formatHandoffTrace(`Cross-host ${mode} handoff violation: ${violations.join("; ")}`, node.trace), + ) + } + if (isUnsafe1469Violation(node.state) && !witness) witness = node.trace + if (depth === HANDOFF_MAX_DEPTH) { + frontier.push(node.state) + continue + } + + for (const step of nextHandoffSteps(node.state)) { + const key = canonical(step.state) + if (visited.has(key)) continue + visited.add(key) + queue.push({ state: step.state, trace: [...node.trace, step] }) + if (visited.size > HANDOFF_MAX_STATES) { + throw new Error(`Cross-host ${mode} handoff exceeded ${HANDOFF_MAX_STATES} states`) + } + } + } + + const unseen = frontier + .flatMap((state) => nextHandoffSteps(state)) + .find((step) => !visited.has(canonical(step.state))) + if (unseen) throw new Error(`Cross-host ${mode} handoff truncated before unseen action ${unseen.action}`) + const missingLandmarks = Object.keys(landmarkPredicates).filter((name) => !landmarks.has(name)) + if (missingLandmarks.length) { + throw new Error(`Cross-host ${mode} handoff has unreachable landmarks: ${missingLandmarks.join(", ")}`) + } + if (mode === "unsafe" && !witness) { + throw new Error("Cross-host unsafe handoff no longer reproduces #1469; promote it to an invariant") + } + return { states: visited.size, maxDepth, errors: witness ? 1 : 0, landmarks, witness } +} + +const unsafeHandoff = runHandoffExplorer("unsafe") +const unsafeHandoffActions = unsafeHandoff.witness!.slice(1).map((step) => step.action) +if (canonical(unsafeHandoffActions) !== canonical(expectedUnsafe1469Actions)) { + throw new Error( + formatHandoffTrace("Cross-host unsafe handoff #1469 shortest causal witness changed", unsafeHandoff.witness!), + ) +} +console.log( + `Known unsafe #1469 protocol: stale child completion cleared replacement D ownership\n ${unsafeHandoffActions.join(" -> ")}`, +) +console.log( + `Cross-host unsafe handoff explored: ${unsafeHandoff.states} states, max depth ${unsafeHandoff.maxDepth}, ${unsafeHandoff.errors} expected error, ${handoffMechanicalInvariantNames.length} invariants, ${unsafeHandoff.landmarks.size}/${Object.keys(unsafeHandoffLandmarks).length} landmarks reached`, +) + +const fixedHandoff = runHandoffExplorer("fixed") +console.log( + `Cross-host fixed handoff model check passed: ${fixedHandoff.states} states, max depth ${fixedHandoff.maxDepth}, ${fixedHandoff.errors} errors, ${handoffFixedInvariantNames.length} invariants, ${fixedHandoff.landmarks.size}/${Object.keys(fixedHandoffLandmarks).length} landmarks reached`, +) diff --git a/src/__tests__/delegation-concurrent.spec.ts b/src/__tests__/delegation-concurrent.spec.ts index 40d9b49ee5..1ee3754e02 100644 --- a/src/__tests__/delegation-concurrent.spec.ts +++ b/src/__tests__/delegation-concurrent.spec.ts @@ -20,6 +20,7 @@ vi.mock("fs", () => ({ })) vi.mock("../utils/safeWriteJson", () => ({ + lockJsonFile: vi.fn().mockResolvedValue(async () => {}), safeWriteJson: vi.fn().mockResolvedValue(undefined), })) diff --git a/src/__tests__/helpers/provider-stub.ts b/src/__tests__/helpers/provider-stub.ts index 59dde33933..1a22aa4cf4 100644 --- a/src/__tests__/helpers/provider-stub.ts +++ b/src/__tests__/helpers/provider-stub.ts @@ -6,17 +6,22 @@ type ProviderStubFields = { delegationTransitionLocks?: Map> cancelledDelegationChildIds?: Set log?: ReturnType - taskHistoryStore?: { get: (id: string) => unknown } + taskHistoryStore?: { + get: (id: string) => unknown + withTaskFileLock?: (id: string, callback: () => Promise) => Promise + } taskRegistry?: TaskRegistry clineStack?: Task[] tasks?: Task[] runDelegationTransition?: unknown + runLockedDelegationTransition?: unknown removeClineFromStack?: unknown evictCurrentTask?: unknown } type PrivateProviderMethods = { runDelegationTransition: (this: unknown, ...args: unknown[]) => unknown + runLockedDelegationTransition: (this: unknown, ...args: unknown[]) => unknown removeClineFromStack: (this: unknown, ...args: unknown[]) => unknown evictCurrentTask: (this: unknown, ...args: unknown[]) => unknown } @@ -38,6 +43,7 @@ export function makeProviderStub(stub: T): ClineProvider { s.cancelledDelegationChildIds ??= new Set() s.log ??= vi.fn() s.taskHistoryStore ??= { get: () => undefined } + s.taskHistoryStore.withTaskFileLock ??= async (_id, callback) => callback() // Convert legacy clineStack array into a TaskRegistry if (!s.taskRegistry) { @@ -49,6 +55,7 @@ export function makeProviderStub(stub: T): ClineProvider { delete s.clineStack s.runDelegationTransition ??= proto.runDelegationTransition.bind(s) + s.runLockedDelegationTransition ??= proto.runLockedDelegationTransition.bind(s) s.removeClineFromStack ??= proto.removeClineFromStack.bind(s) s.evictCurrentTask ??= proto.evictCurrentTask.bind(s) return s as unknown as ClineProvider diff --git a/src/__tests__/history-resume-delegation.spec.ts b/src/__tests__/history-resume-delegation.spec.ts index d3a24a3140..bd78e74e0c 100644 --- a/src/__tests__/history-resume-delegation.spec.ts +++ b/src/__tests__/history-resume-delegation.spec.ts @@ -54,6 +54,10 @@ import { readTaskMessages } from "../core/task-persistence/taskMessages" import { readApiMessages, saveApiMessages, saveTaskMessages } from "../core/task-persistence" import { makeProviderStub } from "./helpers/provider-stub" +type LockedDelegationAccess = { + runLockedDelegationTransition: (parentTaskId: string, transition: () => Promise) => Promise +} + /** * Create a minimal taskHistoryStore stub whose atomicUpdatePair calls both updaters * with the provided items and resolves, simulating the happy-path atomic write. @@ -74,16 +78,28 @@ function makeTaskHistoryStoreStub( secondId: string, firstUpdater: (h: HistoryItem) => HistoryItem, secondUpdater: (h: HistoryItem) => HistoryItem, + options?: { + firstDiskGuard?: (item: HistoryItem) => void + whileFirstFileLocked?: () => Promise + firstFileLockAcquired?: boolean + storeLockAcquired?: boolean + rollbackBothOnCallbackFailure?: boolean + }, ) => { - firstUpdater(itemMap.get(firstId) as HistoryItem) + const first = itemMap.get(firstId) as HistoryItem + options?.firstDiskGuard?.(first) + firstUpdater(first) secondUpdater(itemMap.get(secondId) as HistoryItem) + await options?.whileFirstFileLocked?.() return [] }, ) + const withTaskFileLock = vi.fn(async (_id: string, callback: () => Promise) => callback()) return { atomicUpdatePair: overrides.atomicUpdatePair ?? atomicUpdatePair, get: vi.fn((id: string) => itemMap.get(id)), + withTaskFileLock, } } @@ -92,6 +108,27 @@ describe("History resume delegation - parent metadata transitions", () => { vi.clearAllMocks() }) + it("runs locked transitions without optional post-lock callbacks", async () => { + const transitionResult = { completed: true } + const transition = vi.fn().mockResolvedValue(transitionResult) + const provider = makeProviderStub({ + delegationTransitionLocks: new Map(), + taskHistoryStore: { + withTaskFileLock: vi.fn(async (_taskId: string, callback: () => Promise) => callback()), + }, + }) + const lockedProvider = provider as unknown as LockedDelegationAccess + + await expect(lockedProvider.runLockedDelegationTransition("parent-success", transition)).resolves.toBe( + transitionResult, + ) + await expect( + lockedProvider.runLockedDelegationTransition("parent-failure", async () => { + throw new Error("transition failed") + }), + ).rejects.toThrow("transition failed") + }) + it("rejects a stale restored completion action before changing parent or child state", async () => { const parentHistoryItem = { id: "parent-1", @@ -157,8 +194,14 @@ describe("History resume delegation - parent metadata transitions", () => { } const childHistoryItem = { id: "child-1", status: "active", pendingAction: expectedAction } const atomicUpdatePair = vi.fn( - async (_firstId: string, _secondId: string, firstUpdater: (item: HistoryItem) => HistoryItem) => { - firstUpdater({ + async ( + _firstId: string, + _secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + secondUpdater: (item: HistoryItem) => HistoryItem, + ) => { + firstUpdater(parentHistoryItem as HistoryItem) + secondUpdater({ ...childHistoryItem, pendingAction: { ...expectedAction, actionId: "replacement-action" }, } as unknown as HistoryItem) @@ -190,6 +233,58 @@ describe("History resume delegation - parent metadata transitions", () => { expect(createTaskWithHistoryItem).not.toHaveBeenCalled() }) + it("rejects missing pending-action ownership inside the atomic child updater", async () => { + const parentHistoryItem = { + id: "parent-missing-action", + status: "delegated", + awaitingChildId: "child-missing-action", + ts: 1, + task: "Parent task", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const expectedAction = { + kind: "finish_subtask" as const, + actionId: "finish-action", + approvalText: "{}", + parentTaskId: "parent-missing-action", + result: "Done", + } + const childHistoryItem = { id: "child-missing-action", status: "active", pendingAction: expectedAction } + const atomicUpdatePair = vi.fn( + async ( + _firstId: string, + _secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + secondUpdater: (item: HistoryItem) => HistoryItem, + ) => { + firstUpdater(parentHistoryItem as HistoryItem) + secondUpdater({ ...childHistoryItem, pendingAction: undefined } as unknown as HistoryItem) + return [] + }, + ) + const taskHistoryStore = makeTaskHistoryStoreStub(childHistoryItem, parentHistoryItem, { atomicUpdatePair }) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }), + getCurrentTask: vi.fn(() => undefined), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + taskHistoryStore, + log: vi.fn(), + }) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-missing-action", + childTaskId: "child-missing-action", + completionResultSummary: "Done", + pendingActionId: "finish-action", + }), + ).rejects.toThrow("Pending action mismatch for child child-missing-action") + }) + it("reopenParentFromDelegation accepts an active parent awaiting the returning child", async () => { const providerEmit = vi.fn() const parentHistoryItem = { @@ -237,7 +332,7 @@ describe("History resume delegation - parent metadata transitions", () => { removeClineFromStack, createTaskWithHistoryItem, taskHistoryStore, - } as any) + }) vi.mocked(readTaskMessages).mockResolvedValue([]) vi.mocked(readApiMessages).mockResolvedValue([]) @@ -249,15 +344,22 @@ describe("History resume delegation - parent metadata transitions", () => { pendingActionId: "finish-action", }) - // atomicUpdatePair called with child first, parent second + // atomicUpdatePair guards and writes the parent before completing the child. expect(taskHistoryStore.atomicUpdatePair).toHaveBeenCalledTimes(1) - const [firstId, secondId, firstUpdater, secondUpdater] = taskHistoryStore.atomicUpdatePair.mock.calls[0] - expect(firstId).toBe("child-1") - expect(secondId).toBe("parent-1") + const [firstId, secondId, firstUpdater, secondUpdater, options] = + taskHistoryStore.atomicUpdatePair.mock.calls[0] + expect(firstId).toBe("parent-1") + expect(secondId).toBe("child-1") + expect(taskHistoryStore.withTaskFileLock).toHaveBeenCalledWith("parent-1", expect.any(Function)) + expect(options).toMatchObject({ + rollbackFirstOnSecondFailure: true, + firstFileLockAcquired: true, + storeLockAcquired: true, + rollbackBothOnCallbackFailure: true, + }) - // Verify child updater produces completed status and persists completionResultSummary - // so startup reconciliation has the real result if the parent write fails. - const updatedChild = firstUpdater({ + // Verify child updater produces completed status and persists completionResultSummary. + const updatedChild = secondUpdater({ id: "child-1", status: "active", pendingAction: { @@ -273,7 +375,7 @@ describe("History resume delegation - parent metadata transitions", () => { expect(updatedChild.pendingAction).toBeUndefined() // Verify parent updater produces active status with correct fields - const updatedParent = secondUpdater(parentHistoryItem as HistoryItem) + const updatedParent = firstUpdater(parentHistoryItem as HistoryItem) expect(updatedParent).toMatchObject({ id: "parent-1", status: "active", @@ -291,7 +393,7 @@ describe("History resume delegation - parent metadata transitions", () => { // Verify child closed and parent reopened with updated metadata expect(removeClineFromStack).toHaveBeenCalledTimes(1) - expect(removeClineFromStack).toHaveBeenCalledWith() + expect(removeClineFromStack).toHaveBeenCalledWith({ saveMessages: false }) expect(createTaskWithHistoryItem).toHaveBeenCalledWith( expect.objectContaining({ status: "active", @@ -301,6 +403,75 @@ describe("History resume delegation - parent metadata transitions", () => { ) }) + it("preserves an unrelated child pending action when completion has no action owner", async () => { + const parentHistoryItem = { + id: "parent-unowned-action", + status: "delegated", + awaitingChildId: "child-unowned-action", + childIds: ["child-unowned-action"], + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const pendingAction = { + kind: "finish_subtask" as const, + actionId: "other-action", + approvalText: "{}", + parentTaskId: "parent-unowned-action", + result: "Other result", + } + const childHistoryItem = { + id: "child-unowned-action", + status: "active", + pendingAction, + } + let updatedChild: HistoryItem | undefined + const taskHistoryStore = makeTaskHistoryStoreStub(childHistoryItem, parentHistoryItem, { + atomicUpdatePair: vi.fn( + async ( + _firstId: string, + _secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + secondUpdater: (item: HistoryItem) => HistoryItem, + options?: { whileFirstFileLocked?: () => Promise }, + ) => { + firstUpdater(parentHistoryItem as HistoryItem) + updatedChild = secondUpdater(childHistoryItem as HistoryItem) + await options?.whileFirstFileLocked?.() + return [] + }, + ), + }) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => undefined), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + }), + taskHistoryStore, + }) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + vi.mocked(saveTaskMessages).mockResolvedValue(undefined) + vi.mocked(saveApiMessages).mockResolvedValue(undefined) + + await ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-unowned-action", + childTaskId: "child-unowned-action", + completionResultSummary: "Done", + }) + + expect(updatedChild?.pendingAction).toEqual(pendingAction) + }) + it("reopenParentFromDelegation injects subtask_result into both UI and API histories", async () => { const parentItem = { id: "p1", @@ -342,6 +513,9 @@ describe("History resume delegation - parent metadata transitions", () => { completionResultSummary: "Subtask completed successfully", }) + expect(readTaskMessages).toHaveBeenCalledWith({ taskId: "p1", globalStoragePath: "/storage" }) + expect(readApiMessages).toHaveBeenCalledWith({ taskId: "p1", globalStoragePath: "/storage" }) + // Verify UI history injection (say: subtask_result) expect(saveTaskMessages).toHaveBeenCalledWith( expect.objectContaining({ @@ -476,6 +650,74 @@ describe("History resume delegation - parent metadata transitions", () => { expect((injectedMsg.content[0] as any).content).toMatch(/^Subtask .+ completed\.\n\nResult:\n/) }) + it("updates an existing matching tool_result instead of appending a duplicate", async () => { + const parentItem = { + id: "p-existing-result", + status: "delegated", + awaitingChildId: "c-existing-result", + childIds: ["c-existing-result"], + ts: 100, + task: "Parent with an existing result", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c-existing-result", status: "active" }, parentItem) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/storage" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "c-existing-result" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + }), + taskHistoryStore, + }) + const existingApiMessages = [ + { + role: "assistant" as const, + content: [ + { type: "tool_use" as const, name: "read_file", id: "tool-unrelated", input: {} }, + { type: "tool_use" as const, name: "new_task", id: "tool-existing", input: {} }, + ], + }, + { + role: "user" as const, + content: [ + { type: "tool_result" as const, tool_use_id: "tool-unrelated", content: "read result" }, + { type: "tool_result" as const, tool_use_id: "tool-existing", content: "old result" }, + ], + }, + ] + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue(existingApiMessages) + vi.mocked(saveTaskMessages).mockResolvedValue(undefined) + vi.mocked(saveApiMessages).mockResolvedValue(undefined) + + await ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "p-existing-result", + childTaskId: "c-existing-result", + completionResultSummary: "replacement result", + }) + + const persistedApiMessages = vi.mocked(saveApiMessages).mock.calls[0][0].messages + expect(persistedApiMessages).toHaveLength(2) + expect(persistedApiMessages[1]).toMatchObject({ + role: "user", + content: expect.arrayContaining([ + { + type: "tool_result", + tool_use_id: "tool-existing", + content: "Subtask c-existing-result completed.\n\nResult:\nreplacement result", + }, + ]), + }) + }) + it("reopenParentFromDelegation injects plain text when no new_task tool_use exists in API history", async () => { const parentItem = { id: "p-no-tool", @@ -526,6 +768,61 @@ describe("History resume delegation - parent metadata transitions", () => { expect((injected.content[0] as any).text).toContain("Subtask c-no-tool completed") }) + it("keeps already-injected UI and fallback API completion records idempotent", async () => { + const parentItem = { + id: "p-existing-fallback", + status: "delegated", + awaitingChildId: "c-existing-fallback", + childIds: ["c-existing-fallback"], + ts: 100, + task: "Parent with existing fallback", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const completionResultSummary = "Already recorded" + const fallbackText = `Subtask c-existing-fallback completed.\n\nResult:\n${completionResultSummary}` + const existingUiMessages = [ + { + type: "say" as const, + say: "subtask_result" as const, + text: completionResultSummary, + ts: 50, + }, + ] + const existingApiMessages = [ + { role: "user" as const, content: [{ type: "text" as const, text: fallbackText }], ts: 50 }, + ] + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "c-existing-fallback", status: "active" }, parentItem) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/storage" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + emit: vi.fn(), + getCurrentTask: vi.fn(() => ({ taskId: "c-existing-fallback" })), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTaskWithHistoryItem: vi.fn().mockResolvedValue({ + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + }), + taskHistoryStore, + }) + + vi.mocked(readTaskMessages).mockResolvedValue(existingUiMessages) + vi.mocked(readApiMessages).mockResolvedValue(existingApiMessages) + vi.mocked(saveTaskMessages).mockResolvedValue(undefined) + vi.mocked(saveApiMessages).mockResolvedValue(undefined) + + await ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "p-existing-fallback", + childTaskId: "c-existing-fallback", + completionResultSummary, + }) + + expect(vi.mocked(saveTaskMessages).mock.calls[0][0].messages).toEqual(existingUiMessages) + expect(vi.mocked(saveApiMessages).mock.calls[0][0].messages).toEqual(existingApiMessages) + }) + it("reopenParentFromDelegation sets skipPrevResponseIdOnce via resumeAfterDelegation", async () => { const parentInstance: any = { skipPrevResponseIdOnce: false, @@ -663,7 +960,7 @@ describe("History resume delegation - parent metadata transitions", () => { vi.mocked(readApiMessages).mockResolvedValue([]) await expect( - (ClineProvider.prototype as any).reopenParentFromDelegation.call(provider, { + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { parentTaskId: "parent-rpd06", childTaskId: "child-rpd06", completionResultSummary: "Subtask finished despite overwrite failures", @@ -672,6 +969,10 @@ describe("History resume delegation - parent metadata transitions", () => { expect(parentInstance.overwriteClineMessages).toHaveBeenCalledTimes(1) expect(parentInstance.overwriteApiConversationHistory).toHaveBeenCalledTimes(1) + expect(parentInstance.overwriteClineMessages).toHaveBeenCalledWith(expect.any(Array), { persist: false }) + expect(parentInstance.overwriteApiConversationHistory).toHaveBeenCalledWith(expect.any(Array), { + persist: false, + }) expect(parentInstance.resumeAfterDelegation).toHaveBeenCalledTimes(1) expect(emitSpy).toHaveBeenCalledWith( @@ -776,14 +1077,14 @@ describe("History resume delegation - parent metadata transitions", () => { expect(removeClineFromStack).not.toHaveBeenCalled() - // Verify atomicUpdatePair called with child first (completed) and parent second (active) + // Verify atomicUpdatePair guards the parent before completing the child. expect(taskHistoryStore.atomicUpdatePair).toHaveBeenCalledTimes(1) const [firstId, secondId, firstUpdater, secondUpdater] = taskHistoryStore.atomicUpdatePair.mock.calls[0] - expect(firstId).toBe("child-rpd02") - expect(secondId).toBe("parent-rpd02") - const updatedChild = firstUpdater({ id: "child-rpd02", status: "active" } as HistoryItem) + expect(firstId).toBe("parent-rpd02") + expect(secondId).toBe("child-rpd02") + const updatedChild = secondUpdater({ id: "child-rpd02", status: "active" } as HistoryItem) expect(updatedChild.status).toBe("completed") - const updatedParent = secondUpdater(parentItem as HistoryItem) + const updatedParent = firstUpdater(parentItem as HistoryItem) expect(updatedParent).toMatchObject({ id: "parent-rpd02", status: "active", completedByChildId: "child-rpd02" }) expect(createTaskWithHistoryItem).toHaveBeenCalledWith( @@ -885,8 +1186,188 @@ describe("History resume delegation - parent metadata transitions", () => { }), ).rejects.toThrow(persistError) - // Child is closed before the atomic write (new ordering) — child closed, parent not reopened - expect(removeClineFromStack).toHaveBeenCalledTimes(1) + // A failed handoff leaves the child available for retry. + expect(removeClineFromStack).not.toHaveBeenCalled() + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() + }) + + it("keeps the delegation retryable when API history persistence fails", async () => { + const parentItem = { + id: "parent-api-save-failure", + status: "delegated", + awaitingChildId: "child-api-save-failure", + childIds: ["child-api-save-failure"], + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const originalUiMessages = [{ type: "say" as const, say: "text" as const, text: "before", ts: 1 }] + const originalApiMessages = [{ role: "user" as const, content: [{ type: "text" as const, text: "before" }] }] + const taskHistoryStore = makeTaskHistoryStoreStub( + { id: "child-api-save-failure", status: "active" }, + parentItem, + ) + const removeClineFromStack = vi.fn() + const createTaskWithHistoryItem = vi.fn() + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + getCurrentTask: vi.fn(() => ({ taskId: "child-api-save-failure" })), + removeClineFromStack, + createTaskWithHistoryItem, + taskHistoryStore, + }) + + vi.mocked(readTaskMessages).mockResolvedValue(originalUiMessages) + vi.mocked(readApiMessages).mockResolvedValue(originalApiMessages) + vi.mocked(saveTaskMessages).mockResolvedValue(undefined) + vi.mocked(saveApiMessages).mockRejectedValueOnce(new Error("api save failed")).mockResolvedValueOnce(undefined) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-api-save-failure", + childTaskId: "child-api-save-failure", + completionResultSummary: "Done", + }), + ).rejects.toThrow("api save failed") + + expect(taskHistoryStore.atomicUpdatePair).not.toHaveBeenCalled() + expect(removeClineFromStack).not.toHaveBeenCalled() + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() + expect(saveTaskMessages).toHaveBeenLastCalledWith(expect.objectContaining({ messages: originalUiMessages })) + expect(saveApiMessages).toHaveBeenLastCalledWith(expect.objectContaining({ messages: originalApiMessages })) + }) + + it("surfaces all restoration failures without committing completion metadata", async () => { + const parentItem = { + id: "parent-restore-failure", + status: "delegated", + awaitingChildId: "child-restore-failure", + childIds: ["child-restore-failure"], + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-restore-failure", status: "active" }, parentItem) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + getCurrentTask: vi.fn(() => ({ taskId: "child-restore-failure" })), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + taskHistoryStore, + }) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + const initialError = new Error("initial UI save failed") + const uiRestoreError = new Error("UI restore failed") + const apiRestoreError = new Error("API restore failed") + vi.mocked(saveTaskMessages).mockRejectedValueOnce(initialError).mockRejectedValueOnce(uiRestoreError) + vi.mocked(saveApiMessages).mockRejectedValueOnce(apiRestoreError) + + const result = ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-restore-failure", + childTaskId: "child-restore-failure", + completionResultSummary: "Done", + }) + await expect(result).rejects.toMatchObject({ + name: "AggregateError", + message: expect.stringContaining("Failed to restore parent parent-restore-failure conversation files"), + errors: [initialError, uiRestoreError, apiRestoreError], + }) + expect(taskHistoryStore.atomicUpdatePair).not.toHaveBeenCalled() + }) + + it("propagates a UI history read rejection without changing persistence or the task stack", async () => { + const parentItem = { + id: "parent-read-failure", + status: "delegated", + awaitingChildId: "child-read-failure", + childIds: ["child-read-failure"], + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub({ id: "child-read-failure", status: "active" }, parentItem) + const removeClineFromStack = vi.fn() + const createTaskWithHistoryItem = vi.fn() + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + getCurrentTask: vi.fn(() => ({ taskId: "child-read-failure" })), + removeClineFromStack, + createTaskWithHistoryItem, + taskHistoryStore, + }) + + vi.mocked(readTaskMessages).mockRejectedValue(new Error("UI read failed")) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-read-failure", + childTaskId: "child-read-failure", + completionResultSummary: "Done", + }), + ).rejects.toThrow("UI read failed") + + expect(readApiMessages).not.toHaveBeenCalled() + expect(saveTaskMessages).not.toHaveBeenCalled() + expect(saveApiMessages).not.toHaveBeenCalled() + expect(taskHistoryStore.atomicUpdatePair).not.toHaveBeenCalled() + expect(removeClineFromStack).not.toHaveBeenCalled() + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() + }) + + it("propagates an API history read rejection without changing persistence or the task stack", async () => { + const parentItem = { + id: "parent-api-read-failure", + status: "delegated", + awaitingChildId: "child-api-read-failure", + childIds: ["child-api-read-failure"], + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const taskHistoryStore = makeTaskHistoryStoreStub( + { id: "child-api-read-failure", status: "active" }, + parentItem, + ) + const removeClineFromStack = vi.fn() + const createTaskWithHistoryItem = vi.fn() + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + getCurrentTask: vi.fn(() => ({ taskId: "child-api-read-failure" })), + removeClineFromStack, + createTaskWithHistoryItem, + taskHistoryStore, + }) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockRejectedValue(new Error("API read failed")) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-api-read-failure", + childTaskId: "child-api-read-failure", + completionResultSummary: "Done", + }), + ).rejects.toThrow("API read failed") + + expect(saveTaskMessages).not.toHaveBeenCalled() + expect(saveApiMessages).not.toHaveBeenCalled() + expect(taskHistoryStore.atomicUpdatePair).not.toHaveBeenCalled() + expect(removeClineFromStack).not.toHaveBeenCalled() expect(createTaskWithHistoryItem).not.toHaveBeenCalled() }) @@ -1061,6 +1542,478 @@ describe("History resume delegation - parent metadata transitions", () => { expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[reopenParentFromDelegation] Aborting")) }) + it("aborts before reading histories when the refreshed parent awaits another child", async () => { + const persistedParent = { + id: "parent-refreshed-stale", + status: "delegated", + awaitingChildId: "child-original", + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const refreshedParent = { ...persistedParent, awaitingChildId: "child-replacement" } + const atomicUpdatePair = vi.fn() + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: persistedParent }), + getCurrentTask: vi.fn(() => undefined), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + taskHistoryStore: { + get: vi.fn((id: string) => (id === persistedParent.id ? refreshedParent : undefined)), + atomicUpdatePair, + withTaskFileLock: vi.fn(async (_id: string, callback: () => Promise) => callback()), + }, + log: vi.fn(), + }) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: persistedParent.id, + childTaskId: "child-original", + completionResultSummary: "stale result", + }), + ).resolves.toBe(false) + + expect(readTaskMessages).not.toHaveBeenCalled() + expect(readApiMessages).not.toHaveBeenCalled() + expect(atomicUpdatePair).not.toHaveBeenCalled() + }) + + it("reopenParentFromDelegation aborts when another host re-delegates after the initial guard", async () => { + const staleParent = { + id: "parent-cross-host", + status: "delegated", + awaitingChildId: "child-old", + delegatedToId: "child-old", + childIds: ["child-old"], + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const diskRecords = new Map([ + [ + "parent-cross-host", + { + ...staleParent, + awaitingChildId: "child-new", + delegatedToId: "child-new", + childIds: ["child-old", "child-new"], + } as HistoryItem, + ], + [ + "child-old", + { + id: "child-old", + status: "interrupted", + parentTaskId: "parent-cross-host", + } as HistoryItem, + ], + ]) + let diskGuardError: Error | undefined + const atomicUpdatePair = vi.fn( + async ( + firstId: string, + secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + secondUpdater: (item: HistoryItem) => HistoryItem, + options?: { firstDiskGuard?: (item: HistoryItem) => void }, + ) => { + const first = diskRecords.get(firstId)! + const second = diskRecords.get(secondId)! + try { + options?.firstDiskGuard?.(first) + } catch (error) { + diskGuardError = error as Error + throw error + } + firstUpdater(first) + secondUpdater(second) + return [] + }, + ) + const createTaskWithHistoryItem = vi.fn() + const removeClineFromStack = vi.fn() + const log = vi.fn() + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: staleParent }), + emit: vi.fn(), + log, + getCurrentTask: vi.fn(() => ({ taskId: "child-old" })), + removeClineFromStack, + createTaskWithHistoryItem, + taskHistoryStore: { + atomicUpdatePair, + get: vi.fn((id: string) => (id === "parent-cross-host" ? staleParent : diskRecords.get(id))), + }, + }) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: "parent-cross-host", + childTaskId: "child-old", + completionResultSummary: "stale result", + }), + ).resolves.toBe(false) + + expect(createTaskWithHistoryItem).not.toHaveBeenCalled() + expect(removeClineFromStack).not.toHaveBeenCalled() + expect(saveTaskMessages).toHaveBeenCalledTimes(2) + expect(saveApiMessages).toHaveBeenCalledTimes(2) + expect(log).toHaveBeenCalledWith(expect.stringContaining("is no longer delegated to child child-old")) + expect(diskGuardError?.message).toBe("stale cross-instance delegation") + }) + + it("treats a status change inside the atomic parent updater as a stale delegation", async () => { + const parentItem = { + id: "parent-atomic-status-change", + status: "delegated", + awaitingChildId: "child-atomic-status-change", + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const childItem = { id: "child-atomic-status-change", status: "active" } + const atomicUpdatePair = vi.fn( + async ( + _firstId: string, + _secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + _secondUpdater: (item: HistoryItem) => HistoryItem, + options?: { firstDiskGuard?: (item: HistoryItem) => void }, + ) => { + options?.firstDiskGuard?.(parentItem as HistoryItem) + firstUpdater({ ...parentItem, status: "completed" } as HistoryItem) + return [] + }, + ) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + getCurrentTask: vi.fn(() => undefined), + removeClineFromStack: vi.fn(), + createTaskWithHistoryItem: vi.fn(), + taskHistoryStore: { + get: vi.fn((id: string) => (id === parentItem.id ? parentItem : childItem)), + atomicUpdatePair, + withTaskFileLock: vi.fn(async (_id: string, callback: () => Promise) => callback()), + }, + log: vi.fn(), + }) + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: parentItem.id, + childTaskId: childItem.id, + completionResultSummary: "stale result", + }), + ).resolves.toBe(false) + + expect(saveTaskMessages).toHaveBeenCalledTimes(2) + expect(saveApiMessages).toHaveBeenCalledTimes(2) + expect(provider.log).toHaveBeenCalledWith( + expect.stringContaining(`parent ${parentItem.id} is no longer delegated to child ${childItem.id}`), + ) + }) + + it("restores the child after parent rehydration fails and allows completion to retry", async () => { + const parentItem = { + id: "parent-rehydrate-failure", + status: "delegated", + awaitingChildId: "child-rehydrate-failure", + delegatedToId: "child-rehydrate-failure", + childIds: ["child-rehydrate-failure"], + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const childItem = { + id: "child-rehydrate-failure", + status: "active", + parentTaskId: "parent-rehydrate-failure", + ts: 2, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + let lockHeld = false + let currentTaskId: string | undefined = childItem.id + const withTaskFileLock = vi.fn(async (_id: string, callback: () => Promise) => { + lockHeld = true + try { + return await callback() + } finally { + lockHeld = false + } + }) + const atomicUpdatePair = vi.fn( + async ( + _firstId: string, + _secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + secondUpdater: (item: HistoryItem) => HistoryItem, + options?: { + whileFirstFileLocked?: () => Promise + rollbackBothOnCallbackFailure?: boolean + firstFileLockAcquired?: boolean + storeLockAcquired?: boolean + }, + ) => { + expect(lockHeld).toBe(true) + const parentSnapshot = structuredClone(parentItem) + const childSnapshot = structuredClone(childItem) + Object.assign(parentItem, firstUpdater(parentItem as HistoryItem)) + Object.assign(childItem, secondUpdater(childItem as HistoryItem)) + try { + await options?.whileFirstFileLocked?.() + } catch (error) { + expect(options?.rollbackBothOnCallbackFailure).toBe(true) + for (const key of Object.keys(parentItem)) delete (parentItem as Record)[key] + for (const key of Object.keys(childItem)) delete (childItem as Record)[key] + Object.assign(parentItem, parentSnapshot) + Object.assign(childItem, childSnapshot) + throw error + } + return [] + }, + ) + const removeLockStates: boolean[] = [] + const removeClineFromStack = vi.fn(async () => { + removeLockStates.push(lockHeld) + currentTaskId = undefined + }) + let parentCreateAttempts = 0 + const createCalls: Array<{ historyItem: HistoryItem; lockHeld: boolean; startTask: boolean | undefined }> = [] + const resumedParent = { + taskId: parentItem.id, + overwriteClineMessages: vi.fn().mockResolvedValue(undefined), + overwriteApiConversationHistory: vi.fn().mockResolvedValue(undefined), + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + } + const createTaskWithHistoryItem = vi.fn(async (historyItem: HistoryItem, options?: { startTask?: boolean }) => { + createCalls.push({ historyItem: structuredClone(historyItem), lockHeld, startTask: options?.startTask }) + currentTaskId = historyItem.id + if (historyItem.id === parentItem.id && parentCreateAttempts++ === 0) { + throw new Error("parent rehydration failed") + } + return historyItem.id === parentItem.id + ? resumedParent + : { + taskId: childItem.id, + resumeAfterDelegation: vi.fn().mockResolvedValue(undefined), + } + }) + const taskHistoryStore = { + atomicUpdatePair, + get: vi.fn((id: string) => + id === parentItem.id ? parentItem : id === childItem.id ? childItem : undefined, + ), + withTaskFileLock, + } + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockImplementation(async () => ({ historyItem: structuredClone(parentItem) })), + getCurrentTask: vi.fn(() => (currentTaskId ? { taskId: currentTaskId } : undefined)), + removeClineFromStack, + createTaskWithHistoryItem, + taskHistoryStore, + }) + + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + vi.mocked(saveTaskMessages).mockResolvedValue(undefined) + vi.mocked(saveApiMessages).mockResolvedValue(undefined) + + const completion = { + parentTaskId: parentItem.id, + childTaskId: childItem.id, + completionResultSummary: "Done", + } + await expect(ClineProvider.prototype.reopenParentFromDelegation.call(provider, completion)).rejects.toThrow( + "parent rehydration failed", + ) + + expect(parentItem).toMatchObject({ + status: "delegated", + awaitingChildId: childItem.id, + delegatedToId: childItem.id, + }) + expect(childItem.status).toBe("active") + expect(currentTaskId).toBe(childItem.id) + expect(createCalls[1]).toEqual({ historyItem: childItem, lockHeld: false, startTask: false }) + expect(removeLockStates).toEqual([true, false]) + expect(removeClineFromStack).toHaveBeenNthCalledWith(1, { saveMessages: false }) + expect(removeClineFromStack).toHaveBeenNthCalledWith(2, { saveMessages: false }) + expect(saveTaskMessages).toHaveBeenLastCalledWith(expect.objectContaining({ messages: [] })) + expect(saveApiMessages).toHaveBeenLastCalledWith(expect.objectContaining({ messages: [] })) + + await expect(ClineProvider.prototype.reopenParentFromDelegation.call(provider, completion)).resolves.toBe(true) + expect(parentItem.status).toBe("active") + expect(parentItem.awaitingChildId).toBeUndefined() + expect(childItem.status).toBe("completed") + expect(resumedParent.resumeAfterDelegation).toHaveBeenCalledOnce() + expect(withTaskFileLock).toHaveBeenCalledTimes(2) + expect(atomicUpdatePair).toHaveBeenCalledTimes(2) + }) + + it("aggregates the transition and child-restoration failures", async () => { + const transitionError = new Error("parent rehydration failed") + const restorationError = new Error("child restoration failed") + const parentItem = { + id: "parent-recovery-error", + status: "delegated", + awaitingChildId: "child-recovery-error", + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const childItem = { + id: "child-recovery-error", + status: "active", + parentTaskId: parentItem.id, + ts: 2, + task: "Child", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + let currentTaskId: string | undefined = childItem.id + const removeClineFromStack = vi.fn(async () => { + currentTaskId = undefined + }) + const createTaskWithHistoryItem = vi.fn(async (historyItem: HistoryItem) => { + if (historyItem.id === parentItem.id) throw transitionError + throw restorationError + }) + const atomicUpdatePair = vi.fn( + async ( + _firstId: string, + _secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + secondUpdater: (item: HistoryItem) => HistoryItem, + options?: { whileFirstFileLocked?: () => Promise }, + ) => { + firstUpdater(parentItem as HistoryItem) + secondUpdater(childItem as HistoryItem) + await options?.whileFirstFileLocked?.() + return [] + }, + ) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + getCurrentTask: vi.fn(() => (currentTaskId ? { taskId: currentTaskId } : undefined)), + removeClineFromStack, + createTaskWithHistoryItem, + taskHistoryStore: { + get: vi.fn((id: string) => (id === parentItem.id ? parentItem : childItem)), + atomicUpdatePair, + withTaskFileLock: vi.fn(async (_id: string, callback: () => Promise) => callback()), + }, + }) + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: parentItem.id, + childTaskId: childItem.id, + completionResultSummary: "Done", + }), + ).rejects.toMatchObject({ + name: "AggregateError", + message: `Failed to restore child ${childItem.id}`, + errors: [transitionError, restorationError], + }) + expect(removeClineFromStack).toHaveBeenCalledOnce() + expect(createTaskWithHistoryItem).toHaveBeenNthCalledWith(1, expect.objectContaining({ id: parentItem.id }), { + startTask: false, + }) + expect(createTaskWithHistoryItem).toHaveBeenNthCalledWith(2, expect.objectContaining({ id: childItem.id }), { + startTask: false, + }) + }) + + it("leaves an unrelated current task untouched when parent recovery fails", async () => { + const transitionError = new Error("parent rehydration failed") + const parentItem = { + id: "parent-unrelated-recovery", + status: "delegated", + awaitingChildId: "child-unrelated-recovery", + ts: 1, + task: "Parent", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + } + const childItem = { id: "child-unrelated-recovery", status: "active" } + let currentTaskId = childItem.id + const removeClineFromStack = vi.fn(async () => { + currentTaskId = "unrelated-task" + }) + const createTaskWithHistoryItem = vi.fn(async () => { + currentTaskId = "unrelated-task" + throw transitionError + }) + const atomicUpdatePair = vi.fn( + async ( + _firstId: string, + _secondId: string, + firstUpdater: (item: HistoryItem) => HistoryItem, + secondUpdater: (item: HistoryItem) => HistoryItem, + options?: { whileFirstFileLocked?: () => Promise }, + ) => { + firstUpdater(parentItem as HistoryItem) + secondUpdater(childItem as HistoryItem) + await options?.whileFirstFileLocked?.() + return [] + }, + ) + const provider = makeProviderStub({ + contextProxy: { globalStorageUri: { fsPath: "/tmp" } }, + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentItem }), + getCurrentTask: vi.fn(() => ({ taskId: currentTaskId })), + removeClineFromStack, + createTaskWithHistoryItem, + taskHistoryStore: { + get: vi.fn((id: string) => (id === parentItem.id ? parentItem : childItem)), + atomicUpdatePair, + withTaskFileLock: vi.fn(async (_id: string, callback: () => Promise) => callback()), + }, + }) + vi.mocked(readTaskMessages).mockResolvedValue([]) + vi.mocked(readApiMessages).mockResolvedValue([]) + + await expect( + ClineProvider.prototype.reopenParentFromDelegation.call(provider, { + parentTaskId: parentItem.id, + childTaskId: childItem.id, + completionResultSummary: "Done", + }), + ).rejects.toThrow(transitionError) + + expect(currentTaskId).toBe("unrelated-task") + expect(removeClineFromStack).toHaveBeenCalledOnce() + expect(createTaskWithHistoryItem).toHaveBeenCalledOnce() + }) + it("serializes delegation transitions and continues after a rejected predecessor", async () => { const provider = makeProviderStub({} as any) as any const calls: string[] = [] @@ -1114,8 +2067,8 @@ describe("History resume delegation - parent metadata transitions", () => { ]) const taskHistoryStore = { atomicUpdatePair: vi.fn(async (_fId: string, _sId: string, fU: (h: any) => any, sU: (h: any) => any) => { - fU(childItem) - sU(parentItem) + fU(parentItem) + sU(childItem) return [] }), get: vi.fn((id: string) => itemMap.get(id)), @@ -1238,8 +2191,8 @@ describe("History resume delegation - parent metadata transitions", () => { secondUpdater: (h: HistoryItem) => HistoryItem, ) => { // Both updaters must be applied atomically - capturedChildResult = firstUpdater(childItem as unknown as HistoryItem) - capturedParentResult = secondUpdater(parentItem as unknown as HistoryItem) + capturedParentResult = firstUpdater(parentItem as unknown as HistoryItem) + capturedChildResult = secondUpdater(childItem as unknown as HistoryItem) return [] }, ) @@ -1334,9 +2287,11 @@ describe("History resume delegation - parent metadata transitions", () => { secondId: string, firstUpdater: (h: any) => any, secondUpdater: (h: any) => any, + options?: { whileFirstFileLocked?: () => Promise }, ) => { - Object.assign(childItem, firstUpdater(childItem)) - Object.assign(parentItem, secondUpdater(parentItem)) + Object.assign(parentItem, firstUpdater(parentItem)) + Object.assign(childItem, secondUpdater(childItem)) + await options?.whileFirstFileLocked?.() return [] }, ), diff --git a/src/__tests__/nested-delegation-resume.spec.ts b/src/__tests__/nested-delegation-resume.spec.ts index 8464f81b12..f24d7872cc 100644 --- a/src/__tests__/nested-delegation-resume.spec.ts +++ b/src/__tests__/nested-delegation-resume.spec.ts @@ -157,12 +157,14 @@ describe("Nested delegation resume (A → B → C)", () => { secondId: string, firstUpdater: (h: any) => any, secondUpdater: (h: any) => any, + options?: { whileFirstFileLocked?: () => Promise }, ) => { // Apply both updaters and persist to historyIndex atomically const updatedFirst = firstUpdater(historyIndex[firstId]) const updatedSecond = secondUpdater(historyIndex[secondId]) historyIndex[firstId] = updatedFirst historyIndex[secondId] = updatedSecond + await options?.whileFirstFileLocked?.() return Object.values(historyIndex) }, ), diff --git a/src/__tests__/provider-delegation.spec.ts b/src/__tests__/provider-delegation.spec.ts index 0b7aef8775..50f010aef3 100644 --- a/src/__tests__/provider-delegation.spec.ts +++ b/src/__tests__/provider-delegation.spec.ts @@ -20,6 +20,7 @@ function makeStoreStub( overrides: Partial<{ atomicReadAndUpdate: ReturnType; get: ReturnType }> = {}, ) { return { + withTaskFileLock: vi.fn(async (_taskId: string, callback: () => Promise) => callback()), atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (h: HistoryItem) => HistoryItem) => { updater(parentHistoryItem) return [] @@ -43,6 +44,51 @@ const makeParentTask = () => }) as any describe("ClineProvider.delegateParentAndOpenChild()", () => { + it("forwards saveMessages false only when explicitly removing without persistence", async () => { + const task = { + taskId: "child-1", + instanceId: "instance-1", + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + } + const provider = { + taskRegistry: { + length: 1, + current: task, + remove: vi.fn().mockReturnValue(task), + }, + taskEventListeners: new Map(), + log: vi.fn(), + } as unknown as ClineProvider + + await ClineProvider.prototype.removeClineFromStack.call(provider, { saveMessages: false }) + + expect(task.abortTask).toHaveBeenCalledWith(true, { saveMessages: false }) + }) + + it("uses normal task persistence when remove options are omitted", async () => { + const task = { + taskId: "child-1", + instanceId: "instance-1", + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + } + const provider = { + taskRegistry: { + length: 1, + current: task, + remove: vi.fn().mockReturnValue(task), + }, + taskEventListeners: new Map(), + log: vi.fn(), + } as unknown as ClineProvider + + await ClineProvider.prototype.removeClineFromStack.call(provider) + + expect(task.abortTask).toHaveBeenCalledTimes(1) + expect(task.abortTask).toHaveBeenCalledWith(true) + }) + it("rejects a stale restored action before delegation side effects", async () => { const parentTask = makeParentTask() const removeClineFromStack = vi.fn() @@ -97,6 +143,7 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { } let current: HistoryItem = { ...parentHistoryItem, status: "active", pendingAction } const taskHistoryStore = { + withTaskFileLock: vi.fn(async (_taskId: string, callback: () => Promise) => callback()), get: vi.fn(() => current), atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { current = updater(current) @@ -129,6 +176,48 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(current).toMatchObject({ status: "delegated", awaitingChildId: "child-1" }) }) + it("preserves an unrelated pending action when delegation has no action owner", async () => { + const pendingAction = { + kind: "create_subtask" as const, + actionId: "other-action", + approvalText: "{}", + mode: "code", + message: "Other request", + todos: [], + } + let current: HistoryItem = { ...parentHistoryItem, status: "active", pendingAction } + const taskHistoryStore = { + withTaskFileLock: vi.fn(async (_taskId: string, callback: () => Promise) => callback()), + get: vi.fn(() => current), + atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { + current = updater(current) + return [current] + }), + } + const parentTask = makeParentTask() + const child = { taskId: "child-1", run: vi.fn().mockResolvedValue(undefined) } + const provider = { + taskScheduler: new TaskScheduler(), + emit: vi.fn(), + getCurrentTask: vi.fn(() => parentTask), + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue(child), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + log: vi.fn(), + isViewLaunched: false, + taskHistoryStore, + } as unknown as ClineProvider + + await ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + }) + + expect(current.pendingAction).toEqual(pendingAction) + }) + it("rolls back when pending-action ownership changes before the atomic parent update", async () => { const pendingAction = { kind: "create_subtask" as const, @@ -185,6 +274,53 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect(createTaskWithHistoryItem).toHaveBeenCalledWith(parentHistoryItem) }) + it("rolls back with a pending-action mismatch when ownership disappears before the atomic update", async () => { + const pendingAction = { + kind: "create_subtask" as const, + actionId: "create-action", + approvalText: "{}", + mode: "code", + message: "Do something", + todos: [], + } + const parentTask = makeParentTask() + const child = { taskId: "child-1", run: vi.fn().mockResolvedValue(undefined) } + const getCurrentTask = vi.fn(() => parentTask) + const taskHistoryStore = makeStoreStub({ + get: vi.fn().mockReturnValue({ ...parentHistoryItem, status: "active", pendingAction }), + atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { + updater({ ...parentHistoryItem, status: "active", pendingAction: undefined }) + return [] + }), + }) + const provider = { + taskScheduler: new TaskScheduler(), + emit: vi.fn(), + getCurrentTask, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue(child), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + deleteTaskWithId: vi.fn().mockResolvedValue(undefined), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: parentHistoryItem }), + createTaskWithHistoryItem: vi.fn().mockResolvedValue(undefined), + log: vi.fn(), + isViewLaunched: false, + taskHistoryStore, + } as unknown as ClineProvider + + await expect( + ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Do something", + initialTodos: [], + mode: "code", + pendingActionId: "create-action", + }), + ).rejects.toThrow( + "[delegateParentAndOpenChild] Pending action mismatch for parent parent-1: expected create-action, found undefined", + ) + }) + it("persists parent delegation metadata via atomicReadAndUpdate and emits TaskDelegated", async () => { const providerEmit = vi.fn() const parentTask = makeParentTask() @@ -230,8 +366,9 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { // Delegation metadata written via atomicReadAndUpdate with correct taskId expect(taskHistoryStore.atomicReadAndUpdate).toHaveBeenCalledTimes(1) - const [calledTaskId, updater] = taskHistoryStore.atomicReadAndUpdate.mock.calls[0] + const [calledTaskId, updater, updateOptions] = taskHistoryStore.atomicReadAndUpdate.mock.calls[0] expect(calledTaskId).toBe("parent-1") + expect(updateOptions).toEqual({ fileLockAcquired: true, storeLockAcquired: true }) // The updater must produce the correct delegation fields const result = updater(parentHistoryItem) @@ -475,6 +612,53 @@ describe("ClineProvider.delegateParentAndOpenChild()", () => { expect((provider as any).deleteTaskWithId).toHaveBeenCalledWith("child-2", false) }) + it("reports a missing awaited child as an invalid re-delegation instead of dereferencing it", async () => { + const oldChildId = "missing-child" + const alreadyDelegatedParent: HistoryItem = { + ...parentHistoryItem, + status: "delegated", + awaitingChildId: oldChildId, + delegatedToId: oldChildId, + } as unknown as HistoryItem + const child = { taskId: "child-2", run: vi.fn().mockResolvedValue(undefined) } + const getCurrentTask = vi.fn().mockReturnValue(makeParentTask()) + const taskHistoryStore = makeStoreStub({ + get: vi.fn((id: string) => (id === "parent-1" ? alreadyDelegatedParent : undefined)), + atomicReadAndUpdate: vi.fn(async (_taskId: string, updater: (item: HistoryItem) => HistoryItem) => { + updater(alreadyDelegatedParent) + return [] + }), + }) + const provider = { + taskScheduler: new TaskScheduler(), + emit: vi.fn(), + getCurrentTask, + removeClineFromStack: vi.fn().mockResolvedValue(undefined), + createTask: vi.fn().mockResolvedValue(child), + handleModeSwitch: vi.fn().mockResolvedValue(undefined), + deleteTaskWithId: vi.fn().mockResolvedValue(undefined), + getTaskWithId: vi.fn().mockResolvedValue({ historyItem: alreadyDelegatedParent }), + createTaskWithHistoryItem: vi.fn().mockResolvedValue(undefined), + log: vi.fn(), + isViewLaunched: false, + taskHistoryStore, + } as unknown as ClineProvider + + await expect( + ClineProvider.prototype.delegateParentAndOpenChild.call(provider, { + parentTaskId: "parent-1", + message: "Continue", + initialTodos: [], + mode: "code", + }), + ).rejects.toThrow( + "Cannot re-delegate task parent-1: existing child missing-child is undefined, not interrupted", + ) + + expect(child.run).not.toHaveBeenCalled() + expect(provider.deleteTaskWithId).toHaveBeenCalledWith("child-2", false) + }) + it("rolls back the paused child and restores the parent when atomicReadAndUpdate fails", async () => { const persistError = new Error("parent metadata persist failed") const parentTask = makeParentTask() diff --git a/src/core/task-persistence/TaskHistoryStore.ts b/src/core/task-persistence/TaskHistoryStore.ts index 3d4cc47604..ce491369e8 100644 --- a/src/core/task-persistence/TaskHistoryStore.ts +++ b/src/core/task-persistence/TaskHistoryStore.ts @@ -7,7 +7,7 @@ import deepEqual from "fast-deep-equal" import type { HistoryItem } from "@roo-code/types" import { GlobalFileNames } from "../../shared/globalFileNames" -import { LOCK_STALE_MS, safeWriteJson } from "../../utils/safeWriteJson" +import { LOCK_STALE_MS, lockJsonFile, safeWriteJson } from "../../utils/safeWriteJson" import { getStorageBasePath } from "../../utils/storage" import { assertValidTransition, type HistoryItemStatus } from "./taskLifecycle" import { computeHistoryDelta, DeltaRejectedError, mergeHistoryDelta } from "./taskStoreConcurrency" @@ -19,8 +19,17 @@ export { DeltaRejectedError } from "./taskStoreConcurrency" * Build a `safeWriteJson` merge callback that applies only `delta` to the * current disk state, preserving fields written by another process. */ -function mergeWithDisk(delta: Partial): (existing: unknown, incoming: unknown) => unknown { - return (existing, incoming) => mergeHistoryDelta(existing, incoming as HistoryItem, delta) +function mergeWithDisk( + delta: Partial, + options: { mergeChildIds?: boolean } = {}, +): (existing: unknown, incoming: unknown) => unknown { + return (existing, incoming) => { + const merged = mergeHistoryDelta(existing, incoming as HistoryItem, delta) + if (options.mergeChildIds === false && delta.childIds) { + merged.childIds = delta.childIds + } + return merged + } } /** @@ -77,6 +86,25 @@ export interface TaskHistoryStoreOptions { onWrite?: (items: HistoryItem[]) => Promise } +export interface AtomicUpdatePairOptions { + /** Validate the first record against its current on-disk state while its cross-process lock is held. */ + firstDiskGuard?: (current: HistoryItem) => void + /** Restore the first record's exact guarded pre-image if writing the second record fails. */ + rollbackFirstOnSecondFailure?: boolean + /** Restore both exact guarded pre-images if post-write callback work fails. */ + rollbackBothOnCallbackFailure?: boolean + /** + * Run finite handoff work after both writes and `onWrite`, before releasing the first file lock. + * The callback runs inside the non-reentrant store lock and must not call store mutation, + * invalidation, or reconciliation methods. Rejection occurs after both records are initially durable. + */ + whileFirstFileLocked?: () => Promise + /** The caller already holds the first record's cross-process lock. */ + firstFileLockAcquired?: boolean + /** The caller already holds the in-process store lock. */ + storeLockAcquired?: boolean +} + export class TaskHistoryStore { private readonly globalStoragePath: string private readonly onWrite?: (items: HistoryItem[]) => Promise @@ -849,13 +877,25 @@ export class TaskHistoryStore { * process are preserved. Without a delta the full item is written * as-is (used by administrative repair paths that are authoritative). */ - private async writeTaskFile(item: HistoryItem, delta?: Partial): Promise { + private async writeTaskFile( + item: HistoryItem, + delta?: Partial, + diskGuard?: (current: HistoryItem) => void, + options?: { mergeChildIds?: boolean; lockAcquired?: boolean }, + ): Promise { const filePath = await this.getTaskFilePath(item.id) if (delta) { let written: HistoryItem = item - const mergeFn = mergeWithDisk(delta) + const mergeFn = mergeWithDisk(delta, options) await safeWriteJson(filePath, item, { + lockAcquired: options?.lockAcquired, merge: (existing, incoming) => { + if (diskGuard) { + if (Object(existing) !== existing || !("id" in (existing as object))) { + throw new Error(`[TaskHistoryStore] guarded write: task ${item.id} not found on disk`) + } + diskGuard(existing as HistoryItem) + } const result = mergeFn(existing, incoming) written = result as HistoryItem return result @@ -868,6 +908,36 @@ export class TaskHistoryStore { } } + private async restoreTaskFilePreImage( + taskId: string, + preImage: HistoryItem, + expectedWritten: HistoryItem, + lockAcquired: boolean, + ): Promise { + try { + await safeWriteJson(await this.getTaskFilePath(taskId), preImage, { + lockAcquired, + merge: (existing) => { + if (!existing || typeof existing !== "object" || !("id" in existing)) { + throw new Error(`[TaskHistoryStore] atomicUpdatePair: ${taskId} missing during compensation`) + } + if (!deepEqual(existing, expectedWritten)) { + throw new Error( + `[TaskHistoryStore] atomicUpdatePair: cannot compensate ${taskId} after a concurrent update`, + ) + } + return preImage + }, + }) + this.cache.set(taskId, structuredClone(preImage)) + } catch (error) { + const current = await this.readTaskFile(taskId) + this.cache.delete(taskId) + if (current) this.cache.set(taskId, current) + throw error + } + } + /** * Read a HistoryItem from its per-task `history_item.json` file. */ @@ -958,39 +1028,80 @@ export class TaskHistoryStore { // ────────────────────────────── Atomic read-modify-write ────────────────────────────── /** - * Read a HistoryItem from the in-memory cache and write back an updated version, - * all within a single lock acquisition so no concurrent writer can interleave - * between the read and the write. - * - * The `updater` receives the current cached item and must return the new item - * synchronously. It must not perform I/O or acquire any other lock. + * Run a bounded parent transition while holding the in-process store lock and then + * the task's cross-process file lock. Store mutations inside the callback must use + * their already-acquired-lock options; other store mutation, invalidation, and + * reconciliation methods are non-reentrant and must not be called. + */ + public async withTaskFileLock(taskId: string, callback: () => Promise): Promise { + return this.withLock(async () => { + const releaseFileLock = await lockJsonFile(await this.getTaskFilePath(taskId)) + try { + const current = await this.readTaskFile(taskId) + if (current) this.cache.set(taskId, current) + return await callback() + } finally { + await releaseFileLock() + } + }) + } + + /** + * Read the current on-disk HistoryItem and write back an updated version while + * holding both the in-process store lock and the record's cross-process lock. + * The synchronous updater must not perform I/O or acquire another lock. * * @throws If the task ID is not present in the cache. */ - public atomicReadAndUpdate(taskId: string, updater: (current: HistoryItem) => HistoryItem): Promise { - return this.withLock(async () => { - const current = this.cache.get(taskId) - if (!current) { + public atomicReadAndUpdate( + taskId: string, + updater: (current: HistoryItem) => HistoryItem, + options: { fileLockAcquired?: boolean; storeLockAcquired?: boolean } = {}, + ): Promise { + const update = async () => { + const cached = this.cache.get(taskId) + if (!cached) { throw new Error(`[TaskHistoryStore] atomicReadAndUpdate: task ${taskId} not found in cache`) } - // Deep-copy so a mutating updater cannot alter cached state before persistence. - const snapshot = structuredClone(current) - const updated = updater(snapshot) - if (updated.id !== taskId) { - throw new Error( - `[TaskHistoryStore] atomicReadAndUpdate: updater changed task id from ${taskId} to ${updated.id}`, - ) + const releaseFileLock = options.fileLockAcquired + ? async () => {} + : await lockJsonFile(await this.getTaskFilePath(taskId)) + try { + const current = (await this.readTaskFile(taskId)) ?? cached + const updated = updater(structuredClone(current)) + if (updated.id !== taskId) throw new Error(`Task updater changed id from ${taskId} to ${updated.id}`) + if (updated.status !== undefined) { + const currentStatus: HistoryItemStatus = current.status ?? "active" + if (updated.status !== currentStatus) { + assertValidTransition(current.status, updated.status) + } + } + + const merged = { ...current, ...updated } + const written = await this.writeTaskFile(merged, this.buildDelta(taskId, current, updated), undefined, { + lockAcquired: true, + }) + this.cache.set(taskId, written) + const all = this.getAll() + if (this.onWrite) await this.onWrite(all) + return all + } finally { + await releaseFileLock() } - return this.upsertCore(updated) - }) + } + return options.storeLockAcquired ? update() : this.withLock(update) } /** - * Update two related HistoryItems within a single in-process lock acquisition. - * Both updaters run synchronously (no I/O, no lock re-entry). Both writes - * complete before the lock releases, so no in-process reader can observe an - * intermediate state. Cross-process atomicity is NOT guaranteed — each - * writeTaskFile call acquires and releases its own advisory file lock. + * Update two related HistoryItems within one in-process lock acquisition. Both + * updaters are synchronous and both writes finish before the store lock releases. + * + * By default each record write takes only its own file lock, so cross-process + * atomicity is not guaranteed. Supplying a first-record guard, rollback, compensation, or + * `whileFirstFileLocked` holds the first record's lock across both writes, + * `onWrite`, and the callback; the second record's lock still covers only its own + * write. `firstFileLockAcquired` and `storeLockAcquired` reuse locks held by + * `withTaskFileLock` and must only be set by that lock-scoped callback. * * @throws If either task ID is not present in the cache. */ @@ -999,8 +1110,9 @@ export class TaskHistoryStore { secondId: string, firstUpdater: (current: HistoryItem) => HistoryItem, secondUpdater: (current: HistoryItem) => HistoryItem, + options?: AtomicUpdatePairOptions, ): Promise { - return this.withLock(async () => { + const update = async () => { const first = this.cache.get(firstId) if (!first) throw new Error(`[TaskHistoryStore] atomicUpdatePair: ${firstId} not found`) const second = this.cache.get(secondId) @@ -1036,28 +1148,144 @@ export class TaskHistoryStore { // Merge with existing cache entries before writing, mirroring upsertCore. const mergedFirst = { ...first, ...updatedFirst } const mergedSecond = { ...second, ...updatedSecond } + const holdFirstFileLock = Boolean( + options?.firstDiskGuard || + options?.rollbackFirstOnSecondFailure || + options?.rollbackBothOnCallbackFailure || + options?.whileFirstFileLocked, + ) + const releaseFirstFileLock = options?.firstFileLockAcquired + ? async () => {} + : holdFirstFileLock + ? await lockJsonFile(await this.getTaskFilePath(firstId)) + : async () => {} - const writtenFirst = await this.writeTaskFile(mergedFirst, this.buildDelta(firstId, first, updatedFirst)) - let writtenSecond: HistoryItem try { - writtenSecond = await this.writeTaskFile(mergedSecond, this.buildDelta(secondId, second, updatedSecond)) - } catch (error) { - // First record is committed on disk. Update cache so it - // reflects disk state before propagating the error. + let firstDiskSnapshot: HistoryItem | undefined + const firstDiskGuard = options?.firstDiskGuard + const captureAndGuardFirst = + firstDiskGuard || options?.rollbackFirstOnSecondFailure || options?.rollbackBothOnCallbackFailure + ? (current: HistoryItem) => { + if (firstDiskGuard) firstDiskGuard(current) + firstDiskSnapshot = structuredClone(current) + } + : undefined + const firstDelta = this.buildDelta(firstId, first, updatedFirst) + const writtenFirst = await this.writeTaskFile(mergedFirst, firstDelta, captureAndGuardFirst, { + lockAcquired: holdFirstFileLock || options?.firstFileLockAcquired, + }) + let secondDiskSnapshot: HistoryItem | undefined + const secondDelta = this.buildDelta(secondId, second, updatedSecond) + const captureSecond = options?.rollbackBothOnCallbackFailure + ? (current: HistoryItem) => { + secondDiskSnapshot = structuredClone(current) + } + : undefined + let writtenSecond: HistoryItem + try { + writtenSecond = await this.writeTaskFile(mergedSecond, secondDelta, captureSecond) + } catch (error) { + if (options?.rollbackFirstOnSecondFailure && firstDiskSnapshot) { + try { + const rollbackSnapshot = firstDiskSnapshot + let restoredFirst = rollbackSnapshot + await safeWriteJson(await this.getTaskFilePath(firstId), rollbackSnapshot, { + lockAcquired: true, + merge: (existing) => { + if (!existing || typeof existing !== "object" || !("id" in existing)) { + throw new Error( + `[TaskHistoryStore] atomicUpdatePair: ${firstId} missing during rollback`, + ) + } + const current = existing as HistoryItem + const firstWriteStillCurrent = Object.entries(firstDelta).every(([key, value]) => + deepEqual((current as Record)[key], value), + ) + if (!firstWriteStillCurrent) { + throw new Error( + `[TaskHistoryStore] atomicUpdatePair: cannot roll back ${firstId} after a concurrent update`, + ) + } + restoredFirst = structuredClone(rollbackSnapshot) + return restoredFirst + }, + }) + this.cache.set(firstId, restoredFirst) + } catch (rollbackError) { + this.cache.set(firstId, writtenFirst) + throw new AggregateError( + [error, rollbackError], + `[TaskHistoryStore] atomicUpdatePair: second write and first-record rollback failed`, + ) + } + } else { + // First record is committed on disk. Update cache so it + // reflects disk state before propagating the error. + this.cache.set(firstId, writtenFirst) + } + throw error + } + + // Both disk writes succeeded — now update the cache. this.cache.set(firstId, writtenFirst) - throw error - } + this.cache.set(secondId, writtenSecond) - // Both disk writes succeeded — now update the cache. - this.cache.set(firstId, writtenFirst) - this.cache.set(secondId, writtenSecond) + const all = this.getAll() + try { + if (this.onWrite) await this.onWrite(all) + await options?.whileFirstFileLocked?.() + return all + } catch (error) { + if (!options?.rollbackBothOnCallbackFailure) throw error - const all = this.getAll() - if (this.onWrite) { - await this.onWrite(all) + const compensationErrors: unknown[] = [] + const persistedWrittenSecond = JSON.parse(JSON.stringify(writtenSecond)) as HistoryItem + const persistedWrittenFirst = JSON.parse(JSON.stringify(writtenFirst)) as HistoryItem + + // Both snapshots are captured by guarded writes before callback work can run. + try { + await this.restoreTaskFilePreImage( + secondId, + secondDiskSnapshot as HistoryItem, + persistedWrittenSecond, + false, + ) + } catch (compensationError) { + compensationErrors.push(compensationError) + } + + try { + await this.restoreTaskFilePreImage( + firstId, + firstDiskSnapshot as HistoryItem, + persistedWrittenFirst, + true, + ) + } catch (compensationError) { + compensationErrors.push(compensationError) + } + + if (this.onWrite) { + try { + await this.onWrite(this.getAll()) + } catch (compensationError) { + compensationErrors.push(compensationError) + } + } + + if (compensationErrors.length > 0) { + throw new AggregateError( + [error, ...compensationErrors], + `[TaskHistoryStore] atomicUpdatePair: callback and compensation failed`, + ) + } + throw error + } + } finally { + await releaseFirstFileLock() } - return all - }) + } + return options?.storeLockAcquired ? update() : this.withLock(update) } // ────────────────────────────── Private: Write lock ────────────────────────────── diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.crossInstanceDelegation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.crossInstanceDelegation.spec.ts new file mode 100644 index 0000000000..998a360a5d --- /dev/null +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.crossInstanceDelegation.spec.ts @@ -0,0 +1,1059 @@ +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" + +import type { HistoryItem } from "@roo-code/types" + +import { TaskHistoryStore, assertValidTransition } from "../TaskHistoryStore" + +vi.mock("../../../utils/storage", () => ({ + getStorageBasePath: vi.fn(async (defaultPath: string) => defaultPath), +})) + +const makeHistoryItem = (id: string, overrides: Partial): HistoryItem => ({ + id, + number: 1, + ts: Date.now(), + task: id, + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + workspace: "/test/workspace", + ...overrides, +}) + +type WriteTaskFile = ( + item: HistoryItem, + delta?: Partial, + diskGuard?: (current: HistoryItem) => void, + options?: { mergeChildIds?: boolean; lockAcquired?: boolean }, +) => Promise + +const getWriteTaskFile = (store: TaskHistoryStore): WriteTaskFile => { + const writeTaskFile: unknown = Reflect.get(store, "writeTaskFile") + if (typeof writeTaskFile !== "function") throw new TypeError("TaskHistoryStore.writeTaskFile is not callable") + return (item, delta, diskGuard, options) => Reflect.apply(writeTaskFile, store, [item, delta, diskGuard, options]) +} + +type RestoreTaskFilePreImage = ( + taskId: string, + preImage: HistoryItem, + expectedWritten: HistoryItem, + lockAcquired: boolean, +) => Promise + +const getRestoreTaskFilePreImage = (store: TaskHistoryStore): RestoreTaskFilePreImage => { + const restoreTaskFilePreImage: unknown = Reflect.get(store, "restoreTaskFilePreImage") + if (typeof restoreTaskFilePreImage !== "function") { + throw new TypeError("TaskHistoryStore.restoreTaskFilePreImage is not callable") + } + return (taskId, preImage, expectedWritten, lockAcquired) => + Reflect.apply(restoreTaskFilePreImage, store, [taskId, preImage, expectedWritten, lockAcquired]) +} + +describe("TaskHistoryStore cross-instance delegation", () => { + it("unions child IDs by default and replaces them only when explicitly requested", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-child-id-merge-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + const task = makeHistoryItem("parent", { childIds: ["cached-child"], tokensIn: 1 }) + await store.upsert(task) + const taskFile = path.join(storage, "tasks", "parent", "history_item.json") + const writeTaskFile = getWriteTaskFile(store) + + await fs.writeFile(taskFile, JSON.stringify({ ...task, childIds: ["peer-child"] })) + const unioned = await writeTaskFile( + { ...task, childIds: ["local-child"] }, + { id: task.id, childIds: ["local-child"] }, + ) + expect(unioned.childIds).toEqual(["peer-child", "local-child"]) + expect(JSON.parse(await fs.readFile(taskFile, "utf8")).childIds).toEqual(["peer-child", "local-child"]) + + await fs.writeFile(taskFile, JSON.stringify({ ...task, childIds: ["new-peer-child"] })) + const replaced = await writeTaskFile( + { ...task, childIds: ["replacement-child"] }, + { id: task.id, childIds: ["replacement-child"] }, + undefined, + { mergeChildIds: false }, + ) + expect(replaced.childIds).toEqual(["replacement-child"]) + + await fs.writeFile(taskFile, JSON.stringify({ ...task, childIds: ["preserved-child"] })) + const unrelatedUpdate = await writeTaskFile( + { ...task, tokensIn: 2 }, + { id: task.id, tokensIn: 2 }, + undefined, + { mergeChildIds: false }, + ) + expect(unrelatedUpdate).toMatchObject({ tokensIn: 2, childIds: ["preserved-child"] }) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("rejects a stale child completion before either delegation record is written", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-delegation-")) + const hostA = new TaskHistoryStore(storage) + const hostB = new TaskHistoryStore(storage) + const staleDelegationError = new Error("stale delegation") + + try { + await hostA.initialize() + await hostB.initialize() + await hostA.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child-old", + delegatedToId: "child-old", + childIds: ["child-old"], + }), + ) + await hostA.upsert(makeHistoryItem("child-old", { status: "active", parentTaskId: "parent" })) + await hostB.reconcile({ forceRefresh: true }) + + await hostB.atomicReadAndUpdate("child-old", (child) => ({ ...child, status: "interrupted" })) + await hostB.atomicReadAndUpdate("parent", (parent) => ({ + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + })) + await hostB.upsert(makeHistoryItem("child-new", { status: "active", parentTaskId: "parent" })) + await hostB.atomicReadAndUpdate("parent", (parent) => ({ + ...parent, + status: "delegated", + awaitingChildId: "child-new", + delegatedToId: "child-new", + childIds: [...(parent.childIds ?? []), "child-new"], + })) + + const assertStillAwaitingOldChild = (parent: HistoryItem) => { + if (parent.awaitingChildId !== "child-old") throw staleDelegationError + } + + await expect( + hostA.atomicUpdatePair( + "parent", + "child-old", + (parent) => { + assertStillAwaitingOldChild(parent) + assertValidTransition(parent.status, "active") + return { + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + completedByChildId: "child-old", + } + }, + (child) => ({ ...child, status: "completed" }), + { firstDiskGuard: assertStillAwaitingOldChild }, + ), + ).rejects.toBe(staleDelegationError) + + await hostB.invalidate("parent") + await hostB.invalidate("child-old") + await hostB.invalidate("child-new") + + expect(hostB.get("parent")).toMatchObject({ + status: "delegated", + awaitingChildId: "child-new", + delegatedToId: "child-new", + }) + expect(hostB.get("child-old")?.status).toBe("interrupted") + expect(hostB.get("child-new")).toMatchObject({ status: "active", parentTaskId: "parent" }) + } finally { + hostA.dispose() + hostB.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("restores the parent delegation when completing the child cannot be persisted", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-rollback-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + childIds: [], + }), + ) + await store.upsert(makeHistoryItem("child", { status: "active", parentTaskId: "parent" })) + const parentFile = path.join(storage, "tasks", "parent", "history_item.json") + const persistedParentBeforeFailure = JSON.parse(await fs.readFile(parentFile, "utf8")) + await fs.writeFile(parentFile, JSON.stringify({ ...persistedParentBeforeFailure, tokensIn: 99 })) + + const childDirectory = path.join(storage, "tasks", "child") + await fs.rm(childDirectory, { recursive: true }) + await fs.writeFile(childDirectory, "blocks child history writes", "utf8") + + await expect( + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + completedByChildId: "child", + childIds: [...(parent.childIds ?? []), "child"], + }), + (child) => ({ ...child, status: "completed" }), + { + firstDiskGuard: (parent) => { + if (parent.awaitingChildId !== "child") throw new Error("stale delegation") + }, + rollbackFirstOnSecondFailure: true, + }, + ), + ).rejects.toThrow() + + expect(store.get("parent")).toMatchObject({ + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + }) + expect(store.get("parent")?.completedByChildId).toBeUndefined() + expect(store.get("parent")?.childIds).toEqual([]) + expect(store.get("parent")?.tokensIn).toBe(99) + const persistedParent = JSON.parse(await fs.readFile(parentFile, "utf8")) + expect(persistedParent).toEqual(store.get("parent")) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("holds the parent lock through both writes and finite handoff work", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-lock-scope-")) + const hostA = new TaskHistoryStore(storage) + const hostB = new TaskHistoryStore(storage) + let releaseHandoff!: () => void + const handoffCanFinish = new Promise((resolve) => { + releaseHandoff = resolve + }) + let handoffStarted!: () => void + const handoffDidStart = new Promise((resolve) => { + handoffStarted = resolve + }) + const order: string[] = [] + + try { + await hostA.initialize() + await hostB.initialize() + await hostA.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child-old", + delegatedToId: "child-old", + childIds: ["child-old"], + }), + ) + await hostA.upsert(makeHistoryItem("child-old", { status: "active", parentTaskId: "parent" })) + await hostB.reconcile({ forceRefresh: true }) + await hostB.upsert(makeHistoryItem("child-new", { status: "active", parentTaskId: "parent" })) + + const completion = hostA.atomicUpdatePair( + "parent", + "child-old", + (parent) => ({ + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + completedByChildId: "child-old", + }), + (child) => ({ ...child, status: "completed" }), + { + firstDiskGuard: (parent) => { + if (parent.awaitingChildId !== "child-old") throw new Error("stale delegation") + }, + whileFirstFileLocked: async () => { + order.push("handoff-start") + handoffStarted() + await handoffCanFinish + order.push("handoff-end") + }, + }, + ) + + await handoffDidStart + let redelegationSettled = false + const redelegation = hostB + .atomicReadAndUpdate("parent", (parent) => ({ + ...parent, + status: "delegated", + awaitingChildId: "child-new", + delegatedToId: "child-new", + childIds: [...(parent.childIds ?? []), "child-new"], + })) + .then(() => { + redelegationSettled = true + order.push("redelegation-end") + }) + + await Promise.resolve() + expect(redelegationSettled).toBe(false) + + releaseHandoff() + await Promise.all([completion, redelegation]) + + expect(order).toEqual(["handoff-start", "handoff-end", "redelegation-end"]) + await hostA.invalidate("parent") + await hostA.invalidate("child-old") + expect(hostA.get("parent")).toMatchObject({ + status: "delegated", + awaitingChildId: "child-new", + delegatedToId: "child-new", + }) + expect(hostA.get("child-old")?.status).toBe("completed") + } finally { + hostA.dispose() + hostB.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("restores both authoritative records and write-through state when the lock-scoped callback fails", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-callback-compensation-")) + const onWrite = vi.fn().mockResolvedValue(undefined) + const store = new TaskHistoryStore(storage, { onWrite }) + const callbackError = new Error("completion handoff failed") + + try { + await store.initialize() + await store.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + childIds: ["child"], + }), + ) + await store.upsert(makeHistoryItem("child", { status: "active", parentTaskId: "parent" })) + const parentFile = path.join(storage, "tasks", "parent", "history_item.json") + const childFile = path.join(storage, "tasks", "child", "history_item.json") + const parentBefore = JSON.parse(await fs.readFile(parentFile, "utf8")) + const childBefore = JSON.parse(await fs.readFile(childFile, "utf8")) + const restoreTaskFilePreImage = getRestoreTaskFilePreImage(store) + const compensationLockStates: Array<[string, boolean]> = [] + Reflect.set(store, "restoreTaskFilePreImage", async (...args: Parameters) => { + compensationLockStates.push([args[0], args[3]]) + await restoreTaskFilePreImage(...args) + }) + onWrite.mockClear() + + await expect( + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + completedByChildId: "child", + }), + (child) => ({ ...child, status: "completed" }), + { + rollbackBothOnCallbackFailure: true, + whileFirstFileLocked: async () => { + throw callbackError + }, + }, + ), + ).rejects.toBe(callbackError) + + expect(JSON.parse(await fs.readFile(parentFile, "utf8"))).toEqual(parentBefore) + expect(JSON.parse(await fs.readFile(childFile, "utf8"))).toEqual(childBefore) + expect(store.get("parent")).toEqual(parentBefore) + expect(store.get("child")).toEqual(childBefore) + expect(compensationLockStates).toEqual([ + ["child", false], + ["parent", true], + ]) + expect(onWrite).toHaveBeenCalledTimes(2) + expect(onWrite.mock.calls[0][0]).toEqual( + expect.arrayContaining([ + expect.objectContaining({ id: "parent", status: "active" }), + expect.objectContaining({ id: "child", status: "completed" }), + ]), + ) + expect(onWrite.mock.calls[1][0]).toEqual(expect.arrayContaining([parentBefore, childBefore])) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("compensates when write-through rejects and preserves the original error", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-onwrite-compensation-")) + const onWrite = vi.fn().mockResolvedValue(undefined) + const store = new TaskHistoryStore(storage, { onWrite }) + const callbackError = new Error("write-through failed") + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "delegated" })) + await store.upsert(makeHistoryItem("child", { status: "active" })) + onWrite.mockClear() + onWrite.mockRejectedValueOnce(callbackError).mockResolvedValueOnce(undefined) + + await expect( + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active" }), + (child) => ({ ...child, status: "completed" }), + { rollbackBothOnCallbackFailure: true }, + ), + ).rejects.toBe(callbackError) + + expect(store.get("parent")?.status).toBe("delegated") + expect(store.get("child")?.status).toBe("active") + expect(onWrite).toHaveBeenCalledTimes(2) + expect(onWrite.mock.calls[1][0]).toEqual( + expect.arrayContaining([ + expect.objectContaining({ id: "parent", status: "delegated" }), + expect.objectContaining({ id: "child", status: "active" }), + ]), + ) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("aggregates callback and guarded compensation failures while reconciling partial cache state", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-compensation-guard-")) + const onWrite = vi.fn().mockResolvedValue(undefined) + const hostA = new TaskHistoryStore(storage, { onWrite }) + const hostB = new TaskHistoryStore(storage) + const callbackError = new Error("completion handoff failed") + const writeThroughError = new Error("compensated write-through failed") + + try { + await hostA.initialize() + await hostB.initialize() + await hostA.upsert(makeHistoryItem("parent", { status: "delegated", awaitingChildId: "child" })) + await hostA.upsert(makeHistoryItem("child", { status: "active", tokensIn: 1 })) + await hostB.reconcile({ forceRefresh: true }) + onWrite.mockClear() + onWrite.mockResolvedValueOnce(undefined).mockRejectedValueOnce(writeThroughError) + + const result = hostA.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active", awaitingChildId: undefined }), + (child) => ({ ...child, status: "completed" }), + { + rollbackBothOnCallbackFailure: true, + whileFirstFileLocked: async () => { + await hostB.atomicReadAndUpdate("child", (child) => ({ ...child, tokensIn: 9 })) + throw callbackError + }, + }, + ) + + await expect(result).rejects.toMatchObject({ + name: "AggregateError", + message: "[TaskHistoryStore] atomicUpdatePair: callback and compensation failed", + errors: [ + callbackError, + expect.objectContaining({ message: expect.stringContaining("concurrent update") }), + writeThroughError, + ], + }) + expect(hostA.get("parent")).toMatchObject({ status: "delegated", awaitingChildId: "child" }) + expect(hostA.get("child")).toMatchObject({ status: "completed", tokensIn: 9 }) + expect(onWrite.mock.calls.at(-1)?.[0]).toEqual( + expect.arrayContaining([ + expect.objectContaining({ id: "parent", status: "delegated" }), + expect.objectContaining({ id: "child", status: "completed", tokensIn: 9 }), + ]), + ) + } finally { + hostA.dispose() + hostB.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it.each([ + ["missing", undefined], + ["primitive", 42], + ["object without an id", { status: "completed" }], + ] as const)("rejects compensation when the second record is %s", async (_description, invalidRecord) => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-invalid-compensation-")) + const store = new TaskHistoryStore(storage) + const callbackError = new Error("completion handoff failed") + + try { + await store.initialize() + const parent = makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + }) + const child = makeHistoryItem("child", { status: "active", parentTaskId: "parent" }) + await store.upsert(parent) + await store.upsert(child) + const childFile = path.join(storage, "tasks", "child", "history_item.json") + + const result = store.atomicUpdatePair( + "parent", + "child", + (current) => ({ ...current, status: "active", awaitingChildId: undefined, delegatedToId: undefined }), + (current) => ({ ...current, status: "completed" }), + { + rollbackBothOnCallbackFailure: true, + whileFirstFileLocked: async () => { + if (invalidRecord === undefined) { + await fs.unlink(childFile) + } else { + await fs.writeFile(childFile, JSON.stringify(invalidRecord)) + } + throw callbackError + }, + }, + ) + + const aggregate = await result.catch((error: unknown) => error) + expect(aggregate).toBeInstanceOf(AggregateError) + expect((aggregate as AggregateError).message).toBe( + "[TaskHistoryStore] atomicUpdatePair: callback and compensation failed", + ) + expect((aggregate as AggregateError).errors[0]).toBe(callbackError) + expect((aggregate as AggregateError).errors[1]).toMatchObject({ + message: "[TaskHistoryStore] atomicUpdatePair: child missing during compensation", + }) + expect(store.get("parent")).toEqual(parent) + expect(store.get("child")).toBeUndefined() + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("reports failures from compensating both records and refreshes both cache entries", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-double-compensation-")) + const store = new TaskHistoryStore(storage) + const callbackError = new Error("completion handoff failed") + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "delegated", awaitingChildId: "child" })) + await store.upsert(makeHistoryItem("child", { status: "active", tokensIn: 1 })) + const parentFile = path.join(storage, "tasks", "parent", "history_item.json") + const childFile = path.join(storage, "tasks", "child", "history_item.json") + + const result = store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active", awaitingChildId: undefined }), + (child) => ({ ...child, status: "completed" }), + { + rollbackBothOnCallbackFailure: true, + whileFirstFileLocked: async () => { + const persistedParent = JSON.parse(await fs.readFile(parentFile, "utf8")) + const persistedChild = JSON.parse(await fs.readFile(childFile, "utf8")) + await fs.writeFile(parentFile, JSON.stringify({ ...persistedParent, tokensOut: 8 })) + await fs.writeFile(childFile, JSON.stringify({ ...persistedChild, tokensIn: 9 })) + throw callbackError + }, + }, + ) + + await expect(result).rejects.toMatchObject({ + name: "AggregateError", + errors: [ + callbackError, + expect.objectContaining({ message: expect.stringContaining("cannot compensate child") }), + expect.objectContaining({ message: expect.stringContaining("cannot compensate parent") }), + ], + }) + expect(store.get("parent")).toMatchObject({ status: "active", tokensOut: 8 }) + expect(store.get("child")).toMatchObject({ status: "completed", tokensIn: 9 }) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("keeps both writes committed when callback compensation was not requested", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-no-callback-compensation-")) + const store = new TaskHistoryStore(storage) + const callbackError = new Error("handoff failed without compensation") + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "delegated" })) + await store.upsert(makeHistoryItem("child", { status: "active" })) + + await expect( + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active" }), + (child) => ({ ...child, status: "completed" }), + { + whileFirstFileLocked: async () => { + throw callbackError + }, + }, + ), + ).rejects.toBe(callbackError) + expect(store.get("parent")?.status).toBe("active") + expect(store.get("child")?.status).toBe("completed") + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("recreates a missing first record when no disk guard or rollback was requested", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-unguarded-create-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "delegated" })) + await store.upsert(makeHistoryItem("child", { status: "active" })) + await fs.unlink(path.join(storage, "tasks", "parent", "history_item.json")) + + await store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active" }), + (child) => ({ ...child, status: "completed" }), + ) + + const persistedParent = JSON.parse( + await fs.readFile(path.join(storage, "tasks", "parent", "history_item.json"), "utf8"), + ) + expect(persistedParent).toMatchObject({ id: "parent", status: "active" }) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("preserves a write-through error without options and leaves both writes committed", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-onwrite-no-options-")) + const onWrite = vi.fn().mockResolvedValue(undefined) + const store = new TaskHistoryStore(storage, { onWrite }) + const writeThroughError = new Error("write-through failed without options") + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "delegated" })) + await store.upsert(makeHistoryItem("child", { status: "active" })) + onWrite.mockRejectedValueOnce(writeThroughError) + + await expect( + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active" }), + (child) => ({ ...child, status: "completed" }), + ), + ).rejects.toBe(writeThroughError) + expect(store.get("parent")?.status).toBe("active") + expect(store.get("child")?.status).toBe("completed") + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("keeps the first write committed when only a disk guard was requested", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-guard-without-rollback-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "delegated", awaitingChildId: "child" })) + await store.upsert(makeHistoryItem("child", { status: "active" })) + const writeTaskFile = getWriteTaskFile(store) + let writeCount = 0 + Reflect.set(store, "writeTaskFile", async (...args: Parameters) => { + writeCount++ + if (writeCount === 2) throw new Error("child write failed") + return writeTaskFile(...args) + }) + + await expect( + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active", awaitingChildId: undefined }), + (child) => ({ ...child, status: "completed" }), + { firstDiskGuard: () => {} }, + ), + ).rejects.toThrow("child write failed") + expect(store.get("parent")?.status).toBe("active") + expect(store.get("parent")?.awaitingChildId).toBeUndefined() + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("refreshes stale parent state before a lock-scoped update without re-entering either lock", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-lock-refresh-")) + const hostA = new TaskHistoryStore(storage) + const hostB = new TaskHistoryStore(storage) + + try { + await hostA.initialize() + await hostB.initialize() + await hostA.upsert(makeHistoryItem("parent", { status: "active", tokensIn: 1 })) + await hostB.reconcile({ forceRefresh: true }) + await hostB.atomicReadAndUpdate("parent", (parent) => ({ ...parent, tokensIn: 2 })) + + expect(hostA.get("parent")?.tokensIn).toBe(1) + await hostA.withTaskFileLock("parent", async () => { + expect(hostA.get("parent")?.tokensIn).toBe(2) + await hostA.atomicReadAndUpdate( + "parent", + (parent) => ({ ...parent, status: "delegated", awaitingChildId: "child" }), + { fileLockAcquired: true, storeLockAcquired: true }, + ) + }) + + await hostB.invalidate("parent") + expect(hostB.get("parent")).toMatchObject({ + tokensIn: 2, + status: "delegated", + awaitingChildId: "child", + }) + } finally { + hostA.dispose() + hostB.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("refuses to roll back the parent over an intervening first-record change", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-rollback-guard-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + childIds: ["child"], + }), + ) + await store.upsert(makeHistoryItem("child", { status: "active", parentTaskId: "parent" })) + + const writeTaskFile = getWriteTaskFile(store) + let pairWrite = 0 + const replacement: WriteTaskFile = async (item, delta, diskGuard, options) => { + pairWrite++ + if (pairWrite === 1) { + const written = await writeTaskFile(item, delta, diskGuard, options) + const parentFile = path.join(storage, "tasks", "parent", "history_item.json") + await fs.writeFile(parentFile, JSON.stringify({ ...written, completedByChildId: "peer-child" })) + return written + } + throw new Error("child write failed") + } + Reflect.set(store, "writeTaskFile", replacement) + + const result = store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + completedByChildId: "child", + }), + (child) => ({ ...child, status: "completed" }), + { rollbackFirstOnSecondFailure: true }, + ) + await expect(result).rejects.toMatchObject({ + name: "AggregateError", + message: "[TaskHistoryStore] atomicUpdatePair: second write and first-record rollback failed", + errors: [ + expect.objectContaining({ message: "child write failed" }), + expect.objectContaining({ + message: + "[TaskHistoryStore] atomicUpdatePair: cannot roll back parent after a concurrent update", + }), + ], + }) + + const persistedParent = JSON.parse( + await fs.readFile(path.join(storage, "tasks", "parent", "history_item.json"), "utf8"), + ) + expect(persistedParent.completedByChildId).toBe("peer-child") + expect(store.get("parent")).toMatchObject({ status: "active", completedByChildId: "child" }) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("rejects a guarded pair update when the authoritative parent record disappeared", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-missing-parent-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + }), + ) + await store.upsert(makeHistoryItem("child", { status: "active", parentTaskId: "parent" })) + await fs.unlink(path.join(storage, "tasks", "parent", "history_item.json")) + + await expect( + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active", awaitingChildId: undefined }), + (child) => ({ ...child, status: "completed" }), + { firstDiskGuard: () => {} }, + ), + ).rejects.toThrow("guarded write: task parent not found on disk") + expect(store.get("parent")?.status).toBe("delegated") + expect(store.get("child")?.status).toBe("active") + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("rejects an atomic updater that changes the task identity", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-id-guard-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "active" })) + + await expect( + store.atomicReadAndUpdate("parent", (parent) => ({ ...parent, id: "replacement" })), + ).rejects.toThrow("changed id from parent to replacement") + expect(store.get("parent")?.id).toBe("parent") + expect(store.get("replacement")).toBeUndefined() + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("rejects an atomic update for a task missing from the local cache", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-missing-cache-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await expect(store.atomicReadAndUpdate("missing", (item) => item)).rejects.toThrow( + "task missing not found in cache", + ) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("recreates a missing task file from cached state and publishes the update", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-cached-fallback-")) + const onWrite = vi.fn().mockResolvedValue(undefined) + const store = new TaskHistoryStore(storage, { onWrite }) + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "active", tokensIn: 1 })) + await fs.unlink(path.join(storage, "tasks", "parent", "history_item.json")) + onWrite.mockClear() + + await store.atomicReadAndUpdate("parent", (parent) => ({ ...parent, tokensIn: 2 })) + + expect(onWrite).toHaveBeenCalledTimes(1) + expect(store.get("parent")?.tokensIn).toBe(2) + const persisted = JSON.parse( + await fs.readFile(path.join(storage, "tasks", "parent", "history_item.json"), "utf8"), + ) + expect(persisted).toMatchObject({ id: "parent", tokensIn: 2 }) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("keeps the cached snapshot available when a locked task file is missing", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-missing-locked-file-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: "active", tokensIn: 3 })) + await fs.unlink(path.join(storage, "tasks", "parent", "history_item.json")) + + const tokensIn = await store.withTaskFileLock("parent", async () => store.get("parent")?.tokensIn) + + expect(tokensIn).toBe(3) + expect(store.get("parent")?.tokensIn).toBe(3) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("treats a legacy missing status as active during an atomic transition", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-legacy-status-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert(makeHistoryItem("parent", { status: undefined })) + + await store.atomicReadAndUpdate("parent", (parent) => ({ + ...parent, + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + })) + + expect(store.get("parent")).toMatchObject({ + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + }) + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it("runs pair write-through inside an already-held parent transition lock", async () => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-held-pair-lock-")) + const onWrite = vi.fn().mockResolvedValue(undefined) + const store = new TaskHistoryStore(storage, { onWrite }) + + try { + await store.initialize() + await store.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + }), + ) + await store.upsert(makeHistoryItem("child", { status: "active", parentTaskId: "parent" })) + onWrite.mockClear() + + await store.withTaskFileLock("parent", () => + store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ + ...parent, + status: "active", + awaitingChildId: undefined, + delegatedToId: undefined, + }), + (child) => ({ ...child, status: "completed" }), + { + firstDiskGuard: (parent) => { + expect(parent.awaitingChildId).toBe("child") + }, + firstFileLockAcquired: true, + storeLockAcquired: true, + }, + ), + ) + + expect(onWrite).toHaveBeenCalledTimes(1) + expect(store.get("parent")?.status).toBe("active") + expect(store.get("child")?.status).toBe("completed") + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }) + + it.each([ + ["disappears", undefined], + ["becomes a primitive", 42], + ["loses its id", { status: "active" }], + ] as const)( + "surfaces rollback failure when the first record %s after its write", + async (_description, invalidRecord) => { + const storage = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-missing-rollback-")) + const store = new TaskHistoryStore(storage) + + try { + await store.initialize() + await store.upsert( + makeHistoryItem("parent", { + status: "delegated", + awaitingChildId: "child", + delegatedToId: "child", + }), + ) + await store.upsert(makeHistoryItem("child", { status: "active", parentTaskId: "parent" })) + + const writeTaskFile = getWriteTaskFile(store) + let pairWrite = 0 + const replacement: WriteTaskFile = async (item, delta, diskGuard, options) => { + pairWrite++ + if (pairWrite === 1) { + const written = await writeTaskFile(item, delta, diskGuard, options) + const parentFile = path.join(storage, "tasks", "parent", "history_item.json") + if (invalidRecord === undefined) { + await fs.unlink(parentFile) + } else { + await fs.writeFile(parentFile, JSON.stringify(invalidRecord)) + } + return written + } + throw new Error("child write failed") + } + Reflect.set(store, "writeTaskFile", replacement) + + const result = store.atomicUpdatePair( + "parent", + "child", + (parent) => ({ ...parent, status: "active", awaitingChildId: undefined }), + (child) => ({ ...child, status: "completed" }), + { rollbackFirstOnSecondFailure: true }, + ) + await expect(result).rejects.toMatchObject({ + name: "AggregateError", + message: "[TaskHistoryStore] atomicUpdatePair: second write and first-record rollback failed", + errors: [ + expect.objectContaining({ message: "child write failed" }), + expect.objectContaining({ + message: "[TaskHistoryStore] atomicUpdatePair: parent missing during rollback", + }), + ], + }) + expect(store.get("parent")?.status).toBe("active") + } finally { + store.dispose() + await fs.rm(storage, { recursive: true, force: true }) + } + }, + ) +}) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.realConcurrency.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.realConcurrency.spec.ts index d94ca8f782..9bec5067f7 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.realConcurrency.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.realConcurrency.spec.ts @@ -77,24 +77,20 @@ describe("TaskHistoryStore real cross-host locking", () => { const storagePath = await fs.mkdtemp(path.join(os.tmpdir(), "task-history-real-lock-")) const storeA = new TaskHistoryStore(storagePath) const storeB = new TaskHistoryStore(storagePath) - let writeBarrier: WriteBarrier | undefined try { await storeA.initialize() await storeA.upsert(item("shared-task")) await storeB.initialize() - writeBarrier = synchronizeNextWrites([storeA, storeB]) await Promise.all([ storeA.atomicReadAndUpdate("shared-task", (current) => ({ ...current, mode: "architect" })), storeB.atomicReadAndUpdate("shared-task", (current) => ({ ...current, totalCost: 42 })), ]) - expect(writeBarrier.arrivals()).toBe(2) await storeA.invalidate("shared-task") expect(storeA.get("shared-task")).toMatchObject({ mode: "architect", totalCost: 42 }) } finally { - writeBarrier?.dispose() storeA.dispose() storeB.dispose() await fs.rm(storagePath, { recursive: true, force: true }) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts index e37fd1a25e..86630b500c 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.ts @@ -20,7 +20,10 @@ const writeJson = async (filePath: string, data: unknown): Promise => { const safeWriteJsonMock = vi.hoisted(() => vi.fn()) -vi.mock("../../../utils/safeWriteJson", () => ({ safeWriteJson: safeWriteJsonMock })) +vi.mock("../../../utils/safeWriteJson", () => ({ + lockJsonFile: vi.fn().mockResolvedValue(async () => {}), + safeWriteJson: safeWriteJsonMock, +})) safeWriteJsonMock.mockImplementation(writeJson) diff --git a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts index 3e277ac867..078e9a11d4 100644 --- a/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts +++ b/src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts @@ -6,9 +6,10 @@ import * as os from "os" import type { HistoryItem } from "@roo-code/types" -import { TaskHistoryStore, assertValidTransition } from "../TaskHistoryStore" +import { TaskHistoryStore, assertValidTransition, type AtomicUpdatePairOptions } from "../TaskHistoryStore" import { GlobalFileNames } from "../../../shared/globalFileNames" import { ClineProvider } from "../../webview/ClineProvider" +import { lockJsonFile, safeWriteJson } from "../../../utils/safeWriteJson" vi.mock("../../../utils/storage", () => ({ getStorageBasePath: vi.fn().mockImplementation((defaultPath: string) => { @@ -18,6 +19,7 @@ vi.mock("../../../utils/storage", () => ({ // Mock safeWriteJson to use plain fs writes in tests (avoids proper-lockfile issues) vi.mock("../../../utils/safeWriteJson", () => ({ + lockJsonFile: vi.fn().mockResolvedValue(async () => {}), safeWriteJson: vi.fn().mockImplementation(async (filePath: string, data: any) => { await fs.mkdir(path.dirname(filePath), { recursive: true }) await fs.writeFile(filePath, JSON.stringify(data, null, "\t"), "utf8") @@ -577,7 +579,78 @@ describe("TaskHistoryStore", () => { }) }) + describe("withTaskFileLock()", () => { + it("releases the file lock when the callback rejects", async () => { + await store.initialize() + await store.upsert(makeHistoryItem({ id: "locked-callback", status: "active" })) + const release = vi.fn().mockResolvedValue(undefined) + vi.mocked(lockJsonFile).mockResolvedValueOnce(release) + const callbackError = new Error("locked callback failed") + + await expect( + store.withTaskFileLock("locked-callback", async () => { + throw callbackError + }), + ).rejects.toBe(callbackError) + expect(release).toHaveBeenCalledTimes(1) + }) + + it("treats an explicit active status as a no-op for a legacy record", async () => { + await store.initialize() + await store.upsert(makeHistoryItem({ id: "legacy-active", status: undefined })) + + await expect( + store.atomicReadAndUpdate("legacy-active", (current) => ({ ...current, status: "active" })), + ).resolves.toEqual([expect.objectContaining({ id: "legacy-active", status: "active" })]) + }) + }) + describe("atomicUpdatePair()", () => { + it("does not claim the first file lock when no lock-scoped option is enabled", async () => { + await store.initialize() + await store.upsert(makeHistoryItem({ id: "first-unlocked", status: "active" })) + await store.upsert(makeHistoryItem({ id: "second-unlocked", status: "active" })) + vi.mocked(lockJsonFile).mockClear() + vi.mocked(safeWriteJson).mockClear() + + await store.atomicUpdatePair( + "first-unlocked", + "second-unlocked", + (first) => ({ ...first, status: "completed" }), + (second) => ({ ...second, status: "completed" }), + ) + + expect(lockJsonFile).not.toHaveBeenCalled() + expect(vi.mocked(safeWriteJson).mock.calls[0]?.[2]).toMatchObject({ lockAcquired: undefined }) + }) + + it.each([ + ["disk guard", { firstDiskGuard: () => {} }], + ["second-write rollback", { rollbackFirstOnSecondFailure: true }], + ["callback compensation", { rollbackBothOnCallbackFailure: true }], + ["lock-scoped callback", { whileFirstFileLocked: async () => {} }], + ] satisfies Array<[string, AtomicUpdatePairOptions]>)( + "holds the first file lock when only the %s option is enabled", + async (_description, options) => { + await store.initialize() + await store.upsert(makeHistoryItem({ id: "first-locked", status: "active" })) + await store.upsert(makeHistoryItem({ id: "second-locked", status: "active" })) + vi.mocked(lockJsonFile).mockClear() + vi.mocked(safeWriteJson).mockClear() + + await store.atomicUpdatePair( + "first-locked", + "second-locked", + (first) => ({ ...first, status: "completed" }), + (second) => ({ ...second, status: "completed" }), + options, + ) + + expect(lockJsonFile).toHaveBeenCalledTimes(1) + expect(vi.mocked(safeWriteJson).mock.calls[0]?.[2]).toMatchObject({ lockAcquired: true }) + }, + ) + it("updates both records and both files are written before lock releases", async () => { await store.initialize() diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 37281a9010..a34b0849af 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1017,9 +1017,9 @@ export class Task extends EventEmitter implements TaskLike { // For API requests, consecutive same-role messages are merged via mergeConsecutiveApiMessages() // so rewind/edit behavior can still reference original message boundaries. - async overwriteApiConversationHistory(newHistory: ApiMessage[]) { + async overwriteApiConversationHistory(newHistory: ApiMessage[], options: { persist?: boolean } = {}) { this.apiConversationHistory = newHistory - await this.saveApiConversationHistory() + if (options.persist !== false) await this.saveApiConversationHistory() } /** @@ -1178,10 +1178,10 @@ export class Task extends EventEmitter implements TaskLike { } } - public async overwriteClineMessages(newMessages: ClineMessage[]) { + public async overwriteClineMessages(newMessages: ClineMessage[], options: { persist?: boolean } = {}) { this.clineMessages = newMessages restoreTodoListForTask(this) - await this.saveClineMessages() + if (options.persist !== false) await this.saveClineMessages() // When overwriting messages (e.g., during task resume), repopulate the cloud sync tracking Set // with timestamps from all non-partial messages to prevent re-syncing previously synced messages @@ -2464,7 +2464,7 @@ export class Task extends EventEmitter implements TaskLike { this.debouncedEmitTokenUsage.flush() } - public async abortTask(isAbandoned = false) { + public async abortTask(isAbandoned = false, options: { saveMessages?: boolean } = {}) { // Aborting task // Will stop any autonomously running promises. @@ -2498,6 +2498,7 @@ export class Task extends EventEmitter implements TaskLike { console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) // Don't rethrow - we want abort to always succeed } + if (options.saveMessages === false) return // Guard: a history task whose message load has not finished yet has // clineMessages = []. Saving now would call taskMetadata() with an // empty array, which writes the "no messages" placeholder as the diff --git a/src/core/task/__tests__/Task.persistence.spec.ts b/src/core/task/__tests__/Task.persistence.spec.ts index 671bd7d4b7..d04b2c1e31 100644 --- a/src/core/task/__tests__/Task.persistence.spec.ts +++ b/src/core/task/__tests__/Task.persistence.spec.ts @@ -387,6 +387,74 @@ describe("Task persistence", () => { }) }) + describe("overwrite persistence options", () => { + it.each([ + ["omitted", undefined], + ["true", true], + ] as const)("persists API history when persist is %s", async (_label, persist) => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const messages = [{ role: "user" as const, content: [{ type: "text" as const, text: "replacement" }] }] + + await task.overwriteApiConversationHistory(messages, persist === undefined ? {} : { persist }) + + expect(task.apiConversationHistory).toBe(messages) + expect(mockSaveApiMessages).toHaveBeenCalledTimes(1) + }) + + it("does not persist API history when persist is false", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const messages = [{ role: "user" as const, content: [{ type: "text" as const, text: "replacement" }] }] + + await task.overwriteApiConversationHistory(messages, { persist: false }) + + expect(task.apiConversationHistory).toBe(messages) + expect(mockSaveApiMessages).not.toHaveBeenCalled() + }) + + it.each([ + ["omitted", undefined], + ["true", true], + ] as const)("persists Cline messages when persist is %s", async (_label, persist) => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const messages = [{ type: "say" as const, say: "text" as const, text: "replacement", ts: 1 }] + + await task.overwriteClineMessages(messages, persist === undefined ? {} : { persist }) + + expect(task.clineMessages).toBe(messages) + expect(mockSaveTaskMessages).toHaveBeenCalledTimes(1) + }) + + it("does not persist Cline messages when persist is false", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const messages = [{ type: "say" as const, say: "text" as const, text: "replacement", ts: 1 }] + + await task.overwriteClineMessages(messages, { persist: false }) + + expect(task.clineMessages).toBe(messages) + expect(mockSaveTaskMessages).not.toHaveBeenCalled() + }) + }) + // ── saveClineMessages ──────────────────────────────────────────────── describe("saveClineMessages", () => { @@ -495,6 +563,20 @@ describe("Task persistence", () => { // ── abortTask history hydration guard ───────────────────────────────── describe("abortTask", () => { + it("does not mark a normally aborted task as abandoned", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "New task", + startTask: false, + }) + + await task.abortTask() + + expect(task.abort).toBe(true) + expect(task.abandoned).toBe(false) + }) + it("skips persistence when a history task aborts before messages load", async () => { const messagesDeferred = createDeferred() mockReadTaskMessages.mockReturnValueOnce(messagesDeferred.promise) @@ -586,6 +668,23 @@ describe("Task persistence", () => { expect(saveClineMessagesSpy).toHaveBeenCalledTimes(1) expect(mockSaveTaskMessages).toHaveBeenCalledTimes(1) }) + + it("can abort a completed handoff without persisting stale messages", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "Completed delegated child", + startTask: false, + }) + const saveClineMessagesSpy = vi.spyOn(getTaskPersistenceAccess(task), "saveClineMessages") + + await task.abortTask(true, { saveMessages: false }) + + expect(saveClineMessagesSpy).not.toHaveBeenCalled() + expect(mockSaveTaskMessages).not.toHaveBeenCalled() + expect(task.abort).toBe(true) + expect(task.abandoned).toBe(true) + }) }) // ── resumeTaskFromHistory — interrupted tool calls must be recorded as errors ── diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0a251aba5f..2d68fc8887 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -110,6 +110,7 @@ import { Task } from "../task/Task" import { webviewMessageHandler } from "./webviewMessageHandler" import type { ClineMessage, TodoItem } from "@roo-code/types" import { + type ApiMessage, readApiMessages, saveApiMessages, saveTaskMessages, @@ -239,6 +240,25 @@ export class ClineProvider return runDelegationTransition(this.delegationTransitionLocks, parentTaskId, fn) } + private runLockedDelegationTransition( + parentTaskId: string, + transition: () => Promise, + afterUnlock?: (result: T) => Promise, + afterUnlockError?: (error: unknown) => Promise, + ): Promise { + return this.runDelegationTransition(parentTaskId, async () => { + let result: T + try { + result = await this.taskHistoryStore.withTaskFileLock(parentTaskId, transition) + } catch (error) { + await afterUnlockError?.(error) + throw error + } + await afterUnlock?.(result) + return result + }) + } + private enqueueProviderProfileMutation(fn: (signal: AbortSignal) => Promise): Promise { const controller = new AbortController() // Run fn after either outcome so a rejected mutation never poisons the queue. @@ -597,7 +617,7 @@ export class ClineProvider // Removes and destroys the top Cline instance (the current finished task), // activating the previous one (resuming the parent task). - async removeClineFromStack() { + async removeClineFromStack(options: { saveMessages?: boolean } = {}) { if (this.taskRegistry.length === 0) { return } @@ -614,7 +634,11 @@ export class ClineProvider try { // Abort the running task and set isAbandoned to true so // all running promises will exit as well. - await task.abortTask(true) + if (options.saveMessages === false) { + await task.abortTask(true, options) + } else { + await task.abortTask(true) + } } catch (e) { this.log( `[ClineProvider#removeClineFromStack] abortTask() failed ${task.taskId}.${task.instanceId}: ${e.message}`, @@ -3812,9 +3836,6 @@ export class ClineProvider }): Promise { const { parentTaskId, message, initialTodos, mode, pendingActionId } = params - // Metadata-driven delegation is always enabled - - // 1) Get parent (must be current task) const parent = this.getCurrentTask() if (!parent) { throw new Error("[delegateParentAndOpenChild] No current task") @@ -3832,23 +3853,12 @@ export class ClineProvider ) } } - // 2) Flush pending tool results to API history BEFORE disposing the parent. - // This is critical: when tools are called before new_task, - // their tool_result blocks are in userMessageContent but not yet saved to API history. - // If we don't flush them, the parent's API conversation will be incomplete and - // cause 400 errors when resumed (missing tool_result for tool_use blocks). - // - // NOTE: We do NOT pass the assistant message here because the assistant message - // is already added to apiConversationHistory by the normal flow in - // recursivelyMakeClineRequests BEFORE tools start executing. We only need to - // flush the pending user message with tool_results. + try { const flushSuccess = await parent.flushPendingToolResultsToHistory() - if (!flushSuccess) { console.warn(`[delegateParentAndOpenChild] Flush failed for parent ${parentTaskId}, retrying...`) const retrySuccess = await parent.retrySaveApiConversationHistory() - if (!retrySuccess) { console.error( `[delegateParentAndOpenChild] CRITICAL: Parent ${parentTaskId} API history not persisted to disk. Child return may produce stale state.`, @@ -3866,9 +3876,6 @@ export class ClineProvider ) } - // 3) Enforce single-open invariant by closing/disposing the parent first - // This ensures we never have >1 tasks open at any time during delegation. - // Await abort completion to ensure clean disposal and prevent unhandled rejections. try { await this.removeClineFromStack() } catch (error) { @@ -3877,13 +3884,8 @@ export class ClineProvider error instanceof Error ? error.message : String(error) }`, ) - // Non-fatal: proceed with child creation even if parent cleanup had issues } - // 3) Switch provider mode to child's requested mode BEFORE creating the child task - // This ensures the child's system prompt and configuration are based on the correct mode. - // The mode switch must happen before createTask() because the Task constructor - // initializes its mode from provider.getState() during initializeTaskMode(). try { await this.handleModeSwitch(mode as any) } catch (e) { @@ -3894,52 +3896,36 @@ export class ClineProvider ) } - // 4) Create child as sole active (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. - // - // Pass startTask: false to prevent the child from beginning its task loop - // (and writing to globalState via saveClineMessages → updateTaskHistory) - // before we persist the parent's delegation metadata in step 5. - // 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, { initialTodos, initialStatus: "active", startTask: false, }) - // 5) Persist parent delegation metadata BEFORE the child starts writing. - // atomicReadAndUpdate reads from the in-memory cache and writes back within a - // single lock acquisition — no concurrent writer can slip between the read and - // write, and the pure updater cannot re-enter the lock (no deadlock). - // Broadcast and cache invalidation happen outside the lock after it releases. - // - // If the parent is already "delegated" to a previous interrupted child (the user - // navigated back to the parent and continued working), we implicitly sever the old - // link here (delegated → active → delegated) so no explicit Abandon step is needed. - // The old awaited child's status is re-read INSIDE the updater (which runs - // synchronously under the store lock) so a concurrent abandon or completion cannot - // slip between the status snapshot and the write. An active child must never be - // silently detached. try { - await this.taskHistoryStore.atomicReadAndUpdate(parentTaskId, (historyItem) => { - if (pendingActionId && historyItem.pendingAction?.actionId !== pendingActionId) { - throw new Error( - `[delegateParentAndOpenChild] Pending action mismatch for parent ${parentTaskId}: expected ${pendingActionId}, found ${historyItem.pendingAction?.actionId}`, - ) - } - const awaitedChildStatus = historyItem.awaitingChildId - ? this.taskHistoryStore.get(historyItem.awaitingChildId)?.status - : undefined - const delegated = delegateTaskToChild(historyItem, child.taskId, awaitedChildStatus) - return { - ...delegated, - pendingAction: - delegated.pendingAction?.actionId === pendingActionId ? undefined : delegated.pendingAction, - } + await this.taskHistoryStore.withTaskFileLock(parentTaskId, async () => { + await this.taskHistoryStore.atomicReadAndUpdate( + parentTaskId, + (historyItem) => { + if (pendingActionId && historyItem.pendingAction?.actionId !== pendingActionId) { + throw new Error( + `[delegateParentAndOpenChild] Pending action mismatch for parent ${parentTaskId}: expected ${pendingActionId}, found ${historyItem.pendingAction?.actionId}`, + ) + } + const awaitedChildStatus = historyItem.awaitingChildId + ? this.taskHistoryStore.get(historyItem.awaitingChildId)?.status + : undefined + const delegated = delegateTaskToChild(historyItem, child.taskId, awaitedChildStatus) + return { + ...delegated, + pendingAction: + delegated.pendingAction?.actionId === pendingActionId + ? undefined + : delegated.pendingAction, + } + }, + { fileLockAcquired: true, storeLockAcquired: true }, + ) }) this.recentTasksCache = undefined if (this.isViewLaunched) { @@ -3955,8 +3941,6 @@ export class ClineProvider }`, ) try { - // Only pop the stack if the child we just created is still on top. - // A concurrent delegation could have pushed another child since we created ours. if (this.getCurrentTask()?.taskId === child.taskId) { await this.removeClineFromStack() } @@ -3989,10 +3973,7 @@ export class ClineProvider throw err } - // 6) Start the child task now that parent metadata is safely persisted. scheduleTask(this.taskScheduler, child, "delegateParentAndOpenChild") - - // 7) Emit TaskDelegated (provider-level) try { this.emit(RooCodeEventName.TaskDelegated, parentTaskId, child.taskId) } catch { @@ -4012,11 +3993,12 @@ export class ClineProvider pendingActionId?: string }): Promise { const { parentTaskId, childTaskId, completionResultSummary, pendingActionId } = params - return this.runDelegationTransition(parentTaskId, async () => { + let parentToResume: Task | undefined + let childToRestore: HistoryItem | undefined + const transition = async () => { const globalStoragePath = this.contextProxy.globalStorageUri.fsPath - - // 1) Load parent from history and current persisted messages const { historyItem } = await this.getTaskWithId(parentTaskId) + const refreshedParent = this.taskHistoryStore.get(parentTaskId) const childHistory = this.taskHistoryStore.get(childTaskId) if (pendingActionId && childHistory?.pendingAction?.actionId !== pendingActionId) { this.log( @@ -4032,6 +4014,7 @@ export class ClineProvider // routing output back would corrupt an unrelated task. if ( this.cancelledDelegationChildIds.has(childTaskId) || + (refreshedParent?.status === "delegated" && refreshedParent.awaitingChildId !== childTaskId) || (historyItem.status !== "delegated" && historyItem.status !== "active") || historyItem.awaitingChildId !== childTaskId ) { @@ -4042,33 +4025,20 @@ export class ClineProvider return false } - let parentClineMessages: ClineMessage[] = [] - try { - parentClineMessages = await readTaskMessages({ - taskId: parentTaskId, - globalStoragePath, - }) - } catch { - parentClineMessages = [] - } + const originalParentClineMessages: ClineMessage[] = await readTaskMessages({ + taskId: parentTaskId, + globalStoragePath, + }) - let parentApiMessages: any[] = [] - try { - parentApiMessages = (await readApiMessages({ - taskId: parentTaskId, - globalStoragePath, - })) as any[] - } catch { - parentApiMessages = [] - } + const originalParentApiMessages: ApiMessage[] = await readApiMessages({ + taskId: parentTaskId, + globalStoragePath, + }) - // 2) Inject synthetic records: UI subtask_result and update API tool_result + const parentClineMessages = structuredClone(originalParentClineMessages) + const parentApiMessages = structuredClone(originalParentApiMessages) const ts = Date.now() - // Defensive: ensure arrays - if (!Array.isArray(parentClineMessages)) parentClineMessages = [] - if (!Array.isArray(parentApiMessages)) parentApiMessages = [] - const subtaskUiMessage: ClineMessage = { type: "say", say: "subtask_result", @@ -4083,8 +4053,6 @@ export class ClineProvider ) { parentClineMessages.push(subtaskUiMessage) } - await saveTaskMessages({ messages: parentClineMessages, taskId: parentTaskId, globalStoragePath }) - // Find the tool_use_id from the last assistant message's new_task tool_use let toolUseId: string | undefined for (let i = parentApiMessages.length - 1; i >= 0; i--) { @@ -4168,46 +4136,132 @@ export class ClineProvider } } - await saveApiMessages({ messages: parentApiMessages as any, taskId: parentTaskId, globalStoragePath }) + const restoreConversationFiles = async (cause: unknown): Promise => { + const restorationResults = await Promise.allSettled([ + saveTaskMessages({ + messages: originalParentClineMessages, + taskId: parentTaskId, + globalStoragePath, + }), + saveApiMessages({ + messages: originalParentApiMessages, + taskId: parentTaskId, + globalStoragePath, + }), + ]) + const restorationErrors = restorationResults.flatMap((restorationResult) => + restorationResult.status === "rejected" ? [restorationResult.reason] : [], + ) + if (restorationErrors.length > 0) { + throw new AggregateError( + [cause, ...restorationErrors], + `[reopenParentFromDelegation] Failed to restore parent ${parentTaskId} conversation files`, + ) + } + } - // 4) Close child instance if still open (single-open-task invariant). - // This MUST happen BEFORE marking the child "completed" because - // removeClineFromStack() → abortTask(true) → saveClineMessages() writes - // the historyItem with initialStatus (typically "active"), which would - // overwrite a "completed" status set later. - const current = this.getCurrentTask() - if (current?.taskId === childTaskId) { - await this.removeClineFromStack() + try { + await saveTaskMessages({ + messages: parentClineMessages, + taskId: parentTaskId, + globalStoragePath, + }) + await saveApiMessages({ + messages: parentApiMessages, + taskId: parentTaskId, + globalStoragePath, + }) + } catch (error) { + await restoreConversationFiles(error) + throw error } - // 3+5) Atomically mark child completed and parent active in one lock acquisition. - // No intermediate state is ever persisted — no sentinel needed. - // Build the parent update inside the updater from the locked snapshot so - // any concurrent write that landed between step 1 and the lock acquisition - // is preserved rather than silently overwritten. let updatedHistory!: typeof historyItem - let completingChild!: HistoryItem - await this.taskHistoryStore.atomicUpdatePair( - childTaskId, - parentTaskId, - (child) => { - if (pendingActionId && child.pendingAction?.actionId !== pendingActionId) { - throw new Error(`[reopenParentFromDelegation] Pending action mismatch for child ${childTaskId}`) - } - completingChild = { ...child } - const lifecycleUpdate = completeDelegatedChild(historyItem, child, completionResultSummary) - return { - ...lifecycleUpdate.child, - pendingAction: - child.pendingAction?.actionId === pendingActionId ? undefined : child.pendingAction, - } - }, - (parent) => { - const lifecycleUpdate = completeDelegatedChild(parent, completingChild, completionResultSummary) - updatedHistory = lifecycleUpdate.parent - return updatedHistory - }, - ) + const staleDelegationError = new Error("stale cross-instance delegation") + const assertCurrentDelegation = (parent: HistoryItem) => { + if ( + (parent.status !== "delegated" && parent.status !== "active") || + parent.awaitingChildId !== childTaskId + ) { + throw staleDelegationError + } + } + try { + let parentInstance: Task | undefined + let completingParent!: HistoryItem + let completingChild!: HistoryItem + const completionOptions = { + firstDiskGuard: assertCurrentDelegation, + rollbackFirstOnSecondFailure: true, + rollbackBothOnCallbackFailure: true, + firstFileLockAcquired: true, + storeLockAcquired: true, + whileFirstFileLocked: async () => { + const current = this.getCurrentTask() + if (current?.taskId === childTaskId) { + childToRestore = completingChild + await this.removeClineFromStack({ saveMessages: false }) + } + parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { + startTask: false, + }) + try { + await parentInstance.overwriteClineMessages(parentClineMessages, { persist: false }) + } catch { + // non-fatal + } + try { + await parentInstance.overwriteApiConversationHistory(parentApiMessages, { + persist: false, + }) + } catch { + // non-fatal + } + }, + } + await this.taskHistoryStore.atomicUpdatePair( + parentTaskId, + childTaskId, + (parent) => { + assertCurrentDelegation(parent) + completingParent = { ...parent } + const reducerChild = { ...parent, id: childTaskId, status: "active" as const } + updatedHistory = completeDelegatedChild(parent, reducerChild, completionResultSummary).parent + return updatedHistory + }, + (child) => { + completingChild = { ...child } + if (pendingActionId && child.pendingAction?.actionId !== pendingActionId) { + throw new Error( + `[reopenParentFromDelegation] Pending action mismatch for child ${childTaskId}`, + ) + } + const completedChild = completeDelegatedChild( + completingParent, + child, + completionResultSummary, + ).child + return { + ...completedChild, + pendingAction: + child.pendingAction?.actionId === pendingActionId ? undefined : child.pendingAction, + } + }, + completionOptions, + ) + + parentToResume = parentInstance + } catch (error) { + await restoreConversationFiles(error) + if (error === staleDelegationError) { + this.log( + `[reopenParentFromDelegation] Aborting: parent ${parentTaskId} is no longer delegated to child ${childTaskId}`, + ) + return false + } + throw error + } + this.recentTasksCache = undefined // Notify the webview of both updated items so its in-memory history stays current. @@ -4215,10 +4269,16 @@ export class ClineProvider const updatedChild = this.taskHistoryStore.get(childTaskId) const updatedParent = this.taskHistoryStore.get(parentTaskId) if (updatedChild) { - await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedChild }) + await this.postMessageToWebview({ + type: "taskHistoryItemUpdated", + taskHistoryItem: updatedChild, + }) } if (updatedParent) { - await this.postMessageToWebview({ type: "taskHistoryItemUpdated", taskHistoryItem: updatedParent }) + await this.postMessageToWebview({ + type: "taskHistoryItemUpdated", + taskHistoryItem: updatedParent, + }) } } @@ -4229,27 +4289,6 @@ export class ClineProvider // non-fatal } - // 7) Reopen the parent from history as the sole active task (restores saved mode) - // IMPORTANT: startTask=false to suppress resume-from-history ask scheduling - const parentInstance = await this.createTaskWithHistoryItem(updatedHistory, { startTask: false }) - - // 8) Inject restored histories into the in-memory instance before resuming - if (parentInstance) { - try { - await parentInstance.overwriteClineMessages(parentClineMessages) - } catch { - // non-fatal - } - try { - await parentInstance.overwriteApiConversationHistory(parentApiMessages as any) - } catch { - // non-fatal - } - - // Auto-resume parent without ask("resume_task") - await parentInstance.resumeAfterDelegation() - } - // 9) Emit TaskDelegationResumed (provider-level) try { this.emit(RooCodeEventName.TaskDelegationResumed, parentTaskId, childTaskId) @@ -4259,7 +4298,23 @@ export class ClineProvider this.cancelledDelegationChildIds.delete(childTaskId) return true - }) + } + return this.runLockedDelegationTransition( + parentTaskId, + transition, + async () => parentToResume?.resumeAfterDelegation(), + async (error) => { + if (!childToRestore) return + try { + if (this.getCurrentTask()?.taskId === parentTaskId) + await this.removeClineFromStack({ saveMessages: false }) + if (!this.getCurrentTask()) + await this.createTaskWithHistoryItem(childToRestore, { startTask: false }) + } catch (restoreError) { + throw new AggregateError([error, restoreError], `Failed to restore child ${childTaskId}`) + } + }, + ) } /** diff --git a/src/core/webview/__tests__/ClineProvider.delegation-mutation.spec.ts b/src/core/webview/__tests__/ClineProvider.delegation-mutation.spec.ts new file mode 100644 index 0000000000..a1a541e58c --- /dev/null +++ b/src/core/webview/__tests__/ClineProvider.delegation-mutation.spec.ts @@ -0,0 +1,4 @@ +// Keep the focused delegation suites discoverable by changed-code mutation testing, +// which prefers test filenames matching the mutated production module. +import "../../../__tests__/history-resume-delegation.spec" +import "../../../__tests__/provider-delegation.spec" diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 0706dbe6fb..68bbf9fc04 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -26,7 +26,7 @@ }, "__tests__/history-resume-delegation.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 72 + "count": 70 } }, "__tests__/migrateSettings.spec.ts": { @@ -1026,7 +1026,7 @@ }, "core/webview/ClineProvider.ts": { "@typescript-eslint/no-explicit-any": { - "count": 12 + "count": 8 } }, "core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.ts": { @@ -1716,7 +1716,7 @@ }, "utils/safeWriteJson.ts": { "@typescript-eslint/no-explicit-any": { - "count": 4 + "count": 3 } }, "utils/tts.ts": { diff --git a/src/utils/__tests__/safeWriteJson.locking.spec.ts b/src/utils/__tests__/safeWriteJson.locking.spec.ts new file mode 100644 index 0000000000..08a82e09e5 --- /dev/null +++ b/src/utils/__tests__/safeWriteJson.locking.spec.ts @@ -0,0 +1,201 @@ +import * as fs from "fs/promises" +import * as os from "os" +import * as path from "path" + +const lockMock = vi.hoisted(() => vi.fn()) + +vi.mock("proper-lockfile", () => ({ lock: lockMock })) + +import { LOCK_STALE_MS, lockJsonFile, safeWriteJson } from "../safeWriteJson" + +describe("lockJsonFile", () => { + beforeEach(() => { + lockMock.mockReset() + }) + + it("acquires the lock with bounded retries and compromise handling", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const underlyingRelease = vi.fn(async () => {}) + lockMock.mockResolvedValueOnce(underlyingRelease) + + try { + const release = await lockJsonFile(filePath) + + expect(lockMock).toHaveBeenCalledWith(path.resolve(filePath), { + stale: LOCK_STALE_MS, + update: 10000, + realpath: false, + retries: { + retries: 5, + factor: 2, + minTimeout: 100, + maxTimeout: 1000, + }, + onCompromised: expect.any(Function), + }) + await release() + } finally { + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) + + it("defers a delayed compromise until release without throwing from the callback", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const compromised = new Error("lock ownership lost") + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + const underlyingRelease = vi.fn(async () => {}) + let onCompromised: ((error: Error) => void) | undefined + lockMock.mockImplementationOnce(async (_target: string, options: { onCompromised: (error: Error) => void }) => { + onCompromised = options.onCompromised + return underlyingRelease + }) + + try { + const release = await lockJsonFile(filePath) + + expect(() => onCompromised?.(compromised)).not.toThrow() + onCompromised?.(new Error("later compromise")) + await expect(release()).rejects.toBe(compromised) + expect(underlyingRelease).toHaveBeenCalledOnce() + expect(consoleError).toHaveBeenCalledWith(expect.stringContaining("was compromised"), compromised) + } finally { + consoleError.mockRestore() + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) + + it("surfaces a release error without logging an operation-failure arbitration message", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const releaseError = new Error("unlock failed") + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + lockMock.mockResolvedValueOnce(vi.fn().mockRejectedValueOnce(releaseError)) + + try { + await expect(safeWriteJson(filePath, { completed: true })).rejects.toBe(releaseError) + expect(consoleError).not.toHaveBeenCalled() + } finally { + consoleError.mockRestore() + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) + + it("logs an underlying release error but rejects with the earlier compromise", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const absoluteFilePath = path.resolve(filePath) + const compromised = new Error("lock ownership lost") + const releaseError = new Error("unlock failed") + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + lockMock.mockImplementationOnce(async (_target: string, options: { onCompromised: (error: Error) => void }) => { + return async () => { + options.onCompromised(compromised) + throw releaseError + } + }) + + try { + const release = await lockJsonFile(filePath) + + await expect(release()).rejects.toBe(compromised) + expect(consoleError).toHaveBeenNthCalledWith( + 2, + `Failed to release compromised lock for ${absoluteFilePath}:`, + releaseError, + ) + } finally { + consoleError.mockRestore() + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) + + it("logs the target path and acquisition error before propagating it", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const absoluteFilePath = path.resolve(filePath) + const acquisitionError = new Error("lock unavailable") + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + lockMock.mockRejectedValueOnce(acquisitionError) + + try { + await expect(safeWriteJson(filePath, { completed: true })).rejects.toBe(acquisitionError) + expect(consoleError).toHaveBeenCalledOnce() + expect(consoleError).toHaveBeenCalledWith( + `Failed to acquire lock for ${absoluteFilePath}:`, + acquisitionError, + ) + } finally { + consoleError.mockRestore() + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) + + it("rejects a successful write when the lock is compromised before release", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const compromised = new Error("lock ownership lost") + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + lockMock.mockImplementationOnce(async (_target: string, options: { onCompromised: (error: Error) => void }) => { + return async () => { + options.onCompromised(compromised) + } + }) + + try { + await expect(safeWriteJson(filePath, { completed: true })).rejects.toBe(compromised) + } finally { + consoleError.mockRestore() + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) + + it("preserves an operation error when release also fails", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const absoluteFilePath = path.resolve(filePath) + const operationError = new Error("merge failed") + const releaseError = new Error("unlock failed") + const consoleError = vi.spyOn(console, "error").mockImplementation(() => {}) + lockMock.mockResolvedValueOnce(vi.fn().mockRejectedValueOnce(releaseError)) + + try { + const write = safeWriteJson( + filePath, + { completed: true }, + { + merge: () => { + throw operationError + }, + }, + ) + + await expect(write).rejects.toBe(operationError) + expect(consoleError).toHaveBeenCalledWith( + `Operation failed for ${absoluteFilePath}: [Original Error Caught]`, + operationError, + ) + expect(consoleError).toHaveBeenCalledWith(`Failed to release lock for ${absoluteFilePath}:`, releaseError) + } finally { + consoleError.mockRestore() + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) + + it("resolves after a normal release", async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "safe-write-lock-")) + const filePath = path.join(tempDir, "history_item.json") + const underlyingRelease = vi.fn(async () => {}) + lockMock.mockResolvedValueOnce(underlyingRelease) + + try { + const release = await lockJsonFile(filePath) + + await expect(release()).resolves.toBeUndefined() + expect(underlyingRelease).toHaveBeenCalledOnce() + } finally { + await fs.rm(tempDir, { recursive: true, force: true }) + } + }) +}) diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 957a0bb20f..ee98a33b70 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -25,6 +25,47 @@ export interface SafeWriteJsonOptions { * cannot be parsed. */ merge?: (existing: unknown, incoming: unknown) => unknown + + /** The caller already holds this file's lock. Internal use only. */ + lockAcquired?: boolean +} + +export async function lockJsonFile(filePath: string): Promise<() => Promise> { + const absoluteFilePath = path.resolve(filePath) + const dirPath = path.dirname(absoluteFilePath) + let compromisedError: Error | undefined + + await fs.mkdir(dirPath, { recursive: true }) + await fs.access(dirPath) + + const release = await lockfile.lock(absoluteFilePath, { + stale: LOCK_STALE_MS, + update: 10000, + realpath: false, + retries: { + retries: 5, + factor: 2, + minTimeout: 100, + maxTimeout: 1000, + }, + onCompromised: (err) => { + if (!compromisedError) { + compromisedError = err + console.error(`Lock at ${absoluteFilePath} was compromised:`, err) + } + }, + }) + + return async () => { + try { + await release() + } catch (releaseError) { + if (!compromisedError) throw releaseError + console.error(`Failed to release compromised lock for ${absoluteFilePath}:`, releaseError) + } + + if (compromisedError) throw compromisedError + } } /** @@ -45,47 +86,18 @@ export interface SafeWriteJsonOptions { async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJsonOptions): Promise { const absoluteFilePath = path.resolve(filePath) let releaseLock = async () => {} // Initialized to a no-op + let operationFailed = false + let operationError: unknown + let unlockFailed = false + let unlockError: unknown - // For directory creation - const dirPath = path.dirname(absoluteFilePath) - - // Ensure directory structure exists with improved reliability - try { - // Create directory with recursive option - await fs.mkdir(dirPath, { recursive: true }) - - // Verify directory exists after creation attempt - await fs.access(dirPath) - } catch (dirError: any) { - console.error(`Failed to create or access directory for ${absoluteFilePath}:`, dirError) - throw dirError - } - - // Acquire the lock before any file operations - try { - releaseLock = await lockfile.lock(absoluteFilePath, { - stale: LOCK_STALE_MS, - update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long - realpath: false, // the file may not exist yet, which is acceptable - retries: { - // Configuration for retrying lock acquisition - retries: 5, // Number of retries after the initial attempt - factor: 2, // Exponential backoff factor (e.g., 100ms, 200ms, 400ms, ...) - minTimeout: 100, // Minimum time to wait before the first retry (in ms) - maxTimeout: 1000, // Maximum time to wait for any single retry (in ms) - }, - onCompromised: (err) => { - console.error(`Lock at ${absoluteFilePath} was compromised:`, err) - throw err - }, - }) - } catch (lockError) { - // If lock acquisition fails, we throw immediately. - // The releaseLock remains a no-op, so the finally block in the main file operations - // try-catch-finally won't try to release an unacquired lock if this path is taken. - console.error(`Failed to acquire lock for ${absoluteFilePath}:`, lockError) - // Propagate the lock acquisition error - throw lockError + if (!options?.lockAcquired) { + try { + releaseLock = await lockJsonFile(absoluteFilePath) + } catch (lockError) { + console.error(`Failed to acquire lock for ${absoluteFilePath}:`, lockError) + throw lockError + } } // Variables to hold the actual paths of temp files if they are created. @@ -162,6 +174,8 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso } } } catch (originalError) { + operationFailed = true + operationError = originalError console.error(`Operation failed for ${absoluteFilePath}: [Original Error Caught]`, originalError) const newFileToCleanupWithinCatch = actualTempNewFilePath @@ -205,18 +219,21 @@ async function safeWriteJson(filePath: string, data: any, options?: SafeWriteJso ) } } - throw originalError // This MUST be the error that rejects the promise. } finally { // Release the lock in the main finally block. try { // releaseLock will be the actual unlock function if lock was acquired, // or the initial no-op if acquisition failed. await releaseLock() - } catch (unlockError) { - // Do not re-throw here, as the originalError from the try/catch (if any) is more important. - console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + } catch (error) { + unlockFailed = true + unlockError = error + if (operationFailed) console.error(`Failed to release lock for ${absoluteFilePath}:`, error) } } + + if (operationFailed) throw operationError + if (unlockFailed) throw unlockError } /**