feat(errors): typed per-operation error hierarchy#540
Conversation
e538dd6 to
5a8e07e
Compare
5a8e07e to
a92d9cc
Compare
51efedf to
f97cd67
Compare
| sub_type=self.sub_type, | ||
| ) | ||
| ) | ||
| raise |
There was a problem hiding this comment.
Not replay safe if ExecutionError is thrown
There was a problem hiding this comment.
Good catch. first-run raises raw ExecutionError but replay gives ChildContextError. I'll wrap ExecutionError as ChildContextError so first run and replay match, and while I am here, I'll do the same in wait_for_condition.
There was a problem hiding this comment.
Right now a non-retryable BotoClientError skips the FAIL checkpoint and the context op stays STARTED in a FAILED execution. Same in wait_for_condition.py.
Gate prob needs to be isinstance(e, InvocationError) and e.is_retryable() per #546? If so, please add the check and a non-retryable test case.
| # error records its own type; a typed wrapper records its operation kind. | ||
| # For a DurableOperationError also preserve its own data and stack_trace | ||
| # so the inner info survives across a single operation boundary. | ||
| if isinstance(exception, DurableOperationError): |
There was a problem hiding this comment.
yes I am going to fix it separately
52614ad to
e913712
Compare
e913712 to
44d9963
Compare
|
|
||
| def __init_subclass__(cls, **kwargs: object) -> None: | ||
| super().__init_subclass__(**kwargs) | ||
| _DURABLE_OPERATION_ERROR_REGISTRY[cls.__name__] = cls |
There was a problem hiding this comment.
__init_subclass__ registers every subclass anyone defines, including user-space code, so the wire-reconstruction path ends up calling constructors the SDK doesn't control?
what happens if this raises out of the Step?
class CustomError(StepError):
def __init__(self, my_arg: str):
super().__init__(f"arb: {my_arg}")
- The step fails, and the FAIL checkpoint records ErrorType: "CustomError" (the checkpoint always stores the raw escaping type.
- On retry-exhaustion (first run) or replay, the SDK reconstructs: registry lookup finds
CustomError, calls it withmessage=..., error_type=..., data=..., stack_trace=.... - But init only accepts my_arg, so
TypeError: PaymentDeclinedError.__init__() got an unexpected keyword argument 'message'
It might be an idea to close the registry, so instead of init_subclass with an explicit dict literal of the SDK's own classes, built once after the class definitions, without metaclass magic. i.e something like
@classmethod
def from_error_fields(cls, error_type, message, data, stack_trace) -> DurableOperationError:
target_cls: type[DurableOperationError] = _DURABLE_OPERATION_ERROR_REGISTRY.get(
error_type or "", DurableOperationError
)
return target_cls(message=message, error_type=error_type, data=data, stack_trace=stack_trace)
For comparison, the JS reference implementation demotes any unknown type to StepError, so a JS user subclassing StepError gets the same fallback.
| # error records its own type; a typed wrapper records its operation kind. | ||
| # For a DurableOperationError also preserve its own data and stack_trace | ||
| # so the inner info survives across a single operation boundary. | ||
| if isinstance(exception, DurableOperationError): |
| def raise_operation_error( | ||
| self, | ||
| operation_error_cls: type[DurableOperationError], | ||
| msg: str | None = None, |
There was a problem hiding this comment.
NoReturn, rather than None?
| InvokeError, | ||
| StepError, | ||
| ValidationError, | ||
| WaitForConditionError, |
There was a problem hiding this comment.
CallbackError not here?
saying that, we could argue only to keep the "big" or notable err types here in the root export. InvocationError/ExecutionError/DurableOperationError/ValidationError? Not necessarily convinced of this, mind, you could equally argue these should be top-level exports too 😅
| # ChildContextError wrapping the escaping error (reconstructed as its | ||
| # typed form where possible). Remaining errors stay in get_errors(). | ||
| cause: DurableOperationError = first_error.to_durable_operation_error() | ||
| raise ChildContextError( |
There was a problem hiding this comment.
doesn't this duplicate what first_error.raise_as_operation_error(ChildContextError) does?
| sub_type=self.sub_type, | ||
| ) | ||
| ) | ||
| raise |
There was a problem hiding this comment.
Right now a non-retryable BotoClientError skips the FAIL checkpoint and the context op stays STARTED in a FAILED execution. Same in wait_for_condition.py.
Gate prob needs to be isinstance(e, InvocationError) and e.is_retryable() per #546? If so, please add the check and a non-retryable test case.
| # Each item runs in a child context, so a failed item surfaces as a | ||
| # ChildContextError wrapping the escaping error (reconstructed as its | ||
| # typed form where possible). Remaining errors stay in get_errors(). | ||
| cause: DurableOperationError = first_error.to_durable_operation_error() |
There was a problem hiding this comment.
first_error here holds a different record live vs replay, I think.
the live path stores from_exception(ChildContextError) (type="ChildContextError", executor.py _create_result), replay stores the raw checkpoint error (type="ValueError").
So the ChildContextError raised here carries a different error_type across replay.
This is a pre-existing issue, but we might as well take this oppo. to fix this and normalize the live path in _create_result to build the ErrorObject from the wrapper's fields (type=e.error_type) so both paths equal the checkpoint record.
| assert result.error.to_dict() == { | ||
| "ErrorMessage": "my callback error", | ||
| "ErrorType": "CallableRuntimeError", | ||
| "ErrorType": "ChildContextError", |
There was a problem hiding this comment.
I see in PR description callback is deferred for follow up, so jotting this down so I don't forget:
with this PR, wait_for_callback failures surface as ChildContextError.
The desired end state is JS's behavior (wait-for-callback-handler.ts errorMapper): callback errors pass through typed (CallbackError/CallbackTimeoutError/CallbackExternalError), submitter StepError maps to CallbackSubmitterError.
So the graded-callback follow-up will change this surface again.
Issue #, if available:
#120
#546
Description of changes:
Replaces the single catch-all
CallableRuntimeErrorwith a typed, per-operation error hierarchy. A failed step, invoke, child context, or wait_for_condition now surfaces as a distinct, catchable exception type. It also lands thefrom_exception/throw_if_errorchanges those types require. TheCallbackErrorrework and the nested-boundary regression test are intentionally deferred to follow-up PRs.Breaking changes
Testing
Follow-ups
CallbackErrorunderDurableOperationErrorwith graded subclasses (CallbackExternalError,CallbackTimeoutError,CallbackSubmitterError) and dropCALLBACK_ERRORmetadata.datasurviving ≥2 nestedrun_in_child_contextboundariesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.