fix(node): don't delete clients when a node reports an empty snapshot (#6734)

* 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>

* fix(node): keep hub settings on an empty node snapshot; address review

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 <noreply@anthropic.com>

* fix(node): re-push hub clients to a node that reports an empty snapshot

23c51074 stopped the empty snapshot from blanking inbounds.settings, but
nothing re-sent those settings: the node was never marked dirty, and the
reconcile fingerprint from the last good push still matched, so
ReconcileInbound skipped the inbound. A reset node stayed empty while the
hub kept handing out links to it. The ClientStats sweep also deleted the
node baseline on that tick, so traffic used until recovery was lost.

The empty snapshot is now classified once, before adoption: its settings
and wire fields are not adopted, the ClientStats sweep, link rebuild and
orphan sweep skip it, the node is marked dirty and the inbound's pushed
fingerprint is dropped (Remote.ForgetPushedInbound) so reconcile re-sends
the hub's clients.

---------

Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
Co-authored-by: Yuriy Khachaturian <y.khachaturian@souzmult.ru>
This commit is contained in:
Yuri Khachaturyan
2026-10-05 17:12:08 +03:00
committed by GitHub
parent d4a7086c4e
commit 2c5fc8e72c
6 changed files with 284 additions and 13 deletions
+43 -1
View File
@@ -427,6 +427,20 @@ func adoptedWireInbound(c, snapIb *model.Inbound, adoptedSettings string) *model
return &a
}
// snapshotDropsEveryHubClient reports a node that lists no clients where the hub
// still links some: a reset or half-started node, never an authoritative removal.
func snapshotDropsEveryHubClient(tx *gorm.DB, inboundID int, wireSettings string) bool {
clients, err := ParseInboundSettingsClients(wireSettings)
if err != nil || len(clients) > 0 {
return false
}
var links int64
if err := tx.Table("client_inbounds").Where("inbound_id = ?", inboundID).Count(&links).Error; err != nil {
return false
}
return links > 0
}
// clientEmailsOwnedElsewhere returns the emails attached only to inbounds of
// other nodes: email is unique, so adopting one would overwrite a client this
// node does not serve. Attached nowhere means soft-orphaned, hence adoptable.
@@ -644,6 +658,7 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
wireSettings string
}
var pendingAdopts []pendingAdopt
degradedInbounds := map[int]string{}
newInboundIDs := make(map[int]struct{})
@@ -782,7 +797,9 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
adoptedSettings = deduped
}
updates := map[string]any{}
if !dirty {
if !dirty && snapshotDropsEveryHubClient(tx, c.Id, adoptedSettings) {
degradedInbounds[c.Id] = c.Tag
} else if !dirty {
// Defer lifecycle lift until after client_traffics absorbs this tick's
// deltas so quota stale-disable matches SQL (#6228).
pendingAdopts = append(pendingAdopts, pendingAdopt{
@@ -1121,6 +1138,9 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
if k.inboundID != c.Id {
continue
}
if _, degraded := degradedInbounds[c.Id]; degraded {
continue
}
if _, kept := snapEmails[k.email]; kept {
continue
}
@@ -1228,6 +1248,13 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
applyMasterClientLifecycle(&clients[i], existing, csPtr)
filtered = append(filtered, clients[i])
}
// 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 _, degraded := degradedInbounds[c.Id]; degraded {
logger.Warningf("setRemoteTraffic: node %d reported zero clients for tag %q while the hub has %d attached — keeping them and re-pushing", nodeID, snapIb.Tag, len(oldEmailsRows))
syncFailedInbounds[c.Id] = struct{}{}
continue
}
localEmails := make([]string, 0, len(filtered))
for i := range filtered {
if filtered[i].Email != "" {
@@ -1337,6 +1364,21 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
}
committed = true
if len(degradedInbounds) > 0 {
if mgr := runtime.GetManager(); mgr != nil {
if rt, rtErr := mgr.RuntimeFor(&nodeID); rtErr == nil {
if rem, ok := rt.(*runtime.Remote); ok {
for _, tag := range degradedInbounds {
rem.ForgetPushedInbound(tag)
}
}
}
}
if err := (&NodeService{}).MarkNodeDirty(nodeID); err != nil {
logger.Warningf("setRemoteTraffic: mark node %d dirty after an empty snapshot failed: %v", nodeID, err)
}
}
if lifecycleLifted && !dirty {
var already model.Node
if err := database.GetDB().Select("config_dirty").Where("id = ?", nodeID).First(&already).Error; err == nil && already.ConfigDirty {