Stamp webhook rate limiter only on successful delivery

internal/alert/notify.go

@@ -184,48 +184,59 @@ func (n *Notifier) dispatch(ctx context.Context, ev Event) {
 			n.log.Error("alert notification render failed", "url", wh.url, "error", err)
 			continue
 		}
-		n.deliver(ctx, wh, body)
+		if n.deliver(ctx, wh, body) {
+			n.recordSent(wh.url, ev)
+		}
 	}
 }
 
 // rateLimited reports whether delivering ev to url should be suppressed
-// because the previous delivery of the same metric to the same URL was too
-// recent, and records the time otherwise. The rate limit is keyed per
-// (url, metric, resource) so a fast-flapping metric can't flood a webhook,
-// while a distinct metric alerting in the same tick is still delivered. Only
-// firing events reach here (cleared events bypass the limiter in dispatch),
-// so it purely coalesces repeated escalations. The event timestamp (not wall
-// clock) drives the decision so it is deterministic and testable.
+// because the previous successful delivery of the same metric to the same
+// URL was too recent. The rate limit is keyed per (url, metric, resource) so
+// a fast-flapping metric can't flood a webhook, while a distinct metric
+// alerting in the same tick is still delivered. Only firing events reach
+// here (cleared events bypass the limiter in dispatch), so it purely
+// coalesces repeated escalations. The event timestamp (not wall clock) drives
+// the decision so it is deterministic and testable.
 func (n *Notifier) rateLimited(url string, ev Event) bool {
 	if n.minInterval <= 0 {
 		return false
 	}
 	key := url + "\x00" + ev.Metric + "\x00" + ev.Resource
-	if last, ok := n.lastSent[key]; ok && ev.At.Sub(last) < n.minInterval {
-		return true
+	last, ok := n.lastSent[key]
+	return ok && ev.At.Sub(last) < n.minInterval
+}
+
+// recordSent stamps the last-delivery time for (url, metric, resource) after
+// a successful delivery, so the rate limiter counts deliveries rather than
+// attempts: a failed delivery must not suppress the next firing.
+func (n *Notifier) recordSent(url string, ev Event) {
+	if n.minInterval <= 0 {
+		return
 	}
+	key := url + "\x00" + ev.Metric + "\x00" + ev.Resource
 	n.lastSent[key] = ev.At
-	return false
 }
 
 // deliver POSTs body to a webhook, retrying with exponential backoff on
 // failure until it succeeds, exhausts maxRetries, or ctx is canceled. It
-// always returns without panicking so a dead endpoint can't crash the worker.
-func (n *Notifier) deliver(ctx context.Context, wh webhook, body []byte) {
+// reports whether delivery ultimately succeeded, and always returns without
+// panicking so a dead endpoint can't crash the worker.
+func (n *Notifier) deliver(ctx context.Context, wh webhook, body []byte) bool {
 	backoff := n.backoff
 	for attempt := 0; ; attempt++ {
 		if err := n.post(ctx, wh, body); err == nil {
-			return
+			return true
 		} else if attempt >= n.maxRetries {
 			n.log.Error("alert notification giving up after retries",
 				"url", wh.url, "attempts", attempt+1, "error", err)
-			return
+			return false
 		} else {
 			n.log.Warn("alert notification delivery failed, will retry",
 				"url", wh.url, "attempt", attempt+1, "error", err)
 		}
 		if !sleepCtx(ctx, backoff) {
-			return // context canceled during backoff
+			return false // context canceled during backoff
 		}
 		backoff *= 2
 	}

internal/alert/notify_test.go

@@ -355,6 +355,61 @@ func TestNotifier_RateLimitIsPerMetric(t *testing.T) {
 	}
 }
 
+// A delivery that fails all retries must not be treated as "sent" for rate
+// limiting purposes: the next firing of the same metric, even inside the
+// rate-limit window, must still be delivered once the endpoint recovers
+// (regression test for #62).
+func TestNotifier_RateLimitDoesNotCountFailedDelivery(t *testing.T) {
+	var failing atomic.Bool
+	failing.Store(true)
+	ch := make(chan receivedRequest, 16)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		if failing.Load() {
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+		ch <- receivedRequest{body: body, contentType: r.Header.Get("Content-Type")}
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	n, err := NewNotifier(config.Alerts{
+		Webhooks:                  []config.Webhook{{URL: srv.URL}},
+		NotifyMinIntervalSeconds:  60,
+		NotifyMaxRetries:          0,
+		NotifyRetryBackoffSeconds: 0.001,
+	}, nil)
+	if err != nil {
+		t.Fatalf("NewNotifier: %v", err)
+	}
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	n.Start(ctx)
+
+	base := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
+	n.Notify([]Event{firedEvent(base)})
+
+	// The first delivery fails; no request is ever pushed onto ch for it, so
+	// give the worker time to exhaust its (zero) retries before proceeding.
+	time.Sleep(50 * time.Millisecond)
+
+	// The endpoint recovers, and a second firing of the same metric arrives
+	// 1s later — deep inside the 60s rate-limit window. Because the first
+	// delivery failed, this one must still go out.
+	failing.Store(false)
+	n.Notify([]Event{firedEvent(base.Add(time.Second))})
+
+	req := waitForRequest(t, ch)
+	var got eventView
+	if err := json.Unmarshal(req.body, &got); err != nil {
+		t.Fatalf("bad payload: %v", err)
+	}
+	if !got.At.Equal(base.Add(time.Second)) {
+		t.Fatalf("expected the second firing delivered after the first failed, got %+v", got)
+	}
+}
+
 func TestEventReaches(t *testing.T) {
 	cases := []struct {
 		name string