Repository navigation
Route SCRIPT KILL and FUNCTION KILL to every primary in cluster mode - #4568
LindseyZ1205 wants to merge 4 commits into
Conversation
Neither command was in the static metadata or in COMMAND_FLAGS, and both are keyless, so the cluster clients raised "No way to dispatch". Record the request_policy:all_shards and response_policy:one_succeeded tips the server reports for them, and handle ONE_SUCCEEDED as the tip says: send the command to every target and return the first reply that is not an error, raising one only if every target replies with one.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 3e7bb1d212
ℹ️ 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".
| RequestPolicy.SPECIAL: self.get_special_nodes, | ||
| ResponsePolicy.DEFAULT_KEYLESS: lambda res: res, | ||
| ResponsePolicy.DEFAULT_KEYED: lambda res: res, | ||
| ResponsePolicy.ONE_SUCCEEDED: lambda res: res, |
There was a problem hiding this comment.
When a sync ClusterPipeline queues execute_command("FUNCTION KILL", target_nodes=node) and a response callback is registered for that command, the explicit target avoids the multi-target rejection but the newly resolved ONE_SUCCEEDED response policy is retained. PipelineStrategy._send_cluster_commands() then indexes the pipeline's own _policies_callback_mapping, which still contains only the two default response policies, so this previously supported callback path now raises KeyError instead of returning the callback result; mirror this entry in ClusterPipeline.__init__ as well (the async pipeline reuses the client map).
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
A pipeline sends FUNCTION KILL to a single target and keeps its ONE_SUCCEEDED response policy, so with a response callback registered for the command, the pipeline's own policy callback map raised KeyError.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf32034d6f
ℹ️ 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".
| "exists": _CACHEABLE_KEYED, | ||
| "expiretime": _CACHEABLE_KEYED, | ||
| "fcall_ro": _SCRIPT_RUNNER_MOVABLE_KEYS, | ||
| "function kill": _WRITE_KEYLESS_ALL_SHARDS_ONE_SUCCEEDED, |
There was a problem hiding this comment.
When FUNCTION KILL is queued in a cluster pipeline with a single explicit node, a read/write ConnectionError triggers the pipeline retry after the sync strategy has already popped target_nodes at redis/cluster.py:5825; it then calls parent_execute_command at line 6037 without that target. This new ALL_SHARDS policy consequently broadcasts the retry to every primary, so a transient failure can terminate a function on a shard the caller did not select; the async strategy likewise pops the kwarg at redis/asyncio/cluster.py:3795 but retries into “Too many targets,” so the stacks diverge. Preserve the explicit target or disable retries for explicitly targeted pipeline commands.
AGENTS.md reference: AGENTS.md:L146-L149
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
A ConnectionError or TimeoutError from one target was raised even after another target had answered, so a SCRIPT KILL that had stopped the script was retried, and the retry got NOTBUSY from every shard. The sync client also stopped at that target, without trying the others. Now an error that isn't a reply only matters when no target succeeds, and it is then raised before the error replies, as the script may run on that node.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1127b3c4a
ℹ️ 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".
| if first_error is None: | ||
| first_error = e |
There was a problem hiding this comment.
When a running script or function has already performed a write, its shard replies UNKILLABLE while every other primary replies NOTBUSY. Keeping the first ResponseError (and the async fallback to replies[0] at redis/asyncio/cluster.py:1644-1650) means that whenever a nonbusy primary is ordered first, script_kill()/function_kill() report NOTBUSY even though the execution is still running and cannot be killed. Prefer a non-NOTBUSY reply when no shard succeeds, using NOTBUSY only when all shards return it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit c1127b3. Configure here.
When no target succeeded, the first error reply was raised, so the NOTBUSY of a node that runs no script could hide the UNKILLABLE of the node whose script can't be stopped. CLUSTERDOWN, a ResponseError that the retry handling applies to, was also taken as a reply, so an earlier NOTBUSY was raised and the command wasn't retried. Now an error that isn't a reply or that the retry handling applies to comes first, then the other error replies, and NOTBUSY last.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33dc0ca859
ℹ️ 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".
| # the script or function, while the others answer NOTBUSY. | ||
| _WRITE_KEYLESS_ALL_SHARDS_ONE_SUCCEEDED = replace( | ||
| _WRITE_KEYLESS, | ||
| request_policy=RequestPolicy.ALL_SHARDS, |
There was a problem hiding this comment.
When a long-running EVAL_RO/EVALSHA_RO or FCALL_RO is routed to a replica (for example with read_from_replicas or a replicas-only load-balancing strategy), this policy expands via get_primaries() at redis/cluster.py:1208, so script_kill()/function_kill() only collect NOTBUSY from primaries and leave the replica's execution running. Those read-only runners are explicitly replica-safe in the static metadata and get_nodes_from_slot() selects replicas for them, so the kill broadcast must include replicas (or all nodes), not only shards' primaries.
AGENTS.md reference: AGENTS.md:L159-L163
Useful? React with 👍 / 👎.
|
Hey @LindseyZ1205, thank you for your contribution! The problem is real: There is one difference worth noting. This PR keeps a success when another shard can't be reached. #4569 fails the command so it can be retried. If you have a real scenario where that retry hides a successful kill, please add it on #4569. |
Description of change
script_kill()andfunction_kill()can't be used withRedisCluster:Neither command is in
COMMAND_FLAGSor in the static command metadata, and they take no key, so the client has nothing to route them by.execute_command("SCRIPT KILL", target_nodes=RedisCluster.PRIMARIES)fails the same way, because a nodes flag still goes through that lookup.The server tips both commands
request_policy:all_shardsandresponse_policy:one_succeeded: the script or function runs on one shard, and the others answerNOTBUSY. This adds both to the static metadata with those policies, and makes the cluster clients handleONE_SUCCEEDEDthe way the tip describes: the command is sent to every target and the first reply that isn't an error is returned, whatever the other targets answer, since retrying a kill that succeeded would only getNOTBUSY. An error is raised only if no target succeeds, soNOTBUSYstill comes back when nothing is running, as with a standalone client. An error that isn't a reply or that the retry handling applies to, such as aConnectionErrororCLUSTERDOWN, then comes first, as the script may run on that node, and it goes through the retry handling as before. Next comes an error reply such asUNKILLABLE, from a node whose script can't be stopped, andNOTBUSYcomes last.Until now
ONE_SUCCEEDEDcould only come from a server-derived resolver, and it didn't work there either. With aDynamicPolicyResolver, the sync client sentSCRIPT KILLto the first primary only and raised itsNOTBUSYwhen the script ran elsewhere. When the script ran on the first primary, it was killed, but_process_result()then raisedKeyError: <ResponsePolicy.ONE_SUCCEEDED: 'one_succeeded'>, because the response-policy callbacks only knew the two defaults. The asyncio client had noONE_SUCCEEDEDhandling: it sent the command to every primary and raised the firstNOTBUSY, even when the script was killed.FUNCTION KILLisn't inPIPELINE_BLOCKED_COMMANDS, so in a cluster pipeline it now raises "Too many targets", like other commands that go to every primary such asFUNCTION FLUSH, instead of going to a single node. With a single node intarget_nodes, it still goes to that node, and a response callback set for it still applies: the sync cluster pipeline, which has its own response policy callbacks, knowsONE_SUCCEEDEDtoo.#2599, which was closed for inactivity, listed both commands among the ones that should be sent to all primaries.
With a
DynamicPolicyResolver, commands tipped with the other aggregate response policies still raiseKeyErrorin_process_result()the same way. I checkedDBSIZE(agg_sum),PING,CONFIG SET,CLIENT SETNAMEandSCRIPT FLUSH(all_succeeded),SCRIPT EXISTS(agg_logical_and), andRANDOMKEYandFUNCTION STATS(special). I left those out of this change.Tests
test_kill_reaches_every_primary_and_returns_the_first_successandtest_kill_raises_when_every_primary_replies_with_an_errorinTestStaticMetadataRouting, for both clients and both commands, fail without this change. I updated the docstrings oftest_explicit_target_nodes_do_not_apply_the_commands_aggregation, which described the oldONE_SUCCEEDEDhandling.test_pipeline_function_kill_on_one_node_reaches_its_response_callback, for both clients, runsFUNCTION KILLon one node in a pipeline with a response callback set. Without the sync pipeline'sONE_SUCCEEDEDentry, the sync one fails withKeyError.test_kill_succeeds_when_another_primary_cannot_be_reachedandtest_kill_raises_the_error_of_an_unreachable_primary_first, for both clients, cover a primary that can't be reached, before and after the others.test_kill_raises_another_error_rather_than_notbusy, for both clients, checks thatUNKILLABLEorCLUSTERDOWNfrom one primary is raised rather than theNOTBUSYof the others. Against a local Redis 8.2.1 cluster, with a script or function that has written running on the last primary,script_kill()andfunction_kill()now raiseUNKILLABLEwith both clients, where they raisedNOTBUSY.busy-reply-threshold 100, with a script or a function running on the last primary,script_kill()andfunction_kill()stop it with both clients: by default, withtarget_nodes=RedisCluster.PRIMARIES, and with aDynamicPolicyResolver. With nothing running they raiseNOTBUSY.tests/test_cluster.py,tests/test_cluster_transaction.py,tests/test_pubsub.pyand their asyncio versions give the same results with and without this change, against a local Redis 8.2.1 cluster (3 primaries, 3 replicas) with--protocol2 and 3, and in afixed_clientrun. So dotests/test_command_metadata.pyand its asyncio version. The only failures aretest_geosearchstore_distandtest_cluster_georadius_store_dist, whose distances differ in the last digits with my Redis build, the asyncioTestSSLtests, which need certificates I don't have locally, and the metadata tests that need the module server onlocalhost:6479.COMMAND INFOreply of Redis 8.2.1 (flagsnoscriptandallow_busy, and the two tips). I don't have the 8.10 server that the table's provenance note mentions, so I couldn't check them against it.ruff check,ruff format --check(0.9.6) andvulturepass.Note
Medium Risk
Changes core multi-node command dispatch and error aggregation for a new response policy, affecting SCRIPT/FUNCTION KILL and any metadata using ONE_SUCCEEDED, though behavior is scoped and heavily tested.
Overview
SCRIPT KILL and FUNCTION KILL now work on cluster clients by registering them in static metadata with
request_policy:all_shardsandresponse_policy:one_succeeded, matching Redis server tips.Cluster execution for
ONE_SUCCEEDEDis implemented in sync, async, and pipeline paths: the command is sent to every resolved target, the first non-error reply is returned (others may answerNOTBUSY), and if none succeed_one_succeeded_errorpicks the raised error (connectivity/retryable errors beforeUNKILLABLE-style replies, withNOTBUSYlast). The sync client no longer stops after the first primary; the async client no longer fails on the firstNOTBUSYwhile a later shard succeeded.Tests cover all-primaries routing, all-
NOTBUSY, unreachable nodes, preferring serious errors overNOTBUSY, and pipelineFUNCTION KILLwith response callbacks; docstrings for explicittarget_nodesvs cluster aggregation were updated.Reviewed by Cursor Bugbot for commit 33dc0ca. Bugbot is set up for automated code reviews on this repo. Configure here.