Skip to content

Handle ASK redirects to slotless nodes during resharding - #4214

Draft
lh0156 wants to merge 1 commit into
redis:masterfrom
lh0156:agent/ask-redirect-resharding
Draft

Handle ASK redirects to slotless nodes during resharding#4214
lh0156 wants to merge 1 commit into
redis:masterfrom
lh0156:agent/ask-redirect-resharding

Conversation

@lh0156

@lh0156 lh0156 commented Jul 26, 2026

Copy link
Copy Markdown

Fixes #4015

What changed

  • Preserve an ASK redirect target for the current retry even when it is absent from the refreshed slots cache.
  • Create a temporary primary ClusterNode only when the redirect target is unknown.
  • Keep the existing ASK sequence unchanged: send ASKING, then retry the original command.
  • Do not patch slot ownership or retain the temporary node in nodes_cache.
  • Apply the same behavior to synchronous and asynchronous cluster clients.
  • Add deterministic regression tests for an ASK target with no slots.

Verification

  • pytest -q tests/test_cluster.py -m onlycluster -k 'ask_redirection_to_node_without_slots' — 1 passed
  • pytest -q tests/test_asyncio/test_cluster.py -k 'ask_redirection_to_node_without_slots' — 1 passed
  • invoke linters
  • ruff format --check
  • git diff --check
  • python -m compileall

The full Redis Cluster integration suite requires local Redis cluster services, which are not available in this workspace.

@petyaslavova petyaslavova left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up, and for the precise scoping — the diagnosis is right. initialize() builds nodes_cache purely from CLUSTER SLOTS, so a slotless importing node is never in it and the ASK retry dereferences None in both stacks. That part of the fix is the direction we want.

Before we can merge it, the redirect target needs an owner. As written, the temporary ClusterNode is intentionally kept out of nodes_cache and startup_nodes, which are the only collections aclose() / set_nodes(remove_old=True) ever tear down, so every ASK-redirected command leaves a connection behind. On the async side ClusterNode.__del__ can only warn ("Unclosed ClusterNode object") — it cannot close the transport. On the sync side each redirect builds a fresh Redis client, connection pool, and handshake via create_redis_connections(). During a real reshard that is one orphaned connection per redirected command, in exactly the sustained-traffic scenario the issue describes.

Could you please:

  • make the ad-hoc node manager-owned and short-lived — reused for repeated redirects to the same address, and disconnected once it is no longer needed;
  • route the redirect address through remap_host_port() so address_remap users are covered;
  • extend the tests so they exercise real connection acquisition and release and assert that nothing is left behind after the command. The current tests patch out get_redis_connection / ClusterNode.execute_command, so they prove the None dereference is gone but not the resource behavior.

A rebase is also needed — the branch currently conflicts with master — and the PR is still a draft. Happy to review again once those are in.

@eeshsaxena eeshsaxena left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The slotless-node handling looks right. On the sync side ClusterNode(e.host, e.port, PRIMARY) is created with redis_connection=None, and the first time it is used get_redis_connection -> nodes_manager.create_redis_connections([node]) -> create_redis_node(..., **self.connection_kwargs), so TLS/auth/timeout config is still applied even though the node was built ad hoc for the ASK target. The async side passes **self.nodes_manager.connection_kwargs at construction since the async ClusterNode owns its own pool. Different mechanism, same outcome: both inherit the cluster's connection settings.

This also fixes the previous behavior where get_node(node_name=...) returned None for a node not in the slot map, and the next loop iteration would call execute_command("ASKING") on None. Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resharding redis cluster

3 participants