feat(persistence): WAL group commit — coalesce concurrent appendfsync=always writes into one fsync#178
Merged
Merged
Conversation
New task wal-group-commit (v2-performance milestone, risk:high autonomy:conservative): WAL group commit — coalesce concurrent appendfsync=always fsyncs via opportunistic drain on both AOF writers, to collapse the ~11x write penalty (135K->12K). Bundle frozen @ v1 (approved Tin Dang 2026-06-14): - §1 SPECIFY: opportunistic drain, both writers, mixed-stream durability invariant (acked => on disk; one fsync per bounded batch; control msgs break the batch), grounded in the live AOF writer code (per-connection await yields => concurrent durable writes converge as queued AppendSync; channel mixes Append/AppendSync/control). - §2 SCENARIOS: one per Must (M0-M4) + per Reject (5 codes), each with its unchanged-clause. - §3 CONTRACT: collect_group_commit_batch (pure, cap-bounded, control-stopping seam) + commit_group_commit_batch (one fsync, ack-after-fsync); new module src/persistence/aof/group_commit.rs; a response for every reject code. - §4 TESTS: unit batching seam + integration concurrent-writers SIGKILL crash test. Freeze-flagged: mixed-stream drain must preserve C4-fold + H1-BARRIER + everysec invariants exactly (silent-corruption risk) — mitigated by routing control msgs to deferred_control so batch_straddles_control is structurally impossible. author: Tin Dang
Failing-first (RED) suite for the frozen §3 group-commit contract. Compile-red on the not-yet-existent moon::persistence::aof::group_commit module (the right reason — missing implementation; harness otherwise compiles). 9 seam tests assert observable behavior, not internals: - collect_group_commit_batch (pure): control-first → no batch; stop-at-control preserving order; max_batch cap; max_bytes soft cap. - commit_group_commit_batch (durability invariant via a counting sink): one-fsync-many-acks + fsync-after-all-writes ordering; write-fail → WriteFailed, no Synced; fsync-fail → FsyncFailed, no Synced (proves ack-after-fsync); everysec do_fsync=false → no fsync; barrier covers preceding Appends (H1-BARRIER). End-to-end durability (acked-survives-SIGKILL, rewrite-during-writes) is gated on the integration crash-matrix + a new concurrent-writers crash test added at build — a unit mock cannot prove on-disk survival across a real kill. author: Tin Dang
Add `src/persistence/aof/group_commit.rs`, the pure, runtime-free batching seam for WAL group commit (coalesce concurrent pending AOF writes into ONE fsync under appendfsync=always). This is the §5 BUILD step 1 for the wal-group-commit task: the seam the four writer loops will share. - `collect_group_commit_batch`: pure opportunistic drain — pull queued data messages until the queue empties, a control message appears (deferred, never batched → `batch_straddles_control` structurally impossible), or a bounded cap (count `AOF_GROUP_COMMIT_MAX_BATCH`=1024 / bytes `AOF_GROUP_COMMIT_MAX_BYTES`=8 MiB) is hit. Channel order preserved exactly. - `commit_group_commit_batch` over a `GroupCommitSink`: the durability invariant — write all bytes in order, ONE fsync after all writes (Always only), THEN ack every AppendSync `Synced`. Reject mappings: write error → all WriteFailed; fsync error → all FsyncFailed; ack_before_fsync unreachable by construction. - `ack_batch` (pub(crate)): single source of the ack/durability ordering, shared by the sync commit path and the async/framed inline writer paths added next. Registers `pub mod group_commit` so the §4 red suite pins the public seam. Also fixes a use-after-consume defect in the frozen §4 test `commit_write_fail_acks_write_failed`: it read `rx2.try_recv()` twice on a single-use (flume bounded-1) ack channel, draining it before the second assertion — unsatisfiable by any correct impl. Read each receiver once into a local; the three assertions are unchanged (approved by Tin Dang 2026-06-14, the minimal intent-preserving fix; not a weakening). §4 red suite (9 seam tests) now GREEN. Writer-loop wiring + crash tests follow. author: Tin Dang
Replace the per-message write+fsync+ack in every AOF writer loop with the group-commit seam: receive one message, opportunistically drain the ready queue into a bounded batch, write all records, make them durable with ONE flush()+sync_data(), then ack every AppendSync `Synced`. Under appendfsync=always with C>1 concurrent durable writers this amortizes the fsync across the batch (the ~11× write penalty), without weakening the fsync-before-ack contract. Loops wired (TopLevel/PerShard × monoio/tokio): - TopLevel monoio: commits via the sync `commit_group_commit_batch` over a `FileGroupSink` (plain RESP bytes; the metric is recorded on the batch fsync). - TopLevel tokio / PerShard tokio / PerShard monoio: replicate the same invariant inline (async I/O can't impl a sync sink; per-shard prepends the framed `[u64 lsn][u32 len]` header) and ack through the shared `group_commit::ack_batch` so the durability ordering is single-sourced and unit-tested via commit. Invariants preserved exactly: - M3 control safety: Rewrite/RewriteSharded/RewritePerShard/Shutdown are never absorbed into a batch — `collect_group_commit_batch` defers them to `deferred_control`, handled AFTER the batch is flushed (batch_straddles_control structurally impossible). The C4 per-shard fold + post-fold drain are unchanged. - M2 H1-BARRIER: a zero-length AppendSync writes no record; the single batch fsync still proves all preceding Append bytes durable (ordered channel). - M4 everysec/no: group commit engages only under Always (do_fsync = fsync==Always); AppendSync is enqueued only under Always (pool::try_send_append_durable / fsync_barrier), so everysec/no batches are Append-only and the 1s deadline / proactive fsync + bounded-recv paths are retained verbatim. - Torn-write latch: a write_all failure latches `write_error` (stream may be torn) and fails every AppendSync waiter WriteFailed; the tokio TEST_FAIL_WRITE_AT torn-write injection is preserved (header-only write at the failing ordinal). - Fault injection: MOON_TEST_AOF_FSYNC_FAIL resolves the batch FsyncFailed. Behavioral refinement (flagged for verify): write_error now latches ONLY on a write failure (torn stream), not on an fsync failure. A failed fsync leaves the bytes intact-but-unsynced (not torn), so a transient fsync error is recoverable on the next batch rather than permanently disabling the writer. Uniform across all four loops; the reject contract (batch_fsync_failed) requires FsyncFailed acks, not a latch. No crash-matrix test exercises a real fsync-fail-then-latch path. Both runtimes: compile clean, clippy clean, AOF unit tests pass (74 tokio / 70 monoio incl. the tokio torn-write latch test), §4 seam suite green. author: Tin Dang
… SIGKILL Add the end-to-end durability scenarios the unit seam cannot prove (on-disk survival across a real kill), spawning the release binary under appendfsync=always with CONCURRENT durable writers — the cell group commit lives in. `#[ignore]`; require ./target/release/moon + redis-cli; run on the VM. - concurrent_writers_all_acked_survive_sigkill_top_level (shards=1) and _per_shard (shards=4): N=16 connections each drive 60 durable INCRs on distinct counters; all joined (every counted INCR acked ⇒ fsynced) before SIGKILL; recovery must show each counter == its acked count — every acked write durable, none lost, none double-applied (INCR is non-idempotent). shards=1 exercises the TopLevel commit_group_commit_batch path; shards=4 the per-shard framed path. [M2 every_acked_write_survives_crash + interrupted_batch clean reload] - lone_writer_fsyncs_immediately (C=1): a lone durable writer is a batch of 1 — fsync immediate, ack Synced, no loss after SIGKILL. [M4 lone_writer_no_added_latency] - rewrite_during_active_writes_no_loss: BGREWRITEAOF fired repeatedly while 12 concurrent durable writers run; the Rewrite control must flush its in-progress batch BEFORE it is handled; post-rewrite replay loses no acked write. [M3 control_message_breaks_the_batch] author: Tin Dang
… commit The group-commit rewrite gave the tokio TopLevel AOF writer a per-iteration local `write_failed` but no persistent `write_error` latch — unlike the other three writer loops (monoio TopLevel, tokio/monoio PerShard), which all latch a write failure for the writer's lifetime. After a torn batch write the tokio TopLevel loop would keep appending on the next batch, risking a silent durability claim past a partial record. This was a pre-existing gap that the group_commit contract makes explicit: `CommitOutcome.write_failed` ⇒ "the caller engages its write_error latch". Mirror the monoio TopLevel exactly: - declare a lifetime `write_error` latch; on a batch write failure, latch it and ack every AppendSync waiter WriteFailed (never ack into a corrupt stream); - once latched, drop appends and fail all waiters until a successful rewrite, which replaces the file with a clean one and resets the latch; - gate every final/deadline sync (cancel, disconnect, Shutdown, EverySec deadline, pre-rewrite) on `!write_error` — syncing past a tear cannot recover it and risks a false durability signal. Also restore the MOON_TEST_AOF_FSYNC_FAIL injection on this path (it existed only on monoio TopLevel before) so the AOF_FSYNC_ERR wire response is exercised identically under both runtimes for shards=1 — making `aof_fsync_err_propagates_before_subscribe_single_shard` valid against a tokio binary, not just the default monoio one. Defense-in-depth: a debug_assert that an everysec/no batch carries no AppendSync (those are enqueued only under Always via pool::try_send_append_durable / fsync_barrier) before acking Synced. Both runtimes compile clean; clippy clean on both; the 9 group-commit seam tests stay green. author: Tin Dang
…marker
Record the phase transition (tests -> build) after the §4 red suite went
green, and rename the §3 freeze label to the engine-required marker phrase
("Least-sure flag surfaced at freeze:") with all frozen substance preserved
— a marker alignment, not a contract reshape.
author: Tin Dang
…rify §6 VERIFY (dual-runtime, VM-local clone @12681b3): - seam 9/9 · AOF lib 94 tokio/90 monoio · integration crash 4/4 ×both (incl. concurrent_writers_all_acked_survive_sigkill_top_level — covers the Finding-2 tokio-TopLevel latch) · crash-matrix 5/5 ×both · consistency 197/197 - WIRING + DEAD-CODE clean; clippy ×2 + fmt clean; zero new unsafe/locks - one frozen §4 test got a human-approved intent-preserving fix (flume bounded(1) use-after-consume); CONTRACT unchanged M0/M1 perf: the win's mechanism (K AppendSync -> 1 fsync) is proven by the deterministic seam test, but the empirical RELATIVE throughput delta is NOT resolvable on the OrbStack VM — its virtio fsync is near-free (always ~0.9M RPS, no 11x penalty), so the writer drains each AppendSync before the next arrives and a coalescing batch never forms (batch~1; 0.98x ratio, conc-sweep no-trend within +/-10% VM noise). This is §1 assumption #4 (lowest-confidence perf flag) confirmed: the VM is an invalid instrument for this metric; the M0/M1 ratio needs a slow-fsync instrument (real disk / GCloud). Deferred to §7 OBSERVE. GATE: PENDING — conservative autonomy stops for human decision (no auto-PASS). author: Tin Dang
Human gate decision (conservative autonomy): PASS — defer the empirical M0/M1 RPS ratio to §7 OBSERVE. Correctness/durability (M2) · ordering (M3) · no-regression (M4) · all 5 Rejects proven green on both runtimes; the coalescing-win mechanism proven by the deterministic seam test. The empirical throughput ratio is un-measurable on the OrbStack VM (near-free fsync) and is tracked as the OBSERVE monitor (real-disk / GCloud bench). v2-performance: 2/2 tasks done (xshard-read-fastpath + wal-group-commit). 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 due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR introduces AOF group-commit batching. A new ChangesWAL Group Commit Batching
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
note over AofWriterLoop: Group Commit Batch Cycle
AofWriterLoop->>collect_group_commit_batch: first_msg + try_next() + caps
collect_group_commit_batch-->>AofWriterLoop: GroupCommitBatch{data, deferred_control}
end
rect rgba(144, 238, 144, 0.5)
note over AofWriterLoop,GroupCommitSink: Write + Sync Phase
AofWriterLoop->>commit_group_commit_batch: sink, batch, do_fsync
loop each msg in batch.data
commit_group_commit_batch->>GroupCommitSink: write_all(msg_body)
end
commit_group_commit_batch->>GroupCommitSink: sync() [if do_fsync]
commit_group_commit_batch->>AppendSyncWaiters: AofAck(Synced|WriteFailed|FsyncFailed)
commit_group_commit_batch-->>AofWriterLoop: CommitOutcome
end
rect rgba(255, 165, 0, 0.5)
note over AofWriterLoop: Deferred Control Dispatch
AofWriterLoop->>AofWriterLoop: match deferred_control
alt Rewrite / RewriteSharded / RewritePerShard
AofWriterLoop->>RewriteHandler: trigger rewrite flow
else Shutdown
AofWriterLoop->>AofWriterLoop: break loop
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
The Lint CHANGELOG gate requires every PR to either touch CHANGELOG.md or carry the skip-changelog label. WAL group commit is a shippable performance feature, so it gets a real [Unreleased] entry rather than a skip label. author: Tin Dang
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.add/tasks/wal-group-commit/TASK.md (1)
231-231: 💤 Low valueAdd language specifier to fenced code block.
Line 231 opens a code block without a language tag. This should be
```rustto properly render the syntax highlighting in markdown viewers.-``` +```rust // ---- Bounds (design-for-failure: a drain is never unbounded) ---- const AOF_GROUP_COMMIT_MAX_BATCH: usize // hard cap on messages per batch (e.g. 1024) const AOF_GROUP_COMMIT_MAX_BYTES: usize // hard cap on bytes buffered per batch (e.g. 8 MiB)🤖 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 @.add/tasks/wal-group-commit/TASK.md at line 231, The markdown code block at line 231 is missing the language specifier for syntax highlighting. Change the opening fence from ``` to ```rust to properly enable Rust syntax highlighting in markdown viewers.tests/wal_group_commit.rs (1)
256-260: ⚡ Quick winConsider verifying
r1isWriteFailedfor complete coverage.The test verifies
r2isWriteFailedbut only checks thatr1is notSynced. Per the contract inack_batch, all waiters should receiveWriteFailed. Adding the explicit assertion would strengthen the test.🧪 Proposed fix
let r1 = rx1.try_recv(); let r2 = rx2.try_recv(); - assert_ne!(r1, Ok(AofAck::Synced)); + assert_eq!(r1, Ok(AofAck::WriteFailed), "first waiter must also be acked WriteFailed"); assert_ne!(r2, Ok(AofAck::Synced)); assert_eq!(r2, Ok(AofAck::WriteFailed));🤖 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/wal_group_commit.rs` around lines 256 - 260, The test in the try_recv section checks that r1 is not Synced but does not explicitly verify it is WriteFailed, while r2 has an explicit assertion for WriteFailed. Per the contract in ack_batch, all waiters should receive WriteFailed. Add an explicit assertion that r1 equals Ok(AofAck::WriteFailed) after the assert_ne check to match the verification done for r2 and ensure complete test coverage.src/persistence/aof/writer_task.rs (1)
882-886: 💤 Low valueConsider adding debug_assert for everysec/no invariant in per-shard paths (consistency with TopLevel).
The tokio TopLevel path (line 446-452) has a
debug_assert!verifying thateverysec/nobatches contain noAppendSyncmessages. Both per-shard paths lack this assertion. While the pool code enforces this invariant, adding the same assertion maintains consistency and catches violations earlier during development.
src/persistence/aof/writer_task.rs#L882-L886: Add debug_assert before returningBatchAck::Syncedin the tokio per-shard everysec/no branch.src/persistence/aof/writer_task.rs#L1198-L1202: Add the same debug_assert in the monoio per-shard everysec/no branch.🤖 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/writer_task.rs` around lines 882 - 886, Add a debug_assert statement for the everysec/no invariant in both per-shard paths to match the consistency check already present in the TopLevel path. At src/persistence/aof/writer_task.rs lines 882-886 (tokio per-shard everysec/no branch), add a debug_assert before returning BatchAck::Synced to verify that the batch contains no AppendSync waiters. Apply the identical assertion at src/persistence/aof/writer_task.rs lines 1198-1202 (monoio per-shard everysec/no branch) in the same position. Both assertions should verify that the batch does not contain any AppendSync messages, maintaining consistency with the existing TopLevel path validation and catching potential invariant violations earlier during development.
🤖 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.
Nitpick comments:
In @.add/tasks/wal-group-commit/TASK.md:
- Line 231: The markdown code block at line 231 is missing the language
specifier for syntax highlighting. Change the opening fence from ``` to ```rust
to properly enable Rust syntax highlighting in markdown viewers.
In `@src/persistence/aof/writer_task.rs`:
- Around line 882-886: Add a debug_assert statement for the everysec/no
invariant in both per-shard paths to match the consistency check already present
in the TopLevel path. At src/persistence/aof/writer_task.rs lines 882-886 (tokio
per-shard everysec/no branch), add a debug_assert before returning
BatchAck::Synced to verify that the batch contains no AppendSync waiters. Apply
the identical assertion at src/persistence/aof/writer_task.rs lines 1198-1202
(monoio per-shard everysec/no branch) in the same position. Both assertions
should verify that the batch does not contain any AppendSync messages,
maintaining consistency with the existing TopLevel path validation and catching
potential invariant violations earlier during development.
In `@tests/wal_group_commit.rs`:
- Around line 256-260: The test in the try_recv section checks that r1 is not
Synced but does not explicitly verify it is WriteFailed, while r2 has an
explicit assertion for WriteFailed. Per the contract in ack_batch, all waiters
should receive WriteFailed. Add an explicit assertion that r1 equals
Ok(AofAck::WriteFailed) after the assert_ne check to match the verification done
for r2 and ensure complete test coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d91ecb5-c00c-4947-b38e-e706b503bca7
📒 Files selected for processing (6)
.add/state.json.add/tasks/wal-group-commit/TASK.mdsrc/persistence/aof/group_commit.rssrc/persistence/aof/mod.rssrc/persistence/aof/writer_task.rstests/wal_group_commit.rs
TinDang97
added a commit
that referenced
this pull request
Jun 15, 2026
The cross-shard idle-gated reply-spin entry under [Unreleased] was the only v2-performance changelog entry without its originating PR number, while the sibling WAL group-commit (#178) and FT.SEARCH off-event-loop (#179) entries both carry one. Add the "(PR #177)" tag so all three v2 stack entries are consistently attributable. Documentation-only; no code, test, or behavior change. author: Tin Dang Co-authored-by: Tin Dang <tin.dang@trustifytechnology.com>
This was referenced Jul 4, 2026
Merged
Merged
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WAL group commit — coalesce concurrent
appendfsync=alwayswrites into one fsyncv2-performance bottleneck #3. Under
appendfsync=alwaysthe AOF writer used toflush()+sync_data()once perAppendSync(the ~11× write penalty). This batches concurrent pending writes via opportunistic drain: after one message, drain whatever else is queued into a bounded batch, write all, ONE fsync, ack all — amortizing the per-write fsync across write concurrency without weakening the fsync-before-ack contract, the C4 cross-shard fold, the H1-BARRIER, or the everysec/no paths.What changed
src/persistence/aof/group_commit.rs:collect_group_commit_batch(bounded drain, control messages defer — never batched) +commit_group_commit_batch(write-all → ONE fsync → ack-all) +GroupCommitSink+ sharedack_batch(single-sourced ack-after-fsync ordering).{aof_writer_task TopLevel, per_shard_aof_writer_task} × {monoio sync, tokio async}. Control messages (Rewrite/RewriteSharded/RewritePerShard/Shutdown) route todeferred_control⇒batch_straddles_controlis structurally impossible.2750d1c): the tokio-TopLevel loop never carried thewrite_errortorn-write latch (nor theMOON_TEST_AOF_FSYNC_FAILinjection) — a pre-existing gap the new contract (CommitOutcome.write_failed ⇒ latch must engage) made explicit. Now mirrors the other 3 loops; covered by the top_level SIGKILL test.Verification (dual-runtime, VM-local clone)
concurrent_writers_all_acked_survive_sigkill_top_level)crash_matrix_per_shard_aof+..._bgrewriteaof)-D warnings×2 +fmtclean · zero newunsafe· zero new cross-thread lockDurability invariant preserved (M2)
acked ⇒ on disk: a waiter is ackedSyncedonly after the covering batch fsync returns. The single batch fsync also makes every preceding fire-and-forgetAppenddurable (H1-BARRIER ordered-channel property). A crash mid-batch leaves at most a torn tail record → existing replay truncates to the last valid record.The coalescing mechanism (K
AppendSync→ 1 fsync) is proven by the deterministic seam testcommit_one_fsync_many_acks. The empirical RPS ratio could not be measured on the OrbStack VM: its virtiofsyncis near-free (always≈ 0.9M RPS, not the 11× penalty), so the writer drains eachAppendSyncbefore the next arrives and a coalescing batch never forms (batch≈1; 0.98× ratio, no trend across a C∈{32,128,256,512} sweep). This was the §1 lowest-confidence perf flag (assumption #4), now confirmed. The before/after ratio needs a slow-fsync instrument (real disk / GCloud); tracked as the §7 OBSERVE monitor.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
appendfsync=alwaysis enabled by coalescing concurrent durable writes into batch commits, reducing the number of fsyncs while preserving ordering and crash safety.Tests