ipn/ipnlocal: fix race condition that results in a panic sending on a closed channel

Fixes #13288

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

ipn/ipnlocal/local.go

@@ -161,6 +161,7 @@ func RegisterNewSSHServer(fn newSSHServerFunc) {
 type watchSession struct {
 	ch        chan *ipn.Notify
 	sessionID string
+	cancel    func() // call to signal that the session must be terminated
 }
 
 // LocalBackend is the glue between the major pieces of the Tailscale
@@ -2635,7 +2636,15 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa
 		}
 	}
 
-	mak.Set(&b.notifyWatchers, sessionID, &watchSession{ch, sessionID})
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
+	session := &watchSession{
+		ch:        ch,
+		sessionID: sessionID,
+		cancel:    cancel,
+	}
+	mak.Set(&b.notifyWatchers, sessionID, session)
 	b.mu.Unlock()
 
 	defer func() {
@@ -2666,8 +2675,6 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa
 	// request every 2 seconds.
 	// TODO(bradfitz): plumb this further and only send a Notify on change.
 	if mask&ipn.NotifyWatchEngineUpdates != 0 {
-		ctx, cancel := context.WithCancel(ctx)
-		defer cancel()
 		go b.pollRequestEngineStatus(ctx)
 	}
 
@@ -2680,7 +2687,7 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa
 		select {
 		case <-ctx.Done():
 			return
-		case n, ok := <-ch:
+		case n := <-ch:
 			// URLs flow into Notify.BrowseToURL via two means:
 			//    1. From MapResponse.PopBrowserURL, which already says they're dup
 			//       suppressed if identical, and that's done by the controlclient,
@@ -2696,7 +2703,7 @@ func (b *LocalBackend) WatchNotifications(ctx context.Context, mask ipn.NotifyWa
 					lastURLPop = v
 				}
 			}
-			if !ok || !fn(n) {
+			if !fn(n) {
 				return
 			}
 		}

ipn/ipnlocal/serve.go

@@ -331,7 +331,7 @@ func (b *LocalBackend) setServeConfigLocked(config *ipn.ServeConfig, etag string
 			if !has(k) {
 				for _, sess := range b.notifyWatchers {
 					if sess.sessionID == k {
-						close(sess.ch)
+						sess.cancel()
 					}
 				}
 			}

ipn/ipnlocal/serve_test.go

@@ -251,6 +251,14 @@ func TestServeConfigForeground(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Introduce a race between [LocalBackend] sending notifications
+	// and [LocalBackend.WatchNotifications] shutting down due to
+	// setting the serve config below.
+	const N = 1000
+	for range N {
+		go b.send(ipn.Notify{})
+	}
+
 	// Setting a new serve config should shut down WatchNotifications
 	// whose session IDs are no longer found: session1 goes, session2 stays.
 	err = b.SetServeConfig(&ipn.ServeConfig{