refactor(persistence)!: remove WAL v2 — WAL v3 only, XactCommit unified, AOF-authority recovery#236
Merged
Merged
Conversation
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
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
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
…ection
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
… 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
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? |
📝 WalkthroughWalkthroughThis PR removes the legacy WAL v2 implementation entirely, consolidating all write-ahead logging on WAL v3. It updates record encoding/replay to be db-index-aware, rewires shard event loop and SPSC fanout paths to a single WalWriterV3, changes recovery fallback ordering (AOF authority, WAL v3 last-resort), ports graph WAL scanning to v3 segments, and updates tests/changelog. ChangesWAL v2 Removal and WAL v3 Consolidation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant TxnHandler
participant WalV3Record
participant WalV3Replay
participant Database
TxnHandler->>WalV3Record: encode_xact_commit_payload(txn_id, db_index, undo_records)
WalV3Record-->>TxnHandler: payload bytes
TxnHandler->>WalV3Replay: write_wal_v3_record(XactCommit, payload)
WalV3Replay->>WalV3Replay: replay_xact_commit(databases, payload)
WalV3Replay->>Database: apply ops to db_index (fallback to db 0 if out of range)
flowchart TD
Start --> CheckLegacyWalV2
CheckLegacyWalV2 --> LogErrorIgnore
LogErrorIgnore --> CheckAof
CheckAof -->|exists| ReplayAof
CheckAof -->|absent| ReplayWalV3Dir
ReplayAof --> UpdateTotalKeys
ReplayWalV3Dir --> UpdateTotalKeys
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/shard/spsc_handler.rs (1)
2218-2234: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not apply
SWAPDBafter durability fanout fails.
wal_append_and_fanoutcan returnfalseon AOF backpressure, but this arm discards the result and still swaps databases. That creates a state change that may not be replayable after restart.Changing this cleanly likely requires the
SwapDbreply contract to carry an error so the coordinator can fail loudly before applying the mutation.🤖 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/spsc_handler.rs` around lines 2218 - 2234, The SWAPDB path in spsc_handler::handle command flow ignores the boolean result from wal_append_and_fanout and still performs the ShardSlice database swap, which can mutate state after durability fanout fails. Update the SwapDb handling so the mutation is only applied when wal_append_and_fanout succeeds, and propagate an error through the SwapDb reply contract so the coordinator can fail loudly instead of proceeding; use wal_append_and_fanout, SwapDb, and with_shard as the key symbols to locate the fix.src/shard/event_loop.rs (1)
430-453: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not advertise the WAL append channel when WAL initialization failed.
Line 442 downgrades
WalWriterV3::newfailure toNone, but Lines 451-452 still publishwal_append_txwhenever appendonly/disk-offload is enabled. Local handlers can then enqueue WAL records that the drain path drops becausewal_writerisNone, silently disabling the promised WAL v3 durability/CDC path.Suggested direction
- let mut wal_writer: Option<WalWriterV3> = wal_shard_dir.and_then(|shard_dir| { + let mut wal_writer: Option<WalWriterV3> = wal_shard_dir.and_then(|shard_dir| { let wal_dir = shard_dir.join("wal-v3"); match WalWriterV3::new(shard_id, &wal_dir, server_config.wal_segment_size_bytes()) { @@ Err(e) => { - tracing::warn!("Shard {}: WAL init failed: {}", shard_id, e); + tracing::error!("Shard {}: WAL init failed: {}", shard_id, e); None } } }); @@ - if appendonly_enabled || server_config.disk_offload_enabled() { + if wal_writer.is_some() { shard_databases.set_wal_append_tx(shard_id, wal_append_tx); }If WAL is required by configuration, prefer failing shard startup instead of continuing with
wal_writer = None.🤖 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 430 - 453, The shard setup in event_loop should not expose wal_append_tx when WalWriterV3::new fails and wal_writer becomes None. Update the initialization flow so WAL v3 startup failure is treated as a hard error when appendonly or disk offload requires it, and only call shard_databases.set_wal_append_tx after a successful WalWriterV3::new. Keep the logic centered around wal_writer, wal_append_tx, and shard_databases so the append channel is never advertised unless the WAL drain path is actually available.
🧹 Nitpick comments (1)
src/shard/spsc_handler.rs (1)
1053-1075: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the PipelineBatch test with the current AOF-routing contract.
The implementation now passes
aof_poolfrom the PipelineBatch arms, while this test still documents the old “caller passesNone” invariant and only exercises the helper directly. Add an arm-level test or update the test name/comment so it catches the real double-write/drop contract.Also applies to: 1645-1668, 3331-3345
🤖 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/spsc_handler.rs` around lines 1053 - 1075, The PipelineBatch test still encodes the old AOF-routing assumption that the caller passes None, while the current SPSC flow now routes aof_pool through wal_append_and_fanout. Update the test around wal_append_and_fanout/PipelineBatch to exercise the arm-level behavior instead of only the helper, so it verifies the real double-write/drop contract. If the existing helper-only coverage stays, rename or rewrite its comment/name to match the new contract and avoid documenting stale behavior. Ensure the updated test clearly covers the current aof_pool-driven path and the observed restart/double-apply failure mode.
🤖 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 `@src/persistence/wal_v3/replay.rs`:
- Around line 201-203: The legacy WAL v3 fallback in replay.rs is passing the
raw RESP payload into CommandReplayEngine::replay_command instead of parsing it
into a command name and arguments first. Update the WalRecordType::Command
handling to decode the payload before dispatch so replay_command receives the
expected cmd_name plus parsed args, and ensure the replayed counter only
increments when a command was actually applied rather than on the fallback path.
- Around line 88-90: The Phase-4 recovery dispatcher is missing db-index-aware
handling for WalRecordType::XactCommit, so committed KV ops can be skipped
during normal WAL v3 crash recovery. Update recover_shard_v3_pitr in
src/persistence/recovery.rs to mirror the XactCommit branch used by
replay_wal_auto, and route XactCommit records through the same
replay_xact_commit-style logic as the WAL replay path instead of falling
through.
In `@src/shard/event_loop.rs`:
- Around line 1684-1685: The shutdown paths in event_loop currently ignore WAL
fsync failures by discarding the result of wal.flush_sync(), so update the
shutdown handling around wal_writer to capture and log any error returned from
flush_sync() with enough context for operators to notice. Apply the same fix in
both shutdown branches mentioned in the comment, using the existing wal_writer
and flush_sync call sites to locate them, and ensure failures are surfaced
through the shutdown logging rather than being silently dropped.
---
Outside diff comments:
In `@src/shard/event_loop.rs`:
- Around line 430-453: The shard setup in event_loop should not expose
wal_append_tx when WalWriterV3::new fails and wal_writer becomes None. Update
the initialization flow so WAL v3 startup failure is treated as a hard error
when appendonly or disk offload requires it, and only call
shard_databases.set_wal_append_tx after a successful WalWriterV3::new. Keep the
logic centered around wal_writer, wal_append_tx, and shard_databases so the
append channel is never advertised unless the WAL drain path is actually
available.
In `@src/shard/spsc_handler.rs`:
- Around line 2218-2234: The SWAPDB path in spsc_handler::handle command flow
ignores the boolean result from wal_append_and_fanout and still performs the
ShardSlice database swap, which can mutate state after durability fanout fails.
Update the SwapDb handling so the mutation is only applied when
wal_append_and_fanout succeeds, and propagate an error through the SwapDb reply
contract so the coordinator can fail loudly instead of proceeding; use
wal_append_and_fanout, SwapDb, and with_shard as the key symbols to locate the
fix.
---
Nitpick comments:
In `@src/shard/spsc_handler.rs`:
- Around line 1053-1075: The PipelineBatch test still encodes the old
AOF-routing assumption that the caller passes None, while the current SPSC flow
now routes aof_pool through wal_append_and_fanout. Update the test around
wal_append_and_fanout/PipelineBatch to exercise the arm-level behavior instead
of only the helper, so it verifies the real double-write/drop contract. If the
existing helper-only coverage stays, rename or rewrite its comment/name to match
the new contract and avoid documenting stale behavior. Ensure the updated test
clearly covers the current aof_pool-driven path and the observed
restart/double-apply failure mode.
🪄 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: 5e156b25-c17d-4dca-8dc5-06d3629e85d6
📒 Files selected for processing (15)
CHANGELOG.mdsrc/persistence/mod.rssrc/persistence/recovery.rssrc/persistence/wal.rssrc/persistence/wal_v3/record.rssrc/persistence/wal_v3/replay.rssrc/server/conn/handler_monoio/txn.rssrc/server/conn/handler_sharded/txn.rssrc/shard/event_loop.rssrc/shard/mod.rssrc/shard/persistence_tick.rssrc/shard/shared_databases.rssrc/shard/spsc_handler.rssrc/shard/timers.rstests/crash_matrix_per_shard_aof.rs
💤 Files with no reviewable changes (3)
- src/persistence/mod.rs
- src/persistence/wal.rs
- src/shard/timers.rs
Comment on lines
88
to
90
| WalRecordType::XactCommit => { | ||
| // XactCommit: replay KV ops from payload | ||
| // XactCommit: db-index-aware KV replay | ||
| replay_xact_commit(databases, &record.payload); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Mirror XactCommit replay in the main shard recovery dispatcher.
Line 90 applies db-aware commits in replay_wal_auto, but the supplied recover_shard_v3_pitr Phase-4 dispatcher in src/persistence/recovery.rs still falls through for WalRecordType::XactCommit. Since the txn handlers now write XactCommit, normal WAL v3 crash recovery can drop committed transaction KV ops unless an AOF fallback happens.
Proposed fix
-fn replay_xact_commit(databases: &mut [crate::storage::Database], payload: &[u8]) {
+pub(crate) fn replay_xact_commit(databases: &mut [crate::storage::Database], payload: &[u8]) {+ WalRecordType::XactCommit => {
+ crate::persistence::wal_v3::replay::replay_xact_commit(
+ databases,
+ &record.payload,
+ );
+ result.commands_replayed += 1;
+ }📝 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
| WalRecordType::XactCommit => { | |
| // XactCommit: replay KV ops from payload | |
| // XactCommit: db-index-aware KV replay | |
| replay_xact_commit(databases, &record.payload); | |
| pub(crate) fn replay_xact_commit(databases: &mut [crate::storage::Database], payload: &[u8]) { |
🤖 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/wal_v3/replay.rs` around lines 88 - 90, The Phase-4 recovery
dispatcher is missing db-index-aware handling for WalRecordType::XactCommit, so
committed KV ops can be skipped during normal WAL v3 crash recovery. Update
recover_shard_v3_pitr in src/persistence/recovery.rs to mirror the XactCommit
branch used by replay_wal_auto, and route XactCommit records through the same
replay_xact_commit-style logic as the WAL replay path instead of falling
through.
Comment on lines
+201
to
+203
| WalRecordType::Command => { | ||
| engine.replay_command(databases, &record.payload, &[], &mut selected_db); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Parse RESP command payloads before dispatching legacy WAL v3 fallback.
CommandReplayEngine::replay_command expects cmd_name plus parsed args, but Line 202 passes the full RESP payload as the command name and Line 239 still counts it as replayed. A last-resort WAL v3 fallback can report success while applying no SET/DEL data.
Proposed fix
WalRecordType::Command => {
- engine.replay_command(databases, &record.payload, &[], &mut selected_db);
+ let mut buf = bytes::BytesMut::from(&record.payload[..]);
+ let parse_cfg = crate::protocol::ParseConfig::default();
+ while let Ok(Some(frame)) = crate::protocol::parse::parse(&mut buf, &parse_cfg) {
+ if let crate::protocol::Frame::Array(ref arr) = frame {
+ if arr.is_empty() {
+ continue;
+ }
+ let cmd_name = match &arr[0] {
+ crate::protocol::Frame::BulkString(s) => s.as_ref(),
+ crate::protocol::Frame::SimpleString(s) => s.as_ref(),
+ _ => continue,
+ };
+ engine.replay_command(databases, cmd_name, &arr[1..], &mut selected_db);
+ commands_replayed += 1;
+ }
+ }
}
@@
WalRecordType::XactCommit => {
replay_xact_commit(databases, &record.payload);
tracing::trace!(lsn = record.lsn, "WAL replay: XactCommit");
+ commands_replayed += 1;
}
@@
- commands_replayed += 1;
};Also applies to: 239-239
🤖 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/wal_v3/replay.rs` around lines 201 - 203, The legacy WAL v3
fallback in replay.rs is passing the raw RESP payload into
CommandReplayEngine::replay_command instead of parsing it into a command name
and arguments first. Update the WalRecordType::Command handling to decode the
payload before dispatch so replay_command receives the expected cmd_name plus
parsed args, and ensure the replayed counter only increments when a command was
actually applied rather than on the fallback path.
Comment on lines
1684
to
+1685
| if let Some(ref mut wal) = wal_writer { | ||
| let _ = wal.shutdown(); | ||
| } | ||
| if let Some(ref mut wal_v3) = wal_v3_writer { | ||
| let _ = wal_v3.flush_sync(); | ||
| let _ = wal.flush_sync(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Log shutdown WAL fsync failures.
Both shutdown paths discard flush_sync() errors, so the operator gets no signal when final WAL bytes fail to reach stable storage.
Suggested fix
if let Some(ref mut wal) = wal_writer {
- let _ = wal.flush_sync();
+ if let Err(e) = wal.flush_sync() {
+ tracing::error!("Shard {}: final WAL flush failed during shutdown: {}", shard_id, e);
+ }
}Also applies to: 1820-1821
🤖 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 1684 - 1685, The shutdown paths in
event_loop currently ignore WAL fsync failures by discarding the result of
wal.flush_sync(), so update the shutdown handling around wal_writer to capture
and log any error returned from flush_sync() with enough context for operators
to notice. Apply the same fix in both shutdown branches mentioned in the
comment, using the existing wal_writer and flush_sync call sites to locate them,
and ensure failures are surfaced through the shutdown logging rather than being
silently dropped.
TinDang97
added a commit
that referenced
this pull request
Jul 7, 2026
WalWriterV3::flush_sync() ran fdatasync synchronously on the shard
event-loop thread — every fsync stalled the shard (no SPSC drain, no
conn I/O, no CDC fan-out until the disk acks). GCE pd baseline
(tmp/WALV3-OFFLOOP-FSYNC.md §8, c2d-standard-16, 3 reps): the everysec
1s timer froze every connection on the shard for 10-16 ms once per
second whenever the WAL held real bytes, and appendfsync=always paid
-20% RPS / ~2x tail (p999 5.4ms -> 9.8ms) on top of the AOF cost.
Design (spec §3, Option A — fsync offload with a durable-LSN watermark):
the writer keeps encode/buffer/page-cache-write/rotate/recycle on the
shard thread; ONLY the fsync moves.
- New src/persistence/wal_v3/sync_agent.rs: per-shard WalSyncAgent
std::thread receives fd-dup'd SyncRequests over flume::bounded(8) and
publishes a monotonic durable-LSN watermark (AtomicU64 fetch_max)
after each successful fdatasync. fd-dup shares the file description,
and write_all happens-before try_clone on the shard thread, so each
fsync covers all previously written bytes; rotation stays safe because
rotate_segment already fsyncs the old segment inline before switching.
- WalWriterV3::request_sync(): non-blocking initiation. Queue-full /
dup-failure / agent-spawn-failure all fall back to inline fsync — a
durability request is never dropped. Agent spawn is lazy (first call)
and never retried after a failure (warn once, inline forever).
- WalWriterV3::wait_durable(lsn, timeout): bounded blocking wait, used
ONLY by the two checkpoint ordering invariants and shutdown:
* log-before-data (flush_dirty_pages page gate, persistence_tick)
* WAL-before-manifest (checkpoint Finalize now waits on the actual
checkpoint record LSN before manifest.commit())
Both bounded by WAIT_DURABLE_TIMEOUT (5s); failure aborts that
checkpoint step (retried next tick) so redo_lsn never advances past
durability.
- Failure policy: an fsync error POISONS the agent permanently (POSIX
post-error fsync semantics are undefined) with tracing::error!;
subsequent request_sync/wait_durable fail loudly and the checkpoint
stalls rather than silently opening a data-loss window.
- Call sites: timers::sync_wal_v3 (everysec) and both runtimes'
appendfsync=always per-drain-batch syncs use request_sync(); shutdown
paths keep inline flush_sync() (which now also publishes the
watermark, as does rotation). everysec semantics become "sync
initiated every 1s, durable typically ms later" — the same window the
AOF everysec writers provide.
Testing (red/green per tmp/WALV3-OFFLOOP-FSYNC.md §4):
- 7 unit tests in sync_agent.rs with a gated/failing injectable fsync
backend: watermark-advances-only-after-fsync, wait-blocks-then-
returns, wait-timeout, poison-fails-loud (wakes parked waiters, never
publishes, refuses new requests), backpressure-hands-request-back,
drop-drains-pending-syncs, watermark-monotonic.
- 2 writer-level tests in segment.rs: request_sync+wait_durable covers
all appends (bytes verified on disk), wait_durable fast paths.
- tests/loom_wal_sync_agent.rs: loom model of the watermark/poison
monitor (no lost wakeup: publisher takes the mutex before notify,
waiter re-checks under it) + std smoke variant when not under loom.
Gates (this worktree): fmt clean; clippy 0 warnings both feature sets
(1 pre-existing vendored-monoio warning); cargo test --lib 3864 passed
(monoio) + 3143 passed (tokio); cargo test --no-run compiles all
targets on both feature sets.
Refs: tmp/WALV3-OFFLOOP-FSYNC.md (spec + GCE baseline), stacked on
refactor/wal-v3-only (PR #236).
author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 7, 2026
WalWriterV3::flush_sync() ran fdatasync synchronously on the shard
event-loop thread — every fsync stalled the shard (no SPSC drain, no
conn I/O, no CDC fan-out until the disk acks). GCE pd baseline
(tmp/WALV3-OFFLOOP-FSYNC.md §8, c2d-standard-16, 3 reps): the everysec
1s timer froze every connection on the shard for 10-16 ms once per
second whenever the WAL held real bytes, and appendfsync=always paid
-20% RPS / ~2x tail (p999 5.4ms -> 9.8ms) on top of the AOF cost.
Design (spec §3, Option A — fsync offload with a durable-LSN watermark):
the writer keeps encode/buffer/page-cache-write/rotate/recycle on the
shard thread; ONLY the fsync moves.
- New src/persistence/wal_v3/sync_agent.rs: per-shard WalSyncAgent
std::thread receives fd-dup'd SyncRequests over flume::bounded(8) and
publishes a monotonic durable-LSN watermark (AtomicU64 fetch_max)
after each successful fdatasync. fd-dup shares the file description,
and write_all happens-before try_clone on the shard thread, so each
fsync covers all previously written bytes; rotation stays safe because
rotate_segment already fsyncs the old segment inline before switching.
- WalWriterV3::request_sync(): non-blocking initiation. Queue-full /
dup-failure / agent-spawn-failure all fall back to inline fsync — a
durability request is never dropped. Agent spawn is lazy (first call)
and never retried after a failure (warn once, inline forever).
- WalWriterV3::wait_durable(lsn, timeout): bounded blocking wait, used
ONLY by the two checkpoint ordering invariants and shutdown:
* log-before-data (flush_dirty_pages page gate, persistence_tick)
* WAL-before-manifest (checkpoint Finalize now waits on the actual
checkpoint record LSN before manifest.commit())
Both bounded by WAIT_DURABLE_TIMEOUT (5s); failure aborts that
checkpoint step (retried next tick) so redo_lsn never advances past
durability.
- Failure policy: an fsync error POISONS the agent permanently (POSIX
post-error fsync semantics are undefined) with tracing::error!;
subsequent request_sync/wait_durable fail loudly and the checkpoint
stalls rather than silently opening a data-loss window.
- Call sites: timers::sync_wal_v3 (everysec) and both runtimes'
appendfsync=always per-drain-batch syncs use request_sync(); shutdown
paths keep inline flush_sync() (which now also publishes the
watermark, as does rotation). everysec semantics become "sync
initiated every 1s, durable typically ms later" — the same window the
AOF everysec writers provide.
Testing (red/green per tmp/WALV3-OFFLOOP-FSYNC.md §4):
- 7 unit tests in sync_agent.rs with a gated/failing injectable fsync
backend: watermark-advances-only-after-fsync, wait-blocks-then-
returns, wait-timeout, poison-fails-loud (wakes parked waiters, never
publishes, refuses new requests), backpressure-hands-request-back,
drop-drains-pending-syncs, watermark-monotonic.
- 2 writer-level tests in segment.rs: request_sync+wait_durable covers
all appends (bytes verified on disk), wait_durable fast paths.
- tests/loom_wal_sync_agent.rs: loom model of the watermark/poison
monitor (no lost wakeup: publisher takes the mutex before notify,
waiter re-checks under it) + std smoke variant when not under loom.
Gates (this worktree): fmt clean; clippy 0 warnings both feature sets
(1 pre-existing vendored-monoio warning); cargo test --lib 3864 passed
(monoio) + 3143 passed (tokio); cargo test --no-run compiles all
targets on both feature sets.
Refs: tmp/WALV3-OFFLOOP-FSYNC.md (spec + GCE baseline), stacked on
refactor/wal-v3-only (PR #236).
author: Tin Dang
Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
6 tasks
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 8, 2026
…237) Wave-2 graph engine (W2-1..W2-14): frozen-tier copy-up writes (SET/DELETE/ MERGE on frozen rows), IndexScan range predicates, row-BFS multi-segment fast path, write-side plan cache, Cypher aggregations (count/sum/avg/min/ max/collect), OPTIONAL MATCH null-padding + WITH mid-pipeline rebinding, configurable traversal timeout, and a kill-9 crash suite for Cypher SET/SET-label WAL durability. Stable node/edge ids across WAL replay. P0 v3 graph-durability fix: disk-offload graphs replay through a dedicated WAL path (GCP-soak-found, re-verified 45,464/45,464 recovery). Cypher-2x wave (profiling showed 82.5% of shard CPU in the mutable-tier index_scan_keys linear scan): P1 mutable-tier property index (point queries 25.5k -> 29.0k qps, p50 -27%, shard CPU -70%); P2 write-gen- invalidated result cache with TinyLFU doorkeeper admission (cache-hostile cycling -21% -> -2.4%); P3 CONTAINS/STARTS WITH/ENDS WITH text predicates via FTS SegmentTextIndex reuse. Vector time-to-index-green: parallel HNSW build (shared-graph concurrent inserts, per-node parking_lot locks, connectivity-repair pass) + insert- path compaction trigger + a Linux affinity-mask fix (core-pinned shard threads made available_parallelism() return 1, silently serializing the "parallel" build and sizing the compactor pool to 1 worker). FT.COMPACT 30.2s -> 2.71s; GCE load-to-HNSW-serving now beats Qdrant 1.6x (x86) / 2.3x (ARM). Multi-shard verified. Benchmarks (BENCHMARK.md): GCE dedicated-core validation vs FalkorDB (Cypher 1-hop 2.3-2.7x, dedicated-core p99 beats FalkorDB) and vs Qdrant (ingest 10x, matched-recall search 2.7-3.4x, time-to-green now a win on both arches). Full CI-parity gate green after merging main (fmt, clippy default+tokio, tests default+tokio); merge resolved conflicts from the WAL v2 removal (#236) and AOF/pub-sub perf waves (#238-#242). 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
BREAKING (pre-1.0 format freeze): WAL v2 is removed; WAL v3 is the only WAL. Also unifies the XactCommit WAL record to the db-aware variant and hardens legacy-dir recovery with explicit AOF-authority semantics.
Moon is pre-release, so this drops on-disk compatibility no shipped release depended on. Operators with leftover WAL v2 files (
shard-N.wal) or pre-freezeXactCommit(0x51)records must let them age out via AOF-authoritative recovery or replay on a pre-freeze build first — a stray v2 file now triggers a loudtracing::error!instead of a silent skip.Commits
refactor(persistence): unify XactCommit WAL record, retire v1 tag 0x51— deletes the pre-1.0XactCommit(tag0x51) record type; renamesXactCommitV2→XactCommit, keeping discriminant0x53(0x51retired, never reused). All producers already wrote only the db-aware record — pure read-side simplification.refactor(persistence)!: remove WAL v2; WAL v3 is now the only WAL— deletessrc/persistence/wal.rsentirely. WAL v3 now also fills the old v2 niche (--appendonly yes --disk-offload disable), rooted at<dir>/shard-N/wal-v3/. Closes a latent gap:--appendfsync alwaysnow fsyncs the WAL unconditionally.replay_wal_autorejects v2 files with a loudWalError::UnsupportedVersion.replay_graph_walported to the v3 segment directory.refactor(shard): simplify WAL fanout gate—wal_append_and_fanout/wal_fanout_has_workdrop the v2 parameter + "v3 supersedes v2" branch;finalize_snapshot_successno longer takes a WAL writer (v3 retention is LSN-driven, independent of legacy snapshot epochs).fix(persistence): restore WAL-preferred recovery + loud legacy-v2 detection+ 5.fix(persistence): AOF stays recovery authority; WAL v3 is last-resort only— review of the refactor found the legacy-dir recovery paths ignoring WAL v3 data; the first fix restored the retired v2 "prefer WAL" contract, and the second corrects its direction: post-perf(persistence): eliminate AOF+WAL KV double-write + fail-loud everysec backpressure #211 WAL v3's KV coverage is intentionally partial (--wal-kv-logdefault-off; connection-local writes bypass it; measured 79.2% incomplete), so a non-empty WAL v3 (which counts temporal/graph/txn records) must never shadow the AOF — that would recover with zero KV data. Final semantics in bothShard::restore_from_persistence_v2andrecover_shard_v3_with_fallbackPhase 4b:appendonly.aofis the authority whenever present; the legacy-modeshard-N/wal-v3/dir is replayed only when no AOF exists, with a loud partial-coverage warning.Why no SIGKILL integration test
Empirically unreliable within repo constraints (documented in
tests/crash_matrix_per_shard_aof.rs):--disk-offloaddefaults to enable (routes around the fixed path), local KV writes never reach WAL v3, and forcing cross-shard traffic depends on non-deterministic SO_REUSEPORT connection routing. Covered instead by deterministic unit tests that construct WAL v3 segments directly.Test plan
cargo fmt --checkcargo clippy -- -D warnings(default) — clean (1 pre-existing vendored-monoio warning)cargo clippy --no-default-features --features runtime-tokio,jemalloc -- -D warnings— cleancargo test --lib— 3855 passed, 1 ignoredcargo test --lib --no-default-features --features runtime-tokio,jemalloc— 3134 passed, 1 ignoredcargo test --no-runboth feature sets — all test binaries compileBreaking-change notes
wal_append_and_fanoutsignature drops the v2 writer parameter (internal).[Unreleased]BREAKING entry included.Summary by CodeRabbit
Breaking Changes
appendonly.aof; older WAL v2 files are rejected and will not be used.Bug Fixes