Fix offset lost - #776
Open
mvSapphire wants to merge 1 commit into
Open
Conversation
Author
|
@brmagadutra @joelfoliveira Hi guys, I’d really appreciate it if you could take a look. |
bblankenship78
approved these changes
Aug 25, 2026
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
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.
Description
Messages are silently skipped when the worker pool is restarted by a workers count change (e.g.
WithConsumerLagWorkerBalancer). A partition can permanently lose an offset — offset2is never processed while3,4, … are consumed and committed — and because the commit moves past the gap, a process restart does not recover it either.The root cause is that the
OffsetManagerlifetime is tied to the worker pool instead of to the partition assignment.ConsumerWorkerPool.StopAsyncsets_offsetManager = nullandStartAsyncbuilds a fresh one, but a workers count change does not rewind the librdkafka read position. Everything the discardedOffsetManagerstill had pending is forgotten, and the new instance happily commits over it.Two distinct paths lead to the loss:
1. Pending contexts are dropped with the
OffsetManagerWaitContextsCompletionAsync()is awaited with.WithCancellation(token, false), so it returns silently onceWorkerStopTimeoutelapses. Contexts that were neverMarkAsProcessed(cancelled mid-processing, or not completed whenAutoMessageCompletionis disabled) stay inPartitionOffsets._receivedContextsand are thrown away with the manager. The new manager starts empty, so the next completed message becomes the head of the queue and its offset is committed — writing straight over the gap.2. Messages are dropped before reaching the
OffsetManagerChangeWorkersCountAsyncstops the feeder first, which cancels the token passed toIWorkerDistributionStrategy.GetWorkerAsync.BytesSumDistributionStrategy(the default) andPartitionKeyDistributionStrategyreturnnullon that token, andFreeWorkerDistributionStrategythrowsOperationCanceledExceptionfromChannel.Reader.ReadAsync, which the feeder loop swallows. In both casesConsumerWorkerPool.EnqueueAsyncreturned without enqueuing, so a message that was already read from Kafka is never tracked anywhere. This path does not requireWorkerStopTimeoutto expire at all — withFreeWorkerDistributionStrategythe window is "any moment all workers are busy".Changes
IConsumerWorkerPool.StopAsynctakes akeepOffsetManagerflag.ConsumerWorkerPool.StartAsynconly creates anOffsetManager(and starts theOffsetCommitter) when the pool does not already own one, so the offset bookkeeping now follows the partition assignment rather than the workers.ConsumerWorkerPool.StopAsynckeeps theOffsetManagerand theOffsetCommitteralive when asked to.ConsumerWorkerPool.EnqueueAsyncno longer drops a message that was already read from Kafka. TheOperationCanceledExceptionis handled, the context is registered in theOffsetManagerand then discarded, so it blocks the commit instead of disappearing.ConsumerManager.ChangeWorkersCountAsyncstops the pool withkeepOffsetManager: true. Partition revoke/assign and consumer shutdown keep the previous behaviour, which is correct there because Kafka re-reads from the committed offset after a rebalance.This also removes a related inconsistency: contexts capture the
OffsetManagerthey were created with, so a detached task from the previous pool generation finishing late used to mark offsets on a discarded manager that still shared the liveOffsetCommitter, pushing a stale offset into the commit. With a single manager across the restart, late completions land in the right place.Behaviour change
An unprocessed message no longer disappears — instead the committed offset for that partition freezes at the gap and the message is delivered again the next time the consumer starts from the committed offset (process restart or a rebalance). This is the expected at-least-once behaviour and it is already what a discarded message does within a single
OffsetManagerlifetime; the change only makes it survive a worker pool restart. The visible consequences are that broker-side lag for the affected partition stops draining until then, and messages after the gap are redelivered.Fully removing the freeze would require seeking the partition back to the first unprocessed offset on restart. The internal
IConsumerabstraction does not exposeSeektoday, so that is left out of this PR.Fixes # (issue)
How Has This Been Tested?
New
ConsumerWorkerPoolOffsetTrackingTestsdrives a realConsumerWorkerPoolwith a realOffsetManagerandOffsetCommitter, captures the committed offsets and covers one test per loss path plus a happy path:ChangingWorkersCount_WithMessageNotProcessed_ShouldNotCommitPastIt— fails onmasterwithExpected CommittedOffsets() to contain only items matching (offset <= 1), but {4L} do(es) not match.ChangingWorkersCount_WithMessageNotAssignedToAnyWorker_ShouldNotCommitPastIt— fails onmasterwith{3L} do(es) not match.ChangingWorkersCount_WithAllMessagesProcessed_ShouldCommitTheLastOffset— passes before and after, guarding against over-correcting and stalling commits when nothing is pending.ConsumerManagerTests.ChangingWorkersCount_StopsWorkerPoolKeepingTheOffsetManagercovers the wiring and fails ifChangeWorkersCountAsyncgoes back toStopAsync().The existing
StopAsyncmock setups inConsumerManagerTestsandConsumerManagerCooperativeStickyTestswere made explicit (StopAsync(false)) because Moq expression trees cannot use optional arguments. They keep asserting the previous behaviour for the revoke/assign/shutdown paths.Full unit test suite: 123 passed, 0 failed. Integration tests were not run (they require a live broker).
Checklist