[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.60% 48.51% -0.10%
+ Complexity 13690 13650 -40
=============================================
Files 1381 1381
Lines 101464 101463 -1
Branches 13187 13187
=============================================
- Hits 49318 49226 -92
- Misses 46158 46226 +68
- Partials 5988 6011 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
LGTM. Code changes look good.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Defensive fix with proper validation and test coverage. LGTM.
Automated review by github-manager-bot
|
Rebased onto the latest develop and verified the review request. The regression test uses get(5, TimeUnit.SECONDS), so a stalled pop future fails deterministically rather than hanging CI. Verified with: mvn -q -pl proxy -am -Dtest=ConsumerProcessorTest -Dsurefire.failIfNoSpecifiedTests=false test |
de992d8 to
34e8a66
Compare
|
Consolidated into #10729; both changes cover ConsumerProcessor POP message handling and diagnostics. |
What is changed
ConsumerProcessorTestcoverage to verify missing-handle messages are dropped and not passed to the POP result filter.Fixes #10784
Verification