ipn/ipnlocal: shut down old control client before starting new one

LocalBackend.Start previously shut down the previous control client in
a goroutine, letting it run concurrently with the new one. An in-flight
lite map update carrying stale Hostinfo.RequestTags could then be
processed by the control plane after the new client had already changed
the node's tags. Control treats such a request as an invalid tag
transition and expires the node key to force a reauth, so retagging a
node with "tailscale up --advertise-tags" intermittently logged the
machine out.

Instead, detach the old client under b.mu and shut it down
synchronously with the lock released, before creating the new client.
Shutdown cancels the old client's in-flight requests and waits for its
goroutines to exit, so the cancellation of any stale update reaches the
server before the new client sends its first request. Per the deadlock
history in #18052, Shutdown must not be called with b.mu held; this
uses the same pattern as DisconnectControl.

Also teach the testcontrol server to model the control plane's tag
transition handling (including expiring the node key on an invalid
transition and ignoring updates from canceled requests), add an
integration test reproducing the race, and add an ipnlocal test
verifying that Start waits for the old client to shut down.

Updates #20365
Updates #18052

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Change-Id: If8c8e145bdadcef1b1b8fe6209453cf5f5a8d616

ipn/ipnlocal/local.go

@@ -1150,14 +1150,28 @@ func (b *LocalBackend) shouldPauseControlClientLocked(prefs ipn.PrefsView) bool
 	return false
 }
 
-// DisconnectControl shuts down control client. This can be run before node shutdown to force control to consider this ndoe
-// inactive. This can be used to ensure that nodes that are HA subnet router or app connector replicas are shutting
-// down, clients switch over to other replicas whilst the existing connections are kept alive for some period of time.
+// DisconnectControl shuts down the control client. This can be run before
+// node shutdown to force control to consider this node inactive. This can
+// be used to ensure that nodes that are HA subnet router or app connector
+// replicas are shutting down, clients switch over to other replicas whilst
+// the existing connections are kept alive for some period of time.
+//
+// Shutdown of the detached client is synchronous: it cancels the client's
+// in-flight requests and waits for its goroutines to exit, but it does not
+// wait for pending updates to be delivered.
 func (b *LocalBackend) DisconnectControl() {
 	b.mu.Lock()
 	cc := b.resetControlClientLocked()
 	b.mu.Unlock()
 
+	// The Shutdown call must not run while b.mu is held, per the deadlock
+	// history in tailscale/tailscale#18052: controlclient.Auto's
+	// goroutines deliver callbacks into LocalBackend through an execqueue
+	// whose RunSync holds the queue mutex while the callback acquires
+	// b.mu, and Auto.Shutdown acquires that same queue mutex, so calling
+	// it with b.mu held inverts the lock order. A previous attempt to
+	// shut the client down synchronously inside resetControlClientLocked
+	// with b.mu held (#18127) deadlocked and was reverted (#18149).
 	if cc != nil {
 		cc.Shutdown()
 	}
@@ -3004,6 +3018,23 @@ func (b *LocalBackend) controlDebugFlags() []string {
 func (b *LocalBackend) Start(opts ipn.Options) error {
 	defer b.CheckDeadlocks()()
 
+	// Shut down the previous control client, if any, before starting a
+	// new one, so the old client can't race with the new one. Without
+	// this, an in-flight lite map update carrying stale Hostinfo (notably
+	// RequestTags) could be processed by the control plane after the new
+	// client's requests, which made retagging with "tailscale up
+	// --advertise-tags" intermittently look like an invalid tag
+	// transition and log the node out (tailscale/tailscale#20365).
+	//
+	// TODO(bradfitz,nickkhyl): this is still racy if Start is called
+	// concurrently: whichever call loses the race to reacquire b.mu
+	// below then detaches the winner's new control client in startLocked
+	// and shuts it down in a goroutine, without the ordering guarantee
+	// that this call provides. This is a workaround until #18052 is
+	// properly fixed and a control client can be shut down synchronously
+	// with b.mu held.
+	b.DisconnectControl()
+
 	b.mu.Lock()
 	defer b.mu.Unlock()
 	return b.startLocked(opts)

ipn/ipnlocal/state_test.go

@@ -352,6 +352,39 @@ func (b *LocalBackend) nonInteractiveLoginForStateTest() {
 	cc.Login(b.loginFlags | controlclient.LoginInteractive)
 }
 
+// TestStartShutsDownPreviousControlClient verifies that Start waits for the
+// previous control client to fully shut down before creating a new one.
+//
+// If the old client is still alive when the new one starts, its in-flight
+// requests (carrying stale Hostinfo, notably RequestTags) can race with the
+// new client's requests at the control plane. That made retagging a node
+// with "tailscale up --advertise-tags" intermittently log the node out
+// (tailscale/tailscale#20365): a stale RequestTags update processed after
+// the tag transition looks like an invalid transition, so the control
+// server expires the node key.
+func TestStartShutsDownPreviousControlClient(t *testing.T) {
+	const enableLogging = true
+	var cc *mockControl
+	b := newLocalBackendWithTestControl(t, enableLogging, func(tb testing.TB, opts controlclient.Options) controlclient.Client {
+		if cc != nil {
+			select {
+			case <-cc.shutdown:
+			default:
+				t.Errorf("new control client created before the previous one was shut down")
+			}
+		}
+		cc = newClient(t, opts)
+		return cc
+	})
+
+	for i := range 3 {
+		t.Logf("Start %d", i+1)
+		if err := b.Start(ipn.Options{}); err != nil {
+			t.Fatalf("Start: %v", err)
+		}
+	}
+}
+
 // A very precise test of the sequence of function calls generated by
 // ipnlocal.Local into its controlclient instance, and the events it
 // produces upstream into the UI.