go: sqle: Fix concurrent-clone deadlock between two remotesapi sql-servers - #11281
Merged
reltuk merged 1 commit intoJul 9, 2026
Merged
Conversation
CloneDatabaseFromRemote held the database-provider write lock across the entire clone, including the network fetch of chunks from the remote. When a server serves a --remotesapi-port, that fetch hits a peer's remotesapi whose handler needs a read lock on this same provider (via SessionDatabase). Two sql-servers that CALL DOLT_CLONE from each other's remotesapi at the same time therefore deadlock: each holds its own provider write lock while blocked on the peer, and neither peer can take the read lock needed to answer. DOLT_PULL is immune because it operates on an already-registered database and does not hold the write lock across its fetch. Narrow the lock scope so the fetch does not run under the provider write lock: reserve the database name under a brief lock acquisition (a new creatingDatabases set), release the lock for the network fetch, then re-acquire it only to register the finished database. The reservation keeps concurrent CREATE DATABASE / dolt_clone / undrop of the same name mutually exclusive. A shared databaseNameUnavailableLocked helper performs the availability checks for all creation paths and preserves the interrupted-create marker behavior (ErrIncompleteDatabaseDir). Names in creatingDatabases and deletingDatabases are normalized with formatDbMapKeyName (case-insensitive), matching p.databases. Unlike deletingDatabases, creatingDatabases deliberately does not gate database enumeration: an in-progress clone is simply invisible until it registers. Add a bats regression test (two remotesapi servers cross-cloning concurrently) and Go unit tests for the reservation semantics.
Contributor
|
This is awesome. Thank you for the bug report and thank you for the contribution. I will make a secondary pass on comments and maybe where the testing lands, but in general this approach seems totally reasonable and I had no blocking feedback about the implementation itself. |
Contributor
|
If you're curious, follow up PR is here: #11281. It just slightly reworks some comments and function names and moves the test around. |
Contributor
Author
|
excellent news! I'm using dolt in a local first single master, many follows mesh pattern, and this was quite an issue... now I can get back on official releases 😁 |
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.
DoltDatabaseProvider.CloneDatabaseFromRemoteheld the providerRWMutexwrite lock across the entire clone, including the network fetch of all chunks from the remote. When a server serves a--remotesapi-port, that fetch hits a peer's remotesapi whose handler needs a read lock on this same provider (viaSessionDatabase). Two sql-servers thatCALL DOLT_CLONEfrom each other's remotesapi at the same time therefore deadlock: each holds its own provider write lock while blocked on the peer, and neither peer can take the read lock needed to answer.DOLT_PULLis immune because it operates on an already-registered database and does not hold the write lock across its fetch. This is the lock the existing// TODO: This holds the database provider lock across the entire duration of the clone ...was warning about.The fix narrows the lock scope so the network fetch does not run under the provider write lock:
creatingDatabasesset), release the lock for the fetch, then re-acquire it only to register the finished database (registerNewDatabasestill requires the lock).CREATE DATABASE/dolt_clone/undropof the same name mutually exclusive, so those races behave as before even though the lock is dropped for the fetch.databaseNameUnavailableLockedhelper performs the availability checks for all creation paths and preserves the interrupted-create in-progress marker behaviour (ErrIncompleteDatabaseDir/dbfactory.MarkDatabaseInProgress). The in-memory reservation and the on-disk marker are complementary: the marker guards crash recovery across processes, whilecreatingDatabasesserialises concurrent in-process operations while the lock is dropped.creatingDatabasesanddeletingDatabasesare normalised withformatDbMapKeyName(Dolt database names are case-insensitive), matching howp.databasesis keyed. UnlikedeletingDatabases,creatingDatabasesdeliberately does not gate database enumeration: an in-progress clone is simply invisible until it registers, so enumeration / remotesapi reads never block on it.Tests:
integration-tests/bats/sql-server-remotesrv.bats: a regression test that starts two--remotesapi-portservers cloning from each other concurrently. It hangs (clones time out) on the buggy build and passes on the fixed build; it also fails if a clone errors for any non-deadlock reason.go/libraries/doltcore/sqle/database_provider_test.go:TestCreatingDatabaseReservationcovering the reservation semantics — conflict, case-insensitivity across create/clone/undrop, release, and that a reservation does not blockAllDatabases.Verified with
go test -race ./libraries/doltcore/sqle/and the new bats test;gofmt/goimportsclean.Fixes #11280