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: 4ef24b822d
ℹ️ 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".
| if err := c.onClose.run(); err != nil && firstErr == nil { | ||
| firstErr = err | ||
| } |
There was a problem hiding this comment.
When a failover-cluster topology reload overlaps Close, this hook can close the current Sentinel and then have it immediately recreated: clusterStateHolder.LazyReload goroutines are not stopped by ClusterClient.Close, and their ClusterSlots callback calls sentinelFailover.MasterAddr/replicaAddrs, which create a new Sentinel whenever c.sentinel is nil because sentinelFailover has no closed state. The listener itself schedules an initial reload, so closing shortly after first use can hit this race; the reopened Sentinel's PubSub goroutine and connections then outlive the closed client. Shutdown needs to make Sentinel closure terminal or cancel/wait for reloads before running this hook.
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 high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 4ef24b8. Configure here.
| } | ||
| if err := c.onClose.run(); err != nil && firstErr == nil { | ||
| firstErr = err | ||
| } |
There was a problem hiding this comment.
Sentinel can restart after Close
Medium Severity
ClusterClient.Close runs the Sentinel hook after nodes.Close, unlike FailoverClient, so listen can still call ReloadState during teardown. A concurrent LazyReload can then invoke ClusterSlots after failover.Close, and MasterAddr reconnects and starts a new listen goroutine that is never shut down.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4ef24b8. Configure here.
ndyakov
left a comment
ndyakov
left a comment
There was a problem hiding this comment.
Thank you @roganartu, this looks good.
|
@roganartu would you be able to check the bot review, thank you. |
2 participants
Fixes #3999
Note
Low Risk
Lifecycle-only change that mirrors existing FailoverClient close behavior; low risk aside from hook ordering interacting with other cluster onClose registrations.
Overview
Fixes sentinel resource cleanup when closing a failover cluster client (#3999).
ClusterClientnow owns anonClosehook registry (initialized inNewClusterClient) andClose()runs registered hooks after autopipeliners and node pools shut down.NewFailoverClusterClientregistersfailover.Closeon that registry—the same pattern asNewFailoverClient—so closing the cluster client tears down the Sentinel client and failover pub/sub instead of leaving them running.Reviewed by Cursor Bugbot for commit e33a937. Bugbot is set up for automated code reviews on this repo. Configure here.