[SFN] Improve Handling of Empty SendTaskFailure Calls #10750
Conversation
MEPalma
added
the
semver: patch
dominikschubert
left a comment
dominikschubert
left a comment
There was a problem hiding this comment.
Mostly LGTM! Definitely another interesting edge case.
Changes requested is mostly referring to the thread handling which I'd like to verify before a final approval.
localstack/services/stepfunctions/asl/component/common/catch/catcher_decl.py
Outdated
Show resolved
Hide resolved
| if ( | ||
| request.node.name | ||
| == "test_sqs_failure_in_wait_for_task_tok_no_error_field[SQS_PARALLEL_WAIT_FOR_TASK_TOKEN]" | ||
| ): | ||
| # TODO: The conditions in which TaskStateAborted error events are logged requires further investigations. | ||
| # These appear to be logged for Task state workers but only within Parallel states. The behaviour with | ||
| # other 'Abort' errors should also be investigated. | ||
| pytest.skip("Investigate occurrence logic of 'TaskStateAborted' errors") |
There was a problem hiding this comment.
Is this also not working against AWS or just localstack?
There was a problem hiding this comment.
It works against AWS. For LocalStack I first want to collect more insight into when it is triggered before updating the interpreter
There was a problem hiding this comment.
nit: You could in this case extend the if case by a check against is_aws_cloud() as well to make sure we only skip it when running against LS
LocalStack Community integration with Pro 2 files 2 suites 1h 34m 13s ⏱️ Results for commit ee13a0b. ♻️ This comment has been updated with latest results. |
dominikschubert
left a comment
dominikschubert
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the previous points. Minor nit regarding the test skip, but generally good to go from my side :)
| if ( | ||
| request.node.name | ||
| == "test_sqs_failure_in_wait_for_task_tok_no_error_field[SQS_PARALLEL_WAIT_FOR_TASK_TOKEN]" | ||
| ): | ||
| # TODO: The conditions in which TaskStateAborted error events are logged requires further investigations. | ||
| # These appear to be logged for Task state workers but only within Parallel states. The behaviour with | ||
| # other 'Abort' errors should also be investigated. | ||
| pytest.skip("Investigate occurrence logic of 'TaskStateAborted' errors") |
There was a problem hiding this comment.
nit: You could in this case extend the if case by a check against is_aws_cloud() as well to make sure we only skip it when running against LS
Motivation
Currently the StepFunctions v2 interpreter is unable of handling empty SentTaskFailure api calls. These are SendTaskFailure messages for waitForTaskToken tasks which do not provide an error string. Such calls would result in internal States.Runtime errors, and misleading error messages such as
Custom Error Names MUST NOT begin with the prefix 'States.', got 'State.Runtime'. This PR adds snapshot tests replicating this occurrence and add support for CustomErrorNames to have nullable error strings. It also identifies need for future work on StateTaskAbort events logging.Changes