fix(shard): gate autovacuum Pass C WAL recycle on checkpoint-backed mode (task #43, P1)#288
Merged
Merged
Conversation
…de WAL history (task #43) Adds tests/crash_recovery_wal_recycle_legacy.rs, a live kill-9 + restart integration test proving that autovacuum's Pass C (`src/shard/autovacuum.rs`) deletes WAL v3 segments that are the SOLE durable copy of graph/workspace history when disk-offload is disabled (legacy mode). Pass C calls `WalWriterV3::recycle_aggressive(redo_lsn = wal.current_lsn())` whenever total WAL bytes exceed `--max-wal-size`, with no gate on whether a checkpoint protocol backs that floor. In legacy mode there is no periodic plane snapshot: `save_graph_store` only runs at graceful shutdown, and the workspace/MQ/temporal registries have no snapshot format at all — they replay purely from the WAL. Once the ceiling is breached, every sealed segment is deleted regardless of whether its records survive anywhere else. The test forces the condition deterministically: tiny --wal-segment-size / --max-wal-size plus a fast --autovacuum-interval-secs make Pass C run within seconds. It asserts two things: (1) byte-level survival — the earliest WorkspaceCreate and graph-node WAL records are still present on disk after Pass C has had several chances to run, and (2) round-trip survival — after a real kill -9 + restart, WS LIST still returns the workspace. Against current main, both fail: assertion (1) panics with "the WorkspaceCreate record ... was deleted from every on-disk WAL segment." Confirmed RED via `git stash` of the (separately staged) fix and rerunning this test: it fails with exactly that assertion. Restoring the fix turns it green. Disk-offload mode is a pre-existing pass (`g4`/`g5` in crash_recovery_graph_durability.rs, unmodified) — this test targets only the legacy-mode gap. Full graph reconstruction via GRAPH.QUERY after restart is deliberately not asserted in legacy mode: independent probing found graph command replay from WAL v3 does not reconstruct the graph in that mode even with zero Pass C interference (default --max-wal-size), a separate, pre-existing gap unrelated to WAL recycling and out of scope here. author: Tin Dang
…ode (task #43, P1) `AutovacuumDaemon::run_tick`'s Pass C called `WalWriterV3::recycle_aggressive` against `wal.current_lsn()` — the LSN about to be assigned, i.e. "every sealed segment is durable elsewhere" — whenever total WAL bytes exceeded `--max-wal-size`, regardless of whether disk-offload (the only mode with a real checkpoint protocol) was enabled. That assumption only holds when a checkpoint maintains a genuine redo_lsn and `persist_graph_at_checkpoint` snapshots the graph write-buffer before the floor advances. In legacy (non-disk-offload) mode there is no periodic plane snapshot at all: `save_graph_store` runs only at graceful shutdown, and the workspace/MQ/ temporal registries have no snapshot format whatsoever — they replay purely from the WAL. Pass C therefore deleted the sole durable copy of that history once the ceiling was breached; a kill -9 afterwards lost it permanently. Design choice (documented in autovacuum.rs and CHANGELOG): rather than inventing a snapshot format for workspace/MQ/temporal (explicitly out of scope, and the guidance's own fallback for exactly this case), Pass C now takes a `disk_offload_enabled` flag: - true (disk-offload / checkpoint-backed): unchanged — same `recycle_aggressive(current_lsn())` call as before this fix. - false (legacy mode): skip the recycle entirely. Emit a rate-limited (5 min) tracing::warn! and increment the new `RECL_WAL_RECYCLE_BLOCKED_NO_CHECKPOINT_TOTAL` counter (exposed as `reclamation_wal_recycle_blocked_no_checkpoint_total` in `INFO # Reclamation`) so operators can observe unbounded WAL growth in this mode instead of silently losing data. A partial snapshot-then-floor design (freeze+persist the graph plane via the already-callable `persist_graph_at_checkpoint`, then recycle only segments proven free of un-snapshotted workspace/MQ/temporal records) was considered and rejected: it would still leave those three planes unprotected without inventing a snapshot format for them, adding complexity without closing the gap. "Never trade data loss for disk space" wins; operators needing bounded legacy-mode WAL growth should enable `--disk-offload enable`. Both call sites in `event_loop.rs` (tokio select! and monoio tick loop) now pass `server_config.disk_offload_enabled()`. Verification: - tests/crash_recovery_wal_recycle_legacy.rs (previous commit) goes green. - Two new fast unit tests in autovacuum.rs (test_pass_c_legacy_mode_skips_recycle_and_counts_blocked, test_pass_c_disk_offload_mode_recycles_unchanged) exercise Pass C directly against a real WalWriterV3 without spawning a server, proving disk-offload mode's segment count still drops (byte-for-byte unchanged behavior) while legacy mode's does not. - crash_recovery_graph_durability.rs g4/g5 (disk-offload / checkpoint path) pass unmodified, confirming no behavior change outside legacy mode. - Full `cargo test --release --lib` (4145 passed), shardslice_live (6 passed), `cargo clippy` default and `--no-default-features --features runtime-tokio,jemalloc` (zero warnings), `cargo fmt --check` clean. CHANGELOG updated under [Unreleased]. author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLegacy-mode autovacuum no longer recycles WAL without checkpoint support. Disk-offload mode retains existing recycling, while legacy mode emits a rate-limited warning and increments an INFO counter. Unit and ignored crash-recovery tests cover both modes and kill-9 restart behavior. ChangesWAL recycle protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ShardEventLoop
participant AutovacuumDaemon
participant WAL
participant ReclamationMetric
ShardEventLoop->>AutovacuumDaemon: run_tick(..., disk_offload_enabled)
AutovacuumDaemon->>WAL: stats()
alt disk offload enabled
AutovacuumDaemon->>WAL: recycle_aggressive(current_lsn)
else legacy mode
AutovacuumDaemon->>ReclamationMetric: increment blocked counter
AutovacuumDaemon-->>ShardEventLoop: skip recycling and emit rate-limited warning
end
🚥 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 |
![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_wal_recycle_legacy.rs`:
- Around line 139-146: Add an accurate `// SAFETY:` comment immediately before
the `unsafe` block in `sigkill`, documenting why calling `libc::kill` with the
child process ID is safe.
🪄 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: 5b97e9e3-39c2-4c42-af09-37d16aca2fa1
📒 Files selected for processing (5)
CHANGELOG.mdsrc/command/info_reclamation.rssrc/shard/autovacuum.rssrc/shard/event_loop.rstests/crash_recovery_wal_recycle_legacy.rs
CodeRabbit review finding on PR #288: the libc::kill FFI call in the crash_recovery_wal_recycle_legacy harness lacked the repo-mandated // SAFETY: justification. Documents the invariant (owned live Child, targeted pid, no memory-safety preconditions). author: Tin Dang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
P1 data loss (task #43, storage audit 2026-07-12): autovacuum Pass C recycled WAL v3 segments that were the SOLE durable copy of graph/temporal/workspace/MQ history in legacy (non-disk-offload) mode.
Pass C ran in every appendonly-enabled configuration and called
recycle_aggressive(redo_lsn = wal.current_lsn())— the current WAL head, not a checkpoint-verified floor — deleting every sealed segment once total WAL exceeded--max-wal-size(256MB default). In disk-offload mode this is safe (the checkpoint protocol maintains a realredo_lsnand snapshots graph write-buffers). In legacy mode there is no periodic plane snapshot (save_graph_storeruns only at graceful shutdown), so GRAPH.*/TEMPORAL/WS/MQ WAL records were the only durable form of that history — Pass C deleting them meant permanent loss on the next kill-9. KV was never at risk (AOF-authority).Design: skip-and-warn in legacy mode (option B)
Snapshot-then-recycle (option A) was investigated and rejected: workspace/MQ/temporal registries have no snapshot format at all — their replay functions rebuild purely by WAL rescan — so a graph-only snapshot floor would still lose WS/MQ/temporal records, while per-segment record-type scanning adds real complexity. Instead:
AutovacuumDaemon::run_ticknow takesdisk_offload_enabled. True → byte-for-byte unchanged (checkpoint-backedrecycle_aggressive). False → Pass C skips the recycle, logs a rate-limited (5 min) warning ("WAL exceeds --max-wal-size but cannot be recycled without a checkpoint; run BGSAVE or enable disk-offload"), and increments the newreclamation_wal_recycle_blocked_no_checkpoint_totalINFO counter.Verification (red/green)
MOON_BINpinned to the ELF build).WalWriterV3over the ceiling withdisk_offload_enabled=trueand asserts segments are recycled; the untouched checkpoint-path suite (crash_recovery_graph_durabilityg4/g5) passes unmodified.shardslice_live6/6; clippy-D warningsboth matrices; fmt.crash_recovery_graph_durabilityfail pre-existing on an unrelated harness bug (polls the stale v2shard-0.walpath; zero diff on this branch) — root cause recorded in task GPU: device memory pool for zero-allocation compaction cycles #31.Follow-ups
Summary by CodeRabbit
Bug Fixes
Monitoring
Tests