5222 - Skip action update if execution already in completed state.

[khushboobhatia01]

Race condition causing orquesta workflow status to change to running after it completed successfully. To avoid this incorrect status update, skipping execution object update is execution is already in completed state.

More details about issue: #5222

[m4dcoder]

1. I don't completely recall but I think there may be some edge cases impacted by this. One test case I can think of is with items task in which one of the executions for an item failed, this leads to the workflow execution going into failed state. However, items that are still in progress are allowed to update result into the execution. I suggest getting CI jobs here to completely pass so we know it is not impacting other documented test cases.
2. Please make sure there's unit test associated with your change.
3. Have you tested this change locally on your workflows? Did it completely resolve?

[khushboobhatia01]

Please find response inline @m4dcoder

1. I don't completely recall but I think there may be some edge cases impacted by this. One test case I can think of is with items task in which one of the executions for an item failed, this leads to the workflow execution going into failed state. However, items that are still in progress are allowed to update result into the execution. I suggest getting CI jobs here to completely pass so we know it is not impacting other documented test cases.

----> All the test cases are fixed now. The one-off edge case you mentioned regarding with-items failure is verified with test_with_items_failure test case which is running fine.

2. Please make sure there's unit test associated with your change.

----> Unit test case added st2common/tests/unit/test_executions_util.py

3. Have you tested this change locally on your workflows? Did it completely resolve?

----> Yes, I've verified the changes locally. I've setup an environment where I'm able to reproduce the race condition.
Below screenshot is without my changes. You can see in the execution logs, status changed from running -> succeeded -> running.

With my changes, you can see the status not changing to running after it succeeded. Also, added screenshot of log line when execution update was skipped.

[m4dcoder]

Everything passes. But I have some more questions, specifically the patching in the unit tests.

[m4dcoder]

Overall I don't have any issue with the changes since all unit and integrate tests pass and the concern address from the change is valid. I requested some minor cleanups. I will approve when cleanups are addressed and everything goes green.

[khushboobhatia01]

Overall I don't have any issue with the changes since all unit and integrate tests pass and the concern address from the change is valid. I requested some minor cleanups. I will approve when cleanups are addressed and everything goes green.

@m4dcoder All review comments addressed. Please approve and merge.

[m4dcoder]

Almost there, still missing some cleanups. Can you check the unit tests that you've refactored and make sure there are no duplicate query/asserts on the live action and action execution? If there are asserts that are out of place after the cleanup, please move them closer to where they should be checked (i.e. move live action result check closer to where the status was checked).

[khushboobhatia01]

Almost there, still missing some cleanups. Can you check the unit tests that you've refactored and make sure there are no duplicate query/asserts on the live action and action execution? If there are asserts that are out of place after the cleanup, please move them closer to where they should be checked (i.e. move live action result check closer to where the status was checked).

@m4dcoder All test case cleanups are addressed.

[m4dcoder]

Good job!