From b1fa76f9b684b350aafd1349dfe193199f07539a Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sun, 5 Jul 2026 20:28:26 +0200 Subject: [PATCH] fix(node): fully delete clients on nodes instead of only detaching them Deleting a client on the master propagated to nodes via the detach endpoint, which removes the client from that one inbound's settings but deliberately keeps the client record. The node ended up with an orphaned record that kept showing in its Clients view; the master and node could never converge on a delete. Full-delete and detach intent now travel separately: the Runtime interface gains DeleteClient, which on Remote hits the node's panel/api/clients/del endpoint (record, attachments, traffic; repeat calls for a client on several inbounds of the same node are swallowed as idempotent "not found"). Delete/DeleteByEmail/BulkDelete use it for node inbounds, while Detach/BulkDetach keep the inbound-scoped detach RPC so removing a client from one inbound never wipes it node-wide (the #5543 guarantee is preserved and covered by tests). Bulk deletes above the fold threshold still converge membership via reconcile; their leftover node records can be cleaned with the node's delete-orphans action. Closes #5797 --- internal/web/runtime/local.go | 4 ++ internal/web/runtime/remote.go | 16 +++++ internal/web/runtime/runtime.go | 7 ++ internal/web/service/client_bulk.go | 5 +- internal/web/service/client_crud.go | 6 +- .../service/client_delete_node_full_test.go | 66 +++++++++++++++++++ internal/web/service/client_inbound_apply.go | 14 +++- .../web/service/client_wireguard_crud_test.go | 2 +- .../service/del_shared_email_runtime_test.go | 2 +- .../web/service/node_bulk_dispatch_test.go | 12 +++- internal/web/service/node_dirty_test.go | 2 +- .../web/service/sync_scale_postgres_test.go | 2 +- 12 files changed, 124 insertions(+), 14 deletions(-) create mode 100644 internal/web/service/client_delete_node_full_test.go diff --git a/internal/web/runtime/local.go b/internal/web/runtime/local.go index be5a960d8..48af05466 100644 --- a/internal/web/runtime/local.go +++ b/internal/web/runtime/local.go @@ -130,6 +130,10 @@ func (l *Local) DeleteUser(ctx context.Context, ib *model.Inbound, email string) return nil } +func (l *Local) DeleteClient(context.Context, string) error { + return nil +} + func (l *Local) UpdateUser(ctx context.Context, ib *model.Inbound, oldEmail string, payload model.Client) error { if oldEmail != "" { if err := l.RemoveUser(ctx, ib, oldEmail); err != nil && !strings.Contains(err.Error(), "not found") { diff --git a/internal/web/runtime/remote.go b/internal/web/runtime/remote.go index d6d0faf1b..3f529e070 100644 --- a/internal/web/runtime/remote.go +++ b/internal/web/runtime/remote.go @@ -553,6 +553,22 @@ func (r *Remote) DeleteUser(ctx context.Context, ib *model.Inbound, email string return err } +func (r *Remote) DeleteClient(ctx context.Context, email string) error { + if email == "" { + return nil + } + _, err := r.do(ctx, http.MethodPost, + "panel/api/clients/del/"+url.PathEscape(email), nil) + if err == nil { + return nil + } + var apiErr *remoteAPIError + if errors.As(err, &apiErr) && strings.Contains(strings.ToLower(apiErr.msg), "not found") { + return nil + } + return err +} + func (r *Remote) UpdateUser(ctx context.Context, ib *model.Inbound, oldEmail string, payload model.Client) error { if oldEmail == "" { oldEmail = payload.Email diff --git a/internal/web/runtime/runtime.go b/internal/web/runtime/runtime.go index fa3832045..d0540055c 100644 --- a/internal/web/runtime/runtime.go +++ b/internal/web/runtime/runtime.go @@ -23,6 +23,13 @@ type Runtime interface { DeleteUser(ctx context.Context, ib *model.Inbound, email string) error AddClient(ctx context.Context, ib *model.Inbound, client model.Client) error + // DeleteClient removes the client identified by email entirely from the + // runtime's own store: on Remote it hits the node's full-delete endpoint + // (record, attachments, traffic), unlike DeleteUser which only detaches + // from one inbound and leaves the node's client record behind. Local has + // no client store of its own, so it is a no-op there. + DeleteClient(ctx context.Context, email string) error + RestartXray(ctx context.Context) error ResetClientTraffic(ctx context.Context, ib *model.Inbound, email string) error diff --git a/internal/web/service/client_bulk.go b/internal/web/service/client_bulk.go index 97512c041..e3305c8d2 100644 --- a/internal/web/service/client_bulk.go +++ b/internal/web/service/client_bulk.go @@ -1067,9 +1067,12 @@ func (s *ClientService) bulkDelInboundClients( push = false } if push { + // bulkDelInboundClients only runs for full client deletion + // (BulkDelete), so the node must drop its client record too, + // not just detach from this inbound (#5797). pushFailed := false for email := range foundEmails { - if err1 := rt.DeleteUser(context.Background(), oldInbound, email); err1 != nil { + if err1 := rt.DeleteClient(context.Background(), email); err1 != nil { logger.Warning("Error in deleting client on", rt.Name(), ":", err1) pushFailed = true } diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index 1171cfa4a..a1b85a2a5 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -451,7 +451,7 @@ func (s *ClientService) Delete(inboundSvc *InboundService, id int, keepTraffic b if existing.Email == "" { continue } - nr, delErr := s.DelInboundClientByEmail(inboundSvc, ibId, existing.Email, false) + nr, delErr := s.DelInboundClientByEmail(inboundSvc, ibId, existing.Email, false, true) if delErr != nil { // The client is already absent from this inbound (data drift or a // retried delete). Skip it — deletion stays idempotent. @@ -609,7 +609,7 @@ func (s *ClientService) DeleteByEmail(inboundSvc *InboundService, email string, } needRestart := false for _, ibId := range inboundIds { - nr, delErr := s.DelInboundClientByEmail(inboundSvc, ibId, email, false) + nr, delErr := s.DelInboundClientByEmail(inboundSvc, ibId, email, false, true) if delErr != nil { if errors.Is(delErr, ErrClientNotInInbound) { continue @@ -672,7 +672,7 @@ func (s *ClientService) Detach(inboundSvc *InboundService, id int, inboundIds [] if existing.Email == "" { continue } - nr, delErr := s.DelInboundClientByEmail(inboundSvc, ibId, existing.Email, true) + nr, delErr := s.DelInboundClientByEmail(inboundSvc, ibId, existing.Email, true, false) if delErr != nil { if errors.Is(delErr, ErrClientNotInInbound) { continue diff --git a/internal/web/service/client_delete_node_full_test.go b/internal/web/service/client_delete_node_full_test.go new file mode 100644 index 000000000..377b95b1b --- /dev/null +++ b/internal/web/service/client_delete_node_full_test.go @@ -0,0 +1,66 @@ +package service + +import ( + "testing" + + "github.com/google/uuid" + + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// A full client delete must reach the node as the full-delete RPC so the node +// drops its own client record too — the detach RPC leaves an orphaned record +// that keeps showing in the node's client list (#5797). +func TestDelete_NodeClientDispatchesFullDeleteRPC(t *testing.T) { + setupBulkDB(t) + nodeID, fake := setupNodeRuntime(t) + + clients := []model.Client{{ID: uuid.NewString(), Email: "full-del@x", Enable: true}} + nodeInbound(t, nodeID, 32001, clients) + + svc := &ClientService{} + inboundSvc := &InboundService{} + + rec, err := svc.GetRecordByEmail(nil, "full-del@x") + if err != nil { + t.Fatalf("GetRecordByEmail: %v", err) + } + if _, err := svc.Delete(inboundSvc, rec.Id, false); err != nil { + t.Fatalf("Delete: %v", err) + } + + if got := fake.deleteClient.Load(); got != 1 { + t.Fatalf("full delete dispatched %d DeleteClient RPCs, want 1", got) + } + if got := fake.deleteUser.Load(); got != 0 { + t.Fatalf("full delete dispatched %d DeleteUser (detach) RPCs, want 0", got) + } +} + +// A plain detach must stay scoped to the one inbound via the detach RPC and +// never escalate to the node-wide full delete. +func TestDetach_NodeClientStaysOnDetachRPC(t *testing.T) { + setupBulkDB(t) + nodeID, fake := setupNodeRuntime(t) + + clients := []model.Client{{ID: uuid.NewString(), Email: "detach-me@x", Enable: true}} + ib := nodeInbound(t, nodeID, 32002, clients) + + svc := &ClientService{} + inboundSvc := &InboundService{} + + rec, err := svc.GetRecordByEmail(nil, "detach-me@x") + if err != nil { + t.Fatalf("GetRecordByEmail: %v", err) + } + if _, err := svc.Detach(inboundSvc, rec.Id, []int{ib.Id}); err != nil { + t.Fatalf("Detach: %v", err) + } + + if got := fake.deleteUser.Load(); got != 1 { + t.Fatalf("detach dispatched %d DeleteUser RPCs, want 1", got) + } + if got := fake.deleteClient.Load(); got != 0 { + t.Fatalf("detach dispatched %d DeleteClient RPCs, want 0", got) + } +} diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 7303c4364..ad38760ae 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -831,7 +831,7 @@ func (s *ClientService) UpdateInboundClient(inboundSvc *InboundService, data *mo return needRestart, nil } -func (s *ClientService) DelInboundClientByEmail(inboundSvc *InboundService, inboundId int, email string, keepTraffic bool) (bool, error) { +func (s *ClientService) DelInboundClientByEmail(inboundSvc *InboundService, inboundId int, email string, keepTraffic bool, fullDelete bool) (bool, error) { defer lockInbound(inboundId).Unlock() oldInbound, err := inboundSvc.GetInbound(inboundId) @@ -972,9 +972,17 @@ func (s *ClientService) DelInboundClientByEmail(inboundSvc *InboundService, inbo } else { // Node inbound: propagate the delete regardless of the enable flag — // the node's own DB still carries a disabled client and would - // resurrect it on the next snapshot otherwise. + // resurrect it on the next snapshot otherwise. A full client delete + // must remove the node's client record too, not just detach it from + // this inbound (#5797). if push { - if err1 := rt.DeleteUser(context.Background(), oldInbound, email); err1 != nil { + var err1 error + if fullDelete { + err1 = rt.DeleteClient(context.Background(), email) + } else { + err1 = rt.DeleteUser(context.Background(), oldInbound, email) + } + if err1 != nil { logger.Warning("Error in deleting client on", rt.Name(), ":", err1) } else { advancePushedInbound(rt, prevSettings, oldInbound) diff --git a/internal/web/service/client_wireguard_crud_test.go b/internal/web/service/client_wireguard_crud_test.go index 8d2e1b1ae..4db0ae543 100644 --- a/internal/web/service/client_wireguard_crud_test.go +++ b/internal/web/service/client_wireguard_crud_test.go @@ -80,7 +80,7 @@ func TestWireGuardClientAddUpdateDeleteRoundTrip(t *testing.T) { t.Fatalf("settings lost wg fields after metadata edit: %+v", listAfter) } - if _, err := svc.DelInboundClientByEmail(inboundSvc, ib.Id, "alice@wg", false); err != nil { + if _, err := svc.DelInboundClientByEmail(inboundSvc, ib.Id, "alice@wg", false, false); err != nil { t.Fatalf("DelInboundClientByEmail: %v", err) } final, err := svc.ListForInbound(nil, ib.Id) diff --git a/internal/web/service/del_shared_email_runtime_test.go b/internal/web/service/del_shared_email_runtime_test.go index a83a22e1a..504732dbc 100644 --- a/internal/web/service/del_shared_email_runtime_test.go +++ b/internal/web/service/del_shared_email_runtime_test.go @@ -25,7 +25,7 @@ func TestDelInboundClientByEmail_SharedEmailStillRemovesFromRuntime(t *testing.T svc := &ClientService{} inboundSvc := &InboundService{} - if _, err := svc.DelInboundClientByEmail(inboundSvc, ibA.Id, "shared@x", false); err != nil { + if _, err := svc.DelInboundClientByEmail(inboundSvc, ibA.Id, "shared@x", false, false); err != nil { t.Fatalf("DelInboundClientByEmail: %v", err) } diff --git a/internal/web/service/node_bulk_dispatch_test.go b/internal/web/service/node_bulk_dispatch_test.go index f52509fa2..8069ffc31 100644 --- a/internal/web/service/node_bulk_dispatch_test.go +++ b/internal/web/service/node_bulk_dispatch_test.go @@ -16,9 +16,10 @@ import ( // fakeNodeRuntime is a runtime.Runtime stub that counts the per-client dispatch // calls so a test can assert a bulk op does NOT stream one RPC per client. type fakeNodeRuntime struct { - addClient atomic.Int32 - deleteUser atomic.Int32 - updateUser atomic.Int32 + addClient atomic.Int32 + deleteUser atomic.Int32 + deleteClient atomic.Int32 + updateUser atomic.Int32 } func (f *fakeNodeRuntime) Name() string { return "fake-node" } @@ -40,6 +41,11 @@ func (f *fakeNodeRuntime) DeleteUser(context.Context, *model.Inbound, string) er return nil } +func (f *fakeNodeRuntime) DeleteClient(context.Context, string) error { + f.deleteClient.Add(1) + return nil +} + func (f *fakeNodeRuntime) AddClient(context.Context, *model.Inbound, model.Client) error { f.addClient.Add(1) return nil diff --git a/internal/web/service/node_dirty_test.go b/internal/web/service/node_dirty_test.go index 0de264114..26f89d6e3 100644 --- a/internal/web/service/node_dirty_test.go +++ b/internal/web/service/node_dirty_test.go @@ -99,7 +99,7 @@ func TestDelInboundClientByEmail_DisabledNodeClientMarksDirty(t *testing.T) { inboundSvc := &InboundService{} clientSvc := &ClientService{} - if _, err := clientSvc.DelInboundClientByEmail(inboundSvc, central.Id, "a@x", false); err != nil { + if _, err := clientSvc.DelInboundClientByEmail(inboundSvc, central.Id, "a@x", false, false); err != nil { t.Fatalf("DelInboundClientByEmail: %v", err) } diff --git a/internal/web/service/sync_scale_postgres_test.go b/internal/web/service/sync_scale_postgres_test.go index 9b111c32f..bf69e2325 100644 --- a/internal/web/service/sync_scale_postgres_test.go +++ b/internal/web/service/sync_scale_postgres_test.go @@ -201,7 +201,7 @@ func TestAddDelClientPostgresScale(t *testing.T) { delEmail := clients[n/2].Email start = time.Now() - if _, err := svc.DelInboundClientByEmail(inboundSvc, ib.Id, delEmail, false); err != nil { + if _, err := svc.DelInboundClientByEmail(inboundSvc, ib.Id, delEmail, false, false); err != nil { t.Fatalf("DelInboundClientByEmail: %v", err) } delDur := time.Since(start)