From 721a2e595f4bc7f99e68463bcc2b95276c1d154c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 9 Jul 2026 16:12:56 -0400 Subject: [PATCH] fix(deployment): audit-write fault can no longer flip a committed Success deployment to Failed Route all post-terminal-status lifecycle audit writes (Deploy/Disable/Enable/Delete/DeleteOrphaned) through a swallow-and-warn TryLogAuditAsync helper so a failed best-effort audit write can neither reach the outer catch (flipping Success to Failed) nor propagate to the caller. Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj --- .../DeploymentService.cs | 46 ++++++++++++++++--- .../DeploymentServiceTests.cs | 30 ++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index a7fbfa62..3e405507 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -326,8 +326,11 @@ public class DeploymentService forceEnabledState: true, cancellationToken); } - // Audit log - await _auditService.LogAsync(user, "Deploy", "Instance", instanceId.ToString(), + // Audit log. Best-effort and post-terminal: the deployment record's + // status is already committed above, so a failed audit write must NOT + // fall through to the outer catch and flip a committed Success back to + // Failed (audit is best-effort — the action's own result is authoritative). + await TryLogAuditAsync(user, "Deploy", "Instance", instanceId.ToString(), instance.UniqueName, new { DeploymentId = deploymentId, Status = record.Status.ToString() }, cancellationToken); @@ -458,7 +461,7 @@ public class DeploymentService await _repository.SaveChangesAsync(cancellationToken); } - await _auditService.LogAsync(user, "Disable", "Instance", instanceId.ToString(), + await TryLogAuditAsync(user, "Disable", "Instance", instanceId.ToString(), instance.UniqueName, new { CommandId = commandId, response.Success }, cancellationToken); @@ -522,7 +525,7 @@ public class DeploymentService await _repository.SaveChangesAsync(cancellationToken); } - await _auditService.LogAsync(user, "Enable", "Instance", instanceId.ToString(), + await TryLogAuditAsync(user, "Enable", "Instance", instanceId.ToString(), instance.UniqueName, new { CommandId = commandId, response.Success }, cancellationToken); @@ -609,7 +612,7 @@ public class DeploymentService "removed -- the central record is now orphaned and must be reconciled manually", instance.UniqueName); - await _auditService.LogAsync(user, "DeleteOrphaned", "Instance", instanceId.ToString(), + await TryLogAuditAsync(user, "DeleteOrphaned", "Instance", instanceId.ToString(), instance.UniqueName, new { CommandId = commandId, Error = ex.Message }, CancellationToken.None); @@ -619,7 +622,7 @@ public class DeploymentService } } - await _auditService.LogAsync(user, "Delete", "Instance", instanceId.ToString(), + await TryLogAuditAsync(user, "Delete", "Instance", instanceId.ToString(), instance.UniqueName, new { CommandId = commandId, response.Success }, cancellationToken); @@ -1035,6 +1038,37 @@ public class DeploymentService /// The instance unique name used as the audit target name. /// The lifecycle command's correlation id, so the audit entry can be matched to logs. /// The captured or . + /// + /// Best-effort audit write for a post-terminal-status action. By the time this + /// runs, the deployment/lifecycle action has already committed its authoritative + /// result; a failed audit write must be logged for operator reconciliation but + /// must NEVER surface to the caller or flip the committed status — "audit-write + /// failure NEVER aborts the user-facing action" (Centralized Audit Log design). + /// Any exception is swallowed and logged at Warning. + /// + private async Task TryLogAuditAsync( + string user, + string action, + string entityType, + string entityId, + string entityName, + object? afterState, + CancellationToken cancellationToken) + { + try + { + await _auditService.LogAsync( + user, action, entityType, entityId, entityName, afterState, cancellationToken); + } + catch (Exception auditEx) + { + _logger.LogWarning(auditEx, + "Failed to write {Action} audit entry for {EntityType} {EntityName} -- the action " + + "itself succeeded and its committed status is authoritative; audit is best-effort", + action, entityType, entityName); + } + } + private async Task TryLogLifecycleTimeoutAsync( string user, string action, diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs index e8581351..ddcb4801 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -878,6 +878,36 @@ public class DeploymentServiceTests : TestKit Arg.Any(), Arg.Any()); } + [Fact] + public async Task DeployInstanceAsync_DeployAuditWriteThrows_KeepsCommittedSuccess() + { + // #05-T17: the post-success "Deploy" audit write is best-effort. If it + // throws, the already-committed Success record must NOT be flipped to + // Failed by the outer catch — "audit-write failure NEVER aborts the + // user-facing action". + var instance = new Instance("AuditFaultInst") { Id = 52, SiteId = 1, State = InstanceState.NotDeployed }; + _repo.GetInstanceByIdAsync(52, Arg.Any()).Returns(instance); + SetupValidPipeline(52, "AuditFaultInst", "sha256:target"); + _repo.GetCurrentDeploymentStatusAsync(52, Arg.Any()) + .Returns((DeploymentRecord?)null); + + // The Deploy audit LogAsync faults. + _audit.When(a => a.LogAsync( + Arg.Any(), "Deploy", Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any(), Arg.Any())) + .Do(_ => throw new InvalidOperationException("audit pipeline down")); + + var counters = new ReconcileProbeCounters(); + var commActor = Sys.ActorOf(Props.Create(() => + new ReconcileProbeActor(counters, siteHash: "sha256:target", failQuery: false))); + var service = CreateServiceWithCommActor(commActor); + + var result = await service.DeployInstanceAsync(52, "admin"); + + Assert.True(result.IsSuccess); + Assert.Equal(DeploymentStatus.Success, result.Value.Status); + } + // ── Telemetry follow-on: scadabridge.deployments.applied on deploy success ── [Fact]