Browse Source

fix(outbounds): preserve stable subscription tags (#6345)

An inserted link could claim a previous positional tag before the existing identity that owned it was processed. The owner was then suffixed and the swapped mapping persisted across refreshes.

Reserve tags for identities still present in the batch so positional fallback, fresh allocation, and collision suffixes cannot take them.
dawn 16 hours ago
parent
commit
8abe87b625

+ 11 - 2
internal/web/service/outbound_subscription.go

@@ -462,6 +462,13 @@ func (s *OutboundSubscriptionService) recordError(sub *model.OutboundSubscriptio
 // written back into parsed[i]["tag"]. The returned slice holds the assigned tags
 // written back into parsed[i]["tag"]. The returned slice holds the assigned tags
 // in order. When tagPrefix is empty a "sub<subID>-" prefix is used for fresh tags.
 // in order. When tagPrefix is empty a "sub<subID>-" prefix is used for fresh tags.
 func assignStableTags(parsed []link.Outbound, identities []string, prev map[string]string, prevTagByIndex map[int]string, subID int, tagPrefix string) []string {
 func assignStableTags(parsed []link.Outbound, identities []string, prev map[string]string, prevTagByIndex map[int]string, subID int, tagPrefix string) []string {
+	reservedStableTags := map[string]bool{}
+	for i := range parsed {
+		if i < len(identities) && prev[identities[i]] != "" {
+			reservedStableTags[prev[identities[i]]] = true
+		}
+	}
+
 	used := map[string]bool{} // uniqueness within this refresh batch
 	used := map[string]bool{} // uniqueness within this refresh batch
 	assigned := make([]string, len(parsed))
 	assigned := make([]string, len(parsed))
 	for i := range parsed {
 	for i := range parsed {
@@ -470,12 +477,14 @@ func assignStableTags(parsed []link.Outbound, identities []string, prev map[stri
 			id = identities[i]
 			id = identities[i]
 		}
 		}
 		candidate := ""
 		candidate := ""
+		identityTag := ""
 		if old, ok := prev[id]; ok && old != "" {
 		if old, ok := prev[id]; ok && old != "" {
 			candidate = old
 			candidate = old
+			identityTag = old
 		}
 		}
 		if candidate == "" {
 		if candidate == "" {
 			// try to reuse by rough positional match from previous fetch (best effort)
 			// try to reuse by rough positional match from previous fetch (best effort)
-			if old, ok := prevTagByIndex[i]; ok && old != "" {
+			if old, ok := prevTagByIndex[i]; ok && old != "" && !reservedStableTags[old] {
 				candidate = old
 				candidate = old
 			}
 			}
 		}
 		}
@@ -493,7 +502,7 @@ func assignStableTags(parsed []link.Outbound, identities []string, prev map[stri
 		}
 		}
 		// ensure local uniqueness inside this batch
 		// ensure local uniqueness inside this batch
 		final := candidate
 		final := candidate
-		for k := 1; used[final]; k++ {
+		for k := 1; used[final] || (reservedStableTags[final] && final != identityTag); k++ {
 			final = fmt.Sprintf("%s-%d", candidate, k)
 			final = fmt.Sprintf("%s-%d", candidate, k)
 		}
 		}
 		used[final] = true
 		used[final] = true

+ 43 - 1
internal/web/service/outbound_subscription_test.go

@@ -3,6 +3,7 @@ package service
 import (
 import (
 	"bytes"
 	"bytes"
 	"errors"
 	"errors"
+	"slices"
 	"testing"
 	"testing"
 
 
 	"gorm.io/gorm"
 	"gorm.io/gorm"
@@ -166,12 +167,53 @@ func TestAssignStableTags(t *testing.T) {
 
 
 	t.Run("falls back to the previous tag at the same position", func(t *testing.T) {
 	t.Run("falls back to the previous tag at the same position", func(t *testing.T) {
 		parsed := []link.Outbound{{"tag": "JP-Tokyo"}}
 		parsed := []link.Outbound{{"tag": "JP-Tokyo"}}
-		got := assignStableTags(parsed, []string{"id-new"}, map[string]string{}, map[int]string{0: "sub1-oldpos"}, 1, "")
+		prev := map[string]string{"id-gone": "sub1-oldpos"}
+		got := assignStableTags(parsed, []string{"id-new"}, prev, map[int]string{0: "sub1-oldpos"}, 1, "")
 		if got[0] != "sub1-oldpos" {
 		if got[0] != "sub1-oldpos" {
 			t.Fatalf("got %q, want sub1-oldpos", got[0])
 			t.Fatalf("got %q, want sub1-oldpos", got[0])
 		}
 		}
 	})
 	})
 
 
+	t.Run("does not let an inserted link steal a stable tag", func(t *testing.T) {
+		parsed := []link.Outbound{{"tag": "Poland"}, {"tag": "NewServer"}, {"tag": "Netherlands"}}
+		prev := map[string]string{
+			"id-poland":      "sub1-poland",
+			"id-netherlands": "sub1-netherlands",
+		}
+		prevTagByIndex := map[int]string{0: "sub1-poland", 1: "sub1-netherlands"}
+
+		got := assignStableTags(parsed, []string{"id-poland", "id-new", "id-netherlands"}, prev, prevTagByIndex, 1, "")
+		want := []string{"sub1-poland", "sub1-newserver", "sub1-netherlands"}
+		if !slices.Equal(got, want) {
+			t.Fatalf("got %v, want %v", got, want)
+		}
+	})
+
+	t.Run("does not let a fresh tag steal a stable tag", func(t *testing.T) {
+		parsed := []link.Outbound{{"tag": "Netherlands"}, {"tag": "Renamed"}}
+		prev := map[string]string{"id-netherlands": "sub1-netherlands"}
+
+		got := assignStableTags(parsed, []string{"id-new", "id-netherlands"}, prev, nil, 1, "")
+		want := []string{"sub1-netherlands-1", "sub1-netherlands"}
+		if !slices.Equal(got, want) {
+			t.Fatalf("got %v, want %v", got, want)
+		}
+	})
+
+	t.Run("skips reserved tags while adding a suffix", func(t *testing.T) {
+		parsed := []link.Outbound{{"tag": "Netherlands"}, {"tag": "First"}, {"tag": "Second"}}
+		prev := map[string]string{
+			"id-first":  "sub1-netherlands",
+			"id-second": "sub1-netherlands-1",
+		}
+
+		got := assignStableTags(parsed, []string{"id-new", "id-first", "id-second"}, prev, nil, 1, "")
+		want := []string{"sub1-netherlands-2", "sub1-netherlands", "sub1-netherlands-1"}
+		if !slices.Equal(got, want) {
+			t.Fatalf("got %v, want %v", got, want)
+		}
+	})
+
 	t.Run("allocates a fresh tag with the default sub<id>- prefix", func(t *testing.T) {
 	t.Run("allocates a fresh tag with the default sub<id>- prefix", func(t *testing.T) {
 		parsed := []link.Outbound{{"tag": "Tokyo"}}
 		parsed := []link.Outbound{{"tag": "Tokyo"}}
 		got := assignStableTags(parsed, []string{"id-x"}, nil, nil, 7, "")
 		got := assignStableTags(parsed, []string{"id-x"}, nil, nil, 7, "")