-
Notifications
You must be signed in to change notification settings - Fork 66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Honor all non-completion commands #569
Merged
dandavison
merged 6 commits into
main
from
sdk-528-let-coroutines-complete-before-setting-completion
Aug 5, 2024
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
bd7930c
Honor commands generated after the first completion command
dandavison 64ece8e
Add type annotation needed by mypy
dandavison 42d66db
Update core
dandavison ab55ee6
Add test that timer can be started after workflow completion
dandavison 8c5e9c3
Skip update tests under Java server
dandavison 7a847d5
Test replay backwards compatibility
dandavison File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Submodule sdk-core
updated
17 files
+1 −1 | Cargo.toml | |
+42 −15 | client/src/lib.rs | |
+10 −6 | client/src/raw.rs | |
+4 −3 | core/Cargo.toml | |
+3 −3 | core/src/abstractions.rs | |
+1 −1 | core/src/core_tests/determinism.rs | |
+87 −21 | core/src/core_tests/workflow_tasks.rs | |
+17 −7 | core/src/internal_flags.rs | |
+1 −10 | core/src/pollers/poll_buffer.rs | |
+3 −3 | core/src/test_help/mod.rs | |
+1 −4 | core/src/worker/activities/local_activities.rs | |
+1 −4 | core/src/worker/mod.rs | |
+7 −1 | core/src/worker/workflow/machines/workflow_machines.rs | |
+214 −14 | core/src/worker/workflow/managed_run.rs | |
+1 −11 | core/src/worker/workflow/mod.rs | |
+8 −0 | sdk-core-protos/src/history_builder.rs | |
+131 −5 | tests/integ_tests/client_tests.rs |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -310,3 +310,78 @@ def new_say_hello_worker(client: Client) -> Worker: | |
workflows=[SayHelloWorkflow], | ||
activities=[say_hello], | ||
) | ||
|
||
|
||
@workflow.defn | ||
class UpdateCompletionAfterWorkflowReturn: | ||
def __init__(self) -> None: | ||
self.workflow_returned = False | ||
|
||
@workflow.run | ||
async def run(self) -> str: | ||
self.workflow_returned = True | ||
return "workflow-result" | ||
|
||
@workflow.update | ||
async def my_update(self) -> str: | ||
await workflow.wait_condition(lambda: self.workflow_returned) | ||
return "update-result" | ||
|
||
|
||
async def test_replayer_command_reordering_backward_compatibility() -> None: | ||
""" | ||
The UpdateCompletionAfterWorkflowReturn workflow above features an update handler that returns | ||
after the main workflow coroutine has exited. It will (if an update is sent in the first WFT) | ||
generate a raw command sequence (before sending to core) of | ||
Comment on lines
+333
to
+335
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, great comment, makes sense 👍 |
||
|
||
[UpdateAccepted, CompleteWorkflowExecution, UpdateCompleted]. | ||
|
||
Prior to https://github.com/temporalio/sdk-python/pull/569, Python truncated this command | ||
sequence to | ||
|
||
[UpdateAccepted, CompleteWorkflowExecution]. | ||
|
||
With #569, Python performs no truncation, and Core changes it to | ||
|
||
[UpdateAccepted, UpdateCompleted, CompleteWorkflowExecution]. | ||
|
||
This test takes a history generated using pre-#569 SDK code, and replays it. This succeeds. | ||
The history is | ||
|
||
1 WorkflowExecutionStarted | ||
2 WorkflowTaskScheduled | ||
3 WorkflowTaskStarted | ||
4 WorkflowTaskCompleted | ||
5 WorkflowExecutionUpdateAccepted | ||
6 WorkflowExecutionCompleted | ||
|
||
Note that the history lacks a WorkflowExecutionUpdateCompleted event. | ||
|
||
If Core's logic (which involves a flag) incorrectly allowed this history to be replayed | ||
using Core's post-#569 implementation, then a non-determinism error would result. Specifically, | ||
Core would, at some point during replay, do the following: | ||
|
||
Receive [UpdateAccepted, CompleteWorkflowExecution, UpdateCompleted] from lang, | ||
change that to [UpdateAccepted, UpdateCompleted, CompleteWorkflowExecution] | ||
and create an UpdateMachine instance (the WorkflowTaskMachine instance already exists). | ||
Then continue to consume history events. | ||
|
||
Event 5 WorkflowExecutionUpdateAccepted would apply to the UpdateMachine associated with | ||
the UpdateAccepted command, but event 6 WorkflowExecutionCompleted would not, since | ||
core is expecting an event that can be applied to the UpdateMachine corresponding to | ||
UpdateCompleted. If we modify core to incorrectly apply its new logic then we do see that: | ||
|
||
[TMPRL1100] Nondeterminism error: Update machine does not handle this event: HistoryEvent(id: 6, WorkflowExecutionCompleted) | ||
|
||
The test passes because core in fact (because the history lacks the flag) uses its old logic | ||
and changes the command sequence from [UpdateAccepted, CompleteWorkflowExecution, UpdateCompleted] | ||
to [UpdateAccepted, CompleteWorkflowExecution], and events 5 and 6 can be applied to the | ||
corresponding state machines. | ||
""" | ||
with Path(__file__).with_name( | ||
"test_replayer_command_reordering_backward_compatibility.json" | ||
).open() as f: | ||
history = f.read() | ||
await Replayer(workflows=[UpdateCompletionAfterWorkflowReturn]).replay_workflow( | ||
WorkflowHistory.from_json("fake", history) | ||
) |
113 changes: 113 additions & 0 deletions
113
tests/worker/test_replayer_command_reordering_backward_compatibility.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
{ | ||
"events": [ | ||
{ | ||
"eventId": "1", | ||
"eventTime": "2024-08-02T23:35:00.061520Z", | ||
"eventType": "EVENT_TYPE_WORKFLOW_EXECUTION_STARTED", | ||
"taskId": "1049558", | ||
"workflowExecutionStartedEventAttributes": { | ||
"workflowType": { | ||
"name": "UpdateCompletionAfterWorkflowReturn" | ||
}, | ||
"taskQueue": { | ||
"name": "tq", | ||
"kind": "TASK_QUEUE_KIND_NORMAL" | ||
}, | ||
"workflowTaskTimeout": "10s", | ||
"originalExecutionRunId": "a32ce0cb-b50e-4734-b003-784dda811861", | ||
"identity": "[email protected]", | ||
"firstExecutionRunId": "a32ce0cb-b50e-4734-b003-784dda811861", | ||
"attempt": 1, | ||
"firstWorkflowTaskBackoff": "0s", | ||
"workflowId": "wf-dd1e2267-d1bf-4822-be38-2a97a499331e" | ||
} | ||
}, | ||
{ | ||
"eventId": "2", | ||
"eventTime": "2024-08-02T23:35:00.070867Z", | ||
"eventType": "EVENT_TYPE_WORKFLOW_TASK_SCHEDULED", | ||
"taskId": "1049559", | ||
"workflowTaskScheduledEventAttributes": { | ||
"taskQueue": { | ||
"name": "tq", | ||
"kind": "TASK_QUEUE_KIND_NORMAL" | ||
}, | ||
"startToCloseTimeout": "10s", | ||
"attempt": 1 | ||
} | ||
}, | ||
{ | ||
"eventId": "3", | ||
"eventTime": "2024-08-02T23:35:00.155562Z", | ||
"eventType": "EVENT_TYPE_WORKFLOW_TASK_STARTED", | ||
"taskId": "1049564", | ||
"workflowTaskStartedEventAttributes": { | ||
"scheduledEventId": "2", | ||
"identity": "[email protected]", | ||
"requestId": "b03f25fb-b2ab-4b93-b2ad-0f6899f6e2e2", | ||
"historySizeBytes": "260" | ||
} | ||
}, | ||
{ | ||
"eventId": "4", | ||
"eventTime": "2024-08-02T23:35:00.224744Z", | ||
"eventType": "EVENT_TYPE_WORKFLOW_TASK_COMPLETED", | ||
"taskId": "1049568", | ||
"workflowTaskCompletedEventAttributes": { | ||
"scheduledEventId": "2", | ||
"startedEventId": "3", | ||
"identity": "[email protected]", | ||
"workerVersion": { | ||
"buildId": "17647b02191ec9e4e58b623a9c71f20a" | ||
}, | ||
"sdkMetadata": { | ||
"coreUsedFlags": [ | ||
1, | ||
2 | ||
] | ||
}, | ||
"meteringMetadata": {} | ||
} | ||
}, | ||
{ | ||
"eventId": "5", | ||
"eventTime": "2024-08-02T23:35:00.242507Z", | ||
"eventType": "EVENT_TYPE_WORKFLOW_EXECUTION_UPDATE_ACCEPTED", | ||
"taskId": "1049569", | ||
"workflowExecutionUpdateAcceptedEventAttributes": { | ||
"protocolInstanceId": "my-update", | ||
"acceptedRequestMessageId": "my-update/request", | ||
"acceptedRequestSequencingEventId": "2", | ||
"acceptedRequest": { | ||
"meta": { | ||
"updateId": "my-update", | ||
"identity": "[email protected]" | ||
}, | ||
"input": { | ||
"name": "my_update" | ||
} | ||
} | ||
} | ||
}, | ||
{ | ||
"eventId": "6", | ||
"eventTime": "2024-08-02T23:35:00.258465Z", | ||
"eventType": "EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED", | ||
"taskId": "1049570", | ||
"workflowExecutionCompletedEventAttributes": { | ||
"result": { | ||
"payloads": [ | ||
{ | ||
"metadata": { | ||
"encoding": "anNvbi9wbGFpbg==", | ||
"encodingDecoded": "json/plain" | ||
}, | ||
"data": "workflow-result" | ||
} | ||
] | ||
}, | ||
"workflowTaskCompletedEventId": "4" | ||
} | ||
} | ||
] | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite following this bit of the question:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the replay backward compatibility test. This should be ready to go.