From ff89e3145c5e36d650728271a709e968df1c5170 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 8 Jul 2026 15:51:56 -0400 Subject: [PATCH] =?UTF-8?q?fix(host):=20/health/active=20=3D=20oldest=20me?= =?UTF-8?q?mber=20+=20DB=20reachable=20=E2=80=94=20partition-safe=20Traefi?= =?UTF-8?q?k=20routing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Health/OldestNodeActiveHealthCheck.cs | 31 ++++++++++ src/ZB.MOM.WW.ScadaBridge.Host/Program.cs | 33 ++++++----- .../HealthCheckTests.cs | 58 +++++++++++++++++-- 3 files changed, 105 insertions(+), 17 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.Host/Health/OldestNodeActiveHealthCheck.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Health/OldestNodeActiveHealthCheck.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Health/OldestNodeActiveHealthCheck.cs new file mode 100644 index 00000000..30c85d39 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Health/OldestNodeActiveHealthCheck.cs @@ -0,0 +1,31 @@ +using Akka.Actor; +using Microsoft.Extensions.Diagnostics.HealthChecks; + +namespace ZB.MOM.WW.ScadaBridge.Host.Health; + +/// +/// /health/active check: Healthy only on the oldest Up member — the node that +/// hosts the cluster singletons. Replaces the shared package's leader-based +/// ActiveNodeHealthCheck (review 01 [High]): leadership is address-ordered and +/// diverges from singleton placement after a restart, and during a partition +/// BOTH nodes compute themselves leader, so Traefik served both. Oldest-member +/// semantics keep exactly one active node through a partition (the non-oldest +/// side still sees the old oldest member until SBR resolves membership). +/// +public sealed class OldestNodeActiveHealthCheck : IHealthCheck +{ + private readonly ActorSystem _system; + + /// Initializes a new bound to the running actor system. + /// The live cluster actor system. + public OldestNodeActiveHealthCheck(ActorSystem system) => _system = system; + + /// + public Task CheckHealthAsync(HealthCheckContext context, CancellationToken ct = default) + { + var cluster = Akka.Cluster.Cluster.Get(_system); + return Task.FromResult(ClusterActivityEvaluator.SelfIsOldest(cluster) + ? HealthCheckResult.Healthy("oldest Up member (singleton host)") + : HealthCheckResult.Unhealthy("not the oldest Up member (standby)")); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index 7f75a8eb..6bea9790 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -210,15 +210,19 @@ try builder.Services.AddConfigurationDatabase(configDbConnectionString); // Health checks for readiness gating — shared ZB.MOM.WW.Health probes. - // Check names and the ready/active tier split are preserved: database + akka-cluster - // carry the Ready tag (/health/ready), active-node carries the Active tag (/health/active). - // The Akka checks resolve ActorSystem from DI via the transient bridge registered below; + // Ready tier (/health/ready): database + akka-cluster + required-singletons. + // Active tier (/health/active): active-node = oldest Up member (singleton host) AND + // database reachable (review 01 [High]) — a DB-dead active node drops from Traefik + // rotation, and the active-node check is the host-local OldestNodeActiveHealthCheck, + // NOT the shared leader-based ActiveNodeHealthCheck (leadership diverges from singleton + // placement after a restart and both sides claim leadership during a partition). + // The Akka checks resolve ActorSystem from DI via the singleton bridge registered below; // the DatabaseHealthCheck resolves a scoped ScadaBridgeDbContext (no factory). builder.Services.AddHealthChecks() .AddTypeActivatedCheck>( "database", failureStatus: null, - tags: new[] { ZbHealthTags.Ready }) + tags: new[] { ZbHealthTags.Ready, ZbHealthTags.Active }) .AddTypeActivatedCheck( "akka-cluster", failureStatus: null, @@ -236,7 +240,9 @@ try "required-singletons", failureStatus: null, tags: new[] { ZbHealthTags.Ready }) - .AddTypeActivatedCheck( + // Host-local oldest-member check (review 01 [High]) — see OldestNodeActiveHealthCheck. + // Resolves ActorSystem from DI, like the akka-cluster check above. + .AddTypeActivatedCheck( "active-node", failureStatus: null, tags: new[] { ZbHealthTags.Active }); @@ -261,9 +267,9 @@ try // standby-node gating is actually enforced (the InboundApiEndpointFilter // consults IActiveNodeGate and defaults to "allow" when none is registered, // which leaves the design's "central cluster only (active node)" guarantee - // unenforced in deployed binaries). The gate is backed by the same Akka - // cluster-leadership check as ActiveNodeHealthCheck above, so the inbound - // API and the /health/active endpoint Traefik routes against agree on + // unenforced in deployed binaries). The gate is backed by the same oldest-member + // evaluator (ClusterActivityEvaluator) as OldestNodeActiveHealthCheck above, so the + // inbound API and the /health/active endpoint Traefik routes against agree on // which node is active. builder.Services.AddSingleton(); @@ -352,12 +358,13 @@ try branch => branch.UseAuditWriteMiddleware()); // Map the canonical three-tier health endpoints in one call: - // /health/ready — Ready-tagged checks (database + akka-cluster). REQ-HOST-4a defines - // readiness as cluster membership + DB connectivity, explicitly NOT - // cluster leadership, so the leader-only active-node check is excluded + // /health/ready — Ready-tagged checks (database + akka-cluster + required-singletons). + // REQ-HOST-4a defines readiness as cluster membership + DB connectivity, + // explicitly NOT active-node status, so the active-node check is excluded // (a fully operational standby central node still reports ready). - // /health/active — Active-tagged check (active-node); returns 200 only on the cluster - // leader; used by Traefik for routing. + // /health/active — Active-tagged checks (active-node = oldest Up member + database + // reachable); returns 200 only on the singleton-host node with a live + // DB; used by Traefik for routing. // /healthz — bare process liveness; runs no checks (always 200 while the process // is up). New tier added by adopting the shared library. // All three are anonymous and use the canonical ZbHealthWriter JSON output. diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs index 547b39ea..1a17b58c 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/HealthCheckTests.cs @@ -5,6 +5,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Diagnostics.HealthChecks; using Microsoft.Extensions.Options; using ZB.MOM.WW.Health; +using ZB.MOM.WW.ScadaBridge.Host.Health; namespace ZB.MOM.WW.ScadaBridge.Host.Tests; @@ -177,10 +178,11 @@ public class HealthCheckTests : IDisposable } [Fact] - public void ActiveTier_Carries_Only_ActiveNode() + public void ActiveTier_Carries_ActiveNode_And_Database() { - // The active-node leader check carries the Active tag (→ /health/active); the readiness - // checks do not, so /health/active reports leadership alone. + // Review 01 [High]: /health/active = oldest Up member AND database reachable, so a + // DB-dead active node is pulled from Traefik rotation. active-node + database carry + // the Active tag; akka-cluster does not (readiness-only). var previousEnv = Environment.GetEnvironmentVariable("DOTNET_ENVIRONMENT"); try { @@ -192,7 +194,8 @@ public class HealthCheckTests : IDisposable Assert.True(registrations.ContainsKey("active-node"), "Expected an 'active-node' health check."); Assert.Contains(ZbHealthTags.Active, registrations["active-node"].Tags); - Assert.DoesNotContain(ZbHealthTags.Active, registrations["database"].Tags); + // Database gates active too (DB-dead active node must drop out of rotation). + Assert.Contains(ZbHealthTags.Active, registrations["database"].Tags); Assert.DoesNotContain(ZbHealthTags.Active, registrations["akka-cluster"].Tags); } finally @@ -200,4 +203,51 @@ public class HealthCheckTests : IDisposable Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", previousEnv); } } + + [Fact] + public void ActiveNodeCheck_IsHostLocalOldestNodeCheck() + { + // The shared package's ActiveNodeHealthCheck is LEADER-based (review 01 [High]): + // during a partition both nodes think they lead and Traefik serves both. The + // active-node check must be the host-local oldest-member check instead. + var previousEnv = Environment.GetEnvironmentVariable("DOTNET_ENVIRONMENT"); + try + { + Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", "Central"); + var factory = CreateCentralFactory(); + + var active = Registrations(factory).Single(r => r.Name == "active-node"); + Assert.Contains(ZbHealthTags.Active, active.Tags); + + var check = active.Factory(factory.Services); + Assert.IsType(check); + } + finally + { + Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", previousEnv); + } + } + + [Fact] + public void DatabaseCheck_AlsoGatesActive() + { + // Review 01 [High] recommendation: /health/active should require Ready + // (DB reachable) in addition to singleton-host status, so a DB-dead + // active node is pulled from Traefik rotation. + var previousEnv = Environment.GetEnvironmentVariable("DOTNET_ENVIRONMENT"); + try + { + Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", "Central"); + var factory = CreateCentralFactory(); + + var registrations = Registrations(factory).ToDictionary(r => r.Name); + var db = registrations["database"]; + Assert.Contains(ZbHealthTags.Ready, db.Tags); + Assert.Contains(ZbHealthTags.Active, db.Tags); + } + finally + { + Environment.SetEnvironmentVariable("DOTNET_ENVIRONMENT", previousEnv); + } + } }