From 6e334e64a1543a42b218a87fe2df8bfdf1d1fa15 Mon Sep 17 00:00:00 2001
From: James Tucker <james@tailscale.com>
Date: Fri, 5 Apr 2024 12:12:18 -0700
Subject: [PATCH] net/netcheck,wgengine/magicsock: align DERP frame receive
 time heuristics

The netcheck package and the magicksock package coordinate via the
health package, but both sides have time based heuristics through
indirect dependencies. These were misaligned, so the implemented
heuristic aimed at reducing DERP moves while there is active traffic
were non-operational about 3/5ths of the time.

It is problematic to setup a good test for this integration presently,
so instead I added comment breadcrumbs along with the initial fix.

Updates #8603

Signed-off-by: James Tucker <james@tailscale.com>
---
 net/netcheck/netcheck.go        |  7 ++++---
 net/netcheck/netcheck_test.go   |  4 ++--
 wgengine/magicsock/derp.go      |  8 +++++++-
 wgengine/magicsock/derp_test.go | 16 ++++++++++++++++
 4 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100644 wgengine/magicsock/derp_test.go

diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go
index 862743540..3181119f2 100644
--- a/net/netcheck/netcheck.go
+++ b/net/netcheck/netcheck.go
@@ -1388,10 +1388,11 @@ const (
 	// node is near region 1 @ 4ms and region 2 @ 5ms, region 1 getting
 	// 5ms slower would cause a flap).
 	preferredDERPAbsoluteDiff = 10 * time.Millisecond
-	// preferredDERPFrameTime is the time which, if a DERP frame has been
+	// PreferredDERPFrameTime is the time which, if a DERP frame has been
 	// received within that period, we treat that region as being present
 	// even without receiving a STUN response.
-	preferredDERPFrameTime = 2 * time.Second
+	// Note: must remain higher than the derp package frameReceiveRecordRate
+	PreferredDERPFrameTime = 8 * time.Second
 )
 
 // addReportHistoryAndSetPreferredDERP adds r to the set of recent Reports
@@ -1480,7 +1481,7 @@ func (c *Client) addReportHistoryAndSetPreferredDERP(rs *reportState, r *Report,
 			now := c.timeNow()
 
 			heardFromOldRegionRecently = lastHeard.After(rs.start)
-			heardFromOldRegionRecently = heardFromOldRegionRecently || lastHeard.After(now.Add(-preferredDERPFrameTime))
+			heardFromOldRegionRecently = heardFromOldRegionRecently || lastHeard.After(now.Add(-PreferredDERPFrameTime))
 		}
 	}
 
diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go
index 80aa5a657..aaff4e86b 100644
--- a/net/netcheck/netcheck_test.go
+++ b/net/netcheck/netcheck_test.go
@@ -410,7 +410,7 @@ func TestAddReportHistoryAndSetPreferredDERP(t *testing.T) {
 			},
 			opts: &GetReportOpts{
 				GetLastDERPActivity: mkLDAFunc(map[int]time.Time{
-					1: startTime.Add(2*time.Second + preferredDERPFrameTime/2), // within active window of step (3)
+					1: startTime.Add(2*time.Second + PreferredDERPFrameTime/2), // within active window of step (3)
 				}),
 			},
 			wantPrevLen: 3,
@@ -425,7 +425,7 @@ func TestAddReportHistoryAndSetPreferredDERP(t *testing.T) {
 			},
 			opts: &GetReportOpts{
 				GetLastDERPActivity: mkLDAFunc(map[int]time.Time{
-					1: startTime.Add(4*time.Second - preferredDERPFrameTime - 1), // not within active window of (3)
+					1: startTime.Add(4*time.Second - PreferredDERPFrameTime - 1), // not within active window of (3)
 				}),
 			},
 			wantPrevLen: 3,
diff --git a/wgengine/magicsock/derp.go b/wgengine/magicsock/derp.go
index 73147e128..32611546f 100644
--- a/wgengine/magicsock/derp.go
+++ b/wgengine/magicsock/derp.go
@@ -35,6 +35,12 @@ import (
 	"tailscale.com/util/testenv"
 )
 
+// frameReceiveRecordRate is the minimum time between updates to last frame
+// received times.
+// Note: this is relevant to other parts of the system, such as netcheck
+// preferredDERPFrameTime, so update with care.
+const frameReceiveRecordRate = 5 * time.Second
+
 // useDerpRoute reports whether magicsock should enable the DERP
 // return path optimization (Issue 150).
 //
@@ -569,7 +575,7 @@ func (c *Conn) runDerpReader(ctx context.Context, derpFakeAddr netip.AddrPort, d
 		bo.BackOff(ctx, nil) // reset
 
 		now := time.Now()
-		if lastPacketTime.IsZero() || now.Sub(lastPacketTime) > 5*time.Second {
+		if lastPacketTime.IsZero() || now.Sub(lastPacketTime) > frameReceiveRecordRate {
 			health.NoteDERPRegionReceivedFrame(regionID)
 			lastPacketTime = now
 		}
diff --git a/wgengine/magicsock/derp_test.go b/wgengine/magicsock/derp_test.go
new file mode 100644
index 000000000..ffb230789
--- /dev/null
+++ b/wgengine/magicsock/derp_test.go
@@ -0,0 +1,16 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package magicsock
+
+import (
+	"testing"
+
+	"tailscale.com/net/netcheck"
+)
+
+func CheckDERPHeuristicTimes(t *testing.T) {
+	if netcheck.PreferredDERPFrameTime <= frameReceiveRecordRate {
+		t.Errorf("PreferredDERPFrameTime too low; should be at least frameReceiveRecordRate")
+	}
+}