fix(health): M2.16 review nit — real idempotency guard for SiteEventLog health bridge (#30)

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<long> 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<IHostedService>().OfType<T>()).

src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/ServiceCollectionExtensions.cs

@@ -8,6 +8,18 @@ namespace ZB.MOM.WW.ScadaBridge.HealthMonitoring;
 
 public static class ServiceCollectionExtensions
 {
+    /// <summary>
+    /// Sentinel marker used by <see cref="AddSiteEventLogHealthMetricsBridge"/> to
+    /// implement an idempotency guard. Because the reporter is registered via a
+    /// factory-lambda overload of <c>AddHostedService</c>, its
+    /// <see cref="Microsoft.Extensions.DependencyInjection.ServiceDescriptor.ImplementationType"/>
+    /// is <see langword="null"/> — checking it would be a silent no-op. Registering
+    /// this marker as a singleton and guarding on its <c>ServiceType</c> gives a
+    /// reliable, allocation-free sentinel that works regardless of how the hosted
+    /// service was wired.
+    /// </summary>
+    private sealed class SiteEventLogHealthMetricsBridgeMarker { }
+
     /// <summary>
     /// 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
     /// </para>
     /// <para>
     /// <b>Why a Func&lt;long&gt; delegate instead of ISiteEventLogger.</b>
-    /// <c>HealthMonitoring</c> must not reference <c>SiteEventLogging</c> directly —
-    /// the <c>StoreAndForward → SiteEventLogging</c> edge already exists in the
-    /// transitive graph, and <c>HealthMonitoring → StoreAndForward</c> is an
-    /// existing direct reference; adding <c>HealthMonitoring → SiteEventLogging</c>
-    /// would complete a cycle. The <see cref="Func{TResult}"/> delegate seam keeps
-    /// the dependency acyclic: the caller (Host site wiring) captures
+    /// A direct <c>HealthMonitoring → SiteEventLogging</c> reference is avoided to
+    /// prevent an undesirable low-level coupling: <c>SiteEventLogging</c> is a
+    /// leaf component that should not pull in higher-level infrastructure. The
+    /// <see cref="Func{TResult}"/> delegate seam keeps the reference one-way and
+    /// loose: the caller (Host site wiring) captures
     /// <c>ISiteEventLogger.FailedWriteCount</c> as a lambda and passes it here.
+    /// Note: <c>HealthMonitoring → StoreAndForward → SiteEventLogging</c> already
+    /// exists as a transitive path, so a direct reference would not introduce a
+    /// cycle — the delegate is purely a coupling-avoidance measure.
     /// </para>
     /// <para>
-    /// Idempotent — a sentinel check on the
-    /// <see cref="SiteEventLogFailureCountReporter"/> hosted-service descriptor
-    /// short-circuits subsequent calls so the hosted service is not
-    /// double-registered (AddHostedService has no TryAdd variant).
+    /// Idempotent — a <see cref="SiteEventLogHealthMetricsBridgeMarker"/> singleton
+    /// is used as the sentinel. Because the reporter is registered via a factory-lambda
+    /// overload of <c>AddHostedService</c>, its
+    /// <see cref="Microsoft.Extensions.DependencyInjection.ServiceDescriptor.ImplementationType"/>
+    /// is <see langword="null"/>; checking it would be a silent no-op and a second
+    /// call would spin up a second polling timer. Guarding on the marker's
+    /// <c>ServiceType</c> is always reliable regardless of how the hosted service
+    /// was wired (AddHostedService has no TryAdd variant).
     /// </para>
     /// </remarks>
     /// <param name="services">The service collection to register into.</param>
@@ -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<SiteEventLogHealthMetricsBridgeMarker>();
         services.AddHostedService(sp => new SiteEventLogFailureCountReporter(
             failedWriteCountProvider(sp),
             sp.GetRequiredService<ISiteHealthCollector>(),

src/ZB.MOM.WW.ScadaBridge.HealthMonitoring/SiteEventLogFailureCountReporter.cs

@@ -13,15 +13,17 @@ namespace ZB.MOM.WW.ScadaBridge.HealthMonitoring;
 /// <remarks>
 /// <para>
 /// <b>Why a Func&lt;long&gt; and not ISiteEventLogger directly.</b>
-/// <c>HealthMonitoring</c> does not (and cannot) reference
-/// <c>SiteEventLogging</c> — <c>HealthMonitoring → StoreAndForward →
-/// SiteEventLogging</c> already exists in the transitive graph, so adding a
-/// direct reference would create a cycle. The <see cref="Func{TResult}"/>
-/// delegate seam breaks the coupling: the caller (Host site wiring) captures
-/// <c>ISiteEventLogger.FailedWriteCount</c> 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 <c>HealthMonitoring → SiteEventLogging</c> reference is avoided
+/// to prevent an undesirable low-level coupling: <c>SiteEventLogging</c> is a
+/// leaf component that should not pull in higher-level infrastructure. Note that
+/// <c>HealthMonitoring → StoreAndForward → SiteEventLogging</c> already
+/// exists as a transitive path (confirmed: <c>StoreAndForward.csproj</c> references
+/// <c>SiteEventLogging.csproj</c>), so a direct reference would NOT introduce a
+/// cycle — the delegate is purely a coupling-avoidance measure. The
+/// <see cref="Func{TResult}"/> seam lets the caller (Host site wiring) capture
+/// <c>ISiteEventLogger.FailedWriteCount</c> 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.
 /// </para>
 /// <para>
 /// <b>Cadence.</b> 30 s by default — the same cadence as

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;
+
+/// <summary>
+/// M2.16 (#30) idempotency regression — code-review finding on commit d81f747.
+/// <para>
+/// <see cref="ServiceCollectionExtensions.AddSiteEventLogHealthMetricsBridge"/> uses a
+/// factory-lambda overload of <c>AddHostedService</c>, which sets only
+/// <c>ImplementationFactory</c> and leaves <c>ImplementationType</c> null.  The original
+/// <c>ImplementationType ==</c> guard was therefore a silent no-op: a second call would spin
+/// up a second <see cref="SiteEventLogFailureCountReporter"/> (two timers both polling).
+/// The fix uses a private marker singleton whose <c>ServiceType</c> is always set.
+/// </para>
+/// </summary>
+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<ILoggerFactory, NullLoggerFactory>();
+        services.AddSingleton(typeof(ILogger<>), typeof(NullLogger<>));
+        services.AddHealthMonitoring();
+
+        Func<IServiceProvider, Func<long>> 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<IHostedService>()
+            .OfType<SiteEventLogFailureCountReporter>()
+            .ToList();
+
+        Assert.Single(reporters);
+    }
+}

tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests/ZB.MOM.WW.ScadaBridge.HealthMonitoring.Tests.csproj

@@ -11,6 +11,7 @@
   <ItemGroup>
     <PackageReference Include="coverlet.collector" />
     <PackageReference Include="Microsoft.Data.Sqlite" />
+    <PackageReference Include="Microsoft.Extensions.DependencyInjection" />
     <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
     <PackageReference Include="Microsoft.Extensions.Options" />
     <PackageReference Include="Microsoft.NET.Test.Sdk" />