Azure / Azure/durabletask

Rewind fails to rewind activities that were called with retry

Open
#811 8 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
1.7k
Forks
335
Avg merge
2d 23h
Merged PRs (30d)
6

Description

When calling an activity with retry (via `ScheduleTask` via `CallActivityWithRetryAsync`, from durable task extension), the `RetryInterceptor` retries the activities if they fail. For each retry, a `TimerCreated` (and consequent `TimerFired`) events are added to the history of the orchestration.

(And due to the following:

```c#
if (isLastRetry)
{
// Earlier versions of this retry interceptor had a bug that scheduled an extra delay timer.
// It's unfortunately not possible to remove the extra timer since that would potentially
// break the history replay for existing orchestrations. Instead, we do the next best thing
// and schedule a timer that fires immediately instead of waiting for a full delay interval.
await this.context.CreateTimer(this.context.CurrentUtcDateTime, "Dummy timer for back-compat");
break;
}
```

after the last attempt, another `TimerEvent` and `TimerFired` event are added).

When `AzureTableTrackingStore.RewindHistoryAsync` is called to rewind the orchestration, only `TaskFailed` and `SubOrchestrationInstanceFailed` (and their corresponding `TaskScheduled` and `SubOrchestrationInstanceCreated`) get their `EventType` reset to `GenericEvent`. So when the orchestration restarts, it encounters `TimerCreated` and `TimerFired` events that it did not expect, and causes the following error:

```
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. Was a change made to the orchestrator code after this instance had already startedrunning?
```

I think to fix this, the rewind algorithm should take the timer events into account, and also overwrite their `EventType` to `GenericEvent`. I've tested this by modifying the table storage entries before rewinding and that works. I can imagine that the fix is to find all the `TimerCreated` events that have an `EventId` higher than the `TaskScheduled` that is being reset. The corresponding `TimerFired` events can be found using the `TimerId` property.

I don't mind implementing the fix for this, but I would like to know if this is the best approach. I can imagine that this change can inadvertently reset some timers it should not touch. But as the Rewind algorithm just resets all the `TaskFailed` events, resetting the timer events after those events might just work fine.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.