`MemoryCache` size accounting can be corrupted when `SizeLimit` changes between `null` and a value
- Dominant language
- C#
- Stars
- 18.3k
- Forks
- 5.6k
- PR merge metrics
- PR metrics pending
Description
`MemoryCache` retains the supplied `MemoryCacheOptions` instance and reads `SizeLimit` during every cache operation.
`SizeLimit = null` currently controls two separate behaviours:
1. Whether capacity is enforced.
2. Whether entry sizes are added to and subtracted from `_cacheSize`.
If `SizeLimit` crosses between `null` and a value while an entry remains in the cache, insertion and removal can make different accounting decisions. This permanently corrupts `_cacheSize`.
This is related to #88733, but does not require mutating `ICacheEntry.Size`.
### Reproduction Steps
```csharp
using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.Options;
var options = new MemoryCacheOptions
{
TrackStatistics = true
};
using var cache = new MemoryCache(Options.Create(options));
// Size accounting is disabled, so this entry is not added to _cacheSize.
cache.Set("a", "value", new MemoryCacheEntryOptions { Size = 100 });
// Enable size accounting while the entry is still present.
options.SizeLimit = 50;
// Removal now subtracts a size which was never added.
cache.Remove("a");
Console.WriteLine(
cache.GetCurrentStatistics()!.CurrentEstimatedSize); // -100
// The negative value is cast to ulong by the admission check,
// so even a size-1 entry is rejected.
cache.Set("b", "value", new MemoryCacheEntryOptions { Size = 1 });
Console.WriteLine(cache.TryGetValue("b", out _)); // False
```
The reverse transition also corrupts the ledger:
1. Insert an entry while a limit is configured, adding its size.
2. Set `SizeLimit` to `null`.
3. Remove the entry, skipping the subtraction.
4. Re-enable a limit.
The cache is empty, but `_cacheSize` still contains the removed entry's size.
Changing between two non-null limits does not cause this mismatch because size accounting remains enabled throughout.
### Expected behavior
Changing the supplied options object after cache construction must not cause insertion and removal to account for the same entry differently.
### Actual behavior
Insertion and removal independently inspect the current value of `MemoryCacheOptions.SizeLimit`.
This can produce:
- A negative `_cacheSize`, causing every subsequent insertion to be rejected by the unsigned overflow check.
- A positive phantom `_cacheSize`, causing an empty cache to appear full.
- Incorrect replacement and rollback calculations.
Normal insertion, removal and compaction cannot reliably repair the total. `Clear()` or replacing the cache resets it.
### Proposed fix
Separate lifetime size tracking from the current capacity threshold.
`MemoryCache` should capture whether size tracking is enabled when it is constructed:
```csharp
private readonly bool _trackSize;
// Constructor
_trackSize = _options.HasSizeLimit;
```
Use `_trackSize` consistently for:
- Requiring entries to specify `Size`.
- Incrementing `_cacheSize`.
- Replacement accounting.
- Failed-insertion rollback.
- Removal accounting.
- Reporting `CurrentEstimatedSize`.
The numeric `SizeLimitValue` can remain live for admission and compaction.
This permits safe changes between configured limits, for example `50` to `100`, while preserving a stable accounting mode for the cache's lifetime.
If a cache was constructed with `SizeLimit = null`, assigning a limit later would not enable tracking for that existing cache. Supporting that transition fully would require accounting for every existing entry under synchronisation, including defining behaviour for entries without a size.
Contributor guide
Research direction
Start by locating MemoryCache and tracing its insertion, replacement, rollback, removal, compaction, and CurrentEstimatedSize paths, especially where SizeLimit is read. Reproduce both null-to-value and value-to-null transitions from the issue, then verify that accounting remains consistent while capacity admission still follows the current limit.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 66/100