store: add read-set validation to ApplyMutations for SSI by bootjp · Pull Request #499 · bootjp/elastickv

bootjp · 2026-04-12T19:16:15Z

Add readKeys parameter to ApplyMutations so that the store validates both write-write and read-write conflicts under the apply lock. This upgrades transaction isolation from Snapshot Isolation (SI) to Serializable Snapshot Isolation (SSI), preventing write skew anomalies where concurrent transactions read overlapping keys but write disjoint ones.

The read set collected in the Redis adapter txnContext is threaded through the coordinator, FSM, and proto layers into the store, where each read key is checked for commits after startTS.

Add readKeys parameter to ApplyMutations so that the store validates both write-write and read-write conflicts under the apply lock. This upgrades transaction isolation from Snapshot Isolation (SI) to Serializable Snapshot Isolation (SSI), preventing write skew anomalies where concurrent transactions read overlapping keys but write disjoint ones. The read set collected in the Redis adapter txnContext is threaded through the coordinator, FSM, and proto layers into the store, where each read key is checked for commits after startTS.

gemini-code-assist · 2026-04-12T19:16:21Z

Warning

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

claude · 2026-04-12T19:18:10Z

[Test Coverage] レビュー結果

総評

このPRはSI（Snapshot Isolation）からSSI（Serializable Snapshot Isolation）へのアップグレードという重要な変更にもかかわらず、新しいロジックに対する専用テストがゼロです。既存テストへの変更はすべて nil を追加する機械的な修正のみであり、新機能そのものの動作は一切検証されていません。

不足しているテスト

1. `pebbleStore.checkReadConflicts` のユニットテスト（`store/lsm_store_txn_test.go`）

新規追加メソッドに対するテストが皆無です。以下のケースが必要です：

// 提案テスト例
func TestPebbleStore_ApplyMutations_ReadConflict(t *testing.T) {
    // T=10 に "k1" をコミット → startTS=5 で readKeys=["k1"] → ErrWriteConflict を期待
}

func TestPebbleStore_ApplyMutations_ReadNoConflict_OldCommit(t *testing.T) {
    // T=5 に "k1" をコミット → startTS=10 で readKeys=["k1"] → 競合なし（ts <= startTS）
}

func TestPebbleStore_ApplyMutations_ReadConflict_BoundaryTS(t *testing.T) {
    // T=10 に "k1" をコミット → startTS=10 で readKeys=["k1"] → 競合なし（ts == startTS は > ではない）
}

func TestPebbleStore_ApplyMutations_ReadConflict_KeyNotExist(t *testing.T) {
    // 一度も書かれていない "ghost" キーを readKeys に含める → 競合なし
}

2. `mvccStore` の読み込みキー競合チェックのテスト（`store/mvcc_store_snapshot_test.go`）

mvccStore.ApplyMutations に追加されたループにも同様にテストがありません：

func TestMVCCStore_ApplyMutations_ReadConflict(t *testing.T) {
    // pebble 側と同等のシナリオを mvccStore で検証
}

3. Write Skew（書き込みスキュー）防止の統合テスト（最重要）

PR の目的そのものを証明するテストが存在しません。古典的な write skew シナリオ（「医師当直問題」等）を再現するテストが必要です：

func TestWriteSkewPrevented_SSI(t *testing.T) {
    // 初期状態: balance_a=100, balance_b=100
    // Txn1: reads balance_a & balance_b (sum=200 ≥ 200 → OK), writes balance_a -= 200
    // Txn2: reads balance_a & balance_b (sum=200 ≥ 200 → OK), writes balance_b -= 200
    // 両方が並行コミット → どちらか一方が ErrWriteConflict になることを確認
    // SI では両方コミットされてしまう（残高合計が -200 になるバグ）
}

このテストなしには PR が意図した SSI の保証を実際に提供しているかどうかが確認できません。

4. 並行読み書き競合テスト（`store/mvcc_store_concurrency_test.go` 相当）

既存の TestMVCCConcurrentApplyMutations は write-write 競合を対象としています。readKeys を使った並行 read-write 競合シナリオ（複数 goroutine が readKeys 付きでコミット）のテストも必要です。

5. `adapter/redis_txn_test.go` — readKeys の伝播テスト

adapter/redis.go で t.readKeys を OperationGroup.ReadKeys に変換して渡す部分のテストがありません。Redis MULTI/EXEC トランザクション中に読み込んだキーが ReadKeys として coordinator に正しく渡されることを確認するテストが必要です。

軽微な指摘

checkReadConflicts のシグネチャで渡された ctx が無視され、内部で context.Background() が直接使われています（lsm_store.go:866）。他の check* メソッドと不整合であり、コンテキストのキャンセルが効かなくなっています。テストとは別の問題ですが、同時に修正することを推奨します。

まとめ

対象	状況
`pebbleStore.checkReadConflicts`	テストなし ❌
`mvccStore` read-set チェック	テストなし ❌
Write Skew 防止シナリオ	テストなし ❌
並行 read-write 競合	テストなし ❌
`adapter` readKeys 伝播	テストなし ❌
既存テストの破損	なし ✅

claude · 2026-04-12T19:18:30Z

[Concurrency] レビュー結果

🔴 HIGH: マルチシャードトランザクションで `readKeys` が無声破棄される

場所: kv/sharded_coordinator.go:211-227

if len(gids) == 1 {
    return c.dispatchSingleShardTxn(startTS, commitTS, primaryKey, gids[0], elems, readKeys)
}
// ↓ readKeys が prewriteTxn に渡されていない
prepared, err := c.prewriteTxn(startTS, commitTS, primaryKey, grouped, gids)

シャードが2つ以上にまたがるトランザクションでは readKeys が prewriteTxn / commitPrimaryTxn / commitSecondaryTxns のいずれにも伝播されません。その結果、マルチシャードトランザクションは依然として SI（Snapshot Isolation）のみであり、PR の目的である SSI（Serializable Snapshot Isolation）はシングルシャード限定で有効になるに過ぎません。異なるシャードにまたがるキーを読み書きするトランザクション間での write skew は防止できません。

prewriteTxn が発行する pb.Phase_PREPARE リクエストに ReadKeys を含め、FSM の handlePrepareRequest でも read-set 検証を行う必要があります。あるいは COMMIT フェーズの pb.Request に ReadKeys を付与する方針でも可。

🟡 MEDIUM: `checkReadConflicts` がコンテキストを無視している

場所: store/lsm_store.go:864-875

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // ← context.Background() を新規生成

直上の checkConflicts（line 849）は受け取った ctx を正しく latestCommitTS へ渡しているのに、checkReadConflicts はパラメータを _ で捨てて context.Background() を直接使っています。これにより：

呼び出し元がタイムアウト付きコンテキストを渡しても、read-set チェック中はキャンセルが伝播しない
ApplyMutations のコンテキストが既にキャンセル済みでも latestCommitTS が実行される

修正: シグネチャを func (s *pebbleStore) checkReadConflicts(ctx context.Context, ...) とし、s.latestCommitTS(ctx, key) を呼ぶようにする。

🔵 INFO: アダプタ層の二重バリデーションとフォロワーの古いデータ

場所: adapter/redis.go:2105-2108

if err := txn.validateReadSet(dispatchCtx); err != nil {  // ① ロックなし・ローカルストア参照
    return err
}
if err := txn.commit(); err != nil {                      // ② FSM の applyMu 下で再チェック

validateReadSet（①）はフォロワーノード上のローカルストアに対してロックなしで実行されます。フォロワーの Raft 適用が遅延していると、リーダー側に既にコミットされているバージョンが見えず、本来 abort すべきトランザクションを通過させる可能性があります。ただし FSM 層（②）での applyMu 下の検証が権威的なため、最終的な安全性は保たれます（誤コミットはしない）。

一方で逆方向も存在します: フォロワーが古いデータを反映したまま validateReadSet がコンフリクトを誤検知し、FSM 層では問題ないトランザクションを不必要に abort させる可能性があります。これは不要なリトライを引き起こしますが、正確性には影響しません。

アーキテクチャ上の懸念として、フォロワーの事前チェックを残す意図（早期 abort 最適化）であれば、そのことをコメントで明示することを推奨します。

claude · 2026-04-12T19:18:33Z

[Consistency] レビュー結果

問題1（Critical）: クロスシャードトランザクションで `readKeys` が無視される

ファイル: kv/sharded_coordinator.go

ShardedCoordinator.dispatchTxn は readKeys を受け取っているが、トランザクションが複数シャードにまたがる場合（len(gids) > 1）、prewriteTxn → commitPrimaryTxn のパスに入り、readKeys がいずれにも渡されない。

if len(gids) == 1 {
    return c.dispatchSingleShardTxn(startTS, commitTS, primaryKey, gids[0], elems, readKeys)
}

// readKeys がここで黙って捨てられる
prepared, err := c.prewriteTxn(startTS, commitTS, primaryKey, grouped, gids)

影響: 単一シャードのトランザクションは SSI（Serializable Snapshot Isolation）が適用されるが、クロスシャードのトランザクションは依然として SI（Snapshot Isolation）のままとなり、ライトスキューが発生しうる。PRのコメント更新（「Both write-write and read-write conflicts are checked」）とも矛盾している。

対応案:

prewriteTxn の PREPARE リクエストに ReadKeys を含め、FSM の handlePrepareRequest 側でも検証する
または、単一シャードへの制限として API コントラクトに明記し、クロスシャード時は ErrNotSupported を返す

問題2（Minor）: `checkReadConflicts` がコンテキストを無視する

ファイル: store/lsm_store.go

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // ctx を使うべき

引数の ctx を捨てて context.Background() を使用しているため、FSM からのタイムアウトやキャンセルが readKeys のチェック中に伝播しない。checkConflicts は ctx を正しく使っているため統一性もない。

対応案: _ context.Context を ctx context.Context に変え、s.latestCommitTS(ctx, key) を使用する。

確認事項（設計意図の明確化を推奨）

CatalogStore.applySaveMutations（distribution/catalog.go）および applyCommitWithIdempotencyFallback（kv/fsm.go）で nil を渡しているのは意図的と思われるが、前者はカタログ操作が read set を追跡しない設計であることをコメントで明示すると保守性が上がる。

claude · 2026-04-12T19:18:51Z

[Performance] レビュー結果

1. リードセットの二重検証（高影響）

adapter/redis.go の commit() 呼び出し前に validateReadSet がすでに実行されており（L2105）、さらに ApplyMutations 内の checkReadConflicts でも同じチェックが行われる。結果として Redis adapter 経由のトランザクションでは、読み取りセット全体の LatestCommitTS ディスク I/O が 2倍になる。

// adapter/redis.go L2105
if err := txn.validateReadSet(dispatchCtx); err != nil { ... }  // 1回目
if err := txn.commit(); err != nil { ... }   // → ApplyMutations → checkReadConflicts (2回目)

読み取りセットが大きい場合（SCAN / KEYS 等）は顕著なオーバーヘッドになる。どちらか一方に統一すべき。

2. `applyMu.Lock()` 保持中に O(N) 逐次 Pebble ポイントルックアップ（高影響）

checkReadConflicts は applyMu.Lock() を保持したまま読み取りキー数分だけ pebble.NewIter → SeekGE → Close を繰り返す（lsm_store.go:864-875）。

// lsm_store.go:864
func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // イテレータ生成×N
        ...
    }
}

applyMu.Lock() はすべての並行 ApplyMutations を直列化するため、読み取りセットが大きいトランザクションが来るたびに他の書き込みが待機させられる。書き込みセットも同様だが、読み取りセットは一般に桁違いに大きくなり得る（例：SCAN 全件）。

3. 書き込みセットと読み取りセットの重複チェック（中影響）

同じキーが mutations と readKeys の両方に含まれる場合、checkConflicts と checkReadConflicts でそれぞれ Pebble ルックアップが走る。書き込まれるキーはすでに書き込みコンフリクト検出済みなので、checkReadConflicts 側でスキップできる。

4. `context.Background()` の使用（低影響）

lsm_store.go:866 にて引数として受け取ったコンテキストを無視して context.Background() を生成している（関数シグネチャでも _ context.Context として捨てている）。タイムアウトやキャンセルが伝播されず、負荷時にゴルーチンが詰まる可能性がある。checkConflicts は渡された ctx をそのまま使っており、挙動が不一致。

claude · 2026-04-12T19:19:37Z

[Data Loss] レビュー結果

結論: 重大なデータロストリスクは検出されず。ただし1件の隔離レベル低下による潜在リスクあり。

問題なし: コアの書き込みアトミシティ

lsm_store.go:900-944 の ApplyMutations において、新たに追加された checkReadConflicts は既存の checkConflicts と同じ applyMu.Lock() 臨界区内で実行されています。

s.applyMu.Lock()
defer s.applyMu.Unlock()

checkConflicts(...)        // 既存: write-write 競合チェック
checkReadConflicts(...)    // 新規: read-write 競合チェック ← ロック内で正しく実行
applyMutationsBatch(...)   // バッチ書き込み
b.Commit(pebble.Sync)      // fsync 付きコミット

チェックと書き込みがアトミックに行われるため、TOCTOU（Time-of-Check-Time-of-Use）競合による誤コミットは発生しません。WAL/Pebble の耐久性（pebble.Sync）も変更なく維持されています。

懸念: マルチシャードトランザクションで read-set 検証がアトミックにならない

kv/sharded_coordinator.go:215 の dispatchTxn において、シャードが複数の場合 readKeys が prewriteTxn に渡されていません。

if len(gids) == 1 {
    return c.dispatchSingleShardTxn(..., readKeys)  // ✅ readKeys が渡される
}

prepared, err := c.prewriteTxn(...)  // ❌ readKeys が渡されない（2PC パス）

シングルシャードの場合は readKeys が Raft ログに含まれ、FSM の applyMu 内でアトミックに検証されます。しかし マルチシャードトランザクションでは 、アダプター層の validateReadSet（redis.go:2105）のみが実行されます。このチェックは Dispatch 呼び出しの前に行われるため、validateReadSet と実際のコミット完了の間に別トランザクションが割り込むと write skew（書き込み歪み）が発生する可能性があります。

write skew が発生した場合、不正なスナップショットに基づいたデータが書き込まれてしまい、論理的なデータ破壊につながる可能性があります。
ただし既存の 2PC パスで readKeys をどのシャードに渡すか（primary のみか全シャードか）の設計判断が必要なため、修正コストは高い可能性があります。
現状 PR の説明文にはシングルシャードのみ対象と明記されておらず、クロスシャードでも SSI が保証されていると誤解されるリスクがあります。

提案: dispatchTxn のマルチシャードパスに、read-set 検証が行われない旨のコメントを追加するか、あるいはマルチシャードトランザクションで readKeys が非空の場合はエラーを返すことを検討してください。

軽微: `checkReadConflicts` でコンテキストが無視されている

store/lsm_store.go:861:

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // context.Background() を直接使用

引数の ctx を無視し context.Background() を使っているため、呼び出し元のキャンセル・タイムアウトが伝播しません。データロストには直結しませんが、長い read-set を持つトランザクションでシャットダウン遅延が発生する可能性があります。

総括

観点	判定
Write/Delete の意図しないデータ消失	問題なし
WAL/Raft ログの永続化	問題なし (`pebble.Sync` 維持)
エラーハンドリング欠如によるデータ消失	問題なし
トランザクションのコミット/ロールバック	問題なし
TTL/Expire による意図しない削除	問題なし
スナップショット/バックアップ関連	問題なし
マルチシャード SSI の write skew	要確認 (隔離レベル低下リスク)

claude · 2026-04-12T19:22:27Z

[Performance] レビュー結果

問題 1 — `checkReadConflicts` が apply ロック保持中に N 個の Pebble イテレータを生成する（重要）

ファイル: store/lsm_store.go

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)

latestCommitTS は呼び出しごとに db.NewIter(...) でPebbleイテレータを1つ生成し（SeekGE + Close）、これをread key数 N 回繰り返す。この処理はすべて s.applyMu.Lock() 保持中に実行される。読み取りセットが大きいトランザクションほど、apply ミューテックスの保持時間が線形に増加し、並列トランザクションのスループットが低下する。

既存の checkConflicts（書き込みキー用）も同じパターンだが、読み取りセットは書き込みセットより一般的に大きくなりやすい点に注意。

改善案: 読み取りキーをソートした上で、単一のPebbleイテレータを再利用して SeekGE で各キーの最新バージョンを走査すれば、イテレータの生成コストを O(N) → O(1) アロケーションに削減できる。

問題 2 — `context.Background()` がループ内で使われている（軽微）

ファイル: store/lsm_store.go

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)

引数の context.Context を _ で無視し、ループ内で context.Background() を使っている。latestCommitTS 自体もコンテキストを無視するため現状は実害なし（context.Background() はシングルトンなので追加アロケーションもない）。ただし将来的にキャンセル/タイムアウト伝播が必要になった際の修正漏れリスクがある。引数 ctx をそのまま渡すべき。

問題 3 — 読み取りセットを2回 Pebble 検索する（中程度）

ファイル: adapter/redis.go（validateReadSet）と store/lsm_store.go（checkReadConflicts）

validateReadSet（dispatch前、ロック外）と checkReadConflicts（apply ロック内）の両方で、読み取りキーごとに latestCommitTS を呼び出す。1回のSSIトランザクションコミットで read key 1件につき Pebble 点読み取りが計 2回発生する。

validateReadSet はロック取得前のフェイルファスト最適化として意図されており、設計上の意図は理解できる。しかし読み取りセットが大きいワークロードでは合計I/Oが倍増する。フェイルファスト効果と I/O コストのトレードオフを明示的にコメントで記載しておくと可読性・保守性が向上する。

問題 4 — `readKeys` のスライス化（軽微）

ファイル: adapter/redis.go

readKeys := make([][]byte, 0, len(t.readKeys))
for _, k := range t.readKeys {
    readKeys = append(readKeys, k)
}

t.readKeys が map[string][]byte である以上この変換は避けられないが、トランザクションコミットごとにヒープアロケーションが発生する。txnContext の設計段階で readKeys を [][]byte（+ dedup用 map[string]struct{}）として保持すればこの変換を排除できる。現状はコミット頻度が低ければ大きな問題ではないが、高スループットシナリオでは GC 圧力になりうる。

claude · 2026-04-12T19:22:39Z

[Test Coverage] レビュー結果

対象PR: #499 store: add read-set validation to ApplyMutations for SSI

🔴 重大: 新規コアロジックに専用テストがない

本PRが追加した主要な新機能は以下の3点です。

pebbleStore.checkReadConflicts (store/lsm_store.go) — 新規関数
mvccStore.ApplyMutations の readKeys ループ (store/mvcc_store.go) — 新規ロジック
エンドツーエンド: txnContext.commit() が readKeys を収集し、handleOnePhaseTxnRequest 経由で ApplyMutations まで伝播

いずれも専用テストが存在しません。 テストファイルの変更はすべて、新しい readKeys 引数に nil を渡す機械的な修正のみです。

不足しているテストケース (具体的な提案)

1. `checkReadConflicts` — 読み書きコンフリクト検出 (最重要)

func TestPebbleStore_ApplyMutations_ReadConflict(t *testing.T) {
    // setup: key "k" committed at TS=20
    // txn reads "k" at startTS=10 (read set: ["k"])
    // → ApplyMutations with readKeys=["k"], startTS=10 must return ErrWriteConflict
}

2. `checkReadConflicts` — コンフリクトなし (正常系)

func TestPebbleStore_ApplyMutations_ReadNoConflict(t *testing.T) {
    // key "k" committed at TS=5, startTS=10
    // readKeys=["k"] → 競合なし、成功すること
}

3. 境界値: nil/空の readKeys は no-op

// nil と [][]byte{} の両方で既存の書き込み競合検出が壊れないことを確認

4. 存在しないキーを readKeys に含む場合

// 一度も書き込まれていないキーを readKeys に指定した場合、競合しないこと

5. `mvccStore` の readKeys コンフリクト検出

mvccStore 側にも同等の実装が追加されているが、テストが皆無です。

func TestMVCCStore_ApplyMutations_ReadConflict(t *testing.T) { ... }

6. エンドツーエンド: Write Skew 防止 (SSIの本質的な検証)

PRタイトルが「SSI」を謳っているにもかかわらず、Write Skew (読み書きスキュー) が実際に防止されることを示すテストがありません。

// 典型例:
// T1: read(x)=0, write(y)=1 (readKeys=["x"])
// T2: read(y)=0, write(x)=1 (readKeys=["y"])
// 両方コミット → SSI では少なくとも一方が ErrWriteConflict になるべき
func TestSSI_PreventWriteSkew(t *testing.T) { ... }

7. 並行アクセス: readKeys コンフリクト検出の競合安全性

// checkReadConflicts + checkConflicts が apply ロック下で
// 競合なく動作することを並行ゴルーチンで検証
func TestPebbleStore_ApplyMutations_ConcurrentReadConflict(t *testing.T) { ... }

8. ReadKeys がプロトコルバッファ経由で正しく伝播するか

onePhaseTxnRequest で ReadKeys が pb.Request に格納され、handleOnePhaseTxnRequest まで届くことのテストが存在しません。

既存テストへの影響

既存テストはシグネチャ変更に追随しており、壊れているものはありません。

まとめ

観点	状態
変更ロジックに対応するテスト	❌ なし (checkReadConflicts, mvccStore readKeys)
エッジケース (nil, 空, 存在しないキー)	❌ なし
正常系テスト	❌ なし
エラーパス (ErrWriteConflict from readKeys)	❌ なし
並行アクセステスト	❌ なし
Write Skew 防止のE2Eテスト	❌ なし
既存テストの破損	✅ 問題なし

SSI実装の正しさが自動テストで一切検証されていない状態です。上記テストケースの追加を強く推奨します。

claude · 2026-04-12T19:23:05Z

[Consistency] レビュー結果

問題1（重大）: マルチシャードトランザクションで readKeys が無視される

対象: kv/sharded_coordinator.go

dispatchTxn は readKeys パラメータを受け取るが、トランザクションが複数シャードにまたがる場合（2PC パス）は readKeys が prewriteTxn / commitPrimaryTxn に渡されず、サイレントに破棄される。

if len(gids) == 1 {
    return c.dispatchSingleShardTxn(..., elems, readKeys)  // readKeys が渡される ✓
}

// 2PC パス: readKeys が渡されない ✗
prepared, err := c.prewriteTxn(startTS, commitTS, primaryKey, grouped, gids)

影響: SSI（Serializable Snapshot Isolation）への昇格はシングルシャードトランザクションにのみ適用される。複数シャードにまたがるトランザクションは依然として SI（Snapshot Isolation）のままであり、write skew 異常が発生しうる。PR説明文「This upgrades transaction isolation from SI to SSI」および handleOnePhaseTxnRequest のコメント「Both write-write and read-write conflicts are checked」は、全ケースに適用されるかのように読めるため、誤解を招く。

問題2（軽微）: `checkReadConflicts` がコンテキストを無視している

対象: store/lsm_store.go

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // ← context.Background() を直接使用

渡された ctx を破棄して context.Background() を使用しているため、呼び出し元のキャンセル・タイムアウトが伝播しない。checkConflicts は正しく ctx を使用しており、整合性がない。_ context.Context の代わりに ctx context.Context を使い、内部でも渡すべき。

まとめ

深刻度	問題	場所
重大	マルチシャード 2PC で read-set バリデーションがスキップされる（write skew が残存）	`kv/sharded_coordinator.go`
軽微	`checkReadConflicts` が `ctx` を無視し `context.Background()` を使用	`store/lsm_store.go`

claude · 2026-04-12T19:23:51Z

[Concurrency] レビュー結果

[HIGH] マルチシャードトランザクションで `readKeys` が伝播されない

kv/sharded_coordinator.go の dispatchTxn は、readKeys をシングルシャードパスにのみ渡しています。

// シングルシャード → readKeys が onePhaseTxnRequest に渡る（正しい）
if len(gids) == 1 {
    return c.dispatchSingleShardTxn(startTS, commitTS, primaryKey, gids[0], elems, readKeys)
}

// マルチシャード 2PC → readKeys が prewriteTxn / commitPrimaryTxn / commitSecondaryTxns に渡らない
prepared, err := c.prewriteTxn(startTS, commitTS, primaryKey, grouped, gids)

prewriteTxn が構築する pb.Request には ReadKeys フィールドが設定されておらず（kv/sharded_coordinator.go:276-280）、FSM 側の checkReadConflicts が呼ばれません。

Redis の MULTI/EXEC ブロック内で異なるシャードに属するキーを読み書きするケース（例: GET shard-A → SET shard-B）では、Write Skew が依然として発生します。

アダプタ層の validateReadSet（adapter/redis.go:1893）はコミット直前にクライアント側で呼ばれますが、この検査は applyMu 外で行われるため TOCTOU ギャップがあり、validateReadSet 合格後・Raft ログ適用前に別トランザクションが読み取りキーへ書き込む可能性があります。2PC パスを SSI の保証対象とするなら、PREPARE フェーズのリクエストに ReadKeys を付与して FSM に検証させる必要があります。

[MEDIUM] `checkReadConflicts` がコンテキストを無視している

store/lsm_store.go:864 の実装：

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)

引数のコンテキスト（タイムアウト・キャンセル付き）を _ で捨てて context.Background() を使っています。
この関数は applyMu.Lock() を保持した状態で呼ばれます。読み取りキー数が多い場合や latestCommitTS が遅延した場合、リーダー変更時のコンテキストキャンセルが効かず、applyMu を長時間占有します。
修正は _ context.Context → ctx context.Context に変え、latestCommitTS(ctx, key) に渡すだけです。

[LOW] `readKeys` スライスの順序が非決定的

adapter/redis.go:1926-1929 で map[string][]byte を [][]byte に変換する際、Go のマップ反復順序は非決定的です。

readKeys := make([][]byte, 0, len(t.readKeys))
for _, k := range t.readKeys {
    readKeys = append(readKeys, k)
}

各キーは独立してチェックされるため正確性への影響はありませんが、proto/internal.pb.go 上の ReadKeys フィールドが Raft ログに書き込まれるため、テストの再現性やログの比較において順序不定は混乱を招く可能性があります。ソートを検討してください。

まとめ

重要度	箇所	内容
HIGH	`kv/sharded_coordinator.go:215`	マルチシャード 2PC で `readKeys` が未伝播 → SSI 未達成
MEDIUM	`store/lsm_store.go:864`	`checkReadConflicts` がコンテキストを捨てている
LOW	`adapter/redis.go:1926`	`readKeys` の順序が非決定的

単一シャードパス（onePhaseTxnRequest 経由）での実装は、applyMu による check→commit 原子性が正しく確保されており問題ありません。

claude · 2026-04-12T19:24:44Z

[Data Loss] レビュー結果

対象PR: #499 store: add read-set validation to ApplyMutations for SSI

結論: 重大なデータロストリスクなし

ただし以下の注意事項があります。

確認項目

✅ Write前の競合チェック順序（`store/lsm_store.go`）

if err := s.checkConflicts(ctx, mutations, startTS); err != nil {
    return err
}
if err := s.checkReadConflicts(ctx, readKeys, startTS); err != nil {
    return err
}
// ← ここで初めてデータ書き込み
if err := s.applyMutationsBatch(b, mutations, commitTS); err != nil {

checkConflicts と checkReadConflicts の両方がデータ書き込みより前に実行されており、どちらかが失敗しても書き込みは行われません。パーシャルライトのリスクはありません。

✅ `mvccStore.ApplyMutations`（`store/mvcc_store.go`）

同様に、readKeys の競合チェックはミューテックスロック取得後・実際の書き込み前に行われており正しい順序です。

✅ nil readKeys の扱い

Raw リクエスト・Prepare・Abort・コミットフォールバックなど、非SSIパスはすべて nil を渡しており、checkReadConflicts は nil スライスに対して何もしないため問題ありません。

✅ Raftログへの永続化

read_keys は proto/internal.proto の Request メッセージに field 5 として追加されています。Raft ログに含まれるため、リプレイ時も同じ競合チェックが実行されます。

注意事項

⚠️ クロスシャードトランザクションへのSSI適用漏れ（データ整合性の懸念）

kv/sharded_coordinator.go の dispatchTxn において、シングルシャードの場合のみ readKeys が渡されます:

if len(gids) == 1 {
    return c.dispatchSingleShardTxn(startTS, commitTS, primaryKey, gids[0], elems, readKeys)
}
// ↓ マルチシャード(2PC)パスでは readKeys が渡されない
prepared, err := c.prewriteTxn(startTS, commitTS, primaryKey, grouped, gids)

クロスシャードトランザクションでは read-write コンフリクト（Write Skew）が防止されません。データが「消える」わけではありませんが、SSI が期待される場面では不整合なデータが書き込まれる可能性があります。意図的な制限であれば、コメント等での明記を推奨します。

🔍 軽微: `checkReadConflicts` がコンテキストを無視（`store/lsm_store.go`）

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key) // ← context.Background()

渡された context は捨てられ、context.Background() が使われています。データロストには直結しませんが、タイムアウトやキャンセルが効かないため、大量の readKeys がある場合にリソースリークとなりえます。

- Thread readKeys through multi-shard PREPARE phase so cross-shard transactions also validate read-write conflicts (Concurrency/Consistency review) - Remove redundant validateReadSet call in Redis adapter; the store-layer check under applyMu is sufficient and avoids double LatestCommitTS I/O (Performance review) - Extract checkConflictsLocked in mvcc_store to reduce ApplyMutations cyclomatic complexity below the cyclop threshold (Lint review) - Add dedicated read-write conflict tests: conflict detected, no conflict when read key is unchanged, nonexistent read key, and nil readKeys backward compatibility (Test Coverage review)

claude · 2026-04-12T19:30:52Z

[Test Coverage] レビュー結果

PR #499 は SSI (Serializable Snapshot Isolation) の read-write 競合検出を ApplyMutations に組み込む変更です。pebbleStore 向けに 4 件の新規テストが追加されており、基本的なカバレッジは確保されています。ただし、以下の不足点が確認されました。

不足しているテスト

1. `mvccStore` の読み取り競合テストが皆無

深刻度: 高

mvcc_store.go に checkConflictsLocked が新設され read-write 競合を検出するように変更されましたが、mvcc_store_snapshot_test.go / mvcc_store_concurrency_test.go にはこの新ロジックを直接検証するテストが追加されていません。pebbleStore と対称になるよう、以下の 4 ケースが必要です。

提案テストケース:

// TestMVCCStore_ApplyMutations_ReadConflict
// TestMVCCStore_ApplyMutations_ReadConflict_NoConflictWhenReadKeyUnchanged
// TestMVCCStore_ApplyMutations_ReadConflict_NonexistentReadKey
// TestMVCCStore_ApplyMutations_ReadConflict_NilReadKeys

それぞれ pebble 側の同名テストと同じロジックで mvccStore を対象に実施する。

2. write-skew (書き込みスキュー) の並行シナリオテストがない

深刻度: 高

SSI 対応の核心はスキューの防止ですが、並行テストが一切追加されていません。

提案テストケース:

// TestPebbleStore_SSI_WriteSkew_Concurrent
// Txn A: reads k1 (TS=10), writes k2 → startTS=10, commitTS=30
// Txn B: reads k2 (TS=10), writes k1 → startTS=10, commitTS=20
// B が先に commit → A は read-write conflict で弾かれることを検証

3. 空スライス (non-nil empty) の `readKeys` テストがない

深刻度: 低

adapter/redis.go では `make([][]byte, 0, ...)\ で空スライスを生成しますが、nil と empty slice の動作差異が保証されていません。

提案テストケース:

// err = s.ApplyMutations(ctx, mutations, [][]byte{}, startTS, commitTS)
// → nil の場合と同一挙動(競合なし)を確認

4. FSM レイヤーの統合テストがない

深刻度: 中

fsm.go の handleOnePhaseTxnRequest / handlePrepareRequest が r.ReadKeys を ApplyMutations に渡す経路が変更されましたが、FSM 経由で read-write 競合が正しく伝播するかを検証するテストがありません。

提案テストケース:

// kv/fsm_test.go (または既存の FSM テストファイル)
// pb.Request{ReadKeys: [][]byte{conflictedKey}} を使った handleOnePhaseTxnRequest 呼び出しで
// ErrWriteConflict が返ることを確認

5. マルチシャード (ShardedCoordinator) での read-write 競合テストがない

深刻度: 中

sharded_coordinator.go の prewriteTxn / dispatchSingleShardTxn で readKeys が PREPARE リクエストに渡されるようになりましたが、sharded レベルで read-write 競合が検知されるシナリオのテストがありません。

既存テストへの影響

全呼び出し箇所で nil が渡されるように機械的に更新されており、既存テストは壊れていません。問題なし。

まとめ

観点	評価
pebbleStore の新ロジックテスト	基本ケースは網羅
mvccStore の新ロジックテスト	不足
write-skew 並行テスト	不足
FSM 統合テスト	不足
ShardedCoordinator テスト	不足
空 readKeys / nil 互換性	不足
既存テスト破損	なし

最優先は mvccStore の読み取り競合テスト と write-skew 並行シナリオテスト の追加です。

claude · 2026-04-12T19:31:41Z

[Performance] レビュー結果

PR #499: store: add read-set validation to ApplyMutations for SSI

1. 不要なスライスコピー ( ~1923行)

readKeys := make([][]byte, 0, len(t.readKeys))
for _, k := range t.readKeys {
    readKeys = append(readKeys, k)
}

この for ループは [][]byte のスライスヘッダを1要素ずつコピーしているだけで、バイト列の deep copy にはなっていない。append([][]byte(nil), t.readKeys...) もしくは commit 後に t.readKeys を変更しないなら直接渡せる。1アロケーション余分に発生する。

2. apply ロック保持中の sequential per-key I/O ( ~483行)

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // Pebble点読み
        ...
    }
}

ApplyMutations は applyMu を保持したまま checkReadConflicts を呼ぶ。write-write 側の checkConflicts も同様の構造だが、read set はトランザクションの参照キー数に比例して大きくなりやすい（write set より一般的に大きい）。N キーについて逐次 Pebble point read が発生し、ロック保持時間が O(N) に伸びる。

write set のキーと read set のキーをまとめてバッチで引けるかを検討する余地がある（Pebble の NewBatch / MultiGet 相当）。現状の latestCommitTS がイテレータを毎回開くなら、イテレータの再利用も有効。

3. 全シャードへの readKeys 送信 ( ~277行, `prewriteTxn`)

multi-shard トランザクションの prepare フェーズで、各シャードのリクエストに ReadKeys: readKeys（トランザクション全体の read set）が丸ごと付与される:

req := &pb.Request{
    ...
    Mutations: append([]*pb.Mutation{prepareMeta}, grouped[gid]...),
    ReadKeys:  readKeys,   // ← 全 read key を全シャードへ
}

各シャードは自分が保持しないキーの conflict check を行うことになり、無駄な I/O と Raft ログ肥大化が生じる。本来は「各シャードが担当するキーに絞った read set のみ送る」べき。シャードが管轄外キーの latestCommitTS を引いた場合に何を返すかにも依存するが、少なくとも proto ペイロードサイズが不必要に増大する。

claude · 2026-04-12T19:32:53Z

[Data Loss] レビュー結果

対象PR: store: add read-set validation to ApplyMutations for SSI (#499)

全体評価

直接的なデータロストパスは検出されませんでした。 ただし、以下の点を記録します。

確認項目ごとの評価

1. Write/Delete操作でデータが意図せず失われるパスがないか ✅

pebbleStore.ApplyMutations（lsm_store.go:900）の実行順序：

applyMu ロック取得
checkConflicts（書き込み競合チェック）
checkReadConflicts（読み取り競合チェック） ← 今回追加
applyMutationsBatch（バッチへの書き込み）
b.Commit(pebble.Sync)（永続化）

ステップ2・3でエラーが返れば、applyMutationsBatch に到達しないため、データは書き込まれません。競合チェックと書き込みが applyMu ロック下でアトミックに実行される構造は正しいです。

mvccStore 側（mvcc_store.go）でも同様に、ロック下で checkConflictsLocked が書き込み前に実行されており、問題ありません。

2. エラーハンドリングの欠如によりデータが消失する可能性がないか ⚠️ 軽微

checkReadConflicts（lsm_store.go:864）に軽微な問題があります：

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // ← ctx が無視されている

引数で受け取った ctx を _ で捨て、context.Background() を使っています。呼び出し元のコンテキストがキャンセルされてもこのチェックは続行されます。

データロストへの影響: なし。チェックが完走するだけで、競合検出ロジックは正しく機能します。ただし、呼び出し側の ctx の一貫性という観点では checkConflicts と非対称です。

3. トランザクションのコミット/ロールバック処理でデータが失われないか ✅

adapter/redis.go から validateReadSet の呼び出しを削除した変更（2102-2107行）について：

旧コード（削除前）は ApplyMutations の外（ストアロック外）でread-set検証を行っていました。新コードは ApplyMutations 内のロック下で検証します。検証自体はなくなっておらず、より強い原子性の下で実行されるようになっています。データロストの心配はありません。

4. マルチシャードトランザクションにおける読み取り競合検出の抜け ⚠️ 注意（データロストではなく分離レベルの問題）

sharded_coordinator.go:prewriteTxn（266行）で、ReadKeys は 全シャード に送信されます：

req := &pb.Request{
    Phase:     pb.Phase_PREPARE,
    Mutations: append([]*pb.Mutation{prepareMeta}, grouped[gid]...),
    ReadKeys:  readKeys,  // 全シャードに同一の readKeys を送信
}

各シャードは自シャードが持たないキーについては latestCommitTS で exists=false を返し競合なしと判定します。

問題シナリオ: トランザクションがシャードAのキー K1 を読み取り、シャードBにのみ書き込む場合、K1 の読み取り競合はシャードAに問い合わせがされず検出されません（シャードBはK1を知らないため）。

これは write skew（書き込みスキュー）が防げないSSIの抜けであり、PR本文で謳う SSI の保証を完全には達成できないケースです。ただし、データが永続化されなくなる・消えるという意味での データロストではありません。

まとめ

観点	評価
書き込みのアトミック性	✅ 問題なし（applyMu ロック下で競合チェック→書き込みが直列化）
WAL/Raftへの永続化	✅ 問題なし（pebble.Sync コミット、ReadKeys はRaftログに含まれる）
エラー時のロールバック	✅ 問題なし（エラー時はバッチが Commit されない）
validateReadSet の削除	✅ 問題なし（同等の検証がストアロック下に移動）
checkReadConflicts の ctx 無視	⚠️ データロストではないが ctx 不整合
マルチシャード cross-shard 読み取り競合	⚠️ SSI の抜け（データロストではなく分離レベルの問題）

直接的なデータロストに該当する問題は検出されませんでした。

claude · 2026-04-12T19:32:54Z

[Concurrency] レビュー結果

以下 3 点の問題を検出しました。

[Bug] `checkReadConflicts` がコンテキストを無視している

ファイル: store/lsm_store.go

新規追加の checkReadConflicts はシグネチャでコンテキストを受け取りながら、内部で context.Background() を新たに生成しています。

// 追加されたコード
func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)  // ← 渡された ctx を捨てている

一方、既存の checkConflicts は正しく ctx を伝播させています:

func (s *pebbleStore) checkConflicts(ctx context.Context, mutations []*KVPairMutation, startTS uint64) error {
    ...
        ts, exists, err := s.latestCommitTS(ctx, mut.Key)  // ← ctx を伝播

影響: Raft のリーダー切り替え時など、FSM が上位レイヤーからコンテキストキャンセルを受け取った場合、checkConflicts はキャンセルに応答して早期リターンしますが、checkReadConflicts は context.Background() を使い続けるため、read keys の数が多い場合に不必要なブロッキングが発生します。

修正案:

func (s *pebbleStore) checkReadConflicts(ctx context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(ctx, key)

[Correctness] 複数シャードのトランザクションで SSI が保証されない TOCTOU ウィンドウ

ファイル: kv/sharded_coordinator.go — prewriteTxn

マルチシャードトランザクションの prewrite では、readKeys を全シャードに送信しています:

req := &pb.Request{
    Phase:     pb.Phase_PREPARE,
    Ts:        startTS,
    Mutations: append([]*pb.Mutation{prepareMeta}, grouped[gid]...),
    ReadKeys:  readKeys,  // ← 全シャードに同じ read set を送る
}

各シャードは自シャードの applyMu 下で独立して read set を検証します。下記シナリオで write skew が依然として発生します:

トランザクション T1: シャード A のキー K を読み取り (startTS=10)、シャード B のキー W に書き込む
シャード A の prepare: K の latestCommitTS=5 ≤ 10 → OK、W のロック書き込み完了
トランザクション T2: K にコミット (commitTS=25) — シャード A の applyMu は既に解放されているので成功
シャード B の prepare: K をシャード B で検索 → exists=false → 競合なし → T1 コミット成功

T1 は T2 による K への書き込みを見逃したまま commitTS=30 でコミットされ、SSI が破られます。

根本原因: read set の検証はシャードをまたいで原子的に行われていません。単一シャードトランザクションについては本 PR で正しく SSI が実装されていますが、マルチシャードトランザクションの SSI 保証はこの PR では不完全です。PR 説明・コメントへの明記、もしくは設計上の既知制限として issue 化を推奨します。

[Correctness] 2PC prepare フェーズで read set 検証が早すぎるタイミングで実施される

ファイル: kv/fsm.go — handlePrepareRequest

if err := f.store.ApplyMutations(ctx, storeMuts, r.ReadKeys, startTS, startTS); err != nil {

prepare (プリライト) 時に read set を検証しますが、上記の複数シャード問題の延長として、prepare → commit 間に read key が書き変わっても検知できません。prepare ではロックを書き込むだけで read key を保護しないため、commit フェーズまでの間に競合が発生し得ます。

single-shard の one-phase path (handleOnePhaseTxnRequest) では applyMu 下で検証と適用が原子的に行われるため問題ありません。2PC マルチシャード path のみ影響します。

サマリー

深刻度	問題	場所
中	でコンテキストが無視される（リーダー切り替え時に不必要なブロック）
中	マルチシャードトランザクションで SSI が不完全（TOCTOU、write skew が依然発生しうる）	,
低	2PC prepare の read set 検証タイミングが遅い競合を補足できない（上記の派生）

単一シャードトランザクションのロジック（mvcc_store.go の checkConflictsLocked、lsm_store.go の ApplyMutations での applyMu によるチェック→コミットの原子化）は正しく実装されています。

claude · 2026-04-12T19:33:15Z

[Consistency] レビュー結果

1. 重大: 多シャードトランザクションにおける read-write 競合の検出漏れ

場所: kv/sharded_coordinator.go:280

prewriteTxn（2PC の Prepare フェーズ）では、readKeys の全要素を書き込みシャードすべてに一律送付しています。

req := &pb.Request{
    Phase:     pb.Phase_PREPARE,
    Mutations: append([]*pb.Mutation{prepareMeta}, grouped[gid]...),
    ReadKeys:  readKeys,  // 全 readKeys を全ての write shard へ
}

これにより、書き込みセットに含まれないシャードに属する読み取りキーが一切チェックされない問題が発生します。

具体例（write skew が発生するシナリオ）

TXN1: read key_A (shard A), write key_B (shard B)
TXN2: write key_A (shard A), startTS > TXN1.startTS で commit 済み

TXN1 の prewriteTxn は shard B のみに Prepare を送付
shard B は key_A を自身のストアで検索 → 存在しない → 競合なしと判断
shard A には Prepare リクエストが届かない → key_A の競合が永久に検出されない
TXN1 が commit → key_A の stale read に基づいて書き込みが確定

単一シャード (dispatchSingleShardTxn) では readKeys が正しいシャードに届くため問題ありませんが、多シャードトランザクションかつ読み取りキーが書き込みシャード以外に存在する場合、SSI の保証が破れます。

修正案

prewriteTxn では、各 readKey をそのキーが属するシャードに対応した Prepare リクエストへのみ含める必要があります。書き込みセットに含まれないシャードが readKey を所有している場合は、そのシャードに対して書き込みなし・読み取り検証のみの Prepare リクエストを追加で送付する設計が必要です。

2. 軽微: `checkReadConflicts` でのコンテキスト破棄

場所: store/lsm_store.go:864

func (s *pebbleStore) checkReadConflicts(_ context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(context.Background(), key)

引数の ctx を破棄して context.Background() を使っています。呼び出し元（Raft FSM の apply タイムアウトなど）がキャンセル済みのコンテキストを渡しても無視されます。context.Background() を ctx に置き換えてください。

その他の観点（問題なし）

観点	判定
Raft を経由すべき操作がローカルで直接実行されていないか	✅ `ApplyMutations` は FSM の `Apply()` 内から呼ばれており、Raft ログコミット後に実行されている
`applyMu` による check-apply のアトミック性	✅ `lsm_store.go:906` の `s.applyMu.Lock()` が checkConflicts / checkReadConflicts / applyMutationsBatch を直列化している
readKeys のキーエンコーディング一貫性	✅ adapter は storage-level encoded keys (`redisStrKey`, `listMetaKey` 等) を `trackReadKey` に渡しており、mutation キーと同一エンコーディング
TTL とデータ本体の整合性	✅ `redisTTLKey(userKey)` も readKeys に追加されており、TTL の読み取りも競合検出対象になっている
`validateReadSet` 削除の妥当性	✅ 旧来の client 側での TOCTOU ありの事前チェックを廃止し、Raft 直列化の下でのアトミックな検証に移行した設計は正しい
proto / FSM 間の readKeys 伝搬	✅ `pb.Request.read_keys` (field 5) → FSM → `ApplyMutations` まで一貫して伝達されている

- Fix checkReadConflicts to propagate ctx instead of context.Background() (Concurrency review) - Route readKeys by shard ID in prewriteTxn so each shard only validates read keys that belong to it, and read keys on non-write shards are correctly checked by their owning shard (Consistency review) - Add mvccStore read-write conflict tests mirroring the pebbleStore coverage: conflict detected, no conflict when unchanged, nonexistent key, nil readKeys (Test Coverage review)

claude · 2026-04-12T19:40:22Z

[Performance] レビュー結果

問題1: `checkReadConflicts` が `applyMu` 保持中に O(|readKeys|) のディスクI/O を実行する（中〜高）

ファイル: store/lsm_store.go

// ApplyMutations 内
s.applyMu.Lock()
defer s.applyMu.Unlock()
...
if err := s.checkReadConflicts(ctx, readKeys, startTS); err != nil { ... }

checkReadConflicts は各 read key に対して latestCommitTS を呼び出す。
latestCommitTS は1件あたり db.NewIter → iter.First() のランダムディスクI/Oを発生させる。

これが applyMu を保持したまま直列実行 されるため、read set が大きいトランザクション（read-heavy な SSI ワークロードでは mutations より readKeys が多くなりがち）では、ロック保持時間が O(|readKeys|) に比例して長くなる。
結果として、並行する他のトランザクションのコミットが待たされる。

既存の checkConflicts（write-write 検出）も同じパターンだが、write set は通常 read set よりはるかに小さいため、影響が大きく異なる。

改善案: read key の latestCommitTS チェックを Pebble の単一 NewIter で範囲スキャンしてまとめる（イテレータの生成コスト削減）か、将来的にはブルームフィルタ等で no-conflict を早期検出することを検討。

問題2: `groupReadKeysByShardID` で内部スライスの pre-alloc がない（低）

ファイル: kv/sharded_coordinator.go

grouped := make(map[uint64][][]byte)
for _, key := range readKeys {
    gid := c.engineGroupIDForKey(key)
    ...
    grouped[gid] = append(grouped[gid], key)  // 容量ヒントなし
}

全キーが同一シャードに集中する場合（シングルシャードのトランザクションでは common case）、grouped[gid] のスライスが 1 件ずつ追加されるため O(log N) の再アロケーションが発生する。

改善案:

// 最悪ケースで全キーが1シャードに集中しても realloc なし
grouped := make(map[uint64][][]byte, 1)
for _, key := range readKeys {
    gid := c.engineGroupIDForKey(key)
    if gid == 0 {
        continue
    }
    if _, ok := grouped[gid]; !ok {
        grouped[gid] = make([][]byte, 0, len(readKeys))
    }
    grouped[gid] = append(grouped[gid], key)
}

または単純に len(readKeys) を capacity として初期スライスを用意するだけでも realloc を防げる。

確認済みで問題なしの点

adapter/redis.go の readKeys コピー: t.readKeys が map[string][]byte 型のため、[][]byte への変換ループは必要。make([][]byte, 0, len(t.readKeys)) による cap 事前確保も適切で realloc なし。
validateReadSet の削除: コメントに明記の通り、ApplyMutations 内の applyMu 下での検証に統合することで不要な LatestCommitTS の二重呼び出しを除去しており、I/O削減として正しい方向。
nil/空 readKeys の early return: groupReadKeysByShardID は len(readKeys) == 0 で早期 return しており、非SSIパスのオーバーヘッドはなし。

claude · 2026-04-12T19:40:56Z

[Test Coverage] レビュー結果

PR #499: store: add read-set validation to ApplyMutations for SSI

概要

ストア層・MVCC層の単体テストは適切に追加されており、主要なハッピーパスとバックワード互換性は網羅されています。
ただし、以下の 4件の不足 を確認しました。

不足テスト

1. のユニットテストなし【高】

対象ファイル:

新規追加された関数に専用テストがありません。特に以下の挙動が未検証です。

ルーティング不能な read key がサイレントに検証スキップされることで、SSI の保護漏れが生じる可能性があります。
意図的な設計か、バグかをテストで明文化すべきです。

提案テストケース:

2. マルチシャード SSI の統合テストなし【高】

対象ファイル: ( パス)

パスは単体テスト相当のカバーがありますが、（マルチシャード 2PC）パスで read keys が各シャードに正しく分配されることを確認するテストがありません。

提案テストケース:

3. コーディネーター層で readKeys が Request に含まれることを検証するテストなし【中】

対象ファイル:

既存のがに更新されただけで、実際にを渡したときにプロトバッファに正しく含まれるかを確認するテストがありません。

提案テストケース:

4. 複数 read key のうち一部のみが競合するケースのテストなし【低】

対象ファイル: ,

現在のテストは read key が 1 つのケースのみです。複数の read key を渡したとき、最初のコンフリクトで正しく早期リターンするかを検証するテストがありません。

提案テストケース:

既存テストへの影響

の全呼び出し箇所（19ファイル）が渡しに正しく更新されており、既存テストの破壊はありません。

総評

ストア層の SSI 基本動作テストは充実しています。上記のうち 1, 2 は分散KVSとして特に重要な未検証領域のため、マージ前に対処することを推奨します。

claude · 2026-04-12T19:41:07Z

[Data Loss] レビュー結果

対象PR: store: add read-set validation to ApplyMutations for SSI (#499)

問題 1: `groupReadKeysByShardID` で gid=0 のread keyが無言でスキップされる

ファイル: kv/sharded_coordinator.go

func (c *ShardedCoordinator) groupReadKeysByShardID(readKeys [][]byte) map[uint64][][]byte {
    if len(readKeys) == 0 {
        return nil
    }
    grouped := make(map[uint64][][]byte)
    for _, key := range readKeys {
        gid := c.engineGroupIDForKey(key)
        if gid == 0 {
            continue  // ← gid=0 のキーを無言でドロップ
        }
        grouped[gid] = append(grouped[gid], key)
    }
    return grouped
}

engineGroupIDForKey がルーティング不能な場合にgid=0を返すと、そのキーへのread-set conflict checkが 一切実施されない。
シャードが追加・再配置されたタイミングや、キーの所属シャードが判定できないケースで、read keyがここで黙って除外されると write-skewが検出されずトランザクションがコミットされる 可能性がある。

gid=0 が「ルーティング不能」の意味なのか「シャード0」の意味なのかを明示し、ルーティング不能の場合は ErrUnroutedKey などのエラーを返すべきか検討が必要。

問題 2: `validateReadSet` 除去による検証範囲の変化

ファイル: adapter/redis.go

// 削除された旧コード
if err := txn.validateReadSet(dispatchCtx); err != nil {
    return err
}

コメントには「redundant」とあるが、旧 validateReadSet がクロスシャード全体を対象に一括検証していた場合、新実装（各シャードの ApplyMutations 内でのチェック）と意味が等価かどうかが差分から確認できない。
validateReadSet の実装内容が削除前と新しいパスで 同じキー集合を同じ条件で検証していることを確認するテストがない点は不安要素。

問題 3 (軽微): `handlePrepareRequest` でのread-set再検証とRaft replayの整合性

kv/fsm.go の handlePrepareRequest で r.ReadKeys を ApplyMutations に渡すことで、followerのRaft log replay時にも同じread-set validationが実行される。Raftの順序保証があるため通常は問題ないが、followerのMVCC状態がleaderと異なるタイミング（遅延apply中）でreplay時にvalidationが失敗すると、FSMが divergeするリスクがある。既存のwrite-write conflict checkも同様の前提で動作しているなら許容範囲だが、明示的なコメントが欲しい。

claude · 2026-04-12T19:41:33Z

[Concurrency] レビュー結果

問題なし (単一シャード構成)

単一シャードの Coordinate / pebbleStore / mvccStore パスは問題ありません。

pebbleStore.ApplyMutations: dbMu.RLock → applyMu.Lock → mtx.Lock のロック順序は一貫しており、デッドロックはない
checkConflicts と checkReadConflicts はどちらも applyMu 下でアトミックに実行されるため、check-then-commit の race はない
checkReadConflicts は既に dbMu.RLock を保持している caller から呼ばれる内部関数 s.latestCommitTS を正しく使用している
mvccStore.checkConflictsLocked は s.mtx.Lock() 下で呼ばれており安全
Raft FSM は apply を直列に処理するため、リーダー切り替え後も競合検知は正しく動作する
フォロワー→リーダーのプロキシ (coordinator.go:redirect) でも ReadKeys が正しく伝搬されている
context のキャンセルは latestCommitTS に渡されており適切

問題あり: `ShardedCoordinator` における cross-shard 読み取り検証の欠落

深刻度: High — SSI の正確性に関わる regression

問題 1: `prewriteTxn` でのread-only シャード見落とし

// sharded_coordinator.go
for _, gid := range gids {   // gids = 書き込みのあるシャードのみ
    req := &pb.Request{
        ReadKeys: groupedReadKeys[gid],  // 書き込みシャードに属するreadキーのみ
    }
}

gids は書き込みミューテーションを持つシャードのみを含む。
トランザクションがシャードAのキーを読み取り、シャードBのキーのみを書き込む場合、groupedReadKeys[A] はどの Prepare リクエストにも含まれず、シャードAの読み取り-書き込みコンフリクト検証がスキップされる。

問題 2: `dispatchSingleShardTxn` での readKeys の誤ルーティング

// 全readKeysを単一の書き込みシャード(gid)に送る
onePhaseTxnRequest(startTS, commitTS, primaryKey, elems, readKeys)

readKeys に別シャードのキーが含まれる場合、そのシャードのローカル pebbleStore にそのキーのデータは存在しないため、checkReadConflicts は「conflict なし」と誤判定する。

変更前との比較

変更前の validateReadSet (adapter/redis.go) は t.server.store.LatestCommitTS(ctx, key) でキーごとに正しいシャードへルーティングしていた。今回それを削除したことで、シャード間の read-write conflict 検証が失われている。

修正案

prewriteTxn: 書き込みのないシャードであっても readKeys を持つシャードを gids に追加して Prepare フェーズに含める (read-only validate request)
または: ShardedCoordinator では validateReadSet を adapter 層で維持し、FSM 内の検証はシングルシャード構成のみに適用する

Minor: dead code

adapter/redis.go:1893 の validateReadSet 関数はどこからも呼ばれなくなっているが、定義だけ残っている。混乱を防ぐために削除を推奨。

claude · 2026-04-12T19:41:42Z

[Test Coverage] レビュー結果

PR #499: store: add read-set validation to ApplyMutations for SSI

概要

ストア層・MVCC層の単体テストは適切に追加されており、主要なハッピーパスとバックワード互換性は網羅されています。
ただし、以下の 4件の不足 を確認しました。

不足テスト

1. `groupReadKeysByShardID` のユニットテストなし【高】

対象ファイル: kv/sharded_coordinator.go

新規追加された関数に専用テストがありません。特に以下の挙動が未検証です。

if gid == 0 {
    continue  // ルーティングできないキーを無言でスキップ
}

ルーティング不能な read key がサイレントに検証スキップされることで、SSI の保護漏れが生じる可能性があります。
意図的な設計か、バグかをテストで明文化すべきです。

提案テストケース:

TestShardedCoordinator_groupReadKeysByShardID_NilWhenEmpty
TestShardedCoordinator_groupReadKeysByShardID_GroupsByShardID
TestShardedCoordinator_groupReadKeysByShardID_SkipsUnroutableKeys

2. マルチシャード SSI の統合テストなし【高】

対象ファイル: kv/sharded_coordinator.go (prewriteTxn パス)

dispatchSingleShardTxn パスは単体テスト相当のカバーがありますが、prewriteTxn（マルチシャード 2PC）パスで read keys が各シャードに正しく分配されコンフリクト検出が機能することを確認するテストがありません。

提案テストケース:

// シャードAのキーを書き、シャードBのキーを読む → シャードBのPREPAREでコンフリクト検出
func TestShardedCoordinator_CrossShardReadConflict(t *testing.T)

3. コーディネーター層で readKeys が Request に含まれることを検証するテストなし【中】

対象ファイル: kv/coordinator_txn_test.go

既存の TestCoordinateDispatchTxn_UsesOnePhaseRequest が nil に更新されただけで、実際に readKeys を渡したときにプロトバッファ Request.ReadKeys に正しく含まれるかを確認するテストがありません。

提案テストケース:

func TestCoordinateDispatchTxn_ReadKeysIncludedInRequest(t *testing.T)
// readKeys を指定して dispatchTxn を呼び、tx.reqs[0][0].ReadKeys に期待値が含まれることを assert する

4. 複数 read key のうち一部のみが競合するケースのテストなし【低】

対象ファイル: store/lsm_store_txn_test.go, store/mvcc_store_concurrency_test.go

現在のテストは read key が 1 つのケースのみです。複数の read key を渡したとき、競合するものがあれば正しく ErrWriteConflict を返すかを検証するテストがありません。

提案テストケース:

func TestPebbleStore_ApplyMutations_ReadConflict_MultipleReadKeys_PartialConflict(t *testing.T)
// k1(TS=5), k2(TS=20), k3(TS=5) をセットし、startTS=10 で [k1, k2, k3] を readKeys に指定
// k2 でコンフリクト → ErrWriteConflict, conflictKey == k2

既存テストへの影響

ApplyMutations の全呼び出し箇所（19ファイル）が nil 渡しに正しく更新されており、既存テストの破壊はありません。

総評

ストア層の SSI 基本動作テストは充実しています。上記のうち 1, 2 は分散KVSとして特に重要な未検証領域のため、マージ前に対処することを推奨します。

Add validateReadOnlyShards to send read-set validation requests to shards that have read keys but no mutations. Previously, a read key on shard A would go unchecked if the transaction only wrote to shard B, allowing write skew across shards. Also fix checkReadConflicts to propagate the caller context (already addressed in prior commit for the function signature, this ensures consistent usage). Add partial-conflict test case where only some read keys conflict.

claude · 2026-04-12T19:46:38Z

[Data Loss] レビュー結果

総評

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

write-write衝突チェックと同様に、新しい read-write 衝突チェックも配下でアトミックに実行されており、競合チェック → batch commit のシリアライズが正しく保たれています。以下に各観点の確認結果を記載します。

確認項目

1. Write/Delete 操作のデータ消失パス

問題なし。
checkReadConflicts は checkConflicts (write-write) の直後、applyMutationsBatch の前に呼ばれます。いずれかのチェックがエラーを返した場合、batch への書き込みは一切行われません (store/lsm_store.go の ApplyMutations 内の制御フロー)。

2. WAL/Raft ログへの永続化

問題なし。
b.Commit(pebble.Sync) によるfsync保証は変更されていません。read-conflict チェックはこの commit 以前に実施されるため、永続化の保証に影響しません。

3. エラーハンドリングの欠如によるデータ消失

問題なし。
checkReadConflicts はエラーを適切に返しており、無視されるパスは存在しません。

4. トランザクションのコミット/ロールバック処理

問題なし。
2PC パスの prepare フェーズ (handlePrepareRequest) では r.ReadKeys を渡して read-conflict を検証し、commit フェーズ (applyCommitWithIdempotencyFallback) では nil を渡します。これは 2PC において read 衝突検証を prepare 時に完結させる正しい設計です。

5. TTL/Expire 処理での意図しないデータ削除

問題なし。
TTL 関連の処理は本 PR で変更されていません。

6. スナップショット/バックアップ関連のデータ欠損

問題なし。
スナップショット/バックアップパスは変更されていません。

注意事項 (データロストではなく SSI 正確性のギャップ)

groupReadKeysByShardID での gid == 0 キーの無声スキップ (kv/sharded_coordinator.go)

if gid == 0 {
    continue  // ルーティング不明のキーを無声でスキップ
}

multi-shard の prewrite パスにおいて、engineGroupIDForKey が 0 を返すキー(ルーティングテーブルに存在しないキー)は read-conflict チェックの対象外となります。
これはデータロストではなく、write skew(書き込みスキュー)が検出されないケースが生じうるという SSI 保証のギャップです。
実際に read したキーが必ず既知のシャードにルーティングされるのであれば実用上の問題はありませんが、意図的な設計かどうか、コメントでの明示を検討してください。

claude · 2026-04-12T19:47:23Z

[Consistency] レビュー結果

✅ 正しい実装

applyMu による原子的な check-then-commit
lsm_store.go:906 にて applyMu.Lock() が checkConflicts → checkReadConflicts → applyMutationsBatch 全体を直列化している。TOCTOU なし。

validateReadSet の除去は正しい
従来の validateReadSet(dispatchCtx) は Raft ログ適用前に leader 側で独立して行われていたため、 Dispatch 後に競合が起きても検知できなかった。Raft ログに ReadKeys を含め FSM 内で applyMu 下で検証することで真に原子的な SSI が実現されている。

プロトコル層の伝播
proto/internal.proto に read_keys (field 5) が追加され、フォロワーへの Raft ログ再適用でも同じ readKeys が渡される点は一貫性上正しい。

⚠️ 問題点

[重大] 多シャード 2PC: read-set 検証が PREPARE 時のみ実施される

sharded_coordinator.go:282 の prewriteTxn では readKeys が PREPARE フェーズの各シャードへ配布され、handlePrepareRequest → ApplyMutations(ctx, storeMuts, r.ReadKeys, startTS, startTS) で checkReadConflicts が実行される。

しかし COMMIT フェーズ (applyCommitWithIdempotencyFallback) では readKeys = nil が渡されており、PREPARE 完了〜COMMIT 完了の間に別トランザクションが read key に書き込んでも検知されない。

Txn A:  PREPARE (checkReadConflicts OK @ startTS)
Txn B:                  ← write to one of A's readKeys ←
Txn A:                  COMMIT (readKeys=nil, 見逃し)

SSI が必要な多シャードトランザクションで write skew を防ぎきれないウィンドウが残る。

[軽微] `groupReadKeysByShardID` でルート不明キーが無言スキップされる

sharded_coordinator.go の groupReadKeysByShardID 内で gid == 0（ルート未解決）のキーが continue でスキップされる。シャード移行中などで一時的にルートが解決できない場合、そのキーへの書き込み競合が SSI 検査から外れる。スキップ理由を log.Warn 等で記録するか、解決できない場合はトランザクション自体をエラーにすることを検討してほしい。

claude · 2026-04-12T19:47:34Z

[Performance] レビュー結果

1. `checkReadConflicts` でのイテレータ N 回アロケーション（高重要度）

ファイル: store/lsm_store.go

checkReadConflicts は applyMu.Lock() 保持中に latestCommitTS を readKey の数だけ逐次呼び出す。

func (s *pebbleStore) checkReadConflicts(ctx context.Context, readKeys [][]byte, startTS uint64) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(ctx, key)  // 毎回 Pebble イテレータを new/close
        ...
    }
}

latestCommitTS の実装は s.db.NewIter() で毎回新しいイテレータを生成して Close() する（822行付近）。Pebble のイテレータ生成は各レベルのマージビューを構築するため、ポイントアクセスとしては比較的コストが高い。

影響: N 個の read key がある場合、N 個のイテレータを逐次生成・破棄しつつ applyMu（コミットのシリアライズロック）を保持し続ける。既存の checkConflicts（書き込みセット）も同様のパターンだが、SSI では read セットが write セットより大きくなりやすく、ロック保持時間が比例的に増加して全トランザクションコミットをブロックする。

改善案: 単一の Pebble イテレータを生成し、SeekGE で各キーを走査することでイテレータのアロケーション回数を O(N) → O(1) に削減できる（既存の checkConflicts と同様の最適化パスが適用可能）。

2. `adapter/redis.go:commit()` の readKeys コピーループ（軽微）

readKeys := make([][]byte, 0, len(t.readKeys))
for _, k := range t.readKeys {
    readKeys = append(readKeys, k)
}

t.readKeys が map[string][]byte なのでスライスへの変換は必要だが、ループを append 一発に置き換えられる：

readKeys := make([][]byte, 0, len(t.readKeys))
for _, k := range t.readKeys {
    readKeys = append(readKeys, k)
}
// ↑ これは機能的には正しいが、慣用的には以下でも同じ

cap 指定は正しいので、実質的な問題はない。現状維持でも許容範囲内。

3. `groupReadKeysByShardID` のマップ容量未指定（軽微、マルチシャードのみ）

ファイル: kv/sharded_coordinator.go

grouped := make(map[uint64][][]byte)  // 初期容量なし

マルチシャードトランザクション時のみ呼ばれるパスだが、多数の read key が少数シャードに集まる場合にマップの再ハッシュが発生しうる。make(map[uint64][][]byte, len(gids)) のように呼び出し元が持つ gids のサイズをヒントに与えると良い（ただし gids の参照をこの関数に渡す必要がある）。

総括

最も重要な懸念は 1 の applyMu 保持中の N 回イテレータ生成 で、read セットが大きいトランザクション（例: MULTI/EXEC で多数の GET + 少数の SET）において全コミットのスループットに影響しうる。2・3 は軽微。

claude · 2026-04-12T19:48:01Z

[Test Coverage] レビュー結果

概要

PR #499 は ApplyMutations に readKeys [][]byte パラメータを追加し、SSI (Serializable Snapshot Isolation) の Read-Write コンフリクト検出をストア層へ移動する変更です。

カバーされている項目 (良好)

pebbleStore.checkReadConflicts: 5 件のユニットテスト (lsm_store_txn_test.go)
- 基本コンフリクト・コンフリクトなし・存在しないキー・nil readKeys・部分コンフリクト
mvccStore.checkConflictsLocked (read側): 4 件のユニットテスト (mvcc_store_concurrency_test.go)
ApplyMutations シグネチャ変更: 既存テストへの nil 引数追加は全ファイルで適切に対応済み

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

1. `ShardedCoordinator.groupReadKeysByShardID` — テストなし

新規追加の関数にテストが一切ありません。

提案するテストケース:

// gid==0 (ルーティング不能キー) はスキップされること
// 複数キーが同一シャードに集約されること
// 異なるシャードに振り分けられること
// 空/nil のreadKeysでnilマップが返ること

2. `ShardedCoordinator.validateReadOnlyShards` — テストなし

マルチシャードトランザクションにおいて「書き込みがないシャードの読み込みキーを検証する」最も重要な新ロジックにテストがありません。

提案するテストケース:

// readOnlyシャードへのコンカレント書き込みが検出されること
// validateReadOnlyShards失敗時にprewriteされたトランザクションが正しくabortされること
// c.groups[gid] が存在しないキーはスキップされること (サイレントスキップの意図確認)

3. マルチシャードSSIのE2Eテストなし

sharded_coordinator_txn_test.go に ReadKeys を使った統合テストがありません。

提案するテストケース:

// Txn: shard-A に書き込み、shard-B のキーを読み込み
// → コミット前に shard-B のキーに別トランザクションが書き込む
// → 元のTxnがErrWriteConflictで失敗すること

4. `onePhaseTxnRequest` で ReadKeys が実際に pb.Request に含まれるか未検証

coordinator_txn_test.go の TestCoordinateDispatchTxn_UsesOnePhaseRequest は nil を渡すのみで、非nil ReadKeys が pb.Request.ReadKeys に正しくセットされるかを検証していません。

提案するテストケース:

_, err := c.dispatchTxn([]*Elem[OP]{...}, startTS, 0, [][]byte{[]byte("rkey")})
// tx.reqs[0][0].ReadKeys == [][]byte{[]byte("rkey")} であること

5. `redis.go` のE2Eパス変更のカバレッジ不足

TestRedisTxnValidateReadSetDetectsStaleListMeta は削除された独立の validateReadSet を直接呼び出してテストしていますが、PR後は ReadKeys が Dispatch → ApplyMutations 経由で検証されます。この新しいパスのE2Eテストがありません。

提案するテストケース:

// Redis MULTI/EXEC で listキーをWATCH相当で読み込み
// EXEC前にそのキーを別のルートで更新
// EXEC がコンフリクトエラーで失敗すること (ApplyMutations経由)

6. `pebbleStore` の並行 Read-Conflict テストなし

mvccStore には TestMVCCConcurrentApplyMutations (並行テスト) がありますが、pebbleStore.checkReadConflicts には並行アクセス下でのテストがありません。

提案するテストケース:

// 複数goroutineが同一readKeyに対してApplyMutationsを並行実行
// checkReadConflictsとcheckConflictsの間にレース条件がないこと

7. readKeys にライトキーと同一のキーが含まれる場合

書き込みキー (mutations) と読み込みキー (readKeys) が重複する場合の挙動が未テストです。

総評

store 層の新機能 (checkReadConflicts, checkConflictsLocked) は十分にカバーされています。一方、ShardedCoordinator の新ロジック (groupReadKeysByShardID, validateReadOnlyShards) にテストが全くない点が最大の懸念事項です。このコードパスはマルチシャード環境での SSI 保証の要であり、バグが混入しても単体テストで発見できません。最低限、項目 2 と 3 のテスト追加を推奨します。

claude · 2026-04-12T19:58:14Z

[Test Coverage] レビュー結果

PR #499: store: add read-set validation to ApplyMutations for SSI

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

store/lsm_store_txn_test.go: checkReadConflicts を 5 ケースで網羅（基本的な競合、競合なし、存在しないキー、nil 渡し、部分的競合）
store/mvcc_store_concurrency_test.go: mvccStore.checkConflictsLocked の読み取り競合を 2 ケースでカバー
既存テスト: ApplyMutations の全呼び出しに nil を渡すよう更新済み（リグレッションなし）
TestNormalizeRetryableRedisTxnErrZSetKey: ZSet 内部キーのエラー正規化

テスト不足 ❌

1. `validateReadOnlyShards` にテストなし（重大）

kv/sharded_coordinator.go の新関数 validateReadOnlyShards は、書き込みのないシャードで read-write 競合を検出する重要なロジックです。この関数に対するテストが一切存在しません。

提案するテストケース:

// TestShardedCoordinator_validateReadOnlyShards_DetectsConflict
// 書き込みシャードと別シャードに read key があり、その key が startTS 後に更新された場合 → ErrWriteConflict
// TestShardedCoordinator_validateReadOnlyShards_NoConflictWhenUnchanged
// read-only シャードの key が startTS 以前に更新された場合 → 競合なし
// TestShardedCoordinator_validateReadOnlyShards_SkipsWriteShards
// 書き込みシャードに属する read key は二重チェックされない

2. `groupReadKeysByShardID` にテストなし

gid == 0 (ルーティング不明) のキーがスキップされることの確認
空の readKeys スライスで nil が返ることの確認

3. SSI の並行テストが `readKeys=nil` のみ

既存の並行テスト (TestMVCCConcurrentApplyMutations など) は全て nil を渡すよう更新されただけで、非 nil の readKeys を使った SSI が並行アクセス下で正しく機能するかを検証するテストがありません。

提案するテストケース:

// TestMVCCStore_ConcurrentReadWriteConflict_SSI
// Goroutine A: key1 を読み取り、key2 を書き込む
// Goroutine B: key2 を読み取り、key1 を書き込む（同時実行）
// → 少なくとも一方が ErrWriteConflict を返すことを検証

4. `handlePrepareRequest` の readKeys パス (2PC) にテストなし

kv/fsm.go の handlePrepareRequest では r.ReadKeys を ApplyMutations に渡しますが、2PC (PREPARE フェーズ) で readKeys が正しく検証されるかのテストがありません。

5. エンドツーエンド SSI テストなし

Redis の MULTI/EXEC を使ったトランザクションで、読み取り対象のキーが並行ライターによって更新された場合に、Redisプロトコルレベルで正しくエラーが返るかの統合テストがありません。

提案するテストケース:

// TestRedisMultiExecSSIConflict
// 1. MULTI; GET key1; SET key2 val; EXEC
// 2. 実行中に別のクライアントが key1 を更新
// → EXEC がエラーを返すことを確認

claude · 2026-04-12T19:59:06Z

[Concurrency] レビュー結果

問題 1 — `validateReadOnlyShards` が `context.Background()` を使用（重大度: 中）

場所: kv/sharded_coordinator.go:630

ctx := context.Background()   // ← 呼び出し元のコンテキストを無視
for gid, keys := range groupedReadKeys {
    ...
    ts, exists, err := g.Store.LatestCommitTS(ctx, key)

呼び出しスタックには redisDispatchTimeout などのタイムアウトが設定されたコンテキストが存在するが、LatestCommitTS にはそれが渡されない。リーダー不在や一時的な障害で LatestCommitTS がブロックした場合、この goroutine はタイムアウトなしに永久に待ち続ける。

修正案: prewriteTxn のシグネチャに ctx context.Context を加えて validateReadOnlyShards へ伝搬する。Dispatch の呼び出し元コンテキストはすでに存在する。

問題 2 — `groupReadKeysByShardID` で `gid == 0` のキーをサイレントに無視（重大度: 中）

場所: kv/sharded_coordinator.go:607

gid := c.engineGroupIDForKey(key)
if gid == 0 {
    continue  // ルーティングエントリが存在しないキーを黙って脱落させる
}

engineGroupIDForKey がルーティングテーブルにキーを見つけられないと 0 を返す。その読みキーは groupedReadKeys に追加されず、write shards への PREPARE にも validateReadOnlyShards にも渡らないため、そのキーへの read-write 競合が検出されない。

一方、commitPrimaryTxn は同じ gid == 0 を ErrInvalidRequest として扱う (同ファイル:304)。読みキーだけ別扱いになっているのは不整合であり、write skew が発生しうる。

修正案: ルーティングに失敗した読みキーがある場合は ErrInvalidRequest などのエラーを返す（もしくは明示的に「ルーティングなしのキーは無視する」という設計意図をコメントで補足する）。

問題 3 — 多シャード 2PC における PREPARE→COMMIT ウィンドウでの read-set 競合（重大度: 高・設計上の制約）

場所: kv/sharded_coordinator.go:283–296、kv/fsm.go:316、kv/fsm.go:420

単一シャード (handleOnePhaseTxnRequest) では applyMu.Lock() 下で read-set チェックと書き込みが不可分に実行されるため、SSI は完全に成立する。

多シャード 2PC の場合は以下のウィンドウが存在する:

[T1] shard-A PREPARE (applyMu 下で ReadKeys チェック → write keys をロック)
[T2] shard-A の読みキー k1 に書き込み (ロックなし → 成功)
[T1] shard-B PREPARE
[T1] validateReadOnlyShards
[T1] COMMIT (applyCommitWithIdempotencyFallback: ReadKeys=nil → read-set を再検査しない)

handlePrepareRequest が r.ReadKeys を渡すのは正しいが、COMMIT フェーズ (applyCommitWithIdempotencyFallback) は readKeys=nil で ApplyMutations を呼ぶため、PREPARE 後に発生した read-set への書き込みは検出されない。読みキーはロックされないので T2 は T1 の PREPARE を見てもブロックされない。

また validateReadOnlyShards はラフト FSM の外（ローカルストア直接アクセス）で呼ばれるため、Raft ログの未適用エントリとの競合も考慮が必要。

影響範囲: 単一シャード・単一 Raft グループのトランザクションは問題なし。多シャード 2PC の SSI 保証は Percolator 設計上の制約と同様に PREPARE→COMMIT ウィンドウ内での違反を防げない。

推奨対応: PR 説明・コメントに「多シャード 2PC では PREPARE 時刻以降の read-set 競合を検出できない（Percolator の既知の制約）」と明記し、将来の改善課題として記録する。または read 対象キーにも PREPARE ロックを置く shared-lock 方式への移行を検討する。

確認済み（問題なし）

pebbleStore.ApplyMutations のロック順序: dbMu.RLock() → applyMu.Lock() の順序は一貫しており、checkConflicts / checkReadConflicts / applyMutationsBatch は applyMu.Lock() 下でアトミックに実行される（コメント通り）。
mvccStore.checkConflictsLocked: mtx.Lock() で完全に直列化されており正しい。
Raft リーダー切り替え: FSM の ApplyMutations はすべてのノードで同じ Raft ログエントリを同じ順序で適用するため、リーダー切り替え後も確定したログエントリに対しては冪等性が保たれる。
goroutine リーク / チャネルブロッキング: 新規 goroutine は起動されておらず、問題なし。
フォロワー→リーダーへのプロキシ (LeaderRoutedStore.ApplyMutations): シグネチャ変更のみで既存のロジックに変更なし。問題なし。
コンテキストキャンセル（単一シャードパス）: dispatchTxn → onePhaseTxnRequest のパスでは呼び出し元のコンテキストが正常に伝搬される。

claude · 2026-04-12T20:04:01Z

[Consistency] レビュー結果

概要

SI → SSI へのアップグレードの基本設計は正しく、シングルシャードパスの一貫性は向上している。ただし、マルチシャードトランザクションの読み取り専用シャードに対するread-write conflict検出がRaftを経由しないという重要な問題がある。

問題1 — 重要: `validateReadOnlyShards` がRaftを経由しない（線形化可能性の懸念）

kv/sharded_coordinator.go の validateReadOnlyShards:

for _, key := range keys {
    ts, exists, err := g.Store.LatestCommitTS(ctx, key)  // Raftを経由しないローカル読み取り
    if exists && ts > startTS {
        return errors.WithStack(store.NewWriteConflictError(key))
    }
}

書き込みシャードへの readKeys は PREPARE リクエストに乗ってRaftログを通じFSMのapplyロック下で検証される（正しい）。しかし、書き込みのない読み取り専用シャードへのキーは、Raft外でローカルストアに直接アクセスして検証される。

具体的なwrite skewシナリオ:

Txn A が shard2 のキー X を読み取り（startTS=100）、shard1 のキー Y に書き込む
Txn A の PREPARE が shard1 を通じてコミット完了
Txn B が shard2 のキー X に書き込み、TS=105 でコミット（Raftログに到達）
validateReadOnlyShards がローカル状態を読む時点で Txn B の変更がまだ未適用 → LatestCommitTS(X) が古い値を返す
Txn A がコミット → X の変更を見逃したまま書き込みが通過 → write skew

なお、旧コードの adapter 層の validateReadSet も同様に Raft 外で実行されていたため、マルチシャードケースは既存の制約の引き継ぎでもある。ただし本PRのコメントには「read-write conflicts are checked atomically」と記載されており、読み取り専用シャードについてもこの保証が成立するかのように読める点は修正が必要。

修正案: FSMが空のmutation listを含む検証専用PREPAREリクエストを受け付けるか、read-indexを使ったリニアライズド読み取りを検討する。短期対応として、コメントに「読み取り専用シャードの検証はRaft外（ベストエフォート）」と明示することを推奨。

問題2 — 軽微: 不明グループのサイレントスキップ

kv/sharded_coordinator.go validateReadOnlyShards:

g, ok := c.groups[gid]
if !ok {
    continue  // conflict検出をスキップ
}

ルーティングテーブルに存在するIDで groups に見つからない場合、そのシャードのread-write conflictが検出されずにコミットが通過する。ErrShardNotFound 相当のエラーを返すか、少なくともログに警告を出すべき。

問題3 — 軽微: `validateReadOnlyShards` で `context.Background()` を使用

ctx := context.Background()
for gid, keys := range groupedReadKeys {
    ...
    ts, exists, err := g.Store.LatestCommitTS(ctx, key)

トランザクション本体のコンテキスト（タイムアウト・キャンセル付き）が引き継がれていない。呼び出し元の context を受け取るよう修正することを推奨。

正しく実装されている点

シングルシャードパス: readKeys がRaftログに含まれ、FSMの applyMu ロック下で checkConflicts と checkReadConflicts が原子的に実行される。これはSI→SSI への正しい改善。
マルチシャードの書き込みシャード: readKeys が PREPARE リクエストに含まれ、Raft FSM下で検証される。
pebble/mvcc 両ストア: checkConflicts と checkReadConflicts が同一ロック下で直列化されており、書き込みとの原子性が保たれている。
アダプタ層の二重検証削除: validateReadSet を adapter 層から削除し ApplyMutations に集約することで、TOCTOU 窓が縮小された。
2PC PREPARE フェーズ: readKeys を PREPARE 時に検証しており、コミット前の適切なタイミングで競合を検出できる。

claude · 2026-04-12T20:04:08Z

[Test Coverage] レビュー結果

追加されたテスト（良い点）

store 層のテストは充実しています:

TestPebbleStore_ApplyMutations_ReadConflict 系 5ケース（競合あり/なし/非存在キー/nil/部分的競合）
TestMVCCStore_ApplyMutations_ReadConflict 系 4ケース
既存テストへの nil 追加も適切

不足しているテスト

1. `validateReadOnlyShards` のユニットテストがない（Critical）

kv/sharded_coordinator.go に追加された validateReadOnlyShards は、マルチシャードトランザクションにおいてライトのないシャード上の読み取りキーを検証する重要なSSIロジックです。しかし、このメソッドのテストが一切存在しません。

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

// 読み取り専用シャードで競合が発生するケース
func TestShardedCoordinator_ValidateReadOnlyShards_Conflict(t *testing.T) { ... }

// 読み取り専用シャードで競合なし（startTS以前にコミット済み）
func TestShardedCoordinator_ValidateReadOnlyShards_NoConflict(t *testing.T) { ... }

// readKeys が nil の場合は早期リターン
func TestShardedCoordinator_ValidateReadOnlyShards_NilReadKeys(t *testing.T) { ... }

// 書き込みシャードと読み取りシャードが重なる場合は検証をスキップ
func TestShardedCoordinator_ValidateReadOnlyShards_SkipsWriteShards(t *testing.T) { ... }

2. `groupReadKeysByShardID` のユニットテストがない（Critical）

同じく kv/sharded_coordinator.go の groupReadKeysByShardID も未テストです。

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

// 空の readKeys → nil を返す
// gid=0 のキー（ルーティング不明）はスキップされる
// 複数シャードへの正しい振り分け
func TestShardedCoordinator_GroupReadKeysByShardID(t *testing.T) { ... }

3. `readKeys` の proto 層への伝播テストがない（High）

kv/coordinator_txn_test.go の TestCoordinateDispatchTxn_UsesOnePhaseRequest は req.Mutations を検証していますが、req.ReadKeys を検証していません。readKeys が正しく onePhaseTxnRequest → pb.Request.ReadKeys に設定されることを確認するテストが必要です。

// readKeys が非 nil の場合、Request.ReadKeys に反映されること
func TestCoordinateDispatchTxn_PropagatesReadKeys(t *testing.T) {
    // ...
    _, err := c.dispatchTxn([]*Elem[OP]{...}, startTS, 0, [][]byte{[]byte("rk1")})
    // req.ReadKeys を assert
}

4. マルチシャードSSI の結合テストがない（High）

sharded_coordinator_txn_test.go に、ライトスキューシナリオのテストが一切ありません。

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

// シャードAに書き込み＋シャードBを読み取り、シャードBへの並行書き込みで競合検出
func TestShardedCoordinator_DispatchTxn_WriteSkewDetected(t *testing.T) { ... }

// 読み取りキーが書き込みシャードと同じ場合はFSM内で検証（二重検証にならない）
func TestShardedCoordinator_DispatchTxn_ReadKeysOnWriteShard(t *testing.T) { ... }

5. `adapter/redis.go` の `validateReadSet` がデッドコードになったが、テストが残っている（Moderate）

runTransaction から validateReadSet の呼び出しが削除されましたが、TestRedisTxnValidateReadSetDetectsStaleListMeta は今もこの使われない関数を直接テストしています。代わりに、Redis MULTI/EXEC レベルでライトスキューが実際に防がれること を確認するエンドツーエンドテストが必要です。

// MULTI/EXEC中にキーが別トランザクションで更新された場合、EXEC がエラーになること
func TestRedisServer_MULTI_EXEC_WriteSkewPrevented(t *testing.T) { ... }

6. `validateReadOnlyShards` が `context.Background()` を使用（Moderate）

validateReadOnlyShards は親コンテキストを無視して context.Background() を使用しています。これにより、呼び出し元でタイムアウトが設定されていてもキャンセルが伝播されません。タイムアウト・キャンセルシナリオのテストはありません。

まとめ

対象	重要度	状況
`validateReadOnlyShards` ユニットテスト	Critical	不足
`groupReadKeysByShardID` ユニットテスト	Critical	不足
`readKeys` の proto 伝播テスト	High	不足
マルチシャードSSI 結合テスト	High	不足
Redis adapter ライトスキュー E2E テスト	Moderate	不足
`validateReadSet` のデッドコードテスト整理	Low	要整理

store 層の単体テストはよくカバーされていますが、coordinator 層（特にマルチシャード SSI）のテストが全く存在しない点が主な懸念です。

claude · 2026-04-12T20:06:15Z

[Concurrency] レビュー結果

PR #499: store: add read-set validation to ApplyMutations for SSI

問題1: `validateReadOnlyShards` がコンテキストを破棄する【重大度: 高】

ファイル: kv/sharded_coordinator.go（差分 +401行目付近）

// 問題のあるコード
ctx := context.Background()  // ← 呼び出し元のコンテキストを無視
for gid, keys := range groupedReadKeys {
    ...
    ts, exists, err := g.Store.LatestCommitTS(ctx, key)

呼び出しチェーンは Dispatch(ctx) → dispatchTxn → prewriteTxn → validateReadOnlyShards であり、すべての関数で ctx が引き渡されているにもかかわらず、validateReadOnlyShards だけが context.Background() を生成しています。

影響:

ノード障害・ネットワーク分断時に g.Store.LatestCommitTS が無期限にブロック
redisDispatchTimeout 等の上位タイムアウトが効かなくなる
タイムアウト不能なゴルーチンが積み重なりリークの原因になりうる

修正案:

// validateReadOnlyShards のシグネチャを変更
func (c *ShardedCoordinator) validateReadOnlyShards(ctx context.Context, groupedReadKeys map[uint64][][]byte, writeGIDs []uint64, startTS uint64) error {

呼び出し元 prewriteTxn も同様に ctx を受け取るよう変更し、引き渡す。

問題2: 読み取り専用シャードの TOCTOU ウィンドウ【重大度: 中】

ファイル: kv/sharded_coordinator.go（差分 +359–362行目付近）

書き込みシャードの読み取りキーは PREPARE リクエスト経由で Raft ログに乗り、FSM の applyMu 下でアトミックに検証されます（store/lsm_store.go の checkReadConflicts）。

一方、書き込みが存在しないシャード（読み取り専用シャード）は PREPARE できないため、validateReadOnlyShards が Raft の外側で LatestCommitTS を直接呼び出します。これにより:

この隙間で並行書き込みが発生しても検出されず、Write Skew が残ります。

注記: 以前の validateReadSet（アダプタ層）も同様に Raft 外検証だったため、本 PR で状況が悪化しているわけではありません。ただし、「Read-set validation is now atomic」とコメントに記載されているのは、単一シャードトランザクションと書き込みシャードの読み取りキーにのみ当てはまります。マルチシャードかつ読み取り専用シャードが存在するケースでは完全な SSI は依然として保証されないことをコメントまたは設計ドキュメントに明記することを推奨します。

問題なしと確認した点

readKeys マップのデータ競合: txnContext は Redis コネクションごとに単一ゴルーチンで順次処理されるため、commit() でのマップ反復と trackReadKey() の呼び出しは同時実行されない。競合なし。
ロック順序 (lsm_store.go): dbMu.RLock → applyMu.Lock → checkConflicts → checkReadConflicts → applyMutationsBatch → mtx.Lock の順序は一貫しており、デッドロックの危険なし。
フォロワー→リーダープロキシ (coordinator.go redirect): reqs.ReadKeys が正しく onePhaseTxnRequest に渡されている。
ゴルーチンリーク: 新規ゴルーチン生成なし。コンテキストキャンセルは defer cancel() で適切に処理。
Raft リーダー切り替え: FSM の handleOnePhaseTxnRequest / handlePrepareRequest は Raft ログ経由で ReadKeys を受け取り、ApplyMutations の apply ロック下で検証するため、リーダー切り替え時も再適用の安全性を保持。

claude · 2026-04-12T20:06:33Z

[Performance] レビュー結果

[P1] `applyMu` 保持中に O(N) の連続ディスク読み取り — `store/lsm_store.go`

ApplyMutations 内で checkReadConflicts が applyMu.Lock() の保持中 に呼び出されており、read-key の数だけ Pebble の latestCommitTS（= seek I/O）が 逐次実行 される。

// store/lsm_store.go (PR後)
s.applyMu.Lock()
defer s.applyMu.Unlock()

checkConflicts(...)          // 書き込みキー × Pebble seek
checkReadConflicts(...)      // ← 読み取りキー × Pebble seek (追加)
applyMutationsBatch(...)

applyMu はすべての ApplyMutations を直列化するグローバルなボトルネックであり、read-set サイズ N に比例してロック保持時間が増加し、コミットスループット全体が低下する。

checkConflicts（書き込みキー）と同じ設計だが、SSI では read-set がトランザクションで触れた全キーを含むため、write-set より一般的に多くなりうる。

改善案: checkReadConflicts の Pebble I/O をロック取得前（applyMu 外）に楽観的にプリフェッチし、ロック内では最終確認のみ行う二段階チェックを検討する（ただし TOCTOU のトレードオフを要評価）。

[P2] `validateReadOnlyShards` が per-key で逐次 `LatestCommitTS` を呼び出す — `kv/sharded_coordinator.go`

// kv/sharded_coordinator.go
for gid, keys := range groupedReadKeys {
    if _, isWrite := writeSet[gid]; isWrite { continue }
    for _, key := range keys {
        ts, exists, err := g.Store.LatestCommitTS(ctx, key)  // 逐次 I/O
        ...
    }
}

read-only シャードが複数ある場合、シャード間・キー間ともに並列化されず、全キー分の I/O が直列実行される。read-set が大きい MULTI トランザクションではコミットレイテンシに直接影響する。

改善案: シャード単位で errgroup による並列実行を検討する。シャード内のキーについては、Pebble の NewIter で prefix scan をまとめて実行できる可能性もある。

[P3] `groupReadKeysByShardID` の map にサイズヒントがない — `kv/sharded_coordinator.go`

grouped := make(map[uint64][][]byte)  // サイズヒントなし

read-key 数が多い場合、map の再ハッシュが複数回発生する。少なくともシャード数の上限が分かる場合は make(map[uint64][][]byte, len(c.groups)) とすることで再ハッシュを抑制できる。

補足（性能改善点）

adapter/redis.go で validateReadSet の呼び出しを削除したことは パフォーマンス改善 であり評価できる（コメントにある通り、LatestCommitTS の二重呼び出しが排除される）。

t.readKeys が map[string][]byte であるため、commit 時に [][]byte へ変換するループ（make + for + append）は型変換として必要であり、cap も正しく指定されている。

claude · 2026-04-12T20:07:39Z

[Data Loss] レビュー結果

概要

データロストに関して致命的な問題は検出されませんでした。ただし、1点リスク軽微な懸念を指摘します。

OK: `checkReadConflicts` が `applyMu.Lock()` 内に配置されている

lsm_store.go の ApplyMutations では、applyMu.Lock() 取得後に checkConflicts → checkReadConflicts → applyMutationsBatch の順で呼ばれます（store/lsm_store.go 行 893–903 付近）。

これにより「コンフリクト検出とバッチ書き込みのアトミック性」が保証されており、データが意図せず消失・上書きされるパスはありません。

OK: `mvccStore` の競合チェックもロック内で統合されている

checkConflictsLocked に書き込みキーと読み込みキー両方の検証が統合されており、s.mtx.Lock() 内で一括実行されます。

OK: `validateReadSet` の削除は安全

旧実装では runTransaction でディスパッチ前に validateReadSet を呼んでいましたが、これはロック外での検証であり TOCTOU 窓がありました。本 PR で ApplyMutations 内（ストアのアプライロック下）に移動したことで、むしろ整合性が向上しています。

OK: トランザクションアボートパス

多シャードトランザクションで validateReadOnlyShards がコンフリクトを検知した場合、abortPreparedTxn が呼ばれ、準備済みシャードのロックが正しく解放されます。データが中途半端な状態で残るパスはありません。

OK: `ReadKeys` の Raft ログへの永続化

internal.proto に read_keys (field 5) が追加され、Raft ログにシリアライズされます。FSM Apply 時に checkReadConflicts が実行されるため、フォロワーノードでも同一の競合検出が再現されます（Raft の決定論的適用保証）。

軽微な懸念: `gid == 0` の読み込みキーがサイレントにスキップされる

該当箇所: kv/sharded_coordinator.go の groupReadKeysByShardID

gid := c.engineGroupIDForKey(key)
if gid == 0 {
    continue // ← ルートが見つからない場合、サイレントスキップ
}

engineGroupIDForKey はルートが存在しないキーに対して 0 を返しますが、このキーは SSI の読み込み競合チェックから除外されます。

データロストへの影響: データロストは発生しません。書き込みシャードへの書き込みは正常に完了します。ただし、このキーへの並行書き込みが検出されない「書き込みスキュー（write skew）」が発生する可能性があります。これは一貫性の問題であり、データロストではありませんが、SSI の保証が一部無効化されます。

この点が問題になるケースとしては、本来はルーティングされるべきキーが何らかの理由でルートテーブルに存在しない場合です。エラーを返す（フェイルセーフ）か、少なくともログ警告を出す対応を検討してください。

結論

データロスト観点での致命的問題なし。上記の gid == 0 スキップは SSI 保証の観点で確認を推奨しますが、データが意図せず失われるパスではありません。

claude · 2026-04-12T20:10:30Z

[Test Coverage] レビュー結果

総評

SI → SSI へのアップグレードという重要な変更に対して、ストア層のユニットテストは充実しているが、以下の層でカバレッジに顕著なギャップがある。

不足しているテスト

1. `kv/sharded_coordinator.go` — `validateReadOnlyShards` / `groupReadKeysByShardID` のテストが皆無【高】

70行追加された新規ロジックに対して、sharded_coordinator_txn_test.go および sharded_integration_test.go はこのPRで一切変更されていない。

提案テストケース:

groupReadKeysByShardID — readKeysが複数シャードに振り分けられること、空スライス/nilで空mapが返ること
validateReadOnlyShards — readKey のみ存在するシャード（write無し）で ts > startTS のとき ErrWriteConflict を返すこと
validateReadOnlyShards — write シャードのreadKeyは二重チェックされないこと（skip される）
validateReadOnlyShards — LatestCommitTS がエラーを返したときエラーが伝播すること
ShardedCoordinator.Dispatch の多シャードtxnで、readKeyが write-free シャードに存在し、他トランザクションがそこに書き込んでいる場合にconflictが検出されること（cross-shard SSI の end-to-end テスト）

2. `kv/coordinator.go` — `onePhaseTxnRequest` に non-nil `ReadKeys` が含まれることの検証なし【中】

既存テスト TestCoordinateDispatchTxn_UsesOnePhaseRequest は nil readKeysのみ。ReadKeysがprotoリクエストに正しくシリアライズされるかが未検証。

提案テストケース:

// readKeys を渡して dispatchTxn を呼び出し、
// tx.reqs[0][0].ReadKeys に期待値が含まれることをアサートする
_, err := c.dispatchTxn([]*Elem[OP]{...}, startTS, 0, [][]byte{[]byte("rk1")})
require.Equal(t, [][]byte{[]byte("rk1")}, tx.reqs[0][0].ReadKeys)

3. `adapter/redis.go` — Redis MULTI/EXEC 経由の write skew 防止に統合テストなし【中】

validateReadSet の直接呼び出しを削除し、readKeys を OperationGroup 経由で渡す方式に変更したが、この新パスをカバーする redis_txn_test.go の新テストがない。

提案テストケース:

MULTI/EXEC で GET した後、別トランザクションが同キーに書き込んだ場合、EXEC が ErrWriteConflict を返すこと
txnContext.commit() 呼び出し時に group.ReadKeys に readKeys が正しく詰められていること

4. SSI を使う並行アクセステストなし【低】

TestMVCCConcurrentApplyMutations / TestMVCCConcurrentApplyMutationsMultiKey はどちらも nil readKeys で呼ばれており、新しい checkConflictsLocked の並行安全性（ロックが正しく取られているか）を実質的に検証していない。

提案テストケース:

複数ゴルーチンから同じreadKeyを含むトランザクションを並行実行し、古い読み取りを持つトランザクションが正しくrejectされること

5. readKey がwriteKeyと重複する場合のテストなし【低】

readKeys に mutations と同一のキーが含まれる場合、checkConflicts と checkReadConflicts の両方でチェックされる（二重チェック）。意図された挙動なら明示的なテストがほしい。

問題なし（適切にカバーされている箇所）

pebbleStore.checkReadConflicts: 5テストで基本・境界・nil・部分conflictを網羅
mvccStore.checkConflictsLocked: 4テストで同等のパターンを網羅
シグネチャ変更に伴う既存テストの nil 追加は漏れなく対応されている

まとめ

箇所	深刻度	理由
`validateReadOnlyShards` / `groupReadKeysByShardID`	高	多シャード環境でのSSIの要となるロジックが完全未テスト
`onePhaseTxnRequest` + ReadKeys のシリアライズ	中	ReadKeysがプロトに正しく含まれるか未検証
Redis adapter の統合パス	中	削除した `validateReadSet` の代替パスの統合テストなし
SSI 並行アクセス	低	新しいロックパスの並行安全性が未検証

claude · 2026-04-12T20:10:42Z

[Concurrency] レビュー結果

問題1 (Critical): `validateReadOnlyShards` が Raft 線形化を迂回している

場所: kv/sharded_coordinator.go — validateReadOnlyShards()

ctx := context.Background()   // タイムアウトなし
for gid, keys := range groupedReadKeys {
    ...
    ts, exists, err := g.Store.LatestCommitTS(ctx, key)  // ローカルストア直読み

問題: write シャードへの PREPARE は Raft FSM を経由して原子的に適用されるが、read-only シャードの競合検査は Raft を通さずローカルストアを直接読む。

Raft ログにコミット済みの書き込みが、FSM のローカル適用 (Apply()) に届く前に LatestCommitTS が呼ばれると その書き込みが存在しないかのように見える。結果として、read-only シャードにまたがる write skew (read-write conflict) を検出できないケースが生じる。

再現シナリオ:

Txn-A がシャードS を読み (read-only)、シャードW に書く (write)
Txn-B がシャードS に書き込み → Raft でコミット済みだが FSM 未適用
Txn-A の PREPARE がシャードW で完了
validateReadOnlyShards がシャードS のローカルストアを読む → Txn-B の書き込みが見えない → 競合を見逃す → write skew が発生

修正案: read-only シャードにも linearizable read barrier を挟む (例: LinearizableReadForKey を経由してから LatestCommitTS を呼ぶ)。あるいは FSM が空ミューテーションリストを拒否しない特別な「read-validate」リクエストフェーズを設ける。

問題2 (Medium): `validateReadOnlyShards` でコンテキストが伝播されない

場所: kv/sharded_coordinator.go:630

ctx := context.Background()  // 呼び出し元のコンテキストを無視

呼び出し元 (prewriteTxn → dispatchTxn → Dispatch) はすべてユーザ起点のコンテキストを受け取っているが、ここでは context.Background() を使い直している。ネットワーク分断やノード障害時に LatestCommitTS がブロックすると、元のコンテキストがキャンセルされても goroutine がリークする。

修正案: validateReadOnlyShards に ctx context.Context を追加して呼び出し元から伝播する。

問題3 (Medium): multi-shard PREPARE と `validateReadOnlyShards` の間に競合窓がある

場所: kv/sharded_coordinator.go — prewriteTxn()

// 1) 全 write シャードに PREPARE 送信 (Raft コミット済)
for _, gid := range gids {
    if _, err := g.Txn.Commit([]*pb.Request{req}); err != nil { ... }
    prepared = append(prepared, ...)
}

// 2) その後で read-only シャードを検査
if err := c.validateReadOnlyShards(groupedReadKeys, gids, startTS); err != nil {
    c.abortPreparedTxn(...)
    return nil, err
}

ステップ1 とステップ2 の間に、別トランザクションが read-only シャードに書き込み → Raft コミット → FSM 適用まで完了した場合でも、上記の問題1 によりローカルストアが追いついていなければ検出できない。write シャードへの PREPARE は "取り消せる" がコスト (abort) が発生するため、検査の原子性は SSI の正確さに直結する。

問題4 (Low): `handlePrepareRequest` での read key 検証の副作用

場所: kv/fsm.go:315

if err := f.store.ApplyMutations(ctx, storeMuts, r.ReadKeys, startTS, startTS); err != nil {

PREPARE フェーズで read key の競合を検出するのは早期に abort できるため望ましい設計だが、startTS == commitTS という引数の非対称性 (ロック行を startTS に書く) を念頭に、テストで PREPARE 後 COMMIT 前に read key が更新されるケース (PREPARE は通過したが COMMIT 時に再度検査が必要か) が明示されていない。one-phase では handleOnePhaseTxnRequest 側で再検査されるため二重チェックになっているが、two-phase (PREPARE → COMMIT) フローでの重複・漏れがないかドキュメントかテストで明確にすると良い。

問題なしな点

mvccStore.checkConflictsLocked は s.mtx.Lock() 保持下で実行されており、単一ストアの原子性は保証されている
pebbleStore.ApplyMutations は applyMu.Lock() 配下で checkConflicts と checkReadConflicts を連続実行しており、同ストア内のロック順序は一貫している
onePhaseTxnRequest で ReadKeys が Protobuf に正しくシリアライズされ Raft ログに乗る設計は正しい
abortPreparedTxn は validateReadOnlyShards 失敗時に呼ばれており、PREPARE ロックのリークは防いでいる

まとめ: 問題1 (read-only シャードの non-linearizable 検査) が SSI の本質的な正確さに関わる重大な問題。問題2 のコンテキスト漏れと合わせてフィックスが必要です。

claude · 2026-04-12T20:11:07Z

[Data Loss] レビュー結果

観点: Write/Delete操作でのデータ消失

データロストに関わる問題は検出されなかった。

確認事項と判定

1. `validateReadSet` の削除 (`adapter/redis.go`)

旧フロー（削除前）:

txnコマンド実行
validateReadSet（ストアロック外、TOCTOU競合あり）
commit（書き込みのみをDispatch）

新フロー（このPR）:

txnコマンド実行
commit（readKeys付きでDispatch）
ApplyMutations 内部で applyLock 下に checkConflicts + checkReadConflicts を実行

旧実装はストアロック外でのTOCTOU競合があった。新実装はapplyLock下で書き込みと同一クリティカルセクション内に読み込み競合チェックを移動しており、データ消失リスクはむしろ低減している。

2. `checkReadConflicts` の実行順序 (`store/lsm_store.go`)

checkConflicts(mutations)    → エラー時はreturn（書き込み不発生）
checkReadConflicts(readKeys) → エラー時はreturn（書き込み不発生）
applyMutationsBatch(...)     ← 両チェックを通過した場合のみ実行

競合検出時は必ずエラーリターンし、applyMutationsBatch は呼ばれない。既存データが上書き・消失するパスはない。

3. マルチシャードトランザクションのabort (`kv/sharded_coordinator.go`)

validateReadOnlyShards で競合検出 → abortPreparedTxn 呼び出し。PREPAREフェーズで書かれたロックレコードのロールバックが実施され、コミットされないデータが残留するパスはない。

4. `nil` readKeys の後方互換性

全呼び出し箇所で既存コード（CatalogStore、FSMのRAW/abort/commit処理等）は nil を渡している。nil 時は checkReadConflicts が空ループで即リターンし、従来どおりの書き込み競合のみの挙動となる。データロストリスクなし。

5. TTL/Expire への影響

本PRはTTL/Expire処理に変更なし。対象外。

6. スナップショット/バックアップへの影響

本PRはスナップショット・バックアップ処理に変更なし。対象外。

総合判定

問題なし。 readKeysのnilガードが適切で、競合検出はデータ書き込みの前に実施されており、abort時のロールバックパスも存在する。

claude · 2026-04-12T20:11:12Z

[Performance] レビュー結果

問題あり

1. `applyMu` 保持中に O(N) Pebble 読み取りが発生する（重大）

場所: store/lsm_store.go — ApplyMutations 内の checkReadConflicts 呼び出し

applyMu.Lock() を保持したまま checkReadConflicts を実行しており、read key 1件につき latestCommitTS (Pebble ポイントルックアップ) が 1回発生する。trackTypeReadKeys は 1 ユーザーキーあたり ~8 件の内部キーを登録するため、典型的なトランザクションでも数十件のディスク読み取りをロック保持中に行うことになる。

これにより並列トランザクションの 書き込みスループットが read-set サイズに比例して低下 する。

改善案の方向性:

applyMu 取得前に checkReadConflicts を呼び出す (TOCTOU は既存の checkConflicts と同等の扱いで問題ない)
もしくは read key の latestCommitTS を Pebble Iterator でバッチ取得してロック保持時間を圧縮する

2. `validateReadOnlyShards` の LatestCommitTS 呼び出しが逐次（中程度）

場所: kv/sharded_coordinator.go — validateReadOnlyShards

read-only シャードに対して、各キーの LatestCommitTS をネストループで逐次呼び出している。これは shard × read key 数分のシリアルな I/O になる。

改善案: シャードごとに goroutine を起動して並列検証することで、レイテンシを最大のシャードコスト1つ分に抑えられる。

3. `readKeys` スライスの不要なコピー（軽微）

場所: adapter/redis.go — txnContext.commit()

readKeys := make([][]byte, 0, len(t.readKeys))
for _, k := range t.readKeys {
    readKeys = append(readKeys, k)
}

t.readKeys は map[string][]byte なのでこの変換自体は必要だが、値の []byte はすでに bytes.Clone 済みのため、append ループで再度スライスを構築することに実質的なコピーコストはない。ただし make + append パターンは maps.Values (Go 1.23) や手書きで明示的に書くのと変わらず、このままで問題はない（情報提供のみ）。

claude · 2026-04-12T20:16:14Z

[Consistency] レビュー結果

総評: 問題なし（一貫性は強化されている）

1. validateReadSet の削除と ApplyMutations への移動

runTransaction から validateReadSet() を削除し、readKeys を ApplyMutations 内で検証するように変更した点は、一貫性の強化として正しい実装です。

旧実装の問題点（解消済み）:

validateReadSet() ← ここで conflict チェック
     ↓  ← この window に concurrent write が入り込める
commit() → Raft → ApplyMutations

新実装（正しい）:

commit() → Raft → ApplyMutations
                    └─ applyMu 下で read-write conflict チェック
                    └─ 同じ critical section で batch commit

アトミックに検証とコミットが行われるため、race window が排除されています。

2. SSI の達成

checkReadConflicts / checkConflictsLocked が write-write・read-write 両方のコンフリクトを applyMu（pebbleStore）または mtx（mvccStore）のロック下で検証しているため、線形化可能性（Linearizability）の要件を満たしています。

3. 2PC パスの確認

handlePrepareRequest での ApplyMutations(ctx, storeMuts, r.ReadKeys, startTS, startTS) はプリペア時に read-set を検証・コミットする設計で、MVCC の snapshot semantics と整合しています。プリペア後にコミット前で read key に concurrent write が来るケースは、プリペアがすでに決定点であることから許容範囲と判断します。

4. readKeys のトラッキング

PR #483 と合わせて trackTypeReadKeys に store.ZSetMetaKey(key) が追加されており、wide-column ZSet の SSI 競合検出が正しく機能します。

問題なし。一貫性の観点では改善です。

Address critical review: validateReadOnlyShards was reading the local store directly, bypassing Raft linearization. A concurrent write that was committed in Raft but not yet applied to the FSM would be invisible. Fix: issue a linearizable read barrier (LinearizableRead) on each read-only shard before checking LatestCommitTS, ensuring the FSM has applied all committed entries. Also: - Propagate ctx through dispatchTxn/prewriteTxn/validateReadOnlyShards instead of using context.Background() (Medium review) - Document the remaining TOCTOU limitation for read-only shards in multi-shard transactions (the check is outside applyMu) - Add test: onePhaseTxnRequest includes ReadKeys in proto request - Add tests: readKey overlapping writeKey (conflict and no-conflict)

github-actions · 2026-04-13T05:49:45Z

kv/sharded_coordinator.go

+// would require a dedicated "read-validate" FSM request phase. For
+// single-shard transactions and write-shard read keys, validation is fully
+// atomic under applyMu.
+func (c *ShardedCoordinator) validateReadOnlyShards(ctx context.Context, groupedReadKeys map[uint64][][]byte, writeGIDs []uint64, startTS uint64) error {


🚫 [golangci] _{reported by reviewdog 🐶}
calculated cyclomatic complexity for function validateReadOnlyShards is 11, max is 10 (cyclop)

claude · 2026-04-13T05:49:47Z

[Data Loss] レビュー結果

対象PR: #499 store: add read-set validation to ApplyMutations for SSI

確認観点と結果

1. Write/Delete操作でデータが意図せず失われるパスがないか

問題なし。

pebbleStore.ApplyMutations の実行順序は以下の通り：

checkConflicts（書き込み競合チェック）
checkReadConflicts（読み取り競合チェック） ← 今回追加
applyMutationsBatch（実際の書き込み）

競合チェックはバッチ適用前に行われるため、チェックがエラーを返した場合、pebble Batch への書き込みは行われずデータは失われない。mvccStore も同様に checkConflictsLocked でミューテックスホールド下でアトミックにチェック・適用している。

2. WAL/Raftログへの書き込みが適切に永続化されているか

問題なし。

ReadKeys は既存の Raft コミットパス（pb.Request.read_keys フィールド追加）を通じて伝播する。新規フィールドの追加のみであり、既存のWAL永続化フローに変更はない。

3. エラーハンドリングの欠如によりデータが消失する可能性がないか

問題なし。

全エラーパスで errors.WithStack でラップして返しており、サイレントなエラー握り潰しは見当たらない。

prewriteTxn で validateReadOnlyShards が失敗した場合、abortPreparedTxn を呼び出して準備済みシャードをクリーンアップしており、部分適用状態のままにはならない。

4. トランザクションのコミット/ロールバック処理でデータが失われないか

問題なし。

旧実装では validateReadSet がRaft送信前にapplyMuロック外で呼び出されていたが、今回の変更で検証を ApplyMutations 内（applyMuロック下）に移動した。これにより検証とコミットがアトミックになり、検証後・コミット前の競合ウィンドウが除去されている（改善）。

5. マルチシャードトランザクションにおける TOCTOU ウィンドウについて

データロストではなく、分離レベルの問題。

validateReadOnlyShards（kv/sharded_coordinator.go）は FSM の applyMu ロック外でリニアライザブル読み取りバリア → LatestCommitTS チェックを行う。コード中のコメントにも明記されているように、このウィンドウ内で並行書き込みがあった場合に write skew が検出されない可能性がある。

ただし、これは「本来 abort すべきトランザクションが commit される」可能性（isolation の低下）であり、既存データが消失するパスではない。

6. TTL/Expire処理、スナップショット/バックアップ

変更なし。

本PRはTTL・Expire・スナップショット・バックアップのロジックに変更を加えていない。

総評

データロストに関する問題は検出されなかった。

競合検出を applyMu ロック下に移動したことで、既存の SI 実装よりもアトミック性が向上している。マルチシャード読み取り専用シャードの TOCTOU ウィンドウは isolation レベルの問題であり、データ消失には繋がらない。

claude · 2026-04-13T05:50:14Z

[Performance] レビュー結果

1. `applyMu` 保持時間の増大（重要度: 高）

場所: store/lsm_store.go — ApplyMutations 内の checkReadConflicts 呼び出し

s.applyMu.Lock()
defer s.applyMu.Unlock()

if err := s.checkConflicts(ctx, mutations, startTS); err != nil { ... }
if err := s.checkReadConflicts(ctx, readKeys, startTS); err != nil { ... }  // ← 追加

applyMu は「コンフリクト検査 → コミット」をシリアライズする排他ロックです。既存の checkConflicts は書きセット N_write 件のディスク I/O をロック保持中に行いますが、今回追加された checkReadConflicts はさらに読みセット N_read 件の latestCommitTS（pebble ポイントルックアップ）を同じロック保持中に追加します。

トランザクションの読みセットは書きセットより大幅に大きくなるケース（例: 100 キー読んで 2 キー書く）が多く、このような場合にすべての並行書き込みがロック待ちになります。ホットパスでの applyMu 保持時間が O(N_read) に比例して伸びる点は書き込みスループットへの直接的な影響があります。

改善案: checkReadConflicts の実行を applyMu の外（dbMu.RLock は保持したまま）に移動し、検査後に再度 applyMu を取得してコミットする「楽観的チェック」方式を検討してください。ただし、TOCTOU が生じるため、ロック取得後に再検証するか、読みセット検査をコミット前の別フェーズとして設計する必要があります。

2. `validateReadOnlyShards` での逐次 `LatestCommitTS` 呼び出し（重要度: 中）

場所: kv/sharded_coordinator.go — validateReadOnlyShards

for _, key := range keys {
    ts, exists, err := g.Store.LatestCommitTS(ctx, key)  // キーごとにディスク I/O
    ...
}

shard ごとに線形化読み取りバリア（linearizableReadEngineCtx）を 1 回発行した後、そのシャードの読みキーを 1 件ずつ逐次ルックアップしています。キー数が多い場合、N_read 回のディスク I/O が直列に発行されます。pebble が MultiGet インターフェースを持つ場合はバッチ化を、持たない場合でも goroutine による並行化でレイテンシを削減できます。

3. `groupReadKeysByShardID` のマップ容量未指定（重要度: 低）

場所: kv/sharded_coordinator.go — groupReadKeysByShardID

grouped := make(map[uint64][][]byte)  // 容量未指定

シャード数は len(c.groups) で既知なので、make(map[uint64][][]byte, len(c.groups)) とすることで初期リハッシュを回避できます。

まとめ

#	場所	内容	重要度
1	`store/lsm_store.go`	`applyMu` 保持中の O(N_read) ディスク I/O 追加	高
2	`kv/sharded_coordinator.go`	読みオンリーシャードの逐次 `LatestCommitTS`	中
3	`kv/sharded_coordinator.go`	`groupReadKeysByShardID` マップ容量未指定	低

問題 1 は、読みセットが大きいトランザクションで書き込みスループットが顕著に低下する可能性があります。書き込みスループット重視の場合はロック外への移動を、実装コストを抑えたい場合は読みセットサイズに上限（例: 最大 N 件）を設けることでリスクを限定できます。

claude · 2026-04-13T05:50:34Z

[Test Coverage] レビュー結果

概要

新規ロジック (checkReadConflicts, checkConflictsLocked, validateReadOnlyShards, groupReadKeysByShardID) に対してストア層のテストは充実しているが、上位レイヤー (ShardedCoordinator・FSM・adapter) のSSI経路にテストが存在しない点が主な懸念です。

問題あり

1. `ShardedCoordinator.validateReadOnlyShards` — テスト未存在（重大）

sharded_coordinator.go に追加された最も複雑なロジックにも関わらず、kv/sharded_coordinator_txn_test.go に対応するテストが一切追加されていません。

カバーすべきケース:

// (1) マルチシャードTxn: read-onlyシャードで書き込みを検出 → ErrWriteConflict
// (2) マルチシャードTxn: read-onlyシャードのキーが startTS 以前にコミット → 競合なし
// (3) groupReadKeysByShardID: gid=0 にマップされるキーが無視される動作の確認
// (4) validateReadOnlyShards: 空の groupedReadKeys → 即リターン (nil safe)

また、コード中に以下のコメントがあります:

// NOTE: This check is performed outside the FSM's applyMu lock, so there
// is a small TOCTOU window between the linearizable read barrier and the
// LatestCommitTS check.

この既知の制限を文書化するテストまたはコメントテストも望まれます。

2. FSM層での `r.ReadKeys` スレッディング — テスト未存在（重大）

kv/fsm.go では handleOnePhaseTxnRequest と handlePrepareRequest の両方が r.ReadKeys を ApplyMutations に渡すよう変更されましたが、kv/fsm_txn_test.go / kv/fsm_occ_test.go にこの経路を検証するテストがありません。

カバーすべきケース:

// (1) handleOnePhaseTxnRequest: ReadKeys に競合キーがある → ErrWriteConflict
// (2) handleOnePhaseTxnRequest: ReadKeys に競合なし → 正常コミット
// (3) handlePrepareRequest: ReadKeys 付き PREPARE フェーズでの競合検出

3. adapter層のエンドツーエンド経路 — テスト未存在（中）

adapter/redis.go の runTransaction から validateReadSet 呼び出しが削除され、その役割が ApplyMutations 内部に移譲されました。しかし adapter/redis_txn_test.go には依然として古い validateReadSet を直接呼ぶテストが存在し、新しい経路 (readKeys → Dispatch → FSM → ApplyMutations) を通じてwrite-skewが防止されるかを確認するエンドツーエンドテストがありません。

// 提案: adapter層でredisトランザクションを使い、並行書き込み後に
// ErrWriteConflict が返ることを確認するテスト

4. `pebbleStore.checkReadConflicts` の並行テスト不足（軽微）

TestPebbleStore_ApplyMutations_ConcurrentConflictDetection は write-write の並行競合をテストしていますが、read-write (SSI) の並行競合に相当するテストがありません。mvccStore 側も同様。

問題なし

以下については十分なカバレッジが確認できました:

store/lsm_store_txn_test.go: 7件の新規テストで checkReadConflicts の正常系・異常系・境界値 (nil, 空key, 部分競合, 読み書き同一キー) を網羅
store/mvcc_store_concurrency_test.go: 4件の新規テストで mvccStore.checkConflictsLocked をカバー
kv/coordinator_txn_test.go: TestCoordinateDispatchTxn_ReadKeysInRequest で readKeys が pb.Request に正しくセットされることを確認
シグネチャ変更に伴う既存テストの nil 引数追加は全ファイルで適切に対応済み

claude · 2026-04-13T05:50:54Z

[Data Loss] レビュー結果

総評

コアとなる書き込みパスでのデータロストは確認されませんでした。ただし、マルチシャードトランザクションにおけるリードオンリーシャードのTOCTOU窓に、隔離レベルの抜け穴（Write Skew）があり、ユーザーデータが論理的に不整合な状態でコミットされるリスクがあります。

問題なし (安全性を確認した箇所)

1. checkReadConflicts の位置が正しい (store/lsm_store.go)

// ApplyMutations 内
if err := s.checkConflicts(ctx, mutations, startTS); err != nil {
    return err
}
if err := s.checkReadConflicts(ctx, readKeys, startTS); err != nil {
    return err   // ← applyMutationsBatch より前に返す
}
if err := s.applyMutationsBatch(b, mutations, commitTS); err != nil {

競合を検出した場合は applyMutationsBatch を呼ばずに返しているため、バッチが中途半端に書き込まれることはない。

2. mvccStore.checkConflictsLocked は mtx.Lock() 保持中に実行

Write-Write / Read-Write チェックがどちらも同一ミューテックス下で行われており、アトミック性が保たれている。

3. validateReadSet の削除 (adapter/redis.go)

旧来の外部バリデーション（txn.validateReadSet）を削除し、ApplyMutations 内の apply lock 下でのチェックに一本化している。チェックタイミングが apply lock 取得後になったことで競合窓がむしろ縮小しており、データロストリスクは増加していない。

4. コンフリクト時の abort 処理 (kv/sharded_coordinator.go)

if err := c.validateReadOnlyShards(ctx, groupedReadKeys, gids, startTS); err != nil {
    c.abortPreparedTxn(startTS, primaryKey, prepared, abortTSFrom(startTS, commitTS))
    return nil, err
}

バリデーション失敗時に abortPreparedTxn を呼んでいるため、PREPARE 済みシャードへの部分コミットは防がれている。

要注意: Write Skew の抜け穴 (データ整合性リスク)

validateReadOnlyShards の TOCTOU 窓 (kv/sharded_coordinator.go:382-385)

コード自身がコメントで認めている通り：

// NOTE: This check is performed outside the FSM's applyMu lock, so there
// is a small TOCTOU window between the linearizable read barrier and the
// LatestCommitTS check. A concurrent write that commits in this window may
// go undetected.

マルチシャードトランザクションにおいて、リードオンリーシャード（そのトランザクションで書き込みが発生しないシャード）上の Read-Write 競合は FSM の applyMu 外でチェックしているため、タイミング次第でWrite Skewが検出されずにコミットされる可能性がある。

例:

Txn A: シャード1のキー k1 を読んでシャード2に k2 を書く
Txn B: シャード2のキー k2 を読んでシャード1に k1 を書く
validateReadOnlyShards の linearizable read barrier 後、LatestCommitTS チェック前のわずかな窓に相手方がコミットした場合、両方が通過してしまう

影響範囲: 純粋なデータロスト（書いたデータが消える）ではなく、本来 abort されるべきトランザクションが commit されることで、アプリケーション上の論理的データ不整合が発生しうる。

コードの NOTE コメントには「single-shard および write-shard の read keys は applyMu 下で完全アトミックに検証される」と書かれており、問題は multi-shard 且つ read-only shard にあるケースに限定される。PR 説明にも「Full SSI for read-only shards in multi-shard transactions would require a dedicated 'read-validate' FSM request phase.」と記載があり、既知の制限として文書化されている点は評価できる。

推奨: この制限をユーザー向けドキュメントや CHANGELOG にも明記し、将来的に dedicated READ_VALIDATE FSM フェーズを追加することを検討すること。

小計

観点	状態
Write/Delete での意図しないデータ消失	問題なし
WAL/Raftログへの書き込み永続化	問題なし
エラーハンドリング欠如によるデータ消失	問題なし
トランザクションのコミット/ロールバック処理	問題なし
TTL/Expire による意図しない削除	問題なし
スナップショット/バックアップ関連の欠損	問題なし
隔離レベル抜け穴によるデータ不整合	要注意 (既知の制限・コメントで明記済み)

claude · 2026-04-13T05:51:17Z

[Test Coverage] レビュー結果

PR #499 は store 層の SSI テストは充実していますが、上位レイヤーに複数の未テスト箇所があります。

高優先度

1. `validateReadOnlyShards` のテストが存在しない

kv/sharded_coordinator.go に追加された最も複雑なロジックですが、sharded_coordinator_txn_test.go には対応するテストがゼロです。コード自身が TOCTOU ウィンドウを認め注記しているにもかかわらず、以下のケースが未検証です。

提案テストケース:

// マルチシャードTxnで、read-only shard のキーに別TxnがstartTS後に書き込んだ場合 → ErrWriteConflict
// マルチシャードTxnで、read-only shard のキーが変更されなかった場合 → 成功
// groupedReadKeys が nil の場合 → 早期リターンで何もしない

2. `validateReadSet` がデッドコードになりテストが旧メカニズムを検証している

runTransaction から validateReadSet 呼び出しが削除されたため、adapter/redis.go:1893 の validateReadSet は本番フローで到達不能です。しかし TestRedisTxnValidateReadSetDetectsStaleListMeta（adapter/redis_txn_test.go:11）はその関数を直接呼び出しており、新しいメカニズム（readKeys → ApplyMutations）ではなく旧メカニズムを検証しています。

提案テストケース:

// MULTI/EXEC中にlistStateをロードし、commit前に別TxnがそのキーをstartTS後に書き込む
// → txnContext.commit() が ErrWriteConflict を返すことを検証

中優先度

3. `ShardedCoordinator` の `ReadKeys` パスにテストがない

sharded_coordinator_txn_test.go（299行）に ReadKeys / readKeys の参照が皆無です。dispatchSingleShardTxn と prewriteTxn を readKeys 付きで呼ぶパスが未テストです。

提案テストケース:

// シングルシャードTxnでReadKeysを渡した場合、onePhaseTxnRequestにReadKeysが含まれること
// マルチシャードTxnでReadKeysが各シャードに正しく振り分けられること

4. FSM レベルでの `ReadKeys` テストがない

fsm_txn_test.go と fsm_occ_test.go は ReadKeys を参照していません。handlePrepareRequest と handleOnePhaseTxnRequest に渡された r.ReadKeys が ApplyMutations に正しく流れるかが FSM 単体では未検証です。

提案テストケース:

// ReadKeys付きの pb.Request を handleOnePhaseTxnRequest に渡し、
// startTS後に書き込まれたread keyがある場合はErrWriteConflictになること

5. アダプター層のエンドツーエンド write-skew テストがない

PRの主目的である「write skew の防止」を実証する統合テストがありません。

提案テストケース:

// Tx1: GET k1, SET k2 / Tx2: GET k2, SET k1 (並行に実行)
// → 少なくとも一方が EXECABORT になることを検証

低優先度

6. `groupReadKeysByShardID` でシャード不明キーがサイレントに無視される

engineGroupIDForKey が gid == 0 を返したキーは continue でスキップされます（kv/sharded_coordinator.go:604）。これにより SSI 保護がサイレントに欠落します。テストも存在しません。

提案テストケース:

// 既知シャードと未知シャード混在のキーリスト → 既知シャード分のみグループ化されること

7. 並行 SSI テスト（mvccStore）がない

TestMVCCConcurrentApplyMutations 等の既存並行テストは readKeys=nil のみを使用しています。readKeys を持つ並行シナリオで applyMu ロックが競合をきちんと防ぐかが未検証です。

総括

層	カバレッジ
`store/lsm_store` (pebble)	充実（7テスト追加）
`store/mvcc_store`	充実（4テスト追加）
`kv/coordinator` (単一)	部分的（ReadKeys伝播のみ）
`kv/sharded_coordinator`	未テスト
`kv/fsm`	未テスト
`adapter/redis` (エンドツーエンド)	旧メカニズムのテストのみ

claude · 2026-04-13T05:52:17Z

[Consistency] レビュー結果

概要

SSI (Serializable Snapshot Isolation) 実装の方針自体は正しく、単一シャードのトランザクションでは write-write・read-write 両コンフリクトが applyMu ロック下でアトミックに検証される点を確認。以下、データ一貫性の観点から指摘する。

[重要] マルチシャードのread-onlyシャードにTOCTOUウィンドウが存在する

該当箇所: kv/sharded_coordinator.go — validateReadOnlyShards

// NOTE: This check is performed outside the FSM's applyMu lock, so there
// is a small TOCTOU window between the linearizable read barrier and the
// LatestCommitTS check.

PRのコード自体がコメントで認めているとおり、マルチシャードトランザクションで 書き込みミューテーションを持たないシャード（read-onlyシャード） のread-writeコンフリクト検証は、FSMの applyMu ロック外で実施される。

具体的な問題シナリオ:

Txn1: シャードAのキーaに書き込み、シャードBのキーbを読む
Txn2: シャードBのキーbに書き込み
Txn1の validateReadOnlyShards がシャードBの線形化readバリアを通過した直後に、Txn2のRaftログがシャードBのFSMに適用される
Txn1のシャードBに対する LatestCommitTS(b) 検査は Txn2 のコミットを見落とす → write skewが成立

シングルシャードトランザクションおよびwrite-shardに属するread-keyは applyMu 下で完全にアトミックに検証されているため、影響はマルチシャードかつread-onlyシャードを持つトランザクションに限定される。コメントで言及されているように、完全な対策としては "read-validate" FSMリクエストフェーズの追加が必要。

[中程度] Phantom readが検出されない

該当箇所: store/lsm_store.go — checkReadConflicts、store/mvcc_store.go — checkConflictsLocked

if exists && ts > startTS {
    return NewWriteConflictError(key)
}

Txn開始時に存在しなかったキーが、startTS以降に別のトランザクションによってINSERTされた場合、exists == false のためコンフリクトが返らない。

Txn1: key="x"を読む(存在しない) → startTS=10
Txn2: key="x"をPUT @ commitTS=15
Txn1: key="y"にPUT @ commitTS=20 → checkReadConflicts("x") → exists=true, ts=15 > 10 → ???

実際には exists=true になるため上記ケースは検出できる。問題は Txn1が読んだ時点で存在せず、Txn1のコミット前にTxn2がINSERTした後でTxn1のチェックが走った場合:

latestCommitTS("x") → exists=true, ts=15 > startTS=10 → 検出できる ✓

ただし range scan（例: SCAN・KEYS）でキーが存在しないことに依存するロジック（"テーブルが空なら初期化する"パターン等）はread-setにキーが追加されないため phantom readとして検出不可。これは trackReadKey が 実際にアクセスした既知キー のみを追跡するアーキテクチャ的制約であり、現PRの設計範囲を超えているが、SSI達成の観点では限界として記録しておく。

[確認済み・問題なし] Raftを経由すべき操作の直接実行

LeaderRoutedStore.ApplyMutations は s.local.ApplyMutations に委譲しているが、これはフォロワーからの呼び出しを拒否するパスであり、FSM経由のRaft適用が保たれている。handleRawRequest 等でFSMが直接 ApplyMutations を呼ぶのは、Raftログが既にコミット済みのエントリを適用している文脈で正しい。

[確認済み・問題なし] ロック下での線形化可能性

pebbleStore.ApplyMutations では applyMu.Lock() 下で checkConflicts → checkReadConflicts → applyMutationsBatch の順に実行されており、write-writeとread-writeの両コンフリクト検出と書き込み適用がアトミックに保護されている。

[確認済み・問題なし] TTL・エンコーディングの一貫性

trackReadKey は TTLキー（redisTTLKey）、メタキー（listMetaKey）、集約キー（redisZSetKey）を適切に追跡しており、データ本体とTTLの整合性は維持されている。プレフィックスエンコーディングの変更はなく一貫性に問題なし。

まとめ

観点	評価
単一シャードSSI（1PC・applyMuロック下）	✅ 正しく実装
マルチシャードread-onlyシャードSSI	⚠️ TOCTOU window あり（コード内で文書化済み）
Phantom read検出	⚠️ range scan由来のphantomは検出不可（設計上の限界）
Raftバイパスの有無	✅ 問題なし
TTL・エンコーディング一貫性	✅ 問題なし
2PCのread-writeコンフリクト	✅ PREPARE時にreadKeysをシャードごとに分配して正しく検証

claude · 2026-04-13T05:52:50Z

[Concurrency] レビュー結果

1. 問題なし: `applyMu` 下での read-set 検証（原子性）

pebbleStore.ApplyMutations（store/lsm_store.go:906）では
s.applyMu.Lock() → checkConflicts → checkReadConflicts → applyMutationsBatch
という順序が一貫しており、write-write・read-write 両チェックとバッチコミットが同一ロック下で原子的に実行される。

mvccStore.ApplyMutations（store/mvcc_store.go）も s.mtx.Lock() 下で checkConflictsLocked が呼ばれており安全。

Raft ログのリプレイ時も FSM は決定論的に同一ストア状態を参照するため、handlePrepareRequest / handleOnePhaseTxnRequest で r.ReadKeys を渡す実装は安全。

2. 問題なし: `redirect` 経路での ReadKeys 伝播

coordinator.go の redirect 関数で onePhaseTxnRequest(..., reqs.ReadKeys) が追加されており、フォロワーがリーダーへ転送する場合も read set が正しく伝達される。

3. 【中】既知 TOCTOU: `validateReadOnlyShards`（`sharded_coordinator.go:632`）

コード内のコメントで明記されているが、マルチシャードトランザクションで「書き込みなし・読み取りのみ」のシャードに対する検証は applyMu の外 で行われる。

linearizableReadEngineCtx(ctx, engine) → LatestCommitTS(ctx, key) の間

このウィンドウで別トランザクションが Raft コミットすると検出漏れが発生する。
単一シャードおよびライトシャードのキーは applyMu 下で完全原子的なため 回帰ではない。
ただし、完全 SSI が必要な場合は「read-validate」FSM フェーズが必要である旨、コメント（:625）が正確に記録済み。

abort パスも正しく処理されている:
validateReadOnlyShards 失敗時に abortPreparedTxn(startTS, primaryKey, prepared, ...) が呼ばれ、PREPARE 済みシャードのロックが解放される。

4. 【低】`txnContext.validateReadSet` がデッドコードになっている（`adapter/redis.go:1893`）

runTransaction から呼び出しを削除したことで、プロダクションコードでは validateReadSet は呼ばれなくなった。
adapter/redis_txn_test.go:38 からのみ直接呼ばれているが、このテストは現在の重要パスを検証していない。

安全性には影響しないが、将来の開発者が混乱する可能性がある。
validateReadSet メソッドの削除と、テストを store 層の SSI テストへ置き換えることを推奨する。

5. ロック順序・goroutine リーク・チャネルブロッキング

dbMu.RLock() → applyMu.Lock() → mtx.Lock() の取得順序は既存コードと一貫しており、デッドロックリスクなし。
コンテキストのキャンセル伝播（linearizableReadEngineCtx(ctx, ...) など）は適切。goroutine リークやチャネルのブロッキングに関する新規問題は確認されなかった。

総評: 主要な変更（単一シャードでの読み取りセット検証の applyMu 下への移動）は正しく安全。マルチシャード TOCTOU は文書化済みの既知制約。4 のデッドコード整理を除けば、並行性・分散障害の観点での問題はない。

claude · 2026-04-13T05:53:22Z

[Concurrency] レビュー結果

対象: #499 (store: add read-set validation to ApplyMutations for SSI)

問題 1: `validateReadOnlyShards` の TOCTOU ウィンドウ — 重大 (設計上の限界)

場所: kv/sharded_coordinator.go — validateReadOnlyShards

コード自身がコメントで認めている通り、マルチシャードトランザクションで書き込みミューテーションを持たないシャードの read key 検証に TOCTOU ウィンドウが存在する。

linearizableReadEngineCtx(ctx, ...)  ← ここでラフトの適用待ち
↓
g.Store.LatestCommitTS(ctx, key)     ← ←  ← この間に別の txn がコミットしても検出不能

PR タイトルは「SSI を追加」と謳っているが、実際の保証は以下の通り非対称:

トランザクション種別	保証
シングルシャード	完全な SSI (`applyMu` 下でアトミック)
マルチシャード — 書き込みシャードの read key	完全な SSI (PREPARE リクエスト内で FSM がアトミックに検証)
マルチシャード — 読み取り専用シャードの read key	ベストエフォートのみ (TOCTOU 窓あり)

コメントで「dedicated read-validate FSM request phase が必要」と明記されているので、設計上の制限として認識されていることは理解できる。ただし PR 説明 / CHANGELOG でこの保証の非対称性を明示することを強く推奨する。「SSI」として外部向けに宣伝すると、マルチシャード環境で write skew が発生した際に原因調査が困難になる。

問題 2: `groupReadKeysByShardID` が `gid == 0` の read key をサイレントにドロップ — 低

場所: kv/sharded_coordinator.go — groupReadKeysByShardID

gid := c.engineGroupIDForKey(key)
if gid == 0 {
    continue  // ← ルートが見つからない read key を無言でスキップ
}

engineGroupIDForKey はルートが存在しない場合に 0 を返す。read key にルートがなければ PREPARE にも validateReadOnlyShards にも渡らず、コンフリクト検証が完全にスキップされる。

ルートが存在しない key をそもそも「読める」か？という問いはあるが、ルーティングテーブル更新中の一時的な不整合や nil/空 key の誤混入時に、無言スキップではなくエラーを返す方が安全。少なくとも slog.Warn でログを出すことを検討してほしい。

問題 3: `prewriteTxn` 内 `txnGroupForID` 失敗時の abort 漏れ — 既存バグ (参考情報)

場所: kv/sharded_coordinator.go — prewriteTxn ループ先頭

for _, gid := range gids {
    g, err := c.txnGroupForID(gid)
    if err != nil {
        return nil, err  // ← prepared 済みシャードが abort されない
    }

このコードパスは本 PR では変更されていないが、本 PR によって validateReadOnlyShards 失敗時の abort が正しく実装されたことで、この既存パスとの非対称性が際立つ。別 issue として起票を推奨する。

問題なし (確認済み)

単一シャード SSI: checkConflictsLocked が s.mtx.Lock() 下で write-write・read-write コンフリクトをアトミックに検証 → 完全な原子性を保証
フォロワー→リーダー proxy: coordinator.go の redirect パスで reqs.ReadKeys が onePhaseTxnRequest に正しく渡されている
コンテキスト伝播: ctx が prewriteTxn → validateReadOnlyShards → linearizableReadEngineCtx まで正しくスレッドされており、クライアントのキャンセル/タイムアウトが abortPreparedTxn を適切にトリガーする
abort シーケンス: validateReadOnlyShards 失敗時に abortPreparedTxn が全 PREPARE 済みシャードに対して正しく呼ばれる
ロック順序: mvccStore.ApplyMutations は s.mtx.Lock() 1 本のみ取得し、内部で追加のロックを取得しないためデッドロックリスクなし
ゴルーチンリーク: validateReadOnlyShards は同期的で goroutine を生成しない
Raft リーダー切り替え: FSM の Apply は全ノードで決定論的に実行されるため、read key コンフリクト検証が FSM 内で行われることは Raft の整合性と矛盾しない
read key の defensive copy: commit() 内で t.readKeys マップから新スライスにコピーしてから OperationGroup.ReadKeys に渡しており、並行アクセスに対して安全

claude · 2026-04-13T05:54:34Z

[Consistency] レビュー結果

対象PR: #499 (store: add read-set validation to ApplyMutations for SSI) および #483 (adapter: decompose ZSet storage into per-member wide-column keys)

PR #499 — SSIのための読み取りセット検証

⚠️ [中程度] マルチシャードSSIにおけるTOCTOUウィンドウ（既知の制限）

kv/sharded_coordinator.go の validateReadOnlyShards() は、書き込みを持たない読み取り専用シャードに対して FSM の applyMu ロックの外で競合チェックを行う。

// NOTE: This check is performed outside the FSM's applyMu lock, so there
// is a small TOCTOU window between the linearizable read barrier and the
// LatestCommitTS check.

コード自体にNOTE付きで文書化されており、実装者も認識している制限。ただし以下のシナリオで線形化可能性が破られる：

トランザクションTがシャードAに書き込み、シャードBからのみ読み取る
validateReadOnlyShards() がシャードBの線形化バリアを取得し、LatestCommitTS チェックを通過
その直後、別のトランザクションT'がシャードBのキーに書き込みコミット
TのシャードAへの ApplyMutations() が完了 → シャードBのSSI違反が検出されない

シングルシャードトランザクションおよび書き込みシャード上の読み取りキーは applyMu ロック内で正しく検証されており、完全なSSI保証がある。マルチシャードSSIを完全に保証するには「read-validate」FSMフェーズが必要。

アクション: 文書化済みのトレードオフのため許容可能だが、将来の改善タスクとして追跡することを推奨。

✅ read-set検証の移動（`validateReadSet` 削除）

adapter/redis.go の runTransaction() から validateReadSet() を削除し、FSMの applyMu ロック下で checkReadConflicts() を呼ぶ構成は正しい。applyMu ロック下での検証はcheck-then-actが原子的であり、旧実装より強い保証を提供する。

✅ ReadKeysの伝播経路

txnContext.commit() → OperationGroup.ReadKeys → Dispatch() → dispatchTxn() → FSM handleOnePhaseTxnRequest() → ApplyMutations(readKeys, ...) → checkReadConflicts() の経路は正しく実装されている。非トランザクションパス（handleRawRequest() 等）は nil を渡しており、SSIが不要な操作に適切。

PR #483 — ZSet wide-column分解

🔴 [重要] `buildZSetStateElems` でのTTL削除の非対称性

adapter/redis_compat_helpers.go の buildZSetStateElems にて：

case len(st.members) == 0 && st.fromLegacy:
    // Legacy: TTLキーを明示的に削除 ✅
    return []*kv.Elem[kv.OP]{
        {Op: kv.Del, Key: redisZSetKey(keyBytes)},
        {Op: kv.Del, Key: redisTTLKey(keyBytes)},  // TTL削除あり
    }, nil

case len(st.members) == 0:
    // Wide-column: buildZSetDiffElems に委譲 → TTLキー削除なし ⚠️
    return buildZSetDiffElems(keyBytes, st.origMembers, st.members)

legacyケースは redisTTLKey を明示的に削除するが、wide-columnケースは buildZSetDiffElems を使用しTTLキーを削除しない。TTL削除は buildTTLElems() に委ねられており、これはトランザクション内で ttlStates が適切に「削除済み」マークされている場合のみ機能する。

懸念: ZREM でZSetの全メンバーを削除するパスで ttlStates[key].value = nil がセットされない場合、TTLキーが孤立する。孤立したTTLキーが残ると、同じキーに対して後から作成されたZSetが誤ったTTLを継承するリスクがある（hasExpiredTTLAt が古いTTLエントリを参照するため）。

確認事項: zremTxn() および全メンバーが削除される全パスで ttlStates が適切に更新されることを確認してください。buildZSetStateElems のwide-columnケースに redisTTLKey の削除を追加するか、legacyケースと同様に明示的に削除することを検討してください。

⚠️ [軽微] `buildZSetDiffElems` による削除は `origMembers` のキーのみ対象

ZSet削除時（len(st.members) == 0 ケース）に buildZSetDiffElems を使うと、origMembers に含まれるキーのみが削除される。一方、deleteZSetWideColumnElems はフルプレフィックススキャンで孤立キーも削除する（コメント: "including any orphaned keys from partial writes or meta.Len inconsistencies"）。

SSI の競合検知が正常に機能している限り実運用上は問題ないが、meta.Len 不整合等の異常ケースで孤立したmember/scoreキーが残る可能性がある。防御的観点から、txnContextパスのZSet削除も deleteZSetWideColumnElems のフルスキャン方式に統一することを検討してください。

✅ ZSet wide-column read-set追跡

trackTypeReadKeys() に store.ZSetMetaKey(key) を追加したことは正しい。ZSetへの全書き込みは必ずメタキーを更新するため、メタキーのみの追跡でZSet全体の競合検知に十分。

✅ `deleteZSetWideColumnElems` のフルスキャン削除

deleteLogicalKeyElems 経由のZSet削除（DEL コマンド等）がフルプレフィックススキャンで全wide-columnキーを削除する実装は正しく、TTLキーも含めて削除されている。

✅ `buildZSetDiffElems` のメタLen更新

差分更新時、len(origMembers) != len(newMembers) の場合のみメタを更新する実装は正しい。スコアのみ変更の場合はLen不変であるため不要な書き込みを避けており、効率的かつ正確。

まとめ

#	PR	分類	内容
1	#499	⚠️ 中程度（既知の制限）	マルチシャードSSIのTOCTOUウィンドウ（コードに文書化済み）
2	#483	🔴 重要	wide-columnケースでのTTL削除非対称性 — 孤立TTLキーのリスク
3	#483	⚠️ 軽微	`buildZSetDiffElems` 削除が `origMembers` のみ対象（孤立キー対策として `deleteZSetWideColumnElems` の使用を検討）

claude · 2026-04-13T05:56:07Z

[Performance] レビュー結果

[中] `checkReadConflicts` がロック保持中に O(N) Pebble イテレータを生成

場所: store/lsm_store.go:864 (checkReadConflicts)、store/lsm_store.go:906 (ApplyMutations)

s.applyMu.Lock()
defer s.applyMu.Unlock()
// ...
if err := s.checkReadConflicts(ctx, readKeys, startTS); err != nil { ... }

func (s *pebbleStore) checkReadConflicts(...) error {
    for _, key := range readKeys {
        ts, exists, err := s.latestCommitTS(ctx, key) // ← キーごとに新規 Pebble イテレータを作成
        ...
    }
}

latestCommitTS はキーごとに s.db.NewIter(...) でイテレータを生成する。トランザクションが 100 キーを読んでいれば applyMu を保持したまま 100 個のイテレータ生成 (memtable + SST をマージする内部処理) が走り、並行する ApplyMutations を全期間ブロックする。

改善案: 1 つのイテレータを生成して各キーに対して SeekGE で移動するか、LowerBound/UpperBound を拡張した単一イテレータで複数キーを連続チェックすることでイテレータ生成コストと applyMu 保持時間を大幅に削減できる。

[低] `readKeys` の map→slice 変換アロケーション

場所: adapter/redis.go:1923

readKeys := make([][]byte, 0, len(t.readKeys))
for _, k := range t.readKeys {
    readKeys = append(readKeys, k)
}

t.readKeys は map[string][]byte で、スライスヘッダーのみをコピーしているためデータコピーは発生しない。ただし、トランザクションの read-set サイズ N に比例するスライスアロケーションが毎コミットで発生する。影響は小さいが、将来的には txnContext に [][]byte と map[string]struct{} を並列管理することで変換を省けることを付記しておく。

[参考] `validateReadOnlyShards` でのシャード数比例 RPC

場所: kv/sharded_coordinator.go

read-only シャードごとに LinearizableReadBarrier を発行するため、マルチシャードトランザクションでは read-only シャード数に比例してネットワーク往復が増える。これは SSI の正確性のために必要なコストだが、多数のシャードにまたがるトランザクションでは顕著なレイテンシ増大になりうる点をドキュメント化しておくことを推奨する。

claude · 2026-04-13T06:00:49Z

[Data Loss] レビュー結果

対象PR: store: add read-set validation to ApplyMutations for SSI

総評: データロストの問題なし

データロスト観点での危険なパスは検出されませんでした。以下、各チェックポイントの結果です。

Write/Delete操作でのデータ消失パス

問題なし。

ApplyMutations内の処理順序は applyMu ロック下で次の順に実行される:

checkConflicts (write-write 競合チェック)
checkReadConflicts (read-write 競合チェック) ← 今回追加
applyMutationsBatch (Pebble batch への書き込み)
batch.Commit(pebble.Sync) (fsync で永続化)

競合が検出された場合はステップ2または1で return err し、Pebble batch への書き込みは一切発生しない。データが意図せず消えるパスはない。

`validateReadSet` 削除の安全性 (redis.go)

問題なし。

runTransaction での明示的な validateReadSet 呼び出しが削除されたが、同等のチェックが ApplyMutations 内の applyMu ロック下で行われるようになった。旧実装は validateReadSet → commit 間に TOCTOU ウィンドウがあったため、新実装の方が安全。

WAL/Raftログへの永続化

問題なし。

checkReadConflicts は既存の latestCommitTS（内部実装）を使用しており、ロック取得の二重化を避けている
Pebble batch は pebble.Sync オプションでコミットされており、fsync による永続化が保証されている
ReadKeys は proto の Request.read_keys フィールド (field number 5) として Raft ログに含まれる

エラーハンドリングによるデータ消失

問題なし。

checkReadConflicts でエラー発生時は batch への書き込み前に return err する
commit フェーズ (applyCommitWithIdempotencyFallback) では readKeys = nil を渡しており、PREPARE フェーズで既に検証済みの設計として正しい
abort フェーズ (handleAbortRequest) でも readKeys = nil を渡しており、abort 処理に競合検証は不要なため正しい

マルチシャードトランザクションの abort 安全性

問題なし。

validateReadOnlyShards が失敗した場合、abortPreparedTxn が呼ばれ PREPARE 済みシャードをクリーンアップする。abort 処理が失敗しても既存データは変更されておらず (PREPARE は新規書き込み)、データロストは発生しない。

備考（データロスト範囲外）

以下は isolation の弱さに関する観点であり、データロストではないが念のため記録:

validateReadOnlyShards の TOCTOU ウィンドウ（コード内にコメントあり）はwrite skew anomaly を引き起こしうるが、既存データの消失ではない
groupReadKeysByShardID で gid == 0（ルート未解決）のキーをスキップする実装は SSI の false negative になりうるが、データロストではない

claude · 2026-04-13T06:02:48Z

[Performance] レビュー結果

[中] `checkReadConflicts` が `applyMu` ロック保持中に O(R) のディスク I/O を実行

store/lsm_store.go の pebbleStore.ApplyMutations は applyMu 書き込みロックを保持したまま以下を逐次実行します。

checkConflicts(mutations)     // O(W) Pebble point lookups
checkReadConflicts(readKeys)  // O(R) Pebble point lookups  ← 今回追加
applyMutationsBatch(...)

checkReadConflicts の各 latestCommitTS 呼び出しはストレージ層への point lookup であり、read key 数に比例してロック保持時間が延びます。Redis の WATCH / トランザクション命令では read set が write set より大きくなるケース（例: KEYS スキャン後に一部キーを更新）が想定されます。checkConflicts（write-write 側）と同様の問題ですが、read set はより大きくなりやすいため影響が出やすいです。

[小] `validateReadOnlyShards` での per-key 逐次 LatestCommitTS 呼び出し

kv/sharded_coordinator.go の内ループ:

for _, key := range keys {
    ts, exists, err := g.Store.LatestCommitTS(ctx, key)
    ...
}

同一シャード内の read keys を逐次処理しています。multi-shard SSI はレアケースであり優先度は低いですが、同一シャードの複数キーをバッチ読み取りできると改善できます。

claude · 2026-04-13T06:03:28Z

[Concurrency] レビュー結果

対象PR: #499 store: add read-set validation to ApplyMutations for SSI

問題点

[高] `groupReadKeysByShardID` — ルーティング不能なリードキーが無音でスキップされる

kv/sharded_coordinator.go の新関数 groupReadKeysByShardID で、engineGroupIDForKey(key) が 0 を返した場合（シャードルートが未設定のキー）、当該リードキーは continue で無音スキップされる。

gid := c.engineGroupIDForKey(key)
if gid == 0 {
    continue  // read key の競合チェックが完全にスキップされる
}

結果として、そのキーへの並行書き込みが startTS 以降に発生していても検出されず、SSI 保証が黙って崩れる。通常運用では全キーにルートが存在するはずだが、シャードマイグレーション中や設定ミス時に無告知で安全性が低下する。

対処案: ルートが見つからない場合はエラーを返す（もしくは少なくともコメント/ログで明示する）。

[中] `validateReadOnlyShards` — TOCTOU 競合ウィンドウ（既文書化、但し利用者への周知不足）

マルチシャードトランザクションで書き込みを持たないシャードのリードキーは、FSM の applyMu ロック外で検証される。

linearizableReadEngineCtx()  ←── ここで線形読み取りバリア
         ↓
LatestCommitTS(key)          ←── 並行コミットがここで割り込む可能性

この窓で別トランザクションがコミットしても検出されない。コードには NOTE: で説明されているが、呼び出し元（coordinator 利用者）から見えない。validateReadOnlyShards の戻り値や呼び出し箇所でも明示的に「マルチシャードの read-only shard については SSI が best-effort」と伝えるコメントがあると良い。また、この制約は validateReadOnlyShards が prewriteTxn に内包されているため、外部 API ユーザーから隠蔽されている点も注意。

問題なし / 正しい実装

観点	判定	根拠
`applyMu` 下での read-write 競合チェック	✓	`checkConflicts`(write-write) → `checkReadConflicts`(read-write) の順で同一 `applyMu.Lock()` 内に実行。アトミック性確保。
`latestCommitTS` の再帰的なロック取得	✓	`checkReadConflicts` は内部版 `s.latestCommitTS`（`dbMu` 再取得なし）を使用。`ApplyMutations` が既に `dbMu.RLock()` を保持しているため正しい。
Raft リプレイの決定性	✓	`ReadKeys` は `pb.Request` (Raft ログエントリ) に永続化されるため、リーダー切り替え後のリプレイで同一の競合チェックが再実行される。
`handleOnePhaseTxnRequest` への `r.ReadKeys` 伝搬	✓	ワンフェーズコミットで `r.ReadKeys` が正しく `ApplyMutations` に渡される。
`handlePrepareRequest` での `r.ReadKeys` 適用	✓	2PC PREPARE フェーズで read 競合を検出する設計として妥当。`startTS, startTS` の使用も PREPARE 時点では commitTS 未確定のため正しい。
旧 `validateReadSet` の削除	✓	`ApplyMutations` 内でロック下に検証されるため、アダプタ層での事前チェックは TOCTOU を内包しており削除が正しい。
コンテキスト・タイムアウト伝搬	✓	`txnContext.commit()` が `redisDispatchTimeout` 付きの新規 ctx を生成。`validateReadOnlyShards` も ctx を受け取りリードバリアに渡している。
goroutine リーク・チャネルブロック	✓	新規 goroutine の起動はなし。

claude · 2026-04-13T06:03:42Z

[Consistency] レビュー結果

概要

SSI（Serializable Snapshot Isolation）実装のために読み取りセット（read-set）検証を ApplyMutations 内部へ移動した変更です。単一シャードのトランザクションについては applyMu ロック下での原子的な read-write 競合検出が正しく実現されており、基本設計は妥当です。

問題点

[HIGH] 複数シャードにまたがる読み取り専用シャードの TOCTOU 窓（SSI 不完全）

対象: kv/sharded_coordinator.go の validateReadOnlyShards

書き込みがない読み取り専用シャードの競合チェックは、各書き込みシャードの ApplyMutations が FSM の applyMu ロック下で検証するのとは異なり、FSM の外で実行されます。コードにコメントとして記載されているとはいえ、Raft にコミットされた書き込みがリニアライザブルバリア通過後・LatestCommitTS チェック前に適用された場合、その競合が検出されません。複数キーのループ内でも同様の窓が存在します（あるキーのチェック後に、そのキーへの書き込みが入る等）。

現状の動作を許容する場合でも、「単一シャードトランザクションのみ完全な SSI が保証される」という制約を Coordinator インターフェース等の API コメントに明示することを推奨します。

[MEDIUM] `groupReadKeysByShardID` がルート未解決キーをサイレントに無視する

対象: kv/sharded_coordinator.go の groupReadKeysByShardID

gid := c.engineGroupIDForKey(key)
if gid == 0 {
    continue   // 競合チェックがスキップされる
}

groupMutations は書き込みキーのルートが見つからない場合にエラーを返すのに対し、groupReadKeysByShardID は読み取りキーの場合にサイレントにスキップします。この非対称な処理により、ルーティングテーブルに存在しない読み取りキーの read-write 競合が検出されません。

シャード分割・移動等のエッジケースで読み取り専用シャードへの競合が見逃される可能性があります。少なくとも警告ログを出すか、書き込みキーと同様にエラーとして扱うことを検討してください。

同様に validateReadOnlyShards 内でも c.groups[gid] が見つからない場合にサイレントスキップしています。

[LOW] `validateReadSet` がデッドコードになる

対象: adapter/redis.go

PR により runTransaction からの validateReadSet 呼び出しが削除されますが、validateReadSet メソッド自体は txnContext に残ります。残存するデッドコードは将来的に混乱を招く可能性があります。削除を推奨します。

正しく実装されている点

pebbleStore.ApplyMutations と mvccStore.ApplyMutations における checkConflicts・checkReadConflicts の両者が applyMu ロック下で原子的に実行されており、単一シャードにおける SSI の線形化可能性は正しく保証されています。
txnContext.readKeys にはストレージレイヤーのエンコーディング済みキー（redisStrKey、listMetaKey、redisTTLKey 等）が格納されており、ストア層のキーと正しく一致します。
書き込み操作がゼロの場合（len(elems) == 0）に commit() が早期リターンする動作は、write skew には最低 1 つの書き込みが必要であるため、SSI 上の問題はありません。
handlePrepareRequest での r.ReadKeys 渡しと handleOnePhaseTxnRequest での r.ReadKeys 渡しは正しく動作します。
TTL 付きデータの競合検出も redisTTLKey が readKeys に追加されており、TTL とデータ本体の整合性が保たれています。

claude · 2026-04-13T06:03:53Z

[Test Coverage] レビュー結果

対象PR: #499 store: add read-set validation to ApplyMutations for SSI

不足しているテスト

🔴 HIGH: `validateReadOnlyShards()` のテストが皆無

kv/sharded_coordinator.go に追加された74行の新ロジック（validateReadOnlyShards）に対してテストが一切ありません。マルチシャードトランザクションのread-onlyシャード上でのread-write競合検出という中核ロジックです。

提案テストケース:

// TestShardedCoordinator_ValidateReadOnlyShards_DetectsConflict
// read-onlyシャードのキーがstartTS以降に更新済み → ErrWriteConflict

// TestShardedCoordinator_ValidateReadOnlyShards_NoConflict
// read-onlyシャードのキーがstartTS以前のみ更新 → nil

// TestShardedCoordinator_ValidateReadOnlyShards_NonexistentKey
// read-onlyシャードに存在しないキー → nil (競合なし)

// TestShardedCoordinator_ValidateReadOnlyShards_SkipsWriteShards
// writeGIDsに含まれるシャードはスキップされる

// TestShardedCoordinator_ValidateReadOnlyShards_GroupNotFound
// groups[gid]が存在しないシャードID → 安全にスキップ

// TestShardedCoordinator_ValidateReadOnlyShards_LinearizableReadError
// linearizableReadEngineCtx がエラーを返す → エラー伝播確認

🔴 HIGH: `groupReadKeysByShardID()` のテストがない

kv/sharded_coordinator.go に追加されたルーティングヘルパーに単体テストがありません。

提案テストケース:

// TestGroupReadKeysByShardID_Nil → nilを返す
// TestGroupReadKeysByShardID_MultiShard → 複数シャードへの正しいルーティング
// TestGroupReadKeysByShardID_UnroutableKey → gid==0のキーはスキップ

🔴 HIGH: `ShardedCoordinator` の end-to-end ReadKeys テストがない

kv/sharded_coordinator_txn_test.go の既存テストはすべて ReadKeys: nil で実行されており、ReadKeysを実際に設定した場合の挙動が一切検証されていません。

提案テストケース:

// TestShardedCoordinatorDispatchTxn_SingleShard_ReadConflict
// 単一シャードTxn: ReadKeysのキーがstartTS後に更新済み → コミット失敗

// TestShardedCoordinatorDispatchTxn_MultiShard_ReadKeyOnWriteShard
// マルチシャードTxn: 書き込みシャード上のReadKeysが競合 → PREPARE失敗

// TestShardedCoordinatorDispatchTxn_MultiShard_ReadKeyOnReadOnlyShard
// マルチシャードTxn: read-onlyシャード上のReadKeysが競合 → validateReadOnlyShards経由でエラー

🟡 MEDIUM: FSMレイヤーの ReadKeys テストがない

kv/fsm_txn_test.go および kv/fsm_occ_test.go に、pb.Request.ReadKeys を設定してFSMを通じたread-write競合検知を検証するテストがありません。handleOnePhaseTxnRequest と handlePrepareRequest の新パス（kv/fsm.go）が直接テストされていません。

提案テストケース:

// TestKvFSM_OnePhaseTxn_ReadKeyConflict
// ReadKeysを含むRequest → ApplyMutations経由でErrWriteConflict

// TestKvFSM_Prepare_ReadKeyConflict
// PREPARE phaseでReadKeysが競合 → エラー返却

🟡 MEDIUM: `adapter/redis.go` の `validateReadSet` 削除に対するリグレッションテストがない

runTransaction から validateReadSet 呼び出しが削除されました。これはバリデーションをストア層（ApplyMutations）に委譲する設計変更ですが、Redisアダプター経由で動作することを検証するintegration testがありません。

現在の TestRedisTxnValidateReadSetDetectsStaleListMeta は validateReadSet を直接呼び出しており、本番パス（runTransaction → commit() → Dispatch() → ApplyMutations）を通りません。validateReadSet は本番パスから呼び出されなくなっているためデッドコードになっている可能性があります。

提案テストケース:

// TestRedis_MULTI_EXEC_ReadConflict
// MULTI/EXEC トランザクションで読んだキーが別ゴルーチンに更新される
// → トランザクションがErrWriteConflictで失敗することを検証

問題なし

レイヤー	状況
`store/lsm_store` (PebbleStore)	✅ 6ケース追加で十分
`store/mvcc_store`	✅ 4ケース追加で十分
`kv/coordinator`	✅ ReadKeys伝播テストあり (`TestCoordinateDispatchTxn_ReadKeysInRequest`)
`kv/sharded_coordinator`	❌ `validateReadOnlyShards`, `groupReadKeysByShardID`, end-to-end ReadKeys 未テスト
`kv/fsm`	❌ ReadKeysを使ったFSMレベルテスト未テスト
`adapter/redis`	⚠️ validateReadSet削除の回帰テスト不足
`adapter/s3`	✅ 既存テスト更新のみ

最も重要なのは validateReadOnlyShards() のテスト不足です。マルチシャード環境でのSSI保証の中核部分が一切検証されていません。

claude bot mentioned this pull request Apr 12, 2026

adapter: decompose ZSet storage into per-member wide-column keys #483

Open

bootjp enabled auto-merge April 13, 2026 05:42

github-actions bot reviewed Apr 13, 2026

View reviewed changes

Conversation

bootjp commented Apr 12, 2026

Uh oh!

gemini-code-assist bot commented Apr 12, 2026

Uh oh!

claude bot commented Apr 12, 2026

[Test Coverage] レビュー結果

総評

不足しているテスト

1. pebbleStore.checkReadConflicts のユニットテスト（store/lsm_store_txn_test.go）

2. mvccStore の読み込みキー競合チェックのテスト（store/mvcc_store_snapshot_test.go）

3. Write Skew（書き込みスキュー）防止の統合テスト（最重要）

4. 並行読み書き競合テスト（store/mvcc_store_concurrency_test.go 相当）

5. adapter/redis_txn_test.go — readKeys の伝播テスト

軽微な指摘

まとめ

Uh oh!

claude bot commented Apr 12, 2026

[Concurrency] レビュー結果

🔴 HIGH: マルチシャードトランザクションで readKeys が無声破棄される

🟡 MEDIUM: checkReadConflicts がコンテキストを無視している

🔵 INFO: アダプタ層の二重バリデーションとフォロワーの古いデータ

Uh oh!

claude bot commented Apr 12, 2026

[Consistency] レビュー結果

問題1（Critical）: クロスシャードトランザクションで readKeys が無視される

問題2（Minor）: checkReadConflicts がコンテキストを無視する

確認事項（設計意図の明確化を推奨）

Uh oh!

claude bot commented Apr 12, 2026

[Performance] レビュー結果

1. リードセットの二重検証（高影響）

2. applyMu.Lock() 保持中に O(N) 逐次 Pebble ポイントルックアップ（高影響）

3. 書き込みセットと読み取りセットの重複チェック（中影響）

4. context.Background() の使用（低影響）

Uh oh!

claude bot commented Apr 12, 2026

[Data Loss] レビュー結果

結論: 重大なデータロストリスクは検出されず。ただし1件の隔離レベル低下による潜在リスクあり。

問題なし: コアの書き込みアトミシティ

懸念: マルチシャードトランザクションで read-set 検証がアトミックにならない

軽微: checkReadConflicts でコンテキストが無視されている

総括

Uh oh!

claude bot commented Apr 12, 2026

[Performance] レビュー結果

問題 1 — checkReadConflicts が apply ロック保持中に N 個の Pebble イテレータを生成する（重要）

問題 2 — context.Background() がループ内で使われている（軽微）

問題 3 — 読み取りセットを2回 Pebble 検索する（中程度）

問題 4 — readKeys のスライス化（軽微）

Uh oh!

claude bot commented Apr 12, 2026

[Test Coverage] レビュー結果

🔴 重大: 新規コアロジックに専用テストがない

不足しているテストケース (具体的な提案)

1. checkReadConflicts — 読み書きコンフリクト検出 (最重要)

2. checkReadConflicts — コンフリクトなし (正常系)

3. 境界値: nil/空の readKeys は no-op

4. 存在しないキーを readKeys に含む場合

5. mvccStore の readKeys コンフリクト検出

6. エンドツーエンド: Write Skew 防止 (SSIの本質的な検証)

7. 並行アクセス: readKeys コンフリクト検出の競合安全性

8. ReadKeys がプロトコルバッファ経由で正しく伝播するか

既存テストへの影響

まとめ

Uh oh!

claude bot commented Apr 12, 2026

[Consistency] レビュー結果

問題1（重大）: マルチシャードトランザクションで readKeys が無視される

問題2（軽微）: checkReadConflicts がコンテキストを無視している

まとめ

Uh oh!

claude bot commented Apr 12, 2026

[Concurrency] レビュー結果

[HIGH] マルチシャードトランザクションで readKeys が伝播されない

[MEDIUM] checkReadConflicts がコンテキストを無視している

[LOW] readKeys スライスの順序が非決定的

Uh oh!

claude bot commented Apr 12, 2026

[Data Loss] レビュー結果

1. `pebbleStore.checkReadConflicts` のユニットテスト（`store/lsm_store_txn_test.go`）

2. `mvccStore` の読み込みキー競合チェックのテスト（`store/mvcc_store_snapshot_test.go`）

4. 並行読み書き競合テスト（`store/mvcc_store_concurrency_test.go` 相当）

5. `adapter/redis_txn_test.go` — readKeys の伝播テスト

🔴 HIGH: マルチシャードトランザクションで `readKeys` が無声破棄される

🟡 MEDIUM: `checkReadConflicts` がコンテキストを無視している

問題1（Critical）: クロスシャードトランザクションで `readKeys` が無視される

問題2（Minor）: `checkReadConflicts` がコンテキストを無視する

2. `applyMu.Lock()` 保持中に O(N) 逐次 Pebble ポイントルックアップ（高影響）

4. `context.Background()` の使用（低影響）

軽微: `checkReadConflicts` でコンテキストが無視されている

問題 1 — `checkReadConflicts` が apply ロック保持中に N 個の Pebble イテレータを生成する（重要）

問題 2 — `context.Background()` がループ内で使われている（軽微）

問題 4 — `readKeys` のスライス化（軽微）

1. `checkReadConflicts` — 読み書きコンフリクト検出 (最重要)

2. `checkReadConflicts` — コンフリクトなし (正常系)

5. `mvccStore` の readKeys コンフリクト検出

問題2（軽微）: `checkReadConflicts` がコンテキストを無視している

[HIGH] マルチシャードトランザクションで `readKeys` が伝播されない

[MEDIUM] `checkReadConflicts` がコンテキストを無視している

[LOW] `readKeys` スライスの順序が非決定的

✅ Write前の競合チェック順序（`store/lsm_store.go`）

✅ `mvccStore.ApplyMutations`（`store/mvcc_store.go`）

🔍 軽微: `checkReadConflicts` がコンテキストを無視（`store/lsm_store.go`）

1. `mvccStore` の読み取り競合テストが皆無

3. 空スライス (non-nil empty) の `readKeys` テストがない

1. 不要なスライスコピー ( ~1923行)

3. 全シャードへの readKeys 送信 ( ~277行, `prewriteTxn`)

[Bug] `checkReadConflicts` がコンテキストを無視している

2. 軽微: `checkReadConflicts` でのコンテキスト破棄

問題1: `checkReadConflicts` が `applyMu` 保持中に O(|readKeys|) のディスクI/O を実行する（中〜高）

問題2: `groupReadKeysByShardID` で内部スライスの pre-alloc がない（低）