Skip to content

Feature: rocksdb#142

Open
junjihashimoto wants to merge 5 commits into
masterfrom
feature/rocksdb
Open

Feature: rocksdb#142
junjihashimoto wants to merge 5 commits into
masterfrom
feature/rocksdb

Conversation

@junjihashimoto

@junjihashimoto junjihashimoto commented Mar 18, 2026

Copy link
Copy Markdown
Member
  • RocksDBバックエンドの追加。
  • WALを使ったインクリメンタルレプリケーションをサポート。
  • github actionで新旧のモードの両方をテスト。
  • エラーモードへの対応(AZ障害を考慮したネットワーク障害、スプリットブレイン対策)
  • 新機能にともなうログの追加

@junjihashimoto junjihashimoto force-pushed the feature/rocksdb branch 2 times, most recently from 055960e to 9ee9097 Compare March 29, 2026 06:12
@junjihashimoto junjihashimoto force-pushed the feature/rocksdb branch 2 times, most recently from da313b9 to 066cbdb Compare April 10, 2026 18:27
Introduce storage_rocksdb as a new pluggable storage backend alongside
storage_tcb/storage_tch. Behavior matches storage_tcb line-by-line for
set/get/remove/incr paths so the common storage test suite passes
identically (1618 tests, 100%).

Key points:
- storage_rocksdb.{h,cc}: RocksDB-backed storage with header cache for
  deleted entries, snapshot-isolated iteration (localized ReadOptions
  so Get() in set/remove/incr always sees latest state), and version
  checking semantics matching storage_tcb.
- op_repl_sync_wal.{h,cc}: WAL-based incremental replication op.
- handler_dump_replication, op_meta, flared: wire up the new backend
  and replication path.
- test/lib/test_storage_rocksdb.cc: run the common storage test suite
  against the RocksDB backend.
- Build: configure.ac, Makefile.am, flake.nix, nix/default.nix,
  cutter.patch, CI workflow, Dockerfile.test, BUILD.md,
  ROCKSDB_REPLICATION.md.
The repartition test in flare-tests asserts a statistical bound
(deviation < 0.3) on QuickCheck-generated key distribution and fails
intermittently. Retry the nix test build up to 3 times (failed nix
builds are not cached, so a retry re-runs the tests).

Also stop running the workflow twice per PR commit (push + pull_request
on the same SHA) by limiting push triggers to master, and cancel
superseded in-progress runs.
Protocol (F1/F2/F3/F7):
- Redesign repl_sync_wal as a push protocol: the replication source
  asks the destination for its recorded position (repl_sync_wal begin),
  streams its own WAL delta, and the destination applies it. The old
  integration ran the exchange backwards (the push handler pulled the
  destination's WAL) and never read the response, so incremental sync
  could never succeed and left unread bytes desynchronizing the shared
  connection.
- Add repl_sync_wal seed: after a successful full dump the source
  records its lineage token and the covered WAL position on the
  destination, enabling incremental resyncs from then on.
- Detect purged-WAL gaps in get_updates_since(): GetUpdatesSince()
  returns OK positioned at the first available batch even when the
  requested sequence was purged, so verify continuity instead of
  relying on IsNotFound(); fetch in bounded chunks (has_more) instead
  of materializing the whole retention window in RAM.
- Guard all client/server stream parsing with try/catch, read exact
  sizes via readsize(), and bound batch sizes with a hard limit plus
  the configured ceiling on both ends; drain refused batches so the
  connection stays framed. Streaming throttle now runs on the sending
  side, making the rocksdb-wal-* ini options effective.
- Filter reserved metadata keys out of applied batches so a peer's
  markers can never overwrite ours; stop applying after the first
  failed batch (no gaps).

Accounting (F4):
- Record resync success/failure from the actual outcome (op_set or
  iterator failure counts as failure; graceful shutdown is not
  counted), reset the failure streak on WAL success too, and account
  for early connect/iter_begin failures.

Concurrency (F5/F6/F8):
- Serialize iter_begin/iter_end with a dedicated mutex (busy return no
  longer leaks the whole-lock; concurrent iterations no longer race).
- Protect _master_id with a mutex and return it by value.
- truncate() now takes the whole lock exclusively and batches deletes;
  apply_batch* also runs exclusively.

Behavior/perf (F8/F9):
- incr matches storage_tcb: clamp to UINT64_MAX on overflow and reap
  expired records; store with a single Put instead of a second Get via
  set().
- count() is O(1) via an exact maintained record counter (persisted on
  clean close, re-counted after a crash, delta-tracked through WAL
  applies) instead of a whole-DB scan per stats request.

Build (F10):
- RocksDB is now an explicit opt-in: plain ./configure no longer links
  librocksdb just because it exists on the build host.
- Add rocksdb to the dev shell.

Tests (F11):
- New test_op_repl_sync_wal.cc covers both protocol sides: framing,
  lineage mismatch, seed, abort/ack synchronization, oversized and
  malformed input, and a client-to-server roundtrip.
- New storage tests: reopen persistence (data + repl_last_lsn), incr
  overflow clamp, count maintenance across restart/truncate/WAL apply,
  fetch budget progress, reserved-key filtering.

Docs: ROCKSDB_REPLICATION.md updated to the push protocol;
ROCKSDB_REVIEW.md added (review viewpoints, verdicts with evidence,
fix status, verification checklist).

@junjihashimoto junjihashimoto left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

再レビュー (138286a) — F1〜F11 の修正確認 + 新規指摘

お疲れさまです。前回指摘 (ROCKSDB_REVIEW.md の F1〜F11) への対応をコードで追試しました。修正はいずれも本物で質が高いです:

  • F1(応答未読/方向逆転/血統): push 型プロトコルへの再設計を確認。begin/seed の全失敗経路(サーバ拒否・クライアント ABORT・mid-stream 失敗のドレイン・desync 時の再接続)で行同期が保たれていること、_read_final_result/_abort_stream で全応答をパースしていることを確認しました。
  • F3: 4 箇所の lexical_cast が両側で try/catch 保護、BATCH サイズは uint64 で解析し 1GiB ハード上限 + 受信側 ini 上限で拒否、readsize() の使用も正しい(短絡読みなし)。
  • F4: resync 計上の全 8 出口パスの分類が正しくなり、_notify_resync_result に集約。WAL 成功でストリークがリセットされ、graceful shutdown は計上されないことを確認。
  • F5/F6/F7/F9/F10: iter ロックの原子化、_master_id の mutex 保護 + 値返し、スロットルの送信ループ実効化、維持カウントの全変更パス整合、configure の明示 opt-in 化(-Iyes/include バグ修正含む)をいずれも確認。
  • F2: LSN 連続性チェックの境界演算にオフバイワンなし。ただしpurged 検出経路のテストが実在しない(下記コメント参照)。

ローカル (nix build .#test-flare-rocksdb) と CI の両方でテストスイートが green であることも確認しました。


ただし、再設計後のコードに 新規 critical が 3 件あります(いずれもマージ前に対応判断を推奨)

  • C1: WAL 適用が宛先クラスタのルーティング/クラスタ内スレーブ複製をバイパス → 宛先が複数パーティション or 複数レプリカだとデータが正しく行き渡らないまま「成功」報告。
  • C2: PR #140 で指摘した orphan purge の prepare 状態全消しが未修正のまま
  • C3: master_id トークンが物理 DB のシーケンスドメインを識別できず、再構築でのトークン継承後に増分同期がサイレントにデータ欠落を起こす。

それぞれ該当行にインラインで詳細を書きました。major(ライブネス系のデッドロック/無限ループ、seed の非アトミック性、CI リトライのスコープ、テストギャップ)も併せてコメントしています。

優先度: C2 は小さな状態チェックの追加で防げる被害最大級のバグなので最優先。C1/C3 は設計判断が要るので、当面は「WAL 同期は宛先 1 ノード/1 パーティション構成限定」を明記し、検出時はフルダンプへ強制フォールバックする形が現実的だと思います。

int partition = self.node_partition;
int partition_size = this->_cluster->get_node_partition_map_size();

if (partition < 0) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C2 / critical] orphan purge が prepare 状態のノードで全データを消去しうる(PR #140 指摘の未修正持ち越し)

ガードが partition < 0 のみです。新パーティションのマスターが state_prepare/state_ready のとき node_partition = N ≥ 0 ですが、そのパーティションは _node_partition_prepare_map 側に存在し、get_node_partition_map_size()(:99)は active マップしか数えないため partition_size = N になります。すると resolve(h, N) ∈ [0, N-1] ≠ N全キーで成立し、100% が orphan 判定 → 再構築中のデータセット全体が purge されます。prepare は数時間続く定常状態で node_map_version も不変なため token も失効しません。

修正方向: purge/scan の実行前に node_state == state_active かつ partition が active マップに存在することを確認して拒否する。

Comment thread src/lib/op_orphan_scan.cc
partition, partition_size, (unsigned long long)nmv_start, rdb->get_master_id().c_str());

if (partition < 0) {
// Node is not currently assigned to a partition (proxy,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C2 関連] このコメントは「proxy/down/prepare の場合はここで弾く」と主張していますが、prepare 状態のノードは partition >= 0(prepare マップ側)なのでこの partition < 0 分岐には入りません。結果として prepare 中のノードで scan が全キーを orphan と報告し、有効な token を発行してしまいます(purge 側 C2 参照)。状態(state_active)チェックの追加が必要です。


if (fail_reason.empty()) {
rocksdb::WriteBatch batch(string(batch_data, batch_size));
if (rocksdb->apply_batch_with_lsn(batch, seq) < 0) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C1 / critical] WAL 適用が宛先クラスタのルーティングとスレーブ複製をバイパス

フルダンプ経路は op_set 経由なので宛先側で pre_proxy_write(所有ノードへのルーティング)と post_proxy_write(宛先ノード自身のスレーブへの複製)が働きます。一方この WAL 経路は受信エントリノードのローカル RocksDB に _apply_batch_filtered で直接適用するため、どちらも通りません。

  • 宛先クラスタが >1 パーティション、またはエントリノードが所有者でない場合、キーが間違ったノードの storage に入り読めなくなります(ダンプ経路ならルーティングされていた)。
  • 1:1 構成でも、宛先ノードのスレーブに増分が伝わらずサイレントに乖離し、宛先側フェイルオーバーで増分が全損します。

いずれのケースも resync は成功を報告しストリークをリセットします。ROCKSDB_REPLICATION.md にも宛先トポロジの制約が明記されていません。当面は「WAL 同期は宛先 1 ノード/1 パーティション限定」を制約として明記し、それ以外は検出時にフルダンプへ強制フォールバックするのが安全だと思います。

Comment thread src/lib/handler_reconstruction.cc Outdated
// master_id_mismatch on every WAL attempt and burn cycles on
// redundant full dumps.
if (this->_storage->get_type() == storage::type_rocksdb) {
storage_rocksdb* rdb = dynamic_cast<storage_rocksdb*>(this->_storage);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[C3 / critical] master_id トークンが WAL シーケンスドメインを識別できない

ここでピアの master_id別の物理 DB(独自のシーケンスカウンタを持つ)に採用しますが、対応するシーケンス位置は採用しません。下流(op_repl_sync_wal / handler_dump_replication)は master_id 一致を「宛先が記録した LSN は自分の WAL 上の位置」と解釈します。

具体シナリオ: パーティション分割で新マスター M1 が M0 から再構築して M0 のトークンを継承 → M0.id == M1.id。両者が mode=duplicate で同じ宛先に push。M1 が先にダンプ+seed(M1 ドメインの lsn)。M0 の次回 resync は id 一致を理由に M1 ドメインの lsn を「自分の位置」として信用し、その番号以降の M0-WAL エントリだけをストリーム → M0 はフルダンプせず、宛先は M0 のパーティションの大半を欠いたまま成功報告lsn_ahead チェックは幸運な方向しか捕捉しません。ドキュメント記載のスレーブ昇格ケースでも同じドメイン混同が起きます。

修正方向: トークンは「血統」ではなく「物理 DB = シーケンスドメイン」を識別する必要があります。継承時に位置も併せて意味づけるか、DB ごとの epoch を導入し、ドメインが変わったら必ずフルダンプにフォールバックする。

uint64_t from_lsn = dest_lsn;
uint64_t streamed = 0;
bool has_more = true;
while (has_more) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[N1 / major] ストリーミングループが graceful shutdown で中断できず、追いつきループが無限化しうる

この送信ループ(_stream_batches)と受信側 _receive_batches(:405 の for(;;))は is_shutdown_request をチェックしません(op_dump.cc は毎キー確認、本ハンドラの Phase 3 も毎キー確認しています)。

  • 受信側: bwlimit で絞られた数 GB のストリームが request スレッドを数時間 _receive_batches に留め、データが流れ続けるため 10 分の read timeout も発火せず、宛先ノードの graceful shutdown が完了しない。
  • 送信側: 書き込みレートが絞られた送信レートを持続的に上回ると、毎回の 64MiB フェッチが has_more=true で終わり while(has_more) が終わらない → resync が永遠に完了せず結果も返さず、ハンドラスレッドはソケットが死ぬまで shutdown を無視。

総バイト/総時間の上限もありません。各イテレーションで shutdown フラグを見る + 収束しない追いつきは N チャンクでフルダンプへ切る、を推奨します。

Comment thread src/lib/op_repl_sync_wal.cc Outdated
if (rocksdb->set_master_id(this->_client_master_id) < 0) {
return this->_send_result(result_server_error, "seed_failed");
}
if (rocksdb->set_repl_last_lsn(this->_seed_lsn) < 0) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[M1 / major] seed が master_id と repl_last_lsn を非アトミックに永続化

set_master_id(:385)→ set_repl_last_lsn(:388)が別々の同期 Put です。1 つ目のコミット後に宛先がクラッシュ、または 2 つ目が失敗して seed_failed を返すと、(新 master_id, 古い lsn)が残ります。その古い lsn が別 lineage の残骸で source の最新シーケンス以下だと、次回同期でギャップをサイレントスキップします(C3 と複合)。

修正方向: 順序を lsn→id に入れ替えるか、両者を 1 つの WriteBatch で書く。バッチ適用側では既にこれを不変条件 S5 として実装済み(:1209 付近)なので、それに揃えるのが自然です。

Comment thread src/lib/handler_dump_replication.cc Outdated
if (c->open() < 0) {
log_err("failed to connect to cluster replication server (name=%s, port=%d)",
this->_replication_server_name.c_str(), this->_replication_server_port);
this->_notify_resync_result(false);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[M5 / major→minor] 接続失敗が source の self-demote カウント対象になっています。このハンドラは健全で権威のある source マスター上で動くため、宛先クラスタが停止/到達不能な状態 + SIGHUP による cluster_replication 設定リロードが数回起きると(ストリークはインメモリで生き残る)、健全な source が threshold 3 で自分を state_down にしてしまいます。ROCKSDB_REPLICATION.md の意図表は op_set/イテレータエラーのみを失敗として挙げており接続失敗は含みません。接続失敗は計上しない、またはリロードとの相互作用を明記することを検討してください。

if ((b & behavior_skip_timestamp) == 0 && e.expire > 0 && e.expire <= stats_object->get_timestamp()) {
log_debug("entry expired (key=%s, expire=%ld, timestamp=%ld)",
e.key.c_str(), e.expire, stats_object->get_timestamp());
r = result_not_found;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F8 残 / minor] get() で期限切れレコードを検出しても物理削除しません(r = result_not_found を返すだけ)。tcb の get はアンロック後に remove します(storage_tcb.cc 参照)し、compaction filter も設定されていないため、TTL 主体のワークロードでは期限切れレコードが上書き/明示削除されるまで残存し、ディスクと curr_items が単調増加します。add/replace の意味論自体は正しく出る(set 側で st_gone 扱い)ので破損ではなくドリフトですが、tcb との挙動差として記録しておきます。

// verify the two reachable conditions: contiguous fetch succeeds, and
// a request beyond the latest sequence reports "up to date", never a
// bogus batch range.
void test_wal_get_updates_since_boundaries() {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F2 テストギャップ / major] このテストは自身のコメントで「purged-gap 条件は live WAL では再現できない」と認めており、実際には contiguous-from-0 と beyond-latest しか検証していません。ところがチェックリスト(ROCKSDB_REVIEW.md)ではこのテストを根拠に F2 の purged 検出を ✓ としています。F2 はサイレントなデータ欠落だった S 級修正なので、その回帰テストが無いのは最大の乖離です。小さい wal_ttl/wal_size_limit で開き、書き込み後に Flush + WAL ファイルを削除してから LSN 0 で要求する、等で ERR_LSN_PURGED 経路を実際に踏めます。

# The repartition test in flare-tests is statistical (random key
# distribution, fails when deviation >= 0.3), so retry up to 3 times.
run: |
for i in 1 2 3; do

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CI / major] この 3 回リトライは nix build 全体(flare ビルド + cutter 全ユニットテスト + flare-tests 統合テスト)を包んでいます。flaky なのは統計的な repartition テストだけのはずですが、この構造だと新規 WAL テストを含む全テストが 3 回チャンスをもらいます。30% の確率で落ちる新規レースが約 97% で CI を通過してしまいます。リトライは flare-tests 内の repartition ケースだけにスコープする(または unit テストと flare-tests を別ステップに分けてリトライは後者のみにする)ことを推奨します。同じ問題が :53 の rocksdb 側にもあります。

Fixes found reviewing the push-protocol redesign (138286a). See
ROCKSDB_REVIEW.md section 4b for the full write-up.

Critical:
- C1: WAL apply bypasses the destination cluster's key routing and slave
  fan-out, so it is only correct for a single active partition with no
  slave. The destination now enforces this
  (cluster::is_wal_sync_destination_safe(), checked in begin/seed) and
  replies topology_unsupported otherwise; the source falls back to a
  routed full dump. op_repl_sync_wal now receives cluster*.
- C2: orphan scan/purge guarded only on partition<0, so a node in
  state_prepare/ready (partition in the PREPARE map, absent from the
  active map) judged every key an orphan and could wipe the whole
  dataset mid-reconstruction. Both ops now additionally require
  state_active and partition < partition_size.
- C3: the master_id token identified a lineage but not the physical DB
  (WAL sequence domain); a reconstructed master inheriting a peer's
  token could trust the peer's LSN as a position in its own WAL and
  skip updates. Introduce a separate immutable per-DB master_id and a
  repl_source_id marker recording which source repl_last_lsn belongs to;
  begin matches source_id against repl_source_id, and reconstruction no
  longer adopts the peer's token.

Major:
- M1: seed wrote master_id and repl_last_lsn as two Puts; a crash
  between them left a new source with a stale position. set_repl_source()
  now writes both in one atomic WriteBatch.
- N1: neither stream side checked is_shutdown_request and a persistently-
  behind catch-up looped forever. Both loops now check shutdown each
  iteration; the source caps catch-up at max_fetch_iterations and falls
  back to a full dump.
- N1': iter_begin held the wholelock rdlock until iter_end while the
  same thread's get()/remove() re-acquired it and truncate()/apply now
  take it as a writer — a deadlock/stall. Iteration no longer holds the
  wholelock at all (the RocksDB snapshot already gives a consistent
  view); _mutex_iter_lock still serializes iterations.
- M5: connect/reconnect failures no longer count toward the source's
  self-demote streak (only real dump failures do).

Minor / tests:
- F8 residual: get() now physically removes an expired record after
  releasing its read locks (version-guarded), matching storage_tcb.
- F2 test gap: add test_wal_get_updates_since_purged_detected, which
  flushes + deletes archived WAL files (flush_and_purge_wal_for_test)
  and asserts get_updates_since returns ERR_LSN_PURGED.
- CI: split the workflow so unit tests run without retry and only the
  statistical flare-tests integration suite is retried.
- Rewrote test_op_repl_sync_wal.cc for the source_id / topology model
  (mock_cluster); 20 wire-protocol tests pass. Updated
  ROCKSDB_REPLICATION.md (topology constraint, source vs master id).

@junjihashimoto junjihashimoto left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3rd review (8253147) — second-review 指摘への対応を検証しました

Address second-review findings の各修正を、ROCKSDB_REVIEW.md の記述ではなく実コードで追試しました。結論から言うと、指摘した項目はほぼ全て正しく修正されています。特に critical 3件はいずれも設計として妥当な直し方で、コードにも反映済みでした。素晴らしい対応です。

critical — すべて VERIFIED-FIXED

  • C1(WAL適用が宛先ルーティング/スレーブ複製をバイパス): cluster::is_wal_sync_destination_safe() を新設し、begin/seed 両方で「active partition が partition 0 の1個のみ・スレーブ0・prepare partition 0」を検査。不適合なら topology_unsupported を返し、ソース側は client_not_supported に分類して正しくルーティングされる full dump にフォールバック。「1パーティションと報告するが実はスレーブがいる」バイパスも塞がれています。_receive_batchesapply_batch_with_lsn はこのガードを通る _run_server_begin 以外から到達不可なことも確認しました。
  • C2(orphan purge の prepare 状態全消し): state_active 必須 かつ partition < partition_size の二重ガードに。prepare/ready 状態のノードは両条件で拒否され、token は発行時・実行時の両方で再評価。具体シナリオ(partition 2 のマスターが prepare、active map size 2)で両条件とも拒否になることを確認。
  • C3(master_id のシーケンスドメイン混同): 再構築時の peer-master_id 採用を削除し、repl_source_id マーカーで「宛先の repl_last_lsn がどのソースのドメインか」を明示追跡。パーティション分割で M0/M1 が別 UUID になり、begin は宛先自身の master_id ではなく記録済み repl_source_id と照合するため、クロスドメイン LSN を二度と信用しません。

major/minor — すべて VERIFIED-FIXED

  • N1: 両ストリーミングループが毎イテレーションで shutdown 確認(受信側は mid-record で壊さず drain して枠同期維持)、追いつきは 64 イテレーション上限 + 64MiB/fetch で有界化。
  • N1': iter_begin/next/end が wholelock を一切取らなくなり(スナップショットで整合性確保)、新設 writer(truncate/apply_batch の wrlock)との再帰 rdlock デッドロック連鎖が消滅。
  • M1: set_repl_source() が source_id + lsn を単一 WriteBatch(sync=true)で書き、非アトミック窓を解消。
  • M5: 接続/再接続失敗は self-demote に計上せず、実ダンプ失敗のみカウント。意図もコメント化。
  • F8残: get() が tcb 同様に期限切れレコードを delete-on-read(version_equal ガード付きで並行上書きに安全)。
  • CI: ユニットテストはリトライ無しの別ステップになり、3回リトライは統計的 repartition 統合テストのみに限定。

新規バグの混入も特に警戒して見ました(F8 の skip_lock レース / shutdown による stream desync / 予約キーの WriteBatch 混入)が、いずれも正しくハンドリングされており新規バグなしです。

唯一の残提案(ブロッカーではありません)

F2 のテスト(test_wal_get_updates_since_purged_detected)だけ PARTIALLY-FIXED です。本番の連続性チェック(get_updates_since の3点の purge 検出)は正しく実装済みで、テストも実際に flush + WAL ファイル削除して呼ぶ本物の purgeになっています。ただしアサーションが ERR_LSN_PURGEDrc==0(contiguous)の両方を許容するため、環境が WAL を保持したままだと purge 検出分岐を踏まずに通ります。詳細は該当行のインラインコメントに書きました。


総評: 私の再レビュー指摘は critical 含めほぼ全項目が正しく対応されており、マージ可能な品質に達していると判断します。F2 テストのハードアサーション化だけ、余裕があれば対応を推奨します。

Comment thread test/lib/test_storage_rocksdb.cc Outdated

// Two acceptable outcomes, both correct; a truncated stream (rc==0
// with a first batch whose sequence > 1) is the silent-gap bug and
// must NOT happen.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F2 / PARTIALLY-FIXED] このテストは実際に flush + WAL ファイル削除して purge を起こしており、以前の contiguous/beyond-latest しか見なかった版から大きく改善しています。ただしアサーションが ERR_LSN_PURGED(purge 検出)と rc==0(contiguous)の両方を許容する条件分岐なので、テスト環境が flush_and_purge_wal_for_test() 後も sequence 0 を WAL に保持していると、purge 検出分岐(updates.empty() を確認する側)を一度も踏まずに else 側で通ってしまいます。F2 はサイレントなデータ欠落だった S 級修正なので、その回帰テストが「purge しても検出しない」状態でも緑になり得るのは惜しいです。

提案: flush_and_purge_wal_for_test() が実際に early sequence を消したことを前提に、rc == ERR_LSN_PURGEDハードアサーションにする(purge 済みなら必ず検出、を保証)。もし環境依存で WAL 保持がありうるなら、purge 後に GetLatestSequenceNumber や WAL ファイルの実在を確認してから分岐させ、「purge が起きたなら必ず ERR_LSN_PURGED」という条件付きハードアサーションにすると、検出経路が毎回確実に実行されます。本番ロジックは正しいので、これはテストの厳密性だけの話です。

The test allowed both ERR_LSN_PURGED and a contiguous stream, so if the
environment retained the WAL it passed without ever exercising the
purge-detection branch — the exact regression F2 guards against could
have gone green.

flush_and_purge_wal_for_test() now probes GetUpdatesSince(1) after the
flush + WAL-file delete and returns whether a purge was actually
confirmed (early sequences no longer retrievable). When it was, the
test hard-asserts ERR_LSN_PURGED; only if the WAL was genuinely retained
does it fall back to the contiguous-stream check. Production continuity
logic is unchanged — this is test strictness only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant