ai-yang opened a new issue, #10750: URL: https://github.com/apache/rocketmq/issues/10750
## Affected branch `develop` at `a06836dd564e5e43115493f775626cf98d51d10e`. ## Problem `PopConsumerLockService.removeTimeout()` decides that an entry is expired from a previously read `TimedLock.lockTime`, then removes the entry through the map iterator. The expiration check and removal are not atomic with `tryLock()` for the same key. If the lock is currently free but its old timestamp is expired, the following interleaving is possible: 1. cleanup reads the expired timestamp; 2. another thread successfully reacquires the same `TimedLock` and refreshes its timestamp; 3. cleanup removes the mapping based on its stale decision; 4. a third caller creates a new `TimedLock` for the same key and also acquires it. The first and third callers then both believe that they hold the POP consumer lock. ## Deterministic reproduction A unit test installs a `TimedLock` whose `getLockTime()` uses two latches to pause cleanup immediately after it captures the old timestamp. While cleanup is paused, the test reacquires the lock and refreshes its time, then lets cleanup continue. Without unlocking the first holder, a second `tryLock()` incorrectly succeeds. The unmodified branch failed identically in 5/5 isolated JDK 8 Maven processes: ```text Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 java.lang.AssertionError: Cleanup removed the reacquired lock and allowed a second holder ``` The test uses no sleep, network, random scheduling, or stress loop. All latch waits are bounded only to make a broken test fail instead of hanging. ## Impact Two broker threads can concurrently enter a code path that is intended to be serialized for the same consumer group and topic after a cleanup/reacquire race. This weakens the mutual-exclusion guarantee used by POP consumption and can allow duplicate concurrent processing of protected work. ## Expected behavior Once a lock has been reacquired and its lease timestamp refreshed, cleanup must not remove that current mapping based on an older expiration observation. A subsequent caller must continue to observe the lock as held. ## Suggested direction Serialize acquisition/refresh and the authoritative expiration recheck/removal per map key: - perform lookup/creation, CAS acquisition, and timestamp refresh inside `ConcurrentMap.compute`; - keep the outer expiration read only as a fast filter, then use `computeIfPresent` to re-read the current mapped lock and its current timestamp before deciding whether to return `null`. This preserves the existing behavior that a genuinely expired lease may be removed even if its old holder remains marked as locked. The separate, pre-existing question of an old timed-out holder later calling key-only `unlock()` on a newer lock generation is outside this report. ## Related work checked Searches covered open and closed issues and pull requests using the class name, `removeTimeout`, `TimedLock`, POP lock timeout cleanup, and reacquisition terms. No equivalent report, implementation, assignee, or maintainer handoff was found. #10519 and #10448 only mention POP locks while changing batch invisibility/delete behavior and do not touch this cleanup race. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
