Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7fead53
Fix MemoryCache negative _cacheSize drift on concurrent replace+remove
sablancoleis Jun 10, 2026
5461528
Add concurrency regression test for MemoryCache size-tracking drift (…
sablancoleis Jun 10, 2026
ed121f9
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 10, 2026
4f3d193
Merge main into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 10, 2026
01f90d3
Keep prior-aware capacity check; commit only entry.Size
sablancoleis Jun 10, 2026
ee67b97
Use explicit types and target-typed new in the regression test
sablancoleis Jun 10, 2026
5904776
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 10, 2026
9fddf79
Use LongRunning tasks for the concurrent size-tracking regression test
sablancoleis Jun 10, 2026
03aeb9f
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 10, 2026
8664fcb
Fix build: use PlatformDetection.IsMultithreadingSupported
sablancoleis Jun 11, 2026
25f37a3
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 11, 2026
f3bb16a
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 12, 2026
6538000
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 12, 2026
a243d46
Address review: remove redundant SizeLimit test, mark concurrency tes…
sablancoleis Jun 15, 2026
b28f90e
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 15, 2026
5491394
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 15, 2026
9d93550
Merge branch 'main' into fix/memorycache-sizelimit-negative-drift
sablancoleis Jun 16, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand All @@ -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();
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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()
Comment thread
sablancoleis marked this conversation as resolved.
{
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);
}
}
}
Loading