Synchronize ConditionState branch collection access by Copilot · Pull Request #3895 · OPCFoundation/UA-.NETStandard

Copilot · 2026-06-19T06:59:01Z

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 and read the CONTRIBUTING doc.
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.
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.

CLAassistant · 2026-06-19T06:59:08Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ marcschier
✅ mrsuciu
❌ Copilot
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codecov · 2026-06-19T10:09:05Z

Codecov Report

❌ Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.14%. Comparing base (35708e8) to head (6e95135).

Files with missing lines	Patch %	Lines
Stack/Opc.Ua.Types/State/NodeStateCollection.cs	50.00%	2 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           master378    #3895      +/-   ##
=============================================
+ Coverage      60.12%   60.14%   +0.01%     
=============================================
  Files            378      378              
  Lines          79061    79063       +2     
  Branches       13837    13838       +1     
=============================================
+ Hits           47539    47552      +13     
+ Misses         27099    27091       -8     
+ Partials        4423     4420       -3

Files with missing lines	Coverage Δ
...ies/Opc.Ua.Client/CoreClientUtils.NodeSetExport.cs	`56.38% <100.00%> (ø)`
Stack/Opc.Ua.Types/State/NodeStateCollection.cs	`49.01% <50.00%> (-0.39%)`	⬇️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This PR addresses a concurrency defect in ConditionState where the branch dictionary could be mutated while being enumerated from other threads, leading to crashes. It adds synchronization around branch access and updates branch enumeration to use a stable snapshot, plus a stress test to validate the fix.

Changes:

Added a per-ConditionState lock to guard all reads/writes of the branch dictionary.
Changed GetBranches() to return a snapshot copy to make enumeration safe under concurrent mutation.
Added a multithreaded stress test that concurrently creates/clears branches while enumerating/counting.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
Stack/Opc.Ua.Core/Stack/State/ConditionState.cs	Synchronizes branch dictionary access and returns snapshot copies for safe enumeration.
Tests/Opc.Ua.Core.Tests/Stack/State/ConditionStateTests.cs	Adds a concurrency stress test for branch create/clear vs enumerate/count.

Co-authored-by: Copilot Autofix powered by AI <[email protected]>

marcschier · 2026-06-21T10:09:17Z

@copilot address PR feedback and ensure CI run is green

mrsuciu · 2026-06-23T15:19:59Z

@marcschier @copilot
The logs continuously show the following errors which are both test errors, the second one having the potential to invalidate a test run:

First (multiple times therefore it must be caught somewhere probably and is handled):
(BadTypeMismatch) 'The server returned data value of type UInt32 when a value of type Int32 was expected.', EndpointUrl=(null), RequestCount=0/0
Seems to be caused by the following:

The keep-alive read validates the returned value as Int32 (Opc.Ua.Client.Session::OnSendKeepAliveAsync(line 3091) and casts it to int (line 3109),
while the test mock (SessionTests.cs (line 1243) returns "new Variant(0u)" which is UInt32.

Second:
Failed ExportNodesToNodeSet2_DefaultOptionsAsync [38 ms]
Error Message:
Default export should not be larger than Complete
Assert.That(arg1, Is.LessThanOrEqualTo(arg2))
Expected: less than or equal to 3703
But was: 3704

Is Flaky due to the posibility of NoteStateCollection:SaveAsNodeSet2() for the LastModified DateTime value to differ in precision on the two calls of the ExportNodesToNodeSet2 at two moments in time.
If the above assumption is true than comparison to contain the exact size or be less then is "wrong" intuitively.

marcschier · 2026-06-23T15:33:13Z

@copilot, second test failure, test was fixed in master, port it here, investigate the first test issue, goal is ci run gree

@marcschier @copilot The logs continuously show the following errors which are both test errors, the second one having the potential to invalidate a test run:

First (multiple times therefore it must be caught somewhere probably and is handled): (BadTypeMismatch) 'The server returned data value of type UInt32 when a value of type Int32 was expected.', EndpointUrl=(null), RequestCount=0/0 Seems to be caused by the following:

The keep-alive read validates the returned value as Int32 (Opc.Ua.Client.Session::OnSendKeepAliveAsync(line 3091) and casts it to int (line 3109), while the test mock (SessionTests.cs (line 1243) returns "new Variant(0u)" which is UInt32.

Second: Failed ExportNodesToNodeSet2_DefaultOptionsAsync [38 ms] Error Message: Default export should not be larger than Complete Assert.That(arg1, Is.LessThanOrEqualTo(arg2)) Expected: less than or equal to 3703 But was: 3704

Is Flaky due to the posibility of NoteStateCollection:SaveAsNodeSet2() for the LastModified DateTime value to differ in precision on the two calls of the ExportNodesToNodeSet2 at two moments in time. If the above assumption is true than comparison to contain the exact size or be less then is "wrong" intuitively.

…og noise - SessionTests.cs: fix keep-alive mock returning UInt32 (0u); changed to Int32 (0) so ValidateDataValue(typeof(int)) no longer logs BadTypeMismatch errors. - NodeStateCollection.SaveAsNodeSet2: add overload with DateTime? lastModified parameter so callers can pin the timestamp for deterministic output. - CoreClientUtils.ExportNodesToNodeSet2: add optional DateTime? lastModified parameter, passed through to SaveAsNodeSet2. - NodeSetExportTest.ExportNodesToNodeSet2_DefaultOptionsAsync: capture a single DateTime.UtcNow and pass it to both Default and Complete exports so the LastModified field is identical in both files, eliminating the flaky byte-count difference. Also switch to Assert.That/Is.LessThanOrEqualTo per project style.

Copilot · 2026-06-23T15:50:24Z

@copilot, second test failure, test was fixed in master, port it here, investigate the first test issue, goal is ci run gree

@marcschier @copilot The logs continuously show the following errors w...

Both issues addressed in the latest commit:

First (keep-alive BadTypeMismatch log noise): The mock in SessionTests.cs was returning new Variant(0u) (UInt32) while OnSendKeepAliveAsync validates with typeof(int). The error was caught and didn't fail the test, but produced noisy log output on every keep-alive cycle. Fixed by changing the mock to new Variant(0) (Int32).

Second (ExportNodesToNodeSet2_DefaultOptionsAsync flakiness): Ported the lastModified parameter from master to both NodeStateCollection.SaveAsNodeSet2 (new 4-parameter overload) and CoreClientUtils.ExportNodesToNodeSet2 (optional DateTime? lastModified parameter). The test now captures a single DateTime.UtcNow before both exports and passes it to both calls, so the LastModified XML field is identical in both outputs — eliminating the byte-count race. Also updated the assertion to Assert.That(..., Is.LessThanOrEqualTo(...)) per project style.

Initial plan

21a40f6

Copilot AI assigned Copilot and marcschier Jun 19, 2026

Copilot started work on behalf of marcschier June 19, 2026 06:59 View session

Copilot AI linked an issue Jun 19, 2026 that may be closed by this pull request

Condition's branch collection (ConditionState.m_branches) accessed from multiple threads without synchronization, resulting in crashes #3894

Closed

5 tasks

Synchronize ConditionState branch collection access

1ae3ced

Copilot AI changed the title ~~[WIP] Fix race condition in ConditionState branch collection~~ Synchronize ConditionState branch collection access Jun 19, 2026

Copilot finished work on behalf of marcschier June 19, 2026 07:05

Copilot AI requested a review from marcschier June 19, 2026 07:05

marcschier approved these changes Jun 19, 2026

View reviewed changes

marcschier marked this pull request as ready for review June 19, 2026 10:06

marcschier requested review from Copilot, mrsuciu and romanett June 19, 2026 10:06

Copilot started reviewing on behalf of marcschier June 19, 2026 10:07 View session

Copilot AI reviewed Jun 19, 2026

View reviewed changes

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

Comment thread Tests/Opc.Ua.Core.Tests/Stack/State/ConditionStateTests.cs Outdated

Apply suggestions from code review

76c8721

Co-authored-by: Copilot Autofix powered by AI <[email protected]>

Copilot started work on behalf of marcschier June 19, 2026 10:19 View session

marcschier added the 1.5.378 Only affects 1.5.378 (pre 2.0) label Jun 19, 2026

Fix GetBranches XML doc to reflect snapshot semantics

a5bdc62

Copilot finished work on behalf of marcschier June 19, 2026 10:20

Copilot AI requested a review from marcschier June 19, 2026 10:20

romanett reviewed Jun 21, 2026

View reviewed changes

Comment thread Stack/Opc.Ua.Core/Stack/State/ConditionState.cs Outdated

romanett reviewed Jun 21, 2026

View reviewed changes

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

romanett approved these changes Jun 21, 2026

View reviewed changes

Copilot started work on behalf of marcschier June 21, 2026 10:09 View session

Copilot started work on behalf of marcschier June 21, 2026 10:12 View session

Copilot started work on behalf of marcschier June 23, 2026 15:33 View session

Copilot finished work on behalf of marcschier June 23, 2026 15:50

marcschier approved these changes Jun 23, 2026

View reviewed changes

marcschier added the ready Ready to merge once CI Passes label Jun 23, 2026

mrsuciu merged commit 5924fe6 into master378 Jun 24, 2026
90 of 92 checks passed

marcschier deleted the copilot/fix-condition-branches-crash branch July 8, 2026 14:55

dependabot Bot mentioned this pull request Jul 22, 2026

Bump the opcfoundation group with 11 updates OPCFoundation/UA-.NETStandard-Samples#710

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Synchronize ConditionState branch collection access#3895

Synchronize ConditionState branch collection access#3895
mrsuciu merged 9 commits into
master378from
copilot/fix-condition-branches-crash

Copilot AI commented Jun 19, 2026 •

edited

Loading

Uh oh!

CLAassistant commented Jun 19, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jun 19, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcschier commented Jun 21, 2026

Uh oh!

mrsuciu commented Jun 23, 2026

Uh oh!

marcschier commented Jun 23, 2026

Uh oh!

Copilot AI commented Jun 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

Copilot AI commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Uh oh!

CLAassistant commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcschier commented Jun 21, 2026

Uh oh!

mrsuciu commented Jun 23, 2026

Uh oh!

marcschier commented Jun 23, 2026

Uh oh!

Copilot AI commented Jun 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Copilot AI commented Jun 19, 2026 •

edited

Loading

CLAassistant commented Jun 19, 2026 •

edited

Loading

codecov Bot commented Jun 19, 2026 •

edited

Loading