fix(cluster): rebind abort/timeout listeners when a command moves to another queue - #3367
fix(cluster): rebind abort/timeout listeners when a command moves to another queue#3367GiHoon1123 wants to merge 2 commits into
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}
…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
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.
| if (command.timeout) { | ||
| RedisCommandsQueue.#removeTimeoutListener(command); | ||
| } | ||
| result.push(command); |
There was a problem hiding this comment.
Loop only processes first element due to remove-before-advance
High Severity
The newly added listener-removal code in extractAllCommands (lines 729–735) only executes for the first command in the queue. DoublyLinkedList.remove() sets node.next = undefined, so current = current.next on line 738 always yields undefined after the first iteration, ending the loop. Compare with extractCommandsForSlots which correctly saves current = current.next before calling remove(toRemove). During cluster migration with multiple queued commands, only the first gets its listeners detached; the rest remain in the queue with stale listeners still bound to the old node.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c3a5280. Configure here.
There was a problem hiding this comment.
|
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 |
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.