diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index cb3ae9b561..d1278fa3b0 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -780,6 +780,25 @@ must migrate through `internal/alerts/operational_contract.go` and name their limited provenance honestly rather than inventing confirmed provider evidence. Acknowledgement remains distinct from resolution, and every resolution transition references recovery evidence separate from its trigger evidence. +A resolved alert may carry a typed `AlertResolution` +(`internal/alerts/resolution.go`) when it closed without its condition being +observed to clear. The one reason so far, `moved_to_agent`, describes a metric +alert whose resource's linked Pulse agent now owns the metric. +`releaseCanonicalMetricAlert` is the disabled-threshold release extracted from +`evaluateCanonicalMetricAlert`: a caller releasing a metric for any other +reason passes a resolution, and `clearAlertWithResolution` stamps a copy on the +removed alert before it is resolved. The resolved callback snapshot, the +history row, the resolved event-log entry (the reason code, with `Summary()` as +its message) and the Alert JSON then carry it. Resolved consumers still receive +the close, because integrations close their incident by alert ID. The +synthesized closing evidence carries the resolution reason and summary instead +of `legacy_alert_recovery_projection`, and the resolve transition carries the +summary as its `reason`. The cause stays `recovery_evidence`, the only cause +operational trust lets enter `resolved`. A nil resolution leaves the +disabled-threshold path and every recovery unchanged. +`TestReleasedMetricAlertCarriesResolutionInsteadOfRecovery` and +`TestResolvedHistoryRowKeepsResolutionWithoutEventLog` in +`internal/alerts/operational_contract_test.go` pin this. ## Extension Points diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 637c689ced..0afc28d582 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -1216,3 +1216,35 @@ The live monitor does not interpret pending-only cancellation counts as recipient acceptance. Per-occurrence/per-destination receipts remain the sole recovery admission proof, including partial delivery and lost RAM markers after restart; an unannounced destination receives no recovery. + +### A close that was not a recovery says what happened + +A resolved alert can carry an `alerts.AlertResolution` when it closed without +recovering. The one reason so far, `moved_to_agent`, means the metric now +belongs to the resource's linked Pulse agent. Delivery is unchanged: the close +is still queued, gated on the firing receipt and sent, and PagerDuty still +receives `resolve` for the alert's dedup key. Only the wording changes. +`resolvedAlertMessage` gives the resolution's summary with the alert named +("Memory alert moved to pve1 (Host Agent). This is not a recovery: check the +agent for the current reading.") in place of " on is now +healthy" for every templated webhook `.Message`. Grouped lists append the +short outcome ("moved to pve1 (Host Agent)") to the moved entry. ntfy titles a +single moved alert `MOVED: `. Email and Apprise title it `Pulse alert +moved: ` and add the summary after the breach message. Recovery +visuals follow the same rule: when any close in a resolved delivery was not a +recovery, `WebhookPayloadData.NotRecovered` is set and the built-in templates +drop them (Discord's green embed becomes the neutral blue, both Teams cards +lose recovery green, Mattermost's check emoji becomes `:arrow_right:`), and +ntfy swaps its green check tag for `arrow_right`, for a single alert or a batch. +Built-in titles still say Resolved, which is accurate for the closed alert; +custom templates can read `.NotRecovered`, and one that hard-codes recovery +text keeps it. The resolution rides the Alert JSON, so it survives the delivery +queue round trip and appears in the default JSON payload's `alerts` array. +Issue #1682 is the precedent: a recipient called "is now healthy" a false +factual claim when an alert closed without recovering. + +Verification: `resolved_handover_test.go` drives the firing, its receipt and +the queued close through the normal entry points for ntfy, and renders Discord, +both Teams cards, Mattermost, PagerDuty, a grouped Slack list, a grouped ntfy +batch and email/Apprise content, plus the unchanged ordinary recovery wording +and visuals. diff --git a/internal/alerts/active_lifecycle.go b/internal/alerts/active_lifecycle.go index c2d150d324..9d9649e14b 100644 --- a/internal/alerts/active_lifecycle.go +++ b/internal/alerts/active_lifecycle.go @@ -98,6 +98,12 @@ func (m *Manager) addRecentlyResolvedWithPrimaryLock(resolved *ResolvedAlert) { // clearAlert removes an alert if it exists. func (m *Manager) clearAlert(alertID string) { + m.clearAlertWithResolution(alertID, nil) +} + +// clearAlertWithResolution removes an alert if it exists. A non-nil +// resolution marks the close as something other than a recovery. +func (m *Manager) clearAlertWithResolution(alertID string, resolution *AlertResolution) { m.mu.Lock() alert, exists := m.getActiveAlertNoLock(alertID) if exists { @@ -111,6 +117,7 @@ func (m *Manager) clearAlert(alertID string) { } publicID := effectiveAlertID(alert, alertID) + alert.Resolution = resolution.Clone() resolvedAlert := m.newResolvedAlert(alert, time.Now(), nil) m.addRecentlyResolvedUnlocked(resolvedAlert) diff --git a/internal/alerts/callbacks.go b/internal/alerts/callbacks.go index 4a6a7a9d05..ccf0a8b64c 100644 --- a/internal/alerts/callbacks.go +++ b/internal/alerts/callbacks.go @@ -430,7 +430,13 @@ func (m *Manager) safeCallResolvedAlertCallback(resolved *ResolvedAlert, fallbac } // Record the resolution before the callback guard: this funnel is the // resolve seam for every lifecycle path, with or without subscribers. - m.recordAlertEvent(eventlog.TypeResolved, alert, fallbackID, "", "Alert resolved.", nil) + eventReason, eventMessage := "", "Alert resolved." + if alert != nil { + if summary := alert.Resolution.Summary(); summary != "" { + eventReason, eventMessage = string(alert.Resolution.Reason), summary + } + } + m.recordAlertEvent(eventlog.TypeResolved, alert, fallbackID, eventReason, eventMessage, nil) callbacks := m.getResolvedCallbacks() resolvedCallback := m.callbacks.resolvedAlertCallback() diff --git a/internal/alerts/canonical_metric.go b/internal/alerts/canonical_metric.go index 7e3a76580b..53bc013063 100644 --- a/internal/alerts/canonical_metric.go +++ b/internal/alerts/canonical_metric.go @@ -76,6 +76,30 @@ func formatMetricValue(value float64, suffix string) string { return fmt.Sprintf("%.1f%s", value, suffix) } +// releaseCanonicalMetricAlert stops evaluating a metric spec the way a +// disabled threshold does: any pending run is dropped and any open alert is +// resolved. A non-nil resolution says why the close was not a recovery, and +// travels with the resolved alert to every consumer. +func (m *Manager) releaseCanonicalMetricAlert(spec alertspecs.ResourceAlertSpec, resourceName, node, instance, resourceType string, value float64, resolution *AlertResolution) { + if spec.MetricThreshold == nil { + return + } + storageKey := canonicalTrackingKeyForSpec(spec, spec.ID) + m.mu.Lock() + m.core.ApplyMetric(reducer.MetricSignal{ + ResourceID: spec.ResourceID, + Key: spec.ID, + Metric: spec.MetricThreshold.Metric, + Value: value, + ObservedAt: time.Now(), + }, reducer.MetricRule{}) + m.mu.Unlock() + // A guest that moved nodes may hold this alert under its old node-scoped + // identity; re-home it first so the clear below can resolve it. + m.rehomeStrandedGuestAlert(storageKey, spec.ID, string(spec.Kind), spec.ResourceID, resourceName, node, instance, resourceType) + m.clearAlertWithResolution(storageKey, resolution) +} + func (m *Manager) evaluateCanonicalMetricAlert(spec alertspecs.ResourceAlertSpec, resourceName, node, instance, resourceType string, value float64, threshold *HysteresisThreshold, opts *metricOptions) { if spec.MetricThreshold == nil { return @@ -86,19 +110,7 @@ func (m *Manager) evaluateCanonicalMetricAlert(spec alertspecs.ResourceAlertSpec trackingKey := storageKey metricType := spec.MetricThreshold.Metric if spec.Disabled || spec.MetricThreshold.Trigger <= 0 { - m.mu.Lock() - m.core.ApplyMetric(reducer.MetricSignal{ - ResourceID: spec.ResourceID, - Key: spec.ID, - Metric: metricType, - Value: value, - ObservedAt: time.Now(), - }, reducer.MetricRule{}) - m.mu.Unlock() - // A guest that moved nodes may hold this alert under its old node-scoped - // identity; re-home it first so the clear below can resolve it. - m.rehomeStrandedGuestAlert(storageKey, spec.ID, string(spec.Kind), spec.ResourceID, resourceName, node, instance, resourceType) - m.clearAlert(storageKey) + m.releaseCanonicalMetricAlert(spec, resourceName, node, instance, resourceType, value, nil) return } diff --git a/internal/alerts/history.go b/internal/alerts/history.go index 10d38cd244..efefa385cb 100644 --- a/internal/alerts/history.go +++ b/internal/alerts/history.go @@ -394,6 +394,7 @@ func (hm *HistoryManager) UpdateAlertOperationalContractForAlert(alert *Alert) { hm.history[i].Alert.LatestTransition = snapshot.LatestTransition hm.history[i].Alert.Transitions = snapshot.Transitions hm.history[i].Alert.Evidence = snapshot.Evidence + hm.history[i].Alert.Resolution = snapshot.Resolution // The live alert is authoritative for acknowledgement; a row // that keeps its fire-time ack state shows an acknowledged // alert as unacknowledged for its whole active life. diff --git a/internal/alerts/model.go b/internal/alerts/model.go index 4b6324a2fd..cce963c444 100644 --- a/internal/alerts/model.go +++ b/internal/alerts/model.go @@ -136,6 +136,7 @@ type Alert struct { AckTime *time.Time `json:"ackTime,omitempty"` AckUser string `json:"ackUser,omitempty"` Correlation *AlertCorrelation `json:"correlation,omitempty"` + Resolution *AlertResolution `json:"resolution,omitempty"` // set only when a close was not a recovery Metadata map[string]interface{} `json:"metadata,omitempty"` LastNotified *time.Time `json:"lastNotified,omitempty"` LastEscalation int `json:"lastEscalation,omitempty"` @@ -168,6 +169,8 @@ func (a *Alert) Clone() *Alert { clone.Correlation = cloneAlertCorrelation(a.Correlation) } + clone.Resolution = a.Resolution.Clone() + if len(a.EscalationTimes) > 0 { clone.EscalationTimes = append([]time.Time(nil), a.EscalationTimes...) } diff --git a/internal/alerts/operational_contract.go b/internal/alerts/operational_contract.go index 0923383a8d..f59fe89a1b 100644 --- a/internal/alerts/operational_contract.go +++ b/internal/alerts/operational_contract.go @@ -311,6 +311,7 @@ func markOperationalResolved( Cause: operationaltrust.TransitionRecoveryEvidence, CauseKey: record.CauseKey, EvidenceIDs: recoveryEvidenceIDs, + Reason: alert.Resolution.Summary(), } alert.Transitions = appendOperationalTransition( alert.Transitions, @@ -497,6 +498,16 @@ func legacyAlertRecoveryEvidenceEnvelope( if err != nil { return operationaltrust.EvidenceEnvelope{}, false } + reason := &operationaltrust.EvidenceReason{ + Code: legacyAlertRecoveryEvidenceReason, + } + // A close that was not a recovery must not be recorded as one. + if summary := alert.Resolution.Summary(); summary != "" { + reason = &operationaltrust.EvidenceReason{ + Code: string(alert.Resolution.Reason), + Message: summary, + } + } envelope := operationaltrust.EvidenceEnvelope{ ID: id, Source: source, @@ -506,9 +517,7 @@ func legacyAlertRecoveryEvidenceEnvelope( Completeness: operationaltrust.EvidencePartial, Confidence: operationaltrust.EvidenceUnknown, Permissions: operationaltrust.EvidencePermissionsUnknown, - Reason: &operationaltrust.EvidenceReason{ - Code: legacyAlertRecoveryEvidenceReason, - }, + Reason: reason, } return envelope, envelope.Validate() == nil } diff --git a/internal/alerts/operational_contract_test.go b/internal/alerts/operational_contract_test.go index 56cee31783..f483dd11b0 100644 --- a/internal/alerts/operational_contract_test.go +++ b/internal/alerts/operational_contract_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/rcourtman/pulse-go-rewrite/internal/alerts/eventlog" alertspecs "github.com/rcourtman/pulse-go-rewrite/internal/alerts/specs" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/operationaltrust" @@ -412,6 +413,162 @@ func TestResolvedOperationalContractUpdatesHistorySnapshot(t *testing.T) { } } +func TestReleasedMetricAlertCarriesResolutionInsteadOfRecovery(t *testing.T) { + manager := newUnifiedEvalParityManager(t) + store, err := eventlog.OpenInMemory() + if err != nil { + t.Fatalf("open in-memory event log: %v", err) + } + manager.SetEventLog(store) + manager.UpdateConfig(AlertConfig{ + Enabled: true, + NodeDefaults: ThresholdConfig{ + CPU: &HysteresisThreshold{Trigger: 80, Clear: 75}, + Memory: &HysteresisThreshold{Trigger: 85, Clear: 80}, + }, + }) + disableTestTimeThresholds(manager) + node := models.Node{ + ID: "pve-1", + Name: "pve-1", + Instance: "test", + Status: "online", + CPU: 0.95, + Memory: models.Memory{Total: 8 << 30, Used: 7 << 30, Usage: 95}, + } + manager.CheckNode(node) + if active := manager.GetActiveAlerts(); len(active) != 2 { + t.Fatalf("active alerts = %#v, want cpu and memory", active) + } + + resolvedByType := map[string]*ResolvedAlert{} + manager.SetResolvedAlertCallback(func(resolved *ResolvedAlert) { + resolvedByType[resolved.Alert.Type] = resolved + }) + release := func(metric string, resolution *AlertResolution) { + t.Helper() + spec, err := buildCanonicalMetricSpec(node.ID, node.Name, unifiedresources.ResourceType("node"), metric, nil) + if err != nil { + t.Fatalf("build %s spec: %v", metric, err) + } + manager.releaseCanonicalMetricAlert(spec, node.Name, node.Name, node.Instance, "node", 0, resolution) + } + + handover := &AlertResolution{ + Reason: AlertResolutionMovedToAgent, + SuccessorResourceID: "agent-pve-1", + SuccessorName: "pve-1 (Host Agent)", + } + release("memory", handover) + // The close is recorded as a copy: the caller's value cannot rewrite it. + handover.SuccessorName = "changed" + release("cpu", nil) + + if active := manager.GetActiveAlerts(); len(active) != 0 { + t.Fatalf("active alerts after release = %#v, want none: the close must still happen", active) + } + + moved := resolvedByType["memory"] + if moved == nil { + t.Fatal("memory release did not reach the resolved consumer") + } + resolution := moved.Alert.Resolution + if resolution == nil || + resolution.Reason != AlertResolutionMovedToAgent || + resolution.SuccessorResourceID != "agent-pve-1" || + resolution.SuccessorName != "pve-1 (Host Agent)" { + t.Fatalf("resolution = %+v, want moved to pve-1 (Host Agent)", resolution) + } + wantSummary := "Alert moved to pve-1 (Host Agent). This is not a recovery: check the agent for the current reading." + if got := resolution.Summary(); got != wantSummary { + t.Fatalf("Summary() = %q, want %q", got, wantSummary) + } + if got := resolution.Outcome(); got != "moved to pve-1 (Host Agent)" { + t.Fatalf("Outcome() = %q", got) + } + if got := resolution.Describe("Memory alert"); got != "Memory alert moved to pve-1 (Host Agent). This is not a recovery: check the agent for the current reading." { + t.Fatalf("Describe() = %q", got) + } + + record := moved.Alert.OperationalRecord + if record == nil || record.State != operationaltrust.OperationalResolved { + t.Fatalf("operational record = %+v, want resolved", record) + } + if err := record.Validate(); err != nil { + t.Fatalf("operational record Validate() error = %v", err) + } + transition := moved.Alert.LatestTransition + if transition == nil || transition.Reason != wantSummary { + t.Fatalf("resolve transition = %+v, want reason %q", transition, wantSummary) + } + if err := transition.Validate(); err != nil { + t.Fatalf("resolve transition Validate() error = %v", err) + } + closing := moved.Alert.Evidence[len(moved.Alert.Evidence)-1] + if closing.ID != transition.EvidenceIDs[0] || + closing.Reason == nil || + closing.Reason.Code != string(AlertResolutionMovedToAgent) || + closing.Reason.Message != wantSummary { + t.Fatalf("closing evidence = %+v, want the handover, not a recovery projection", closing) + } + + recovered := resolvedByType["cpu"] + if recovered == nil || recovered.Alert.Resolution != nil { + t.Fatalf("plain disabled release = %+v, want an ordinary resolve with no resolution", recovered) + } + if last := recovered.Alert.Evidence[len(recovered.Alert.Evidence)-1]; last.Reason == nil || + last.Reason.Code != legacyAlertRecoveryEvidenceReason { + t.Fatalf("plain release evidence = %+v, want legacy recovery projection", last.Reason) + } + + var historyResolution *AlertResolution + for _, entry := range manager.GetAlertHistory(10) { + if entry.Type == "memory" { + historyResolution = entry.Resolution + } + } + if historyResolution == nil || historyResolution.Reason != AlertResolutionMovedToAgent { + t.Fatalf("history resolution = %+v, want moved_to_agent", historyResolution) + } + + events, err := manager.AlertEvents(eventlog.Filter{AlertID: moved.Alert.ID, Types: []string{eventlog.TypeResolved}}) + if err != nil { + t.Fatalf("AlertEvents: %v", err) + } + if len(events) != 1 || + events[0].Reason != string(AlertResolutionMovedToAgent) || + events[0].Message != wantSummary { + t.Fatalf("resolved events = %+v, want one handover event", events) + } +} + +func TestResolvedHistoryRowKeepsResolutionWithoutEventLog(t *testing.T) { + firstObservedAt := time.Date(2026, 10, 6, 9, 0, 0, 0, time.UTC) + alert := &Alert{ + ID: "pve-1-memory", + Type: "memory", + Level: AlertLevelWarning, + ResourceID: "pve-1", + CanonicalSpecID: "metric-threshold:memory", + CanonicalState: "pve-1::metric-threshold:memory", + StartTime: firstObservedAt, + LastSeen: firstObservedAt, + } + ensureOperationalContract(alert, firstObservedAt) + history := newTestHistoryManager(t) + history.AddAlert(*alert) + manager := &Manager{historyManager: history} + + alert.Resolution = &AlertResolution{Reason: AlertResolutionMovedToAgent, SuccessorName: "pve-1 (Host Agent)"} + manager.newResolvedAlert(alert, firstObservedAt.Add(time.Minute), nil) + + entries := history.GetAllHistory(1) + if len(entries) != 1 || entries[0].Resolution == nil || + entries[0].Resolution.Reason != AlertResolutionMovedToAgent { + t.Fatalf("history entries = %+v, want the row to keep the moved_to_agent resolution", entries) + } +} + func TestMergeOperationalRecurrenceReopensStableCause(t *testing.T) { firstObservedAt := time.Date(2026, 7, 18, 20, 0, 0, 0, time.UTC) resolvedAt := firstObservedAt.Add(5 * time.Minute) diff --git a/internal/alerts/resolution.go b/internal/alerts/resolution.go new file mode 100644 index 0000000000..6ba3fbba54 --- /dev/null +++ b/internal/alerts/resolution.go @@ -0,0 +1,79 @@ +package alerts + +import ( + "fmt" + "strings" +) + +// AlertResolutionReason says why an alert closed when the close was not a +// recovery. The close still reaches every resolved consumer, so an incident +// opened from the alert does not stay open on the receiving side, but no +// surface may report the resource as healthy. +type AlertResolutionReason string + +const ( + // AlertResolutionMovedToAgent means a Pulse agent linked to the alert's + // resource now owns the same metric, so the agent's own alert settings + // decide whether the condition is still alerted on. + AlertResolutionMovedToAgent AlertResolutionReason = "moved_to_agent" +) + +// AlertResolution records why an alert closed without its condition being +// observed to clear. A nil resolution is an ordinary recovery. +type AlertResolution struct { + Reason AlertResolutionReason `json:"reason"` + // SuccessorResourceID and SuccessorName identify the resource that owns + // the alert from now on, named the way that resource's own alerts are. + SuccessorResourceID string `json:"successorResourceId,omitempty"` + SuccessorName string `json:"successorName,omitempty"` +} + +// Clone returns a copy of the resolution. +func (r *AlertResolution) Clone() *AlertResolution { + if r == nil { + return nil + } + clone := *r + return &clone +} + +// Outcome is the short form of a close that was not a recovery, for lists +// such as "moved to pve1 (Host Agent)". It is empty for an ordinary recovery. +func (r *AlertResolution) Outcome() string { + if r == nil { + return "" + } + switch r.Reason { + case AlertResolutionMovedToAgent: + successor := strings.TrimSpace(r.SuccessorName) + if successor == "" { + successor = "its Pulse agent" + } + return "moved to " + successor + default: + return "" + } +} + +// Summary is the one-line account of a close that was not a recovery. It is +// empty for an ordinary recovery. +func (r *AlertResolution) Summary() string { + return r.Describe("Alert") +} + +// Describe is Summary with the alert named by the caller, such as +// "Memory alert", so a message about one of several closes says which one. +func (r *AlertResolution) Describe(subject string) string { + if r == nil { + return "" + } + if subject = strings.TrimSpace(subject); subject == "" { + subject = "Alert" + } + switch r.Reason { + case AlertResolutionMovedToAgent: + return fmt.Sprintf("%s %s. This is not a recovery: check the agent for the current reading.", subject, r.Outcome()) + default: + return "" + } +} diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index adda188023..71f467b967 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -432,6 +432,47 @@ func annotateResolvedMetadata(alert *alerts.Alert, resolvedAt time.Time) { alert.Metadata[metadataResolvedAt] = resolvedAt.Format(time.RFC3339) } +// resolvedAlertMessage is the one-line account of a resolved alert. A close +// that was not a recovery, such as a node alert that moved to the Pulse agent +// now monitoring the machine, says what happened instead of "now healthy". +func resolvedAlertMessage(alert *alerts.Alert) string { + if summary := resolvedAlertNotRecoveredSummary(alert); summary != "" { + return summary + } + return fmt.Sprintf("%s on %s is now healthy", alert.ResourceName, alert.Node) +} + +// resolvedAlertNotRecoveredSummary names the alert in its resolution summary, +// so one of several closes for a machine says which alert moved. It is empty +// for an ordinary recovery. +func resolvedAlertNotRecoveredSummary(alert *alerts.Alert) string { + label := alertTypeDisplay(alert.Type) + if label != "" { + label += " alert" + } + return alert.Resolution.Describe(label) +} + +// resolvedAlertsIncludeNotRecovered reports whether any close in a resolved +// notification was not a recovery, so recovery visuals must not be used. +func resolvedAlertsIncludeNotRecovered(alertList []*alerts.Alert) bool { + for _, alert := range alertList { + if alert != nil && alert.Resolution.Outcome() != "" { + return true + } + } + return false +} + +// resolvedAlertListEntry names one alert in a grouped recovery list. +func resolvedAlertListEntry(alert *alerts.Alert) string { + entry := fmt.Sprintf("%s on %s", alert.ResourceName, alert.Node) + if outcome := alert.Resolution.Outcome(); outcome != "" { + entry += ", " + outcome + } + return entry +} + func quietHoursReplayAtForAlert(alert *alerts.Alert, now time.Time) *time.Time { if alert == nil || alert.Metadata == nil { return nil @@ -2267,6 +2308,9 @@ func buildResolvedNotificationContent(alertList []*alerts.Alert, resolvedAt time resolvedLabel := resolvedAt.Format(time.RFC3339) title := fmt.Sprintf("Pulse alert resolved: %s", primary.ResourceName) + if primary.Resolution.Outcome() != "" { + title = fmt.Sprintf("Pulse alert moved: %s", primary.ResourceName) + } if len(validAlerts) > 1 { title = fmt.Sprintf("Pulse alerts resolved (%d)", len(validAlerts)) } @@ -2282,6 +2326,10 @@ func buildResolvedNotificationContent(alertList []*alerts.Alert, resolvedAt time bodyBuilder.WriteString(alert.Message) bodyBuilder.WriteString("\n") } + if summary := resolvedAlertNotRecoveredSummary(alert); summary != "" { + bodyBuilder.WriteString(summary) + bodyBuilder.WriteString("\n") + } if !alert.StartTime.IsZero() { bodyBuilder.WriteString("Started: ") bodyBuilder.WriteString(alert.StartTime.Format(time.RFC3339)) @@ -2980,14 +3028,15 @@ func (n *NotificationManager) sendResolvedWebhook(webhook WebhookConfig, alertLi data.ResolvedAt = resolvedAt.Format(time.RFC3339) data.ResolvedAtISO = resolvedAt.Format(time.RFC3339) data.Duration = formatWebhookDuration(resolvedAt.Sub(alert.StartTime)) - data.Message = fmt.Sprintf("%s on %s is now healthy", alert.ResourceName, alert.Node) + data.Message = resolvedAlertMessage(alert) + data.NotRecovered = resolvedAlertsIncludeNotRecovered(alertList) data.AlertCount = len(alertList) data.Alerts = alertList if len(alertList) > 1 { names := make([]string, 0, len(alertList)) for _, a := range alertList { if a != nil { - names = append(names, fmt.Sprintf("%s on %s", a.ResourceName, a.Node)) + names = append(names, resolvedAlertListEntry(a)) } } data.Message = fmt.Sprintf("%d alerts resolved: %s", len(alertList), strings.Join(names, "; ")) @@ -3036,28 +3085,34 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale return fmt.Errorf("rate limit exceeded for webhook %s", webhook.Name) } - // Build plain-text body + // Build plain-text body and title. A single alert that moved rather than + // recovered says so in both, and no batch holding such a close carries + // the green check. var body strings.Builder + title := "RESOLVED" + tags := "white_check_mark,pulse,resolved" + if resolvedAlertsIncludeNotRecovered(alertList) { + tags = "arrow_right,pulse,resolved" + } if len(alertList) == 1 && alertList[0] != nil { a := alertList[0] - fmt.Fprintf(&body, "Resolved: %s on %s is now healthy", a.ResourceName, a.Node) + title = fmt.Sprintf("RESOLVED: %s", a.ResourceName) + if summary := resolvedAlertNotRecoveredSummary(a); summary != "" { + title = fmt.Sprintf("MOVED: %s", a.ResourceName) + body.WriteString(summary) + } else { + fmt.Fprintf(&body, "Resolved: %s", resolvedAlertMessage(a)) + } } else { + title = fmt.Sprintf("RESOLVED: %d alerts", len(alertList)) fmt.Fprintf(&body, "%d alerts resolved at %s:\n", len(alertList), resolvedAt.Format(time.RFC822)) for _, a := range alertList { if a != nil { - fmt.Fprintf(&body, "- %s on %s\n", a.ResourceName, a.Node) + fmt.Fprintf(&body, "- %s\n", resolvedAlertListEntry(a)) } } } - // Build title - title := "RESOLVED" - if len(alertList) == 1 && alertList[0] != nil { - title = fmt.Sprintf("RESOLVED: %s", alertList[0].ResourceName) - } else { - title = fmt.Sprintf("RESOLVED: %d alerts", len(alertList)) - } - method := webhook.Method if method == "" { method = "POST" @@ -3071,7 +3126,7 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale req.Header.Set("Content-Type", "text/plain") req.Header.Set("Title", title) req.Header.Set("Priority", "default") - req.Header.Set("Tags", "white_check_mark,pulse,resolved") + req.Header.Set("Tags", tags) req.Header.Set("User-Agent", "Pulse-Monitoring/2.0") // Apply any custom headers from webhook config diff --git a/internal/notifications/resolved_handover_test.go b/internal/notifications/resolved_handover_test.go new file mode 100644 index 0000000000..f144a178be --- /dev/null +++ b/internal/notifications/resolved_handover_test.go @@ -0,0 +1,239 @@ +package notifications + +import ( + "encoding/json" + "io" + "net/http" + "strings" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/alerts" +) + +const movedSummary = "Memory alert moved to pve1 (Host Agent). This is not a recovery: check the agent for the current reading." + +// movedNodeMemoryAlert is a node memory alert that closed because the Pulse +// agent linked to the node took over its usage alerts, not because memory +// came back down. +func movedNodeMemoryAlert() *alerts.Alert { + return &alerts.Alert{ + ID: "pve1-memory", + Type: "memory", + Level: alerts.AlertLevelWarning, + ResourceName: "pve1", + ResourceID: "pve1", + Node: "pve1", + Instance: "https://pve.local:8006", + Message: "Node memory at 95.0%", + Value: 95, + Threshold: 90, + StartTime: time.Now().Add(-10 * time.Minute), + Resolution: &alerts.AlertResolution{ + Reason: alerts.AlertResolutionMovedToAgent, + SuccessorResourceID: "agent-pve1", + SuccessorName: "pve1 (Host Agent)", + }, + } +} + +// Exercise the normal entry points: the firing goes out, its receipt gates the +// close, and the close is persisted in the delivery queue and read back before +// it is rendered. +func TestMovedAlertCloseIsDeliveredAsMovedNotRecovered(t *testing.T) { + type message struct{ title, body, tags string } + received := make(chan message, 4) + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("read notification: %v", err) + } + received <- message{r.Header.Get("Title"), string(body), r.Header.Get("Tags")} + w.WriteHeader(http.StatusAccepted) + })) + defer server.Close() + + webhook := WebhookConfig{ID: "ops", Name: "ops", URL: server.URL + "/topic", Enabled: true, Service: "ntfy"} + m := NewNotificationManagerWithDataDir("", t.TempDir()) + t.Cleanup(m.Stop) + m.webhookClient = server.Client() + if err := m.UpdateAllowedPrivateCIDRs("127.0.0.1/32"); err != nil { + t.Fatal(err) + } + m.SetGroupingConfig(false, 0, false, false) + m.SetNotifyOnResolve(true) + m.AddWebhook(webhook) + + next := func() message { + t.Helper() + select { + case msg := <-received: + return msg + case <-time.After(10 * time.Second): + t.Fatal("notification not received") + return message{} + } + } + + alert := movedNodeMemoryAlert() + firing := alert.Clone() + firing.Resolution = nil + m.SendAlert(firing) + next() + recoveryJob := notificationDeliveryJob{Type: "webhook", Event: eventResolved, Alerts: []*alerts.Alert{alert}, WebhookConfig: &webhook} + deadline := time.Now().Add(10 * time.Second) + for len(m.filterResolvedJobsByDeliveryReceipt([]notificationDeliveryJob{recoveryJob})) != 1 { + if time.Now().After(deadline) { + t.Fatal("firing receipt never recorded") + } + time.Sleep(time.Millisecond) + } + + m.SendResolvedAlert(&alerts.ResolvedAlert{Alert: alert, ResolvedTime: time.Now()}) + closing := next() + if closing.title != "MOVED: pve1" || closing.body != movedSummary { + t.Fatalf("close = %+v, want MOVED title and the handover summary", closing) + } + if strings.Contains(closing.body, "healthy") || strings.Contains(closing.tags, "white_check_mark") { + t.Fatalf("close reads as a recovery: %+v", closing) + } +} + +// captureResolvedWebhook renders one resolved delivery through an isolated +// manager and returns what the destination received. +func captureResolvedWebhook(t *testing.T, service string, alertList []*alerts.Alert) ([]byte, http.Header) { + t.Helper() + type request struct { + body []byte + header http.Header + } + received := make(chan request, 1) + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + received <- request{body: body, header: r.Header.Clone()} + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + nm := NewNotificationManagerWithDataDir("https://pulse.local", t.TempDir()) + t.Cleanup(nm.Stop) + if err := nm.UpdateAllowedPrivateCIDRs("127.0.0.1"); err != nil { + t.Fatal(err) + } + webhook := WebhookConfig{Name: service, URL: server.URL + "/hook", Enabled: true, Service: service, + CustomFields: map[string]string{"routing_key": "rk"}} + if err := nm.sendResolvedWebhook(webhook, alertList, time.Now()); err != nil { + t.Fatalf("sendResolvedWebhook %s: %v", service, err) + } + select { + case got := <-received: + return got.body, got.header + case <-time.After(5 * time.Second): + t.Fatalf("%s destination received nothing", service) + return nil, nil + } +} + +func decodeResolvedPayload(t *testing.T, service string, body []byte) map[string]any { + t.Helper() + var payload map[string]any + if err := json.Unmarshal(body, &payload); err != nil { + t.Fatalf("unmarshal %s payload: %v\n%s", service, err, body) + } + return payload +} + +func TestMovedAlertCloseRendersHonestlyOnEveryChannel(t *testing.T) { + recovered := &alerts.Alert{ID: "vm-cpu", Type: "cpu", ResourceName: "web-vm-01", Node: "pve1", StartTime: time.Now().Add(-time.Minute)} + + t.Run("discord says moved without the recovery colour", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "discord", []*alerts.Alert{movedNodeMemoryAlert()}) + embed := decodeResolvedPayload(t, "discord", body)["embeds"].([]any)[0].(map[string]any) + description, _ := embed["description"].(string) + if !strings.Contains(description, movedSummary) || strings.Contains(description, "healthy") { + t.Fatalf("description = %q, want the handover summary", description) + } + if embed["color"] != float64(3447003) { + t.Fatalf("embed color = %v, want the neutral 3447003, not recovery green", embed["color"]) + } + }) + + t.Run("teams cards drop recovery green", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "teams", []*alerts.Alert{movedNodeMemoryAlert()}) + if got := decodeResolvedPayload(t, "teams", body)["themeColor"]; got != "0076D7" { + t.Fatalf("teams themeColor = %v, want 0076D7", got) + } + body, _ = captureResolvedWebhook(t, "teams-adaptive", []*alerts.Alert{movedNodeMemoryAlert()}) + if !strings.Contains(string(body), `"color": "Accent"`) || strings.Contains(string(body), `"color": "Good"`) { + t.Fatalf("teams adaptive card = %s, want the Accent title colour", body) + } + }) + + t.Run("mattermost drops the check mark", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "mattermost", []*alerts.Alert{movedNodeMemoryAlert()}) + text, _ := decodeResolvedPayload(t, "mattermost", body)["text"].(string) + if strings.Contains(text, ":white_check_mark:") || !strings.Contains(text, ":arrow_right: **RESOLVED**") || !strings.Contains(text, movedSummary) { + t.Fatalf("mattermost text = %q", text) + } + }) + + t.Run("pagerduty still closes the incident", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "pagerduty", []*alerts.Alert{movedNodeMemoryAlert()}) + payload := decodeResolvedPayload(t, "pagerduty", body) + if payload["event_action"] != "resolve" || payload["dedup_key"] != "pve1-memory" { + t.Fatalf("payload = %v, want a resolve for the node alert's incident", payload) + } + }) + + t.Run("grouped list names the move", func(t *testing.T) { + body, _ := captureResolvedWebhook(t, "slack", []*alerts.Alert{movedNodeMemoryAlert(), recovered}) + if !strings.Contains(string(body), "pve1 on pve1, moved to pve1 (Host Agent)") || !strings.Contains(string(body), "web-vm-01 on pve1") { + t.Fatalf("grouped body = %s, want the moved alert marked and the recovery listed", body) + } + }) + + t.Run("grouped ntfy batch drops the check mark", func(t *testing.T) { + body, header := captureResolvedWebhook(t, "ntfy", []*alerts.Alert{movedNodeMemoryAlert(), recovered}) + if tags := header.Get("Tags"); strings.Contains(tags, "white_check_mark") || !strings.Contains(tags, "arrow_right") { + t.Fatalf("ntfy tags = %q, want no check mark on a batch holding a move", tags) + } + if !strings.Contains(string(body), "- pve1 on pve1, moved to pve1 (Host Agent)\n") { + t.Fatalf("ntfy body = %q", body) + } + }) + + t.Run("email and apprise say moved", func(t *testing.T) { + title, htmlBody, textBody := buildResolvedNotificationContent([]*alerts.Alert{movedNodeMemoryAlert()}, time.Now(), "") + if title != "Pulse alert moved: pve1" { + t.Fatalf("title = %q", title) + } + if !strings.Contains(textBody, "Node memory at 95.0%\n"+movedSummary+"\n") { + t.Fatalf("text body = %q, want the breach followed by the handover summary", textBody) + } + if !strings.Contains(htmlBody, "This is not a recovery: check the agent for the current reading.") { + t.Fatalf("html body = %q", htmlBody) + } + }) + + t.Run("ordinary recovery keeps its wording and visuals", func(t *testing.T) { + plain := movedNodeMemoryAlert() + plain.Resolution = nil + if got := resolvedAlertMessage(plain); got != "pve1 on pve1 is now healthy" { + t.Fatalf("resolvedAlertMessage() = %q", got) + } + if title, _, _ := buildResolvedNotificationContent([]*alerts.Alert{plain}, time.Now(), ""); title != "Pulse alert resolved: pve1" { + t.Fatalf("title = %q", title) + } + body, _ := captureResolvedWebhook(t, "discord", []*alerts.Alert{plain}) + if embed := decodeResolvedPayload(t, "discord", body)["embeds"].([]any)[0].(map[string]any); embed["color"] != float64(3066993) { + t.Fatalf("ordinary recovery embed color = %v, want green 3066993", embed["color"]) + } + body, _ = captureResolvedWebhook(t, "mattermost", []*alerts.Alert{plain}) + if text, _ := decodeResolvedPayload(t, "mattermost", body)["text"].(string); !strings.Contains(text, ":white_check_mark: **RESOLVED**") { + t.Fatalf("ordinary mattermost text = %q", text) + } + _, header := captureResolvedWebhook(t, "ntfy", []*alerts.Alert{plain}) + if tags := header.Get("Tags"); tags != "white_check_mark,pulse,resolved" { + t.Fatalf("ordinary ntfy tags = %q", tags) + } + }) +} diff --git a/internal/notifications/webhook_enhanced.go b/internal/notifications/webhook_enhanced.go index 5c390fd1c6..c586c2e9fb 100644 --- a/internal/notifications/webhook_enhanced.go +++ b/internal/notifications/webhook_enhanced.go @@ -65,6 +65,10 @@ type WebhookPayloadData struct { Event string // "alert" or "resolved" ResolvedAt string // RFC3339 timestamp when alert was resolved ResolvedAtISO string // Same as ResolvedAt (alias for template consistency) + // NotRecovered marks a resolved notification in which at least one close + // was not a recovery (an alert that moved to a linked Pulse agent), so + // templates can drop recovery colours and check marks. + NotRecovered bool // Tenant identity (multi-tenant orgs and MSP client runtimes). // Empty on plain single-tenant installs. diff --git a/internal/notifications/webhook_templates.go b/internal/notifications/webhook_templates.go index 8c9f8db4f0..5e4b2bf152 100644 --- a/internal/notifications/webhook_templates.go +++ b/internal/notifications/webhook_templates.go @@ -56,7 +56,7 @@ func GetWebhookTemplates() []WebhookTemplate { "embeds": [{ "title": "Resolved: {{.ResourceName | jsonString}}", "description": "{{.Message | jsonString}}", - "color": 3066993, + "color": {{if .NotRecovered}}3447003{{else}}3066993{{end}}, "fields": [ {"name": "Resource", "value": "{{.ResourceName | jsonString}}", "inline": true}, {"name": "Node", "value": "{{.Node | jsonString}}", "inline": true}, @@ -241,7 +241,7 @@ func GetWebhookTemplates() []WebhookTemplate { ResolvedPayloadTemplate: `{ "@type": "MessageCard", "@context": "http://schema.org/extensions", - "themeColor": "2DC72D", + "themeColor": "{{if .NotRecovered}}0076D7{{else}}2DC72D{{end}}", "summary": "Resolved: {{.ResourceName | jsonString}}", {{if .Mention}}"text": "{{.Mention | jsonString}}",{{end}} "sections": [{ @@ -372,7 +372,7 @@ func GetWebhookTemplates() []WebhookTemplate { "text": "Resolved: {{.ResourceName | jsonString}}", "weight": "Bolder", "size": "Large", - "color": "Good" + "color": "{{if .NotRecovered}}Accent{{else}}Good{{end}}" }, { "type": "TextBlock", @@ -521,7 +521,7 @@ View in Pulse: {{.Instance}}`, ResolvedPayloadTemplate: `{ "username": "Pulse Monitoring", "icon_url": "https://raw.githubusercontent.com/rcourtman/Pulse/main/frontend-modern/public/android-chrome-192x192.png", - "text": "{{if .Mention}}{{.Mention | jsonString}}\n\n{{end}}:white_check_mark: **RESOLVED**\n\n**{{.ResourceName | jsonString}}** on **{{.Node | jsonString}}**\n\n{{.Message | jsonString}}\n\n| Detail | Value |\n|:-------|:------|\n| Resource | {{.ResourceName | jsonString}} |\n| Node | {{.Node | jsonString}} |\n| Type | {{.Type | title | jsonString}} |\n| Duration | {{.Duration | jsonString}} |\n| Resolved At | {{.ResolvedAt | jsonString}} |\n| Alert Identifier | {{.ID | jsonString}} |" + "text": "{{if .Mention}}{{.Mention | jsonString}}\n\n{{end}}{{if .NotRecovered}}:arrow_right:{{else}}:white_check_mark:{{end}} **RESOLVED**\n\n**{{.ResourceName | jsonString}}** on **{{.Node | jsonString}}**\n\n{{.Message | jsonString}}\n\n| Detail | Value |\n|:-------|:------|\n| Resource | {{.ResourceName | jsonString}} |\n| Node | {{.Node | jsonString}} |\n| Type | {{.Type | title | jsonString}} |\n| Duration | {{.Duration | jsonString}} |\n| Resolved At | {{.ResolvedAt | jsonString}} |\n| Alert Identifier | {{.ID | jsonString}} |" }`, Instructions: "1. In Mattermost, go to Integrations > Incoming Webhooks\n2. Create a new webhook and select the channel\n3. Copy the webhook URL and paste it here\n\nNote: This template uses Markdown formatting which is fully supported by Mattermost.", },