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".
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 👍 / 👎.
…tries' into feature/pr580-oct23-refresh
…tries' into feature/pr580-oct23-refresh
…tries' into feature/pr580-oct27-refresh # Conflicts: # .agents/skills/foundatio/SKILL.md
Renewal previously reported success after ownership was lost, and multi-resource acquisition could return an incomplete set. This change rejects lost cache-backed renewal, requires every requested lease, and releases partial acquisitions. Stacked on #570; #586 contains required follow-up corrections that are not yet in this PR's head.
Breaking runtime changes:
CacheLockProvider.RenewAsyncthrowsLockExceptionfor missing, expired, released, or replaced ownership; supplied renewal durations below 5 ms throwArgumentOutOfRangeExceptionbefore cache mutation. Scoped renewal requires the held lock's scopedILock.ResourceandLockId. Multi-resource acquisition is all-or-nothing: unsuccessful sets returnnullfromTryAcquireAsyncor throwLockAcquisitionTimeoutExceptionfromAcquireAsync. Work-item lease loss cancels the linked handler token and prevents automatic completion even when the handler swallows cancellation.Validation at
64a4fa6a68e42beff1d583c6763bc29035bf690a: Hosted Release build and tests passed: 2,362 passed / 22 skipped / 0 failed. Local Release build, 163 conditional-cache tests, 47 lock tests, changed-file whitespace verification and diff checks passed. PR Build and push Build succeeded. The existing ASPIRE010 warning remains.Before merge/release: incorporate #586 through the reviewed stack merge, then rerun affected combined validation on the resulting head. Current #580 still delays lease-loss cancellation behind queue renewal and overstates queue-renewal protection for the default
EmptyLock; acquisition-latency handling also requires #586. Historical combined Redis execution and binary API compatibility passed for the October 23 revisions below; neither was rerun after the October 27 parent correction. Durable provider test registrations remain a separate release prerequisite; isolated registrations are not provider-main/CI coverage. The sample MessagePack advisory is addressed by the parent dependency update; no NU1902 is reported in this head’s CI log. Next-release and documented cache-performance acceptance remain separate release decisions; green CI does not close them.Verification, merge sequence and migration notes
Exact revisions
64a4fa6a68e42beff1d583c6763bc29035bf690a.0aa9d6804beefe88401eb7eddc0466afdc6d85ad, merged forward from previous Breaking: reject lost-lock renewal and incomplete lock acquisition #580 head53ce936b6ebd663901ee1095ad78cf0035142150. The parent rejects conditional replacements that expire during preparation or retry without replacing a still-live lease.be10785cfed4e1850350dfaa024e1c3f25b7fac7.fffdf32f9cd56dca0f2c350cc1d19042eb250c55, included in the combined validation below. Its corrections are still absent from this PR’s own head.Feedback disposition
The newest-lease-after-slow-renewal finding is satisfied here: acquisition checks every member after renewal, and
TryAcquireAsync_AfterSlowRenewal_ReturnsOnlyUnexpiredLeasescovers the expiry boundary and replacement owner. The immediate-cancellation and exclusive-lock documentation findings remain open until #586 is incorporated. The Redis execution finding is satisfied by actual discovery and passing runtime results against exact combined binaries. Durable provider-main/CI registration remains a separate release gate; no permanent registration is inferred from the isolated test checkout.Safe integration sequence
Consumer migration / release note
Observe renewal failures in an awaited task; stop cooperative protected work on
LockException. An expired handle cannot revive ownership: reacquire and revalidate the work. Replace timer orasync voidrenewal patterns that leave exceptions unobserved. Use renewal durations of at least 5 ms and use scoped identifiers from the held lock. Handle failed multi-resource acquisition as failure of the whole set. Partial cleanup attempts every release; failures are logged and unreleased leases expire naturally. Empty and throttling locks have no-op renewal and provide no renewable exclusive ownership. OverrideGetWorkItemLockAsyncwith a shared exclusive lock when work-item exclusion is required. Locks are not destination-enforced fencing, and cancellation cannot revoke writes already dispatched.Historical October 23 combined validation (separate from this PR head)
Combined core
70533664e7dfccf011fae4ddb5ef41c95dc68d63, treee3052914a241d1116fae694969d80b028fe2d912, includes #570d2c24efd, #58053ce936b, #5823907f54a, #586fffdf32f, and #587143f24fe, with main/API baselinebe10785c.be092d17combines main06668733, registration PR Bump Microsoft.Extensions.Hosting from 3.1.1 to 3.1.2 #191ae79c527, and test-only cache registrations. These isolated registrations remain unpublished; provider main/CI is not claimed to discover them automatically.The combined tree is an integration-validation artifact, not a change to this PR branch. Its passing tests do not resolve child-only source findings before #586 is actually integrated. This refresh reran the local and hosted checks listed above. No combined tree, provider runtime suite, API compatibility check or benchmark was rerun by this refresh.
October 27 parent evidence and retained limits
The parent owner reported a corrected combined core run with 2,493 passed / 22 skipped / 0 failed at tree
83733043e2a0b4cca7fca9b9a2a27ca4273f53d8. An initial run failed the existing timing-sensitiveScopedInMemoryCacheClientTests.ListAddAsync_WithExpiration_SetsExpirationCorrectly; the affected fixtures and full rerun passed. This separate evidence does not establish current provider runtime or API compatibility. The October 25 custom-sizing benchmark with cloning disabled recorded 3.28x time / 37x allocation at 1,000 entries and 6.70x / 318x at 10,000; performance acceptance remains open.Limits and reproduction
The hosted build retains ASPIRE010. The parent pins sample MessagePack 2.5.303; the current CI log has no NU1902 warning. This is not a claim that every repository dependency is advisory-free. Skips remain skips. The historical combined run establishes binary compatibility and Redis runtime verification only for its listed revisions, not the current head. No benchmark improvement or non-Redis runtime coverage is claimed.
dotnet build Foundatio.slnx -c Release --disable-build-servers -m:1 -p:UseSharedCompilation=false dotnet test --solution Foundatio.slnx -c Release --no-build git diff --checkUse
Foundatio.All.slnxfor combined source-workspace validation. The ordinary push workflow successfully published nine CI preview packages, version13.0.6-preview.reject-lost-lock-renewal.0.5, to GitHub Packages and Feedz; release publication was skipped.