fix(persistence): TopLevel-monoio AOF writer honors EverySec bound when idle#233
Merged
Merged
Conversation
…en idle The TopLevel monoio AOF writer blocked on an UNTIMED rx.recv(), with its EverySec deadline check living only inside the batch-commit path. A batch written under appendfsync everysec gets no per-batch fsync, so if the client stopped writing right after a burst, the buffered bytes only became durable when the NEXT message happened to arrive — the 1s fsync bound was deferred indefinitely while idle. Exposure is host-crash-only: the per-batch flush() already reaches the kernel page cache, so a plain process kill (and therefore any kill-9 crash test) loses nothing — which is exactly why this can't be red/green tested from userspace and is instead validated by mirroring the audited PerShard pattern plus full AOF regression reruns. The loop now matches the PerShard writers line-for-line: - bounded rx.recv_timeout() on the wave-5 IdleWait ladder (50ms -> 250ms -> 1s; reset on message, escalate on empty timeout) - mark_pending() after an everysec batch is buffered without its fsync, pinning the poll at the 50ms floor until the fsync lands - the EverySec deadline check moves from inside the batch path to the END of the loop, so it runs on message AND timeout iterations — timeout wake-ups are what restore the ~1s bound while idle - clear_pending() when the proactive fsync succeeds, letting the idle cadence escalate again Also updates the IdleWait struct docs, which claimed "TopLevel monoio blocks on an untimed rx.recv() and needs none of this" — that was the documented follow-up from the wave-5 review, now closed. Validation: clippy clean on both feature sets; AOF regression suites rerun green under default (monoio) features — persistence lib tests, wal_group_commit, coordinator_local_leg_durability (incl. ignored), crash_matrix_per_shard_aof, aof_toplevel_multishard_refusal, aof_fsync_err_subscribe_ordering. Follow-up from the RSS/CPU wave-5 review (PR #232). author: Tin Dang
…rver tests Two pre-existing harness defects in crash_matrix_per_shard_aof.rs surfaced while validating the TopLevel-monoio idle-fsync fix; both convert environmental conditions into bogus total-data-loss failures: 1. redis_set asserted only redis-cli's EXIT STATUS, which is 0 even for server ERROR replies. With the host disk under Moon's 5% diskfull guard, every SET replied "MOONERR diskfull: writes paused" yet the test sailed on and later reported "200 missing, 0 mismatched" — pointing at the recovery path instead of the guard. The helper now pins the reply to +OK and names the likely culprit (and the MOON_DISK_FREE_MIN_PCT=0 escape hatch) in the assert message. 2. The three server-spawning tests SPLIT-BRAIN when run in parallel (the libtest default): unique_port() hands out OS-sequential ephemeral ports, the other two tests offset by +1/+2, and moon's per-shard SO_REUSEPORT listeners bind an already-taken port WITHOUT an error — so one test's redis-cli traffic silently lands on another test's server, observed as rotating "200 missing" / "LLEN 0 expected 20" false alarms (which test fails depends on OS port allocation order). A shared static Mutex now serializes the three tests; costs ~seconds, kills the whole interference class. Validated: suite green under default parallel threads AND --test-threads=1, with MOON_DISK_FREE_MIN_PCT=0 exported (host disk currently 96% full — below the guard). 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 fixes the monoio TopLevel AOF writer's EverySec fsync deadline, which could be deferred indefinitely during idle periods, by switching to bounded ChangesAOF Writer EverySec Fix
Crash Matrix Test Hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 9: The changelog heading still contains the placeholder PR reference for
the TopLevel-monoio AOF writer fix; update the entry in CHANGELOG.md to replace
“(PR `#TBD`)” with the actual merged PR number, keeping the rest of the heading
unchanged.
In `@src/persistence/aof/writer_task.rs`:
- Around line 438-506: The monoio rewrite handling in writer_task’s deferred
control message match resets write_error after successful Rewrite and
RewriteSharded, but it does not refresh the fsync scheduling state. Update the
successful paths in do_rewrite_single/do_rewrite_sharded handling to mirror the
tokio behavior by back-dating last_fsync and calling idle_wait.mark_pending(),
so the next proactive fsync is not delayed after a rewrite.
In `@tests/crash_matrix_per_shard_aof.rs`:
- Around line 41-56: Replace the shared test lock in serialize_server_test by
using parking_lot::Mutex instead of std::sync::Mutex, and keep the same
SERVER_TEST_LOCK/serialize_server_test flow so the serialization behavior
remains unchanged. Update the lock initialization and guard handling to use
parking_lot’s API, removing the std::sync import entirely while preserving the
poison-free locking semantics expected by the project guidelines.
🪄 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: 9c9e4cde-4858-44e3-bd59-c52ec77b2b85
📒 Files selected for processing (3)
CHANGELOG.mdsrc/persistence/aof/writer_task.rstests/crash_matrix_per_shard_aof.rs
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Fixed — TopLevel-monoio AOF writer: EverySec fsync deferred indefinitely when idle (PR #TBD) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Placeholder PR number left in changelog heading.
(PR #TBD) should be replaced with the actual PR number before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 9, The changelog heading still contains the placeholder
PR reference for the TopLevel-monoio AOF writer fix; update the entry in
CHANGELOG.md to replace “(PR `#TBD`)” with the actual merged PR number, keeping
the rest of the heading unchanged.
Comment on lines
+438
to
506
| // -- handle the control message that ended the drain (if any) -- | ||
| // A control message is NEVER absorbed into the batch: the batch above | ||
| // is already committed before the control message is handled | ||
| // (batch_straddles_control is structurally impossible). | ||
| match batch.deferred_control { | ||
| None => {} | ||
| Some(AofMessage::Shutdown) => { | ||
| if !write_error { | ||
| if let Err(e) = file.flush().and_then(|_| file.sync_data()) { | ||
| error!("AOF final sync failed (seq {}): {}", manifest.seq, e); | ||
| } | ||
| } | ||
| info!("AOF writer shutting down (monoio, seq {})", manifest.seq); | ||
| break; | ||
| } | ||
| info!("AOF writer shutting down (monoio, seq {})", manifest.seq); | ||
| break; | ||
| } | ||
| Some(AofMessage::Rewrite(db)) => { | ||
| if !write_error { | ||
| if let Err(e) = file.flush().and_then(|_| file.sync_data()) { | ||
| error!("AOF pre-rewrite sync failed (seq {}): {}", manifest.seq, e); | ||
| Some(AofMessage::Rewrite(db)) => { | ||
| if !write_error { | ||
| if let Err(e) = file.flush().and_then(|_| file.sync_data()) { | ||
| error!("AOF pre-rewrite sync failed (seq {}): {}", manifest.seq, e); | ||
| } | ||
| } | ||
| } | ||
| match do_rewrite_single(&db, &mut manifest, &mut file, &rx) { | ||
| Ok(()) => { | ||
| write_error = false; // Reset on successful rewrite | ||
| match do_rewrite_single(&db, &mut manifest, &mut file, &rx) { | ||
| Ok(()) => { | ||
| write_error = false; // Reset on successful rewrite | ||
| } | ||
| Err(e) => error!("AOF rewrite failed (seq {}): {}", manifest.seq, e), | ||
| } | ||
| Err(e) => error!("AOF rewrite failed (seq {}): {}", manifest.seq, e), | ||
| crate::command::persistence::AOF_REWRITE_IN_PROGRESS | ||
| .store(false, std::sync::atomic::Ordering::SeqCst); | ||
| } | ||
| crate::command::persistence::AOF_REWRITE_IN_PROGRESS | ||
| .store(false, std::sync::atomic::Ordering::SeqCst); | ||
| } | ||
| Some(AofMessage::RewriteSharded(shard_dbs)) => { | ||
| if !write_error { | ||
| if let Err(e) = file.flush().and_then(|_| file.sync_data()) { | ||
| error!("AOF pre-rewrite sync failed (seq {}): {}", manifest.seq, e); | ||
| Some(AofMessage::RewriteSharded(shard_dbs)) => { | ||
| if !write_error { | ||
| if let Err(e) = file.flush().and_then(|_| file.sync_data()) { | ||
| error!("AOF pre-rewrite sync failed (seq {}): {}", manifest.seq, e); | ||
| } | ||
| } | ||
| } | ||
| // C4 TopLevel cooperative fold: pass the wired fold channels | ||
| // (producer + notifier for shard 0) so do_rewrite_sharded can | ||
| // use the AofFold SPSC protocol instead of the deleted RwLock | ||
| // path. `fold_channels` is `None` only if main.rs failed to | ||
| // wire them at startup (Arc::get_mut race — logged at boot). | ||
| match do_rewrite_sharded( | ||
| &shard_dbs, | ||
| &mut manifest, | ||
| &mut file, | ||
| &rx, | ||
| fold_channels.as_ref(), | ||
| ) { | ||
| Ok(()) => { | ||
| write_error = false; | ||
| // C4 TopLevel cooperative fold: pass the wired fold channels | ||
| // (producer + notifier for shard 0) so do_rewrite_sharded can | ||
| // use the AofFold SPSC protocol instead of the deleted RwLock | ||
| // path. `fold_channels` is `None` only if main.rs failed to | ||
| // wire them at startup (Arc::get_mut race — logged at boot). | ||
| match do_rewrite_sharded( | ||
| &shard_dbs, | ||
| &mut manifest, | ||
| &mut file, | ||
| &rx, | ||
| fold_channels.as_ref(), | ||
| ) { | ||
| Ok(()) => { | ||
| write_error = false; | ||
| } | ||
| Err(e) => error!("AOF rewrite failed (seq {}): {}", manifest.seq, e), | ||
| } | ||
| Err(e) => error!("AOF rewrite failed (seq {}): {}", manifest.seq, e), | ||
| crate::command::persistence::AOF_REWRITE_IN_PROGRESS | ||
| .store(false, std::sync::atomic::Ordering::SeqCst); | ||
| } | ||
| crate::command::persistence::AOF_REWRITE_IN_PROGRESS | ||
| .store(false, std::sync::atomic::Ordering::SeqCst); | ||
| // [F6] A TopLevel writer never owns per-shard files; receiving | ||
| // RewritePerShard means a routing bug. Self-abort so the | ||
| // coordinator's countdown completes and the flag clears. | ||
| Some(AofMessage::RewritePerShard { coord, .. }) => { | ||
| warn!( | ||
| "AOF TopLevel writer received RewritePerShard — routing bug; aborting" | ||
| ); | ||
| coord.mark_failed(); | ||
| coord.shard_done(); | ||
| } | ||
| // collect_group_commit_batch only ever defers a control message. | ||
| Some(_) => {} | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the PerShard writer's rewrite handling back-dates
# last_fsync/marks idle_wait pending, to confirm the intended parity.
rg -n "last_fsync|idle_wait" src/persistence/aof/*.rs | rg -n "rewrite" -A5 -B5 -i
fd -e rs . src/persistence/aofRepository: pilotspace/moon
Length of output: 310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline src/persistence/aof/writer_task.rs --view expanded
printf '\n---\n'
ast-grep outline src/persistence/aof/rewrite.rs --view expanded
printf '\n---\n'
ast-grep outline src/persistence/aof/mod.rs --view expandedRepository: pilotspace/moon
Length of output: 5156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the monoio writer loop around the rewrite handling.
sed -n '430,520p' src/persistence/aof/writer_task.rs
printf '\n--- TOKIO / TOPLEVEL REWRITE HANDLING ---\n'
sed -n '680,770p' src/persistence/aof/writer_task.rs
printf '\n--- REWRITE HELPERS ---\n'
sed -n '780,980p' src/persistence/aof/rewrite.rsRepository: pilotspace/moon
Length of output: 19393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every place the writer back-dates fsync or marks pending,
# to compare monoio vs tokio handling around rewrites.
rg -n "last_fsync\s*=|mark_pending\(\)" src/persistence/aof/writer_task.rsRepository: pilotspace/moon
Length of output: 1313
Back-date fsync state after monoio rewrites src/persistence/aof/writer_task.rs:453-493 — after a successful Rewrite / RewriteSharded, the monoio path resets write_error but leaves last_fsync and idle_wait untouched. That can leave the post-rewrite backlog on an escalated timeout and delay the next proactive fsync by up to the full idle cadence. Mirror the tokio path here with the same back-date + idle_wait.mark_pending().
🤖 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 438 - 506, The monoio
rewrite handling in writer_task’s deferred control message match resets
write_error after successful Rewrite and RewriteSharded, but it does not refresh
the fsync scheduling state. Update the successful paths in
do_rewrite_single/do_rewrite_sharded handling to mirror the tokio behavior by
back-dating last_fsync and calling idle_wait.mark_pending(), so the next
proactive fsync is not delayed after a rewrite.
Comment on lines
+41
to
+56
| /// Serializes the server-spawning tests in this binary. Run in parallel they | ||
| /// intermittently SPLIT-BRAIN: `unique_port()` hands out OS-sequential | ||
| /// ephemeral ports and the other tests offset by +1/+2, so two concurrently | ||
| /// starting tests can land on the SAME port — and moon's per-shard | ||
| /// SO_REUSEPORT listeners bind it without an error, silently splitting one | ||
| /// test's redis-cli traffic across another test's server (observed as "200 | ||
| /// missing" total-loss false alarms). A shared lock costs ~seconds and kills | ||
| /// the whole class. | ||
| static SERVER_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); | ||
|
|
||
| fn serialize_server_test() -> std::sync::MutexGuard<'static, ()> { | ||
| SERVER_TEST_LOCK | ||
| .lock() | ||
| .unwrap_or_else(|poisoned| poisoned.into_inner()) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use parking_lot::Mutex instead of std::sync::Mutex.
Coding guidelines require parking_lot locks and prohibit std::sync locks project-wide.
🔧 Proposed fix
-static SERVER_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
+static SERVER_TEST_LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(());
-fn serialize_server_test() -> std::sync::MutexGuard<'static, ()> {
- SERVER_TEST_LOCK
- .lock()
- .unwrap_or_else(|poisoned| poisoned.into_inner())
-}
+fn serialize_server_test() -> parking_lot::MutexGuard<'static, ()> {
+ SERVER_TEST_LOCK.lock()
+}As per coding guidelines, "Use parking_lot::RwLock and parking_lot::Mutex; never use std::sync locks."
📝 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
| /// Serializes the server-spawning tests in this binary. Run in parallel they | |
| /// intermittently SPLIT-BRAIN: `unique_port()` hands out OS-sequential | |
| /// ephemeral ports and the other tests offset by +1/+2, so two concurrently | |
| /// starting tests can land on the SAME port — and moon's per-shard | |
| /// SO_REUSEPORT listeners bind it without an error, silently splitting one | |
| /// test's redis-cli traffic across another test's server (observed as "200 | |
| /// missing" total-loss false alarms). A shared lock costs ~seconds and kills | |
| /// the whole class. | |
| static SERVER_TEST_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); | |
| fn serialize_server_test() -> std::sync::MutexGuard<'static, ()> { | |
| SERVER_TEST_LOCK | |
| .lock() | |
| .unwrap_or_else(|poisoned| poisoned.into_inner()) | |
| } | |
| /// Serializes the server-spawning tests in this binary. Run in parallel they | |
| /// intermittently SPLIT-BRAIN: `unique_port()` hands out OS-sequential | |
| /// ephemeral ports and the other tests offset by +1/+2, so two concurrently | |
| /// starting tests can land on the SAME port — and moon's per-shard | |
| /// SO_REUSEPORT listeners bind it without an error, silently splitting one | |
| /// test's redis-cli traffic across another test's server (observed as "200 | |
| /// missing" total-loss false alarms). A shared lock costs ~seconds and kills | |
| /// the whole class. | |
| static SERVER_TEST_LOCK: parking_lot::Mutex<()> = parking_lot::Mutex::new(()); | |
| fn serialize_server_test() -> parking_lot::MutexGuard<'static, ()> { | |
| SERVER_TEST_LOCK.lock() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/crash_matrix_per_shard_aof.rs` around lines 41 - 56, Replace the shared
test lock in serialize_server_test by using parking_lot::Mutex instead of
std::sync::Mutex, and keep the same SERVER_TEST_LOCK/serialize_server_test flow
so the serialization behavior remains unchanged. Update the lock initialization
and guard handling to use parking_lot’s API, removing the std::sync import
entirely while preserving the poison-free locking semantics expected by the
project guidelines.
Source: Coding guidelines
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
Closes the follow-up documented in the wave-5 review (PR #232): the TopLevel monoio AOF writer blocked on an untimed
rx.recv(), with its EverySec deadline check living only inside the batch-commit path. A batch written underappendfsync everysecgets no per-batch fsync, so if the client stopped writing right after a burst, the buffered bytes only became durable when the next message happened to arrive — the 1s fsync bound was deferred indefinitely while idle.Exposure is host-crash-only: the per-batch
flush()already reaches the kernel page cache, so a plain process kill (and therefore any kill-9 crash test) loses nothing — which is exactly why this can't be red/green tested from userspace and is instead validated by mirroring the audited PerShard pattern plus full AOF regression reruns.Changes
src/persistence/aof/writer_task.rs: the loop now matches the PerShard writers line-for-line — boundedrecv_timeouton the wave-5IdleWaitladder (50ms → 250ms → 1s; reset on message, escalate on empty timeout),mark_pending()after an everysec batch is buffered unfsynced (pins the poll at the 50ms floor), and the EverySec deadline check moved from inside the batch path to the end of the loop so it runs on message AND timeout iterations.IdleWaitstruct docs updated (they claimed TopLevel monoio needed none of this).tests/crash_matrix_per_shard_aof.rsharness hardening (2nd commit; both defects pre-existing, found while validating): (1)redis_setnow pins the reply to+OK— redis-cli exits 0 even forMOONERR diskfullerror replies, which turned a tripped diskfull guard into a bogus 'all 200 keys missing after recovery'; (2) the three server-spawning tests are serialized via a shared mutex — run in parallel they split-brain (OS-sequential ephemeral ports ++1/+2offsets + SO_REUSEPORT binding an already-taken port without error).Validation
-D warningsclean on default (monoio) andruntime-tokio,jemalloc; tests compile under both.idle_wait_tests),wal_group_commit×13,coordinator_local_leg_durability×7 (incl. ignored),crash_matrix_per_shard_aof×3 (parallel AND--test-threads=1),aof_toplevel_multishard_refusal×2,aof_fsync_err_subscribe_ordering×2.--shards 2 --appendfsync always→ SET acked → bytes in shard incr AOF → kill -9 → restart → key recovered.Summary by CodeRabbit
Bug Fixes
Documentation