-
Notifications
You must be signed in to change notification settings - Fork 24
Address various entity-related bugs #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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