Address various entity-related bugs#109
Conversation
durabletask/worker.py
Outdated
| try: | ||
| result_json = result if is_result_encoded else shared.to_json(result) | ||
| except TypeError: | ||
| result_json = shared.to_json(str(JsonEncodeOutputException(result))) |
There was a problem hiding this comment.
Reviewers - this is the "safe" approach where a return value that we cannot process to json is instead stringified along with an error message. I believe the other SDKs would just fail the orchestration outright - is this preferable here too?
There was a problem hiding this comment.
Pull request overview
This PR fixes several Durable Entities correctness issues in the Python SDK, aligning behavior with other Durable Task SDKs/backends (notably around entity ID normalization, entity failure propagation, and orchestration completion semantics).
Changes:
- Normalize
EntityInstanceId.entityand registered entity names to lowercase and tighten entity ID parsing validation. - Propagate
entityOperationFailedevents as task failures surfaced to user code (viaTaskFailedErrorwrapping anEntityOperationFailedException). - Prevent orchestrations from hanging when user code returns a non-JSON-encodable result, and add extensive DTS (sidecar/emulator) E2E coverage for entities.
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/durabletask/entities/test_entity_id_parsing.py | Adds unit coverage for entity ID parsing and case-insensitivity. |
| tests/durabletask-azuremanaged/entities/test_dts_function_based_entities_e2e.py | Adds DTS E2E coverage for function-based entity scenarios (signal/call/locks/unlocks). |
| tests/durabletask-azuremanaged/entities/test_dts_entity_failure_handling.py | Adds DTS E2E coverage for entity operation failures and lock recovery behavior. |
| tests/durabletask-azuremanaged/entities/test_dts_class_based_entities_e2e.py | Adds DTS E2E coverage for class-based entities and custom names. |
| tests/durabletask-azuremanaged/entities/init.py | Introduces entities test package module. |
| durabletask/worker.py | Implements lowercase entity registration, removes cached entity instances, adds entity failure handling, and guards orchestration completion encoding. |
| durabletask/internal/json_encode_output_exception.py | Adds a custom exception type used to describe JSON encoding failures for orchestration outputs. |
| durabletask/entities/entity_operation_failed_exception.py | Defines a typed exception for failed entity operations (used for user-visible failures). |
| durabletask/entities/entity_instance_id.py | Lowercases entity names and hardens parsing against invalid formats. |
| durabletask/entities/init.py | Exposes EntityOperationFailedException as part of the public entities API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/durabletask-azuremanaged/entities/test_dts_entity_failure_handling.py
Outdated
Show resolved
Hide resolved
halspang
left a comment
There was a problem hiding this comment.
Seems fine overall, just a few questions.
Addresses the following issues:
a. NOTE: The fix for this (normalizing entity names to lowercase) is a breaking change under the following scenarios:
register_entity(counter)andregister_entity(Counter)on the same worker will now failinstance_id.name == 'Counter') may breaka. Now, entity failures are handled like any other task failure by the orchestrator, and returned to user code as an EntityOperationFailedException wrapped in a TaskFailedError.
Resolves #108