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.