feat(persistence): automatic AOF rewrite + un-gated multi-shard BGREWRITEAOF (#433) - #443
Conversation
…RITEAOF (#433) The AOF was append-only with no compaction path: it grew with write volume, not dataset size (observed live: 4.8 GB appendonlydir over a 2.43 GB dataset, ~1 GB/day) until the diskfull guard paused writes. And on the default multi-shard config even the manual escape hatch was gated off (BGREWRITEAOF refused unless --experimental-per-shard-rewrite). Three changes: 1. Redis-parity automatic rewrite. New flags --auto-aof-rewrite-percentage (default 100, 0 disables) and --auto-aof-rewrite-min-size (default "64mb", size strings accepted; both in CONFIG GET, conf-file keys work via the generic key->flag synthesis). A monitor thread (src/persistence/aof/auto_rewrite.rs) samples the on-disk appendonlydir size once a second and dispatches bgrewriteaof_start_sharded — the exact entry the command uses (CAS in-progress flag, PerShard fan-out vs TopLevel routing) — when current >= min_size and (current-base)*100/max(base,1) >= percentage. Base re-records after boot recovery and each completed rewrite (deterministic post-dispatch wait + flag-transition + shrunk-below- base detection for manual rewrites that finish between ticks). Design-for-failure: failed dispatch arms a 60 s cooldown (no hot-retry livelock); skipped while BGSAVE or a rewrite runs; one directory walk per second, nothing on the hot path. 2. Multi-shard BGREWRITEAOF un-gated. The per-shard fan-out (C4 cooperative snapshot + synchronized manifest commit) is the default; MULTI_SHARD_AOF_REWRITE_UNSAFE is no longer set at boot. --experimental-per-shard-rewrite is deprecated (warn, no-op). The historical ~38%-key-loss gate predated the C4 redesign; the current path holds exact INCR recovery across a straddling rewrite + SIGKILL. 3. INFO persistence reports real AOF state (#432). aof_enabled and aof_rewrite_in_progress were hardcoded 0; now real, plus new aof_base_size / aof_current_size fields exposing the trigger's inputs (current refreshed on read — INFO is cold path). Also fixes the crash-matrix harness that made the per-shard rewrite look broken on this host: crash_matrix_per_shard_bgrewriteaof lacked --disk-free-min-pct 0 and parsed MOONERR-diskfull INCR rejections as -1 — on a root volume hovering at ~4% free the straddle test "lost" 261/500 INCRs that were never acked (reply-capture hunt proved the server exact). The harness now disables the guard and panics on any non-numeric INCR reply; suite green 5/5 consecutive runs. Layout note (found by the dual-runtime run): the tokio TopLevel (shards=1) writer appends to a legacy flat appendonly.aof, not the appendonlydir manifest the monoio writers use — the size sampler measures both (manifest walk + flat-file stat), and its in-place rewrite is detected in tests via shrink-below-high-water sampled DURING the write stream (a post-hoc poll misses a mid-stream compaction that immediately regrows). Tests (red/green): tests/aof_auto_rewrite.rs — un-gated manual rewrite (shards=2, no flag) with SIGKILL-exact recovery; auto trigger fires on growth at shards=2 AND shards=1 with INCR-exact recovery; percentage=0 never triggers; INFO fields real and current>base after writes. All #[ignore]d (spawn real binaries + SIGKILL, crash-suite convention), run explicitly against BOTH runtime binaries — monoio and tokio 5/5 each. Unit tests pin the trigger predicate (boundaries, zero-base=1 Redis rule, min-size floor, overflow). Gates: crash matrix green (repeat runs), shardslice_live fold suite green, full monoio release suite green, clippy -D warnings both feature sets, fmt. No hot-path code touched — bench waived (monitor is 1 stat-walk/s off-thread). Fixes #433 Fixes #432 author: Tin Dang
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds automatic AOF rewrite monitoring with configurable size and growth thresholds. Multi-shard ChangesAOF rewrite lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AOFMonitor
participant AOFStorage
participant BGREWRITEAOF
AOFMonitor->>AOFStorage: Refresh current AOF size
AOFMonitor->>AOFMonitor: Evaluate thresholds and rewrite guards
AOFMonitor->>BGREWRITEAOF: Dispatch sharded rewrite
BGREWRITEAOF->>AOFStorage: Compact AOF generations
AOFMonitor->>AOFStorage: Record new baseline
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
PR Summary by Qodofeat(persistence): auto AOF rewrite + default multi-shard BGREWRITEAOF
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main.rs (1)
1754-1761: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
auto-aof-rewrite-min-sizefallback is duplicated.ServerConfigstores the raw string and exposes no resolved accessor, so two call sites each runServerConfig::parse_sizeand each hardcode64 * 1024 * 1024on failure. The two values agree today only by coincidence. If one default changes,CONFIG GETreports a threshold the monitor does not use.Add one resolver on
ServerConfig, for examplepub fn auto_aof_rewrite_min_size_bytes(&self) -> u64, that owns both the parse and the default, then call it from both sites.
src/main.rs#L1754-L1761: replace the inlineparse_size(...).unwrap_or_else(...)with the new accessor. Keep the startuptracing::warn!for the unparseable case inside the accessor or at this site only.src/command/config.rs#L48-L54: replaceServerConfig::parse_size(&server_config.auto_aof_rewrite_min_size).unwrap_or(64 * 1024 * 1024)with the same accessor.🤖 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/main.rs` around lines 1754 - 1761, Centralize auto-AOF rewrite minimum-size resolution in a new ServerConfig accessor such as auto_aof_rewrite_min_size_bytes, owning parsing and the 64 MiB fallback. Update src/main.rs lines 1754-1761 to use it while preserving the startup warning, and update src/command/config.rs lines 48-54 to use the same accessor instead of parsing and hardcoding the fallback independently.tests/aof_auto_rewrite.rs (1)
200-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or document the unused compacting helper.
wait_for_compactionis not called by any test intests/aof_auto_rewrite.rs, so it will produce an unused-private-function warning. It also cannot observe the in-place shrink path: a freshCompactionTrackerhashigh_water == 0, and the shrink check requireshigh_water > 1024. A new helper should document seq-only semantics if kept, otherwise remove it.🤖 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/aof_auto_rewrite.rs` around lines 200 - 202, Remove the unused wait_for_compaction helper from tests/aof_auto_rewrite.rs; if retaining it, document its seq-only semantics and ensure callers use a CompactionTracker with the required high_water state so it can observe in-place shrinking.Source: Coding guidelines
🤖 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/command/connection.rs`:
- Around line 281-291: Update the AOF size assignment in the INFO-building flow
to read the existing auto-rewrite::AOF_CURRENT_SIZE atomic with the appropriate
relaxed ordering instead of calling refresh_current_size(). Remove the
synchronous filesystem-walk path from this INFO request while preserving the
disabled-AOF value of zero.
In `@src/persistence/aof/auto_rewrite.rs`:
- Around line 137-152: Make the auto-rewrite monitor cancellable: in
src/persistence/aof/auto_rewrite.rs lines 137-152, add a CancellationToken
parameter to spawn_monitor, pass it to monitor_loop, and exit after each TICK
sleep or during the 300-second completion wait when cancellation is set; in
src/main.rs lines 1749-1768, pass cancel_token.child_token() to spawn_monitor.
- Around line 216-229: Update the completion-wait branch around
AOF_REWRITE_IN_PROGRESS so record_base_size() and saw_in_progress reset occur
only after the flag has actually cleared before the deadline. If the wait times
out while the rewrite remains in progress, preserve the existing baseline and
saw_in_progress state so a later tick can detect completion and rebase
correctly.
In `@tests/aof_auto_rewrite.rs`:
- Around line 350-354: Update the max_base_seq assertion in the
auto-aof-rewrite-percentage 0 test to accept any value less than or equal to 1,
preserving the intended no-rewrite behavior while allowing fresh multi-shard
boots that have not created a seq-1 base file.
- Around line 99-107: Add an accurate // SAFETY: comment immediately before the
unsafe block in sigkill, documenting why calling libc::kill with the child
process ID is safe. Leave the surrounding platform-specific termination and wait
behavior unchanged.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 1754-1761: Centralize auto-AOF rewrite minimum-size resolution in
a new ServerConfig accessor such as auto_aof_rewrite_min_size_bytes, owning
parsing and the 64 MiB fallback. Update src/main.rs lines 1754-1761 to use it
while preserving the startup warning, and update src/command/config.rs lines
48-54 to use the same accessor instead of parsing and hardcoding the fallback
independently.
In `@tests/aof_auto_rewrite.rs`:
- Around line 200-202: Remove the unused wait_for_compaction helper from
tests/aof_auto_rewrite.rs; if retaining it, document its seq-only semantics and
ensure callers use a CompactionTracker with the required high_water state so it
can observe in-place shrinking.
🪄 Autofix
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 Plus
Run ID: 60fed3ad-bc0e-4556-b57a-3697cafdf405
📒 Files selected for processing (10)
CHANGELOG.mddocs/runbooks/multi-shard-aof-rewrite.mdsrc/command/config.rssrc/command/connection.rssrc/config.rssrc/main.rssrc/persistence/aof/auto_rewrite.rssrc/persistence/aof/mod.rstests/aof_auto_rewrite.rstests/crash_matrix_per_shard_bgrewriteaof.rs
| // #432: aof_enabled / aof_rewrite_in_progress / sizes are real state, not | ||
| // hardcoded zeros. Sizes come from the auto-rewrite monitor's statics | ||
| // (#433); refresh_current_size keeps `aof_current_size` honest when INFO | ||
| // is read between monitor ticks (one directory walk — INFO is cold path). | ||
| let aof_enabled = crate::persistence::aof::auto_rewrite::AOF_ENABLED | ||
| .load(std::sync::atomic::Ordering::Relaxed); | ||
| let aof_current_size = if aof_enabled { | ||
| crate::persistence::aof::auto_rewrite::refresh_current_size() | ||
| } else { | ||
| 0 | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not run a filesystem walk on every INFO call.
Line 288 calls refresh_current_size(), which calls measure_total_size() in src/persistence/aof/auto_rewrite.rs. That function walks the whole appendonlydir tree recursively and issues one metadata syscall per file, plus one for the legacy flat file.
The comment says INFO is a cold path. Two facts contradict that:
infoat Line 172 ignores_argsand always builds every section. A bareINFOtherefore triggers the walk, not onlyINFO persistence.- Monitoring agents poll
INFOcontinuously, often from several collectors at once.
The walk is synchronous blocking IO on the shard event loop thread that serves the connection, so it stalls command processing for that shard. The cost grows with the number of retained generations and shards.
The auto-rewrite monitor already stores a fresh value in AOF_CURRENT_SIZE every second. Read that atomic instead and accept at most one second of staleness. init() seeds the atomic through record_base_size(), so it is populated before the first INFO.
⚡ Proposed fix
let aof_enabled = crate::persistence::aof::auto_rewrite::AOF_ENABLED
.load(std::sync::atomic::Ordering::Relaxed);
let aof_current_size = if aof_enabled {
- crate::persistence::aof::auto_rewrite::refresh_current_size()
+ // Read the monitor's cached sample (refreshed every TICK). A directory
+ // walk here would run on the shard event loop on every INFO call.
+ crate::persistence::aof::auto_rewrite::AOF_CURRENT_SIZE
+ .load(std::sync::atomic::Ordering::Relaxed)
} else {
0
};📝 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.
| // #432: aof_enabled / aof_rewrite_in_progress / sizes are real state, not | |
| // hardcoded zeros. Sizes come from the auto-rewrite monitor's statics | |
| // (#433); refresh_current_size keeps `aof_current_size` honest when INFO | |
| // is read between monitor ticks (one directory walk — INFO is cold path). | |
| let aof_enabled = crate::persistence::aof::auto_rewrite::AOF_ENABLED | |
| .load(std::sync::atomic::Ordering::Relaxed); | |
| let aof_current_size = if aof_enabled { | |
| crate::persistence::aof::auto_rewrite::refresh_current_size() | |
| } else { | |
| 0 | |
| }; | |
| // `#432`: aof_enabled / aof_rewrite_in_progress / sizes are real state, not | |
| // hardcoded zeros. Sizes come from the auto-rewrite monitor's statics | |
| // (`#433`); refresh_current_size keeps `aof_current_size` honest when INFO | |
| // is read between monitor ticks (one directory walk — INFO is cold path). | |
| let aof_enabled = crate::persistence::aof::auto_rewrite::AOF_ENABLED | |
| .load(std::sync::atomic::Ordering::Relaxed); | |
| let aof_current_size = if aof_enabled { | |
| // Read the monitor's cached sample (refreshed every TICK). A directory | |
| // walk here would run on the shard event loop on every INFO call. | |
| crate::persistence::aof::auto_rewrite::AOF_CURRENT_SIZE | |
| .load(std::sync::atomic::Ordering::Relaxed) | |
| } else { | |
| 0 | |
| }; |
🤖 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/command/connection.rs` around lines 281 - 291, Update the AOF size
assignment in the INFO-building flow to read the existing
auto-rewrite::AOF_CURRENT_SIZE atomic with the appropriate relaxed ordering
instead of calling refresh_current_size(). Remove the synchronous
filesystem-walk path from this INFO request while preserving the disabled-AOF
value of zero.
| pub fn spawn_monitor( | ||
| pool: Arc<super::AofWriterPool>, | ||
| shard_databases: Arc<crate::shard::shared_databases::ShardDatabases>, | ||
| percentage: u64, | ||
| min_size: u64, | ||
| ) { | ||
| let spawned = std::thread::Builder::new() | ||
| .name("aof-auto-rewrite".to_string()) | ||
| .spawn(move || { | ||
| monitor_loop(&pool, &shard_databases, percentage, min_size); | ||
| }); | ||
| if let Err(e) = spawned { | ||
| // Non-fatal: manual BGREWRITEAOF still works; sizes go stale. | ||
| warn!("aof-auto-rewrite monitor failed to spawn: {e}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The auto-rewrite monitor thread cannot be stopped. spawn_monitor takes no CancellationToken, so monitor_loop runs an unbounded loop for the life of the process. Every other auxiliary thread started in src/main.rs takes cancel_token.child_token() (per-shard AOF writers at Line 880, the TopLevel writer at Line 915, the auto-save thread at Line 2016). The monitor can therefore dispatch a rewrite after pool.broadcast_shutdown() runs at src/main.rs Line 2069.
src/persistence/aof/auto_rewrite.rs#L137-L152: add acancel: CancellationTokenparameter tospawn_monitorand forward it tomonitor_loop. Checkcancel.is_cancelled()after eachTICKsleep and inside the 300 s completion wait, and return from the loop when it is set.src/main.rs#L1749-L1768: passcancel_token.child_token()as the new argument tospawn_monitor.
📍 Affects 2 files
src/persistence/aof/auto_rewrite.rs#L137-L152(this comment)src/main.rs#L1749-L1768
🤖 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/auto_rewrite.rs` around lines 137 - 152, Make the
auto-rewrite monitor cancellable: in src/persistence/aof/auto_rewrite.rs lines
137-152, add a CancellationToken parameter to spawn_monitor, pass it to
monitor_loop, and exit after each TICK sleep or during the 300-second completion
wait when cancellation is set; in src/main.rs lines 1749-1768, pass
cancel_token.child_token() to spawn_monitor.
| _ => { | ||
| // Started. Wait for completion here (bounded) so the rebase | ||
| // is deterministic even when the whole rewrite fits inside | ||
| // one tick; the transition/shrink detection above is the | ||
| // fallback for manual rewrites. | ||
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(300); | ||
| while AOF_REWRITE_IN_PROGRESS.load(Ordering::SeqCst) | ||
| && std::time::Instant::now() < deadline | ||
| { | ||
| std::thread::sleep(std::time::Duration::from_millis(100)); | ||
| } | ||
| record_base_size(); | ||
| saw_in_progress = false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not rebase the baseline when the completion wait times out.
If the 300 s deadline expires while AOF_REWRITE_IN_PROGRESS is still true, Line 227 still calls record_base_size(). At that moment both the old and the new AOF generation are on disk, so the measured total is inflated. aof_base_size in INFO persistence then reports a wrong value, and the next growth evaluation is measured against it.
Line 228 also clears saw_in_progress while the flag is still set, so the real completion transition is not detected on a later tick. The shrunk_below_base fallback recovers the correct baseline after the prune, so the state self-corrects, but only after at least one extra tick.
Rebase only when the flag actually cleared.
🐛 Proposed fix
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(300);
while AOF_REWRITE_IN_PROGRESS.load(Ordering::SeqCst)
&& std::time::Instant::now() < deadline
{
std::thread::sleep(std::time::Duration::from_millis(100));
}
- record_base_size();
- saw_in_progress = false;
+ if AOF_REWRITE_IN_PROGRESS.load(Ordering::SeqCst) {
+ // Deadline expired with the rewrite still running. Leave the
+ // baseline alone and let the next tick's transition/shrink
+ // detection rebase it.
+ warn!(
+ "aof-auto-rewrite: rewrite still in progress after 300s; \
+ deferring baseline rebase"
+ );
+ saw_in_progress = true;
+ } else {
+ record_base_size();
+ saw_in_progress = false;
+ }📝 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.
| _ => { | |
| // Started. Wait for completion here (bounded) so the rebase | |
| // is deterministic even when the whole rewrite fits inside | |
| // one tick; the transition/shrink detection above is the | |
| // fallback for manual rewrites. | |
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(300); | |
| while AOF_REWRITE_IN_PROGRESS.load(Ordering::SeqCst) | |
| && std::time::Instant::now() < deadline | |
| { | |
| std::thread::sleep(std::time::Duration::from_millis(100)); | |
| } | |
| record_base_size(); | |
| saw_in_progress = false; | |
| } | |
| _ => { | |
| // Started. Wait for completion here (bounded) so the rebase | |
| // is deterministic even when the whole rewrite fits inside | |
| // one tick; the transition/shrink detection above is the | |
| // fallback for manual rewrites. | |
| let deadline = std::time::Instant::now() + std::time::Duration::from_secs(300); | |
| while AOF_REWRITE_IN_PROGRESS.load(Ordering::SeqCst) | |
| && std::time::Instant::now() < deadline | |
| { | |
| std::thread::sleep(std::time::Duration::from_millis(100)); | |
| } | |
| if AOF_REWRITE_IN_PROGRESS.load(Ordering::SeqCst) { | |
| // Deadline expired with the rewrite still running. Leave the | |
| // baseline alone and let the next tick's transition/shrink | |
| // detection rebase it. | |
| warn!( | |
| "aof-auto-rewrite: rewrite still in progress after 300s; \ | |
| deferring baseline rebase" | |
| ); | |
| saw_in_progress = true; | |
| } else { | |
| record_base_size(); | |
| saw_in_progress = false; | |
| } | |
| } |
🤖 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/auto_rewrite.rs` around lines 216 - 229, Update the
completion-wait branch around AOF_REWRITE_IN_PROGRESS so record_base_size() and
saw_in_progress reset occur only after the flag has actually cleared before the
deadline. If the wait times out while the rewrite remains in progress, preserve
the existing baseline and saw_in_progress state so a later tick can detect
completion and rebase correctly.
| fn sigkill(child: &mut Child) { | ||
| #[cfg(unix)] | ||
| unsafe { | ||
| libc::kill(child.id() as i32, libc::SIGKILL); | ||
| } | ||
| #[cfg(not(unix))] | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a // SAFETY: comment to the unsafe block.
The coding guidelines require an accurate // SAFETY: comment on every unsafe block. Line 101 opens one without it.
🛡️ Proposed fix
fn sigkill(child: &mut Child) {
#[cfg(unix)]
+ // SAFETY: libc::kill with a pid owned by this process and a valid signal
+ // number. The call has no memory-safety preconditions; a reaped pid only
+ // returns ESRCH, which is ignored.
unsafe {
libc::kill(child.id() as i32, libc::SIGKILL);
}As per coding guidelines: "every unsafe block must include an accurate // SAFETY: comment".
📝 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.
| fn sigkill(child: &mut Child) { | |
| #[cfg(unix)] | |
| unsafe { | |
| libc::kill(child.id() as i32, libc::SIGKILL); | |
| } | |
| #[cfg(not(unix))] | |
| let _ = child.kill(); | |
| let _ = child.wait(); | |
| } | |
| fn sigkill(child: &mut Child) { | |
| #[cfg(unix)] | |
| // SAFETY: `libc::kill` has no memory-safety preconditions; `SIGKILL` is valid. | |
| unsafe { | |
| libc::kill(child.id() as i32, libc::SIGKILL); | |
| } | |
| #[cfg(not(unix))] | |
| let _ = child.kill(); | |
| let _ = child.wait(); | |
| } |
🤖 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/aof_auto_rewrite.rs` around lines 99 - 107, Add an accurate // SAFETY:
comment immediately before the unsafe block in sigkill, documenting why calling
libc::kill with the child process ID is safe. Leave the surrounding
platform-specific termination and wait behavior unchanged.
Source: Coding guidelines
| assert_eq!( | ||
| max_base_seq(&dir), | ||
| 1, | ||
| "auto-aof-rewrite-percentage 0 must disable automatic rewrites" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert <= 1 instead of == 1.
The test intent is "no rewrite happened". max_base_seq returns 0 when it finds no moon.aof.<seq>.base.rdb file at all. If a fresh multi-shard boot has not yet materialized a seq-1 base file, this assertion fails even though automatic rewrites were correctly disabled.
💚 Proposed fix
assert_eq!(
max_base_seq(&dir),
1,
"auto-aof-rewrite-percentage 0 must disable automatic rewrites"
);+ assert!(
+ max_base_seq(&dir) <= 1,
+ "auto-aof-rewrite-percentage 0 must disable automatic rewrites; \
+ found a compacted base with seq {}",
+ max_base_seq(&dir)
+ );🤖 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/aof_auto_rewrite.rs` around lines 350 - 354, Update the max_base_seq
assertion in the auto-aof-rewrite-percentage 0 test to accept any value less
than or equal to 1, preserving the intended no-rewrite behavior while allowing
fresh multi-shard boots that have not created a seq-1 base file.
…iterals Three integration tests (workspace_integration, mq_integration, txn_kv_wiring) construct ServerConfig as exhaustive struct literals without a ..default spread; the two new #433 fields broke their compile. CI's tokio `cargo test --no-run` caught it (the local gate ran lib tests only for tokio). All test targets now compile under both default and runtime-tokio,jemalloc feature sets. Refs #433 author: Tin Dang
Code Review by Qodo
1. sigkill() unsafe missing SAFETY
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| fn sigkill(child: &mut Child) { | ||
| #[cfg(unix)] | ||
| unsafe { | ||
| libc::kill(child.id() as i32, libc::SIGKILL); |
There was a problem hiding this comment.
3. sigkill() unsafe missing safety 📘 Rule violation ≡ Correctness
tests/aof_auto_rewrite.rs introduces an unsafe block calling libc::kill without the required adjacent // SAFETY: comment. This violates the repository unsafe policy and increases audit risk around unsafe usage.
Agent Prompt
## Issue description
A new `unsafe` block was added without an adjacent `// SAFETY:` comment, violating `UNSAFE_POLICY.md`.
## Issue Context
`UNSAFE_POLICY.md` requires every `unsafe` block to have a `// SAFETY:` comment describing the upheld preconditions and why UB is avoided. In this case, the unsafe can likely be removed entirely by using the existing safe `Child::kill()` approach (or reusing the safe helper in `tests/common`).
## Fix Focus Areas
- tests/aof_auto_rewrite.rs[99-107]
- UNSAFE_POLICY.md[14-26]
- tests/common/mod.rs[168-175]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| std::thread::sleep(std::time::Duration::from_millis(100)); | ||
| } | ||
| record_base_size(); | ||
| saw_in_progress = false; |
There was a problem hiding this comment.
4. Premature base size rebase 🐞 Bug ≡ Correctness
auto_rewrite::monitor_loop calls record_base_size() and clears saw_in_progress after a bounded 300s wait even if AOF_REWRITE_IN_PROGRESS is still true. This can prevent later rewrite completion detection and leave aof_base_size/auto-trigger math incorrect, suppressing or misfiring subsequent automatic rewrites.
Agent Prompt
### Issue description
The auto-rewrite monitor rebases `AOF_BASE_SIZE` even when a rewrite hasn’t actually finished (rewrite still in progress after the 300s deadline), and it also clears `saw_in_progress`. This breaks the monitor’s state machine: it can miss the eventual `in_progress: true -> false` transition and permanently keep an incorrect baseline.
### Issue Context
- The monitor’s completion detection depends on `saw_in_progress && !in_progress`.
- The timeout path should not be treated as completion.
### Fix Focus Areas
- src/persistence/aof/auto_rewrite.rs[175-229]
### Suggested fix
- After the bounded wait, **check** `AOF_REWRITE_IN_PROGRESS` again:
- If it is still `true`, **do not** call `record_base_size()` and **do not** set `saw_in_progress = false`; optionally log a warning that the rewrite exceeded the wait bound.
- Only call `record_base_size()` (and reset monitor state) once `AOF_REWRITE_IN_PROGRESS` is observed `false` (either via the normal tick-based `completed_since_last_tick` path, or immediately after the wait loop if it actually completed within the deadline).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| let _ = AOF_DIR.set(persistence_dir.join("appendonlydir")); | ||
| let _ = AOF_LEGACY_FILE.set(persistence_dir.join(appendfilename)); | ||
| AOF_ENABLED.store(true, Ordering::Relaxed); | ||
| record_base_size(); |
There was a problem hiding this comment.
5. Double-counts aof layouts 🐞 Bug ≡ Correctness
measure_total_size() sums both appendonlydir/ contents and the legacy appendonly.aof file whenever they exist, instead of measuring only the active persistence layout. In the supported tokio --shards 1 path where a multi-part manifest may exist but legacy recovery is used, this inflates aof_base_size/aof_current_size and can delay or prevent auto-rewrite for the active legacy AOF.
Agent Prompt
### Issue description
The auto-rewrite monitor’s size sampler counts both:
1) the manifest directory (`<dir>/appendonlydir/**`), and
2) the legacy single file (`<dir>/<appendfilename>`),
whenever they exist.
But the codebase explicitly supports scenarios where a multi-part manifest exists on disk while the active runtime/layout uses legacy `appendonly.aof` (tokio + `--shards 1`). In that case, the sampler’s total size (and thus `aof_base_size` and trigger inputs) includes stale/unreferenced multi-part files, making the trigger math wrong for the actively-written file.
### Issue Context
- `main.rs` warns that tokio `--shards 1` will not replay multi-part even if the manifest exists.
- `auto_rewrite` currently assumes the two formats “never coexist” and adds both sizes.
### Fix Focus Areas
- src/persistence/aof/auto_rewrite.rs[58-101]
- src/main.rs[1745-1767]
### Suggested fix
Implement layout-aware measurement:
- Decide *once at init* which storage is active (e.g., `ActiveAofLayout::{LegacyFile, ManifestDir}`), and store it in a static.
- For tokio `--shards 1`, choose `LegacyFile`.
- For PerShard pools (and monoio TopLevel multi-part), choose `ManifestDir`.
- Update `measure_total_size()` to measure **only** the chosen layout.
Optional hardening (if you want exactness):
- When measuring `ManifestDir`, load the manifest and sum only files referenced by the current committed seq/layout (exclude old generations / temp files), so base/current align with what rewrite/recovery actually uses.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
1 participant
Fixes #433. Fixes #432.
Problem
The AOF was append-only with no compaction path: it grew with write volume, not dataset size (observed live: 4.8 GB
appendonlydirover a 2.43 GB dataset, ~1 GB/day) until the diskfull guard paused writes. On the default multi-shard config even the manual escape hatch was gated off (BGREWRITEAOFrefused unless--experimental-per-shard-rewrite). AndINFO persistencereportedaof_enabled:0unconditionally, so operators couldn't even see it happening.Changes
1. Redis-parity automatic rewrite. New
--auto-aof-rewrite-percentage(default 100,0disables) and--auto-aof-rewrite-min-size(default64mb; size strings). A monitor thread (src/persistence/aof/auto_rewrite.rs) samples the on-disk AOF size once a second and dispatchesbgrewriteaof_start_sharded— the exact entry the command uses — whencurrent >= min_sizeand(current−base)·100/max(base,1) >= percentage. Base re-records after boot recovery and each completed rewrite. Design-for-failure: failed dispatch arms a 60 s cooldown (no hot-retry livelock); skipped during BGSAVE/rewrite; nothing on the hot path. Both knobs inCONFIG GET; conf-file keys work.2. Multi-shard
BGREWRITEAOFun-gated. The per-shard fan-out (C4 cooperative snapshot + synchronized manifest commit) is the default. The gate dated from the pre-C4 ~38%-key-loss era; the current path holds exact INCR recovery across a rewrite straddling a live write stream + SIGKILL.--experimental-per-shard-rewritedeprecated (warn, no-op).3.
INFO persistencereal AOF state (#432).aof_enabled/aof_rewrite_in_progressreal; newaof_base_size/aof_current_sizeexpose the trigger's inputs.4. Crash-matrix harness fix.
crash_matrix_per_shard_bgrewriteaoflacked--disk-free-min-pct 0and parsedMOONERR diskfullINCR rejections as-1— on this host (root volume ~4% free) the straddle test "lost" 261/500 INCRs that were never acked; a reply-capture hunt proved the rewrite itself exact. Harness now disables the guard and panics on non-numeric INCR replies; green 5/5 consecutive runs.Layout note (found by the dual-runtime leg): tokio's TopLevel (shards=1) writer appends to a legacy flat
appendonly.aof, not theappendonlydirmanifest monoio uses. The sampler measures both; its in-place rewrite is detected in tests by shrink-below-high-water sampled during the write stream.Tests (red/green TDD)
tests/aof_auto_rewrite.rs(all#[ignore], crash-suite convention, run explicitly against both runtime binaries — monoio and tokio 5/5 each):percentage 0disablescurrent > baseafter writesUnit tests pin the trigger predicate (boundaries, zero-base→1 Redis rule, min-size floor, overflow).
Gates
shardslice_livefold suite greenclient_tracking_invalidationpush-delivery flakes, A/B-verified same failure band on pristine main under identical host conditions-D warningsboth feature sets; fmtSummary by CodeRabbit
New Features
BGREWRITEAOFby default.Documentation
Bug Fixes