[client] make AllowedIPs swap self-healing on WireGuard errors

Address CodeRabbit review on #6799.

Decrement now decides whether to reprogram based on whether the currently
installed peer still holds references (e.peers[e.active] > 0) instead of
whether the released peer was the active one. If a prior swap's remove/add
failed, e.active was left pointing at a peer with zero references and every
later Decrement returned early, permanently stranding the prefix and leaking
the entry. The active peer is now detached only when e.active != "", and a
failed hand-off is retried on the next Decrement/Increment.

Also clear currentPeerKey unconditionally in the static handler's
RemoveAllowedIPs (matching dynamic/dnsinterceptor) and assert Increment
errors in the tests. Added self-heal tests for the failed remove/add paths.

client/internal/routemanager/dnsinterceptor/handler.go

@@ -95,7 +95,7 @@ func (d *DnsInterceptor) RemoveRoute() error {
 
 			// AllowedIPs should use real IPs
 			if d.currentPeerKey != "" {
-				if _, err := d.allowedIPsRefcounter.Decrement(prefix); err != nil {
+				if _, err := d.allowedIPsRefcounter.Decrement(prefix, d.currentPeerKey); err != nil {
 					merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %v", prefix, err))
 				}
 			}
@@ -172,7 +172,7 @@ func (d *DnsInterceptor) removeAllowedIP(realPrefix netip.Prefix) error {
 	}
 
 	// AllowedIPs use real IPs
-	if _, err := d.allowedIPsRefcounter.Decrement(realPrefix); err != nil {
+	if _, err := d.allowedIPsRefcounter.Decrement(realPrefix, d.currentPeerKey); err != nil {
 		return fmt.Errorf("remove allowed IP %s: %v", realPrefix, err)
 	}
 
@@ -205,7 +205,7 @@ func (d *DnsInterceptor) RemoveAllowedIPs() error {
 	for _, prefixes := range d.interceptedDomains {
 		for _, prefix := range prefixes {
 			// AllowedIPs use real IPs
-			if _, err := d.allowedIPsRefcounter.Decrement(prefix); err != nil {
+			if _, err := d.allowedIPsRefcounter.Decrement(prefix, d.currentPeerKey); err != nil {
 				merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %v", prefix, err))
 			}
 		}

client/internal/routemanager/dynamic/route.go

@@ -135,7 +135,7 @@ func (r *Route) RemoveAllowedIPs() error {
 	var merr *multierror.Error
 	for _, domainPrefixes := range r.dynamicDomains {
 		for _, prefix := range domainPrefixes {
-			if _, err := r.allowedIPsRefcounter.Decrement(prefix); err != nil {
+			if _, err := r.allowedIPsRefcounter.Decrement(prefix, r.currentPeerKey); err != nil {
 				merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %w", prefix, err))
 			}
 		}
@@ -320,7 +320,7 @@ func (r *Route) removeRoutes(prefixes []netip.Prefix) ([]netip.Prefix, error) {
 			merr = multierror.Append(merr, fmt.Errorf("remove dynamic route for IP %s: %w", prefix, err))
 		}
 		if r.currentPeerKey != "" {
-			if _, err := r.allowedIPsRefcounter.Decrement(prefix); err != nil {
+			if _, err := r.allowedIPsRefcounter.Decrement(prefix, r.currentPeerKey); err != nil {
 				merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %s: %w", prefix, err))
 			}
 		}

client/internal/routemanager/manager.go

@@ -215,7 +215,7 @@ func (m *DefaultManager) setupRefCounters(useNoop bool) {
 		)
 	}
 
-	m.allowedIPsRefCounter = refcounter.New(
+	m.allowedIPsRefCounter = refcounter.NewAllowedIPs(
 		func(prefix netip.Prefix, peerKey string) (string, error) {
 			// save peerKey to use it in the remove function
 			return peerKey, m.wgInterface.AddAllowedIP(peerKey, prefix)

client/internal/routemanager/refcounter/allowedips.go

@@ -0,0 +1,185 @@
+package refcounter
+
+import (
+	"errors"
+	"fmt"
+	"net/netip"
+	"sort"
+	"sync"
+
+	"github.com/hashicorp/go-multierror"
+
+	nberrors "github.com/netbirdio/netbird/client/errors"
+)
+
+// allowedIPsEntry holds the per-peer reference counts for a single prefix and which peer is
+// currently installed in WireGuard. WireGuard allows a prefix on exactly one peer, so at most
+// one peer is active at a time even when several peers reference the prefix.
+type allowedIPsEntry struct {
+	// peers maps a peerKey to the number of references holding the prefix for that peer.
+	peers map[string]int
+	// active is the peerKey currently installed in WireGuard for this prefix ("" if none).
+	active string
+	// total is the sum of all per-peer reference counts (kept in sync with peers).
+	total int
+}
+
+// AllowedIPsRefCounter is a peer-aware reference counter for WireGuard AllowedIPs.
+//
+// The generic Counter keys only by prefix and remembers a single Out value set by the first
+// caller, which it never changes. That is wrong for AllowedIPs: two independent watchers (or
+// multiple resolved domains) can reference the same prefix through different peers, and when the
+// peer currently installed in WireGuard releases its last reference the prefix must be handed over
+// to a surviving peer instead of being left pointing at the released one.
+//
+// It calls add/remove (which program WireGuard) only on the transitions that matter:
+//   - add on the first reference for a prefix, or when swapping the active peer;
+//   - remove on the last reference for a prefix, or on the old peer during a swap.
+type AllowedIPsRefCounter struct {
+	mu      sync.Mutex
+	entries map[netip.Prefix]*allowedIPsEntry
+	add     AddFunc[netip.Prefix, string, string]
+	remove  RemoveFunc[netip.Prefix, string]
+}
+
+// NewAllowedIPs creates a new peer-aware AllowedIPs reference counter.
+// add programs a prefix on a peer in WireGuard and returns the peerKey to store as the active peer.
+// remove unprograms the prefix from the given peer.
+func NewAllowedIPs(add AddFunc[netip.Prefix, string, string], remove RemoveFunc[netip.Prefix, string]) *AllowedIPsRefCounter {
+	return &AllowedIPsRefCounter{
+		entries: map[netip.Prefix]*allowedIPsEntry{},
+		add:     add,
+		remove:  remove,
+	}
+}
+
+// Increment adds a reference to prefix for peerKey. WireGuard is programmed only for the first
+// reference to a prefix; while a different peer is already installed the prefix is left with it
+// (first peer wins, HA at the WireGuard layer is not possible) and only the reference count is kept.
+func (rm *AllowedIPsRefCounter) Increment(prefix netip.Prefix, peerKey string) (Ref[string], error) {
+	rm.mu.Lock()
+	defer rm.mu.Unlock()
+
+	e, ok := rm.entries[prefix]
+	if !ok {
+		e = &allowedIPsEntry{peers: map[string]int{}}
+		rm.entries[prefix] = e
+	}
+
+	logCallerF("Increasing allowed IP ref count for prefix %v peer %s [peer %d -> %d, total %d -> %d, active %q]",
+		prefix, peerKey, e.peers[peerKey], e.peers[peerKey]+1, e.total, e.total+1, e.active)
+
+	// Program WireGuard only when nothing is installed yet for this prefix.
+	if e.active == "" {
+		out, err := rm.add(prefix, peerKey)
+		if errors.Is(err, ErrIgnore) {
+			if e.total == 0 {
+				delete(rm.entries, prefix)
+			}
+			return Ref[string]{Count: e.total, Out: e.active}, nil
+		}
+		if err != nil {
+			if e.total == 0 {
+				delete(rm.entries, prefix)
+			}
+			return Ref[string]{}, fmt.Errorf("failed to add allowed IP %v for peer %s: %w", prefix, peerKey, err)
+		}
+		e.active = out
+	}
+
+	e.peers[peerKey]++
+	e.total++
+
+	return Ref[string]{Count: e.total, Out: e.active}, nil
+}
+
+// Decrement removes a reference to prefix for peerKey. When the peer currently installed in
+// WireGuard releases its last reference, the prefix is swapped to a surviving peer if one exists,
+// otherwise it is removed from WireGuard.
+func (rm *AllowedIPsRefCounter) Decrement(prefix netip.Prefix, peerKey string) (Ref[string], error) {
+	rm.mu.Lock()
+	defer rm.mu.Unlock()
+
+	e, ok := rm.entries[prefix]
+	if !ok {
+		logCallerF("No allowed IP reference found for prefix %v", prefix)
+		return Ref[string]{}, nil
+	}
+
+	if e.peers[peerKey] > 0 {
+		logCallerF("Decreasing allowed IP ref count for prefix %v peer %s [peer %d -> %d, total %d -> %d, active %q]",
+			prefix, peerKey, e.peers[peerKey], e.peers[peerKey]-1, e.total, e.total-1, e.active)
+		e.peers[peerKey]--
+		e.total--
+		if e.peers[peerKey] == 0 {
+			delete(e.peers, peerKey)
+		}
+	} else {
+		logCallerF("No allowed IP reference found for prefix %v peer %s", prefix, peerKey)
+	}
+
+	// If the peer currently installed in WireGuard still holds references, nothing to reprogram.
+	// Keying the check on the active peer (not the one just released) makes this self-healing:
+	// a prior swap whose remove/add failed leaves e.active pointing at a peer with no references,
+	// and this retries the hand-off on the next Decrement instead of getting stuck.
+	if e.active != "" && e.peers[e.active] > 0 {
+		return Ref[string]{Count: e.total, Out: e.active}, nil
+	}
+
+	// Detach the stale/gone active peer from WireGuard before reprogramming.
+	if e.active != "" {
+		if err := rm.remove(prefix, e.active); err != nil {
+			return Ref[string]{Count: e.total, Out: e.active}, fmt.Errorf("remove allowed IP %v for peer %s: %w", prefix, e.active, err)
+		}
+		e.active = ""
+	}
+
+	// Hand the prefix over to a surviving peer, or drop the entry when none remain.
+	if survivor, ok := pickSurvivor(e.peers); ok {
+		out, err := rm.add(prefix, survivor)
+		if err != nil {
+			return Ref[string]{Count: e.total, Out: ""}, fmt.Errorf("swap allowed IP %v to peer %s: %w", prefix, survivor, err)
+		}
+		e.active = out
+		return Ref[string]{Count: e.total, Out: e.active}, nil
+	}
+
+	delete(rm.entries, prefix)
+	return Ref[string]{Count: 0, Out: ""}, nil
+}
+
+// Flush removes all prefixes from WireGuard and clears the counter.
+func (rm *AllowedIPsRefCounter) Flush() error {
+	rm.mu.Lock()
+	defer rm.mu.Unlock()
+
+	var merr *multierror.Error
+	for prefix, e := range rm.entries {
+		if e.active == "" {
+			continue
+		}
+		logCallerF("Flushing allowed IP for prefix %v peer %s", prefix, e.active)
+		if err := rm.remove(prefix, e.active); err != nil {
+			merr = multierror.Append(merr, fmt.Errorf("remove allowed IP %v for peer %s: %w", prefix, e.active, err))
+		}
+	}
+
+	clear(rm.entries)
+
+	return nberrors.FormatErrorOrNil(merr)
+}
+
+// pickSurvivor deterministically selects a peer still referencing the prefix. WireGuard cannot do
+// multipath for a single prefix, so any surviving peer is a valid winner; the choice is made stable
+// (lowest peerKey) for predictable behavior and testability.
+func pickSurvivor(peers map[string]int) (string, bool) {
+	if len(peers) == 0 {
+		return "", false
+	}
+	keys := make([]string, 0, len(peers))
+	for k := range peers {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	return keys[0], true
+}

client/internal/routemanager/refcounter/allowedips_test.go

@@ -0,0 +1,241 @@
+package refcounter
+
+import (
+	"errors"
+	"net/netip"
+	"testing"
+)
+
+// fakeWG models WireGuard's cryptokey routing: a prefix can be installed on exactly one peer.
+// failAdd/failRemove make the next add/remove fail once, to exercise the self-healing error paths.
+type fakeWG struct {
+	installed  map[netip.Prefix]string
+	adds       int
+	removes    int
+	failAdd    bool
+	failRemove bool
+}
+
+func newFakeWG() *fakeWG {
+	return &fakeWG{installed: map[netip.Prefix]string{}}
+}
+
+func (f *fakeWG) counter() *AllowedIPsRefCounter {
+	return NewAllowedIPs(
+		func(prefix netip.Prefix, peerKey string) (string, error) {
+			if f.failAdd {
+				f.failAdd = false
+				return "", errors.New("add failed")
+			}
+			f.adds++
+			f.installed[prefix] = peerKey
+			return peerKey, nil
+		},
+		func(prefix netip.Prefix, peerKey string) error {
+			if f.failRemove {
+				f.failRemove = false
+				return errors.New("remove failed")
+			}
+			f.removes++
+			// only clear if this peer is the one installed, mirroring wg semantics
+			if f.installed[prefix] == peerKey {
+				delete(f.installed, prefix)
+			}
+			return nil
+		},
+	)
+}
+
+func mustPrefix(t *testing.T, s string) netip.Prefix {
+	t.Helper()
+	p, err := netip.ParsePrefix(s)
+	if err != nil {
+		t.Fatalf("parse prefix %q: %v", s, err)
+	}
+	return p
+}
+
+func mustIncrement(t *testing.T, c *AllowedIPsRefCounter, p netip.Prefix, peer string) Ref[string] {
+	t.Helper()
+	ref, err := c.Increment(p, peer)
+	if err != nil {
+		t.Fatalf("Increment(%v, %s): %v", p, peer, err)
+	}
+	return ref
+}
+
+func mustDecrement(t *testing.T, c *AllowedIPsRefCounter, p netip.Prefix, peer string) Ref[string] {
+	t.Helper()
+	ref, err := c.Decrement(p, peer)
+	if err != nil {
+		t.Fatalf("Decrement(%v, %s): %v", p, peer, err)
+	}
+	return ref
+}
+
+// TestAllowedIPs_SwapOnActivePeerRemoval reproduces the reported bug: two networks with the same
+// prefix routed by different peers. Removing the network whose peer is installed must hand the
+// prefix over to the surviving peer instead of leaving it on the removed one.
+func TestAllowedIPs_SwapOnActivePeerRemoval(t *testing.T) {
+	f := newFakeWG()
+	c := f.counter()
+	p := mustPrefix(t, "10.44.8.0/24")
+
+	mustIncrement(t, c, p, "peerA")
+	mustIncrement(t, c, p, "peerB")
+	// First peer wins while both are present.
+	if got := f.installed[p]; got != "peerA" {
+		t.Fatalf("expected peerA installed, got %q", got)
+	}
+
+	// Remove the active peer's network -> must swap to peerB.
+	mustDecrement(t, c, p, "peerA")
+	if got := f.installed[p]; got != "peerB" {
+		t.Fatalf("BUG: prefix stuck on removed peer, want peerB got %q", got)
+	}
+
+	// Remove the last one -> prefix gone.
+	mustDecrement(t, c, p, "peerB")
+	if _, ok := f.installed[p]; ok {
+		t.Fatalf("expected prefix removed, still installed on %q", f.installed[p])
+	}
+}
+
+// TestAllowedIPs_RemoveNonActivePeer removing a non-installed peer must not touch WireGuard.
+func TestAllowedIPs_RemoveNonActivePeer(t *testing.T) {
+	f := newFakeWG()
+	c := f.counter()
+	p := mustPrefix(t, "10.44.8.0/24")
+
+	mustIncrement(t, c, p, "peerA")
+	mustIncrement(t, c, p, "peerB")
+	removesBefore := f.removes
+
+	mustDecrement(t, c, p, "peerB")
+	if f.installed[p] != "peerA" {
+		t.Fatalf("active peer must stay peerA, got %q", f.installed[p])
+	}
+	if f.removes != removesBefore {
+		t.Fatalf("removing a non-active peer must not call wg remove")
+	}
+}
+
+// TestAllowedIPs_SamePeerMultipleRefs two references via the same peer must keep the prefix until
+// the last reference is released (the reason the per-peer count must be an int, not a set).
+func TestAllowedIPs_SamePeerMultipleRefs(t *testing.T) {
+	f := newFakeWG()
+	c := f.counter()
+	p := mustPrefix(t, "10.44.8.0/24")
+
+	mustIncrement(t, c, p, "peerA")
+	mustIncrement(t, c, p, "peerA")
+	if f.adds != 1 {
+		t.Fatalf("expected a single wg add for the same peer, got %d", f.adds)
+	}
+
+	mustDecrement(t, c, p, "peerA")
+	if f.installed[p] != "peerA" {
+		t.Fatalf("prefix must stay while a reference remains, got %q", f.installed[p])
+	}
+	if f.removes != 0 {
+		t.Fatalf("no wg remove expected while a reference remains, got %d", f.removes)
+	}
+
+	mustDecrement(t, c, p, "peerA")
+	if _, ok := f.installed[p]; ok {
+		t.Fatalf("prefix must be removed after last reference")
+	}
+}
+
+// TestAllowedIPs_RefCountAndActive checks the Ref returned to callers (used for the HA-disabled log).
+func TestAllowedIPs_RefCountAndActive(t *testing.T) {
+	f := newFakeWG()
+	c := f.counter()
+	p := mustPrefix(t, "10.44.8.0/24")
+
+	ref := mustIncrement(t, c, p, "peerA")
+	if ref.Count != 1 || ref.Out != "peerA" {
+		t.Fatalf("want {1, peerA}, got {%d, %q}", ref.Count, ref.Out)
+	}
+	ref = mustIncrement(t, c, p, "peerB")
+	if ref.Count != 2 || ref.Out != "peerA" {
+		t.Fatalf("want {2, peerA}, got {%d, %q}", ref.Count, ref.Out)
+	}
+}
+
+// TestAllowedIPs_Flush removes everything installed and clears the counter.
+func TestAllowedIPs_Flush(t *testing.T) {
+	f := newFakeWG()
+	c := f.counter()
+	p1 := mustPrefix(t, "10.44.8.0/24")
+	p2 := mustPrefix(t, "10.44.9.0/24")
+
+	mustIncrement(t, c, p1, "peerA")
+	mustIncrement(t, c, p2, "peerB")
+
+	if err := c.Flush(); err != nil {
+		t.Fatal(err)
+	}
+	if len(f.installed) != 0 {
+		t.Fatalf("expected all prefixes removed, got %v", f.installed)
+	}
+	// After flush, a fresh increment must add again.
+	mustIncrement(t, c, p1, "peerC")
+	if f.installed[p1] != "peerC" {
+		t.Fatalf("counter not reset after flush")
+	}
+}
+
+// TestAllowedIPs_SelfHealAfterSwapAddError ensures a failed add during a swap does not permanently
+// strand the prefix: the next Decrement (or Increment) must retry and install a surviving peer.
+func TestAllowedIPs_SelfHealAfterSwapAddError(t *testing.T) {
+	f := newFakeWG()
+	c := f.counter()
+	p := mustPrefix(t, "10.44.8.0/24")
+
+	mustIncrement(t, c, p, "peerA")
+	mustIncrement(t, c, p, "peerB")
+	mustIncrement(t, c, p, "peerC")
+
+	// Removing the active peerA triggers a swap to a survivor; make the add fail once.
+	f.failAdd = true
+	if _, err := c.Decrement(p, "peerA"); err == nil {
+		t.Fatalf("expected error from failed swap add")
+	}
+	if _, ok := f.installed[p]; ok {
+		t.Fatalf("nothing should be installed after a failed swap add, got %q", f.installed[p])
+	}
+
+	// A later Decrement of a non-active survivor must retry the hand-off (self-heal), not stay stuck.
+	ref := mustDecrement(t, c, p, "peerC")
+	if got := f.installed[p]; got == "" {
+		t.Fatalf("self-heal failed: prefix left unrouted after add recovered")
+	}
+	if ref.Out == "" {
+		t.Fatalf("expected an active peer after self-heal, got empty")
+	}
+}
+
+// TestAllowedIPs_SelfHealAfterRemoveError ensures a failed remove during a swap is retried instead
+// of leaving e.active stuck on a peer that no longer holds references.
+func TestAllowedIPs_SelfHealAfterRemoveError(t *testing.T) {
+	f := newFakeWG()
+	c := f.counter()
+	p := mustPrefix(t, "10.44.8.0/24")
+
+	mustIncrement(t, c, p, "peerA")
+	mustIncrement(t, c, p, "peerB")
+
+	// Releasing active peerA must detach it (remove) then add peerB; fail the remove once.
+	f.failRemove = true
+	if _, err := c.Decrement(p, "peerA"); err == nil {
+		t.Fatalf("expected error from failed remove")
+	}
+
+	// Next Decrement of the non-active survivor retries: removes stale peerA, installs peerB.
+	mustDecrement(t, c, p, "peerB")
+	// peerB had only one ref, so after retry the prefix is fully released.
+	if _, ok := f.installed[p]; ok {
+		t.Fatalf("expected prefix released after self-heal, still on %q", f.installed[p])
+	}
+}

client/internal/routemanager/refcounter/types.go

@@ -5,5 +5,7 @@ import "net/netip"
 // RouteRefCounter is a Counter for Route, it doesn't take any input on Increment and doesn't use any output on Decrement
 type RouteRefCounter = Counter[netip.Prefix, struct{}, struct{}]
 
-// AllowedIPsRefCounter is a Counter for AllowedIPs, it takes a peer key on Increment and passes it back to Decrement
-type AllowedIPsRefCounter = Counter[netip.Prefix, string, string]
+// AllowedIPsRefCounter tracks WireGuard AllowedIPs per prefix. Unlike the generic Counter it is peer-aware:
+// a prefix can be claimed by several peers at once and WireGuard allows a given prefix on exactly one peer,
+// so the counter records the per-peer reference count and swaps the installed peer when the active one is released.
+// See allowedips.go.

client/internal/routemanager/static/route.go

@@ -15,6 +15,11 @@ type Route struct {
 	route                *route.Route
 	routeRefCounter      *refcounter.RouteRefCounter
 	allowedIPsRefcounter *refcounter.AllowedIPsRefCounter
+	// currentPeerKey is the routing peer this watcher currently has the prefix installed on
+	// (the HA winner elected by the watcher). It can differ from route.Peer and change on
+	// failover, so it is recorded on AddAllowedIPs and used on RemoveAllowedIPs to decrement
+	// the exact peer that was incremented.
+	currentPeerKey string
 }
 
 func NewRoute(params common.HandlerParams) *Route {
@@ -52,12 +57,15 @@ func (r *Route) AddAllowedIPs(peerKey string) error {
 			ref.Out,
 		)
 	}
+	r.currentPeerKey = peerKey
 	return nil
 }
 
 func (r *Route) RemoveAllowedIPs() error {
-	if _, err := r.allowedIPsRefcounter.Decrement(r.route.Network); err != nil {
-		return err
+	var err error
+	if _, decErr := r.allowedIPsRefcounter.Decrement(r.route.Network, r.currentPeerKey); decErr != nil {
+		err = fmt.Errorf("remove allowed IP %s: %w", r.route.Network, decErr)
 	}
-	return nil
+	r.currentPeerKey = ""
+	return err
 }