diff --git a/CHANGELOG.md b/CHANGELOG.md index bd476080c..6e7e5bcb5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,136 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added — unified per-shard floor register + min-across-planes WAL recycle (kernel M3 stage 2 / K2) + +`ShardControlFile` (`src/persistence/control.rs`) gains `graph_floor_lsn`, +`ws_floor_lsn`, `mq_floor_lsn: u64` fields (control payload 57→81 bytes, +backward-compatible: fields default to `0` when reading a control file +written by an older binary, keyed off the on-disk `payload_bytes` header). +Both WAL-recycle call sites — checkpoint `Finalize` +(`src/shard/persistence_tick.rs`) and autovacuum Pass C +(`src/shard/autovacuum.rs`, wired via a new `control_file` parameter on +`run_tick`) — now recycle up to `min(kv_floor, graph_floor)` instead of the +KV-only floor, so a WAL segment is never recycled while it still holds the +only copy of an unflushed graph record. WS/MQ floors are tracked but +deliberately **excluded** from the `min()` (brief's Risk #2): both planes +have no snapshot format in any mode, so folding their sentinel `0` floor +into the minimum 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 +match arm now that the LSN floor covers that record type, with a new +`RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL` counter in the `# Reclamation` +INFO section marking segments it stops blocking). + +**Task #53 root-caused and fixed in the same stage** (required by this +stage's mandate: fix in-scope if the root cause is the Finalize +floor/durability-ordering invariant K2 formalizes). The +checkpoint-`Finalize`-window graph-total-loss finding +(`tests/crash_matrix_cross_plane.rs`'s former RED cell 4) was exactly that +invariant: `save_graph_store` (`src/graph/recovery.rs`) 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 +`persistence::atomic::atomic_write_durable` helper) so the floor write can +never itself be torn. Safe against double-replay: +`graph::replay::node_present` checks both `write_buf` and loaded CSR +segments before re-inserting a WAL-logged node/edge, so replaying a record +whose payload actually made it to disk before the kill is a no-op, not a +duplicate. 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). `cross_plane_prod_s1_mixed_all_planes_mid_checkpoint` / +`cross_plane_prod_s4_mixed_all_planes_mid_checkpoint` now run ungated — +the crash-matrix suite's default-GREEN count moves from 29/40 to 31/40 (9 +RED cells remain: task #52's cross-store TXN graph leg and the legacy-mode +graph-reconstruction/MQ-resurrection findings above, all still tracked +separately and explicitly out of scope for this stage). A subsequent +adversarial review round found a second graph-durability P0 plus a VACUUM +floor bypass on top of this fix — see the next section — which add 2 more +GREEN cells, moving the suite to 42 cells total (33 GREEN by default). + +`src/persistence/recovery.rs`'s PITR path threads the three new floors +through unchanged on replay. 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 — the WS/MQ sentinel floor must never collapse recycling to +zero). + +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; a VM hot-path A/B bench was judged unnecessary for this +change and not run. + +### Fixed — K2 adversarial review round: graph drop-resurrection + VACUUM floor bypass (kernel M3 stage 2) + +An adversarial review of the floor-register work above (SHIP-WITH-FIXES +verdict) found two P0s and a P1 in the same area before the stage could +close: + +**P0 — `GRAPH.DELETE`'d graphs could resurrect across repeated +checkpoints.** `persist_graph_at_checkpoint`'s short-circuit +(`!store.is_dirty() || store.graph_count() == 0 -> return true`) skipped +`save_graph_store` whenever a delete emptied the graph map — even though +the delete itself marks the store dirty via the WAL drain. `graph_count() +== 0` and "nothing to persist" are independent conditions: an +empty-but-dirty store still needs `graph_metadata.json` rewritten to +reflect zero graphs at the new `snapshot_lsn`. With the skip in place, +metadata stayed stale forever (dirty never clears without a real save) +while every later checkpoint kept advancing `control.graph_floor_lsn` past +the WAL record holding the `DELETE` — once that segment was recycled, a +crash+restart loaded the stale metadata with nothing left in the WAL to +replay the deletion. Fixed by dropping `graph_count() == 0` from the +short-circuit; dirty alone gates it now, and `save_graph_store`'s per-graph +loop correctly no-ops on zero graphs while `store.save_metadata` still +durably rewrites the (now-empty) graph list. New regression cells +`cross_plane_prod_s1_graph_drop_survives_repeated_checkpoints` / +`_s4_...` (`tests/crash_matrix_cross_plane/scenarios.rs`), RED-first +verified against the pre-fix binary at both shard counts (two earlier +padding designs — a live second graph, then MQ pushes — were each proven +NOT to reproduce the bug before a third, using throwaway create+delete +graph cycles as padding, did). + +**P0 — manual `VACUUM` bypassed the unified floor register.** +`run_vacuum_passes` (`src/command/server_admin.rs`) recycled WAL to +`w.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 the control-file floors +into `VACUUM`: in checkpoint-backed (disk-offload) mode it now recycles to +`min(last_checkpoint_lsn, graph_floor_lsn)` read from the shard's +`ShardControlFile`, mirroring autovacuum Pass C; in legacy mode (no +disk-offload dir — no snapshot format to bound WS/MQ recycling against) it +now REFUSES to recycle WAL at all and increments the shared +`RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL` counter (surfaced in +`INFO`/`DEBUG RECLAMATION`'s `# Reclamation` section), the same +skip-and-warn shape Pass C already uses for the identical precondition. + +**P1 — the WAL-overflow emergency recycle path** (`src/shard/ +persistence_tick.rs`) used `last_checkpoint_lsn` alone; now +`.min(control.graph_floor_lsn)`, closing the same gap in the emergency +code path. + +Plus two 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); and a stale +doc comment on `scenarios::mixed_mid_checkpoint` that still described the +Finalize-ordering bug as unfixed was corrected. + +Re-gated after these fixes: the crash-matrix suite (42 cells, 33 GREEN by +default, including the 2 new regression cells), `mixed_mid_checkpoint` +`MOON_CRASH_MATRIX_RED=1 MOON_CRASH_MATRIX_ITERS=20` soak at both shard +counts (re-verified since the P0 fix touches the same `Finalize` path), +`g4`/`g5` graph durability cells, `cargo fmt --check`, and both clippy +matrices (default features; `runtime-tokio,jemalloc`). + ### Added — cross-plane kill-9 crash-matrix suite (kernel M3 stage 1 / G1) New `tests/crash_matrix_cross_plane.rs` + `tests/crash_matrix_cross_plane/` diff --git a/src/command/info_reclamation.rs b/src/command/info_reclamation.rs index de2118f7a..65ea2b8b4 100644 --- a/src/command/info_reclamation.rs +++ b/src/command/info_reclamation.rs @@ -144,6 +144,18 @@ pub static RECL_AUTOVACUUM_THROTTLED_DUE_TO_LOAD: AtomicU64 = AtomicU64::new(0); /// configured ceiling; it never means data was lost. pub static RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL: AtomicU64 = AtomicU64::new(0); +/// Cumulative count of sealed WAL segments recycled that contained at least +/// one `GraphTemporal` (`TEMPORAL.INVALIDATE`) record — kernel M3 K2's +/// complement counter to [`RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL`], +/// making the `segment_holds_plane_history` guard shrink (`GraphTemporal` +/// moved out of the WS/MQ "no floor ever" bucket once `graph_floor_lsn` is +/// an explicit, checked value) observable to operators. Non-zero means the +/// guard shrink is actively reclaiming disk that pre-K2 builds would have +/// kept pinned forever; it never means data was lost — `graph_floor_lsn` +/// only lets these segments through once the graph snapshot durably covers +/// them. +pub static RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL: AtomicU64 = AtomicU64::new(0); + /// Cumulative count of plane WAL records (MQ/WS/temporal) DROPPED because the /// shard's `wal_append` channel was full at enqueue time (capacity 4096, /// drained every 1ms by the same shard thread — blocking there would deadlock @@ -209,10 +221,12 @@ pub fn write_reclamation_section(buf: &mut String) { "reclamation_wal_bytes:{}\r\n\ reclamation_wal_segments:{}\r\n\ reclamation_wal_recycle_blocked_no_checkpoint_total:{}\r\n\ + reclamation_wal_recycle_graph_temporal_freed_total:{}\r\n\ reclamation_wal_append_channel_dropped_total:{}\r\n", RECL_WAL_BYTES.load(Ordering::Relaxed), RECL_WAL_SEGMENTS.load(Ordering::Relaxed), RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL.load(Ordering::Relaxed), + RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL.load(Ordering::Relaxed), RECL_WAL_APPEND_CHANNEL_DROPPED_TOTAL.load(Ordering::Relaxed) ); @@ -424,6 +438,7 @@ mod tests { "reclamation_plan_cache_evictions_total:", "reclamation_delete_pending_visible_lsn:", "reclamation_wal_append_channel_dropped_total:", + "reclamation_wal_recycle_graph_temporal_freed_total:", ]; for field in required_fields { diff --git a/src/command/server_admin.rs b/src/command/server_admin.rs index ad06db777..fa43f392e 100644 --- a/src/command/server_admin.rs +++ b/src/command/server_admin.rs @@ -932,12 +932,18 @@ impl VacuumCounts { /// - `freeze`: when `true`, calls `mark_old_snapshots_killed` with /// `threshold = Duration::ZERO` (kills ALL non-system snapshots). /// - `mvcc_prune_margin`: `oldest_snapshot - margin` is the GC floor. +/// - `disk_offload_dir` / `shard_id`: kernel M3 K2 review round 2 / P0-2. +/// Used ONLY to locate this shard's `ShardControlFile` for the WAL +/// recycle pass below — see that pass's own doc for why the unified +/// floor register must gate this exactly like autovacuum Pass C. fn run_vacuum_passes( vector_store: &mut crate::vector::store::VectorStore, manifest: Option<&mut ShardManifest>, wal: Option<&mut WalWriterV3>, freeze: bool, mvcc_prune_margin: u64, + disk_offload_dir: Option<&std::path::Path>, + shard_id: usize, ) -> VacuumCounts { let now = Instant::now(); let mut counts = VacuumCounts::default(); @@ -985,17 +991,65 @@ fn run_vacuum_passes( // ── 6. WAL aggressive recycle (P6) ────────────────────────────────────── // Only runs when WAL is configured AND total WAL exceeds max_wal_bytes. + // + // Kernel M3 K2 review round 2 / P0-2: this used to recycle to + // `w.current_lsn()` — the LIVE, uncheckpointed LSN counter — with NO + // disk-offload gate at all. That is not a durable floor: in legacy + // mode it deleted the sole durable copy of live KV/graph/plane data on + // a routine client command (legacy has no checkpoint/snapshot + // protocol whatsoever); in disk-offload mode it deleted + // not-yet-checkpointed pages/graph writes recycle should never touch. + // The "unified floor register" K2 claims to be does not exist while a + // client-reachable command bypasses it — VACUUM must use EXACTLY the + // same recycle floor as autovacuum Pass C + // (`AutovacuumDaemon::run_tick`, `src/shard/autovacuum.rs`): + // `min(control.last_checkpoint_lsn, control.graph_floor_lsn)` in + // checkpoint-backed mode, refused entirely in legacy mode. if let Some(w) = wal { let should_recycle = w .stats() .map(|s| s.total_bytes > w.max_wal_bytes()) .unwrap_or(false); if should_recycle { - let redo_lsn = w.current_lsn(); - match w.recycle_aggressive(redo_lsn) { - Ok(stats) => counts.wal_segments_recycled = stats.segments_recycled as u64, - Err(e) => { - tracing::warn!("VACUUM: WAL recycle_aggressive failed: {e}"); + match disk_offload_dir { + None => { + // Legacy (non-disk-offload) mode: no checkpoint or + // plane-snapshot protocol exists at all, so there is no + // durable floor to recycle against — mirror Pass C's + // skip-and-warn (task #43) exactly, including the same + // shared counter, so operators see WHY a routine + // VACUUM freed 0 WAL segments here (visible via `INFO` + // / `DEBUG RECLAMATION`'s `# Reclamation` section). + crate::command::info_reclamation::RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + tracing::warn!( + "VACUUM: WAL recycle SKIPPED — this shard has no checkpoint \ + floor to recycle against (legacy/non-disk-offload mode); \ + recycling here would risk permanently losing graph/ \ + workspace/MQ/temporal history that has no snapshot outside \ + the WAL. Enable --disk-offload for bounded WAL growth." + ); + } + Some(dir) => { + let shard_dir = dir.join(format!("shard-{shard_id}")); + let ctrl_path = crate::persistence::control::ShardControlFile::control_path( + &shard_dir, shard_id, + ); + // Same-shape fallback as autovacuum's first tick + // (before any checkpoint has ever completed): no + // control file yet -> floor 0, i.e. recycle nothing + // this call. Maximally conservative, self-heals once + // Finalize runs. + let redo_lsn = crate::persistence::control::ShardControlFile::read(&ctrl_path) + .ok() + .map(|c| c.last_checkpoint_lsn.min(c.graph_floor_lsn)) + .unwrap_or(0); + match w.recycle_aggressive(redo_lsn) { + Ok(stats) => counts.wal_segments_recycled = stats.segments_recycled as u64, + Err(e) => { + tracing::warn!("VACUUM: WAL recycle_aggressive failed: {e}"); + } + } } } } @@ -1049,6 +1103,16 @@ fn run_vacuum_passes( /// - Under disk-pause: runs normally (VACUUM reclaims, does not write data). /// - During active checkpoint: WAL recycle is idempotent (P6 `recycle_aggressive` /// is a no-op if no segments are over the threshold). +/// - **Legacy mode (`--disk-offload disable`):** WAL recycle is REFUSED +/// entirely (kernel M3 K2 review round 2 / P0-2) — legacy mode has no +/// checkpoint/plane-snapshot protocol, so there is no durable floor to +/// recycle against; `wal_segments_recycled` stays 0 and +/// `RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL` increments (same +/// counter, same reasoning as autovacuum Pass C's legacy-mode skip). +/// - **Checkpoint-backed mode:** recycles to +/// `min(control.last_checkpoint_lsn, control.graph_floor_lsn)` — the +/// exact same floor Pass C uses — never the live, uncheckpointed +/// `wal.current_lsn()`. /// /// ## FREEZE warning /// `VACUUM (FREEZE)` forcibly kills ALL non-system MVCC snapshots on this shard, @@ -1062,6 +1126,13 @@ pub fn vacuum( wal: Option<&mut WalWriterV3>, args: &[Frame], mvcc_prune_margin: u64, + // Kernel M3 K2 review round 2 / P0-2: `None` on the direct-dispatch + // callers (handler_single/handler_sharded/handler_monoio — `wal` is + // always `None` there too per this fn's own doc, so these are dead on + // that path), `Some(..)`/real shard id on the SPSC/console-gateway + // path where `wal` is real. See `run_vacuum_passes`'s WAL-recycle pass. + disk_offload_dir: Option<&std::path::Path>, + shard_id: usize, ) -> Frame { // Parse subcommand (optional first arg). let sub = args.first().and_then(|f| extract_bytes(f)); @@ -1091,7 +1162,15 @@ pub fn vacuum( b"ERR syntax error: VACUUM (VERBOSE) takes no additional arguments", )); } - let counts = run_vacuum_passes(vector_store, manifest, wal, false, mvcc_prune_margin); + let counts = run_vacuum_passes( + vector_store, + manifest, + wal, + false, + mvcc_prune_margin, + disk_offload_dir, + shard_id, + ); counts.to_verbose_frame() } @@ -1106,7 +1185,15 @@ pub fn vacuum( "VACUUM (FREEZE): forcibly killing ALL active MVCC snapshots on this shard. \ In-flight TXN.BEGIN clients will receive 'snapshot too old' errors." ); - let counts = run_vacuum_passes(vector_store, manifest, wal, true, mvcc_prune_margin); + let counts = run_vacuum_passes( + vector_store, + manifest, + wal, + true, + mvcc_prune_margin, + disk_offload_dir, + shard_id, + ); counts.to_frame() } @@ -1124,7 +1211,15 @@ pub fn vacuum( // ── Plain VACUUM ───────────────────────────────────────────────────── None => { - let counts = run_vacuum_passes(vector_store, manifest, wal, false, mvcc_prune_margin); + let counts = run_vacuum_passes( + vector_store, + manifest, + wal, + false, + mvcc_prune_margin, + disk_offload_dir, + shard_id, + ); counts.to_frame() } @@ -1548,7 +1643,7 @@ mod tests { #[test] fn vacuum_no_persistence_returns_array() { let mut store = crate::vector::store::VectorStore::new(); - let f = vacuum(&mut store, None, None, &[], 1000); + let f = vacuum(&mut store, None, None, &[], 1000, None, 0); match f { Frame::Array(ref arr) => { assert_eq!(arr.len(), 12, "expect 6 key/value pairs = 12 elements"); @@ -1573,7 +1668,7 @@ mod tests { #[test] fn vacuum_files_no_manifest_returns_zero() { let mut store = crate::vector::store::VectorStore::new(); - let f = vacuum(&mut store, None, None, &[bulk(b"FILES")], 1000); + let f = vacuum(&mut store, None, None, &[bulk(b"FILES")], 1000, None, 0); match f { Frame::Array(ref arr) => { assert_eq!(arr.len(), 2); @@ -1592,7 +1687,7 @@ mod tests { #[test] fn vacuum_verbose_includes_diagnostic_lines() { let mut store = crate::vector::store::VectorStore::new(); - let f = vacuum(&mut store, None, None, &[bulk(b"(VERBOSE)")], 1000); + let f = vacuum(&mut store, None, None, &[bulk(b"(VERBOSE)")], 1000, None, 0); match f { Frame::Array(ref arr) => { // Must have at least 6 diagnostic lines + 12 kv pairs @@ -1620,7 +1715,7 @@ mod tests { #[test] fn vacuum_freeze_returns_kv_array() { let mut store = crate::vector::store::VectorStore::new(); - let f = vacuum(&mut store, None, None, &[bulk(b"(FREEZE)")], 1000); + let f = vacuum(&mut store, None, None, &[bulk(b"(FREEZE)")], 1000, None, 0); match f { Frame::Array(ref arr) => { assert_eq!(arr.len(), 12, "FREEZE must return 12-element kv array"); @@ -1639,6 +1734,8 @@ mod tests { None, &[bulk(b"VECTOR"), bulk(b"myidx")], 1000, + None, + 0, ); match f { Frame::SimpleString(ref b) => { @@ -1656,7 +1753,15 @@ mod tests { #[test] fn vacuum_graph_returns_pending() { let mut store = crate::vector::store::VectorStore::new(); - let f = vacuum(&mut store, None, None, &[bulk(b"GRAPH"), bulk(b"g")], 1000); + let f = vacuum( + &mut store, + None, + None, + &[bulk(b"GRAPH"), bulk(b"g")], + 1000, + None, + 0, + ); match f { Frame::SimpleString(ref b) => { assert!( @@ -1673,7 +1778,7 @@ mod tests { #[test] fn vacuum_unknown_subcommand_returns_error() { let mut store = crate::vector::store::VectorStore::new(); - let f = vacuum(&mut store, None, None, &[bulk(b"BOGUS")], 1000); + let f = vacuum(&mut store, None, None, &[bulk(b"BOGUS")], 1000, None, 0); match f { Frame::Error(_) => {} _ => panic!("expected ERR for unknown VACUUM subcommand, got {f:?}"), @@ -1686,7 +1791,7 @@ mod tests { fn vacuum_freeze_kills_active_snapshots() { let mut store = crate::vector::store::VectorStore::new(); let _txn = store.txn_manager_mut().begin(); - let f = vacuum(&mut store, None, None, &[bulk(b"(FREEZE)")], 1000); + let f = vacuum(&mut store, None, None, &[bulk(b"(FREEZE)")], 1000, None, 0); // Extract mvcc_snapshots_killed from returned array. let killed = match &f { Frame::Array(arr) => { @@ -1730,6 +1835,161 @@ mod tests { _ => panic!("expected BulkString from DEBUG RECLAMATION, got {f:?}"), } } + + // ── Kernel M3 K2 review round 2 / P0-2: VACUUM floor unification ─────── + + /// Count `*.wal` files under `wal_dir` (pattern: + /// `shard::autovacuum::tests::count_wal_segments`). + fn count_wal_segments(wal_dir: &std::path::Path) -> usize { + std::fs::read_dir(wal_dir) + .map(|entries| { + entries + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_string_lossy().ends_with(".wal")) + .count() + }) + .unwrap_or(0) + } + + /// Build a `WalWriterV3` with enough sealed (non-active) segments to + /// exceed a tiny `max_wal_bytes` ceiling, so the recycle-eligibility + /// check in `run_vacuum_passes` fires (pattern: + /// `shard::autovacuum::tests::wal_writer_over_ceiling`). + fn wal_writer_over_ceiling( + wal_dir: &std::path::Path, + ) -> crate::persistence::wal_v3::segment::WalWriterV3 { + use crate::persistence::wal_v3::record::WalRecordType; + let mut writer = + crate::persistence::wal_v3::segment::WalWriterV3::new(0, wal_dir, 512).unwrap(); + writer.set_wal_bounds(0, 256); + for i in 0..80 { + writer.append(WalRecordType::Command, b"vacuum-p0-2 EARLY-MARKER"); + if (i + 1) % 3 == 0 { + writer.flush_sync().unwrap(); + } + } + writer.flush_sync().unwrap(); + assert!( + writer.current_segment_sequence() >= 3, + "test setup must produce several sealed segments" + ); + writer + } + + /// P0-2 (drop-resurrection sibling finding): `VACUUM` in legacy + /// (non-disk-offload) mode used to recycle WAL segments against the + /// live, uncheckpointed `wal.current_lsn()` — deleting the sole + /// durable copy of unflushed data on a routine client command. Must + /// now refuse entirely, mirroring autovacuum Pass C's task #43 fix, + /// including the same shared blocked-counter. + #[test] + fn vacuum_legacy_mode_refuses_wal_recycle_and_counts_blocked() { + let tmp = tempfile::tempdir().unwrap(); + let wal_dir = tmp.path().join("wal"); + let mut writer = wal_writer_over_ceiling(&wal_dir); + let before_segments = count_wal_segments(&wal_dir); + let before_blocked = + crate::command::info_reclamation::RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL + .load(std::sync::atomic::Ordering::Relaxed); + + let mut store = crate::vector::store::VectorStore::new(); + // disk_offload_dir: None => legacy mode, no matter what `wal` holds. + let counts = run_vacuum_passes(&mut store, None, Some(&mut writer), false, 1000, None, 0); + + assert_eq!( + counts.wal_segments_recycled, 0, + "legacy mode must recycle 0 segments via VACUUM" + ); + let after_segments = count_wal_segments(&wal_dir); + assert_eq!( + after_segments, before_segments, + "legacy mode must not delete any WAL segment via VACUUM (P0-2)" + ); + let after_blocked = + crate::command::info_reclamation::RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL + .load(std::sync::atomic::Ordering::Relaxed); + assert!( + after_blocked > before_blocked, + "legacy-mode VACUUM skip must be observable via \ + RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL (same counter as Pass C)" + ); + } + + /// P0-2: `VACUUM` in checkpoint-backed (disk-offload) mode must recycle + /// to exactly the same floor as autovacuum Pass C — + /// `min(control.last_checkpoint_lsn, control.graph_floor_lsn)` read + /// from this shard's `ShardControlFile` — never the live + /// `wal.current_lsn()`. With both floors pinned to `u64::MAX` (a + /// checkpoint that has covered everything written), recycling must + /// actually happen. + #[test] + fn vacuum_disk_offload_mode_recycles_to_control_file_floor() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().to_path_buf(); + let wal_dir = dir.join("shard-0").join("wal-v3"); + let mut writer = wal_writer_over_ceiling(&wal_dir); + let before_segments = count_wal_segments(&wal_dir); + + let shard_dir = dir.join("shard-0"); + std::fs::create_dir_all(&shard_dir).unwrap(); + let mut control = crate::persistence::control::ShardControlFile::new([0u8; 16]); + control.last_checkpoint_lsn = u64::MAX; + control.graph_floor_lsn = u64::MAX; + let ctrl_path = crate::persistence::control::ShardControlFile::control_path(&shard_dir, 0); + control.write(&ctrl_path).unwrap(); + + let mut store = crate::vector::store::VectorStore::new(); + let counts = run_vacuum_passes( + &mut store, + None, + Some(&mut writer), + false, + 1000, + Some(&dir), + 0, + ); + + assert!( + counts.wal_segments_recycled > 0, + "disk-offload mode with a control-file floor covering everything \ + must actually recycle via VACUUM (P0-2)" + ); + let after_segments = count_wal_segments(&wal_dir); + assert!( + after_segments < before_segments, + "recycled segments must actually be removed from disk" + ); + } + + /// P0-2 edge case: disk-offload mode but no control file has ever been + /// written yet (very first VACUUM before any checkpoint completes). + /// Must be maximally conservative — floor 0, recycle nothing — not + /// panic or fall back to the unsafe live-LSN behavior. + #[test] + fn vacuum_disk_offload_mode_no_control_file_yet_recycles_nothing() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().to_path_buf(); + let wal_dir = dir.join("shard-0").join("wal-v3"); + let mut writer = wal_writer_over_ceiling(&wal_dir); + let before_segments = count_wal_segments(&wal_dir); + + let mut store = crate::vector::store::VectorStore::new(); + let counts = run_vacuum_passes( + &mut store, + None, + Some(&mut writer), + false, + 1000, + Some(&dir), + 0, + ); + + assert_eq!( + counts.wal_segments_recycled, 0, + "no control file yet => floor 0 => recycle nothing" + ); + assert_eq!(count_wal_segments(&wal_dir), before_segments); + } } // ── VACUUM VECTOR (P2) ────────────────────────────────────────────────── diff --git a/src/graph/manifest.rs b/src/graph/manifest.rs index 43786260c..406ccdf80 100644 --- a/src/graph/manifest.rs +++ b/src/graph/manifest.rs @@ -89,24 +89,17 @@ impl GraphManifest { } /// Write this manifest as pretty-printed JSON to `path` atomically. - /// Uses write-to-temp + fsync + rename to prevent corruption on crash. + /// + /// Kernel M3 K2 review round 2 / P2-2 (behavior-equivalent refactor): + /// routed through the shared `atomic_write_durable` helper + /// (`src/persistence/atomic.rs`, K3) instead of a hand-rolled + /// temp+fsync+rename+dir-fsync sequence — same durability contract, + /// one fewer independent implementation of it to keep in sync. pub fn save(&self, path: &Path) -> io::Result<()> { - use std::io::Write; let json = serde_json::to_string_pretty(self) .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; - let tmp_path = path.with_extension("tmp"); - let mut file = std::fs::File::create(&tmp_path)?; - file.write_all(json.as_bytes())?; - file.sync_all()?; - std::fs::rename(&tmp_path, path)?; - // Fsync the parent directory to ensure the rename metadata is durable. - // Without this, a crash between rename and directory fsync loses the manifest. - // Routed through the cross-platform helper (no-op on Windows); errors - // propagate so callers see a save() that is not actually durable. - if let Some(parent) = path.parent() { - crate::persistence::fsync::fsync_directory(parent)?; - } - Ok(()) + crate::persistence::atomic::atomic_write_durable(path, json.as_bytes()) + .map_err(|e| io::Error::other(e.to_string())) } /// Load a manifest from a JSON file at `path`. diff --git a/src/graph/recovery.rs b/src/graph/recovery.rs index 91de6d905..14a8735b5 100644 --- a/src/graph/recovery.rs +++ b/src/graph/recovery.rs @@ -181,7 +181,31 @@ pub fn recover_graph_store( /// Save graph persistence data for a shard. /// -/// Writes metadata, manifests, and CSR segment files. +/// Writes CSR segments + manifests (the **payload**) first, then +/// `graph_metadata.json` (the **reference** — it carries `snapshot_lsn`, +/// the WAL-replay floor recovery trusts) LAST. +/// +/// This order is load-bearing (task #53 / kernel M3 K2 brief §1.2 root +/// cause): the OLD code wrote `graph_metadata.json` FIRST. A kill-9 in the +/// window between that write landing and the segment/manifest writes that +/// follow it durably advanced the WAL-replay floor (`snapshot_lsn`) past +/// data whose on-disk segments were still stale or absent — replay then +/// SKIPPED every record `<= snapshot_lsn` (trusting the published floor) +/// while the segments claiming to cover that range were never actually +/// written, permanently losing the entire batch since the prior checkpoint. +/// Reference-before-payload is exactly the "floor must never advance past +/// what is actually durable" violation this milestone's K2 register exists +/// to prevent structurally; this call site had it backwards internally. +/// +/// Writing metadata last does NOT introduce a new double-apply risk on the +/// opposite crash window (segments/manifest durable, metadata still old): +/// WAL replay's node/edge insert path (`src/graph/replay.rs`'s +/// `node_present` cross-tier check) already probes loaded immutable +/// segments before re-inserting a WAL-logged id — it was built for the +/// "restart NodeKey aliasing" P0 and is exactly the redo-idempotency an +/// ARIES-style "payload before reference" ordering requires of its replay +/// path. A crash in this window only replays already-covered records +/// again; it never duplicates them. pub fn save_graph_store( store: &GraphStore, persistence_dir: &Path, @@ -190,11 +214,7 @@ pub fn save_graph_store( let shard_dir = persistence_dir.join(format!("shard_{shard_id}")); std::fs::create_dir_all(&shard_dir)?; - // Save metadata. - let meta_path = shard_dir.join(GRAPH_METADATA_FILE); - store.save_metadata(&meta_path)?; - - // For each graph, save manifest and CSR segment files. + // For each graph, save manifest and CSR segment files (payload) FIRST. for graph_name_bytes in store.list_graphs() { let graph_name = String::from_utf8_lossy(graph_name_bytes); let graph = match store.get_graph(graph_name_bytes) { @@ -220,7 +240,9 @@ pub fn save_graph_store( // Write manifest (including the write buffer's id-allocation // cursors, so recovery resumes allocation past every id ever - // handed out even if the WAL was truncated). + // handed out even if the WAL was truncated). Still payload-side — + // it must land before metadata publishes the floor that assumes + // these segments exist. let manifest = GraphManifest::from_segments( &graph_name, &segments.immutable, @@ -231,6 +253,14 @@ pub fn save_graph_store( manifest.save(&manifest_path)?; } + // Save metadata (the reference / replay-skip floor) LAST — only once + // every graph's segments + manifest for this snapshot are durably on + // disk. A crash before this point leaves the OLD (safe, lower) floor + // in place; the next checkpoint attempt simply retries with a fresh + // snapshot_lsn. + let meta_path = shard_dir.join(GRAPH_METADATA_FILE); + store.save_metadata(&meta_path)?; + Ok(()) } @@ -264,7 +294,26 @@ pub fn persist_graph_at_checkpoint( shard_id: usize, snapshot_lsn: u64, ) -> bool { - if !store.is_dirty() || store.graph_count() == 0 { + // Task #53 review round 2 / P0-1: `graph_count() == 0` must NOT be part + // of this short-circuit. A GRAPH.DELETE that empties the graph map + // marks the store dirty via the WAL drain but drives `graph_count()` + // to 0 in the SAME tick — the old `!is_dirty() || graph_count() == 0` + // condition then short-circuited on the `== 0` disjunct and skipped + // `save_graph_store` entirely, leaving `graph_metadata.json` on disk + // stale (still describing the graph as it existed before the delete). + // Later checkpoints kept advancing `control.graph_floor_lsn` past the + // WAL record holding the DELETE (nothing here ever re-checks + // `is_dirty` once it's been wrongly treated as "nothing to persist"), + // so recycle eventually freed the segment holding that DELETE — crash, + // and recovery loads the stale pre-delete metadata with no WAL record + // left to replay the deletion: the dropped graph resurrects. Dirty + // alone is now the only gate; an empty-but-dirty store still calls + // `save_graph_store` below, whose per-graph loop correctly no-ops on + // zero graphs while `store.save_metadata` still rewrites + // `graph_metadata.json` to reflect zero graphs at the new + // `snapshot_lsn` — advancing the floor and clearing dirty only once + // that fact is durable. + if !store.is_dirty() { return true; } // No persistence dir (e.g. --appendonly no): graph writes carry no diff --git a/src/graph/store.rs b/src/graph/store.rs index dc68be52e..43fb6148c 100644 --- a/src/graph/store.rs +++ b/src/graph/store.rs @@ -496,6 +496,14 @@ impl GraphStore { /// /// This captures enough information to recreate the GraphStore structure /// on recovery. Actual graph data lives in CSR segment files and WAL. + /// + /// This file carries `snapshot_lsn` — the WAL-replay floor recovery + /// trusts unconditionally — so its write must be atomic and fsync'd + /// (task #53 / kernel M3 K2 brief §1.2 root cause: a bare `fs::write` + /// here, on top of being called before its payload was durable, could + /// also leave a torn/partial file on a kill-9 mid-write). Uses the same + /// shared temp+fsync+rename+dir-fsync primitive as CSR segments + /// (`CsrSegment::write_to_file`) and the control file. pub fn save_metadata(&self, path: &Path) -> io::Result<()> { let entries: Vec = match &self.graphs { Some(map) => map @@ -515,7 +523,8 @@ impl GraphStore { }; let json = serde_json::to_string_pretty(&meta) .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; - std::fs::write(path, json.as_bytes()) + crate::persistence::atomic::atomic_write_durable(path, json.as_bytes()) + .map_err(|e| io::Error::other(e.to_string())) } /// Load graph metadata from a JSON file and recreate graph shells. diff --git a/src/persistence/control.rs b/src/persistence/control.rs index b17a68114..26fa75acc 100644 --- a/src/persistence/control.rs +++ b/src/persistence/control.rs @@ -10,8 +10,30 @@ use std::path::{Path, PathBuf}; use crate::persistence::fsync::fsync_directory; use crate::persistence::page::{MOONPAGE_HEADER_SIZE, MoonPageHeader, PAGE_4K, PageType}; -/// Control file payload size: 1 + 8 + 8 + 8 + 8 + 8 + 16 = 57 bytes. -const CONTROL_PAYLOAD_SIZE: u32 = 57; +/// Legacy (pre-kernel-M3-K2) control file payload size: 1 + 8 + 8 + 8 + 8 + +/// 8 + 16 = 57 bytes. Named purely to anchor the backward-compat reader +/// logic and its round-trip test — [`CONTROL_PAYLOAD_SIZE`] is what +/// [`ShardControlFile::write`] actually emits today. +#[cfg(test)] +const LEGACY_CONTROL_PAYLOAD_SIZE: u32 = 57; + +/// Control file payload size: legacy 57 bytes + kernel M3 K2's per-plane +/// floor register (3 x `u64` = 24 bytes: `graph_floor_lsn`, `ws_floor_lsn`, +/// `mq_floor_lsn`) = 81 bytes. +/// +/// Additive, `payload_bytes`-driven format bump — see +/// `.planning/reviews/kernel-m3-brief-2026-07-12.md` §Stage 2, Risk #4. A +/// control file written by a pre-K2 binary has `payload_bytes == 57` and +/// nothing meaningful past byte 57 (the page is still zero-padded out to +/// 4096, but the old writer never touched that region). [`ShardControlFile::read`] +/// treats a payload shorter than a given trailing field's end offset as +/// "field absent" and defaults it to the sentinel `0` ("no floor +/// published") rather than trusting whatever bytes happen to already be +/// there — this is what makes a hand-built legacy payload (not just an +/// honestly-zero-padded one) parse safely. An OLD binary reading a NEW +/// 81-byte file also still works: it only ever reads bytes `0..57` and +/// ignores the rest. +const CONTROL_PAYLOAD_SIZE: u32 = 81; /// Shard operational state. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -61,6 +83,41 @@ pub struct ShardControlFile { pub next_page_id: u64, /// Unique shard identifier (UUID bytes). pub shard_uuid: [u8; 16], + /// Kernel M3 K2 — WAL v3 LSN covered by the last durably-persisted graph + /// snapshot, mirrored here from `GraphStore::snapshot_lsn()` at the + /// exact moment [`crate::graph::recovery::persist_graph_at_checkpoint`] + /// wrote it (same tick, same local variable — see the Finalize call + /// site; never a second, independently-recomputed LSN, per the brief's + /// Risk #1). `graph_metadata.json` remains the graph engine's own + /// replay-skip authority; this field is a *recycle-decision mirror* + /// only, so `min(last_checkpoint_lsn, graph_floor_lsn)` — not + /// `last_checkpoint_lsn`/`wal.current_lsn()` alone — is the safe floor + /// to pass to WAL segment recycling. + /// + /// Sentinel `0` = "no floor published yet" (legacy/non-disk-offload + /// mode always stays at this sentinel — Finalize, the only writer of + /// this field, never runs there). This is deliberately ambiguous with a + /// legitimately-zero LSN: recycling nothing at shard creation is + /// indistinguishable in effect from "no floor," so the ambiguity is + /// harmless (Open Decision #2 in the brief; a tagged `Option` + /// encoding was considered and rejected as unneeded complexity for a + /// case where `0` already means the same thing either way). + pub graph_floor_lsn: u64, + /// Kernel M3 K2 — reserved for a future WS (workspace registry) + /// snapshot floor. Workspaces have no snapshot format in this + /// milestone (brief §4 Non-Goals / Open Decision #1) so this field is + /// unconditionally the sentinel `0` ("no floor") and MUST be excluded + /// from the min-across-planes recycle computation — including it would + /// make `min(kv, graph, ws=0)` collapse to `0` the instant a shard has + /// ever seen a WS record, which is strictly worse than today's + /// per-segment `segment_holds_plane_history` content scan (Risk #2). + pub ws_floor_lsn: u64, + /// Kernel M3 K2 — reserved for a future MQ (streams/PEL/DLQ/triggers) + /// snapshot floor. Same sentinel-0/excluded-from-min contract as + /// [`Self::ws_floor_lsn`]; MQ snapshot format is out of scope for this + /// milestone (comparable in size to the graph engine's own format + /// work). + pub mq_floor_lsn: u64, } impl ShardControlFile { @@ -74,6 +131,9 @@ impl ShardControlFile { next_txn_id: 0, next_page_id: 0, shard_uuid, + graph_floor_lsn: 0, + ws_floor_lsn: 0, + mq_floor_lsn: 0, } } @@ -102,6 +162,10 @@ impl ShardControlFile { buf[p + 25..p + 33].copy_from_slice(&self.next_txn_id.to_le_bytes()); buf[p + 33..p + 41].copy_from_slice(&self.next_page_id.to_le_bytes()); buf[p + 41..p + 57].copy_from_slice(&self.shard_uuid); + // Kernel M3 K2 — per-plane floor register (payload bytes 57..81). + buf[p + 57..p + 65].copy_from_slice(&self.graph_floor_lsn.to_le_bytes()); + buf[p + 65..p + 73].copy_from_slice(&self.ws_floor_lsn.to_le_bytes()); + buf[p + 73..p + 81].copy_from_slice(&self.mq_floor_lsn.to_le_bytes()); // Compute CRC32C over payload and embed in header MoonPageHeader::compute_checksum(&mut buf); @@ -197,6 +261,31 @@ impl ShardControlFile { let mut shard_uuid = [0u8; 16]; shard_uuid.copy_from_slice(&buf[p + 41..p + 57]); + // Kernel M3 K2 — per-plane floor register, backward-compat-parsed. + // `hdr.payload_bytes` (not the always-4096-byte buffer length) is + // the honest marker of how much of the page a given writer + // actually populated; a pre-K2 writer stamps `payload_bytes = 57` + // and never touches bytes 57..81, so trust that marker per field + // rather than assuming the trailing region happens to be zero + // (defends against a deliberately hand-built legacy-shaped test + // payload, not just an honestly-short one — brief Risk #4). + let payload_len = hdr.payload_bytes as usize; + let graph_floor_lsn = if payload_len >= 65 { + read_u64(&buf[p + 57..p + 65])? + } else { + 0 + }; + let ws_floor_lsn = if payload_len >= 73 { + read_u64(&buf[p + 65..p + 73])? + } else { + 0 + }; + let mq_floor_lsn = if payload_len >= 81 { + read_u64(&buf[p + 73..p + 81])? + } else { + 0 + }; + Ok(Self { shard_state, last_checkpoint_lsn, @@ -205,6 +294,9 @@ impl ShardControlFile { next_txn_id, next_page_id, shard_uuid, + graph_floor_lsn, + ws_floor_lsn, + mq_floor_lsn, }) } @@ -400,4 +492,103 @@ mod tests { let read_back = ShardControlFile::read(&path).unwrap(); assert_eq!(read_back.shard_uuid, [0x42; 16]); } + + // -- Kernel M3 K2: per-plane floor register ----------------------------- + + #[test] + fn test_graph_ws_mq_floor_lsn_roundtrip() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("shard-0.control"); + + let mut ctl = ShardControlFile::new([9u8; 16]); + ctl.last_checkpoint_lsn = 500; + ctl.graph_floor_lsn = 480; + ctl.ws_floor_lsn = 0; // sentinel: no WS snapshot format this milestone + ctl.mq_floor_lsn = 0; // sentinel: no MQ snapshot format this milestone + + ctl.write(&path).unwrap(); + let read_back = ShardControlFile::read(&path).unwrap(); + assert_eq!(read_back, ctl); + assert_eq!(read_back.graph_floor_lsn, 480); + assert_eq!(read_back.ws_floor_lsn, 0); + assert_eq!(read_back.mq_floor_lsn, 0); + } + + #[test] + fn test_graph_floor_lsn_survives_max_values() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("shard-0.control"); + + let mut ctl = ShardControlFile::new([1u8; 16]); + ctl.graph_floor_lsn = u64::MAX; + ctl.ws_floor_lsn = u64::MAX - 1; + ctl.mq_floor_lsn = u64::MAX - 2; + + ctl.write(&path).unwrap(); + let read_back = ShardControlFile::read(&path).unwrap(); + assert_eq!(read_back.graph_floor_lsn, u64::MAX); + assert_eq!(read_back.ws_floor_lsn, u64::MAX - 1); + assert_eq!(read_back.mq_floor_lsn, u64::MAX - 2); + } + + #[test] + fn test_write_produces_81_byte_payload_marker() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("shard-0.control"); + ShardControlFile::new([0u8; 16]).write(&path).unwrap(); + + let buf = std::fs::read(&path).unwrap(); + let hdr = MoonPageHeader::read_from(&buf).unwrap(); + assert_eq!(hdr.payload_bytes, CONTROL_PAYLOAD_SIZE); + assert_eq!(CONTROL_PAYLOAD_SIZE, 81); + // File itself is still exactly one 4KB page — the format bump adds + // fields inside the existing headroom, not a new page size. + assert_eq!(buf.len(), PAGE_4K); + } + + /// Brief Risk #4: a control file written by a PRE-K2 binary + /// (`payload_bytes == 57`, nothing meaningful past byte 57 — not even + /// honestly zeroed, to prove the reader trusts `payload_bytes` and not + /// the buffer's happenstance contents) must still parse cleanly, with + /// the three new floor fields defaulting to the sentinel `0`. + #[test] + fn test_backward_compat_57_byte_legacy_payload_parses() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("shard-0.control"); + + let mut buf = [0u8; PAGE_4K]; + let mut hdr = MoonPageHeader::new(PageType::ControlPage, 0, 0); + hdr.payload_bytes = LEGACY_CONTROL_PAYLOAD_SIZE; + hdr.write_to(&mut buf); + + let p = MOONPAGE_HEADER_SIZE; + buf[p] = ShardState::Running as u8; + buf[p + 1..p + 9].copy_from_slice(&777u64.to_le_bytes()); // last_checkpoint_lsn + buf[p + 9..p + 17].copy_from_slice(&3u64.to_le_bytes()); // last_checkpoint_epoch + buf[p + 17..p + 25].copy_from_slice(&778u64.to_le_bytes()); // wal_flush_lsn + buf[p + 25..p + 33].copy_from_slice(&10u64.to_le_bytes()); // next_txn_id + buf[p + 33..p + 41].copy_from_slice(&20u64.to_le_bytes()); // next_page_id + buf[p + 41..p + 57].copy_from_slice(&[0x77u8; 16]); // shard_uuid + // Deliberately poison bytes 57..81 with non-zero garbage — a + // correct reader must IGNORE these because payload_bytes says the + // legacy writer never populated them, not merely happen to see + // zeros there. + buf[p + 57..p + 81].copy_from_slice(&[0xEEu8; 24]); + + MoonPageHeader::compute_checksum(&mut buf); + std::fs::write(&path, buf).unwrap(); + + let read_back = ShardControlFile::read(&path).unwrap(); + assert_eq!(read_back.last_checkpoint_lsn, 777); + assert_eq!(read_back.last_checkpoint_epoch, 3); + assert_eq!(read_back.wal_flush_lsn, 778); + assert_eq!(read_back.next_txn_id, 10); + assert_eq!(read_back.next_page_id, 20); + assert_eq!(read_back.shard_uuid, [0x77u8; 16]); + // The K2 fields must default to the sentinel, NOT the poisoned + // 0xEE bytes physically present past payload_bytes=57. + assert_eq!(read_back.graph_floor_lsn, 0); + assert_eq!(read_back.ws_floor_lsn, 0); + assert_eq!(read_back.mq_floor_lsn, 0); + } } diff --git a/src/persistence/recovery.rs b/src/persistence/recovery.rs index e7ce9303c..377aa36b4 100644 --- a/src/persistence/recovery.rs +++ b/src/persistence/recovery.rs @@ -749,6 +749,21 @@ pub fn recover_shard_v3_pitr( new_control.wal_flush_lsn = result.last_lsn; new_control.next_txn_id = control.as_ref().map(|c| c.next_txn_id).unwrap_or(0); new_control.next_page_id = control.as_ref().map(|c| c.next_page_id).unwrap_or(0); + // Kernel M3 K2: carry the per-plane floor register forward across + // restart, same pattern as every other field above. `graph_floor_lsn` + // is only a recycle-decision MIRROR of `graph_metadata.json`'s own + // `snapshot_lsn` (graph's real replay-skip authority, loaded + // independently by `recover_graph_store`); both were written together + // in the same Finalize tick, so the last successfully-written control + // file's mirror is still consistent with it. Losing this on restart + // (resetting to the `0` sentinel) would not be UNSAFE — `0` is + // maximally conservative — but it would silently disable WAL recycling + // for graph-touched shards until the next checkpoint completes, which + // is a real, avoidable regression. ws/mq stay at whatever sentinel they + // already were (always `0` this milestone — no writer exists yet). + new_control.graph_floor_lsn = control.as_ref().map(|c| c.graph_floor_lsn).unwrap_or(0); + new_control.ws_floor_lsn = control.as_ref().map(|c| c.ws_floor_lsn).unwrap_or(0); + new_control.mq_floor_lsn = control.as_ref().map(|c| c.mq_floor_lsn).unwrap_or(0); if let Err(e) = new_control.write(&control_path) { tracing::error!( "Shard {}: control file update to Running failed: {}", diff --git a/src/persistence/wal_v3/segment.rs b/src/persistence/wal_v3/segment.rs index 1847dac81..266548750 100644 --- a/src/persistence/wal_v3/segment.rs +++ b/src/persistence/wal_v3/segment.rs @@ -102,41 +102,80 @@ pub struct RecycleStats { /// Total bytes freed from disk. pub bytes_reclaimed: u64, /// LSN-eligible segments KEPT because they hold sole-copy plane history - /// (see [`segment_holds_plane_history`]). Callers surface this via the + /// (see [`segment_plane_scan`]). Callers surface this via the /// `RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL` counter. pub segments_blocked_plane: usize, } -/// Return `true` when the sealed segment at `path` contains at least one +/// Content-scan result for a sealed segment's plane-record classification. +/// +/// `blocks_recycle` is `true` when the sealed segment contains at least one /// record whose ONLY durable copy is the WAL itself. /// -/// The workspace / MQ / temporal planes have no snapshot or checkpoint -/// format in ANY mode (their replay is WAL-only — see task #43 and the -/// Wave B stage 2a review): the disk-offload checkpoint covers KV pages and -/// the graph store, nothing else. A segment holding such a record must -/// never be recycled regardless of the caller's LSN floor, or that history -/// is permanently lost on the next restart. `GraphTemporal` is included -/// conservatively: autovacuum's recycle floor (`current_lsn`) is not tied -/// to a completed graph checkpoint. +/// The workspace / MQ / temporal-upsert planes have no snapshot or +/// checkpoint format in ANY mode (their replay is WAL-only — see task #43 +/// and the Wave B stage 2a review): the disk-offload checkpoint covers KV +/// pages and the graph store, nothing else. A segment holding such a record +/// must never be recycled regardless of the caller's LSN floor, or that +/// history is permanently lost on the next restart. +/// +/// `GraphTemporal` is deliberately NOT in that set (kernel M3 K2, brief +/// §1.3): `TEMPORAL.INVALIDATE` mutates `node.valid_to`/`edge.valid_to` +/// directly on the graph's mutable write buffer +/// (`src/command/temporal.rs::apply_invalidate`), the exact tier +/// `persist_graph_at_checkpoint`'s `freeze_and_compact` flushes to CSR — +/// `valid_to` is carried into the frozen `NodeMeta`/`EdgeMeta` +/// (`src/graph/compaction.rs`). Graph's `snapshot_lsn` floor already +/// structurally covers `GraphTemporal` mutations; treating it the same as +/// WS/MQ (which have NO floor at all, ever) was over-conservative — it +/// blocked recycling a segment forever purely because it once carried a +/// `TEMPORAL.INVALIDATE`, even after the graph engine had durably +/// checkpointed well past it. K2 formalizes `graph_floor_lsn` as an +/// explicit, checked value (replacing the implicit begin()-vs-Finalize +/// ordering this guard used to compensate for), so callers now gate on +/// `min(kv_floor, graph_floor)` BEFORE reaching this content scan — +/// `GraphTemporal` segments recycle once that floor covers them, same as +/// any other graph record. /// /// Fail-closed contract: an unreadable file, a non-v3 header, an unknown /// record-type byte (a future plane type this build predates), or a -/// malformed/torn record tail all return `true` (keep the segment). A -/// crash-torn tail can only occur in the previously-active segment of an -/// earlier process generation — blocking that one segment is bounded and -/// beats guessing. A `record_len == 0` tail is normal zero-padding → end -/// of records. -fn segment_holds_plane_history(path: &Path) -> bool { +/// malformed/torn record tail all set `blocks_recycle = true` (keep the +/// segment). A crash-torn tail can only occur in the previously-active +/// segment of an earlier process generation — blocking that one segment is +/// bounded and beats guessing. A `record_len == 0` tail is normal +/// zero-padding → end of records. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +struct SegmentPlaneScan { + /// WS/MQ/`TemporalUpsert` present (or unreadable/torn/future-typed) — + /// this segment must never be recycled by an LSN floor alone. + blocks_recycle: bool, + /// At least one `GraphTemporal` record is present. Never gates + /// recycling by itself (that's `graph_floor_lsn`'s job, K2) — surfaced + /// only so callers can observe the guard-shrink take effect via + /// `RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL`. + has_graph_temporal: bool, +} + +/// Single-pass content scan backing both the recycle-gating boolean +/// (`blocks_recycle`) and the K2 `GraphTemporal` observability +/// counter, so recyclers never need to read a sealed segment file twice. +fn segment_plane_scan(path: &Path) -> SegmentPlaneScan { use super::record::WalRecordType; + const BLOCKED: SegmentPlaneScan = SegmentPlaneScan { + blocks_recycle: true, + has_graph_temporal: false, + }; + let data = match fs::read(path) { Ok(d) => d, - Err(_) => return true, + Err(_) => return BLOCKED, }; if data.len() < WAL_V3_HEADER_SIZE || &data[..6] != WAL_V3_MAGIC || data[6] != WAL_V3_VERSION { - return true; + return BLOCKED; } let mut offset = WAL_V3_HEADER_SIZE; + let mut has_graph_temporal = false; while offset + 4 <= data.len() { let record_len = u32::from_le_bytes([ data[offset], @@ -145,16 +184,19 @@ fn segment_holds_plane_history(path: &Path) -> bool { data[offset + 3], ]) as usize; if record_len == 0 { - return false; // zero-padded tail — clean end of records + // zero-padded tail — clean end of records + return SegmentPlaneScan { + blocks_recycle: false, + has_graph_temporal, + }; } // Minimum framing: len(4) + header(12) + crc(4). if record_len < 20 || offset + record_len > data.len() { - return true; // torn/malformed sealed tail — fail closed + return BLOCKED; // torn/malformed sealed tail — fail closed } - let plane = match WalRecordType::from_u8(data[offset + 12]) { + match WalRecordType::from_u8(data[offset + 12]) { Some( WalRecordType::TemporalUpsert - | WalRecordType::GraphTemporal | WalRecordType::WorkspaceCreate | WalRecordType::WorkspaceDrop | WalRecordType::MqCreate @@ -162,16 +204,17 @@ fn segment_holds_plane_history(path: &Path) -> bool { | WalRecordType::MqPush | WalRecordType::MqPop | WalRecordType::MqTrigger, - ) => true, - Some(_) => false, - None => return true, // unknown (future) type — fail closed - }; - if plane { - return true; + ) => return BLOCKED, + Some(WalRecordType::GraphTemporal) => has_graph_temporal = true, + Some(_) => {} + None => return BLOCKED, // unknown (future) type — fail closed } offset += record_len; } - false + SegmentPlaneScan { + blocks_recycle: false, + has_graph_temporal, + } } /// WAL v3 writer with segmented files, per-record LSN, and batched fsync. @@ -585,8 +628,13 @@ impl WalWriterV3 { // Plane guard: WS/MQ/temporal records have no snapshot in any // mode — the WAL is their sole durable copy, so no LSN floor // makes this segment safe to delete. See - // `segment_holds_plane_history` for the fail-closed contract. - if segment_holds_plane_history(&seg.path) { + // `segment_plane_scan` for the fail-closed contract. + // `GraphTemporal` is no longer in that guard (K2, brief §1.3) — + // `scan.has_graph_temporal` observes when THIS caller's floor + // (already `min(kv, graph)`-derived by K2 callers) is what let + // such a segment through. + let scan = segment_plane_scan(&seg.path); + if scan.blocks_recycle { segments_blocked_plane += 1; continue; } @@ -597,6 +645,10 @@ impl WalWriterV3 { bytes_reclaimed += seg.file_size; // Emit reclamation metrics for P10 INFO emitter. crate::admin::metrics_setup::record_wal_aggressive_recycle(1, seg.file_size); + if scan.has_graph_temporal { + crate::command::info_reclamation::RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } } Err(e) => { // Log but continue — partial reclamation is better than none. @@ -781,8 +833,13 @@ impl WalWriterV3 { } // Plane guard: the checkpoint floor covers KV pages + graph // only — WS/MQ/temporal history has no snapshot in any mode. - // See `segment_holds_plane_history` (fail-closed). - if segment_holds_plane_history(&seg.path) { + // See `segment_plane_scan` (fail-closed). `redo_lsn` + // is `min(control.last_checkpoint_lsn, control.graph_floor_lsn)` + // as of K2, so `GraphTemporal` segments (no longer in that + // guard's match arm) recycle here once the graph floor covers + // them. + let scan = segment_plane_scan(&seg.path); + if scan.blocks_recycle { tracing::debug!( "WAL recycle: keeping segment {:?} — holds sole-copy plane history", seg.path @@ -799,6 +856,10 @@ impl WalWriterV3 { } else { total_wal_size -= seg.file_size; recycled += 1; + if scan.has_graph_temporal { + crate::command::info_reclamation::RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + } } } Ok(recycled) @@ -1321,6 +1382,146 @@ mod tests { ); } + /// Kernel M3 K2 — inverted mirror of + /// `test_recycle_aggressive_keeps_plane_history_segments`: a sealed + /// segment holding ONLY `GraphTemporal` records must now be freed once + /// the graph floor covers it (brief §1.3's "real, low-risk K2 win"). + /// Also asserts the `RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL` + /// observability counter (work item 5) fires. + #[test] + fn test_recycle_frees_graphtemporal_segment_once_graph_floor_covers_it() { + let tmp = tempfile::tempdir().unwrap(); + let wal_dir = tmp.path().join("wal"); + let mut writer = WalWriterV3::new(0, &wal_dir, 512).unwrap(); + writer.set_wal_bounds(0, u64::MAX); + + // First record = a GraphTemporal (TEMPORAL.INVALIDATE) plane record + // → lands in segment 1. + writer.append(WalRecordType::GraphTemporal, b"\x01graph-temporal-payload"); + for i in 0..60 { + writer.append(WalRecordType::Command, b"SET key val"); + if (i + 1) % 3 == 0 { + writer.flush_sync().unwrap(); + } + } + writer.flush_sync().unwrap(); + assert!( + writer.current_segment_sequence() >= 4, + "need several sealed segments" + ); + + let before_freed = + crate::command::info_reclamation::RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL + .load(std::sync::atomic::Ordering::Relaxed); + + // A high floor simulates K2's min(kv_floor, graph_floor) once a + // checkpoint's graph snapshot has covered this record — segment.rs + // itself only ever sees the pre-computed floor, never + // `ws_floor_lsn`/`mq_floor_lsn` (brief §Stage 2's min-across-planes + // correction: those stay excluded, see the sibling Risk #2 test). + let recycled = writer.recycle_segments_before(u64::MAX).unwrap(); + assert!(recycled >= 1, "some segment must have been recycled"); + + let plane_path = WalSegment::segment_path(&wal_dir, 1); + assert!( + !plane_path.exists(), + "segment holding ONLY a GraphTemporal record must now recycle \ + once the graph floor covers it (K2 — GraphTemporal is no \ + longer in the no-floor bucket)" + ); + + let after_freed = + crate::command::info_reclamation::RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL + .load(std::sync::atomic::Ordering::Relaxed); + assert!( + after_freed > before_freed, + "freeing a GraphTemporal-holding segment must be observable via \ + RECL_WAL_RECYCLE_GRAPH_TEMPORAL_FREED_TOTAL" + ); + } + + /// Kernel M3 K2 brief Risk #2 — "min-across-planes collapsing to zero": + /// a pure-KV segment written AFTER the shard's last WS/MQ record must + /// still recycle once the KV+graph floor covers it. This is the + /// regression the brief explicitly warns against: folding `ws_floor_lsn` + /// (sentinel `0`, no snapshot format this milestone) into a single + /// scalar `min(kv, graph, ws, mq)` would make the floor `0` forever on + /// any shard that has EVER seen a WS/MQ record — recycling nothing, + /// ever, from that point on, which is strictly worse than the + /// per-segment content scan alone. The correct caller-side computation + /// (`persistence_tick.rs` / `autovacuum.rs`) excludes the WS/MQ + /// sentinel entirely; this test proves `recycle_segments_before` still + /// frees later pure-KV segments when driven by that (correct) floor, + /// not a WS/MQ-poisoned one. + #[test] + fn test_recycle_min_across_planes_pure_kv_after_ws_still_recycles() { + let tmp = tempfile::tempdir().unwrap(); + let wal_dir = tmp.path().join("wal"); + let mut writer = WalWriterV3::new(0, &wal_dir, 512).unwrap(); + writer.set_wal_bounds(0, u64::MAX); + + // The shard's ONLY WS record, early — segment 1. + writer.append(WalRecordType::WorkspaceCreate, b"ws-plane-payload"); + // Plenty of pure-KV traffic AFTER it, sealing several later + // Command-only segments. + for i in 0..120 { + writer.append(WalRecordType::Command, b"SET key val filler-bytes"); + if (i + 1) % 3 == 0 { + writer.flush_sync().unwrap(); + } + } + writer.flush_sync().unwrap(); + assert!( + writer.current_segment_sequence() >= 5, + "need several sealed segments after the WS record" + ); + let count_wals = || -> usize { + fs::read_dir(&wal_dir) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_string_lossy().ends_with(".wal")) + .count() + }; + let before = count_wals(); + + // Correct K2 caller-side floor: min(kv_floor, graph_floor) ONLY — + // ws_floor_lsn (sentinel 0) is NOT part of this computation, per + // the brief's explicit correction. Simulate a fully-caught-up + // checkpoint (both KV and graph floors at the WAL's current head). + let kv_floor = writer.current_lsn(); + let graph_floor = writer.current_lsn(); + let correct_floor = kv_floor.min(graph_floor); + + let recycled = writer.recycle_segments_before(correct_floor).unwrap(); + assert!( + recycled >= 1, + "pure-KV segments after the shard's last WS record must still \ + recycle under the correct min(kv, graph) floor" + ); + let after = count_wals(); + assert!(after < before, "segment count must have decreased"); + + // The WS-holding segment itself must still survive (content-scan + // AND-gate, orthogonal to the LSN floor). + let plane_path = WalSegment::segment_path(&wal_dir, 1); + assert!( + plane_path.exists(), + "the WS-holding segment must still survive — only pure-KV \ + segments after it were expected to recycle" + ); + + // Negative control (documentation, not a runtime assertion — the + // whole point of Risk #2 is that a buggy caller folding + // `ws_floor_lsn` (sentinel 0) into the min would compute floor `0` + // here, at which point `recycle_segments_before(0)` recycles + // NOTHING — the opposite of what this test just proved with the + // correct floor above). + assert!( + correct_floor > 0, + "sanity: the correct floor is non-zero here" + ); + } + #[test] fn test_recycle_respects_min_wal_size() { let tmp = tempfile::tempdir().unwrap(); diff --git a/src/server/conn/handler_monoio/mod.rs b/src/server/conn/handler_monoio/mod.rs index c204d384c..514d5ae28 100644 --- a/src/server/conn/handler_monoio/mod.rs +++ b/src/server/conn/handler_monoio/mod.rs @@ -1292,6 +1292,8 @@ pub(crate) async fn handle_connection_sharded_monoio< None, // wal_v3 — not available in connection handler cmd_args, crate::command::server_admin::DEFAULT_VACUUM_PRUNE_MARGIN, + None, // disk_offload_dir — dead: wal is None on this path too + 0, // shard_id — dead, see above ) }); responses.push(response); diff --git a/src/server/conn/handler_sharded/mod.rs b/src/server/conn/handler_sharded/mod.rs index d7ebedb5a..b7628ad94 100644 --- a/src/server/conn/handler_sharded/mod.rs +++ b/src/server/conn/handler_sharded/mod.rs @@ -1156,6 +1156,8 @@ pub(crate) async fn handle_connection_sharded_inner< None, cmd_args, crate::command::server_admin::DEFAULT_VACUUM_PRUNE_MARGIN, + None, // disk_offload_dir — dead: wal is None on this path too + 0, // shard_id — dead, see above ) }); responses.push(response); diff --git a/src/server/conn/handler_single.rs b/src/server/conn/handler_single.rs index 538a11c7d..1b24da548 100644 --- a/src/server/conn/handler_single.rs +++ b/src/server/conn/handler_single.rs @@ -2097,6 +2097,8 @@ pub async fn handle_connection( None, // wal_v3 — not available here d_args, crate::command::server_admin::DEFAULT_VACUUM_PRUNE_MARGIN, // see server_admin.rs + None, // disk_offload_dir — dead: wal is None on this path too + 0, // shard_id — dead, see above ); drop(vs_guard); responses[resp_idx] = response; @@ -2399,6 +2401,8 @@ pub async fn handle_connection( None, // wal_v3 — not available here d_args, crate::command::server_admin::DEFAULT_VACUUM_PRUNE_MARGIN, // see server_admin.rs + None, // disk_offload_dir — dead: wal is None on this path too + 0, // shard_id — dead, see above ); drop(vs_guard); responses[resp_idx] = response; diff --git a/src/shard/autovacuum.rs b/src/shard/autovacuum.rs index 165a8b3f5..8d3589966 100644 --- a/src/shard/autovacuum.rs +++ b/src/shard/autovacuum.rs @@ -245,6 +245,11 @@ impl AutovacuumDaemon { #[cfg(feature = "graph")] graph_store: &mut crate::graph::store::GraphStore, manifest: Option<&mut crate::persistence::manifest::ShardManifest>, wal_v3: Option<&mut crate::persistence::wal_v3::segment::WalWriterV3>, + // Kernel M3 K2: the shard's floor register, read-only here (Pass C + // never writes it — only checkpoint Finalize does). `None` in + // legacy/non-disk-offload mode (no control file exists there) or + // before the first checkpoint has run. + control_file: Option<&crate::persistence::control::ShardControlFile>, max_wal_bytes: u64, disk_offload_enabled: bool, manifest_retain_epochs: u64, @@ -356,17 +361,33 @@ impl AutovacuumDaemon { match wal.stats() { Ok(wal_stats) if max_wal_bytes > 0 && wal_stats.total_bytes > max_wal_bytes => { if disk_offload_enabled { - // Disk-offload mode: the checkpoint protocol maintains - // a real redo_lsn and persist_graph_at_checkpoint - // snapshots the graph write-buffer, so recycling - // everything before current_lsn is backed by an - // actual durable floor — for KV and graph. The - // WS/MQ/temporal planes have NO snapshot in this mode - // either; `recycle_aggressive` itself skips any - // segment holding such records (Wave B stage 2a - // review fix) and reports them via - // `segments_blocked_plane`. - let redo_lsn = wal.current_lsn(); + // Disk-offload mode: kernel M3 K2 min-across-planes + // floor. `wal.current_lsn()` (the live, not-yet- + // checkpointed LSN counter) is NOT a durable floor — + // using it here would let Pass C recycle segments + // holding KV/graph records that no completed + // checkpoint or graph snapshot has actually + // materialized yet. The real floor is + // `min(control.last_checkpoint_lsn, + // control.graph_floor_lsn)` — only KV and graph + // publish a real LSN floor; ws/mq stay at the + // sentinel `0` and are DELIBERATELY excluded from + // this min (see `ShardControlFile::ws_floor_lsn` + // doc + brief §Stage 2's "min-across-planes" + // correction) — folding them in would collapse the + // floor to `0` forever on any shard that has ever + // seen a WS/MQ record. `segment_holds_plane_history` + // remains the orthogonal, per-segment, content-scan + // AND-gate for those planes (`recycle_aggressive` + // skips any segment holding such a record and + // reports it via `segments_blocked_plane`). No + // control file yet (very first tick, before any + // checkpoint has completed) → floor `0`, i.e. + // recycle nothing this tick — maximally + // conservative, self-heals once Finalize runs. + let redo_lsn = control_file + .map(|c| c.last_checkpoint_lsn.min(c.graph_floor_lsn)) + .unwrap_or(0); match wal.recycle_aggressive(redo_lsn) { Ok(recycled) => { let pass_ms = pass_start.elapsed().as_millis() as u64; @@ -941,6 +962,7 @@ mod tests { &mut graph_store, None, Some(&mut writer), + None, // control_file: legacy mode never publishes a floor 256, // max_wal_bytes: tiny, so the ceiling is already breached false, // disk_offload_enabled: legacy mode — the fix under test 0, @@ -994,12 +1016,19 @@ mod tests { #[cfg(feature = "graph")] let mut graph_store = crate::graph::store::GraphStore::new(); let mut daemon = AutovacuumDaemon::new(AutovacuumConfig::default()); + // Kernel M3 K2: Pass C now floors on min(kv, graph) from the control + // file rather than the live `wal.current_lsn()` — simulate a + // checkpoint that has already covered everything written above. + let mut control_file = crate::persistence::control::ShardControlFile::new([0u8; 16]); + control_file.last_checkpoint_lsn = u64::MAX; + control_file.graph_floor_lsn = u64::MAX; daemon.run_tick( &mut vector_store, #[cfg(feature = "graph")] &mut graph_store, None, Some(&mut writer), + Some(&control_file), 256, // max_wal_bytes: tiny, so the ceiling is already breached true, // disk_offload_enabled 0, @@ -1031,7 +1060,9 @@ mod tests { /// Disk-offload mode recycling of PURE-KV history is unchanged by task /// #43 and by the stage-2a plane guard: Pass C still recycles - /// Command-only segments against `current_lsn()`. + /// Command-only segments once a real checkpoint floor + /// (`min(control.last_checkpoint_lsn, control.graph_floor_lsn)`, kernel + /// M3 K2) covers them. #[test] fn test_pass_c_disk_offload_mode_recycles_unchanged() { let tmp = tempfile::tempdir().unwrap(); @@ -1044,12 +1075,16 @@ mod tests { #[cfg(feature = "graph")] let mut graph_store = crate::graph::store::GraphStore::new(); let mut daemon = AutovacuumDaemon::new(AutovacuumConfig::default()); + let mut control_file = crate::persistence::control::ShardControlFile::new([0u8; 16]); + control_file.last_checkpoint_lsn = u64::MAX; + control_file.graph_floor_lsn = u64::MAX; daemon.run_tick( &mut vector_store, #[cfg(feature = "graph")] &mut graph_store, None, Some(&mut writer), + Some(&control_file), 256, // max_wal_bytes: tiny, so the ceiling is already breached true, // disk_offload_enabled: checkpoint-backed mode — unchanged 0, diff --git a/src/shard/event_loop.rs b/src/shard/event_loop.rs index d843ba951..c700bbcf7 100644 --- a/src/shard/event_loop.rs +++ b/src/shard/event_loop.rs @@ -1744,6 +1744,7 @@ impl super::Shard { &mut s.graph_store, shard_manifest.as_mut(), wal_writer.as_mut(), + control_file.as_ref(), server_config.max_wal_size_bytes(), server_config.disk_offload_enabled(), server_config.manifest_tombstone_retain_epochs, @@ -2351,6 +2352,7 @@ impl super::Shard { &mut s.graph_store, shard_manifest.as_mut(), wal_writer.as_mut(), + control_file.as_ref(), server_config.max_wal_size_bytes(), server_config.disk_offload_enabled(), server_config.manifest_tombstone_retain_epochs, diff --git a/src/shard/persistence_tick.rs b/src/shard/persistence_tick.rs index ec71eace1..3366d6d14 100644 --- a/src/shard/persistence_tick.rs +++ b/src/shard/persistence_tick.rs @@ -1045,7 +1045,12 @@ pub(crate) fn maybe_force_checkpoint_on_wal_overflow( // was a no-op (checkpoint already active) or failed silently, using the // current WAL head would be unsafe — we would recycle segments whose dirty // pages have not been flushed to data files yet. - let redo_lsn = control.last_checkpoint_lsn; + // + // Kernel M3 K2 review round 2 / P1-1: same min-across-planes floor as + // every other recycle call site (Finalize, Pass C, VACUUM) — KV alone + // is not enough, the graph engine's own snapshot floor must also cover + // whatever this emergency path is about to recycle. + let redo_lsn = control.last_checkpoint_lsn.min(control.graph_floor_lsn); match wal.recycle_aggressive(redo_lsn) { Ok(stats) if stats.segments_recycled > 0 => { tracing::info!( @@ -1265,15 +1270,33 @@ pub(crate) fn handle_checkpoint_tick( // covers records strictly below it, so the floor is one less // (the G5 crash test's first post-checkpoint record lands // exactly on `current_lsn()` and must NOT be skipped). - if !graph_save(wal.current_lsn().saturating_sub(1)) { + // + // Kernel M3 K2: capture this LSN into a local variable ONCE and + // reuse it both as the arg to `graph_save` AND, below, as + // `control.graph_floor_lsn` — never a second, independently + // recomputed `wal.current_lsn()` call at the control-file write + // site. Two computations of "now" a few lines apart is exactly + // the silent-drift risk the brief's Risk #1 calls out: a future + // refactor that moves one call earlier than the other would + // desynchronize the mirror from what `persist_graph_at_checkpoint` + // actually snapshotted, with nothing failing except a rare crash + // test. Same variable, same tick, by construction. + let graph_floor_lsn = wal.current_lsn().saturating_sub(1); + if !graph_save(graph_floor_lsn) { tracing::error!("Checkpoint aborted: graph snapshot failed"); checkpoint_mgr.note_finalize_failed(std::time::Instant::now()); return false; } - // 4. Update control file with new checkpoint LSN + // 4. Update control file with new checkpoint LSN + the graph + // floor mirror (K2). `graph_metadata.json` (written durably by + // `graph_save` above, which just returned `true`) remains the + // graph engine's own replay-skip authority; `graph_floor_lsn` + // here is a recycle-decision mirror of that SAME value, so the + // two can never disagree. control.last_checkpoint_lsn = redo_lsn; control.last_checkpoint_epoch = manifest.epoch(); + control.graph_floor_lsn = graph_floor_lsn; if let Err(e) = control.write(control_path) { tracing::error!("Checkpoint control file update failed: {}", e); checkpoint_mgr.note_finalize_failed(std::time::Instant::now()); @@ -1283,8 +1306,25 @@ pub(crate) fn handle_checkpoint_tick( // 5. Mark checkpoint complete (also clears the finalize backoff). checkpoint_mgr.complete(); - // 6. Recycle old WAL segments that are fully before redo_lsn - match wal.recycle_segments_before(redo_lsn) { + // 6. Recycle old WAL segments — kernel M3 K2's min-across-planes + // floor. Only KV (`last_checkpoint_lsn`) and graph + // (`graph_floor_lsn`) publish a real LSN floor this milestone; + // ws/mq stay at the sentinel `0` and are DELIBERATELY excluded + // from this min (see `ShardControlFile::ws_floor_lsn` doc + + // brief §Stage 2's "min-across-planes" correction / Risk #2) — + // folding them in would collapse the floor to `0` forever on + // any shard that has ever seen a WS/MQ record, which is + // strictly worse than today's per-segment + // `segment_holds_plane_history` content scan (still applied + // inside `recycle_segments_before`, orthogonally, as the + // AND-gate for those planes). `redo_lsn == control.last_checkpoint_lsn` + // here (just assigned above), so this is `min(redo_lsn, + // graph_floor_lsn)` in practice — structurally `>= redo_lsn` + // always held implicitly via begin()-vs-Finalize call-site + // ordering before K2; this makes it an explicit, checked value + // instead of relying on that ordering never drifting. + let recycle_floor = control.last_checkpoint_lsn.min(control.graph_floor_lsn); + match wal.recycle_segments_before(recycle_floor) { Ok(n) if n > 0 => { tracing::info!("Checkpoint: recycled {} old WAL segment(s)", n); } @@ -1295,8 +1335,9 @@ pub(crate) fn handle_checkpoint_tick( } tracing::info!( - "Checkpoint complete: redo_lsn={}, epoch={}", + "Checkpoint complete: redo_lsn={}, graph_floor_lsn={}, epoch={}", redo_lsn, + graph_floor_lsn, manifest.epoch() ); true diff --git a/src/shard/spsc_handler.rs b/src/shard/spsc_handler.rs index 3107620a2..9d2a5d6ab 100644 --- a/src/shard/spsc_handler.rs +++ b/src/shard/spsc_handler.rs @@ -568,6 +568,8 @@ pub(crate) fn handle_shard_message_shared( wal_writer.as_mut(), args, mvcc_prune_margin, + disk_offload_dir, + shard_id, ) }); let _ = reply_tx.send(frame); diff --git a/tests/crash_matrix_cross_plane.rs b/tests/crash_matrix_cross_plane.rs index 22ac8210f..175c32070 100644 --- a/tests/crash_matrix_cross_plane.rs +++ b/tests/crash_matrix_cross_plane.rs @@ -68,8 +68,16 @@ //! vacuous kv-spill filler, a shared `red_guard` masking an unrelated GREEN //! atomicity claim, a missing `GRAPH.CREATE` sync-wait in the split-out //! atomicity check, a substring-based benign-error allowlist) — see git -//! history for the fix-by-fix trail. **40 cells total: 29 GREEN by -//! default, 11 RED.** +//! history for the fix-by-fix trail. Kernel M3 stage 2 (K2, task #53) then +//! root-caused and fixed former RED cell 4 (checkpoint-Finalize graph total +//! loss), flipping its 2 cells GREEN. A K2 adversarial review round then +//! found and fixed a second graph-durability P0 (drop-resurrection across +//! repeated checkpoints — an empty-but-dirty graph store's checkpoint save +//! was wrongly short-circuited) and added 2 new regression cells, +//! `cross_plane_prod_s1_graph_drop_survives_repeated_checkpoints` / +//! `cross_plane_prod_s4_graph_drop_survives_repeated_checkpoints`, both +//! GREEN by default (RED-first verified against the pre-fix code before +//! the fix landed). **42 cells total: 33 GREEN by default, 9 RED.** //! //! RED cells are gated by `harness::red_guard` — NOT by //! `#[ignore = "RED: ..."]` alone, because this suite's own execution @@ -133,36 +141,40 @@ //! atomicity check's own graph-leg assertion errors "graph not found" //! before it can ever evaluate atomicity). //! -//! **RED cell 4 — checkpoint-Finalize window total-losses the graph plane, -//! NEW (2 cells):** `cross_plane_prod_s1_mixed_all_planes_mid_checkpoint`, +//! **FIXED (was RED cell 4) — checkpoint-Finalize window total-losses the +//! graph plane, kernel M3 stage 2 / K2, task #53 (2 cells, now GREEN):** +//! `cross_plane_prod_s1_mixed_all_planes_mid_checkpoint`, //! `cross_plane_prod_s4_mixed_all_planes_mid_checkpoint`. Found via //! `MOON_CRASH_MATRIX_ITERS` soak, NOT the default single-iteration run — -//! this is a PROBABILISTIC finding (some, not all, kill offsets in the -//! 0-150ms post-`BGSAVE` window trigger it — confirmed absent in the -//! default `MOON_CRASH_MATRIX_RED=1`/`ITERS=1` full-suite run that produced -//! this doc's final numbers, exactly as this caution note warns). Some -//! offsets cause TOTAL loss of the graph plane's content — not the unsynced -//! tail, the FULL `MixedPlan::default()` batch that was already confirmed -//! durable via a wal-v3 sync-wait BEFORE `BGSAVE` was even issued — while -//! KV, vector, WS, and MQ all survive in the same run. Reviewed and -//! confirmed NOT a harness artifact (KV assertion runs first, -//! unconditionally, in `assert_mixed_truth_recovered`, and never fails -//! here). Reproduced via `MOON_CRASH_MATRIX_ITERS=20` in isolation at both -//! shard counts (prod_s1: hit at iteration 11/20; prod_s4: hit at iteration -//! 7/20) — confirming this is a real, load-sensitive timing window, not a -//! one-off VM hiccup from the full-suite run that first surfaced it. See -//! `scenarios::mixed_mid_checkpoint`'s doc for the full analysis -//! (consistent with, though not proven at the step level to be, a -//! `persist_graph_at_checkpoint`-vs-`wal.recycle_segments_before` ordering -//! gap in `CheckpointAction::Finalize`). Strong P0 candidate for kernel M3 -//! stage 2 (K2, the unified per-shard floor register); not this stage's -//! job to fix. **Caution when reproducing:** because this is probabilistic, -//! `MOON_CRASH_MATRIX_RED=1` alone with the default `MOON_CRASH_MATRIX_ITERS=1` -//! can report green by chance — use -//! `MOON_CRASH_MATRIX_RED=1 MOON_CRASH_MATRIX_ITERS=20` to reproduce -//! reliably. -//! -//! **Every other cell (29/40) is GREEN** — including, notably, +//! this was a PROBABILISTIC finding (some, not all, kill offsets in the +//! 0-150ms post-`BGSAVE` window triggered it — confirmed absent in a +//! default `MOON_CRASH_MATRIX_RED=1`/`ITERS=1` full-suite run, exactly as +//! this caution note warns). Some offsets caused TOTAL loss of the graph +//! plane's content — not the unsynced tail, the FULL `MixedPlan::default()` +//! batch that was already confirmed durable via a wal-v3 sync-wait BEFORE +//! `BGSAVE` was even issued — while KV, vector, WS, and MQ all survived in +//! the same run. Root cause: `save_graph_store` +//! (`src/graph/recovery.rs`) wrote the reference/floor +//! (`graph_metadata.json`, via `GraphStore::save_metadata`) BEFORE the +//! payload it claims durable (CSR segments + `manifest.json`) — an +//! ARIES-inverted ordering. A kill-9 landing between the two writes left a +//! fully-advanced floor pointing at a payload that never made it to disk, +//! so recovery trusted the floor and skipped WAL replay for records the +//! floor claimed were already covered. 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 +//! `persistence::atomic::atomic_write_durable`). Safe against +//! double-replay: `graph::replay::node_present` checks both `write_buf` +//! and loaded CSR segments before re-inserting a WAL-logged node/edge, so +//! replaying a record whose payload DID make it to disk before the kill is +//! a no-op. See `scenarios::mixed_mid_checkpoint`'s doc for the full +//! scenario analysis. Confirmed fixed via `MOON_CRASH_MATRIX_RED=1 +//! MOON_CRASH_MATRIX_ITERS=20` soak, 20/20 clean at both shard counts +//! (pre-fix baseline: prod_s1 hit at iteration 11/20, prod_s4 at iteration +//! 7/20) — these two tests now run ungated (green-only default suite). +//! +//! **Every other cell (33/42) is GREEN** — including, notably, //! `kv_isolated`/`kv_spilled_isolated`/`vector_isolated` on ALL configs (KV //! and vector-HSET durability via the AOF; `kv_spilled_isolated`'s filler //! sizing was hardened in review round 3 — see @@ -174,13 +186,13 @@ //! #291's effect-record fix holds), `txn_isolated_atomicity` on //! `prod_s1`/`prod_s4` (queuing without `EXEC` then killing applies nothing //! — verified 3× consecutive), `mixed_all_planes_synced`/`mid_pass_c` on -//! both production shard counts, `mixed_all_planes_mid_checkpoint` at the -//! default single-iteration sample (RED cell 4 above is probabilistic — -//! most single samples don't hit the window), and -//! `mixed_all_planes_concurrent_burst_no_corruption` on all 3 configs it -//! runs on (no plane's on-disk structures were ever corrupted badly enough -//! to error post-restart, beyond the expected "schema object never made it -//! to disk before the kill" not-found case). +//! both production shard counts, `mixed_all_planes_mid_checkpoint` on both +//! production shard counts (kernel M3 stage 2 / K2 fix above — now +//! unconditionally GREEN, not just at the default single-iteration +//! sample), and `mixed_all_planes_concurrent_burst_no_corruption` on all 3 +//! configs it runs on (no plane's on-disk structures were ever corrupted +//! badly enough to error post-restart, beyond the expected "schema object +//! never made it to disk before the kill" not-found case). //! //! # Soak cadence //! diff --git a/tests/crash_matrix_cross_plane/planes.rs b/tests/crash_matrix_cross_plane/planes.rs index 890072355..bd1864451 100644 --- a/tests/crash_matrix_cross_plane/planes.rs +++ b/tests/crash_matrix_cross_plane/planes.rs @@ -180,6 +180,35 @@ pub fn temporal_invalidate_node(c: &mut Conn, node_id: &str, graph: &str) { ); } +/// `GRAPH.DELETE ` — drops a named graph and all its data. +pub fn graph_delete(c: &mut Conn, name: &str) { + assert_eq!( + c.cmd_s(&["GRAPH.DELETE", name]), + Resp::Simple("OK".into()), + "GRAPH.DELETE {name}" + ); +} + +/// `GRAPH.LIST` — the set of currently-registered graph names. Used as the +/// single source of truth for "does this graph still exist" (task #53 +/// review round 2 / P0-1 drop-resurrection regression cell) instead of +/// `GRAPH.QUERY`'s error-string matching, which conflates "graph never +/// existed" with "graph correctly deleted". +pub fn graph_list_names(c: &mut Conn) -> BTreeSet { + match c.cmd_s(&["GRAPH.LIST"]) { + Resp::Array(Some(items)) => items + .iter() + .map(|it| match it { + Resp::Bulk(Some(b)) => String::from_utf8_lossy(b).into_owned(), + Resp::Simple(s) => s.clone(), + other => panic!("GRAPH.LIST unexpected item: {other:?}"), + }) + .collect(), + Resp::Array(None) => BTreeSet::new(), + other => panic!("GRAPH.LIST malformed reply: {other:?}"), + } +} + // --------------------------------------------------------------------------- // Vector // --------------------------------------------------------------------------- diff --git a/tests/crash_matrix_cross_plane/scenarios.rs b/tests/crash_matrix_cross_plane/scenarios.rs index 634e2934b..8c0687336 100644 --- a/tests/crash_matrix_cross_plane/scenarios.rs +++ b/tests/crash_matrix_cross_plane/scenarios.rs @@ -657,33 +657,21 @@ pub fn mixed_synced(cfg: &Config) { /// injection). Only meaningful when `disk_offload` is enabled (Finalize is /// the checkpoint-backed-mode-only code path, brief §1.1). /// -/// REAL RED FINDING (kernel M3, NEW — found via `MOON_CRASH_MATRIX_ITERS` -/// soak, reviewed and confirmed NOT a harness artifact): on both -/// `prod_s1` and `prod_s4`, some kill offsets in the 0-150ms -/// post-`BGSAVE` window cause TOTAL loss of the graph plane's content — -/// not partial loss of the unsynced tail, but the FULL set of already -/// wal-v3-synced nodes (the entire `MixedPlan::default()` graph batch, -/// confirmed durable via `graph_addnode_marker`'s wal-v3 wait BEFORE -/// `BGSAVE` is even issued) comes back completely EMPTY. KV, vector, WS, -/// and MQ all survive in the same run (`assert_mixed_truth_recovered` -/// checks KV first, unconditionally, before the graph check that panics — -/// KV never fails here). Reproduced via `MOON_CRASH_MATRIX_ITERS=20` in -/// isolation (not full-suite — the full-suite run's contention only -/// makes this window easier to hit, it isn't the cause): prod_s1 hit at -/// iteration 11/20, prod_s4 at iteration 7/20 — a real, load-sensitive but -/// unambiguous timing window, not a one-off VM hiccup. Consistent with -/// (though not proven to be exactly) a checkpoint `Finalize` step ordering -/// gap between `persist_graph_at_checkpoint`'s snapshot write and -/// `wal.recycle_segments_before`'s WAL-v3 segment recycle -/// (`src/shard/persistence_tick.rs` ~1182-1301) — if recycle can run (or -/// its effect become visible after a kill) before the graph snapshot is -/// durably on disk, the WAL copy of the data is gone AND the snapshot -/// never had it either. Not proven at the step level (no fault-injection -/// hooks exist — see the module doc's `kill_mid_checkpoint` honesty note); -/// this is exactly the class of finding that note anticipated soak testing -/// would surface. This is a strong candidate P0 for kernel M3 stage 2 (K2, -/// the unified per-shard floor register) to close, not this stage's job to -/// fix. +/// FIXED (kernel M3 stage 2 / K2, task #53): on both `prod_s1` and +/// `prod_s4`, some kill offsets in the 0-150ms post-`BGSAVE` window used +/// to cause TOTAL loss of the graph plane's content — not partial loss of +/// the unsynced tail, but the FULL set of already wal-v3-synced nodes. +/// Root cause: `save_graph_store` (`src/graph/recovery.rs`) wrote the +/// reference/floor (`graph_metadata.json`) 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. Fixed by reordering +/// `save_graph_store` to write the payload first, the floor last (atomic +/// via `atomic_write_durable`). Confirmed via `MOON_CRASH_MATRIX_RED=1 +/// MOON_CRASH_MATRIX_ITERS=20` soak: 20/20 clean at both shard counts +/// (pre-fix baseline: prod_s1 hit at iteration 11/20, prod_s4 at iteration +/// 7/20). See `tests/crash_matrix_cross_plane.rs`'s module doc for the +/// full writeup. pub fn mixed_mid_checkpoint(cfg: &Config) { assert!( cfg.disk_offload, @@ -801,6 +789,166 @@ pub fn mixed_mid_pass_c(cfg: &Config) { } } +/// `BGSAVE` + poll `INFO persistence` until `rdb_bgsave_in_progress:0` — +/// makes each checkpoint in [`graph_drop_survives_repeated_checkpoints`] an +/// awaited, deterministic step instead of a fire-and-forget async kick. +/// `SAVE_IN_PROGRESS` (`src/command/persistence.rs`) is a single +/// process-global atomic cleared only once every shard's local checkpoint +/// finishes, so polling it via any one connection is valid at every shard +/// count this suite runs, including `prod_s4`. +fn wait_for_bgsave(c: &mut Conn) { + let reply = c.cmd_s(&["BGSAVE"]); + assert!( + matches!(reply, crate::resp::Resp::Simple(_)) + || matches!(&reply, crate::resp::Resp::Error(e) + if e.to_lowercase().contains("already in progress")), + "BGSAVE must be accepted (or report already-in-progress), got {reply:?}" + ); + let deadline = std::time::Instant::now() + Duration::from_secs(20); + loop { + let info = c.cmd_s(&["INFO", "persistence"]).flat(); + if info.contains("rdb_bgsave_in_progress:0") { + return; + } + assert!( + std::time::Instant::now() < deadline, + "BGSAVE never completed within 20s (INFO persistence: {info})" + ); + std::thread::sleep(Duration::from_millis(50)); + } +} + +/// Task #53 review round 2 / P0-1 regression cell: a `GRAPH.DELETE`'d +/// graph must stay deleted across repeated checkpoints, even though the +/// delete leaves the graph map EMPTY (`graph_count() == 0`). +/// +/// Root cause this cell pins down: `persist_graph_at_checkpoint`'s old +/// short-circuit (`!is_dirty() || graph_count() == 0`) skipped +/// `save_graph_store` whenever a delete emptied the graph map — even +/// though the delete itself marks the store dirty via the WAL drain — so +/// `graph_metadata.json` kept describing the PRE-delete graph while later +/// checkpoints kept advancing `control.graph_floor_lsn` past the WAL +/// segment holding the DELETE record. Once that segment is recycled, a +/// crash + restart loads the stale metadata with nothing left in the WAL +/// to replay the delete: the dropped graph resurrects. Fixed by dropping +/// `graph_count() == 0` from the short-circuit — dirty alone gates it now; +/// `save_graph_store` on an empty graph store correctly no-ops its +/// per-graph loop while still rewriting `graph_metadata.json` to describe +/// zero graphs, durably advancing the floor past the delete. +/// +/// Tiny `--wal-segment-size`/`--max-wal-size` (pattern: `mixed_mid_pass_c`) +/// force real WAL segment rotation so the DELETE's record actually becomes +/// recycle-eligible within the test's lifetime; `--checkpoint-timeout +/// 3600` disables the periodic background checkpoint so every checkpoint +/// in this cell is an explicit, awaited `BGSAVE` (`wait_for_bgsave`) — +/// deterministic step count, no timer race with the kill point. +pub fn graph_drop_survives_repeated_checkpoints(cfg: &Config) { + assert!( + cfg.disk_offload, + "graph_drop_survives_repeated_checkpoints requires disk-offload enable" + ); + let dir = harness::unique_dir(&format!("graphdrop-{}", cfg.label)); + let extra = [ + "--wal-segment-size", + "32kb", + "--max-wal-size", + "96kb", + "--checkpoint-timeout", + "3600", + // Default is 30s (`--autovacuum-interval-secs`, `src/config.rs`) — + // far too slow for this cell to observe a Pass C tick. Ordinary + // checkpoint recycle (`recycle_segments_before`) respects a + // hardcoded 48 MiB `min_wal_bytes` floor with no CLI override, so + // it NEVER physically removes segments at this test's WAL volume + // regardless of how low the (buggy) floor computes — only Pass C's + // `recycle_aggressive` (which skips that floor check) can, so this + // cell is unreproducible without it. + "--autovacuum-interval-secs", + "1", + ]; + let (guard, port) = harness::spawn_moon_on(&dir, cfg, &extra); + let mut c = Conn::open(port); + let name = graph_name("graphdrop"); + + graph_create(&mut c, &name); + let handles: Vec = (0..6) + .map(|i| graph_addnode(&mut c, &name, "N", i)) + .collect(); + for i in 0..6 { + graph_addedge(&mut c, &name, handles[i], handles[(i + 1) % 6]); + } + + // Checkpoint 1: the graph exists — durably snapshots it. This is the + // pre-delete state `graph_metadata.json` must NOT still describe once + // the delete below has been through a later checkpoint. + wait_for_bgsave(&mut c); + + graph_delete(&mut c, &name); + + // Several more checkpoints under WAL-v3 traffic, each round padded + // enough to roll the tiny WAL ceiling and each checkpoint fully + // awaited. Padding must NOT (a) touch the SAME `GraphStore` in a way + // that leaves `graph_count() > 0` at a later checkpoint time, or + // (b) land on a plane `segment_plane_scan` treats as having no + // snapshot format (WorkspaceCreate/Drop, MQ, GraphTemporal, + // TemporalUpsert) — those block recycling of ANY segment that holds + // them, including a segment that also holds the DELETE record this + // cell targets. Two earlier designs were hand-verified against the + // pre-fix binary and BOTH failed to reproduce the bug for exactly + // these reasons before this one was found: + // 1. A single live padding graph: creating it un-empties + // `graph_count()`, so the very next checkpoint takes the NORMAL + // (non-buggy) path and correctly rewrites `graph_metadata.json` + // — which also correctly excludes the deleted graph, self-healing + // the exact precondition under test before any crash occurs. + // 2. MQ pushes: `segment_plane_scan` (`src/persistence/wal_v3/segment.rs`) + // blocks recycling of any segment holding an `MqPush` record, and + // that block covers the SAME segment holding the DELETE (pushes + // immediately follow it) — so the DELETE's segment survives + // autovacuum and WAL replay correctly re-applies the delete on + // restart despite the stale metadata, masking the bug. + // + // Fix: pad with THROWAWAY graphs that are created AND deleted within + // the same round, before that round's checkpoint — `graph_count()` is + // back to 0 by checkpoint time every round, so the buggy short-circuit + // keeps firing (never self-heals) while `GRAPH.ADDNODE`/`GRAPH.DELETE` + // are still real WAL-v3 `Command` records (unconditionally logged, + // unlike plain KV SETs which need `--wal-kv-log on` and still bypass + // wal-v3 on connection-local writes per the recovery warning text) — + // enough volume to roll the tiny segment ceiling. Same hash tag as + // `name` so the padding lands on the SAME shard at `prod_s4` (cross-tag + // padding would roll a sibling shard's WAL, not the one holding the + // DELETE). + let padding = "x".repeat(2048); + const ROUNDS: u64 = 4; + for round in 0..ROUNDS { + let pad_graph = format!("{{graphdrop}}pad{round}"); + graph_create(&mut c, &pad_graph); + for i in 0..80u64 { + graph_addnode_marker(&mut c, &pad_graph, &format!("{padding}{round}-{i}")); + } + graph_delete(&mut c, &pad_graph); + wait_for_bgsave(&mut c); + } + // Give the 1s autovacuum Pass C tick several chances to aggressively + // recycle (pattern: `mixed_mid_pass_c`) before the kill — this is what + // physically removes the WAL segment holding the DELETE record. + std::thread::sleep(Duration::from_secs(4)); + + harness::crash(guard, port); + + let (guard2, port2) = harness::spawn_moon_on(&dir, cfg, &extra); + let mut c2 = Conn::open(port2); + let names = graph_list_names(&mut c2); + assert!( + !names.contains(&name), + "cell {}: GRAPH.DELETE'd graph {name:?} resurrected after \ + {ROUNDS} post-delete checkpoints + kill-9 — GRAPH.LIST = {names:?}", + cfg.label, + ); + drop(guard2); +} + /// Genuinely-concurrent mid-burst kill: no synchronization at all. Proves /// only "no corruption" (every plane still answers well-formed queries) — /// see `mixed::start_concurrent_burst`'s doc for why no content assertion diff --git a/tests/crash_matrix_cross_plane/tests_prod.rs b/tests/crash_matrix_cross_plane/tests_prod.rs index 128202633..14bf8e6a6 100644 --- a/tests/crash_matrix_cross_plane/tests_prod.rs +++ b/tests/crash_matrix_cross_plane/tests_prod.rs @@ -21,26 +21,27 @@ const TXN_GRAPH_LEG_RED_REASON: &str = "cross-store TXN graph leg not durable, t the atomicity half (`cross_plane_*_txn_isolated_atomicity`) is a \ genuinely different, unrelated, GREEN claim and runs ungated."; -/// NEW finding (kernel M3, not a harness artifact — see -/// `scenarios::mixed_mid_checkpoint`'s doc for the full analysis): some -/// kill offsets in the post-`BGSAVE` window cause TOTAL loss of the graph -/// plane's already-wal-v3-synced content. PROBABILISTIC, not -/// deterministic — a single default run (`MOON_CRASH_MATRIX_ITERS=1`) has -/// a real chance of NOT hitting the window and reporting green even with -/// `MOON_CRASH_MATRIX_RED=1` set. Reproduce reliably with -/// `MOON_CRASH_MATRIX_RED=1 MOON_CRASH_MATRIX_ITERS=20` (confirmed hit -/// within 20 iterations on both shard counts during this stage's -/// investigation — prod_s1 at iteration 11/20, prod_s4 at iteration -/// 7/20). -const MID_CHECKPOINT_GRAPH_LOSS_RED_REASON: &str = "checkpoint-Finalize window can total-loss the graph plane, NEW. Some \ - kill offsets in the 0-150ms post-BGSAVE window lose the FULL synced \ - graph batch (not just the unsynced tail) while KV/vector/WS/MQ all \ - survive in the same run — reviewed and confirmed not a harness \ - artifact. PROBABILISTIC (~1-in-7 to 1-in-12 per MOON_CRASH_MATRIX_ITERS \ - sample in this stage's investigation) — reproduce reliably with \ - MOON_CRASH_MATRIX_RED=1 MOON_CRASH_MATRIX_ITERS=20, a single default \ - run may report green by chance. Strong P0 candidate for kernel M3 \ - stage 2 (K2). Tracked separately, not this stage's job to fix."; +/// FIXED (kernel M3 stage 2 / K2, task #53). Root cause: `save_graph_store` +/// (`src/graph/recovery.rs`) wrote the reference/floor +/// (`graph_metadata.json` via `store.save_metadata`) BEFORE the payload it +/// claims durable (CSR segments + `manifest.json`) — an ARIES-inverted +/// ordering. A kill-9 landing between the two writes left a +/// fully-advanced floor pointing at a payload that was never persisted, +/// so recovery trusted the floor and skipped WAL replay for records the +/// floor 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 +/// `persistence::atomic::atomic_write_durable`) so the floor write can +/// never itself be torn. Safe against double-replay because +/// `graph::replay::node_present` checks both `write_buf` and loaded CSR +/// segments before re-inserting a WAL-logged node/edge — replaying a +/// record whose payload actually made it to disk is a no-op, not a +/// duplicate. Confirmed via `MOON_CRASH_MATRIX_RED=1 +/// MOON_CRASH_MATRIX_ITERS=20` soak, 20/20 clean on both prod_s1 and +/// prod_s4 (pre-fix baseline: prod_s1 hit at iteration 11/20, prod_s4 at +/// iteration 7/20). See `scenarios::mixed_mid_checkpoint`'s doc for the +/// full scenario analysis. #[test] #[ignore] // Requires built release binary; run explicitly. @@ -106,12 +107,17 @@ fn cross_plane_prod_s1_mixed_all_planes_synced() { #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_prod_s1_mixed_all_planes_mid_checkpoint() { - if !harness::red_guard(MID_CHECKPOINT_GRAPH_LOSS_RED_REASON) { - return; - } scenarios::mixed_mid_checkpoint(&Config::PROD_S1); } +/// Task #53 review round 2 / P0-1 regression cell — see +/// `scenarios::graph_drop_survives_repeated_checkpoints`'s doc. +#[test] +#[ignore] // Requires built release binary; run explicitly. +fn cross_plane_prod_s1_graph_drop_survives_repeated_checkpoints() { + scenarios::graph_drop_survives_repeated_checkpoints(&Config::PROD_S1); +} + #[test] #[ignore] fn cross_plane_prod_s1_mixed_all_planes_concurrent_burst_no_corruption() { @@ -180,12 +186,17 @@ fn cross_plane_prod_s4_mixed_all_planes_synced() { #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_prod_s4_mixed_all_planes_mid_checkpoint() { - if !harness::red_guard(MID_CHECKPOINT_GRAPH_LOSS_RED_REASON) { - return; - } scenarios::mixed_mid_checkpoint(&Config::PROD_S4); } +/// Task #53 review round 2 / P0-1 regression cell — see +/// `scenarios::graph_drop_survives_repeated_checkpoints`'s doc. +#[test] +#[ignore] // Requires built release binary; run explicitly. +fn cross_plane_prod_s4_graph_drop_survives_repeated_checkpoints() { + scenarios::graph_drop_survives_repeated_checkpoints(&Config::PROD_S4); +} + #[test] #[ignore] fn cross_plane_prod_s4_mixed_all_planes_concurrent_burst_no_corruption() {