Clean up and fix thread safety issues in MemoryCache

- Mark as sealed and change protected members to private
- Change "_dict" from "IDictionary<TKey, TValue>" to "ConcurrentDictionary<TKey, TValue>" and rename to "_map".
- Rename "CacheEntry" to "Entry"
- Change "Entry" to protect its data
- Change "Entry.LastAccess" to get and set its data using Volatile.Read/Write to ensure that other threads don't get stale values due to instruction reordering.
- Change "Compact" method to "CompactIfNeeded" and make it acquire the lock.
- Use double-checked locking for compaction
- Use the "SelectAndOrderByAsArray" to avoid the extra allocation of LINQ "OrderBy" followed by "ToArray"
- Update compaction to be thread-safe by...
    1. Capture the value of LastAccess when acquiring an ordered array of all items. This is used when removing items to ensure that items touched by other threads during compaction aren't thrown away.
    2. Switch from IDictionary<TKey, TValue>.Remove(...) to ConcurrentDictionary<TKey, TValue>.TryRemove(...) for removing items.
    3. If we remove an item that was touched during compaction, call TryAdd to restore it.
    4. Count the number of items we remove to ensure that we remove *at least* the expected number of items.
- Add comments throughout.

Author: DustinCampbell

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Utilities/MemoryCache`2.Entry.cs

@@ -0,0 +1,24 @@
+// 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;
+
+namespace Microsoft.CodeAnalysis.Razor.Utilities;
+
+internal sealed partial class MemoryCache<TKey, TValue> where TKey : notnull
+    where TValue : class
+{
+    private sealed class Entry(TValue value)
+    {
+        private long _lastAccessTicks = DateTime.UtcNow.Ticks;
+
+        public DateTime LastAccess => new(Volatile.Read(ref _lastAccessTicks));
+        public TValue Value => value;
+
+        public void UpdateLastAccess()
+        {
+            Volatile.Write(ref _lastAccessTicks, DateTime.UtcNow.Ticks);
+        }
+    }
+}

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Utilities/MemoryCache.TestAccessor.cs renamed to src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Utilities/MemoryCache`2.TestAccessor.cs

@@ -21,7 +21,7 @@ public event Action Compacted
 
         public readonly bool TryGetLastAccess(TKey key, out DateTime result)
         {
-            if (instance._dict.TryGetValue(key, out var value))
+            if (instance._map.TryGetValue(key, out var value))
             {
                 result = value.LastAccess;
                 return true;

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Utilities/MemoryCache`2.cs

@@ -5,81 +5,126 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
-using System.Linq;
 
 namespace Microsoft.CodeAnalysis.Razor.Utilities;
 
-// We've created our own MemoryCache here, ideally we would use the one in Microsoft.Extensions.Caching.Memory,
-// but until we update O# that causes an Assembly load problem.
-internal partial class MemoryCache<TKey, TValue>
+/// <summary>
+///  A thread-safe, size-limited cache with approximate LRU (Least Recently Used)
+///  eviction policy. When the cache reaches its size limit, it removes approximately
+///  half of the least recently used entries.
+/// </summary>
+/// <typeparam name="TKey">The type of keys in the cache.</typeparam>
+/// <typeparam name="TValue">The type of values in the cache.</typeparam>
+/// <param name="sizeLimit">The maximum number of entries the cache can hold before compaction is triggered.</param>
+/// <param name="concurrencyLevel">The estimated number of threads that will update the cache concurrently.</param>
+internal sealed partial class MemoryCache<TKey, TValue>(int sizeLimit = 50, int concurrencyLevel = 2)
     where TKey : notnull
     where TValue : class
 {
-    private const int DefaultSizeLimit = 50;
-    private const int DefaultConcurrencyLevel = 2;
-
-    protected IDictionary<TKey, CacheEntry> _dict;
-
-    private readonly object _compactLock;
-    private readonly int _sizeLimit;
-
+    private readonly ConcurrentDictionary<TKey, Entry> _map = new(concurrencyLevel, capacity: sizeLimit);
+
+    /// <summary>
+    ///  Lock used to synchronize cache compaction operations. This prevents multiple threads
+    ///  from attempting to compact the cache simultaneously while allowing concurrent reads.
+    /// </summary>
+    private readonly object _compactLock = new();
+    private readonly int _sizeLimit = sizeLimit;
+
+    /// <summary>
+    ///  Optional callback invoked after cache compaction completes. Only used by tests.
+    /// </summary>
     private Action? _compactedHandler;
 
-    public MemoryCache(int sizeLimit = DefaultSizeLimit, int concurrencyLevel = DefaultConcurrencyLevel)
-    {
-        _sizeLimit = sizeLimit;
-        _dict = new ConcurrentDictionary<TKey, CacheEntry>(concurrencyLevel, capacity: _sizeLimit);
-        _compactLock = new object();
-    }
-
-    public bool TryGetValue(TKey key, [NotNullWhen(returnValue: true)] out TValue? result)
+    /// <summary>
+    ///  Attempts to retrieve a value from the cache and updates its last access time if found.
+    /// </summary>
+    public bool TryGetValue(TKey key, [NotNullWhen(true)] out TValue? result)
     {
-        if (_dict.TryGetValue(key, out var value))
+        if (_map.TryGetValue(key, out var entry))
         {
-            value.LastAccess = DateTime.UtcNow;
-            result = value.Value;
+            entry.UpdateLastAccess();
+            result = entry.Value;
             return true;
         }
 
         result = default;
         return false;
     }
 
+    /// <summary>
+    ///  Adds or updates a value in the cache. If the cache is at capacity, triggers compaction
+    ///  before adding the new entry.
+    /// </summary>
     public void Set(TKey key, TValue value)
     {
+        CompactIfNeeded();
+
+        _map[key] = new Entry(value);
+    }
+
+    /// <summary>
+    ///  Removes approximately half of the least recently used entries when the cache reaches capacity.
+    /// </summary>
+    private void CompactIfNeeded()
+    {
+        // Fast path: check size without locking
+        if (_map.Count < _sizeLimit)
+        {
+            return;
+        }
+
         lock (_compactLock)
         {
-            if (_dict.Count >= _sizeLimit)
+            // Double-check after acquiring lock in case another thread already compacted
+            if (_map.Count < _sizeLimit)
             {
-                Compact();
+                return;
             }
-        }
 
-        _dict[key] = new CacheEntry
-        {
-            LastAccess = DateTime.UtcNow,
-            Value = value,
-        };
-    }
+            // Create a snapshot with last access times to implement approximate LRU eviction.
+            // This captures each entry's access time to determine which entries were least recently used.
+            var orderedItems = _map.ToArray().SelectAndOrderByAsArray(
+                selector: static x => (x.Key, x.Value.LastAccess),
+                keySelector: static x => x.LastAccess);
 
-    public void Clear() => _dict.Clear();
+            var toRemove = Math.Max(_sizeLimit / 2, 1);
 
-    protected virtual void Compact()
-    {
-        var kvps = _dict.ToArray().OrderBy(x => x.Value.LastAccess).ToArray();
+            // Remove up to half of the oldest entries using an atomic remove-then-check pattern.
+            // This ensures we don't remove entries that were accessed after our snapshot was taken.
+            foreach (var (itemKey, itemLastAccess) in orderedItems)
+            {
+                // Atomic remove-then-check pattern eliminates race conditions
+                // Note: If TryRemove fails, another thread already removed this entry.
+                if (_map.TryRemove(itemKey, out var removedEntry))
+                {
+                    if (removedEntry.LastAccess == itemLastAccess)
+                    {
+                        // Entry was still old when removed - successful eviction
+                        toRemove--;
+
+                        // Stop early if we've removed enough entries
+                        if (toRemove == 0)
+                        {
+                            break;
+                        }
+                    }
+                    else
+                    {
+                        // Entry was accessed after snapshot - try to restore it
+                        // If TryAdd fails, another thread already added a new entry with this key,
+                        // which is acceptable - we preserve the hot entry's data either way
+                        _map.TryAdd(itemKey, removedEntry);
+                    }
+                }
+            }
 
-        for (var i = 0; i < _sizeLimit / 2; i++)
-        {
-            _dict.Remove(kvps[i].Key);
+            _compactedHandler?.Invoke();
         }
-
-        _compactedHandler?.Invoke();
     }
 
-    protected class CacheEntry
-    {
-        public required TValue Value { get; init; }
-
-        public required DateTime LastAccess { get; set; }
-    }
+    /// <summary>
+    ///  Removes all entries from the cache.
+    /// </summary>
+    public void Clear()
+        => _map.Clear();
 }