diff --git a/internal/alerts/active_cleanup.go b/internal/alerts/active_cleanup.go index 16772a8ca..a35c6e5b9 100644 --- a/internal/alerts/active_cleanup.go +++ b/internal/alerts/active_cleanup.go @@ -310,16 +310,7 @@ func (m *Manager) CleanupAlertsForNodes(existingNodes map[string]bool) { if removedCount > 0 { log.Debug().Int("removed", removedCount).Int("remaining", len(m.activeAlerts)).Msg("cleaned up alerts for non-existent nodes") - go func() { - defer func() { - if r := recover(); r != nil { - log.Error().Interface("panic", r).Msg("panic in SaveActiveAlerts goroutine (cleanup)") - } - }() - if err := m.SaveActiveAlerts(); err != nil { - log.Error().Err(err).Msg("failed to save alerts after cleanup") - } - }() + m.saveActiveAlertsAsync("node cleanup") } else { log.Debug().Msg("no alerts needed cleanup") } @@ -358,14 +349,5 @@ func (m *Manager) ClearActiveAlerts() { log.Info().Msg("cleared all active and pending alerts") - go func() { - defer func() { - if r := recover(); r != nil { - log.Error().Interface("panic", r).Msg("panic in SaveActiveAlerts goroutine (clear)") - } - }() - if err := m.SaveActiveAlerts(); err != nil { - log.Error().Err(err).Msg("failed to persist cleared alerts") - } - }() + m.saveActiveAlertsAsync("clear active alerts") } diff --git a/internal/alerts/active_persistence.go b/internal/alerts/active_persistence.go index d04690489..bc28e24f5 100644 --- a/internal/alerts/active_persistence.go +++ b/internal/alerts/active_persistence.go @@ -14,15 +14,30 @@ import ( "github.com/rs/zerolog/log" ) -func asyncSaveActiveAlerts(reason string, save func() error) { +func (m *Manager) saveActiveAlertsAsync(context string) { + m.stopMu.RLock() + if m.stopping { + m.stopMu.RUnlock() + return + } + m.workerWG.Add(1) + m.stopMu.RUnlock() + go func() { + defer m.workerWG.Done() defer func() { if r := recover(); r != nil { - log.Error().Interface("panic", r).Str("reason", reason).Msg("panic in SaveActiveAlerts goroutine") + log.Error(). + Interface("panic", r). + Str("context", context). + Msg("Panic in SaveActiveAlerts goroutine") } }() - if err := save(); err != nil { - log.Error().Err(err).Str("reason", reason).Msg("failed to save active alerts") + if err := m.SaveActiveAlerts(); err != nil { + log.Error(). + Err(err). + Str("context", context). + Msg("Failed to save active alerts") } }() } @@ -107,25 +122,6 @@ func (m *Manager) SaveActiveAlerts() error { return nil } -func (m *Manager) saveActiveAlertsAsync(context string) { - go func() { - defer func() { - if r := recover(); r != nil { - log.Error(). - Interface("panic", r). - Str("context", context). - Msg("Panic in SaveActiveAlerts goroutine") - } - }() - if err := m.SaveActiveAlerts(); err != nil { - log.Error(). - Err(err). - Str("context", context). - Msg("Failed to save active alerts") - } - }() -} - // LoadActiveAlerts restores active alerts from disk. func (m *Manager) LoadActiveAlerts() error { m.mu.Lock() diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index 1d8fc5baa..08edaab96 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -101,28 +101,8 @@ func newTestManager(t *testing.T) *Manager { m := NewManager() // Restore env var and release mutex when test completes. - // We also stop the history manager's background goroutines (but not the - // full manager Stop which includes a 100ms sleep) to prevent writes to - // the temp directory after the test completes. t.Cleanup(func() { - // Stop the history manager to halt background save routines - m.historyManager.Stop() - // Close escalation channel to stop that goroutine too - select { - case <-m.escalationStop: - // Already closed - default: - close(m.escalationStop) - } - // Close cleanup channel - select { - case <-m.cleanupStop: - // Already closed - default: - close(m.cleanupStop) - } - // Brief pause to let goroutines finish any in-flight operations - time.Sleep(10 * time.Millisecond) + m.Stop() if hadOld { os.Setenv("PULSE_DATA_DIR", oldVal) diff --git a/internal/alerts/backup_snapshot.go b/internal/alerts/backup_snapshot.go index 0d4aa4c68..23aaa47eb 100644 --- a/internal/alerts/backup_snapshot.go +++ b/internal/alerts/backup_snapshot.go @@ -525,7 +525,7 @@ func (m *Manager) CheckSnapshotsForInstance(instanceName string, snapshots []mod }, }) if result.Transition != nil && result.Transition.Kind == alertspecs.EvaluationTransitionActivated { - asyncSaveActiveAlerts("snapshot", m.SaveActiveAlerts) + m.saveActiveAlertsAsync("snapshot") } } @@ -913,7 +913,7 @@ func (m *Manager) CheckBackupsWithInventory( }, }) if result.Transition != nil && result.Transition.Kind == alertspecs.EvaluationTransitionActivated { - asyncSaveActiveAlerts("backup", m.SaveActiveAlerts) + m.saveActiveAlertsAsync("backup") } } diff --git a/internal/alerts/canonical_metric.go b/internal/alerts/canonical_metric.go index a3cbc6eed..62bb5b5e9 100644 --- a/internal/alerts/canonical_metric.go +++ b/internal/alerts/canonical_metric.go @@ -113,7 +113,7 @@ func (m *Manager) evaluateCanonicalMetricAlert(spec alertspecs.ResourceAlertSpec migratedAlertIdentity := false defer func() { if migratedAlertIdentity { - asyncSaveActiveAlerts("canonical guest metric node move", m.SaveActiveAlerts) + m.saveActiveAlertsAsync("canonical guest metric node move") } }() defer m.mu.Unlock() @@ -237,7 +237,7 @@ func (m *Manager) evaluateCanonicalMetricAlert(spec alertspecs.ResourceAlertSpec m.recentAlerts[trackingKey] = alert m.historyManager.AddAlert(*alert) - asyncSaveActiveAlerts("canonical metric create", m.SaveActiveAlerts) + m.saveActiveAlertsAsync("canonical metric create") if alertForAICallback := m.getAlertForAICallback(); alertForAICallback != nil { alertCopy := cloneAlertForOutput(alert) @@ -315,7 +315,7 @@ func (m *Manager) evaluateCanonicalMetricAlert(spec alertspecs.ResourceAlertSpec ResolvedTime: observedAt, } m.removeActiveAlertNoLock(storageKey) - asyncSaveActiveAlerts("canonical metric resolution", m.SaveActiveAlerts) + m.saveActiveAlertsAsync("canonical metric resolution") m.addRecentlyResolvedWithPrimaryLock(resolvedAlert) m.safeCallResolvedAlertCallback(existingAlert, storageKey, true) } diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index 53ce1ad19..8be490b27 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -538,16 +538,7 @@ func (m *Manager) reevaluateActiveAlertsLocked() { } if len(alertsToResolve) > 0 { - go func() { - defer func() { - if r := recover(); r != nil { - log.Error().Interface("panic", r).Msg("panic in SaveActiveAlerts goroutine (config update)") - } - }() - if err := m.SaveActiveAlerts(); err != nil { - log.Error().Err(err).Msg("failed to save active alerts after config update") - } - }() + m.saveActiveAlertsAsync("config update") } } diff --git a/internal/alerts/history.go b/internal/alerts/history.go index fd423efe3..4e5bc3480 100644 --- a/internal/alerts/history.go +++ b/internal/alerts/history.go @@ -54,6 +54,7 @@ type HistoryManager struct { saveInterval time.Duration stopChan chan struct{} saveTicker *time.Ticker + workerWG sync.WaitGroup callbacks []AlertCallback // Called when alerts are added } @@ -118,7 +119,11 @@ func NewHistoryManager(dataDir string) *HistoryManager { hm.startPeriodicSave() // Start cleanup routine - go hm.cleanupRoutine() + hm.workerWG.Add(1) + go func() { + defer hm.workerWG.Done() + hm.cleanupRoutine() + }() return hm } @@ -462,7 +467,9 @@ func syncDir(dirPath string) error { func (hm *HistoryManager) startPeriodicSave() { hm.saveTicker = time.NewTicker(hm.saveInterval) + hm.workerWG.Add(1) go func() { + defer hm.workerWG.Done() for { select { case <-hm.saveTicker.C: @@ -643,6 +650,8 @@ func (hm *HistoryManager) Stop() { hm.saveTicker.Stop() } + hm.workerWG.Wait() + // Save one final time if err := hm.saveHistory(); err != nil { log.Error().Err(err).Msg("Failed to save alert history on shutdown") diff --git a/internal/alerts/lifecycle.go b/internal/alerts/lifecycle.go index 72ee3560d..a35386ece 100644 --- a/internal/alerts/lifecycle.go +++ b/internal/alerts/lifecycle.go @@ -1,23 +1,24 @@ package alerts import ( - "time" - "github.com/rs/zerolog/log" ) // Stop stops the alert manager and saves history func (m *Manager) Stop() { m.stopOnce.Do(func() { + m.stopMu.Lock() + m.stopping = true closeSignalChannel(m.escalationStop) closeSignalChannel(m.cleanupStop) + m.stopMu.Unlock() + + m.workerWG.Wait() + if m.historyManager != nil { m.historyManager.Stop() } - // Give background goroutines time to exit cleanly - time.Sleep(100 * time.Millisecond) - // Save active alerts before stopping if err := m.SaveActiveAlerts(); err != nil { log.Error().Err(err).Msg("Failed to save active alerts on stop") diff --git a/internal/alerts/manager.go b/internal/alerts/manager.go index c52137ff7..34f8008e5 100644 --- a/internal/alerts/manager.go +++ b/internal/alerts/manager.go @@ -86,6 +86,9 @@ type Manager struct { quietHoursLoc *time.Location now func() time.Time stopOnce sync.Once + stopMu sync.RWMutex + stopping bool + workerWG sync.WaitGroup } type ackRecord struct { @@ -151,14 +154,20 @@ func NewManagerWithDataDir(dataDir string) *Manager { log.Error().Err(err).Msg("failed to load active alerts") } - // Start escalation checker - go m.escalationChecker() - - // Start periodic save of active alerts - go m.periodicSaveAlerts() - - // Start periodic cleanup of stale tracking map entries - go m.trackingMapCleanup() + // Start background workers. + m.workerWG.Add(3) + go func() { + defer m.workerWG.Done() + m.escalationChecker() + }() + go func() { + defer m.workerWG.Done() + m.periodicSaveAlerts() + }() + go func() { + defer m.workerWG.Done() + m.trackingMapCleanup() + }() return m } diff --git a/internal/alerts/metric_runtime.go b/internal/alerts/metric_runtime.go index 566a974ef..876786d98 100644 --- a/internal/alerts/metric_runtime.go +++ b/internal/alerts/metric_runtime.go @@ -175,7 +175,7 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource migratedAlertIdentity := false defer func() { if migratedAlertIdentity { - asyncSaveActiveAlerts("guest metric node move", m.SaveActiveAlerts) + m.saveActiveAlertsAsync("guest metric node move") } }() defer m.mu.Unlock() @@ -342,17 +342,7 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource m.recentAlerts[trackingKey] = alert m.historyManager.AddAlert(*alert) - // Save active alerts after adding new one - go func() { - defer func() { - if r := recover(); r != nil { - log.Error().Interface("panic", r).Msg("panic in SaveActiveAlerts goroutine") - } - }() - if err := m.SaveActiveAlerts(); err != nil { - log.Error().Err(err).Msg("failed to save active alerts after creation") - } - }() + m.saveActiveAlertsAsync("metric create") log.Warn(). Str("alertID", alertID). @@ -505,17 +495,7 @@ func (m *Manager) checkMetric(resourceID, resourceName, node, instance, resource // Remove from active alerts m.removeActiveAlertNoLock(alertID) - // Save active alerts after resolution - go func() { - defer func() { - if r := recover(); r != nil { - log.Error().Interface("panic", r).Msg("panic in SaveActiveAlerts goroutine (resolution)") - } - }() - if err := m.SaveActiveAlerts(); err != nil { - log.Error().Err(err).Msg("failed to save active alerts after resolution") - } - }() + m.saveActiveAlertsAsync("metric resolution") // Add to recently resolved while preventing lock-order inversions m.addRecentlyResolvedWithPrimaryLock(resolvedAlert) diff --git a/internal/alerts/tracking_cleanup.go b/internal/alerts/tracking_cleanup.go index b07b91458..e7951ec86 100644 --- a/internal/alerts/tracking_cleanup.go +++ b/internal/alerts/tracking_cleanup.go @@ -223,16 +223,7 @@ func (m *Manager) cleanupStaleMaps() { } if staleResolved > 0 { - go func() { - defer func() { - if r := recover(); r != nil { - log.Error().Interface("panic", r).Msg("panic in SaveActiveAlerts goroutine (stale cleanup)") - } - }() - if err := m.SaveActiveAlerts(); err != nil { - log.Error().Err(err).Msg("failed to save active alerts after stale cleanup") - } - }() + m.saveActiveAlertsAsync("stale cleanup") log.Info(). Int("count", staleResolved). Msg("Auto-resolved stale alerts") diff --git a/internal/alerts/unified_eval_parity_test.go b/internal/alerts/unified_eval_parity_test.go index bceabde67..0c620a41d 100644 --- a/internal/alerts/unified_eval_parity_test.go +++ b/internal/alerts/unified_eval_parity_test.go @@ -2,7 +2,6 @@ package alerts import ( "testing" - "time" "github.com/rcourtman/pulse-go-rewrite/internal/models" ) @@ -11,14 +10,8 @@ func newUnifiedEvalParityManager(t *testing.T) *Manager { t.Helper() m := NewManagerWithDataDir(t.TempDir()) - // Stop background workers to avoid post-test writes into temp dirs. t.Cleanup(func() { - if m.historyManager != nil { - m.historyManager.Stop() - } - closeSignalChannel(m.escalationStop) - closeSignalChannel(m.cleanupStop) - time.Sleep(10 * time.Millisecond) + m.Stop() }) return m