Skip to content

fix(outbounds): enforce tagPrefix on refresh and deduplicate outbound tags (#7110) - #7111

Open
Gaoshou101 wants to merge 2 commits into
MHSanaei:mainfrom
Gaoshou101:fix/sub-tagprefix-dups-7110
Open

Gaoshou101 wants to merge 2 commits into
MHSanaei:mainfrom
Gaoshou101:fix/sub-tagprefix-dups-7110

Conversation

@Gaoshou101

@Gaoshou101 Gaoshou101 commented Nov 10, 2026 •

Copy link
Copy Markdown

Fixes #7110

Root Cause

  1. In assignStableTags, existing identity tags from sub.LinkIdentities (prev[id]) and positional fallbacks (prevTagByIndex[i]) were reused unconditionally without checking if they match the subscription's active tagPrefix. Additionally, Update() wrote tag_prefix without updating persisted link_identities / last_fetched_outbounds.
  2. In mergeSubscriptionOutbounds and activeOutboundsSplit, subscription outbounds (prepend and appendList) were not checked against template outbound tags or other subscriptions' tags. When subscriptions contained overlapping tags, duplicate tags reached the generated Xray configuration, causing Xray-core to fail on startup with app/proxyman/outbound: existing tag found: <tag>.

Key Changes

  1. Enforce active prefix matching (including shortened prefixes like eu-de- -> eu-) and rewrite on Update():
    • In assignStableTags (matchesActiveTagPrefix), verify that cached tags start with the normalized expectedPrefix and do not retain an extra prefix segment ahead of the link's base slug when a prefix is shortened (e.g., eu-de-frankfurt -> eu-frankfurt).
    • In Update(), when the normalized prefix changes (oldEffective != newEffective), rewriteStoredPrefix immediately updates persisted last_fetched_outbounds and link_identities to the new prefix.
  2. Preserve legacy blank-prefix subscriptions (sub<id>-) on Update():
    • When sub.TagPrefix == "" (legacy row using sub<id>- fallback) and the user saves the edit form with tagPrefix == "", keep tag_prefix blank instead of allocating an unrelated subN- via nextDefaultSubPrefix(sub.Id).
  3. Persist cross-subscription / template collision disambiguation and unify panel surfaces (AllActiveOutbounds / AllActiveOutboundTags):
    • In fetchOutbounds and rewriteStoredPrefix, collect reservedExternalTags(sub.Id) (tags from template outbounds and other subscriptions' LastFetchedOutbounds) and pass them to assignStableTagsWithExternal so -N disambiguation suffixes are persisted directly in LastFetchedOutbounds and LinkIdentities (preventing priority reordering or subscription disabling from silently swapping tags).
    • Apply dedupeSubscriptionOutboundList (with logger.Warningf logging) in both activeOutboundsSplit and mergeSubscriptionOutbounds so AllActiveOutbounds, AllActiveOutboundTags, OutboundTraffics, and the generated Xray config always expose identical tags.

Verification

  • go test ./internal/web/service -run 'TestAssignStableTags|TestDefaultPrefixNumber|TestOutboundSubscription|TestActiveOutbounds|TestMergeSubscriptionOutbounds' -count=1 -v (20/20 PASS)
  • go vet ./internal/web/service, gofmt -l, git diff --check (all clean)

… tags (MHSanaei#7110)

- Check expectedPrefix in assignStableTags so modified tagPrefix regenerates tags instead of indefinitely reusing stale tags
- Disambiguate colliding tags in mergeSubscriptionOutbounds to prevent Xray startup crashes
- Add unit tests for tagPrefix updates and outbound tag deduplication

Fixes MHSanaei#7110
Co-authored-by: yzykov <37885628+yzykov@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

An operator who gave several outbound subscriptions overlapping tag prefixes, or who later changed a prefix, was left with Xray refusing to start ("existing tag found"), and on every automatic refresh the old tags came back. With this change, a prefix edit now renames that subscription's outbounds under the new prefix at the next refresh. Any outbound tag that still collides is renamed with a numeric suffix while the config is built, so the core starts again. Two things are new for operators. Routing rules or balancers that named an old tag exactly stop matching once the prefix changes; the issue asked for this, but it is a user-visible behaviour change for the maintainer to accept. And the suffix renaming happens only inside the generated config, which the panel never shows. Verdict: Real, partly fixed. The startup failure is gone, but making a prefix shorter (for example eu-de- to eu-) still keeps the old tags.

No blocking issues — 0 HIGH / 2 MEDIUM / 1 LOW open (3 new), 0 pre-existing

Reviewed head: 93a7aeb

Evidence. Problem is real (read on base): assignStableTags reuses prev[id] (internal/web/service/outbound_subscription.go:620) and prevTagByIndex[i] (:626) before the prefix is ever read. The prefix is only used for a fresh allocation (:632). Update writes tag_prefix (:252) and leaves link_identities alone. mergeSubscriptionOutbounds (internal/web/service/xray.go:1124-1145 on base) concatenates prepend + template + append with no tag check. Xray-core rejects the result in Manager.AddHandler (xtls/xray-core app/proxyman/outbound/outbound.go:116, existing tag found: ), in the base-pinned module. I agree with the issue-analyst bot's root cause and with the two halves it named. Root fix (read on head): identity and positional reuse now require the normalized prefix (pr-head/internal/web/service/outbound_subscription.go:608-610, :617, :632, :638), and that normalization matches link.SuggestTag (internal/util/link/outbound.go:1310-1313), so freshly generated tags pass their own check on the next refresh. The merge sink reserves template tags first and suffixes collisions in both subscription groups (pr-head/internal/web/service/xray.go:1137-1182). That covers the shared-prefix trigger even when no prefix ever changes. Tests (read, not run): the subtest at pr-head/internal/web/service/outbound_subscription_test.go:391 fails on base, because base returns gh-canada-1 from prev. TestMergeSubscriptionOutboundsDeduplicatesDuplicateTags (pr-head/internal/web/service/xray_config_inject_test.go:1089) fails on base at its seen[tag] check, because base emits gh-canada-1 and direct twice. Both can tell the broken behaviour from the fixed one. Parallel work: no other open PR or merged commit touches this. #6341 (within-batch duplication) is a different cause.

Findings

MEDIUM — the merge-time rename produces tags that no panel surface knows about (pr-head/internal/web/service/xray.go:1167-1175). This PR added the in-place cloned["tag"] = final rename. It writes the new tag only into the generated Xray config. The stored LastFetchedOutbounds keep the original tag. So do AllActiveOutbounds / AllActiveOutboundTags (internal/web/service/outbound_subscription.go:661, :752), which feed the Outbounds tab and the balancer/routing tag pickers through subscriptionOutbounds / subscriptionOutboundTags (internal/web/controller/xray_setting.go:126-131). For two subscriptions that both emit gh-canada-1, the UI lists gh-canada-1 twice and never lists gh-canada-1-1. Every rule or balancer that names gh-canada-1 goes to whichever subscription comes first in priority order. The second server cannot be selected by any tag the panel offers. Its traffic is recorded under a tag (OutboundTraffics.Tag, internal/database/model/model.go:112) that matches no outbound the panel lists. Which subscription gets the bare tag depends on priority and enabled state (activeOutboundsSplit, :676, priority asc, id asc). So moving or disabling a subscription silently swaps which server an existing rule sends traffic through. The rename leaves no log line or error, so an operator has nothing to trace the swap back to. Rests on reading; not reproduced.

MEDIUM — editing a legacy blank-prefix subscription now silently renames all its tags (pr-head/internal/web/service/outbound_subscription.go:632). This PR added the strings.HasPrefix(old, expectedPrefix) gate. Before it, an implicit prefix rewrite in Update did not affect existing tags. A row stored with an empty tag_prefix has tags sub<id>-… (the fallback at :605-607). The code says such rows exist (comment at :137). The edit form pre-fills the stored blank (frontend/src/pages/xray/outbounds/OutboundsTab.tsx:381). Saving any field (URL, interval, remark) therefore lands in Update's blank branch (:236-237), which stores nextDefaultSubPrefix(sub.Id): the smallest free subN-, which excludes the row's own id (defaultPrefixNumber, :142-144) and so is generally not sub<id>-. On base, identity reuse kept the old tags. On head, the next refresh drops every sub<id>- tag and regenerates them as subN-…, although the operator never touched the prefix. Routing rules that named the old tags then point at missing outbounds, and Xray closes those connections instead of falling back to the default outbound (xtls/xray-core app/dispatcher/default.go:467-470, base-pinned module). The trigger needs a row written before blank prefixes were defaulted at save time. That is inferred from the code comment and could not be confirmed, because this clone has no history.

LOW — shortening a prefix still keeps the old tags (pr-head/internal/web/service/outbound_subscription.go:632, :638). This PR introduced the plain strings.HasPrefix gate. A tag from the old prefix passes whenever the new normalized prefix is a leading part of it: prefix eu-de- changed to eu- keeps every eu-de-… tag through both the identity and the positional path. The issue's expected behaviour ("Changing TagPrefix should regenerate outbound tags using the new prefix") is not met for that edit, and the new subtest only covers an unrelated pair (gh- to gh1-). Read, not run.

Coverage

  • Head 93a7aeb; 4 files, +109/−9, all read in full against base.
  • Tag assignment: assignStableTags, its caller fetchOutbounds, Create/Update prefix defaulting, defaultPrefixNumber, link.SuggestTag normalization.
  • Merge sink: mergeSubscriptionOutbounds, its single caller in GetXrayConfig, and the later injectors (mtproto, AmneziaWG, TUIC, panel and node egress), which run after the merge and are unchanged.
  • Panel consumers of subscription tags: AllActiveOutbounds(Tags), the xray_setting controller, frontend tag picker, OutboundTraffics.
  • Upstream (base-pinned xray-core): duplicate-tag rejection and missing-outTag handling verified at the cited files.
  • Runtime/dispatch, migrations, endpoints, workflows: not touched by this diff; no schema change, no new route.
  • Tests: both new tests read and reasoned red-on-base; not executed (this job runs no PR code).
  • CI: the CI workflow run is action_required and awaits maintainer approval for this fork, so nothing it covers (go-test, race, lint, frontend, Postgres) is verified. prepare succeeded; review is this run.
  • No text in the PR body, commits or diff tries to steer this review.

… persist cross-sub deduped tags (MHSanaei#7110)

- Shortened TagPrefix ('eu-de-' -> 'eu-'): detect extra prefix segments in matchesActiveTagPrefix during assignStableTags, and rewrite stored LinkIdentities / LastFetchedOutbounds in Update() when the normalized prefix changes.
- Legacy blank TagPrefix: preserve blank tag_prefix ('sub<id>-' fallback) in Update() when the stored TagPrefix is already empty and the user saves without entering a new prefix, avoiding silent renames to an unrelated 'subN-'.
- Cross-subscription & template tag collisions: pass reservedExternalTags into fetchOutbounds / rewriteStoredPrefix so collision suffixes ('-N') are persisted in LastFetchedOutbounds & LinkIdentities (preventing priority moves or disables from swapping tags), and apply dedupeSubscriptionOutboundList (with warning logs) in activeOutboundsSplit so AllActiveOutbounds / AllActiveOutboundTags always match the generated Xray config.
@Gaoshou101

Copy link
Copy Markdown
Author

Addressed all three findings from the review in commit 168361c7:

1. MEDIUM — Merge-time rename vs. panel surfaces (AllActiveOutbounds / AllActiveOutboundTags), priority swaps, and logging

  • Persisted disambiguation at fetch/update time: Added reservedExternalTags(excludeSubID) to collect tags from template outbounds and other subscriptions' LastFetchedOutbounds. Both fetchOutbounds (via assignStableTagsWithExternal) and rewriteStoredPrefix now disambiguate against externalUsed before persisting to LastFetchedOutbounds and LinkIdentities. Because the -N suffix (e.g., gh-canada-1-1) is stored directly on the second subscription's row, moving subscription priorities (Move) or disabling the first subscription does not silently swap the second subscription's tag.
  • Unified panel & runtime config surfaces: activeOutboundsSplit() now runs dedupeSubscriptionOutboundList against templateOutboundTags() before returning prepend and appendList, so AllActiveOutbounds(), AllActiveOutboundTags(), the Outbounds tab, balancer/routing tag pickers, OutboundTraffics, and mergeSubscriptionOutbounds() all see the exact same tags.
  • Warning log on collision: dedupeSubscriptionOutboundList logs logger.Warningf("outbound subscription tag %q collides with an existing outbound, renamed to %q", tag, final) whenever a duplicate tag is disambiguated.

2. MEDIUM — Editing a legacy blank-prefix subscription silently renames all its tags

  • In OutboundSubscriptionService.Update, nextDefaultSubPrefix(sub.Id) is now only called when prefix == "" and sub.TagPrefix != "" (i.e., the operator cleared a previously non-empty prefix). When editing a legacy row whose stored TagPrefix is already "" and leaving the prefix blank, tag_prefix remains "" and its sub<id>-… tags are preserved across edits and refreshes.

3. LOW — Shortening a prefix (e.g. eu-de- to eu-) keeps old tags

  • Pure tag assignment (matchesActiveTagPrefix): Checks whether the remainder after expectedPrefix still carries a leftover prefix segment ahead of the link's own slug (rest != baseSlug && strings.HasSuffix(rest, "-"+baseSlug), including -N collision suffixes), rejecting stale tags like eu-de-frankfurt when expectedPrefix is shortened to eu-.
  • Immediate rewrite on Update() (rewriteStoredPrefix): When oldEffective != newEffective in Update(), stored LastFetchedOutbounds and LinkIdentities have their oldEffective prefix rewritten to newEffective in-place.

Verification

$ go test ./internal/web/service -run 'TestAssignStableTags|TestDefaultPrefixNumber|TestOutboundSubscription|TestActiveOutbounds|TestMergeSubscriptionOutbounds' -count=1 -v
=== RUN   TestOutboundSubscriptionCreatePropagatesAllocationDatabaseFailures
--- PASS: TestOutboundSubscriptionCreatePropagatesAllocationDatabaseFailures (3.06s)
=== RUN   TestOutboundSubscriptionUpdatePropagatesPrefixQueryFailureWithoutMutation
--- PASS: TestOutboundSubscriptionUpdatePropagatesPrefixQueryFailureWithoutMutation (0.32s)
=== RUN   TestAssignStableTags
=== RUN   TestAssignStableTags/regenerates_tags_with_new_prefix_when_tagPrefix_changed_(#7110)
=== RUN   TestAssignStableTags/regenerates_tags_when_tagPrefix_is_shortened_(#7110)
--- PASS: TestAssignStableTags (0.00s)
=== RUN   TestOutboundSubscriptionUpdateRewritesShortenedPrefixAndPreservesLegacyBlankPrefix
--- PASS: TestOutboundSubscriptionUpdateRewritesShortenedPrefixAndPreservesLegacyBlankPrefix (0.88s)
=== RUN   TestOutboundSubscriptionCrossSubscriptionCollisionPersistsAndMatchesPanelSurfaces
--- PASS: TestOutboundSubscriptionCrossSubscriptionCollisionPersistsAndMatchesPanelSurfaces (0.84s)
=== RUN   TestMergeSubscriptionOutboundsDeduplicatesDuplicateTags
--- PASS: TestMergeSubscriptionOutboundsDeduplicatesDuplicateTags (0.00s)
PASS
ok  	github.com/mhsanaei/3x-ui/v3/internal/web/service	13.235s

$ go vet ./internal/web/service && gofmt -l internal/web/service/outbound_subscription.go internal/web/service/xray.go internal/web/service/outbound_subscription_test.go internal/web/service/xray_config_inject_test.go && git diff --check
(exit 0, 0 warnings)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Changing outbound subscription TagPrefix preserves old tags, causing duplicate outbounds and Xray startup failure

1 participant