diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index 88bd8141b8..c57293fce7 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -1407,6 +1407,15 @@ export class Task extends EventEmitter implements TaskLike { // - Final state is emitted when updates stop (trailing: true) this.debouncedEmitTokenUsage(tokenUsage, this.toolUsage) + // Guard: don't update the history item for abandoned tasks. Fire-and-forget + // saveClineMessages() calls can reach updateTaskHistory() after + // abandonSubtask's atomicUpdatePair() has already cleared + // parentTaskId/rootTaskId; writing this live Task's stale values would + // silently reattach the severed parent-child link. + if (this.abandoned) { + return false + } + const provider = this.providerRef.deref() const existingStatus = provider?.taskHistoryStore.get(this.taskId)?.status await provider?.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 72d0641efd..604f288983 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -30,6 +30,9 @@ import { ContextProxy } from "../../config/ContextProxy" import { processUserContentMentions } from "../../mentions/processUserContentMentions" import { MultiSearchReplaceDiffStrategy } from "../../diff/strategies/multi-search-replace" import type { ApiMessage } from "../../task-persistence" +import * as taskMetadataModule from "../../task-persistence/taskMetadata" +import * as taskMessagesModule from "../../task-persistence/taskMessages" +import { getApiMetrics } from "../../../shared/getApiMetrics" import { asyncStreamFrom } from "../../../test-utils/stream" import { McpHub } from "../../../services/mcp/McpHub" import { McpServerManager } from "../../../services/mcp/McpServerManager" @@ -6498,3 +6501,176 @@ describe("pushToolResultToUserContent", () => { expect(task.userMessageContent[2]).toEqual(toolResult) }) }) + +describe("saveClineMessages abandoned guard (#1021)", () => { + beforeEach(() => { + if (!TelemetryService.hasInstance()) { + TelemetryService.createInstance([]) + } + }) + + // The history item carries the stale link: a fire-and-forget save that + // reaches updateTaskHistory() after abandonSubtask's atomicUpdatePair() + // cleared parentTaskId/rootTaskId would silently reattach the severed + // parent-child link. + const staleHistoryItem: HistoryItem = { + id: "orphan-subtask", + number: 7, + ts: Date.now(), + task: "orphan subtask", + tokensIn: 0, + tokensOut: 0, + totalCost: 0, + rootTaskId: "stale-root", + parentTaskId: "stale-parent", + } + + // Task receives a full ClineProvider at runtime; these focused unit tests only + // exercise these methods, so the partial double is cast. taskHistoryStore must + // be stubbed: without it provider?.taskHistoryStore.get() throws before the + // guard is evaluated and the catch would mask whether execution reached + // updateTaskHistory(). + // + // The double does not structurally overlap MockedClineProvider, so a direct + // literal `as` fails (TS2352). Object.assign over an empty-object single cast + // keeps one assertion: the target carries the declared type and the source's + // property types stay inferred. + function makeMockProvider() { + const mockProvider = Object.assign({} as MockedClineProvider, { + context: { + globalStorageUri: { fsPath: "/test/storage" }, + globalState: { + get: vi.fn().mockImplementation(() => undefined), + update: vi.fn().mockResolvedValue(undefined), + keys: vi.fn().mockReturnValue([]), + }, + }, + getState: vi.fn().mockResolvedValue({ + apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, + mcpEnabled: false, + }), + getMcpHub: vi.fn().mockReturnValue(undefined), + postMessageToWebview: vi.fn().mockResolvedValue(undefined), + updateTaskHistory: vi.fn().mockResolvedValue(undefined), + taskHistoryStore: { get: vi.fn(() => undefined) }, + }) + return mockProvider + } + + function createTask(provider: MockedClineProvider) { + return new Task({ + provider, + apiConfiguration: { apiProvider: providerIdentifiers.anthropic, apiKey: "test-key" }, + task: "orphan subtask", + startTask: false, + }) + } + + it("persists messages but does not update task history when the task was abandoned", async () => { + const saveSpy = vi.spyOn(taskMessagesModule, "saveTaskMessages").mockResolvedValue([]) + const metaSpy = vi + .spyOn(taskMetadataModule, "taskMetadata") + .mockResolvedValue({ historyItem: staleHistoryItem, tokenUsage: getApiMetrics([]) }) + + try { + const mockProvider = makeMockProvider() + const task = createTask(mockProvider) + + // Seed a message so the payload assertion is meaningful: clineMessages + // starts empty, and an empty-payload save would pass a call-count-only + // assertion even if the save dropped the messages field. + task.clineMessages = [{ ts: 1, type: "say", say: "text", text: "seeded message" }] + + // abandonSubtask severs the link, then aborts the subtask with + // isAbandoned=true; an in-flight fire-and-forget save lands here. + task.abandoned = true + + const saved = await getTaskTestAccess(task).saveClineMessages() + + expect(saved).toBe(false) + expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ + taskId: task.taskId, + messages: expect.arrayContaining([expect.objectContaining({ text: "seeded message" })]), + }), + ) + expect(mockProvider.updateTaskHistory).not.toHaveBeenCalled() // history link is not reattached + } finally { + saveSpy.mockRestore() + metaSpy.mockRestore() + } + }) + + it("updates task history for a non-abandoned task (guard does not block the normal path)", async () => { + const saveSpy = vi.spyOn(taskMessagesModule, "saveTaskMessages").mockResolvedValue([]) + const metaSpy = vi + .spyOn(taskMetadataModule, "taskMetadata") + .mockResolvedValue({ historyItem: staleHistoryItem, tokenUsage: getApiMetrics([]) }) + + try { + const mockProvider = makeMockProvider() + const task = createTask(mockProvider) + + const saved = await getTaskTestAccess(task).saveClineMessages() + + expect(saved).toBe(true) + expect(saveSpy).toHaveBeenCalledTimes(1) + // Control case: the guard must not block the normal path, so execution + // genuinely reached updateTaskHistory(). No pre-existing store entry, + // so the item is written as-is. + expect(mockProvider.updateTaskHistory).toHaveBeenCalledTimes(1) + expect(mockProvider.updateTaskHistory).toHaveBeenCalledWith(staleHistoryItem) + } finally { + saveSpy.mockRestore() + metaSpy.mockRestore() + } + }) + + it("skips the history update when the task is abandoned while the save is in flight", async () => { + // Fire-and-forget race: the save starts while the task is still active, + // abandonSubtask severs the link mid-save, and the guard must catch it + // when the awaited saveTaskMessages() finally resolves. + let resolveSave!: (value: import("@roo-code/types").ClineMessage[]) => void + const saveSpy = vi + .spyOn(taskMessagesModule, "saveTaskMessages") + .mockImplementation( + () => new Promise((resolve) => (resolveSave = resolve)), + ) + const metaSpy = vi + .spyOn(taskMetadataModule, "taskMetadata") + .mockResolvedValue({ historyItem: staleHistoryItem, tokenUsage: getApiMetrics([]) }) + + try { + const mockProvider = makeMockProvider() + const task = createTask(mockProvider) + + // Seed a message so the payload assertion is meaningful: clineMessages + // starts empty, and an empty-payload save would pass a call-count-only + // assertion even if the save dropped the messages field. + task.clineMessages = [{ ts: 1, type: "say", say: "text", text: "seeded message" }] + + const savePromise = getTaskTestAccess(task).saveClineMessages() + + // The save is in flight (awaiting saveTaskMessages) when the task is + // abandoned; only after it resumes does the save hit the guard. + task.abandoned = true + resolveSave([]) + + const saved = await savePromise + + expect(saved).toBe(false) + expect(saveSpy).toHaveBeenCalledTimes(1) // messages are still persisted + expect(saveSpy).toHaveBeenCalledWith( + expect.objectContaining({ + messages: expect.arrayContaining([expect.objectContaining({ text: "seeded message" })]), + }), + ) + expect(mockProvider.updateTaskHistory).not.toHaveBeenCalled() // history link is not reattached + } finally { + resolveSave?.([]) // settle the deferred save if an assertion failed above + saveSpy.mockRestore() + metaSpy.mockRestore() + } + }) +})