fix(persistence): multi-shard AOF gate + per-shard AOF foundation (Option B step 1)

[pilotspacex-byte]

Summary

Three commits on this branch:

1. e0bb658 — P0 gate (P0-FIX-01a/b). BGREWRITEAOF refusal + startup refusal for --shards >= 2 + --appendonly yes, with --unsafe-multishard-aof escape hatch. Empirical loss matrix below.
2. 7b61898 — docs/changelog. v0.1.12 launch posture, 3-way Moon/Redis/Valkey comparison, alpha-leak qualifiers, missing v0.1.9/v0.1.10/v0.1.12 CHANGELOG entries.
3. 3bb4790 — per-shard AOF manifest format (Option B step 1). First step of the 9-step RFC at tmp/rfc-per-shard-aof-v02.md that eventually lifts the gate in step 9.

Why the gate (commit 1)

Empirical re-verification on HEAD 6e49050 (2026-05-26) found the durability bug is in the multi-shard AOF path itself, not the rewrite path the older bug memory blamed:

Configuration	Recovered
--shards 1 --appendonly yes --appendfsync always (control)	5000 / 5000
--shards 1 --disk-offload enable --appendonly yes (control)	12714 / 12714
--shards 2 --disk-offload enable --appendonly yes (BGREWRITEAOF + SIGKILL)	7892 / 12662 (-38%)
--shards 2 --disk-offload enable --appendonly yes (plain SIGKILL)	7888 / 12655 (-38%)
--shards 2 --disk-offload enable --appendonly yes --appendfsync always	2474 / 5000 (-50%)
--shards 2 --disk-offload disable --appendonly yes --appendfsync always	2453 / 5000 (-50%)

Root cause investigation in tmp/P0-INVEST-01-multishard-aof-rootcause.md identified two complementary bugs:

• H2 (structural): src/main.rs:562-563 literally skips multi-part AOF replay for num_shards >= 2. Closed by step 4.
• H1 (in-flight): try_send(AofMessage::Append) is fire-and-forget; +OK returns before the writer thread fsyncs; channel buffer is lost on SIGKILL. Closed by step 7.

Why step 1 lands here (commit 3)

The decision tree:

1. Build Option B (per-shard AOF, 16d + 1wk soak) vs make the gate permanent → user chose Option B.
2. First obstacle is that the current AofManifest has no place to describe per-shard segments. Step 1 introduces that structure additively.

Strictly additive at the file-system level:

• v1 manifests continue to load as AofLayout::TopLevel single-shard (shard_id=0).
• No in-place migration triggered. No behavior change for any existing deployment.
• The --unsafe-multishard-aof gate from commit 1 remains the load-bearing safety net until step 9.

What step 1 adds

• AofLayout { TopLevel, PerShard } discriminator.
• ShardManifest { shard_id: u16, max_lsn: u64 } — max_lsn semantics deferred to step 3.
• AofManifest.layout + AofManifest.shards: Vec<ShardManifest>.
• initialize_multi(dir, num_shards) — v2 PerShard constructor.
• shard_dir, shard_base_path[_seq], shard_incr_path[_seq] — per-shard path helpers.
• verify_shard_count(expected) — returns the verbatim RFC § 3 error.
• is_legacy_top_level_layout(dir) — pure detection (no side effects).
• migrate_top_level_to_per_shard() — explicit in-place rename for RFC § 5 case 1; idempotent.
• global_max_lsn() — computed accessor, not stored (avoids drift with the per-shard records).

Manifest text format

v1 (unchanged, full backcompat):

seq <N>
base moon.aof.<N>.base.rdb
incr moon.aof.<N>.incr.aof

v2 (new):

version 2
seq <N>
shards <K>
shard 0 max_lsn <lsn0>
shard 1 max_lsn <lsn1>
...

Paths are derived from shard_id + seq rather than stored explicitly — the layout is canonical, so a stored path could drift from the computed location.

Test plan

• Unit test test_bgrewriteaof_sharded_refuses_under_unsafe_config covers gate-on + gate-off paths.
• Live OrbStack boundary tests for the P0 gate:
  • PASS --shards 1 + AOF starts cleanly
  • PASS --shards 2 + AOF + --unsafe-multishard-aof starts
  • PASS --shards 2 + --appendonly no starts
  • REFUSED --shards 2 + AOF without escape hatch (exit code 2)
• 9 new step-1 unit tests (in aof_manifest.rs tests_v2 module):
  • v1_manifest_loads_as_top_level_single_shard
  • v2_manifest_round_trips
  • verify_shard_count_emits_rfc_error_verbatim
  • migrate_top_level_to_per_shard_moves_files_and_rewrites_manifest
  • global_max_lsn_returns_max_across_shards
  • is_legacy_top_level_layout_detects_v1_files
  • is_legacy_top_level_layout_returns_false_for_v2
  • parse_v2_rejects_shard_count_mismatch_in_file
  • parse_v2_rejects_non_contiguous_shard_ids
• 21 existing persistence::aof tests still green; library cargo check --no-default-features --features runtime-tokio,jemalloc clean.
• CI green (Rust 1.94, both feature sets, clippy, audit-unsafe, audit-unwrap).
• CRASH-01-LITE 7-config matrix as #[ignore] tests (step 8 of the RFC).

Operator impact

• Existing --shards >= 2 + --appendonly yes deployments fail to start after upgrade. Error message is actionable: pick --shards 1, --appendonly no, or --unsafe-multishard-aof. Runbook walks each option.
• Single-shard deployments unaffected.
• --appendonly no (any shard count) unaffected.

Next steps on this branch

Per the RFC implementation table (16 days + 1 wk soak):

Step	Effort	Blocked by
2 — Per-shard AofWriter task; aof_tx: Vec<Sender>	3d	1
3 — LSN tagging in AofMessage::Append	1d	2 + v0.2 S1.3
4 — Replace Multi-part AOF skipped skip branch (closes H2)	2d	1, 2, 3
5 — Cross-shard ordering merge (TXN + SCRIPT)	2d	4
6 — moon migrate-aof subcommand (RFC § 5 case 2)	2d	1
7 — AppendSync { bytes, ack } rendezvous (closes H1)	3d	2 (parallel with 3-6)
8 — CRASH-01-LITE matrix in tests/crash_matrix.rs	1d	4
9 — Lift --unsafe-multishard-aof gate	1h	8 green

Summary by CodeRabbit

• New Features

  • Server now refuses unsafe multi-shard + appendonly configs unless explicitly acknowledged via a new CLI flag
  • Added multi-shard AOF manifest/layout support and per-shard AOF writer behavior

• Documentation

  • Updated README (status badges, production-readiness matrix, benchmarks, quick-start TTL examples)
  • Added runbook describing multi-shard AOF safety and recovery procedures

• Tests

  • Test harnesses updated to opt out of unsafe multi-shard AOF by default

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[coderabbitai]

Warning

Review limit reached

@TinDang97, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 3 minutes and 1 second. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 84a55a2f-5d0b-4f98-a45f-88fda6865899

📥 Commits

Reviewing files that changed from the base of the PR and between 5004f4e and e46dc4e.

📒 Files selected for processing (7)

• src/persistence/aof.rs
• src/replication/state.rs
• src/server/conn/blocking.rs
• src/server/conn/handler_monoio/mod.rs
• src/server/conn/handler_sharded/mod.rs
• src/server/conn/handler_single.rs
• src/server/conn/tests.rs

📝 Walkthrough

Walkthrough

This PR enforces a startup/runtime safety gate for a known multi-shard AOF rewrite durability bug (CLI override added), adds v1/v2 AOF manifest layouts with migration, implements an AOF writer-pool and per-shard writer task, migrates connection/handler wiring to the pool, and updates tests, runbook, README, and CHANGELOG.

Changes

Multi-Shard AOF Safety Gates

Layer / File(s)	Summary
Release notes and operational runbook CHANGELOG.md, docs/runbooks/multi-shard-aof-rewrite.md	Reorganizes CHANGELOG into dated releases, adds v0.2.0-alpha/v0.1.12 sections, and adds a runbook documenting startup and BGREWRITEAOF refusal gates, measured data-loss characteristics, recovery steps, telemetry signals, and planned removal.
Configuration flag and startup enforcement src/config.rs, src/main.rs	Adds --unsafe-multishard-aof to ServerConfig (default false) and implements an early startup refusal when --shards >= 2 + appendonly yes without the override (exits code 2 and prints a data-loss warning).
Runtime BGREWRITEAOF command gating src/command/persistence.rs, src/main.rs	Introduces MULTI_SHARD_AOF_REWRITE_UNSAFE AtomicBool; maps AOF pool send errors to RESP frames; bgrewriteaof_start_sharded refuses when the gate is set; main sets the gate at runtime for the unsafe config and logs warnings.
Gate validation tests src/command/persistence.rs	Adds a mutex-protected unit test asserting the unsafe gate returns the documented refusal and keeps AOF_REWRITE_IN_PROGRESS unset, then restores prior global state.
AOF manifest multi-layout and migration src/persistence/aof_manifest.rs	Adds AofLayout (TopLevel/PerShard) and ShardManifest, extends AofManifest with layout/shards, implements layout-aware path helpers, v1/v2 parse & write, orphan cleanup, in-place TopLevel→PerShard migration with rollback, initialize_multi, verification helpers, and extensive v2 tests.
AOF writer pool & per-shard writer task src/persistence/aof.rs	Introduces AofWriterPool (top-level/per-shard constructors, routing, try_send_append/try_send_rewrite, broadcast_shutdown) and per_shard_aof_writer_task (PerShard incremental AOF processing with fsync policies and cancellation); includes unit tests for routing, rewrite acceptance/rejection, and shutdown.
Connection context and handler migration src/server/conn/core.rs, src/server/conn_state.rs, src/server/conn/handler_monoio/mod.rs, src/shard/conn_accept.rs, src/shard/event_loop.rs, src/server/listener.rs, src/server/embedded.rs	Adds optional aof_pool: Option<Arc<AofWriterPool>> to connection context/state, threads a top-level AofWriterPool into shard and connection startup, and migrates handlers/inline-dispatch/write paths to call aof_pool.try_send_append(shard_id, bytes) including cross-shard batching bookkeeping to route remote-write appends to the owning shard.
Handlers, inline dispatch, and tests src/server/conn/handler_single.rs, src/server/conn/handler_sharded/mod.rs, src/server/conn/blocking.rs, src/server/conn/tests.rs	Updates BGREWRITEAOF/SWAPDB/SUBSCRIBE/graph WAL and inline dispatch to use the pool API and updates tests to construct AofWriterPool::top_level(...) where needed; many integration tests now explicitly set unsafe_multishard_aof: false.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• pilotspace/moon#71: Overlaps inline-dispatch AOF append logic and may conflict with handler append changes.
• pilotspace/moon#100: Touches BGREWRITEAOF / AOF rewrite concurrency and SWAPDB interactions with rewrite flags.
• pilotspace/moon#63: Prior work on sharded AOF rewrite paths that this PR extends and guards.

Suggested labels: enhancement

"🐰 I hop where shards and AOF collide,
A careful flag keeps unsafe rewrites aside.
Manifests split and writers find their lane,
Docs, tests, and pools to guide the terrain.
Hooray — safer starts, one careful stride."

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adding a multi-shard AOF gate and laying the foundation for per-shard AOF architecture (Option B step 1).
Description check	✅ Passed	The PR description is comprehensive, covering three commits with detailed explanations of the gate, manifest format changes, test plan, operator impact, and next steps. All sections are well-documented.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/p0-multishard-aof-gate

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

README.md (1)

229-233: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Quick-start production flags now conflict with startup safety gate.

This command should fail under the new startup refusal (--shards >= 2 + --appendonly yes without override), so the README is currently instructing an invalid config.

Suggested README correction

 # Or with production flags
 ./target/release/moon \
   --port 6379 \
-  --shards 8 \
-  --appendonly yes --appendfsync everysec \
+  --shards 1 \
+  --appendonly yes --appendfsync everysec \
   --maxmemory 8g --maxmemory-policy allkeys-lfu
+
+# Multi-shard cache-only alternative
+# ./target/release/moon --shards 8 --appendonly no ...
+
+# Unsafe override (not recommended; known durability risk)
+# ./target/release/moon --shards 8 --appendonly yes --unsafe-multishard-aof ...

🤖 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 `@README.md` around lines 229 - 233, The README's quick-start example uses
conflicting flags (--shards 8 together with --appendonly yes) which will trigger
the new startup safety gate and refuse to start; update the example command
under the block that contains the flags (--port, --shards, --appendonly,
--appendfsync, --maxmemory, --maxmemory-policy) to a valid configuration (e.g.,
set --shards 1 or remove/disable --appendonly) or explicitly show the required
override flag and text that allows bypassing the safety gate (add a clear
placeholder like --<startup-override> if an override exists) so the documented
command actually starts successfully.

🤖 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 `@docs/runbooks/multi-shard-aof-rewrite.md`:
- Around line 10-16: Three fenced code blocks in
docs/runbooks/multi-shard-aof-rewrite.md are missing language identifiers
(markdownlint MD040). Edit the three blocks shown (the startup refusal block
starting "REFUSING TO START: --shards 2 + --appendonly yes...", the BGREWRITEAOF
interaction block containing "BGREWRITEAOF" and "(error) ERR BGREWRITEAOF...",
and the final explanatory block starting "BGREWRITEAOF gated for this
config...") and add the language tags: use ```text for the two plain-text blocks
and ```redis for the BGREWRITEAOF example so markdownlint MD040 is satisfied.

In `@src/main.rs`:
- Around line 273-289: The --check-config path currently returns before the
multishard-AOF safety gate runs, so add the same refusal logic used at startup
into the check_config branch: detect the condition (num_shards >= 2 &&
config.appendonly == "yes" && !config.unsafe_multishard_aof) inside the
check_config handling and print the identical error message and exit non‑zero
(or return an error) so preflight fails the same way real startup would; use the
same symbols/strings (num_shards, config.appendonly,
config.unsafe_multishard_aof) and the same message text used near the startup
gate to keep behavior consistent.

---

Outside diff comments:
In `@README.md`:
- Around line 229-233: The README's quick-start example uses conflicting flags
(--shards 8 together with --appendonly yes) which will trigger the new startup
safety gate and refuse to start; update the example command under the block that
contains the flags (--port, --shards, --appendonly, --appendfsync, --maxmemory,
--maxmemory-policy) to a valid configuration (e.g., set --shards 1 or
remove/disable --appendonly) or explicitly show the required override flag and
text that allows bypassing the safety gate (add a clear placeholder like
--<startup-override> if an override exists) so the documented command actually
starts successfully.

🪄 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: c11a2da9-b702-43f0-91ac-59786ae9a841

📥 Commits

Reviewing files that changed from the base of the PR and between 6e49050 and 7b61898.

📒 Files selected for processing (6)

• CHANGELOG.md
• README.md
• docs/runbooks/multi-shard-aof-rewrite.md
• src/command/persistence.rs
• src/config.rs
• src/main.rs

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add fenced code languages to satisfy markdownlint MD040.

These three fenced blocks are missing language identifiers and will keep markdownlint warnings active.

Suggested doc-only fix

-```
+```text
 REFUSING TO START: --shards 2 + --appendonly yes has a known data-loss
 ...
-```
+```

-```
+```redis
 > BGREWRITEAOF
 (error) ERR BGREWRITEAOF is unsafe with --shards >= 2 + --disk-offload enable
 ...
-```
+```

-```
+```text
 BGREWRITEAOF gated for this config (known data-loss path; see
 docs/runbooks/multi-shard-aof-rewrite.md). Use --shards 1 or
 --disk-offload disable to re-enable rewrite.
-```
+```

Also applies to: 20-26, 88-92

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 10-10: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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 `@docs/runbooks/multi-shard-aof-rewrite.md` around lines 10 - 16, Three fenced
code blocks in docs/runbooks/multi-shard-aof-rewrite.md are missing language
identifiers (markdownlint MD040). Edit the three blocks shown (the startup
refusal block starting "REFUSING TO START: --shards 2 + --appendonly yes...",
the BGREWRITEAOF interaction block containing "BGREWRITEAOF" and "(error) ERR
BGREWRITEAOF...", and the final explanatory block starting "BGREWRITEAOF gated
for this config...") and add the language tags: use ```text for the two
plain-text blocks and ```redis for the BGREWRITEAOF example so markdownlint
MD040 is satisfied.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Mirror this refusal in --check-config validation.

Line 143 returns from --check-config before Line 280 runs, so preflight can pass a config that real startup immediately refuses. Please enforce the same gate in the check_config branch.

Suggested patch

@@
     if config.check_config {
+        if config.shards >= 2 && config.appendonly == "yes" && !config.unsafe_multishard_aof {
+            return Err(anyhow::anyhow!(
+                "--shards {} + --appendonly yes is refused unless --unsafe-multishard-aof is set (or use --shards 1 / --appendonly no)",
+                config.shards
+            ));
+        }
         // Validate shard count is reasonable
         if config.shards == 0 {
             return Err(anyhow::anyhow!("--shards must be >= 1"));
         }

🤖 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 273 - 289, The --check-config path currently
returns before the multishard-AOF safety gate runs, so add the same refusal
logic used at startup into the check_config branch: detect the condition
(num_shards >= 2 && config.appendonly == "yes" && !config.unsafe_multishard_aof)
inside the check_config handling and print the identical error message and exit
non‑zero (or return an error) so preflight fails the same way real startup
would; use the same symbols/strings (num_shards, config.appendonly,
config.unsafe_multishard_aof) and the same message text used near the startup
gate to keep behavior consistent.

[coderabbitai]

Actionable comments posted: 2

🤖 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/aof_manifest.rs`:
- Around line 669-775: migrate_top_level_to_per_shard and initialize_multi are
making AofLayout::PerShard visible before the rest of the I/O path
(replay_multi_part, manifest.base_path/incr_path, manifest.incr_path(),
manifest.advance()) understands per-shard locations; this causes subsequent
boots/writes to look in the wrong place. Fix by deferring setting self.layout =
AofLayout::PerShard (and any manifest persisted as PerShard via
write_manifest()) until after you have created/moved the per-shard files and
ensured callers will open the new paths: in migrate_top_level_to_per_shard move
the layout assignment to after the rename/create operations and only call
write_manifest() once layout is set; in initialize_multi avoid persisting a
PerShard manifest or exposing PerShard paths until all shard dirs/files are
created (set layout and call write_manifest() last). Alternatively, make
replay_multi_part, base_path(), incr_path(), manifest.incr_path(), and
manifest.advance() layout-aware so they resolve per-shard paths immediately;
pick one approach and apply it consistently.
- Around line 688-717: After renaming old_base→new_base and optionally
old_incr→new_incr, add a rollback guard so any subsequent error (including
write_manifest() failing) moves the files back and restores self.layout to
AofLayout::TopLevel; implement this by tracking that the base (and possibly
incr) have been moved and on any error attempt std::fs::rename(new_base,
old_base) and, if incr was moved, std::fs::rename(new_incr, old_incr) (or remove
created new_incr if it was created), then set self.layout = AofLayout::TopLevel
before returning the error. Ensure the guard runs for failures after the first
rename but not if everything succeeds (write_manifest() completes), and
reference the existing symbols new_base, new_incr, old_base, old_incr,
self.layout, and write_manifest() to locate where to add the rollback.

🪄 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: 5a131377-fab2-4cd3-8f99-12ed9fc7f9ff

📥 Commits

Reviewing files that changed from the base of the PR and between 7b61898 and 3bb4790.

📒 Files selected for processing (1)

• src/persistence/aof_manifest.rs

[coderabbitai]

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 `@src/persistence/aof.rs`:
- Around line 858-930: The per-shard writer loop currently blocks on rx.recv()
and never checks cancel, so a cancellation-only shutdown can hang; modify the
loop around rx.recv() (the match handling AofMessage::{Append, Rewrite,
RewriteSharded, Shutdown}) to make cancellation reachable by either using a
non-blocking/timeout receive (e.g., try_recv/recv_timeout) or by selecting
between rx.recv() and checking the cancel flag (atomic or channel) before/after
the recv, breaking out when cancel is set; ensure you still perform the same
final flush/sync logic (the file.flush().and_then(|_| file.sync_data()) block
guarded by write_error) and preserve the metrics/fsync handling and warnings for
Rewrite/RewriteSharded.
- Around line 720-756: The per-shard AOF writer currently ignores errors from
writer.flush().await and writer.get_ref().sync_data().await in the EverySec,
Shutdown, and cancel branches (as well as the Always path already checked),
which can silently drop durability; update the branches handling interval.tick()
when fsync == FsyncPolicy::EverySec, the Ok(AofMessage::Shutdown) / Err(_)
shutdown branch, and the cancel.cancelled() branch to check the Result values
from flush() and sync_data(), log failures including shard_id and the error, and
surface a degraded state (e.g., return or set a tracer/metric) instead of
discarding errors—modify the calls around writer.flush().await and
writer.get_ref().sync_data().await and add error handling/logging similar to the
existing Append path handling.

In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 1536-1541: The AOF is serializing the original client Frame
(`frame`) instead of the possibly workspace-rewritten command arguments
(`cmd_args`), causing persisted writes to use unprefixed keys; change the AOF
serialization for local writes to serialize the dispatched command (the same
representation used by `dispatch_frame`) by passing the rewritten command args
(or the dispatched command object) into `aof::serialize_command` before calling
`pool.try_send_append` so local writes persist the post-rewrite command; update
the block that checks `is_write`/`ctx.aof_pool` to use `cmd_args` (or the
dispatched command) rather than `frame`, keeping the
`pool.try_send_append(ctx.shard_id, ...)` call.

🪄 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: b9290671-bc3d-4bf2-9a53-f512eecb98c0

📥 Commits

Reviewing files that changed from the base of the PR and between 3bb4790 and a05f3d8.

📒 Files selected for processing (6)

• src/persistence/aof.rs
• src/persistence/aof_manifest.rs
• src/server/conn/core.rs
• src/server/conn/handler_monoio/mod.rs
• src/server/conn_state.rs
• src/shard/conn_accept.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/persistence/aof_manifest.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Surface fsync/flush failures in the Tokio per-shard writer.

Lines 726-727, 748-755 ignore flush() / sync_data() errors. That makes appendfsync always and shutdown/cancel paths silently lose durability without logging or tripping degraded state.

Suggested direction

-                            if matches!(fsync, FsyncPolicy::Always) {
-                                let _ = writer.flush().await;
-                                let _ = writer.get_ref().sync_data().await;
+                            if matches!(fsync, FsyncPolicy::Always) {
+                                if let Err(e) = writer.flush().await {
+                                    error!("AOF flush failed shard {}: {}", shard_id, e);
+                                    break;
+                                }
+                                if let Err(e) = writer.get_ref().sync_data().await {
+                                    error!("AOF sync failed shard {}: {}", shard_id, e);
+                                    break;
+                                }
                             }

Apply the same check to the EverySec, Shutdown, and cancel branches too.

🤖 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.rs` around lines 720 - 756, The per-shard AOF writer
currently ignores errors from writer.flush().await and
writer.get_ref().sync_data().await in the EverySec, Shutdown, and cancel
branches (as well as the Always path already checked), which can silently drop
durability; update the branches handling interval.tick() when fsync ==
FsyncPolicy::EverySec, the Ok(AofMessage::Shutdown) / Err(_) shutdown branch,
and the cancel.cancelled() branch to check the Result values from flush() and
sync_data(), log failures including shard_id and the error, and surface a
degraded state (e.g., return or set a tracer/metric) instead of discarding
errors—modify the calls around writer.flush().await and
writer.get_ref().sync_data().await and add error handling/logging similar to the
existing Append path handling.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make cancellation reachable in the Monoio per-shard writer loop.

After manifest load, this branch blocks on rx.recv() and never checks cancel again. A cancellation-only shutdown can therefore hang until another channel message arrives.

🤖 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.rs` around lines 858 - 930, The per-shard writer loop
currently blocks on rx.recv() and never checks cancel, so a cancellation-only
shutdown can hang; modify the loop around rx.recv() (the match handling
AofMessage::{Append, Rewrite, RewriteSharded, Shutdown}) to make cancellation
reachable by either using a non-blocking/timeout receive (e.g.,
try_recv/recv_timeout) or by selecting between rx.recv() and checking the cancel
flag (atomic or channel) before/after the recv, breaking out when cancel is set;
ensure you still perform the same final flush/sync logic (the
file.flush().and_then(|_| file.sync_data()) block guarded by write_error) and
preserve the metrics/fsync handling and warnings for Rewrite/RewriteSharded.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Serialize the dispatched command, not the client frame, for local AOF writes.

Line 1539 still uses frame, but by this point cmd_args may already be workspace-rewritten. Remote writes handle this correctly with dispatch_frame; local writes will persist the unprefixed command and replay to the wrong key after restart.

Suggested fix

-                        if let Some(ref pool) = ctx.aof_pool {
-                            let serialized = aof::serialize_command(&frame);
+                        if let Some(ref pool) = ctx.aof_pool {
+                            let aof_frame = if rewritten.is_some() {
+                                let mut parts = Vec::with_capacity(1 + cmd_args.len());
+                                parts.push(Frame::BulkString(Bytes::copy_from_slice(cmd)));
+                                parts.extend_from_slice(cmd_args);
+                                Frame::Array(parts.into())
+                            } else {
+                                frame.clone()
+                            };
+                            let serialized = aof::serialize_command(&aof_frame);
                             pool.try_send_append(ctx.shard_id, serialized);
                         }

📝 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

	// AOF logging for successful local writes
	if !matches!(response, Frame::Error(_)) && is_write {
	if let Some(ref tx) = ctx.aof_tx {
	if let Some(ref pool) = ctx.aof_pool {
	let serialized = aof::serialize_command(&frame);
	let _ = tx.try_send(AofMessage::Append(serialized));
	pool.try_send_append(ctx.shard_id, serialized);
	}
	// AOF logging for successful local writes
	if !matches!(response, Frame::Error(_)) && is_write {
	if let Some(ref pool) = ctx.aof_pool {
	let aof_frame = if rewritten.is_some() {
	let mut parts = Vec::with_capacity(1 + cmd_args.len());
	parts.push(Frame::BulkString(Bytes::copy_from_slice(cmd)));
	parts.extend_from_slice(cmd_args);
	Frame::Array(parts.into())
	} else {
	frame.clone()
	};
	let serialized = aof::serialize_command(&aof_frame);
	pool.try_send_append(ctx.shard_id, serialized);
	}

🤖 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/server/conn/handler_monoio/mod.rs` around lines 1536 - 1541, The AOF is
serializing the original client Frame (`frame`) instead of the possibly
workspace-rewritten command arguments (`cmd_args`), causing persisted writes to
use unprefixed keys; change the AOF serialization for local writes to serialize
the dispatched command (the same representation used by `dispatch_frame`) by
passing the rewritten command args (or the dispatched command object) into
`aof::serialize_command` before calling `pool.try_send_append` so local writes
persist the post-rewrite command; update the block that checks
`is_write`/`ctx.aof_pool` to use `cmd_args` (or the dispatched command) rather
than `frame`, keeping the `pool.try_send_append(ctx.shard_id, ...)` call.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/command/persistence.rs (1)

34-47: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Keep the unsafe-gate message aligned with the actual gate.

The new docs/error text still says this refusal only applies when --disk-offload enable is set, but the PR objective describes the startup/runtime gate as --shards >= 2 plus --appendonly yes. If MULTI_SHARD_AOF_REWRITE_UNSAFE is set under that broader condition, this response tells operators to try the wrong workaround (--disk-offload disable).

Also applies to: 284-287

🤖 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/persistence.rs` around lines 34 - 47, The refusal
message/documentation around MULTI_SHARD_AOF_REWRITE_UNSAFE is misleading:
update the error/runbook text and any related doc comments (including the
instance at the later lines ~284-287) to reflect the actual gate condition
(shards >= 2 AND --appendonly yes) rather than suggesting it only applies when
--disk-offload enable; search for and edit the strings emitted by
bgrewriteaof_start_sharded and the public comment for
MULTI_SHARD_AOF_REWRITE_UNSAFE to mention the correct combination (shards >= 2 +
appendonly yes) and adjust suggested operator workarounds/runbook pointers
accordingly.

src/server/embedded.rs (1)

123-139: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Refuse multi-shard AOF in run_embedded too.

Lines 123-139 still start the legacy top-level AOF writer for any appendonly == "yes", but this path never enforces the new shards >= 2 && !unsafe_multishard_aof gate. That leaves embedded deployments on the known write-loss path this PR is supposed to block. Please reuse the same startup validation here before spawning the writer.

🤖 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/server/embedded.rs` around lines 123 - 139, The code unconditionally
starts the top-level AOF writer when config.appendonly == "yes" without checking
multi-shard safety; update the block that spawns the AOF writer in run_embedded
to reuse the same startup validation used elsewhere: check the shard count
(shards) and the unsafe_multishard_aof flag (or config.unsafe_multishard_aof)
and refuse/return an error if shards >= 2 and unsafe_multishard_aof is false,
before creating the channel, token, and calling AofWriterPool::top_level or
spawning aof::aof_writer_task; ensure you mirror the exact error message/flow
used in the other validation path so embedded deployments cannot start the
legacy top-level writer in unsafe multi-shard configurations.

🤖 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/server/conn/handler_sharded/mod.rs`:
- Around line 1122-1123: The current AOF serialization captures aof_bytes from
the original frame (variable frame) before workspace prefix injection, which
records client-visible keys and can replay to wrong shards; change the logic to
compute aof_bytes from the post-rewrite command used for execution—i.e.,
serialize the actual dispatched command (use dispatch_frame or the rewritten
cmd_args that include the {ws_id} injection) when is_write is true and
ctx.aof_pool.is_some(); keep the same is_write calculation
(metadata::is_write(cmd)) but ensure aof::serialize_command is called on the
final command used for dispatch/execution instead of the pre-injection frame so
AOF reflects the physical stored keys.

---

Outside diff comments:
In `@src/command/persistence.rs`:
- Around line 34-47: The refusal message/documentation around
MULTI_SHARD_AOF_REWRITE_UNSAFE is misleading: update the error/runbook text and
any related doc comments (including the instance at the later lines ~284-287) to
reflect the actual gate condition (shards >= 2 AND --appendonly yes) rather than
suggesting it only applies when --disk-offload enable; search for and edit the
strings emitted by bgrewriteaof_start_sharded and the public comment for
MULTI_SHARD_AOF_REWRITE_UNSAFE to mention the correct combination (shards >= 2 +
appendonly yes) and adjust suggested operator workarounds/runbook pointers
accordingly.

In `@src/server/embedded.rs`:
- Around line 123-139: The code unconditionally starts the top-level AOF writer
when config.appendonly == "yes" without checking multi-shard safety; update the
block that spawns the AOF writer in run_embedded to reuse the same startup
validation used elsewhere: check the shard count (shards) and the
unsafe_multishard_aof flag (or config.unsafe_multishard_aof) and refuse/return
an error if shards >= 2 and unsafe_multishard_aof is false, before creating the
channel, token, and calling AofWriterPool::top_level or spawning
aof::aof_writer_task; ensure you mirror the exact error message/flow used in the
other validation path so embedded deployments cannot start the legacy top-level
writer in unsafe multi-shard configurations.

🪄 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: 717300dd-c55e-4336-83a6-22aca7f3b2c6

📥 Commits

Reviewing files that changed from the base of the PR and between a05f3d8 and 5004f4e.

📒 Files selected for processing (25)

• src/command/persistence.rs
• src/main.rs
• src/server/conn/blocking.rs
• src/server/conn/core.rs
• src/server/conn/handler_monoio/dispatch.rs
• src/server/conn/handler_monoio/mod.rs
• src/server/conn/handler_sharded/dispatch.rs
• src/server/conn/handler_sharded/mod.rs
• src/server/conn/handler_single.rs
• src/server/conn/tests.rs
• src/server/conn_state.rs
• src/server/embedded.rs
• src/server/listener.rs
• src/shard/conn_accept.rs
• src/shard/event_loop.rs
• tests/ft_search_multi_shard_as_of.rs
• tests/ft_search_temporal_parity.rs
• tests/integration.rs
• tests/kill_snapshot.rs
• tests/mq_integration.rs
• tests/replication_test.rs
• tests/txn_ft_search_snapshot.rs
• tests/txn_kv_wiring.rs
• tests/vacuum_commands.rs
• tests/workspace_integration.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• src/server/conn_state.rs
• src/main.rs
• src/server/conn/handler_monoio/mod.rs
• src/shard/conn_accept.rs

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Serialize the rewritten command into AOF.

aof_bytes is captured from frame before workspace prefix injection, but both the local path (cmd_args) and the remote path (dispatch_frame) can execute the {ws_id}-prefixed form. In a workspace session this persists the client-visible key instead of the physical stored key, so AOF replay diverges from live state and can even route the write to the wrong shard.

Suggested fix

-                    let is_write = if ctx.aof_pool.is_some() || conn.tracking_state.enabled { metadata::is_write(cmd) } else { false };
-                    let aof_bytes = if is_write && ctx.aof_pool.is_some() { Some(aof::serialize_command(&frame)) } else { None };
+                    let is_write = if ctx.aof_pool.is_some() || conn.tracking_state.enabled {
+                        metadata::is_write(cmd)
+                    } else {
+                        false
+                    };
+                    let aof_bytes = if is_write && ctx.aof_pool.is_some() {
+                        Some(match rewritten.as_deref() {
+                            Some(rewritten_args) => {
+                                let mut parts = Vec::with_capacity(1 + rewritten_args.len());
+                                parts.push(Frame::BulkString(Bytes::copy_from_slice(cmd)));
+                                parts.extend_from_slice(rewritten_args);
+                                aof::serialize_command(&Frame::Array(parts.into()))
+                            }
+                            None => aof::serialize_command(&frame),
+                        })
+                    } else {
+                        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/server/conn/handler_sharded/mod.rs` around lines 1122 - 1123, The current
AOF serialization captures aof_bytes from the original frame (variable frame)
before workspace prefix injection, which records client-visible keys and can
replay to wrong shards; change the logic to compute aof_bytes from the
post-rewrite command used for execution—i.e., serialize the actual dispatched
command (use dispatch_frame or the rewritten cmd_args that include the {ws_id}
injection) when is_write is true and ctx.aof_pool.is_some(); keep the same
is_write calculation (metadata::is_write(cmd)) but ensure aof::serialize_command
is called on the final command used for dispatch/execution instead of the
pre-injection frame so AOF reflects the physical stored keys.