Fix child WF ID generation #1803

gow · 2025-02-05T07:04:24Z

What was changed

We now use workflowInfo.OriginalRunID in parent when it is generating a child workflow ID.

Why?

We need this change to enable resetting of workflows that have pending children. What this means is Go SDK should be able to successfully replay StartChildWorkflowExecutionInitiated event in a parent that was reset.
Without this change Go SDK will run into non-determinism error since the child ID it generates (based on new run ID) doesn't match with the ID available in the StartChildWorkflowExecutionInitiated event. Since OriginalRunID stays the same across resets, with this change the SDK can successfully replay StartChildWorkflowExecutionInitiated events.

Checklist

Closes
N/A
How was this tested:
Manually tested it. Added integration tests and replay tests.
Any docs updates needed?
N/A

cretz · 2025-02-05T14:23:18Z

Questions here:

Can we confirm what Java does here?
IIUC Core-based languages make child workflow IDs with a seeded random that changes seed on reset midway through execution, meaning post-reset child workflow IDs would be different than they were on original run but pre-reset ones are the same. This was done very much by intention. Should we consider a similar approach in Go/Java that has a "current run ID" that can change midway through execution?
Can we assess the impact of behavior changes like this on existing users with existing expectations? I am guessing there is no impact because you could not reset to before child workflows?
Can you confirm OriginalRunID always matches RunID at this point of code today? Meaning since resets were never allowed over child workflows, it is impossible to have these two values different at child workflow creation time today?

gow · 2025-02-05T21:16:28Z

Questions here:

Can we confirm what Java does here?

Java seems to be generating UUIDs when an ID is not provided in ChildWorkflow options. Additionally when I replayed an existing StartChildWorkflowExecutionInitiated event (by resetting to a point after the child completion) it didn't run into non-determinism error. Go on the other hand ran into non-det error complaining about workflowID. ("Error":"[TMPRL1100] unknown command CommandType: ChildWorkflow, ID: 0174afb2-0213-4644-9214-8b977310cc9f_14, possible causes are nondeterministic workflow definition code or incompatible change in the workflow definition")

IIUC Core-based languages make child workflow IDs with a seeded random that changes seed on reset midway through execution, meaning post-reset child workflow IDs would be different than they were on original run but pre-reset ones are the same. This was done very much by intention. Should we consider a similar approach in Go/Java that has a "current run ID" that can change midway through execution?

Absolutely yes. Java seems to be already doing this (at least in my testing). Looks like it changes the seed at reset-point?
If this is an easier change in Go-SDK, I'd rather do it. Otherwise, we still need this fix since Go-SDK is currently broken and the upcoming reset changes will increase the exposure to this bug (See below)

Can we assess the impact of behavior changes like this on existing users with existing expectations? I am guessing there is no impact because you could not reset to before child workflows?

Yes. So far all server versions have prevented resetting of workflows when the reset point falls in between child Init & Completed events (i.e within the lifespan of child). So no potential user impact or backward compatibility issues there.
However the server has always allowed workflows to be reset when the reset-point chosen is outside the child init & completed events. In cases where reset-point is after child complete event, workers with Go SDK run into non-det error while other SDKs seems to simply reapply events (child Init, Start & complete) without any issues. In that regard this PR is more of a bug fix in Go-SDK. And now with the upcoming server changes to allow reset-points to be in between Init-Completed, it's important that Go-SDK successfully replay a StartChildWorkflowExecutionInitiated event.

Can you confirm OriginalRunID always matches RunID at this point of code today? Meaning since resets were never allowed over child workflows, it is impossible to have these two values different at child workflow creation time today?

Yes it is possible to have these two values different at child creation time. When the reset-point is outside the child's lifespan (Init - Completed) then the server has always allowed resets. So in cases when the reset-point is before the child-init, the parent will try to create a child again and this time OriginalRunID is different from RunID.

cretz · 2025-02-05T22:53:03Z

@gow - thanks for this info! So reading all of that I am seeing two things:

We need to fix Go to be at least a little like other SDKs. We have two options:
1. Start offering a re-seeded random to users and we use it for child IDs in Go with an SDK flag to only apply to new workflows
2. Make available (maybe only to us) a "current run ID" that is updated when reset that is used for child workflow IDs (no SDK flag needed for compatibility)
We are not concerned with backwards compatibility of only changing Go since it never worked anyways after reset, so long as the non-reset logic remains working

For 1, I will start an internal discussion and invite you. Regardless, I am not sure the desired outcome is what is in this PR.

gow · 2025-02-05T23:10:58Z

The desired outcome of this PR is for Go-SDK to be able to replay StartChildWorkflowExecutionInitiated events like other SDKs. Currently it fails. Using OriginalRunID will fix this issue and unblock reset feature development. But I understand it doesn't bring Go-SDK to be fully be at parity with other SDKs.

gow · 2025-02-07T19:41:49Z

Putting this on hold. Will discuss internally and work on a better fix.

gow · 2025-02-12T23:53:32Z

After an internal discussion, we decided to bring Go SDK on par with the rest of the SDKs. So instead of simply using OriginalRunID, we are using the FirstExecutionRunId as the initial seed and updating it with the currentRunID whenever we encounter a reset.

…hild_workflow_id

yuandrew

overall LGTM, deferring to @Quinn-With-Two-Ns or @cretz for approval. I vaguely remember there was a test they were hoping was added, but can't remember exactly what that test was.

internal/internal_event_handlers.go

internal/internal_task_handlers_interfaces_test.go

gow

I vaguely remember there was a test they were hoping was added, but can't remember exactly what that test was.

We need to add some integration tests here. I'm trying to figure out the structuring and setup of them in this codebase. I'll probably sync with you all offline to get some pointers.

internal/internal_task_handlers_interfaces_test.go

internal/internal_event_handlers.go

cretz · 2025-02-13T16:05:39Z

internal/internal_task_handlers.go

@@ -492,8 +492,7 @@ OrderEvents:
 				break OrderEvents
 			}
 		case enumspb.EVENT_TYPE_WORKFLOW_TASK_SCHEDULED,
-			enumspb.EVENT_TYPE_WORKFLOW_TASK_TIMED_OUT,
-			enumspb.EVENT_TYPE_WORKFLOW_TASK_FAILED:


@Quinn-With-Two-Ns - can you think of any ramifications for now putting workflow task failed events in taskEvents.events where we hadn't before?

…hild_workflow_id

cretz

LGTM, though now we get into the more fun challenge of adding tests, heh

cretz · 2025-02-13T21:24:27Z

test/replaytests/replay_test.go

@@ -493,6 +493,24 @@ func (s *replayTestSuite) TestPartialReplayNonCommandEvent() {
 	require.NoError(s.T(), err)
 }

+func (s *replayTestSuite) TestResetWorkflowWithChildSuccess() {


These don't seem to have resets in them. Here's my best guess on some tests needed:

Capture a JSON of a before-this-PR-change workflow that completed that has a reset after child workflow, and confirm it replays properly

Capture a JSON of a before-this-PR-change workflow that completed that has a before-this-PR-change reset before child workflow, and confirm it replays properly because it created a whole new child workflow

Make workflow with two child workflows, run it to completion, reset to in-between those two child workflows and let that reset one run to completion too. Confirm both replay successfully. Confirm that the original workflow's first child ID (i.e. the pre-reset-point one) is the same as the new workflow's first child ID by looking at history. Confirm the original workflow's second child ID (i.e. the post-reset-point one) is not the same as the new workflow's second child ID.

Those are the primary three I can think of, @Quinn-With-Two-Ns probably knows more. And you may want more to test certain behaviors such as the fact that the default parent close policy of terminate does what is expected.

Sorry, I should have marked this as draft again. Still working on it. Just sent it to see the CI builds. Talked to @Quinn-With-Two-Ns and I'm adding the cases you mentioned except this one

Capture a JSON of a before-this-PR-change workflow that completed that has a reset after child workflow, and confirm it replays properly

Because before-this-PR-change is broken and doesn't replay properly. But reset before the child workflow was working. So adding that one.

I think Chads point here was even though it doesn't replay properly with the current SDK, your change should cause it to replay properly

Agree. I've added both cases.
Adding integration tests now.

So you should be able to add 3 test since you can test it

The third case he mentioned above is an integration tests isn't it?

It would be an integration test and a replay test. You have two scenarios:

Workflow was started with an old SDK, workflow had a child workflow and was reset after the child workflow finished. Currently that workflow is stuck due to NDE. Your change here should fix that workflow

Workflow started with a new SDK, workflow had a child workflow and was reset after the child workflow finished. Workflow should complete successfully

is a replay test, 2 is an integration test

Got it. Done.
I added 1 integration test resetting a WF at 3 different reset points. The parent starts 2 children in sequence.

Reset # 1 - before both children. Both have new IDs after reset.

Reset # 2 - In between child-1 & child-2. Child-1 ID stays same. Child-2 ID changes.

Reset # 3 - After both children. Both have the same IDs as before reset.

I think there are lot more tests we need to write related to resets in general that are outside the scope of this PR. We should probably add these in followup PRs just to increase coverage.

Reset a running parent.

Reset a completed (base) parent but with another execution already running.

Reset a running/completed parent that handles a failed child.

Reset a running/completed parent where the reset points are in between Child init and child completed events (coming up in next server version)

Quinn-With-Two-Ns · 2025-02-14T00:07:15Z

Also this TestNumPollersCounter test failure is not your fault. Seems to be very flaky after Go 1.24

cretz · 2025-02-14T16:12:06Z

test/integration_test.go

@@ -2040,6 +2040,101 @@ func (ts *IntegrationTestSuite) TestResetWorkflowExecution() {
 	ts.Equal(originalResult, newResult)
 }

+// TestResetWorkflowExecutionWithChildren tests the behavior of child workflow ID generation when a workflow with children is reset.
+// It repeatedly resets the workflow at different points in its execution and verifies that the child workflow IDs are generated correctly.
+func (ts *IntegrationTestSuite) TestResetWorkflowExecutionWithChildren() {


This is a great test

cretz

LGTM, but wait for @Quinn-With-Two-Ns to be able to look at test coverage and scenarios, he may be aware of more/better ones

gow requested a review from a team as a code owner February 5, 2025 07:04

gow requested review from cretz, yycptt and Quinn-With-Two-Ns February 5, 2025 07:04

gow marked this pull request as draft February 7, 2025 19:41

gow force-pushed the cg/reset_child_workflow_id branch from c54c992 to ab1a0c3 Compare February 12, 2025 06:31

gow changed the title ~~Use OriginalRunID instead of RunID to generate child workflow IDs~~ Fix seed child WF ID generation Feb 12, 2025

gow changed the title ~~Fix seed child WF ID generation~~ Fix child WF ID generation Feb 12, 2025

gow added 3 commits February 12, 2025 15:45

Use OriginalRunID instead of RunID to generate child workflow IDs

9520db2

Use first execution run

ccb73b2

Unskip EVENT_TYPE_WORKFLOW_TASK_FAILED in internal_task_handlers.go

f0eb17d

gow force-pushed the cg/reset_child_workflow_id branch from ab1a0c3 to f0eb17d Compare February 12, 2025 23:49

gow marked this pull request as ready for review February 12, 2025 23:52

gow added 3 commits February 12, 2025 15:57

Remove log

7a812be

Fix existing tests

1e60a0e

Merge branch 'master' of github.com:temporalio/sdk-go into cg/reset_c…

98f65da

…hild_workflow_id

yuandrew reviewed Feb 13, 2025

View reviewed changes

internal/internal_event_handlers.go Outdated Show resolved Hide resolved

internal/internal_task_handlers_interfaces_test.go Show resolved Hide resolved

gow commented Feb 13, 2025

View reviewed changes

internal/internal_task_handlers_interfaces_test.go Show resolved Hide resolved

internal/internal_event_handlers.go Outdated Show resolved Hide resolved

cretz reviewed Feb 13, 2025

View reviewed changes

gow added 2 commits February 13, 2025 09:20

Merge branch 'master' of github.com:temporalio/sdk-go into cg/reset_c…

0354986

…hild_workflow_id

Use Original Execution Run ID

3cbeb33

cretz reviewed Feb 13, 2025

View reviewed changes

Added replay tests

13fdd9a

cretz reviewed Feb 13, 2025

View reviewed changes

gow marked this pull request as draft February 13, 2025 21:43

Capture events with a reset point before/after child

cf0d13f

Add integration tests

985cba8

gow marked this pull request as ready for review February 14, 2025 02:01

Remove print

a6e46a3

cretz reviewed Feb 14, 2025

View reviewed changes

cretz approved these changes Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix child WF ID generation #1803

Fix child WF ID generation #1803

gow commented Feb 5, 2025 •

edited

Loading

cretz commented Feb 5, 2025 •

edited

Loading

gow commented Feb 5, 2025 •

edited

Loading

cretz commented Feb 5, 2025

gow commented Feb 5, 2025

gow commented Feb 7, 2025

gow commented Feb 12, 2025

yuandrew left a comment

gow left a comment

cretz Feb 13, 2025

cretz left a comment

cretz Feb 13, 2025 •

edited

Loading

gow Feb 13, 2025

Quinn-With-Two-Ns Feb 14, 2025

gow Feb 14, 2025

Quinn-With-Two-Ns Feb 14, 2025

gow Feb 14, 2025

Quinn-With-Two-Ns Feb 14, 2025

Quinn-With-Two-Ns Feb 14, 2025

gow Feb 14, 2025 •

edited

Loading

Quinn-With-Two-Ns commented Feb 14, 2025

cretz Feb 14, 2025

cretz left a comment

Fix child WF ID generation #1803

Are you sure you want to change the base?

Fix child WF ID generation #1803

Conversation

gow commented Feb 5, 2025 • edited Loading

What was changed

Why?

Checklist

cretz commented Feb 5, 2025 • edited Loading

gow commented Feb 5, 2025 • edited Loading

cretz commented Feb 5, 2025

gow commented Feb 5, 2025

gow commented Feb 7, 2025

gow commented Feb 12, 2025

yuandrew left a comment

Choose a reason for hiding this comment

gow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

cretz Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gow Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Feb 14, 2025

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

gow commented Feb 5, 2025 •

edited

Loading

cretz commented Feb 5, 2025 •

edited

Loading

gow commented Feb 5, 2025 •

edited

Loading

cretz Feb 13, 2025 •

edited

Loading

gow Feb 14, 2025 •

edited

Loading