From 7892c622cf7b3c44d716600e95a9f645cca39b79 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:13:53 -0500 Subject: [PATCH] fix(health): site /health/active uses the shared role-scoped ActiveNodeHealthCheck (ScadaBridge#97) Replace the private SitePairActiveNodeHealthCheck (503 whenever no ActorSystem, no data) with the shared ZB.MOM.WW.Health ActiveNodeHealthCheck constructed with the site-{SiteId} role: Degraded (200) during startup per the Health spec, and it publishes selfAddress/activeRole/activeNode so the Overview dashboard can name the active site node. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- CLAUDE.md | 2 +- docs/requirements/Component-Host.md | 2 +- .../Health/SitePairActiveNodeHealthCheck.cs | 39 ------ .../LoggerConfigurationFactory.cs | 2 +- .../SiteServiceRegistration.cs | 13 +- .../SiteHealthCheckTests.cs | 112 ++++++++++++------ 6 files changed, 85 insertions(+), 85 deletions(-) delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Host/Health/SitePairActiveNodeHealthCheck.cs diff --git a/CLAUDE.md b/CLAUDE.md index 377d0bbf..8ce4201b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -201,7 +201,7 @@ spec for each is `docs/requirements/Component-.md`, and `README.md` carrie - Failure detection: 2s heartbeat, 10s threshold. Total failover ~25s (drill-measured 2026-07-21 under auto-down: active-crash TAKEOVER in 28s, standby-crash removal in 27s with 0 routing blips — `docker/failover-drill.sh`). - CoordinatedShutdown for graceful singleton handover. - Automatic dual-node recovery from persistent storage. -- **Active/standby is decided by `ActiveNodeEvaluator.SelfIsOldestUp`, never by cluster leadership** — see the Architecture note above. **Central's** `/health/active` backs Traefik's active-node routing and the Inbound API uses the shared `ZB.MOM.WW.Health.IActiveNodeGate` (`AkkaActiveNodeGate`, same `ClusterActiveNode` rule; the private InboundAPI gate was removed in Gitea #80), so the proxy and the Inbound API always agree on which node is active. **Site nodes DO map `/health/*` too** — the "site nodes map no `/health/*` at all" claim that stood here was stale: `SiteServiceRegistration` registers `akka-cluster` + `localdb` (Ready) and `SitePairActiveNodeHealthCheck` (Active, role-scoped to `site-{SiteId}`, deliberately NOT central's unscoped shared check), and `Program.cs`'s Site branch calls `MapZbHealth()` on the HTTP/1.1 listener so the family overview dashboard can probe every node identically. Nothing in this repo probes a site's `/health/active` (Traefik health-probes central only), and there is **no `IHealthCheckPublisher` anywhere**, so those checks run only on an inbound HTTP probe. Consequence worth knowing: the standby's correct-by-design `active-node` Unhealthy verdict is logged by `DefaultHealthCheckService` at **Error** on every probe, which `LoggerConfigurationFactory` now suppresses with one narrowly-keyed Serilog filter (register row 71 — see `Component-Host.md` → *Standby health-check log noise*; genuine failures of every other check still log at Error). Central reaches a site by dialling `GrpcNodeAAddress`/`GrpcNodeBAddress` explicitly and flipping on error; whichever node answers, the site-internal `ClusterSingletonProxy` lands the work on the active node for free, so central still never needs to track which *site* node is active. +- **Active/standby is decided by `ActiveNodeEvaluator.SelfIsOldestUp`, never by cluster leadership** — see the Architecture note above. **Central's** `/health/active` backs Traefik's active-node routing and the Inbound API uses the shared `ZB.MOM.WW.Health.IActiveNodeGate` (`AkkaActiveNodeGate`, same `ClusterActiveNode` rule; the private InboundAPI gate was removed in Gitea #80), so the proxy and the Inbound API always agree on which node is active. **Site nodes DO map `/health/*` too** — the "site nodes map no `/health/*` at all" claim that stood here was stale: `SiteServiceRegistration` registers `akka-cluster` + `localdb` (Ready) and the shared `ActiveNodeHealthCheck` (Active, constructed with the `site-{SiteId}` role so it is role-scoped; Degraded/200 during startup and publishes `activeNode`/`activeRole`/`selfAddress` data for the Overview dashboard — Gitea #97; central registers the same class unscoped), and `Program.cs`'s Site branch calls `MapZbHealth()` on the HTTP/1.1 listener so the family overview dashboard can probe every node identically. Nothing in this repo probes a site's `/health/active` (Traefik health-probes central only), and there is **no `IHealthCheckPublisher` anywhere**, so those checks run only on an inbound HTTP probe. Consequence worth knowing: the standby's correct-by-design `active-node` Unhealthy verdict is logged by `DefaultHealthCheckService` at **Error** on every probe, which `LoggerConfigurationFactory` now suppresses with one narrowly-keyed Serilog filter (register row 71 — see `Component-Host.md` → *Standby health-check log noise*; genuine failures of every other check still log at Error). Central reaches a site by dialling `GrpcNodeAAddress`/`GrpcNodeBAddress` explicitly and flipping on error; whichever node answers, the site-internal `ClusterSingletonProxy` lands the work on the active node for free, so central still never needs to track which *site* node is active. - **Seed-node ordering: every node lists ITSELF first (decision 2026-07-22) — the boot-alone gap is CLOSED.** Only `seed-nodes[0]` may self-join to form a new cluster (Akka runs `FirstSeedNodeProcess` for it, `JoinSeedNodeProcess` — which can never form one — for everyone else). All 14 shipped node appsettings now lead with the node's own address, so any node can cold-start alone and become operational unattended (~5s, `seed-node-timeout`); `StartupValidator` fails the boot if the ordering is broken (compares host AND port; Akka does no DNS canonicalisation). Two nodes cold-starting together while mutually reachable converge on ONE cluster via the `InitJoin` handshake — they split only under a genuine boot-time partition, the same class auto-down accepts. **An external self-form timer (`Cluster.Join(SelfAddress)` after a window) was implemented and REJECTED:** it sits outside the join handshake, so on a routine standby restart — where the peer is alive but the join is stalled behind removal of the node's own stale incarnation — it fires mid-join and permanently splits the pair (measured: still split after 90s). Regression tests: `SelfFirstSeedBootstrapTests`. The keep-oldest active-crash total outage was separately closed by the auto-down decision. See `docs/requirements/Component-ClusterInfrastructure.md` → Seed Node Ordering. - **Simultaneous-cold-start split-brain guard (opt-in, Gitea #33) — the residual self-first cost, closed.** Self-first-on-both means a *truly simultaneous* cold start (shared power/hypervisor event) races `FirstSeedNodeProcess` on BOTH nodes → two 1-node clusters that never merge (in-process loopback tests converge and hide it; real parallel VM starts don't — OtOpcUa reproduced it live). Ported from OtOpcUa (`lmxopcua` `d1dac87f`): a **dark switch `ScadaBridge:Cluster:BootstrapGuard:Enabled` (default OFF, guard-off byte-identical)**. On: `BuildHocon` emits an EMPTY seed list (Akka does not auto-join) and `ClusterBootstrapCoordinator` (`IHostedService`, registered in BOTH the Central and Site composition roots) issues ONE reachability-gated `JoinSeedNodes` — the pure `ClusterBootstrapGuard` core makes the lower canonical `host:port` the **founder** (self-first, forms immediately), the higher node TCP-probes the founder up to `PartnerProbeSeconds` (25s) and joins **peer-first** if reachable else **self-first** (cold-start-alone preserved). Decided BEFORE a single join, **never re-forms mid-handshake** (that was the rejected self-form-timer's flaw); case-insensitive founder tie-break; probe timings validated `>0` when enabled. Residual accepted trade: founder dies in the probe→join window → higher node hangs unjoined, coordinator WARNs, a restart recovers it. Tests: `ClusterBootstrapGuardTests` (pure) + `ClusterBootstrapCoordinatorTests` (real-ActorSystem, incl. both-cold-start-together-form-one-cluster). **ENABLED on the docker rig (all 8 nodes) 2026-08-02** after `deploy.sh`'s simultaneous recreate split site pairs twice on 2026-08-01 with the guard off; live gate PASS — two consecutive simultaneous-start trials (a full redeploy + a full-topology `docker compose restart`), all four pairs converged (every lower-address node founded self-first, every higher node probed-then-joined peer-first), zero splits. Default remains OFF elsewhere (wonder-app-vd03, docker-env2, Host defaults). See `docs/requirements/Component-ClusterInfrastructure.md` → Simultaneous cold start. diff --git a/docs/requirements/Component-Host.md b/docs/requirements/Component-Host.md index 7d2ae5bf..7735fb7e 100644 --- a/docs/requirements/Component-Host.md +++ b/docs/requirements/Component-Host.md @@ -231,7 +231,7 @@ verdict of the `active-node` probe (register row 71). `DefaultHealthCheckService` logs one line per check per probe and picks the level from the verdict — Debug / Warning / **Error** for Healthy / Degraded / Unhealthy. Both active-node checks -(`SitePairActiveNodeHealthCheck` on a site, the shared `ActiveNodeHealthCheck` on central) answer +(the shared `ActiveNodeHealthCheck`, role-scoped to `site-{SiteId}` on a site and unscoped on central) answer Unhealthy on the standby, because 503 is exactly the contract `/health/active` publishes to Traefik and the family dashboard. A perfectly healthy standby therefore logged ERR on every probe — every 10 s on a site node, every 5 s on a central node behind Traefik's active-node probe — which floods diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Health/SitePairActiveNodeHealthCheck.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Health/SitePairActiveNodeHealthCheck.cs deleted file mode 100644 index 1ed049ea..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Health/SitePairActiveNodeHealthCheck.cs +++ /dev/null @@ -1,39 +0,0 @@ -using Microsoft.Extensions.Diagnostics.HealthChecks; -using ZB.MOM.WW.ScadaBridge.HealthMonitoring; - -namespace ZB.MOM.WW.ScadaBridge.Host.Health; - -/// -/// /health/active check for a SITE pair: Healthy only on the pair's primary — the oldest Up member -/// carrying the site-{SiteId} role. Same active/standby semantics central publishes, so a -/// consumer reads one contract fleet-wide (200 = Active, 503 = Standby). -/// -/// -/// Deliberately NOT central's unscoped shared ActiveNodeHealthCheck: that one calls -/// ClusterActivityEvaluator.SelfIsOldest(cluster) with no role argument, so on a mesh that -/// carries more than one site's members it computes "oldest" across the wrong member set and a site -/// node could report Active because it happens to be the oldest member overall. Delegating to -/// reuses the role-scoped evaluation the site -/// already trusts for singleton placement and event-log purge — one primary definition per site, -/// not two that can disagree. -/// -public sealed class SitePairActiveNodeHealthCheck : IHealthCheck -{ - private readonly IClusterNodeProvider _nodeProvider; - - /// Initializes a new . - /// The role-scoped site cluster node provider. - public SitePairActiveNodeHealthCheck(IClusterNodeProvider nodeProvider) => - _nodeProvider = nodeProvider ?? throw new ArgumentNullException(nameof(nodeProvider)); - - /// Reports Healthy on the site pair's primary and Unhealthy on the standby. - /// The health check context (unused; the verdict depends only on cluster membership). - /// Cancellation token. - /// Healthy on the primary; Unhealthy (503 = Standby) otherwise. - public Task CheckHealthAsync( - HealthCheckContext context, - CancellationToken cancellationToken = default) => - Task.FromResult(_nodeProvider.SelfIsPrimary - ? HealthCheckResult.Healthy("site pair primary (oldest Up member of the site role)") - : HealthCheckResult.Unhealthy("not the site pair primary (standby)")); -} diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/LoggerConfigurationFactory.cs b/src/ZB.MOM.WW.ScadaBridge.Host/LoggerConfigurationFactory.cs index 8275ee6b..a3fbf728 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/LoggerConfigurationFactory.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/LoggerConfigurationFactory.cs @@ -117,7 +117,7 @@ public static class LoggerConfigurationFactory /// The problem. DefaultHealthCheckService logs one line per check per probe, /// picking the level from the verdict — Debug for Healthy, Warning for Degraded, /// Error for Unhealthy. Both active-node checks - /// (SitePairActiveNodeHealthCheck on a site, the shared ActiveNodeHealthCheck + /// (the shared ActiveNodeHealthCheck, role-scoped on a site and unscoped /// on central) answer Unhealthy on the standby, because 503 is exactly the contract /// /health/active publishes to Traefik and the family dashboard. So a perfectly /// healthy standby logged ERR on every probe — every 10 s on a site node, every 5 s on a diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs index 973ca7bf..2bbe0073 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs @@ -274,12 +274,17 @@ public static class SiteServiceRegistration "localdb", failureStatus: null, tags: new[] { ZbHealthTags.Ready }) - // NOT central's unscoped shared ActiveNodeHealthCheck — that one is role-less; see - // SitePairActiveNodeHealthCheck for why a site pair needs the role-scoped answer. - .AddTypeActivatedCheck( + // The shared ActiveNodeHealthCheck, ROLE-SCOPED to this site's own `site-{SiteId}` role + // (ScadaBridge#97). Central registers it unscoped; a site must scope it, because on a + // mesh carrying more than one site's members an unscoped "oldest Up member" is computed + // over the wrong set. The shared check is startup-safe (Degraded = 200 until the + // ActorSystem/cluster is available) and publishes `selfAddress` / `activeRole` / + // `activeNode` data, which is what the Overview dashboard reads to place the Active chip. + .AddTypeActivatedCheck( "active-node", failureStatus: null, - tags: new[] { ZbHealthTags.Active }); + tags: new[] { ZbHealthTags.Active }, + args: new object[] { $"site-{config["ScadaBridge:Node:SiteId"]}" }); // Options binding BindSharedOptions(services, config); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthCheckTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthCheckTests.cs index 011777d1..23bc33b9 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthCheckTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteHealthCheckTests.cs @@ -118,7 +118,7 @@ public class SiteHealthCheckTests : IDisposable [Theory] [InlineData("akka-cluster", typeof(AkkaClusterHealthCheck))] [InlineData("localdb", typeof(SiteLocalDbHealthCheck))] - [InlineData("active-node", typeof(SitePairActiveNodeHealthCheck))] + [InlineData("active-node", typeof(ActiveNodeHealthCheck))] public void Site_ChecksActivateToTheIntendedType(string name, Type expected) { var registration = Assert.Single(Registrations, r => r.Name == name); @@ -130,20 +130,81 @@ public class SiteHealthCheckTests : IDisposable Assert.IsType(expected, check); } - /// Verifies that the site active-node check is role-scoped and is not the unscoped central check. + /// Verifies the site active-node check answers Degraded (HTTP 200), not Unhealthy, while the ActorSystem is not yet available (ScadaBridge#97). + /// A task that represents the asynchronous operation. [Fact] - public void Site_ActiveNodeCheck_IsRoleScoped_NotTheUnscopedCentralCheck() + public async Task Site_ActiveNodeCheck_IsDegradedNotUnhealthy_DuringStartup() { - // Central registers the shared ActiveNodeHealthCheck UNSCOPED — oldest Up member of the whole - // cluster. On a mesh carrying more than one site's members that computes "oldest" across the - // wrong member set, so a site node could report Active purely for being the oldest member - // overall. A site must answer for its own site-{SiteId} role, which SitePairActiveNodeHealthCheck - // does by delegating to the site's IClusterNodeProvider (itself now backed by the shared - // ClusterActiveNode, so the rule is shared even though the scoping is local). - var check = Assert.Single(Registrations, r => r.Name == "active-node").Factory(_host.Services); + // The Health spec says a node still booting is Degraded, not a failed standby. The old + // private site check returned Unhealthy (503) whenever no ActorSystem was up yet. + var registration = Assert.Single(Registrations, r => r.Name == "active-node"); + using var empty = new ServiceCollection().BuildServiceProvider(); + var check = registration.Factory(empty); - Assert.IsType(check); - Assert.IsNotType(check); + var result = await check.CheckHealthAsync(NewContext("active-node", check)); + + Assert.Equal(HealthStatus.Degraded, result.Status); + } + + /// Verifies the site active-node check reports the active node in its data and is scoped to the site's own role (ScadaBridge#97). + /// A task that represents the asynchronous operation. + [Fact] + public async Task Site_ActiveNodeCheck_ReportsActiveNode_AndIsScopedToTheSiteRole() + { + var registration = Assert.Single(Registrations, r => r.Name == "active-node"); + + // A real single-node cluster that carries the site's role: it is the oldest Up member, so + // it is Active and must name itself as activeNode with activeRole = site-TestSite. + using var inRole = await StartSelfJoinedClusterAsync("site-TestSite"); + var check = registration.Factory(inRole.Provider); + var result = await check.CheckHealthAsync(NewContext("active-node", check)); + + Assert.Equal(HealthStatus.Healthy, result.Status); + Assert.Equal("site-TestSite", result.Data["activeRole"]); + Assert.Equal(inRole.SelfAddress, result.Data["activeNode"]); + Assert.Equal(inRole.SelfAddress, result.Data["selfAddress"]); + + // A node that is the oldest member overall but carries ANOTHER site's role is not this + // site's active node: the check is scoped, so it must not answer Healthy. + using var otherRole = await StartSelfJoinedClusterAsync("site-Other"); + var other = registration.Factory(otherRole.Provider); + var otherResult = await other.CheckHealthAsync(NewContext("active-node", other)); + + Assert.NotEqual(HealthStatus.Healthy, otherResult.Status); + Assert.False(otherResult.Data.ContainsKey("activeNode")); + } + + private static async Task StartSelfJoinedClusterAsync(string role) + { + var config = Akka.Configuration.ConfigurationFactory.ParseString($$""" + akka.actor.provider = cluster + akka.remote.dot-netty.tcp.hostname = "127.0.0.1" + akka.remote.dot-netty.tcp.port = 0 + akka.cluster.roles = ["{{role}}"] + akka.loglevel = OFF + """); + var system = Akka.Actor.ActorSystem.Create("hc" + Guid.NewGuid().ToString("N")[..8], config); + var cluster = Akka.Cluster.Cluster.Get(system); + cluster.Join(cluster.SelfAddress); + var deadline = DateTime.UtcNow.AddSeconds(20); + while (cluster.SelfMember.Status != Akka.Cluster.MemberStatus.Up && DateTime.UtcNow < deadline) + await Task.Delay(50); + Assert.Equal(Akka.Cluster.MemberStatus.Up, cluster.SelfMember.Status); + + var services = new ServiceCollection(); + services.AddSingleton(system); + return new ClusterHandle(system, services.BuildServiceProvider(), cluster.SelfAddress.ToString()); + } + + private sealed class ClusterHandle(Akka.Actor.ActorSystem system, ServiceProvider provider, string selfAddress) : IDisposable + { + public IServiceProvider Provider { get; } = provider; + public string SelfAddress { get; } = selfAddress; + + public void Dispose() + { + system.Terminate().Wait(TimeSpan.FromSeconds(10)); + } } /// Verifies that a site does not register central-only health checks. @@ -204,38 +265,11 @@ public class SiteHealthCheckTests : IDisposable Assert.Equal(HealthStatus.Unhealthy, result.Status); } - /// Verifies that the active-node check separates the primary from the standby. - /// Whether this node is the primary. - /// The health status the check is expected to report. - /// A task that represents the asynchronous operation. - [Theory] - [InlineData(true, HealthStatus.Healthy)] - [InlineData(false, HealthStatus.Unhealthy)] - public async Task Site_ActiveNodeCheck_SplitsPrimaryFromStandby(bool selfIsPrimary, HealthStatus expected) - { - // 200 on the primary / 503 on the standby is the same contract central publishes, so a - // consumer reads one active-node meaning fleet-wide. - var check = new SitePairActiveNodeHealthCheck(new StubNodeProvider(selfIsPrimary)); - - var result = await check.CheckHealthAsync(NewContext("active-node", check)); - - Assert.Equal(expected, result.Status); - } - private static HealthCheckContext NewContext(string name, IHealthCheck check) => new() { Registration = new HealthCheckRegistration(name, check, HealthStatus.Unhealthy, tags: null), }; - private sealed class StubNodeProvider(bool selfIsPrimary) : IClusterNodeProvider - { - /// - public bool SelfIsPrimary { get; } = selfIsPrimary; - - /// - public IReadOnlyList GetClusterNodes() => []; - } - private sealed class ThrowingLocalDb : ILocalDb { /// Always throws to simulate an unreachable store.