This commit contains the changes for the following:

1. Patch prefs handler doesn't add the remote routes to pref if the pref update is also turning appconnector off.
2. Unadvertise routes when StoreRoutes is on but appConnector is turning off.
3. When appConnector is turning on, add a task to recreate and reload routeInfo from store to the queue
3. When shouldStore is changed, remove the state saved for routeInfo (Set to nil)

appc/appconnector.go

@@ -19,6 +19,7 @@ import (
 	xmaps "golang.org/x/exp/maps"
 	"golang.org/x/net/dns/dnsmessage"
 	"tailscale.com/appc/routeinfo"
+	"tailscale.com/ipn"
 	"tailscale.com/types/logger"
 	"tailscale.com/types/views"
 	"tailscale.com/util/dnsname"
@@ -105,7 +106,9 @@ func (e *AppConnector) RouteInfo() *routeinfo.RouteInfo {
 	if e.routeInfo == nil {
 		ret, err := e.routeAdvertiser.ReadRouteInfo()
 		if err != nil {
-			e.logf("Unsuccessful Read RouteInfo: ", err)
+			if err != ipn.ErrStateNotExist {
+				e.logf("Unsuccessful Read RouteInfo: ", err)
+			}
 			return routeinfo.NewRouteInfo()
 		}
 		return ret
@@ -113,6 +116,34 @@ func (e *AppConnector) RouteInfo() *routeinfo.RouteInfo {
 	return e.routeInfo
 }
 
+// RecreateRouteInfoFromStore searches from presist store for existing routeInfo for current profile,
+// then update local routes of routeInfo in store as the current advertised routes.
+func (e *AppConnector) RecreateRouteInfoFromStore(localRoutes []netip.Prefix) {
+	e.queue.Add(func() {
+		ri := e.RouteInfo()
+		ri.Local = localRoutes
+		err := e.routeAdvertiser.StoreRouteInfo(ri)
+		if err != nil {
+			e.logf("Appc recreate routeInfo: Error updating routeInfo in store: ", err)
+		}
+		err = e.routeAdvertiser.AdvertiseRoute(ri.Routes(false, true, true)...)
+		if err != nil {
+			e.logf("Appc recreate routeInfo: Error advertise routes: ", err)
+		}
+		e.routeInfo = ri
+	})
+}
+
+// UpdateRouteInfo updates routeInfo value of the AppConnector and
+// stores the routeInfo value to presist store.
+func (e *AppConnector) UpdateRouteInfo(ri *routeinfo.RouteInfo) {
+	err := e.routeAdvertiser.StoreRouteInfo(ri)
+	if err != nil {
+		e.logf("Appc: Failed to remove routeInfo from store: ", err)
+	}
+	e.routeInfo = ri
+}
+
 // UpdateDomains asynchronously replaces the current set of configured domains
 // with the supplied set of domains. Domains must not contain a trailing dot,
 // and should be lower case. If the domain contains a leading '*' label it
@@ -180,9 +211,15 @@ func (e *AppConnector) updateRoutes(routes []netip.Prefix) {
 	if e.ShouldStoreRoutes {
 		routeInfo, err = e.routeAdvertiser.ReadRouteInfo()
 		if err != nil {
-			e.logf("failed to read routeInfo from store")
+			if err != ipn.ErrStateNotExist {
+				e.logf("Appc: Unsuccessful Read RouteInfo: ", err)
+			}
+			routeInfo = routeinfo.NewRouteInfo()
 		}
 		oldControl := routeInfo.Control
+		routeInfo.Control = routes
+		e.routeInfo = routeInfo
+		e.routeAdvertiser.StoreRouteInfo(e.routeInfo)
 		oldOtherRoutes := routeInfo.Routes(true, false, true)
 		for _, ipp := range oldControl {
 			if slices.Contains(routes, ipp) {
@@ -197,7 +234,6 @@ func (e *AppConnector) updateRoutes(routes []netip.Prefix) {
 		if err := e.routeAdvertiser.UnadvertiseRoute(toRemove...); err != nil {
 			e.logf("failed to unadvertise old routes: %v: %v", routes, err)
 		}
-		routeInfo.Control = routes
 	}
 	if err := e.routeAdvertiser.AdvertiseRoute(routes...); err != nil {
 		e.logf("failed to advertise routes: %v: %v", routes, err)
@@ -223,10 +259,6 @@ nextRoute:
 		e.logf("failed to unadvertise routes: %v: %v", toRemove, err)
 	}
 	e.controlRoutes = routes
-	if e.ShouldStoreRoutes {
-		e.routeInfo = routeInfo
-		e.routeAdvertiser.StoreRouteInfo(e.routeInfo)
-	}
 }
 
 // Domains returns the currently configured domain list.

appc/appconnector_test.go

@@ -102,8 +102,8 @@ func TestUpdateRoutesNotUnadvertiseRoutesFromOtherSources(t *testing.T) {
 		rc := &appctest.RouteCollector{}
 		a := NewAppConnector(t.Logf, rc, shouldStore)
 		testRi := routeinfo.NewRouteInfo()
-		a.routeInfo.Local = []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24")}
 		testRi.Local = append(testRi.Local, netip.MustParsePrefix("192.0.2.0/24"))
+		a.routeInfo = testRi
 		rc.StoreRouteInfo(testRi)
 
 		routes := []netip.Prefix{netip.MustParsePrefix("192.0.2.0/24"), netip.MustParsePrefix("192.0.0.1/32")}

appc/appctest/appctest.go

@@ -8,6 +8,7 @@ import (
 	"slices"
 
 	"tailscale.com/appc/routeinfo"
+	"tailscale.com/ipn"
 )
 
 // RouteCollector is a test helper that collects the list of routes advertised
@@ -42,7 +43,7 @@ func (rc *RouteCollector) StoreRouteInfo(ri *routeinfo.RouteInfo) error {
 
 func (rc *RouteCollector) ReadRouteInfo() (*routeinfo.RouteInfo, error) {
 	if rc.routeInfo == nil {
-		return routeinfo.NewRouteInfo(), nil
+		return nil, ipn.ErrStateNotExist
 	}
 	return rc.routeInfo, nil
 }

ipn/ipnlocal/local.go

@@ -3147,14 +3147,18 @@ func (b *LocalBackend) checkFunnelEnabledLocked(p *ipn.Prefs) error {
 func (b *LocalBackend) PatchPrefsHandler(mp *ipn.MaskedPrefs) (ipn.PrefsView, error) {
 	// we believe that for the purpose of figuring out advertisedRoutes setPrefsLockedOnEntry is _only_ called when
 	// up or set is used on the tailscale cli _not_ when we calculate the new advertisedRoutes field.
-	if b.appConnector != nil && b.appConnector.ShouldStoreRoutes {
+	if b.appConnector != nil && b.appConnector.ShouldStoreRoutes && mp.AdvertiseRoutesSet {
 		routeInfo := b.appConnector.RouteInfo()
 		curRoutes := routeInfo.Routes(false, true, true)
-
-		if mp.AdvertiseRoutesSet {
-			routeInfo.Local = mp.AdvertiseRoutes
-			b.StoreRouteInfo(routeInfo)
-			curRoutes := append(curRoutes, mp.AdvertiseRoutes...)
+		routeInfo.Local = mp.AdvertiseRoutes
+		b.appConnector.UpdateRouteInfo(routeInfo)
+		// When b.appConnector != nil, AppConnectorSet = true means
+		// The appConnector is turned off, in this case we should not
+		// append the remote routes to mp.AdvertiseRoutes. Appc will be
+		// set to nil first and unadvertise remote routes, but these remote routes
+		// will then be advertised again when the prefs are sent.
+		if !mp.AppConnectorSet {
+			curRoutes = append(curRoutes, mp.AdvertiseRoutes...)
 			mp.AdvertiseRoutes = curRoutes
 		}
 	}
@@ -3530,22 +3534,27 @@ func (b *LocalBackend) reconfigAppConnectorLocked(nm *netmap.NetworkMap, prefs i
 		}
 	}()
 
-	if !prefs.AppConnector().Advertise {
-		if b.appConnector != nil {
-			b.appConnector.UpdateDomainsAndRoutes([]string{}, []netip.Prefix{})
-		}
-		b.appConnector = nil
-		return
-	}
-
 	shouldAppCStoreRoutesHasChanged := false
 	shouldAppCStoreRoutes := b.ControlKnobs().AppCStoreRoutes.Load()
 	if b.appConnector != nil {
 		shouldAppCStoreRoutesHasChanged = b.appConnector.ShouldStoreRoutes != shouldAppCStoreRoutes
 	}
 
+	if !prefs.AppConnector().Advertise {
+		if b.appConnector != nil && shouldAppCStoreRoutes {
+			b.appConnector.UpdateDomainsAndRoutes([]string{}, []netip.Prefix{})
+		}
+		b.appConnector = nil
+		return
+	}
+
 	if b.appConnector == nil || shouldAppCStoreRoutesHasChanged {
 		b.appConnector = appc.NewAppConnector(b.logf, b, shouldAppCStoreRoutes)
+		if shouldAppCStoreRoutes {
+			b.appConnector.RecreateRouteInfoFromStore(prefs.AsStruct().AdvertiseRoutes)
+		} else if shouldAppCStoreRoutesHasChanged && !shouldAppCStoreRoutes {
+			b.appConnector.UpdateRouteInfo(nil)
+		}
 	}
 	if nm == nil {
 		return
@@ -6076,10 +6085,8 @@ func (b *LocalBackend) ReadRouteInfo() (*routeinfo.RouteInfo, error) {
 	key := namespaceKeyForCurrentProfile(b.pm, routeInfoStateStoreKey)
 	bs, err := b.pm.Store().ReadState(key)
 	ri := &routeinfo.RouteInfo{}
-	if err != nil && err != ipn.ErrStateNotExist {
+	if err != nil {
 		return nil, err
-	} else if err == ipn.ErrStateNotExist {
-		return ri, nil
 	}
 	if err := json.Unmarshal(bs, ri); err != nil {
 		return nil, err

ipn/ipnlocal/local_test.go

@@ -2568,7 +2568,6 @@ func TestPatchPrefsHandlerWithPresistStore(t *testing.T) {
 	prefix1 := netip.MustParsePrefix("1.2.3.4/32")
 	prefix2 := netip.MustParsePrefix("1.2.3.5/32")
 	prefix3 := netip.MustParsePrefix("1.2.3.6/32")
-	// rc := &appctest.RouteCollector{}
 	mp := new(ipn.MaskedPrefs)
 	mp.AdvertiseRoutesSet = true
 	mp.AdvertiseRoutes = []netip.Prefix{prefix1}
@@ -2665,7 +2664,7 @@ func TestPatchPrefsHandlerWithoutPresistStore(t *testing.T) {
 	prefix1 := netip.MustParsePrefix("1.2.3.4/32")
 	prefix2 := netip.MustParsePrefix("1.2.3.5/32")
 	prefix3 := netip.MustParsePrefix("1.2.3.6/32")
-	// rc := &appctest.RouteCollector{}
+
 	mp := new(ipn.MaskedPrefs)
 	mp.AdvertiseRoutesSet = true
 	mp.AdvertiseRoutes = []netip.Prefix{prefix1}
@@ -2707,11 +2706,8 @@ func TestPatchPrefsHandlerWithoutPresistStore(t *testing.T) {
 
 	//Check if route is stored in Appc/Appc.routeAdvertiser
 	storedRouteInfo, _ := b.ReadRouteInfo()
-	if len(storedRouteInfo.Local) != 0 {
-		t.Fatalf("wanted %d, got %d", 0, len(storedRouteInfo.Local))
-	}
-	if slices.Contains(storedRouteInfo.Local, prefix1) {
-		t.Fatalf("New local route not stored.")
+	if storedRouteInfo != nil {
+		t.Fatalf("wanted nil, got %v", storedRouteInfo)
 	}
 
 	//Patch again with no route, see if prefix1 is removed/ prefix2, prefix3 presists.