fix(hop): drop marked-failed nodes before priority selection

http.failCodes (and probe) marking had no effect when the failing node
had a strictly higher matcher priority: the priority short-circuit
returned the top node directly, and FailFilter no-ops on a single-node
tier (len(vs) <= 1 guard), so the marked node was always re-selected
and failover never happened.

Pre-filter nodes marked failed (same Count >= maxFails && within
failTimeout criteria as FailFilter) out of the candidate pool before
priority selection. Single-node hops keep their lone node (safety net),
and an all-failed pool returns nil as before.

Wired maxFails/failTimeout from the hop selector config into the hop
via FailFilterSettingsOption.
This commit is contained in:
ginuerzh
2026-08-18 23:15:59 +08:00
parent fc5fc052e4
commit 64ea5184f8
3 changed files with 140 additions and 0 deletions
+12
View File
@@ -15,6 +15,7 @@ import (
selector_parser "github.com/go-gost/x/config/parsing/selector" selector_parser "github.com/go-gost/x/config/parsing/selector"
xhop "github.com/go-gost/x/hop" xhop "github.com/go-gost/x/hop"
hop_plugin "github.com/go-gost/x/hop/plugin" hop_plugin "github.com/go-gost/x/hop/plugin"
xs "github.com/go-gost/x/selector"
"github.com/go-gost/x/internal/loader" "github.com/go-gost/x/internal/loader"
"github.com/go-gost/x/internal/plugin" "github.com/go-gost/x/internal/plugin"
"github.com/go-gost/x/metadata" "github.com/go-gost/x/metadata"
@@ -158,10 +159,21 @@ func ParseHop(cfg *config.HopConfig, log logger.Logger) (hop.Hop, error) {
sel = selector_parser.DefaultNodeSelector() sel = selector_parser.DefaultNodeSelector()
} }
maxFails, timeout := xs.DefaultMaxFails, xs.DefaultFailTimeout
if cfg.Selector != nil {
if cfg.Selector.MaxFails > 0 {
maxFails = cfg.Selector.MaxFails
}
if cfg.Selector.FailTimeout > 0 {
timeout = cfg.Selector.FailTimeout
}
}
opts := []xhop.Option{ opts := []xhop.Option{
xhop.NameOption(cfg.Name), xhop.NameOption(cfg.Name),
xhop.NodeOption(nodes...), xhop.NodeOption(nodes...),
xhop.SelectorOption(sel), xhop.SelectorOption(sel),
xhop.FailFilterSettingsOption(maxFails, timeout),
xhop.BypassOption(xbypass.BypassGroup(bypass_parser.List(cfg.Bypass, cfg.Bypasses...)...)), xhop.BypassOption(xbypass.BypassGroup(bypass_parser.List(cfg.Bypass, cfg.Bypasses...)...)),
xhop.ReloadPeriodOption(cfg.Reload), xhop.ReloadPeriodOption(cfg.Reload),
xhop.LoggerOption(log.WithFields(map[string]any{ xhop.LoggerOption(log.WithFields(map[string]any{
+59
View File
@@ -22,6 +22,7 @@ import (
"github.com/go-gost/core/routing" "github.com/go-gost/core/routing"
"github.com/go-gost/core/selector" "github.com/go-gost/core/selector"
"github.com/go-gost/x/config" "github.com/go-gost/x/config"
xs "github.com/go-gost/x/selector"
mdutil "github.com/go-gost/x/metadata/util" mdutil "github.com/go-gost/x/metadata/util"
node_parser "github.com/go-gost/x/config/parsing/node" node_parser "github.com/go-gost/x/config/parsing/node"
"github.com/go-gost/x/internal/loader" "github.com/go-gost/x/internal/loader"
@@ -33,6 +34,12 @@ type options struct {
nodes []*chain.Node nodes []*chain.Node
bypass bypass.Bypass bypass bypass.Bypass
selector selector.Selector[*chain.Node] selector selector.Selector[*chain.Node]
// failMaxFails / failTimeout mirror the FailFilter config used by the
// selector. The pre-selection dead-node filter (Stage 2.5) consults them
// to decide whether a node is currently marked failed (FailFilter itself
// no-ops on a single node, so the marker is checked directly here).
failMaxFails int
failTimeout time.Duration
fileLoader loader.Loader fileLoader loader.Loader
redisLoader loader.Loader redisLoader loader.Loader
httpLoader loader.Loader httpLoader loader.Loader
@@ -71,6 +78,16 @@ func SelectorOption(s selector.Selector[*chain.Node]) Option {
} }
} }
// FailFilterSettingsOption sets the maxFails/failTimeout used by the hop's
// FailFilter. The pre-selection dead-node filter consults these to decide
// whether a node is currently marked failed.
func FailFilterSettingsOption(maxFails int, timeout time.Duration) Option {
return func(o *options) {
o.failMaxFails = maxFails
o.failTimeout = timeout
}
}
// ReloadPeriodOption sets the interval for periodic reloading of node lists. // ReloadPeriodOption sets the interval for periodic reloading of node lists.
func ReloadPeriodOption(period time.Duration) Option { func ReloadPeriodOption(period time.Duration) Option {
return func(opts *options) { return func(opts *options) {
@@ -243,6 +260,30 @@ func (p *chainHop) Select(ctx context.Context, opts ...hop.SelectOption) *chain.
nodes = append(nodes, node) nodes = append(nodes, node)
} }
// Stage 2.5: drop nodes currently marked failed (http.failCodes match,
// probe failure, etc.). A failed node must not win via the priority
// shortcut (Stage 3) or a lone-node tier, neither of which consults the
// marker — otherwise failCodes could never trigger failover. This mirrors
// FailFilter's per-node criteria; FailFilter still runs in Stage 4 but
// finds nothing left to exclude.
if len(nodes) > 0 {
alive := nodes[:0]
for _, n := range nodes {
if !nodeIsFailed(n, p.options.failMaxFails, p.options.failTimeout) {
alive = append(alive, n)
}
}
if len(alive) == 0 {
if len(nodes) == 1 {
alive = nodes // safety net: keep the only node (matches FailFilter's len(vs) <= 1 guard and the single-node early return below)
} else {
return nil
}
}
nodes = alive
}
if len(nodes) == 0 { if len(nodes) == 0 {
return nil return nil
} }
@@ -327,6 +368,24 @@ func isAllBackup(nodes []*chain.Node) bool {
return len(nodes) > 0 return len(nodes) > 0
} }
// nodeIsFailed reports whether node is currently marked failed per the same
// criteria FailFilter uses: failure count has reached maxFails and the last
// failure is still within failTimeout. FailFilter itself no-ops on a single
// node, so the priority short-circuit checks the marker directly here.
func nodeIsFailed(node *chain.Node, maxFails int, timeout time.Duration) bool {
marker := node.Marker()
if marker == nil {
return false
}
if maxFails <= 0 {
maxFails = xs.DefaultMaxFails
}
if timeout <= 0 {
timeout = xs.DefaultFailTimeout
}
return !(marker.Count() < int64(maxFails) || time.Since(marker.Time()) >= timeout)
}
func (p *chainHop) isEligible(node *chain.Node, opts *hop.SelectOptions) bool { func (p *chainHop) isEligible(node *chain.Node, opts *hop.SelectOptions) bool {
if node == nil { if node == nil {
return false return false
+69
View File
@@ -19,6 +19,7 @@ import (
"github.com/go-gost/core/routing" "github.com/go-gost/core/routing"
xmd "github.com/go-gost/x/metadata" xmd "github.com/go-gost/x/metadata"
xlogger "github.com/go-gost/x/logger" xlogger "github.com/go-gost/x/logger"
xselector "github.com/go-gost/x/selector"
"github.com/go-gost/x/registry" "github.com/go-gost/x/registry"
) )
@@ -548,6 +549,74 @@ func TestSelect_EqualPriorityDoesNotShortcut(t *testing.T) {
} }
} }
// TestSelect_FailCodesHighPriorityNode reproduces the reverse-proxy failCodes
// bug: a node with a stricter (longer) matcher wins by auto-computed priority,
// so the priority shortcut selected it directly and bypassed the FailFilter —
// a node marked failed by http.failCodes could never be excluded, so failover
// never happened. After the fix, marking the high-priority node makes Select
// fall through to the healthy lower-priority node.
func TestSelect_FailCodesHighPriorityNode(t *testing.T) {
// highPriority: stricter matcher -> longer rule -> higher auto priority.
highPriority := chain.NewNode("high", "127.0.0.1:8080",
chain.MatcherNodeOption(&testMatcher{match: true}),
chain.PriorityNodeOption(100), // simulates len(longer-rule)
)
// healthy: less specific matcher -> lower priority.
healthy := chain.NewNode("healthy", "127.0.0.1:9090",
chain.MatcherNodeOption(&testMatcher{match: true}),
chain.PriorityNodeOption(10),
)
sel := xselector.NewSelector[*chain.Node](
xselector.FIFOStrategy[*chain.Node](),
xselector.FailFilter[*chain.Node](1, 60*time.Second),
)
h := newTestHop(NodeOption(highPriority, healthy), SelectorOption(sel),
FailFilterSettingsOption(1, 60*time.Second))
defer h.Close()
// Before marking: high-priority node wins via the shortcut.
node := h.Select(context.Background())
if node == nil {
t.Fatal("expected node, got nil")
}
if node.Name != "high" {
t.Fatalf("expected 'high' (higher priority) before marking, got %q", node.Name)
}
// Simulate http.failCodes marking the high-priority node failed (429/5xx).
highPriority.Marker().Mark()
// After marking: Select must fall through to the healthy node.
node = h.Select(context.Background())
if node == nil {
t.Fatal("expected a node after marking, got nil")
}
if node.Name != "healthy" {
t.Fatalf("expected 'healthy' (marked node excluded by FailFilter), got %q", node.Name)
}
}
// TestSelect_SingleNodeKeptWhenFailed preserves the legacy safety net: a hop
// with a single node still returns it even when marked failed (FailFilter's
// len(vs) <= 1 guard / the single-node early return).
func TestSelect_SingleNodeKeptWhenFailed(t *testing.T) {
only := chain.NewNode("only", "127.0.0.1:8080")
h := newTestHop(NodeOption(only))
defer h.Close()
only.Marker().Mark()
node := h.Select(context.Background())
if node == nil {
t.Fatal("expected the only node, got nil")
}
if node.Name != "only" {
t.Fatalf("expected 'only', got %q", node.Name)
}
}
func TestSelect_NilNodesSkipped(t *testing.T) { func TestSelect_NilNodesSkipped(t *testing.T) {
h := newTestHop() h := newTestHop()
defer h.Close() defer h.Close()