Hare/yoi
1
1
Fork 0
Code Actions Packages Releases Activity
55c5ac4942
yoi/tickets/entry-hash-abolish.review.md

7.6 KiB
Raw Blame History

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 のタイミングで吸収できる内容。