Synchronize ConditionState branch collection access (#3895)

# Description

`ConditionState.m_branches` (a plain `Dictionary`) was read and written
from multiple threads without synchronization. A state change on a
background thread could mutate the dictionary while readers
(`GetRetainState`/`UpdateRetainState`, `ConditionRefresh`, `GetBranch`)
enumerated it, yielding torn entries and native access violations
(observed as `IsBranch` on a null `this`). This ports the master378 fix
to `master`, where the type lives at
`Stack/Opc.Ua.Core.Types/State/ConditionState.cs` and uses
`ByteString`/nullable refs.

- **Lock all branch access**: added a private `m_branchesLock` guarding
every read/write of `m_branches` (`CreateBranch`, `GetBranch`,
`ReplaceBranchEvent`, `RemoveBranchEvent`, `ClearBranches`,
`GetBranchCount`).
- **Snapshot enumeration**: `GetBranches()` now returns a copy taken
under the lock, so internal readers and
`AlarmHolder.GetBranchesForConditionRefresh` iterate a stable collection
while other threads mutate the live one. Public signature is unchanged.
- **Test**: added `ConcurrentBranchAccessIsThreadSafe`, stressing
concurrent create/clear writers against enumerate/count readers.

`lock` is used rather than `SemaphoreSlim` because all affected methods
are synchronous. Each `ConditionState` holds its own lock, so the
recursive `UpdateRetainState` traversal across branches neither contends
nor deadlocks. `ConcurrentDictionary` was rejected: it would change the
public return type and still leave compound read-modify-write sequences
(e.g. `ReplaceBranchEvent`) unprotected.

## Related Issues

## Checklist

_Put an `x` in the boxes that apply. You can complete these step by step
after opening the PR._

- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [ ] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Suciu Mircea Adrian <Mircea-Adrian.Suciu@softing.com>

Author: 4 people

Libraries/Opc.Ua.Client/CoreClientUtils.NodeSetExport.cs

@@ -109,12 +109,14 @@ public static void ExportNodesToNodeSet2(
         /// <param name="nodes">The list of nodes to export.</param>
         /// <param name="outputStream">The output stream to write the NodeSet2 XML to.</param>
         /// <param name="options">Options controlling the export behavior.</param>
+        /// <param name="lastModified">The last-modified timestamp to embed, or <c>null</c> to use the current time.</param>
         /// <exception cref="ArgumentNullException"><paramref name="nodes"/> is <c>null</c>.</exception>
         public static void ExportNodesToNodeSet2(
             ISystemContext context,
             IList<INode> nodes,
             Stream outputStream,
-            NodeSetExportOptions options)
+            NodeSetExportOptions options,
+            DateTime? lastModified = null)
         {
             if (context == null)
             {
@@ -148,7 +150,7 @@ public static void ExportNodesToNodeSet2(
             }
 
             // Use the existing export functionality
-            nodeStates.SaveAsNodeSet2(context, outputStream);
+            nodeStates.SaveAsNodeSet2(context, outputStream, null, lastModified);
         }
 
         /// <summary>

Stack/Opc.Ua.Core/Stack/State/ConditionState.cs

@@ -30,6 +30,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
 
 namespace Opc.Ua
 {
@@ -197,9 +198,10 @@ public virtual ConditionState CreateBranch(ISystemContext context, NodeId branch
 
                 string postEventId = Utils.ToHexString(branchedNodeState.EventId.Value);
 
-                Dictionary<string, ConditionState> branches = GetBranches();
-
-                branches.Add(postEventId, branchedNodeState);
+                lock (m_branchesLock)
+                {
+                    (m_branches ??= []).Add(postEventId, branchedNodeState);
+                }
 
                 state = branchedNodeState;
             }
@@ -208,15 +210,22 @@ public virtual ConditionState CreateBranch(ISystemContext context, NodeId branch
         }
 
         /// <summary>
-        /// Retrieves the branches for the current ConditionState.
-        /// Creates the branches dictionary if required
+        /// Returns a snapshot of the current branches for this ConditionState.
         /// </summary>
         /// <remarks>
         /// Function exists because constructor is in auto generated code.
+        /// Returns a snapshot copy so the caller can safely enumerate it
+        /// while other threads concurrently modify the branch collection.
+        /// If no branches exist, an empty dictionary is returned.
         /// </remarks>
         public Dictionary<string, ConditionState> GetBranches()
         {
-            return m_branches ??= [];
+            lock (m_branchesLock)
+            {
+                return m_branches != null
+                    ? new Dictionary<string, ConditionState>(m_branches)
+                    : [];
+            }
         }
 
         /// <summary>
@@ -241,20 +250,21 @@ public virtual ConditionState GetEventByEventId(byte[] eventId)
         /// <returns>ConditionState branch if it exists</returns>
         public ConditionState GetBranch(byte[] eventId)
         {
-            ConditionState alarm = null;
-
-            Dictionary<string, ConditionState> branches = GetBranches();
-
-            foreach (ConditionState branchEvent in branches.Values)
+            lock (m_branchesLock)
             {
-                if (branchEvent.EventId.Value.SequenceEqual(eventId))
+                if (m_branches != null)
                 {
-                    alarm = branchEvent;
-                    break;
+                    foreach (ConditionState branchEvent in m_branches.Values)
+                    {
+                        if (branchEvent.EventId.Value.SequenceEqual(eventId))
+                        {
+                            return branchEvent;
+                        }
+                    }
                 }
             }
 
-            return alarm;
+            return null;
         }
 
         /// <summary>
@@ -267,10 +277,12 @@ protected void ReplaceBranchEvent(byte[] originalEventId, ConditionState alarm)
             string originalKey = Utils.ToHexString(originalEventId);
             string newKey = Utils.ToHexString(alarm.EventId.Value);
 
-            Dictionary<string, ConditionState> branches = GetBranches();
-
-            branches.Remove(originalKey);
-            branches.Add(newKey, alarm);
+            lock (m_branchesLock)
+            {
+                m_branches ??= [];
+                m_branches.Remove(originalKey);
+                m_branches.Add(newKey, alarm);
+            }
         }
 
         /// <summary>
@@ -281,18 +293,21 @@ protected void RemoveBranchEvent(byte[] eventId)
         {
             string key = Utils.ToHexString(eventId);
 
-            Dictionary<string, ConditionState> branches = GetBranches();
-
-            branches.Remove(key);
+            lock (m_branchesLock)
+            {
+                m_branches?.Remove(key);
+            }
         }
 
         /// <summary>
         /// Clear all branches for this event
         /// </summary>
         public void ClearBranches()
         {
-            Dictionary<string, ConditionState> branches = GetBranches();
-            branches.Clear();
+            lock (m_branchesLock)
+            {
+                m_branches?.Clear();
+            }
         }
 
         /// <summary>
@@ -344,9 +359,10 @@ protected virtual bool GetRetainState()
         /// </returns>
         public virtual int GetBranchCount()
         {
-            Dictionary<string, ConditionState> branches = GetBranches();
-
-            return branches.Count;
+            lock (m_branchesLock)
+            {
+                return m_branches?.Count ?? 0;
+            }
         }
 
         /// <summary>
@@ -797,6 +813,11 @@ protected bool IsBranch()
         /// Branches
         /// </summary>
         protected Dictionary<string, ConditionState> m_branches;
+
+        /// <summary>
+        /// Lock protecting all access to <see cref="m_branches"/>.
+        /// </summary>
+        protected readonly Lock m_branchesLock = new();
         private PropertyState<bool> m_supportsFilteredRetain;
     }
 

Stack/Opc.Ua.Types/State/NodeStateCollection.cs

@@ -178,17 +178,29 @@ public AliasToUse(string alias, NodeId nodeId)
         /// </summary>
         public void SaveAsNodeSet2(ISystemContext context, Stream ostrm)
         {
-            SaveAsNodeSet2(context, ostrm, null);
+            SaveAsNodeSet2(context, ostrm, null, null);
         }
 
         /// <summary>
         /// Writes the collection to a stream using the Opc.Ua.Schema.UANodeSet schema.
         /// </summary>
         public void SaveAsNodeSet2(ISystemContext context, Stream ostrm, string version)
+        {
+            SaveAsNodeSet2(context, ostrm, version, null);
+        }
+
+        /// <summary>
+        /// Writes the collection to a stream using the Opc.Ua.Schema.UANodeSet schema.
+        /// </summary>
+        /// <param name="context">The system context.</param>
+        /// <param name="ostrm">The output stream.</param>
+        /// <param name="version">The version string to embed in the NodeSet2 XML, or <c>null</c>.</param>
+        /// <param name="lastModified">The last-modified timestamp to embed, or <c>null</c> to use <see cref="DateTime.UtcNow"/>.</param>
+        public void SaveAsNodeSet2(ISystemContext context, Stream ostrm, string version, DateTime? lastModified)
         {
             var nodeSet = new Export.UANodeSet
             {
-                LastModified = DateTime.UtcNow,
+                LastModified = lastModified ?? DateTime.UtcNow,
                 LastModifiedSpecified = true
             };
 

Tests/Opc.Ua.Client.Tests/NodeSetExportTest.cs

@@ -27,6 +27,7 @@
  * http://opcfoundation.org/License/MIT/1.00/
  * ======================================================================*/
 
+using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
@@ -335,6 +336,10 @@ public async Task ExportNodesToNodeSet2_DefaultOptionsAsync()
 
             Assert.Greater(allNodes.Count, 0, "Should have found at least one node");
 
+            // Use a fixed timestamp so both exports have identical LastModified fields,
+            // preventing flakiness from sub-second precision differences.
+            var lastModified = DateTime.UtcNow;
+
             // Export with default options
             string tempFile = Path.GetTempFileName();
             try
@@ -347,7 +352,7 @@ public async Task ExportNodesToNodeSet2_DefaultOptionsAsync()
                         ServerUris = Session.ServerUris
                     };
 
-                    CoreClientUtils.ExportNodesToNodeSet2(systemContext, allNodes, stream, NodeSetExportOptions.Default);
+                    CoreClientUtils.ExportNodesToNodeSet2(systemContext, allNodes, stream, NodeSetExportOptions.Default, lastModified);
                 }
 
                 // Read it back and verify values are not exported
@@ -380,20 +385,16 @@ public async Task ExportNodesToNodeSet2_DefaultOptionsAsync()
                             ServerUris = Session.ServerUris
                         };
 
-                        CoreClientUtils.ExportNodesToNodeSet2(systemContext, allNodes, stream, NodeSetExportOptions.Complete);
+                        CoreClientUtils.ExportNodesToNodeSet2(systemContext, allNodes, stream, NodeSetExportOptions.Complete, lastModified);
                     }
 
                     var completeFile = new FileInfo(tempFileComplete);
                     long completeSize = completeFile.Length;
 
                     // Default should be smaller or equal to complete
                     // (Equal if nodes don't have values to export)
-                    if (completeSize < defaultSize)
-                    {
-                        TestContext.AddTestAttachment(tempFileComplete, "Complete export file");
-                        TestContext.AddTestAttachment(tempFile, "Default export file");
-                        Assert.LessOrEqual(defaultSize, completeSize, "Default export should not be larger than Complete");
-                    }
+                    Assert.That(defaultSize, Is.LessThanOrEqualTo(completeSize),
+                        "Default export should not be larger than Complete");
                 }
                 finally
                 {

Tests/Opc.Ua.Client.Tests/SessionTests.cs

@@ -1240,7 +1240,7 @@ public async Task OpenAsyncShouldOpenSessionSuccessfullyAsync()
                 {
                     Results =
                     [
-                        new (new Variant(0u))
+                        new (new Variant(0))
                     ],
                     DiagnosticInfos = []
                 }))

Tests/Opc.Ua.Core.Tests/Stack/State/ConditionStateTests.cs

@@ -328,6 +328,64 @@ public void DerivedClassCanOverrideEvaluateRetainStateOnEnable()
             Assert.That(condition.Retain.Value, Is.True);  // Custom logic always sets to true
         }
 
+        /// <summary>
+        /// Test that the branch collection can be safely created, enumerated and
+        /// cleared from multiple threads concurrently without throwing.
+        /// Reproduces the race that previously crashed the process when the
+        /// branch dictionary was enumerated while being modified.
+        /// </summary>
+        [Test]
+        public void ConcurrentBranchAccessIsThreadSafe()
+        {
+            var condition = new ConditionState(null);
+            condition.Create(m_context, new NodeId(1), "Condition", null, true);
+            // Ensure each created branch receives a unique EventId.
+            condition.AutoReportStateChanges = true;
+            condition.SetEnableState(m_context, true);
+
+            const int iterations = 2000;
+            using var cts = new System.Threading.CancellationTokenSource();
+
+            // Writer: continuously creates and clears branches.
+            System.Threading.Tasks.Task writer = System.Threading.Tasks.Task.Run(() =>
+            {
+                for (int i = 0; i < iterations; i++)
+                {
+                    condition.CreateBranch(m_context, new NodeId((uint)(i + 2)));
+                    if (i % 10 == 0)
+                    {
+                        condition.ClearBranches();
+                    }
+                }
+            });
+
+            // Readers: continuously enumerate and count branches.
+            System.Threading.Tasks.Task reader = System.Threading.Tasks.Task.Run(() =>
+            {
+                while (!cts.Token.IsCancellationRequested)
+                {
+                    foreach (System.Collections.Generic.KeyValuePair<string, ConditionState> branch
+                        in condition.GetBranches())
+                    {
+                        Assert.That(branch.Value, Is.Not.Null);
+                    }
+
+                    _ = condition.GetBranchCount();
+                }
+            });
+
+            try
+            {
+                writer.GetAwaiter().GetResult();
+            }
+            finally
+            {
+                cts.Cancel();
+            }
+
+            Assert.That(reader.Wait(System.TimeSpan.FromSeconds(5)), Is.True, "Reader task did not stop after cancellation.");
+        }
+
         /// <summary>
         /// Test condition that exposes method to force Retain value for testing.
         /// </summary>