Conversation
…in appendAck appendAck set PROPERTY_UNIQ_CLIENT_MESSAGE_ID_KEYIDX correctly per ack type (genBatchAckUniqueId for BatchAckMsg, genAckUniqueId otherwise), but a later unconditional put overwrote it with genAckUniqueId for every ack. Batch acks therefore landed on the revive topic with a bogus uniq key: the offset segment was the -1 sentinel the batch path assigns and the tag segment was ACK instead of BATCH_ACK, so every batch ack of a pop produced the same non-unique client id and tracing by uniq key was impossible. The buffered path in PopBufferMergeService already writes the batch uniq key without overwriting it. Drop the stray put so the per-type key set just above survives.
RockteMQ-AI
approved these changes
Sep 11, 2026
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
Contributor
There was a problem hiding this comment.
Summary
Fixes a bug where appendAck was overwriting the batch ack unique key with the single ack key. Removes the erroneous overwrite line. Test uses ArgumentCaptor to verify the correct key is retained.
LGTM — correct fix with proper test verification.
Automated review by github-manager-bot
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
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.
Motivation
appendAcksetPROPERTY_UNIQ_CLIENT_MESSAGE_ID_KEYIDXcorrectly per ack type (genBatchAckUniqueIdforBatchAckMsg,genAckUniqueIdotherwise), but a later unconditionalputoverwrote it withgenAckUniqueIdfor every ack. Batch acks therefore landed on the revive topic with a bogus uniq key: the offset segment was the-1sentinel the batch path assigns, and the tag segment wasackinstead ofbAck, so every batch ack of a pop produced the same non-unique client id and tracing by uniq key was impossible.The buffered path in
PopBufferMergeServicealready writes the batch uniq key without overwriting it.Modifications
Drop the stray
putso the per-type key set just above survives.Verification
Fail-before (new test, run against the unpatched code):
Pass-after: