From 7cb31b3a82e2cfa22083020dfdb208ed87dfd0e2 Mon Sep 17 00:00:00 2001 From: Paul Lorenz Date: Sat, 17 Oct 2026 21:58:26 -0400 Subject: [PATCH] Union a link destination's health across the controllers reporting it Health was one flag, overwritten by whichever controller reported last. A controller that lost sight of a destination could therefore mark it unhealthy while others still had it, and the router would back off dialing a destination that was up. - records health per reporting controller and reads the union, so a destination is up while any controller says it is - tracks when a controller the router is connected to last affirmed the destination, which the union cannot: the union is sticky over every report ever received, so a controller that said "up" and then went away holds it true forever - answers removal from the affirming controllers only, and the dial backoff from the union, because the two want different answers: treating a controller outage as "nobody says this is up" would switch every destination to the unhealthy backoff, whose interval starts at a minute and multiplies toward an hour, stalling re-dials of links that are fine - gives a brand new destination the full grace period before it can be considered abandoned --- router/link/link_events.go | 17 ++++++++++---- router/link/link_state.go | 48 +++++++++++++++++++++++++++++++------- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/router/link/link_events.go b/router/link/link_events.go index 40f39da92..980c47dba 100644 --- a/router/link/link_events.go +++ b/router/link/link_events.go @@ -120,12 +120,19 @@ func (self *linkDestUpdate) Handle(registry *linkRegistryImpl) { if dest == nil { dest = newLinkDest(self.id) registry.destinations[self.id] = dest - } else { - if !dest.healthy && self.healthy { - becameHealthy = true - } } - dest.update(self) + + wasHealthy := dest.healthy + dest.updateHealthFromCtrl(self.ctrlId, self.healthy) + dest.healthy = dest.isHealthy() + becameHealthy = !wasHealthy && dest.healthy + + if dest.healthy { + dest.lastAffirmedAt = time.Now() + dest.version.Store(self.version) + } else if wasHealthy { + dest.unhealthyAt = time.Now() + } // Cache the latest listener set so the registry's local dialer // rescan path (localDialersChangedEvent) can re-evaluate matches // without an upstream listener event. diff --git a/router/link/link_state.go b/router/link/link_state.go index 01e329c81..98463df5b 100644 --- a/router/link/link_state.go +++ b/router/link/link_state.go @@ -66,7 +66,13 @@ type linkDest struct { version concurrenz.AtomicValue[string] healthy bool unhealthyAt time.Time - linkMap map[string]*linkState + ctrlHealth map[string]bool // per-controller health reports + // lastAffirmedAt is the last time a controller the router was connected to said this destination was up. + // It drives removal of an abandoned destination, which healthy cannot: healthy is a sticky union over + // every report ever received, so a controller that said "up" and then went away holds it true forever. + // See affirmedBy for why removal asks a different question than the dial backoff does. + lastAffirmedAt time.Time + linkMap map[string]*linkState // listeners caches the most recent listener set advertised by this // destination router. Stored so we can re-evaluate matches when the @@ -75,16 +81,42 @@ type linkDest struct { listeners []*ctrl_pb.Listener } -func (self *linkDest) update(update *linkDestUpdate) { - if self.healthy && !update.healthy { - self.unhealthyAt = time.Now() +// updateHealthFromCtrl records a health report from a specific controller. +func (self *linkDest) updateHealthFromCtrl(ctrlId string, healthy bool) { + if self.ctrlHealth == nil { + self.ctrlHealth = map[string]bool{} } + self.ctrlHealth[ctrlId] = healthy +} - self.healthy = update.healthy - - if update.healthy { - self.version.Store(update.version) +// isHealthy returns true if any controller reports the destination as healthy. +func (self *linkDest) isHealthy() bool { + for _, healthy := range self.ctrlHealth { + if healthy { + return true + } } + return false +} + +// affirmedBy reports whether any of the given controllers says this destination is up. It exists so that +// deciding a destination has been abandoned can ignore reports from controllers the router is no longer +// connected to, which isHealthy deliberately does not. +// +// The two want different answers. The dial backoff reads isHealthy, and treating a controller outage as +// "nobody says this is up" would switch every destination to the unhealthy backoff, whose retry interval +// starts at a minute and multiplies by ten toward an hour; a controller restart would then stall re-dialing +// links that are perfectly fine. Removal has no such problem, because the caller only acts once a destination +// has gone unaffirmed for far longer than any outage lasts. +func (self *linkDest) affirmedBy(ctrlIds map[string]struct{}) bool { + for ctrlId, healthy := range self.ctrlHealth { + if healthy { + if _, ok := ctrlIds[ctrlId]; ok { + return true + } + } + } + return false } type linkFault struct {