fix(dcl): async OPC UA endpoint discovery bounded by OperationTimeoutMs/verify budget; adapter connect never runs on the actor thread (review 2026-10-21 02/P2)

2026-10-22 06:37:23 -04:00
parent cfe95a7e5a
commit e6406c494c
6 changed files with 170 additions and 28 deletions
@@ -71,7 +71,7 @@ All protocols produce the same value tuple consumed by Instance Actors. Before t

 ### OPC UA
 - Uses the **OPC Foundation .NET Standard Library** (`OPCFoundation.NetStandard.Opc.Ua.Client`).
- Session-based connection with endpoint discovery, certificate handling, and configurable security modes.
+- Session-based connection with endpoint discovery, certificate handling, and configurable security modes. Endpoint discovery is **asynchronous** and bounded by the connection's `OperationTimeoutMs` (the discovery client is built from the connection's own `EndpointConfiguration`) and by the caller's cancellation; a cancelled discovery is never mistaken for "discovery unavailable". The `DataConnectionActor` starts every adapter `ConnectAsync` on the thread pool, so no adapter connect work — whatever runs before its first yielding await — can occupy the actor's dispatcher thread (review 2026-10-21; previously a synchronous discovery against a black-holed endpoint held the thread for the SDK's 120 s default on every reconnect tick).
 - Subscriptions via OPC UA Monitored Items with data change notifications (1000ms sampling, queue size 10, discard-oldest).
 - Read/Write via OPC UA Read/Write services with StatusCode-based quality mapping.
 - Disconnect detection via `Session.KeepAlive` event (see Disconnect Detection Pattern below).
@@ -282,7 +282,7 @@ IAddressSpaceSearchable

 Before saving or deploying an OPC UA data connection, the Central UI can ask the target site to **probe** the configured endpoint — this exercises connectivity (and TLS trust) against the live config, including edited-but-unsaved values.

- `DataConnectionManagerActor` handles `VerifyEndpointCommand` (`SiteId`, protocol, config JSON). The site spins up a **temporary** `RealOpcUaClient` from the submitted config, attempts discovery + a session with a short timeout (a few seconds), then disconnects and disposes the client. The reply is a `VerifyEndpointResult` (`Success`, a typed `FailureKind`, an error message, and an optional captured server cert).
+- `DataConnectionManagerActor` handles `VerifyEndpointCommand` (`SiteId`, protocol, config JSON). The site spins up a **temporary** `RealOpcUaClient` from the submitted config, attempts discovery + a session with a short timeout (a few seconds — the budget bounds discovery too, not only the session create), then disconnects and disposes the client. The reply is a `VerifyEndpointResult` (`Success`, a typed `FailureKind`, an error message, and an optional captured server cert).
 - The probe forces **`AutoAcceptUntrustedCerts = false`** and hooks the certificate-validation callback so it can **capture** an untrusted server certificate (Subject / Issuer / Thumbprint / NotBefore / NotAfter / DER) for the UI to display. Critically, the probe **never trusts** the cert — the validation callback always rejects (`Accept = false`); trusting is a separate, explicit, Admin-gated action that writes the cert into the site PKI store (see the cert-trust path in Component-SiteRuntime.md). The probe is read-only with respect to the trust boundary.

 ## Native Alarm Mirroring
@@ -957,7 +957,14 @@ public class DataConnectionActor : UntypedActor, IWithTimers
        // is still in flight swaps the adapter and bumps the counter, and the OUTGOING
        // adapter's answer must never be applied to its replacement.
        var generation = _adapterGeneration;
-        _adapter.ConnectAsync(_connectionDetails).ContinueWith(t =>
+        // Started on the thread pool, never inline: everything an adapter's ConnectAsync does
+        // before its first YIELDING await would otherwise run on this actor's dispatcher thread
+        // — OPC UA endpoint discovery was synchronous there and parked the thread for the SDK's
+        // operation timeout on every reconnect tick to a black-holed endpoint (review 2026-10-21
+        // 02/P2). Adapter and details are captured here, on the actor thread.
+        var adapter = _adapter;
+        var details = _connectionDetails;
+        Task.Run(() => adapter.ConnectAsync(details)).ContinueWith(t =>
        {
            if (t.IsCompletedSuccessfully)
                return new ConnectResult(true, null, generation);
@@ -96,19 +96,42 @@ public class RealOpcUaClient : IOpcUaClient
    /// <inheritdoc />
    public bool IsConnected => _session?.Connected ?? false;

+    /// <summary>
+    /// Asynchronous endpoint discovery bounded by <paramref name="appConfig"/>'s
+    /// <c>TransportQuotas.OperationTimeout</c> (the connection's <c>OperationTimeoutMs</c>) and by
+    /// <paramref name="ct"/> (review 2026-10-21 02/P2). It replaces the synchronous
+    /// <c>DiscoveryClient.Create(uri).GetEndpoints(null)</c>, which ran on whatever thread called
+    /// the connect — the actor thread, when <c>ValidateAsync</c> completed synchronously — under
+    /// the SDK's default 120 s operation timeout, and could not be cancelled by a verify probe's
+    /// budget.
+    /// </summary>
+    private static async Task<EndpointDescriptionCollection> DiscoverEndpointsAsync(
+        ApplicationConfiguration appConfig, string endpointUrl, CancellationToken ct)
+    {
+        var discoveryConfig = EndpointConfiguration.Create(appConfig);
+        using var discoveryClient = await DiscoveryClient.CreateAsync(
+            appConfig, new Uri(endpointUrl), discoveryConfig, DiagnosticsMasks.None, ct).ConfigureAwait(false);
+        return await discoveryClient.GetEndpointsAsync(null, ct).ConfigureAwait(false);
+    }
+
    /// <summary>Raised when the OPC UA connection is lost.</summary>
    public event Action? ConnectionLost;

    /// <summary>Selects exactly the requested protection mode; protected discovery failures fail closed.</summary>
    /// <param name="url">The discovery URL to query.</param>
    /// <param name="requested">The security mode that must be matched exactly.</param>
-    /// <param name="discover">Delegate that returns the server's advertised endpoints.</param>
+    /// <param name="discover">Delegate that returns the server's advertised endpoints, honouring the token.</param>
+    /// <param name="ct">
+    /// Bounds the discovery. A cancellation is rethrown — never mistaken for "discovery
+    /// unavailable", which would otherwise fall back to an unsecured endpoint and carry on.
+    /// </param>
    /// <returns>The endpoint matching the requested security mode.</returns>
-    internal static EndpointDescription DiscoverEndpoint(string url, MessageSecurityMode requested,
-        Func<EndpointDescriptionCollection> discover)
+    internal static async Task<EndpointDescription> DiscoverEndpointAsync(string url, MessageSecurityMode requested,
+        Func<CancellationToken, Task<EndpointDescriptionCollection>> discover, CancellationToken ct = default)
    {
        EndpointDescriptionCollection endpoints;
-        try { endpoints = discover(); }
+        try { endpoints = await discover(ct).ConfigureAwait(false); }
+        catch (OperationCanceledException) when (ct.IsCancellationRequested) { throw; }
        catch (Exception ex)
        {
            if (requested != MessageSecurityMode.None)
@@ -163,13 +186,8 @@ public class RealOpcUaClient : IOpcUaClient
        }

        // Discover endpoints from the server, pick the preferred security mode
-        var endpoint = DiscoverEndpoint(endpointUrl, preferredSecurityMode, () =>
-        {
-#pragma warning disable CS0618
-            using var discoveryClient = DiscoveryClient.Create(new Uri(endpointUrl));
-            return discoveryClient.GetEndpoints(null);
-#pragma warning restore CS0618
-        });
+        var endpoint = await DiscoverEndpointAsync(endpointUrl, preferredSecurityMode,
+            token => DiscoverEndpointsAsync(appConfig, endpointUrl, token), cancellationToken);

        // The server advertises its own base address, which is frequently unroutable from here
        // (a 0.0.0.0 wildcard bind, or an internal container/NAT hostname). Swap it back to the
@@ -391,13 +409,9 @@ public class RealOpcUaClient : IOpcUaClient
            await appConfig.ValidateAsync(ApplicationType.Client);

            // Discover endpoints, pick the preferred security mode (same logic as ConnectAsync).
-            var endpoint = DiscoverEndpoint(endpointUrl, preferredSecurityMode, () =>
-            {
-#pragma warning disable CS0618
-                using var discoveryClient = DiscoveryClient.Create(new Uri(endpointUrl));
-                return discoveryClient.GetEndpoints(null);
-#pragma warning restore CS0618
-            });
+            // Bounded by the probe's own budget (linkedCts), not just the SDK operation timeout.
+            var endpoint = await DiscoverEndpointAsync(endpointUrl, preferredSecurityMode,
+                token => DiscoverEndpointsAsync(appConfig, endpointUrl, token), linkedCts.Token);

            // Same reachability rewrite as ConnectAsync — a probe against a server advertising a
            // 0.0.0.0 / NAT base address must dial the reachable host, not the advertised one.
@@ -126,4 +126,40 @@ public class DataConnectionHousekeepingTests : TestKit
                && message.Contains("established", StringComparison.OrdinalIgnoreCase)),
            TimeSpan.FromSeconds(5));
    }
+
+    /// <summary>
+    /// 02/P2: an adapter whose connect does synchronous work before its first yielding await
+    /// (OPC UA discovery used to) must not park the actor's dispatcher thread — the actor keeps
+    /// answering while the connect is in progress.
+    /// </summary>
+    [Fact]
+    public async Task AdapterConnect_NeverRunsOnTheActorThread()
+    {
+        const string name = "connect-off-actor-thread";
+        var adapter = Substitute.For<IDataConnection>();
+        using var release = new ManualResetEventSlim(false);
+        adapter.ConnectAsync(Arg.Any<IDictionary<string, string>>(), Arg.Any<CancellationToken>())
+            .Returns(_ =>
+            {
+                release.Wait(TimeSpan.FromSeconds(10)); // synchronous I/O before any await
+                return Task.CompletedTask;
+            });
+        adapter.Status.Returns(ConnectionHealth.Connecting);
+
+        var actor = Sys.ActorOf(Props.Create(() => new DataConnectionActor(
+            name, adapter, Options(), _health, _factory, "OpcUa")), name);
+        AwaitCondition(() => adapter.ReceivedCalls().Any(c => c.GetMethodInfo().Name == nameof(IDataConnection.ConnectAsync)),
+            TimeSpan.FromSeconds(5));
+
+        try
+        {
+            var report = await actor.Ask<DataConnectionHealthReport>(
+                new DataConnectionActor.GetHealthReport(), TimeSpan.FromSeconds(2));
+            Assert.Equal(name, report.ConnectionName);
+        }
+        finally
+        {
+            release.Set();
+        }
+    }
 }
@@ -8,19 +8,19 @@ public class OpcUaSecurityDowngradeTests
    [Theory]
    [InlineData(false)]
    [InlineData(true)]
-    public void DC4_RequestedProtectionNeverFallsBackToNone(bool discoveryFails)
+    public async Task DC4_RequestedProtectionNeverFallsBackToNone(bool discoveryFails)
    {
-        Assert.Throws<InvalidOperationException>(() => RealOpcUaClient.DiscoverEndpoint("opc.tcp://test:4840",
-            MessageSecurityMode.SignAndEncrypt, () => discoveryFails
+        await Assert.ThrowsAsync<InvalidOperationException>(() => RealOpcUaClient.DiscoverEndpointAsync("opc.tcp://test:4840",
+            MessageSecurityMode.SignAndEncrypt, _ => discoveryFails
                ? throw new Exception("discovery unavailable")
-                : new EndpointDescriptionCollection { new("opc.tcp://test:4840") { SecurityMode = MessageSecurityMode.None } }));
+                : Task.FromResult(new EndpointDescriptionCollection { new("opc.tcp://test:4840") { SecurityMode = MessageSecurityMode.None } })));
    }
    /// <summary>Verifies that DC4: exact requested mode is selected.</summary>
    [Fact]
-    public void DC4_ExactRequestedModeIsSelected()
+    public async Task DC4_ExactRequestedModeIsSelected()
    {
        var encrypted = new EndpointDescription("opc.tcp://test:4840") { SecurityMode = MessageSecurityMode.SignAndEncrypt };
-        Assert.Same(encrypted, RealOpcUaClient.DiscoverEndpoint("opc.tcp://test:4840", MessageSecurityMode.SignAndEncrypt,
-            () => new EndpointDescriptionCollection { new("opc.tcp://test:4840"), encrypted }));
+        Assert.Same(encrypted, await RealOpcUaClient.DiscoverEndpointAsync("opc.tcp://test:4840", MessageSecurityMode.SignAndEncrypt,
+            _ => Task.FromResult(new EndpointDescriptionCollection { new("opc.tcp://test:4840"), encrypted })));
    }
 }
@@ -0,0 +1,85 @@
+using System.Diagnostics;
+using System.Net;
+using System.Net.Sockets;
+using Microsoft.Extensions.Logging.Abstractions;
+using ZB.MOM.WW.ScadaBridge.Commons.Types.DataConnections;
+using ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Adapters;
+
+namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests.Adapters;
+
+/// <summary>
+/// Review 2026-10-21 02/P2: OPC UA endpoint discovery ran SYNCHRONOUSLY on a discovery client
+/// built with the SDK's default configuration, so neither the connection's
+/// <c>OperationTimeoutMs</c> nor the verify probe's budget bounded it — a black-holed endpoint held
+/// the caller for the SDK's default operation timeout instead.
+/// </summary>
+public class RealOpcUaDiscoveryBoundTests
+{
+    /// <summary>
+    /// A listener that accepts the TCP connection and then never speaks — the protocol-level
+    /// black hole. The verify probe must give up within (about) its own budget.
+    /// </summary>
+    [Fact]
+    public async Task Verify_AgainstABlackHoledEndpoint_ReturnsWithinItsBudget()
+    {
+        var listener = new TcpListener(IPAddress.Loopback, 0);
+        listener.Start();
+        var held = new List<TcpClient>();
+        using var stop = new CancellationTokenSource();
+        _ = Task.Run(async () =>
+        {
+            try
+            {
+                while (!stop.IsCancellationRequested)
+                    held.Add(await listener.AcceptTcpClientAsync(stop.Token));
+            }
+            catch (OperationCanceledException) { }
+            catch (ObjectDisposedException) { }
+        });
+
+        try
+        {
+            var port = ((IPEndPoint)listener.LocalEndpoint).Port;
+            var config = new OpcUaEndpointConfig
+            {
+                EndpointUrl = $"opc.tcp://127.0.0.1:{port}",
+                OperationTimeoutMs = 60_000,
+            };
+
+            var sw = Stopwatch.StartNew();
+            var verify = RealOpcUaClient.VerifyEndpointAsync(
+                config, new OpcUaGlobalOptions(), NullLogger.Instance, TimeSpan.FromSeconds(2), CancellationToken.None);
+            var result = await verify.WaitAsync(TimeSpan.FromSeconds(20));
+            sw.Stop();
+
+            Assert.False(result.Success);
+            Assert.True(sw.Elapsed < TimeSpan.FromSeconds(10), $"verify took {sw.Elapsed}");
+        }
+        finally
+        {
+            stop.Cancel();
+            listener.Stop();
+            foreach (var c in held) c.Dispose();
+        }
+    }
+
+    /// <summary>
+    /// Cancellation of discovery is never mistaken for "discovery unavailable": the unsecured
+    /// fallback endpoint is only for a genuine discovery failure, not for an abandoned attempt.
+    /// </summary>
+    [Fact]
+    public async Task CancelledDiscovery_IsNotTreatedAsADiscoveryFailure()
+    {
+        using var cts = new CancellationTokenSource();
+        cts.Cancel();
+
+        await Assert.ThrowsAnyAsync<OperationCanceledException>(() => RealOpcUaClient.DiscoverEndpointAsync(
+            "opc.tcp://test:4840", Opc.Ua.MessageSecurityMode.None,
+            async ct =>
+            {
+                await Task.Delay(Timeout.Infinite, ct);
+                return new Opc.Ua.EndpointDescriptionCollection();
+            },
+            cts.Token));
+    }
+}