Skip to content

Conversation

@yuandrew
Copy link
Contributor

@yuandrew yuandrew commented Jun 5, 2025

What was changed

Plumb WorkerStopChannel through for Local Activities

Why?

Looks like this was never implemented to begin with

Checklist

  1. Closes GetWorkerStopChannel no longer indicates worker shutdown for Local Activities #1963

  2. How was this tested:

Changed a few tests to use this to avoid a race condition

  1. Any docs updates needed?

@yuandrew yuandrew requested a review from a team as a code owner June 5, 2025 00:12
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
localActivityFn := func(ctx context.Context) error {
// There is a slight delay between when workerStopChannel closes and when backgroundContextCancel is canceled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the delay should be the graceful shutdown time no?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workerStopChannel should be closed during graceful shutdown and then the context should be cancelled when the graceful period elapse or the shutdown finished

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, looking at the graceful shutdown case, there is no delay.

but looking at TestLocalActivityWorkerShutdownNoHeartbeat, which I realized is non-graceful shutdown, we need a sleep. Otherwise, both LAs are able to complete before shutdown stops the LA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm for graceful shutdown there should be a delay between the channel closing and the context closing. The context shouldn't close during the graceful shutdown period

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was getting confused because the tests are doing different things. Added some comments to the tests to hopefully avoid any future confusion

For the non-graceful test, we want the LA to return context canceled err in order to test that we don't heartbeat on failure.

For the graceful tests, we want the LA to wait for shutdown to start before we complete, so when ctx is canceled doesn't matter in these tests

The context shouldn't close during the graceful shutdown period

I've separately confirmed that this is true.

@yuandrew yuandrew merged commit a1a14b9 into temporalio:master Jun 26, 2025
13 checks passed
@yuandrew yuandrew deleted the local-activity-worker-stop-c branch June 26, 2025 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GetWorkerStopChannel no longer indicates worker shutdown for Local Activities

2 participants