Repository navigation
Clarify expired-value semantics for non-conditional cache operations #571
Description
Review the pre-existing behavior of non-conditional operations on expired entries, especially numeric updates and unconditional replacement. Define the intended contract before changing those semantics, and add shared provider tests where applicable.
PR #570 handles conditional-operation races, rejected-update accounting, lost-lock renewal, and the large-list update investigation. The reproduced cloner crash is handled separately in lofcz/FastCloner#58; #570 keeps its vendored cloner unchanged. Its performance results and remaining storage/custom-sizing tradeoffs belong in that PR; this issue is not a reason to defer or approve those changes.
File splitting and decomposition are out of scope. Keep InMemoryCacheClient in its current file.
Hybrid local-cache concurrency remains #572. Automatic job cancellation remains outside #570; #26 tracks the broader concern.
Activity
niemyjski commentedon Sep 22, 2026
Suggested first step from the #570 review: now that entries are immutable, replace the per-site
wasNewEntry/oldSize/newSizecaptured locals with oneUpsert(key, addEntry, Func<CacheEntry, CacheEntry> update)helper (TryGetValue -> TryUpdate/TryAdd loop) that computes the memory delta from the (previous, current) entries after the swap succeeds, with a matchingTryReplacefor ReplaceIfEqual/SetExpiration/ListRemove. That deletes the retry-sensitive locals at ~10 sites (the class of bug fixed in SetInternalAsync in #570) and makes the double/long dedupe trivial.[-]Refactor InMemoryCacheClient: dedupe numeric/list operations and split the file[/-][+]Refactor InMemoryCacheClient: split the file[/+]niemyjski commentedon Sep 22, 2026
The review of #570 also found a few pre-existing issues (they're on
maintoo) that fit with this cleanup:SetExpirationAsync,IncrementAsyncandSetIfHigherAsync/SetIfLowerAsyncon an entry that has expired but hasn't been swept yet act on the stale value instead of treating the key as missing.AddAsyncalready treats an expired entry as absent; the otherUpdateEntrycallbacks should do the same.ListAddAsyncstoresSize = -1when a list grows pastMaxEntrySizeand throwing is disabled (CalculateEntrySizereturns -1 in the update path).(
IncrementAsync(long)keeping a stale size when overwriting a non-numeric value was also reported. #570 already fixes it, because the shared generic implementation recalculates the size.)niemyjski commentedon Sep 24, 2026
PR #570 follow-up at f06ec7c fixes the small size-rejection/accounting defects while retaining this issue for broader decomposition and expired-value semantics. Correct file counts: 1,875 lines at the PR merge base, 1,484 after the original refactor, 1,486 after the targeted corrections.
Please also track allocation optimization here. A BenchmarkDotNet default-job comparison (MemoryDiagnoser, M4 Max, .NET 10) confirms a 1,000-item list add/remove cycle with cloning/sizing disabled changes from 2.111 to 6.332 μs (3.00×), and 2.02 to 80.08 KB (39.73×). Copying occurs even with CloneValues=false, and CAS retries may repeat it. With cloning enabled, the exploratory comparison instead reduces allocation from 158.08 to 80.34 KB by avoiding repeated deep cloning. Scalar increments allocate 432 → 560 B; expiration updates 56 → 176 B.
Optimize these costs without reintroducing mutable-entry publication, lost concurrent updates, or mutable list snapshots. The PR description contains the full validation scope and benchmark caveats. Hybrid concurrency stays in #572; automatic job cancellation stays outside this PR (#26).
[-]Refactor InMemoryCacheClient: split the file[/-][+]Review in-memory cache list allocation costs and expired-value semantics[/+][-]Review in-memory cache list allocation costs and expired-value semantics[/-][+]Clarify expired-value semantics for non-conditional cache operations[/+]