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
| Id = message.Id, | ||
| TaskMessage = session.MessageData.TaskMessage, | ||
| LockedUntilUtc = message.OriginalQueueMessage.NextVisibleOn.Value.UtcDateTime, | ||
| DeliveryAttempt = message.OriginalQueueMessage.DequeueCount, |
There was a problem hiding this comment.
nit: I think ServiceBus also supports DequeueCount, but we aren't actively maintaining it AFAIK, so we aren't updating it in this PR. Correct me if I am wrong.
There was a problem hiding this comment.
Yeah, it does, but you are exactly right - it isn't being actively maintained so I decided to keep the scope of this PR to just Azure Storage for now
| // limitations under the License. | ||
| // ---------------------------------------------------------------------------------- | ||
| #nullable enable | ||
| namespace DurableTask.AzureStorage.Tests |
There was a problem hiding this comment.
I think we have basic unit test coverage.
Do we have integration testing support to actually fail an activity and validate DeliveryAttempt is increasing across multiple invocations? I have a sample scenario shared for the same, but that needs Azurite emulator.
Intent is to cover E2E integration test scenarios. Would like to understand what support DTF testing has.
There was a problem hiding this comment.
Thanks for the suggestion, I added some basic coverage confirming that the delivery attempt fields for both the dispatcher middleware and the TaskContext increment for a failed Activity in AzureStorageScenarioTests
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
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.