diff --git a/README.md b/README.md index 0c3ab0a..21758c0 100644 --- a/README.md +++ b/README.md @@ -286,7 +286,7 @@ includes: Each endpoint is sent each notification on its own, in the background. A delivery that fails (a network error, no reply within 10 seconds, or an HTTP -status of 400 or more) is retried up to 5 times: the first retry after about 1 +status other than 2xx) is retried up to 5 times: the first retry after about 1 second, each wait after that twice as long up to 60 seconds, every wait varied at random by up to 25%. A delivery still failing after that is logged and dropped. diff --git a/TODO.md b/TODO.md index 1834d71..368aa6c 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ milestone: https://git.eeqj.de/sneak/dnswatcher/milestone/7 # Completed Steps +- 2026-10-15: a notification endpoint that answers with a redirect, or any + status other than 2xx, is a failed delivery, retried and logged (closes #287). - 2026-10-15: a nameserver is asked at each of its IPv4 addresses in turn, so it has failed only when none of them gives a usable reply (closes #285). - 2026-10-15: the log names a notification endpoint (`slack`, `mattermost`, diff --git a/internal/notify/delivery_test.go b/internal/notify/delivery_test.go index 8d699d7..61a0e15 100644 --- a/internal/notify/delivery_test.go +++ b/internal/notify/delivery_test.go @@ -1185,6 +1185,42 @@ func sendToEachEndpoint(t *testing.T, baseURL, title string) string { return logs.String() } +// assertEachEndpointRetriedThenFailed fails the test unless, for each +// of ntfy, Slack and Mattermost, the lines of logs whose error +// contains reason are the retry line and then the final failure line. +func assertEachEndpointRetriedThenFailed( + t *testing.T, logs, reason string, +) { + t.Helper() + + // For each endpoint, the messages of its lines whose error + // contains reason, in the order they were logged. + got := map[string][]string{} + + for _, record := range parseLog(t, logs) { + if strings.Contains(record.Error, reason) { + got[record.Endpoint] = append( + got[record.Endpoint], record.Msg, + ) + } + } + + lines := []string{ + "notification delivery failed, retrying", + "failed to send notification after retries", + } + want := map[string][]string{ + endpointNtfy: lines, + endpointSlack: lines, + endpointMattermost: lines, + } + + if !maps.EqualFunc(got, want, slices.Equal) { + t.Errorf("lines whose error contains %q = %v, want %v", + reason, got, want) + } +} + // TestSendNotificationDebugLogOmitsWebhookURL checks that, with debug // logging on, a delivery to each endpoint logs the endpoint's name and // the title, and no part of the URL's path or query. @@ -1240,33 +1276,7 @@ func TestSendNotificationErrorLogOmitsWebhookURL(t *testing.T) { logs := sendToEachEndpoint(t, srv.URL, "t") assertNoWebhookSecret(t, logs) - - // For each endpoint, the messages of its lines whose error names - // the refused connection, in the order they were logged. - got := map[string][]string{} - - for _, record := range parseLog(t, logs) { - if strings.Contains(record.Error, "connection refused") { - got[record.Endpoint] = append( - got[record.Endpoint], record.Msg, - ) - } - } - - lines := []string{ - "notification delivery failed, retrying", - "failed to send notification after retries", - } - want := map[string][]string{ - endpointNtfy: lines, - endpointSlack: lines, - endpointMattermost: lines, - } - - if !maps.EqualFunc(got, want, slices.Equal) { - t.Errorf("lines whose error names the refused connection = %v, "+ - "want %v", got, want) - } + assertEachEndpointRetriedThenFailed(t, logs, "connection refused") } // ── SlackPayload JSON marshaling ────────────────────────── diff --git a/internal/notify/notify.go b/internal/notify/notify.go index 6ed09ac..e17e615 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -25,9 +25,6 @@ import ( // HTTP client timeout. const httpClientTimeout = 10 * time.Second -// HTTP status code thresholds. -const httpStatusClientError = 400 - // Sentinel errors for notification failures. var ( // ErrNtfyFailed indicates the ntfy request failed. @@ -344,7 +341,10 @@ func (svc *Service) sendNtfy( defer func() { _ = resp.Body.Close() }() - if resp.StatusCode >= httpStatusClientError { + // Only a 2xx reply means ntfy took the notification. A + // redirect is not followed, so it is a failure too. + if resp.StatusCode < http.StatusOK || + resp.StatusCode >= http.StatusMultipleChoices { return fmt.Errorf( "%w: status %d", ErrNtfyFailed, resp.StatusCode, @@ -385,9 +385,9 @@ type SlackAttachment struct { // sendSlack posts to a Slack or Mattermost incoming webhook, which // take the same payload. endpoint, "slack" or "mattermost", names // the webhook in the log in place of its URL, which lets anyone who -// has it post to the channel. An HTTP error status is returned -// wrapped in failed, ErrSlackFailed or ErrMattermostFailed, so the -// error names the endpoint. +// has it post to the channel. A reply status other than 2xx is +// returned wrapped in failed, ErrSlackFailed or ErrMattermostFailed, +// so the error names the endpoint. func (svc *Service) sendSlack( ctx context.Context, webhookURL *url.URL, @@ -437,7 +437,10 @@ func (svc *Service) sendSlack( defer func() { _ = resp.Body.Close() }() - if resp.StatusCode >= httpStatusClientError { + // Only a 2xx reply means the webhook took the notification. A + // redirect is not followed, so it is a failure too. + if resp.StatusCode < http.StatusOK || + resp.StatusCode >= http.StatusMultipleChoices { return fmt.Errorf( "%w: status %d", failed, resp.StatusCode, diff --git a/internal/notify/retry_test.go b/internal/notify/retry_test.go index 4d47897..2fd9f58 100644 --- a/internal/notify/retry_test.go +++ b/internal/notify/retry_test.go @@ -536,3 +536,33 @@ func TestSendNotificationPermanentFailureLogsError( return attempts >= 3 }) } + +// TestSendNotificationRedirectFails checks that a 302 reply is a +// failed delivery at each endpoint: the server is sent the first +// attempt and the retry, and each endpoint logs a retry line and +// then the final failure line, both naming the 302. +func TestSendNotificationRedirectFails(t *testing.T) { + t.Parallel() + + var requests atomic.Int64 + + srv := httptest.NewServer( + http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + http.Redirect(w, r, "/moved", http.StatusFound) + }), + ) + defer srv.Close() + + logs := sendToEachEndpoint(t, srv.URL, "t") + + // ntfy, Slack and Mattermost each send once and retry once. + const wantRequests = 3 * 2 + + if sent := requests.Load(); sent != wantRequests { + t.Errorf("server got %d requests, want %d", sent, wantRequests) + } + + assertEachEndpointRetriedThenFailed(t, logs, "status 302") +}