fix(stepfunctions): Choice.afterwards() excludes the otherwise branch from end states by default#37874
Conversation
|
Exemption request: this fix changes traversal logic only. No new CloudFormation resource types, properties, or custom resources are involved. The behavior change is fully covered by unit tests (28/28 pass), including a new regression test that directly reproduces the poll-loop failure from the issue. |
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Thanks for picking this up @Zelys-DFKH. The stepfunctions fix logic matches what was discussed on the issue and looks good to me. One thing I noticed: the diff also includes unrelated changes to
Splitting the ECS change into its own PR would probably move both faster while this one stays a focused stepfunctions bugfix (which is what the exemption request is asking the linter to grant), and the ECS prop gets its own design review without being conflated with a traversal-logic fix. Also, minor: the PR linter is asking for the title's first word not to be capitalized. |
…m end states by default AfterwardsOptions.includeOtherwise is documented with @default false but was ignored. outgoingTransitions() always added defaultChoice to the traversal, causing afterwards().next(Y) to traverse the otherwise branch and call next() on states reachable from it. In poll-loop patterns this threw StateAlreadyHasNextState. Fix adds includeDefaultChoiceTransitions to FindStateOptions, gates the defaultChoice push in outgoingTransitions() on it, and passes options.includeOtherwise ?? false from Choice.afterwards(). All external callers (bindToGraph, findReachableStates) are unaffected since the field defaults to true (undefined !== false). Fixes aws#37649.
df97c72 to
b6c3050
Compare
Choice.afterwards() excludes the otherwise branch from end states by default
Issue # (if applicable)
Closes #37649.
Credit to @taesungh for the clear diagnosis and reproduction steps, to @pahud for tracing the root cause to
outgoingTransitions()unconditionally includingdefaultChoiceregardless ofincludeOtherwise, and to @vishwakt for the design analysis on the feature-flag question.Reason for this change
AfterwardsOptions.includeOtherwiseis documented with@default false, meaningChoice.afterwards()should exclude the otherwise state from its returned end states by default. In practice the option was ignored.afterwards()calledState.findReachableEndStates()without forwarding any flag about the default transition, andState.outgoingTransitions()always addeddefaultChoiceto the traversal. The result: callingafterwards().next(Y)traversed into the otherwise branch, found states reachable from there, and called.next(Y)on them too. In the poll-loop pattern (whereotherwise()points to a state already chained earlier), this threwStateAlreadyHasNextStateinstead of working correctly.Description of changes
FindStateOptionsgainsreadonly includeDefaultChoiceTransitions?: boolean. Default istrue(i.e.undefined !== false), sobindToGraph,findReachableStates, and all external callers are unaffected. OnlyChoice.afterwards()passes this asfalse.State.outgoingTransitions()gates thedefaultChoicepush onoptions.includeDefaultChoiceTransitions !== false.Choice.afterwards()passesincludeDefaultChoiceTransitions: options.includeOtherwise ?? false, excluding the otherwise branch by default and including it only when the caller explicitly opts in.On feature flags: The old traversal contradicted the documented
@default falseforincludeOtherwise. No code relying on that traversal had a documented guarantee to stand on. The AGENTS.md feature-flag criteria technically apply here (behavior change for previously-synthesizable code), so I'm happy to add aBugFixflag (aws-stepfunctions:respectAfterwardsIncludeOtherwise) if the team prefers.Out of scope: The related case where
choice.afterwards({ includeOtherwise: true })throws whenotherwise()was already called (noted by @taesungh in the issue thread) is a separate problem and not addressed here.Describe any new or updated permissions being added
None.
Description of how you validated changes
New unit test
'afterwards() excludes the otherwise state from end states by default (poll-loop pattern)'instates-language.test.ts. It builds the exact poll-loop definition from the issue and asserts: (1) synth does not throwStateAlreadyHasNextState, and (2) the synthesized template showsDefault: 'WaitToRepeat'still rendering correctly andDoOtherThingchained only through theCompletedbranch. The existing'Can include OTHERWISE transition for Choice in afterwards()'test still passes, confirmingincludeOtherwise: trueis unaffected. All 28 stepfunctions unit tests pass.No integration test was added: this fix touches traversal logic only, with no new CloudFormation resource types, properties, or custom resources. An exemption comment will be added if the linter flags it.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license