Conversation
Cache entries were mutable objects shared through the ConcurrentDictionary, and ten code paths changed a published entry in place. Maintenance checked IsExpired and then removed by key, so a concurrent refresh could make it delete a live entry, throw "Removed item was not expired", skip memory accounting and abort the expiration sweep. Published entries are now immutable: every change publishes a copy, and conditional removals use ConcurrentDictionary.TryRemove(KeyValuePair) so only the exact entry that was checked can be removed. This also fixes: - CompactAsync evicting a just-written entry (same remove-by-key flaw) - lost concurrent updates in Increment/SetIfHigher/SetIfLower/List* - RemoveIfEqualAsync/ListRemoveAsync faking deletes with a DateTime.MinValue tombstone and never releasing tracked memory - Items returning the internal CacheEntry instead of the cached value
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
ListAddAsync/ListRemoveAsync modified the dictionary returned by the entry's Value getter, which is the stored instance when CloneValues is off (the default), so concurrent adds wrote to the same Dictionary. They now copy the stored payload once and publish the copy. WithValue no longer clones; ReplaceIfEqualAsync publishes an entry built by CreateEntry, like SetAsync, so the caller's value is cloned once and sized outside the retry loop.
…g on races SetInternalAsync's AddOrUpdate value overload kept wasUpdated/oldSize from a discarded update-factory attempt when the key was removed and the add path won the retry. AddAsync then stored the key but returned false. Now that RemoveIfEqualAsync deletes immediately, a concurrent lock release makes this reachable, orphaning the lock key. Locals are now reset on every attempt. CompactAsync picks another candidate instead of stopping when its choice was replaced, bounded by maxRemovals.
Every read-modify-write now goes through UpdateEntry: read the entry, decide the next state, publish it with a compare-and-swap (add, replace or identity-conditional remove), and adjust memory size by the difference. Retries rerun a pure callback against the latest entry, so the per-attempt locals (wasUpdated, oldSize, difference, wasNewEntry) that caused the AddAsync bug are gone. - Collapse double/long SetIfHigher, SetIfLower and Increment into generic INumber<T> implementations - Collapse the string and generic ListAddAsync branches - Remove UpdateMemorySizeForEntry and the separate memory-tracking paths in SetInternalAsync - Move contract tests (concurrent add/release, increment, list add, SetIfHigher/SetIfLower) into CacheClientTestsBase so every provider runs them; skip the concurrent numeric ones on HybridCacheClient pending #572 - Restore the background maintenance path and comments in the time zone regression test - Add deterministic tests that force each race interleaving through the UpdateEntry callback, no clock hooks needed
…d list values - ReplaceIfEqualAsync builds the replacement only after the expected value matches, so a mismatch returns false instead of throwing MaxEntrySizeExceededCacheException (as on main) - ListAddAsync merges from the new entry's stored values, which CreateEntry already cloned, so CloneValues keeps the caller's objects out of the cache - Document that CacheEntry must keep reference equality for the compare-and-swap operations
Folds in the lock-ownership fixes from #573 on top of UpdateEntry: - ReplaceIfEqualAsync/RemoveIfEqualAsync treat an expired entry as a miss, checked after the value comparison so a lease that expires mid-comparison is not revived - CacheLockProvider.RenewAsync throws LockException when renewal fails instead of silently reporting success; documented on ILockProvider and ILock - Multi-resource TryAcquireAsync releases what it holds and returns null when an earlier lock is lost during acquisition - Clock-controlled throttle test, docs, and skill gotcha
niemyjski
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 589d686e4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return existingEntry; | ||
| // Check expiry after the comparison: a lease that expired while it was being compared no longer | ||
| // belongs to the caller. Maintenance removes the entry and raises ItemExpired. | ||
| return current.IsExpired ? (current, false) : (null, true); |
There was a problem hiding this comment.
When a matching entry expires before or during comparison, this branch deliberately retains it, but the method no longer calls StartMaintenanceAsync. Since maintenance is only started by later writes rather than by a persistent timer, expired locks conditionally released after their TTL can remain in the backing dictionary indefinitely, keep CurrentMemorySize inflated, and never raise ItemExpired if no unrelated write occurs. Schedule maintenance here or conditionally remove and notify for the observed expired entry while still returning false.
AGENTS.md reference: AGENTS.md:L322-L322
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This branch has not been deployed
1 participant
Concurrent in-memory cache writes could overwrite newer values, lose list updates, or remove a replacement entry. This fixes atomic publication and rejected-update accounting, and rejects renewal of lost cache-backed leases.
Compatibility changes:
CacheLockProviderthrowsLockException. Durations below 5 ms throwArgumentOutOfRangeExceptionwithout cache mutation.ItemExpired. Expired conditional operations return false; expired conditional removal reclaims memory and emits the genuine expiration event. Oversized replacements preserve value, TTL, and tracked size.Itemsreturns cached values with nullable annotations. Source warnings may change; CLR signatures remain compatible.RemoveIfEqualAsyncretains task-based exception delivery.Validation: Release core 2,183 passed, 22 skipped, 0 failed; aligned provider cache/lock selection 1,858 passed, 27 skipped, 0 failed; core/all-provider builds, docs, formatting, and net8/net10 core APICompat passed. Three existing cloner-crash cases and six Hybrid concurrency cases remain skipped. Provider tooling/registrations were adapted only in the isolated validation workspace; unrelated provider integration suites were not run. Both hosted builds and CLA passed at the new revision.
Performance needs reviewer acceptance. Historical 1,000-item default/fixed steady writes improve time but allocate 3.07×; opaque custom sizing still costs ~3× time/~40× allocation, dynamic sizing 1.40×/4.08×, first modification 4.58× time, and retained heap ~2.21×. Bulk/cloned contention also costs more. The secondary fix adds 64 B and ~12 ns to conditional-removal misses; matched set/remove cycles allocate 56 B less. This is not a blanket performance approval.
Suggested stack: cache correctness/performance, then dependent renewal behavior/docs/tests. Both remain here. FastCloner #58 is independent; its vendored source is unchanged.
Verification and implementation details
Correctness and compatibility
Candidate
1ca207a5a565c315a6e6aed16d54570a374cfa18against merge-base2b24e106b69af19514af9cc9b659127025a40525.Conditional updates/removals compare entry identities. Numeric/list size rejection preserves the prior value, expiration, and accounting in both throwing modes; successful higher/lower replacements use the validated size.
Lists preserve previous snapshots, stored dictionary types and comparers, and default non-cloning semantics for exposed dictionaries. Clone-enabled writes reuse existing values internally and clone added values once. Small lists retain dictionary storage; large eligible lists copy affected blocks and the block directory, sharing immutable index nodes. Bulk changes use one dictionary copy when rebuilding index paths would cost more.
Built-in fixed sizing is identified by its library-owned constant calculator; replacing it with a custom calculator disables the optimization. Arbitrary callbacks still receive ordinary dictionaries and retain full rejection validation.
FastCloner regression tests and the conservative cycle-analysis fix now live in upstream PR Update README.md #58. No FastCloner or update-script diff remains here. Only the three clone-enabled ConcurrentDictionary comparer cases skip pending that fix; other comparer/type cases still run.
InMemoryCacheClient.csremains one file: base 1,875 lines, original refactor 1,484, current revision 1,785. File splitting is out of scope.Renewal documentation is scoped to CacheLockProvider; throttling/empty locks have no-op renewal. The example observes renewal failures, cancels cooperative protected work, and awaits renewal shutdown before release. Ownership checks are not storage-enforced fencing. Automatic job cancellation remains outside this PR (Automatic lock renewal in QueueProcessorJobBase #26); WorkItemJob logs renewal failures and continues.
Maintenance samples the clock once and conditionally removes observed entries. ConcurrentDictionary enumeration is safe during writes; taking an array snapshot would not make subsequent removals atomic. The previous clock/lookahead heuristics predate immutable entry publication. Concurrent disposal is not an operation barrier; global memory accounting remains approximate under concurrency.
Secondary review fixed expired conditional-removal cleanup, including expiry during comparison and within the 250 ms maintenance throttle. Only the successfully removed expired entry raises
ItemExpired; a replacement owner remains intact. Seven new regressions failed before the correction.RemoveIfEqualAsyncretains its original async exception delivery: invalid arguments or comparison errors return a faulted task. Disabled maintenance trace logging no longer boxes its boolean or creates an argument array.Portable released/missing/replaced/scoped-owner and invalid-duration regressions live in
LockTestBase; concrete providers must register them. In-memory fake-clock tests retain deterministic expiry coverage. Newly acquired locks are disposed, renewal cases are alphabetical, and the extra blank line before disposal is removed. The multi-resource lost-lock warning now includes the exception message.Verification
Both hosted builds passed at
1ca207a5: PR build and push build; CLA passed.1ca207a5: full Release core suite 2,205 total, 2,183 passed, 22 skipped, zero failed. Existing six Hybrid skips remain under HybridCacheClient: concurrent Increment/SetIfHigher/SetIfLower can leave a stale local value #572; three cloner comparer/cycle skips remain pending upstream Update README.md #58.Performance verification
The list table below is historical evidence from
589d686eafter cloner separation: 26 Short-job base/candidate configurations, cloning on/off, six sizing/clone modes, and 10/1,000 items. The broader table is historical evidence from6a77e97b. These matrices were not rerun for the secondary-review commits; fresh conditional-removal measurements follow them.BenchmarkDotNet 0.15.8, MemoryDiagnoser, Apple M4 Max, macOS 27, .NET 10.0.12. Separate base/candidate builds; timing did not overlap task builds/tests. Benchmark process-priority elevation was unavailable, so measurements ran at normal priority. The earlier review included a 118-case Short comparison plus Default confirmation. The cloner-separation pass ran the 26 list configurations described above. Short figures are directional measurements, not production latency predictions.
Historical broader workload measurements (
6a77e97b, before removing the cloner patch):The base non-cloning contended-list case throws a collection-modified exception, so no valid ratio is reported. Candidate contention includes retry and scheduling costs; retry counts were not separately instrumented. Cloned base operations can still lose updates, so that baseline is not correctness-equivalent.
Approximate retained-heap probe (100 caches × 1,000 integers, full GC): base 83,032 bytes/cache initially and 83,158 after modification; candidate 150,875 and 183,437. This includes whole-cache overhead and is not a BenchmarkDotNet allocation measurement. The first small modification builds the index; this cost recurs after bulk rebuilding. Reads of raw dictionaries force copying on the next write.
The checked-in
InMemoryListBenchmarkscovers eight operations, two list sizes, cloning on/off, and none/fixed/custom/dynamic sizing. Reproduce representative measurements with the command above; compare separate base/candidate builds using identical runtime and inputs. Short-job numbers should not be treated as universal guarantees.Fresh secondary-review comparison: prior PR head
589d686eversus the candidate, separate before/after builds, BenchmarkDotNet 0.15.8/MemoryDiagnoser, .NET 10.0.12, Apple M4 Max. Frozen UTC keeps maintenance inside its throttle window to isolate hot-path costs; matched rows include setting then removing the value. Cloning uses the same record payload. Timing did not overlap builds/tests. Short runs are directional; the missing-entry case also ran a full Default confirmation.The Default missing-entry case is 1.24× slower and allocates 64 B more. Guarding disabled maintenance trace logging saves 56 B per call, benefiting matched set/remove allocation. These microbenchmarks do not measure provider I/O or production contention and do not establish a timing improvement for the Short matched cases.
Proposed stack boundary
Keep immutable cache publication, expiration comparisons, size accounting, list performance, cache tests, caching docs, and benchmarks together in #570. A dependent lock PR would contain CacheLockProvider renewal rejection, ILockProvider/ILock documentation and multi-resource handling, LockTestBase, InMemoryLockTests, the locks guide, and the renewal skill gotcha. The lease PR depends on atomic expired-entry conditional behavior. This review split does not require source-file decomposition. Automatic job cancellation remains outside the PR (#26); WorkItemJob still logs renewal failures and continues.