fix(blocking): multi-key blocking pops serve exactly once; refuse spanning BLMPOP/BZMPOP (moon#989) - #1021
Merged
Conversation
…nning BLMPOP/BZMPOP (moon#989)
At --shards >= 2 a multi-key blocking pop (BLMPOP, BZMPOP, BLPOP, BRPOP,
BZPOPMIN, BZPOPMAX) over keys co-located under one {hash} tag replied
correctly while a SECOND non-empty key lost its head element, delivered to
nobody:
BLMPOP 0.3 3 {t}a {t}b {t}c LEFT ({t}a empty, {t}b=[B1 B2], {t}c=[C1 C2])
redis 8.6.1 -> {t}b [B1] {t}c=[C1 C2]
moon --shards 4 -> {t}b [B1] {t}c=[C2] <-- C1 destroyed
Root cause (the triage hypothesis, confirmed from code and reproduction):
when the client's connection lives on a different shard than the keys,
immediate_scan can see none of them, so the multi-key coordinator sent one
BlockRegister per key to their owner. The owner handled each message on its
own -- register, see data, serve -- and so 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 twice, and skipped Redis's
-WRONGTYPE for an earlier wrong-typed key. It only happens when the
connection is off the owner shard, which is why it varied between server
instances and never showed at --shards 1.
Fix:
- The coordinator now sends ONE BlockRegisterGroup per owner shard carrying
every key that owner holds, in argument order (blocking_multikey.rs, shared
by both runtimes). The owner registers every member under one wait_id and
then serves in argument order in one synchronous stretch
(blocking::group::register_group); serving runs remove_wait, which
unregisters the siblings before the next key is looked at, so the waiter
is served at most once. When the group is the whole command it also
answers the -WRONGTYPE Redis owes; a partial group skips a wrong-typed key
instead of tearing down its sibling registrations.
- Spanning BLMPOP/BZMPOP are refused with CROSSSLOT in immediate_scan,
before anything is popped or registered, via the same family list moon#962
uses for LMPOP/ZMPOP. That placement also popped the WRONG key (a later
local key over an earlier remote one) and could be served by two owners.
- BlockRegister now carries single-key waiters only, so its sole_key flag
is gone and the owner's type check is unconditional.
Not changed: spanning BLPOP/BRPOP/BZPOPMIN/BZPOPMAX can still be served by
two owner shards (measured unchanged within noise, 35 vs 40 extra pops of
120 over 10 instances each). Refusing them is a client-visible decision and
is moon#1019.
Tests:
- tests/blocking_multikey_cross_shard.rs: 7 tests. Co-located rows run for
tags owned by EVERY shard, so they cannot pass by the connection happening
to land on the owner. RED on 43ad387 (bmk1 58/80, bmk3, bmk6 20/32),
GREEN with the fix on monoio and tokio; mutating the grouping or the
refusal turns them red again.
- unit tests for register_group and stage_multikey_wait.
- test-consistency.sh: blmpop/bzmpop span+colo rows, colo-only rows for the
other four, and answered rows now compare the keyspace with redis, not
just the reply. test-commands.sh: co-located BLMPOP/BZMPOP rows with a
read of the key they must not touch.
Refs: moon#989, moon#962, moon#1019
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 reachedNext included review available in 51 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
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 |
CHANGELOG.md was the only conflict: both sides added entries at the same place in [Unreleased]. Both are kept. No code file changed, so the tree outside CHANGELOG.md is exactly the tested branch plus main. author: Tin Dang
Collaborator
Author
|
Merged main into this branch to clear a CHANGELOG.md-only conflict; both sides' entries are kept. Every non-CHANGELOG file is byte-identical to the tested head |
TinDang97
added a commit
that referenced
this pull request
Sep 19, 2026
…r; keep a serve that races the end of a wait A multi-key BLPOP/BRPOP/BZPOPMIN/BZPOPMAX whose keys span shards could pop the wrong key (the client's shard served a later local key over an earlier non-empty remote one), or pop on two or three shards and deliver one reply, destroying the rest (moon#1019). These commands keep working across shards, as decided for untagged BLPOP q1 q2 q3 0 worker loops, and now answer as standalone redis does, including the -WRONGTYPE ladder. Separately, a wait that ended by timeout, disconnect or shutdown sent BlockCancel and dropped its receivers, and a serve landing in between was dropped with them, element included, because the owner undoes only a FAILED send (moon#1023). One mechanism fixes both: a claim token (src/blocking/claim.rs), shared by every registration of a waiter that lives on more than one thread. WAITING --try_claim (a shard, element already popped)--> CLAIMED WAITING --settle (the waiter gives up)-----------------> DEAD Each terminal state is entered by exactly one CAS from WAITING, so exactly one of "a shard serves" and "the waiter gives up" happens. - Owners pop first and claim second, with the element in hand. On a lost claim the element goes back in the same synchronous stretch (WakeUndo), and the waker moves on to the next waiter. There is no "claimed but nothing to send" state to release. Errors (-WRONGTYPE at registration) are claimed too, so exactly one answer of any kind can exist. - Wakers skip a settled waiter before touching the datastore (WaitEntry::is_settled), and keep serving a key's waiters while it still holds data. One push of several elements used to answer exactly one waiter (the review P3 on #989 in register_group). Redis 8.6.1 answers both waiters of RPUSH k a b. - Argument order: immediate_scan now STOPS at the first remote key instead of skipping it. The remaining keys are registered one run of same-shard keys at a time, in argument order, and every run but the last is acknowledged before the next is sent. So each run is decided only after every earlier key's owner found it empty and of the right type, and that owner decides its run exactly as --shards 1 would. BlockRegisterGroup's whole_command flag is gone: every run is decisive. Later local runs go through the same register_group on the client's thread. Cost: one extra round trip per remote run that is not last. Co-located keys and single-key waits send exactly the messages they sent before. - Settle (blocking_multikey::settle): when a wait ends without a reply, the waiter settles the token. DEAD means no shard served and none can, so the receivers are dropped as they are, with no drain needed. CLAIMED means the winner's reply is in flight on exactly one receiver. It is taken and then delivered on timeout or shutdown, or on peer-gone its element is restored to the key's owner (new ShardMessage::BlockRestore, re-woken for the next waiter). A waiter whose keys are all local carries no token. It drains after its synchronous remove_wait, which is exact on one thread, and its fast path is otherwise unchanged. BLMPOP/BZMPOP spanning shards are still refused with CROSSSLOT, like LMPOP/ZMPOP. Evidence (macOS, release-fast, --shards 4). RED was measured on #1021's tip plus the test hook; its blocking files are byte-identical to main's. - tests/blocking_spanning_claim.rs, before -> after, monoio / tokio: immediate pops differing from redis 91/192, 95/192 -> 0/192 on both -WRONGTYPE ladder skipped 14/16, 10/16 -> 0/16 concurrent pushes lost elements 39/80, 64/80 -> 0/80 push racing a timeout lost it 13/16, 13/16 -> 0/16 push racing a disconnect lost it 14/16, 14/16 -> 0/16 GREEN 3/3 consecutive runs per runtime. bsc4 also asserts that at least 2*(SHARDS-1) serves landed inside the window, so it cannot pass vacuously. - Mutations, each turning its test RED: ignoring the claim (bsc1 83/192, bsc3 48/80); no run acknowledgement (bsc1 87/192, bsc2; bsc3 stays green because order and exactly-once are guarded separately); a no-op restore (bsc5 10/16); one waiter per push (bsc6 4/4). - tests/loom_blocking_claim.rs: exhaustive under cfg(loom), and it fails when the model's claim is bypassed. - The blocking suites are green on both runtimes. test-consistency.sh --shards 4 has 0 blocking failures. Spanning blpop/brpop/bzpopmin/bzpopmax rows moved from colo-only to span+colo, and a CROSSSLOT for them now fails the row. Test hook: MOON_TEST_BLOCK_SETTLE_DELAY_MS, read once through a OnceLock, the same pattern as MOON_TEST_AOF_FSYNC_STALL_MS. Closes #1019 Closes #1023 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.
Fixes #989. Follow-up for the part left on purpose: #1019.
Root cause
This confirms the triage hypothesis, from the code and a live reproduction against redis 8.6.1.
When a client's connection is on a different shard than the keys,
blocking::immediate_scancan see none of them. That is correct (moon#557). The multi-key coordinator inhandle_blocking_command{,_monoio}then sent oneBlockRegisterper key to their owner. The owner handled each message on its own: register the key, see data, serve the waiter (spsc_handler.rs). So the same waiter was served once per non-empty key. The client kept the first reply from itsFuturesUnorderedand dropped the rest, and the elements in those replies had already left the keyspace.A2's undo does not help, because thesendinto the still-live receiver succeeds.{hash}-tagged keys, varied between server instances, stayed stable within one instance, and never appeared at--shards 1. (On macOS moon'sSO_REUSEPORTlisteners send every connection to one shard.)BLPOP,BRPOP,BZPOPMINandBZPOPMAXlose data exactly likeBLMPOP/BZMPOP.BLMPOP 0 2 k k LEFT). It also skipped Redis's-WRONGTYPEfor an earlier wrong-typed key, because per-key registrations hadsole_key: false.Options considered
--shards 1vs redisBLMPOP/BZMPOP(the moon#962 rule)remove_waitunregisters siblings before the next key is looked at). Spanning MPOP: refused before anything is touched--shards 1, including-WRONGTYPEand duplicate keysblocking.rsshrinks; one message per owner instead of one per keyBehaviour for co-located keys changes only toward redis parity (no more element loss;
-WRONGTYPEwhere redis gives it). The only new refusal is spanningBLMPOP/BZMPOP, consistent withLMPOP/ZMPOPsince #990. It has a BEHAVIOUR CHANGE CHANGELOG entry.Deliberately not done: spanning
BLPOP/BRPOP/BZPOPMIN/BZPOPMAXcan still be served by two owners, or pop the wrong key. Refusing them breaks untaggedBLPOP q1 q2 q3 0worker loops at--shards > 1, which is a product decision. That is filed as #1019 with options and a recommendation. An interleaved A/B shows this PR leaves that placement unchanged within noise: extra pops 35 (merge-base) vs 40 (branch) out of 120, over 10 fresh instances each. When every key is on a different shard the protocol sends the same messages as before.Changes
src/server/conn/blocking_multikey.rs(new):stage_multikey_wait, shared by both runtimes. Local keys are registered directly; remote keys are grouped into oneBlockRegisterGroupper owner, in argument order, withwhole_commandset when the owner holds every key.src/blocking/group.rs(new):register_group, the owner side. A whole-command group runs Redis's type ladder first (-WRONGTYPEfor the first existing wrong-typed key, stopping at the first servable one). Then it registers every member and serves in argument order, stopping once the waiter is gone. A wrong-typed key is never offered to the wakers: they would pop the waiter, fail, andremove_waitall of its siblings.ShardMessage::BlockRegisterGroup(boxed; the 64-byte enum cap still holds).BlockRegisternow carries single-key waiters only, sosole_keyis removed and the owner's type check is unconditional.immediate_scanrefuses spanningBLMPOP/BZMPOPthroughcross_shard_multikey_rejection. They are added to its family list ((6, b'b'), a new arm with no collision), so there is one list, not two.BlockingRegistry::is_waiting.Dispatch paths
command::dispatch/dispatch_read: not reached. Blocking commands are intercepted by the connection handlers before dispatch. Inside MULTI they are queued as their non-blocking twin (queued_blocking_frame), and EXEC's locality check covers them (bmk7).try_inline_dispatch: GET/SET only; not reachable.handle_blocking_command*now call the same staging function.redis.call('BLMPOP', ...)answersunknown commandon moon, both before and after this PR (redis runs it non-blocking). Pre-existing and out of scope.Evidence
Measured before rebasing onto 16c4213 (the two commits it adds touch ACL SAVE and CI sharding only; the new test and clippy were re-run after the rebase). All numbers are from a native macOS host against redis 8.6.1 (
redis-serveron PATH); moon is arelease-fastbuild.Repro, clean (one listener per port verified with
lsoffor every leg), 3 fresh instances per binary,{tag}keys × 16 per command, commandsBLMPOP/BZMPOP/BLPOP/BZPOPMIN:tests/blocking_multikey_cross_shard.rs(7 tests; the co-located rows use tags constructed to be owned by every shard, so they cannot pass by the connection landing on the owner):bmk158/80 rows "ELEMENT DESTROYED" (including the duplicate-key rows),bmk3every remote-owner row pops instead of-WRONGTYPE,bmk620/32 spanning probes pop an unanswered key. Controlsbmk2(--shards 1),bmk4(block then wake),bmk5(timeout leaves no ghost waiter) andbmk7(MULTI/EXEC) are green on both trees.bmk1/bmk3red on both runtimes (60/80 and 59/80); removing the refusal arm turnsbmk6red (25 placements).scripts/test-consistency.sh --shards 4: new rows red on the merge-base binary (blmpop/bzmpop span+colo, blpop/brpop/bzpopmin/bzpopmax colo) and green on this branch. The same 6 unrelated failures appear on both binaries (5tracking:controls, see the known 4-shard tracking flake; andROLE on a master), so they are not from this change.--shards 1(branch only): onlyROLE on a masterfails, the same row that fails on the merge-base binary at--shards 4. Answeredroute_probe_multirows now also compare the keyspace with redis, not just the reply; the reply alone is exactly what hid this bug.scripts/test-commands.sh: 4 new rows inblocking. I could not run the script itself in my sandbox. I replayed the exact rows against redis: 12/12 pass on this branch over 3 instances, and the merge-base binary fails the "later key untouched" rows on every instance.Unit tests:
blocking::group(9),blocking_multikey(3).cargo clippy --all-targets -D warningsis clean on both runtimes andcargo fmt --checkis clean. The lib testgate_is_skipped_with_spill_sender_when_no_limit_is_configuredfails under a full parallel--librun on this branch and on the merge-base source alike. It is #856, unrelated; it passes in isolation.No
unsafe, nounwrapoutside tests, no atomics. Not measured on Linux: io_uring andSO_REUSEPORThashing change which connection lands where, but not the protocol.ci-local.shwas not run from this sandboxed session (it needs the moon-dev VM); the hosted matrix is dispatched.