Skip to content

Commit 79b5252

Browse files
authored
fix(messagequeue): release leases on drained partitions (#533)
## 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.
1 parent 596163b commit 79b5252

9 files changed

Lines changed: 471 additions & 10 deletions

File tree

platform/extension/messagequeue/mysql/mock_stores.go

Lines changed: 28 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

platform/extension/messagequeue/mysql/offset_store.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,3 +132,23 @@ func (s *sqloffsetStore) GetMinAckedOffset(ctx context.Context, topic string, pa
132132

133133
return minOffset, true, nil
134134
}
135+
136+
// DeleteOffset removes one consumer group's offset row for a partition.
137+
// Idempotent — see the offsetStore interface doc.
138+
func (s *sqloffsetStore) DeleteOffset(ctx context.Context, topic string, partitionKey string, consumerGroup string) (retErr error) {
139+
op := metrics.Begin(s.scope, "delete_offset", metrics.StorageLatencyBuckets,
140+
metrics.NewTag("topic", topic),
141+
metrics.NewTag("partition_key", partitionKey),
142+
metrics.NewTag("consumer_group", consumerGroup))
143+
defer func() { op.Complete(retErr) }()
144+
145+
_, err := s.db.ExecContext(ctx, fmt.Sprintf(`
146+
DELETE FROM %s WHERE consumer_group = ? AND topic = ? AND partition_key = ?
147+
`, OffsetsTableName), consumerGroup, topic, partitionKey)
148+
149+
if err != nil {
150+
return fmt.Errorf("delete offset topic=%s partition=%s: %w", topic, partitionKey, err)
151+
}
152+
153+
return nil
154+
}

platform/extension/messagequeue/mysql/offset_store_test.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,3 +188,54 @@ func TestOffsetStore_GetMinAckedOffset(t *testing.T) {
188188
})
189189
}
190190
}
191+
192+
func TestOffsetStore_DeleteOffset(t *testing.T) {
193+
tests := []struct {
194+
name string
195+
setup func(mock sqlmock.Sqlmock)
196+
wantErr bool
197+
}{
198+
{
199+
name: "deletes the consumer group's offset row",
200+
setup: func(mock sqlmock.Sqlmock) {
201+
mock.ExpectExec("DELETE FROM queue_offsets").
202+
WithArgs(testConsumerGroup, "test_topic", "part-1").
203+
WillReturnResult(sqlmock.NewResult(0, 1))
204+
},
205+
},
206+
{
207+
name: "idempotent - row already gone",
208+
setup: func(mock sqlmock.Sqlmock) {
209+
mock.ExpectExec("DELETE FROM queue_offsets").
210+
WithArgs(testConsumerGroup, "test_topic", "part-1").
211+
WillReturnResult(sqlmock.NewResult(0, 0))
212+
},
213+
},
214+
{
215+
name: "database error",
216+
setup: func(mock sqlmock.Sqlmock) {
217+
mock.ExpectExec("DELETE FROM queue_offsets").
218+
WithArgs(testConsumerGroup, "test_topic", "part-1").
219+
WillReturnError(fmt.Errorf("db error"))
220+
},
221+
wantErr: true,
222+
},
223+
}
224+
225+
for _, tt := range tests {
226+
t.Run(tt.name, func(t *testing.T) {
227+
db, mock, store := setupoffsetStoreTest(t)
228+
defer db.Close()
229+
230+
tt.setup(mock)
231+
232+
err := store.DeleteOffset(context.Background(), "test_topic", "part-1", testConsumerGroup)
233+
if tt.wantErr {
234+
require.Error(t, err)
235+
} else {
236+
require.NoError(t, err)
237+
}
238+
require.NoError(t, mock.ExpectationsWereMet())
239+
})
240+
}
241+
}

platform/extension/messagequeue/mysql/partition_lease_store.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,36 @@ func (s *sqlpartitionLeaseStore) GetAllLeases(ctx context.Context, topic string,
228228
return leases, nil
229229
}
230230

231+
// PurgeStale deletes lease rows not renewed within olderThanMs. See the
232+
// partitionLeaseStore interface doc.
233+
func (s *sqlpartitionLeaseStore) PurgeStale(ctx context.Context, topic string, consumerGroup string, olderThanMs int64) (retErr error) {
234+
op := metrics.Begin(s.scope, "purge_stale", metrics.StorageLatencyBuckets, metrics.NewTag("topic", topic))
235+
defer func() { op.Complete(retErr) }()
236+
237+
threshold := currentTimeMillis() - olderThanMs
238+
239+
result, err := s.db.ExecContext(ctx, fmt.Sprintf(`
240+
DELETE FROM %s
241+
WHERE consumer_group = ? AND topic = ? AND lease_renewed_at < ?
242+
`, PartitionLeasesTableName), consumerGroup, topic, threshold)
243+
244+
if err != nil {
245+
return fmt.Errorf("failed to purge stale leases: %w", err)
246+
}
247+
248+
// RowsAffected error is swallowed because the DELETE itself succeeded;
249+
// the count is for observability only.
250+
if deleted, err := result.RowsAffected(); err == nil && deleted > 0 {
251+
metrics.NamedCounter(s.scope, "purge_stale", "rows_deleted", deleted, metrics.NewTag("topic", topic))
252+
s.logger.Debugw("purged stale leases",
253+
logTopic, topic,
254+
"deleted", deleted,
255+
)
256+
}
257+
258+
return nil
259+
}
260+
231261
// DiscoverAndAcquirePartitions discovers partitions from messages table and tries to acquire leases.
232262
// Returns the number of new leases acquired and the full list of discovered partitions.
233263
// maxPartitions limits how many total partitions this subscriber can own (0 = unlimited)

platform/extension/messagequeue/mysql/partition_lease_store_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package mysql
1717
import (
1818
"context"
1919
"database/sql"
20+
"fmt"
2021
"testing"
2122
"time"
2223

@@ -412,3 +413,54 @@ func TestPartitionLeaseStore_DiscoverAndAcquirePartitions(t *testing.T) {
412413
})
413414
}
414415
}
416+
417+
func TestPartitionLeaseStore_PurgeStale(t *testing.T) {
418+
tests := []struct {
419+
name string
420+
setup func(mock sqlmock.Sqlmock)
421+
wantErr bool
422+
}{
423+
{
424+
name: "deletes rows older than threshold",
425+
setup: func(mock sqlmock.Sqlmock) {
426+
mock.ExpectExec("DELETE FROM queue_partition_leases").
427+
WithArgs(testConsumerGroup, "test_topic", sqlmock.AnyArg()).
428+
WillReturnResult(sqlmock.NewResult(0, 2))
429+
},
430+
},
431+
{
432+
name: "no stale rows is a no-op",
433+
setup: func(mock sqlmock.Sqlmock) {
434+
mock.ExpectExec("DELETE FROM queue_partition_leases").
435+
WithArgs(testConsumerGroup, "test_topic", sqlmock.AnyArg()).
436+
WillReturnResult(sqlmock.NewResult(0, 0))
437+
},
438+
},
439+
{
440+
name: "database error",
441+
setup: func(mock sqlmock.Sqlmock) {
442+
mock.ExpectExec("DELETE FROM queue_partition_leases").
443+
WithArgs(testConsumerGroup, "test_topic", sqlmock.AnyArg()).
444+
WillReturnError(fmt.Errorf("db error"))
445+
},
446+
wantErr: true,
447+
},
448+
}
449+
450+
for _, tt := range tests {
451+
t.Run(tt.name, func(t *testing.T) {
452+
db, mock, store := setuppartitionLeaseStoreTest(t)
453+
defer db.Close()
454+
455+
tt.setup(mock)
456+
457+
err := store.PurgeStale(context.Background(), "test_topic", testConsumerGroup, 300_000)
458+
if tt.wantErr {
459+
require.Error(t, err)
460+
} else {
461+
require.NoError(t, err)
462+
}
463+
require.NoError(t, mock.ExpectationsWereMet())
464+
})
465+
}
466+
}

platform/extension/messagequeue/mysql/stores.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,12 @@ type offsetStore interface {
101101
// Used by the subscriber to compute the GC threshold without messageStore
102102
// needing to query the offsets table.
103103
GetMinAckedOffset(ctx context.Context, topic string, partitionKey string) (offset int64, found bool, err error)
104+
105+
// DeleteOffset removes one consumer group's offset row for a partition.
106+
// Callers use this when retiring a fully-drained partition; Initialize
107+
// recreates the row if the partition ever receives messages again.
108+
// Idempotent: no-op if the row is already gone.
109+
DeleteOffset(ctx context.Context, topic string, partitionKey string, consumerGroup string) error
104110
}
105111

106112
// leaseInfo describes one partition's current lease row (internal use only)
@@ -137,6 +143,14 @@ type partitionLeaseStore interface {
137143
// instead of write-probing every lease row each discovery tick.
138144
GetAllLeases(ctx context.Context, topic string, consumerGroup string) ([]leaseInfo, error)
139145

146+
// PurgeStale deletes lease rows not renewed within olderThanMs. Backstop
147+
// for holders that crashed while owning a drained partition: acquisition
148+
// only probes discovered partitions, so a stale lease on a partition
149+
// with no messages is otherwise never refreshed or removed. Deleting a
150+
// stale row is equivalent to lease expiry — a concurrent renewal makes
151+
// the row fresh and the age predicate skips it.
152+
PurgeStale(ctx context.Context, topic string, consumerGroup string, olderThanMs int64) error
153+
140154
// DiscoverAndAcquirePartitions discovers partitions from messages table and tries to acquire leases.
141155
// Returns the number of new leases acquired and the full list of discovered partitions.
142156
// leaseDurationMs is how long the lease is valid (in milliseconds)

0 commit comments

Comments
 (0)