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

@@ -135,6 +135,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 +184,10 @@ await Client.FindAliasAsync("%", NodeId.Null, ct)
                 m_forward = forward;
                 m_serverUris = serverUris;
                 m_reverse = reverse;
-                Volatile.Write(ref m_loaded, 1);
+                if (Interlocked.Read(ref m_invalidationGeneration) == generation)
+                {
+                    Volatile.Write(ref m_loaded, 1);
+                }
             }
             finally
             {
@@ -290,6 +298,10 @@ public async Task<IReadOnlyList<ExpandedNodeId>> ResolveAsync(
         /// </summary>
         public void Invalidate()
         {
+            // Bump first: a refresh that is already fetching compares the
+            // generation before it marks the cache loaded, so its stale result
+            // can no longer swallow this invalidation.
+            Interlocked.Increment(ref m_invalidationGeneration);
             Volatile.Write(ref m_loaded, 0);
         }
 
@@ -413,6 +425,13 @@ private Dictionary<string, ExpandedNodeId[]> m_forward
 
         private Dictionary<ExpandedNodeId, string> m_reverse = [];
         private int m_loaded;
+
+        /// <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

@@ -120,9 +120,51 @@ public async ValueTask StartAsync(
                 DiscardOldest = true,
                 MonitoringMode = MonitoringMode.Reporting
             };
-            item.Notification += OnNotification;
-            subscription.AddItem(item);
-            await subscription.ApplyChangesAsync(ct).ConfigureAwait(false);
+            try
+            {
+                item.Notification += OnNotification;
+                subscription.AddItem(item);
+                await subscription.ApplyChangesAsync(ct).ConfigureAwait(false);
+            }
+            catch
+            {
+                // 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.
+                item.Notification -= OnNotification;
+                try
+                {
+                    subscription.RemoveItem(item);
+                }
+                catch (Exception cleanupException)
+                    when (cleanupException is not OutOfMemoryException)
+                {
+                    // Best-effort cleanup.
+                }
+                if (ownsSubscription)
+                {
+                    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;

src/Opc.Ua.Client/Browser.cs

@@ -352,6 +352,9 @@ public async ValueTask<ArrayOf<ReferenceDescription>> BrowseAsync(
             }
             catch (OperationCanceledException) when (!continuationPoint.IsEmpty)
             {
+                // Release the continuation point before propagating: returning
+                // the partial list here would hand the caller a silently
+                // truncated result that looks like a complete browse.
                 session = Session;
                 if (session != null)
                 {
@@ -361,6 +364,7 @@ public async ValueTask<ArrayOf<ReferenceDescription>> BrowseAsync(
                     true,
                     default).ConfigureAwait(false);
                 }
+                throw;
             }
             // return the results.
             return references;
@@ -424,10 +428,17 @@ public async ValueTask<ResultSet<ArrayOf<ReferenceDescription>>> BrowseAsync(
             var resultForPass = new List<List<ReferenceDescription>>(count);
             resultForPass.AddRange(result);
 
-            var errorsForPass = new List<ServiceResult>(count);
-            errorsForPass.AddRange(errors);
+            // Index into the caller's result/error lists for every entry of the
+            // current pass. A retry pass browses a shrunken subset, so its own
+            // offsets no longer line up with the caller's lists.
+            var originalIndexForPass = new List<int>(count);
+            for (int i = 0; i < count; i++)
+            {
+                originalIndexForPass.Add(i);
+            }
 
             int passCount = 0;
+            int passesWithoutProgress = 0;
 
             do
             {
@@ -451,7 +462,7 @@ public async ValueTask<ResultSet<ArrayOf<ReferenceDescription>>> BrowseAsync(
                 var nodesToBrowseForNextPass = new List<NodeId>();
                 var referenceDescriptionsForNextPass
                     = new List<List<ReferenceDescription>>();
-                var errorsForNextPass = new List<ServiceResult>();
+                var originalIndexForNextPass = new List<int>();
 
                 // loop over the batches
                 foreach (ArrayOf<NodeId> nodesToBrowseBatch in
@@ -500,23 +511,29 @@ var referenceDescriptionsForNextPass
                                     nodesToBrowseForPass[resultOffset]);
                                 referenceDescriptionsForNextPass.Add(
                                     resultForPass[resultOffset]);
-                                errorsForNextPass.Add(errorsForPass[resultOffset]);
+                                originalIndexForNextPass.Add(
+                                    originalIndexForPass[resultOffset]);
                             }
                         }
 
                         resultForPass[resultOffset].Clear();
                         resultForPass[resultOffset].AddRange(results.Results[ii]);
-                        errorsForPass[resultOffset] = results.Errors[ii];
-                        errors[resultOffset] = results.Errors[ii];
+                        errors[originalIndexForPass[resultOffset]] = results.Errors[ii];
                         resultOffset++;
                     }
 
                     batchOffset += nodesToBrowseBatchCount;
                 }
 
+                // A pass only ever drops nodes, so an unchanged count means not
+                // a single node completed and the next pass would repeat this
+                // one exactly.
+                bool progressed = nodesToBrowseForNextPass.Count < nodesToBrowseForPass.Count;
+                passesWithoutProgress = progressed ? 0 : passesWithoutProgress + 1;
+
                 resultForPass = referenceDescriptionsForNextPass;
-                errorsForPass = errorsForNextPass;
                 nodesToBrowseForPass = nodesToBrowseForNextPass;
+                originalIndexForPass = originalIndexForNextPass;
 
                 if (badCPInvalidErrorsPerPass > 0)
                 {
@@ -549,6 +566,25 @@ var referenceDescriptionsForNextPass
                 }
 
                 passCount++;
+
+                if (passesWithoutProgress >= kMaxManagedBrowsePassesWithoutProgress &&
+                    nodesToBrowseForPass.Count > 0)
+                {
+                    // Guard against a non-conforming server. Per Part 4 §7.9 a
+                    // server shall never answer BadNoContinuationPoints when
+                    // continuing a halted operation, so a conforming peer frees
+                    // its quota and every pass completes at least the nodes
+                    // that quota covers; one that keeps returning it for the
+                    // same nodes would otherwise spin this loop forever. A
+                    // large browse that completes only a few nodes per pass
+                    // is progressing and is never cut short here.
+                    // Report the last error for the nodes that never completed.
+                    m_logger.ManagedBrowsePassPassCountErrorS(
+                        passCount,
+                        nodesToBrowseForPass.Count,
+                        "continuation point errors that did not resolve; giving up");
+                    break;
+                }
             } while (nodesToBrowseForPass.Count > 0);
             return ResultSet.From(result.ConvertAll(l => (ArrayOf<ReferenceDescription>)l), errors);
         }
@@ -866,6 +902,15 @@ private class ReferenceWrapper<T>
             public required T Reference { get; set; }
         }
 
+        /// <summary>
+        /// Consecutive managed browse passes that may complete no node at all
+        /// before the browse gives up. Guards against a server that answers
+        /// every retry with another continuation point error; a single such
+        /// pass can also be a concurrent browse transiently exhausting the
+        /// quota, hence more than one.
+        /// </summary>
+        private const int kMaxManagedBrowsePassesWithoutProgress = 3;
+
         private readonly ILogger m_logger;
         private readonly ITelemetryContext? m_telemetry;