[Server] Add live NodeManager shadow and immediate reload - #4147
[Server] Add live NodeManager shadow and immediate reload#4147marcschier wants to merge 12 commits into
Conversation
Extends the NodeManager lifecycle already in master with ShadowReloadAsync, which stages a replacement generation and routes new service requests to it while existing monitored items continue against the retired generation until subscriptions drain. ImmediateReloadAsync is the alternative policy for callers that cannot wait for a graceful drain. It detaches the affected non-durable monitored items through the existing detach path, so they report BadNodeIdUnknown, and does not park them for later recovery, since the generation they belonged to is gone. The existing fail-closed ReloadAsync and RemoveAsync behaviour is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
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 shadow-reload and immediate-reload support to the NodeManager lifecycle, including request-drain coordination so existing monitored items can continue to be serviced by a retired generation while new requests route to a replacement generation.
Changes:
- Introduces
ShadowReloadAsync/ImmediateReloadAsynclifecycle paths and runtime NodeSet extension helpers. - Adds request-lifecycle coordination + shutdown sequencing updates to drain/admit requests safely during lifecycle operations.
- Expands test coverage for runtime NodeSet configuration hooks, shadow reload behavior, and monitored-item transfer rollback.
Reviewed changes
Copilot reviewed 35 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Opc.Ua.Server.Tests/SubscriptionTests.cs | Updates mocks to match new monitored-item transfer signature. |
| tests/Opc.Ua.Server.Tests/RuntimeNodeSet/RuntimeNodeSetLifecycleTests.cs | Adds an end-to-end shadow reload test covering routing vs. existing monitored items. |
| tests/Opc.Ua.Server.Tests/RuntimeNodeSet/RuntimeNodeSetLifecycleExtensionsTests.cs | New unit tests for runtime NodeSet lifecycle extension method argument contracts and wiring. |
| tests/Opc.Ua.Server.Tests/RuntimeNodeSet/RuntimeNodeSetConfigureAsyncTests.cs | New tests for RuntimeNodeSetOptions.ConfigureAsync ordering + generation-owner disposal semantics. |
| tests/Opc.Ua.Server.Tests/Opc.Ua.Server.Tests.csproj | Removes WotCon server project reference. |
| tests/Opc.Ua.Server.Tests/NodeManager/*Tests.cs | Updates/extends adapter delegation + rollback coverage for monitored-item transfers. |
| tests/Opc.Ua.Server.Tests/Hosting/HostedNodeManagerLifecycleTests.cs | Adds ShadowReloadAsync delegation/guard tests. |
| tests/Opc.Ua.Server.Tests/Hosting/CombinedHostingTests.cs | Removes WotCon hosting test. |
| src/Opc.Ua.Server/Subscription/SubscriptionManager.cs | Adds internal test hooks around condition refresh worker shutdown/reset. |
| src/Opc.Ua.Server/Server/StandardServer.cs | Refactors shutdown/disposal orchestration, adds deferred shutdown observer + new lifecycle registration. |
| src/Opc.Ua.Server/Server/RequestManager.cs | Adds lifecycle admission/drain coordination and waiter exclusion support. |
| src/Opc.Ua.Server/Server/RequestManagerLifecycleExtension.cs | New request lifecycle coordination helper for shutdown/lifecycle waits. |
| src/Opc.Ua.Server/RuntimeNodeSet/* | Adds ConfigureAsync, lifecycle-from-request option, and shadow/immediate reload extension methods. |
| src/Opc.Ua.Server/NodeManager/MasterNodeManager.cs | Adds monitored-item transfer transaction/rollback and ownership-sensitive dispatch to support shadow-retired generations. |
| src/Opc.Ua.Server/NodeManager/Lifecycle/* | Adds routing table Contains, lifecycle interface surface for shadow/immediate reload, and a committed-reload exception type. |
| src/Opc.Ua.Server/NodeManager/NodeManager.cs | Adds transfer options + rollback hooks to sync/async node managers and adapters. |
| samples/Quickstarts.Servers/SampleNodeManager/SampleNodeManager.cs | Updates sample node manager transfer handling to honor new transfer options and rollback hook. |
Suppressed comments (1)
tests/Opc.Ua.Server.Tests/Opc.Ua.Server.Tests.csproj:1
- This PR removes the WotCon server project reference (and a related hosting test elsewhere), but the PR description focuses on NodeManager shadow/immediate reload and runtime NodeSet helpers. Either update the PR description to explicitly call out the WotCon test/reference removal, or move that change into a separate PR to keep scope aligned.
…nager NodeStateLookupExtensions.FindByDataType has no call site in this change; its consumers are the fluent NodeManager builder and the generated node manager templates, so it moves to the source generation PR that introduces them. Adds the RequestManager admission and drain tests and the StandardServer shutdown coverage that belong with the shutdown work already in this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
|
Pushed commit 0382d0a with the two requested corrections:
Validation: net10.0 server build passed, net10.0 server tests passed (3927 passed / 9 skipped). net48 server tests built and full-suite run had only the known |
Shutdown completion tracking used a linear scan over a list for every node manager, making a shutdown pass quadratic in the number of managers. It now uses a set keyed by reference identity, reusing the existing RefEqualityComparer. Grouping monitored items by owning node manager did the same linear search per item. An index keyed by reference identity makes it linear overall while the returned list keeps its first-encounter ordering, which callers depend on. Also fills in the empty exception documentation on the runtime node set factory helpers and corrects the copyright year on RequestManagerLifecycleExtension. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
|
Flagging a genuine regression on this branch that I have not yet fixed, with the evidence so far. Symptom
It is not a flake, and not pre-existing
Three reproductions, and the master baseline is clean - so this is caused by this PR, not by the It is the code, not the added testsThis PR adds only two test files ( Running only Why local validation missed itThe test was added to master by 636912b (#4115, "refresh the client namespace table on model This generalises to the whole stack: PR CI builds Where I would look nextThis PR modifies Not resolving this comment; it is a real blocker for merging this PR. |
|
Correcting and sharpening my earlier analysis of the What I got wrong. I said "it is the code, not the added tests". That inference was based on What has since been established. I re-ran the full suite with Consolidated evidence:
So: some fixture that runs earlier in the full suite leaves state this branch is sensitive to and What is ruled out. The client side is not involved. #4115, which added both this test and the Still open. I have not identified the specific poisoning predecessor. Bisecting further needs a Leaving this unresolved - it remains a blocker for this PR, and the characterisation above should |
|
Note on the second CI failure, so it is not confused with the namespace-refresh regression.
That is a test-host crash, not an assertion failure - no test actually reported a result. This PR returns nothing. So it should clear on the next run. The only substantive failure on this PR remains |
|
Bisection update on I split
So it is neither parallel contention nor one poisoning predecessor - it only manifests under the Worth stressing what this does not change: Two concrete suggestions for whoever picks this up:
|
|
New decisive finding on It is not slowness — the event never arrives. I raised only the inner wait from 60s to 105s, staying inside the existing The captured log narrows it further. Within the test's window: So the model-change subscription is created successfully with its monitored item, and then Two incidental observations from the same log, both worth a look independently of this bug:
Combined with the earlier bisection - no subset of the suite reproduces it, it still fails with Still open; not resolving. |
StartTrackingAsync used to return after queueing the pump task, before StreamingSubscription had created the event monitored item on the server. A server-side model change raised in that window is not replayed, so the lifecycle namespace refresh test could lose the GeneralModelChangeEvent under CI load. Plumb an internal readiness hook through StreamingSubscription and have ModelChangeTracker wait until the event monitored item reports Created, surfacing terminal creation errors instead of weakening the test with sleeps or longer timeouts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
… into marcschier/wot-05-lifecycle
StandardServer.Dispose() must keep existing synchronous callers working, but it cannot block on the asynchronous shutdown task without violating the repository's sync-over-async ban and risking deadlocks while requests are draining. Add IAsyncDisposable so callers that need deterministic release can await DisposeAsync(). The async path joins the existing server-internal shutdown machinery, waits for base-resource disposal to finish even when synchronous Dispose races with it, and preserves the deferred observer used by Dispose(). Add disposal lifecycle tests for started, never-started, idempotent, mixed synchronous/asynchronous, and deferred-shutdown paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
| /// transfer implementations that have not been updated to consume | ||
| /// <see cref="MonitoredItemTransferOptions"/>. | ||
| /// </summary> | ||
| public static class MonitoredItemTransferExecution |
There was a problem hiding this comment.
i dont like that this introduces ambient state, just fail the transfer for legacy or make it work wihtout a scope
There was a problem hiding this comment.
Agreed that ambient state is a smell, and I would rather remove it than defend it.
Before I change the behaviour I want to confirm which of your two suggestions you mean, because they are not equivalent:
- "just fail the transfer for legacy" — a behaviour change: legacy
INodeManagerimplementations that do not flow the scope would get a failedTransferSubscriptionsinstead of a working one. Clean, but it breaks existing third-party node managers that work today. - "make it work without a scope" — keep the behaviour and thread the state explicitly through the call chain instead of ambiently. No breakage, but it touches the
INodeManagertransfer signatures.
I lean towards the second (explicit parameter, no AsyncLocal) since it removes the ambient state without breaking existing implementers, but it does change a public interface so I would like your call before doing it. Leaving open.
There was a problem hiding this comment.
Adding the costing we already did on this, so the choice can be made on numbers rather than preference. There are three ways to get rid of the ambient scope and they are very different in cost and blast radius:
1. Full removal — flow the context explicitly everywhere. Roughly 1-2 weeks. It breaks INodeManagerLifecycle for third-party implementers, and the main risk is a missed context hand-off silently reintroducing a wait-on-self in the request drain — which is the failure mode the scope exists to prevent.
2. Additive explicit-context overloads. Roughly 2-4 days. New overloads take the context as a parameter, the old ones keep working, and the ambient scope stops being the only path. Non-breaking, and it is the option that actually answers your objection.
3. A single ambient token instead of the current scope. Roughly 0.5-1 day, but it still leaves ambient state, so I do not think it satisfies what you are asking for.
Your other suggestion — "just fail the transfer for legacy" — is cheapest of all, but it is a behaviour change: existing third-party INodeManager implementations that work today would start getting failed TransferSubscriptions. Worth doing only if we are happy to declare those unsupported.
My recommendation is option 2: it removes the ambient-state smell you are objecting to, keeps third parties working, and is days rather than weeks. If you agree I will do that; if you would rather have the full removal in option 1, that is a bigger piece of work and I would want it tracked separately from this PR rather than expanding a 45-file change further.
There was a problem hiding this comment.
My suggestion: Option 1. @romanett agreed?
| public sealed class RuntimeNodeSetNodeManagerFactory : IAsyncNodeManagerFactory | ||
| public sealed class RuntimeNodeSetNodeManagerFactory : | ||
| IAsyncNodeManagerFactory, | ||
| IRequestCallbackSafeNodeManagerFactory |
There was a problem hiding this comment.
i dont like that this new interface is introduced, just leave the property on the type
There was a problem hiding this comment.
Happy to drop the interface — I want to check where you want the property to live, because the two options differ in compatibility.
IRequestCallbackSafeNodeManagerFactory is internal and used in exactly one place, as a capability probe: NodeManagerLifecycle.cs:1804 does factory is IRequestCallbackSafeNodeManagerFactory. A test factory also implements it to opt in.
- Property on the concrete
RuntimeNodeSetNodeManagerFactory— thenNodeManagerLifecyclehas to type-check the concrete class, which couples the lifecycle to one factory implementation and stops any other factory (including third-party ones) from opting in. - Property on
IAsyncNodeManagerFactory— cleaner to read and no marker interface, but it is a breaking change for every existing implementer unless it gets a default implementation.
If you mean the second and are happy for it to be a breaking addition to IAsyncNodeManagerFactory (or defaulted), say so and I will make that change. Leaving open until then.
| CompleteServerDispose(disposeRequest); | ||
| } | ||
|
|
||
| private ServerDisposeRequest BeginServerDispose() |
There was a problem hiding this comment.
i also think this overcomplicates stuff, just do the blocking Dispose
There was a problem hiding this comment.
Flagging a direct conflict rather than changing this unilaterally, because two maintainers are asking for opposite things on the same code.
@cristipogacean reviewed this same Dispose() and asked for the opposite:
If shutdown must be async, expose
IAsyncDisposable/StopAsyncand makeDisposeeither block deterministically or document loudly. As-is, "disposed" no longer means "released".
That is what 1b61a0b9c implemented — StandardServer now implements IAsyncDisposable, DisposeAsync() awaits shutdown and base-resource disposal deterministically, and Dispose() still initiates shutdown but documents that release completes asynchronously. @cristipogacean approved the PR after that change landed.
Blocking Dispose() is also not something I can just switch to:
- The repository''s own contributor rules state "DO NOT create SYNC over ASYNC (
GetAwaiter().GetResult(),Wait(),Result) unless explicitly requested/confirmed." MakingDispose()block requires exactly that on the shutdown task. - The shutdown waits for admitted requests to drain. Blocking a caller thread on it risks deadlock when
Dispose()is called from a thread that the drain depends on — for example from a request-handling or callback context.
So this needs a decision between the two of you rather than me picking a side:
- Keep the current shape —
DisposeAsync()for deterministic release,Dispose()documented as initiate-only. - Blocking
Dispose()— I can do it, but it needs an explicit sign-off on the sync-over-async exception, and I would want to address the deadlock path. - Both — keep
DisposeAsync()and haveDispose()block on it; still needs the same exception.
Leaving this thread open pending that call. The same question applies to your comment on #4150 (ServerInternalData.cs:156), which asks for .GetAwaiter().GetResult() explicitly.
There was a problem hiding this comment.
Replied on #4150. Follow my direction on the feedback thread there.
| } | ||
| } | ||
|
|
||
| private async Task JoinActiveServerInternalShutdownFromRequest( |
There was a problem hiding this comment.
this shutdown is way to complicated
There was a problem hiding this comment.
Taking this together with your Dispose() comment, since the complexity here is mostly the cost of the deterministic-shutdown contract @cristipogacean asked for.
Most of the machinery exists to guarantee three things at once: request admission closes, already-admitted requests drain before resources are torn down, and disposal happens exactly once no matter which of Dispose() / DisposeAsync() / StopAsync arrives first or concurrently. Dropping any one of those simplifies the code substantially.
If the team is willing to relax the drain guarantee — that is, accept that a shutdown may tear down while requests are still in flight — the shutdown collapses to something much smaller. That is a product decision rather than a refactor, so I would rather have it made explicitly than quietly simplify and change shutdown semantics.
Tell me which guarantee you are willing to give up and I will simplify accordingly. Leaving open.
There was a problem hiding this comment.
No, drain guarantees before shutdown need to remain. Investigate how to make the code more comprehensible and easy to maintain.
| /// <summary> | ||
| /// Optional request lifecycle coordination used by server shutdown and NodeManager lifecycle | ||
| /// operations. | ||
| /// </summary> |
There was a problem hiding this comment.
this adds so much stuff no one understands and probably does not need, needs to be reworked
There was a problem hiding this comment.
Fair challenge, and I would rather cut this than leave something no one can follow.
RequestManagerLifecycleExtension exists to hold the request-admission and drain state that the orderly shutdown depends on — closing admission, tracking admitted requests, and signalling when the last one has drained so teardown can proceed. It is the mechanism behind the "disposed means released" guarantee @cristipogacean asked for on Dispose().
So its fate is tied to the shutdown-complexity thread above: if the drain guarantee stays, something has to own this state and I would rather improve its naming, documentation and test coverage than delete it; if the team accepts teardown while requests are in flight, most of it can go.
Rather than rework it blind, tell me which way the drain guarantee goes and I will either simplify it away or make it comprehensible — concretely: a short design note in docs/, clearer names, and tests that show the admission/drain transitions. Leaving open.
There was a problem hiding this comment.
Make it more comprehensible, but leave as is
Explain the three live reload modes and their client-visible tradeoffs. Clarify when monitored-item transfer rollback is used, and enable a deterministic rollback test that exercises a non-no-op owner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
The type summary described a hand-over between the current and replacement NodeManagers during a lifecycle transition, but the interface prepares a monitored item move from a source session to a destination session for TransferSubscriptions. Describe what it actually coordinates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
Summary
Adds PR 5 of the stack splitting #4093 into independently reviewable pieces, targeting
masterdirectly.This PR extends the NodeManager lifecycle with live shadow reload support and RuntimeNodeSet lifecycle helpers.
ShadowReloadAsyncstages a replacement generation and routes new service requests to it while monitored items already owned by the retired generation continue there until subscriptions drain.ImmediateReloadAsynckeeps the immediate policy but implements it on the existing detach machinery at the maintainer's request: affected non-durable monitored items are detached/marked deleted withIDetachableMonitoredItem, queueBadNodeIdUnknown, and are not migrated to the replacement generation.The prior NodeManager generation is disposed before
ImmediateReloadAsyncreturns in the covered lifecycle path. The bespoke permanent retirement-error path is intentionally removed, so follow-up operations use the existing detached-item semantics rather than a separate retirement state.Validation
dotnet build src\Opc.Ua.Server\Opc.Ua.Server.csproj -c Release -f net10.0 -v:mdotnet build tests\Opc.Ua.Server.Tests\Opc.Ua.Server.Tests.csproj -c Release -p:CustomTestTarget=net10.0 -v:mdotnet test tests\Opc.Ua.Server.Tests\Opc.Ua.Server.Tests.csproj -c Release -p:CustomTestTarget=net10.0 --no-build— 3914 passed, 9 skipped, 0 faileddotnet build tests\Opc.Ua.Server.Tests\Opc.Ua.Server.Tests.csproj -c Release -p:CustomTestTarget=net48 -v:mdotnet test tests\Opc.Ua.Server.Tests\Opc.Ua.Server.Tests.csproj -c Release -p:CustomTestTarget=net48 --no-build— full run hit local durable queue file-lock flake; the failed test passed in isolation. Full run otherwise reported 3913 passed, 9 skipped, 1 failed.