Skip to content

Use locking reads for positions once the scope is locked - #72

Open
brendon wants to merge 9 commits into
mainfrom
fix/locking-reads
Open

brendon wants to merge 9 commits into
mainfrom
fix/locking-reads

Conversation

@brendon

@brendon brendon commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Depends on #71 (and so #70), which change the same lines. This branch includes their commits, so review just the last commit, or wait until they're merged and the diff shrinks to this change.

Under MySQL's default REPEATABLE READ isolation, a plain SELECT reads the snapshot taken at the transaction's first read, while UPDATEs always act on the latest rows. If anything read in the transaction before we took the lock, and another transaction moved records while we waited for it, then position_was, last_position and the relative record's position came from stale data while expand/contract operated on current rows.

Any read before the lock is enough: a uniqueness validation, a belongs_to presence check that loads the parent, a callback, or app code in a surrounding transaction.

The new test reads inside the transaction, waits on the lock while another transaction inserts at position 1, then moves a record. On main, MySQL:

ActiveRecord::RecordNotUnique: Mysql2::Error: Duplicate entry '1-1-2' for key 'authors.index_authors_on_list_id_and_enabled_and_position'

and on a table without a unique index it silently produced [1, 2, 2, 4, 5, 6] (a duplicate 2 and a missing 3). PostgreSQL (READ COMMITTED takes a new snapshot per statement) and SQLite were already fine; the test passes on them either way.

Fix

Make those reads locking reads (FOR UPDATE), which always see the latest committed rows. MAX() can't be combined with FOR UPDATE on PostgreSQL, so the maximum is read as the top row instead, with NULLs excluded since PostgreSQL sorts them first when descending. These are rows we already hold the scope lock for and are about to update anyway.

Changelog: Fix stale position reads on MySQL (REPEATABLE READ) after waiting for the scope lock.

🤖 Generated with Claude Code

brendon and others added 3 commits October 16, 2026 12:09
`record_scope` is how Mechanisms reads and moves the record's own row,
but it applied the model's default scope and used the in-memory primary
key:

- A default scope `where` that hides the row (e.g. the archived/soft
  deleted pattern from #59, with the timestamp in the positioning scope)
  meant restoring a record raised NoMethodError on nil.
- A default scope `select` that omits the scope columns raised
  MissingAttributeError on any scope change.
- Changing part of a composite primary key in the same save as the
  position meant the row couldn't be found and update_position raised.

Use `unscoped` and `id_in_database`; this only affects queries for the
record's own row, not the list scope.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`position_was` is memoized with `||=`, which doesn't memoize nil. So for
a row whose position is NULL, `move_out_of_the_way` set it to 0 and the
next call read back 0, and the record was then treated as if it had
been at position 0. The result was a 0 left in the list and every other
record shifted down by one, on update, destroy and scope change. With a
relative position it raised NoMethodError instead.

That affects tables that gained a position column but haven't been
healed yet, and destroying a record whose row is already gone, e.g. a
repeated request (#39, #30), which raised `undefined method '+' for nil`.

Memoize nil properly and treat a NULL position as not being in the list
yet: there's no gap to close when it leaves, and it's inserted (rather
than moved) when it's given a position.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Under MySQL's default REPEATABLE READ isolation, a plain SELECT returns
the snapshot taken at the transaction's first read, but UPDATEs always
see the latest rows. If anything read in the transaction before we took
the lock (a validation, a callback, app code), and another transaction
moved records while we waited for the lock, `position_was`, the maximum
and the relative record's position were computed from stale data while
expand/contract operated on current rows. The result was a unique
violation or, without a unique index, duplicate and missing positions.

Make those reads locking reads, which always see the latest committed
rows. `MAX()` can't be combined with `FOR UPDATE` on PostgreSQL, so the
maximum is read as the top row instead. PostgreSQL (READ COMMITTED) and
SQLite are unaffected either way.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
brendon and others added 6 commits October 19, 2026 11:21
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant