From c80969ab2bff001c81a279840103dc5a69567318 Mon Sep 17 00:00:00 2001 From: Moses Narrow <36607567+0pcom@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:13:27 -0500 Subject: [PATCH] fix(router): self-heal failed-path exclusion for dead-edge route setup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a route-group setup handshake fails to complete within handshakeAwaitTimeout (even with #4057 retransmit — e.g. the path funnels through a dead/non-forwarding intermediary), hold that route's intermediate hops on a per-destination suspect list for a bounded TTL. The next dial (an app reconnect after a wedge) excludes the suspect hops up front via DialRoutes' ExcludeIntermediatePKs, so it picks a DIFFERENT route instead of re-hammering the dead path and wedging the app in "starting". Self-healing: the suspect entry expires after the TTL (never a permanent blacklist), so a recovered hop is reused. Configurable via routing FailedHopExclusionTTL (0 = default; feature on). Bounded grace: a single transient failure is handled by #4057 retransmit; only a full-window handshake failure marks the path suspect. New: pkg/router/suspect_hops.go (+ test). Addresses the dead-edge wedge observed live where all candidate routes shared one dead second-to-last hop. --- pkg/router/router.go | 23 ++++ pkg/router/router_dial.go | 55 ++++++++++ pkg/router/suspect_hops.go | 120 +++++++++++++++++++++ pkg/router/suspect_hops_test.go | 180 ++++++++++++++++++++++++++++++++ 4 files changed, 378 insertions(+) create mode 100644 pkg/router/suspect_hops.go create mode 100644 pkg/router/suspect_hops_test.go diff --git a/pkg/router/router.go b/pkg/router/router.go index bad68d528a..84d49a2e1b 100644 --- a/pkg/router/router.go +++ b/pkg/router/router.go @@ -181,6 +181,22 @@ type Config struct { // query deliverer is configured (SetTransportQueryDeliverer); see // rsn_oracle_routes.go. TPD is still used for routes with >=2 intermediates. EnableRSNOracleRoutes bool + + // FailedHopExclusionTTL configures the self-heal failed-path exclusion + // (dead-edge route-setup fix, the route-level-failover follow-up to the + // #4057 handshake retransmit). When a route's setup handshake fails to + // complete within handshakeAwaitTimeout, its intermediate hops are held on + // a per-destination suspect list for this duration so the NEXT dial (an + // app-reconnect after a wedge) excludes them up front and picks a different + // route instead of re-selecting the same dead hop. The exclusion expires + // after the TTL — a hop that was merely busy/restarting is retried once it + // recovers (self-heal); it is never permanently blacklisted. + // + // 0 (the default) → use defaultSuspectHopTTL (feature ON). + // <0 → disable cross-dial persistence (bounded grace still + // applies within a single DialRoutes call). + // >0 → custom TTL. + FailedHopExclusionTTL time.Duration } // SetDefaults sets default values for certain empty values. @@ -552,6 +568,12 @@ type router struct { // Set via SetDstTransportOracle. dstTpOracle DstTransportOracle dstTpOracleMu sync.Mutex + // suspectHops is the self-heal failed-path exclusion cache: intermediates + // whose route-group setup handshake recently failed, keyed by destination, + // each expiring after Config.FailedHopExclusionTTL. Seeded into a dial's + // opts.ExcludeIntermediatePKs at DialRoutes entry and written on a + // handshake/data-plane setup failure. See suspect_hops.go. + suspectHops *suspectHopCache } // DstTransportOracle fetches a destination visor's OWN transport list @@ -659,6 +681,7 @@ func New(dmsgC *dmsg.Client, config *Config, routeSetupHooks []RouteSetupHook) ( trustedVisors: trustedVisors, routeSetupHooks: routeSetupHooks, tpdCache: newTPDSnapshotCache(), + suspectHops: newSuspectHopCache(resolveFailedHopExclusionTTL(config.FailedHopExclusionTTL)), } go r.rulesGCLoop() diff --git a/pkg/router/router_dial.go b/pkg/router/router_dial.go index 0132a165d1..012feb8cd8 100644 --- a/pkg/router/router_dial.go +++ b/pkg/router/router_dial.go @@ -269,6 +269,25 @@ func (r *router) DialRoutes( // DOES accept. See WithForceLegacyRouteSetup. escalateToLegacy := false + // Self-heal failed-path exclusion: seed this dial's exclude set with any + // intermediates that recently failed route-group setup to this destination + // (see the handshake/data-plane failure branch below + suspect_hops.go). + // This is what carries the exclusion across the app's reconnect loop: each + // reconnect enters DialRoutes with a FRESH opts, so without this the hard-won + // knowledge that a hop is dead is lost and the finder — ranking by latency, + // not liveness — hands back the same dead-hop candidate set, wedging the app + // in "starting". Bounded grace: an entry only lands in the cache after a FULL + // handshakeAwaitTimeout failure (never a first transient loss, which #4057's + // retransmit recovers), and expires after Config.FailedHopExclusionTTL so a + // recovered hop is retried. No-op when the cache is disabled or empty. + for _, pk := range r.suspectHops.suspects(rPK) { + opts = appendExcludeIntermediate(opts, pk) + } + if n := len(opts.ExcludeIntermediatePKs); n > 0 { + log.WithField("excluded_intermediates", n). + Debug("Seeded dial with recently-failed intermediate exclusions (self-heal)") + } + for attempt := 1; attempt <= maxRetries; attempt++ { var forwardPath, reversePath []routing.Hop var err error @@ -465,6 +484,16 @@ func (r *router) DialRoutes( // stale-TPD error still fail fast — retrying the same direct path // to a down peer would only burn the budget. deadInter := intermediatePKsOfPath(forwardPath, lPK, rPK) + // Persist the dead intermediates across dials (self-heal exclusion): + // the handshake await ran its FULL window (#4057 retransmitted the + // whole setup handshake with no reciprocal) before saveRouteGroupRules + // returned, so this is a confirmed dead / non-forwarding path, not a + // transient loss. Marking here means the app's reconnect loop — which + // re-enters DialRoutes with fresh opts — starts by excluding this hop + // instead of re-selecting it, which is what breaks the perpetual + // "starting" wedge. Expires after Config.FailedHopExclusionTTL so a + // recovered hop rejoins the candidate set. + r.suspectHops.mark(rPK, deadInter) // A cascade-installed route that fails its handshake is ALSO the // fingerprint of a destination that does not trust cascade route // setup (rules ACKed over transports, but the destination — running @@ -948,6 +977,26 @@ fetchRoutesAgain: fwdNum := findRouteNum(opts.EffectiveMuxRoutes(true)) revNum := findRouteNum(opts.EffectiveMuxRoutes(false)) + // Widen the requested candidate count when intermediates are excluded + // (disjoint-mux, or — the dead-edge case — the failed-hop self-heal + // exclusion). The finder defaults to 3 candidates ranked by latency, and + // when a dead-but-low-latency intermediary sits on ALL of the top-3 paths + // (the funnel that wedges the app: every candidate shares the same + // second-to-last hop), the post-filter drops every one and the disjoint + // pick has nothing left — even though an alternate path avoiding the dead + // hop exists deeper in the finder's BFS order. Asking for more candidates + // surfaces those alternates so the exclusion can actually fail over. + // Bounded by the finder's own ceiling; only widens (never shrinks below the + // mux-driven count). + if len(opts.ExcludeIntermediatePKs) > 0 { + if fwdNum < widenedRouteCandidates { + fwdNum = widenedRouteCandidates + } + if revNum < widenedRouteCandidates { + revNum = widenedRouteCandidates + } + } + var paths map[routing.PathEdges][][]routing.Hop if fwdMinHops == revMinHops { // Common path: single query covers both directions. One count @@ -1355,6 +1404,12 @@ func pathLatencyScore(path []routing.Hop, latencyFor func(uuid.UUID) float64, ty const ( baseRouteCandidates = 3 muxRouteHeadroom = 2 + // widenedRouteCandidates is the per-edge count requested from the route- + // finder once any intermediate is excluded, so alternates that avoid the + // excluded hop(s) are surfaced past the default top-3 (which can all funnel + // through a single dead intermediary). Matches the finder's routesCeiling — + // the most it will ever return — so nothing usable is left unseen. + widenedRouteCandidates uint16 = 20 ) // findRouteNum returns the per-edge route count to request from the diff --git a/pkg/router/suspect_hops.go b/pkg/router/suspect_hops.go new file mode 100644 index 0000000000..03b695a136 --- /dev/null +++ b/pkg/router/suspect_hops.go @@ -0,0 +1,120 @@ +// Package router pkg/router/suspect_hops.go c2-net-routing +package router + +import ( + "sync" + "time" + + "github.com/skycoin/skywire/pkg/cipher" +) + +// defaultSuspectHopTTL is how long an intermediate visor stays on the +// per-destination failed-hop exclusion list after a route through it failed to +// complete its setup handshake within handshakeAwaitTimeout. +// +// This is the "self-heal" half of the dead-edge route-setup fix (#80 route- +// level failover follow-up to #4057). #4057's handshake retransmit recovers a +// single lost setup packet on an otherwise-live path; it does NOT help when the +// picked path funnels through an intermediary that ACKs route-setup but does not +// forward data — every retransmit is futile, the whole handshake window is +// burned, and the app's reconnect loop re-runs the route-finder which (ranking +// by latency, not liveness) hands back the SAME dead-hop candidate set. Marking +// the failed intermediary suspect for a bounded window lets the very next dial +// exclude it up front and pick a different route. +// +// The TTL bounds the blacklisting so a hop that was merely busy/restarting is +// retried once it recovers (self-heal) — the exclusion is never permanent. 90s +// comfortably outlives the app reconnect cadence (so the reconnect after a +// wedge avoids the dead hop) while being short enough that a recovered hop +// rejoins the candidate set quickly. +var defaultSuspectHopTTL = 90 * time.Second + +// resolveFailedHopExclusionTTL maps a Config.FailedHopExclusionTTL value to the +// cache TTL: 0 → the built-in default (feature on), <0 → disabled, >0 → as-is. +func resolveFailedHopExclusionTTL(cfg time.Duration) time.Duration { + if cfg == 0 { + return defaultSuspectHopTTL + } + return cfg +} + +// suspectHopCache is a TTL set of intermediate-visor PKs that recently failed +// route-group setup, keyed by destination. It is consulted at the start of a +// dial to pre-seed opts.ExcludeIntermediatePKs (so a fresh reconnect avoids a +// known-dead hop) and written when a route's setup handshake fails to complete +// within handshakeAwaitTimeout. +// +// All methods are safe to call on a nil receiver (they no-op / return nil), so +// router values constructed without a cache — e.g. in narrow unit tests — keep +// their pre-existing behavior. +type suspectHopCache struct { + mu sync.Mutex + ttl time.Duration + m map[cipher.PubKey]map[cipher.PubKey]time.Time // dst -> intermediate -> expiry +} + +// newSuspectHopCache builds a cache with the given TTL. A non-positive TTL +// disables the cross-dial persistence entirely (mark/suspects become no-ops): +// bounded grace still applies WITHIN a single DialRoutes call via the local +// opts.ExcludeIntermediatePKs accumulation, but nothing survives to the next +// dial — the pre-fix behavior. +func newSuspectHopCache(ttl time.Duration) *suspectHopCache { + return &suspectHopCache{ + ttl: ttl, + m: make(map[cipher.PubKey]map[cipher.PubKey]time.Time), + } +} + +// mark records each intermediate in inter as suspect for dst, expiring after +// the cache TTL. No-op when disabled (nil / non-positive TTL) or inter empty. +func (c *suspectHopCache) mark(dst cipher.PubKey, inter []cipher.PubKey) { + if c == nil || c.ttl <= 0 || len(inter) == 0 { + return + } + exp := time.Now().Add(c.ttl) + c.mu.Lock() + defer c.mu.Unlock() + set := c.m[dst] + if set == nil { + set = make(map[cipher.PubKey]time.Time, len(inter)) + c.m[dst] = set + } + for _, pk := range inter { + if pk.Null() { + continue + } + set[pk] = exp + } +} + +// suspects returns the non-expired suspect intermediates for dst, pruning +// expired entries as it goes (the self-heal step — a hop past its TTL rejoins +// the candidate set). Returns nil when disabled or nothing is currently +// suspect. +func (c *suspectHopCache) suspects(dst cipher.PubKey) []cipher.PubKey { + if c == nil || c.ttl <= 0 { + return nil + } + now := time.Now() + c.mu.Lock() + defer c.mu.Unlock() + set := c.m[dst] + if len(set) == 0 { + return nil + } + out := make([]cipher.PubKey, 0, len(set)) + for pk, exp := range set { + if now.After(exp) { + delete(set, pk) + continue + } + out = append(out, pk) + } + if len(set) == 0 { + delete(c.m, dst) + } + if len(out) == 0 { + return nil + } + return out +} diff --git a/pkg/router/suspect_hops_test.go b/pkg/router/suspect_hops_test.go new file mode 100644 index 0000000000..c038300b3f --- /dev/null +++ b/pkg/router/suspect_hops_test.go @@ -0,0 +1,180 @@ +// suspect_hops_test.go — unit tests for the self-heal failed-path exclusion +// (dead-edge route-setup fix, the route-level-failover follow-up to #4057). +// Covers the suspect-hop TTL cache in isolation plus the fetchBestRoutes +// candidate-count widening that lets the exclusion actually fail over when the +// finder's default top-3 all funnel through one dead intermediary. +package router + +import ( + "context" + "testing" + "time" + + "github.com/skycoin/skywire/pkg/cipher" + "github.com/skycoin/skywire/pkg/logging" + "github.com/skycoin/skywire/pkg/rfclient" + "github.com/skycoin/skywire/pkg/routing" +) + +func TestResolveFailedHopExclusionTTL(t *testing.T) { + if got := resolveFailedHopExclusionTTL(0); got != defaultSuspectHopTTL { + t.Errorf("0 → %v, want default %v", got, defaultSuspectHopTTL) + } + if got := resolveFailedHopExclusionTTL(-1); got != -1 { + t.Errorf("negative → %v, want -1 (disabled passthrough)", got) + } + if got := resolveFailedHopExclusionTTL(42 * time.Second); got != 42*time.Second { + t.Errorf("positive → %v, want 42s", got) + } +} + +func TestSuspectHopCache_MarkAndSuspects(t *testing.T) { + dst := mustPK(t) + a, b := mustPK(t), mustPK(t) + c := newSuspectHopCache(time.Minute) + c.mark(dst, []cipher.PubKey{a, b}) + + got := c.suspects(dst) + if len(got) != 2 { + t.Fatalf("suspects=%d, want 2", len(got)) + } + set := map[cipher.PubKey]struct{}{got[0]: {}, got[1]: {}} + if _, ok := set[a]; !ok { + t.Errorf("suspect a missing") + } + if _, ok := set[b]; !ok { + t.Errorf("suspect b missing") + } +} + +func TestSuspectHopCache_PerDestinationIsolation(t *testing.T) { + dstA, dstB := mustPK(t), mustPK(t) + hop := mustPK(t) + c := newSuspectHopCache(time.Minute) + c.mark(dstA, []cipher.PubKey{hop}) + + if got := c.suspects(dstB); len(got) != 0 { + t.Errorf("dstB leaked %d suspects from dstA", len(got)) + } + if got := c.suspects(dstA); len(got) != 1 { + t.Errorf("dstA suspects=%d, want 1", len(got)) + } +} + +func TestSuspectHopCache_Disabled(t *testing.T) { + dst := mustPK(t) + hop := mustPK(t) + for _, ttl := range []time.Duration{-time.Second, -1} { + c := newSuspectHopCache(ttl) + c.mark(dst, []cipher.PubKey{hop}) + if got := c.suspects(dst); got != nil { + t.Errorf("ttl=%v: suspects=%v, want nil (disabled)", ttl, got) + } + } +} + +func TestSuspectHopCache_Expiry(t *testing.T) { + dst := mustPK(t) + a, b := mustPK(t), mustPK(t) + c := newSuspectHopCache(time.Minute) + c.mark(dst, []cipher.PubKey{a, b}) + + // Force a to be expired; b stays live. suspects must prune a and return b. + c.mu.Lock() + c.m[dst][a] = time.Now().Add(-time.Second) + c.mu.Unlock() + + got := c.suspects(dst) + if len(got) != 1 || got[0] != b { + t.Fatalf("after expiry: got=%v, want [b]", got) + } + + // Expiring the last entry should drop the destination key entirely. + c.mu.Lock() + c.m[dst][b] = time.Now().Add(-time.Second) + c.mu.Unlock() + if got := c.suspects(dst); got != nil { + t.Errorf("after full expiry: got=%v, want nil", got) + } + c.mu.Lock() + _, present := c.m[dst] + c.mu.Unlock() + if present { + t.Errorf("destination key not pruned after full expiry") + } +} + +func TestSuspectHopCache_SkipsNull(t *testing.T) { + dst := mustPK(t) + c := newSuspectHopCache(time.Minute) + c.mark(dst, []cipher.PubKey{{}}) // null PK only + if got := c.suspects(dst); len(got) != 0 { + t.Errorf("null PK stored: got=%v", got) + } +} + +func TestSuspectHopCache_NilReceiver(t *testing.T) { + var c *suspectHopCache + // Must not panic on a nil cache (routers built without one in narrow tests). + c.mark(mustPK(t), []cipher.PubKey{mustPK(t)}) + if got := c.suspects(mustPK(t)); got != nil { + t.Errorf("nil cache suspects=%v, want nil", got) + } +} + +// recordingFinder is a minimal rfclient.Client that records the NumRoutes it +// was last asked for and returns a fixed candidate set. +type recordingFinder struct { + lastNumRoutes uint16 + paths map[routing.PathEdges][][]routing.Hop +} + +func (f *recordingFinder) FindRoutes(_ context.Context, _ []routing.PathEdges, opts *rfclient.RouteOptions) (map[routing.PathEdges][][]routing.Hop, error) { + if opts != nil { + f.lastNumRoutes = opts.NumRoutes + } + return f.paths, nil +} + +// TestFetchBestRoutes_WidensCandidatesWhenExcluding verifies the Part-A half of +// the fix: with an intermediate excluded, fetchBestRoutes asks the finder for a +// wider candidate set (so an alternate avoiding the dead hop can be surfaced), +// whereas a plain single-route dial keeps the finder's default. +func TestFetchBestRoutes_WidensCandidatesWhenExcluding(t *testing.T) { + src, dst := mustPK(t), mustPK(t) + inter := mustPK(t) + + forward := routing.PathEdges{src, dst} + backward := routing.PathEdges{dst, src} + finder := &recordingFinder{ + paths: map[routing.PathEdges][][]routing.Hop{ + forward: {{hop(src, inter), hop(inter, dst)}}, + backward: {{hop(dst, inter), hop(inter, src)}}, + }, + } + r := &router{ + conf: &Config{RouteFinder: finder, MaxHops: 5}, + logger: logging.MustGetLogger("test"), + } + + // No exclusions: single-route dial → finder default (0 = use its own). + // Retries>0 is required for fetchBestRoutes to consume the finder result + // (retries==0 short-circuits to the local-calc fallback). + if _, _, err := r.fetchBestRoutes(context.Background(), nil, src, dst, &DialOptions{Retries: 3}, 2); err != nil { + t.Fatalf("fetch (no exclude) err: %v", err) + } + if finder.lastNumRoutes != 0 { + t.Errorf("no-exclude NumRoutes=%d, want 0 (finder default)", finder.lastNumRoutes) + } + + // Exclude an unrelated PK (so the candidate through `inter` survives the + // filter): fetch must widen the requested candidate count. + excludeOnly := mustPK(t) + opts := &DialOptions{Retries: 3, ExcludeIntermediatePKs: []cipher.PubKey{excludeOnly}} + if _, _, err := r.fetchBestRoutes(context.Background(), nil, src, dst, opts, 2); err != nil { + t.Fatalf("fetch (with exclude) err: %v", err) + } + if finder.lastNumRoutes != widenedRouteCandidates { + t.Errorf("with-exclude NumRoutes=%d, want %d (widened)", finder.lastNumRoutes, widenedRouteCandidates) + } +}