feat(replication): Wave A plane replication — eviction/expiry DELs + Lua write effects on both planes#285
Conversation
8 black-box tests, all failing on the missing features by design: eviction parity (s1+s4), EVAL effects parity (s1+s4), EVAL-write AOF durability across kill-9, evicted-key resurrection from AOF replay, expiry DEL emission (offset must advance), SELECT-in-script loud error. author: Tin Dang <tindang.ht97@gmail.com>
…n-drops Wave A part 1 (task #34): a key removed by active TTL expiry or by memory-pressure eviction previously vanished from the master's own keyspace without telling either durability plane. An attached replica kept serving the stale key forever, and a kill -9 + restart against --appendonly yes resurrected it from the AOF replay (the AOF recorded the original SET but never the removal). Add src/replication/reason_del.rs with two call shapes for the two places a reason-based removal happens: - record_reason_del — shard-event-loop context (background active expiry, background eviction tick). Reuses shard::spsc_handler::wal_append_and_fanout verbatim, the same mechanism ShardMessage::SwapDb already uses for a synthetic write with no originating client command: one call does the WAL v3 log, the replication backlog append + offset advance + deferred live fan-out, and the AOF pool append. - record_reason_del_conn — connection-handler context (the monoio inline SET fast path and the generic per-command write-eviction gate). Mirrors handler_monoio::ft::record_local_write_db's mechanics (fused SELECT for multi-shard masters, emit-on-change SELECT tracking for single-shard masters, backlog append + offset advance + deferred ShardMessage::ReplicaLiveFanout push) and adds the AOF leg record_local_write_db omits, since a synthetic eviction DEL has no client command to hang the existing per-command AOF gate off of. Wire both flavors into every genuine plain-drop site, while leaving every hot-to-cold spill/tiering path emitting nothing (a spilled key stays cold-readable, not deleted): - server/expiration.rs: expire_cycle / expire_cycle_direct gain a removed-key sink parameter, invoked only for the whole-key TTL sweep (hash-field TTL reaps are out of scope for this wave). - shard/timers.rs: run_active_expiry and run_eviction thread the shard-loop's WAL/backlog/fanout/AOF handles through to the sink. - shard/persistence_tick.rs: run_eviction_tick and handle_memory_pressure thread the same handles to the sync-spill fallback branch that has no manifest (a true plain-drop); the durable-manifest branch is left calling the original non-reporting function since that victim is never actually deleted. - storage/eviction.rs: evict_one_with_spill gains an on_plain_drop sink, fired only when spill was None for that victim (captured before spill is consumed). try_evict_if_needed_budget, try_evict_if_needed_with_spill_and_total_budget, and try_evict_if_needed_async_spill_with_total_budget each split into a _reporting variant plus a thin no-op-sink wrapper preserving the original public signature, so out-of-scope callers (Lua bridge, tokio-only handlers, storage/db_quota.rs's per-db quota path, shard/spsc_two_db.rs's cross-db COPY destination eviction) need no changes. - server/conn/blocking.rs (try_inline_dispatch) and server/conn/handler_monoio/mod.rs (run_write_eviction_gate): the two connection-handler-context write-eviction gates now emit via record_reason_del_conn. - shard/spsc_handler.rs: spsc_eviction_gate gains the same sink parameter; all 6 cross-shard write call sites wire it to record_reason_del. Also fixes a pre-existing replication gap uncovered while chasing the eviction-parity tests: try_inline_dispatch's monoio SET fast path only ever fed the AOF, never the replication backlog/fan-out at all (an attached replica never saw a lone `SET foo bar` under --disk-offload disable, independent of eviction). can_inline_writes now also requires !fanout_hint_active(), falling back to the fully- replicating generic dispatch path once any replica has ever attached — the same "fall back once the fast path can't do enough" precedent ctx.spill_sender.is_none() already established for disk-offload. Turns 4 of the committed RED tests in tests/replication_planes.rs green: eviction_parity_shards1, eviction_parity_shards4, evicted_keys_stay_dead_after_restart, expiry_del_propagates. The 4 Lua/EVAL scenarios (eval_effects_parity_shards1/4, eval_writes_survive_restart, select_in_script_errors) are untouched and remain red, owned by a follow-up agent in src/scripting/. Verified: cargo test --release --lib (4115 passed) and cargo test --no-default-features --features runtime-tokio,jemalloc --lib (3316 passed) both clean; cargo clippy -D warnings clean on both feature matrices; cargo fmt --check clean; replication_multishard (9/9), replication_streaming (7/7), replication_hardening (5/5), aof_multidb_kill9 (4/4), and oom_bypass_closure (8/8) all still green. Known follow-ups left as documented no-op gaps, out of scope for this wave: hash-field TTL reaps, Lua write effects, storage/db_quota.rs's per-db maxmemory eviction, and spsc_two_db.rs's cross-db COPY destination eviction do not yet emit reason-DEL records. Refs: task #34, .planning/rfcs/plane-replication-design.md (Wave A) author: Tin Dang <tindang.ht97@gmail.com>
…ut (Wave A part 2) Part 1 (f543183) gave background expiry/eviction plain-drops a dual-plane DEL emission path but left Lua scripting untouched. Before this commit, a Lua script's `redis.call`/`redis.pcall` write effects reached NEITHER durability plane: EVAL/EVALSHA deliberately carry no WRITE command-metadata flag, so the generic per-command AOF/replication gate in handler_monoio never saw them. A script's writes vanished on kill -9 + restart against --appendonly yes, and an attached replica never observed a script's writes at all. Separately, redis.call('SELECT', ...) inside a script silently corrupted state: dispatch's SELECT handler mutated only a local variable, so every subsequent write in the script kept landing in the ORIGINAL db while looking like it had switched. scripting::bridge::make_redis_call_fn now records every successfully- executed, WRITE-flagged inner command to both planes itself, immediately after each redis.call/redis.pcall returns — not batched to script end, so a script that writes two keys and then errors on a third still durably records the first two. New replication::reason_del::record_effect_write (sharing a record_bytes_conn core refactored out of part 1's record_reason_del_conn) does the emission, reusing the identical fused- SELECT/backlog/offset/fan-out mechanics every other write path already uses, recording the verbatim cmd+args the script invoked. LuaEvictionCtx (built once per shard at Lua-VM setup, already caching the OOM eviction handles) now also carries num_shards/repl_state/aof_pool for this emission. Every production construction site picked up the three new arguments: the 4 call sites in shard::conn_accept (tokio, monoio, and their migrated-connection twins) plus server::conn::core::ConnectionContext::build_lua_eviction_ctx, which is shared by EVAL/EVALSHA and FCALL/FCALL_RO — both route through the same bridge closure, so FCALL gets the identical effect emission with no extra wiring. redis.call('SELECT', ...) inside any script now fails loud with "ERR SELECT inside scripts is not supported by moon yet" instead of silently corrupting state, intercepted before dispatch so no partial write from the same call ever lands. A real multi-db-scripts feature is a follow-up. Deliberately did NOT flip EVAL/EVALSHA to WRITE in the command metadata table: that would route the literal "EVAL <script> ..." invocation through the generic per-command AOF/replication gate ON TOP OF the effect records above, double-applying every write the script made (e.g. an INCR landing as 2 instead of 1). This invariant is guarded by a new unit test (command::metadata::eval_evalsha_never_write_flagged) and a black-box regression test (eval_incr_no_double_apply). FCALL, unlike EVAL, IS WRITE-flagged in the metadata table (mirrors upstream Redis Functions, where FCALL always requires write permission and FCALL_RO is the read-only variant) — that flag is confirmed to only feed ACL/READONLY-replica gating, since try_handle_functions always consumes FCALL with `continue` before the generic AOF/replication block runs, so it rides the same single-emission bridge path without risk of a second recording. Turns the 4 remaining RED tests in tests/replication_planes.rs green: eval_effects_parity_shards1, eval_effects_parity_shards4, eval_writes_survive_restart, select_in_script_errors. Added eval_incr_no_double_apply as the targeted regression guard requested for this wave. All 9 tests in the suite now pass (the 5 from part 1 plus this new one). Known gap, unchanged from before this commit: a write-EVAL issued directly against a read-only replica is not rejected (try_enforce_readonly only gates on the WRITE metadata flag, which EVAL intentionally lacks). This cannot cause replication divergence on its own (replicas never receive EVAL itself over the wire, only the effect records), but a client misdirected to write against a replica directly could locally corrupt that replica's state. Tracked as a follow-up, out of Wave A's scope per the RFC. Verified: cargo test --release --lib (4115 passed) and cargo test --no-default-features --features runtime-tokio,jemalloc --lib (3316 passed) both clean; cargo clippy -D warnings clean on both feature matrices; cargo fmt --check clean. replication_planes 9/9, replication_multishard 9/9, replication_streaming 7/7, replication_hardening 5/5, aof_multidb_kill9 4/4 all green. Scripting coverage: 13 EVAL/EVALSHA/ SCRIPT/sandbox integration tests (tests/integration.rs, runtime-tokio) and 9 FUNCTION/FCALL/FCALL_RO integration tests (tests/functions_fcall.rs) green, confirming FCALL's shared bridge path is unaffected. Refs: task #34, .planning/rfcs/plane-replication-design.md (Wave A) author: Tin Dang <tindang.ht97@gmail.com>
…roof guards The suite hardcoded ports 17301-17382 and spawned children before Guard construction: a panic in await_ready leaked raw Children, and because moon binds SO_REUSEPORT a leaked server silently SHARES the fixed port with the next run and splits its connections (observed on the Linux VM: three masters on port 17301 after two failed runs, cascading 5/9 failures). Now spawns via common::spawn_listening into the guard immediately; only the two same-port restart legs keep direct fixed-port spawns. author: Tin Dang <tindang.ht97@gmail.com>
…ecord allocs Three defects found in an adversarial review of Wave A (plane replication, task #34), fixed with red/green tests where the code path is reachable via a unit test, and black-box replica-convergence tests where it isn't. 1. Non-string eviction victims under a live `SpillContext` were dropped with no cold copy AND no DEL record. `storage::eviction:: evict_one_with_spill` snapshotted `is_plain_drop = spill.is_none()` BEFORE its spill body ran, but that body only ever spills `RedisValueRef::String` values -- a Hash/List/Set/ZSet victim picked under a `SpillContext` took neither branch of the `is_string` check (no bytes written anywhere) yet fell through to the unconditional `db.remove` unreported, because `is_plain_drop` was already `false`. Fixed by tracking whether THIS victim was actually spilled (`spilled`, set only inside the branch that performs the write) instead of snapshotting the precondition. Also threaded the real `record_reason_del` sink into `persistence_tick::handle_memory_pressure`'s durable-spill branch, which previously passed a hardcoded no-op sink. Black-box testing this surfaced that the narrowly-described `SpillContext` path is dead code via any current CLI configuration (`spill_thread`/`shard_manifest` are co-gated on the same `disk_offload_enabled()` check). The two call sites an ordinary write actually reaches under `--disk-offload enable` -- `handler_monoio::run_write_eviction_gate` and `spsc_handler::spsc_eviction_gate` -- had the identical unwired-sink bug in their spill-sender branch and are fixed the same way. 2. `record_reason_del`/`record_reason_del_conn`/`record_effect_write` allocated a fully serialized RESP record BEFORE checking whether any replica or AOF pool was even wired -- paid on every background-tick DEL and script write effect on the most common deployment shape (standalone, no replica, `--appendonly no`), only to be discarded a few instructions later. Fixed by hoisting the existing has-work predicate (`wal_fanout_has_work` / a new `conn_has_work`) to the top of all three entry points, before any serialization. Behavior when there IS work is unchanged (same predicate, same downstream call). 3. Lua's bystander-eviction gate (`scripting::bridge::LuaEvictionCtx::gate`) called the non-reporting eviction variants, so a script-triggered bystander eviction never reached the AOF/replication planes. New `try_evict_if_needed_async_spill_budget_reporting` wrapper; `gate` now threads a real sink through to `record_reason_del_conn`. Verification: replication_planes (11/11, sequential, --test-threads 1; one of four full-suite runs saw the pre-existing, untouched `expiry_del_propagates` timing flake -- confirmed unrelated, same function at base commit 152a39c, passes 3/3 in isolation), oom_bypass_closure (8/8, no spill-path regression), cargo clippy -D warnings clean on both the default and --no-default-features --features runtime-tokio,jemalloc matrices, cargo fmt --check clean, cargo test --lib green on both matrices (4121 passed / 3320 passed, 0 failed). Also documents three known Wave A limitations in CHANGELOG.md: redis.pcall('SELECT', ...) swallowing the rejection error, nondeterministic write commands replicating verbatim from scripts, and fanout_hint_active's sticky one-way-latch semantics. author: Tin Dang <tindang.ht97@gmail.com>
…replica catch-up) The 200ms TTL raced the replica's catch-up under load (~1-in-4 on a busy host): keys expired before dbsize(replica) reached N, failing the SETUP invariant, never the feature. TTL now 3s with an explicit guard that the baseline offset was captured before any TTL could fire. author: Tin Dang <tindang.ht97@gmail.com>
…ery replica-attached server Five scenario tails cleared the Guard's Vec<Child> after passing: clearing drops the Child HANDLES (std Child never kills on drop) so Guard::drop later iterated an empty vec — every replica-attached test leaked both servers on macOS AND Linux (48 orphans accumulated on the VM across three suite runs, verified guarded pids == leaked pids, GUARD drop never logged). Suite green 11/11 with zero leaked processes after removal. author: Tin Dang <tindang.ht97@gmail.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds Wave A durability reporting for plain drops, expirations, evictions, and Lua write effects across replication and AOF paths. It also rejects ChangesWave A durability and replication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LuaBridge
participant Database
participant DurabilityPlanes
Client->>LuaBridge: EVAL or EVALSHA
LuaBridge->>Database: execute inner redis.call write
Database-->>LuaBridge: successful command result
LuaBridge->>DurabilityPlanes: emit verbatim write effect
LuaBridge->>DurabilityPlanes: report any plain-drop eviction
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/shard/event_loop.rs (1)
1654-1660: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider hoisting the repeated
wal_kv_log_mode→ bool computation.The same
match wal_kv_log_mode { On => true, Off => false, Auto => !appendonly_enabled || !cdc_registry.is_empty() }is inlined at ~7 call sites in this function (run_active_expiry, run_eviction_tick, and the drain_spsc_shared calls). A local closure recomputed just before each dispatch (so it still reflects the livecdc_registrystate) would remove the duplication and prevent the Auto formula from drifting across sites.let wal_kv_log_now = || 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(), };🤖 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 1654 - 1660, In the enclosing event-loop function, hoist the repeated wal_kv_log_mode-to-bool match into a local closure such as wal_kv_log_now, then replace the inlined matches at run_active_expiry, run_eviction_tick, and each drain_spsc_shared dispatch with calls to that closure. Invoke it immediately before each dispatch so Auto continues reading the current cdc_registry state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 11-12: Update the Wave A summary in CHANGELOG.md to state “Four
defects” instead of “Three defects,” leaving the four documented defect entries
unchanged.
- Around line 157-160: Update the Wave-A-scoped gaps entry in the changelog to
remove the resolved Lua `redis.call` write-effects item, or explicitly qualify
it as a historical part-1 limitation; retain the other unresolved limitations
unchanged.
- Around line 207-211: Update the replication_planes.rs changelog entry to
report all 11 ignored scenarios, including the two disk-offload hash-parity
tests, instead of stating that all 9 tests pass.
---
Nitpick comments:
In `@src/shard/event_loop.rs`:
- Around line 1654-1660: In the enclosing event-loop function, hoist the
repeated wal_kv_log_mode-to-bool match into a local closure such as
wal_kv_log_now, then replace the inlined matches at run_active_expiry,
run_eviction_tick, and each drain_spsc_shared dispatch with calls to that
closure. Invoke it immediately before each dispatch so Auto continues reading
the current cdc_registry state.
🪄 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: 2cc6d45b-765c-4ae7-b417-23547c54b6da
📒 Files selected for processing (19)
CHANGELOG.mdsrc/command/hash/mod.rssrc/command/metadata.rssrc/replication/mod.rssrc/replication/reason_del.rssrc/scripting/bridge.rssrc/server/conn/blocking.rssrc/server/conn/core.rssrc/server/conn/handler_monoio/mod.rssrc/server/expiration.rssrc/shard/conn_accept.rssrc/shard/event_loop.rssrc/shard/persistence_tick.rssrc/shard/spsc_handler.rssrc/shard/spsc_two_db.rssrc/shard/timers.rssrc/storage/db_quota.rssrc/storage/eviction.rstests/replication_planes.rs
| Three defects found reviewing Wave A (plane replication) before merge, all | ||
| fixed on top of the branch's part 1/2/3 work below. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the defect count.
The section says “Three defects” but documents four top-level defects beginning on Lines 14, 31, 49, and 65.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 11 - 12, Update the Wave A summary in CHANGELOG.md
to state “Four defects” instead of “Three defects,” leaving the four documented
defect entries unchanged.
| - Known Wave-A-scoped gaps (documented, not silent): hash-field TTL reaps, | ||
| Lua `redis.call` write effects, per-db quota (`--db-maxmemory`) eviction, | ||
| and cross-db `COPY ... DB n` destination eviction do not yet emit — | ||
| tracked as follow-ups. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the resolved Lua-effects gap from the part-1 limitations.
This says Lua redis.call write effects “do not yet emit,” but the part-2 entry below documents that they now emit to both planes. Qualify this as a historical part-1 limitation or remove that item.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 157 - 160, Update the Wave-A-scoped gaps entry in
the changelog to remove the resolved Lua `redis.call` write-effects item, or
explicitly qualify it as a historical part-1 limitation; retain the other
unresolved limitations unchanged.
| - Turns the 4 remaining RED tests in `tests/replication_planes.rs` green: | ||
| `eval_effects_parity_shards1`, `eval_effects_parity_shards4`, | ||
| `eval_writes_survive_restart`, `select_in_script_errors`. All 9 tests in | ||
| the suite (the 5 from part 1 plus the new `eval_incr_no_double_apply`) | ||
| pass. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the regression-suite count.
tests/replication_planes.rs currently defines 11 ignored scenarios, including the two disk-offload hash-parity tests added above, not nine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 207 - 211, Update the replication_planes.rs
changelog entry to report all 11 ignored scenarios, including the two
disk-offload hash-parity tests, instead of stating that all 9 tests pass.
Summary
Wave A of task #34 (RFC
.planning/rfcs/plane-replication-design.md): the three GA-blocking write planes that mutated replicated state invisibly now emit records to BOTH planes (AOF + replication). Until this PR, a replica silently diverged — and in two cases the MASTER itself lost data across restart:redis.callwrite effectsDesign
record_reason_del(newsrc/replication/reason_del.rs): db-awareDELrecord emitted at every genuine plain-drop — active-expiry sweep (expire_cyclegained a removed-key sink), background eviction tick, memory-pressure fallback, and the three write-path eviction gates (inline, generic, SPSC cross-shard). Same mechanics as ordinary writes: fusedSELECTatnum_shards>1, backlog append + shard-offset advance + deferredReplicaLiveFanout, plus the AOF leg. Spill paths never emit — a key that lands in the cold tier is not a delete;is_plain_dropis computed from whether a durable spill actually happened (not from whether a spill context was passed — non-string victims can't spill and ARE plain drops).src/scripting/bridge.rs): after each WRITE-flagged inner command returns non-error, the bridge emits the command verbatim to both planes (record_effect_write). EVAL/EVALSHA deliberately stay un-WRITE-flagged (pinned by a metadata unit test) so the generic gate can't double-log —eval_incr_no_double_applyproves exactly-once end to end. Partial scripts record their completed writes (effects semantics). The bridge's own bystander-eviction gate reports through the same helper.SELECTinside scripts now errors loudly. Previously it silently wrote to the WRONG db (dispatch mutated the db index while the script kept writing the originalDatabase). Real multi-db scripts are follow-up task chore(deps): bump tokio from 1.50.0 to 1.51.0 in the async-runtime group #38.fanout_hint_active).Verification (red/green + standing replication gates)
-D warningsboth matrices; fmt.Test-harness fixes shipped alongside (found by the VM runs)
tests/replication_planes.rsinitially used fixed ports + spawned children before Guard construction; with SO_REUSEPORT a leaked server silently shares the port with the next run (three masters on one port observed). Now uses the test(harness): TOCTOU-safe shared port/spawn helpers across all 33 server-spawning suites #284spawn_listeninghelpers.guard.0.clear()— which drops theChildhandles WITHOUT killing (stdChildhas no kill-on-drop), so every replica-attached test leaked both servers on both platforms. Removed; suite verified zero-leak.expiry_del_propagatesde-raced (200ms TTL raced replica catch-up under load; now 3s with an explicit baseline-capture guard).Known limitations (documented in CHANGELOG)
redis.pcall('SELECT', ...)swallows the new error into a Lua table (loud only viaredis.call).SPOPetc.) replicate verbatim — pre-existing verbatim-replication class, first exposed for Lua here.fanout_hint_activeis sticky: once any replica has attached, the inline-protocol write fast path stays off for the process lifetime.runtime-tokiothe new emission hooks are no-ops (master-side replication is monoio-only; pre-existing).Wave B (WS./MQ. deterministic effect records) remains under task #34.
Summary by CodeRabbit
New Features
SELECTnow fail clearly to prevent unintended database changes.Bug Fixes
Tests