Run positioned saves at READ COMMITTED on MySQL

CHANGELOG.md

@@ -1,5 +1,8 @@
 ## [Unreleased]
 
+- BREAKING CHANGE: On MySQL, a save or destroy of a positioned record that begins its own transaction now runs at READ COMMITTED. This fixes duplicate positions (or unique index violations) when anything reads before the scope lock, such as a validation, a callback or nested attributes. It applies to every save on the model, along with its callbacks and any records it autosaves. A save that joins an open transaction, such as `list.items.create`, instead takes the lock as soon as it begins. If you rely on REPEATABLE READ or gap locks, opt out with `positioning_isolation = nil`. See the Concurrency section of the README.
+- Scope locks are now taken by key without loading the associated record, and ignore default scopes, so they no longer run a default scope's preloads or miss the rows it hides.
+
 ## [0.4.9] - 2026-11-06
 
 - Fix the healer not locking scopes without a `belongs_to` association (or with a nil one).

README.md

@@ -286,6 +286,30 @@ The queries that this gem runs (especially those that seek the next position int
 
 **Please Note SQLite Users:** Row locking isn't supported by SQLite. Since writes are non-concurrent by default, the worst you'll probably see are errors about the database being locked under high load.
 
+### MySQL and transaction isolation
+
+Under MySQL's default isolation level (REPEATABLE READ) a transaction's first read fixes the snapshot that its later reads see. Anything that reads before we take the lock (a validation, a callback, nested attributes) could hide changes committed while we waited for it, which leads to duplicate positions or unique index violations. PostgreSQL's default isn't affected, and SQLite doesn't allow concurrent writers.
+
+So on MySQL, when a save or destroy of a positioned record begins a transaction, that transaction runs at READ COMMITTED. There are a few things to be aware of:
+
+- This applies to every save on the model, not just those that change a position, and to everything in that transaction: its callbacks and any records it autosaves. Reading the same row twice in a callback can now give different results, and `FOR UPDATE` no longer stops other transactions inserting into the gaps between the rows it locks.
+- An open transaction's level can't be changed, so a save inside one runs at that transaction's level. That includes `list.items.create` and `list.items << item`, as Rails opens a transaction around them. We lock as soon as such a save begins, but that can't help if the transaction has already read, or if a parent is autosaving the record and has already validated it. In those cases, open the transaction yourself with `transaction(isolation: :read_committed)`.
+- A level you've chosen with `ActiveRecord.with_transaction_isolation_level` (Rails 8.1+) takes precedence.
+- This has no effect inside transactional tests, as they hold a transaction open. Set `use_transactional_tests = false` to exercise it.
+- MySQL needs `binlog_format` to be `ROW` (the default) or `MIXED` for writes at this level.
+
+You can turn this off, but a read before the lock can then lead to duplicate positions again:
+
+```ruby
+# For the whole app, in an initializer
+ActiveRecord::Base.positioning_isolation = nil
+
+# For one model
+class Item < ApplicationRecord
+  self.positioning_isolation = nil
+end
+```
+
 If you have any concerns or improvements please file a GitHub issue.
 
 ## Development

lib/positioning.rb

@@ -10,9 +10,75 @@ class Error < StandardError; end
 
   RelativePosition = Struct.new(:before, :after, keyword_init: true)
 
+  # Under MySQL's default REPEATABLE READ, a transaction's first plain read fixes the snapshot that its later reads
+  # see. Validations, callbacks and attribute assignment can all read before we lock the scope, which would hide
+  # changes committed while we waited for the lock. So a save that begins its own transaction runs at READ COMMITTED.
+  # An open transaction's level can't be changed (Rails opens one around `association.create`, for example), so a save
+  # that joins one takes the lock before anything else in the save can read.
+  module Isolation
+    def self.mysql?(connection)
+      connection.adapter_name.match?(/mysql|trilogy/i)
+    end
+
+    def self.level_for(model, connection)
+      return if connection.transaction_open? || !mysql?(connection)
+
+      # The app chose a level for this block with ActiveRecord.with_transaction_isolation_level (Rails 8.1+)
+      return if ActiveRecord.try(:default_transaction_isolation_level)
+
+      model.positioning_isolation
+    end
+
+    def with_transaction_returning_status
+      self.class.connection_pool.with_connection do |connection|
+        level = Isolation.level_for(self.class, connection)
+        next super unless level
+
+        status = nil
+        connection.transaction(isolation: level) do
+          status = super()
+          # Joining swallows the inner ActiveRecord::Rollback, so the real transaction needs telling too
+          raise ActiveRecord::Rollback unless status
+        end
+        status
+      end
+    end
+
+    def save(**)
+      lock_if_joining(:lock_for_save) { super }
+    end
+
+    def save!(**)
+      lock_if_joining(:lock_for_save) { super }
+    end
+
+    def destroy
+      lock_if_joining(:lock_for_destroy) { super }
+    end
+
+    private
+
+    def lock_if_joining(lock)
+      self.class.connection_pool.with_connection do |connection|
+        if connection.transaction_open? && Isolation.mysql?(connection)
+          self.class.base_class.positioning_columns.each_key do |column|
+            Mechanisms.new(self, column).send(lock)
+          end
+        end
+
+        yield
+      end
+    end
+  end
+
   module Behaviour
     extend ActiveSupport::Concern
 
+    included do
+      class_attribute :positioning_isolation, instance_accessor: false, instance_predicate: false,
+        default: :read_committed
+    end
+
     class_methods do
       def positioning_columns
         @positioning_columns ||= {}
@@ -51,6 +117,8 @@ def positioned(on: [], column: :position)
             super(position)
           end
 
+          include Isolation
+
           before_create { Mechanisms.new(self, column).create_position }
           before_update { Mechanisms.new(self, column).update_position }
           before_destroy { Mechanisms.new(self, column).destroy_position }

lib/positioning/mechanisms.rb

@@ -56,6 +56,14 @@ def destroy_position
       end
     end
 
+    def lock_for_save
+      lock_positioning_scope! if @positioned.new_record? || positioning_scope_changed? || position_changed?
+    end
+
+    def lock_for_destroy
+      lock_positioning_scope! unless destroyed_via_positioning_scope?
+    end
+
     private
 
     def base_class
@@ -196,22 +204,37 @@ def positioning_scope
     end
 
     def lock_positioning_scope!
+      if @positioned.persisted? && positioning_scope_changed?
+        lock_scope scope_columns.index_with { |scope_column| @positioned.attribute_in_database(scope_column) }
+      end
+
+      lock_scope @positioned.slice(*scope_columns)
+    end
+
+    # Lock by key, without reading anything first. On MySQL a transaction's first plain read fixes its snapshot,
+    # which would hide changes committed while we waited for the lock. Default scopes are ignored so that they
+    # can't hide a row from the lock or preload anything.
+    def lock_scope(scope_attributes)
       if scope_associations.present?
         scope_associations.each do |scope_association|
-          if @positioned.persisted? && positioning_scope_changed?
-            associated_record = record_scope.first.send(scope_association)
-            associated_record.class.base_class.lock.find(associated_record.id) if associated_record
+          reflection = base_class.reflect_on_association(scope_association)
+          key = scope_attributes.values_at(*Array(reflection.foreign_key))
+
+          klass = if reflection.polymorphic?
+            type = scope_attributes[reflection.foreign_type]
+            base_class.polymorphic_class_for(type) if type
+          else
+            reflection.klass
           end
 
-          associated_record = @positioned.send(scope_association)
-          associated_record.class.base_class.lock.find(associated_record.id) if associated_record
+          if klass && key.none?(&:nil?)
+            klass.base_class.unscoped.where(Array(reflection.association_primary_key(klass)).zip(key).to_h).lock.ids
+          end
         end
       else
-        if @positioned.persisted? && positioning_scope_changed?
-          positioning_scope_was.lock.all.load
-        end
-
-        positioning_scope.lock.all.load
+        # Whole rows, as selecting only ids lets MySQL lock through a covering index on the position column. A
+        # transaction queued there would block our own position writes into the gap before it, and deadlock.
+        base_class.unscoped.where(scope_attributes).lock.all.load
       end
     end