From 123821b02a8dc4a4203406672cf6df628eb89914 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:04:51 +0100 Subject: [PATCH 1/3] Carry a typed reason when an alert closes without recovering When a Pulse agent is linked to a Proxmox node, the node releases its CPU, memory and disk alerts to the agent, and any node alert still open closes. The close has to reach integrations so a PagerDuty or webhook incident opened from the node alert does not stay open forever, but it went out as a recovery: "pve1 on pve1 is now healthy" for a node still at 95% memory, with synthesized "recovery projection" evidence behind it. #1682 is the same complaint from another trigger: a close announced as "now healthy" when nothing recovered. A resolved alert can now carry an AlertResolution saying why it closed. The first reason is moved_to_agent, with the agent's alert resource ID and name. releaseCanonicalMetricAlert, the disabled-threshold release pulled out of evaluateCanonicalMetricAlert, takes the resolution, and clearAlertWithResolution stamps a copy on the alert before it resolves. It rides the Alert JSON into the resolved callback, the delivery queue, the history row and the event log. The synthesized closing evidence and the resolve transition say what happened instead of claiming a recovery projection. Notifications still send the close and change only the words: "Memory alert moved to pve1 (Host Agent). This is not a recovery: check the agent for the current reading." for webhook messages, a MOVED title without the green check on ntfy, and "Pulse alert moved" on email and Apprise. A nil resolution, which every caller passes today, leaves ordinary recoveries and the disabled-threshold path unchanged. The node release adopts the reason once the host-agent node link records which metrics the agent evaluates. --- .../v6/internal/subsystems/alerts.md | 19 ++ .../v6/internal/subsystems/notifications.md | 26 +++ internal/alerts/active_lifecycle.go | 7 + internal/alerts/callbacks.go | 8 +- internal/alerts/canonical_metric.go | 38 ++-- internal/alerts/history.go | 1 + internal/alerts/model.go | 3 + internal/alerts/operational_contract.go | 15 +- internal/alerts/operational_contract_test.go | 157 +++++++++++++++ internal/alerts/resolution.go | 79 ++++++++ internal/notifications/notifications.go | 68 +++++-- .../notifications/resolved_handover_test.go | 183 ++++++++++++++++++ 12 files changed, 573 insertions(+), 31 deletions(-) create mode 100644 internal/alerts/resolution.go create mode 100644 internal/notifications/resolved_handover_test.go diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index c270dc70d3..4a1cc835d1 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..bb8bba2518 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -1216,3 +1216,29 @@ 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: ` and swaps the green check tag for +`arrow_right`. Email and Apprise title it `Pulse alert moved: ` and +add the summary after the breach message. The resolution rides the Alert JSON, +so it survives the delivery queue round trip and appears in the default JSON +payload's `alerts` array. Built-in template titles still say Resolved, which is +accurate for the closed alert, and a custom template that hard-codes recovery +text keeps it. 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, +PagerDuty, a grouped Slack list and email/Apprise content, plus the unchanged +ordinary recovery wording. diff --git a/internal/alerts/active_lifecycle.go b/internal/alerts/active_lifecycle.go index 3771f98205..0357ce5d66 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 fcfa75baa5..4a57560cfc 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 444cb7aba3..a4d0dc43bc 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" @@ -744,3 +745,159 @@ func TestResolvedOccurrenceCallbackSurvivesLegacyPanic(t *testing.T) { t.Fatal("legacy panic prevented occurrence-qualified recovery") } } + +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) + } +} 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..93ae16e0f3 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -432,6 +432,36 @@ 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) +} + +// 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 +2297,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 +2315,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 +3017,14 @@ 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.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 +3073,31 @@ 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 drops the green check. var body strings.Builder + title := "RESOLVED" + tags := "white_check_mark,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) + tags = "arrow_right,pulse,resolved" + 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 +3111,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..b980c6fb56 --- /dev/null +++ b/internal/notifications/resolved_handover_test.go @@ -0,0 +1,183 @@ +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) + } +} + +func TestMovedAlertCloseRendersHonestlyOnEveryChannel(t *testing.T) { + capture := func(t *testing.T, service string) map[string]any { + t.Helper() + var gotBody []byte + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + nm := NewNotificationManager("https://pulse.local") + _ = nm.UpdateAllowedPrivateCIDRs("127.0.0.1") + webhook := WebhookConfig{Name: service, URL: server.URL + "/hook", Enabled: true, Service: service, + CustomFields: map[string]string{"routing_key": "rk"}} + if err := nm.sendResolvedWebhook(webhook, []*alerts.Alert{movedNodeMemoryAlert()}, time.Now()); err != nil { + t.Fatalf("sendResolvedWebhook %s: %v", service, err) + } + var payload map[string]any + if err := json.Unmarshal(gotBody, &payload); err != nil { + t.Fatalf("unmarshal %s payload: %v", service, err) + } + return payload + } + + t.Run("discord says moved", func(t *testing.T) { + embed := capture(t, "discord")["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) + } + }) + + t.Run("pagerduty still closes the incident", func(t *testing.T) { + payload := capture(t, "pagerduty") + 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) { + var gotBody []byte + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + nm := NewNotificationManager("https://pulse.local") + _ = nm.UpdateAllowedPrivateCIDRs("127.0.0.1") + recovered := &alerts.Alert{ID: "vm-cpu", Type: "cpu", ResourceName: "web-vm-01", Node: "pve1", StartTime: time.Now().Add(-time.Minute)} + webhook := WebhookConfig{Name: "slack", URL: server.URL + "/hook", Enabled: true, Service: "slack"} + if err := nm.sendResolvedWebhook(webhook, []*alerts.Alert{movedNodeMemoryAlert(), recovered}, time.Now()); err != nil { + t.Fatalf("sendResolvedWebhook: %v", err) + } + body := string(gotBody) + if !strings.Contains(body, "pve1 on pve1, moved to pve1 (Host Agent)") || !strings.Contains(body, "web-vm-01 on pve1") { + t.Fatalf("grouped body = %s, want the moved alert marked and the recovery listed", 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 is unchanged", func(t *testing.T) { + recovered := movedNodeMemoryAlert() + recovered.Resolution = nil + if got := resolvedAlertMessage(recovered); got != "pve1 on pve1 is now healthy" { + t.Fatalf("resolvedAlertMessage() = %q", got) + } + if title, _, _ := buildResolvedNotificationContent([]*alerts.Alert{recovered}, time.Now(), ""); title != "Pulse alert resolved: pve1" { + t.Fatalf("title = %q", title) + } + }) +} From 602e2d04cd51a316f1fd14ca5ad3bee7f611c5ea Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:19:17 +0100 Subject: [PATCH 2/3] Move the handover tests off the end of the contract test file main appended platform-stamping tests to the end of operational_contract_test.go after this branch appended the handover tests there, so the two insertions conflicted. Placing the handover tests beside the resolved-history test lets main merge in cleanly. No test changes. --- internal/alerts/operational_contract_test.go | 312 +++++++++---------- 1 file changed, 156 insertions(+), 156 deletions(-) diff --git a/internal/alerts/operational_contract_test.go b/internal/alerts/operational_contract_test.go index a4d0dc43bc..51922249de 100644 --- a/internal/alerts/operational_contract_test.go +++ b/internal/alerts/operational_contract_test.go @@ -413,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) @@ -745,159 +901,3 @@ func TestResolvedOccurrenceCallbackSurvivesLegacyPanic(t *testing.T) { t.Fatal("legacy panic prevented occurrence-qualified recovery") } } - -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) - } -} From a89c08a59e408dc42c2b95aa870a56e2087041e9 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Tue, 6 Oct 2026 16:17:57 +0100 Subject: [PATCH 3/3] Drop recovery colours and check marks from moved alert closes A second review found the moved-alert text was honest but the visuals still said recovered: Discord kept its green embed, both Teams cards kept recovery green, Mattermost led with a check emoji, and a grouped ntfy batch holding a moved alert kept the green check tag. A check mark on a phone is read before the sentence under it. Resolved webhook data now carries NotRecovered when any close in the delivery was not a recovery. The built-in templates switch to neutral colours and an arrow when it is set, and ntfy uses the arrow tag for a single alert or a batch. Titles still say Resolved, which stays true for the closed alert, and ordinary recoveries keep their green. The render tests now use an isolated data directory, stop their managers, and hand the captured request over a channel, so they no longer touch a shared queue or race on the captured body. --- .../v6/internal/subsystems/notifications.md | 26 ++-- internal/notifications/notifications.go | 19 ++- .../notifications/resolved_handover_test.go | 144 ++++++++++++------ internal/notifications/webhook_enhanced.go | 4 + internal/notifications/webhook_templates.go | 8 +- 5 files changed, 141 insertions(+), 60 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index bb8bba2518..0afc28d582 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -1229,16 +1229,22 @@ receives `resolve` for the alert's dedup key. Only the wording changes. 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: ` and swaps the green check tag for -`arrow_right`. Email and Apprise title it `Pulse alert moved: ` and -add the summary after the breach message. The resolution rides the Alert JSON, -so it survives the delivery queue round trip and appears in the default JSON -payload's `alerts` array. Built-in template titles still say Resolved, which is -accurate for the closed alert, and a custom template that hard-codes recovery -text keeps it. Issue #1682 is the precedent: a recipient called "is now -healthy" a false factual claim when an alert closed without recovering. +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, -PagerDuty, a grouped Slack list and email/Apprise content, plus the unchanged -ordinary recovery wording. +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/notifications/notifications.go b/internal/notifications/notifications.go index 93ae16e0f3..71f467b967 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -453,6 +453,17 @@ func resolvedAlertNotRecoveredSummary(alert *alerts.Alert) string { 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) @@ -3018,6 +3029,7 @@ func (n *NotificationManager) sendResolvedWebhook(webhook WebhookConfig, alertLi data.ResolvedAtISO = resolvedAt.Format(time.RFC3339) data.Duration = formatWebhookDuration(resolvedAt.Sub(alert.StartTime)) data.Message = resolvedAlertMessage(alert) + data.NotRecovered = resolvedAlertsIncludeNotRecovered(alertList) data.AlertCount = len(alertList) data.Alerts = alertList if len(alertList) > 1 { @@ -3074,16 +3086,19 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale } // Build plain-text body and title. A single alert that moved rather than - // recovered says so in both, and drops the green check. + // 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] title = fmt.Sprintf("RESOLVED: %s", a.ResourceName) if summary := resolvedAlertNotRecoveredSummary(a); summary != "" { title = fmt.Sprintf("MOVED: %s", a.ResourceName) - tags = "arrow_right,pulse,resolved" body.WriteString(summary) } else { fmt.Fprintf(&body, "Resolved: %s", resolvedAlertMessage(a)) diff --git a/internal/notifications/resolved_handover_test.go b/internal/notifications/resolved_handover_test.go index b980c6fb56..f144a178be 100644 --- a/internal/notifications/resolved_handover_test.go +++ b/internal/notifications/resolved_handover_test.go @@ -99,64 +99,108 @@ func TestMovedAlertCloseIsDeliveredAsMovedNotRecovered(t *testing.T) { } } -func TestMovedAlertCloseRendersHonestlyOnEveryChannel(t *testing.T) { - capture := func(t *testing.T, service string) map[string]any { - t.Helper() - var gotBody []byte - server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotBody, _ = io.ReadAll(r.Body) - w.WriteHeader(http.StatusOK) - })) - defer server.Close() - nm := NewNotificationManager("https://pulse.local") - _ = nm.UpdateAllowedPrivateCIDRs("127.0.0.1") - webhook := WebhookConfig{Name: service, URL: server.URL + "/hook", Enabled: true, Service: service, - CustomFields: map[string]string{"routing_key": "rk"}} - if err := nm.sendResolvedWebhook(webhook, []*alerts.Alert{movedNodeMemoryAlert()}, time.Now()); err != nil { - t.Fatalf("sendResolvedWebhook %s: %v", service, err) - } - var payload map[string]any - if err := json.Unmarshal(gotBody, &payload); err != nil { - t.Fatalf("unmarshal %s payload: %v", service, err) - } - return payload +// 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 + } +} - t.Run("discord says moved", func(t *testing.T) { - embed := capture(t, "discord")["embeds"].([]any)[0].(map[string]any) +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) { - payload := capture(t, "pagerduty") + 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) { - var gotBody []byte - server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - gotBody, _ = io.ReadAll(r.Body) - w.WriteHeader(http.StatusOK) - })) - defer server.Close() - nm := NewNotificationManager("https://pulse.local") - _ = nm.UpdateAllowedPrivateCIDRs("127.0.0.1") - recovered := &alerts.Alert{ID: "vm-cpu", Type: "cpu", ResourceName: "web-vm-01", Node: "pve1", StartTime: time.Now().Add(-time.Minute)} - webhook := WebhookConfig{Name: "slack", URL: server.URL + "/hook", Enabled: true, Service: "slack"} - if err := nm.sendResolvedWebhook(webhook, []*alerts.Alert{movedNodeMemoryAlert(), recovered}, time.Now()); err != nil { - t.Fatalf("sendResolvedWebhook: %v", err) - } - body := string(gotBody) - if !strings.Contains(body, "pve1 on pve1, moved to pve1 (Host Agent)") || !strings.Contains(body, "web-vm-01 on pve1") { + 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" { @@ -170,14 +214,26 @@ func TestMovedAlertCloseRendersHonestlyOnEveryChannel(t *testing.T) { } }) - t.Run("ordinary recovery is unchanged", func(t *testing.T) { - recovered := movedNodeMemoryAlert() - recovered.Resolution = nil - if got := resolvedAlertMessage(recovered); got != "pve1 on pve1 is now healthy" { + 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{recovered}, time.Now(), ""); title != "Pulse alert resolved: pve1" { + 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.", },