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. |
![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: 2f1c51e015
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4fbcd4c17
ℹ️ 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".
|
Sequential follow-up for Repositories #307/#327: inspected this PR at This existing implementation is the upstream provider repair for step 1 of FoundatioFx/Foundatio.Repositories#307 (comment). It is not being duplicated. Downstream consumption remains open: Repositories still references 13.0.4; no invented package version or release is being substituted. The migration-side heartbeat, lost-ownership behavior, and outstanding-request fencing are separate checks and are not certified by these provider tests. |
Repository migration dependency — sequential gate 1This PR is the first release gate for Foundatio.Repositories #307/#327. The latest commit Repositories must not consume this behavior until this PR is merged and released. Even after consumption, lease renewal is coordination—not storage fencing—so the migration PRs still separately need single-dispatch/task-lineage and stale-controller protections. I am holding the downstream sequence on this exact head's CI before moving to the next runtime gate. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f4d12392
ℹ️ 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 (entry.IsExpired) | ||
| { | ||
| if (((ICollection<KeyValuePair<string, CacheEntry>>)_memory).Remove(new KeyValuePair<string, CacheEntry>(key, replacement))) |
There was a problem hiding this comment.
When two renewals for the same lease overlap at the expiry boundary, this post-CAS cleanup can still leave the expired lease renewed: the first call installs replacement after the old entry expires and pauses before this check, while the second call observes that fresh replacement, successfully replaces it again, and returns true; the first call then detects the old expiry but cannot remove its no-longer-current replacement. The new post-CAS removal is fresh evidence that the previously reported boundary race remains possible, so expiry must be enforced as part of the mutation rather than by best-effort cleanup afterward.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #570 (d207f44). ReplaceIfEqualAsync now checks expiry inside the same UpdateEntry callback that publishes the change, after the value comparison, so expiry is part of the compare-and-swap rather than a cleanup step afterward. Both the overlapping-renewal case and the paused-comparison case are covered by RemoveIfEqualAsync_WhenEntryExpiresDuringComparison_ReturnsFalse.
| var replacement = CreateEntry(value, expiresAt); | ||
| if (replacement is null) | ||
| return false; |
There was a problem hiding this comment.
When the key is absent, expired, or contains a mismatched value, constructing the replacement here now clones the supplied value and runs its size calculator before the method checks whether replacement is permitted. Consequently an unclonable value, a throwing custom calculator, or an oversized value with ShouldThrowOnMaxEntrySizeExceeded can now throw where ReplaceIfEqualAsync previously returned false without inspecting the replacement; perform replacement construction only after the expected value has matched.
AGENTS.md reference: AGENTS.md:L162-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in #570. The replacement is built lazily, only after the expected value matches (and now only if the entry hasn't expired). Covered by ReplaceIfEqualAsync_WithMismatchedOversizedValue_ReturnsFalseWithoutThrowing.
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
|
Superseded by #570, which now includes this PR's lock-ownership fixes, rebuilt on #570's single
Both PRs rewrote the same |
1 participant
Summary
Related consistency dependency for FoundatioFx/Foundatio.Repositories#307 and #327. Current head:
c4fbcd4c173969b5560f88954c54ae62307f7a95.Three lease defects are addressed:
CacheLockProvider.RenewAsyncnow checks the compare-and-replace result and throwsLockExceptionwhen ownership was lost. Failed renewal does not increment the wrapper's renewal count.Another owner's lease is not renewed or removed, and normal successful renewal remains supported. Documentation identifies this intentional behavior correction and explicitly distinguishes leases from storage-enforced fencing.
Commits
2f1c51e0b2117439c4fbcd4cExecuted validation
Run 35795463344: initial implementation tests were 3 failed / 1 passed before repair; 425 lock/cache cases passed, none failed or skipped, afterward. Release build compiled .NET8/.NET10 libraries with zero warnings/errors at that revision.
Run 35799107410 generated the exact two follow-up commits:
git diff --checkpassed.10725336070contains the test/build logs, exact commit IDs, patch and bundle.The first CAS validation had 426 passes and one failure in the old wall-clock throttle test. Its trace straddled a two-second throttle window, so the test incorrectly expected the next call to be rejected. The clock-controlled replacement still verifies 25 accepted calls, rejection within the exhausted period, and acceptance after advancing to the next period.
No audit workflow, release configuration, dependency bump, or publishing change is in this PR. The branch was advanced without force-pushing. Final-head ordinary CI is separate from the above inspected candidate evidence.
Integration and remaining boundaries
Repositories still references released Foundatio 13.0.4. This PR must be merged/released and explicitly consumed before repository migrations gain the provider repair. Opening or updating this PR does not change consumers' installed package.
These changes do not revoke already-dispatched external operations, implement a migration fencing token, or certify every unrelated cache mutation race. Migration journal/takeover protections remain separate release gates on the repository PRs. Custom cache and lock providers must honor equivalent conditional ownership semantics.