[release/6.0] Cache LastAccessed during MemoryCache compaction (#62286)

* Cache LastAccessed during MemoryCache compaction (#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 #61032

* Update fix for 6.0.x servicing.

1. Remove the dependency on ValueTuple and use a custom struct instead.
2. Add servicing package changes.
3. In the tests, move the DisableParallelization collection declaration in the test project, since it is only in "Common" in main.

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<CompactPriorityEntry>();
+            var normalPriEntries = new List<CompactPriorityEntry>();
+            var highPriEntries = new List<CompactPriorityEntry>();
             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(new CompactPriorityEntry(entry, entry.LastAccessed));
                             break;
                         case CacheItemPriority.Normal:
-                            normalPriEntries.Add(entry);
+                            normalPriEntries.Add(new CompactPriorityEntry(entry, entry.LastAccessed));
                             break;
                         case CacheItemPriority.High:
-                            highPriEntries.Add(entry);
+                            highPriEntries.Add(new CompactPriorityEntry(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<CompactPriorityEntry> priorityEntries)
             {
                 // Do we meet our quota by just removing expired entries?
                 if (removalSizeTarget <= removedSize)
@@ -458,9 +459,10 @@ 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 (CompactPriorityEntry priorityEntry in priorityEntries)
                 {
+                    CacheEntry entry = priorityEntry.Entry;
                     entry.SetExpired(EvictionReason.Capacity);
                     entriesToRemove.Add(entry);
                     removedSize += computeEntrySize(entry);
@@ -473,6 +475,20 @@ static void ExpirePriorityBucket(ref long removedSize, long removalSizeTarget, F
             }
         }
 
+        // use a struct instead of a ValueTuple to avoid adding a new dependency
+        // on System.ValueTuple on .NET Framework in a servicing release
+        private readonly struct CompactPriorityEntry
+        {
+            public readonly CacheEntry Entry;
+            public readonly DateTimeOffset LastAccessed;
+
+            public CompactPriorityEntry(CacheEntry entry, DateTimeOffset lastAccessed)
+            {
+                Entry = entry;
+                LastAccessed = lastAccessed;
+            }
+        }
+
         public void Dispose()
         {
             Dispose(true);

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

@@ -4,6 +4,8 @@
     <TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
     <EnableDefaultItems>true</EnableDefaultItems>
     <PackageDescription>In-memory cache implementation of Microsoft.Extensions.Caching.Memory.IMemoryCache.</PackageDescription>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>1</ServicingVersion>
   </PropertyGroup>
 
   <ItemGroup>

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,60 @@ public void CompactPrioritizesLRU()
             Assert.Equal("value4", cache.Get("key4"));
         }
     }
+
+    [CollectionDefinition(nameof(DisableParallelization), DisableParallelization = true)]
+    public class DisableParallelization { }
+
+    [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);
+        }
+    }
 }