Skip to content

fix: remove stale data listener explicitly on socket error#3209

Open
Chrisp1tv wants to merge 2 commits intoredis:masterfrom
Chrisp1tv:fix-issue
Open

fix: remove stale data listener explicitly on socket error#3209
Chrisp1tv wants to merge 2 commits intoredis:masterfrom
Chrisp1tv:fix-issue

Conversation

@Chrisp1tv
Copy link
Copy Markdown

@Chrisp1tv Chrisp1tv commented Mar 26, 2026

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:

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 socket error/reconnect handling; while small, it can affect connection teardown timing and event ordering during failures/reconnects.

Overview
Fixes a race in RedisSocket teardown on errors by atomically clearing this.#socket before emitting the error event, and then explicitly removing the old socket’s data listeners before destroying it.

Also hardens the initiator error path by using this.#socket?.destroy() to avoid crashing if the socket reference has already been cleared.

Written by Cursor Bugbot for commit c6b6288. This will update automatically on new commits. Configure here.

@jit-ci
Copy link
Copy Markdown

jit-ci bot commented Mar 26, 2026

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 Chrisp1tv marked this pull request as ready for review March 27, 2026 08:05
@Chrisp1tv
Copy link
Copy Markdown
Author

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 🙏

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix All in Cursor

@nkaradzhov
Copy link
Copy Markdown
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants