Repository navigation
Conversation
`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>
When a scope has no belongs_to association, its records are locked with `lock.all.load`, which builds an Active Record object for every row in the scope on every create, update and destroy. With no scope at all that's the whole table. Only the row locks are needed, so select just the primary keys. Order that locking read by primary key. Selecting only the id lets MySQL satisfy it from the position index instead, so a waiting transaction queues inside that index, and InnoDB then blocks our own UPDATEs from writing position values into the gap it's waiting on: a deadlock. Ordering by primary key keeps those waits on the primary key. The new concurrency test deadlocks every time without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When a scope has a belongs_to association we lock the associated record, and when it has none we lock the records in the scope (#43). But when the association exists and is nil, e.g. root nodes of a tree or an optional belongs_to, nothing was locked at all. Concurrent creates of root categories produced duplicate positions on PostgreSQL (the unique index can't catch it because NULL parent_ids never conflict) and frequent deadlocks on MySQL. Fall back to locking the records in the scope, as when there's no association. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Lock ordering's MySQL tests also rely on locking reads.
When a record changes scope we lock its old scope and then its new one. Two transactions moving records between the same two scopes in opposite directions therefore take the same locks in opposite orders, and one of them fails with ActiveRecord::Deadlocked. That happened on PostgreSQL and MySQL alike, both when locking parent records and when locking scope rows. Collect everything to lock first, then lock associated records sorted by table and id, followed by unassociated scopes in a stable order. 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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ope-association 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(This branch merges both, because its MySQL test also needs the locking reads from #72 to get positions right.)
When a record changes scope we lock its old scope, then its new one. Two transactions moving records between the same two scopes in opposite directions therefore take the same locks in opposite orders: a textbook deadlock.
The tests pause each thread after its first lock (via an
sql.active_recordsubscriber) so the interleaving is deterministic. Without this change:Deadlockedon PostgreSQL and MySQL;enabled: trueandfalse(a column scope):Deadlockedon PostgreSQL. MySQL happens to survive that one, because with no index onenabledits locking read locks the whole table anyway.Fix
Collect everything to lock first, then lock associated records sorted by table and id, followed by unassociated scopes in a stable order (by their SQL).
Changelog: Fix deadlocks when records move between the same scopes in opposite directions at the same time.
🤖 Generated with Claude Code