Browse Source

fix: prevent parent task state loss during orchestrator delegation (#11281)

Hannes Rudolph 5 days ago
parent
commit
6826e20da2

+ 86 - 0
src/core/task-persistence/__tests__/apiMessages.spec.ts

@@ -0,0 +1,86 @@
+// cd src && npx vitest run core/task-persistence/__tests__/apiMessages.spec.ts
+
+import * as os from "os"
+import * as path from "path"
+import * as fs from "fs/promises"
+
+import { readApiMessages } from "../apiMessages"
+
+let tmpBaseDir: string
+
+beforeEach(async () => {
+	tmpBaseDir = await fs.mkdtemp(path.join(os.tmpdir(), "roo-test-api-"))
+})
+
+describe("apiMessages.readApiMessages", () => {
+	it("returns empty array when api_conversation_history.json contains invalid JSON", async () => {
+		const taskId = "task-corrupt-api"
+		const taskDir = path.join(tmpBaseDir, "tasks", taskId)
+		await fs.mkdir(taskDir, { recursive: true })
+		const filePath = path.join(taskDir, "api_conversation_history.json")
+		await fs.writeFile(filePath, "<<<corrupt data>>>", "utf8")
+
+		const result = await readApiMessages({
+			taskId,
+			globalStoragePath: tmpBaseDir,
+		})
+
+		expect(result).toEqual([])
+	})
+
+	it("returns empty array when claude_messages.json fallback contains invalid JSON", async () => {
+		const taskId = "task-corrupt-fallback"
+		const taskDir = path.join(tmpBaseDir, "tasks", taskId)
+		await fs.mkdir(taskDir, { recursive: true })
+
+		// Only write the old fallback file (claude_messages.json), NOT the new one
+		const oldPath = path.join(taskDir, "claude_messages.json")
+		await fs.writeFile(oldPath, "not json at all {[!", "utf8")
+
+		const result = await readApiMessages({
+			taskId,
+			globalStoragePath: tmpBaseDir,
+		})
+
+		expect(result).toEqual([])
+
+		// The corrupted fallback file should NOT be deleted
+		const stillExists = await fs
+			.access(oldPath)
+			.then(() => true)
+			.catch(() => false)
+		expect(stillExists).toBe(true)
+	})
+
+	it("returns [] when file contains valid JSON that is not an array", async () => {
+		const taskId = "task-non-array-api"
+		const taskDir = path.join(tmpBaseDir, "tasks", taskId)
+		await fs.mkdir(taskDir, { recursive: true })
+		const filePath = path.join(taskDir, "api_conversation_history.json")
+		await fs.writeFile(filePath, JSON.stringify("hello"), "utf8")
+
+		const result = await readApiMessages({
+			taskId,
+			globalStoragePath: tmpBaseDir,
+		})
+
+		expect(result).toEqual([])
+	})
+
+	it("returns [] when fallback file contains valid JSON that is not an array", async () => {
+		const taskId = "task-non-array-fallback"
+		const taskDir = path.join(tmpBaseDir, "tasks", taskId)
+		await fs.mkdir(taskDir, { recursive: true })
+
+		// Only write the old fallback file, NOT the new one
+		const oldPath = path.join(taskDir, "claude_messages.json")
+		await fs.writeFile(oldPath, JSON.stringify({ key: "value" }), "utf8")
+
+		const result = await readApiMessages({
+			taskId,
+			globalStoragePath: tmpBaseDir,
+		})
+
+		expect(result).toEqual([])
+	})
+})

+ 34 - 1
src/core/task-persistence/__tests__/taskMessages.spec.ts

@@ -12,7 +12,7 @@ vi.mock("../../../utils/safeWriteJson", () => ({
 }))
 
 // Import after mocks
-import { saveTaskMessages } from "../taskMessages"
+import { saveTaskMessages, readTaskMessages } from "../taskMessages"
 
 let tmpBaseDir: string
 
@@ -66,3 +66,36 @@ describe("taskMessages.saveTaskMessages", () => {
 		expect(persisted).toEqual(messages)
 	})
 })
+
+describe("taskMessages.readTaskMessages", () => {
+	it("returns empty array when file contains invalid JSON", async () => {
+		const taskId = "task-corrupt-json"
+		// Manually create the task directory and write corrupted JSON
+		const taskDir = path.join(tmpBaseDir, "tasks", taskId)
+		await fs.mkdir(taskDir, { recursive: true })
+		const filePath = path.join(taskDir, "ui_messages.json")
+		await fs.writeFile(filePath, "{not valid json!!!", "utf8")
+
+		const result = await readTaskMessages({
+			taskId,
+			globalStoragePath: tmpBaseDir,
+		})
+
+		expect(result).toEqual([])
+	})
+
+	it("returns [] when file contains valid JSON that is not an array", async () => {
+		const taskId = "task-non-array-json"
+		const taskDir = path.join(tmpBaseDir, "tasks", taskId)
+		await fs.mkdir(taskDir, { recursive: true })
+		const filePath = path.join(taskDir, "ui_messages.json")
+		await fs.writeFile(filePath, JSON.stringify("hello"), "utf8")
+
+		const result = await readTaskMessages({
+			taskId,
+			globalStoragePath: tmpBaseDir,
+		})
+
+		expect(result).toEqual([])
+	})
+})

+ 21 - 9
src/core/task-persistence/apiMessages.ts

@@ -51,17 +51,23 @@ export async function readApiMessages({
 		const fileContent = await fs.readFile(filePath, "utf8")
 		try {
 			const parsedData = JSON.parse(fileContent)
-			if (Array.isArray(parsedData) && parsedData.length === 0) {
+			if (!Array.isArray(parsedData)) {
+				console.warn(
+					`[readApiMessages] Parsed data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${filePath}`,
+				)
+				return []
+			}
+			if (parsedData.length === 0) {
 				console.error(
 					`[Roo-Debug] readApiMessages: Found API conversation history file, but it's empty (parsed as []). TaskId: ${taskId}, Path: ${filePath}`,
 				)
 			}
 			return parsedData
 		} catch (error) {
-			console.error(
-				`[Roo-Debug] readApiMessages: Error parsing API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`,
+			console.warn(
+				`[readApiMessages] Error parsing API conversation history file, returning empty. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`,
 			)
-			throw error
+			return []
 		}
 	} else {
 		const oldPath = path.join(taskDir, "claude_messages.json")
@@ -70,7 +76,13 @@ export async function readApiMessages({
 			const fileContent = await fs.readFile(oldPath, "utf8")
 			try {
 				const parsedData = JSON.parse(fileContent)
-				if (Array.isArray(parsedData) && parsedData.length === 0) {
+				if (!Array.isArray(parsedData)) {
+					console.warn(
+						`[readApiMessages] Parsed OLD data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${oldPath}`,
+					)
+					return []
+				}
+				if (parsedData.length === 0) {
 					console.error(
 						`[Roo-Debug] readApiMessages: Found OLD API conversation history file (claude_messages.json), but it's empty (parsed as []). TaskId: ${taskId}, Path: ${oldPath}`,
 					)
@@ -78,11 +90,11 @@ export async function readApiMessages({
 				await fs.unlink(oldPath)
 				return parsedData
 			} catch (error) {
-				console.error(
-					`[Roo-Debug] readApiMessages: Error parsing OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`,
+				console.warn(
+					`[readApiMessages] Error parsing OLD API conversation history file (claude_messages.json), returning empty. TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`,
 				)
-				// DO NOT unlink oldPath if parsing failed, throw error instead.
-				throw error
+				// DO NOT unlink oldPath if parsing failed.
+				return []
 			}
 		}
 	}

+ 15 - 1
src/core/task-persistence/taskMessages.ts

@@ -23,7 +23,21 @@ export async function readTaskMessages({
 	const fileExists = await fileExistsAtPath(filePath)
 
 	if (fileExists) {
-		return JSON.parse(await fs.readFile(filePath, "utf8"))
+		try {
+			const parsedData = JSON.parse(await fs.readFile(filePath, "utf8"))
+			if (!Array.isArray(parsedData)) {
+				console.warn(
+					`[readTaskMessages] Parsed data is not an array (got ${typeof parsedData}), returning empty. TaskId: ${taskId}, Path: ${filePath}`,
+				)
+				return []
+			}
+			return parsedData
+		} catch (error) {
+			console.warn(
+				`[readTaskMessages] Failed to parse ${filePath} for task ${taskId}, returning empty: ${error instanceof Error ? error.message : String(error)}`,
+			)
+			return []
+		}
 	}
 
 	return []

+ 46 - 11
src/core/task/Task.ts

@@ -1203,10 +1203,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 	 * tools execute (added in recursivelyMakeClineRequests after streaming completes).
 	 * So we usually only need to flush the pending user message with tool_results.
 	 */
-	public async flushPendingToolResultsToHistory(): Promise<void> {
+	public async flushPendingToolResultsToHistory(): Promise<boolean> {
 		// Only flush if there's actually pending content to save
 		if (this.userMessageContent.length === 0) {
-			return
+			return true
 		}
 
 		// CRITICAL: Wait for the assistant message to be saved to API history first.
@@ -1236,7 +1236,7 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 
 		// If task was aborted while waiting, don't flush
 		if (this.abort) {
-			return
+			return false
 		}
 
 		// Save the user message with tool_result blocks
@@ -1253,25 +1253,58 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 		const userMessageWithTs = { ...validatedMessage, ts: Date.now() }
 		this.apiConversationHistory.push(userMessageWithTs as ApiMessage)
 
-		await this.saveApiConversationHistory()
+		const saved = await this.saveApiConversationHistory()
+
+		if (saved) {
+			// Clear the pending content since it's now saved
+			this.userMessageContent = []
+		} else {
+			console.warn(
+				`[Task#${this.taskId}] flushPendingToolResultsToHistory: save failed, retaining pending tool results in memory`,
+			)
+		}
 
-		// Clear the pending content since it's now saved
-		this.userMessageContent = []
+		return saved
 	}
 
-	private async saveApiConversationHistory() {
+	private async saveApiConversationHistory(): Promise<boolean> {
 		try {
 			await saveApiMessages({
-				messages: this.apiConversationHistory,
+				messages: structuredClone(this.apiConversationHistory),
 				taskId: this.taskId,
 				globalStoragePath: this.globalStoragePath,
 			})
+			return true
 		} catch (error) {
-			// In the off chance this fails, we don't want to stop the task.
 			console.error("Failed to save API conversation history:", error)
+			return false
 		}
 	}
 
+	/**
+	 * Public wrapper to retry saving the API conversation history.
+	 * Uses exponential backoff: up to 3 attempts with delays of 100 ms, 500 ms, 1500 ms.
+	 * Used by delegation flow when flushPendingToolResultsToHistory reports failure.
+	 */
+	public async retrySaveApiConversationHistory(): Promise<boolean> {
+		const delays = [100, 500, 1500]
+
+		for (let attempt = 0; attempt < delays.length; attempt++) {
+			await new Promise<void>((resolve) => setTimeout(resolve, delays[attempt]))
+			console.warn(
+				`[Task#${this.taskId}] retrySaveApiConversationHistory: retry attempt ${attempt + 1}/${delays.length}`,
+			)
+
+			const success = await this.saveApiConversationHistory()
+
+			if (success) {
+				return true
+			}
+		}
+
+		return false
+	}
+
 	// Cline Messages
 
 	private async getSavedClineMessages(): Promise<ClineMessage[]> {
@@ -1333,10 +1366,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 		}
 	}
 
-	private async saveClineMessages() {
+	private async saveClineMessages(): Promise<boolean> {
 		try {
 			await saveTaskMessages({
-				messages: this.clineMessages,
+				messages: structuredClone(this.clineMessages),
 				taskId: this.taskId,
 				globalStoragePath: this.globalStoragePath,
 			})
@@ -1366,8 +1399,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
 			this.debouncedEmitTokenUsage(tokenUsage, this.toolUsage)
 
 			await this.providerRef.deref()?.updateTaskHistory(historyItem)
+			return true
 		} catch (error) {
 			console.error("Failed to save Roo messages:", error)
+			return false
 		}
 	}
 

+ 471 - 0
src/core/task/__tests__/Task.persistence.spec.ts

@@ -0,0 +1,471 @@
+// cd src && npx vitest run core/task/__tests__/Task.persistence.spec.ts
+
+import * as os from "os"
+import * as path from "path"
+import * as vscode from "vscode"
+
+import type { GlobalState, ProviderSettings } from "@roo-code/types"
+import { TelemetryService } from "@roo-code/telemetry"
+
+import { Task } from "../Task"
+import { ClineProvider } from "../../webview/ClineProvider"
+import { ContextProxy } from "../../config/ContextProxy"
+
+// ─── Hoisted mocks ───────────────────────────────────────────────────────────
+
+const {
+	mockSaveApiMessages,
+	mockSaveTaskMessages,
+	mockReadApiMessages,
+	mockReadTaskMessages,
+	mockTaskMetadata,
+	mockPWaitFor,
+} = vi.hoisted(() => ({
+	mockSaveApiMessages: vi.fn().mockResolvedValue(undefined),
+	mockSaveTaskMessages: vi.fn().mockResolvedValue(undefined),
+	mockReadApiMessages: vi.fn().mockResolvedValue([]),
+	mockReadTaskMessages: vi.fn().mockResolvedValue([]),
+	mockTaskMetadata: vi.fn().mockResolvedValue({
+		historyItem: { id: "test-id", ts: Date.now(), task: "test" },
+		tokenUsage: {
+			totalTokensIn: 0,
+			totalTokensOut: 0,
+			totalCacheWrites: 0,
+			totalCacheReads: 0,
+			totalCost: 0,
+			contextTokens: 0,
+		},
+	}),
+	mockPWaitFor: vi.fn().mockResolvedValue(undefined),
+}))
+
+// ─── Module mocks ────────────────────────────────────────────────────────────
+
+vi.mock("delay", () => ({
+	__esModule: true,
+	default: vi.fn().mockResolvedValue(undefined),
+}))
+
+vi.mock("execa", () => ({
+	execa: vi.fn(),
+}))
+
+vi.mock("fs/promises", async (importOriginal) => {
+	const actual = (await importOriginal()) as Record<string, any>
+	return {
+		...actual,
+		mkdir: vi.fn().mockResolvedValue(undefined),
+		writeFile: vi.fn().mockResolvedValue(undefined),
+		readFile: vi.fn().mockResolvedValue("[]"),
+		unlink: vi.fn().mockResolvedValue(undefined),
+		rmdir: vi.fn().mockResolvedValue(undefined),
+		default: {
+			mkdir: vi.fn().mockResolvedValue(undefined),
+			writeFile: vi.fn().mockResolvedValue(undefined),
+			readFile: vi.fn().mockResolvedValue("[]"),
+			unlink: vi.fn().mockResolvedValue(undefined),
+			rmdir: vi.fn().mockResolvedValue(undefined),
+		},
+	}
+})
+
+vi.mock("p-wait-for", () => ({
+	default: mockPWaitFor,
+}))
+
+vi.mock("../../task-persistence", () => ({
+	saveApiMessages: mockSaveApiMessages,
+	saveTaskMessages: mockSaveTaskMessages,
+	readApiMessages: mockReadApiMessages,
+	readTaskMessages: mockReadTaskMessages,
+	taskMetadata: mockTaskMetadata,
+}))
+
+vi.mock("vscode", () => {
+	const mockDisposable = { dispose: vi.fn() }
+	const mockEventEmitter = { event: vi.fn(), fire: vi.fn() }
+	const mockTextDocument = { uri: { fsPath: "/mock/workspace/path/file.ts" } }
+	const mockTextEditor = { document: mockTextDocument }
+	const mockTab = { input: { uri: { fsPath: "/mock/workspace/path/file.ts" } } }
+	const mockTabGroup = { tabs: [mockTab] }
+
+	return {
+		TabInputTextDiff: vi.fn(),
+		CodeActionKind: {
+			QuickFix: { value: "quickfix" },
+			RefactorRewrite: { value: "refactor.rewrite" },
+		},
+		window: {
+			createTextEditorDecorationType: vi.fn().mockReturnValue({ dispose: vi.fn() }),
+			visibleTextEditors: [mockTextEditor],
+			tabGroups: {
+				all: [mockTabGroup],
+				close: vi.fn(),
+				onDidChangeTabs: vi.fn(() => ({ dispose: vi.fn() })),
+			},
+			showErrorMessage: vi.fn(),
+		},
+		workspace: {
+			workspaceFolders: [
+				{
+					uri: { fsPath: "/mock/workspace/path" },
+					name: "mock-workspace",
+					index: 0,
+				},
+			],
+			createFileSystemWatcher: vi.fn(() => ({
+				onDidCreate: vi.fn(() => mockDisposable),
+				onDidDelete: vi.fn(() => mockDisposable),
+				onDidChange: vi.fn(() => mockDisposable),
+				dispose: vi.fn(),
+			})),
+			fs: {
+				stat: vi.fn().mockResolvedValue({ type: 1 }),
+			},
+			onDidSaveTextDocument: vi.fn(() => mockDisposable),
+			getConfiguration: vi.fn(() => ({ get: (_key: string, defaultValue: unknown) => defaultValue })),
+		},
+		env: {
+			uriScheme: "vscode",
+			language: "en",
+		},
+		EventEmitter: vi.fn().mockImplementation(() => mockEventEmitter),
+		Disposable: {
+			from: vi.fn(),
+		},
+		TabInputText: vi.fn(),
+	}
+})
+
+vi.mock("../../mentions", () => ({
+	parseMentions: vi.fn().mockImplementation((text) => {
+		return Promise.resolve({ text: `processed: ${text}`, mode: undefined, contentBlocks: [] })
+	}),
+	openMention: vi.fn(),
+	getLatestTerminalOutput: vi.fn(),
+}))
+
+vi.mock("../../../integrations/misc/extract-text", () => ({
+	extractTextFromFile: vi.fn().mockResolvedValue("Mock file content"),
+}))
+
+vi.mock("../../environment/getEnvironmentDetails", () => ({
+	getEnvironmentDetails: vi.fn().mockResolvedValue(""),
+}))
+
+vi.mock("../../ignore/RooIgnoreController")
+
+vi.mock("../../condense", async (importOriginal) => {
+	const actual = (await importOriginal()) as Record<string, unknown>
+	return {
+		...actual,
+		summarizeConversation: vi.fn().mockResolvedValue({
+			messages: [{ role: "user", content: [{ type: "text", text: "continued" }], ts: Date.now() }],
+			summary: "summary",
+			cost: 0,
+			newContextTokens: 1,
+		}),
+	}
+})
+
+vi.mock("../../../utils/storage", () => ({
+	getTaskDirectoryPath: vi
+		.fn()
+		.mockImplementation((globalStoragePath, taskId) => Promise.resolve(`${globalStoragePath}/tasks/${taskId}`)),
+	getSettingsDirectoryPath: vi
+		.fn()
+		.mockImplementation((globalStoragePath) => Promise.resolve(`${globalStoragePath}/settings`)),
+}))
+
+vi.mock("../../../utils/fs", () => ({
+	fileExistsAtPath: vi.fn().mockReturnValue(false),
+}))
+
+// ─── Test suite ──────────────────────────────────────────────────────────────
+
+describe("Task persistence", () => {
+	let mockProvider: ClineProvider & Record<string, any>
+	let mockApiConfig: ProviderSettings
+	let mockOutputChannel: vscode.OutputChannel
+	let mockExtensionContext: vscode.ExtensionContext
+
+	beforeEach(() => {
+		vi.clearAllMocks()
+
+		if (!TelemetryService.hasInstance()) {
+			TelemetryService.createInstance([])
+		}
+
+		const storageUri = { fsPath: path.join(os.tmpdir(), "test-storage") }
+
+		mockExtensionContext = {
+			globalState: {
+				get: vi.fn().mockImplementation((_key: keyof GlobalState) => undefined),
+				update: vi.fn().mockImplementation((_key, _value) => Promise.resolve()),
+				keys: vi.fn().mockReturnValue([]),
+			},
+			globalStorageUri: storageUri,
+			workspaceState: {
+				get: vi.fn().mockImplementation((_key) => undefined),
+				update: vi.fn().mockImplementation((_key, _value) => Promise.resolve()),
+				keys: vi.fn().mockReturnValue([]),
+			},
+			secrets: {
+				get: vi.fn().mockImplementation((_key) => Promise.resolve(undefined)),
+				store: vi.fn().mockImplementation((_key, _value) => Promise.resolve()),
+				delete: vi.fn().mockImplementation((_key) => Promise.resolve()),
+			},
+			extensionUri: { fsPath: "/mock/extension/path" },
+			extension: { packageJSON: { version: "1.0.0" } },
+		} as unknown as vscode.ExtensionContext
+
+		mockOutputChannel = {
+			appendLine: vi.fn(),
+			append: vi.fn(),
+			clear: vi.fn(),
+			show: vi.fn(),
+			hide: vi.fn(),
+			dispose: vi.fn(),
+		} as unknown as vscode.OutputChannel
+
+		mockProvider = new ClineProvider(
+			mockExtensionContext,
+			mockOutputChannel,
+			"sidebar",
+			new ContextProxy(mockExtensionContext),
+		) as ClineProvider & Record<string, any>
+
+		mockApiConfig = {
+			apiProvider: "anthropic",
+			apiModelId: "claude-3-5-sonnet-20241022",
+			apiKey: "test-api-key",
+		}
+
+		mockProvider.postMessageToWebview = vi.fn().mockResolvedValue(undefined)
+		mockProvider.postStateToWebview = vi.fn().mockResolvedValue(undefined)
+		mockProvider.postStateToWebviewWithoutTaskHistory = vi.fn().mockResolvedValue(undefined)
+		mockProvider.updateTaskHistory = vi.fn().mockResolvedValue(undefined)
+	})
+
+	// ── saveApiConversationHistory (via retrySaveApiConversationHistory) ──
+
+	describe("saveApiConversationHistory", () => {
+		it("returns true on success", async () => {
+			mockSaveApiMessages.mockResolvedValueOnce(undefined)
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			task.apiConversationHistory.push({
+				role: "user",
+				content: [{ type: "text", text: "hello" }],
+			})
+
+			const result = await task.retrySaveApiConversationHistory()
+			expect(result).toBe(true)
+		})
+
+		it("returns false on failure", async () => {
+			vi.useFakeTimers()
+
+			// All 3 retry attempts must fail for retrySaveApiConversationHistory to return false
+			mockSaveApiMessages
+				.mockRejectedValueOnce(new Error("fail 1"))
+				.mockRejectedValueOnce(new Error("fail 2"))
+				.mockRejectedValueOnce(new Error("fail 3"))
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			const promise = task.retrySaveApiConversationHistory()
+			await vi.runAllTimersAsync()
+			const result = await promise
+
+			expect(result).toBe(false)
+			expect(mockSaveApiMessages).toHaveBeenCalledTimes(3)
+
+			vi.useRealTimers()
+		})
+
+		it("succeeds on 2nd retry attempt", async () => {
+			vi.useFakeTimers()
+
+			mockSaveApiMessages.mockRejectedValueOnce(new Error("fail 1")).mockResolvedValueOnce(undefined) // succeeds on 2nd try
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			const promise = task.retrySaveApiConversationHistory()
+			await vi.runAllTimersAsync()
+			const result = await promise
+
+			expect(result).toBe(true)
+			expect(mockSaveApiMessages).toHaveBeenCalledTimes(2)
+
+			vi.useRealTimers()
+		})
+
+		it("snapshots the array before passing to saveApiMessages", async () => {
+			mockSaveApiMessages.mockResolvedValueOnce(undefined)
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			const originalMsg = {
+				role: "user" as const,
+				content: [{ type: "text" as const, text: "snapshot test" }],
+			}
+			task.apiConversationHistory.push(originalMsg)
+
+			await task.retrySaveApiConversationHistory()
+
+			expect(mockSaveApiMessages).toHaveBeenCalledTimes(1)
+
+			const callArgs = mockSaveApiMessages.mock.calls[0][0]
+			// The messages passed should be a COPY, not the live reference
+			expect(callArgs.messages).not.toBe(task.apiConversationHistory)
+			// But the content should be the same
+			expect(callArgs.messages).toEqual(task.apiConversationHistory)
+		})
+	})
+
+	// ── saveClineMessages ────────────────────────────────────────────────
+
+	describe("saveClineMessages", () => {
+		it("returns true on success", async () => {
+			mockSaveTaskMessages.mockResolvedValueOnce(undefined)
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			const result = await (task as Record<string, any>).saveClineMessages()
+			expect(result).toBe(true)
+		})
+
+		it("returns false on failure", async () => {
+			mockSaveTaskMessages.mockRejectedValueOnce(new Error("write error"))
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			const result = await (task as Record<string, any>).saveClineMessages()
+			expect(result).toBe(false)
+		})
+
+		it("snapshots the array before passing to saveTaskMessages", async () => {
+			mockSaveTaskMessages.mockResolvedValueOnce(undefined)
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			task.clineMessages.push({
+				type: "say",
+				say: "text",
+				text: "snapshot test",
+				ts: Date.now(),
+			})
+
+			await (task as Record<string, any>).saveClineMessages()
+
+			expect(mockSaveTaskMessages).toHaveBeenCalledTimes(1)
+
+			const callArgs = mockSaveTaskMessages.mock.calls[0][0]
+			// The messages passed should be a COPY, not the live reference
+			expect(callArgs.messages).not.toBe(task.clineMessages)
+			// But the content should be the same
+			expect(callArgs.messages).toEqual(task.clineMessages)
+		})
+	})
+
+	// ── flushPendingToolResultsToHistory — save failure/success ───────────
+
+	describe("flushPendingToolResultsToHistory persistence", () => {
+		it("retains userMessageContent on save failure", async () => {
+			mockSaveApiMessages.mockRejectedValueOnce(new Error("disk full"))
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			// Skip waiting for assistant message
+			task.assistantMessageSavedToHistory = true
+
+			task.userMessageContent = [
+				{
+					type: "tool_result",
+					tool_use_id: "tool-fail",
+					content: "Result that should be retained",
+				},
+			]
+
+			const saved = await task.flushPendingToolResultsToHistory()
+
+			expect(saved).toBe(false)
+			// userMessageContent should NOT be cleared on failure
+			expect(task.userMessageContent.length).toBeGreaterThan(0)
+			expect(task.userMessageContent[0]).toMatchObject({
+				type: "tool_result",
+				tool_use_id: "tool-fail",
+			})
+		})
+
+		it("clears userMessageContent on save success", async () => {
+			mockSaveApiMessages.mockResolvedValueOnce(undefined)
+
+			const task = new Task({
+				provider: mockProvider,
+				apiConfiguration: mockApiConfig,
+				task: "test task",
+				startTask: false,
+			})
+
+			// Skip waiting for assistant message
+			task.assistantMessageSavedToHistory = true
+
+			task.userMessageContent = [
+				{
+					type: "tool_result",
+					tool_use_id: "tool-ok",
+					content: "Result that should be cleared",
+				},
+			]
+
+			const saved = await task.flushPendingToolResultsToHistory()
+
+			expect(saved).toBe(true)
+			// userMessageContent should be cleared on success
+			expect(task.userMessageContent).toEqual([])
+		})
+	})
+})

+ 4 - 0
src/core/task/__tests__/flushPendingToolResultsToHistory.spec.ts

@@ -21,6 +21,10 @@ vi.mock("execa", () => ({
 	execa: vi.fn(),
 }))
 
+vi.mock("../../../utils/safeWriteJson", () => ({
+	safeWriteJson: vi.fn().mockResolvedValue(undefined),
+}))
+
 vi.mock("fs/promises", async (importOriginal) => {
 	const actual = (await importOriginal()) as Record<string, any>
 	const mockFunctions = {

+ 46 - 23
src/core/webview/ClineProvider.ts

@@ -1665,31 +1665,40 @@ export class ClineProvider
 		const history = this.getGlobalState("taskHistory") ?? []
 		const historyItem = history.find((item) => item.id === id)
 
-		if (historyItem) {
-			const { getTaskDirectoryPath } = await import("../../utils/storage")
-			const globalStoragePath = this.contextProxy.globalStorageUri.fsPath
-			const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id)
-			const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory)
-			const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages)
-			const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath)
-
-			if (fileExists) {
-				const apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8"))
-
-				return {
-					historyItem,
-					taskDirPath,
-					apiConversationHistoryFilePath,
-					uiMessagesFilePath,
-					apiConversationHistory,
-				}
+		if (!historyItem) {
+			throw new Error("Task not found")
+		}
+
+		const { getTaskDirectoryPath } = await import("../../utils/storage")
+		const globalStoragePath = this.contextProxy.globalStorageUri.fsPath
+		const taskDirPath = await getTaskDirectoryPath(globalStoragePath, id)
+		const apiConversationHistoryFilePath = path.join(taskDirPath, GlobalFileNames.apiConversationHistory)
+		const uiMessagesFilePath = path.join(taskDirPath, GlobalFileNames.uiMessages)
+		const fileExists = await fileExistsAtPath(apiConversationHistoryFilePath)
+
+		let apiConversationHistory: Anthropic.MessageParam[] = []
+
+		if (fileExists) {
+			try {
+				apiConversationHistory = JSON.parse(await fs.readFile(apiConversationHistoryFilePath, "utf8"))
+			} catch (error) {
+				console.warn(
+					`[getTaskWithId] api_conversation_history.json corrupted for task ${id}, returning empty history: ${error instanceof Error ? error.message : String(error)}`,
+				)
 			}
+		} else {
+			console.warn(
+				`[getTaskWithId] api_conversation_history.json missing for task ${id}, returning empty history`,
+			)
 		}
 
-		// if we tried to get a task that doesn't exist, remove it from state
-		// FIXME: this seems to happen sometimes when the json file doesnt save to disk for some reason
-		await this.deleteTaskFromState(id)
-		throw new Error("Task not found")
+		return {
+			historyItem,
+			taskDirPath,
+			apiConversationHistoryFilePath,
+			uiMessagesFilePath,
+			apiConversationHistory,
+		}
 	}
 
 	async getTaskWithAggregatedCosts(taskId: string): Promise<{
@@ -3182,7 +3191,21 @@ export class ClineProvider
 		//    recursivelyMakeClineRequests BEFORE tools start executing. We only need to
 		//    flush the pending user message with tool_results.
 		try {
-			await parent.flushPendingToolResultsToHistory()
+			const flushSuccess = await parent.flushPendingToolResultsToHistory()
+
+			if (!flushSuccess) {
+				console.warn(`[delegateParentAndOpenChild] Flush failed for parent ${parentTaskId}, retrying...`)
+				const retrySuccess = await parent.retrySaveApiConversationHistory()
+
+				if (!retrySuccess) {
+					console.error(
+						`[delegateParentAndOpenChild] CRITICAL: Parent ${parentTaskId} API history not persisted to disk. Child return may produce stale state.`,
+					)
+					vscode.window.showWarningMessage(
+						"Warning: Parent task state could not be saved. The parent task may lose recent context when resumed.",
+					)
+				}
+			}
 		} catch (error) {
 			this.log(
 				`[delegateParentAndOpenChild] Error flushing pending tool results (non-fatal): ${

+ 49 - 0
src/core/webview/__tests__/ClineProvider.spec.ts

@@ -3770,4 +3770,53 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
 			})
 		})
 	})
+
+	describe("getTaskWithId", () => {
+		it("returns empty apiConversationHistory when file is missing", async () => {
+			const historyItem = { id: "missing-api-file-task", task: "test task", ts: Date.now() }
+			vi.mocked(mockContext.globalState.get).mockImplementation((key: string) => {
+				if (key === "taskHistory") {
+					return [historyItem]
+				}
+				return undefined
+			})
+
+			const deleteTaskSpy = vi.spyOn(provider, "deleteTaskFromState")
+
+			const result = await (provider as any).getTaskWithId("missing-api-file-task")
+
+			expect(result.historyItem).toEqual(historyItem)
+			expect(result.apiConversationHistory).toEqual([])
+			expect(deleteTaskSpy).not.toHaveBeenCalled()
+		})
+
+		it("returns empty apiConversationHistory when file contains invalid JSON", async () => {
+			const historyItem = { id: "corrupt-api-task", task: "test task", ts: Date.now() }
+			vi.mocked(mockContext.globalState.get).mockImplementation((key: string) => {
+				if (key === "taskHistory") {
+					return [historyItem]
+				}
+				return undefined
+			})
+
+			// Make fileExistsAtPath return true so the read path is exercised
+			const fsUtils = await import("../../../utils/fs")
+			vi.spyOn(fsUtils, "fileExistsAtPath").mockResolvedValue(true)
+
+			// Make readFile return corrupted JSON
+			const fsp = await import("fs/promises")
+			vi.mocked(fsp.readFile).mockResolvedValueOnce("{not valid json!!!" as never)
+
+			const deleteTaskSpy = vi.spyOn(provider, "deleteTaskFromState")
+
+			const result = await (provider as any).getTaskWithId("corrupt-api-task")
+
+			expect(result.historyItem).toEqual(historyItem)
+			expect(result.apiConversationHistory).toEqual([])
+			expect(deleteTaskSpy).not.toHaveBeenCalled()
+
+			// Restore the spy
+			vi.mocked(fsUtils.fileExistsAtPath).mockRestore()
+		})
+	})
 })