From 2cea1a0687af1afef1c41ef8027d444637f3a00b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 7 Oct 2026 21:36:37 -0400 Subject: [PATCH] =?UTF-8?q?fix(ab-transport):=20final=20review=20I2=20?= =?UTF-8?q?=E2=80=94=20wire=20DeviceSessionPool.Shared=20to=20the=20host?= =?UTF-8?q?=20logger?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConfigureSharedLogger had no production caller, so the pool's only diagnostics (first-wins options conflicts, scheduler/close failures) went to NullLogger. DriverFactoryBootstrap now configures it from the driver registry factory and from the AbCip/AbLegacy probe factories (admin-only nodes reach the pool only through Test Connect). Wiring tests reset the pool to a null logger first, so removing the call goes red (verified). Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- .../DeviceSessionPool.cs | 3 + ....MOM.WW.OtOpcUa.Driver.Ab.Transport.csproj | 1 + .../Drivers/DriverFactoryBootstrap.cs | 28 ++++- .../AbCipDriverDefaultsOptionsTests.cs | 1 + .../AbTransportSharedLoggerWiringTests.cs | 108 ++++++++++++++++++ 5 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbTransportSharedLoggerWiringTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs index 21cfd740..e2a2db66 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/DeviceSessionPool.cs @@ -49,6 +49,9 @@ public sealed class DeviceSessionPool Shared._logger = loggerFactory.CreateLogger(); } + /// Wiring-test hook: the logger this pool currently writes to (final review I2). + internal ILogger Logger => _logger; + /// Test hook: keys with a live entry. internal int EntryCount { get { lock (_lock) return _entries.Count; } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.csproj index c15cdd13..cdf4d90e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.csproj @@ -18,5 +18,6 @@ + diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs index fd2ec172..dc687cf8 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Drivers/DriverFactoryBootstrap.cs @@ -80,6 +80,9 @@ public static class DriverFactoryBootstrap // whole mechanism exists to prevent, and it would fail as a plant outage rather than as a // startup error. var primaryRoleSource = sp.GetRequiredService(); + // Final review I2: the native AB transport's process-wide pool logs options conflicts and close + // failures; without this it writes to NullLogger in production. + ConfigureAbTransportLogging(sp); Register(registry, loggerFactory, secretResolver, primaryRoleSource, ResolveAbCipDefaultTransport(sp), scriptRoot); return registry; @@ -148,8 +151,16 @@ public static class DriverFactoryBootstrap // driver would run. The generic factory overload keeps AbCipProbe as the implementation type, which // TryAddEnumerable de-dups on. services.TryAddEnumerable(ServiceDescriptor.Singleton( - sp => new AbCipProbe(tagFactory: null, defaultTransport: ResolveAbCipDefaultTransport(sp)))); - services.TryAddEnumerable(ServiceDescriptor.Singleton()); + sp => + { + ConfigureAbTransportLogging(sp); // admin-only nodes reach the pool through the probes alone + return new AbCipProbe(tagFactory: null, defaultTransport: ResolveAbCipDefaultTransport(sp)); + })); + services.TryAddEnumerable(ServiceDescriptor.Singleton(sp => + { + ConfigureAbTransportLogging(sp); + return new AbLegacyProbe(); + })); services.TryAddEnumerable(ServiceDescriptor.Singleton()); services.TryAddEnumerable(ServiceDescriptor.Singleton()); services.TryAddEnumerable(ServiceDescriptor.Singleton()); @@ -163,6 +174,19 @@ public static class DriverFactoryBootstrap return services; } + /// + /// Gives the native AB transport's process-wide DeviceSessionPool.Shared the host logger factory + /// (final review I2). Called from every DI factory that can put the pool to use on this node (the driver + /// registry, and the AbCip/AbLegacy probes for admin-only nodes); idempotent. A container without an + /// leaves the pool on its null logger. + /// + /// The service provider. + internal static void ConfigureAbTransportLogging(IServiceProvider sp) + { + if (sp.GetService() is { } loggerFactory) + Driver.Ab.Transport.DeviceSessionPool.ConfigureSharedLogger(loggerFactory); + } + /// /// The node-level AbCip default transport (Drivers:AbCip:DefaultTransport, bound + validated in /// Program.cs). A container without the options bound (test graphs) gets the compiled default. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbCipDriverDefaultsOptionsTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbCipDriverDefaultsOptionsTests.cs index 66d23e97..9565cdc2 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbCipDriverDefaultsOptionsTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbCipDriverDefaultsOptionsTests.cs @@ -20,6 +20,7 @@ namespace ZB.MOM.WW.OtOpcUa.Host.Tests.Configuration; /// node (Libplctag) against the compiled default (Native), so a wiring that ignores the node value goes red. /// [Trait("Category", "Unit")] +[Collection(AbTransportSharedPoolCollection.Name)] // resolves the registry, which configures the shared pool public sealed class AbCipDriverDefaultsOptionsTests { private const string NoKey = """{"devices":[{"hostAddress":"ab://10.0.0.1/1,0"}],"probe":{"enabled":false}}"""; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbTransportSharedLoggerWiringTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbTransportSharedLoggerWiringTests.cs new file mode 100644 index 00000000..4e4c9585 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.Tests/Configuration/AbTransportSharedLoggerWiringTests.cs @@ -0,0 +1,108 @@ +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Core.Hosting; +using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport; +using ZB.MOM.WW.OtOpcUa.Host.Drivers; +using ZB.MOM.WW.Secrets.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Host.Tests.Configuration; + +/// The shared pool is process-wide, so the tests that touch it must not run in parallel. +[CollectionDefinition(Name, DisableParallelization = true)] +public sealed class AbTransportSharedPoolCollection +{ + public const string Name = "AbTransportSharedPool"; +} + +/// +/// Final review I2: DeviceSessionPool.Shared must log through the HOST logger factory, on driver +/// nodes (via the driver registry) and on admin-only nodes (via the AbCip / AbLegacy Test Connect probes). +/// Each test first resets the pool to a null logger and asserts that baseline, so deleting the wiring +/// call turns it red (the logger would stay the reset one). +/// +[Trait("Category", "Unit")] +[Collection(AbTransportSharedPoolCollection.Name)] +public sealed class AbTransportSharedLoggerWiringTests +{ + private static RecordingLoggerFactory Reset() + { + DeviceSessionPool.ConfigureSharedLogger(NullLoggerFactory.Instance); + var factory = new RecordingLoggerFactory(); + AssertPoolLogsTo(factory, expected: false); // baseline: the wiring under test has not run yet + return factory; + } + + // Logger wraps the factory's logger, so identity cannot be compared: emit through the pool's logger + // and check whether it lands in the recording factory under the pool's category. + private static void AssertPoolLogsTo(RecordingLoggerFactory factory, bool expected) + { + var marker = Guid.NewGuid().ToString("N"); + DeviceSessionPool.Shared.Logger.LogWarning("wiring-probe {Marker}", marker); + factory.Messages(typeof(DeviceSessionPool).FullName!).Any(m => m.Contains(marker)).ShouldBe(expected); + } + + [Fact] + public void Resolving_the_driver_registry_wires_the_pool_to_the_host_logger_factory() + { + var factory = Reset(); + var services = new ServiceCollection(); + services.AddSingleton(factory); + services.AddSingleton(new StubSecretResolver()); + services.AddOtOpcUaDriverFactories(); + using var sp = services.BuildServiceProvider(); + + sp.GetRequiredService(); + + AssertPoolLogsTo(factory, expected: true); + } + + [Theory] + [InlineData(DriverTypeNames.AbCip)] + [InlineData(DriverTypeNames.AbLegacy)] + public void An_admin_only_node_wires_the_pool_through_the_AB_probes(string driverType) + { + var factory = Reset(); + var services = new ServiceCollection(); + services.AddSingleton(factory); + services.AddOtOpcUaDriverProbes(); // the admin-only path: no driver registry + using var sp = services.BuildServiceProvider(); + + sp.GetServices().Single(p => p.DriverType == driverType); + + AssertPoolLogsTo(factory, expected: true); + } + + private sealed class RecordingLoggerFactory : ILoggerFactory + { + private readonly List<(string Category, string Message)> _messages = []; + + public IReadOnlyList Messages(string category) + { + lock (_messages) return [.. _messages.Where(m => m.Category == category).Select(m => m.Message)]; + } + + public ILogger CreateLogger(string categoryName) => new Recording(this, categoryName); + public void AddProvider(ILoggerProvider provider) { } + public void Dispose() { } + + private sealed class Recording(RecordingLoggerFactory owner, string category) : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + lock (owner._messages) owner._messages.Add((category, formatter(state, exception))); + } + } + } + + private sealed class StubSecretResolver : ISecretResolver + { + public Task GetAsync(SecretName name, CancellationToken ct) => Task.FromResult(null); + } +}