Tidy comments in BoxProvisioning.Sqlite

src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxDetectionHelper.cs

@@ -75,7 +75,7 @@ public async Task<bool> 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<int> 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 = @"

src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteBoxMigrationRunner.cs

@@ -70,12 +70,11 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite;
 /// <c>SqliteConnection.DefaultTimeout</c> 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 <c>BEGIN IMMEDIATE</c>. 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
 /// <see cref="SqliteException"/> with <c>SqliteErrorCode == 5</c> (<c>SQLITE_BUSY</c>)
-/// propagates to the runner's catch path. Note: <c>PRAGMA busy_timeout</c> 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. <c>PRAGMA busy_timeout</c> issued in SQL is
+/// silently overwritten by the driver before the next command, so DefaultTimeout is the only
+/// reliable hook.
 /// </para>
 /// </remarks>
 public class SqliteBoxMigrationRunner : SqlBoxMigrationRunner<SqliteConnection, SqliteTransaction>
@@ -106,10 +105,7 @@ public SqliteBoxMigrationRunner(
     }
 
     /// <summary>
-    /// Convenience ctor used by the <c>AddSqliteOutbox</c>/<c>AddSqliteInbox</c> registration
-    /// extensions: synthesises a default <see cref="SqliteBoxDetectionHelper"/> 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 <see cref="SqliteBoxDetectionHelper"/>.
     /// </summary>
     public SqliteBoxMigrationRunner(
         IAmABoxMigrationCatalog catalog,
@@ -124,8 +120,8 @@ public SqliteBoxMigrationRunner(
 
     /// <summary>
     /// Convenience constructor using the default <see cref="BoxProvisioningOptions.MigrationLockTimeout"/>
-    /// 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.
     /// </summary>
     public SqliteBoxMigrationRunner(
         IAmABoxMigrationCatalog catalog,
@@ -142,8 +138,6 @@ public SqliteBoxMigrationRunner(
     /// <see cref="MigrationHistoryScope.PerSchema"/> is a no-op.</remarks>
     protected override string? DefaultHistorySchema => null;
 
-    // ==== Per-backend hook overrides for SqlBoxMigrationRunner ====
-
     protected override async Task<SqliteConnection> OpenConnectionAsync(CancellationToken cancellationToken)
     {
         var connection = new SqliteConnection(Configuration.ConnectionString);
@@ -168,9 +162,9 @@ protected override Task<IAmAProvisioningUnitOfWork<SqliteTransaction>> 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).
+    /// <inheritdoc />
+    /// <remarks>SQLite has no schema concept, so the resource is the table name. It is only
+    /// logged: <c>BEGIN IMMEDIATE</c> is the lock.</remarks>
     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<IAmABoxMigration> 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<IAmABoxMigration> migrations, CancellationToken cancellationToken)
     {
-        _ = schemaName; // SQLite has no schema concept.
+        _ = schemaName;
 
         var maxVersion = await DetectionHelper.GetMaxVersionAsync(
             connection, tableName, null, ResolveHistorySchema(), cancellationToken, transaction);
@@ -274,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);
         }
@@ -301,6 +274,16 @@ await InsertHistoryRowAsync(
             migration.Version, migration.Description.Value, cancellationToken);
     }
 
+    private static async Task<bool> 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<bool> ScalarIsPositiveAsync(
         SqliteConnection connection, SqliteTransaction transaction,
         string sql, CancellationToken cancellationToken)
@@ -322,15 +305,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));
     }
 

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<SqliteBoxDetectionHelper>();

src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxMigrationCatalog.cs

@@ -32,10 +32,8 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite;
 /// </summary>
 /// <remarks>
 /// V1's <c>UpScript</c> is the literal historical baseline DDL — the first SQLite inbox
-/// builder shape (commit <c>695522367</c>, 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 <see cref="FreshInstallDdl"/>, so V1.UpScript is free to carry the honest
-/// historical shape. V2 adds the <c>ContextKey</c> column (spec version stamped 787c31c52,
+/// builder shape (commit <c>695522367</c>, March 2019). The fresh-install fast path
+/// (ADR 0057 §3) uses <see cref="FreshInstallDdl"/> instead. V2 adds the <c>ContextKey</c> column (spec version stamped 787c31c52,
 /// Oct 2018) via a plain <c>ALTER TABLE ADD COLUMN</c> — SQLite's grammar lacks
 /// <c>ALTER TABLE ADD COLUMN IF NOT EXISTS</c>, so the existence guard lives in
 /// <see cref="IAmABoxMigration.IdempotencyCheckSql"/> 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,"

src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteInboxProvisioner.cs

@@ -52,10 +52,8 @@ protected override SqliteConnection CreateConnection(string connectionString)
     protected override string PayloadColumnName => "CommandBody";
 
     /// <summary>
-    /// SQLite has no schema concept (per ADR 0057 §6); the detection helper and payload
-    /// validator receive <c>null</c> at every call site. The runner call inside
-    /// <c>ProvisionAsync</c> still propagates <c>_configuration.SchemaName</c> 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 <c>null</c>.
     /// </summary>
     protected override string? EffectiveSchemaName => null;
 }

src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxMigrationCatalog.cs

@@ -33,10 +33,8 @@ namespace Paramore.Brighter.BoxProvisioning.Sqlite;
 /// </summary>
 /// <remarks>
 /// V1's <c>UpScript</c> is the literal historical baseline DDL — the first SQLite outbox
-/// builder shape (commit <c>695522367</c>, 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 <see cref="FreshInstallDdl"/>, so V1.UpScript is free to carry the
-/// honest historical shape. V2..V7 are plain <c>ALTER TABLE ADD COLUMN</c> statements —
+/// builder shape (commit <c>695522367</c>, March 2019; pre-Dispatched). The fresh-install fast
+/// path (ADR 0057 §3) uses <see cref="FreshInstallDdl"/> instead. V2..V7 are plain <c>ALTER TABLE ADD COLUMN</c> statements —
 /// SQLite's grammar lacks <c>ALTER TABLE ADD COLUMN IF NOT EXISTS</c>, so the idempotency
 /// guard lives in a separate <see cref="IAmABoxMigration.IdempotencyCheckSql"/> field per
 /// ADR 0057 §6 (probing <c>pragma_table_info</c>). The <c>SqliteBoxMigrationRunner</c>
@@ -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 <see cref="PostgreSqlOutboxMigrationCatalog"/>.
-/// V8 adds <c>CausationId</c> (Spec 0027, #2541) plus a replay index on it.
+/// V8 adds <c>CausationId</c> plus a replay index on it.
 /// The accumulated <c>LogicalColumns</c> across V1..V8 equals the live builder's column set
 /// — verified by the drift test in <c>tests/Paramore.Brighter.Sqlite.Tests/BoxProvisioning</c>.
 /// </para>
@@ -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]);";
 

src/Paramore.Brighter.BoxProvisioning.Sqlite/SqliteOutboxProvisioner.cs

@@ -52,10 +52,8 @@ protected override SqliteConnection CreateConnection(string connectionString)
     protected override string PayloadColumnName => "Body";
 
     /// <summary>
-    /// SQLite has no schema concept (per ADR 0057 §6); the detection helper and payload
-    /// validator receive <c>null</c> at every call site. The runner call inside
-    /// <c>ProvisionAsync</c> still propagates <c>_configuration.SchemaName</c> 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 <c>null</c>.
     /// </summary>
     protected override string? EffectiveSchemaName => null;
 }