[ISSUE #10784] Drop POP messages without receipt handles - #10785
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes handling of malformed POP responses by dropping messages that lack a receipt handle, preventing invalid messages from reaching filtering and clients.
Changes:
- Skip POP messages when the broker response does not include a receipt handle.
- Add a unit test to ensure missing-handle messages are dropped and the POP result filter is not invoked.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/processor/ConsumerProcessor.java | Stops adding messages with missing receipt handles into the returned POP result. |
| proxy/src/test/java/org/apache/rocketmq/proxy/processor/ConsumerProcessorTest.java | Adds coverage verifying missing-handle messages are dropped and not passed to the result filter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PopResult popResult = this.consumerProcessor.popMessage( | ||
| createContext(), | ||
| (ctx, messageQueueView) -> mock(AddressableMessageQueue.class), | ||
| CONSUMER_GROUP, | ||
| TOPIC, | ||
| 60, | ||
| invisibleTime, | ||
| Duration.ofSeconds(3).toMillis(), | ||
| ConsumeInitMode.MAX, | ||
| FilterAPI.build(TOPIC, "*", ExpressionType.TAG), | ||
| false, | ||
| popMessageResultFilter, | ||
| null, | ||
| Duration.ofSeconds(3).toMillis() | ||
| ).get(); |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes a bug in ConsumerProcessor.processRequest() where POP messages without receipt handles were still being added to the result list despite being logged as [BUG]. Now such messages are properly dropped.
Findings
- [Info]
ConsumerProcessor.java:218— RemovingmessageExtList.add(messageExt)from the null-handle branch is the correct fix. The original code logged a bug but still delivered the message, which could cause downstream issues for consumers. - [Info] Test
testProcessRequest_NullReceiptHandle_DropsMessagecorrectly verifies that messages without handles are excluded from the response. - [Info] The fix is minimal and focused — no unnecessary changes.
Suggestions
- None. Clean, targeted bug fix.
LGTM — correct fix for a clear bug where invalid messages were being delivered to consumers.
Automated review by github-manager-bot
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10785 +/- ##
=============================================
- Coverage 48.24% 48.23% -0.01%
+ Complexity 13496 13489 -7
=============================================
Files 1380 1380
Lines 101104 101103 -1
Branches 13107 13107
=============================================
- Hits 48773 48763 -10
- Misses 46350 46358 +8
- Partials 5981 5982 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What is changed
ConsumerProcessorTestcoverage to verify missing-handle messages are dropped and not passed to the POP result filter.Fixes #10784
Verification