diff --git a/js/app/components/settings/notifications-category-settings.tsx b/js/app/components/settings/notifications-category-settings.tsx index 9ef4f303..402c570c 100644 --- a/js/app/components/settings/notifications-category-settings.tsx +++ b/js/app/components/settings/notifications-category-settings.tsx @@ -46,16 +46,20 @@ export function NotificationsCategorySettings() { ? permission === "granted" && !!notificationToken : permission === "granted"; - // Re-check permission when the screen gains focus (the user may have - // changed it in browser settings). - const [refreshKey, setRefreshKey] = useState(0); + // Re-check permission when the browser's permission state changes (e.g. the + // user toggled it in browser settings while the app is open). + const [, forceRender] = useState(0); useEffect(() => { - if (!isWeb) return; - const interval = setInterval(() => setRefreshKey((k) => k + 1), 1000); - return () => clearInterval(interval); + if (!isWeb || typeof navigator.permissions === "undefined") return; + let status: PermissionStatus | undefined; + navigator.permissions.query({ name: "notifications" }).then((s) => { + status = s; + s.onchange = () => forceRender((k) => k + 1); + }); + return () => { + if (status) status.onchange = null; + }; }, [isWeb]); - // touch refreshKey so the linter doesn't complain and the re-render happens - void refreshKey; const handleToggle = async (value: boolean) => { if (busy) return; diff --git a/js/app/store/slices/platformSlice.ts b/js/app/store/slices/platformSlice.ts index 75e4b284..426457b1 100644 --- a/js/app/store/slices/platformSlice.ts +++ b/js/app/store/slices/platformSlice.ts @@ -145,19 +145,27 @@ export const createPlatformSlice: StateCreator< if (existing && existing.endpoint === sub.endpoint) { await existing.unsubscribe(); } - // Tell the server to drop the row. + // Tell the server to drop the row. Only clear the local token after + // the DELETE succeeds — otherwise the toggle shows "off" while the + // server keeps pushing to a subscription the user thought they + // disabled, with no way to retry. if (url) { - await fetch(`${url}/api/notification`, { + const res = await fetch(`${url}/api/notification`, { method: "DELETE", headers: { "content-type": "application/json" }, body: JSON.stringify({ token: notificationToken }), }); + if (!res.ok) { + throw new Error(`server delete failed: ${res.status}`); + } } + set({ notificationToken: null }); } } catch (e) { console.error("disableWebNotifications error", e); - } finally { - set({ notificationToken: null }); + // Leave notificationToken set so the toggle stays "on" and the user + // can retry. The browser-side unsubscribe may have already succeeded, + // but the server row is what matters for stopping future pushes. } }, webNotificationPermission: () => { diff --git a/pkg/api/api.go b/pkg/api/api.go index 331ab77c..bc8a9688 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -645,7 +645,10 @@ func (a *StreamplaceAPI) HandleVapidPublicKey(ctx context.Context) http.HandlerF return } w.Header().Set("Content-Type", "application/json") - if _, err := w.Write([]byte(`{"publicKey":"` + keys.PublicKey + `"}`)); err != nil { + resp := struct { + PublicKey string `json:"publicKey"` + }{PublicKey: keys.PublicKey} + if err := json.NewEncoder(w).Encode(resp); err != nil { log.Error(ctx, "error writing vapid public key", "error", err) } } diff --git a/pkg/atproto/sync.go b/pkg/atproto/sync.go index 401a6b3f..03aa1370 100644 --- a/pkg/atproto/sync.go +++ b/pkg/atproto/sync.go @@ -981,9 +981,15 @@ func (atsync *ATProtoSynchronizer) notifyBetaInvite(ctx context.Context, rec *pl } if err := atsync.Noter.Blast(ctx, targets, blast); err != nil { log.Error(ctx, "beta invite notification: blast failed", "did", rec.Did, "feature", rec.Feature, "err", err) - return + } else { + log.Log(ctx, "sent beta invite notification", "did", rec.Did, "feature", rec.Feature, "tokens", len(notifications)) + } + // Prune dead web push subscriptions so they don't accumulate. + for _, token := range notificationpkg.ExpiredTokens(err) { + if delErr := atsync.StatefulDB.DeleteNotification(token); delErr != nil { + log.Error(ctx, "beta invite notification: failed to prune expired", "token", token, "err", delErr) + } } - log.Log(ctx, "sent beta invite notification", "did", rec.Did, "feature", rec.Feature, "tokens", len(notifications)) } // betaInviteBlast builds the push payload for a newly-granted beta feature. diff --git a/pkg/notifications/multi.go b/pkg/notifications/multi.go index ae008dee..a4ec9c5e 100644 --- a/pkg/notifications/multi.go +++ b/pkg/notifications/multi.go @@ -2,7 +2,7 @@ package notifications import ( "context" - "fmt" + "errors" ) // MultiNotifier fans a single blast out to every transport it wraps. It @@ -42,11 +42,7 @@ func (m *MultiNotifier) Blast(ctx context.Context, targets []NotificationTarget, errs = append(errs, err) } } - if len(errs) == 1 { - return errs[0] - } - if len(errs) > 1 { - return fmt.Errorf("multi-notifier: %d transports failed: %v", len(errs), errs) - } - return nil + // errors.Join preserves the tree so callers can errors.As into the + // individual transport errors (e.g. to extract expired web tokens). + return errors.Join(errs...) } diff --git a/pkg/notifications/webpush.go b/pkg/notifications/webpush.go index ba5adbaa..8c8dbf9d 100644 --- a/pkg/notifications/webpush.go +++ b/pkg/notifications/webpush.go @@ -3,6 +3,7 @@ package notifications import ( "context" "encoding/json" + "errors" "fmt" "sync" @@ -43,6 +44,11 @@ func NewWebPushNotifier(keys VAPIDKeys, subscriber string) *WebPushNotifier { // Blast implements Notifier. It fans a push out to every web target in // parallel (each subscription is an independent HTTP POST to the browser // push service). Firebase targets are ignored. +// +// Expired subscriptions (410 Gone / 404) are collected and returned as a +// *BlastError whose Expired field holds the raw subscription tokens that +// should be pruned from the DB. Callers can extract them with +// ExpiredTokens(err). func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarget, blast *NotificationBlast) error { webTargets := make([]NotificationTarget, 0, len(targets)) for _, t := range targets { @@ -65,6 +71,7 @@ func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarge success int failed int errs []error + expired []string ) for _, t := range webTargets { @@ -77,6 +84,10 @@ func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarge if err != nil { failed++ errs = append(errs, err) + var expiredErr *ExpiredSubscriptionError + if errors.As(err, &expiredErr) { + expired = append(expired, token) + } log.Error(ctx, "web push failed", "err", err) } else { success++ @@ -85,14 +96,77 @@ func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarge } wg.Wait() - log.Log(ctx, "web push blast complete", "success", success, "failed", failed, "total", len(webTargets)) + log.Log(ctx, "web push blast complete", "success", success, "failed", failed, "total", len(webTargets), "expired", len(expired)) if len(errs) == 0 { return nil } - if len(errs) == 1 { - return errs[0] + return &BlastsError{ + Errs: errs, + Expired: expired, } - return fmt.Errorf("web push blast: %d of %d failed: %v", len(errs), len(webTargets), errs) +} + +// BlastsError wraps the per-target errors from a WebPushNotifier.Blast and +// exposes the subscription tokens that should be pruned because their +// endpoints returned 410 Gone / 404. +type BlastsError struct { + // Errs are all per-target errors, including ExpiredSubscriptionErrors. + Errs []error + // Expired holds the raw subscription tokens (DB primary keys) whose + // endpoints are dead and should be deleted. + Expired []string +} + +func (e *BlastsError) Error() string { + if len(e.Errs) == 1 { + return e.Errs[0].Error() + } + return fmt.Sprintf("web push blast: %d targets failed", len(e.Errs)) +} + +// Unwrap returns the wrapped errors so errors.Is / errors.As can traverse them. +func (e *BlastsError) Unwrap() []error { return e.Errs } + +// ExpiredTokens walks an error tree (handling errors.Join, MultiNotifier, and +// BlastsError wrapping) and returns the raw subscription tokens whose push +// endpoints returned 410 Gone / 404. Callers should delete these rows from +// the notifications table so dead subscriptions don't accumulate. +func ExpiredTokens(err error) []string { + if err == nil { + return nil + } + var expired []string + // If this node is a BlastsError, take its Expired slice directly and + // stop — recursing into its Unwrap() would only re-encounter the same + // ExpiredSubscriptionErrors and double-count. + var be *BlastsError + if errors.As(err, &be) { + return be.Expired + } + // Otherwise traverse children (errors.Join from MultiNotifier, or a + // single Unwrap chain) looking for nested BlastsErrors. + for _, inner := range errorsUnwrap(err) { + expired = append(expired, ExpiredTokens(inner)...) + } + return expired +} + +// errorsUnwrap returns the direct children of err for tree-walking. Supports +// errors.Join (Unwrap() []error) and single-wrap (Unwrap() error). +func errorsUnwrap(err error) []error { + // errors.Join / BlastsError expose Unwrap() []error + type multiUnwrapper interface{ Unwrap() []error } + if mu, ok := err.(multiUnwrapper); ok { + return mu.Unwrap() + } + // standard single-wrap + type unwrapper interface{ Unwrap() error } + if u, ok := err.(unwrapper); ok { + if inner := u.Unwrap(); inner != nil { + return []error{inner} + } + } + return nil } // sendOne decrypts the stored subscription JSON and POSTs the encrypted diff --git a/pkg/notifications/webpush_test.go b/pkg/notifications/webpush_test.go index 7a57b311..bd63554a 100644 --- a/pkg/notifications/webpush_test.go +++ b/pkg/notifications/webpush_test.go @@ -110,6 +110,58 @@ func TestMultiNotifierFanout(t *testing.T) { require.Equal(t, []string{"web-1"}, web.seen, "web notifier should only see web targets") } +// TestExpiredTokensExtraction confirms that ExpiredTokens walks the +// MultiNotifier → BlastsError tree and returns the dead subscription tokens +// so callers can prune them from the DB. +func TestExpiredTokensExtraction(t *testing.T) { + priv, pub, err := webpush.GenerateVAPIDKeys() + require.NoError(t, err) + notifier := NewWebPushNotifier(VAPIDKeys{PublicKey: pub, PrivateKey: priv}, "mailto:test@example.com") + + // Two web targets: one live, one that returns 410 Gone. + var liveSub, deadSub webpush.Subscription + _, livePub, err := webpush.GenerateVAPIDKeys() + require.NoError(t, err) + liveSub.Keys.P256dh = livePub + _, deadPub, err := webpush.GenerateVAPIDKeys() + require.NoError(t, err) + deadSub.Keys.P256dh = deadPub + + mux := http.NewServeMux() + liveHits := 0 + deadHits := 0 + mux.HandleFunc("/live", func(w http.ResponseWriter, r *http.Request) { + liveHits++ + w.WriteHeader(201) + }) + mux.HandleFunc("/dead", func(w http.ResponseWriter, r *http.Request) { + deadHits++ + w.WriteHeader(410) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + liveSub.Endpoint = srv.URL + "/live" + deadSub.Endpoint = srv.URL + "/dead" + liveJSON, _ := json.Marshal(liveSub) + deadJSON, _ := json.Marshal(deadSub) + + targets := []NotificationTarget{ + {Token: string(liveJSON), Type: NotificationTypeWeb}, + {Token: string(deadJSON), Type: NotificationTypeWeb}, + } + // Wrap in a MultiNotifier to test the full error tree the callers see. + multi := NewMultiNotifier(notifier) + err = multi.Blast(context.Background(), targets, &NotificationBlast{Title: "t", Body: "b"}) + require.Error(t, err) + + expired := ExpiredTokens(err) + require.Len(t, expired, 1, "only the dead subscription should be extracted") + require.Equal(t, string(deadJSON), expired[0]) + require.Equal(t, 1, liveHits) + require.Equal(t, 1, deadHits) +} + // recordingNotifier is a test double that records the tokens it was asked to // blast, filtered to a single type. type recordingNotifier struct { diff --git a/pkg/statedb/queue_processor.go b/pkg/statedb/queue_processor.go index cc765433..1756f1d4 100644 --- a/pkg/statedb/queue_processor.go +++ b/pkg/statedb/queue_processor.go @@ -497,6 +497,15 @@ func (state *StatefulDB) processNotificationTask(ctx context.Context, task *AppT } else { log.Log(ctx, "sent notifications", "user", userDID, "count", len(notifications), "content", nb) } + // Prune web push subscriptions whose endpoints returned 410 Gone / + // 404 — they're dead and would just fail again on every future blast. + for _, token := range notificationpkg.ExpiredTokens(err) { + if delErr := state.DeleteNotification(token); delErr != nil { + log.Error(ctx, "failed to prune expired notification", "token", token, "err", delErr) + } else { + log.Log(ctx, "pruned expired notification", "token", token) + } + } } else { log.Log(ctx, "no notifier configured, skipping notifications", "user", userDID, "count", len(notifications)) }