From 1ffcfbeaf667c6579d4ac9879c4336cd3261fa46 Mon Sep 17 00:00:00 2001
From: rcourtman <courtmanr@gmail.com>
Date: Sat, 11 Oct 2025 09:58:34 +0000
Subject: [PATCH] Guard storage backups from failed polls

---
 internal/monitoring/backup_guard_test.go | 68 ++++++++++++++++++++++++
 internal/monitoring/monitor.go           | 41 ++++++++++++++
 2 files changed, 109 insertions(+)
 create mode 100644 internal/monitoring/backup_guard_test.go

diff --git a/internal/monitoring/backup_guard_test.go b/internal/monitoring/backup_guard_test.go
new file mode 100644
index 000000000..76516cf84
--- /dev/null
+++ b/internal/monitoring/backup_guard_test.go
@@ -0,0 +1,68 @@
+package monitoring
+
+import "testing"
+
+func TestShouldPreserveBackups(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name               string
+		nodeCount          int
+		hadSuccessfulNode  bool
+		storagesWithBackup int
+		contentSuccess     int
+		want               bool
+	}{
+		{
+			name:               "no successful nodes with nodes present",
+			nodeCount:          2,
+			hadSuccessfulNode:  false,
+			storagesWithBackup: 0,
+			contentSuccess:     0,
+			want:               true,
+		},
+		{
+			name:               "no nodes skips preservation",
+			nodeCount:          0,
+			hadSuccessfulNode:  false,
+			storagesWithBackup: 0,
+			contentSuccess:     0,
+			want:               false,
+		},
+		{
+			name:               "storages present but no content success",
+			nodeCount:          3,
+			hadSuccessfulNode:  true,
+			storagesWithBackup: 5,
+			contentSuccess:     0,
+			want:               true,
+		},
+		{
+			name:               "storages present with successes",
+			nodeCount:          3,
+			hadSuccessfulNode:  true,
+			storagesWithBackup: 5,
+			contentSuccess:     2,
+			want:               false,
+		},
+		{
+			name:               "no storages and no successes but had success elsewhere",
+			nodeCount:          1,
+			hadSuccessfulNode:  true,
+			storagesWithBackup: 0,
+			contentSuccess:     0,
+			want:               false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := shouldPreserveBackups(tt.nodeCount, tt.hadSuccessfulNode, tt.storagesWithBackup, tt.contentSuccess)
+			if got != tt.want {
+				t.Fatalf("shouldPreserveBackups() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go
index 96fd03167..8e7ee8615 100644
--- a/internal/monitoring/monitor.go
+++ b/internal/monitoring/monitor.go
@@ -4468,6 +4468,11 @@ func (m *Monitor) pollStorageBackupsWithNodes(ctx context.Context, instanceName
 
 	var allBackups []models.StorageBackup
 	seenVolids := make(map[string]bool) // Track seen volume IDs to avoid duplicates
+	hadSuccessfulNode := false          // Track if at least one node responded successfully
+	storagesWithBackup := 0             // Number of storages that should contain backups
+	contentSuccess := 0                 // Number of successful storage content fetches
+	contentFailures := 0                // Number of failed storage content fetches
+	storageQueryErrors := 0             // Number of nodes where storage list could not be queried
 
 	// For each node, get storage and check content
 	for _, node := range nodes {
@@ -4504,9 +4509,12 @@ func (m *Monitor) pollStorageBackupsWithNodes(ctx context.Context, instanceName
 		if err != nil {
 			monErr := errors.NewMonitorError(errors.ErrorTypeAPI, "get_storage_for_backups", instanceName, err).WithNode(node.Node)
 			log.Warn().Err(monErr).Str("node", node.Node).Msg("Failed to get storage for backups - skipping node")
+			storageQueryErrors++
 			continue
 		}
 
+		hadSuccessfulNode = true
+
 		// For each storage that can contain backups or templates
 		for _, storage := range storages {
 			// Check if storage supports backup content
@@ -4514,6 +4522,8 @@ func (m *Monitor) pollStorageBackupsWithNodes(ctx context.Context, instanceName
 				continue
 			}
 
+			storagesWithBackup++
+
 			// Get storage content
 			contents, err := client.GetStorageContent(ctx, node.Node, storage.Storage)
 			if err != nil {
@@ -4522,9 +4532,12 @@ func (m *Monitor) pollStorageBackupsWithNodes(ctx context.Context, instanceName
 					Str("node", node.Node).
 					Str("storage", storage.Storage).
 					Msg("Failed to get storage content")
+				contentFailures++
 				continue
 			}
 
+			contentSuccess++
+
 			// Convert to models
 			for _, content := range contents {
 				// Skip if we've already seen this item (shared storage duplicate)
@@ -4599,6 +4612,24 @@ func (m *Monitor) pollStorageBackupsWithNodes(ctx context.Context, instanceName
 		}
 	}
 
+	// Decide whether to keep existing backups when every query failed
+	if shouldPreserveBackups(len(nodes), hadSuccessfulNode, storagesWithBackup, contentSuccess) {
+		if len(nodes) > 0 && !hadSuccessfulNode {
+			log.Warn().
+				Str("instance", instanceName).
+				Int("nodes", len(nodes)).
+				Int("errors", storageQueryErrors).
+				Msg("Failed to query storage on all nodes; keeping previous backup list")
+		} else if storagesWithBackup > 0 && contentSuccess == 0 {
+			log.Warn().
+				Str("instance", instanceName).
+				Int("storages", storagesWithBackup).
+				Int("failures", contentFailures).
+				Msg("All storage content queries failed; keeping previous backup list")
+		}
+		return
+	}
+
 	// Update state with storage backups for this instance
 	m.state.UpdateStorageBackupsForInstance(instanceName, allBackups)
 
@@ -4608,6 +4639,16 @@ func (m *Monitor) pollStorageBackupsWithNodes(ctx context.Context, instanceName
 		Msg("Storage backups polled")
 }
 
+func shouldPreserveBackups(nodeCount int, hadSuccessfulNode bool, storagesWithBackup, contentSuccess int) bool {
+	if nodeCount > 0 && !hadSuccessfulNode {
+		return true
+	}
+	if storagesWithBackup > 0 && contentSuccess == 0 {
+		return true
+	}
+	return false
+}
+
 func (m *Monitor) calculateBackupOperationTimeout(instanceName string) time.Duration {
 	const (
 		minTimeout      = 2 * time.Minute