Skip to content

[#1715] Prevent KahaDB subscription ack from moving backward - #2806

Open
mattrpav wants to merge 1 commit into
apache:mainfrom
mattrpav:amq-gh-1715-lastack-forward-only
Open

mattrpav wants to merge 1 commit into
apache:mainfrom
mattrpav:amq-gh-1715-lastack-forward-only

Conversation

@mattrpav

@mattrpav mattrpav commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

When multiple consumers share a durable subscription and ack messages out of order, a blind overwrite of LastAck can move the ack position backward. On restart, the recovery cursor replays already-acked messages. Check the update with a forward-only check so LastAck only advances.

Test demonstrates recovery cost when lastAckedSequence regresses due to blind overwrite during out-of-order acks. Two durable subscriptions share a topic; the fast subscription acks 499 of 500 messages in reverse order. Without the forward-only guard in MessageDatabase, lastAck regresses to sequence 1 and recovery scans all 499 entries.

Note: this impacts non-shared subscriptions as well

When multiple consumers share a durable subscription and ack
messages out of order, a blind overwrite of LastAck can move the
ack position backward. On restart, the recovery cursor replays
already-acked messages. Check the update with a forward-only check
so LastAck only advances.

Test demonstrates recovery cost when lastAckedSequence regresses due to
blind overwrite during out-of-order acks. Two durable subscriptions
share a topic; the fast subscription acks 499 of 500 messages in
reverse order. Without the forward-only guard in MessageDatabase,
lastAck regresses to sequence 1 and recovery scans all 499 entries.

@cshannon cshannon left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, this is very unlikely to be an issue now with normal durables but with future shared subs i can see this being more of a problem so might as well protect against it

@cshannon

cshannon commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Something I just thought of is we could do some modernization to the KahaDB index interfaces (like BTreeNode) and add some new methods that exist in Map that are useful, such as computeIfAbsent, computeIfPresent, merge, etc.

In particular, a merge method would be useful in this case. We could encapsulate the new check into a function and conditionally return the new/old value.

I don't think we need to do it now and would be a separate PR but there's probably other places in the code that could be a bit simpler with new methods like that.

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

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants