fix(deploy): persist the artifact deployment record before the best-effort site stamp

Both repositories are scoped over one DbContext, so a failed stamp save left the
Site entries tracked as Modified and the deployment record's (uncaught)
SaveChangesAsync rethrew it — losing the audit row and failing a deploy in which
every site succeeded. Ordering rule now: audit row first, stamp after, inside its
own try/catch. Same in RetryForSiteAsync, where the site load is caught too so
nothing on the stamp path can reach the method's catch.

Also: the method-definitions canonicaliser matches the name property
case-insensitively (a naming-policy change at the producer would otherwise
silently turn the sort into a no-op), and ComputeCurrentArtifactHashesAsync
documents that a per-site query failure aborts the whole batch by design.

Claude-Session: https://claude.ai/code/session_01Y5ef34iUUMjmwnMpL9xgmB

src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs

@@ -190,6 +190,11 @@ public class ArtifactDeploymentService
         foreach (var es in externalSystems)
         {
             var methods = await _externalSystemRepo.GetMethodsByExternalSystemIdAsync(es.Id, cancellationToken);
+            // The property NAMES emitted here are part of a contract:
+            // ArtifactRevisionHashService.CanonicalizeMethodDefinitions re-orders this array by
+            // each element's "Name" property (case-insensitively) so repository row order cannot
+            // move a site's artifact revision hash. Renaming Name here would silently degrade
+            // that canonicalisation to "leave the order alone".
             var methodsJson = methods.Count > 0
                 ? JsonSerializer.Serialize(methods.Select(m => new
                 {
@@ -361,31 +366,6 @@ public class ArtifactDeploymentService
             perSiteResults[result.SiteId] = result;
         }
 
-        // Stamp the deployed artifact revision on every site whose deploy SUCCEEDED.
-        // A failed site is deliberately left untouched so it keeps its previous stamp
-        // (or none at all) and keeps reading "out of date" on the Sites page.
-        //
-        // The stamp is flushed here, BEFORE the deployment record is added, through
-        // ISiteRepository's own SaveChangesAsync. ISiteRepository and
-        // IDeploymentManagerRepository are both scoped over the same DbContext, so a
-        // single save would in fact commit both — but keeping the stamp save separate
-        // and ahead of the record is what lets a stamp failure be swallowed (logged at
-        // Warning) without taking the deployment record's save down with it. Audit
-        // trail beats staleness bookkeeping: a stamp failure must never turn a
-        // successful deploy into a reported failure. (A failed SaveChanges does leave
-        // the site entities tracked as modified on the shared context, so the record's
-        // save may re-attempt them — it cannot make the stamp any more wrong than the
-        // failure already did.)
-        var successfulSites = sites
-            .Where(site => perSiteResults.TryGetValue(site.SiteIdentifier, out var r) && r.Success)
-            .ToList();
-        if (successfulSites.Count > 0)
-        {
-            await StampArtifactRevisionAsync(
-                successfulSites.Select(site => (site, siteCommands[site.Id])).ToList(),
-                cancellationToken);
-        }
-
         // Persist the system artifact deployment record.
         // SystemArtifactDeploymentRecord has no dedicated
         // DeploymentId column (adding one is a Commons/ConfigurationDatabase
@@ -404,6 +384,31 @@ public class ArtifactDeploymentService
         await _deploymentRepo.AddSystemArtifactDeploymentAsync(record, cancellationToken);
         await _deploymentRepo.SaveChangesAsync(cancellationToken);
 
+        // ORDERING RULE: audit row first; the stamp is best-effort bookkeeping and its
+        // save failure must not be able to reach the record's save.
+        //
+        // ISiteRepository and IDeploymentManagerRepository are scoped over the SAME
+        // DbContext, so a failed stamp save leaves the Site entries tracked as Modified.
+        // Stamping first would therefore let a persistent stamp failure (concurrency
+        // conflict, constraint violation) rethrow out of the record's own — uncaught —
+        // SaveChangesAsync: the deployment record would be lost and a wholly successful
+        // deploy would surface as an exception. Running the record's save first and the
+        // stamp after keeps the failure inside StampArtifactRevisionAsync's try/catch,
+        // where it is logged at Warning and the deploy still reports Success.
+        //
+        // Only sites whose deploy SUCCEEDED are stamped. A failed site is deliberately
+        // left untouched so it keeps its previous stamp (or none at all) and keeps
+        // reading "out of date" on the Sites page.
+        var successfulSites = sites
+            .Where(site => perSiteResults.TryGetValue(site.SiteIdentifier, out var r) && r.Success)
+            .ToList();
+        if (successfulSites.Count > 0)
+        {
+            await StampArtifactRevisionAsync(
+                successfulSites.Select(site => (site, siteCommands[site.Id])).ToList(),
+                cancellationToken);
+        }
+
         var summary = new ArtifactDeploymentSummary(
             deploymentId,
             results.ToList(),
@@ -442,21 +447,23 @@ public class ArtifactDeploymentService
 
             var result = new SiteArtifactResult(siteIdentifier, siteIdentifier, response.Success, response.ErrorMessage);
 
+            await _auditService.LogAsync(user, "RetryArtifactDeployment", "SystemArtifact",
+                command.DeploymentId, siteIdentifier, new { response.Success }, cancellationToken);
+
             // Same stamp-on-success rule as the sweep path: a retry that lands is a
             // successful artifact deploy and must move the site's revision stamp; a
             // retry that fails leaves the previous stamp in place.
+            //
+            // Same ORDERING RULE too — audit row first, best-effort stamp after. Here the
+            // method's own catch would convert a stamp exception into a Failure result for
+            // an artifact deploy that actually succeeded, so the stamp must (and does) keep
+            // its own try/catch inside StampArtifactRevisionAsync rather than lean on this
+            // one.
             if (response.Success)
             {
-                var site = await _siteRepo.GetSiteByIdAsync(siteDbId, cancellationToken);
+                await StampRetriedSiteAsync(siteDbId, command, cancellationToken);
-                if (site is not null)
-                {
-                    await StampArtifactRevisionAsync([(site, command)], cancellationToken);
-                }
             }
 
-            await _auditService.LogAsync(user, "RetryArtifactDeployment", "SystemArtifact",
-                command.DeploymentId, siteIdentifier, new { response.Success }, cancellationToken);
-
             return response.Success
                 ? Result<SiteArtifactResult>.Success(result)
                 : Result<SiteArtifactResult>.Failure(response.ErrorMessage ?? "Retry failed.");
@@ -486,6 +493,11 @@ public class ArtifactDeploymentService
     /// which is harmless because <see cref="ArtifactRevisionHashService"/> excludes both the
     /// deployment id and the command timestamp from the hash — that exclusion is what makes a
     /// computed-now hash comparable with one stamped by a past deploy.
+    /// <para>
+    /// A failing per-site data-connection query aborts the WHOLE batch by design: a partial map
+    /// would silently report unqueried sites as up to date. The pending-updates cache discards a
+    /// faulted flight and the UI shows the aggregate as unavailable instead.
+    /// </para>
     /// </remarks>
     public async Task<IReadOnlyDictionary<int, string>> ComputeCurrentArtifactHashesAsync(
         IReadOnlyList<Site> sites,
@@ -508,6 +520,37 @@ public class ArtifactDeploymentService
         return hashes;
     }
 
+    /// <summary>
+    /// Loads the retried site and stamps the artifact revision it just accepted. Best-effort
+    /// end to end: the load is caught here and the stamp carries its own catch, so nothing on
+    /// this path can escape into <see cref="RetryForSiteAsync"/>'s catch and report a
+    /// successful artifact deploy as a failure.
+    /// </summary>
+    private async Task StampRetriedSiteAsync(
+        int siteDbId,
+        DeployArtifactsCommand command,
+        CancellationToken cancellationToken)
+    {
+        Site? site;
+        try
+        {
+            site = await _siteRepo.GetSiteByIdAsync(siteDbId, cancellationToken);
+        }
+        catch (Exception ex)
+        {
+            _logger.LogWarning(ex,
+                "Artifact retry for site {SiteDbId} succeeded but the site row could not be loaded to " +
+                "stamp its artifact revision; it will report out-of-date artifacts until the next deploy",
+                siteDbId);
+            return;
+        }
+
+        if (site is null)
+            return;
+
+        await StampArtifactRevisionAsync([(site, command)], cancellationToken);
+    }
+
     /// <summary>
     /// Stamps <see cref="Site.ArtifactRevisionHash"/> and <see cref="Site.ArtifactsDeployedAt"/>
     /// for every (site, deployed command) pair and flushes them in one save. Best-effort: a

src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactRevisionHashService.cs

@@ -135,6 +135,13 @@ public sealed class ArtifactRevisionHashService
     /// by name (ordinal), so the repository's row order cannot leak into the hash. Anything
     /// that is not a parseable JSON array is returned verbatim — the hash still changes when
     /// the payload does, which is the only guarantee this method owes.
+    /// <para>
+    /// The name property is matched CASE-INSENSITIVELY. Today's producer
+    /// (<c>ArtifactDeploymentService.FetchGlobalArtifactsAsync</c>) serializes an anonymous
+    /// type, so the key is PascalCase <c>Name</c> — but a future camelCase naming policy there
+    /// would otherwise silently turn this sort into a no-op and let row order move the hash
+    /// again, with nothing failing.
+    /// </para>
     /// </summary>
     private static string? CanonicalizeMethodDefinitions(string? methodDefinitionsJson)
     {
@@ -150,13 +157,7 @@ public sealed class ArtifactRevisionHashService
             var ordered = document.RootElement
                 .EnumerateArray()
                 .Select(element => element.Clone())
-                .OrderBy(element =>
+                .OrderBy(MethodSortKey, StringComparer.Ordinal)
-                    element.ValueKind == JsonValueKind.Object
-                    && element.TryGetProperty("Name", out var name)
-                    && name.ValueKind == JsonValueKind.String
-                        ? name.GetString() ?? string.Empty
-                        : string.Empty,
-                    StringComparer.Ordinal)
                 .ToList();
 
             return JsonSerializer.Serialize(ordered, CanonicalJsonOptions);
@@ -167,6 +168,31 @@ public sealed class ArtifactRevisionHashService
         }
     }
 
+    /// <summary>
+    /// Sort key for one serialized external-system method: its name property, matched
+    /// case-insensitively so the key survives a naming-policy change at the producer.
+    /// Elements that are not objects, or carry no string name, sort first under an empty key —
+    /// their relative order is then whatever the payload gave, which is the best a nameless
+    /// element allows.
+    /// </summary>
+    private static string MethodSortKey(JsonElement element)
+    {
+        if (element.ValueKind != JsonValueKind.Object)
+            return string.Empty;
+
+        foreach (var property in element.EnumerateObject())
+        {
+            if (string.Equals(property.Name, "Name", StringComparison.OrdinalIgnoreCase))
+            {
+                return property.Value.ValueKind == JsonValueKind.String
+                    ? property.Value.GetString() ?? string.Empty
+                    : string.Empty;
+            }
+        }
+
+        return string.Empty;
+    }
+
     // Internal types for deterministic serialization. Properties MUST be declared in
     // alphabetical (ordinal) order — System.Text.Json emits them in declaration order
     // and does not sort. See the DETERMINISM CONTRACT note on the class summary.

tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs

@@ -354,10 +354,13 @@ public class ArtifactDeploymentServiceTests : TestKit
     }
 
     [Fact]
-    public async Task DeployToAllSitesAsync_StampSaveThrows_DeployStillReportsSuccess()
+    public async Task DeployToAllSitesAsync_StampSaveThrows_DeploymentRecordStillPersistedAndSuccessReported()
     {
-        // Audit trail beats staleness bookkeeping: the artifacts DID reach the site, so a
+        // ORDERING RULE: audit row first, best-effort stamp after. Both repositories are
-        // failure to record the stamp is logged and swallowed, never reported as a failure.
+        // scoped over ONE DbContext, so a failed stamp save leaves the Site entries tracked
+        // as Modified — stamping first would let that failure rethrow out of the deployment
+        // record's own (uncaught) SaveChangesAsync and lose the audit row for a deploy where
+        // every site succeeded. The record must therefore be saved BEFORE the stamp is tried.
         var site = new Site("Site One", "site-1") { Id = 1 };
         _siteRepo.GetAllSitesAsync(Arg.Any<CancellationToken>()).Returns(new List<Site> { site });
         _siteRepo.SaveChangesAsync(Arg.Any<CancellationToken>())
@@ -370,6 +373,14 @@ public class ArtifactDeploymentServiceTests : TestKit
 
         Assert.True(result.IsSuccess);
         Assert.Equal(1, result.Value.SuccessCount);
+        await _deploymentRepo.Received(1).AddSystemArtifactDeploymentAsync(
+            Arg.Any<SystemArtifactDeploymentRecord>(), Arg.Any<CancellationToken>());
+        await _deploymentRepo.Received(1).SaveChangesAsync(Arg.Any<CancellationToken>());
+        Received.InOrder(() =>
+        {
+            _deploymentRepo.SaveChangesAsync(Arg.Any<CancellationToken>());
+            _siteRepo.UpdateSiteAsync(site, Arg.Any<CancellationToken>());
+        });
     }
 
     [Fact]

tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactRevisionHashServiceTests.cs

@@ -103,6 +103,39 @@ public class ArtifactRevisionHashServiceTests
         Assert.Equal(_sut.ComputeHash(a), _sut.ComputeHash(b));
     }
 
+    [Fact]
+    public void ComputeHash_ReorderedCamelCaseMethodNames_ProducesSameHash()
+    {
+        // The method-name lookup is case-insensitive so that a naming-policy change at the
+        // producer (PascalCase anonymous type today) cannot silently degrade the sort into a
+        // no-op and let repository row order move the hash again.
+        var a = Command(externalSystems:
+        [
+            new ExternalSystemArtifact("Mes", "https://mes", "ApiKey", null,
+                "[{\"name\":\"MoveOut\"},{\"name\":\"MoveIn\"}]")
+        ]);
+        var b = Command(externalSystems:
+        [
+            new ExternalSystemArtifact("Mes", "https://mes", "ApiKey", null,
+                "[{\"name\":\"MoveIn\"},{\"name\":\"MoveOut\"}]")
+        ]);
+
+        Assert.Equal(_sut.ComputeHash(a), _sut.ComputeHash(b));
+    }
+
+    [Fact]
+    public void ComputeHash_UnparseableMethodDefinitions_StillParticipatesInTheHash()
+    {
+        // Non-array / non-JSON payloads pass through verbatim: the order guarantee is lost
+        // (there is nothing to sort), but a content change must still move the hash.
+        var a = Command(externalSystems:
+            [new ExternalSystemArtifact("Mes", "https://mes", "ApiKey", null, "not json")]);
+        var b = Command(externalSystems:
+            [new ExternalSystemArtifact("Mes", "https://mes", "ApiKey", null, "not json either")]);
+
+        Assert.NotEqual(_sut.ComputeHash(a), _sut.ComputeHash(b));
+    }
+
     [Fact]
     public void ComputeHash_DifferentDeploymentIdAndTimestamp_ProducesSameHash()
     {