Skip to content

Commit aadb0d5

Browse files
authored
fix: state.json の保存競合と非 atomic write を修正(#112) (#119)
## 概要 `#112` の対応。セッション復元用 `.codiva/state.json` の保存にあった 2 つの問題を修正しました。 1. **非 atomic write** — 保存先へ直接 `writeFile` していたため、書き込み中にプロセスが死ぬと切れた JSON が残り、次回起動で `loadState` が破損扱いして `emptyPersistedState()` に落ちる(= 復元可能なセッションを全部失う。worktree は残るが codiva から辿れない)。 2. **保存の競合** — debounce の非同期保存が飛んでいる最中に終了時 `flushAsync()` / シグナル時 `flushSync()` が最新状態を書くと、遅れて完了した古い書き込みが最新状態を巻き戻す。 ## 変更内容 ### `src/utils/state-store.ts` - 同一ディレクトリの一時ファイルへ書き、**fsync → close → rename** で差し替える(同期版 `saveStateSync` も同じ)。rename は POSIX で atomic なので、途中で死んでも `state.json` は「前回の完全な内容」か「新しい完全な内容」のどちらかになる。 - 一時ファイル名は `<path>.<pid>.<async|sync>.tmp` 固定。**非同期の書き込みはパスごとに直列化**することで同名 temp を 2 本同時に開かないようにし、同時に rename の順序が呼び出し順と一致する(= 古い保存が新しい保存を上書きしない)。sync 側は別名なので in-flight の async と衝突しない(そちらの rename は `process.exit` により実行されないので巻き戻しも起きない)。 - pid を含めるのは、同じリポジトリでもう 1 つ codiva が動いていたときに半端な temp を掴み合わないため。名前を固定(呼び出しごとにユニークにしない)のは、強制終了で残る temp を 1 プロセスあたり最大 2 本に抑えるため。 - 書き込みが失敗したら temp を掃除し、既存の `state.json` はそのまま残す。 ### `src/bootstrap/persist-controller.ts` - 書き込みをキューで直列化し、**`snapshot()` は「書き始めた時点」で読む**(スケジュール時点で固めない)。これにより、遅れて着地する書き込みが古い状態を書くことが構造的に起きなくなる。 - `flushSync` は世代カウンタを上げ、その最中に走っていた非同期書き込みは**完了後に最新 snapshot を書き直す**(実際の kill 経路では直後に `process.exit` するが、プロセスが生き残る経路でも順序が壊れないようにするため)。 - 書き込みが失敗してもキューが死なない(以降の保存が黙って止まらない)。 ### テスト - `state-store.spec.ts`: 同時保存で最後の呼び出しが勝つ / temp を残さない / 書き込み失敗時に前回のファイルが無傷であること。 - `persist-controller.spec.ts`(新規): スケジュール時点ではなく書き込み時点の snapshot を書く / 書き込み中に同期 flush が入っても最新へ復旧する / 失敗後も保存を続ける / `flushSync` が投げない。 ### ドキュメント - `docs/ARCHITECTURE.md`・`.claude/rules/git-and-io.md` に「state.json は直接書かない(temp → fsync → rename)」「snapshot は書き始めた時点で読む」を不変条件として追記。 ## 対応案のうち見送ったもの - **前回正常ファイルのバックアップからの復旧**: atomic rename により「途中まで書かれたファイル」が公開されなくなるため、`state.json.bak` の世代管理を足す価値が薄いと判断しました(復元不能な壊れ方をするのは、そもそも rename が atomic でないファイルシステムの場合のみ)。必要なら別 issue で。 ## テスト計画 - [ ] CI(lint / typecheck / test / build) - [ ] 手動: セッションを複数走らせた状態で `kill -TERM` → 再起動して一覧が復元されること Closes #112
1 parent 83557a3 commit aadb0d5

6 files changed

Lines changed: 287 additions & 10 deletions

File tree

.claude/rules/git-and-io.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,18 @@
121121
設定ミスや壊れた JSON で TUI を落とさない。
122122
- 保存は debounce(500ms、`bootstrap/persist-controller.ts`)+ 終了時 flush +
123123
SIGTERM/SIGHUP の**同期 flush**`saveStateSync`)。この3経路を1つに減らさない。
124+
- **`state.json` は直接書かない**`utils/state-store.ts`)。同じディレクトリの一時ファイルへ書き、
125+
**fsync → close → rename** で差し替える。途中で死んで切れた JSON が残ると `loadState`
126+
空状態へフォールバックし、**復元可能なセッションが全部消える**(worktree は残るが codiva から
127+
辿れない)。一時ファイル名は `<path>.<pid>.<async|sync>.tmp` 固定で、
128+
**非同期の書き込みはパスごとに直列化**する(同名の temp を 2 本同時に開かないため。
129+
ついでに rename の順序が呼び出し順と一致するので、古い保存が新しい保存を上書きしない)。
130+
- **保存内容は「書き始めた時点」の snapshot にする**`persist-controller` が直列化したキューの中で
131+
`snapshot()` を呼ぶ)。スケジュール時に固めると、debounce の書き込みが飛んでいる最中に
132+
最終 flush が走ったとき、遅れて完了した古い書き込みが最新状態を巻き戻す。
133+
同期 flush は世代カウンタを上げ、その最中に走っていた非同期書き込みは**完了後に書き直す**
134+
書き直しは**世代が安定するまで繰り返す**(1 回だけだと、その書き直しの最中に入った 2 度目の
135+
同期 flush を、書き直し自身の rename が巻き戻す)。
124136

125137
## 端末・OS への副作用
126138

docs/ARCHITECTURE.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -857,7 +857,11 @@ UI 文字列は日本語/英語を設定で切り替えられる。規約は [.c
857857
`activeElapsedMs` で稼働中セグメントを畳み込んで凍結し、復元時は `activeSince` を未設定
858858
(idle)にしてオフライン時間を数えない。
859859
保存は `onPersist` → debounce(合成ルート)+終了時の最終フラッシュ+ SIGTERM/SIGHUP 時の
860-
同期フラッシュ(`saveStateSync`)。`stop()` は保留中の許可要求を deny で解決してから停止し、
860+
同期フラッシュ(`saveStateSync`)。**書き込みは temp → fsync → rename の atomic 差し替え**で、
861+
非同期の書き込みはパスごとに直列化する(`utils/state-store.ts`)。直接書くと、途中で死んだときに
862+
切れた JSON が残って `loadState` が空状態へ落ち、復元できるセッションを全部失う。
863+
保存する snapshot は**書き始めた時点**で読む(`persist-controller`)ので、飛んでいる最中の
864+
古い書き込みが最新状態を巻き戻すことはない。`stop()` は保留中の許可要求を deny で解決してから停止し、
861865
resume 先のトランスクリプトが未応答の `tool_use` で終わらないようにする(best-effort)。
862866

863867
## Phase 10 機能(origin 追従 / PR 自動化 / 競合検知)
Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
import { mkdir, mkdtemp, rm } from 'node:fs/promises';
2+
import { tmpdir } from 'node:os';
3+
import { join } from 'node:path';
4+
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
5+
import type { PersistedState } from '@/core';
6+
import { defaultStatePath, loadState } from '@/utils';
7+
import { createPersistController, type PersistController } from './persist-controller';
8+
9+
let dir: string;
10+
11+
beforeEach(async () => {
12+
dir = await mkdtemp(join(tmpdir(), 'codiva-persist-'));
13+
});
14+
15+
afterEach(async () => {
16+
await rm(dir, { recursive: true, force: true });
17+
});
18+
19+
function stateWith(id: string): PersistedState {
20+
return {
21+
version: 1,
22+
sessions: [
23+
{
24+
id,
25+
title: id,
26+
prompt: 'do it',
27+
slug: id,
28+
branch: `codiva/${id}`,
29+
worktreePath: `/tmp/wt/${id}`,
30+
base: 'main',
31+
sdkSessionId: `sdk-${id}`,
32+
status: 'completed',
33+
startedAt: 0,
34+
todos: [],
35+
},
36+
],
37+
};
38+
}
39+
40+
async function savedIds(path: string): Promise<string[]> {
41+
return (await loadState(path)).sessions.map((s) => s.id);
42+
}
43+
44+
/** A path that every write fails on (renaming a file onto a non-empty directory). */
45+
async function blockedPath(): Promise<string> {
46+
const path = join(dir, 'blocked');
47+
await mkdir(join(path, 'child'), { recursive: true });
48+
return path;
49+
}
50+
51+
describe('createPersistController', () => {
52+
it('writes the snapshot as of the write, not as of scheduling', async () => {
53+
const path = defaultStatePath(dir);
54+
let current = stateWith('old');
55+
const persist = createPersistController(() => current, path);
56+
persist.schedule();
57+
current = stateWith('new');
58+
await persist.flushAsync();
59+
expect(await savedIds(path)).toEqual(['new']);
60+
});
61+
62+
it('repairs the file when a synchronous flush lands mid-write', async () => {
63+
const path = defaultStatePath(dir);
64+
let calls = 0;
65+
let persist: PersistController | undefined;
66+
const snapshot = (): PersistedState => {
67+
if (calls++ === 0) {
68+
// The kill path fires while this (already stale) write is still in flight.
69+
queueMicrotask(() => persist?.flushSync());
70+
return stateWith('old');
71+
}
72+
return stateWith('newest');
73+
};
74+
persist = createPersistController(snapshot, path);
75+
await persist.flushAsync();
76+
expect(await savedIds(path)).toEqual(['newest']);
77+
});
78+
79+
it('repairs again when another synchronous flush lands during the repair write', async () => {
80+
const path = defaultStatePath(dir);
81+
// Pins the ordering from the review: async write (gen 0) → sync flush (gen 1)
82+
// → repair write → sync flush again (gen 2) → repair write's rename lands last.
83+
// Repairing only once would leave 'stale2' on disk.
84+
const script = ['stale1', 'mid1', 'stale2', 'mid2', 'final'];
85+
let call = 0;
86+
let persist: PersistController | undefined;
87+
const snapshot = (): PersistedState => {
88+
const index = call++;
89+
if (index === 0 || index === 2) {
90+
// A sync flush fires while this write is still in flight.
91+
queueMicrotask(() => persist?.flushSync());
92+
}
93+
return stateWith(script[index] ?? 'final');
94+
};
95+
persist = createPersistController(snapshot, path);
96+
await persist.flushAsync();
97+
expect(await savedIds(path)).toEqual(['final']);
98+
});
99+
100+
it('keeps saving after a failed write', async () => {
101+
const blocked = await blockedPath();
102+
const persist = createPersistController(() => stateWith('a'), blocked);
103+
await expect(persist.flushAsync()).resolves.toBeUndefined();
104+
await expect(persist.flushAsync()).resolves.toBeUndefined();
105+
const path = defaultStatePath(dir);
106+
const ok = createPersistController(() => stateWith('b'), path);
107+
await ok.flushAsync();
108+
expect(await savedIds(path)).toEqual(['b']);
109+
});
110+
111+
it('flushSync swallows write failures', async () => {
112+
const persist = createPersistController(() => stateWith('a'), await blockedPath());
113+
expect(() => persist.flushSync()).not.toThrow();
114+
});
115+
});

src/bootstrap/persist-controller.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,35 @@ export function createPersistController(
2121
statePath: string,
2222
): PersistController {
2323
let timer: ReturnType<typeof setTimeout> | undefined;
24-
const save = () => saveState(snapshot(), statePath).catch(() => undefined);
24+
// Bumped by every synchronous flush. An async write that was already running
25+
// captured the snapshot *before* that flush, so its rename rolled the file back
26+
// to older state — write the current snapshot again to repair it. (In the real
27+
// shutdown path `process.exit` follows the sync flush and nothing runs, but the
28+
// repair keeps the ordering honest wherever the process survives.)
29+
let syncGeneration = 0;
30+
// Writes are chained so the *content* is current too: the snapshot is read when
31+
// the write starts, never when it was scheduled. That makes it impossible for a
32+
// queued save to land state older than what the previous write already published.
33+
let queue: Promise<void> = Promise.resolve();
34+
const save = (): Promise<void> => {
35+
const run = queue.then(async () => {
36+
// Loop until no sync flush landed while we were writing: repairing once is
37+
// not enough, since a second flush during the repair write would itself be
38+
// rolled back by that write's rename. Terminates because the only caller of
39+
// `flushSync` is the signal handler, which exits right after.
40+
let generation = syncGeneration;
41+
for (;;) {
42+
await saveState(snapshot(), statePath);
43+
if (generation === syncGeneration) {
44+
return;
45+
}
46+
generation = syncGeneration;
47+
}
48+
});
49+
// Keep the chain alive after a failed write; saves are best-effort.
50+
queue = run.catch(() => undefined);
51+
return queue;
52+
};
2553
return {
2654
schedule: () => {
2755
if (timer) {
@@ -36,6 +64,8 @@ export function createPersistController(
3664
saveStateSync(snapshot(), statePath);
3765
} catch {
3866
// best-effort — never block shutdown on a failed save
67+
} finally {
68+
syncGeneration += 1;
3969
}
4070
},
4171
flushAsync: async () => {

src/utils/state-store.spec.ts

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises';
1+
import { mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises';
22
import { tmpdir } from 'node:os';
33
import { join } from 'node:path';
44
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
@@ -72,6 +72,35 @@ describe('saveState / loadState', () => {
7272
saveStateSync(state, path);
7373
expect(await loadState(path)).toEqual(state);
7474
});
75+
76+
it('serializes concurrent writes so the last caller wins', async () => {
77+
const path = defaultStatePath(dir);
78+
const first = sampleState('/tmp/wt/first');
79+
const last = sampleState('/tmp/wt/last');
80+
// Both start before either finishes: the earlier (stale) write must not land last.
81+
await Promise.all([saveState(first, path), saveState(last, path)]);
82+
expect(await loadState(path)).toEqual(last);
83+
});
84+
85+
it('leaves no temp files behind', async () => {
86+
const path = defaultStatePath(dir);
87+
await saveState(sampleState('/tmp/wt/a'), path);
88+
saveStateSync(sampleState('/tmp/wt/b'), path);
89+
expect(await readdir(join(dir, '.codiva'))).toEqual(['state.json']);
90+
});
91+
92+
it('keeps the previous file intact when a write fails', async () => {
93+
const path = defaultStatePath(dir);
94+
const good = sampleState('/tmp/wt/good');
95+
await saveState(good, path);
96+
// A directory in the way makes the rename fail after the temp file is written.
97+
await mkdir(join(dir, 'blocked', 'child'), { recursive: true });
98+
const blocked = join(dir, 'blocked');
99+
await expect(saveState(sampleState('/tmp/wt/bad'), blocked)).rejects.toThrow();
100+
expect(() => saveStateSync(sampleState('/tmp/wt/bad'), blocked)).toThrow();
101+
expect((await readdir(dir)).sort()).toEqual(['.codiva', 'blocked']);
102+
expect(await loadState(path)).toEqual(good);
103+
});
75104
});
76105

77106
describe('pruneMissingWorktrees', () => {

src/utils/state-store.ts

Lines changed: 94 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,14 @@
1-
import { existsSync, mkdirSync, writeFileSync } from 'node:fs';
2-
import { mkdir, readFile, writeFile } from 'node:fs/promises';
1+
import {
2+
closeSync,
3+
existsSync,
4+
fsyncSync,
5+
mkdirSync,
6+
openSync,
7+
renameSync,
8+
rmSync,
9+
writeSync,
10+
} from 'node:fs';
11+
import { mkdir, open, readFile, rename, rm } from 'node:fs/promises';
312
import { dirname, join } from 'node:path';
413
import { emptyPersistedState, fromPersistedJson, type PersistedState } from '@/core';
514

@@ -21,19 +30,97 @@ export async function loadState(path: string): Promise<PersistedState> {
2130
}
2231
}
2332

24-
/** Write persisted state, creating `.codiva/` if needed. */
25-
export async function saveState(state: PersistedState, path: string): Promise<void> {
33+
function serialize(state: PersistedState): string {
34+
return `${JSON.stringify(state, null, 2)}\n`;
35+
}
36+
37+
/**
38+
* Temp file for the atomic write, in the same directory so `rename` stays within
39+
* one filesystem. The name is fixed per (path, process, writer) instead of unique
40+
* per call so a process killed mid-write leaves at most two strays, not one per
41+
* write: async writes are serialized (see `saveState`) and the sync writer gets its
42+
* own name, so no two live writes ever share a temp file. The pid keeps a second
43+
* codiva running on the same repo from clobbering our half-written temp.
44+
*/
45+
function tempPath(path: string, writer: 'async' | 'sync'): string {
46+
return `${path}.${process.pid}.${writer}.tmp`;
47+
}
48+
49+
/** Serializes writes per state path — see `saveState`. */
50+
const writeQueues = new Map<string, Promise<void>>();
51+
52+
async function writeAtomic(state: PersistedState, path: string): Promise<void> {
2653
await mkdir(dirname(path), { recursive: true });
27-
await writeFile(path, `${JSON.stringify(state, null, 2)}\n`, 'utf8');
54+
const tmp = tempPath(path, 'async');
55+
try {
56+
const handle = await open(tmp, 'w');
57+
try {
58+
await handle.writeFile(serialize(state), 'utf8');
59+
// fsync before the rename: without it a crash can publish a rename whose
60+
// bytes never reached disk, and `loadState` would fall back to empty state.
61+
await handle.sync();
62+
} finally {
63+
await handle.close();
64+
}
65+
await rename(tmp, path);
66+
} catch (error) {
67+
await rm(tmp, { force: true }).catch(() => undefined);
68+
throw error;
69+
}
70+
}
71+
72+
/**
73+
* Write persisted state, creating `.codiva/` if needed.
74+
*
75+
* Two hazards this has to avoid, both of which lose every restorable session:
76+
* 1. **Torn file** — writing `path` in place leaves truncated JSON if the process
77+
* dies mid-write, and `loadState` reads that as "no sessions". So we write a
78+
* temp file, fsync it, and `rename` it over the target (atomic on POSIX).
79+
* 2. **Out-of-order writes** — a debounced save still in flight must not land
80+
* after a newer one. Writes to the same path are chained, so the renames
81+
* happen in call order and the last caller wins.
82+
*/
83+
export async function saveState(state: PersistedState, path: string): Promise<void> {
84+
const tail = writeQueues.get(path) ?? Promise.resolve();
85+
const run = tail.then(() => writeAtomic(state, path));
86+
// The queue itself must never reject: one failed write must not poison later saves.
87+
writeQueues.set(
88+
path,
89+
run.then(
90+
() => undefined,
91+
() => undefined,
92+
),
93+
);
94+
await run;
2895
}
2996

3097
/**
3198
* Synchronous save for exit/signal handlers (SIGTERM/SIGHUP), where the event
32-
* loop won't run pending async writes before the process dies.
99+
* loop won't run pending async writes before the process dies. Same temp+rename
100+
* dance as `saveState`, on its own temp file so it can't collide with an async
101+
* write that is still in flight (that one's rename never happens — the process
102+
* exits first — so it cannot roll this snapshot back either).
33103
*/
34104
export function saveStateSync(state: PersistedState, path: string): void {
35105
mkdirSync(dirname(path), { recursive: true });
36-
writeFileSync(path, `${JSON.stringify(state, null, 2)}\n`, 'utf8');
106+
const tmp = tempPath(path, 'sync');
107+
try {
108+
const fd = openSync(tmp, 'w');
109+
try {
110+
writeSync(fd, serialize(state));
111+
fsyncSync(fd);
112+
} finally {
113+
closeSync(fd);
114+
}
115+
renameSync(tmp, path);
116+
} catch (error) {
117+
try {
118+
rmSync(tmp, { force: true });
119+
} catch {
120+
// leaving a stray temp file behind is better than masking the real error
121+
}
122+
throw error;
123+
}
37124
}
38125

39126
/**

0 commit comments

Comments
 (0)