-
Notifications
You must be signed in to change notification settings - Fork 5
feat(workflow): support workflow failure #57
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,21 +1,24 @@ | ||||||||
| import logging | ||||||||
| from dataclasses import dataclass | ||||||||
| import traceback | ||||||||
| from typing import List | ||||||||
|
|
||||||||
| from cadence._internal.workflow.context import Context | ||||||||
| from cadence._internal.workflow.decision_events_iterator import DecisionEventsIterator | ||||||||
| from cadence._internal.workflow.statemachine.decision_manager import DecisionManager | ||||||||
| from cadence._internal.workflow.workflow_intance import WorkflowInstance | ||||||||
| from cadence.api.v1.common_pb2 import Payload | ||||||||
| from cadence.api.v1.common_pb2 import Failure, Payload | ||||||||
| from cadence.api.v1.decision_pb2 import ( | ||||||||
| CompleteWorkflowExecutionDecisionAttributes, | ||||||||
| Decision, | ||||||||
| FailWorkflowExecutionDecisionAttributes, | ||||||||
| ) | ||||||||
| from cadence.api.v1.history_pb2 import ( | ||||||||
| HistoryEvent, | ||||||||
| WorkflowExecutionStartedEventAttributes, | ||||||||
| ) | ||||||||
| from cadence.api.v1.service_worker_pb2 import PollForDecisionTaskResponse | ||||||||
| from cadence.error import WorkflowFailure | ||||||||
| from cadence.workflow import WorkflowDefinition, WorkflowInfo | ||||||||
|
|
||||||||
| logger = logging.getLogger(__name__) | ||||||||
|
|
@@ -75,23 +78,29 @@ def process_decision( | |||||||
| decisions = self._decision_manager.collect_pending_decisions() | ||||||||
|
|
||||||||
| # complete workflow if it is done | ||||||||
| try: | ||||||||
| if self._workflow_instance.is_done(): | ||||||||
| if self._workflow_instance.is_done(): | ||||||||
| try: | ||||||||
| result = self._workflow_instance.get_result() | ||||||||
| except WorkflowFailure as e: | ||||||||
| decisions.append( | ||||||||
| Decision( | ||||||||
| fail_workflow_execution_decision_attributes=FailWorkflowExecutionDecisionAttributes( | ||||||||
| failure=_failure_from_workflow_failure(e) | ||||||||
| ) | ||||||||
| ) | ||||||||
| ) | ||||||||
| # TODO: handle cancellation error | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually for other exceptions, they should be handled all together in the outter most layer WorkflowWorker. |
||||||||
| except Exception: | ||||||||
| raise | ||||||||
| else: | ||||||||
| decisions.append( | ||||||||
| Decision( | ||||||||
| complete_workflow_execution_decision_attributes=CompleteWorkflowExecutionDecisionAttributes( | ||||||||
| result=result | ||||||||
| ) | ||||||||
| ) | ||||||||
| ) | ||||||||
| return DecisionResult(decisions=decisions) | ||||||||
|
|
||||||||
| except Exception: | ||||||||
| # TODO: handle CancellationError | ||||||||
| # TODO: handle WorkflowError | ||||||||
| # TODO: handle unknown error, fail decision task and try again instead of breaking the engine | ||||||||
| raise | ||||||||
| return DecisionResult(decisions=decisions) | ||||||||
|
|
||||||||
| except Exception as e: | ||||||||
| # Log decision task failure with full context (matches Java ReplayDecisionTaskHandler) | ||||||||
|
|
@@ -221,3 +230,16 @@ def _extract_workflow_input( | |||||||
| return started_attrs.input | ||||||||
|
|
||||||||
| raise ValueError("No WorkflowExecutionStarted event found in history") | ||||||||
|
|
||||||||
|
|
||||||||
| def _failure_from_workflow_failure(e: WorkflowFailure) -> Failure: | ||||||||
| cause = e.__cause__ | ||||||||
|
|
||||||||
| stacktrace = "".join(traceback.format_exception(cause)) | ||||||||
|
|
||||||||
| details = f"message: {str(cause)}\nstacktrace: {stacktrace}" | ||||||||
|
|
||||||||
| return Failure( | ||||||||
| reason=type(cause).__name__, | ||||||||
| details=details.encode("utf-8"), | ||||||||
| ) | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| from asyncio import Task | ||
| from asyncio import CancelledError, InvalidStateError, Task | ||
| from typing import Any, Optional | ||
| from cadence._internal.workflow.deterministic_event_loop import DeterministicEventLoop | ||
| from cadence.api.v1.common_pb2 import Payload | ||
| from cadence.data_converter import DataConverter | ||
| from cadence.error import WorkflowFailure | ||
| from cadence.workflow import WorkflowDefinition | ||
|
|
||
|
|
||
|
|
@@ -33,6 +34,11 @@ def is_done(self) -> bool: | |
| def get_result(self) -> Payload: | ||
| if self._task is None: | ||
| raise RuntimeError("Workflow is not started yet") | ||
| result = self._task.result() | ||
| try: | ||
| result = self._task.result() | ||
| except (CancelledError, InvalidStateError) as e: | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to wrap cancelled error? There are different scenarios to iterate on this. |
||
| raise e | ||
| except Exception as e: | ||
| raise WorkflowFailure(f"Workflow failed: {e}") from e | ||
| # TODO: handle result with multiple outputs | ||
| return self._data_converter.to_data([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.
Nit: Maybe use the
exception()method instead of try/catch to simplify the flowThere 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.
I've thought about this. The exception() actually would raise
CancelledErrorrather than just return it. So there isn't any simplification for this. I still want to keep theWorkflowInstance's API easier: one way to get the result or throw an exception.