From c2245b812277a63d71b408528bd49a627f82f7f6 Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Wed, 11 Feb 2026 16:11:52 +0100 Subject: [PATCH] fix(plan-files): use atomic writes to prevent 0-byte corruption (#1785) * fix(plan-files): use atomic writes to prevent 0-byte corruption writeFileSync truncates the file before writing content. If the process crashes between truncation and write, the file is left at 0 bytes, causing "Unexpected end of JSON input" errors on next load. Replace all bare writeFileSync calls for implementation_plan.json with atomic write-to-temp-then-rename pattern across plan-file-utils.ts and project-store.ts. Co-Authored-By: Claude Opus 4.6 * refactor: consolidate atomic write implementations into shared utility Add writeFileAtomicSync to atomic-file.ts and replace three duplicate implementations in plan-file-utils.ts, execution-handlers.ts, and project-store.ts. Also convert the bare writeFileSync in updateTaskMetadataPrUrl to use the atomic variant for consistency. Uses randomBytes for collision-safe temp file naming instead of process.pid. Co-Authored-By: Claude Opus 4.6 * fix: add path.resolve to writeFileAtomicSync and add test coverage Add path.resolve() for API consistency with the async writeFileAtomic variant. Add test suite covering: writing new files, overwriting, Buffer data, relative path resolution, temp file cleanup on success and error, and missing directory errors. Co-Authored-By: Claude Opus 4.6 * fix: improve writeFileAtomicSync tests and JSDoc Use readdirSync instead of async fsPromises.readdir in sync tests. Replace vacuous cleanup test with one that actually exercises the unlinkSync cleanup path by targeting a directory (rename fails after temp file creation). Add JSDoc note that sync variant does not create parent directories. Co-Authored-By: Claude Opus 4.6 * fix: use atomic writes for ProjectStore.save() and archive/unarchive Replace bare writeFileSync with writeFileAtomicSync in the save() method (highest-traffic write path) and in archiveTasks/unarchiveTasks for task_metadata.json writes. Remove unused writeFileSync import. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- .../ipc-handlers/task/execution-handlers.ts | 32 ++----- .../main/ipc-handlers/task/plan-file-utils.ts | 30 +++--- apps/frontend/src/main/project-store.ts | 12 ++- .../main/utils/__tests__/atomic-file.test.ts | 95 ++++++++++++++++++- apps/frontend/src/main/utils/atomic-file.ts | 33 ++++++- 5 files changed, 150 insertions(+), 52 deletions(-) diff --git a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts index 0879df2c..95bcb9d8 100644 --- a/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts @@ -2,7 +2,7 @@ import { ipcMain, BrowserWindow } from 'electron'; import { IPC_CHANNELS, AUTO_BUILD_PATHS, getSpecsDir } from '../../../shared/constants'; import type { IPCResult, TaskStartOptions, TaskStatus, ImageAttachment } from '../../../shared/types'; import path from 'path'; -import { existsSync, readFileSync, writeFileSync, renameSync, unlinkSync, mkdirSync } from 'fs'; +import { existsSync, readFileSync, writeFileSync, mkdirSync } from 'fs'; import { spawnSync, execFileSync } from 'child_process'; import { getToolPath } from '../../cli-tool-manager'; import { AgentManager } from '../../agent'; @@ -17,31 +17,11 @@ import { createPlanIfNotExists, resetStuckSubtasks } from './plan-file-utils'; +import { writeFileAtomicSync } from '../../utils/atomic-file'; import { findTaskWorktree } from '../../worktree-paths'; import { projectStore } from '../../project-store'; import { getIsolatedGitEnv, detectWorktreeBranch } from '../../utils/git-isolation'; -/** - * Atomic file write to prevent TOCTOU race conditions. - * Writes to a temporary file first, then atomically renames to target. - * This ensures the target file is never in an inconsistent state. - */ -function atomicWriteFileSync(filePath: string, content: string): void { - const tempPath = `${filePath}.${process.pid}.tmp`; - try { - writeFileSync(tempPath, content, 'utf-8'); - renameSync(tempPath, filePath); - } catch (error) { - // Clean up temp file if rename failed - try { - unlinkSync(tempPath); - } catch { - // Ignore cleanup errors - } - throw error; - } -} - /** * Safe file read that handles missing files without TOCTOU issues. * Returns null if file doesn't exist or can't be read. @@ -846,7 +826,7 @@ export function registerTaskExecutionHandlers( resumed_at: new Date().toISOString(), resumed_by: 'user' }); - atomicWriteFileSync(resumeFilePath, resumeContent); + writeFileAtomicSync(resumeFilePath, resumeContent); console.log(`[TASK_RESUME_PAUSED] Wrote RESUME file to: ${resumeFilePath}`); // Also write to worktree if it exists (backend may be running inside the worktree) @@ -854,7 +834,7 @@ export function registerTaskExecutionHandlers( if (worktreePath) { const worktreeResumeFilePath = path.join(worktreePath, specsBaseDir, task.specId, 'RESUME'); try { - atomicWriteFileSync(worktreeResumeFilePath, resumeContent); + writeFileAtomicSync(worktreeResumeFilePath, resumeContent); console.log(`[TASK_RESUME_PAUSED] Also wrote RESUME file to worktree: ${worktreeResumeFilePath}`); } catch (worktreeError) { // Non-fatal - main spec dir RESUME is sufficient @@ -1014,7 +994,7 @@ export function registerTaskExecutionHandlers( let writeSucceededForComplete = false; for (const pathToUpdate of planPathsToUpdate) { try { - atomicWriteFileSync(pathToUpdate, planContent); + writeFileAtomicSync(pathToUpdate, planContent); console.log(`[Recovery] Successfully wrote to: ${pathToUpdate}`); writeSucceededForComplete = true; } catch (writeError) { @@ -1150,7 +1130,7 @@ export function registerTaskExecutionHandlers( const restartPlanContent = JSON.stringify(plan, null, 2); for (const pathToUpdate of planPathsToUpdate) { try { - atomicWriteFileSync(pathToUpdate, restartPlanContent); + writeFileAtomicSync(pathToUpdate, restartPlanContent); console.log(`[Recovery] Wrote restart status to: ${pathToUpdate}`); } catch (writeError) { console.error(`[Recovery] Failed to write plan file for restart at ${pathToUpdate}:`, writeError); diff --git a/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts b/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts index 253553be..dfb08bf5 100644 --- a/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts +++ b/apps/frontend/src/main/ipc-handlers/task/plan-file-utils.ts @@ -18,11 +18,12 @@ */ import path from 'path'; -import { readFileSync, writeFileSync, mkdirSync, renameSync, unlinkSync } from 'fs'; +import { readFileSync, mkdirSync } from 'fs'; import { AUTO_BUILD_PATHS, getSpecsDir } from '../../../shared/constants'; import type { TaskStatus, Project, Task } from '../../../shared/types'; import { projectStore } from '../../project-store'; import type { TaskEventPayload } from '../../agent/task-event-schema'; +import { writeFileAtomicSync } from '../../utils/atomic-file'; // In-memory locks for plan file operations // Key: plan file path, Value: Promise chain for serializing operations @@ -112,7 +113,7 @@ export async function persistPlanStatus(planPath: string, status: TaskStatus, pr plan.planStatus = mapStatusToPlanStatus(status); plan.updated_at = new Date().toISOString(); - writeFileSync(planPath, JSON.stringify(plan, null, 2), 'utf-8'); + writeFileAtomicSync(planPath, JSON.stringify(plan, null, 2)); console.warn(`[plan-file-utils] Successfully persisted status: ${status} to implementation_plan.json`); // Invalidate tasks cache since status changed @@ -168,7 +169,7 @@ export function persistPlanStatusSync(planPath: string, status: TaskStatus, proj plan.planStatus = mapStatusToPlanStatus(status); plan.updated_at = new Date().toISOString(); - writeFileSync(planPath, JSON.stringify(plan, null, 2), 'utf-8'); + writeFileAtomicSync(planPath, JSON.stringify(plan, null, 2)); // Invalidate tasks cache since status changed if (projectId) { @@ -205,7 +206,7 @@ export function persistPlanLastEventSync(planPath: string, event: TaskEventPaylo }; plan.updated_at = new Date().toISOString(); - writeFileSync(planPath, JSON.stringify(plan, null, 2), 'utf-8'); + writeFileAtomicSync(planPath, JSON.stringify(plan, null, 2)); return true; } catch (err) { if (isFileNotFoundError(err)) { @@ -264,7 +265,7 @@ export function persistPlanStatusAndReasonSync( } plan.updated_at = new Date().toISOString(); - writeFileSync(planPath, JSON.stringify(plan, null, 2), 'utf-8'); + writeFileAtomicSync(planPath, JSON.stringify(plan, null, 2)); if (projectId) { projectStore.invalidateTasksCache(projectId); @@ -327,7 +328,7 @@ export function persistPlanPhaseSync( plan.updated_at = new Date().toISOString(); - writeFileSync(planPath, JSON.stringify(plan, null, 2), 'utf-8'); + writeFileAtomicSync(planPath, JSON.stringify(plan, null, 2)); if (projectId) { projectStore.invalidateTasksCache(projectId); @@ -362,7 +363,7 @@ export async function updatePlanFile>( // Add updated_at timestamp - use type assertion since T extends Record (updatedPlan as Record).updated_at = new Date().toISOString(); - writeFileSync(planPath, JSON.stringify(updatedPlan, null, 2), 'utf-8'); + writeFileAtomicSync(planPath, JSON.stringify(updatedPlan, null, 2)); console.warn(`[plan-file-utils] Successfully updated implementation_plan.json`); return updatedPlan; } catch (err) { @@ -429,7 +430,7 @@ export async function createPlanIfNotExists( } } - writeFileSync(planPath, JSON.stringify(plan, null, 2), 'utf-8'); + writeFileAtomicSync(planPath, JSON.stringify(plan, null, 2)); }); } @@ -476,16 +477,7 @@ export async function resetStuckSubtasks(planPath: string, projectId?: string): // Only write if we actually reset something if (resetCount > 0) { plan.updated_at = new Date().toISOString(); - const content = JSON.stringify(plan, null, 2); - // Atomic write: write to temp file then rename to prevent corruption on crash - const tempPath = `${planPath}.${process.pid}.tmp`; - try { - writeFileSync(tempPath, content, 'utf-8'); - renameSync(tempPath, planPath); - } catch (writeError) { - try { unlinkSync(tempPath); } catch { /* ignore cleanup */ } - throw writeError; - } + writeFileAtomicSync(planPath, JSON.stringify(plan, null, 2)); console.log(`[plan-file-utils] Successfully reset ${resetCount} stuck subtask(s) in implementation_plan.json`); // Invalidate tasks cache since subtask status changed @@ -539,7 +531,7 @@ export function updateTaskMetadataPrUrl(metadataPath: string, prUrl: string): bo mkdirSync(path.dirname(metadataPath), { recursive: true }); // Write back - writeFileSync(metadataPath, JSON.stringify(metadata, null, 2), 'utf-8'); + writeFileAtomicSync(metadataPath, JSON.stringify(metadata, null, 2)); return true; } catch (err) { console.warn(`[plan-file-utils] Could not update metadata at ${metadataPath}:`, err); diff --git a/apps/frontend/src/main/project-store.ts b/apps/frontend/src/main/project-store.ts index 09c73346..ca70b28c 100644 --- a/apps/frontend/src/main/project-store.ts +++ b/apps/frontend/src/main/project-store.ts @@ -1,5 +1,5 @@ import { app } from 'electron'; -import { readFileSync, writeFileSync, existsSync, mkdirSync, readdirSync, Dirent } from 'fs'; +import { readFileSync, existsSync, mkdirSync, readdirSync, Dirent } from 'fs'; import path from 'path'; import { v4 as uuidv4 } from 'uuid'; import type { Project, ProjectSettings, Task, TaskStatus, TaskMetadata, ImplementationPlan, ReviewReason, PlanSubtask, KanbanPreferences, ExecutionPhase } from '../shared/types'; @@ -8,6 +8,7 @@ import { getAutoBuildPath, isInitialized } from './project-initializer'; import { getTaskWorktreeDir } from './worktree-paths'; import { findAllSpecPaths } from './utils/spec-path-helpers'; import { ensureAbsolutePath } from './utils/path-helpers'; +import { writeFileAtomicSync } from './utils/atomic-file'; interface TabState { openProjectIds: string[]; @@ -78,7 +79,7 @@ export class ProjectStore { * Save store to disk */ private save(): void { - writeFileSync(this.storePath, JSON.stringify(this.data, null, 2), 'utf-8'); + writeFileAtomicSync(this.storePath, JSON.stringify(this.data, null, 2)); } /** @@ -630,7 +631,8 @@ export class ProjectStore { executionPhase: 'complete' }; try { - writeFileSync(planPath, JSON.stringify(correctedPlan, null, 2), 'utf-8'); + // Atomic write to prevent 0-byte corruption on crash + writeFileAtomicSync(planPath, JSON.stringify(correctedPlan, null, 2)); // Write succeeded — apply mutations to the in-memory plan so the rest of // loadTasksFromSpecsDir sees the corrected values (e.g., executionProgress) Object.assign(plan, correctedPlan); @@ -798,7 +800,7 @@ export class ProjectStore { metadata.archivedInVersion = version; } - writeFileSync(metadataPath, JSON.stringify(metadata, null, 2), 'utf-8'); + writeFileAtomicSync(metadataPath, JSON.stringify(metadata, null, 2)); } catch (error) { console.error(`[ProjectStore] archiveTasks: Failed to archive task ${taskId} at ${specPath}:`, error); hasErrors = true; @@ -856,7 +858,7 @@ export class ProjectStore { delete metadata.archivedAt; delete metadata.archivedInVersion; - writeFileSync(metadataPath, JSON.stringify(metadata, null, 2), 'utf-8'); + writeFileAtomicSync(metadataPath, JSON.stringify(metadata, null, 2)); } catch (error) { console.error(`[ProjectStore] unarchiveTasks: Failed to unarchive task ${taskId} at ${specPath}:`, error); hasErrors = true; diff --git a/apps/frontend/src/main/utils/__tests__/atomic-file.test.ts b/apps/frontend/src/main/utils/__tests__/atomic-file.test.ts index cae446a8..536532d7 100644 --- a/apps/frontend/src/main/utils/__tests__/atomic-file.test.ts +++ b/apps/frontend/src/main/utils/__tests__/atomic-file.test.ts @@ -3,10 +3,11 @@ */ import { describe, expect, it, beforeEach, afterEach, vi } from 'vitest'; -import { existsSync } from 'fs'; +import { existsSync, readFileSync, writeFileSync, readdirSync, mkdirSync } from 'fs'; import path from 'path'; import { writeFileAtomic, + writeFileAtomicSync, writeFileWithRetry, readFileWithRetry, writeJsonAtomic, @@ -436,6 +437,98 @@ describe('writeJsonWithRetry', () => { }); }); +describe('writeFileAtomicSync', () => { + beforeEach(async () => { + if (existsSync(TEST_DIR)) { + await rm(TEST_DIR, { recursive: true, force: true }); + } + await mkdir(TEST_DIR, { recursive: true }); + }); + + afterEach(async () => { + if (existsSync(TEST_DIR)) { + await rm(TEST_DIR, { recursive: true, force: true }); + } + }); + + describe('basic operations', () => { + it('should write a new file atomically', () => { + const filePath = path.join(TEST_DIR, 'sync-test.txt'); + const content = 'Hello, sync!'; + + writeFileAtomicSync(filePath, content); + + const result = readFileSync(filePath, 'utf-8'); + expect(result).toBe(content); + }); + + it('should overwrite an existing file atomically', () => { + const filePath = path.join(TEST_DIR, 'sync-existing.txt'); + writeFileSync(filePath, 'Initial content', 'utf-8'); + + writeFileAtomicSync(filePath, 'New content'); + + const result = readFileSync(filePath, 'utf-8'); + expect(result).toBe('New content'); + }); + + it('should write Buffer data', () => { + const filePath = path.join(TEST_DIR, 'sync-buffer.bin'); + const buffer = Buffer.from([0x48, 0x65, 0x6c, 0x6c, 0x6f]); + + writeFileAtomicSync(filePath, buffer); + + const result = readFileSync(filePath); + expect(result).toEqual(buffer); + }); + + it('should resolve relative paths', () => { + const absolutePath = path.join(TEST_DIR, 'sync-resolve.txt'); + // Use a relative path that resolves to the same location + const relativePath = path.relative(process.cwd(), absolutePath); + + writeFileAtomicSync(relativePath, 'resolved content'); + + const result = readFileSync(absolutePath, 'utf-8'); + expect(result).toBe('resolved content'); + }); + }); + + describe('temp file cleanup', () => { + it('should not leave temp files after successful write', () => { + const filePath = path.join(TEST_DIR, 'sync-no-temp.txt'); + + writeFileAtomicSync(filePath, 'content'); + + const files = readdirSync(TEST_DIR); + const tempFiles = files.filter(name => name.includes('.tmp.')); + expect(tempFiles).toHaveLength(0); + }); + + it('should clean up temp file when rename fails', () => { + // Create a subdirectory as the "target" — renameSync will fail because + // you can't atomically replace a directory with a file + const dirTarget = path.join(TEST_DIR, 'is-a-dir'); + mkdirSync(dirTarget); + + expect(() => writeFileAtomicSync(dirTarget, 'content')).toThrow(); + + // Verify temp file was cleaned up (it was created in TEST_DIR) + const files = readdirSync(TEST_DIR); + const tempFiles = files.filter(name => name.includes('.tmp.')); + expect(tempFiles).toHaveLength(0); + }); + }); + + describe('error handling', () => { + it('should throw when directory does not exist', () => { + const filePath = path.join(TEST_DIR, 'no', 'such', 'dir', 'file.txt'); + + expect(() => writeFileAtomicSync(filePath, 'content')).toThrow(); + }); + }); +}); + describe('AtomicFileError', () => { it('should be an instance of Error', () => { const error = new AtomicFileError('Test error'); diff --git a/apps/frontend/src/main/utils/atomic-file.ts b/apps/frontend/src/main/utils/atomic-file.ts index d3d8329f..1bc934c6 100644 --- a/apps/frontend/src/main/utils/atomic-file.ts +++ b/apps/frontend/src/main/utils/atomic-file.ts @@ -15,7 +15,7 @@ */ import { mkdir, rename, unlink, writeFile, readFile } from 'fs/promises'; -import { existsSync } from 'fs'; +import { existsSync, writeFileSync, renameSync, unlinkSync } from 'fs'; import path from 'path'; import { randomBytes } from 'crypto'; @@ -83,6 +83,37 @@ export async function writeFileAtomic( } } +/** + * Synchronous variant of writeFileAtomic. + * + * Write data to file atomically using temp file and rename. + * Uses randomBytes for collision-safe temp file naming. + * + * NOTE: Unlike writeFileAtomic, this function does NOT create parent directories. + * The caller must ensure the target directory exists. + * + * @param filepath - Target file path + * @param data - Data to write (string or Buffer) + * @param encoding - File encoding (default: 'utf-8') + */ +export function writeFileAtomicSync( + filepath: string, + data: string | Buffer, + encoding: BufferEncoding = 'utf-8' +): void { + const absolutePath = path.resolve(filepath); + const dir = path.dirname(absolutePath); + const tempSuffix = randomBytes(8).toString('hex'); + const tempPath = path.join(dir, `.${path.basename(absolutePath)}.tmp.${tempSuffix}`); + try { + writeFileSync(tempPath, data, encoding); + renameSync(tempPath, absolutePath); + } catch (err) { + try { unlinkSync(tempPath); } catch { /* ignore cleanup */ } + throw err; + } +} + /** * Write data to file atomically with retry logic. *