Honor all non-completion commands

[dandavison]

Fixes #528

With this change we honor all non-completion commands emitted by workflow coroutines, even if they come after a completion command (i.e. complete/CAN/cancel/fail). A consequence is that when an update completion is returned in the same WFT response as a workflow completion, the client will always get the update response; previously that was only the case if the update handler returned prior to any completion command being emitted by another coroutine.

The solution involves devolving responsibilites for this logic to core: see explanatory code comments in temporalio/sdk-core#776.

Evidence that this is correct

$ pytest -k "completion" tests/worker/test_workflow.py

# These two fail on main
tests/worker/test_workflow.py::test_update_completion_is_honored_when_after_workflow_return_1 PASSED
tests/worker/test_workflow.py::test_update_completion_is_honored_when_after_workflow_return_2 PASSED

# These two check that we respect ordering of completion commands. They pass on main as well as with the new changes.
tests/worker/test_workflow.py::test_first_of_two_signal_completion_commands_is_honored PASSED                                                                                                                
tests/worker/test_workflow.py::test_workflow_return_is_honored_when_it_precedes_signal_completion_command PASSED

[cretz]

LGTM, but want to wait until temporalio/sdk-core#776 settles and the submodule can be updated to master.

[cretz]

Any concerns that removing this is backwards incompatible w/ already completed workflows, or can you confirm that in no-flag-replaying situations the core behavior was always the same (sans query stuff)? One thing you can do is make a workflow that has post-complete command, run it in older SDK, grab JSON history, and run replayer in tests here with new code.

[dandavison]

I'm not quite following this bit of the question:

or can you confirm that in no-flag-replaying situations the core behavior was always the same

Here's how I am thinking of it:

• sdk-python v1.0 was released in Jan 2023, and dropped post-terminal commands from the beginning, until this change.

• Therefore, prior to this change, all Python WFTs had their post-terminal commands dropped.

• Incidentally, Core started also dropping post-terminal commands since March 2023: Drop all post-terminal commands & sort activation jobs sdk-core#502

• The new SDK code drops post-terminal commands when replaying without the flag set, and there is test coverage for this: https://github.com/temporalio/sdk-core/blob/master/core/src/core_tests/workflow_tasks.rs#L2558-L2577. Therefore we do not expect NDEs: the command sequence applied to core state machines when replaying without the flag will be the same as it was prior to this change.

[cretz]

I'm not quite following this bit of the question:

Think about a user with an old workflow (i.e. sans flag). If you remove the old Python behavior that runs sans flag, it now relies on the old Core behavior sans flag. If that old behavior doesn't match Python's old behavior, they will get a non-determinism error. So we need to confirm that old Core code does the same thing as old Python code before removing old Python code. Did they drop post-terminal commands the same way? If so, we're all good here.

The new SDK code drops post-terminal commands when replaying without the flag set, and there is test coverage for this

IMO you should grab a workflow history JSON or two from a workflow that had post-terminal commands from a Python SDK before this change, then run it through a replayer in the test on this version. There's a couple of other JSON files in the test suite that you can see how their tests are doing this. Also, I assume the test in this PR is testing that now commands after workflow complete are properly included?

[dandavison]

Think about a user with an old workflow (i.e. sans flag). If you remove the old Python behavior that runs sans flag, it now relies on the old Core behavior sans flag. If that old behavior doesn't match Python's old behavior, they will get a non-determinism error. So we need to confirm that old Core code does the same thing as old Python code before removing old Python code. Did they drop post-terminal commands the same way? If so, we're all good here.

Personally I would substitute s/old Core/new Core/ throughout this paragraph, since we're never going to be running old Core code: rather it's new Core code which, when replaying without the flag, is intended to behave as old Core did (i.e. truncating at first terminal command). This is tested in two different ways in the Core test suite, but I agree that SDK-specific tests replaying old workflows with post-terminal commands would be good too.

[cretz]

👍 Makes sense, yeah whatever the terms are that mean "Workflows with post-complete commands on previous Python SDK versions work the exact same with this PR"

[dandavison]

Added the replay backward compatibility test. This should be ready to go.

[Sushisource]

Yeah, great comment, makes sense 👍