Fix MemoryCache concurrency tests

src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs

@@ -28,7 +28,6 @@ internal sealed partial class CacheEntry : ICacheEntry
         private long _absoluteExpirationTicks = NotSet;
         private short _absoluteExpirationOffsetMinutes;
         private bool _isDisposed;
-        private bool _isExpired;
         private bool _isValueSet;
         private byte _evictionReason;
         private byte _priority = (byte)CacheItemPriority.Normal;
@@ -228,17 +227,20 @@ private void CommitWithTracking()
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)] // added based on profiling
         internal bool CheckExpired(DateTime utcNow)
-            => _isExpired
+            => EvictionReason != EvictionReason.None
                 || CheckForExpiredTime(utcNow)
                 || (_tokens != null && _tokens.CheckForExpiredTokens(this));
 
         internal void SetExpired(EvictionReason reason)
         {
+            // The eviction reason doubles as the "is expired" flag, so that a reader observing an expired
+            // entry always observes the reason that expired it. A separate flag would be a second,
+            // independently visible write: on a weak memory model a concurrent reader could see the entry
+            // as expired while still reading EvictionReason.None, and evict a live entry (dotnet/runtime#72879).
             if (EvictionReason == EvictionReason.None)
             {
                 EvictionReason = reason;
             }
-            _isExpired = true;
             _tokens?.DetachTokens();
         }
 

src/libraries/Microsoft.Extensions.Caching.Memory/tests/CapacityTests.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Runtime.CompilerServices;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Caching.Memory.Infrastructure;
@@ -487,11 +488,18 @@ public void ClearZeroesTheSize()
 
         internal static void AssertCacheSize(long size, MemoryCache cache)
         {
-            // Size is only eventually consistent, so retry a few times
-            RetryHelper.Execute(() =>
-            {
-                Assert.Equal(size, cache.Size);
-            }, maxAttempts: 12, (iteration) => (int)Math.Pow(2, iteration)); // 2ms, 4ms.. 4096 ms. In practice, retries are rarely needed.
+            // Size is only eventually consistent, so retry a few times. Note that the expected size must
+            // be a constant. Reading it from the cache instead produces a stale snapshot that a
+            // concurrent overcapacity compaction can move away from, and no number of retries will then
+            // converge; use AssertEventually and re-read both sides inside the callback for that case.
+            AssertEventually(() => Assert.Equal(size, cache.Size));
         }
+
+        /// <summary>
+        /// Retries <paramref name="assert"/> until the cache state it inspects settles. Every value the
+        /// assertion depends on must be read inside the callback.
+        /// </summary>
+        internal static void AssertEventually(Action assert, [CallerMemberName] string? testName = null) =>
+            RetryHelper.Execute(assert, maxAttempts: 12, (iteration) => (int)Math.Pow(2, iteration), testName: testName); // 2ms, 4ms.. 2048ms. In practice, retries are rarely needed.
     }
 }

src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs

@@ -569,185 +569,156 @@ public void SetGetAndRemoveWorksWithObjectKeysWhenDifferentReferences()
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/72879")] // issue in cache
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/72890")] // issue in test
         public void GetAndSet_AreThreadSafe_AndUpdatesNeverLeavesNullValues()
         {
             var cache = CreateCache();
             string key = "myKey";
-            var cts = new CancellationTokenSource();
-            var readValueIsNull = false;
+            bool readValueIsNull = false;
 
             cache.Set(key, new Guid());
 
-            var task0 = Task.Run(() =>
-            {
-                while (!cts.IsCancellationRequested)
-                {
-                    cache.Set(key, Guid.NewGuid());
-                }
-            });
+            const int WriterCount = 2;
+            const int WriterIterations = 20_000;
+            int activeWriters = WriterCount;
+            using var barrier = new Barrier(WriterCount + 1);
 
-            var task1 = Task.Run(() =>
+            var workers = new Task[WriterCount + 1];
+            for (int i = 0; i < WriterCount; i++)
             {
-                while (!cts.IsCancellationRequested)
+                workers[i] = StartWorker(() =>
                 {
-                    cache.Set(key, Guid.NewGuid());
-                }
-            });
+                    try
+                    {
+                        barrier.SignalAndWait();
+                        for (int j = 0; j < WriterIterations; j++)
+                        {
+                            cache.Set(key, Guid.NewGuid());
+                        }
+                    }
+                    finally
+                    {
+                        Interlocked.Decrement(ref activeWriters);
+                    }
+                });
+            }
 
-            var task2 = Task.Run(() =>
+            // The reader keeps polling for as long as any writer is still replacing the entry, so it
+            // covers the whole write window rather than a fixed number of iterations of its own.
+            workers[WriterCount] = StartWorker(() =>
             {
-                while (!cts.IsCancellationRequested)
+                barrier.SignalAndWait();
+                while (Volatile.Read(ref activeWriters) > 0)
                 {
-                    if (cache.Get(key) == null)
+                    if (cache.Get(key) is null)
                     {
-                        // Stop this task and update flag for assertion
                         readValueIsNull = true;
-                        break;
+                        return;
                     }
                 }
             });
 
-            var task3 = Task.Delay(TimeSpan.FromSeconds(7));
-
-            Task.WaitAny(task0, task1, task2, task3);
+            WaitForWorkers(workers);
 
             Assert.False(readValueIsNull);
-            Assert.Equal(TaskStatus.Running, task0.Status);
-            Assert.Equal(TaskStatus.Running, task1.Status);
-            Assert.Equal(TaskStatus.Running, task2.Status);
-            Assert.Equal(TaskStatus.RanToCompletion, task3.Status);
-
-            cts.Cancel();
-            Task.WaitAll(task0, task1, task2, task3);
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/72890")]
         public void OvercapacityPurge_AreThreadSafe()
         {
-            var cache = new MemoryCache(new MemoryCacheOptions
+            const long SizeLimit = 10;
+            using var cache = new MemoryCache(new MemoryCacheOptions
             {
                 ExpirationScanFrequency = TimeSpan.Zero,
-                SizeLimit = 10,
+                SizeLimit = SizeLimit,
                 CompactionPercentage = 0.5
             });
-            var cts = new CancellationTokenSource();
-            var limitExceeded = false;
-
-            var task0 = Task.Run(() =>
-            {
-                while (!cts.IsCancellationRequested)
-                {
-                    if (cache.Size > 10)
-                    {
-                        limitExceeded = true;
-                        break;
-                    }
-                    cache.Set(Guid.NewGuid(), Guid.NewGuid(), new MemoryCacheEntryOptions { Size = 1 });
-                }
-            }, cts.Token);
 
-            var task1 = Task.Run(() =>
-            {
-                while (!cts.IsCancellationRequested)
-                {
-                    if (cache.Size > 10)
-                    {
-                        limitExceeded = true;
-                        break;
-                    }
-                    cache.Set(Guid.NewGuid(), Guid.NewGuid(), new MemoryCacheEntryOptions { Size = 1 });
-                }
-            }, cts.Token);
+            const int WorkerCount = 3;
+            const int IterationsPerWorker = 10_000;
+            using var barrier = new Barrier(WorkerCount);
+            long sizeOverLimit = 0;
 
-            var task2 = Task.Run(() =>
+            var workers = new Task[WorkerCount];
+            for (int i = 0; i < WorkerCount; i++)
             {
-                while (!cts.IsCancellationRequested)
+                workers[i] = StartWorker(() =>
                 {
-                    if (cache.Size > 10)
+                    barrier.SignalAndWait();
+                    for (int j = 0; j < IterationsPerWorker; j++)
                     {
-                        limitExceeded = true;
-                        break;
+                        long size = cache.Size;
+                        if (size > SizeLimit)
+                        {
+                            Interlocked.CompareExchange(ref sizeOverLimit, size, 0);
+                            return;
+                        }
+
+                        cache.Set(Guid.NewGuid(), Guid.NewGuid(), new MemoryCacheEntryOptions { Size = 1 });
                     }
-                    cache.Set(Guid.NewGuid(), Guid.NewGuid(), new MemoryCacheEntryOptions { Size = 1 });
-                }
-            }, cts.Token);
+                });
+            }
 
-            cts.CancelAfter(TimeSpan.FromSeconds(5));
-            var task3 = Task.Delay(TimeSpan.FromSeconds(7));
+            WaitForWorkers(workers);
 
-            Task.WaitAll(task0, task1, task2, task3);
+            Assert.True(sizeOverLimit == 0, $"Cache size reached {sizeOverLimit}, above the limit of {SizeLimit}.");
 
-            Assert.Equal(TaskStatus.RanToCompletion, task0.Status);
-            Assert.Equal(TaskStatus.RanToCompletion, task1.Status);
-            Assert.Equal(TaskStatus.RanToCompletion, task2.Status);
-            Assert.Equal(TaskStatus.RanToCompletion, task3.Status);
-            CapacityTests.AssertCacheSize(cache.Count, cache);
-            Assert.InRange(cache.Count, 0, 10);
-            Assert.False(limitExceeded);
+            // Overcapacity compaction is queued to the thread pool, so entries can still be evicted for a
+            // short while after the writers stop. Re-read both values on every attempt: capturing one of
+            // them up front compares a stale snapshot against a value a late compaction is still moving.
+            CapacityTests.AssertEventually(() =>
+            {
+                long count = cache.Count;
+                Assert.Equal(count, cache.Size);
+                Assert.InRange(count, 0L, SizeLimit);
+            });
         }
 
         [Fact]
-        [ActiveIssue("https://github.com/dotnet/runtime/issues/72890")]
         public void AddAndReplaceEntries_AreThreadSafe()
         {
-            var cache = new MemoryCache(new MemoryCacheOptions
+            const int KeyCount = 10;
+            using var cache = new MemoryCache(new MemoryCacheOptions
             {
                 ExpirationScanFrequency = TimeSpan.Zero,
                 SizeLimit = 20,
                 CompactionPercentage = 0.5
             });
-            var cts = new CancellationTokenSource();
 
-            var random = new Random();
+            const int WorkerCount = 3;
+            const int IterationsPerWorker = 10_000;
+            using var barrier = new Barrier(WorkerCount);
 
-            var task0 = Task.Run(() =>
+            var workers = new Task[WorkerCount];
+            for (int i = 0; i < WorkerCount; i++)
             {
-                while (!cts.IsCancellationRequested)
+                // Random is not thread safe, so every worker gets its own seeded instance.
+                var random = new Random(i);
+                workers[i] = StartWorker(() =>
                 {
-                    var entrySize = random.Next(0, 5);
-                    cache.Set(random.Next(0, 10), entrySize, new MemoryCacheEntryOptions { Size = entrySize });
-                }
-            });
+                    barrier.SignalAndWait();
+                    for (int j = 0; j < IterationsPerWorker; j++)
+                    {
+                        int entrySize = random.Next(0, 5);
+                        cache.Set(random.Next(0, KeyCount), entrySize, new MemoryCacheEntryOptions { Size = entrySize });
+                    }
+                });
+            }
 
-            var task1 = Task.Run(() =>
-            {
-                while (!cts.IsCancellationRequested)
-                {
-                    var entrySize = random.Next(0, 5);
-                    cache.Set(random.Next(0, 10), entrySize, new MemoryCacheEntryOptions { Size = entrySize });
-                }
-            });
+            WaitForWorkers(workers);
 
-            var task2 = Task.Run(() =>
+            // Each entry stores its own size as its value, so the sum over every possible key is the size
+            // the cache should be tracking. See OvercapacityPurge_AreThreadSafe for why this is retried.
+            CapacityTests.AssertEventually(() =>
             {
-                while (!cts.IsCancellationRequested)
+                long expectedSize = 0;
+                for (int i = 0; i < KeyCount; i++)
                 {
-                    var entrySize = random.Next(0, 5);
-                    cache.Set(random.Next(0, 10), entrySize, new MemoryCacheEntryOptions { Size = entrySize });
+                    expectedSize += cache.Get<int>(i);
                 }
-            });
-
-            cts.CancelAfter(TimeSpan.FromSeconds(5));
-            var task3 = Task.Delay(TimeSpan.FromSeconds(7));
-
-            Task.WaitAll(task0, task1, task2, task3);
 
-            Assert.Equal(TaskStatus.RanToCompletion, task0.Status);
-            Assert.Equal(TaskStatus.RanToCompletion, task1.Status);
-            Assert.Equal(TaskStatus.RanToCompletion, task2.Status);
-            Assert.Equal(TaskStatus.RanToCompletion, task3.Status);
-
-            var cacheSize = 0;
-            for (var i = 0; i < 10; i++)
-            {
-                cacheSize += cache.Get<int>(i);
-            }
-
-            CapacityTests.AssertCacheSize(cacheSize, cache);
-            Assert.InRange(cache.Count, 0, 20);
+                Assert.Equal(expectedSize, cache.Size);
+                Assert.InRange(cache.Count, 0, KeyCount);
+            });
         }
 
         [Fact]
@@ -865,6 +836,24 @@ public void MixedKeysUsage()
             Assert.Equal("decimal value", cache.Get(key1));
         }
 
+        /// <summary>
+        /// Runs <paramref name="work"/> on a dedicated thread rather than a thread pool thread. The
+        /// concurrency tests above hammer the cache for their whole run without ever yielding, so leaving
+        /// them on the pool would delay the cache's own background work (overcapacity compaction, expired
+        /// item scans) and the sibling test collections xunit runs in parallel. Note this frees pool
+        /// threads, not cores, so the workers still compete for CPU with whatever runs alongside them.
+        /// </summary>
+        private static Task StartWorker(Action work) =>
+            Task.Factory.StartNew(work, CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default);
+
+        /// <summary>
+        /// Waits for the workers, failing rather than hanging if they do not finish. Those tests exist to
+        /// catch deadlocks and livelocks in <see cref="MemoryCache"/>, so an unbounded wait would turn the
+        /// very bug they hunt into an unattributable CI job timeout instead of a test failure.
+        /// </summary>
+        private static void WaitForWorkers(Task[] workers) =>
+            Assert.True(Task.WaitAll(workers, TimeSpan.FromMinutes(2)), "Cache workers did not complete.");
+
         private class TestKey
         {
             public override bool Equals(object obj) => true;