ipn/ipnlocal,tstest/integration: pause the control client consistently (#19846)

There are two places where tailscaled transitions into a paused state:
1. tailscaled’s controlclient is initially created,
2. tailscale down, or the GUI equivalent, commands it to.

This patch unifies the implementation of both scenarios into
LocalBackend.shouldPauseControlClientLocked to prevent the
implementation from drifting.

The flaky tstest/integration.TestNoControlConnWhenDown test exposed
this mismatch, but only by accident. This patch also changes
TestNode.MustDown so that it runs `tailscale down` and then waits for
the testcontrol server to finish handling any associated /machine/map
requests.

Fixes #19831

Signed-off-by: Simon Law <sfllaw@tailscale.com>

ipn/ipnlocal/local.go

@@ -1037,13 +1037,37 @@ func (b *LocalBackend) pauseOrResumeControlClientLocked() {
 	if b.cc == nil {
 		return
 	}
-	networkUp := b.interfaceState.AnyInterfaceUp()
-	pauseForNetwork := (b.state == ipn.Stopped && b.NetMapNoPeers() != nil) || (!networkUp && !testenv.InTest() && !envknob.AssumeNetworkUp())
+	b.cc.SetPaused(b.shouldPauseControlClientLocked(b.pm.CurrentPrefs()))
+}
+
+// shouldPauseControlClientLocked reports whether the control client should be paused:
+// if the LocalBackend is in Stopped state with a valid NetMap,
+// if there is no network available,
+// or if syncing preferences from the control plane has been disabled.
+//
+// b.mu must be held.
+func (b *LocalBackend) shouldPauseControlClientLocked(prefs ipn.PrefsView) bool {
+	syncs.RequiresMutex(&b.mu)
 
-	prefs := b.pm.CurrentPrefs()
 	pauseForSyncPref := prefs.Valid() && prefs.Sync().EqualBool(false)
+	if pauseForSyncPref {
+		return true
+	}
 
-	b.cc.SetPaused(pauseForNetwork || pauseForSyncPref)
+	// If tailscaled is being restarted, but it is supposed to be stopped,
+	// it mustn’t pause until the initial netmap has been loaded.
+	isStopped := b.state == ipn.Stopped && b.NetMapNoPeers() != nil
+	if isStopped {
+		return true
+	}
+
+	networkUp := b.interfaceState.AnyInterfaceUp()
+	pauseForNetwork := !networkUp && !testenv.InTest() && !envknob.AssumeNetworkUp()
+	if pauseForNetwork {
+		return true
+	}
+
+	return false
 }
 
 // DisconnectControl shuts down control client. This can be run before node shutdown to force control to consider this ndoe
@@ -2961,7 +2985,7 @@ func (b *LocalBackend) startLocked(opts ipn.Options) error {
 		ControlKnobs:         b.sys.ControlKnobs(),
 		Shutdown:             ccShutdown,
 		Bus:                  b.sys.Bus.Get(),
-		StartPaused:          prefs.Sync().EqualBool(false),
+		StartPaused:          b.shouldPauseControlClientLocked(prefs),
 	})
 	if err != nil {
 		return err

tstest/integration/integration.go

@@ -919,6 +919,20 @@ func (n *TestNode) MustDown() {
 	if err := n.Tailscale("down", "--accept-risk=all").Run(); err != nil {
 		t.Fatalf("down: %v", err)
 	}
+
+	// The tailscale down command is asynchronous, so it returns early.
+	// Wait for tailscaled to drop its connection before continuing.
+	if err := tstest.WaitFor(time.Second, func() error {
+		if err := t.Context().Err(); err != nil {
+			return err
+		}
+		if c := n.env.Control.InServeMap(); c != 0 {
+			return fmt.Errorf("%d connections remaining in serve map", c)
+		}
+		return nil
+	}); err != nil {
+		t.Fatalf("tailscale down: %v", err)
+	}
 }
 
 func (n *TestNode) MustLogOut() {

tstest/integration/integration_test.go

@@ -1058,8 +1058,6 @@ func TestC2NPingRequest(t *testing.T) {
 // Issue 2434: when "down" (WantRunning false), tailscaled shouldn't
 // be connected to control.
 func TestNoControlConnWhenDown(t *testing.T) {
-	flakytest.Mark(t, "https://github.com/tailscale/tailscale/issues/19831")
-
 	tstest.Shard(t)
 	tstest.Parallel(t)
 	env := NewTestEnv(t)
@@ -1083,14 +1081,24 @@ func TestNoControlConnWhenDown(t *testing.T) {
 
 	n1.AwaitBackendState("Stopped")
 
+	// The real test: verify our daemon doesn't have an HTTP request open.
+	// Stopping the client may take some time to disconnect from testcontrol.
+	if err := tstest.WaitFor(time.Second, func() error {
+		if n := env.Control.InServeMap(); n != 0 {
+			return fmt.Errorf("in serve map = %d; want 0", n)
+		}
+		return nil
+	}); err != nil {
+		t.Fatalf("unexpected connections while stopped: %v", err)
+	}
+
 	ip2 := n1.AwaitIP4()
 	if ip1 != ip2 {
 		t.Errorf("IPs different: %q vs %q", ip1, ip2)
 	}
 
-	// The real test: verify our daemon doesn't have an HTTP request open.
 	if n := env.Control.InServeMap(); n != 0 {
-		t.Errorf("in serve map = %d; want 0", n)
+		t.Fatalf("unexpected connection triggered by tailscale ip: in serve map = %d; want 0", n)
 	}
 
 	d2.MustCleanShutdown(t)