From 444e9c7e3957439693e43962675f92ef88e87d42 Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 16:34:45 +0000 Subject: [PATCH 01/10] fix(task): await disposal cleanup (#1526) --- packages/types/src/task.ts | 2 +- src/core/task/Task.ts | 27 ++++--- src/core/task/__tests__/Task.dispose.test.ts | 72 +++++++++++++++---- src/core/task/__tests__/Task.spec.ts | 49 ++++++++++--- src/core/task/__tests__/Task.throttle.test.ts | 4 +- .../task/__tests__/grace-retry-errors.spec.ts | 4 +- 6 files changed, 118 insertions(+), 40 deletions(-) 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/src/core/task/Task.ts b/src/core/task/Task.ts index 37281a9010..50fd95f52e 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -2493,7 +2493,7 @@ export class Task extends EventEmitter implements TaskLike { this.emit(RooCodeEventName.TaskAborted) try { - this.dispose() // Call the centralized dispose method + await this.dispose() // Call the centralized dispose method } catch (error) { console.error(`Error during task ${this.taskId}.${this.instanceId} disposal:`, error) // Don't rethrow - we want abort to always succeed @@ -2514,8 +2514,9 @@ export class Task extends EventEmitter implements TaskLike { } } - public dispose(): void { + public dispose(): Promise { console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`) + const pendingCleanup: Promise[] = [] // Stop the idle telemetry check and report any unflushed activity as a // shutdown installment, so a task torn down mid-work (panel closed, task @@ -2563,14 +2564,16 @@ export class Task extends EventEmitter implements TaskLike { } // Cleanup command output artifacts - getTaskDirectoryPath(this.globalStoragePath, this.taskId) - .then((taskDir) => { - const outputDir = path.join(taskDir, "command-output") - return OutputInterceptor.cleanup(outputDir) - }) - .catch((error) => { - console.error("Error cleaning up command output artifacts:", error) - }) + pendingCleanup.push( + getTaskDirectoryPath(this.globalStoragePath, this.taskId) + .then((taskDir) => { + const outputDir = path.join(taskDir, "command-output") + return OutputInterceptor.cleanup(outputDir) + }) + .catch((error) => { + console.error("Error cleaning up command output artifacts:", error) + }), + ) try { if (this.rooIgnoreController) { @@ -2591,11 +2594,13 @@ 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) + pendingCleanup.push(this.diffViewProvider.revertChanges().catch(console.error)) } } catch (error) { console.error("Error reverting diff changes:", error) } + + return Promise.all(pendingCleanup).then(() => undefined) } // Subtasks diff --git a/src/core/task/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 9f00e9d852..8a712c6280 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -2,7 +2,9 @@ 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 +13,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"), })) @@ -80,13 +79,60 @@ describe("Task dispose method", () => { }) }) - afterEach(() => { + afterEach(async () => { // Clean up if (task && !task.abort) { - task.dispose() + await task.dispose() } }) + 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("/test/path/tasks/test-task/command-output") + 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 + vi.spyOn(task.diffViewProvider, "revertChanges").mockReturnValue(reversion) + + const disposal = task.dispose() + let disposalComplete = false + void disposal.then(() => { + disposalComplete = true + }) + await Promise.resolve() + expect(disposalComplete).toBe(false) + + resolveReversion!() + await disposal + expect(disposalComplete).toBe(true) + }) + 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 +152,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 +174,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 +189,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 +239,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 +290,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 +308,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 +326,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..0c2877bf3a 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -2111,7 +2111,7 @@ 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() @@ -2132,7 +2132,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 +2142,33 @@ describe("Cline", () => { expect(disposeSpy).toHaveBeenCalled() }) + it("waits for 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()) + let abortComplete = false + void abort.then(() => { + abortComplete = true + }) + await Promise.resolve() + expect(abortComplete).toBe(false) + + resolveDisposal!() + await abort + expect(abortComplete).toBe(true) + }) + it("flushes pending state before TaskAborted and disposal while queue state is intact", async () => { const task = new Task({ provider: mockProvider, @@ -2154,7 +2181,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 +2210,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 +2247,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() @@ -2374,7 +2401,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 +3930,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 +4087,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 +4103,7 @@ describe("Telemetry installments (idle/shutdown flush)", () => { task.flushTelemetryInstallment("attempt_completion") captureTaskCompletedSpy.mockClear() - task.dispose() + void task.dispose() expect(captureTaskCompletedSpy).not.toHaveBeenCalled() }) @@ -4085,7 +4112,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() From d95df419b82d8beab072e6fcf66ac27a5c0e4cf7 Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 16:52:02 +0000 Subject: [PATCH 02/10] fix(task): cover cleanup completion branches --- src/core/task/Task.ts | 24 +++++++++----------- src/core/task/__tests__/Task.dispose.test.ts | 21 +++++++++++++++-- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 50fd95f52e..1b438247ac 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -2516,7 +2516,6 @@ export class Task extends EventEmitter implements TaskLike { public dispose(): Promise { console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`) - const pendingCleanup: Promise[] = [] // Stop the idle telemetry check and report any unflushed activity as a // shutdown installment, so a task torn down mid-work (panel closed, task @@ -2564,16 +2563,14 @@ export class Task extends EventEmitter implements TaskLike { } // Cleanup command output artifacts - pendingCleanup.push( - getTaskDirectoryPath(this.globalStoragePath, this.taskId) - .then((taskDir) => { - const outputDir = path.join(taskDir, "command-output") - return OutputInterceptor.cleanup(outputDir) - }) - .catch((error) => { - console.error("Error cleaning up command output artifacts:", error) - }), - ) + let pendingCleanup = getTaskDirectoryPath(this.globalStoragePath, this.taskId) + .then((taskDir) => { + const outputDir = path.join(taskDir, "command-output") + return OutputInterceptor.cleanup(outputDir) + }) + .catch((error) => { + console.error("Error cleaning up command output artifacts:", error) + }) try { if (this.rooIgnoreController) { @@ -2594,13 +2591,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) { - pendingCleanup.push(this.diffViewProvider.revertChanges().catch(console.error)) + const pendingReversion = this.diffViewProvider.revertChanges().catch(console.error) + pendingCleanup = pendingCleanup.then(() => pendingReversion) } } catch (error) { console.error("Error reverting diff changes:", error) } - return Promise.all(pendingCleanup).then(() => undefined) + return pendingCleanup } // Subtasks diff --git a/src/core/task/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 8a712c6280..9e6665f8bb 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -1,3 +1,5 @@ +import path from "node:path" + import { type ProviderSettings, RooCodeEventName } from "@roo-code/types" import { Task } from "../Task" @@ -107,10 +109,23 @@ describe("Task dispose method", () => { resolveTaskDirectory!("/test/path/tasks/test-task") await disposal - expect(OutputInterceptor.cleanup).toHaveBeenCalledWith("/test/path/tasks/test-task/command-output") + expect(OutputInterceptor.cleanup).toHaveBeenCalledWith( + path.join("/test/path/tasks/test-task", "command-output"), + ) expect(disposalComplete).toBe(true) }) + test("should report command output cleanup failures before disposal completes", async () => { + const cleanupError = new Error("cleanup failed") + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + vi.mocked(getTaskDirectoryPath).mockRejectedValueOnce(cleanupError) + + await task.dispose() + + expect(consoleErrorSpy).toHaveBeenCalledWith("Error cleaning up command output artifacts:", cleanupError) + consoleErrorSpy.mockRestore() + }) + test("should expose completion of deferred diff reversion", async () => { let resolveReversion: () => void const reversion = new Promise((resolve) => { @@ -118,15 +133,17 @@ describe("Task dispose method", () => { }) task.isStreaming = true task.diffViewProvider.isEditing = true - vi.spyOn(task.diffViewProvider, "revertChanges").mockReturnValue(reversion) + 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 From 1141702ca5460bc63d7c12c8e0ae1f984101160f Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 17:54:52 +0000 Subject: [PATCH 03/10] fix(task): preserve abort completion semantics --- src/core/task/Task.ts | 49 ++++++++-- src/core/task/__tests__/Task.dispose.test.ts | 90 +++++++++++++++++-- src/core/task/__tests__/Task.spec.ts | 34 +++++-- src/core/webview/ClineProvider.ts | 4 + .../ClineProvider.flicker-free-cancel.spec.ts | 8 +- .../webview/__tests__/ClineProvider.spec.ts | 29 ++++++ 6 files changed, 190 insertions(+), 24 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 1b438247ac..dfb61471ef 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 { - await 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 @@ -2515,6 +2526,27 @@ export class Task extends EventEmitter implements TaskLike { } public dispose(): Promise { + if (this.disposalPromise) { + return this.disposalPromise + } + + let resolveDisposal!: () => void + let rejectDisposal!: (error: unknown) => void + this.disposalPromise = new Promise((resolve, reject) => { + resolveDisposal = resolve + rejectDisposal = reject + }) + + try { + void this.disposeOnce().then(resolveDisposal, rejectDisposal) + } catch (error) { + rejectDisposal(error) + } + + return this.disposalPromise + } + + private 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 +2595,7 @@ export class Task extends EventEmitter implements TaskLike { } // Cleanup command output artifacts - let pendingCleanup = 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,14 +2623,13 @@ 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) { - const pendingReversion = this.diffViewProvider.revertChanges().catch(console.error) - pendingCleanup = pendingCleanup.then(() => pendingReversion) + this.diffReversionPromise = this.diffViewProvider.revertChanges().catch(console.error) } } catch (error) { console.error("Error reverting diff changes:", error) } - return pendingCleanup + return pendingCleanup.then(() => this.diffReversionPromise) } // Subtasks diff --git a/src/core/task/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 9e6665f8bb..9cb35694ff 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -50,6 +50,7 @@ describe("Task dispose method", () => { context: { globalStorageUri: { fsPath: string } } getState: ReturnType log: ReturnType + flushPostStateToWebviewThrottled: ReturnType } let mockApiConfiguration: ProviderSettings let task: Task @@ -65,6 +66,7 @@ describe("Task dispose method", () => { }, getState: vi.fn().mockResolvedValue({ mode: "code" }), log: vi.fn(), + flushPostStateToWebviewThrottled: vi.fn().mockResolvedValue(undefined), } // Mock API configuration @@ -82,8 +84,7 @@ describe("Task dispose method", () => { }) afterEach(async () => { - // Clean up - if (task && !task.abort) { + if (task) { await task.dispose() } }) @@ -117,15 +118,94 @@ describe("Task dispose method", () => { test("should report command output cleanup failures before disposal completes", async () => { const cleanupError = new Error("cleanup failed") - const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - vi.mocked(getTaskDirectoryPath).mockRejectedValueOnce(cleanupError) + 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 + }), + ) - await task.dispose() + 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) => { diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 0c2877bf3a..909a51c4f7 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -2142,7 +2142,7 @@ describe("Cline", () => { expect(disposeSpy).toHaveBeenCalled() }) - it("waits for disposal cleanup before abort resolves", async () => { + it("does not wait for ancillary disposal cleanup before abort resolves", async () => { const task = new Task({ provider: mockProvider, apiConfiguration: mockApiConfig, @@ -2157,16 +2157,32 @@ describe("Cline", () => { const abort = task.abortTask() await vi.waitFor(() => expect(disposeSpy).toHaveBeenCalledOnce()) - let abortComplete = false - void abort.then(() => { - abortComplete = true - }) - await Promise.resolve() - expect(abortComplete).toBe(false) + await abort + expect(disposeSpy).toHaveBeenCalledOnce() resolveDisposal!() - await abort - expect(abortComplete).toBe(true) + 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 () => { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 0a251aba5f..c1932b9da6 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -827,10 +827,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 task?.dispose() } while (this.taskRegistry.length > 0) { + const task = this.taskRegistry.current await this.removeClineFromStack() + await task?.dispose() } 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..2154d73b2e 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,33 @@ describe("ClineProvider", () => { expect(disposeCalls).toHaveLength(1) }) + test("dispose drains task cleanup after abort completes", async () => { + let resolveCleanup!: () => void + const cleanup = new Promise((resolve) => { + resolveCleanup = resolve + }) + const task = { + taskId: "shutdown-task", + emit: vi.fn(), + abortTask: vi.fn().mockResolvedValue(undefined), + dispose: vi.fn().mockReturnValue(cleanup), + } + Object.assign(provider, { taskRegistry: new TaskRegistry() }) + provider["taskRegistry"].push(task as unknown as Task) + let shutdownComplete = false + + const shutdown = provider.dispose() + void shutdown.then(() => { + shutdownComplete = true + }) + await vi.waitFor(() => expect(task.dispose).toHaveBeenCalledOnce()) + + expect(shutdownComplete).toBe(false) + resolveCleanup() + await shutdown + expect(shutdownComplete).toBe(true) + }) + test("handles webviewDidLaunch message", async () => { await provider.resolveWebviewView(mockWebviewView) From f008344eb1332d276ca357733153f740a561fef2 Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 18:00:21 +0000 Subject: [PATCH 04/10] test(task): cover disposal failure branches --- src/core/task/__tests__/Task.dispose.test.ts | 14 +++++++++++- src/core/task/__tests__/Task.spec.ts | 23 ++++++++++++++++++++ src/core/webview/ClineProvider.ts | 8 +++---- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/core/task/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 9cb35694ff..8f346697b4 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -85,7 +85,7 @@ describe("Task dispose method", () => { afterEach(async () => { if (task) { - await task.dispose() + await task.dispose().catch(() => {}) } }) @@ -116,6 +116,18 @@ describe("Task dispose method", () => { expect(disposalComplete).toBe(true) }) + test("should reject the memoized completion promise when disposal cannot start", async () => { + const disposalError = new Error("disposal failed") + vi.spyOn(console, "log").mockImplementationOnce(() => { + throw disposalError + }) + + const disposal = task.dispose() + + await expect(disposal).rejects.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 diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 909a51c4f7..0376f437cb 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -2118,6 +2118,7 @@ describe("Cline", () => { // Verify abort flag is set expect(task.abort).toBe(true) + expect(task.abandoned).toBe(false) // Verify TaskAborted event was emitted expect(emitSpy).toHaveBeenCalledWith("taskAborted") @@ -2301,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({ diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index c1932b9da6..42edc2f6e6 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -827,14 +827,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 + const task = this.taskRegistry.current! await this.evictCurrentTask() - await task?.dispose() + await task.dispose() } while (this.taskRegistry.length > 0) { - const task = this.taskRegistry.current + const task = this.taskRegistry.current! await this.removeClineFromStack() - await task?.dispose() + await task.dispose() } this.log("Cleared all tasks") From bd193469f1be7f84c870f6a036de2ee43459647f Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 18:06:09 +0000 Subject: [PATCH 05/10] test(task): fail fast on stalled disposal --- src/core/task/__tests__/Task.dispose.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/core/task/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 8f346697b4..5e8d391cf6 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -54,10 +54,12 @@ describe("Task dispose method", () => { } let mockApiConfiguration: ProviderSettings let task: Task + let skipCleanup: boolean beforeEach(() => { // Reset all mocks vi.clearAllMocks() + skipCleanup = false // Mock provider mockProvider = { @@ -84,7 +86,7 @@ describe("Task dispose method", () => { }) afterEach(async () => { - if (task) { + if (task && !skipCleanup) { await task.dispose().catch(() => {}) } }) @@ -118,13 +120,19 @@ describe("Task dispose method", () => { 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() - await expect(disposal).rejects.toBe(disposalError) + expect(rejection).toBe(disposalError) expect(task.dispose()).toBe(disposal) }) From 7d135f96d41659ec62aa12fcd0b240c86efdd3c9 Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 19:14:32 +0000 Subject: [PATCH 06/10] test(task): cover multi-task shutdown disposal --- src/core/webview/ClineProvider.ts | 15 ++- .../webview/__tests__/ClineProvider.spec.ts | 92 ++++++++++++++++--- 2 files changed, 93 insertions(+), 14 deletions(-) diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 42edc2f6e6..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 @@ -829,12 +840,12 @@ export class ClineProvider if (this.taskRegistry.length > 0) { const task = this.taskRegistry.current! await this.evictCurrentTask() - await task.dispose() + await this.drainTaskDisposal(task) } while (this.taskRegistry.length > 0) { const task = this.taskRegistry.current! await this.removeClineFromStack() - await task.dispose() + await this.drainTaskDisposal(task) } this.log("Cleared all tasks") diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 2154d73b2e..02052aac65 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1140,33 +1140,101 @@ describe("ClineProvider", () => { expect(disposeCalls).toHaveLength(1) }) - test("dispose drains task cleanup after abort completes", async () => { - let resolveCleanup!: () => void - const cleanup = new Promise((resolve) => { - resolveCleanup = resolve - }) - const task = { - taskId: "shutdown-task", + 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().mockResolvedValue(undefined), - dispose: vi.fn().mockReturnValue(cleanup), + 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(task as unknown as Task) + 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 }) - await vi.waitFor(() => expect(task.dispose).toHaveBeenCalledOnce()) + 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) - resolveCleanup() + 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("handles webviewDidLaunch message", async () => { await provider.resolveWebviewView(mockWebviewView) From 7bddaf19c2e2f6dea9f41e12f7432a2c4d165f4f Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 19:56:44 +0000 Subject: [PATCH 07/10] test(formal): model task cleanup protocol --- .../task-cleanup-protocol-model.md | 33 ++ docs/architecture/task-lifecycle-model.md | 4 + package.json | 3 +- scripts/check-task-cleanup-protocol.ts | 323 ++++++++++++++++++ src/core/task/__tests__/Task.dispose.test.ts | 17 + .../webview/__tests__/ClineProvider.spec.ts | 35 +- 6 files changed, 411 insertions(+), 4 deletions(-) create mode 100644 docs/architecture/task-cleanup-protocol-model.md create mode 100644 scripts/check-task-cleanup-protocol.ts diff --git a/docs/architecture/task-cleanup-protocol-model.md b/docs/architecture/task-cleanup-protocol-model.md new file mode 100644 index 0000000000..2dc2251fc3 --- /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 50,000-state budget. Abort, disposal, final-save, provider-drain, and shutdown-cursor state are modeled directly. 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/scripts/check-task-cleanup-protocol.ts b/scripts/check-task-cleanup-protocol.ts new file mode 100644 index 0000000000..9b43d97c56 --- /dev/null +++ b/scripts/check-task-cleanup-protocol.ts @@ -0,0 +1,323 @@ +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" | "draining" | "done" + shutdownIndex: number + drained: Record +} + +interface Step { + action: string + state: ModelState +} + +const MAX_DEPTH = 20 +const MAX_STATES = 50_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", + "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 = "draining" + result.push({ action: "start-shutdown()", state: next }) + } else if (state.shutdown === "draining") { + const taskId = taskIds[state.shutdownIndex] + if (taskId) { + const current = state.tasks[taskId] + if (isTerminal(current.abort) && isTerminal(current.disposal)) { + const next = clone(state) + next.drained[taskId] = true + next.shutdownIndex += 1 + if (next.shutdownIndex === taskIds.length) next.shutdown = "done" + 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.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/__tests__/Task.dispose.test.ts b/src/core/task/__tests__/Task.dispose.test.ts index 5e8d391cf6..472218fce5 100644 --- a/src/core/task/__tests__/Task.dispose.test.ts +++ b/src/core/task/__tests__/Task.dispose.test.ts @@ -250,6 +250,23 @@ describe("Task dispose method", () => { 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(() => {}) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 02052aac65..690051b759 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1181,9 +1181,11 @@ describe("ClineProvider", () => { let shutdownComplete = false const shutdown = provider.dispose() - void shutdown.then(() => { - shutdownComplete = true - }) + void shutdown + .then(() => { + shutdownComplete = true + }) + .catch(() => {}) await vi.waitFor(() => expect(currentTask.abortTask).toHaveBeenCalledOnce()) expect(currentTask.dispose).not.toHaveBeenCalled() expect(remainingTask.abortTask).not.toHaveBeenCalled() @@ -1235,6 +1237,33 @@ describe("ClineProvider", () => { ) }) + 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.dispose).toHaveBeenCalledOnce() + expect(remainingTask.abortTask).toHaveBeenCalledOnce() + expect(remainingTask.dispose).toHaveBeenCalledOnce() + }) + test("handles webviewDidLaunch message", async () => { await provider.resolveWebviewView(mockWebviewView) From a54e4db6a8c95b73128a7eb056a28f5e59062b81 Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 20:06:03 +0000 Subject: [PATCH 08/10] test(formal): model provider cleanup phases --- .../task-cleanup-protocol-model.md | 2 +- scripts/check-task-cleanup-protocol.ts | 25 ++++++++++++++----- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/docs/architecture/task-cleanup-protocol-model.md b/docs/architecture/task-cleanup-protocol-model.md index 2dc2251fc3..e46991365b 100644 --- a/docs/architecture/task-cleanup-protocol-model.md +++ b/docs/architecture/task-cleanup-protocol-model.md @@ -16,7 +16,7 @@ This is a separate child model from the persisted task lifecycle and shared-stor ## Bounds and environment actions -The model uses two tasks and explores every reachable interleaving through depth 20, with an explicit 50,000-state budget. Abort, disposal, final-save, provider-drain, and shutdown-cursor state are modeled directly. Cleanup and editor-reversion settlement or rejection are environment actions, so the explorer does not assume they eventually occur. +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: diff --git a/scripts/check-task-cleanup-protocol.ts b/scripts/check-task-cleanup-protocol.ts index 9b43d97c56..466fad6464 100644 --- a/scripts/check-task-cleanup-protocol.ts +++ b/scripts/check-task-cleanup-protocol.ts @@ -18,7 +18,7 @@ interface TaskState { interface ModelState { tasks: Record - shutdown: "idle" | "draining" | "done" + shutdown: "idle" | "call-abort" | "wait-abort" | "wait-disposal" | "done" shutdownIndex: number drained: Record } @@ -29,7 +29,7 @@ interface Step { } const MAX_DEPTH = 20 -const MAX_STATES = 50_000 +const MAX_STATES = 100_000 const expectedActions = [ "abort", "dispose", @@ -44,6 +44,8 @@ const expectedActions = [ "skip-final-save", "complete-disposal", "start-shutdown", + "shutdown-abort", + "shutdown-dispose", "advance-shutdown", ] as const @@ -171,17 +173,25 @@ function transitions(state: ModelState): Step[] { if (state.shutdown === "idle") { const next = clone(state) - next.shutdown = "draining" + next.shutdown = "call-abort" result.push({ action: "start-shutdown()", state: next }) - } else if (state.shutdown === "draining") { + } else if (state.shutdown !== "done") { const taskId = taskIds[state.shutdownIndex] if (taskId) { const current = state.tasks[taskId] - if (isTerminal(current.abort) && isTerminal(current.disposal)) { + 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 - if (next.shutdownIndex === taskIds.length) next.shutdown = "done" + next.shutdown = next.shutdownIndex === taskIds.length ? "done" : "call-abort" result.push({ action: `advance-shutdown(${taskId})`, state: next }) } } @@ -222,6 +232,9 @@ function invariantViolations(state: ModelState): string[] { 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") } From ca1f005d086531e8e3d32f68d9bb924a3cb86894 Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 20:30:50 +0000 Subject: [PATCH 09/10] fix(task): simplify disposal promise memoization --- src/core/task/Task.ts | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index dfb61471ef..4f122feefc 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -2530,23 +2530,11 @@ export class Task extends EventEmitter implements TaskLike { return this.disposalPromise } - let resolveDisposal!: () => void - let rejectDisposal!: (error: unknown) => void - this.disposalPromise = new Promise((resolve, reject) => { - resolveDisposal = resolve - rejectDisposal = reject - }) - - try { - void this.disposeOnce().then(resolveDisposal, rejectDisposal) - } catch (error) { - rejectDisposal(error) - } - + this.disposalPromise = this.disposeOnce() return this.disposalPromise } - private disposeOnce(): Promise { + 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 @@ -2629,7 +2617,8 @@ export class Task extends EventEmitter implements TaskLike { console.error("Error reverting diff changes:", error) } - return pendingCleanup.then(() => this.diffReversionPromise) + await pendingCleanup + await this.diffReversionPromise } // Subtasks From d602da4c13ba6b2afd6171c962ef5709d5477d7b Mon Sep 17 00:00:00 2001 From: Roomote Date: Fri, 4 Sep 2026 21:20:52 +0000 Subject: [PATCH 10/10] test(task): assert rejected abort invocation --- src/core/webview/__tests__/ClineProvider.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 690051b759..1a6a82a5b0 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -1259,6 +1259,7 @@ describe("ClineProvider", () => { await expect(provider.dispose()).resolves.toBeUndefined() + expect(currentTask.abortTask).toHaveBeenCalledOnce() expect(currentTask.dispose).toHaveBeenCalledOnce() expect(remainingTask.abortTask).toHaveBeenCalledOnce() expect(remainingTask.dispose).toHaveBeenCalledOnce()