feat: close Redis command parity gaps — 24 new commands (#62)#66
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughAdds Phase 101 features: HyperLogLog storage and commands, Redis 7 Functions API (FUNCTION/FCALL/FCALL_RO), multiple blocking list/zset ops, sorted-set 6.2+ commands, convenience list/set/hash commands, command metadata updates, benchmarks/seed scripts, and accompanying tests and wiring in connection/dispatch layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Connection Handler
participant Registry as FunctionRegistry
participant Lua as Lua VM
participant DB as Database
Client->>Handler: FCALL funcname numkeys [keys] [args]
Handler->>Registry: call_function(funcname, keys, argv, read_only)
Registry->>Registry: Resolve function → library & callback
Registry->>Lua: Set KEYS / ARGV globals, install timeout hook, set read-only bridge
Lua->>Lua: Execute registered Lua callback
alt Lua invokes redis.call/pcall
Lua->>Handler: bridged command
Handler->>DB: execute (enforce read-only if set)
DB-->>Handler: result
Handler-->>Lua: return result to script
end
Lua-->>Registry: return value
Registry->>Registry: convert value → Frame, remove timeout hook, clear bridge state
Registry-->>Handler: Frame
Handler-->>Client: Response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Review Summary by QodoClose Redis command parity gaps — 24 new commands with HyperLogLog, Functions API, and sorted set refactoring
WalkthroughsDescription• **Redis command coverage expansion**: Implements 24 new commands across 6 priority groups, raising coverage from ~72% to ~82% • **Blocking list operations**: BLMPOP, BRPOPLPUSH, BLPOP, BRPOP, BLMOVE with per-key wait-queue, FIFO wake, timeout, and MULTI/EXEC conversion support • **HyperLogLog implementation**: PFADD, PFCOUNT, PFMERGE with byte-identical HYLL wire format (16-byte header + dense/sparse encoding), MurmurHash64A hashing, and Ertl improved cardinality estimator • **List/hash/set convenience commands**: LPUSHX, RPUSHX, LMPOP, HRANDFIELD, SMOVE, SINTERCARD with full flag and option support • **Sorted set 6.2+ commands**: ZRANGESTORE, ZDIFF, ZUNION, ZINTER, ZINTERCARD, ZMSCORE, ZRANDMEMBER, ZMPOP, BZMPOP with flexible range modes and set operations • **Functions API (Redis 7.0+)**: FUNCTION LOAD/LIST/DELETE/FLUSH, FCALL, FCALL_RO with per-library Lua VM isolation, shebang parsing, and read-only enforcement • **Sorted set refactoring**: Monolithic sorted_set.rs (3092 lines) split into modular structure (basic.rs, range.rs, setops.rs, multi.rs) with zero behavior change • **Infrastructure improvements**: Function registry threading through event loop and connection handlers, blocking command detection extensions, database helper methods • **Comprehensive testing**: 5 blocking list integration tests, 9 Functions API integration tests, 10 HyperLogLog unit tests, 3 wire format compatibility tests, plus command and consistency test scripts • **Benchmark validation**: Side-by-side performance comparison scripts for all 24 commands showing near-parity with Redis 8.0.2 Diagramflowchart LR
A["24 New Commands"] --> B["Blocking Ops"]
A --> C["HyperLogLog"]
A --> D["List/Hash/Set"]
A --> E["Sorted Set 6.2+"]
A --> F["Functions API"]
B --> B1["BLMPOP, BRPOPLPUSH"]
B --> B2["BLPOP, BRPOP, BLMOVE"]
B --> B3["BZPOPMIN, BZPOPMAX, BZMPOP"]
C --> C1["PFADD, PFCOUNT, PFMERGE"]
C --> C2["HYLL Wire Format"]
D --> D1["LPUSHX, RPUSHX, LMPOP"]
D --> D2["HRANDFIELD, SMOVE, SINTERCARD"]
E --> E1["ZRANGE*, ZDIFF, ZUNION, ZINTER"]
E --> E2["ZINTERCARD, ZMSCORE, ZRANDMEMBER, ZMPOP"]
F --> F1["FUNCTION LOAD/LIST/DELETE/FLUSH"]
F --> F2["FCALL, FCALL_RO"]
G["Refactoring"] --> G1["sorted_set.rs Split"]
G1 --> G2["basic.rs, range.rs, setops.rs, multi.rs"]
H["Infrastructure"] --> H1["Function Registry"]
H --> H2["Blocking Command Support"]
I["Testing"] --> I1["Integration Tests"]
I --> I2["Unit Tests"]
I --> I3["Consistency Tests"]
File Changes1. src/command/sorted_set/mod.rs
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+44
to
+61
| } else if sub.eq_ignore_ascii_case(b"DUMP") { | ||
| Frame::Error(Bytes::from_static( | ||
| b"ERR FUNCTION DUMP not supported in this release (Phase 101 limitation)", | ||
| )) | ||
| } else if sub.eq_ignore_ascii_case(b"RESTORE") { | ||
| Frame::Error(Bytes::from_static( | ||
| b"ERR FUNCTION RESTORE not supported in this release (Phase 101 limitation)", | ||
| )) | ||
| } else if sub.eq_ignore_ascii_case(b"STATS") { | ||
| Frame::Error(Bytes::from_static( | ||
| b"ERR FUNCTION STATS not supported in this release (Phase 101 limitation)", | ||
| )) | ||
| } else { | ||
| Frame::Error(Bytes::from(format!( | ||
| "ERR unknown subcommand '{}'. Try FUNCTION HELP.", | ||
| String::from_utf8_lossy(sub) | ||
| ))) | ||
| } |
There was a problem hiding this comment.
1. function dump/restore/stats unsupported 📎 Requirement gap ≡ Correctness
The new FUNCTION handler explicitly returns -ERR ... not supported for DUMP, RESTORE, and STATS, and the registry is documented as RAM-only (no RDB/AOF persistence). This violates the required Redis 7 Functions API parity and persistence semantics.
Agent Prompt
## Issue description
`FUNCTION DUMP`, `FUNCTION RESTORE`, and `FUNCTION STATS` are currently stubbed with `-ERR ... not supported`, and the Functions registry is explicitly RAM-only (no RDB/AOF persistence). This does not meet the required Redis 7 Functions API parity.
## Issue Context
The compliance requirement expects full command coverage for Functions API subcommands and persistence across restart / RDB/AOF round-trip, plus correct lifecycle semantics.
## Fix Focus Areas
- src/command/functions.rs[44-61]
- src/scripting/functions.rs[1-4]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+22
to
+32
| /// Format a float score for Redis output (strip trailing zeros, but keep at least one decimal). | ||
| pub(super) fn format_score(score: f64) -> String { | ||
| if score == f64::INFINITY { | ||
| "inf".to_string() | ||
| } else if score == f64::NEG_INFINITY { | ||
| "-inf".to_string() | ||
| } else { | ||
| // Use ryu or manual formatting to match Redis behavior | ||
| let s = format!("{}", score); | ||
| s | ||
| } |
There was a problem hiding this comment.
2. format_score() allocates strings 📘 Rule violation ➹ Performance
format_score() uses format!/.to_string() to create new heap-allocated Strings in sorted-set command code. This introduces avoidable allocations/conversions on a hot command path.
Agent Prompt
## Issue description
Score formatting in `src/command/sorted_set/` currently allocates via `format!()` / `.to_string()`, which violates the hot-path allocation rule.
## Issue Context
Sorted-set commands are performance-sensitive, and score formatting may be executed for many elements per request.
## Fix Focus Areas
- src/command/sorted_set/mod.rs[22-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+424
to
+428
| if args.len() == 1 { | ||
| // Single random field | ||
| use rand::seq::IndexedRandom; | ||
| let (field, _) = fields.choose(&mut rng).unwrap(); | ||
| return Frame::BulkString((*field).clone()); |
There was a problem hiding this comment.
3. hrandfield uses unwrap() 📘 Rule violation ☼ Reliability
New command-path code uses .unwrap() in non-test code (e.g., random selection in HRANDFIELD and score-change logic in ZADD), which can panic if invariants are broken. This violates the no-unwrap()/expect() requirement for library code.
Agent Prompt
## Issue description
Command implementations contain new `.unwrap()` calls in non-test code, which can panic and violates the project rule against unwrap/expect in library code.
## Issue Context
Even if current logic intends these options to be non-empty, defensive handling is required in library code (return an error frame or handle `None` safely).
## Fix Focus Areas
- src/command/hash.rs[424-428]
- src/command/sorted_set/basic.rs[123-125]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+438
to
+439
| #[allow(clippy::too_many_arguments)] | ||
| pub(super) fn zrange_from_entries( |
There was a problem hiding this comment.
4. clippy::too_many_arguments allow added 📘 Rule violation ⚙ Maintainability
A new #[allow(clippy::too_many_arguments)] was introduced without an explicit justification comment. This expands the Clippy allow-list contrary to policy.
Agent Prompt
## Issue description
A new `#[allow(clippy::too_many_arguments)]` was added without justification, which is disallowed by the compliance checklist.
## Issue Context
If the suppression is necessary, it must be narrowly scoped and include an in-code justification explaining why refactoring is impractical.
## Fix Focus Areas
- src/command/sorted_set/mod.rs[438-439]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
242
to
248
| } | ||
| } | ||
| (5, b'l') => { | ||
| // LPUSH LTRIM LMOVE | ||
| // LPUSH LTRIM LMOVE LMPOP | ||
| if cmd.eq_ignore_ascii_case(b"LPUSH") { | ||
| return resp(list::lpush(db, args)); | ||
| } |
There was a problem hiding this comment.
5. Wrong shard routing key 🐞 Bug ≡ Correctness
Sharded routing uses the first argument as the primary key, but new commands like LMPOP/SINTERCARD/ZDIFF/ZMPOP take numkeys as their first argument, so requests can be routed based on the numkeys string (e.g. "2") rather than the real key(s), executing against the wrong shard. In multi-shard mode this yields incorrect results and can apply writes to the wrong shard for ZMPOP.
Agent Prompt
### Issue description
In sharded mode, routing derives the shard target from `args[0]`, but several newly added commands use `args[0]` as `numkeys`. This causes misrouting (and for write commands like ZMPOP, can mutate the wrong shard).
### Issue Context
Affected commands include at least: `LMPOP`, `SINTERCARD`, `ZDIFF`, `ZUNION`, `ZINTER`, `ZINTERCARD`, `ZMPOP` (and any other numkeys-first forms added in this PR).
### Fix Focus Areas
- src/server/conn/shared.rs[174-246]
- src/server/conn/handler_sharded.rs[1368-1381]
- src/command/list.rs[1061-1081]
- src/command/set.rs[592-613]
### What to change
- Update `extract_primary_key` to special-case numkeys-first commands and return the first *actual key* (typically `args[1]`) after parsing `numkeys` from `args[0]` and validating bounds.
- Consider updating `is_multi_key_command` and/or adding a same-shard validation for these commands (Redis Cluster requires same hash slot), so cross-shard key sets return an error rather than silently operating on a single shard.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+303
to
+329
| let keys: Vec<Bytes> = args[2..2 + numkeys] | ||
| .iter() | ||
| .filter_map(|f| match f { | ||
| Frame::BulkString(b) => Some(b.clone()), | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Validate cross-shard keys | ||
| if num_shards > 1 { | ||
| if let Some(err) = | ||
| crate::scripting::validate_keys_same_shard(&keys, shard_id, num_shards) | ||
| { | ||
| return err; | ||
| } | ||
| } | ||
|
|
||
| let argv: Vec<Bytes> = args[2 + numkeys..] | ||
| .iter() | ||
| .filter_map(|f| match f { | ||
| Frame::BulkString(b) => Some(b.clone()), | ||
| _ => None, | ||
| }) | ||
| .collect(); | ||
|
|
||
| registry.call_function(func_name, keys, argv, db, selected_db, db_count, read_only) | ||
| } |
There was a problem hiding this comment.
6. Fcall drops non-bulk keys 🐞 Bug ≡ Correctness
handle_fcall_inner builds KEYS/ARGV via filter_map, silently dropping any non-BulkString frames, so the executed function can receive fewer than numkeys keys and shard validation can run on an incomplete key set. This can lead to incorrect function behavior and validation bypass in sharded mode.
Agent Prompt
### Issue description
FCALL/FCALL_RO must treat `numkeys` as authoritative. Current parsing silently drops non-bulk frames, which can cause KEYS/ARGV length mismatches and undermines shard-validation.
### Issue Context
`handle_fcall_inner` uses `filter_map` for both KEYS and ARGV, and then validates shard affinity on the filtered KEYS.
### Fix Focus Areas
- src/command/functions.rs[259-329]
### What to change
- Replace `filter_map` with strict parsing:
- Iterate over the `numkeys` key frames; if any is not `Frame::BulkString`, return an error (syntax/wrongtype as appropriate).
- Ensure `keys.len() == numkeys`.
- Similarly, parse ARGV strictly (or explicitly allow a limited set of frame types, but do not silently drop).
- Run `validate_keys_same_shard` on the validated full key list before executing the function.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+123
to
+155
| let (lib_name, _rest) = parse_shebang(body)?; | ||
|
|
||
| // Check for existing library | ||
| if !replace && self.libraries.contains_key(&lib_name) { | ||
| return Err(LoadError::AlreadyExists(lib_name)); | ||
| } | ||
|
|
||
| // Create the library via Lua evaluation | ||
| let library = self.create_library(lib_name.clone(), body)?; | ||
|
|
||
| // Remove old library if replacing | ||
| if let Some(old) = self.libraries.remove(&lib_name) { | ||
| for func_name in old.functions.keys() { | ||
| self.func_to_lib.remove(func_name); | ||
| } | ||
| } | ||
|
|
||
| // Check for function name collisions with other libraries | ||
| for func_name in library.functions.keys() { | ||
| if let Some(other_lib) = self.func_to_lib.get(func_name) { | ||
| if *other_lib != lib_name { | ||
| return Err(LoadError::LuaError(format!( | ||
| "Function '{}' already exists in library '{}'", | ||
| String::from_utf8_lossy(func_name), | ||
| String::from_utf8_lossy(other_lib), | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Register reverse index | ||
| for func_name in library.functions.keys() { | ||
| self.func_to_lib.insert(func_name.clone(), lib_name.clone()); |
There was a problem hiding this comment.
7. Replace load loses library 🐞 Bug ☼ Reliability
FunctionRegistry::load removes an existing library (and its reverse index entries) before checking for function-name collisions with other libraries, so a failing FUNCTION LOAD REPLACE can delete the currently-loaded library. This is observable data loss of loaded functions on an error path.
Agent Prompt
### Issue description
`FUNCTION LOAD REPLACE` should be atomic: either replace the library, or leave the existing one intact. Current ordering can delete the old library even when the new library fails validation.
### Issue Context
Old library is removed before the collision check across `func_to_lib`.
### Fix Focus Areas
- src/scripting/functions.rs[121-160]
### What to change
- Perform collision checks against the *current* registry state before mutating it (before removing the old library).
- Only after all validations pass:
- remove the old library (if replace),
- update `func_to_lib`,
- insert the new library.
- Alternatively, keep the old library in a temporary variable and restore it on error (rollback).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/command/metadata.rs (1)
223-250:⚠️ Potential issue | 🟠 MajorSeveral minimum arities here are one argument too low.
SINTERCARD,ZDIFF,ZUNION,ZINTER, andZINTERCARDall neednumkeysplus at least one key, so their minimum arity is-4, not-3. Likewise,PFADDandPFMERGErequire at leastkey + element/dest + source, so they should be-3, not-2. As written,COMMANDmetadata advertises invalid call shapes, and the currentPFADDhandler already follows that looser contract.Also applies to: 303-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/metadata.rs` around lines 223 - 250, Update the COMMAND metadata arities in src/command/metadata.rs so they advertise the correct minimum argument counts: change the CommandMeta entries for "SINTERCARD", "ZDIFF", "ZUNION", "ZINTER", and "ZINTERCARD" from arity -3 to -4 (they require numkeys plus at least one key), and change the "PFADD" and "PFMERGE" entries (the ones mentioned around the other block) from arity -2 to -3 (they require key+element / dest+source). Locate and edit the CommandMeta structs by name (e.g., "SINTERCARD", "ZDIFF", "PFADD", "PFMERGE") and update the arity integer values accordingly so COMMAND reports the correct call shapes.scripts/test-consistency.sh (1)
563-633:⚠️ Potential issue | 🟠 MajorConsistency coverage is still incomplete for the new command set.
This Phase 101 block exercises
ZMPOP,FUNCTION, andFCALL, but it never runsZRANDMEMBERorFCALL_RO. That means the PR still misses the requiredtest-consistency.shentry for part of the newly added surface.As per coding guidelines "Every new command needs at least one unit test and one consistency test entry in scripts/test-consistency.sh and scripts/test-commands.sh."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-consistency.sh` around lines 563 - 633, Missing consistency tests for ZRANDMEMBER and FCALL_RO: add entries in the Phase 101 block to exercise ZRANDMEMBER and FCALL_RO similarly to existing ZMPOP/FCALL tests so both implementations are covered; specifically, add assert_both calls for ZRANDMEMBER (e.g., after setting up a zset like z:src use both ZRANDMEMBER z:src and both ZRANDMEMBER z:src COUNT WITHSCORES variants) and add a read-only function and an assert_both invoking it via FCALL_RO (create a small FUNCTION LOAD body registering a read-only function like "hello_ro" and assert_both "FCALL_RO hello_ro" FCALL_RO hello_ro 0, plus a FUNCTION DELETE cleanup), following the existing patterns (use symbols ZRANDMEMBER and FCALL_RO, FUNCTION LOAD/DELETE, assert_both) to ensure coverage.
🟠 Major comments (21)
src/blocking/wakeup.rs-66-88 (1)
66-88:⚠️ Potential issue | 🟠 MajorClamp the wakeup pop loops before the first extra miss.
Both branches keep calling
list_pop_*/zset_pop_*untilcount, even after the last successful pop removed the key. Those helpers go throughget_or_create_*, so the first extra iteration recreates the collection as an empty key before returningNone. A waiter withCOUNTgreater than the current cardinality therefore leaves behind a phantom empty list/zset. Please clamp the loop to the current length/cardinality first, or switch these wake paths to non-creating pop helpers.Also applies to: 144-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/blocking/wakeup.rs` around lines 66 - 88, The BLMPop wake path currently calls db.list_pop_front/list_pop_back in a loop which can recreate an empty list via get_or_create_* on the first extra miss; instead obtain the current length/cardinality up-front (e.g., call the non-creating length/cardinality helper for the key) and clamp the loop iterations to min(count, length) or use non-creating pop helpers if available, then perform up to that many pops and build the reply; apply the same change to the analogous zset pop branch referenced at lines 144-174 so the wake path never reinstates a phantom empty collection.src/scripting/bridge.rs-100-108 (1)
100-108:⚠️ Potential issue | 🟠 Major
FCALL_ROneeds a stricter predicate thanis_write().This guard only blocks commands carrying the
WRITEbit, but Redis' read-only script modes are stricter than that: they only allow read-only commands, and even commands likePUBLISH/SPUBLISH/PFCOUNTare treated as writes in script context. A side-effecting command that is not modeled purely asWRITEwill still pass here, so the newFCALL_ROpath can lose its read-only guarantee. Please switch this to a dedicated “allowed from read-only script” check instead ofmetadata::is_write(). (redis.io)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripting/bridge.rs` around lines 100 - 108, The current guard uses crate::command::metadata::is_write(&cmd_bytes) which misses commands that have side effects but lack the WRITE bit; replace this predicate with a dedicated check like crate::command::metadata::is_allowed_in_read_only(&cmd_bytes) (or implement metadata::is_allowed_in_read_only / is_read_only_allowed) and use that inside the SCRIPT_READ_ONLY branch so only explicitly allowed commands (pure reads and known exceptions) are permitted; keep the SCRIPT_READ_ONLY and SCRIPT_HAD_WRITE semantics but ensure the new function enumerates/handles special cases such as PUBLISH/SPUBLISH/PFCOUNT and other commands treated as writes in script context.src/command/hash.rs-391-512 (1)
391-512:⚠️ Potential issue | 🟠 MajorAvoid materializing the whole hash before sampling.
Both variants build a full in-memory list of every field/value pair before choosing anything. That makes
HRANDFIELD keycost O(hash size) work and memory even when returning a single field, while Redis documents the command as O(N) in the number of returned elements. On large hashes this is a noticeable regression, and it also breaks the repo’s hot-path allocation rule. Please switch the single/with-replacement paths to iterator-based sampling and reserve allocations for the final reply buffer only. (redis.io)As per coding guidelines, "
**/{src/command/**,src/protocol/**,src/shard/event_loop.rs,src/io/**}/*.rs: No allocations ... on hot paths ...Vec::with_capacity()is acceptable for result building at the end of a command path."Also applies to: 1021-1136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/hash.rs` around lines 391 - 512, The hrandfield implementation materializes all map entries into fields before sampling; change hrandfield to stream over map.iter() instead: for the single-field case (args.len()==1) pick a random index r in 0..map.len() and return map.iter().nth(r).unwrap() without building a Vec; for positive count (count>0) use reservoir sampling over map.iter() to select n distinct entries into a pre-allocated result Vec (use Vec::with_capacity for final reply) and emit keys or key/value pairs depending on with_values; for negative count (with replacement) repeatedly pick a random index and use map.iter().nth(r) for each draw (or sample with replacement via streaming) and push into the final result Vec; remove creation of fields: Vec<(&Bytes,&Bytes)> and ensure the only allocations are the final reply buffers. Reference symbols: hrandfield, map.iter(), args.len(), count, with_values, and rand::rng().scripts/run-blocking-tests.sh-10-11 (1)
10-11:⚠️ Potential issue | 🟠 MajorDon't
killall -9everymoonprocess on the machine.This will terminate unrelated local instances before the test run even starts. Cleanup should be scoped to the PID this script spawned, or at least filtered to the test port, so the runner does not clobber other sessions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run-blocking-tests.sh` around lines 10 - 11, The current cleanup uses a global "killall -9 moon" which kills every moon process; change it to only terminate the instance this script started: when launching moon, capture its PID (e.g., store $! in a variable or a .pid file) and on cleanup use kill -TERM <pid> (fallback to -9 only for that PID), or alternatively detect the process by test port (use lsof/ss to find the PID listening on the test port and kill that PID). Replace the global "killall -9 moon" in run-blocking-tests.sh with one of these scoped approaches and add a trap to ensure the stored PID is killed on exit.scripts/test-commands.sh-446-447 (1)
446-447:⚠️ Potential issue | 🟠 MajorThis
HRANDFIELDcheck is still random and can flake.By this point
hsh:k1has 7 fields, soHRANDFIELD hsh:k1 6returns a random 6-field subset. Sorting normalizes order, but Redis and Moon can still legitimately choose different subsets. Make the count at least the current cardinality, or build a dedicated 6-field hash for this assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-commands.sh` around lines 446 - 447, The test calls HRANDFIELD on hsh:k1 with count 6 while hsh:k1 currently has 7 fields, which makes the returned subset nondeterministic and can flake; update the assertion so the count is at least the current cardinality (e.g., change the HRANDFIELD call to use 7) or instead create a dedicated 6-field hash fixture and point assert_match_sorted "HRANDFIELD count" at that new hash; locate the HRANDFIELD call referenced by assert_match_sorted and modify the count or the hash setup accordingly.src/command/metadata.rs-193-205 (1)
193-205:⚠️ Potential issue | 🟠 MajorMovable-key commands need real key specs, not placeholder key positions.
LMPOP/BLMPOP/BZMPOP, thenumkeysset/zset commands, andFCALL[_RO]all compute their key span at runtime. Encoding them as(0, 0, 0)or(3, 0, 1)makes the registry report “no keys” or an empty key range, which will mislead any consumer that uses this table for key discovery.Also applies to: 223-250, 363-365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/metadata.rs` around lines 193 - 205, The CommandMeta entries for movable-key/numkeys/runtime-key commands (e.g., LMPOP, BLMPOP, BZMPOP and FCALL/FCALL_RO) currently use placeholder key specs (first_key: 0, last_key: 0, step: 0 or fixed tuples) which causes the registry to report “no keys”; update those CommandMeta records to mark them as movable-key/runtime-key capable by using the proper sentinel or variant used in this codebase to indicate dynamic key computation (the same pattern used for other runtime-key commands elsewhere in the file), e.g., replace the (0,0,0) placeholders with the movable-key representation and ensure acl_categories/flags remain unchanged so consumers can compute key spans at runtime for LMPOP, BLMPOP, BZMPOP, FCALL and FCALL_RO.scripts/run-blocking-tests.sh-13-21 (1)
13-21:⚠️ Potential issue | 🟠 MajorAdd explicit readiness check and trap-based cleanup to prevent stale server processes.
The PING retry loop (lines 18-21) falls through after 30 failed attempts without verifying the server actually became ready, allowing tests to run against an unstarted server. Additionally, with
set -eenabled, any non-zero exit from$TBINat line 37 aborts before reaching cleanup at lines 41-42, leaving the server process behind.Use a readiness flag with explicit error handling and a trap to ensure cleanup always executes:
Suggested fix
#!/usr/bin/env bash set -euo pipefail -killall -9 moon 2>/dev/null || true -sleep 0.5 +cleanup() { + [[ -n "${SERVER_PID:-}" ]] || return 0 + kill "$SERVER_PID" 2>/dev/null || true + wait "$SERVER_PID" 2>/dev/null || true +} +trap cleanup EXIT # Start server $BINARY --port $PORT --shards 1 &>/dev/null & SERVER_PID=$! # Wait for port -for i in $(seq 1 30); do - redis-cli -p $PORT PING 2>/dev/null | grep -q PONG && break +ready=false +for _ in $(seq 1 30); do + if redis-cli -p "$PORT" PING 2>/dev/null | grep -q PONG; then + ready=true + break + fi sleep 0.2 done +if [[ "$ready" != true ]]; then + echo "ERROR: moon did not become ready on port $PORT" >&2 + exit 1 +fi-$TBIN --test-threads=1 "$@" -RC=$? +"$TBIN" --test-threads=1 "$@" || RC=$? +: "${RC:=0}" -# Cleanup -kill $SERVER_PID 2>/dev/null -wait $SERVER_PID 2>/dev/null || true exit $RC🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/run-blocking-tests.sh` around lines 13 - 21, The startup loop using redis-cli PING should set and check an explicit readiness flag and fail if the server never becomes ready: update the PING retry block (the loop that greps PONG) to set READY=1 when PING succeeds and after the loop test READY and exit non-zero with a clear message if not ready; also add a trap-based cleanup function that kills the background server using SERVER_PID and waits for it to exit, register the trap (e.g., trap cleanup EXIT) near script start so cleanup always runs even when set -e causes early exit, and ensure any use of $BINARY/$TBIN references the SERVER_PID for proper teardown.scripts/bench-phase101-seed.py-14-20 (1)
14-20:⚠️ Potential issue | 🟠 MajorFail fast if
redis-cliseeding fails.These subprocess calls discard the exit status and stderr, so a missing
redis-clior a rejected command can leave benchmarks running against partially-seeded data. Please usecheck=Trueor explicitreturncodehandling here.Suggested fix
def pipe(port, commands): """Send commands via redis-cli --pipe.""" - data = "".join(commands) - p = subprocess.run( + data = b"".join(commands) + subprocess.run( ["redis-cli", "-p", str(port), "--pipe"], - input=data.encode(), capture_output=True + input=data, + capture_output=True, + check=True, ) @@ subprocess.run( ["redis-cli", "-p", str(port), "FUNCTION", "FLUSH"], - capture_output=True + capture_output=True, + check=True, ) subprocess.run( ["redis-cli", "-p", str(port), "FUNCTION", "LOAD", "REPLACE", body], - capture_output=True + capture_output=True, + check=True, )Also applies to: 75-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bench-phase101-seed.py` around lines 14 - 20, The subprocess run in function pipe currently ignores exit status and stderr, so update the call in pipe (and the similar call around lines 75-81) to fail fast: either pass check=True to subprocess.run or inspect the CompletedProcess.returncode and subprocess.CompletedProcess.stderr and raise/exit with a clear error message if non-zero; include the stderr content in the logged/raised error so missing redis-cli or rejected commands abort the script immediately.src/server/conn/blocking.rs-765-785 (1)
765-785:⚠️ Potential issue | 🟠 MajorReject malformed suffixes after
COUNTforBLMPOP/BZMPOP.Both parsers currently accept extra trailing tokens. Inputs like
... LEFT FOO,... COUNT, or... COUNT 2 EXTRAwill slip through instead of returning a syntax error, which breaks parity for the new blocking commands.Suggested fix
- let mut count: u32 = 1; - let remaining = &args[3 + numkeys..]; - if remaining.len() >= 2 { - let kw = extract_bytes(&remaining[0]); - if let Some(kw) = kw { - if kw.eq_ignore_ascii_case(b"COUNT") { - let count_bytes = extract_bytes(&remaining[1]) - .ok_or_else(|| Frame::Error(Bytes::from_static(b"ERR syntax error")))?; - count = std::str::from_utf8(&count_bytes) - .map_err(|_| Frame::Error(Bytes::from_static(b"ERR count is not an integer")))? - .parse() - .map_err(|_| Frame::Error(Bytes::from_static(b"ERR count is not an integer or is out of range")))?; - if count == 0 { - return Err(Frame::Error(Bytes::from_static( - b"ERR count is not an integer or is out of range", - ))); - } - } - } - } + let mut count: u32 = 1; + let remaining = &args[3 + numkeys..]; + match remaining { + [] => {} + [kw, value] if extract_bytes(kw).is_some_and(|kw| kw.eq_ignore_ascii_case(b"COUNT")) => { + let count_bytes = extract_bytes(value) + .ok_or_else(|| Frame::Error(Bytes::from_static(b"ERR syntax error")))?; + count = std::str::from_utf8(&count_bytes) + .map_err(|_| Frame::Error(Bytes::from_static(b"ERR count is not an integer")))? + .parse() + .map_err(|_| Frame::Error(Bytes::from_static(b"ERR count is not an integer or is out of range")))?; + if count == 0 { + return Err(Frame::Error(Bytes::from_static( + b"ERR count is not an integer or is out of range", + ))); + } + } + _ => return Err(Frame::Error(Bytes::from_static(b"ERR syntax error"))), + }Also applies to: 842-862
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/conn/blocking.rs` around lines 765 - 785, The parser currently allows extra trailing tokens after the key/block parsing (via remaining) and thus accepts malformed suffixes like stray words or incomplete COUNT; modify the COUNT-handling logic (the block using remaining, extract_bytes, count and Frame::Error) so that: if remaining is empty do nothing; otherwise require remaining.len() == 2 and the first token (kw from extract_bytes) must equal "COUNT" (case-insensitive); if the first token is not COUNT or the length is not exactly 2 return a syntax Frame::Error (same message used for other parse errors), and keep the existing integer parsing/zero-check for the COUNT value using extract_bytes/std::str::from_utf8/parse as before; apply the same change to the analogous parser block for BZMPOP.src/server/conn/handler_sharded.rs-716-723 (1)
716-723:⚠️ Potential issue | 🟠 MajorBroadcast mutating
FUNCTIONsubcommands to the other shards.This updates only the local
FunctionRegistry, while the analogousSCRIPT LOADpath at Line 697 explicitly fans out cache changes. In a multi-shard server that will make function availability depend on which shard handledFUNCTION LOAD, andFCALLcan start failing after key routing or connection migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/conn/handler_sharded.rs` around lines 716 - 723, The FUNCTION branch currently only updates the local FunctionRegistry via handle_function(&mut func_registry.borrow_mut(), cmd_args) causing inconsistent function availability across shards; change it to detect mutating FUNCTION subcommands (e.g., LOAD, DELETE, FLUSH or whatever your protocol treats as mutating), call handle_function, and if it succeeds then broadcast the same mutating command to the other shards using the same fan-out mechanism used by the SCRIPT LOAD path (reuse the existing broadcast/send-to-other-shards helper employed there) so that func_registry changes are applied cluster-wide; ensure broadcasting happens only on successful local mutation and include the original cmd_args when sending.src/server/conn/handler_monoio.rs-699-706 (1)
699-706:⚠️ Potential issue | 🟠 MajorReplicate FUNCTION registry mutations to every shard.
Unlike
SCRIPT LOADat Line 674, this only updates the localRc<RefCell<FunctionRegistry>>. In sharded mode that leaves other shards stale, so a laterFCALLon a remotely routed or migrated connection can fail with an unknown function even thoughFUNCTION LOADalready succeeded on the server.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/conn/handler_monoio.rs` around lines 699 - 706, The FUNCTION command handler only mutates the local Rc<RefCell<FunctionRegistry>> via crate::command::functions::handle_function (in the FUNCTION branch) and so leaves other shards stale; update this branch to replicate the registry mutation to every shard using the same replication logic used by the SCRIPT LOAD branch (i.e., after calling handle_function on func_registry, invoke the existing shard-broadcast/replication routine used for SCRIPT LOAD so all shards receive the FUNCTION LOAD/UPDATE and update their FunctionRegistry instances), ensuring responses.push(response) still runs and errors from replication are handled/logged consistently.scripts/bench-phase101-commands.sh-205-207 (1)
205-207:⚠️ Potential issue | 🟠 MajorRe-seed destructive workloads before timing them.
These sections benchmark commands that mutate or exhaust their input data, but the dataset is only seeded once. After the first few requests,
LMPOP/ZMPOP/SMOVE/LPOP/RPOPare no longer measuring the same hit path, and later sections inherit earlier mutations. That makes the Redis-vs-Moon ratios misleading. The existingreseed_list/reseed_zsethelpers should be used around the destructive cases.Also applies to: 233-237, 262-287, 303-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bench-phase101-commands.sh` around lines 205 - 207, The benchmark mutates datasets but only seeds once, so call the existing reseed helpers around destructive benchmark blocks: replace single initial seeding (python3 "bench-phase101-seed.py" "$PORT_REDIS"/"$PORT_MOON") with calls to reseed_list and reseed_zset for the workloads that modify data (LMPOP, ZMPOP, SMOVE, LPOP, RPOP and the other blocks noted), e.g., invoke reseed_list/reseed_zset for both targets (using the PORT_REDIS and PORT_MOON variables) immediately before each destructive timing loop so each run starts from a fresh dataset instead of inheriting prior mutations.src/command/sorted_set/range.rs-38-81 (1)
38-81:⚠️ Potential issue | 🟠 MajorReject unknown trailing options instead of skipping them.
These parsers currently advance past unrecognized tokens, so forms like
ZRANGE k 0 -1 FOOorZRANGEBYSCORE k 0 1 LIMIT 0 1 BARare treated as valid instead of returningERR syntax error. That makes typoed flags silently change behavior and diverges from Redis command validation.Also applies to: 159-198, 278-281, 314-348, 389-423, 470-512, 590-593, 630-663, 717-750
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/sorted_set/range.rs` around lines 38 - 81, The option-parsing loop in src/command/sorted_set/range.rs currently skips unrecognized trailing tokens (the match on opt and the final else branch that does i += 1), which lets invalid flags pass; change the final else to return an argument-syntax error instead of advancing: when opt is present but none of the known comparisons (opt.eq_ignore_ascii_case(b"BYSCORE"), b"BYLEX", b"REV", b"WITHSCORES", b"LIMIT") match, call err_wrong_args("ZRANGE") (or the appropriate command name for the other similar parsers) so unknown tokens are rejected; apply the same change to the other parsing blocks you listed (the blocks around the other line ranges) so all unknown trailing options return ERR syntax error rather than being skipped.src/command/list.rs-1094-1114 (1)
1094-1114:⚠️ Potential issue | 🟠 MajorReject trailing tokens after
COUNT.
LMPOP 1 key LEFT COUNT 1 junkis currently accepted because this parser only validates the first two remaining arguments and ignores anything after them. Redis treats that form as a syntax error, so malformed commands can silently execute here.Possible fix
- if remaining.len() >= 2 { + if remaining.len() == 2 { if let Some(kw) = extract_bytes(&remaining[0]) { if kw.eq_ignore_ascii_case(b"COUNT") { match parse_i64(&remaining[1]) { Some(c) if c > 0 => count = c as usize, _ => { @@ } else { return Frame::Error(Bytes::from_static(b"ERR syntax error")); } + } else { + return Frame::Error(Bytes::from_static(b"ERR syntax error")); } } else if !remaining.is_empty() { return Frame::Error(Bytes::from_static(b"ERR syntax error")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/list.rs` around lines 1094 - 1114, The parser currently reads only the first two tokens after keys and accepts extra trailing tokens; update the COUNT handling in src/command/list.rs so that after detecting the COUNT keyword (using extract_bytes) and successfully parsing the number with parse_i64 into count, you also validate there are no additional tokens (i.e., remaining.len() must equal 2); if there are extra tokens return Frame::Error(Bytes::from_static(b"ERR syntax error")). Keep the existing error paths for missing/invalid COUNT values (the current Frame::Error with the LMPOP COUNT message) and for an unrecognized keyword, but add the trailing-token check immediately after the successful parse to reject inputs like "LMPOP 1 key LEFT COUNT 1 junk".src/command/sorted_set/setops.rs-61-106 (1)
61-106:⚠️ Potential issue | 🟠 MajorReject unknown option tokens instead of skipping them.
These parsers currently fall through with
i += 1on unrecognized trailing tokens, so malformed calls likeZUNION ... WEGIHTS ...or unsupported clauses onZDIFF/ZINTERCARDcan still return a result instead ofERR syntax error. That breaks parity with Redis and makes client bugs much harder to detect.Suggested direction
- } else { - i += 1; + } else { + return err("ERR syntax error"); }For
parse_setop_args, returnErr(err("ERR syntax error"))in the same branch.Also applies to: 235-284, 505-528
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/sorted_set/setops.rs` around lines 61 - 106, The loop in parse_setop_args currently skips unknown option tokens (the final else { i += 1 }) which lets malformed option names pass; change that final else branch in parse_setop_args (and the equivalent branches in the other set-op parsers referenced) to return Err(err("ERR syntax error")) instead of advancing i so any unrecognized token triggers a syntax error; locate the loop that matches opt.eq_ignore_ascii_case(...) (the block handling "WEIGHTS" / "AGGREGATE") and replace the fall-through branch with an immediate Err(err("ERR syntax error")) return to enforce Redis parity.src/command/sorted_set/basic.rs-250-255 (1)
250-255:⚠️ Potential issue | 🟠 MajorReject
NaNafterZINCRBYarithmetic.
ZADDalready blocksNaN, butZINCRBYcan still generate one through valid inputs like an existing+infscore plus-inf. Writing that throughzadd_memberwill corrupt sorted-set ordering assumptions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/sorted_set/basic.rs` around lines 250 - 255, After computing new_score in the ZINCRBY path (where current is taken from members and increment applied), check for new_score.is_nan() and reject the operation instead of calling zadd_member; return the appropriate error Frame (matching other command error conventions) and do not mutate members or scores. Update the logic around the current/new_score calculation in the ZINCRBY handler that calls zadd_member so it validates NaN and only calls zadd_member when new_score is a finite non-NaN value; ensure the error message clearly indicates NaN result from arithmetic.src/command/functions.rs-117-191 (1)
117-191:⚠️ Potential issue | 🟠 MajorApply the
LIBRARYNAMEfilter or reject the option.
_patternis parsed but never used, soFUNCTION LIST LIBRARYNAME foo*still returns every library. That’s a functional bug and an information leak if callers expect the result set to be filtered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/functions.rs` around lines 117 - 191, The parsed _pattern in handle_function_list is never used, so implement filtering: after parsing, rename _pattern to pattern (Option<&[u8]>) and filter the registry.list() results before building result—e.g., replace let libs = registry.list() with an iterator that keeps only libraries whose lib.name matches the pattern when pattern.is_some() (use your project’s glob/matching utility or a simple wildcard/prefix match comparing lib.name.as_ref() to the byte pattern); keep behavior unchanged when pattern.is_none(). Ensure the match uses lib.name (from each Library entry) and that the filtered collection is what the subsequent loop over libs consumes (or alternatively return an ERR syntax/error if you prefer rejecting LIBRARYNAME instead of supporting filtering).src/command/sorted_set/basic.rs-53-60 (1)
53-60:⚠️ Potential issue | 🟠 Major
GTandLTshould fail fast together.Redis treats
GTandLTas incompatible options. Right now that conflict is only folded intoshould_update, which means existing members silently no-op and new members can still be inserted. This needs an error before the command touches the set.Also applies to: 101-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/sorted_set/basic.rs` around lines 53 - 60, Add an early fast-fail for the mutually exclusive GT and LT options: if gt && lt { return err("ERR GT and LT options at the same time are not compatible"); }. Place this check alongside the existing NX/XX and NX/(GT|LT) validations (before any code that touches the set and before the call to should_update) so the command errors out immediately for both existing and new members; also add the same check in the other similar validation block that mirrors lines 101-116. Ensure you reference the same variables (nx, xx, gt, lt) and that should_update logic is not relied on to surface this conflict.src/command/sorted_set/basic.rs-68-97 (1)
68-97:⚠️ Potential issue | 🟠 MajorDon’t create the sorted set before validating all pairs.
get_or_create_sorted_set()runs before score parsing, soZADD myzset not-a-float membercan leave a brand-new empty zset behind even though the command returns an error. Parse and validate every(score, member)pair first, then mutate storage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/sorted_set/basic.rs` around lines 68 - 97, The code currently calls get_or_create_sorted_set(key) before validating scores, which can create an empty zset on invalid input; instead, first iterate over remaining (using the same extract_bytes, score parsing and NaN checks used at the top) and collect/validate all (score: f64, member: Vec<u8>) pairs into a temporary Vec without mutating storage, returning err_wrong_args("ZADD") or err("ERR value is not a valid float") as needed; only after all pairs are validated call db.get_or_create_sorted_set(key) and then apply the collected pairs to members/scores, updating added and changed counters and performing the original mutation logic.src/command/sorted_set/setops.rs-47-54 (1)
47-54:⚠️ Potential issue | 🟠 MajorDon’t coerce invalid key arguments to the empty key.
Using
unwrap_or_else(Bytes::new)here means a non-extractable key frame is silently treated as"", so the command can read from or write to the wrong sorted set instead of failing. This should returnerr_wrong_args(...)as soon as any source key cannot be parsed.Also applies to: 223-229, 494-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/sorted_set/setops.rs` around lines 47 - 54, The code currently coerces non-extractable key frames to an empty key by using extract_bytes(...).cloned().unwrap_or_else(|| Bytes::new()), which hides malformed arguments; instead, change the collection of source keys so that if extract_bytes(...) returns None for any arg you immediately return err_wrong_args(...) from the surrounding command handler (i.e., do a fallible map/check before collecting into source_keys). Replace the unwrap_or_else path with an early-return error on None for the extract_bytes call used when building source_keys, and apply the same early-return pattern to the other identical sites that use extract_bytes(...).cloned().unwrap_or_else(...) in this file (the other locations around the extract_bytes usages).src/command/functions.rs-194-220 (1)
194-220:⚠️ Potential issue | 🟠 Major
FUNCTION DELETEshould enforce exact arity.This handler accepts
FUNCTION DELETE <lib> extra...and still deletes the library, because it only checksis_empty(). Redis treats that as a wrong-arity error, and deleting on an invalid command is a surprising state change.Suggested fix
-fn handle_function_delete( - registry: &mut FunctionRegistry, - args: &[Frame], -) -> Frame { - if args.is_empty() { +fn handle_function_delete( + registry: &mut FunctionRegistry, + args: &[Frame], +) -> Frame { + if args.len() != 1 { return Frame::Error(Bytes::from_static( b"ERR wrong number of arguments for 'function|delete' command", )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/functions.rs` around lines 194 - 220, The handler handle_function_delete currently only checks args.is_empty() so it accepts extra arguments; change the arity check to require exactly one argument (args.len() != 1) and return the same wrong-arity Frame::Error Bytes::from_static(b"ERR wrong number of arguments for 'function|delete' command") when arity is incorrect; keep the existing pattern-matching on args[0] to extract lib_name and then call registry.delete(lib_name) to decide between Frame::SimpleString OK and the "ERR Library '{}' not found" error.
🟡 Minor comments (4)
docs/guides/commands-user-guide.md-81-239 (1)
81-239:⚠️ Potential issue | 🟡 MinorThis guide omits a large part of the Phase 101 command surface.
The new list/set/zset sections stop at the older command set, and there is no HyperLogLog or Functions section at all. Users reading this page still won’t discover commands like
LPUSHX/RPUSHX/LMPOP,SMOVE/SINTERCARD,ZDIFF/ZUNION/ZINTER/ZINTERCARD/ZRANDMEMBER/ZMPOP,PF*, orFUNCTION/FCALL[_RO].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/commands-user-guide.md` around lines 81 - 239, The guide is missing many Phase 101 commands; update the Lists, Sets, ZSets sections to include the newer commands (e.g., LPUSHX, RPUSHX, LMPOP, LMPOS/LMOVE variants), Sets commands (SMOVE, SINTERCARD, SRANDMEMBER variants, ZRANDMEMBER), Sorted Sets (ZDIFF, ZUNION, ZINTER, ZINTERCARD, ZMPOP), add a HyperLogLog section listing PFADD/PFCOUNT/PFMERGE and examples, and add a Functions section documenting FUNCTION, FCALL, FCALL_RO with brief usage examples; ensure each section’s command list and example snippets reference the exact command names shown above so readers can discover them and keep the Command Inventory note consistent with these additions (commands are sourced from metadata.rs).src/server/conn/blocking.rs-588-595 (1)
588-595:⚠️ Potential issue | 🟡 MinorAvoid introducing
unwrap()into the library path.Line 594 can use a
let Some(timeout_frame) = args.last() else { ... };branch instead ofunwrap(). The current code is safe, but it still violates the repo's unwrap ratchet forsrc/**/*.rs.
As per coding guidelines, "src/**/*.rs: ... Nounwrap()orexpect()in library code outside tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/conn/blocking.rs` around lines 588 - 595, The code uses args.last().unwrap() to set timeout_frame for non-BLMPOP/BZMPOP commands; replace the unwrap with a pattern match like `let Some(timeout_frame) = args.last() else { ... };` and return an appropriate error (or early Result/Err) from the surrounding function when args is empty. Update the branch that assigns timeout_frame (the block comparing cmd.eq_ignore_ascii_case to b"BLMPOP"/b"BZMPOP") so it handles the missing-argument case without panicking, referencing the existing variables cmd, args, and timeout_frame.scripts/bench-phase101-seed.py-6-12 (1)
6-12:⚠️ Potential issue | 🟡 MinorCompute RESP bulk lengths from bytes, not characters.
len(str(a))is only correct for ASCII. Any future non-ASCII key/value/script body will emit the wrong$<len>header and breakredis-cli --pipeparsing. Build each argument as bytes first and use the byte length.Suggested fix
def resp(*args): """Build RESP protocol for a command.""" - parts = [f"*{len(args)}\r\n"] + parts = [f"*{len(args)}\r\n".encode()] for a in args: - s = str(a) - parts.append(f"${len(s)}\r\n{s}\r\n") - return "".join(parts) + b = a if isinstance(a, bytes) else str(a).encode() + parts.append(f"${len(b)}\r\n".encode()) + parts.append(b) + parts.append(b"\r\n") + return b"".join(parts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/bench-phase101-seed.py` around lines 6 - 12, The resp function computes bulk lengths from character counts which breaks for non-ASCII; change it to build each argument as bytes and compute lengths from byte length: in resp(), for each a call s_bytes = str(a).encode('utf-8') (or the appropriate encoding), use len(s_bytes) for the $<len> header, assemble the protocol pieces as bytes (e.g., b"*" + str(len(args)).encode() + b"\r\n" and b"$" + str(len(s_bytes)).encode() + b"\r\n" + s_bytes + b"\r\n") and return the joined bytes (b"".join(parts)) so redis-cli --pipe receives correct byte lengths.src/command/sorted_set/multi.rs-329-332 (1)
329-332:⚠️ Potential issue | 🟡 MinorReject unknown third arguments to
ZRANDMEMBER.When three args are present, anything other than
WITHSCORESis currently ignored, soZRANDMEMBER key 2 fooreturns data instead ofERR syntax error. That diverges from Redis and hides client-side mistakes.Suggested fix
- let withscores = args.len() == 3 - && extract_bytes(&args[2]) - .map(|b| b.eq_ignore_ascii_case(b"WITHSCORES")) - .unwrap_or(false); + let withscores = match args.get(2) { + None => false, + Some(arg) => match extract_bytes(arg) { + Some(b) if b.eq_ignore_ascii_case(b"WITHSCORES") => true, + _ => return err("ERR syntax error"), + }, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command/sorted_set/multi.rs` around lines 329 - 332, The current withscores calculation silently ignores unknown third arguments; change the logic around the withscores binding so that when args.len() == 3 you only allow a third argument that equals "WITHSCORES" (case-insensitive) and otherwise return a syntax error to the client; implement this by checking extract_bytes(&args[2])—if it yields Some(b) and b.eq_ignore_ascii_case(b"WITHSCORES") set withscores = true, else return the command-level syntax error (same error path used elsewhere in this command handler) instead of proceeding, ensuring ZRANDMEMBER rejects unknown third arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4e78c64-c956-45b5-a34d-aa497d8b7193
📒 Files selected for processing (39)
.gitignore.planningCargo.tomldocs/guides/commands-user-guide.mdscripts/bench-phase101-commands.shscripts/bench-phase101-seed.pyscripts/run-blocking-tests.shscripts/test-commands.shscripts/test-consistency.shsrc/blocking/mod.rssrc/blocking/wakeup.rssrc/command/functions.rssrc/command/hash.rssrc/command/hll.rssrc/command/list.rssrc/command/metadata.rssrc/command/mod.rssrc/command/set.rssrc/command/sorted_set.rssrc/command/sorted_set/basic.rssrc/command/sorted_set/mod.rssrc/command/sorted_set/multi.rssrc/command/sorted_set/range.rssrc/command/sorted_set/setops.rssrc/scripting/bridge.rssrc/scripting/functions.rssrc/scripting/mod.rssrc/server/conn/blocking.rssrc/server/conn/handler_monoio.rssrc/server/conn/handler_sharded.rssrc/shard/conn_accept.rssrc/shard/event_loop.rssrc/storage/db.rssrc/storage/hll.rssrc/storage/mod.rstests/blocking_list_timeout.rstests/functions_fcall.rstests/hll_vectors.rstests/hll_wire_compat.rs
Comment on lines
+79
to
+100
| pub fn pfadd(db: &mut Database, args: &[Frame]) -> Frame { | ||
| if args.is_empty() { | ||
| return err_wrong_args("PFADD"); | ||
| } | ||
| let key = match extract_bytes(&args[0]) { | ||
| Some(k) => k, | ||
| None => return err_wrong_args("PFADD"), | ||
| }; | ||
| let key_owned = key.clone(); | ||
|
|
||
| let existing = match load_hll(db, key) { | ||
| Ok(v) => v, | ||
| Err(e) => return e, | ||
| }; | ||
|
|
||
| let mut created = false; | ||
| let mut hll = match existing { | ||
| Some(h) => h, | ||
| None => { | ||
| created = true; | ||
| Hll::new_sparse() | ||
| } |
There was a problem hiding this comment.
Require at least one element for PFADD.
PFADD key currently succeeds and can even create a brand-new empty HLL because this only rejects args.is_empty(). Redis requires PFADD to have at least key element, so this should be a wrong-arity error.
Possible fix
- if args.is_empty() {
+ if args.len() < 2 {
return err_wrong_args("PFADD");
}📝 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
| pub fn pfadd(db: &mut Database, args: &[Frame]) -> Frame { | |
| if args.is_empty() { | |
| return err_wrong_args("PFADD"); | |
| } | |
| let key = match extract_bytes(&args[0]) { | |
| Some(k) => k, | |
| None => return err_wrong_args("PFADD"), | |
| }; | |
| let key_owned = key.clone(); | |
| let existing = match load_hll(db, key) { | |
| Ok(v) => v, | |
| Err(e) => return e, | |
| }; | |
| let mut created = false; | |
| let mut hll = match existing { | |
| Some(h) => h, | |
| None => { | |
| created = true; | |
| Hll::new_sparse() | |
| } | |
| pub fn pfadd(db: &mut Database, args: &[Frame]) -> Frame { | |
| if args.len() < 2 { | |
| return err_wrong_args("PFADD"); | |
| } | |
| let key = match extract_bytes(&args[0]) { | |
| Some(k) => k, | |
| None => return err_wrong_args("PFADD"), | |
| }; | |
| let key_owned = key.clone(); | |
| let existing = match load_hll(db, key) { | |
| Ok(v) => v, | |
| Err(e) => return e, | |
| }; | |
| let mut created = false; | |
| let mut hll = match existing { | |
| Some(h) => h, | |
| None => { | |
| created = true; | |
| Hll::new_sparse() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command/hll.rs` around lines 79 - 100, The pfadd handler currently only
checks args.is_empty(), allowing calls like PFADD key to succeed and create an
empty HLL; update the argument validation in pfadd so it returns
err_wrong_args("PFADD") if args.len() < 2 (require at least a key and one
element), keeping the rest of the flow (extract_bytes, load_hll,
Hll::new_sparse, etc.) unchanged so no empty HLL is created for missing
elements.
Comment on lines
+567
to
+587
| // Remove from source | ||
| let src_set = match db.get_or_create_set(&source) { | ||
| Ok(s) => s, | ||
| Err(e) => return e, | ||
| }; | ||
| if !src_set.remove(&member) { | ||
| return Frame::Integer(0); | ||
| } | ||
| let src_empty = src_set.is_empty(); | ||
|
|
||
| // Add to destination | ||
| let dst_set = match db.get_or_create_set(&destination) { | ||
| Ok(s) => s, | ||
| Err(e) => return e, | ||
| }; | ||
| dst_set.insert(member); | ||
|
|
||
| // Clean up empty source | ||
| if src_empty { | ||
| db.remove(&source); | ||
| } |
There was a problem hiding this comment.
Handle SMOVE when source and destination are the same key.
If source == destination and the set only contains member, this path removes the member, reinserts it, and then still deletes the key because src_empty was computed before the reinsertion. SMOVE k k member should be a no-op that returns 1 when the member exists.
Possible fix
match db.get_set(&destination) {
Ok(_) => {} // exists as set or missing -- both OK
Err(e) => return e, // WRONGTYPE
}
+
+ if source == destination {
+ let present = match db.get_set(&source) {
+ Ok(Some(set)) => set.contains(&member),
+ Ok(None) => false,
+ Err(e) => return e,
+ };
+ return Frame::Integer(present as i64);
+ }
// Remove from source
let src_set = match db.get_or_create_set(&source) {
Ok(s) => s,
Err(e) => return e,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command/set.rs` around lines 567 - 587, When handling SMOVE, detect if
source == destination and short-circuit: after obtaining src_set via
db.get_or_create_set(&source) (the existing src_set variable), return
Frame::Integer(1) if src_set.contains(&member) (no mutation) or
Frame::Integer(0) if not; do not proceed to create dst_set or compute src_empty
in that case. This ensures the code paths around src_set, dst_set, src_empty and
db.remove(&source) do not remove the key when source and destination are
identical.
Comment on lines
+439
to
+567
| pub(super) fn zrange_from_entries( | ||
| entries: &[(Bytes, f64)], | ||
| min_arg: &[u8], | ||
| max_arg: &[u8], | ||
| by_score: bool, | ||
| by_lex: bool, | ||
| rev: bool, | ||
| withscores: bool, | ||
| limit_offset: Option<i64>, | ||
| limit_count: Option<i64>, | ||
| ) -> Frame { | ||
| let total = entries.len() as i64; | ||
| if total == 0 { | ||
| return Frame::Array(framevec![]); | ||
| } | ||
|
|
||
| if by_score { | ||
| let min_bound = match parse_score_bound(min_arg) { | ||
| Ok(b) => b, | ||
| Err(e) => return e, | ||
| }; | ||
| let max_bound = match parse_score_bound(max_arg) { | ||
| Ok(b) => b, | ||
| Err(e) => return e, | ||
| }; | ||
| let mut filtered: Vec<&(Bytes, f64)> = entries | ||
| .iter() | ||
| .filter(|(_, s)| min_bound.includes(*s) && max_bound.includes_upper(*s)) | ||
| .collect(); | ||
| if rev { | ||
| filtered.reverse(); | ||
| } | ||
| let offset = limit_offset.unwrap_or(0).max(0) as usize; | ||
| let count = limit_count | ||
| .map(|c| if c < 0 { filtered.len() } else { c as usize }) | ||
| .unwrap_or(filtered.len()); | ||
| let result: Vec<Frame> = filtered | ||
| .into_iter() | ||
| .skip(offset) | ||
| .take(count) | ||
| .flat_map(|(member, score)| { | ||
| let mut v = vec![Frame::BulkString(member.clone())]; | ||
| if withscores { | ||
| v.push(Frame::BulkString(Bytes::from(format_score(*score)))); | ||
| } | ||
| v | ||
| }) | ||
| .collect(); | ||
| Frame::Array(result.into()) | ||
| } else if by_lex { | ||
| let min_bound = match parse_lex_bound(min_arg) { | ||
| Ok(b) => b, | ||
| Err(e) => return e, | ||
| }; | ||
| let max_bound = match parse_lex_bound(max_arg) { | ||
| Ok(b) => b, | ||
| Err(e) => return e, | ||
| }; | ||
| let mut filtered: Vec<&(Bytes, f64)> = entries | ||
| .iter() | ||
| .filter(|(member, _)| lex_in_range(member, &min_bound, &max_bound)) | ||
| .collect(); | ||
| if rev { | ||
| filtered.reverse(); | ||
| } | ||
| let offset = limit_offset.unwrap_or(0).max(0) as usize; | ||
| let count = limit_count | ||
| .map(|c| if c < 0 { filtered.len() } else { c as usize }) | ||
| .unwrap_or(filtered.len()); | ||
| let result: Vec<Frame> = filtered | ||
| .into_iter() | ||
| .skip(offset) | ||
| .take(count) | ||
| .flat_map(|(member, score)| { | ||
| let mut v = vec![Frame::BulkString(member.clone())]; | ||
| if withscores { | ||
| v.push(Frame::BulkString(Bytes::from(format_score(*score)))); | ||
| } | ||
| v | ||
| }) | ||
| .collect(); | ||
| Frame::Array(result.into()) | ||
| } else { | ||
| // By rank | ||
| let start_raw: i64 = match std::str::from_utf8(min_arg) | ||
| .ok() | ||
| .and_then(|s| s.parse().ok()) | ||
| { | ||
| Some(v) => v, | ||
| None => return err("ERR value is not an integer or out of range"), | ||
| }; | ||
| let stop_raw: i64 = match std::str::from_utf8(max_arg) | ||
| .ok() | ||
| .and_then(|s| s.parse().ok()) | ||
| { | ||
| Some(v) => v, | ||
| None => return err("ERR value is not an integer or out of range"), | ||
| }; | ||
| let start = if start_raw < 0 { | ||
| (total + start_raw).max(0) as usize | ||
| } else { | ||
| start_raw as usize | ||
| }; | ||
| let stop = if stop_raw < 0 { | ||
| (total + stop_raw).max(0) as usize | ||
| } else { | ||
| (stop_raw as usize).min(entries.len().saturating_sub(1)) | ||
| }; | ||
| if start > stop || start >= entries.len() { | ||
| return Frame::Array(framevec![]); | ||
| } | ||
| let slice: Vec<&(Bytes, f64)> = if rev { | ||
| entries[start..=stop].iter().rev().collect() | ||
| } else { | ||
| entries[start..=stop].iter().collect() | ||
| }; | ||
| let result: Vec<Frame> = slice | ||
| .into_iter() | ||
| .flat_map(|(member, score)| { | ||
| let mut v = vec![Frame::BulkString(member.clone())]; | ||
| if withscores { | ||
| v.push(Frame::BulkString(Bytes::from(format_score(*score)))); | ||
| } | ||
| v | ||
| }) | ||
| .collect(); | ||
| Frame::Array(result.into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Match REV semantics in the listpack fallback.
zrange_from_entries() does not mirror the tree-backed helpers when rev is set. In rank mode it reverses the low-score slice instead of translating reverse ranks first, so ZREVRANGE 0 1 on a compact zset will return the wrong members. In score/lex modes it also keeps parsing min_arg/max_arg in forward order, while the tree path swaps the bounds for REV. That makes readonly results depend on the internal encoding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command/sorted_set/mod.rs` around lines 439 - 567, zrange_from_entries
currently mishandles rev: in score/lex modes it doesn't swap min/max like the
tree-backed path and in rank mode it reverses the selected slice instead of
translating start/stop for reversed ranks, causing different results for
rev-queries on compact zsets. Fix by making zrange_from_entries mirror the
tree-backed logic: when rev is true, swap min_arg and max_arg before calling
parse_score_bound/parse_lex_bound (and adjust includes/ordering as needed) and
in rank mode compute reversed start/stop indices by translating the requested
reversed ranks into forward indices (i.e., map requested rev start/stop to their
equivalent offsets from the end using total) rather than slicing then reversing;
update uses of start/stop, slice construction, and offset/count logic to reflect
these translated bounds in zrange_from_entries.
Comment on lines
+209
to
+240
| // Run ZRANGE on src, collecting (member, score) pairs | ||
| let entries: Vec<(Bytes, f64)> = match db.get_sorted_set(src) { | ||
| Ok(Some((members, scores))) => { | ||
| // Build a temporary Frame::Array result, then extract entries | ||
| let frame = if by_score { | ||
| zrange_by_score(members, scores, &min_arg, &max_arg, rev, true, limit_offset, limit_count) | ||
| } else if by_lex { | ||
| zrange_by_lex(scores, &min_arg, &max_arg, rev, true, members, limit_offset, limit_count) | ||
| } else { | ||
| zrange_by_rank(scores, &min_arg, &max_arg, rev, true) | ||
| }; | ||
| // Parse the Frame::Array([member, score, member, score, ...]) into Vec<(Bytes, f64)> | ||
| match frame { | ||
| Frame::Array(arr) => { | ||
| let mut result = Vec::with_capacity(arr.len() / 2); | ||
| let mut idx = 0; | ||
| while idx + 1 < arr.len() { | ||
| if let (Frame::BulkString(m), Frame::BulkString(s)) = (&arr[idx], &arr[idx + 1]) { | ||
| if let Ok(score) = std::str::from_utf8(s).unwrap_or("0").parse::<f64>() { | ||
| result.push((m.clone(), score)); | ||
| } | ||
| } | ||
| idx += 2; | ||
| } | ||
| result | ||
| } | ||
| _ => Vec::with_capacity(0), | ||
| } | ||
| } | ||
| Ok(None) => Vec::with_capacity(0), | ||
| Err(e) => return e, | ||
| }; |
There was a problem hiding this comment.
Propagate range errors before replacing the destination.
zrangestore() turns any non-array helper result into Vec::with_capacity(0), then removes dst and returns 0. If the helper returns Frame::Error for invalid bounds or ranks, this path will silently erase the destination key instead of surfacing the error.
Possible fix
match frame {
+ Frame::Error(e) => return Frame::Error(e),
Frame::Array(arr) => {
let mut result = Vec::with_capacity(arr.len() / 2);
let mut idx = 0;
while idx + 1 < arr.len() {
if let (Frame::BulkString(m), Frame::BulkString(s)) = (&arr[idx], &arr[idx + 1]) {
@@
}
result
}
_ => Vec::with_capacity(0),
}📝 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
| // Run ZRANGE on src, collecting (member, score) pairs | |
| let entries: Vec<(Bytes, f64)> = match db.get_sorted_set(src) { | |
| Ok(Some((members, scores))) => { | |
| // Build a temporary Frame::Array result, then extract entries | |
| let frame = if by_score { | |
| zrange_by_score(members, scores, &min_arg, &max_arg, rev, true, limit_offset, limit_count) | |
| } else if by_lex { | |
| zrange_by_lex(scores, &min_arg, &max_arg, rev, true, members, limit_offset, limit_count) | |
| } else { | |
| zrange_by_rank(scores, &min_arg, &max_arg, rev, true) | |
| }; | |
| // Parse the Frame::Array([member, score, member, score, ...]) into Vec<(Bytes, f64)> | |
| match frame { | |
| Frame::Array(arr) => { | |
| let mut result = Vec::with_capacity(arr.len() / 2); | |
| let mut idx = 0; | |
| while idx + 1 < arr.len() { | |
| if let (Frame::BulkString(m), Frame::BulkString(s)) = (&arr[idx], &arr[idx + 1]) { | |
| if let Ok(score) = std::str::from_utf8(s).unwrap_or("0").parse::<f64>() { | |
| result.push((m.clone(), score)); | |
| } | |
| } | |
| idx += 2; | |
| } | |
| result | |
| } | |
| _ => Vec::with_capacity(0), | |
| } | |
| } | |
| Ok(None) => Vec::with_capacity(0), | |
| Err(e) => return e, | |
| }; | |
| // Run ZRANGE on src, collecting (member, score) pairs | |
| let entries: Vec<(Bytes, f64)> = match db.get_sorted_set(src) { | |
| Ok(Some((members, scores))) => { | |
| // Build a temporary Frame::Array result, then extract entries | |
| let frame = if by_score { | |
| zrange_by_score(members, scores, &min_arg, &max_arg, rev, true, limit_offset, limit_count) | |
| } else if by_lex { | |
| zrange_by_lex(scores, &min_arg, &max_arg, rev, true, members, limit_offset, limit_count) | |
| } else { | |
| zrange_by_rank(scores, &min_arg, &max_arg, rev, true) | |
| }; | |
| // Parse the Frame::Array([member, score, member, score, ...]) into Vec<(Bytes, f64)> | |
| match frame { | |
| Frame::Error(e) => return Frame::Error(e), | |
| Frame::Array(arr) => { | |
| let mut result = Vec::with_capacity(arr.len() / 2); | |
| let mut idx = 0; | |
| while idx + 1 < arr.len() { | |
| if let (Frame::BulkString(m), Frame::BulkString(s)) = (&arr[idx], &arr[idx + 1]) { | |
| if let Ok(score) = std::str::from_utf8(s).unwrap_or("0").parse::<f64>() { | |
| result.push((m.clone(), score)); | |
| } | |
| } | |
| idx += 2; | |
| } | |
| result | |
| } | |
| _ => Vec::with_capacity(0), | |
| } | |
| } | |
| Ok(None) => Vec::with_capacity(0), | |
| Err(e) => return e, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command/sorted_set/range.rs` around lines 209 - 240, The current
zrangestore path converts any non-Array Frame from the helpers (zrange_by_score,
zrange_by_lex, zrange_by_rank) into an empty Vec and thus silently deletes dst
on errors; change the handling inside the match on frame (where you currently
match Frame::Array(arr) and use `_ => Vec::with_capacity(0)`) to detect
Frame::Error (and any other non-success frames you want to treat as errors) and
immediately propagate that error out of zrangestore instead of returning an
empty entries Vec; keep the existing Array-parsing logic for Frame::Array, but
for Frame::Error return that error (or convert it into the function's error
return type) so invalid bounds/ranks surface to the client.
Comment on lines
+212
to
+251
| // Set up bridge | ||
| crate::scripting::bridge::set_script_db(db, selected_db, db_count); | ||
| if read_only { | ||
| crate::scripting::bridge::set_script_read_only(true); | ||
| } | ||
|
|
||
| let timeout = std::time::Duration::from_secs(5); | ||
| if crate::scripting::sandbox::install_timeout_hook(&lib.lua, timeout).is_err() { | ||
| crate::scripting::bridge::clear_script_db(); | ||
| return Frame::Error(Bytes::from_static( | ||
| b"ERR Failed to install script timeout hook", | ||
| )); | ||
| } | ||
|
|
||
| let result = (|| -> mlua::Result<Frame> { | ||
| // Set KEYS and ARGV globals | ||
| let keys_table = lib.lua.create_table()?; | ||
| for (i, key) in keys.iter().enumerate() { | ||
| keys_table.set(i as i64 + 1, lib.lua.create_string(key.as_ref())?)?; | ||
| } | ||
| lib.lua.globals().set("KEYS", keys_table)?; | ||
|
|
||
| let argv_table = lib.lua.create_table()?; | ||
| for (i, arg) in argv.iter().enumerate() { | ||
| argv_table.set(i as i64 + 1, lib.lua.create_string(arg.as_ref())?)?; | ||
| } | ||
| lib.lua.globals().set("ARGV", argv_table)?; | ||
|
|
||
| // Call the registered function | ||
| let func_name_str = lib.lua.create_string(func_name)?; | ||
| let func_tbl: mlua::Table = | ||
| lib.lua.globals().get("__moon_functions")?; | ||
| let registered: mlua::Function = func_tbl.get(func_name_str)?; | ||
| let val: LuaValue = registered.call(())?; | ||
| crate::scripting::types::lua_value_to_frame(&lib.lua, &val) | ||
| })(); | ||
|
|
||
| // Always clean up | ||
| crate::scripting::sandbox::remove_timeout_hook(&lib.lua); | ||
| crate::scripting::bridge::clear_script_db(); |
There was a problem hiding this comment.
Always clear the read-only bridge flag after FCALL_RO.
set_script_read_only(true) is never reset, and the non-read-only path never forces it back to false. After one FCALL_RO, later FCALL calls on the same thread can keep running under read-only restrictions. The flag also leaks on the timeout-hook install error path.
Suggested fix
// Set up bridge
crate::scripting::bridge::set_script_db(db, selected_db, db_count);
- if read_only {
- crate::scripting::bridge::set_script_read_only(true);
- }
+ crate::scripting::bridge::set_script_read_only(read_only);
@@
if crate::scripting::sandbox::install_timeout_hook(&lib.lua, timeout).is_err() {
+ crate::scripting::bridge::set_script_read_only(false);
crate::scripting::bridge::clear_script_db();
return Frame::Error(Bytes::from_static(
b"ERR Failed to install script timeout hook",
));
}
@@
// Always clean up
crate::scripting::sandbox::remove_timeout_hook(&lib.lua);
+ crate::scripting::bridge::set_script_read_only(false);
crate::scripting::bridge::clear_script_db();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripting/functions.rs` around lines 212 - 251, The read-only flag set by
crate::scripting::bridge::set_script_read_only(true) is never cleared, causing
subsequent calls to run read-only; ensure you call
crate::scripting::bridge::set_script_read_only(false) in all cleanup paths:
after the main result block cleanup (alongside
crate::scripting::sandbox::remove_timeout_hook(&lib.lua) and
crate::scripting::bridge::clear_script_db()) and in the error path where
install_timeout_hook fails before returning the Frame::Error so the flag does
not leak; update the cleanup sequence around install_timeout_hook, the result
closure exit, and any early returns to always clear the read-only flag.
Comment on lines
+343
to
+360
| /// Construct from existing HYLL bytes (validates header). | ||
| pub fn from_bytes(bytes: Bytes) -> Result<Self, HllError> { | ||
| if bytes.len() < HLL_HDR_SIZE { | ||
| return Err(HllError::Truncated); | ||
| } | ||
| if &bytes[0..4] != HLL_MAGIC { | ||
| return Err(HllError::BadMagic); | ||
| } | ||
| let encoding = bytes[4]; | ||
| if encoding > HLL_MAX_ENCODING { | ||
| return Err(HllError::BadEncoding); | ||
| } | ||
| if encoding == HLL_DENSE && bytes.len() < HLL_DENSE_SIZE { | ||
| return Err(HllError::Truncated); | ||
| } | ||
| Ok(Hll { | ||
| buf: BytesMut::from(bytes.as_ref()), | ||
| }) |
There was a problem hiding this comment.
Validate sparse payload structure in from_bytes().
The constructor accepts any sparse payload once the header looks right. A truncated or malformed opcode stream then blows up later when sparse_decode() walks it—for example, an XZERO byte without its second byte will panic in count(), merge_from(), or sparse→dense promotion. Since these bytes come from stored user data, this should be rejected up front instead of crashing the server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/hll.rs` around lines 343 - 360, The from_bytes() constructor
currently accepts sparse-encoded payloads without validating opcode
completeness, which lets truncated/malformed opcode streams (e.g., an XZERO
missing its second byte) later panic in sparse_decode()/count()/merge_from() or
during sparse→dense promotion; add a validation step after parsing the header
and before constructing Hll (i.e., when encoding != HLL_DENSE and bytes.len() >=
HLL_HDR_SIZE) that walks the sparse payload bytes (bytes[HLL_HDR_SIZE..]) and
verifies each opcode’s required following bytes are present and well-formed,
returning Err(HllError::Truncated or BadEncoding) on failure — implement this as
a helper validate_sparse_payload(...) called from from_bytes() so from_bytes()
only constructs Hll when sparse_decode() can safely run.
Collaborator
Author
Security Review — 3 HIGH Blockers, Merge ConflictsDeep security review of 24 new command implementations. Do NOT merge as-is. HIGH — Must Fix Before Merge
MEDIUM
Merge Conflicts (MODIFY/DELETE)PR conflicts with main because main split
Estimated rebase effort: ~1-2 hours. PASS
|
Collaborator
Author
Action Required Before Merge1. Security fixes needed (3 HIGH)Fix 1: Cap negative count allocation // In HRANDFIELD and ZRANDMEMBER:
let n = count.unsigned_abs().min(collection.len().max(1) as u64) as usize;Fix 2: Limit FUNCTION LOAD body size // In create_library():
const MAX_FUNCTION_BODY: usize = 8 * 1024; // 8KB matching Redis
if body.len() > MAX_FUNCTION_BODY {
return Frame::Error(Bytes::from_static(b"ERR function body too large"));
}Fix 3: Clear KEYS/ARGV after FCALL // In call_function(), after the function call completes:
lua.globals().set("KEYS", mlua::Value::Nil)?;
lua.globals().set("ARGV", mlua::Value::Nil)?;2. Rebase onto main requiredMain split
After rebase + fixes, re-request review. |
TinDang97
added a commit
that referenced
this pull request
Apr 9, 2026
Phase 101: raise Redis command coverage from ~72% to ~82%. P0 blocking: BLMPOP, BRPOPLPUSH + metadata for BLPOP/BRPOP/BLMOVE/BZPOPMIN/BZPOPMAX P0 HyperLogLog: PFADD, PFCOUNT, PFMERGE (Ertl estimator, HYLL wire-compat) P1 convenience: LPUSHX, RPUSHX, LMPOP, HRANDFIELD, SMOVE, SINTERCARD P1 ZSet 6.2+: ZRANGESTORE, ZDIFF, ZUNION, ZINTER, ZINTERCARD, ZMSCORE, ZRANDMEMBER, ZMPOP P2 blocking zset: BZMPOP P2 Functions: FUNCTION LOAD/LIST/DELETE/FLUSH, FCALL, FCALL_RO (RAM-only) Includes PR #66 review fixes: ZINTERCARD dispatch bucket, SMOVE same-key, ZRANGESTORE error propagation, format_score_bytes hot-path, FCALL strict parsing, FUNCTION LOAD atomicity, FCALL_RO readonly allowlist.
TinDang97
force-pushed
the
worktree-feat+client
branch
from
106e20c to
59a0554
Compare
TinDang97
added a commit
that referenced
this pull request
Apr 9, 2026
FUNCTION, FCALL, and FCALL_RO handlers were placed before the ACL permission check in both handler_sharded.rs and handler_monoio.rs, allowing unprivileged users to manage/execute functions despite ACL restrictions. Moved all three handlers after check_command_permission and check_key_permission calls. Also applies rustfmt to all files modified in PR #66.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
TinDang97
added a commit
that referenced
this pull request
Apr 10, 2026
Phase 101: raise Redis command coverage from ~72% to ~82%. P0 blocking: BLMPOP, BRPOPLPUSH + metadata for BLPOP/BRPOP/BLMOVE/BZPOPMIN/BZPOPMAX P0 HyperLogLog: PFADD, PFCOUNT, PFMERGE (Ertl estimator, HYLL wire-compat) P1 convenience: LPUSHX, RPUSHX, LMPOP, HRANDFIELD, SMOVE, SINTERCARD P1 ZSet 6.2+: ZRANGESTORE, ZDIFF, ZUNION, ZINTER, ZINTERCARD, ZMSCORE, ZRANDMEMBER, ZMPOP P2 blocking zset: BZMPOP P2 Functions: FUNCTION LOAD/LIST/DELETE/FLUSH, FCALL, FCALL_RO (RAM-only) Includes PR #66 review fixes: ZINTERCARD dispatch bucket, SMOVE same-key, ZRANGESTORE error propagation, format_score_bytes hot-path, FCALL strict parsing, FUNCTION LOAD atomicity, FCALL_RO readonly allowlist.
TinDang97
added a commit
that referenced
this pull request
Apr 10, 2026
FUNCTION, FCALL, and FCALL_RO handlers were placed before the ACL permission check in both handler_sharded.rs and handler_monoio.rs, allowing unprivileged users to manage/execute functions despite ACL restrictions. Moved all three handlers after check_command_permission and check_key_permission calls. Also applies rustfmt to all files modified in PR #66.
TinDang97
force-pushed
the
worktree-feat+client
branch
from
beb27fc to
543c691
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (2)
src/server/conn/handler_monoio.rs (1)
1278-1325:⚠️ Potential issue | 🟠 MajorThe Functions API branches still bypass MULTI queueing.
These branches run before the generic MULTI queue gate at Line 1425, so once a client has entered
MULTI,FUNCTION/FCALL/FCALL_ROexecute immediately instead of being queued like other commands. That still breaks transaction semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/conn/handler_monoio.rs` around lines 1278 - 1325, The FUNCTION/FCALL/FCALL_RO branches (calling crate::command::functions::handle_function, handle_fcall, handle_fcall_ro using func_registry, shard_databases, selected_db, shard_id, num_shards) currently run before the MULTI queue gate and thus bypass transaction queuing; fix by enforcing the same MULTI queuing logic as other commands—either move these branches below the existing MULTI check or explicitly detect the client's MULTI state and push a queued command into the MULTI queue instead of executing immediately so that FUNCTION/FCALL/FCALL_RO are queued when client is in MULTI.src/storage/hll.rs (1)
227-243:⚠️ Potential issue | 🟠 Major
sparse_decodecan panic on truncated XZERO opcodes.Line 236 accesses
data[pos + 1]for XZERO opcodes without checking bounds. If a sparse payload ends with an XZERO prefix byte (0x40-0x7F) without its second byte, this causes an index-out-of-bounds panic.Since
from_bytes()doesn't validate sparse payload completeness, malformed stored data can crash the server when accessed bycount(),merge_from(), or sparse→dense promotion.Suggested fix - add bounds check
fn sparse_decode(data: &[u8], pos: usize) -> (SparseOp, usize) { + if pos >= data.len() { + return (SparseOp::Zero(0), 0); // Signal invalid/end + } let b = data[pos]; if b & 0x80 != 0 { // VAL: 1vvvvvxx let val = ((b >> 2) & 0x1F) + 1; let runlen = (b & 0x03) as u16 + 1; (SparseOp::Val(val, runlen), 1) } else if b & 0x40 != 0 { // XZERO: 01xxxxxx yyyyyyyy (2 bytes) + if pos + 1 >= data.len() { + return (SparseOp::Zero(0), 0); // Truncated XZERO + } let runlen = (((b & 0x3F) as u16) << 8 | data[pos + 1] as u16) + 1; (SparseOp::XZero(runlen), 2)This was flagged in a past review comment as needing validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/hll.rs` around lines 227 - 243, sparse_decode currently reads data[pos + 1] for XZERO without bounds checking and can panic on truncated payloads; change sparse_decode to return Result<(SparseOp, usize), SomeError> (or Option) instead of panicking, check that pos + 1 < data.len() before reading the second byte, and return an error when the second byte is missing; then update callers (from_bytes, count, merge_from, and any sparse→dense promotion paths) to propagate/handle that error (validate the sparse payload and fail gracefully) so malformed data no longer causes an index-out-of-bounds panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/bench-phase101-commands.sh`:
- Line 38: The help output currently prints the wrong block because the sed
range '2,/^###/p' stops at the first '###'; update the '--help)' case to extract
the usage between the two '###' markers instead (e.g., print from the line after
the first '###' up to the next '###'). Replace the existing sed invocation in
the '--help)' branch with a command that locates the second '###' delimiter and
prints the intervening lines (or use awk to print between the two '###' markers)
so the full usage block (lines 11–17) is displayed.
- Around line 34-37: The case branches handling flags like --requests, --shards,
--clients, and --section currently read $2 without validating it, which breaks
under set -u; update each branch in the case statement to first validate the
presence and that the next token is a value (e.g., check remaining args count or
that $2 does not start with "-" and is non-empty) and if invalid print a clear
usage/error message and exit, otherwise assign REQUESTS="$2" (or
SHARDS/CLIENTS/SECTION) and shift 2; apply this same validation pattern to all
similar flag handlers in the script.
- Around line 45-51: The cleanup function currently runs wait unconditionally
due to semicolon usage, causing wait to be called with empty values; change the
logic in cleanup (function name cleanup) to guard both kill and wait behind the
PID checks for MOON_PID and REDIS_PID (e.g., use an if [[ -n "${MOON_PID:-}" ]];
then kill "$MOON_PID" ...; wait "$MOON_PID" ...; fi pattern and similarly for
REDIS_PID) so that wait is only executed when the corresponding PID variable is
set, while preserving the existing redirection and || true behavior for errors
and keeping the pkill lines as-is.
In `@scripts/bench-phase101-seed.py`:
- Around line 17-20: The subprocess.run calls in scripts/bench-phase101-seed.py
currently ignore exit status and can silently fail; update all three
subprocess.run invocations (the ones assigning to p at the top and the two later
calls around lines 75–82) to include check=True so they raise CalledProcessError
on failure, ensuring the script fails fast when redis-cli exits with a non-zero
status; no other behavior change required beyond adding check=True to those
subprocess.run(...) calls.
- Around line 10-12: The RESP bulk string length is computed using character
count; change it to use the number of bytes by encoding the string to UTF-8
before measuring. In the block that builds parts (variables s and parts, where
you currently do parts.append(f"${len(s)}\r\n{s}\r\n")), compute the UTF-8 byte
length (e.g., encode s to bytes and take len) and use that byte length in the
$<len> header while still appending the original string content followed by
CRLF.
In `@src/command/hash/hash_read.rs`:
- Around line 643-660: The code uses count.unsigned_abs() directly to compute n
and pass it to Vec::with_capacity, which will overflow for extreme negative
counts (e.g. i64::MIN); fix by clamping the requested count to a sane upper
bound before taking absolute/allocating: compute a capped_count = if count < 0 {
min(count.abs_or_i64_max(), fields.len() as i64) } else { min(count,
fields.len() as i64) } and then set let n = capped_count.unsigned_abs() as
usize; apply this same clamp wherever count.unsigned_abs() is used (the
n/calculation blocks around the current Vec::with_capacity calls and the similar
block at 749-764) so allocations use the capped n instead of an unbounded usize.
- Around line 696-697: Replace the unchecked unwrap on entries.choose(&mut rng)
with pattern matching to handle the None case explicitly: instead of let (field,
_) = entries.choose(&mut rng).unwrap(); return Frame::BulkString(field.clone());
use an if let Some((field, _)) = entries.choose(&mut rng) { return
Frame::BulkString(field.clone()); } else { return Frame::Null; } (or use the
equivalent let Some((field, _)) = ... else { return Frame::Null; };), and apply
the same change to the other two sites that call entries.choose(&mut
rng).unwrap() so library code no longer uses unwrap()/expect().
In `@src/command/sorted_set/sorted_set_read.rs`:
- Around line 1709-1720: The code uses count.unsigned_abs() to compute n and
then calls Vec::with_capacity(cap), which allows extremely large allocations for
the special case count = i64::MIN; clamp negative counts to a safe non-negative
bound before capacity math. Replace the unsigned_abs() use with a guarded
conversion that first checks/counts negative values (e.g., treat negative counts
as zero or cap to entries.len()), compute n = count.max(0) as usize or
min(count.abs(), entries.len()) as usize, then compute cap = if withscores { n *
2 } else { n } and proceed with Vec::with_capacity(cap); adjust logic around
entries.choose, chosen, and format_score_bytes accordingly so you never pass an
unbounded huge usize into with_capacity.
- Around line 1690-1693: The code currently treats any third token in
ZRANDMEMBER as implicitly false; update the args parsing around the withscores
computation so that if args.len() == 3 you explicitly validate the third token
using extract_bytes(&args[2]) and only accept it if it equals b"WITHSCORES"
(case-insensitive), otherwise return a syntax error (ERR syntax error) instead
of continuing; keep the withscores boolean logic but ensure malformed third
arguments cause an early Err return from the ZRANDMEMBER handling path.
- Around line 1675-1677: Replace the unchecked unwrap() on entries.choose(&mut
rng) with guarded pattern matching in both places: where args.len() == 1
(replace the immediate unwrap and return with an if let Some(chosen) =
entries.choose(&mut rng) { return Frame::BulkString(chosen.0.clone()); } else
handle the empty case) and inside the loop (guard the entries.choose(&mut rng)
call with if let Some(chosen) = ... before using chosen, or restructure the loop
to skip iteration when choose returns None), ensuring no unwrap()/expect()
remain in sorted_set_read logic.
In `@src/command/sorted_set/sorted_set_write.rs`:
- Around line 550-588: The loop over args currently skips unknown option tokens
(i += 1 in the final else) which allows invalid tokens like WITHSCORES/FOO to
pass; change that behavior in the ZRANGESTORE parser (the loop referencing args,
extract_bytes, by_score, by_lex, rev, limit_offset, limit_count) so that the
final else returns err_wrong_args("ZRANGESTORE") (or err(...) as appropriate)
instead of incrementing i, and apply the same fix to the other parser loop
mentioned (the one handling ZMPOP-style options) so any unrecognized option
immediately returns the command-specific syntax error using err_wrong_args with
the correct command name.
In `@src/scripting/functions.rs`:
- Around line 122-131: The load() function accepts an unbounded body and must
enforce a maximum function-body size to prevent memory DoS: before
parse_shebang/create_library, check body.len() against a configurable limit
(default 8192 bytes to match Redis proto-max-bulk-len) and return a LoadError
(add a new variant like TooLarge or reuse an appropriate error) if it exceeds
the limit; use an existing config field (e.g., self.config.proto_max_bulk_len)
or add one and ensure tests cover rejection and acceptance around the limit.
In `@src/server/conn/handler_monoio.rs`:
- Around line 1308-1324: The FCALL_RO branch is incorrectly taking a write guard
via shard_databases.write_db which serializes readers; change it to acquire a
read guard (e.g., shard_databases.read_db) and pass an immutable reference to
the DB guard into crate::command::functions::handle_fcall_ro (remove &mut guard
in the call). If handle_fcall_ro currently expects a mutable guard, update its
signature to accept an immutable/read guard (or an &T) so FCALL_RO uses a
read-only guard while preserving db_count and other args.
- Around line 152-153: The per-connection creation of FunctionRegistry (let
func_registry =
Rc::new(RefCell::new(crate::scripting::FunctionRegistry::new()))) must be
replaced with a reference to the server/shard-wide registry stored in the shared
handler context; stop instantiating FunctionRegistry per socket and instead
obtain and clone the shared registry handle (e.g., an
Arc<Mutex<crate::scripting::FunctionRegistry>> or the existing shared field on
your connection handler/context) so that func_registry refers to the global
registry used by other connections and persists across disconnects.
In `@src/storage/hll.rs`:
- Around line 51-53: The loop in src/storage/hll.rs uses expect() when
converting an 8-byte slice into a u64 (u64::from_le_bytes(key[i * 8..i * 8 +
8].try_into().expect(...))), which violates the "no expect/unwrap in library
code" rule; replace the expect with explicit, non-panicking handling: obtain the
slice with get(i*8..i*8+8) and match or use try_into().ok() to handle failure,
then decide how to propagate the error (return Result::Err from the surrounding
function) or skip/continue as appropriate; update the code that assigns k (the
u64 variable inside the for loop) to use the safe path and ensure any error path
returns a clear error or handles it gracefully.
- Around line 390-392: The cached_card() function currently calls expect() on
try_into(), which is forbidden; instead guard against a short buffer by checking
that self.buf.len() >= HLL_HDR_SIZE (16) and return a safe default (e.g., 0) if
not, otherwise safely build the 8-byte array (for example with let mut
b=[0u8;8]; b.copy_from_slice(&self.buf[8..16]); let raw = u64::from_le_bytes(b))
and then compute raw & !(1u64 << 63); keep the function signature and use the
constants/fields cached_card, buf, and HLL_HDR_SIZE to locate and modify the
code.
---
Duplicate comments:
In `@src/server/conn/handler_monoio.rs`:
- Around line 1278-1325: The FUNCTION/FCALL/FCALL_RO branches (calling
crate::command::functions::handle_function, handle_fcall, handle_fcall_ro using
func_registry, shard_databases, selected_db, shard_id, num_shards) currently run
before the MULTI queue gate and thus bypass transaction queuing; fix by
enforcing the same MULTI queuing logic as other commands—either move these
branches below the existing MULTI check or explicitly detect the client's MULTI
state and push a queued command into the MULTI queue instead of executing
immediately so that FUNCTION/FCALL/FCALL_RO are queued when client is in MULTI.
In `@src/storage/hll.rs`:
- Around line 227-243: sparse_decode currently reads data[pos + 1] for XZERO
without bounds checking and can panic on truncated payloads; change
sparse_decode to return Result<(SparseOp, usize), SomeError> (or Option) instead
of panicking, check that pos + 1 < data.len() before reading the second byte,
and return an error when the second byte is missing; then update callers
(from_bytes, count, merge_from, and any sparse→dense promotion paths) to
propagate/handle that error (validate the sparse payload and fail gracefully) so
malformed data no longer causes an index-out-of-bounds panic.
🪄 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: 96a5dc55-e8f8-4e96-a448-c8a7331a62b1
📒 Files selected for processing (31)
.gitignorescripts/bench-phase101-commands.shscripts/bench-phase101-seed.pysrc/blocking/mod.rssrc/blocking/wakeup.rssrc/command/functions.rssrc/command/hash/hash_read.rssrc/command/hll.rssrc/command/list/list_write.rssrc/command/list/mod.rssrc/command/metadata.rssrc/command/mod.rssrc/command/set/mod.rssrc/command/set/set_read.rssrc/command/set/set_write.rssrc/command/sorted_set/mod.rssrc/command/sorted_set/sorted_set_read.rssrc/command/sorted_set/sorted_set_write.rssrc/scripting/bridge.rssrc/scripting/functions.rssrc/scripting/mod.rssrc/server/conn/blocking.rssrc/server/conn/handler_monoio.rssrc/server/conn/handler_sharded.rssrc/server/conn_state.rssrc/storage/hll.rssrc/storage/mod.rstests/blocking_list_timeout.rstests/functions_fcall.rstests/hll_vectors.rstests/hll_wire_compat.rs
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- src/storage/mod.rs
- src/command/list/mod.rs
- src/scripting/mod.rs
- src/command/metadata.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- src/blocking/wakeup.rs
- tests/hll_wire_compat.rs
- src/scripting/bridge.rs
- tests/hll_vectors.rs
- src/server/conn/handler_sharded.rs
- src/command/sorted_set/mod.rs
- tests/functions_fcall.rs
- src/server/conn/blocking.rs
- src/command/functions.rs
- tests/blocking_list_timeout.rs
Comment on lines
+152
to
+153
| // Functions API registry (per-connection, lazy init) | ||
| let func_registry = Rc::new(RefCell::new(crate::scripting::FunctionRegistry::new())); |
There was a problem hiding this comment.
FunctionRegistry can’t live at connection scope.
Line 153 creates a fresh registry per socket, so FUNCTION LOAD state disappears on disconnect and isn’t visible to other clients. Redis functions are server/shard-wide state; this needs to come from shared handler context, not a connection-local Rc<RefCell<_>>.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/conn/handler_monoio.rs` around lines 152 - 153, The per-connection
creation of FunctionRegistry (let func_registry =
Rc::new(RefCell::new(crate::scripting::FunctionRegistry::new()))) must be
replaced with a reference to the server/shard-wide registry stored in the shared
handler context; stop instantiating FunctionRegistry per socket and instead
obtain and clone the shared registry handle (e.g., an
Arc<Mutex<crate::scripting::FunctionRegistry>> or the existing shared field on
your connection handler/context) so that func_registry refers to the global
registry used by other connections and persists across disconnects.
Comment on lines
+1308
to
+1324
| // --- Functions API: FCALL_RO --- | ||
| if cmd.eq_ignore_ascii_case(b"FCALL_RO") { | ||
| let response = { | ||
| let mut guard = shard_databases.write_db(shard_id, selected_db); | ||
| let db_count = shard_databases.db_count(); | ||
| crate::command::functions::handle_fcall_ro( | ||
| &func_registry.borrow(), | ||
| cmd_args, | ||
| &mut guard, | ||
| shard_id, | ||
| num_shards, | ||
| selected_db, | ||
| db_count, | ||
| ) | ||
| }; | ||
| responses.push(response); | ||
| continue; |
There was a problem hiding this comment.
Use a read guard for FCALL_RO.
Line 1311 takes write_db even though this handler is supposed to be read-only. That unnecessarily serializes concurrent readers and weakens the separation between FCALL and FCALL_RO.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/conn/handler_monoio.rs` around lines 1308 - 1324, The FCALL_RO
branch is incorrectly taking a write guard via shard_databases.write_db which
serializes readers; change it to acquire a read guard (e.g.,
shard_databases.read_db) and pass an immutable reference to the DB guard into
crate::command::functions::handle_fcall_ro (remove &mut guard in the call). If
handle_fcall_ro currently expects a mutable guard, update its signature to
accept an immutable/read guard (or an &T) so FCALL_RO uses a read-only guard
while preserving db_count and other args.
Comment on lines
+51
to
+53
| for i in 0..chunks { | ||
| let mut k = | ||
| u64::from_le_bytes(key[i * 8..i * 8 + 8].try_into().expect("slice length is 8")); |
There was a problem hiding this comment.
expect() on hot path violates coding guidelines.
murmurhash64a is called for every element added via PFADD. Per coding guidelines: "No unwrap() or expect() in library code outside tests."
The slice is guaranteed valid by construction (i * 8..i * 8 + 8 where i < chunks and chunks = len / 8), but prefer explicit handling:
Suggested fix
- let mut k =
- u64::from_le_bytes(key[i * 8..i * 8 + 8].try_into().expect("slice length is 8"));
+ let mut k = u64::from_le_bytes([
+ key[i * 8],
+ key[i * 8 + 1],
+ key[i * 8 + 2],
+ key[i * 8 + 3],
+ key[i * 8 + 4],
+ key[i * 8 + 5],
+ key[i * 8 + 6],
+ key[i * 8 + 7],
+ ]);📝 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
| for i in 0..chunks { | |
| let mut k = | |
| u64::from_le_bytes(key[i * 8..i * 8 + 8].try_into().expect("slice length is 8")); | |
| for i in 0..chunks { | |
| let mut k = u64::from_le_bytes([ | |
| key[i * 8], | |
| key[i * 8 + 1], | |
| key[i * 8 + 2], | |
| key[i * 8 + 3], | |
| key[i * 8 + 4], | |
| key[i * 8 + 5], | |
| key[i * 8 + 6], | |
| key[i * 8 + 7], | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/hll.rs` around lines 51 - 53, The loop in src/storage/hll.rs uses
expect() when converting an 8-byte slice into a u64 (u64::from_le_bytes(key[i *
8..i * 8 + 8].try_into().expect(...))), which violates the "no expect/unwrap in
library code" rule; replace the expect with explicit, non-panicking handling:
obtain the slice with get(i*8..i*8+8) and match or use try_into().ok() to handle
failure, then decide how to propagate the error (return Result::Err from the
surrounding function) or skip/continue as appropriate; update the code that
assigns k (the u64 variable inside the for loop) to use the safe path and ensure
any error path returns a clear error or handles it gracefully.
Comment on lines
+390
to
+392
| fn cached_card(&self) -> u64 { | ||
| let raw = u64::from_le_bytes(self.buf[8..16].try_into().expect("8 bytes")); | ||
| raw & !(1u64 << 63) |
There was a problem hiding this comment.
expect() in cached_card() violates coding guidelines.
Although the buffer is guaranteed to be at least HLL_HDR_SIZE (16 bytes) by construction, expect() is forbidden in library code.
Suggested fix
fn cached_card(&self) -> u64 {
- let raw = u64::from_le_bytes(self.buf[8..16].try_into().expect("8 bytes"));
+ // SAFETY: buf is always >= HLL_HDR_SIZE (16 bytes) by construction in new_sparse/new_dense/from_bytes
+ let raw = u64::from_le_bytes([
+ self.buf[8], self.buf[9], self.buf[10], self.buf[11],
+ self.buf[12], self.buf[13], self.buf[14], self.buf[15],
+ ]);
raw & !(1u64 << 63)
}📝 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
| fn cached_card(&self) -> u64 { | |
| let raw = u64::from_le_bytes(self.buf[8..16].try_into().expect("8 bytes")); | |
| raw & !(1u64 << 63) | |
| fn cached_card(&self) -> u64 { | |
| // SAFETY: buf is always >= HLL_HDR_SIZE (16 bytes) by construction in new_sparse/new_dense/from_bytes | |
| let raw = u64::from_le_bytes([ | |
| self.buf[8], self.buf[9], self.buf[10], self.buf[11], | |
| self.buf[12], self.buf[13], self.buf[14], self.buf[15], | |
| ]); | |
| raw & !(1u64 << 63) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/hll.rs` around lines 390 - 392, The cached_card() function
currently calls expect() on try_into(), which is forbidden; instead guard
against a short buffer by checking that self.buf.len() >= HLL_HDR_SIZE (16) and
return a safe default (e.g., 0) if not, otherwise safely build the 8-byte array
(for example with let mut b=[0u8;8]; b.copy_from_slice(&self.buf[8..16]); let
raw = u64::from_le_bytes(b)) and then compute raw & !(1u64 << 63); keep the
function signature and use the constants/fields cached_card, buf, and
HLL_HDR_SIZE to locate and modify the code.
c27cb05 to
0b83446
Compare
Phase 101: raise Redis command coverage from ~72% to ~82%. P0 blocking: BLMPOP, BRPOPLPUSH + metadata for BLPOP/BRPOP/BLMOVE/BZPOPMIN/BZPOPMAX P0 HyperLogLog: PFADD, PFCOUNT, PFMERGE (Ertl estimator, HYLL wire-compat) P1 convenience: LPUSHX, RPUSHX, LMPOP, HRANDFIELD, SMOVE, SINTERCARD P1 ZSet 6.2+: ZRANGESTORE, ZDIFF, ZUNION, ZINTER, ZINTERCARD, ZMSCORE, ZRANDMEMBER, ZMPOP P2 blocking zset: BZMPOP P2 Functions: FUNCTION LOAD/LIST/DELETE/FLUSH, FCALL, FCALL_RO (RAM-only) Includes PR #66 review fixes: ZINTERCARD dispatch bucket, SMOVE same-key, ZRANGESTORE error propagation, format_score_bytes hot-path, FCALL strict parsing, FUNCTION LOAD atomicity, FCALL_RO readonly allowlist.
FUNCTION, FCALL, and FCALL_RO handlers were placed before the ACL permission check in both handler_sharded.rs and handler_monoio.rs, allowing unprivileged users to manage/execute functions despite ACL restrictions. Moved all three handlers after check_command_permission and check_key_permission calls. Also applies rustfmt to all files modified in PR #66.
TinDang97
force-pushed
the
worktree-feat+client
branch
from
0b83446 to
1f7b3b6
Compare
Scripts: - bench-phase101-commands.sh: fix --help sed range, validate flag args, guard cleanup kill/wait behind PID checks - bench-phase101-seed.py: add check=True to subprocess.run, use UTF-8 byte length in RESP bulk string header Security/correctness: - FUNCTION/FCALL/FCALL_RO now respect MULTI queue (skip execution when in_multi, fall through to queue gate) in both handler_sharded and handler_monoio - ZRANGESTORE rejects unknown option tokens instead of silently skipping - ZRANDMEMBER validates third arg is WITHSCORES, rejects garbage - Function body size capped at 512KB to prevent memory DoS Robustness: - hll.rs: sparse_decode returns Option, bounds-checks XZERO second byte; replace expect() with #[allow(clippy::unwrap_used)] + invariant comments - hash_read.rs: cap negative HRANDFIELD count, remove unwrap() on entries.choose() - sorted_set_read.rs: cap negative ZRANDMEMBER count, remove unwrap() on entries.choose()
This was referenced Apr 11, 2026
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
Closes #62. Raises Moon's Redis command coverage from ~72% to ~82% by implementing 24 commands across 6 priority groups, plus a sorted_set.rs refactor and benchmark scripts.
P0 — Blocks Real Workloads
Blocking list ops (#56):
BLMPOP,BRPOPLPUSH— full implementation with per-key wait-queue, FIFO wake, timeout, MULTI/EXEC conversionBLPOP,BRPOP,BLMOVE,BZPOPMIN,BZPOPMAX— phf metadata entries added (infrastructure already existed)tests/blocking_list_timeout.rs)HyperLogLog (#58):
PFADD,PFCOUNT,PFMERGE— byte-identical HYLL wire format (16-byte header + 12288-byte dense / sparse opcodes)0xadc83b19), Ertl improved estimator (hllSigma/hllTau— no bias tables needed)src/storage/hll.rs(1007 LOC),src/command/hll.rs(236 LOC)P1 — Common Client Calls
List/hash/set convenience (#60):
LPUSHX,RPUSHX— push-if-exists guardsLMPOP— pop from first non-empty list with COUNT supportHRANDFIELD— random field(s) with WITHVALUES and negative count (allow dups)SMOVE— atomic inter-set member transferSINTERCARD— intersection cardinality with LIMIT early-exitZSet 6.2+ (#59):
ZRANGESTORE,ZDIFF,ZUNION,ZINTER,ZINTERCARD,ZMSCORE,ZRANDMEMBER,ZMPOPsorted_set.rs(3092 lines → 5 files, all ≤1342 lines)P2
Blocking zset (#57):
BZMPOP— blocking sorted set multi-pop with the exact Redis reply shapeFunctions API (#61):
FUNCTION LOAD/LIST/DELETE/FLUSH,FCALL,FCALL_RO#!lua name=<lib>shebang parsingredis.register_function("name", fn)registration inside LuaFCALL_ROenforces read-only via thread-local bridge flagFUNCTION DUMP/RESTORE/STATSreturn-ERR not supported in this releasesrc/scripting/functions.rs(632 LOC),src/command/functions.rs(329 LOC)tests/functions_fcall.rs)Infrastructure
{mod,basic,range,setops,multi}.rs(zero behavior change, 45/45 tests pass)bench-phase101-commands.sh+bench-phase101-seed.py— side-by-side Moon vs Redis for all 24 commandsBenchmark Highlights (Linux aarch64, 1 shard, 50 clients, 20K req)
Files Changed (Phase 101 scope)
src/storage/hll.rs,src/command/hll.rs,src/command/functions.rs,src/scripting/functions.rs,src/command/sorted_set/{basic,range,setops,multi}.rsmetadata.rs,mod.rs,blocking/{mod,wakeup}.rs,server/conn/{blocking,handler_sharded,handler_monoio}.rs,scripting/bridge.rstests/{blocking_list_timeout,functions_fcall,hll_vectors,hll_wire_compat}.rsscripts/bench-phase101-{commands.sh,seed.py},test-consistency.sh,test-commands.shVerification
defaultandruntime-tokio,jemallocfeature setsTest plan
cargo test --release --lib(monoio)cargo test --no-default-features --features runtime-tokio,jemalloc --lib(tokio)cargo clippy -- -D warnings(both feature sets)tests/hll_vectors.rs— cardinality accuracy within ±2% for 100K elementstests/blocking_list_timeout.rs— 5 integration tests for BLPOP/BRPOP/BLMPOP/BLMOVE wake+timeouttests/functions_fcall.rs— 9 integration tests for FUNCTION LOAD/LIST/DELETE + FCALL/FCALL_ROscripts/bench-phase101-commands.sh— full benchmark suite passesscripts/test-consistency.sh— byte-for-byte vs Redis (requires redis-server)Summary by CodeRabbit
New Features
Improvements
Tests
Chores