fix(controlplane): validate and compose a deployment from ONE snapshot read

StartDeployment read the whole config DB twice with no transaction — DraftSnapshotFactory for the validator, then ConfigComposer for the artifact, plus clusters/nodes and Script.SourceCode again in between. An AdminUI save landing between the two shipped an artifact the DraftValidator never saw. - DraftSnapshot carries ServerClusters, ClusterNodes and ApiKeys; DraftSnapshotFactory reads every table inside one SNAPSHOT transaction on a relational provider (no-op on InMemory or under an ambient transaction). - ConfigComposer.Compose(DraftSnapshot) is pure over the snapshot (ordinal-sorted keys); SnapshotAndFlattenAsync(db) delegates to it, byte-identical. - AdminOperationsActor validates topology, estimates compile cost and composes the artifact from the one snapshot it holds. - Migration EnableSnapshotIsolation: ALTER DATABASE CURRENT SET ALLOW_SNAPSHOT_ISOLATION ON (suppressTransaction; no model change). Tests: the SQL Server snapshot read ignores a row committed mid-read (positive control: a later read sees it); an edit injected between the Equipment and Tags reads is absent from the shipped artifact and rejected by the next deploy; Compose(snapshot) == SnapshotAndFlattenAsync(db). Refs #733 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 06:36:12 -04:00
parent b437c34f92
commit 73e74e270d
10 changed files with 2077 additions and 36 deletions
@@ -0,0 +1,36 @@
+using Microsoft.EntityFrameworkCore.Migrations;
+
+#nullable disable
+
+namespace ZB.MOM.WW.OtOpcUa.Configuration.Migrations
+{
+    /// <summary>
+    ///     Enables <c>ALLOW_SNAPSHOT_ISOLATION</c> on the config database so the deploy path can read every
+    ///     table inside one <c>SNAPSHOT</c> transaction (<c>DraftSnapshotFactory.FromConfigDbAsync</c>):
+    ///     the validator and the artifact composer then see one consistent state instead of two reads
+    ///     an AdminUI save can land between. No schema change — the model snapshot is untouched.
+    /// </summary>
+    /// <remarks>
+    ///     <c>ALTER DATABASE</c> cannot run inside the user transaction the migrator wraps each migration in,
+    ///     hence <c>suppressTransaction: true</c>. Turning the option ON does not need exclusive access (unlike
+    ///     <c>READ_COMMITTED_SNAPSHOT</c>); it waits for in-flight transactions and then takes effect. Down
+    ///     leaves it ON: a database that has run with it is not harmed by keeping it, and switching it OFF
+    ///     would fail any deploy running at that instant.
+    /// </remarks>
+    public partial class EnableSnapshotIsolation : Migration
+    {
+        /// <inheritdoc />
+        protected override void Up(MigrationBuilder migrationBuilder)
+        {
+            migrationBuilder.Sql(
+                "ALTER DATABASE CURRENT SET ALLOW_SNAPSHOT_ISOLATION ON;",
+                suppressTransaction: true);
+        }
+
+        /// <inheritdoc />
+        protected override void Down(MigrationBuilder migrationBuilder)
+        {
+            // Deliberately a no-op; see the class remarks.
+        }
+    }
+}
@@ -66,6 +66,23 @@ public sealed class DraftSnapshot
    /// silently dropped when the permission trie is built, so the grant has to be caught here instead.</summary>
    public IReadOnlyList<NodeAcl> NodeAcls { get; init; } = [];

+    /// <summary>
+    ///     Cluster rows. Not read by the row-level rules; carried so the deploy path validates the cluster
+    ///     topology (<see cref="DraftValidator.ValidateClusterTopology"/>) and composes the artifact from
+    ///     the SAME read as every other table — a second read between validate and compose is exactly how
+    ///     an edit ships unvalidated.
+    /// </summary>
+    public IReadOnlyList<ServerCluster> ServerClusters { get; init; } = [];
+
+    /// <summary>Cluster-node rows; see <see cref="ServerClusters"/> for why they ride the snapshot.</summary>
+    public IReadOnlyList<ClusterNode> ClusterNodes { get; init; } = [];
+
+    /// <summary>
+    ///     Data-plane API keys (peppered HMAC only, never a secret). Not validated; carried so the artifact
+    ///     composer ships the keys a driver-only node authenticates against from the one consistent read.
+    /// </summary>
+    public IReadOnlyList<ApiKey> ApiKeys { get; init; } = [];
+
    /// <summary>Prior Equipment rows (any generation, same cluster) for stability checks.</summary>
    public IReadOnlyList<Equipment> PriorEquipment { get; init; } = [];

@@ -1,3 +1,4 @@
+using System.Data;
 using Microsoft.EntityFrameworkCore;

 namespace ZB.MOM.WW.OtOpcUa.Configuration.Validation;
@@ -21,6 +22,16 @@ namespace ZB.MOM.WW.OtOpcUa.Configuration.Validation;
 ///         / <see cref="DraftSnapshot.Site"/> are left null so the path-length rule uses its
 ///         conservative upper bound.
 ///     </para>
+///     <para>
+///         <b>One consistent read.</b> On a relational provider the tables are read inside a
+///         <see cref="IsolationLevel.Snapshot"/> transaction, so a <c>/uns</c> or <c>/raw</c> save that
+///         lands mid-read cannot leave half the tables on the new state and half on the old. The deploy path
+///         validates AND composes its artifact from this one snapshot, which is what makes "the artifact the
+///         validator never saw" unrepresentable. SQL Server requires <c>ALLOW_SNAPSHOT_ISOLATION ON</c>
+///         (migration <c>EnableSnapshotIsolation</c>); without it the first query fails with error 3952 and
+///         the deploy is rejected rather than silently read unisolated. A non-relational provider (the
+///         InMemory test provider) and a context already inside a transaction read without one.
+///     </para>
 /// </remarks>
 public static class DraftSnapshotFactory
 {
@@ -29,6 +40,23 @@ public static class DraftSnapshotFactory
    /// <param name="ct">Cancellation token.</param>
    /// <returns>A snapshot populated from the live DB, ready for <see cref="DraftValidator.Validate"/>.</returns>
    public static async Task<DraftSnapshot> FromConfigDbAsync(OtOpcUaConfigDbContext db, CancellationToken ct = default)
+    {
+        ArgumentNullException.ThrowIfNull(db);
+
+        var ownsTransaction = db.Database.IsRelational() && db.Database.CurrentTransaction is null;
+        var tx = ownsTransaction
+            ? await db.Database.BeginTransactionAsync(IsolationLevel.Snapshot, ct).ConfigureAwait(false)
+            : null;
+        await using (tx)
+        {
+            var snapshot = await ReadAsync(db, ct).ConfigureAwait(false);
+            // Read-only: committing releases the snapshot's version-store reservation; nothing was written.
+            if (tx is not null) await tx.CommitAsync(ct).ConfigureAwait(false);
+            return snapshot;
+        }
+    }
+
+    private static async Task<DraftSnapshot> ReadAsync(OtOpcUaConfigDbContext db, CancellationToken ct)
        => new DraftSnapshot
        {
            // GenerationId=0 and null Enterprise/Site are intentional conservative fallbacks at the
@@ -36,6 +64,8 @@ public static class DraftSnapshotFactory
            // EquipmentUuid checks run in separate validator passes — no rule here reads these fields.
            GenerationId = 0,            // generation model dropped; placeholder (no rule reads it)
            ClusterId = string.Empty,    // global snapshot; rules compare entity ClusterId fields, not this
+            ServerClusters     = await db.ServerClusters.AsNoTracking().ToListAsync(ct),
+            ClusterNodes       = await db.ClusterNodes.AsNoTracking().ToListAsync(ct),
            // v3: Namespace entity retired — no namespace list to materialize. The Raw tree
            // (RawFolders/TagGroups) + UnsTagReferences feed the v3 rules (charset, tagname length,
            // effective-leaf uniqueness).
@@ -53,6 +83,7 @@ public static class DraftSnapshotFactory
            Scripts            = await db.Scripts.AsNoTracking().ToListAsync(ct),
            PollGroups         = await db.PollGroups.AsNoTracking().ToListAsync(ct),
            NodeAcls           = await db.NodeAcls.AsNoTracking().ToListAsync(ct),
+            ApiKeys            = await db.ApiKeys.AsNoTracking().ToListAsync(ct),
            PriorEquipment     = [],   // intentional: no prior-generation table to diff against at this level
            ActiveReservations = await db.ExternalIdReservations
                .AsNoTracking()