Skip to content

Commit a85904d

Browse files
nkaradzhovclaude
andcommitted
fix(client): route and split the raw cluster sendCommand path correctly
The policy refactor folded the raw cluster sendCommand into _executeWithPolicies but built its parser wrong, regressing behaviour master got right by routing on the explicit firstKey: - firstKey was prepended to args, so redisArgs[0] became a key instead of the command name (breaking policy resolution) and no key was ever marked, leaving parser.firstKey unset so routing fell back to undefined. - makeFn ignored the per-entry sub-parser and always sent the original args, so a split multi_shard command sent the full command to every shard. Build redisArgs as an exact copy of args (command name at index 0) and register the caller's firstKey via a new BasicCommandParser.markRoutingKey, which records the key for routing without re-appending it to redisArgs. Send p.redisArgs from the closure so split sub-commands each carry their own slot's arguments. A known multi_shard command sent raw now splits correctly, since the resolver supplies its key specs and the splitter operates on the flat redisArgs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent dc4f1f7 commit a85904d

3 files changed

Lines changed: 55 additions & 3 deletions

File tree

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import { strict as assert } from 'node:assert';
2+
import { BasicCommandParser } from './parser';
3+
4+
describe('BasicCommandParser', () => {
5+
describe('markRoutingKey', () => {
6+
it('sets firstKey without appending to redisArgs', () => {
7+
const parser = new BasicCommandParser();
8+
parser.push('MGET', 'k1', 'k2');
9+
parser.markRoutingKey('k1');
10+
11+
// redisArgs stays an exact copy of what was pushed (the wire command).
12+
assert.deepEqual(parser.redisArgs, ['MGET', 'k1', 'k2']);
13+
// ...but the key is registered for routing.
14+
assert.deepEqual(parser.keys, ['k1']);
15+
assert.equal(parser.firstKey, 'k1');
16+
});
17+
18+
it('leaves keys empty when never called (keyless raw command)', () => {
19+
const parser = new BasicCommandParser();
20+
parser.push('PING');
21+
22+
assert.deepEqual(parser.redisArgs, ['PING']);
23+
assert.deepEqual(parser.keys, []);
24+
assert.equal(parser.firstKey, undefined);
25+
});
26+
27+
it('keeps commandIdentifier pointing at the command name', () => {
28+
const parser = new BasicCommandParser();
29+
parser.push('GET', 'k1');
30+
parser.markRoutingKey('k1');
31+
32+
assert.deepEqual(parser.commandIdentifier, { command: 'GET', subcommand: 'k1' });
33+
});
34+
});
35+
});

packages/client/lib/client/parser.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,17 @@ export class BasicCommandParser implements CommandParser {
9494
this.#redisArgs.push(key);
9595
}
9696

97+
/**
98+
* Records a routing key whose value is already present in the pushed args,
99+
* without appending it again. Used by the raw cluster `sendCommand` path,
100+
* where the caller supplies the routing key separately from the full,
101+
* already-assembled argument list — so `firstKey` resolves for routing while
102+
* `redisArgs` stays an exact copy of the command sent on the wire.
103+
*/
104+
markRoutingKey(key: RedisArgument) {
105+
this.#keys.push(key);
106+
}
107+
97108
pushKeysLength(keys: RedisVariadicArgument) {
98109
if (Array.isArray(keys)) {
99110
this.#redisArgs.push(keys.length.toString());

packages/client/lib/cluster/index.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -617,17 +617,23 @@ export default class RedisCluster<
617617
...options
618618
}
619619

620+
// `args` is the full command as sent on the wire (name at index 0), so it
621+
// becomes `redisArgs` verbatim — policy resolution reads the command name
622+
// and the multi_shard splitter's key-spec offsets line up. The caller's
623+
// `firstKey` is marked for routing without re-appending it.
620624
const parser = new BasicCommandParser();
621-
if (firstKey) parser.push(firstKey);
622625
args.forEach(arg => parser.push(arg));
626+
if (firstKey !== undefined) parser.markRoutingKey(firstKey);
623627

624628
// Raw path: no command object, so readonly-ness stays an explicit caller
625-
// argument and the reply is returned untransformed.
629+
// argument and the reply is returned untransformed. The closure sends the
630+
// per-entry parser's args, so split multi_shard sub-commands each carry
631+
// their own slot's arguments (and the unsplit case sends `args` unchanged).
626632
return this._self._executeWithPolicies(
627633
parser,
628634
isReadonly,
629635
opts,
630-
() => (client, opts) => client.sendCommand(args, opts)
636+
p => (client, opts) => client.sendCommand(p.redisArgs as CommandArguments, opts)
631637
);
632638
}
633639

0 commit comments

Comments
 (0)