Fix RewindAsync non-determinism when the failed step is not the last executed step

[wangbill (YunchuWang)]

Fixes Azure/azure-functions-durable-extension#444

Problem

RewindAsync fails with Non-Deterministic workflow detected whenever the failed step was not the last thing the orchestrator did before failing. The most common shape is a try/catch where the catch block schedules something (a cleanup activity, a sub-orchestration, an external event) before rethrowing:

try
{
    await context.ScheduleTask<string>(nameof(Foo), "");   // fails
}
catch (Exception)
{
    await context.ScheduleTask<string>(nameof(Cleanup), ""); // <-- scheduled *because* of the failure
    throw;
}

This was confirmed by Katy Shimizu (@kashimiz) back in 2018 ("the rewind process's cleanup phase fails to scrub the history events of the first FN.DispatchSignalREvent ... the failed step in the orchestrator must be the last step executed before the orchestrator itself fails. This is due to a logical oversight in our implementation") and reactivated by Chris Gillum (@cgillum) "so that we don't forget to actually fix this."

Root cause

ProcessRewindOrchestrationDecision rebuilt the history by filtering only on event type + failed task IDs:

if (!(evt is TaskScheduledEvent ts && failedTaskIds.Contains(ts.EventId))
    && evt is not TaskFailedEvent
    && evt is not SubOrchestrationInstanceFailedEvent
    && evt is not ExecutionCompletedEvent)

That correctly removes the failed task, but it retains every event the orchestrator scheduled as a consequence of observing that failure. After rewind the failure is no longer visible in the history, so on replay the orchestrator takes the success path and blocks awaiting the re-scheduled activity. It never reaches the sequence ID of the leftover TaskScheduledEvent, and TaskOrchestrationContext.HandleTaskScheduledEvent throws NonDeterministicOrchestrationException.

This is exactly why the failure only shows up when the failed step isn't the last one — if nothing was scheduled after the failure, there is nothing left over to trip on.

Fix

Make the scrub episode-aware. History is divided into episodes delimited by OrchestratorStartedEvent; an episode boundary is precisely where the orchestrator observed new results and reacted to them.

1. Collect failedTaskIds and failureEpisode = the earliest episode in which any failure is delivered.
2. Collect consequenceTaskIds = sequence IDs of everything scheduled at episode >= failureEpisode.
3. Rebuild, dropping the failed/consequence scheduling events and their corresponding result events.

Dropping the result events matters: a stale result left behind could otherwise satisfy a different task that later gets assigned the same sequence ID.

Two details that are easy to miss and are handled here:

• Replay matches the orchestrator's sequence-ID counter for four event types, not just task scheduling: TaskScheduledEvent, SubOrchestrationInstanceCreatedEvent, TimerCreatedEvent, EventSentEvent. All four are scrubbed.
• RetryInterceptor always creates a delay timer after the final failed attempt, so ScheduleWithRetry leaves behind a TimerCreatedEvent that is itself a consequence of the failure. Without removing it, rewinding a retried activity produces the timer variant of the same error (scheduled a timer task with sequence number 1 ...).

The same rule is applied to the Azure Storage rewind path in AzureTableTrackingStore.RewindHistoryAsync, which is what AzureStorageOrchestrationService.RewindTaskOrchestrationAsync actually calls.

Behaviors deliberately preserved

• Fan-out/fan-in: parallel branches are all scheduled in an episode before the failure is observed, so they are retained and not re-executed.
• Failed sub-orchestrations: the SubOrchestrationInstanceCreatedEvent precedes the episode that delivers the failure, so it is retained and the child rewind message is still emitted.

Known tradeoff

Within the failure episode the scrub errs on the side of removing too much. Without re-running orchestrator code there is no way to distinguish "scheduled because of the failure" from "unrelated work that happened to be batched into the same episode". The cost is that a small number of successful tasks may be re-executed on rewind; the benefit is a history that always replays. Given that rewind is an explicit, manual recovery operation on an already-failed instance, and that the alternative is rewind failing outright, this is the right trade. Activities should already be idempotent for rewind to be meaningful at all.

Note for other backends

ProcessRewindOrchestrationDecision is not the only implementation of this scrub — some backends (notably the Durable Task Scheduler) replicate it server-side. The WARNING comment above the method has been expanded into an explicit contract describing the rule so those implementations can be kept in sync. They will still exhibit this bug until updated.

Tests

New Test/DurableTask.Core.Tests/RewindTests.cs (8 tests) drives real orchestrations through real episodes, rewinds, and replays the result.

Regression tests (fail without the fix):

• Rewind_CatchBlockSchedulesActivity_ProducesReplayableHistory
• Rewind_CatchBlockCreatesSubOrchestration_ProducesReplayableHistory
• Rewind_CatchBlockSendsEvent_ProducesReplayableHistory
• Rewind_ScheduleWithRetry_RemovesRetryTimers

Guard tests (protect existing behavior):

• Rewind_SimpleActivityFailure_ReschedulesOnlyTheFailedActivity
• Rewind_FanOutFanIn_RetainsSuccessfulBranches
• Rewind_FailedSubOrchestration_RetainsCreationAndEmitsChildRewindMessage
• Rewind_AssignsNewExecutionId

Two end-to-end tests in AzureStorageScenarioTests.cs cover the Azure Storage path against real storage: RewindActivityFailWithCleanupActivity and RewindActivityFailWithRetry.

Verification

Both halves of the change were validated with A/B controls rather than by assuming the tests are meaningful.

Check	Result
New Core tests, net8.0 + net48	8/8 pass
Core tests with the new logic surgically neutralized	4 fail — the regression tests genuinely catch the bug
Full Core suite, net8.0	147/147 pass
E2E tests with AzureTableTrackingStore reverted to pre-fix	both fail with the exact issue #444 message
E2E tests with the fix	both pass

The pre-fix E2E failures reproduce the original 2018 report verbatim:

Non-Deterministic workflow detected: A previous execution of this orchestration scheduled
an activity task with sequence ID 1 and name '...Hello' (version ''), but the current replay
execution hasn't (yet?) scheduled this task.

and, for the retry variant:

Non-Deterministic workflow detected: A previous execution of this orchestration scheduled
a timer task with sequence number 1 but the current replay execution hasn't (yet?) scheduled this task.

Note: net48 has 7 pre-existing failures in the Core suite (all TraceHelper_* / Dispatcher_RestartsTraceActivity_ForContinueAsNewStartNewTrace). These were confirmed against a baseline with this change removed (7/127 before vs 7/135 after) and are unrelated to rewind.

[Copilot]

Pull request overview

This PR fixes RewindAsync non-determinism by making the rewind scrub “episode-aware”, ensuring that any work scheduled after a failure is first observed (and the corresponding result events) is removed so the rewound history can always replay deterministically. It applies the same rule both in the SDK-layer scrub (TaskOrchestrationDispatcher.ProcessRewindOrchestrationDecision) and the Azure Storage backend scrub (AzureTableTrackingStore.RewindHistoryAsync), and adds regression/E2E coverage for the previously failing patterns (cleanup work in catch blocks, retry timers, etc.).

Changes:

• Update Core rewind history scrubbing to remove failure-consequence scheduled events (TaskScheduled/SubOrchestrationCreated/TimerCreated/EventSent) and their results, based on episode boundaries.
• Update Azure Table rewind scrubbing to match the same episode-aware rule over stored history entities.
• Add new rewind regression tests (Core) and new Azure Storage end-to-end rewind scenarios.

Reviewed changes

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

File	Description
Test/DurableTask.Core.Tests/RewindTests.cs	Adds new Core rewind tests to validate replayability after episode-aware scrubbing (note: currently placed under Test/).
test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs	Adds two E2E Azure Storage rewind tests plus supporting orchestrations/activities for cleanup + retry cases.
src/DurableTask.Core/TaskOrchestrationDispatcher.cs	Implements episode-aware rewind scrub and expands the contract comment to keep backend implementations in sync.
src/DurableTask.AzureStorage/Tracking/AzureTableTrackingStore.cs	Implements the equivalent episode-aware scrub for Azure Table history entities.

Suppressed comments (1)

test/DurableTask.AzureStorage.Tests/AzureStorageScenarioTests.cs:1491

• Same issue as the cleanup test: if an assertion throws, HelloFailRetryActivity.ShouldFail may remain flipped and host.StopAsync() won't run, which can impact subsequent tests. A try/finally ensures both the flag and the host lifecycle are always reset.

                Activities.HelloFailRetryActivity.ShouldFail = true;
                await host.StartAsync();

                string singletonInstanceId = $"Test_{Guid.NewGuid():N}";

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[wangbill (YunchuWang)]

Fixed in 0759af8 — moved to test/DurableTask.Core.Tests/RewindTests.cs. Git recorded it as a rename ({Test => test}/DurableTask.Core.Tests/RewindTests.cs), so the file now sits next to the .csproj.

One correction to the reasoning, for the record: the tests were compiled and executed in CI. eng/ci/public-build.yml runs on 1es-windows-2022 (os: windows), and NTFS is case-insensitive, so Test\ and test\ resolve to the same physical directory and the SDK glob picked the file up. I verified this locally — the tests ran and the negative controls behaved as expected.

That said, the underlying defect is real and worth fixing: on any case-sensitive filesystem the file would land in a separate directory and be silently dropped from the build. Silent is the dangerous part — the suite would still go green, just with less coverage.

While fixing this I noticed the repo already has pre-existing strays in the same shape, which is presumably how I tripped over it:

Test/DurableTask.Core.Tests/ScheduleTaskOptionsTests.cs
Test/DurableTask.Core.Tests/TaskOrchestrationContextTests.cs
Test/DurableTask.Core.Tests/TraceHelperTests.cs
Test/DurableTask.Core.Tests/VersionSettingsTests.cs
Test/DurableTask.AzureStorage.Tests/OrchestrationSessionTests.cs
Test/DurableTask.AzureStorage.Tests/QueueClientEncodingStrategyIntegrationTests.cs
Test/DurableTask.AzureStorage.Tests/Storage/DurableTaskStorageExceptionTests.cs

For comparison, test/DurableTask.Core.Tests/ (lowercase) holds 21 files including the project file, so lowercase is clearly the intended location. I've deliberately left those 7 alone to keep this PR scoped to the rewind fix — happy to send a separate cleanup PR if you'd like.

[wangbill (YunchuWang)]

Good catch — fixed in 0759af8, and the suggestion turned out to be more valuable than a cosmetic cleanup.

Two changes:

1. The hard-coded 3 is now read back from the pre-rewind history, so the assertion tracks the cleanup activity rather than a fixed sequence ID:

int cleanupTaskId = result.HistoryAtFailure
    .OfType<TaskScheduledEvent>()
    .Single(e => e.Name == "Cleanup")
    .EventId;

2. Added the general invariant you suggested as AssertNoOrphanedResultEvents, called from AssertReplayable so every test in the file gets it. It covers all four scheduling/result pairs: TaskScheduled←TaskCompleted/TaskFailed, SubOrchestrationInstanceCreated←SubOrchestrationInstanceCompleted/Failed, and TimerCreated←TimerFired.

To check the new assertion wasn't just decoration, I neutralized only the result-event removal in the rebuild loop (line 1526, TryGetCompletedTaskId(...) && consequenceTaskIds.Contains(...)) and left the rest of the fix intact. Result:

• The replay assertion passed — no NonDeterministicOrchestrationException at all.
• Three tests failed on the new orphan check:

Found TaskCompletedEvent event(s) with no matching TaskScheduledEvent
  in the rewound history (sequence ID(s): 3)
Found TimerFiredEvent event(s) with no matching TimerCreatedEvent
  in the rewound history (sequence ID(s): 1, 3)
Found SubOrchestrationInstanceCompletedEvent event(s) with no matching
  SubOrchestrationInstanceCreatedEvent in the rewound history (sequence ID(s): 1)

So orphaned result events are a failure mode the replay assertion cannot observe on its own — the history replays cleanly and the danger only materialises later, when a stale result silently satisfies a different task assigned the same sequence ID. The old assertion would have caught one of those three, and only because 3 happened to be right.

Full suite after the change: 8/8 rewind tests pass on net8.0 and net48; Core suite 147/147 on net8.0.

[wangbill (YunchuWang)]

Agreed — fixed in 0759af8 for both tests (the one flagged here and the RewindActivityFailWithRetry one from the suppressed comment).

Both now capture the flag's original value and restore it in a finally that also stops the host:

bool originalShouldFail = Activities.HelloFailCleanupActivity.ShouldFail;
try
{
    Activities.HelloFailCleanupActivity.ShouldFail = true;
    ...
}
finally
{
    Activities.HelloFailCleanupActivity.ShouldFail = originalShouldFail;
    await host.StopAsync();
}

Worth noting that using alone was not sufficient here: TestOrchestrationHost.Dispose() only calls this.worker.Dispose() and never StopAsync(), so the graceful shutdown really was being skipped on an assertion failure rather than merely deferred.

Verified by running all nine Rewind* tests in AzureStorageScenarioTests together against Azurite — 9/9 pass, so the new tests don't leak state into the existing RewindActivityFail* neighbours that share this pattern.

[Copilot]

Pull request overview

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

Suppressed comments (1)

Test/DurableTask.Core.Tests/RewindTests.cs:14

• These regression tests are added under uppercase Test/, but the active SDK project and DurableTask.sln reference lowercase test/DurableTask.Core.Tests (DurableTask.Core.Tests.csproj:1-28, DurableTask.sln:26). On case-sensitive checkouts this file is outside the project directory, so none of these eight tests are compiled or run. Move it to test/DurableTask.Core.Tests/RewindTests.cs.

namespace DurableTask.Core.Tests

[Copilot]

Pull request overview

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

[wangbill (YunchuWang)]

Good catch — fixed in 99f3d05.

You are right that IsScheduledEventType has four branches but the end-to-end tests only drove two of them (TaskScheduled via the cleanup-activity test, TimerCreated via the retry test). Added two storage-backed scenarios matching the Core cases:

• RewindActivityFailWithCleanupSubOrchestration — catch block starts CleanupChildWorkflow, exercising the SubOrchestrationInstanceCreated / SubOrchestrationInstanceCompleted pair.
• RewindActivityFailWithSendEvent — catch block calls context.SendEvent on the orchestration itself, exercising the EventSent branch.

Verified these actually guard the branches rather than just passing. Surgically removed only SubOrchestrationInstanceCreated and EventSent from IsScheduledEventType (leaving the rest of the change intact) and re-ran:

Failed RewindActivityFailWithCleanupSubOrchestration
  Non-Deterministic workflow detected: A previous execution of this orchestration
  scheduled a sub-orchestration task with sequence ID 1 and name '...CleanupChildWorkflow' ...

Failed RewindActivityFailWithSendEvent
  Non-Deterministic workflow detected: A previous execution of this orchestration
  scheduled a send event task with sequence ID 1, type 'EventSent' name 'ActivityFailed' ...

Each test fails naming its own event type, so the two tests are not redundant with each other. Restoring the branches makes them pass again, and all 11 Rewind* end-to-end tests pass together (no cross-test state leakage).

One note on the sub-orchestration test: CleanupChildWorkflow deliberately schedules no work of its own. TestOrchestrationHost only resolves [KnownType] one level deep, so a child that scheduled an activity would hang on an unregistered activity type. The events under test live in the parent's history and are emitted regardless of what the child does internally.

[Copilot]

Pull request overview

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