fix: recover web push subscriptions after PWA reinstall (#24720)

coderd/webpush/webpush.go

@@ -29,6 +29,22 @@ import (
 
 const defaultSubscriptionCacheTTL = 3 * time.Minute
 
+// isStaleSubscriptionStatus reports whether a status code from a push
+// service indicates that the subscription is permanently invalid and
+// should be removed from the database. Other 4xx and 5xx responses
+// (rate limits, transient failures) leave the subscription in place
+// so it can be retried on the next dispatch.
+func isStaleSubscriptionStatus(statusCode int) bool {
+	switch statusCode {
+	case http.StatusBadRequest, // 400: malformed subscription per the push service.
+		http.StatusForbidden, // 403: Apple BadJwtToken / VAPID rejected, key rotation.
+		http.StatusNotFound,  // 404: FCM/Mozilla endpoint no longer valid.
+		http.StatusGone:      // 410: standard "subscription expired" signal.
+		return true
+	}
+	return false
+}
+
 // Dispatcher is an interface that can be used to dispatch
 // web push notifications to clients such as browsers.
 type Dispatcher interface {
@@ -203,11 +219,23 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, msg codersdk
 				return xerrors.Errorf("send webpush notification: %w", err)
 			}
 
-			if statusCode == http.StatusGone {
-				// The subscription is no longer valid, remove it.
+			if isStaleSubscriptionStatus(statusCode) {
+				// Remove subscriptions that the push service has marked as
+				// permanently invalid (Apple returns 403 BadJwtToken and 404
+				// for invalidated subscriptions, FCM returns 404 for
+				// expired endpoints, all push services return 410 for
+				// permanently gone subscriptions, and 400 indicates a
+				// malformed subscription that cannot be retried). Without
+				// this, stale rows accumulate after PWA reinstalls and the
+				// in-memory cache keeps trying to deliver to dead
+				// subscriptions.
 				mu.Lock()
 				cleanupSubscriptions = append(cleanupSubscriptions, subscription.ID)
 				mu.Unlock()
+			}
+
+			if statusCode == http.StatusGone {
+				// 410 Gone is informational, not a delivery error.
 				return nil
 			}
 
@@ -221,24 +249,43 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, msg codersdk
 		})
 	}
 
-	err = eg.Wait()
-	if err != nil {
-		return xerrors.Errorf("send webpush notifications: %w", err)
-	}
+	dispatchErr := eg.Wait()
 
-	if len(cleanupSubscriptions) > 0 {
-		// nolint:gocritic // These are known to be invalid subscriptions.
-		err = n.store.DeleteWebpushSubscriptions(dbauthz.AsNotifier(ctx), cleanupSubscriptions)
-		if err != nil {
-			n.log.Error(ctx, "failed to delete stale push subscriptions", slog.Error(err))
-		} else {
-			n.pruneSubscriptions(userID, cleanupSubscriptions)
-		}
+	// Always remove subscriptions that the push service rejected as
+	// permanently invalid, even when sibling deliveries returned a
+	// non-stale error. The cleanup must run before the error return so a
+	// transient delivery failure on one subscription cannot block the
+	// deletion of a 410/404/403/400 sibling. Without this ordering,
+	// stale rows accumulate after PWA reinstalls and silently mask the
+	// new subscription on every subsequent dispatch.
+	n.cleanupStaleSubscriptions(ctx, userID, cleanupSubscriptions)
+
+	if dispatchErr != nil {
+		return xerrors.Errorf("send webpush notifications: %w", dispatchErr)
 	}
 
 	return nil
 }
 
+// cleanupStaleSubscriptions deletes the rows the push service flagged as
+// permanently invalid (see isStaleSubscriptionStatus) and clears the cached
+// entries for the affected user. Failures are logged at error level rather
+// than returned: the caller is in the middle of returning a delivery error
+// and shouldn't have its error shadowed by a cleanup failure. The cache
+// prune is gated on a successful database delete so a partial state cannot
+// leak into the cache.
+func (n *Webpusher) cleanupStaleSubscriptions(ctx context.Context, userID uuid.UUID, ids []uuid.UUID) {
+	if len(ids) == 0 {
+		return
+	}
+	// nolint:gocritic // These are known to be invalid subscriptions.
+	if err := n.store.DeleteWebpushSubscriptions(dbauthz.AsNotifier(ctx), ids); err != nil {
+		n.log.Error(ctx, "failed to delete stale push subscriptions", slog.Error(err))
+		return
+	}
+	n.pruneSubscriptions(userID, ids)
+}
+
 func (n *Webpusher) subscriptionsForUser(ctx context.Context, userID uuid.UUID) ([]database.WebpushSubscription, error) {
 	if subscriptions, ok := n.cachedSubscriptions(userID); ok {
 		return subscriptions, nil

coderd/webpush/webpush_test.go

@@ -102,12 +102,14 @@ func TestPush(t *testing.T) {
 	})
 
 	t.Run("FailedDelivery", func(t *testing.T) {
+		// 5xx responses are transient failures. The subscription should
+		// remain after a failed delivery so it can be retried later.
 		t.Parallel()
 		ctx := testutil.Context(t, testutil.WaitShort)
 		manager, store, serverURL := setupPushTest(ctx, t, func(w http.ResponseWriter, r *http.Request) {
 			assertWebpushPayload(t, r)
-			w.WriteHeader(http.StatusBadRequest)
-			w.Write([]byte("Invalid request"))
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write([]byte("Internal server error"))
 		})
 
 		user := dbgen.User(t, store, database.User{})
@@ -123,7 +125,7 @@ func TestPush(t *testing.T) {
 		msg := randomWebpushMessage(t)
 		err = manager.Dispatch(ctx, user.ID, msg)
 		require.Error(t, err)
-		assert.Contains(t, err.Error(), "Invalid request")
+		assert.Contains(t, err.Error(), "Internal server error")
 
 		subscriptions, err := store.GetWebpushSubscriptionsByUserID(ctx, user.ID)
 		require.NoError(t, err)
@@ -131,6 +133,138 @@ func TestPush(t *testing.T) {
 		assert.Equal(t, subscriptions[0].ID, sub.ID, "The subscription should not be deleted")
 	})
 
+	// StaleSubscriptionStatuses verifies that documented permanent-failure
+	// status codes from the push service cause the subscription to be
+	// deleted. iOS Safari returns 404 and 403 BadJwtToken for invalidated
+	// subscriptions, FCM returns 404 for endpoints that are no longer
+	// valid, and a 400 means the subscription cannot be used.
+	t.Run("StaleSubscriptionStatuses", func(t *testing.T) {
+		t.Parallel()
+		cases := []struct {
+			name           string
+			statusCode     int
+			body           string
+			expectError    bool
+			expectErrorMsg string
+		}{
+			{
+				name:           "NotFound",
+				statusCode:     http.StatusNotFound,
+				body:           "Not Found",
+				expectError:    true,
+				expectErrorMsg: "Not Found",
+			},
+			{
+				name:           "Forbidden",
+				statusCode:     http.StatusForbidden,
+				body:           "BadJwtToken",
+				expectError:    true,
+				expectErrorMsg: "BadJwtToken",
+			},
+			{
+				name:           "BadRequest",
+				statusCode:     http.StatusBadRequest,
+				body:           "Invalid request",
+				expectError:    true,
+				expectErrorMsg: "Invalid request",
+			},
+		}
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+				ctx := testutil.Context(t, testutil.WaitShort)
+				manager, store, serverURL := setupPushTest(ctx, t, func(w http.ResponseWriter, r *http.Request) {
+					assertWebpushPayload(t, r)
+					w.WriteHeader(tc.statusCode)
+					w.Write([]byte(tc.body))
+				})
+				user := dbgen.User(t, store, database.User{})
+				_, err := store.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{
+					UserID:            user.ID,
+					Endpoint:          serverURL,
+					EndpointAuthKey:   validEndpointAuthKey,
+					EndpointP256dhKey: validEndpointP256dhKey,
+					CreatedAt:         dbtime.Now(),
+				})
+				require.NoError(t, err)
+
+				msg := randomWebpushMessage(t)
+				err = manager.Dispatch(ctx, user.ID, msg)
+				if tc.expectError {
+					require.Error(t, err)
+					assert.Contains(t, err.Error(), tc.expectErrorMsg)
+				} else {
+					require.NoError(t, err)
+				}
+
+				subscriptions, err := store.GetWebpushSubscriptionsByUserID(ctx, user.ID)
+				require.NoError(t, err)
+				assert.Len(t, subscriptions, 0, "Stale subscription should be deleted on %d", tc.statusCode)
+			})
+		}
+	})
+
+	// StaleAndFailedSubscriptions verifies that a stale subscription
+	// returning 404 is cleaned up even when a sibling subscription's
+	// delivery fails with a transient error in the same Dispatch call.
+	// Regression test for the case where a delivery error short-circuits
+	// stale subscription cleanup, leaving permanently invalid rows in
+	// the database.
+	t.Run("StaleAndFailedSubscriptions", func(t *testing.T) {
+		t.Parallel()
+		ctx := testutil.Context(t, testutil.WaitShort)
+
+		manager, store, server500URL := setupPushTest(ctx, t, func(w http.ResponseWriter, r *http.Request) {
+			assertWebpushPayload(t, r)
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write([]byte("transient error"))
+		})
+
+		serverStale := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			assertWebpushPayload(t, r)
+			w.WriteHeader(http.StatusNotFound)
+		}))
+		defer serverStale.Close()
+		serverStaleURL := serverStale.URL
+
+		user := dbgen.User(t, store, database.User{})
+
+		subFailed, err := store.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{
+			UserID:            user.ID,
+			Endpoint:          server500URL,
+			EndpointAuthKey:   validEndpointAuthKey,
+			EndpointP256dhKey: validEndpointP256dhKey,
+			CreatedAt:         dbtime.Now(),
+		})
+		require.NoError(t, err)
+
+		_, err = store.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{
+			UserID:            user.ID,
+			Endpoint:          serverStaleURL,
+			EndpointAuthKey:   validEndpointAuthKey,
+			EndpointP256dhKey: validEndpointP256dhKey,
+			CreatedAt:         dbtime.Now(),
+		})
+		require.NoError(t, err)
+
+		msg := randomWebpushMessage(t)
+		err = manager.Dispatch(ctx, user.ID, msg)
+		// Should still surface a delivery error from one of the
+		// failing siblings. errgroup returns whichever goroutine
+		// finishes with an error first, so the error may originate
+		// from either the 500 or the 404 sibling. The contract we
+		// care about is that the stale (404) subscription is
+		// cleaned up regardless of which error wins the race.
+		require.Error(t, err)
+
+		// The stale subscription should have been cleaned up regardless.
+		subscriptions, err := store.GetWebpushSubscriptionsByUserID(ctx, user.ID)
+		require.NoError(t, err)
+		if assert.Len(t, subscriptions, 1, "Only the transiently failing subscription should remain") {
+			assert.Equal(t, subFailed.ID, subscriptions[0].ID, "The transiently failing subscription should not be deleted")
+		}
+	})
+
 	t.Run("MultipleSubscriptions", func(t *testing.T) {
 		t.Parallel()
 		ctx := testutil.Context(t, testutil.WaitShort)