fix(blocking): multi-key blocking pops serve exactly once; refuse spanning BLMPOP/BZMPOP (moon#989)

CHANGELOG.md

@@ -31,6 +31,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   `WEIGHTS nan` used to be accepted and poison every aggregated score; it is now
   `ERR weight value is not a float`. Infinite weights remain legal. A client
   relying on either form silently doing nothing will now see an error.
+- **BEHAVIOUR CHANGE — `BLMPOP`/`BZMPOP` whose keys span shards are refused with
+  `CROSSSLOT`** at `--shards > 1` (moon#989), the rule moon#962 already applies
+  to `LMPOP`/`ZMPOP`. They used to answer, and measured at `--shards 4` over 16
+  three-shard placements, 20 of 32 probes popped a key the reply did not name:
+  either the WRONG key (a later local key served over an earlier remote one) or
+  a second key whose element no client ever received. "Pop from the first
+  non-empty key in argument order, exactly once" is a property of the whole key
+  vector that no single shard can see. The refusal is decided from the key
+  names before anything is touched, so the keyspace is unchanged. Keys under
+  one `{hash}` tag, and every placement at `--shards 1`, are unaffected.
 
 ### Fixed
 
@@ -63,6 +73,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   family is deferred by the #438 early-flush guard one statement above this
   predicate in both handlers whenever remote work is pending, and the derived
   predicate now says wait for them regardless.
+
+- **Multi-key blocking pops no longer destroy an element from a key they did
+  not answer with** (moon#989). `BLMPOP`, `BZMPOP`, `BLPOP`, `BRPOP`,
+  `BZPOPMIN` and `BZPOPMAX` over keys co-located under one `{hash}` tag replied
+  correctly while a SECOND non-empty key silently lost its head element:
+  `BLMPOP 0.3 3 {t}a {t}b {t}c LEFT` answered `{t}b B1` and left `{t}c` at
+  `[C2]`, with `C1` delivered to nobody. It happened whenever the client's
+  connection lived on a different shard than the keys: the client's shard
+  could not see them, so it sent one registration per key to their owner, and
+  the owner served the same waiter once per non-empty key — the client kept
+  the first reply and dropped the rest. The same fan-out popped a key named
+  twice (`BLMPOP 0 2 k k LEFT`) twice, and skipped Redis's `-WRONGTYPE` for an
+  earlier key of the wrong type. Measured at `--shards 4` (`BLMPOP`, `BZMPOP`,
+  `BLPOP`, `BZPOPMIN` × 16 tags × 3 server instances, against redis 8.6.1): 139
+  of 192 probes destroyed an element before, 0 after; `--shards 1` was and is 0. The client now sends ONE registration per owner shard carrying
+  every key it owns, and the owner registers, type-checks and serves them in
+  one synchronous stretch, so a waiter is served at most once there. A
+  spanning `BLPOP`/`BRPOP`/`BZPOPMIN`/`BZPOPMAX` can still be served by two
+  owner shards at once; that placement is unchanged by this fix and is
+  tracked as moon#1019.
+
 - **Commands routed to another shard are counted and timed** (moon#982).
   At `--shards > 1` a command whose key lives on a shard other than the
   connection's went through no telemetry probe at all — neither the

scripts/test-commands.sh

@@ -1808,6 +1808,19 @@ if should_run "blocking"; then
     # moon#570: `{blk}` co-locates the pair -- see the LMOVE row above.
     rcli RPUSH {blk}:src x y z >/dev/null 2>&1; mcli RPUSH {blk}:src x y z >/dev/null 2>&1
     assert_match "BLMOVE (ready)"      BLMOVE {blk}:src {blk}:dst LEFT RIGHT 1
+
+    # moon#989: a multi-key blocking pop serves from the FIRST non-empty key,
+    # exactly once. The reply alone cannot show a SECOND key losing an element
+    # (moon answered correctly while destroying it), so each row is followed by
+    # a read of the key it must not touch. `{blk}` co-locates the three keys.
+    rcli RPUSH {blk}:mp2 B1 B2 >/dev/null 2>&1; mcli RPUSH {blk}:mp2 B1 B2 >/dev/null 2>&1
+    rcli RPUSH {blk}:mp3 C1 C2 >/dev/null 2>&1; mcli RPUSH {blk}:mp3 C1 C2 >/dev/null 2>&1
+    assert_match "BLMPOP (ready, 3 co-located keys)" BLMPOP 1 3 {blk}:mp1 {blk}:mp2 {blk}:mp3 LEFT
+    assert_match "BLMPOP left the later key untouched" LRANGE {blk}:mp3 0 -1
+    rcli ZADD {blk}:zp2 1 B1 2 B2 >/dev/null 2>&1; mcli ZADD {blk}:zp2 1 B1 2 B2 >/dev/null 2>&1
+    rcli ZADD {blk}:zp3 1 C1 2 C2 >/dev/null 2>&1; mcli ZADD {blk}:zp3 1 C1 2 C2 >/dev/null 2>&1
+    assert_match "BZMPOP (ready, 3 co-located keys)" BZMPOP 1 3 {blk}:zp1 {blk}:zp2 {blk}:zp3 MIN
+    assert_match "BZMPOP left the later key untouched" ZRANGE {blk}:zp3 0 -1 WITHSCORES
 fi
 
 # ===========================================================================

scripts/test-consistency.sh

@@ -2013,7 +2013,7 @@ mk_norm() {
 # wrong-key pop cannot happen.
 route_probe_multi() {
     local mode="$1" label="$2" n="$3" seeds="$4" probe="$5" check="${6:-}"
-    local i j port s p r m before="" after=""
+    local i j port s p r m before="" after="" after_redis=""
     local wrong=0 refused=0
     local -a keys seedv sv pv
     for i in $(seq 1 "$MK_TRIALS"); do
@@ -2050,6 +2050,7 @@ route_probe_multi() {
         m="$(mk_norm "$(redis-cli -p "$PORT_RUST"  "${pv[@]}" 2>&1)")"
         if [[ -n "$check" ]]; then
             after="$(mk_state "$PORT_RUST" "$check" "${keys[@]}")"
+            after_redis="$(mk_state "$PORT_REDIS" "$check" "${keys[@]}")"
         fi
         # A SUBSTRING test, not an anchored `case` pattern. The reply may carry
         # a leading blank line (see `mk_norm`), and an anchored pattern that
@@ -2071,6 +2072,12 @@ route_probe_multi() {
         elif [[ "$r" != "$m" ]]; then
             echo "  FAIL detail: ${label}[$i] ($mode) answered '$m'; redis says '$r'"
             wrong=$((wrong + 1))
+        elif [[ -n "$check" && "$after" != "$after_redis" ]]; then
+            # moon#989: the RIGHT reply is not enough. BLMPOP answered exactly
+            # like redis while popping a second key it never named, and only
+            # the keyspace after the probe could show it.
+            echo "  FAIL detail: ${label}[$i] ($mode) answered like redis but the keyspace differs: moon '$after' vs redis '$after_redis'"
+            wrong=$((wrong + 1))
         fi
     done
     assert_eq "moon#962 ${label} ${mode} (shards=${SHARDS}, ${MK_TRIALS} placements)" \
@@ -2099,9 +2106,33 @@ MK_ROWS=(
   "touch|3|SET %K v|SET %K v|SET %K v|TOUCH %K1 %K2 %K3|GET %K"
   "lmpop|3||RPUSH %K B1 B2|RPUSH %K C1 C2|LMPOP 3 %K1 %K2 %K3 LEFT|LRANGE %K 0 -1"
   "zmpop|3||ZADD %K 1 B1 2 B2|ZADD %K 1 C1 2 C2|ZMPOP 3 %K1 %K2 %K3 MIN|ZRANGE %K 0 -1"
+  # moon#989: the blocking twins. Data is seeded, so neither blocks -- the
+  # 0.1s timeout only bounds a regression that would. `colo` is the row that
+  # caught the defect: the reply matched redis while a second co-located key
+  # lost its head element, visible only through the per-key check.
+  "blmpop|3||RPUSH %K B1 B2|RPUSH %K C1 C2|BLMPOP 0.1 3 %K1 %K2 %K3 LEFT|LRANGE %K 0 -1"
+  "bzmpop|3||ZADD %K 1 B1 2 B2|ZADD %K 1 C1 2 C2|BZMPOP 0.1 3 %K1 %K2 %K3 MIN|ZRANGE %K 0 -1"
+)
+
+# moon#989: the rest of the multi-key blocking-pop family, CO-LOCATED only.
+# They shared BLMPOP's double-pop and are fixed with it, so `colo` must agree
+# with redis byte for byte. Their SPANNING placement is still a known defect
+# (two owner shards can each serve the same waiter) and is deliberately not
+# refused yet -- that is a behaviour decision tracked as moon#1019, so a
+# `span` row here would only assert the bug.
+MK_COLO_ONLY_ROWS=(
+  "blpop|3||RPUSH %K B1 B2|RPUSH %K C1 C2|BLPOP %K1 %K2 %K3 0.1|LRANGE %K 0 -1"
+  "brpop|3||RPUSH %K B1 B2|RPUSH %K C1 C2|BRPOP %K1 %K2 %K3 0.1|LRANGE %K 0 -1"
+  "bzpopmin|3||ZADD %K 1 B1 2 B2|ZADD %K 1 C1 2 C2|BZPOPMIN %K1 %K2 %K3 0.1|ZRANGE %K 0 -1"
+  "bzpopmax|3||ZADD %K 1 B1 2 B2|ZADD %K 1 C1 2 C2|BZPOPMAX %K1 %K2 %K3 0.1|ZRANGE %K 0 -1"
 )
 
-for mk_row in "${MK_ROWS[@]}"; do
+for mk_row in "${MK_ROWS[@]}" "${MK_COLO_ONLY_ROWS[@]/#/colo-only:}"; do
+    mk_modes="span colo"
+    if [[ "$mk_row" == colo-only:* ]]; then
+        mk_modes="colo"
+        mk_row="${mk_row#colo-only:}"
+    fi
     IFS='|' read -r -a mk_f <<<"$mk_row"
     mk_label="${mk_f[0]}"; mk_n="${mk_f[1]}"
     # fields 2..(2+n-1) are the per-key seeds, then the probe, then the check
@@ -2112,8 +2143,9 @@ for mk_row in "${MK_ROWS[@]}"; do
     mk_seeds="${mk_seeds%|}"
     mk_probe="${mk_f[$((2 + mk_n))]}"
     mk_check="${mk_f[$((3 + mk_n))]:-}"
-    route_probe_multi span "$mk_label" "$mk_n" "$mk_seeds" "$mk_probe" "$mk_check"
-    route_probe_multi colo "$mk_label" "$mk_n" "$mk_seeds" "$mk_probe" "$mk_check"
+    for mk_mode in $mk_modes; do
+        route_probe_multi "$mk_mode" "$mk_label" "$mk_n" "$mk_seeds" "$mk_probe" "$mk_check"
+    done
 done
 
 # Non-vacuity. At --shards>1 the span sweep MUST have reached the cross-shard
@@ -2133,7 +2165,7 @@ assert_eq "moon#962 TOUCH is never refused (shards=$SHARDS)" "0" "$MK_TOUCH_REFU
 
 # Tidy up by exact name -- `--scan | xargs -r` is GNU-only and this script runs
 # on macOS too.
-for mk_row in "${MK_ROWS[@]}"; do
+for mk_row in "${MK_ROWS[@]}" "${MK_COLO_ONLY_ROWS[@]}"; do
     IFS='|' read -r -a mk_f <<<"$mk_row"
     for mk_i in $(seq 1 "$MK_TRIALS"); do
         for mk_j in $(seq 1 "${mk_f[1]}"); do