Repository navigation
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This is a breaking change! Standardize the exception hierarchy, introducing CadenceError as the root of all Errors and FailureError as the root of any Failure expected in Workflow code. We call it FailureError rather than WorkflowError as ApplicationError is raised by Activity implementations. Add ApplicationError. This allows users to explicitly specify the error reason, details, and any FailureOptions from a Workflow or an Activity. If a Workflow or Activity raises any other type of Error then the reason will be the name of the error type and the details will be a serialized representation of the error. We don't attempt to make this representation of the error round-trippable. It's purely for manual inspection and debugging, not to propagate stacktraces through a Workflow. Within Workflows we introduce a consistent hierarchy for receiving exceptions from Activities or Child Workflows. For Activities: - If cancelled, we raise asyncio.CancelledError - Else, we raise ActivityError. The reason for the error is the backing `__cause__`, which we also expose as a property: - ApplicationError if the activity failed. This contains the reason and details. - CadenceTimeoutError if the activity timed out. For Child Workflows: - If cancelled by the Workflow, we raise asyncio.CancelledError - If we failed to start the Child Workflow, we raise StartChildWorkflowExecutionError (extends ChildWorkflowError) - Else, we raise ChildWorkflowError. Again the reason is the backing cause: - ApplicationError if the child workflow failed. - WorkflowCancelledError if the child workflow was cancelled externally - CadenceTimeoutError if the child workflow timed out. - TerminatedError if the child worfklow was terminated. Distinguishing between cancellation from within the Workflow and external Cancellation is important because asyncio.CancelledError extends BaseException instead of Exception. It's unlikely that Workflow code expects a non-Exception type if they're not the one cancelling the Workflow. This aligns with the Java SDK. SignalFailure is removed, it was an implementation detail that was never exposed to users. Usage was replaced with FatalDecisionError. The breaking changes are: - Removed most subtypes of ChildWorkflowError to align it with Activities. - Renamed all errors to end with Error: - ActivityFailure -> ActivityError - WorkflowFailure -> FailureError - SignalExternalWorkflowFailed -> SignalExternalWorkflowError - StartChildWorkflowExecutionError -> StartChildWorkflowExecutionError - Renamed StartChildWorkflowExecutionFailedError.cause -> failure_cause to avoid confusion with Error `cause`.
| if cancel_requested: | ||
| self.result.force_cancel(event.details.data.decode()) |
There was a problem hiding this comment.
💡 Edge Case: Strict UTF-8 decode of child cancel details can crash the decision task
When the parent has requested cancellation, handle_canceled calls self.result.force_cancel(event.details.data.decode()). This decodes the raw details payload as strict UTF-8. That payload comes from the pluggable DataConverter (or from whichever client canceled the child), so it is not guaranteed to be valid UTF-8; a binary converter such as protobuf or msgpack will produce invalid bytes. When it is not, a UnicodeDecodeError is raised inside the history event handler, which fails the decision task, and replaying the same history fails again every time. The rest of this PR decodes untrusted payload bytes with errors="replace" (see ApplicationError._from_payload). Do the same here, or leave the details out of the cancel message entirely.
Decode leniently so malformed bytes can't raise in the event handler:
self.result.force_cancel(
event.details.data.decode("utf-8", errors="replace")
)
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| class ActivityError(FailureError): | ||
| """An activity failed or timed out. | ||
|
|
||
| ``cause`` is an :class:`ApplicationError` if the activity failed, or a | ||
| :class:`CadenceTimeoutError` if it timed out. Cancellation is raised as | ||
| :class:`asyncio.CancelledError` instead. | ||
| """ | ||
|
|
||
| class SignalExternalWorkflowFailed(Exception): | ||
| def __init__(self, message: str, cause: Any) -> None: | ||
| def __init__(self, message: str, *, activity_id: str, activity_type: str) -> None: | ||
| super().__init__(message) | ||
| self.cause = cause | ||
| self.activity_id = activity_id | ||
| self.activity_type = activity_type | ||
|
|
||
| @property | ||
| def cause(self) -> BaseException | None: |
There was a problem hiding this comment.
⚠️ Cross-Repo: Cadence-Docs Python error guide still uses removed error classes
Fix here and in cadence-workflow/Cadence-Docs
Breaks in: cadence-workflow/Cadence-Docs - docs/07-python-client/09-error-handling.md - Python error handling guide (Activity failures, Child workflow failures, Signal failures sections and Error reference table)
The Python error-handling guide in cadence-workflow/Cadence-Docs (docs/07-python-client/09-error-handling.md) imports and catches ActivityFailure, StartChildWorkflowExecutionFailed, ChildWorkflowExecution{Failed,Canceled,TimedOut,Terminated}, SignalExternalWorkflowFailed, WorkflowFailure and SignalFailure from cadence.error. This PR removes or renames all of these to ActivityError, ChildWorkflowError (with a cause), StartChildWorkflowExecutionError, SignalExternalWorkflowError and FailureError, and cadence/error.py defines no aliases for the old names. So every snippet in the guide now raises ImportError. The guide's statement that the five child-workflow classes are subclasses of ChildWorkflowError is also no longer true: a failure now arrives as a ChildWorkflowError whose cause is an ApplicationError, CadenceTimeoutError, TerminatedError or WorkflowCanceledError, and cancellation the parent requested surfaces as asyncio.CancelledError. The break is intentional, but the docs need a coordinated update. The PR description also promises aliases that the code does not provide.
Here: Add the deprecated aliases the PR description promises so the old imports keep resolving during migration, and link the docs update from the release notes.
There: Rewrite the Activity, Child workflow and Signal failure sections and the Error reference table in docs/07-python-client/09-error-handling.md for the new hierarchy: catch ActivityError / ChildWorkflowError and inspect .cause, use StartChildWorkflowExecutionError.failure_cause and SignalExternalWorkflowError, drop SignalFailure, and document ApplicationError and asyncio.CancelledError for cancellation the workflow itself requested.
In cadence-workflow/Cadence-Docs - docs/07-python-client/09-error-handling.md:
from cadence.error import ActivityError, ApplicationError, CadenceTimeoutError
try:
await workflow.execute_activity(...)
except ActivityError as e:
if isinstance(e.cause, ApplicationError):
print(e.cause.reason, e.cause.details())
elif isinstance(e.cause, CadenceTimeoutError):
print(e.cause.timeout_type)
Hold the contract here:
# Deprecated aliases kept for backwards compatibility
ActivityFailure = ActivityError
WorkflowFailure = FailureError
SignalExternalWorkflowFailed = SignalExternalWorkflowError
StartChildWorkflowExecutionFailed = StartChildWorkflowExecutionError
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
CI failed: Two integration tests in test_failures.py fail on application-error propagation: one hits an IndexError on an empty workflow history, and the reraised-error test fails with no visible cause in the truncated log. Relation to this PR is unclear for both, since the PR touches error handling but no specific behavior change explains the failures.OverviewTwo unique failure templates across 4 logs (2 jobs, both showing the same two failing integration tests in FailuresApplication-error round-trip and reraised-error integration tests fail (confidence: medium)
Summary
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
This branch has not been deployed
1 participant
This is a breaking change!
Standardize the exception hierarchy, introducing CadenceError as the root of all Errors and FailureError as the root of any Failure expected in Workflow code. We call it FailureError rather than WorkflowError as ApplicationError is raised by Activity implementations.
Add ApplicationError. This allows users to explicitly specify the error reason, details, and any FailureOptions from a Workflow or an Activity. If a Workflow or Activity raises any other type of Error then the reason will be the name of the error type and the details will be a serialized representation of the error. We don't attempt to make this representation of the error round-trippable. It's purely for manual inspection and debugging, not to propagate stacktraces through a Workflow.
Within Workflows we introduce a consistent hierarchy for receiving exceptions from Activities or Child Workflows.
For Activities:
__cause__, which we also expose as a property:For Child Workflows:
SignalFailure is removed, it was an implementation detail that was never exposed to users. Usage was replaced with FatalDecisionError.
The breaking changes are:
cause.We add aliases for removed error types to reduce the impact of breaking changes.
What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes