fix(persistence): replayed DEL/FLUSH tombstone the cold plane; spills no longer suppress AOF recovery#257
Merged
Conversation
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? |
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughPreserves cold-tier wiring (cold_shard_dir, cold_index) across AOF/RDB replay in main.rs and shard_replay.rs, attaches recovered cold index directly to databases[0] in recovery.rs Phase 3, and changes the legacy AOF fallback gate to depend on KV command counts rather than aggregate counts. Adds a crash-recovery integration test and changelog entries. ChangesCrash-recovery cold-tier and AOF fallback fixes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
🤖 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/crash_recovery_cold_del_resurrection.rs`:
- Around line 143-164: `wait_for_port_down` currently exits silently after all
retry polls, so it should mirror `wait_for_port` by panicking when the port
never transitions to down. Update the `wait_for_port_down` function to keep its
current polling behavior, but add a terminal panic after the loop that clearly
reports the port stayed up and includes the `port` value, so callers like
`start_moon_alive` fail fast with a direct error.
🪄 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: 6c2d1db3-94d7-46c4-adc1-bef6581b80df
📒 Files selected for processing (6)
CHANGELOG.mdsrc/main.rssrc/persistence/aof_manifest/shard_replay.rssrc/persistence/recovery.rssrc/shard/mod.rstests/crash_recovery_cold_del_resurrection.rs
Comment on lines
+143
to
+164
| fn wait_for_port_down(port: u16) { | ||
| let addr = format!("127.0.0.1:{}", port); | ||
| let mut consecutive_refused = 0; | ||
| for _ in 0..120 { | ||
| match std::net::TcpStream::connect_timeout( | ||
| &addr.parse().expect("addr"), | ||
| Duration::from_millis(100), | ||
| ) { | ||
| Ok(_) => { | ||
| consecutive_refused = 0; | ||
| std::thread::sleep(Duration::from_millis(100)); | ||
| } | ||
| Err(_) => { | ||
| consecutive_refused += 1; | ||
| if consecutive_refused >= 2 { | ||
| return; | ||
| } | ||
| std::thread::sleep(Duration::from_millis(50)); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
wait_for_port_down should panic on timeout.
Unlike wait_for_port (line 138), this function silently returns after exhausting all 120 polls without confirming the port is down. If the port never goes down, start_moon_alive will burn through 6 retry attempts (~48s) before panicking with a less clear message. Add a terminal panic matching the pattern in wait_for_port.
Proposed fix
std::thread::sleep(Duration::from_millis(50));
}
}
}
+ panic!("moon port {} still up after {} polls", port, 120);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn wait_for_port_down(port: u16) { | |
| let addr = format!("127.0.0.1:{}", port); | |
| let mut consecutive_refused = 0; | |
| for _ in 0..120 { | |
| match std::net::TcpStream::connect_timeout( | |
| &addr.parse().expect("addr"), | |
| Duration::from_millis(100), | |
| ) { | |
| Ok(_) => { | |
| consecutive_refused = 0; | |
| std::thread::sleep(Duration::from_millis(100)); | |
| } | |
| Err(_) => { | |
| consecutive_refused += 1; | |
| if consecutive_refused >= 2 { | |
| return; | |
| } | |
| std::thread::sleep(Duration::from_millis(50)); | |
| } | |
| } | |
| } | |
| } | |
| fn wait_for_port_down(port: u16) { | |
| let addr = format!("127.0.0.1:{}", port); | |
| let mut consecutive_refused = 0; | |
| for _ in 0..120 { | |
| match std::net::TcpStream::connect_timeout( | |
| &addr.parse().expect("addr"), | |
| Duration::from_millis(100), | |
| ) { | |
| Ok(_) => { | |
| consecutive_refused = 0; | |
| std::thread::sleep(Duration::from_millis(100)); | |
| } | |
| Err(_) => { | |
| consecutive_refused += 1; | |
| if consecutive_refused >= 2 { | |
| return; | |
| } | |
| std::thread::sleep(Duration::from_millis(50)); | |
| } | |
| } | |
| } | |
| panic!("moon port {} still up after {} polls", port, 120); | |
| } |
🤖 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_recovery_cold_del_resurrection.rs` around lines 143 - 164,
`wait_for_port_down` currently exits silently after all retry polls, so it
should mirror `wait_for_port` by panicking when the port never transitions to
down. Update the `wait_for_port_down` function to keep its current polling
behavior, but add a terminal panic after the loop that clearly reports the port
stayed up and includes the `port` value, so callers like `start_moon_alive` fail
fast with a direct error.
…ills no longer suppress AOF recovery
Two crash-recovery bugs in the disk-offload two-plane design (hot
DashTable + cold-spill .mpf files), both in the "logically deleted but
manifest-Active" window before the cold orphan sweep:
1. Deleted/flushed cold keys resurrected after kill-9. The live DEL
path tombstones the in-memory ColdIndex, but a crash loses that
tombstone and boot-time replay could not re-apply it — every replay
path ran against databases whose cold_index was None, making
remove_counting_cold()/clear() silent cold-plane no-ops:
- recover_shard_v3_pitr built the ColdIndex in Phase 3 but only
stashed it on RecoveryResult; Phase 4 WAL replay never saw it.
It is now attached to databases[0] BEFORE Phase 4 (and before the
Phase 4b AOF fallback); shard/mod.rs backfills the other dbs.
- The per-shard / multi-part AOF replay (main.rs) take()'d the cold
wiring off every database before the replay block and re-attached
it only after. The wiring is now re-attached after the pre-replay
hot wipe and BEFORE replay, and replay_per_shard/replay_multi_part
bridge it across rdb::load's wholesale database swap (which
silently dropped it — the original B-2 bug, now fixed at the seam
where it belongs).
Measured pre-fix: 85–97/200 deleted probes resurrected via cold
read-through (shards=4, appendonly=yes, disk-offload).
2. Any spill silently discarded the AOF on the next restart. The
Phase 4b "WAL replayed 0 commands -> fall back to the AOF" gate
counted vector + file-lifecycle records, so a single FileCreate
record from a disk-offload spill made the WAL look non-empty and
skipped appendonly.aof — the only complete KV history, since
--wal-kv-log is auto-off when the AOF is the authority. The gate now
keys on KV Command records only.
New tests:
- tests/crash_recovery_cold_del_resurrection.rs: e2e kill-9 suite
(DEL + FLUSHALL scenarios) inside the pre-sweep window; red/green
verified against pre-fix and fixed binaries. Harness disables the
diskfull guard (--disk-free-min-pct 0: the guard write-flags
DEL/FLUSHALL by design, and near-full dev machines silently gutted
the test — redis-cli exits 0 on MOONERR replies) and asserts
server-side deletion took effect before the crash.
- recovery.rs unit test: a FileCreate lifecycle record must not
suppress the AOF fallback.
author: Tin Dang <tindang.ht97@gmail.com>
TinDang97
force-pushed
the
fix/cold-plane-replay-resurrection
branch
from
a4c2a61 to
afefe15
Compare
TinDang97
added a commit
that referenced
this pull request
Jul 10, 2026
Under `--disk-offload enable` + `--appendonly no`, the memory-pressure eviction path queued a key's SpillRequest to the background SpillThread and immediately dropped the hot value from RAM -- before the pwrite, fsync, or manifest commit had happened. Under `--appendonly yes` this ordering is safe (a crash in that window is covered by AOF replay of the original write), but with no AOF backstop the value existed nowhere -- not in RAM, not yet durable on disk, no WAL/AOF record -- for the entire SpillThread batching window. A kill -9 in that window was silent, unrecoverable data loss. `try_evict_if_needed_async_spill_with_total_budget` (src/storage/ eviction.rs) now branches on `--appendonly`. With an AOF backstop the existing fast fire-and-forget path is unchanged. Without one, a new `evict_batch_durable_no_aof` collects up to 256 victims without removing them from RAM, spills the whole batch durably in one synchronous call to `spill_thread::flush_buffer` (pwrite + fsync + manifest commit, now `pub(crate)` for reuse -- batching is required, not optional: an earlier draft did one fsync per key and stalled the event loop under sustained pressure), and only then removes from RAM the keys whose file both pwrite-succeeded and manifest-committed, immediately populating ColdIndex. `db.remove()` is called before the ColdIndex insert, never after -- Database::remove's own cold-tier tombstone cleanup would otherwise silently wipe the insert back out (the #257 DEL/cold-tier resurrection class); this ordering bug was caught by the test suite during development, not by inspection. The manifest is threaded through as `Option<&mut ShardManifest>`; only the tick-driven memory-pressure cascade (handle_memory_pressure) has one to give. The four other call sites (inline per-connection write-path gate, sharded/monoio handlers, spsc_handler, the Lua scripting bridge) have no manifest reachable, so under `--appendonly no` they now bail -- retain the hot value, surface OOM -- instead of risking the same crash window; the next 100ms tick reclaims the memory via the durable path. This does mean those inline sites no longer evict opportunistically under `--appendonly no` (that unconditional eviction regardless of durability was the bug), so a write burst that crosses maxmemory now converges to budget only as fast as the 100ms tick reclaims it. Verified live via the shard's own estimated_memory() accounting (not RSS, which includes allocator/SSO overhead the eviction budget doesn't model) that this converges correctly and stays converged -- not a stall or a leak, just a smaller, safer per-tick eviction volume than the old unconditional fast path. A second, independent bug in the same file: evict_one_with_spill (the legacy fully-synchronous path used when no SpillThread exists) logged a warning on spill I/O failure but still evicted the key -- data loss on every spill failure regardless of appendonly or crash. Now returns without evicting on failure, matching the async path's fail-closed contract. Regression coverage (src/storage/eviction.rs tests): - sync_spill_failure_retains_hot_value_no_silent_drop - async_spill_no_aof_backstop_durably_spills_before_drop (locks the pwrite+fsync+manifest-commit-before-drop ordering, immediate ColdIndex read-through, and a DEL-after-spill check guarding against the #257 resurrection class) - async_spill_no_aof_backstop_no_manifest_retains_hot_value - async_spill_no_aof_backstop_spill_failure_retains_hot_value Verification: cargo test --release --lib storage::eviction:: (30/30 passed), cargo clippy -- -D warnings (clean), cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings (clean), cargo fmt --check (clean), cargo check --no-default-features --features runtime-tokio,jemalloc (clean). author: Tin Dang <tindang.ht97@gmail.com>
TinDang97
added a commit
that referenced
this pull request
Jul 10, 2026
Under `--disk-offload enable` + `--appendonly no`, the memory-pressure eviction path queued a key's SpillRequest to the background SpillThread and immediately dropped the hot value from RAM -- before the pwrite, fsync, or manifest commit had happened. Under `--appendonly yes` this ordering is safe (a crash in that window is covered by AOF replay of the original write), but with no AOF backstop the value existed nowhere -- not in RAM, not yet durable on disk, no WAL/AOF record -- for the entire SpillThread batching window. A kill -9 in that window was silent, unrecoverable data loss. `try_evict_if_needed_async_spill_with_total_budget` (src/storage/ eviction.rs) now branches on `--appendonly`. With an AOF backstop the existing fast fire-and-forget path is unchanged. Without one, a new `evict_batch_durable_no_aof` collects up to 256 victims without removing them from RAM, spills the whole batch durably in one synchronous call to `spill_thread::flush_buffer` (pwrite + fsync + manifest commit, now `pub(crate)` for reuse -- batching is required, not optional: an earlier draft did one fsync per key and stalled the event loop under sustained pressure), and only then removes from RAM the keys whose file both pwrite-succeeded and manifest-committed, immediately populating ColdIndex. `db.remove()` is called before the ColdIndex insert, never after -- Database::remove's own cold-tier tombstone cleanup would otherwise silently wipe the insert back out (the #257 DEL/cold-tier resurrection class); this ordering bug was caught by the test suite during development, not by inspection. The manifest is threaded through as `Option<&mut ShardManifest>`; only the tick-driven memory-pressure cascade (handle_memory_pressure) has one to give. The four other call sites (inline per-connection write-path gate, sharded/monoio handlers, spsc_handler, the Lua scripting bridge) have no manifest reachable, so under `--appendonly no` they now bail -- retain the hot value, surface OOM -- instead of risking the same crash window; the next 100ms tick reclaims the memory via the durable path. This does mean those inline sites no longer evict opportunistically under `--appendonly no` (that unconditional eviction regardless of durability was the bug), so a write burst that crosses maxmemory now converges to budget only as fast as the 100ms tick reclaims it. Verified live via the shard's own estimated_memory() accounting (not RSS, which includes allocator/SSO overhead the eviction budget doesn't model) that this converges correctly and stays converged -- not a stall or a leak, just a smaller, safer per-tick eviction volume than the old unconditional fast path. A second, independent bug in the same file: evict_one_with_spill (the legacy fully-synchronous path used when no SpillThread exists) logged a warning on spill I/O failure but still evicted the key -- data loss on every spill failure regardless of appendonly or crash. Now returns without evicting on failure, matching the async path's fail-closed contract. Regression coverage (src/storage/eviction.rs tests): - sync_spill_failure_retains_hot_value_no_silent_drop - async_spill_no_aof_backstop_durably_spills_before_drop (locks the pwrite+fsync+manifest-commit-before-drop ordering, immediate ColdIndex read-through, and a DEL-after-spill check guarding against the #257 resurrection class) - async_spill_no_aof_backstop_no_manifest_retains_hot_value - async_spill_no_aof_backstop_spill_failure_retains_hot_value Verification: cargo test --release --lib storage::eviction:: (30/30 passed), cargo clippy -- -D warnings (clean), cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings (clean), cargo fmt --check (clean), cargo check --no-default-features --features runtime-tokio,jemalloc (clean). author: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 10, 2026
Under `--disk-offload enable` + `--appendonly no`, the memory-pressure eviction path queued a key's SpillRequest to the background SpillThread and immediately dropped the hot value from RAM -- before the pwrite, fsync, or manifest commit had happened. Under `--appendonly yes` this ordering is safe (a crash in that window is covered by AOF replay of the original write), but with no AOF backstop the value existed nowhere -- not in RAM, not yet durable on disk, no WAL/AOF record -- for the entire SpillThread batching window. A kill -9 in that window was silent, unrecoverable data loss. `try_evict_if_needed_async_spill_with_total_budget` (src/storage/ eviction.rs) now branches on `--appendonly`. With an AOF backstop the existing fast fire-and-forget path is unchanged. Without one, a new `evict_batch_durable_no_aof` collects up to 256 victims without removing them from RAM, spills the whole batch durably in one synchronous call to `spill_thread::flush_buffer` (pwrite + fsync + manifest commit, now `pub(crate)` for reuse -- batching is required, not optional: an earlier draft did one fsync per key and stalled the event loop under sustained pressure), and only then removes from RAM the keys whose file both pwrite-succeeded and manifest-committed, immediately populating ColdIndex. `db.remove()` is called before the ColdIndex insert, never after -- Database::remove's own cold-tier tombstone cleanup would otherwise silently wipe the insert back out (the #257 DEL/cold-tier resurrection class); this ordering bug was caught by the test suite during development, not by inspection. The manifest is threaded through as `Option<&mut ShardManifest>`; only the tick-driven memory-pressure cascade (handle_memory_pressure) has one to give. The four other call sites (inline per-connection write-path gate, sharded/monoio handlers, spsc_handler, the Lua scripting bridge) have no manifest reachable, so under `--appendonly no` they now bail -- retain the hot value, surface OOM -- instead of risking the same crash window; the next 100ms tick reclaims the memory via the durable path. This does mean those inline sites no longer evict opportunistically under `--appendonly no` (that unconditional eviction regardless of durability was the bug), so a write burst that crosses maxmemory now converges to budget only as fast as the 100ms tick reclaims it. Verified live via the shard's own estimated_memory() accounting (not RSS, which includes allocator/SSO overhead the eviction budget doesn't model) that this converges correctly and stays converged -- not a stall or a leak, just a smaller, safer per-tick eviction volume than the old unconditional fast path. A second, independent bug in the same file: evict_one_with_spill (the legacy fully-synchronous path used when no SpillThread exists) logged a warning on spill I/O failure but still evicted the key -- data loss on every spill failure regardless of appendonly or crash. Now returns without evicting on failure, matching the async path's fail-closed contract. Regression coverage (src/storage/eviction.rs tests): - sync_spill_failure_retains_hot_value_no_silent_drop - async_spill_no_aof_backstop_durably_spills_before_drop (locks the pwrite+fsync+manifest-commit-before-drop ordering, immediate ColdIndex read-through, and a DEL-after-spill check guarding against the #257 resurrection class) - async_spill_no_aof_backstop_no_manifest_retains_hot_value - async_spill_no_aof_backstop_spill_failure_retains_hot_value Verification: cargo test --release --lib storage::eviction:: (30/30 passed), cargo clippy -- -D warnings (clean), cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings (clean), cargo fmt --check (clean), cargo check --no-default-features --features runtime-tokio,jemalloc (clean). author: Tin Dang <tindang.ht97@gmail.com> Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
7 tasks
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 12, 2026
…ED root-cause groups (kernel M3 stage 1) (#298) * test(shard): cross-plane kill-9 crash-matrix suite (kernel M3 stage 1 / 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 * fix(shard): G1 crash-matrix review round 3 — vacuous spill fix + guard 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 * test(review): CodeRabbit round — safe Child::kill sigkill + non-vacuous 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 --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 13, 2026
…LUSHDB no longer resurrect on kill-9 (task #46) (#301) Root cause: MQ durable streams live as ordinary keys in the shard keyspace, but replay_mq_wal had no way to represent "this queue was deleted after these pushes". A generic DEL/UNLINK/FLUSHDB/FLUSHALL removed the stream from the live db and the DurableQueueRegistry, but replay unconditionally reapplies every MqCreate/MqPush/... record on boot regardless of that delete — a kill-9 after the delete resurrected the full pre-delete content on the next restart. Same bug class as the already-fixed KV/vector cold-plane resurrection (PR #257), now closed for MQ. Reproduced via the crash-matrix RED cell cross_plane_seeded_red_mq_generic_del_resurrection (kernel M3 brief §1.4), now un-gated (no more harness::red_guard) and green 3/3 consecutive runs, plus a full 44/44 default-GREEN crash-matrix pass. Fix: a new MqDrop WAL v3 record (discriminant 0x75) is emitted whenever a durable MQ stream is removed via generic DEL/UNLINK/FLUSHDB/FLUSHALL. Layout matches the other versioned MQ records: [version:u8][db_index:u32] [key_len:u32][key:N], fails closed on any malformed/future-version payload. New hooks mq_exec::auto_drop_mq_streams / auto_drop_mq_streams_on_flush are wired into every connection-layer write path that already runs the equivalent vector/text index-parity hooks (handler_monoio/mod.rs, handler_sharded/mod.rs, the sharded MULTI/EXEC helper in server/conn/shared.rs) plus the replica-side apply_index_parity_hooks in replication/apply.rs — a replica must tombstone its OWN WAL too, or it resurrects the stream on its own restart even though its live copy stayed correctly deleted via normal command replication. Boot-time replay applies apply_mq_drop (shared_databases.rs) strictly in WAL order alongside every other MQ record, so a Drop only kills records that PRECEDE it for that key — a later MqCreate/MqPush for the same key survives intact (create -> drop -> create round-trips a kill-9 with the second incarnation whole; proven by both a unit test and the new crash-matrix cell cross_plane_mq_create_drop_create_survives). segment_plane_scan's plane-history block set gained MqDrop alongside the other MQ discriminants so autovacuum/recycle never deletes a sealed segment still holding an unfloored tombstone. The MQ WAL fuzz target now also fuzzes decode_mq_drop. Replication decision: MQ effect records replicate live only at num_shards == 1 (the pre-existing gate shared with MqCreate/etc). MqDrop follows the same posture — MQ._REPL.DROP is emitted and applied by replication::apply::apply_mq exactly like the other MQ replay commands. Separately, a replicated generic DEL/UNLINK/FLUSHDB/FLUSHALL already removes the stream from the replica's live keyspace via normal command replication regardless of that gate; what it did not do before this fix is tombstone the replica's own wal-v3 MQ plane, which apply_index_parity_hooks now closes. Scope decision (documented in code): DurableQueueRegistry entries are NOT db-indexed (pre-existing limitation, same as MqCreate's registry) — a key match tombstones regardless of which db the deleting command ran in, and FLUSHDB drops every registered durable queue exactly like FLUSHALL since the registry cannot scope to one db. Two different dbs sharing an MQ queue NAME is not a supported configuration. Test-gotcha fixed along the way: the crash-matrix DEL/FLUSHALL scenarios' original sync-marker strategy waited on an AOF-family write to prove the delete was durable — correct pre-fix (DEL only touched the AOF), but MqDrop lands on the wal-v3 MQ plane via a separate fire-and-forget channel drained on its own 1ms tick, so an AOF-only marker no longer proves the tombstone itself reached disk. Both tests now also sync a throwaway durable queue's MQ.PUSH (wal-v3-family) after the delete/flush before crashing. Files touched: - src/mq/wal.rs — encode_mq_drop/decode_mq_drop + MQ_REPL_DROP + is_mq_replay_command update + module docs + unit tests - src/persistence/wal_v3/record.rs — WalRecordType::MqDrop = 0x75 - src/persistence/wal_v3/replay.rs — MqDrop routed through on_command - src/persistence/wal_v3/segment.rs — MqDrop added to plane-history block set - src/shard/shared_databases.rs — apply_mq_drop + replay dispatch wiring + MqReplayStats.drop + 4 new unit tests - src/shard/mq_exec.rs — auto_drop_mq_streams / auto_drop_mq_streams_on_flush / emit_mq_drops - src/server/conn/handler_monoio/mod.rs, src/server/conn/handler_sharded/mod.rs, src/server/conn/shared.rs — hook call sites (DEL/UNLINK + FLUSHDB/FLUSHALL) - src/replication/apply.rs — MQ_REPL_DROP apply arm + apply_index_parity_hooks replica-side tombstone - fuzz/fuzz_targets/mq_wal_record.rs — fuzz decode_mq_drop - tests/crash_matrix_cross_plane/tests_seeded_red.rs — un-gated the RED cell, fixed its wal-v3-family sync race, added a FLUSHALL sibling and a create->drop->create ordering test - CHANGELOG.md — new [Unreleased] entry + removed stale #46 caveats author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
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
Fixes two crash-recovery bugs in the disk-offload two-plane design (hot DashTable + cold-spill
.mpffiles), both living in the "logically deleted but manifest-Active" window before the cold orphan sweep. Closes the DEL-replay→cold-tombstone verification item from the two-plane storage analysis.1. Deleted/flushed cold keys resurrected after kill-9
The live DEL path tombstones the in-memory ColdIndex, but the manifest entry stays Active until the orphan sweep — a crash inside that window loses the tombstone, and boot-time replay could not re-apply it: every replay path ran against databases whose
cold_indexwasNone, makingremove_counting_cold()/clear()silent cold-plane no-ops.recover_shard_v3_pitrbuilt the ColdIndex in Phase 3 but only stashed it onRecoveryResult— Phase 4 WAL replay never saw it. Now attached todatabases[0]before Phase 4 (and the Phase 4b AOF fallback).main.rs)take()'d cold wiring off every database before the replay block and re-attached it only after. Now re-attached after the pre-replay hot wipe and before replay;replay_per_shard/replay_multi_partbridge it acrossrdb::load's wholesale database swap.Measured pre-fix: 85–97/200 deleted probes resurrected via cold read-through (shards=4, appendonly=yes, disk-offload).
2. Any spill silently discarded the AOF on the next restart
The Phase 4b "WAL replayed 0 commands → fall back to the AOF" gate counted vector + file-lifecycle records, so a single spill
FileCreaterecord made the WAL look non-empty and skippedappendonly.aof— the only complete KV history (--wal-kv-logis auto-off when the AOF is the authority). The gate now keys on KVCommandrecords only.Tests (red/green TDD)
tests/crash_recovery_cold_del_resurrection.rs(DEL + FLUSHALL scenarios, crash inside the pre-sweep window). RED on pre-fix binary (85–97/200 resurrections), GREEN 2/2 with the fix, verified in both orders.test_spill_lifecycle_records_do_not_suppress_aof_fallback).--disk-free-min-pct 0(the diskfull guard write-flags DEL/FLUSHALL by design and near-full dev machines silently gutted the test — redis-cli exits 0 on MOONERR replies) + pre-crash assertion that deletion took effect server-side.Verification
cargo test --release --test crash_recovery_cold_del_resurrection -- --ignored→ 2/2cargo test --release --lib persistence::recovery→ 16/16cargo fmt --check,cargo clippy -- -D warnings(default + tokio,jemalloc) → cleanFollow-up (documented in-code)
When the Phase-4 WAL carries some KV records (
--wal-kv-log on/CDC) alongside an AOF, the gate keeps the WAL's partial view and skips the AOF (replaying both would double-apply non-idempotent commands). Needs an AOF-first Phase-4 redesign — tracked in the roadmap.Summary by CodeRabbit
Bug Fixes
Tests