From 0287f8f64fd0189fccb2d56d6861d95cf411536d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 14 Sep 2026 13:51:34 -0400 Subject: [PATCH] fix(deploy): persist the artifact deployment record before the best-effort site stamp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../ArtifactDeploymentService.cs | 109 ++++++++++++------ .../ArtifactRevisionHashService.cs | 40 +++++-- .../ArtifactDeploymentServiceTests.cs | 17 ++- .../ArtifactRevisionHashServiceTests.cs | 33 ++++++ 4 files changed, 156 insertions(+), 43 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs index 9f312dc0..27e64bb6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactDeploymentService.cs +++ b/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); - if (site is not null) - { - await StampArtifactRevisionAsync([(site, command)], cancellationToken); - } + await StampRetriedSiteAsync(siteDbId, command, cancellationToken); } - await _auditService.LogAsync(user, "RetryArtifactDeployment", "SystemArtifact", - command.DeploymentId, siteIdentifier, new { response.Success }, cancellationToken); - return response.Success ? Result.Success(result) : Result.Failure(response.ErrorMessage ?? "Retry failed."); @@ -486,6 +493,11 @@ public class ArtifactDeploymentService /// which is harmless because 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. + /// + /// 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. + /// /// public async Task> ComputeCurrentArtifactHashesAsync( IReadOnlyList sites, @@ -508,6 +520,37 @@ public class ArtifactDeploymentService return hashes; } + /// + /// 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 's catch and report a + /// successful artifact deploy as a failure. + /// + 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); + } + /// /// Stamps and /// for every (site, deployed command) pair and flushes them in one save. Best-effort: a diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactRevisionHashService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactRevisionHashService.cs index be55a8be..a22d31a2 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/ArtifactRevisionHashService.cs +++ b/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. + /// + /// The name property is matched CASE-INSENSITIVELY. Today's producer + /// (ArtifactDeploymentService.FetchGlobalArtifactsAsync) serializes an anonymous + /// type, so the key is PascalCase Name — 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. + /// /// 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 => - element.ValueKind == JsonValueKind.Object - && element.TryGetProperty("Name", out var name) - && name.ValueKind == JsonValueKind.String - ? name.GetString() ?? string.Empty - : string.Empty, - StringComparer.Ordinal) + .OrderBy(MethodSortKey, StringComparer.Ordinal) .ToList(); return JsonSerializer.Serialize(ordered, CanonicalJsonOptions); @@ -167,6 +168,31 @@ public sealed class ArtifactRevisionHashService } } + /// + /// 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. + /// + 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. diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs index e08b7d46..a4cffe83 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactDeploymentServiceTests.cs +++ b/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 - // failure to record the stamp is logged and swallowed, never reported as a failure. + // ORDERING RULE: audit row first, best-effort stamp after. Both repositories are + // 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()).Returns(new List { site }); _siteRepo.SaveChangesAsync(Arg.Any()) @@ -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(), Arg.Any()); + await _deploymentRepo.Received(1).SaveChangesAsync(Arg.Any()); + Received.InOrder(() => + { + _deploymentRepo.SaveChangesAsync(Arg.Any()); + _siteRepo.UpdateSiteAsync(site, Arg.Any()); + }); } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactRevisionHashServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactRevisionHashServiceTests.cs index aa9463a8..a2d0316a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/ArtifactRevisionHashServiceTests.cs +++ b/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() {