Avoid losing shutdown RPC handle - #1261
Open
yuandrew wants to merge 5 commits into
Open
Conversation
jmaeagle99
reviewed
May 13, 2026
| // The handle is stored and awaited in shutdown() to ensure completion. | ||
| let mut guard = self.shutdown_rpc_handle.lock(); | ||
| if guard.is_some() || already_initiated_shutdown { | ||
| if shutdown_rpc_handle.is_some() || already_initiated_shutdown { |
Contributor
There was a problem hiding this comment.
Should this short circuit be move up just under the lock acquisition so that other callers do not do the same checks and logging?
jmaeagle99
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
shutdown()callers all wait for the same in-flightShutdownWorkerRPC.Why?
Graceful poll shutdown requires the
ShutdownWorkerRPC to complete before Core waits for local polls to drain.Before, concurrent
initiate_shutdown()andshutdown()calls could expose the shutdown state before the RPC handle was stored. Concurrentshutdown()calls could also race to take the handle, allowing one caller to begin draining polls without waiting for the RPC.If the caller holding the join handle was cancelled, dropping the Tokio
JoinHandledetached the RPC task, leaving subsequent callers without a handle to await. A shared completion signal now lets those callers wait for the original RPC.Without this ordering, shutdown can stall until server poll timeouts and can recreate the graceful-poll shutdown deadlock this RPC is intended to prevent.
Original report: #1255 (comment)
Checklist
Closes
How was this tested:
wrote a few tests that verify that concurrent and replacement shutdown callers wait for the same single in-flight ShutdownWorker RPC, even when the caller owning its join handle is cancelled.
Note
Medium Risk
Changes ordering and synchronization on the core worker shutdown path that graceful poll shutdown depends on; risk is mitigated by focused regression tests but incorrect behavior could still stall shutdown until poll timeouts.
Overview
Fixes races where concurrent
shutdown()/initiate_shutdown()could start poll draining before theShutdownWorkerRPC finished, or lose the RPC entirely when the task that held itsJoinHandlewas cancelled.Worker shutdown replaces the single optional RPC join handle with
ShutdownRpcState(not started vs started, handle may already be taken), aCancellationTokenthat fires when the RPC task completes (via drop guard), and ashutdown_completeflag so repeatshutdown()calls are safe and serialized. Callers that do not get the handle still block on that completion signal before draining pollers.Tests add blocked-RPC scenarios for concurrent shutdown callers and for aborting the first
shutdown()waiter; one activity shutdown test now assertsPollError::ShutDownand awaits the shutdown future instead of only draining the poller.Reviewed by Cursor Bugbot for commit 684ced8. Bugbot is set up for automated code reviews on this repo. Configure here.