test(persistence): G1 cross-plane kill-9 crash matrix — 37 cells, 4 RED root-cause groups (kernel M3 stage 1)#298
Merged
Conversation
… / G1)
Adds a RED-first kill-9 crash-recovery tripwire suite covering every
persistence plane moon ships: KV/AOF, KV disk-offload, graph/vector/WS/MQ
WAL v3 effect records, cross-store MULTI/EXEC, mixed-plane concurrent
workloads under load, and a checkpoint-Finalize-window kill. 37 cells run
across {shards=1, shards=4} x {appendonly, disk-offload} — the config the
kernel M3 brief calls out as "the config that matters most" gets full
plane x workload coverage (tests_prod.rs); the rest of the matrix gets
targeted spot/legacy/seeded-RED coverage.
27 cells are GREEN by default; 10 are known-RED and gated behind a runtime
guard (harness::red_guard, MOON_CRASH_MATRIX_RED=1) rather than
#[ignore = "RED: ..."] — the suite's own --ignored invocation convention
(every cell needs a release binary) makes an ignore-reason string alone a
non-functional gate, since --ignored explicitly RUNS ignored tests. A
MOON_CRASH_MATRIX_ITERS env var (default 1) supports ad-hoc soak runs for
probabilistic findings.
Two real findings surfaced during harness development and are deliberately
left RED (fixing them is out of scope for this stage — the RED harness
itself is the deliverable):
- Cross-store TXN graph leg not durable (task #52): a committed MULTI/EXEC
mixing SET + GRAPH.ADDNODE survives kill-9 on the KV leg but loses the
graph leg. Deterministic 3x-consecutive repro at both shard counts via
--checkpoint-timeout 3600 + kill immediately post-sync-wait.
- Checkpoint-Finalize window can total-loss the graph plane (new): some
kill offsets in the 0-150ms post-BGSAVE window lose the entire synced
graph batch while KV/vector/WS/MQ all survive. Probabilistic
(~1-in-7 to 1-in-12/iteration) - needs MOON_CRASH_MATRIX_ITERS=20 to
reproduce reliably; a single default run can false-green. Strong P0
candidate for kernel M3 stage 2 (K2).
Also confirmed GREEN (not RED): WS DROP durability under kill-9 -
WorkspaceDrop WAL records replay correctly in order, narrowing the brief's
"MQ/WS resurrection" gap to MQ generic-DEL only (still RED, same bug class
as the already-fixed KV/vector cold-plane resurrection, PR #257).
tests/common/mod.rs: adds find_moon_binary()/sigkill()/wait_for_port_down()
shared helpers the new suite depends on.
Test-only; no production code changed in this stage.
author: Tin Dang
…d granularity (kernel M3 stage 1) Adversarial review of the G1 cross-plane crash-matrix suite (previous commit) found two must-fix defects before it could ship as a trustworthy tripwire, plus several cheap hardening items. P0 (empirically proven false-GREEN): kv_spilled_isolated's filler (3000x512B against a 4 MiB --maxmemory cap) never actually forced a cold-tier spill at either shard count - reproduced on moon-dev with reclamation_cold_segments:0 and zero heap-*.mpf files. The cell silently degenerated to ordinary AOF-replay coverage, already proven by kv_isolated. Fixed by mirroring crash_recovery_cold_del_resurrection.rs's proven filler ratios (16,000x600B against an 8 MiB cap - shard-count independent under --maxmemory's per-shard division) plus a hard count_heap_mpf_files precondition assert, so a future load-parameter regression fails loud instead of vacuously passing. Confirmed on the VM: both shard counts now genuinely spill and pass. P1 (guard granularity): txn_isolated gated both of its rounds behind one red_guard, but the "transaction queued and killed before EXEC must apply nothing" atomicity claim has nothing to do with task #52's graph-leg durability finding and is GREEN on prod_s1/prod_s4 - sharing the guard hid a working, unrelated regression tripwire by default. Split into txn_isolated_committed (still RED, task #52) and txn_isolated_atomicity (ungated on prod_s1/prod_s4; still RED on legacy_yes_s1, folded into the same pre-existing legacy-graph-reconstruction root cause as every other legacy graph cell there - confirmed on the VM, not assumed). The initial split missed that the atomicity round's own GRAPH.CREATE was never sync-waited before the kill, so its own precondition raced the WAL flush tick and failed identically on every config regardless of durability semantics - fixed by sync-waiting GRAPH.CREATE before queuing the never-committed transaction; the kill point itself is unweakened. P2s: wait_for_port_down now panics on loop exhaustion instead of silently returning (no silent-pass verification helpers in a tripwire codebase); BenignDisconnectPanicFilter documents its --test-threads=1 requirement; is_unexpected_plane_error switched from substring to exact error-string matching (a corruption message that happened to start with "unknown index" could otherwise misclassify as benign); module doc's legacy-family cell count fixed from a self-contradictory "4 cells" / "5 cells" to a consistent 6 (now includes the new atomicity RED cell); CHANGELOG/module-doc phrasing corrected to say tests/common's shared helpers were added but migrating the 5 existing crash_recovery_*.rs suites' own local copies is deferred, not done; graph_isolated gained a cheap VALID_AT positive control (a never-invalidated node must still be visible at a far-future VALID_AT) so the existing negative check cannot pass vacuously if VALID_AT plumbing itself broke. Net: 37 -> 40 cells (txn_isolated's split adds 3: 2 new GREEN atomicity cells on prod_s1/prod_s4, 1 new RED atomicity cell on legacy_yes_s1). 27 -> 29 GREEN by default, 10 -> 11 RED (gated). Full default run: 40 passed, 0 failed. RED=1 run: 31 passed, 9 failed (the 2 probabilistic checkpoint-Finalize cells did not trigger in this single-iteration sample, as documented). 3x-consecutive re-verification on the VM: kv_spilled_isolated passes with a genuine spill at both shard counts; txn_isolated_atomicity is GREEN 3x on prod_s1/prod_s4 and RED 3x on legacy_yes_s1. Test-only; no production code changed in this stage. author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a feature-gated kill-9 crash/recovery matrix covering multiple persistence planes, mixed workloads, durability markers, no-durability configurations, seeded regressions, and legacy, production, and spot configurations. ChangesCross-plane crash recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TestScenario
participant MoonServer
participant DurabilityFiles
participant RestartedServer
TestScenario->>MoonServer: start configured instance
TestScenario->>MoonServer: execute plane or mixed workload
MoonServer->>DurabilityFiles: write WAL/AOF markers
TestScenario->>DurabilityFiles: poll for durability boundary
TestScenario->>MoonServer: SIGKILL
TestScenario->>RestartedServer: restart with test directory
TestScenario->>RestartedServer: verify recovered state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/crash_matrix_cross_plane/planes.rs (1)
108-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reply-parsing between
graph_query_idsandgraph_query_ids_valid_at.The two functions differ only in the extra
VALID_AT <ms>argument; the entire nestedArray → rows → cells → Int/Bulkdecode is copy-pasted. Extracting a shared parser removes ~25 duplicated lines and avoids the two copies drifting apart if the row-shape decoding ever changes.♻️ Sketch: shared parser
+fn parse_graph_query_ids(reply: Resp, cypher: &str, ctx: &str) -> BTreeSet<i64> { + match reply { + Resp::Array(Some(outer)) => match outer.get(1) { + Some(Resp::Array(Some(rows))) => rows + .iter() + .map(|r| match r { + Resp::Array(Some(cells)) => match cells.first() { + Some(Resp::Int(i)) => *i, + Some(Resp::Bulk(Some(b))) => { + String::from_utf8_lossy(b).parse().expect("int cell") + } + other => panic!("unexpected cell: {other:?}"), + }, + other => panic!("malformed row: {other:?}"), + }) + .collect(), + other => panic!("malformed query reply, rows slot = {other:?}"), + }, + Resp::Error(e) => panic!("query {cypher:?} {ctx}errored: {e}"), + other => panic!("malformed query reply: {other:?}"), + } +}Then both helpers become thin wrappers around
c.cmd_s(...)+parse_graph_query_ids(...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/crash_matrix_cross_plane/planes.rs` around lines 108 - 163, Extract the shared nested response decoding from graph_query_ids and graph_query_ids_valid_at into a dedicated parse_graph_query_ids helper. Keep the existing Array/rows/cells Int-or-Bulk handling and malformed-response panics in that parser, then have both query functions call it after their respective c.cmd_s invocations while retaining their distinct query error context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/common/mod.rs`:
- Around line 171-188: Update both cfg-specific sigkill implementations into one
platform-independent helper that calls Child::kill() and then waits via
Child::wait(). Remove the raw libc::kill invocation, unsafe block, Unix
conditional compilation, and unused pid handling.
In `@tests/crash_matrix_cross_plane/scenarios.rs`:
- Around line 881-885: Replace the vacuous as_int_or_zero(... ) >= 0 assertion
in the MQ validation block with a direct Resp check, reusing the nearby
is_unexpected_plane_error helper if appropriate. Ensure an unexpected XLEN error
fails the assertion while allowing a missing queue’s Int(0) response.
---
Nitpick comments:
In `@tests/crash_matrix_cross_plane/planes.rs`:
- Around line 108-163: Extract the shared nested response decoding from
graph_query_ids and graph_query_ids_valid_at into a dedicated
parse_graph_query_ids helper. Keep the existing Array/rows/cells Int-or-Bulk
handling and malformed-response panics in that parser, then have both query
functions call it after their respective c.cmd_s invocations while retaining
their distinct query error context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a0efd16-bf68-460b-af24-b33b6b920fc0
📒 Files selected for processing (12)
CHANGELOG.mdtests/common/mod.rstests/crash_matrix_cross_plane.rstests/crash_matrix_cross_plane/harness.rstests/crash_matrix_cross_plane/mixed.rstests/crash_matrix_cross_plane/planes.rstests/crash_matrix_cross_plane/resp.rstests/crash_matrix_cross_plane/scenarios.rstests/crash_matrix_cross_plane/tests_legacy.rstests/crash_matrix_cross_plane/tests_prod.rstests/crash_matrix_cross_plane/tests_seeded_red.rstests/crash_matrix_cross_plane/tests_spot.rs
…us MQ burst assertion tests/common::sigkill: Child::kill() is documented to send SIGKILL on Unix, so the raw libc::kill unsafe block and the unix/non-unix cfg split were unnecessary — one safe implementation, same semantics. concurrent_burst MQ check: `as_int_or_zero(XLEN) >= 0` was vacuous — error replies mapped to 0 and the assertion could never fail, silently passing a genuinely corrupted MQ plane. Now asserts the reply is not an unexpected plane error (XLEN on a never-durable queue replies Int(0), so no benign not-found allowlist is needed). Removed the now-unused as_int_or_zero. author: Tin Dang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
New
tests/crash_matrix_cross_plane.rs+tests/crash_matrix_cross_plane/suite: kernel M3 stage 1 (G1) — a RED-first kill-9 crash-recovery tripwire covering every persistence plane moon ships (KV/AOF, KV disk-offload, graph/vector/WS/MQ WAL v3, cross-storeMULTI/EXEC, mixed-plane concurrent workloads, and a checkpoint-Finalize-window kill) across{shards=1, shards=4} x {appendonly, disk-offload}. Spec:.planning/reviews/kernel-m3-brief-2026-07-12.md§Stage 1.This stage does not fix anything found — the RED harness itself is the deliverable. Two coordinator review rounds hardened the suite (root cause vs. harness bug triage, a broken
#[ignore]-based RED gate, a vacuous kv-spill precondition, a guard-granularity bug) before this could be trusted as a tripwire.Cell inventory — 40 cells, 29 GREEN by default, 11 RED (gated)
RED cells are gated by
harness::red_guard()(MOON_CRASH_MATRIX_RED=1), not#[ignore = "RED: ..."]— the suite's own--ignoredinvocation convention runs ignored tests, so an ignore-reason string alone is not a functional gate.MOON_CRASH_MATRIX_ITERS(default 1) enables ad-hoc soaks for probabilistic findings.4 RED root-cause groups:
cross_plane_legacy_yes_s1_graph_isolated,_txn_isolated_committed,_txn_isolated_atomicity,_mixed_all_planes_synced,_mixed_all_planes_mid_pass_c, pluscross_plane_spot_legacy_s4_graph_isolated. PR fix(shard): gate autovacuum Pass C WAL recycle on checkpoint-backed mode (task #43, P1) #288: graph WAL-v3 replay never reconstructs the graph in legacy (--disk-offload disable) mode — pre-existing, not this stage's job to fix.cross_plane_seeded_red_mq_generic_del_resurrection. Same bug class as the already-fixed KV/vector cold-plane resurrection (PR fix(persistence): replayed DEL/FLUSH tombstone the cold plane; spills no longer suppress AOF recovery #257), not yet applied to MQ. The WS half of this brief §1.4 finding turned out GREEN —WorkspaceDropWAL records replay correctly in order.cross_plane_prod_s1_txn_isolated_committed,_s4_txn_isolated_committed. A committedMULTI/EXECmixingSET+GRAPH.ADDNODEsurvives kill-9 on the KV leg (PR fix(server): sharded MULTI/EXEC safety, durability & owner-routing (Phase A+B) #247'spersist_txn_aof) but loses the graph leg. Deterministic 3x-consecutive repro at both shard counts (--checkpoint-timeout 3600, kill immediately post-sync-wait). Real P1 finding, flagging for its own task.cross_plane_prod_s1_mixed_all_planes_mid_checkpoint,_s4_.... Some kill offsets in the 0-150ms post-BGSAVEwindow lose the entire already-synced graph batch while KV/vector/WS/MQ all survive. Probabilistic (~1-in-7 to 1-in-12/iteration) — a single default run can false-green even with the guard on; reproduce reliably withMOON_CRASH_MATRIX_RED=1 MOON_CRASH_MATRIX_ITERS=20. Strong P0 candidate for kernel M3 stage 2 (K2), flagging for its own task.Review round: SHIP-WITH-FIXES → addressed
kv_spilled_isolated's filler (3000×512B/4MiB) never forced a real cold-tier spill — fixed by mirroringcrash_recovery_cold_del_resurrection.rs's proven ratios (16,000×600B/8MiB) plus a hardheap-*.mpfprecondition assert. Confirmed genuinely spilling on the VM at both shard counts.txn_isolatedsplit intotxn_isolated_committed(RED, task P2-3: recovery time is O(files × phases) — index manifest entries once #52) andtxn_isolated_atomicity(a queued-but-never-committed transaction must apply nothing) — the atomicity claim is unrelated and GREEN onprod_s1/prod_s4; now runs ungated there (still RED onlegacy_yes_s1, same root cause as group 1).wait_for_port_downpanics on loop exhaustion instead of silently returning;BenignDisconnectPanicFilterdocuments its--test-threads=1requirement;is_unexpected_plane_errorswitched substring → exact-match; module-doc cell-count self-contradiction fixed; CHANGELOG/doc phrasing corrected (shared helpers added, migration of existing suites' local copies deferred);graph_isolatedgained a VALID_AT positive control.Gates (all green)
cargo fmt --check,cargo clippy -- -D warnings(default + tokio/jemalloc matrices, repo-wide and scoped to this test target) — all clean.cargo check --no-default-features --features runtime-tokio,jemalloc --test crash_matrix_cross_plane— compiles both runtimes.orb run -m moon-dev) default green-only run:test result: ok. 40 passed; 0 failed.MOON_CRASH_MATRIX_RED=1run:test result: FAILED. 31 passed; 9 failed(2 probabilistic checkpoint-Finalize cells did not trigger this single-iteration sample, as documented).kv_spilled_isolatedpasses with a genuine spill at both shard counts;txn_isolated_atomicityGREEN 3x onprod_s1/prod_s4, RED 3x onlegacy_yes_s1;txn_isolated_committedRED 3x at both shard counts.Test-only; no production code changed in this stage.
Test plan
cargo fmt --checkcargo clippy -- -D warnings(default features)cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warningscargo check --no-default-features --features runtime-tokio,jemalloc --test crash_matrix_cross_planeMOON_CRASH_MATRIX_RED=1VM run — 31 passed, 9 failed (matches documented RED inventory)Summary by CodeRabbit