diff --git a/internal/web/service/client_bulk.go b/internal/web/service/client_bulk.go index 45030d466..3d0a6bbfd 100644 --- a/internal/web/service/client_bulk.go +++ b/internal/web/service/client_bulk.go @@ -1505,9 +1505,6 @@ func (s *ClientService) bulkCreate(inboundSvc *InboundService, payloads []Client skip(prep[idx].client.Email, reason[idx]) continue } - // The client is already live after fanout; never leave a stale delete - // tombstone merely because applying its optional HWID limit failed. - withdrawClientTombstones(prep[idx].client.Email) if err := s.setClientLimitHwidByEmail(prep[idx].client.Email, prep[idx].limitHwid); err != nil { skip(prep[idx].client.Email, err.Error()) continue @@ -1517,8 +1514,6 @@ func (s *ClientService) bulkCreate(inboundSvc *InboundService, payloads []Client inserted = append(inserted, prep[idx].payloadIdx) } } - // A re-created email is a live identity again: a delete tombstone left - // standing makes the next node merge prune the new client's inbound links. return result, inserted, needRestart, nil } diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index 4f6a8b839..2ff74b41f 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -256,9 +256,6 @@ func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreate // already existed, and a create the panel reported as failed must not. return needRestart, fanoutErr } - // A re-created email is a live identity again: a delete tombstone left - // standing makes the next node merge prune the new client's inbound links. - withdrawClientTombstones(client.Email) return needRestart, s.setClientLimitHwidByEmail(client.Email, payload.LimitHwid) } diff --git a/internal/web/service/client_delete_tombstone_test.go b/internal/web/service/client_delete_tombstone_test.go index 4d0513ca2..262177378 100644 --- a/internal/web/service/client_delete_tombstone_test.go +++ b/internal/web/service/client_delete_tombstone_test.go @@ -1,9 +1,12 @@ package service import ( + "errors" "fmt" "testing" + "gorm.io/gorm" + "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/model" "github.com/mhsanaei/3x-ui/v3/internal/web/runtime" @@ -104,3 +107,93 @@ func TestRecreatedClientSurvivesNodeMerge(t *testing.T) { t.Fatalf("re-created client has %d inbound links after the node merge, want 1: the delete tombstone outlived the email and the merge pruned it", links) } } + +// Re-adding a node inbound with a just-deleted email creates that client again +// through AddInbound, so the next merge must not prune it as the deleted one. +func TestReaddedNodeInboundKeepsRecreatedClient(t *testing.T) { + db := initTrafficTestDB(t) + inboundSvc := &InboundService{} + clientSvc := &ClientService{} + + const email = "tunnel-81@x" + seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true, InboundSyncMode: "all"}) + recreated := fmt.Sprintf(`{"id": "55555555-5555-4555-8555-555555555555", "email": %q, "enable": true}`, email) + nodeInbound := func(clients string) *model.Inbound { + nodeID := 1 + return &model.Inbound{ + UserId: 1, NodeID: &nodeID, Enable: true, Listen: "127.0.0.1", Port: 18453, Protocol: model.VLESS, + Settings: `{"clients": [` + clients + `], "decryption": "none"}`, + StreamSettings: `{"network": "tcp", "security": "none"}`, + } + } + + first, _, err := inboundSvc.AddInbound(nodeInbound(recreated)) + if err != nil { + t.Fatalf("add node inbound: %v", err) + } + if _, err := inboundSvc.DelInbound(first.Id); err != nil { + t.Fatalf("delete node inbound: %v", err) + } + t.Cleanup(func() { withdrawClientTombstones(email) }) + // inbounds/del only detaches; the record must go too or the re-add is a duplicate. + if _, err := clientSvc.DeleteByEmail(inboundSvc, email, false); err != nil { + t.Fatalf("delete detached client: %v", err) + } + // A second client keeps the tombstone-filtered snapshot non-empty, past the #6734 guard. + readded, _, err := inboundSvc.AddInbound(nodeInbound(recreated + `, {"id": "66666666-6666-4666-8666-666666666666", "email": "neighbour@x", "enable": true}`)) + if err != nil { + t.Fatalf("re-add node inbound: %v", err) + } + + // The next tick reconciles the push, clears dirty and merges a snapshot that has it. + if err := db.Model(&model.Node{}).Where("id = ?", 1).Update("config_dirty", false).Error; err != nil { + t.Fatalf("clear config_dirty: %v", err) + } + snap := &runtime.TrafficSnapshot{Inbounds: []*model.Inbound{{ + Tag: readded.Tag, Enable: true, Listen: readded.Listen, Port: readded.Port, Protocol: model.VLESS, + Settings: readded.Settings, StreamSettings: readded.StreamSettings, + ClientStats: []xray.ClientTraffic{{Email: email, Enable: true}, {Email: "neighbour@x", Enable: true}}, + }}} + if _, err := inboundSvc.setRemoteTrafficLocked(1, snap, false, true); err != nil { + t.Fatalf("node merge: %v", err) + } + + var rec model.ClientRecord + if err := db.Where("email = ?", email).First(&rec).Error; err != nil { + t.Fatalf("load re-created client: %v", err) + } + var links int64 + if err := db.Model(&model.ClientInbound{}). + Where("client_id = ? AND inbound_id = ?", rec.Id, readded.Id). + Count(&links).Error; err != nil { + t.Fatalf("count client links: %v", err) + } + if links != 1 || rec.SyncOrphanedAt != 0 { + t.Fatalf("after the merge the re-added client has %d links to its inbound and sync_orphaned_at=%d, want 1 and 0: the delete tombstone outlived the re-add and the merge pruned the client for ReapSyncOrphans", links, rec.SyncOrphanedAt) + } +} + +// The lift waits for the commit: a create that rolls back must leave the tombstone +// standing, or a snapshot taken before the delete resurrects the client. +func TestRolledBackCreateKeepsTombstone(t *testing.T) { + setupBulkDB(t) + const email = "rolled-back@x" + ib := mkInbound(t, 30451, model.VLESS, `{"clients": []}`) + tombstoneClientEmail(email) + t.Cleanup(func() { withdrawClientTombstones(email) }) + + errAbort := errors.New("abort after the client was created") + err := runSerializedTx(func(tx *gorm.DB) error { + created := []model.Client{{Email: email, ID: "77777777-7777-4777-8777-777777777777", Enable: true}} + if err := (&ClientService{}).SyncInbound(tx, ib.Id, created); err != nil { + return err + } + return errAbort + }) + if !errors.Is(err, errAbort) { + t.Fatalf("runSerializedTx error = %v, want the injected abort", err) + } + if !isClientEmailTombstoned(email) { + t.Fatal("a create that rolled back withdrew the delete tombstone, so a stale node snapshot can resurrect the deleted client") + } +} diff --git a/internal/web/service/client_link.go b/internal/web/service/client_link.go index 3af9b354e..61058b161 100644 --- a/internal/web/service/client_link.go +++ b/internal/web/service/client_link.go @@ -208,6 +208,9 @@ func (s *ClientService) syncInboundClients(tx *gorm.DB, inboundId int, clients [ if err := tx.CreateInBatches(toCreate, 200).Error; err != nil { return err } + // Whichever API re-created the client, a delete tombstone left standing makes + // the next node merge prune it and ReapSyncOrphans delete it. + liftTombstonesOnCommit(tx, toCreate) disabledIDs := make([]int, 0) for i, rec := range toCreate { idByEmail[rec.Email] = rec.Id diff --git a/internal/web/service/client_locks.go b/internal/web/service/client_locks.go index 3788f4be1..e77bfce6e 100644 --- a/internal/web/service/client_locks.go +++ b/internal/web/service/client_locks.go @@ -119,6 +119,18 @@ func withdrawClientTombstones(emails ...string) { } } +// liftTombstonesOnCommit withdraws the delete tombstones of clients a serialized +// tx re-created once it commits; a rolled-back create leaves them standing. +func liftTombstonesOnCommit(tx *gorm.DB, created []*model.ClientRecord) { + state := serializedTxOf(tx) + if state == nil { + return + } + for _, rec := range created { + state.createdClientEmails = append(state.createdClientEmails, rec.Email) + } +} + func tombstoneClientEmails(emails []string) { if len(emails) == 0 { return diff --git a/internal/web/service/traffic_writer.go b/internal/web/service/traffic_writer.go index 4cf4c3466..e8c43d7a4 100644 --- a/internal/web/service/traffic_writer.go +++ b/internal/web/service/traffic_writer.go @@ -127,19 +127,34 @@ func runTrafficWriter(ctx context.Context, queue chan *trafficWriteRequest, done // timeout. Apply runtime changes after this returns. func runSerializedTx(fn func(tx *gorm.DB) error) error { return submitTrafficWrite(func() error { - return database.GetDB().Transaction(func(tx *gorm.DB) error { - ctx := context.WithValue(tx.Statement.Context, serializedTxContextKey{}, true) + state := &serializedTx{} + err := database.GetDB().Transaction(func(tx *gorm.DB) error { + ctx := context.WithValue(tx.Statement.Context, serializedTxContextKey{}, state) return fn(tx.WithContext(ctx)) }) + if err == nil { + // Still on the writer, so no node merge runs between the commit and the lift. + withdrawClientTombstones(state.createdClientEmails...) + } + return err }) } -func isSerializedTx(tx *gorm.DB) bool { +// serializedTx carries what may only take effect once its transaction commits. +type serializedTx struct { + createdClientEmails []string +} + +func serializedTxOf(tx *gorm.DB) *serializedTx { if tx == nil || tx.Statement == nil || tx.Statement.Context == nil { - return false + return nil } - active, _ := tx.Statement.Context.Value(serializedTxContextKey{}).(bool) - return active + state, _ := tx.Statement.Context.Value(serializedTxContextKey{}).(*serializedTx) + return state +} + +func isSerializedTx(tx *gorm.DB) bool { + return serializedTxOf(tx) != nil } func safeApply(fn func() error) (err error) {