From cd97e5e7465e754e00633c8cfe830dea022b1a39 Mon Sep 17 00:00:00 2001
From: ginuerzh <ginuerzh@gmail.com>
Date: Fri, 26 Jun 2026 22:53:17 +0800
Subject: [PATCH] fix(hop): skip priority short-circuit when multiple nodes
 share the same priority
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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).
---
 config/config.go             |  9 +++++++++
 config/parsing/node/parse.go |  4 ++++
 hop/hop.go                   | 10 +++++++---
 hop/hop_test.go              | 25 +++++++++++++++++++++++++
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/config/config.go b/config/config.go
index 7cb19761..7d462c0d 100644
--- a/config/config.go
+++ b/config/config.go
@@ -405,6 +405,15 @@ type NodeFilterConfig struct {
 	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 {
 	Rule     string `yaml:",omitempty" json:"rule,omitempty"`
 	Priority int    `yaml:",omitempty" json:"priority,omitempty"`
diff --git a/config/parsing/node/parse.go b/config/parsing/node/parse.go
index deb77ffc..feb09096 100644
--- a/config/parsing/node/parse.go
+++ b/config/parsing/node/parse.go
@@ -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 matcher, err := routing.NewMatcher(rule); err == nil {
 				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 {
 					priority = len(cfg.Matcher.Rule)
 				}
diff --git a/hop/hop.go b/hop/hop.go
index 70cec3bd..56411076 100644
--- a/hop/hop.go
+++ b/hop/hop.go
@@ -208,13 +208,17 @@ func (p *chainHop) Select(ctx context.Context, opts ...hop.SelectOption) *chain.
 	})
 
 	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.
 		// Conditions: (1) top priority > 0 means a matcher indicated routing
 		// specificity, so the top node is authoritative for this request;
 		// (2) no backup node is present, otherwise BackupFilter would be
-		// silently bypassed. When both hold the selector (FailFilter,
-		// BackupFilter, strategy) is skipped and the best node wins directly.
+		// silently bypassed; (3) the top priority is strictly higher than
+		// 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)
 		return nodes[0]
 	}
diff --git a/hop/hop_test.go b/hop/hop_test.go
index 2f84809f..77fb3e1e 100644
--- a/hop/hop_test.go
+++ b/hop/hop_test.go
@@ -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) {
 	h := newTestHop()
 	defer h.Close()