Repository navigation
Conversation
This was referenced Oct 13, 2026
niemyjski
marked this pull request as ready for review
October 13, 2026 04:39
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".
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #582. Preserve live cache entries when incoming values expire during preparation, reclaim expired stored entries through the atomic update path, and reduce unnecessary list-update allocation while preserving existing custom-calculator contracts.
Validation: Current head
143f24fepassed both hosted build/test runs. Local core validation atca9b5c9apassed Release build/formatting and 2,446 tests with 22 skips; the only subsequent source change is the sample-only advisory correction, withsrc/andtests/unchanged. Push CI and PR CI passed. The expired-entry review finding is resolved.Combined validation: 2,484 core tests and 778 Redis cache/lock tests passed; 22 core and 18 Redis existing skips; zero failures. Binary API checks passed for core net8/net10 and TestHarness net10. All-provider source build passed with an isolated RabbitMQ AppHost dependency pin; unmodified RabbitMQ main still fails its MessagePack advisory check.
Before merge/release: accept the documented custom-calculator performance tradeoff, retain durable provider test registrations, and resolve the RabbitMQ sample dependency in its own release work. Redis results do not establish non-Redis runtime coverage. The core sample advisory correction is included. Existing merge/release holds remain.
Verification and implementation details
Published head:
143f24fe11ebfc1be0f17cecea2df8c17353b14e; base:3907f54ab0d70a6acd0073c5121abd4f6358afcc. Fresh mainbe10785cfed4e1850350dfaa024e1c3f25b7fac7is included.The latest regression reproduced retained memory in all nine new cases before the fix. After routing expired preparations through
UpdateEntry, all 18 focused cases passed, including preservation of live stored entries. The integrated full suite ran 2,468 cases: 2,446 passed and 22 existing skips. That earlier local build retained Aspire and sample MessagePack warnings; the later sample-only correction pins patched MessagePack 2.5.303 and passed current hosted CI. The ordinary push workflow's CI-package publication step succeeded; release-package publication was skipped.Existing custom calculators still receive concrete dictionaries. The read-only overload/option is additive and 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 key/value enumeration have allocation regressions in the core suite.Historical paired measurements at 1,000 items with cloning disabled showed 3.27× time and 37× allocation versus main on the compatible custom-calculator path. These timings were not rerun at this head; the read-only opt-in avoids the dictionary copy but does not eliminate immutable-update allocation. This is an explicit compatibility/performance decision, not a universal performance claim.
Combined validation on 2026-10-23 used isolated core
70533664e7dfccf011fae4ddb5ef41c95dc68d63, containing #570d2c24efd, #58053ce936b, #5823907f54a, #586fffdf32fand this head. Its source tree is identical to the prepared combined candidate; only ancestry changed. No synthetic branch was pushed.be10785c: Foundatio net8/net10 and TestHarness net10 passed without suppressions.340357f1required an isolated AppHost MessagePack 2.5.303 pin; no audit suppression or provider publication occurred.06668733, registration PR Bump Microsoft.Extensions.Hosting from 3.1.1 to 3.1.2 #191ae79c527, and six shared cache-contract Fact overrides in each direct/scoped fixture. These test-only registrations are unpublished; provider-main discovery must not be inferred from the isolated run.Tests used a dedicated loopback-only Redis container, removed afterward. No non-Redis runtime coverage or new timing benchmarks are claimed.