ManageIQ / floe

Floe is a runner for Amazon States Language workflows
Apache License 2.0
0 stars 5 forks source link

Don't set next_state until finish for Wait state #159

Closed agrare closed 7 months ago

agrare commented 7 months ago

By setting the next_state in Wait#start we were bypassing the Wait#finish step because the next_state was already set. This caused missing entries in the context for the state completion.

Before:

$ bundle exec exe/floe --podman --workflow examples/wait.asl --input '{"foo": 1}'
I, [2024-02-14T11:19:44.894906 #87255]  INFO -- : checking 1 workflows...
I, [2024-02-14T11:19:44.895029 #87255]  INFO -- : Running state: [FirstState] with input [{"foo"=>1}]...
D, [2024-02-14T11:19:44.895117 #87255] DEBUG -- : Running podman run --detach -e foo\=1 --name floe-sleep-4f985a02 localhost/sleep:latest
I, [2024-02-14T11:19:59.828972 #87255]  INFO -- : Running state: [FirstState] with input [{"foo"=>1}]...Complete - next state: [WaitState] output: [{"foo"=>1}]
I, [2024-02-14T11:20:00.035521 #87255]  INFO -- : Running state: [WaitState] with input [{"foo"=>1}]...
I, [2024-02-14T11:20:05.000492 #87255]  INFO -- : Running state: [SuccessState] with input [{"foo"=>1}]...
I, [2024-02-14T11:20:05.000658 #87255]  INFO -- : Running state: [SuccessState] with input [{"foo"=>1}]...Complete - next state: [] output: [{"foo"=>1}]
I, [2024-02-14T11:20:05.000713 #87255]  INFO -- : checking 1 workflows...Complete - 1 ready
{"foo"=>1}

Notice there is no Running state: [WaitState] with input [{"foo"=>1}]...Complete log line

miq-bot commented 7 months ago

Checked commits https://github.com/agrare/floe/compare/9cdfde57056fa0783d2ea0dae51298e13f04ba29~...f019789800e55b8a390d5e7f3763c1da60d0b602 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 4 files checked, 0 offenses detected Everything looks fine. :+1:

kbrock commented 7 months ago

I remember running into issues around wait and setting next. Think there may be a similar issue with Task.

Does this logic make sense:


^ think that logic is what you do here. looks good. Do we want to delay setting the output until after the state has completed?

agrare commented 7 months ago

@kbrock yeah exactly, Wait and Task are the only two currently which aren't ready? immediately, and Task already set next_state in #finish

kbrock commented 7 months ago

Would it make sense to put the next_state change into finish for Fail, Pass, and Succeed even though they are ready immediately? (maybe Choice?)

I'm wondering if next_state is correct with a Task that has a retry. Doesn't seem like Choice or Task could set it in finish since information would not be present at that time.

agrare commented 7 months ago

Would it make sense to put finish into everything and put the next_state change into fail, pass, and succeed even though they are ready immediately.

I think that's what I've done, every state that has Next sets next_state in finish now. Task is a little different as you mentioned which is why I clear the next state in the failure case.