From c4448f4ea80f1027ae05ec310523f2ab5695bf17 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sat, 11 Jul 2026 21:13:20 +0200 Subject: [PATCH] fix(clients): rename client record atomically with inbound settings Update committed an email rename to the clients table with a standalone write before the per-inbound loop rewrote each inbound's settings JSON. In that window the record held the new email while the JSON still held the old one, so any concurrent SyncInbound (traffic poll, another edit) found no record for the old email and inserted a duplicate seeded from the stale JSON - carrying the same subId, which then failed every later edit with "Duplicate subId". The subId collision check also ran after that write, so even a rejected update permanently renamed the email. Move the rename inside UpdateInboundClient's serialized transaction, next to the settings save and SyncInbound, so no other writer can see one without the other; skip it when a record already owns the target email (the merge case). Update now only runs collision checks before the loop and falls back to a direct rename solely for records with no attached inbound. This covers both the REST API and the web UI editor, which share this path. Closes #5870 --- internal/web/service/client_crud.go | 15 ++-- internal/web/service/client_inbound_apply.go | 13 +++ .../web/service/client_update_rename_test.go | 88 +++++++++++++++++++ 3 files changed, 111 insertions(+), 5 deletions(-) create mode 100644 internal/web/service/client_update_rename_test.go diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index 2bca49d7e..693a5ea9f 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -373,11 +373,6 @@ func (s *ClientService) Update(inboundSvc *InboundService, id int, updated model if collisionCount > 0 { return false, common.NewError("Duplicate email:", updated.Email) } - if err := database.GetDB().Model(&model.ClientRecord{}). - Where("id = ?", id). - Update("email", updated.Email).Error; err != nil { - return false, err - } } if updated.SubID != "" { @@ -428,6 +423,16 @@ func (s *ClientService) Update(inboundSvc *InboundService, id int, updated model } } + // UpdateInboundClient renames the record atomically with each inbound's + // settings JSON; this direct write only covers records with no inbound left. + if updated.Email != existing.Email { + if err := database.GetDB().Model(&model.ClientRecord{}). + Where("id = ? AND email = ?", id, existing.Email). + Update("email", updated.Email).Error; err != nil { + return needRestart, err + } + } + reverseStr := "" if updated.Reverse != nil && strings.TrimSpace(updated.Reverse.Tag) != "" { if b, mErr := json.Marshal(updated.Reverse); mErr == nil { diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 849f9a90d..89e787fa0 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -805,6 +805,19 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo if e := tx.Save(oldInbound).Error; e != nil { return e } + // Rename the client record in the same transaction as the settings JSON + // so no concurrent SyncInbound can see one renamed without the other. + if len(oldEmail) > 0 && !strings.EqualFold(oldEmail, clients[0].Email) { + var renameTaken int64 + if e := tx.Model(&model.ClientRecord{}).Where("email = ?", clients[0].Email).Count(&renameTaken).Error; e != nil { + return e + } + if renameTaken == 0 { + if e := tx.Model(&model.ClientRecord{}).Where("email = ?", oldEmail).Update("email", clients[0].Email).Error; e != nil { + return e + } + } + } finalClients, gcErr := inboundSvc.GetClients(oldInbound) if gcErr != nil { return gcErr diff --git a/internal/web/service/client_update_rename_test.go b/internal/web/service/client_update_rename_test.go new file mode 100644 index 000000000..d14823cb0 --- /dev/null +++ b/internal/web/service/client_update_rename_test.go @@ -0,0 +1,88 @@ +package service + +import ( + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +func countClientRecords(t *testing.T) int64 { + t.Helper() + var n int64 + if err := database.GetDB().Model(&model.ClientRecord{}).Count(&n).Error; err != nil { + t.Fatalf("count client records: %v", err) + } + return n +} + +func TestUpdateInboundClientRenameDoesNotDuplicateRecord(t *testing.T) { + setupBulkDB(t) + svc := &ClientService{} + inboundSvc := &InboundService{} + + source := []model.Client{{Email: "old@x", ID: "aaaaaaaa-0000-0000-0000-000000000001", SubID: "sub-old", Enable: true}} + ib := mkInbound(t, 22001, model.VLESS, clientsSettings(t, source)) + if err := svc.SyncInbound(nil, ib.Id, source); err != nil { + t.Fatalf("seed linkage: %v", err) + } + origId := lookupClientRecord(t, "old@x").Id + + renamed := source + renamed[0].Email = "new@x" + if _, err := svc.UpdateInboundClient(inboundSvc, &model.Inbound{ + Id: ib.Id, + Settings: clientsSettings(t, renamed), + }, "old@x"); err != nil { + t.Fatalf("UpdateInboundClient: %v", err) + } + + if n := countClientRecords(t); n != 1 { + t.Fatalf("client records after rename = %d, want 1", n) + } + rec := lookupClientRecord(t, "new@x") + if rec.Id != origId { + t.Fatalf("record id after rename = %d, want %d", rec.Id, origId) + } +} + +func TestClientUpdateDuplicateSubIDDoesNotRenameEmail(t *testing.T) { + setupBulkDB(t) + svc := &ClientService{} + inboundSvc := &InboundService{} + + source := []model.Client{ + {Email: "keep@x", ID: "aaaaaaaa-0000-0000-0000-000000000003", SubID: "sub-keep", Enable: true}, + {Email: "other@x", ID: "aaaaaaaa-0000-0000-0000-000000000004", SubID: "sub-other", Enable: true}, + } + ib := mkInbound(t, 22003, model.VLESS, clientsSettings(t, source)) + if err := svc.SyncInbound(nil, ib.Id, source); err != nil { + t.Fatalf("seed linkage: %v", err) + } + origId := lookupClientRecord(t, "keep@x").Id + origSettings := mustInboundSettings(t, inboundSvc, ib.Id) + + updated := source[0] + updated.Email = "kept@x" + updated.SubID = "sub-other" + if _, err := svc.Update(inboundSvc, origId, updated); err == nil { + t.Fatalf("Update with colliding subId succeeded, want error") + } + + rec := lookupClientRecord(t, "keep@x") + if rec.Id != origId { + t.Fatalf("record id changed after rejected update") + } + if got := mustInboundSettings(t, inboundSvc, ib.Id); got != origSettings { + t.Fatalf("inbound settings changed after rejected update") + } +} + +func mustInboundSettings(t *testing.T, inboundSvc *InboundService, id int) string { + t.Helper() + ib, err := inboundSvc.GetInbound(id) + if err != nil { + t.Fatalf("GetInbound %d: %v", id, err) + } + return ib.Settings +}