fix(messagequeue): release leases on drained partitions - #533
Open
behinddwalls wants to merge 1 commit into
Open
fix(messagequeue): release leases on drained partitions#533behinddwalls wants to merge 1 commit into
behinddwalls wants to merge 1 commit into
Conversation
behinddwalls
force-pushed
the
preetam/mq-heartbeat-gc
branch
from
August 6, 2026 16:58
c26d89e to
4b4888f
Compare
behinddwalls
force-pushed
the
preetam/mq-idle-lease-release
branch
from
August 6, 2026 16:58
412313c to
ac048a1
Compare
behinddwalls
marked this pull request as ready for review
August 6, 2026 16:59
## Summary ### Why? A lease is held forever once acquired: `ReleaseLease` is only called on shutdown and fair-share shedding, renewal is unconditional, and workers are reconciled from the lease set — so when a partition drains (every message acked and garbage-collected, hence absent from discovery), its holder keeps a goroutine polling an empty partition at ~4-5 queries per poll tick, permanently, plus a lease row and a `queue_offsets` row that nothing ever deletes. On topics with bounded partition keys (queue names) this is harmless stickiness; on topics with unbounded short-lived keys it is a leak proportional to cumulative traffic: buildsignal partitions per batch ID, and the gateway log and cancel topics partition per request ID — every request ever processed would leave a ghost worker behind. The crash variant is worse: a stale lease on a drained partition is never even stealable, because acquisition only probes discovered partitions. ### What? - Idle-lease release: the discovery tick tracks, per owned partition absent from discovery, when it was first observed drained (a partition with any in-flight, postponed, or unacked message still has stored rows and is always discovered — only fully-consumed partitions qualify). After a grace of 2x `LeaseDurationMs` (60s at defaults) the subscriber deletes its consumer group's offsets row (while still holding the lease, so no concurrent initialization is possible), releases the lease, and reconciliation stops the worker. If a message arrives later, the partition reappears in discovery and is reacquired like any new partition — `Initialize` recreates the offsets row, and since drained meant zero stored rows there is nothing to replay. - Stale-lease purge: the lease tick deletes lease rows not renewed within 10x `LeaseDurationMs`, covering holders that crashed while owning a drained partition. Deleting a stale row is equivalent to expiry — a concurrent renewal refreshes the row and the age predicate skips it. - Rebalance integration tests pin `Retry.MaxAttempts` high: they publish messages they never ack, and dead-lettering mid-test would now drain the partitions and dissolve the lease distribution their assertions wait on. ## Test Plan - ✅ New integration test `TestIdleLeaseRelease` covers the full lifecycle against real MySQL: consume, wait out GC + grace, assert the lease and offsets rows are gone, then republish to the same partition key and assert delivery resumes through normal discovery.
behinddwalls
force-pushed
the
preetam/mq-idle-lease-release
branch
from
August 6, 2026 20:40
ac048a1 to
80888ad
Compare
roychying
reviewed
Aug 7, 2026
| ) | ||
| continue | ||
| } | ||
| if err := s.leaseStore.ReleaseLease(ctx, sub.topic, pk, cfg.SubscriberName, cfg.ConsumerGroup); err != nil { |
Contributor
There was a problem hiding this comment.
nit: shall we stop the worker immediately after each release here? currently it happens in reconcilePartitionWorkers after the whole batch processed.
I saw rebalance does kill the worker after each release, with an explicit comment about preventing duplicate processing.
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.
Summary
Why?
A lease is held forever once acquired:
ReleaseLeaseis only called on shutdown and fair-share shedding, renewal is unconditional, and workers are reconciled from the lease set — so when a partition drains (every message acked and garbage-collected, hence absent from discovery), its holder keeps a goroutine polling an empty partition at ~4-5 queries per poll tick, permanently, plus a lease row and aqueue_offsetsrow that nothing ever deletes. On topics with bounded partition keys (queue names) this is harmless stickiness; on topics with unbounded short-lived keys it is a leak proportional to cumulative traffic: buildsignal partitions per batch ID, and the gateway log and cancel topics partition per request ID — every request ever processed would leave a ghost worker behind. The crash variant is worse: a stale lease on a drained partition is never even stealable, because acquisition only probes discovered partitions.What?
LeaseDurationMs(60s at defaults) the subscriber deletes its consumer group's offsets row (while still holding the lease, so no concurrent initialization is possible), releases the lease, and reconciliation stops the worker. If a message arrives later, the partition reappears in discovery and is reacquired like any new partition —Initializerecreates the offsets row, and since drained meant zero stored rows there is nothing to replay.LeaseDurationMs, covering holders that crashed while owning a drained partition. Deleting a stale row is equivalent to expiry — a concurrent renewal refreshes the row and the age predicate skips it.Retry.MaxAttemptshigh: they publish messages they never ack, and dead-lettering mid-test would now drain the partitions and dissolve the lease distribution their assertions wait on.Test Plan
TestIdleLeaseReleasecovers the full lifecycle against real MySQL: consume, wait out GC + grace, assert the lease and offsets rows are gone, then republish to the same partition key and assert delivery resumes through normal discovery.