diff --git a/docs/architecture/task-cleanup-protocol-model.md b/docs/architecture/task-cleanup-protocol-model.md new file mode 100644 index 0000000000..e46991365b --- /dev/null +++ b/docs/architecture/task-cleanup-protocol-model.md @@ -0,0 +1,33 @@ +# Task cleanup protocol model check + +Zoo Code checks the in-memory task cleanup protocol with a bounded explicit-state explorer. It runs under the umbrella command: + +```sh +pnpm lifecycle:model-check +``` + +For focused debugging, run: + +```sh +pnpm cleanup-protocol:model-check +``` + +This is a separate child model from the persisted task lifecycle and shared-store concurrency models. It follows the native tool-call parser model pattern: keep an independent bounded state space for an independent protocol, require every action and semantic landmark to remain reachable, and connect the abstract claims to focused production tests. + +## Bounds and environment actions + +The model uses two tasks and explores every reachable interleaving through depth 20, with an explicit 100,000-state budget. Abort, disposal, final-save, provider abort/drain phases, and shutdown-cursor state are modeled directly. Independent abort and disposal calls may interleave freely, while provider-initiated calls are gated to the current shutdown task. Cleanup and editor-reversion settlement or rejection are environment actions, so the explorer does not assume they eventually occur. + +The model checks these finite safety properties: + +1. repeated abort and disposal calls reuse their first logical handle and start each operation at most once; +2. final message persistence, or the history-task save skip, cannot occur before editor reversion settles or rejects; +3. abort may complete while ancillary output cleanup remains pending; +4. disposal completes only after cleanup and reversion both settle or reject; +5. provider shutdown advances through tasks only after each task's abort and disposal reach terminal states; +6. shutdown advances through tasks in registry order and is complete exactly when every modeled task is drained; and +7. abort, ancillary cleanup, or disposal-start rejection is isolated so shutdown can continue to later tasks. + +Named landmarks require abort completion during pending ancillary cleanup, contained cleanup rejection, final-save attempt after rejected reversion, a history-task final-save skip, two-task shutdown completion, and shutdown continuation after rejected abort, ancillary cleanup, or disposal start. + +These are bounded safety and reachability claims only. The model does not claim filesystem or editor Promise liveness, fairness, timing bounds, arbitrary task counts, or that cleanup can never remain pending. Deterministic Vitest coverage in `Task.dispose.test.ts`, `Task.spec.ts`, and `ClineProvider.spec.ts` exercises the corresponding production Promise identities, ordering, rejection handling, and multi-task shutdown behavior. diff --git a/docs/architecture/task-lifecycle-model.md b/docs/architecture/task-lifecycle-model.md index 588ffd5204..3218eb4ba8 100644 --- a/docs/architecture/task-lifecycle-model.md +++ b/docs/architecture/task-lifecycle-model.md @@ -63,6 +63,10 @@ The known-unsafe witnesses currently compare exact shortest action sequences. Th `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. +## Task cleanup protocol model + +The umbrella command also runs a separate bounded child model for in-memory abort, disposal, and provider-shutdown ordering. It models cleanup settlement and rejection as environment transitions and makes no filesystem, editor Promise, fairness, or timing-liveness claim. See [Task cleanup protocol model check](./task-cleanup-protocol-model.md). + ## Invariants The checker currently enforces: diff --git a/package.json b/package.json index 8431467918..1a44a12680 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,8 @@ "check-types": "turbo check-types --log-order grouped --output-logs new-only", "test": "turbo test --log-order grouped --output-logs new-only", "test:mutation-ci": "node --test scripts/stryker-diff.test.mjs", - "lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts", + "lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && pnpm cleanup-protocol:model-check", + "cleanup-protocol:model-check": "tsx scripts/check-task-cleanup-protocol.ts", "test:coverage": "turbo test:coverage --log-order grouped --output-logs new-only", "format": "turbo format --log-order grouped --output-logs new-only", "build": "turbo build --log-order grouped --output-logs new-only", diff --git a/packages/types/src/task.ts b/packages/types/src/task.ts index 572302861b..cac34cc6f2 100644 --- a/packages/types/src/task.ts +++ b/packages/types/src/task.ts @@ -128,7 +128,7 @@ export interface TaskLike { approveAsk(options?: { text?: string; images?: string[] }): void denyAsk(options?: { text?: string; images?: string[] }): void submitUserMessage(text: string, images?: string[], mode?: string, providerProfile?: string): Promise - abortTask(): void + abortTask(): Promise } export type TaskEvents = { diff --git a/scripts/check-task-cleanup-protocol.ts b/scripts/check-task-cleanup-protocol.ts new file mode 100644 index 0000000000..466fad6464 --- /dev/null +++ b/scripts/check-task-cleanup-protocol.ts @@ -0,0 +1,336 @@ +import assert from "node:assert/strict" + +const taskIds = ["A", "B"] as const +type TaskId = (typeof taskIds)[number] +type PendingResult = "idle" | "pending" | "resolved" | "rejected" + +interface TaskState { + abort: PendingResult + abortHandle?: 1 + abortStarts: number + disposal: PendingResult + disposalHandle?: 1 + disposalStarts: number + reversion: PendingResult + cleanup: PendingResult + finalization: "idle" | "attempted" | "skipped" +} + +interface ModelState { + tasks: Record + shutdown: "idle" | "call-abort" | "wait-abort" | "wait-disposal" | "done" + shutdownIndex: number + drained: Record +} + +interface Step { + action: string + state: ModelState +} + +const MAX_DEPTH = 20 +const MAX_STATES = 100_000 +const expectedActions = [ + "abort", + "dispose", + "abort-starts-disposal", + "reject-disposal-start", + "settle-reversion", + "reject-reversion", + "settle-cleanup", + "reject-cleanup", + "reject-abort", + "complete-abort", + "skip-final-save", + "complete-disposal", + "start-shutdown", + "shutdown-abort", + "shutdown-dispose", + "advance-shutdown", +] as const + +function task(): TaskState { + return { + abort: "idle", + abortStarts: 0, + disposal: "idle", + disposalStarts: 0, + reversion: "idle", + cleanup: "idle", + finalization: "idle", + } +} + +function initialState(): ModelState { + return { + tasks: { A: task(), B: task() }, + shutdown: "idle", + shutdownIndex: 0, + drained: { A: false, B: false }, + } +} + +function clone(state: ModelState): ModelState { + return structuredClone(state) +} + +function isTerminal(result: PendingResult): boolean { + return result === "resolved" || result === "rejected" +} + +function callAbort(state: ModelState, taskId: TaskId): ModelState { + const next = clone(state) + const current = next.tasks[taskId] + if (current.abort === "idle") { + current.abort = "pending" + current.abortHandle = 1 + current.abortStarts += 1 + } + return next +} + +function callDispose(state: ModelState, taskId: TaskId): ModelState { + const next = clone(state) + const current = next.tasks[taskId] + if (current.disposal === "idle") { + current.disposal = "pending" + current.disposalHandle = 1 + current.disposalStarts += 1 + current.reversion = "pending" + current.cleanup = "pending" + } + return next +} + +function transitions(state: ModelState): Step[] { + const result: Step[] = [] + for (const taskId of taskIds) { + const current = state.tasks[taskId] + if (current.abort === "idle") { + result.push({ action: `abort(${taskId})`, state: callAbort(state, taskId) }) + } + if (current.disposal === "idle") { + result.push({ action: `dispose(${taskId})`, state: callDispose(state, taskId) }) + const next = clone(state) + const failed = next.tasks[taskId] + failed.disposal = "rejected" + failed.disposalHandle = 1 + failed.disposalStarts = 1 + failed.reversion = "rejected" + failed.cleanup = "rejected" + result.push({ action: `reject-disposal-start(${taskId})`, state: next }) + } + if (current.abort === "pending" && current.disposal === "idle") { + result.push({ action: `abort-starts-disposal(${taskId})`, state: callDispose(state, taskId) }) + } + if (current.reversion === "pending") { + for (const outcome of ["resolved", "rejected"] as const) { + const next = clone(state) + next.tasks[taskId].reversion = outcome + result.push({ + action: `${outcome === "resolved" ? "settle" : "reject"}-reversion(${taskId})`, + state: next, + }) + } + } + if (current.cleanup === "pending") { + for (const outcome of ["resolved", "rejected"] as const) { + const next = clone(state) + next.tasks[taskId].cleanup = outcome + result.push({ + action: `${outcome === "resolved" ? "settle" : "reject"}-cleanup(${taskId})`, + state: next, + }) + } + } + if (current.abort === "pending") { + const next = clone(state) + next.tasks[taskId].abort = "rejected" + result.push({ action: `reject-abort(${taskId})`, state: next }) + } + if ( + current.abort === "pending" && + current.disposal !== "idle" && + isTerminal(current.reversion) && + current.finalization === "idle" + ) { + for (const finalization of ["attempted", "skipped"] as const) { + const next = clone(state) + next.tasks[taskId].finalization = finalization + next.tasks[taskId].abort = "resolved" + result.push({ + action: `${finalization === "attempted" ? "complete-abort" : "skip-final-save"}(${taskId})`, + state: next, + }) + } + } + if (current.disposal === "pending" && isTerminal(current.reversion) && isTerminal(current.cleanup)) { + const next = clone(state) + next.tasks[taskId].disposal = "resolved" + result.push({ action: `complete-disposal(${taskId})`, state: next }) + } + } + + if (state.shutdown === "idle") { + const next = clone(state) + next.shutdown = "call-abort" + result.push({ action: "start-shutdown()", state: next }) + } else if (state.shutdown !== "done") { + const taskId = taskIds[state.shutdownIndex] + if (taskId) { + const current = state.tasks[taskId] + if (state.shutdown === "call-abort") { + const next = callAbort(state, taskId) + next.shutdown = "wait-abort" + result.push({ action: `shutdown-abort(${taskId})`, state: next }) + } else if (state.shutdown === "wait-abort" && isTerminal(current.abort)) { + const next = callDispose(state, taskId) + next.shutdown = "wait-disposal" + result.push({ action: `shutdown-dispose(${taskId})`, state: next }) + } else if (state.shutdown === "wait-disposal" && isTerminal(current.disposal)) { + const next = clone(state) + next.drained[taskId] = true + next.shutdownIndex += 1 + next.shutdown = next.shutdownIndex === taskIds.length ? "done" : "call-abort" + result.push({ action: `advance-shutdown(${taskId})`, state: next }) + } + } + } + return result +} + +function invariantViolations(state: ModelState): string[] { + const violations: string[] = [] + for (const taskId of taskIds) { + const current = state.tasks[taskId] + if (current.abortStarts > 1 || current.disposalStarts > 1) { + violations.push(`${taskId}: abort and disposal may each start at most once`) + } + if ((current.abort === "idle") === Boolean(current.abortHandle)) { + violations.push(`${taskId}: abort handle must exist exactly when abort has started`) + } + if ((current.disposal === "idle") === Boolean(current.disposalHandle)) { + violations.push(`${taskId}: disposal handle must exist exactly when disposal has started`) + } + if (current.finalization !== "idle" && !isTerminal(current.reversion)) { + violations.push(`${taskId}: abort finalization occurred before editor reversion settled`) + } + if (current.abort === "resolved" && current.finalization === "idle") { + violations.push(`${taskId}: abort resolved before its final save attempt or history-task skip`) + } + if (current.finalization !== "idle" && current.abort !== "resolved") { + violations.push(`${taskId}: abort finalization exists without a resolved abort`) + } + if (current.disposal === "resolved" && (!isTerminal(current.cleanup) || !isTerminal(current.reversion))) { + violations.push(`${taskId}: disposal resolved before all cleanup branches settled`) + } + if (state.drained[taskId] && (!isTerminal(current.abort) || !isTerminal(current.disposal))) { + violations.push(`${taskId}: provider advanced before abort and disposal completed`) + } + } + if (state.shutdownIndex !== taskIds.filter((taskId) => state.drained[taskId]).length) { + violations.push("shutdown cursor must match the drained task prefix") + } + if (state.drained.B && !state.drained.A) violations.push("provider drained tasks out of order") + if (state.shutdownIndex === 1 && !state.drained.A) { + violations.push("provider advanced to the second task before draining the first") + } + if ((state.shutdown === "done") !== taskIds.every((taskId) => state.drained[taskId])) { + violations.push("shutdown is done exactly when every modeled task is drained") + } + return violations +} + +function canonical(state: ModelState): string { + return JSON.stringify(state) +} + +function runRepresentativeMemoizationChecks(): void { + const start = initialState() + const firstAbort = callAbort(start, "A") + assert.deepEqual(callAbort(firstAbort, "A"), firstAbort, "repeated abort must reuse the first handle") + const firstDisposal = callDispose(start, "A") + assert.deepEqual(callDispose(firstDisposal, "A"), firstDisposal, "repeated disposal must reuse the first handle") +} + +function runModelCheck(): { states: number; actions: number; landmarks: number } { + const start = initialState() + const queue: Array<{ state: ModelState; trace: Step[] }> = [{ state: start, trace: [] }] + const visited = new Set([canonical(start)]) + const reachedActions = new Set() + const reachedLandmarks = new Set() + const frontier: ModelState[] = [] + + for (let index = 0; index < queue.length; index++) { + const node = queue[index]! + const violations = invariantViolations(node.state) + if (violations.length) { + throw new Error( + `Task cleanup protocol invariant failed: ${violations.join("; ")}\n${node.trace.map((step, i) => `${i + 1}. ${step.action}`).join("\n")}`, + ) + } + if (node.state.tasks.A.abort === "resolved" && node.state.tasks.A.cleanup === "pending") { + reachedLandmarks.add("abort-completes-before-ancillary-cleanup") + } + if (node.state.tasks.A.disposal === "resolved" && node.state.tasks.A.cleanup === "rejected") { + reachedLandmarks.add("cleanup-rejection-is-observed-and-contained") + } + if ( + node.state.tasks.A.abort === "resolved" && + node.state.tasks.A.reversion === "rejected" && + node.state.tasks.A.finalization === "attempted" + ) { + reachedLandmarks.add("reversion-rejection-does-not-block-final-save") + } + if (node.state.drained.A && node.state.tasks.A.cleanup === "rejected" && node.state.drained.B) { + reachedLandmarks.add("shutdown-continues-after-cleanup-rejection") + } + if (node.state.drained.A && node.state.tasks.A.disposal === "rejected" && node.state.drained.B) { + reachedLandmarks.add("shutdown-continues-after-disposal-rejection") + } + if (node.state.drained.A && node.state.tasks.A.abort === "rejected" && node.state.drained.B) { + reachedLandmarks.add("shutdown-continues-after-abort-rejection") + } + if (node.state.tasks.A.abort === "resolved" && node.state.tasks.A.finalization === "skipped") { + reachedLandmarks.add("history-task-final-save-skip") + } + if (node.state.shutdown === "done") reachedLandmarks.add("multi-task-shutdown-drained") + + if (node.trace.length === MAX_DEPTH) { + frontier.push(node.state) + continue + } + for (const step of transitions(node.state)) { + reachedActions.add(step.action.slice(0, step.action.indexOf("("))) + 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 > MAX_STATES) throw new Error(`Cleanup protocol exceeded ${MAX_STATES} states`) + } + } + + const unseen = frontier.flatMap(transitions).find((step) => !visited.has(canonical(step.state))) + if (unseen) throw new Error(`Cleanup protocol truncated before unseen action ${unseen.action}`) + const missingActions = expectedActions.filter((action) => !reachedActions.has(action)) + assert.deepEqual(missingActions, [], `Cleanup protocol has unreachable actions: ${missingActions.join(", ")}`) + const expectedLandmarks = [ + "abort-completes-before-ancillary-cleanup", + "cleanup-rejection-is-observed-and-contained", + "reversion-rejection-does-not-block-final-save", + "shutdown-continues-after-cleanup-rejection", + "shutdown-continues-after-disposal-rejection", + "shutdown-continues-after-abort-rejection", + "history-task-final-save-skip", + "multi-task-shutdown-drained", + ] + const missingLandmarks = expectedLandmarks.filter((landmark) => !reachedLandmarks.has(landmark)) + assert.deepEqual(missingLandmarks, [], `Cleanup protocol has unreachable landmarks: ${missingLandmarks.join(", ")}`) + return { states: visited.size, actions: reachedActions.size, landmarks: reachedLandmarks.size } +} + +runRepresentativeMemoizationChecks() +const result = runModelCheck() +console.log( + `Task cleanup protocol model check passed: ${result.states} reachable states, ${result.actions}/${expectedActions.length} actions reachable, ${result.landmarks}/8 landmarks reached, depth <= ${MAX_DEPTH}, tasks=${taskIds.length}`, +) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 37281a9010..4f122feefc 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -370,6 +370,9 @@ export class Task extends EventEmitter implements TaskLike { private lastTelemetryFlushAt: number = Date.now() private telemetryToolUsageBaseline: ToolUsage = {} private telemetryMessageCountsBaseline: { user: number; assistant: number } = { user: 0, assistant: 0 } + private abortPromise?: Promise + private disposalPromise?: Promise + private diffReversionPromise: Promise = Promise.resolve() // Checkpoints enableCheckpoints: boolean @@ -2464,15 +2467,18 @@ export class Task extends EventEmitter implements TaskLike { this.debouncedEmitTokenUsage.flush() } - public async abortTask(isAbandoned = false) { - // Aborting task - - // Will stop any autonomously running promises. + public abortTask(isAbandoned = false): Promise { if (isAbandoned) { this.abandoned = true } this.abort = true + this.abortPromise ??= this.abortTaskOnce() + return this.abortPromise + } + + private async abortTaskOnce(): Promise { + // Aborting task // Reset consecutive error counters on abort (manual intervention) this.consecutiveNoToolUseCount = 0 @@ -2493,7 +2499,12 @@ export class Task extends EventEmitter implements TaskLike { this.emit(RooCodeEventName.TaskAborted) try { - this.dispose() // Call the centralized dispose method + void this.dispose().catch((error) => { + console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) + }) + // Reversion affects the user's workspace and must finish before the + // final task state is saved. Artifact deletion is drained separately. + await this.diffReversionPromise } catch (error) { console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) // Don't rethrow - we want abort to always succeed @@ -2514,7 +2525,16 @@ export class Task extends EventEmitter implements TaskLike { } } - public dispose(): void { + public dispose(): Promise { + if (this.disposalPromise) { + return this.disposalPromise + } + + this.disposalPromise = this.disposeOnce() + return this.disposalPromise + } + + private async disposeOnce(): Promise { console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`) // Stop the idle telemetry check and report any unflushed activity as a @@ -2563,7 +2583,7 @@ export class Task extends EventEmitter implements TaskLike { } // Cleanup command output artifacts - getTaskDirectoryPath(this.globalStoragePath, this.taskId) + const pendingCleanup = getTaskDirectoryPath(this.globalStoragePath, this.taskId) .then((taskDir) => { const outputDir = path.join(taskDir, "command-output") return OutputInterceptor.cleanup(outputDir) @@ -2591,11 +2611,14 @@ export class Task extends EventEmitter implements TaskLike { try { // If we're not streaming then `abortStream` won't be called. if (this.isStreaming && this.diffViewProvider.isEditing) { - this.diffViewProvider.revertChanges().catch(console.error) + this.diffReversionPromise = this.diffViewProvider.revertChanges().catch(console.error) } } catch (error) { console.error("Error reverting diff changes:", error) } + + await pendingCleanup + await this.diffReversionPromise } // Subtasks diff --git a/src/core/task/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 9f00e9d852..472218fce5 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -1,8 +1,12 @@ +import path from "node:path" + import { type ProviderSettings, RooCodeEventName } from "@roo-code/types" import { Task } from "../Task" import { ClineProvider } from "../../webview/ClineProvider" +import { OutputInterceptor } from "../../../integrations/terminal/OutputInterceptor" import { providerIdentifiers } from "@roo-code/types/provider-identifiers" +import { getTaskDirectoryPath } from "../../../utils/storage" // Mock dependencies vi.mock("../../webview/ClineProvider") @@ -11,10 +15,7 @@ vi.mock("../../../integrations/terminal/TerminalRegistry", () => ({ releaseTerminalsForTask: vi.fn(), }, })) -// dispose() fires an UNawaited getTaskDirectoryPath -> OutputInterceptor.cleanup chain. -// Mock both so it resolves immediately with no real fs and no late console.error, -// otherwise that dangling promise logs after the test ends and trips Vitest's -// "Closing rpc while onUserConsoleLog was pending" teardown race. +// Keep disposal tests independent of the real filesystem and output interceptor. vi.mock("../../../utils/storage", () => ({ getTaskDirectoryPath: vi.fn().mockResolvedValue("/test/path/tasks/test-task"), })) @@ -49,13 +50,16 @@ describe("Task dispose method", () => { context: { globalStorageUri: { fsPath: string } } getState: ReturnType log: ReturnType + flushPostStateToWebviewThrottled: ReturnType } let mockApiConfiguration: ProviderSettings let task: Task + let skipCleanup: boolean beforeEach(() => { // Reset all mocks vi.clearAllMocks() + skipCleanup = false // Mock provider mockProvider = { @@ -64,6 +68,7 @@ describe("Task dispose method", () => { }, getState: vi.fn().mockResolvedValue({ mode: "code" }), log: vi.fn(), + flushPostStateToWebviewThrottled: vi.fn().mockResolvedValue(undefined), } // Mock API configuration @@ -80,13 +85,188 @@ describe("Task dispose method", () => { }) }) - afterEach(() => { - // Clean up - if (task && !task.abort) { - task.dispose() + afterEach(async () => { + if (task && !skipCleanup) { + await task.dispose().catch(() => {}) } }) + test("should expose completion of deferred command output cleanup", async () => { + let resolveTaskDirectory: (taskDirectory: string) => void + vi.mocked(getTaskDirectoryPath).mockReturnValueOnce( + new Promise((resolve) => { + resolveTaskDirectory = resolve + }), + ) + + const disposal = task.dispose() + let disposalComplete = false + void disposal.then(() => { + disposalComplete = true + }) + await Promise.resolve() + + expect(disposalComplete).toBe(false) + expect(OutputInterceptor.cleanup).not.toHaveBeenCalled() + + resolveTaskDirectory!("/test/path/tasks/test-task") + await disposal + + expect(OutputInterceptor.cleanup).toHaveBeenCalledWith( + path.join("/test/path/tasks/test-task", "command-output"), + ) + expect(disposalComplete).toBe(true) + }) + + test("should reject the memoized completion promise when disposal cannot start", async () => { + const disposalError = new Error("disposal failed") + skipCleanup = true + vi.spyOn(console, "log").mockImplementationOnce(() => { + throw disposalError + }) + + const disposal = task.dispose() + let rejection: unknown + void disposal.catch((error) => { + rejection = error + }) + await Promise.resolve() + + expect(rejection).toBe(disposalError) + expect(task.dispose()).toBe(disposal) + }) + + test("should report command output cleanup failures before disposal completes", async () => { + const cleanupError = new Error("cleanup failed") + let disposalComplete = false + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => { + expect(disposalComplete).toBe(false) + }) + let rejectTaskDirectory!: (error: Error) => void + vi.mocked(getTaskDirectoryPath).mockReturnValueOnce( + new Promise((_, reject) => { + rejectTaskDirectory = reject + }), + ) + + const disposal = task.dispose() + void disposal.then(() => { + disposalComplete = true + }) + rejectTaskDirectory(cleanupError) + await vi.waitFor(() => expect(consoleErrorSpy).toHaveBeenCalled()) + + expect(consoleErrorSpy).toHaveBeenCalledWith("Error cleaning up command output artifacts:", cleanupError) + await disposal + expect(disposalComplete).toBe(true) + consoleErrorSpy.mockRestore() + }) + + test("should wait for deferred output cleanup and memoize repeated disposal", async () => { + let resolveCleanup!: () => void + vi.mocked(OutputInterceptor.cleanup).mockReturnValueOnce( + new Promise((resolve) => { + resolveCleanup = resolve + }), + ) + const removeAllListenersSpy = vi.spyOn(task, "removeAllListeners") + + const firstDisposal = task.dispose() + const secondDisposal = task.dispose() + let disposalComplete = false + void firstDisposal.then(() => { + disposalComplete = true + }) + await vi.waitFor(() => expect(OutputInterceptor.cleanup).toHaveBeenCalledOnce()) + + expect(secondDisposal).toBe(firstDisposal) + expect(disposalComplete).toBe(false) + expect(removeAllListenersSpy).toHaveBeenCalledOnce() + + resolveCleanup() + await firstDisposal + expect(disposalComplete).toBe(true) + }) + + test("should await diff reversion during abort without waiting for output cleanup", async () => { + let resolveCleanup!: () => void + let resolveReversion!: () => void + vi.mocked(OutputInterceptor.cleanup).mockReturnValueOnce( + new Promise((resolve) => { + resolveCleanup = resolve + }), + ) + task.isStreaming = true + task.diffViewProvider.isEditing = true + const revertChangesSpy = vi.spyOn(task.diffViewProvider, "revertChanges").mockReturnValue( + new Promise((resolve) => { + resolveReversion = resolve + }), + ) + const saveMessages = vi.fn().mockResolvedValue(true) + Object.defineProperty(task, "saveClineMessages", { value: saveMessages }) + + const abort = task.abortTask() + await vi.waitFor(() => expect(revertChangesSpy).toHaveBeenCalledOnce()) + expect(saveMessages).not.toHaveBeenCalled() + + resolveReversion() + await abort + expect(saveMessages).toHaveBeenCalledOnce() + + let disposalComplete = false + void task.dispose().then(() => { + disposalComplete = true + }) + await Promise.resolve() + expect(disposalComplete).toBe(false) + + resolveCleanup() + await task.dispose() + expect(disposalComplete).toBe(true) + }) + + test("should expose completion of deferred diff reversion", async () => { + let resolveReversion: () => void + const reversion = new Promise((resolve) => { + resolveReversion = resolve + }) + task.isStreaming = true + task.diffViewProvider.isEditing = true + const revertChangesSpy = vi.spyOn(task.diffViewProvider, "revertChanges").mockReturnValue(reversion) + + const disposal = task.dispose() + await vi.waitFor(() => expect(OutputInterceptor.cleanup).toHaveBeenCalled()) + let disposalComplete = false + void disposal.then(() => { + disposalComplete = true + }) + await Promise.resolve() + expect(disposalComplete).toBe(false) + expect(revertChangesSpy).toHaveBeenCalledOnce() + + resolveReversion!() + await disposal + expect(disposalComplete).toBe(true) + }) + + test("should log rejected diff reversion and continue final abort persistence", async () => { + const reversionError = new Error("reversion failed") + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + task.isStreaming = true + task.diffViewProvider.isEditing = true + vi.spyOn(task.diffViewProvider, "revertChanges").mockRejectedValue(reversionError) + const saveMessages = vi.fn().mockResolvedValue(true) + Object.defineProperty(task, "saveClineMessages", { value: saveMessages }) + + await expect(task.abortTask()).resolves.toBeUndefined() + await expect(task.dispose()).resolves.toBeUndefined() + + expect(consoleErrorSpy).toHaveBeenCalledWith(reversionError) + expect(saveMessages).toHaveBeenCalledOnce() + consoleErrorSpy.mockRestore() + }) + test("should remove all event listeners when dispose is called", () => { // Add some event listeners using type assertion to bypass strict typing for testing const listener1 = vi.fn(() => {}) @@ -106,7 +286,7 @@ describe("Task dispose method", () => { const removeAllListenersSpy = vi.spyOn(task, "removeAllListeners") // Call dispose - task.dispose() + void task.dispose() // Verify removeAllListeners was called expect(removeAllListenersSpy).toHaveBeenCalledOnce() @@ -128,7 +308,7 @@ describe("Task dispose method", () => { const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) // Call dispose - should not throw - expect(() => task.dispose()).not.toThrow() + expect(() => void task.dispose()).not.toThrow() // Verify error was logged expect(consoleErrorSpy).toHaveBeenCalledWith("Error removing event listeners:", expect.any(Error)) @@ -143,7 +323,7 @@ describe("Task dispose method", () => { const consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}) // Call dispose - task.dispose() + void task.dispose() // Verify dispose was called and logged expect(consoleLogSpy).toHaveBeenCalledWith( @@ -193,7 +373,7 @@ describe("Task dispose method", () => { expect(task.listenerCount(RooCodeEventName.TaskUnpaused)).toBe(1) // Call dispose - task.dispose() + void task.dispose() // Verify all listeners are removed expect(task.listenerCount(RooCodeEventName.TaskStarted)).toBe(0) @@ -244,7 +424,7 @@ describe("Task.run() idempotency", () => { const callsBefore = startTaskSpy.mock.calls.length // constructor fired it once void t.run() expect(startTaskSpy.mock.calls.length).toBe(callsBefore) // run() must not add a second call - t.dispose() + await t.dispose() startTaskSpy.mockRestore() }) @@ -262,7 +442,7 @@ describe("Task.run() idempotency", () => { void t.run() expect(startTaskSpy.mock.calls.length).toBe(callsAfterStart) // no additional call - t.dispose() + await t.dispose() startTaskSpy.mockRestore() }) @@ -280,7 +460,7 @@ describe("Task.run() idempotency", () => { const p2 = t.run() expect(p1).toBe(p2) await p1 - t.dispose() + await t.dispose() startTaskSpy.mockRestore() }) }) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 37e228f887..0376f437cb 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -2111,13 +2111,14 @@ describe("Cline", () => { const emitSpy = vi.spyOn(task, "emit") // Mock the dispose method to avoid actual cleanup - vi.spyOn(task, "dispose").mockImplementation(() => {}) + vi.spyOn(task, "dispose").mockResolvedValue(undefined) // Call abortTask await task.abortTask() // Verify abort flag is set expect(task.abort).toBe(true) + expect(task.abandoned).toBe(false) // Verify TaskAborted event was emitted expect(emitSpy).toHaveBeenCalledWith("taskAborted") @@ -2132,7 +2133,7 @@ describe("Cline", () => { }) // Mock the dispose method to track cleanup - const disposeSpy = vi.spyOn(task, "dispose").mockImplementation(() => {}) + const disposeSpy = vi.spyOn(task, "dispose").mockResolvedValue(undefined) // Call abortTask await task.abortTask() @@ -2142,6 +2143,49 @@ describe("Cline", () => { expect(disposeSpy).toHaveBeenCalled() }) + it("does not wait for ancillary disposal cleanup before abort resolves", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + let resolveDisposal: () => void + const disposal = new Promise((resolve) => { + resolveDisposal = resolve + }) + const disposeSpy = vi.spyOn(task, "dispose").mockReturnValue(disposal) + + const abort = task.abortTask() + await vi.waitFor(() => expect(disposeSpy).toHaveBeenCalledOnce()) + await abort + + expect(disposeSpy).toHaveBeenCalledOnce() + resolveDisposal!() + await disposal + }) + + it("memoizes concurrent aborts while preserving abandoned state", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const emitSpy = vi.spyOn(task, "emit") + vi.spyOn(task, "dispose").mockResolvedValue(undefined) + + const firstAbort = task.abortTask() + const secondAbort = task.abortTask(true) + + expect(secondAbort).toBe(firstAbort) + expect(task.abandoned).toBe(true) + await firstAbort + expect( + (emitSpy.mock.calls as unknown[][]).filter(([event]) => event === RooCodeEventName.TaskAborted), + ).toHaveLength(1) + }) + it("flushes pending state before TaskAborted and disposal while queue state is intact", async () => { const task = new Task({ provider: mockProvider, @@ -2154,7 +2198,7 @@ describe("Cline", () => { queuedMessagesAtFlush = task.messageQueueService.messages.length }) const emitSpy = vi.spyOn(task, "emit") - const disposeSpy = vi.spyOn(task, "dispose").mockImplementation(() => {}) + const disposeSpy = vi.spyOn(task, "dispose").mockResolvedValue(undefined) task.messageQueueService.addMessage("queued text") await task.abortTask() @@ -2183,7 +2227,7 @@ describe("Cline", () => { const error = new Error("state flush failed") const flushSpy = vi.mocked(mockProvider.flushPostStateToWebviewThrottled).mockRejectedValueOnce(error) const taskAbortedListener = vi.fn() - const disposeSpy = vi.spyOn(task, "dispose").mockImplementation(() => {}) + const disposeSpy = vi.spyOn(task, "dispose").mockResolvedValue(undefined) const saveSpy = vi.spyOn(getTaskTestAccess(task), "saveClineMessages").mockResolvedValue(true) const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) task.on(RooCodeEventName.TaskAborted, taskAbortedListener) @@ -2220,7 +2264,7 @@ describe("Cline", () => { expect(typeof taskLike.abortTask).toBe("function") // Mock the dispose method to avoid actual cleanup - vi.spyOn(task, "dispose").mockImplementation(() => {}) + vi.spyOn(task, "dispose").mockResolvedValue(undefined) // Call abortTask through interface await taskLike.abortTask() @@ -2258,6 +2302,28 @@ describe("Cline", () => { // Restore console.error consoleErrorSpy.mockRestore() }) + + it("should handle asynchronous disposal errors gracefully", async () => { + const task = new Task({ + provider: mockProvider, + apiConfiguration: mockApiConfig, + task: "test task", + startTask: false, + }) + const disposalError = new Error("Disposal failed asynchronously") + vi.spyOn(task, "dispose").mockRejectedValue(disposalError) + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + await expect(task.abortTask()).resolves.toBeUndefined() + await vi.waitFor(() => + expect(consoleErrorSpy).toHaveBeenCalledWith( + `Error during task ${task.taskId}.${task.instanceId} disposal:`, + disposalError, + ), + ) + + consoleErrorSpy.mockRestore() + }) describe("Stream Failure Retry", () => { it("should not abort task on stream failure, only on user cancellation", async () => { const task = new Task({ @@ -2374,7 +2440,7 @@ describe("Cline", () => { vi.spyOn(task, "removeAllListeners").mockImplementation(() => task) // Call dispose - task.dispose() + void task.dispose() // Verify cancelCurrentRequest was called expect(cancelSpy).toHaveBeenCalled() @@ -3903,9 +3969,9 @@ describe("Telemetry installments (idle/shutdown flush)", () => { const createdTasks: Task[] = [] - afterEach(() => { + afterEach(async () => { for (const task of createdTasks) { - task.dispose() + await task.dispose() } createdTasks.length = 0 vi.useRealTimers() @@ -4060,7 +4126,7 @@ describe("Telemetry installments (idle/shutdown flush)", () => { task.recordToolUsage("read_file") task.messageCounts = { user: 1, assistant: 1 } - task.dispose() + void task.dispose() expect(captureTaskCompletedSpy).toHaveBeenCalledWith( task.taskId, @@ -4076,7 +4142,7 @@ describe("Telemetry installments (idle/shutdown flush)", () => { task.flushTelemetryInstallment("attempt_completion") captureTaskCompletedSpy.mockClear() - task.dispose() + void task.dispose() expect(captureTaskCompletedSpy).not.toHaveBeenCalled() }) @@ -4085,7 +4151,7 @@ describe("Telemetry installments (idle/shutdown flush)", () => { vi.useFakeTimers() const task = createTask() task.recordToolUsage("read_file") - task.dispose() + void task.dispose() captureTaskCompletedSpy.mockClear() vi.advanceTimersByTime(60 * 60 * 1000) diff --git a/src/core/task/__tests__/Task.throttle.test.ts b/src/core/task/__tests__/Task.throttle.test.ts index eaacb32faf..f0b298bfdd 100644 --- a/src/core/task/__tests__/Task.throttle.test.ts +++ b/src/core/task/__tests__/Task.throttle.test.ts @@ -99,10 +99,10 @@ describe("Task token usage throttling", () => { }) }) - afterEach(() => { + afterEach(async () => { vi.useRealTimers() if (task && !task.abort) { - task.dispose() + await task.dispose() } }) diff --git a/src/core/task/__tests__/grace-retry-errors.spec.ts b/src/core/task/__tests__/grace-retry-errors.spec.ts index 9584559c8f..3f72924f21 100644 --- a/src/core/task/__tests__/grace-retry-errors.spec.ts +++ b/src/core/task/__tests__/grace-retry-errors.spec.ts @@ -237,7 +237,7 @@ describe("Grace Retry Error Handling", () => { task.consecutiveNoAssistantMessagesCount = 5 // Mock dispose to prevent actual cleanup - vi.spyOn(task, "dispose").mockImplementation(() => {}) + vi.spyOn(task, "dispose").mockResolvedValue(undefined) await task.abortTask() @@ -257,7 +257,7 @@ describe("Grace Retry Error Handling", () => { task.consecutiveNoToolUseCount = 4 // Mock dispose to prevent actual cleanup - vi.spyOn(task, "dispose").mockImplementation(() => {}) + vi.spyOn(task, "dispose").mockResolvedValue(undefined) await task.abortTask() diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0a251aba5f..138b0b6173 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -810,6 +810,17 @@ export class ClineProvider } } + /** Drain one task's memoized cleanup without preventing the remaining provider shutdown work. */ + private async drainTaskDisposal(task: Task): Promise { + try { + await task.dispose() + } catch (error) { + this.log( + `[ClineProvider#dispose] Task cleanup failed for ${task.taskId}.${task.instanceId}: ${error instanceof Error ? error.message : String(error)}`, + ) + } + } + async dispose() { if (this._disposed) { return @@ -827,10 +838,14 @@ export class ClineProvider // so an active delegated child is marked interrupted before the extension shuts down, // rather than being left persisted as "active" across the reload. if (this.taskRegistry.length > 0) { + const task = this.taskRegistry.current! await this.evictCurrentTask() + await this.drainTaskDisposal(task) } while (this.taskRegistry.length > 0) { + const task = this.taskRegistry.current! await this.removeClineFromStack() + await this.drainTaskDisposal(task) } this.log("Cleared all tasks") diff --git a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts index e0ece4f9f7..f2832b2468 100644 --- a/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts @@ -22,7 +22,12 @@ type CreatedHistoryTask = Awaited { taskId: "task-1", // Same ID for rehydration scenario instanceId: "instance-2", // Different instance emit: vi.fn(), + dispose: vi.fn().mockResolvedValue(undefined), on: vi.fn(), off: vi.fn(), } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index ad6ea143a8..1a6a82a5b0 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -266,6 +266,7 @@ vi.mock("../../task/Task", () => ({ return { api: undefined, abortTask: vi.fn(), + dispose: vi.fn().mockResolvedValue(undefined), handleWebviewAskResponse: vi.fn(), clineMessages: [], apiConversationHistory: [], @@ -413,6 +414,7 @@ describe("ClineProvider", () => { const task: any = { api: undefined, abortTask: vi.fn(), + dispose: vi.fn().mockResolvedValue(undefined), handleWebviewAskResponse: vi.fn(), clineMessages: [], apiConversationHistory: [], @@ -1138,6 +1140,131 @@ describe("ClineProvider", () => { expect(disposeCalls).toHaveLength(1) }) + test("dispose drains every task in abort-then-cleanup order", async () => { + let resolveCurrentAbort!: () => void + let resolveCurrentCleanup!: () => void + let resolveRemainingAbort!: () => void + let resolveRemainingCleanup!: () => void + const currentTask = { + taskId: "current-task", + instanceId: "current-instance", + emit: vi.fn(), + abortTask: vi.fn().mockReturnValue( + new Promise((resolve) => { + resolveCurrentAbort = resolve + }), + ), + dispose: vi.fn().mockReturnValue( + new Promise((resolve) => { + resolveCurrentCleanup = resolve + }), + ), + } + const remainingTask = { + taskId: "remaining-task", + instanceId: "remaining-instance", + emit: vi.fn(), + abortTask: vi.fn().mockReturnValue( + new Promise((resolve) => { + resolveRemainingAbort = resolve + }), + ), + dispose: vi.fn().mockReturnValue( + new Promise((resolve) => { + resolveRemainingCleanup = resolve + }), + ), + } + Object.assign(provider, { taskRegistry: new TaskRegistry() }) + provider["taskRegistry"].push(remainingTask as unknown as Task) + provider["taskRegistry"].push(currentTask as unknown as Task) + let shutdownComplete = false + + const shutdown = provider.dispose() + void shutdown + .then(() => { + shutdownComplete = true + }) + .catch(() => {}) + await vi.waitFor(() => expect(currentTask.abortTask).toHaveBeenCalledOnce()) + expect(currentTask.dispose).not.toHaveBeenCalled() + expect(remainingTask.abortTask).not.toHaveBeenCalled() + + resolveCurrentAbort() + await vi.waitFor(() => expect(currentTask.dispose).toHaveBeenCalledOnce()) + expect(remainingTask.abortTask).not.toHaveBeenCalled() + + resolveCurrentCleanup() + await vi.waitFor(() => expect(remainingTask.abortTask).toHaveBeenCalledOnce()) + expect(remainingTask.dispose).not.toHaveBeenCalled() + + resolveRemainingAbort() + await vi.waitFor(() => expect(remainingTask.dispose).toHaveBeenCalledOnce()) + + expect(shutdownComplete).toBe(false) + resolveRemainingCleanup() + await shutdown + expect(shutdownComplete).toBe(true) + }) + + test("dispose continues draining tasks after cleanup rejects", async () => { + const cleanupError = new Error("cleanup failed") + const logSpy = vi.spyOn(provider, "log") + const remainingTask = { + taskId: "remaining-task", + instanceId: "remaining-instance", + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + dispose: vi.fn().mockResolvedValue(undefined), + } + const currentTask = { + taskId: "current-task", + instanceId: "current-instance", + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + dispose: vi.fn().mockRejectedValue(cleanupError), + } + Object.assign(provider, { taskRegistry: new TaskRegistry() }) + provider["taskRegistry"].push(remainingTask as unknown as Task) + provider["taskRegistry"].push(currentTask as unknown as Task) + + await expect(provider.dispose()).resolves.toBeUndefined() + + expect(currentTask.dispose).toHaveBeenCalledOnce() + expect(remainingTask.dispose).toHaveBeenCalledOnce() + expect(logSpy).toHaveBeenCalledWith( + "[ClineProvider#dispose] Task cleanup failed for current-task.current-instance: cleanup failed", + ) + }) + + test("dispose continues draining tasks after abort rejects", async () => { + const abortError = new Error("abort failed") + const remainingTask = { + taskId: "remaining-task", + instanceId: "remaining-instance", + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + dispose: vi.fn().mockResolvedValue(undefined), + } + const currentTask = { + taskId: "current-task", + instanceId: "current-instance", + emit: vi.fn(), + abortTask: vi.fn().mockRejectedValue(abortError), + dispose: vi.fn().mockResolvedValue(undefined), + } + Object.assign(provider, { taskRegistry: new TaskRegistry() }) + provider["taskRegistry"].push(remainingTask as unknown as Task) + provider["taskRegistry"].push(currentTask as unknown as Task) + + await expect(provider.dispose()).resolves.toBeUndefined() + + expect(currentTask.abortTask).toHaveBeenCalledOnce() + expect(currentTask.dispose).toHaveBeenCalledOnce() + expect(remainingTask.abortTask).toHaveBeenCalledOnce() + expect(remainingTask.dispose).toHaveBeenCalledOnce() + }) + test("handles webviewDidLaunch message", async () => { await provider.resolveWebviewView(mockWebviewView)