fix(node): keep hub settings on an empty node snapshot; address review

The empty-snapshot guard skipped only the link rebuild and orphan sweep,
but Phase A had already adopted the node's `{"clients":[]}` blob into
`inbounds.settings`. Reconcile builds each push from that blob, so the hub
kept re-pushing an empty client list and a reset/restarted node never got
its clients back — the guard fired forever. Phase A now refuses to blank the
settings of an inbound the hub still populates, so the hub stays
authoritative and reconcile re-pushes the real clients (recovery). A
node-side removal of the last client is therefore hub-authoritative by
design; the partial-snapshot path still prunes an inbound that reports other
clients.

- test: assert the inbound's settings survive an empty snapshot (fails
  without the Phase A fix).
- drop TestSetRemoteTraffic_EmptySnapshotSurvivesReap (no branch the core
  test doesn't already cover) and the duplicate orphanMark helper
  (readOrphanMark already exists); trim the comment blocks to the 2-line
  CLAUDE.md limit.
- fix a pre-existing -race/-shuffle flake: TestGetAmneziaWGLogs owned no DB
  and relied on the ambient global one, which a sibling's dbtest cleanup
  closes under shuffle, panicking on nil in amneziawgLogActivity. It now
  owns a throwaway DB.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

internal/web/service/client_sync_orphan_test.go

@@ -30,12 +30,8 @@ func backdateOrphanMark(t *testing.T, db *gorm.DB, email string) {
 	}
 }
 
-// The merge must soft-orphan, not delete: everything stays recoverable until
-// the grace period has elapsed and the reaper confirms nothing reclaimed it.
-// The removal is driven by a partial snapshot (the node is alive and still
-// serves another client, so dropping one is authoritative); a fully empty
-// snapshot is treated as a degraded node and never orphans — see
-// TestSetRemoteTraffic_EmptySnapshotKeepsClients.
+// A partial snapshot (node alive, still serving another client) authoritatively drops one;
+// the merge soft-orphans, recoverable until the grace elapses and the reaper confirms it.
 func TestSyncOrphanSurvivesMergeUntilGraceElapses(t *testing.T) {
 	db := initTrafficTestDB(t)
 	svc := &InboundService{}
@@ -92,10 +88,8 @@ func TestSyncOrphanSurvivesMergeUntilGraceElapses(t *testing.T) {
 	}
 }
 
-// A client the node reports again was never gone: clearing the mark is what
+// A client the node reports again (partial snapshot) was never gone: clearing the mark
 // turns a bad merge into a recoverable blip instead of a delayed deletion.
-// The drop is driven by a partial snapshot (node alive, still serving another
-// client); a fully empty snapshot is a degraded node and never orphans.
 func TestSyncOrphanMarkClearedOnReattach(t *testing.T) {
 	db := initTrafficTestDB(t)
 	svc := &InboundService{}

internal/web/service/inbound_amneziawg_test.go

@@ -287,6 +287,9 @@ func TestNormalizeAmneziaWGSettings_CanonicalizesClientAllowedIPs(t *testing.T)
 }
 
 func TestGetAmneziaWGLogs_ClampsCountAndFiltersEvents(t *testing.T) {
+	// GetAmneziaWGLogs appends peer handshake activity, which reads the DB;
+	// own a throwaway one so -shuffle can't leave us the global nil DB.
+	setupConflictDB(t)
 	logger.InitLogger(logging.DEBUG)
 	logger.Info("amneziawg: started interface awg1 for inbound 1")
 	logger.Info("xray: unrelated line that must never show up here")

internal/web/service/inbound_node.go

@@ -1167,6 +1167,16 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
 			lifecycleLifted = true
 		}
 		if p.central.Settings != adoptedSettings {
+			// #6734: a zero-client snapshot must not blank an inbound the hub still
+			// populates, or reconcile re-pushes an empty blob and the node never recovers.
+			snapCopy := *p.snapIb
+			snapCopy.Settings = adoptedSettings
+			if blob, gcErr := s.GetClients(&snapCopy); gcErr == nil && len(blob) == 0 {
+				var hubAttached int64
+				if err := tx.Table("client_inbounds").Where("inbound_id = ?", p.central.Id).Count(&hubAttached).Error; err == nil && hubAttached > 0 {
+					continue
+				}
+			}
 			if err := tx.Model(model.Inbound{}).
 				Where("id = ?", p.central.Id).
 				Update("settings", adoptedSettings).Error; err != nil {
@@ -1228,22 +1238,8 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
 			applyMasterClientLifecycle(&clients[i], existing, csPtr)
 			filtered = append(filtered, clients[i])
 		}
-		// Empty-snapshot guard. A node that comes back reporting zero clients for
-		// an inbound the hub still has clients on is indistinguishable from a
-		// genuine "every client was removed" — and in practice it is almost always
-		// the former: the node was just deleted, reset, restarted, or returned a
-		// snapshot before its config loaded. Acting on it is destructive and hard
-		// to undo: SyncInbound makes the inbound's links match the set exactly, so
-		// an empty set deletes every link for the inbound; a client shared across
-		// nodes then loses its links one node at a time and, once the last one
-		// goes, is orphan-marked and hard-deleted by ReapSyncOrphans — the
-		// 2026-10-04 outage, where removing one node deleted clients that still
-		// lived on the others. Treat a zero-client snapshot as non-authoritative:
-		// skip the link rebuild and the orphan sweep for this inbound (the same
-		// handling a failed SyncInbound gets) and wait for a snapshot that carries
-		// clients. Removing the last client from a node is instead done from the
-		// hub (which updates links and pushes); a node that still serves other
-		// clients prunes a removed one authoritatively through the partial path.
+		// A degraded node (reset/restart/removal) reports zero clients for an inbound the
+		// hub populates; adopting it empties links and ReapSyncOrphans deletes shared clients (#6734).
 		if len(filtered) == 0 && len(oldEmailsRows) > 0 {
 			logger.Warningf("setRemoteTraffic: node snapshot for tag %q reported zero clients while the hub has %d attached — treating as a degraded snapshot, skipping link rebuild and orphan sweep for this inbound", snapIb.Tag, len(oldEmailsRows))
 			syncFailedInbounds[c.Id] = struct{}{}

internal/web/service/node_degraded_snapshot_test.go

@@ -1,6 +1,7 @@
 package service
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
@@ -23,22 +24,8 @@ func linkCount(t *testing.T, db *gorm.DB, email string) int64 {
 	return n
 }
 
-func orphanMark(t *testing.T, db *gorm.DB, email string) int64 {
-	t.Helper()
-	var at int64
-	if err := db.Model(&model.ClientRecord{}).Where("email = ?", email).
-		Pluck("sync_orphaned_at", &at).Error; err != nil {
-		t.Fatalf("read sync_orphaned_at %q: %v", email, err)
-	}
-	return at
-}
-
-// TestSetRemoteTraffic_EmptySnapshotKeepsClients is the core guard: a managed
-// node that comes back reporting zero clients for an inbound the hub still has
-// clients on is treated as degraded (just deleted/reset/restarted, or a
-// snapshot taken before the config loaded), not as "every client was removed".
-// Its links must stay and nothing may be orphan-marked — otherwise SyncInbound
-// would strip every link and ReapSyncOrphans would later hard-delete the row.
+// A degraded node reporting zero clients for an inbound the hub populates must
+// keep its links and never orphan-mark, or SyncInbound/ReapSyncOrphans delete the row.
 func TestSetRemoteTraffic_EmptySnapshotKeepsClients(t *testing.T) {
 	db := initTrafficTestDB(t)
 	svc := &InboundService{}
@@ -66,50 +53,22 @@ func TestSetRemoteTraffic_EmptySnapshotKeepsClients(t *testing.T) {
 	if n := linkCount(t, db, "svc@x"); n != 1 {
 		t.Fatalf("empty snapshot stripped the client link: links=%d, want 1", n)
 	}
-	if at := orphanMark(t, db, "svc@x"); at != 0 {
+	if at := readOrphanMark(t, db, "svc@x"); at != 0 {
 		t.Fatalf("empty snapshot orphan-marked a live client: sync_orphaned_at=%d, want 0", at)
 	}
-}
-
-// TestSetRemoteTraffic_EmptySnapshotSurvivesReap closes the loop on the outage
-// of 2026-10-04: without the guard an empty/degraded snapshot orphan-marks the
-// inbound's clients and ReapSyncOrphans hard-deletes them after the grace
-// period. The guard keeps them attached, so even a backdated reap can't take
-// them — which is exactly what deleting one node must not do to the clients it
-// served.
-func TestSetRemoteTraffic_EmptySnapshotSurvivesReap(t *testing.T) {
-	db := initTrafficTestDB(t)
-	svc := &InboundService{}
-	clientSvc := &ClientService{}
-
-	seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true})
-	createNodeInboundWithClient(t, db, 1, "n1-in", 41001, "svc@x")
-
-	settings := `{"clients":[{"email":"svc@x","enable":true}]}`
-	if _, err := svc.setRemoteTrafficLocked(1, snapshotWithClients(t, "n1-in", settings,
-		xray.ClientTraffic{Email: "svc@x", Enable: true}), false, false); err != nil {
-		t.Fatalf("seed sync: %v", err)
+	// The hub must keep the client in the inbound's settings, or reconcile re-pushes
+	// an empty blob to the node and the clients never come back (#6734).
+	var ib model.Inbound
+	if err := db.Where("tag = ?", "n1-in").First(&ib).Error; err != nil {
+		t.Fatalf("read central inbound: %v", err)
 	}
-
-	if _, err := svc.setRemoteTrafficLocked(1, snapshotWithoutClients(t, "n1-in"), false, false); err != nil {
-		t.Fatalf("empty-snapshot sync: %v", err)
-	}
-
-	backdateOrphanMark(t, db, "svc@x") // no-op if unmarked; proves reap can't take it
-	if reaped, err := clientSvc.ReapSyncOrphans(); err != nil {
-		t.Fatalf("reap: %v", err)
-	} else if reaped != 0 {
-		t.Fatalf("reaped %d client(s) off an empty snapshot, want 0", reaped)
-	}
-	if rec, _ := countClientRows(t, db, "svc@x"); rec != 1 {
-		t.Fatalf("empty snapshot + reap deleted the client: clients=%d, want 1", rec)
+	if !strings.Contains(ib.Settings, "svc@x") {
+		t.Fatalf("empty snapshot blanked the inbound settings: %q", ib.Settings)
 	}
 }
 
-// TestSetRemoteTraffic_PartialSnapshotStillPrunes confirms the guard is narrow:
-// a snapshot that still carries at least one client is authoritative, so a
-// client the node really dropped is still unlinked and left for the orphan
-// sweep. Only the all-empty snapshot is treated as degraded.
+// The guard is narrow: a snapshot still carrying a client is authoritative, so a
+// client the node really dropped is unlinked and orphan-marked; only all-empty is degraded.
 func TestSetRemoteTraffic_PartialSnapshotStillPrunes(t *testing.T) {
 	db := initTrafficTestDB(t)
 	svc := &InboundService{}
@@ -140,7 +99,7 @@ func TestSetRemoteTraffic_PartialSnapshotStillPrunes(t *testing.T) {
 	if n := linkCount(t, db, "drop@x"); n != 0 {
 		t.Fatalf("partial snapshot kept an unreported client linked: drop@x links=%d, want 0", n)
 	}
-	if at := orphanMark(t, db, "drop@x"); at <= 0 {
+	if at := readOrphanMark(t, db, "drop@x"); at <= 0 {
 		t.Fatalf("partial snapshot did not orphan-mark the removed client: sync_orphaned_at=%d, want >0", at)
 	}
 }