From d0940d03c4656a05e7c8fec1d4cb766dedd53047 Mon Sep 17 00:00:00 2001 From: Owen Date: Thu, 18 Dec 2025 11:33:59 -0500 Subject: [PATCH] Cleanup unclean shutdown cross platform Former-commit-id: c1a2efd9d253b852cd45563760d188f3626277ed --- dns/platform/darwin.go | 161 +++++++++++++++++++++++++++++++- dns/platform/file.go | 30 +++++- dns/platform/network_manager.go | 35 ++++++- dns/platform/resolvconf.go | 33 ++++++- dns/platform/systemd.go | 22 ++++- dns/platform/types.go | 4 + dns/platform/windows.go | 12 +++ 7 files changed, 285 insertions(+), 12 deletions(-) diff --git a/dns/platform/darwin.go b/dns/platform/darwin.go index a31f3a4..61cc81b 100644 --- a/dns/platform/darwin.go +++ b/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{ - createdKeys: make(map[string]struct{}), - }, nil + stateFilePath := getDNSStateFilePath() + + 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 +} \ No newline at end of file diff --git a/dns/platform/file.go b/dns/platform/file.go index 8f6f766..5f1cede 100644 --- a/dns/platform/file.go +++ b/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) diff --git a/dns/platform/network_manager.go b/dns/platform/network_manager.go index a88f5e9..44eb655 100644 --- a/dns/platform/network_manager.go +++ b/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") diff --git a/dns/platform/resolvconf.go b/dns/platform/resolvconf.go index 4202c4c..6f95c1f 100644 --- a/dns/platform/resolvconf.go +++ b/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 diff --git a/dns/platform/systemd.go b/dns/platform/systemd.go index 61f9ca6..2f18009 100644 --- a/dns/platform/systemd.go +++ b/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 diff --git a/dns/platform/types.go b/dns/platform/types.go index 471ba29..66d30b5 100644 --- a/dns/platform/types.go +++ b/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 diff --git a/dns/platform/windows.go b/dns/platform/windows.go index f4c5896..1f76171 100644 --- a/dns/platform/windows.go +++ b/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)