ticket: entry-hash-abolish レビュー (Approve)
This commit is contained in:
parent
d5dff6d17b
commit
6e791d8668
|
|
@ -40,3 +40,9 @@ write 経路は既に sync 化済み (`6e5b148`)。前提足場は揃ってい
|
|||
- `crates/session-store/src/session.rs`
|
||||
- `crates/pod/src/pod.rs:1591` `ensure_head_or_fork` 経路
|
||||
- `tickets/segment-rename.md` (後続)
|
||||
|
||||
## Review
|
||||
|
||||
- 状態: Approve
|
||||
- レビュー詳細: [./entry-hash-abolish.review.md](./entry-hash-abolish.review.md)
|
||||
- 日付: 2026-05-20
|
||||
|
|
|
|||
56
tickets/entry-hash-abolish.review.md
Normal file
56
tickets/entry-hash-abolish.review.md
Normal file
|
|
@ -0,0 +1,56 @@
|
|||
# Review: session-store: Entry hash chain の廃止
|
||||
|
||||
対象コミット: `d5dff6d update: entry hash chain と session_head mutex を撤廃`
|
||||
ベース: `develop`
|
||||
|
||||
## 前提・要件の確認
|
||||
|
||||
- `HashedEntry` 廃止 / JSONL = 1 行 1 `LogEntry`: 達成。
|
||||
- `crates/session-store/src/session_log.rs` から型・関連 impl が削除。
|
||||
- `FsStore::append` / `read_all` / `create_session` 全部 `&LogEntry` / `Vec<LogEntry>` に書き換え (`crates/session-store/src/fs_store.rs:68-110`)。
|
||||
- `compute_hash` / `build_chain` / `EntryHash` 撤去: 達成。`grep` でコードベース上に残存なし。`crates/session-store/src/lib.rs:48` の re-export からも消えている。
|
||||
- `SessionOrigin.at_hash → at_turn_index`: 達成 (`crates/session-store/src/session_log.rs:167-177`)。doc コメントで「`TurnEnd` の `turn_count` 由来。`0` は SessionStart 直後」と意味も明文化済み。
|
||||
- `ensure_head_or_fork` を末尾比較ベースに置換: 達成。entry count 比較に統一 (`crates/session-store/src/session.rs:99-118`、Pod 側は `crates/pod/src/pod.rs:1619-1666`)。`Store::read_head_hash` も `read_entry_count` に改名されインタフェースとして整合 (`crates/session-store/src/store.rs:58-64`)。
|
||||
- `session_head` mutex 撤去 / writer ハンドル = `Arc`: 達成。`Arc<SyncMutex<SessionHead>>` を `Arc<SessionState>` (`ArcSwap<SessionId>` + `AtomicUsize`) に置換し (`crates/pod/src/pod.rs:45-86`)、`parking_lot` 依存も削除 (`crates/pod/Cargo.toml:33`)。`LogWriterHandle` は store + state + sink の薄い 3 タプル。
|
||||
- 既存 JSONL の読み込み互換不要: 仕様通り。新フォーマットは「素の `LogEntry` (`tag = "kind"`)」で、旧 `HashedEntry` ラッパー行は deserialize できないが、ticket の明示的な範囲外。
|
||||
|
||||
## 完了条件の対応
|
||||
|
||||
- `HashedEntry / prev_hash / compute_hash / build_chain / EntryHash` 残存無し: 確認 (`grep -r` で hit 無し、コメント/テスト中の文字列残存のみ — 後述 nit)。
|
||||
- `SessionOrigin.at_turn_index` & `fork_at(source_id, at_turn_index)`: 確認 (`session.rs:373-409`)。
|
||||
- `session_head` mutex 参照消失 / Writer は `Arc` のみ: 確認 (`grep -rn parking_lot|SyncMutex|SessionHead crates/pod/src/ crates/session-store/src/` で 0 件)。
|
||||
- `cargo check --workspace` / `cargo test -p session-store -p pod`: 通る。
|
||||
- `pod` 側で `double_run_returns_error` がたまに失敗するが、`KNOWN_ISSUES.md` に既知 flakiness として登録済み (本チケット起因ではない、再実行で pass)。
|
||||
- 既存 session を新規再生成して 1 行 1 `LogEntry`: 形式上は `FsStore::append` が `serde_json::to_string(&LogEntry)` を 1 行で書き出すだけになっており、目視確認できる構造である。
|
||||
|
||||
## アーキテクチャ・スコープ
|
||||
|
||||
- スコープ厳守: 範囲外と宣言した「`SessionId → SegmentId` rename / Session grouping / live-fork marker 形式の最終決定」のいずれにも踏み込んでいない。`fork_at` の API は `at_turn_index: usize` という最小限の置換で止まっており、後続チケットの設計余地を温存している。
|
||||
- レイヤ境界: `session-store` は依然として低レベル persistence、Pod が状態保持と fork 判定を行うという責務分割が維持されている。`LogWriterHandle` / `SessionState` は pod クレート内で完結し、外部に新しい抽象を生やしていない。
|
||||
- 不要な後方互換無し: 旧 JSONL の読み出しを試みる shim も無く、設計方針 (CLAUDE.md「変更量を最小にするために設計を歪めたり、設計問題に対して不必要な後方互換性を作らない」) に合致。
|
||||
- 依存追加 `arc-swap = "1.9.1"`: `cargo add` で入った形跡 (pinned patch version)。`manifest` クレートで既に同 crate を使っており、新規導入ではない。プレフィックス無し短い名前ポリシーと無関係 (third-party)。問題なし。
|
||||
|
||||
## 指摘事項
|
||||
|
||||
### Blocking
|
||||
|
||||
なし。
|
||||
|
||||
### Non-blocking / Follow-up
|
||||
|
||||
- **mirror と disk の順序乖離リスク** — `crates/pod/src/pod.rs:622-636` (`commit_entry`) と `crates/pod/src/pod.rs:108-117` (`LogWriterHandle::append_entry`) は、`store.append` (kernel が直列化) と `sink.publish` (mirror mutex 内で直列化) を別ロックで行う。現状の Pod runtime では実質的に append は単一タスク (worker.run 駆動経路) に閉じているため race は発生しない。ただし将来「Pod の独立タスクから `LogWriterHandle::append_entry` を発火する」変更が入ると、kernel 順序と mirror 順序が一致しない可能性がある (disk に B が後着でも mirror の `subscribe_with_snapshot` プレフィックスでは A の前に B が並ぶ等)。ticket は「`O_APPEND` は kernel が atomic に直列化」までしか論証しておらず、mirror 側の不変条件は明示されていない。後続 `live-fork-marker` で marker entry 形式を確定する際に併せて「mirror の順序保証は append 経路の単一性に依存する」旨を pod 側 doc に書き残しておくと事故防止になる。
|
||||
- **`FsStore::read_entry_count` の O(n) 読み込み** — `fs_store.rs:121-122` は `fs::read_to_string` でファイル全体を読んで行数を数える。ターンの度に `ensure_head_or_fork` の中で呼ばれるため、長期セッション (数千 entry) では毎ターン無駄が出る。代替: ファイルサイズ + 末尾シーク + line counter、もしくは entry 数が膨らんできた段階で別途 metadata ファイル化。`live-fork-marker` チケットで marker entry を導入する際の見直しポイント。
|
||||
- **stale doc / test コメント** — 以下に旧用語 (`head_hash` / `SessionHead`) が残っており、ticket 完了後にも誤読を招く:
|
||||
- `crates/session-store/src/lib.rs:22` doc example の `let (session_id, head_hash) = create_session(...)` (API は既に `SessionId` 単独を返す)。
|
||||
- `crates/pod/tests/restore_test.rs:66` のコメント「`collect_state` returns `head_hash = None`」。
|
||||
- `crates/pod/tests/compact_events_test.rs:523-598` の `SessionHead` / `head_hash` を前提とした説明文。テスト自体は session_id 比較で正しく動くが、説明が現実装と乖離している。
|
||||
本チケット範囲を厳密に解釈すれば追従不要だが、近くを触る次のチケット (`segment-rename` 等) で同時に潰しておくのが望ましい。
|
||||
|
||||
### Nits
|
||||
|
||||
- `crates/pod/Cargo.toml:33` は `arc-swap = "1.9.1"` と patch まで pin されている。`manifest` 側は `"1"`。整合の必要はないが、workspace.dependencies に上げる選択肢もある (将来 1.x bump 時に一箇所で動く)。
|
||||
- `pod.rs:2148-2165` 圧縮経路で `let old_session_id = self.session_state.session_id();` の直後に `let w = self.worker.as_ref().unwrap();` を 2 回行っており (1 度目は turn_count 用、2 度目は entry 構築用)、まとめて 1 つの `let w = ...` で済む。可読性のみの話。
|
||||
|
||||
## 判断
|
||||
|
||||
**Approve** — チケットの要件・完了条件は全て充足され、削除と置換が clean に行われている。`session_state` の lock-free 化はチケットの論拠 (single-appender + `O_APPEND` 原子性) の範囲内では正しく、`ensure_head_or_fork` の entry-count 比較は hash 比較と検出力上等価 (writer の自前 tally と on-disk 行数の乖離 = 他プロセス侵入)。指摘した non-blocking 項目はいずれも本チケットのスコープ外、または後続 `live-fork-marker` / `segment-rename` のタイミングで吸収できる内容。
|
||||
Loading…
Reference in New Issue
Block a user