diff --git a/internal/web/service/client_link.go b/internal/web/service/client_link.go index fe69c523b..141e84c3c 100644 --- a/internal/web/service/client_link.go +++ b/internal/web/service/client_link.go @@ -387,6 +387,18 @@ func (s *ClientService) reconcileInboundLinks(tx *gorm.DB, inboundId int, wanted }).CreateInBatches(toInsert, 200).Error; err != nil { return nil, err } + // Not left to the next node merge: there may never be one, and a mark that outlives the + // attach hands the client to the reaper on its next detach. + linked := make([]int, 0, len(toInsert)) + for _, link := range toInsert { + linked = append(linked, link.ClientId) + } + for _, batch := range chunkInts(linked, sqlInChunk) { + if err := tx.Model(&model.ClientRecord{}).Where("id IN ? AND sync_orphaned_at > 0", batch). + Updates(map[string]any{"sync_orphaned_at": 0, "sync_orphaned_by": 0}).Error; err != nil { + return nil, err + } + } } relinked := append([]int(nil), toDelete...) diff --git a/internal/web/service/client_sync_orphan.go b/internal/web/service/client_sync_orphan.go index 4aa8d4275..b1a9a17aa 100644 --- a/internal/web/service/client_sync_orphan.go +++ b/internal/web/service/client_sync_orphan.go @@ -12,7 +12,7 @@ import ( ) // How long a client stays recoverable after a node merge concluded it is gone. -// Any merge that sees it attached again inside this window clears the mark. +// Attaching it again inside this window, by a merge or an admin, clears the mark. const syncOrphanReapGrace = 15 * time.Minute // A traffic row with no clients row behind it is drift the orphan sweep will @@ -42,17 +42,20 @@ func clearSyncOrphanMarks(tx *gorm.DB) error { Updates(map[string]any{"sync_orphaned_at": 0, "sync_orphaned_by": 0}).Error } -// ReapSyncOrphans deletes the clients the node-snapshot sweep marked and no -// later merge reclaimed. It is the only path that hard-deletes for that sweep. +func reapableSyncOrphans(tx *gorm.DB, cutoff int64) *gorm.DB { + return tx.Model(&model.ClientRecord{}). + Where("sync_orphaned_at > 0 AND sync_orphaned_at <= ?", cutoff). + Where("NOT EXISTS (SELECT 1 FROM client_inbounds WHERE client_inbounds.client_id = clients.id)") +} + +// ReapSyncOrphans deletes the clients the node-snapshot sweep marked and nothing +// attached again since. It is the only path that hard-deletes for that sweep. func (s *ClientService) ReapSyncOrphans() (int, error) { db := database.GetDB() cutoff := time.Now().Add(-syncOrphanReapGrace).UnixMilli() var emails []string - if err := db.Model(&model.ClientRecord{}). - Where("sync_orphaned_at > 0 AND sync_orphaned_at <= ?", cutoff). - Where("NOT EXISTS (SELECT 1 FROM client_inbounds WHERE client_inbounds.client_id = clients.id)"). - Pluck("email", &emails).Error; err != nil { + if err := reapableSyncOrphans(db, cutoff).Pluck("email", &emails).Error; err != nil { return 0, err } if len(emails) == 0 { @@ -60,8 +63,16 @@ func (s *ClientService) ReapSyncOrphans() (int, error) { } reaped := 0 - for _, batch := range chunkStrings(emails, sqlInChunk) { + for _, listed := range chunkStrings(emails, sqlInChunk) { + var batch []string if err := runSerializedTx(func(tx *gorm.DB) error { + // The list was read outside the writer; an attach may have committed since. + if err := reapableSyncOrphans(tx, cutoff).Where("email IN ?", listed).Pluck("email", &batch).Error; err != nil { + return err + } + if len(batch) == 0 { + return nil + } if err := adjustGroupBaselinesForRemovedTraffic(tx, batch); err != nil { return err } @@ -78,6 +89,9 @@ func (s *ClientService) ReapSyncOrphans() (int, error) { }); err != nil { return reaped, err } + if len(batch) == 0 { + continue + } reaped += len(batch) logger.Infof("reaped %d client(s) confirmed removed on their node", len(batch)) } diff --git a/internal/web/service/client_sync_orphan_test.go b/internal/web/service/client_sync_orphan_test.go index 1a0a1b8ff..4d1eb3488 100644 --- a/internal/web/service/client_sync_orphan_test.go +++ b/internal/web/service/client_sync_orphan_test.go @@ -155,3 +155,49 @@ func TestReapSyncOrphansIgnoresUnmarkedOrphans(t *testing.T) { t.Fatalf("unmarked orphan was reaped: %d rows survive, want 1", surviving) } } + +// With its only node silent no merge runs, so an admin's attach has to clear the mark +// itself, or the client's next detach is reaped instead of kept for manual cleanup. +func TestAdminAttachClearsSyncOrphanMark(t *testing.T) { + db := initTrafficTestDB(t) + svc := &InboundService{} + clientSvc := &ClientService{} + + seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true}) + + const email = "rescued@x" + const keep = "keep@x" + createNodeInboundWithClient(t, db, 1, "n1-in", 41001, keep) + bothSettings := fmt.Sprintf(`{"clients":[{"email":%q,"enable":true},{"email":%q,"enable":true}]}`, keep, email) + syncNodeWithSettings(t, svc, 1, "n1-in", bothSettings, + xray.ClientTraffic{Email: keep, Enable: true}, + xray.ClientTraffic{Email: email, Enable: true}) + syncNodeWithSettings(t, svc, 1, "n1-in", fmt.Sprintf(`{"clients":[{"email":%q,"enable":true}]}`, keep), + xray.ClientTraffic{Email: keep, Enable: true}) + if readOrphanMark(t, db, email) <= 0 { + t.Fatal("setup: expected the merge to mark the client") + } + backdateOrphanMark(t, db, email) + + master := &model.Inbound{UserId: 1, Tag: "master-in", Enable: true, Port: 40001, Protocol: model.VLESS, Settings: `{"clients":[]}`} + if err := db.Create(master).Error; err != nil { + t.Fatalf("create master inbound: %v", err) + } + if err := clientSvc.ApplyInboundClientDelta(db, master.Id, []model.Client{{Email: email, Enable: true}}, nil); err != nil { + t.Fatalf("attach: %v", err) + } + if at := readOrphanMark(t, db, email); at != 0 { + t.Fatalf("attached client kept its orphan mark: sync_orphaned_at=%d", at) + } + + if err := clientSvc.ApplyInboundClientDelta(db, master.Id, nil, []string{email}); err != nil { + t.Fatalf("detach: %v", err) + } + reaped, err := clientSvc.ReapSyncOrphans() + if err != nil { + t.Fatalf("reap: %v", err) + } + if rec, _ := countClientRows(t, db, email); reaped != 0 || rec != 1 { + t.Fatalf("admin-detached client was reaped: reaped=%d clients=%d, want 0/1", reaped, rec) + } +}