Retry device registration when a concurrent request wins the race

AGENTS.md

@@ -103,7 +103,7 @@ Cross-platform push via the `noticed` (v3) and `action_push_native` gems. APNs f
   - `json_response` for parsing JSON API responses
   - `create_new_auth_token` for generating auth headers (Devise Token Auth)
   - Fixtures in test/fixtures/ and seed data in db/fixtures/test/
-- Run tests: `bin/rails test` (500 tests, 1250 assertions)
+- Run tests: `bin/rails test` (505 tests, 1261 assertions)
 
 ### Development Server Configuration
 - `HOST` (Wi-Fi IP) and `PORT` are required in `.env`; `Procfile.dev` uses `${HOST:?...}` so Rails fails loudly if unset, and `development.rb` uses `ENV.fetch("HOST")` for `action_mailer.default_url_options`. Must match `NATIVEAPPTEMPLATE_API_DOMAIN` in the iOS scheme and Android `gradle.properties`. Never `127.0.0.1`, `localhost`, or `0.0.0.0`.

app/controllers/api/v1/shopkeeper/devices_controller.rb

@@ -10,15 +10,8 @@ class Api::V1::Shopkeeper::DevicesController < Api::V1::Shopkeeper::BaseControll
   def create
     authorize ApplicationPushDevice
 
-    device = ApplicationPushDevice.find_or_initialize_by(
-      platform: device_params[:platform],
-      token: device_params[:token]
-    )
-    device.owner = current_shopkeeper
-    device.bundle_id = device_params[:bundle_id]
-    device.last_active_at = Time.current
-
-    if device.save
+    device = save_device
+    if device.persisted?
       render json: ApplicationPushDeviceSerializer.new(device).serializable_hash, status: device.previously_new_record? ? :created : :ok
     else
       render_validation_error(device)
@@ -35,6 +28,25 @@ def destroy
 
   private
 
+  # Two first-time registrations of one token can both pass the uniqueness
+  # validation, and the unique index rejects the later INSERT. Retry once: the
+  # lookup then finds the row the other request created and updates it.
+  def save_device(retried: false)
+    device = ApplicationPushDevice.find_or_initialize_by(
+      platform: device_params[:platform],
+      token: device_params[:token]
+    )
+    device.owner = current_shopkeeper
+    device.bundle_id = device_params[:bundle_id]
+    device.last_active_at = Time.current
+    device.save
+    device
+  rescue ActiveRecord::RecordNotUnique
+    raise if retried
+
+    save_device(retried: true)
+  end
+
   def set_device
     @device = current_shopkeeper.application_push_devices.find(params[:id])
   end

test/controllers/api/v1/shopkeeper/devices_controller_test.rb

@@ -25,6 +25,40 @@ class Api::V1::Shopkeeper::DevicesControllerTest < ActionDispatch::IntegrationTe
     assert_equal "com.nativeapptemplate.example", attrs["bundle_id"]
   end
 
+  # Two first-time registrations of one token can both pass the uniqueness
+  # validation; the unique index then rejects the later INSERT. Simulate the
+  # other request winning: just before this request's INSERT, commit the same
+  # token from a separate connection (outside the test transaction).
+  test "create still succeeds when a concurrent request registered the same token first" do
+    # Same server and credentials as the test connection, whatever the config source (DATABASE_URL in CI)
+    other_request = PG.connect(**ActiveRecord::Base.connection.raw_connection.conninfo_hash.slice(:host, :port, :user, :password, :dbname).compact)
+    racer_id = shopkeepers(:two).id
+    commit_competing_row = -> {
+      other_request.exec_params(<<~SQL, [racer_id])
+        INSERT INTO action_push_native_devices (platform, token, owner_type, owner_id, last_active_at, created_at, updated_at)
+        VALUES ('apple', 'raced', 'Shopkeeper', $1, now(), now(), now())
+      SQL
+    }
+    ApplicationPushDevice.before_create(commit_competing_row)
+
+    post api_v1_shopkeeper_devices_url,
+      params: {device: {token: "raced", platform: "apple"}},
+      headers: @shopkeeper.create_new_auth_token
+
+    assert_response :ok
+    devices = ApplicationPushDevice.where(platform: "apple", token: "raced")
+    assert_equal 1, devices.count
+    assert_equal @shopkeeper, devices.first.owner
+  ensure
+    ApplicationPushDevice.skip_callback(:create, :before, commit_competing_row) if commit_competing_row
+    # The committed row is locked by this test's transaction until it rolls
+    # back, so delete it from the other connection only after that
+    @after_rollback = -> {
+      other_request&.exec("DELETE FROM action_push_native_devices WHERE platform = 'apple' AND token = 'raced'")
+      other_request&.close
+    }
+  end
+
   test "create registers a google (FCM) device and returns 201" do
     assert_difference -> { ApplicationPushDevice.count }, 1 do
       post api_v1_shopkeeper_devices_url,
@@ -112,4 +146,10 @@ class Api::V1::Shopkeeper::DevicesControllerTest < ActionDispatch::IntegrationTe
     end
     assert_response :not_found
   end
+
+  # Runs after the fixtures transaction has rolled back
+  def after_teardown
+    super
+    @after_rollback&.call
+  end
 end