Release and reuse the clients held by ClientManager - #2853
Merged
Merged
Conversation
Contributor
Hello francoisferrand,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.6 #2853 +/- ##
===================================================
+ Coverage 76.50% 76.60% +0.09%
===================================================
Files 206 206
Lines 14427 14450 +23
===================================================
+ Hits 11038 11069 +31
+ Misses 3379 3371 -8
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
francoisferrand
force-pushed
the
bugfix/BB-877
branch
from
September 15, 2026 20:58
98e06df to
4378d82
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
francoisferrand
force-pushed
the
bugfix/BB-877
branch
from
September 15, 2026 21:25
4378d82 to
5adc856
Compare
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
delthas
approved these changes
Sep 16, 2026
benzekrimaha
approved these changes
Sep 17, 2026
The interval sweeping inactive credentials was started and never stopped: it kept the event loop alive, so a process could not exit on its own, and it kept the whole ClientManager reachable along with its keepAlive agents and client caches. The lifecycle processors meant to clear it, but did so on themselves rather than on the ClientManager, which never had anything to clear it with. Give the class a close() and call it from the services owning a manager, once their consumers and producers are done with the clients. Issue: BB-877
A queue processor built a new ClientManager for every entry it replicated, and another one each time a retry moved to the next host. Each of them was left behind with its credentials cache, its keepAlive agents and the sweep timer holding it, so a role was assumed again for every single object. Hold them on the queue processor, keyed by destination host and role, the way the copy location tasks already share theirs, and release them on shutdown. They assume their role on the destination STS, so they cannot share the cache the copy location tasks read their sources with. Issue: BB-877
The spec ended up with two adjacent describe('stop') blocks, one left by
the redis subscriber teardown and one by the client manager teardown, so
mocha reported the shutdown tests under two identically titled sections.
Keep a single suite holding all four of them, which is what the output is
expected to show for a class with one stop().
Issue: BB-877
francoisferrand
force-pushed
the
bugfix/BB-877
branch
from
September 17, 2026 13:21
f055adc to
3a2229b
Compare
Contributor
Author
|
/approve |
Contributor
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue BB-877. Goodbye francoisferrand. The following options are set: approve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ClientManagerstarts a 30 min interval to sweep inactive credentials, keeps the handle, and never uses it again. Nothing clears it and the timer is notunref'd, so a process cannot exit on its own. The lifecycle processors look like they handle this, but they clear the property on themselves rather than on the ClientManager, so those calls have always been no-ops.unref()alone would not be enough: a live interval is a GC root, and its closure captures the manager, so the keepAlive agents, the client caches and the CredentialsManager stay reachable for the life of the process. The class gets aclose()instead, called by the services that own a manager once their consumers and producers are done with the clients.That matters more since BB-812, which made managers per endpoint and role rather than one per process. Looking at the other holders,
ReplicateObjectturned out to be the worse case: it built a brand-new manager for every entry it replicated, and another one on each retry that moved to the next host, dropping the previous one every time. Beyond the leak, each entry started from an empty credentials cache, so a role was assumed again for every single object. Those clients are now held on the queue processor and keyed by destination host and role, the way the copy location tasks already share theirs. The two caches stay separate because they assume their roles on different STS endpoints — the remote site's for the sources, the destination's for the targets.Idle managers are not evicted from either cache while the process runs; they drain their own credentials and clients, so what remains is the shell. Bounded by endpoints and roles rather than by object count, and followed up in BB-886.
Issue: BB-877