Skip to content

fix(messagequeue): run queue garbage collection on busy partitions - #622

Open
Jal-Bafana wants to merge 1 commit into
uber:mainfrom
Jal-Bafana:fix/busy-partition-gc
Open

fix(messagequeue): run queue garbage collection on busy partitions#622
Jal-Bafana wants to merge 1 commit into
uber:mainfrom
Jal-Bafana:fix/busy-partition-gc

Conversation

@Jal-Bafana

Copy link
Copy Markdown

Why?

Garbage collection of acknowledged queue_messages rows currently runs only on idle poll ticks.

A continuously busy partition resets the GC counter whenever it delivers a message, so it can never reach the GC threshold. Under sustained traffic, acknowledged message rows can therefore accumulate indefinitely.

This also means the message deduplication horizon can remain unnecessarily large on busy partitions.

What?

Run the existing garbage collection cadence on every poll tick rather than only idle ticks.

The existing 100-tick throttle is preserved, so the per-partition GC frequency is not increased. The GC safety mechanism is also unchanged: garbage collection still uses the minimum acknowledged offset across consumer groups.

Added:

  • unit regression coverage proving GC runs on busy ticks
  • MySQL integration coverage proving acknowledged rows are reclaimed under continuous traffic

Testing

  • go test ./platform/extension/messagequeue/... — PASS
  • go test ./platform/base/... — PASS
  • go vet on the messagequeue and integration packages — PASS
  • gofmt and git diff --check — clean

Not run locally due to environment limitations:

  • MySQL integration test TestGCReclaimsAckedRowsUnderContinuousTraffic — Docker daemon unavailable
  • make gazelle, make fmt, make lint, make check-tidy, make check-gazelle and Bazel tests — make unavailable and Bazel's Windows C++ toolchain is not configured

Copilot AI lite review requested due to automatic review settings August 19, 2026 19:18
@Jal-Bafana
Jal-Bafana requested review from a team, behinddwalls and sbalabanov as code owners August 19, 2026 19:18
@CLAassistant

CLAassistant commented Aug 19, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes MySQL-backed message queue garbage collection (GC) starvation on continuously busy partitions by running the existing GC cadence on every poll tick (still throttled), and adds regression tests to prevent reintroduction.

Changes:

  • Run per-partition GC every N poll ticks regardless of whether messages were delivered.
  • Add a unit test ensuring GC triggers even when every tick delivers a message.
  • Add a MySQL integration test verifying acked rows are reclaimed under sustained traffic.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
test/integration/extension/messagequeue/mysql/queue_test.go Adds an integration regression test validating GC reclaims acked rows during continuous traffic.
platform/extension/messagequeue/mysql/subscriber.go Changes GC scheduling from “idle ticks only” to “every N poll ticks” while preserving throttling.
platform/extension/messagequeue/mysql/subscriber_test.go Adds a unit test proving GC runs on busy ticks (no idle periods).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

partition := "gc-busy-part"
consumerGroup := "gc-busy-cg"

signalCh := make(chan queueMySQL.HookSignal, 100)

@behinddwalls behinddwalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two notes on the diff — the core change looks right to me. Removing the messageCount == 0 gate genuinely fixes the starvation (the old else branch reset the counter to 0 forever on a continuously busy partition), GC load stays bounded the same way it was, and run() only logs pollAndDeliver errors so a GC failure can't kill a worker or cause re-delivery.

Comment on lines +1205 to +1209
w.gcCounter++
if w.gcCounter >= gcTickInterval {
w.gcCounter = 0
if err := w.garbageCollect(ctx); err != nil {
return fmt.Errorf("garbage collect: %w", err)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This early return now suppresses the messages_delivered counter on a busy tick.

Before this change GC could only run when messageCount == 0, so the garbage collect error path could never skip the if messageCount > 0 metrics block a few lines below. Now it can: the messages were already pushed to deliveryCh, but the counter is dropped — so throughput reads low exactly when the store is unhealthy and you most want the number.

Moving the metrics block above the GC block (or folding it into the existing defer) fixes it.

Comment on lines +2312 to +2315
receiveN(t, deliveryChan, initialBatch, func(d extqueue.Delivery, _ int) {
require.NoError(t, d.Ack(s.ctx))
})
require.Equal(t, initialBatch, countMessages())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This assertion assumes GC hasn't fired yet, but the change under test is what can invalidate that — the drain above is busy ticks, which now increment gcCounter.

The subscription is live before the publishes, so the counter accrues across both the publish phase and the drain. At PollIntervalMs = 50 that lands somewhere around 30–80 ticks on a fast box and passes; if the 200 inserts take more than ~3.5s on a loaded CI runner with Dockerized MySQL, the counter crosses the 100-tick threshold, GC reclaims acked rows mid-drain, and this require.Equal fails. Roughly 2x margin against a wall-clock threshold.

Asserting countMessages() > 0 here, or seeding the acked backlog before subscribing, removes the timing dependency without weakening what the test actually proves (the waitForCondition below is the real assertion).

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.

4 participants