fix(client): emit termination event when reconnecting stops - #3433
fix(client): emit termination event when reconnecting stops#3433GiHoon1123 wants to merge 7 commits into
Conversation
![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: b2705d2bb9
ℹ️ 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".
| }); | ||
| ``` | ||
|
|
||
| An `'error'` event fires on every disconnect, including ones the client is about to retry, so it can't tell you whether reconnection is still in progress. Once `reconnectStrategy` gives up (returns `false` or an `Error`), the client also emits a `'terminated'` event with the reason — that's the signal that reconnection has permanently stopped and the client needs to be recreated: |
There was a problem hiding this comment.
terminated to the exhaustive event tables
The canonical event references in README.md:384-397 and packages/redis/README.md:298-310 both omit terminated and explicitly state that the client emits no events beyond those listed. Introducing this public event here makes the published documentation contradictory and hides the new terminal-reconnection signal from users consulting the event API; update both tables with its semantics and Error argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated the event tables in both README files with the terminated event and its Error argument. I also updated the reconnect strategy example to call destroy() before replacing the client, so the existing cleanup path is used for client-level resources.
| this.#maybeScheduleWrite(); | ||
| }) | ||
| .on('reconnecting', () => this.emit('reconnecting')) | ||
| .on('terminated', cause => this.emit('terminated', cause)) |
There was a problem hiding this comment.
When a previously connected client has an active streaming-credentials subscription or was registered with the initialized OpenTelemetry ClientRegistry, this callback only forwards the terminal event and leaves those client-level resources alive. Their cleanup occurs only in RedisClient.destroy() (index.ts:2236-2238), so an application that follows the new documentation by dropping this client and creating a replacement without explicitly destroying the old one leaves a registry handle that strongly retains the client and may leave its credential observer subscribed. Dispose these resources on the terminal path or explicitly require destroy() before replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I kept cleanup explicit rather than doing it in the event forwarding callback. terminated reports that reconnection has ended, while destroy() remains the client lifecycle operation that releases credentials and metrics resources. The documentation now calls destroy() before replacing the client, and the example uses it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b9516352c
ℹ️ 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.emit('error', cause); | ||
| this.emit('terminated', cause); |
There was a problem hiding this comment.
terminated before its guaranteed error
When callers await this new event with Node's standard events.once(client, 'terminated'), the promise always rejects on the preceding error instead of resolving with the termination cause; the added test explicitly acknowledges and works around this behavior. This makes the terminal signal unusable with the Promise-based EventEmitter API in every give-up path, even when the client has a separate error listener. Emit terminated before the companion error (in both strategy branches) so awaiting it can succeed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87a1fc7299
ℹ️ 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".
| internal: false, | ||
| clientId: this.#clientId | ||
| })); | ||
| this.emit('terminated', cause); |
There was a problem hiding this comment.
terminated before the first post-ready error
When an already-ready connection dies, #onSocketError still emits error before invoking #shouldReconnect, so events.once(client, 'terminated') rejects before this event is reached whenever the strategy gives up. Fresh evidence in the final diff is the unchanged this.emit('error', err) at line 416 preceding the call to this changed method at line 425; determine the terminal decision before that first error emission so the documented ordering also holds for post-ready disconnects.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36a1037980
ℹ️ 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".
| ```javascript | ||
| client.on('terminated', cause => { | ||
| console.error('client will not reconnect:', cause); | ||
| client.destroy(); |
There was a problem hiding this comment.
When a live connection terminates with commands pending, this synchronous destroy() runs from the terminated listener before the socket emits its companion error. RedisClient.destroy() therefore flushes every queued command with DisconnectsClientError, and the subsequent socket error handler has nothing left to reject with the actual connection failure. Users following this example lose the meaningful failure cause for all in-flight and offline-queued commands; defer the cleanup to a microtask or otherwise ensure the companion error handler flushes the queue first.
Useful? React with 👍 / 👎.
| this.emit('reconnecting'); | ||
| this.#connect().catch(() => { |
There was a problem hiding this comment.
When a ready connection fails with a numeric retry strategy and an error listener synchronously calls client.destroy() or client.close(), that listener sets the socket's #isOpen to false, but execution then reaches these lines unconditionally. Because #connect() performs its first attempt before checking the loop condition, the destroyed client can establish a new socket and emit connect/ready after end, leaving a live connection behind; recheck #isOpen after the synchronous error emission before announcing or starting reconnection.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: a04d4f81d4
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| const retryIn = this.#shouldReconnect(0, err); |
There was a problem hiding this comment.
When an established connection fails and the custom reconnectStrategy throws or returns an invalid value, this call enters #createReconnectStrategy's catch block, which synchronously emits the strategy exception before the actual socket error is emitted below. The RedisClient error handler therefore flushes pending commands with the strategy exception, whereas previously the waiting commands were rejected with the real connection failure before the strategy ran. Determine the retry decision without letting that intermediate error consume the command queue, or preserve the original socket-error emission ordering for this fallback path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit a04d4f8. Configure here.
|
Thanks for the review. The terminal path now emits |
|
Thanks for the PR and for working through all the ordering feedback! 🙏 One small ask: could you briefly document the
Right now that reasoning only lives in the review threads, and the ordering can look confusing without it. |
|
Documented the ordering in both README event tables and expanded the client configuration note to explain the |
2 participants
Fixes #2948
When reconnecting stops because
reconnectStrategyreturnsfalseor anError, the client currently emitserrorbut provides no separate signal that it will not retry again.This adds a
terminatedevent toRedisSocket, forwards it throughRedisClient, and documents when it is emitted. Tests cover initial connection failure, terminal reconnect failures, and the retrying case.Note
Medium Risk
Touches core connection/reconnect lifecycle and error emission ordering; behavior change for apps that assumed only
erroron fatal disconnect, but additive event with documented ordering.Overview
Adds a
terminatedevent whenreconnectStrategystops retrying (falseor anError), including after a connection was alreadyready—cases that previously only surfaced aserror, same as transient disconnects (#2948).RedisSocketcentralizes that path in#shouldReconnect: emitterminated(with cause, oftenReconnectStrategyError) before the companionerror, and refactor reconnect/error handling so retries still emiterrorwithoutterminated.RedisClientre-emitsterminatedfrom the socket.README and
client-configuration.mddocument the event and a pattern (await events.once(client, 'terminated'), thendestroy()). New socket and client tests cover ordering, post-ready loss, and noterminatedwhile retrying.Reviewed by Cursor Bugbot for commit f74c465. Bugbot is set up for automated code reviews on this repo. Configure here.