fix(kv): v3-4 KV write correctness & data-integrity parity#206
Closed
pilotspacex-byte wants to merge 10 commits into
Closed
fix(kv): v3-4 KV write correctness & data-integrity parity#206pilotspacex-byte wants to merge 10 commits into
pilotspacex-byte wants to merge 10 commits into
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
Qodo is busy workingCheck back in a few minutes. Qodo's code review agents are on it. |
📝 WalkthroughWalkthroughThis PR adds a KV correctness milestone with expiry overflow guards, wrong-type non-destructive checks for GETDEL/GETSET/SET..GET, a new atomic MSETNX command with cross-shard CROSSSLOT rejection, coordinator local-leg AOF durability for co-located MSET/MSETNX, along with milestone/changelog docs and extensive tests. ChangesKV correctness and MSETNX feature
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
PR Summary by QodoFix KV write correctness: WRONGTYPE safety, overflow guards, atomic MSETNX, durable coordinator local legs
AI Description
Diagram
High-Level Assessment
Files changed (22)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/command/key.rs (2)
1337-2181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
key.rsexceeds the 1500-line guideline.The file already spans well past the 1500-line limit; this PR's new tests push it further. Consider splitting command groups/tests into submodules, particularly since this file houses many unrelated key-command families.
As per coding guidelines, "No single Rust source file should exceed 1500 lines; split command groups into submodules and move read/write operations into separate files when they exceed 1000 lines."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command/key.rs` around lines 1337 - 2181, The test module in key.rs is pushing the file past the 1500-line limit, so it should be split into smaller submodules. Move the grouped key-command tests from mod tests into separate test modules/files organized by command family (for example, expiry, rename, scan, type, and misc), and keep the existing helpers like setup_db_with_key, setup_db_with_expiry, and bs in a shared test support module so the individual test groups can reuse them cleanly.Source: Coding guidelines
88-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated overflow/range-check pattern into a shared helper.
The
checked_mul/checked_add+expiry_ms_in_rangefilter chain is duplicated three times with only the unit multiplier and base (current_time_ms()vs.0) differing. Consolidating intohelpers.rswould reduce duplication and centralize any future adjustment to the overflow-rejection semantics.♻️ Proposed helper extraction
+/// Computes `base_ms + delta * unit_ms`, bounded to the i64 domain. +/// Returns `None` on overflow or when the result is not representable +/// as a positive i64 (see `expiry_ms_in_range`). +#[inline] +pub fn checked_expiry_ms(base_ms: u64, delta: u64, unit_ms: u64) -> Option<u64> { + delta + .checked_mul(unit_ms) + .and_then(|d| base_ms.checked_add(d)) + .filter(|ms| expiry_ms_in_range(*ms)) +}- let expires_at_ms = match (seconds as u64) - .checked_mul(1000) - .and_then(|delta| current_time_ms().checked_add(delta)) - .filter(|ms| expiry_ms_in_range(*ms)) - { + let expires_at_ms = match checked_expiry_ms(current_time_ms(), seconds as u64, 1000) { Some(ms) => ms, None => { ... } };Also applies to: 152-164, 296-308
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/command/key.rs` around lines 88 - 116, The overflow/range-check logic is duplicated in the EXPIRE/EXPIREAT/related expiry paths, so extract the checked_mul/checked_add plus expiry_ms_in_range sequence into a shared helper in helpers.rs and have the key command code call that helper from the EXPIRE handling in key.rs. Keep the helper generic over the base time and multiplier so the existing semantics stay unchanged while centralizing the rejection behavior for all call sites.src/shard/coordinator.rs (1)
1-2575: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFile exceeds the 1500-line cap and this PR adds further to it.
This file is already well over the guideline's line limit (the
testsmodule alone starts around line 2264), and this PR adds ~150 more lines (persist_local_leg,serialize_local_mset,coordinate_msetnx). Consider splitting command-group coordination logic (e.g., MSET/MSETNX coordination, or the local-leg persistence helpers) into a submodule as a follow-up.As per coding guidelines, "No single Rust source file should exceed 1500 lines; split command groups into submodules and move read/write operations into separate files when they exceed 1000 lines."
#!/bin/bash wc -l src/shard/coordinator.rs🤖 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/coordinator.rs` around lines 1 - 2575, The coordinator module is far beyond the file-size guideline and this change adds more logic to an already oversized file. Move the newly added MSET/MSETNX local-persistence helpers and related coordination functions out of `coordinate_multi_key` into a dedicated submodule, keeping `src/shard/coordinator.rs` as a thin dispatcher with shared utilities only. Use the existing symbols `persist_local_leg`, `serialize_local_mset`, and `coordinate_msetnx` as the split boundary, and move the `tests` module or most of it into separate test files if needed to bring the source file back under the limit.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/command/string/string_write.rs`:
- Around line 254-274: The MSETNX flow in string_write::msetnx only validates
keys before writing, so a non-bytes value in a later pair can cause partial
writes and then an err_wrong_args("MSETNX") return. Update the MSETNX handling
to validate every key and value in the first pass, alongside the existing
existence checks, and only call db.set_string after all pairs have been
confirmed valid and absent; use the msetnx loop and extract_bytes paths to
locate the fix.
---
Nitpick comments:
In `@src/command/key.rs`:
- Around line 1337-2181: The test module in key.rs is pushing the file past the
1500-line limit, so it should be split into smaller submodules. Move the grouped
key-command tests from mod tests into separate test modules/files organized by
command family (for example, expiry, rename, scan, type, and misc), and keep the
existing helpers like setup_db_with_key, setup_db_with_expiry, and bs in a
shared test support module so the individual test groups can reuse them cleanly.
- Around line 88-116: The overflow/range-check logic is duplicated in the
EXPIRE/EXPIREAT/related expiry paths, so extract the checked_mul/checked_add
plus expiry_ms_in_range sequence into a shared helper in helpers.rs and have the
key command code call that helper from the EXPIRE handling in key.rs. Keep the
helper generic over the base time and multiplier so the existing semantics stay
unchanged while centralizing the rejection behavior for all call sites.
In `@src/shard/coordinator.rs`:
- Around line 1-2575: The coordinator module is far beyond the file-size
guideline and this change adds more logic to an already oversized file. Move the
newly added MSET/MSETNX local-persistence helpers and related coordination
functions out of `coordinate_multi_key` into a dedicated submodule, keeping
`src/shard/coordinator.rs` as a thin dispatcher with shared utilities only. Use
the existing symbols `persist_local_leg`, `serialize_local_mset`, and
`coordinate_msetnx` as the split boundary, and move the `tests` module or most
of it into separate test files if needed to bring the source file back under the
limit.
🪄 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: 3e7872d6-69aa-41a8-bac0-67f7cdc96e78
📒 Files selected for processing (22)
.add/milestones/v3-3-vector-kv-polish/MILESTONE.md.add/milestones/v3-4-kv-correctness/MILESTONE.md.add/state.jsonBENCHMARK.mdCHANGELOG.mdCLAUDE.mdscripts/test-commands.shscripts/test-consistency.shsrc/command/helpers.rssrc/command/key.rssrc/command/metadata.rssrc/command/mod.rssrc/command/string/mod.rssrc/command/string/string_read.rssrc/command/string/string_write.rssrc/persistence/replay.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/shared.rssrc/shard/coordinator.rstests/coordinator_local_leg_durability.rstests/msetnx_cross_shard_reject.rs
Comment on lines
+254
to
+274
| // Phase 1: verify NONE of the keys exist. | ||
| for pair in args.chunks(2) { | ||
| let key = match extract_bytes(&pair[0]) { | ||
| Some(k) => k, | ||
| None => return err_wrong_args("MSETNX"), | ||
| }; | ||
| if db.exists(key) { | ||
| return Frame::Integer(0); | ||
| } | ||
| } | ||
| // Phase 2: all keys absent -> set them all. | ||
| for pair in args.chunks(2) { | ||
| let key = match extract_bytes(&pair[0]) { | ||
| Some(k) => k.clone(), | ||
| None => return err_wrong_args("MSETNX"), | ||
| }; | ||
| let value = match extract_bytes(&pair[1]) { | ||
| Some(v) => v.clone(), | ||
| None => return err_wrong_args("MSETNX"), | ||
| }; | ||
| db.set_string(key, value); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate all MSETNX values before any write.
Phase 1 only validates keys. If a later value is not bytes, Phase 2 can write earlier pairs and then return err_wrong_args("MSETNX"), breaking the command’s all-or-nothing contract.
Proposed fix
- // Phase 1: verify NONE of the keys exist.
+ // Phase 1: validate all key/value frames before any semantic check or write.
+ for pair in args.chunks(2) {
+ if extract_bytes(&pair[0]).is_none() || extract_bytes(&pair[1]).is_none() {
+ return err_wrong_args("MSETNX");
+ }
+ }
+ // Phase 2: verify NONE of the keys exist.
for pair in args.chunks(2) {
let key = match extract_bytes(&pair[0]) {
Some(k) => k,
None => return err_wrong_args("MSETNX"),
};
if db.exists(key) {
return Frame::Integer(0);
}
}
- // Phase 2: all keys absent -> set them all.
+ // Phase 3: all keys absent -> set them all.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Phase 1: verify NONE of the keys exist. | |
| for pair in args.chunks(2) { | |
| let key = match extract_bytes(&pair[0]) { | |
| Some(k) => k, | |
| None => return err_wrong_args("MSETNX"), | |
| }; | |
| if db.exists(key) { | |
| return Frame::Integer(0); | |
| } | |
| } | |
| // Phase 2: all keys absent -> set them all. | |
| for pair in args.chunks(2) { | |
| let key = match extract_bytes(&pair[0]) { | |
| Some(k) => k.clone(), | |
| None => return err_wrong_args("MSETNX"), | |
| }; | |
| let value = match extract_bytes(&pair[1]) { | |
| Some(v) => v.clone(), | |
| None => return err_wrong_args("MSETNX"), | |
| }; | |
| db.set_string(key, value); | |
| // Phase 1: validate all key/value frames before any semantic check or write. | |
| for pair in args.chunks(2) { | |
| if extract_bytes(&pair[0]).is_none() || extract_bytes(&pair[1]).is_none() { | |
| return err_wrong_args("MSETNX"); | |
| } | |
| } | |
| // Phase 2: verify NONE of the keys exist. | |
| for pair in args.chunks(2) { | |
| let key = match extract_bytes(&pair[0]) { | |
| Some(k) => k, | |
| None => return err_wrong_args("MSETNX"), | |
| }; | |
| if db.exists(key) { | |
| return Frame::Integer(0); | |
| } | |
| } | |
| // Phase 3: all keys absent -> set them all. | |
| for pair in args.chunks(2) { | |
| let key = match extract_bytes(&pair[0]) { | |
| Some(k) => k.clone(), | |
| None => return err_wrong_args("MSETNX"), | |
| }; | |
| let value = match extract_bytes(&pair[1]) { | |
| Some(v) => v.clone(), | |
| None => return err_wrong_args("MSETNX"), | |
| }; | |
| db.set_string(key, value); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/command/string/string_write.rs` around lines 254 - 274, The MSETNX flow
in string_write::msetnx only validates keys before writing, so a non-bytes value
in a later pair can cause partial writes and then an err_wrong_args("MSETNX")
return. Update the MSETNX handling to validate every key and value in the first
pass, alongside the existing existence checks, and only call db.set_string after
all pairs have been confirmed valid and absent; use the msetnx loop and
extract_bytes paths to locate the fix.
Contributor
Author
GCloud A/B benchmark — Finding-1 durability fix validatedMeasured what making the co-located A/B design: Verdict: ship. The fix's cost is free or confined to a non-default config:
The
|
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 4, 2026
…c reply-slot UAF fix (#207) Bundles everything since v0.4.1: v3-4 KV write correctness & data-integrity parity (supersedes #206), the p=1 poll-mode park (--io-busy-poll-us) + multi-shard convoy fix + L3b, the vendored monoio 0.2.4 fork, and the review-driven Arc-owned ResponseSlotPtr fix for the panic-unwind cross-shard reply UAF. Reviews: security (supply-chain) clean; rust drop-safety finding fixed in-PR. Full CI green both runtimes.
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
Contributor
Author
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.
Milestone: v3-4 — KV Write Correctness & Data-Integrity Parity
Brings Moon's primary KV engine into line with Redis's data-integrity contracts:
no wrong-type data loss, no integer-overflow panics/wraps, past-expire deletes,
atomic
MSETNX, and — closing the milestone's adversarial-review Finding 1 —durable cross-shard coordinator local legs.
Single-shard (
--shards 1, the recommended default) users are unaffected by thecross-shard items; the coordinator is bypassed when
num_shards <= 1.Changes
Data-integrity (P0)
GETDEL/GETSET/SET … GETon a key holding anon-string now return
WRONGTYPEand preserve the value (check-before-mutate),instead of silently deleting/overwriting it.
DECRBY key -9223372036854775808no longer panics; everyexpiry-setting command (
SET EX/PX/EXAT,SETEX,PSETEX,EXPIRE/EXPIREAT/PEXPIRE) rejects a time whose absolute expiry falls outsidethe
i64-millisecond domain — matching Redis. Closes a latent wrap where anaccepted-but-huge TTL surfaced as a negative
PTTL/PEXPIRETIMEon a live key.EXPIRE/PEXPIRE/EXPIREATwith a non-positive/already-pasttime deletes the key and returns 1 (Redis parity); verified to propagate through
command-based WAL replay so replicas stay consistent.
New command
MSETNX— atomic multi-key string write (set all iff none exist; returns 1/0).Single-shard atomic (two-phase check-then-set, no await between phases). The
cross-shard coordinator rejects a key span with
CROSSSLOT(by design — no 2PC) andruns co-located
{hash-tag}keys atomically on the owning shard.Finding 1 (HIGH, pre-existing) — cross-shard coordinator local-leg durability
The 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) always persists viawal_append_and_fanout. So a co-locatedMSET/MSETNXwas durable when the owner was a remote shard but silentlynon-durable when the owner was the connection's own shard
(blast radius:
--shards >1+appendonly+ own-shard keys).Fix: the local leg now persists to the owning shard's AOF via a new
persist_local_leghelper — the sameAofWriterPool::issue_append_lsn+try_send_append_durablepath every local single-key write already uses(
ChannelMeshhas no self-send slot, so a local leg cannot route through the SPSCwal_append_and_fanout). Granularity: the whole command for a co-located owner;a synthesized
MSETover only the local keys for a scatteredMSET's local slice(never the full scattered command —
my_sharddoesn't own the remote keys, and replayre-dispatches raw commands). Returns
AOF_FSYNC_ERRinstead of a false+OKon appendfailure. This strictly adds durability and changes nothing about replication
(live fan-out via
replica_txsis SPSC-only and untouched).Tests
--shards 4integration).tests/coordinator_local_leg_durability.rs— 3 crash-recovery tests(
--shards 4 --appendonly yes; one co-located group per shard so exactly one is thelocal leg regardless of SO_REUSEPORT landing; SIGKILL → restart → reload). RED before
the fix, GREEN after, on both monoio and tokio.
cargo fmt --check+ both clippy gates clean; full lib suites green on monoio andtokio (
MOON_NO_URING=1).Follow-up (disclosed, not in this milestone's command scope)
The same coordinator local-leg non-durability still applies to
BITOP/COPY(via
run_on_owner's local branch) andDEL/UNLINK(viacoordinate_multi_del_or_exists). These are tracked as a focused follow-up to routetheir local legs through
persist_local_legtoo.Summary by CodeRabbit
MSETNX, including atomic all-or-nothing writes on a single shard andCROSSSLOTrejection across shards.GETDEL,GETSET, andSET ... GETno longer overwrite or delete values unexpectedly.