2 Commits

Author	SHA1	Message	Date
Marc Schier	168b3de353	Replace the RequestManager AsyncLocal guard with an explicit operation context (#4163) ... # Description `RequestManager` tracked whether the calling flow was serving a Client request in a `private readonly AsyncLocal<bool> m_inServiceDispatch`. It was set by a `StandardServer.ProcessRequestAsync` override and read by `NodeManagerLifecycle`, so that a lifecycle operation (`AddAsync` / `ReloadAsync` / `RemoveAsync`) started from inside a request fails fast with `InvalidOperationException` instead of draining — and therefore waiting for — its own request. Two problems with that mechanism, raised in review of #4093: 1. An `AsyncLocal<T>` written inside an `async` method never flows back to the caller that awaited it, so every scope had to chain to the previous value by hand and restore it on dispose. Subtle, and easy to get wrong when a new entry point is added. 2. Ambient state is invisible at the call site, so the lifetime of the mark could not be reasoned about from the code that depended on it — especially once work is handed to a background task. `RequestManager` also had no business carrying ambient state on behalf of a different subsystem; its job is tracking requests for the drain. ## What changed The guard is now an **exact identity lookup** against the request registry `RequestManager` already maintains, and the operation is threaded **explicitly**: * **`RequestManager`** — `internal bool IsExecutingRequest(IOperationContext?)` performs a reference-identity lookup against `m_requests` under the existing `m_requestsLock`. `m_inServiceDispatch`, `EnterServiceDispatchScope()`, the nested `ServiceDispatchScope` class and the flag-based `IsExecutingRequest` property are gone. * **`StandardServer`** — the `ProcessRequestAsync` override is removed; it existed only to open the dispatch scope. * **`INodeManagerLifecycle`** — every member (`AddAsync` ×2 factory kinds, `ReloadAsync` ×2, `RemoveAsync`) now takes the caller's `IOperationContext?` directly, ahead of `CancellationToken ct = default`. No overloads and no parallel signature are kept: the interface is new in 2.0 and outside the 1.5.378 compatibility rule, so call sites state their context explicitly or pass `null`. * **`HostedNodeManagerLifecycle`** and **`RuntimeNodeSetLifecycleExtensions`** thread the argument through; `AddRuntimeNodeSetAsync` / `ReloadRuntimeNodeSetAsync` remain single methods. * **`SystemContextOperationExtensions.GetOperationContext(this ISystemContext)`** (new, `Opc.Ua.Core`) lets a NodeManager or Method callback hand over the operation it already received without downcasting. It switches on `SessionSystemContext` and `SystemContext`, the two independent roots every context derives from. No new interface is introduced: casting the context to `IOperationContext` would not do, because a context implements that by delegating to the operation it was created for, so the cast yields the context rather than the operation. * **`NodeManagerLifecycle.EnsureNotRequestCallback`** no longer consults `ServerState`. The previous `CurrentState == Running` predicate could let a genuinely re-entrant call slip past while the server was shutting down; the request registry answers exactly in every state. A NodeManager or Method callback now reads: ```csharp await m_lifecycle.ReloadAsync(m_registration, replacement, context.GetOperationContext(), ct); ``` and a control-plane caller (hosted service, DI consumer) passes `null`. ## Behaviour Unchanged for the case the guard exists to catch: a lifecycle call made on behalf of an executing request is still rejected up front with the same `InvalidOperationException` and the same message. Two deliberate differences: * **Improvement** — an internal `OperationContext` that was never enrolled as a Client request is now correctly allowed through. The boolean flag could not distinguish it from a real request. * **Accepted trade-off** — a caller inside a request that passes no operation is no longer detected at entry and falls back to the pre-existing bounded `RequestManager.RequestDrainTimeout`, which already covered any request that bypassed the service pipeline. Automatic detection for an uncooperative caller inherently requires ambient state, which is exactly what this change removes. Documented in `docs/NodeManagers.md`. ## Alternatives considered and rejected | Alternative | Why rejected | | --- | --- | | Delete the guard, rely only on `RequestDrainTimeout` | Turns a clear immediate error into a multi-minute hang. | | Infer the caller from the `CancellationToken` it passes | Works only for the exact request token, breaks on linked tokens; ambient magic of a different kind. | | In-flight callback counter on `MasterNodeManager` | False positives: rejects legitimate control-plane calls whenever any request is concurrently dispatching. | | Defer the drain to a background task so re-entrancy cannot deadlock | The drains are woven into transactional commit / rollback and binding reconciliation; deferring them breaks the transactional guarantees. | | `Activity.Current` / `ThreadLocal` | The same ambient magic the issue asks to remove. | ## Testing * `RequestManagerTests` — the three `EnterServiceDispatchScope` tests are replaced by coverage for `IsExecutingRequest(context)`: null, never-registered, only-while-scope-open, visible across an `await` and a background `Task.Run`, and two concurrently executing requests not confusing each other. * `NodeManagerLifecycleTests` — rejection from an executing request for Add / Reload / Remove without invoking the factory, acceptance for a context that is not an executing request, acceptance once the request has completed, and the full callback shape (`ServerSystemContext.Copy(operation)` → `GetOperationContext()` → rejected). * `HostedNodeManagerLifecycleTests` — forwarding tests for the caller-context argument on all five members. * `tests/Opc.Ua.Core.Tests/Stack/State/SystemContextOperationExtensionsTests.cs` (new) — `GetOperationContext` across both context roots and the null-argument guard. ## Related Issues - Fixes #4149 ## 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. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 14402d11-e540-4faa-a5cd-94881bd7ce85	2026-08-03 20:31:51 +00:00
Marc Schier	636912bd4c	Harden subscription retirement after #4114 and refresh the client namespace 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	2026-07-30 12:17:00 +02:00