Cleanup unclean shutdown cross platform

Former-commit-id: c1a2efd9d2

dns/platform/darwin.go

@@ -5,9 +5,13 @@ package dns
 import (
 	"bufio"
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"net/netip"
+	"os"
 	"os/exec"
+	"path/filepath"
+	"runtime"
 	"strconv"
 	"strings"
 
@@ -28,19 +32,38 @@ const (
 	keyServerPort                       = "ServerPort"
 	arraySymbol                         = "* "
 	digitSymbol                         = "# "
+
+	// State file name for crash recovery
+	dnsStateFileName = "dns_state.json"
 )
 
+// DNSPersistentState represents the state saved to disk for crash recovery
+type DNSPersistentState struct {
+	CreatedKeys []string `json:"created_keys"`
+}
+
 // DarwinDNSConfigurator manages DNS settings on macOS using scutil
 type DarwinDNSConfigurator struct {
 	createdKeys   map[string]struct{}
 	originalState *DNSState
+	stateFilePath string
 }
 
 // NewDarwinDNSConfigurator creates a new macOS DNS configurator
 func NewDarwinDNSConfigurator() (*DarwinDNSConfigurator, error) {
-	return &DarwinDNSConfigurator{
+	stateFilePath := getDNSStateFilePath()
-		createdKeys: make(map[string]struct{}),
+
-	}, nil
+	configurator := &DarwinDNSConfigurator{
+		createdKeys:   make(map[string]struct{}),
+		stateFilePath: stateFilePath,
+	}
+
+	// Clean up any leftover state from a previous crash
+	if err := configurator.CleanupUncleanShutdown(); err != nil {
+		logger.Warn("Failed to cleanup previous DNS state: %v", err)
+	}
+
+	return configurator, nil
 }
 
 // Name returns the configurator name
@@ -67,6 +90,11 @@ func (d *DarwinDNSConfigurator) SetDNS(servers []netip.Addr) ([]netip.Addr, erro
 		return nil, fmt.Errorf("apply DNS servers: %w", err)
 	}
 
+	// Persist state to disk for crash recovery
+	if err := d.saveState(); err != nil {
+		logger.Warn("Failed to save DNS state for crash recovery: %v", err)
+	}
+
 	// Flush DNS cache
 	if err := d.flushDNSCache(); err != nil {
 		// Non-fatal, just log
@@ -85,6 +113,11 @@ func (d *DarwinDNSConfigurator) RestoreDNS() error {
 		}
 	}
 
+	// Clear state file after successful restoration
+	if err := d.clearState(); err != nil {
+		logger.Warn("Failed to clear DNS state file: %v", err)
+	}
+
 	// Flush DNS cache
 	if err := d.flushDNSCache(); err != nil {
 		fmt.Printf("warning: failed to flush DNS cache: %v\n", err)
@@ -112,6 +145,47 @@ func (d *DarwinDNSConfigurator) GetCurrentDNS() ([]netip.Addr, error) {
 	return servers, nil
 }
 
+// CleanupUncleanShutdown removes any DNS keys left over from a previous crash
+func (d *DarwinDNSConfigurator) CleanupUncleanShutdown() error {
+	state, err := d.loadState()
+	if err != nil {
+		if os.IsNotExist(err) {
+			// No state file, nothing to clean up
+			return nil
+		}
+		return fmt.Errorf("load state: %w", err)
+	}
+
+	if len(state.CreatedKeys) == 0 {
+		// No keys to clean up
+		return nil
+	}
+
+	logger.Info("Found DNS state from previous session, cleaning up %d keys", len(state.CreatedKeys))
+
+	// Remove all keys from previous session
+	var lastErr error
+	for _, key := range state.CreatedKeys {
+		logger.Debug("Removing leftover DNS key: %s", key)
+		if err := d.removeKeyDirect(key); err != nil {
+			logger.Warn("Failed to remove DNS key %s: %v", key, err)
+			lastErr = err
+		}
+	}
+
+	// Clear state file
+	if err := d.clearState(); err != nil {
+		logger.Warn("Failed to clear DNS state file: %v", err)
+	}
+
+	// Flush DNS cache after cleanup
+	if err := d.flushDNSCache(); err != nil {
+		logger.Warn("Failed to flush DNS cache after cleanup: %v", err)
+	}
+
+	return lastErr
+}
+
 // applyDNSServers applies the DNS server configuration
 func (d *DarwinDNSConfigurator) applyDNSServers(servers []netip.Addr) error {
 	if len(servers) == 0 {
@@ -156,15 +230,25 @@ func (d *DarwinDNSConfigurator) addDNSState(state, domains string, dnsServer net
 	return nil
 }
 
-// removeKey removes a DNS configuration key
+// removeKey removes a DNS configuration key and updates internal state
 func (d *DarwinDNSConfigurator) removeKey(key string) error {
+	if err := d.removeKeyDirect(key); err != nil {
+		return err
+	}
+
+	delete(d.createdKeys, key)
+	return nil
+}
+
+// removeKeyDirect removes a DNS configuration key without updating internal state
+// Used for cleanup operations
+func (d *DarwinDNSConfigurator) removeKeyDirect(key string) error {
 	cmd := fmt.Sprintf("remove %s\n", key)
 
 	if _, err := d.runScutil(cmd); err != nil {
 		return fmt.Errorf("remove key: %w", err)
 	}
 
-	delete(d.createdKeys, key)
 	return nil
 }
 
@@ -266,3 +350,70 @@ func (d *DarwinDNSConfigurator) runScutil(commands string) ([]byte, error) {
 
 	return output, nil
 }
+
+// getDNSStateFilePath returns the path to the DNS state file
+func getDNSStateFilePath() string {
+	var stateDir string
+	switch runtime.GOOS {
+	case "darwin":
+		stateDir = filepath.Join(os.Getenv("HOME"), "Library", "Application Support", "olm-client")
+	default:
+		stateDir = filepath.Join(os.Getenv("HOME"), ".config", "olm-client")
+	}
+
+	if err := os.MkdirAll(stateDir, 0755); err != nil {
+		logger.Warn("Failed to create state directory: %v", err)
+	}
+
+	return filepath.Join(stateDir, dnsStateFileName)
+}
+
+// saveState persists the current DNS state to disk
+func (d *DarwinDNSConfigurator) saveState() error {
+	keys := make([]string, 0, len(d.createdKeys))
+	for key := range d.createdKeys {
+		keys = append(keys, key)
+	}
+
+	state := DNSPersistentState{
+		CreatedKeys: keys,
+	}
+
+	data, err := json.MarshalIndent(state, "", "  ")
+	if err != nil {
+		return fmt.Errorf("marshal state: %w", err)
+	}
+
+	if err := os.WriteFile(d.stateFilePath, data, 0644); err != nil {
+		return fmt.Errorf("write state file: %w", err)
+	}
+
+	logger.Debug("Saved DNS state to %s", d.stateFilePath)
+	return nil
+}
+
+// loadState loads the DNS state from disk
+func (d *DarwinDNSConfigurator) loadState() (*DNSPersistentState, error) {
+	data, err := os.ReadFile(d.stateFilePath)
+	if err != nil {
+		return nil, err
+	}
+
+	var state DNSPersistentState
+	if err := json.Unmarshal(data, &state); err != nil {
+		return nil, fmt.Errorf("unmarshal state: %w", err)
+	}
+
+	return &state, nil
+}
+
+// clearState removes the DNS state file
+func (d *DarwinDNSConfigurator) clearState() error {
+	err := os.Remove(d.stateFilePath)
+	if err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("remove state file: %w", err)
+	}
+
+	logger.Debug("Cleared DNS state file")
+	return nil
+}

dns/platform/file.go

@@ -22,7 +22,11 @@ type FileDNSConfigurator struct {
 
 // NewFileDNSConfigurator creates a new file-based DNS configurator
 func NewFileDNSConfigurator() (*FileDNSConfigurator, error) {
-	return &FileDNSConfigurator{}, nil
+	f := &FileDNSConfigurator{}
+	if err := f.CleanupUncleanShutdown(); err != nil {
+		return nil, fmt.Errorf("cleanup unclean shutdown: %w", err)
+	}
+	return f, nil
 }
 
 // Name returns the configurator name
@@ -78,6 +82,30 @@ func (f *FileDNSConfigurator) RestoreDNS() error {
 	return nil
 }
 
+// CleanupUncleanShutdown removes any DNS configuration left over from a previous crash
+// For the file-based configurator, we check if a backup file exists (indicating a crash
+// happened while DNS was configured) and restore from it if so.
+func (f *FileDNSConfigurator) CleanupUncleanShutdown() error {
+	// Check if backup file exists from a previous session
+	if !f.isBackupExists() {
+		// No backup file, nothing to clean up
+		return nil
+	}
+
+	// A backup exists, which means we crashed while DNS was configured
+	// Restore the original resolv.conf
+	if err := copyFile(resolvConfBackupPath, resolvConfPath); err != nil {
+		return fmt.Errorf("restore from backup during cleanup: %w", err)
+	}
+
+	// Remove backup file
+	if err := os.Remove(resolvConfBackupPath); err != nil {
+		return fmt.Errorf("remove backup file during cleanup: %w", err)
+	}
+
+	return nil
+}
+
 // GetCurrentDNS returns the currently configured DNS servers
 func (f *FileDNSConfigurator) GetCurrentDNS() ([]netip.Addr, error) {
 	content, err := os.ReadFile(resolvConfPath)

dns/platform/network_manager.go

@@ -50,11 +50,18 @@ func NewNetworkManagerDNSConfigurator(ifaceName string) (*NetworkManagerDNSConfi
 		return nil, fmt.Errorf("NetworkManager conf.d directory not found: %s", networkManagerConfDir)
 	}
 
-	return &NetworkManagerDNSConfigurator{
+	configurator := &NetworkManagerDNSConfigurator{
 		ifaceName:    ifaceName,
 		confPath:     networkManagerConfDir + "/" + networkManagerDNSConfFile,
 		dispatchPath: networkManagerDispatcherDir + "/" + networkManagerDispatcherFile,
-	}, nil
+	}
+
+	// Clean up any stale configuration from a previous unclean shutdown
+	if err := configurator.CleanupUncleanShutdown(); err != nil {
+		return nil, fmt.Errorf("cleanup unclean shutdown: %w", err)
+	}
+
+	return configurator, nil
 }
 
 // Name returns the configurator name
@@ -100,6 +107,30 @@ func (n *NetworkManagerDNSConfigurator) RestoreDNS() error {
 	return nil
 }
 
+// CleanupUncleanShutdown removes any DNS configuration left over from a previous crash
+// For NetworkManager, we check if our config file exists and remove it if so.
+// This ensures that if the process crashed while DNS was configured, the stale
+// configuration is removed on the next startup.
+func (n *NetworkManagerDNSConfigurator) CleanupUncleanShutdown() error {
+	// Check if our config file exists from a previous session
+	if _, err := os.Stat(n.confPath); os.IsNotExist(err) {
+		// No config file, nothing to clean up
+		return nil
+	}
+
+	// Remove the stale configuration file
+	if err := os.Remove(n.confPath); err != nil && !os.IsNotExist(err) {
+		return fmt.Errorf("remove stale DNS config file: %w", err)
+	}
+
+	// Reload NetworkManager to apply the change
+	if err := n.reloadNetworkManager(); err != nil {
+		return fmt.Errorf("reload NetworkManager after cleanup: %w", err)
+	}
+
+	return nil
+}
+
 // GetCurrentDNS returns the currently configured DNS servers by reading /etc/resolv.conf
 func (n *NetworkManagerDNSConfigurator) GetCurrentDNS() ([]netip.Addr, error) {
 	content, err := os.ReadFile("/etc/resolv.conf")

dns/platform/resolvconf.go

@@ -31,10 +31,17 @@ func NewResolvconfDNSConfigurator(ifaceName string) (*ResolvconfDNSConfigurator,
 		return nil, fmt.Errorf("detect resolvconf type: %w", err)
 	}
 
-	return &ResolvconfDNSConfigurator{
+	configurator := &ResolvconfDNSConfigurator{
 		ifaceName: ifaceName,
 		implType:  implType,
-	}, nil
+	}
+
+	// Call cleanup function to remove any stale DNS config for this interface
+	if err := configurator.CleanupUncleanShutdown(); err != nil {
+		return nil, fmt.Errorf("cleanup unclean shutdown: %w", err)
+	}
+
+	return configurator, nil
 }
 
 // Name returns the configurator name
@@ -84,6 +91,28 @@ func (r *ResolvconfDNSConfigurator) RestoreDNS() error {
 	return nil
 }
 
+// CleanupUncleanShutdown removes any DNS configuration left over from a previous crash
+// For resolvconf, we attempt to delete any entry for the interface name.
+// This ensures that if the process crashed while DNS was configured, the stale
+// entry is removed on the next startup.
+func (r *ResolvconfDNSConfigurator) CleanupUncleanShutdown() error {
+	// Try to delete any existing entry for this interface
+	// This is idempotent - if no entry exists, resolvconf will just return success
+	var cmd *exec.Cmd
+
+	switch r.implType {
+	case "openresolv":
+		cmd = exec.Command(resolvconfCommand, "-f", "-d", r.ifaceName)
+	default:
+		cmd = exec.Command(resolvconfCommand, "-d", r.ifaceName)
+	}
+
+	// Ignore errors - the entry may not exist, which is fine
+	_ = cmd.Run()
+
+	return nil
+}
+
 // GetCurrentDNS returns the currently configured DNS servers
 func (r *ResolvconfDNSConfigurator) GetCurrentDNS() ([]netip.Addr, error) {
 	// resolvconf doesn't provide a direct way to query per-interface DNS

dns/platform/systemd.go

@@ -73,10 +73,17 @@ func NewSystemdResolvedDNSConfigurator(ifaceName string) (*SystemdResolvedDNSCon
 		return nil, fmt.Errorf("get link: %w", err)
 	}
 
-	return &SystemdResolvedDNSConfigurator{
+	config := &SystemdResolvedDNSConfigurator{
 		ifaceName:      ifaceName,
 		dbusLinkObject: dbus.ObjectPath(linkPath),
-	}, nil
+	}
+
+	// Call cleanup function here
+	if err := config.CleanupUncleanShutdown(); err != nil {
+		fmt.Printf("warning: cleanup unclean shutdown failed: %v\n", err)
+	}
+
+	return config, nil
 }
 
 // Name returns the configurator name
@@ -133,6 +140,17 @@ func (s *SystemdResolvedDNSConfigurator) RestoreDNS() error {
 	return nil
 }
 
+// CleanupUncleanShutdown removes any DNS configuration left over from a previous crash
+// For systemd-resolved, the DNS configuration is tied to the network interface.
+// When the interface is destroyed and recreated, systemd-resolved automatically
+// clears the per-link DNS settings, so there's nothing to clean up.
+func (s *SystemdResolvedDNSConfigurator) CleanupUncleanShutdown() error {
+	// systemd-resolved DNS configuration is per-link and automatically cleared
+	// when the link (interface) is destroyed. Since the WireGuard interface is
+	// recreated on restart, there's no leftover state to clean up.
+	return nil
+}
+
 // GetCurrentDNS returns the currently configured DNS servers
 // Note: systemd-resolved doesn't easily expose current per-link DNS servers via D-Bus
 // This is a placeholder that returns an empty list

dns/platform/types.go

@@ -17,6 +17,10 @@ type DNSConfigurator interface {
 
 	// Name returns the name of this configurator implementation
 	Name() string
+
+	// CleanupUncleanShutdown removes any DNS configuration left over from
+	// a previous crash or unclean shutdown. This should be called on startup.
+	CleanupUncleanShutdown() error
 }
 
 // DNSConfig contains the configuration for DNS override

dns/platform/windows.go

@@ -113,6 +113,18 @@ func (w *WindowsDNSConfigurator) RestoreDNS() error {
 	return nil
 }
 
+// CleanupUncleanShutdown removes any DNS configuration left over from a previous crash
+// On Windows, we rely on the registry-based approach which doesn't leave orphaned state
+// in the same way as macOS scutil. The DNS settings are tied to the interface which
+// gets recreated on restart.
+func (w *WindowsDNSConfigurator) CleanupUncleanShutdown() error {
+	// Windows DNS configuration via registry is interface-specific.
+	// When the WireGuard interface is recreated, it gets a new GUID,
+	// so there's no leftover state to clean up from previous sessions.
+	// The old interface's registry keys are effectively orphaned but harmless.
+	return nil
+}
+
 // GetCurrentDNS returns the currently configured DNS servers
 func (w *WindowsDNSConfigurator) GetCurrentDNS() ([]netip.Addr, error) {
 	regKey, err := w.getInterfaceRegistryKey(registry.QUERY_VALUE)