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>
This commit is contained in:
Yuriy Khachaturian
2026-10-05 16:54:44 +03:00
parent 2fc4dc7b42
commit 23c51074a2
4 changed files with 32 additions and 80 deletions
+12 -16
View File
@@ -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{}{}