diff --git a/ZB.MOM.WW.Secrets/CHANGELOG.md b/ZB.MOM.WW.Secrets/CHANGELOG.md index 6792cd1..2cac8aa 100644 --- a/ZB.MOM.WW.Secrets/CHANGELOG.md +++ b/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 diff --git a/ZB.MOM.WW.Secrets/Directory.Build.props b/ZB.MOM.WW.Secrets/Directory.Build.props index 6ceaaee..9f00eb1 100644 --- a/ZB.MOM.WW.Secrets/Directory.Build.props +++ b/ZB.MOM.WW.Secrets/Directory.Build.props @@ -5,7 +5,7 @@ enable enable latest - 0.5.0 + 0.5.1 true README.md diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretsStoreMigrator.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretsStoreMigrator.cs index 653f0f5..51bc4fd 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretsStoreMigrator.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretsStoreMigrator.cs @@ -7,21 +7,46 @@ namespace ZB.MOM.WW.Secrets.Replicator.SqlServer; /// /// 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 -/// whose recorded version is newer than this build supports. +/// run on every node at every startup, including concurrently — lost provisioning races surface as +/// retryable s (see ) and the +/// retry re-runs the IF NOT EXISTS path against the objects the winner just created. Refuses +/// to run against a database whose recorded version is newer than this build supports. /// /// Factory for connections to the shared secret database. public sealed class SqlServerSecretsStoreMigrator(SecretsSqlServerConnectionFactory connectionFactory) : ISecretsStoreMigrator { - // Concurrent Serializable DDL against the system catalogs can pick a deadlock victim (error - // 1205) even though the IF NOT EXISTS guards make the logic itself safe. Every node runs this at - // every startup, so a virgin database being provisioned by N nodes at once is the NORMAL case, - // not a rare race — and an unretried victim aborts that node's whole host at startup. Retrying - // is correct precisely because the migration is idempotent. - private const int MaxDeadlockRetries = 5; + // Every node runs this at every startup, so a virgin database being provisioned by N nodes at + // once is the NORMAL case, not a rare race — and an unretried failure aborts that node's whole + // host at startup. Two failure shapes come out of that race, and retrying is correct for both + // precisely because the migration is idempotent: + // + // - 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; + + /// Deadlock victim. private const int SqlDeadlockVictimErrorNumber = 1205; + /// "There is already an object named '%ls'" — lost CREATE SCHEMA / CREATE TABLE race. + private const int SqlObjectAlreadyExistsErrorNumber = 2714; + + /// "An index or statistics with name '%ls' already exists" — lost CREATE INDEX race. + private const int SqlIndexAlreadyExistsErrorNumber = 1913; + + /// Primary-key violation — lost version-row MERGE-insert race. + private const int SqlPrimaryKeyViolationErrorNumber = 2627; + /// /// /// The recorded schema version is newer than . @@ -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); } } } + /// + /// Whether any error carried by is a retryable provisioning-race + /// outcome. Scans the whole collection rather than only + /// 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. + /// + /// The exception the migration attempt failed with. + /// when a retry may succeed. + internal static bool IsRetryableMigrationError(SqlException exception) + { + foreach (SqlError error in exception.Errors) + { + if (IsRetryableMigrationErrorNumber(error.Number)) + { + return true; + } + } + + return IsRetryableMigrationErrorNumber(exception.Number); + } + + /// + /// 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 IF NOT EXISTS 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 CREATE TABLE + /// statement, so no cross-session race can produce it. + /// + /// The to classify. + /// when the number is a known provisioning-race outcome. + 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 - // two nodes converge on one schema instead of one of them failing with "object exists". + // race. The isolation level narrows the race window but does NOT close it — the IF NOT + // 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); diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/Live/SqlServerSecretStoreLiveTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/Live/SqlServerSecretStoreLiveTests.cs index 46a0a02..c5cc1b1 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/Live/SqlServerSecretStoreLiveTests.cs +++ b/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); + } + } } /// diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/SqlServerSecretsStoreMigratorTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/SqlServerSecretsStoreMigratorTests.cs new file mode 100644 index 0000000..501ddef --- /dev/null +++ b/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; + +/// +/// Offline pins for the migrator's retry classification. A +/// 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. +/// +public sealed class SqlServerSecretsStoreMigratorTests +{ + /// + /// 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. + /// + [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)); + + /// + /// Errors that reproduce identically on retry must propagate so the node fails closed instead + /// of spinning through the retry budget against a genuine misconfiguration. + /// + [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)); +}