Repository navigation
Add activity delivery attempt metadata - #1404
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The Azure Storage test is outside the compiled test tree, and Service Bus does not populate the new delivery-attempt value.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds delivery-attempt metadata to activity work items, contexts, and middleware, with Azure Storage dequeue-count propagation.
Changes:
- Adds
DeliveryAttemptmetadata across core activity types. - Propagates the value through activity dispatch.
- Maps Azure Storage dequeue counts and adds tests.
File summaries
| File | Summary |
|---|---|
test/DurableTask.Core.Tests/DispatcherMiddlewareTests.cs |
Tests delivery-attempt propagation. |
Test/DurableTask.AzureStorage.Tests/ActivityWorkItemTests.cs |
Tests Azure Storage delivery counts; currently outside the compiled test tree. |
src/DurableTask.Core/WorkItemMetadata.cs |
Adds middleware delivery-attempt metadata. |
src/DurableTask.Core/TaskContext.cs |
Exposes delivery attempts to activities. |
src/DurableTask.Core/TaskActivityWorkItem.cs |
Defines activity delivery-attempt metadata. |
src/DurableTask.Core/TaskActivityDispatcher.cs |
Propagates metadata during dispatch. |
src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs |
Sets attempts from Azure Storage dequeue counts. |
Review details
- Files reviewed: 7/7 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.
There was a problem hiding this comment.
🔵 Needs a closer look
The Azure Storage test is inactive, and Service Bus delivery attempts are not populated.
Review details
Suppressed comments (2)
Test/DurableTask.AzureStorage.Tests/ActivityWorkItemTests.cs:24
- This test is under
Test/, but the repository's solution references the Azure Storage test project under lowercasetest/(DurableTask.sln:42), and there is noTest/**/*.csproj. Consequently this test is not compiled or executed, so the new Azure Storage delivery-attempt behavior has no active coverage; move the file totest/DurableTask.AzureStorage.Tests/ActivityWorkItemTests.cs.
public class ActivityWorkItemTests
src/DurableTask.Core/TaskActivityWorkItem.cs:47
- Only the Azure Storage provider populates this new value, but Service Bus already exposes the received message's one-based
SystemProperties.DeliveryCountand returnsTaskActivityWorkIteminstances without assigning it (ServiceBusOrchestrationService.cs:930-960). As a result, Service Bus activity middleware andTaskContextwill reportnulleven though the delivery attempt is available; populateDeliveryAttemptfromreceivedMessage.SystemProperties.DeliveryCountthere as well.
public long? DeliveryAttempt { get; set; }
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The queue test is not compiled or discovered, and Service Bus delivery attempts remain unpopulated.
Review details
Suppressed comments (2)
Test/DurableTask.AzureStorage.Tests/ActivityWorkItemTests.cs:27
- Please move this test into the active
test/DurableTask.AzureStorage.Teststree. The repository's buildable project istest/DurableTask.AzureStorage.Tests/DurableTask.AzureStorage.Tests.csproj(and the solution references that path), while this new file is under uppercaseTest/where no project file exists; it therefore will not be compiled or discovered, so the dequeue-count behavior is not covered in CI.
public async Task DeliveryAttemptComesFromDequeuedQueueMessage()
src/DurableTask.Core/TaskActivityWorkItem.cs:47
- This property is defined as the delivery attempt for every activity work item, but the Service Bus provider is left with
null: its receive path already exposesreceivedMessage.SystemProperties.DeliveryCountand does not assign it when constructingTaskActivityWorkItem. ConsequentlyTaskContext.DeliveryAttemptand the middleware metadata are unavailable for Service Bus redeliveries; populate this field fromDeliveryCount(and add provider coverage) or explicitly document that provider as unsupported.
public long? DeliveryAttempt { get; set; }
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Move the misplaced Azure Storage test into the active project and populate delivery attempts for Service Bus.
Review details
Suppressed comments (2)
Test/DurableTask.AzureStorage.Tests/ActivityWorkItemTests.cs:27
- This test is under
Test/, but the active Azure Storage test project is under lowercasetest/(and the solution references that tree), with no project underTest/. SDK compilation therefore excludes this file, so the only direct test ofDequeueCountflowing intoDeliveryAttemptnever runs. Move it intotest/DurableTask.AzureStorage.Testsor explicitly include it in the active project.
public async Task DeliveryAttemptComesFromDequeuedQueueMessage()
src/DurableTask.Core/TaskActivityWorkItem.cs:47
- Service Bus is another supported activity backend and already exposes
receivedMessage.SystemProperties.DeliveryCount, but itsLockNextTaskActivityWorkIteminitializer does not assign this new field. Consequently Service Bus middleware andTaskContext.DeliveryAttemptalways seenulleven though the delivery count is available, making this generic metadata inconsistent across providers. Populate it fromDeliveryCountand add a provider-level test.
public long? DeliveryAttempt { get; set; }
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Balasubramaniam B (bbala-github)
left a comment
There was a problem hiding this comment.
LGTM. Approving.
wangbill (YunchuWang)
left a comment
There was a problem hiding this comment.
LGTM. Reviewed delivery-attempt propagation from Azure Storage through activity middleware and TaskContext; no blocking issues found. Non-blocking cleanup: move ActivityWorkItemTests.cs from uppercase Test/ to lowercase test/ so it is included on case-sensitive filesystems.
This PR adds new fields to the
TaskActivityWorkItem,TaskContext, andWorkItemMetadataclasses to represent the delivery attempt number of the task activity work item. In the Azure Storage case this will just be the dequeue count of theTaskScheduledevent that is included in theTaskActivityWorkItem.