diff --git a/docs/drivers/OpcUaClient.md b/docs/drivers/OpcUaClient.md index 66651886..a74de6f1 100644 --- a/docs/drivers/OpcUaClient.md +++ b/docs/drivers/OpcUaClient.md @@ -26,6 +26,7 @@ out-of-process isolation. |---------|--------|------| | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/` | net10.0 | In-process driver — session lifetime, read / write / subscribe / alarm / history passthrough | | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/` | net10.0 | `IDriverBrowser` — live address-picker browse used by the AdminUI. Its side-panel `AttributesAsync` reads a variable's DataType / ValueRank / UserAccessLevel and maps them through the same `UpstreamAttributeMapping` discovery uses; it authenticates Anonymous, Username **or Certificate** through the driver's own `OpcUaUserIdentityFactory`, resolving `secret:` credentials through the host's secret store (fail-closed) — review 2026-10-21 O-5 (it was a stub, and Certificate was refused) | +| | | ⚠️ Browse Certificate auth reads `UserCertificatePath` on the **AdminUI host's** filesystem, which may differ from the driver node's; the browse session disposes the certificate it loaded when it closes. | | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/` | net10.0 | The SDK-typed layer the driver AND the browser share: `NamespaceMap`, `UpstreamAttributeMapping`, `OpcUaUserIdentityFactory` | | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/` | net10.0 | Config records + enums bound from `DriverConfig` JSON | @@ -98,7 +99,13 @@ The session keep-alive (`KeepAliveInterval`, default 5 s) is the liveness signal only the stamp, so a subscription-only driver stayed `Degraded` until its next re-initialise). - **Read / write / ack responses are replies.** Any service response — per-node refusals such as `BadNotWritable` included — restores `Healthy` and advances the stamp; only a call that got **no** response (transport failure, - timeout) writes `Degraded` with the stamp preserved. A write the caller cancelled touches no health. + timeout — including an SDK request timeout surfacing as a cancellation the caller did not cause) writes `Degraded` + with the stamp preserved; acks follow the same rule. **Deliberate deviation:** a call the *caller* cancelled touches + no health — including a write the host cancelled at its own write deadline against a hung upstream; the session + keep-alive owns liveness and reports that outage itself. A reply that was in flight across a bad keep-alive advances + the stamp but does not write `Healthy` while the outage is published — the keep-alive recovery ends the outage. + An ack none of whose condition ids resolves sends nothing and fails `BadNodeIdUnknown`; with no session an ack fails + `BadNotConnected` (before or inside the gate alike). - **Bad keep-alive** ⇒ health `Degraded` (`LastError = "keep-alive: "`, read stamp preserved), host status `Stopped`, and — **once per outage**, not per keep-alive — every subscribed ref is published `BadNoCommunication` with its last value retained and **source + server timestamp = the detection time** (live-gate F1: stamped with the diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs index fa534016..9224a80a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientBrowseSession.cs @@ -17,6 +17,7 @@ internal sealed class OpcUaClientBrowseSession : IBrowseSession private readonly NodeId _rootNodeId; private readonly SemaphoreSlim _gate = new(1, 1); private volatile bool _disposed; + private readonly IDisposable? _ownedCredential; /// /// Construct a browse session bound to an already-connected . @@ -25,8 +26,10 @@ internal sealed class OpcUaClientBrowseSession : IBrowseSession /// Namespace snapshot taken at connect time; used to render outbound /// NodeIds in the server-stable nsu=… form. /// The node under which browses one level. - internal OpcUaClientBrowseSession(ISession session, NamespaceMap nsMap, NodeId rootNodeId) + /// A credential (the user certificate) this session owns and disposes with itself. + internal OpcUaClientBrowseSession(ISession session, NamespaceMap nsMap, NodeId rootNodeId, IDisposable? ownedCredential = null) { + _ownedCredential = ownedCredential; _session = session; _nsMap = nsMap; _rootNodeId = rootNodeId; @@ -242,5 +245,8 @@ internal sealed class OpcUaClientBrowseSession : IBrowseSession try { _gate.Dispose(); } catch { /* best-effort */ } + + try { _ownedCredential?.Dispose(); } + catch { /* best-effort */ } } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs index 845d2caa..da1411b1 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser/OpcUaClientDriverBrowser.cs @@ -69,55 +69,66 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser // R06:O-5 — Certificate auth is supported: the identity is built (and any secret: credential resolved) by the // same factory the runtime driver uses, BEFORE any network I/O, so a bad certificate path / password fails fast // with its own message. - var identity = await BuildBrowseUserIdentityAsync(opts, _secretResolver, cancellationToken).ConfigureAwait(false); - - if (opts.AutoAcceptCertificates) - _logger.LogWarning( - "AdminUI browse session opens against {Endpoint} with form's AutoAcceptCertificates=true — " + - "browse uses its own cert store and does NOT auto-accept; trust the cert via the runtime " + - "driver's PKI store instead.", - endpoint); - - var appConfig = await BuildBrowseAppConfigurationAsync(cancellationToken).ConfigureAwait(false); - - var perEndpointBudget = TimeSpan.FromSeconds( - Math.Clamp(opts.PerEndpointConnectTimeout.TotalSeconds, 5, 30)); - using var connectCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - connectCts.CancelAfter(perEndpointBudget); - - var endpointDesc = await SelectEndpointAsync( - appConfig, endpoint, opts.SecurityPolicy, opts.SecurityMode, connectCts.Token).ConfigureAwait(false); - var endpointCfg = EndpointConfiguration.Create(appConfig); - endpointCfg.OperationTimeout = (int)opts.Timeout.TotalMilliseconds; - var configuredEndpoint = new ConfiguredEndpoint(null, endpointDesc, endpointCfg); - - var session = await new DefaultSessionFactory(telemetry: null!).CreateAsync( - appConfig, - configuredEndpoint, - updateBeforeConnect: false, - sessionName: "OtOpcUa AdminUI Browse", - (uint)opts.SessionTimeout.TotalMilliseconds, - identity, - preferredLocales: null, - connectCts.Token).ConfigureAwait(false); + var (identity, ownedCertificate) = await BuildBrowseCredentialAsync(opts, _secretResolver, cancellationToken) + .ConfigureAwait(false); try { - var nsMap = NamespaceMap.FromSession(session); - var rootNodeId = string.IsNullOrEmpty(opts.BrowseRoot) - ? ObjectIds.ObjectsFolder - : NodeId.Parse(session.MessageContext, opts.BrowseRoot); + if (opts.AutoAcceptCertificates) + _logger.LogWarning( + "AdminUI browse session opens against {Endpoint} with form's AutoAcceptCertificates=true — " + + "browse uses its own cert store and does NOT auto-accept; trust the cert via the runtime " + + "driver's PKI store instead.", + endpoint); - _logger.LogInformation( - "AdminUI OPC UA browse session opened against {Endpoint} (policy {Policy}, mode {Mode})", - endpoint, opts.SecurityPolicy, opts.SecurityMode); + var appConfig = await BuildBrowseAppConfigurationAsync(cancellationToken).ConfigureAwait(false); - return new OpcUaClientBrowseSession(session, nsMap, rootNodeId); + var perEndpointBudget = TimeSpan.FromSeconds( + Math.Clamp(opts.PerEndpointConnectTimeout.TotalSeconds, 5, 30)); + using var connectCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + connectCts.CancelAfter(perEndpointBudget); + + var endpointDesc = await SelectEndpointAsync( + appConfig, endpoint, opts.SecurityPolicy, opts.SecurityMode, connectCts.Token).ConfigureAwait(false); + var endpointCfg = EndpointConfiguration.Create(appConfig); + endpointCfg.OperationTimeout = (int)opts.Timeout.TotalMilliseconds; + var configuredEndpoint = new ConfiguredEndpoint(null, endpointDesc, endpointCfg); + + var session = await new DefaultSessionFactory(telemetry: null!).CreateAsync( + appConfig, + configuredEndpoint, + updateBeforeConnect: false, + sessionName: "OtOpcUa AdminUI Browse", + (uint)opts.SessionTimeout.TotalMilliseconds, + identity, + preferredLocales: null, + connectCts.Token).ConfigureAwait(false); + + try + { + var nsMap = NamespaceMap.FromSession(session); + var rootNodeId = string.IsNullOrEmpty(opts.BrowseRoot) + ? ObjectIds.ObjectsFolder + : NodeId.Parse(session.MessageContext, opts.BrowseRoot); + + _logger.LogInformation( + "AdminUI OPC UA browse session opened against {Endpoint} (policy {Policy}, mode {Mode})", + endpoint, opts.SecurityPolicy, opts.SecurityMode); + + return new OpcUaClientBrowseSession(session, nsMap, rootNodeId, ownedCertificate); + } + catch + { + try { if (session is Session s) await s.CloseAsync().ConfigureAwait(false); } catch { /* best-effort */ } + try { session.Dispose(); } catch { /* best-effort */ } + throw; + } } catch { - try { if (session is Session s) await s.CloseAsync().ConfigureAwait(false); } catch { /* best-effort */ } - try { session.Dispose(); } catch { /* best-effort */ } + // Review m9: the certificate (and its private-key handle) is owned by the browse session once handed + // over; any failure before that releases it here instead of leaving it to the GC. + ownedCertificate?.Dispose(); throw; } } @@ -192,10 +203,29 @@ public sealed class OpcUaClientDriverBrowser : IDriverBrowser /// Cancellation token for the secret resolution. /// The identity the browse session activates with. internal static async Task BuildBrowseUserIdentityAsync( - OpcUaClientDriverOptions opts, ISecretResolver? secretResolver, CancellationToken ct) + OpcUaClientDriverOptions opts, ISecretResolver? secretResolver, CancellationToken ct) => + (await BuildBrowseCredentialAsync(opts, secretResolver, ct).ConfigureAwait(false)).Identity; + + /// + /// As , also handing back the user certificate a Certificate-auth + /// identity holds, so the browse session that uses it can dispose it (review m9: the browse opens one per picker + /// session, and an undisposed X509Certificate2 pins a private-key handle until GC). + /// + /// Driver options carrying the form's auth fields. + /// The secret store, or null when none is available. + /// Cancellation token for the secret resolution. + /// The identity, and the certificate it owns (null for Anonymous / Username). + internal static async Task<(UserIdentity Identity, System.Security.Cryptography.X509Certificates.X509Certificate2? Certificate)> + BuildBrowseCredentialAsync(OpcUaClientDriverOptions opts, ISecretResolver? secretResolver, CancellationToken ct) { var resolved = await OpcUaUserIdentityFactory.ResolveSecretRefsAsync(opts, secretResolver, ct).ConfigureAwait(false); - return OpcUaUserIdentityFactory.Build(resolved); + if (resolved.AuthType != OpcUaAuthType.Certificate) + { + return (OpcUaUserIdentityFactory.Build(resolved), null); + } + + var certificate = OpcUaUserIdentityFactory.LoadCertificate(resolved); + return (new UserIdentity(certificate), certificate); } /// Select the endpoint matching the requested SecurityPolicy + SecurityMode pair. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserIdentityFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserIdentityFactory.cs index 16be7eed..29510067 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserIdentityFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session/OpcUaUserIdentityFactory.cs @@ -37,7 +37,15 @@ public static class OpcUaUserIdentityFactory /// /// Driver options containing certificate configuration. /// The certificate-backed . - public static UserIdentity FromCertificate(OpcUaClientDriverOptions options) + public static UserIdentity FromCertificate(OpcUaClientDriverOptions options) => new(LoadCertificate(options)); + + /// + /// Loads (and validates) the user-authentication certificate wraps. The caller owns + /// the returned certificate — dispose it when the session that presented it is gone. + /// + /// Driver options containing certificate configuration. + /// The certificate, with its private key. + public static System.Security.Cryptography.X509Certificates.X509Certificate2 LoadCertificate(OpcUaClientDriverOptions options) { if (string.IsNullOrWhiteSpace(options.UserCertificatePath)) throw new InvalidOperationException( @@ -53,11 +61,14 @@ public static class OpcUaUserIdentityFactory .LoadPkcs12FromFile(options.UserCertificatePath, options.UserCertificatePassword); if (!cert.HasPrivateKey) + { + cert.Dispose(); throw new InvalidOperationException( $"User certificate at '{options.UserCertificatePath}' has no private key — " + "the private key is required to sign the OPC UA user-token challenge at session activation."); + } - return new UserIdentity(cert); + return cert; } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index 0bbd14ae..480c81e2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -473,6 +473,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit WriteLifecycleHealth(generation, _ => new DriverHealth(DriverState.Initializing, null, null)); ISession? session = null; KeepAliveEventHandler? keepAliveHandler = null; + var certValidation = new CertValidationHook(); var installed = false; try { @@ -481,11 +482,18 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit // with — the endpoint, security policy and tag set were all frozen at construction, and only // secret rotation was picked up. if (HasConfigBody(driverConfigJson)) - _options = OpcUaClientDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); + { + var parsed = OpcUaClientDriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); + // Review m2: a superseded init must not overwrite the options a newer one adopted. + lock (_probeLock) + { + if (Volatile.Read(ref _lifecycleGeneration) == generation) _options = parsed; + } + } ValidateTuning(_options); - var appConfig = await BuildApplicationConfigurationAsync(cancellationToken).ConfigureAwait(false); + var appConfig = await BuildApplicationConfigurationAsync(certValidation, cancellationToken).ConfigureAwait(false); // when a DiscoveryUrl is configured, FindServers/GetEndpoints on it first and try the // discovered URLs ahead of the static list. Non-fatal — an LDS outage must not block init. @@ -587,6 +595,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit Session = session; _keepAliveHandler = keepAliveHandler; _connectedEndpointUrl = connectedUrl; + // Review m2: the cert-validation handler is published with the session — a previous one (a + // re-initialise without shutdown) is detached first so it is not leaked on its validator. + DetachCertValidation(_certValidatorRef, _certValidationHandler); + _certValidationHandler = certValidation.Handler; + _certValidatorRef = certValidation.Validator; installed = true; } } @@ -607,6 +620,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit // superseded check) does not orphan it and its keep-alive timer until GC. if (session is not null && !installed) await DiscardSessionAsync(session, keepAliveHandler).ConfigureAwait(false); + if (!installed) DetachCertValidation(certValidation.Validator, certValidation.Handler); if (Volatile.Read(ref _lifecycleGeneration) == generation) { if (installed) @@ -618,6 +632,19 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit } } + /// The auto-accept certificate-validation handler one initialise attached, handed back to it (review m2). + private sealed class CertValidationHook + { + public CertificateValidationEventHandler? Handler { get; set; } + public CertificateValidator? Validator { get; set; } + } + + private static void DetachCertValidation(CertificateValidator? validator, CertificateValidationEventHandler? handler) + { + if (validator is null || handler is null) return; + try { validator.CertificateValidation -= handler; } catch { /* best-effort */ } + } + /// /// Test seam: opens the session for one endpoint URL in place of the real discovery + session create, so /// the post-connect half of (and its superseded / failure paths) can be driven @@ -672,7 +699,11 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit /// — so multiple driver instances in the same OtOpcUa server process share one /// certificate store without extra config. /// - private async Task BuildApplicationConfigurationAsync(CancellationToken ct) + /// Receives the auto-accept handler this build attached (if any) — published by the + /// caller only when its initialise is still current (review m2), detached otherwise. + /// Cancellation token. + private async Task BuildApplicationConfigurationAsync( + CertValidationHook certValidation, CancellationToken ct) { // The ITelemetryContext overload is the non-obsolete ctor; SDK warnings reach the driver's // logger when one was supplied. @@ -762,10 +793,10 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit e.Accept = true; }; config.CertificateValidator.CertificateValidation += handler; - // Store refs so ShutdownAsync + Dispose can detach the delegate and avoid - // leaking a closure on a potentially process-shared validator. - _certValidationHandler = handler; - _certValidatorRef = config.CertificateValidator; + // Handed back to the caller, which stores the refs (so ShutdownAsync + Dispose can detach the delegate) + // only while its initialise is the current lifecycle generation. + certValidation.Handler = handler; + certValidation.Validator = config.CertificateValidator; } // Ensure an application certificate exists. The SDK auto-generates one if missing. @@ -1856,11 +1887,13 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit TimestampsToReturn = TimestampsToReturn.Both, }); + long subscribeGeneration = 0; await _subscriptionGate.WaitAsync(cancellationToken).ConfigureAwait(false); try { // Re-read Session inside the critical section: a reconnect completing while we were // blocked on _gate has already swapped in the new session via a re-initialise. + subscribeGeneration = Volatile.Read(ref _lifecycleGeneration); var session = Session ?? throw new InvalidOperationException( "OpcUaClient session was lost before the subscription could be created."); @@ -1925,9 +1958,23 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit } await subscription.CreateItemsAsync(cancellationToken).ConfigureAwait(false); + ThrowIfSubscribeSuperseded(subscribeGeneration); } } + /// + /// Review m10: a subscribe that a shutdown / re-initialise overtook must not register its subscription — the + /// teardown has already enumerated the registry, so the entry would outlive it and the next session's stall + /// check would declare a dead subscription lost. Thrown inside the create's try, so the half-built + /// subscription is removed like any other failed create. + /// + private void ThrowIfSubscribeSuperseded(long subscribeGeneration) + { + if (Volatile.Read(ref _lifecycleGeneration) != subscribeGeneration) + throw new InvalidOperationException( + "OpcUaClient session was released (shutdown or re-initialise) while the subscription was being created."); + } + /// public async Task UnsubscribeAsync(ISubscriptionHandle handle, CancellationToken cancellationToken) { @@ -2151,6 +2198,17 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit private void RecordDeviceAnswered(long generation, DateTime now) { if (Volatile.Read(ref _subscriptionLost) == 1) return; + // Review m7: during a published outage (bad keep-alive) a reply that was in flight is evidence — the stamp + // advances — but it does not declare the outage over; the keep-alive recovery path does that (it also + // re-publishes the refs it published Bad), so Healthy is only written once the outage latch is clear. + if (Volatile.Read(ref _sessionLost) == 1) + { + UpdateHealth(generation, h => h.State is DriverState.Healthy or DriverState.Degraded + ? h with { LastSuccessfulRead = now } + : null); + return; + } + UpdateHealth(generation, _ => new DriverHealth(DriverState.Healthy, now, null)); } @@ -2336,11 +2394,13 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit MonitoredItem? eventItem = null; MonitoredItemNotificationEventHandler? notifHandler = null; + long subscribeGeneration = 0; await _subscriptionGate.WaitAsync(cancellationToken).ConfigureAwait(false); try { // Re-read Session inside the critical section: a reconnect completing while we were // blocked on _gate has already swapped in the new session via a re-initialise. + subscribeGeneration = Volatile.Read(ref _lifecycleGeneration); var session = Session ?? throw new InvalidOperationException( "OpcUaClient session was lost before the alarm subscription could be created."); @@ -2404,6 +2464,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit eventItem.Notification += notifHandler; subscription.AddItem(eventItem); await subscription.CreateItemsAsync(cancellationToken).ConfigureAwait(false); + ThrowIfSubscribeSuperseded(subscribeGeneration); _alarmSubscriptions[id] = new RemoteAlarmSubscription(subscription, handle, eventItem, notifHandler); } @@ -2440,7 +2501,10 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit // Fast-fail before queuing if the driver isn't connected, but do NOT bind the ack calls // (or the namespace-relative ConditionId parse) to this reference — a reconnect can swap // Session while we wait on _gate. The session actually used is re-read inside the gate. - _ = RequireSession(); + // Review m5: the same BadNotConnected as the in-gate check, so "no session" is one status to the caller. + if (Session is null) + throw new ServiceResultException(StatusCodes.BadNotConnected, + "OpcUaClientDriver is not connected; the acknowledgement was not sent."); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); try @@ -2490,7 +2554,15 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit }); } - if (callRequests.Count == 0) return; + if (callRequests.Count == 0) + { + // Review m3: nothing resolved to a condition node, so nothing was sent — that is not an applied ack. + _logger.LogWarning( + "OpcUaClientDriver '{DriverInstanceId}': AcknowledgeAsync resolved none of {Count} condition id(s) " + + "against the live session; nothing was sent upstream.", _driverInstanceId, acknowledgements.Count); + throw new ServiceResultException(StatusCodes.BadNodeIdUnknown, + "None of the acknowledged condition ids resolved to an upstream condition node."); + } try { @@ -2528,6 +2600,8 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit _logger.LogWarning(ex, "OpcUaClientDriver '{DriverInstanceId}': AcknowledgeAsync transport error.", _driverInstanceId); + // Review m4: a timeout (not the caller cancelling) is a call with no reply, like a read or write. + if (!cancellationToken.IsCancellationRequested) RecordCommsFault(generation, $"ack: {ex.Message}"); throw; } catch (Exception ex) @@ -2538,6 +2612,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit "OpcUaClientDriver '{DriverInstanceId}': AcknowledgeAsync failed; " + "acknowledgements were not applied.", _driverInstanceId); + RecordCommsFault(generation, $"ack: {ex.Message}"); throw; } } @@ -3524,7 +3599,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit { try { - if (gate.Wait(TimeSpan.FromSeconds(2))) + if (gate.Wait(TimeSpan.FromMilliseconds(500))) gate.Release(); } catch { /* timeout or already disposed — proceed */ } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor index a9097d97..95506d0e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Drivers/Forms/OpcUaClientDriverForm.razor @@ -219,7 +219,7 @@
-
Max 60. Used by Test Connect. Default 15.
+
Caps Test Connect: it can only shorten the 10 s Test Connect budget, never extend it.
diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionAttributesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionAttributesTests.cs index 820be4c4..928a1e44 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionAttributesTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Browser.Tests/OpcUaClientBrowseSessionAttributesTests.cs @@ -100,4 +100,27 @@ public sealed class OpcUaClientBrowseSessionAttributesTests await Should.ThrowAsync(() => session.AttributesAsync("nsu=urn:not-in-table;s=X", TestContext.Current.CancellationToken)); } + + private sealed class TrackingCredential : IDisposable + { + public int Disposed { get; private set; } + + public void Dispose() => Disposed++; + } + + /// Review m9: the browse session disposes the user certificate it was handed, with itself. + [Fact] + public async Task The_session_disposes_the_credential_it_owns() + { + var mock = SessionReturning(NodeClass.Variable, DataTypeIds.Int32, -1, 1); + var credential = new TrackingCredential(); + var session = new OpcUaClientBrowseSession( + mock.Object, NamespaceMap.FromTable(new NamespaceTable()), ObjectIds.ObjectsFolder, credential); + + credential.Disposed.ShouldBe(0, "positive control: still owned while the session is open"); + await session.DisposeAsync(); + await session.DisposeAsync(); + + credential.Disposed.ShouldBe(1); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAcknowledgeFailureTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAcknowledgeFailureTests.cs index 63e9b62a..149de872 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAcknowledgeFailureTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAcknowledgeFailureTests.cs @@ -80,4 +80,39 @@ public sealed class OpcUaClientAcknowledgeFailureTests await Should.ThrowAsync(() => call); } + + /// Review m3: an ack none of whose condition ids resolves sent nothing upstream — that fails, it is not "applied". + [Fact] + public async Task An_ack_with_no_resolvable_condition_fails_with_BadNodeIdUnknown() + { + using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-m3"); + var session = NewSession(); + drv.SetSessionForTest(session.Object); + + var ex = await Should.ThrowAsync(() => drv.AcknowledgeAsync( + [new AlarmAcknowledgeRequest("nsu=urn:not-in-table;s=X", "nsu=urn:not-in-table;s=X", "ack")], + TestContext.Current.CancellationToken)); + + ex.StatusCode.ShouldBe(StatusCodes.BadNodeIdUnknown); + session.Verify(s => s.CallAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + /// Review m4: an ack with no reply is a comms fault like a read or write — Degraded, stamp preserved. + [Fact] + public async Task An_ack_transport_failure_degrades_and_preserves_the_stamp() + { + using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-m4"); + var stamp = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc); + drv.SetHealthForTest(new DriverHealth(DriverState.Healthy, stamp, null)); + var session = NewSession(); + session.Setup(s => s.CallAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ThrowsAsync(new ServiceResultException(StatusCodes.BadConnectionClosed)); + drv.SetSessionForTest(session.Object); + + await Should.ThrowAsync(() => drv.AcknowledgeAsync(Ack(), TestContext.Current.CancellationToken)); + + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + drv.GetHealth().LastSuccessfulRead.ShouldBe(stamp); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs index 07c65f1b..b9a3d06b 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientAlarmTests.cs @@ -51,15 +51,17 @@ public sealed class OpcUaClientAlarmTests await drv.UnsubscribeAlarmsAsync(new FakeAlarmHandle(), TestContext.Current.CancellationToken); } - /// Verifies that AcknowledgeAsync without initialize throws InvalidOperationException. + /// Verifies that AcknowledgeAsync without initialize throws ServiceResultException(BadNotConnected). [Fact] - public async Task AcknowledgeAsync_without_initialize_throws_InvalidOperationException() + public async Task AcknowledgeAsync_without_initialize_throws_BadNotConnected() { + // Review m5: the same status as a session lost while the ack queued (one "no session" answer). using var drv = new OpcUaClientDriver(new OpcUaClientDriverOptions(), "opcua-ack-uninit"); - await Should.ThrowAsync(async () => + var ex = await Should.ThrowAsync(async () => await drv.AcknowledgeAsync( [new AlarmAcknowledgeRequest("ns=2;s=Src", "ns=2;s=Cond", "operator ack")], TestContext.Current.CancellationToken)); + ex.StatusCode.ShouldBe(Opc.Ua.StatusCodes.BadNotConnected); } /// Verifies that AcknowledgeAsync with empty batch is noop even without init. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientWriteHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientWriteHealthTests.cs index 10e4637e..320d9c30 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientWriteHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientWriteHealthTests.cs @@ -117,4 +117,47 @@ public sealed class OpcUaClientWriteHealthTests h.LastSuccessfulRead.ShouldNotBeNull().ShouldBeGreaterThan(Stale); h.LastError.ShouldBeNull(); } + + /// + /// Review m8: a write that TIMED OUT (an OperationCanceledException the caller did not cause) is a call with no + /// reply — Degraded, stamp preserved. The genuine positive control for the caller-cancelled test above (same + /// catch branch, the caller's token is the only difference). + /// + [Fact] + public async Task A_write_that_timed_out_without_caller_cancellation_degrades() + { + var (drv, session) = NewConnectedDriver(new DriverHealth(DriverState.Healthy, Stale, null)); + using var _ = drv; + session.Setup(s => s.WriteAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ThrowsAsync(new OperationCanceledException("SDK request timeout")); + + var results = await drv.WriteAsync(Write(), TestContext.Current.CancellationToken); + + results.Single().StatusCode.ShouldBe(0x800A0000u); // BadTimeout + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + drv.GetHealth().LastSuccessfulRead.ShouldBe(Stale); + } + + /// + /// Review m7: a reply that was in flight across a bad keep-alive advances the stamp (it is evidence) but does not + /// declare the published outage over — health stays Degraded until the keep-alive recovers. + /// + [Fact] + public async Task A_reply_during_a_published_outage_advances_the_stamp_but_keeps_degraded() + { + var (drv, session) = NewConnectedDriver(new DriverHealth(DriverState.Healthy, Stale, null)); + using var _ = drv; + drv.RaiseKeepAliveForTest(StatusCodes.BadNoCommunication); + drv.GetHealth().State.ShouldBe(DriverState.Degraded, "positive control: the outage is published"); + session.Setup(s => s.WriteAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(new WriteResponse { ResponseHeader = new ResponseHeader(), Results = [StatusCodes.Good], DiagnosticInfos = [] }); + + await drv.WriteAsync(Write(), TestContext.Current.CancellationToken); + + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + drv.GetHealth().LastSuccessfulRead.ShouldNotBeNull().ShouldBeGreaterThan(Stale); + + drv.RaiseKeepAliveForTest(StatusCodes.Good); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "the keep-alive recovery is what ends the outage"); + } }