Fix possible NDEs caused by LAs & immediate cancels resolving in different order upon replay #808
Conversation
| Attributes::WorkflowExecutionStartedEventAttributes(_) => { EventType::WorkflowExecutionStarted } | ||
| Attributes::WorkflowExecutionCompletedEventAttributes(_) => { EventType::WorkflowExecutionCompleted } |
There was a problem hiding this comment.
Rustfmt decided to go hard on some stuff I guess
| /// 6. local activity resolutions | ||
| /// 7. queries |
There was a problem hiding this comment.
So Python still does its own job ordering: https://github.com/temporalio/sdk-python/blob/4aef4bfda19e56c7e7abac36b69e98f60861468c/temporalio/worker/_workflow_instance.py#L343-L356. But it's more primitive so this should still work (it's idea of 5 and 6 is put together). The problem is where we tick and run conditions is each "job set" so we cannot easily remove the Python logic. We probably do need to make sure to handle workflow init first. It hasn't even been updated to handle that message I don't think.
There was a problem hiding this comment.
Yes we should refactor it to work the way I describe in the one comment in here / the way it now is in TS, but yes that'll take a little time.
What was changed
Always resolve LAs last within one activation
I am confident this does not need a flag for the reasons explained in the docstring for the test. If you were in this situation before, you were experiencing UB that would likely lead to an NDE. The only situation this change impacts is exactly that situation. The test docstring explains why you can't force something else to happen.
However, we should still test this change in lang SDKs with the fuzzer before releasing them with it.
Why?
Well explained by docstrings
Checklist
Closes [Bug] Local activity combined with cancellation of something that doesn't wait can get out of order on replay #803
How was this tested:
Added reproing test
Any docs updates needed?