From 6fecc16c3b49b3b0cb1d71cae541823c1c038807 Mon Sep 17 00:00:00 2001
From: Maisem Ali <maisem@tailscale.com>
Date: Sat, 26 Mar 2022 20:12:12 -0700
Subject: [PATCH] ipn/ipnlocal: do not process old status messages received out
 of order

When `setWgengineStatus` is invoked concurrently from multiple
goroutines, it is possible that the call invoked with a newer status is
processed before a call with an older status. e.g. a status that has
endpoints might be followed by a status without endpoints. This causes
unnecessary work in the engine and can result in packet loss.

This patch adds an `AsOf time.Time` field to the status to specifiy when the
status was calculated, which later allows `setWgengineStatus` to ignore
any status messages it receives that are older than the one it has
already processed.

Updates tailscale/corp#2579

Signed-off-by: Maisem Ali <maisem@tailscale.com>
---
 ipn/ipnlocal/local.go      | 8 ++++++++
 ipn/ipnlocal/state_test.go | 5 +++--
 wgengine/userspace.go      | 1 +
 wgengine/wgengine.go       | 2 ++
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index 5a646c57f..339158c49 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -140,6 +140,7 @@ type LocalBackend struct {
 	peerAPIListeners []*peerAPIListener
 	loginFlags       controlclient.LoginFlags
 	incomingFiles    map[*incomingFile]bool
+	lastStatusTime   time.Time // status.AsOf value of the last processed status update
 	// directFileRoot, if non-empty, means to write received files
 	// directly to this directory, without staging them in an
 	// intermediate buffered directory for "pick-up" later. If
@@ -706,6 +707,13 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) {
 	}
 
 	b.mu.Lock()
+	if s.AsOf.Before(b.lastStatusTime) {
+		// Don't process a status update that is older than the one we have
+		// already processed. (corp#2579)
+		b.mu.Unlock()
+		return
+	}
+	b.lastStatusTime = s.AsOf
 	es := b.parseWgStatusLocked(s)
 	cc := b.cc
 	b.engineStatus = es
diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go
index db6398387..9f8dfc4cd 100644
--- a/ipn/ipnlocal/state_test.go
+++ b/ipn/ipnlocal/state_test.go
@@ -62,6 +62,7 @@ func (nt *notifyThrottler) put(n ipn.Notify) {
 // drain pulls the notifications out of the queue, asserting that there are
 // exactly count notifications that have been put so far.
 func (nt *notifyThrottler) drain(count int) []ipn.Notify {
+	nt.t.Helper()
 	nt.mu.Lock()
 	ch := nt.ch
 	nt.mu.Unlock()
@@ -923,7 +924,7 @@ func TestStateMachine(t *testing.T) {
 	}
 	notifies.expect(1)
 	// Fake a DERP connection.
-	b.setWgengineStatus(&wgengine.Status{DERPs: 1}, nil)
+	b.setWgengineStatus(&wgengine.Status{DERPs: 1, AsOf: time.Now()}, nil)
 	{
 		nn := notifies.drain(1)
 		cc.assertCalls("unpause")
@@ -1016,7 +1017,7 @@ func TestWGEngineStatusRace(t *testing.T) {
 			if i == 0 {
 				n = 1
 			}
-			b.setWgengineStatus(&wgengine.Status{DERPs: n}, nil)
+			b.setWgengineStatus(&wgengine.Status{AsOf: time.Now(), DERPs: n}, nil)
 		}(i)
 	}
 	wg.Wait()
diff --git a/wgengine/userspace.go b/wgengine/userspace.go
index e78d5939c..972cc288b 100644
--- a/wgengine/userspace.go
+++ b/wgengine/userspace.go
@@ -1113,6 +1113,7 @@ func (e *userspaceEngine) getStatus() (*Status, error) {
 	}
 
 	return &Status{
+		AsOf:       time.Now(),
 		LocalAddrs: append([]tailcfg.Endpoint(nil), e.endpoints...),
 		Peers:      peers,
 		DERPs:      derpConns,
diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go
index bdfa21a3f..15ff35775 100644
--- a/wgengine/wgengine.go
+++ b/wgengine/wgengine.go
@@ -6,6 +6,7 @@ package wgengine
 
 import (
 	"errors"
+	"time"
 
 	"inet.af/netaddr"
 	"tailscale.com/ipn/ipnstate"
@@ -23,6 +24,7 @@ import (
 //
 // TODO(bradfitz): remove this, subset of ipnstate? Need to migrate users.
 type Status struct {
+	AsOf       time.Time // the time at which the status was calculated
 	Peers      []ipnstate.PeerStatusLite
 	LocalAddrs []tailcfg.Endpoint // the set of possible endpoints for the magic conn
 	DERPs      int                // number of active DERP connections