From 6aa87f4e57549b2dd9078716db91391c1a25a981 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sat, 11 Jul 2026 21:37:45 +0200 Subject: [PATCH] fix(clients): finish deleting from every inbound when one fails Delete aborted its per-inbound loop on the first error, so a client attached to inbounds across several nodes lost at most one per attempt: the loop never reached the remaining nodes, the record cleanup after the loop never ran, and each retry started over with whatever was left. Operators with many nodes had to delete the same client once per node. Collect per-inbound failures and keep going so every reachable inbound and node is cleaned in a single pass, then keep the client record only when something failed - its settings JSON still holds the client there, so the next delete retries exactly the leftovers - and return the joined failures instead of silently reporting success. DeleteByEmail's legacy fallback loop gets the same treatment. Closes #5845 --- internal/web/service/client_crud.go | 19 +++++- .../service/client_delete_continue_test.go | 64 +++++++++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) create mode 100644 internal/web/service/client_delete_continue_test.go diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index 693a5ea9f..b8a41b824 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -493,12 +493,14 @@ func (s *ClientService) Delete(inboundSvc *InboundService, id int, keepTraffic b } needRestart := false + var delErrs []error for _, ibId := range inboundIds { if _, getErr := inboundSvc.GetInbound(ibId); getErr != nil { if errors.Is(getErr, gorm.ErrRecordNotFound) { continue } - return needRestart, getErr + delErrs = append(delErrs, fmt.Errorf("inbound %d: %w", ibId, getErr)) + continue } // Always delete by email — the client's stable identity. This removes @@ -515,12 +517,18 @@ func (s *ClientService) Delete(inboundSvc *InboundService, id int, keepTraffic b if errors.Is(delErr, ErrClientNotInInbound) { continue } - return needRestart, delErr + delErrs = append(delErrs, fmt.Errorf("inbound %d: %w", ibId, delErr)) + continue } if nr { needRestart = true } } + // A failed inbound still holds the client in its settings JSON: keep the + // record so the next delete retries exactly the leftovers, and report it. + if len(delErrs) > 0 { + return needRestart, errors.Join(delErrs...) + } db := database.GetDB() if err := db.Transaction(func(tx *gorm.DB) error { @@ -668,18 +676,23 @@ func (s *ClientService) DeleteByEmail(inboundSvc *InboundService, email string, return false, common.NewError(fmt.Sprintf("client %q not found in any inbound or client record", email)) } needRestart := false + var delErrs []error for _, ibId := range inboundIds { nr, delErr := s.DelInboundClientByEmail(inboundSvc, ibId, email, false, true) if delErr != nil { if errors.Is(delErr, ErrClientNotInInbound) { continue } - return needRestart, delErr + delErrs = append(delErrs, fmt.Errorf("inbound %d: %w", ibId, delErr)) + continue } if nr { needRestart = true } } + if len(delErrs) > 0 { + return needRestart, errors.Join(delErrs...) + } if !keepTraffic { db := database.GetDB() if err := db.Where("email = ?", email).Delete(&xray.ClientTraffic{}).Error; err != nil { diff --git a/internal/web/service/client_delete_continue_test.go b/internal/web/service/client_delete_continue_test.go new file mode 100644 index 000000000..a0d549b5e --- /dev/null +++ b/internal/web/service/client_delete_continue_test.go @@ -0,0 +1,64 @@ +package service + +import ( + "strconv" + "strings" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +func TestDeleteContinuesPastFailedInbound(t *testing.T) { + setupBulkDB(t) + svc := &ClientService{} + inboundSvc := &InboundService{} + + source := []model.Client{{Email: "spread@x", ID: "cccccccc-0000-0000-0000-000000000001", SubID: "sub-spread", Enable: true}} + ib1 := mkInbound(t, 23001, model.VLESS, clientsSettings(t, source)) + ib2 := mkInbound(t, 23002, model.VLESS, clientsSettings(t, source)) + ib3 := mkInbound(t, 23003, model.VLESS, clientsSettings(t, source)) + for _, ib := range []*model.Inbound{ib1, ib2, ib3} { + if err := svc.SyncInbound(nil, ib.Id, source); err != nil { + t.Fatalf("seed linkage for %d: %v", ib.Id, err) + } + } + rec := lookupClientRecord(t, "spread@x") + + missingNode := 9999 + if err := database.GetDB().Model(&model.Inbound{}).Where("id = ?", ib2.Id). + Update("node_id", missingNode).Error; err != nil { + t.Fatalf("point inbound 2 at a missing node: %v", err) + } + + _, err := svc.Delete(inboundSvc, rec.Id, false) + if err == nil { + t.Fatalf("Delete with a failing inbound succeeded, want error") + } + if !strings.Contains(err.Error(), "inbound "+strconv.Itoa(ib2.Id)) { + t.Fatalf("Delete error = %q, want it to name inbound %d", err, ib2.Id) + } + + for _, ib := range []*model.Inbound{ib1, ib3} { + if settingsHoldUUID(t, inboundSvc, ib.Id, "spread@x") { + t.Fatalf("inbound %d still holds the client after Delete", ib.Id) + } + } + if _, err := svc.GetByID(rec.Id); err != nil { + t.Fatalf("record removed despite a failed inbound: %v", err) + } + + if err := database.GetDB().Model(&model.Inbound{}).Where("id = ?", ib2.Id). + Update("node_id", nil).Error; err != nil { + t.Fatalf("repair inbound 2: %v", err) + } + if _, err := svc.Delete(inboundSvc, rec.Id, false); err != nil { + t.Fatalf("retry Delete: %v", err) + } + if _, err := svc.GetByID(rec.Id); err == nil { + t.Fatalf("record still present after successful retry") + } + if settingsHoldUUID(t, inboundSvc, ib2.Id, "spread@x") { + t.Fatalf("inbound 2 still holds the client after retry") + } +}