Disconnect the redis client the metrics consumer opens - #2850
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 3 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.6 #2850 +/- ##
===================================================
+ Coverage 76.77% 76.99% +0.21%
===================================================
Files 205 205
Lines 14288 14293 +5
===================================================
+ Hits 10970 11005 +35
+ Misses 3308 3278 -30
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
delthas
approved these changes
Sep 14, 2026
benzekrimaha
approved these changes
Sep 15, 2026
MetricsConsumer built its RedisClient as a local and handed it straight to the StatsModel, so nothing could reach it afterwards: close() only tore down the kafka consumer, and the socket stayed open for the life of the process. Keep the client as a member and disconnect it alongside the consumer. Both populators already close their metrics consumer, so there is nothing to wire up on their side. close() also dereferenced the consumer unconditionally, while start() only assigns it on the ready event: closing during startup threw a TypeError and left the caller's series waiting on a callback that never came. Guard it, and still call back. Issue: BB-882
francoisferrand
changed the base branch from
development/9.6
to
improvement/BB-804
September 15, 2026 16:36
francoisferrand
changed the base branch from
improvement/BB-804
to
development/9.6
September 15, 2026 17:18
francoisferrand
force-pushed
the
bugfix/BB-882-metricsconsumer-redis-clea
branch
from
September 15, 2026 17:19
4ac8887 to
f1e21b8
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-882. 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.
MetricsConsumerbuilt itsRedisClientas a constructor local and handed it straight to theStatsModel, which keeps it as its private_redis. Nothing in backbeat could reach it afterwards, andclose()only tore down the kafka consumer — so the socket stayed open until the process died. One lingering connection per populator process.Keeping the client as a member is enough to fix it:
close()now disconnects it alongside the consumer, on both the success and error paths. Both owners already call_mConsumer.close()from their own close paths (QueuePopulator,IngestionPopulator), so there was nothing to wire up on their side.While in there:
close()dereferencedthis._consumerunconditionally, butstart()only assigns it on the consumer'sreadyevent. Closing during startup threw aTypeErrorand left the caller'sasync.serieswaiting on a callback that never came. Guarded, and it still calls back.Adds a unit test for
MetricsConsumer, which had none.Scope note: this is the same class of leak as BB-320, but a distinct component and an independent branch — no overlap with the files that PR touches. It does not make
mocha --exitremovable on its own; that needs BB-320 and at least thegetRedisClient.jsleaks too, tracked as BB-321.Issue: BB-882