fix: retry transient notification delivery failures - #454
Conversation
Every HTTP-based notification kind (Slack, Discord, Telegram, PagerDuty, Teams, Mattermost, Lark, RocketChat, Webex, Google Chat, Opsgenie, Gotify, Ntfy, Resend, and the generic webhook, for both alert rules and deploy-outcome notifications) funnels through one send path, postJSONWithAuth. It made exactly one attempt: a transient 500 or timeout from the receiver was recorded as a permanent failure with no second chance, indistinguishable from a genuinely broken channel. For a crashloop or deploy-failure alert, that meant a real, time-sensitive notification could be silently lost to a one-off receiver hiccup. postJSONWithAuth now retries up to 3 times with a short exponential backoff (500ms, 1s) on a transient failure: a transport-level error (DNS, TLS, connection refused, a client-side timeout) or a 5xx/429 response. Any other status (a malformed payload, a bad credential, a 404'd webhook URL) still fails on the first attempt, since retrying an inherently-wrong request only delays surfacing the real, fixable problem. The retry loop also respects context cancellation during its backoff wait rather than blindly sleeping through it. deploy_notify.go already reuses this exact same postJSONWithAuth/ postJSON pair for every one of its own channel kinds, so deploy-outcome notifications get the same retry behavior with no separate change. What this doesn't do: - Doesn't cover email: it sends through the control plane's own SMTP client, a different transport with different failure semantics (auth failures shouldn't retry the same way a connection timeout should), not wired into this retry path. - Doesn't add exposed delivery-attempt-count or retry-history detail to the notification_deliveries table or the dashboard's delivery history view: a delivery row still just records the final outcome, the same shape as before, not each individual attempt within it. - Doesn't surface a channel's recent failure rate anywhere at a glance (the channel list still only shows enabled/disabled): an operator still has to open a channel's own delivery history to notice it's been failing. Left as a separate follow-up.
github-actions
Bot
added
size/l
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesThe notification HTTP path now retries transient transport, 5xx, and 429 failures up to three attempts with exponential backoff. Permanent client errors stop immediately. Tests and observability documentation cover the behavior. Notification delivery retries
Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Notify
participant postJSONWithAuth
participant NotificationEndpoint
Notify->>postJSONWithAuth: deliver notification
postJSONWithAuth->>NotificationEndpoint: send HTTP request
NotificationEndpoint-->>postJSONWithAuth: transient failure
postJSONWithAuth->>postJSONWithAuth: wait with exponential backoff
postJSONWithAuth->>NotificationEndpoint: retry request
NotificationEndpoint-->>postJSONWithAuth: success or final failure
postJSONWithAuth-->>Notify: return result
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for attempt := 1; attempt <= notifyMaxAttempts; attempt++ { | ||
| lastErr = postJSONAttempt(ctx, client, url, body, authHeader) |
There was a problem hiding this comment.
If a receiver processes a POST but its response is lost or the client times out, this loop sends the same payload again. Because the generic webhook payload has no idempotency identifier, receivers that do not deduplicate requests may emit the same alert multiple times. This creates at-least-once delivery behavior that should be accounted for, ideally with receiver-supported deduplication where available.
| func isRetryableNotifyError(err error) bool { | ||
| var httpErr *notifyHTTPError | ||
| if errors.As(err, &httpErr) { | ||
| return httpErr.statusCode >= http.StatusInternalServerError || httpErr.statusCode == http.StatusTooManyRequests | ||
| } | ||
| return true |
There was a problem hiding this comment.
Every error other than notifyHTTPError is classified as retryable, including deterministic request-construction and client-configuration errors. A user-supplied malformed or unsupported URL therefore incurs all three attempts and 1.5 seconds of backoff even though it cannot succeed, delaying later rules in the sequential notification pass. Please distinguish permanent request errors from retryable network failures.
| case <-ctx.Done(): | ||
| timer.Stop() | ||
| return lastErr |
There was a problem hiding this comment.
When the context is canceled during backoff, this branch returns the error from the preceding delivery attempt rather than ctx.Err(). For example, a deadline exceeded while waiting after a 503 is reported as another receiver failure, obscuring that delivery stopped because the caller canceled the operation.
| func TestNotify_TransportError_Retries(t *testing.T) { | ||
| // A server that's already closed: every request fails at the | ||
| // transport level (connection refused), never even reaching an HTTP | ||
| // status, the other retryable case isRetryableNotifyError covers. | ||
| srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})) | ||
| url := srv.URL | ||
| srv.Close() | ||
|
|
||
| r := Rule{ID: "r1", NotifyURL: url} | ||
| notifier := NewNotifier(nil, nil, r) | ||
|
|
||
| if err := notifier.Notify(context.Background(), Event{Rule: r}); err == nil { | ||
| t.Fatal("Notify() error = nil, want an error against a closed server") | ||
| } | ||
| } |
There was a problem hiding this comment.
This test only checks that sending to a closed server returns an error, so it would still pass if transport failures were attempted exactly once. Add an observable attempt count, such as through a counting RoundTripper, so the test detects regressions that remove transport-error retries.
1 participant
Summary
Every HTTP-based notification kind (Slack, Discord, Telegram, PagerDuty, Teams, Mattermost, Lark, RocketChat, Webex, Google Chat, Opsgenie, Gotify, Ntfy, Resend, and the generic webhook, for both alert rules and deploy-outcome notifications) funnels through one send path,
postJSONWithAuth. It made exactly one attempt: a transient 500 or timeout from the receiver was recorded as a permanent failure with no second chance, indistinguishable from a genuinely broken channel. For a crashloop or deploy-failure alert, that meant a real, time-sensitive notification could be silently lost to a one-off receiver hiccup.postJSONWithAuthnow retries up to 3 times with a short exponential backoff (500ms, 1s) on a transient failure: a transport-level error (DNS, TLS, connection refused, a client-side timeout) or a 5xx/429 response. Any other status (a malformed payload, a bad credential, a 404'd webhook URL) still fails on the first attempt, since retrying an inherently-wrong request only delays surfacing the real, fixable problem. The retry loop also respects context cancellation during its backoff wait rather than blindly sleeping through it.deploy_notify.goalready reuses this exact samepostJSONWithAuth/postJSONpair for every one of its own channel kinds, so deploy-outcome notifications get the same retry behavior with no separate change.Verification
notifyMaxAttemptstimes then fails, not fewer or unbounded), a 4xx case (never retried, fails on the first attempt), a transport-error case (connection refused against a closed server), and a context-cancellation-during-backoff case (stops retrying oncectxis done instead of sleeping through it).notifyRetryBaseDelayis avar, shrunk to 1ms via a newTestMainfor this package, so the whole suite (including the pre-existingTestNotify_ReceiverErrorStatus_Errors, which now also retries) stays fast.go build ./...,go vet ./...,golangci-lint runclean. Fullgo test ./... -short(includingtest/e2e) passes.What this doesn't do
notification_deliveriestable or the dashboard's delivery history view: a delivery row still just records the final outcome, the same shape as before, not each individual attempt within it.Summary by CodeRabbit
New Features
Bug Fixes
Documentation