Merge rows 80+73+71: nullable LocalDb health fields on unreplicated sites (Host supplies ReplicationIsConfigured), StartupReconcilerActive on the health report (proto field 29) with central No-self-heal badge, standby active-node ERR downgraded via narrowly-keyed Serilog filter
# Conflicts: # CLAUDE.md
This commit is contained in:
Joseph Doherty
@@ -178,6 +178,18 @@ public class SiteLocalDbWiringTests : IDisposable
|
||||
// End-to-end through the REAL collector: probe once, then collect. This is what
|
||||
// catches a bridge that is registered but wired to the wrong provider — the
|
||||
// registration test above would pass either way.
|
||||
//
|
||||
// This fixture IS a replicating node: it carries LocalDb:Replication:ApiKey (the
|
||||
// passive half of the rig's site-a pair), and configured-ness is PeerAddress OR
|
||||
// ApiKey. So false/0 is the correct, meaningful answer here — "replication is on,
|
||||
// no session is up yet, and the oplog really is empty". Register row 80 is about
|
||||
// the OTHER case, which is pinned by UnreplicatedSiteLocalDbHealthTests: a node
|
||||
// with NEITHER key must report null/null, because the engine's inert false/0 there
|
||||
// means "not applicable", not "disconnected".
|
||||
//
|
||||
// Do not weaken this assertion to null without checking which fixture you are in —
|
||||
// conflating "no PeerAddress" with "replication unconfigured" is precisely the
|
||||
// mistake that produced row 80.
|
||||
var reporter = _host.Services.GetServices<IHostedService>()
|
||||
.OfType<LocalDbReplicationStatusReporter>()
|
||||
.Single();
|
||||
@@ -186,7 +198,6 @@ public class SiteLocalDbWiringTests : IDisposable
|
||||
reporter.Probe();
|
||||
var report = collector.CollectReport("TestSite");
|
||||
|
||||
// No peer configured: not connected, and a REAL 0 backlog rather than null.
|
||||
Assert.False(report.LocalDbReplicationConnected);
|
||||
Assert.Equal(0L, report.LocalDbOplogBacklog);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,152 @@
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using Serilog;
|
||||
using Serilog.Events;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Deferred-work register row 71 — a standby node logged the correct-by-design
|
||||
/// <c>active-node</c> health-check verdict at ERROR on every probe.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <c>DefaultHealthCheckService</c> picks the log level from the verdict (Debug/Warning/Error
|
||||
/// for Healthy/Degraded/Unhealthy). Both active-node checks answer Unhealthy on the standby
|
||||
/// because 503 is the contract <c>/health/active</c> publishes, so a healthy standby logged
|
||||
/// ERR every 10 s on a site node and every 5 s on a central node behind Traefik's probe.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The constraint on the fix is that a GENUINE failure must still log at Error, which is why
|
||||
/// the filter is keyed on the check NAME rather than on the logger category.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public class StandbyHealthCheckLogNoiseTests
|
||||
{
|
||||
private const string HealthCheckCategory =
|
||||
"Microsoft.Extensions.Diagnostics.HealthChecks.DefaultHealthCheckService";
|
||||
|
||||
private const string Template =
|
||||
"Health check {HealthCheckName} with status {HealthStatus} completed after "
|
||||
+ "{ElapsedMilliseconds}ms with message '{HealthCheckDescription}'";
|
||||
|
||||
private static ILogger BuildLogger(InMemorySink sink)
|
||||
{
|
||||
var config = new ConfigurationBuilder()
|
||||
.AddInMemoryCollection(new Dictionary<string, string?>
|
||||
{
|
||||
["ScadaBridge:Logging:MinimumLevel"] = "Debug",
|
||||
})
|
||||
.Build();
|
||||
|
||||
return LoggerConfigurationFactory
|
||||
.Build(config, "Site", "site-a", "site-a-b")
|
||||
.WriteTo.Sink(sink)
|
||||
.CreateLogger();
|
||||
}
|
||||
|
||||
/// <summary>Emits one event shaped exactly like the framework's per-check line.</summary>
|
||||
private static void EmitHealthCheckLine(
|
||||
ILogger logger, string category, LogEventLevel level, string checkName, string status) =>
|
||||
logger
|
||||
.ForContext("SourceContext", category)
|
||||
.Write(level, Template, checkName, status, 1.0, "some message");
|
||||
|
||||
[Fact]
|
||||
public void StandbyActiveNodeVerdict_IsNotLogged()
|
||||
{
|
||||
// The row-71 regression: "Health check active-node with status Unhealthy … 'not the
|
||||
// site pair primary (standby)'" at Error, once per probe, forever.
|
||||
var sink = new InMemorySink();
|
||||
var logger = BuildLogger(sink);
|
||||
|
||||
EmitHealthCheckLine(
|
||||
logger, HealthCheckCategory, LogEventLevel.Error, "active-node", "Unhealthy");
|
||||
|
||||
Assert.Empty(sink.LogEvents);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData("localdb")]
|
||||
[InlineData("akka-cluster")]
|
||||
[InlineData("database")]
|
||||
[InlineData("required-singletons")]
|
||||
public void GenuineCheckFailure_StillLogsAtError(string checkName)
|
||||
{
|
||||
// The constraint. The filter is name-scoped, so every OTHER check on the same logger
|
||||
// category keeps its Error line — a blanket category override would have silenced
|
||||
// these too, which is exactly the trap this test exists to catch.
|
||||
var sink = new InMemorySink();
|
||||
var logger = BuildLogger(sink);
|
||||
|
||||
EmitHealthCheckLine(
|
||||
logger, HealthCheckCategory, LogEventLevel.Error, checkName, "Unhealthy");
|
||||
|
||||
var evt = Assert.Single(sink.LogEvents);
|
||||
Assert.Equal(LogEventLevel.Error, evt.Level);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ActiveNodeHealthyVerdict_IsUnaffected()
|
||||
{
|
||||
// The active node reports Healthy, which the framework logs at Debug. Nothing about
|
||||
// the filter should touch it — it only ever matches Error.
|
||||
var sink = new InMemorySink();
|
||||
var logger = BuildLogger(sink);
|
||||
|
||||
EmitHealthCheckLine(
|
||||
logger, HealthCheckCategory, LogEventLevel.Debug, "active-node", "Healthy");
|
||||
|
||||
Assert.Single(sink.LogEvents);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ActiveNodeDegradedVerdict_IsUnaffected()
|
||||
{
|
||||
// Degraded logs at Warning. If a future check shape ever reports Degraded for
|
||||
// active-node, that should still be visible — the filter is Error-only.
|
||||
var sink = new InMemorySink();
|
||||
var logger = BuildLogger(sink);
|
||||
|
||||
EmitHealthCheckLine(
|
||||
logger, HealthCheckCategory, LogEventLevel.Warning, "active-node", "Degraded");
|
||||
|
||||
Assert.Single(sink.LogEvents);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void AnErrorMentioningActiveNode_FromAnotherCategory_IsNotSuppressed()
|
||||
{
|
||||
// The predicate requires BOTH the health-check category and the check name, so
|
||||
// application code logging about the active node keeps its Error.
|
||||
var sink = new InMemorySink();
|
||||
var logger = BuildLogger(sink);
|
||||
|
||||
EmitHealthCheckLine(
|
||||
logger,
|
||||
"ZB.MOM.WW.ScadaBridge.Host.Actors.AkkaHostedService",
|
||||
LogEventLevel.Error,
|
||||
"active-node",
|
||||
"Unhealthy");
|
||||
|
||||
Assert.Single(sink.LogEvents);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void OrdinaryErrorsWithNoHealthCheckProperties_AreNotSuppressed()
|
||||
{
|
||||
var sink = new InMemorySink();
|
||||
var logger = BuildLogger(sink);
|
||||
|
||||
logger.Error("something genuinely broke");
|
||||
|
||||
Assert.Single(sink.LogEvents);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Predicate_RejectsANullEvent()
|
||||
{
|
||||
// Defensive: Serilog never hands the filter a null, but the predicate is internal
|
||||
// and reachable, and returning "suppress" for null would be the wrong default.
|
||||
Assert.False(LoggerConfigurationFactory.IsExpectedStandbyActiveNodeProbe(null!));
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,134 @@
|
||||
using Microsoft.AspNetCore.Builder;
|
||||
using Microsoft.Extensions.Configuration;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Microsoft.Extensions.Hosting;
|
||||
using ZB.MOM.WW.LocalDb.Replication;
|
||||
using ZB.MOM.WW.ScadaBridge.HealthMonitoring;
|
||||
using ZB.MOM.WW.ScadaBridge.Host.Actors;
|
||||
|
||||
namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// Deferred-work register row 80 — a site node with replication UNCONFIGURED must report
|
||||
/// <c>LocalDbReplicationConnected</c> and <c>LocalDbOplogBacklog</c> as <c>null</c>.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// The sibling <see cref="SiteLocalDbWiringTests"/> fixture is a <em>replicating</em> node
|
||||
/// (it sets <c>LocalDb:Replication:ApiKey</c>, modelling the passive half of the rig's
|
||||
/// site-a pair), so false/0 is the right answer there. This fixture is the other posture —
|
||||
/// the rig's site-b/site-c, and the shipping default — with NEITHER <c>PeerAddress</c> nor
|
||||
/// <c>ApiKey</c>.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The replication engine is registered unconditionally and idles, so <c>ISyncStatus</c>
|
||||
/// still answers <c>Connected=false</c> with a real <c>0</c> backlog. Publishing that pair
|
||||
/// is the row-80 bug: a dashboard reader sees <c>connected: false</c> on a perfectly healthy
|
||||
/// unreplicated site and reads a fault, and central's null branches — the conditional KPI
|
||||
/// emission and the sustained-badge anchor exclusion built for register row 33 — never
|
||||
/// receive the null they exist for, so unreplicated sites emitted a misleading
|
||||
/// <c>localDbReplicationConnected=0</c> series.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Built from the REAL composition root for the same reason the sibling fixture is: the
|
||||
/// defect is in the wiring, and a hand-assembled <c>ServiceCollection</c> would prove
|
||||
/// nothing about what the Host actually passes.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public class UnreplicatedSiteLocalDbHealthTests : IDisposable
|
||||
{
|
||||
private readonly WebApplication _host;
|
||||
private readonly string _tempDbPath;
|
||||
private readonly string _tempSiteDbPath;
|
||||
private readonly string _tempTrackingDbPath;
|
||||
|
||||
public UnreplicatedSiteLocalDbHealthTests()
|
||||
{
|
||||
var stamp = Guid.NewGuid();
|
||||
_tempDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_unrep_localdb_{stamp}.db");
|
||||
_tempSiteDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_unrep_site_{stamp}.db");
|
||||
_tempTrackingDbPath = Path.Combine(Path.GetTempPath(), $"scadabridge_unrep_track_{stamp}.db");
|
||||
|
||||
var builder = WebApplication.CreateBuilder();
|
||||
builder.Configuration.Sources.Clear();
|
||||
builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
|
||||
{
|
||||
["ScadaBridge:Node:Role"] = "Site",
|
||||
["ScadaBridge:Node:NodeName"] = "node-a",
|
||||
["ScadaBridge:Node:NodeHostname"] = "test-site-b",
|
||||
["ScadaBridge:Node:SiteId"] = "TestSiteB",
|
||||
["ScadaBridge:Node:RemotingPort"] = "0",
|
||||
["ScadaBridge:Node:GrpcPort"] = "0",
|
||||
["ScadaBridge:Database:SiteDbPath"] = _tempSiteDbPath,
|
||||
["ScadaBridge:OperationTracking:ConnectionString"] = $"Data Source={_tempTrackingDbPath}",
|
||||
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
|
||||
["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552",
|
||||
|
||||
// The point of this fixture: LocalDb:Path only. No PeerAddress, no ApiKey —
|
||||
// SiteLocalDbSetup.ReplicationIsConfigured is false, matching rig site-b/site-c.
|
||||
["LocalDb:Path"] = _tempDbPath,
|
||||
});
|
||||
|
||||
builder.Services.AddGrpc();
|
||||
builder.Services.AddSingleton<ZB.MOM.WW.ScadaBridge.Communication.Grpc.SiteStreamGrpcServer>();
|
||||
|
||||
SiteServiceRegistration.Configure(builder.Services, builder.Configuration);
|
||||
|
||||
AkkaHostedServiceRemover.RemoveAkkaHostedServiceOnly(builder.Services);
|
||||
|
||||
_host = builder.Build();
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
(_host as IDisposable)?.Dispose();
|
||||
foreach (var path in new[] { _tempDbPath, _tempSiteDbPath, _tempTrackingDbPath })
|
||||
{
|
||||
try { File.Delete(path); } catch { /* best effort */ }
|
||||
}
|
||||
|
||||
GC.SuppressFinalize(this);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void UnreplicatedSite_HealthReport_ReportsNullForBothLocalDbFields()
|
||||
{
|
||||
// The row-80 regression, end-to-end through the real container and the real
|
||||
// collector. Before the fix this produced false/0.
|
||||
var reporter = _host.Services.GetServices<IHostedService>()
|
||||
.OfType<LocalDbReplicationStatusReporter>()
|
||||
.Single();
|
||||
var collector = _host.Services.GetRequiredService<ISiteHealthCollector>();
|
||||
|
||||
reporter.Probe();
|
||||
var report = collector.CollectReport("TestSiteB");
|
||||
|
||||
Assert.Null(report.LocalDbReplicationConnected);
|
||||
Assert.Null(report.LocalDbOplogBacklog);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void UnreplicatedSite_StillRegistersTheHealthBridge()
|
||||
{
|
||||
// The fix is a GATE on what the reporter publishes, not a skipped registration.
|
||||
// Keeping the hosted service present means turning replication on is a config
|
||||
// change rather than a different service graph, and it keeps one code path for
|
||||
// both postures.
|
||||
var hosted = _host.Services.GetServices<IHostedService>();
|
||||
|
||||
Assert.Contains(hosted, h => h is LocalDbReplicationStatusReporter);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void UnreplicatedSite_ReplicationEngine_StillResolvesAndIdles()
|
||||
{
|
||||
// Establishes the premise the row-80 fix rests on: the engine really does answer
|
||||
// false/0 here. If this ever starts returning null on its own, the gate becomes
|
||||
// redundant rather than wrong — but the test above still pins the observable
|
||||
// contract either way.
|
||||
var status = _host.Services.GetRequiredService<ISyncStatus>();
|
||||
|
||||
Assert.False(status.Connected);
|
||||
Assert.Equal(0L, status.OplogBacklog);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user