hostmap: replace the shared next/prev hostinfo chain with independent per-address lists so divergent or overlapping vpnAddr sets cannot corrupt the map (#1790)

handshake_manager.go

@@ -430,14 +430,11 @@ func (hm *HandshakeManager) CheckAndComplete(hostinfo *HostInfo, handshakePacket
 	// Check if we already have a tunnel with this vpn ip
 	existingHostInfo, found := hm.mainHostMap.Hosts[hostinfo.vpnAddrs[0]]
 	if found && existingHostInfo != nil {
-		testHostInfo := existingHostInfo
-		for testHostInfo != nil {
-			// Is it just a delayed handshake packet?
+		// Is it just a delayed handshake packet? Check every hostinfo we hold for this address.
+		for _, testHostInfo := range hm.mainHostMap.unlockedGetHostList(hostinfo.vpnAddrs[0]) {
 			if bytes.Equal(hostinfo.HandshakePacket[handshakePacket], testHostInfo.HandshakePacket[handshakePacket]) {
 				return testHostInfo, ErrAlreadySeen
 			}
-
-			testHostInfo = testHostInfo.next
 		}
 
 		// Is this a newer handshake?

hostmap.go

@@ -56,11 +56,20 @@ type Relay struct {
 }
 
 type HostMap struct {
-	sync.RWMutex    //Because we concurrently read and write to our maps
-	Indexes         map[uint32]*HostInfo
-	Relays          map[uint32]*HostInfo // Maps a Relay IDX to a Relay HostInfo object
-	RemoteIndexes   map[uint32]*HostInfo
+	sync.RWMutex  //Because we concurrently read and write to our maps
+	Indexes       map[uint32]*HostInfo
+	Relays        map[uint32]*HostInfo // Maps a Relay IDX to a Relay HostInfo object
+	RemoteIndexes map[uint32]*HostInfo
+	// Hosts maps a vpn address to its primary hostinfo, one entry per address we hold a tunnel
+	// for. moreHosts only has an entry while an address is held by 2 or more hostinfos and stores
+	// the full most-recent-first list; moreHosts[a][0] is always the same hostinfo as Hosts[a].
+	// Each address gets its own independent list, so a hostinfo owning multiple addresses can
+	// never corrupt another address's ordering the way the old shared next/prev chain could.
+	// Entries in moreHosts are only ever written by unlockedSetHostsForAddr; Hosts is written
+	// directly only in the single-hostinfo fast paths where moreHosts is known to have no entry,
+	// and unlockedDeleteHostInfo swaps either map for a fresh one when it fully drains.
 	Hosts           map[netip.Addr]*HostInfo
+	moreHosts       map[netip.Addr][]*HostInfo
 	preferredRanges atomic.Pointer[[]netip.Prefix]
 	l               *slog.Logger
 }
@@ -266,10 +275,6 @@ type HostInfo struct {
 	lastRoam       time.Time
 	lastRoamRemote netip.AddrPort
 
-	// Used to track other hostinfos for this vpn ip since only 1 can be primary
-	// Synchronised via hostmap lock and not the hostinfo lock.
-	next, prev *HostInfo
-
 	//TODO: in, out, and others might benefit from being an atomic.Int32. We could collapse connectionManager pendingDeletion, relayUsed, and in/out into this 1 thing
 	in, out, pendingDeletion atomic.Bool
 
@@ -334,6 +339,7 @@ func newHostMap(l *slog.Logger) *HostMap {
 		Relays:        map[uint32]*HostInfo{},
 		RemoteIndexes: map[uint32]*HostInfo{},
 		Hosts:         map[netip.Addr]*HostInfo{},
+		moreHosts:     map[netip.Addr][]*HostInfo{},
 		l:             l,
 	}
 }
@@ -382,13 +388,55 @@ func (hm *HostMap) EmitStats() {
 	metrics.GetOrRegisterGauge("hostmap.main.relayIndexes", nil).Update(int64(relaysLen))
 }
 
-// DeleteHostInfo will fully unlink the hostinfo and return true if it was the final hostinfo for this vpn ip
+// unlockedSetHostsForAddr stores the per-address hostinfo list (list[0] is the primary). An empty
+// list removes the address. This is the one place Hosts and moreHosts are written together, keep
+// it that way. Callers must hold the write lock.
+func (hm *HostMap) unlockedSetHostsForAddr(addr netip.Addr, list []*HostInfo) {
+	if len(list) == 0 {
+		delete(hm.Hosts, addr)
+		delete(hm.moreHosts, addr)
+		return
+	}
+	hm.Hosts[addr] = list[0]
+	if len(list) > 1 {
+		hm.moreHosts[addr] = list
+	} else {
+		delete(hm.moreHosts, addr)
+	}
+}
+
+// unlockedGetHostList returns every hostinfo holding addr, primary first, or nil if we have no
+// tunnel for addr. The common single-hostinfo case builds a fresh one element list, so keep this
+// off the packet hot path; the primary is a direct Hosts read. Callers must hold the lock (read
+// or write).
+func (hm *HostMap) unlockedGetHostList(addr netip.Addr) []*HostInfo {
+	if list, ok := hm.moreHosts[addr]; ok {
+		return list
+	}
+	if h, ok := hm.Hosts[addr]; ok {
+		return []*HostInfo{h}
+	}
+	return nil
+}
+
+// removeHostInfo returns list with hi removed (order preserved), or list unchanged if hi is
+// absent. It deletes in place: every mutator holds the hostmap write lock and no reader ever
+// retains a slice across a mutation (readers iterate under RLock), so there is no snapshot to
+// invalidate.
+func removeHostInfo(list []*HostInfo, hi *HostInfo) []*HostInfo {
+	idx := slices.Index(list, hi)
+	if idx < 0 {
+		return list
+	}
+	return slices.Delete(list, idx, idx+1)
+}
+
+// DeleteHostInfo will fully unlink the hostinfo and return true if no other hostinfo still holds
+// any of its vpn addrs, meaning we no longer have a tunnel to the peer
 func (hm *HostMap) DeleteHostInfo(hostinfo *HostInfo) bool {
 	// Delete the host itself, ensuring it's not modified anymore
 	hm.Lock()
-	// If we have a previous or next hostinfo then we are not the last one for this vpn ip
-	final := (hostinfo.next == nil && hostinfo.prev == nil)
-	hm.unlockedDeleteHostInfo(hostinfo)
+	final := hm.unlockedDeleteHostInfo(hostinfo)
 	hm.Unlock()
 
 	return final
@@ -401,71 +449,63 @@ func (hm *HostMap) MakePrimary(hostinfo *HostInfo) {
 }
 
 func (hm *HostMap) unlockedMakePrimary(hostinfo *HostInfo) {
-	// Get the current primary, if it exists
-	oldHostinfo := hm.Hosts[hostinfo.vpnAddrs[0]]
-
-	// Every address in the hostinfo gets elevated to primary
-	for _, vpnAddr := range hostinfo.vpnAddrs {
-		//NOTE: It is possible that we leave a dangling hostinfo here but connection manager works on
-		// indexes so it should be fine.
-		hm.Hosts[vpnAddr] = hostinfo
-	}
-
-	// If we are already primary then we won't bother re-linking
-	if oldHostinfo == hostinfo {
+	// A hostinfo that is no longer in the hostmap must not be re-inserted here. Callers can race
+	// tunnel teardown, deciding to promote under the read lock and only taking the write lock
+	// after a delete fully unlinked the hostinfo (connection manager swapPrimary, AddRelay). Every
+	// live hostinfo is registered in Indexes by unlockedAddHostInfo, so this is a membership test.
+	if hm.Indexes[hostinfo.localIndexId] != hostinfo {
 		return
 	}
 
-	// Unlink this hostinfo
-	if hostinfo.prev != nil {
-		hostinfo.prev.next = hostinfo.next
-	}
-	if hostinfo.next != nil {
-		hostinfo.next.prev = hostinfo.prev
-	}
-
-	// If there wasn't a previous primary then clear out any links
-	if oldHostinfo == nil {
-		hostinfo.next = nil
-		hostinfo.prev = nil
-		return
+	// Move hostinfo to the front (primary) of each of its address lists. The lists are
+	// independent per address, so this can never leave a dangling entry the way promoting
+	// against a single shared chain could.
+	for _, addr := range hostinfo.vpnAddrs {
+		if hm.Hosts[addr] == hostinfo {
+			// Already primary for this address, the list is already in the right order
+			continue
+		}
+		list := removeHostInfo(hm.unlockedGetHostList(addr), hostinfo)
+		list = append([]*HostInfo{hostinfo}, list...)
+		hm.unlockedSetHostsForAddr(addr, list)
 	}
-
-	// Relink the hostinfo as primary
-	hostinfo.next = oldHostinfo
-	oldHostinfo.prev = hostinfo
-	hostinfo.prev = nil
 }
 
-func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) {
-	isLastHostinfo := hostinfo.next == nil && hostinfo.prev == nil
-
+// unlockedDeleteHostInfo removes hostinfo from every one of its address lists and from the index
+// maps. It returns true if this was the last hostinfo for all of its addresses (we no longer have
+// any tunnel to the peer), which the caller uses to decide whether to clear learned lighthouse
+// state and disestablish relays.
+func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) bool {
+	// Remove this hostinfo from each of its address lists. The lists are independent, so a
+	// sibling is never promoted to an address it does not own and no other list is touched.
+	final := true
 	for _, addr := range hostinfo.vpnAddrs {
-		if hm.Hosts[addr] != hostinfo {
-			continue
-		}
-		if hostinfo.next != nil {
-			// Promote the next hostinfo in the shared chain to primary for this address
-			hm.Hosts[addr] = hostinfo.next
-		} else {
-			delete(hm.Hosts, addr)
+		if list, ok := hm.moreHosts[addr]; ok {
+			list = removeHostInfo(list, hostinfo)
+			hm.unlockedSetHostsForAddr(addr, list)
+			if len(list) > 0 {
+				final = false
+			}
+		} else if existing, ok := hm.Hosts[addr]; ok {
+			if existing == hostinfo {
+				// Common case, the only hostinfo for this address. moreHosts has no entry to clean up.
+				delete(hm.Hosts, addr)
+			} else {
+				// We don't hold this address but another hostinfo does, we still have a tunnel to the peer
+				final = false
+			}
 		}
 	}
+
+	// Go maps never shrink their buckets, replace fully drained maps so a node that churned
+	// through a large peer count gives the memory back. Same idiom as the index maps below.
 	if len(hm.Hosts) == 0 {
 		hm.Hosts = map[netip.Addr]*HostInfo{}
 	}
-
-	// Splice this hostinfo out of the shared chain exactly once
-	if hostinfo.prev != nil {
-		hostinfo.prev.next = hostinfo.next
-	}
-	if hostinfo.next != nil {
-		hostinfo.next.prev = hostinfo.prev
+	if len(hm.moreHosts) == 0 {
+		hm.moreHosts = map[netip.Addr][]*HostInfo{}
 	}
 
-	hostinfo.next = nil
-	hostinfo.prev = nil
-
 	// The remote index uses index ids outside our control so lets make sure we are only removing
 	// the remote index pointer here if it points to the hostinfo we are deleting
 	hostinfo2, ok := hm.RemoteIndexes[hostinfo.remoteIndexId]
@@ -488,7 +528,7 @@ func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) {
 		)
 	}
 
-	if isLastHostinfo {
+	if final {
 		// I have lost connectivity to my peers. My relay tunnel is likely broken. Mark the next
 		// hops as 'Requested' so that new relay tunnels are created in the future.
 		hm.unlockedDisestablishVpnAddrRelayFor(hostinfo)
@@ -497,6 +537,8 @@ func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) {
 	for _, localRelayIdx := range hostinfo.relayState.CopyRelayForIdxs() {
 		delete(hm.Relays, localRelayIdx)
 	}
+
+	return final
 }
 
 func (hm *HostMap) QueryIndex(index uint32) *HostInfo {
@@ -540,19 +582,30 @@ func (hm *HostMap) QueryVpnAddrsRelayFor(targetIps []netip.Addr, relayHostIp net
 	hm.RLock()
 	defer hm.RUnlock()
 
+	// This runs per relayed packet, so check the primary with a single map probe and only consult
+	// moreHosts when the primary can't relay for us.
 	h, ok := hm.Hosts[relayHostIp]
 	if !ok {
 		return nil, nil, errors.New("unable to find host")
 	}
 
-	for h != nil {
-		for _, targetIp := range targetIps {
-			r, ok := h.relayState.QueryRelayForByIp(targetIp)
-			if ok && r.State == Established {
-				return h, r, nil
+	for _, targetIp := range targetIps {
+		r, ok := h.relayState.QueryRelayForByIp(targetIp)
+		if ok && r.State == Established {
+			return h, r, nil
+		}
+	}
+
+	if list, ok := hm.moreHosts[relayHostIp]; ok {
+		// list[0] is the primary we already checked
+		for _, h := range list[1:] {
+			for _, targetIp := range targetIps {
+				r, ok := h.relayState.QueryRelayForByIp(targetIp)
+				if ok && r.State == Established {
+					return h, r, nil
+				}
 			}
 		}
-		h = h.next
 	}
 
 	return nil, nil, errors.New("unable to find host with relay")
@@ -560,20 +613,14 @@ func (hm *HostMap) QueryVpnAddrsRelayFor(targetIps []netip.Addr, relayHostIp net
 
 func (hm *HostMap) unlockedDisestablishVpnAddrRelayFor(hi *HostInfo) {
 	for _, relayHostIp := range hi.relayState.CopyRelayIps() {
-		if h, ok := hm.Hosts[relayHostIp]; ok {
-			for h != nil {
-				h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished)
-				h = h.next
-			}
+		for _, h := range hm.unlockedGetHostList(relayHostIp) {
+			h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished)
 		}
 	}
 	for _, rs := range hi.relayState.CopyAllRelayFor() {
 		if rs.Type == ForwardingType {
-			if h, ok := hm.Hosts[rs.PeerAddr]; ok {
-				for h != nil {
-					h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished)
-					h = h.next
-				}
+			for _, h := range hm.unlockedGetHostList(rs.PeerAddr) {
+				h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished)
 			}
 		}
 	}
@@ -623,22 +670,27 @@ func (hm *HostMap) unlockedAddHostInfo(hostinfo *HostInfo, f *Interface) {
 }
 
 func (hm *HostMap) unlockedInnerAddHostInfo(vpnAddr netip.Addr, hostinfo *HostInfo, f *Interface) {
-	existing := hm.Hosts[vpnAddr]
-	hm.Hosts[vpnAddr] = hostinfo
+	existing, ok := hm.Hosts[vpnAddr]
+	if !ok {
+		// Common case, the first hostinfo for this address. moreHosts stays empty.
+		hm.Hosts[vpnAddr] = hostinfo
+		return
+	}
 
-	if existing != nil && existing != hostinfo {
-		hostinfo.next = existing
-		existing.prev = hostinfo
+	// The new hostinfo becomes the primary for this address. Remove any stale copy of it first so
+	// we never hold a duplicate, then prepend.
+	list, ok := hm.moreHosts[vpnAddr]
+	if !ok {
+		list = []*HostInfo{existing}
 	}
+	list = removeHostInfo(list, hostinfo)
+	list = append([]*HostInfo{hostinfo}, list...)
+	hm.unlockedSetHostsForAddr(vpnAddr, list)
 
-	i := 1
-	check := hostinfo
-	for check != nil {
-		if i > MaxHostInfosPerVpnIp {
-			hm.unlockedDeleteHostInfo(check)
-		}
-		check = check.next
-		i++
+	// Enforce the per-address cap by fully retiring the oldest hostinfo once we exceed it.
+	// Deleting it removes it from all of its addresses and the index maps, matching prior behavior.
+	if len(list) > MaxHostInfosPerVpnIp {
+		hm.unlockedDeleteHostInfo(list[len(list)-1])
 	}
 }
 

hostmap_test.go

@@ -2,6 +2,7 @@ package nebula
 
 import (
 	"net/netip"
+	"slices"
 	"testing"
 
 	"github.com/slackhq/nebula/config"
@@ -10,78 +11,84 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// chainIds returns the localIndexIds of the hostinfos holding addr, primary (index 0) first. It
+// also validates the Hosts/moreHosts sync contract on every call so a mutation that broke it
+// fails fast.
+func chainIds(t *testing.T, hm *HostMap, addr netip.Addr) []uint32 {
+	t.Helper()
+	assertHostMapInvariants(t, hm)
+	list := hm.unlockedGetHostList(addr)
+	ids := make([]uint32, len(list))
+	for i, h := range list {
+		ids[i] = h.localIndexId
+	}
+	return ids
+}
+
+// assertHostMapInvariants checks the Hosts/moreHosts contract: moreHosts only holds addresses
+// with 2 or more hostinfos, its first entry is always the primary in Hosts, lists never hold
+// duplicates, every hostinfo in a list owns the address and is registered in Indexes, and every
+// indexed hostinfo is reachable through each of its addresses.
+func assertHostMapInvariants(t *testing.T, hm *HostMap) {
+	t.Helper()
+	for addr, list := range hm.moreHosts {
+		require.GreaterOrEqualf(t, len(list), 2, "moreHosts[%s] must hold at least 2 hostinfos", addr)
+		require.Samef(t, hm.Hosts[addr], list[0], "moreHosts[%s][0] must match the primary in Hosts", addr)
+		seen := map[*HostInfo]bool{}
+		for _, h := range list {
+			require.NotNilf(t, h, "moreHosts[%s] must never hold a nil hostinfo", addr)
+			require.Falsef(t, seen[h], "moreHosts[%s] holds hostinfo %d twice", addr, h.localIndexId)
+			seen[h] = true
+			require.Samef(t, hm.Indexes[h.localIndexId], h, "moreHosts[%s] member %d is not registered in Indexes", addr, h.localIndexId)
+			require.Truef(t, slices.Contains(h.vpnAddrs, addr), "moreHosts[%s] member %d does not own the address", addr, h.localIndexId)
+		}
+	}
+	for addr, h := range hm.Hosts {
+		require.NotNilf(t, h, "Hosts[%s] must never be nil", addr)
+		require.Samef(t, hm.Indexes[h.localIndexId], h, "Hosts[%s] primary %d is not registered in Indexes", addr, h.localIndexId)
+		require.Truef(t, slices.Contains(h.vpnAddrs, addr), "Hosts[%s] primary (index %d) does not own the address", addr, h.localIndexId)
+	}
+	for idx, h := range hm.Indexes {
+		require.Equalf(t, idx, h.localIndexId, "Indexes[%d] holds hostinfo with localIndexId %d", idx, h.localIndexId)
+		for _, va := range h.vpnAddrs {
+			require.Truef(t, slices.Contains(hm.unlockedGetHostList(va), h), "indexed hostinfo %d is missing from the list for %s", idx, va)
+		}
+	}
+}
+
 func TestHostMap_MakePrimary(t *testing.T) {
 	l := test.NewLogger()
 	hm := newHostMap(l)
 
 	f := &Interface{}
+	a := netip.MustParseAddr("0.0.0.1")
 
-	h1 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 1}
-	h2 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 2}
-	h3 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 3}
-	h4 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 4}
+	h1 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1}
+	h2 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 2}
+	h3 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 3}
+	h4 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 4}
 
 	hm.unlockedAddHostInfo(h4, f)
 	hm.unlockedAddHostInfo(h3, f)
 	hm.unlockedAddHostInfo(h2, f)
 	hm.unlockedAddHostInfo(h1, f)
 
-	// Make sure we go h1 -> h2 -> h3 -> h4
-	prim := hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h1.localIndexId, prim.localIndexId)
-	assert.Equal(t, h2.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h1.localIndexId, h2.prev.localIndexId)
-	assert.Equal(t, h3.localIndexId, h2.next.localIndexId)
-	assert.Equal(t, h2.localIndexId, h3.prev.localIndexId)
-	assert.Equal(t, h4.localIndexId, h3.next.localIndexId)
-	assert.Equal(t, h3.localIndexId, h4.prev.localIndexId)
-	assert.Nil(t, h4.next)
-
-	// Swap h3/middle to primary
+	// Most-recently-added is primary: h1, h2, h3, h4
+	assert.Equal(t, []uint32{1, 2, 3, 4}, chainIds(t, hm, a))
+	assert.Equal(t, h1, hm.QueryVpnAddr(a))
+
+	// Swap the middle to primary: h3, h1, h2, h4
 	hm.MakePrimary(h3)
+	assert.Equal(t, []uint32{3, 1, 2, 4}, chainIds(t, hm, a))
+	assert.Equal(t, h3, hm.QueryVpnAddr(a))
 
-	// Make sure we go h3 -> h1 -> h2 -> h4
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h3.localIndexId, prim.localIndexId)
-	assert.Equal(t, h1.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h2.localIndexId, h1.next.localIndexId)
-	assert.Equal(t, h3.localIndexId, h1.prev.localIndexId)
-	assert.Equal(t, h4.localIndexId, h2.next.localIndexId)
-	assert.Equal(t, h1.localIndexId, h2.prev.localIndexId)
-	assert.Equal(t, h2.localIndexId, h4.prev.localIndexId)
-	assert.Nil(t, h4.next)
-
-	// Swap h4/tail to primary
+	// Swap the tail to primary: h4, h3, h1, h2
 	hm.MakePrimary(h4)
+	assert.Equal(t, []uint32{4, 3, 1, 2}, chainIds(t, hm, a))
 
-	// Make sure we go h4 -> h3 -> h1 -> h2
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h4.localIndexId, prim.localIndexId)
-	assert.Equal(t, h3.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h1.localIndexId, h3.next.localIndexId)
-	assert.Equal(t, h4.localIndexId, h3.prev.localIndexId)
-	assert.Equal(t, h2.localIndexId, h1.next.localIndexId)
-	assert.Equal(t, h3.localIndexId, h1.prev.localIndexId)
-	assert.Equal(t, h1.localIndexId, h2.prev.localIndexId)
-	assert.Nil(t, h2.next)
-
-	// Swap h4 again should be no-op
+	// Swapping the current primary again is a no-op
 	hm.MakePrimary(h4)
-
-	// Make sure we go h4 -> h3 -> h1 -> h2
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h4.localIndexId, prim.localIndexId)
-	assert.Equal(t, h3.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h1.localIndexId, h3.next.localIndexId)
-	assert.Equal(t, h4.localIndexId, h3.prev.localIndexId)
-	assert.Equal(t, h2.localIndexId, h1.next.localIndexId)
-	assert.Equal(t, h3.localIndexId, h1.prev.localIndexId)
-	assert.Equal(t, h1.localIndexId, h2.prev.localIndexId)
-	assert.Nil(t, h2.next)
+	assert.Equal(t, []uint32{4, 3, 1, 2}, chainIds(t, hm, a))
 }
 
 func TestHostMap_DeleteHostInfo(t *testing.T) {
@@ -89,13 +96,14 @@ func TestHostMap_DeleteHostInfo(t *testing.T) {
 	hm := newHostMap(l)
 
 	f := &Interface{}
+	a := netip.MustParseAddr("0.0.0.1")
 
-	h1 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 1}
-	h2 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 2}
-	h3 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 3}
-	h4 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 4}
-	h5 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 5}
-	h6 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 6}
+	h1 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1}
+	h2 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 2}
+	h3 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 3}
+	h4 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 4}
+	h5 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 5}
+	h6 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 6}
 
 	hm.unlockedAddHostInfo(h6, f)
 	hm.unlockedAddHostInfo(h5, f)
@@ -104,94 +112,110 @@ func TestHostMap_DeleteHostInfo(t *testing.T) {
 	hm.unlockedAddHostInfo(h2, f)
 	hm.unlockedAddHostInfo(h1, f)
 
-	// h6 should be deleted
-	assert.Nil(t, h6.next)
-	assert.Nil(t, h6.prev)
-	h := hm.QueryIndex(h6.localIndexId)
-	assert.Nil(t, h)
-
-	// Make sure we go h1 -> h2 -> h3 -> h4 -> h5
-	prim := hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h1.localIndexId, prim.localIndexId)
-	assert.Equal(t, h2.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h1.localIndexId, h2.prev.localIndexId)
-	assert.Equal(t, h3.localIndexId, h2.next.localIndexId)
-	assert.Equal(t, h2.localIndexId, h3.prev.localIndexId)
-	assert.Equal(t, h4.localIndexId, h3.next.localIndexId)
-	assert.Equal(t, h3.localIndexId, h4.prev.localIndexId)
-	assert.Equal(t, h5.localIndexId, h4.next.localIndexId)
-	assert.Equal(t, h4.localIndexId, h5.prev.localIndexId)
-	assert.Nil(t, h5.next)
-
-	// Delete primary
-	hm.DeleteHostInfo(h1)
-	assert.Nil(t, h1.prev)
-	assert.Nil(t, h1.next)
-
-	// Make sure we go h2 -> h3 -> h4 -> h5
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h2.localIndexId, prim.localIndexId)
-	assert.Equal(t, h3.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h3.localIndexId, h2.next.localIndexId)
-	assert.Equal(t, h2.localIndexId, h3.prev.localIndexId)
-	assert.Equal(t, h4.localIndexId, h3.next.localIndexId)
-	assert.Equal(t, h3.localIndexId, h4.prev.localIndexId)
-	assert.Equal(t, h5.localIndexId, h4.next.localIndexId)
-	assert.Equal(t, h4.localIndexId, h5.prev.localIndexId)
-	assert.Nil(t, h5.next)
-
-	// Delete in the middle
-	hm.DeleteHostInfo(h3)
-	assert.Nil(t, h3.prev)
-	assert.Nil(t, h3.next)
-
-	// Make sure we go h2 -> h4 -> h5
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h2.localIndexId, prim.localIndexId)
-	assert.Equal(t, h4.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h4.localIndexId, h2.next.localIndexId)
-	assert.Equal(t, h2.localIndexId, h4.prev.localIndexId)
-	assert.Equal(t, h5.localIndexId, h4.next.localIndexId)
-	assert.Equal(t, h4.localIndexId, h5.prev.localIndexId)
-	assert.Nil(t, h5.next)
-
-	// Delete the tail
-	hm.DeleteHostInfo(h5)
-	assert.Nil(t, h5.prev)
-	assert.Nil(t, h5.next)
-
-	// Make sure we go h2 -> h4
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h2.localIndexId, prim.localIndexId)
-	assert.Equal(t, h4.localIndexId, prim.next.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Equal(t, h4.localIndexId, h2.next.localIndexId)
-	assert.Equal(t, h2.localIndexId, h4.prev.localIndexId)
-	assert.Nil(t, h4.next)
-
-	// Delete the head
-	hm.DeleteHostInfo(h2)
-	assert.Nil(t, h2.prev)
-	assert.Nil(t, h2.next)
-
-	// Make sure we only have h4
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Equal(t, h4.localIndexId, prim.localIndexId)
-	assert.Nil(t, prim.prev)
-	assert.Nil(t, prim.next)
-	assert.Nil(t, h4.next)
-
-	// Delete the only item
-	hm.DeleteHostInfo(h4)
-	assert.Nil(t, h4.prev)
-	assert.Nil(t, h4.next)
-
-	// Make sure we have nil
-	prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1"))
-	assert.Nil(t, prim)
+	// h6 is evicted by the MaxHostInfosPerVpnIp cap; the rest are newest-first.
+	assert.Nil(t, hm.QueryIndex(h6.localIndexId))
+	assert.Equal(t, []uint32{1, 2, 3, 4, 5}, chainIds(t, hm, a))
+
+	// Delete primary; not final since siblings remain.
+	assert.False(t, hm.DeleteHostInfo(h1))
+	assert.Equal(t, []uint32{2, 3, 4, 5}, chainIds(t, hm, a))
+
+	// Deleting the same hostinfo again must not report final while siblings remain and must not
+	// disturb the list. The old chain code got this wrong: the first delete nil'd next/prev, so a
+	// second delete looked final and wiped lighthouse state out from under the live sibling.
+	assert.False(t, hm.DeleteHostInfo(h1))
+	assert.Equal(t, []uint32{2, 3, 4, 5}, chainIds(t, hm, a))
+
+	// Delete a middle node.
+	assert.False(t, hm.DeleteHostInfo(h3))
+	assert.Equal(t, []uint32{2, 4, 5}, chainIds(t, hm, a))
+
+	// Delete the tail.
+	assert.False(t, hm.DeleteHostInfo(h5))
+	assert.Equal(t, []uint32{2, 4}, chainIds(t, hm, a))
+
+	// Delete the head; h4 remains and becomes primary.
+	assert.False(t, hm.DeleteHostInfo(h2))
+	assert.Equal(t, []uint32{4}, chainIds(t, hm, a))
+	assert.Equal(t, h4, hm.QueryVpnAddr(a))
+
+	// Delete the only remaining item; final is true and the address is gone.
+	assert.True(t, hm.DeleteHostInfo(h4))
+	assert.Empty(t, chainIds(t, hm, a))
+	assert.Nil(t, hm.QueryVpnAddr(a))
+
+	// Deleting an already-gone hostinfo is still final; nothing holds the address anymore.
+	assert.True(t, hm.DeleteHostInfo(h4))
+	assert.Empty(t, chainIds(t, hm, a))
+}
+
+// TestHostMap_MakePrimary_DeletedHostInfo covers promoting a hostinfo that lost a race with
+// tunnel teardown: swapPrimary and AddRelay decide to promote while holding a stale pointer and
+// only take the write lock after a delete fully unlinked the hostinfo. MakePrimary must be a
+// no-op, not a resurrection that installs an unmanaged primary.
+func TestHostMap_MakePrimary_DeletedHostInfo(t *testing.T) {
+	l := test.NewLogger()
+	hm := newHostMap(l)
+	f := &Interface{}
+	a := netip.MustParseAddr("0.0.0.1")
+
+	h1 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1}
+	h2 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 2}
+	hm.unlockedAddHostInfo(h1, f)
+	hm.unlockedAddHostInfo(h2, f)
+
+	// h1 is fully deleted while another goroutine still holds a pointer to it.
+	assert.False(t, hm.DeleteHostInfo(h1))
+	assert.Equal(t, []uint32{2}, chainIds(t, hm, a))
+
+	// The stale promote must not bring it back.
+	hm.MakePrimary(h1)
+	assert.Equal(t, []uint32{2}, chainIds(t, hm, a))
+	assert.Equal(t, h2, hm.QueryVpnAddr(a))
+	assert.Nil(t, hm.QueryIndex(h1.localIndexId))
+}
+
+// TestHostMap_QueryVpnAddrsRelayFor_NonPrimary makes sure a relay established on an older
+// hostinfo is still found after a newer tunnel without relay state takes primary for the same
+// address. The lookup checks the primary first and falls back to the rest of the list.
+func TestHostMap_QueryVpnAddrsRelayFor_NonPrimary(t *testing.T) {
+	l := test.NewLogger()
+	hm := newHostMap(l)
+	f := &Interface{}
+	relayAddr := netip.MustParseAddr("0.0.0.9")
+	target := netip.MustParseAddr("0.0.0.1")
+
+	older := &HostInfo{
+		vpnAddrs:     []netip.Addr{relayAddr},
+		localIndexId: 1,
+		relayState: RelayState{
+			relayForByAddr: map[netip.Addr]*Relay{},
+			relayForByIdx:  map[uint32]*Relay{},
+		},
+	}
+	older.relayState.InsertRelay(target, 100, &Relay{Type: ForwardingType, State: Established, LocalIndex: 100, PeerAddr: target})
+	hm.unlockedAddHostInfo(older, f)
+
+	// The relay is found on the primary.
+	h, r, err := hm.QueryVpnAddrsRelayFor([]netip.Addr{target}, relayAddr)
+	require.NoError(t, err)
+	assert.Equal(t, older, h)
+	assert.Equal(t, uint32(100), r.LocalIndex)
+
+	// A re-handshake with no relay state takes primary; the established relay on the older
+	// hostinfo must still be found through the fallback.
+	newer := &HostInfo{vpnAddrs: []netip.Addr{relayAddr}, localIndexId: 2}
+	hm.unlockedAddHostInfo(newer, f)
+	assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, relayAddr))
+
+	h, r, err = hm.QueryVpnAddrsRelayFor([]netip.Addr{target}, relayAddr)
+	require.NoError(t, err)
+	assert.Equal(t, older, h)
+	assert.Equal(t, uint32(100), r.LocalIndex)
+
+	// No hostinfo at all is a plain miss.
+	_, _, err = hm.QueryVpnAddrsRelayFor([]netip.Addr{target}, netip.MustParseAddr("0.0.0.42"))
+	require.Error(t, err)
 }
 
 // TestHostMap_DeleteHostInfo_MultipleVpnAddrs exercises the case where a hostinfo carries more than one
@@ -216,32 +240,82 @@ func TestHostMap_DeleteHostInfo_MultipleVpnAddrs(t *testing.T) {
 	hm.unlockedAddHostInfo(other, f)
 	hm.unlockedAddHostInfo(head, f)
 
-	// head is primary for both addresses, other is next in the shared chain
-	assert.Equal(t, head.localIndexId, hm.QueryVpnAddr(a).localIndexId)
-	assert.Equal(t, head.localIndexId, hm.QueryVpnAddr(b).localIndexId)
-	assert.Equal(t, other.localIndexId, head.next.localIndexId)
-	assert.Equal(t, head.localIndexId, other.prev.localIndexId)
+	// head is primary for both addresses, other is next in each address's list.
+	assert.Equal(t, head, hm.QueryVpnAddr(a))
+	assert.Equal(t, head, hm.QueryVpnAddr(b))
+	assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, a))
+	assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, b))
 
 	// Delete the head. other is still live, so it must become primary for BOTH addresses.
-	hm.DeleteHostInfo(head)
-
-	// Pre-fix: QueryVpnAddr(b) came back nil here because the second address was deleted rather than
-	// promoted, leaving other unreachable at b.
-	require.NotNil(t, hm.QueryVpnAddr(a))
-	require.NotNil(t, hm.QueryVpnAddr(b))
-	assert.Equal(t, other.localIndexId, hm.QueryVpnAddr(a).localIndexId)
-	assert.Equal(t, other.localIndexId, hm.QueryVpnAddr(b).localIndexId)
-
-	// other is now the only hostinfo in the chain
-	assert.Nil(t, other.prev)
-	assert.Nil(t, other.next)
-
-	// head is fully detached
-	assert.Nil(t, head.prev)
-	assert.Nil(t, head.next)
+	assert.False(t, hm.DeleteHostInfo(head))
+	assert.Equal(t, other, hm.QueryVpnAddr(a))
+	assert.Equal(t, other, hm.QueryVpnAddr(b))
+	assert.Equal(t, []uint32{1}, chainIds(t, hm, a))
+	assert.Equal(t, []uint32{1}, chainIds(t, hm, b))
+
+	// head is fully removed from the index map.
 	assert.Nil(t, hm.QueryIndex(head.localIndexId))
 }
 
+// TestHostMap_DeleteHostInfo_DivergentVpnAddrs covers chained hostinfos for the same peer whose
+// vpnAddrs sets differ (a re-handshake cert added a second address). Deleting the superset node
+// must not promote a sibling to an address it does not own.
+func TestHostMap_DeleteHostInfo_DivergentVpnAddrs(t *testing.T) {
+	l := test.NewLogger()
+	hm := newHostMap(l)
+	f := &Interface{}
+	a := netip.MustParseAddr("0.0.0.1")
+	b := netip.MustParseAddr("0.0.0.2")
+
+	// sub owns only a; super (a newer handshake) owns a and b.
+	sub := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1}
+	super := &HostInfo{vpnAddrs: []netip.Addr{a, b}, localIndexId: 2}
+	hm.unlockedAddHostInfo(sub, f)
+	hm.unlockedAddHostInfo(super, f)
+
+	assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, a))
+	assert.Equal(t, []uint32{2}, chainIds(t, hm, b))
+
+	// Delete super: a promotes to sub (which owns it); b has no remaining owner and must be
+	// removed, not dangled at sub (which does not own b).
+	assert.False(t, hm.DeleteHostInfo(super))
+	assert.Equal(t, []uint32{1}, chainIds(t, hm, a))
+	assert.Empty(t, chainIds(t, hm, b))
+	assert.Equal(t, sub, hm.QueryVpnAddr(a))
+	assert.Nil(t, hm.QueryVpnAddr(b))
+	assert.Nil(t, hm.QueryIndex(super.localIndexId))
+
+	// Deleting sub cleans up fully.
+	assert.True(t, hm.DeleteHostInfo(sub))
+	assert.Nil(t, hm.QueryVpnAddr(a))
+	assertHostMapInvariants(t, hm)
+}
+
+// TestHostMap_AddDivergentOverlap covers a new hostinfo claiming addresses currently owned by two
+// DIFFERENT hostinfos. The old single shared next/prev chain overwrote a pointer and orphaned one
+// of them (in Indexes but unreachable via its address); independent per-address lists cannot.
+func TestHostMap_AddDivergentOverlap(t *testing.T) {
+	l := test.NewLogger()
+	hm := newHostMap(l)
+	f := &Interface{}
+	a := netip.MustParseAddr("0.0.0.1")
+	b := netip.MustParseAddr("0.0.0.2")
+
+	hiA := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1}
+	hiP := &HostInfo{vpnAddrs: []netip.Addr{b}, localIndexId: 2}
+	hm.unlockedAddHostInfo(hiA, f)
+	hm.unlockedAddHostInfo(hiP, f)
+
+	hiB := &HostInfo{vpnAddrs: []netip.Addr{a, b}, localIndexId: 3}
+	hm.unlockedAddHostInfo(hiB, f)
+
+	assert.Equal(t, []uint32{3, 1}, chainIds(t, hm, a))
+	assert.Equal(t, []uint32{3, 2}, chainIds(t, hm, b))
+	// hiA is still reachable via its address (not orphaned) and still indexed.
+	assert.Contains(t, chainIds(t, hm, a), hiA.localIndexId)
+	assert.NotNil(t, hm.QueryIndex(hiA.localIndexId))
+}
+
 // TestHostMap_MaxHostInfosPerVpnIp_MultipleVpnAddrs verifies the MaxHostInfosPerVpnIp overflow prune
 // (unlockedInnerAddHostInfo calls unlockedDeleteHostInfo on the oldest node once the chain is too long)
 // still behaves when hostinfos carry more than one vpnAddr. The pruned node is always the tail, so it is
@@ -267,32 +341,14 @@ func TestHostMap_MaxHostInfosPerVpnIp_MultipleVpnAddrs(t *testing.T) {
 
 	oldest := hostinfos[len(hostinfos)-1]
 
-	// The oldest hostinfo should have been pruned and fully detached
-	assert.Nil(t, oldest.next)
-	assert.Nil(t, oldest.prev)
+	// The oldest hostinfo was pruned from both lists and the index map.
 	assert.Nil(t, hm.QueryIndex(oldest.localIndexId))
 
-	// Both addresses resolve to the same head, and that head is one of the survivors (not the pruned one)
-	primA := hm.QueryVpnAddr(a)
-	primB := hm.QueryVpnAddr(b)
-	require.NotNil(t, primA)
-	require.NotNil(t, primB)
-	assert.Equal(t, primA.localIndexId, primB.localIndexId)
-	assert.NotEqual(t, oldest.localIndexId, primA.localIndexId)
-
-	// Walk the shared chain: exactly MaxHostInfosPerVpnIp survivors, no cycles, oldest absent
-	seen := map[uint32]struct{}{}
-	for h := primA; h != nil; h = h.next {
-		_, dup := seen[h.localIndexId]
-		require.False(t, dup, "cycle detected in hostinfo chain")
-		seen[h.localIndexId] = struct{}{}
-		if h.next != nil {
-			assert.Equal(t, h.localIndexId, h.next.prev.localIndexId, "prev pointer must mirror next")
-		}
-	}
-	assert.Len(t, seen, MaxHostInfosPerVpnIp)
-	_, prunedStillPresent := seen[oldest.localIndexId]
-	assert.False(t, prunedStillPresent)
+	// Both addresses hold exactly MaxHostInfosPerVpnIp survivors in the same order; oldest is absent.
+	require.Len(t, chainIds(t, hm, a), MaxHostInfosPerVpnIp)
+	assert.Equal(t, chainIds(t, hm, a), chainIds(t, hm, b), "both addresses must list the same survivors in the same order")
+	assert.NotContains(t, chainIds(t, hm, a), oldest.localIndexId)
+	assert.Equal(t, hm.QueryVpnAddr(a), hm.QueryVpnAddr(b))
 }
 
 func TestHostMap_reload(t *testing.T) {