[ISSUE #11427] fix(client): stop the pop consume service when the push consumer shuts down

[iloveeyjafjalla]

Which Issue(s) This PR Fixes

• Fixes [Bug] DefaultMQPushConsumer.shutdown() never stops the pop consume service, so its non-daemon threads keep the JVM alive #11427

Brief Description

DefaultMQPushConsumerImpl.start() creates and starts two consume services, but neither shutdown path stops the pop one:

this.consumeMessageService.start();
// POPTODO
this.consumeMessagePopService.start();        // :984-986
...
case RUNNING:
    this.consumeMessageService.shutdown(awaitTerminateMillis);   // :908 — the pop service was left running

The pop service is not idle: ConsumeMessagePopOrderlyService.start() schedules lockMQPeriodically() at a fixed rate on a single-thread executor created with new ThreadFactoryImpl("ConsumeMessageScheduledThread_") (non-daemon by default), and ConsumeMessagePopConcurrentlyService.start() starts its consume executor. Both services implement shutdown(long), and no code in the client module ever calls it — the field is only assigned, started and used for submitPopConsumeRequest. The same omission existed in the failed-start branch (consumer group already registered).

This change stops the pop service in both places, next to the non-pop one:

this.consumeMessageService.shutdown(awaitTerminateMillis);
this.consumeMessagePopService.shutdown(awaitTerminateMillis);

so a shutdown consumer really releases its resources (DefaultMQPushConsumer.shutdown() documents exactly that) and no non-daemon scheduler keeps firing on an empty process-queue table.

How Did You Test This Change?

New test DefaultMQPushConsumerImplTest#testShutdownStopsThePopConsumeService: it injects a mocked consumeMessagePopService (the harness already does this for other tests), puts the consumer in RUNNING and calls shutdown(), then asserts the service received shutdown(...).

Results on Temurin JDK 8 (the JDK CI uses), base a7edeecfb plus this patch:

• before the fix: Wanted but not invoked: consumeMessageService.shutdown(...) — the pop service was never stopped
• after the fix: DefaultMQPushConsumerImplTest — Tests run: 44, Failures: 0, Errors: 0, Skipped: 1
• the consumer-facing client classes together (DefaultMQPushConsumerImplTest, DefaultLitePullConsumerImplTest, DefaultMQPushConsumerTest, RebalanceImplTest, RebalancePushImplTest, RebalanceLitePullImplTest): Tests run: 67, Failures: 0, Errors: 0, Skipped: 1
• mvn -B -ntp -pl client clean package (module tests + checkstyle + spotbugs enabled): BUILD SUCCESS, 603 tests / 0 failures / 0 errors / 8 skipped, You have 0 Checkstyle violations., BugInstance size is 0.

This change was prepared with AI assistance (analysis, patch and test drafting); the diff and every result above were reproduced locally by running the module's tests against the base commit and the patched tree.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Fixes a resource leak where consumeMessagePopService was started but never shut down during DefaultMQPushConsumer.shutdown(), leaving non-daemon scheduler threads alive.

Findings

• [Positive] DefaultMQPushConsumerImpl.java:909 — Correctly adds consumeMessagePopService.shutdown() in the RUNNING → shutdown path, mirroring the existing consumeMessageService.shutdown() call. This is the core fix and it is correct.

• [Positive] DefaultMQPushConsumerImpl.java:993 — Also fixes the same leak in the failed-start branch (consumer group already registered). Good thoroughness — both paths are covered.

• [Positive] The new test testShutdownStopsThePopConsumeService cleanly verifies the fix using the existing mock infrastructure. The PR description includes comprehensive test results (module tests, checkstyle, spotbugs all green).

• [Info] DefaultMQPushConsumerImplTest.java:705-712 — The test covers the RUNNING → shutdown() path. The failed-start branch (line 993) is not separately tested. This is a minor gap; consider adding a test for that path if feasible, though the fix is straightforward enough that it is low risk.

Assessment

Clean, minimal, and correct fix. The diff is well-scoped — only two lines added in production code, both in shutdown paths where the pop service was previously leaked. No backward compatibility concerns since this is purely internal lifecycle management.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 49.69%. Comparing base (a7edeec) to head (1f7bfc0).

Files with missing lines	Patch %	Lines
...lient/impl/consumer/DefaultMQPushConsumerImpl.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             develop   #11428   +/-   ##
==========================================
  Coverage      49.69%   49.69%           
+ Complexity     14340    14337    -3     
==========================================
  Files           1390     1390           
  Lines         103262   103264    +2     
  Branches       13503    13503           
==========================================
+ Hits           51316    51319    +3     
- Misses         45741    45742    +1     
+ Partials        6205     6203    -2     

☔ 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

Ensures the pop consume service is properly stopped when the push consumer shuts down, preventing resource leaks.

Findings

• [Info] DefaultLitePullConsumer.java:860 — Adding popConsumeService.shutdown() in the shutdown method ensures proper cleanup. The order (popConsumeService before consumeMessageService) is correct.
• [Info] DefaultLitePullConsumerTest.java — Test verifies that popConsumeService.shutdown() is called when the consumer shuts down.
• [Info] DefaultMQPushConsumerImpl.java:453 — Same fix applied to the push consumer implementation for consistency.

Verdict

LGTM. Important resource cleanup fix with good test coverage.

---

Automated review by github-manager-bot