From d199ecac80083e64d32baf3b473c67b11a6e6936 Mon Sep 17 00:00:00 2001
From: Nick Khyl <nickk@tailscale.com>
Date: Wed, 3 Dec 2025 19:54:52 -0600
Subject: [PATCH] ipn/ipnlocal: shut down old control client synchronously on
 reset

Previously, callers of (*LocalBackend).resetControlClientLocked were supposed
to call Shutdown on the returned controlclient.Client after releasing b.mu.
In #17804, we started calling Shutdown while holding b.mu, which caused
deadlocks during profile switches due to the (*ExecQueue).RunSync implementation.

We first patched this in #18053 by calling Shutdown in a new goroutine,
which avoided the deadlocks but made TestStateMachine flaky because
the shutdown order was no longer guaranteed.

In #18070, we updated (*ExecQueue).RunSync to allow shutting down
the queue without waiting for RunSync to return. With that change,
shutting down the control client while holding b.mu became safe.

Therefore, this PR updates (*LocalBackend).resetControlClientLocked
to shut down the old client synchronously during the reset, instead of
returning it and shifting that responsibility to the callers.

This fixes the flaky tests and simplifies the code.

Fixes #18052

Signed-off-by: Nick Khyl <nickk@tailscale.com>
---
 ipn/ipnlocal/local.go | 39 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index fbf34aa42..ce2acf311 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -948,12 +948,8 @@ func (b *LocalBackend) pauseOrResumeControlClientLocked() {
 // down, clients switch over to other replicas whilst the existing connections are kept alive for some period of time.
 func (b *LocalBackend) DisconnectControl() {
 	b.mu.Lock()
-	cc := b.resetControlClientLocked()
-	b.mu.Unlock()
-
-	if cc != nil {
-		cc.Shutdown()
-	}
+	defer b.mu.Unlock()
+	b.resetControlClientLocked()
 }
 
 // linkChange is our network monitor callback, called whenever the network changes.
@@ -2419,14 +2415,6 @@ func (b *LocalBackend) startLocked(opts ipn.Options) error {
 	logf := logger.WithPrefix(b.logf, "Start: ")
 	b.startOnce.Do(b.initOnce)
 
-	var clientToShutdown controlclient.Client
-	defer func() {
-		if clientToShutdown != nil {
-			// Shutdown outside of b.mu to avoid deadlocks.
-			b.goTracker.Go(clientToShutdown.Shutdown)
-		}
-	}()
-
 	if opts.UpdatePrefs != nil {
 		if err := b.checkPrefsLocked(opts.UpdatePrefs); err != nil {
 			return err
@@ -2469,7 +2457,7 @@ func (b *LocalBackend) startLocked(opts ipn.Options) error {
 	// into sync with the minimal changes. But that's not how it
 	// is right now, which is a sign that the code is still too
 	// complicated.
-	clientToShutdown = b.resetControlClientLocked()
+	b.resetControlClientLocked()
 	httpTestClient := b.httpTestClient
 
 	if b.hostinfo != nil {
@@ -5810,13 +5798,12 @@ func (b *LocalBackend) setControlClientLocked(cc controlclient.Client) {
 	b.ignoreControlClientUpdates.Store(cc == nil)
 }
 
-// resetControlClientLocked sets b.cc to nil and returns the old value. If the
-// returned value is non-nil, the caller must call Shutdown on it after
-// releasing b.mu.
-func (b *LocalBackend) resetControlClientLocked() controlclient.Client {
+// resetControlClientLocked sets b.cc to nil and shuts down the previous
+// control client, if any.
+func (b *LocalBackend) resetControlClientLocked() {
 	syncs.RequiresMutex(&b.mu)
 	if b.cc == nil {
-		return nil
+		return
 	}
 
 	b.resetAuthURLLocked()
@@ -5836,7 +5823,7 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client {
 	}
 	prev := b.cc
 	b.setControlClientLocked(nil)
-	return prev
+	prev.Shutdown()
 }
 
 // resetAuthURLLocked resets authURL, canceling any pending interactive login.
@@ -6930,10 +6917,7 @@ func (b *LocalBackend) resetForProfileChangeLocked() error {
 	b.updateFilterLocked(ipn.PrefsView{})
 	// Reset the NetworkMap in the engine
 	b.e.SetNetworkMap(new(netmap.NetworkMap))
-	if prevCC := b.resetControlClientLocked(); prevCC != nil {
-		// Shutdown outside of b.mu to avoid deadlocks.
-		b.goTracker.Go(prevCC.Shutdown)
-	}
+	b.resetControlClientLocked()
 	// TKA errors should not prevent resetting the backend state.
 	// However, we should still return the error to the caller.
 	tkaErr := b.initTKALocked()
@@ -7012,10 +6996,7 @@ func (b *LocalBackend) ResetAuth() error {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 
-	if prevCC := b.resetControlClientLocked(); prevCC != nil {
-		// Shutdown outside of b.mu to avoid deadlocks.
-		b.goTracker.Go(prevCC.Shutdown)
-	}
+	b.resetControlClientLocked()
 	if err := b.clearMachineKeyLocked(); err != nil {
 		return err
 	}