Request response policies - #2996
Conversation
e74bb9a to
2ea355f
Compare
fd526fa to
e857c1b
Compare
a85904d to
25716d1
Compare
55e1732 to
fb6d15e
Compare
c413da5 to
005db4a
Compare
a745ca1 to
b3371b2
Compare
1947819 to
689af4d
Compare
8f70362 to
8dff5a0
Compare
177e8e9 to
313219c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Regenerate the static command-metadata table from a live server COMMAND dump and add the generator + overrides tooling that produces it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drive each command's routing and client-side-cache eligibility from the regenerated server COMMAND metadata instead of per-file hand annotations, updating the command definitions across all packages accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Parse the COMMAND key-specifications into CommandReply, add the request/response policy constants, and the static + dynamic policy resolvers that back cluster routing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split a multi_shard command's keys per hash slot using the server key specs as the reconstruction recipe (numkeys rewrite, options-suffix copy, single-slot fast path), recording group indices for reply reassembly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add _executeWithPolicies: resolve a command's policy, route it to an execution plan (pass-through, fan-out, or multi_shard split), and reduce the replies via the response policy. Includes the default-policy fallback for policy-less commands and the raw sendCommand split path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implement the client-side special policies: cluster-wide SCAN behind a virtual cursor token, RANDOMKEY fan-out with a non-nil reduction, and sticky FT.CURSOR routing bound to the node that served the aggregate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6983991 to
8cc07fd
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6983991f2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
8cc07fd to
0ce49cb
Compare
0ce49cb to
090eea5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 090eea51da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
090eea5 to
959dacb
Compare
959dacb to
7eef2a9
Compare
7eef2a9 to
f7d7e38
Compare
Unit-test the splitter, resolvers, aggregators and cursor routers, and add cluster-docker integration coverage with one assertion per reachable request/response policy type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f7d7e38 to
91ea94d
Compare
WAITAOF is tagged all_shards/agg_min but its reply is an array ([numlocal, numreplicas]); the scalar aggregateMin threw "All replies must be numbers", breaking every raw cluster fan-out. Fold array replies per-position — matching the server's AGG_MIN/AGG_MAX semantics and the existing element-wise logical AND/OR aggregators — via a shared helper; scalar aggregation is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The server flags TOUCH `[readonly, fast]` (no `write`), so the derived isReplicaSafe would route it to a replica. TOUCH only bumps LRU/LFU access metadata that drives eviction on the master, so it must stay master-only. Restore the explicit IS_READ_ONLY: false override (dropped when overrides were replaced by metadata derivation). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The metadata-derived isReplicaSafe returns false for every keyless command (it cannot tell a keyless read from keyless admin like SHUTDOWN), so keyless reads that master routed to replicas now go master-only in sentinel. Restore the explicit IS_READ_ONLY: true override on the keyless data reads — DBSIZE, KEYS, SCAN, RANDOMKEY, the RediSearch reads (FT.SEARCH/SPELLCHECK/EXPLAIN/ EXPLAINCLI/INFO/DICTDUMP/TAGVALS/SYNDUMP/PROFILE) and the time-series reads (TS.MGET/MRANGE/MREVRANGE/QUERYINDEX) — matching what master declared. Each is a non-mutating read over replicated data. FT.AGGREGATE is deliberately left master-only (WITHCURSOR mints server-side cursor state), as on master. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The cluster and sentinel MULTI/pipeline builders passed command.IS_READ_ONLY raw, bypassing the metadata resolver the standalone path uses. After the overrides were dropped in favor of derivation, a read-only command in a pipeline (e.g. GET) passed `undefined` and routed to the master even though the same command standalone reaches a replica. Route the std and module builders through isReplicaSafe(lookup(id), command.IS_READ_ONLY) — override first, else server metadata — memoized per command method like the standalone path. Function/script builders keep their explicit IS_READ_ONLY, unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa9fa83fa2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
`&&`/`||` return the operand, not a boolean, so a future agg_logical_and/or command returning non-binary numbers would leak values (1 && 2 === 2). Clamp each element to 0/1. Correct today for SCRIPT EXISTS (0/1 inputs); this future-proofs the registered reducers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The FT.CURSOR cluster tests retry the initial aggregate until the freshly created index has propagated to the coordinator, but the retry predicate only matched the 7.x "No such index" wording. On 8.x the server returns "SEARCH_INDEX_NOT_FOUND Index not found", which slipped past the predicate and rethrew on the first racy attempt (intermittent CI failures). Broaden the predicate to match both wordings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PavelPashov
left a comment
PavelPashov
left a comment
There was a problem hiding this comment.
Could we document these clustering changes and add release notes before merging? Cross-slot mGet/mSet/del now succeed, cluster.scan() uses instance-specific virtual cursors, keys()/dbSize()/flushAll() fan out, and raw sendCommand(['DBSIZE']) has new reply semantics. This is a breaking change for raw-path users that docs/clustering.md doesn’t cover.
…ail the route routeAllShards/routeAllNodes awaited getAllMasterClients()/getAllClients(), which Promise.all over lazy node connects — a single failed connect rejected the entire route before any reducer ran, so a one_succeeded command like SCRIPT KILL failed even when another shard was reachable. Return the fan-out target nodes without connecting (getAllMasterNodes/getAllNodes) and defer each node's connect into its own per-entry promise via a getClient thunk, resolved inside the plan's per-entry execution. A connect failure now rejects only that entry, so the reducer (Promise.any for one_succeeded, Promise.all for the all-must-succeed policies) sees the reachable shards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 86128d2. Configure here.
A blocking read (XREAD ... BLOCK) can return null on timeout; cached, that reply would be replayed instead of blocking for new data. Eligibility is per-command so the non-blocking form can't be carved out — add a `blocking` flag exclusion to isCacheable. Verified against a live COMMAND reply (447 commands, 130 cacheable): XREAD is the only command this flips. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a28d8ec06d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| publish(CHANNELS.COMMAND_REPLY, () => ({ args: sanitizeArgs(parser.redisArgs), reply: finalReply, clientId: this._self._clientId })); | ||
| // Override-first: a defined `Command.CACHEABLE` wins; otherwise CSC | ||
| // eligibility derives from the server flags/tips (see `isCacheable`). | ||
| const cacheable = isCacheable(defaultCommandMetadata.lookup(parser.commandIdentifier), command.CACHEABLE); |
There was a problem hiding this comment.
When client-side caching is enabled, deriving cacheability from metadata makes JSON.GET/JSON.MGET cacheable even though their reviver lives in parser.preserve and is not part of redisArgs. handleCache keys only on the wire args and returns the already transformed value on a hit, so json.get('k', { reviver: a }) followed by the same JSON.GET with reviver b will skip b and return a clone of a's transformed result until the key is invalidated; keep these commands explicitly non-cacheable or include the preserve/transform context in the cache entry.
Useful? React with 👍 / 👎.
2 participants
Description
Checklist
npm testpass with this change (including linting)?Note
High Risk
Large behavioral change to cluster command routing (cross-slot MGET, fan-out admin commands, cursor state); incorrect policy or aggregation logic could misroute writes or return wrong merged results.
Overview
Adds cluster-wide command routing driven by server command metadata: resolve request/response policies, fan out or split commands across nodes, aggregate replies, and fall back to key-based routing when metadata is missing.
Execution path: Cluster commands and
sendCommandgo through_executeWithPolicies(policy resolution → router → per-node_executewith MOVED/ASK → reducer → optional post-hooks). Single-key default-keyed traffic keeps a fast path. Multi-shard commands (e.g.MGET,MSET,DEL) are split by hash slot using COMMAND key specs and replies are reassembled in caller key order. All-shards / all-nodes policies fan out with reducers (sum, min, merge,one_succeeded, etc.) and numeric aggregates re-apply type mappings after aggregation.Special cursors: Virtual tokens and bindings on
RedisClusterSlotsimplement cluster-wide SCAN (walk masters sequentially) and sticky FT.CURSOR / FT.AGGREGATE routing so keyless cursor commands hit the correct node.Parser / metadata:
CommandParsergainscommandIdentifierandmarkRoutingKeyfor raw cluster sends; client-side cache eligibility usescommand-metadata+isCacheablewhen CSC is enabled. Bloom module commands drop per-commandIS_READ_ONLY(replica safety comes from metadata). Cluster multi pipelines useisReplicaSafethe same way.Reviewed by Cursor Bugbot for commit a28d8ec. Bugbot is set up for automated code reviews on this repo. Configure here.