[ISSUE #10786] Tolerate malformed local POP offset metadata by Aias00 · Pull Request #10787 · apache/rocketmq

Aias00 · 2026-08-03T07:47:21Z

What is changed

Guard local POP receipt-handle reconstruction against missing startOffsetInfo / msgOffsetInfo entries and invalid offset indexes.
Skip only the malformed message entry instead of failing the whole local POP response translation.
Add LocalMessageServiceTest coverage for a POP response with startOffsetInfo present but missing msgOffsetInfo.

Fixes #10786

Notes

This intentionally does not change the existing raw MessageExt warning covered by Avoid logging raw MessageExt bodies in LocalMessageService #10730 / fix: harden local POP scans and diagnostics #10731.

Verification

mvn -pl proxy -Dtest=LocalMessageServiceTest test

Copilot

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.

Improve robustness of local POP response translation by tolerating malformed offset metadata, skipping only invalid message entries, and adding regression coverage for missing msgOffsetInfo.

Changes:

Guard POP receipt-handle reconstruction against missing startOffsetInfo / msgOffsetInfo and invalid offset indexes.
Skip only malformed messages and return remaining valid messages.
Add a unit test for POP responses with startOffsetInfo present but missing msgOffsetInfo.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
proxy/src/main/java/org/apache/rocketmq/proxy/service/message/LocalMessageService.java	Adds defensive checks for offset metadata and filters invalid messages instead of failing translation.
proxy/src/test/java/org/apache/rocketmq/proxy/service/message/LocalMessageServiceTest.java	Adds regression test ensuring messages are skipped when `msgOffsetInfo` is missing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

                    messageExt.getProperties().computeIfAbsent(MessageConst.PROPERTY_FIRST_POP_TIME, k -> String.valueOf(responseHeader.getPopTime()));
                    messageExt.setBrokerName(messageQueue.getBrokerName());
                    messageExt.setTopic(messageQueue.getTopic());
+                    validMessageExtList.add(messageExt);
                }
+                popResult.setMsgFoundList(validMessageExtList);


+                            List<Long> sortQueueOffsets = sortMap.get(key);
+                            List<Long> msgQueueOffsets = msgOffsetInfo == null ? null : msgOffsetInfo.get(key);
+                            Long startOffset = startOffsetInfo.get(key);


+                            if (sortQueueOffsets == null || msgQueueOffsets == null || startOffset == null) {
+                                log.warn("Pop response offset metadata is missing, key:{}", key);
+                                continue;
+                            }
+                            int index = sortQueueOffsets.indexOf(messageExt.getQueueOffset());
+                            if (index < 0 || index >= msgQueueOffsets.size()) {
+                                log.warn("Pop response offset metadata index is invalid, key:{}, index:{}, msgOffsetCount:{}",
+                                    key, index, msgQueueOffsets.size());
+                                continue;
+                            }


+    @Test
+    public void testPopMessageShouldSkipMessageWithMissingOffsetMetadata() throws Exception {


RockteMQ-AI

Review by github-manager-bot

Summary

Adds null-safe checks for POP offset metadata (sortQueueOffsets, msgQueueOffsets, startOffset) in LocalMessageService. Messages with missing/malformed metadata are now skipped with a warning instead of causing NPE.

Findings

[Info] LocalMessageService.java:432-435 — The three-way null check (sortQueueOffsets, msgQueueOffsets, startOffset) correctly guards against all NPE paths in the original code.
[Info] The validMessageExtList pattern is clean — only messages passing all validation are included in the result.
[Info] Test testPopLocalMessage_MalformedOffsetMetadata_SkipsMessage properly verifies the skip behavior.

Suggestions

Minor: the msgQueueOffsets == null ? null : msgQueueOffsets.get(key) pattern could be simplified with Optional or a helper, but this is a style preference and not blocking.

LGTM — solid defensive fix against NPE in POP message processing.

Automated review by github-manager-bot

codecov-commenter · 2026-08-03T08:39:53Z

Codecov Report

❌ Patch coverage is 83.33333% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.51%. Comparing base (293f588) to head (7078de5).

Files with missing lines	Patch %	Lines
...tmq/proxy/service/message/LocalMessageService.java	83.33%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10787      +/-   ##
=============================================
- Coverage      48.60%   48.51%   -0.10%     
+ Complexity     13690    13658      -32     
=============================================
  Files           1381     1381              
  Lines         101464   101490      +26     
  Branches       13187    13195       +8     
=============================================
- Hits           49318    49239      -79     
- Misses         46158    46232      +74     
- Partials        5988     6019      +31

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

RockteMQ-AI

Summary

This PR looks mostly good with some suggestions for improvement.

Findings Overview

1 warning(s) and suggestions for improvement

Please review the inline comments.

Automated review by github-manager-bot

RockteMQ-AI · 2026-08-05T11:35:26Z

                    sortMap.get(key).add(messageExt.getQueueOffset());
                }
                Map<String, String> map = new HashMap<>(5);
+                List<MessageExt> validMessageExtList = new ArrayList<>(messageExtList.size());


Raw type usage detected. Use parameterized types for type safety.

RockteMQ-AI

Summary

Improved tolerance for malformed POP offset metadata. The second commit significantly improves the implementation:

Batch logging — counts skipped messages and logs once per category instead of per-message, reducing log noise under load.
NO_NEW_MSG status — correctly sets the status when all messages are filtered out, preventing unnecessary polling retries.
Test refactoring — mockPopMessageResponse() helper eliminates duplication across test cases.
Additional test coverage — new tests for missing start offset and invalid offset index scenarios.

The raw type concern from the previous review appears to be about pre-existing code, not the new changes. All new code uses proper parameterized types. LGTM.

Automated review by github-manager-bot

RockteMQ-AI

Summary

Defensive fix with proper validation and test coverage. LGTM.

Automated review by github-manager-bot

Aias00 · 2026-08-14T13:33:42Z

Rebased onto the latest develop and verified the review fixes already included in the branch: malformed POP metadata is summarized once per response, valid-message filtering preserves the status/list invariant, and targeted tests cover missing start-offset and invalid offset-index cases.

Verified with:

mvn -q -pl proxy -am -Dtest=LocalMessageServiceTest -Dsurefire.failIfNoSpecifiedTests=false test

Aias00 · 2026-08-15T07:33:48Z

Consolidated into #10731; both changes harden LocalMessageService POP scans and diagnostics.

Copilot AI review requested due to automatic review settings August 3, 2026 07:47

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot started reviewing on behalf of Aias00 August 3, 2026 08:10 View session

RockteMQ-AI approved these changes Aug 3, 2026

View reviewed changes

RockteMQ-AI reviewed Aug 5, 2026

View reviewed changes

RockteMQ-AI approved these changes Aug 8, 2026

View reviewed changes

RockteMQ-AI approved these changes Aug 11, 2026

View reviewed changes

Aias00 added 2 commits August 14, 2026 06:32

[ISSUE apache#10786] Tolerate malformed local POP offset metadata

ef7899a

fix(proxy): preserve local pop response invariants

7078de5

Aias00 force-pushed the fix/proxy-local-pop-offset-metadata branch from 099d812 to 7078de5 Compare August 14, 2026 13:33

Aias00 closed this Aug 15, 2026

Navigation Menu

Uh oh!

[ISSUE #10786] Tolerate malformed local POP offset metadata - #10787

[ISSUE #10786] Tolerate malformed local POP offset metadata#10787
Aias00 wants to merge 2 commits into
apache:developfrom
Aias00:fix/proxy-local-pop-offset-metadata

Aias00 commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

RockteMQ-AI left a comment

Uh oh!

codecov-commenter commented Aug 3, 2026 •

edited

Loading

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI Aug 5, 2026

Uh oh!

RockteMQ-AI left a comment

Uh oh!

RockteMQ-AI left a comment

Uh oh!

Aias00 commented Aug 14, 2026

Uh oh!

Aias00 commented Aug 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@Test
		public void testPopMessageShouldSkipMessageWithMissingOffsetMetadata() throws Exception {

Uh oh!

Conversation

Aias00 commented Aug 3, 2026

What is changed

Notes

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Review by github-manager-bot

Summary

Findings

Suggestions

Uh oh!

codecov-commenter commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Findings Overview

Uh oh!

RockteMQ-AI Aug 5, 2026

Choose a reason for hiding this comment

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Aias00 commented Aug 14, 2026

Uh oh!

Aias00 commented Aug 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Aug 3, 2026 •

edited

Loading