Skip to content

Address remaining gap in pending acks cleanup for BacklogQuotaManager #25613

Description

@lhotari

PR #25592 causes a regression in pending acks cleanup which is essential for at least Key_Shared subscriptions. There's a need to cover the gap in pending acks cleanup for BacklogQuotaManager.


Assisted analysis of BacklogQuotaManager:

That said, while verifying this I noticed a related gap on the BacklogQuotaManager side that I think the PR does miss. The new Dispatcher#markDeletePositionMoveForward hook is wired in three places:

Source Location Triggering API
Subscription clear-backlog PersistentSubscription#clearBacklog cursor.asyncClearBacklog(...)
Subscription skip-messages PersistentSubscription#skipMessages cursor.asyncSkipEntries(...)
Expiry monitor PersistentMessageExpiryMonitor#findEntryComplete cursor.asyncMarkDelete(...)

But BacklogQuotaManager advances durable subscription cursors directly through the ManagedCursor API, bypassing the PersistentSubscription wrappers:

BQM path Advancement call Hook fires?
dropBacklogForSizeLimit slowestConsumer.skipEntries(messagesToSkip, IndividualDeletedEntries.Include) — directly on ManagedCursor, bypassing PersistentSubscription#skipMessages ❌ No
dropBacklogForTimeLimit precise=true subscription.getExpiryMonitor().expireMessages(target) ✅ Yes (via expiry monitor)
dropBacklogForTimeLimit precise=false slowestConsumer.markDelete(nextPosition) — directly on ManagedCursor, no subscription-level callback ❌ No

So two of the three quota-eviction paths advance the durable subscription mark-delete without firing the new cleanup hook. Pre-PR these were silently cleaned up by the per-dispatch loop in readMoreEntries that this PR removes, so it's a regression introduced here. Triggers under BacklogQuota.RetentionPolicy.consumer_backlog_eviction. Side-effects when it does trigger:

  • Consumer.pendingAcks accumulates entries for already-mark-deleted positions until the consumer disconnects.
  • Consumer.unackedMessages counter drifts (entries the consumer "still owes" but the broker has already discarded).
  • redeliveryMessages may retain stale entries.

Two ways to close it:

  1. Have BacklogQuotaManager route through PersistentSubscription#skipMessages (and an analogous subscription-level "markDelete forward" method) so the existing callbacks fire, or
  2. Have BacklogQuotaManager invoke dispatcher.markDeletePositionMoveForward() directly after each eviction step.

Option 1 is cleaner architecturally; option 2 is a smaller diff. Either is fine with me — happy to take this in a follow-up if you'd prefer to keep this PR scoped to its current changes.

Originally posted by @lhotari in #25592 (comment)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions