Skip to content

Add activity delivery attempt metadata - #1404

Open
sophiatev wants to merge 5 commits into
mainfrom
stevosyan/activity-delivery-count
Open

sophiatev wants to merge 5 commits into
mainfrom
stevosyan/activity-delivery-count

Conversation

@sophiatev

Copy link
Copy Markdown
Contributor

This PR adds new fields to the TaskActivityWorkItem, TaskContext, and WorkItemMetadata classes 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 the TaskScheduled event that is included in the TaskActivityWorkItem.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 19:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 DeliveryAttempt metadata 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.

Comment thread Test/DurableTask.AzureStorage.Tests/ActivityWorkItemTests.cs
Comment thread src/DurableTask.Core/TaskActivityWorkItem.cs
Copilot AI review requested due to automatic review settings September 17, 2026 19:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 lowercase test/ (DurableTask.sln:42), and there is no Test/**/*.csproj. Consequently this test is not compiled or executed, so the new Azure Storage delivery-attempt behavior has no active coverage; move the file to test/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.DeliveryCount and returns TaskActivityWorkItem instances without assigning it (ServiceBusOrchestrationService.cs:930-960). As a result, Service Bus activity middleware and TaskContext will report null even though the delivery attempt is available; populate DeliveryAttempt from receivedMessage.SystemProperties.DeliveryCount there 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,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sophia Tevosyan and others added 2 commits September 18, 2026 12:18
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 18, 2026 19:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.Tests tree. The repository's buildable project is test/DurableTask.AzureStorage.Tests/DurableTask.AzureStorage.Tests.csproj (and the solution references that path), while this new file is under uppercase Test/ 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 exposes receivedMessage.SystemProperties.DeliveryCount and does not assign it when constructing TaskActivityWorkItem. Consequently TaskContext.DeliveryAttempt and the middleware metadata are unavailable for Service Bus redeliveries; populate this field from DeliveryCount (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>
Copilot AI review requested due to automatic review settings September 18, 2026 19:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 lowercase test/ (and the solution references that tree), with no project under Test/. SDK compilation therefore excludes this file, so the only direct test of DequeueCount flowing into DeliveryAttempt never runs. Move it into test/DurableTask.AzureStorage.Tests or 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 its LockNextTaskActivityWorkItem initializer does not assign this new field. Consequently Service Bus middleware and TaskContext.DeliveryAttempt always see null even though the delivery count is available, making this generic metadata inconsistent across providers. Populate it from DeliveryCount and add a provider-level test.
        public long? DeliveryAttempt { get; set; }
  • Files reviewed: 9/9 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants