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
July 10, 2026 01:34
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>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 14, 2026
…sed] + resolve PR #TBD placeholders (#330) The v0.6.0 tag (355f68d, 2026-07-10) absorbed several PRs that merged after the v0.6.0 release PR itself but before the tag was cut. Their changelog entries were left under [Unreleased], understating what shipped in the tagged v0.6.0 binary. Audit (task #65, v0.7.0 roll-up prep) identified 23 sections whose PR merge commits are verified ancestors of v0.6.0 via `git merge-base --is-ancestor`: PR #248 (FastScan SIMD, SQ8 default, TQ ADC L2 fix, EF_RUNTIME FT.CONFIG, RERANK_MULT+EXACT_BEAM, CLI/moon.conf tuning defaults), PR #250 (13 production-hardening sections), PR #251 (COLD-segment reload off event loop), PR #254 (roadmap docs suite), PR #255 (CI macOS 30m), PR #256 (v0.6.0 ledger closure). Moved verbatim to a new subsection at the top of [0.6.0], with a note explaining the absorption. Sections from PR #252, #253, and #257-#263 were verified NOT ancestors of v0.6.0 and correctly remain in [Unreleased]. Also resolves the "PR #TBD" placeholders left by those sections (now #248, #257, #261) now that the real PR numbers are known. Section-count invariant holds (306 before, 306 after); content unchanged except for the PR-number substitutions. author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 16, 2026
…les, ~129× fewer heap files (v0.8 item 3) (#350) * perf(storage): coalesce oversized entries into shared spill batch files v0.8 roadmap item 3 / task #57 follow-up: disk-offload spill files must batch effectively so file count scales as ~keys/batch, not ~keys. The G2 acceptance run (260K x 10KB keys, 256MB cap) produced ~236K heap-*.mpf files even though the format nominally supports batching (<=256 keys per file). Root cause: SpillThread::flush_buffer already coalesced up to 256 requests per flush, but partitioned them by value size before writing — every entry over INLINE_MAX_VALUE_BYTES (3500B, e.g. all of G2's 10KB values) was routed to its own dedicated single-entry file via spill_single_entry, defeating the batch entirely. build_kv_spill_batch / write_kv_spill_batch (src/storage/tiered/kv_spill.rs) are rewritten to pack inline AND oversized entries into ONE shared batch file per flush: each oversized entry gets a dedicated leaf page (holding an overflow pointer) plus its overflow-page chain placed inline in the same page stream, via a new BatchSlot enum (Leaf | Overflow) replacing the old two-Vec (leaves, overflow) BatchPages layout. The atomic temp-file+sync_all+rename+fsync_directory write sequence is unchanged, so the payload-before-reference durability ordering (a key is only marked cold after its containing segment is durable) is preserved. flush_buffer (src/storage/tiered/spill_thread.rs) no longer partitions the buffer by size — it builds one SpillEntry list and calls build_kv_spill_batch once per flush, producing one SpillCompletion (or falling back to per-entry salvage only on a build/write error). Fixing this uncovered a latent bug in build_overflow_chain (src/persistence/kv_page.rs): its prev/next link computation silently assumed every chain's start_page_id == 1 (true of every caller before this change, since a single-entry file always put its one leaf at page 0) — a chain-local i+1/i+2 formula that produced dangling/wrong links the moment a caller (this new batching code) passed a variable start_page_id > 1 for the second, third, etc. oversized entry sharing a file. Links are now computed as file-absolute (start_page_id + i), matching what read_overflow_chain already expected. Caught by the new test_rebuild_from_manifest_oversized_batch_roundtrip test before it could ship. Empirical measurement (macOS, matching G2's shape): RED (pre-fix) 3742 files / ~34,908 evicted 10KB keys; GREEN (post-fix) 29 files for the same eviction load — a ~129x reduction, spill_batches_flushed now matches file count 1:1 as designed. New tests: - kv_spill.rs::tests: test_build_kv_spill_batch_oversized_entries_share_one_file, test_build_kv_spill_batch_mixed_inline_and_oversized, test_rebuild_from_manifest_oversized_batch_roundtrip - tests/crash_recovery_spill_batch_kill9.rs: end-to-end kill-9-mid-spill regression guard. Drives a real server through a sustained eviction+spill burst of 4000 x 8000-byte (overflow-chain) values under --appendonly yes --appendfsync always, SIGKILLs it with no settle window (maximizing the chance of a torn trailing batch — either an unrenamed heap-*.tmp or a just-renamed file whose manifest commit raced the kill), then restarts and asserts zero acknowledged-write loss, byte- exact content, a batched (<= keys/64) heap-file count before the kill, and no lingering .tmp files after the post-restart orphan sweep settles. Verified green on both the monoio (default) and tokio (--features runtime-tokio,jemalloc) runtimes. Note: an earlier version of this test restarted round 2 with the same small --maxmemory as round 1 and saw non-deterministic scattered loss (68-533 keys across repeated runs) on BOTH this fix and the pre-fix binary (A/B'd by hand via git stash) — root-caused to AOF replay re-inserting the full dataset hot with no eviction gate, then racing the verification reads' cold-promotions against the periodic post-restart eviction tick. That is a distinct, pre-existing gap orthogonal to spill batching (adjacent to the in-flight used_memory-accounting fix in a sibling worktree) and out of scope here; round 2 now restarts with a generous --maxmemory to remove that confound and isolate the contract actually in scope. Preserved invariants (verified via the full existing kv_spill/spill_thread test suite plus the crash test above): next_spill_file_id_seed uniqueness across orphans, crash-orphan sweep classification, FLUSH/DEL tombstone + cold-plane replay (PR #257), and eviction fail-close semantics (#273) are all unaffected — none of that logic was touched. Gates: cargo fmt --check, cargo clippy -- -D warnings (default features and --no-default-features --features runtime-tokio,jemalloc, both including --tests), full cargo test --lib (4333 passed), targeted kv_spill/spill_thread/kv_page unit tests, the new crash_recovery_spill_batch_kill9 integration test (3x stable on monoio, 1x green on tokio), and the pre-existing crash_recovery_disk_offload_no_aof regression test — all green. Not in scope (deferred, flagged for follow-up): the synchronous per-key SpillContext eviction path (timers::run_eviction, used when the async memory-pressure cascade is not active) still spills one file per victim. The empirical repro matching G2's shape (--appendonly yes) confirmed the async batching path above is dominant and fully fixes the reported regression; the sync path is lower-traffic and was left alone given the scoped time budget. author: Tin Dang <tindang.ht97@gmail.com> * fix(storage): close adversarial-review gaps on spill-segment-batching Adversarial review of perf/spill-segment-batching (SHIP-WITH-FIXES) flagged three gaps in the spill-file-batching rewrite before merge: 1. Test gap: the existing kill-9 test only proves AOF-replay-derived recovery (every acked key ends up hot via DispatchReplayEngine, never touching cold_read_through), so it could not tell "the leaf-offset + overflow-chain-with-start_page_id>1 read path works" apart from "AOF replay papered over a broken cold layout". Added spill_batch_shared_file_survives_cold_read_after_kill9 (tests/crash_recovery_spill_batch_kill9.rs): pre-seeds a 3-entry shared batch file (1 inline + 2 oversized, ordered so the second oversized entry's overflow chain starts at file-absolute page_idx > 1) directly via build_kv_spill_batch/write_kv_spill_batch + ShardManifest, boots a real --appendonly no server on top of it, kills it without ever touching the keys, restarts, and GETs all three -- the only way any of these bytes reach the client is a genuine ColdIndex::rebuild_from_manifest + cold_read_through read. Getting this test to actually exercise that path (rather than a hot-RAM hit) required pulling in src/persistence/recovery.rs's fix from the concurrent task #56 effort (fix/t56-used-memory-offload, commit 689c52e): v3 recovery Phase 3 used to re-insert every previously-spilled String entry directly into the hot DashTable in ADDITION to rebuilding the ColdIndex stub for the same key -- for an entry_flags::OVERFLOW entry this hot-preload loop ignored the flag and inserted the raw 4-byte page-pointer as if it were the literal value, silently corrupting every oversized cold key on every restart (a pre-existing bug, independent of this batching fix -- it affected the old single-entry-per-file layout too). That recovery.rs hunk is a clean, self-contained cherry-pick (verified: 689c52e's parent has an IDENTICAL recovery.rs to this branch's pre-fix state, and the hunk does not touch any of task #56's other, out-of-scope-here, used_memory ledger/RSS/AOF-replay-demotion changes) and is a hard prerequisite for this test's premise -- there is no way to route around it from this side. A companion library-level unit test, test_rebuild_from_manifest_mixed_inline_and_oversized_roundtrip (kv_spill.rs), independently proves the exact same read path correct via direct calls to the production functions, without depending on server boot sequencing. 2. Memory-pressure characteristic: build_kv_spill_batch materialized an entire flush's pages in RAM before write_kv_spill_batch touched disk -- fine for the old ~50KB/256-small-entries case, but this fix lets oversized entries share that same unconditional batch, so 256 consistently-large (e.g. near-maxmemory-sized) values could resident hundreds of MB at once, exactly when spills fire under memory pressure. flush_buffer (spill_thread.rs) now splits each FLUSH_ENTRY_CAP-sized buffer into sub-batches capped at 4 MiB of cumulative value_bytes (chosen against the OLD per-entry path's peak -- one value's pages at a time, no cross-entry amplification -- while leaving the G2 acceptance shape, 256 x ~10KB = ~2.5 MiB, as a single file, unchanged), each becoming its own file via that sub-batch's own already-pre-assigned file_id. New unit test: oversized_flush_splits_into_byte_capped_sub_batches. 3. Hygiene: fixed the stale FLUSH_ENTRY_CAP comment (no longer claims to bound in-RAM size by itself -- superseded by BATCH_BYTES_CAP) and the stale entry_flags::OVERFLOW doc (corrected from a fictitious 12-byte file_id:u64+page_id:u32 layout to the actual 4-byte file-absolute start_page_idx -- no file_id is stored, the chain always lives in the same physical file as its leaf stub). Added a comment at write_kv_spill_batch explaining why it hand-rolls temp+fsync+rename instead of calling persistence::atomic::atomic_write_durable: that helper takes one pre-materialized &[u8], and concatenating batch.pages into a single buffer first would undo BATCH_BYTES_CAP's whole point. Gates (macOS, this worktree): - cargo fmt --check: clean - cargo clippy --tests -- -D warnings (default features): clean - cargo clippy --no-default-features --features runtime-tokio,jemalloc --tests -- -D warnings: clean - cargo test --release --lib (default features): 4335 passed, 0 failed, 1 ignored - cargo test --no-default-features --features runtime-tokio,jemalloc --release --lib: 3518 passed, 0 failed, 1 ignored - cargo test --release --lib storage::tiered::kv_spill::: 16 passed - cargo test --release --lib persistence::kv_page::: 23 passed - cargo test --release --lib persistence::recovery::: 15 passed - cargo test --release --lib storage::tiered::spill_thread::: 13 passed (incl. new byte-cap-split test) - cargo test --release --test crash_recovery_spill_batch_kill9 -- --ignored (monoio, default features): 2 passed (spill_batch_shared_file_survives_cold_read_after_kill9 re-run 3x clean before this final pass) - cargo test --no-default-features --features runtime-tokio,jemalloc --release --test crash_recovery_spill_batch_kill9 -- --ignored (tokio, MOON_NO_URING=1): 2 passed author: Tin Dang <tindang.ht97@gmail.com> --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
5 tasks
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