fix(coding-agent): make session-manager _rewriteFile atomic

[jasonkneen]

Problem

packages/coding-agent/src/core/session-manager.ts _rewriteFile() (~line 989) opens the live session file with openSync(file, "w"), which truncates it immediately, then writes entries one at a time with writeFileSync. If the process crashes, throws, or hits ENOSPC partway through, the file is left truncated or empty — silent data loss for the whole session. This path runs on empty-file init (~line 908), whenever migrateToCurrentVersion() reports a change on load (~line 918), and on fork (~line 1520). packages/agent-core/src/harness/session/jsonl/storage.ts already uses a staged "write sibling temp file, fsync, atomic rename" pattern for the async harness; the coding-agent SessionManager (the path the CLI actually uses) did not.

Fix

_rewriteFile() now:

1. Captures the destination's existing permission bits (statSync(...).mode & 0o777) if it exists.
2. Writes all entries to a sibling temp file in the same directory, named ${sessionFile}.${pid}.${randomUUID()}.tmp so concurrent instances can't collide.
3. Applies the preserved permission bits to the temp file via fchmodSync before writing (falls back to the default mode if the destination didn't exist yet, e.g. empty-file init).
4. fsyncSync's the temp file descriptor before closing it.
5. renameSync's the temp file over the destination — an atomic, same-filesystem operation, so the destination is either the old complete content or the new complete content, never partial.
6. On any failure (write or rename), best-effort removes the temp file and rethrows the original error, leaving the destination untouched.

All three call sites (_setSessionFile's empty-file init and migration-triggered rewrite, and fork()) go through this same function, so all are covered. No locking was added (tracked as a separate concern per the issue).

Test

packages/coding-agent/test/session-manager/rewrite-atomic.test.ts:

• Mocks fs.writeFileSync (via vi.mock("fs", ...) + vi.hoisted counters) to succeed on the first write (the header) and throw on the second, simulating a crash/ENOSPC partway through populating the temp file.
• Writes a v1-style session file (entries without id/parentId) so migrateToCurrentVersion() reports a change and SessionManager.open() triggers _rewriteFile().
• Before the fix: SessionManager.open() threw as expected, but the original file was truncated to empty ('') — confirmed by running this test against the pre-fix source.
• After the fix: the original file is byte-identical to its pre-open content, and no *.tmp file is left in the session directory.
• A second test confirms the happy path still produces the same migrated 3-line content as before.

Also ran the full targeted suite (test/session-manager/, 8 files / 102 tests) and the full packages/coding-agent suite (--maxWorkers=2): 8 pre-existing failures across context-projection.test.ts, resource-loader.test.ts, step-tool-profile.test.ts, and test/suite/regressions/2791-fswatch-error-crash.test.ts remain, matching the documented baseline (these are unrelated to session persistence — provider-catalog fixtures, tool-profile expectations, and a workspace-resolution issue in a child-process regression test). No new failures were introduced. packages/agent-core and packages/providers remain fully green.

Risk / behaviour change

• Inode replacement: renameSync replaces the destination inode rather than truncating in place. Any hard link to the session file would no longer see updates, and a symlink at the session-file path would be replaced by a regular file rather than having its target's content rewritten. Session files aren't expected to be hard-linked or watched by inode, but this is a real behavior change from in-place truncation.
• Extra fsync: adds one fsync syscall per rewrite (migration/init/fork only, not the hot per-turn append path), acceptable for durability.
• Didn't touch: _persist()'s openSync(file, "wx") bulk-write loop (~line 1060), which is a separate, still-non-atomic write pattern. Left alone because it's out of the assigned scope and because it opens with "wx" (fails if the file already exists) — it only ever creates a brand-new file on the first flush of buffered entries, so a mid-write failure there leaves a partial new file rather than clobbering existing session data. Worth a follow-up if the team wants full parity.

https://claude.ai/code/session_01GUdnnHEaDThHUATSwXBpV9

[Copilot]

Copilot review overview

🟡 Changes recommended

Non-ENOENT stat errors must propagate, and permission preservation needs test coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Rethrow non-ENOENT stat errors · New

What changed in this PR

This PR makes session-file rewrites atomic to prevent partial data loss during failures.

Changes:

• Writes to unique temporary files, preserves permissions, fsyncs, and atomically renames.
• Cleans up failed temporary writes.
• Adds failure and migration regression tests.

File	Summary
packages/​coding-agent/​test/​session-manager/​rewrite-atomic.test.ts	Tests failed-write preservation and successful migration output.
packages/​coding-agent/​src/​core/​session-manager.ts	Implements atomic rewriting. Critical (3 votes): rethrow non-ENOENT statSync errors. Nit (1 vote): add POSIX permission-preservation coverage.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.