fix: validate pf state before stabilization

cmd/cli/vpn_dns.go

@@ -47,6 +47,9 @@ type vpnDNSManager struct {
 	// as additional nameservers for queries that match split-DNS rules
 	// (from ctrld config, AD domain, or VPN suffix config).
 	domainlessServers []string
+	// appliedExemptions advances only after the platform PF/WFP callback succeeds.
+	// Keeping it separate from discovered configs makes failed rule updates retryable.
+	appliedExemptions []vpnDNSExemption
 	// retainedAfterEmptyDiscovery means Windows reported an empty VPN DNS
 	// snapshot once while previous VPN DNS state existed. We keep that last-known
 	// state for one guarded refresh cycle because Windows can briefly report an
@@ -55,9 +58,13 @@ type vpnDNSManager struct {
 	// discoverVPNDNS is injected for tests so Refresh does not depend on the
 	// runner host's real VPN/virtual adapter state.
 	discoverVPNDNS func(context.Context) []ctrld.VPNDNSConfig
-	// refreshRunning keeps noisy network-change storms from running overlapping
-	// scutil/networksetup VPN DNS discovery work.
-	refreshRunning atomic.Bool
+	// refreshStateMu keeps noisy network-change storms from running overlapping
+	// full VPN DNS refreshes and retains one trailing refresh when an event arrives
+	// during discovery so the newest OS state is not lost.
+	refreshStateMu sync.Mutex
+	refreshRunning bool
+	refreshPending bool
+	discoveryMu    sync.Mutex
 	// Called when VPN DNS server list changes, to update intercept exemptions.
 	onServersChanged vpnDNSExemptFunc
 }
@@ -75,15 +82,41 @@ func newVPNDNSManager(logger *atomic.Pointer[ctrld.Logger], exemptFunc vpnDNSExe
 }
 
 // Refresh re-discovers VPN DNS configs from the OS.
-// Called on network change events.
+// Called on network change events. Overlapping calls are coalesced into one
+// trailing refresh so a newer OS snapshot is never silently discarded.
 func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers ...bool) {
 	logger := ctrld.LoggerFromCtx(ctx)
 	guardedRefresh := len(guardAgainstNoNameservers) > 0 && guardAgainstNoNameservers[0]
-	if !m.refreshRunning.CompareAndSwap(false, true) {
-		ctrld.Log(ctx, logger.Debug(), "VPN DNS refresh already running, skipping duplicate")
+	m.refreshStateMu.Lock()
+	if m.refreshRunning {
+		m.refreshPending = true
+		m.refreshStateMu.Unlock()
+		ctrld.Log(ctx, logger.Debug(), "VPN DNS refresh already running, coalescing trailing refresh")
 		return
 	}
-	defer m.refreshRunning.Store(false)
+	m.refreshRunning = true
+	m.refreshStateMu.Unlock()
+
+	for {
+		m.refreshOnce(ctx, guardedRefresh)
+
+		m.refreshStateMu.Lock()
+		if m.refreshPending {
+			m.refreshPending = false
+			m.refreshStateMu.Unlock()
+			guardedRefresh = true
+			continue
+		}
+		m.refreshRunning = false
+		m.refreshStateMu.Unlock()
+		return
+	}
+}
+
+func (m *vpnDNSManager) refreshOnce(ctx context.Context, guardAgainstNoNameservers bool) {
+	logger := ctrld.LoggerFromCtx(ctx)
+	m.discoveryMu.Lock()
+	defer m.discoveryMu.Unlock()
 
 	ctrld.Log(ctx, logger.Debug(), "Refreshing VPN DNS configurations")
 	discoverVPNDNS := m.discoverVPNDNS
@@ -110,9 +143,7 @@ func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers .
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	previousExemptions := m.currentExemptionsLocked()
-
-	if vpnDNSSettlingEnabled && len(configs) == 0 && guardedRefresh && m.hasVPNDNSStateLocked() {
+	if vpnDNSSettlingEnabled && len(configs) == 0 && guardAgainstNoNameservers && m.hasVPNDNSStateLocked() {
 		if !m.retainedAfterEmptyDiscovery {
 			exemptions := m.currentExemptionsLocked()
 			m.retainedAfterEmptyDiscovery = true
@@ -122,6 +153,8 @@ func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers .
 			if m.onServersChanged != nil {
 				if err := m.onServersChanged(exemptions); err != nil {
 					ctrld.Log(ctx, logger.Error().Err(err), "Failed to re-apply retained VPN DNS exemptions")
+				} else {
+					m.appliedExemptions = append([]vpnDNSExemption(nil), exemptions...)
 				}
 			}
 			return
@@ -200,41 +233,47 @@ func (m *vpnDNSManager) Refresh(ctx context.Context, guardAgainstNoNameservers .
 	ctrld.Log(ctx, logger.Debug(), "VPN DNS refresh completed: %d configs, %d routes, %d domainless servers, %d unique exemptions",
 		len(m.configs), len(m.routes), len(m.domainlessServers), len(exemptions))
 
-	// Update intercept rules to permit VPN DNS traffic only when the exemption set
-	// actually changes. Network-change events can fire repeatedly while macOS/VPN
-	// state is otherwise identical; rewriting pf for identical exemptions can feed
-	// a self-triggering network-change loop. Empty exemptions are still applied
-	// when they differ from the previous set, so stale VPN exemptions are cleared
-	// on disconnect.
-	m.updateInterceptExemptionsIfChanged(ctx, logger, previousExemptions, exemptions, "VPN DNS")
+	// Update intercept rules only when desired exemptions differ from the last
+	// successfully applied set. Failed PF/WFP callbacks remain retryable on the
+	// next refresh even when discovery returns the same VPN DNS state.
+	m.updateInterceptExemptionsIfChanged(ctx, logger, exemptions, "VPN DNS")
 }
 
-func (m *vpnDNSManager) updateInterceptExemptionsIfChanged(ctx context.Context, logger *ctrld.Logger, before, after []vpnDNSExemption, reason string) {
+func (m *vpnDNSManager) updateInterceptExemptionsIfChanged(ctx context.Context, logger *ctrld.Logger, desired []vpnDNSExemption, reason string) {
 	if m.onServersChanged == nil {
 		return
 	}
-	if vpnDNSExemptionsEqual(before, after) {
+	if vpnDNSExemptionsEqual(m.appliedExemptions, desired) {
 		ctrld.Log(ctx, logger.Debug(), "VPN DNS exemptions unchanged after %s refresh; skipping intercept rule update", reason)
 		return
 	}
-	if err := m.onServersChanged(after); err != nil {
+	if err := m.onServersChanged(desired); err != nil {
 		ctrld.Log(ctx, logger.Error().Err(err), "Failed to update intercept exemptions for VPN DNS servers")
+		return
 	}
+	m.appliedExemptions = append([]vpnDNSExemption(nil), desired...)
 }
 
-// RefreshRoutesOnly re-discovers VPN DNS configs and updates only ctrld's
-// in-memory split-DNS routes. It intentionally does not call onServersChanged,
-// so it does not rewrite/reload pf/WFP rules. Use this for post-settle discovery
-// checks where we only need to learn late-published VPN search domains.
+// RefreshRoutesOnly re-discovers VPN DNS configs and updates ctrld's
+// in-memory split-DNS routes. It applies intercept exemptions only when that set
+// changes, while holding the shared discovery lane so a concurrent full refresh
+// cannot commit a newer snapshot and then be overwritten by this one.
 func (m *vpnDNSManager) RefreshRoutesOnly() (routes, domainlessServers, exemptions int) {
 	logger := mainLog.Load()
+	if m.logger != nil && m.logger.Load() != nil {
+		logger = m.logger.Load()
+	}
+	ctx := ctrld.LoggerCtx(context.Background(), logger)
 
-	logger.Debug().Msg("Refreshing VPN DNS route state only")
+	m.discoveryMu.Lock()
+	defer m.discoveryMu.Unlock()
+
+	ctrld.Log(ctx, logger.Debug(), "Refreshing VPN DNS route state only")
 	discoverVPNDNS := m.discoverVPNDNS
 	if discoverVPNDNS == nil {
 		discoverVPNDNS = ctrld.DiscoverVPNDNS
 	}
-	configs := discoverVPNDNS(context.Background())
+	configs := discoverVPNDNS(ctx)
 
 	if dri, err := netmon.DefaultRouteInterface(); err == nil && dri != "" {
 		for i := range configs {
@@ -275,10 +314,26 @@ func (m *vpnDNSManager) RefreshRoutesOnly() (routes, domainlessServers, exemptio
 		}
 	}
 	m.domainlessServers = domainless
+	currentExemptions := m.currentExemptionsLocked()
 
-	logger.Debug().Msgf("VPN DNS route-only refresh completed: %d configs, %d routes, %d domainless servers, %d exemptions",
-		len(m.configs), len(m.routes), len(m.domainlessServers), len(m.currentExemptionsLocked()))
-	return len(m.routes), len(m.domainlessServers), len(m.currentExemptionsLocked())
+	ctrld.Log(ctx, logger.Debug(), "VPN DNS route-only refresh completed: %d configs, %d routes, %d domainless servers, %d exemptions",
+		len(m.configs), len(m.routes), len(m.domainlessServers), len(currentExemptions))
+	m.updateInterceptExemptionsIfChanged(ctx, logger, currentExemptions, "route-only VPN DNS")
+	return len(m.routes), len(m.domainlessServers), len(currentExemptions)
+}
+
+func (m *vpnDNSManager) markInterceptExemptionsApplied(applied []vpnDNSExemption) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	if vpnDNSExemptionsEqual(m.currentExemptionsLocked(), applied) {
+		m.appliedExemptions = append([]vpnDNSExemption(nil), applied...)
+	}
+}
+
+func (m *vpnDNSManager) interceptExemptionsPending() bool {
+	m.mu.RLock()
+	defer m.mu.RUnlock()
+	return !vpnDNSExemptionsEqual(m.appliedExemptions, m.currentExemptionsLocked())
 }
 
 func (m *vpnDNSManager) hasVPNDNSStateLocked() bool {