diff --git a/CHANGELOG.md b/CHANGELOG.md index af83d953..ba69418a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — legacy-mode (`--disk-offload disable`) graph WAL replay silently dropped the entire graph plane on kill-9 restart (task #60) + +`replay_graph_wal` (`src/shard/shared_databases.rs`, the legacy-mode-only +replay path taken when `persistence_dir` is set but disk-offload is +disabled) passed the raw RESP-encoded `WalRecord::payload` directly as the +`cmd` argument to `CommandReplayEngine::replay_command`, instead of parsing +it into frames and passing the bare command name + `&[Frame]` args as the +sibling `replay_graph_wal_v3` (disk-offload-enabled path) and +`recovery.rs`'s KV `Command` replay both already did. +`GraphReplayCollector::is_graph_command` compares against literal names +like `b"GRAPH.CREATE"`, so it never matched the multi-line RESP blob — +`graph_command_count()` stayed 0, the `replay_graph_commands` guard never +fired, and every graph mutation was silently lost on restart. Regression +from PR #236 (WAL v2 → v3 port). Un-gates 6 previously-RED crash-matrix +cells (`tests/crash_matrix_cross_plane/{tests_legacy.rs,tests_spot.rs}`): +legacy s1 `graph_isolated`, `txn_isolated_committed`, +`txn_isolated_atomicity`, `mixed_all_planes_synced`, +`mixed_all_planes_mid_pass_c`, and spot-check +`cross_plane_spot_legacy_s4_graph_isolated`. + ### Added — allocator-overhead and PageCache observability in INFO memory / Prometheus (task #58) `INFO memory` and `/metrics` previously had no way to see the gap between diff --git a/src/shard/shared_databases.rs b/src/shard/shared_databases.rs index 78d91b89..b6467595 100644 --- a/src/shard/shared_databases.rs +++ b/src/shard/shared_databases.rs @@ -1042,8 +1042,32 @@ pub fn replay_graph_wal( let mut dummy_dbs: Vec = (0..db_count).map(|_| Database::new()).collect(); let mut selected_db = 0usize; let on_command = &mut |record: &WalRecord| { - if record.record_type == WalRecordType::Command { - engine.replay_command(&mut dummy_dbs, &record.payload, &[], &mut selected_db); + if record.record_type != WalRecordType::Command { + return; + } + // Payload is RESP-encoded (same format as AOF/WAL v2 blocks) — it + // must be parsed into frames before dispatch. `replay_command` + // expects a bare command name plus already-parsed `&[Frame]` + // args, never the raw multi-line RESP blob (mirrors + // `replay_graph_wal_v3` above and `recovery.rs`'s KV Command + // replay; passing the raw payload as `cmd` made + // `GraphReplayCollector::is_graph_command` never match, silently + // dropping the entire legacy-mode graph plane on restart). + let mut buf = bytes::BytesMut::from(&record.payload[..]); + let parse_cfg = crate::protocol::ParseConfig::default(); + while let Ok(Some(frame)) = crate::protocol::parse::parse(&mut buf, &parse_cfg) { + let crate::protocol::Frame::Array(ref arr) = frame else { + continue; + }; + if arr.is_empty() { + continue; + } + let cmd_name: &[u8] = match &arr[0] { + crate::protocol::Frame::BulkString(s) => s.as_ref(), + crate::protocol::Frame::SimpleString(s) => s.as_ref(), + _ => continue, + }; + engine.replay_command(&mut dummy_dbs, cmd_name, &arr[1..], &mut selected_db); } }; let on_fpi = &mut |_record: &WalRecord| {}; diff --git a/tests/crash_matrix_cross_plane/tests_legacy.rs b/tests/crash_matrix_cross_plane/tests_legacy.rs index 2f922bed..07e68dfe 100644 --- a/tests/crash_matrix_cross_plane/tests_legacy.rs +++ b/tests/crash_matrix_cross_plane/tests_legacy.rs @@ -1,28 +1,22 @@ //! Legacy cells (`--disk-offload disable`), shards=1 only per the brief //! ("legacy mode has no cross-shard novelty for this suite's purpose"). //! -//! 5 cells here are RED, all one root cause: PR #288's own changelog states -//! graph command replay from WAL v3 does not reconstruct the graph in -//! legacy mode even with zero Pass C interference — a pre-existing gap -//! unrelated to WAL recycling, deliberately not asserted by -//! `tests/crash_recovery_wal_recycle_legacy.rs` for the same reason. This -//! suite DOES assert it (the shared `scenarios::graph_isolated` / -//! `mixed_synced` / `mixed_mid_pass_c` / `txn_isolated_committed` / -//! `txn_isolated_atomicity` bodies make no exceptions), so each surfaces the -//! same "ERR graph not found" — `harness::red_guard` skips them by default -//! (set `MOON_CRASH_MATRIX_RED=1` to reproduce); see that function's doc for -//! why a plain `#[ignore = "RED: ..."]` annotation does NOT achieve this -//! (`--ignored` runs ignored tests). +//! GREEN as of task #60: the 5 cells below were RED because +//! `replay_graph_wal` (`src/shard/shared_databases.rs`, legacy-mode-only — +//! the disk-offload path uses the separate `replay_graph_wal_v3`) passed +//! the raw RESP-encoded `WalRecord::payload` blob directly as the `cmd` +//! argument to `CommandReplayEngine::replay_command`, instead of parsing it +//! into frames and passing the bare command name + `&[Frame]` args first +//! (the pattern `replay_graph_wal_v3` and `recovery.rs`'s KV Command replay +//! both already used). `GraphReplayCollector::is_graph_command` compares +//! against literal names like `b"GRAPH.CREATE"`, so it never matched a +//! multi-line RESP blob — `graph_command_count()` stayed 0 and the entire +//! graph plane was silently dropped on restart. Fixed by adding the same +//! parse-then-dispatch step; cells un-gated (no more `harness::red_guard`). -use crate::harness::{self, Config}; +use crate::harness::Config; use crate::scenarios; -const LEGACY_GRAPH_RED_REASON: &str = "legacy-mode (--disk-offload disable) graph WAL replay \ - does not reconstruct the graph even with zero Pass C interference — \ - PR #288 / task #43 changelog; tests/crash_recovery_wal_recycle_legacy.rs \ - deliberately does not assert this either. Pre-existing gap, not this \ - stage's job to fix — tracked separately."; - #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_legacy_yes_s1_kv_isolated() { @@ -32,9 +26,6 @@ fn cross_plane_legacy_yes_s1_kv_isolated() { #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_legacy_yes_s1_graph_isolated() { - if !harness::red_guard(LEGACY_GRAPH_RED_REASON) { - return; - } scenarios::graph_isolated(&Config::LEGACY_YES_S1); } @@ -56,49 +47,33 @@ fn cross_plane_legacy_yes_s1_mq_isolated() { scenarios::mq_isolated(&Config::LEGACY_YES_S1); } -// One root cause (legacy-mode graph WAL replay never reconstructs the -// graph — same gap as `cross_plane_legacy_yes_s1_graph_isolated`) surfaces -// in the 4 cells below too: each hits the same "ERR graph not found" the -// isolated graph scenario does. This includes BOTH txn_isolated halves -// (review round 3, P1 restructuring, task #52) — unlike prod_s1/prod_s4, -// the atomicity half is NOT independently green here: `graph_create` itself -// never survives a restart in legacy mode, so the atomicity scenario's -// graph-leg assertion panics on "ERR graph not found" before it can ever -// evaluate the atomicity claim. Confirmed on the VM, not assumed. +// The same root cause (formerly: legacy-mode graph WAL replay never +// reconstructed the graph — see the module doc) covered the 4 cells below +// too, including BOTH txn_isolated halves (review round 3, P1 +// restructuring, task #52). Fixed alongside `graph_isolated` by task #60; +// all now GREEN and un-gated. #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_legacy_yes_s1_txn_isolated_committed() { - if !harness::red_guard(LEGACY_GRAPH_RED_REASON) { - return; - } scenarios::txn_isolated_committed(&Config::LEGACY_YES_S1); } #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_legacy_yes_s1_txn_isolated_atomicity() { - if !harness::red_guard(LEGACY_GRAPH_RED_REASON) { - return; - } scenarios::txn_isolated_atomicity(&Config::LEGACY_YES_S1); } #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_legacy_yes_s1_mixed_all_planes_synced() { - if !harness::red_guard(LEGACY_GRAPH_RED_REASON) { - return; - } scenarios::mixed_synced(&Config::LEGACY_YES_S1); } #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_legacy_yes_s1_mixed_all_planes_mid_pass_c() { - if !harness::red_guard(LEGACY_GRAPH_RED_REASON) { - return; - } scenarios::mixed_mid_pass_c(&Config::LEGACY_YES_S1); } diff --git a/tests/crash_matrix_cross_plane/tests_spot.rs b/tests/crash_matrix_cross_plane/tests_spot.rs index 6922a410..5fe447df 100644 --- a/tests/crash_matrix_cross_plane/tests_spot.rs +++ b/tests/crash_matrix_cross_plane/tests_spot.rs @@ -2,7 +2,7 @@ //! production/legacy set (brief: "spot-check the remaining cells with 1-2 //! representative scenarios each"). -use crate::harness::{self, Config}; +use crate::harness::Config; use crate::scenarios; // --------------------------------------------------------------------------- @@ -27,25 +27,15 @@ fn cross_plane_spot_offload_no_s4_no_durability_contract_liveness() { // --------------------------------------------------------------------------- // Legacy mode at shards=4 (excluded from the full matrix — "legacy mode has // no cross-shard novelty" — but the brief still wants 1-2 representative -// scenarios). graph_isolated here reproduces the SAME RED gap as -// legacy_yes_s1 (the bug is legacy-mode WAL replay, not shard-count-specific; -// spot-checking at shards=4 confirms the gap is not accidentally -// shard-count-dependent). Gated by `harness::red_guard` — see that -// function's doc for why `#[ignore = "RED: ..."]` alone does not skip it. +// scenarios). graph_isolated here used to reproduce the SAME RED gap as +// legacy_yes_s1 (the bug was legacy-mode WAL replay, not shard-count- +// specific). GREEN as of task #60 (see `tests_legacy.rs` module doc for the +// root cause); confirms the fix is not accidentally shard-count-dependent. // --------------------------------------------------------------------------- #[test] #[ignore] // Requires built release binary; run explicitly. fn cross_plane_spot_legacy_s4_graph_isolated() { - if !harness::red_guard( - "legacy-mode (--disk-offload disable) graph WAL replay does not \ - reconstruct the graph — same gap as \ - cross_plane_legacy_yes_s1_graph_isolated, confirmed shard-count- \ - independent (PR #288 / task #43 changelog). Not this stage's job \ - to fix — tracked separately.", - ) { - return; - } scenarios::graph_isolated(&Config::SPOT_LEGACY_S4); }