perf(persistence): eliminate AOF+WAL KV double-write + fail-loud everysec backpressure

[pilotspacex-byte]

Summary

Two write-path durability fixes from the 2026-07-04 WAL/AOF investigation (tmp/WAL-AOF-REVIEW.md, built on the byte-level attribution in tmp/WRITE-DIAG.md), plus the GCE A/B harness that proves them.

1. AOF+WAL double-write eliminated — --wal-kv-log auto|on|off (default auto)

At --appendonly yes, every SPSC-executed KV write was logged to both the per-shard AOF and the per-shard WAL, yet Phase-B recovery (main.rs) wipes WAL-replayed state and replays the AOF — the WAL copy was pure write amplification. The gate lives at the single KV→WAL choke point (wal_append_and_fanout); auto skips KV records while the AOF is the recovery authority and no CDC subscriber is attached (re-engages dynamically on attach), on restores the old behavior (PITR / full CDC history), off never logs. FPI/checkpoint/feature records are untouched.

2. everysec/no appends no longer silently dropped under backpressure

A full AOF writer channel used to warn! + drop the record while the client still received +OK — client-acked write loss, and AOF/memory divergence that corrupts replay of dependent commands (e.g. INCR replayed without its SET). Now:

• async handler paths (try_send_append_durable): await enqueue bounded by --aof-fsync-timeout-ms, surface ChannelFull/WriteFailed as an error frame;
• sync paths (SPSC drain, monoio inline SET): 5 ms bounded blocking send (AOF_SPSC_BACKPRESSURE_BOUND) before a counted, error!-logged last-resort drop. Fast path is the same try_send — zero cost until the channel is full. Watch aof_backpressure_dropped in INFO.

GCE proof (c3-standard-4, pd-ssd, steal=0, same-instance A/B, 3 fresh-server reps)

SET 500K × 64B × c50, --appendonly yes everysec. Raw: tmp/wal-ab-c3-standard-4.txt.

shards=4	on (old)	auto (fix)	delta
WAL KV bytes	47,588,553	256 (headers)	eliminated
device write bytes	107.3 MB	59.7 MB	−44.4%
SET P64 median	621,158	684,975	+10.3% (all reps ordered)
SET P1 median	89,461	90,041	parity
kill -9 recovery	—	393,163/393,163 keys + marker	lossless from AOF alone
backpressure drops	0	0	fail-loud path never engages on healthy disk

shards=1: byte-identical volumes and throughput parity in both modes (gate is structurally a no-op there).

Bonus measurement: WAL-only posture (--appendonly no, same instance type)

config	WAL bytes	P1 med	P64 med	kill-9 recovery
s1 walonly	64 (headers)	132,310	651,083	n/a — nothing logged at shards=1
s4 walonly	47.6 MB	83,195	1,152,148	79.2% (311,551/393,394)

WAL-only is fast (P64 +68% over AOF-only) but not durable: conn-local writes never reach the WAL, so recovery loses ~1/num_shards of writes at shards≥2 and everything at shards=1. Documented in docs/guides/persistence.md; confirms the AOF as the durability log and the WAL KV stream as CDC/PITR/offload infrastructure. (Cross-check: the s4 on run's WAL held 80% of AOF bytes — the same ~1/4 conn-local fraction.)

Tests

• everysec_full_channel_errs_instead_of_silent_drop — red-proven against the old fire-and-forget branch (reverted → fails, restored → passes) + drain-in-time success + sync-path bound/fast-path tests (pool.rs).
• test_kv_append_skipped_when_wal_kv_log_false — WAL segment stays header-only while the AOF pool still receives the entry; control asserts on logs (spsc_handler.rs).
• fmt + clippy clean (default & tokio feature sets); full lib suites green on both runtimes; OrbStack CI-parity matrix run before push.
• Pre-existing, unrelated: memory_doctor_returns_documented_schema fails identically on unmodified main on macOS (DashTable accounting = 0).

Docs

docs/guides/configuration.md (+ "Defaults are tuned for the single-shard case" section), docs/configuration.md, docs/guides/persistence.md ("One KV log at a time"), CHANGELOG [Unreleased].

Follow-ups (tracked in .add/milestones/v3-5-write-path-durability/, not this PR)

• Coordinator local-leg durable writes through group-commit/fsync_barrier (the 2000 ms always pipeline tail — [HIGH] carry-over from v3-4).
• Hot-path Bytes::copy_from_slice elimination + single-pass encode in wal_append_and_fanout.
• WAL-v3 always fsync off the shard event-loop thread.
• BITOP/COPY/DEL/UNLINK coordinator local-leg durability.

🤖 Generated with Claude Code — investigation, fix, red/green tests, and GCE validation in tmp/WAL-AOF-REVIEW.md.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 18 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: add920d7-1b24-4559-9cc3-089673efb9c8

📥 Commits

Reviewing files that changed from the base of the PR and between a2f5170 and 32167fd.

📒 Files selected for processing (7)

• CHANGELOG.md
• docs/guides/persistence.md
• scripts/gcloud-wal-aof-ab.sh
• src/config.rs
• src/persistence/aof/pool.rs
• src/server/conn/blocking.rs
• src/shard/spsc_handler.rs

📝 Walkthrough

Walkthrough

Adds a --wal-kv-log configuration mode and threads it through shard write paths to control when KV data is written to WAL versus AOF. Also changes AOF enqueue behavior under backpressure, updates docs and tests, and adds a GCE benchmark harness.

Changes

WAL-KV logging and AOF backpressure

Layer / File(s)	Summary
wal_kv_log config field and mode resolution src/config.rs	Adds wal_kv_log, WalKvLogMode, and mode resolution on ServerConfig.
AOF pool bounded backpressure and enqueue reporting src/persistence/aof/mod.rs, src/persistence/aof/pool.rs	Adds a backpressure bound, changes append enqueue behavior, and updates EverySec/No durability handling and tests.
Inline SET path uses bounded blocking append src/server/conn/blocking.rs	Switches the inline SET AOF append path to bounded blocking enqueue.
Shard event loop and spsc handler WAL-KV gating src/shard/event_loop.rs, src/shard/spsc_handler.rs, src/shard/mod.rs	Resolves WAL-KV mode, threads wal_kv_log through drain and fanout paths, updates WAL/AOF routing, and adjusts tests.
Documentation and changelog updates CHANGELOG.md, docs/configuration.md, docs/guides/configuration.md, docs/guides/persistence.md	Documents --wal-kv-log, --aof-fsync-timeout-ms, and the related persistence behavior.
gcloud WAL/AOF A/B benchmark script scripts/gcloud-wal-aof-ab.sh	Adds a VM-based benchmark harness for WAL/AOF write-path measurements.
Test server configs set wal_kv_log=auto tests/*.rs	Sets wal_kv_log: "auto" across integration and unit test server configurations.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ShardEventLoop
  participant SpscHandler
  participant WalWriter
  participant AofWriterPool

  Client->>ShardEventLoop: write command
  ShardEventLoop->>ShardEventLoop: resolve wal_kv_log_mode()
  ShardEventLoop->>SpscHandler: drain_spsc_shared(..., wal_kv_log)
  alt wal_kv_log enabled
    SpscHandler->>WalWriter: append KV record
  else wal_kv_log disabled
    SpscHandler->>SpscHandler: skip WAL KV append
  end
  SpscHandler->>AofWriterPool: send_append_bounded_blocking(bytes)
  alt enqueued within bound
    AofWriterPool-->>SpscHandler: ok
  else channel full past bound
    AofWriterPool-->>SpscHandler: dropped / timeout
  end
  SpscHandler-->>Client: +OK

Loading

Possibly related PRs

• pilotspace/moon#100: Both PRs modify WAL append wiring in src/shard/spsc_handler.rs, including SWAPDB/MOVE/COPY handling paths.
• pilotspace/moon#129: Both PRs touch the AofWriterPool append/enqueue plumbing in the AOF writer code.
• pilotspace/moon#168: Both PRs modify the shard SPSC drain/write hot path in src/shard/spsc_handler.rs.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Title check	✅ Passed	The title clearly captures the two main persistence fixes: removing AOF+WAL KV double-writes and making everysec backpressure fail loud.
Description check	✅ Passed	The description covers the summary, tests, docs, and performance evidence, but it does not explicitly follow the template's Checklist and Notes sections.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch perf/wal-aof-write-path

---

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.}

[qodo-code-review]

PR Summary by Qodo

perf(persistence): eliminate AOF+WAL KV double-write + fail-loud everysec backpressure

✨ Enhancement 🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Adds --wal-kv-log auto|on|off (default auto) to skip redundant WAL KV records when AOF is the
 crash-recovery authority, eliminating 2.7× file-byte / 4.1× device write amplification at `--shards
 >= 2`.
• Replaces silent fire-and-forget AOF drops under backpressure with bounded-wait paths: async
 handlers await enqueue under --aof-fsync-timeout-ms and return ChannelFull/WriteFailed errors;
 sync SPSC/monoio paths block up to 5 ms before a counted error!-logged last-resort drop.
• Adds WalKvLogMode enum and wal_kv_log_mode() resolver in config.rs; threads `wal_kv_log:
 bool through drain_spsc_shared, handle_shard_message_shared, wal_fanout_has_work`, and
 wal_append_and_fanout.
• Adds new AofWriterPool methods: send_append_bounded_blocking (sync) and
 send_append_backpressure (async); try_send_append now returns bool and counts drops in
 AOF_BACKPRESSURE_DROPPED.
• Adds GCE A/B benchmark harness (scripts/gcloud-wal-aof-ab.sh) proving −44.4% device writes and
 lossless AOF-only recovery at shards=4.
• Adds red/green tests: everysec_full_channel_errs_instead_of_silent_drop,
 everysec_backpressure_succeeds_when_writer_drains_in_time, spsc_bounded_blocking_*, and
 test_kv_append_skipped_when_wal_kv_log_false.
• Updates CHANGELOG.md, docs/configuration.md, docs/guides/configuration.md, and
 docs/guides/persistence.md with new flags and behavior.

Diagram

graph TD
    CLI["--wal-kv-log auto|on|off"] --> CFG["config.rs\nWalKvLogMode"]
    CFG --> EL["event_loop.rs\nper-drain resolution"]
    EL -->|wal_kv_log: bool| SPSC["spsc_handler.rs\ndrain_spsc_shared"]
    SPSC --> FAN["wal_append_and_fanout"]
    FAN -->|wal_kv_log=true| WAL[("WAL v2/v3")]
    FAN -->|always| AOF_POOL["AofWriterPool"]
    AOF_POOL --> BOUND["send_append_bounded_blocking\n5 ms sync bound"]
    AOF_POOL --> ASYNC_BP["send_append_backpressure\nasync, fsync_timeout bound"]
    BOUND --> AOF_FILE[("AOF file")]
    ASYNC_BP --> AOF_FILE
    BLOCK["blocking.rs\ntry_inline_dispatch"] --> BOUND
    ASYNC_BP -->|Err ChannelFull| CLIENT["Client error frame"]

    subgraph Legend
        direction LR
        _db[("Storage")] ~~~ _svc["Component"] ~~~ _ext["External / CLI"]
    end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. AtomicBool for wal_kv_log instead of per-cycle bool parameter

• ➕ Avoids threading the bool through every call site
• ➕ CDC attach/detach can flip the flag without waiting for the next drain cycle
• ➖ Adds a shared atomic read on every fanout call (minor)
• ➖ Harder to reason about — the current approach makes the decision explicit and testable at the call site

2. Dedicated overflow queue for AOF backpressure instead of bounded blocking

• ➕ Zero stall on the shard event loop even under disk pressure
• ➕ Decouples write acceptance from disk throughput
• ➖ Adds memory pressure and a second queue to drain/monitor
• ➖ Complicates ordering guarantees and the 'was this write durable?' answer
• ➖ More code surface for a path that should never be reached on a healthy disk

Recommendation: The PR's approach — a single bool gate threaded through the existing fanout call chain — is the right choice for minimal diff surface and zero hot-path overhead when the channel is healthy. The main alternative worth considering is a dynamic runtime toggle (e.g., an AtomicBool read at the fanout site) rather than re-evaluating the CDC registry on every drain cycle, but the current approach is correct and the per-cycle cost is negligible. The bounded-blocking approach for sync paths is a deliberate, well-justified trade-off over alternatives like a dedicated overflow queue or dropping to a separate thread.

Files changed (22) +814 / -37

Enhancement (3) +76 / -0

config.rsAdd '--wal-kv-log' flag and 'WalKvLogMode' enum +37/-0

Add '--wal-kv-log' flag and 'WalKvLogMode' enum

• Adds the 'wal_kv_log: String' CLI field to 'ServerConfig' with full doc-comment. Introduces the 'WalKvLogMode { Auto, On, Off }' enum and the 'wal_kv_log_mode()' resolver method that parses the string flag, defaulting unknown values to 'Auto'.

src/config.rs

mod.rsExport 'AOF_SPSC_BACKPRESSURE_BOUND' constant (5 ms) +8/-0

Export 'AOF_SPSC_BACKPRESSURE_BOUND' constant (5 ms)

• Adds the 'AOF_SPSC_BACKPRESSURE_BOUND' public constant (5 ms 'Duration') used by sync callers of 'send_append_bounded_blocking' to cap the worst-case shard event-loop stall.

src/persistence/aof/mod.rs

event_loop.rsResolve 'wal_kv_log_mode' per drain cycle and pass to SPSC drain +31/-0

Resolve 'wal_kv_log_mode' per drain cycle and pass to SPSC drain

• Reads 'wal_kv_log_mode' from 'server_config' at event-loop startup and emits a warning when '--wal-kv-log off' is combined with '--appendonly no'. Passes the resolved 'wal_kv_log: bool' (computed from mode + 'appendonly_enabled' + CDC registry emptiness) to all three 'drain_spsc_shared' call sites.

src/shard/event_loop.rs

Bug fix (3) +464 / -36

pool.rsAdd bounded-backpressure AOF send paths; fix silent drop under everysec +294/-14

Add bounded-backpressure AOF send paths; fix silent drop under everysec

• Refactors 'try_send_append' to return 'bool' and count drops in 'AOF_BACKPRESSURE_DROPPED'. Adds 'send_append_bounded_blocking' (sync, 'send_timeout'-based) for SPSC/monoio callers and 'send_append_backpressure' (async, dual-runtime 'select!'/'timeout') for durable handler paths. Updates 'try_send_append_durable' to await 'send_append_backpressure' under 'EverySec'/'No' instead of fire-and-forget. Adds four new unit tests covering the full-channel error, drain-in-time success, sync bound/fast-path cases.

src/persistence/aof/pool.rs

spsc_handler.rsThread 'wal_kv_log' through SPSC drain and fanout; gate WAL KV writes +159/-21

Thread 'wal_kv_log' through SPSC drain and fanout; gate WAL KV writes

• Adds 'wal_kv_log: bool' parameter to 'drain_spsc_shared', 'handle_shard_message_shared', 'wal_fanout_has_work', and 'wal_append_and_fanout'. Updates 'wal_fanout_has_work' to exclude WAL writers from the 'has work' predicate when 'wal_kv_log' is false. Guards the WAL v2/v3 append in 'wal_append_and_fanout' behind 'if wal_kv_log'. Replaces 'try_send_append' with 'send_append_bounded_blocking' in the AOF fanout step. Updates all existing tests to pass 'true' for legacy behavior; adds 'test_kv_append_skipped_when_wal_kv_log_false'.

src/shard/spsc_handler.rs

blocking.rsReplace fire-and-forget AOF send with bounded-blocking in inline SET path +11/-1

Replace fire-and-forget AOF send with bounded-blocking in inline SET path

• Replaces the 'try_send_append' call in 'try_inline_dispatch' with 'send_append_bounded_blocking' using 'AOF_SPSC_BACKPRESSURE_BOUND', preventing silent client-acked write loss on the monoio inline SET fast path.

src/server/conn/blocking.rs

Tests (11) +19 / -0

mod.rsPass 'wal_kv_log: true' to shard test helpers +2/-0

Pass 'wal_kv_log: true' to shard test helpers

• Adds the new 'wal_kv_log' boolean argument to two in-module test 'drain_spsc_shared' calls, preserving pre-existing legacy behavior in unit tests.

src/shard/mod.rs

integration.rsAdd 'wal_kv_log: "auto"' to all integration test server configs +7/-0

Add 'wal_kv_log: "auto"' to all integration test server configs

• Adds the new required 'wal_kv_log' field (set to '"auto"') to all 'ServerConfig' struct literals in the integration test helpers to keep them compiling after the config struct change.

tests/integration.rs

kill_snapshot.rsAdd 'wal_kv_log: "auto"' to kill-snapshot test config +1/-0

Add 'wal_kv_log: "auto"' to kill-snapshot test config

• Adds the new 'wal_kv_log' field to 'base_config' in the kill-snapshot test suite.

tests/kill_snapshot.rs

replication_test.rsAdd 'wal_kv_log: "auto"' to replication test server config +1/-0

Add 'wal_kv_log: "auto"' to replication test server config

• Adds the new 'wal_kv_log' field to the replication integration test server config.

tests/replication_test.rs

mq_integration.rsAdd 'wal_kv_log: "auto"' to MQ integration test config +1/-0

Add 'wal_kv_log: "auto"' to MQ integration test config

• Adds the new 'wal_kv_log' field to the MQ server config struct literal.

tests/mq_integration.rs

txn_kv_wiring.rsAdd 'wal_kv_log: "auto"' to transaction KV wiring test config +1/-0

Add 'wal_kv_log: "auto"' to transaction KV wiring test config

• Adds the new 'wal_kv_log' field to the transaction server config struct literal.

tests/txn_kv_wiring.rs

vacuum_commands.rsAdd 'wal_kv_log: "auto"' to vacuum commands test config +1/-0

Add 'wal_kv_log: "auto"' to vacuum commands test config

• Adds the new 'wal_kv_log' field to the vacuum test base config.

tests/vacuum_commands.rs

workspace_integration.rsAdd 'wal_kv_log: "auto"' to workspace integration test configs +2/-0

Add 'wal_kv_log: "auto"' to workspace integration test configs

• Adds the new 'wal_kv_log' field to both workspace server config struct literals.

tests/workspace_integration.rs

ft_search_multi_shard_as_of.rsAdd 'wal_kv_log: "auto"' to FT search multi-shard test config +1/-0

Add 'wal_kv_log: "auto"' to FT search multi-shard test config

• Adds the new 'wal_kv_log' field to the FT search multi-shard test config.

tests/ft_search_multi_shard_as_of.rs

ft_search_temporal_parity.rsAdd 'wal_kv_log: "auto"' to FT search temporal parity test config +1/-0

Add 'wal_kv_log: "auto"' to FT search temporal parity test config

• Adds the new 'wal_kv_log' field to the FT search temporal parity test config.

tests/ft_search_temporal_parity.rs

txn_ft_search_snapshot.rsAdd 'wal_kv_log: "auto"' to transaction FT search snapshot test config +1/-0

Add 'wal_kv_log: "auto"' to transaction FT search snapshot test config

• Adds the new 'wal_kv_log' field to the transaction FT search snapshot test config.

tests/txn_ft_search_snapshot.rs

Documentation (4) +45 / -1

CHANGELOG.mdDocument AOF+WAL double-write fix and everysec backpressure fix in [Unreleased] +19/-0

Document AOF+WAL double-write fix and everysec backpressure fix in [Unreleased]

• Adds two entries under '[Unreleased] Fixed': the '--wal-kv-log auto' double-write elimination and the bounded-backpressure everysec/no drop fix with 'aof_backpressure_dropped' counter reference.

CHANGELOG.md

configuration.mdAdd '--wal-kv-log' row and update '--aof-fsync-timeout-ms' description +2/-1

Add '--wal-kv-log' row and update '--aof-fsync-timeout-ms' description

• Adds the '--wal-kv-log auto' row to the persistence configuration table. Expands the '--aof-fsync-timeout-ms' description to cover its new role as a writer-queue backpressure bound under 'everysec'.

docs/configuration.md

configuration.mdAdd 'Defaults are tuned for the single-shard case' guidance section +13/-0

Add 'Defaults are tuned for the single-shard case' guidance section

• Inserts a new prose section explaining single-shard default rationale and the 'one KV record to disk exactly once' guarantee under '--wal-kv-log auto'. Adds '--aof-fsync-timeout-ms' and '--wal-kv-log' rows to the persistence table.

docs/guides/configuration.md

persistence.mdAdd 'One KV log at a time' section explaining '--wal-kv-log' +11/-0

Add 'One KV log at a time' section explaining '--wal-kv-log'

• Adds a new subsection under the WAL documentation explaining why '--wal-kv-log auto' skips the WAL KV copy when AOF is the recovery authority, and when to set '--wal-kv-log on' for PITR/CDC.

docs/guides/persistence.md

Other (1) +210 / -0

gcloud-wal-aof-ab.shAdd GCE A/B benchmark harness for WAL/AOF write-path fixes +210/-0

Add GCE A/B benchmark harness for WAL/AOF write-path fixes

• New 210-line bash script that provisions a GCE instance, builds the local source snapshot, and runs a same-instance A/B comparison across '--wal-kv-log on' vs 'auto' at 'shards={1,4}'. Measures on-disk byte volumes (AOF vs WAL), '/proc/<pid>/io' device writes, SET throughput (P1/P64, 3 reps), kill-9 recovery correctness, and 'aof_backpressure_dropped' counter.

scripts/gcloud-wal-aof-ab.sh

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (2) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 40 rules

1. SPSC path ignores AOF loss ✓ Resolved 🐞 Bug ≡ Correctness

Description

wal_append_and_fanout and the monoio inline write fast path (try_inline_dispatch) route write
commands to the per-shard AOF via send_append_bounded_blocking but discard its boolean result, so
a timed-out/disconnected enqueue can still return +OK/the original success frame to the caller.
This acknowledges in-memory mutations without a corresponding AOF entry, reintroducing acknowledged
write loss under backpressure and risking replay divergence.

Code

src/shard/spsc_handler.rs[R2544-2560]

+    // Bounded-blocking (`send_append_bounded_blocking`) because this function
+    // is sync and cannot await the fsync rendezvous: the fast path is the same
+    // try_send as before; only when the writer channel is FULL (writer >10k
+    // appends behind) does the shard thread block up to the bound instead of
+    // silently losing a record the client already got `+OK` for. The
+    // `appendfsync=always` ack is handled by the async connection handler
+    // (handler_sharded / handler_single). LSN=0 is safe here: per-shard order
+    // is preserved by write order; the LSN is only meaningful for cross-shard
+    // TXN merge (RFC step 5, not yet wired).
    if let Some(pool) = aof_pool {
-        pool.try_send_append(shard_id, 0, bytes::Bytes::copy_from_slice(data));
+        pool.send_append_bounded_blocking(
+            shard_id,
+            0,
+            bytes::Bytes::copy_from_slice(data),
+            crate::persistence::aof::AOF_SPSC_BACKPRESSURE_BOUND,
+        );
    }

Relevance

⭐⭐⭐ High

Team often fixes “silent success on failed send/recv” by propagating errors instead of defaults (PR
#17).

PR-#17

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In the synchronous SPSC write handling flow, wal_append_and_fanout is invoked before the response
is queued/constructed, yet it ignores the bool returned by
AofWriterPool::send_append_bounded_blocking, even though that API returns false precisely when
the append was not enqueued (timeout/disconnect). Similarly, try_inline_dispatch performs the same
bounded append for the inline SET AOF path but does not check the returned boolean and
unconditionally appends +OK, meaning both paths can acknowledge success despite the AOF writer
never receiving the record.

src/shard/spsc_handler.rs[723-747]
src/shard/spsc_handler.rs[2483-2561]
src/persistence/aof/pool.rs[458-497]
src/server/conn/blocking.rs[1419-1439]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Writes are acknowledged even when their AOF append fails: both the SPSC write path (`wal_append_and_fanout` as used by shard message handling) and the monoio inline SET fast path (`try_inline_dispatch`) call `AofWriterPool::send_append_bounded_blocking()` but ignore its `bool` return, so timeout/disconnect/backpressure can drop the AOF record while the client still receives a success frame (`+OK`/original success). We want to propagate bounded-enqueue failure back into the response so non-durable writes are not acknowledged.

## Issue Context
`send_append_bounded_blocking()` is explicitly designed to return `false` when the append was NOT enqueued (timeout/disconnect). In the SPSC handling, the response frame is constructed after `wal_append_and_fanout` is invoked, so the failure can be surfaced by overwriting the response frame to an error; for inline dispatch, the code currently unconditionally writes `+OK` and should instead emit an error (and ideally stop inlining further commands for that connection/batch) when the enqueue fails.

## Fix Focus Areas
- src/shard/spsc_handler.rs[2483-2561]
- src/shard/spsc_handler.rs[723-747]
- src/server/conn/blocking.rs[1425-1438]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unannotated unwrap() in wal_append_tests 📘 Rule violation ✧ Quality

Description

New .unwrap() calls were added in tests in src/shard/spsc_handler.rs and
src/persistence/aof/pool.rs without the required adjacent #[allow(clippy::unwrap_used)] and
immediately preceding justification comment. This violates the unwrap-annotation/audit convention
and can undermine ratcheting expectations or fail lint/audit gates that enforce annotated unwrap
usage.

Code

src/shard/spsc_handler.rs[R2783-2789]

+        let tmp = tempfile::tempdir().unwrap();
+        let wal_dir = tmp.path().join("wal");
+        let mut w3 = Some(WalWriterV3::new(0, &wal_dir, 16 * 1024 * 1024).unwrap());
+        w3.as_mut().unwrap().flush_sync().unwrap();
+        let seg = wal_dir.join("000000000001.wal");
+        let base_len = std::fs::metadata(&seg).unwrap().len();
+

Relevance

⭐⭐⭐ High

Repo enforces unwrap-annotation even in tests; similar request was accepted (PR #71).

PR-#71

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In src/shard/spsc_handler.rs, the newly added test test_kv_append_skipped_when_wal_kv_log_false
includes multiple .unwrap() calls (e.g., tempfile::tempdir().unwrap(),
WalWriterV3::new(...).unwrap(), and metadata reads) without an adjacent
#[allow(clippy::unwrap_used)] on the containing scope and without a direct justification comment
immediately above such an allow, which the rule requires. Similarly, in the added tests in
src/persistence/aof/pool.rs, .unwrap() is used (including on operations like try_send(...))
but there is no nearby single-line // ... justification comment immediately above a
#[allow(clippy::unwrap_used)] attribute attached to the relevant function/block, demonstrating the
same policy violation.

Rule 302083: Annotate safe unwrap calls with allow and justification
src/shard/spsc_handler.rs[2783-2813]
src/persistence/aof/pool.rs[1925-1932]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR introduces new `.unwrap()` calls in tests without meeting the project policy that each unwrap must be covered by an immediately attached `#[allow(clippy::unwrap_used)]` on the containing item/scope and an immediately preceding single-line justification comment explaining why the unwrap is safe.

## Issue Context
Policy requires each unwrap to be covered by:
- `// <why unwrap is safe>` directly above
- `#[allow(clippy::unwrap_used)]` directly attached to the containing item/scope (e.g., the test function or a narrower block)

Update the affected tests so every new unwrap is either removed (by handling errors explicitly) or is properly justified and locally allowed per the convention.

## Fix Focus Areas
- src/shard/spsc_handler.rs[2783-2813]
- src/persistence/aof/pool.rs[1925-2006]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. wal-kv-log typo fallback ✓ Resolved 🐞 Bug ☼ Reliability

Description

ServerConfig::wal_kv_log_mode maps any unrecognized --wal-kv-log string to Auto, so typos
silently change whether KV commands are logged to the WAL. This can unexpectedly disable WAL KV
history needed for PITR/CDC without any validation or warning.

Code

src/config.rs[R763-771]

+    /// Resolve `--wal-kv-log` into its mode. Unknown values fall back to
+    /// `Auto` (matches the string-flag convention used by `--wal-fpi` etc.).
+    pub fn wal_kv_log_mode(&self) -> WalKvLogMode {
+        match self.wal_kv_log.as_str() {
+            "on" => WalKvLogMode::On,
+            "off" => WalKvLogMode::Off,
+            _ => WalKvLogMode::Auto,
+        }
+    }

Relevance

⭐⭐ Medium

Mixed: strict unknown-token validation accepted in commands (PR #66) but clap flag validators
rejected (PR #157).

PR-#66
PR-#157

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The config field is a String and the resolver explicitly maps all unknown strings to Auto,
meaning clap will accept arbitrary values and the server will proceed without alerting the operator.

src/config.rs[339-351]
src/config.rs[763-771]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`--wal-kv-log` is stored as a free-form `String` and `wal_kv_log_mode()` falls back to `Auto` for any unknown value. This silently accepts operator typos and can change durability/CDC behavior without notice.

## Issue Context
This flag controls whether KV commands are logged into the WAL, which affects PITR/CDC expectations. Silent fallback makes misconfiguration hard to detect.

## Fix Focus Areas
- src/config.rs[339-351]
- src/config.rs[763-771]

## Suggested fix
- Use a clap-enforced enum (e.g., `#[derive(clap::ValueEnum)] enum WalKvLogMode { Auto, On, Off }`) and store the enum directly in `ServerConfig`.
 - Or, add a `value_parser` restricting values to `{auto,on,off}`.
- If you intentionally want permissive parsing, at least emit a `warn!` once on unknown values and include the provided string.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Tests added outside mod.rs 📘 Rule violation ▣ Testability

Description

New unit tests were added in src/persistence/aof/pool.rs, but this module is part of a split
directory-module (src/persistence/aof/mod.rs). This violates the requirement that tests for split
modules live only in mod.rs, which helps keep module-level test organization consistent.

Code

src/persistence/aof/pool.rs[R1916-1923]

+    /// RED before the fix: `try_send_append_durable` under EverySec returned
+    /// `Ok(())` while the append was silently dropped. Now the caller awaits
+    /// enqueue under `fsync_timeout` and gets `Err(AofAck::ChannelFull)` when
+    /// the writer stays full past the bound — so it returns an error frame
+    /// instead of a false `+OK`.
+    #[cfg(feature = "runtime-tokio")]
+    #[tokio::test]
+    async fn everysec_full_channel_errs_instead_of_silent_drop() {

Relevance

⭐ Low

Same “tests must be in mod.rs for split modules” suggestion was rejected before (GEO split-module
tests, PR #68).

PR-#68

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

src/persistence/aof/mod.rs declares a split module structure including mod pool;, while the PR
adds new #[tokio::test] tests inside the leaf implementation file src/persistence/aof/pool.rs,
which violates the split-module test location requirement.

Rule 302093: Keep test code for split Rust modules in mod.rs
src/persistence/aof/mod.rs[477-497]
src/persistence/aof/pool.rs[1916-2050]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/persistence/aof/` is a split module (directory `mod.rs` + submodules), but new tests were added to the leaf file `pool.rs`. Policy requires unit tests for split modules to live in the directory `mod.rs` only.

## Issue Context
Keeping tests centralized in `mod.rs` avoids scattering unit tests across sibling implementation files and simplifies review/maintenance.

## Fix Focus Areas
- src/persistence/aof/pool.rs[1916-2050]
- src/persistence/aof/mod.rs[477-497]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

2. Unannotated unwrap() in wal_append_tests 📘 Rule violation ✧ Quality

New .unwrap() calls were added in tests in src/shard/spsc_handler.rs and
src/persistence/aof/pool.rs without the required adjacent #[allow(clippy::unwrap_used)] and
immediately preceding justification comment. This violates the unwrap-annotation/audit convention
and can undermine ratcheting expectations or fail lint/audit gates that enforce annotated unwrap
usage.

Agent Prompt

## Issue description
The PR introduces new `.unwrap()` calls in tests without meeting the project policy that each unwrap must be covered by an immediately attached `#[allow(clippy::unwrap_used)]` on the containing item/scope and an immediately preceding single-line justification comment explaining why the unwrap is safe.

## Issue Context
Policy requires each unwrap to be covered by:
- `// <why unwrap is safe>` directly above
- `#[allow(clippy::unwrap_used)]` directly attached to the containing item/scope (e.g., the test function or a narrower block)

Update the affected tests so every new unwrap is either removed (by handling errors explicitly) or is properly justified and locally allowed per the convention.

## Fix Focus Areas
- src/shard/spsc_handler.rs[2783-2813]
- src/persistence/aof/pool.rs[1925-2006]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[coderabbitai]

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (1)

src/persistence/aof/pool.rs (1)

237-248: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale doc contradicts the new EverySec backpressure behavior.

The surrounding doc still describes EverySec/No as fire-and-forget: line 235 says it "stays on the fire-and-forget path (zero new latency)" and lines 244-248 say "The non-Always branch resolves immediately (no actual suspension)". With the new send_append_backpressure path, a full writer channel now suspends up to fsync_timeout and can return Err(AofAck::ChannelFull). The ~5 ns claim only holds for the fast (non-full) path. Please reconcile lines 235 and 244-248 with the new bounded-await semantics you added at 237-242.

🤖 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/pool.rs` around lines 237 - 248, The doc comments around
the Aof append/ack path are now stale and contradict the new bounded-await
behavior. Update the comments on the `send_append_backpressure`-based flow and
the surrounding `AofAck` handling to say EverySec/No can block up to
`fsync_timeout` and may return `Err(AofAck::ChannelFull)` when the writer
channel is full, rather than describing it as fire-and-forget or zero-latency.
Also qualify the `~5 ns`/“no actual suspension” claim so it only applies to the
fast non-full path, and keep the Always-path failure semantics accurate in the
same doc block.

🧹 Nitpick comments (1)

src/shard/event_loop.rs (1)

1180-1186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the triplicated wal_kv_log_mode match into a helper.

The same 6-line match resolving the log-enable boolean is duplicated verbatim at three call sites (tokio notify wake, tokio periodic tick, monoio every-wake). A small free function keeps the three sites in sync and avoids drift if the Auto heuristic changes later.

♻️ Proposed refactor

+fn wal_kv_log_enabled(
+    mode: crate::config::WalKvLogMode,
+    appendonly_enabled: bool,
+    cdc_registry: &crate::cdc::CdcSubscriberRegistry,
+) -> bool {
+    match mode {
+        crate::config::WalKvLogMode::On => true,
+        crate::config::WalKvLogMode::Off => false,
+        crate::config::WalKvLogMode::Auto => !appendonly_enabled || !cdc_registry.is_empty(),
+    }
+}

And at each call site:

-                        match wal_kv_log_mode {
-                            crate::config::WalKvLogMode::On => true,
-                            crate::config::WalKvLogMode::Off => false,
-                            crate::config::WalKvLogMode::Auto => {
-                                !appendonly_enabled || !cdc_registry.is_empty()
-                            }
-                        },
+                        wal_kv_log_enabled(wal_kv_log_mode, appendonly_enabled, &cdc_registry),

Also applies to: 1282-1288, 1860-1866

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/shard/event_loop.rs` around lines 1180 - 1186, Extract the repeated
wal_kv_log_mode boolean resolution into a shared helper function and call it
from all three sites (tokio notify wake, tokio periodic tick, monoio
every-wake). Move the existing WalKvLogMode::On/Off/Auto match logic into a
small free function or similarly named helper near event_loop, and replace each
duplicated match with that helper so the Auto heuristic lives in one place and
stays consistent.

🤖 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 `@scripts/gcloud-wal-aof-ab.sh`:
- Around line 205-210: `--measure-one` in the main subcommand dispatch can hit
an unbound-variable crash before `die` runs because `measure_single` is called
with `$2` and `$3` unconditionally. Update the argument handling around the
`case` in `gcloud-wal-aof-ab.sh` to validate that `--measure-one` has both
required parameters before referencing them, and route missing-arg cases to
`die` with a usage message; use the `measure_single` and `die` symbols to keep
the fix localized.

In `@src/persistence/aof/pool.rs`:
- Around line 528-546: The timeout handling around send_fut in the AOF pool
write path can misreport a successful append as ChannelFull because
flume::Sender::send_async is not cancel-safe. Update the outcome logic in the
AofAck write/backpressure branch to use a cancel-safe strategy instead of
directly timing out send_fut, and make sure the AOF_BACKPRESSURE_DROPPED/client
response only reflects writes that are truly not delivered.

In `@src/shard/spsc_handler.rs`:
- Around line 2553-2559: The batched AOF append path in the shard handler resets
backpressure for every write, which can block the event loop for N times the
bounded wait. Update the `MultiExecute` and `PipelineBatch(Slotted)` flow in
`spsc_handler` to track a single remaining deadline or budget across the whole
batch, and pass the reduced timeout into each `send_append_bounded_blocking`
call instead of always using the full `AOF_SPSC_BACKPRESSURE_BOUND`. Keep the
change localized to the batch-processing logic around `aof_pool` so unrelated
connections are not stalled by large batches.

---

Outside diff comments:
In `@src/persistence/aof/pool.rs`:
- Around line 237-248: The doc comments around the Aof append/ack path are now
stale and contradict the new bounded-await behavior. Update the comments on the
`send_append_backpressure`-based flow and the surrounding `AofAck` handling to
say EverySec/No can block up to `fsync_timeout` and may return
`Err(AofAck::ChannelFull)` when the writer channel is full, rather than
describing it as fire-and-forget or zero-latency. Also qualify the `~5 ns`/“no
actual suspension” claim so it only applies to the fast non-full path, and keep
the Always-path failure semantics accurate in the same doc block.

---

Nitpick comments:
In `@src/shard/event_loop.rs`:
- Around line 1180-1186: Extract the repeated wal_kv_log_mode boolean resolution
into a shared helper function and call it from all three sites (tokio notify
wake, tokio periodic tick, monoio every-wake). Move the existing
WalKvLogMode::On/Off/Auto match logic into a small free function or similarly
named helper near event_loop, and replace each duplicated match with that helper
so the Auto heuristic lives in one place and stays consistent.

🪄 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: 10d967fa-da60-4e32-8d3c-278cac006653

📥 Commits

Reviewing files that changed from the base of the PR and between 2bbc2dc and 80af5eb.

📒 Files selected for processing (22)

• CHANGELOG.md
• docs/configuration.md
• docs/guides/configuration.md
• docs/guides/persistence.md
• scripts/gcloud-wal-aof-ab.sh
• src/config.rs
• src/persistence/aof/mod.rs
• src/persistence/aof/pool.rs
• src/server/conn/blocking.rs
• src/shard/event_loop.rs
• src/shard/mod.rs
• src/shard/spsc_handler.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