MEM-10945: Fix App-Level Upsert Race Condition

CHANGELOG.md

@@ -52,3 +52,6 @@
 
 # Version 5.3.0
 * Changed salesforce_ar_sync to use bang (!) versions of Restforce CRUD methods to raise errors on Salesforce save failures instead of returning false.
+
+# Version 5.3.1
+* Fix race condition in app-level upsert by retrying on ActiveRecord::RecordNotUnique

lib/salesforce_ar_sync/salesforce_sync.rb

@@ -71,24 +71,49 @@ module ClassMethods
       def salesforce_update(attributes = {})
         raise ArgumentError, "#{salesforce_id_attribute_name} parameter required" if attributes[salesforce_id_attribute_name].blank?
 
+        attempt_salesforce_update(attributes)
+      end
+
+      private
+
+      def find_record(attributes)
         data_source = unscoped_updates ? unscoped : self
         object = data_source.find_by(salesforce_id: attributes[salesforce_id_attribute_name])
         object ||= data_source.find_by(activerecord_web_id_attribute_name => attributes[salesforce_web_id_attribute_name]) if salesforce_sync_web_id? && attributes[salesforce_web_id_attribute_name]
+        return object if object.present?
 
-        if !object && additional_lookup_fields
+        if additional_lookup_fields
           additional_lookup_fields.each do |attribute_name, salesforce_attribute_name|
             object = data_source.find_by(attribute_name => attributes[salesforce_attribute_name]) if attributes[salesforce_attribute_name]
+            break if object
           end
+          return object
         end
+      end
 
-        if object.nil?
-          object = new
+      def build_record(attributes)
+        new.tap do |object|
           salesforce_default_attributes_for_create.merge(salesforce_id: attributes[salesforce_id_attribute_name]).each_pair do |k, v|
             object.send("#{k}=", v)
           end
         end
+      end
+
+      def attempt_salesforce_update(attributes, retried: false)
+        object = find_record(attributes) || build_record(attributes)
+        return unless object
+
+        object.salesforce_process_update(attributes) if salesforce_stale?(object, attributes[:SystemModstamp])
+      rescue ActiveRecord::RecordNotUnique
+        raise if retried
+
+        attempt_salesforce_update(attributes, retried: true)
+      end
+
+      def salesforce_stale?(object, modstamp)
+        return false if modstamp.blank?
 
-        object.salesforce_process_update(attributes) if object && (object.salesforce_updated_at.nil? || (object.salesforce_updated_at && object.salesforce_updated_at < Time.parse(attributes[:SystemModstamp])))
+        object.salesforce_updated_at.nil? || object.salesforce_updated_at < Time.parse(modstamp)
       end
     end
 

lib/salesforce_ar_sync/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module SalesforceArSync
-  VERSION = '5.3.0'
+  VERSION = '5.3.1'
 end

spec/salesforce_ar_sync/salesforce_ar_sync_spec.rb

@@ -137,13 +137,48 @@ class TestSyncable < ActiveRecord::Base
     it 'looks for unscoped records when unscoped_updates is set' do
       sf_id = 1
       contact = Contact.new(salesforce_id: sf_id)
+      modstamp = '2012-03-26T19:54:50.000Z'
       allow(Contact).to receive(:unscoped_updates).and_return(true)
 
       expect(contact).to receive(:salesforce_process_update) { nil }
       expect(Contact).to receive(:unscoped).and_return(self)
       expect(self).to receive(:find_by).with(salesforce_id: sf_id).and_return(contact)
 
-      Contact.salesforce_update(Id: sf_id)
+      Contact.salesforce_update(Id: sf_id, SystemModstamp: modstamp)
+    end
+
+    context 'when an existing record is not found but ActiveRecord::RecordNotUnique is raised on save' do
+      let(:contact) do
+        Contact.new(
+          first_name: 'Bob',
+          last_name: 'Smith',
+          phone: '519 555-1212',
+          email: 'bsmith@example.com',
+          salesforce_skip_sync: true
+        )
+      end
+
+      before do
+        allow(Contact).to receive(:salesforce_sync_web_id?).and_return(false)
+        allow(Contact).to receive(:unscoped_updates?).and_return(false)
+        # simulate the record being created after the find but before the save
+        allow(Contact).to receive(:new).and_return(contact)
+        allow(contact).to receive(:save!).once do
+          contact.save
+          raise(ActiveRecord::RecordNotUnique)
+        end
+      end
+
+      it 'retries and updates the found record' do
+        expect(Contact).to receive(:new).once
+        expect(Contact).to receive(:find_by).with(salesforce_id: 321).twice.and_call_original
+
+        expect {
+          Contact.salesforce_update(Id: 321, SystemModstamp: '2024-01-01T00:00:00.000Z')
+        }.not_to raise_exception
+
+        expect(Contact.where(salesforce_id: 321)).to exist
+      end
     end
   end