diff --git a/internal/api/handlers/admin_apple_push.go b/internal/api/handlers/admin_apple_push.go index 7028afdd..f5b858a5 100644 --- a/internal/api/handlers/admin_apple_push.go +++ b/internal/api/handlers/admin_apple_push.go @@ -3,8 +3,10 @@ package handlers import ( "encoding/json" "errors" + "math" "net/http" "os" + "strconv" "time" "github.com/Silo-Server/silo-server/internal/notifications" @@ -146,6 +148,9 @@ func (h *AdminApplePushHandler) HandleRegisterRelay(w http.ResponseWriter, r *ht return } status, code, message := mapRelayRegistrationError(err) + if errors.As(err, &relayErr) && relayErr.RetryAfter > 0 { + w.Header().Set("Retry-After", strconv.Itoa(max(1, int(math.Ceil(relayErr.RetryAfter.Seconds()))))) + } writeError(w, status, code, message) return } diff --git a/internal/api/handlers/admin_apple_push_test.go b/internal/api/handlers/admin_apple_push_test.go index b2213059..5786b3a6 100644 --- a/internal/api/handlers/admin_apple_push_test.go +++ b/internal/api/handlers/admin_apple_push_test.go @@ -103,6 +103,7 @@ func TestAdminApplePushHandlerRegistersRelayAndStoresKey(t *testing.T) { func TestAdminApplePushHandlerMapsRelayRateLimit(t *testing.T) { settings := &fakeServerSettingsStore{values: map[string]string{}} relay := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Retry-After", "60") writeJSON(w, http.StatusTooManyRequests, map[string]any{ "error": map[string]string{"code": "rate_limited", "message": "too many deployment registrations from this network"}, }) @@ -120,6 +121,9 @@ func TestAdminApplePushHandlerMapsRelayRateLimit(t *testing.T) { if rec.Code != http.StatusTooManyRequests { t.Fatalf("status = %d (%s), want 429", rec.Code, rec.Body.String()) } + if got := rec.Header().Get("Retry-After"); got != "60" { + t.Fatalf("Retry-After = %q, want 60", got) + } if settings.values[notifications.SettingPushRelayAPIKey] != "" { t.Fatal("api key was stored after relay rejected registration") } diff --git a/internal/notifications/push_sender.go b/internal/notifications/push_sender.go index e74a3782..70067065 100644 --- a/internal/notifications/push_sender.go +++ b/internal/notifications/push_sender.go @@ -25,10 +25,12 @@ var pushRetrySchedule = []time.Duration{ } const ( - pushMaxAttempts = 5 - pushDispatchQueue = 512 - pushRetryClaimLimit = 100 - relayAppleSendPath = "/v1/apple/send" + pushMaxAttempts = 5 + pushDispatchQueue = 512 + pushRetryClaimLimit = 100 + pushRelayRequestTimeout = 15 * time.Second + pushRelayMaxRetryAfter = 23 * time.Hour + relayAppleSendPath = "/v1/apple/send" ) func pushRetryDelay(completedAttempt int) (time.Duration, bool) { @@ -38,6 +40,30 @@ func pushRetryDelay(completedAttempt int) (time.Duration, bool) { return pushRetrySchedule[completedAttempt] - pushRetrySchedule[completedAttempt-1], true } +func pushRetryDelayWithHint(completedAttempt int, retryAfter time.Duration) (time.Duration, bool) { + delay, more := pushRetryDelay(completedAttempt) + if retryAfter > 0 { + // The relay retains idempotency state for 24 hours. Stay safely inside + // that window even if APNs returns an unusually large Retry-After value. + delay = min(retryAfter, pushRelayMaxRetryAfter) + } + return delay, more +} + +func terminalAPNsDeviceRejection(status int, code, message string) bool { + if status != http.StatusUnprocessableEntity || code != "apns_rejected" { + return false + } + const prefix = "APNs rejected the notification:" + reason := strings.TrimSpace(strings.TrimPrefix(message, prefix)) + switch reason { + case "BadDeviceToken", "InvalidToken", "DeviceTokenNotForTopic", "Unregistered": + return true + default: + return false + } +} + type pushRelayAppleRequest struct { Token string `json:"token"` Environment string `json:"environment"` @@ -85,12 +111,16 @@ type pushSender struct { } func newPushSender(devices *PushDeviceRepository, deliveries *DeliveryRepository, cipher *secret.Cipher, settings *Settings) *pushSender { + // The Worker allows APNs up to 10 seconds. Leave enough room for edge + // routing and response processing so Silo receives the relay's classified + // outcome instead of manufacturing an ambiguous client-side timeout. + client := newNotificationHTTPClient(nil, pushRelayRequestTimeout) return &pushSender{ devices: devices, deliveries: deliveries, cipher: cipher, settings: settings, - client: newWebhookHTTPClient(nil), + client: client, logger: slog.Default().With("component", "notifications.apple_push"), developmentRelayURL: os.Getenv("SILO_PUSH_RELAY_DEVELOPMENT_URL"), now: time.Now, @@ -156,10 +186,7 @@ func (s *pushSender) processAttempt(ctx context.Context, attempt PushDeliveryAtt } _ = s.devices.RecordPushFailure(ctx, device.ID, code, result.TerminalDevice) - delay, more := pushRetryDelay(attemptNumber) - if result.RetryAfter > 0 { - delay = result.RetryAfter - } + delay, more := pushRetryDelayWithHint(attemptNumber, result.RetryAfter) if more && !result.TerminalDevice && retryableHTTPStatus(result.HTTPStatus) { nextRetry := time.Now().Add(delay) return s.finalize(ctx, attempt, PushOutcomeRetrying, result.UpstreamReason, result.Message, statusPtr, result.RelayRequestID, &nextRetry) @@ -304,7 +331,7 @@ func (s *pushSender) sendWithCapability(ctx context.Context, attempt PushDeliver RelayRequestID: parsed.Error.RequestID, UpstreamReason: code, Message: strings.TrimSpace(message), - TerminalDevice: resp.StatusCode == http.StatusUnprocessableEntity && code == "apns_rejected", + TerminalDevice: terminalAPNsDeviceRejection(resp.StatusCode, code, message), } } diff --git a/internal/notifications/push_sender_test.go b/internal/notifications/push_sender_test.go index 94091f1a..65745d08 100644 --- a/internal/notifications/push_sender_test.go +++ b/internal/notifications/push_sender_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" ) type mapSettingStore map[string]string @@ -152,6 +153,65 @@ func TestPushSenderSendMapsRelayTerminalAPNsRejection(t *testing.T) { } } +func TestTerminalAPNsDeviceRejectionReasons(t *testing.T) { + for _, reason := range []string{ + "BadDeviceToken", + "InvalidToken", + "DeviceTokenNotForTopic", + "Unregistered", + } { + t.Run(reason, func(t *testing.T) { + message := "APNs rejected the notification: " + reason + if !terminalAPNsDeviceRejection(http.StatusUnprocessableEntity, "apns_rejected", message) { + t.Fatalf("reason %q was not terminal for the device", reason) + } + }) + } + + if terminalAPNsDeviceRejection( + http.StatusUnprocessableEntity, + "apns_rejected", + "APNs rejected the notification: PayloadTooLarge", + ) { + t.Fatal("request-level APNs rejection was terminal for the device") + } +} + +func TestPushSenderDoesNotDisableDeviceForRequestLevelAPNsRejection(t *testing.T) { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + _ = json.NewEncoder(w).Encode(pushRelayErrorResponse{ + Error: struct { + Code string `json:"code"` + Message string `json:"message"` + RequestID string `json:"request_id"` + }{ + Code: "apns_rejected", + Message: "APNs rejected the notification: PayloadTooLarge", + RequestID: "relay-request-request-rejection", + }, + }) + })) + defer server.Close() + + sender := newPushSender(nil, nil, nil, NewSettings(mapSettingReader{ + SettingPushRelayURL: server.URL, + SettingPushRelayAPIKey: "relay-key", + })) + sender.client = server.Client() + sender.developmentRelayURL = server.URL + + result := sender.send(context.Background(), PushDeliveryAttempt{ID: "attempt-1"}, &PushDevice{ + APNsEnvironment: APNsEnvironmentSandbox, + APNsTopic: ApplePushTopicSilo, + ServerDeviceID: "server-device-1", + }, strings.Repeat("a", 64)) + + if result.OK || result.TerminalDevice || result.HTTPStatus != http.StatusUnprocessableEntity { + t.Fatalf("request-level rejection result = %+v", result) + } +} + func TestPushSenderSendMapsRelayRetryAfter(t *testing.T) { server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Retry-After", "30") @@ -189,6 +249,20 @@ func TestPushSenderSendMapsRelayRetryAfter(t *testing.T) { } } +func TestPushSenderUsesRelayAwareTimeoutAndRetryHorizon(t *testing.T) { + sender := newPushSender(nil, nil, nil, NewSettings(mapSettingStore{})) + if sender.client.Timeout != pushRelayRequestTimeout { + t.Fatalf("relay client timeout = %s, want %s", sender.client.Timeout, pushRelayRequestTimeout) + } + + if delay, more := pushRetryDelayWithHint(1, 10*time.Second); !more || delay != 10*time.Second { + t.Fatalf("short Retry-After delay = %s, more = %v", delay, more) + } + if delay, more := pushRetryDelayWithHint(1, 24*time.Hour); !more || delay != pushRelayMaxRetryAfter { + t.Fatalf("capped Retry-After delay = %s, more = %v", delay, more) + } +} + func TestPushSenderRenewsExpiredCapabilityAndRetriesStableDelivery(t *testing.T) { token := strings.Repeat("a", 64) var sendKeys []string diff --git a/internal/notifications/relay_credentials.go b/internal/notifications/relay_credentials.go index 50860481..2d824f0f 100644 --- a/internal/notifications/relay_credentials.go +++ b/internal/notifications/relay_credentials.go @@ -51,9 +51,10 @@ type relayNestedError struct { } type RelayCredentialError struct { - Status int - Code string - Message string + Status int + Code string + Message string + RetryAfter time.Duration } func (e RelayCredentialError) Error() string { return e.Code } @@ -143,7 +144,12 @@ func RequestRelayCredential(ctx context.Context, client RelayHTTPDoer, relayURL, if message == "" { message = http.StatusText(resp.StatusCode) } - return RelayCredentialResult{}, RelayCredentialError{Status: resp.StatusCode, Code: code, Message: message} + return RelayCredentialResult{}, RelayCredentialError{ + Status: resp.StatusCode, + Code: code, + Message: message, + RetryAfter: parseRetryAfter(resp.Header.Get("Retry-After"), time.Now()), + } } var parsed relayCredentialResponse if err := json.Unmarshal(data, &parsed); err != nil { diff --git a/internal/notifications/webhook_http.go b/internal/notifications/webhook_http.go index e7f6721a..8fe24ad0 100644 --- a/internal/notifications/webhook_http.go +++ b/internal/notifications/webhook_http.go @@ -32,12 +32,19 @@ type webhookSendResult struct { Message string } -// newWebhookHTTPClient builds the delivery client: 10s total timeout, -// non-overridable TLS verification, bounded redirects, and a dialer Control -// hook that re-validates every resolved address at connect time (DNS -// rebinding mitigation — the guard runs on the address actually being -// connected to, each redirect hop included). +// newWebhookHTTPClient builds the webhook delivery client with its standard +// request timeout. func newWebhookHTTPClient(allowPrivate func() bool) *http.Client { + return newNotificationHTTPClient(allowPrivate, webhookRequestTimeout) +} + +// newNotificationHTTPClient builds a delivery client with non-overridable TLS +// verification, bounded redirects, and a dialer Control hook that re-validates +// every resolved address at connect time (DNS rebinding mitigation — the guard +// runs on the address actually being connected to, each redirect hop included). +// Callers may use a longer total timeout when an upstream service has its own +// request deadline that must expire before Silo gives up waiting for a response. +func newNotificationHTTPClient(allowPrivate func() bool, requestTimeout time.Duration) *http.Client { dialer := &net.Dialer{ Timeout: 5 * time.Second, Control: func(network, address string, _ syscall.RawConn) error { @@ -61,10 +68,10 @@ func newWebhookHTTPClient(allowPrivate func() bool) *http.Client { MaxIdleConns: 16, IdleConnTimeout: 60 * time.Second, TLSHandshakeTimeout: 5 * time.Second, - ResponseHeaderTimeout: webhookRequestTimeout, + ResponseHeaderTimeout: requestTimeout, } return &http.Client{ - Timeout: webhookRequestTimeout, + Timeout: requestTimeout, Transport: transport, CheckRedirect: func(req *http.Request, via []*http.Request) error { if len(via) >= webhookMaxRedirects {