fix: refresh stale client_traffics row when an inbound-deleted client's email is reused (#6003)

* fix: refresh stale client_traffics row when an inbound-deleted client's email is reused

AddClientStat's OnConflict was DoNothing on email, so once an inbound is
deleted (DelInbound only removes the client_inbounds link, matching
ClientService.Detach's intentional Detach-then-later-Attach behavior) the
orphaned client_traffics row for that email survives untouched. Re-creating
a client under the same email on a new inbound silently kept the old
enable/expiry_time/reset/total/inbound_id instead of adopting the new
client's config.

Switch the conflict path to DoUpdates on inbound_id/total/expiry_time/
enable/reset. up/down stay excluded on purpose: every call for an
already-attached identity carries the same config values (one call per
inbound), so the refresh is a no-op for that legitimate multi-inbound
share, while zeroing usage counters on each additional attach would erase
real traffic.

Fixes #5958

* fix: don't let AddClientStat clobber import's forced-enabled ClientStats rows

github-actions[bot] review on #6003 found that AddInbound writes client_traffics
twice for the same import payload: first inserting each ClientStats row
(DoNothing, with Enable forced true by controller.importInbound), then calling
AddClientStat once per Settings-derived client. With AddClientStat's OnConflict
now DoUpdates, that second call was unconditionally overwriting enable (and
total/expiry_time/reset/inbound_id) with the Settings.clients[].enable value —
which still holds whatever the client had at export time, silently undoing the
controller's "always import as enabled" behavior for any client disabled at
export.

Fix: track which emails were already seeded by the ClientStats loop and skip
the AddClientStat call for those emails, leaving the import path's forced
values as authoritative. Plain (non-import) creates are unaffected since
ClientStats is empty there, so every client still goes through AddClientStat's
refresh as before.

Also updated a stale comment in addClientTraffic that still described
AddClientStat as DoNothing.

Added TestAddInbound_ImportForcedEnableSurvivesDisabledSettingsClient, which
reproduces the exact regression (verified it fails without this fix) and
passes with it.

internal/web/service/client_stat_reuse_test.go

@@ -0,0 +1,177 @@
+package service
+
+import (
+	"testing"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/database"
+	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
+	"github.com/mhsanaei/3x-ui/v3/internal/xray"
+)
+
+// TestAddClientStat_RefreshesStaleRowOnInboundDeleteThenReuse covers #5958:
+// deleting a client's only inbound leaves its clients/client_traffics rows in
+// place (matching ClientService.Detach, which does the same on purpose so a
+// later Attach can resume a client with its accumulated traffic intact). If
+// that same email is instead reused for a freshly (re)created client via
+// ClientService.Create, the new enable/expiry/reset/total must win over
+// whatever the orphaned row still holds instead of being silently ignored by
+// AddClientStat's OnConflict.
+func TestAddClientStat_RefreshesStaleRowOnInboundDeleteThenReuse(t *testing.T) {
+	setupBulkDB(t)
+	svc := &ClientService{}
+	inboundSvc := &InboundService{}
+
+	const email = "[email protected]"
+	const subID = "sub-reused"
+
+	ibA := mkInbound(t, 22001, model.VLESS, `{"clients":[]}`)
+
+	// Create starts every client enabled (it forces Enable=true), so the
+	// depleted/disabled shape a client has by the time it's naturally deleted
+	// is reached the same way production reaches it: a follow-up Update, not
+	// the initial Create.
+	if _, err := svc.Create(inboundSvc, &ClientCreatePayload{
+		Client: model.Client{
+			Email: email, SubID: subID, Enable: true,
+			TotalGB: 0, ExpiryTime: 1000, Reset: 0,
+		},
+		InboundIds: []int{ibA.Id},
+	}); err != nil {
+		t.Fatalf("initial Create: %v", err)
+	}
+	rec0 := lookupClientRecord(t, email)
+	if _, err := svc.Update(inboundSvc, rec0.Id, model.Client{
+		Email: email, SubID: subID, Enable: false,
+		TotalGB: 0, ExpiryTime: 1000, Reset: 0,
+	}); err != nil {
+		t.Fatalf("Update to disabled: %v", err)
+	}
+
+	db := database.GetDB()
+	var before xray.ClientTraffic
+	if err := db.Model(xray.ClientTraffic{}).Where("email = ?", email).First(&before).Error; err != nil {
+		t.Fatalf("lookup client_traffics before delete: %v", err)
+	}
+	if before.Enable || before.Reset != 0 || before.Total != 0 {
+		t.Fatalf("unexpected initial client_traffics row: %+v", before)
+	}
+
+	// Delete the client's only inbound. This must NOT delete the client or its
+	// traffic row (that's the documented, intentional Detach-parity behavior) —
+	// it only orphans them.
+	if _, err := inboundSvc.DelInbound(ibA.Id); err != nil {
+		t.Fatalf("DelInbound: %v", err)
+	}
+
+	rec := lookupClientRecord(t, email)
+	ids, err := svc.GetInboundIdsForRecord(rec.Id)
+	if err != nil {
+		t.Fatalf("GetInboundIdsForRecord: %v", err)
+	}
+	if len(ids) != 0 {
+		t.Fatalf("client should be fully detached after its only inbound was deleted, still attached to: %v", ids)
+	}
+	var stillThere xray.ClientTraffic
+	if err := db.Model(xray.ClientTraffic{}).Where("email = ?", email).First(&stillThere).Error; err != nil {
+		t.Fatalf("client_traffics row should survive inbound deletion (Detach parity), lookup failed: %v", err)
+	}
+
+	// Reuse the same email + subId (the only way ClientService.Create allows
+	// re-adding under an already-used email) on a freshly created inbound, with
+	// deliberately different, "fresh" settings.
+	ibB := mkInbound(t, 22002, model.VLESS, `{"clients":[]}`)
+	const wantExpiry = int64(9999999999000)
+	if _, err := svc.Create(inboundSvc, &ClientCreatePayload{
+		Client: model.Client{
+			Email: email, SubID: subID, Enable: true,
+			TotalGB: 10 << 30, ExpiryTime: wantExpiry, Reset: 5,
+		},
+		InboundIds: []int{ibB.Id},
+	}); err != nil {
+		t.Fatalf("reuse Create: %v", err)
+	}
+
+	var after xray.ClientTraffic
+	if err := db.Model(xray.ClientTraffic{}).Where("email = ?", email).First(&after).Error; err != nil {
+		t.Fatalf("lookup client_traffics after reuse: %v", err)
+	}
+	if !after.Enable {
+		t.Errorf("client_traffics.enable still stale (false) after reuse, want true")
+	}
+	if after.Reset != 5 {
+		t.Errorf("client_traffics.reset = %d, want 5 (stale value from before delete was 0)", after.Reset)
+	}
+	if after.Total != 10<<30 {
+		t.Errorf("client_traffics.total = %d, want %d", after.Total, int64(10<<30))
+	}
+	if after.ExpiryTime != wantExpiry {
+		t.Errorf("client_traffics.expiry_time = %d, want %d", after.ExpiryTime, wantExpiry)
+	}
+	if after.InboundId != ibB.Id {
+		t.Errorf("client_traffics.inbound_id = %d, want refreshed to new inbound %d (was %d)", after.InboundId, ibB.Id, ibA.Id)
+	}
+
+	// up/down are deliberately NOT refreshed by AddClientStat — confirm that
+	// stays true (would matter if the original client had real usage).
+	if after.Up != before.Up || after.Down != before.Down {
+		t.Errorf("up/down should be left untouched by the conflict refresh: before up=%d down=%d, after up=%d down=%d",
+			before.Up, before.Down, after.Up, after.Down)
+	}
+}
+
+// TestAddClientStat_MultiInboundReattachStaysIdempotent guards the legitimate
+// case AddClientStat's OnConflict is also responsible for: a client attached
+// to two inbounds at once shares one client_traffics row, and re-asserting
+// its own current settings for the second inbound must be a no-op in effect,
+// not a data loss. In particular it must not zero out real accumulated
+// traffic just because the client gained a second attachment.
+func TestAddClientStat_MultiInboundReattachStaysIdempotent(t *testing.T) {
+	setupBulkDB(t)
+	svc := &ClientService{}
+	inboundSvc := &InboundService{}
+
+	const email = "[email protected]"
+	const subID = "sub-multi"
+
+	ibA := mkInbound(t, 22003, model.VLESS, `{"clients":[]}`)
+	if _, err := svc.Create(inboundSvc, &ClientCreatePayload{
+		Client: model.Client{
+			Email: email, SubID: subID, Enable: true,
+			TotalGB: 5 << 30, ExpiryTime: 42, Reset: 3,
+		},
+		InboundIds: []int{ibA.Id},
+	}); err != nil {
+		t.Fatalf("first Create: %v", err)
+	}
+
+	db := database.GetDB()
+	// Simulate real accumulated usage before the second attachment.
+	if err := db.Model(xray.ClientTraffic{}).Where("email = ?", email).
+		Updates(map[string]any{"up": int64(123), "down": int64(456)}).Error; err != nil {
+		t.Fatalf("seed usage: %v", err)
+	}
+
+	ibB := mkInbound(t, 22004, model.VLESS, `{"clients":[]}`)
+	// Re-adding the same identity to a second inbound: same email/subId/settings,
+	// exactly what the panel does when attaching an existing client elsewhere.
+	if _, err := svc.Create(inboundSvc, &ClientCreatePayload{
+		Client: model.Client{
+			Email: email, SubID: subID, Enable: true,
+			TotalGB: 5 << 30, ExpiryTime: 42, Reset: 3,
+		},
+		InboundIds: []int{ibB.Id},
+	}); err != nil {
+		t.Fatalf("second Create (attach to ibB): %v", err)
+	}
+
+	var row xray.ClientTraffic
+	if err := db.Model(xray.ClientTraffic{}).Where("email = ?", email).First(&row).Error; err != nil {
+		t.Fatalf("lookup after second attach: %v", err)
+	}
+	if row.Up != 123 || row.Down != 456 {
+		t.Errorf("accumulated traffic was reset by re-attach: up=%d down=%d, want 123/456", row.Up, row.Down)
+	}
+	if !row.Enable || row.Reset != 3 || row.Total != 5<<30 || row.ExpiryTime != 42 {
+		t.Errorf("config columns changed unexpectedly on idempotent re-assert: %+v", row)
+	}
+}

internal/web/service/inbound.go

@@ -830,10 +830,17 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo
 		if err := tx.Omit("ClientStats").Save(inbound).Error; err != nil {
 			return err
 		}
+		// Emails seeded here (import's ClientStats, e.g. the controller's forced
+		// Enable=true on every imported stat row) are authoritative for this call
+		// and must not be clobbered by the AddClientStat loop below, which derives
+		// its enable/total/expiry/reset from Settings.clients[] instead — a second,
+		// possibly-stale source for the same columns on a plain (non-import) create.
+		statEmails := make(map[string]bool, len(inbound.ClientStats))
 		for i := range inbound.ClientStats {
 			if inbound.ClientStats[i].Email == "" {
 				continue
 			}
+			statEmails[inbound.ClientStats[i].Email] = true
 			inbound.ClientStats[i].Id = 0
 			inbound.ClientStats[i].InboundId = inbound.Id
 			if err := tx.Clauses(clause.OnConflict{
@@ -844,6 +851,9 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo
 			}
 		}
 		for _, client := range clients {
+			if statEmails[client.Email] {
+				continue
+			}
 			if err := s.AddClientStat(tx, inbound.Id, &client); err != nil {
 				return err
 			}

internal/web/service/inbound_import_shared_clients_test.go

@@ -125,3 +125,37 @@ func TestAddInbound_ImportStatsMissingClientStillGetsTrafficRow(t *testing.T) {
 		t.Fatalf("erin Total = %d, want 2000 (quota taken from client settings)", erin.Total)
 	}
 }
+
+// TestAddInbound_ImportForcedEnableSurvivesDisabledSettingsClient covers a
+// regression the AddClientStat OnConflict fix (#5958) could otherwise
+// introduce: controller.importInbound forces every ClientStats row to
+// Enable=true (imports are meant to bring every client back enabled), but
+// Settings.clients[].enable is untouched and can still say false for a client
+// that was disabled at export time. AddInbound runs the ClientStats loop
+// first (plain insert, Enable=true) and then calls AddClientStat once per
+// Settings-derived client on the same email — that second call must not let
+// the stale, disabled Settings value win over the forced-enabled row.
+func TestAddInbound_ImportForcedEnableSurvivesDisabledSettingsClient(t *testing.T) {
+	setupConflictDB(t)
+	svc := &InboundService{}
+
+	settings := `{"clients":[` +
+		`{"id":"66666666-6666-6666-6666-666666666666","email":"frank","subId":"s-frank","enable":false,"totalGB":5000}` +
+		`],"decryption":"none","encryption":"none"}`
+	// makeImportInbound forces Enable=true on every stats row, matching
+	// controller.importInbound's behavior regardless of what's passed here.
+	in := makeImportInbound("in-9104-tcp", 9104, settings, []xray.ClientTraffic{
+		{Email: "frank", Up: 10, Down: 20, Total: 5000},
+	})
+	if _, _, err := svc.AddInbound(in); err != nil {
+		t.Fatalf("import inbound: %v", err)
+	}
+
+	var frank xray.ClientTraffic
+	if err := database.GetDB().Where("email = ?", "frank").First(&frank).Error; err != nil {
+		t.Fatalf("frank row: %v", err)
+	}
+	if !frank.Enable {
+		t.Fatalf("frank.Enable = false, want true (import must force-enable even though Settings still says disabled)")
+	}
+}

internal/web/service/inbound_traffic.go

@@ -116,8 +116,9 @@ func (s *InboundService) addClientTraffic(tx *gorm.DB, traffics []*xray.ClientTr
 	// attached to a local inbound. The old `inbound_id NOT IN (node inbounds)`
 	// filter dropped the local traffic of a client attached to both a node and the
 	// mother inbound whenever the node inbound happened to be attached first — its
-	// shared row then carried the node inbound's id (AddClientStat uses OnConflict
-	// DoNothing and never refreshes it), so the local poll skipped it entirely.
+	// shared row then carried the node inbound's id (AddClientStat used to use
+	// OnConflict DoNothing and never refreshed it; it now refreshes inbound_id on
+	// conflict, but this filter was removed rather than relying on that ordering).
 	err = tx.Model(xray.ClientTraffic{}).
 		Where("email IN (?)", emails).
 		Find(&dbClientTraffics).Error
@@ -446,9 +447,28 @@ func (s *InboundService) autoRenewClients(tx *gorm.DB) (bool, int64, error) {
 	return needRestart, int64(len(traffics)), nil
 }
 
-// AddClientStat inserts a per-client accounting row, no-op on email
-// conflict. Xray reports traffic per email, so the surviving row acts as
-// the shared accumulator for inbounds that re-use the same identity.
+// AddClientStat inserts a per-client accounting row, or refreshes the
+// config-derived columns on an email conflict. Xray reports traffic per
+// email, so the surviving row also acts as the shared accumulator for
+// inbounds that re-use the same identity — every call for that identity
+// (one per attached inbound) carries the same enable/expiry/reset/total,
+// so re-asserting them here is idempotent for that legitimate case.
+//
+// The conflict path matters on its own for a second reason: an inbound
+// delete detaches its clients (InboundService.DelInbound) without deleting
+// their client_traffics row, by design — mirroring ClientService.Detach,
+// which intentionally leaves a fully-detached client's row in place so a
+// later Attach can resume it with its accumulated traffic intact. If that
+// same email is instead reused for a freshly (re)created client, the new
+// config's enable/expiry/reset/total must win over whatever the orphaned
+// row still holds; DoNothing left them stale indefinitely (#5958).
+//
+// up/down are deliberately excluded from the refresh: they are the
+// accumulated traffic totals, and zeroing them here would erase real usage
+// every time an existing, actively-used client is attached to one more
+// inbound. One tradeoff this does not resolve: a genuinely new client that
+// happens to reuse an orphaned email still inherits that row's leftover
+// up/down, since nothing at this call site can tell the two cases apart.
 func (s *InboundService) AddClientStat(tx *gorm.DB, inboundId int, client *model.Client) error {
 	clientTraffic := xray.ClientTraffic{
 		InboundId:  inboundId,
@@ -458,8 +478,10 @@ func (s *InboundService) AddClientStat(tx *gorm.DB, inboundId int, client *model
 		Enable:     client.Enable,
 		Reset:      client.Reset,
 	}
-	return tx.Clauses(clause.OnConflict{Columns: []clause.Column{{Name: "email"}}, DoNothing: true}).
-		Create(&clientTraffic).Error
+	return tx.Clauses(clause.OnConflict{
+		Columns:   []clause.Column{{Name: "email"}},
+		DoUpdates: clause.AssignmentColumns([]string{"inbound_id", "total", "expiry_time", "enable", "reset"}),
+	}).Create(&clientTraffic).Error
 }
 
 func (s *InboundService) UpdateClientStat(tx *gorm.DB, email string, client *model.Client) error {