fix(persistence): write state.json and the search index atomically (#854)

src/__tests__/e2e/push-stale-worktree-812.test.ts

@@ -34,11 +34,11 @@ interface RunResult {
   output: string;
 }
 
-function runCLI(args: string[], cwd: string, home: string): Promise<RunResult> {
+function runCLI(args: string[], cwd: string, home: string, extraEnv: Record<string, string> = {}): Promise<RunResult> {
   return new Promise((resolve) => {
     const child = spawn('node', [CLI, ...args], {
       cwd,
-      env: { ...process.env, ...GIT_ENV, HOME: home, FORCE_COLOR: '0' },
+      env: { ...process.env, ...GIT_ENV, HOME: home, FORCE_COLOR: '0', ...extraEnv },
       stdio: ['ignore', 'pipe', 'pipe'],
     });
     let output = '';
@@ -401,18 +401,37 @@ describe('push from a stale linked worktree (#812)', () => {
 
   it.skipIf(process.getuid?.() === 0)('stops the push when the synced revision cannot be recorded', async () => {
     const partial = path.join(sandbox, 'wt-g');
-    teammatePublishes('# Team rule\n\nVersion synced while state.json is read-only.\n');
+    teammatePublishes('# Team rule\n\nVersion synced while the state file refuses writes.\n');
     const stateFile = projectState()?.file;
     expect(stateFile).toBeDefined();
     if (!stateFile) return;
-    fs.chmodSync(stateFile, 0o444);
+    // state.json is saved through writeJsonAtomic, so a chmod on the file
+    // cannot force the failure: the writer stages a temp sibling and renames,
+    // and rename needs only the writable directory (a read-only target is
+    // replaced just fine). Inject the failure at the write call instead, like
+    // the atomic-save unit tests do: a preload hook fails every fs.writeFile
+    // targeting the staged temp of this state file inside the CLI process.
+    const hook = path.join(sandbox, 'fail-state-write.cjs');
+    fs.writeFileSync(hook, [
+      "'use strict';",
+      `const TARGET = ${JSON.stringify(stateFile)};`,
+      "const fs = require('node:fs');",
+      "const hit = (p) => typeof p === 'string' && p.startsWith(TARGET + '.') && p.endsWith('.tmp');",
+      'const fail = (p) => { throw new Error(`simulated state write failure: ${p}`); };',
+      'const writeFile = fs.writeFile.bind(fs);',
+      'fs.writeFile = (p, ...rest) => (hit(p) ? fail(p) : writeFile(p, ...rest));',
+      'const writeFileSync = fs.writeFileSync.bind(fs);',
+      'fs.writeFileSync = (p, ...rest) => (hit(p) ? fail(p) : writeFileSync(p, ...rest));',
+      'const pWriteFile = fs.promises.writeFile.bind(fs.promises);',
+      'fs.promises.writeFile = (p, ...rest) => (hit(p) ? fail(p) : pWriteFile(p, ...rest));',
+    ].join('\n'));
     try {
-      const r = await runCLI(['--dry-run', 'push'], partial, home);
+      const r = await runCLI(['--dry-run', 'push'], partial, home, { NODE_OPTIONS: `--require ${hook}` });
       expect(r.code, r.output).toBe(1);
       expect(r.output).toContain('Nothing was pushed');
       expect(r.output).not.toContain('Scanning local resources');
     } finally {
-      fs.chmodSync(stateFile, 0o644);
+      fs.rmSync(hook, { force: true });
     }
   });
 });

src/__tests__/recall-rebuild-roots.test.ts

@@ -1,4 +1,5 @@
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import fse from 'fs-extra';
 import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
@@ -279,10 +280,21 @@ describe('recall rebuilding a missing index with a team manifest it cannot read
   });
 
   it('names the cause when the build fails for another reason, not "No learnings available"', async () => {
-    // The index path is a directory, so writing the index fails.
-    fs.mkdirSync(indexPath(), { recursive: true });
-
-    await recall('retry budget', {});
+    // Fail the index write itself — the atomic writer's staged temp file
+    // included (#854) — so the build fails for a reason the warning must name.
+    const realWriteFile = fse.writeFile;
+    const failIndexWrites = vi.spyOn(fse, 'writeFile').mockImplementation(async (file: unknown, data: unknown) => {
+      if (typeof file !== 'string' || typeof data !== 'string') throw new Error('unexpected writeFile call in test');
+      if (file === indexPath() || file.startsWith(`${indexPath()}.`)) {
+        throw Object.assign(new Error(`EISDIR: illegal operation on a directory, open '${file}'`), { code: 'EISDIR' });
+      }
+      return realWriteFile(file, data, 'utf-8');
+    });
+    try {
+      await recall('retry budget', {});
+    } finally {
+      failIndexWrites.mockRestore();
+    }
 
     expect(warnings()).toContainEqual(expect.stringMatching(/Recall could not build the user search index: .*EISDIR/));
     expect(vi.mocked(log.info).mock.calls.map(([message]) => String(message)))
@@ -351,16 +363,26 @@ describe('recall rebuilding an older-format index with a team manifest it cannot
     expect(warnings()).toContainEqual(expect.stringContaining('Index rebuild skipped'));
   });
 
-  // Root writes through the read-only bit, so the rebuild would succeed.
-  it.skipIf(process.getuid?.() === 0)('searches nothing, not the older index, when the partial index cannot be written', async () => {
-    fs.chmodSync(indexPath(), 0o444);
+  // The atomic index write (#854) stages a temp sibling and renames it into
+  // place, so a read-only index file no longer fails the write — rename needs
+  // only the directory. Fail the writes at the index path itself, the temp file
+  // included, which works for both the staged and the in-place writer.
+  it('searches nothing, not the older index, when the partial index cannot be written', async () => {
     const out: string[] = [];
     const write = vi.spyOn(process.stdout, 'write').mockImplementation((chunk) => { out.push(String(chunk)); return true; });
+    const realWriteFile = fse.writeFile;
+    const failIndexWrites = vi.spyOn(fse, 'writeFile').mockImplementation(async (file: unknown, data: unknown) => {
+      if (typeof file !== 'string' || typeof data !== 'string') throw new Error('unexpected writeFile call in test');
+      if (file === indexPath() || file.startsWith(`${indexPath()}.`)) {
+        throw Object.assign(new Error(`EACCES: permission denied, open '${file}'`), { code: 'EACCES' });
+      }
+      return realWriteFile(file, data, 'utf-8');
+    });
     try {
       await recall('retry budget', {});
     } finally {
       write.mockRestore();
-      fs.chmodSync(indexPath(), 0o644);
+      failIndexWrites.mockRestore();
     }
 
     expect(out.join('')).not.toContain('stale');

src/__tests__/state-atomic-save.test.ts

@@ -0,0 +1,140 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import fse from 'fs-extra';
+import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import { loadStateForScope, saveState, saveStateForScope } from '../config.js';
+import { buildIndex, loadIndex } from '../utils/search-index.js';
+import type { LocalConfig, State } from '../types.js';
+
+// Issue #854: state.json and search-index.json were written in place. Opening
+// the target for writing truncates it, so a reader mid-save saw an empty file —
+// and readJson answers null for what it cannot parse: state.json came back as
+// parse({}), dropping every pull/push base (the stale-base overwrite class the
+// worktree fixes closed), and the search index as no index at all, wiping
+// recall until the next rebuild. config.yaml got the same fix in #831, and
+// saveUserVotes documents the identical failure mode for the votes file.
+describe('machine state saves are atomic', () => {
+  const originalHome = process.env.HOME;
+  let home: string;
+  let stateDir: string;
+  let statePath: string;
+  let localConfig: LocalConfig;
+
+  beforeEach(() => {
+    home = mkdtempSync(path.join(os.tmpdir(), 'teamai-atomic-state-'));
+    process.env.HOME = home;
+    stateDir = path.join(home, '.teamai');
+    statePath = path.join(stateDir, 'state.json');
+    mkdirSync(stateDir, { recursive: true });
+    localConfig = {
+      repo: { localPath: '/nonexistent/team-repo', remote: 'https://github.com/acme/team.git' },
+      username: 'dev',
+      updatePolicy: 'auto',
+      scope: 'user',
+    } as unknown as LocalConfig;
+  });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+    if (originalHome === undefined) delete process.env.HOME;
+    else process.env.HOME = originalHome;
+    rmSync(home, { recursive: true, force: true });
+  });
+
+  const initialState = JSON.stringify({
+    lastPull: '2026-09-27T00:00:00.000Z',
+    lastPullRev: 'abc1234',
+    lastPullByWorkspace: { work: { rev: 'abc1234', targets: ['claude'], pushBaseRevs: ['def5678'] } },
+  }, null, 2) + '\n';
+
+  const updatedState = {
+    lastPull: null,
+    lastPush: null,
+    lastPullRev: 'fedcba9',
+  } as unknown as State;
+
+  /**
+   * Every write into the state dir first truncates its target — the state an
+   * in-place write exposes between open(O_TRUNC) and the data landing. Then
+   * `during` runs (a concurrent reader), and the write either completes or
+   * fails with ENOSPC. Same method as the config.yaml save test (#831).
+   */
+  function interruptStateWrites(during: () => Promise<void>, outcome: 'complete' | 'fail'): void {
+    const realWriteFile = fse.writeFile;
+    vi.spyOn(fse, 'writeFile').mockImplementation(async (file: unknown, data: unknown) => {
+      if (typeof file !== 'string' || typeof data !== 'string') throw new Error('unexpected writeFile call in test');
+      if (path.dirname(file) !== stateDir) return realWriteFile(file, data, 'utf-8');
+      await realWriteFile(file, '', 'utf-8');
+      await during();
+      if (outcome === 'fail') throw Object.assign(new Error('ENOSPC: no space left on device, write'), { code: 'ENOSPC' });
+      return realWriteFile(file, data, 'utf-8');
+    });
+  }
+
+  describe.each([
+    ['saveState', (state: State) => saveState(state)],
+    ['saveStateForScope', (state: State) => saveStateForScope(state, localConfig)],
+  ] as const)('%s', (_name, save) => {
+    it('never lets a concurrent reader see an empty or partial state', async () => {
+      writeFileSync(statePath, initialState, 'utf-8');
+      let seenMidSave: string | null | undefined;
+      interruptStateWrites(async () => {
+        seenMidSave = (await loadStateForScope(localConfig))?.lastPullRev ?? null;
+      }, 'complete');
+
+      await save(updatedState);
+
+      expect(seenMidSave).toBe('abc1234');
+      expect((await loadStateForScope(localConfig))?.lastPullRev).toBe('fedcba9');
+    });
+
+    it('a failed write leaves the previous state intact', async () => {
+      writeFileSync(statePath, initialState, 'utf-8');
+      interruptStateWrites(async () => {}, 'fail');
+
+      await expect(save(updatedState)).rejects.toThrow('ENOSPC');
+
+      expect((await loadStateForScope(localConfig))?.lastPullRev).toBe('abc1234');
+    });
+  });
+
+  describe('search index', () => {
+    let tmpDir: string;
+    let learnings: string;
+    let indexPath: string;
+
+    beforeEach(async () => {
+      tmpDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-atomic-index-'));
+      learnings = path.join(tmpDir, 'learnings');
+      mkdirSync(learnings, { recursive: true });
+      indexPath = path.join(tmpDir, 'search-index.json');
+      writeFileSync(path.join(learnings, 'a.md'), '---\ntitle: note a\n---\nretry budget');
+    });
+
+    afterEach(() => {
+      vi.restoreAllMocks();
+      rmSync(tmpDir, { recursive: true, force: true });
+    });
+
+    it('never lets a concurrent loadIndex see a torn index', async () => {
+      await buildIndex({ learningsDirs: [learnings], indexPath });
+
+      const realWriteFile = fse.writeFile;
+      vi.spyOn(fse, 'writeFile').mockImplementation(async (file: unknown, data: unknown) => {
+        if (typeof file !== 'string' || typeof data !== 'string') throw new Error('unexpected writeFile call in test');
+        if (path.dirname(file) !== tmpDir) return realWriteFile(file, data, 'utf-8');
+        await realWriteFile(file, '', 'utf-8');
+        const seenMidSave = await loadIndex(indexPath);
+        expect(seenMidSave?.entries.map((e) => e.filename)).toEqual(['a.md']);
+        return realWriteFile(file, data, 'utf-8');
+      });
+
+      // The shrink guard must not mistake the tiny fixture corpus for a partial
+      // build: one entry rebuilt as one entry is a full index, not a shrink.
+      await buildIndex({ learningsDirs: [learnings], indexPath });
+
+      expect((await loadIndex(indexPath))?.entries.map((e) => e.filename)).toEqual(['a.md']);
+    });
+  });
+});

src/config.ts

@@ -17,7 +17,7 @@ import {
   getStatePath,
   getDataHome,
 } from './types.js';
-import { readFileSafe, readJson, writeFileAtomic, writeJson, expandHome, pathExists } from './utils/fs.js';
+import { readFileSafe, readJson, writeFileAtomic, writeJsonAtomic, expandHome, pathExists } from './utils/fs.js';
 import { resolveAnchors } from './utils/git.js';
 import { getUserHome } from './utils/home.js';
 import { resolvePartitionDir, writeAnchorFile } from './utils/partition.js';
@@ -142,7 +142,10 @@ export async function loadState(): Promise<State> {
  * Save the local state
  */
 export async function saveState(state: State): Promise<void> {
-  await writeJson(expandHome(getUserStatePath()), state);
+  // state.json holds the pull/push bases (lastPullRev, per-checkout targets).
+  // A torn in-place write reads back as parse({}) on the next run, which drops
+  // every base and re-opens the stale-base overwrites #827 closed (#854).
+  await writeJsonAtomic(expandHome(getUserStatePath()), state);
 }
 
 /**
@@ -298,7 +301,7 @@ export async function loadStateForScope(localConfig: LocalConfig): Promise<State
  */
 export async function saveStateForScope(state: State, localConfig: LocalConfig): Promise<void> {
   const statePath = getStatePath(localConfig);
-  await writeJson(expandHome(statePath), state);
+  await writeJsonAtomic(expandHome(statePath), state);
 }
 
 /**

src/utils/search-index.ts

@@ -2,7 +2,7 @@ import path from 'node:path';
 import { readdir, rm } from 'node:fs/promises';
 import type { Dirent } from 'node:fs';
 import matter from 'gray-matter';
-import { readFileSafe, readJson, writeJson, listFiles, listFilesRecursive, listDirs, pathExists } from './fs.js';
+import { readFileSafe, readJson, writeJsonAtomic, listFiles, listFilesRecursive, listDirs, pathExists } from './fs.js';
 import { tokenize, wordSegments, MAX_TOKENIZE_CHARS } from './tokenizer.js';
 import { log } from './logger.js';
 import {
@@ -808,7 +808,9 @@ export async function buildIndex(
     df,
   };
 
-  await writeJson(targetPath, index);
+  // A torn in-place write parses as null on the next loadIndex, which silently
+  // wipes recall until the next rebuild — same shape as the votes file (#854).
+  await writeJsonAtomic(targetPath, index);
 
   if (elapsed > 2000) {
     log.warn(`Search index build took ${elapsed}ms — consider incremental updates for large knowledge bases`);