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

src/core/task/Task.ts

@@ -1885,6 +1885,32 @@
 			this.debouncedEmitTokenUsage(tokenUsage, this.toolUsage)
 
 			const provider = this.providerRef.deref()
+
+			// Fire-and-forget saveClineMessages() calls can reach this point after
+			// abandonSubtask's atomicUpdatePair() cleared parentTaskId/rootTaskId. Writing
+			// this live Task's stale link values would silently reattach the severed
+			// parent-child link. Keep the upsert (so a plain cancel still records its
+			// metadata) but omit the link keys when the store already has this task's row:
+			// upsertCore merges over the existing record, so omitted keys keep the
+			// severed/null values while tokens, cost, ts and title are refreshed.
+			if (this.abandoned) {
+				const existing = provider?.taskHistoryStore.get(this.taskId)
+				if (!existing) {
+					// 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). No stored row means there is no link to
+					// reattach, and creating one here would write a task the store never had.
+					return true
+				}
+				const { parentTaskId: _parentTaskId, rootTaskId: _rootTaskId, ...detachedHistoryItem } = historyItem
+				await provider?.updateTaskHistory(
+					existing.status ? { ...detachedHistoryItem, status: existing.status } : detachedHistoryItem,
+				)
+				return true
+			}
+
 			const existingStatus = provider?.taskHistoryStore.get(this.taskId)?.status
 			await provider?.updateTaskHistory(existingStatus ? { ...historyItem, status: existingStatus } : historyItem)
 		} catch (error) {

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

@@ -36,6 +36,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"
@@ -8679,3 +8682,224 @@ describe("pushToolResultToUserContent", () => {
 		expect(task.userMessageContent).toEqual([toolResult1, toolResult2, image, text1, text2])
 	})
 })
+
+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("refreshes an existing history row without reattaching the severed link when abandoned", async () => {
+		const saveSpy = vi.spyOn(taskMessagesModule, "saveTaskMessages").mockResolvedValue([])
+		const metaSpy = vi.spyOn(taskMetadataModule, "taskMetadata").mockResolvedValue({
+			historyItem: { ...staleHistoryItem, tokensIn: 11, tokensOut: 7, totalCost: 0.02 },
+			tokenUsage: getApiMetrics([]),
+		})
+
+		try {
+			const mockProvider = makeMockProvider()
+			// The row already exists with the severed link; a plain cancel's final save
+			// must refresh metadata but keep the link severed (upsertCore merges, so the
+			// link keys must be omitted from the update).
+			;(mockProvider.taskHistoryStore.get as ReturnType<typeof vi.fn>).mockReturnValue({
+				id: "orphan-subtask",
+				status: "completed",
+			})
+			const task = createTask(mockProvider)
+			task.clineMessages = [{ ts: 1, type: "say", say: "text", text: "seeded message" }]
+			task.requestAbort(true)
+
+			const saved = await getTaskTestAccess(task).saveClineMessages()
+
+			expect(saved).toBe(true)
+			expect(mockProvider.updateTaskHistory).toHaveBeenCalledTimes(1)
+			const update = (mockProvider.updateTaskHistory as ReturnType<typeof vi.fn>).mock.calls[0][0]
+			// Metadata is refreshed...
+			expect(update).toEqual(
+				expect.objectContaining({
+					id: "orphan-subtask",
+					tokensIn: 11,
+					tokensOut: 7,
+					totalCost: 0.02,
+					status: "completed",
+				}),
+			)
+			// ...but the update carries no link keys: the merge must leave the stored
+			// severed link untouched.
+			expect(update).not.toHaveProperty("parentTaskId")
+			expect(update).not.toHaveProperty("rootTaskId")
+		} 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()
+		}
+	})
+})