Repository navigation
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".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Updated 2026-10-06: the stack owner has already published these repairs in #570 at Source comparison confirms the published cache implementation is byte-for-byte identical to our repaired version: OrderedDictionary comparer preservation, explicit rejection of unsupported dictionary copies, expiry checks before conversion/comparison, retained post-comparison checks, and expiry rechecking after replacement construction. Live comparison exceptions retain faulted-task behavior. All added regressions remain; the published tests additionally assert TTL and memory retention after unsupported-dictionary rejection. #580 changes only the six renewal-related files relative to #570. Both build checks and CLA pass on each exact head: #570 build 1 / build 2; #580 build 1 / build 2. This update verifies source/test retention and hosted check results; it does not represent a new local test run or a new independent review of the published stack. Historical local validation (2026-10-05, Performance acceptance remains open, including the disclosed custom-sized large-list costs. Unsupported IDictionary mutation explicitly throws NotSupportedException when its comparer cannot be preserved. Existing skips and provider/performance coverage limits remain; green checks and source equality are not a zero-defect guarantee. This correction changes no workflow and does not resolve the separate pending publishing-guard approval. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48f5404ac6
ℹ️ 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".
…st compaction races spending the removal budget - UpdateEntry hides expired entries from callbacks, matching reads and Redis: Increment/SetIfHigher/SetIfLower/ListAdd start fresh, Replace/SetExpiration/ListRemove/RemoveIfEqual/ReplaceIfEqual miss, and SetExpirationAsync no longer revives an expired key. The displaced expired entry is reclaimed and raises ItemExpired once. - UpdateStoredEntry keeps the raw view for maintenance, compaction, RemoveAsync and RemoveExpiredKey - Add/Set/Replace share one write path; ReplaceAsync is now atomic instead of ContainsKey-then-Set - Compaction no longer charges a lost compare-and-swap to its removal budget (conflicts have their own cap)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…k when nobody subscribes - UpdateEntry runs its own compare-and-swap loop instead of wrapping UpdateStoredEntry in a capturing lambda, which added 96 B to every write. Both loops share TryPublish. - OnItemExpired returns before queueing a task when there are no handlers (the old null check could never be true) - Add InMemoryWriteBenchmarks for the scalar write paths
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
DoMaintenanceAsync_WithMaxTimeSpanExpiration_ShouldNotThrowException and DoMaintenanceAsync_WithPositiveTimezoneOffset_ShouldNotThrowOnDateTimeMinValue had been rewritten or replaced. Every test from main now exists unchanged; the stronger rewrite keeps its own name (DoMaintenanceAsync_WithMaxTimeSpanExpiration_KeepsEntryAfterTimeAdvances).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…, cache the disposal token, document behavior changes
…reference-holding structs, keep snapshot values owned Moves the conflict lock onto the single remaining update loop (eager stripes, mask index, System.Threading.Lock on net9+). Custom SizeCalculators receive a Dictionary copy again; only the built-in calculators read the read-only view. Struct items holding references are cloned under CloneValues. GetSnapshot always hands out a copy, so reads use the owned dictionary without layout checks. Skips publishing an unchanged list, avoids a discarded clone for new shareable lists, and adds Utc suffixes to new expiration names.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 351d2cba33
ℹ️ 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".
| if (current is null || update(current) is not { } decision) | ||
| return (current, missResult); |
There was a problem hiding this comment.
When an entry crosses its TTL during a mismatching comparison in RemoveIfEqualAsync or ReplaceIfEqualAsync, update(current) returns null, so this early return skips the post-comparison IsExpired check. If maintenance ran less than 250 ms earlier, the subsequent maintenance request is throttled; with no later writes, the expired entry remains in the dictionary, keeps tracked memory inflated, and never raises ItemExpired. The fresh evidence beyond the earlier matching-expiration report is this distinct mismatch path, which returns before the new reclaim logic; recheck expiration before returning the miss.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Concurrent in-memory writes could overwrite newer values, lose list updates, or delete a replacement entry. Publish only against the entry observed, and preserve the previous value, expiration, and tracked size when an update is rejected.
Behavior/source changes that can break consumers:
ItemExpiredwhen reclaimed; live conditional/list deletion is immediate and no longer raises that event.Itemsreturns cached payloads with nullable annotations, which can affect warnings-as-errors builds.NotSupportedExceptionwhen they cannot preserve an unsupported custom dictionary's comparer.Validation: combined Release 2,448 passed / 22 skipped / 0 failed; current provider source workspace build, Redis 777 passed / 18 skipped, repository tests 152 passed, net8/net10 API compatibility, and changed-file formatting passed. All five stack heads have green hosted Build workflows.
Performance decision remains: existing concrete-dictionary custom calculators still copy whole lists. At 1,000 items with cloning off, the compatibility path costs 3.27× time / 37× allocation versus main. #587 adds an explicit read-only sizing opt-in: 0.95 µs / 7.80 KiB, versus 7.04 µs / 85.57 KiB without it and 2.16 µs / 2.31 KiB on main. This removes sizing copies, not all immutable-update allocation. Accept this migration/cost tradeoff before merging the cache stack.
Verification and implementation details
Validated heads: #570
351d2cba, #5809760d3a8, #58211488067, #586e19ee7f3, #5873f24b90b; baseline mainbe10785c. Final combined core: 2,470 total, 2,448 passed, 22 skipped. Redis selection: 795 total, 777 passed, 18 skipped.The restore failures are resolved by the separate DataProtection update already on main (
be10785c): 8.0.32 resolves Xml 8.0.5 and Pkcs 8.0.2. Current provider revisions also resolve the old xUnit pins and Parsers cryptography advisory. No workflow edits or audit suppression.Correctness and compatibility
RemoveIfEqualAsyncretains task-based exception delivery. APICompat found no existing public CLR signature or parameter-name break; behavioral compatibility is separate.ConcurrentDictionaryand conditionally removes the observed entry. An array snapshot cannot prevent a later replacement race. Dispose still clears without becoming a barrier against subsequent operations; concurrent clear/dispose/recount accounting remains approximate, as on main.Verification limits
Ten current sibling repositories build with
ReferenceFoundatioSource=true; two existingASPIRE010warnings remain. Redis 8.10.2 uses the exact combined core DLL, with the 14 lock cases from Redis #191 and six extra shared cache registrations in the temporary validation checkout. These registrations are not a claim that provider main/CI discovers them. Six Hybrid and three FastCloner cloning skips remain. Non-Redis service-backed suites and the documentation-site build were not run.All five exact-head hosted workflows are green: #570, #580, #582, #586, #587.
Performance and scope
See #582/#587 for paired measurements, including small lists, 10,000-item lists, cloning both ways, and the compatible/default path. Scalar, compaction, bulk, and retained-heap matrices were not rerun in this pass. Cold updates and mutable-element cloning still require whole-list work. No universal performance-neutrality claim.
File-size history: the original refactor was 1,875 → 1,484 lines; this parent currently has 1,832 lines. #582 already extracts snapshots; no further decomposition is proposed. Expired-entry semantics originally discussed in #571 are included here. Hybrid concurrency remains under #572.