Skip to content

[Server]: Queue Events from the Server Node using multiple channel consumers#3823

Open
Copilot wants to merge 15 commits into
masterfrom
copilot/add-special-case-for-consumer-task
Open

[Server]: Queue Events from the Server Node using multiple channel consumers#3823
Copilot wants to merge 15 commits into
masterfrom
copilot/add-special-case-for-consumer-task

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jun 1, 2026

Description

This pull request introduces a scalable consumer task model for the MonitoredNode2 class, optimizing the handling of event notifications for notifier nodes. Any notifier node can opt into (or out of) dynamic scaling of consumer tasks based on the number of event monitored items, improving throughput and responsiveness under high event loads. The Server node (ObjectIds.Server) is always auto-opted-in. The implementation includes safe scaling up and down of consumers, robust disposal logic, Fluent API integration, and tests to verify correct delivery of events.

Scalable consumer task management:

  • Added an enableMultipleEventConsumers constructor parameter to MonitoredNode2 that enables dynamic scaling of consumer tasks to match the number of event monitored items. The Server node (ObjectIds.Server) always auto-opts-in regardless of this parameter.
  • Added AllowMultipleEventConsumers(NodeId, bool enable = true) protected method on AsyncCustomNodeManager with an internal MultiConsumerNodeIds registry, so any node manager can programmatically opt in or out specific nodes.
  • Added IsMultipleEventConsumerNode(NodeId) to the IAsyncNodeManager interface, allowing consumers to query multi-consumer opt-in status through the interface without casting to AsyncCustomNodeManager.
  • Both MonitoredNodeMonitoredItemManager and SamplingGroupMonitoredItemManager use the IAsyncNodeManager interface method when creating MonitoredNode2 instances.

Fluent API integration:

  • Added AllowMultipleEventConsumers(bool enable = true) method to INodeBuilder and its NodeBuilder implementation, allowing any notifier node to be opted in or out via the fluent builder pipeline (e.g. builder.Node("myNotifier").AllowMultipleEventConsumers()).
  • The NodeManagerBuilder routes the registration to the owning AsyncCustomNodeManager's multi-consumer registry, supporting both opt-in and opt-out.

Testing and verification:

  • Added unit tests to verify multi-consumer behavior for both Server node (auto-opt-in) and non-Server nodes (explicit opt-in), as well as default single-consumer behavior for nodes without opt-in.
  • Fixed race condition in permission cache tests (OnMonitoredNodeChanged_DefaultPermissionsChanged_CacheInvalidated and InvalidatePermissionCacheForSession tests) that caused flaky CI failures. The tests now signal on QueueValue (which fires after the cache is populated) instead of ValidateRolePermissionsAsync (which fires before), ensuring cache invalidation operates on a populated cache.

Related Issues

Checklist

  • 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.

Copilot AI added 8 commits May 31, 2026 16:35
…ngle-channel bottleneck

For the Server node (ObjectIds.Server), which subscribes to all events,
the previous single-channel/single-consumer architecture serialized event
processing across all event monitored items. This change creates a dedicated
channel and consumer task per event MonitoredItem on the Server node,
allowing concurrent event processing and eliminating the bottleneck.
Add ConsumerCount option to HistorianCaptureOptions (default 1).
When ConsumerCount > 1, multiple consumer tasks drain the same
bounded Channel concurrently, improving throughput when the
historian provider has significant insert latency.
…nitoredNode2

Replace per-item event channels/consumers with a single shared event
channel drained by multiple consumer tasks for the Server node.
This reduces write amplification and simplifies lifecycle management
while retaining concurrent event processing throughput.

Add ServerNode_MultipleEventConsumers_AllEventsDelivered test.
Replace fixed event consumer count with dynamic scaling that starts
with 1 consumer task and adds/removes consumers as event monitored
items are created/deleted on the Server node.

- Remove k_serverNodeEventConsumerCount constant and internal constructor
- Always start with 1 event consumer task
- Scale up when event MI is added (1 consumer per MI)
- Scale down when event MI is removed (keep minimum of 1)
- Use List<EventConsumerEntry> with lock for thread-safe scaling
- Update tests to use public constructor (scaling is automatic)
@romanett romanett changed the title refactor: remove separate event channel in MonitoredNode2, use single channel with multiple consumers [Server]: Queue Events from the Server Node using multiple channel consumers Jun 1, 2026
Comment thread Libraries/Opc.Ua.Server/NodeManager/MonitoredItem/MonitoredNode.cs
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@romanett romanett marked this pull request as ready for review June 1, 2026 05:32
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 52.43902% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.13%. Comparing base (01d7162) to head (633ed0b).

Files with missing lines Patch % Lines
....Server/NodeManager/MonitoredItem/MonitoredNode.cs 59.61% 19 Missing and 2 partials ⚠️
...braries/Opc.Ua.Server/Fluent/NodeManagerBuilder.cs 0.00% 10 Missing ⚠️
...pc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs 14.28% 6 Missing ⚠️
Libraries/Opc.Ua.Server/Fluent/NodeBuilder.cs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3823      +/-   ##
==========================================
- Coverage   71.13%   71.13%   -0.01%     
==========================================
  Files         778      778              
  Lines      143274   143351      +77     
  Branches    24234    24252      +18     
==========================================
+ Hits       101918   101967      +49     
- Misses      33005    33030      +25     
- Partials     8351     8354       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI requested a review from romanett June 1, 2026 05:42
Comment thread Libraries/Opc.Ua.Server/NodeManager/MonitoredItem/MonitoredNode.cs Outdated
Copilot AI requested a review from romanett June 1, 2026 06:11
Comment thread Libraries/Opc.Ua.Server/NodeManager/MonitoredItem/MonitoredNode.cs Outdated
Copilot AI requested a review from marcschier June 1, 2026 10:12
Comment thread Libraries/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs
Copilot AI requested a review from romanett June 1, 2026 10:43
Copy link
Copy Markdown
Collaborator

@marcschier marcschier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Comment thread Libraries/Opc.Ua.Server/Fluent/INodeBuilder.cs Outdated
Comment thread Tests/Opc.Ua.Server.Tests/AsyncCustomNodeManagerTests.cs
…th opt-out support, fix method ordering and test style
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jun 1, 2026

@copilot address my feedback

All feedback addressed in the latest commit:

  • Renamed EnableMultipleEventConsumersAllowMultipleEventConsumers(bool enable = true) with opt-out support
  • Moved IsMultiConsumerNode before private fields in both managers
  • Used full method body in test adapter

Copilot AI requested a review from marcschier June 1, 2026 11:41
@romanett
Copy link
Copy Markdown
Contributor

romanett commented Jun 1, 2026

@copilot check the flaky CI and fix the test to run reliably

…QueueValue

The tests were signaling on ValidateRolePermissionsAsync which fires before the
cache is populated (line 545). By the time DefaultPermissionsChanged cleared the
cache, it was still empty, and the consumer then populated it after the clear.
The second notification then found a populated cache and skipped validation.

Fix: signal on QueueValue which fires AFTER the cache is populated, ensuring
the cache invalidation actually clears a populated cache.
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jun 1, 2026

@copilot check the flaky CI and fix the test to run reliably

Fixed in the latest commit. The race condition was caused by signaling on ValidateRolePermissionsAsync (which fires before the cache is populated on line 545) instead of QueueValue (which fires after). When DefaultPermissionsChanged fired between the validation call and the cache write, it cleared an empty cache, and the consumer then populated it — causing the second notification to hit the cache and skip re-validation. Applied the same fix to the InvalidatePermissionCacheForSession tests that had the same pattern.

Copilot AI requested a review from romanett June 1, 2026 15:06
@marcschier
Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

…-case-for-consumer-task

# Conflicts:
#	Tests/Opc.Ua.Server.Tests/MonitoredNode2Tests.cs
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jun 1, 2026

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflicts in MonitoredNode2Tests.cs. The conflicts were caused by master adding firstValidationSignal declarations that our branch had already replaced with QueueValue-based signaling. Also restored the license header that was corrupted during the merge. All 26 MonitoredNode2 tests pass.

Copilot AI requested a review from marcschier June 1, 2026 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants