Skip to content

fix(testing): handle workflow failure states correctly - #682

Open
hln33 wants to merge 1 commit into
mainfrom
fix/656-emulator-null-invocation-error-causes-successful-execution
Open

fix(testing): handle workflow failure states correctly#682
hln33 wants to merge 1 commit into
mainfrom
fix/656-emulator-null-invocation-error-causes-successful-execution

Conversation

@hln33

@hln33 hln33 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Issue #, if available:
#656

Description of changes:

  • Modify handling of failed invocations to authoratively call self._fail_workflow() instead of self._complete_workflow() so that executions fail despite not having an error payload
    • Confirmed against the real backend service that failed invocations without error payloads should result in a failed execution
    • Prior to this change, failed invocations with no error payloads would result in a successful execution
  • Add tests covering the following behavior (requested in [Bug]: FAILED invocation without ErrorObject is completed as SUCCEEDED by local emulator #656):
    • A failed invocation results in a failed execution
    • Caught Child context failures do not cause execution failure

Testing:

  • Ran all tests in package via hatch run test:all
  • Invoked SAM CLI with local emulator changes and a reproducing Java durable function. This confirms failed invocations with empty error payloads result in failed execution:
sam local invoke WaitForConditionChildFailureJavaFunction

...

Execution Summary:
=========================
ARN:      31d25a2e-2e37-4bea-b67e-02441a5fa80c/55c89648-9aca-4b8e-8147-f01658dabf0a                                                                           Name:     N/A                                                                  Duration: 6.58s
Status:   FAILED ❌
Input:    {}

Commands you can use next
=========================
[*] Get execution details: sam local execution get 31d25a2e-2e37-4bea-b67e-02441a5fa80c/55c89648-9aca-4b8e-8147-f01658dabf0a
[*] View execution history: sam local execution history 31d25a2e-2e37-4bea-b67e-02441a5fa80c/55c89648-9aca-4b8e-8147-f01658dabf0a

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@hln33
hln33 temporarily deployed to ai-pr-review-runtime August 25, 2026 22:56 — with GitHub Actions Inactive
@hln33
hln33 temporarily deployed to ai-pr-review-runtime August 25, 2026 22:56 — with GitHub Actions Inactive
@hln33
hln33 temporarily deployed to ai-pr-review-runtime August 25, 2026 22:56 — with GitHub Actions Inactive
@hln33
hln33 marked this pull request as draft August 25, 2026 22:56
@hln33
hln33 force-pushed the fix/656-emulator-null-invocation-error-causes-successful-execution branch from f4129a2 to b6a604a Compare August 25, 2026 22:59
@hln33
hln33 force-pushed the fix/656-emulator-null-invocation-error-causes-successful-execution branch from b6a604a to 861756a Compare August 25, 2026 23:20
@hln33
hln33 force-pushed the fix/656-emulator-null-invocation-error-causes-successful-execution branch from 861756a to 09acaf6 Compare August 25, 2026 23:22
@hln33
hln33 deployed to ai-pr-review August 25, 2026 23:22 — with GitHub Actions Active
@hln33
hln33 had a problem deploying to ai-pr-review-runtime August 25, 2026 23:35 — with GitHub Actions Failure
@hln33
hln33 temporarily deployed to ai-pr-review-runtime August 25, 2026 23:35 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@hln33
hln33 marked this pull request as ready for review August 26, 2026 00:24
@hln33
hln33 temporarily deployed to ai-pr-review-runtime August 26, 2026 00:24 — with GitHub Actions Inactive
@hln33
hln33 temporarily deployed to ai-pr-review-runtime August 26, 2026 00:24 — with GitHub Actions Inactive
@hln33
hln33 had a problem deploying to ai-pr-review-runtime August 26, 2026 00:24 — with GitHub Actions Failure
@github-actions

This comment has been minimized.

bchampp
bchampp previously approved these changes Aug 26, 2026

@yaythomas yaythomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, thank you! just some nits.

Comment thread packages/aws-durable-execution-sdk-python-testing/tests/executor_test.py Outdated
Comment thread packages/aws-durable-execution-sdk-python-testing/tests/executor_test.py Outdated
Comment thread packages/aws-durable-execution-sdk-python-testing/tests/execution_test.py Outdated
@github-actions

This comment has been minimized.

@hln33
hln33 temporarily deployed to ai-pr-review-runtime August 26, 2026 22:15 — with GitHub Actions Inactive
@hln33
hln33 had a problem deploying to ai-pr-review-runtime August 26, 2026 22:15 — with GitHub Actions Failure
if execution.status == "FAILED":
logger.warning("Execution failed")
return execution
if execution.status in ["TIMED_OUT", "ABORTED"]:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think "ABORTED" is a valid execution status. Could not find any references to it in other SDKs. The API documentation also does not list "ABORTED" as being a valid execution status.

I think it should instead be "STOPPED", as that specific status is not covered in this function.

I noticed this when implementing a new "execution_status" field on the test result class and my AI coding agent flagged this to me.

@hln33
hln33 requested a review from yaythomas August 26, 2026 22:18
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings. Residual risk is limited to runtime validation because tests were not executed under the review constraints.

Reviewed commit d8badbeb9c64d9256e556361cd6534e8e9a48eca. Workflow run

@yaythomas yaythomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

friendly reminder to squash at merge with a conventional-commits message (lint-commits is currently red only because of the squash: markers).

Preserve failed invocation results when Lambda returns FAILED without an
error payload. The local test runner now reports a failed execution
instead of treating the result as successful.

Expose terminal execution status on durable function test results and
align cloud runner polling with the STOPPED terminal status.
@hln33
hln33 force-pushed the fix/656-emulator-null-invocation-error-causes-successful-execution branch from d8badbe to 79d088b Compare August 27, 2026 21:10
@hln33
hln33 requested a deployment to ai-pr-review-runtime August 27, 2026 21:10 — with GitHub Actions In progress
@hln33
hln33 requested a deployment to ai-pr-review-runtime August 27, 2026 21:10 — with GitHub Actions In progress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants