fix(cluster): rebind abort/timeout listeners when a command moves to another queue - #3367
Conversation
![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: 6e53e66ac9
ℹ️ 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".
| }; | ||
| value.slotNumber = options?.slotNumber; | ||
|
|
||
| const node = this.#toWrite.add(value, options?.asap); |
There was a problem hiding this comment.
When commandOptions.timeout is negative, non-integer, infinite, or otherwise outside AbortSignal.timeout's accepted range, that call throws after this line has already inserted the command. The Promise constructor converts the exception into a rejected promise, but sendCommand still calls #scheduleWrite, so Redis may execute a command whose caller already received a RangeError. Previously the node was added only after timeout and abort-listener setup succeeded; construct those resources before insertion or remove the node if setup fails.
Useful? React with 👍 / 👎.
6e53e66 to
cdb403b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdb403b790
ℹ️ 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".
| value.abort.listener = this.#createAbortListener(node, value); | ||
| value.abort.signal.addEventListener("abort", value.abort.listener, { | ||
| once: true, |
There was a problem hiding this comment.
When a migrated command has both a timeout and an abort signal and both fire while it remains queued, the first listener removes its destination node but leaves the other listener registered. The second listener then calls DoublyLinkedList.remove on the already-detached node; because remove is not idempotent, this decrements the queue length again and can reset its head or tail, dropping unrelated queued commands. Remove the sibling listener when either cancellation wins, or otherwise ensure the node is removed only once.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
cdb403b to
870d447
Compare
|
@GiHoon1123 , thanks, the underlying bug is valid and this is the right layer for the fix. Two points before merge. First, extraction now detaches listeners unconditionally, so a command that is extracted but never prepended (destMasterNode.client is optional-chained and lastDestNode can be undefined) loses cancellation entirely and its promise never settles, whereas master would still reject it. Please guarantee reject-or-rebind and add a test for that path. Second, this edits the same extractAllCommands loop that #3364 fixes, so please rebase on top of #3364 once it lands. |
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 c3a5280. Configure here.
|
Addressed the two cases you mentioned. Extracted commands are now rejected when no destination queue is available, and abort/timeout listeners are rebound when commands are prepended to a new queue. I also added a regression test for the extracted-but-not-prepended case. I'll rebase #3367 on top of #3364 once it merges and resolve the overlapping |
|
@GiHoon1123 #3364 is now merged. After the rebase, please add one test that extracts multiple queued commands via extractAllCommands and asserts every command's listeners were detached (e.g. each one rebinds and cancels correctly from the destination). |
…another queue addCommand's abort/timeout listeners close over the queue and linked-list node a command was originally queued on. When cluster-slots.ts moves a command to another node's queue (extractAllCommands/extractCommandsForSlots + prependCommandsToWrite), only the command's value moves — the listener still points at the old queue and the now-detached old node. If the signal fires after the move, the promise still rejects, but removal targets the old (already torn down) queue instead of the new one, so the command can be left behind and sent to the server after the caller already saw it as canceled. Rebind the listeners to the node/queue they're prepended into, and handle the case where the signal already fired before the move by rejecting immediately instead of attaching a listener that can never fire. Fixes redis#3365
Add a regression test extracting several queued commands at once via extractAllCommands and confirming each one's abort listener was independently rebound to its own node in the destination queue, in an order that doesn't corrupt the destination's linked list for the others still queued.
…versable The multi-command rebind test only checked pendingCount after each abort, which wouldn't catch a bug that rejected the wrong command or corrupted the linked list while still landing on the right count by coincidence. Track each command's settlement explicitly and assert only the aborted one flips, and replace the final pendingCount check with an actual extractAllCommands() call so the destination queue's traversal path is exercised, not just its length counter.
- Move the rejectCommands test out of the prependCommandsToWrite describe block into its own, since it exercises a different method. - Add explicit signal.aborted assertions in the two 'timeout already fired before prepend' tests, so a slow timer under CI can't silently make the test pass through the wrong branch. - Add a second rejectCommands case going through extractCommandsForSlots(), mirroring cluster-slots.ts's partial-slot- migration path (the full rejectCommands test already covered the full-node-loss path via extractAllCommands()).
c3a5280 to
f61ee5d
Compare
|
Rebased on top of #3364 and added the multi-command test. |
2 participants
Fixes #3365.
During cluster slot migration, queued commands can move from one node queue to another. The abort and timeout listeners used to keep pointing at the original queue and linked-list node.
After a command moved, cancellation could reject the caller while leaving the command in the destination queue. The command could then still be sent.
This rebinds the listeners when commands are prepended to a new queue. If a signal has already fired, the command is rejected and removed instead. Extracted commands are also rejected when no destination client is available.
Tests cover abort and timeout after a move, signals that fired before prepend, listener cleanup, and the extracted-but-not-prepended case.
This branch overlaps #3364's extractAllCommands() change and will be rebased after #3364 merges.
Note
Medium Risk
Touches core command-queue lifecycle during cluster SMIGRATED handling; incorrect listener binding could still send aborted/timed-out commands, but behavior is heavily covered by new tests.
Overview
Fixes cluster slot migration so queued commands stay consistent when they move between node write queues.
RedisCommandsQueuenow centralizes cancellation in#rejectCommand(with arejectedguard), detaches abort/timeout listeners when commands are extracted, andprependCommandsToWriterebinds those listeners to the destination queue’s linked-list nodes. If abort or timeout already fired before prepend, the command is rejected and dropped instead of staying queued.rejectCommandshandles extracted commands that cannot be forwarded.addCommandenrolls the command in the queue before attaching listeners and rolls back on setup failure (e.g. invalid timeout).cluster-slotsrejects migrated or leftover commands withDisconnectsClientErrorwhen no destination client exists, instead of prepending into a missing client.Broad unit tests cover migration-style extract/reject paths and abort/timeout behavior after moves.
Reviewed by Cursor Bugbot for commit f61ee5d. Bugbot is set up for automated code reviews on this repo. Configure here.