MemoryCache with SizeLimit permanently stops caching (silent Set drops) after _cacheSize drifts negative under concurrency

[sablancoleis]

Description

When MemoryCacheOptions.SizeLimit is set, the internal running size counter (CoherentState._cacheSize) is maintained non-atomically with respect to the entry collection: increments happen inside the Interlocked.CompareExchange retry loop in UpdateCacheSizeExceedsCapacity, while decrements happen via Interlocked.Add(ref _cacheSize, -entry.Size) in Remove, CoherentState.RemoveEntry (eviction/expiration), and the failed-add rollback in SetEntry.

Under sustained concurrent Set/Get/Remove on a small set of string keys with short expirations, _cacheSize can drift negative. Once it is negative, the capacity check rejects every subsequent insert, because the comparison casts to unsigned:

// UpdateCacheSizeExceedsCapacity
long newSize = sizeRead + entry.Size;
if ((ulong)newSize > (ulong)sizeLimit)   // negative newSize -> huge ulong -> always true
{
    return true;                          // entry silently rejected (EvictionReason.Capacity)
}

A negative newSize becomes a very large unsigned value, so the check is always true and SetEntry takes the over-capacity branch. SetEntry returns void and does not throw, so callers see success while nothing is retained. Because nothing in normal operation resets _cacheSize to a non-negative value, the cache stays in this state permanently — CurrentEntryCount stuck at 0, every Get a miss — until the process is restarted.

This is a correctness issue (permanent, silent loss of all cached data), and is closely related to the size-accounting problems already tracked in #88733 (faulty/negative CurrentEstimatedSize) and #111959 / #124430 (contended size tracking, "silent entry drops when retries are exhausted"). The novel point here is that the negative-size/cache-empties symptom is reachable purely from concurrent Set/Remove/expiration races — without setting Size after insertion (the trigger in #88733).

Reproduction Steps

Self-contained console app. Reproduces within a few seconds on a multi-core machine; if it does not latch on the first run, re-run or increase Threads/StormFor.

using System.Diagnostics;
using Microsoft.Extensions.Caching.Memory;

var cache = new MemoryCache(new MemoryCacheOptions
{
    SizeLimit = 200L * 1024 * 1024, // 200 MB — far larger than the working set below
    TrackStatistics = true,
});

const int Threads = 64;
const int Keys = 16;            // tiny keyspace -> heavy contention on the same entries
const int ValueSize = 4096;     // working set ~= Keys * ValueSize ~= 64 KB (<< SizeLimit)
var payload = new byte[ValueSize];
var expiry = TimeSpan.FromMilliseconds(15);   // short -> expiry-driven removals race with sets
var stormFor = TimeSpan.FromSeconds(8);

var workers = new Task[Threads];
for (int t = 0; t < Threads; t++)
{
    workers[t] = Task.Run(() =>
    {
        var rnd = new Random(Environment.CurrentManagedThreadId);
        var sw = Stopwatch.StartNew();
        while (sw.Elapsed < stormFor)
        {
            string key = "k" + rnd.Next(Keys);
            int roll = rnd.Next(100);
            if (roll < 65)
            {
                using var e = cache.CreateEntry(key);
                e.AbsoluteExpirationRelativeToNow = expiry;
                e.Size = payload.Length;
                e.Value = payload;
            }
            else if (roll < 85)
            {
                cache.TryGetValue(key, out _);
            }
            else
            {
                cache.Remove(key);
            }
        }
    });
}
await Task.WhenAll(workers);

// Drain the working set and let any remaining entries expire.
for (int k = 0; k < Keys; k++) cache.Remove("k" + k);
await Task.Delay(200);

var afterDrain = cache.GetCurrentStatistics()!;
Console.WriteLine($"After drain : Count={afterDrain.CurrentEntryCount}, EstimatedSize={afterDrain.CurrentEstimatedSize}");

// Retention probe: cache is now logically empty. Set fresh, NON-expiring keys and read each back.
int retained = 0;
for (int i = 0; i < 1000; i++)
{
    string key = "fresh-" + i;
    using (var e = cache.CreateEntry(key)) { e.Size = payload.Length; e.Value = payload; }
    if (cache.TryGetValue(key, out _)) retained++;
}
var final = cache.GetCurrentStatistics()!;
Console.WriteLine($"Retained    : {retained} / 1000");
Console.WriteLine($"Final       : Count={final.CurrentEntryCount}, EstimatedSize={final.CurrentEstimatedSize}");

Expected behavior

After drain : Count=0, EstimatedSize=0      (or any value >= 0)
Retained    : 1000 / 1000
Final       : Count=1000, EstimatedSize=4096000

Set never silently drops entries; CurrentEstimatedSize never goes negative.

Actual behavior (once the race triggers)

After drain : Count=0, EstimatedSize=-40960     <-- negative
Retained    : 0 / 1000                          <-- every fresh Set silently dropped
Final       : Count=0, EstimatedSize=-40960     <-- stuck; never recovers

No exception is thrown by any Set. The cache is permanently unusable for the lifetime of the MemoryCache instance.

Regression?

Yes -- introduced in 9.0 by #103931 ("Subtract prior entry size when adding entry to cache", fixing #36039). Bisected empirically: 8.0.x is not affected; 9.0.x and 10.0.x are. #103931 moved the prior entry's _cacheSize decrement out of the post-swap if (entryAdded) block into the speculative capacity computation (before the TryUpdate swap), which is what races with a concurrent removal and double-counts.

Known Workarounds

• Do not set SizeLimit (disables the size accounting entirely and avoids the defect); bound the cache by expiration / entry count instead.
• A negative-clamp on _cacheSize (or replacing the CAS loop with Interlocked.Add + rollback, as proposed in Replace CAS retry loop with Interlocked.Add in MemoryCache size tracking #124430) would prevent the permanent-reject state.

Configuration

• Microsoft.Extensions.Caching.Memory 10.0.7
• Observed on a .NET Framework 4.7.2 host (the net462 build of the package), x64.
• The size-tracking code path is TFM-independent, so the same race is expected on .NET 8/9/10.

Other information

• Mechanism: negative _cacheSize + the (ulong) cast in UpdateCacheSizeExceedsCapacity make the capacity check permanently true.
• Related: Setting Size on MemoryCacheEntry after insertion causes faulty CurrentEstimatedSize #88733 (faulty/negative CurrentEstimatedSize), High Contention in MemoryCache _cacheSize Updates Causing Performance Degradation #111959 (contended _cacheSize updates), Replace CAS retry loop with Interlocked.Add in MemoryCache size tracking #124430 (proposed Interlocked.Add rewrite; closed, not merged).

[sablancoleis]

Root cause identified (with a fix + bisection)

I dug into this and pinned the exact mechanism, bisected it, and validated a fix against the genuine source. Sharing here before opening a PR.

Mechanism: a double-decrement of the prior entry's size

In MemoryCache.SetEntry, a replace (Set on an existing key) accounts for the prior entry's size speculatively inside UpdateCacheSizeExceedsCapacity, before the entry is actually swapped into the backing dictionary:

// UpdateCacheSizeExceedsCapacity
long newSize = sizeRead + entry.Size;
if (priorEntry != null)
{
    Debug.Assert(entry.Key == priorEntry.Key);
    newSize -= priorEntry.Size;   // <-- speculative: prior not yet removed from the dictionary
}

The actual swap happens later via coherentState.TryUpdate(entry.Key, entry, priorEntry). If another thread concurrently runs RemoveEntry(priorEntry) (expiration scan, explicit Remove, or eviction) in that window, priorEntry.Size is subtracted twice — once speculatively above, and once by the real removal in CoherentState.RemoveEntry (Interlocked.Add(ref _cacheSize, -entry.Size)).

Each leaked decrement drives _cacheSize negative. Once it is negative, the capacity gate latches permanently because the comparison casts to unsigned:

if ((ulong)newSize > (ulong)sizeLimit)   // negative newSize -> huge ulong -> always true
    return true;                          // every subsequent Set silently rejected (no throw)

So after a tiny drift (often ~10 entries), the cache silently drops every insert for the lifetime of the process — CurrentEntryCount stuck at 0, every Get a miss, no exception. This is permanent, silent data loss, not just a perf issue.

Bisection: a .NET 9 regression

• 8.0.x — not affected. 8.x subtracts priorEntry.Size only after a successful TryUpdate, inside the if (entryAdded) block, atomically tied to the swap. A concurrent removal therefore cannot double-count: whichever path actually removes the prior entry is the one (and only one) that decrements it.
• 9.0.x and 10.0.x — affected. The regression is the move of the prior-size subtraction into UpdateCacheSizeExceedsCapacity (speculative, before the swap).

Reproduced empirically with a concurrent Set/Get/Remove storm on a small set of string keys under a generous SizeLimit (working set << limit, so no legitimate capacity rejection can occur):

Package build	Runtime	_cacheSize min	fresh entries retained
8.0.10 (net462)	.NET Framework 4.8	0	512 / 512
9.0.13 (net462)	.NET Framework 4.8	negative	0 / 512
10.0.7 (net462)	.NET Framework 4.8	negative	0 / 512
10.0.7 (net10.0)	.NET 10	negative	0 / 512

Config/operational mitigations do not help (verified): single-threaded writes still drift (the cache's own background expiration/compaction threads race), as do large keyspaces and long expirations. The only effective workarounds are removing SizeLimit or downgrading to 8.x.

Fix: restore the atomic post-swap decrement (8.x semantics)

Decrement priorEntry.Size exactly once, atomically with the TryUpdate that performs the swap, instead of speculatively inside the capacity check. UpdateCacheSizeExceedsCapacity then commits only +entry.Size (worst-case for the limit check, exactly as 8.x).

This deliberately keeps the existing CompareExchange retry loop and the "check capacity before committing" ordering, so it avoids both problems that closed #124430 (overflow-rollback corruption, and false rejections from a transiently-inflated size).

Validation

Against the genuine vendored v10.0.7 MemoryCache source compiled locally:

• Unpatched: reproduces the drift (size goes negative, 0/512 retained).
• Patched: no drift (size stays >= 0), 512/512 retained across repeated runs, and SizeLimit eviction still enforced (a tiny limit still bounds count/size correctly).
• A single-threaded pure-replace microbenchmark (the only path the fix changes) shows the patched vs original difference is within run-to-run noise (~100 ns/op either way) - the per-replace cost is dominated by entry allocation + dictionary ops, and the fix only restores 8.x's one extra Interlocked.Add on the replace path.

I'm preparing a PR with the patch plus a concurrency regression test. Related: #111959, #124430.

[sablancoleis]

Update: bisected to a specific PR, plus a correction to my fix description above.

Regressor: #103931 (9.0, "Subtract prior entry size when adding entry to cache", fixing #36039). It correctly made the capacity check prior-aware so a same-or-smaller replace fits at the size limit, but it also moved the prior entry's _cacheSize decrement out of the post-swap if (entryAdded) block into the speculative pre-swap capacity computation. That speculative decrement is what races with a concurrent RemoveEntry(priorEntry) and double-counts. (So the "materially identical across versions" note in the original description was wrong -- I've corrected it.)

Correction to the fix: my earlier comment said the fix makes the limit check use +entry.Size "exactly as 8.x". That would regress #36039 (it fails ReplaceOldEntryWithSameSizeOrLessNewEntryAtSizeLimitCapacity). The actual fix keeps #103931's prior-aware check (sizeRead + entry.Size - priorSize) and only commits +entry.Size to _cacheSize, moving the -priorSize decrement back to be atomic with the TryUpdate swap -- preserving #36039 while removing the race.

PR: #129215.

[sablancoleis]

We actually hit this one in production, on a service running at pretty large scale. Under load the MemoryCache size counter drifted negative and the cache basically stopped caching, every Set was silently dropped, no exception, nothing in the logs. Took us a while to track down because it fails so quietly.

The only quick mitigation we had was to drop SizeLimit entirely. That gets caching working again, but it leaves us with no memory bound, which isn't something we want to keep running in prod long-term. To safely turn SizeLimit back on we really need this fix (or something equivalent) to land. Root cause is the replace-at-limit accounting from #103931, full writeup in #129186.

The fix is ready in #129215 (CI's green). @cincuranet, @mgravell given the prod impact, could we get this triaged and labeled as a bug? Would really appreciate getting it prioritized, and I'm happy to adjust the fix or test however works best.

[cincuranet]

Added "Bug". Also reviewed the PR, few minor comments, otherwise looks good.

[mgravell]

(responding to ping: I'm no longer "on the inside", so I can't help; good luck, seems a problem indeed)