Make update_execution() atomic

[khushboobhatia01]

Making update_execution() atomic to avoid concurrent updates causing inconsistency.

In the below screenshot two concurrent updates to the execution object resulted in inconsistent data where overall execution is set to running, but the log field depicts the execution had succeeded.

How did this happen?
Two interleaved update_execution() can cause this.

P1	P2
liveaction.status = running	liveaction.status = succeeded
execution.status = running	execution.status = running
if condition is not valid. Move forward.	if condition is not valid. Move forward.
if condition is not valid. No log is pushed	if condition is valid. succeeded log is pushed
	Action update happens
Action update happens

After the above interleaved execution, P1 will overwrite overall execution status (succeeded set by P2) to running.

[CLAassistant]

All committers have signed the CLA.

[cognifloyd]

So basically, all you did was add one line to create the with ...get_lock(...): line, and then indent then block of code to protect it within the lock. And then add some more to the tests. Nice.

Why did adding 1 lock in 1 method increase the number of lock side effects so much in the tests?

[cognifloyd]

Could we add a prefix to this to scope the change just to this block? Or are there other places already using this the live action id as the lock name that you also want to block?

[khushboobhatia01]

@cognifloyd No, get_lock() is not being used anywhere else.
And to answer why tests are failing, https://github.com/StackStorm/st2/blob/master/st2actions/tests/unit/test_workflow_engine.py#L155-L159 mocks get_lock() to return ToozConnectionError (to test

st2/st2common/st2common/services/workflows.py

Line 941 in 007beed

with coord_svc.get_coordinator(start_heart=True).get_lock(wf_ex_id):

).

Because of the above mocking, action execution update calls will fail with my changes and cause the test assertions to fail. I'm working on the fix for this.

[khushboobhatia01]

@cognifloyd All the tests are passing now. Could you please review and merge the request? Thanks

[cognifloyd]

LGTM I'm not sure if this will get merged for 3.6.0 or if we'll wait till 3.7.0 to include it.

[arm4b]

I remember there is another PR that is more involved related to changing the behavior in st2workflowengine #5367.
This one #5358 looks like an easy hotfix, tests are good and no blockers here. It would be really nice if we could include it for the v3.6.0.

@khushboobhatia01 Could you please include the Changelog record as well for this PR to make it complete?

[cognifloyd]

I added a changelog entry

[arm4b]

Thanks, looks we're all good ✔️ on this one!