fix(sqlserver): make EnsureDatabaseExistsAsync safe under concurrent callers (weasel#415)

src/Weasel.EntityFrameworkCore.Tests/SqlServer/SqlServerDatabaseBootstrap.cs

@@ -1,4 +1,5 @@
 using Microsoft.Data.SqlClient;
+using Weasel.SqlServer;
 
 namespace Weasel.EntityFrameworkCore.Tests.SqlServer;
 
@@ -9,41 +10,16 @@ namespace Weasel.EntityFrameworkCore.Tests.SqlServer;
 ///     logins — so creation swallows the "already exists" race and login is
 ///     retried until the database is actually reachable.
 /// </summary>
+/// <remarks>
+///     This used to carry its own copy of that logic. It now lives in
+///     <see cref="SqlServerMigrator.EnsureDatabaseExistsAsync" /> (weasel#415), so this is a thin
+///     shim kept for the call sites.
+/// </remarks>
 public static class SqlServerDatabaseBootstrap
 {
     public static async Task EnsureDatabaseExistsAsync(string connectionString)
     {
-        var builder = new SqlConnectionStringBuilder(connectionString);
-        var database = builder.InitialCatalog;
-        builder.InitialCatalog = "master";
-
-        await using (var master = new SqlConnection(builder.ConnectionString))
-        {
-            await master.OpenAsync();
-            await using var cmd = master.CreateCommand();
-            cmd.CommandText = $"IF DB_ID('{database}') IS NULL CREATE DATABASE [{database}]";
-            try
-            {
-                await cmd.ExecuteNonQueryAsync();
-            }
-            // 1801: database already exists — a concurrent test won the race
-            catch (SqlException e) when (e.Number == 1801)
-            {
-            }
-        }
-
-        for (var attempt = 0; ; attempt++)
-        {
-            try
-            {
-                await using var conn = new SqlConnection(connectionString);
-                await conn.OpenAsync();
-                return;
-            }
-            catch (SqlException) when (attempt < 30)
-            {
-                await Task.Delay(1000);
-            }
-        }
+        await using var connection = new SqlConnection(connectionString);
+        await new SqlServerMigrator().EnsureDatabaseExistsAsync(connection);
     }
 }

src/Weasel.Postgresql/PostgresqlMigrator.cs

@@ -370,7 +370,17 @@ public override async Task EnsureDatabaseExistsAsync(DbConnection connection, Ca
 
         if (!await adminConn.DatabaseExists(databaseName, ct).ConfigureAwait(false))
         {
-            await new DatabaseSpecification().BuildDatabase(adminConn, databaseName, ct).ConfigureAwait(false);
+            try
+            {
+                await new DatabaseSpecification().BuildDatabase(adminConn, databaseName, ct).ConfigureAwait(false);
+            }
+            catch (PostgresException e) when (e.SqlState == PostgresErrorCodes.DuplicateDatabase)
+            {
+                // A concurrent caller created it between the existence check and this statement
+                // (weasel#415). That is the outcome we wanted anyway. Unlike SQL Server, PostgreSQL
+                // accepts connections to the new database as soon as CREATE DATABASE returns, so
+                // there is nothing further to wait for.
+            }
         }
     }
 

src/Weasel.SqlServer.Tests/SqlServerMigratorTests.cs

@@ -1,3 +1,4 @@
+using JasperFx.Core;
 using Microsoft.Data.SqlClient;
 using Shouldly;
 using Weasel.SqlServer.Tables;
@@ -83,4 +84,150 @@ public async Task ensure_database_is_idempotent()
         await using var connection = new SqlConnection(builder.ConnectionString);
         await migrator.EnsureDatabaseExistsAsync(connection);
     }
+
+    /// <summary>
+    ///     weasel#415. Several callers provisioning the same database at once used to leave the losers of
+    ///     the check-then-create race holding SqlException 1801. Against 9.22.0 this fails.
+    /// </summary>
+    [Fact]
+    public async Task ensure_database_is_safe_under_concurrent_callers()
+    {
+        var databaseName = $"weasel_ensure_race_{Guid.NewGuid():N}";
+
+        var builder = new SqlConnectionStringBuilder(ConnectionSource.ConnectionString)
+        {
+            InitialCatalog = databaseName
+        };
+
+        try
+        {
+            // All of these see DB_ID() return null and race into CREATE DATABASE together.
+            var attempts = Enumerable.Range(0, 8).Select(async _ =>
+            {
+                await using var conn = new SqlConnection(builder.ConnectionString);
+                await new SqlServerMigrator().EnsureDatabaseExistsAsync(conn);
+            });
+
+            await Task.WhenAll(attempts);
+
+            // Every caller must be able to take the postcondition at face value: the database exists
+            // and accepts a connection by the time EnsureDatabaseExistsAsync returns.
+            await using var verifyConn = new SqlConnection(builder.ConnectionString);
+            await verifyConn.OpenAsync();
+        }
+        finally
+        {
+            await dropDatabaseAsync(databaseName);
+        }
+    }
+
+    /// <summary>
+    ///     A database name carrying a ']' would otherwise close the delimited identifier early.
+    /// </summary>
+    [Fact]
+    public async Task ensure_database_escapes_a_bracket_in_the_database_name()
+    {
+        var databaseName = $"weasel_ensure_]_{Guid.NewGuid():N}";
+
+        var builder = new SqlConnectionStringBuilder(ConnectionSource.ConnectionString)
+        {
+            InitialCatalog = databaseName
+        };
+
+        try
+        {
+            await using var conn = new SqlConnection(builder.ConnectionString);
+            await new SqlServerMigrator().EnsureDatabaseExistsAsync(conn);
+
+            await using var verifyConn = new SqlConnection(builder.ConnectionString);
+            await verifyConn.OpenAsync();
+        }
+        finally
+        {
+            await dropDatabaseAsync(databaseName);
+        }
+    }
+
+    /// <summary>
+    ///     The wait for the database to come online has to end in a clear failure rather than in silence
+    ///     or a hang. An offline database is the reproducible stand-in for "created, but still refusing
+    ///     logins": DB_ID() reports it, so nothing is created, and no connection to it will ever succeed.
+    /// </summary>
+    [Fact]
+    public async Task times_out_with_a_clear_message_when_the_database_never_accepts_connections()
+    {
+        var databaseName = $"weasel_ensure_offline_{Guid.NewGuid():N}";
+
+        var builder = new SqlConnectionStringBuilder(ConnectionSource.ConnectionString)
+        {
+            InitialCatalog = databaseName
+        };
+
+        try
+        {
+            await using (var conn = new SqlConnection(builder.ConnectionString))
+            {
+                await new SqlServerMigrator().EnsureDatabaseExistsAsync(conn);
+            }
+
+            await executeAgainstMasterAsync(
+                $"ALTER DATABASE [{databaseName}] SET OFFLINE WITH ROLLBACK IMMEDIATE;");
+
+            var migrator = new SqlServerMigrator
+            {
+                DatabaseAvailabilityTimeout = 1.Seconds(), DatabaseAvailabilityPollingInterval = 100.Milliseconds()
+            };
+
+            await using var offlineConn = new SqlConnection(builder.ConnectionString);
+
+            var ex = await Should.ThrowAsync<TimeoutException>(async () =>
+                await migrator.EnsureDatabaseExistsAsync(offlineConn));
+
+            ex.Message.ShouldContain(databaseName);
+            ex.Message.ShouldContain(nameof(SqlServerMigrator.DatabaseAvailabilityTimeout));
+            ex.InnerException.ShouldBeOfType<SqlException>();
+        }
+        finally
+        {
+            await executeAgainstMasterAsync($"ALTER DATABASE [{databaseName}] SET ONLINE;");
+            await dropDatabaseAsync(databaseName);
+        }
+    }
+
+    private static async Task executeAgainstMasterAsync(string sql)
+    {
+        var adminBuilder = new SqlConnectionStringBuilder(ConnectionSource.ConnectionString)
+        {
+            InitialCatalog = "master"
+        };
+        await using var adminConn = new SqlConnection(adminBuilder.ConnectionString);
+        await adminConn.OpenAsync();
+
+        var cmd = adminConn.CreateCommand();
+        cmd.CommandText = sql;
+        await cmd.ExecuteNonQueryAsync();
+    }
+
+    private static async Task dropDatabaseAsync(string databaseName)
+    {
+        var adminBuilder = new SqlConnectionStringBuilder(ConnectionSource.ConnectionString)
+        {
+            InitialCatalog = "master"
+        };
+        await using var adminConn = new SqlConnection(adminBuilder.ConnectionString);
+        await adminConn.OpenAsync();
+
+        var cmd = adminConn.CreateCommand();
+        cmd.CommandText = $@"
+            IF DB_ID(@name) IS NOT NULL
+            BEGIN
+                ALTER DATABASE [{databaseName.Replace("]", "]]")}] SET SINGLE_USER WITH ROLLBACK IMMEDIATE;
+                DROP DATABASE [{databaseName.Replace("]", "]]")}];
+            END";
+        var param = cmd.CreateParameter();
+        param.ParameterName = "@name";
+        param.Value = databaseName;
+        cmd.Parameters.Add(param);
+        await cmd.ExecuteNonQueryAsync();
+    }
 }

src/Weasel.SqlServer/SqlServerMigrator.cs

@@ -192,6 +192,42 @@ FROM    sys.schemas
 ";
     }
 
+    /// <summary>
+    ///     SQL Server error 1801, "Database 'x' already exists". Raised by <c>CREATE DATABASE</c> when a
+    ///     concurrent caller created the database between our existence check and our create (weasel#415).
+    /// </summary>
+    private const int DatabaseAlreadyExists = 1801;
+
+    /// <summary>
+    ///     How long <see cref="EnsureDatabaseExistsAsync" /> will keep retrying a connection to the target
+    ///     database before giving up. A freshly created SQL Server database can briefly refuse logins, and
+    ///     on a cold container that window can run to tens of seconds -- so the method does not return until
+    ///     the database actually accepts a connection. Set to <see cref="TimeSpan.Zero" /> to make a single
+    ///     attempt and fail fast, which is usually what you want against a warm local server.
+    /// </summary>
+    public TimeSpan DatabaseAvailabilityTimeout { get; set; } = 30.Seconds();
+
+    /// <summary>
+    ///     How long <see cref="EnsureDatabaseExistsAsync" /> waits between connection attempts while the
+    ///     newly created database is still refusing logins.
+    /// </summary>
+    public TimeSpan DatabaseAvailabilityPollingInterval { get; set; } = 1.Seconds();
+
+    /// <summary>
+    ///     Creates the database named by the connection's <c>Initial Catalog</c> if it does not already
+    ///     exist, then blocks until that database accepts a connection.
+    /// </summary>
+    /// <remarks>
+    ///     Safe to call from several processes at once (weasel#415). The existence check and the
+    ///     <c>CREATE DATABASE</c> are not atomic -- SQL Server offers no form that makes them so -- so the
+    ///     loser of the race is recognised by error 1801 and treated as success. Waiting for the database to
+    ///     accept a connection is done unconditionally rather than only after we create it, because a
+    ///     concurrent creator leaves the same window open for us.
+    /// </remarks>
+    /// <exception cref="ArgumentException">The connection string does not name a database.</exception>
+    /// <exception cref="TimeoutException">
+    ///     The database exists but did not accept a connection within <see cref="DatabaseAvailabilityTimeout" />.
+    /// </exception>
     public override async Task EnsureDatabaseExistsAsync(DbConnection connection, CancellationToken ct = default)
     {
         var builder = new SqlConnectionStringBuilder(connection.ConnectionString);
@@ -202,24 +238,76 @@ public override async Task EnsureDatabaseExistsAsync(DbConnection connection, Ca
             throw new ArgumentException("The connection string does not specify a database name (Initial Catalog).");
         }
 
+        var targetConnectionString = builder.ConnectionString;
+
         builder.InitialCatalog = "master";
-        await using var adminConn = new SqlConnection(builder.ConnectionString);
-        await adminConn.OpenAsync(ct).ConfigureAwait(false);
+        await using (var adminConn = new SqlConnection(builder.ConnectionString))
+        {
+            await adminConn.OpenAsync(ct).ConfigureAwait(false);
 
-        var cmd = adminConn.CreateCommand();
-        cmd.CommandText = "SELECT DB_ID(@name)";
-        var param = cmd.CreateParameter();
-        param.ParameterName = "@name";
-        param.Value = databaseName;
-        cmd.Parameters.Add(param);
+            var cmd = adminConn.CreateCommand();
+            cmd.CommandText = "SELECT DB_ID(@name)";
+            var param = cmd.CreateParameter();
+            param.ParameterName = "@name";
+            param.Value = databaseName;
+            cmd.Parameters.Add(param);
 
-        var result = await cmd.ExecuteScalarAsync(ct).ConfigureAwait(false);
+            var result = await cmd.ExecuteScalarAsync(ct).ConfigureAwait(false);
 
-        if (result is null or DBNull)
+            if (result is null or DBNull)
+            {
+                var createCmd = adminConn.CreateCommand();
+
+                // CREATE DATABASE takes no parameters, so the name has to be interpolated. Doubling ']'
+                // is what makes it a well-formed delimited identifier.
+                createCmd.CommandText = $"CREATE DATABASE [{databaseName.Replace("]", "]]")}]";
+
+                try
+                {
+                    await createCmd.ExecuteNonQueryAsync(ct).ConfigureAwait(false);
+                }
+                catch (SqlException e) when (e.Number == DatabaseAlreadyExists)
+                {
+                    // Another caller created it between our DB_ID check and this statement. That is the
+                    // outcome we wanted anyway.
+                }
+            }
+        }
+
+        await waitUntilDatabaseAcceptsConnectionsAsync(targetConnectionString, databaseName, ct)
+            .ConfigureAwait(false);
+    }
+
+    private async Task waitUntilDatabaseAcceptsConnectionsAsync(
+        string connectionString,
+        string databaseName,
+        CancellationToken ct
+    )
+    {
+        var timeout = DatabaseAvailabilityTimeout;
+        var deadline = DateTimeOffset.UtcNow + timeout;
+
+        while (true)
         {
-            var createCmd = adminConn.CreateCommand();
-            createCmd.CommandText = $"CREATE DATABASE [{databaseName}]";
-            await createCmd.ExecuteNonQueryAsync(ct).ConfigureAwait(false);
+            ct.ThrowIfCancellationRequested();
+
+            try
+            {
+                await using var conn = new SqlConnection(connectionString);
+                await conn.OpenAsync(ct).ConfigureAwait(false);
+                return;
+            }
+            catch (SqlException e)
+            {
+                if (DateTimeOffset.UtcNow >= deadline)
+                {
+                    throw new TimeoutException(
+                        $"Database '{databaseName}' exists, but did not accept a connection within {timeout}. See {nameof(SqlServerMigrator)}.{nameof(DatabaseAvailabilityTimeout)} if the database needs longer to come online.",
+                        e);
+                }
+
+                await Task.Delay(DatabaseAvailabilityPollingInterval, ct).ConfigureAwait(false);
+            }
         }
     }