Repository navigation
fix(ontology): retry an edit-lock acquire that loses a MySQL deadlock; de-flake TaskResourceIT and TestCaseResourceIT - #35507
Conversation
OntologyEditLockService.acquire reads the lease row with SELECT ... FOR UPDATE and inserts it when absent, in one transaction. On MySQL a locking read of an absent key takes a gap lock, so two first-time acquires deadlock on their inserts even for different resources, and one returns 500. Wrap the transaction in DeadlockRetry: the rolled-back acquire replays and succeeds, or sees the winner's row and gets the usual 409. Also scope TaskResourceIT's time-range assertions to the test's own domain: paging.total vs page size over everything visible to the shared USER1 moved whenever a concurrent test created a task for USER1 between the two queries.
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
backend
safe to test
test_bulkAddAllTestCasesToLogicalTestSuite and test_bulkAddAllTestCasesWithExcludeIds add every test case in the shared database to a suite, rewriting each one's search doc while the parallel lane runs. That write lands inside other tests' async search updates to their own test cases, e.g. TableCertificationPropagationIT's cert cascade, which is then skipped on the version conflict (ejected #35321). Hold the global lock for just these two methods, as ContainerResourceIT already does; the rest of the class stays concurrent.
sonika-shah
changed the title
testListFluentAPI, testAutoPaginationFluentAPI, test_listTestCasesWithPagination and test_listTestCasesFromSearch listed every test case in the shared database. Their size() >= N checks passed on other tests' rows, so they never checked their own, and they could only fail when another test's row broke the list (e.g. its table hard-deleted mid-call). Filter by the test's own entityLink, as test_listTestCases_200_OK already does, and assert the created ids come back; the pagination test now walks every page and checks each id once.
Code Review ✅ Approved🟡 Medium risk · Retrying deadlocked ontology edit-lock acquisitions could delay requests during contention; remaining changes only scope and isolate integration tests. Wraps the ontology edit-lock acquire transaction in VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
3 participants
Summary
Two merge-queue IT flakes, fixed at the root cause:
OntologyEditLockService.acquiredoesSELECT … FOR UPDATEand thenINSERTwhen no lease exists, in one transaction. On MySQL a locking read of an absent key takes a gap lock, so two first-time acquires deadlock on their inserts, even for different resources, and one user gets a 500. The fix wraps the transaction in the existingDeadlockRetry(same pattern asConversationRepository). The rolled-back acquire replays and either succeeds, or finds the winner's row and gets the normal 409.TaskResourceIT.testScopedTaskEndpointsHonorTimeRange(test bug). It assertedpaging.total == data.size()over every task visible to the sharedUSER1, with a window ending in the future. A concurrent test creating a task forUSER1between the count and list queries made them disagree. The listers are now scoped to the test's own domain, which every task endpoint already filters on.TestCaseResourceIT's add-every-test-case tests run alone (test isolation).test_bulkAddAllTestCasesToLogicalTestSuiteandtest_bulkAddAllTestCasesWithExcludeIdsadd every test case in the shared DB to a suite, rewriting each one's search doc while the parallel lane runs. That write lands inside other tests' async search updates to their own test cases.TableCertificationPropagationIT's cert cascade is one, and it is skipped on the version conflict (ejected Fixes #35317: check hybrid search with a query, not the stored pipeline #35321). These two methods now hold@ResourceLock(Resources.GLOBAL, READ_WRITE), the same asContainerResourceIT; the rest of the class stays concurrent. The product-side race (async update-by-query withconflicts=proceed, never retried) is still tracked in the issue.TestCaseResourceIT's list tests are scoped to their own table.testListFluentAPI,testAutoPaginationFluentAPI,test_listTestCasesWithPaginationandtest_listTestCasesFromSearchlisted every test case in the shared DB. Theirsize() >= Nchecks passed on other tests' rows, and they could only fail when another test's row broke the list call. They now filter by their ownentityLink(astest_listTestCases_200_OKdoes) and assert their own ids. The pagination test walks every page and checks each id appears once.testFluentListis unchanged: the fluentTestCases.list()has no filter option.Merge-queue ejections these caused: #33908 (
OntologyStructureITdeadlock), #32038 (TaskResourceIT) and #35321 (TableCertificationPropagationIT). Context and the other flakes: #35508 and #35505.Tests
OntologyEditLockResourceIT.concurrentFirstAcquiresOnDifferentResourcesAllSucceed(8 concurrent first acquires on different terms), MySQL + ESDeadlock on OntologyEditLockDAO.insert, 500sDeadlockRetrylogs show the deadlocks absorbedTaskResourceIT#testScopedTaskEndpointsHonorTimeRange, MySQL + ES and PostgreSQL + OpenSearchTestCaseResourceIT#test_bulkAddAll*+#test_bulkAddTestCasesToLogicalTestSuiteByIdswithTableCertificationPropagationIT, PostgreSQL + OpenSearch paralleltestFluentList+ the 3 bulk-add tests, PostgreSQL + OpenSearch parallelDetails
FOR UPDATEfinds the row and takes a record lock only. Only first acquires collide, and on a mostly-empty table almost any two overlapping ones share a gap.FOR UPDATEon a missing row, so this deadlock is MySQL-only. A same-resource race on Postgres still surfaces as a duplicate-key error rather than a 409; that's pre-existing and out of scope here.DeadlockRetryretries only deadlock and lock-wait errors (MySQL 1213/1205, SQLSTATE 40001/40P01), so the policy's 409 for an active foreign lease is never retried.FOR UPDATEinRdfInfraDAOsand theontology/package, so no sibling has the same shape.