From 4224dbe96c6a71811a0ed482063ef19988216189 Mon Sep 17 00:00:00 2001 From: Niklas Ye Date: Fri, 7 Aug 2026 13:31:03 +0200 Subject: [PATCH] Record notification delivery on the incident timeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An incident's history went quiet after "Incident opened": nothing said that anybody had been paged, reminded, or told it resolved. Delivery lived only in the notifications outbox, which no API exposes, so when a page failed to arrive there was nothing in the product that said whether it had been sent. The notifier now writes two event types. A notified event once ntfy accepts the publish, carrying the kind in detail and the paged user in user_id — absent when the page went to the shared fallback topic, which belongs to nobody. And a notify_failed event when a notification exhausts its retries, which is the one worth having: without it a page that never landed leaves the timeline identical to one that did. Both are written from the delivery result rather than at enqueue. A queued notification is an intention, and the timeline is append-only, so claiming somebody was told before ntfy accepted it would be a lie that stays there. A failed timeline write is logged rather than returned, so it cannot make a delivered row look unsent and send the page twice. The topic is deliberately in neither: it is a shared secret with the ntfy server, and every API key can read the timeline. No migration — incident_events.type is free text, unlike notifications.kind. --- README.md | 18 ++++- internal/api/notifier.go | 31 ++++++++ internal/api/notify_test.go | 144 ++++++++++++++++++++++++++++++++++++ 3 files changed, 191 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6b5eb5b..3de20a5 100644 --- a/README.md +++ b/README.md @@ -235,6 +235,12 @@ Delivery is a queue, not an inline call: the webhook writes a row and a background notifier sends it within 30 seconds, retrying with exponential backoff up to 8 attempts. Nothing about ingestion blocks on ntfy being reachable. +Every delivery is recorded on the incident's timeline: a `notified` event once +ntfy accepts the publish, and a `notify_failed` event when a notification +exhausts its retries. Written from the result rather than at enqueue, so the +timeline says what actually happened — and a page that never landed is visible +instead of looking the same as one that did. + ### Stale alert expiry A resolved webhook is the only signal that an alert has stopped firing, so a @@ -353,8 +359,16 @@ name: degrade unknown values to "resolved, reason unknown". Types written today: `triggered`, `alert_added`, `alert_resolved`, `acknowledged`, `unacknowledged`, `assigned`, `snoozed`, `unsnoozed`, `resolved`, -`note`. On an `assigned` event `user_id` is the **assignee**, not the actor. New -types may be added; render unknown ones generically rather than dropping them. +`note`, `notified`, `notify_failed`. On an `assigned` event `user_id` is the +**assignee**, not the actor. New types may be added; render unknown ones +generically rather than dropping them. + +On `notified` and `notify_failed`, `detail` carries the notification kind +(`triggered` | `reminder` | `resolved`), and on a failure the reason after it. +`user_id` is who was paged — absent means the page went to the shared fallback +topic and so belongs to nobody. The topic itself is never written to the +timeline: it is a shared secret with the ntfy server, and every API key can read +this. ### Alerts diff --git a/internal/api/notifier.go b/internal/api/notifier.go index 348e625..71072f0 100644 --- a/internal/api/notifier.go +++ b/internal/api/notifier.go @@ -45,6 +45,19 @@ const ( notifyResolved = "resolved" ) +// Timeline event types the notifier writes, so an incident's history says who +// was paged and whether the page landed. Written from the delivery result +// rather than at enqueue: a queued notification is an intention, and claiming +// somebody was told before ntfy accepted it would be a lie the timeline keeps. +// +// The topic is deliberately absent from both. It is a shared secret with the +// ntfy server — anyone holding it can publish to it — and the timeline is +// readable by every API key. +const ( + eventNotified = "notified" + eventNotifyFailed = "notify_failed" +) + // NotifyConfig is everything the notifier needs to reach ntfy and to build URLs // a phone can follow back to this server. type NotifyConfig struct { @@ -208,6 +221,11 @@ func deliverPending(ctx context.Context, db *sql.DB, cfg NotifyConfig) { time.Now().Unix(), n.id); err != nil { log.Printf("notifier: mark sent %d: %v", n.id, err) } + // Logged, not returned: the page has already gone out, and treating a + // failed timeline write as a failed delivery would send it again. + if err := logEvent(ctx, db, n.incidentID, eventNotified, n.userID, nil, &n.kind); err != nil { + log.Printf("notifier: log delivery of %d: %v", n.id, err) + } sent++ } if sent > 0 { @@ -243,6 +261,11 @@ func pendingNotifications(ctx context.Context, db *sql.DB) ([]outboxRow, error) } // markFailed bumps the attempt count and pushes the row out to its next retry. +// +// The attempt that exhausts the budget also writes a timeline event. Without it +// a page that never landed leaves the incident's history identical to one that +// did, which is the failure most worth seeing: nobody was told, and nothing +// says so. func markFailed(ctx context.Context, db *sql.DB, n outboxRow, cause error) { next := time.Now().Add(retryDelay(n.attempts)).Unix() if _, err := db.ExecContext(ctx, @@ -250,6 +273,14 @@ func markFailed(ctx context.Context, db *sql.DB, n outboxRow, cause error) { next, cause.Error(), n.id); err != nil { log.Printf("notifier: mark failed %d: %v", n.id, err) } + + if n.attempts+1 < notifyMaxAttempts { + return + } + detail := fmt.Sprintf("%s: %s", n.kind, cause) + if err := logEvent(ctx, db, n.incidentID, eventNotifyFailed, n.userID, nil, &detail); err != nil { + log.Printf("notifier: log failure of %d: %v", n.id, err) + } } // retryDelay doubles the wait per attempt, up to notifyRetryMax. diff --git a/internal/api/notify_test.go b/internal/api/notify_test.go index cf7ab02..91062b6 100644 --- a/internal/api/notify_test.go +++ b/internal/api/notify_test.go @@ -199,6 +199,150 @@ func TestNotify_DeliveredOnlyOnce(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Delivery on the timeline +// --------------------------------------------------------------------------- + +// notifyEvents picks the notifier's entries out of an incident's timeline. +// Asserted through the API rather than the table: the timeline is what the +// clients read, so its shape is the contract worth covering. +func notifyEvents(t *testing.T, s *ts, id int) []map[string]any { + t.Helper() + var out []map[string]any + for _, e := range timeline(t, s, id) { + if e["type"] == "notified" || e["type"] == "notify_failed" { + out = append(out, e) + } + } + return out +} + +func TestNotify_DeliveryIsRecordedOnTheTimeline(t *testing.T) { + s, _ := notifyTS(t, api.NotifyConfig{PublicURL: "https://terdut.example.com"}) + + fireCritical(t, s) + // Queued is not notified: nothing is on the timeline until ntfy accepts it. + if got := notifyEvents(t, s, 1); len(got) != 0 { + t.Fatalf("expected no event before delivery, got %v", got) + } + + s.sweepNotify(t) + + events := notifyEvents(t, s, 1) + if len(events) != 1 { + t.Fatalf("expected 1 notification event, got %v", events) + } + e := events[0] + if e["type"] != "notified" { + t.Errorf("expected a notified event, got %v", e["type"]) + } + if e["detail"] != "triggered" { + t.Errorf("expected the kind in detail, got %v", e["detail"]) + } + if e["username"] != "admin" { + t.Errorf("expected the paged user attached, got %v", e["username"]) + } + // The topic is a shared secret with ntfy; the timeline is not the place for it. + for _, v := range e { + if s, ok := v.(string); ok && strings.Contains(s, "terdut-admin") { + t.Errorf("expected the topic kept out of the timeline, found it in %v", e) + } + } +} + +// A redelivery-free pass must not double-log either. +func TestNotify_TimelineRecordsOneEventPerDelivery(t *testing.T) { + s, _ := notifyTS(t, api.NotifyConfig{ + PublicURL: "https://terdut.example.com", + RepeatEvery: 15 * time.Minute, + }) + + fireCritical(t, s) + s.sweepNotify(t) + s.sweepNotify(t) + s.ageNotifications(t, 20*time.Minute) + s.sweepNotify(t) + + events := notifyEvents(t, s, 1) + if len(events) != 2 { + t.Fatalf("expected one event per delivery, got %v", events) + } + if events[0]["detail"] != "triggered" || events[1]["detail"] != "reminder" { + t.Errorf("expected triggered then reminder, got %v and %v", + events[0]["detail"], events[1]["detail"]) + } +} + +func TestNotify_AllClearIsRecordedOnTheTimeline(t *testing.T) { + s, _ := notifyTS(t, api.NotifyConfig{PublicURL: "https://terdut.example.com"}) + + fireCritical(t, s) + s.sweepNotify(t) + + postWebhook(t, s, []map[string]any{ + amAlert("fp-notify", "DiskFull", "resolved", "2026-05-20T10:00:00Z", + "2026-05-20T11:00:00Z", map[string]string{"severity": "critical"}), + }, "{}:{alertname=\"DiskFull\"}") + s.sweepNotify(t) + + events := notifyEvents(t, s, 1) + if len(events) != 2 { + t.Fatalf("expected the all-clear recorded, got %v", events) + } + if events[1]["detail"] != "resolved" { + t.Errorf("expected a resolved event, got %v", events[1]["detail"]) + } +} + +// A page to the shared fallback belongs to nobody, and the timeline has to say +// so rather than attributing it to whoever happens to be on call now. +func TestNotify_FallbackDeliveryHasNoUser(t *testing.T) { + f := newFakeNtfy(t) + s := newTS(t, api.NotifyConfig{ + BaseURL: f.URL, + FallbackTopic: "terdut-oncall", + PublicURL: "https://terdut.example.com", + }) + + fireCritical(t, s) + s.sweepNotify(t) + + events := notifyEvents(t, s, 1) + if len(events) != 1 { + t.Fatalf("expected 1 notification event, got %v", events) + } + if got, ok := events[0]["username"]; ok && got != nil && got != "" { + t.Errorf("expected no user on a fallback-topic page, got %v", got) + } +} + +// The failure worth seeing: nobody was paged, and the timeline says so instead +// of looking exactly like a delivery that worked. +func TestNotify_ExhaustedRetriesAreRecordedOnce(t *testing.T) { + s, f := notifyTS(t, api.NotifyConfig{PublicURL: "https://terdut.example.com"}) + f.failWith(http.StatusInternalServerError) + + fireCritical(t, s) + // One pass per attempt, each made due by clearing the backoff the last one set. + for i := 0; i < 10; i++ { + s.sweepNotify(t) + s.exec(t, "UPDATE notifications SET send_after = ? WHERE sent_at IS NULL", + time.Now().Add(-time.Second).Unix()) + } + + events := notifyEvents(t, s, 1) + if len(events) != 1 { + t.Fatalf("expected exactly one failure event, got %v", events) + } + if events[0]["type"] != "notify_failed" { + t.Errorf("expected notify_failed, got %v", events[0]["type"]) + } + detail, _ := events[0]["detail"].(string) + if !strings.HasPrefix(detail, "triggered: ") || !strings.Contains(detail, "500") { + t.Errorf("expected the kind and the reason in %q", detail) + } +} + // --------------------------------------------------------------------------- // Acknowledging from the notification // ---------------------------------------------------------------------------