From 738163699e1e7a02d25355671dc4a67366bd756a Mon Sep 17 00:00:00 2001 From: Kuzz007 Date: Tue, 4 Aug 2026 01:46:32 +0300 Subject: [PATCH] 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). --- internal/amneziawgnet/manager.go | 71 +++++++++++++++------- internal/amneziawgnet/manager_test.go | 86 +++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 21 deletions(-) diff --git a/internal/amneziawgnet/manager.go b/internal/amneziawgnet/manager.go index 94bb09c42..da34e6450 100644 --- a/internal/amneziawgnet/manager.go +++ b/internal/amneziawgnet/manager.go @@ -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) diff --git a/internal/amneziawgnet/manager_test.go b/internal/amneziawgnet/manager_test.go index 3f2741c2b..c53b38c94 100644 --- a/internal/amneziawgnet/manager_test.go +++ b/internal/amneziawgnet/manager_test.go @@ -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") + } +}