Tidy comments in BoxProvisioning.PostgreSql (slice 3)

docs/adr/0057-box-schema-versioning-and-migrations.md

@@ -223,7 +223,7 @@ The asymmetry across backends is documented per-catalog: MSSQL/MySQL/SQLite outb
 | Backend | Strategy | Why no extra serialization |
 |---|---|---|
 | MSSQL | `IF NOT EXISTS` + `CREATE TABLE`; catch 2714 | Statement-terminating under default `XACT_ABORT OFF` — loser's tx stays usable |
-| PostgreSQL | `CREATE TABLE IF NOT EXISTS`; catch 23505 / 42P07 / 42710 | pg_class check and pg_type insert are not atomic |
+| PostgreSQL | `CREATE TABLE IF NOT EXISTS` inside a `SAVEPOINT`; catch 23505 / 42P07 / 42710 and `ROLLBACK TO SAVEPOINT` | pg_class check and pg_type insert are not atomic. Postgres marks the whole transaction aborted on the error even when it is caught, so without the savepoint the next statement fails with 25P02 |
 | MySQL | `CREATE TABLE IF NOT EXISTS` | `MDL_EXCLUSIVE` metadata lock makes the statement atomic |
 | SQLite | `CREATE TABLE IF NOT EXISTS` | `BEGIN IMMEDIATE` writer lock already serialises concurrent runners |
 

docs/adr/0060-multi-tenancy-migration-history-scope.md

@@ -250,6 +250,12 @@ Structural changes precede behavioural ones, in separate commits (design princip
 
 These corrections are reflected in `specs/0029-multi-tenancy-migrations/tasks.md` (S1, T3) and do not change the decision's substance (single resolver, opt-in scope, upgrade-safe default).
 
+**2026-10-12 (D5 as implemented, MSSQL and PostgreSQL):**
+
+3. **The seed runs on every `PerSchema` provision, not only when the per-schema history table is first created.** D5's "One-time" bullet gates the seed on a pre-create probe. That gate is wrong when more than one box shares the per-schema history table: the second box type to flip (e.g. the inbox after the outbox) finds the table already created by the first, skips its seed, and the bootstrap path then stamps a fresh `bootstrap: detected at V_latest` row — re-running the migration ledger and breaking FR5. The table-level gate is therefore dropped. The seed runs whenever the resolved history schema differs from the backend default; the per-row `NOT EXISTS` guard makes a steady-state run a zero-row `INSERT … SELECT`, and the cost is that one round-trip per provision. The consequence for the "Cross-schema read permission" risk is that read access to the legacy table is needed for the lifetime of the `PerSchema` deployment, not just at flip time.
+
+4. **A failure to read the legacy history table throws `ConfigurationException`.** Silently absorbing it would let an empty seed through, and the next provision would bootstrap-stamp a fresh row, again breaking FR5; the throw rolls back the surrounding transaction so no partial per-schema state is left. The catch is filtered to the provider's permission-denied codes (PostgreSQL `42501`; SQL Server `229`/`230`), and wraps both the legacy-existence probe and the seed `INSERT … SELECT`, so a deadlock, dropped connection or timeout propagates unchanged rather than being misreported as "grant SELECT on the legacy table".
+
 ## References
 
 - Requirements: [specs/0029-multi-tenancy-migrations/requirements.md](../../specs/0029-multi-tenancy-migrations/requirements.md)

src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlAdvisoryLock.cs

@@ -39,41 +39,24 @@ namespace Paramore.Brighter.BoxProvisioning.PostgreSql;
 /// The deadline is measured against an injectable <see cref="TimeProvider"/>; the default
 /// <see cref="TimeProvider.System"/> reads <see cref="System.Diagnostics.Stopwatch.GetTimestamp"/>
 /// (monotonic), so a wall-clock jump (NTP correction during a long lock wait, leap-second
-/// smear, container clock skew on VM resume) cannot collapse or extend the budget. Tests
-/// inject a fake provider to drive the deadline check deterministically.
+/// smear, container clock skew on VM resume) cannot collapse or extend the budget.
 /// <para>
 /// Lock-key derivation: the per-call key (typically <c>BrighterMigration_&lt;schema&gt;.&lt;table&gt;</c>
-/// from the runner) is combined with the Brighter namespace constant into the composite
-/// <c>"74726:&lt;lockKey&gt;"</c>, hashed with SHA-256, and the first 8 bytes of the digest are read
-/// fixed big-endian (via <c>BinaryPrimitives.ReadInt64BigEndian</c>) into a signed <c>long</c>.
-/// That single 64-bit value is passed to the single-argument <c>pg_try_advisory_lock(bigint)</c> /
-/// <c>pg_advisory_unlock(bigint)</c> overloads; there is no <c>hashtext</c> call and the namespace
-/// is folded into the hash input rather than passed as a separate SQL argument. The birthday-bound
-/// collision probability across distinct lock keys is therefore ~1 in 2^64, hardened from the
-/// ~1 in 2^32 of the prior <c>hashtext → int4</c> scheme and aligned with the 64-bit SHA-256
-/// principle the MySQL long-form fallback in <c>MySqlMigrationLockName</c> follows. Any collision
-/// merely serialises two migrations on a shared advisory lock (correctness preserved, only the
-/// concurrency boundary widens), and the per-deployment population is typically &lt; 100 box tables.
-/// Fixing big-endian keeps the derived key identical on every host, OS, and culture (deterministic);
-/// see ADR 0062.
+/// from the runner) is prefixed with a Brighter namespace as <c>"74726:&lt;lockKey&gt;"</c>, hashed
+/// with SHA-256, and the first 8 bytes of the digest are read big-endian into the signed
+/// <c>bigint</c> passed to <c>pg_try_advisory_lock</c> / <c>pg_advisory_unlock</c>. Reading
+/// big-endian keeps the key identical on every host. A collision between two lock keys
+/// (about 1 in 2^64) only serialises two migrations on one lock. See ADR 0062.
 /// </para>
 /// </remarks>
 public class PostgreSqlAdvisoryLock : IPostgreSqlAdvisoryLock
 {
-    // Brighter-specific advisory-lock namespace, folded into the hash input by DeriveLockKey (the
-    // composite prefix "74726:<lockKey>") rather than passed as a separate SQL argument. The single
-    // 64-bit advisory-lock space is partitioned per box table by the lock-key string; the namespace
-    // distinguishes Brighter's locks from any other application using the same composite scheme. The
-    // numeric value is "BRIG" interpreted as ASCII (74726 decimal); not load-bearing — collision
-    // with another application's choice is diagnostic not correctness.
+    // Prefixed to every lock key before hashing, to keep Brighter's advisory locks apart from
+    // other applications' (ADR 0062).
     private const int BRIGHTER_LOCK_NAMESPACE = 74726;
 
     private readonly TimeProvider _timeProvider;
 
-    // Derives the 64-bit advisory-lock key for a Brighter lock key. The namespace is folded into a
-    // composite ("74726:<lockKey>"), hashed with SHA-256, and the first 8 bytes are read fixed
-    // big-endian into a signed long (ADR 0062). Both AcquireAsync and ReleaseAsync route through
-    // this single helper, so they always derive byte-identical keys for the same lockKey.
     private static long DeriveLockKey(string lockKey)
     {
         var composite = $"{BRIGHTER_LOCK_NAMESPACE}:{lockKey}";
@@ -85,8 +68,7 @@ private static long DeriveLockKey(string lockKey)
     /// Initialises a new <see cref="PostgreSqlAdvisoryLock"/>.
     /// </summary>
     /// <param name="timeProvider">Source of timestamps for the acquisition deadline. Defaults
-    /// to <see cref="TimeProvider.System"/> (monotonic). Tests pass
-    /// <c>FakeTimeProvider</c> from <c>Microsoft.Extensions.TimeProvider.Testing</c>.</param>
+    /// to <see cref="TimeProvider.System"/> (monotonic).</param>
     public PostgreSqlAdvisoryLock(TimeProvider? timeProvider = null)
     {
         _timeProvider = timeProvider ?? TimeProvider.System;

src/Paramore.Brighter.BoxProvisioning.PostgreSql/PostgreSqlBoxDetectionHelper.cs

@@ -55,14 +55,8 @@ public class PostgreSqlBoxDetectionHelper :
 
     private readonly ILogger _logger;
 
-    /// <summary>
-    /// Resolves and quotes the schema that physically holds the history table for a read: null
-    /// (Global default) → <c>"public"</c>; otherwise the configured SchemaName supplied by the
-    /// runner under <see cref="MigrationHistoryScope.PerSchema"/>. Folded via
-    /// <see cref="PgIdentifier.Quote"/> so it matches the case the runner created, after
-    /// <c>Identifiers.AssertSafe</c> validation since the result is interpolated into the query
-    /// text (no parameterisation of identifiers in SQL).
-    /// </summary>
+    // Null means the Global default, "public". Quoting folds the name to the case the runner
+    // created it with; it is validated first because it is interpolated into the SQL.
     private static string QuotedHistorySchema(string? historySchema)
     {
         var schema = historySchema ?? DefaultSchemaName;
@@ -72,10 +66,7 @@ private static string QuotedHistorySchema(string? historySchema)
 
     /// <summary>
     /// Initialises the detection helper with an optional logger. When unspecified, falls back
-    /// to <see cref="ApplicationLogging.CreateLogger{T}"/>. Existing callers that use the
-    /// parameterless form continue to work — the logger is currently only consumed for the
-    /// rare <c>UndefinedTable</c>-swallow Debug emission in
-    /// <see cref="DoesHistoryExistAsync"/>; the helper remains a safe DI singleton.
+    /// to <see cref="ApplicationLogging.CreateLogger{T}"/>.
     /// </summary>
     public PostgreSqlBoxDetectionHelper(ILogger? logger = null)
     {
@@ -85,7 +76,7 @@ public PostgreSqlBoxDetectionHelper(ILogger? logger = null)
     /// <summary>
     /// Returns true if a table with the given name exists in the given schema.
     /// </summary>
-    /// <param name="schemaName">Optional. Null is substituted with <c>"public"</c> per ADR 0057 §A.1.</param>
+    /// <param name="schemaName">Optional. Null is substituted with <c>"public"</c>.</param>
     public async Task<bool> DoesTableExistAsync(
         NpgsqlConnection connection, string tableName, string? schemaName,
         CancellationToken cancellationToken = default,
@@ -96,18 +87,9 @@ public async Task<bool> DoesTableExistAsync(
         command.CommandText = @"
 SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
 WHERE TABLE_SCHEMA = @SchemaName AND TABLE_NAME = @TableName)";
-        // information_schema.tables stores PG-folded (lowercase) names. Normalize so a
-        // mixed-case configured value (e.g. default "Outbox") matches the stored "outbox".
-        // The internal __BrighterMigrationHistory system-table existence check does NOT
-        // route through this method — its CREATE DDL quotes the name and therefore
-        // case-preserves it in pg_class, so DoesHistoryExistAsync inlines a literal-name
-        // lookup that bypasses the fold normalization done here.
         command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
         command.Parameters.AddWithValue("@TableName", PgIdentifier.Normalize(tableName));
 
-        // Pattern-match rather than (bool)raw! so a driver returning null surfaces as a named
-        // InvalidOperationException instead of a bare NullReferenceException — Npgsql has
-        // returned null from ExecuteScalarAsync under server-side errors in the wild.
         var raw = await command.ExecuteScalarAsync(cancellationToken);
         return raw is bool b
             ? b
@@ -120,47 +102,25 @@ SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
     /// given box table.
     /// </summary>
     /// <param name="schemaName">Optional. The box-table schema used to filter history rows. Null
-    /// is substituted with <c>"public"</c> per ADR 0057 §A.1.</param>
+    /// is substituted with <c>"public"</c>.</param>
     /// <param name="historySchema">Optional. The physical schema holding the history table; null
-    /// resolves to <c>"public"</c> (today's behaviour).</param>
+    /// resolves to <c>"public"</c>, the Global default.</param>
     public async Task<bool> DoesHistoryExistAsync(
         NpgsqlConnection connection, string tableName, string? schemaName, string? historySchema,
         CancellationToken cancellationToken = default,
         NpgsqlTransaction? transaction = null)
     {
-        // The physical history schema: Global → "public" (today's behaviour); PerSchema → the
-        // configured SchemaName passed by the runner. Both sides fold the same way (Normalize for
-        // the information_schema param, Quote for the qualified table ref) so a mixed-case configured
-        // value reaches the same physical schema the runner created.
         var foldedHistorySchema = PgIdentifier.Normalize(historySchema ?? DefaultSchemaName);
         var quotedHistorySchema = QuotedHistorySchema(historySchema);
 
-        // System-table existence: __BrighterMigrationHistory is created via quoted DDL
-        // (`CREATE TABLE IF NOT EXISTS <schema>."__BrighterMigrationHistory"`), so its name
-        // is case-PRESERVED in pg_class. Routing through DoesTableExistAsync would lowercase
-        // the lookup to `'__brightermigrationhistory'` and miss. Inline the literal-name check,
-        // parameterising only the (folded) schema.
-        using (var existsCmd = connection.CreateCommand())
-        {
-            if (transaction != null) existsCmd.Transaction = transaction;
-            existsCmd.CommandText = @"
-SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
-WHERE TABLE_SCHEMA = @HistorySchema AND TABLE_NAME = '__BrighterMigrationHistory')";
-            existsCmd.Parameters.AddWithValue("@HistorySchema", foldedHistorySchema);
-            var existsRaw = await existsCmd.ExecuteScalarAsync(cancellationToken);
-            var historyTableExists = existsRaw is bool b && b;
-            if (!historyTableExists)
-                return false;
-        }
+        if (!await HistoryTableExistsAsync(connection, foldedHistorySchema, cancellationToken, transaction))
+            return false;
 
         using var command = connection.CreateCommand();
         if (transaction != null) command.Transaction = transaction;
         command.CommandText = $@"
 SELECT COUNT(1) FROM {quotedHistorySchema}.""__BrighterMigrationHistory""
 WHERE ""BoxTableName"" = @BoxTableName AND ""SchemaName"" = @SchemaName";
-        // History rows are stored with PG-folded (lowercase) identifiers by the runner so
-        // that lookups remain consistent across mixed-case configured values; normalize
-        // here too. See PostgreSqlBoxMigrationRunner.InsertHistory.
         command.Parameters.AddWithValue("@BoxTableName", PgIdentifier.Normalize(tableName));
         command.Parameters.AddWithValue("@SchemaName", PgIdentifier.Normalize(schemaName ?? DefaultSchemaName));
 
@@ -175,39 +135,42 @@ SELECT COUNT(1) FROM {quotedHistorySchema}.""__BrighterMigrationHistory""
         }
         catch (PostgresException ex) when (ex.SqlState == PostgresErrorCodes.UndefinedTable)
         {
-            // TOCTOU: another connection dropped __BrighterMigrationHistory between our
-            // existence check and this count query. In production this cannot happen — the
-            // history table is created once and never dropped — but parallel tests do drop
-            // it deliberately, and a "table dropped between two queries" outcome is
-            // semantically equivalent to "no history". Returning false lets the caller fall
-            // through to the runner, which re-creates the history table under its lock.
-            //
-            // Per PR #4039 reviewer Nit (item 4485697019): Debug-level log so the swallow is
-            // observable if a future contributor adds a DROP-history operation outside test
-            // setup. Production should never see this; if it does, the deployment has a
-            // history-table-management bug worth surfacing.
             _logger.LogDebug(
                 ex,
                 "PostgreSqlBoxDetectionHelper.DoesHistoryExistAsync: swallowed UndefinedTable on COUNT(1) after positive existence check — __BrighterMigrationHistory was dropped between the two queries. Returning false. In production this indicates a history-table-management bug; in parallel tests it is expected because tests drop the table deliberately.");
             return false;
         }
     }
 
+    // The history table is created with a quoted name, so its case is preserved; this cannot use
+    // DoesTableExistAsync, which folds the name to lowercase.
+    private static async Task<bool> HistoryTableExistsAsync(
+        NpgsqlConnection connection, string foldedHistorySchema,
+        CancellationToken cancellationToken, NpgsqlTransaction? transaction)
+    {
+        using var command = connection.CreateCommand();
+        if (transaction != null) command.Transaction = transaction;
+        command.CommandText = @"
+SELECT EXISTS(SELECT 1 FROM INFORMATION_SCHEMA.TABLES
+WHERE TABLE_SCHEMA = @HistorySchema AND TABLE_NAME = '__BrighterMigrationHistory')";
+        command.Parameters.AddWithValue("@HistorySchema", foldedHistorySchema);
+        var raw = await command.ExecuteScalarAsync(cancellationToken);
+        return raw is bool exists && exists;
+    }
+
     /// <summary>
     /// Returns the highest migration version recorded in history for the given box table,
     /// or 0 if no rows exist.
     /// </summary>
     /// <param name="schemaName">Optional. The box-table schema used to filter history rows. Null
-    /// is substituted with <c>"public"</c> per ADR 0057 §A.1.</param>
+    /// is substituted with <c>"public"</c>.</param>
     /// <param name="historySchema">Optional. The physical schema holding the history table; null
-    /// resolves to <c>"public"</c> (today's behaviour).</param>
+    /// resolves to <c>"public"</c>, the Global default.</param>
     public async Task<int> GetMaxVersionAsync(
         NpgsqlConnection connection, string tableName, string? schemaName, string? historySchema,
         CancellationToken cancellationToken = default,
         NpgsqlTransaction? transaction = null)
     {
-        // Read max version from the same physical history schema the runner writes to: Global →
-        // "public"; PerSchema → the configured SchemaName, folded identically via PgIdentifier.Quote.
         var quotedHistorySchema = QuotedHistorySchema(historySchema);
         using var command = connection.CreateCommand();
         if (transaction != null) command.Transaction = transaction;
@@ -228,7 +191,7 @@ SELECT COALESCE(MAX(""MigrationVersion""), 0) FROM {quotedHistorySchema}.""__Bri
     /// Reads the column name set for the given table from <c>information_schema.columns</c>.
     /// Comparison is case-sensitive; PostgreSQL returns unquoted identifiers folded to lowercase.
     /// </summary>
-    /// <param name="schemaName">Optional. Null is substituted with <c>"public"</c> per ADR 0057 §A.1.</param>
+    /// <param name="schemaName">Optional. Null is substituted with <c>"public"</c>.</param>
     public async Task<IReadOnlyCollection<string>> GetTableColumnsAsync(
         NpgsqlConnection connection, string tableName, string? schemaName,
         CancellationToken cancellationToken = default,
@@ -248,7 +211,7 @@ public async Task<IReadOnlyCollection<string>> GetTableColumnsAsync(
     /// </list>
     /// </summary>
     /// <param name="boxType">Selects the discriminator: <c>headerbag</c> for outbox, <c>commandbody</c> for inbox.</param>
-    /// <param name="schemaName">Optional. Null is substituted with <c>"public"</c> per ADR 0057 §A.1.</param>
+    /// <param name="schemaName">Optional. Null is substituted with <c>"public"</c>.</param>
     public async Task<int> DetectCurrentVersionAsync(
         NpgsqlConnection connection, string tableName, string? schemaName,
         BoxType boxType, IReadOnlyList<IAmABoxMigration> migrations,