Refactor alert system: fix race conditions, memory leaks, and improve code quality

- Rename checkFlapping to checkFlappingLocked to clarify lock contract
- Replace goto statements with structured control flow
- Wire up unused recordAlertFired/recordAlertResolved metric hooks
- Add trackingMapCleanup goroutine to prevent memory leaks from stale entries
- Tighten alert ID validation to alphanumeric + safe punctuation
- Fix history save error handling to properly manage backup lifecycle
- Add auto-migration for deprecated GroupingWindow field
- Refactor 300+ line UpdateConfig into focused helper functions
- Unify duplicate evaluateVMCondition/evaluateContainerCondition
- Add constants for magic numbers (thresholds, timing, flapping)
- Update tests to match new backup behavior

internal/alerts/alerts.go

@@ -34,6 +34,43 @@ const (
 	ActivationSnoozed ActivationState = "snoozed"
 )
 
+// Default thresholds and configuration values
+const (
+	// Default threshold values
+	DefaultCPUTrigger     = 80.0
+	DefaultCPUClear       = 75.0
+	DefaultMemoryTrigger  = 85.0
+	DefaultMemoryClear    = 80.0
+	DefaultDiskTrigger    = 90.0
+	DefaultDiskClear      = 85.0
+	DefaultStorageTrigger = 85.0
+	DefaultStorageClear   = 80.0
+	DefaultTempTrigger    = 80.0
+	DefaultTempClear      = 75.0
+
+	// Time thresholds
+	DefaultDelaySeconds      = 5
+	DefaultSuppressionWindow = 5 // minutes
+
+	// Alert management
+	DefaultMinimumDelta      = 2.0 // minimum % change to trigger new alert
+	DefaultHysteresisMargin  = 5.0 // % margin between trigger and clear
+	DefaultObservationWindow = 24  // hours
+
+	// Flapping detection
+	DefaultFlappingWindow   = 300 // seconds (5 minutes)
+	DefaultFlappingThreshold = 5   // state changes to trigger flapping
+	DefaultFlappingCooldown  = 15  // minutes
+
+	// Confirmation counts for transient state detection
+	RequiredOfflineConfirmations = 3
+	RequiredStateConfirmations   = 2
+
+	// Cleanup intervals
+	StaleTrackingThreshold = 24 * time.Hour
+	RateLimitCleanupWindow = 1 * time.Hour
+)
+
 func normalizePoweredOffSeverity(level AlertLevel) AlertLevel {
 	switch strings.ToLower(string(level)) {
 	case string(AlertLevelCritical):
@@ -220,7 +257,7 @@ type GroupingConfig struct {
 type ScheduleConfig struct {
 	QuietHours      QuietHours       `json:"quietHours"`
 	Cooldown        int              `json:"cooldown"`        // minutes
-	GroupingWindow  int              `json:"groupingWindow"`  // seconds (deprecated, use Grouping.Window)
+	GroupingWindow  int              `json:"groupingWindow,omitempty"` // Deprecated: use Grouping.Window instead. Will be auto-migrated on config update.
 	MaxAlertsHour   int              `json:"maxAlertsHour"`   // max alerts per hour per resource
 	NotifyOnResolve bool             `json:"notifyOnResolve"` // Send notification when alert clears
 	Escalation      EscalationConfig `json:"escalation"`
@@ -432,14 +469,15 @@ var (
 )
 
 // SetMetricHooks registers callbacks for recording alert metrics.
-// Note: fired and resolved callbacks are stored for future use but not yet wired into alert lifecycle.
+// - fired: called when an alert is dispatched (in dispatchAlert)
+// - resolved: called when an alert is cleared (in clearAlertNoLock)
+// - suppressed: called when an alert is suppressed due to flapping
+// - acknowledged: called when an alert is acknowledged
 func SetMetricHooks(fired func(*Alert), resolved func(*Alert), suppressed func(string), acknowledged func()) {
 	recordAlertFired = fired
 	recordAlertResolved = resolved
 	recordAlertSuppressed = suppressed
 	recordAlertAcknowledged = acknowledged
-	// Silence staticcheck U1000 - hooks are stored for future metric integration
-	_, _ = recordAlertFired, recordAlertResolved
 }
 
 type Manager struct {
@@ -476,6 +514,8 @@ type Manager struct {
 	// Flapping detection tracking
 	flappingHistory map[string][]time.Time // Track state change times for flapping detection
 	flappingActive  map[string]bool        // Track which alerts are currently in flapping state
+	// Cleanup control
+	cleanupStop chan struct{} // Signal to stop cleanup goroutine
 }
 
 type ackRecord struct {
@@ -514,6 +554,7 @@ func NewManager() *Manager {
 		ackState:              make(map[string]ackRecord),
 		flappingHistory:       make(map[string][]time.Time),
 		flappingActive:        make(map[string]bool),
+		cleanupStop:           make(chan struct{}),
 		config: AlertConfig{
 			Enabled:                true,
 			ActivationState:        ActivationPending,
@@ -608,9 +649,9 @@ func NewManager() *Manager {
 					},
 					Suppress: QuietHoursSuppression{},
 				},
-				Cooldown:        5,  // ON - 5 minutes prevents spam
+				Cooldown:      5,  // ON - 5 minutes prevents spam
-				GroupingWindow:  30, // ON - 30 seconds groups related alerts
+				MaxAlertsHour: 10, // ON - 10 alerts/hour prevents flooding
-				MaxAlertsHour:   10, // ON - 10 alerts/hour prevents flooding
+				// Note: GroupingWindow is deprecated - use Grouping.Window instead
 				NotifyOnResolve: true,
 				Escalation: EscalationConfig{
 					Enabled: false, // OFF - requires user configuration
@@ -650,6 +691,9 @@ func NewManager() *Manager {
 	// Start periodic save of active alerts
 	go m.periodicSaveAlerts()
 
+	// Start periodic cleanup of stale tracking map entries
+	go m.trackingMapCleanup()
+
 	return m
 }
 
@@ -736,12 +780,10 @@ func (m *Manager) safeCallEscalateCallback(alert *Alert, level int) {
 	}(alertCopy, level)
 }
 
-// dispatchAlert delivers an alert to the configured callback, cloning it first to
+// checkFlappingLocked detects alert flapping and returns true if alert should be suppressed.
-// prevent concurrent mutations from racing with consumers.
+// It modifies flappingHistory, flappingActive, and suppressedUntil maps.
-// checkFlapping checks if an alert is flapping (changing state too rapidly)
+// IMPORTANT: Caller MUST hold m.mu before calling this function.
-// checkFlapping detects alert flapping and returns true if alert should be suppressed.
+func (m *Manager) checkFlappingLocked(alertID string) bool {
-// IMPORTANT: Caller MUST hold m.mu (writes to flappingHistory, flappingActive, suppressedUntil maps)
-func (m *Manager) checkFlapping(alertID string) bool {
 	if !m.config.FlappingEnabled {
 		return false
 	}
@@ -795,8 +837,8 @@ func (m *Manager) dispatchAlert(alert *Alert, async bool) bool {
 		return false
 	}
 
-	// Check for flapping
+	// Check for flapping (caller must hold m.mu)
-	if m.checkFlapping(alert.ID) {
+	if m.checkFlappingLocked(alert.ID) {
 		log.Debug().
 			Str("alertID", alert.ID).
 			Msg("Alert suppressed due to flapping")
@@ -831,6 +873,11 @@ func (m *Manager) dispatchAlert(alert *Alert, async bool) bool {
 		return false
 	}
 
+	// Record metric for fired alert
+	if recordAlertFired != nil {
+		recordAlertFired(alert)
+	}
+
 	alertCopy := alert.Clone()
 	if async {
 		go func(a *Alert) {
@@ -903,43 +950,93 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
-	// Preserve defaults for zero values
+	// Normalize all config sections
+	normalizeStorageDefaults(&config)
+	normalizeDockerDefaults(&config)
+	normalizePMGDefaults(&config)
+	normalizeSnapshotDefaults(&config)
+	normalizeBackupDefaults(&config)
+	normalizeNodeDefaults(&config)
+	normalizeHostDefaults(&config)
+	normalizeGeneralSettings(&config)
+	normalizeTimeThresholds(&config)
+
+	config.GuestDefaults.PoweredOffSeverity = normalizePoweredOffSeverity(config.GuestDefaults.PoweredOffSeverity)
+	config.NodeDefaults.PoweredOffSeverity = normalizePoweredOffSeverity(config.NodeDefaults.PoweredOffSeverity)
+	config.DockerIgnoredContainerPrefixes = NormalizeDockerIgnoredPrefixes(config.DockerIgnoredContainerPrefixes)
+
+	// Migration logic for activation state (backward compatibility)
+	m.migrateActivationState(&config)
+
+	// Validate hysteresis thresholds to prevent stuck alerts
+	validateHysteresisThresholds(&config)
+
+	// Validate timezone if quiet hours are enabled
+	validateQuietHoursTimezone(&config)
+
+	m.config = config
+	normalizeOverrides(m.config.Overrides)
+
+	if !m.config.SnapshotDefaults.Enabled {
+		m.clearSnapshotAlertsForInstanceLocked("")
+	}
+	if !m.config.BackupDefaults.Enabled {
+		m.clearBackupAlertsLocked()
+	}
+
+	m.applyGlobalOfflineSettingsLocked()
+
+	log.Info().
+		Bool("enabled", config.Enabled).
+		Interface("guestDefaults", config.GuestDefaults).
+		Msg("Alert configuration updated")
+
+	// Re-evaluate active alerts against new thresholds
+	m.reevaluateActiveAlertsLocked()
+}
+
+// normalizeStorageDefaults ensures storage default thresholds are set
+func normalizeStorageDefaults(config *AlertConfig) {
 	if config.StorageDefault.Trigger <= 0 {
 		config.StorageDefault.Trigger = 85
 		config.StorageDefault.Clear = 80
 	}
+}
 
-	// Initialize Docker defaults while allowing explicit disables (trigger == 0)
+// normalizeDockerThreshold normalizes a single Docker threshold
-	normalizeDockerThreshold := func(th HysteresisThreshold, defaultTrigger float64, metricName string) HysteresisThreshold {
+func normalizeDockerThreshold(th HysteresisThreshold, defaultTrigger float64, metricName string) HysteresisThreshold {
-		normalized := th
+	normalized := th
 
-		// Negative triggers are treated as unset and replaced with defaults.
+	// Negative triggers are treated as unset and replaced with defaults.
-		if normalized.Trigger < 0 {
+	if normalized.Trigger < 0 {
-			normalized.Trigger = defaultTrigger
+		normalized.Trigger = defaultTrigger
+	}
+
+	// Explicit disable: keep trigger at 0 and clamp clear to 0.
+	if normalized.Trigger == 0 {
+		if normalized.Clear < 0 {
+			normalized.Clear = 0
 		}
-
-		// Explicit disable: keep trigger at 0 and clamp clear to 0.
-		if normalized.Trigger == 0 {
-			if normalized.Clear < 0 {
-				normalized.Clear = 0
-			}
-			return normalized
-		}
-
-		if normalized.Clear <= 0 {
-			normalized.Clear = normalized.Trigger - 5
-			if normalized.Clear < 0 {
-				normalized.Clear = 0
-			}
-		}
-
-		ensureValidHysteresis(&normalized, metricName)
 		return normalized
 	}
 
+	if normalized.Clear <= 0 {
+		normalized.Clear = normalized.Trigger - 5
+		if normalized.Clear < 0 {
+			normalized.Clear = 0
+		}
+	}
+
+	ensureValidHysteresis(&normalized, metricName)
+	return normalized
+}
+
+// normalizeDockerDefaults ensures Docker default thresholds are set
+func normalizeDockerDefaults(config *AlertConfig) {
 	config.DockerDefaults.CPU = normalizeDockerThreshold(config.DockerDefaults.CPU, 80, "docker.cpu")
 	config.DockerDefaults.Memory = normalizeDockerThreshold(config.DockerDefaults.Memory, 85, "docker.memory")
 	config.DockerDefaults.Disk = normalizeDockerThreshold(config.DockerDefaults.Disk, 85, "docker.disk")
+
 	if config.DockerDefaults.RestartCount <= 0 {
 		config.DockerDefaults.RestartCount = 3
 	}
@@ -970,8 +1067,10 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 		config.DockerDefaults.StatePoweredOffSeverity = AlertLevelWarning
 	}
 	config.DockerDefaults.StatePoweredOffSeverity = normalizePoweredOffSeverity(config.DockerDefaults.StatePoweredOffSeverity)
+}
 
-	// Initialize PMG defaults if missing/zero
+// normalizePMGDefaults ensures PMG (Proxmox Mail Gateway) defaults are set
+func normalizePMGDefaults(config *AlertConfig) {
 	if config.PMGDefaults.QueueTotalWarning <= 0 {
 		config.PMGDefaults.QueueTotalWarning = 500
 	}
@@ -1020,7 +1119,10 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 	if config.PMGDefaults.QuarantineGrowthCritMin <= 0 {
 		config.PMGDefaults.QuarantineGrowthCritMin = 500
 	}
+}
 
+// normalizeSnapshotDefaults ensures snapshot alert thresholds are valid
+func normalizeSnapshotDefaults(config *AlertConfig) {
 	if config.SnapshotDefaults.WarningDays < 0 {
 		config.SnapshotDefaults.WarningDays = 0
 	}
@@ -1045,6 +1147,10 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 	if config.SnapshotDefaults.CriticalSizeGiB == 0 && config.SnapshotDefaults.WarningSizeGiB > 0 {
 		config.SnapshotDefaults.CriticalSizeGiB = config.SnapshotDefaults.WarningSizeGiB
 	}
+}
+
+// normalizeBackupDefaults ensures backup alert thresholds are valid
+func normalizeBackupDefaults(config *AlertConfig) {
 	if config.BackupDefaults.WarningDays < 0 {
 		config.BackupDefaults.WarningDays = 0
 	}
@@ -1054,18 +1160,10 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 	if config.BackupDefaults.CriticalDays > 0 && config.BackupDefaults.WarningDays > config.BackupDefaults.CriticalDays {
 		config.BackupDefaults.WarningDays = config.BackupDefaults.CriticalDays
 	}
+}
 
-	// Ensure minimums for other important fields
+// normalizeNodeDefaults ensures node threshold defaults exist
-	if config.MinimumDelta <= 0 {
+func normalizeNodeDefaults(config *AlertConfig) {
-		config.MinimumDelta = 2.0
-	}
-	if config.SuppressionWindow <= 0 {
-		config.SuppressionWindow = 5
-	}
-	if config.HysteresisMargin <= 0 {
-		config.HysteresisMargin = 5.0
-	}
-
 	// Ensure temperature defaults exist for nodes so high temps alert out of the box
 	if config.NodeDefaults.Temperature == nil || config.NodeDefaults.Temperature.Trigger <= 0 {
 		config.NodeDefaults.Temperature = &HysteresisThreshold{Trigger: 80, Clear: 75}
@@ -1075,8 +1173,10 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 			config.NodeDefaults.Temperature.Clear = 75
 		}
 	}
+}
 
-	// Ensure host agent defaults exist
+// normalizeHostDefaults ensures host agent threshold defaults exist
+func normalizeHostDefaults(config *AlertConfig) {
 	if config.HostDefaults.CPU == nil || config.HostDefaults.CPU.Trigger <= 0 {
 		config.HostDefaults.CPU = &HysteresisThreshold{Trigger: 80, Clear: 75}
 	} else if config.HostDefaults.CPU.Clear <= 0 {
@@ -1101,8 +1201,26 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 			config.HostDefaults.Disk.Clear = 85
 		}
 	}
+}
 
-	// Normalize any metric-level delay overrides
+// normalizeGeneralSettings ensures general alert settings have valid values
+func normalizeGeneralSettings(config *AlertConfig) {
+	if config.MinimumDelta <= 0 {
+		config.MinimumDelta = 2.0
+	}
+	if config.SuppressionWindow <= 0 {
+		config.SuppressionWindow = 5
+	}
+	if config.HysteresisMargin <= 0 {
+		config.HysteresisMargin = 5.0
+	}
+	if config.ObservationWindowHours <= 0 {
+		config.ObservationWindowHours = 24
+	}
+}
+
+// normalizeTimeThresholds ensures time threshold settings are valid
+func normalizeTimeThresholds(config *AlertConfig) {
 	config.MetricTimeThresholds = normalizeMetricTimeThresholds(config.MetricTimeThresholds)
 
 	const defaultDelaySeconds = 5
@@ -1125,15 +1243,10 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 	if delay, ok := config.TimeThresholds["all"]; ok && delay < 0 {
 		config.TimeThresholds["all"] = defaultDelaySeconds
 	}
-	config.DockerIgnoredContainerPrefixes = NormalizeDockerIgnoredPrefixes(config.DockerIgnoredContainerPrefixes)
+}
 
-	config.GuestDefaults.PoweredOffSeverity = normalizePoweredOffSeverity(config.GuestDefaults.PoweredOffSeverity)
+// migrateActivationState handles backward compatibility for activation state
-	config.NodeDefaults.PoweredOffSeverity = normalizePoweredOffSeverity(config.NodeDefaults.PoweredOffSeverity)
+func (m *Manager) migrateActivationState(config *AlertConfig) {
-
-	// Migration logic for activation state (backward compatibility)
-	if config.ObservationWindowHours <= 0 {
-		config.ObservationWindowHours = 24
-	}
 	if config.ActivationState == "" {
 		// Determine if this is an existing installation or new
 		// Existing installations have active alerts already
@@ -1150,8 +1263,10 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 			log.Info().Msg("New installation: alerts pending activation")
 		}
 	}
+}
 
-	// Validate hysteresis thresholds to prevent stuck alerts
+// validateHysteresisThresholds ensures hysteresis thresholds won't cause stuck alerts
+func validateHysteresisThresholds(config *AlertConfig) {
 	ensureValidHysteresis(config.GuestDefaults.CPU, "guest.cpu")
 	ensureValidHysteresis(config.GuestDefaults.Memory, "guest.memory")
 	ensureValidHysteresis(config.GuestDefaults.Disk, "guest.disk")
@@ -1159,48 +1274,32 @@ func (m *Manager) UpdateConfig(config AlertConfig) {
 	ensureValidHysteresis(config.NodeDefaults.Memory, "node.memory")
 	ensureValidHysteresis(config.NodeDefaults.Temperature, "node.temperature")
 	ensureValidHysteresis(&config.StorageDefault, "storage")
+}
 
-	// Validate timezone if quiet hours are enabled
+// validateQuietHoursTimezone validates the timezone for quiet hours
-	if config.Schedule.QuietHours.Enabled {
+func validateQuietHoursTimezone(config *AlertConfig) {
-		if config.Schedule.QuietHours.Timezone != "" {
+	if config.Schedule.QuietHours.Enabled && config.Schedule.QuietHours.Timezone != "" {
-			_, err := time.LoadLocation(config.Schedule.QuietHours.Timezone)
+		_, err := time.LoadLocation(config.Schedule.QuietHours.Timezone)
-			if err != nil {
+		if err != nil {
-				log.Error().
+			log.Error().
-					Err(err).
+				Err(err).
-					Str("timezone", config.Schedule.QuietHours.Timezone).
+				Str("timezone", config.Schedule.QuietHours.Timezone).
-					Msg("Invalid timezone in quiet hours config, disabling quiet hours")
+				Msg("Invalid timezone in quiet hours config, disabling quiet hours")
-				// Disable quiet hours rather than silently using wrong timezone
+			// Disable quiet hours rather than silently using wrong timezone
-				config.Schedule.QuietHours.Enabled = false
+			config.Schedule.QuietHours.Enabled = false
-			}
 		}
 	}
+}
 
-	m.config = config
+// normalizeOverrides normalizes all threshold overrides
-	for id, override := range m.config.Overrides {
+func normalizeOverrides(overrides map[string]ThresholdConfig) {
+	for id, override := range overrides {
 		override.PoweredOffSeverity = normalizePoweredOffSeverity(override.PoweredOffSeverity)
 		if override.Usage != nil {
 			override.Usage = ensureHysteresisThreshold(override.Usage)
-			m.config.Overrides[id] = override
 		}
-		m.config.Overrides[id] = override
+		overrides[id] = override
 	}
-
-	if !m.config.SnapshotDefaults.Enabled {
-		m.clearSnapshotAlertsForInstanceLocked("")
-	}
-	if !m.config.BackupDefaults.Enabled {
-		m.clearBackupAlertsLocked()
-	}
-
-	m.applyGlobalOfflineSettingsLocked()
-
-	log.Info().
-		Bool("enabled", config.Enabled).
-		Interface("guestDefaults", config.GuestDefaults).
-		Msg("Alert configuration updated")
-
-	// Re-evaluate active alerts against new thresholds
-	m.reevaluateActiveAlertsLocked()
 }
 
 // normalizeMetricTimeThresholds cleans resource/metric keys and drops invalid delay overrides.
@@ -8222,6 +8321,179 @@ func (m *Manager) periodicSaveAlerts() {
 	}
 }
 
+// trackingMapCleanup periodically cleans up stale entries from tracking maps
+// to prevent unbounded memory growth from deleted/decommissioned resources.
+func (m *Manager) trackingMapCleanup() {
+	// Run cleanup every hour
+	ticker := time.NewTicker(1 * time.Hour)
+	defer ticker.Stop()
+
+	for {
+		select {
+		case <-ticker.C:
+			m.cleanupStaleMaps()
+		case <-m.cleanupStop:
+			return
+		}
+	}
+}
+
+// cleanupStaleMaps removes stale entries from tracking maps.
+// Entries are considered stale if they haven't been updated in 24 hours
+// and don't correspond to any active alert.
+func (m *Manager) cleanupStaleMaps() {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	now := time.Now()
+	staleThreshold := StaleTrackingThreshold
+	cleaned := 0
+
+	// Clean up flapping history for resources without active alerts
+	for alertID, history := range m.flappingHistory {
+		if _, hasAlert := m.activeAlerts[alertID]; !hasAlert {
+			// Check if history is stale (last entry older than threshold)
+			if len(history) == 0 || now.Sub(history[len(history)-1]) > staleThreshold {
+				delete(m.flappingHistory, alertID)
+				delete(m.flappingActive, alertID)
+				cleaned++
+			}
+		}
+	}
+
+	// Clean up suppressedUntil entries that have expired
+	for alertID, suppressUntil := range m.suppressedUntil {
+		if now.After(suppressUntil) {
+			delete(m.suppressedUntil, alertID)
+			cleaned++
+		}
+	}
+
+	// Clean up pending alerts older than threshold without active alerts
+	for alertID, pendingTime := range m.pendingAlerts {
+		if _, hasAlert := m.activeAlerts[alertID]; !hasAlert {
+			if now.Sub(pendingTime) > staleThreshold {
+				delete(m.pendingAlerts, alertID)
+				cleaned++
+			}
+		}
+	}
+
+	// Clean up offline confirmation counts for resources without active alerts
+	for resourceID := range m.offlineConfirmations {
+		hasRelatedAlert := false
+		for alertID := range m.activeAlerts {
+			if strings.Contains(alertID, resourceID) {
+				hasRelatedAlert = true
+				break
+			}
+		}
+		if !hasRelatedAlert {
+			delete(m.offlineConfirmations, resourceID)
+			cleaned++
+		}
+	}
+
+	// Clean up node offline counts (legacy)
+	for nodeID := range m.nodeOfflineCount {
+		hasRelatedAlert := false
+		for alertID := range m.activeAlerts {
+			if strings.Contains(alertID, nodeID) {
+				hasRelatedAlert = true
+				break
+			}
+		}
+		if !hasRelatedAlert {
+			delete(m.nodeOfflineCount, nodeID)
+			cleaned++
+		}
+	}
+
+	// Clean up Docker tracking maps
+	for containerID := range m.dockerStateConfirm {
+		hasRelatedAlert := false
+		for alertID := range m.activeAlerts {
+			if strings.Contains(alertID, containerID) {
+				hasRelatedAlert = true
+				break
+			}
+		}
+		if !hasRelatedAlert {
+			delete(m.dockerStateConfirm, containerID)
+			cleaned++
+		}
+	}
+
+	for hostID := range m.dockerOfflineCount {
+		hasRelatedAlert := false
+		for alertID := range m.activeAlerts {
+			if strings.Contains(alertID, hostID) {
+				hasRelatedAlert = true
+				break
+			}
+		}
+		if !hasRelatedAlert {
+			delete(m.dockerOfflineCount, hostID)
+			cleaned++
+		}
+	}
+
+	// Clean up Docker restart tracking for stale containers
+	for containerID, record := range m.dockerRestartTracking {
+		if record != nil && now.Sub(record.lastChecked) > staleThreshold {
+			delete(m.dockerRestartTracking, containerID)
+			delete(m.dockerLastExitCode, containerID)
+			cleaned++
+		}
+	}
+
+	// Clean up rate limit entries older than 1 hour
+	rateLimitThreshold := RateLimitCleanupWindow
+	for resourceID, times := range m.alertRateLimit {
+		// Filter to keep only recent entries
+		var recent []time.Time
+		for _, t := range times {
+			if now.Sub(t) < rateLimitThreshold {
+				recent = append(recent, t)
+			}
+		}
+		if len(recent) == 0 {
+			delete(m.alertRateLimit, resourceID)
+			cleaned++
+		} else if len(recent) < len(times) {
+			m.alertRateLimit[resourceID] = recent
+		}
+	}
+
+	// Clean up recent alerts older than suppression window
+	suppressWindow := time.Duration(m.config.SuppressionWindow) * time.Minute
+	if suppressWindow <= 0 {
+		suppressWindow = 5 * time.Minute
+	}
+	for alertID, alert := range m.recentAlerts {
+		if now.Sub(alert.LastSeen) > suppressWindow {
+			delete(m.recentAlerts, alertID)
+			cleaned++
+		}
+	}
+
+	// Clean up ackState for alerts that no longer exist and are older than threshold
+	for alertID, record := range m.ackState {
+		if _, hasAlert := m.activeAlerts[alertID]; !hasAlert {
+			if now.Sub(record.time) > staleThreshold {
+				delete(m.ackState, alertID)
+				cleaned++
+			}
+		}
+	}
+
+	if cleaned > 0 {
+		log.Debug().
+			Int("entriesCleaned", cleaned).
+			Msg("Cleaned stale entries from alert tracking maps")
+	}
+}
+
 // hasKnownFirmwareBug checks if a disk model is known to have firmware bugs that cause
 // false health status reports. These drives may report FAILED or other error states
 // due to firmware issues (e.g., incorrect temperature thresholds) even when the drive
@@ -8260,24 +8532,25 @@ func (m *Manager) CheckDiskHealth(instance, node string, disk proxmox.Disk) {
 
 	// Check if disk health is not PASSED
 	normalizedHealth := strings.ToUpper(strings.TrimSpace(disk.Health))
-	if normalizedHealth != "" && normalizedHealth != "UNKNOWN" && normalizedHealth != "PASSED" && normalizedHealth != "OK" {
+	healthCheckNeeded := normalizedHealth != "" && normalizedHealth != "UNKNOWN" && normalizedHealth != "PASSED" && normalizedHealth != "OK"
-		// Skip health alerts for drives with known firmware bugs that cause false reports
-		// These drives may report FAILED status due to firmware issues even when healthy
-		// We still monitor wearout below, which is more reliable for these drives
-		if hasKnownFirmwareBug(disk.Model) {
-			log.Debug().
-				Str("node", node).
-				Str("disk", disk.DevPath).
-				Str("model", disk.Model).
-				Str("health", disk.Health).
-				Msg("Skipping health alert for drive with known firmware bug - health status unreliable")
 
-			// Clear any existing health alert since we now recognize this is a false positive
+	// Skip health alerts for drives with known firmware bugs that cause false reports
-			m.clearAlertNoLock(alertID)
+	// These drives may report FAILED status due to firmware issues even when healthy
+	// We still monitor wearout below, which is more reliable for these drives
+	if healthCheckNeeded && hasKnownFirmwareBug(disk.Model) {
+		log.Debug().
+			Str("node", node).
+			Str("disk", disk.DevPath).
+			Str("model", disk.Model).
+			Str("health", disk.Health).
+			Msg("Skipping health alert for drive with known firmware bug - health status unreliable")
 
-			// Continue to wearout check below - wearout monitoring is still valid
+		// Clear any existing health alert since we now recognize this is a false positive
-			goto checkWearout
+		m.clearAlertNoLock(alertID)
-		}
+		healthCheckNeeded = false // Skip to wearout check
+	}
+
+	if healthCheckNeeded {
 		// Check if alert already exists
 		if _, exists := m.activeAlerts[alertID]; !exists {
 			// Create new health alert
@@ -8324,7 +8597,6 @@ func (m *Manager) CheckDiskHealth(instance, node string, disk proxmox.Disk) {
 		m.clearAlertNoLock(alertID)
 	}
 
-checkWearout:
 	// Check for low wearout (SSD life remaining)
 	if disk.Wearout > 0 && disk.Wearout < 10 {
 		wearoutAlertID := fmt.Sprintf("disk-wearout-%s-%s-%s", instance, node, disk.DevPath)
@@ -8403,6 +8675,11 @@ func (m *Manager) clearAlertNoLock(alertID string) {
 		return
 	}
 
+	// Record metric for resolved alert
+	if recordAlertResolved != nil {
+		recordAlertResolved(alert)
+	}
+
 	m.removeActiveAlertNoLock(alertID)
 	resolvedAlert := &ResolvedAlert{
 		Alert:        alert,

internal/alerts/alerts_test.go

@@ -2384,13 +2384,13 @@ func TestCheckFlapping(t *testing.T) {
 			}
 			m.mu.Unlock()
 
-			// Call checkFlapping
+			// Call checkFlappingLocked
 			m.mu.Lock()
-			result := m.checkFlapping(alertID)
+			result := m.checkFlappingLocked(alertID)
 			m.mu.Unlock()
 
 			if result != tt.expectFlapping {
-				t.Errorf("checkFlapping() = %v, want %v", result, tt.expectFlapping)
+				t.Errorf("checkFlappingLocked() = %v, want %v", result, tt.expectFlapping)
 			}
 
 			// Check if flapping was newly detected
@@ -2437,13 +2437,13 @@ func TestCheckFlappingAlreadyFlapping(t *testing.T) {
 	}
 	m.mu.Unlock()
 
-	// Call checkFlapping - should return true but NOT update suppression
+	// Call checkFlappingLocked - should return true but NOT update suppression
 	m.mu.Lock()
-	result := m.checkFlapping(alertID)
+	result := m.checkFlappingLocked(alertID)
 	m.mu.Unlock()
 
 	if !result {
-		t.Errorf("checkFlapping() = false, want true for already flapping alert")
+		t.Errorf("checkFlappingLocked() = false, want true for already flapping alert")
 	}
 
 	// Verify suppression time was NOT updated (existing suppression should remain)
@@ -2479,14 +2479,14 @@ func TestCheckFlappingWindowExpiry(t *testing.T) {
 	}
 	m.mu.Unlock()
 
-	// Call checkFlapping - old entries should be pruned
+	// Call checkFlappingLocked - old entries should be pruned
 	m.mu.Lock()
-	result := m.checkFlapping(alertID)
+	result := m.checkFlappingLocked(alertID)
 	historyLen := len(m.flappingHistory[alertID])
 	m.mu.Unlock()
 
 	if result {
-		t.Errorf("checkFlapping() = true, want false (old entries should be pruned)")
+		t.Errorf("checkFlappingLocked() = true, want false (old entries should be pruned)")
 	}
 
 	// Only the current call should remain in history

internal/alerts/filter_evaluation.go

@@ -20,148 +20,145 @@ func (m *Manager) evaluateFilterCondition(guest interface{}, condition FilterCon
 	}
 }
 
-// evaluateVMCondition evaluates a filter condition against a VM
+// guestMetrics holds common metrics for filter evaluation
-func (m *Manager) evaluateVMCondition(vm models.VM, condition FilterCondition) bool {
+type guestMetrics struct {
+	CPU        float64 // CPU usage as percentage (0-100)
+	MemUsage   float64 // Memory usage percentage
+	DiskUsage  float64 // Disk usage percentage
+	DiskRead   int64   // Bytes/s
+	DiskWrite  int64   // Bytes/s
+	NetworkIn  int64   // Bytes/s
+	NetworkOut int64   // Bytes/s
+	Name       string
+	Node       string
+	ID         string
+	Status     string
+}
+
+// extractGuestMetrics extracts common metrics from a VM or Container
+func extractGuestMetrics(guest interface{}) (guestMetrics, bool) {
+	switch g := guest.(type) {
+	case models.VM:
+		return guestMetrics{
+			CPU:        g.CPU * 100,
+			MemUsage:   g.Memory.Usage,
+			DiskUsage:  g.Disk.Usage,
+			DiskRead:   g.DiskRead,
+			DiskWrite:  g.DiskWrite,
+			NetworkIn:  g.NetworkIn,
+			NetworkOut: g.NetworkOut,
+			Name:       g.Name,
+			Node:       g.Node,
+			ID:         g.ID,
+			Status:     g.Status,
+		}, true
+	case models.Container:
+		return guestMetrics{
+			CPU:        g.CPU * 100,
+			MemUsage:   g.Memory.Usage,
+			DiskUsage:  g.Disk.Usage,
+			DiskRead:   g.DiskRead,
+			DiskWrite:  g.DiskWrite,
+			NetworkIn:  g.NetworkIn,
+			NetworkOut: g.NetworkOut,
+			Name:       g.Name,
+			Node:       g.Node,
+			ID:         g.ID,
+			Status:     g.Status,
+		}, true
+	default:
+		return guestMetrics{}, false
+	}
+}
+
+// evaluateGuestCondition evaluates a filter condition against guest metrics
+func evaluateGuestCondition(metrics guestMetrics, condition FilterCondition) bool {
 	switch condition.Type {
 	case "metric":
 		value := 0.0
 		switch strings.ToLower(condition.Field) {
 		case "cpu":
-			value = vm.CPU * 100
+			value = metrics.CPU
 		case "memory":
-			value = vm.Memory.Usage
+			value = metrics.MemUsage
 		case "disk":
-			value = vm.Disk.Usage
+			value = metrics.DiskUsage
 		case "diskread":
-			value = float64(vm.DiskRead) / 1024 / 1024 // Convert to MB/s
+			value = float64(metrics.DiskRead) / 1024 / 1024 // Convert to MB/s
 		case "diskwrite":
-			value = float64(vm.DiskWrite) / 1024 / 1024
+			value = float64(metrics.DiskWrite) / 1024 / 1024
 		case "networkin":
-			value = float64(vm.NetworkIn) / 1024 / 1024
+			value = float64(metrics.NetworkIn) / 1024 / 1024
 		case "networkout":
-			value = float64(vm.NetworkOut) / 1024 / 1024
+			value = float64(metrics.NetworkOut) / 1024 / 1024
 		default:
 			return false
 		}
-
+		return evaluateNumericCondition(value, condition)
-		condValue, ok := condition.Value.(float64)
-		if !ok {
-			// Try to convert from int
-			if intVal, ok := condition.Value.(int); ok {
-				condValue = float64(intVal)
-			} else {
-				return false
-			}
-		}
-
-		switch condition.Operator {
-		case ">":
-			return value > condValue
-		case "<":
-			return value < condValue
-		case ">=":
-			return value >= condValue
-		case "<=":
-			return value <= condValue
-		case "=", "==":
-			return value >= condValue-0.5 && value <= condValue+0.5
-		}
 
 	case "text":
 		searchValue := strings.ToLower(fmt.Sprintf("%v", condition.Value))
 		switch strings.ToLower(condition.Field) {
 		case "name":
-			return strings.Contains(strings.ToLower(vm.Name), searchValue)
+			return strings.Contains(strings.ToLower(metrics.Name), searchValue)
 		case "node":
-			return strings.Contains(strings.ToLower(vm.Node), searchValue)
+			return strings.Contains(strings.ToLower(metrics.Node), searchValue)
 		case "vmid":
-			return strings.Contains(vm.ID, searchValue)
+			return strings.Contains(metrics.ID, searchValue)
 		}
 
 	case "raw":
 		if condition.RawText != "" {
 			term := strings.ToLower(condition.RawText)
-			return strings.Contains(strings.ToLower(vm.Name), term) ||
+			return strings.Contains(strings.ToLower(metrics.Name), term) ||
-				strings.Contains(vm.ID, term) ||
+				strings.Contains(metrics.ID, term) ||
-				strings.Contains(strings.ToLower(vm.Node), term) ||
+				strings.Contains(strings.ToLower(metrics.Node), term) ||
-				strings.Contains(strings.ToLower(vm.Status), term)
+				strings.Contains(strings.ToLower(metrics.Status), term)
 		}
 	}
 
 	return false
 }
 
-// evaluateContainerCondition evaluates a filter condition against a Container
+// evaluateNumericCondition evaluates a numeric comparison
-func (m *Manager) evaluateContainerCondition(ct models.Container, condition FilterCondition) bool {
+func evaluateNumericCondition(value float64, condition FilterCondition) bool {
-	// Similar logic to evaluateVMCondition but for Container type
+	condValue, ok := condition.Value.(float64)
-	switch condition.Type {
+	if !ok {
-	case "metric":
+		// Try to convert from int
-		value := 0.0
+		if intVal, ok := condition.Value.(int); ok {
-		switch strings.ToLower(condition.Field) {
+			condValue = float64(intVal)
-		case "cpu":
+		} else {
-			value = ct.CPU * 100
-		case "memory":
-			value = ct.Memory.Usage
-		case "disk":
-			value = ct.Disk.Usage
-		case "diskread":
-			value = float64(ct.DiskRead) / 1024 / 1024
-		case "diskwrite":
-			value = float64(ct.DiskWrite) / 1024 / 1024
-		case "networkin":
-			value = float64(ct.NetworkIn) / 1024 / 1024
-		case "networkout":
-			value = float64(ct.NetworkOut) / 1024 / 1024
-		default:
 			return false
 		}
-
-		condValue, ok := condition.Value.(float64)
-		if !ok {
-			if intVal, ok := condition.Value.(int); ok {
-				condValue = float64(intVal)
-			} else {
-				return false
-			}
-		}
-
-		switch condition.Operator {
-		case ">":
-			return value > condValue
-		case "<":
-			return value < condValue
-		case ">=":
-			return value >= condValue
-		case "<=":
-			return value <= condValue
-		case "=", "==":
-			return value >= condValue-0.5 && value <= condValue+0.5
-		}
-
-	case "text":
-		searchValue := strings.ToLower(fmt.Sprintf("%v", condition.Value))
-		switch strings.ToLower(condition.Field) {
-		case "name":
-			return strings.Contains(strings.ToLower(ct.Name), searchValue)
-		case "node":
-			return strings.Contains(strings.ToLower(ct.Node), searchValue)
-		case "vmid":
-			return strings.Contains(ct.ID, searchValue)
-		}
-
-	case "raw":
-		if condition.RawText != "" {
-			term := strings.ToLower(condition.RawText)
-			return strings.Contains(strings.ToLower(ct.Name), term) ||
-				strings.Contains(ct.ID, term) ||
-				strings.Contains(strings.ToLower(ct.Node), term) ||
-				strings.Contains(strings.ToLower(ct.Status), term)
-		}
 	}
 
+	switch condition.Operator {
+	case ">":
+		return value > condValue
+	case "<":
+		return value < condValue
+	case ">=":
+		return value >= condValue
+	case "<=":
+		return value <= condValue
+	case "=", "==":
+		return value >= condValue-0.5 && value <= condValue+0.5
+	}
 	return false
 }
 
+// evaluateVMCondition evaluates a filter condition against a VM
+func (m *Manager) evaluateVMCondition(vm models.VM, condition FilterCondition) bool {
+	metrics, _ := extractGuestMetrics(vm)
+	return evaluateGuestCondition(metrics, condition)
+}
+
+// evaluateContainerCondition evaluates a filter condition against a Container
+func (m *Manager) evaluateContainerCondition(ct models.Container, condition FilterCondition) bool {
+	metrics, _ := extractGuestMetrics(ct)
+	return evaluateGuestCondition(metrics, condition)
+}
+
 // evaluateFilterStack evaluates a filter stack against a guest
 func (m *Manager) evaluateFilterStack(guest interface{}, stack FilterStack) bool {
 	if len(stack.Filters) == 0 {
@@ -270,66 +267,9 @@ func (m *Manager) getGuestThresholds(guest interface{}, guestID string) Threshol
 
 	// If not found, try legacy ID formats for migration
 	if !exists {
-		var legacyID string
+		override, exists = m.tryLegacyOverrideMigration(guest, guestID)
-		var node string
-		var vmid int
-		var instance string
-
-		// Extract node, vmid, and instance from the guest object
-		switch g := guest.(type) {
-		case models.VM:
-			node = g.Node
-			vmid = g.VMID
-			instance = g.Instance
-		case models.Container:
-			node = g.Node
-			vmid = g.VMID
-			instance = g.Instance
-		default:
-			// Not a VM or container, skip legacy migration
-			goto skipLegacyMigration
-		}
-
-		// Try legacy format: instance-node-VMID
-		if instance != node {
-			legacyID = fmt.Sprintf("%s-%s-%d", instance, node, vmid)
-			if legacyOverride, legacyExists := m.config.Overrides[legacyID]; legacyExists {
-				log.Info().
-					Str("legacyID", legacyID).
-					Str("newID", guestID).
-					Msg("Migrating guest override from legacy ID format")
-
-				// Move to new ID
-				m.config.Overrides[guestID] = legacyOverride
-				delete(m.config.Overrides, legacyID)
-
-				// Config will be persisted on next save cycle
-				override = legacyOverride
-				exists = true
-			}
-		}
-
-		// If still not found, try standalone format: node-VMID
-		if !exists && instance == node {
-			legacyID = fmt.Sprintf("%s-%d", node, vmid)
-			if legacyOverride, legacyExists := m.config.Overrides[legacyID]; legacyExists {
-				log.Info().
-					Str("legacyID", legacyID).
-					Str("newID", guestID).
-					Msg("Migrating guest override from legacy standalone ID format")
-
-				// Move to new ID
-				m.config.Overrides[guestID] = legacyOverride
-				delete(m.config.Overrides, legacyID)
-
-				// Config will be persisted on next save cycle
-				override = legacyOverride
-				exists = true
-			}
-		}
 	}
 
-skipLegacyMigration:
 	if exists {
 		// Apply the disabled flag if set
 		if override.Disabled {
@@ -378,3 +318,62 @@ skipLegacyMigration:
 
 	return thresholds
 }
+
+// tryLegacyOverrideMigration attempts to find and migrate legacy override formats.
+// Returns the override and true if found, or zero value and false otherwise.
+func (m *Manager) tryLegacyOverrideMigration(guest interface{}, guestID string) (ThresholdConfig, bool) {
+	var node string
+	var vmid int
+	var instance string
+
+	// Extract node, vmid, and instance from the guest object
+	switch g := guest.(type) {
+	case models.VM:
+		node = g.Node
+		vmid = g.VMID
+		instance = g.Instance
+	case models.Container:
+		node = g.Node
+		vmid = g.VMID
+		instance = g.Instance
+	default:
+		// Not a VM or container, no legacy migration possible
+		return ThresholdConfig{}, false
+	}
+
+	// Try legacy format: instance-node-VMID
+	if instance != node {
+		legacyID := fmt.Sprintf("%s-%s-%d", instance, node, vmid)
+		if legacyOverride, legacyExists := m.config.Overrides[legacyID]; legacyExists {
+			log.Info().
+				Str("legacyID", legacyID).
+				Str("newID", guestID).
+				Msg("Migrating guest override from legacy ID format")
+
+			// Move to new ID
+			m.config.Overrides[guestID] = legacyOverride
+			delete(m.config.Overrides, legacyID)
+
+			return legacyOverride, true
+		}
+	}
+
+	// Try standalone format: node-VMID
+	if instance == node {
+		legacyID := fmt.Sprintf("%s-%d", node, vmid)
+		if legacyOverride, legacyExists := m.config.Overrides[legacyID]; legacyExists {
+			log.Info().
+				Str("legacyID", legacyID).
+				Str("newID", guestID).
+				Msg("Migrating guest override from legacy standalone ID format")
+
+			// Move to new ID
+			m.config.Overrides[guestID] = legacyOverride
+			delete(m.config.Overrides, legacyID)
+
+			return legacyOverride, true
+		}
+	}
+
+	return ThresholdConfig{}, false
+}

internal/alerts/history.go

@@ -192,15 +192,19 @@ func (hm *HistoryManager) saveHistoryWithRetry(maxRetries int) error {
 	historyFile := hm.historyFile
 	backupFile := hm.backupFile
 
+	// Create backup of existing file once before any write attempts.
+	// This ensures we don't lose data if all retries fail.
+	backupCreated := false
+	if _, err := os.Stat(historyFile); err == nil {
+		if err := os.Rename(historyFile, backupFile); err != nil {
+			log.Warn().Err(err).Msg("Failed to create backup file")
+		} else {
+			backupCreated = true
+		}
+	}
+
 	var lastErr error
 	for attempt := 1; attempt <= maxRetries; attempt++ {
-		// Create backup of existing file before writing
-		if _, err := os.Stat(historyFile); err == nil {
-			if err := os.Rename(historyFile, backupFile); err != nil {
-				log.Warn().Err(err).Msg("Failed to create backup file")
-			}
-		}
-
 		// Write new file
 		if err := os.WriteFile(historyFile, data, 0644); err != nil {
 			lastErr = err
@@ -210,13 +214,6 @@ func (hm *HistoryManager) saveHistoryWithRetry(maxRetries int) error {
 				Int("maxRetries", maxRetries).
 				Msg("Failed to write history file, will retry")
 
-			// Restore backup if write failed
-			if _, statErr := os.Stat(backupFile); statErr == nil {
-				if restoreErr := os.Rename(backupFile, historyFile); restoreErr != nil {
-					log.Error().Err(restoreErr).Msg("Failed to restore backup after write failure")
-				}
-			}
-
 			// Exponential backoff: 100ms, 200ms, 400ms
 			if attempt < maxRetries {
 				backoff := time.Duration(100*(1<<uint(attempt-1))) * time.Millisecond
@@ -225,11 +222,23 @@ func (hm *HistoryManager) saveHistoryWithRetry(maxRetries int) error {
 			continue
 		}
 
-		// Success
+		// Success - remove backup file now that we've successfully written
+		if backupCreated {
+			_ = os.Remove(backupFile)
+		}
 		log.Debug().Int("entries", len(snapshot)).Msg("Saved alert history")
 		return nil
 	}
 
+	// All retries failed - restore backup if we have one
+	if backupCreated {
+		if restoreErr := os.Rename(backupFile, historyFile); restoreErr != nil {
+			log.Error().Err(restoreErr).Msg("Failed to restore backup after all write attempts failed")
+		} else {
+			log.Info().Msg("Restored backup after history save failure")
+		}
+	}
+
 	return fmt.Errorf("failed to write history file after %d attempts: %w", maxRetries, lastErr)
 }
 

internal/alerts/history_test.go

@@ -3,6 +3,7 @@ package alerts
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 )
@@ -485,9 +486,9 @@ func TestSaveHistory_CreatesBackup(t *testing.T) {
 		t.Fatalf("saveHistory error: %v", err)
 	}
 
-	// Backup should exist (created from previous file)
+	// Backup should be removed after successful save (backup is only for recovery)
-	if _, err := os.Stat(hm.backupFile); os.IsNotExist(err) {
+	if _, err := os.Stat(hm.backupFile); !os.IsNotExist(err) {
-		t.Error("backup file should exist after save with existing main file")
+		t.Error("backup file should be removed after successful save")
 	}
 }
 
@@ -511,18 +512,19 @@ func TestSaveHistoryWithRetry_CreatesBackup(t *testing.T) {
 		t.Fatalf("saveHistoryWithRetry error: %v", err)
 	}
 
-	// Backup should exist with original content
+	// Backup should be removed after successful save (backup is only for recovery)
-	if _, err := os.Stat(hm.backupFile); os.IsNotExist(err) {
+	if _, err := os.Stat(hm.backupFile); !os.IsNotExist(err) {
-		t.Error("backup file should exist after save")
+		t.Error("backup file should be removed after successful save")
 	}
 
-	backupData, err := os.ReadFile(hm.backupFile)
+	// Verify the main file was written correctly
+	mainData, err := os.ReadFile(hm.historyFile)
 	if err != nil {
-		t.Fatalf("Failed to read backup file: %v", err)
+		t.Fatalf("Failed to read main file: %v", err)
 	}
 
-	if string(backupData) != existingContent {
+	if !strings.Contains(string(mainData), "new-alert") {
-		t.Errorf("backup content = %s, want %s", backupData, existingContent)
+		t.Errorf("main file should contain new-alert, got: %s", mainData)
 	}
 }
 

internal/api/alerts.go

@@ -37,7 +37,9 @@ func (h *AlertHandlers) SetMonitor(m *monitoring.Monitor) {
 	h.monitor = m
 }
 
-// validateAlertID validates an alert ID for security
+// validateAlertID validates an alert ID for security.
+// Alert IDs should be alphanumeric with limited punctuation (hyphens, underscores, colons, periods).
+// This prevents issues with logging, URL encoding, and potential injection attacks.
 func validateAlertID(alertID string) bool {
 	// Guard against empty strings or extremely large payloads that could impact memory usage.
 	if len(alertID) == 0 || len(alertID) > 500 {
@@ -49,14 +51,24 @@ func validateAlertID(alertID string) bool {
 		return false
 	}
 
-	// Permit any printable ASCII character (including spaces) so user supplied identifiers
+	// Allow alphanumeric characters and safe punctuation commonly used in IDs.
-	// like instance names remain valid, while excluding control characters and DEL.
+	// Restrict to a safe character set to avoid issues with logging, URLs, and shell escaping.
 	for _, r := range alertID {
-		if r < 32 || r == 127 {
+		isAlphanumeric := (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9')
+		isSafePunctuation := r == '-' || r == '_' || r == ':' || r == '.' || r == '/' || r == '@'
+		// Allow spaces for backward compatibility with existing alert IDs that include instance names
+		isSpace := r == ' '
+
+		if !isAlphanumeric && !isSafePunctuation && !isSpace {
 			return false
 		}
 	}
 
+	// Reject IDs that start or end with spaces (likely malformed)
+	if alertID[0] == ' ' || alertID[len(alertID)-1] == ' ' {
+		return false
+	}
+
 	return true
 }
 
@@ -85,9 +97,16 @@ func (h *AlertHandlers) UpdateAlertConfig(w http.ResponseWriter, r *http.Request
 	// Update notification manager with schedule settings
 	h.monitor.GetNotificationManager().SetCooldown(config.Schedule.Cooldown)
 
+	// Migrate deprecated GroupingWindow to Grouping.Window if needed
 	groupWindow := config.Schedule.Grouping.Window
 	if groupWindow == 0 && config.Schedule.GroupingWindow != 0 {
 		groupWindow = config.Schedule.GroupingWindow
+		// Migrate the value to the new location
+		config.Schedule.Grouping.Window = groupWindow
+		config.Schedule.GroupingWindow = 0 // Clear deprecated field
+		log.Info().
+			Int("window", groupWindow).
+			Msg("Migrated deprecated GroupingWindow to Grouping.Window")
 	}
 	h.monitor.GetNotificationManager().SetGroupingWindow(groupWindow)
 	h.monitor.GetNotificationManager().SetGroupingOptions(