Add poison message handling to Azure Storage

[sophiatev]

This PR introduces poison message handling to the Azure Storage. This is a very primitive implementation which simply stores a message in poison storage if its dequeue count exceeds the (user-configurable) maximum, and deletes it from the queue.

[sophiatev]

Unrelated to this PR but I bug I found when testing (JSON was not able to deserialize this event because it lacked a 0-arg constructor and the other constructors all had multiple parameters)

[sophiatev]

Also unrelated to this PR, but I realized while working on it that this code I wrote a while back had some incorrect assumptions so I took the opportunity to fix it

[Copilot]

Pull request overview

This PR adds an extensibility hook (IPoisonMessageHandler) and integrates poison/invalid message detection into the core dispatchers so that corrupted or “poisoned” inputs can be handled deterministically (e.g., fail orchestration/activity/entity work) instead of always throwing.

Changes:

• Introduces IPoisonMessageHandler and wires it into orchestration/activity/entity dispatchers for invalid work items and poison message detection.
• Adds structured logging support for poison-message detection (new event ID + event source + log event).
• Adds dispatch-count tracking on history events and propagates poison metadata through entity request processing.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/DurableTask.Core/Tracing/TraceHelper.cs	Adjusts entity invocation activity ending to better handle partial result sets.
src/DurableTask.Core/TaskOrchestrationDispatcher.cs	Adds poison detection/handling and updates reconciliation to return a drop reason.
src/DurableTask.Core/TaskEntityDispatcher.cs	Adds poison detection/handling for entity messages, plus poison-aware batching/result shaping.
src/DurableTask.Core/TaskActivityDispatcher.cs	Adds poison/invalid handling for activity scheduling messages (including failing poisoned tasks).
src/DurableTask.Core/Logging/StructuredEventSource.cs	Adds a new structured event for poison message detection.
src/DurableTask.Core/Logging/LogHelper.cs	Adds PoisonMessageDetected helper overloads emitting structured logs.
src/DurableTask.Core/Logging/LogEvents.cs	Adds a new structured log event type for poison messages.
src/DurableTask.Core/Logging/EventIds.cs	Reserves a new event ID for poison message detection.
src/DurableTask.Core/IPoisonMessageHandler.cs	New interface defining poison detection and handling hooks.
src/DurableTask.Core/History/HistoryEvent.cs	Adds DispatchCount to history events for poisoning heuristics/telemetry.
src/DurableTask.Core/History/ExecutionRewoundEvent.cs	Adds a parameterless ctor for JSON deserialization compatibility.
src/DurableTask.Core/Entities/OrchestrationEntityContext.cs	Adds AbandonAcquire() to reset lock acquisition state on failure.
src/DurableTask.Core/Entities/EventFormat/RequestMessage.cs	Adds poison metadata fields used during entity request processing.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

[cgillum]

Some initial comments. I haven't gone through the dispatcher code yet (those are bigger diffs).

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

Test/DurableTask.AzureStorage.Tests/PoisonMessageHandlingTests.cs:16

• This test file is added under Test/ (capital T), but the repository’s active test projects are under test/ and referenced from DurableTask.sln (e.g., test\\DurableTask.AzureStorage.Tests). As-is, these tests are unlikely to be built or run in CI, so the new poison-message behavior won’t be covered.

namespace DurableTask.AzureStorage.Tests
{
    using System;

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

Test/DurableTask.AzureStorage.Tests/PoisonMessageHandlingTests.cs:34

• This new test file is under Test/ (uppercase), but the repo’s active test projects are under test/ (lowercase) and referenced from DurableTask.sln. As a result, these tests won’t be compiled or executed in CI unless the file is moved into an included test project (e.g., test/DurableTask.AzureStorage.Tests/) and referenced by its .csproj.

    /// <summary>
    /// Integration tests for poison message handling in <see cref="AzureStorageOrchestrationService"/>.
    /// These tests require the Azure Storage emulator (Azurite) to be running.
    /// </summary>
    /// <remarks>

src/DurableTask.AzureStorage/Messaging/TaskHubQueue.cs:464

• If storing the poison blob succeeds but DeleteMessageAsync throws a 404 (message already deleted by a race), the catch logs an error and returns false, which can cause callers to treat the move as a failure even though the message is already gone. Consider treating a 404 from DeleteMessageAsync as success to avoid repeated retries/duplicate blobs and misleading errors.

                await this.storageQueue.DeleteMessageAsync(queueMessage, cancellationToken: cancellationToken);

[cgillum]

I haven't looked at the test cases yet, but here's some feedback based on the core implementation.

[cgillum]

I wonder if we should remove this property and just rely on MaxDequeueCount. The default value could be something really large, like 10K, which would represent roughly a month of retries and pretty much guarantees that customer doesn't care about this message. Having this enabled by default would allow many customers with existing poison messages to automatically self-heal.

[sophiatev]

We can remove it but I can also imagine that there might be customers which really do want to turn this off and not have to think about it. Maybe we should keep it and just leave it true by default? But I don't feel strongly about this at all, we can just remove it entirely if you prefer

[cgillum]

I'm fine with true by default as a compromise.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

Test/DurableTask.AzureStorage.Tests/PoisonMessageHandlingTests.cs:44

• This test file is under Test/ (uppercase), but the active test project referenced by DurableTask.sln is test/DurableTask.AzureStorage.Tests/DurableTask.AzureStorage.Tests.csproj, which won’t compile or run files in Test/. As a result, these poison-message tests won’t execute in CI. Move this file into test/DurableTask.AzureStorage.Tests/ (or update the .csproj to include ../../Test/DurableTask.AzureStorage.Tests/**/*.cs).

    [TestClass]
    public class PoisonMessageHandlingTests
    {
        static readonly TimeSpan DefaultTimeout = Debugger.IsAttached ? TimeSpan.FromMinutes(5) : TimeSpan.FromSeconds(60);

src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs:344

• PoisonMessageStorageContainerNamePrefix is user-configurable, but there’s no validation that the composed container name ({taskHub}-{prefix}-…) stays within Azure’s container naming constraints (<=63 chars, lowercase, hyphen rules). If the name is invalid/too long, poison handling will repeatedly fail at runtime (and messages will keep retrying) with only log spam. Consider validating these settings up-front when IsPoisonMessageStorageEnabled is true (e.g., in AzureStorageOrchestrationService.ValidateSettings) and throwing a clear configuration exception.

        /// The container name must adhere to the Azure Blob Storage container naming rules:
        /// https://learn.microsoft.com/en-us/rest/api/storageservices/Naming-and-Referencing-Containers--Blobs--and-Metadata#directory-names
        /// In particular, this means the total length of the name must not exceed 63 characters, it can only contain lowercase letters, numbers,
        /// and hyphens, and must start and end with a letter or number.
        /// Additionally, every hyphen must be immediately preceded and followed by a letter or number.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

Test/DurableTask.AzureStorage.Tests/PoisonMessageHandlingTests.cs:33

• This test file is under Test/, but the active test projects referenced by DurableTask.sln are under test/ (lowercase) and the test/DurableTask.AzureStorage.Tests project will not compile/run sources from Test/DurableTask.AzureStorage.Tests. As-is, these new poison-message tests won't execute in CI; move the file into test/DurableTask.AzureStorage.Tests (or update the test .csproj to include it).

    /// <summary>
    /// Integration tests for poison message handling in <see cref="AzureStorageOrchestrationService"/>.
    /// These tests require the Azure Storage emulator (Azurite) to be running.
    /// </summary>

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

Test/DurableTask.AzureStorage.Tests/PoisonMessageHandlingTests.cs:18

• This test file is under Test/ (capital T), but the solution references the AzureStorage test project under test/DurableTask.AzureStorage.Tests/ (see DurableTask.sln), and SDK-style projects only compile sources under their project directory by default. As-is, these tests likely won’t be built or executed in CI, leaving the new poison-message behavior effectively untested.

namespace DurableTask.AzureStorage.Tests
{
    using System;
    using System.Collections.Generic;
    using System.Diagnostics;

src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs:323

• Typo in XML doc: "messsage(s)" should be "message(s)" (and the sentence is missing "to" before "make progress").

        /// non-terminal state) if the messsage(s) necessary for them make progress are deemed "poisoned" and deleted.

src/DurableTask.AzureStorage/Messaging/TaskHubQueue.cs:464

• On failure to store a poison message, the MessageFailure log call drops the eventType/taskEventId context by passing empty/0, even though those values are available in this method.

                    string.Empty /* EventType */,
                    0 /* TaskEventId */,

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

Test/DurableTask.AzureStorage.Tests/PoisonMessageHandlingTests.cs:18

• This new test file is under Test/, but the solution and active test projects are under the lowercase test/ folder (e.g., DurableTask.AzureStorage.Tests is test\\DurableTask.AzureStorage.Tests\\DurableTask.AzureStorage.Tests.csproj in DurableTask.sln). With no Test/**/*.csproj in the repo, these tests won’t be compiled or executed in CI.

namespace DurableTask.AzureStorage.Tests
{
    using System;
    using System.Collections.Generic;
    using System.Diagnostics;

src/DurableTask.AzureStorage/AzureStorageOrchestrationServiceSettings.cs:323

• Grammar issue in the XML documentation: "make to progress" should be "to make progress".

        /// non-terminal state) if the message(s) necessary for them make to progress are deemed "poisoned" and deleted.