fix: remove stale data listener explicitly on socket error

[Chrisp1tv]

Description

Fix a race condition in RedisSocket where buffered data events from a broken socket could be emitted after a reconnect had already started, causing stale responses to be matched to new commands.

This PR tries to provide a fix for the bug reported in:

• On node versions higher than 10.19.0, a socket disconnect can permanently break the internal command_queue state #1593
• possibly TypeError: Cannot read property 'callback' of undefined #1427

I must give some credits to @nicklvsa for the reproduction script I've been using to test the issue and my fixes. Thanks for this 🙏

Root cause (written by Claude):

RedisSocket attaches a persistent data listener to each TCP socket it creates. When a socket error occurs, #onSocketError flushes the command queue and resets the RESP decoder — but the old socket was not destroyed immediately, leaving its data listener alive. Any responses already buffered in the kernel for the old socket would then fire, get parsed by the reset decoder, and be matched against new commands in the queue. This causes silent data corruption: GET key1 could resolve with the value of key2.

This fix:

• Captures and clears this.#socket before emitting error, so the reference is atomically transferred to a local variable before any listener runs
• Calls socket.removeAllListeners('data') before socket.destroy(), ensuring no buffered data events from the old socket reach the decoder after teardown
• Calls socket.destroy() before the reconnect

---

Checklist

• Does npm test pass with this change (including linting)?
• Is the new or changed code fully tested?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

---

Note

Medium Risk
Touches core connection error/reconnect teardown in RedisSocket; behavior change is narrow but affects all error-driven reconnect paths.

Overview
Fixes a reconnect race where buffered data from a failed TCP connection could still reach the RESP decoder and be paired with new commands after the client had already moved on.

On socket error, #onSocketError now captures and clears this.#socket immediately, then removeAllListeners('data') and destroy() on that old socket before any reconnect logic runs, so late kernel-buffered bytes cannot be forwarded through RedisSocket's data event.

Adds an integration-style test that emits error then data on the underlying net.Socket and asserts nothing is forwarded to RedisSocket listeners.

^{Reviewed by Cursor Bugbot for commit 4fbc686. Bugbot is set up for automated code reviews on this repo. Configure here.}

[jit-ci]

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

[Chrisp1tv]

Hey, sorry I didn't have time to introduce a reliable test for this yet! That would be great if a maintainer could add a test or give some guidance. Thanks 🙏

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

[nkaradzhov]

@Chrisp1tv thanks for this PR, i will take a look

[nkaradzhov]

@Chrisp1tv, thanks for the careful writeup — the race is real. Traced in current master: on a socket error, RedisSocket.#onSocketError emits 'error', the client's error handler flushes and calls resetDecoder(), but the old net socket is still referenced and keeps its data listener, so any buffered bytes delivered afterward reach the freshly-reset decoder and can be matched to a new command. Removing the data listener and destroying the old socket inside #onSocketError is the right fix at the right layer, and the this.#socket?.destroy() guard in the #connect catch is a correct companion, since #socket can now become undefined mid-initiator.

Two things needed before merge:

1. Please rebase onto current master. #onSocketError has since gained publish(CHANNELS.ERROR, …) before the emit('error') and a wasReady-guarded publish(CHANNELS.CONNECTION_CLOSED, …) after it. Re-integrate the socket-capture/null/remove-listeners/destroy sequence without dropping those publishes, keeping ordering intact (capture + null → publish ERROR → emit → remove data listeners + destroy → CONNECTION_CLOSED → reconnect decision).

2. Add a regression test for the interleaving. A fake socket that emits a data chunk after the error event, with an assertion that the reset decoder receives no stale bytes and a subsequent command's reply is not corrupted, would lock this in. Happy to point you at the fake-socket helpers in the client test-utils if useful.

Once rebased with that test, this is good to merge.

[Chrisp1tv]

Hi @nkaradzhov! Sorry for the delay, didn't see the notification.

I handled your review, re-adding the fix and implementing a regression test. :) Thanks for the clear instructions 🙏

[nkaradzhov]

Thanks @Chrisp1tv, this is looking good!