Fixing One Bug Woke Up Two More That Were Hiding Underneath

A record of tracking down and fixing cluster slot migration bugs in node-redis (PR #3364, #3367, #3377)

Redis is infrastructure I use every day. But if you asked me to explain exactly what the client does when a slot moves in cluster mode, I couldn't give a real answer. The docs only go as far as "the client detects it and resends to the new node," so I ended up reading the source.

node-redis is the official Node.js client maintained by Redis. I found and fixed a bug there that corrupted the command queue during slot migration. But the fix did something I didn't expect: fixing the first bug exposed two more bugs that had been hiding underneath it, for the first time. An automated review bot and a maintainer review both caught them, and tracing with git log -S showed both were pre-existing bugs, not something my fix introduced.

This post is a record of the order I found these three bugs in, and how I separated and fixed them one at a time. To get the outcome out of the way: all three PRs were merged into master. But what matters isn't the PR numbers — it's the chain reaction of how fixing one bug revealed the next.

---

Background — Queue structure and migration

Redis Cluster splits the entire keyspace into 16384 slots, and each node owns a subset of them. When nodes are added or removed, or when Redis Enterprise restarts a node for maintenance, slots move to different nodes (migration). The client has to detect this and resend commands for the moved slots to the new owning node.

node-redis's cluster client manages two queues per node connection.

Queue	Holds	Data structure	Reason
#toWrite	Commands not yet written to the socket, waiting their turn	DoublyLinkedList (doubly linked)	Needs O(1) removal from any position
#waitingForReply	Commands already sent, waiting only for a reply	EmptyAwareSinglyLinkedList (singly linked)	Always drains strictly in order from the front

When a command is added to #toWrite (via addCommand()), its abort/timeout listener captures the node it was placed on in a closure.

const node = this.#toWrite.add(value, options?.asap);

if (signal) {
  value.abort = {
    signal,
    listener: this.#createAbortListener(node, value), // captures the node reference in a closure
  };
  signal.addEventListener('abort', value.abort.listener, { once: true });
}

From here, this command can (1) be canceled by abort/timeout, (2) get moved to another node's queue by slot migration, or (3) get sent normally — there's no way to know at enqueue time which happens first. With an array, indices would keep shifting as other elements are added or removed, invalidating references; a linked list holds onto the node itself and only has to unlink that one node, so it stays valid regardless of what happens around it.

There's a design assumption worth calling out here. This O(1) removal assumes not just that "the command is currently in the queue," but that "the command still lives on the same node object it was originally queued on." If the command moves to a different queue (i.e., gets re-inserted as a new node), the old node reference the listener captured in its closure needs to be invalidated — but nowhere in the code was it specified whose job that was. Bug 2 comes from exactly this spot.

There was nothing wrong with the structure itself. The problem was in the code that used it, and migration was the only trigger that exposed it.

---

Bug 1 — Queue traversal stops at the first item

The original report (#3363) was filed by another contributor. When a node loses all its slots (at which point extractAllCommands() is called), it's supposed to drain every remaining command from the queue — but in practice, only the first one ever came out.

// Before fix
while (current) {
  result.push(current.value);
  this.#toWrite.remove(current);
  current = current.next; // reads a link that remove() already cut
}

The moment remove(current) runs, current.next gets cleared to undefined. Since this loop reads .next after deleting the node, it terminates after the first iteration. The remaining commands stay stuck in the queue of a node that no longer owns any slots at all.

The fix (#3364) is just to reorder things — grab the next node before removing the current one.

// After fix
while (current) {
  result.push(current.value);
  const toRemove = current;
  current = current.next; // grab the next one first
  this.#toWrite.remove(toRemove); // then remove it
}

It's a one-line ordering problem. But this fix meant the loop walked the entire queue for the first time ever. Since it had always stopped after one iteration before, whatever other defects sat on this path never had a chance to run.

As soon as I opened the PR, an automated review bot (chatgpt-codex-connector) scanned the commit and left two inline comments. Neither was about style — both pointed at real runtime behavior. These two comments turned into bugs 2 and 3.

---

Bug 2 — A listener references the old node after a command moves

The first comment pointed out that addCommand()'s abort/timeout listener keeps holding onto the node the command was originally placed on.

listener: () => {
  this.#toWrite.remove(node); // still points at the "original" node
  value.reject(new AbortError());
}

During slot migration, a command can move from one node's queue to another's. If abort/timeout fires after the move, the listener operates on the old node — which has zero effect on a command that already moved. The caller's promise still rejects, so the app believes the request was canceled, but the actual command stays sitting in the new destination queue and can still get sent to Redis. It's the worst kind of state: you believe you canceled it, but it actually runs.

Tracing this pattern with git log -S went all the way back to the V5 rewrite commit. I didn't introduce it — it had been there from the start. Before my fix, though, extractAllCommands() stopped at the first item, so only the first command could ever hit this problem; once I fixed it to move the entire queue, every command started taking this path.

I didn't fold this into the current PR's scope; I filed it as a new issue (#3365) instead. Since it was an independent defect in prependCommandsToWrite() itself, I could branch straight off upstream/master, and fixed it in #3367.

The fix creates a new node on the destination queue, then immediately tears down the old listener and swaps in one that captures the new node. But during self-review I realized rebinding alone wasn't enough. Once an AbortSignal has fired, it never calls any listener registered after that. I confirmed this by running it myself.

const controller = new AbortController();
controller.abort();
controller.signal.addEventListener('abort', () => console.log('FIRED'), { once: true });
// "FIRED" never prints

So if the signal had already fired before the command moved, attaching a new listener does nothing. It had to reject right there and get removed from the destination queue too. So I added a signal.aborted branch to the rebinding logic myself — reject immediately if it already fired, otherwise re-register the listener on the new node. (Full code in PR #3367.)

Maintainer nkaradzhov's review surfaced one more case: when an extracted command can't even find a destination client to move to — there's simply nowhere to rebind, so the promise never settles.

"extraction now detaches listeners unconditionally, so a command that is extracted but never prepended [...] 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."

I addressed it by adding a new rejectCommands() that cleans up the listeners and explicitly rejects when there's no destination. This same function gets reused in bug 3 — because the same failure pattern shows up again.

---

Bug 3 — Transactions have no slotNumber, so routing breaks

The second comment (filed as #3366) was the trickiest one. MULTI/pipeline commands were being queued without a slotNumber in the first place.

this._self.#queue.addCommand(args, { chainId, typeMapping }); // no slotNumber

During a partial migration, where a node only loses some of its slots, extractCommandsForSlots() picks which commands to move based on slotNumber. Batch commands without a slotNumber never matched this filter, so they just stayed on the original node — even after that node no longer owned the slot at all. On full node loss, the fallback sent everything to whatever destination was processed last, which could be a completely unrelated node. git log -S confirmed this was another pre-existing bug going back to the V5 rewrite.

Maintainer nkaradzhov also flagged this exact spot in the same review.

"The cluster MULTI and pipeline executors drop the slot number, so batches are skipped by extractCommandsForSlots and forwarded to whichever destination was processed last. [...] The fix should tag the whole chain with the routing key slot and only relocate fully queued chains."

There were two requirements: ① tag the whole chain with a slot, ② only relocate chains that are fully queued. And while working on it, I caught a third one myself — ③ an ordering reversal.

① Tagging the whole chain with a slot

Add a slotNumber parameter to the MULTI/pipeline execution functions, and pass the same value to every command in the chain.

async _executeMulti(commands, selectedDB, slotNumber) {
  const chainId = Symbol('MULTI Chain');
  const promises = [this.#queue.addCommand(['MULTI'], { chainId, slotNumber })];

  for (const { args } of commands) {
    promises.push(this.#queue.addCommand(args, { chainId, slotNumber }));
  }

  promises.push(this.#queue.addCommand(['EXEC'], { chainId, slotNumber }));
  // ...
}

Because slotNumber is a single function parameter, there's no way in the code's structure for commands in the same chain to end up with different slots — this lines up exactly with Redis Cluster's premise that one transaction lives in one slot.

Destination relocation is implemented with groupCommandsByDestination(), which buckets commands by slot owner. Here's the core of it.

const destination =
  command.slotNumber === undefined
    ? fallback
    : (slots[command.slotNumber]?.master ?? fallback);

if (!destination) {
  unrouted.push(command); // collect commands with no destination instead of dropping them
  continue;
}

I caught another bug here during self-review. The first version didn't put commands with no matching destination into unrouted — it just dropped them with a bare continue. With no destination node, a command gets neither prepended nor rejected — it just silently vanishes. The exact failure #3367 was built to prevent — a command that gets extracted but never settles anywhere — had reappeared in a different shape in this new grouping code. I recognized it immediately, not because it was the same code, but because it was the same failure pattern. I fixed it by explicitly returning unrouted and having the caller pass it to rejectCommands() (built in bug 2).

② Only relocating fully queued chains

This was the hard part. If a migration notification arrives while MULTI has already gone out over the socket and is sitting in the reply-wait queue, with only SET/EXEC still in the write queue, relocating just that remaining tail by slot would split the transaction across two different nodes.

The Redis server manages the state between MULTI and EXEC as session state scoped to a single connection. Once a server gets MULTI, it stops executing subsequent commands on that connection and just queues them, then runs them all at once when EXEC arrives on the same connection. There's no cluster-wide transaction coordination protocol. So if MULTI and EXEC end up on different nodes: the node that got MULTI waits forever for an EXEC that will never come, and the node that got EXEC rejects it outright since it never saw a MULTI. Instead of a transaction, you get two meaningless commands.

So I handled it in opposite ways depending on whether the connection survives.

• Full node loss (connection gets destroyed): reject the remaining tail of the in-flight chain. A guaranteed failure beats risking a half-relocated transaction.
• Partial migration (connection survives): leave the tail queued right where it is instead of moving it. Once MULTI's reply comes back, it keeps sending, and the same-connection guarantee holds.

The core of this bug was that the same migration event demands opposite safe responses depending on whether the connection lives or dies.

③ Where to stop the scan — an ordering reversal

Adding ② surfaced a new problem. Now that MULTI/EXEC had a slotNumber from ①, they became matches for extractCommandsForSlots() — but after skipping the chain tail, the scan kept going and pulled out unrelated commands queued behind it too. I reproduced it with nothing but a queue.

queue.addCommand(['MULTI'], { chainId, slotNumber: 1 });
queue.addCommand(['SET', 'k', 'v'], { chainId, slotNumber: 1 });
queue.addCommand(['EXEC'], { chainId, slotNumber: 1 });
queue.addCommand(['GET', 'k'], { slotNumber: 1 }); // unrelated command queued after the chain

writer.next(); // only MULTI gets sent over the socket
queue.extractCommandsForSlots(new Set([1]));
// => ['SET', 'EXEC', 'GET'] all get extracted (bug)
// GET can run on the new node before the transaction finishes

The fix was to stop the scan entirely the moment it hits a chain tail (break instead of continue), so anything queued after it stays on the original connection and order is preserved.

Submission and maintainer review

Even after finishing the local implementation, I didn't submit this PR right away. The logic depends on extractAllCommands() walking the queue to completion, so it could only be verified properly once #3364 landed on master first. I waited for the merge, rebased onto upstream/master, and submitted it as #3377.

(Before starting, I checked whether another contributor's PR (#3368) was already open on the issue. The direction — passing slotNumber down the execution path — was right, but it was mixed with unrelated changes, and the core requirement, "only relocate fully queued chains," was missing. I requested a review, and after a day of no response, went ahead separately.)

nkaradzhov requested changes, asking for two 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 [...]
• Please add an assertion that on full node loss, the already-sent head of an in-flight chain (in waitingForReply) is also rejected [...]

The implementation already handled both cases, but there was no test proving it explicitly. I added the tests and replied, got approval, and after passing all 15 CI checks (TypeScript analysis, lint, tests across five Redis version combinations, CodeQL, and more), it merged into master.

---

Summary

All three bugs sat on the same execution path.

Bug	How it was found	Issue filed	Fix PR
1. Queue traversal stops at the first item	External report	#3363 (someone else)	#3364
2. Listener references the old node after a command moves	Review bot → traced with git log -S	#3365 (filed by me)	#3367
3. Transaction routing breaks with no slotNumber	Review bot + maintainer → traced	#3366 (filed by me)	#3377

Once the first fix normalized queue traversal, the other two became reachable for the first time — which is exactly why automated review and maintainer review could catch them. Had I stopped at #3364, these two would have stayed undiscovered, left in a state primed to fail even more often.

Five things came out of this that I expect to outlast the code itself.

1. A fix that widens what code actually executes needs a second look beyond the diff itself. These defects weren't in #3364's diff — they were on a path that only became reachable once that diff actually ran. Diff size and blast radius are two different things.
2. "Relocate" isn't a single policy for migration. Whether to reject or leave an in-flight chain in place flips depending on whether the connection lives or dies. The same event demands opposite safe responses depending on context.
3. How long a bug has existed decides a PR's scope. Tracing with git log -S showed both were pre-existing, and that determination decided whether to cram them into the current PR or split them into separate issues. Had they been new, fixing them in the same PR would have been the right call.
4. Fixing tail-relocation logic means re-checking the ordering guarantees in front of it too. Just like fixing bug 3 created a new ordering reversal (③), any logic dealing with relative order in a queue needs a separate check for whether fixing one case broke the ordering of an adjacent one.
5. Naming a failure class once you've fixed it once makes it recognizable the next time. The failure #3367 stopped — "a command that gets extracted but never settles anywhere" — reappeared in a different shape in #3377's new code. I recognized it during self-review immediately, because it was the same pattern, not the same code.

Other things fixed around the same time

Outside of slot migration, I fixed a few other things on the Redis client side around this time. In node-redis, I fixed a crash when running commands before the cluster connection is ready (#3321) and a hang when Sentinel fails to connect to the master (#3331). On ioredis, a third-party client, I fixed a process crash from recursive calls while parsing deeply nested RESP responses (#2138) and a rounding bug in RESP3 double decoding (#2150).

---

If you're running node-redis in cluster mode, all three of these slot-migration fixes are in master now.

I started out unable to explain exactly what the client does when a slot moves. Now I can — down to which data structure holds the commands, what assumptions it runs on, and exactly where those assumptions break. A library I use every day at work turns out to get maintained this way too, in practice — bugs surface, get fixed, and it keeps running. This time, I confirmed that with code, myself.