diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs index 440eeefca2831c..51b5c3fe523e64 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs @@ -177,11 +177,25 @@ internal void SetEntry(CacheEntry entry) // Try to update with the new entry if a previous entries exist. entryAdded = coherentState.TryUpdate(entry.Key, entry, priorEntry); - if (!entryAdded) + if (entryAdded) + { + if (_options.HasSizeLimit) + { + // The prior entry was atomically replaced by this entry via TryUpdate, so + // no other path can also remove (and decrement) it. Decrement its size here + // exactly once, tied to the swap we performed. Doing this speculatively + // inside UpdateCacheSizeExceedsCapacity (before the swap) races with a + // concurrent RemoveEntry of the prior entry and double-counts the decrement, + // drifting _cacheSize negative and permanently blocking all future inserts. + Interlocked.Add(ref coherentState._cacheSize, -priorEntry.Size); + } + } + else { // The update will fail if the previous entry was removed after retrieval. // Adding the new entry will succeed only if no entry has been added since. // This guarantees removing an old entry does not prevent adding a new entry. + // The prior entry's size is decremented by whichever path removed it, not here. entryAdded = coherentState.TryAdd(entry.Key, entry); } } @@ -194,8 +208,8 @@ internal void SetEntry(CacheEntry entry) { if (_options.HasSizeLimit) { - // Entry could not be added, reset cache size - Interlocked.Add(ref coherentState._cacheSize, -entry.Size + (priorEntry?.Size).GetValueOrDefault()); + // Entry could not be added, roll back the size increment for this entry only. + Interlocked.Add(ref coherentState._cacheSize, -entry.Size); } entry.SetExpired(EvictionReason.Replaced); entry.InvokeEvictionCallbacks(); @@ -534,23 +548,27 @@ private bool UpdateCacheSizeExceedsCapacity(CacheEntry entry, CacheEntry? priorE return false; } + long priorSize = priorEntry?.Size ?? 0; long sizeRead = coherentState.Size; for (int i = 0; i < 100; i++) { - long newSize = sizeRead + entry.Size; - if (priorEntry != null) - { - Debug.Assert(entry.Key == priorEntry.Key); - newSize -= priorEntry.Size; - } + // The capacity decision still accounts for the prior entry being replaced (its size is + // freed by the replace), so a same-or-smaller replacement at the size limit is admitted. + // However, only the new entry's size is committed to _cacheSize here. The prior entry's + // size is decremented by the caller, atomically with the dictionary swap that actually + // removes it. Decrementing the prior size here (before the swap) races with a concurrent + // RemoveEntry of the same prior entry and double-counts the decrement, drifting + // _cacheSize negative and permanently blocking all future inserts. + long sizeAfterReplace = sizeRead + entry.Size - priorSize; - if ((ulong)newSize > (ulong)sizeLimit) + if ((ulong)sizeAfterReplace > (ulong)sizeLimit) { - // Overflow occurred, return true without updating the cache size + // Exceeds the limit (or overflow); return true without updating the cache size. return true; } - long original = Interlocked.CompareExchange(ref coherentState._cacheSize, newSize, sizeRead); + long committedSize = sizeRead + entry.Size; + long original = Interlocked.CompareExchange(ref coherentState._cacheSize, committedSize, sizeRead); if (sizeRead == original) { return false; diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheConcurrentSizeTrackingTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheConcurrentSizeTrackingTests.cs new file mode 100644 index 00000000000000..8e108f93f97f51 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheConcurrentSizeTrackingTests.cs @@ -0,0 +1,121 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace Microsoft.Extensions.Caching.Memory +{ + public class MemoryCacheConcurrentSizeTrackingTests + { + // Regression test for the size-tracking double-decrement that drives _cacheSize negative and + // permanently latches a size-limited cache into rejecting all inserts. Many threads + // concurrently Set (replace), Get, and Remove a small set of string keys with short + // expirations under a generous SizeLimit. The working set is a tiny fraction of the limit, so + // no legitimate capacity rejection can occur. After the storm the tracked size must not be + // negative and the cache must still retain fresh, non-expiring entries. + // + // The workers are LongRunning tasks, which the default scheduler backs with dedicated threads + // rather than the shared ThreadPool. This prevents the storm from starving timing-sensitive + // post-eviction callbacks in sibling tests. It runs as OuterLoop because it is a long-running + // stress test, and ConditionalFact skips platforms without real thread support (e.g. browser/wasm). + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsMultithreadingSupported))] + [OuterLoop] + public void ConcurrentSetReplaceAndRemove_DoesNotDriftSizeNegative_NorLatch() + { + using MemoryCache cache = new(new MemoryCacheOptions + { + SizeLimit = 200L * 1024 * 1024, // far larger than the working set below + TrackStatistics = true + }); + + const int KeyCount = 16; + const int ValueSize = 4096; + const int IterationsPerThread = 200_000; + const int SampleMask = 1023; // sample CurrentEstimatedSize roughly every 1024 iterations + byte[] payload = new byte[ValueSize]; + int threadCount = Math.Min(Math.Max(4, Environment.ProcessorCount), 16); + + long observedMinSize = 0; + + Task[] workers = new Task[threadCount]; + for (int t = 0; t < threadCount; t++) + { + int seed = t + 1; + workers[t] = Task.Factory.StartNew( + () => + { + Random rnd = new(seed); + for (int i = 0; i < IterationsPerThread; i++) + { + string key = "k" + rnd.Next(KeyCount); + int roll = rnd.Next(100); + if (roll < 65) + { + using ICacheEntry entry = cache.CreateEntry(key); + entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromMilliseconds(15); + entry.Size = ValueSize; + entry.Value = payload; + } + else if (roll < 85) + { + cache.TryGetValue(key, out _); + } + else + { + cache.Remove(key); + } + + if ((i & SampleMask) == 0) + { + long? size = cache.GetCurrentStatistics()?.CurrentEstimatedSize; + if (size.HasValue && size.Value < Interlocked.Read(ref observedMinSize)) + { + Interlocked.Exchange(ref observedMinSize, size.Value); + } + } + } + }, + CancellationToken.None, + TaskCreationOptions.LongRunning, + TaskScheduler.Default); + } + + Task.WaitAll(workers); + + // Drain the working set so the cache is logically empty. + for (int k = 0; k < KeyCount; k++) + { + cache.Remove("k" + k); + } + Thread.Sleep(100); + + Assert.True(observedMinSize >= 0, $"CurrentEstimatedSize drifted negative to {observedMinSize}."); + + long drainedSize = cache.GetCurrentStatistics().CurrentEstimatedSize ?? 0; + Assert.True(drainedSize >= 0, $"CurrentEstimatedSize is negative after drain: {drainedSize}."); + + // The cache must still retain fresh, non-expiring entries (i.e., it is not latched). + const int Probe = 512; + int retained = 0; + for (int i = 0; i < Probe; i++) + { + string key = "fresh-" + i; + using (ICacheEntry entry = cache.CreateEntry(key)) + { + entry.Size = ValueSize; + entry.Value = payload; + } + + if (cache.TryGetValue(key, out _)) + { + retained++; + } + } + + Assert.Equal(Probe, retained); + } + } +}