Cache LastAccessed during MemoryCache compaction (dotnet#61187)

* Cache LastAccessed during MemoryCache compaction

During cache compaction, we are sorting entries based on their LastAccessed time. However, since the cache entries can still be used concurrently on other threads, the LastAccessed time may be updated in the middle of sorting the entries. This leads to exceptions in a background thread, crashing the process.

The fix is to cache the LastAccessed time outside of the entry when we are adding it to the list. This will ensure the time is stable during the compaction process.

Fix dotnet#61032

Author: eerhardt

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

@@ -395,9 +395,10 @@ public void Compact(double percentage)
         private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntrySize)
         {
             var entriesToRemove = new List<CacheEntry>();
-            var lowPriEntries = new List<CacheEntry>();
-            var normalPriEntries = new List<CacheEntry>();
-            var highPriEntries = new List<CacheEntry>();
+            // cache LastAccessed outside of the CacheEntry so it is stable during compaction
+            var lowPriEntries = new List<(CacheEntry entry, DateTimeOffset lastAccessed)>();
+            var normalPriEntries = new List<(CacheEntry entry, DateTimeOffset lastAccessed)>();
+            var highPriEntries = new List<(CacheEntry entry, DateTimeOffset lastAccessed)>();
             long removedSize = 0;
 
             // Sort items by expired & priority status
@@ -415,13 +416,13 @@ private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntry
                     switch (entry.Priority)
                     {
                         case CacheItemPriority.Low:
-                            lowPriEntries.Add(entry);
+                            lowPriEntries.Add((entry, entry.LastAccessed));
                             break;
                         case CacheItemPriority.Normal:
-                            normalPriEntries.Add(entry);
+                            normalPriEntries.Add((entry, entry.LastAccessed));
                             break;
                         case CacheItemPriority.High:
-                            highPriEntries.Add(entry);
+                            highPriEntries.Add((entry, entry.LastAccessed));
                             break;
                         case CacheItemPriority.NeverRemove:
                             break;
@@ -445,7 +446,7 @@ private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntry
             // ?. Items with the soonest absolute expiration.
             // ?. Items with the soonest sliding expiration.
             // ?. Larger objects - estimated by object graph size, inaccurate.
-            static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, Func<CacheEntry, long> computeEntrySize, List<CacheEntry> entriesToRemove, List<CacheEntry> priorityEntries)
+            static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, Func<CacheEntry, long> computeEntrySize, List<CacheEntry> entriesToRemove, List<(CacheEntry Entry, DateTimeOffset LastAccessed)> priorityEntries)
             {
                 // Do we meet our quota by just removing expired entries?
                 if (removalSizeTarget <= removedSize)
@@ -458,8 +459,8 @@ static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, F
                 // TODO: Refine policy
 
                 // LRU
-                priorityEntries.Sort((e1, e2) => e1.LastAccessed.CompareTo(e2.LastAccessed));
-                foreach (CacheEntry entry in priorityEntries)
+                priorityEntries.Sort(static (e1, e2) => e1.LastAccessed.CompareTo(e2.LastAccessed));
+                foreach ((CacheEntry entry, _) in priorityEntries)
                 {
                     entry.SetExpired(EvictionReason.Capacity);
                     entriesToRemove.Add(entry);

src/libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj

@@ -14,4 +14,8 @@
     <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\src\Microsoft.Extensions.Primitives.csproj" />
   </ItemGroup>
 
+  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">
+    <PackageReference Include="System.ValueTuple" Version="$(SystemValueTupleVersion)" />
+  </ItemGroup>
+
 </Project>

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

@@ -2,6 +2,8 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
+using System.Threading;
+using System.Threading.Tasks;
 using Microsoft.Extensions.Internal;
 using Xunit;
 
@@ -83,4 +85,57 @@ public void CompactPrioritizesLRU()
             Assert.Equal("value4", cache.Get("key4"));
         }
     }
+
+    [Collection(nameof(DisableParallelization))]
+    public class CompactTestsDisableParallelization
+    {
+        /// <summary>
+        /// Tests a race condition in Compact where CacheEntry.LastAccessed is getting updated
+        /// by a different thread than what is doing the Compact, leading to sorting failing.
+        /// 
+        /// See https://github.com/dotnet/runtime/issues/61032.
+        /// </summary>
+        [Fact]
+        public void CompactLastAccessedRaceCondition()
+        {
+            const int numEntries = 100;
+            MemoryCache cache = new MemoryCache(new MemoryCacheOptions());
+            Random random = new Random();
+
+            void FillCache()
+            {
+                for (int i = 0; i < numEntries; i++)
+                {
+                    cache.Set($"key{i}", $"value{i}");
+                }
+            }
+
+            // start a few tasks to access entries in the background
+            Task[] backgroundAccessTasks = new Task[Environment.ProcessorCount];
+            bool done = false;
+
+            for (int i = 0; i < backgroundAccessTasks.Length; i++)
+            {
+                backgroundAccessTasks[i] = Task.Run(async () =>
+                {
+                    while (!done)
+                    {
+                        cache.TryGetValue($"key{random.Next(numEntries)}", out _);
+                        await Task.Yield();
+                    }
+                });
+            }
+
+            for (int i = 0; i < 1000; i++)
+            {
+                FillCache();
+
+                cache.Compact(1);
+            }
+
+            done = true;
+
+            Task.WaitAll(backgroundAccessTasks);
+        }
+    }
 }

src/libraries/Microsoft.Extensions.Caching.Memory/tests/Microsoft.Extensions.Caching.Memory.Tests.csproj

@@ -5,6 +5,11 @@
     <EnableDefaultItems>true</EnableDefaultItems>
   </PropertyGroup>
 
+  <ItemGroup>
+    <Compile Include="$(CommonTestPath)TestUtilities\System\DisableParallelization.cs"
+             Link="Common\TestUtilities\System\DisableParallelization.cs" />
+  </ItemGroup>
+  
   <ItemGroup>
     <ProjectReference Include="..\src\Microsoft.Extensions.Caching.Memory.csproj" SkipUseReferenceAssembly="true" />
     <ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.DependencyInjection\src\Microsoft.Extensions.DependencyInjection.csproj" />