From 607de7ccfd3e3d685869730c34a286dd2dcd57f6 Mon Sep 17 00:00:00 2001 From: ydflow Date: Sun, 27 Sep 2026 13:13:08 +0800 Subject: [PATCH 1/3] fix(persistence): write state.json and the search index atomically (#854) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit saveState, saveStateForScope and buildIndex wrote in place: open + truncate + write. A crash, kill, ENOSPC or power loss mid-write leaves truncated JSON, and the readers answer null for what they cannot parse: - state.json: loadStateForScope falls back to StateSchema.parse({}) — lastPullRev and every per-checkout pushBaseRevs entry are gone without a word, so the next push compares against a stale base. That is the stale-base overwrite class the worktree fixes (#827) closed. - search-index.json: loadIndex returns null, so recall silently loses the whole corpus until the next rebuild, and the shrink guard loses its baseline. The repo already writes config.yaml (#831) and the votes file atomically for exactly this reason, and writeJsonAtomic exists: temp file + rename, preserving the target's permission bits. Move the three writers to it. No reader changes; a successful write behaves as before. --- src/__tests__/state-atomic-save.test.ts | 140 ++++++++++++++++++++++++ src/config.ts | 9 +- src/utils/search-index.ts | 6 +- 3 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 src/__tests__/state-atomic-save.test.ts diff --git a/src/__tests__/state-atomic-save.test.ts b/src/__tests__/state-atomic-save.test.ts new file mode 100644 index 000000000..8d90a80c1 --- /dev/null +++ b/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, 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']); + }); + }); +}); diff --git a/src/config.ts b/src/config.ts index 04cc88c99..688720700 100644 --- a/src/config.ts +++ b/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 { * Save the local state */ export async function saveState(state: State): Promise { - 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 { const statePath = getStatePath(localConfig); - await writeJson(expandHome(statePath), state); + await writeJsonAtomic(expandHome(statePath), state); } /** diff --git a/src/utils/search-index.ts b/src/utils/search-index.ts index 51530d5fd..dfea0c025 100644 --- a/src/utils/search-index.ts +++ b/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`); From 96bdda68c5e3232e7ecca73c29cce277e1b27d09 Mon Sep 17 00:00:00 2001 From: ydflow Date: Sun, 27 Sep 2026 13:30:50 +0800 Subject: [PATCH 2/3] test: inject index-write failures at the write call, not the file mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed recall-rebuild-roots: the 'cannot be written' test chmod'd the index file 0o444, which fails the in-place writer but not the atomic one — writeJsonAtomic stages a temp sibling (the directory is writable) and renames over the target, and rename needs only the directory's permission. Same for the EISDIR test: renaming a file over a directory is EISDIR on POSIX but EPERM on Windows. Fail the fse.writeFile calls at the index path itself — the staged temp file included — so the injection works for both writers on every platform. The root-skip guard goes with the chmod it existed for. --- src/__tests__/recall-rebuild-roots.test.ts | 38 +++++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/__tests__/recall-rebuild-roots.test.ts b/src/__tests__/recall-rebuild-roots.test.ts index 76975ff78..7bf00456d 100644 --- a/src/__tests__/recall-rebuild-roots.test.ts +++ b/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'); From 80b3200633702ef5287ac89095932370c153265e Mon Sep 17 00:00:00 2001 From: ydflow <314143294+ydflow@users.noreply.github.com> Date: Sun, 27 Sep 2026 17:57:26 +0800 Subject: [PATCH 3/3] test: force the #812 e2e state-write failure at the write call, not the file mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI failed the 'cannot be recorded' case: it chmod'd state.json 0o444, which stops the in-place writer but not the atomic one — writeJsonAtomic stages a temp sibling and renames, and rename needs only the directory's permission, so the push completed normally and exited 0. (The e2e config retries once; the retry then failed teammatePublishes' git commit with 'nothing to commit', the run's second error.) The CLI runs as a subprocess, so the write call cannot be spied on the way state-atomic-save does it. Inject the failure from outside instead: a preload hook (NODE_OPTIONS --require) fails every fs.writeFile targeting the staged temp of this state file inside the CLI process — the same seam as the unit tests, on every platform. --- .../e2e/push-stale-worktree-812.test.ts | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/src/__tests__/e2e/push-stale-worktree-812.test.ts b/src/__tests__/e2e/push-stale-worktree-812.test.ts index 285693dce..4c5d0c164 100644 --- a/src/__tests__/e2e/push-stale-worktree-812.test.ts +++ b/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 { +function runCLI(args: string[], cwd: string, home: string, extraEnv: Record = {}): Promise { 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 }); } }); });