Propagate durable activity failures to orchestrator function #571
Conversation
AnatoliB
changed the title
| // - https://github.com/Azure/azure-functions-durable-js/pull/145 | ||
| // - https://github.com/Azure/azure-functions-durable-extension/pull/1171 | ||
| var orchestrationMessage = new OrchestrationMessage(isDone: false, new List<List<OrchestrationAction>> { actions }, output: null, exception.Message); | ||
| var message = $"{exception.Message}{OutOfProcDataLabel}{JsonConvert.SerializeObject(orchestrationMessage)}"; |
There was a problem hiding this comment.
Is value orchestrationMessage always a valid JSON?
There was a problem hiding this comment.
Please note we are converting to JSON, not from JSON. orchestrationMessage is an object, and the result of this conversion is expected to be a valid JSON (assuming Newtonsoft.Json is correct).
There was a problem hiding this comment.
You are correct Anatoli, I missed read it. My comment does not apply. Resolving.
| { | ||
| case HttpStatusCode.Accepted: | ||
| { | ||
| if (DateTime.UtcNow > startTime + orchestrationCompletionTimeout) |
There was a problem hiding this comment.
Nit: Extra tab before the if statement.
| var thrownException = Assert.Throws<OrchestrationFailureException>(() => InvokeOrchestration(completed: true, output)); | ||
| Assert.Same(originalException, thrownException.InnerException); | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we have a test case that validates that the FullyQualifiedErrorId contains Functions.Durable.ActivityFailure? I can see this becoming a popular search online later on.
There was a problem hiding this comment.
Good point. We cannot validate this right here because FullyQualifiedErrorId does not belong to the exception. It's the other way around: the exception is wrapped into an ErrorRecord, and this ErrorRecord contains FullyQualifiedErrorId... But I agree we should emphasize this because this errorId will become a part of the public contract that we shouldn't break in the future. I think this check should go to DurableActivityErrorHandler unit tests, I'll do that.
There was a problem hiding this comment.
Sounds good. Thanks Anatoli.
Resolves #572
There are two problems to solve here:
Make
Invoke-ActivityFunctionreport an error on activity failure. This part is simple:TaskFailedevents are recorded in the orchestration history, so we just makeInvoke-ActivityFunctionwrite an error (ActivityFailureException) to the Error stream. By default, this is treated as a non-terminating error in PowerShell, but the orchestrator function author can use either$ErrorActionPreference = 'Stop'or-ErrorAction Stopto convert it to an exception, if desired.Unfortunately, completing (1) is not enough: throwing this exception from the orchestrator function leads to errors complaining about non-deterministic orchestrator behavior because of certain implementation quirks affecting out-of-proc language workers. This PR follows the approach taken by the JavaScript Durable implementation (Fix nondeterminism on Activity/Suborchestration failures azure-functions-durable-js#145): the orchestration replay state is embedded into the exception message after the
$OutOfProcData$marker.