Fix the Opc.Ua.Client defects reported by the client stack audit

src/Opc.Ua.Client/AliasNames/AliasNameClient.cs

@@ -445,35 +445,10 @@ public async IAsyncEnumerable<AliasNameSubCategoryInfo>
                 throw new ServiceResultException(br.StatusCode);
             }
 
-            foreach (ReferenceDescription r in SnapshotReferences(br.References))
-            {
-                if (!r.TypeDefinition.Equals(ObjectTypeIds.AliasNameCategoryType))
-                {
-                    continue;
-                }
-                var localId = ExpandedNodeId.ToNodeId(
-                    r.NodeId, Session.NamespaceUris);
-                if (localId.IsNull)
-                {
-                    continue;
-                }
-                yield return new AliasNameSubCategoryInfo(
-                    localId,
-                    r.BrowseName,
-                    r.DisplayName);
-            }
-
             ByteString continuationPoint = br.ContinuationPoint;
-            while (!continuationPoint.IsEmpty)
+            try
             {
-                (_, continuationPoint, ArrayOf<ReferenceDescription> nextReferences) =
-                    await Session.BrowseNextAsync(
-                        requestHeader: null,
-                        releaseContinuationPoint: false,
-                        continuationPoint,
-                        ct).ConfigureAwait(false);
-
-                foreach (ReferenceDescription r in SnapshotReferences(nextReferences))
+                foreach (ReferenceDescription r in SnapshotReferences(br.References))
                 {
                     if (!r.TypeDefinition.Equals(ObjectTypeIds.AliasNameCategoryType))
                     {
@@ -490,6 +465,41 @@ await Session.BrowseNextAsync(
                         r.BrowseName,
                         r.DisplayName);
                 }
+
+                while (!continuationPoint.IsEmpty)
+                {
+                    (_, continuationPoint, ArrayOf<ReferenceDescription> nextReferences) =
+                        await Session.BrowseNextAsync(
+                            requestHeader: null,
+                            releaseContinuationPoint: false,
+                            continuationPoint,
+                            ct).ConfigureAwait(false);
+
+                    foreach (ReferenceDescription r in SnapshotReferences(nextReferences))
+                    {
+                        if (!r.TypeDefinition.Equals(ObjectTypeIds.AliasNameCategoryType))
+                        {
+                            continue;
+                        }
+                        var localId = ExpandedNodeId.ToNodeId(
+                            r.NodeId, Session.NamespaceUris);
+                        if (localId.IsNull)
+                        {
+                            continue;
+                        }
+                        yield return new AliasNameSubCategoryInfo(
+                            localId,
+                            r.BrowseName,
+                            r.DisplayName);
+                    }
+                }
+            }
+            finally
+            {
+                // Part 4 §5.9.3.2 requires releasing a continuation point the
+                // client stops following.
+                await Session.ReleaseContinuationPointAsync(continuationPoint)
+                    .ConfigureAwait(false);
             }
         }
 

src/Opc.Ua.Client/AliasNames/AliasNameResolver.cs

@@ -99,7 +99,8 @@ public AliasNameResolver(
         /// </summary>
         public async Task EnsureLoadedAsync(CancellationToken ct = default)
         {
-            if (Volatile.Read(ref m_loaded) == 1)
+            if (Volatile.Read(ref m_loadedGeneration) ==
+                Interlocked.Read(ref m_invalidationGeneration))
             {
                 return;
             }
@@ -135,6 +136,11 @@ await m_strategy.StartAsync(Client, Invalidate, ct)
         /// </summary>
         public async Task RefreshAsync(CancellationToken ct = default)
         {
+            // Capture the invalidation generation before the fetch: an
+            // Invalidate that lands while it is in flight must not be undone by
+            // marking the (already stale) result as loaded.
+            long generation = Interlocked.Read(ref m_invalidationGeneration);
+
             var forward = new Dictionary<string, ExpandedNodeId[]>(StringComparer.Ordinal);
             var serverUris = new Dictionary<string, string?[]>(StringComparer.Ordinal);
             var reverse = new Dictionary<ExpandedNodeId, string>();
@@ -179,7 +185,14 @@ await Client.FindAliasAsync("%", NodeId.Null, ct)
                 m_forward = forward;
                 m_serverUris = serverUris;
                 m_reverse = reverse;
-                Volatile.Write(ref m_loaded, 1);
+
+                // Publish the generation this data was fetched for rather than
+                // a separate "loaded" flag: an Invalidate that lands between a
+                // check and the flag write would otherwise be overwritten and
+                // mark pre-invalidation data as current forever. A reader
+                // compares the two generations, so the single write below can
+                // never swallow an invalidation.
+                Volatile.Write(ref m_loadedGeneration, generation);
             }
             finally
             {
@@ -290,7 +303,11 @@ public async Task<IReadOnlyList<ExpandedNodeId>> ResolveAsync(
         /// </summary>
         public void Invalidate()
         {
-            Volatile.Write(ref m_loaded, 0);
+            // One atomic step: a refresh that is already fetching publishes the
+            // generation it fetched for, which no longer matches this one, so
+            // its stale result can neither be served nor swallow this
+            // invalidation.
+            Interlocked.Increment(ref m_invalidationGeneration);
         }
 
         /// <summary>
@@ -412,7 +429,21 @@ private Dictionary<string, ExpandedNodeId[]> m_forward
             = new(StringComparer.Ordinal);
 
         private Dictionary<ExpandedNodeId, string> m_reverse = [];
-        private int m_loaded;
+
+        /// <summary>
+        /// The <see cref="m_invalidationGeneration"/> the cached data was
+        /// fetched for. The cache is loaded exactly while this equals the
+        /// current generation; <c>-1</c> is "never loaded", which no
+        /// generation can collide with.
+        /// </summary>
+        private long m_loadedGeneration = -1;
+
+        /// <summary>
+        /// Incremented by every <see cref="Invalidate"/> so an in-flight
+        /// <see cref="RefreshAsync"/> can tell whether its result is still
+        /// current when it completes.
+        /// </summary>
+        private long m_invalidationGeneration;
         private int m_strategyStarted;
     }
 }

src/Opc.Ua.Client/AliasNames/Refresh/MonitoredItemAliasNameRefreshStrategy.cs

@@ -91,38 +91,89 @@ public async ValueTask StartAsync(
                 return;
             }
 
-            Subscription subscription;
+            Subscription? subscription = null;
             bool ownsSubscription = false;
-            if (Options.SharedSubscription != null)
+            MonitoredItem? item = null;
+            try
             {
-                subscription = Options.SharedSubscription;
-            }
-            else
-            {
-                subscription = new Subscription(client.Session.MessageContext.Telemetry)
+                if (Options.SharedSubscription != null)
+                {
+                    subscription = Options.SharedSubscription;
+                }
+                else
+                {
+                    subscription = new Subscription(client.Session.MessageContext.Telemetry)
+                    {
+                        DisplayName = Options.SubscriptionDisplayName,
+                        PublishingEnabled = true,
+                        PublishingInterval = (int)Options.PublishingIntervalMs
+                    };
+                    client.Session.AddSubscription(subscription);
+
+                    // Claim ownership before the first await that can fail:
+                    // CreateAsync throwing must still tear the registration
+                    // down, otherwise the subscription this cleanup exists to
+                    // prevent is exactly what a failed start leaves behind.
+                    ownsSubscription = true;
+                    await subscription.CreateAsync(ct).ConfigureAwait(false);
+                }
+
+                item = new MonitoredItem(subscription.DefaultItem)
                 {
-                    DisplayName = Options.SubscriptionDisplayName,
-                    PublishingEnabled = true,
-                    PublishingInterval = (int)Options.PublishingIntervalMs
+                    StartNodeId = lastChangeId,
+                    AttributeId = Attributes.Value,
+                    DisplayName = "LastChange",
+                    SamplingInterval = (int)Options.SamplingIntervalMs,
+                    QueueSize = 1,
+                    DiscardOldest = true,
+                    MonitoringMode = MonitoringMode.Reporting
                 };
-                client.Session.AddSubscription(subscription);
-                await subscription.CreateAsync(ct).ConfigureAwait(false);
-                ownsSubscription = true;
+                item.Notification += OnNotification;
+                subscription.AddItem(item);
+                await subscription.ApplyChangesAsync(ct).ConfigureAwait(false);
             }
-
-            var item = new MonitoredItem(subscription.DefaultItem)
+            catch
             {
-                StartNodeId = lastChangeId,
-                AttributeId = Attributes.Value,
-                DisplayName = "LastChange",
-                SamplingInterval = (int)Options.SamplingIntervalMs,
-                QueueSize = 1,
-                DiscardOldest = true,
-                MonitoringMode = MonitoringMode.Reporting
-            };
-            item.Notification += OnNotification;
-            subscription.AddItem(item);
-            await subscription.ApplyChangesAsync(ct).ConfigureAwait(false);
+                // Do not leave a half-built subscription behind: the caller
+                // retries StartAsync on every resolve, and each failed attempt
+                // would otherwise add another subscription to the session.
+                if (item != null && subscription != null)
+                {
+                    item.Notification -= OnNotification;
+                    try
+                    {
+                        subscription.RemoveItem(item);
+                    }
+                    catch (Exception cleanupException)
+                        when (cleanupException is not OutOfMemoryException)
+                    {
+                        // Best-effort cleanup.
+                    }
+                }
+                if (ownsSubscription && subscription != null)
+                {
+                    try
+                    {
+                        // Cleanup must run to completion even when the caller's
+                        // token is what aborted the start.
+                        await client.Session
+                            .RemoveSubscriptionAsync(subscription, CancellationToken.None)
+                            .ConfigureAwait(false);
+                        await subscription.DeleteAsync(silent: true, CancellationToken.None)
+                            .ConfigureAwait(false);
+                    }
+                    catch (Exception cleanupException)
+                        when (cleanupException is not OutOfMemoryException)
+                    {
+                        // Best-effort cleanup; the session may already be gone.
+                    }
+                    finally
+                    {
+                        subscription.Dispose();
+                    }
+                }
+                throw;
+            }
 
             m_subscription = subscription;
             m_item = item;