From ee7ed0093d6a05adc5006d470aaf16405cf6f59e Mon Sep 17 00:00:00 2001 From: junhyeong9812 Date: Fri, 11 Sep 2026 01:39:42 +0900 Subject: [PATCH] Prevent double size decrement in ConcurrentLruCache markAsRemoved transitions a node to the removed state and decrements the current size, but it did not check whether the node had already been removed. The eviction path and an explicit removal can process the same node in sequence: when a write drain runs a queued AddTask whose eviction polls a node that a concurrent remove(K) has already taken out of the cache, the eviction decrements the size, and the queued RemovalTask for the same node decrements it again. The sibling transition markForRemoval guards against invalid transitions; this one did not. Each extra decrement makes currentSize permanently smaller than the number of cached entries, so eviction stops triggering and the cache exceeds its capacity for good, silently. A bounded two-thread stress run accumulates the drift reliably: before the change the cache stabilized far above its capacity in 20 out of 20 runs. markAsRemoved now returns without decrementing when the entry is already in the removed state, mirroring the guard in markForRemoval. The removed state is terminal, so each node is counted down exactly once. The new test races explicit removals against eviction and then verifies that the cache converges back to its capacity; it also asserts that the racing thread ran and terminated cleanly. Signed-off-by: junhyeong9812 --- .../util/ConcurrentLruCache.java | 6 ++- .../util/ConcurrentLruCacheTests.java | 45 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java b/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java index 38dffe5d8faf..62cadbf212b7 100644 --- a/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java +++ b/spring-core/src/main/java/org/springframework/util/ConcurrentLruCache.java @@ -198,11 +198,15 @@ public void clear() { } /* - * Transition the node to the {@code removed} state and decrement the current size of the cache. + * Transition the node to the {@code removed} state and decrement the + * current size of the cache, unless the node has already been removed. */ private void markAsRemoved(Node node) { for (; ; ) { CacheEntry current = node.get(); + if (current.state == CacheEntryState.REMOVED) { + return; + } CacheEntry removed = new CacheEntry<>(current.value, CacheEntryState.REMOVED); if (node.compareAndSet(current, removed)) { this.currentSize.lazySet(this.currentSize.get() - 1); diff --git a/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java b/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java index 784bc97f3763..b4dfadf08e0d 100644 --- a/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java +++ b/spring-core/src/test/java/org/springframework/util/ConcurrentLruCacheTests.java @@ -16,6 +16,10 @@ package org.springframework.util; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; @@ -108,4 +112,45 @@ void clearAndSize() { assertThat(this.cache.contains("k3")).isTrue(); } + @Test + void removeRacingWithEvictionDoesNotExceedCapacity() throws Exception { + ConcurrentLruCache cache = new ConcurrentLruCache<>(2, key -> "value" + key); + AtomicBoolean stop = new AtomicBoolean(); + AtomicInteger removals = new AtomicInteger(); + AtomicReference failure = new AtomicReference<>(); + Thread remover = new Thread(() -> { + try { + while (!stop.get()) { + cache.get(0); + if (cache.remove(0)) { + removals.incrementAndGet(); + } + } + } + catch (Throwable ex) { + failure.set(ex); + } + }); + remover.start(); + try { + for (int i = 1; i <= 50_000; i++) { + cache.get(i); + } + } + finally { + stop.set(true); + remover.join(5000); + } + int budget = 50_000; + int key = 100_000; + while (cache.size() > cache.capacity() && budget-- > 0) { + cache.get(key++); + } + + assertThat(remover.isAlive()).isFalse(); + assertThat(failure.get()).isNull(); + assertThat(removals.get()).isGreaterThan(0); + assertThat(cache.size()).isLessThanOrEqualTo(cache.capacity()); + } + }