Skip to content

Answer every rebalance callback raised during shutdown - #2835

Draft
delthas wants to merge 1 commit into
improvement/BB-833/stop-consuming-on-shutdownfrom
improvement/BB-833/answer-rebalance-callbacks
Draft

Answer every rebalance callback raised during shutdown#2835
delthas wants to merge 1 commit into
improvement/BB-833/stop-consuming-on-shutdownfrom
improvement/BB-833/answer-rebalance-callbacks

Conversation

@delthas

@delthas delthas commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

librdkafka requires every rebalance callback to be answered, and the shutdown path answered none of them. Each case fails differently, and each leaves the client parked in the rebalance, so the disconnect wedges on it and the process can never exit.

flowchart TD
    R["rebalance callback raised<br/>after the shutdown began"] --> Q{"which"}
    Q -->|revoke| U["un-assign — answers it"]
    Q -->|grant| A["decline with an empty assign"]
    A --> F{"disconnect already started?"}
    F -->|"yes — assign is refused"| U2["un-assign — still permitted"]
    F -->|no| D["declined, nothing to revoke later"]
    U --> OK["client leaves the rebalance,<br/>disconnect completes"]
    U2 --> OK
    D --> OK
Loading

Changes

A revoke was left for close() to answer by un-assigning later. That holds for the revoke unsubscribe() itself raises, but one arriving after close() has already un-assigned has nothing left to answer it. Answering here does not cut the drain short: close() still waits for the in-flight work, the partitions are just handed back sooner, and the offsets that drain exists to commit (BB-758) are unaffected.

A grant was accepted, which left the disconnect an assignment to revoke all over again. It is declined instead — but assign() is refused once disconnect() has started, since the binding gates it on isConnected() while permitting unassign() for the whole close, so the decline falls back to unassign(). librdkafka coerces an assign into a full unassign during termination anyway.

Verification

Three unit tests: a grant during shutdown is declined, a grant is still answered when assign() is refused mid-close, and a revoke during shutdown is answered. Each was checked against the previous implementation to confirm it fails there.

This is the defect that decides whether the process exits at all, and it was found by measurement rather than review. Across 8 full runs of the lib suite per arm on CI:

before this PR with it
suite runs where the process exits 1 / 8 8 / 8 (matches 9.5)
disconnect bound fires → process fails to exit 40 / 40 bound no longer fires

The bounded disconnect was returning from close() looking successful while leaving a client whose destructor blocks. Answering the callbacks is what stops it firing, so the bound is a backstop rather than load-bearing.

The pod-level census found the grant case independently: the callback still accepted a partition grant after close() had handed the partitions back, so the disconnect had to revoke them again, wedged, hit its 5 s bound, and returned without a LeaveGroup — the surviving members then waited out session.timeout.ms. That was 6 of 69 iterations; declining took the SIGKILL rate to zero and halved the residual. No message was lost and nothing was committed past unprocessed work across 240 iterations.

End-to-end measurement of the whole stack is in #2819.

Issue: BB-833

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.80%. Comparing base (653f932) to head (44ebdf4).

Additional details and impacted files

Impacted file tree graph

Files with missing lines Coverage Δ
lib/BackbeatConsumer.js 94.29% <100.00%> (-1.21%) ⬇️

... and 1 file with indirect coverage changes

Components Coverage Δ
Bucket Notification 80.27% <ø> (ø)
Core Library 81.97% <100.00%> (-0.11%) ⬇️
Ingestion 70.09% <ø> (ø)
Lifecycle 80.46% <ø> (ø)
Oplog Populator 85.83% <ø> (ø)
Replication 62.01% <ø> (ø)
Bucket Scanner 85.76% <ø> (ø)
@@                                Coverage Diff                                @@
##           improvement/BB-833/stop-consuming-on-shutdown    #2835      +/-   ##
=================================================================================
- Coverage                                          75.84%   75.80%   -0.04%     
=================================================================================
  Files                                                200      200              
  Lines                                              13976    13983       +7     
=================================================================================
  Hits                                               10600    10600              
- Misses                                              3366     3373       +7     
  Partials                                              10       10              
Flag Coverage Δ
api:retry 9.06% <0.00%> (-0.01%) ⬇️
api:routes 8.83% <0.00%> (-0.01%) ⬇️
bucket-scanner 85.76% <ø> (ø)
ft_test:queuepopulator 10.45% <22.22%> (-0.58%) ⬇️
ingestion 12.22% <0.00%> (-0.01%) ⬇️
lib 9.13% <100.00%> (-0.02%) ⬇️
lifecycle 19.43% <44.44%> (-0.01%) ⬇️
notification 1.01% <0.00%> (-0.01%) ⬇️
oplogPopulator 0.13% <0.00%> (-0.01%) ⬇️
replication 18.97% <44.44%> (+<0.01%) ⬆️
unit 55.51% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

librdkafka requires every rebalance callback to be answered, and the
shutdown path answered none of them. Each case fails differently, and
each leaves the client parked in the rebalance, so the disconnect wedges
on it and the process can never exit.

A revoke was left for close() to answer by un-assigning later. That holds
for the revoke unsubscribe() itself raises, but one arriving after
close() has already un-assigned has nothing left to answer it. Answering
here does not cut the drain short: close() still waits for the in-flight
work, the partitions are just handed back sooner.

A grant was accepted, which left the disconnect an assignment to revoke
all over again. It is declined instead -- but assign() is refused once
disconnect() has started, since the binding gates it on isConnected()
while permitting unassign() for the whole close, so the decline falls
back to unassign(). librdkafka coerces an assign into a full unassign
during termination anyway.

Measured across 8 full lib-suite runs per arm on CI: whenever the
disconnect bound fired the process failed to exit, 40 times out of 40.
Answering the callbacks takes the suite from 1 of 8 runs exiting to 8 of
8, matching 9.5 itself, with no message lost and nothing committed past
unprocessed work across 240 iterations.

Issue: BB-833
@delthas
delthas force-pushed the improvement/BB-833/answer-rebalance-callbacks branch from a5595c4 to 44ebdf4 Compare September 1, 2026 09:15
@delthas
delthas marked this pull request as ready for review September 1, 2026 09:35
@delthas
delthas requested review from a team, benzekrimaha and francoisferrand September 1, 2026 09:35

@francoisferrand francoisferrand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this have been created by the first PR, which changes the unsubscribe sequence?
If we are not subscribed, we should not get assigned: so doing it in the shutdown avoids the issue? Or is it just the race condition where we unsubscribe right before processing the assign partitions event?

Comment thread lib/BackbeatConsumer.js
Comment on lines +827 to +832
// assign() is refused once disconnect() has started -- the
// binding gates it on isConnected(), which is already false --
// while unassign() is explicitly permitted while closing, and
// librdkafka coerces an assign into a full unassign during
// termination anyway. Leaving the callback unanswered is what
// parks the client in the rebalance.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on this, shoud we not just always call unassign() in that case, instead of trying to assign([]) first?

Comment thread lib/BackbeatConsumer.js
return;
}

this._setDrain(null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be needed, c.f. first PR in the stack: a new assign cannot happen during an earlier unassign, so setDrain should stay managed in a single place (during partition revoke).

@delthas
delthas marked this pull request as draft September 1, 2026 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants