From d4aeed25548970dc4af1b0a4209113f19ac4a56a Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Tue, 25 Aug 2026 19:00:45 -0700 Subject: [PATCH 1/5] fix trim safety --- .../Lfu/ConcurrentLfuSoakTests.cs | 17 ++ .../Lfu/WeightedTest.cs | 153 ++++++++++++++++++ BitFaster.Caching/Lfu/ConcurrentLfuCore.cs | 8 +- 3 files changed, 174 insertions(+), 4 deletions(-) create mode 100644 BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs diff --git a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs index 69a560d7..931cac4f 100644 --- a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs +++ b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs @@ -272,6 +272,23 @@ await Threaded.Run(threads, () => await RunIntegrityCheckAsync(lfu, iteration); } + [Theory] + [Repeat(soakIterations)] + public async Task Repro(int iteration) + { + const int size = 100; + const long budget = 10_000; + string value = new string('x', size); + + var t = new WeightedTest(maxItems: 1_000_000, maxItemBytes: 1000, maxTotalBytes: budget); + + Parallel.For(0, 5000, i => t.Populate("k" + i, value, size)); + + t.Populate("settle", value, size); + + await RunIntegrityCheckAsync(t._cache, iteration); + } + #if NET9_0_OR_GREATER [Theory] [Repeat(soakIterations)] diff --git a/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs b/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs new file mode 100644 index 00000000..79a9c548 --- /dev/null +++ b/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs @@ -0,0 +1,153 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using BitFaster.Caching.Lfu; +using BitFaster.Caching.Scheduler; +using Castle.Core.Logging; + +namespace BitFaster.Caching.UnitTests.Lfu +{ + internal class WeightedTest + { + private const int DefaultMaxItems = 100000; + private const int DefaultMaxItemBytes = 2 * 1024 * 1024; // 2 MB per-item cap (telemetry: a 1 MB cap drops + // ~446K HOT fetches/day of 1-2 MB resources; 2 MB + // excludes only ~0.28%; the rare multi-MB tail is + // kept out by frequency eviction + the byte budget). + private const long DefaultMaxTotalBytes = 250L * 1024 * 1024; // 250 MB total resident budget + + private const int MinTrimBatch = 16; + private const int MaxTrimIterations = 64; + + private readonly int _maxItemBytes; + private readonly long _maxTotalBytes; + private long _currentBytes; + + private readonly ConcurrentDictionary _sizes = new ConcurrentDictionary(StringComparer.Ordinal); + + private int _reconcileInProgress; + + public readonly ConcurrentLfu _cache; + + internal WeightedTest(int maxItems, int maxItemBytes, long maxTotalBytes) + { + _maxItemBytes = maxItemBytes > 0 ? maxItemBytes : DefaultMaxItemBytes; + _maxTotalBytes = maxTotalBytes > 0 ? maxTotalBytes : DefaultMaxTotalBytes; + + // ForegroundScheduler runs maintenance (including the eviction the policy performs) inline on the calling + // thread instead of the thread pool, so the trim + size reconciliation below is deterministic. + _cache = new ConcurrentLfu( + Environment.ProcessorCount, + maxItems > 0 ? maxItems : DefaultMaxItems, + new ForegroundScheduler(), + EqualityComparer.Default); + } + + public void Populate(string key, string? value, int sizeBytes) + { + if (value is null || value.Length == 0 || sizeBytes > _maxItemBytes) + { + return; + } + + // A given key always maps to identical content (content hash, or org+id+version), so if it is already + // cached there is nothing to store — the TryGet here also bumps its frequency. + if (_cache.TryGet(key, out _)) + { + return; + } + + _cache.AddOrUpdate(key, value); + if (_sizes.TryAdd(key, value.Length)) + { + Interlocked.Add(ref _currentBytes, value.Length); + } + + this.EnforceByteBudget(); + } + + private void EnforceByteBudget() + { + // Single-writer gate: under the parallel retrieve fan-out many threads can Populate over-budget at once. + // Without this gate each would independently run the full trim loop (ProcessorCount x the work); here only + // the winner trims and reconciles, others return immediately without blocking. Any residual over-budget is + // picked up by the next Populate, so eventual convergence holds. + if (Interlocked.CompareExchange(ref _reconcileInProgress, 1, 0) != 0) + { + return; + } + + try + { + try + { + if (_cache.Policy.Eviction.HasValue) + { + IBoundedPolicy eviction = _cache.Policy.Eviction.Value!; + int iterations = 0; + bool trimmed = false; + + // Drive the trim loop off an ESTIMATED byte decrement (toTrim * avg) and reconcile the size map + // exactly ONCE after the loop settles — instead of the authoritative O(live-keys) reconcile after + // every Trim. That keeps the reconciliation cost off the inner loop (previously up to + // MaxTrimIterations reconciles per Populate); the final ReconcileSizes restores the exact total, + // and if the estimate left us marginally over budget the next Populate trims the remainder. + long estimatedBytes = Interlocked.Read(ref _currentBytes); + while (estimatedBytes > _maxTotalBytes && _cache.Count > 0 && iterations++ < MaxTrimIterations) + { + long over = estimatedBytes - _maxTotalBytes; + int entries = Math.Max(_sizes.Count, 1); + long avg = Math.Max(1, estimatedBytes / entries); + int toTrim = (int)Math.Min(_cache.Count, Math.Max(MinTrimBatch, (over / avg) + 1)); + + eviction.Trim(toTrim); + estimatedBytes -= toTrim * avg; + trimmed = true; + } + + // Reconcile only when we actually trimmed (i.e. were over budget). Reconciling on every Populate + // would race with concurrent adds — a just-added key may not yet be visible in _cache.Keys, so its + // bytes would be wrongly subtracted — undercounting resident bytes under budget. + if (trimmed) + { + this.ReconcileSizes(); + } + } + + // Catch silent capacity evictions that may have left stale size entries even while under budget. + if (_sizes.Count > _cache.Count) + { + this.ReconcileSizes(); + } + } +#pragma warning disable CA1031 // Cache maintenance is best-effort: never fail the retrieve that just populated. + catch (Exception ex) +#pragma warning restore CA1031 + { + throw; + //Logger.LogWarning(ex, "WebResourceContentCache.EnforceByteBudget threw; swallowing. Cache may temporarily exceed the byte budget."); + } + } + finally + { + Volatile.Write(ref _reconcileInProgress, 0); + } + } + + private void ReconcileSizes() + { + var live = new HashSet(_cache.Keys, StringComparer.Ordinal); + foreach (string key in _sizes.Keys) + { + if (!live.Contains(key) && _sizes.TryRemove(key, out int size)) + { + Interlocked.Add(ref _currentBytes, -(long)size); + } + } + } + } +} diff --git a/BitFaster.Caching/Lfu/ConcurrentLfuCore.cs b/BitFaster.Caching/Lfu/ConcurrentLfuCore.cs index 923cb8fa..9bb388a3 100644 --- a/BitFaster.Caching/Lfu/ConcurrentLfuCore.cs +++ b/BitFaster.Caching/Lfu/ConcurrentLfuCore.cs @@ -200,15 +200,15 @@ private void Trim(int itemCount, ItemRemovedReason reason) TakeCandidatesInLruOrder(this.probationLru, candidates, itemCount); TakeCandidatesInLruOrder(this.protectedLru, candidates, itemCount); TakeCandidatesInLruOrder(this.windowLru, candidates, itemCount); - } #if NET6_0_OR_GREATER foreach (var candidate in CollectionsMarshal.AsSpan(candidates)) #else - foreach (var candidate in candidates) + foreach (var candidate in candidates) #endif - { - Evict(candidate, reason); + { + Evict(candidate, reason); + } } } From 6e219135ce7ad26496b07571e3d1295c27fdb5a8 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Tue, 25 Aug 2026 19:07:07 -0700 Subject: [PATCH 2/5] cleanup --- .../Lfu/WeightedTest.cs | 29 ++----------------- 1 file changed, 2 insertions(+), 27 deletions(-) diff --git a/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs b/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs index 79a9c548..484e3c40 100644 --- a/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs +++ b/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs @@ -1,24 +1,17 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Linq; -using System.Text; using System.Threading; -using System.Threading.Tasks; using BitFaster.Caching.Lfu; using BitFaster.Caching.Scheduler; -using Castle.Core.Logging; namespace BitFaster.Caching.UnitTests.Lfu { internal class WeightedTest { private const int DefaultMaxItems = 100000; - private const int DefaultMaxItemBytes = 2 * 1024 * 1024; // 2 MB per-item cap (telemetry: a 1 MB cap drops - // ~446K HOT fetches/day of 1-2 MB resources; 2 MB - // excludes only ~0.28%; the rare multi-MB tail is - // kept out by frequency eviction + the byte budget). - private const long DefaultMaxTotalBytes = 250L * 1024 * 1024; // 250 MB total resident budget + private const int DefaultMaxItemBytes = 2 * 1024 * 1024; + private const long DefaultMaxTotalBytes = 250L * 1024 * 1024; private const int MinTrimBatch = 16; private const int MaxTrimIterations = 64; @@ -38,8 +31,6 @@ internal WeightedTest(int maxItems, int maxItemBytes, long maxTotalBytes) _maxItemBytes = maxItemBytes > 0 ? maxItemBytes : DefaultMaxItemBytes; _maxTotalBytes = maxTotalBytes > 0 ? maxTotalBytes : DefaultMaxTotalBytes; - // ForegroundScheduler runs maintenance (including the eviction the policy performs) inline on the calling - // thread instead of the thread pool, so the trim + size reconciliation below is deterministic. _cache = new ConcurrentLfu( Environment.ProcessorCount, maxItems > 0 ? maxItems : DefaultMaxItems, @@ -54,8 +45,6 @@ public void Populate(string key, string? value, int sizeBytes) return; } - // A given key always maps to identical content (content hash, or org+id+version), so if it is already - // cached there is nothing to store — the TryGet here also bumps its frequency. if (_cache.TryGet(key, out _)) { return; @@ -72,10 +61,6 @@ public void Populate(string key, string? value, int sizeBytes) private void EnforceByteBudget() { - // Single-writer gate: under the parallel retrieve fan-out many threads can Populate over-budget at once. - // Without this gate each would independently run the full trim loop (ProcessorCount x the work); here only - // the winner trims and reconciles, others return immediately without blocking. Any residual over-budget is - // picked up by the next Populate, so eventual convergence holds. if (Interlocked.CompareExchange(ref _reconcileInProgress, 1, 0) != 0) { return; @@ -91,11 +76,6 @@ private void EnforceByteBudget() int iterations = 0; bool trimmed = false; - // Drive the trim loop off an ESTIMATED byte decrement (toTrim * avg) and reconcile the size map - // exactly ONCE after the loop settles — instead of the authoritative O(live-keys) reconcile after - // every Trim. That keeps the reconciliation cost off the inner loop (previously up to - // MaxTrimIterations reconciles per Populate); the final ReconcileSizes restores the exact total, - // and if the estimate left us marginally over budget the next Populate trims the remainder. long estimatedBytes = Interlocked.Read(ref _currentBytes); while (estimatedBytes > _maxTotalBytes && _cache.Count > 0 && iterations++ < MaxTrimIterations) { @@ -109,16 +89,12 @@ private void EnforceByteBudget() trimmed = true; } - // Reconcile only when we actually trimmed (i.e. were over budget). Reconciling on every Populate - // would race with concurrent adds — a just-added key may not yet be visible in _cache.Keys, so its - // bytes would be wrongly subtracted — undercounting resident bytes under budget. if (trimmed) { this.ReconcileSizes(); } } - // Catch silent capacity evictions that may have left stale size entries even while under budget. if (_sizes.Count > _cache.Count) { this.ReconcileSizes(); @@ -129,7 +105,6 @@ private void EnforceByteBudget() #pragma warning restore CA1031 { throw; - //Logger.LogWarning(ex, "WebResourceContentCache.EnforceByteBudget threw; swallowing. Cache may temporarily exceed the byte budget."); } } finally From 8137bc291ee303ce3bc677d63758dfc00f43d619 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Tue, 25 Aug 2026 19:53:49 -0700 Subject: [PATCH 3/5] simplify test logic --- .../Lfu/ConcurrentLfuSoakTests.cs | 17 +-- .../Lfu/TrimmingLfuCache.cs | 74 ++++++++++ .../Lfu/WeightedTest.cs | 128 ------------------ 3 files changed, 81 insertions(+), 138 deletions(-) create mode 100644 BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs delete mode 100644 BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs diff --git a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs index 931cac4f..98978f3f 100644 --- a/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs +++ b/BitFaster.Caching.UnitTests/Lfu/ConcurrentLfuSoakTests.cs @@ -274,19 +274,16 @@ await Threaded.Run(threads, () => [Theory] [Repeat(soakIterations)] - public async Task Repro(int iteration) + public async Task WhenConcurrentTryGetAddOrUpdateAndTrimCacheEndsInConsistentState(int iteration) { - const int size = 100; - const long budget = 10_000; - string value = new string('x', size); + const long trimAfter = 100; + string value = "x"; - var t = new WeightedTest(maxItems: 1_000_000, maxItemBytes: 1000, maxTotalBytes: budget); + var trimmingCache = new TrimmingLfuCache(maxItems: 1_000_000, trimAfter: trimAfter); + Parallel.For(0, 5000, i => trimmingCache.AddWithTrim("x" + i, value)); + trimmingCache.AddWithTrim("y", value); - Parallel.For(0, 5000, i => t.Populate("k" + i, value, size)); - - t.Populate("settle", value, size); - - await RunIntegrityCheckAsync(t._cache, iteration); + await RunIntegrityCheckAsync(trimmingCache._cache, iteration); } #if NET9_0_OR_GREATER diff --git a/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs b/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs new file mode 100644 index 00000000..04135ae9 --- /dev/null +++ b/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs @@ -0,0 +1,74 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using BitFaster.Caching.Lfu; +using BitFaster.Caching.Scheduler; + +namespace BitFaster.Caching.UnitTests.Lfu +{ + // simple wrapper around ConcurrentLfu to trim the cache after a certain number of items have been added: repros reported bug + internal class TrimmingLfuCache + { + private const int MinTrimBatch = 16; + private const int MaxTrimIterations = 64; + + private readonly long trimAfter; + + private int trimInProgress; + + public readonly ConcurrentLfu _cache; + + internal TrimmingLfuCache(int maxItems, long trimAfter) + { + this.trimAfter = trimAfter; + + _cache = new ConcurrentLfu( + Environment.ProcessorCount, + maxItems , + new ForegroundScheduler(), + EqualityComparer.Default); + } + + public void AddWithTrim(string key, string? value) + { + if (_cache.TryGet(key, out _)) + { + return; + } + + _cache.AddOrUpdate(key, value); + + this.Trim(); + } + + private void Trim() + { + if (Interlocked.CompareExchange(ref trimInProgress, 1, 0) != 0) + { + return; + } + + try + { + if (_cache.Policy.Eviction.HasValue) + { + IBoundedPolicy eviction = _cache.Policy.Eviction.Value!; + int iterations = 0; + + long currentCount = _cache.Count; + while (currentCount > trimAfter && _cache.Count > 0 && iterations++ < MaxTrimIterations) + { + long over = currentCount - trimAfter; + int toTrim = (int)Math.Min(_cache.Count, Math.Max(MinTrimBatch, (over) + 1)); + + eviction.Trim(toTrim); + } + } + } + finally + { + Volatile.Write(ref trimInProgress, 0); + } + } + } +} diff --git a/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs b/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs deleted file mode 100644 index 484e3c40..00000000 --- a/BitFaster.Caching.UnitTests/Lfu/WeightedTest.cs +++ /dev/null @@ -1,128 +0,0 @@ -using System; -using System.Collections.Concurrent; -using System.Collections.Generic; -using System.Threading; -using BitFaster.Caching.Lfu; -using BitFaster.Caching.Scheduler; - -namespace BitFaster.Caching.UnitTests.Lfu -{ - internal class WeightedTest - { - private const int DefaultMaxItems = 100000; - private const int DefaultMaxItemBytes = 2 * 1024 * 1024; - private const long DefaultMaxTotalBytes = 250L * 1024 * 1024; - - private const int MinTrimBatch = 16; - private const int MaxTrimIterations = 64; - - private readonly int _maxItemBytes; - private readonly long _maxTotalBytes; - private long _currentBytes; - - private readonly ConcurrentDictionary _sizes = new ConcurrentDictionary(StringComparer.Ordinal); - - private int _reconcileInProgress; - - public readonly ConcurrentLfu _cache; - - internal WeightedTest(int maxItems, int maxItemBytes, long maxTotalBytes) - { - _maxItemBytes = maxItemBytes > 0 ? maxItemBytes : DefaultMaxItemBytes; - _maxTotalBytes = maxTotalBytes > 0 ? maxTotalBytes : DefaultMaxTotalBytes; - - _cache = new ConcurrentLfu( - Environment.ProcessorCount, - maxItems > 0 ? maxItems : DefaultMaxItems, - new ForegroundScheduler(), - EqualityComparer.Default); - } - - public void Populate(string key, string? value, int sizeBytes) - { - if (value is null || value.Length == 0 || sizeBytes > _maxItemBytes) - { - return; - } - - if (_cache.TryGet(key, out _)) - { - return; - } - - _cache.AddOrUpdate(key, value); - if (_sizes.TryAdd(key, value.Length)) - { - Interlocked.Add(ref _currentBytes, value.Length); - } - - this.EnforceByteBudget(); - } - - private void EnforceByteBudget() - { - if (Interlocked.CompareExchange(ref _reconcileInProgress, 1, 0) != 0) - { - return; - } - - try - { - try - { - if (_cache.Policy.Eviction.HasValue) - { - IBoundedPolicy eviction = _cache.Policy.Eviction.Value!; - int iterations = 0; - bool trimmed = false; - - long estimatedBytes = Interlocked.Read(ref _currentBytes); - while (estimatedBytes > _maxTotalBytes && _cache.Count > 0 && iterations++ < MaxTrimIterations) - { - long over = estimatedBytes - _maxTotalBytes; - int entries = Math.Max(_sizes.Count, 1); - long avg = Math.Max(1, estimatedBytes / entries); - int toTrim = (int)Math.Min(_cache.Count, Math.Max(MinTrimBatch, (over / avg) + 1)); - - eviction.Trim(toTrim); - estimatedBytes -= toTrim * avg; - trimmed = true; - } - - if (trimmed) - { - this.ReconcileSizes(); - } - } - - if (_sizes.Count > _cache.Count) - { - this.ReconcileSizes(); - } - } -#pragma warning disable CA1031 // Cache maintenance is best-effort: never fail the retrieve that just populated. - catch (Exception ex) -#pragma warning restore CA1031 - { - throw; - } - } - finally - { - Volatile.Write(ref _reconcileInProgress, 0); - } - } - - private void ReconcileSizes() - { - var live = new HashSet(_cache.Keys, StringComparer.Ordinal); - foreach (string key in _sizes.Keys) - { - if (!live.Contains(key) && _sizes.TryRemove(key, out int size)) - { - Interlocked.Add(ref _currentBytes, -(long)size); - } - } - } - } -} From 6737b025626f67d39a90eca853f87d27836e5d8b Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Tue, 25 Aug 2026 22:07:49 -0700 Subject: [PATCH 4/5] fix test code --- BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs b/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs index 04135ae9..53467441 100644 --- a/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs +++ b/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs @@ -56,12 +56,12 @@ private void Trim() int iterations = 0; long currentCount = _cache.Count; - while (currentCount > trimAfter && _cache.Count > 0 && iterations++ < MaxTrimIterations) + while (_cache.Count > trimAfter && currentCount > 0 && iterations++ < MaxTrimIterations) { long over = currentCount - trimAfter; - int toTrim = (int)Math.Min(_cache.Count, Math.Max(MinTrimBatch, (over) + 1)); - + int toTrim = (int)Math.Min(currentCount, Math.Max(MinTrimBatch, (over) + 1)); eviction.Trim(toTrim); + currentCount = _cache.Count; } } } From 11cee38c95923d36942cd170ba59af8822ea0366 Mon Sep 17 00:00:00 2001 From: Alex Peck Date: Tue, 25 Aug 2026 22:09:24 -0700 Subject: [PATCH 5/5] fix nullable --- BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs b/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs index 53467441..f06c4fef 100644 --- a/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs +++ b/BitFaster.Caching.UnitTests/Lfu/TrimmingLfuCache.cs @@ -29,7 +29,7 @@ internal TrimmingLfuCache(int maxItems, long trimAfter) EqualityComparer.Default); } - public void AddWithTrim(string key, string? value) + public void AddWithTrim(string key, string value) { if (_cache.TryGet(key, out _)) {