[Bug] POP lock timeout cleanup can remove a reacquired lock
- Dominant language
- Java
- Stars
- 22.6k
- Forks
- 12k
- Avg merge
- 3d 1h
- Merged PRs (30d)
- 27
Description
## 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.
Contributor guide
Research direction
Start with PopConsumerLockService.removeTimeout() and TimedLock, then run or add the deterministic latch-based unit test described in the report. Done means cleanup rechecks the current mapping after reacquisition, and a subsequent tryLock remains unsuccessful while the first holder is still locked.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100