From 883eeb905b519d0f15ba4b9ff3adb74f40d6791e Mon Sep 17 00:00:00 2001 From: dadachi Date: Mon, 5 Oct 2026 06:58:07 +0900 Subject: [PATCH 1/2] Retry device registration when a concurrent request wins the race Two first-time registrations of the same push token could both pass the uniqueness validation; the unique index then rejected the later INSERT with RecordNotUnique, a 500. Retry once: the lookup finds the row the other request created and updates it (200), like any re-registration. Reassigning a token to the caller stays as is: it's the documented way a device moves to whoever signs in on it, and pushes go to the device, so someone who obtained another person's token only redirects their own notifications to that device. Also update the AGENTS.md test count (505 tests after #134 and this). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01L4Pma9VAqB5A22KCEEin9R --- AGENTS.md | 2 +- .../api/v1/shopkeeper/devices_controller.rb | 30 +++++++++----- .../v1/shopkeeper/devices_controller_test.rb | 39 +++++++++++++++++++ 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ae09292..164478c 100644 --- a/AGENTS.md +++ b/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`. diff --git a/app/controllers/api/v1/shopkeeper/devices_controller.rb b/app/controllers/api/v1/shopkeeper/devices_controller.rb index 673ffc5..f6260c0 100644 --- a/app/controllers/api/v1/shopkeeper/devices_controller.rb +++ b/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 diff --git a/test/controllers/api/v1/shopkeeper/devices_controller_test.rb b/test/controllers/api/v1/shopkeeper/devices_controller_test.rb index 399f5f2..563ad0d 100644 --- a/test/controllers/api/v1/shopkeeper/devices_controller_test.rb +++ b/test/controllers/api/v1/shopkeeper/devices_controller_test.rb @@ -25,6 +25,39 @@ 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 + other_request = PG.connect(**ActiveRecord::Base.connection_db_config.configuration_hash.slice(:host, :port, :user, :password).merge(dbname: ActiveRecord::Base.connection_db_config.database).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) + # 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 +145,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 From 52d266c742d26b6dcc2cb305385045750d61ee2e Mon Sep 17 00:00:00 2001 From: dadachi Date: Mon, 5 Oct 2026 07:16:10 +0900 Subject: [PATCH 2/2] Connect the race test with the live connection's parameters The second connection read :user from configuration_hash, but the key is :username, so it only worked where the OS user matches the DB user. CI connects as postgres through DATABASE_URL and failed. Use the test connection's own conninfo, and only skip the callback if it was added. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01L4Pma9VAqB5A22KCEEin9R --- .../controllers/api/v1/shopkeeper/devices_controller_test.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/controllers/api/v1/shopkeeper/devices_controller_test.rb b/test/controllers/api/v1/shopkeeper/devices_controller_test.rb index 563ad0d..cfee870 100644 --- a/test/controllers/api/v1/shopkeeper/devices_controller_test.rb +++ b/test/controllers/api/v1/shopkeeper/devices_controller_test.rb @@ -30,7 +30,8 @@ class Api::V1::Shopkeeper::DevicesControllerTest < ActionDispatch::IntegrationTe # 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 - other_request = PG.connect(**ActiveRecord::Base.connection_db_config.configuration_hash.slice(:host, :port, :user, :password).merge(dbname: ActiveRecord::Base.connection_db_config.database).compact) + # 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]) @@ -49,7 +50,7 @@ class Api::V1::Shopkeeper::DevicesControllerTest < ActionDispatch::IntegrationTe assert_equal 1, devices.count assert_equal @shopkeeper, devices.first.owner ensure - ApplicationPushDevice.skip_callback(:create, :before, commit_competing_row) + 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 = -> {