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

@@ -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))
 		}
 	}
 

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,

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
 		}

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")
+	}
+}