Inverse the order of writes on create transition

lib/statesman/adapters/active_record.rb

@@ -63,26 +63,50 @@ def last(force_reload: false)
 
       private
 
+      # rubocop:disable Metrics/MethodLength
       def create_transition(from, to, metadata)
-        transition_attributes = { to_state: to,
-                                  sort_key: next_sort_key,
-                                  metadata: metadata }
-
-        transition_attributes[:most_recent] = true
-
-        transition = transitions_for_parent.build(transition_attributes)
+        transition = transitions_for_parent.build(
+          default_transition_attributes(to, metadata),
+        )
 
         ::ActiveRecord::Base.transaction(requires_new: true) do
           @observer.execute(:before, from, to, transition)
-          unset_old_most_recent
+
+          # We save the transition first with most_recent falsy, then mark most_recent
+          # true after to avoid letting MySQL acquire a next-key lock which can cause
+          # deadlocks.
+          #
+          # To avoid an additional query, we manually adjust the most_recent attribute on
+          # our transition assuming that update_most_recents will have set it to true.
           transition.save!
+          unless update_most_recents(transition.id) > 0
+            raise ActiveRecord::Rollback, "failed to update most_recent"
+          end
+
+          transition.assign_attributes(most_recent: true)
+
           @last_transition = transition
           @observer.execute(:after, from, to, transition)
           add_after_commit_callback(from, to, transition)
         end
 
         transition
       end
+      # rubocop:enable Metrics/MethodLength
+
+      def default_transition_attributes(to, metadata)
+        transition_attributes = { to_state: to,
+                                  sort_key: next_sort_key,
+                                  metadata: metadata }
+
+        # see comment on `unset_old_most_recent` method
+        if transition_class.columns_hash["most_recent"].null == false
+          transition_attributes[:most_recent] = false
+        else
+          transition_attributes[:most_recent] = nil
+        end
+        transition_attributes
+      end
 
       def add_after_commit_callback(from, to, transition)
         ::ActiveRecord::Base.connection.add_transaction_record(
@@ -96,21 +120,58 @@ def transitions_for_parent
         parent_model.send(@association_name)
       end
 
-      def unset_old_most_recent
-        most_recent = transitions_for_parent.where(most_recent: true)
+      # Sets the given transition most_recent = t while unsetting the most_recent of any
+      # previous transitions.
+      def update_most_recents(most_recent_id)
+        transitions = transitions_for_parent
+        last_or_current = transitions.where(id: most_recent_id).or(
+          transitions.where(most_recent: true),
+        )
 
-        # Check whether the `most_recent` column allows null values. If it
-        # doesn't, set old records to `false`, otherwise, set them to `NULL`.
-        #
-        # Some conditioning here is required to support databases that don't
-        # support partial indexes. By doing the conditioning on the column,
-        # rather than Rails' opinion of whether the database supports partial
-        # indexes, we're robust to DBs later adding support for partial indexes.
-        if transition_class.columns_hash["most_recent"].null == false
-          most_recent.update_all(with_updated_timestamp(most_recent: false))
-        else
-          most_recent.update_all(with_updated_timestamp(most_recent: nil))
+        last_or_current.update_all(
+          build_most_recents_update_all(most_recent_id),
+        )
+      end
+
+      # Generates update_all parameters that will touch the updated timestamp (if valid
+      # for this model) and ensure only the transition with the most_recent_id has
+      # most_recent set to true.
+      #
+      # This is quite nasty, but combines two updates (set all most_recent = f, set
+      # current most_recent = t) into one, which helps improve transition performance
+      # especially when database latency is significant.
+      #
+      # The SQL this can help produce looks like:
+      #
+      #   update transitions
+      #      set most_recent = (case when id = 'PA123' then TRUE else FALSE end)
+      #        , updated_at = '...'
+      #      ...
+      #
+      def build_most_recents_update_all(most_recent_id)
+        clause = "most_recent = (case when id = ? then ? else ? end)"
+        parameters = [most_recent_id, true, not_most_recent_value]
+
+        updated_column, updated_at = updated_timestamp
+        if updated_column
+          clause += ", #{updated_column} = ?"
+          parameters.push(updated_at)
         end
+
+        [clause, *parameters]
+      end
+
+      # Check whether the `most_recent` column allows null values. If it doesn't, set old
+      # records to `false`, otherwise, set them to `NULL`.
+      #
+      # Some conditioning here is required to support databases that don't support partial
+      # indexes. By doing the conditioning on the column, rather than Rails' opinion of
+      # whether the database supports partial indexes, we're robust to DBs later adding
+      # support for partial indexes.
+      def not_most_recent_value
+        return false if transition_class.columns_hash["most_recent"].null == false
+
+        nil
       end
 
       def next_sort_key
@@ -166,7 +227,8 @@ def association_join_primary_key(association)
         end
       end
 
-      def with_updated_timestamp(params)
+      # updated_timestamp should return [column_name, value]
+      def updated_timestamp
         # TODO: Once we've set expectations that transition classes should conform to
         # the interface of Adapters::ActiveRecordTransition as a breaking change in the
         # next major version, we can stop calling `#respond_to?` first and instead
@@ -180,15 +242,12 @@ def with_updated_timestamp(params)
                    ActiveRecordTransition::DEFAULT_UPDATED_TIMESTAMP_COLUMN
                  end
 
-        return params if column.nil?
-
-        timestamp = if ::ActiveRecord::Base.default_timezone == :utc
-                      Time.now.utc
-                    else
-                      Time.now
-                    end
+        # No updated timestamp column, don't return anything
+        return nil if column.nil?
 
-        params.merge(column => timestamp)
+        [
+          column, ::ActiveRecord::Base.default_timezone == :utc ? Time.now.utc : Time.now
+        ]
       end
     end