Related to #727: normalize persisted Proxmox hosts

internal/config/persistence.go

@@ -958,7 +958,12 @@ func (c *ConfigPersistence) saveNodesConfig(pveInstances []PVEInstance, pbsInsta
 // LoadNodesConfig loads nodes configuration from file (decrypts if encrypted)
 func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) {
 	c.mu.RLock()
-	defer c.mu.RUnlock()
+	unlocked := false
+	defer func() {
+		if !unlocked {
+			c.mu.RUnlock()
+		}
+	}()
 
 	data, err := os.ReadFile(c.nodesFile)
 	if err != nil {
@@ -1075,6 +1080,41 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) {
 	// Track if any migrations were applied
 	migrationApplied := false
 
+	normalizeHostWithDefault := func(host, defaultPort string) (string, bool) {
+		normalized := normalizeHostPort(host, defaultPort)
+		if normalized == "" || normalized == host {
+			return host, false
+		}
+		return normalized, true
+	}
+
+	// Normalize PVE hosts (and cluster endpoints) to restore default ports for configs
+	// saved during the 4.32.0 regression that skipped default port injection.
+	for i := range config.PVEInstances {
+		if normalized, changed := normalizeHostWithDefault(config.PVEInstances[i].Host, defaultPVEPort); changed {
+			log.Info().
+				Str("instance", config.PVEInstances[i].Name).
+				Str("oldHost", config.PVEInstances[i].Host).
+				Str("newHost", normalized).
+				Msg("Normalized PVE host to include default port")
+			config.PVEInstances[i].Host = normalized
+			migrationApplied = true
+		}
+
+		for j := range config.PVEInstances[i].ClusterEndpoints {
+			if normalized, changed := normalizeHostWithDefault(config.PVEInstances[i].ClusterEndpoints[j].Host, defaultPVEPort); changed {
+				log.Info().
+					Str("instance", config.PVEInstances[i].Name).
+					Str("node", config.PVEInstances[i].ClusterEndpoints[j].NodeName).
+					Str("oldHost", config.PVEInstances[i].ClusterEndpoints[j].Host).
+					Str("newHost", normalized).
+					Msg("Normalized PVE cluster endpoint host to include default port")
+				config.PVEInstances[i].ClusterEndpoints[j].Host = normalized
+				migrationApplied = true
+			}
+		}
+	}
+
 	// Fix for bug where TokenName was incorrectly set when using password auth
 	// If a PBS instance has both Password and TokenName, clear the TokenName
 	for i := range config.PBSInstances {
@@ -1084,49 +1124,17 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) {
 				Msg("Fixing PBS config: clearing TokenName since Password is set")
 			config.PBSInstances[i].TokenName = ""
 			config.PBSInstances[i].TokenValue = ""
+			migrationApplied = true
 		}
 
-		// Fix for missing port in PBS host
+		if normalized, changed := normalizeHostWithDefault(config.PBSInstances[i].Host, defaultPBSPort); changed {
-		host := config.PBSInstances[i].Host
+			log.Info().
-		if host != "" {
+				Str("instance", config.PBSInstances[i].Name).
-			// Check if we need to add default port
+				Str("oldHost", config.PBSInstances[i].Host).
-			protocolEnd := 0
+				Str("newHost", normalized).
-			if strings.HasPrefix(host, "https://") {
+				Msg("Normalized PBS host to include default port")
-				protocolEnd = 8
+			config.PBSInstances[i].Host = normalized
-			} else if strings.HasPrefix(host, "http://") {
+			migrationApplied = true
-				protocolEnd = 7
-			} else if !strings.Contains(host, "://") {
-				// No protocol specified, add https and check for port
-				if !strings.Contains(host, ":") {
-					// No port specified, add protocol and default port
-					config.PBSInstances[i].Host = "https://" + host + ":8007"
-					log.Info().
-						Str("instance", config.PBSInstances[i].Name).
-						Str("oldHost", host).
-						Str("newHost", config.PBSInstances[i].Host).
-						Msg("Fixed PBS host by adding protocol and default port")
-				} else {
-					// Port specified, just add protocol
-					config.PBSInstances[i].Host = "https://" + host
-					log.Info().
-						Str("instance", config.PBSInstances[i].Name).
-						Str("oldHost", host).
-						Str("newHost", config.PBSInstances[i].Host).
-						Msg("Fixed PBS host by adding protocol")
-				}
-			} else if protocolEnd > 0 {
-				// Has protocol, check if port is missing
-				hostAfterProtocol := host[protocolEnd:]
-				if !strings.Contains(hostAfterProtocol, ":") {
-					// No port specified, add default PBS port
-					config.PBSInstances[i].Host = host + ":8007"
-					log.Info().
-						Str("instance", config.PBSInstances[i].Name).
-						Str("oldHost", host).
-						Str("newHost", config.PBSInstances[i].Host).
-						Msg("Fixed PBS host by adding default port 8007")
-				}
-			}
 		}
 
 		// Migration: Ensure MonitorBackups is enabled for PBS instances
@@ -1150,42 +1158,14 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) {
 			migrationApplied = true
 		}
 
-		host := config.PMGInstances[i].Host
+		if normalized, changed := normalizeHostWithDefault(config.PMGInstances[i].Host, defaultPVEPort); changed {
-		if host == "" {
-			continue
-		}
-
-		protocolEnd := 0
-		if strings.HasPrefix(host, "https://") {
-			protocolEnd = 8
-		} else if strings.HasPrefix(host, "http://") {
-			protocolEnd = 7
-		} else if !strings.Contains(host, "://") {
-			if !strings.Contains(host, ":") {
-				config.PMGInstances[i].Host = "https://" + host + ":8006"
-			} else {
-				config.PMGInstances[i].Host = "https://" + host
-			}
 			log.Info().
 				Str("instance", config.PMGInstances[i].Name).
-				Str("oldHost", host).
+				Str("oldHost", config.PMGInstances[i].Host).
-				Str("newHost", config.PMGInstances[i].Host).
+				Str("newHost", normalized).
-				Msg("Fixed PMG host by adding protocol/port")
+				Msg("Normalized PMG host to include default port")
+			config.PMGInstances[i].Host = normalized
 			migrationApplied = true
-			continue
-		}
-
-		if protocolEnd > 0 {
-			hostAfterProtocol := host[protocolEnd:]
-			if !strings.Contains(hostAfterProtocol, ":") {
-				config.PMGInstances[i].Host = host + ":8006"
-				log.Info().
-					Str("instance", config.PMGInstances[i].Name).
-					Str("oldHost", host).
-					Str("newHost", config.PMGInstances[i].Host).
-					Msg("Fixed PMG host by adding default port 8006")
-				migrationApplied = true
-			}
 		}
 	}
 
@@ -1209,6 +1189,7 @@ func (c *ConfigPersistence) LoadNodesConfig() (*NodesConfig, error) {
 		copy(pmgCopy, config.PMGInstances)
 
 		// Release read lock before saving (SaveNodesConfig acquires write lock)
+		unlocked = true
 		c.mu.RUnlock()
 
 		log.Info().Msg("Migrations applied, saving updated configuration")

internal/config/persistence_test.go

@@ -808,6 +808,63 @@ func TestImportAcceptsVersion40Bundle(t *testing.T) {
 	assertJSONEqual(t, tokensAfter, baselineTokens, "api tokens unchanged for 4.0 import")
 }
 
+func TestLoadNodesConfigNormalizesPVEHostsAndClusterEndpoints(t *testing.T) {
+	tempDir := t.TempDir()
+	cp := config.NewConfigPersistence(tempDir)
+	if err := cp.EnsureConfigDir(); err != nil {
+		t.Fatalf("EnsureConfigDir: %v", err)
+	}
+
+	pveNodes := []config.PVEInstance{
+		{
+			Name: "pve-cluster",
+			Host: "https://pve.local",
+			ClusterEndpoints: []config.ClusterEndpoint{
+				{NodeName: "pve1", Host: "https://pve1.local"},
+				{NodeName: "pve2", Host: "pve2.local"},
+			},
+		},
+	}
+
+	if err := cp.SaveNodesConfig(pveNodes, nil, nil); err != nil {
+		t.Fatalf("SaveNodesConfig: %v", err)
+	}
+
+	loaded, err := cp.LoadNodesConfig()
+	if err != nil {
+		t.Fatalf("LoadNodesConfig: %v", err)
+	}
+
+	if len(loaded.PVEInstances) != 1 {
+		t.Fatalf("expected 1 PVE instance, got %d", len(loaded.PVEInstances))
+	}
+
+	pve := loaded.PVEInstances[0]
+	if pve.Host != "https://pve.local:8006" {
+		t.Fatalf("expected primary host normalized with default port, got %q", pve.Host)
+	}
+
+	if len(pve.ClusterEndpoints) != 2 {
+		t.Fatalf("expected 2 cluster endpoints, got %d", len(pve.ClusterEndpoints))
+	}
+
+	if pve.ClusterEndpoints[0].Host != "https://pve1.local:8006" {
+		t.Fatalf("expected endpoint host normalized, got %q", pve.ClusterEndpoints[0].Host)
+	}
+	if pve.ClusterEndpoints[1].Host != "https://pve2.local:8006" {
+		t.Fatalf("expected endpoint host normalized, got %q", pve.ClusterEndpoints[1].Host)
+	}
+
+	// Second load should keep normalized values and not panic on migration.
+	loadedAgain, err := cp.LoadNodesConfig()
+	if err != nil {
+		t.Fatalf("LoadNodesConfig second read: %v", err)
+	}
+	if loadedAgain.PVEInstances[0].Host != "https://pve.local:8006" {
+		t.Fatalf("expected normalized host persisted, got %q", loadedAgain.PVEInstances[0].Host)
+	}
+}
+
 func mustDecodeExport(t *testing.T, payload, passphrase string) config.ExportData {
 	t.Helper()
 

pkg/proxmox/cluster_client.go

@@ -581,6 +581,7 @@ func (cc *ClusterClient) executeWithFailover(ctx context.Context, fn func(*Clien
 		// Error 500 with any "guest agent" message means VM-specific issue, not node failure
 		// Error 400 with "ds" parameter error means Proxmox 9.x doesn't support RRD data source filtering
 		// JSON unmarshal errors are data format issues, not connectivity problems
+		// Context deadline/timeout errors on storage endpoints mean storage issues, not node unreachability
 		if strings.Contains(errStr, "595") ||
 			(strings.Contains(errStr, "500") && strings.Contains(errStr, "hostname lookup")) ||
 			(strings.Contains(errStr, "500") && strings.Contains(errStr, "Name or service not known")) ||
@@ -593,7 +594,9 @@ func (cc *ClusterClient) executeWithFailover(ctx context.Context, fn func(*Clien
 			(strings.Contains(errStr, "400") && strings.Contains(errStr, "\"ds\"") && strings.Contains(errStr, "property is not defined in schema")) ||
 			strings.Contains(errStr, "json: cannot unmarshal") ||
 			(strings.Contains(errStr, "storage '") && strings.Contains(errStr, "is not available on node")) ||
-			strings.Contains(errStr, "unexpected response format") {
+			strings.Contains(errStr, "unexpected response format") ||
+			(strings.Contains(errStr, "context deadline exceeded") && strings.Contains(errStr, "/storage/")) ||
+			(strings.Contains(errStr, "Client.Timeout exceeded") && strings.Contains(errStr, "/storage/")) {
 			// This is likely a node-specific failure, not an endpoint failure
 			// Return the error but don't mark the endpoint as unhealthy
 			log.Debug().