From 0bbb196c4612aec7ae2c66c31e9bcd5bcd77ad9c Mon Sep 17 00:00:00 2001 From: Shubham S Taple <155555100+ShubhamTaple@users.noreply.github.com> Date: Wed, 29 Apr 2026 19:34:06 +0530 Subject: [PATCH] Fix sharded pubsub unsubscribe lookup using cached command slot (#15094) Fixes #15085 ## Problem getKeySlot() may return `server.current_client->slot` while a command is executing instead of computing the slot from the provided string. The unsubscribe can be triggered by another client, in which case server.current_client is not the client being unsubscribed, so getKeySlot() would return that client's cached slot. Using this wrong slot would make the lookup in type.serverPubSubChannels miss the channel and ultimately trigger the assertion below. ## Fix Always use keyHashSlot() instead of getKeySlot() on unsubscribe. --------- Co-authored-by: debing.sun --- src/pubsub.c | 5 ++++- tests/unit/cluster/sharded-pubsub.tcl | 27 +++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/pubsub.c b/src/pubsub.c index 7199be1e0..b9198d263 100644 --- a/src/pubsub.c +++ b/src/pubsub.c @@ -293,7 +293,10 @@ int pubsubUnsubscribeChannel(client *c, robj *channel, int notify, pubsubtype ty retval = 1; /* Remove the client from the channel -> clients list hash table */ if (server.cluster_enabled && type.shard) { - slot = getKeySlot(channel->ptr); + /* Compute the slot from the channel directly instead of using getKeySlot(), + * because the unsubscribe may be triggered by a different client, and + * getKeySlot() would return the cached slot of that client. */ + slot = keyHashSlot(channel->ptr, sdslen(channel->ptr)); } de = kvstoreDictFind(*type.serverPubSubChannels, slot, channel); serverAssertWithInfo(c,NULL,de != NULL); diff --git a/tests/unit/cluster/sharded-pubsub.tcl b/tests/unit/cluster/sharded-pubsub.tcl index 57b550ab7..5f78b7f0f 100644 --- a/tests/unit/cluster/sharded-pubsub.tcl +++ b/tests/unit/cluster/sharded-pubsub.tcl @@ -64,4 +64,31 @@ start_cluster 1 1 {tags {external:skip cluster}} { catch {[$replica EXEC]} err assert_match {EXECABORT*} $err } + + # Regression: shard channel slot must not follow getKeySlot() current_client + # cache when CLIENT KILL runs inside another client's EXEC (pubsubUnsubscribeChannel). + test {Shard pubsub: CLIENT KILL subscriber inside MULTI/EXEC (cross-slot)} { + # SET fixes the transaction client's slot to keyk's slot; the subscriber must + # use a shard channel in a different slot so a wrong-slot lookup would fail. + set keyk "{06S}k" + set channel "{Qi}ch" + assert {[R 0 cluster keyslot $channel] != [R 0 cluster keyslot $keyk]} + + set rd_sub [redis_deferring_client] + $rd_sub client id + set cid [$rd_sub read] + $rd_sub ssubscribe $channel + $rd_sub read + + $primary multi + $primary set $keyk v + $primary client kill id $cid + set got [$primary exec] + + assert_equal {OK 1} $got + assert_equal PONG [$primary ping] + + catch {$rd_sub read} + $rd_sub close + } }