Repository navigation
fix(pub-sub): guard #subscribing against underflow in resubscribe callbacks - #3476
Merged
Merged
Conversation
…lbacks When flushWaitingForReply/flushAll fires, #pubSub.reset() zeroes #subscribing before rejecting the in-flight commands. The deferred reject callbacks in #normalResubscribe and #shardedResubscribe then drove #subscribing below zero, causing #updateIsActive() to keep #isActive true permanently — even after all channels were unsubscribed. On RESP2 this silently skipped re-authentication when credentials rotated after the connection recovered. Guard both resolve and reject callbacks with if (this.#subscribing > 0) so they become no-ops when reset() has already zeroed the counter.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
flushWaitingForReply/flushAllcalls#pubSub.reset()first (zeroing#subscribingto 0) and then rejects the in-flight commands. The deferred reject callbacks in#normalResubscribeand#shardedResubscribewere() => this.#subscribing--, so they drove#subscribingto-1afterreset()had already set it to0.Because
#updateIsActive()usesthis.#subscribing !== 0as one of its conditions, a counter of-1caused#isActiveto staytruepermanently — even after every channel was fully unsubscribed.Consequences:
isPubSubActivereturnedtruewith no active subscriptions after the client recovered from a rapid double-disconnect.client/index.ts:856(!(isPubSubActive && RESP === 2)) silently skipped re-authentication, leaving the connection unauthenticated.RESETcommand on every returned client.Fix: Guard both
resolveandrejectcallbacks in#normalResubscribeand#shardedResubscribewithif (this.#subscribing > 0)so they become no-ops whenreset()has already zeroed the counter. Adds a regression test that simulates the double-disconnect path end-to-end.Fixes the
#subscribingunderflow introduced by the sharedconst callback = () => this.#subscribing--pattern used in both methods.Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Touches reconnect/resubscribe state that drives
isPubSubActive, with downstream effects on re-auth and pool RESET behavior, but the change is a narrow guard plus regression test.Overview
Fixes a PubSub
#subscribingunderflow when a connection drops twice while resubscribe commands are still in flight.reset()already sets#subscribingto0, but deferred resolve/reject callbacks from#normalResubscribeand#shardedResubscribestill decremented the counter, leaving it at-1and keepingisActivestuck true after all listeners were removed.The change makes those callbacks no-op unless
#subscribing > 0, so#updateIsActive()can clear activity correctly. A new spec walks the double-disconnect → resubscribe → reject → recover → unsubscribe path to lock this in.Reviewed by Cursor Bugbot for commit 208f727. Bugbot is set up for automated code reviews on this repo. Configure here.