[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.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Maxim Egorov
2026-08-21 14:04:20 +02:00
parent 79a06720b6
commit f4d9c1c0d0
2 changed files with 57 additions and 8 deletions

View File

@@ -32,6 +32,23 @@ 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 naming only unavailable routes would deselect everything the user had and put
// nothing in its place. Requesting no routes at all is a different thing - that's the
// caller asking to deselect all - and 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 +63,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

View File

@@ -887,3 +887,40 @@ 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")
}
// TestRouteSelector_SelectRoutes_AllUnavailableKeepsSelection covers the destructive case: a
// non-append selection clears the current selection before applying the requested one, so a
// request naming only unavailable routes used to leave everything deselected while still
// returning an error - a typo in a route ID silently dropped the user's exit node. A request
// with at least one available route keeps applying the valid part (see "Select non-existing
// route" above); this is only about the all-invalid case.
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")
}
}
// TestRouteSelector_SelectRoutes_EmptyRequestStillDeselectsAll guards the boundary of the check
// above: asking for no routes is the caller deselecting everything, not a failed request, 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")
}
}