Answer every caller when close() is called more than once - #2836
Draft
delthas wants to merge 1 commit into
Draft
Conversation
This was referenced Sep 1, 2026
The services install their SIGTERM handlers with process.on rather than once, so a repeated signal calls close() again. The second call started its own drain wait, overwriting the single drain slot the first was waiting on, and the first caller was then only released by its own timeout, minutes after the consumer had already left the group. Coalesce instead: the first call runs the shutdown, later ones attach to it, and every caller is answered once it completes. Issue: BB-833
delthas
force-pushed
the
improvement/BB-833/coalesce-close-callers
branch
from
September 1, 2026 09:15
573f054 to
46d1b26
Compare
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 @@
## improvement/BB-833/answer-rebalance-callbacks #2836 +/- ##
=================================================================================
- Coverage 75.80% 75.68% -0.13%
=================================================================================
Files 200 200
Lines 13983 13990 +7
=================================================================================
- Hits 10600 10588 -12
- Misses 3373 3392 +19
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
delthas
marked this pull request as ready for review
September 1, 2026 09:35
delthas
requested review from
a team,
DarkIsDude,
SylvainSenechal and
francoisferrand
and removed request for
SylvainSenechal
September 1, 2026 09:35
Contributor
Author
|
Requested @francoisferrand in place of Sylvain Senechal, who is currently on PTO. |
francoisferrand
left a comment
francoisferrand
left a comment
Contributor
There was a problem hiding this comment.
Is the issue really the callbacks, or really just that the second call to close() should immediately abort (i.e. not trigger further drain/...) ?
i.e. find with that approach, but I wonder if just switch to once() (or a guard in the event handler) would not be simpler and more appropriate?
delthas
marked this pull request as draft
September 1, 2026 14:57
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
2 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.
The services install their SIGTERM handlers with
process.onrather thanonce, so a repeated signal callsclose()again. The second call started its own drain wait, overwriting the single drain slot the first was waiting on.sequenceDiagram participant A as first caller participant C as BackbeatConsumer participant B as second caller A->>C: close() Note right of C: drain starts, first caller<br/>waits on the single drain slot B->>C: close() — SIGTERM repeated Note right of C: second wait overwrites the slot C-->>B: answered when the drain completes Note over A: never answered — released only by<br/>its own timeout, minutes after the<br/>consumer had already left the groupChanges
Coalesce instead: the first call runs the shutdown, later ones attach to it, and every caller is answered once it completes.
Verification
One unit test: closing twice answers both callers exactly once. It was checked against the previous implementation to confirm it fails there.
End-to-end measurement of the whole stack is in #2819.
Issue: BB-833