release(0.5.0): KV correctness (v3-4) + p=1/multi-shard throughput + Arc reply-slot UAF fix#207
Merged
Merged
Conversation
… v3-3 scope) Doc-only follow-through from the 2026-07-02 KV deep review. No code changes. - BENCHMARK.md: clarify that the 0.46-0.51x p=1 figure is a bench-production.sh confound (distributed -r keys + 512B-4KB values + c=100/200), not a clean shard-count signal; the cross-shard hop is ~10us (~2% of the ~460us baseline), and clean bench-compare is flat 0.79->0.82 across shards. - CLAUDE.md: correct the pending_wakers lock rule. The cross-shard reply path awaits a flume oneshot directly; the pending_wakers relay is still swept each event-loop iteration (pub/sub + backpressure) but is no longer the reply-wake mechanism (test swf0 retired that premise; event_loop.rs ~1730). - v3-3-vector-kv-polish/MILESTONE.md: fold the review's KV hot-path findings into scope -- kv-inline-get-nocopy (blocking.rs:1275 double-copy) and the inline-path std::sync::RwLock replica-check site (handler_monoio/mod.rs:524) into kv-dispatch-lock-discipline. Refs: tmp/KV-DEEP-REVIEW.md (durable review deliverable, gitignored) author: Tin Dang
GETDEL called db.remove(key) BEFORE checking the value type. For a key holding a non-string (hash/list/set/zset), the key was destroyed and the handler then returned WRONGTYPE -- the client saw an error implying nothing happened while the data was already gone. Unrecoverable. Fix: peek the type with a cheap non-cloning as_bytes() borrow first; return WRONGTYPE without mutating if the value is not a string, and only call db.remove() once the string type is confirmed. Matches Redis check-before- mutate semantics (APPEND/GETRANGE already do this). Red/green TDD: test_getdel_wrongtype_preserves_key asserts the wrong-type key still exists after GETDEL returns WRONGTYPE -- failed before the fix (key gone), passes after. Refs: tmp/KV-AUDIT-CORRECTNESS.md #1 (P0 CRITICAL); tmp/KV-DEEP-REVIEW.md §7 author: Tin Dang
… (data loss) Both SET key val GET and legacy GETSET computed the WRONGTYPE error for a non-string key but then ran db.set/db.set_string UNCONDITIONALLY, overwriting (destroying) the hash/list/set/zset before returning the error. The client saw WRONGTYPE while the original value was already gone. Unrecoverable. Fix (Redis check-before-mutate parity): - SET: return WRONGTYPE immediately after computing old_value, BEFORE the NX/XX blocks and before db.set -- the wrong-type error takes precedence over NX/XX, matching Redis's evaluation order. - GETSET: only call db.set_string when the old value is not a wrong-type error; otherwise return WRONGTYPE untouched. Red/green TDD: test_set_get_option_wrongtype_preserves_key and test_getset_wrongtype_preserves_key assert a plain GET after the op still reports WRONGTYPE (key unchanged). Both failed before (GET returned the new string), pass after. Full command::string suite green (79 tests). Refs: tmp/KV-AUDIT-CORRECTNESS.md #1 (P0 CRITICAL); tmp/KV-DEEP-REVIEW.md §7 author: Tin Dang
…low guard
Two correctness bugs on the key-TTL path:
1. Non-positive TTL (Redis parity). EXPIRE/PEXPIRE with seconds/millis <= 0
returned "ERR invalid expire time" and left the key in place. Redis treats a
non-positive TTL as a past-time expiry and DELETES the key immediately,
returning 1 (or 0 if absent). EXPIREAT already did this; EXPIRE/PEXPIRE now
mirror it. The doc comment that mislabeled the error path as "modern Redis 7+
behavior" is corrected.
2. Unchecked u64 overflow. now_ms + seconds*1000 wrapped silently in release and
panicked in debug ("attempt to multiply with overflow") for large seconds
(up to i64::MAX). Now guarded with checked_mul/checked_add; an out-of-range
expiry returns an error and leaves the key untouched, matching Redis.
Propagation verified (advisor gate): WAL replay re-dispatches the raw command
via crate::command::dispatch, so replaying `EXPIRE k -1` re-runs the fixed
handler and deletes the key -- a WAL-recovered replica stays consistent with the
master. New test persistence::replay::replay_expire_nonpositive_deletes_key
proves this (RED before fix: replay kept the key; GREEN after). CdcOp still
classifies EXPIRE as Upsert, which is pre-existing and shared with EXPIREAT --
it does not affect KV replay (raw command is re-dispatched regardless).
Red/green TDD: test_expire_negative replaced by test_expire_nonpositive_deletes;
added test_expire_nonpositive_missing_key, test_expire_overflow_rejected,
test_pexpire_nonpositive_deletes, and the two replay guards. key + replay suites
green (68 tests).
Refs: tmp/KV-AUDIT-CORRECTNESS.md #3 (P0 HIGH); tmp/KV-DEEP-REVIEW.md §7
author: Tin Dang
…ror) Several arithmetic sites could panic in debug (attempt to multiply/negate with overflow) and silently wrap in release, given adversarial-but-valid i64 input: - DECRBY key <i64::MIN>: negating i64::MIN is unrepresentable. Now checked_neg returns "ERR decrement would overflow" (INCRBY/DECR already used checked_add for the value itself; only the DECRBY pre-negation was unguarded). - SET ... EX / EXAT, SETEX, EXPIREAT: (seconds|timestamp) * 1000 (+ now_ms) overflowed u64 for large values. Now guarded with checked_mul/checked_add; an out-of-range expiry returns "ERR invalid expire time" and performs no write, matching Redis. PSETEX/PEXPIRE/PEXPIREAT/SET PX/PXAT operate in ms and cannot overflow u64 from a valid i64, so they are left as-is (SETEX/PSETEX <=0-is-error is already correct and unchanged). Red/green TDD: test_decrby_min_overflow, test_setex_overflow_rejected, test_set_ex_overflow_rejected, test_expireat_overflow_rejected. All panicked before the fix, return clean errors after; the affected key is untouched. string + key suites green (141 tests). Refs: tmp/KV-AUDIT-CORRECTNESS.md #4 (P0); tmp/KV-DEEP-REVIEW.md §7 author: Tin Dang
Formatting-only: rustfmt wrapping of long assert_eq!/assert! lines in the EXPIRE/PEXPIRE/EXPIREAT and replay test additions. No logic change. author: Tin Dang
Moon was missing MSETNX. Add it as a Redis-parity atomic multi-key write:
set every key/value pair iff NONE of the keys already exist; return 1 when
all were set, 0 when any key existed (writing nothing).
MSETNX's all-or-nothing contract cannot be honored atomically across shards
(like MSET, cross-shard writes scatter with no two-phase commit or rollback).
Per design decision, the coordinator REJECTS a MSETNX whose keys span more
than one shard with a CROSSSLOT error and writes nothing; when the keys are
co-located on a single shard (naturally or via a {hash-tag}) it runs the
whole command atomically on that shard's owner via run_on_owner.
- string::msetnx: single-shard two-phase handler (check-all, then set-all;
no await between phases -> atomic on one database).
- coordinate_msetnx: groups keys by shard, CROSSSLOT on span>1, else routes
the intact command to the owning shard (local dispatch or remote MultiExecute).
- Wired into is_multi_key_command, coordinate_multi_key, the phf metadata
table (arity -3, write, keys 1..-1 step 2), and the (6,'m') dispatch arm.
Tests (red/green verified):
- unit: all-new -> 1, one-exists -> 0 (no partial), odd-arity error.
- integration (tests/msetnx_cross_shard_reject.rs, --shards 4): provably
cross-shard keys -> CROSSSLOT + no partial write; co-located {t} keys ->
atomic 1/0. RED confirmed by neutralizing only the CROSSSLOT branch (the
cross-shard case regressed to Int(1) while co-located stayed green).
- scripts: MSETNX entries in test-consistency.sh (hash-tagged for 1/4/12
shard parity with Redis) and test-commands.sh.
author: Tin Dang
…TL wrap The expire-overflow guards added earlier in this milestone bounded the arithmetic against u64::MAX, but expiry is stored as u64 and read back through PTTL/PEXPIRETIME as i64 — an absolute expiry in (i64::MAX, u64::MAX] was accepted, then surfaced as a NEGATIVE TTL on a key that is very much alive (e.g. `EXPIRE k 15000000000000000` -> PTTL < 0). Redis rejects such out-of-range expiries outright (`when > LLONG_MAX/1000`). Add a shared `expiry_ms_in_range` helper (u64 <= i64::MAX) and apply it at every expiry-setting site: EXPIRE, PEXPIRE, EXPIREAT (key.rs) and SET EX/PX/EXAT, SETEX, PSETEX (string_write.rs). Also reorder EXPIRE/EXPIREAT so an extreme negative (`seconds < i64::MIN/1000`) errors instead of taking the past-time delete branch — matching Redis, which bounds negatives before deleting. Found by the v3-4 adversarial code review (Findings 2 & 3): tightens the kv-integer-overflow-guard task so the exit criterion "not a wrapped TTL" holds on the READ path too, not just the set path. Red/green TDD: 9 new unit tests (i64-bound reject + extreme-negative preserve) across key.rs and string/mod.rs — RED before, GREEN after. Full default lib suite 3642 pass, tokio+jemalloc 2984 pass, both clippy gates + fmt clean. author: Tin Dang
…sclosure) Record the whole-diff adversarial code review (SHIP verdict) in the v3-4 MILESTONE.md ship-review: - Finding 2/3 (i64-domain expiry bound) FIXED in d6b4136 — evidence row updated with the new commit and the 9 red/green tests. - Finding 1 (pre-existing cross-shard coordinator local-leg WAL durability asymmetry) disclosed with the verified blast radius and deferred to a dedicated P0 follow-up (out of v3-4 scope — a co-located MSET/MSETNX on the connection's own shard is not persisted, while the remote leg is; NOT an MSETNX regression). Also commits the .add/state.json registration of v3-4 as the active milestone. author: Tin Dang
… AOF The cross-shard coordinator's LOCAL leg executed co-located multi-key writes in memory but never appended them to the owning shard's AOF, while the REMOTE leg (MultiExecute in spsc_handler) always persists via wal_append_and_fanout. So a co-located MSET/MSETNX was durable when the owner was a remote shard but silently non-durable when the owner was the connection's own shard. Blast radius: multi-shard (--shards >1) + appendonly + keys hashing to the connection's own shard. Single-shard (the recommended default) was never affected — the coordinator is bypassed for num_shards<=1 and normal dispatch persists. Pre-existing; not introduced by MSETNX (it copied the existing MSET coordinator pattern). Fix: the local leg now persists to the owning shard's AOF via a new persist_local_leg helper — the same AofWriterPool::issue_append_lsn + try_send_append_durable path every local single-key write already uses (the local-write contract, not the SPSC remote path; ChannelMesh has no self-send slot, so a local leg cannot route through wal_append_and_fanout). Granularity: the whole command for a co-located owner (MSETNX; MSET fast path), and a synthesized MSET over only the local keys for a scattered MSET's local slice (never the full scattered command — my_shard does not own the remote keys, and replay re-dispatches raw commands). On AOF append failure the leg returns AOF_FSYNC_ERR instead of a false +OK (design-for-failure). This strictly adds durability and changes nothing about replication (live fan-out via replica_txs is SPSC-only and untouched). Red/green crash-recovery TDD in tests/coordinator_local_leg_durability.rs (--shards 4 --appendonly yes; one co-located group per shard so exactly one is the local leg regardless of SO_REUSEPORT landing; SIGKILL -> restart -> reload): RED before, GREEN after, on both monoio and tokio. fmt + both clippy gates clean; full lib suites green on both runtimes. Remaining (tracked follow-up, same mechanism, out of this KV milestone's command scope): coordinator BITOP / COPY (via run_on_owner's local branch) and DEL / UNLINK (via coordinate_multi_del_or_exists) carry the identical local-leg non-durability and are not yet fixed. Resolves v3-4 milestone Finding 1 (HIGH). author: Tin Dang
The inline GET fast path materialized every hit's value through an intermediate `val.to_vec()` (heap alloc + memcpy) before framing it into `write_buf`. Frame the `$<len>\r\n<bytes>\r\n` reply directly from the borrowed `&[u8]` inside the `with_shard_db` closure instead, so the value is copied exactly once (borrow -> write_buf). The closure stays non-reentrant and await-free. Byte-parity test pins the reply framing across the CompactValue SSO boundary (0/1/12/13/65536-byte values) so the refactor cannot change a single reply byte. Part of the kv-s1-outperform goal (p=1 hot-path polish, v3-3). author: Tin Dang
…hot path GCloud c4a (ARM Axion) n=3 showed Moon losing p=1 single-op latency to Redis by ~8% while winning on x86 — an asymmetry that points at synchronization cost (CAS/RMW atomics are pricier under ARM's weak memory model than x86 TSO). A perf/strace profile plus a full code trace of the inline dispatch path found four per-op costs Redis does not pay, all off the sharding model (Moon already does 2 syscalls/op vs Redis 3): 1. Replica gate: the inline path took `std::sync::RwLock::try_read()` on repl_state EVERY op. The lock-free `is_replica_mirror` added for exactly this cost (S3.5a, 2026-04-27, after ARM perf-annotate showed the try_read CAS dominating a sibling fn) was never propagated here. Now a single AtomicBool Acquire load — and unlike try_read, accurate while the lock is held. 2. Inline SET refcount churn: `value.clone()` into Entry::new_string, `key.clone()` into db.set, and `CompactKey::from(key.clone())` in Database::set were all dead clones (last use). Move semantics + borrow drop 3 atomic RMW pairs per SET. 3. Eviction gate: every inline SET paid a parking_lot RwLock read pair on runtime_config just to learn eviction has nothing to do. New lock-free pre-gate (`inline_write_can_skip_eviction`) proves the no-op case from two Relaxed atomic hints published at startup and on CONFIG SET maxmemory; unpublished sentinel fails safe into the existing locked path, so OOM/noeviction semantics are unchanged. 4. `ClientLiveState::touch()` called `Instant::now()`-equivalent per batch, violating the timestamp-caching invariant. It now takes the caller's shard-cached epoch ms (cold CLIENT LIST/INFO paths pass `current_time_ms()`), with saturating math for cached-clock skew. Tests: pure-core gate test (deterministic — the process-global hints race CONFIG SET tests in the parallel suite, learned the hard way), touch caller-clock + saturation test, existing inline byte-parity suite. Part of the kv-s1-outperform goal (beat Redis at p=1 on BOTH arches); GCloud cross-arch re-measure to follow. author: Tin Dang
The env contract documents MOON_NO_URING=1 as "force-disable io_uring everywhere (monoio runtime + tokio bridge)", but the monoio factory built FusionDriver unconditionally — the kill-switch only gated the tokio bridge and uring_active(); monoio's own driver choice ignored it. Discovered during the c4a p=1 driver A/B: a run launched with MOON_NO_URING=1 still showed anon_inode:[io_uring] fds in /proc. block_on_local now builds the epoll/kqueue LegacyDriver explicitly when MOON_NO_URING is set (FusionDriver offers no runtime introspection, so forcing the type is the only reliable switch). Verified empirically on Linux: with the fix, MOON_NO_URING=1 serves traffic with zero io_uring fds; unset keeps io_uring active. This unblocks a same-instance io_uring-vs-epoll A/B on GCloud ARM to attribute the residual p=1 latency gap vs Redis. author: Tin Dang
…o runtime Same-instance driver A/B on GCE c4a (ARM Axion, dedicated, pinned, best-of-5, redis control): monoio's epoll LegacyDriver beats io_uring by 3.2-3.9% at p=1 AND matches-or-beats it at every pipeline depth (P1/8/32/128) for KV workloads — epoll GET P128 keeps the 1.20x win vs Redis. Switching driver closes ~40% of the ARM p=1 gap vs Redis (0.92 -> 0.95). Other platforms (e.g. OrbStack aarch64) favor io_uring, so this ships as an opt-in flag, not a default flip — bench per platform (v2-1 lesson: knees do not transfer across targets). --io-driver epoll sets a process-wide AtomicBool read by MonoioRuntimeFactory::block_on_local (safe alternative to mutating MOON_NO_URING via unsafe set_var in edition 2024); values are clap-validated. MOON_NO_URING=1 remains the env kill-switch and now reaches the monoio driver too (previous commit). Verified on Linux: --io-driver epoll serves traffic with zero io_uring fds and logs the forced choice; default keeps FusionDriver (io_uring). author: Tin Dang
…t MOON_NO_URING doc gcloud-kv-p1-baseline.sh gains: - --measure-driver: same-instance A/B of moon(io_uring) vs moon(epoll, via MOON_NO_URING=1) vs redis control. Same-instance is required — Moon's absolute p=1 RPS varies up to 2.5% between c4a instances, the same order as the driver effect, so cross-instance driver deltas are unreadable. - MOON_START_ENV passthrough (local -> gcloud ssh -> start_moon) for server-process env in diagnostics. - io_uring self-report after server start: monoio's FusionDriver picks a driver silently, so /proc/<pid>/fd is asserted and logged per run. CLAUDE.md: MOON_NO_URING description was aspirational — before f58ed02 it never reached the monoio driver. Documented the fix, the /proc verification method, the --io-driver CLI equivalent, and the measured GCE result (epoll beats io_uring 2-4% at all pipeline depths for KV on c4a Axion; 1-1.5% on c3 x86; both same-instance A/Bs). author: Tin Dang
…EFER_TASKRUN GCE p=1 attribution (same-instance, perf tracepoints + strace): the stock io_uring ring LOSES to epoll on both c4a ARM (-3.2..3.9%) and c3 x86 (-1.0..1.5%) because io_uring_enter is expensive there, while the epoll path pays 4 syscalls/op (a speculative recvfrom that EAGAINs every op at p=1 — readiness stays set after a successful read) vs Redis's 3. io_uring does 2 syscalls/op — it just needs cheaper enters. COOP_TASKRUN + SINGLE_ISSUER + DEFER_TASKRUN are the canonical thread-per-core flags: completions are processed only at enter time on the ring's owning thread, no IPIs/task_work preemption. Moon's model satisfies the constraints: each shard creates and enters its ring on its own thread only; cross-thread signalling is fd-based (flume/ eventfd/UnixStream), never a ring op from a foreign thread. Fallback ladder: kernels <6.1 reject the flags -> warn + stock FusionDriver; MOON_URING_PLAIN=1 skips tuning; MOON_NO_URING/ --io-driver epoll still force the legacy poller. Needs io-uring 0.6 (monoio 0.2.4's uring_builder type) alongside the existing 0.7 used by the tokio bridge -> renamed dep io-uring-06. Verified on Linux 6.17: tuned ring initializes (uring fds present, no fallback warning), serves traffic correctly. author: Tin Dang
…iments) ARM p=1 attribution ledger so far: 4 atomic fixes ~0, epoll best-driver 0.93-0.95, tuned uring flags (140ccfb) +0 on c4a. The diag decomposition says the residual gap vs Redis is kernel-entry + scheduler-wake cost (el0_svc 1.39us/op vs 0.46, both engines park+wake every op). Syscall counting alone cannot close it; removing the SLEEP can. Two env-gated experiments, both default-off (byte-identical behavior when unset): 1. MOON_URING_SQPOLL=<idle_ms> [+ MOON_URING_SQPOLL_CPU=<n>]: kernel- side submission polling. RESULT: functionally broken under monoio -- the driver is never CQE-woken and reaps only on its 1ms timer tick (p50 collapsed to 0.999ms, 1.4k ops/s). Kept as a documented diagnostic gate; do not use without a driver-level fix. 2. MOON_URING_SPIN_US=<budget>: poll-mode reaping inside the io_uring driver park -- submit pending SQEs immediately, then busy-poll the completion queue (shared-memory head/tail, zero syscalls) for the budget before falling back to the stock blocking wait. On hit the shard thread never sleeps, deleting the scheduler wakeup from the per-op path (the ScyllaDB-style thread-per-core poll design). Requires patching monoio's driver, so monoio 0.2.4 is vendored at vendor/monoio via [patch.crates-io]; the diff vs upstream is confined to src/driver/uring/mod.rs (grep "moon patch", no new unsafe in the added lines). Correctness: cross_shard_consistency_red 7/7 + shardslice_live 6/6 green under MOON_URING_SPIN_US=40. OrbStack VM shows spin SLOWER (virtualized syscalls/wakes are nearly free, so spin is pure overhead there) -- decisive numbers must come from pinned GCE cores where the wake path costs real microseconds. author: Tin Dang
--measure-driver hard-reset MOON_START_ENV per cell (uring cell wiped it to ""), silently discarding caller-provided experiment env such as MOON_URING_SPIN_US=40 -- the A/B would re-measure the plain ring while claiming to test the experiment. Capture the incoming value once and compose: uring cell = caller env as-is, epoll cell = caller env + MOON_NO_URING=1. author: Tin Dang
CQ spin on io_uring is structurally dead for network ops: CQE posting requires the ring-owner task to run task_work. DEFER_TASKRUN posts only inside enter(GETEVENTS) (measured: spin burns its full budget, 49us/op on c4a, 3x regression); a plain ring posts via TWA_SIGNAL kernel kicks (~9us/op measured); SQPOLL does not run poll task_work on the sqpoll thread on 6.17 (CQEs never post while spinning, 498 ops/s). The io_uring spin gate now forces a plain ring so the earlier experiment is at least well-defined, but the winning primitive is epoll: epoll readiness is published by softirq with NO task participation, so a zero-timeout epoll_wait spin observes it ~0.3us after arrival with no sleep, no scheduler wake, no signal kick. MOON_EPOLL_SPIN_US=<budget> patches the vendored monoio LegacyDriver park: busy-loop poll(timeout=0) for the budget, fall back to the stock blocking poll on miss. Foreign wakes surface during the spin (the waker eventfd is registered in the same mio poll). Default-off; zero behavior change when unset. Pinned-core VM A/B (taskset 0 vs 2, c1 GET p=1): stock epoll 34.3k/29.8k (p50 31us) vs spin40 47.4k (p50 23us) = +40-60%, p50 -8us. Unpinned VM numbers are misleading for spin designs (a 100%-busy thread displaces the client); pinned disjoint cores are the valid condition and match the GCE instrument. GCE cross-arch verdict pending. author: Tin Dang
Promote the epoll readiness spin-poll (MOON_EPOLL_SPIN_US, vendored
monoio LegacyDriver patch) to a first-class CLI flag:
--io-busy-poll-us <N> busy-poll the shard event loop N microseconds
before sleeping (0 = off, the default)
The flag forces the legacy (epoll/kqueue) driver -- busy-poll parks
cannot exist on io_uring since CQEs are not userspace-observable
without task participation -- and hands the budget to the vendored
driver through a programmatic setter (monoio::set_legacy_spin_budget_us,
AtomicU64 override with env fallback; no set_var/unsafe needed).
No-op warning under runtime-tokio.
GCE same-instance A/B (c1 GET/SET p=1, pinned disjoint cores, best-of-5)
with the 40us budget vs Redis:
ARM c4a Axion SET 1.193 / GET 1.206 (was 0.95 losing)
x86 c3 Intel SET 1.657 / GET 1.657 (was 1.06)
Cost model: up to N us CPU per idle park (~4%/core at 40us against 1ms
timer parks); unpinned shared-core hosts can see spin as a regression --
documented in CLAUDE.md. Cross-arch replication (n=3/arch) in flight.
Verified: flag parse test, VM end-to-end (io_uring fds absent, spin log
line, serves), cross_shard_consistency_red 7/7 + shardslice_live 6/6
under spin.
author: Tin Dang
…st literals
1) gcloud-kv-p1-baseline.sh gains --measure-shards: shards x busy-poll
sweep ({1,4} x {0,40us} vs single Redis control) with the p=1 rigor
(pinned cores, steal gate, fresh-server best-of-N, strict keyspace so
multi-shard actually scatters). Cells per config: c1/P1 single-conn
latency (captures the cross-shard-hop story at shards=4 -- the target
shard normally sleeps; busy-poll removes that wake too), c8/P1,
c8/P16, c8/P64 with a 3-thread client on disjoint cores. Needs an
8-vCPU instance (shards pin to 0..3, client to 5-7). start_moon
generalized via MOON_SHARDS/SERVER_CORES/MOON_EXTRA_FLAGS (defaults
preserve the shards=1 path); bench_pair gains optional --threads.
2) Fix tokio-feature-gated tests that construct exhaustive ServerConfig
literals (mq_integration, workspace_integration x2, txn_kv_wiring):
the io_driver/io_busy_poll_us fields broke compilation ONLY under
--features runtime-tokio because cargo clippy without --all-targets
never compiles test targets -- CI-parity gap, caught by the full
tokio suite run. All-targets check now clean on both feature sets.
author: Tin Dang
…2.10) New 2.10: same-instance A/B tables (Moon busy-poll vs tuned-uring vs Redis control) on GCE dedicated 4-vCPU, pinned disjoint cores, best-of-5, n=3 instances per arch -- ARM c4a 1.193-1.212x, x86 c3 1.646-1.663x. Supersedes the "loses p=1" rows in 2.7-2.9 when busy-poll is enabled; documents mechanism (wake elimination), multi-client probe, idle cost, and the pinned-cores measurement caveat. Executive summary gains the two p=1 rows; header Last-Updated refreshed. author: Tin Dang
Cross-arch --measure-shards results at 74de849: the 2.10 busy-poll p=1 win replicates on the 8-vCPU shape (5th ARM + 4th x86 instance); busy-poll recovers +22-42% of the cross-shard hop (removes the target shard's wake) but shards=4 still loses every non-pipelined cell to the SPSC dispatch cost; Redis-server-bound SET P16 shows Moon >=2.0x. Deep-pipeline cells on the co-located pinned topology are flagged as client-saturated ceilings, not measurements. Guidance sharpened: --shards 1 --io-busy-poll-us 40 is the p=1 configuration. author: Tin Dang
…stance (multi-shard diag) Lets --measure-diag profile a shards=N busy-poll server: the perf attribution of the residual cross-shard hop needs the diag mode to start moon with --shards 4 --io-busy-poll-us 40 on cores 0-3. author: Tin Dang
Cross-shard dispatch pays a full cross-thread wake per message: flume try_send -> foreign-waker relay -> eventfd_write syscall (s4 diag on GCE c4a: eventfd_write 2.01% self + 3.4% CAS/SWP atomics on the conn shard). When the target shard's driver is already busy-polling (--io-busy-poll-us), that wake is pure waste — the target is awake and polling anyway. New skip-notify handshake, active only with busy-poll AND shards > 1: - channel::Notify gains a skip_wake advertisement flag. Senders (notify_one) elide the entire wake when the target advertises it is spinning; a process-global gate keeps even the check off notify_one for every other deployment shape. notify_local() bypasses the gate for same-thread delivery. - Vendored monoio legacy driver (grep "moon patch") gains per-thread spin-park hooks: advertise(true) at spin entry, advertise(false) at every exit, and a probe each spin iteration that checks the shard's SPSC ringbufs and re-arms the race2 Notify from the local thread (run-queue push, no syscall) when items are pending. - Lost-wake safety is a Dekker handshake: the sender fences SeqCst after its ringbuf push before reading the flag; the driver retracts the flag (SeqCst store + fence) and runs one final probe on every spin exit. Either the final probe sees the push or the sender sees the cleared flag — store-buffering reordering is forbidden by the paired fences. Unit-tested (gate off / spinning / local bypass / retracted) in channel.rs. - Observability: spsc_notify_skipped counter (INFO Stats), plus a per-shard registration log line. tokio runtime is untouched: hooks are monoio-only, the gate stays off, and notify_one behavior is bit-identical. No new unsafe. Smoke-verified on a live 4-shard busy-poll server (skipped counter engages, cross-shard MGET/MSET correct); full release suite green on Linux under MOON_NO_URING=1 MOON_EPOLL_SPIN_US=40 (the skip-notify regime forced for every multi-shard integration test). Arithmetic expectation, to be validated on GCE same-instance A/B: recovers ~1-1.5us of the ~5.3us s4-vs-s1 c1P1 residual; parity at shards>1 c1 p=1 is NOT expected (irreducible reply hop remains). author: Tin Dang
Skip-notify's expected delta (~1-1.5us of a ~28us s4 c1P1 op) is a few percent — inside the ~2.5% instance-to-instance variance — so base and new commits MUST share one instance. New mode builds both refs (AB_BASE_REF vs MOON_REF, per-ref cached binaries), runs shards x spin cells (spin=0 doubles as a no-regression control since skip-notify is gated off there) with the same pinned/steal-gated/fresh-server-best-of-N rigor, emits base|new|redis per cell, and ends with an engagement proof (INFO spsc_notify_skipped after a sustained c1P1 burst). AB_* env forwarded to the instance. author: Tin Dang
Multi-shard investigation instrumentation (diagnostic, defaults unchanged): - MOON_XSHARD_SPIN_BUDGET / MOON_XSHARD_SPIN_GATE env overrides for the C2 reply-side spin constants (read once via OnceLock; 0 budget fully disables the spin). Enables same-instance A/Bs of the convoy hypothesis — at s4 c8P1 two conns share each shard thread and the synchronous reply spin (gate=2 admits both) may block SPSC drain of other shards' requests, a circular cross-shard stall that would explain the 0.5x c8P1 collapse. - --measure-ab gains AB_BASE_ENV / AB_NEW_ENV (per-side MOON_START_ENV so one binary can be A/B'd across env knobs) and AB_COMBOS (configurable clients|pipe cells, e.g. "1|1 8|1 64|1"). author: Tin Dang
…nvoy fix) The C2 reply-side spin is synchronous on the shard thread. At shards=4 c8P1 every shard hosts 2 connections and the existing gates admit both: while conn A spins awaiting shard B's reply, its thread can neither run the sibling conn nor drain incoming SPSC requests from other shards — a circular cross-shard convoy. Same-instance GCE c4a A/B (env knob MOON_XSHARD_SPIN_BUDGET=0 vs default, e8ef3d7): s4 c8P1 spin-off 199.8k/200.0k ops/s vs spin-on 72.7k — 2.75x, lifting Moon from 0.45x to 1.25x Redis. The same A/B confirms the spin must stay for c1 (spin-off loses 13.5% there). Fix: spin only when the requesting connection is ALONE on its shard thread. New thread-local SHARD_CONN_COUNT maintained by a ShardConnGuard RAII at both connection-handler entries (monoio + tokio, fresh + migrated); xshard_may_spin() now also requires count <= XSHARD_SPIN_MAX_CONNS (default 1, env override MOON_XSHARD_SPIN_MAX_CONNS for diagnostic A/Bs). The existing XSHARD_INFLIGHT gate cannot see a sibling whose readable event is still parked in the driver — it only counts conns already inside a reply-wait; the conn count closes that executor-level blind spot. Known trade-off (documented): a solo hot connection sharing its shard with idle extra connections (e.g. a monitoring conn) now parks instead of spinning — the safe direction; busy-poll still covers the target-side wake. Regression test solo_conn_gate_blocks_spin_with_sibling_connection in tests/xshard_fastpath_api.rs pins the semantics. author: Tin Dang
Both cross-shard write arms in drain_spsc_shared called aof::serialize_command (full command alloc + copy) BEFORE wal_append_and_fanout, whose S3.5b bypass then discarded the result whenever WAL v2/v3, replicas, and the AOF pool are all off — pure waste on every cross-shard write with persistence disabled. Flagged by the multi-shard serialization trace; suspect for part of the s4 SET-vs-GET gap (200k vs 266k ops/s at c8P1 spin-off, GCE c4a). Extract the bypass criterion as wal_fanout_has_work() (inline, derivable from inputs, no shared state), use it inside wal_append_and_fanout (behavior unchanged) and as a pre-gate before serialize_command at both write arms. The blocking-wake path (LPUSH/RPUSH/LMOVE/ZADD/XADD) is outside the gate and unaffected. Cold SWAPDB site left as is. Durability/persistence/spsc suites green (coordinator_local_leg 3, persistence 440, spsc 13). author: Tin Dang
The c64 cells hit a ~266k ops/s ceiling with the hardcoded 3 client threads (unpipelined syscall-pair bound) — five identical 266,667 readings across engines/cells in the convoy A/B are the instrument saturating, not the server. Make the thread count configurable so the goal measurement can drive from 4 client cores (SHARD_CLIENT_CORES=4-7 AB_BENCH_THREADS=4). author: Tin Dang
The monoio Phase 2b cross-shard path allocated a fresh flume oneshot per batch and awaited it through a chunked race2 park (100ms chunks, 30s cap). The tokio handler had already moved to the loom-tested zero-allocation ResponseSlotPool; the slotted ShardMessage variants and the target-side fill arms were runtime-shared and idle on monoio. This unifies monoio on the same design: - Phase 2b now sends ShardMessage::PipelineBatchSlotted carrying a ResponseSlotPtr into the connection's pre-allocated pool (one slot per target shard) — no per-op oneshot alloc, no flume channel internals, no lazily boxed recv future on the reply path. - Reply await is tokio-parity: C2 idle-gated spin over slot.try_take(), then a plain infinite future_for(target).await. The chunked 30s timeout structure is REMOVED — required by the ResponseSlotPtr lifetime contract (the pool lives on the connection task's stack; abandoning the await while a slotted message is in flight would turn the target shard's eventual slot.fill() into a use-after-free). A backpressure reject never pushes the message, so the slot stays EMPTY and reuse is safe. Contract documented at the await site. - The vestigial reply_futures Vec (pre-allocated, cleared, never populated) is now the carrier for (meta, target), deleting the per-batch oneshot_futures Vec allocation as well. - spsc_handler: the PipelineBatch + PipelineBatchSlotted arms get the wal_fanout_has_work serialize-skip gate (b3fa7be gated only the MultiExecute pair), so persistence-off cross-shard writes no longer pay the aof::serialize_command alloc on the arm monoio now uses. - response_slot: try_take() is live under both runtimes; dead_code cfg_attr lifted. CROSS_SHARD_RESPONSE_{CHUNK,TIMEOUT}_MS deleted. No new unsafe blocks: the single deref site remains the existing SAFETY-commented arm in spsc_handler; ResponseSlotPtr's Send impl and the slot state machine are unchanged (loom: tests/loom_response_slot.rs; cross-thread wake proof: tests/spsc_wake_floor_red.rs::swf0). Coverage: existing monoio suites now exercise the slotted arm end-to-end (cross-shard consistency, C4-FOLD double-apply, H1 fsync barrier); full VM suite green on both runtimes. Part of the multi-shard ≥1.5×-Redis line (tmp/MULTISHARD-REDESIGN.md): c64P1 already ≥1.65× both arches after the L1 convoy fix; c8P1 is latency-bound and needs ~4µs off the serial reply path — this is that lever. GCE same-instance A/B to follow. author: Tin Dang
a5ec041 added the AB_BENCH_THREADS knob at the measure_ab/measure_shards usage sites but not to gcloud_run_one's env forwarding, so an OUTER `--one` invocation silently fell back to 3 client threads (the ~266k c64P1 instrument ceiling the knob exists to lift). Forward it like the other AB_* vars so the de-capped goal matrix is reproducible from a single outer command. author: Tin Dang
Records the C2 reply-side spin overrides (BUDGET/GATE/MAX_CONNS) in the CLAUDE.md environment-variable table with their defaults, the budget=0 A/B use that proved the c8P1 convoy, and a warning that raising the solo-conn ceiling re-creates the multi-conn collapse the L1 fix removed. author: Tin Dang
Defaults audit outcome: the shipped defaults already encode the measured evidence for the most common deployment (single app, moderate connections, durability on) — `--shards 1` (best per-op latency; a cross-shard hop costs ~10µs), `--appendonly yes` + `everysec` (measured free at the default shard count on GCE), busy-poll off (regresses on shared hosts), admin port off. No code-default changes. What was wrong or missing was the documentation: - docs/configuration.md claimed `--shards` defaults to `0 (auto)` and `--appendonly` to `no`; actual defaults are `1` and `yes`. docs/guides/configuration.md had the same appendonly error. - `--io-driver`, `--io-busy-poll-us`, `--initial-keyspace-hint`, `--memory-arenas-cap`, `--aof-fsync-timeout-ms` were absent from the flags reference despite busy-poll being the single largest latency lever we ship. - No workload-oriented tuning documentation existed. New docs/guides/tuning.md (in the site nav under Operations): quick recipes plus measured guidance per workload — shard-count selection (1 vs 4 vs auto, hash-tag co-location), busy-poll trade-offs and the dedicated-core requirement, persistence cost table (everysec free / always −9% P1 with deep-pipeline tail caveat / multi-shard AOF+WAL disk volume), memory (elastic budget, keyspace hint, container arena caps), platform notes (io_uring vs epoll per platform, macOS dev-only, pinning), client-side checklist (ulimit, pool sizing vs shards, pipelining), observability cost, and vector quantization-by-dimension. All numbers from the 2026-07 GCE dedicated-vCPU runs (ARM c4a + x86 c3). Also refreshed the `--shards` CLI help (was pipeline-only advice; multi-shard now also wins at 8+ connections post convoy fix). author: Tin Dang
…L3b) Records the validated multi-shard result on GCE dedicated-vCPU instances: after the reply-spin convoy fix (solo-conn gate, 795c4f0) and the monoio ResponseSlotPool unification (fd13f03), s4 P1 with busy-poll 40 measures vs Redis: c8 GET/SET: ARM 1.57/1.71x — x86 1.9/2.0x (>=1.6x vs Redis's best client config) c64 GET/SET: 2.50x both arches (>=1.86x best-vs-best) c1: 0.91-0.96 = the documented structural hop ceiling (s1 latency) Supersedes the s4 c>=8 collapse rows in §6.5 (0.44-0.64x), which were the convoy bug, and updates the guidance: --shards 4 wins from 8 concurrent connections up even without pipelining. Instrument notes recorded: redis-benchmark --threads 4 lowers Redis's x86 c8/c64 readings 15-20% vs --threads 3 (Moon thread-insensitive, ARM unaffected; dedicated Redis-only control run), and unpipelined c64 readings quantize to duration attractors. Raw files under tmp/. author: Tin Dang
The cross-shard reply fast-path (ResponseSlotPool, L3b + the pre-existing
tokio path) sent a raw `*const ResponseSlot` into a pool that lives on the
connection task's stack. A panic unwinding through the reply dispatch loop
(Phase 2b) or the drain loop drops that stack pool in-place during the
unwind, freeing the slot heap while a target shard still holds the raw
pointer in its SPSC queue — the target's later `slot.fill()` is then a
use-after-free / heap corruption. Confirmed-mechanism in review (async-fn
locals are dropped during panic-unwind; the cross-thread fill race is real);
no client trigger demonstrated, but there is no panic boundary and panic
surfaces exist in the window (Vec indexing, RefCell borrow in the dispatch
backpressure closure). `panic = "abort"` is not an option (mlua + existing
catch_unwind recovery rely on unwinding).
Fix: `ResponseSlotPtr` now carries an `Arc<ResponseSlot>` instead of a raw
pointer, and `ResponseSlotFuture` owns an `Arc` clone instead of borrowing
the pool. The refcount keeps each slot alive until BOTH the connection's
pool AND the in-flight message drop, so abandoning the reply (drop,
panic-unwind, or a future shutdown break) can never dangle a concurrent
`fill()`. This structurally closes the window on both the dispatch and
drain sides, on both runtimes — retroactively fixing the same latent hazard
on tokio (shipped since v0.4.0), where monoio previously used a panic-safe
flume oneshot.
Cost: one refcount bump per batch (<= num_shards), pool allocated once per
connection — no per-op allocation is reintroduced (L3b removed the per-op
flume-oneshot alloc, ~80-120ns; this adds single-digit-ns atomics). Net
`unsafe` REDUCTION: removes the `unsafe impl Send for ResponseSlotPtr` and
four `unsafe { &*response_slot.0 }` derefs in the SPSC fill arms.
TDD: `test_slot_arc_outlives_pool_drop_and_is_fillable` proves via
`Arc::strong_count` that a sent handle survives the pool being dropped and
remains fillable (the raw-pointer design could not express this and would
dangle). Full suite green both runtimes.
Follow-up (now safe to add): a shutdown-aware bound on the reply await to
close the partial-shutdown liveness hang (target thread torn down while a
producer is parked).
author: Tin Dang
Reconcile CHANGELOG [Unreleased] to cover BOTH milestone bodies since v0.4.1 (previously only v3-4 was documented): - v3-4 KV write correctness & data-integrity parity (existing). - p=1 & multi-shard throughput: --io-busy-poll-us / --io-driver, the convoy fix + L3b, GET no-copy, lock/atomic strips, corrected default docs + tuning guide. - Dependencies: vendored monoio 0.2.4 supply-chain disclosure (verified vs pristine upstream: 4-file bounded patch, zero new unsafe, deps + behavior byte-identical when unset). - Fixed: the Arc cross-shard reply UAF (memory safety). Also: correct the Cargo.toml [patch.crates-io] comment to reflect the real 4-file moon-patch scope (was "1 file") + record the upstream sha, and fix an int_plus_one clippy nit in a metrics_setup test. author: Tin Dang
…rces Add a KEYSPACE env knob so bench-compare.sh / bench-resources.sh can spread ops across a real keyspace (-r) for large-scale runs instead of redis-benchmark's default single hot key. Empty by default (unchanged behavior). Also fold the v3-4 milestone completion doc. author: Tin Dang
|
Important Review skippedToo many files! This PR contains 225 files, which is 75 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (225)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoRelease 0.5.0: KV correctness, p=1 + multi-shard throughput, Arc response-slot UAF fix
AI Description
Diagram
High-Level Assessment
Files changed (54)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
37 rules 1. Monoio cross-thread AtomicWaker wake
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1717
to
+1720
| // L3b: await each response slot directly (tokio parity — | ||
| // handler_sharded/mod.rs). Cross-thread wakes reach this task via | ||
| // the slot's AtomicWaker + monoio's `sync`-feature waker channel | ||
| // (proven by tests/spsc_wake_floor_red.rs::swf0 on both drivers). |
There was a problem hiding this comment.
1. Monoio cross-thread atomicwaker wake 📘 Rule violation ≡ Correctness
The monoio handler now awaits cross-shard replies via ResponseSlotPool and explicitly relies on cross-thread waking via the slot’s AtomicWaker. This violates the requirement to relay pending wakers on the runtime thread and use flume::bounded(1) for cross-thread signalling, and may break on monoio if waking from non-runtime threads is unsupported.
Agent Prompt
## Issue description
`ResponseSlot` wakes a task via `AtomicWaker` from a different shard thread, and the monoio handler now depends on this cross-thread wake behavior.
## Issue Context
Compliance requires that monoio wakers are not woken directly from non-runtime threads; cross-thread notifications must go through `flume::bounded(1)` and a runtime-thread waker relay.
## Fix Focus Areas
- src/server/conn/handler_monoio/mod.rs[1635-1765]
- src/server/response_slot.rs[58-125]
- src/runtime/channel.rs[169-244]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+926
to
+927
| #[allow(clippy::too_many_arguments)] | ||
| async fn coordinate_msetnx( |
There was a problem hiding this comment.
2. coordinate_msetnx allow lacks justification 📘 Rule violation ✧ Quality
A new #[allow(clippy::too_many_arguments)] was added without a justification comment. This violates the requirement that new lint suppressions be narrowly scoped and justified rather than used to silence warnings.
Agent Prompt
## Issue description
A new clippy suppression was introduced without justification.
## Issue Context
The compliance rule requires new `#[allow(clippy::...)]` uses to be narrowly scoped and justified in-code (or avoided by refactoring).
## Fix Focus Areas
- src/shard/coordinator.rs[918-939]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
+29
| fn moon_binary() -> std::path::PathBuf { | ||
| std::path::PathBuf::from(env!("CARGO_BIN_EXE_moon")) | ||
| } |
There was a problem hiding this comment.
3. moon_binary() ignores moon_bin 📘 Rule violation ▣ Testability
New integration tests spawn the server using env!("CARGO_BIN_EXE_moon") instead of requiring
MOON_BIN to be set. This violates the requirement that integration tests explicitly set and use
MOON_BIN rather than relying on default/fallback binary paths.
Agent Prompt
## Issue description
Integration tests that spawn the server do not use `MOON_BIN` and instead rely on `CARGO_BIN_EXE_moon`.
## Issue Context
Compliance requires tests to explicitly use `MOON_BIN` (fail-fast if missing) to avoid silently running an unintended binary.
## Fix Focus Areas
- tests/msetnx_cross_shard_reject.rs[27-52]
- tests/coordinator_local_leg_durability.rs[53-85]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
208
to
+219
| pub fn notify_one(&self) { | ||
| if notify_skip_wake_enabled() { | ||
| // Dekker handshake with set_skip_wake(false): the SeqCst fence | ||
| // orders the caller's ringbuf push before this flag load, so | ||
| // either the spinning target's final probe sees the push or this | ||
| // load sees the cleared flag — a wake is never lost (classic | ||
| // store-buffering prevention; both sides fence). | ||
| std::sync::atomic::fence(std::sync::atomic::Ordering::SeqCst); | ||
| if self.skip_wake.load(std::sync::atomic::Ordering::Relaxed) { | ||
| crate::admin::metrics_setup::bump_spsc_notify_skipped(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
4. Skip-wake ordering unsound 🐞 Bug ☼ Reliability
Notify::notify_one() uses a SeqCst fence but then reads skip_wake with Ordering::Relaxed, which does not participate in the intended synchronization with set_skip_wake(false). Under --io-busy-poll-us this can (depending on architecture/interleaving) incorrectly elide the only wake and delay or stall SPSC draining until another unrelated wake occurs.
Agent Prompt
## Issue description
`Notify::notify_one()` uses a `Relaxed` load of `skip_wake` while relying on a Dekker-style handshake described in comments. A `Relaxed` load does not provide the intended synchronization with `set_skip_wake(false)` (which uses `SeqCst` stores/fences), so the sender-side fast path may observe a stale `true` and skip the wake even after the target retracts `skip_wake`, risking a missed wake.
## Issue Context
This logic is exercised only when the process-global skip-wake fast path is enabled and shard busy-poll hooks are registered (monoio legacy driver busy-poll). In that mode, cross-thread notification is intentionally elided, so correctness of the handshake is critical to avoid lost wakeups.
## Fix Focus Areas
- src/runtime/channel.rs[208-240]
- src/shard/event_loop.rs[783-803]
### Concrete fix direction
- Change `self.skip_wake.load(Ordering::Relaxed)` to `Ordering::SeqCst` (simplest) or `Ordering::Acquire`.
- Re-evaluate whether the extra `fence(SeqCst)` is still needed once the load is strengthened; if keeping the fence, ensure the design is correct under Rust/C++ memory model (avoid relying on fences with relaxed operations for synchronization).
- Add a stress test (loom or a targeted concurrency test if available in-repo) that toggles `set_skip_wake(false)` concurrently with `notify_one()` and asserts the receiver is eventually notified.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+2429
to
+2436
| pub(crate) fn wal_fanout_has_work( | ||
| wal_writer: &Option<WalWriter>, | ||
| wal_v3_writer: &Option<WalWriterV3>, | ||
| replica_txs: &[(u64, channel::MpscSender<bytes::Bytes>)], | ||
| aof_pool: Option<&std::sync::Arc<crate::persistence::aof::AofWriterPool>>, | ||
| ) -> bool { | ||
| wal_writer.is_some() || wal_v3_writer.is_some() || !replica_txs.is_empty() || aof_pool.is_some() | ||
| } |
There was a problem hiding this comment.
6. Backlog not updated offline 🐞 Bug ≡ Correctness
wal_fanout_has_work() ignores whether a replication backlog is allocated, so wal_append_and_fanout() returns early whenever WAL/AOF are off and replica_txs is empty. Because UnregisterReplica removes replica_txs entries without clearing the backlog, writes during replica-disconnected windows stop appending to the backlog and can force later PSYNC attempts to fall back to full resync (missing offsets).
Agent Prompt
## Issue description
`wal_fanout_has_work()` is used to skip WAL/backlog/offset work when it would be a no-op, but the predicate does not account for an *already-allocated* replication backlog (or offset handle). After the last replica disconnects, `replica_txs` becomes empty while the backlog remains allocated, and the early return stops backlog appends and offset advancement, breaking partial resync coverage.
## Issue Context
- `ShardMessage::RegisterReplica` allocates the backlog if needed.
- `ShardMessage::UnregisterReplica` only removes the sender from `replica_txs`; it does not clear the backlog.
- PSYNC partial resync requires the requested offset to be present in all per-shard backlogs.
## Fix Focus Areas
- src/shard/spsc_handler.rs[1789-1803]
- src/shard/spsc_handler.rs[2429-2453]
- src/replication/handshake.rs[47-75]
### Concrete fix direction
Update the predicate so backlog/offset maintenance continues when replication is active even if no replicas are currently connected. Options:
1) Extend `wal_fanout_has_work(...)` to accept an additional boolean/handle:
- include `repl_state.is_some()` / `repl_offsets.is_some()` as work, OR
- include a cheap mirror flag like `replication_backlog_allocated: AtomicBool` in `ReplicationState` set by `ensure_backlogs_allocated` / `RegisterReplica`.
2) If you must key off the backlog itself, add a non-blocking/cheap signal rather than taking the backlog mutex on every write.
Add a regression test:
- Register a replica (allocate backlog),
- Unregister it (replica_txs empty but backlog still allocated),
- Perform writes with WAL/AOF off,
- Assert backlog grows / contains offsets needed for `evaluate_psync` to allow partial resync.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
TinDang97
pushed a commit
that referenced
this pull request
Jul 4, 2026
Version 0.4.1 -> 0.5.0 (Cargo.toml + Cargo.lock lockstep). Bundles everything since v0.4.1 (merged in #207): - v3-4 KV write correctness & data-integrity parity (supersedes #206). - p=1 & multi-shard throughput: --io-busy-poll-us poll-mode park (beats Redis at p=1 on both GCE arches), C2 reply-spin convoy fix (multi-shard wins from c8 up), L3b ResponseSlotPool, corrected default docs + tuning guide. - Vendored monoio 0.2.4 fork (bounded moon-patch, verified vs upstream). - Memory-safety: Arc-owned ResponseSlotPtr closes a panic-unwind cross-shard reply UAF (found in review; also fixes the pre-existing tokio path; net -4 unsafe blocks). Reviews: security supply-chain CLEAN; rust drop-safety finding FIXED in-PR. Full CI green both runtimes; GCloud A/B confirmed the multi-shard win held. shardslice cross-shard-read waiver rides (validated, retirable). - CHANGELOG.md: reconcile [Unreleased] -> [0.5.0] - RELEASES.md: ledger row (waiver disclosed) Engine records the cut; tag + publish follow. author: Tin Dang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles everything since v0.4.1 into one release. Two milestone bodies plus a reviewed memory-safety fix:
GETDEL/GETSET/SET…GET), no integer-overflow panics/wraps, past-expire deletes, atomicMSETNX, and cross-shard coordinator local-leg durability.--io-busy-poll-us), and multi-shard now wins from 8 concurrent connections up (s4 c8 1.57–2.0×, c64 2.50×) after fixing the C2 reply-spin convoy (solo-conn gate) and unifying the monoio reply path on the zero-allocResponseSlotPool(L3b).Review outcomes (gate before release)
Two focused reviews + full CI:
monoio(see below) was diffed against pristine upstream 0.2.4: delta confined to 4 files, zero new unsafe, dependency list byte-identical (no smuggled deps), behavior byte-identical when theMOON_*spin knobs are unset. MSETNX four-point wiring ✅, coordinator local-leg durability ✅.*const ResponseSlotinto a pool on the connection task's stack; a panic unwinding the reply dispatch/drain would drop that pool while a target shard still held the pointer, making its laterslot.fill()a UAF. (Pre-existing on the tokio path since v0.4.0; L3b extended it to the default monoio runtime.) Fix:ResponseSlotPtrnow carriesArc<ResponseSlot>, so the slot outlives whichever of {pool, in-flight message} drops last — structurally closing both windows on both runtimes and removing 4 unsafe blocks (net unsafe reduction). NewArc::strong_countregression test.panic=abortwas ruled out (mlua + existingcatch_unwindrecovery rely on unwinding).fmt+clippy(default /--features graph/ tokio) +cargo teston both runtimes (232 test-groups, 0 failures).⚠ Reviewer attention
monoio0.2.4 (vendor/monoio, ~27.7k lines, wired via[patch.crates-io]). This is a fork of upstream 0.2.4 (git shaf7827ddd, in.cargo_vcs_info.json) with a bounded "moon patch" (grep "moon patch", 4 files) implementing the--io-busy-poll-uspoll-mode park. Verified vs upstream — see security review above. It ships because the default runtime is monoio and the busy-poll win depends on it.ResponseSlotPtris nowArc-owned (src/shard/dispatch.rs,src/server/response_slot.rs) — the memory-safety fix.Perf validation
Numbers measured on GCE dedicated-vCPU instances (see BENCHMARK.md §6.6). A same-instance A/B confirmed the Arc UAF fix preserves the multi-shard c8/c64 s4 P1 win vs the pre-Arc commit (near-free: one refcount op on an already-hot cache line).
Deferred follow-ups (tracked, non-blocking)
vendor/monoiovs pristine upstream (security MEDIUM).SAFETY:comment on the legacy-driver spin-hook no-reentrancy invariant (security LOW).BITOP/COPY/DEL/UNLINK.🤖 Reviewed by security-reviewer + senior-rust-engineer subagents; findings addressed above.