raftengine/etcd: clean up orphaned snapshot spool files on startup

[bootjp]

Snapshot spool files (elastickv-etcd-snapshot-*) were left behind when the engine crashed before snapshotSpool.Close() could run. Since the spool directory was moved from os temp to cfg.DataDir, these files accumulated without bound, eventually filling the disk.

Add cleanupStaleSnapshotSpools() and call it from openDiskState() so orphaned spools are removed on every engine restart.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[claude]

[Performance] レビュー結果

問題なし。

はスタートアップ時の1回限りの処理であり、ホットパスではありません。 → のループはO(N)ですが、孤立したスプールファイル数は限定的であり、I/Oコストも許容範囲です。

[claude]

[Test Coverage] レビュー結果

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

---

✅ カバーされているテスト

テスト	内容
TestCleanupStaleSnapshotSpools	5ファイル削除 + 無関係ファイル保持の正常系
TestCleanupStaleSnapshotSpoolsEmptyDir	空ディレクトリに対するクリーンアップ

---

⚠️ 不足しているテストケース

1. エラー蓄積パスのテストがない

cleanupStaleSnapshotSpools は errors.CombineErrors でエラーを蓄積する実装になっているが、
一部ファイルの削除が失敗した場合（例: パーミッション不足）のエラー蓄積動作がテストされていない。

提案テストケース:

func TestCleanupStaleSnapshotSpoolsPartialFailure(t *testing.T) {
    dir := t.TempDir()
    // 削除不可ファイルを作成（chmod 000 の親ディレクトリに配置するか、
    // os.Remove をモックする手法でテスト）
    // CombineErrors により複数エラーが蓄積されることを検証
}

2. openDiskState 統合のエラーパスがテストされていない

wal_store.go の openDiskState に追加されたクリーンアップ呼び出しが失敗した際、
エラーが openDiskState から正しく返却されるかを検証するテストがない。

提案テストケース:

// openDiskState がクリーンアップエラーを正しく返すことを確認
// （削除不可なスプールファイルが存在する状態で openDiskState を呼ぶ）

3. 存在しないディレクトリへの呼び出し

dir が存在しない場合、filepath.Glob は nil, nil を返すため実害はないが、
明示的なテストが存在しない。

提案テストケース（低優先度）:

func TestCleanupStaleSnapshotSpoolsNonExistentDir(t *testing.T) {
    require.NoError(t, cleanupStaleSnapshotSpools("/non/existent/path"))
}

---

総評

正常系・空ディレクトリのカバーは十分。
エラー蓄積パス（CombineErrors）と openDiskState 統合のエラーパスにテストが不足しています。
特に、クリーンアップ失敗時に起動をブロックする実装（openDiskState でエラーリターン）の正しさを保証するテストの追加を推奨します。

[claude]

[Concurrency] レビュー結果

対象PR: #498 raftengine/etcd: clean up orphaned snapshot spool files on startup

検討項目

1. クリーンアップとアクティブなスプールファイルの競合

cleanupStaleSnapshotSpools は openDiskState の先頭で呼ばれる（WAL オープン前）。各グループは groupDataDir で独立したディレクトリを持つため、クリーンアップ対象ディレクトリを別のエンジンが同時に使用している状況は発生しない。問題なし。

2. TOCTOU (Glob → Remove 間の競合)

filepath.Glob でリストアップした後、別プロセスがファイルを先に削除する可能性がある。これは os.IsNotExist(removeErr) で正しくハンドリングされており、ファイルが消えていてもエラーにならない。問題なし。

3. errors.WithStack(combined) が nil の場合

combined が nil のまま返る場合、errors.WithStack(nil) は nil を返す（cockroachdb/errors の仕様通り）。問題なし。

4. エンジン再起動中の snapshot 操作との競合

openDiskState はエンジン初期化パスであり、この時点ではスナップショット処理を行う goroutine はまだ存在しない。問題なし。

結論

問題なし。クリーンアップロジックは startup シーケンスに正しく配置されており、想定される競合ケースも適切にハンドリングされている。

[claude]

[Test Coverage] レビュー結果

概要

新規追加された2つのユーティリティ関数 (, ) のユニットテストは網羅性が高く、基本的な品質は良好です。ただし、以下の 4件の未カバー箇所 を検出しました。

---

✅ カバーされているケース

テスト	対象
TestCleanupStaleSnapshotSpools	正常系 (5件のスプールファイルを削除、無関係ファイルを保護)
TestCleanupStaleSnapshotSpoolsEmptyDir	空ディレクトリ
TestPurgeOldSnapFiles	上限超過 (6件→3件に削減、境界値の検証あり)
TestPurgeOldSnapFilesUnderLimit	上限以下 (削除なし)
TestPurgeOldSnapFilesEmptyDir	空ディレクトリ

---

⚠️ 不足しているテストケース

1. の挙動変更 (重要度: 高)

engine.go の switch 文リファクタリングにより、 / / のケースでも が呼ばれるようになりました。変更前はこれらのケースで即 していました。

提案テストケース:

// persistLocalSnapshotPayload が ErrCompacted を返した場合でも
// purgeOldSnapFiles が呼ばれ、エラーが正しく伝播することを確認
func TestPersistLocalSnapshotPurgesOnErrCompacted(t *testing.T) { ... }

2. / でのエラー伝播 (重要度: 中)

がエラーを返した場合、 のメソッドがそのエラーを呼び出し元に返すことを確認するテストがありません。ファイル削除失敗がスナップショット処理全体の失敗として扱われることの検証が欠けています。

提案テストケース:

// purgeOldSnapFiles の対象ディレクトリが存在しない場合に
// persistCreatedSnapshot がエラーを返すことを確認
func TestPersistCreatedSnapshotPropagatesPurgeError(t *testing.T) { ... }

3. の起動失敗パス (重要度: 中)

がエラーを返したとき、 がエラーを返してエンジン起動をブロックすることを確認するテストがありません。

提案テストケース:

// スプールファイルが削除不可能な場合 (権限エラー等) に
// openDiskState がエラーを返すことを確認
func TestOpenDiskStateFailsWhenCleanupErrors(t *testing.T) { ... }

4. の非存在ディレクトリ (重要度: 低)

は存在しないディレクトリに対してエラーを返します。このエラーパスが未テストです (空ディレクトリとは異なるケース)。

提案テストケース:

func TestPurgeOldSnapFilesNonExistentDir(t *testing.T) {
    err := purgeOldSnapFiles("/nonexistent/path", defaultMaxSnapFiles)
    require.Error(t, err)
}

---

まとめ

最も重要な指摘は #1 です。 等のエラーケースでも が実行されるという挙動変更は、意図的なものであればテストで明示すべきです。意図的でない場合、 のケース (すなわち 分岐) のみで purge するよう修正が必要です。

[claude]

[Data Loss] レビュー結果

要約

データロストに繋がる可能性のある問題を 2件 検出しました。

---

【中リスク】① でのエラー伝播 (engine.go:1322-1325)

問題点:
と が両方成功した時点で、スナップショットは既にディスクに永続化されています。その後の (古いファイルの削除という非本質的なクリーンアップ操作) が失敗した場合、スナップショット永続化が失敗したかのようにエラーを返します。

呼び出し元 (engine.go:1263) がこのエラーを受け取ると、エンジンが applied index を進めない可能性があり、既にスナップショット化されたエントリを再適用しようとする状態に陥るリスクがあります。

推奨対処:
purge の失敗はログ警告にとどめ、スナップショット永続化の成否には影響させないようにする。

---

【中リスク】② で 時にも purge を実行 (engine.go:2268-2281)

問題点:
元のコードでは はストレージが利用不能な状態を示しており、即座に していました。新しいコードでは の場合でも を実行します。

ストレージが不安定な状態で古いスナップショットファイルを削除すると、その不安定さが解消されてリカバリが必要になった際に、リカバリに使えるはずだった ファイルが既に削除済みという状況が発生する可能性があります。

推奨対処:
の場合は purge をスキップする。あるいは少なくとも purge 失敗をエラーとして返さない。

---

問題なし: その他の変更点

変更箇所	評価
をスタートアップ時に呼び出し	安全。パターン はコミット済みの ファイルと完全に別であり、エンジン起動前に実行されるため競合なし
	etcd 標準の慣行に沿っており、WAL が差分をカバーするため問題なし
のソートロジック	16進数ゼロパディングによる辞書順=時系列順は正しい

[claude]

[Performance] レビュー結果

purgeOldSnapFiles の冗長なソート (wal_store.go)

sort.Strings(snaps)

os.ReadDir はファイル名の辞書順でエントリを返すことが Go の仕様で保証されています。.snap ファイルだけをフィルタリングしても並び順は保持されるため、この sort.Strings は不要な O(N log N) 処理です。削除可能です。

---

var snaps []string の cap 未指定 (wal_store.go)

var snaps []string
for _, e := range entries {
    if !e.IsDir() && filepath.Ext(e.Name()) == ".snap" {
        snaps = append(snaps, e.Name())
    }
}

スナップショット数は通常少数かつ len(entries) で上界が既知なので、make([]string, 0, len(entries)) で初期容量を指定するとリアロケーションを防げます。件数は小さいため影響は軽微ですが、指摘として記載します。

---

以上の 2 点以外に性能上の問題は見当たりません。スナップショット操作はホットパスではないため、purgeOldSnapFiles の呼び出し自体（ディレクトリ読み取り＋ファイル削除）によるレイテンシ増加は許容範囲と判断します。

[claude]

[Data Loss] レビュー結果

データロストに繋がる可能性のある問題を 2件 検出しました。

---

【中リスク】① persistCreatedSnapshot でのエラー伝播 (engine.go:1322-1325)

問題点:

e.persist.SaveSnap(snap) と e.persist.Release(snap) が両方成功した時点で、スナップショットはディスクに永続化済みです。その後の purgeOldSnapFiles（古いファイル削除という非本質的なクリーンアップ）が失敗した場合、スナップショット永続化が失敗したかのようにエラーを返します。

呼び出し元 (engine.go:1263) がこのエラーを受け取ると、エンジンが applied index を進めない可能性があり、既にスナップショット化されたエントリを再適用しようとするリスクがあります。

推奨対処:

purge の失敗はログ警告にとどめ、スナップショット永続化の成否に影響させないようにする。

if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {
    e.logger.Warn("failed to purge old snap files", zap.Error(purgeErr))
    // エラーは返さない
}
return nil

---

【中リスク】② persistLocalSnapshotPayload で ErrUnavailable 時にも purge を実行 (engine.go:2270-2281)

問題点:

元のコードでは ErrUnavailable（ストレージ利用不能）の場合は即座に return nil していました。新コードではこのケースでも purgeOldSnapFiles が実行されます。

ストレージが不安定な状態で古いスナップショットファイルを削除すると、その後リカバリが必要になった際に、リカバリに使えるはずだった .snap ファイルが既に削除済みという状況が発生する可能性があります。

推奨対処:

正常系（err == nil）のみ purge を実行し、ErrUnavailable などのソフトエラー時はスキップする。

switch {
case err == nil:
    if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {
        e.logger.Warn("failed to purge old snap files", zap.Error(purgeErr))
    }
case errors.Is(err, etcdraft.ErrCompacted):
case errors.Is(err, etcdraft.ErrUnavailable):
case errors.Is(err, etcdraft.ErrSnapOutOfDate):
default:
    return err
}
return nil

---

問題なし: その他の変更点

変更箇所	評価
cleanupStaleSnapshotSpools をスタートアップ時に呼び出し	安全。パターン elastickv-etcd-snapshot-* はコミット済みの .snap ファイルと完全に別であり、エンジン起動前に実行されるため競合なし
defaultMaxSnapFiles = 3	etcd 標準の慣行に沿っており、WAL が差分をカバーするため問題なし
purgeOldSnapFiles のソートロジック	16進数ゼロパディングによる辞書順=時系列順は正しい

[claude]

[Performance] レビュー結果

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

---

purgeOldSnapFiles のスナップショット書き込みパスへの組み込み

persistCreatedSnapshot および persistLocalSnapshotPayload から毎回 purgeOldSnapFiles を呼び出している。この関数は内部で os.ReadDir によるディレクトリ全体の読み取りと sort.Strings を実行する。

// engine.go:1320
snapDir := filepath.Join(e.dataDir, snapDirName)
if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {

スナップショット操作はもともとディスクI/Oが重い処理であり、defaultMaxSnapFiles = 3 の場合、実際にソートする要素数は高々 defaultMaxSnapFiles + 1 程度（これを超えたら削除されるため）。ディレクトリエントリ数も少なく、sort.Strings の O(N log N) コストは無視できる水準であるため実害は小さい。

ただし、ディレクトリ読み取りのシステムコールがスナップショットのコミットレイテンシに加算される点は留意。スナップショット頻度が非常に高い環境（バルクリストア等）では問題になり得る。

cleanupStaleSnapshotSpools の起動時一括実行

起動時に一度だけ呼ばれる処理であり、filepath.Glob + 個別 os.Remove という構成も件数が少ない前提では問題なし。

---

総評: 性能上の重大な問題はなし。purgeOldSnapFiles のコストはスナップショット自体のI/Oコストに対して無視できる水準。

[claude]

[Test Coverage] レビュー結果

対象PR: raftengine/etcd: clean up orphaned snapshot spool files on startup

新規テストファイル (snapshot_spool_test.go) は基本ケースを十分にカバーしていますが、以下の不足点があります。

---

問題1: persistLocalSnapshotPayload のスイッチ文リファクタリングにテストなし【重要】

変更点（engine.go）:

// Before: ErrCompacted 等の場合は purge せずに return nil
case errors.Is(err, etcdraft.ErrCompacted):
    return nil

// After: 空ケースになり、purgeOldSnapFiles が実行される
case errors.Is(err, etcdraft.ErrCompacted):
// → fall-through to purge

ErrCompacted・ErrUnavailable・ErrSnapOutOfDate のとき、以前は purge をスキップして即 nil を返していましたが、変更後は purge が実行されます。この動作変更に対応するテストが存在しません。

提案テストケース:

func TestPersistLocalSnapshotPayload_ErrCompacted_StillPurges(t *testing.T) {
    // persistLocalSnapshotPayload が ErrCompacted を返す状況をセットアップし、
    // その後 purgeOldSnapFiles が呼ばれてファイルが削除されることを確認する
}

---

問題2: openDiskState からの cleanup 呼び出しに統合テストなし【重要】

このPRの主目的（「エンジン再起動時にスプールファイルを自動削除する」）は openDiskState() への呼び出し追加によって実現されています。しかし engine_test.go / persistence_test.go に 再起動シナリオの統合テストが存在しません。

提案テストケース:

func TestOpenDiskState_CleansUpStaleSpools(t *testing.T) {
    dir := t.TempDir()
    // DataDir に orphan なスプールファイルを事前配置
    for i := 0; i < 3; i++ {
        f, _ := os.CreateTemp(dir, snapshotSpoolPattern)
        f.Close()
    }
    // openDiskState を呼び出す
    _, err := openDiskState(OpenConfig{DataDir: dir, ...}, nil)
    require.NoError(t, err)
    // スプールファイルが消えていることを確認
    matches, _ := filepath.Glob(filepath.Join(dir, snapshotSpoolPattern))
    require.Empty(t, matches)
}

---

問題3: purgeOldSnapFiles に非存在ディレクトリのテストなし

cleanupStaleSnapshotSpools は filepath.Glob を使うため非存在ディレクトリでもエラーを返しません（テスト済み）。一方 purgeOldSnapFiles は os.ReadDir を使うため、存在しないディレクトリを渡すとエラーになります。エンジン起動直後（snap/ ディレクトリが未作成）のケースが考慮されていません。

提案テストケース:

func TestPurgeOldSnapFilesNonExistentDir(t *testing.T) {
    err := purgeOldSnapFiles(filepath.Join(t.TempDir(), "no-such-snap-dir"), defaultMaxSnapFiles)
    // os.ReadDir はエラーを返す — これは許容すべきか、または無視すべきか？
    // 現実装は errors.WithStack(err) を返すため、呼び出し側でのハンドリングを確認すること
}

---

問題4: cleanupStaleSnapshotSpools の部分削除エラー合成ロジックが未テスト

エラーが発生しても残りのファイルの削除を継続し、errors.CombineErrors で集約するロジックがありますが、テストされていません。

提案テストケース:

func TestCleanupStaleSnapshotSpools_PartialFailure(t *testing.T) {
    dir := t.TempDir()
    // 削除不可ファイル（chmod 000）を混在させる
    // エラーが返り、かつ削除可能なファイルは削除されることを確認
}

---

問題5（軽微）: purgeOldSnapFiles の複数ターム混在ソートが未テスト

現テストはすべて term=1 のみ。term=2, index=50 と term=1, index=100 の混在時に、16進ゼロパディングで正しくソートされることを確認するテストがあると安心です。

---

総評

基本的なハッピーパス・空ディレクトリ・境界値（ファイル数が閾値以下）は網羅されています。
問題1・2は動作仕様に直結する変更のためテスト追加を推奨します。問題3はエッジケースですが、本番環境で初回起動時に snap ディレクトリが存在しない場合に影響する可能性があります。

[claude]

[Test Coverage] レビュー結果

対象PR: raftengine/etcd: clean up orphaned snapshot spool files on startup

変更ファイル: engine.go, snapshot_spool.go, snapshot_spool_test.go, wal_store.go

---

追加されたテストの評価

新規テストファイル snapshot_spool_test.go (112行) で以下がカバーされています:

テスト	内容
TestCleanupStaleSnapshotSpools	✅ 正常系: 5ファイル削除・無関係ファイルの保持
TestCleanupStaleSnapshotSpoolsEmptyDir	✅ 空ディレクトリ
TestCleanupStaleSnapshotSpoolsNonExistentDir	✅ 存在しないディレクトリ
TestPurgeOldSnapFiles	✅ 正常系: 6ファイル中古い3件を削除
TestPurgeOldSnapFilesUnderLimit	✅ 上限以下のファイル数
TestPurgeOldSnapFilesEmptyDir	✅ 空ディレクトリ

---

不足しているテストケース

1. purgeOldSnapFiles — 境界値テストの欠如

TestPurgeOldSnapFilesUnderLimit はファイル数が 2(上限3未満)のケースのみ。ちょうど maxSnap と等しいファイル数(3件)のケースが未テスト。

// 追加すべきテストケース
func TestPurgeOldSnapFilesAtLimit(t *testing.T) {
    dir := t.TempDir()
    // ちょうど defaultMaxSnapFiles=3 件のファイルを作成
    for i := uint64(1); i <= 3; i++ {
        createSnapFile(t, dir, 1, i*10000)
    }
    require.NoError(t, purgeOldSnapFiles(dir, defaultMaxSnapFiles))
    entries, err := os.ReadDir(dir)
    require.NoError(t, err)
    require.Len(t, entries, 3) // 何も削除されないこと
}

2. purgeOldSnapFiles — 存在しないディレクトリ時の挙動未テスト

cleanupStaleSnapshotSpools は filepath.Glob を使用するため、存在しないディレクトリでもエラーなし。一方、purgeOldSnapFiles は os.ReadDir を使用するため、存在しないディレクトリ渡しで エラーが返る。TestCleanupStaleSnapshotSpoolsNonExistentDir に相当するテストが purgeOldSnapFiles には存在しない。

func TestPurgeOldSnapFilesNonExistentDir(t *testing.T) {
    // 存在しないディレクトリを渡した場合の挙動を明示
    err := purgeOldSnapFiles(filepath.Join(t.TempDir(), "no-such-dir"), defaultMaxSnapFiles)
    require.Error(t, err) // or require.NoError — 仕様を明確化する
}

3. persistLocalSnapshotPayload のロジック変更が未テスト

engine.go の persistLocalSnapshotPayload は switch 文が大きくリファクタリングされ、ErrCompacted / ErrUnavailable / ErrSnapOutOfDate のケースで以前は即 return nil していたのに対し、変更後は purgeOldSnapFiles を呼び出した後に return nil するよう変わっている。

この変更により、これらのエラーケースで purgeOldSnapFiles が失敗すると、以前は無視されていたエラーが上位に伝播するようになる。既存の engine_test.go / persistence_test.go にはこのコードパスを明示的にカバーするテストが見当たらない。

提案:

// persistLocalSnapshotPayload が ErrCompacted 等を受けたとき
// purgeOldSnapFiles のエラーが正しく伝播するかを確認するユニットテスト

4. cleanupStaleSnapshotSpools の部分失敗 (errors.CombineErrors) が未テスト

複数のファイルのうち一部の削除が失敗する(例: パーミッションエラー)ケースで、errors.CombineErrors によるエラー集約が正しく動作するかのテストがない。正常系・全削除成功のみ検証されている。

---

分散環境・並行アクセス観点

cleanupStaleSnapshotSpools と purgeOldSnapFiles は単一エンジンの起動シーケンス (openDiskState) 内での呼び出しであり、並行アクセスは設計上想定外。この観点での追加テストは不要と判断。

---

総評

ユニットテストとして基本的なパスは網羅されており品質は良好。上記の4点のうち、特に 境界値テスト(issue 1) と persistLocalSnapshotPayload の動作変更(issue 3) は回帰リスクがあるため対応を推奨します。

[claude]

[Consistency] レビュー結果

対象PR: #498 raftengine/etcd: clean up orphaned snapshot spool files on startup

問題なし。

スナップショットスプールファイルの清掃とスナップファイルのパージはいずれも Raft コンセンサス状態の外で行われる補助的な操作であり、データ一貫性への影響はない。

• cleanupStaleSnapshotSpools は WAL ロードより前の起動時に実行されるため、コンセンサス処理への介入なし
• purgeOldSnapFiles のソート順（ゼロパディング16進数による辞書順＝term優先→index順）は etcd の命名規約と一致しており、「最新を残す」セマンティクスが正しく実現されている
• persistLocalSnapshotPayload のswitch文変更（各 case の return nil を削除してパージ処理を共通化）は、いずれの case も成功扱いでありパージ失敗以外は nil を返す挙動が維持されている
• すべての変更はローカルファイルI/Oのみで、Raft 経由すべき操作のバイパスは一切ない

[claude]

[Performance] レビュー結果

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

対象ファイル: internal/raftengine/etcd/wal_store.go, snapshot_spool.go

---

指摘事項

[軽微] purgeOldSnapFiles をスナップショット書き込みパスで同期実行している

// engine.go:1318
snapDir := filepath.Join(e.dataDir, snapDirName)
if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {

persistCreatedSnapshot と persistLocalSnapshotPayload の両方から呼ばれており、スナップショット生成のたびに os.ReadDir(snapDir) (ディレクトリ列挙) + os.Remove がホットパスに追加される。スナップショットパスはすでに I/O ヘビーなため、相対的な影響は小さいが、非同期化または goroutine での実行も検討できる。

[極小] snaps スライスのキャパシティ未指定

var snaps []string
for _, e := range entries {
    if !e.IsDir() && filepath.Ext(e.Name()) == ".snap" {
        snaps = append(snaps, e.Name())
    }
}

entries の長さが上限として分かっているので make([]string, 0, len(entries)) と事前確保できる。ただし snap ファイル数は常に数件程度のため実用上の影響はほぼない。

---

総評: 機能的にはスナップショット書き込みパスへの追加 I/O が懸念されるが、スナップショット生成自体が低頻度かつすでに I/O ヘビーな操作であるため、実用上の性能影響は限定的。問題は軽微レベル。

[claude]

[Concurrency] レビュー結果

対象PR: raftengine/etcd: clean up orphaned snapshot spool files on startup

総評

並行性・分散障害の観点において、実装に問題となる点はありません。以下に各観点の詳細を記します。

---

1. による相互排他 — ✅ 問題なし

新たに追加された の呼び出し元はすべて を保持済みです。

呼び出し経路	ロック取得箇所
（メソッド）→
→ → →
→ → →

両関数が同時にパージを実行する競合は発生しません。また、 チェックにより、他ゴルーチンによる先行削除も安全に吸収されます。

---

2. Raft リーダー→フォロワー スナップショット転送との競合 — ✅ 問題なし

ファイルが読み込まれるのは起動時の / のみです。通常運用時のリーダー→フォロワー転送では （メモリ上の値）が使われるため、 が稼働中に ファイルを削除してもスナップショット転送には影響しません。

---

3. switch 文リファクタリングの正確性 — ✅ 問題なし

これらのソフトエラーは または から発生し、 が成功する前に返るケースが大半です。いずれも「新しい ファイルを書いていない状態でのパージ」であり、既存ファイルを最新 3 件に整理するだけなので副作用はありません。

---

4. のタイミング — ✅ 問題なし

（起動時・単一スレッド）内でエンジンゴルーチン開始前に呼び出されており、並行アクセスは発生しません。スプールファイル（）と ファイルは完全に別ものであり、相互干渉もありません。

---

5. vs のエラーハンドリング差異 — ℹ️ 参考情報

• : は存在しないディレクトリに対して を返すため、ディレクトリ不在でも正常終了します。
• : は存在しないディレクトリに対してエラーを返します。ただし は 内の で必ず作成されるため、実運用上の問題は生じません。

---

以上。並行性・分散障害の観点で 問題となる点は検出されませんでした。

[claude]

[Data Loss] レビュー結果

対象PR: raftengine/etcd: clean up orphaned snapshot spool files on startup

---

調査範囲

以下の変更点をデータロスト観点で精査しました。

1. — 起動時の孤立スプールファイル削除
2. — スナップ永続化後の古い .snap ファイル削除
3. のフロー変更（エラーsilence後のpurge実行）

---

指摘事項

[中] スナップ作成失敗（エラーsilence）後にpurgeが実行される

場所: engine.go — Engine.persistLocalSnapshotPayload

問題の詳細:

は の が返します。この時点では （同332行より前）は成功しており、対象indexの .snap ファイルがディスクに作成されています。しかし は呼ばれていません（persistLocalSnapshotPayloadpurgeOldSnapFilesErrSnapOutOfDatepurgeOldSnapFilesdefaultMaxSnapFiles=3persist.SaveSnapReleaseEngine.persistLocalSnapshotPayload

と が成功した後でも、 がエラーを返すと呼び出し元にエラーが伝搬されます。スナップ自体はディスクに正しく永続化されているため、データロストは発生しません。しかし呼び出し元がこのエラーをスナップ永続化失敗として扱いリトライを発生させた場合、無駄な再スナップが試みられます。

---

[問題なし] 起動時のスプールファイル削除

は WAL や .snap ファイルではなく、前回クラッシュで残った一時的なスプールファイル（アセンブル途中の未コミットデータ）を削除します。これらはいずれの永続化ストレージにも反映前のファイルであり、削除してもデータロストにはなりません。

[問題なし] のソート順と削除対象

スナップファイル名のゼロパディングhex形式によりlexicographicソート＝chronologicalソートが成立しており、最古から順に削除・最新3件を保持する実装は正しいです。WALリカバリに必要なスナップは最新1件あれば足りるため、3件保持は十分なマージンです。

---

総評

重大なデータロストリスクは確認されませんでした。

中程度の懸念として、 などのsilenceされたエラー後にpurgeが実行されるケースで、リリースされていないスナップファイルがpurge対象になり得る点があります。ただしそのファイルは無効なスナップ（古いindex）であり、有効な最新スナップは の保護により削除されません。

実害として顕在化する可能性が低い指摘ですが、 のケースではpurgeを実行しない（または呼び出し元で明示的にスキップする）設計にするとより安全です。

[claude]

[Data Loss] レビュー結果

対象PR: #498 「raftengine/etcd: clean up orphaned snapshot spool files on startup」

データロスの観点から差分をレビューした結果、致命的なデータロスリスクは検出されませんでした。ただし、以下の注意点があります。

---

注意点1: persistLocalSnapshotPayload のスイッチ文リファクタリングによる動作変更

該当箇所: engine.go の persistLocalSnapshotPayload メソッド

変更前: ErrCompacted / ErrUnavailable / ErrSnapOutOfDate のいずれかが返された場合、即 nil を返して終了。
変更後: それらのエラーでも purgeOldSnapFiles が呼ばれるようになった。

// 変更後 (抜粋)
switch {
case err == nil:
case errors.Is(err, etcdraft.ErrCompacted):
case errors.Is(err, etcdraft.ErrUnavailable):
case errors.Is(err, etcdraft.ErrSnapOutOfDate):
default:
    return err
}
// ← エラーケースでもここに落ちてpurgeOldSnapFilesが呼ばれる

ErrCompacted/ErrUnavailable は内部の persistLocalSnapshotPayload (inner関数) 内で buildLocalSnapshot が失敗した場合にも発生し得ます。その場合 persist.SaveSnap は呼ばれていない（新しいスナップファイルが存在しない）にもかかわらず、purgeOldSnapFiles が古いスナップファイルを削除します。

影響度: 低。purgeOldSnapFiles は常に最新 defaultMaxSnapFiles=3 ファイルを保持するため、有効な回復ポイントは必ず残ります。データロスにはなりませんが、意図的な動作変更かどうか確認が必要です。

---

注意点2: cleanupStaleSnapshotSpools のクリーンアップ対象ディレクトリ

該当箇所: wal_store.go:42 および snapshot_spool.go:70

// openDiskState 内
if err := cleanupStaleSnapshotSpools(cfg.DataDir); err != nil { ... }

• GRPCTransport のスプールは cfg.DataDir (engine.go:339: e.transport.SetSpoolDir(cfg.DataDir)) に作成 → クリーンアップされる ✓
• bootstrapWalState 内の stateMachineSnapshotBytes(fsm, snapDir) は snapDir（cfg.DataDir/snap）にスプールを作成 → クリーンアップされない △

ブートストラップ中のクラッシュで cfg.DataDir/snap に elastickv-etcd-snapshot-* が残った場合、本PRのクリーンアップでは回収されません。ただし snapshotBytesAndClose は正常完了後にスプールを削除するため、通常運用時は問題なし。本PR目的のメインパス（GRPCTransport経由の受信スナップショット）はカバーされています。データロスではなくディスクリークの軽微な未対処ケース。

---

安全性が確認された点

• persistCreatedSnapshot での順序: SaveSnap → Release → purgeOldSnapFiles の順序は正しく、新しいスナップが永続化された後に古いスナップを削除している ✓
• defaultMaxSnapFiles=3 は etcd デフォルト (--max-snapshots=5 より保守的だが妥当) ✓
• スプールファイルはコミット前の一時データであり、起動時に削除しても永続化されたRaftログ・スナップには影響しない ✓
• cleanupStaleSnapshotSpools がエラーを返した場合、openDiskState がフェイルセーフに失敗する（起動阻止）ことで不整合状態での動作を防いでいる ✓

[claude]

[Concurrency] レビュー結果

対象PR: raftengine/etcd: clean up orphaned snapshot spool files on startup

総評

並行性・分散障害の観点において、実装に問題となる点はありません。以下に各観点の詳細を記します。

---

1. snapshotMu による相互排他 — ✅ 問題なし

新たに追加された purgeOldSnapFiles の呼び出し元はすべて e.snapshotMu を保持済みです。

呼び出し経路	ロック取得箇所
persistLocalSnapshotPayload（メソッド）→ purgeOldSnapFiles	engine.go:2257-2258
persistConfigState → persistConfigSnapshotPayloadLocked → persistCreatedSnapshot → purgeOldSnapFiles	engine.go:1215-1216
persistConfigSnapshotPayload → persistConfigSnapshotPayloadLocked → persistCreatedSnapshot → purgeOldSnapFiles	engine.go:1246-1247

両関数が同時にパージを実行する競合は発生しません。また、os.IsNotExist チェックにより、他ゴルーチンによる先行削除も安全に吸収されます。

---

2. Raft リーダー→フォロワー スナップショット転送との競合 — ✅ 問題なし

.snap ファイルが読み込まれるのは起動時の openDiskState / loadWalState のみです。通常運用時のリーダー→フォロワー転送では raftpb.Snapshot.Data（メモリ上の値）が使われるため、purgeOldSnapFiles が稼働中に .snap ファイルを削除してもスナップショット転送には影響しません。

---

3. switch 文リファクタリングの正確性 — ✅ 問題なし

// Before: ErrCompacted / ErrUnavailable / ErrSnapOutOfDate は即 return nil
// After:  これらも fall-through してパージを実行
switch {
case err == nil:
case errors.Is(err, etcdraft.ErrCompacted):
case errors.Is(err, etcdraft.ErrUnavailable):
case errors.Is(err, etcdraft.ErrSnapOutOfDate):
default:
    return err
}
purgeOldSnapFiles(...)

これらのソフトエラーは buildLocalSnapshot または storage.CreateSnapshot から発生し、SaveSnap が成功する前に返るケースが大半です。いずれも「新しい .snap ファイルを書いていない状態でのパージ」であり、既存ファイルを最新 3 件に整理するだけなので副作用はありません。

---

4. cleanupStaleSnapshotSpools のタイミング — ✅ 問題なし

openDiskState（起動時・単一スレッド）内でエンジンゴルーチン開始前に呼び出されており、並行アクセスは発生しません。スプールファイル（elastickv-etcd-snapshot-*）と .snap ファイルは完全に別ものであり、相互干渉もありません。

---

5. filepath.Glob vs os.ReadDir のエラーハンドリング差異 — ℹ️ 参考情報

• cleanupStaleSnapshotSpools: filepath.Glob は存在しないディレクトリに対して nil, nil を返すため、ディレクトリ不在でも正常終了します。
• purgeOldSnapFiles: os.ReadDir は存在しないディレクトリに対してエラーを返します。ただし snapDir は openDiskState 内の os.MkdirAll で必ず作成されるため、実運用上の問題は生じません。

---

以上。並行性・分散障害の観点で 問題となる点は検出されませんでした。

[claude]

[Performance] レビュー結果

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

問題1: スナップショット永続化パスへの同期ディスクI/O追加

persistCreatedSnapshot と persistLocalSnapshotPayload の両方で purgeOldSnapFiles() を同期的に呼び出している。

// engine.go:1322
snapDir := filepath.Join(e.dataDir, snapDirName)
if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {
    return errors.Wrap(purgeErr, "purge old snap files")
}

purgeOldSnapFiles は内部で os.ReadDir(snapDir)（ディレクトリ全体の読み出し）+ sort.Strings()（O(N log N)）+ os.Remove()（ファイル削除）を実行する。スナップショット永続化自体は高頻度ではないが、毎回の呼び出しに不必要なディレクトリ走査が追加される。

スナップショット件数が少ない（defaultMaxSnapFiles = 3）ため実害は軽微だが、非同期（goroutine）で実行するか、カウンタで「maxSnap を超えたときだけ実行」に限定することで避けられる。

問題2: ソフトエラーケース（ErrCompacted等）でも purge を実行

persistLocalSnapshotPayload の switch 文を fall-through 形式に変更したことで、ErrCompacted / ErrUnavailable / ErrSnapOutOfDate のいずれのケースでも purge が実行されるようになった。これらのケースではスナップショットは実際には書かれていないため、purge の呼び出しは不要なディスクI/Oとなる。

startup の cleanupStaleSnapshotSpools については問題なし（起動時の一度限りの処理）。

[claude]

[Concurrency] レビュー結果

対象ファイル: internal/raftengine/etcd/engine.go, snapshot_spool.go, wal_store.go

問題なし。

確認事項:

• purgeOldSnapFiles は persistCreatedSnapshot（→ persistConfigSnapshotPayloadLocked 経由）および persistLocalSnapshotPayload の両方から呼び出されるが、いずれも snapshotMu.Lock() を保持した状態で実行されるため、同時ファイル削除のレースは発生しない。
• cleanupStaleSnapshotSpools は openDiskState（起動時のシングルスレッド）から呼ばれており、並行性の懸念なし。
• ファイル削除時に os.IsNotExist を適切にガードしており、TOCTOU（確認後削除前の競合）は安全に処理されている。
• goroutineリークやチャネルブロッキングの追加なし。
• Raftリーダー切り替え時も snapshotMu でシリアライズされており、スナップショット操作の順序は保たれる。

[claude]

[Concurrency] レビュー結果

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

---

1. purgeOldSnapFiles はRaft readyループ内の同期I/O操作（低リスク）

箇所: engine.go — persistCreatedSnapshot / persistLocalSnapshotPayload

purgeOldSnapFiles はRaft readyループから同期的に呼ばれる。ディスクが遅い場合やスナップショットファイルが多数存在する場合、Raftのready処理を遅延させる可能性がある。ただし、エラー時は slog.Warn に留めており（returnしない）、readyループをブロックしないため実害は限定的。

2. cleanupStaleSnapshotSpools の起動タイミング（問題なし）

openDiskState から呼ばれるため、エンジン起動の単一スレッド文脈で実行される。並行アクセスの問題はない。

3. ファイル削除の並行安全性（問題なし）

purgeOldSnapFiles と cleanupStaleSnapshotSpools の両方で os.IsNotExist を正しく処理しており、並行削除による2重削除エラーは吸収される。

4. スナップショット送信中のファイル削除（問題なし）

最新3件を保持するため、フォロワーへの転送中スナップショット（最新）は削除されない。3世代以上遅れたフォロワーは通常ないため、実用上問題はない。

5. persistLocalSnapshotPayload のswitch再構成（問題なし）

旧コードの各 case に return nil があった部分を、fallthrough後に一括で purge+return する形に変更している。ErrCompacted / ErrUnavailable / ErrSnapOutOfDate の各ケースは引き続き return nil のままで意味的に同等。

---

総評: 並行性・分散障害の観点で重大な問題はない。

[claude]

[Test Coverage] レビュー結果

全体的にテストは丁寧に書かれており、主要なケースはカバーされています。以下、不足している点を報告します。

---

指摘1: purgeOldSnapFiles — 境界値（ちょうど defaultMaxSnapFiles 件）のテストが欠如

重要度: 中

wal_store.go:366 の条件は len(snaps) <= defaultMaxSnapFiles（つまり3件以下なら削除しない）ですが、現在のテストは「2件（上限未満）」と「6件（上限超過）」のみで、ちょうど3件のケースが未テストです。オフバイワンのバグがあっても検出できません。

提案するテストケース:

func TestPurgeOldSnapFilesExactlyAtLimit(t *testing.T) {
    dir := t.TempDir()
    // ちょうど defaultMaxSnapFiles (3) 件 — 削除は発生しないはず
    createSnapFile(t, dir, 1, 1000)
    createSnapFile(t, dir, 1, 2000)
    createSnapFile(t, dir, 1, 3000)

    require.NoError(t, purgeOldSnapFiles(dir))

    entries, err := os.ReadDir(dir)
    require.NoError(t, err)
    require.Len(t, entries, 3)
}

---

指摘2: purgeOldSnapFiles — 存在しないディレクトリに対するエラーハンドリングが未テスト

重要度: 中

cleanupStaleSnapshotSpools は filepath.Glob を使うため、存在しないディレクトリを渡してもエラーにならず（Glob は常に nil を返す）、TestCleanupStaleSnapshotSpoolsNonExistentDir が成立します。

一方、purgeOldSnapFiles は os.ReadDir を使うため、存在しないディレクトリを渡すとエラーが返ります（snapshot_spool.go:71 との非対称性）。この関数は engine.go:1322・engine.go:2271 から filepath.Join(e.dataDir, snapDirName) で呼ばれますが、snap ディレクトリが未作成の状態でも呼ばれうるため、エラーパスのカバレッジが欠けています（現在は警告ログのみで握り潰し）。

提案するテストケース:

func TestPurgeOldSnapFilesNonExistentDir(t *testing.T) {
    err := purgeOldSnapFiles(filepath.Join(t.TempDir(), "no-such-dir"))
    // os.ReadDir はエラーを返す — 現在の実装ではエラーがそのまま返る
    require.Error(t, err)
}

もしくは、cleanupStaleSnapshotSpools と同様に os.IsNotExist を無視する実装にした上でテストを追加してください。

---

指摘3: purgeOldSnapFiles — 複数タームにまたがるファイルの順序付け未テスト

重要度: 低

既存テストは term=1 の単一タームのみです。ファイル名フォーマットは %016x-%016x.snap（term-index）であり、辞書順ソートでタームが先に来るため、term=2, index=1 のファイルは term=1, index=100 より「新しい」と判定されます。これは etcd の Snapshotter の仕様と一致しますが、異なるタームのファイルが混在する場合の挙動がテストされていません。

提案するテストケース:

func TestPurgeOldSnapFilesMultiTerm(t *testing.T) {
    dir := t.TempDir()
    // term=1 の古いファイル群と term=2 の新しいファイルが混在
    createSnapFile(t, dir, 1, 9000)
    createSnapFile(t, dir, 1, 10000)
    createSnapFile(t, dir, 2, 1000) // term が新しい
    createSnapFile(t, dir, 2, 2000)

    require.NoError(t, purgeOldSnapFiles(dir))

    entries, err := os.ReadDir(dir)
    require.NoError(t, err)
    // term=1 のファイルは全削除、term=2 の2件と最新のterm=1が残る想定
    require.Len(t, entries, 3)
}

---

指摘4: cleanupStaleSnapshotSpools — 一部ファイル削除失敗時のエラー結合パスが未テスト

重要度: 低

snapshot_spool.go:79 の errors.CombineErrors による複数エラー結合のロジックがテストされていません。削除権限がないファイルが混在するケースで、残りのファイルが正しく削除されることの確認が欠けています。OS 依存のテストになるため低優先度ですが、可能なら追加を検討してください。

---

まとめ

#	対象関数	欠けているケース	重要度
1	purgeOldSnapFiles	境界値（ちょうど3件）	中
2	purgeOldSnapFiles	存在しないディレクトリ	中
3	purgeOldSnapFiles	複数ターム混在	低
4	cleanupStaleSnapshotSpools	一部削除失敗のエラー結合	低

指摘1・2 は実装バグを隠す可能性があるため、対応を推奨します。

[claude]

[Performance] レビュー結果

[軽微] purgeOldSnapFiles: sort.Strings が冗長

wal_store.go の purgeOldSnapFiles:

sort.Strings(snaps)

os.ReadDir はディレクトリエントリをファイル名の昇順でソートして返す（Go ドキュメント保証）。その後で sort.Strings(snaps) を呼ぶのは二重ソートになっており、不要な O(N log N) 処理が走る。削除して問題なし。

---

[軽微] purgeOldSnapFiles: スライスのキャパシティ未指定

var snaps []string
for _, e := range entries {
    if !e.IsDir() && filepath.Ext(e.Name()) == ".snap" {
        snaps = append(snaps, e.Name())
    }
}

entries の長さが分かっているため make([]string, 0, len(entries)) で事前確保すると再アロケーションを回避できる。スナップ数が多い環境では小さな改善になる。

---

以上2点は軽微。機能的な問題はなく、スナップショット管理はホットパスでもないため影響は限定的。

[claude]

[Test Coverage] レビュー結果

PR #498: raftengine/etcd - orphaned snapshot spool ファイルのクリーンアップ

---

✅ カバーされているテスト

テスト	対象
TestCleanupStaleSnapshotSpools	正常ケース（5ファイル＋無関係ファイル保持）
TestCleanupStaleSnapshotSpoolsEmptyDir	空ディレクトリ
TestCleanupStaleSnapshotSpoolsNonExistentDir	存在しないディレクトリ（filepath.Glob はエラーを返さない）
TestPurgeOldSnapFiles	上限超え (6→3保持)、非snapファイル保持
TestPurgeOldSnapFilesUnderLimit	上限未満 (2ファイル)
TestPurgeOldSnapFilesEmptyDir	空ディレクトリ

---

❌ 不足テスト（3件）

---

1. purgeOldSnapFiles — 存在しないディレクトリでエラーが返ることが未検証

cleanupStaleSnapshotSpools は filepath.Glob を使うため存在しないディレクトリでもエラーなし（TestCleanupStaleSnapshotSpoolsNonExistentDir で確認済み）。しかし purgeOldSnapFiles は os.ReadDir を使うため、存在しないディレクトリではエラーを返す。この挙動が未テスト。

func TestPurgeOldSnapFilesNonExistentDir(t *testing.T) {
    err := purgeOldSnapFiles(filepath.Join(t.TempDir(), "no-such-dir"), defaultMaxSnapFiles)
    require.Error(t, err) // os.ReadDir は ENOENT を返す
}

---

2. purgeOldSnapFiles — maxSnap と同数の境界値が未テスト

現在のテストは「上限超え (6>3)」と「上限未満 (2<3)」のみ。len(snaps) <= maxSnap の等号境界 (ちょうど3ファイル) が確認されていない。

func TestPurgeOldSnapFilesAtLimit(t *testing.T) {
    dir := t.TempDir()
    for i := uint64(1); i <= uint64(defaultMaxSnapFiles); i++ {
        createSnapFile(t, dir, 1, i*10000)
    }
    require.NoError(t, purgeOldSnapFiles(dir, defaultMaxSnapFiles))
    entries, _ := os.ReadDir(dir)
    require.Len(t, entries, defaultMaxSnapFiles) // 削除ゼロ
}

---

3. persistLocalSnapshotPayload — 動作変更のテストが未カバー

engine.go の persistLocalSnapshotPayload を修正した switch 文は、ErrCompacted / ErrUnavailable / ErrSnapOutOfDate 発生時に 以前は即 return nil だったが、修正後は purgeOldSnapFiles を呼ぶようになった。このパスのテストが存在しない。

// 例: ErrCompacted 時にも purge が走ることを確認するユニットテスト
// （または engine_test.go にある既存の統合テストで副作用を検証）

[claude]

[Data Loss] レビュー結果

問題なし

確認した観点と判断根拠

1. スナップショットスプールファイルの削除 ()

• スプールファイル () は、スナップショットペイロードを組み立てる際の一時ファイルであり、実際の永続データ ( ファイル) ではない
• クラッシュ前に が実行された場合: ファイルは既に削除済み (残存なし)
• クラッシュ後にスプールが残存する場合: スナップショットは ファイルに保存される前にクラッシュしているため、スプールの内容はコミットされていない未永続データ → 削除しても問題なし
• の呼び出しタイミング (起動時・新しいスプール作成前) なのでレース条件もない

2. 古い ファイルの削除 ()

• : → が成功した後にパージを呼び出している。新しいスナップショットが既にディスクに保存されてから古いものを削除する順序が正しい
• : → → が成功した後 ( のケースのみ) にパージを呼び出している
• 保持数 は etcd 標準の値と一致
• ゼロパディングされた16桁16進数のファイル名は辞書順 = 時系列順となるため、ソートロジックは正しい
• パージ失敗時は でログを残して処理を継続。スナップショット自体は既に保存済みのため、データロストにはならない

3. パターンマッチの安全性

• は WAL ファイル () や ファイルとは完全に異なるパターンであり、誤削除のリスクはない

[claude]

[Concurrency] レビュー結果

対象PR: #498 raftengine/etcd: clean up orphaned snapshot spool files on startup

---

1. purgeOldSnapFiles の並行呼び出し — 問題なし

purgeOldSnapFiles は以下の2経路から呼ばれる。

• persistCreatedSnapshot → persistConfigSnapshotPayloadLocked 経由で常に e.snapshotMu 保持下で呼ばれる
• persistLocalSnapshotPayload（メソッド）→ 自身が e.snapshotMu.Lock() を取得してから呼ぶ

両者は e.snapshotMu によって直列化されており、同時呼び出しによる競合は発生しない。

---

2. purgeOldSnapFiles のエラー非成功ケース呼び出し — 軽微な指摘

persistLocalSnapshotPayload（メソッド）の switch 文リファクタ後、ErrCompacted / ErrUnavailable / ErrSnapOutOfDate の場合も fall-through して purgeOldSnapFiles が呼ばれるようになった。

switch {
case err == nil:
case errors.Is(err, etcdraft.ErrCompacted):
case errors.Is(err, etcdraft.ErrUnavailable):
case errors.Is(err, etcdraft.ErrSnapOutOfDate):
default:
    return err
}
// ↑ いずれのケースでも purgeOldSnapFiles が実行される

ErrCompacted や ErrSnapOutOfDate は buildLocalSnapshot の失敗（persist.SaveSnap 実行前）から返ることがあるため、新規 .snap ファイルが書かれていないにもかかわらず削除処理が走る。
ただし purgeOldSnapFiles は常に最新 maxSnap 件を残すため 安全上の問題はない。意図的であればコメントを添えると読み手に親切。

---

3. cleanupStaleSnapshotSpools の起動時呼び出し — 問題なし

openDiskState 内の cleanupStaleSnapshotSpools(cfg.DataDir) 呼び出しは、Raft エンジンのゴルーチンが一切起動する前の初期化フェーズにある。スプールファイルへの同時アクセスは発生しない。

---

4. ノード障害・リーダー切り替え時の挙動 — 問題なし

purgeOldSnapFiles / cleanupStaleSnapshotSpools はすべてローカルファイル操作であり、Raft 状態機械とは独立している。クラッシュ直後の再起動時に openDiskState から cleanupStaleSnapshotSpools が呼ばれる設計は冪等かつ安全。

---

5. ゴルーチンリーク・チャネルブロッキング — 問題なし

新規ゴルーチン・チャネルの追加はなく、該当リスクはない。

---

総評

重大な並行性・分散障害の問題は検出されなかった。指摘 (2) は安全上の欠陥ではなく、セマンティクスのわかりやすさに関するもの。

[claude]

[Data Loss] レビュー結果

対象PR: #498 (snapshot spool cleanup)

---

1. persistCreatedSnapshot でのパージ失敗時の誤ったエラー伝播

場所: internal/raftengine/etcd/engine.go — persistCreatedSnapshot()

if err := e.persist.Release(snap); err != nil {   // スナップショットは既に永続化済み
    return errors.WithStack(err)
}

snapDir := filepath.Join(e.dataDir, snapDirName)
if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {
    return errors.Wrap(purgeErr, "purge old snap files")  // ← スナップショットは保存済みなのにエラーを返す
}

e.persist.Release(snap) 成功後にスナップショットはすでにディスクに永続化されている。その後 purgeOldSnapFiles が失敗した場合、呼び出し元にはスナップショット永続化が失敗したかのようにエラーが返る。

呼び出し元がこのエラーに応じてスナップショット処理をリトライしたり、適用済みインデックスを進めなかったりすると、ログとスナップショット状態の整合性に影響する可能性がある。スナップショットデータ自体はディスク上に存在するため直接的なデータロストではないが、パージ失敗を warning ログとして記録して処理を続行する方が安全。

2. purgeOldSnapFiles の安全性 — 問題なし

• ファイル名は <term_hex>-<index_hex>.snap であり、lex ソートは時系列順に一致する
• 最新 3 ファイルが保持されるため、直前に永続化したスナップショットが誤って削除されることはない
• defaultMaxSnapFiles = 3 の保持数は妥当

3. 起動時の stale spool 削除 — 問題なし

cleanupStaleSnapshotSpools は openDiskState (初期化時) で呼ばれており、その時点では他のスナップショット操作は未開始のため、削除対象ファイルが使用中になることはない。

---

総評: 直接的なデータロストリスクは低いが、purgeOldSnapFiles 失敗時の誤ったエラー返却が、スナップショット処理フローに悪影響を与える可能性がある。パージ失敗はログ警告にとどめ、関数自体は成功を返すよう変更することを推奨。

[claude]

[Consistency] レビュー結果

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

データ一貫性の観点では 問題なし。

変更内容はスナップショットスプールファイル（起動前のクラッシュで残存したテンポラリファイル）と古い .snap ファイルのクリーンアップであり、Raft ログや適用済みステートマシンには一切触れていない。

確認ポイント:

• cleanupStaleSnapshotSpools は起動時（openDiskState）に呼ばれており、Raft が動作を開始する前に実行される。Raft のリプレイやスナップショット適用に影響しない
• purgeOldSnapFiles は persistCreatedSnapshot / persistLocalSnapshotPayload の成功後に呼ばれており、新しいスナップショットが確実に永続化された後に古いファイルを削除する順序が守られている
• 削除対象は「最新 3 件より古い .snap ファイル」のみ。etcd の Snapshotter は最新スナップを読み込むため、古いスナップの削除はリカバリに影響しない

[claude]

[Consistency] レビュー結果

評価: 問題なし ✓

このPRはRaftエンジンのスナップショットスプールファイルのクリーンアップに関するものです。データ一貫性への影響はありません。

• Raft経由の必要性: クリーンアップ対象は未完了の一時ファイル(.tmp)のみ。永続化済みスナップショットには一切触れない。
• 線形化可能性: エンジン起動時の一回限りの操作であり、実行中のデータ操作との競合が生じない。persistCreatedSnapshot() / persistLocalSnapshotPayload() 内で Release() 完了後にクリーンアップを実行しており、順序は保証されている。
• TTL・データ整合性: スコープ外。
• レプリカ同期: 一時ファイルはレプリケーション対象ではないため、影響なし。

[claude]

[Test Coverage] レビュー結果

対象PR: #498 raftengine/etcd: clean up orphaned snapshot spool files on startup

概要

新規テストファイル snapshot_spool_test.go が追加されており、主要な関数に対するユニットテストは一通り揃っている。ただし以下の不足が見られる。

---

不足しているテスト

1. purgeOldSnapFiles — 存在しないディレクトリの挙動 (重要)

cleanupStaleSnapshotSpools は filepath.Glob を使用するため存在しないディレクトリでも nil, nil を返す（TestCleanupStaleSnapshotSpoolsNonExistentDir でカバー済み）。
一方 purgeOldSnapFiles は os.ReadDir を使用するため、ディレクトリが存在しない場合はエラーを返す。この挙動のテストが存在しない。

func TestPurgeOldSnapFilesNonExistentDir(t *testing.T) {
    err := purgeOldSnapFiles(filepath.Join(t.TempDir(), "no-such-dir"))
    // os.ReadDir はエラーを返すが、engine.go 側は Warn ログのみで無視する。
    // 関数の実際の挙動を明示的に検証すべき。
    require.Error(t, err)
}

2. purgeOldSnapFiles — ちょうど defaultMaxSnapFiles(3) 件の境界値

TestPurgeOldSnapFilesUnderLimit は 2 件のみ試している。条件が <= defaultMaxSnapFiles のため、ちょうど 3 件のときも削除されないが、境界値の明示的なテストがない。

func TestPurgeOldSnapFilesAtLimit(t *testing.T) {
    dir := t.TempDir()
    createSnapFile(t, dir, 1, 1000)
    createSnapFile(t, dir, 1, 2000)
    createSnapFile(t, dir, 1, 3000) // exactly defaultMaxSnapFiles
    require.NoError(t, purgeOldSnapFiles(dir))
    entries, err := os.ReadDir(dir)
    require.NoError(t, err)
    require.Len(t, entries, 3) // nothing removed
}

3. 起動時クリーンアップのインテグレーションテストが不在 (重要)

本PRの主目的（クラッシュ後に残ったスプールファイルを再起動時に掃除する）が end-to-end でテストされていない。openDiskState / Open を通じた以下シナリオのテストを追加すべき。

// 提案するテストの骨格
func TestOpenCleanupStaleSnapshotSpoolsOnRestart(t *testing.T) {
    dir := t.TempDir()
    // 1. エンジンを開き、正常にクローズする
    // 2. cfg.DataDir に "orphan" スプールファイルを手動配置
    f, _ := os.CreateTemp(dir, snapshotSpoolPattern)
    f.Close()
    // 3. エンジンを再起動 (openDiskState / Open)
    // 4. スプールファイルが消えていることを確認
    matches, _ := filepath.Glob(filepath.Join(dir, snapshotSpoolPattern))
    require.Empty(t, matches)
}

4. エラー蓄積パス (errors.CombineErrors) のテストがない

cleanupStaleSnapshotSpools および purgeOldSnapFiles はどちらも os.Remove が ENOENT 以外のエラーで失敗したときにエラーを蓄積して返す。この部分的失敗パス（一部ファイルは削除成功・一部は権限エラー等で失敗）のテストがない。

---

問題なし (カバー済み)

テスト	対象
TestCleanupStaleSnapshotSpools	正常系・無関係ファイル保持
TestCleanupStaleSnapshotSpoolsEmptyDir	空ディレクトリ
TestCleanupStaleSnapshotSpoolsNonExistentDir	存在しないディレクトリ
TestPurgeOldSnapFiles	6件→3件保持・非snapファイル保持
TestPurgeOldSnapFilesUnderLimit	上限未満(2件)は削除しない
TestPurgeOldSnapFilesEmptyDir	空ディレクトリ

---

優先度まとめ

優先度	項目
高	起動時クリーンアップのインテグレーションテスト（本PRの主要機能）
中	purgeOldSnapFiles の非存在ディレクトリ挙動の明示
低	ちょうど3件の境界値テスト
低	エラー蓄積パスのテスト

[claude]

[Concurrency] レビュー結果

検査対象

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

---

結論: 概ね問題なし。1点指摘あり。

---

各観点の確認

1. Race condition / 並行アクセス ✓

purgeOldSnapFiles の呼び出し元はすべて snapshotMu 保持下

• engine.go:1322（persistCreatedSnapshot 内）→ persistConfigSnapshotPayloadLocked 経由で常に snapshotMu を保持して呼ばれる（engine.go:1215, 1246）
• engine.go:2271（persistLocalSnapshotPayload 内）→ 直接 snapshotMu.Lock()（engine.go:2257）

2箇所の purgeOldSnapFiles が並行して走ることはなく、スナップショット書き込みと削除のレースは発生しない。

cleanupStaleSnapshotSpools の呼び出しタイミング

openDiskState から呼ばれるのはエンジン起動時のみ。Raftループ・gRPCトランスポートが起動する前なので、newSnapshotSpool が並行して走ることはない。起動後に cleanupStaleSnapshotSpools が再度呼ばれる経路はなく、稼働中との競合はない。

2. Raftリーダー切り替え時の安全性 ✓

purgeOldSnapFiles は最新 defaultMaxSnapFiles=3 ファイルを残す。スナップ送信中のファイルが削除されても Linux の unlink セマンティクスによりオープン中のファイルディスクリプタは継続有効。また送信中のファイルは最新であり削除対象外になるため実質的に問題なし。

3. ノード障害・再起動時の挙動 ✓

クラッシュ前のスプールファイルを起動時に削除する設計は正しい。snapshotSpool.Close() が呼ばれなかったファイルだけが対象となる（パターン elastickv-etcd-snapshot-*）。

4. ソート順の前提 ✓

purgeOldSnapFiles のコメント「os.ReadDir はファイル名順で返す」は正確。ゼロパディング16桁16進の term-index.snap 名は辞書順＝時系列順となる。Raftのログインデックスは単調増加なので逆転しない。

---

指摘事項

[WARN] cleanupStaleSnapshotSpools のエラーがエンジン起動失敗を引き起こす

場所: wal_store.go:41-43

if err := cleanupStaleSnapshotSpools(cfg.DataDir); err != nil {
    return nil, errors.Wrap(err, "cleanup stale snapshot spools")
}

スプールファイルの削除失敗（権限エラー、別プロセスによるロック等）がそのままエンジン起動失敗になる。スプールのクリーンアップはベストエフォートであり、失敗してもエンジン動作自体には影響しない。ディスクが溢れる前に警告ログを残すだけで十分な場面が多い。

提案:

if err := cleanupStaleSnapshotSpools(cfg.DataDir); err != nil {
    slog.Warn("failed to cleanup stale snapshot spools", "error", err)
}

ただし「クリーンアップ失敗をfastfailにする」という意図的な設計であれば許容範囲。現行コードがクラッシュセーフ性を優先しているなら現状維持もあり得る。

---

goroutineリーク・チャネルブロッキング

本PRで新たなgoroutine起動・チャネル操作は一切なし。問題なし。

[claude]

[Concurrency] レビュー結果

🟡 注意: purgeOldSnapFiles とスナップショット転送の競合リスク

ファイル: internal/raftengine/etcd/engine.go (persistCreatedSnapshot, persistLocalSnapshotPayload)

persistCreatedSnapshot および persistLocalSnapshotPayload の末尾で purgeOldSnapFiles を呼んでいます:

snapDir := filepath.Join(e.dataDir, snapDirName)
if purgeErr := purgeOldSnapFiles(snapDir); purgeErr != nil {
    slog.Warn("failed to purge old snap files", "error", purgeErr)
}

リーダーが フォロワーへスナップショットを転送中 に、新しいスナップショットの永続化が完了して purgeOldSnapFiles が古いスナップショットファイルを削除すると、転送中のファイルが消える可能性があります。デフォルト保持数 defaultMaxSnapFiles = 3 で、転送に時間がかかる場合（大きなスナップショット、低速ネットワーク）は現実的なリスクです。

etcd の fileutil.PurgeFile は flock でアクティブに使用中のファイルをスキップしますが、本実装にはその保護がありません。

確認事項: snap.Snapshotter がスナップショットファイルを読み取る際にファイルロックを保持しているか確認してください。etcd の内部 Snapshotter は Open 時にロックを取得しないため、外部からの削除を防ぐ仕組みがありません。

修正案の例:

• purgeOldSnapFiles 内で os.Open によるロック確認を行う（または flock 相当）
• あるいは、エンジンレベルのミューテックスでスナップショット操作をシリアライズする

---

✅ その他の観点: 問題なし

• goroutine リーク: 新規 goroutine の追加なし
• チャネルブロッキング: 非同期処理の追加なし
• コンテキストキャンセル: cleanupStaleSnapshotSpools は起動時の同期処理で問題なし
• os.IsNotExist ハンドリング: purgeOldSnapFiles と cleanupStaleSnapshotSpools 両方で適切に処理されている
• Raft リーダー切り替え時: openDiskState での cleanup は初期化時のみで、並行性の問題なし

[claude]

[Performance] レビュー結果

PR #498: raftengine/etcd: clean up orphaned snapshot spool files on startup

指摘事項

[軽微] スナップショット永続化パスへの同期的なディスクI/O追加

persistCreatedSnapshot と persistLocalSnapshotPayload の両方から purgeOldSnapFiles が 同期的 に呼ばれています。

// engine.go
snapDir := filepath.Join(e.dataDir, snapDirName)
if purgeErr := purgeOldSnapFiles(snapDir); purgeErr != nil {
    slog.Warn(...)
}

purgeOldSnapFiles は内部で os.ReadDir (ディレクトリスキャン) + 複数の os.Remove を発行します。スナップショット永続化はもともと重いI/O操作ですが、その末尾にさらにディレクトリスキャンと削除が加わります。スナップショット発生頻度は低いため実害は少ないですが、ゴルーチンに切り出して非同期化することでレイテンシへの影響を排除できます。

[軽微] filepath.Join の都度アロケーション

e.dataDir + snapDirName の組み合わせは起動後に変わらないにもかかわらず、呼び出しごとに filepath.Join で新規文字列を生成しています。Engine 構造体にフィールドとして保持することでアロケーションを削減できます（スナップショット頻度が低いため影響は微小）。

---

全体として性能への影響は小さく、実用上の問題は生じにくい変更です。

[claude]

[Data Loss] レビュー結果

対象PR: raftengine/etcd: clean up orphaned snapshot spool files on startup

---

1. cleanupStaleSnapshotSpools（起動時のスプールファイル削除）

問題なし

• 削除対象は snapshotSpoolPattern = "elastickv-etcd-snapshot-*" にマッチするファイルで、os.CreateTemp で生成された一時ステージングファイル
• コミット済みの .snap ファイル（dataDir/snap/）とはディレクトリも命名規則も異なるため、誤削除のリスクなし
• クラッシュ中断されたスナップショット転送のスプールファイルを削除しても、転送はリーダーから再実行されるためデータロストにはならない

---

2. purgeOldSnapFiles（古い .snap ファイルの削除）

問題なし

• では が成功した後に を呼び出しており、新スナップショットがディスクに確定してから旧ファイルを削除する順序が守られている
• でも同様、成功パス（）の中でのみ削除を実行
• 削除失敗時は でログ出力するのみで処理は継続されるため、パージ失敗がデータロストにつながらない

---

3. ソート順の前提（スナップショット選別ロジック）

問題なし

• コメントに「os.ReadDir returns entries in directory order」とあるが、Go の はファイル名でソートして返す（仕様保証）
• スナップファイルの命名規則は （term-index のゼロパディング16進数）のため、辞書順 = 時系列順が成立
• Raft の性質上、(term, index) の両方が単調増加するため、ソート順の反転（古いインデックスが新しい term で発生）は起こり得ない

---

総評

データロストリスク: なし

スプールファイルはコミット済みスナップショットとは独立した一時ファイルであり、その削除はデータロストにつながらない。スナップファイルのパージは新スナップショット確定後に実施される設計で、削除順序も正しい。

[claude]

[Data Loss] レビュー結果

対象PR: raftengine/etcd: clean up orphaned snapshot spool files on startup

---

1. cleanupStaleSnapshotSpools（起動時のスプールファイル削除）

問題なし

• 削除対象は snapshotSpoolPattern = "elastickv-etcd-snapshot-*" にマッチするファイルで、os.CreateTemp で生成された一時ステージングファイル
• コミット済みの .snap ファイル（dataDir/snap/）とはディレクトリも命名規則も異なるため、誤削除のリスクなし
• クラッシュ中断されたスナップショット転送のスプールファイルを削除しても、転送はリーダーから再実行されるためデータロストにはならない

---

2. purgeOldSnapFiles（古い .snap ファイルの削除）

問題なし

• persistCreatedSnapshot では e.persist.Release(snap) が成功した後に purgeOldSnapFiles を呼び出しており、新スナップショットがディスクに確定してから旧ファイルを削除する順序が守られている
• persistLocalSnapshotPayload でも同様、成功パス（err == nil）の中でのみ削除を実行
• 削除失敗時は slog.Warn でログ出力するのみで処理は継続されるため、パージ失敗がデータロストにつながらない

---

3. ソート順の前提（スナップショット選別ロジック）

問題なし

• コメントに "os.ReadDir returns entries in directory order" とあるが、Go の os.ReadDir はファイル名でソートして返す（仕様保証）
• スナップファイルの命名規則は %016x-%016x.snap（term-index のゼロパディング16進数）のため、辞書順 = 時系列順が成立
• Raft の性質上、(term, index) の両方が単調増加するため、ソート順の反転（古いインデックスが新しい term で発生）は起こり得ない

---

総評

データロストリスク: なし

スプールファイルはコミット済みスナップショットとは独立した一時ファイルであり、その削除はデータロストにつながらない。スナップファイルのパージは新スナップショット確定後に実施される設計で、削除順序も正しい。