[Feature Request] SDK should not return an update handle if the update has not reached the desired state

[Quinn-With-Two-Ns]

Is your feature request related to a problem? Please describe.

When users make a request to update a workflow execution they specify the update life cycle stage they would like to wait for before returning. Depending on the SDK this is specified either directly through an option or through the SDK API they use. Currently the SDK just passes this option to the gRPC call, but that is not sufficient since the gRPC call can return early if the server encounters an internal timeout. If the server encounters an internal timeout it will return an empty response and the SDK should poll. Currently the SDK returns an update handle if the server returns an empty response. This is dangerous since updates are not durable until accepted so it is possible to return a handle to a non durable update that can be lost.

Solution

If the server returns an update not in the desired life cycle stage, the SDK should start polling the update request until it reaches the desired life cycle stage before returning the handle to the user.

Client places:

• Go - [Feature Request] SDK should not return an update handle if the update has not reached the desired state sdk-go#1414
• CLI - Should be covered by the Go SDK
• Java - [Feature Request] SDK should not return an update handle if the update has not reached the desired state sdk-java#2002
• TypeScript - [Feature Request] SDK should not return an update handle if the update has not reached the desired state  sdk-typescript#1372
• Python - [Feature Request] SDK should not return an update handle if the update has not reached the desired state sdk-python#485
• .NET - [Feature Request] SDK should not return an update handle if the update has not reached the desired state sdk-dotnet#199

[cretz]

To clarify, we originally thought it would be best to have the handle contain a wait-for-stage call that we ourselves could leverage inside the start call, but since "admitted" is not supported, there's not much value at this time.

[Quinn-With-Two-Ns]

Talking with @mfateev about this, he brought up the server should return the Updates Request current life cycle state instead of an empty response when the servers internal deadline is reached. If the server returns the current life cycle state I purpose the logic should be:

If the Update Request reaches or passes the desired lifecycle state:

• Return the update handle to the user

else If the server hits an internal deadline while waiting for the desired lifecycle state it returns a response with the current life cycle state:

• If the Update Request state is ADMITTED
  • SDK should retry the request since the update request it could be lost. (SDK must be supplying an update ID in this case)
• else If the Update Request is ACCEPTED
  • Poll until the update is COMPLETED

There is also a potential optimization we can make for low latency use cases by allowing users to specify two different WaitStages, one for the initial update request and one for polling. This would allow a users to say that if an update completes quickly they just want to wait for completed, but if it is taking longer then expected just return after it is accepted.

[cretz]

he brought up the server should return the Updates Request current life cycle state instead of an empty response when the servers internal deadline is reached

I thought it already did this

If the server returns the current life cycle state I purpose the logic should be

I propose similar, but stated simply: "SDK should wait for user-provided lifecycle stage or ACCEPTED by default. ADMITTED is not a valid lifecycle stage the user can give at this time (which means it will always wait for ACCEPTED or COMPLETED today)."

[Quinn-With-Two-Ns]

I thought it already did this

It does not, I confirmed

I propose similar, but stated simply: "SDK should wait for user-provided lifecycle stage or ACCEPTED by default. ADMITTED is not a valid lifecycle stage the user can give at this time (which means it will always wait for ACCEPTED or COMPLETED today)."

Users cannot give ADMITTED correct, but the server could still return ADMITTED if the update is in that stage when it reaches the deadline. At that point we have two options,

1. poll and hope the update doesn't get lost
2. retry and know the update won't get lost.

[cretz]

It does not, I confirmed

Documentation at https://github.com/temporalio/api/blob/6776e414d82c7f469e470caa1a60b22e88231342/temporal/api/workflowservice/v1/request_response.proto#L1251-L1261 says it sets this value on timeout and the poll response has this too. I wonder if there's just a server version mismatch compared to API.

At that point we have two options,

1. poll and hope the update doesn't get lost
2. retry and know the update won't get lost.

Hrmm, I was thinking option 1 would be best here, but now I see what you're saying. So you're saying never poll on start, always continually call the start call because the start call with the same ID is the same as polling?

[Quinn-With-Two-Ns]

The documentation aligns with what I am saying

    // UNSPECIFIED will be returned if and only if the server's maximum wait
    // time was reached before the Update reached the stage specified in the
    // request WaitPolicy, and before the context deadline expired; clients may
    // may then retry the call as needed.

Yes just keep calling start until the update is durable then we can poll

[cretz]

I see, I thought "known to have reached" applied regardless (I didn't read far enough, sorry). I think we should change the current behavior and docs. It should never return UNSPECIFIED IMO, so we're all in agreement I think here.

[alexshtin]

I am going to fix "empty response return" to include actually reached state (ADMITTED or ACCEPTED) although I propose not to rely on it's value. I'm also making server to return specific error if update is lost and server knows about it. Roughly internals of UpdateWorkflow should be:

Step 1: Call to UpdateWorkflowExecution API. If it returns:

• non-retryable error => return error to the caller
• "update is lost" error => repeat Step 1
• non-empty response => return it to the caller
• empty response => start polling (Step 2)

Step 2: Call PollWorflowExecutionUpdate. If it returns:

• non-retryable error => return error to the caller
• "update is lost" or NotFound error => go to Step 1
• non-empty response => return it to the caller
• empty response => continue polling (go to Step 2)

Timeout needs special tricky handling.

[cretz]

@alexshtin - This can't work exactly as stated, I have started an internal discussion.