Arthurnn/deadlocks

lib/statesman/adapters/active_record.rb

@@ -3,9 +3,6 @@
 module Statesman
   module Adapters
     class ActiveRecord
-      attr_reader :transition_class
-      attr_reader :parent_model
-
       JSON_COLUMN_TYPES = %w[json jsonb].freeze
 
       def self.database_supports_partial_indexes?
@@ -27,12 +24,15 @@ def initialize(transition_class, parent_model, observer, options = {})
         end
 
         @transition_class = transition_class
+        @transition_table = transition_class.arel_table
         @parent_model = parent_model
         @observer = observer
         @association_name =
           options[:association_name] || @transition_class.table_name
       end
 
+      attr_reader :transition_class, :transition_table, :parent_model
+
       def create(from, to, metadata = {})
         create_transition(from.to_s, to.to_s, metadata)
       rescue ::ActiveRecord::RecordNotUnique => e
@@ -63,26 +63,51 @@ 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,23 +121,100 @@ 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) # rubocop:disable Metrics/AbcSize
+        update = build_arel_manager(::Arel::UpdateManager)
+        update.table(transition_table)
+
+        update.where(
+          transition_table[parent_join_foreign_key.to_sym].eq(parent_model.id).and(
+            transition_table[:id].eq(most_recent_id).or(
+              transition_table[:most_recent].eq(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))
+        update.set(build_most_recents_update_all_values(most_recent_id))
+
+        # MySQL will validate index constraints across the intermediate result of an
+        # update. This means we must order our update to deactivate the previous
+        # most_recent before setting the new row to be true.
+        update.order(transition_table[:most_recent].desc) if db_mysql?
+
+        ::ActiveRecord::Base.connection.exec_update(update.to_sql, nil, [])
+      end
+
+      # Generates update_all Arel values that will touch the updated timestamp (if valid
+      # for this model) and set most_recent to true only for the transition with a
+      # matching most_recent ID.
+      #
+      # 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 = '...'
+      #      ...
+      #
+      # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
+      def build_most_recents_update_all_values(most_recent_id)
+        values = [
+          [
+            transition_table[:most_recent],
+            Arel::Nodes::SqlLiteral.new(
+              Arel::Nodes::Case.new.
+                when(transition_table[:id].eq(most_recent_id)).
+                  then(Arel::Nodes::True.new).
+                else(not_most_recent_value).to_sql,
+            ),
+          ],
+        ]
+
+        # Only if we support the updated at timestamps should we add this column to the
+        # update
+        updated_column, updated_at = updated_timestamp
+        if updated_column
+          values << [
+            transition_table[updated_column.to_sym],
+            updated_at,
+          ]
+        end
+
+        values
+      end
+      # rubocop:enable Metrics/MethodLength, Metrics/AbcSize
+
+      # Provide a wrapper for constructing an update manager which handles a breaking API
+      # change in Arel as we move into Rails >6.0.
+      #
+      # https://github.com/rails/rails/commit/7508284800f67b4611c767bff9eae7045674b66f
+      def build_arel_manager(manager)
+        if manager.instance_method(:initialize).arity.zero?
+          manager.new
         else
-          most_recent.update_all(with_updated_timestamp(most_recent: nil))
+          manager.new(::ActiveRecord::Base)
         end
       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
+        if transition_class.columns_hash["most_recent"].null == false
+          return Arel::Nodes::False.new
+        end
+
+        nil
+      end
+
       def next_sort_key
         (last && last.sort_key + 10) || 10
       end
@@ -168,7 +270,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
@@ -182,15 +285,16 @@ def with_updated_timestamp(params)
                    ActiveRecordTransition::DEFAULT_UPDATED_TIMESTAMP_COLUMN
                  end
 
-        return params if column.nil?
+        # No updated timestamp column, don't return anything
+        return nil if column.nil?
 
-        timestamp = if ::ActiveRecord::Base.default_timezone == :utc
-                      Time.now.utc
-                    else
-                      Time.now
-                    end
+        [
+          column, ::ActiveRecord::Base.default_timezone == :utc ? Time.now.utc : Time.now
+        ]
+      end
 
-        params.merge(column => timestamp)
+      def db_mysql?
+        ::ActiveRecord::Base.connection.adapter_name.downcase.starts_with?("mysql")
       end
     end