From 8b525b5be621dc7773e55feee3e6afc7e52d8d5f Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sun, 1 Nov 2026 13:30:53 +0100 Subject: [PATCH] fix(node): clear a client's orphan mark when it is attached again A client with an inbound link is not an orphan, but the node sweep's mark was cleared only at the end of a node merge. An admin attach left a stale mark, and ReapSyncOrphans deleted the client at its next detach, traffic and IPs included, without the grace window. Inserting a link now clears the mark of the clients it links, and the reaper rereads its candidates inside its writer, so an attach that commits after the list was read is not reaped. --- internal/web/service/client_link.go | 12 +++++ internal/web/service/client_sync_orphan.go | 30 ++++++++---- .../web/service/client_sync_orphan_test.go | 46 +++++++++++++++++++ 3 files changed, 80 insertions(+), 8 deletions(-) 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) + } +}