mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-10-18 03:45:02 +03:00
fix(node): don't delete clients when a node reports an empty snapshot
A node snapshot that comes back with zero clients for an inbound the hub still has clients on was treated as authoritative: SyncInbound strips every link for that inbound, the orphan sweep marks the now-linkless clients, and ReapSyncOrphans hard-deletes them once the grace period elapses. But a zero-client snapshot is indistinguishable from a degraded node — one that was just deleted, reset, restarted, or answered before its config loaded. On 2026-10-04 this deleted clients across the whole hub when a single node was removed. Treat a zero-client snapshot for an inbound that still has clients as non-authoritative: skip the link rebuild and the orphan sweep for that inbound (the same handling a failed SyncInbound already gets) and wait for a snapshot that carries clients. Removing a node's last client is done from the hub (which updates links and pushes); a node still serving other clients prunes a removed one through the existing partial path. The two orphan tests that drove removal via an empty snapshot now drive it via a partial snapshot (node alive, still serving another client), the authoritative path. New tests in node_degraded_snapshot_test.go cover the guard and its narrowness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -32,6 +32,10 @@ func backdateOrphanMark(t *testing.T, db *gorm.DB, email string) {
|
|||||||
|
|
||||||
// The merge must soft-orphan, not delete: everything stays recoverable until
|
// The merge must soft-orphan, not delete: everything stays recoverable until
|
||||||
// the grace period has elapsed and the reaper confirms nothing reclaimed it.
|
// the grace period has elapsed and the reaper confirms nothing reclaimed it.
|
||||||
|
// The removal is driven by a partial snapshot (the node is alive and still
|
||||||
|
// serves another client, so dropping one is authoritative); a fully empty
|
||||||
|
// snapshot is treated as a degraded node and never orphans — see
|
||||||
|
// TestSetRemoteTraffic_EmptySnapshotKeepsClients.
|
||||||
func TestSyncOrphanSurvivesMergeUntilGraceElapses(t *testing.T) {
|
func TestSyncOrphanSurvivesMergeUntilGraceElapses(t *testing.T) {
|
||||||
db := initTrafficTestDB(t)
|
db := initTrafficTestDB(t)
|
||||||
svc := &InboundService{}
|
svc := &InboundService{}
|
||||||
@@ -40,16 +44,20 @@ func TestSyncOrphanSurvivesMergeUntilGraceElapses(t *testing.T) {
|
|||||||
seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true})
|
seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true})
|
||||||
|
|
||||||
const email = "gone@x"
|
const email = "gone@x"
|
||||||
createNodeInboundWithClient(t, db, 1, "n1-in", 41001, email)
|
const keep = "keep@x"
|
||||||
settings := fmt.Sprintf(`{"clients":[{"email":%q,"enable":true}]}`, email)
|
createNodeInboundWithClient(t, db, 1, "n1-in", 41001, keep)
|
||||||
syncNodeWithSettings(t, svc, 1, "n1-in", settings,
|
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, Up: 5, Down: 5, Enable: true})
|
xray.ClientTraffic{Email: email, Up: 5, Down: 5, Enable: true})
|
||||||
|
|
||||||
if rec, traf := countClientRows(t, db, email); rec != 1 || traf != 1 {
|
if rec, traf := countClientRows(t, db, email); rec != 1 || traf != 1 {
|
||||||
t.Fatalf("setup: clients=%d client_traffics=%d, want 1/1", rec, traf)
|
t.Fatalf("setup: clients=%d client_traffics=%d, want 1/1", rec, traf)
|
||||||
}
|
}
|
||||||
|
|
||||||
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithoutClients(t, "n1-in"), false, false); err != nil {
|
keepOnly := fmt.Sprintf(`{"clients":[{"email":%q,"enable":true}]}`, keep)
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithClients(t, "n1-in", keepOnly,
|
||||||
|
xray.ClientTraffic{Email: keep, Enable: true}), false, false); err != nil {
|
||||||
t.Fatalf("orphaning merge: %v", err)
|
t.Fatalf("orphaning merge: %v", err)
|
||||||
}
|
}
|
||||||
if rec, traf := countClientRows(t, db, email); rec != 1 || traf != 1 {
|
if rec, traf := countClientRows(t, db, email); rec != 1 || traf != 1 {
|
||||||
@@ -86,6 +94,8 @@ func TestSyncOrphanSurvivesMergeUntilGraceElapses(t *testing.T) {
|
|||||||
|
|
||||||
// A client the node reports again was never gone: clearing the mark is what
|
// A client the node reports again was never gone: clearing the mark is what
|
||||||
// turns a bad merge into a recoverable blip instead of a delayed deletion.
|
// turns a bad merge into a recoverable blip instead of a delayed deletion.
|
||||||
|
// The drop is driven by a partial snapshot (node alive, still serving another
|
||||||
|
// client); a fully empty snapshot is a degraded node and never orphans.
|
||||||
func TestSyncOrphanMarkClearedOnReattach(t *testing.T) {
|
func TestSyncOrphanMarkClearedOnReattach(t *testing.T) {
|
||||||
db := initTrafficTestDB(t)
|
db := initTrafficTestDB(t)
|
||||||
svc := &InboundService{}
|
svc := &InboundService{}
|
||||||
@@ -94,19 +104,24 @@ func TestSyncOrphanMarkClearedOnReattach(t *testing.T) {
|
|||||||
seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true})
|
seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true})
|
||||||
|
|
||||||
const email = "flaky@x"
|
const email = "flaky@x"
|
||||||
createNodeInboundWithClient(t, db, 1, "n1-in", 41001, email)
|
const keep = "keep@x"
|
||||||
settings := fmt.Sprintf(`{"clients":[{"email":%q,"enable":true}]}`, email)
|
createNodeInboundWithClient(t, db, 1, "n1-in", 41001, keep)
|
||||||
syncNodeWithSettings(t, svc, 1, "n1-in", settings,
|
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, Up: 5, Down: 5, Enable: true})
|
xray.ClientTraffic{Email: email, Up: 5, Down: 5, Enable: true})
|
||||||
|
|
||||||
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithoutClients(t, "n1-in"), false, false); err != nil {
|
keepOnly := fmt.Sprintf(`{"clients":[{"email":%q,"enable":true}]}`, keep)
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithClients(t, "n1-in", keepOnly,
|
||||||
|
xray.ClientTraffic{Email: keep, Enable: true}), false, false); err != nil {
|
||||||
t.Fatalf("orphaning merge: %v", err)
|
t.Fatalf("orphaning merge: %v", err)
|
||||||
}
|
}
|
||||||
if readOrphanMark(t, db, email) <= 0 {
|
if readOrphanMark(t, db, email) <= 0 {
|
||||||
t.Fatal("setup: expected the merge to mark the client")
|
t.Fatal("setup: expected the merge to mark the client")
|
||||||
}
|
}
|
||||||
|
|
||||||
syncNodeWithSettings(t, svc, 1, "n1-in", settings,
|
syncNodeWithSettings(t, svc, 1, "n1-in", bothSettings,
|
||||||
|
xray.ClientTraffic{Email: keep, Enable: true},
|
||||||
xray.ClientTraffic{Email: email, Up: 6, Down: 6, Enable: true})
|
xray.ClientTraffic{Email: email, Up: 6, Down: 6, Enable: true})
|
||||||
|
|
||||||
if orphanedAt := readOrphanMark(t, db, email); orphanedAt != 0 {
|
if orphanedAt := readOrphanMark(t, db, email); orphanedAt != 0 {
|
||||||
|
|||||||
@@ -1228,6 +1228,27 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
|
|||||||
applyMasterClientLifecycle(&clients[i], existing, csPtr)
|
applyMasterClientLifecycle(&clients[i], existing, csPtr)
|
||||||
filtered = append(filtered, clients[i])
|
filtered = append(filtered, clients[i])
|
||||||
}
|
}
|
||||||
|
// Empty-snapshot guard. A node that comes back reporting zero clients for
|
||||||
|
// an inbound the hub still has clients on is indistinguishable from a
|
||||||
|
// genuine "every client was removed" — and in practice it is almost always
|
||||||
|
// the former: the node was just deleted, reset, restarted, or returned a
|
||||||
|
// snapshot before its config loaded. Acting on it is destructive and hard
|
||||||
|
// to undo: SyncInbound makes the inbound's links match the set exactly, so
|
||||||
|
// an empty set deletes every link for the inbound; a client shared across
|
||||||
|
// nodes then loses its links one node at a time and, once the last one
|
||||||
|
// goes, is orphan-marked and hard-deleted by ReapSyncOrphans — the
|
||||||
|
// 2026-10-04 outage, where removing one node deleted clients that still
|
||||||
|
// lived on the others. Treat a zero-client snapshot as non-authoritative:
|
||||||
|
// skip the link rebuild and the orphan sweep for this inbound (the same
|
||||||
|
// handling a failed SyncInbound gets) and wait for a snapshot that carries
|
||||||
|
// clients. Removing the last client from a node is instead done from the
|
||||||
|
// hub (which updates links and pushes); a node that still serves other
|
||||||
|
// clients prunes a removed one authoritatively through the partial path.
|
||||||
|
if len(filtered) == 0 && len(oldEmailsRows) > 0 {
|
||||||
|
logger.Warningf("setRemoteTraffic: node snapshot for tag %q reported zero clients while the hub has %d attached — treating as a degraded snapshot, skipping link rebuild and orphan sweep for this inbound", snapIb.Tag, len(oldEmailsRows))
|
||||||
|
syncFailedInbounds[c.Id] = struct{}{}
|
||||||
|
continue
|
||||||
|
}
|
||||||
localEmails := make([]string, 0, len(filtered))
|
localEmails := make([]string, 0, len(filtered))
|
||||||
for i := range filtered {
|
for i := range filtered {
|
||||||
if filtered[i].Email != "" {
|
if filtered[i].Email != "" {
|
||||||
|
|||||||
@@ -0,0 +1,146 @@
|
|||||||
|
package service
|
||||||
|
|
||||||
|
import (
|
||||||
|
"testing"
|
||||||
|
|
||||||
|
"github.com/mhsanaei/3x-ui/v3/internal/database/model"
|
||||||
|
"github.com/mhsanaei/3x-ui/v3/internal/xray"
|
||||||
|
|
||||||
|
"gorm.io/gorm"
|
||||||
|
)
|
||||||
|
|
||||||
|
// linkCount returns how many client_inbounds links a client currently has,
|
||||||
|
// across every inbound — the value ReapSyncOrphans checks before deleting.
|
||||||
|
func linkCount(t *testing.T, db *gorm.DB, email string) int64 {
|
||||||
|
t.Helper()
|
||||||
|
var n int64
|
||||||
|
if err := db.Table("client_inbounds").
|
||||||
|
Joins("JOIN clients ON clients.id = client_inbounds.client_id").
|
||||||
|
Where("clients.email = ?", email).
|
||||||
|
Count(&n).Error; err != nil {
|
||||||
|
t.Fatalf("count links for %q: %v", email, err)
|
||||||
|
}
|
||||||
|
return n
|
||||||
|
}
|
||||||
|
|
||||||
|
func orphanMark(t *testing.T, db *gorm.DB, email string) int64 {
|
||||||
|
t.Helper()
|
||||||
|
var at int64
|
||||||
|
if err := db.Model(&model.ClientRecord{}).Where("email = ?", email).
|
||||||
|
Pluck("sync_orphaned_at", &at).Error; err != nil {
|
||||||
|
t.Fatalf("read sync_orphaned_at %q: %v", email, err)
|
||||||
|
}
|
||||||
|
return at
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestSetRemoteTraffic_EmptySnapshotKeepsClients is the core guard: a managed
|
||||||
|
// node that comes back reporting zero clients for an inbound the hub still has
|
||||||
|
// clients on is treated as degraded (just deleted/reset/restarted, or a
|
||||||
|
// snapshot taken before the config loaded), not as "every client was removed".
|
||||||
|
// Its links must stay and nothing may be orphan-marked — otherwise SyncInbound
|
||||||
|
// would strip every link and ReapSyncOrphans would later hard-delete the row.
|
||||||
|
func TestSetRemoteTraffic_EmptySnapshotKeepsClients(t *testing.T) {
|
||||||
|
db := initTrafficTestDB(t)
|
||||||
|
svc := &InboundService{}
|
||||||
|
|
||||||
|
seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true})
|
||||||
|
createNodeInboundWithClient(t, db, 1, "n1-in", 41001, "svc@x")
|
||||||
|
|
||||||
|
settings := `{"clients":[{"email":"svc@x","enable":true}]}`
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithClients(t, "n1-in", settings,
|
||||||
|
xray.ClientTraffic{Email: "svc@x", Enable: true}), false, false); err != nil {
|
||||||
|
t.Fatalf("seed sync: %v", err)
|
||||||
|
}
|
||||||
|
if n := linkCount(t, db, "svc@x"); n != 1 {
|
||||||
|
t.Fatalf("setup: svc@x links=%d, want 1", n)
|
||||||
|
}
|
||||||
|
|
||||||
|
// The node returns an empty snapshot — the trigger that deleted real clients.
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithoutClients(t, "n1-in"), false, false); err != nil {
|
||||||
|
t.Fatalf("empty-snapshot sync: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if rec, _ := countClientRows(t, db, "svc@x"); rec != 1 {
|
||||||
|
t.Fatalf("empty snapshot deleted the client row: clients=%d, want 1", rec)
|
||||||
|
}
|
||||||
|
if n := linkCount(t, db, "svc@x"); n != 1 {
|
||||||
|
t.Fatalf("empty snapshot stripped the client link: links=%d, want 1", n)
|
||||||
|
}
|
||||||
|
if at := orphanMark(t, db, "svc@x"); at != 0 {
|
||||||
|
t.Fatalf("empty snapshot orphan-marked a live client: sync_orphaned_at=%d, want 0", at)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestSetRemoteTraffic_EmptySnapshotSurvivesReap closes the loop on the outage
|
||||||
|
// of 2026-10-04: without the guard an empty/degraded snapshot orphan-marks the
|
||||||
|
// inbound's clients and ReapSyncOrphans hard-deletes them after the grace
|
||||||
|
// period. The guard keeps them attached, so even a backdated reap can't take
|
||||||
|
// them — which is exactly what deleting one node must not do to the clients it
|
||||||
|
// served.
|
||||||
|
func TestSetRemoteTraffic_EmptySnapshotSurvivesReap(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})
|
||||||
|
createNodeInboundWithClient(t, db, 1, "n1-in", 41001, "svc@x")
|
||||||
|
|
||||||
|
settings := `{"clients":[{"email":"svc@x","enable":true}]}`
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithClients(t, "n1-in", settings,
|
||||||
|
xray.ClientTraffic{Email: "svc@x", Enable: true}), false, false); err != nil {
|
||||||
|
t.Fatalf("seed sync: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithoutClients(t, "n1-in"), false, false); err != nil {
|
||||||
|
t.Fatalf("empty-snapshot sync: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
backdateOrphanMark(t, db, "svc@x") // no-op if unmarked; proves reap can't take it
|
||||||
|
if reaped, err := clientSvc.ReapSyncOrphans(); err != nil {
|
||||||
|
t.Fatalf("reap: %v", err)
|
||||||
|
} else if reaped != 0 {
|
||||||
|
t.Fatalf("reaped %d client(s) off an empty snapshot, want 0", reaped)
|
||||||
|
}
|
||||||
|
if rec, _ := countClientRows(t, db, "svc@x"); rec != 1 {
|
||||||
|
t.Fatalf("empty snapshot + reap deleted the client: clients=%d, want 1", rec)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestSetRemoteTraffic_PartialSnapshotStillPrunes confirms the guard is narrow:
|
||||||
|
// a snapshot that still carries at least one client is authoritative, so a
|
||||||
|
// client the node really dropped is still unlinked and left for the orphan
|
||||||
|
// sweep. Only the all-empty snapshot is treated as degraded.
|
||||||
|
func TestSetRemoteTraffic_PartialSnapshotStillPrunes(t *testing.T) {
|
||||||
|
db := initTrafficTestDB(t)
|
||||||
|
svc := &InboundService{}
|
||||||
|
|
||||||
|
seedNodeRow(t, db, &model.Node{Id: 1, Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true})
|
||||||
|
createNodeInboundWithClient(t, db, 1, "n1-in", 41001, "keep@x")
|
||||||
|
|
||||||
|
bothSettings := `{"clients":[{"email":"keep@x","enable":true},{"email":"drop@x","enable":true}]}`
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithClients(t, "n1-in", bothSettings,
|
||||||
|
xray.ClientTraffic{Email: "keep@x", Enable: true},
|
||||||
|
xray.ClientTraffic{Email: "drop@x", Enable: true}), false, false); err != nil {
|
||||||
|
t.Fatalf("seed sync: %v", err)
|
||||||
|
}
|
||||||
|
if n := linkCount(t, db, "drop@x"); n != 1 {
|
||||||
|
t.Fatalf("setup: drop@x links=%d, want 1", n)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Node now reports only keep@x — drop@x was genuinely removed there.
|
||||||
|
keepOnlySettings := `{"clients":[{"email":"keep@x","enable":true}]}`
|
||||||
|
if _, err := svc.setRemoteTrafficLocked(1, snapshotWithClients(t, "n1-in", keepOnlySettings,
|
||||||
|
xray.ClientTraffic{Email: "keep@x", Enable: true}), false, false); err != nil {
|
||||||
|
t.Fatalf("partial-snapshot sync: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if n := linkCount(t, db, "keep@x"); n != 1 {
|
||||||
|
t.Fatalf("partial snapshot dropped a reported client: keep@x links=%d, want 1", n)
|
||||||
|
}
|
||||||
|
if n := linkCount(t, db, "drop@x"); n != 0 {
|
||||||
|
t.Fatalf("partial snapshot kept an unreported client linked: drop@x links=%d, want 0", n)
|
||||||
|
}
|
||||||
|
if at := orphanMark(t, db, "drop@x"); at <= 0 {
|
||||||
|
t.Fatalf("partial snapshot did not orphan-mark the removed client: sync_orphaned_at=%d, want >0", at)
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user