[ISSUE #10661] Unregister all producer topics on gRPC client termination

[yx9o]

• Fixes [Enhancement] gRPC producer termination only unregisters the first publishing topic #10661 .

[codecov-commenter]

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 48.22%. Comparing base (d8c3f9e) to head (50808ee).

Files with missing lines	Patch %	Lines
.../rocketmq/proxy/grpc/v2/client/ClientActivity.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10662      +/-   ##
=============================================
- Coverage      48.29%   48.22%   -0.07%     
+ Complexity     13504    13478      -26     
=============================================
  Files           1380     1380              
  Lines         101091   101091              
  Branches       13101    13101              
=============================================
- Hits           48823    48756      -67     
- Misses         46301    46352      +51     
- Partials        5967     5983      +16     

☔ 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

Fixes a bug in ClientActivity.notifyClientTermination where only the first publishing topic gets unregistered when a gRPC producer terminates. The removeChannel() call was inside the topic loop, so only the first iteration got a non-null channel; subsequent iterations silently skipped unregistration.

Findings

• [Positive] ClientActivity.java:159-167 — Correct fix: moves grpcChannelManager.removeChannel(clientId) outside the topic loop, so the same ClientChannelInfo is reused to unregister all publishing topics. This matches the registration pattern in the heartbeat path.
• [Positive] ClientActivity.java:159 — Added block scope { } around the PRODUCER case, which is good practice when introducing local variables in a switch case.
• [Positive] ClientActivityTest.java — The updated test now registers two topics (TOPIC and anotherTopic) and uses ArgumentCaptor to verify that unRegisterProducer is called exactly twice with the same ClientChannelInfo but different topic names. This is a solid regression test.
• [Info] ClientActivityTest.java:232 — The test uses sendClientTelemetry for setup. Consider verifying that this helper correctly populates ClientType.PRODUCER settings with both topics — if the helper path differs from the actual registration flow, the test may not catch real-world regressions.
• [Info] The fix correctly handles the channel == null case (no-op), which means if removeChannel was already called (e.g., duplicate termination), it won't throw NPE. Good defensive behavior.

Suggestions

• Consider adding a brief comment in the PRODUCER case explaining why removeChannel is outside the loop (e.g., // Remove channel once, reuse for all topic unregistrations), to prevent future contributors from accidentally moving it back inside.

Verdict

Approve — Clean, minimal fix with a good regression test. The logic change is straightforward and correctly addresses the issue.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

Summary

Fixes a bug in notifyClientTermination for PRODUCER clients: the original code called removeChannel(clientId) inside the per-topic loop, meaning only the first topic iteration would get a non-null channel. Subsequent iterations would get null and skip unRegisterProducer. The fix moves removeChannel before the loop so the same ClientChannelInfo is reused for all topics.

Findings

• [Critical] ClientActivity.java:159-170 — The original logic was clearly broken: removeChannel(clientId) is a destructive operation (removes and returns), so calling it N times for N topics would only succeed on the first call. The fix correctly hoists the channel removal outside the loop. Good use of a block scope {} to limit variable visibility.
• [Info] ClientActivityTest.java — Test is updated to register two topics (TOPIC and anotherTopic) and verify both are properly unregistered. This directly validates the fix.

Verdict

Clean bug fix with correct scope management and test coverage. The change is minimal and targeted.

---

Automated review by github-manager-bot