perf(persistence): eliminate AOF+WAL KV double-write + fail-loud everysec backpressure#211
Merged
Merged
Conversation
Two write-path fixes from the 2026-07-04 WAL/AOF investigation (tmp/WAL-AOF-REVIEW.md, byte-level attribution in tmp/WRITE-DIAG.md): 1. --wal-kv-log auto|on|off (default auto). At --appendonly yes every SPSC-executed KV write was logged to BOTH the per-shard AOF and the per-shard WAL (2.7x file-byte / 4.1x device amplification at shards=4), yet Phase-B recovery wipes WAL-replayed state and replays the AOF. `auto` skips WAL KV records while the AOF is the recovery authority and no CDC subscriber is attached (re-engages dynamically on attach); `on` = pre-0.6 behavior (PITR / full CDC history); `off` = never. FPI/checkpoint/feature records unaffected. 2. everysec/no appends are no longer silently dropped on writer backpressure. Durable handler paths await enqueue bounded by --aof-fsync-timeout-ms and surface ChannelFull/WriteFailed as an error frame instead of a false +OK; the sync SPSC-drain and monoio inline-SET paths use a 5ms bounded blocking send before a counted, error!-logged last-resort drop (AOF_BACKPRESSURE_DROPPED in INFO). Red/green: everysec_full_channel_errs_instead_of_silent_drop proven RED against the old fire-and-forget branch; WAL gate pinned by test_kv_append_skipped_when_wal_kv_log_false (header-only segment + AOF still receives the entry). fmt + clippy (default & tokio) clean; lib suites green on both runtimes. GCE A/B harness added (scripts/gcloud-wal-aof-ab.sh): volume attribution, throughput reps, kill -9 AOF-alone recovery, backpressure-counter watch. author: Tin Dang
…ed probes First GCE run hung after config 1: moon survives SIGTERM/SHUTDOWN (graceful path lingers), SO_REUSEPORT lets the next rep's server bind the SAME port alongside the half-dead one, and wait_port's un-bounded redis-cli ping then blocks forever against the split-traffic pair. stop_moon(): SIGKILL + pkill pattern backstop + wait-for-port-free; every redis-cli probe now timeout-bounded; leftover state swept at measure start. kill -9 is the documented house convention for moon bench harnesses (CLAUDE.md). author: Tin Dang
…ed remote driving gcloud ssh cannot run under the local Monitor sandbox and a full --measure exceeds foreground tool timeouts; --measure-one <shards> <walkv> runs a single config (~2-3 min) so the outer driver can chunk the sweep as short ssh calls. Used for the shipped GCE proof (tmp/wal-ab-c3-standard-4.txt). author: Tin Dang
…iterals The new --wal-kv-log flag added a field to ServerConfig; 16 struct-literal initializers across 10 tokio-gated integration tests (integration.rs, txn_kv_wiring.rs, workspace_integration.rs, mq_integration.rs, ...) failed to compile under the runtime-tokio feature set (E0063). Set "auto" (the default) everywhere so test behavior is unchanged. Caught by the OrbStack CI-parity matrix (tokio stage); the monoio suite never compiles these cfg-gated files, so it stayed green. author: Tin Dang
|
Warning Review limit reached
Next review available in: 18 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 (7)
📝 WalkthroughWalkthroughAdds a ChangesWAL-KV logging and AOF backpressure
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ShardEventLoop
participant SpscHandler
participant WalWriter
participant AofWriterPool
Client->>ShardEventLoop: write command
ShardEventLoop->>ShardEventLoop: resolve wal_kv_log_mode()
ShardEventLoop->>SpscHandler: drain_spsc_shared(..., wal_kv_log)
alt wal_kv_log enabled
SpscHandler->>WalWriter: append KV record
else wal_kv_log disabled
SpscHandler->>SpscHandler: skip WAL KV append
end
SpscHandler->>AofWriterPool: send_append_bounded_blocking(bytes)
alt enqueued within bound
AofWriterPool-->>SpscHandler: ok
else channel full past bound
AofWriterPool-->>SpscHandler: dropped / timeout
end
SpscHandler-->>Client: +OK
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
PR Summary by Qodoperf(persistence): eliminate AOF+WAL KV double-write + fail-loud everysec backpressure
AI Description
Diagram
High-Level Assessment
Files changed (22)
|
New config `walonly` = --appendonly no with default disk-offload (enable), making WAL-v3 the sole KV log (wal-kv-log auto keeps logging when there is no AOF to defer to). Measures the WAL-only performance posture and extends the shards=4 kill-9 recovery check to WAL-alone recovery — sizing the conn-local-write gap (local-path writes never enter the SPSC drain and are not WAL-logged, so WAL-only recovery is expected to be incomplete). author: Tin Dang
Code Review by Qodo
Context used✅ Compliance rules (platform):
40 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+2783
to
+2789
| let tmp = tempfile::tempdir().unwrap(); | ||
| let wal_dir = tmp.path().join("wal"); | ||
| let mut w3 = Some(WalWriterV3::new(0, &wal_dir, 16 * 1024 * 1024).unwrap()); | ||
| w3.as_mut().unwrap().flush_sync().unwrap(); | ||
| let seg = wal_dir.join("000000000001.wal"); | ||
| let base_len = std::fs::metadata(&seg).unwrap().len(); | ||
|
|
There was a problem hiding this comment.
2. Unannotated unwrap() in wal_append_tests 📘 Rule violation ✧ Quality
New .unwrap() calls were added in tests in src/shard/spsc_handler.rs and src/persistence/aof/pool.rs without the required adjacent #[allow(clippy::unwrap_used)] and immediately preceding justification comment. This violates the unwrap-annotation/audit convention and can undermine ratcheting expectations or fail lint/audit gates that enforce annotated unwrap usage.
Agent Prompt
## Issue description
The PR introduces new `.unwrap()` calls in tests without meeting the project policy that each unwrap must be covered by an immediately attached `#[allow(clippy::unwrap_used)]` on the containing item/scope and an immediately preceding single-line justification comment explaining why the unwrap is safe.
## Issue Context
Policy requires each unwrap to be covered by:
- `// <why unwrap is safe>` directly above
- `#[allow(clippy::unwrap_used)]` directly attached to the containing item/scope (e.g., the test function or a narrower block)
Update the affected tests so every new unwrap is either removed (by handling errors explicitly) or is properly justified and locally allowed per the convention.
## Fix Focus Areas
- src/shard/spsc_handler.rs[2783-2813]
- src/persistence/aof/pool.rs[1925-2006]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/persistence/aof/pool.rs (1)
237-248: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc contradicts the new EverySec backpressure behavior.
The surrounding doc still describes EverySec/No as fire-and-forget: line 235 says it "stays on the fire-and-forget path (zero new latency)" and lines 244-248 say "The non-Always branch resolves immediately (no actual suspension)". With the new
send_append_backpressurepath, a full writer channel now suspends up tofsync_timeoutand can returnErr(AofAck::ChannelFull). The~5 nsclaim only holds for the fast (non-full) path. Please reconcile lines 235 and 244-248 with the new bounded-await semantics you added at 237-242.🤖 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 `@src/persistence/aof/pool.rs` around lines 237 - 248, The doc comments around the Aof append/ack path are now stale and contradict the new bounded-await behavior. Update the comments on the `send_append_backpressure`-based flow and the surrounding `AofAck` handling to say EverySec/No can block up to `fsync_timeout` and may return `Err(AofAck::ChannelFull)` when the writer channel is full, rather than describing it as fire-and-forget or zero-latency. Also qualify the `~5 ns`/“no actual suspension” claim so it only applies to the fast non-full path, and keep the Always-path failure semantics accurate in the same doc block.
🧹 Nitpick comments (1)
src/shard/event_loop.rs (1)
1180-1186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the triplicated
wal_kv_log_modematch into a helper.The same 6-line match resolving the log-enable boolean is duplicated verbatim at three call sites (tokio notify wake, tokio periodic tick, monoio every-wake). A small free function keeps the three sites in sync and avoids drift if the
Autoheuristic changes later.♻️ Proposed refactor
+fn wal_kv_log_enabled( + mode: crate::config::WalKvLogMode, + appendonly_enabled: bool, + cdc_registry: &crate::cdc::CdcSubscriberRegistry, +) -> bool { + match mode { + crate::config::WalKvLogMode::On => true, + crate::config::WalKvLogMode::Off => false, + crate::config::WalKvLogMode::Auto => !appendonly_enabled || !cdc_registry.is_empty(), + } +}And at each call site:
- match wal_kv_log_mode { - crate::config::WalKvLogMode::On => true, - crate::config::WalKvLogMode::Off => false, - crate::config::WalKvLogMode::Auto => { - !appendonly_enabled || !cdc_registry.is_empty() - } - }, + wal_kv_log_enabled(wal_kv_log_mode, appendonly_enabled, &cdc_registry),Also applies to: 1282-1288, 1860-1866
🤖 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 `@src/shard/event_loop.rs` around lines 1180 - 1186, Extract the repeated wal_kv_log_mode boolean resolution into a shared helper function and call it from all three sites (tokio notify wake, tokio periodic tick, monoio every-wake). Move the existing WalKvLogMode::On/Off/Auto match logic into a small free function or similarly named helper near event_loop, and replace each duplicated match with that helper so the Auto heuristic lives in one place and stays consistent.
🤖 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 `@scripts/gcloud-wal-aof-ab.sh`:
- Around line 205-210: `--measure-one` in the main subcommand dispatch can hit
an unbound-variable crash before `die` runs because `measure_single` is called
with `$2` and `$3` unconditionally. Update the argument handling around the
`case` in `gcloud-wal-aof-ab.sh` to validate that `--measure-one` has both
required parameters before referencing them, and route missing-arg cases to
`die` with a usage message; use the `measure_single` and `die` symbols to keep
the fix localized.
In `@src/persistence/aof/pool.rs`:
- Around line 528-546: The timeout handling around send_fut in the AOF pool
write path can misreport a successful append as ChannelFull because
flume::Sender::send_async is not cancel-safe. Update the outcome logic in the
AofAck write/backpressure branch to use a cancel-safe strategy instead of
directly timing out send_fut, and make sure the AOF_BACKPRESSURE_DROPPED/client
response only reflects writes that are truly not delivered.
In `@src/shard/spsc_handler.rs`:
- Around line 2553-2559: The batched AOF append path in the shard handler resets
backpressure for every write, which can block the event loop for N times the
bounded wait. Update the `MultiExecute` and `PipelineBatch(Slotted)` flow in
`spsc_handler` to track a single remaining deadline or budget across the whole
batch, and pass the reduced timeout into each `send_append_bounded_blocking`
call instead of always using the full `AOF_SPSC_BACKPRESSURE_BOUND`. Keep the
change localized to the batch-processing logic around `aof_pool` so unrelated
connections are not stalled by large batches.
---
Outside diff comments:
In `@src/persistence/aof/pool.rs`:
- Around line 237-248: The doc comments around the Aof append/ack path are now
stale and contradict the new bounded-await behavior. Update the comments on the
`send_append_backpressure`-based flow and the surrounding `AofAck` handling to
say EverySec/No can block up to `fsync_timeout` and may return
`Err(AofAck::ChannelFull)` when the writer channel is full, rather than
describing it as fire-and-forget or zero-latency. Also qualify the `~5 ns`/“no
actual suspension” claim so it only applies to the fast non-full path, and keep
the Always-path failure semantics accurate in the same doc block.
---
Nitpick comments:
In `@src/shard/event_loop.rs`:
- Around line 1180-1186: Extract the repeated wal_kv_log_mode boolean resolution
into a shared helper function and call it from all three sites (tokio notify
wake, tokio periodic tick, monoio every-wake). Move the existing
WalKvLogMode::On/Off/Auto match logic into a small free function or similarly
named helper near event_loop, and replace each duplicated match with that helper
so the Auto heuristic lives in one place and stays consistent.
🪄 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: 10d967fa-da60-4e32-8d3c-278cac006653
📒 Files selected for processing (22)
CHANGELOG.mddocs/configuration.mddocs/guides/configuration.mddocs/guides/persistence.mdscripts/gcloud-wal-aof-ab.shsrc/config.rssrc/persistence/aof/mod.rssrc/persistence/aof/pool.rssrc/server/conn/blocking.rssrc/shard/event_loop.rssrc/shard/mod.rssrc/shard/spsc_handler.rstests/ft_search_multi_shard_as_of.rstests/ft_search_temporal_parity.rstests/integration.rstests/kill_snapshot.rstests/mq_integration.rstests/replication_test.rstests/txn_ft_search_snapshot.rstests/txn_kv_wiring.rstests/vacuum_commands.rstests/workspace_integration.rs
GCE measurement (walonly posture, c3-standard-4): with --appendonly no only SPSC-dispatched writes reach the WAL — conn-local writes are logged nowhere. kill -9 recovery at 4 shards restored 311,551/393,394 keys (79.2% ≈ 1 - 1/num_shards); at 1 shard nothing is logged at all. Document that --appendonly yes remains the durability posture and the WAL KV stream serves CDC/PITR/disk-offload only. author: Tin Dang
…shared budget (PR #211 review) Addresses the four valid PR #211 review findings (qodo + CodeRabbit): 1. AOF-loss no longer acked (qodo Bug): `wal_append_and_fanout` returns bool and `send_append_bounded_blocking` failures now replace the success frame with `MOONERR AOF backpressure ...` at every SPSC arm (Execute/Slotted, MultiExecute/Slotted, PipelineBatch/Slotted, MOVE, COPY) and the monoio inline-SET fast path (blocking.rs) — previously a timed-out enqueue still returned +OK. SwapDb discards the result deliberately (coordinator broadcast, no client frame). 2. Batch-shared backpressure budget (CodeRabbit Major): `send_append_bounded_blocking` now takes `&mut Duration` and decrements by time actually blocked; MultiExecute/PipelineBatch arms share ONE 5ms budget per batch, so sustained backpressure stalls the shard thread at most BOUND total, not BOUND x batch-len. Exhausted budget fails immediately (new test proves <10ms). 3. --wal-kv-log typo fallback (qodo): clap value_parser rejects unknown values at parse time instead of silently mapping to auto. 4. Harness --measure-one without args (CodeRabbit): ${2:?}/${3:?} usage message instead of set -u unbound-variable crash. Skipped with justification: - Test-code unwrap() annotations: tests are exempt per repo policy ("no unwrap in library code OUTSIDE tests"); the audit-unwrap ratchet passed in CI. - flume send_async cancel-safety on the async timeout race: on timeout the record MAY still be delivered while the client sees an error — that is the safe failure direction (at-least-once on retry, never silent loss); documented in a comment instead of a heavy refactor. Tests: spsc_bounded_blocking_* (3, incl. new exhausted-budget red/green), wal_append fanout suite (6), everysec tokio suite (4), kv gate test — all green; fmt + clippy clean on both feature sets. author: Tin Dang
TinDang97
pushed a commit
that referenced
this pull request
Jul 4, 2026
Patch release bundling PR #211 (squash 450769b): - AOF+WAL KV double-write eliminated (--wal-kv-log auto, -44% device writes / +10% P64 at shards=4, GCE-proven, lossless kill-9 recovery) - everysec/no AOF appends fail-loud under backpressure (error frame instead of +OK; batch-shared 5ms bound on SPSC paths) - Docker image build fix for the vendored-monoio chef stages CHANGELOG [Unreleased] -> [0.5.1]; RELEASES.md ledger recorded via add.py release v0.5.1 --force (no milestone bundled — hotfix-style patch; v3-5 milestone remains open for the follow-up scope). author: Tin Dang
This was referenced Jul 4, 2026
Merged
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 7, 2026
…ed, AOF-authority recovery (#236) * refactor(persistence): unify XactCommit WAL record, retire v1 tag 0x51 Collapse the two XactCommit record variants into one. `XactCommitV2` (tag 0x53, db-aware: txn_id + db_index + kv_op_count header) becomes the sole `XactCommit`; the original non-db-aware v1 record (tag 0x51) is deleted outright rather than reused, so any pre-freeze WAL segment still carrying 0x51 fails loud on replay instead of being silently misinterpreted as the new layout. - src/persistence/wal_v3/record.rs: drop `WalRecordType::XactCommit = 0x51` and the v1 `encode_xact_commit_payload`; rename the v2 encoder to `encode_xact_commit_payload` (txn_id, db_index, undo_records, db). `from_u8` no longer maps 0x51 to anything (pinned by a new unit test). - src/persistence/wal_v3/replay.rs: merge `replay_xact_commit` and `replay_xact_commit_v2` into one db-aware replay function; out-of-range db_index falls back to db 0 with a `tracing::warn!`. - src/server/conn/handler_sharded/txn.rs, src/server/conn/handler_monoio/txn.rs: follow the rename (`encode_xact_commit_payload_v2` -> `encode_xact_commit_payload`, `WalRecordType::XactCommitV2` -> `WalRecordType::XactCommit`). Tests (red/green): `test_record_type_discriminants` extended to assert `WalRecordType::from_u8(0x51).is_none()`; renamed `test_xact_commit_replays_into_selected_db` and `test_xact_commit_out_of_range_db_falls_back_to_db0` cover the unified replay path. Both failed to compile against the old dual-variant API before the rename, pass now. author: Tin Dang * refactor(persistence)!: remove WAL v2; WAL v3 is now the only WAL BREAKING (pre-1.0 format freeze): delete the flat-file WAL v2 writer, replay path, and file layout entirely. WAL v3 (segment-based, LSN- tracked `WalWriterV3`) becomes the sole write-ahead log everywhere — event loop, shard restore, workspace/graph replay, and cross-shard fanout. A WAL v2 file found on disk after upgrade is rejected loudly (`WalError::UnsupportedVersion { version: 2 }`) instead of silently replayed or ignored; operators must re-ingest via AOF or replay it on a pre-freeze Moon build first. - src/persistence/wal.rs: deleted (958 lines — `WalWriter`, `replay_wal`, `wal_path`, tests). - src/persistence/mod.rs: drop `pub mod wal;`. - src/persistence/wal_v3/replay.rs: `replay_wal_auto`'s v2 branch now logs a loud `tracing::error!` and returns `WalError::UnsupportedVersion` instead of delegating to the deleted v2 replay function. - src/persistence/recovery.rs: Phase 4b simplified from "WAL v2 fallback then AOF fallback" to a single AOF-only fallback (`<dir>/appendonly.aof`) when the WAL replayed zero commands. - src/shard/event_loop.rs: single writer-construction path builds `Option<WalWriterV3>` rooted at `<offload-or-persistence-dir>/shard-N/ wal-v3/` (the old v2 file's niche); all downstream call sites (`drain_spsc_shared`, `finalize_snapshot_success`, flush/sync/shutdown) take one `wal_writer` argument instead of a v2+v3 pair. - src/shard/persistence_tick.rs: `finalize_snapshot_success` no longer takes a WAL writer at all — v2's per-snapshot `truncate_after_snapshot(epoch)` had no v3 equivalent; v3 retention is LSN-driven (autovacuum Pass C `recycle_aggressive` / `recycle_segments_before`) and already runs independently of snapshot epochs. `flush_wal_if_needed` (v2) deleted; `flush_wal_v3_if_needed` kept. - src/shard/timers.rs: `sync_wal` (v2) deleted; `sync_wal_v3` kept. - src/shard/mod.rs: `restore_from_persistence_v2` (legacy recovery path) now does snapshot-load + AOF replay only, no WAL v2 fallback. - src/shard/shared_databases.rs: `replay_graph_wal` ported from v2 (`wal::wal_path` + `wal::replay_wal`) to v3 (`wal-v3/*.wal` segment scan + `replay_wal_v3_file`), mirroring `replay_workspace_wal`'s existing v3 pattern; stale "WAL v2/v3" doc comments on `wal_append_txs` and `wal_append` updated to "WAL v3". Tests (red/green): all WAL v3 unit tests continue to pass (`cargo test --lib wal_v3::` — 52/52); full `--lib` suite 3781 passed, 0 failed, 1 ignored (default features) and 3126 passed, 0 failed, 1 ignored (runtime-tokio,jemalloc — 1 unrelated pre-existing environmental disk-free flake in `coordinator_local_leg_durability`, confirmed passing 7/7 in isolation). `cargo test --no-run` (all targets) compiles clean under both feature sets. Deferred (pre-existing, out of scope for this refactor): - `replay_temporal_wal` in shared_databases.rs scans `<dir>/shard-N/` directly instead of `<dir>/shard-N/wal-v3/` — directory-mismatch bug that predates this change, discovered while porting `replay_graph_wal`. - `replay_wal_auto`'s v3 "Command" record replay passes the raw RESP payload as a parsed command name with empty args, which looks architecturally suspect; preserved bug-for-bug in the ported `replay_graph_wal` for consistency since it predates this task. author: Tin Dang * refactor(shard): simplify WAL fanout gate now that v2 fallback is gone Apply the simplification the WAL v2 removal unlocks: the cross-shard write fanout no longer needs to reason about two possible WAL writers. - src/shard/spsc_handler.rs: `wal_fanout_has_work` collapses to `(wal_kv_log && wal_writer.is_some()) || !replica_txs.is_empty() || aof_pool.is_some()` (was: OR of v2-has-work / v3-has-work). `wal_append_and_fanout` drops the v3-then-v2-fallback if/else and appends to the single `Option<WalWriterV3>` directly. Both functions lose the now-redundant second writer parameter, and every call site in `drain_spsc_shared` / `handle_shard_message_shared` follows. - CHANGELOG.md: add the `[Unreleased]` BREAKING entry documenting the XactCommit unification and WAL v2 removal, including the two deferred follow-ups called out in the previous commit. Gates (full local CI parity, from the worktree root): - `cargo fmt --check` — clean. - `cargo clippy -- -D warnings` — 0 errors (1 pre-existing `must_use` warning in vendored `vendor/monoio/src/builder.rs`, out of scope). - `cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings` — no issues found. - `cargo test --lib` (default features) — 3781 passed, 0 failed, 1 ignored. - `cargo test --no-default-features --features runtime-tokio,jemalloc` — 3126 passed, 0 failed, 1 ignored (1 pre-existing environmental disk-free flake, confirmed passing 7/7 in isolation). - `cargo test --no-run` (all targets, both feature sets) — compiles clean. - `bash scripts/audit-unsafe.sh` — 258/258 unsafe blocks have SAFETY comments. - `bash scripts/audit-unwrap.sh` — 0 un-annotated unwraps in hot-path modules (baseline 0). author: Tin Dang * fix(persistence): restore WAL-preferred recovery + loud legacy-v2 detection The WAL-v2-removal refactor (branch refactor/wal-v3-only) silently dropped the pre-refactor "prefer WAL, fall back to AOF" recovery contract in both non-disk-offload restore (Shard::restore_from_persistence_v2, src/shard/mod.rs) and the disk-offload fallback path (recover_shard_v3_with_fallback, src/persistence/recovery.rs). Both paths went straight to AOF replay, meaning any WAL v3 record written after the last AOF flush (or ahead of what a lost/rebuilt appendonlydir/ manifest reflects) would be silently lost on recovery -- a durability regression versus the retired WAL v2 behavior. This change: - Adds replay_wal_v3_dir_commands() (src/persistence/wal_v3/replay.rs), a thin wrapper over the existing segment-replay primitives that drives a CommandReplayEngine across every Command/XactCommit record in a WAL v3 directory and returns the count replayed. - restore_from_persistence_v2 and recover_shard_v3_with_fallback's Phase 4b now try WAL v3 first and only fall back to AOF replay when the WAL v3 directory is absent or empty, mirroring the old WAL-v2-era preference and closing the "no durable write lost" gap. - Both paths now tracing::error! (not silently skip) when a leftover legacy shard-{id}.wal (v2) file is found on disk, naming the file and stating that v2 support was removed and this build will not replay it. - Corrects a misleading test comment (src/persistence/wal_v3/record.rs) that claimed unrecognized WAL v3 record tags are "warn + skip" -- the segment replay loop actually stops replaying the rest of that segment on any unrecognized/corrupt record. No behavior changed, comment only. Testing: 8 new unit tests (3 in shard/mod.rs, 3 in persistence/recovery.rs, plus the pre-existing record.rs assertion whose comment was corrected) cover WAL-v3-preferred-over-AOF, AOF-fallback-when-WAL-v3-absent, and stray-legacy-v2-file-does-not-break-recovery, for both the non-offload and disk-offload-fallback code paths. Verified RED (temporarily hardcoding the pre-fix "always AOF" behavior reproduced the exact failures: e.g. `test_legacy_wal_v3_preferred_over_stale_aof` failed with left=2 right=5, `test_restore_from_persistence_v2_prefers_wal_v3_over_aof` failed with left=1 right=3) then GREEN after restoring the real fix. An end-to-end SIGKILL crash-recovery integration test was attempted per the original review ask but proven empirically unreliable within repo constraints: --disk-offload defaults to "enable" (routes around the fixed path entirely), ordinary local KV writes never touch WAL v3 (they go through aof_pool.try_send_append_durable; WAL v3 only receives cross-shard SPSC writes gated by --wal-kv-log, or WS/MQ/GRAPH/TXN records), and forcing cross-shard dispatch via SO_REUSEPORT connection routing is non-deterministic across separate redis-cli invocations. The integration test and its helper were removed in favor of the deterministic unit tests above, which exercise the same fixed functions directly; the rationale is documented as a comment in tests/crash_matrix_per_shard_aof.rs. Gates run locally (this worktree, refactor/wal-v3-only): - cargo fmt --check: clean - cargo clippy -- -D warnings: 0 errors (1 pre-existing vendored monoio warning, unrelated to this change) - CARGO_TARGET_DIR=target-tokio cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings: no issues found - cargo test --lib: 3787 passed, 1 ignored - CARGO_TARGET_DIR=target-tokio cargo test --lib --no-default-features --features runtime-tokio,jemalloc: 3132 passed, 1 ignored - cargo test --no-run (both feature sets): all test binaries compile author: Tin Dang * fix(persistence): AOF stays recovery authority; WAL v3 is last-resort only Corrects the recovery-preference direction introduced by the previous commit ("restore WAL-preferred recovery"). That fix resurrected the retired WAL v2 "prefer the WAL, skip the AOF when the WAL replays >0" contract for WAL v3 — but the contract's premise no longer holds: Post-#211, WAL v3's KV coverage is intentionally PARTIAL. `--wal-kv-log` is default-off, and connection-local KV writes bypass WAL v3 entirely even when it is on (measured WAL-only recovery: 79.2% incomplete, see tmp/WRITE-DIAG.md). Meanwhile `replay_wal_v3_dir_commands` counts EVERY replayable record type (temporal/graph/txn included), so a WAL v3 dir holding only non-KV records returns non-zero, shadows the AOF, and the node recovers with ZERO KV data — total KV loss, strictly worse than the gap the previous commit set out to close. This change flips the preference in both legacy-dir recovery paths: - `Shard::restore_from_persistence_v2` (src/shard/mod.rs) and Phase 4b of `recover_shard_v3_with_fallback` (src/persistence/recovery.rs) now replay `appendonly.aof` as the authority whenever it exists, and replay the legacy-mode `shard-N/wal-v3/` directory ONLY when no AOF exists — with a loud `tracing::warn!` stating the fallback's partial KV coverage. The loud legacy-v2 `shard-N.wal` detection from the previous commit is kept unchanged. - `replay_wal_v3_dir_commands` doc comment rewritten: the helper is explicitly NOT an authority signal; callers must never skip the AOF on a non-zero return. - Tests flipped/renamed to assert the corrected semantics, both paths: AOF-wins-over-populated-WAL (would have been the KV-loss scenario) and WAL-v3-last-resort-when-no-AOF (partial recovery beats none). AOF-when-WAL-absent and stray-legacy-v2 tests kept as-is. - CHANGELOG BREAKING entry + crash_matrix_per_shard_aof.rs NOTE updated to describe the AOF-authority + last-resort-fallback semantics. author: Tin Dang --------- Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
This was referenced Jul 7, 2026
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
Two write-path durability fixes from the 2026-07-04 WAL/AOF investigation (
tmp/WAL-AOF-REVIEW.md, built on the byte-level attribution intmp/WRITE-DIAG.md), plus the GCE A/B harness that proves them.1. AOF+WAL double-write eliminated —
--wal-kv-log auto|on|off(defaultauto)At
--appendonly yes, every SPSC-executed KV write was logged to both the per-shard AOF and the per-shard WAL, yet Phase-B recovery (main.rs) wipes WAL-replayed state and replays the AOF — the WAL copy was pure write amplification. The gate lives at the single KV→WAL choke point (wal_append_and_fanout);autoskips KV records while the AOF is the recovery authority and no CDC subscriber is attached (re-engages dynamically on attach),onrestores the old behavior (PITR / full CDC history),offnever logs. FPI/checkpoint/feature records are untouched.2. everysec/no appends no longer silently dropped under backpressure
A full AOF writer channel used to
warn!+ drop the record while the client still received+OK— client-acked write loss, and AOF/memory divergence that corrupts replay of dependent commands (e.g. INCR replayed without its SET). Now:try_send_append_durable): await enqueue bounded by--aof-fsync-timeout-ms, surfaceChannelFull/WriteFailedas an error frame;AOF_SPSC_BACKPRESSURE_BOUND) before a counted,error!-logged last-resort drop. Fast path is the sametry_send— zero cost until the channel is full. Watchaof_backpressure_droppedinINFO.GCE proof (c3-standard-4, pd-ssd, steal=0, same-instance A/B, 3 fresh-server reps)
SET 500K × 64B × c50,
--appendonly yeseverysec. Raw:tmp/wal-ab-c3-standard-4.txt.on(old)auto(fix)shards=1: byte-identical volumes and throughput parity in both modes (gate is structurally a no-op there).
Bonus measurement: WAL-only posture (
--appendonly no, same instance type)WAL-only is fast (P64 +68% over AOF-only) but not durable: conn-local writes never reach the WAL, so recovery loses ~
1/num_shardsof writes at shards≥2 and everything at shards=1. Documented indocs/guides/persistence.md; confirms the AOF as the durability log and the WAL KV stream as CDC/PITR/offload infrastructure. (Cross-check: the s4onrun's WAL held 80% of AOF bytes — the same ~1/4 conn-local fraction.)Tests
everysec_full_channel_errs_instead_of_silent_drop— red-proven against the old fire-and-forget branch (reverted → fails, restored → passes) + drain-in-time success + sync-path bound/fast-path tests (pool.rs).test_kv_append_skipped_when_wal_kv_log_false— WAL segment stays header-only while the AOF pool still receives the entry; control assertsonlogs (spsc_handler.rs).memory_doctor_returns_documented_schemafails identically on unmodifiedmainon macOS (DashTable accounting = 0).Docs
docs/guides/configuration.md(+ "Defaults are tuned for the single-shard case" section),docs/configuration.md,docs/guides/persistence.md("One KV log at a time"), CHANGELOG[Unreleased].Follow-ups (tracked in
.add/milestones/v3-5-write-path-durability/, not this PR)fsync_barrier(the 2000 msalwayspipeline tail — [HIGH] carry-over from v3-4).Bytes::copy_from_sliceelimination + single-pass encode inwal_append_and_fanout.alwaysfsync off the shard event-loop thread.BITOP/COPY/DEL/UNLINKcoordinator local-leg durability.🤖 Generated with Claude Code — investigation, fix, red/green tests, and GCE validation in
tmp/WAL-AOF-REVIEW.md.