Repository navigation
Conversation
…ket initiates Motivation: since sharded resubscribe sends one SSUBSCRIBE per channel, a client with many sharded subscriptions can queue more than the socket's 64 KiB write buffer during reconnect. RedisSocket#write stops at writableNeedDrain, and the 'drain' handler only schedules writes once the socket is ready. The socket becomes ready only after the initiator's resubscribe replies arrive, so the unwritten commands are never sent and the client never becomes ready (no error, no reconnect). Approach: while the socket initiator is waiting on its promises, the 'drain' handler writes the queue directly instead of going through #scheduleWrite (which is a no-op until ready). The flag is cleared when the initiator settles. Validation: Docker is not available here, so the Docker-backed suite was not run. The new test in index.spec.ts uses a mock TCP server (2000 SSUBSCRIBE channels, then all server-side sockets destroyed). Run as a standalone copy of that test with mocha -r tsx: it timed out without the change and passed with it. npm run build and npm run lint pass. Report: redis#3584 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5-5 (via Claude Code)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db6b6e933b
ℹ️ 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".
| this.#write(); | ||
| return Promise.all(promises); | ||
| return Promise.all(promises).finally(() => { | ||
| this.#initiatorWriting = false; |
There was a problem hiding this comment.
RedisSocket.#initiateWhileSocketAlive deliberately abandons a suspended initiator when its socket dies and starts the retry while the abandoned async credentials/handshake invocation can still settle. Because all invocations share this boolean, an old invocation's finally can run after the replacement has set it to true and clear the replacement's state; if that replacement still has more than one socket buffer of resubscribe commands, the next drain falls through to #maybeScheduleWrite(), which is a no-op before ready, recreating the hang this patch targets. Make the state attempt-scoped (or reference-counted) and only clear the invocation that set it.
Useful? React with 👍 / 👎.
Description
Fixes #3584.
After a reconnect, a client with enough sharded subscriptions (about 64 KiB of
SSUBSCRIBEcommands, one per channel since 6.0.1) never becomes ready.RedisSocket#writestops atwritableNeedDrain. Thedrainhandler then calls#scheduleWrite(), which does nothing while the socket isn't ready. The socket only becomes ready after the resubscribe replies arrive, so the unwritten commands are never sent.The
drainhandler now writes the queue directly while the socket initiator is pending. Behavior is unchanged once the client is ready.Verification: Docker isn't available in my environment, so I couldn't run the Docker-backed suite. The added test uses a mock TCP server (2000 channels, then all server sockets destroyed). I ran it as a standalone copy: it timed out without the change and passed with it. I didn't run the issue's repro against a real Redis.
Checklist
npm testpass with this change (including linting)? (Build and lint pass; full suite needs Docker and wasn't run locally.)Note
Medium Risk
Changes reconnect write scheduling on a critical path; scope is narrow (drain during socket initiator only) but incorrect flushing could affect handshake or large post-reconnect command batches.
Overview
Fixes reconnect hangs for clients with large sharded pub/sub subscriptions (issue #3584). After a drop, the socket initiator pipelines thousands of
SSUBSCRIBEcommands; the first chunk fills the TCP write buffer anddrainused to call#maybeScheduleWrite(), which is a no-op until the socket isready.readyonly fires after those resubscribe replies arrive, so remaining commands were never sent and the client stayed stuck.The
drainhandler now calls#write()directly while#initiatorWritingis set (handshake/resubscribe batch in flight) and the queue still has data to flush; normal post-readybehavior is unchanged.A mock TCP integration test subscribes to 2000 sharded channels, forces reconnect, and asserts every
SSUBSCRIBEis replayed beforeready.Reviewed by Cursor Bugbot for commit db6b6e9. Bugbot is set up for automated code reviews on this repo. Configure here.