fix(protocol): RESP3 keyed sorted-set pops send Double scores (#559) - #573
Conversation
|
ⓘ 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 Limit details: You’ve used the included review currently available. 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (7)
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 |
31210c5 to
637beb4
Compare
…h the choke point BZPOPMIN/BZPOPMAX/ZMPOP/BZMPOP answered `$3\r\n1.5` where redis-server 8.x answers `,1.5`. Every one of those replies is built by Redis's `genericZpopCommand`, which emits the score through `addReplyDouble` — the same call ZPOPMIN uses, and the same call Moon already honoured for ZPOPMIN. Measured both before and after with a raw-socket type-byte sweep of the whole score-replying family (RESP2 + RESP3 x standalone/MULTI); the RESP2 column is identical byte for byte across the change. Two independent causes, both fixed at the ONE seam rather than per command: 1. Classification (`protocol::resp3::resp3_shape_of`). BZPOPMIN/BZPOPMAX shared an arm with ZPOPMIN, whose rule is "a second argument is a COUNT, so the reply is pair-wrapped". A blocking pop's second argument is the TIMEOUT, so the simplest possible call classified as ScoredPairs — and because the reply is the 3-element `[key, member, score]`, the pair-wrapper saw an odd length and passed it through untouched. The bug was therefore invisible: a shape was assigned, it just could never fire. ZMPOP/BZMPOP had no rule at all. They now have their own shapes, KeyedScoredFlat and KeyedScoredPairs, and the inner score re-typing reuses `scored_flat`/`bulk_to_double` so ZPOPMIN and the keyed forms cannot drift apart. 2. Path (moon#462's class). The monoio handler's blocking branch is an INTERCEPT: it short-circuits the dispatch exit where every other reply meets the RESP3 policy, and it never applied that policy itself. So on the SHIPPED runtime the whole blocking family answered RESP2 shapes to RESP3 clients, while the tokio handler — which does convert at its blocking site — was right. Fixed by routing the reply through `apply_resp3_conversion`, the same choke point, not a second table. The blocking-in-MULTI executor now converts too: its "deliberately no conversion here" note was written when the live monoio path did not convert either, and the two are equal again with both converting, on the side that matches redis-server. Also pre-registered, inert until the commands land, so the reply is right the day they do: ZRANK/ZREVRANK WITHSCORE (Double score, moon#521 / PR #564) and ZADD ... INCR (Double reply). Both classified POSITIONALLY — a member literally named WITHSCORE or INCR is not the modifier, and `zadd_has_incr` stops scanning at the end of the leading flag run exactly where Redis's parser does. Verified unchanged against the oracle: GEODIST (`addReplyDoubleDistance` -> bulk in both protocols) and ZSCAN scores stay BulkStrings. Second fix, found by the same sweep: ZREVRANGE carried arity 4 where Redis has -4. The MULTI queue gate is the only consumer of that number, so the optional WITHSCORES turned a legal command into a wrong-arity error at QUEUE time and aborted the whole transaction — while the identical command answered fine standalone, which is why no suite ever saw it. Tests (red first; the four integration tests failed with `*3[$]` vs `*3[$|,]` before the fix): tests/resp3_type_fidelity.rs r3f14 keyed pops carry a Double standalone, in MULTI and pipelined r3f15 RESP2 stays three BulkStrings AND the Double text equals the bulk text r3f16 a pop that really BLOCKED and was woken by another client r3f17 shape is shard-independent (BZPOPMIN intercept + ZMPOP cross-shard tag) r3f18 ZREVRANGE WITHSCORES is queueable inside MULTI src/protocol/resp3.rs (unit) blocking_pops_are_keyed_not_the_zpopmin_rule zrank_withscore_is_positional zadd_incr_is_a_double_and_only_in_the_flag_run keyed_scored_flat_types_only_the_score keyed_scored_pairs_types_every_inner_score Gates: lib 4714/4714, resp3_type_fidelity 18/18 under BOTH runtimes (monoio and runtime-tokio), blocking/multi/pubsub/resp integration suites green, fmt, clippy --all-targets -D warnings, tokio feature check, audit-unsafe, audit-unwrap, client-compat differ self-tests 34/34. Refs #559, #462 author: Tin Dang
637beb4 to
0914edd
Compare
1 participant
Closes #559.
Under RESP3, zset scores must be Doubles (
,score). BZPOPMIN/BZPOPMAX fell into ZPOPMIN's count-form arm (args.len() >= 2is true forkey timeout), so the 3-element[key, member, score]reply passed through with a BulkString score; ZMPOP/BZMPOP were unclassified.Through the choke point (
resp3.rs): new shapesKeyedScoredFlat+KeyedScoredPairs; the blocking intercept and blocking-in-MULTI arms now route throughapply_resp3_conversionlike every other arm. Forward-compatible rules pre-registered for ZRANK/ZREVRANK WITHSCORE (#564) and ZADD INCR — inert until those land. Bonus fix: ZREVRANGE arity4→-4(WITHSCORES inside MULTI was EXECABORT-ing).Full divergence table measured raw-socket vs Redis 8 oracle semantics on both protocol versions; RESP2 byte-identical everywhere (asserted). Red-first: 5 new integration tests failed on the pre-fix binary. Gates: lib 4714 passed, fidelity 18/18 on both runtimes, fmt/clippy/audits/differ 0. 5 new unwaived client-compat manifest probes exercise the oracle on the compat leg.
Deliberately out of scope (filed as #568): GEOSEARCH/GEORADIUS WITHCOORD coordinates.