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. |
…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".
Concurrent in-memory writes could overwrite newer values, lose list updates, or delete replacement entries. Updates now publish against the observed entry and preserve live values when rejected.
Compatibility and behavior changes:
ItemExpired; genuine expiration still does when reclaimed.Itemsreturns cached payloads with nullable annotations, which can affect warnings-as-errors builds. Unsupported list-dictionary comparers throwNotSupportedException.LockExceptionfor lost renewal, rejects durations below 5 ms before cache access, and cooperatively cancels/abandons work items when their exclusive work-item lock is lost.Validation (October 28): all reported parent/child CI checks are green. Final combined candidate: 2,517 passed / 22 skips; Redis: 778 passed / 18 skips, including all 42 required contracts. Source-provider build and binary API checks passed with the qualification below.
Hold: #598 fixes the flaky test and reduces sizing overhead, but compatible custom sizing at 10,000 items still costs 3.28× time / about 300× allocation versus main with cloning disabled. Provider builds still require an unpublished RabbitMQ dependency pin; durable Redis registrations and non-Redis service runtime coverage remain outstanding.
Verification and implementation details
Reviewable changes and merge order
Final source and verification
Main remains
be10785cfed4e1850350dfaa024e1c3f25b7fac7. Stack heads: #5700aa9d680, #58064a4fa6a, #582439e0d32, #5863a888094, #587f891a9b0, and #598988ab9ff. The isolated final combined source tree isbaef740a8b2b4390b9f125d845b93c3887a6e9ae.Foundatio.All.slnxRelease build passed with two existing ASPIRE010 warnings after the isolated RabbitMQ pin described below. All 14 test-assembly outputs used the candidate core DLL.The previous expiration failure was reproduced with an injected 200 ms pause in all seven core fixtures. #598 removes the upper timing deadline instead of relying on reruns. Twelve fake-time cases retain overlapping finite-expiration coverage. Allocation and unchanged-update regressions failed before their corresponding corrections.
Correctness and runtime contracts
The parent rejects conditional replacements that expire during preparation or retry, preserving a still-live lease instead of reporting false renewal success. Mismatching comparisons also recheck expiry before returning. Rejected writes can reclaim expired entries and notify earlier; live entries remain intact. Conditional removal retains task-based exception delivery.
The children enforce size limits on fallback estimates, reclaim sparse list storage, preserve mutable-struct cloning, and avoid eager allocation when sampling read-only keys/values. Snapshot collections can still share mutable elements with cloning disabled. Raw list expiration values may normalize to UTC when indexing; DateTime.Kind preservation is not promised. Concurrent clear/dispose/recount memory accounting remains approximate, as on main.
Lock renewal requires atomic conditional replacement from custom cache providers. Lost ownership requires protected work to stop; cooperative cancellation is not storage-enforced fencing. Default empty work-item locks provide no exclusive protection. Queue-only renewal errors remain logged. These lock/job behavior changes are isolated in #580/#586, not hidden inside the cache refactor.
Dependency and provider limits
#599 pins only the non-packable core AppHost sample to MessagePack 2.5.303. A fresh unpatched-main sample build emitted NU1902 as a warning; the patched sample builds with only ASPIRE010 and its transitive vulnerability audit reports no affected packages. The shipped serializer already uses 3.1.12. See GHSA-qhrr-8q5h-9q3h.
Unmodified RabbitMQ main
340357f1fails the all-provider build because its test AppHost resolves affected MessagePack 2.5.302 and treats NU1902 as an error. A separate one-line 2.5.303 test-AppHost pin makes the workspace build pass. That provider correction remains unpublished; the overlay is not proof that unmodified provider main builds. No audit suppression was used.Redis validation used main
06668733, the existing registration PR #191 patch, and six cache-contract overrides in each direct/scoped fixture. The additional registrations remain a test-only validation overlay, so provider-main/CI coverage is not yet durable. Non-Redis service runtime suites were not run.Performance and compatibility decision
BenchmarkDotNet 0.15.8, .NET 10.0.14 arm64, Apple M4 Max; one launch, three warmups, seven measured iterations. One operation adds/removes one item after warmup with a fixed clock. Main, prior #587 and #598 loaded in separate assembly contexts with verified hashes and identical bound-delegate calls. Twenty-four cases cover 1,000/10,000 items, cloning on/off, and compatible/read-only custom sizing. Cold-write and contention timing remain unmeasured in this pass.
#598 reduces 10,000-item compatible-sizing time by about 16% and allocation by about 3% relative to the previous stack in the same run. Concrete-dictionary compatibility still requires a private whole-list copy. Interface-based, non-mutating calculators can opt in with
SizeCalculator(calculateSize, supportsReadOnlyCollections: true); built-in fixed/dynamic sizing already supports that path. Changing the custom-calculator default would break callbacks that cast to Dictionary. No universal performance-neutrality claim.The original refactor was 1,875 → 1,484 lines; subsequent correctness work changed the count. No further decomposition is proposed. Hybrid concurrency remains tracked in #572. Vendored FastCloner and workflows remain unchanged.