fix: lock the actor instance by primary key - #53
Merged
Merged
Conversation
Concurrent callers that created the same actor from inside their own transaction deadlocked on MySQL. The portable conflict clause becomes INSERT IGNORE, which leaves a shared lock on the identity index when the row already exists. The mailbox then read the same row FOR UPDATE, so two callers each held shared and each waited for exclusive on one index record. Six of eight concurrent callers failed with ER_LOCK_DEADLOCK. Top level enqueue hid this, because it retries ER_LOCK_DEADLOCK eight times. Nested callers get no retry, and the effect executor, the reminder scheduler, the retry path, and effect recovery all enqueue inside a transaction that already wrote. Read the instance without a lock first, on every database, then lock it by primary key. After an ignored insert on MySQL, read the winning row in shared mode and select only its id, so the read stays inside the identity index and never locks the clustered record. A shared read that selects every column locks the clustered record too, which only moves the same upgrade deadlock onto the primary key. PostgreSQL and SQLite take no shared lock. A share lock on PostgreSQL creates the upgrade deadlock it prevents on MySQL, and its regression test fails when that lock is applied there. Validate with the default suite, the MySQL suite, the PostgreSQL suite, format:check, check, build, test:package, and test:recovery.
|
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.
Why
Concurrent callers that create the same actor from inside their own transaction
deadlock on MySQL.
mysqlSqlrewrites the portableON CONFLICT (...) DO NOTHINGintoINSERT IGNORE(src/database/mysql.ts:237). When the row already exists,INSERT IGNOREleaves a shared lock on the identity index.enqueueInTransactionthen read that same rowFOR UPDATE, so each caller heldshared and each waited for exclusive on one index record.
Six of eight concurrent callers failed with
ER_LOCK_DEADLOCK.Why it was not visible
enqueueretriesER_LOCK_DEADLOCKup to eight times (repository.ts:280),so the top level path absorbs it. That retry does not wrap
enqueueInTransaction, and every nested caller enqueues inside a transactionthat already wrote: the effect executor (
:804), the retry path (:1137),effect recovery (
:1523,:1571), reminders (:1762), and the recoverycoordinator (
:2025). A deadlock there aborts the caller's whole transaction.The top level path deadlocks too. It only looks healthy because it pays a
deadlock and a backoff on every cold start burst.
What changed
primary key rather than by actor type and actor id.
select only its id.
Selecting only the id matters. A shared read that selects every column also
locks the clustered record, which does not remove the upgrade deadlock, it
moves it onto the primary key:
An index-only read stays inside the identity index and never touches the
clustered record, so the later primary-key lock has nothing to upgrade.
PostgreSQL and SQLite take no shared lock. PostgreSQL defaults to read
committed, so a plain read already sees the winning row, and a share lock there
creates the upgrade deadlock it prevents on MySQL.
Tests
Both are new and follow the failing-test-first order.
test/mysql.test.tsdrives eight concurrent nested callers at one new actor.It fails on
mainwith sixER_LOCK_DEADLOCKresults and passes here, fourruns out of four.
test/postgresql.test.tscarries the same race. PostgreSQL never had thisbug, so that test guards the fix rather than the original defect: it fails
when the MySQL shared read is applied to PostgreSQL as well, which is how the
adapter split was chosen rather than assumed.
Both assert one instance row and sequences 1 through 8.
Validation
format:checkcheckbuildtest:packagetest:recoveryCompatibility
No migration and no API change.
FOR SHAREneeds MySQL 8.0, which is alreadythe tested minimum in
src/doctor.tsand both CI versions. The removedlockoption on the private
findInstancehad no remaining caller.Release
This branch also cuts 0.15.2, matching solid_objects 0.15.2 for Ruby, which
carries the same fix.