fix(task): guard saveClineMessages against abandoned tasks (fixes #1021)

src/core/task/Task.ts

@@ -1811,6 +1811,20 @@ export class Task extends EventEmitter<TaskEvents> 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) {
+				// The message write succeeded, and the boolean reflects the message write
+				// only (see method docs): returning false here would make callers retry an
+				// already-persisted save (persistQueuedFeedbackAndAcknowledge) or skip a
+				// durable-state-gated update whose on-disk record is in fact current
+				// (finalizePartialToolAsk). Only the history write is intentionally skipped.
+				return true
+			}
+
 			const provider = this.providerRef.deref()
 			const existingStatus = provider?.taskHistoryStore.get(this.taskId)?.status
 			await provider?.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem)

src/core/task/__tests__/Task.spec.ts

@@ -35,6 +35,9 @@ import { processUserContentMentions } from "../../mentions/processUserContentMen
 import { MultiSearchReplaceDiffStrategy } from "../../diff/strategies/multi-search-replace"
 import type { ApiMessage } from "../../task-persistence"
 import type { RequestPolicySnapshot } from "../../prompts/tools/effective-tool-policy"
+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"
@@ -8252,3 +8255,179 @@ 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.requestAbort(true)
+
+			const saved = await getTaskTestAccess(task).saveClineMessages()
+
+			// The boolean reflects the message write only: the write succeeded, so the
+			// save reports success even though the history update is skipped.
+			expect(saved).toBe(true)
+			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<import("@roo-code/types").ClineMessage[]>((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.requestAbort(true)
+			resolveSave([])
+
+			const saved = await savePromise
+
+			// Write-success contract: the guard skips the history update, not the save.
+			expect(saved).toBe(true)
+			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()
+		}
+	})
+})