fix(sentinel): cap post-connect rediscovery retries - #3388
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: cbd06bd161
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (count > this.#maxCommandRediscovers) { | ||
| throw e; |
There was a problem hiding this comment.
When a periodic scan or Sentinel error starts rediscovery during an outage lasting beyond maxCommandRediscovers, this new throw rejects #reset(), but the background callers (setInterval(this.#reset.bind(this), ...), #handleSentinelFailure(), and #handlePubSubControlChannel()) discard that promise without a rejection handler. On the supported Node.js versions, this can become an unhandled rejection and terminate the process rather than merely rejecting the affected command; background resets need to catch/report the capped failure while command-triggered resets may propagate it.
Useful? React with 👍 / 👎.
| if (count > this.#maxCommandRediscovers) { | ||
| throw e; |
There was a problem hiding this comment.
If close() or destroy() is called while a post-connect rediscovery is executing its final failed attempt, both methods await #connectPromise before clearing timers and clients. This new throw makes that await reject and aborts the rest of teardown, leaving the Sentinel marked open/ready with sockets or timers still retained; shutdown during an outage should finish cleanup even when the in-flight rediscovery has exhausted its cap.
Useful? React with 👍 / 👎.
| if (count > this.#maxCommandRediscovers) { | ||
| throw e; |
There was a problem hiding this comment.
When scanInterval is shorter than a successful observe/transform cycle, each overlapping timer call sets #anotherReset, so #connect() continues and increments count despite the rediscovery succeeding. Because this condition is now active after the initial connection, a later single transient failure can see count > maxCommandRediscovers and reject immediately without the configured number of retries; the counter should advance for failed attempts rather than every loop iteration.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 cbd06bd. Configure here.
nkaradzhov
left a comment
nkaradzhov
left a comment
There was a problem hiding this comment.
Thanks @DebadityaHait, before we can merge, the change needs to handle the side effects of #connect() now being able to reject after the client is ready:
- Unhandled rejections. #reset() is called fire-and-forget in three places — #handlePubSubControlChannel, #handleSentinelFailure, and the scan-interval timer set up in connect(). Previously #connect() never rejected post-ready, so these were safe. With this change, during a full outage these background resets will reject with no handler, which on supported Node versions can terminate the process. Please attach a handler (e.g. surface it via the client error event) to each of these call sites.
- Teardown during rediscovery. close() and destroy() do a bare await this.#connectPromise. If that promise now rejects mid-rediscovery, the await throws and the rest of teardown (clearing #scanTimer, closing the sentinel/master/replica clients) is skipped, leaking timers and sockets. Please wrap those awaits so teardown always runs to completion.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd90ebdb03
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (++count > this.#maxCommandRediscovers) { | ||
| throw e; |
There was a problem hiding this comment.
When the control Sentinel disconnects for longer than maxCommandRediscovers, this cap also terminates the reset launched by #handleSentinelFailure; the new background catch only emits the rejection and does not schedule another attempt. Because the monitoring client uses reconnectStrategy: false and scanInterval defaults to 0, restoring the Sentinels leaves this instance without a control connection, so a later MASTER_CHANGE cannot migrate #pubSubProxy and a subscription-only workload can remain attached to the old master indefinitely. Background monitoring should continue retrying independently even after command waiters are allowed to reject.
Useful? React with 👍 / 👎.
| if (++count > this.#maxCommandRediscovers) { | ||
| throw e; |
There was a problem hiding this comment.
After an initially successful connection, a full outage that exhausts this cap leaves #isReady unchanged: the throw unwinds through #reset(), whose finally only clears #connectPromise, while readiness is reset only by close() or destroy(). Consequently the public isReady getter continues returning true even though no underlying client is ready and commands are rejecting, so health checks or traffic routing based on this documented readiness signal continue treating the failed Sentinel as usable.
Useful? React with 👍 / 👎.
| const backgroundError = once(sentinel, 'error'); | ||
| await Promise.all(frame.getAllSentinelsPort().map(port => frame.stopSentinel(port.toString()))); | ||
|
|
||
| const [err] = await Promise.race([ | ||
| backgroundError, |
There was a problem hiding this comment.
With all Sentinels stopped, observe() emits an error for each individual connection failure before #connect() increments the failure count and throws at the cap, so this once(sentinel, 'error') resolves on the first low-level connection error rather than the rejection emitted by #resetInBackground(). The test can therefore pass without demonstrating that the newly capped background promise is handled; wait for or assert the distinctive terminal None of the sentinels are available error instead.
Useful? React with 👍 / 👎.
|
Thanks @nkaradzhov, addressed in |
2 participants
Fixes #3385.
Apply maxCommandRediscovers to rediscovery after the Sentinel has already connected. This causes the operation waiting on failed rediscovery to reject once the cap is exhausted instead of waiting indefinitely.
Tests:
Note
Medium Risk
Changes Sentinel failover/rediscovery behavior for already-connected clients; mis-tuned caps could cause earlier command failures or background errors, but the fix aligns with documented limits and prevents indefinite hangs.
Overview
Fixes #3385 by enforcing
maxCommandRediscoverson every topology rediscovery in#connect(), not only before the first successful connect. After a later full Sentinel outage, commands and background resets reject or emiterroronce the cap is hit instead of looping forever.Periodic scan, pub/sub control messages, and sentinel client failures now trigger
#resetInBackground(), which runs#reset()without leaving unhandled rejections and surfaces failures viaerror.close()/destroy()no longer block on a failing in-flight#connectPromise(they swallow rejection while waiting).Regression tests cover in-flight commands after total outage with a low cap and background rediscovery with
maxCommandRediscovers: 0.Reviewed by Cursor Bugbot for commit dd90ebd. Bugbot is set up for automated code reviews on this repo. Configure here.