Repository navigation
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| 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 👍 / 👎
| self._resolve( | ||
| self.result, | ||
| exc=ChildWorkflowExecutionFailed( | ||
| event.failure.reason, | ||
| failure=event.failure, | ||
| ), | ||
| exc=child_workflow_failed_error(event, self._data_converter), | ||
| ) | ||
|
|
||
| @child_workflow_events.event() | ||
| def handle_canceled( | ||
| self, event: history.ChildWorkflowExecutionCanceledEventAttributes | ||
| ) -> None: | ||
| self._transition(DecisionState.COMPLETED) | ||
| self._resolve( | ||
| self.result, | ||
| exc=ChildWorkflowExecutionCanceled( | ||
| "child workflow canceled", details=event.details | ||
| ), | ||
| cancel_requested = self.state in ( | ||
| DecisionState.CANCELED_AFTER_RECORDED, | ||
| DecisionState.CANCELED_AFTER_STARTED, | ||
| DecisionState.CANCELLATION_RECORDED, | ||
| ) | ||
| self._transition(DecisionState.COMPLETED) |
There was a problem hiding this comment.
⚠️ [cross-pr] Merging #199 drops data_converter and uses removed error names
Breaks when this PR and #199 both merge, and the one that lands second keeps #199's version of the state-machine construction and failure handlers. The breakage shows up as soon as an activity or child workflow fails, or the module is imported.
How
- This PR requires
data_converterinActivityStateMachine(request, completed, data_converter)(activity_state_machine.py:24-29).DecisionManager.__init__gets it as an extra argument (decision_manager.py:129-135), andWorkflowEnginepassesinfo.data_converter(workflow_engine.py:65). - feat!: Return Futures Instead of Coroutines #199 rewrites
DecisionManager.__init__(event_loop)and theschedule_activity/schedule_child_workflowcall sites. They buildActivityStateMachine(attrs, future)andChildWorkflowExecutionStateMachine(attrs, execution, result)with nodata_converter. - feat!: Return Futures Instead of Coroutines #199's
handle_failed/handle_canceled/handle_timeout/handle_initiation_failedstill raiseActivityFailure,ChildWorkflowExecutionCanceledand the other old names.cadence/error.pyin this PR no longer defines any of them, and it adds no aliases either (onlyActivityError,ChildWorkflowError,StartChildWorkflowExecutionError, etc. aterror.py:246-320). Keeping feat!: Return Futures Instead of Coroutines #199's lines therefore fails at import, or with a TypeError on construction. Keeping only part of them meansApplicationErrordetails are never decoded and the parent-requested-cancelforce_cancelpath is lost.
Fix
- This PR: nothing needed beyond keeping the
DecisionFuture/ChildWorkflowFutureImplhandling forresultif this PR lands second. - feat!: Return Futures Instead of Coroutines #199: thread
data_converterthroughDecisionManagerinto both state machines, and build errors with thecadence/_internal/failure.pyfactories (activity_failed_error,child_workflow_*_error) instead of the removed exception names. Also update its state-machine tests to the new constructor signatures.
Compared 2be6a55 with #199 at c301d52
Was this helpful? React with 👍 / 👎
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. - WorkflowCanceledError 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. A failing signal handler now fails the decision task via fail_decision with a FatalDecisionError whose __cause__ is the handler's exception, consistent with other fatal decision errors. 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 - StartChildWorkflowExecutionFailed -> StartChildWorkflowExecutionError - Renamed StartChildWorkflowExecutionFailed.cause -> failure_cause to avoid confusion with Error `cause`. - TestWorkflowEnvironment.get_workflow_error() returns a FatalDecisionError (caused by the handler's exception) when a signal handler fails.
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
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:
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. A failing signal handler now fails the decision task via fail_decision with a FatalDecisionError whose cause is the handler's exception, consistent with other fatal decision errors.
The breaking changes are:
cause.What changed?
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes