Repository navigation
fix(client): reject in-flight connect attempt when socket dies during initiator - #3374
Conversation
… initiator If the socket errors or closes while the connection initiator is suspended on async work (DNS resolution in the maintenance handshake, an async credentials provider), the handshake commands it enqueues afterwards are never written to the dead socket and never flushed. The reconnect loop then stays suspended inside the initiator forever: isOpen stays true, isReady stays false, no further lifecycle events are emitted, reconnectStrategy is never consulted again, and connect() rejects with "Socket already opened". Race the initiator against socket error/close so a socket death during initiation always rejects the in-flight attempt and resumes the retry loop. Fixes redis#3346 Co-Authored-By: Claude Fable 5 <[email protected]>
![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: 85df5409f7
ℹ️ 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".
| // flushed on a subsequent failure) — the raced error already drove the retry | ||
| initiated.catch(() => {}); | ||
|
|
||
| return Promise.race([initiated, socketDied]).finally(() => { |
There was a problem hiding this comment.
When socketDied wins this race, the original initiator is left running while #connect immediately opens the next socket. In the real RedisClient initiator, #getHandshakeCommands() awaits DNS/credentials and then mutates the shared queue/subscription state and calls #write(), so a slow first attempt can later enqueue/write stale HELLO/AUTH/maintenance commands onto the replacement socket; with streaming credentials it can also create an extra subscription whose disposable is overwritten or leaked. This affects exactly the async DNS/credentials cases this change targets, so the stale initiator needs to be cancelled or fenced from mutating shared client state after its socket has died.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not a desync — the stranded handshake commands are enqueued, so their replies still match. The reconnect hang stays fixed, so this is not a #3346 regression. The one real residue is a streaming-credentials subscription leak on mid-init failover, which I will split into a follow-up to keep this PR scoped.
| socket.once('close', onSocketDied); | ||
| }); | ||
|
|
||
| const initiated = Promise.resolve(this.#initiator()); |
There was a problem hiding this comment.
If a RedisSocketInitiator throws before returning a promise, this expression throws before the Promise.race(...).finally() cleanup is installed, leaving the new error/close listeners attached. The catch in #connect then destroys the socket, the close listener rejects socketDied with no consumer, and Node can surface it as an unhandled rejection/process error. This affects initiators using the allowed synchronous void | Promise form; invoke the initiator through a promise or wrap it so cleanup is always registered.
Useful? React with 👍 / 👎.
| reject(err instanceof Error ? err : new SocketClosedUnexpectedlyError()); | ||
| }; | ||
| socket.once('error', onSocketDied); | ||
| socket.once('close', onSocketDied); |
There was a problem hiding this comment.
This new close listener treats every close during initiation as an unexpected reconnectable failure. If the caller destroys the client while an async initiator is still pending, destroySocket() clears #socket, this rejection resumes the inner catch, and this.#socket.destroy() throws a TypeError; with the default retry strategy the connect path can then emit error/reconnecting after an intentional destroy and resolve only after a retry delay. This only occurs when shutdown races an in-flight async handshake, so guard application-initiated closes or the socket identity before rejecting here.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Two edge cases in `#initiateWhileSocketAlive`, both raised in review: - A synchronous throw from the initiator escaped before the socket-death race handlers were cleaned up, leaking the `error`/`close` listeners; the reject they later fired on `#socket.destroy()` surfaced as an unhandled rejection (process crash on Node 15+). Defer the initiator into a microtask so a synchronous throw becomes a rejection routed through the race. - Destroying the client while the initiator was suspended left `#socket` undefined, so the inner catch threw a `TypeError` and the outer catch still emitted `error`/`reconnecting` and scheduled a retry after an intentional shutdown. Guard the inner destroy with `?.` and rethrow immediately from the outer catch when `\!isOpen`. Adds deterministic regression tests for both. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
a33ef8d to
5b78289
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 5b78289. Configure here.
| return (retries, cause) => { | ||
| try { | ||
| const retryIn = strategy(retries, cause); | ||
| const retryIn = strategy(retries, cause); |
There was a problem hiding this comment.
Accidental indentation removal breaks code readability
Low Severity
The line const retryIn = strategy(retries, cause); has been accidentally stripped of all indentation and sits at column 0, while it logically belongs inside a try block nested within a returned arrow function inside an if block. All surrounding lines (122, 124–137) maintain proper indentation. The project's ESLint config has no indent rule, so this won't be caught automatically.
Reviewed by Cursor Bugbot for commit 5b78289. Configure here.
2 participants
Description
This pull request resolves #3346.
When the socket errors or closes while the connection initiator is suspended on asynchronous work — the DNS lookup performed by the maintenance-notifications handshake when the host is a hostname (the default on RESP3), or an async/streaming credentials provider — the handshake commands the initiator enqueues afterwards are written to an already-dead socket.
RedisSocket.writesilently skips unwritable sockets, and the earlier error had already flushed the queue, so those commands are never sent and never rejected. The reconnect loop then stays suspended inside the initiator forever: the client reportsisOpen: true, isReady: false, emits no further lifecycle events, never consultsreconnectStrategyagain, andconnect()rejects withSocket already opened— a silent, unrecoverable zombie state typically observed as "the pod never reconnects after a Redis failover".The fix races the initiator against the socket's
error/closeevents, so a socket death during initiation always rejects the in-flight attempt and the retry loop continues perreconnectStrategy. A regression test reproduces the interleaving deterministically with a local TCP server and an initiator suspended at the moment the connection drops; without the fix, the test hangs indefinitely.No public API or behavior changes beyond restoring the documented reconnect semantics.
Checklist
npm testpass with this change (including linting)?🤖 Generated with Claude Code
Note
Medium Risk
Touches core connection/reconnect logic in
RedisSocket; behavior change is limited to failure paths during connect but affects all clients on handshake/async initiator races.Overview
Fixes a zombie connect state where the client stayed
isOpenbut neverisReadyif the TCP socket died while the connection initiator was still waiting on async work (DNS, credentials, handshake). The reconnect loop could hang forever with noreconnecting/reconnectStrategyandconnect()failing with “Socket already opened”.RedisSocketnow runs the initiator via#initiateWhileSocketAlive, racing it against the socket’serrorandcloseso a dead socket always fails the attempt and the existing retry path runs. Synchronous initiator throws are deferred through a microtask so listeners are cleaned up and retries work without unhandled rejections. Ifdestroy()races a suspended initiator, the outer catch bails out when#isOpenis false so intentional shutdown does not emiterror/reconnectingor schedule another retry.Adds three local TCP server tests for suspended initiator + dropped connection, sync throw + retry, and destroy-while-suspended behavior.
Reviewed by Cursor Bugbot for commit 5b78289. Bugbot is set up for automated code reviews on this repo. Configure here.