Repository navigation
fix: retry transient InnoDB deadlocks during option-store lock acquisition - #580
Merged
Merged
Conversation
…ition Two connections racing an INSERT IGNORE (or a conditional takeover UPDATE) against the same unique lock row can make InnoDB report a deadlock (1213) or lock-wait timeout instead of the clean "0 rows affected" outcome the retry loop already handles. acquire_lock() treated any query failure as fatal, so a purely transient storage- engine artifact of the exact admission race it serializes surfaced as an immediate WP_Agent_Run_Control_Store_Exception instead of a retry. Reproduced locally against a live, contended MariaDB instance: the existing 'site option mutex serializes two independent connection mutations' and 'enqueue concurrent with claim succeeds' assertions in tests/run-control-option-store-integration.php failed in roughly 70-80% of runs before this change (confirmed via last_error: 'Deadlock found when trying to get lock; try restarting transaction') and passed 50/50 after it. Fixes #576.
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.
Fixes #576
Root cause: real store bug, not a test-timing artifact
Diagnosis required actual reproduction, not code-reading alone. I ran
tests/run-control-option-store-integration.phpagainst a live, heavily-contended local MariaDB instance (a shared production database, not a clean CI container) and the failure reproduced in roughly 70-80% of runs — far above the "1-in-30" rate reported in the issue, but the same failure mode.Instrumenting
acquire_lock()to log the underlying error onfalse === $createdshowed:Two connections racing
INSERT IGNOREagainst the same uniqueoption_namelock row can make InnoDB report a deadlock (1213) instead of the clean "0 rows affected, key already exists" outcome the retry loop is written to expect.acquire_lock()treated any query failure on that statement as fatal and threwWP_Agent_Run_Control_Store_Exceptionimmediately, with no retry — even though a deadlock here is exactly the same admission race the surroundingdo {} whileloop already retries for the duplicate-key case. The same failure class applies to the conditional takeoverUPDATEused for expired-lock recovery.This directly explains both failing assertions:
site option mutex serializes two independent connection mutations— one of the two concurrentmutate_state()calls threw on the deadlock, so only one increment ever landed (final counter = 1, not 2).enqueue concurrent with claim succeeds(the assertion named in Flaky: run-control-option-store-integration 'enqueue concurrent with claim succeeds' #576) —queue_message()catches\RuntimeExceptionand converts it to aWP_Error(agents_chat_run_atomic_unavailable), so the same deadlock surfaces asqueued => false.This is a genuine correctness/robustness gap in the store's own lock acquisition, not a test synchronization problem — the existing fork/ready/go barrier already guarantees real overlap, and the two-worker mutex model is architecturally sound. The bug is that the retry loop didn't recognize a transient storage-engine outcome of its own contention.
Fix
acquire_lock()now treats anINSERT IGNORE(or takeoverUPDATE) failure as retryable when$db->last_errorindicates a transient InnoDB deadlock or lock-wait timeout, reusing the loop's existing retry/backoff primitive (usleep(50000)+ the existing 5s deadline) instead of failing closed immediately. Non-transient failures (real connectivity/schema errors) still throw immediately, unchanged.The test file is unchanged and now serves as the regression coverage.
Verification
site option mutex...andenqueue concurrent with claim succeeds), consistently witherrno=1213on the lock-creationINSERT IGNORE.php tests/run-control-option-store-integration.phpagainst the same contended local MariaDB (run in a background loop; each run includes a ~16.5s intentional sleep in the lease-race test, so 50 runs took roughly 14 minutes).composer smoke— pass (exit code 0).composer phpstan—[OK] No errors.AI disclosure
AI assistance: Yes
Tool: OpenCode
Used for: diagnosis (reproduction, instrumentation, root-cause isolation of the InnoDB deadlock), fix implementation, and test verification (50-run loop, smoke, phpstan).