Fix LA start to close timeout #1180

Quinn-With-Two-Ns · 2023-07-28T13:49:17Z

This PR fixes a few issues with local activities in Got

ScheduleToCloseTimeout was overriding StartToCloseTimeout if both were set
The docs in LocalActivityOptions were not accurate
If a local activity did timeout it would always be considered a SCHEDULE_TO_CLOSE timeout and that is not retry able
ActivityInfo in local activities was missing some fields

Quinn-With-Two-Ns · 2023-07-28T13:52:03Z

internal/activity.go

+		scheduleToCloseTimeout = startToCloseTimeout
+	}
+	deadline := calculateActivityDeadline(task.scheduledTime, startedTime, scheduleToCloseTimeout, startToCloseTimeout)
+	if task.attempt > 1 && !task.expireTime.IsZero() && task.expireTime.Before(deadline) {


This is still an open issue if we should removed this/can remove this. This expireTime is calculated based on the ScheduleToCloseTimeout and the workflow time, but is only used on the second attempt. This has been basically unchanged for 4+ years, but seems wrong.

While wrong, let's leave for now

Yeah I agree, just wanted to call this out since when we put Go on Core this will be something we have to deal with.

internal/activity.go

internal/internal_event_handlers.go

cretz · 2023-07-31T13:32:22Z

internal/internal_task_pollers.go

-	timeout := task.params.ScheduleToCloseTimeout
-	if task.params.StartToCloseTimeout != 0 && task.params.StartToCloseTimeout < timeout {
-		timeout = task.params.StartToCloseTimeout
-	}
-	timeoutDuration := timeout
-	deadline := time.Now().Add(timeoutDuration)
-	if task.attempt > 1 && !task.expireTime.IsZero() && task.expireTime.Before(deadline) {
-		// this is attempt and expire time is before SCHEDULE_TO_CLOSE timeout
-		deadline = task.expireTime
-	}


I don't see anything obvious, but are there any ways that you can think of that this logic has changed with your new logic?

The only change here is using the scheduled time + ScheduleToCloseTimeout when calculating the deadline (used to be time.Now). It probably won't effect many activities because for the first attempt scheduled time will be close to time.Now and subsequent attempts task.expireTime uses workflow time so it will always be shorter than either deadline

So if today I have a schedule to close timeout of 5s, my second attempt would get the full 5s (even though that's improper)? But after this change, it will only get however much time remains the beginning of the first attempt? Sorry I didn't dig into how expireTime is set. If this is the case, I may hit you up off-PR to communicate with one or two known heavy local activity users to confirm this is acceptable.

So if today I have a schedule to close timeout of 5s, my second attempt

Today you would have no second attempt because schedule to close is not retried

Confirmed off-PR that second-attempt after first-attempt-immediate-failure is bounded today by expireTime regardless.

cretz · 2023-07-31T13:33:11Z

internal/internal_task_pollers.go

@@ -644,7 +635,11 @@ WaitResult:
 			metricsHandler.Counter(metrics.LocalActivityExecutionCanceledCounter).Inc(1)
 			return &localActivityResult{err: ErrCanceled, task: task}
 		} else if ctx.Err() == context.DeadlineExceeded {
-			return &localActivityResult{err: ErrDeadlineExceeded, task: task}
+			if task.params.ScheduleToCloseTimeout != 0 && time.Now().After(info.scheduledTime.Add(task.params.ScheduleToCloseTimeout)) {


I wish there was a better way to know which timeout was hit instead of having to do math here.

Quinn-With-Two-Ns · 2023-07-31T13:52:50Z

internal/activity.go

@@ -156,11 +156,13 @@ type (
 	// LocalActivityOptions stores local activity specific parameters that will be stored inside of a context.
 	LocalActivityOptions struct {
 		// ScheduleToCloseTimeout - The end to end timeout for the local activity including retries.
-		// This field is required.
+		// At least on of ScheduleToCloseTimeout or StartToCloseTimeout is required.
+		// defaults to StartToCloseTimeout if not set.


The default for ScheduleToCloseTimeout for normal activities is infinity. We could be consistent here, but that might be a breaking change.

Quinn-With-Two-Ns commented Jul 28, 2023

View reviewed changes

Quinn-With-Two-Ns marked this pull request as ready for review July 28, 2023 17:03

Quinn-With-Two-Ns requested a review from a team as a code owner July 28, 2023 17:03

cretz reviewed Jul 31, 2023

View reviewed changes

Quinn-With-Two-Ns commented Jul 31, 2023

View reviewed changes

Quinn-With-Two-Ns added 2 commits August 2, 2023 07:54

Fix LA start to close timeout

260ef43

Respond to pr comments

0e260ef

Quinn-With-Two-Ns force-pushed the local-activity-timeout branch from e305c8d to 0e260ef Compare August 2, 2023 16:03

cretz approved these changes Aug 2, 2023

View reviewed changes

Quinn-With-Two-Ns merged commit 1ec43ad into temporalio:master Aug 2, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix LA start to close timeout #1180

Fix LA start to close timeout #1180

Quinn-With-Two-Ns commented Jul 28, 2023

Quinn-With-Two-Ns Jul 28, 2023

cretz Jul 31, 2023

Quinn-With-Two-Ns Aug 2, 2023

cretz Jul 31, 2023

Quinn-With-Two-Ns Jul 31, 2023

cretz Jul 31, 2023 •

edited

Loading

Quinn-With-Two-Ns Jul 31, 2023 •

edited

Loading

cretz Jul 31, 2023

cretz Jul 31, 2023

Quinn-With-Two-Ns Jul 31, 2023

Fix LA start to close timeout #1180

Fix LA start to close timeout #1180

Conversation

Quinn-With-Two-Ns commented Jul 28, 2023

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

cretz Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Quinn-With-Two-Ns Jul 31, 2023 • 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

cretz Jul 31, 2023 •

edited

Loading

Quinn-With-Two-Ns Jul 31, 2023 •

edited

Loading