Repository navigation
Conversation
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f13dca9648
ℹ️ 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".
# Conflicts: # .agents/skills/foundatio/SKILL.md
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Stacked on #582. Preserve live cache entries when incoming values expire during preparation, reclaim expired stored entries atomically, and reduce list-update allocation while preserving custom-calculator contracts. The refreshed parent also rejects conditional replacements that expire before publication.
Validation: Release build/format passed; 1,922 cache tests passed (11 skips). Combined core: 2,493 passed (22 skips); fresh Redis: 778 passed (18 skips), including all 42 required contracts. Fresh binary API checks passed. Exact-head hosted CI: PR and push passed (2,455 tests, 22 skips each).
Before merge/release: performance acceptance and existing merge/release holds remain. Durable provider registrations and the RabbitMQ sample dependency correction remain separate work. No fresh non-Redis runtime coverage is claimed. The first combined run hit an existing short-expiration timing failure; focused and full reruns passed, but the timing weakness remains.
Verification and implementation details
Published head:
f891a9b0808cdea5585ce393b78be1827b78c6ed; exact incorporated #582 parent:439e0d328abb2fb56d26df34decee5ec4e385521. Mainbe10785cfed4e1850350dfaa024e1c3f25b7fac7is included. The refresh preserves the prior child fixes and merges the parent's tested expired-replacement correction. Only adjacent skill documentation conflicted; both guarantees were retained.Final-head Release build passed with one existing ASPIRE010 warning. Cache execution: 1,933 cases, 1,922 passed, 11 existing skips, zero failures/errors. All nine inherited conditional-replacement regressions and all nine expired-stored-entry reclamation cases passed. Changed-file formatting and whitespace checks passed. The reclamation fix previously reproduced retained memory in all nine cases before routing expired preparations through the atomic update path.
Existing custom calculators still receive concrete dictionaries. The additive read-only option requires support for ordinary dictionaries and immutable read-only views. Oversized fallback estimates obey
MaxEntrySize. Cloned reads preserve incremental list writes; uncloned raw reads retain existing shared-value behavior. Sparse storage and lazy enumeration have allocation regression coverage.Historical October 25 measurements for the compatible custom-calculator path, cloning disabled: 3.28x time/37x allocation versus main at 1,000 items; 6.70x/318x at 10,000 items. These timings were not rerun because this refresh leaves list/sizing code unchanged. The read-only option avoids the dictionary copy but does not eliminate immutable-update allocation. This remains an explicit compatibility/performance decision.
Combined validation on October 28 incorporates #570
0aa9d680, #58064a4fa6a, #582439e0d32, #5863a888094and this head. The resulting source tree83733043e2a0b4cca7fca9b9a2a27ca4273f53d8exactly matches the retained October 27 combined build/test inputs. Its Release build and full-suite results were reused rather than duplicated; no synthetic branch was pushed.ScopedInMemoryCacheClientTests.ListAddAsync_WithExpiration_SetsExpirationCorrectly, whose short wall-clock expiry window elapsed before the live assertion. It passed in all seven fixtures on focused rerun. The actual CacheLockProvider probe also preserved the live value/TTL/size, threw LockException and kept RenewalCount at zero.06668733, existing registration PR Bump Microsoft.Extensions.Hosting from 3.1.1 to 3.1.2 #191ae79c527patch, and six shared cache-contract Fact overrides in each direct/scoped fixture. These are isolated test overlays, not provider-main registrations. Source-reference build passed with zero warnings/errors. Dedicated loopback-only Redis was removed afterward.Both hosted runs passed at the exact published head: 2,477 total cases, 2,455 passed, 22 skipped and zero failed. The ordinary push workflow published CI packages successfully; release-package publication was skipped. No manual release or main merge occurred.