MOD-15749 Add support for short form of CLUSTERSET

[galcohen-redislabs]

This initial commit splits the command into two cases: long and short (which currently does nothing).
It also refactors the long form to help writing a clean version for the upcoming short form.

---

Note

High Risk
Changes cluster topology discovery and inter-shard CLUSTERSET propagation; short form depends on new RedisModule cluster slot APIs and misconfiguration could break cross-shard messaging.

Overview
Adds a compact CLUSTERSET form (CLUSTERSET or CLUSTERSET AUTH {pwd}) alongside the existing verbose topology command, and refactors how cluster state is built from incoming arguments.

Short form no longer requires the full HASHFUNC / RANGES / SHARD payload: the module discovers masters, addresses, flags, and slot ranges from the Redis server via GetClusterNodesList, GetClusterNodeInfo, and the newly wired GetClusterNodeSlotRanges API (declared in redismodule.h). Optional AUTH applies one password to all shard connections.

Long form behavior is preserved but split into helpers (InitClusterData, ParseShardEntry, SetClusterDataLongForm). Shared init copies argv for later CLUSTERSETFROMSHARD propagation; only long form still substitutes the per-target MYID token when pushing topology on reconnect/hello retries.

Command entry points accept either form via IsLongFormClusterSet / IsShortFormClusterSet instead of a fixed minimum argc of 10.

^{Reviewed by Cursor Bugbot for commit 4de0d20. Bugbot is set up for automated code reviews on this repo. Configure here.}

[gabsow]

Solid refactor overall — splitting the long-form into ParseShardEntry reads well, and the short-form path matches MOD-15749's intent. A few things to address, the first one blocking:

Severity	Issue
🔴 Blocking	slots[] routing table missing the first range of every node — see inline comment on line 1147
🟠 Important	Slotless fallback claims all 16384 slots
🟠 Important	AUTH password sent even when internal secret is in use (inconsistent with MR_RefreshClusterData)
🟡 Cleanup	InitClusterData ignores its parameter; isMe double-set; log typo; redundant assertion; uint16_t loop bound; argc-based form detection is brittle
🟢 Test gap	Only the happy-path 3-shard / no-AUTH case is covered; no slot-routed assertion (the test would pass even with the first-range bug)

[gabsow]

Argc-based form detection is a bit brittle: short form is argc == 1 || argc == 3, long form is argc >= 10. If the long form ever evolved to use exactly 3 args (or short form to a config with 10+), this would silently dispatch wrong. A leading-token sniff (AUTH / HASHFUNC / nothing) would be more robust. Not blocking, but worth a comment at least.

[galcohen-redislabs]

They will not change that much. Actually we plan to get rid of both. So argc-based logic is good enough.

[gabsow]

Typo: "on after reconnect" → "after reconnect" (extra on).

[gabsow]

InitClusterData takes a Cluster* cluster parameter but ignores it — every line mutates clusterCtx.CurrCluster directly. Either use the parameter throughout (so the helper is testable in isolation and the global mutation is explicit at the call site) or drop the parameter and rename to make it obvious it operates on the global. SetMyId and GenerateRunId get this right.

[galcohen-redislabs]

oy! fixed.

[gabsow]

Redundant: MR_CreateNode already asserts !MR_GetNode(id) (see line 831). Either drop this pre-check or drop the one inside MR_CreateNode.

[galcohen-redislabs]

dropped

[gabsow]

🟠 Slotless-node fallback is incorrect. When slots->num_ranges == 0 (per redis/redis#14953, this happens for unknown nodes / non-cluster mode and plausibly for slotless masters), we fall back to minSlot=0, maxSlot=NUMBER_OF_SLOTS-1 — i.e. we declare this node owns all 16384 slots.

The long form represents slotless with minSlot=0, maxSlot=-1 so that MR_CreateNode adds no range (see if (minSlot <= maxSlot) at line 835). The short form should match — pass 0, -1 (use long long like the long form) and skip the clusterCtx.minSlot/maxSlot overwrite when slotless.

[galcohen-redislabs]

fixed

[gabsow]

🔴 Blocking bug: clusterCtx.CurrCluster->slots[] routing table is never populated for the first range.

MR_CreateNode only stores the first range in the node's own slotRanges list (line 836) — it does not write into the global slots[] table. The loop below at line 1155 then iterates j = 1 onward, so the first range of every node is missing from slots[].

Concrete case: a normal 3-shard cluster where each shard owns one contiguous range → after short-form CLUSTERSET, slots[] is entirely NULL. The dispatch at line 283 (slots[sendMsg->slotToSend]) will then NULL-deref or silently misroute every slot-targeted message.

Compare with SetClusterDataLongForm (line 1220-1222) and MR_RefreshClusterData (line 948-950) — both unconditionally fill slots[k] for every range.

Fix: either run the slot-fill loop before the j=1.. extra-range loop, or unify both into a single for (j=0; j<num_ranges; j++) block:

Node* aMasterNode = MR_CreateNode(nodeId, ip, port, password, NULL, 0, -1);
aMasterNode->isMe = (flags & REDISMODULE_NODE_MYSELF) != 0;
for (size_t j = 0; j < slots->num_ranges; j++) {
    uint16_t minSlot = slots->ranges[j].start;
    uint16_t maxSlot = slots->ranges[j].end;
    mr_listAddNodeTail(aMasterNode->slotRanges, NewSlotRange(minSlot, maxSlot));
    for (int k = minSlot; k <= maxSlot; k++)
        clusterCtx.CurrCluster->slots[k] = aMasterNode;
    if (aMasterNode->isMe && j == 0) {
        clusterCtx.minSlot = minSlot;
        clusterCtx.maxSlot = maxSlot;
    }
}

Note: the test test_short_form_clusterset did NOT catch this because TS.QUERYINDEX fans out via the node dict, not via slots[]. The dict is populated correctly, so the broadcast works. See my comment on the RTS PR for a suggested slot-routed test.

[galcohen-redislabs]

good catch. fixed.
(we don't use the slots[] anyway in ts because it never routes via a hash. the cluster-wide commands always send to all nodes)

[gabsow]

isMe is set twice — MR_CreateNode sets it via strcmp(id, myId) at line 862, then this line overrides via the MYSELF flag. They should always agree, but having two sources of truth is confusing. The MYSELF flag is the more direct signal — consider making MR_CreateNode not set isMe at all when called from this path, or unify.

[galcohen-redislabs]

This is true, and the flag is the more "correct" way. But note that MR_CreateNode is also called from other' flag-less' environment, so we need the strcmp logic as well. I'll add a comment.

[gabsow]

🟠 The password obtained from AUTH <pwd> is passed unconditionally to MR_CreateNode. Compare with MR_RefreshClusterData (line 940):

n = MR_CreateNode(..., RedisModule_GetInternalSecret ? NULL : clusterCtx.password, NULL, ...);

i.e. when an internal secret is available, password auth is intentionally skipped. If a deployment uses internal secrets, the short form will still attempt password auth — at minimum inconsistent with the existing pattern, possibly wrong. Either mirror the same guard or leave a comment explaining the difference.

[galcohen-redislabs]

Note that this is not the same case. Here we fetch the password from the parsing the CLUSTERSET command (similar to what's being done in the parsing of ADDR in the long form). The CLUSTERSET from the DMC will always have a password (which is the bdb's internal_pass attribute).

[gabsow]

uint16_t k is safe for canonical maxSlot ≤ 16383, but would spin forever if a buggy API ever returns maxSlot == UINT16_MAX (e.g. casting -1). The long-form loop at line 1220 uses int k. Match that here.

[galcohen-redislabs]

matched

[gabsow]

LGTM ✅

All 9 inline comments addressed:

• 🔴 Blocking slots[] first-range bug → fixed in 42ce87e
• 🟠 Slotless fallback → fixed in 458f2ca (now uses long long 0, -1 sentinel)
• 🟠 AUTH vs internal-secret → accepting your reasoning (symmetric with long-form ADDR parsing; DMC always provides the password)
• 🟡 InitClusterData parameter, redundant assert, log typo, uint16_t→int loop, isMe rationale comment → all addressed
• 🟡 Argc-based form detection → accepting (you plan to retire both forms)

Flagging for downstream visibility: the RTS PR (#2034) still pins to ad6a45d3 and CI is fully red because the blocking slots[] bug surfaces in tests there. Bumping the submodule to this PR's HEAD (or to the merge commit) should unblock it.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 4de0d20. Configure here.}

[cursor]

Missing thread-safe context lock for Redis API calls

High Severity

SetClusterDataShortForm calls RedisModule_GetClusterNodesList, RedisModule_GetClusterNodeInfo, RedisModule_GetClusterNodeSlotRanges, and their corresponding free functions using mr_staticCtx without acquiring RedisModule_ThreadSafeContextLock. This function runs on the event loop's dedicated pthread, not the Redis main thread. The existing MR_RefreshClusterData — which runs in the same threading context — correctly wraps its RedisModule_GetClusterNodeInfo and RedisModule_Call invocations with lock/unlock pairs. The omission here creates a data race on shared server state.

 

^{Reviewed by Cursor Bugbot for commit 4de0d20. Configure here.}