[Server] Add live NodeManager lifecycle support - #4094
Merged
Conversation
…ode-manager-lifecycle # Conflicts: # src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs
…ode-manager-lifecycle # Conflicts: # src/Opc.Ua.Server/NodeManager/MasterNodeManager.cs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
7 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds live, in-process lifecycle support for NodeManagers (add/reload/remove) with coordinated routing, monitored-item transfer/detach+recovery semantics, and request/session/subscription safety guards.
Changes:
- Introduces
INodeManagerLifecycle(plus hosted forwarding shim) and supporting lifecycle/routing contracts to safely add/reload/remove NodeManagers at runtime. - Extends monitored item + queue handling to preserve required
BadNodeIdUnknownnotifications across deletion/detach/rebind and durable restarts. - Adds broad test coverage and documentation updates for live lifecycle behavior, continuation-point invalidation, request tracking, and durable monitored-item recovery.
Reviewed changes
Copilot reviewed 65 out of 66 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Opc.Ua.Types.Tests/Nodes/TypeTableTests.cs | Adds regression coverage for reparenting subtypes. |
| tests/Opc.Ua.Subscriptions.Tests/MonitorTriggeringTests.cs | Attempts to reduce flakiness in trigger-chain sampling via pre-queue + delay. |
| tests/Opc.Ua.Server.Tests/SubscriptionTests.cs | Adds lifecycle-focused subscription tests (closing sessions, deletion-in-progress, has-items by adapter). |
| tests/Opc.Ua.Server.Tests/StandardServerCoverageTests.cs | Covers new request-validation-in-scope behavior. |
| tests/Opc.Ua.Server.Tests/SessionContinuationPointsTests.cs | Adds tests for invalidating browse continuation points by node manager. |
| tests/Opc.Ua.Server.Tests/RuntimeNodeSet/RuntimeNodeSetTestServer.cs | Exposes test NodeSet resource stream for reuse. |
| tests/Opc.Ua.Server.Tests/RequestManagerTests.cs | Adds extensive tests for request snapshot draining and scope semantics. |
| tests/Opc.Ua.Server.Tests/NodeManager/MasterNodeManagerDeterministicTests.cs | Adds tests around detached monitored-item behavior (modify/transfer/mode). |
| tests/Opc.Ua.Server.Tests/NodeManager/DeterministicServerMock.cs | Ensures CoreNodeManager exists for detached monitored-item ownership. |
| tests/Opc.Ua.Server.Tests/NodeManager/CustomNodeManagerDeterministicTests.cs | Adds tests for teardown/type-tree updates and external-reference tracking. |
| tests/Opc.Ua.Server.Tests/MonitoredItemLifecycleTests.cs | New suite: deleted/detached/rebind publication ordering and required marker behavior. |
| tests/Opc.Ua.Server.Tests/Hosting/OpcUaServerHostedServiceCoverageTests.cs | Updates ctor coverage due to hosted lifecycle injection. |
| tests/Opc.Ua.Server.Tests/Hosting/HostedNodeManagerLifecycleTests.cs | New suite: hosted lifecycle forwarding shim behavior. |
| tests/Opc.Ua.Server.Tests/DurableMonitoredItemTests.cs | Adds durable-queue restart tests preserving lifecycle epochs/markers. |
| tests/Opc.Ua.Server.Tests/AsyncCustomNodeManagerTests.cs | Adds hooks for event subscription + node removal callbacks. |
| tests/Opc.Ua.Aot.Tests/HostingAotTests.cs | Verifies INodeManagerLifecycle DI resolution is AOT-safe. |
| src/Opc.Ua.Types/Nodes/TypeTable.cs | Fixes subtype reparenting cleanup and encoding reset. |
| src/Opc.Ua.Server/Subscription/Subscription.cs | Adds monitored-item tracking/snapshot APIs and mutation coordination for MI operations. |
| src/Opc.Ua.Server/Subscription/MonitoredItem/QueueHandler/DataChangeQueueHandler.cs | Adds lifecycle-aware required marker support and improved durable queue handling. |
| src/Opc.Ua.Server/Subscription/MonitoredItem/MonitoredItem.cs | Implements lifecycle contract; queues required BadNodeIdUnknown; supports durable recovery + rebind/detach. |
| src/Opc.Ua.Server/Subscription/MonitoredItem/IMonitoredItemLifecycle.cs | New internal lifecycle contract for monitored items. |
| src/Opc.Ua.Server/Subscription/MonitoredItem/DetachedMonitoredItemOwnership.cs | New helper to assign long-lived ownership for detached monitored items. |
| src/Opc.Ua.Server/Subscription/ISubscriptionDeletionRegistry.cs | New interface to gate MI mutations during subscription deletion. |
| src/Opc.Ua.Server/Session/SessionContinuationPoints.cs | Adds removal/invalidation of browse continuation points by node manager. |
| src/Opc.Ua.Server/Session/Session.cs | Exposes continuation-point invalidation for lifecycle teardown. |
| src/Opc.Ua.Server/Session/ISessionClosingRegistry.cs | New interface enabling session-closing checks. |
| src/Opc.Ua.Server/Server/StandardServer.cs | Adds NodeManagerLifecycle, request scopes, lifecycle shutdown coordination, and complex-type refresh hook. |
| src/Opc.Ua.Server/Server/ServerInternalData.cs | Tracks closing sessions and implements ISessionClosingRegistry. |
| src/Opc.Ua.Server/Server/RequestManager.cs | Adds validation/execution scopes and snapshot drain (WaitForCurrentRequestsAsync). |
| src/Opc.Ua.Server/RuntimeNodeSet/RuntimeNodeSetNodeManager.cs | Implements reload participant; tracks added references; exposes semantic/type info helpers. |
| src/Opc.Ua.Server/RuntimeNodeSet/RuntimeNodeSetLifecycleExtensions.cs | New convenience extensions for adding/reloading runtime NodeSets via lifecycle. |
| src/Opc.Ua.Server/NugetREADME.md | Documents live add/reload/remove semantics for runtime NodeSets. |
| src/Opc.Ua.Server/NodeManager/MonitoredItem/SamplingGroupMonitoredItemManager.cs | Adds attach/detach snapshot lifecycle operations and safer restore behavior. |
| src/Opc.Ua.Server/NodeManager/MonitoredItem/MonitoredNodeMonitoredItemManager.cs | Adds attach/detach snapshot lifecycle operations and safer restore behavior. |
| src/Opc.Ua.Server/NodeManager/MonitoredItem/MonitoredNode.cs | Adds rebind support and lifecycle deletion signaling; strengthens locking around callbacks/consumers. |
| src/Opc.Ua.Server/NodeManager/MonitoredItem/INodeManagerMonitoredItemLifecycle.cs | New contract for node managers to detach/attach/restore monitored items. |
| src/Opc.Ua.Server/NodeManager/MonitoredItem/IMonitoredItemManagerLifecycle.cs | New internal contract for MI managers to detach/attach items safely. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/NodeManagerRoutingTable.cs | New routing table with copy-on-write snapshots and visibility control. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/NodeManagerRegistration.cs | New generation-aware registration handle for live lifecycle ops. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/ISubscriptionMonitoredItemLifecycle.cs | New subscription contract to snapshot/recover monitored items during lifecycle ops. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/INodeManagerReloadParticipant.cs | New contract letting managers transfer references on reload. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/INodeManagerMutationCoordinator.cs | New coordinator contract to serialize MI mutations around lifecycle transitions. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/INodeManagerMonitoredItemTracker.cs | New public API for checking subscription MI ownership per node manager. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/INodeManagerMonitoredItemRecovery.cs | New recovery contract for detached items based on node reappearance. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/INodeManagerLifecycle.cs | New public lifecycle API for add/reload/remove operations. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/INodeManagerContinuationPointTracker.cs | New public API to invalidate browse continuation points by manager. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/IDynamicNodeManagerHost.cs | New internal host contract for transactional publish/replace/unpublish flows. |
| src/Opc.Ua.Server/NodeManager/Adapters/AsyncNodeManagerAdapter.cs | Implements monitored-item lifecycle forwarding over sync node managers. |
| src/Opc.Ua.Server/Hosting/OpcUaServerHostedService.cs | Attaches/detaches hosted lifecycle forwarding shim to the live server lifecycle. |
| src/Opc.Ua.Server/Hosting/OpcUaServerBuilderExtensions.cs | Registers HostedNodeManagerLifecycle and exposes it as INodeManagerLifecycle in DI. |
| src/Opc.Ua.Server/Hosting/HostedNodeManagerLifecycle.cs | New forwarding shim for DI scenarios prior to server creation. |
| src/Opc.Ua.Server/ComplexTypes/ServerComplexTypeSystem.cs | Adds a complex-type load path that can pre-scan an additional prepared node manager. |
| src/Opc.Ua.Server/ComplexTypes/AddressSpaceComplexTypeResolver.cs | Supports resolving nodes from a prepared (not-yet-published) node manager first. |
| docs/SourceGeneratedNodeManagers.md | Updates runtime NodeSet positioning to include live lifecycle. |
| docs/RuntimeNodeSets.md | Documents startup + live lifecycle semantics, recovery rules, and continuation-point invalidation. |
| docs/DependencyInjection.md | Documents DI availability for INodeManagerLifecycle and usage example. |
| docs/ComplexTypes.md | Updates complex-type behavior docs for live lifecycle generations. |
Comments suppressed due to low confidence (4)
src/Opc.Ua.Server/Subscription/MonitoredItem/QueueHandler/DataChangeQueueHandler.cs:1
- This loop can spin indefinitely if
Dequeuekeeps returningfalse(e.g., due to a persistent durable-queue error, disposal, or an unexpected queue implementation). Consider adding a bounded retry/timeout and throwing aServiceResultException(or falling back to resizing without draining) when progress cannot be made, so server threads cannot hang.
src/Opc.Ua.Server/Subscription/Subscription.cs:1 - Passing
null!entries incleanupErrorsis risky becauseDeleteMonitoredItemsAsyncimplementations may dereference eachServiceResultentry. Prefer initializingcleanupErrorswithServiceResult.Goodfor good results (or cloningerrorsand letting deletes overwrite) to avoid potentialNullReferenceExceptionduring cleanup.
src/Opc.Ua.Server/Session/SessionContinuationPoints.cs:1 - Both
m_store?.RemoveContinuationPoint(...)andcontinuationPoint.Dispose()run while holdingm_lock. If either operation is slow (e.g., store persistence or disposal work), this can unnecessarily block other continuation-point operations. Consider collecting matching continuation points (and ids to remove) under the lock, then performing store removal + disposal after releasing the lock.
tests/Opc.Ua.Subscriptions.Tests/MonitorTriggeringTests.cs:1 - The fixed
Task.Delay(300)can make the test slower and still flaky across environments. Prefer a deterministic wait condition tied to the subscription’s sampling/publishing behavior (e.g., wait until a notification is observed/queued, or wait for at least one sampling interval via configuration) instead of a hard-coded delay.
Collaborator
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
romanett
reviewed
Jul 25, 2026
romanett
reviewed
Jul 25, 2026
romanett
reviewed
Jul 25, 2026
romanett
reviewed
Jul 25, 2026
This was referenced Jul 25, 2026
romanett
reviewed
Jul 25, 2026
romanett
reviewed
Jul 25, 2026
romanett
reviewed
Jul 25, 2026
romanett
reviewed
Jul 27, 2026
romanett
reviewed
Jul 27, 2026
romanett
reviewed
Jul 27, 2026
romanett
reviewed
Jul 27, 2026
romanett
reviewed
Jul 27, 2026
romanett
reviewed
Jul 27, 2026
Addresses review feedback that this PR reached much further into the Subscription area than the NodeManager lifecycle feature needs. SubscriptionManager is restored to its state before this PR, keeping only the three guards that reject work for a Session that is closing. The deletion claim registry, the session closing gates, the closed-session bookkeeping and the deletion restore paths are gone, and with them ISubscription.IsDeleting and the four guards in Subscription that consulted it. The file goes from 729 changed lines to 20. Queueing is disabled again for a MonitoredItem with QueueSize <= 1, as it was before this PR. That is the simple behaviour: without a queue the Client is served the last sampled value, so the missing-Node notification simply becomes that value and a later sample replaces it. Losing values is expected for a MonitoredItem without queuing, so the notification gets no protection there and needs no special handling anywhere. Restoring that shortcut removes the reason for the rest: the publish path goes back to its original branches, the item no longer re-queues a stored notification when it is restored, and EnsureDataChangeQueueHandler, m_deletionNotificationQueuedForEpoch and m_restoredDataChangeQueueAvailable are deleted. It also removes the regression this PR introduced, where an ordinary sampled Bad_NodeIdUnknown could block a size-one queue: with queueing disabled there is no queue to block. In the queue handler EnsurePhysicalCapacity is gone, together with the drain and refill helpers it needed. The marker now lives inside the configured queue, so the physical queue never has to grow, and the limit is the queue's own size again rather than a separate field. SetQueueSize is the original implementation, preserving only the flag that keeps the marker protected across a resize. The internal constructor that carried the extra queue size and diagnostics mask is unused and removed, leaving the two constructors that 1.5.378 shipped. Five tests that asserted the removed behaviour are deleted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
romanett
reviewed
Jul 27, 2026
romanett
reviewed
Jul 27, 2026
romanett
reviewed
Jul 27, 2026
The restore path inferred both flags from the stored last value carrying BadNodeIdUnknown. That is fragile in both directions: an item whose node simply reported that status came back marked deleted, and an item that was deleted but had since published the notification did not. IStoredMonitoredItem now carries IsDeleted and IsDetached, so the state is saved and restored as it was rather than guessed from a status code. The change also showed the inference was wrong rather than merely fragile. DurableConsumedNodeIdUnknownIsNotRepublishedAfterRestart only marked the item deleted, yet asserted the restored item was detached as well, which the inference invented. Deleting a monitored node detaches the item and marks it deleted, so the test now establishes both, as production does. Also drops the reference to INodeManagerContinuationPointTracker from the migration guide; the interface was added and removed inside this PR, so a consumer coming from 1.5.378 has never seen it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
Two things this PR changed that it had no reason to. AddValueToQueue set m_readyToTrigger unconditionally, which made the conditional above it dead and changed the triggering mechanism: a triggered item would fire on its very first value, where before it only fired once a previous value existed. The unconditional set is removed, so triggering behaves as it did before this PR. The missing-Node notification sets the flag in QueueNodeIdUnknown, which is where that behaviour belongs. TransferSubscriptions rejected a transfer when the session that currently owns the subscription is closing. That is backwards: a Client transfers a subscription to a new session precisely because the old one is going away, and the server already supports transferring subscriptions abandoned by a closed session. The guard is removed, leaving the two guards on the requesting session, which is what the review asked for. SubscriptionManager is now 9 lines away from its state before this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Serialize source queue closure with subscription transfer and clear ownership only when the closing session remains the owner. Add deterministic concurrency and stale-queue regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Claim expiration only from the exact current queue or abandoned source, coordinate transfer and expiration under the manager semaphore, and restore ownership on transfer failure. Add deterministic stale-snapshot and current-timeout regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nager A request dispatched while StartApplicationAsync is between publishing the server datastore and creating the request manager dereferenced a null request manager and faulted the connection with BadInternalError.
romanett
reviewed
Jul 28, 2026
SessionPublishQueue tested for the concrete Subscription type and fell back to comparing Session itself when the test failed. The fallback was also weaker than the real path: it compared the owner and then closed in two steps, so a transfer landing in between could strip a subscription that had already moved to another session. ISubscription gains bool SessionClosed(ISession closingSession), which releases the subscription only when the closing session still owns it and reports whether it did. The check is now atomic with the release, because it happens inside the subscription under its own lock. The parameterless SessionClosed() is marked obsolete; it cannot express the ownership test and had no production caller left. The type test and the fallback in SessionPublishQueue are gone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
marcschier
commented
Jul 28, 2026
This document covers both sides of the stack, and the section directly above these notes discusses the client-side ISubscription and ISubscriptionManager from src/Opc.Ua.Client/Subscription/. A bare "ISession" or "ISubscription" therefore reads as the client type a consumer has just been told about. Both headings and first mentions are now namespace qualified, and each note says explicitly which interface it means and which one it does not. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
Three conflicts, all where the ActivateSession transfer security work (#4050) met this branch's subscription transfer race fix. Both intents are kept. Subscription.TransferSessionAsync: master assigns Session at the top and calls UpdateOwnerIdentity to keep the recorded owner identity in sync for the OPC 10000-4 5.7.3.1 transfer checks. This branch defers the assignment until after the monitored items have moved and verifies ownership did not change meanwhile. Kept the deferred assignment and moved the UpdateOwnerIdentity call to where the owner actually changes, so the invariant that owner identity follows Session still holds. TryRestoreSessionAfterFailedTransfer rolls the identity back with the owner for the same reason. SubscriptionManager.TransferSubscriptionsAsync: master replaced raw token equality with IsTransferIdentityCompatible, which compares the resolved ClientUserId so a refreshed issued token still transfers. This branch had moved the identity and ownership checks inside the semaphore that guards against a concurrent session close. Kept the in-semaphore placement and adopted the new compatibility check there. SubscriptionTests: both sides added tests at the same place. Kept both. The transfer fixtures needed the identity setup that master's new code reads: the destination session mock had no IdentityToken and its SessionDiagnostics had no ClientDescription, so the compatibility check could not run. Assertions are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 29378426-2a98-41ee-9a6d-050af5b1141f
7 tasks
marcschier
added a commit
that referenced
this pull request
Jul 30, 2026
…espace table on model change (#4115) # Description Two things, both surfaced by the live NodeManager registration work in #4094. > **Rebased on #4114.** Upstream landed #4114 (`cc4667d0b`) while this PR was open; it fixes the > same #4113 root cause. `origin/master` is merged in and the conflicts were resolved **in favour of > the upstream API shape** — `CompleteAsync(IMessageProcessor, uint subscriptionId, ct)` with the > last known server id passed as an argument. The `IMessageProcessor.LastServerId` member this > branch had added is dropped again. What remains here is the orthogonal hardening #4114 does not > cover, plus all of the #4100 work. ## 1. Remaining subscription-retirement hardening (on top of #4114) #4114 fixed the id-collision itself. Three related holes in the same paths are still open: - **Retirement was not atomic with removal.** `RetireSubscriptionId` is now called *under* `m_subscriptionLock` from both `CompleteAsync` and `RemoveSecondaryPartitionAsync`. `GetById` resolves incoming publish responses under the same lock, so a worker could otherwise observe the subscription as removed while its id had not yet reached the retired-id ring — and send a redundant `DeleteSubscriptions` for an already-retired id. `RemoveSecondaryPartitionAsync` did not record the retired id at all. - **The retired-id ring was far too small.** `kMaxSubscriptionHistory` 10 → 256, so subscription churn cannot evict live history. - **The orphan delete was unguarded.** The publish worker no longer sends `DeleteSubscriptions` for an unresolved id while any subscription is still awaiting its id. A publish response can overtake the `CreateSubscription` continuation that assigns the id, and deleting in that window kills a healthy subscription. Leaving a genuine orphan alive until its lifetime expires is the safe trade-off. - **`PublishControllerAsync` lost resize signals.** It re-created its `AsyncAutoResetEvent` waiter on every loop iteration; the abandoned waiter stayed enqueued and swallowed a later `Set()`, so the publish worker pool stopped resizing. This is a second, latent starvation path independent of the id collision. The wait is now created once and only re-armed after it completes. ## 2. Managed client does not refresh the namespace table on model change (#4100) `NodeManagerLifecycle.NotifyCommittedChangeAsync` updates `NamespaceArray`, bumps `UrisVersion`, and then reports a **`BaseModelChangeEvent`** — an event shape that carries no `Changes` payload. The client tracker *did* observe it and cleared the `INodeCache`, but it held only an `IStreamingSubscription`, an `INodeCache` and an `ILogger`, so nothing could re-read the namespace table. A tracking client therefore resolved NodeIds from a namespace added by a live NodeManager registration against a stale `NamespaceUris` table. - **New `INamespaceTableRefresher`** (`NamespaceUris` + `FetchNamespaceTablesAsync`). The member signatures match what `Session` and `ManagedSession` already expose, so both satisfy the interface without any new members. `ISession` is deliberately left untouched — moving its members onto a base interface would be a binary break for 1.5.378 consumers. - **`ModelChangeTracker`** takes the refresher as an optional *trailing* constructor argument, which keeps every existing call site source-compatible and makes `null` the opt-out. A refresh runs when the event carries no per-node detail, when an affected node is `Server_NamespaceArray`, or when an affected namespace index is beyond the client's table. It runs **before** the cache is invalidated and before `ModelChanged` is raised, so subscribers that re-browse from the handler already see the new uris. A failing refresh is logged and swallowed. - **`ModelChangedEventArgs.NamespaceTableRefreshed`** reports the refresh to applications. - **`ManagedSession.EnableModelChangeTrackingAsync`** wires the session in, so opting into tracking is all an application has to do — no new option, no change to the positional `CreateAsync` signature. - **`ModelChange.TypeDefinition`** changes from `NodeId?` to `NodeId`. `NodeId` is a readonly struct implementing `INullable` and must not be wrapped in `System.Nullable<T>`; use `NodeId.Null` and `IsNull` instead. The type is 2.0-preview only (absent from 1.5.378), and the only producer is the tracker itself, which already assigns a non-null `AffectedType`. Per review feedback the type is also now a `readonly record struct`, matching `DataValueChange` / `EventNotification`. ## Tests Every new regression test was run against a temporarily re-introduced defect and **fails** there: | Test | Covers | |---|---| | `CompleteAsyncRetiresOnlyTheCompletedSubscriptionWhenOthersArePendingCreationAsync` | retire-by-identity (also guards #4114's fix) | | `CompleteAsyncRemembersRetiredServerIdAfterIdWasResetAsync` | retired-id history | | `PublishWorkerDefersDeletingUnknownSubscriptionWhileCreationIsPendingAsync` | orphan-delete guard | | `PublishControllerKeepsResizingAfterWorkersExitAsync` | publish-controller signal loss | | `ConcurrentAddAndCompleteRetireOnlyCompletedSubscriptionsAsync` | parallel add/retire registry integrity | | `ConcurrentSubscriptionChurnNeverStarvesASubscription` (integration) | #4113 repro against the reference server | | 7 × `ModelChangeTrackerTests` namespace-refresh cases | each trigger, no-trigger, throwing refresher, `null` refresher, ordering | | `LiveNodeManagerAddRefreshesTheClientNamespaceTable` (integration) | #4100 repro — times out without the wiring | `FakeMessageAckQueue` records both the completed id (upstream's assertions) and the completing instance (the identity assertion in `MessageProcessorTests`), so neither side's coverage was lost in the merge. Results after merging `origin/master`: - `Opc.Ua.Client.Tests` — **2119/2119** pass on net10.0; the new `ModelChange` and `SubscriptionManager` fixtures also pass on **net48**. - `Opc.Ua.Subscriptions.Tests` churn + coverage fixtures — **28/28** (net10.0); churn fixture also passes on net48. - `Opc.Ua.Server.Tests` `NodeManagerLifecycle*` — **49/49** on net10.0; the new fixture also passes on net48. - No new analyzer warnings; only the three pre-existing `CA1873` hits in `NodeCacheResolver.cs` and `ClassicSubscriptionEngine.cs` (untouched files) remain. ## Documentation - `docs/ModelChangeTracking.md` — new **Namespace table refresh** section with the trigger table, updated `ModelChangedEventArgs` / `ModelChange` snippets, updated manual-construction snippet, and a quick-reference row. - `docs/NodeManagerRegistration.md` — client-side half of the **Namespaces** section. No `docs/migrationguide.md` entry: every changed type is either `internal` or 2.0-preview-only. ## Related Issues - Fixes #4100 - Follow-up hardening for #4113 (closed by #4114) - Related to #4094, #3993 ## Checklist - [x] 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. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] 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. - [x] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e22f9b3c-1682-40bd-8b72-31a11352fa2c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds supported live add, reload, and remove lifecycle operations for NodeManager instances on a running server.
The change introduces a DI-backed
INodeManagerLifecycleservice with directStandardServeraccess, generation-aware registration handles, copy-on-write routing, transactional prepare/commit/rollback, runtime NodeSet convenience APIs, append-only namespace updates, model and semantic change notifications, additive-compatible complex type priming, and complete teardown behavior.Lifecycle publication is coordinated with requests, sessions, subscriptions, monitored items, continuation points, and shutdown so retired managers cannot remain reachable or be disposed while still in use. Compatible monitored items transfer to replacement generations without a transient bad status. Removed or incompatible Nodes publish
BadNodeIdUnknownas required by OPC UA Part 4 section 5.8.4.1, retain their MonitoredItem identity, and automatically recover when a compatible Node with the same NodeId returns.Registration is documented end to end in the new docs/NodeManagerRegistration.md, which covers compile-time, startup, and runtime registration together with the guarantees for MonitoredItems, Browse continuation points, namespace indexes, DataTypes, and change notifications.
Review feedback addressed
Interface surface reduced, as requested:
INodeManagerMonitoredItemTrackerfolded intoISubscriptionMonitoredItemLifecycle.INodeManagerMutationCoordinatorand the asynchronous MonitoredItem recovery entry point folded intoIDynamicNodeManagerHost.ISessionClosingRegistryfolded intoIServerInternal, then replaced byISession.IsClosing.ISubscriptionDeletionRegistryfolded intoISubscriptionManager, then removed entirely.INodeManagerContinuationPointTrackerfolded intoISession.ISyncNodeManagerMonitoredItemRecovery, becauseAddPredefinedNodecannot await; it still routes through the asynchronousINodeManagerMonitoredItemLifecyclethatAsyncNodeManagerAdapterexposes.IMonitoredItemLifecyclerenamed toIDetachableMonitoredItem.Reach into the Subscription area reduced to the minimum the feature needs:
SubscriptionManageralmost untouched: two guards that reject a request from a Session that is closing, onCreateSubscriptionand onTransferSubscriptions, because it would create state that is about to be torn down. The deletion claim registry, the session closing gates, the closed-session bookkeeping and the deletion restore paths introduced by earlier revisions are gone. Transferring a subscription away from a session that is closing stays allowed, since that is the recovery path the service exists for; the race that exposed is fixed by serialising source-queue closure with the transfer rather than by rejecting it.QueueSize <= 1, as before this PR. Without a queue the Client is served the last sampled value, so the missing-Node notification simply becomes that value; losing values is expected for a MonitoredItem without queuing.DataValueplus one bool inDataChangeQueueHandler, mirroring the existing overflow pair, which stops it being the value discarded when a configured queue is full.MonitoredItemkeeps its original publish path, its original queue creation and restore paths, its original triggering behaviour, and no per-epoch bookkeeping.IStoredMonitoredItemcarriesIsDeletedandIsDetached, so a restored item reports the state it was stored with rather than one inferred from the last value's StatusCode.Other feedback:
HostedNodeManagerLifecycle,IDynamicNodeManagerHost,NodeManagerRoutingTable, and theRequestManagervalidation and request scopes.ValidateMonitoredItemAsynctoCanAttachMonitoredItemAsyncandRestoreMonitoredItemAsynctoRecoverMonitoredItemAsync, so the lifecycle API no longer collides with the durable SubscriptionRestoreMonitoredItempath used at server startup.CustomNodeManager2dependency from the synchronous MonitoredItem recovery path.DataChangeQueueHandleris back to the two constructors 1.5.378 shipped, each taking an optionalTimeProvider.INodeManagerReloadParticipanttoday.Fixes found during review
AsyncLocalwritten insideasync ValidateRequestAsync, which is invisible to the caller that awaited it, so a lifecycle call from a NodeManager callback waited for its own request and held the lifecycle semaphore forever. The mark is now applied by the service dispatcher, and the drain is bounded so no path can wedge the lifecycle permanently.ValidateRequestAsyncis no longer virtual. An override that rejected a request after callingbaseleft it registered forever; subclasses now useOnRequestValidatedAsync, whose failures complete the request.DeleteMonitoredItemsAsync.Open review discussions
Three threads are open, replied to and awaiting reviewer decisions: separating service-call dispatch from NodeManager handling in
MasterNodeManager(tracked as #4098), the request-scoping shape inStandardServer, and how much of the async MonitoredItem service implementation should stay inSubscription.Issue #4102 records a specification ambiguity found while implementing the missing-Node notification: Part 4 §5.8.4.1 makes it mandatory, but the protected over-capacity queue entry §5.13.1.5 defines applies to
EventQueueOverflowEventTypeonly, so the specification does not say how it survives a full queue.Validation
Opc.Ua.Server.Testson net10.0: 3,768 passed, 5 skipped.DurableDataValueQueueVerifyReferenceBatchingAsync(flakyBatchPersistorfile contention, passes in isolation on net10.0 and net48) andConfigureApplicationBuildsSharedClientAndServerConfigurationAsync(fails on a clean tree with these changes stashed).Related Issues
Checklist
UA.slnxsolution against at least .NET Framework and .NET 10, and all passed.