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

.github/workflows/e2e.yml

@@ -162,6 +162,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

@@ -28,6 +28,7 @@ AGENTS.local.md
 apps/vscode-e2e/playwright-report-electron/
 apps/vscode-e2e/test-results-electron/
 apps/vscode-e2e/heap-results/
+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()
 	}

apps/vscode-e2e/src/suite/restart-persistence.test.ts

@@ -54,12 +54,13 @@ async function runCreate(api: RooCodeAPI): Promise<void> {
 		await waitUntilCompleted({ api, taskId })
 		assert.strictEqual(sawMarker, true, `Completion should include ${MARKER}`)
 
-		// TaskCompleted fires before the final saveClineMessages() write is
-		// necessarily durable on disk. Wait until the completion marker is
-		// persisted in ui_messages.json before quitting, so the verify phase
-		// never reads a task directory whose final write has not landed yet.
+		// TaskCompleted and the UI marker do not acknowledge all three files.
+		// Check both persisted histories and the store's published history item.
+		// TaskHistoryStore publishes newly created cache entries only after writing them.
 		const createdTaskId: string = taskId
 		await waitFor(() => api.hasTaskUiMessageText(createdTaskId, MARKER))
+		await waitForMarkedCompletion(api, createdTaskId)
+		await waitFor(async () => (await api.getTaskHistoryItem(createdTaskId)) !== undefined)
 
 		const result: PhaseResult = {
 			version: PHASE_RESULT_VERSION,

docs/architecture/file-replacement-restart-model.md

@@ -0,0 +1,78 @@
+# File replacement and restart model
+
+`scripts/check-file-replacement-restart.ts` is an independent bounded submodel
+under `pnpm lifecycle:model-check`. It preserves the completion-readiness model's
+separate scope. It checks three files, one initial publication per file, one later
+replacement of any one file, and one process crash at every publication or
+replacement boundary. Exploration runs to a fixed point with a 10,000-state cap.
+
+## Contract and production mapping
+
+| Model action                                   | Production boundary                                                                                                                                                                                                                                                                                                                |
+| ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `publish-ui`, `publish-api`, `publish-history` | Successful writes of `ui_messages.json`, `api_conversation_history.json`, and `history_item.json`                                                                                                                                                                                                                                  |
+| `acknowledge-all-three-files`                  | Explicit readiness conjunction in the restart E2E: `hasTaskUiMessageText`, `hasTaskApiConversationHistorySequence`, and `getTaskHistoryItem`. For a newly created task, `TaskHistoryStore.upsertCore` publishes its cache entry after `writeTaskFile` succeeds. This is a test readiness boundary, not a new public runtime event. |
+| `stage-replacement-*`                          | `safeWriteJson` writes and flushes a `.new_*.tmp` file under the advisory lock                                                                                                                                                                                                                                                     |
+| `atomic-commit`                                | Single temp-to-canonical rename                                                                                                                                                                                                                                                                                                    |
+| `eperm-backup-rename`, `fallback-commit`       | Windows-compatible EPERM fallback: canonical-to-backup, then temp-to-canonical                                                                                                                                                                                                                                                     |
+| `process-crash`                                | Process exits without rollback, cleanup, or lock release                                                                                                                                                                                                                                                                           |
+| `lock-becomes-stale`                           | `proper-lockfile` staleness check using the shared 31-second window                                                                                                                                                                                                                                                                |
+| `fresh-host-startup`, `locked-reader-recovery` | `TaskHistoryStore.reconcile` recovers a missing history record before stat/read; `readFileWithMissingRetry` invokes the same recovery after a second ENOENT for UI/API reads                                                                                                                                                       |
+| `recovery-io-failure`                          | Recovery scan, access, stat, or restore fails; rejection preserves committed backups for a later retry                                                                                                                                                                                                                             |
+| `explicit-delete-under-lock`                   | `deleteJsonFile` removes matching replacements/backups before the canonical file, serialized with writers and recovery; store deletion remains best effort                                                                                                                                                                         |
+
+The UI marker can become visible while history publication is still pending.
+`Task.saveClineMessages` persists UI messages before updating metadata, and logs
+metadata failures independently. Neither `TaskCompleted` nor UI-marker visibility
+promises that all three files exist. The model explicitly reaches that partial
+publication state and permits interruption there. It requires restart discovery
+only after the readiness conjunction, not after an unfinished first write.
+
+After acknowledgment, the latest committed version of each required file must
+remain in either the canonical file or its backup until explicit deletion.
+Completed fresh reads must return those committed versions and find the history
+record without a new write. A failed recovery must preserve every file version.
+Successful explicit deletion must leave no recoverable version to resurrect.
+The model requires reachable atomic and fallback successes, restart recovery,
+recovery after failure, partial publication, and reads after deletion.
+
+The checker first runs the old two-rename protocol without reader recovery and
+requires a #2130 counterexample. It then exhaustively checks the fixed protocol.
+This is an abstract protocol check, not execution of the production writer.
+
+## Refinement evidence
+
+`src/utils/__tests__/safeWriteJson.interruption.test.ts` runs the real bundled
+writer in a child process, terminates it with SIGKILL, and exercises both commit
+paths before and after the replacement rename for all three filenames. Initial
+publication uses the production writer. UI/API tests call production readers;
+history-record cases initialize a fresh `TaskHistoryStore` before any recovery
+write. One case leaves the crash lock's timestamp unchanged, proving the fresh
+store can wait for staleness instead of requiring the test to age the lock.
+
+`safeWriteJson.test.ts` injects directory scan, access, backup stat, and restore
+failures, checking backup preservation and retry. The store deletion tests cover
+single and batch deletion, leftover cleanup, unrelated-file preservation, and
+cleanup failure retaining the canonical copy. The existing two-extension-host
+restart E2E remains the production activation/rehydration boundary check.
+
+## Assumptions and limits
+
+This is process-crash safety on filesystems with atomic same-directory rename,
+not power-loss safety. Parent directories are not fsynced. Locks are advisory;
+all writers, recoverers, and history-file deleters must use the shared protocol.
+Live holders refresh their locks and must not be mistaken for crashed holders;
+filesystem clock behavior and lock compromise are outside the model. Recovery
+waits up to 35 lock retries at one-second intervals, then reports a lock failure.
+An indefinitely active writer or persistent filesystem fault can still prevent
+recovery; no liveness guarantee is claimed under such faults.
+
+Recovery is abstracted as a locked restore and cleanup; repeated crashes during
+recovery, multiple orphaned versions, external file removal, arbitrary overlapping
+writers, and deletion interrupted between individual unlinks are not exhaustively
+modeled. Focused tests cover newest-backup selection and filesystem failures.
+Deletion does not prohibit an explicit later creation of a task at the same path
+or a stale writer that deliberately publishes new data after deletion. The
+no-resurrection property concerns recovery from leftovers after successful
+deletion. Lifecycle status/delegation reducers and completion-event semantics
+are unchanged.

docs/architecture/task-lifecycle-model.md

@@ -206,6 +206,14 @@ CI runs `pnpm lifecycle:model-check` in `.github/workflows/code-qa.yml` after li
 
 ## Gap audit
 
+The independent [file replacement/restart model](./file-replacement-restart-model.md)
+checks the process-crash replacement protocol separately from completion readiness.
+It models three task files, initial publication, one replacement, one crash,
+locked recovery on fresh reads/startup, and successful explicit deletion. Its
+contract, production mapping, old-protocol counterexample, refinement tests, and
+filesystem/lock assumptions are documented there. A passing completion-readiness
+model alone does not establish this file-level contract.
+
 This section is a summary tracker. Issue links are historical provenance rather than specifications. Coverage status uses these evidence classes:
 
 The [Task lifecycle verification GAP report](./task-lifecycle-gap-report.md) is the authoritative register. It holds the exhaustive repository inventory, the ranked stable burn-down register, the source-based methodology, and the follow-up checklist. This page remains the executable model-suite specification.

package.json

@@ -20,7 +20,7 @@
 		"test:agent-evals:unit": "node --import tsx --test scripts/agent-evals/*.test.ts",
 		"agent-evals:check-types": "tsc -p scripts/agent-evals/tsconfig.json",
 		"test:mutation-ci": "node --test scripts/stryker-diff.test.mjs",
-		"lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-provider-handoff-scheduler.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check && tsx scripts/check-completion-persistence.ts && tsx scripts/check-delegated-mode-readers.ts && pnpm tool-turn:model-check",
+		"lifecycle:model-check": "tsx scripts/check-task-lifecycle.ts && tsx scripts/check-task-store-concurrency.ts && tsx scripts/check-provider-handoff-scheduler.ts && pnpm cleanup-protocol:model-check && pnpm parser-scope:model-check && tsx scripts/check-completion-persistence.ts && tsx scripts/check-file-replacement-restart.ts && tsx scripts/check-delegated-mode-readers.ts && pnpm tool-turn:model-check",
 		"webview-recovery:model-check": "node --import tsx scripts/check-webview-recovery.ts",
 		"webview-recovery:check-types": "tsc -p scripts/tsconfig.webview-recovery.json",
 		"fanout-protocol:model-check": "tsx scripts/check-task-fanout-protocol.ts",