diff --git a/internal/web/job/node_traffic_sync_job.go b/internal/web/job/node_traffic_sync_job.go index e919a2e54..87d283954 100644 --- a/internal/web/job/node_traffic_sync_job.go +++ b/internal/web/job/node_traffic_sync_job.go @@ -307,7 +307,7 @@ func (j *NodeTrafficSyncJob) syncOne(mgr *runtime.Manager, n *model.Node, doIpSy if guidTrees, err := rt.FetchClientIpsByGuid(ipCtx); err != nil { logger.Debugf("node traffic sync: fetch client ip attribution from %s failed: %v", n.Name, err) } else if len(guidTrees) > 0 { - if err := j.inboundService.MergeClientIpsByGuid(guidTrees); err != nil { + if err := j.inboundService.MergeClientIpsByGuid(n, guidTrees); err != nil { logger.Warningf("node traffic sync: merge client ip attribution from %s failed: %v", n.Name, err) } } diff --git a/internal/web/service/inbound_node.go b/internal/web/service/inbound_node.go index cc7653f35..09d8080a2 100644 --- a/internal/web/service/inbound_node.go +++ b/internal/web/service/inbound_node.go @@ -211,23 +211,15 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi // originGuidFor attributes a synced inbound to the panel that physically // hosts it: inbounds the node forwards from its own sub-nodes already carry // a non-empty OriginNodeGuid (kept as-is across hops); the node's own local - // inbounds report empty, so they are attributed to the node's own key. That - // key is the node's panelGuid, unless another of this master's nodes reports - // the same GUID (cloned server — the panelGuid ships in the copied settings), - // in which case it would collapse both nodes into one #4983 bucket, so fall - // back to the node-unique id. Old-build nodes with no GUID use the id too. + // inbounds report empty, so they are attributed to the node's own key — + // effectiveNodeKey, which is the node's panelGuid unless that GUID is + // ambiguous (shared with another node or the master, i.e. a cloned server), + // in which case it falls back to the node-unique id so #4983 attribution + // doesn't collapse two physical nodes into one bucket. var nodeRow model.Node db.Select("guid").Where("id = ?", nodeID).First(&nodeRow) - guidShared := false - if nodeRow.Guid != "" { - var sameGuid int64 - db.Model(&model.Node{}).Where("guid = ?", nodeRow.Guid).Count(&sameGuid) - guidShared = sameGuid > 1 - } - selfKey := nodeRow.Guid - if selfKey == "" || guidShared { - selfKey = synthNodeGuid(nodeID) - } + selfKey := effectiveNodeKey(&model.Node{Id: nodeID, Guid: nodeRow.Guid}) + guidShared := nodeRow.Guid != "" && selfKey != nodeRow.Guid originGuidFor := func(snapIb *model.Inbound) string { if snapIb.OriginNodeGuid != "" { return snapIb.OriginNodeGuid diff --git a/internal/web/service/inbound_node_ips.go b/internal/web/service/inbound_node_ips.go index a173f3f19..13950db15 100644 --- a/internal/web/service/inbound_node_ips.go +++ b/internal/web/service/inbound_node_ips.go @@ -113,8 +113,26 @@ func (s *InboundService) RecordLocalClientIps(panelGuid string, observed map[str // MergeClientIpsByGuid folds a node's guid-keyed attribution report (its own // panelGuid subtree plus any descendants) into the local table, preserving which -// physical node each IP is on across a chain. -func (s *InboundService) MergeClientIpsByGuid(trees map[string]map[string][]model.ClientIpEntry) error { +// physical node each IP is on across a chain. When node is non-nil and its own +// panelGuid is ambiguous (shared with another node or the master — a cloned +// server), the node's own subtree is remapped to its node-unique key so two +// clones don't collapse into one attribution row; descendant subtrees keep their +// distinct GUIDs. A nil node merges the report verbatim. +func (s *InboundService) MergeClientIpsByGuid(node *model.Node, trees map[string]map[string][]model.ClientIpEntry) error { + if node != nil && node.Guid != "" { + if eff := effectiveNodeKey(node); eff != node.Guid { + if sub, ok := trees[node.Guid]; ok { + delete(trees, node.Guid) + if existing, ok := trees[eff]; ok { + for email, ips := range sub { + existing[email] = append(existing[email], ips...) + } + } else { + trees[eff] = sub + } + } + } + } for guid, perEmail := range trees { if err := upsertNodeClientIps(guid, perEmail); err != nil { return err @@ -242,18 +260,24 @@ func (s *InboundService) GetClientIpsWithNodes(email string) ([]ClientIpInfo, er return out, nil } -// nodeGuidNameMap maps each known node's stable guid to its display name. +// nodeGuidNameMap maps each known node's attribution key to its display name, +// keyed by effectiveNodeGuid so a cloned node's IPs (stored under its node-unique +// key) still resolve to the right name instead of colliding under a shared GUID. func (s *InboundService) nodeGuidNameMap() map[string]string { db := database.GetDB() var nodes []model.Node if err := db.Model(&model.Node{}).Find(&nodes).Error; err != nil { return map[string]string{} } + ptrs := make([]*model.Node, len(nodes)) + for i := range nodes { + ptrs[i] = &nodes[i] + } + selfGuid, _ := (&SettingService{}).GetPanelGuid() + ambiguous := ambiguousNodeGuids(ptrs, selfGuid) m := make(map[string]string, len(nodes)) - for _, n := range nodes { - if n.Guid != "" { - m[n.Guid] = n.Name - } + for i := range nodes { + m[effectiveNodeGuid(&nodes[i], ambiguous)] = nodes[i].Name } return m } diff --git a/internal/web/service/inbound_node_ips_test.go b/internal/web/service/inbound_node_ips_test.go index ba4f3769c..c534b14a1 100644 --- a/internal/web/service/inbound_node_ips_test.go +++ b/internal/web/service/inbound_node_ips_test.go @@ -103,7 +103,7 @@ func TestGetClientIpNodeAttribution_NewestGuidWins(t *testing.T) { }); err != nil { t.Fatalf("record gA: %v", err) } - if err := svc.MergeClientIpsByGuid(map[string]map[string][]model.ClientIpEntry{ + if err := svc.MergeClientIpsByGuid(nil, map[string]map[string][]model.ClientIpEntry{ "gB": {"u@x": {{IP: "9.9.9.9", Timestamp: now}}}, }); err != nil { t.Fatalf("merge gB: %v", err) @@ -145,7 +145,7 @@ func TestGetClientIpsWithNodes_LabelsNodes(t *testing.T) { }); err != nil { t.Fatalf("record local: %v", err) } - if err := svc.MergeClientIpsByGuid(map[string]map[string][]model.ClientIpEntry{ + if err := svc.MergeClientIpsByGuid(nil, map[string]map[string][]model.ClientIpEntry{ "node-guid": {"u@x": {{IP: "2.2.2.2", Timestamp: now}}}, }); err != nil { t.Fatalf("merge node: %v", err) diff --git a/internal/web/service/node.go b/internal/web/service/node.go index 33feccf21..58ba65687 100644 --- a/internal/web/service/node.go +++ b/internal/web/service/node.go @@ -14,6 +14,7 @@ import ( "slices" "strconv" "strings" + "sync" "time" "github.com/mhsanaei/3x-ui/v3/internal/database" @@ -192,14 +193,15 @@ func (s *NodeService) GetAll() ([]*model.Node, error) { } } onlineByGuid := s.onlineEmailsByGuid() - shared := sharedNodeGuids(nodes) + selfGuid, _ := (&SettingService{}).GetPanelGuid() + ambiguous := ambiguousNodeGuids(nodes, selfGuid) for _, n := range nodes { n.InboundCount = len(inboundsByNode[n.Id]) n.DepletedCount = depletedByNode[n.Id] // Online is attributed to the node that physically hosts the client // (by GUID): a client on a sub-node counts under the sub-node, not // the intermediate node it syncs through (#4983). - n.OnlineCount = len(onlineByGuid[effectiveNodeGuid(n, shared)]) + n.OnlineCount = len(onlineByGuid[effectiveNodeGuid(n, ambiguous)]) } return nodes, nil @@ -221,39 +223,67 @@ func (s *NodeService) onlineEmailsByGuid() map[string]map[string]struct{} { // effectiveNodeGuid is a node's stable online/inbound attribution key: its // reported panelGuid, or a master-local synthetic node-id fallback when the node -// has no GUID yet (old build) or shares its GUID with another direct node. The -// shared case is a cloned server — the panelGuid is copied with the disk image — -// where an identical GUID would otherwise collapse two physical nodes into one -// #4983 attribution bucket. shared comes from sharedNodeGuids. -func effectiveNodeGuid(n *model.Node, shared map[string]struct{}) string { +// has no GUID yet (old build) or its GUID is ambiguous. ambiguous comes from +// ambiguousNodeGuids. +func effectiveNodeGuid(n *model.Node, ambiguous map[string]struct{}) string { if n.Guid == "" { return synthNodeGuid(n.Id) } if n.Id > 0 { - if _, dup := shared[n.Guid]; dup { + if _, bad := ambiguous[n.Guid]; bad { return synthNodeGuid(n.Id) } } return n.Guid } -// sharedNodeGuids returns the panelGuids reported by more than one of this -// master's own direct nodes (Id > 0). Transitive sub-nodes (Id 0) carry distinct -// descendant GUIDs by construction and are excluded. -func sharedNodeGuids(nodes []*model.Node) map[string]struct{} { +// ambiguousNodeGuids returns the panelGuids a node must not be attributed under +// directly, because doing so would merge two distinct identities: a GUID +// reported by more than one of this master's direct nodes (cloned node servers +// ship the same panelGuid in their copied settings), or a GUID equal to the +// master's own panelGuid (a node cloned from the master). A node holding such a +// GUID falls back to its node-unique synthNodeGuid. Transitive sub-nodes (Id 0) +// carry distinct descendant GUIDs by construction and are excluded. +func ambiguousNodeGuids(nodes []*model.Node, selfGuid string) map[string]struct{} { counts := make(map[string]int, len(nodes)) for _, n := range nodes { if n.Id > 0 && n.Guid != "" { counts[n.Guid]++ } } - shared := make(map[string]struct{}) + ambiguous := make(map[string]struct{}) for guid, c := range counts { if c > 1 { - shared[guid] = struct{}{} + ambiguous[guid] = struct{}{} } } - return shared + if selfGuid != "" { + if _, ok := counts[selfGuid]; ok { + ambiguous[selfGuid] = struct{}{} + } + } + return ambiguous +} + +// effectiveNodeKey returns one node's attribution key without a preloaded node +// list — its panelGuid when that GUID uniquely identifies it among the master's +// nodes and differs from the master's own, otherwise its node-unique +// synthNodeGuid. Same rule as effectiveNodeGuid + ambiguousNodeGuids, for the +// write paths that handle a single node (online tree, IP attribution). +func effectiveNodeKey(node *model.Node) string { + if node == nil { + return "" + } + if node.Guid == "" { + return synthNodeGuid(node.Id) + } + var sameGuid int64 + database.GetDB().Model(&model.Node{}).Where("guid = ?", node.Guid).Count(&sameGuid) + masterGuid, _ := (&SettingService{}).GetPanelGuid() + if sameGuid > 1 || node.Guid == masterGuid { + return synthNodeGuid(node.Id) + } + return node.Guid } func (s *NodeService) GetById(id int) (*model.Node, error) { @@ -484,7 +514,9 @@ func (s *NodeService) Delete(id int) error { return common.NewError(fmt.Sprintf("cannot delete node: %d inbound(s) still attached to it; detach or delete them first", attached)) } // Capture the node's guid before deleting the row so we can drop its per-node - // IP attribution (NodeClientIp is keyed by guid, not node id). + // IP attribution. NodeClientIp is keyed by the node's attribution key, which + // is its guid normally but its node-unique key for a cloned/ambiguous-guid + // node (see effectiveNodeKey) — so we purge both below. var guid string var n model.Node if err := db.Select("guid").Where("id = ?", id).First(&n).Error; err == nil { @@ -498,10 +530,12 @@ func (s *NodeService) Delete(id int) error { if err := tx.Where("node_id = ?", id).Delete(&model.NodeClientTraffic{}).Error; err != nil { return err } + guids := []string{synthNodeGuid(id)} if guid != "" { - if err := tx.Where("node_guid = ?", guid).Delete(&model.NodeClientIp{}).Error; err != nil { - return err - } + guids = append(guids, guid) + } + if err := tx.Where("node_guid IN ?", guids).Delete(&model.NodeClientIp{}).Error; err != nil { + return err } return tx.Where("id = ?", id).Delete(&model.Node{}).Error }); err != nil { @@ -621,6 +655,7 @@ func (s *NodeService) UpdateHeartbeat(id int, p HeartbeatPatch) error { // failed probe) reports none, so the stable identity survives blips. if p.Guid != "" { updates["guid"] = p.Guid + s.warnOnDuplicateGuid(id, p.Guid) } if err := db.Model(model.Node{}).Where("id = ?", id).Updates(updates).Error; err != nil { return err @@ -635,6 +670,30 @@ func (s *NodeService) UpdateHeartbeat(id int, p HeartbeatPatch) error { return nil } +// warnedDupGuid remembers the (nodeID -> guid) pairs already warned about so a +// cloned-server collision is logged once, not every heartbeat. +var warnedDupGuid sync.Map + +// warnOnDuplicateGuid logs once when a node reports a panelGuid already held by +// another node or by the master itself (the cloned-server footgun). Attribution +// still works — it falls back to node-unique keys — but the operator should +// regenerate the duplicate panelGuid to restore real identity and per-node IP +// attribution. Re-arms if the collision later clears. +func (s *NodeService) warnOnDuplicateGuid(id int, guid string) { + var clash int64 + database.GetDB().Model(&model.Node{}).Where("guid = ? AND id <> ?", guid, id).Count(&clash) + masterGuid, _ := (&SettingService{}).GetPanelGuid() + if clash == 0 && guid != masterGuid { + warnedDupGuid.Delete(id) + return + } + if prev, ok := warnedDupGuid.Load(id); ok && prev == guid { + return + } + warnedDupGuid.Store(id, guid) + logger.Warningf("node %d reports panelGuid %s already used by another node or the master (cloned server?) — regenerate it on that node so online and IP attribution stay per-node", id, guid) +} + func (s *NodeService) MarkNodeDirty(id int) error { if id <= 0 { return nil diff --git a/internal/web/service/node_shared_guid_test.go b/internal/web/service/node_shared_guid_test.go index a264244b0..bb77f1ceb 100644 --- a/internal/web/service/node_shared_guid_test.go +++ b/internal/web/service/node_shared_guid_test.go @@ -1,34 +1,41 @@ package service import ( + "fmt" "testing" + "time" "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/model" ) -// Cloned node servers ship an identical panelGuid in their copied settings. -// effectiveNodeGuid must keep each physical node in its own attribution bucket -// by falling back to the node-unique id when a GUID is shared, while leaving a -// uniquely-identified node on its real GUID. -func TestEffectiveNodeGuid_DisambiguatesSharedGuids(t *testing.T) { +// Cloned node servers ship an identical panelGuid in their copied settings, and +// a node cloned from the master shares the master's own GUID. effectiveNodeGuid +// must keep each physical node in its own attribution bucket by falling back to +// the node-unique id for both collision kinds, while leaving a uniquely-named +// node on its real GUID and never folding transitive (Id 0) nodes. +func TestEffectiveNodeGuid_DisambiguatesAmbiguousGuids(t *testing.T) { nodes := []*model.Node{ {Id: 1, Guid: "dup"}, {Id: 2, Guid: "dup"}, {Id: 3, Guid: "uniq"}, {Id: 4, Guid: ""}, + {Id: 5, Guid: "master"}, {Id: 0, Guid: "transitive"}, } - shared := sharedNodeGuids(nodes) + ambiguous := ambiguousNodeGuids(nodes, "master") - if _, ok := shared["dup"]; !ok { - t.Fatalf("dup must be flagged shared, got %v", shared) + if _, ok := ambiguous["dup"]; !ok { + t.Fatalf("dup must be flagged ambiguous, got %v", ambiguous) } - if _, ok := shared["uniq"]; ok { - t.Fatalf("uniq must not be shared, got %v", shared) + if _, ok := ambiguous["master"]; !ok { + t.Fatalf("a node sharing the master GUID must be flagged, got %v", ambiguous) } - if _, ok := shared["transitive"]; ok { - t.Fatalf("transitive (Id 0) must not count toward sharing, got %v", shared) + if _, ok := ambiguous["uniq"]; ok { + t.Fatalf("uniq must not be flagged, got %v", ambiguous) + } + if _, ok := ambiguous["transitive"]; ok { + t.Fatalf("transitive (Id 0) must not count, got %v", ambiguous) } cases := map[*model.Node]string{ @@ -36,15 +43,50 @@ func TestEffectiveNodeGuid_DisambiguatesSharedGuids(t *testing.T) { nodes[1]: "node:2", nodes[2]: "uniq", nodes[3]: "node:4", - nodes[4]: "transitive", + nodes[4]: "node:5", + nodes[5]: "transitive", } for n, want := range cases { - if got := effectiveNodeGuid(n, shared); got != want { + if got := effectiveNodeGuid(n, ambiguous); got != want { t.Errorf("effectiveNodeGuid(Id=%d, Guid=%q) = %q, want %q", n.Id, n.Guid, got, want) } } } +// effectiveNodeKey (the no-preloaded-list variant used by the write paths) must +// agree with the slice helper: fall back to the node-unique id when a GUID is +// shared with another node or with the master, else keep the real GUID. +func TestEffectiveNodeKey_FallsBackOnCollision(t *testing.T) { + setupConflictDB(t) + db := database.GetDB() + selfGuid, _ := (&SettingService{}).GetPanelGuid() + if selfGuid == "" { + t.Fatal("expected a panel guid") + } + + mk := func(id int, name, guid string) *model.Node { + n := &model.Node{Id: id, Name: name, Address: fmt.Sprintf("10.0.0.%d", id), Port: 2053, ApiToken: "t", Guid: guid, Status: "online"} + if err := db.Create(n).Error; err != nil { + t.Fatalf("create %s: %v", name, err) + } + return n + } + dupA := mk(1, "a", "shared") + mk(2, "b", "shared") + uniq := mk(3, "c", "solo") + masterClone := mk(4, "d", selfGuid) + + if got := effectiveNodeKey(dupA); got != "node:1" { + t.Errorf("node-node collision: got %q, want node:1", got) + } + if got := effectiveNodeKey(uniq); got != "solo" { + t.Errorf("unique node: got %q, want solo", got) + } + if got := effectiveNodeKey(masterClone); got != "node:4" { + t.Errorf("master collision: got %q, want node:4", got) + } +} + // recountByGuid must split per-node counts even when two direct nodes share a // GUID and their inbounds still carry that shared GUID as origin (pre-backfill). func TestRecountByGuid_SplitsClonedNodesWithSharedGuid(t *testing.T) { @@ -84,3 +126,37 @@ func TestRecountByGuid_SplitsClonedNodesWithSharedGuid(t *testing.T) { t.Errorf("unique node InboundCount = %d, want 1", n3.InboundCount) } } + +// A cloned node's IP-attribution subtree must be stored under its node-unique +// key, so a second clone sharing the GUID can't overwrite it in node_client_ips. +func TestMergeClientIpsByGuid_RemapsClonedNodeSubtree(t *testing.T) { + setupClientIpTestDB(t) + db := database.GetDB() + svc := &InboundService{} + now := time.Now().Unix() + + n1 := &model.Node{Id: 1, Name: "A", Address: "10.0.0.1", Port: 2053, ApiToken: "t", Guid: "dup", Status: "online"} + n2 := &model.Node{Id: 2, Name: "B", Address: "10.0.0.2", Port: 2053, ApiToken: "t", Guid: "dup", Status: "online"} + for _, n := range []*model.Node{n1, n2} { + if err := db.Create(n).Error; err != nil { + t.Fatalf("create node: %v", err) + } + } + + if err := svc.MergeClientIpsByGuid(n1, map[string]map[string][]model.ClientIpEntry{ + "dup": {"u@x": {{IP: "1.1.1.1", Timestamp: now}}}, + }); err != nil { + t.Fatalf("merge n1: %v", err) + } + + var rows []model.NodeClientIp + if err := db.Find(&rows).Error; err != nil { + t.Fatalf("load rows: %v", err) + } + if len(rows) != 1 { + t.Fatalf("want 1 attribution row, got %d", len(rows)) + } + if rows[0].NodeGuid != "node:1" { + t.Errorf("cloned node IPs must be stored under node-unique key, got %q", rows[0].NodeGuid) + } +} diff --git a/internal/web/service/node_tree.go b/internal/web/service/node_tree.go index f8d168fb8..1ecad16e3 100644 --- a/internal/web/service/node_tree.go +++ b/internal/web/service/node_tree.go @@ -174,7 +174,7 @@ func (s *NodeService) recountByGuid(nodes []*model.Node, selfGuid string) { if err := db.Table("inbounds").Select("id, node_id, origin_node_guid").Scan(&ibRows).Error; err != nil { return } - shared := sharedNodeGuids(nodes) + ambiguous := ambiguousNodeGuids(nodes, selfGuid) effByInbound := make(map[int]string, len(ibRows)) inboundCountByGuid := make(map[string]int) ids := make([]int, 0, len(ibRows)) @@ -187,10 +187,10 @@ func (s *NodeService) recountByGuid(nodes []*model.Node, selfGuid string) { guid = selfGuid } } else if r.NodeID != nil { - // Origin still holds a GUID two direct nodes share (cloned server, - // not yet re-attributed): bucket under the hosting node's unique id - // so the clones don't merge. - if _, dup := shared[guid]; dup { + // Origin still holds an ambiguous GUID (cloned server / master-shared, + // not yet re-attributed): bucket under the hosting node's unique id so + // the clones don't merge. + if _, bad := ambiguous[guid]; bad { guid = synthNodeGuid(*r.NodeID) } } @@ -230,7 +230,7 @@ func (s *NodeService) recountByGuid(nodes []*model.Node, selfGuid string) { onlineByGuid := s.onlineEmailsByGuid() for _, n := range nodes { - guid := effectiveNodeGuid(n, shared) + guid := effectiveNodeGuid(n, ambiguous) n.InboundCount = inboundCountByGuid[guid] n.OnlineCount = len(onlineByGuid[guid]) n.DepletedCount = depletedByGuid[guid]