fix(protocol): RESP3 reply types match Redis, and stop depending on calling context - #463
Merged
Conversation
…alling context
A live differential sweep against redis-server 8.6.1 found Moon's RESP3
conversion table wrong in both directions and, structurally, unable to be
right: it keyed only on the command NAME, while WITHSCORES, WITHVALUES and a
<count> argument are what actually decide the reply shape.
Under-conversion, client-breaking:
- ZRANGE/ZREVRANGE/ZRANGEBYSCORE/ZDIFF/ZUNION/ZINTER/ZRANDMEMBER ... WITHSCORES
arrived as a flat array of bulk strings instead of pair-wrapped
[member, Double]. An unmodified redis-py raises
"ValueError: not enough values to unpack" on this, so every RESP3
application reading sorted-set scores was broken outright.
- HRANDFIELD WITHVALUES / ZRANDMEMBER WITHSCORES answered a Map where Redis
answers an array of pairs.
- ZMSCORE, GEOPOS, SPOP <count>, ZPOPMIN/ZPOPMAX and XINFO STREAM were not
converted at all.
Over-conversion, equally wrong:
- SISMEMBER, HEXISTS, EXPIRE, PEXPIRE, PERSIST, SETNX and MSETNX returned
Boolean where Redis returns Integer.
- INCRBYFLOAT/HINCRBYFLOAT returned a lossy Double (,10.6) where Redis
returns the exact Bulk "10.59999999999999964".
Emptiness must not change the reply TYPE either: HGETALL and CONFIG GET on a
miss answered *0 where Redis answers %0, so a client dispatching on the type
byte broke on exactly the path it hits most.
The conversion is now decided by (command, args) at one policy choke point
(Resp3Shape in src/protocol/resp3.rs) instead of by 11 call sites across three
handlers. The cross-shard reply loop no longer has the command's args by the
time its batch returns, so the shape is classified at ENQUEUE time and a 1-byte
Copy tag travels in RemoteMeta -- no per-command allocation on the shard hot
path. execute_transaction_sharded now takes the connection's protocol version
and converts each inner reply with its own command, so a command answers the
same shape standalone, inside MULTI/EXEC and inside a pipeline; previously
SMEMBERS was a Set outside a transaction and a flat Array inside one. CONFIG
GET (Map) and CLIENT INFO (Verbatim) are fixed at their intercepts, which
short-circuit the dispatch exit entirely -- the reason CONFIG could never be
fixed before.
RESP2 is byte-identical, pinned by a test written before the fix.
Verification (all against a LIVE redis-server 8.6.1, never against Moon's own
expectations):
- scripts/test-client-compat.sh --strict: PASS 98 -> 157, WAIVED 54 -> 25,
FAIL 0, exit 0, with all 13 now-stale waivers deleted.
- tests/resp3_type_fidelity.rs: 13 new tests asserting the wire type byte
directly, including shape equality across standalone/MULTI/pipeline and
across a 4-shard server.
- Raw-socket byte-diff of the miss path: BYTE-IDENTICAL to redis 8.6.1.
- redis-py acceptance: zrange withscores went from raising to [['a', 1.0]].
- clippy -D warnings clean on default and runtime-tokio,jemalloc; fmt clean.
The empty-reply defect was found by an adversarial re-read AFTER every bar was
green: all 152 harness cases populated their key first, so the entire miss path
was undiffed. Five miss-path cases were added and verified discriminating by
running them against the pre-fix binary, where all five fail.
Refs: .add/tasks/resp3-type-fidelity, milestone v0-9-client-compat
author: Tin Dang
…-failure A/B Advances the task to phase=verify and fills the GATE RECORD with the measured evidence. Documents the same-load A/B that cleared this change of the two full-suite residual failures: a pre-change server built from main fails the dbsize_offload_logical guard MORE often (7/8) than this branch (5/8), with an identical assertion, line and key counts on both legs. The gate OUTCOME is deliberately left open — §6's 'a person reviewed and approved the change' is not the AI's box to tick, and this change carries a disclosure (a unit test was deleted during build) that warrants a human read. author: Tin Dang
…llow-ups filed Records the human-approved gate (Tin Dang, 2026-08-10) and the two verification runs added after the first evidence pass: - tokio CI-parity suite (the feature set every CI test job builds): 195 binaries, 4352 passed, 0 failed, exit 0. It also passed both tests that failed the monoio run, independently corroborating the contention diagnosis. - Client-compat harness against the DEPLOYED :6381 flag set on a throwaway server with an empty data dir: PASS 157 / FAIL 0. Live :6381 was deliberately not used as the target -- differ.py FLUSHALLs before every one of its 182 entries and that instance holds ~764k actively-growing keys. Residual findings filed rather than folded in: #459 #460 #461 #462. 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 |
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change replaces command-name-only RESP3 conversion with argument-aware reply-shape classification. It propagates shape and protocol metadata through local, pipelined, transactional, and cross-shard paths, and adds compatibility tests and task records. ChangesRESP3 Fidelity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ConnectionHandler
participant Shard
participant RESP3Converter
Client->>ConnectionHandler: command and arguments
ConnectionHandler->>RESP3Converter: classify reply shape
ConnectionHandler->>Shard: dispatch command and shape metadata
Shard-->>ConnectionHandler: command response
ConnectionHandler->>RESP3Converter: apply stored shape
RESP3Converter-->>Client: RESP3-shaped reply
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (7)
tests/resp3_type_fidelity.rs (2)
750-754: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
ZSCOREhas no absolute type assertion in this suite.
r3f9compares the shape across standalone, MULTI/EXEC, and pipeline. It does not compare the shape to Redis. AZSCOREreply that is consistently wrong in all three contexts passes this test. No other test in this file pins theZSCORERESP3 type. The manifest addshard_zscore_floatwithnumeric_tolerance, which covers the value but not the wire type.Add a
ZSCOREtype assertion tor3f8or to a dedicated case, using the recorded Redis 8.6.1 oracle value.Also applies to: 767-780
🤖 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/resp3_type_fidelity.rs` around lines 750 - 754, Add an absolute RESP3 type assertion for ZSCORE in r3f8 or a dedicated test case, comparing the reply against the recorded Redis 8.6.1 oracle value rather than only comparing shapes across execution modes. Keep the existing r3f9 cross-context coverage and ensure the assertion validates the wire type as well as the expected value.
793-816: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert at least one ZRANGE went over cross-shard dispatch.
With
--shards 4,{t0}and{t7}are only one possible pair that does not hash to the same local shard under this test’s fresh connection placement. The current check passes when every ZRANGE uses the local inlined/local handler path, including cases where each fresh connection lands on the key owner. Add a check that at least one command used cross-shard dispatch, e.g. by capturingmoon_dispatch_path_total{path="cross_spsc"}againstmoon_dispatch_path_total{path="local_inline"}after the five ZRANGEs.🤖 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/resp3_type_fidelity.rs` around lines 793 - 816, Track dispatch-path metrics around the five ZRANGE calls in the test loop and assert that at least one command used cross-shard dispatch, such as verifying the post-run cross_spsc counter increased relative to its baseline. Also capture the local_inline counter as suggested if needed to distinguish paths, while preserving the existing reply-shape assertions..add/tasks/multi-exec-queue-semantics/TASK.md (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the §3 CONTRACT fenced code block in all three new task scaffolds. The three files copy the same unfilled task template, so markdownlint reports MD040 at the same location in each. Change the opening fence from
```to```textat each site. Fixing the source template prevents the warning in future scaffolds.
.add/tasks/multi-exec-queue-semantics/TASK.md#L71-L76: change the opening fence on Line 71 to```text..add/tasks/protocol-error-lifetime/TASK.md#L71-L76: change the opening fence on Line 71 to```text..add/tasks/watch-cas-transactions/TASK.md#L71-L76: change the opening fence on Line 71 to```text.🤖 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 71 - 76, Update the §3 CONTRACT fenced code block opening fence from an unlabeled fence to a text-labeled fence in .add/tasks/multi-exec-queue-semantics/TASK.md lines 71-76, .add/tasks/protocol-error-lifetime/TASK.md lines 71-76, and .add/tasks/watch-cas-transactions/TASK.md lines 71-76. Apply the same change at all three sites so the shared task scaffold satisfies markdownlint.Source: Linters/SAST tools
src/protocol/resp3.rs (1)
244-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn the original frame in the unreachable
scored_flatbranch.The length check guarantees both
next()calls yieldSome. Theelsebranch is unreachable today. If a future edit changes the guard, this branch discards the reply and returns an empty array. The module contract states a conversion never truncates a reply. Returnother-equivalent data instead of an empty array.♻️ Proposed change
fn scored_flat(frame: Frame) -> Frame { match frame { Frame::Array(items) if items.len() == 2 => { + // Length checked above, so both nexts are Some. The fallback + // rebuilds the input rather than dropping it. let mut iter = items.into_iter(); - // SAFETY-FREE: length checked above, so both nexts are Some. let (Some(m), Some(s)) = (iter.next(), iter.next()) else { - return Frame::Array(FrameVec::new()); + return Frame::Array(FrameVec::from_vec(iter.collect())); }; Frame::Array(FrameVec::from_vec(vec![m, bulk_to_double(s)])) }🤖 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/resp3.rs` around lines 244 - 252, Update the unreachable fallback in scored_flat so it preserves and returns the original Frame rather than constructing an empty array. Keep the existing two-item conversion unchanged and ensure any guard/iteration failure does not discard or truncate the reply.src/server/conn/handler_monoio/mod.rs (1)
2999-3016: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueDrop the unused command-name field from
RemoteMeta.Both
src/server/conn/handler_monoio/mod.rsandsrc/server/conn/handler_sharded/mod.rspackcmd_bytesintoRemoteMeta, but the reply destructure only uses_cmd_nameand the response shape is already classified byresp3_shape. Remove this field from both runtime tuples and their constructors to avoid oneBytesconstruction per cross-shard command.🤖 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 2999 - 3016, Remove the unused command-name field from RemoteMeta in both monoio and sharded handlers. Update the runtime tuple constructors and corresponding response-loop destructuring to omit cmd_bytes/_cmd_name, while retaining resp3_shape for response handling and preserving the existing tuple ordering for the remaining metadata.src/server/conn/handler_sharded/mod.rs (1)
2193-2201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a named struct instead of a long positional tuple for cross-shard reply metadata.
RemoteMetaand theremote_groupsentry tuple carry 5-7 positional fields (resp_idx,aof_bytes,cmd_bytes,track_keys,resp3_shape, plusarc_frame/dbat the enqueue site). This tuple is built at Line 2158, unzipped at Lines 2193-2201, and destructured again at Lines 2242, 2294, and 2309. Every site currently matches, but a future field addition, removal, or reorder at any one of these positions silently mismatches the others (for example:resp3_shapecould be reinterpreted astrack_keys), corrupting reply shaping, AOF durability tracking, or CLIENT TRACKING invalidation without a compile error.Replace the tuple with a named struct (e.g.
RemoteReplyMeta { resp_idx, aof_bytes, cmd_bytes, track_keys, resp3_shape }) so each site accesses fields by name.Also applies to: 2309-2319
🤖 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_sharded/mod.rs` around lines 2193 - 2201, Replace the positional RemoteMeta tuple and related remote_groups entry tuple with a named RemoteReplyMeta struct containing resp_idx, aof_bytes, cmd_bytes, track_keys, and resp3_shape; retain arc_frame and db as explicitly named fields where needed. Update construction near the enqueue site, the unzip logic, and all consumers around the reply-processing sites to use named-field access, preserving existing behavior..add/tasks/resp3-type-fidelity/TASK.md (1)
622-627: 🗄️ Data Integrity & Integration | 🔵 TrivialMake the compatibility gate reproducible.
Lines 622-627 state that the default
MOON_BINcan be stale and produce false compatibility results. The gate record at Lines 499-505 does not identify the binary path, commit, hash, or modification time. Confirm that the 157-pass run used the branch binary and record its provenance.🤖 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/resp3-type-fidelity/TASK.md around lines 622 - 627, Update the compatibility gate record and the scripts/test-client-compat.sh harness to capture binary provenance: print the resolved MOON_BIN path and modification time (and identify the branch build or commit when available) in the run header, and refuse or emit a prominent warning when the binary predates the newest source file. Ensure the recorded 157-pass result explicitly confirms it used the branch binary with this provenance.
🤖 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/milestones/v0-9-client-compat/MILESTONE.md:
- Line 68: Update the resp3-type-fidelity milestone entry to state that XINFO
STREAM is corrected to a Map shape only, and link or reference the separate
field-completeness follow-up instead of claiming missing fields are delivered.
Alternatively, keep the milestone task open until field completeness is
addressed.
- Line 85: Update the milestone criterion for MULTI/EXEC queue semantics to
require +QUEUED only for valid queueable commands, while preserving verification
that EXEC returns their actual replies across 1, 2, and 4 shards. Add separate
assertions covering queue-time syntax errors producing EXECABORT and
transaction-control commands returning their defined transaction replies.
In @.add/state.json:
- Around line 287-296: Update the existing info-observability entry’s title in
.add/state.json to remove its protocol-error reply ownership, keeping
protocol-error-lifetime as the sole owner of that scope and aligning with the
v0-9-client-compat milestone boundary.
In @.add/tasks/resp3-type-fidelity/TASK.md:
- Around line 392-393: Resolve the contradiction in TASK.md before the build
gate: update the scope constraint to explicitly permit the documented test
deletion and replacement, including its provenance and approval, or remove the
claimed compliance with the no-test-changes rule. Keep the existing test-change
record and “same build” explanation consistent with the revised scope.
- Around line 280-284: Resolve the contradiction in the task’s
RemoteMeta.cmd_name contract: verify whether AOF and tracking consumers still
read RemoteMeta.cmd_name; if they do, document that evidence and retain the
field, otherwise remove it and update the frozen contract to SPECIFY. Rerun the
affected gate after changing the contract, and align the statements around the
signature changes and follow-up work.
- Around line 330-333: Update the RESP2 assertions in
r3f11_resp2_is_byte_identical and the repeated check near the later RESP2 test
to parse each reply by frame boundaries and reject disallowed RESP3 type markers
only when they are frame prefixes. Preserve arbitrary marker bytes inside
bulk-string payloads instead of scanning the raw reply globally.
- Line 5: Synchronize completion state with the approval checklist: in
.add/tasks/resp3-type-fidelity/TASK.md at lines 5 and 441, either complete the
approval item before retaining phase: done or explicitly make it non-gating; in
.add/state.json at lines 223-230, set phase: done and gate: PASS only when they
agree with the task record.
- Around line 260-263: Update the response-shape specification to consistently
use whole-reply pass-through when any score is unparseable: revise the rule near
line 116 and align the related tests, while preserving the existing behavior for
valid scores, odd element counts, errors, nulls, and proto versions below 3.
- Around line 249-251: The response-shape classifier must handle BZPOPMIN and
BZPOPMAX separately from ScoredPairs: add a dedicated blocking-pop shape that
preserves key, member, score, and timeout semantics, or exclude both commands
from the classifier until supported. Update the relevant shape definitions and
command classification logic, preserving existing non-blocking pop behavior.
In `@CHANGELOG.md`:
- Around line 9-10: Merge the newly added “### Fixed” block in the Unreleased
section with the existing “### Fixed” heading, placing all fixed entries under a
single heading while preserving their content and ordering.
In `@src/protocol/resp3.rs`:
- Around line 112-121: Update the command classification around the RESP3 shape
match to handle BZPOPMIN and BZPOPMAX separately from ZPOPMIN and ZPOPMAX.
Assign the blocking commands a dedicated [key, member, double] shape, or remove
them from this policy until that shape is implemented; do not let the shared
args.len() >= 2 logic classify them as ScoredPairs or ScoredFlat.
In `@src/server/conn/shared.rs`:
- Around line 441-443: Update execute_transaction so each raw dispatch result is
passed through super::util::apply_resp3_conversion with the corresponding
command, command arguments, response, and the executor connection’s protocol
version before being pushed. Match execute_transaction_sharded’s per-command
conversion behavior while preserving result ordering.
In `@tests/resp3_type_fidelity.rs`:
- Around line 822-861: The test r3f11_resp2_is_byte_identical only checks for
forbidden RESP3 type bytes and does not prove byte identity. Either rename it to
reflect that scope, such as r3f11_resp2_has_no_resp3_type_bytes, or add
recorded-baseline byte comparisons for each RESP2 response so payload, ordering,
and element counts are verified.
- Around line 946-949: Update the case table to include the key each command
reads, then use that per-case key in the DEL setup inside the test loop instead
of hardcoding r3f13:nokey. Ensure SMEMBERS deletes r3f13:noset, ZRANGE deletes
r3f13:noz, and each case explicitly constructs its own missing-key path.
---
Nitpick comments:
In @.add/tasks/multi-exec-queue-semantics/TASK.md:
- Around line 71-76: Update the §3 CONTRACT fenced code block opening fence from
an unlabeled fence to a text-labeled fence in
.add/tasks/multi-exec-queue-semantics/TASK.md lines 71-76,
.add/tasks/protocol-error-lifetime/TASK.md lines 71-76, and
.add/tasks/watch-cas-transactions/TASK.md lines 71-76. Apply the same change at
all three sites so the shared task scaffold satisfies markdownlint.
In @.add/tasks/resp3-type-fidelity/TASK.md:
- Around line 622-627: Update the compatibility gate record and the
scripts/test-client-compat.sh harness to capture binary provenance: print the
resolved MOON_BIN path and modification time (and identify the branch build or
commit when available) in the run header, and refuse or emit a prominent warning
when the binary predates the newest source file. Ensure the recorded 157-pass
result explicitly confirms it used the branch binary with this provenance.
In `@src/protocol/resp3.rs`:
- Around line 244-252: Update the unreachable fallback in scored_flat so it
preserves and returns the original Frame rather than constructing an empty
array. Keep the existing two-item conversion unchanged and ensure any
guard/iteration failure does not discard or truncate the reply.
In `@src/server/conn/handler_monoio/mod.rs`:
- Around line 2999-3016: Remove the unused command-name field from RemoteMeta in
both monoio and sharded handlers. Update the runtime tuple constructors and
corresponding response-loop destructuring to omit cmd_bytes/_cmd_name, while
retaining resp3_shape for response handling and preserving the existing tuple
ordering for the remaining metadata.
In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 2193-2201: Replace the positional RemoteMeta tuple and related
remote_groups entry tuple with a named RemoteReplyMeta struct containing
resp_idx, aof_bytes, cmd_bytes, track_keys, and resp3_shape; retain arc_frame
and db as explicitly named fields where needed. Update construction near the
enqueue site, the unzip logic, and all consumers around the reply-processing
sites to use named-field access, preserving existing behavior.
In `@tests/resp3_type_fidelity.rs`:
- Around line 750-754: Add an absolute RESP3 type assertion for ZSCORE in r3f8
or a dedicated test case, comparing the reply against the recorded Redis 8.6.1
oracle value rather than only comparing shapes across execution modes. Keep the
existing r3f9 cross-context coverage and ensure the assertion validates the wire
type as well as the expected value.
- Around line 793-816: Track dispatch-path metrics around the five ZRANGE calls
in the test loop and assert that at least one command used cross-shard dispatch,
such as verifying the post-run cross_spsc counter increased relative to its
baseline. Also capture the local_inline counter as suggested if needed to
distinguish paths, while preserving the existing reply-shape assertions.
🪄 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: 6f7f2d60-c262-40e8-af9e-b54ce8c0dd72
📒 Files selected for processing (23)
.add/milestones/v0-9-client-compat/MILESTONE.md.add/state.json.add/tasks/multi-exec-queue-semantics/TASK.md.add/tasks/protocol-error-lifetime/TASK.md.add/tasks/resp3-type-fidelity/TASK.md.add/tasks/watch-cas-transactions/TASK.mdCHANGELOG.mdscripts/client-compat/manifest.yamlsrc/protocol/resp3.rssrc/server/conn/handler_monoio/dispatch.rssrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_monoio/write.rssrc/server/conn/handler_sharded/dispatch.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_sharded/write.rssrc/server/conn/handler_single.rssrc/server/conn/mod.rssrc/server/conn/shared.rssrc/server/conn/util.rssrc/shard/coordinator.rssrc/shard/dispatch.rssrc/shard/spsc_handler.rstests/resp3_type_fidelity.rs
| - [ ] client-identity-introspection depends-on: client-compat-harness — HELLO `version` → REDIS_COMPAT_VERSION (redis-py refuses client-side caching against the Moon version string); COMMAND COUNT/INFO/GETKEYS/DOCS served from `metadata.rs`; ROLE, RESET, CLIENT SETINFO with lib-name/lib-ver surfaced in CLIENT LIST/INFO; reconcile the registered-but-unreachable set (DUMP, RESTORE, LATENCY, MODULE, RECLAMATION) — implement or deregister — plus a test that the registry and dispatch cannot diverge again. Also closes the INVERSE divergence: MONITOR, MQ and WS are reachable-or-wanted but absent from `metadata.rs`, so they are invisible to COMMAND and uncategorised for ACL — the reconciliation test must sweep both directions, and MONITOR itself (unimplemented, and not on the known-gap allowlist) is implemented or explicitly declared a non-goal here. | ||
| - [ ] watch-cas-transactions depends-on: client-compat-harness — WATCH/UNWATCH answer "unknown command" on both production dispatch paths (`handler_monoio`, `handler_sharded`); the implementation exists only in `handler_single.rs`, the embedded-mode handler, while `metadata.rs` registers both so COMMAND and ACL claim they are available. Optimistic-locking CAS is the one missing primitive with no client-side workaround. Includes the key-touch invalidation path (EXEC must abort when a watched key changed, cross-shard included) and removal of WATCH/UNWATCH from the `BACKLOGGED_UNIMPLEMENTED` allowlist in `tests/wire_reachability_red.rs`. | ||
| - [ ] protocol-error-lifetime depends-on: client-compat-harness — a malformed frame must produce `-ERR Protocol error: <detail>` and then close, after the already-valid pipelined prefix has been answered. Today Moon sends NOTHING for `PING` + a bad multibulk header (the valid PONG dies with the connection), and a bulk header followed by inline garbage produces no reply AND no close, so the client blocks to its own socket timeout. This is a connection-lifetime defect, not the reply-formatting item originally folded into `info-observability` — that scope moves here. | ||
| - [ ] resp3-type-fidelity depends-on: client-compat-harness — fix Map-vs-pairs inversions (ZRANDMEMBER/HRANDFIELD must be pair arrays); add pair-wrapping + Double for ZRANGE/ZPOPMIN/ZDIFF/ZUNION WITHSCORES; stop over-converting SISMEMBER/EXPIRE (Integer) and INCRBYFLOAT (Bulk); route CONFIG and EXEC/pipeline inner replies through `apply_resp3_conversion`; XINFO STREAM → Map with the missing fields; SPOP `<count>` → Set. Oracle sweep 2026-08-09 widened this: the whole `int_to_bool` branch is wrong (Redis answers Integer for all 7 of SISMEMBER/HEXISTS/EXPIRE/PEXPIRE/PERSIST/SETNX/MSETNX), `bulk_to_double` is wrong for INCRBYFLOAT/HINCRBYFLOAT and missing for ZMSCORE/GEOPOS, and the conversion needs ARG awareness (WITHSCORES/WITHVALUES/count decide the shape) — which the current `maybe_convert_resp3(cmd, response, proto)` signature cannot express. Transaction *semantics* moved OUT to `multi-exec-queue-semantics`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Separate the XINFO type fix from field completeness.
Line 68 says resp3-type-fidelity delivers XINFO STREAM with the missing fields. The task record says only the Map type is fixed and the field set remains incomplete. Change this text to Map shape and link the field-completeness follow-up, or keep the task open.
🤖 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/milestones/v0-9-client-compat/MILESTONE.md at line 68, Update the
resp3-type-fidelity milestone entry to state that XINFO STREAM is corrected to a
Map shape only, and link or reference the separate field-completeness follow-up
instead of claiming missing fields are delivered. Alternatively, keep the
milestone task open until field completeness is addressed.
| - [ ] A RESP3 subscriber receives subscribe/unsubscribe confirmations as Push frames and may issue non-pub/sub commands while subscribed, matching Redis (verify: `cargo test --test pubsub_resp3_push`) (← pubsub-resp3-push) | ||
| - [ ] A stock cluster-aware client bootstraps against a Moon cluster via CLUSTER SHARDS, and reports the topology unhealthy while slots are uncovered instead of accepting it (verify: `cargo test --test cluster_client_bootstrap`) (← cluster-client-bootstrap) | ||
| - [ ] `INFO` exposes every field the pinned monitoring-stack manifest reads, with no duplicate sections, and `INFO <section>` returns only that section (verify: `scripts/test-client-compat.sh --info-manifest`) (← info-observability) | ||
| - [ ] Every command answers `+QUEUED` inside MULTI and its real reply from EXEC, with identical values at 1, 2 and 4 shards — no command executes at queue time and no multi-key command silently returns empty (verify: `cargo test --test multi_exec_queue_semantics`) (← multi-exec-queue-semantics) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Limit +QUEUED to valid queueable commands.
Line 85 says every command returns +QUEUED inside MULTI. Line 69 already identifies queue-time errors and -EXECABORT. Transaction-control commands also have their own replies. Redis distinguishes successfully queued commands from queue-time syntax errors. (redis.io)
Update this criterion to cover valid queueable commands. Add separate assertions for queue-time errors and transaction-control commands.
Proposed criterion wording
-- Every command answers `+QUEUED` inside `MULTI` and its real reply from `EXEC`, ...
+- Every valid queueable command answers `+QUEUED` inside `MULTI` and its real reply from `EXEC`.
+- Queue-time errors and transaction-control commands are tested separately.🤖 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/milestones/v0-9-client-compat/MILESTONE.md at line 85, Update the
milestone criterion for MULTI/EXEC queue semantics to require +QUEUED only for
valid queueable commands, while preserving verification that EXEC returns their
actual replies across 1, 2, and 4 shards. Add separate assertions covering
queue-time syntax errors producing EXECABORT and transaction-control commands
returning their defined transaction replies.
Comment on lines
+287
to
+296
| "protocol-error-lifetime": { | ||
| "title": "Protocol errors reply and close cleanly, never stall or eat the valid prefix", | ||
| "phase": "ground", | ||
| "gate": "none", | ||
| "milestone": "v0-9-client-compat", | ||
| "depends_on": [ | ||
| "client-compat-harness" | ||
| ], | ||
| "created": "2026-08-09T16:42:35+00:00", | ||
| "updated": "2026-08-09T16:42:35+00:00" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the duplicate protocol-error ownership.
Lines 287-296 add protocol-error-lifetime, but the existing info-observability title at Lines 254-255 still includes protocol-error replies. .add/milestones/v0-9-client-compat/MILESTONE.md Line 72 says this scope moved out. Update the old title or define a non-overlapping boundary so task gates do not double-count the work.
🤖 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/state.json around lines 287 - 296, Update the existing
info-observability entry’s title in .add/state.json to remove its protocol-error
reply ownership, keeping protocol-error-lifetime as the sole owner of that scope
and aligning with the v0-9-client-compat milestone boundary.
| slug: resp3-type-fidelity · created: 2026-08-09 · stage: production | ||
| autonomy: auto <!-- inherited from the project default (PROJECT.md); explicit level: manual < conservative < auto (visible · overridable) — lower below if a high-risk task needs it, or run `add.py autonomy set`. --> | ||
| phase: ground <!-- ground -> specify -> scenarios -> contract -> tests -> build -> verify -> observe -> done --> | ||
| phase: done <!-- ground -> specify -> scenarios -> contract -> tests -> build -> verify -> observe -> done --> |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Synchronize task completion with the approval checklist.
.add/tasks/resp3-type-fidelity/TASK.md#L5-L5: Setphase: doneonly after the approval item at Line 441 is complete, or make that item explicitly non-gating..add/state.json#L223-L230: Setphase: doneandgate: PASSonly when they agree with the task record.
📍 Affects 2 files
.add/tasks/resp3-type-fidelity/TASK.md#L5-L5(this comment).add/state.json#L223-L230
🤖 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/resp3-type-fidelity/TASK.md at line 5, Synchronize completion
state with the approval checklist: in .add/tasks/resp3-type-fidelity/TASK.md at
lines 5 and 441, either complete the approval item before retaining phase: done
or explicitly make it non-gating; in .add/state.json at lines 223-230, set
phase: done and gate: PASS only when they agree with the task record.
Comment on lines
+249
to
+251
| ScoredPairs, // flat [m,s,…]-> [[m ,s],…] ZRANGE-family/ZDIFF/ZUNION/ZINTER WITHSCORES, | ||
| // ZPOPMIN/MAX <count>, ZRANDMEMBER WITHSCORES | ||
| ScoredFlat, // [m,s] -> [m ,s] ZPOPMIN/ZPOPMAX with no count |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'BZPOPMIN|BZPOPMAX|ScoredPairs|ScoredFlat' \
src/protocol/resp3.rs tests scriptsRepository: pilotspace/moon
Length of output: 12937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resp3.rs relevant classifier =="
sed -n '35,165p' src/protocol/resp3.rs
echo
echo "== TASK.md relevant sections =="
if [ -f .add/tasks/resp3-type-fidelity/TASK.md ]; then
sed -n '1,90p' .add/tasks/resp3-type-fidelity/TASK.md
echo "---"
sed -n '240,255p' .add/tasks/resp3-type-fidelity/TASK.md
echo "---"
sed -n '280,336p' .add/tasks/resp3-type-fidelity/TASK.md
else
echo "TASK.md not found"
fiRepository: pilotspace/moon
Length of output: 17051
Define a separate shape for blocking pops.
BZPOPMIN and BZPOPMAX now have key + timeout arguments and return the key, member, and score, so args.len() >= 2 would apply ScoredPairs and incorrectly pair-wrap and type-coerce the timeout. Add a dedicated blocking-pop shape or remove these commands from the classifier until covered.
🤖 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/resp3-type-fidelity/TASK.md around lines 249 - 251, The
response-shape classifier must handle BZPOPMIN and BZPOPMAX separately from
ScoredPairs: add a dedicated blocking-pop shape that preserves key, member,
score, and timeout semantics, or exclude both commands from the classifier until
supported. Update the relevant shape definitions and command classification
logic, preserving existing non-blocking pop behavior.
Comment on lines
+112
to
+121
| // ZPOPMIN/ZPOPMAX: `<count>` present -> wrapped pairs; absent -> ONE | ||
| // flat [member, score] pair. Redis really does change the nesting on | ||
| // the presence of the count. | ||
| b"ZPOPMIN" | b"ZPOPMAX" | b"BZPOPMIN" | b"BZPOPMAX" => { | ||
| if args.len() >= 2 { | ||
| Resp3Shape::ScoredPairs | ||
| } else { | ||
| Resp3Shape::ScoredFlat | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Redis 8 BZPOPMIN RESP3 reply type key member score double
💡 Result:
The BZPOPMIN command in Redis 8 returns an Array reply containing three elements when a member is successfully popped from a sorted set [1][2]. Under the RESP3 protocol, the structure of this array is: 1. The name of the key from which the member was popped (Bulk String) [1][3][2]. 2. The member that was popped (Bulk String) [1][3][2]. 3. The score of the popped member (Double / Floating Point number) [1][3][2]. If the command times out and no element is popped, it returns a Null reply (Nil) [1][2]. While RESP2 historically returned the score as a Bulk String (containing the numeric value as text), RESP3 standardizes the representation of numeric types, including double-precision floating-point numbers [4][5]. In the context of Redis clients and protocol handling, modern implementations treat these score values as floating-point numbers rather than raw byte strings to ensure consistency across different interfaces [4].
Citations:
- 1: https://redis.antirez.com/commands/content/commands/bzpopmin.md
- 2: https://valkey.io/commands/bzpopmin/
- 3: https://www.devdoc.net/database/redis-site-20210120/commands/bzpopmin.html
- 4: RESP2 and RESP3 response unification redis/redis-py#4025
- 5: https://github.com/redis/redis-specifications/blob/master/protocol/RESP3.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)resp3\.rs$|protocol'
echo "== outline resp3 =="
ast-grep outline src/protocol/resp3.rs --view expanded | sed -n '1,220p'
echo "== relevant code =="
sed -n '1,220p' src/protocol/resp3.rs
echo "== search Resp3Shape/pair =="
rg -n "Resp3Shape|pair_wrap|ScoredPairs|ScoredFlat|BZPOP|ZPOPMIN|ZPOPMAX" src/protocol src -g '*.rs'Repository: pilotspace/moon
Length of output: 28387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== resp3 implementation section =="
sed -n '200,255p' src/protocol/resp3.rs
echo "== tests around shape classification/output =="
sed -n '258,330p' src/protocol/resp3.rs
sed -n '390,515p' src/protocol/resp3.rs
echo "== blocking proxy/handlers around BZPOP =="
sed -n '288,345p' src/server/conn/blocking.rs
sed -n '1176,1212p' src/server/conn/blocking.rs
sed -n '1398,1424p' src/server/conn/blocking.rs
echo "== direct policy probes =="
python3 - <<'PY'
def parse_shape_source(path):
src = path.read_text().splitlines()
for i, line in enumerate(src, 1):
if 'ZPOPMIN' in line or 'BZPOPMIN' in line or 'BZPOPMAX' in line:
print(f"{i}: {''.join(src[max(0,i-3):min(len(src),i+3)])}")
from pathlib import Path
parse_shape_source(Path('src/protocol/resp3.rs'))
# Deterministic behavioral model from the relevant code:
# - score argument is last argument value token, not count.
# - args exclude command name.
# - BZPOPMIN args: [key, key*, timeout]; len >=2 for keys+timeout.
def args(strings):
return strings
def current_shape(cmd_upper, args):
if cmd_upper in (b"ZPOPMIN", b"ZPOPMAX", b"BZPOPMIN", b"BZPOPMAX"):
return "ScoredPairs" if len(args) >= 2 else "ScoredFlat"
return "None"
for argv in [
["z", "1"],
["z", "m", "1"],
["1", "2", "z", "m", "1"],
["z", "1", "1"],
["z", "m", "1"],
["z", "m" , "m", "1"],
]:
print(f"current_shape({argv!r}) =", current_shape("BZPOPMIN".encode(), args(argv)))
PYRepository: pilotspace/moon
Length of output: 16463
Classify BZPOPMIN/BZPOPMAX with a [key, member, double] shape.
BZPOPMIN key [key ...] timeout never accepts a <count> argument, while ZPOPMIN/ZPOPMAX do. The shared args.len() >= 2 branch selects either ScoredPairs or ScoredFlat based on key count, so BZPOPMIN does not get the [key, member, double] RESP3 conversion. Add a dedicated shape for that layout, or remove BZPOPMIN/BZPOPMAX from this policy until it exists.
🤖 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/resp3.rs` around lines 112 - 121, Update the command
classification around the RESP3 shape match to handle BZPOPMIN and BZPOPMAX
separately from ZPOPMIN and ZPOPMAX. Assign the blocking commands a dedicated
[key, member, double] shape, or remove them from this policy until that shape is
implemented; do not let the shared args.len() >= 2 logic classify them as
ScoredPairs or ScoredFlat.
Comment on lines
+441
to
+443
| results.push(super::util::apply_resp3_conversion( | ||
| cmd, cmd_args, response, proto, | ||
| )); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the single-shard transaction executor applies RESP3 conversion per queued command.
set -euo pipefail
# Locate every transaction executor and inspect how each pushes results.
rg -nP -C 10 'fn execute_transaction\b' src/
# Confirm which executors call the conversion helper.
rg -nP -C 3 'apply_resp3_conversion' src/Repository: pilotspace/moon
Length of output: 12139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^src/server/conn/shared\.rs$|^src/server/conn/handler_single\.rs$|^src/server/conn/util\.rs$)' || true
echo "== shared outline =="
ast-grep outline src/server/conn/shared.rs --view compact | sed -n '1,220p' || true
echo "== handler_single execute related =="
rg -n -C 20 'fn execute_transaction\b|execute_transaction\(|transaction|MULTI|EXEC|watched|command_queue|push\(.*apply_resp3_conversion|responses\.push' src/server/conn/handler_single.rs src/server/conn/shared.rs src/server/conn/mod.rs
echo "== util apply response conversion =="
sed -n '1,140p' src/server/conn/util.rsRepository: pilotspace/moon
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apply_resp3_conversion implementation =="
sed -n '63,130p' src/server/conn/util.rs
echo "== single-shard tokio transaction call sites =="
rg -n -C 8 'execute_transaction\(' src/server/conn src/server || true
echo "== single-shard transaction invocation context snippets =="
python3 - <<'PY'
from pathlib import Path
text = Path('src/server/conn/handler_single.rs').read_text()
markers = [
('execute_transaction', text.find('execute_transaction(')),
('MULTI', text.find('MULTI', 0)),
]
for name, idx in markers:
if idx == -1:
print(f'{name}: not found')
continue
start = max(0, idx - 250)
end = min(len(text), idx + 600)
print(f'--- around {name} at {idx} ---')
for i, line in enumerate(text[start:end].splitlines(), start - 120 + start):
if 120 <= i <= 600 or idx == -250 <= start <= idx - 120:
print(f'{i}: {line}')
PYRepository: pilotspace/moon
Length of output: 8893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handler_single around EXEC call =="
sed -n '1350,1430p' src/server/conn/handler_single.rs
echo "== sharded execute_transaction_sharded around dispatch result loop =="
sed -n '348,390p' src/server/conn/shared.rs
echo "== single shared execute_transaction result collection =="
sed -n '180,198p' src/server/conn/shared.rsRepository: pilotspace/moon
Length of output: 7919
Apply RESP3 conversion to single-shard transaction replies.
execute_transaction currently pushes dispatch results raw at src/server/conn/shared.rs:194, while execute_transaction_sharded converts each queued reply with its own command/args. An EXEC’d HGETALL, FT.SEARCH, or similar RESP3 map reply can return a flat Array on the single-shard tokio path; convert the same way and pass the executor the connection’s protocol version.
🤖 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 441 - 443, Update execute_transaction
so each raw dispatch result is passed through
super::util::apply_resp3_conversion with the corresponding command, command
arguments, response, and the executor connection’s protocol version before being
pushed. Match execute_transaction_sharded’s per-command conversion behavior
while preserving result ordering.
Comment on lines
+822
to
+861
| #[test] | ||
| fn r3f11_resp2_is_byte_identical() { | ||
| let dir = tempfile::tempdir().expect("tempdir"); | ||
| let (child, port) = spawn_moon(dir.path(), 1); | ||
| let _g = ServerGuard(child); | ||
|
|
||
| let cases: &[(&[&[&str]], &[&str])] = &[ | ||
| ( | ||
| &[&["DEL", "z"], &["ZADD", "z", "1", "a", "2", "b"]], | ||
| &["ZRANGE", "z", "0", "-1", "WITHSCORES"], | ||
| ), | ||
| ( | ||
| &[&["DEL", "h"], &["HSET", "h", "f", "v"]], | ||
| &["HGETALL", "h"], | ||
| ), | ||
| (&[&["DEL", "s"], &["SADD", "s", "a"]], &["SMEMBERS", "s"]), | ||
| ( | ||
| &[&["DEL", "s2"], &["SADD", "s2", "a"]], | ||
| &["SISMEMBER", "s2", "a"], | ||
| ), | ||
| (&[&["SET", "f", "10.5"]], &["INCRBYFLOAT", "f", "0.1"]), | ||
| (&[], &["CONFIG", "GET", "maxmemory"]), | ||
| ( | ||
| &[&["DEL", "z2"], &["ZADD", "z2", "1.5", "a"]], | ||
| &["ZSCORE", "z2", "a"], | ||
| ), | ||
| ]; | ||
|
|
||
| for (setup, cmd) in cases { | ||
| let got = standalone(port, 2, setup, cmd); | ||
| let shape = got.shape(); | ||
| for forbidden in ['%', '~', ',', '#', '='] { | ||
| assert!( | ||
| !shape.contains(forbidden), | ||
| "RESP2 reply for {cmd:?} contains the RESP3-only type byte '{forbidden}': {shape}. \ | ||
| RESP2 is a hard invariant — a RESP3 fix must never leak into it." | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test checks type-byte absence, not byte identity.
r3f11_resp2_is_byte_identical asserts only that the RESP2 shape() string contains none of %, ~, ,, #, =. It never compares the RESP2 bytes to a recorded baseline. A RESP2 reply could change its element count, its ordering, or its payload and still pass.
The CHANGELOG entry states "RESP2 is byte-identical — pinned by a test written before the fix." That claim is stronger than what this test proves. Either rename the test to match its scope, for example r3f11_resp2_has_no_resp3_type_bytes, or add a recorded-baseline byte comparison.
🤖 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/resp3_type_fidelity.rs` around lines 822 - 861, The test
r3f11_resp2_is_byte_identical only checks for forbidden RESP3 type bytes and
does not prove byte identity. Either rename it to reflect that scope, such as
r3f11_resp2_has_no_resp3_type_bytes, or add recorded-baseline byte comparisons
for each RESP2 response so payload, ordering, and element counts are verified.
Comment on lines
+946
to
+949
| ] { | ||
| let got = standalone(port, 3, &[&["DEL", "r3f13:nokey"][..]], cmd); | ||
| assert_eq!(got.shape(), want, "{what}. Got {got:?}"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Delete the key each case actually reads.
Line 947 passes the same setup for all four cases: DEL r3f13:nokey. The cases for SMEMBERS r3f13:noset and ZRANGE r3f13:noz never delete their own keys. The test passes today only because nothing in this binary writes those keys. The miss path is therefore asserted by accident, not by construction. If the suite later reuses a server or a persisted data directory, the assertion silently changes meaning.
Carry the key to delete in the case table.
🐛 Proposed fix
- for (cmd, want, what) in [
+ for (key, cmd, want, what) in [
(
+ "r3f13:nokey",
&["HGETALL", "r3f13:nokey"][..],
"%0[]",
"an empty HGETALL is still a Map",
),
(
+ "r3f13:unused",
&["CONFIG", "GET", "r3f13-no-such-param"][..],
"%0[]",
"an empty CONFIG GET is still a Map",
),
(
+ "r3f13:noset",
&["SMEMBERS", "r3f13:noset"][..],
"~0[]",
"an empty SMEMBERS is still a Set",
),
(
+ "r3f13:noz",
&["ZRANGE", "r3f13:noz", "0", "-1", "WITHSCORES"][..],
"*0[]",
"an empty scored range stays an Array — emptiness must not promote it",
),
] {
- let got = standalone(port, 3, &[&["DEL", "r3f13:nokey"][..]], cmd);
+ let got = standalone(port, 3, &[&["DEL", key][..]], cmd);
assert_eq!(got.shape(), want, "{what}. Got {got:?}");
}📝 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
| ] { | |
| let got = standalone(port, 3, &[&["DEL", "r3f13:nokey"][..]], cmd); | |
| assert_eq!(got.shape(), want, "{what}. Got {got:?}"); | |
| } | |
| for (key, cmd, want, what) in [ | |
| ( | |
| "r3f13:nokey", | |
| &["HGETALL", "r3f13:nokey"][..], | |
| "%0[]", | |
| "an empty HGETALL is still a Map", | |
| ), | |
| ( | |
| "r3f13:unused", | |
| &["CONFIG", "GET", "r3f13-no-such-param"][..], | |
| "%0[]", | |
| "an empty CONFIG GET is still a Map", | |
| ), | |
| ( | |
| "r3f13:noset", | |
| &["SMEMBERS", "r3f13:noset"][..], | |
| "~0[]", | |
| "an empty SMEMBERS is still a Set", | |
| ), | |
| ( | |
| "r3f13:noz", | |
| &["ZRANGE", "r3f13:noz", "0", "-1", "WITHSCORES"][..], | |
| "*0[]", | |
| "an empty scored range stays an Array — emptiness must not promote it", | |
| ), | |
| ] { | |
| let got = standalone(port, 3, &[&["DEL", key][..]], cmd); | |
| assert_eq!(got.shape(), want, "{what}. Got {got:?}"); | |
| } |
🤖 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/resp3_type_fidelity.rs` around lines 946 - 949, Update the case table
to include the key each command reads, then use that per-case key in the DEL
setup inside the test loop instead of hardcoding r3f13:nokey. Ensure SMEMBERS
deletes r3f13:noset, ZRANGE deletes r3f13:noz, and each case explicitly
constructs its own missing-key path.
…ixed now The harness self-test test_a_diverging_entry_exits_one_and_names_the_divergence asserts that a diverging entry exits 1 and names TYPE. It borrowed a REAL divergence as its fixture: SISMEMBER under RESP3, where Moon over-converted Integer to Boolean. This PR fixes that over-conversion, so the fixture stopped diverging and the test began failing with '0 != 1' — it failed as a reward for fixing something, which is backwards. Re-pointed at COMMAND COUNT (Redis :274 vs Moon *0), an open TYPE divergence owned by client-identity-introspection, so the reply-type line of work cannot silently retire it again. This is the THIRD fixture for this test (GET-inside-MULTI -> SISMEMBER -> COMMAND COUNT); the comment now records the rotation so the next person sees the pattern instead of rediscovering it. No permanent-by-construction TYPE divergence exists to use instead: Moon's proprietary commands return an Error on both servers, and redis 8.6.1 implements 'hotkeys' as well. The durable fix is a test-only injection hook that fabricates a divergence rather than borrowing one — tracked in #461. Caught by CI, not locally: the client-compat job runs this Python suite in addition to the harness itself, and I had only run the harness. author: Tin Dang
…te on GCE Records the performance check for PR #463 on both arches. The change adds a shape classification at enqueue and a conversion at the reply exit, both gated on proto >= 3; redis-benchmark speaks RESP2, so this measures the cost of the GATE, which is what could regress existing workloads. Result: no regression detectable on either arch. x86 median delta +0.00% with 0/18 rows outside their noise floor; ARM +0.72% with 1/18 (a marginal GAIN, not claimed -- Redis moved in the same direction on that row). Method note worth keeping: the first attempt was a single full bench-compare matrix, and it was DISCARDED. It showed apparent Moon regressions of -16.3%, -15.5% and -11.4% on x86 -- but Redis, unchanged code benchmarked in both legs, drifted up to -27.0% between the same legs. When the control moves further than the subject, no per-row number means anything. Replaced with an interleaved 5-repetition A/B (main -> branch alternating) so drift hits both legs equally, with Redis re-measured every repetition. The record states the supportable claim (any effect is below a 3.6-6.0% noise floor) rather than the unsupportable one (there is no regression). author: Tin Dang
Collaborator
Author
GCE performance gate: no regression detectable on either archRecorded in
Grid: {SET, GET, INCR, LPUSH, SPOP, HSET} × p={1, 8, 64}, 50 clients, shards=1, persistence off both sides. The first measurement was thrown away, and that's the part worth readingA single full When the control moves further than the subject, no per-row number is interpretable. Reporting either "GET regressed 11%" or "no regression" off that data would have been unfounded. So it was replaced with an interleaved A/B: 5 repetitions alternating main → branch → main → branch, so thermal and neighbour drift hits both legs equally, with Redis re-measured every repetition as an ongoing control. The "noise floor" per row is the worst within-leg CV across all four series. What is and isn't claimed
Coverage caveat, stated plainly
Also verified rather than assumed: the two legs really were distinct binaries. Their output files came back byte-identical in size, which looks exactly like the "same binary ran twice" failure mode — a content diff confirmed 76 (ARM) / 70 (x86) differing lines. |
Merging main (which now carries #465) back into this branch re-created the duplicate `### Fixed` under `[Unreleased]`: this branch predates the restructure that #465 applied, so it re-added its own heading above the existing one. Two `### Fixed` sections in one release is the exact defect flagged on #465 — Keep a Changelog allows one section per change type, and a parser that indexes by heading reads one block and silently drops the other. Merging as-is would have regressed main straight back into it, and no CI check looks at changelog structure. Folded into the single section, preserving main's `Added -> Security -> Fixed` order and both entries. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 10, 2026
… not the shipped one (#464) * ci(test): run the monoio suite — CI was testing the fallback runtime, not the shipped one Every CI job that EXECUTED tests did so under `--no-default-features --features runtime-tokio,...`. Moon's default feature set is runtime-monoio, and that is what ships on Linux. The result: 26 monoio integration test files and 30 monoio-gated src/ files were unreachable by CI, and the documented local gate in CLAUDE.md ("Local CI Parity", which runs BOTH suites) was strictly stronger than CI itself. That gap is not theoretical. The v0.8.6 inline-GET ACL bypass (#457) was wrong only on the monoio dispatch path and shipped green. The RESP3 type-fidelity work (#463) had to hand-verify one of its two enqueue sites locally, because CI structurally could not see it. Adds `check-monoio`: self-hosted Linux runner (the only place monoio's io_uring driver executes at all), default feature set, `cargo nextest run --profile ci`, its own CARGO_TARGET_DIR, no continue-on-error, MOON_NO_URING deliberately unset. `--profile ci` is load-bearing rather than incidental: the suite has a known load-sensitive flake class, a bare `cargo test` has no retries, and an intermittently-red required job gets disabled -- which is worse than no job, because it still looks like coverage. The existing profile's retries=2 absorbs it while still reporting FLAKY, so the signal survives. Measured on moon-dev (kernel 6.17) before landing: 5145 passed, 1 flaky, 244 skipped, exit 0, 80.3s of test time. VERIFIED BY NEGATIVE CONTROL, not by inspection. A CI-config change can be green and still be worthless, so the claim was tested directly: a deliberate defect injected on `try_inline_dispatch` (cfg(feature = "runtime-monoio"), so tokio cannot reach it) making inline GET answer "$6\r\nBROKEN\r\n": tokio (CI before this change) : multi_queues_inline_get 6 passed <- ships green monoio (the new job) : multi_queues_inline_get 3 FAILED <- caught Reverted immediately; zero residual markers, and the suite back to 6/6. tests/ci_covers_monoio.rs guards the job itself, because the failure mode of CI coverage is silent -- a job that stops running or is switched to the wrong feature set looks exactly like a green build. It fails on a wrong feature set, continue-on-error, a bare `cargo test`, a shared target dir with the tokio job, or removal of tokio coverage. NOTE FOR THE REVIEWER: adding the job makes it RUN, not BLOCK. It must be added to branch protection to gate merges; until then it is advisory. Refs: .add/tasks/monoio-ci-coverage (gate PASS), milestone v0-9-client-compat author: Tin Dang * ci(fix): the monoio job was running io_uring disabled — MOON_NO_URING was a workflow-level global `check-monoio` exists for one reason: to execute the io_uring driver that actually ships on Linux. Its comment said "MOON_NO_URING is deliberately NOT set here", its job block was clean, and `ci_covers_monoio.rs` asserted that block stayed clean. All three were checking the wrong scope. `MOON_NO_URING: "1"` sat in the workflow-level `env:`, which merges into every job — and a job cannot unset an inherited key (an empty value is still a set variable to `env::var_os`). So the one job whose entire premise was io_uring ran with io_uring force-disabled, and had done since the job was written. `monoio_yield_overhead_is_microscopic` reported it: every `cooperative_yield()` fell through `uring_active()` to the `sleep(ZERO)` timer park, 290ms for 200 yields = 1.45ms/yield, against a 100ms budget. That read like a load-sensitive flake on a shared runner. It was not — same binary on moon-dev, the env var as the only delta: io_uring active -> ok, 0.00s MOON_NO_URING=1 -> FAILED, 0.59s Deterministic in both directions. The test was right and the config was wrong. Fix: MOON_NO_URING moves out of workflow-level env onto the jobs that want it — `check` (tokio; the io_uring bridge floods errors under load), `memory-steady- state` (real server on a GitHub-hosted runner where io_uring may be seccomp- restricted), and `client-compat` (kept so its recorded waiver baseline stays comparable; the differ probes wire shapes, not drivers — commented as such, so it is documented rather than silent). Dropped from macOS/Windows/msrv/console/ lint, where it was inherited dead config: no io_uring on those platforms and those jobs execute no Moon. `ci_covers_monoio.rs` gains the assertion it was missing — the workflow-level `env:` block must not define MOON_NO_URING either. Red before the yaml change for exactly that reason, green after; 5/5. Verified on moon-dev with the job's exact fixed config (default features, io_uring live, nextest --profile ci), which no CI run had ever exercised. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 12, 2026
…eding a live bug Two findings from the client-compat harness, which is the check that failed on PR #471 — both real, neither visible to the 14 raw-RESP tests in this task's own suite. 1. ROLE executed at QUEUE time inside MULTI. The first cut intercepted ROLE at the connection layer, ahead of the MULTI queueing step, because its answer lives on ConnectionContext.repl_state rather than in the Database that dispatch() receives. So `MULTI; ROLE; EXEC` replied the role array immediately and EXEC then returned `*0`. The damage is worse than a wrong reply: the command silently vanishes from the EXEC array, so every LATER result shifts down one index and a client reads another command's answer as this one's. Redis queues ROLE like any other command. Fixed by answering ROLE from the shared dispatch table instead, reading the process-global replication handle that INFO already uses and that every entry point registers (main.rs, listener.rs, embedded.rs). That is the only placement where a queued ROLE can work at all, since EXEC replays the queue through dispatch() — and it deleted all three per-handler intercepts, so ROLE now lives in exactly two places instead of five. Verified on the wire: `MULTI; ROLE; PING; EXEC` returns `*2` with the role array first and +PONG still last, and a live replica still reports `slave` through the global handle. ci15 pins the alignment; it fails under both prior states (`*0`, and `*1` with an unknown- command error). Nothing in this task's suite exercised a connection-layer command INSIDE a transaction, which is the coverage lesson: a new intercept has to be tested in MULTI as well as standalone, because its POSITION relative to queueing is the thing that can be wrong. 2. The harness's own divergence test needed a live Moon defect to pass. test_a_diverging_entry_exits_one_and_names_the_divergence borrowed a real bug as its fixture, so it FAILED whenever someone FIXED that bug — a test that punishes the fix. Three fixtures had already been burned this way (GET-inside-MULTI #457, SISMEMBER RESP3 #463, and COMMAND COUNT, retired by this very task, which is what turned the PR red). The durable fix was already designed and filed as #461, so it is implemented here rather than rotating to a fourth defect: a test-only `inject_moon_reply` hook fabricates the divergence, with a guard test asserting the shipped manifest never uses one. Proven load-bearing by disabling the hook, which turns the test red. Manifest: the identity_command_count and identity_role waivers are retired as fixed, and COMMAND INFO / COMMAND GETKEYS / RESET are added as live entries. COMMAND INFO carries a new, accurate waiver — its 10-field SHAPE now matches, but acl_categories is thin (@string where Redis says @READ @string @fast), key_specs is empty, and under RESP3 Redis types flags/acl_categories as Sets and key_specs entries as Maps where Moon emits Arrays. Owners recorded. Harness result against redis-server 8.6.1: PASS=181 FAIL=0 WAIVED=19 (was PASS=179 FAIL=8 before this commit). Gates: fmt, clippy (default + tokio/jemalloc, --all-targets), the identity suite (15 scenarios), and both compat suites (34 differ + 20 e2e) green. Note the unreachable-pattern warning caught during this work: (4, b'r') already existed in dispatch_inner for RPOP, so the first ROLE arm was dead code and EXEC still answered unknown command until it was merged into the existing arm. author: Tin Dang
TinDang97
added a commit
that referenced
this pull request
Aug 12, 2026
…#471) * docs(add): close watch-cas-transactions at PASS, record what the scope gate did not prove The code shipped in #470; this closes the task record. The first `gate PASS` was refused with `scope_violation` naming 7020 files, including `.github/workflows/*.yml` and `Cargo.lock` — paths this task never touched. Cause: the scope baseline is a whole-tree byte snapshot taken at the tests->build crossing, with no git awareness. Between that snapshot and the gate, five dependabot PRs merged into main (#420, #417, #346, #358, #359) and target/ was rebuilt, so every unrelated change read as "this task touched it". Rather than re-snapshot quietly, §6 now records: - the task's ACTUAL change set from `git show --name-only 151a185 1c12af0`, every path of which is inside the declared §5 Scope; - the two entries that needed naming instead of glossing — `conn/mod.rs` (one line, `pub mod watch;`, undeclared) and `.add/tooling/add.py` (25 lines of engine fix carried in from an earlier session, genuinely outside §5); - that the re-taken baseline is the post-merge tree, so the scope walk at this gate compares the tree against itself and proves nothing. The evidence that the build stayed in scope is the git file list, not the green gate. Also ticks the §6 verify checklist against evidence already gathered: the stubbed-counter A/B (reverting only next_birth_version turns 4 new tests red, so the green is earned), the 41->1 tokio failure delta traced to a diskfull $TMPDIR rather than to the change, and the 9/9 dispatched matrix including the Windows/macOS/console jobs no PR run executes. Adds .add/.gitignore, which `add.py init` scaffolds but this project never got. Without it the two 40MB scope-snapshot.json sidecars are commit candidates; they are regenerable working state, and the durable scope declaration is the state.json anchor. One 436K sidecar is already tracked from an older task and is left alone. Method delta for the next loop: as built, any task gated after unrelated merges land inherits an unfalsifiable scope violation. The anchor wants to be a git tree-ish, or the walk should skip gitignored paths and diff the merge-base. author: Tin Dang * feat(command): derive COMMAND/ROLE/RESET/HELLO from real server state Moon's client-identity surface answered from compile-time constants instead of from the server. The failures were invisible by eye because redis-cli renders `:0` and `*0` identically as "0", so every assertion here is on raw RESP bytes. COMMAND and COMMAND COUNT each returned the OTHER'S TYPE: bare COMMAND replied `:0` (an Integer where an Array belongs) and COMMAND COUNT replied `*0` (an Array where an Integer belongs). COMMAND INFO/DOCS/LIST/GETKEYS all replied an empty array. A driver that builds its command map at connect time does not read that as "unsupported" — it reads it as a protocol violation. All six now derive from the COMMAND_META phf registry (263 commands), so registering a command is what makes it introspectable and there is no second table to drift. ROLE and RESET were unknown commands. RESET was registered in the metadata table with full flags while dispatch rejected it — the same advertise-then- reject class as WATCH/UNWATCH before v0.8.6 — and a partial RESET existed only inside handler_sharded's subscribe-mode loop, so it worked if you happened to be subscribed on one runtime and nowhere else. RESET's "default state" is taken from restore_migrated_state(None, ..), the same function ConnectionState::new uses, so it cannot drift from what a fresh connection means by default. HELLO contradicted INFO replication on the same connection: hello_acl built `mode`/`standalone` and `role`/`master` as literals, so a replica announced itself a master. Both fields now read ReplicationState/ClusterState. Redis deliberately uses three vocabularies for this one fact (HELLO -> replica, INFO -> slave, ROLE -> slave); that was measured against a live replica pair on redis-server 8.6.1, and it corrected this task's own frozen contract, which had said "slave" everywhere. The test was made stronger rather than matched to the code: ci12 asserts all three surfaces agree, each in its own vocabulary, plus a negative assertion that HELLO no longer claims master. CLIENT INFO / CLIENT LIST reported the literal `laddr=127.0.0.1:0` from inside the format string, so every client on every listener showed port 0. It now carries the real local address. Wired on all three connection handlers plus the inline fast path. ROLE and RESET cannot ride the shared dispatch table — their answers live on ConnectionContext/ReplicationState, not in the Database that dispatch() receives — so each handler needs its own intercept, and handler_single had neither. That handler is reachable only via listener::run_with_shutdown (an in-process tokio API used by a few test suites; main.rs and embedded.rs both route through run_sharded), which is precisely why it had drifted, and why an A/B caught the gap: reverting the fix there left ci12 green. ci14 drives that handler in-process and goes red with `-ERR unknown command 'ROLE'` without it. Tests: 14 raw-RESP scenarios in tests/client_identity_introspection.rs, with parity legs on the monoio and sharded handlers and the inline path. Startup is serialised across the suite's threads — 13 servers initialising data dirs at once left one unable to answer PING inside 30s at ~1 run in 8; a longer timeout would have been slower and still flaky, and a shared OnceLock server would have leaked a live moon past exit because statics are never dropped. 32/32 green at --test-threads=13 after the fix. Found and filed, not folded in: response batches are serialised at flush time using the FINAL protocol version, so a protocol-changing command retro-encodes earlier replies in the same batch (`HELLO 3` alone -> `%7`; `HELLO 3` + `HELLO 2` in one write -> `*14`). Pre-existing — that reproducer touches none of this code — but RESET, which reverts the protocol by contract, adds a second trigger. Filed as ADD task batch-protocol-version-fidelity with the measurement table. MONITOR was split out to monitor-command-feed at freeze: it is a stream rather than a reply, the only item touching the per-command hot path, and the only one exposing other clients' credentials. Gates: cargo fmt --check, clippy (default + tokio/jemalloc, --all-targets), cargo test --release, and the full tokio CI-parity suite all green. author: Tin Dang * fix(command): queue ROLE inside MULTI, and stop the compat harness needing a live bug Two findings from the client-compat harness, which is the check that failed on PR #471 — both real, neither visible to the 14 raw-RESP tests in this task's own suite. 1. ROLE executed at QUEUE time inside MULTI. The first cut intercepted ROLE at the connection layer, ahead of the MULTI queueing step, because its answer lives on ConnectionContext.repl_state rather than in the Database that dispatch() receives. So `MULTI; ROLE; EXEC` replied the role array immediately and EXEC then returned `*0`. The damage is worse than a wrong reply: the command silently vanishes from the EXEC array, so every LATER result shifts down one index and a client reads another command's answer as this one's. Redis queues ROLE like any other command. Fixed by answering ROLE from the shared dispatch table instead, reading the process-global replication handle that INFO already uses and that every entry point registers (main.rs, listener.rs, embedded.rs). That is the only placement where a queued ROLE can work at all, since EXEC replays the queue through dispatch() — and it deleted all three per-handler intercepts, so ROLE now lives in exactly two places instead of five. Verified on the wire: `MULTI; ROLE; PING; EXEC` returns `*2` with the role array first and +PONG still last, and a live replica still reports `slave` through the global handle. ci15 pins the alignment; it fails under both prior states (`*0`, and `*1` with an unknown- command error). Nothing in this task's suite exercised a connection-layer command INSIDE a transaction, which is the coverage lesson: a new intercept has to be tested in MULTI as well as standalone, because its POSITION relative to queueing is the thing that can be wrong. 2. The harness's own divergence test needed a live Moon defect to pass. test_a_diverging_entry_exits_one_and_names_the_divergence borrowed a real bug as its fixture, so it FAILED whenever someone FIXED that bug — a test that punishes the fix. Three fixtures had already been burned this way (GET-inside-MULTI #457, SISMEMBER RESP3 #463, and COMMAND COUNT, retired by this very task, which is what turned the PR red). The durable fix was already designed and filed as #461, so it is implemented here rather than rotating to a fourth defect: a test-only `inject_moon_reply` hook fabricates the divergence, with a guard test asserting the shipped manifest never uses one. Proven load-bearing by disabling the hook, which turns the test red. Manifest: the identity_command_count and identity_role waivers are retired as fixed, and COMMAND INFO / COMMAND GETKEYS / RESET are added as live entries. COMMAND INFO carries a new, accurate waiver — its 10-field SHAPE now matches, but acl_categories is thin (@string where Redis says @READ @string @fast), key_specs is empty, and under RESP3 Redis types flags/acl_categories as Sets and key_specs entries as Maps where Moon emits Arrays. Owners recorded. Harness result against redis-server 8.6.1: PASS=181 FAIL=0 WAIVED=19 (was PASS=179 FAIL=8 before this commit). Gates: fmt, clippy (default + tokio/jemalloc, --all-targets), the identity suite (15 scenarios), and both compat suites (34 differ + 20 e2e) green. Note the unreachable-pattern warning caught during this work: (4, b'r') already existed in dispatch_inner for RPOP, so the first ROLE arm was dead code and EXEC still answered unknown command until it was merged into the existing arm. author: Tin Dang
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
RESP3 reply types now match real Redis, and a command answers the same shape in every context.
A live differential sweep against
redis-server8.6.1 found the conversion table wrong in both directions and, structurally, unable to be right: it keyed only on the command name, whileWITHSCORES,WITHVALUESand a<count>argument are what actually decide the reply shape.Under-conversion (client-breaking)
ZRANGE … WITHSCORES(and the whole Z-range family,ZDIFF/ZUNION/ZINTER/ZRANDMEMBER) arrived as a flat array of bulk strings instead of pair-wrapped[member, Double]. An unmodified redis-py raisesValueError: not enough values to unpackon that, so every RESP3 application reading sorted-set scores was broken outright:Also missing entirely:
ZMSCORE,GEOPOS,SPOP <count>(Set),ZPOPMIN/ZPOPMAXDouble scores,XINFO STREAM(Map).HRANDFIELD WITHVALUESandZRANDMEMBER WITHSCORESanswered a Map where Redis answers an array of pairs.Over-conversion (equally wrong)
SISMEMBER,HEXISTS,EXPIRE,PEXPIRE,PERSIST,SETNX,MSETNXreturned Boolean where Redis returns Integer.INCRBYFLOAT/HINCRBYFLOATreturned a lossy Double (,10.6) where Redis returns the exact Bulk"10.59999999999999964".Emptiness changed the type
HGETALLandCONFIG GETon a miss answered*0where Redis answers%0— so a client dispatching on the type byte broke on exactly the path it hits most.How
The conversion is decided by
(command, args)at one policy choke point (Resp3Shapeinsrc/protocol/resp3.rs) instead of by 11 call sites across three handlers.The cross-shard reply loop no longer has the command's args by the time its batch returns, so the shape is classified at enqueue time and a 1-byte
Copytag travels inRemoteMeta— no per-command allocation on the shard hot path, and classification is gated onproto >= 3so RESP2 pays one integer compare.execute_transaction_shardednow takes the connection's protocol version and converts each inner reply with its own command — previouslySMEMBERSwas a Set outside a transaction and a flat Array inside one.CONFIG GET(Map) andCLIENT INFO(Verbatim) are fixed at their intercepts, which short-circuit the dispatch exit entirely; that is whyCONFIGcould never be fixed by editing the table before.Verification — all against a live
redis-server8.6.1, never against Moon's own expectationstests/resp3_type_fidelity.rs(new, 13 tests asserting the wire type byte)protocol::resp3unit teststest-client-compat.sh --strict-D warnings, both feature sets ·cargo fmt --check:6381flag setAll 13 now-stale waivers were deleted, and the surviving reasons re-attributed — 4 moved to
multi-exec-queue-semantics(transaction queueing defects, never reply-type ones).RESP2 is byte-identical, pinned by
r3f11, which was written and green before the fix.Two disclosures
A unit test was deleted.
an_empty_map_reply_passes_throughasserted "HGETALL of a missing key: an empty array, not an empty map". I wrote it earlier in this same change from an assumption rather than the oracle, and it is false — Redis answers%0. Being green, it read as coverage while actually locking in the bug. Replaced by a test that fails on the pre-fix binary. The replacement is strictly stronger and the code moved to meet it, not the reverse.The monoio suite's 2 failures are pre-existing, and that was measured rather than argued. A pre-change server was built from
main(4c9bd2c5), both binaries snapshotted before either leg ran, and verified behaviourally distinct first:main(pre-change)Same assertion, same line, same key counts on both legs; the pre-change server fails more. Filed as #459 — it includes a genuine
recovered > livefinding (restart resurrecting evicted keys) that deserves its own look.Blind spot this closes
Every one of the harness's 152 cases populated its key before asking, so the entire miss path was undiffed — which is how two wrong type bytes survived a fully green run. Five miss-path cases were added and verified discriminating by running them against the pre-fix binary, where all five fail.
Follow-ups filed rather than folded in
#459
dbsize_offload_logicalfragility + key resurrection · #460 remove now-deadRemoteMeta.cmd_name(drops aBytesclone per cross-shard command) · #461 harness silently trusts a staleMOON_BIN· #462 intercepts bypass the conversion choke pointKnown remainder, waived and reasoned:
XINFO STREAMis now the right type but still reports 7 fields to Redis's 16 — the missing ones need real stream bookkeeping and are tracked separately rather than fabricated.Task record:
.add/tasks/resp3-type-fidelity/TASK.md(gate PASS) · milestonev0-9-client-compatSummary by CodeRabbit
Bug Fixes
CONFIG GETandCLIENT INFO.Tests