Revert "ipn/ipnlocal: shut down old control client synchronously on reset"

It appears (*controlclient.Auto).Shutdown() can still deadlock when called with b.mu held, and therefore the changes in #18127 are unsafe.

This reverts #18127 until we figure out what causes it.

This reverts commit d199ecac80.

Signed-off-by: Nick Khyl <nickk@tailscale.com>

ipn/ipnlocal/local.go

@@ -950,8 +950,12 @@ 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()
-	defer b.mu.Unlock()
-	b.resetControlClientLocked()
+	cc := b.resetControlClientLocked()
+	b.mu.Unlock()
+
+	if cc != nil {
+		cc.Shutdown()
+	}
 }
 
 // linkChange is our network monitor callback, called whenever the network changes.
@@ -2417,6 +2421,14 @@ 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
@@ -2459,7 +2471,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.
-	b.resetControlClientLocked()
+	clientToShutdown = b.resetControlClientLocked()
 	httpTestClient := b.httpTestClient
 
 	if b.hostinfo != nil {
@@ -5812,12 +5824,13 @@ func (b *LocalBackend) setControlClientLocked(cc controlclient.Client) {
 	b.ignoreControlClientUpdates.Store(cc == nil)
 }
 
-// resetControlClientLocked sets b.cc to nil and shuts down the previous
-// control client, if any.
-func (b *LocalBackend) resetControlClientLocked() {
+// 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 {
 	syncs.RequiresMutex(&b.mu)
 	if b.cc == nil {
-		return
+		return nil
 	}
 
 	b.resetAuthURLLocked()
@@ -5837,7 +5850,7 @@ func (b *LocalBackend) resetControlClientLocked() {
 	}
 	prev := b.cc
 	b.setControlClientLocked(nil)
-	prev.Shutdown()
+	return prev
 }
 
 // resetAuthURLLocked resets authURL, canceling any pending interactive login.
@@ -6931,7 +6944,10 @@ func (b *LocalBackend) resetForProfileChangeLocked() error {
 	b.updateFilterLocked(ipn.PrefsView{})
 	// Reset the NetworkMap in the engine
 	b.e.SetNetworkMap(new(netmap.NetworkMap))
-	b.resetControlClientLocked()
+	if prevCC := b.resetControlClientLocked(); prevCC != nil {
+		// Shutdown outside of b.mu to avoid deadlocks.
+		b.goTracker.Go(prevCC.Shutdown)
+	}
 	// TKA errors should not prevent resetting the backend state.
 	// However, we should still return the error to the caller.
 	tkaErr := b.initTKALocked()
@@ -7010,7 +7026,10 @@ func (b *LocalBackend) ResetAuth() error {
 	b.mu.Lock()
 	defer b.mu.Unlock()
 
-	b.resetControlClientLocked()
+	if prevCC := b.resetControlClientLocked(); prevCC != nil {
+		// Shutdown outside of b.mu to avoid deadlocks.
+		b.goTracker.Go(prevCC.Shutdown)
+	}
 	if err := b.clearMachineKeyLocked(); err != nil {
 		return err
 	}