fix: keep subscriber mode while other channel kinds remain subscribed by askalf · Pull Request #2196 · redis/ioredis

askalf · 2026-09-14T02:18:32Z

Small, isolated bug fix with a focused test, submitted under the carve-out in .github/CONTRIBUTING.md ("Small, isolated bug fixes where the cause and solution are clear and a focused test demonstrates the fix"). Happy to open an issue first instead if you'd prefer that route.

Summary

Redis counts shard channels separately from channels and patterns, so the count carried by an (S)UNSUBSCRIBE reply describes only channels of the same kind.
lib/DataHandler.ts treated that count as global and cleared condition.subscriber whenever it reached 0, in both the RESP3 push path and the RESP2 reply path.
Consequence: SUNSUBSCRIBE of the last shard channel silently discards still-live channel and pattern subscriptions (and UNSUBSCRIBE of the last channel discards shard ones). Messages on the surviving subscriptions stop being delivered: silently under RESP3, and as a Command queue state error under RESP2.
Fix uses SubscriptionSet#isEmpty(), which already existed in lib/SubscriptionSet.ts with no callers; the intended API was never wired up. 13 insertions, 5 deletions.
Regression test test/unit/subscriberMode.ts covers RESP2 and RESP3 with the repo's MockServer, so it needs no Docker or live Redis: 6 of 8 fail on base, 8 of 8 pass with the fix.

$ # --- base (main @ ee48f4c), regression test only ---
$ TS_NODE_TRANSPILE_ONLY=true NODE_ENV=test npx mocha --no-experimental-strip-types "test/unit/subscriberMode.ts"

  subscriber mode across channel kinds (RESP3)
    1) keeps the regular subscription after sunsubscribe
    2) delivers messages on a channel still subscribed after sunsubscribe
    3) keeps the shard subscription after unsubscribe
    ✔ leaves subscriber mode once every kind is unsubscribed

  subscriber mode across channel kinds (RESP2)
    4) keeps the regular subscription after sunsubscribe
    5) delivers messages on a channel still subscribed after sunsubscribe
    6) keeps the shard subscription after unsubscribe
    ✔ leaves subscriber mode once every kind is unsubscribed

  2 passing (8s)
  6 failing

  5) subscriber mode across channel kinds (RESP2)
       delivers messages on a channel still subscribed after sunsubscribe:
     Uncaught Error: Command queue state error. If you can reproduce this, please report it. Last reply: message,regular,hi
      at DataHandler.shiftCommand (lib/DataHandler.ts:381:21)

$ # --- with the fix applied ---
$ TS_NODE_TRANSPILE_ONLY=true NODE_ENV=test npx mocha --no-experimental-strip-types "test/unit/subscriberMode.ts"
  8 passing (176ms)

$ TS_NODE_TRANSPILE_ONLY=true NODE_ENV=test npx mocha --no-experimental-strip-types "test/unit/DataHandler.ts" "test/unit/resubscribe.ts" "test/unit/subscriberMode.ts"
  31 passing (4s)

$ npm run build          # clean
$ npx eslint --ext .js,.ts ./lib
✖ 23 problems (0 errors, 23 warnings)   # all 23 pre-exist on base

Decisions

Ask the SubscriptionSet instead of trusting the per-kind count. The del() calls immediately above already maintain the set per kind, so isEmpty() is accurate at that point, and it is the helper that was written for exactly this. count is still read and still passed to fillUnsubCommand, so what unsubscribe() / sunsubscribe() return to the caller is unchanged; only the internal mode bookkeeping moves.
Both protocol paths. RESP2 has the same defect with a louder symptom; fixing one would leave a protocol-dependent bug.
Not a cluster-layer change. The corrupted state is the per-connection condition.subscriber owned by DataHandler; standalone clients hit it too, as both suites show.
Prettier: lib/DataHandler.ts has a pre-existing --check warning at the protocol !== 3 && handleSubscriberReply(reply) condition from Add RESP3 #2127, nowhere near these hunks; left alone to keep this PR to one bug. The new test file passes --check cleanly.

Not run here: npm run test:js / test:cluster (need npm run docker:setup); the tests above drive test/helpers/mock_server.ts, a real TCP server speaking RESP2 and RESP3, so both paths execute for real.

AI assistance: the bug was traced and the fix and tests were drafted with AI tooling in my workflow; the tests, build and lint were executed as pasted above. I'm responsible for the change and will handle review feedback.

Note

Medium Risk
Changes core pub/sub connection state on every unsubscribe path for RESP2 and RESP3; incorrect logic could still desync the command queue or drop message delivery.

Overview
Fixes subscriber mode being cleared too early when (S)UNSUBSCRIBE replies report a per-kind remaining count of zero while regular, pattern, or shard subscriptions on the same connection are still active.

DataHandler now calls leaveSubscriberModeIfDone, which clears condition.subscriber only when the tracked SubscriptionSet is empty and the reply count is not still positive—on both RESP2 reply routing and RESP3 push handling. Unsubscribe handling also treats empty channel names as valid (only null/undefined means “no channel”) and passes channel values through as ChannelName instead of forcing .toString().

SubscriptionSet stores channels in Maps keyed by byte-accurate latin1 keys so binary-safe names (and edge cases like __proto__) are tracked correctly; isEmpty() uses map sizes for O(1) checks.

Adds test/unit/subscriberMode.ts for mixed subscribe/ssubscribe flows on RESP2/RESP3 plus SubscriptionSet unit cases.

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

Redis counts shard channels separately from channels and patterns, so the count in an (S)UNSUBSCRIBE reply describes only channels of the same kind. DataHandler cleared `condition.subscriber` whenever that count reached zero, discarding subscriptions of the other kinds that were still active: SUNSUBSCRIBE of the last shard channel dropped live channel and pattern subscriptions, and UNSUBSCRIBE of the last channel dropped shard ones. Messages on the surviving subscriptions were then no longer delivered — silently under RESP3, and as "Command queue state error" under RESP2. Use SubscriptionSet#isEmpty() so the state is dropped only once every kind is empty.

cursor

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

^{Reviewed by Cursor Bugbot for commit 66265b3. Configure here.}

askalf · 2026-09-14T02:37:52Z

Good catch on the empty-string channel: if (channel) skips del() for "", so isEmpty() never turns true and the connection would stay in subscriber mode where the old count === 0 check left it. I'll switch both paths to a !== null check and add a test that subscribes and unsubscribes an empty channel name.

`""` is a legal channel name, but the `(S|P)UNSUBSCRIBE` reply handlers guarded `del()` with `if (channel)`, so a reply naming the empty channel never removed it. `SubscriptionSet#isEmpty()` then stayed false and `condition.subscriber` was never cleared, leaving the connection stuck in subscriber mode after its last channel was unsubscribed. The earlier `Number(count) === 0` check did not have this gap. Test both guards for a missing reply element instead of truthiness, and cover subscribe/unsubscribe of an empty channel name for both regular and shard channels under RESP2 and RESP3.

askalf · 2026-09-14T03:09:00Z

Addressed in 7efed45. Both handlers now test for a missing reply element instead of truthiness: const channel = reply[1] == null ? null : reply[1].toString() and if (channel !== null), in returnPush (RESP3) and handleSubscriberReply (RESP2). reply[1] == null covers both a short reply array and a RESP null, which is what the old guard was there for, and "" is a legal channel name so it now goes through del() like any other.

Four regression tests added (two per protocol): unsubscribing an empty regular channel and sunsubscribing an empty shard channel each leave subscriber mode. On 66265b3 all four fail (expected SubscriptionSet{…} to equal false); with the fix:

$ TS_NODE_TRANSPILE_ONLY=true NODE_ENV=test npx mocha --no-experimental-strip-types "test/unit/subscriberMode.ts"
  12 passing

askalf · 2026-09-14T11:01:34Z

The one red cell, test (22.x, 8.8.0), stopped before mocha ran: docker compose up reported container test-cluster-1 is unhealthy (the cluster image's healthcheck), so no test executed there. The other 19 cells, including 20.x/24.x/26.x against 8.8.0, are green on the same head. A re-run of that job should clear it; happy to push if you'd rather have a fresh run.

PavelPashov · 2026-09-14T11:07:11Z

+    it("leaves subscriber mode once every kind is unsubscribed", async () => {
+      redis = new Redis({ port, protocol });
+      await redis.subscribe("regular");
+      await redis.ssubscribe("shard");
+      await redis.unsubscribe("regular");
+      await redis.sunsubscribe("shard");
+
+      expect(redis.condition.subscriber).to.equal(false);
+      expect(redis.mode).to.equal("normal");
+    });
+
+    // `""` is a legal channel name, and the reply naming it must still be
+    // removed from the subscription set — a truthiness check on the channel
+    // skips the removal and leaves the connection stuck in subscriber mode.
+    it("leaves subscriber mode after unsubscribing an empty channel name", async () => {
+      redis = new Redis({ port, protocol });
+      await redis.subscribe("");
+      await redis.unsubscribe("");
+
+      expect(redis.condition.subscriber).to.equal(false);
+      expect(redis.mode).to.equal("normal");
+    });
+
+    it("leaves subscriber mode after sunsubscribing an empty shard channel name", async () => {
+      redis = new Redis({ port, protocol });
+      await redis.ssubscribe("");
+      await redis.sunsubscribe("");
+
+      expect(redis.condition.subscriber).to.equal(false);
+      expect(redis.mode).to.equal("normal");
+    });


Could we make the redis.mode assertions RESP2-only and rename these tests to describe subscription-state cleanup, keeping the redis.condition.subscriber checks for both protocols? Under RESP3, redis.mode is "normal" even while subscribed, so those assertions pass even if cleanup is broken.

Done in 21fb05d: the redis.mode assertions run under RESP2 only, the tests are named for the subscription-state cleanup they check, and the redis.condition.subscriber checks stay for both protocols.

`Redis#mode` is derived from `isResp2SubscriberMode`, which is false for protocol 3, so under RESP3 it reads "normal" even while the connection is subscribed. Asserting on it there passes whether or not the subscription state was cleaned up. Assert `condition.subscriber` on both protocols — that is the check which discriminates — and keep the `mode` assertion for RESP2 only. Rename the affected tests to say they cover subscription-state cleanup.

askalf · 2026-09-14T12:04:06Z

Done in 21fb05d, test file only (lib/DataHandler.ts is unchanged from 7efed45).

You're right about the RESP3 arm: isResp2SubscriberMode (lib/utils/index.ts) returns false whenever protocol === 3, and Redis#mode is derived from it, so mode === "normal" held there with or without the cleanup. The three tests now go through one helper that asserts condition.subscriber === false on both protocols and mode === "normal" only when protocol === 2, and they are renamed to "clears the subscription state …".

Both arms now discriminate: with lib/DataHandler.ts reverted to 66265b3 (the empty-channel fix removed), 4 of the 12 fail (2 under RESP3, 2 under RESP2), all at the condition.subscriber assertion; with the fix, 12 pass. prettier --check, tsc --noEmit and eslint are clean on the file.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21fb05d9ab

ℹ️ 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".

The subscription set keys channels by their utf8 rendering, so channel names that are distinct as bytes but decode alike (e.g. the invalid sequences <80> and <81>, which both render as U+FFFD) collapse onto one key. Unsubscribing either one empties the set while the server still reports a same-kind subscription, and clearing the state on the set alone then drops RESP3 messages and misroutes RESP2 ones to the command queue. Require both the set to be empty and the reply's count to be zero before leaving subscriber mode: the count rules out a same-kind subscription the set collapsed, and the set rules out the channel kinds the count ignores.

…el-kinds

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 73708adce8

ℹ️ 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".

A channel named `__proto__` was never stored as an own key of the plain object backing the subscription set, so `channels()` did not list it and `isEmpty()` reported the set empty while the subscription was live. An (S)UNSUBSCRIBE of another kind then cleared the subscriber state and the connection dropped messages under RESP3 and misrouted them under RESP2.

chatgpt-codex-connector

💡 Codex Review

ioredis/lib/SubscriptionSet.ts

Lines 39 to 42 in 81a8859

    
           return ( 
        
             this.channels("subscribe").length === 0 && 
        
             this.channels("psubscribe").length === 0 && 
        
             this.channels("ssubscribe").length === 0

Make subscription emptiness checks constant-time

When a client removes many subscriptions at once, Redis sends one acknowledgement per channel and DataHandler now calls isEmpty() for every acknowledgement. Because each call materializes all remaining keys with Object.keys, unsubscribing N channels performs Θ(N²) work; with 10,000 subscriptions this repeatedly enumerates roughly 50 million entries and can block the Node.js event loop for seconds even though Redis processes the command quickly. Track the set sizes incrementally so each acknowledgement can test emptiness in constant time.

ℹ️ 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".

The subscription set keyed channels by their utf8 rendering, so names that are distinct as bytes but decode alike (e.g. the invalid sequences <80> and <81>, which both render as U+FFFD) shared one entry. Unsubscribing either one dropped both, leaving the set empty while the server still held a live subscription. The reply count only covers its own kind, so it hides this for the acknowledgement that carries it but not for a later zero count from another kind: SUBSCRIBE <80>, SUBSCRIBE <81>, UNSUBSCRIBE <80>, SSUBSCRIBE, SUNSUBSCRIBE leaves subscriber mode with <81> still subscribed. Its later messages are then dropped under RESP3 and misrouted to the command queue under RESP2. Key the set by latin1, which round-trips every byte, and keep the utf8 rendering as the value so channels() is unchanged for its callers. Storing the channels in a Map also makes isEmpty() a size check. It runs once per unsubscribe acknowledgement and the server sends one per channel, so materializing the keys made unsubscribing N channels O(N^2) work and could stall the event loop for large subscription sets.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e503279ed3

ℹ️ 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".

askalf · 2026-09-14T19:25:18Z

Two more pushes since my last note, both answering the Codex findings:

81a8859: SubscriptionSet backed each kind with a plain {}, so a channel named __proto__ never became an own key and isEmpty() read true while the subscription was live. Reproduced both predicted symptoms as failing tests (RESP3 message dropped, RESP2 misrouted to the command queue), then moved the store to a null-prototype object.
e503279: two findings, one store change. Channels are now keyed by their bytes (Buffer.toString("latin1"), injective, never U+FFFD) in a Map whose value is the UTF-8 rendering, so channels() returns exactly what it did before for the resubscribe and cluster consumers (test/functional/resubscribe unchanged, 8 passing). Two distinct binary names no longer collapse to one entry, so the cross-kind case (unsubscribe one of two colliding names, then the last shard channel with a zero count) keeps subscriber mode. isEmpty() is .size, which also removes the O(N²) I had introduced: 10,000 channels, 5967 ms → 20 ms measured on the class. The five DataHandler call sites now pass reply[1] as raw bytes instead of .toString(). The Map keeps the __proto__ guarantee from 81a8859 by construction.

Each case has a failing test on the previous head and passes on this one; tsc --noEmit, eslint and prettier are clean on the touched files.

A server or proxy that answers SUBSCRIBE with a simple string leaves `reply[1]` as a byte of that string rather than a channel name. Keying channels by their bytes made `SubscriptionSet` reject it outright, and the throw escaped the decoder's reply dispatch as an uncaught error. Accept the value and key it by its string form, as the previous `toString()` did, so only the key derivation changed and not which replies the set tolerates.

askalf · 2026-09-18T00:47:52Z

Everything from the review and the bot findings is addressed at 6cce37e, and CI is 22/22 on that head, including the test (22.x, 8.8.0) cell that had stopped on an unhealthy cluster container earlier.

Summary of what changed since the review: the RESP2-only mode assertions you asked for (21fb05d), the empty-channel guard (7efed45), and the subscription set moved to a Map keyed by the channel's bytes so distinct binary names and __proto__ no longer collapse, with isEmpty() in O(1) (81a8859, e503279). channels() returns exactly what it did before, so resubscribe and cluster consumers are untouched. Each case has a failing test on the previous head.

Ready for another look whenever you have time. Happy to split anything out if the scope has grown past what you want in one PR.

Regular channels and patterns share one remaining count and shard channels have their own; the comments now say so instead of "same kind". New cases: pattern plus shard in both orders, a channel and a pattern sharing one count, a positive server count with an empty local set, and the fire-and-forget blocks in the callback tests now deliver a rejection to done. The byte-writing helper is inlined.

askalf · 2026-10-05T18:38:24Z

@PavelPashov I cut the tests down to make this easier to review. lib/ is unchanged since 00de788; the diff is now +165/-28.

test/unit/subscriberMode.ts went from 21 cases to 4 (410 lines to 86), and test/helpers/mock_server.ts is back to what main has:

a regular subscription survives sunsubscribe of the last shard channel, which is the bug here and fails on main under both protocols
the subscription state clears once every kind is unsubscribed, with the redis.mode assertion under RESP2 only, as you asked
SubscriptionSet keeps a channel named __proto__, and keeps apart two binary names that render as the same string

Happy to reshape it further if you'd like it another way.

PavelPashov

Looks good, thanks

cursor Bot reviewed Sep 14, 2026

View reviewed changes

Comment thread lib/DataHandler.ts Outdated

askalf mentioned this pull request Sep 14, 2026

fix: keep subscriber mode while other channel kinds remain subscribed sprayberry-code/ioredis#1

Closed

PavelPashov requested changes Sep 14, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed Sep 14, 2026

View reviewed changes

Comment thread lib/DataHandler.ts Outdated

askalf mentioned this pull request Sep 14, 2026

[oss-candidate] fix: handle rejection from the db-restoring select on reconnectOnError sprayberry-code/ioredis#2

Open

Merge branch 'main' into fix/pubsub-keep-subscriber-mode-across-chann…

73708ad

…el-kinds

chatgpt-codex-connector Bot reviewed Sep 14, 2026

View reviewed changes

Comment thread lib/DataHandler.ts

chatgpt-codex-connector Bot reviewed Sep 14, 2026

View reviewed changes

Comment thread lib/DataHandler.ts

sprayberry-secondread mentioned this pull request Sep 14, 2026

[oss-candidate] fix: reject stashed commands when the client never becomes ready again sprayberry-code/ioredis#3

Closed

chatgpt-codex-connector Bot reviewed Sep 14, 2026

View reviewed changes

Comment thread lib/SubscriptionSet.ts Outdated

askalf requested a review from PavelPashov September 29, 2026 00:02

test: cut the subscriber mode tests down to the cases the fix needs

8f949b4

PavelPashov approved these changes Oct 6, 2026

View reviewed changes

PavelPashov merged commit eb97beb into redis:main Oct 6, 2026
22 checks passed

	return (
	this.channels("subscribe").length === 0 &&
	this.channels("psubscribe").length === 0 &&
	this.channels("ssubscribe").length === 0

Conversation

askalf commented Sep 14, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Decisions

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

askalf commented Sep 14, 2026

Uh oh!

askalf commented Sep 14, 2026

Uh oh!

askalf commented Sep 14, 2026

Uh oh!

PavelPashov Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

askalf Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

askalf commented Sep 14, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

askalf commented Sep 14, 2026

Uh oh!

askalf commented Sep 18, 2026

Uh oh!

askalf commented Oct 5, 2026

Uh oh!

PavelPashov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

askalf commented Sep 14, 2026 •

edited by cursor Bot

Loading