test(persistence): fix crash_recovery_disk_offload_no_aof floor-vs-ground-truth mismatch (task #44)#328
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? |
📝 WalkthroughWalkthroughThe crash-recovery test now issues filler data through one large ChangesCrash recovery durability test
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…ound-truth mismatch (task #44) crash_recovery_disk_offload_no_aof was the last red suite cited by PRODUCTION-CONTRACT.md row CRASH-01. Investigated whether it was harness rot (like task #31's WAL v2 probe rot) or a real durability defect (like task #60's replay no-op) with on-disk evidence before touching anything. Root cause: harness rot, not a data-loss regression. The test forced LRU eviction with a 16,000-key pipelined SET burst and asserted post-crash recovery >= 65% of PROBE_COUNT, assuming most evicted probes land durably in a heap-*.mpf file. That assumption predates disk_offload_spill_inert() / PR #273 (policy-aware eviction fail-close): under --appendonly no, the per-connection write-path eviction gate (run_write_eviction_gate, src/server/conn/handler_monoio/mod.rs) -- the path every ordinary SET/HSET past maxmemory actually takes -- has no ShardManifest handle and PLAIN-DROPS victims. This is documented Redis "pure cache, no durability" semantics (see docs/PRODUCTION-CONTRACT.md's CRASH-02 note on task #57, which made this drop-not-OOM behavior the intended fix, not a bug). Only the periodic memory-pressure tick (shard/persistence_tick.rs) durably spills under --appendonly no, and a busy connection's synchronous per-write eviction always wins the race before that tick ever observes a sustained over-budget shard. Confirmed via direct manifest/heap-*.mpf inspection: the old pipelined-SET burst durably spilled 0-1/200 probes, making the 65% floor permanently unreachable regardless of settle time or contention tuning. Fix (test-only, no src/ change): 1. write_filler now sends the filler as ONE MSET instead of 16,000 pipelined SETs. MSET's handler applies all pairs with no per-key eviction check, and the write-path gate that does run checks memory once, pre-MSET -- so a shard can end up far over budget with no further write pending to re-trigger the plain-drop path. The periodic tick is then the only thing left to reclaim it, durably spilling the LRU-oldest keys (the probes) via the manifest. 2. The fixed RECOVERY_FLOOR is replaced by count_durable_probe_entries, which reads each shard's manifest + Active KvLeaf DataFiles directly (the same way recover_shard_v3_pitr does at boot) right before the kill to establish ground truth, and asserts recovery returns AT LEAST that many probes -- the actual #22 regression signal, decoupled from eviction throughput. Verified the rewritten test still red-flags a reintroduced #22 regression: manually reverted the `persistence_dir.is_some() || disk_offload_base.is_some()` gate in main.rs, confirmed RED (ground truth 20, post 0), reverted back. Green 12x+ consecutive on macOS (monoio default + tokio runtime); no regression in crash_recovery_cold_del_resurrection or the crash_matrix_cross_plane no-durability-contract spot check (cross_plane_spot_offload_no_s1_no_durability_contract_liveness), which already independently documents the same drop-not-spill contract. fmt clean, clippy clean (default + tokio+jemalloc matrix). Updates CRASH-01's audit caveat in docs/PRODUCTION-CONTRACT.md and adds a CHANGELOG [Unreleased] entry. author: Tin Dang
TinDang97
force-pushed
the
worktree-agent-a2e8e7d3585b88734
branch
from
July 14, 2026 04:14
4b6812d to
7afedc4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@CHANGELOG.md`:
- Around line 24-25: Update the pipelined-SET burst spill-rate statement in
CHANGELOG.md from “0-1/200” to the evidence-supported “1-51/200” range, or
explicitly label the existing value as a particular run rather than a general
result.
In `@docs/PRODUCTION-CONTRACT.md`:
- Line 111: Update the CRASH-01 evidence description in the production contract
to state that durable probes come from each shard’s manifest and active KvLeaf
heap-*.mpf files, rather than claiming the manifest alone is the ground-truth
source. Use the same precise terminology established in the revision history
while preserving the existing test and audit details.
In `@tests/crash_recovery_disk_offload_no_aof.rs`:
- Around line 392-445: Update count_durable_probe_entries to return
Result<HashSet<Vec<u8>>> and propagate errors from shard discovery,
ShardManifest::open, and read_datafile instead of skipping unreadable artifacts.
Preserve the exact distinct probe:* key set, then update the referenced pre-kill
and post-restart assertions to compare every captured key individually, ensuring
no lost key can be masked by aggregate counts or another shard’s nonzero result.
🪄 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: 2130b604-7237-41d6-a9c0-34b327d20f2c
📒 Files selected for processing (3)
CHANGELOG.mddocs/PRODUCTION-CONTRACT.mdtests/crash_recovery_disk_offload_no_aof.rs
Comment on lines
+24
to
+25
| over-budget shard — so the pipelined-`SET` burst durably spilled 0-1/200 | ||
| probes (ground-truth-verified via direct manifest/`heap-*.mpf` inspection), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the reported spill-rate range.
The supplied test rationale reports 1-51/200 durably spilled probes across runs, but this changelog says 0-1/200 for the same pipelined-SET burst. Align the number with the recorded evidence or state that it was a particular run.
🤖 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 `@CHANGELOG.md` around lines 24 - 25, Update the pipelined-SET burst spill-rate
statement in CHANGELOG.md from “0-1/200” to the evidence-supported “1-51/200”
range, or explicitly label the existing value as a particular run rather than a
general result.
| |---|---|---|---|---| | ||
| | ✅ | WAL-01 | WAL v3 unified log, per-shard group commit, checksum-guarded records | `src/persistence/page.rs`, `kv_page.rs`, `clog.rs`, `manifest.rs`; `fuzz/fuzz_targets/wal_v3_record.rs` | GA | | ||
| | ✅ | CRASH-01 | Scripted crash-injection matrix (per-shard AOF, cold-tier DEL resurrection, disk-offload, graph, vacuum, vector durability) | `tests/crash_matrix_per_shard_aof.rs`, `crash_matrix_per_shard_bgrewriteaof.rs`, `crash_recovery_cold_del_resurrection.rs`, `crash_recovery_disk_offload_no_aof.rs`, `crash_recovery_graph_durability.rs`, `crash_recovery_vacuum.rs`, `crash_recovery_vector_durability.rs`; run by `.github/workflows/integration-tests.yml` jobs `Durability Tests` + `Crash Matrix (per-shard AOF)`. **2026-07-14 audit caveats:** the graph-durability suite's g1–g3 never actually ran until PRs #322/#324 (harness polled the deleted WAL v2 flat file AND the legacy-mode replay it guards was a silent no-op — both fixed, g1–g5 green 3× macOS / 5× Linux); `crash_recovery_disk_offload_no_aof` is currently red on main (task #44, open). | GA | | ||
| | ✅ | CRASH-01 | Scripted crash-injection matrix (per-shard AOF, cold-tier DEL resurrection, disk-offload, graph, vacuum, vector durability) | `tests/crash_matrix_per_shard_aof.rs`, `crash_matrix_per_shard_bgrewriteaof.rs`, `crash_recovery_cold_del_resurrection.rs`, `crash_recovery_disk_offload_no_aof.rs`, `crash_recovery_graph_durability.rs`, `crash_recovery_vacuum.rs`, `crash_recovery_vector_durability.rs`; run by `.github/workflows/integration-tests.yml` jobs `Durability Tests` + `Crash Matrix (per-shard AOF)`. **2026-07-14 audit caveats:** the graph-durability suite's g1–g3 never actually ran until PRs #322/#324 (harness polled the deleted WAL v2 flat file AND the legacy-mode replay it guards was a silent no-op — both fixed, g1–g5 green 3× macOS / 5× Linux); `crash_recovery_disk_offload_no_aof` was harness rot, not a data-loss regression — its 65%-eviction-throughput floor predated PR #273's intentional plain-drop write path and was structurally unreachable; fixed (task #44) by asserting against ground truth read directly from the manifest instead, green 10×+ macOS (monoio + tokio). | GA | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the complete ground-truth source.
The test derives durable probes from each shard’s manifest and active KvLeaf heap-*.mpf files, not from the manifest alone. Update this evidence description to match the implementation and the more precise wording already used in the revision history.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~111-~111: The official name of this software platform is spelled with a capital “H”.
Context: ..._recovery_vector_durability.rs; run by .github/workflows/integration-tests.ymljobs...
(GITHUB)
🤖 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 `@docs/PRODUCTION-CONTRACT.md` at line 111, Update the CRASH-01 evidence
description in the production contract to state that durable probes come from
each shard’s manifest and active KvLeaf heap-*.mpf files, rather than claiming
the manifest alone is the ground-truth source. Use the same precise terminology
established in the revision history while preserving the existing test and audit
details.
Comment on lines
+392
to
+445
| /// Ground truth (task #44): read each shard's `ShardManifest` + `Active` | ||
| /// `KvLeaf` `heap-*.mpf` DataFiles directly (best-effort, exactly the way | ||
| /// `persistence::recovery::recover_shard_v3_pitr` reads them at boot) and | ||
| /// count the DISTINCT `probe:*` keys that were durably on disk at the moment | ||
| /// this is called. Call it right before the SIGKILL — nothing else writes | ||
| /// after `SETTLE_BEFORE_KILL`, so this is a faithful snapshot of what the | ||
| /// crash lands on. Errors (a manifest/DataFile caught mid-write) are | ||
| /// swallowed and simply under-count — safe, since under-counting only makes | ||
| /// the recovery assertion easier to satisfy, never harder. | ||
| fn count_durable_probe_entries(dir: &std::path::Path) -> usize { | ||
| let off = dir.join("off"); | ||
| let mut probe_keys: HashSet<Vec<u8>> = HashSet::new(); | ||
| let Ok(shard_dirs) = std::fs::read_dir(&off) else { | ||
| return 0; | ||
| }; | ||
| for shard_entry in shard_dirs.flatten() { | ||
| let shard_dir = shard_entry.path(); | ||
| if !shard_dir.is_dir() { | ||
| continue; | ||
| } | ||
| let Some(shard_name) = shard_dir.file_name().and_then(|n| n.to_str()) else { | ||
| continue; | ||
| }; | ||
| let manifest_path = shard_dir.join(format!("{shard_name}.manifest")); | ||
| if !manifest_path.exists() { | ||
| continue; | ||
| } | ||
| let Ok(manifest) = ShardManifest::open(&manifest_path) else { | ||
| continue; | ||
| }; | ||
| let data_dir = shard_dir.join("data"); | ||
| for file_entry in manifest.files() { | ||
| if file_entry.status != FileStatus::Active | ||
| || file_entry.file_type != PageType::KvLeaf as u8 | ||
| { | ||
| continue; | ||
| } | ||
| let heap_path = data_dir.join(format!("heap-{:06}.mpf", file_entry.file_id)); | ||
| let Ok(pages) = read_datafile(&heap_path) else { | ||
| continue; | ||
| }; | ||
| for page in &pages { | ||
| for slot_idx in 0..page.slot_count() { | ||
| if let Some(kv_entry) = page.get(slot_idx) | ||
| && kv_entry.key.starts_with(b"probe:") | ||
| { | ||
| probe_keys.insert(kv_entry.key); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| probe_keys.len() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the exact durable-key set and fail closed on scan errors.
Collapsing ground truth to a count and silently skipping unreadable active artifacts can produce false passes: a different probe recovered later can mask a lost measured probe, while another shard satisfies the nonzero check.
Return Result<HashSet<Vec<u8>>>, propagate manifest/datafile errors, and verify every captured key individually after restart.
Also applies to: 490-496, 539-552
🤖 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_disk_offload_no_aof.rs` around lines 392 - 445, Update
count_durable_probe_entries to return Result<HashSet<Vec<u8>>> and propagate
errors from shard discovery, ShardManifest::open, and read_datafile instead of
skipping unreadable artifacts. Preserve the exact distinct probe:* key set, then
update the referenced pre-kill and post-restart assertions to compare every
captured key individually, ensuring no lost key can be masked by aggregate
counts or another shard’s nonzero result.
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
Verdict: harness rot, not a data-loss regression. The test's
post >= 65% of PROBE_COUNTfloor assumed evicted probes durably spill under--appendonly no— but since PR #273 (task #57, policy-aware fail-close) the per-write eviction gate intentionally plain-drops victims in that config (documented cache semantics; boot-timedisk_offload_spill_inertWARN). Ground-truth inspection showed only 0–1/200 probes durably spilled → the floor was structurally unreachable.Test-only fix:
write_fillersends oneMSETburst (no per-write plain-drop race) so the manifest-backed periodic memory-pressure tick is the reclaimer.count_durable_probe_entries— reads each shard's manifest + ActiveKvLeafheap files (same mechanism recovery uses) right before the kill; asserts recovery ≥ that ground truth, with adurable_probes > 0sanity guard so the test can't pass vacuously.docs/PRODUCTION-CONTRACT.mdCRASH-01 caveat updated (task P0-6 follow-up: extend bounded LZ4 helper to non-FPI sites #44 closed) + revision-history row.Verification
persistence_dir.is_some() || disk_offload_base.is_some()recovery gate → test fails (ground truth 20, post 0); reverted.crash_recovery_cold_del_resurrection2/2, cross-plane spot cell for the same drop-not-spill contract passes.src/change.Summary by CodeRabbit
Bug Fixes
Documentation