[ISSUE #10791] Reject empty proxy heartbeat bodies - #10792
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 hardens proxy remoting heartbeat handling by explicitly rejecting malformed/empty heartbeat payloads with INVALID_PARAMETER, preventing client registration side effects.
Changes:
- Add validation in
ClientManagerActivity#heartBeatfor empty/undecodable bodies and null producer/consumer datasets. - Add
ClientManagerActivityTestcoverage to ensure malformed heartbeats do not register producers/consumers or touch channel management.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/remoting/activity/ClientManagerActivity.java | Adds early validation and returns INVALID_PARAMETER for empty heartbeat / null datasets. |
| proxy/src/test/java/org/apache/rocketmq/proxy/remoting/activity/ClientManagerActivityTest.java | Adds tests asserting malformed heartbeats are rejected and do not register clients. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (heartbeatData == null) { | ||
| return RemotingCommand.buildErrorResponse(ResponseCode.INVALID_PARAMETER, "heartbeat data is empty"); | ||
| } | ||
| if (heartbeatData.getProducerDataSet() == null || heartbeatData.getConsumerDataSet() == null) { | ||
| return RemotingCommand.buildErrorResponse(ResponseCode.INVALID_PARAMETER, | ||
| "heartbeat producerDataSet and consumerDataSet are required"); | ||
| } |
| @Test | ||
| public void testHeartbeatShouldRejectNullDataSets() { | ||
| RemotingCommand request = RemotingCommand.createRequestCommand(RequestCode.HEART_BEAT, null); | ||
| request.setBody("{\"clientID\":\"client-a\",\"producerDataSet\":null,\"consumerDataSet\":[]}" | ||
| .getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| RemotingCommand response = clientManagerActivity.heartBeat(null, request, ProxyContext.create()); | ||
|
|
||
| assertThat(response.getCode()).isEqualTo(ResponseCode.INVALID_PARAMETER); | ||
| assertThat(response.getRemark()).isEqualTo("heartbeat producerDataSet and consumerDataSet are required"); | ||
| verifyNoClientRegistration(); | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10792 +/- ##
=============================================
- Coverage 48.60% 48.49% -0.11%
+ Complexity 13690 13652 -38
=============================================
Files 1381 1381
Lines 101464 101468 +4
Branches 13187 13189 +2
=============================================
- Hits 49318 49211 -107
- Misses 46158 46240 +82
- Partials 5988 6017 +29 ☔ 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.
Summary
Rejects empty heartbeat request bodies early with a clear error response instead of processing them downstream. Good input validation improvement.
LGTM.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
LGTM — defensive improvement with proper validation and test coverage.
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 heartbeat validation coverage with: mvn -q -pl proxy -am -Dtest=ClientManagerActivityTest -Dsurefire.failIfNoSpecifiedTests=false test The empty-heartbeat validation remains separate from #10775 because it belongs to ClientManagerActivity and has independent remoting behavior/tests. |
b449c1b to
c714d67
Compare
Summary
INVALID_PARAMETERinstead of falling into NPE handlingClientManagerActivityTestcoverage to ensure malformed heartbeats do not register clientsTests
mvn -pl proxy -Dtest=ClientManagerActivityTest testCloses #10791