fix(derp): bound dup client sendHistory growth

derp/derp_server.go

@@ -26,6 +26,7 @@ import (
 	"net/netip"
 	"os/exec"
 	"runtime"
+	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -233,10 +234,14 @@ type dupClientSet struct {
 	// data since.
 	last *sclient
 
-	// sendHistory is a log of which members of set have sent
-	// frames to the derp server, with adjacent duplicates
-	// removed. When a member of set is removed, the same
-	// element(s) are removed from sendHistory.
+	// sendHistory records which members of set have sent frames to
+	// the DERP server, ordered from least to most recently active.
+	// Each member appears at most once: recording a member that is
+	// already present moves it to the end instead of appending a
+	// duplicate. That keeps the slice bounded by the size of set.
+	// Without the bound, two connections sharing a key and taking
+	// turns sending could grow it without limit. When a member of
+	// set is removed, it is also removed from sendHistory.
 	sendHistory []*sclient
 }
 
@@ -1211,7 +1216,14 @@ func (s *Server) noteClientActivity(c *sclient) {
 		}
 	}
 
-	// Append this client to the list of clients who spoke last.
+	// Record c as the most recent sender. If c is already in
+	// sendHistory, remove the earlier occurrence first so that each
+	// member appears at most once and the slice stays bounded by the
+	// number of connections in the set. The tail check above already
+	// handled the case where c is the current tail.
+	if i := slices.Index(ds.sendHistory, c); i >= 0 {
+		ds.sendHistory = slices.Delete(ds.sendHistory, i, i+1)
+	}
 	ds.sendHistory = append(ds.sendHistory, c)
 }
 

derp/derp_test.go

@@ -1285,6 +1285,104 @@ func TestServerDupClients(t *testing.T) {
 	})
 }
 
+// TestServerDupClientSendHistoryBounded verifies that a dup client set's
+// sendHistory does not grow without bound when two connections sharing a
+// key take turns sending frames. See the sendHistory field comment.
+func TestServerDupClientSendHistoryBounded(t *testing.T) {
+	clientPub := key.NewNode().Public()
+
+	newClient := func(t *testing.T, name string) *sclient {
+		return &sclient{key: clientPub, logf: logger.WithPrefix(t.Logf, name+": ")}
+	}
+	wantDupSet := func(t *testing.T, s *Server) *dupClientSet {
+		t.Helper()
+		dup, ok := s.clients[clientPub].(*dupClientSet)
+		if !ok {
+			t.Fatalf("wanted dup set; got %T", s.clients[clientPub])
+		}
+		return dup
+	}
+
+	t.Run("last_writer_active", func(t *testing.T) {
+		s := NewServer(key.NewNode(), t.Logf)
+		s.dupPolicy = lastWriterIsActive
+		c1, c2 := newClient(t, "c1"), newClient(t, "c2")
+		s.registerClient(c1)
+		s.registerClient(c2)
+
+		dup := wantDupSet(t, s)
+		for range 5000 {
+			s.noteClientActivity(c1)
+			s.noteClientActivity(c2)
+		}
+		if got := len(dup.sendHistory); got > len(dup.set) {
+			t.Fatalf("sendHistory len = %d; want <= %d (set size)", got, len(dup.set))
+		}
+		// The clients must remain active and undisabled, and the last
+		// one to send is the active one.
+		if c1.isDisabled.Load() || c2.isDisabled.Load() {
+			t.Fatal("client disabled under lastWriterIsActive; want none disabled")
+		}
+		if got := dup.ActiveClient(); got != c2 {
+			t.Fatalf("active client = %p; want c2 %p", got, c2)
+		}
+
+		// When the active connection drops, the surviving one must
+		// still be picked up.
+		s.unregisterClient(c2)
+		if got := s.clients[clientPub].ActiveClient(); got != c1 {
+			t.Fatalf("after dropping c2, active = %p; want c1 %p", got, c1)
+		}
+	})
+
+	t.Run("history_order_survives_removal", func(t *testing.T) {
+		s := NewServer(key.NewNode(), t.Logf)
+		s.dupPolicy = lastWriterIsActive
+		c1, c2, c3 := newClient(t, "c1"), newClient(t, "c2"), newClient(t, "c3")
+		s.registerClient(c1)
+		s.registerClient(c2)
+		s.registerClient(c3)
+
+		dup := wantDupSet(t, s)
+		for range 5000 {
+			s.noteClientActivity(c1)
+			s.noteClientActivity(c2)
+			s.noteClientActivity(c3)
+		}
+		s.noteClientActivity(c2)
+		if got := len(dup.sendHistory); got > len(dup.set) {
+			t.Fatalf("sendHistory len = %d; want <= %d (set size)", got, len(dup.set))
+		}
+
+		// c2 spoke last, then c3. Dropping c2 must make c3 active.
+		s.unregisterClient(c2)
+		if got := s.clients[clientPub].ActiveClient(); got != c3 {
+			t.Fatalf("after dropping c2, active = %p; want c3 %p", got, c3)
+		}
+	})
+
+	t.Run("disable_fighters", func(t *testing.T) {
+		s := NewServer(key.NewNode(), t.Logf)
+		s.dupPolicy = disableFighters
+		c1, c2 := newClient(t, "c1"), newClient(t, "c2")
+		s.registerClient(c1)
+		s.registerClient(c2)
+
+		dup := wantDupSet(t, s)
+		for range 5000 {
+			s.noteClientActivity(c1)
+			s.noteClientActivity(c2)
+		}
+		if got := len(dup.sendHistory); got > len(dup.set) {
+			t.Fatalf("sendHistory len = %d; want <= %d (set size)", got, len(dup.set))
+		}
+		// Fighting connections must be disabled.
+		if !c1.isDisabled.Load() || !c2.isDisabled.Load() {
+			t.Fatal("fighting clients not disabled; want both disabled")
+		}
+	})
+}
+
 func TestLimiter(t *testing.T) {
 	rl := rate.NewLimiter(rate.Every(time.Minute), 100)
 	for i := 0; i < 200; i++ {