[client] Keep the route selection on an invalid request and apply it on a partial one (#7292)

* [client] Keep the route selection when every requested ID is unavailable

A non-append SelectRoutes() wipes the current selection before applying
the requested one, but it validated the requested IDs only afterwards,
while already mutating. A request naming no available route at all left
every route deselected and returned an error - so a typo in a route ID
silently dropped the user's exit node, and the routes stayed applied
while the selector claimed nothing was selected.

Validate first and bail out before touching any state when nothing in
the request is available. A request with at least one available route
keeps applying the valid part and reporting the rest, and an empty
request still deselects everything, since that is the caller asking for
exactly that rather than a failed lookup.

* [client] Trim the new comments to the contributing guide's length budget

CONTRIBUTING.md caps comments at 90 characters per line and roughly 250
per comment. The three comments added by this PR were over both limits.
The test comments also restated their own test names, so they lose that
half and keep only the why.

* [client] Apply the route selection even when some IDs are unknown

SelectRoutes and DeselectRoutes returned the error before TriggerSelection,
so a request mixing valid and unknown network IDs changed the selector but
never reached the routing table. The valid routes read as selected while
`ip route` showed nothing.

Trigger the selection first and return the error afterwards. The inner
selectRoutes already applied the valid part of a partial request, only the
outer layer dropped it.

* [client] Publish the network selection event on a partial failure

Returning early on error was correct while an error meant nothing had
happened. A partial failure now changes the selection and the routing
table, so returning first left the change with no trace in the event log
or the UI, even though the new state had already been broadcast.

* [client] Cover the append and deselect-all paths of the selection guard

The append path was never destructive and behaves the same with or without
the early return, so that case is characterization rather than a regression
test. The deselect-all case is a real guard: the early return also skips
resetting deselectAll, so a typo no longer drops the "nothing selected,
including future networks" policy.

* [client] Pin that a fully invalid selection disturbs nothing

The selection is now applied on every request, including one where no ID is
known and the selector is left untouched. Nothing may be torn down or
reinstalled on that path.

* Revert "[client] Publish the network selection event on a partial failure"

This reverts commit 26219592.

The event would lie on the opposite path: when no requested ID is available
the selector is left untouched, so an unconditional publish reports a change
that never happened. Telling that case from a partial failure needs the
manager to report whether anything was applied, which is a new signal in its
API and does not belong in a PR about the selector guard. Follow-up instead.

---------

Co-authored-by: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com>

client/internal/routemanager/selection.go

@@ -17,23 +17,30 @@ import (
 // are mutually exclusive: if the selection activates an exit node, every other
 // available exit node is deselected so two can't be active at once. With
 // appendRoute=false the previous selection is replaced instead of extended.
+// A partial failure (e.g. an unknown ID mixed with valid ones) still applies
+// the valid IDs to the routing table; the unknown ones are reported in the
+// returned error.
 func (m *DefaultManager) SelectRoutes(ids []route.NetID, appendRoute bool) error {
-	if err := m.selectRoutes(ids, appendRoute); err != nil {
-		return err
-	}
+	err := m.selectRoutes(ids, appendRoute)
+	// Apply regardless of err: selectRoutes already selects the valid part of a
+	// partial request, and skipping this on error would leave those routes
+	// selected in the selector but never installed in the routing table.
 	m.TriggerSelection(m.GetClientRoutes())
-	return nil
+	return err
 }
 
 // DeselectRoutes removes the routes with the given network IDs from the
 // selection and applies the change. V4/v6 exit-node pairs are expanded
-// automatically.
+// automatically. A partial failure (e.g. an unknown ID mixed with valid ones)
+// still applies the valid IDs to the routing table; the unknown ones are
+// reported in the returned error.
 func (m *DefaultManager) DeselectRoutes(ids []route.NetID) error {
-	if err := m.deselectRoutes(ids); err != nil {
-		return err
-	}
+	err := m.deselectRoutes(ids)
+	// Apply regardless of err: deselectRoutes already deselects the valid part
+	// of a partial request, and skipping this on error would leave those routes
+	// installed in the routing table despite being marked deselected.
 	m.TriggerSelection(m.GetClientRoutes())
-	return nil
+	return err
 }
 
 func (m *DefaultManager) deselectRoutes(ids []route.NetID) error {

client/internal/routemanager/selection_test.go

@@ -1,12 +1,17 @@
 package routemanager
 
 import (
+	"context"
 	"net/netip"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/maps"
 
+	"github.com/netbirdio/netbird/client/internal/peer"
+	"github.com/netbirdio/netbird/client/internal/routemanager/client"
+	"github.com/netbirdio/netbird/client/internal/routemanager/notifier"
 	"github.com/netbirdio/netbird/client/internal/routeselector"
 	"github.com/netbirdio/netbird/route"
 )
@@ -112,6 +117,75 @@ func TestSelectRoutes_UnknownRoute(t *testing.T) {
 	assert.Error(t, m.deselectRoutes([]route.NetID{"missing"}), "deselecting an unavailable route must fail")
 }
 
+// newPartialFailureTestManager exercises the real install/remove path without
+// touching the system: the noop refcounter absorbs the route changes, and every
+// route already has a watcher, so none is started.
+func newPartialFailureTestManager() *DefaultManager {
+	ctx := context.Background()
+
+	m := &DefaultManager{
+		ctx: ctx,
+		clientRoutes: route.HAMap{
+			"lan|192.168.1.0/24": {{NetID: "lan", Network: netip.MustParsePrefix("192.168.1.0/24"), Peer: "p1"}},
+			"other|10.1.2.0/24":  {{NetID: "other", Network: netip.MustParsePrefix("10.1.2.0/24"), Peer: "p2"}},
+		},
+		routeSelector:  routeselector.NewRouteSelector(),
+		notifier:       notifier.NewNotifier(),
+		statusRecorder: peer.NewRecorder("https://mgm"),
+		activeRoutes:   make(map[route.HAUniqueID]client.RouteHandler),
+		clientNetworks: map[route.HAUniqueID]*client.Watcher{
+			"lan|192.168.1.0/24": client.NewWatcher(client.WatcherConfig{Context: ctx}),
+			"other|10.1.2.0/24":  client.NewWatcher(client.WatcherConfig{Context: ctx}),
+		},
+	}
+	m.setupRefCounters(true)
+	return m
+}
+
+// Regression for the reported symptom: a partial failure returned before
+// TriggerSelection ran, so the valid route was marked selected while never
+// reaching the routing table (activeRoutes/ip route).
+func TestSelectRoutes_PartialFailureStillInstallsValidRoute(t *testing.T) {
+	m := newPartialFailureTestManager()
+
+	err := m.SelectRoutes([]route.NetID{"missing", "lan"}, false)
+
+	assert.Error(t, err, "the unknown id must still be reported")
+	assert.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"), "the valid route must be installed despite the error")
+	assert.NotContains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"), "the deselected route must not be installed")
+}
+
+// Mirror of the case above: a partial failure must remove the valid route from
+// the routing table, not just mark it deselected in the selector.
+func TestDeselectRoutes_PartialFailureStillRemovesValidRoute(t *testing.T) {
+	m := newPartialFailureTestManager()
+
+	require.NoError(t, m.SelectRoutes([]route.NetID{"lan", "other"}, false))
+	require.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"))
+	require.Contains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"))
+
+	err := m.DeselectRoutes([]route.NetID{"missing", "other"})
+
+	assert.Error(t, err, "the unknown id must still be reported")
+	assert.NotContains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"), "the deselected route must be removed")
+	assert.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"), "the untouched route stays installed")
+}
+
+// The selection now runs on every request, including one where no ID is known
+// and the selector stays untouched. Nothing may be torn down or reinstalled on
+// that path.
+func TestSelectRoutes_TotalFailureLeavesInstalledRoutesAlone(t *testing.T) {
+	m := newPartialFailureTestManager()
+
+	require.NoError(t, m.SelectRoutes([]route.NetID{"lan", "other"}, false))
+	installed := maps.Keys(m.activeRoutes)
+
+	err := m.SelectRoutes([]route.NetID{"missing"}, false)
+
+	assert.Error(t, err, "the unknown id must still be reported")
+	assert.ElementsMatch(t, installed, maps.Keys(m.activeRoutes), "a fully invalid request must not disturb the routing table")
+}
+
 func TestExitNodeSelectionHelpers(t *testing.T) {
 	routesMap := map[route.NetID][]*route.Route{
 		"exitA": {{Network: netip.MustParsePrefix("0.0.0.0/0")}},

client/internal/routeselector/routeselector.go

@@ -32,6 +32,22 @@ func (rs *RouteSelector) SelectRoutes(routes []route.NetID, appendRoute bool, al
 	rs.mu.Lock()
 	defer rs.mu.Unlock()
 
+	// Validate before mutating: a non-append selection wipes the current selection
+	// first, so a request of only unavailable routes would deselect everything and
+	// put nothing back. An empty request means deselect all, so it still goes through.
+	var err *multierror.Error
+	available := make([]route.NetID, 0, len(routes))
+	for _, r := range routes {
+		if !slices.Contains(allRoutes, r) {
+			err = multierror.Append(err, fmt.Errorf("route '%s' is not available", r))
+			continue
+		}
+		available = append(available, r)
+	}
+	if len(available) == 0 && err != nil {
+		return errors.FormatErrorOrNil(err)
+	}
+
 	if !appendRoute || rs.deselectAll {
 		if rs.deselectedRoutes == nil {
 			rs.deselectedRoutes = map[route.NetID]struct{}{}
@@ -46,14 +62,9 @@ func (rs *RouteSelector) SelectRoutes(routes []route.NetID, appendRoute bool, al
 		}
 	}
 
-	var err *multierror.Error
-	for _, route := range routes {
-		if !slices.Contains(allRoutes, route) {
-			err = multierror.Append(err, fmt.Errorf("route '%s' is not available", route))
-			continue
-		}
-		delete(rs.deselectedRoutes, route)
-		rs.selectedRoutes[route] = struct{}{}
+	for _, r := range available {
+		delete(rs.deselectedRoutes, r)
+		rs.selectedRoutes[r] = struct{}{}
 	}
 
 	rs.deselectAll = false

client/internal/routeselector/routeselector_test.go

@@ -887,3 +887,70 @@ func TestRouteSelector_EnableExitNodeKeepsOtherRoutes(t *testing.T) {
 	assert.True(t, rs.IsSelected("lan1"), "non-exit route must stay selected")
 	assert.True(t, rs.IsSelected("lan2"), "non-exit route must stay selected")
 }
+
+// A non-append selection clears the current selection before applying the requested
+// one, so an all-unavailable request used to leave nothing selected while returning
+// an error. Requests with at least one available route are unaffected.
+func TestRouteSelector_SelectRoutes_AllUnavailableKeepsSelection(t *testing.T) {
+	allRoutes := []route.NetID{"route1", "route2", "route3"}
+
+	rs := routeselector.NewRouteSelector()
+	require.NoError(t, rs.SelectRoutes([]route.NetID{"route1"}, false, allRoutes))
+
+	err := rs.SelectRoutes([]route.NetID{"Route1", "route4"}, false, allRoutes)
+
+	assert.Error(t, err, "an unavailable route ID must still be reported")
+	assert.True(t, rs.IsSelected("route1"), "the previous selection must survive a fully invalid request")
+	for _, id := range []route.NetID{"route2", "route3"} {
+		assert.False(t, rs.IsSelected(id), "no other route may become selected")
+	}
+}
+
+// Boundary of the check above: an empty request is the caller deselecting everything,
+// not a failed lookup, so it must keep working.
+func TestRouteSelector_SelectRoutes_EmptyRequestStillDeselectsAll(t *testing.T) {
+	allRoutes := []route.NetID{"route1", "route2", "route3"}
+
+	rs := routeselector.NewRouteSelector()
+	require.NoError(t, rs.SelectRoutes([]route.NetID{"route1"}, false, allRoutes))
+
+	require.NoError(t, rs.SelectRoutes(nil, false, allRoutes))
+
+	for _, id := range allRoutes {
+		assert.False(t, rs.IsSelected(id), "an empty selection request must deselect everything")
+	}
+}
+
+// Mobile clients always call SelectRoutes with append=true. On that path an
+// all-unavailable request was never destructive to begin with (append skips the
+// wipe regardless of the guard above), but the behavior has no coverage yet.
+func TestRouteSelector_SelectRoutes_AppendAllUnavailableKeepsSelection(t *testing.T) {
+	allRoutes := []route.NetID{"route1", "route2", "route3"}
+
+	rs := routeselector.NewRouteSelector()
+	require.NoError(t, rs.SelectRoutes([]route.NetID{"route1"}, false, allRoutes))
+
+	err := rs.SelectRoutes([]route.NetID{"missing"}, true, allRoutes)
+
+	assert.Error(t, err, "an unavailable route ID must still be reported")
+	assert.True(t, rs.IsSelected("route1"), "the previous selection must survive a fully invalid request")
+	for _, id := range []route.NetID{"route2", "route3"} {
+		assert.False(t, rs.IsSelected(id), "no other route may become selected")
+	}
+}
+
+// The early return for an all-unavailable request must not clear deselectAll,
+// or a typo'd network ID would silently drop the "nothing selected, including
+// future networks" policy.
+func TestRouteSelector_SelectRoutes_AllUnavailableAfterDeselectAllKeepsPolicy(t *testing.T) {
+	allRoutes := []route.NetID{"route1", "route2"}
+
+	rs := routeselector.NewRouteSelector()
+	rs.DeselectAllRoutes()
+
+	err := rs.SelectRoutes([]route.NetID{"missing"}, false, allRoutes)
+
+	assert.Error(t, err, "an unavailable route ID must still be reported")
+	assert.True(t, rs.IsDeselectAll(), "deselect-all policy must survive a fully invalid request")
+	assert.False(t, rs.IsSelected("route3"), "deselect-all must still cover networks not present in allRoutes yet")
+}