From 9d668861adfed07ea09eadea84ad32388c283320 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 15 Aug 2026 21:24:35 -0400 Subject: [PATCH] =?UTF-8?q?test(host):=20row=2062=20=E2=80=94=20pin=20ISit?= =?UTF-8?q?eEventLogger's=20Singleton=20lifetime=20in=20the=20site=20graph?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six SiteRuntime call sites resolve ISiteEventLogger straight off the root IServiceProvider with no scope: AlarmActor (cached in a field for the actor's lifetime), ScriptActor (x2), InstanceActor (x2), NativeAlarmActor, DeploymentManagerActor, and Scripts/ScriptRunLauncher. All correct today, and correct for exactly one reason — the registration is AddSingleton, so a root resolve yields what a scope would. That makes the lifetime a load-bearing invariant with no enforcement anywhere. If it ever changes, all six silently become register row 45, and nothing in the tree notices: ValidateOnBuild walks CONSTRUCTOR graphs and these are imperative GetService calls in method bodies; ValidateScopes would fire, but at runtime on a site node on whichever path logs an event first, not at boot and not in CI; CaptiveDependencyGuard catches the AlarmActor field case by design but cannot see the five per-call resolves at all. This pin is the tripwire, and its failure message names the six call sites that must be fixed alongside any such change. Asserts the REGISTRATION, not the call sites — the SiteRuntime actor files are correct as written and are untouched here. Two facts, not one, because the interface registration is a FACTORY (AddSingleton(sp => sp.GetRequiredService())): a Singleton lifetime on the interface alone does not prove callers share an instance, since downgrading the concrete SiteEventLogger would make that singleton factory resolve a scoped service from the root. Fail-before, both halves, by temporarily flipping the real registration: - AddScoped -> first pin FAILS, "Actual lifetime: Scoped", with the full row-62 pointer in the message. - AddScoped (interface left Singleton) -> the first pin stays GREEN and only the second catches it. That is the evidence the second fact is load-bearing rather than redundant. Registration restored; both green. Registration-only inspection — the provider is never built, so the pin costs ~30 ms and opens no SQLite file. Host.Tests: 507/507 green. --- .../SiteEventLoggerLifetimePinTests.cs | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs new file mode 100644 index 00000000..1f17c8a9 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/SiteEventLoggerLifetimePinTests.cs @@ -0,0 +1,146 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.ScadaBridge.SiteEventLogging; + +namespace ZB.MOM.WW.ScadaBridge.Host.Tests; + +/// +/// Deferred-work register row 62 — hardening pin, not a bug fix. +/// +/// +/// Six SiteRuntime call sites reach ISiteEventLogger straight off the injected +/// , with no scope in between: +/// +/// +/// SiteRuntime/Actors/AlarmActor.cs — resolves once in the +/// constructor and CACHES it in _siteEventLogger, so it holds the instance for +/// the actor's whole lifetime. +/// SiteRuntime/Actors/ScriptActor.cs (two sites) — per-call +/// resolve. +/// SiteRuntime/Actors/InstanceActor.cs (two sites) — per-call +/// resolve. +/// SiteRuntime/Actors/NativeAlarmActor.cs — per-call +/// resolve. +/// SiteRuntime/Actors/DeploymentManagerActor.cs — per-call +/// resolve. +/// SiteRuntime/Scripts/ScriptRunLauncher.cs — per-run resolve. +/// Not among the five the register row named: it is the run body WP3.1 lifted verbatim +/// out of the deleted ScriptExecutionActor/AlarmExecutionActor, so it inherited the +/// pattern rather than adding it. +/// +/// +/// +/// Every one of those is CORRECT TODAY, and this test is the reason it stays that way. +/// They are correct for exactly one reason: ISiteEventLogger is registered +/// , so resolving it from the root provider yields +/// the same instance a scope would. The day that registration changes lifetime, every one +/// of those call sites silently becomes register row 45 — a captive dependency / +/// resolved-outside-its-scope bug — and nothing else in the tree would notice: +/// +/// +/// ValidateScopes catches a SCOPED service resolved from the +/// root provider — but the actors hold the root provider legitimately, and .NET's +/// check fires at the offending GetService call, i.e. at runtime on a site +/// node, on whatever code path happens to log an event first. Not at boot, and not +/// in CI. +/// ValidateOnBuild walks CONSTRUCTOR graphs. These are +/// imperative GetService calls inside method bodies, invisible to it. +/// CaptiveDependencyGuard catches the AlarmActor field case +/// (a cached resolve) by design, but cannot see the five per-call resolves at +/// all. +/// +/// +/// +/// So the lifetime is a load-bearing invariant with no other enforcement. Pinning it here +/// means a future lifetime change fails loudly, in CI, with a message naming the call +/// sites that have to be fixed alongside it — rather than shipping and being found on the +/// rig months later, which is how row 45 was found. +/// +/// +/// +/// This deliberately asserts the REGISTRATION, not the call sites: the SiteRuntime actor +/// files are correct as written and need no change. The pin is a tripwire on the +/// assumption they rest on. +/// +/// +public class SiteEventLoggerLifetimePinTests +{ + /// + /// The failure message is the whole point of the test — it has to carry the reader + /// from "a lifetime assertion failed" to "here is what breaks and why". + /// + private const string Why = + "ISiteEventLogger MUST be registered Singleton in the site service graph " + + "(deferred-work register row 62).\n" + + "Six SiteRuntime call sites resolve it straight off the root IServiceProvider with " + + "no scope: AlarmActor (cached in a field for the actor's lifetime), ScriptActor (x2), " + + "InstanceActor (x2), NativeAlarmActor, DeploymentManagerActor, and " + + "Scripts/ScriptRunLauncher.\n" + + "Under any non-singleton lifetime every one of those becomes a captive-dependency / " + + "resolved-outside-its-scope bug (register row 45), and NOTHING else catches it: " + + "ValidateOnBuild only walks constructor graphs, ValidateScopes fires at runtime on a " + + "site node rather than at boot, and CaptiveDependencyGuard sees only the AlarmActor " + + "field case.\n" + + "If this lifetime change is intended, fix those six call sites to resolve through an " + + "explicit scope in the same change, then update this pin."; + + /// + /// Builds the real Site service graph the way Program.cs's Site branch does and + /// asserts the lifetime of the registration the SiteRuntime root-resolves depend on. + /// Registration only — the provider is never built, so this costs nothing and touches + /// no SQLite file. + /// + /// The configured site service collection. + private static IServiceCollection BuildSiteServiceGraph() + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["ScadaBridge:Node:Role"] = "Site", + ["ScadaBridge:Node:NodeName"] = "node-a", + ["ScadaBridge:Node:NodeHostname"] = "test-site", + ["ScadaBridge:Node:SiteId"] = "TestSite", + ["ScadaBridge:Node:RemotingPort"] = "0", + ["ScadaBridge:Node:GrpcPort"] = "0", + ["LocalDb:Path"] = "/tmp/sb-row62-pin-never-opened.db", + ["AuditLog:SiteWriter:DatabasePath"] = "/tmp/sb-row62-pin-never-opened-audit.db", + ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", + ["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552", + }) + .Build(); + + var services = new ServiceCollection(); + SiteServiceRegistration.Configure(services, configuration); + return services; + } + + [Fact] + public void ISiteEventLogger_IsRegisteredSingleton_InTheSiteServiceGraph() + { + var services = BuildSiteServiceGraph(); + + var descriptor = Assert.Single(services, d => d.ServiceType == typeof(ISiteEventLogger)); + + Assert.True(descriptor.Lifetime == ServiceLifetime.Singleton, + $"{Why}\nActual lifetime: {descriptor.Lifetime}."); + } + + [Fact] + public void SiteEventLogger_TheConcreteTypeTheInterfaceForwardsTo_IsAlsoSingleton() + { + // The interface registration is a FACTORY — + // AddSingleton(sp => sp.GetRequiredService()) + // — so a Singleton lifetime on ISiteEventLogger alone does not prove callers get a + // shared instance. Downgrading the concrete SiteEventLogger to Scoped would leave + // the assertion above passing while making the singleton factory resolve a scoped + // service from the root: the exact bug, with the pin still green. Both halves have + // to be pinned for the invariant to actually hold. + var services = BuildSiteServiceGraph(); + + var descriptor = Assert.Single(services, d => d.ServiceType == typeof(SiteEventLogger)); + + Assert.True(descriptor.Lifetime == ServiceLifetime.Singleton, + $"{Why}\nISiteEventLogger forwards to the concrete SiteEventLogger, whose actual " + + $"lifetime is: {descriptor.Lifetime}."); + } +}