fix(hop): skip priority short-circuit when multiple nodes share the same priority

When two or more nodes have an identical matcher rule they receive
identical default priority (rule string length). The priority shortcut
assumed the top-priority node was the single authoritative choice and
returned it directly, bypassing the selector (FailFilter, BackupFilter,
strategy) — so weight, round-robin, and hash were silently ignored.

Add a third condition: the top priority must be strictly greater than
the second-highest. When multiple nodes share the same highest priority
the selector applies normally for load balancing.

Add doc comments on NodeMatcherConfig.Priority explaining the three
semantic ranges (0=auto, negative=always-selector, positive=shortcut).
This commit is contained in:
ginuerzh
2026-06-26 22:53:17 +08:00
parent c1d954d689
commit cd97e5e746
4 changed files with 45 additions and 3 deletions
+9
View File
@@ -405,6 +405,15 @@ type NodeFilterConfig struct {
Path string `yaml:",omitempty" json:"path,omitempty"` Path string `yaml:",omitempty" json:"path,omitempty"`
} }
// NodeMatcherConfig defines a routing-rule matcher for a hop node.
//
// Priority controls election among multiple matching nodes:
// - 0 (default): auto-computed from the rule string length — longer rules
// (more specific) get higher priority.
// - negative: the node participates in matching but priority short-circuit
// is disabled; the selector (round-robin, random, hash, etc.) always applies.
// - positive: explicit priority; when a single node has strictly higher
// priority than all others, it wins directly, bypassing the selector.
type NodeMatcherConfig struct { type NodeMatcherConfig struct {
Rule string `yaml:",omitempty" json:"rule,omitempty"` Rule string `yaml:",omitempty" json:"rule,omitempty"`
Priority int `yaml:",omitempty" json:"priority,omitempty"` Priority int `yaml:",omitempty" json:"priority,omitempty"`
+4
View File
@@ -176,6 +176,10 @@ func ParseNode(hop string, cfg *config.NodeConfig, log logger.Logger) (*chain.No
if rule := strings.TrimSpace(cfg.Matcher.Rule); rule != "" { if rule := strings.TrimSpace(cfg.Matcher.Rule); rule != "" {
if matcher, err := routing.NewMatcher(rule); err == nil { if matcher, err := routing.NewMatcher(rule); err == nil {
log.Debugf("new matcher for node %s with rule %s", cfg.Name, cfg.Matcher.Rule) log.Debugf("new matcher for node %s with rule %s", cfg.Name, cfg.Matcher.Rule)
// Priority 0 means "use default": automatically set to the
// rule length so longer (more specific) rules outrank shorter
// ones. Use a negative priority to opt out of this behavior
// and always go through the selector.
if priority == 0 { if priority == 0 {
priority = len(cfg.Matcher.Rule) priority = len(cfg.Matcher.Rule)
} }
+7 -3
View File
@@ -208,13 +208,17 @@ func (p *chainHop) Select(ctx context.Context, opts ...hop.SelectOption) *chain.
}) })
if nodes[0].Options().Priority > 0 && if nodes[0].Options().Priority > 0 &&
!anyBackupNode(nodes) { !anyBackupNode(nodes) &&
nodes[0].Options().Priority > nodes[1].Options().Priority {
// Priority short-circuit: highest-priority non-backup node wins. // Priority short-circuit: highest-priority non-backup node wins.
// Conditions: (1) top priority > 0 means a matcher indicated routing // Conditions: (1) top priority > 0 means a matcher indicated routing
// specificity, so the top node is authoritative for this request; // specificity, so the top node is authoritative for this request;
// (2) no backup node is present, otherwise BackupFilter would be // (2) no backup node is present, otherwise BackupFilter would be
// silently bypassed. When both hold the selector (FailFilter, // silently bypassed; (3) the top priority is strictly higher than
// BackupFilter, strategy) is skipped and the best node wins directly. // the second-highest — when multiple nodes share the same matcher
// rule they have equal priority and the selector (FailFilter,
// BackupFilter, strategy) should still apply for load balancing.
// When all three hold the selector is skipped and the best node wins directly.
p.logger.Debugf("priority shortcut: node %s selected", nodes[0].Name) p.logger.Debugf("priority shortcut: node %s selected", nodes[0].Name)
return nodes[0] return nodes[0]
} }
+25
View File
@@ -522,6 +522,31 @@ func TestSelect_WithSelector(t *testing.T) {
} }
} }
func TestSelect_EqualPriorityDoesNotShortcut(t *testing.T) {
// Two nodes with identical matchers get the same default priority.
// The priority shortcut must NOT trigger when multiple nodes share
// the highest priority — the selector should still apply.
n1 := chain.NewNode("n1", "127.0.0.1:8080",
chain.MatcherNodeOption(&testMatcher{match: true}),
chain.PriorityNodeOption(50),
)
n2 := chain.NewNode("n2", "127.0.0.1:9090",
chain.MatcherNodeOption(&testMatcher{match: true}),
chain.PriorityNodeOption(50),
)
sel := &testNodeSelector{selectedIdx: 1} // picks n2
h := newTestHop(NodeOption(n1, n2), SelectorOption(sel))
defer h.Close()
node := h.Select(context.Background())
if node == nil {
t.Fatal("expected node, got nil")
}
if node.Name != "n2" {
t.Errorf("expected 'n2' (selected by selector, not shortcut), 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()