fix(secrets-sqlserver): retry the virgin-DB concurrent migrator race; release family 0.5.1

The ScadaBridge central shared-store live gate proved SqlServerSecretsStoreMigrator's
"safe to run concurrently" claim false on a virgin database: Serializable isolation does
not serialize the IF NOT EXISTS metadata probes against another session's in-flight
CREATE, so of two central nodes booting together the loser died with SqlException 2714
("There is already an object named 'zbsecrets'") — the retry filter covered only
deadlock 1205. Losing such a race means the object now exists (the desired end state),
so 2714 (schema/table), 1913 (index) and 2627 (schema_version MERGE-insert PK) are now
retryable alongside 1205; the retry re-runs the idempotent IF NOT EXISTS path.
Classification scans the whole SqlException.Errors collection (2714 arrives stacked with
2759), the retry cap keeps genuine conflicts failing closed, and 2705 is deliberately
excluded (unreachable — columns live in a single CREATE TABLE statement). Predicate
extracted as an internal static pinned by offline tests; live suite gains an env-gated
4-concurrent-migrators virgin-database test. Six packages pushed to the Gitea feed at
0.5.1.

Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1

ZB.MOM.WW.Secrets/CHANGELOG.md

@@ -5,6 +5,31 @@ follows [Keep a Changelog](https://keepachangelog.com/); versions are shared acr
 package in the family (see `Directory.Build.props`) even when a given release only touches
 one of them.
 
+## 0.5.1 — 2026-08-07
+
+### Fixed
+
+- **`ZB.MOM.WW.Secrets.Replicator.SqlServer` — virgin-database concurrent migrator race**
+  (present since 0.2.0; found by the ScadaBridge central shared-store live gate). The
+  migrator's doc claimed simultaneous provisioning of a virgin database converges, but its
+  retry filter covered only deadlock victims (1205): Serializable isolation does not
+  serialize the `IF NOT EXISTS` metadata probes against another session's in-flight
+  `CREATE`, so of two nodes booting together against an empty database the loser died with
+  `SqlException` 2714 ("There is already an object named 'zbsecrets'") and aborted its
+  host — observed live on an ordinary `docker compose up` of the ScadaBridge central pair.
+  The retry filter now also treats the lost-CREATE/lost-INSERT race outcomes as retryable —
+  2714 (schema/table already exists), 1913 (index already exists), 2627 (schema_version
+  MERGE-insert PK violation) — since losing such a race means the object now exists, which
+  is the desired end state; the retry re-runs the idempotent `IF NOT EXISTS` path and
+  proceeds. Classification scans the whole `SqlException.Errors` collection (a failed DDL
+  batch stacks follow-on errors such as 2759, so the retryable number is not guaranteed
+  first), and the retry cap (5) keeps a genuine conflict — e.g. an unrelated pre-existing
+  object colliding with the schema name — failing closed. 2705 (duplicate column) is
+  deliberately not retried: every column lives inside a single `CREATE TABLE` statement,
+  so no cross-session race can produce it. The error-number predicate is now an internal
+  static pinned by offline tests, and the live suite gains an N-concurrent-migrators
+  virgin-database test.
+
 ## 0.5.0 — 2026-08-07
 
 ### Added

ZB.MOM.WW.Secrets/Directory.Build.props

@@ -5,7 +5,7 @@
     <Nullable>enable</Nullable>
     <ImplicitUsings>enable</ImplicitUsings>
     <LangVersion>latest</LangVersion>
-    <Version>0.5.0</Version>
+    <Version>0.5.1</Version>
     <ManagePackageVersionsCentrally>true</ManagePackageVersionsCentrally>
     <PackageReadmeFile>README.md</PackageReadmeFile>
   </PropertyGroup>

ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretsStoreMigrator.cs

@@ -7,21 +7,46 @@ namespace ZB.MOM.WW.Secrets.Replicator.SqlServer;
 
 /// <summary>
 /// Creates the shared SQL-Server secret schema and records the applied version. Idempotent: safe to
-/// run on every node at every startup, including concurrently. Refuses to run against a database
+/// run on every node at every startup, including concurrently — lost provisioning races surface as
-/// whose recorded version is newer than this build supports.
+/// retryable <see cref="SqlException"/>s (see <see cref="IsRetryableMigrationErrorNumber"/>) and the
+/// retry re-runs the <c>IF NOT EXISTS</c> path against the objects the winner just created. Refuses
+/// to run against a database whose recorded version is newer than this build supports.
 /// </summary>
 /// <param name="connectionFactory">Factory for connections to the shared secret database.</param>
 public sealed class SqlServerSecretsStoreMigrator(SecretsSqlServerConnectionFactory connectionFactory)
     : ISecretsStoreMigrator
 {
-    // Concurrent Serializable DDL against the system catalogs can pick a deadlock victim (error
+    // Every node runs this at every startup, so a virgin database being provisioned by N nodes at
-    // 1205) even though the IF NOT EXISTS guards make the logic itself safe. Every node runs this at
+    // once is the NORMAL case, not a rare race — and an unretried failure aborts that node's whole
-    // every startup, so a virgin database being provisioned by N nodes at once is the NORMAL case,
+    // host at startup. Two failure shapes come out of that race, and retrying is correct for both
-    // not a rare race — and an unretried victim aborts that node's whole host at startup. Retrying
+    // precisely because the migration is idempotent:
-    // is correct precisely because the migration is idempotent.
+    //
-    private const int MaxDeadlockRetries = 5;
+    //   - Deadlock victim (1205): concurrent Serializable DDL against the system catalogs can pick
+    //     a victim even though the IF NOT EXISTS guards make the logic itself safe.
+    //   - Lost CREATE / lost INSERT (2714, 1913, 2627): Serializable range locks do NOT serialize
+    //     the metadata existence checks against another session's in-flight CREATE — observed live
+    //     (ScadaBridge central-pair first boot, 2026-08-07): both nodes passed the sys.schemas
+    //     IF NOT EXISTS probe, the loser's CREATE SCHEMA then died with 2714 "There is already an
+    //     object named 'zbsecrets'". The object the loser tried to create now exists, which is the
+    //     desired end state — the retry's IF NOT EXISTS pass skips it and proceeds.
+    //
+    // The retry cap keeps this from masking a GENUINE conflict (e.g. an unrelated pre-existing
+    // object colliding with the schema name): that reproduces on every attempt, so after
+    // MaxMigrationRetries the real SqlException propagates and the node fails closed.
+    private const int MaxMigrationRetries = 5;
+
+    /// <summary>Deadlock victim.</summary>
     private const int SqlDeadlockVictimErrorNumber = 1205;
 
+    /// <summary>"There is already an object named '%ls'" — lost CREATE SCHEMA / CREATE TABLE race.</summary>
+    private const int SqlObjectAlreadyExistsErrorNumber = 2714;
+
+    /// <summary>"An index or statistics with name '%ls' already exists" — lost CREATE INDEX race.</summary>
+    private const int SqlIndexAlreadyExistsErrorNumber = 1913;
+
+    /// <summary>Primary-key violation — lost version-row MERGE-insert race.</summary>
+    private const int SqlPrimaryKeyViolationErrorNumber = 2627;
+
     /// <inheritdoc />
     /// <exception cref="SecretStoreMigrationException">
     /// The recorded schema version is newer than <see cref="SqlServerSecretsSchema.CurrentVersion"/>.
@@ -36,15 +61,53 @@ public sealed class SqlServerSecretsStoreMigrator(SecretsSqlServerConnectionFact
                 return;
             }
             catch (SqlException ex)
-                when (ex.Number == SqlDeadlockVictimErrorNumber && attempt < MaxDeadlockRetries)
+                when (IsRetryableMigrationError(ex) && attempt < MaxMigrationRetries)
             {
-                // Staggered back-off so two victims do not retry in lockstep and deadlock again.
+                // Staggered back-off so two victims do not retry in lockstep and race again.
                 await Task.Delay(TimeSpan.FromMilliseconds(100 * attempt), cancellationToken)
                     .ConfigureAwait(false);
             }
         }
     }
 
+    /// <summary>
+    /// Whether any error carried by <paramref name="exception"/> is a retryable provisioning-race
+    /// outcome. Scans the whole <see cref="SqlException.Errors"/> collection rather than only
+    /// <see cref="SqlException.Number"/> because a failed DDL batch stacks follow-on errors (e.g.
+    /// 2714 arrives with 2759 "CREATE SCHEMA failed due to previous errors") and the retryable one
+    /// is not guaranteed to be first.
+    /// </summary>
+    /// <param name="exception">The exception the migration attempt failed with.</param>
+    /// <returns><see langword="true"/> when a retry may succeed.</returns>
+    internal static bool IsRetryableMigrationError(SqlException exception)
+    {
+        foreach (SqlError error in exception.Errors)
+        {
+            if (IsRetryableMigrationErrorNumber(error.Number))
+            {
+                return true;
+            }
+        }
+
+        return IsRetryableMigrationErrorNumber(exception.Number);
+    }
+
+    /// <summary>
+    /// Classifies a SQL-Server error number as a retryable outcome of N nodes provisioning the same
+    /// (possibly virgin) database at once. Everything here is a race whose retry re-runs the
+    /// idempotent <c>IF NOT EXISTS</c> path; anything else (login failures, missing database,
+    /// permissions, syntax) reproduces identically on retry and must propagate. 2705 (duplicate
+    /// column) is deliberately absent: every column lives inside a single <c>CREATE TABLE</c>
+    /// statement, so no cross-session race can produce it.
+    /// </summary>
+    /// <param name="errorNumber">The <see cref="SqlError.Number"/> to classify.</param>
+    /// <returns><see langword="true"/> when the number is a known provisioning-race outcome.</returns>
+    internal static bool IsRetryableMigrationErrorNumber(int errorNumber) => errorNumber
+        is SqlDeadlockVictimErrorNumber
+        or SqlObjectAlreadyExistsErrorNumber
+        or SqlIndexAlreadyExistsErrorNumber
+        or SqlPrimaryKeyViolationErrorNumber;
+
     private async Task MigrateOnceAsync(CancellationToken cancellationToken)
     {
         string schema = connectionFactory.SchemaName;
@@ -54,8 +117,11 @@ public sealed class SqlServerSecretsStoreMigrator(SecretsSqlServerConnectionFact
 
         // Serializable: unlike the single-writer SQLite store, EVERY node in the cluster runs this
         // migration at startup, so concurrent first-boots are the normal case rather than a rare
-        // race. The isolation level plus the IF NOT EXISTS guards make a simultaneous provision by
+        // race. The isolation level narrows the race window but does NOT close it — the IF NOT
-        // two nodes converge on one schema instead of one of them failing with "object exists".
+        // EXISTS metadata probes are not serialized against another session's in-flight CREATE, so
+        // a loser can still fail with "object exists" (2714/1913) or a deadlock (1205). The retry
+        // filter in MigrateAsync is what makes simultaneous provision converge: by the time the
+        // loser retries, the winner's objects exist and every guard here skips them.
         await using SqlTransaction transaction = (SqlTransaction)
             await connection.BeginTransactionAsync(IsolationLevel.Serializable, cancellationToken)
                 .ConfigureAwait(false);

ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/Live/SqlServerSecretStoreLiveTests.cs

@@ -355,6 +355,37 @@ public sealed class SqlServerSecretStoreLiveTests : IAsyncLifetime
 
         Assert.NotNull(await _store.GetAsync(new SecretName("survives/migration"), CancellationToken.None));
     }
+
+    [SkippableFact]
+    public async Task Migrator_converges_when_many_nodes_provision_a_virgin_database_concurrently()
+    {
+        Skip.IfNot(LiveSqlServer.IsEnabled, LiveSqlServer.SkipReason);
+
+        // The ScadaBridge central shared-store live gate (2026-08-07): an ordinary compose up
+        // recreated both central nodes at once, both ran the boot migrator against the virgin
+        // database, and the loser died with SqlException 2714 ("There is already an object named
+        // 'zbsecrets'") — the retry filter then covered only deadlock 1205. Deliberately NOT the
+        // class's shared schema: that one is already migrated by InitializeAsync, and this race
+        // only exists against a virgin database.
+        string schema = LiveSqlServer.NewSchemaName();
+        try
+        {
+            await Task.WhenAll(Enumerable.Range(0, 4).Select(_ => Task.Run(() =>
+                new SqlServerSecretsStoreMigrator(
+                        new SecretsSqlServerConnectionFactory(LiveSqlServer.OptionsFor(schema)))
+                    .MigrateAsync(CancellationToken.None))));
+
+            // Every "node" survived; prove the converged schema is actually usable.
+            var store = new SqlServerSecretStore(
+                new SecretsSqlServerConnectionFactory(LiveSqlServer.OptionsFor(schema)));
+            await store.UpsertAsync(MakeSecret("concurrent/provision"), CancellationToken.None);
+            Assert.NotNull(await store.GetAsync(new SecretName("concurrent/provision"), CancellationToken.None));
+        }
+        finally
+        {
+            await LiveSqlServer.DropSchemaAsync(schema);
+        }
+    }
 }
 
 /// <summary>

ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/SqlServerSecretsStoreMigratorTests.cs

@@ -0,0 +1,40 @@
+namespace ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests;
+
+/// <summary>
+/// Offline pins for the migrator's retry classification. A <see cref="Microsoft.Data.SqlClient.SqlException"/>
+/// cannot be constructed normally, so the error-number predicate is extracted as an internal static
+/// and pinned here by number; the concurrent-provision behaviour itself is exercised by the
+/// env-gated live suite.
+/// </summary>
+public sealed class SqlServerSecretsStoreMigratorTests
+{
+    /// <summary>
+    /// The provisioning-race outcomes the ScadaBridge central shared-store live gate (2026-08-07)
+    /// proved reachable — or that the multi-statement DDL makes reachable — when N nodes provision
+    /// one virgin database at once. Losing such a race means the object now exists, which is the
+    /// desired end state, so a retry of the idempotent IF NOT EXISTS path must be taken.
+    /// </summary>
+    [Theory]
+    [InlineData(1205)] // deadlock victim (pre-0.5.1 was the ONLY retried number)
+    [InlineData(2714)] // "There is already an object named ..." — lost CREATE SCHEMA/TABLE (the live-gate defect)
+    [InlineData(1913)] // "index ... already exists" — lost CREATE INDEX
+    [InlineData(2627)] // PK violation — lost schema_version MERGE-insert
+    public void Provisioning_race_error_numbers_are_retryable(int errorNumber)
+        => Assert.True(SqlServerSecretsStoreMigrator.IsRetryableMigrationErrorNumber(errorNumber));
+
+    /// <summary>
+    /// Errors that reproduce identically on retry must propagate so the node fails closed instead
+    /// of spinning through the retry budget against a genuine misconfiguration.
+    /// </summary>
+    [Theory]
+    [InlineData(2705)] // duplicate column — unreachable from the race (single CREATE TABLE statement)
+    [InlineData(208)] // invalid object name
+    [InlineData(229)] // permission denied
+    [InlineData(2759)] // "CREATE SCHEMA failed due to previous errors" — retryable only via its companion 2714
+    [InlineData(4060)] // cannot open database
+    [InlineData(18456)] // login failed
+    [InlineData(0)]
+    [InlineData(-2)] // client-side timeout
+    public void Non_race_error_numbers_are_not_retryable(int errorNumber)
+        => Assert.False(SqlServerSecretsStoreMigrator.IsRetryableMigrationErrorNumber(errorNumber));
+}