From 052304390ac5f126bf5fcf42c3ffbe17b33ea122 Mon Sep 17 00:00:00 2001 From: Jason Kneen Date: Tue, 22 Sep 2026 20:07:42 +0100 Subject: [PATCH] fix(coding-agent): make session-manager _rewriteFile atomic Invariant: a persisted session file on disk must always be either the previous complete contents or the new complete contents, never a partial write. Cause: _rewriteFile() opened the live session file with openSync(file, "w"), which truncates it immediately, then wrote entries one at a time. A crash, thrown exception, or ENOSPC partway through left a truncated or empty session file on disk, silently losing the session. This path runs on empty-file init, on migrateToCurrentVersion() bumping the version, and on fork. Fix: _rewriteFile() now writes all entries to a sibling temp file (same directory, name includes pid + random UUID so concurrent instances can't collide), fsyncs it, and renameSync's it over the destination. The original file is untouched until the rename commits. On any failure the temp file is removed (best-effort) and the original error is rethrown. The destination's existing permission bits are preserved on the temp file before it's renamed in; no locking is added (tracked separately). Test: packages/coding-agent/test/session-manager/rewrite-atomic.test.ts mocks writeFileSync to throw after the header write during a migration- triggered rewrite of an existing (v1-style) session file. Before the fix this truncated the file to empty; after the fix the original file is byte-identical and no temp file is left behind. A second test confirms the happy path still produces the same migrated content. --- .../coding-agent/src/core/session-manager.ts | 44 ++++++- .../session-manager/rewrite-atomic.test.ts | 107 ++++++++++++++++++ 2 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 packages/coding-agent/test/session-manager/rewrite-atomic.test.ts diff --git a/packages/coding-agent/src/core/session-manager.ts b/packages/coding-agent/src/core/session-manager.ts index aab0c0e9..353452d5 100644 --- a/packages/coding-agent/src/core/session-manager.ts +++ b/packages/coding-agent/src/core/session-manager.ts @@ -6,11 +6,15 @@ import { closeSync, createReadStream, existsSync, + fchmodSync, + fsyncSync, mkdirSync, openSync, readdirSync, readSync, + renameSync, statSync, + unlinkSync, writeFileSync, } from "fs"; import { readdir, stat } from "fs/promises"; @@ -977,15 +981,45 @@ export class SessionManager { } } + /** + * Rewrite the entire session file. Writes to a sibling temp file first and renames it + * over the destination so a crash or exception mid-write cannot leave a truncated or + * empty session file behind: the original file is untouched until the rename commits. + */ private _rewriteFile(): void { if (!this.persist || !this.sessionFile) return; - const fd = openSync(this.sessionFile, "w"); + + let existingMode: number | undefined; + try { + // Mask off the file-type bits (S_IFREG etc.) that statSync includes in `mode`; + // fchmodSync only wants the permission bits. + existingMode = statSync(this.sessionFile).mode & 0o777; + } catch { + // Destination doesn't exist yet (e.g. empty-file init); use default mode. + } + + const tempPath = `${this.sessionFile}.${process.pid}.${randomUUID()}.tmp`; try { - for (const entry of this.fileEntries) { - writeFileSync(fd, `${JSON.stringify(entry)}\n`); + const fd = openSync(tempPath, "w"); + try { + if (existingMode !== undefined) { + fchmodSync(fd, existingMode); + } + for (const entry of this.fileEntries) { + writeFileSync(fd, `${JSON.stringify(entry)}\n`); + } + fsyncSync(fd); + } finally { + closeSync(fd); + } + renameSync(tempPath, this.sessionFile); + } catch (error) { + try { + unlinkSync(tempPath); + } catch { + // Best-effort cleanup; the original error is what matters. } - } finally { - closeSync(fd); + throw error; } } diff --git a/packages/coding-agent/test/session-manager/rewrite-atomic.test.ts b/packages/coding-agent/test/session-manager/rewrite-atomic.test.ts new file mode 100644 index 00000000..b4bce7c8 --- /dev/null +++ b/packages/coding-agent/test/session-manager/rewrite-atomic.test.ts @@ -0,0 +1,107 @@ +import { mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +// _rewriteFile() (session-manager.ts) truncates the live session file and then +// writes entries one at a time. If a write fails partway through (crash, +// exception, ENOSPC) the file used to end up truncated or empty, silently +// losing the session. It must instead write a sibling temp file and rename it +// over the destination, so a failure mid-write leaves the original untouched. +const controls = vi.hoisted(() => ({ + failAfterCalls: null as number | null, + callCount: 0, +})); + +vi.mock("fs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + writeFileSync: (...args: Parameters) => { + controls.callCount++; + if (controls.failAfterCalls !== null && controls.callCount > controls.failAfterCalls) { + throw new Error("simulated ENOSPC mid-rewrite"); + } + return actual.writeFileSync(...args); + }, + }; +}); + +const { SessionManager } = await import("../../src/core/session-manager.ts"); + +describe("SessionManager._rewriteFile atomicity", () => { + let tempDir: string; + + beforeEach(() => { + tempDir = join(tmpdir(), `session-rewrite-test-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(tempDir, { recursive: true }); + controls.failAfterCalls = null; + controls.callCount = 0; + }); + + afterEach(() => { + vi.restoreAllMocks(); + rmSync(tempDir, { recursive: true, force: true }); + }); + + function writeV1SessionFile(file: string): string { + // A v1-style session (no per-entry id/parentId) forces migrateToCurrentVersion() + // to report a change, which triggers _rewriteFile() on open. + const content = + `${JSON.stringify({ type: "session", id: "sess-1", timestamp: "2025-01-01T00:00:00Z", cwd: "/tmp" })}\n` + + `${JSON.stringify({ type: "message", timestamp: "2025-01-01T00:00:01Z", message: { role: "user", content: "hi", timestamp: 1 } })}\n` + + `${JSON.stringify({ + type: "message", + timestamp: "2025-01-01T00:00:02Z", + message: { + role: "assistant", + content: [{ type: "text", text: "hello" }], + api: "test", + provider: "test", + model: "test", + usage: { input: 1, output: 1, cacheRead: 0, cacheWrite: 0 }, + stopReason: "stop", + timestamp: 2, + }, + })}\n`; + writeFileSync(file, content); + return content; + } + + it("leaves the original file byte-identical and no temp file behind when a write fails mid-rewrite", () => { + const file = join(tempDir, "session.jsonl"); + const originalContent = writeV1SessionFile(file); + + // Let the first entry write (the header) to the temp file succeed, then fail on + // the second so the rewrite is caught mid-way through populating the sibling + // temp file, after content has actually been written to it. + controls.callCount = 0; + controls.failAfterCalls = 1; + + expect(() => SessionManager.open(file, tempDir)).toThrow("simulated ENOSPC mid-rewrite"); + + // The original file must be untouched. + expect(readFileSync(file, "utf8")).toBe(originalContent); + + // No leftover temp file in the session directory. + const leftovers = readdirSync(tempDir).filter((name) => name !== "session.jsonl"); + expect(leftovers).toEqual([]); + }); + + it("still produces the same migrated content on the happy path", () => { + const file = join(tempDir, "session.jsonl"); + writeV1SessionFile(file); + + const sm = SessionManager.open(file, tempDir); + + const lines = readFileSync(file, "utf8").trim().split("\n"); + expect(lines).toHaveLength(3); + const header = JSON.parse(lines[0]); + expect(header.version).toBe(3); + expect(sm.getSessionId()).toBe("sess-1"); + + // No leftover temp files. + const leftovers = readdirSync(tempDir).filter((name) => name !== "session.jsonl"); + expect(leftovers).toEqual([]); + }); +});