Integrate reviewed active alert acknowledgement retention

Change-source: pulse-maintainer

docs/release-control/v6/internal/subsystems/alerts.md

@@ -43,6 +43,21 @@ callbacks remain outside the manager lock. No retention duration is extended.
 sample and a short recovery/refire. This is synthetic lifecycle proof, not
 installed notification-destination acceptance.
 
+Hourly tracking-map cleanup retains canonical acknowledgement records while
+their incident is active, including legacy-keyed active snapshots and active
+recurrences with an older inactive timestamp. A decision does not expire from
+its acknowledgement age while the condition continues. Recovery starts the
+existing inactive-retention window; the ordinary cleanup's one-hour expiry,
+hourly stale-record fallback and explicit unacknowledgement remain unchanged.
+`TestTrackingCleanupPreservesAcknowledgedProviderRecurrence` in
+`internal/alerts/reducer_parity_ack_test.go` verifies manual and automatic
+acknowledgement through provider-native pool incident reconciliation, affirmative
+recovery, short recurrence, dispatch callbacks and JSON/durable checkpoint restart.
+`TestTrackingCleanupCanonicalAckRetentionBounds` verifies active/legacy identity,
+recent inactivity, both existing expiry paths and missing-timestamp fallback.
+Modeled elapsed time and local callbacks do not prove installed recovery or
+delivery to an external notification destination.
+
 Mobile incident drawers transfer their exact context to Assistant and close
 through the shared explicit handoff callback. Keeping the source drawer above
 Assistant, or dropping its occurrence identity to make navigation work, fails

internal/alerts/history_lifecycle_test.go

@@ -1,10 +1,12 @@
 package alerts
 
 import (
+	"encoding/json"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
+	"testing/synctest"
 	"time"
 )
 
@@ -97,25 +99,54 @@ func TestCleanupRoutine_ReturnsImmediatelyWhenStopped(t *testing.T) {
 }
 
 func TestStartPeriodicSave_PersistsHistoryOnTicker(t *testing.T) {
-	hm := newTestHistoryManager(t)
-	hm.saveInterval = 10 * time.Millisecond
-	hm.history = []HistoryEntry{
-		{Alert: Alert{ID: "periodic-save-alert"}, Timestamp: time.Now()},
-	}
-
-	hm.startPeriodicSave()
-	// Stop waits for the save worker, so an in-flight save cannot race the
-	// TempDir cleanup.
-	defer hm.Stop()
-
-	deadline := time.Now().Add(750 * time.Millisecond)
-	for time.Now().Before(deadline) {
-		data, err := os.ReadFile(hm.historyFile)
-		if err == nil && strings.Contains(string(data), "periodic-save-alert") {
-			return
+	// Exercise the real ticker and persistence worker using virtual time.
+	// The old 750 ms filesystem-poll deadline measured wall-clock scheduling
+	// and I/O as well as periodic-save behaviour. Wait drains each tick's real
+	// file I/O before assertions, without adding a production callback or
+	// letting Stop's final save satisfy the periodic-save assertion.
+	synctest.Test(t, func(t *testing.T) {
+		hm := newTestHistoryManager(t)
+		hm.history = []HistoryEntry{
+			{Alert: Alert{ID: "periodic-save-alert"}, Timestamp: time.Now()},
 		}
-		time.Sleep(15 * time.Millisecond)
-	}
+		hm.startPeriodicSave()
+		defer hm.Stop()
 
-	t.Fatal("periodic save did not persist history data before deadline")
+		synctest.Wait()
+		if _, err := os.Stat(hm.historyFile); !os.IsNotExist(err) {
+			t.Fatalf("history written before first tick: %v", err)
+		}
+		assertPersisted := func(wantIDs ...string) {
+			t.Helper()
+			data, err := os.ReadFile(hm.historyFile)
+			if err != nil {
+				t.Fatalf("read periodic history: %v", err)
+			}
+			var entries []HistoryEntry
+			if err := json.Unmarshal(data, &entries); err != nil {
+				t.Fatalf("decode periodic history: %v", err)
+			}
+			if len(entries) != len(wantIDs) {
+				t.Fatalf("periodic history has %d entries, want %d", len(entries), len(wantIDs))
+			}
+			for i, id := range wantIDs {
+				if entries[i].Alert.ID != id {
+					t.Errorf("entry %d ID = %q, want %q", i, entries[i].Alert.ID, id)
+				}
+			}
+		}
+
+		time.Sleep(hm.saveInterval)
+		synctest.Wait()
+		assertPersisted("periodic-save-alert")
+
+		hm.mu.Lock()
+		hm.history = append(hm.history, HistoryEntry{
+			Alert: Alert{ID: "second-tick-alert"}, Timestamp: time.Now(),
+		})
+		hm.mu.Unlock()
+		time.Sleep(hm.saveInterval)
+		synctest.Wait()
+		assertPersisted("periodic-save-alert", "second-tick-alert")
+	})
 }

internal/alerts/reducer_parity_ack_test.go

@@ -2,11 +2,13 @@ package alerts
 
 import (
 	"fmt"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/rcourtman/pulse-go-rewrite/internal/alerts/reducer"
 	alertspecs "github.com/rcourtman/pulse-go-rewrite/internal/alerts/specs"
+	"github.com/rcourtman/pulse-go-rewrite/internal/storagehealth"
 	"github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources"
 )
 
@@ -262,3 +264,173 @@ func TestAutoAcknowledgementSurvivesMetricEvaluation(t *testing.T) {
 	evaluate(90)
 	assertAcknowledged("short recovery and refire")
 }
+
+// Age only the acknowledgement timestamps, keeping detector observations fresh.
+// The hourly sweep must not discard a long-running operator decision before a
+// real recovery can start the existing inactive-retention window.
+func TestTrackingCleanupPreservesAcknowledgedProviderRecurrence(t *testing.T) {
+	for _, durable := range []bool{false, true} {
+		for _, automatic := range []bool{false, true} {
+			name := "JSON"
+			if durable {
+				name = "durable"
+			}
+			if automatic {
+				name += "/automatic"
+			} else {
+				name += "/manual"
+			}
+			t.Run(name, func(t *testing.T) {
+				dir := t.TempDir()
+				newManager := func() *Manager {
+					return newCleanupRetentionManager(t, dir, durable)
+				}
+				m := newManager()
+				resource := unifiedresources.Resource{
+					ID: "storage:ack-tank", Type: unifiedresources.ResourceTypeStorage,
+					Name: "ack-tank", ParentName: "truenas-main",
+					Sources: []unifiedresources.DataSource{unifiedresources.SourceTrueNAS},
+					Storage: &unifiedresources.StorageMeta{Platform: "truenas", Topology: "pool", Protection: "zfs", IsZFS: true},
+					Incidents: []unifiedresources.ResourceIncident{{
+						Provider: "truenas", NativeID: "ack-pool-alert", Code: "truenas_volume_status",
+						Severity: storagehealth.RiskWarning, Summary: "Pool ack-tank is DEGRADED",
+						StartedAt: time.Now().Add(-26 * time.Hour),
+					}},
+				}
+				healthy := resource
+				healthy.Incidents = nil
+				evaluate := func(m *Manager, observed unifiedresources.Resource) {
+					m.SyncUnifiedResourceIncidents([]unifiedresources.Resource{observed})
+				}
+				var deliveries atomic.Int32
+				m.SetAlertCallback(func(*Alert) { deliveries.Add(1) })
+				evaluate(m, resource)
+				initial := m.GetActiveAlerts()
+				if len(initial) != 1 || deliveries.Load() != 1 {
+					t.Fatalf("initial provider condition: %d alerts / %d dispatches, want 1 / 1", len(initial), deliveries.Load())
+				}
+				id := initial[0].ID
+				user := "operator"
+				if automatic {
+					user = "system-auto"
+					m.mu.Lock()
+					m.config.AutoAcknowledgeAfterHours = 1
+					m.mu.Unlock()
+					m.Cleanup(time.Hour)
+				} else if err := m.AcknowledgeAlert(id, user); err != nil {
+					t.Fatal(err)
+				}
+				ackAt := time.Now().Add(-25 * time.Hour)
+				m.mu.Lock()
+				active, ok := m.getActiveAlertNoLock(id)
+				if !ok || !active.Acknowledged {
+					m.mu.Unlock()
+					t.Fatal("detector acknowledgement was not established")
+				}
+				active.AckTime = &ackAt
+				record := m.ackStateByCanonical[id]
+				record.time = ackAt
+				m.ackStateByCanonical[id] = record
+				m.mirrorAcknowledgeNoLock(active, user, ackAt)
+				m.setActiveAlertNoLock(id, active)
+				m.mu.Unlock()
+
+				checkCycle := func(m *Manager, count *atomic.Int32, stage string) {
+					t.Helper()
+					before := count.Load()
+					m.cleanupStaleMaps()
+					m.mu.RLock()
+					record, retained := m.ackStateByCanonical[id]
+					m.mu.RUnlock()
+					if !retained || record.user != user || !record.time.Equal(ackAt) {
+						t.Errorf("%s: hourly sweep lost the active acknowledgement: %+v, retained=%v", stage, record, retained)
+					}
+					evaluate(m, healthy) // Observed pool with no incident: affirmative recovery.
+					if got := len(m.GetActiveAlerts()); got != 0 {
+						t.Fatalf("%s: real recovery left %d active alerts", stage, got)
+					}
+					m.Cleanup(time.Hour) // Old acknowledgement, newly inactive: retain it.
+					evaluate(m, resource)
+					alerts := m.GetActiveAlerts()
+					if len(alerts) != 1 || alerts[0].ID != id || !alerts[0].Acknowledged || alerts[0].AckUser != user || alerts[0].AckTime == nil || !alerts[0].AckTime.Equal(ackAt) {
+						t.Errorf("%s: short recurrence lost acknowledgement identity: %+v", stage, alerts)
+					}
+					if got := count.Load() - before; got != 0 {
+						t.Errorf("%s: acknowledged recovery/recurrence dispatched %d duplicate notifications", stage, got)
+					}
+					t.Logf("%s: recovery/recurrence dispatch delta %d", stage, count.Load()-before)
+				}
+				checkCycle(m, &deliveries, "before restart")
+				m.Stop() // The actual final checkpoint, not a hand-built JSON fixture.
+				restarted := newManager()
+				var afterRestart atomic.Int32
+				restarted.SetAlertCallback(func(*Alert) { afterRestart.Add(1) })
+				checkCycle(restarted, &afterRestart, "after restart")
+				if err := restarted.UnacknowledgeAlert(id); err != nil {
+					t.Fatal(err)
+				}
+				before := afterRestart.Load()
+				evaluate(restarted, healthy)
+				evaluate(restarted, resource)
+				if got := afterRestart.Load() - before; got != 1 {
+					t.Errorf("explicit unacknowledge/recovery/recurrence dispatched %d notifications, want 1", got)
+				}
+			})
+		}
+	}
+}
+
+func TestTrackingCleanupCanonicalAckRetentionBounds(t *testing.T) {
+	for _, tc := range []struct {
+		name       string
+		active     bool
+		legacy     bool
+		inactive   time.Duration
+		wantHourly bool
+		wantNormal bool
+	}{
+		{name: "active canonical", active: true, wantHourly: true, wantNormal: true},
+		{name: "active legacy identity", active: true, legacy: true, wantHourly: true, wantNormal: true},
+		{name: "active after earlier recurrence", active: true, inactive: 25 * time.Hour, wantHourly: true, wantNormal: true},
+		{name: "recently inactive old acknowledgement", inactive: 30 * time.Minute, wantHourly: true, wantNormal: true},
+		{name: "existing one-hour expiry", inactive: 2 * time.Hour, wantHourly: true},
+		{name: "stale inactive", inactive: 25 * time.Hour},
+		{name: "legacy missing inactive timestamp"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			m := newTestManager(t)
+			now := time.Now()
+			old := now.Add(-25 * time.Hour)
+			id, alert := testNewCanonicalAlert("ack-bounds", "metric-threshold:cpu", "metric-threshold", "cpu")
+			alert.StartTime, alert.LastSeen = old, now
+			alert.Acknowledged, alert.AckUser, alert.AckTime = true, "operator", &old
+			record := ackRecord{acknowledged: true, user: "operator", time: old}
+			if tc.inactive > 0 {
+				record.inactiveAt = now.Add(-tc.inactive)
+			}
+			m.mu.Lock()
+			m.ackStateByCanonical[id] = record
+			if tc.active {
+				storageKey := id
+				if tc.legacy {
+					storageKey = "legacy-cpu-ack-bounds"
+				}
+				m.activeAlerts[storageKey] = alert
+			}
+			m.mu.Unlock()
+			assertRetained := func(stage string, want bool) {
+				t.Helper()
+				m.mu.RLock()
+				got, retained := m.ackStateByCanonical[id]
+				m.mu.RUnlock()
+				if retained != want || (retained && got != record) {
+					t.Errorf("%s: acknowledgement retained=%v (%+v), want %v with unchanged record", stage, retained, got, want)
+				}
+			}
+			m.cleanupStaleMaps()
+			assertRetained("hourly sweep", tc.wantHourly)
+			m.Cleanup(time.Hour)
+			assertRetained("normal one-hour inactive expiry", tc.wantNormal)
+		})
+	}
+}

internal/alerts/tracking_cleanup.go

@@ -139,6 +139,11 @@ func (m *Manager) cleanupStaleMaps() {
 		}
 	}
 	for canonicalID, record := range m.ackStateByCanonical {
+		// As with legacy records and Cleanup, an active incident owns its
+		// acknowledgement until recovery starts the inactive-retention window.
+		if m.hasActiveAlertTrackingKeyNoLock(canonicalID) {
+			continue
+		}
 		checkTime := record.inactiveAt
 		if checkTime.IsZero() {
 			checkTime = record.time