[ISSUE #8714] Prevent receive message NPE when client settings are missing

[Aias00]

Which Issue(s) This PR Fixes

Fixes #8714

Brief Description

ReceiveMessageActivity currently assumes GrpcClientSettingsManager#getClientSettings always returns a non-null Settings instance. When receive-message is called before client settings are cached, the proxy dereferences settings.getClientType() / settings.getSubscription() and returns an internal NPE instead of a structured receive response.

This PR treats missing client settings the same as the existing default-settings path used by tests, allowing receive validation and pop handling to continue without throwing a NullPointerException.

How Did You Test This Change?

• mvn -pl proxy -Dtest=ReceiveMessageActivityTest test

Result: BUILD SUCCESS, Tests run: 10, Failures: 0, Errors: 0, Skipped: 0

[Copilot]

Pull request overview

This PR addresses issue #8714 by preventing a NullPointerException in the gRPC proxy’s ReceiveMessageActivity when client Settings are not yet cached, and adds a regression test covering the missing-settings scenario.

Changes:

• Add a null-guard in ReceiveMessageActivity#receiveMessage to handle GrpcClientSettingsManager#getClientSettings returning null.
• Add a unit test verifying receive-message returns a structured response when client settings are missing.

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/grpc/v2/consumer/ReceiveMessageActivity.java	Adds a fallback path when client settings are missing to avoid NPE during receive-message handling.
proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageActivityTest.java	Adds a regression test ensuring missing client settings don’t cause an internal error response.

---

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.17%. Comparing base (8242c1e) to head (8f6a9c9).
⚠️ Report is 4 commits behind head on develop.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10585      +/-   ##
=============================================
- Coverage      48.26%   48.17%   -0.10%     
+ Complexity     13433    13413      -20     
=============================================
  Files           1378     1378              
  Lines         100817   100823       +6     
  Branches       13040    13042       +2     
=============================================
- Hits           48660    48569      -91     
- Misses         46211    46293      +82     
- Partials        5946     5961      +15     

☔ 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]

Review by github-manager-bot

Summary

Adds a null-guard for getClientSettings() in ReceiveMessageActivity.receiveMessage() to prevent NPE when client settings have not yet been cached, returning a structured receive response instead of an internal error.

Findings

• [Info] proxy/.../ReceiveMessageActivity.java:68 — Using Settings.getDefaultInstance() as the fallback is reasonable and consistent with how other activities handle missing settings. The downstream ClientType.LITE_PUSH_CONSUMER.equals(...) will safely return false for the default instance, and getSubscription() on a default Settings returns an empty subscription — so the flow degrades gracefully.
• [Info] proxy/.../ReceiveMessageActivityTest.java — Test coverage is solid: mocks getClientSettings to return null and verifies the response code. Good practice.

Suggestions

• Consider whether sendMessageActivity or other gRPC v2 activities have the same gap. A quick audit of all getClientSettings(ctx) call sites without null checks would be a useful follow-up.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

Summary

New commit e5f7c3a improves the null-settings fallback by replacing Settings.getDefaultInstance() with a proper getDefaultConsumerSettings() that carries a real maxAttempts value, preventing fresh messages from being routed to the DLQ when client settings are not yet cached.

Changes Since Last Review

1. GrpcClientSettingsManager.java — Added getDefaultConsumerSettings() that delegates to createDefaultConsumerSettingsBuilder().build(), ensuring maxAttempts = retryMaxTimes + 1 instead of the protobuf empty default of 0.
2. ReceiveMessageActivity.java:68 — Fallback now calls getDefaultConsumerSettings() instead of Settings.getDefaultInstance().
3. PopMessageResultFilterImplTest.java — New test class with 3 test cases covering the DLQ regression path (fresh message, retry exhaustion, null settings).
4. GrpcClientSettingsManagerTest.java — New test verifying maxAttempts > 0 on the default consumer settings.
5. ReceiveMessageActivityTest.java — New test verifying the fallback path does not DLQ fresh messages.

Findings

• [Positive] The fix correctly addresses the subtle DLQ risk: with protobuf's empty default, backoffPolicy.maxAttempts == 0 causes PopMessageResultFilterImpl to treat reconsumeTimes == 0 as exhausted. The new default prevents this.
• [Positive] Test coverage is thorough — covers both the settings layer and the filter layer, with clear test names explaining the regression scenario.
• [Info] proxy/.../ReceiveMessageActivity.java:68 — The fallback getDefaultConsumerSettings() creates a new Settings object on every call when settings are null. If this path is hot, consider caching the default. However, this is a minor concern since the null-settings case should be rare (only before first client registration).

Verdict

The improvement is well-executed. The previous review's suggestion about auditing other getClientSettings() call sites remains a useful follow-up.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

PR: Prevent receive message NPE when client settings are missing
Verdict: ✅ Approved

Summary

Fixes a NullPointerException in ReceiveMessageActivity.receiveMessage() when client settings have not been cached yet. When grpcClientSettingsManager.getClientSettings(ctx) returns null, the code now falls back to getDefaultConsumerSettings() instead of dereferencing null.

Analysis

Correctness ✅

• The null check is placed at the right location — immediately after getClientSettings() returns.
• The fallback getDefaultConsumerSettings() returns a properly initialized Settings object with real consumer defaults (including maxAttempts derived from retryMaxTimes), not the protobuf empty default.
• This prevents a subtle secondary bug: with protobuf default maxAttempts=0, PopMessageResultFilterImpl would route fresh messages (reconsumeTimes == 0) straight to the DLQ.

Performance ✅

• The null check is O(1). The fallback builds default settings on each miss, but this only happens before the client registers its settings — a brief window.

Compatibility ✅

• No API changes. The new getDefaultConsumerSettings() method is additive.
• Behavior change is strictly a bug fix — previously crashed, now works correctly.

Tests ✅

• New test testDefaultConsumerSettingsHasNonZeroMaxAttempts validates the fallback settings carry a positive maxAttempts.
• New test class PopMessageResultFilterImplTest (90 lines) validates the DLQ routing behavior with the fix.

Verdict

Solid defensive fix with good test coverage. Prevents both an NPE and a silent DLQ misrouting.