Fix legacy entity protocol: propagate call_entity failures and stop double-encoding results#167
Conversation
call_entity failures delivered via the legacy entity protocol (used by the Azure Functions Durable extension) were silently completing instead of raising. _handle_entity_event_raised now detects failed ResponseMessage payloads (exceptionType/failureDetails) and fails the task with an EntityOperationFailedException, matching the current entity protocol and the .NET SDK.
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness gap in the core durabletask SDK where entity operation failures were not propagated to orchestrations when using the legacy entity protocol (used by the Azure Functions Durable/WebJobs extension). It aligns legacy-protocol behavior with the current protocol (and .NET) by detecting failure indicators in legacy ResponseMessage payloads and failing the awaiting call_entity task accordingly.
Changes:
- Added legacy
ResponseMessagefailure detection/conversion helpers to produceTaskFailureDetailswhenfailureDetailsorexceptionTypeindicate an entity operation failure. - Updated legacy-protocol
eventRaisedhandling to failcall_entitytasks withEntityOperationFailedExceptionwhen failures are detected. - Added orchestration executor tests covering legacy-protocol entity call failure/success cases, and documented the fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/durabletask/test_orchestration_executor.py | Adds regression tests for legacy-protocol entity call failure propagation (both failureDetails and exceptionType) and a success round-trip. |
| durabletask/worker.py | Detects legacy-protocol entity call failures in eventRaised handling and propagates them as task failures to orchestrations. |
| durabletask/internal/helpers.py | Adds helpers to extract/convert legacy ResponseMessage failure data into TaskFailureDetails. |
| CHANGELOG.md | Documents the user-visible fix under ## Unreleased → FIXED. |
The legacy entity protocol delivers the operation result as a serialized JSON string inside ResponseMessage.result. The success path used coerce, which does not parse JSON strings, so string/dict/list results arrived double-encoded and only numeric results with an explicit return_type worked. Deserialize the result string (type-directed) to match the current entity protocol. Adds parametrized legacy-protocol success round-trip tests.
Addresses PR review: _entity_task_id_map and _entity_lock_task_id_map entries were looked up with get() and never removed, so the maps could grow unbounded in long-running orchestrations. Use pop() to release each entry once its eventRaised is handled, matching the new-protocol handlers.
|
Reviewed this and it's in good shape overall — the double-encoding fix and the map cleanup look correct, and propagating failures instead of silently returning The error_message = response.get("exceptionType")
if error_message is not None:
return pb.TaskFailureDetails(errorType="", errorMessage=str(error_message))But public void SetExceptionResult(Exception exception, ...) {
this.ExceptionType = exception.GetType().AssemblyQualifiedName; // a TYPE NAME
this.Result = errorDataConverter.Serialize(exception); // the actual message/content
}The sibling Functions SDKs corroborate that
So for a real WebJobs failure the current code would surface an assembly-qualified type name (or a literal marker like Worth noting: the Two suggestions before merge:
|
In the WebJobs old-protocol ResponseMessage, exceptionType is only a presence marker / exception type name; the human-readable message is serialized into result. Detect failures by exceptionType presence (or a structured failureDetails object) and take the message from the deserialized result, matching azure-functions-durable-python / -js. Updates the exceptionType test to the real wire shape (message in result, type name in exceptionType) and guards against surfacing the type name.
|
Thanks @berndverst — you're right, and I dug in to confirm before fixing. Pushed a correction in 864f1dd. Verification. I checked both the producer and the canonical consumers:
So the two The fix (864f1dd):
On capturing a live payload: agreed it's the gold standard, but standing up the Functions host is heavy for this PR, and the reference SDKs (which shipped as the consumers of exactly this out-of-proc-Python |
…y-error-propagation # Conflicts: # CHANGELOG.md
Two related fixes to
OrchestrationContext.call_entityunder the legacy entity protocol — the protocol the Azure Functions Durable (WebJobs) extension uses when talking to out-of-proc Python workers. The current entity protocol (entityOperationCompleted/entityOperationFailed, used bydurabletask.azuremanaged/ DTS) already behaved correctly; only the legacyeventRaised-based path was affected.1. Entity operation failures were not propagated
A failed entity operation was silently completing (typically returning
None) instead of raising, so orchestrations could never observe or handle the failure.Legacy-protocol entity results arrive as an
eventRaisedhandled by_handle_entity_event_raised, which unconditionally calledentity_task.complete(...)and never inspected the response for a failure indicator. The wire contract is DurableTask.Core'sResponseMessage, whereIsErrorResult => exceptionType != null || failureDetails != null(exceptionTypecarries the error message;failureDetailscarries a structuredFailureDetails).Fix:
_handle_entity_event_raisednow detects a failed response and fails the task with anEntityOperationFailedException, so an awaitingcall_entityraisesTaskFailedError— matching the current entity protocol and the .NET SDK (CallEntityAsyncthrowsEntityOperationFailedExceptionwhen the resultIsError).2. Successful results were double-encoded
The
ResponseMessage.resultfield holds a serialized JSON string of the return value (just like the new protocol'sentityOperationCompleted.output). The success path usedcoerce, which applies a type to an already-parsed value and does not parse JSON strings. As a result, string/dict/list returns arrived double-encoded (e.g. a returned"done"surfaced as"\"done\""), and only numeric returns with an explicitreturn_typehappened to work.Fix: deserialize the
resultstring (type-directed) instead of coercing it, mirroring the current-protocol handler.Also addressed
eventRaiseddispatch topop()_entity_task_id_map/_entity_lock_task_id_mapentries once handled (they were looked up withget()and never removed), preventing unbounded map growth in long-running orchestrations — consistent with the new-protocol handlers. (From PR review.)Changes
durabletask/worker.py: failure detection + propagation in_handle_entity_event_raised; type-directed deserialization of the result;pop()map cleanup.durabletask/internal/helpers.py:entity_response_failure_details(...)plus a recursiveFailureDetailsconverter.tests/durabletask/test_orchestration_executor.py: legacy-protocol tests for failure viafailureDetails, failure viaexceptionType, and a parametrized success round-trip acrossstr/int/float/bool/dict/list(noreturn_typerequired).CHANGELOG.md: FIXED entries for both bugs.Validation
durabletask.azuremanageduses the current protocol and is unaffected.