wgengine/magicsock: only run derpActiveFunc after connecting to DERP (#18814)

derpActiveFunc was being called immediately as a bare goroutine,
before startGate was resolved. For the firstDerp case, startGate
is c.derpStarted which only closes after dc.Connect() completes,
so derpActiveFunc was firing before the DERP connection existed.

We now block it with the same logic used by runDerpReader and by
runDerpWriter.

Updates: #18810

Signed-off-by: Fernando Serboncini <fserb@tailscale.com>

2 files changed, 59 insertions, 1 deletions

diff --git a/wgengine/magicsock/derp.go b/wgengine/magicsock/derp.go
index b3cc5c2ce..f9e505070 100644
--- a/wgengine/magicsock/derp.go
+++ b/wgengine/magicsock/derp.go
@@ -436,7 +436,14 @@ func (c *Conn) derpWriteChanForRegion(regionID int, peer key.NodePublic) chan de
 
 	go c.runDerpReader(ctx, regionID, dc, wg, startGate)
 	go c.runDerpWriter(ctx, dc, ch, wg, startGate)
-	go c.derpActiveFunc()
+	go func() {
+		select {
+		case <-ctx.Done():
+			return
+		case <-startGate:
+			c.derpActiveFunc()
+		}
+	}()
 
 	return ad.writeCh
 }
diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go
index 5fa177b3b..9d6cae87b 100644
--- a/wgengine/magicsock/magicsock_test.go
+++ b/wgengine/magicsock/magicsock_test.go
@@ -530,6 +530,57 @@ func TestPickDERPFallback(t *testing.T) {
 	// have fixed DERP fallback logic.
 }
 
+// TestDERPActiveFuncCalledAfterConnect verifies that DERPActiveFunc is not
+// called until the DERP connection is actually established (i.e. after
+// startGate / derpStarted is closed).
+func TestDERPActiveFuncCalledAfterConnect(t *testing.T) {
+	derpMap, cleanup := runDERPAndStun(t, t.Logf, localhostListener{}, netaddr.IPv4(127, 0, 0, 1))
+	defer cleanup()
+
+	bus := eventbustest.NewBus(t)
+
+	netMon, err := netmon.New(bus, t.Logf)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	resultCh := make(chan bool, 1)
+	var conn *Conn
+
+	conn, err = NewConn(Options{
+		Logf:                   t.Logf,
+		NetMon:                 netMon,
+		EventBus:               bus,
+		HealthTracker:          health.NewTracker(bus),
+		Metrics:                new(usermetric.Registry),
+		DisablePortMapper:      true,
+		TestOnlyPacketListener: localhostListener{},
+		EndpointsFunc:          func([]tailcfg.Endpoint) {},
+		DERPActiveFunc: func() {
+			// derpStarted should already be closed when DERPActiveFunc is called.
+			select {
+			case <-conn.derpStarted:
+				resultCh <- true
+			default:
+				resultCh <- false
+			}
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer conn.Close()
+
+	conn.SetDERPMap(derpMap)
+	if err := conn.SetPrivateKey(key.NewNode()); err != nil {
+		t.Fatal(err)
+	}
+
+	if ok := <-resultCh; !ok {
+		t.Error("DERPActiveFunc was called before DERP connection was established")
+	}
+}
+
 // TestDeviceStartStop exercises the startup and shutdown logic of
 // wireguard-go, which is intimately intertwined with magicsock's own
 // lifecycle. We seem to be good at generating deadlocks here, so if