[ISSUE #10750] Fix POP lock cleanup race

[ai-yang]

Which Issue(s) This PR Fixes

Fixes #10750

Brief Description

PopConsumerLockService.removeTimeout() previously made its expiration decision before removing the map entry. A concurrent tryLock() could reacquire the same TimedLock and refresh its timestamp after that decision, but cleanup would still remove the refreshed entry and allow a second holder to be created.

This change makes acquisition/refresh and the authoritative cleanup recheck atomic for each key:

• tryLock() uses ConcurrentHashMap.compute() to select/create and acquire the mapped lock within the per-key remapping boundary.
• removeTimeout() uses computeIfPresent() to recheck the current lock timestamp before removing it.
• Existing behavior for leases that remain expired at the authoritative recheck is preserved.

How Did You Test This Change?

• Unmodified develop: the deterministic latch regression failed in 5/5 isolated JDK 8 Maven processes; an independent rerun also failed 5/5.
• Fixed targeted test class: 20 isolated JDK 8 Maven processes, 2/2 each (40/40 total).
• Full broker -am test: all 10 reactor modules passed; broker ran 753 tests with 0 failures, 0 errors, and 4 skips.
• Checkstyle: 0 violations.
• SpotBugs: 0 bugs/errors.
• git diff --check: passed.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Fixes a race condition in PopConsumerLockService by replacing ConcurrentHashMapUtils.computeIfAbsent with atomic compute() in tryLock, and refactoring removeTimeout to use computeIfPresent with double-check of timeout condition.

Findings

• [Info] PopConsumerLockService.java:42-48 — The compute() approach ensures lock creation and acquisition are atomic. The AtomicBoolean captures the result from within the compute lambda. This fixes the race where a lock could be removed between creation and acquisition.
• [Info] PopConsumerLockService.java:74-89 — The removeTimeout refactoring correctly re-checks the timeout inside computeIfPresent to handle the case where a lock was re-acquired between the iteration check and the removal attempt.
• [Warning] PopConsumerLockService.java:44 — The AtomicBoolean locked is allocated per tryLock call. Under high contention with many concurrent lock attempts, this creates short-lived object pressure. Consider whether a simpler return pattern is possible, though correctness is not affected.
• [Info] PopConsumerLockServiceTest.java — New test removeTimeoutShouldNotRemoveReacquiredLock uses CountDownLatch to deterministically test the race between cleanup and re-acquisition. Good coverage.

Suggestions

• The AtomicBoolean allocation per call is minor but worth noting for hot paths. An alternative would be to restructure compute to return the lock and check its state outside, but the current approach is correct and readable.

---

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: 7cb64c7a8.

• Rechecked the current upstream implementation: no equivalent fix has landed, and timeout cleanup can still remove a lock after a concurrent reacquisition without this patch, breaking per-key mutual exclusion.

• 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.pop.PopConsumerLockServiceTest -Dsurefire.failIfNoSpecifiedTests=false test

  Result: BUILD SUCCESS; 2 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.49%. Comparing base (e348efa) to head (7cb64c7).

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10751      +/-   ##
=============================================
- Coverage      48.58%   48.49%   -0.09%     
+ Complexity     13676    13650      -26     
=============================================
  Files           1381     1381              
  Lines         101475   101484       +9     
  Branches       13190    13190              
=============================================
- Hits           49299    49219      -80     
- Misses         46174    46247      +73     
- Partials        6002     6018      +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.