feat(persistence): K2 unified per-shard floor register + min-across-planes WAL recycle (kernel M3 stage 2)#299
Merged
Conversation
…es WAL recycle (kernel M3 stage 2 / K2) Extend ShardControlFile with graph_floor_lsn/ws_floor_lsn/mq_floor_lsn (payload 57->81 bytes, backward-compatible via payload_bytes-gated parsing). Both WAL-recycle call sites -- checkpoint Finalize and autovacuum Pass C -- now recycle up to min(kv_floor, graph_floor) instead of the KV-only floor, closing the window where a segment holding the only copy of an unflushed graph record could be recycled. WS/MQ floors are tracked but deliberately excluded from the min (brief Risk #2): both planes have no snapshot format in any mode, so folding their sentinel 0 floor in would permanently freeze recycling on any shard that ever saw a WS/MQ record. Their content stays protected by the existing, orthogonal segment_plane_scan content-scan (renamed from segment_holds_plane_history; GraphTemporal removed from its blocking arm now that the LSN floor covers that record type). Root-caused and fixed task #53 in the same stage, per this stage's mandate to fix in-scope root causes tied to the Finalize floor/durability-ordering invariant K2 formalizes. The checkpoint-Finalize-window graph-total-loss finding was exactly that invariant violated: save_graph_store wrote the reference/floor (graph_metadata.json, via GraphStore::save_metadata) BEFORE the payload it claims durable (CSR segments + manifest.json) -- an ARIES-inverted write order. A kill-9 between the two writes left a fully-advanced floor pointing at a payload that never reached disk, so recovery trusted the floor and skipped WAL replay for records it claimed were already covered, total-losing the graph batch. Fixed by reordering save_graph_store to write CSR segments + manifest.json first, store.save_metadata last, and making save_metadata itself atomic (temp+fsync+rename+dir-fsync via the shared atomic_write_durable helper). Safe against double-replay: graph::replay::node_present already checks both write_buf and loaded CSR segments before re-inserting a WAL-logged node/edge, so replaying a record whose payload made it to disk before the kill is a no-op. Confirmed via MOON_CRASH_MATRIX_RED=1 MOON_CRASH_MATRIX_ITERS=20 soak: 20/20 clean at both prod_s1 and prod_s4 (pre-fix baseline: hit at iteration 11/20 and 7/20 respectively). The two mid_checkpoint crash cells now run ungated; crash-matrix default-GREEN count moves 29/40 -> 31/40. The 9 remaining RED cells (task #52's cross-store TXN graph leg, legacy-mode graph reconstruction, MQ generic-DEL resurrection) are unrelated pre-existing findings, confirmed still correctly RED and unaffected by this change via a full-matrix MOON_CRASH_MATRIX_RED=1 rerun. New unit tests: control-file round-trip + backward-compat (old-format read defaults new floors to 0), an inverted GraphTemporal recycle test (segment recycles once the graph floor covers it even though the plane-scan no longer blocks it), and a Risk #2 regression test (a pure KV write after a WS/MQ record on the same shard still recycles). No per-write hot path touched -- ShardControlFile is only written at checkpoint Finalize and read at Pass C/recovery, both off the command dispatch path. VM hot-path A/B bench judged unnecessary and not run. Gates: 4230/4230 lib tests, fmt clean, both clippy matrices clean (default + tokio/jemalloc), both-runtime compile/test parity, VM G1 default-green run (40/40), VM G4/G5 graph crash suites green, VM full RED-cell matrix (31 passed/9 failed, exactly the expected pre-existing RED cells). Found but explicitly out of scope (not fixed): a pre-existing, unrelated test (g1_mutable_tier_survives_kill9_with_stable_handles in tests/crash_recovery_graph_durability.rs) references a single shard-0.wal file that no longer exists post-WAL-v3 migration (shards now persist to a shard-0/ directory); this test rot predates this change and is orthogonal to it. author: Tin Dang
…on + VACUUM floor bypass (kernel M3 stage 2) An adversarial review of the K2 unified floor-register work (commit 23f28e3) found two P0s and a P1 in the same area before the stage could close. P0 (src/graph/recovery.rs): persist_graph_at_checkpoint's short-circuit `!is_dirty() || graph_count() == 0` skipped save_graph_store whenever a GRAPH.DELETE emptied the graph map, even though the delete itself marks the store dirty via the WAL drain. graph_metadata.json stayed stale forever while later checkpoints kept advancing control.graph_floor_lsn past the WAL record holding the DELETE; once that segment was recycled, a crash+restart loaded the stale pre-delete metadata with nothing left in the WAL to replay the deletion, resurrecting a dropped graph. Fixed by dropping the `graph_count() == 0` disjunct — dirty alone gates the save now; save_graph_store's per-graph loop correctly no-ops on zero graphs while save_metadata still durably rewrites the (now-empty) graph list. Regression cells added: cross_plane_prod_{s1,s4}_graph_drop_survives_repeated_checkpoints (tests/crash_matrix_cross_plane/scenarios.rs, tests_prod.rs, planes.rs), RED-first verified against the pre-fix binary. P0 (src/command/server_admin.rs): manual VACUUM's WAL-recycle pass (run_vacuum_passes) recycled to `wal.current_lsn()` unconditionally — a client-reachable path around the min(kv_floor, graph_floor) invariant K2 established for the checkpoint and autovacuum recycle sites. Fixed by threading disk_offload_dir/shard_id into vacuum()/run_vacuum_passes: checkpoint-backed mode now recycles to min(last_checkpoint_lsn, graph_floor_lsn) read from the shard's ShardControlFile (mirroring autovacuum Pass C); legacy mode (no disk-offload dir) now refuses to recycle WAL at all and increments the shared RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL counter, the same skip-and-warn shape Pass C already uses. All three direct-dispatch callers (handler_single, handler_sharded, handler_monoio) pass None/0 (dead — wal is always None on those paths too); the SPSC path (spsc_handler.rs) passes the real disk_offload_dir/shard_id. P1 (src/shard/persistence_tick.rs): the WAL-overflow emergency recycle path (maybe_force_checkpoint_on_wal_overflow) used last_checkpoint_lsn alone; now .min(control.graph_floor_lsn), closing the same gap in the emergency code path. P2s: GraphManifest::save (src/graph/manifest.rs) now routes through the shared atomic_write_durable helper instead of a hand-rolled temp+fsync+rename+dir-fsync sequence (behavior-equivalent); a stale doc comment on scenarios::mixed_mid_checkpoint describing the Finalize-ordering bug as still-unfixed was corrected to document the fix and its soak-test confirmation. Crash-matrix suite: 42 cells total, 33 GREEN by default (was 40/31). New unit tests in src/command/server_admin.rs cover legacy-mode refusal + blocked counter, disk-offload-mode recycle-to-control-file-floor, and the no-control-file-yet conservative fallback. Gates run on this machine (macOS, default toolchain): cargo fmt --check clean; cargo clippy -- -D warnings clean (default features); cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings clean; cargo test --lib 4233/4233 passed; release build + targeted crash_matrix_cross_plane run confirms both new graph_drop_survives_repeated_checkpoints cells GREEN and no regression on mixed_all_planes_mid_checkpoint / kv_isolated / vector_isolated at s1 and s4. VM/Linux gates (io_uring, cross-arch bench) left to the orchestrator per this worktree's scope. author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR adds per-plane control-file floors, makes graph metadata persistence follow payload durability, applies checkpoint-aware graph floors to WAL recycling and VACUUM, adds GraphTemporal reclamation metrics, preserves floors during recovery, and expands crash-matrix regression coverage. It also enforces AOF barrier-before-response ordering for always-fsync writes. ChangesStorage and recovery invariants
AOF acknowledgement ordering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionHandler
participant AofWriterPool
participant Storage
Client->>ConnectionHandler: Submit write commands
ConnectionHandler->>AofWriterPool: Enqueue AOF entries
ConnectionHandler->>AofWriterPool: Await fsync barrier
AofWriterPool->>Storage: Durably persist batch
Storage-->>AofWriterPool: Barrier acknowledged
AofWriterPool-->>ConnectionHandler: Durability confirmed
ConnectionHandler-->>Client: Send write responses
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Storage-kernel M3 stage 2 (K2): planes now register real durable floors, and every WAL recycler advances to the MIN across planes — the structural replacement for the stopgap fail-closed plane guard from #291.
ShardControlFilegains per-plane floors (graph_floor_lsn/ws_floor_lsn/mq_floor_lsn); checkpoint-Finalize and autovacuum Pass C recycle tomin(kv_floor, plane floors)instead of blanket-blocking segments that hold plane records. A plane with a registered floor advances recycle; a plane without one keeps the feat(mq): MQ effect records — kill-9-durable streams/PEL/DLQ/triggers + WAL recycle plane guard (Wave B stage 2a) #291 fail-closed block.persist_graph_at_checkpointshort-circuit dropped a disjunct — deleted graphs reappeared after repeated checkpoints), threaded the floor invariant through manualVACUUMand the SPSC/WAL-overflow emergency recycle path, and routedGraphManifest::savethrough the sharedatomic_write_durablehelper (K3 contract).cross_plane_prod_{s1,s4}_graph_drop_survives_repeated_checkpoints); suite 40→42 cells.Verification
-D warnings, fmt.Context
Continues the v0.8 'One Storage Kernel: kill-9-lossless on every plane' goal — M3 stage 1 was the G1 crash matrix (#298); this is the K2 contract from the storage-architecture synthesis (unify contracts, keep engines).
Summary by CodeRabbit
Bug Fixes
VACUUM, reducing unsafe reclamation.Monitoring