From 2a0a457da2c8755c9f3bcad283976b516c9ad202 Mon Sep 17 00:00:00 2001 From: iancooper Date: Mon, 12 Oct 2026 07:41:40 +0100 Subject: [PATCH 1/2] refactor: tidy comments in BoxProvisioning.Sqlite Delete comments that narrate code or repeat rationale already in ADR 0057, strip spec, issue and PR references, trim XML remarks that duplicated the runner's or pointed at other files' line numbers, move the busy-timeout note above ToDriverBusyTimeoutSeconds, and turn the LockResourceFor comment into XML remarks on the protected override. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7 --- .../SqliteBoxDetectionHelper.cs | 4 +- .../SqliteBoxMigrationRunner.cs | 56 ++++++------------- .../SqliteBoxProvisioningExtensions.cs | 6 +- .../SqliteInboxMigrationCatalog.cs | 15 ++--- .../SqliteInboxProvisioner.cs | 6 +- .../SqliteOutboxMigrationCatalog.cs | 22 ++------ .../SqliteOutboxProvisioner.cs | 6 +- .../SqliteProvisioningUnitOfWork.cs | 48 ++++------------ 8 files changed, 46 insertions(+), 117 deletions(-) diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxDetectionHelper.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxDetectionHelper.cs index 28272cea9d..0e1b10e908 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxDetectionHelper.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxDetectionHelper.cs @@ -75,7 +75,7 @@ public async Task DoesHistoryExistAsync( CancellationToken cancellationToken = default, SqliteTransaction? transaction = null) { - _ = historySchema; // SQLite has no schema concept; PerSchema is a no-op here. + _ = historySchema; var historyTableExists = await DoesTableExistAsync( connection, "__BrighterMigrationHistory", schemaName, cancellationToken, transaction); if (!historyTableExists) @@ -103,7 +103,7 @@ public async Task GetMaxVersionAsync( CancellationToken cancellationToken = default, SqliteTransaction? transaction = null) { - _ = historySchema; // SQLite has no schema concept; PerSchema is a no-op here. + _ = historySchema; using var command = connection.CreateCommand(); if (transaction != null) command.Transaction = transaction; command.CommandText = @" diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs index b8f1650531..a354f61a79 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs @@ -70,12 +70,11 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite; /// SqliteConnection.DefaultTimeout to the lockTimeout (rounded up to whole seconds — /// the driver's granularity), and Microsoft.Data.Sqlite drives sqlite3_busy_timeout from that /// value on every internal statement, including the synthesised BEGIN IMMEDIATE. The -/// driver's built-in busy handler retries with backoff inside the call; we do not wrap a -/// retry loop in C#. If the budget elapses while the slot is still held, the original +/// driver's busy handler retries with backoff inside the call. If the budget elapses while the slot is still held, the original /// with SqliteErrorCode == 5 (SQLITE_BUSY) -/// propagates to the runner's catch path. Note: PRAGMA busy_timeout issued in SQL is -/// silently overwritten by the next command's CommandTimeout-derived re-application, so -/// DefaultTimeout is the only reliable hook. +/// propagates to the runner's catch path. PRAGMA busy_timeout issued in SQL is +/// silently overwritten by the driver before the next command, so DefaultTimeout is the only +/// reliable hook. /// /// public class SqliteBoxMigrationRunner : SqlBoxMigrationRunner @@ -106,10 +105,7 @@ public SqliteBoxMigrationRunner( } /// - /// Convenience ctor used by the AddSqliteOutbox/AddSqliteInbox registration - /// extensions: synthesises a default so the - /// extension method doesn't have to resolve it from the container before the catalog - /// is in scope. + /// Initialises the runner with a default . /// public SqliteBoxMigrationRunner( IAmABoxMigrationCatalog catalog, @@ -124,8 +120,8 @@ public SqliteBoxMigrationRunner( /// /// Convenience constructor using the default - /// of 30 seconds and WAL journal mode enabled. Tests and callers that need a custom timeout - /// or want to skip the WAL pragma should use the multi-argument form. + /// of 30 seconds and WAL journal mode enabled. Callers that need a custom timeout or want to + /// skip the WAL pragma should use the multi-argument form. /// public SqliteBoxMigrationRunner( IAmABoxMigrationCatalog catalog, @@ -142,8 +138,6 @@ public SqliteBoxMigrationRunner( /// is a no-op. protected override string? DefaultHistorySchema => null; - // ==== Per-backend hook overrides for SqlBoxMigrationRunner ==== - protected override async Task OpenConnectionAsync(CancellationToken cancellationToken) { var connection = new SqliteConnection(Configuration.ConnectionString); @@ -168,9 +162,9 @@ protected override Task> CreateUni new SqliteProvisioningUnitOfWork(connection, Logger)); } - // SQLite has no schema concept, so the schema is folded out of the lock resource. The - // string is symbolic only — SqliteProvisioningUnitOfWork's BeginAsync logs it for trace - // diagnostics but does not use it for locking (BEGIN IMMEDIATE owns that role). + /// + /// SQLite has no schema concept, so the resource is the table name. It is only + /// logged: BEGIN IMMEDIATE is the lock. protected override string LockResourceFor(string? schemaName, string tableName) { _ = schemaName; @@ -181,14 +175,9 @@ protected override async Task EnsureHistoryTableAsync( SqliteConnection connection, SqliteTransaction? transaction, string? schemaName, string tableName, CancellationToken cancellationToken) { - _ = schemaName; // SQLite has no schema concept. - _ = tableName; // SQLite is out of scope for PerSchema; the D5 seed never fires here. - - // No race-handling needed: BEGIN IMMEDIATE above acquires SQLite's database-wide RESERVED - // lock, so only one writer can be inside this transaction at a time. Concurrent runners - // queue at the BEGIN IMMEDIATE call rather than racing on CREATE TABLE — by the time a - // second writer enters this method, the first has already committed and IF NOT EXISTS - // sees the table. + _ = schemaName; + _ = tableName; + using var command = connection.CreateCommand(); command.Transaction = transaction; command.CommandText = $@" @@ -206,13 +195,8 @@ protected override async Task RunFreshPathAsync( SqliteConnection connection, SqliteTransaction? transaction, string? schemaName, string tableName, string freshInstallDdl, int latestVersion, CancellationToken cancellationToken) { - _ = schemaName; // SQLite has no schema concept. + _ = schemaName; - // Execute the V_latest-shape DDL sourced from IAmABoxMigrationCatalog.FreshInstallDdl - // (the live SqliteOutboxBuilder.GetDDL / SqliteInboxBuilder.GetDDL output). Stamp - // directly at V_latest with a "fresh install" marker — V2..V_latest ALTERs would all be - // no-ops (and would in fact throw "duplicate column name" without the IdempotencyCheckSql - // skip), so we elide the chain entirely (spec 0027 R1 fresh-install fast path). await ExecuteUpScriptAsync(connection, transaction!, freshInstallDdl, cancellationToken); await InsertHistoryRowAsync( @@ -224,7 +208,7 @@ protected override async Task RunBootstrapPathAsync( SqliteConnection connection, SqliteTransaction? transaction, string? schemaName, string tableName, BoxType boxType, IReadOnlyList migrations, CancellationToken cancellationToken) { - _ = schemaName; // SQLite has no schema concept. + _ = schemaName; var detected = await DetectionHelper.DetectCurrentVersionAsync( connection, tableName, null, boxType, migrations, cancellationToken, transaction); @@ -261,7 +245,7 @@ protected override async Task RunNormalPathAsync( SqliteConnection connection, SqliteTransaction? transaction, string? schemaName, string tableName, IReadOnlyList migrations, CancellationToken cancellationToken) { - _ = schemaName; // SQLite has no schema concept. + _ = schemaName; var maxVersion = await DetectionHelper.GetMaxVersionAsync( connection, tableName, null, ResolveHistorySchema(), cancellationToken, transaction); @@ -322,15 +306,9 @@ private static async Task ExecuteUpScriptAsync( await command.ExecuteNonQueryAsync(cancellationToken); } + // Whole seconds, at least 1: the driver reads a DefaultTimeout of 0 as "wait forever". private static int ToDriverBusyTimeoutSeconds(TimeSpan lockTimeout) { - // Microsoft.Data.Sqlite drives sqlite3_busy_timeout from connection.DefaultTimeout - // (in seconds), re-applying it before every internal command — including the BEGIN - // command synthesised by BeginTransaction(IsolationLevel, deferred). PRAGMA busy_timeout - // would be silently overwritten by that re-application, so DefaultTimeout is the only - // reliable way to honour lockTimeout. Driver granularity is whole seconds; sub-second - // lockTimeouts floor to 1s. DefaultTimeout = 0 means "wait forever" in this driver - // (not "fail-fast"), so callers wanting fail-fast pass TimeSpan.Zero and get ~1s. return (int)Math.Max(1, Math.Ceiling(lockTimeout.TotalSeconds)); } diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxProvisioningExtensions.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxProvisioningExtensions.cs index 8c75a66f84..5bca471268 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxProvisioningExtensions.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxProvisioningExtensions.cs @@ -193,11 +193,7 @@ public static BoxProvisioningOptions AddSqliteInbox( return options; } - // Detection helper + payload validator are shared across Outbox and Inbox extensions; - // TryAddSingleton makes the registration idempotent so calling both AddSqliteOutbox and - // AddSqliteInbox does not produce duplicate singletons. Catalogue registration is per - // box-type and lives in the call-site (Outbox catalogue from AddSqliteOutbox; Inbox - // catalogue from AddSqliteInbox) per ADR 0058 §A.4 Alternatives. + // Shared by the outbox and inbox registrations; TryAdd makes calling both safe. private static void RegisterSharedRoleImpls(IServiceCollection services) { services.TryAddSingleton(); diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxMigrationCatalog.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxMigrationCatalog.cs index d247dbfb6d..b28e605492 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxMigrationCatalog.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxMigrationCatalog.cs @@ -32,10 +32,8 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite; /// /// /// V1's UpScript is the literal historical baseline DDL — the first SQLite inbox -/// builder shape (commit 695522367, March 2019). Spec 0027 R1 split "live builder -/// DDL" away from V1.UpScript: the fresh-install fast path (ADR 0057 §3) now sources its -/// DDL from , so V1.UpScript is free to carry the honest -/// historical shape. V2 adds the ContextKey column (spec version stamped 787c31c52, +/// builder shape (commit 695522367, March 2019). The fresh-install fast path +/// (ADR 0057 §3) uses instead. V2 adds the ContextKey column (spec version stamped 787c31c52, /// Oct 2018) via a plain ALTER TABLE ADD COLUMN — SQLite's grammar lacks /// ALTER TABLE ADD COLUMN IF NOT EXISTS, so the existence guard lives in /// per ADR 0057 §6 (probing @@ -63,13 +61,8 @@ public class SqliteInboxMigrationCatalog : IAmABoxMigrationCatalog private static readonly string[] s_v3AddedColumns = ["CausationId"]; - // Literal historical SQLite inbox DDL extracted from commit 695522367 (March 2019). - // The original source was a concatenated string literal — preserved verbatim here. The - // table first shipped with ContextKey already present — see the born-past-V1 note in - // the class remarks. {0} = table name (validated). - // The table identifier is bracket-quoted so legal-but-reserved SQLite keyword names - // bootstrap correctly — V2 already bracket-quotes the table identifier, so V1 is the - // only asymmetric step. Per PR #4039 reviewer item F2-1. + // The inbox DDL as first shipped (commit 695522367), kept verbatim, ContextKey included (see + // remarks). {0} is the validated table name, bracket-quoted so reserved words work. private const string V1HistoricalDdl = "CREATE TABLE [{0}] (" + "CommandId uniqueidentifier CONSTRAINT PK_MessageId PRIMARY KEY," diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxProvisioner.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxProvisioner.cs index 7eeaa8a9ab..11e702bb0a 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxProvisioner.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxProvisioner.cs @@ -52,10 +52,8 @@ protected override SqliteConnection CreateConnection(string connectionString) protected override string PayloadColumnName => "CommandBody"; /// - /// SQLite has no schema concept (per ADR 0057 §6); the detection helper and payload - /// validator receive null at every call site. The runner call inside - /// ProvisionAsync still propagates _configuration.SchemaName directly, which - /// the SQLite migration runner accepts and ignores. + /// SQLite has no schema concept (per ADR 0057 §6), so the detection helper and payload + /// validator receive null. /// protected override string? EffectiveSchemaName => null; } diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxMigrationCatalog.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxMigrationCatalog.cs index 20e9c14a3e..f1b1242e69 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxMigrationCatalog.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxMigrationCatalog.cs @@ -33,10 +33,8 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite; /// /// /// V1's UpScript is the literal historical baseline DDL — the first SQLite outbox -/// builder shape (commit 695522367, March 2019; pre-Dispatched). Spec 0027 R1 split -/// "live builder DDL" away from V1.UpScript: the fresh-install fast path (ADR 0057 §3) now -/// sources its DDL from , so V1.UpScript is free to carry the -/// honest historical shape. V2..V7 are plain ALTER TABLE ADD COLUMN statements — +/// builder shape (commit 695522367, March 2019; pre-Dispatched). The fresh-install fast +/// path (ADR 0057 §3) uses instead. V2..V7 are plain ALTER TABLE ADD COLUMN statements — /// SQLite's grammar lacks ALTER TABLE ADD COLUMN IF NOT EXISTS, so the idempotency /// guard lives in a separate field per /// ADR 0057 §6 (probing pragma_table_info). The SqliteBoxMigrationRunner @@ -46,7 +44,7 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite; /// SQLite outbox V1 is one of three relational outboxes whose first-shipped state matches /// the "logical pre-V2 baseline" (alongside MSSQL and MySQL). The PostgreSQL outbox is the /// lone exception — see the born-past-V1 note in . -/// V8 adds CausationId (Spec 0027, #2541) plus a replay index on it. +/// V8 adds CausationId plus a replay index on it. /// The accumulated LogicalColumns across V1..V8 equals the live builder's column set /// — verified by the drift test in tests/Paramore.Brighter.Sqlite.Tests/BoxProvisioning. /// @@ -56,13 +54,8 @@ public class SqliteOutboxMigrationCatalog : IAmABoxMigrationCatalog private static readonly string[] s_v1Columns = ["MessageId", "Topic", "MessageType", "Timestamp", "HeaderBag", "Body"]; - // Literal historical SQLite outbox DDL extracted from commit 695522367 (March 2019). The - // exact pre-Dispatched shape that shipped as "V1" — preserved here so chain replay against - // a legacy table sees the same starting DDL it always saw. {0} = table name (validated). - // The table identifier is bracket-quoted so legal-but-reserved SQLite keyword names - // bootstrap correctly — V2..V7 already bracket-quote the table identifier (and SQLite - // accepts brackets, backticks, or double-quotes for identifier quoting), so V1 is the - // only asymmetric step. Per PR #4039 reviewer item F2-1. + // The outbox DDL as first shipped (commit 695522367), kept verbatim so chain replay starts + // from the same shape. {0} is the validated table name, bracket-quoted so reserved words work. private const string V1HistoricalDdl = """ CREATE TABLE [{0}] ( @@ -195,10 +188,7 @@ private static string AddColumns(string table, params string[] columns) => private static string AddColumn(string table, string column) => $"ALTER TABLE [{table}] ADD COLUMN [{column}] TEXT NULL;"; - // Idempotent replay index (Spec 0027, #2541) on CausationId via SQLite's native - // CREATE INDEX IF NOT EXISTS. The V8 UpScript is gated as a whole by the column - // IdempotencyCheckSql, so on chain replay the index statement is skipped alongside the - // ALTER (the index was created on the first, column-adding pass). + // V8's column check gates the whole UpScript, so replay skips the index with the ALTER. private static string AddCausationIndex(string table) => $"CREATE INDEX IF NOT EXISTS [idx_{table}_CausationId] ON [{table}] ([CausationId]);"; diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxProvisioner.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxProvisioner.cs index 7fff16ec26..9b5c9a0608 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxProvisioner.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxProvisioner.cs @@ -52,10 +52,8 @@ protected override SqliteConnection CreateConnection(string connectionString) protected override string PayloadColumnName => "Body"; /// - /// SQLite has no schema concept (per ADR 0057 §6); the detection helper and payload - /// validator receive null at every call site. The runner call inside - /// ProvisionAsync still propagates _configuration.SchemaName directly, which - /// the SQLite migration runner accepts and ignores. + /// SQLite has no schema concept (per ADR 0057 §6), so the detection helper and payload + /// validator receive null. /// protected override string? EffectiveSchemaName => null; } diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteProvisioningUnitOfWork.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteProvisioningUnitOfWork.cs index 7e0cee41b6..deb702c4fa 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteProvisioningUnitOfWork.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteProvisioningUnitOfWork.cs @@ -34,22 +34,14 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite; /// SQLite unit-of-work for box-table migrations. Per ADR 0057 §4 / ADR 0058 §B.1 SQLite has no /// advisory-lock primitive — the database-wide RESERVED writer slot acquired by /// BEGIN IMMEDIATE is itself the migration lock, combined atomically with opening the -/// transaction. The ctor therefore takes only the connection and a logger; there is no -/// IAmA*AdvisoryLock dependency to inject. +/// transaction. /// /// /// Microsoft.Data.Sqlite's BeginTransactionAsync overload does not expose the /// deferred flag, so the synchronous -/// form is used to guarantee BEGIN IMMEDIATE (deferred: false). The wait under -/// writer-slot contention is bounded by , which -/// sets from the caller-supplied lockTimeout -/// before opening the connection. The driver translates DefaultTimeout into -/// sqlite3_busy_timeout on every internal statement (PRAGMA busy_timeout issued in SQL is -/// silently overwritten by the next command), so the retry-with-backoff under SQLITE_BUSY is -/// driver-side; if the budget expires the original with -/// SqliteErrorCode == 5 propagates to the runner's catch path. Per ADR 0057 §4 / -/// ADR 0058 §B.1 this replaces a separate advisory-lock primitive — BEGIN IMMEDIATE is itself -/// the lock acquisition. +/// form is used to guarantee BEGIN IMMEDIATE. The wait under writer-slot contention is +/// bounded by , which +/// sets from the caller-supplied lockTimeout. /// public class SqliteProvisioningUnitOfWork( SqliteConnection connection, @@ -74,10 +66,7 @@ public class SqliteProvisioningUnitOfWork( /// translates it into on the connection /// before this call, which the driver applies as sqlite3_busy_timeout on the synthesised /// BEGIN IMMEDIATE. If the slot remains held past the budget the call throws - /// with SqliteErrorCode == 5 (SQLITE_BUSY) and - /// propagates to the runner's catch path. See the class-level remarks for the driver - /// quirk that makes DefaultTimeout the only reliable hook (PRAGMA busy_timeout is - /// overwritten per-command). + /// with SqliteErrorCode == 5 (SQLITE_BUSY). /// /// public Task BeginAsync(string lockResource, TimeSpan lockTimeout, CancellationToken cancellationToken) @@ -90,12 +79,8 @@ public Task BeginAsync(string lockResource, TimeSpan lockTimeout, CancellationTo /// /// - /// Commits the underlying transaction opened by . Per ADR 0058 - /// §B.1 SQLite has no advisory-lock primitive — the database-wide RESERVED writer slot - /// reserved by the initial BEGIN IMMEDIATE is itself the migration lock — so - /// committing the transaction is what releases the writer slot. There is no separate - /// lock-release call. A no-op return when _transaction is null tolerates a - /// Commit-without-Begin or a Begin-that-threw-before-opening-the-transaction. + /// Commits the transaction opened by , which releases the writer + /// slot. A no-op if did not open a transaction. /// public Task CommitAsync(CancellationToken cancellationToken) { @@ -105,20 +90,11 @@ public Task CommitAsync(CancellationToken cancellationToken) /// /// - /// Best-effort per ADR 0058 §B.3. The runner may invoke from - /// its catch path after a thrown , by which time the underlying - /// transaction may already be finalised (committed-but-client-side-failed, or zombied by - /// a closed connection); in those cases throws - /// ("Transaction has completed; it is no longer - /// usable."). A zombied connection or async cancellation can surface as - /// / too. - /// MUST NOT throw for any of these — the runner's catch path - /// (catch { uow.RollbackAsync(...); throw; }) cannot have its primary migration - /// exception masked by a cleanup-side failure. Catch (matching the - /// MSSQL UoW stance at MsSqlProvisioningUnitOfWork.cs:108 and the PG UoW stance at - /// PostgreSqlProvisioningUnitOfWork.cs:165) and log a Warning so the unwind continues. - /// SQLite has no advisory-lock primitive to release separately; completing the transaction - /// (or a zombied tx being disposed) releases the writer slot regardless of rollback success. + /// Best-effort per ADR 0058 §B.3: never throws, so a failed rollback cannot mask the + /// migration exception that caused it. The runner may roll back after a failed + /// , when the transaction is already finalised; the failure is + /// logged as a warning. The writer slot is released when the transaction completes, + /// whether or not the rollback succeeds. /// public async Task RollbackAsync(CancellationToken cancellationToken) { From 6ce87d75a129d536130eca09543c16190b0a74d1 Mon Sep 17 00:00:00 2001 From: iancooper Date: Mon, 12 Oct 2026 07:42:30 +0100 Subject: [PATCH 2/2] refactor: extract methods to express intent in BoxProvisioning.Sqlite Replace the explanatory comment in SqliteBoxMigrationRunner.ApplyOrSkipAsync by decomposing its conditional into a private IsAlreadyAppliedAsync, keeping one line above the signature for the history-row trap the name cannot carry. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HsKD52dnGHFLSEr93VZyR7 --- .../SqliteBoxMigrationRunner.cs | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs index a354f61a79..0cf0b6bb38 100644 --- a/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs +++ b/src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs @@ -258,24 +258,13 @@ protected override async Task RunNormalPathAsync( } } + // History is recorded even when the UpScript is skipped, so MAX(V) still advances. private static async Task ApplyOrSkipAsync( SqliteConnection connection, SqliteTransaction transaction, string tableName, IAmABoxMigration migration, CancellationToken cancellationToken) { - // Per ADR §6: if IdempotencyCheckSql is non-null, evaluate it under the lock-bearing - // transaction. A non-zero scalar means the migration's effect is already present — - // skip UpScript but still record history so MAX(V) advances. - if (!string.IsNullOrEmpty(migration.IdempotencyCheckSql?.Value)) - { - var alreadyApplied = await ScalarIsPositiveAsync( - connection, transaction, migration.IdempotencyCheckSql!, cancellationToken); - if (!alreadyApplied) - { - await ExecuteUpScriptAsync(connection, transaction, migration.UpScript.Value, cancellationToken); - } - } - else + if (!await IsAlreadyAppliedAsync(connection, transaction, migration, cancellationToken)) { await ExecuteUpScriptAsync(connection, transaction, migration.UpScript.Value, cancellationToken); } @@ -285,6 +274,16 @@ await InsertHistoryRowAsync( migration.Version, migration.Description.Value, cancellationToken); } + private static async Task IsAlreadyAppliedAsync( + SqliteConnection connection, SqliteTransaction transaction, + IAmABoxMigration migration, CancellationToken cancellationToken) + { + if (string.IsNullOrEmpty(migration.IdempotencyCheckSql?.Value)) return false; + + return await ScalarIsPositiveAsync( + connection, transaction, migration.IdempotencyCheckSql!, cancellationToken); + } + private static async Task ScalarIsPositiveAsync( SqliteConnection connection, SqliteTransaction transaction, string sql, CancellationToken cancellationToken)