From 31ca9407a6ba9d23f2e1b19534de8246d652d9cc Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 11:35:13 -0400 Subject: [PATCH] fix(secrets-sqlserver): retry the virgin-DB concurrent migrator race; release family 0.5.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 25 ++++++ ZB.MOM.WW.Secrets/Directory.Build.props | 2 +- .../SqlServerSecretsStoreMigrator.cs | 90 ++++++++++++++++--- .../Live/SqlServerSecretStoreLiveTests.cs | 31 +++++++ .../SqlServerSecretsStoreMigratorTests.cs | 40 +++++++++ 5 files changed, 175 insertions(+), 13 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Replicator.SqlServer.Tests/SqlServerSecretsStoreMigratorTests.cs 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)); +}