From bf2b9cc61b4e7e04dc581b791edecca9e88be3cb Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 19 Oct 2026 13:34:34 -0700
Subject: [PATCH] Look the endpoint up past the query cache, and keep a
 subscription a race refused

The lookup under the write lock repeated the one made before
validation, so the query cache could answer it with that earlier miss
even after another connection had registered the endpoint. It runs
uncached now.

Enabling notifications unsubscribed the browser on any refusal, so a
session signed out between authenticating and saving lost the browser's
subscription too, and the next session had nothing to post. Only a
refused endpoint unsubscribes it now.

From #371.

(cherry picked from commit aa93269ceb3e2173be9f57fdb3b6e9baef510586)
---
 .../users/push_subscriptions_controller.rb    | 18 ++++++-----
 .../controllers/notifications_controller.js   |  3 +-
 .../push_subscriptions_controller_test.rb     | 30 +++++++++++++++++++
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb
index a3517a83..6b33452f 100644
--- a/app/controllers/users/push_subscriptions_controller.rb
+++ b/app/controllers/users/push_subscriptions_controller.rb
@@ -28,15 +28,17 @@ class Users::PushSubscriptionsController < ApplicationController
 
     # A password change ends every session and subscription. A request that authenticated just before it mustn't
     # put one back, so the session is checked under the same write lock as the save, and the endpoint is looked up
-    # again there so two tabs posting at once register it once. Validation, which resolves the endpoint's address,
-    # runs before taking the lock.
+    # again there, past the query cache, so two tabs posting at once register it once. Validation, which resolves
+    # the endpoint's address, runs before taking the lock.
     def register_while_signed_in(subscription)
-      Push::Subscription.transaction do
-        if Session.exists?(Current.session.id)
-          if existing = @push_subscriptions.find_by(push_subscription_params)
-            existing.touch
-          else
-            subscription.save!(validate: false)
+      Push::Subscription.uncached do
+        Push::Subscription.transaction do
+          if Session.exists?(Current.session.id)
+            if existing = @push_subscriptions.find_by(push_subscription_params)
+              existing.touch
+            else
+              subscription.save!(validate: false)
+            end
           end
         end
       end
diff --git a/app/javascript/controllers/notifications_controller.js b/app/javascript/controllers/notifications_controller.js
index 3ce53a5c..f7dfe7ec 100644
--- a/app/javascript/controllers/notifications_controller.js
+++ b/app/javascript/controllers/notifications_controller.js
@@ -106,7 +106,8 @@ export default class extends Controller {
 
   async #syncPushSubscription(subscription) {
     const response = await post(this.subscriptionsUrlValue, { body: this.#extractJsonPayloadAsString(subscription), responseKind: "turbo-stream" })
-    if (!response.ok) subscription.unsubscribe()
+    // Only a refused endpoint is for good. A session signed out mid-request posts it again after signing in.
+    if (response.unprocessableEntity) subscription.unsubscribe()
   }
 
   async #requestPermissionAndSubscribe(registration) {
diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb
index 7815626b..cbfacb40 100644
--- a/test/controllers/users/push_subscriptions_controller_test.rb
+++ b/test/controllers/users/push_subscriptions_controller_test.rb
@@ -139,3 +139,33 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest
       { "endpoint" => "https://fcm.googleapis.com/fcm/send/abc123", "p256dh_key" => "123", "auth_key" => "456" }
     end
 end
+
+class Users::PushSubscriptionsControllerForeignWriteTest < ActionDispatch::IntegrationTest
+  self.use_transactional_tests = false
+
+  setup do
+    sign_in :david
+    stub_web_push_dns_resolution
+  end
+
+  test "a subscription another connection registers while this one validates isn't registered twice" do
+    params = { "endpoint" => "https://fcm.googleapis.com/fcm/send/foreign", "p256dh_key" => "123", "auth_key" => "456" }
+    Push::Subscription.any_instance.stubs(:valid?).with { foreign_insert(params); true }.returns(true)
+
+    post user_push_subscriptions_url, params: { push_subscription: params }
+
+    assert_response :ok
+    assert_equal 1, Push::Subscription.where(endpoint: params["endpoint"]).count
+  end
+
+  private
+    def foreign_insert(params)
+      unless @inserted
+        SQLite3::Database.new(ActiveRecord::Base.connection_db_config.database) do |database|
+          database.execute "INSERT INTO push_subscriptions (user_id, endpoint, p256dh_key, auth_key, created_at, updated_at) " \
+            "VALUES (?, ?, ?, ?, datetime('now'), datetime('now'))", [ users(:david).id, *params.values ]
+        end
+        @inserted = true
+      end
+    end
+end