fix(deploy): staleness batch logs one Warning per batch, not one per instance

Claude-Session: https://claude.ai/code/session_01Y5ef34iUUMjmwnMpL9xgmB
This commit is contained in:
Joseph Doherty
2026-09-14 13:48:06 -04:00
parent 5214932447
commit 3b55d60ef9
3 changed files with 112 additions and 2 deletions
@@ -207,6 +207,12 @@ public interface IDeploymentManagerRepository
/// InstanceId → RevisionHash for every <c>DeployedConfigSnapshot</c>. A projection read — never
/// materialises <c>ConfigurationJson</c>, so it is safe to call for the whole fleet (the
/// pending-updates aggregate does, once per cache TTL).
///
/// <para>
/// 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.
/// </para>
/// </summary>
/// <param name="cancellationToken">A cancellation token that can be used to cancel the operation.</param>
/// <returns>A dictionary keyed by instance ID; instances with no snapshot are absent.</returns>
@@ -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;
}
@@ -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
/// <summary>The single session every flatten in a batch must share.</summary>
private readonly FlattenSession _session = new(new TemplateGraphWatermark());
/// <summary>Captures the service's log entries so the per-batch Warning can be asserted.</summary>
private readonly CapturingLogger _logger = new();
/// <summary>
/// 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<DeploymentService>.Instance);
_logger);
}
/// <summary>
/// Captures formatted log entries so the batch's single failure Warning can be
/// asserted (mirrors the CapturingLogger used elsewhere in the test suite).
/// </summary>
private sealed class CapturingLogger : ILogger<DeploymentService>
{
/// <summary>Every entry logged, in order.</summary>
public List<(LogLevel Level, string Message)> Entries { get; } = [];
/// <inheritdoc />
public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
/// <inheritdoc />
public bool IsEnabled(LogLevel logLevel) => true;
/// <inheritdoc />
public void Log<TState>(
LogLevel logLevel,
EventId eventId,
TState state,
Exception? exception,
Func<TState, Exception?, string> formatter)
{
Entries.Add((logLevel, formatter(state, exception)));
}
}
/// <summary>Builds a one-attribute flattened config with the given scalar value.</summary>
@@ -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<CancellationToken>(), Arg.Any<bool>(), Arg.Any<FlattenSession?>())
.Returns(Result<FlatteningPipelineResult>.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()
{