Race Condition in MemoryCache.Compact() Causes Intermittent Test Failures

[StevenTCramer]

Race Condition in MemoryCache.Compact() Causes Intermittent Test Failures

Summary

The MemoryCache<TKey, TValue>.Compact() method has a race condition that causes intermittent test failures when the cache is accessed concurrently. The method enumerates a ConcurrentDictionary while it can be modified by other threads, violating thread-safety guarantees.

Environment

• OS: Ubuntu 24.04.3 LTS (WSL2)
• .NET SDK: 9.0.111
• .NET Runtime: 8.0.21, 9.0.10
• Commit: 1f0c390 (PR Don't put breakpoints on component start tags, end tags or attributes #12422)

Steps to Reproduce

1. Clone the repository
2. Run ./restore.sh
3. Run ./build.sh -test
4. Observe intermittent test failure in Microsoft.CodeAnalysis.Razor.Workspaces.Test

Note: This is a race condition, so it may not reproduce consistently.

Expected Behavior

The ConcurrentSets_DoesNotThrow test should pass consistently.

Actual Behavior

Test intermittently fails with:

System.ArgumentException : The index is equal to or greater than the length of the array, or the number of elements in the dictionary is greater than the available space from index to the end of the destination array.

Stack Trace

at System.Collections.Concurrent.ConcurrentDictionary`2.System.Collections.Generic.ICollection<System.Collections.Generic.KeyValuePair<TKey,TValue>>.CopyTo(KeyValuePair`2[] array, Int32 index)
at System.Linq.Enumerable.ICollectionToArray[TSource](ICollection`1 collection)
at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
at System.Linq.Enumerable.OrderedIterator`1.ToArray()
at System.Linq.Enumerable.ToArray[TSource](IEnumerable`1 source)
at Microsoft.CodeAnalysis.Razor.Utilities.MemoryCache`2.Compact()
at Microsoft.CodeAnalysis.Razor.Utilities.MemoryCache`2.Set(TKey key, TValue value)

Test Results

Before fix:

Microsoft.CodeAnalysis.Razor.Workspaces.Test  Total: 635, Errors: 0, Failed: 1, Skipped: 13

After fix:

Build succeeded in 683.8s (all tests pass)

Root Cause

In src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Utilities/MemoryCache2.csat line 67, theCompact()method enumerates theConcurrentDictionary` with LINQ:

protected virtual void Compact()
{
    var kvps = _dict.OrderBy(x => x.Value.LastAccess).ToArray();

    for (var i = 0; i < _sizeLimit / 2; i++)
    {
        _dict.Remove(kvps[i].Key);
    }
}

The problem:

1. Multiple threads call Set() concurrently
2. Set() can trigger Compact() when the cache is full
3. Compact() enumerates the ConcurrentDictionary with .OrderBy().ToArray()
4. While enumerating, another thread modifies the dictionary via Set()
5. The enumeration throws ArgumentException

While ConcurrentDictionary is thread-safe for individual operations, enumerating it while it's being modified can throw exceptions. This is documented behavior of ConcurrentDictionary.

Solution

Snapshot the dictionary before ordering to prevent enumeration exceptions:

protected virtual void Compact()
{
    var kvps = _dict.ToArray().OrderBy(x => x.Value.LastAccess).ToArray();

    for (var i = 0; i < _sizeLimit / 2; i++)
    {
        _dict.Remove(kvps[i].Key);
    }
}

Fix

File: src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Utilities/MemoryCache2.cs`
Line: 67

Before:

var kvps = _dict.OrderBy(x => x.Value.LastAccess).ToArray();

After:

var kvps = _dict.ToArray().OrderBy(x => x.Value.LastAccess).ToArray();

The initial .ToArray() creates a snapshot of the dictionary at that moment, preventing concurrent modifications from affecting the LINQ enumeration.

Additional Context

• This is a flaky/intermittent test - it depends on thread timing
• The test ConcurrentSets_DoesNotThrow is specifically designed to test concurrent access
• The fix adds minimal overhead (one array allocation) but ensures thread-safety during compaction
• The issue may be more apparent on certain systems/runtimes depending on scheduling

Impact

• Current: Intermittent test failures that can block CI/development
• After Fix: Consistent test passes
• Risk: Low - the change properly handles concurrent access as intended by the test

References

• Microsoft Docs: ConcurrentDictionary<TKey,TValue> Class
• Related test: Microsoft.CodeAnalysis.Razor.Utilities.MemoryCacheTest.ConcurrentSets_DoesNotThrow

[davidwengier]

This one fails on my machine regularly, but it's easy enough to ignore. Interestingly I've never seen it fail in CI on our Ubuntu legs.

I presume this was generated/discovered by an AI @StevenTCramer ? Don't suppose you want to tell it to open a PR? :)

[StevenTCramer]

Yes Claude Code found and generated in this particular case. OK PR created.