fix(amneziawgnet): stop resetting live peer sessions on every reconcile tick

Real production bug, found via a live test connection that reset every ~10 seconds: ensureLocked's reconfigure-in-place branch called IpcSet unconditionally on every Ensure, including AmneziaWGJob's routine 10s reconcile tick even when nothing in the DB had changed. amneziawg-go's IpcSet always includes replace_peers=true (buildUAPIConfig), and its own handling of that op is device.RemoveAllPeers() -- unconditional, even when the new peer list is byte-identical to the old one. So every tick tore down and recreated every peer's live handshake/session state, and no connection could ever survive past one reconcile cycle. Root-caused with AMNEZIAWGNET_DEBUG (previous commit) showing "UAPI: Removing all peers" + peer Stopping/Starting exactly ~10s after a real handshake completed, matching AmneziaWGJob's own cadence precisely. Fixed by comparing the freshly rendered UAPI config string against what was last actually applied and skipping IpcSet entirely when identical -- reusing buildUAPIConfig's own exhaustive field coverage instead of a hand-maintained fingerprint that could drift out of sync with it. TestEnsureUnchangedInstanceDoesNotResetLivePeers verifies via device.LookupPeer pointer identity (confirmed to fail without this fix, not just pass trivially with it).
2026-09-06 18:27:14 +00:00 · 2026-08-04 01:46:32 +03:00
parent b0c29b7caa
commit 738163699e
2 changed files with 136 additions and 21 deletions
@@ -45,11 +45,12 @@ type Desired struct {
 // enough of its own configuration to decide whether a later Ensure call can
 // reconfigure it in place or needs to rebuild it from scratch.
 type managed struct {
-	dev      *Device
-	udpRelay *UDPRelay
-	peers    *PeerIndex
-	inst     amneziawg.Instance
-	structFP string
+	dev        *Device
+	udpRelay   *UDPRelay
+	peers      *PeerIndex
+	inst       amneziawg.Instance
+	structFP   string
+	uapiConfig string
 }

 // Manager owns the set of running embedded AmneziaWG interfaces, keyed by
@@ -89,17 +90,23 @@ func (m *Manager) Ensure(d Desired) error {
 }

 // ensureLocked decides between three actions: nothing changed since the
-// last apply (skip entirely); only peers/obfuscation/keys/listen_port
-// changed (reconfigure the existing Device in place via IpcSet, which
-// already sends replace_peers=true -- see buildUAPIConfig -- so removed
-// peers are dropped correctly without a full rebuild); or the interface's
-// own address(es)/MTU changed (these are fixed at netstack-construction
-// time, so the only option is closing the old Device and building a fresh
-// one). This is a coarser split than internal/amneziawg's own three-tier
-// noop/reload/restart fingerprinting (that one also tracks host-side
-// TPROXY/NDP rules this embedded path has no equivalent of) -- correct and
-// sufficient for Phase 1; revisit only if reconcile frequency at real scale
-// makes the address/MTU rebuild path worth avoiding too.
+// last apply (skip entirely -- this is the common case on every 10s
+// reconcile tick when no admin edit happened, and it MUST actually skip the
+// IpcSet call, not just look like it should: amneziawg-go's IpcSet always
+// includes replace_peers=true -- see buildUAPIConfig -- and its own
+// implementation of that op is device.RemoveAllPeers(), unconditionally,
+// even when the new peer list is byte-identical to the old one. A real
+// production bug, found via a live test connection that reset every ~10s:
+// calling IpcSet on every tick regardless of whether anything changed was
+// tearing down every peer's live handshake/session state on every single
+// reconcile, so no connection could ever survive past one tick); only
+// peers/obfuscation/keys/listen_port changed (reconfigure the existing
+// Device in place via IpcSet); or the interface's own address(es)/MTU
+// changed (these are fixed at netstack-construction time, so the only
+// option is closing the old Device and building a fresh one). This is a
+// coarser split than internal/amneziawg's own three-tier noop/reload/
+// restart fingerprinting (that one also tracks host-side TPROXY/NDP rules
+// this embedded path has no equivalent of) -- correct and sufficient here.
 func (m *Manager) ensureLocked(d Desired) error {
 	inst, opts := d.Instance, d.Options
 	if opts.Logger == nil {
@@ -123,11 +130,25 @@ func (m *Manager) ensureLocked(d Desired) error {
 		if err != nil {
 			return fmt.Errorf("amneziawgnet: %w", err)
 		}
+		// True no-op: the rendered UAPI config -- which already covers every
+		// field IpcSet can act on (keys, listen port, obfuscation, AWG 3.0
+		// options, the full peer list) -- is byte-identical to what's
+		// already live. Comparing the rendered string instead of inst
+		// directly means this can never drift out of sync with whatever
+		// buildUAPIConfig actually reads, the way a hand-maintained field
+		// list could.
+		if conf == cur.uapiConfig {
+			cur.peers = NewPeerIndex(inst.Peers)
+			cur.inst = inst
+			applyV6Aliases(diffV6Aliases(oldInst, inst))
+			return nil
+		}
 		if err := cur.dev.IpcSet(conf); err != nil {
 			return fmt.Errorf("amneziawgnet: reconfigure inbound %d: %w", inst.Id, err)
 		}
 		cur.peers = NewPeerIndex(inst.Peers)
 		cur.inst = inst
+		cur.uapiConfig = conf
 		applyV6Aliases(diffV6Aliases(oldInst, inst))
 		return nil
 	}
@@ -141,6 +162,13 @@ func (m *Manager) ensureLocked(d Desired) error {
 	if err != nil {
 		return err
 	}
+	// NewDevice already rendered and applied this exact config internally;
+	// recomputing it here (cheap, pure, guaranteed to succeed since
+	// NewDevice just proved these inputs are valid) is simpler than
+	// threading the string back out of NewDevice's own signature, and gives
+	// the no-op check above a correct baseline to compare the next tick
+	// against instead of an empty string.
+	conf, _ := buildUAPIConfig(inst, opts)

 	relay := socksRelayForInstance(inst)
 	udpRelay := NewUDPRelay(relay, dev.Stack)
@@ -180,11 +208,12 @@ func (m *Manager) ensureLocked(d Desired) error {
 	})

 	m.ifaces[inst.Id] = &managed{
-		dev:      dev,
-		udpRelay: udpRelay,
-		peers:    NewPeerIndex(inst.Peers),
-		inst:     inst,
-		structFP: structFP,
+		dev:        dev,
+		udpRelay:   udpRelay,
+		peers:      NewPeerIndex(inst.Peers),
+		inst:       inst,
+		structFP:   structFP,
+		uapiConfig: conf,
 	}
 	applyV6Aliases(diffV6Aliases(oldInst, inst))
 	logger.Infof("amneziawgnet: started embedded interface %s for inbound %d", inst.InterfaceName, inst.Id)
@@ -3,6 +3,8 @@ package amneziawgnet
 import (
 	"testing"

+	"github.com/amnezia-vpn/amneziawg-go/v3/device"
+
 	"github.com/mhsanaei/3x-ui/v3/internal/amneziawg"
 	"github.com/mhsanaei/3x-ui/v3/internal/util/wireguard"
 )
@@ -83,3 +85,87 @@ func TestManagerLifecycle(t *testing.T) {
 		t.Error("Lookup succeeded after Reconcile([]) removed the interface")
 	}
 }
+
+// TestEnsureUnchangedInstanceDoesNotResetLivePeers is a regression test for a
+// real production bug: an unchanged Ensure call (the common case on every
+// 10s AmneziaWGJob reconcile tick when no admin edit happened) was calling
+// IpcSet unconditionally. amneziawg-go's IpcSet always includes
+// replace_peers=true (see buildUAPIConfig), and its own implementation of
+// that op is device.RemoveAllPeers() -- unconditionally, even when the new
+// peer list is byte-identical to the old one. That tore down every peer's
+// live handshake/session state on every single reconcile tick, so no real
+// connection could ever survive past ~10 seconds. Caught via a live test
+// connection that reset every ~10s with amneziawg-go's own verbose logging
+// enabled (AMNEZIAWGNET_DEBUG) showing "UAPI: Removing all peers" +
+// peer "Stopping"/"Starting" on every tick.
+//
+// Verified here by comparing the *device.Peer pointer LookupPeer returns
+// before and after a no-op Ensure: identical pointer proves the peer object
+// itself survived (no RemoveAllPeers), not just that some higher-level
+// abstraction looks unchanged.
+func TestEnsureUnchangedInstanceDoesNotResetLivePeers(t *testing.T) {
+	priv, pub, err := wireguard.GenerateWireguardKeypair()
+	if err != nil {
+		t.Fatalf("generate server keypair: %v", err)
+	}
+	_, peerPub, err := wireguard.GenerateWireguardKeypair()
+	if err != nil {
+		t.Fatalf("generate peer keypair: %v", err)
+	}
+
+	m := &Manager{ifaces: map[int]*managed{}}
+	inst := amneziawg.Instance{
+		Id:            4,
+		InterfaceName: "awgtest4",
+		ListenPort:    58715,
+		PrivateKey:    priv,
+		PublicKey:     pub,
+		Address:       []string{"10.204.0.1/24"},
+		MTU:           1420,
+		Obfuscation: amneziawg.Obfuscation20{
+			Jc: 4, Jmin: 40, Jmax: 70,
+			S1: 20, S2: 30, S3: 20, S4: 20,
+		},
+		Peers: []amneziawg.Peer{
+			{Email: "peer@test", PublicKey: peerPub, AllowedIPs: []string{"10.204.0.2/32"}},
+		},
+	}
+	defer m.StopAll()
+
+	if err := m.Ensure(Desired{Instance: inst}); err != nil {
+		t.Fatalf("Ensure (create): %v", err)
+	}
+
+	peerPubHex, err := wireguard.KeyToHex(peerPub)
+	if err != nil {
+		t.Fatalf("KeyToHex: %v", err)
+	}
+	var npk device.NoisePublicKey
+	if err := npk.FromHex(peerPubHex); err != nil {
+		t.Fatalf("NoisePublicKey.FromHex: %v", err)
+	}
+
+	dev, _, ok := m.Lookup(inst.Id)
+	if !ok {
+		t.Fatal("Lookup after Ensure: not found")
+	}
+	peerBefore := dev.LookupPeer(npk)
+	if peerBefore == nil {
+		t.Fatal("LookupPeer returned nil right after Ensure created the peer")
+	}
+
+	// Simulate the reconcile job firing again with byte-identical data --
+	// this is what AmneziaWGJob does every 10 seconds regardless of whether
+	// anything actually changed.
+	if err := m.Ensure(Desired{Instance: inst}); err != nil {
+		t.Fatalf("Ensure (unchanged, second tick): %v", err)
+	}
+	peerAfter := dev.LookupPeer(npk)
+	if peerAfter == nil {
+		t.Fatal("LookupPeer returned nil after the unchanged Ensure -- peer was removed and never re-added")
+	}
+	if peerBefore != peerAfter {
+		t.Error("unchanged Ensure recreated the peer object (RemoveAllPeers + re-add) -- " +
+			"any live handshake/session on this peer would have been reset for no reason")
+	}
+}