From d0d305865119afae55f18fbac505f0b9b08ea881 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 12:05:33 -0500 Subject: [PATCH] fix(secrets): jittered exponential backoff in SqlServerSecretsStoreMigrator + 8-way concurrent-migrate live test Flat 100*attempt backoff let concurrent boots retry in lockstep (Auth's twin loop exhausted retries ~1/60 eight-way boots). Backoff is now exponential with equal jitter, via an injectable Random for deterministic tests. Refs scadaproj#18 --- .../SqlServerSecretsStoreMigrator.cs | 37 +++++++++++++++++-- .../Live/SqlServerSecretStoreLiveTests.cs | 16 +++++++- .../SqlServerSecretsStoreMigratorTests.cs | 36 ++++++++++++++++++ 3 files changed, 84 insertions(+), 5 deletions(-) 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 51bc4fd..1601616 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 @@ -13,9 +13,22 @@ namespace ZB.MOM.WW.Secrets.Replicator.SqlServer; /// 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 +/// Jitter source for the retry back-off; inject a seeded instance for deterministic tests. +public sealed class SqlServerSecretsStoreMigrator( + SecretsSqlServerConnectionFactory connectionFactory, + Random random) : ISecretsStoreMigrator { + /// + /// Initializes the migrator with the process-wide as its jitter source. + /// + /// Factory for connections to the shared secret database. + public SqlServerSecretsStoreMigrator(SecretsSqlServerConnectionFactory connectionFactory) + : this(connectionFactory, Random.Shared) + { + } + + private const int BaseBackoffMilliseconds = 100; + // 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 @@ -63,13 +76,29 @@ public sealed class SqlServerSecretsStoreMigrator(SecretsSqlServerConnectionFact catch (SqlException ex) when (IsRetryableMigrationError(ex) && attempt < MaxMigrationRetries) { - // Staggered back-off so two victims do not retry in lockstep and race again. - await Task.Delay(TimeSpan.FromMilliseconds(100 * attempt), cancellationToken) + // Jittered back-off so N victims do not retry in lockstep and race again (a flat + // 100 * attempt schedule exhausted its retries ~1 in 60 eight-way boots in Auth's + // identical loop; scadaproj#18). + await Task.Delay(ComputeBackoff(attempt, random), cancellationToken) .ConfigureAwait(false); } } } + /// + /// Computes the delay before retry number using exponential back-off + /// with "equal jitter": half the exponential window is fixed, the other half is random, so the + /// result lies in [window/2, window) where window = 100 ms * 2^(attempt - 1). + /// + /// The 1-based attempt that just failed. + /// Jitter source; inject a seeded instance for deterministic tests. + /// The delay to wait before the next attempt. + internal static TimeSpan ComputeBackoff(int attempt, Random random) + { + double window = BaseBackoffMilliseconds * Math.Pow(2, attempt - 1); + return TimeSpan.FromMilliseconds((window / 2) + (random.NextDouble() * window / 2)); + } + /// /// Whether any error carried by is a retryable provisioning-race /// outcome. Scans the whole collection rather than only 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 c5cc1b1..2dc2d05 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 @@ -358,6 +358,17 @@ public sealed class SqlServerSecretStoreLiveTests : IAsyncLifetime [SkippableFact] public async Task Migrator_converges_when_many_nodes_provision_a_virgin_database_concurrently() + => await ProvisionConcurrentlyAsync(nodes: 4, rounds: 1); + + /// + /// scadaproj#18: the Auth twin of this loop exhausted its retries ~1 in 60 eight-way boots with + /// flat back-off; repeated rounds of 8 concurrent migrators against fresh virgin schemas. + /// + [SkippableFact] + public async Task Migrate_Concurrently_FromEightTasks_AllSucceed() + => await ProvisionConcurrentlyAsync(nodes: 8, rounds: 10); + + private static async Task ProvisionConcurrentlyAsync(int nodes, int rounds) { Skip.IfNot(LiveSqlServer.IsEnabled, LiveSqlServer.SkipReason); @@ -367,10 +378,12 @@ public sealed class SqlServerSecretStoreLiveTests : IAsyncLifetime // '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. + for (int round = 0; round < rounds; round++) + { string schema = LiveSqlServer.NewSchemaName(); try { - await Task.WhenAll(Enumerable.Range(0, 4).Select(_ => Task.Run(() => + await Task.WhenAll(Enumerable.Range(0, nodes).Select(_ => Task.Run(() => new SqlServerSecretsStoreMigrator( new SecretsSqlServerConnectionFactory(LiveSqlServer.OptionsFor(schema))) .MigrateAsync(CancellationToken.None)))); @@ -385,6 +398,7 @@ public sealed class SqlServerSecretStoreLiveTests : IAsyncLifetime { 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 index 501ddef..779247a 100644 --- 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 @@ -37,4 +37,40 @@ public sealed class SqlServerSecretsStoreMigratorTests [InlineData(-2)] // client-side timeout public void Non_race_error_numbers_are_not_retryable(int errorNumber) => Assert.False(SqlServerSecretsStoreMigrator.IsRetryableMigrationErrorNumber(errorNumber)); + + /// The jittered back-off stays inside [window/2, window) and grows with the attempt. + [Theory] + [InlineData(1, 50, 100)] + [InlineData(2, 100, 200)] + [InlineData(4, 400, 800)] + public void Backoff_is_bounded_by_the_exponential_window(int attempt, double minMs, double maxMs) + { + var random = new Random(12345); + for (int i = 0; i < 1000; i++) + { + double ms = SqlServerSecretsStoreMigrator.ComputeBackoff(attempt, random).TotalMilliseconds; + Assert.InRange(ms, minMs, maxMs); + } + } + + /// Different draws give different delays (the lockstep-retry defect was a constant schedule). + [Fact] + public void Backoff_is_jittered_not_constant() + { + var random = new Random(42); + var delays = Enumerable.Range(0, 8) + .Select(_ => SqlServerSecretsStoreMigrator.ComputeBackoff(3, random)) + .Distinct() + .ToList(); + Assert.True(delays.Count > 1); + } + + /// A fixed jitter source yields an exactly predictable delay. + [Fact] + public void Backoff_is_deterministic_for_a_seeded_source() + { + Assert.Equal( + SqlServerSecretsStoreMigrator.ComputeBackoff(3, new Random(7)), + SqlServerSecretsStoreMigrator.ComputeBackoff(3, new Random(7))); + } }