From c9244d8bdad5d5f743b084c9700a4fab1d722477 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 16 Jun 2026 07:22:35 -0400 Subject: [PATCH] =?UTF-8?q?fix(health):=20M2.16=20review=20nit=20=E2=80=94?= =?UTF-8?q?=20real=20idempotency=20guard=20for=20SiteEventLog=20health=20b?= =?UTF-8?q?ridge=20(#30)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AddSiteEventLogHealthMetricsBridge registered via AddHostedService(factory-lambda), which sets ImplementationFactory and leaves ImplementationType null. The prior ImplementationType == guard was therefore silently dead — a second call would spin up a second SiteEventLogFailureCountReporter. Fix: add a private SiteEventLogHealthMetricsBridgeMarker singleton and guard on its ServiceType instead. Also corrects the cycle-path comment in both ServiceCollectionExtensions.cs and SiteEventLogFailureCountReporter.cs: StoreAndForward.csproj does reference SiteEventLogging.csproj, so the transitive path HealthMonitoring → StoreAndForward → SiteEventLogging is real, but adding a direct HealthMonitoring → SiteEventLogging reference would NOT create a cycle (SiteEventLogging has no back-edge to HealthMonitoring). The Func seam is a coupling-avoidance measure, not a cycle-breaker. Adds AddSiteEventLogHealthMetricsBridgeTests.AddSiteEventLogHealthMetricsBridge_IsIdempotent_DoesNotDoubleRegister_HostedService as a regression test (builds provider and asserts exactly one reporter via GetServices().OfType()). --- .../ServiceCollectionExtensions.cs | 47 +++++++++++++----- .../SiteEventLogFailureCountReporter.cs | 20 ++++---- ...AddSiteEventLogHealthMetricsBridgeTests.cs | 48 +++++++++++++++++++ ....ScadaBridge.HealthMonitoring.Tests.csproj | 1 + 4 files changed, 94 insertions(+), 22 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/AddSiteEventLogHealthMetricsBridgeTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ServiceCollectionExtensions.cs index 28d49604..54e86fcc 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ServiceCollectionExtensions.cs @@ -8,6 +8,18 @@ namespace ZB.MOM.WW.ScadaBridge.HealthMonitoring; public static class ServiceCollectionExtensions { + /// + /// Sentinel marker used by to + /// implement an idempotency guard. Because the reporter is registered via a + /// factory-lambda overload of AddHostedService, its + /// + /// is — checking it would be a silent no-op. Registering + /// this marker as a singleton and guarding on its ServiceType gives a + /// reliable, allocation-free sentinel that works regardless of how the hosted + /// service was wired. + /// + private sealed class SiteEventLogHealthMetricsBridgeMarker { } + /// /// Register site-side health monitoring services (metric collection + periodic reporting). /// Call this on site nodes only. For central, call AddCentralHealthAggregation() instead. @@ -67,19 +79,25 @@ public static class ServiceCollectionExtensions /// /// /// Why a Func<long> delegate instead of ISiteEventLogger. - /// HealthMonitoring must not reference SiteEventLogging directly — - /// the StoreAndForward → SiteEventLogging edge already exists in the - /// transitive graph, and HealthMonitoring → StoreAndForward is an - /// existing direct reference; adding HealthMonitoring → SiteEventLogging - /// would complete a cycle. The delegate seam keeps - /// the dependency acyclic: the caller (Host site wiring) captures + /// A direct HealthMonitoring → SiteEventLogging reference is avoided to + /// prevent an undesirable low-level coupling: SiteEventLogging is a + /// leaf component that should not pull in higher-level infrastructure. The + /// delegate seam keeps the reference one-way and + /// loose: the caller (Host site wiring) captures /// ISiteEventLogger.FailedWriteCount as a lambda and passes it here. + /// Note: HealthMonitoring → StoreAndForward → SiteEventLogging already + /// exists as a transitive path, so a direct reference would not introduce a + /// cycle — the delegate is purely a coupling-avoidance measure. /// /// - /// Idempotent — a sentinel check on the - /// hosted-service descriptor - /// short-circuits subsequent calls so the hosted service is not - /// double-registered (AddHostedService has no TryAdd variant). + /// Idempotent — a singleton + /// is used as the sentinel. Because the reporter is registered via a factory-lambda + /// overload of AddHostedService, its + /// + /// is ; checking it would be a silent no-op and a second + /// call would spin up a second polling timer. Guarding on the marker's + /// ServiceType is always reliable regardless of how the hosted service + /// was wired (AddHostedService has no TryAdd variant). /// /// /// The service collection to register into. @@ -99,13 +117,16 @@ public static class ServiceCollectionExtensions ArgumentNullException.ThrowIfNull(services); ArgumentNullException.ThrowIfNull(failedWriteCountProvider); - // Idempotent guard — mirrors AddAuditLogHealthMetricsBridge's - // SiteAuditBacklogReporter sentinel check. - if (services.Any(d => d.ImplementationType == typeof(SiteEventLogFailureCountReporter))) + // Idempotent guard — uses the marker type rather than ImplementationType because + // AddHostedService(factory-lambda) sets only ImplementationFactory and leaves + // ImplementationType null; an ImplementationType == check is a silent no-op for + // factory-registered services. The marker singleton's ServiceType is always set. + if (services.Any(d => d.ServiceType == typeof(SiteEventLogHealthMetricsBridgeMarker))) { return services; } + services.AddSingleton(); services.AddHostedService(sp => new SiteEventLogFailureCountReporter( failedWriteCountProvider(sp), sp.GetRequiredService(), diff --git a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs index 2076f7eb..02b50b6c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs @@ -13,15 +13,17 @@ namespace ZB.MOM.WW.ScadaBridge.HealthMonitoring; /// /// /// Why a Func<long> and not ISiteEventLogger directly. -/// HealthMonitoring does not (and cannot) reference -/// SiteEventLogging — HealthMonitoring → StoreAndForward → -/// SiteEventLogging already exists in the transitive graph, so adding a -/// direct reference would create a cycle. The -/// delegate seam breaks the coupling: the caller (Host site wiring) captures -/// ISiteEventLogger.FailedWriteCount as a lambda at registration -/// time, and this service reads only the numeric result. The delegate -/// approach is a standard pattern for counter bridges and keeps the -/// registration path self-documenting. +/// A direct HealthMonitoring → SiteEventLogging reference is avoided +/// to prevent an undesirable low-level coupling: SiteEventLogging is a +/// leaf component that should not pull in higher-level infrastructure. Note that +/// HealthMonitoring → StoreAndForward → SiteEventLogging already +/// exists as a transitive path (confirmed: StoreAndForward.csproj references +/// SiteEventLogging.csproj), so a direct reference would NOT introduce a +/// cycle — the delegate is purely a coupling-avoidance measure. The +/// seam lets the caller (Host site wiring) capture +/// ISiteEventLogger.FailedWriteCount as a lambda at registration time; this +/// service reads only the numeric result. The delegate approach is a standard +/// pattern for counter bridges and keeps the registration path self-documenting. /// /// /// Cadence. 30 s by default — the same cadence as diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/AddSiteEventLogHealthMetricsBridgeTests.cs b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/AddSiteEventLogHealthMetricsBridgeTests.cs new file mode 100644 index 00000000..b52d3020 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/AddSiteEventLogHealthMetricsBridgeTests.cs @@ -0,0 +1,48 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests; + +/// +/// M2.16 (#30) idempotency regression — code-review finding on commit d81f747. +/// +/// uses a +/// factory-lambda overload of AddHostedService, which sets only +/// ImplementationFactory and leaves ImplementationType null. The original +/// ImplementationType == guard was therefore a silent no-op: a second call would spin +/// up a second (two timers both polling). +/// The fix uses a private marker singleton whose ServiceType is always set. +/// +/// +public class AddSiteEventLogHealthMetricsBridgeTests +{ + [Fact] + public void AddSiteEventLogHealthMetricsBridge_IsIdempotent_DoesNotDoubleRegister_HostedService() + { + // M2.16 (#30): calling the bridge method twice must register exactly one + // SiteEventLogFailureCountReporter. Without the marker-type guard the + // ImplementationType == check was a no-op for factory-lambda registrations, + // so the second call would have added a second hosted service (two timers). + var services = new ServiceCollection(); + services.AddSingleton(); + services.AddSingleton(typeof(ILogger<>), typeof(NullLogger<>)); + services.AddHealthMonitoring(); + + Func> factory = _ => () => 0L; + + services.AddSiteEventLogHealthMetricsBridge(factory); + services.AddSiteEventLogHealthMetricsBridge(factory); + + // Count IHostedService descriptors whose factory produces a + // SiteEventLogFailureCountReporter. Because it is factory-registered, + // ImplementationType is null — we count by resolving and checking type. + using var provider = services.BuildServiceProvider(); + var reporters = provider.GetServices() + .OfType() + .ToList(); + + Assert.Single(reporters); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests.csproj b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests.csproj index f3bf6d48..478dce1f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests.csproj +++ b/tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests.csproj @@ -11,6 +11,7 @@ +