Gate activity polling on app lease ownership - #1396
Draft
wangbill (YunchuWang) wants to merge 1 commit into
Draft
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| this.isLeaseOwner = false; | ||
| if (!calledFromLeaseRenewer && renewer != null) | ||
| { | ||
| renewerCancellation.Cancel(); |
Comment on lines
+523
to
+531
| catch (Exception ex) | ||
| { | ||
| this.settings.Logger.PartitionManagerError( | ||
| this.storageAccountName, | ||
| this.taskHub, | ||
| this.workerName, | ||
| this.appLeaseContainerName, | ||
| $"Failed to stop AppLeaseManager after losing the app lease. AppLeaseId: {this.appLeaseId} Exception: {ex}"); | ||
| } |
| AppLeaseManager newManager = CreateAppLeaseManager( | ||
| newSettings, | ||
| new TestPartitionManager()); | ||
| AppLeaseOwnershipSignal.AppLeaseOwnership oldOwnership = null; |
| newSettings, | ||
| new TestPartitionManager()); | ||
| AppLeaseOwnershipSignal.AppLeaseOwnership oldOwnership = null; | ||
| AppLeaseOwnershipSignal.AppLeaseOwnership newOwnership = null; |
| } | ||
|
|
||
| await this.partitionManager.StopAsync(); | ||
| CancellationTokenSource renewerCancellation = this.leaseRenewerCancellationTokenSource; |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
New tests are added under Test/ (not included in the solution/test projects) and there is a confirmed trace/telemetry lifecycle bug on the ownership-loss abandon path in activity dequeue.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR corrects UseAppLease=true behavior in the Azure Storage backend so activity polling/execution is gated by app-lease ownership, aligning it with existing orchestration/entity gating and enabling safe failover and same-AppName scale-out.
Changes:
- Introduces an ownership epoch/signal (
AppLeaseOwnershipSignal) and exposes it viaAppLeaseManager.WaitForOwnershipAsyncto coordinate lease ownership and cancellation. - Updates
AppLeaseManagerto reset ownership on lease loss/stop, add a forced-handoff transition fence, and improve shutdown behavior on renewer-triggered lease loss. - Gates
LockNextTaskActivityWorkItembehind lease ownership and abandons dequeued messages that arrive after an ownership loss.
File summaries
| File | Description |
|---|---|
| Test/DurableTask.AzureStorage.Tests/AppLeaseActivityTests.cs | Adds new tests for app-lease activity gating and failover behavior. |
| src/DurableTask.AzureStorage/Partitioning/AppLeaseOwnershipSignal.cs | Adds an ownership epoch + lost-token mechanism used to gate dispatch/polling. |
| src/DurableTask.AzureStorage/Partitioning/AppLeaseManager.cs | Wires ownership signaling into app-lease acquisition/renewal/stop, adds transition fencing, and handles lease-loss shutdown more defensively. |
| src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs | Updates UseAppLease doc comment to reflect orchestration/entity/activity gating and same-AppName parallelism. |
| src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs | Gates activity dequeue/dispatch on ownership; cancels outstanding receives on lease loss and abandons post-loss dequeues. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+14
to
+18
| namespace DurableTask.AzureStorage.Tests | ||
| { | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; |
Comment on lines
+1589
to
+1593
| if (!ownership.TryBeginDispatch()) | ||
| { | ||
| await this.workItemQueue.AbandonMessageAsync(message); | ||
| return null; | ||
| } |
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.
Summary
Fixes the runtime portion of Azure/azure-functions-durable-extension#1540 by making
UseAppLease=truegate activity work as well as orchestration and entity work.Behavior
Before: the app lease selected the app that processed orchestration/entity partitions, but every app sharing the task hub could still dequeue and execute activities from the work-item queue.
After: only workers whose logical
AppNameowns the app lease may begin new orchestration, entity, or activity work. Workers with the sameAppNamederive the same lease ID and continue competing on the shared activity queue in parallel, so activity scale-out within one logical app is preserved. Activities are not bound to the process or app that scheduled them.When app-lease loss is observed, the ownership epoch is reset and outstanding activity receives are canceled. A message returned after that epoch is lost is abandoned through the existing queue abstraction instead of being dispatched. Activities already returned to the dispatcher remain in flight and keep their normal visibility-lock renewal behavior.
Forced takeover publishes a transition fence and waits one renewal interval before the new app begins processing, allowing the old app's renewers to observe lease loss. Natural lease expiration and forced takeover both unblock polling in the new owner.
UseAppLease=falseis unchanged: all apps sharing the task hub may process all work types.Compatibility
This is an intentional breaking behavioral correction for the next major version. It reuses
AzureStorageOrchestrationServiceSettings.UseAppLeaseand does not add a compatibility flag.Tests
UseAppLease=false, forced and natural failover, forced-handoff fencing, receive cancellation/shutdown, post-dequeue ownership loss, in-flight lock renewal, and lease-manager restart after a stop failure.AppLeaseActivityTests: 10/10 passed onnet8.0and 10/10 passed onnet48.AzureStorageScaleTests: 52/52 passed onnet8.0.Required follow-up in Azure/azure-functions-durable-extension
DurableTask.AzureStoragedependency to the next major version containing this change.