Use locking reads for positions once the scope is locked

CHANGELOG.md

@@ -1,6 +1,9 @@
 ## [Unreleased]
 
 - Fix the healer not locking scopes without a `belongs_to` association (or with a nil one).
+- Fix scope changes for records hidden by, or partially selected by, a default scope, and when changing part of a composite primary key.
+- Fix lists being corrupted when a record with a NULL position (or whose row is already gone) is updated or destroyed.
+- Fix stale position reads on MySQL (REPEATABLE READ) after waiting for the scope lock.
 
 ## [0.4.7] - 2025-08-09
 

lib/positioning/mechanisms.rb

@@ -31,7 +31,10 @@ def update_position
       solidify_position
       move_out_of_the_way
 
-      if positioning_scope_changed?
+      # A record without a position doesn't occupy a slot, so there's no gap to close
+      if position_was.nil?
+        expand(positioning_scope, position..)
+      elsif positioning_scope_changed?
         contract(positioning_scope_was, position_was..)
         expand(positioning_scope, position..)
       elsif position_was > position
@@ -46,7 +49,7 @@ def destroy_position
         lock_positioning_scope!
 
         move_out_of_the_way
-        contract(positioning_scope, (position_was + 1)..)
+        contract(positioning_scope, (position_was + 1)..) unless position_was.nil?
       end
     end
 
@@ -76,8 +79,9 @@ def quoted_column
       end
     end
 
+    # The record's row as it is in the database, regardless of default scopes or in-memory key changes
     def record_scope
-      base_class.where primary_key => [@positioned.id]
+      base_class.unscoped.where primary_key => [@positioned.id_in_database]
     end
 
     def position
@@ -96,8 +100,13 @@ def position_changed?
       @positioned.send :"#{@column}_changed?"
     end
 
+    # Reads of positions made after locking the scope are locking reads. Under MySQL's default
+    # REPEATABLE READ isolation a plain read returns the snapshot taken at the transaction's first
+    # read, which can predate changes committed while we were waiting for the lock.
     def position_was
-      @position_was ||= record_scope.pick(@column)
+      return @position_was if defined?(@position_was)
+
+      @position_was = record_scope.lock.pick(@column)
     end
 
     def move_out_of_the_way
@@ -151,7 +160,7 @@ def solidify_position
           relative_record_or_id
         end
 
-        relative_record_scope = positioning_scope.where(primary_key => [relative_id])
+        relative_record_scope = positioning_scope.where(primary_key => [relative_id]).lock
 
         unless relative_record_scope.exists?
           raise Error.new, "relative `#{@column}` record must be in the same scope"
@@ -171,7 +180,10 @@ def solidify_position
     end
 
     def last_position
-      (positioning_scope.maximum(@column) || 0) + (in_positioning_scope? ? 0 : 1)
+      # MAX() can't be combined with FOR UPDATE on PostgreSQL
+      maximum = positioning_scope.where.not(@column => nil).reorder(@column => :desc).lock.pick(@column)
+
+      (maximum || 0) + (in_positioning_scope? ? 0 : 1)
     end
 
     def scope_columns
@@ -211,7 +223,8 @@ def positioning_scope_was
     end
 
     def in_positioning_scope?
-      @positioned.persisted? && positioning_scope.where(primary_key => [@positioned.id]).exists?
+      @positioned.persisted? &&
+        positioning_scope.where(primary_key => [@positioned.id_in_database]).where.not(@column => nil).lock.exists?
     end
 
     def positioning_scope_changed?

test/models/archivable_item.rb

@@ -0,0 +1,7 @@
+class ArchivableItem < ActiveRecord::Base
+  belongs_to :list
+
+  positioned on: [:list, :archived_at]
+
+  default_scope { where(archived_at: nil) }
+end

test/support/active_record.rb

@@ -61,6 +61,15 @@ def configure_connection
 
     add_index :default_scope_items, [:list_id, :position], unique: true
 
+    create_table :archivable_items, force: true do |t|
+      t.string :name
+      t.integer :position, null: false
+      t.datetime :archived_at
+      t.references :list, null: false
+    end
+
+    add_index :archivable_items, [:list_id, :archived_at, :position], unique: true
+
     create_table :composite_primary_key_items, primary_key: [:item_id, :account_id], force: true do |t|
       t.integer :item_id, null: false
       t.integer :account_id, null: false

test/test_helper.rb

@@ -12,6 +12,7 @@
 require_relative "models/default_scope_item"
 require_relative "models/composite_primary_key_item"
 require_relative "models/composite_foreign_key_item"
+require_relative "models/archivable_item"
 require_relative "models/product"
 require_relative "models/category"
 require_relative "models/categorised_item"

test/test_positioning.rb

@@ -17,6 +17,50 @@ def test_struct_takes_keyword_arguments
 end
 
 class TestTransactionSafety < Minitest::Test
+  def test_no_stale_reads_after_waiting_for_the_lock
+    skip "SQLite doesn't support row locking" if ENV["DB"] == "sqlite"
+
+    ActiveRecord::Base.connection_handler.clear_all_connections!
+
+    list = List.create name: "List"
+    students = ["A", "B", "C", "D", "E"].map do |name|
+      list.authors.create name: name, type: "Author::Student"
+    end
+
+    locked = Queue.new
+
+    first_thread = Thread.new do
+      ActiveRecord::Base.connection_pool.with_connection do
+        ActiveRecord::Base.transaction do
+          List.lock.find(list.id)
+          locked << true
+          sleep 0.5
+          list.authors.create name: "New", type: "Author::Student", position: 1
+        end
+      end
+    end
+
+    locked.pop
+
+    second_thread = Thread.new do
+      ActiveRecord::Base.connection_pool.with_connection do
+        ActiveRecord::Base.transaction do
+          # Any read here (validations, callbacks, app code) pins MySQL's REPEATABLE READ snapshot
+          # before the first thread commits.
+          Author.where(list: list).count
+          students[2].update position: 5
+        end
+      end
+    end
+
+    [first_thread, second_thread].each(&:join)
+
+    assert_equal [[1, "New"], [2, "A"], [3, "B"], [4, "D"], [5, "C"], [6, "E"]],
+      list.authors.order(:position).pluck(:position, :name)
+
+    list.destroy
+  end
+
   def test_no_duplicate_row_values_when_creating
     ActiveRecord::Base.connection_handler.clear_all_connections!
 
@@ -686,6 +730,55 @@ def test_that_not_destroyed_via_positioning_scope_closes_gap
   end
 end
 
+class TestRecordScope < Minitest::Test
+  include Minitest::Hooks
+
+  def around
+    ActiveRecord::Base.transaction do
+      super
+      raise ActiveRecord::Rollback
+    end
+  end
+
+  def test_that_a_default_scope_select_does_not_prevent_a_scope_change
+    first_list = List.create name: "First List"
+    second_list = List.create name: "Second List"
+    first_list.default_scope_items.create name: "First Item"
+    second_item = first_list.default_scope_items.create name: "Second Item"
+
+    second_item = DefaultScopeItem.unscoped.find(second_item.id)
+    second_item.update list: second_list
+
+    assert_equal [[second_list.id, 1]], DefaultScopeItem.unscoped.where(id: second_item.id).pluck(:list_id, :position)
+  end
+
+  def test_that_a_record_hidden_by_a_default_scope_can_change_scope
+    list = List.create name: "List"
+    first_item = ArchivableItem.create list: list, name: "First Item"
+    second_item = ArchivableItem.create list: list, name: "Second Item"
+    third_item = ArchivableItem.create list: list, name: "Third Item"
+
+    second_item.update archived_at: Time.current
+    assert_equal [1, 2], [first_item.reload, third_item.reload].map(&:position)
+
+    second_item = ArchivableItem.unscoped.find(second_item.id)
+    second_item.update archived_at: nil
+
+    assert_equal [1, 2, 3], [first_item.reload, third_item.reload, second_item.reload].map(&:position)
+  end
+
+  def test_that_a_primary_key_change_does_not_lose_track_of_the_record
+    list = List.create name: "List"
+    first_item = list.composite_primary_key_items.create item_id: 1, account_id: 1, name: "First Item"
+    second_item = list.composite_primary_key_items.create item_id: 2, account_id: 2, name: "Second Item"
+    third_item = list.composite_primary_key_items.create item_id: 3, account_id: 3, name: "Third Item"
+
+    first_item.update account_id: 4, position: 3
+
+    assert_equal [1, 2, 3], [second_item.reload, third_item.reload, first_item.reload].map(&:position)
+  end
+end
+
 class TestPositioningColumns < Minitest::Test
   include Minitest::Hooks
 
@@ -1376,6 +1469,70 @@ def test_destruction
   end
 end
 
+class TestNullPositions < Minitest::Test
+  include Minitest::Hooks
+
+  def around
+    ActiveRecord::Base.transaction do
+      super
+      raise ActiveRecord::Rollback
+    end
+  end
+
+  def setup
+    @list = List.create name: "List"
+    @other_list = List.create name: "Other List"
+    @legacy_item = @list.new_items.create name: "Legacy Item"
+    @first_item = @list.new_items.create name: "First Item"
+    @second_item = @list.new_items.create name: "Second Item"
+
+    # A row that predates positioning (or hasn't been healed yet)
+    @legacy_item.update_columns position: nil
+    @first_item.update_columns position: 1
+    @second_item.update_columns position: 2
+  end
+
+  def test_that_a_null_position_can_be_given_a_position
+    @legacy_item.update position: 1
+
+    assert_equal [1, 2, 3], [@legacy_item.reload, @first_item.reload, @second_item.reload].map(&:position)
+  end
+
+  def test_that_a_null_position_can_be_moved_to_the_end
+    @legacy_item.update position: :last
+
+    assert_equal [1, 2, 3], [@first_item.reload, @second_item.reload, @legacy_item.reload].map(&:position)
+  end
+
+  def test_that_a_null_position_can_be_positioned_relatively
+    @legacy_item.update position: {after: @first_item}
+
+    assert_equal [1, 2, 3], [@first_item.reload, @legacy_item.reload, @second_item.reload].map(&:position)
+  end
+
+  def test_that_destroying_a_null_position_leaves_the_list_intact
+    @legacy_item.destroy
+
+    assert_equal [1, 2], [@first_item.reload, @second_item.reload].map(&:position)
+  end
+
+  def test_that_moving_a_null_position_to_another_scope_leaves_the_list_intact
+    @legacy_item.update list: @other_list
+
+    assert_equal [1, 2], [@first_item.reload, @second_item.reload].map(&:position)
+    assert_equal 1, @legacy_item.reload.position
+  end
+
+  def test_that_destroying_a_record_whose_row_is_already_gone_does_not_raise
+    item = @list.items.create name: "Item"
+    Item.where(id: item.id).delete_all
+
+    item.destroy
+
+    assert item.destroyed?
+  end
+end
+
 class TestSTIPositioning < Minitest::Test
   include Minitest::Hooks