[Fix] Completed task fails to resume after restart when a message write is interrupted

.github/workflows/e2e.yml

@@ -156,6 +156,18 @@ jobs:
               if: github.event_name != 'pull_request' || steps.e2e-marker.outputs.cache-hit != 'true'
               run: TEST_FILE=restart-persistence.test USE_MOCK=true xvfb-run -a pnpm --filter @roo-code/vscode-e2e exec node ./out/runTest.js
 
+            - name: Upload restart-persistence failure artifacts
+              # runTest.js preserves a failed scenario workspace (phase results,
+              # VS Code logs, task files) under apps/vscode-e2e/test-failures/
+              # instead of deleting it, so the interrupted-write state from
+              # issue #2130-style failures stays inspectable.
+              if: failure()
+              uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
+              with:
+                  name: restart-persistence-failure
+                  path: apps/vscode-e2e/test-failures/
+                  if-no-files-found: ignore
+
             - name: Explain skipped mocked E2E pass marker
               if: steps.e2e-marker.outputs.cache-hit != 'true' && steps.run-e2e.outcome == 'success' && steps.vscode-fallback.outputs.used == 'true'
               run: echo "Skipping mocked E2E pass marker because tests ran against a stale cached VS Code binary (VS Code download endpoints unreachable)."

.gitignore

@@ -27,6 +27,7 @@ AGENTS.local.md
 .vscode-test/
 apps/vscode-e2e/playwright-report-electron/
 apps/vscode-e2e/test-results-electron/
+apps/vscode-e2e/test-failures/
 
 # Docs
 docs/_site/

apps/vscode-e2e/src/restart/scenarioWorkspace.ts

@@ -56,3 +56,56 @@ export async function removeScenarioWorkspace(scenarioWorkspace: ScenarioWorkspa
 
 	await fs.rm(root, { recursive: true, force: true })
 }
+
+const FAILURE_ARTIFACTS_DIR = "test-failures"
+
+/**
+ * Copies the diagnosable parts of a failed scenario workspace into a stable
+ * directory inside the checkout, then removes the temporary scenario root.
+ * CI uploads that directory as a run artifact, so the failure that used to be
+ * deleted in the cleanup path stays inspectable: phase results, VS Code logs,
+ * and the extension's global storage with the task files.
+ *
+ * The directory is keyed by the unique scenario root name and cleared before
+ * copying, so retried runs cannot mix artifacts from different failures.
+ */
+export async function preserveScenarioWorkspace(scenarioWorkspace: ScenarioWorkspace): Promise<string> {
+	const destinationRoot = path.resolve(
+		__dirname,
+		"../../",
+		FAILURE_ARTIFACTS_DIR,
+		path.basename(scenarioWorkspace.root),
+	)
+	await fs.rm(destinationRoot, { recursive: true, force: true })
+	await fs.mkdir(destinationRoot, { recursive: true })
+
+	const selections: Array<{ label: string; source: string; destination: string }> = [
+		{
+			label: "phase results",
+			source: scenarioWorkspace.results,
+			destination: path.join(destinationRoot, "phase-results"),
+		},
+		{
+			label: "VS Code logs",
+			source: path.join(scenarioWorkspace.userData, "logs"),
+			destination: path.join(destinationRoot, "vscode-logs"),
+		},
+		{
+			label: "task files",
+			source: path.join(scenarioWorkspace.userData, "User", "globalStorage"),
+			destination: path.join(destinationRoot, "global-storage"),
+		},
+	]
+
+	for (const selection of selections) {
+		try {
+			await fs.cp(selection.source, selection.destination, { recursive: true, force: true })
+		} catch (error) {
+			// A missing subdirectory must not hide the other artifacts.
+			console.error(`Failed to preserve ${selection.label} from ${selection.source}:`, error)
+		}
+	}
+
+	await removeScenarioWorkspace(scenarioWorkspace)
+	return destinationRoot
+}

apps/vscode-e2e/src/runTest.ts

@@ -21,7 +21,11 @@ import { addSearchFilesResultFixtures } from "./fixtures/search-files"
 import { addSubtaskFixtures } from "./fixtures/subtasks"
 import { addUseMcpToolResultFixtures } from "./fixtures/use-mcp-tool"
 import { addWriteToFileResultFixtures } from "./fixtures/write-to-file"
-import { createScenarioWorkspace, removeScenarioWorkspace } from "./restart/scenarioWorkspace"
+import {
+	createScenarioWorkspace,
+	preserveScenarioWorkspace,
+	removeScenarioWorkspace,
+} from "./restart/scenarioWorkspace"
 import { runRestartScenario } from "./restart/vscodeCoordinator"
 
 function getCliFlagValue(flag: string) {
@@ -95,6 +99,7 @@ async function main() {
 
 	let testWorkspace: string | undefined
 	let scenarioWorkspace: Awaited<ReturnType<typeof createScenarioWorkspace>> | undefined
+	let runFailed = false
 
 	try {
 		// Create a temporary workspace folder for regular tests. Restart scenarios own
@@ -218,14 +223,27 @@ async function main() {
 			})
 		}
 	} catch (error) {
+		runFailed = true
 		console.error("Failed to run tests", error)
 		process.exitCode = 1
 	} finally {
 		if (testWorkspace) {
 			await fs.rm(testWorkspace, { recursive: true, force: true })
 		}
 		if (scenarioWorkspace) {
-			await removeScenarioWorkspace(scenarioWorkspace)
+			// A failed restart scenario must stay inspectable: the phase
+			// results, VS Code logs, and task files are copied to a stable
+			// directory for CI artifact upload before the temp root goes away.
+			if (runFailed) {
+				try {
+					const preservedAt = await preserveScenarioWorkspace(scenarioWorkspace)
+					console.error(`Restart scenario workspace preserved for failure inspection: ${preservedAt}`)
+				} catch (preserveError) {
+					console.error("Failed to preserve the restart scenario workspace:", preserveError)
+				}
+			} else {
+				await removeScenarioWorkspace(scenarioWorkspace)
+			}
 		}
 		await mock?.stop()
 	}

src/core/task-persistence/__tests__/persistenceTrace.spec.ts

@@ -0,0 +1,59 @@
+import { describe, it, expect, vi, afterEach } from "vitest"
+
+import { setPersistenceTraceSink, tracePersistenceEvent } from "../persistenceTrace"
+
+function parseTraceLine(line: string): Record<string, unknown> {
+	return JSON.parse(line.replace("[persistence-trace] ", "")) as Record<string, unknown>
+}
+
+describe("persistenceTrace", () => {
+	afterEach(() => {
+		setPersistenceTraceSink(undefined)
+		vi.restoreAllMocks()
+	})
+
+	it("emits a JSON line with the timestamp, process ID, stage, and fields to the sink", () => {
+		const lines: string[] = []
+		setPersistenceTraceSink((line) => lines.push(line))
+
+		tracePersistenceEvent("ui-messages-write-start", { taskId: "task-1", filePath: "/tmp/ui_messages.json" })
+
+		expect(lines).toHaveLength(1)
+		const event = parseTraceLine(lines[0])
+		expect(event.stage).toBe("ui-messages-write-start")
+		expect(event.taskId).toBe("task-1")
+		expect(event.pid).toBe(process.pid)
+		expect(typeof event.ts).toBe("string")
+		expect(event.filePath).toBe("/tmp/ui_messages.json")
+	})
+
+	it("marks the marker-check result", () => {
+		const lines: string[] = []
+		setPersistenceTraceSink((line) => lines.push(line))
+
+		tracePersistenceEvent("ui-messages-marker-check", { taskId: "task-2", result: "match" })
+
+		const event = parseTraceLine(lines[0])
+		expect(event.stage).toBe("ui-messages-marker-check")
+		expect(event.result).toBe("match")
+	})
+
+	it("falls back to the console when no sink is installed", () => {
+		const consoleSpy = vi.spyOn(console, "log").mockImplementation(() => {})
+
+		tracePersistenceEvent("shutdown")
+
+		expect(consoleSpy).toHaveBeenCalledTimes(1)
+		const line = String(consoleSpy.mock.calls[0]?.[0])
+		expect(line).toContain("[persistence-trace]")
+		expect(parseTraceLine(line).stage).toBe("shutdown")
+	})
+
+	it("never lets a failing sink break the caller", () => {
+		setPersistenceTraceSink(() => {
+			throw new Error("sink failed")
+		})
+
+		expect(() => tracePersistenceEvent("ui-messages-marker-check", { result: "match" })).not.toThrow()
+	})
+})

src/core/task-persistence/__tests__/taskMessages.spec.ts

@@ -21,6 +21,7 @@ vi.mock("../../../utils/safeWriteJson", () => ({
 // Import after mocks
 import { saveTaskMessages, readTaskMessages } from "../taskMessages"
 import { formatErrorDetail } from "../readFileWithMissingRetry"
+import { setPersistenceTraceSink } from "../persistenceTrace"
 
 let tmpBaseDir: string
 
@@ -35,6 +36,7 @@ beforeEach(async () => {
 afterEach(() => {
 	vi.useRealTimers()
 	vi.restoreAllMocks()
+	setPersistenceTraceSink(undefined)
 })
 
 describe("taskMessages.saveTaskMessages", () => {
@@ -63,7 +65,11 @@ describe("taskMessages.saveTaskMessages", () => {
 		expect(hoisted.safeWriteJsonMock).toHaveBeenCalledTimes(1)
 		const [, persisted] = hoisted.safeWriteJsonMock.mock.calls[0]
 		expect(persisted).toEqual(messages)
-		expect(hoisted.safeWriteJsonMock.mock.calls[0][2]).toBeUndefined()
+		const options = hoisted.safeWriteJsonMock.mock.calls[0][2] as { merge?: unknown; trace?: unknown } | undefined
+		expect(options?.merge).toBeUndefined()
+		// The trace callback is always installed so restart logs can follow
+		// the write stages.
+		expect(options?.trace).toBeTypeOf("function")
 	})
 
 	it("persists messages without modification when no metadata", async () => {
@@ -101,6 +107,39 @@ describe("taskMessages.saveTaskMessages", () => {
 		])
 	})
 
+	it("emits persistence trace events with the task ID for the write stages", async () => {
+		const traceLines: string[] = []
+		hoisted.safeWriteJsonMock.mockImplementationOnce(async (_path, _data, options) => {
+			for (const stage of ["recover", "write-start", "backup-rename", "replacement-rename", "write-complete"]) {
+				options.trace({ stage, detail: stage === "backup-rename" ? "eperm-fallback" : undefined })
+			}
+		})
+		setPersistenceTraceSink((line) => traceLines.push(line))
+
+		await saveTaskMessages({
+			messages: [{ ts: 1, type: "say", say: "text", text: "traced" }],
+			taskId: "task-trace",
+			globalStoragePath: tmpBaseDir,
+		})
+
+		const events = traceLines.map(
+			(line) => JSON.parse(line.replace("[persistence-trace] ", "")) as Record<string, unknown>,
+		)
+		expect(events.map((event) => event.stage)).toEqual([
+			"ui-messages-recovery",
+			"ui-messages-write-start",
+			"ui-messages-backup-rename",
+			"ui-messages-replacement-rename",
+			"ui-messages-write-complete",
+		])
+		for (const event of events) {
+			expect(event.taskId).toBe("task-trace")
+			expect(event.pid).toBe(process.pid)
+		}
+		const backupRename = events.find((event) => event.stage === "ui-messages-backup-rename")
+		expect(backupRename?.detail).toBe("eperm-fallback")
+	})
+
 	it("propagates a forced write failure without swallowing or replacing it", async () => {
 		// Fault injection: simulate an interrupted/failed final write (e.g. the
 		// extension host quitting mid-save or the disk refusing the write). The

src/core/task-persistence/persistenceTrace.ts

@@ -0,0 +1,60 @@
+/**
+ * Structured trace points for task-persistence file writes and shutdown.
+ *
+ * The restart-persistence investigation in issue #2130 could not tell which
+ * writer touched ui_messages.json or when, because nothing logged the
+ * replacement-protocol steps. These trace lines record the write stages, the
+ * marker check, and shutdown with the task ID and the process ID, so CI logs
+ * and extension logs can order writer activity across a restart.
+ *
+ * The default sink is the console. Extension activation installs a sink that
+ * also mirrors every line into the extension output channel; tests can
+ * install their own sink to capture events. Tracing must never break
+ * persistence, so sink failures are swallowed.
+ */
+
+export type PersistenceTraceStage =
+	| "ui-messages-recovery"
+	| "ui-messages-write-start"
+	| "ui-messages-backup-rename"
+	| "ui-messages-replacement-rename"
+	| "ui-messages-write-complete"
+	| "ui-messages-marker-check"
+	| "shutdown"
+
+export type PersistenceTraceFields = {
+	taskId?: string
+	filePath?: string
+	result?: string
+	detail?: string
+}
+
+type PersistenceTraceSink = (line: string) => void
+
+let traceSink: PersistenceTraceSink | undefined
+
+/**
+ * Installs the sink used for every subsequent trace event. Pass undefined to
+ * restore the default console sink.
+ */
+export function setPersistenceTraceSink(sink: PersistenceTraceSink | undefined): void {
+	traceSink = sink
+}
+
+export function tracePersistenceEvent(stage: PersistenceTraceStage, fields: PersistenceTraceFields = {}): void {
+	const line = `[persistence-trace] ${JSON.stringify({
+		ts: new Date().toISOString(),
+		pid: process.pid,
+		stage,
+		...fields,
+	})}`
+	try {
+		if (traceSink) {
+			traceSink(line)
+		} else {
+			console.log(line)
+		}
+	} catch {
+		// A failing sink must never break the write, read, or shutdown path.
+	}
+}

src/core/task-persistence/readFileWithMissingRetry.ts

@@ -1,8 +1,10 @@
 import * as fs from "fs/promises"
 
-// safeWriteJson replaces a file in two renames (target to backup, new file to
-// target) because a single rename over an open file fails with EPERM on
-// Windows. A reader can see ENOENT between the two renames, so retry once.
+// safeWriteJson commits a replacement with a single atomic rename. Only the
+// Windows EPERM fallback replaces a file in two renames (target to backup,
+// new file to target), because a single rename over an open file fails with
+// EPERM on Windows. A reader can see ENOENT between those two renames, so
+// retry once.
 const READ_RETRY_MIN_MS = 1
 const READ_RETRY_MAX_MS = 10
 const READ_RETRY_RANGE_MS = READ_RETRY_MAX_MS - READ_RETRY_MIN_MS + 1

src/core/task-persistence/taskMessages.ts

@@ -2,11 +2,20 @@ import * as path from "path"
 
 import type { ClineMessage } from "@roo-code/types"
 
-import { safeWriteJson } from "../../utils/safeWriteJson"
+import { safeWriteJson, type SafeWriteJsonTraceStage } from "../../utils/safeWriteJson"
 import { GlobalFileNames } from "../../shared/globalFileNames"
 import { getTaskDirectoryPath } from "../../utils/storage"
 import { ensureMessageIdentifiers, mergeClineMessageSnapshots } from "./mergeMessageSnapshots"
 import { formatErrorDetail, getErrorCode, readFileWithMissingRetry } from "./readFileWithMissingRetry"
+import { tracePersistenceEvent, type PersistenceTraceStage } from "./persistenceTrace"
+
+const uiMessagesTraceStages: Record<SafeWriteJsonTraceStage, PersistenceTraceStage> = {
+	recover: "ui-messages-recovery",
+	"write-start": "ui-messages-write-start",
+	"backup-rename": "ui-messages-backup-rename",
+	"replacement-rename": "ui-messages-replacement-rename",
+	"write-complete": "ui-messages-write-complete",
+}
 
 export type TaskMessagesReadErrorKind = "not_found" | "invalid" | "io_error"
 
@@ -83,17 +92,21 @@ export async function saveTaskMessages({
 	const taskDir = await getTaskDirectoryPath(globalStoragePath, taskId)
 	const filePath = path.join(taskDir, GlobalFileNames.uiMessages)
 	let savedMessages = messages
-	await safeWriteJson(
-		filePath,
-		messages,
-		merge
-			? {
-					merge: (existing, incoming) => {
-						savedMessages = mergeClineMessageSnapshots(existing, incoming) as ClineMessage[]
-						return savedMessages
-					},
-				}
-			: undefined,
-	)
+	await safeWriteJson(filePath, messages, {
+		...(merge && {
+			merge: (existing, incoming) => {
+				savedMessages = mergeClineMessageSnapshots(existing, incoming) as ClineMessage[]
+				return savedMessages
+			},
+		}),
+		// Trace the replacement-protocol stages with the task ID so restart
+		// logs can order UI-message writer activity across processes.
+		trace: (event) =>
+			tracePersistenceEvent(uiMessagesTraceStages[event.stage], {
+				taskId,
+				filePath,
+				detail: event.detail,
+			}),
+	})
 	return savedMessages
 }