fix: close discarded candidate cluster to prevent VTAdmin dynamic cluster resource leak - #20881
fix: close discarded candidate cluster to prevent VTAdmin dynamic cluster resource leak#20881waterWang wants to merge 1 commit into
Conversation
…venting resource leak
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
There was a problem hiding this comment.
Pull request overview
Closes discarded duplicate dynamic-cluster candidates to prevent VTAdmin resource leaks.
Changes:
- Closes rejected candidates and logs cleanup failures.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if err := c.Close(); err != nil { | ||
| log.Error(fmt.Sprintf("candidate cluster %s leaked resources on close: %v", id, err)) | ||
| } |
![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: b7b0c329d9
ℹ️ 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".
| // The discarded candidate is never stored or used; | ||
| // without this Close() each rejected candidate leaves | ||
| // a schema-cache worker running. | ||
| if err := c.Close(); err != nil { |
There was a problem hiding this comment.
If a caller passes the same *cluster.Cluster already stored under id, Equal succeeds and this branch closes that active object; selectedCluster then points to the closed cluster, so subsequent requests use closed caches and proxies. WithCluster does not require a distinct allocation, so only close c when it differs from existingCluster.
AGENTS.md reference: AGENTS.md:L159-L163
Useful? React with 👍 / 👎.
| // The discarded candidate is never stored or used; | ||
| // without this Close() each rejected candidate leaves | ||
| // a schema-cache worker running. | ||
| if err := c.Close(); err != nil { |
There was a problem hiding this comment.
The existing repeated-request case in TestServeHTTP only checks response contents and passes identically without this change, so it does not prove that the discarded cluster's worker and proxies are closed. Add a test with observable closers or worker lifecycle state that fails on the parent commit and verifies exactly one candidate cleanup here.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
| // without this Close() each rejected candidate leaves | ||
| // a schema-cache worker running. | ||
| if err := c.Close(); err != nil { | ||
| log.Error(fmt.Sprintf("candidate cluster %s leaked resources on close: %v", id, err)) |
There was a problem hiding this comment.
This newly added message formats the cluster ID and error into an opaque string, so structured-log consumers cannot filter or aggregate the failure by cluster or error. Emit a fixed message with slog.String("cluster", id) and slog.Any("error", err) instead.
AGENTS.md reference: AGENTS.md:L198-L201
Useful? React with 👍 / 👎.
| // The discarded candidate is never stored or used; | ||
| // without this Close() each rejected candidate leaves | ||
| // a schema-cache worker running. | ||
| if err := c.Close(); err != nil { |
There was a problem hiding this comment.
When a discarded candidate has an in-flight schema-cache fill or one of its proxy closers stalls, c.Close() waits while clusterMu remains held from the start of WithCluster; every concurrent WithCluster and EjectDynamicCluster call then blocks behind this cleanup. Select and detach the candidate while locked, then close it after unlocking, or otherwise bound the shutdown.
AGENTS.md reference: AGENTS.md:L119-L124
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20881 +/- ##
===========================================
- Coverage 69.67% 61.72% -7.96%
===========================================
Files 1614 76 -1538
Lines 216793 7022 -209771
===========================================
- Hits 151044 4334 -146710
+ Misses 65749 2688 -63061
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #20866
Problem
When
WithClusterreceives a candidate cluster that matches an existing cached cluster, the candidate is discarded but never closed. Each rejected candidate leaves a schema-cache worker running and retains its client proxies and RPC pools.In deployments using
--enable-dynamic-clusters, sending N identical requests creates N schema-cache workers; only 1 is stopped when the API closes.Root Cause
In
WithCluster, whenshouldAddClusteris false (candidate equals existing), theelsebranch only logs a message — the candidatecis neverClose()d.Fix
Close the candidate cluster in the
elsebranch to release its schema-cache worker, client proxies, and RPC pools. This mirrors the existing cleanup pattern used when replacing an existing cluster with a new one.Verification