From 13eda3598597eb1c92ac5371a795a29a72d47f98 Mon Sep 17 00:00:00 2001
From: Vyr Cossont <VyrCossont@users.noreply.github.com>
Date: Sun, 19 Jan 2025 14:16:44 -0800
Subject: [PATCH] Special-case 400 errors other than 408/429

Most client errors should remove the subscription.
---
 internal/webpush/realsender.go      | 23 ++++++++++++-
 internal/webpush/realsender_test.go | 53 +++++++++++++++++++++++++----
 2 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/internal/webpush/realsender.go b/internal/webpush/realsender.go
index 7b0f1a458..7f58390c1 100644
--- a/internal/webpush/realsender.go
+++ b/internal/webpush/realsender.go
@@ -218,13 +218,34 @@ func (r *realSender) sendToSubscription(
 		_ = resp.Body.Close()
 	}()
 
-	// If there's an error, log the response.
 	if resp.StatusCode < 200 || resp.StatusCode > 299 {
+		if resp.StatusCode >= 400 && resp.StatusCode <= 499 &&
+			resp.StatusCode != http.StatusRequestTimeout &&
+			resp.StatusCode != http.StatusTooManyRequests {
+			// We should not send any more notifications to this subscription. Try to delete it.
+			if err := r.state.DB.DeleteWebPushSubscriptionByTokenID(ctx, subscription.TokenID); err != nil {
+				return gtserror.Newf(
+					"received HTTP status %s but failed to delete subscription: %s",
+					resp.Status,
+					err,
+				)
+			}
+			log.Infof(
+				ctx,
+				"Deleted Web Push subscription with token ID %s because push server sent HTTP status %s",
+				subscription.TokenID,
+				resp.Status,
+			)
+			return nil
+		}
+
+		// Otherwise, try to get the response body.
 		bodyBytes, err := io.ReadAll(io.LimitReader(resp.Body, responseBodyMaxLen))
 		if err != nil {
 			return gtserror.Newf("error reading Web Push server response: %w", err)
 		}
 
+		// Log the error with its response body.
 		return gtserror.Newf(
 			"unexpected HTTP status %s received when sending Web Push notification: %s",
 			resp.Status,
diff --git a/internal/webpush/realsender_test.go b/internal/webpush/realsender_test.go
index 093314b7c..27203b657 100644
--- a/internal/webpush/realsender_test.go
+++ b/internal/webpush/realsender_test.go
@@ -178,9 +178,12 @@ func (rc *notifyingReadCloser) Close() error {
 	return nil
 }
 
-func (suite *RealSenderStandardTestSuite) TestSendSuccess() {
-	// Set a timeout on the whole test. If it fails due to the timeout,
-	// the push notification was not sent for some reason.
+// Simulate sending a push notification with the suite's fake web client.
+func (suite *RealSenderStandardTestSuite) simulatePushNotification(
+	statusCode int,
+	expectDeletedSubscription bool,
+) error {
+	// Don't let the test run forever if the push notification was not sent for some reason.
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
@@ -193,17 +196,17 @@ func (suite *RealSenderStandardTestSuite) TestSendSuccess() {
 		bodyClosed: make(chan struct{}, 1),
 	}
 
-	// Simulate a successful response from the Web Push server.
+	// Simulate a response from the Web Push server.
 	suite.webPushHttpClientDo = func(request *http.Request) (*http.Response, error) {
 		return &http.Response{
-			Status:     "200 OK",
-			StatusCode: 200,
+			Status:     http.StatusText(statusCode),
+			StatusCode: statusCode,
 			Body:       rc,
 		}, nil
 	}
 
 	// Send the push notification.
-	suite.NoError(suite.webPushSender.Send(ctx, notification, nil, nil))
+	sendError := suite.webPushSender.Send(ctx, notification, nil, nil)
 
 	// Wait for it to be sent or for the context to time out.
 	bodyClosed := false
@@ -216,6 +219,42 @@ func (suite *RealSenderStandardTestSuite) TestSendSuccess() {
 	}
 	suite.True(bodyClosed)
 	suite.False(contextExpired)
+
+	// Look for the associated Web Push subscription. Some server responses should delete it.
+	subscription, err := suite.state.DB.GetWebPushSubscriptionByTokenID(
+		ctx,
+		suite.testWebPushSubscriptions["local_account_1_token_1"].TokenID,
+	)
+	if expectDeletedSubscription {
+		suite.ErrorIs(err, db.ErrNoEntries)
+	} else {
+		suite.NotNil(subscription)
+	}
+
+	return sendError
+}
+
+// Test a successful response to sending a push notification.
+func (suite *RealSenderStandardTestSuite) TestSendSuccess() {
+	suite.NoError(suite.simulatePushNotification(http.StatusOK, false))
+}
+
+// Test a rate-limiting response to sending a push notification.
+// This should not delete the subscription.
+func (suite *RealSenderStandardTestSuite) TestRateLimited() {
+	suite.NoError(suite.simulatePushNotification(http.StatusTooManyRequests, false))
+}
+
+// Test a non-special-cased client error response to sending a push notification.
+// This should delete the subscription.
+func (suite *RealSenderStandardTestSuite) TestClientError() {
+	suite.NoError(suite.simulatePushNotification(http.StatusBadRequest, true))
+}
+
+// Test a server error response to sending a push notification.
+// This should not delete the subscription.
+func (suite *RealSenderStandardTestSuite) TestServerError() {
+	suite.NoError(suite.simulatePushNotification(http.StatusInternalServerError, false))
 }
 
 func TestRealSenderStandardTestSuite(t *testing.T) {