fix(protocol): name protocol faults on the wire, and make MULTI atomic at queue time - #472
Merged
Merged
Conversation
…c at queue time
Two client-compatibility tasks measured against redis-server 8.6.1 with raw
sockets, since redis-cli cannot express a malformed frame or two commands in
one write.
PROTOCOL ERROR LIFETIME
A malformed frame used to close the connection silently AND discard the valid
frames that preceded it in the same read:
Err(_) => { break_outer = true; break; }
That arm threw away the parse reason (so a client could not tell a bad encoder
from a dropped network) and `batch`, which already held the parsed prefix — so
`PING\r\n*-9\r\n` in one write answered nothing at all.
* ParseError::Invalid gains a typed `kind` (ProtoFault) carrying Redis's
verbatim wire text; the detailed message is kept for logs and fuzz triage.
The two are deliberately different: "invalid bulk string length: -5"
localises a bug, "invalid bulk length" is what a driver author matches on.
* All three handlers now execute and flush the valid prefix, THEN write
`-ERR Protocol error: <reason>`, THEN close. One shared helper so they
cannot drift.
* RespCodec parks the fault in `last_fault` — the Decoder trait pins the
error type to io::Error, which cannot carry it.
* `*-9` (any negative multibulk count) is consumed and yields no frame,
matching measured Redis behaviour; it used to kill the connection.
* Inline quoting is a port of Redis's sdssplitargs. Moon had NO quote
support at all, so `SET k "a b"` became three arguments with literal quote
bytes, and an unterminated quote was silently accepted as part of a key.
memchr2 gates it, leaving the unquoted hot path untouched.
MULTI QUEUE SEMANTICS
Moon had no queue-time validation, so EXEC ran the half of a transaction that
happened to parse. Measured: `MULTI / NOSUCHCMD / SET k v / EXEC` left k SET
where Redis discards everything. That is data corruption, not a wording nit.
* `multi_dirty` on ConnectionState (Redis CLIENT_DIRTY_EXEC): a command that
could never run is refused at queue time and poisons the transaction; EXEC
answers -EXECABORT and executes nothing. DISCARD and RESET clear the flag —
a leaked flag would abort an innocent later transaction, which is worse
than the bug being fixed, so it has its own test.
* A MULTI QUEUE GATE now sits directly below each handler's ACL gate.
Redis executes exactly six commands during a transaction; everything else
queues. Moon decided this hundreds of lines further down, below the INFO /
CLIENT / WS / MQ / PUBLISH / SUBSCRIBE intercepts, so each of those ran for
real: `SUBSCRIBE ch` entered subscriber mode mid-MULTI and `INFO server`
returned a 3 KB dump where Redis returns +QUEUED. The gate is deliberately
BELOW the ACL check — this repo has already shipped one ACL bypass caused
by an intercept sitting above its permission check.
* Queue-time validation reads the same COMMAND_META table dispatch reads, so
a command cannot become queueable-but-undispatchable. A name containing '.'
that misses the table falls through to queueing: the table does not cover
every dotted extension family, and rejecting one would break a command that
works fine outside a transaction.
TESTS
tests/protocol_error_lifetime.rs — 8 tests, 8 green.
tests/multi_exec_queue_semantics.rs — 12 tests, 11 green.
me7 is #[ignore]d with its reason, not weakened: BLPOP-in-MULTI returns a Null
Bulk where Redis returns a Null Array, and `Frame` has no null-array variant at
all — `Frame::Null` serialises to `$-1` in RESP2, full stop. That affects BLPOP
everywhere, not just in transactions, and needs its own contract.
Refs: .add/tasks/protocol-error-lifetime, .add/tasks/multi-exec-queue-semantics
author: Tin Dang
author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
📝 WalkthroughWalkthroughThe PR adds typed Redis-compatible protocol faults, inline quoting support, valid-prefix processing before protocol-error closure, and queue-time MULTI validation. Dirty transactions now return ChangesProtocol and transaction behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RespCodec
participant ConnectionHandler
Client->>RespCodec: send valid frames and malformed frame
RespCodec->>ConnectionHandler: decoded prefix and retained ProtoFault
ConnectionHandler->>Client: flush valid replies
ConnectionHandler->>Client: send protocol error
ConnectionHandler->>Client: close connection
sequenceDiagram
participant Client
participant ConnectionHandler
participant TransactionState
Client->>ConnectionHandler: send MULTI
Client->>ConnectionHandler: send invalid queued command
ConnectionHandler->>TransactionState: set multi_dirty
ConnectionHandler->>Client: return queue-time error
Client->>ConnectionHandler: send EXEC
ConnectionHandler->>TransactionState: clear queue and dirty state
ConnectionHandler->>Client: return EXECABORT
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/conn/handler_monoio/mod.rs (3)
1883-1897: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the now-unreachable MULTI queue block.
The new gate at line 1490 consumes every frame while
conn.in_multiis true, except transaction-control commands. Transaction-control commands are consumed bytry_handle_multi_execat line 1794 and never fall through. Nothing between line 1490 and here setsconn.in_multiand continues to this point, so this block is dead code.
src/server/conn/handler_sharded/mod.rsdeleted its equivalent block in this PR. Keeping the monoio copy leaves two runtimes with different queueing code and invites divergence. The stale copy also lacks themulti_dirtyand blocking-conversion logic, so a future edit applied here would silently do nothing.♻️ Proposed removal
- // --- MULTI queue mode: queue commands when in transaction --- - if conn.in_multi { - // FT.* vector commands aren't wired through the txn execution - // path; reject them explicitly inside MULTI (matches - // handler_single) instead of an incidental later error. - if cmd.len() > 3 && cmd[..3].eq_ignore_ascii_case(b"FT.") { - responses.push(Frame::Error(Bytes::from_static( - b"ERR FT.* commands are not supported inside MULTI/EXEC", - ))); - continue; - } - conn.command_queue.push(frame); - responses.push(Frame::SimpleString(Bytes::from_static(b"QUEUED"))); - continue; - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/conn/handler_monoio/mod.rs` around lines 1883 - 1897, Remove the unreachable MULTI queue-handling block in the monoio connection handler, including its FT.* rejection, command queuing, QUEUED response, and continue path. Keep the surrounding transaction gate and align this runtime with the already-updated sharded handler.
3230-3264: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winA connection migration returns before the protocol-fault delivery check, so the fault is discarded. Both sharded handlers place the migration return ahead of the
proto_fault.take()block at the end of the batch.proto_faultlives on the handler's stack and is not carried inMigratedConnectionState, so the malformed bytes are consumed, no-ERR Protocol error: ...is sent, and the connection keeps serving on the target shard instead of closing.
src/server/conn/handler_monoio/mod.rs#L3230-L3264: add&& proto_fault.is_none()to the migration condition, or move the fault check above the migration block.src/server/conn/handler_sharded/mod.rs#L2600-L2619: apply the same guard to theconn.migration_targetcondition so the fault check at lines 2637-2642 always runs first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/conn/handler_monoio/mod.rs` around lines 3230 - 3264, Ensure protocol faults are delivered before connection migration: in src/server/conn/handler_monoio/mod.rs lines 3230-3264, prevent the migration branch from running when proto_fault is present; in src/server/conn/handler_sharded/mod.rs lines 2600-2619, apply the same guard to the conn.migration_target condition. Preserve the existing proto_fault handling so the error frame is sent and the connection closes instead of migrating.
797-799: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe subscriber-mode parse loop still closes silently on a protocol fault.
This arm returns
MonoioHandlerResult::Donewithout taking the fault or writing an error. The PR objective states that protocol faults produce a Redis-compatible-ERR Protocol error: <reason>before the close. A client that sends a malformed frame while subscribed still sees a bare disconnect.🐛 Proposed fix
- Err(_) => return (MonoioHandlerResult::Done, None), // parse error + Err(_) => { + // Same contract as the main parse loop: + // name the fault before closing. + if let Some(kind) = codec.take_last_fault() { + let data = bytes::Bytes::from( + super::util::proto_error_frame(kind), + ); + let (_wr, _b): (std::io::Result<usize>, bytes::Bytes) = + stream.write_all(data).await; + } + return (MonoioHandlerResult::Done, None); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/conn/handler_monoio/mod.rs` around lines 797 - 799, Update the subscriber-mode parse loop’s Err arm to capture the parse error, write a Redis-compatible “-ERR Protocol error: <reason>” response, then close with MonoioHandlerResult::Done. Preserve the Ok(None) need-more-data behavior and ensure the error reason comes from the parser instead of being discarded.
🧹 Nitpick comments (2)
tests/multi_exec_queue_semantics.rs (1)
394-403: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest dirty-state cleanup through
RESET.
me9resets a clean transaction. AddMULTI,NOSUCHCMD,RESET,MULTI,SET, andEXEC. Assert that the secondEXECdoes not returnEXECABORT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/multi_exec_queue_semantics.rs` around lines 394 - 403, Extend the transaction test around RESET to cover a dirty MULTI state: send MULTI, NOSUCHCMD, and RESET, then start a new MULTI, queue SET, and execute it. Assert the second EXEC succeeds without containing EXECABORT, verifying RESET clears the prior transaction error state.src/server/conn/handler_monoio/mod.rs (1)
1490-1490: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign
is_transaction_controlwith its documentation. The predicate exempts seven transaction commands, includingUNWATCH, plus five replication commands:REPLCONF,PSYNC,SYNC,REPLICAOF, andSLAVEOF. Add focused tests for these exemptions and for ordinary commands such asSELECT.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/conn/handler_monoio/mod.rs` at line 1490, Update is_transaction_control and its callers to classify all documented transaction-control exemptions, including UNWATCH and the replication commands REPLCONF, PSYNC, SYNC, REPLICAOF, and SLAVEOF, while keeping ordinary commands such as SELECT non-exempt. Add focused tests covering each exemption and the ordinary-command behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.add/tasks/multi-exec-queue-semantics/TASK.md:
- Around line 261-281: The test plan must explicitly cover transaction
dirty-state cleanup after poisoned RESET and dirty EXEC, not only poisoned
DISCARD. Extend the relevant multi-exec tests around me3/me9 with cases that
poison a transaction, issue RESET or EXEC as appropriate, then start a fresh
MULTI and verify it queues and executes successfully.
- Around line 261-281: Extend the transaction test plan with explicit wire-level
cases for INFO, CLIENT, SUBSCRIBE, PUBLISH, WS, and MQ, covering each command’s
pre-EXEC behavior, ACL enforcement, and intended result at EXEC. Anchor these
cases to the existing me* scenarios and ensure SUBSCRIBE coverage remains
consistent while adding the omitted intercepted commands; run them on both
runtimes at shards=1 and shards=4.
- Around line 108-110: Resolve the conflict between the MUST requirement for
BLPOP’s null-array response and the stated ignored gap: either add a distinct
null-array representation to Frame and implement RESP2 *-1 handling in the
relevant BLPOP paths, or remove the requirement and explicitly document the
waiver. Keep the task’s requirements, objectives, and scheduled fix consistent
across all repeated references.
- Around line 117-121: Update the unknown-command transaction scenario to use
Redis’s exact no-argument error form: either add arguments to NOSUCHCMD so the
“with args beginning with” suffix is valid, or expect ERR unknown command
'NOSUCHCMD' without that suffix. Apply the same correction to the me1 assertion
and the corresponding repeated scenario section.
- Around line 111-113: Add AOF and replication acceptance tests covering queued
SELECT behavior in the MULTI/EXEC flow. Verify literal queued SELECT commands
are never persisted, single-handler entries record txn_db, and sharded entries
use the database captured before each dispatch.
In @.add/tasks/protocol-error-lifetime/TASK.md:
- Around line 252-260: The protocol error lifetime test plan must add raw-socket
parser cases for rejected multibulk frames, including a non-numeric multibulk
count and an array element with a non-`$` type. Extend the Reject cases
alongside pe2–pe8 to assert each produces the expected error reply and closes
the connection; keep pe8 focused on ProtoFault::wire_text().
- Around line 143-147: Update the valid-prefix scenario to use a malformed frame
that reaches ParseError::Invalid, such as $abc\r\n, after PING\r\n. Preserve the
expected +PONG response and ensure the PING is not swallowed; retain *-9\r\n
only in the negative-count test.
- Around line 197-212: Update the ProtoFault wire-text API so ExpectedDollar(u8)
and UnknownType(u8) produce exact Redis-compatible messages containing the
offending byte; replace the &'static str-only contract with an owned or
otherwise suitable result and update all wire_text callers accordingly, while
preserving the existing static text for non-byte-carrying variants.
- Around line 257-258: Update the pe7 protocol-fault test plan to explicitly
cover handler_single and both runtimes, ensuring handler_monoio is exercised
under runtime-monoio and runtime-tokio coverage includes the intended handler
combinations. Align the cases with the four payloads currently implemented, or
add the two missing payload cases so all planned parity coverage is represented.
In `@src/protocol/frame.rs`:
- Around line 253-271: Correct the doc comment for ProtoFault::wire_text to
state that the two byte-carrying variants return the placeholder "Protocol
error: expected '$', got '?'", rather than claiming the method returns None.
Keep the existing signature and implementation unchanged.
- Around line 300-308: Update the `test_parse_error_invalid_display` test to
provide the required `kind` field when constructing `ParseError::Invalid`, while
preserving its existing `message` and `offset` assertions.
In `@src/protocol/inline.rs`:
- Around line 235-245: Update split_args_quoted’s unquoted-token parsing to
treat single and double quotes as quote-state transitions rather than appending
them, returning an unbalanced-quotes error when the quote is not closed and
merging quoted content into the current argument when it is. Reuse the Redis
whitespace predicate for token boundaries and post-quote checks, ensuring
whitespace such as vertical tab advances the parser instead of leaving it stuck.
In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 1493-1498: Mark the transaction dirty before rejecting unsupported
FT.* commands in all three handlers:
src/server/conn/handler_monoio/mod.rs#L1493-L1498,
src/server/conn/handler_sharded/mod.rs#L878-L883, and
src/server/conn/handler_single.rs#L1755-L1765. Update each FT.* queue-time
rejection, or centralize it through queue_time_rejection, so conn.multi_dirty is
set before the error frame is pushed and EXEC aborts without applying queued
commands.
In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 871-873: Align the transaction queueing logic around
is_transaction_control with the comment’s claim by exempting the replication
handshake commands handled by the REPLCONF and PSYNC intercepts from queuing
inside an open transaction. Ensure those commands continue to reach their
intercept handlers while other non-transaction-control commands remain queued.
In `@src/server/conn/shared.rs`:
- Around line 1105-1112: Update the arity calculation in the command validation
block to use a non-narrowing integer type capable of representing the parser’s
maximum argument count, avoiding the i16 cast and overflow at large inputs. Keep
the existing exact-arity and minimum-arity comparisons with meta.arity correct
by using compatible types or safe conversion.
- Around line 1004-1019: Remove UNWATCH from the CONTROL list in
is_transaction_control, and update handler_single.rs so its pre-queue handling
no longer consumes UNWATCH; when queued inside MULTI, return OK while preserving
watch-version validation before EXEC. Add runtime-specific two-connection
coverage in tests/multi_exec_queue_semantics.rs around lines 263-270 asserting
that a conflicting write causes EXEC to return a null array.
---
Outside diff comments:
In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 1883-1897: Remove the unreachable MULTI queue-handling block in
the monoio connection handler, including its FT.* rejection, command queuing,
QUEUED response, and continue path. Keep the surrounding transaction gate and
align this runtime with the already-updated sharded handler.
- Around line 3230-3264: Ensure protocol faults are delivered before connection
migration: in src/server/conn/handler_monoio/mod.rs lines 3230-3264, prevent the
migration branch from running when proto_fault is present; in
src/server/conn/handler_sharded/mod.rs lines 2600-2619, apply the same guard to
the conn.migration_target condition. Preserve the existing proto_fault handling
so the error frame is sent and the connection closes instead of migrating.
- Around line 797-799: Update the subscriber-mode parse loop’s Err arm to
capture the parse error, write a Redis-compatible “-ERR Protocol error:
<reason>” response, then close with MonoioHandlerResult::Done. Preserve the
Ok(None) need-more-data behavior and ensure the error reason comes from the
parser instead of being discarded.
---
Nitpick comments:
In `@src/server/conn/handler_monoio/mod.rs`:
- Line 1490: Update is_transaction_control and its callers to classify all
documented transaction-control exemptions, including UNWATCH and the replication
commands REPLCONF, PSYNC, SYNC, REPLICAOF, and SLAVEOF, while keeping ordinary
commands such as SELECT non-exempt. Add focused tests covering each exemption
and the ordinary-command behavior.
In `@tests/multi_exec_queue_semantics.rs`:
- Around line 394-403: Extend the transaction test around RESET to cover a dirty
MULTI state: send MULTI, NOSUCHCMD, and RESET, then start a new MULTI, queue
SET, and execute it. Assert the second EXEC succeeds without containing
EXECABORT, verifying RESET clears the prior transaction error state.
🪄 Autofix
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 Plus
Run ID: f12460e6-066c-40bc-ab33-ae8982c99e9d
📒 Files selected for processing (19)
.add/state.json.add/tasks/multi-exec-queue-semantics/TASK.md.add/tasks/protocol-error-lifetime/TASK.mdCHANGELOG.mdsrc/protocol/frame.rssrc/protocol/inline.rssrc/protocol/mod.rssrc/protocol/parse.rssrc/server/codec.rssrc/server/conn/core.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_monoio/write.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_sharded/write.rssrc/server/conn/handler_single.rssrc/server/conn/shared.rssrc/server/conn/util.rstests/multi_exec_queue_semantics.rstests/protocol_error_lifetime.rs
Comment on lines
+111
to
+113
| - Behavior already matching Redis stays matching: nested `MULTI`, `EXEC`/`DISCARD` without | ||
| `MULTI`, `DISCARD` inside `MULTI`, `SELECT` inside `MULTI`, empty `MULTI`/`EXEC`, `RESET` | ||
| inside `MULTI`, and a `WRONGTYPE` at execution time (a runtime error does NOT abort the block). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect persisted-write classification and per-dispatch database capture.
rg -n -C 4 'is_persisted_write|is_write|execute_transaction|selected_db|txn_db|aof' src tests --glob '*.rs'Repository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate transaction files ---'
git ls-files 'src/server/conn/*handler*' 'src/server/conn/shared.rs' 'src/metadata*' 'src/command*' | head -200
printf '%s\n' '--- executor definitions and call sites ---'
rg -n -C 8 --glob '*.rs' \
'fn execute_transaction|execute_transaction_sharded|execute_transaction\(' \
src/server src/command src/metadata.rs src/metadata 2>/dev/null | head -800
printf '%s\n' '--- persistence classification definitions/usages ---'
rg -n -C 6 --glob '*.rs' \
'is_persisted_write|pub fn is_write|fn is_write|metadata::is_write|metadata::is_persisted_write' \
src | head -800Repository: pilotspace/moon
Length of output: 25396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- shared executor: single-db path ---'
sed -n '145,250p' src/server/conn/shared.rs
printf '%s\n' '--- shared executor: sharded path ---'
sed -n '251,430p' src/server/conn/shared.rs
printf '%s\n' '--- command persistence classification ---'
rg -n -C 10 'is_persisted_write|is_write|SELECT|select' src/command/metadata.rs src/command/transaction.rs | head -500
printf '%s\n' '--- single-handler transaction persistence ---'
sed -n '1425,1495p' src/server/conn/handler_single.rs
rg -n -C 8 'txn_aof_entries|persist_txn_aof|record_local_write_db|aof_entries' src/server/conn/handler_single.rs src/server/conn/handler_sharded/write.rs src/server/conn/handler_monoio/write.rs | head -600Repository: pilotspace/moon
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- task test/build sections ---'
sed -n '250,315p' .add/tasks/multi-exec-queue-semantics/TASK.md
printf '%s\n' '--- single transaction AOF conversion and replication ---'
sed -n '1485,1575p' src/server/conn/handler_single.rs
rg -n -C 10 'txn_aof_entries|persist_txn_aof|record_local_write_db|entry_db|txn_db' \
src/server/conn/handler_single.rs src/server/conn/handler_sharded/write.rs \
src/server/conn/handler_monoio/write.rs src/server/conn/shared.rs | head -700
printf '%s\n' '--- transaction-related tests and task references ---'
rg -n -i -C 3 'multi|exec|select|aof|replicat|database|dirty|reset' \
tests .add/tasks/multi-exec-queue-semantics/TASK.md \
--glob '*.rs' --glob '*.md' 2>/dev/null | head -1000
printf '%s\n' '--- read-only structural verifier ---'
python3 - <<'PY'
from pathlib import Path
shared = Path("src/server/conn/shared.rs").read_text()
single = Path("src/server/conn/handler_single.rs").read_text()
task = Path(".add/tasks/multi-exec-queue-semantics/TASK.md").read_text()
checks = {
"single executor uses persisted-write classification":
"let is_write = metadata::is_persisted_write(cmd);" in shared,
"sharded executor uses persisted-write classification":
"if crate::command::metadata::is_persisted_write(cmd)" in shared,
"single handler captures transaction db before execution":
"let txn_db = conn.selected_db;" in single,
"sharded executor captures db before dispatch":
"let entry_db = selected;" in shared and
"with_shard_db(selected" in shared,
"metadata excludes SELECT from persisted writes":
"is_write(cmd) && !cmd.eq_ignore_ascii_case(b\"SELECT\")" in
Path("src/command/metadata.rs").read_text(),
}
for name, ok in checks.items():
print(f"{'PASS' if ok else 'FAIL'}: {name}")
print("--- task coverage keywords ---")
for term in ("AOF", "replication", "persisted", "selected database", "database context"):
print(f"{term}: {term.lower() in task.lower()}")
PYRepository: pilotspace/moon
Length of output: 50372
Add AOF and replication acceptance tests for queued SELECT.
Assert that no literal queued SELECT is persisted. Assert that single-handler entries use txn_db, while sharded entries use the database captured before each dispatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/multi-exec-queue-semantics/TASK.md around lines 111 - 113, Add
AOF and replication acceptance tests covering queued SELECT behavior in the
MULTI/EXEC flow. Verify literal queued SELECT commands are never persisted,
single-handler entries record txn_db, and sharded entries use the database
captured before each dispatch.
Source: Learnings
Comment on lines
+117
to
+121
| - unknown command queued -> "ERR unknown command '<name>', with args beginning with: ..." | ||
| - wrong arity queued -> "ERR wrong number of arguments for '<name>' command" | ||
| - SUBSCRIBE/UNSUBSCRIBE/PSUBSCRIBE/PUNSUBSCRIBE queued -> "ERR <NAME> is not allowed in transactions" | ||
| - WATCH queued -> "ERR WATCH inside MULTI is not allowed" | ||
| - EXEC after any of the above -> "EXECABORT Transaction discarded because of previous errors." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the correct no-argument unknown-command reply.
The scenario sends NOSUCHCMD without arguments but expects with args beginning with:. The protocol task states that Redis omits this suffix when no arguments exist. Either send NOSUCHCMD a b in the scenario, or expect ERR unknown command 'NOSUCHCMD'. Make me1 assert the same exact form.
Also applies to: 154-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/multi-exec-queue-semantics/TASK.md around lines 117 - 121, Update
the unknown-command transaction scenario to use Redis’s exact no-argument error
form: either add arguments to NOSUCHCMD so the “with args beginning with” suffix
is valid, or expect ERR unknown command 'NOSUCHCMD' without that suffix. Apply
the same correction to the me1 assertion and the corresponding repeated scenario
section.
Comment on lines
+261
to
+281
| Coverage target: every Must and every Reject above has one test; suite runs on BOTH runtimes | ||
| and at shards=1 AND shards=4 (the §1 ⚠ is about dispatch-path coverage, which shard count changes). | ||
|
|
||
| Plan (one test per scenario, asserting wire behavior — never internals): | ||
| <test_plan> | ||
| - test_<scenario>: arrange <Given> / act <When> / assert <Then> + assert <unchanged> | ||
| - me1_unknown_command_aborts_and_applies_nothing: MULTI / NOSUCHCMD / SET k v / EXEC; | ||
| assert the unknown-command error, "+QUEUED" for SET, "EXECABORT" for EXEC, and GET k is nil | ||
| - me2_wrong_arity_aborts: MULTI / GET / EXEC; assert arity error then EXECABORT | ||
| - me3_discard_clears_dirty: poison, DISCARD, then a clean MULTI/SET/EXEC applies | ||
| - me4_subscribe_refused_not_executed: assert the error AND that a following PING still answers | ||
| "+PONG" (a real SUBSCRIBE would have put the connection in subscriber mode) | ||
| - me5_watch_inside_multi_refused: assert the exact Redis text | ||
| - me6_bad_frame_inside_multi_names_itself: assert the protocol error arrives BEFORE the close | ||
| - me7_blpop_in_multi_is_null_array: assert the RESP2 bytes are "*-1", not "$-1" | ||
| - me8_wrongtype_does_not_abort: assert the 2-element array and that the sibling SET applied | ||
| - me9_matching_behavior_unchanged: the seven already-correct cases, byte-compared | ||
| - me10_every_command_meta_entry_is_queueable: walk COMMAND_META; for each non-exempt entry, | ||
| queue it with its minimum valid arity and assert "+QUEUED" — this is the §1 ⚠ mitigation, | ||
| the test that catches a command becoming unusable in transactions | ||
| - me11_one_command_per_dispatch_path_queues: a write (dispatch), a read (dispatch_read), and | ||
| an inline-fast-path command each queue successfully at shards=1 and shards=4 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test dirty-state cleanup for RESET and dirty EXEC.
The contract requires the dirty flag to clear after EXEC, DISCARD, and RESET. me3 covers only poisoned DISCARD. The RESET case in me9 does not prove cleanup after a poisoned transaction. Add poisoned RESET and dirty EXEC cases followed by a fresh MULTI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/multi-exec-queue-semantics/TASK.md around lines 261 - 281, The
test plan must explicitly cover transaction dirty-state cleanup after poisoned
RESET and dirty EXEC, not only poisoned DISCARD. Extend the relevant multi-exec
tests around me3/me9 with cases that poison a transaction, issue RESET or EXEC
as appropriate, then start a fresh MULTI and verify it queues and executes
successfully.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add tests for every intercepted command named by the PR.
The PR objectives and CHANGELOG.md name INFO, CLIENT, SUBSCRIBE, PUBLISH, WS, and MQ. The plan explicitly tests only SUBSCRIBE, WATCH, and generic metadata entries. Add cases that verify each intercepted command does not execute before EXEC, preserves ACL checks, and produces its intended result at EXEC.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/multi-exec-queue-semantics/TASK.md around lines 261 - 281, Extend
the transaction test plan with explicit wire-level cases for INFO, CLIENT,
SUBSCRIBE, PUBLISH, WS, and MQ, covering each command’s pre-EXEC behavior, ACL
enforcement, and intended result at EXEC. Anchor these cases to the existing me*
scenarios and ensure SUBSCRIBE coverage remains consistent while adding the
omitted intercepted commands; run them on both runtimes at shards=1 and
shards=4.
Comment on lines
+143
to
+147
| Scenario: the valid prefix is answered before the fault # Must 2 | ||
| Given a fresh connection | ||
| When it writes "PING\r\n*-9\r\n" in ONE write | ||
| Then it receives "+PONG" | ||
| And the PING is not swallowed # Moon answers nothing at all today |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a real protocol fault for the valid-prefix scenario.
*-9\r\n is explicitly ignored and never enters ParseError::Invalid. Therefore PING\r\n*-9\r\n cannot verify that a valid frame is flushed before an invalid frame closes the connection. Use PING\r\n$abc\r\n or another malformed frame for this scenario. Keep *-9\r\n only in the negative-count test.
Also applies to: 251-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.add/tasks/protocol-error-lifetime/TASK.md around lines 143 - 147, Update
the valid-prefix scenario to use a malformed frame that reaches
ParseError::Invalid, such as $abc\r\n, after PING\r\n. Preserve the expected
+PONG response and ensure the PING is not swallowed; retain *-9\r\n only in the
negative-count test.
Comment on lines
+235
to
+245
| None => { | ||
| while i < line.len() { | ||
| match line[i] { | ||
| b' ' | b'\t' | b'\n' | b'\r' | 0x0b | 0x0c => break, | ||
| c => { | ||
| current.push(c); | ||
| i += 1; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Redis sdssplitargs inline command parsing behavior for an unquoted token containing a double quote
💡 Result:
In Redis, the sdssplitargs function is used to parse inline command arguments. Its behavior regarding tokens containing double quotes is strict to prevent malformed input [1][2][3]. If a token is unquoted (i.e., not wrapped in double or single quotes), it must not contain double quotes [4][2]. Specifically, sdssplitargs enforces a rule where a closing quote (either double or single) must be immediately followed by a whitespace character or be at the very end of the input string [1][2][3]. If a double quote appears within what is otherwise an unquoted token—such as "foo"bar—the parser will interpret the first double quote as a closing quote and then encounter the subsequent characters (like 'b' or 'a') as a violation of this rule [1][2][3]. This violation triggers an error, and the function returns NULL [1][2][3]. The documentation explicitly cites "foo"bar as an example of an invalid format that causes the parser to fail [1][2][3]. This design ensures that quoted and unquoted segments are not ambiguously mixed, maintaining consistent parsing logic [4].
Citations:
- 1: https://github.com/redis/redis/blob/320fa02/deps/hiredis/sds.c
- 2: https://github.com/antirez/redis/blob/unstable/src/sds.c
- 3: https://github.com/redis/redis/blob/6.0.8/src/sds.c
- 4: Allow mixing quoted and unquoted inline args valkey-io/valkey#2098
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- inline.rs relevant sections ---'
sed -n '50,90p;110,245p' src/protocol/inline.rs
printf '%s\n' '--- parser symbols and tests ---'
rg -n -C 3 'split_args_quoted|parse_inline|unbalanced|0x0b|0x0c|sdssplitargs' src/protocol tests 2>/dev/null || true
printf '%s\n' '--- tracked files containing inline parser tests ---'
git ls-files | rg '(^|/)(inline|protocol).*(_test|test|tests)?|inline\.rs$' || trueRepository: pilotspace/moon
Length of output: 27423
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("src/protocol/inline.rs").read_text()
for needle in ("fn split_args_quoted", "fn split_args", "parse_frame_zerocopy"):
pos = p.find(needle)
print(f"{needle}: {pos}")
PY
printf '%s\n' '--- Redis reference implementation excerpt ---'
if command -v curl >/dev/null 2>&1; then
curl -L --fail --silent --show-error \
https://raw.githubusercontent.com/redis/redis/unstable/deps/hiredis/sds.c |
rg -n -C 18 'sdssplitargs|inq|insq' | head -n 180
else
printf '%s\n' 'curl unavailable'
fiRepository: pilotspace/moon
Length of output: 6688
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- parse_inline and token finalization ---'
sed -n '1,115p;230,280p' src/protocol/inline.rs
printf '%s\n' '--- inline parser tests after line 440 ---'
sed -n '280,520p' src/protocol/inline.rsRepository: pilotspace/moon
Length of output: 12010
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from enum import Enum
def redis_split(line: bytes):
out, i = [], 0
whitespace = b" \n\r\t\v\f"
while True:
while i < len(line) and line[i] in whitespace:
i += 1
if i >= len(line):
return out
cur = bytearray()
inq = insq = False
while True:
c = line[i] if i < len(line) else 0
if inq:
if c == 92 and i + 1 < len(line) and line[i+1] == ord("x") and i + 3 < len(line):
# Representative inputs below do not use hex escapes.
cur.extend(line[i+1:i+2]); i += 2
elif c == 92 and i + 1 < len(line):
cur.append(line[i+1]); i += 2
elif c == ord('"'):
if i + 1 < len(line) and line[i+1] not in whitespace:
return None
inq = False; i += 1; break
elif i >= len(line):
return None
else:
cur.append(c); i += 1
elif insq:
if c == 92 and i + 1 < len(line) and line[i+1] == ord("'"):
cur.append(ord("'")); i += 2
elif c == ord("'"):
if i + 1 < len(line) and line[i+1] not in whitespace:
return None
insq = False; i += 1; break
elif i >= len(line):
return None
else:
cur.append(c); i += 1
else:
if i >= len(line) or line[i] in whitespace:
break
if c == ord('"'):
inq = True; i += 1
elif c == ord("'"):
insq = True; i += 1
else:
cur.append(c); i += 1
out.append(bytes(cur))
def moon_split(line: bytes):
# This models split_args_quoted's relevant branches, including its
# space/tab-only leading and closing checks.
out, i = [], 0
while True:
while i < len(line) and line[i] in b" \t":
i += 1
if i >= len(line):
return out
cur = bytearray()
quote = line[i] if line[i] in (ord('"'), ord("'")) else None
if quote is not None:
i += 1
if quote == ord('"') or quote == ord("'"):
while True:
if i >= len(line): return None
c = line[i]
if c == quote:
if i + 1 < len(line) and line[i+1] not in b" \t":
return None
i += 1; break
cur.append(c); i += 1
else:
while i < len(line) and line[i] not in b" \t\n\r\v\f":
cur.append(line[i]); i += 1
out.append(bytes(cur))
cases = [b'SET k v"x', b'SET k v"x foo"', b'SET k "v"x', b'SET k "v" x',
b'SET\vkey value', b'SET key\v"value"', b'SET key "value"\vnext']
for line in cases:
print(line, "redis=", redis_split(line), "moon=", moon_split(line))
PYRepository: pilotspace/moon
Length of output: 145
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
WS_REDIS = set(b" \n\r\t\v\f")
WS_MOON_LEADING = set(b" \t")
WS_MOON_UNQUOTED = set(b" \t\n\r\v\f")
def redis_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_REDIS:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
inq = insq = False
while True:
steps += 1
if steps > limit:
return ("stuck", i)
c = line[i] if i < len(line) else 0
if inq:
if c == ord('"'):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
inq = False; i += 1; break
cur.append(c); i += 1
elif insq:
if c == ord("'"):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
insq = False; i += 1; break
cur.append(c); i += 1
else:
if i >= len(line) or c in WS_REDIS:
break
if c == ord('"'):
inq = True; i += 1
elif c == ord("'"):
insq = True; i += 1
else:
cur.append(c); i += 1
if inq or insq:
return ("error", "unbalanced quote")
out.append(bytes(cur))
def moon_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_MOON_LEADING:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
quote = line[i] if line[i] in (ord('"'), ord("'")) else None
if quote is not None:
i += 1
while True:
steps += 1
if steps > limit:
return ("stuck", i)
if quote is not None:
if i >= len(line):
return ("error", "unbalanced quote")
c = line[i]
if c == quote:
if i + 1 < len(line) and line[i + 1] not in WS_MOON_LEADING:
return ("error", "closing quote")
i += 1; break
cur.append(c); i += 1
else:
if i >= len(line) or line[i] in WS_MOON_UNQUOTED:
break
cur.append(line[i]); i += 1
out.append(bytes(cur))
cases = [
b'SET k v"x',
b'SET k v"x foo"',
b'SET k "v"x',
b'SET k "v" x',
b'SET key\v"value"',
b'SET key\n"value"',
b'SET key "value"\vnext',
]
for case in cases:
print(repr(case))
print(" redis:", redis_split(case))
print(" moon :", moon_split(case))
PYRepository: pilotspace/moon
Length of output: 820
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
WS_REDIS = set(b" \n\r\t\v\f")
WS_MOON_LEADING = set(b" \t")
WS_MOON_UNQUOTED = set(b" \t\n\r\v\f")
def redis_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_REDIS:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
inq = insq = False
while True:
steps += 1
if steps > limit:
return ("stuck", i)
if i >= len(line):
if inq or insq:
return ("error", "unbalanced quote")
break
c = line[i]
if inq:
if c == ord('"'):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
inq = False; i += 1; break
cur.append(c); i += 1
elif insq:
if c == ord("'"):
if i + 1 < len(line) and line[i + 1] not in WS_REDIS:
return ("error", "closing quote")
insq = False; i += 1; break
cur.append(c); i += 1
else:
if c in WS_REDIS:
break
if c == ord('"'):
inq = True; i += 1
elif c == ord("'"):
insq = True; i += 1
else:
cur.append(c); i += 1
out.append(bytes(cur))
def moon_split(line, limit=1000):
out, i, steps = [], 0, 0
while True:
while i < len(line) and line[i] in WS_MOON_LEADING:
i += 1
if i >= len(line):
return ("ok", out)
cur = bytearray()
quote = line[i] if line[i] in (ord('"'), ord("'")) else None
if quote is not None:
i += 1
while True:
steps += 1
if steps > limit:
return ("stuck", i)
if quote is not None:
if i >= len(line):
return ("error", "unbalanced quote")
c = line[i]
if c == quote:
if i + 1 < len(line) and line[i + 1] not in WS_MOON_LEADING:
return ("error", "closing quote")
i += 1; break
cur.append(c); i += 1
else:
if i >= len(line) or line[i] in WS_MOON_UNQUOTED:
break
cur.append(line[i]); i += 1
out.append(bytes(cur))
for case in (
b'SET k v"x',
b'SET k v"x foo"',
b'SET k "v"x',
b'SET k "v" x',
b'SET key\v"value"',
b'SET key\n"value"',
b'SET key "value"\vnext',
):
print(repr(case))
print(" redis:", redis_split(case))
print(" moon :", moon_split(case))
PYRepository: pilotspace/moon
Length of output: 834
Match Redis quote and whitespace handling in split_args_quoted
Treat " and ' as quote-state transitions inside unquoted tokens. Do not append them literally. SET k v"x must return an unbalanced-quotes error, while SET k v"x foo" must produce vx foo as one argument. Use the same Redis whitespace predicate for token boundaries and post-quote checks. The current mismatch can leave inputs such as SET key\v"value" stuck at \v because the parser does not advance past it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/protocol/inline.rs` around lines 235 - 245, Update split_args_quoted’s
unquoted-token parsing to treat single and double quotes as quote-state
transitions rather than appending them, returning an unbalanced-quotes error
when the quote is not closed and merging quoted content into the current
argument when it is. Reuse the Redis whitespace predicate for token boundaries
and post-quote checks, ensuring whitespace such as vertical tab advances the
parser instead of leaving it stuck.
Comment on lines
+1493
to
+1498
| if cmd.len() > 3 && cmd[..3].eq_ignore_ascii_case(b"FT.") { | ||
| responses.push(Frame::Error(Bytes::from_static( | ||
| b"ERR FT.* commands are not supported inside MULTI/EXEC", | ||
| ))); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The FT.* rejection inside MULTI does not mark the transaction dirty in any of the three handlers. Each handler rejects FT.* at queue time with an error but leaves conn.multi_dirty false, while the adjacent queue_time_rejection path sets it. MULTI, SET a 1, FT.SEARCH idx q, EXEC therefore errors on the FT.SEARCH and then still applies the SET, where a queue-time error must produce -EXECABORT and run nothing.
src/server/conn/handler_monoio/mod.rs#L1493-L1498: setconn.multi_dirty = true;before pushing theFT.*error frame.src/server/conn/handler_sharded/mod.rs#L878-L883: setconn.multi_dirty = true;before pushing theFT.*error frame.src/server/conn/handler_single.rs#L1755-L1765: setconn.multi_dirty = true;before pushing theFT.*error frame; consider folding this check intoqueue_time_rejectionso the three copies cannot drift again.
📍 Affects 3 files
src/server/conn/handler_monoio/mod.rs#L1493-L1498(this comment)src/server/conn/handler_sharded/mod.rs#L878-L883src/server/conn/handler_single.rs#L1755-L1765
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/conn/handler_monoio/mod.rs` around lines 1493 - 1498, Mark the
transaction dirty before rejecting unsupported FT.* commands in all three
handlers: src/server/conn/handler_monoio/mod.rs#L1493-L1498,
src/server/conn/handler_sharded/mod.rs#L878-L883, and
src/server/conn/handler_single.rs#L1755-L1765. Update each FT.* queue-time
rejection, or centralize it through queue_time_rejection, so conn.multi_dirty is
set before the error frame is pushed and EXEC aborts without applying queued
commands.
Comment on lines
+1004
to
+1019
| pub(crate) fn is_transaction_control(cmd: &[u8]) -> bool { | ||
| const CONTROL: [&[u8]; 12] = [ | ||
| b"MULTI", | ||
| b"EXEC", | ||
| b"DISCARD", | ||
| b"WATCH", | ||
| b"UNWATCH", | ||
| b"RESET", | ||
| b"QUIT", | ||
| b"REPLCONF", | ||
| b"PSYNC", | ||
| b"SYNC", | ||
| b"REPLICAOF", | ||
| b"SLAVEOF", | ||
| ]; | ||
| CONTROL.iter().any(|c| cmd.eq_ignore_ascii_case(c)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Redis 8.6, does UNWATCHqueue afterMULTI? Inspect src/commands/unwatch.json command flags and Redis transaction dispatch behavior.
💡 Result:
In Redis 8.6, the UNWATCH command is queued when sent within a MULTI/EXEC transaction block, rather than being executed immediately [1][2]. While the command is technically accepted by the server inside a transaction, it does not perform its intended function of clearing watched keys at the time of issuance [1]. Because it is queued, it is only processed when EXEC is called, at which point the transaction's status (dirty or clean) has already been determined [1]. Consequently, calling UNWATCH inside a transaction block is functionally ineffective and generally considered a misuse of the command [1][2]. Regarding its implementation, UNWATCH is not explicitly excluded from the queueing mechanism used for other transactional commands [2]. The server's transaction dispatch logic queues commands that are not control commands (like EXEC, DISCARD, or MULTI itself) [2]. Because UNWATCH lacks the specific guard logic that prevents WATCH from being called inside a MULTI block, it follows the default behavior of being queued [1][2]. As of the current Redis version, the development team has opted not to change this behavior to avoid potential breaking changes for client applications that may have inadvertently relied on this existing, albeit ineffective, pattern [1][2].
Citations:
- 1: [BUG] UNWATCH should be rejected within a MULTI/EXEC transaction just like WATCH is redis/redis#11686
- 2: [QUESTION]About executing unwatch in a multi block redis/redis#9108
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- transaction-control definition ---'
sed -n '970,1040p' src/server/conn/shared.rs
printf '%s\n' '--- queueing and transaction dispatch references ---'
rg -n -C 5 'is_transaction_control|WATCH inside MULTI|UNWATCH|watched|EXEC' src/server/conn/shared.rs tests/multi_exec_queue_semantics.rs
printf '%s\n' '--- relevant test section ---'
sed -n '220,310p' tests/multi_exec_queue_semantics.rsRepository: pilotspace/moon
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all control-classification call sites ---'
rg -n -C 12 'is_transaction_control\(' src/server/conn
printf '%s\n' '--- UNWATCH dispatch and state mutation ---'
rg -n -C 15 'UNWATCH|unwatch|watched_keys\.clear|watch.*clear|clear.*watch' src/server/conn src/command tests --glob '*.rs'
printf '%s\n' '--- queueing branches around the control check ---'
rg -n -C 30 'is_transaction_control|in_multi|command_queue\.push|queue_time_rejection' src/server/conn/handler_single.rs src/server/conn/handler_sharded src/server/conn/handler_monoio src/server/conn/shared.rs --glob '*.rs'Repository: pilotspace/moon
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- classification call-site list ---'
rg -n 'is_transaction_control\(' src/server/conn
printf '%s\n' '--- UNWATCH implementation lines ---'
rg -n -C 4 'cmd\.eq_ignore_ascii_case\(b"UNWATCH"\)|watched_keys\.clear\(\)' src/server/conn --glob '*.rs'
printf '%s\n' '--- transaction entry points ---'
rg -n 'try_handle_multi_exec|queue_time_rejection|command_queue\.push' src/server/conn/handler_single.rs src/server/conn/handler_sharded src/server/conn/handler_monoio --glob '*.rs'Repository: pilotspace/moon
Length of output: 10251
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- sharded transaction routing and queueing ---'
sed -n '820,915p' src/server/conn/handler_sharded/mod.rs
printf '%s\n' '--- monoio transaction routing and queueing ---'
sed -n '1450,1530p' src/server/conn/handler_monoio/mod.rs
printf '%s\n' '--- shared WATCH/UNWATCH handler ---'
sed -n '1,95p' src/server/conn/watch.rs
printf '%s\n' '--- sharded and monoio write-handler dispatch order ---'
sed -n '532,620p' src/server/conn/handler_sharded/write.rs
sed -n '707,790p' src/server/conn/handler_monoio/write.rsRepository: pilotspace/moon
Length of output: 23313
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
files = {
"single": Path("src/server/conn/handler_single.rs"),
"sharded_mod": Path("src/server/conn/handler_sharded/mod.rs"),
"monoio_mod": Path("src/server/conn/handler_monoio/mod.rs"),
"shared": Path("src/server/conn/shared.rs"),
}
for name, path in files.items():
text = path.read_text()
lines = text.splitlines()
print(f"--- {name}: relevant ordering ---")
needles = (
'is_transaction_control(cmd)',
'cmd.eq_ignore_ascii_case(b"UNWATCH")',
'try_handle_multi_exec(',
'try_handle_watch_unwatch(',
'if conn.in_multi {',
'if conn.in_multi && !is_transaction_control(cmd)',
)
for i, line in enumerate(lines, 1):
if any(needle in line for needle in needles):
print(f"{i}: {line.strip()}")
print("--- queued transaction executor references to UNWATCH ---")
for path in Path("src/server/conn").rglob("*.rs"):
text = path.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "UNWATCH" in line.upper() or "unwatch" in line:
print(f"{path}:{i}: {line.strip()}")
PY
printf '%s\n' '--- single-handler dispatch context ---'
sed -n '1620,1660p' src/server/conn/handler_single.rs
sed -n '1748,1782p' src/server/conn/handler_single.rs
printf '%s\n' '--- transaction executor command dispatch ---'
sed -n '145,235p' src/server/conn/shared.rsRepository: pilotspace/moon
Length of output: 9845
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- single-handler dispatch implementation ---'
rg -n -C 8 'fn dispatch\(|pub.*dispatch\(' src/server/conn src/command --glob '*.rs'
printf '%s\n' '--- UNWATCH command registration and generated handlers ---'
rg -n -i -C 5 'unwatch' src/command src/server/conn --glob '*.rs' --glob '*.json'
printf '%s\n' '--- all command-name dispatch tables near matching implementations ---'
rg -n -i -C 3 'watch|unwatch' src/command --glob '*.rs' --glob '*.json'Repository: pilotspace/moon
Length of output: 20462
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- command dispatch and unknown-command fallback ---'
sed -n '60,155p' src/command/mod.rs
printf '%s\n' '--- every executable UNWATCH implementation ---'
rg -n -i 'unwatch' src/command src/server/conn --glob '*.rs' --glob '*.json'Repository: pilotspace/moon
Length of output: 4585
Queue UNWATCH inside MULTI across all runtimes.
When UNWATCH is sent inside MULTI, queue it so EXEC checks watched-key versions before any transaction command can clear them. Remove it from CONTROL, update handler_single.rs so its pre-queue arm does not consume it, and return OK for the queued command because the transaction executor has no UNWATCH dispatch. Add runtime-specific two-connection tests that assert a conflicting write makes EXEC return a null array.
📍 Affects 2 files
src/server/conn/shared.rs#L1004-L1019(this comment)tests/multi_exec_queue_semantics.rs#L263-L270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/conn/shared.rs` around lines 1004 - 1019, Remove UNWATCH from the
CONTROL list in is_transaction_control, and update handler_single.rs so its
pre-queue handling no longer consumes UNWATCH; when queued inside MULTI, return
OK while preserving watch-version validation before EXEC. Add runtime-specific
two-connection coverage in tests/multi_exec_queue_semantics.rs around lines
263-270 asserting that a conflicting write causes EXEC to return a null array.
Comment on lines
+1105
to
+1112
| // `arity` counts the command name itself, so compare against args + 1. | ||
| // Positive = exact; negative = minimum (variadic). | ||
| let given = args.len() as i16 + 1; | ||
| let bad_arity = if meta.arity >= 0 { | ||
| given != meta.arity | ||
| } else { | ||
| given < -meta.arity | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/server/conn/shared.rs --match queue_time_rejection --view expanded
sed -n '1098,1120p' src/server/conn/shared.rs
rg -n -C 3 --glob '*.rs' 'ParseConfig|MbulkCountTooBig|max.*(mbulk|multibulk|array)' src/protocol src/serverRepository: pilotspace/moon
Length of output: 23315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- arity definition and queue-time call sites ---'
rg -n -C 5 --glob '*.rs' 'arity:|queue_time_rejection\(|CommandMeta|meta\.arity' src
printf '%s\n' '--- parser array-length and frame construction ---'
sed -n '90,115p' src/protocol/parse.rs
sed -n '455,485p' src/protocol/parse.rs
rg -n -C 4 --glob '*.rs' 'FrameVec|Vec<Frame>|args:.*Frame' src/server src/protocol
printf '%s\n' '--- deterministic i16 behavior ---'
python3 - <<'PY'
I16_MIN, I16_MAX = -32768, 32767
for n in (32766, 32767, 32768, 65534, 65535, 65536, 1_048_575):
cast = ((n + 2**15) % 2**16) - 2**15
wrapped = ((cast + 1 + 2**15) % 2**16) - 2**15
print(f"args={n}: cast={cast}, given_release={wrapped}")
PYRepository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- queue-time implementation and dispatch arity checks ---'
sed -n '1038,1120p' src/server/conn/shared.rs
rg -n -C 8 --glob '*.rs' 'wrong number of arguments|arity_ok|meta\.arity|argv\.len|args\.len' \
src/server/conn src/command | head -n 240
printf '%s\n' '--- parser array limits and frame construction ---'
sed -n '88,112p' src/protocol/parse.rs
sed -n '450,486p' src/protocol/parse.rs
rg -n -C 5 --glob '*.rs' 'Frame::Array|FrameVec' src/protocol/parse.rs | head -n 160
printf '%s\n' '--- reachable wrap cases ---'
python3 - <<'PY'
MAX_ARRAY_LENGTH = 1024 * 1024
for n in (32766, 32767, 32768, 65535, 65536, 65537, MAX_ARRAY_LENGTH - 1):
cast = ((n + 2**15) % 2**16) - 2**15
given = ((cast + 1 + 2**15) % 2**16) - 2**15
print(f"args={n}, i16_cast={cast}, release_given={given}, fixed_arity_2_accepts={given == 2}")
PYRepository: pilotspace/moon
Length of output: 31981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dispatch entry points and command metadata use ---'
rg -n --glob '*.rs' 'fn (dispatch|handle|execute)|COMMAND_META|metadata::lookup|lookup\(cmd\)' \
src/server/conn src/command | head -n 220
printf '%s\n' '--- fixed-arity command handlers ---'
rg -n -C 6 --glob '*.rs' 'eq_ignore_ascii_case\(b"(GET|ECHO|PING|SET)"\)|cmd == b"(GET|ECHO|PING|SET)"|args\.len\(\)' \
src/server/conn/handler_sharded src/server/conn/handler_monoio src/command | head -n 260Repository: pilotspace/moon
Length of output: 28029
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '55,135p' src/command/mod.rs
rg -n -C 8 --glob '*.rs' 'b"GET"|b"ECHO"|b"SET"' src/command/mod.rs | head -n 220
sed -n '1125,1185p' src/command/mod.rsRepository: pilotspace/moon
Length of output: 12634
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob '*.rs' 'pub fn (get|set|get_readonly)|fn (get|set|get_readonly)' src/commandRepository: pilotspace/moon
Length of output: 3547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,60p' src/command/string/string_read.rs
sed -n '315,355p' src/command/string/string_read.rs
sed -n '1,55p' src/command/string/string_write.rsRepository: pilotspace/moon
Length of output: 5342
Use a non-narrowing arity counter.
The parser permits up to 1,048,576 array elements, so this overflow is reachable. In checked builds, args.len() as i16 + 1 overflows at 32,767 arguments. In release builds, the count wraps. For example, GET with 65,537 arguments passes queue-time validation and returns its arity error only during EXEC.
Proposed fix
- let given = args.len() as i16 + 1;
+ let given = args.len().saturating_add(1);
+ let required = usize::from(meta.arity.unsigned_abs());
let bad_arity = if meta.arity >= 0 {
- given != meta.arity
+ given != required
} else {
- given < -meta.arity
+ given < required
};📝 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
| // `arity` counts the command name itself, so compare against args + 1. | |
| // Positive = exact; negative = minimum (variadic). | |
| let given = args.len() as i16 + 1; | |
| let bad_arity = if meta.arity >= 0 { | |
| given != meta.arity | |
| } else { | |
| given < -meta.arity | |
| }; | |
| // `arity` counts the command name itself, so compare against args + 1. | |
| // Positive = exact; negative = minimum (variadic). | |
| let given = args.len().saturating_add(1); | |
| let required = usize::from(meta.arity.unsigned_abs()); | |
| let bad_arity = if meta.arity >= 0 { | |
| given != required | |
| } else { | |
| given < required | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/conn/shared.rs` around lines 1105 - 1112, Update the arity
calculation in the command validation block to use a non-narrowing integer type
capable of representing the parser’s maximum argument count, avoiding the i16
cast and overflow at large inputs. Keep the existing exact-arity and
minimum-arity comparisons with meta.arity correct by using compatible types or
safe conversion.
… array CI caught three things my local gates did not, all in the lib's own `#[cfg(test)]` modules — which `cargo test --test <name>` never compiles, and which a stale clippy incremental cache reported clean. 1. `frame.rs` had a `ParseError::Invalid` initializer missing the new `kind` field (E0063). This broke Check, Check (monoio), Check (Windows), Check (macOS), and took Client-compat down with the build. 2. The `*-9` leniency was applied to the whole `b'*' | b'~' | b'>'` arm, so `~-2` (a RESP3 Set with an invalid count) silently stopped being rejected. There is no Redis behaviour to match for `~`/`>` — only `*` is lenient. Caught by `test_resp3_negative_set_count`, which is exactly why it exists. 3. `*-1` was folded into the same consume-and-drop path, so it returned `Ok(None)` instead of `Frame::Null`. `parse()` also parses REPLIES (replication), not just requests, where `*-1` is the canonical null array. Caught by `test_parse_null_array`. Both parser changes are now scoped: `count == -1` yields a null array for all three types as before; `count < -1` is consumed-and-dropped for `*` only, and still an error for `~`/`>`. Verified: 4578 lib + 11 multi_exec + 8 protocol_error, 0 failures. author: Tin Dang
There was a problem hiding this comment.
🔇 Additional comments (5)
src/protocol/parse.rs (2)
36-53: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that ignored multibulk frames do not stall pipelined input.
parse()consumes*-2\r\nand returnsOk(None). The same result also means incomplete input. Ifsrc/server/codec.rswaits for another read afterNone, a complete frame already after the ignored header remains buffered until more network data arrives.Make
parse()continue parsing after it consumes an ignored negative array. ReturnOk(None)only when no complete subsequent frame exists.
308-545: LGTM!.add/tasks/multi-exec-queue-semantics/TASK.md (3)
249-255: LGTM!
256-259: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Complete the metadata exception proof before marking this flag resolved.
The contract at Lines 224-230 permits
+QUEUEDonly whenCOMMAND_META::lookupfinds the command and its arity matches. If the dotted-name carve-out allows an unknown dotted command throughqueue_time_rejection, a typo can receive+QUEUEDand fail atEXECinstead of marking the transaction dirty. The listed bareGRAPHomission is also not covered by a dotted-name exception.Make
me10bverify known and unknown dotted names andGRAPH. Register every dispatchable command inCOMMAND_META, or document and test an explicit exception that preserves queue-time rejection for unknown commands.
260-261: LGTM!
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4181bc5-b745-4cb8-8fea-71e2d8b730ee
📒 Files selected for processing (4)
.add/tasks/multi-exec-queue-semantics/TASK.md.add/tasks/protocol-error-lifetime/TASK.mdsrc/protocol/frame.rssrc/protocol/parse.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/protocol/frame.rs
- .add/tasks/protocol-error-lifetime/TASK.md
ERR_STALE_WAIVER in CI: multi_aborts_on_unknown_command and multi_aborts_on_wrong_arity no longer reproduce, because multi-exec-queue-semantics fixed them. Retired rather than deleted — both are now LIVE parity assertions, so EXECABORT cannot silently regress. The two CONFIG-GET-in-MULTI waivers still reproduce and keep their waiver, now annotated: the queue gate demonstrably fixed this class for INFO and SUBSCRIBE, and CONFIG sits BELOW the gate in every handler, so intercept order is not the explanation. Recorded as not-yet-understood rather than guessed at. Local: PASS=184 FAIL=0 WAIVED=16 TOTAL=200 (--strict). author: Tin Dang
The queue gate added for multi-exec-queue-semantics made EVERY non-control command queue while `in_multi`. That is correct for anything that reaches `dispatch()` — but `execute_transaction` replays the queue THROUGH `dispatch()`, and a connection-level intercept never gets there. So commands that worked fine before the gate started coming back as `-ERR unknown command` from inside the EXEC array: the client got an error where it used to get data. Probed all 16 candidates against a live server: 7 broke inside EXEC (CONFIG, CLIENT, ACL, CLUSTER, SCRIPT, WAIT) and PUBSUB was rejected at queue time instead, because it is absent from COMMAND_META and has no dot, so the dotted carve-out misses it. `is_intercept_only()` exempts those from both the queue gate and queue-time validation. LATENCY looked like a member and is not. It returns `-ERR unknown command` OUTSIDE a transaction too — Moon simply does not implement it. Exempting it would have hidden a genuinely unimplemented command behind a transaction exemption, and worse, would have let it error without setting `multi_dirty`, so EXEC would proceed past a command the server does not know. Left out, it falls through to `queue_time_rejection` and aborts the transaction the way Redis aborts on any unknown command. The compat harness's two CONFIG-in-MULTI waivers are what surfaced this; me10 was strengthened to assert the queued command actually EXECUTES, not merely that it queued, since the original assertion passed while the regression was live. Tested: protocol_error_lifetime 8/8, multi_exec_queue_semantics 12/12 (me7 ignored, pending Frame::NullArray), lib green. author: Tin Dang
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/conn/shared.rs`:
- Around line 1049-1050: Update the documentation near the queueing explanation
to state that the is_intercept_only classifier exempts PUBSUB from generic MULTI
queueing, rather than claiming queue_time_rejection alone handles it. Keep the
explanation consistent with the surrounding classification logic and handler
call site.
🪄 Autofix
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 Plus
Run ID: 1e82c263-f199-47ed-91ad-793f7a3ff5d5
📒 Files selected for processing (4)
src/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/shared.rstests/multi_exec_queue_semantics.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/multi_exec_queue_semantics.rs
- src/server/conn/handler_sharded/mod.rs
- src/server/conn/handler_monoio/mod.rs
Comment on lines
+1049
to
+1050
| /// assumption named. It is handled by `queue_time_rejection` consulting this | ||
| /// list, not by exempting it from queueing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the PUBSUB queueing explanation.
The queue gate uses is_intercept_only to keep PUBSUB out of generic MULTI queueing. It does not rely only on queue_time_rejection. This sentence conflicts with Line 1035 through Line 1040 and the call site in src/server/conn/handler_sharded/mod.rs, Line 876. State that this classifier exempts PUBSUB from generic queueing.
Proposed documentation fix
-/// It is handled by `queue_time_rejection` consulting this
-/// list, not by exempting it from queueing.
+/// It is handled by exempting it from generic transaction queueing, so
+/// `queue_time_rejection` does not poison the transaction.📝 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
| /// assumption named. It is handled by `queue_time_rejection` consulting this | |
| /// list, not by exempting it from queueing. | |
| /// assumption named. It is handled by exempting it from generic transaction queueing, so | |
| /// `queue_time_rejection` does not poison the transaction. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/conn/shared.rs` around lines 1049 - 1050, Update the documentation
near the queueing explanation to state that the is_intercept_only classifier
exempts PUBSUB from generic MULTI queueing, rather than claiming
queue_time_rejection alone handles it. Keep the explanation consistent with the
surrounding classification logic and handler call site.
TinDang97
added a commit
that referenced
this pull request
Aug 13, 2026
REPO pointed at /Users/tindang/workspaces/tind-repo/moon — the stale second checkout CLAUDE.md warns about, sitting at #126 (hash-ttl era) while main is at #472. Every target built, tested and ran ~350 PRs of the wrong code. The dangerous one is `make ci`, which advertises CI parity and would report green against a tree nobody is shipping. Three fixes: - REPO -> /Volumes/Games/tindang-repo/moon, with a comment naming the stale path so the next person does not "restore" it. - CARGO_TARGET_DIR=target-linux on every cargo target. The VM and the host compile the same shared checkout, so without this the Linux ELF artifacts clobber the macOS Mach-O ones (CLAUDE.md requires it). Declared with `export ... &&` rather than a bare VAR=x prefix, which would only have applied to the first command of each `&&` chain — `make ci` and `make clippy` each run two cargo invocations. - `pkill -x moon` -> `pkill -f "$(REPO)/target-linux/release/moon"`. Matching by bare process name kills any process called `moon`, not just the one this Makefile built. Verified: `make fmt` runs clean end-to-end in the moon-dev VM against the live checkout; `make -n ci` shows the export surviving the full && chain. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 13, 2026
#472 landed the code and tests for protocol-error-lifetime and multi-exec-queue-semantics, but left both phase markers behind — at `tests` and `ground`. Walks them to done and records a gate for each. Evidence is taken from merged main, not from the PR branch: both suites re-run under runtime-monoio (the shipped runtime) AND runtime-tokio. protocol_error_lifetime 8/8 both ways; multi_exec_queue_semantics 12 passed / 1 ignored both ways. protocol-error-lifetime is a clean PASS. Its §6 records what was confirmed rather than what was run: the wiring check is the one that matters, because Moon has four places that read frames and a handler still doing `Err(_) => break` compiles perfectly — so all four were exercised rather than grepped for. multi-exec-queue-semantics is RISK-ACCEPTED, not PASS. Must #7 is not met: BLPOP inside MULTI still replies a Null Bulk where Redis replies a Null Array, and me7 is #[ignore]d rather than passing. The divergence is not introduced by this task and is not specific to transactions — `Frame` has no null-array variant at all, so a plain BLPOP that times out mistypes its reply too. Fixing it means threading Frame::NullArray through every Frame::Null arm in serialize.rs / resp3.rs, which touches every reply path and can flip replies that are currently correct. That earns its own contract; filed as #482, which the gate names as its ticket. RESP2-only — RESP3 spells both nulls `_`. me7 stays in the suite, ignored with a reason naming the missing capability, so the gap is visible to the next reader instead of vanishing with the assertion. The task that adds the variant un-ignores it as its own proof. Also carries the observe deltas forward: the inline-cap divergence above 65 530 B, the write-in-the-valid-prefix ambiguity, and three method lessons — test the wire not the client, assert the state not the reply when the bug is that state changed anyway, and discharge a flagged assumption by test rather than by argument. Refs #472, #482 author: Tin Dang
This was referenced Aug 14, 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
1 participant
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.
Closes two
v0-9-client-compattasks:protocol-error-lifetimeandmulti-exec-queue-semantics.Everything below was measured against redis-server 8.6.1 over raw sockets —
redis-clicannot express a malformed frame, two commands in one write, or the difference between$-1and*-1.1 · A protocol fault now names itself instead of closing mute
The read loops did this:
That arm threw away two things: the parse reason — so a client got a bare FIN and could not tell a bad encoder from a dropped network — and
batch, which already held every valid frame parsed from the same read.PING\r\n*-9\r\nin one write therefore answered nothing at all.All three handlers now execute and flush the valid prefix, then send
-ERR Protocol error: <reason>, then close, in that order.ParseError::Invalidgains a typedkindcarrying Redis's verbatim wording; the detailed message is kept for logs and fuzz triage, deliberately different (invalid bulk string length: -5localises a bug;invalid bulk lengthis what a driver author matches on).Measured and fixed alongside it:
*-9\r\n$abc/$-5/$999999999-ERR Protocol error: invalid bulk length, then closes-ERR Protocol error: too big inline request— Moon already built this string; the handler discarded itGET "unclosed$-1— accepts it as a key-ERR Protocol error: unbalanced quotes in requestPING\r\n*-9\r\n+PONG, then the faultThe inline parser had no quote support whatsoever, so
SET k "a b"became three arguments containing literal quote bytes. It is now a port of Redis'ssdssplitargs(hex/escape handling, closing-quote-must-end-token, unbalanced rejection), gated behind onememchr2pass so the unquoted hot path is untouched.2 · MULTI is atomic with respect to queue-time faults
Moon had no queue-time validation, so
EXECran whichever half of a transaction happened to parse:That is data corruption, not a compatibility nit.
multi_dirty(Redis'sCLIENT_DIRTY_EXEC) is now set when a command that could never run is queued;EXECanswers-EXECABORT Transaction discarded because of previous errors.and executes nothing.DISCARDandRESETclear it — a leaked flag would abort an innocent later transaction, which is worse than the bug being fixed, so it has its own test (me3).The wider defect this exposed
Moon decided "am I in a transaction?" hundreds of lines below the
INFO/CLIENT/WS/MQ/PUBLISH/SUBSCRIBEintercepts — so every one of those executed for real inside a transaction.SUBSCRIBE chput the connection into subscriber mode mid-MULTI;INFO serverreturned a 3 KB dump where Redis returns+QUEUED.The queue decision now sits in one gate directly below each handler's ACL check, fixing the whole class at once. Below the ACL gate, deliberately: Redis refuses a forbidden command at queue time, and this repo has already shipped an ACL bypass caused by an intercept sitting above its permission check.
Safety of the arity/existence check
It reads the same
COMMAND_METAtable dispatch reads, so a command cannot become queueable-but-undispatchable. A name containing.that misses the table falls through to queueing: the table does not cover every dotted extension family, and rejecting one would break a command that works fine outside a transaction — a regression strictly worse than the bug. Pinned byme10b.Tests
tests/protocol_error_lifetime.rs(8) ·tests/multi_exec_queue_semantics.rs(12). Raw sockets throughout; each protocol case asserts the reply bytes, whether the server hung up, and whether a following command still gets served — any one alone hides the interesting half.Green on both runtimes: monoio (default/shipped) and
runtime-tokio,jemalloc. That matters here —handler_shardedandhandler_singleare not even compiled under monoio, so a fix in only one handler would have been invisible to whichever job built the other.Known gap, filed not hidden
me7is#[ignore]d with its reason. BLPOP-in-MULTI returns a Null Bulk where Redis returns a Null Array, andFramehas no null-array variant at all —Frame::Nullserialises to$-1in RESP2, full stop. So this is not a MULTI bug: a plainBLPOP key 1that times out outside a transaction returns the wrong type too. The fix is aFrame::NullArraythreaded through ~14Frame::Nullarms inserialize.rs/resp3.rs— every reply path — and deserves its own contract. The test is already the right assertion, waiting to be un-ignored.Summary by CodeRabbit
MULTIbehavior: invalid or unsupported commands are rejected when queued, dirty transactions abort onEXEC, andDISCARD/RESETrestore normal state.