[messere1]

What changed

• ExclusiveEvictionTombstones now stamps every tombstone with its eviction time (backing store changed from a key set to ConcurrentMap<String, Long>), and gained removeExpired(ttlMillis)
• LiteSubscriptionRegistryImpl.cleanupExpiredSubscriptions discards tombstones older than the check timeout, on the same horizon it already uses to expire subscriptions
• re-adding a tombstone after a later re-eviction refreshes its stamp, restarting the guard window (semantics unchanged otherwise: contains/remove/removeAllOf/removeStale keep their contracts)

Why

A tombstone only needs to survive until the evicted client's next full subscription sync, which any live client performs well within the subscription expiry window (liteSubscriptionCheckTimeoutMills, default 3 minutes). But when the eviction takes the client's last liteTopic, its LiteSubscription is dropped from client2Subscription immediately — and if that client then terminates without sending COMPLETE_REMOVE, nothing ever clears its tombstones:

• the sync-driven removals (remove on re-claim, removeStale on full sync) need requests that never arrive;
• the expiry cleaner only walks client2Subscription, so it cannot see the evicted clientId anymore.

The tombstone set therefore grows without bound under exclusive-mode client churn (every takeover evicts the previous holder; terminated losers leak one clientId$lmqName key each) until broker restart. This is the same reachability gap as #11352 for the channel entry, on the adjacent per-client structure of the same eviction branch.

Expiring on the subscription timeout only affects clients the broker already considers gone: a live client either syncs (clearing its tombstones via removeStale) or lets its subscription expire (which clears them via removeCompleteSubscription → removeAllOf) within the window, so the guard against stale pulls from a just-evicted client is kept for at least as long as the client's own subscription would survive.

Impact

The change is local to the exclusive-eviction tombstone bookkeeping of the lite subscription registry. Non-exclusive groups never get tombstones; eviction, re-claim, full-sync re-notify and client-removal flows keep their behavior.

Fixes #11354

Validation

• mvn -pl broker -Dtest='LiteSubscriptionRegistryImplTest,ExclusiveEvictionTombstonesTest' -Djacoco.skip=true test
  • 54 tests, 0 failures, 0 errors (49 pre-existing + 3 new)
  • testExclusiveEviction_TombstoneExpiredForClientThatNeverSyncsAgain fails on the pristine branch (java.lang.AssertionError — the tombstone survives the expiry sweep with no subscription entry left to sweep) and passes with the fix
  • testExclusiveEviction_FreshTombstoneSurvivesExpirySweep guards the boundary: a tombstone younger than the check timeout is kept by the cleaner (passes on the pristine branch as well)
  • ExclusiveEvictionTombstonesTest#testRemoveExpired covers the TTL unit semantics (fresh kept / aged removed)
• mvn -pl broker -Dtest='org.apache.rocketmq.broker.lite.*Test' -Djacoco.skip=true test
  • 122 tests, 0 failures (whole lite package regression)
• mvn -pl broker -Dtest='AckMessageProcessorTest,LiteManagerProcessorTest,LiteSubscriptionCtlProcessorTest' -Djacoco.skip=true test
  • 39 tests, 0 failures (lite ack/manager/ctl processor regression)
• mvn -pl broker -Dtest='PopLiteMessageProcessorEventLossTest,PopLiteMessageProcessorTest' -Djacoco.skip=true test
  • 24 tests, 0 failures (lite pop processor regression, incl. the tombstone-checked pull path)
• mvn -pl broker checkstyle:check -Dcheckstyle.config.location=style/rmq_checkstyle.xml
  • 0 violations

JaCoCo is skipped locally because the repository's JaCoCo 0.8.5 agent does not support Java 17 class files.

[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

This PR fixes a memory leak in the exclusive eviction tombstone bookkeeping of the lite subscription registry. When a client is evicted from its last liteTopic and then terminates without sending COMPLETE_REMOVE, its tombstones persist indefinitely because the expiry cleaner only walks client2Subscription (which no longer contains the evicted entry). The fix timestamps each tombstone and expires them on the same horizon as the subscription check timeout — a well-reasoned choice since a tombstone only needs to survive until the evicted client's next full sync.

Findings

✅ Correctness — The backing-store migration from Set<String> to ConcurrentMap<String, Long> is clean. All call sites are updated consistently:

• contains() → containsKey() ✓
• add() → put(key, System.currentTimeMillis()) ✓
• removeAllOf() / removeStale() → keySet().removeIf(...) ✓ (correct delegation to map)
• removeExpired() → entrySet().removeIf(...) ✓ (correct for value-condition removal)

✅ Concurrency — ConcurrentHashMap provides all needed guarantees. entrySet().removeIf() and keySet().removeIf() are weakly consistent under concurrent modification — no new race conditions introduced.

✅ Performance — Memory overhead is one Long per tombstone entry (negligible). removeExpired() runs on the same schedule as the existing subscription cleanup — no new hot paths.

✅ Tests — Good coverage including the core expiry scenario, boundary case (fresh tombstone survives sweep), and empty-set edge case. The test that fails on pristine branch and passes with the fix is excellent validation.

Minor Observations (non-blocking)

1. System.currentTimeMillis() and clock adjustments: For relative TTL comparison (now - stamp > ttl), minor NTP adjustments are tolerable. If extreme precision were needed, System.nanoTime() would be preferable, but currentTimeMillis is appropriate here since the TTL window is minutes.

2. Test timing sensitivity: Thread.sleep(50L) in testExclusiveEviction_FreshTombstoneSurvivesExpirySweep is tight. Consider using a slightly larger margin (e.g., 100ms) or mocking the clock to avoid flaky CI on slow runners. This is a minor nit.

Positive Aspects

• Excellent PR description — clearly explains the problem, solution rationale, and impact scope
• Minimal, focused change that doesn't touch unrelated code paths
• The TTL alignment with liteSubscriptionCheckTimeoutMills is well-designed: expired tombstones only affect clients the broker already considers gone
• Re-eviction refreshing the timestamp is a sensible semantic choice

---

Automated review by github-manager-bot

[messere1]

Thanks for the review, and glad the overall approach checks out. Responses to the two minor observations (addressed in 01e5b7d):

1. currentTimeMillis vs nanoTime — deliberate choice: the tombstone stamp is compared against the same System.currentTimeMillis() horizon that the very same sweep already uses for subscription expiry (cleanupExpiredSubscriptions compares LiteSubscription.getUpdateTime() against a currentTimeMillis() read, one code path above removeExpired). Mixing nanoTime into that comparison would put the two expiries in different clock domains. The TTL window is minutes, so NTP slew is well within tolerance.

2. Timing margin of the sleep-based tests — good instinct, though note the margin direction is already safe: Thread.sleep never undershoots, so a slow runner only ages the tombstone further past the TTL (the pass direction for the expiry assertions — the sleeps live in testExclusiveEviction_TombstoneExpiredForClientThatNeverSyncsAgain and ExclusiveEvictionTombstonesTest#testRemoveExpired; testExclusiveEviction_FreshTombstoneSurvivesExpirySweep has no sleep and pits a zero-age tombstone against a 60s TTL, which cannot flake). That said, I bumped both sleeps from 50ms to a 10x margin (200ms vs the 20ms TTL) in 01e5b7d, which additionally rides out a backwards clock step of up to ~180ms between stamp and sweep, at negligible test-runtime cost. Both test classes re-run green (54/54).

Clock injection was considered and rejected for now: it would touch the production registry/tombstone classes for a non-blocking nit, and the repo's lite tests consistently use real-clock aging (e.g. AbstractLiteLifecycleManagerTest).

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

Summary

Updated review after new commit (force-push at 14:04 UTC). The PR converts ExclusiveEvictionTombstones from Set<String> to ConcurrentMap<String, Long> to timestamp each eviction, then adds removeExpired(ttlMillis) to reclaim tombstones of clients that never sync again.

Changes Since Last Review

The diff is structurally identical to the previously reviewed version. The force-push appears to be a rebase or minor amendment — no logic changes detected.

Findings

• [Info] ExclusiveEvictionTombstones.java:86-98 — removeExpired iterates the full map. Acceptable for the expected cardinality (bounded by unique client×topic pairs that go idle). If this ever grows large, consider a ConcurrentSkipListMap ordered by timestamp for O(log n) expiry sweeps.
• [Info] ExclusiveEvictionTombstones.java:49 — put() overwrites the timestamp on re-eviction, correctly restarting the guard window. Good design.
• [Info] Tests cover expiry, re-eviction refresh, and concurrent removal — solid coverage.

Verdict

No new concerns. The fix remains clean and well-scoped.

---

Automated re-review by github-manager-bot

[RockteMQ-AI]

Thanks for the detailed explanations, @messere1.

1. Clock domain consistency — Agreed. Keeping currentTimeMillis aligned with the subscription expiry clock domain is the right call. Mixing nanoTime would introduce unnecessary complexity for a TTL window where NTP slew is negligible.

2. Test timing margin — The 10x margin (200ms vs 20ms TTL) is a solid improvement. The reasoning about Thread.sleep never undershooting is correct, and the additional clock-step tolerance is a nice bonus at minimal test-runtime cost.

Clock injection — Fair to defer. The existing pattern of real-clock aging is consistent across the lite test suite, and injecting a clock for a non-blocking nit would be over-engineering at this stage.

The changes in 01e5b7d address the observations well. No further action needed from my side.