fix(core): Overhaul worker shutdown to make sure all tasks are executed before terminating by maciejdudko · Pull Request #1704 · temporalio/sdk-rust

maciejdudko · 2026-10-26T19:17:40Z

What was changed

Changes in worker shutdown:

Worker now drains the Workflow stream completely before shutting down. This was already the behavior for Activity and Nexus pollers.
Made Poller::poll take &mut self to forbid concurrent polls.
Removed Poller::notify_shutdown as it was redundant with worker shutdown token.
When LongPollBuffer stops, it yields a single Some(PollerError::PollerStopped) before yielding None. This replaces inserting dead poller message in the wrapping streams.
ShutdownWorker RPC will retry for as long as pollers are still polling, to avoid long shutdown on transient RPC failure.
Replaced LocalInputs::BumpStream with WFStreamInput::CheckShutdown that doesn't clog the WFT input stream.
Eager workflow tasks are no longer dropped if worker shutdown started before processing server response.

Other changes in implementation:

Fixed a race condition in ClosableMeteredPermitDealer.
Added ThrottleRetryConfigForCall and RetryCallback request extensions for use by ShutdownWorker RPC.
Fixed tracing span propagation in workflows.
SlotKind trait now has bounds Send + Sync + 'static in the definition so that they don't have to be added ad-hoc.
Removed ExternalPollerInputs type since it's now redundant.

Changes in tests:

Replaced make_wft_stream_interminable with keep_wft_pending_until_worker_shutdown because interminable WFT stream now blocks shutdown.
Added setup_mock_worker_client and setup_manual_mock_worker_client to simplify overriding default mock client behavior.
Fixed a large number of tests that were broken by the changes or that had existing bugs that were exposed by the changes.

Why?

This PR addresses a number of bugs in worker shutdown procedure that caused lost tasks or lost task permits on shutdown, mostly caused by not draining the workflow task poller before finalizing shutdown. This was most prominent in a large number of spurious test failures when running locally, but it also has potential to impact users in real world production scenarios.

Checklist

Closes Activity poll can hang during shutdown when ShutdownWorker returns UNIMPLEMENTED #1297 (note this is only one of the problems being fixed, the others don't have an issue open).
How was this tested:

Unit tests and integration tests were adapted to the new behavior.

…ed before terminating

maciejdudko · 2026-10-27T17:03:52Z

CI timed out:

test core_tests::activity_tasks::eviction_completion_during_shutdown_does_not_panic has been running for over 60 seconds

This is a spurious failure, didn't happen in the first run - may have introduced a new concurrency bug. I'm going to investigate. But the PR is still reviewable.

Sushisource · 2026-10-27T18:24:28Z

    // p5 outstanding and final poll outstanding -- hence one permit available
    assert_eq!(core.available_wft_permits(), Some(1));
    assert_eq!(core.cached_workflows().await, 3);
+    // TODO: the worker is leaking, we should complete all activations and call shutdown.


Leftover todo?

Not leftover, intentional. It's a very complex test and I decided to punt on it for now. The leak is not related to PR changes, it was always doing that.

Sushisource · 2026-10-27T18:58:46Z

+        shutdown_token
+            .into_iter()
+            .for_each(CancellationToken::cancel);


Nit: Could just be an if-let? Or I guess maybe that's annoying with the cfg

Sushisource · 2026-10-27T19:01:22Z

+                        // However, we want to avoid plowing ahead until we've been asked to poll at
+                        // least once. This supports activity-only workers.


Nothing to do with your changes but I'm realizing some earlier comment must've been dropped at some point and now this reads strangely.

Suggested change

// However, we want to avoid plowing ahead until we've been asked to poll at

// least once. This supports activity-only workers.

// We want to avoid plowing ahead until we've been asked to poll at

// least once. This supports activity-only workers.

Sushisource · 2026-10-27T19:02:18Z


-    /// Send a `BumpStream` message to the workflow stream. This ensures that any pending
-    /// workflow activation polls will resolve, like during shutdown, even if there are no other inputs.
+    /// Populates the workflow stream with no-op message. This ensures that any pending workflow


Does it actually "populate" it now though? Since it doesn't send it as an input?

Suggested change

/// Populates the workflow stream with no-op message. This ensures that any pending workflow

/// Wakes the workflow stream with no-op message. This ensures that any pending workflow

Sushisource · 2026-10-27T19:02:55Z

+        #[allow(dead_code)]
        rc: Arc<HistfetchRC>,


I think this works?

Suggested change

#[allow(dead_code)]

rc: Arc<HistfetchRC>,

_rc: Arc<HistfetchRC>,

Sushisource · 2026-10-27T19:37:35Z

+        let max_duration = Duration::from_secs(70); // matches temporalio_client::LONG_POLL_TIMEOUT
+        let retry_options = RetryOptions::builder()
+            .initial_interval(Duration::from_secs(1))
+            .multiplier(2.0)


Not sure we'd actually want to back off that much?

This matches our default throttle backoff config. I did add max_interval(Duration::from_secs(10)) though.

More I meant the multiplier. I'm wondering if we want to just use 1.0 here, or 1.1. Mostly b/c I'm thinking of the case where the first RPC error is just a blip, we save a whole second on shutdown. Not a huge deal either way though.

Sushisource · 2026-10-27T19:38:57Z

+                    #[cfg(debug_assertions)]
+                    if let Err(e) = res && let Ok(e) = e.try_into_panic() {
+                        // In debug build, propagate panics.
+                        if cfg!(debug_assertions) {


The else branch of this can never run since the entire block is #[cfg(debug_assertions)]

Sushisource · 2026-10-27T19:43:08Z

@@ -331,24 +328,35 @@ where
        self.outstanding_permits.fetch_add(1, Ordering::Release);


Does these also need to become SeqCst? How did you determine that was required?

I think there's actually still a race here too:

A reads close_requested == false.

B calls close(), sees 0 outstanding permits, and cancels the token.

A increments the count and gets a permit.

At that point close_complete has already fired and the eager-activity channel has been closed, so add_tasks fails and the eager activity is dropped. Seems we need to fetch_add first, then check close_requested, and if it is set, decrement again and return error.

Yeah, I think you're right. Need to lock them behind a single mutex.

Sushisource · 2026-10-27T22:49:29Z

        }
-        false
+
+        if !self.is_poller_stopped {


Because we now wait completely for the workflow pollers to stop, there's a problem where we could hang forever if we lose connection to the server. This can happen because the client (can) internally retries the errors over and over. We can add a regression test that proves this with mock clients I think.

We probably need a way to set a shutdown token for a retrying client.

I fixed the problem using a semaphore rather than a token so that it doesn't look like we're getting fatal error on shutdown.

Sushisource · 2026-10-27T22:53:10Z

+        handle_task_panic(self.poller_task, None).await;
    }



This should still call the shutdown token I think, which is why it was originally stored in self. It looks like all callers are doing this, but, it seems more maintainable to keep that in here too.

maciejdudko · 2026-11-03T09:10:10Z

Pushed a commit that addresses all review feedback. Unfortunately there are merge conflicts that will take some effort to resolve, as both this PR and main branch made significant changes to the combined normal/sticky WFT poller.

Additionally, the eviction_completion_during_shutdown_does_not_panic failure is an actual bug. The new implementation has a regression where it's again possible for a race condition to occur between finalizing shutdown and completing evictions when ignore_evictions_on_shutdown is true. Haven't finished the fix.

Sushisource · 2026-11-03T19:23:51Z

Pushed a commit that addresses all review feedback. Unfortunately there are merge conflicts that will take some effort to resolve, as both this PR and main branch made significant changes to the combined normal/sticky WFT poller.

Additionally, the eviction_completion_during_shutdown_does_not_panic failure is an actual bug. The new implementation has a regression where it's again possible for a race condition to occur between finalizing shutdown and completing evictions when ignore_evictions_on_shutdown is true. Haven't finished the fix.

Does fixing #1704 not fix that test?

LLMs are pretty great at resolving merge conflicts too, would definitely try to lean on them there.

Sushisource · 2026-11-04T00:04:58Z

@maciejdudko I think this would fix #1753

Plz check if it does and we can close at the same time

fix(core): Overhaul worker shutdown to make sure all tasks are execut…

d1e5a27

…ed before terminating

maciejdudko requested a review from a team as a code owner October 26, 2026 19:17

maciejdudko mentioned this pull request Oct 27, 2026

Handle unavailable ShutdownWorker during graceful poll shutdown #1298

Closed

Changelog and formatting

a7fd763

Sushisource self-assigned this Oct 27, 2026

Sushisource self-requested a review October 27, 2026 22:29

Sushisource reviewed Oct 27, 2026

View reviewed changes

Addressed PR comments

d1e4a71

		// However, we want to avoid plowing ahead until we've been asked to poll at
		// least once. This supports activity-only workers.

	/// Populates the workflow stream with no-op message. This ensures that any pending workflow
	/// Wakes the workflow stream with no-op message. This ensures that any pending workflow

	#[allow(dead_code)]
	rc: Arc<HistfetchRC>,
	_rc: Arc<HistfetchRC>,

		@@ -331,24 +328,35 @@ where
		self.outstanding_permits.fetch_add(1, Ordering::Release);

Conversation

maciejdudko commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

maciejdudko commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maciejdudko commented Nov 3, 2026

Uh oh!

Sushisource commented Nov 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sushisource commented Nov 4, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maciejdudko commented Oct 26, 2026 •

edited

Loading

maciejdudko commented Oct 27, 2026 •

edited

Loading

Sushisource commented Nov 3, 2026 •

edited

Loading