fix(cluster): keep batch commands routed by slot during migration - #3377
fix(cluster): keep batch commands routed by slot during migration#3377GiHoon1123 wants to merge 6 commits into
Conversation
…chain guard to full-node-loss only
- groupCommandsByDestination() now returns {byDestination, unrouted}
instead of silently continue-ing past commands with neither a slot
owner nor a fallback destination. The full-node-loss caller now
rejects unrouted commands explicitly instead of leaving them
extracted-but-never-settled (the same failure mode redis#3365 fixed).
- Removed the in-flight-chain-tail rejection from the partial-slot-
migration path. That source connection isn't destroyed - it keeps
serving its remaining slots - so rejecting the queued tail doesn't
clear whatever transaction state the server still thinks is open on
it. The full-node-loss path keeps the guard, since that connection is
destroyed right after.
- Renamed a chainInExecution test whose name didn't match its
assertion.
…ctCommandsForSlots Once MULTI/pipeline commands carry a slotNumber, extractCommandsForSlots() (used during partial slot migration) started matching them - including a chain whose head was already written to the socket, with only its tail still queued. Relocating just that tail to another node would split the transaction across two connections, and unlike full node loss, this source connection isn't going away. Skip a command when its chainId matches chainInExecution, guarded so an ordinary non-chain command (chainId undefined) isn't mistaken for the tail of nothing (chainInExecution also undefined). The tail is left queued and sent to the original node as before; if the key has since moved, that's a normal MOVED/ASK error on the existing retry path. Caught this by reproducing the exact sequence directly against the queue: send MULTI, then call extractCommandsForSlots() and confirm the queued SET/EXEC come back.
… skipping past it extractCommandsForSlots() was only skipping the in-flight chain's own tail commands, then continuing to extract anything queued after them that matched the requested slots. Since commands on one connection are sent in strict queue order, relocating a later command to a different connection while the chain ahead of it is still pending could let it run out of order - e.g. a GET queued right after an in-flight MULTI/SET/EXEC could execute on the new node before the transaction completes on the old one, reading a stale value. Once the scan reaches the in-flight tail, stop extracting entirely - everything from that point stays on this connection, in original order. Commands queued ahead of the chain aren't affected: by the time any part of a chain is in flight, anything queued before it has necessarily already been sent (this queue processes strictly FIFO), so there's nothing earlier left in #toWrite to reorder. Also corrected a comment in cluster-slots.ts that said the partial- migration path doesn't special-case in-flight chains, when the special- casing actually lives inside extractCommandsForSlots() itself.
…K handling MULTI/pipeline execution doesn't go through cluster's redirect-and-retry loop (that's sendCommand-only, via _execute()) - calling it an 'existing retry path' overstated what actually happens for batches. It's a normal error surfaced to the caller, not something automatically retried.
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
A few things:
- Please add a test for the case where a second chain is fully queued but not yet written behind an already-in-flight chain, asserting the second chain relocates atomically and isn't mistaken for the in-flight tail.
- Please add an assertion that on full node loss, the already-sent head of an in-flight chain (in waitingForReply) is also rejected, so the whole transaction fails consistently rather than half-committing.
…d rejection Addresses review feedback on PR redis#3377 (nkaradzhov): - extractCommandsForSlots now has a test proving a second, fully-queued chain behind an in-flight one on a different slot still relocates atomically and isn't mistaken for the in-flight chain's tail. - A full-node-loss test proves the already-sent head of an in-flight chain (in #waitingForReply) rejects alongside its queued tail, so the transaction fails consistently instead of half-committing. - Extracted the full-node-loss path's in-flight-tail/relocatable split into splitInFlightChainTail() (mirroring the existing groupCommandsByDestination pattern) so it can be unit tested directly, and added a test proving it separates an in-flight chain's tail from an unrelated, fully-queued chain queued right behind it.
![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: 941c90c7c3
ℹ️ 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 (this.#chainInExecution !== undefined && current.value.chainId === this.#chainInExecution) { | ||
| break; |
There was a problem hiding this comment.
When a migrating slot's queue has a partially written MULTI/pipeline at the front, this break leaves not only that unsafe tail but also any fully queued batch behind it on the old source. The SMIGRATED handler then waits for in-flight replies and, if the source still owns other slots, simply unpauses the source without re-running extraction; singles can recover through the cluster redirect loop, but RedisCluster.MULTI dispatches directly to client._executeMulti/_executePipeline (packages/client/lib/cluster/index.ts lines 599-604), so a later fully queued batch for the moved slot is sent to the old node and fails with MOVED/CROSSSLOT even though it was safe to relocate after the first chain drained.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think handling it would require changing the partial-migration drain/re-extraction flow, not just
preserving the selected slot on queued batch commands. I’d prefer to keep this PR focused on #3366 and the maintainer-requested coverage unless the maintainers want this handled here.
| // relocated on its own - it would run as a fragment of the | ||
| // transaction on a different node than the part already sent. | ||
| const chainInExecution = sourceNode.client._getQueue().chainInExecution; | ||
| const { inFlightChainTail, relocatable } = splitInFlightChainTail(remainingCommands, chainInExecution); |
There was a problem hiding this comment.
When a large execAsPipeline() is only partly written before a full source-node migration, waitForInflightCommandsToComplete() can drain the sent prefix first, but chainInExecution still points at that pipeline symbol. This split then classifies the unsent pipeline tail as an in-flight transaction fragment and rejects it, even though pipelines have no server-side MULTI state once the sent replies are back; only MULTI tails need this rejection, while drained pipeline tails should still be moved to their slot owner instead of failing the batch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think fixing this properly would need distinguishing MULTI chains from pipeline chains in the
queue metadata, which is broader than the slot-routing fix in this PR. I’d prefer to leave this out of this PR unless the maintainers want it included here.
…d rejection Addresses review feedback on PR redis#3377 (nkaradzhov): - extractCommandsForSlots now has a test proving a second, fully-queued chain behind an in-flight one on a different slot still relocates atomically and isn't mistaken for the in-flight chain's tail. - A full-node-loss test proves the already-sent head of an in-flight chain (in #waitingForReply) rejects alongside its queued tail, so the transaction fails consistently instead of half-committing. - Extracted the full-node-loss path's in-flight-tail/relocatable split into splitInFlightChainTail() (mirroring the existing groupCommandsByDestination pattern) so it can be unit tested directly, and added a test proving it separates an in-flight chain's tail from an unrelated, fully-queued chain queued right behind it.
941c90c to
aa31b59
Compare
…d rejection Addresses review feedback on PR redis#3377 (nkaradzhov): - extractCommandsForSlots now has a test proving a second, fully-queued chain behind an in-flight one on a different slot still relocates atomically and isn't mistaken for the in-flight chain's tail. - A full-node-loss test proves the already-sent head of an in-flight chain (in #waitingForReply) rejects alongside its queued tail, so the transaction fails consistently instead of half-committing. - Extracted the full-node-loss path's in-flight-tail/relocatable split into splitInFlightChainTail() (mirroring the existing groupCommandsByDestination pattern) so it can be unit tested directly, and added a test proving it separates an in-flight chain's tail from an unrelated, fully-queued chain queued right behind it.
aa31b59 to
571ed55
Compare
…d rejection Addresses review feedback on PR redis#3377 (nkaradzhov): - extractCommandsForSlots now has a test proving a second, fully-queued chain behind an in-flight one on a different slot still relocates atomically and isn't mistaken for the in-flight chain's tail. - A full-node-loss test proves the already-sent head of an in-flight chain (in #waitingForReply) rejects alongside its queued tail, so the transaction fails consistently instead of half-committing. - Extracted the full-node-loss path's in-flight-tail/relocatable split into splitInFlightChainTail() (mirroring the existing groupCommandsByDestination pattern) so it can be unit tested directly, and added a test proving it separates an in-flight chain's tail from an unrelated, fully-queued chain queued right behind it.
571ed55 to
2b3e959
Compare
|
Thanks for the review. I pushed an update. This now covers the extractCommandsForSlots case (a second, fully-queued chain behind an in-flight I also looked at the automated review comments. They look like broader follow-ups to me, so I left |
2 participants
Fixes #3366.
Rebased after #3367 landed.
Cluster MULTI/pipeline execution was already selecting the right slot, but the queued commands
did not keep that
slotNumber. During maintenance slot migration, the full source-nodecleanup path could then fall back to the last processed destination for remaining queued batch
commands.
This tags the whole MULTI/pipeline chain with the selected slot and uses that slot when moving
queued commands during source cleanup. Commands without a route are rejected instead of being
dropped.
An already in-flight chain (its head already written to the socket, tail still queued) is
handled differently depending on whether the connection survives:
left queued there instead of being split onto another connection.
risking a split transaction.
Tested with:
24 tests passing, 0 lint errors, type check clean.
Note
Medium Risk
Touches cluster maintenance command relocation and MULTI/pipeline queue semantics; incorrect handling could reorder commands or leave transactions half-applied during migrations.
Overview
Fixes cluster MULTI/pipeline behavior during slot migration and node loss by tagging every command in a batch with the selected slot and teaching the command queue and maintenance logic how to move or reject queued work without splitting transactions.
Cluster
multi()/exec()andexecAsPipeline()now passslotNumberinto_executeMulti/_executePipeline, so each queued command carries the slot used for routing. On SMIGRATED cleanup when a source shard loses all slots, remaining commands are grouped by current slot owner viagroupCommandsByDestinationinstead of dumping everything on the last destination; commands with no route are rejected rather than dropped.The queue exposes
chainInExecutionandextractCommandsForSlotsstops when it hits the queued tail of a partially sent MULTI/pipeline (and leaves later commands on that connection), avoiding reordering or splitting an open transaction during partial migration. On full node loss,splitInFlightChainTailrejects the in-flight chain’s queued tail while still relocating unrelated fully queued chains atomically.Reviewed by Cursor Bugbot for commit 2b3e959. Bugbot is set up for automated code reviews on this repo. Configure here.