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. |
niemyjski
changed the title
…tries' into HEAD # 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. |
|
Merged the updated #570 ( |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…tries' into feature/reject-lost-lock-renewal
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…tries' into feature/reject-lost-lock-renewal
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ider errors, document breaking changes WorkItemJob no longer completes an entry when the work item lock was lost, even if the handler returns early or swallows the cancellation. Lock-set operations rethrow the definite LockException and log the provider errors they drop. Raises the 250ms lease in the multi-resource renewal tests, restores the InMemoryLockTests member order, and adds a breaking-changes callout to the locks guide.
…ect-lost-lock-renewal
…st lock - Attempt every renew/release in a lock set and prefer provider errors over LockException - Track acquired locks by position instead of name suffix and log cleanup failures on the null-return path - Cancel WorkItemContext.CancellationToken when automatic progress renewal loses the lock - Add regression tests and update lock/job docs
niemyjski
changed the title
![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: 562ba954e5
ℹ️ 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".
…ect-lost-lock-renewal
…' into feature/reject-lost-lock-renewal # Conflicts: # .agents/skills/foundatio/SKILL.md # docs/guide/jobs.md # docs/guide/locks.md # src/Foundatio/Jobs/WorkItemJob/WorkItemJob.cs # src/Foundatio/Lock/DisposableLockCollection.cs # src/Foundatio/Lock/ILockProvider.cs # src/Foundatio/Lock/LockSetOperations.cs # tests/Foundatio.Tests/Jobs/WorkItemJobTests.cs # tests/Foundatio.Tests/Locks/InMemoryLockTests.cs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e33ffb746b
ℹ️ 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: 0bc0f584a8
ℹ️ 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".
| logger.LogTrace("Unable to acquire all {LockCount} locks {UnacquiredResources} releasing acquired locks: {Resource}", unacquiredResources.Count, unacquiredResources, acquiredResources); | ||
| try | ||
| { | ||
| await LockSetOperations.RenewAllAsync(locksToRenew.Select(index => acquiredLocks[index].Lock), timeUntilExpires, logger).AnyContext(); |
There was a problem hiding this comment.
When earlier locks require renewal, the most recently acquired lock is excluded from the renewal set, but this await can itself outlast that lock's lease—for example, with a short lease or a provider retry lasting longer than the remaining duration. The method then returns a supposedly complete lock collection even though the newest resource has already expired and may have another owner; renew or revalidate the full acquired set before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a68d22e522
ℹ️ 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".
|
|
||
| Choose an interval shorter than the lease duration, leaving time for cache latency and retries. A pause can still let the lease expire before renewal detects the loss, and cancellation cannot revoke an operation already dispatched to another system. | ||
|
|
||
| `WorkItemJob` applies this pattern when a handler sets `AutoRenewLockOnProgress`. Each `ReportProgressAsync` call renews the queue entry and the work item lock. Renewal errors are logged and progress reporting continues. If the work item lock is lost (`LockException`), `WorkItemContext.CancellationToken` is cancelled, and the job abandons the queue entry for retry instead of completing it, whether the handler throws, returns early, or swallows the cancellation, and even if the job itself is cancelled afterward. A handler that ignores the token keeps running unprotected until it returns. A failed queue entry renewal is only logged: queues don't report a definite loss, and while the work item lock is held a redelivered entry can't be processed concurrently. |
There was a problem hiding this comment.
This safety claim does not hold for the default WorkItemHandlerBase: GetWorkItemLockAsync returns Disposable.EmptyLock, whose renewal is a no-op, so a failed queue-entry renewal can allow redelivery and concurrent processing while the original handler continues. Please state that this protection applies only when the handler overrides lock acquisition with a genuinely exclusive work-item lock; otherwise the new guidance can lead consumers to overlook duplicate execution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9760d3a8c3
ℹ️ 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".
|
|
||
| try | ||
| { | ||
| await Task.WhenAll(queueEntryRenewal, workItemLockRenewal).AnyContext(); |
There was a problem hiding this comment.
When workItemLockRenewal quickly faults with LockException but a remote queue provider's renewal is slow or stuck, Task.WhenAll prevents leaseLoss.SignalAsync() from running until that unrelated queue operation finishes. Concurrent work already started by the handler therefore continues without cancellation even though another worker may own the lease, potentially indefinitely because queue renewal has no cancellation token; signal the definite lease loss as soon as the work-item renewal settles, while awaiting/logging the queue task separately.
AGENTS.md reference: AGENTS.md:L97-L97
Useful? React with 👍 / 👎.
| return locker.TryUsingAsync("DoLockedWork", async () => await Task.Delay(500), TimeSpan.FromMinutes(1), TimeSpan.Zero); | ||
| } | ||
|
|
||
| public virtual async Task RenewAsync_AfterRelease_ThrowsLockExceptionAndDoesNotRecreateLock() |
There was a problem hiding this comment.
These new virtual tests have no xUnit attributes in the base class, and the inspected Foundatio.Redis RedisLockTests still overrides only the older methods. Consequently the lost-owner, invalid-duration, and partial-acquisition contracts introduced here never run against RedisCacheClient, so an in-memory-only pass cannot validate the provider whose conditional-write behavior underpins this change; add attributed overrides in that provider or otherwise arrange provider execution.
AGENTS.md reference: AGENTS.md:L181-L181
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Renewal could report success after ownership was lost, and multi-resource acquisition could return an incomplete set. Reject lost renewal, require every requested lease, and release partial acquisitions. Stacked on #570; include the corrections in #586 before merging.
Breaking runtime changes:
CacheLockProvider.RenewAsyncthrowsLockExceptionfor missing, expired, released, or replaced ownership. Stop protected work and observe renewal failures in an awaited task.ArgumentOutOfRangeExceptionbefore touching the cache. Multi-resource acquisition can encounter this validation when renewing earlier locks.nullfromTryAcquireAsync, or throwLockAcquisitionTimeoutExceptionfromAcquireAsync.ScopedLockProvider.RenewAsyncrequires the held lock's scopedILock.Resource; an unscoped name now fails instead of silently renewing nothing.WorkItemJobcancels its linked handler token when progress-triggered renewal reports work-item lease loss, and abandons the entry for retry even if the handler swallows cancellation.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.
The cache performance tradeoff in #582/#587 still needs acceptance. Non-Redis service-backed runtime coverage remains unverified.
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.Ownership and migration
A paused process cannot revive an expired lease by renewing after resuming. Acquire a new lease and revalidate the work. Ownership checks are not destination-enforced fencing: cancellation cannot revoke a write already sent.
These guarantees apply to
CacheLockProvider; empty and throttling locks have no-op renewal. Custom caches must implement atomic conditional replacement and treat expired entries as missing. Observe renewal errors in an awaited task; timer/async voidcallers must migrate their error handling.The automatic-renewal example cancels cooperative work, observes renewal failure, and awaits shutdown before disposing the lease. #586 uses nested
finallyblocks so a throwing cancellation callback cannot skip that wait. The source-extracted sample probe passes. In-flight renewal has no cancellation parameter, so configure provider timeouts.Queue-only renewal errors are logged and do not prove work-item lease loss. The default work-item handler uses
EmptyLock; exclusive protection requires overridingGetWorkItemLockAsync. Definite lease loss cancels immediately even while queue renewal is pending; progress reporting still observes both tasks. A recorded loss prevents completion even if the handler swallows cancellation.Acquisition and cleanup
LockExceptiontakes precedence over concurrent provider errors, which are logged.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.Shared contract methods need concrete provider test registration. Deterministic fake-clock races and work-item cancellation cases remain in their implementation-specific fixtures. Tests use AAA, alphabetical placement within groups, and disposed leases.
Review #586 into this branch, then retarget after #570 merges. Release with the cache corrections. Core hosted CI is green; this does not certify independent companion-provider CI against a published package.