[ISSUE #10754] Fix LMQ hold request cleanup race

[ai-yang]

Which Issue(s) This PR Fixes

Fixes #10754

Brief Description

LMQ empty-bucket cleanup previously checked ManyPullRequest.isEmpty() and then removed the map entry in separate operations. A concurrent suspend or notification replay could append a request to the selected bucket after the empty check, while cleanup still removed that bucket from pullRequestTable. The request then remained in a detached object and could no longer be reached by message arrival or timeout scans.

This change keeps bucket selection, append, and empty cleanup atomic per key:

• suspended requests and notification replays use one map-aware compute() helper;
• LMQ cleanup uses computeIfPresent() and performs the authoritative synchronized isEmpty() check inside the remapping boundary;
• ordinary non-LMQ hold-request behavior remains unchanged.

How Did You Test This Change?

• Unmodified develop: the deterministic cleanup-vs-suspend regression failed in 5/5 isolated JDK 8 Maven processes.
• Fixed targeted test class: 3/3 once, then 20 isolated Maven/JVM processes at 3/3 each (60/60 total).
• Full broker -am test: all 10 reactor modules passed; 2,369 tests, 9 skips, 0 failures, and 0 errors. Broker ran 755 tests, including the new class at 3/3.
• Checkstyle: 0 violations across all 10 modules.
• SpotBugs: 0 bugs/errors across all code modules.
• git diff --check: passed.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Fixes a race condition in LMQ pull request hold service cleanup and refactors PullRequestHoldService to use atomic ConcurrentHashMap.compute() operations instead of get-then-put patterns.

Findings

• [Info] LmqPullRequestHoldService.java:57 — Replacing the unsafe get-null-check-remove pattern with computeIfPresent is the correct fix. The original code had a TOCTOU race where a new request could be added between the emptiness check and the removal.
• [Info] PullRequestHoldService.java:48-56 — The addPullRequest helper using compute() ensures atomic bucket creation and request addition. The Consumer<ManyPullRequest> parameter is a clean abstraction.
• [Info] PullRequestHoldService.java:180 — The replay list path now also uses the same addPullRequest helper, ensuring consistency.
• [Info] LmqPullRequestHoldServiceTest.java — New test file with concurrent scenario coverage using CountDownLatch for deterministic race testing. Good approach.

Suggestions

• The Consumer<ManyPullRequest> allocation per call is minor but could be replaced with a method reference or lambda cached in a field if GC pressure becomes a concern under high throughput. Not blocking.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Summary

Defensive fix with proper validation and test coverage. LGTM.

---

Automated review by github-manager-bot

[ai-yang]

Status refresh (2026-08-28):

• Rebased the PR onto the latest upstream develop (e348efa66). New head: 997639abd.

• Rechecked the current upstream implementation: no equivalent fix has landed, and the LMQ empty-bucket cleanup can still detach a concurrently appended/replayed pull request without this patch.

• Re-ran the targeted broker reactor on the rebased head:

  mvn -Dmaven.repo.local=/tmp/codex-maven-repository -pl broker -am -DskipITs -Dtest=org.apache.rocketmq.broker.longpolling.LmqPullRequestHoldServiceTest -Dsurefire.failIfNoSpecifiedTests=false test

  Result: BUILD SUCCESS; 3 tests run, 0 failures, 0 errors. Checkstyle and SpotBugs also passed in the reactor.

No additional implementation changes were needed after the rebase. The issue remains actionable and this PR is not superseded by current develop.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.51%. Comparing base (e348efa) to head (997639a).

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10757      +/-   ##
=============================================
- Coverage      48.58%   48.51%   -0.07%     
+ Complexity     13676    13666      -10     
=============================================
  Files           1381     1381              
  Lines         101475   101472       -3     
  Branches       13190    13187       -3     
=============================================
- Hits           49299    49234      -65     
- Misses         46174    46228      +54     
- Partials        6002     6010       +8     

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

[ai-yang]

All 10 CI checks are green on the refreshed single signed-off commit. @guyinyou @xdkxlk, could you please take a human review when convenient? The production diff is small and most additions are the deterministic LMQ cleanup race regression; the key point is atomic empty-bucket removal without detaching a concurrent request.