fix(cluster): make extractAllCommands drain the write queue - #3364
fix(cluster): make extractAllCommands drain the write queue#3364GiHoon1123 wants to merge 1 commit into
Conversation
remove() clears a node's own next/previous pointers, so reading current.next after removing current always came back undefined and ended the loop early. Same fix as extractCommandsForSlots: grab next before removing. Fixes redis#3363
![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: cf393bdaf0
ℹ️ 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".
| this.#toWrite.remove(current); | ||
| const toRemove = current; | ||
| current = current.next; | ||
| this.#toWrite.remove(toRemove); |
There was a problem hiding this comment.
When a remaining command has an abort signal or timeout and a full-shard migration calls this method, its listener still closes over the original source queue and linked-list node created in addCommand. The cluster caller then passes only the command value to prependCommandsToWrite, which creates a different destination node; if cancellation fires before the destination writes it, the promise is rejected but removal from the now-empty source queue is a no-op, so the destination retains and can execute the canceled command. Rebind these listeners to the destination node as part of transferring the command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, but pre-existing — goes back to the V5 rewrite, not something this PR touches.
The old bug just hid it by dropping most commands instead of moving them. Filing a
separate issue for this.
| result.push(current.value); | ||
| this.#toWrite.remove(current); | ||
| const toRemove = current; | ||
| current = current.next; |
There was a problem hiding this comment.
When a source is split across multiple destinations while a transaction or pipeline is still queued, _executeMulti and _executePipeline add their commands without slotNumber, so extractCommandsForSlots ignores the entire batch. This newly complete drain then lets cluster-slots.ts prepend all of those commands to lastDestNode, which is merely the final destination in notification order and may not own the batch's key slot; the commands consequently receive MOVED errors or an aborted transaction instead of surviving the handoff. Preserve the selected batch slot on these queue entries or otherwise route the extracted chain to the destination owning that slot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Also pre-existing — _executeMulti/_executePipeline not setting slotNumber goes
back to the V5 rewrite too, not something this PR touches. Fixing it properly means
threading slot info through batch commands, bigger than this PR. Filing a separate
issue.
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
@GiHoon1123, thanks for the fix and the focused regression test. Confirmed against master: remove() clears the node links before next is read, so only the head command was ever relocated during a full-shard handoff.
2 participants
Fixes #3363.
extractAllCommands()removed the current linked-list node before readingcurrent.next.DoublyLinkedList.remove()clears the removed node's links, sothe loop stopped after the first command and left the rest in the write queue.
This stores the next node before removing the current one, matching the pattern
used by
extractCommandsForSlots().Also corrected the method comment, since this method does remove the commands it
returns.
Test added for extracting multiple queued commands and leaving the queue empty.
Note
Medium Risk
Cluster slot migration relies on extractAllCommands to relocate slotless commands; the prior bug could leave commands stranded on a node with no slots.
Overview
Fixes a bug in
extractAllCommands()where only the first pending write-queue command was returned. The loop calledremove()before advancingcurrent, andDoublyLinkedList.remove()clears the removed node’s links, socurrent.nextwas lost and the rest of the queue stayed behind.The implementation now saves the next node, then removes the current one—the same pattern as
extractCommandsForSlots()—so the full write queue is extracted and drained. The method comment is updated to state that commands are removed, not merely read.A unit test queues five commands and asserts they are all returned in order and that a second
extractAllCommands()yields an empty queue.Reviewed by Cursor Bugbot for commit cf393bd. Bugbot is set up for automated code reviews on this repo. Configure here.