From 8cd71e07eae140b56a36efd98659c47c183ad83e Mon Sep 17 00:00:00 2001 From: "Mr. Nickson" <259025431+mrnickson-hue@users.noreply.github.com> Date: Tue, 21 Jul 2026 16:57:55 +0300 Subject: [PATCH] fix: refresh stale client_traffics row when an inbound-deleted client's email is reused (#6003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .../web/service/client_stat_reuse_test.go | 177 ++++++++++++++++++ internal/web/service/inbound.go | 10 + .../inbound_import_shared_clients_test.go | 34 ++++ internal/web/service/inbound_traffic.go | 36 +++- 4 files changed, 250 insertions(+), 7 deletions(-) create mode 100644 internal/web/service/client_stat_reuse_test.go diff --git a/internal/web/service/client_stat_reuse_test.go b/internal/web/service/client_stat_reuse_test.go new file mode 100644 index 000000000..69affa270 --- /dev/null +++ b/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 = "reused@example.com" + 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 = "multi@example.com" + 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) + } +} diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index 991295172..2c17665bc 100644 --- a/internal/web/service/inbound.go +++ b/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 } diff --git a/internal/web/service/inbound_import_shared_clients_test.go b/internal/web/service/inbound_import_shared_clients_test.go index c5999cb40..42f4a1806 100644 --- a/internal/web/service/inbound_import_shared_clients_test.go +++ b/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)") + } +} diff --git a/internal/web/service/inbound_traffic.go b/internal/web/service/inbound_traffic.go index 4b2a2953c..c65d9a769 100644 --- a/internal/web/service/inbound_traffic.go +++ b/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 {