From 23c51074a2675445ae149f8ee4faf3385e44408c Mon Sep 17 00:00:00 2001 From: Yuriy Khachaturian Date: Mon, 5 Oct 2026 16:54:44 +0300 Subject: [PATCH] fix(node): keep hub settings on an empty node snapshot; address review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The empty-snapshot guard skipped only the link rebuild and orphan sweep, but Phase A had already adopted the node's `{"clients":[]}` blob into `inbounds.settings`. Reconcile builds each push from that blob, so the hub kept re-pushing an empty client list and a reset/restarted node never got its clients back — the guard fired forever. Phase A now refuses to blank the settings of an inbound the hub still populates, so the hub stays authoritative and reconcile re-pushes the real clients (recovery). A node-side removal of the last client is therefore hub-authoritative by design; the partial-snapshot path still prunes an inbound that reports other clients. - test: assert the inbound's settings survive an empty snapshot (fails without the Phase A fix). - drop TestSetRemoteTraffic_EmptySnapshotSurvivesReap (no branch the core test doesn't already cover) and the duplicate orphanMark helper (readOrphanMark already exists); trim the comment blocks to the 2-line CLAUDE.md limit. - fix a pre-existing -race/-shuffle flake: TestGetAmneziaWGLogs owned no DB and relied on the ambient global one, which a sibling's dbtest cleanup closes under shuffle, panicking on nil in amneziawgLogActivity. It now owns a throwaway DB. Co-Authored-By: Claude Opus 4.8 --- .../web/service/client_sync_orphan_test.go | 12 +--- .../web/service/inbound_amneziawg_test.go | 3 + internal/web/service/inbound_node.go | 28 ++++---- .../service/node_degraded_snapshot_test.go | 69 ++++--------------- 4 files changed, 32 insertions(+), 80 deletions(-) diff --git a/internal/web/service/client_sync_orphan_test.go b/internal/web/service/client_sync_orphan_test.go index d6e05a14a..1a0a1b8ff 100644 --- a/internal/web/service/client_sync_orphan_test.go +++ b/internal/web/service/client_sync_orphan_test.go @@ -30,12 +30,8 @@ func backdateOrphanMark(t *testing.T, db *gorm.DB, email string) { } } -// The merge must soft-orphan, not delete: everything stays recoverable until -// 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. +// A partial snapshot (node alive, still serving another client) authoritatively drops one; +// the merge soft-orphans, recoverable until the grace elapses and the reaper confirms it. func TestSyncOrphanSurvivesMergeUntilGraceElapses(t *testing.T) { db := initTrafficTestDB(t) svc := &InboundService{} @@ -92,10 +88,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 (partial snapshot) was never gone: clearing the mark // 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) { db := initTrafficTestDB(t) svc := &InboundService{} diff --git a/internal/web/service/inbound_amneziawg_test.go b/internal/web/service/inbound_amneziawg_test.go index bdf1d2106..cb0c65da7 100644 --- a/internal/web/service/inbound_amneziawg_test.go +++ b/internal/web/service/inbound_amneziawg_test.go @@ -287,6 +287,9 @@ func TestNormalizeAmneziaWGSettings_CanonicalizesClientAllowedIPs(t *testing.T) } func TestGetAmneziaWGLogs_ClampsCountAndFiltersEvents(t *testing.T) { + // GetAmneziaWGLogs appends peer handshake activity, which reads the DB; + // own a throwaway one so -shuffle can't leave us the global nil DB. + setupConflictDB(t) logger.InitLogger(logging.DEBUG) logger.Info("amneziawg: started interface awg1 for inbound 1") logger.Info("xray: unrelated line that must never show up here") diff --git a/internal/web/service/inbound_node.go b/internal/web/service/inbound_node.go index b49262bbe..0a740833f 100644 --- a/internal/web/service/inbound_node.go +++ b/internal/web/service/inbound_node.go @@ -1167,6 +1167,16 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi lifecycleLifted = true } if p.central.Settings != adoptedSettings { + // #6734: a zero-client snapshot must not blank an inbound the hub still + // populates, or reconcile re-pushes an empty blob and the node never recovers. + snapCopy := *p.snapIb + snapCopy.Settings = adoptedSettings + if blob, gcErr := s.GetClients(&snapCopy); gcErr == nil && len(blob) == 0 { + var hubAttached int64 + if err := tx.Table("client_inbounds").Where("inbound_id = ?", p.central.Id).Count(&hubAttached).Error; err == nil && hubAttached > 0 { + continue + } + } if err := tx.Model(model.Inbound{}). Where("id = ?", p.central.Id). Update("settings", adoptedSettings).Error; err != nil { @@ -1228,22 +1238,8 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi applyMasterClientLifecycle(&clients[i], existing, csPtr) 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. + // A degraded node (reset/restart/removal) reports zero clients for an inbound the + // hub populates; adopting it empties links and ReapSyncOrphans deletes shared clients (#6734). 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{}{} diff --git a/internal/web/service/node_degraded_snapshot_test.go b/internal/web/service/node_degraded_snapshot_test.go index 556a09183..5c74c6141 100644 --- a/internal/web/service/node_degraded_snapshot_test.go +++ b/internal/web/service/node_degraded_snapshot_test.go @@ -1,6 +1,7 @@ package service import ( + "strings" "testing" "github.com/mhsanaei/3x-ui/v3/internal/database/model" @@ -23,22 +24,8 @@ func linkCount(t *testing.T, db *gorm.DB, email string) int64 { 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. +// A degraded node reporting zero clients for an inbound the hub populates must +// keep its links and never orphan-mark, or SyncInbound/ReapSyncOrphans delete the row. func TestSetRemoteTraffic_EmptySnapshotKeepsClients(t *testing.T) { db := initTrafficTestDB(t) svc := &InboundService{} @@ -66,50 +53,22 @@ func TestSetRemoteTraffic_EmptySnapshotKeepsClients(t *testing.T) { 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 { + if at := readOrphanMark(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) + // The hub must keep the client in the inbound's settings, or reconcile re-pushes + // an empty blob to the node and the clients never come back (#6734). + var ib model.Inbound + if err := db.Where("tag = ?", "n1-in").First(&ib).Error; err != nil { + t.Fatalf("read central inbound: %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) + if !strings.Contains(ib.Settings, "svc@x") { + t.Fatalf("empty snapshot blanked the inbound settings: %q", ib.Settings) } } -// 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. +// The guard is narrow: a snapshot still carrying a client is authoritative, so a +// client the node really dropped is unlinked and orphan-marked; only all-empty is degraded. func TestSetRemoteTraffic_PartialSnapshotStillPrunes(t *testing.T) { db := initTrafficTestDB(t) svc := &InboundService{} @@ -140,7 +99,7 @@ func TestSetRemoteTraffic_PartialSnapshotStillPrunes(t *testing.T) { 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 { + if at := readOrphanMark(t, db, "drop@x"); at <= 0 { t.Fatalf("partial snapshot did not orphan-mark the removed client: sync_orphaned_at=%d, want >0", at) } }