From 3b55d60ef94f6ab7ce9838c623932a9c7754c076 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 14 Sep 2026 13:48:06 -0400 Subject: [PATCH] fix(deploy): staleness batch logs one Warning per batch, not one per instance Claude-Session: https://claude.ai/code/session_01Y5ef34iUUMjmwnMpL9xgmB --- .../IDeploymentManagerRepository.cs | 6 ++ .../DeploymentService.cs | 34 ++++++++- .../DeploymentServiceStalenessTests.cs | 74 ++++++++++++++++++- 3 files changed, 112 insertions(+), 2 deletions(-) diff --git a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs index 78f74d2d..09551385 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Commons/Interfaces/Repositories/IDeploymentManagerRepository.cs @@ -207,6 +207,12 @@ public interface IDeploymentManagerRepository /// InstanceId → RevisionHash for every DeployedConfigSnapshot. A projection read — never /// materialises ConfigurationJson, so it is safe to call for the whole fleet (the /// pending-updates aggregate does, once per cache TTL). + /// + /// + /// Deliberately UNFILTERED: it always returns the whole fleet's two-column projection, with no + /// site, state or scope narrowing. A site-scoped or otherwise partial caller must filter the + /// returned map itself rather than assume the read was narrowed for it. + /// /// /// A cancellation token that can be used to cancel the operation. /// A dictionary keyed by instance ID; instances with no snapshot are absent. diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index 47811131..76332b92 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -1708,6 +1708,11 @@ public class DeploymentService // older schema) still yields the frozen-hash staleness result, with a // null diff. var deployedRevisionHash = ComputeDeployedHash(snapshot, instanceId, out var deployedConfig); + + // Deliberately outside ComputeDeployedHash's JsonException catch: DiffService + // compares already-deserialized POCOs and does no JSON work of its own, so it + // cannot raise the exception that catch exists for. A DiffService that ever + // starts parsing JSON must revisit this. var diff = deployedConfig == null ? null : _diffService.ComputeDiff( @@ -1832,6 +1837,16 @@ public class DeploymentService var deployedHashes = await _repository.GetDeployedRevisionHashesAsync(cancellationToken); var session = _flatteningPipeline.CreateSession(); + // Failures are summarised into ONE Warning at the end rather than logged + // per instance: this runs for the whole fleet on a cache-TTL cadence, so a + // single broken shared template fanning out to N instances would otherwise + // emit N Warning lines every minute, forever. The per-instance detail stays + // at Debug for whoever is actually diagnosing it. + var evaluated = 0; + var failedCount = 0; + var firstFailureInstanceId = 0; + string? firstFailureError = null; + foreach (var instanceId in instanceIds.Distinct()) { cancellationToken.ThrowIfCancellationRequested(); @@ -1839,15 +1854,24 @@ public class DeploymentService if (!deployedHashes.TryGetValue(instanceId, out var deployedHash)) continue; + evaluated++; + // Read-only path: hash only, so skip the Roslyn compile — the deploy // gate remains the authoritative compile. var currentResult = await _flatteningPipeline.FlattenAndValidateAsync( instanceId, cancellationToken, validateScripts: false, session); if (currentResult.IsFailure) { - _logger.LogWarning( + _logger.LogDebug( "Could not evaluate staleness of instance {InstanceId}: {Error}", instanceId, currentResult.Error); + if (failedCount == 0) + { + firstFailureInstanceId = instanceId; + firstFailureError = currentResult.Error; + } + + failedCount++; staleness[instanceId] = null; continue; } @@ -1870,6 +1894,14 @@ public class DeploymentService !string.Equals(normalizedDeployedHash, currentHash, StringComparison.Ordinal); } + if (failedCount > 0) + { + _logger.LogWarning( + "Could not evaluate staleness of {FailedCount} of {EvaluatedCount} instances; " + + "first failure: instance {InstanceId}: {Error}", + failedCount, evaluated, firstFailureInstanceId, firstFailureError); + } + return staleness; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceStalenessTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceStalenessTests.cs index 002991c0..47f0b6b1 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceStalenessTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceStalenessTests.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using NSubstitute; @@ -38,6 +39,9 @@ public class DeploymentServiceStalenessTests /// The single session every flatten in a batch must share. private readonly FlattenSession _session = new(new TemplateGraphWatermark()); + /// Captures the service's log entries so the per-batch Warning can be asserted. + private readonly CapturingLogger _logger = new(); + /// /// Builds the service over substituted repository and pipeline seams; every /// other dependency is the real concrete collaborator (as elsewhere in this @@ -73,7 +77,34 @@ public class DeploymentServiceStalenessTests OperationLockTimeout = TimeSpan.FromSeconds(5) }), Options.Create(new CommunicationOptions()), - NullLogger.Instance); + _logger); + } + + /// + /// Captures formatted log entries so the batch's single failure Warning can be + /// asserted (mirrors the CapturingLogger used elsewhere in the test suite). + /// + private sealed class CapturingLogger : ILogger + { + /// Every entry logged, in order. + public List<(LogLevel Level, string Message)> Entries { get; } = []; + + /// + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + /// + public bool IsEnabled(LogLevel logLevel) => true; + + /// + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Entries.Add((logLevel, formatter(state, exception))); + } } /// Builds a one-attribute flattened config with the given scalar value. @@ -215,6 +246,47 @@ public class DeploymentServiceStalenessTests Assert.Null(result[1]); } + [Fact] + public async Task GetInstanceStalenessAsync_ManyFlattenFailures_LogsExactlyOneWarning() + { + // The batch runs for the whole fleet on a cache-TTL cadence, so one broken + // shared template fanning out to N instances must NOT produce N Warning + // lines per minute: the per-instance detail drops to Debug and the batch + // emits a single summary Warning naming the first failure. + ArrangeDeployedHashes((1, "sha256:deployed"), (2, "sha256:deployed")); + foreach (var id in new[] { 1, 2 }) + { + _pipeline.FlattenAndValidateAsync( + id, Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(Result.Failure($"broken template {id}")); + } + + var result = await _service.GetInstanceStalenessAsync([1, 2]); + + Assert.Null(result[1]); + Assert.Null(result[2]); + + var warnings = _logger.Entries.Where(e => e.Level == LogLevel.Warning).ToList(); + var warning = Assert.Single(warnings); + Assert.Contains("2 of 2", warning.Message); + Assert.Contains("broken template 1", warning.Message); + // Both per-instance details are still available, one level down. + Assert.Equal(2, _logger.Entries.Count(e => e.Level == LogLevel.Debug)); + } + + [Fact] + public async Task GetInstanceStalenessAsync_NoFailures_LogsNoWarning() + { + var config = ScalarConfig("5"); + var hash = _hasher.ComputeHash(config); + ArrangeDeployedHashes((1, hash)); + ArrangeFlatten(1, config, hash); + + await _service.GetInstanceStalenessAsync([1]); + + Assert.DoesNotContain(_logger.Entries, e => e.Level == LogLevel.Warning); + } + [Fact] public async Task GetInstanceStalenessAsync_InstanceWithoutSnapshot_IsOmitted() {