GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-20 16:52:07 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,540 Commits 108 Branches 105 Tags
aas
Commit Graph

6 Commits

Author SHA1 Message Date
Marc Schier fb80bfdc82 [WotCon] Track WoT Connectivity/Binding 1.1 and add projections (Section 12) (#4182) 
Follow-up to #4128, which merged the WoT Connectivity 1.1 work against
**draft2** of the companion specifications. Both drafts have moved
since, and this branch tracks them through to **draft3**, adds the
projection feature they introduced, and closes the gaps a bidirectional
re-audit found.

The specifications are the private
[`OPCF-Members/spec-drafts`](https://github.com/OPCF-Members/spec-drafts)
drafts of *OPC UA — WoT Connectivity 1.1* and *OPC UA — WoT Binding
1.1*. They are still drafts, which is why this is opened as a draft PR —
see [Status](#status) at the bottom.

## What changed in the specifications

Two revisions landed upstream while #4128 was in flight.

**draft2** replaced the `eventGroup` / `actionGroup` grouping constructs
with a single **projection / view** concept, added fifteen Section 6
vocabulary terms, and made two identifier rules normative. **draft3**
([spec PR #8](https://github.com/OPCF-Members/spec-drafts/pull/8))
un-deprecated the incorporated OPC 10100-1 v1.02 surface, restructured
the conformance clause into three profiles, and tightened two security
obligations.

The draft3 NodeSet is **structurally identical** to draft2 — I diffed
all 286 nodes node-by-node. Every change is metadata: 96
`ReleaseStatus="Deprecated"` attributes removed, `PublicationDate`
advanced, and `WOTC-ProjectionMaterialization` claimed by three Type
Nodes that previously implemented it in name only.

## Projections (WoT Binding Section 12)

The substantial addition. A projection document selects affordances from
other Thing Descriptions or Thing Models and resolves to a document of
its own; materialized, it becomes an OPC UA `View` that **creates no
affordance Node** and only `Organizes` what its sources already
materialized.

| Piece | Where |
|---|---|
| Document model and validation | `Opc.Ua.Types/Wot/WotProjection.cs`,
`WotProjectionModel.cs` |
| Four-stage resolution engine |
`Opc.Ua.Types/Wot/WotProjectionResolver.cs` |
| Materialization plan |
`Opc.Ua.WotCon.Server/Materialization/WotViewProjectionPlan.cs`,
`WotProjectionViewBuilder.cs` |
| Live address-space host | `WotProjectionViewNodeManager.cs`,
`LifecycleWotViewProjectionHost.cs`, `IWotViewProjectionHost.cs` |
| Node lookup seam | `IWotMaterializedNodeIndex.cs` |

Groups reached by `ua:Organizes` become organizational Objects rather
than nested Views, only the outermost materialization is a `View`, and a
member whose source is served by another server is omitted and reported
rather than recreated — the federation case the clause calls out.

### How this is validated

The specification publishes twenty worked examples, two of which are a
**golden pair**: a projection document and the resolved view it is
defined to produce. `WotSpecExampleTests` embeds all twenty and runs the
pair through the resolver, asserting against *the specification's own
expected output* rather than against my reading of the prose. That
single document exercises all three selection forms, the bulk naming
rule, the security closure naming and the provenance term.

This is the strongest evidence in the PR, and it is worth more than the
unit tests around it: it is the same check the upstream
`validate_local.py` performs.

## Other spec work

- **Section 6 vocabulary** — fifteen missing model and platform terms
with their validation rules (`WotNodeSetConverter.ModelVocabulary.cs`).
- **Portable identifiers** — the two rules release 1.1 made normative,
with `WotNodeSetConverterOptions.AllowNonPortableIdentifiers` as a
documented downgrade-to-warning switch for documents authored against
v1.00. Defaults to `false`.
- **xRegistry §6.9 identifier construction** (`XRegistryIdentifier.cs`)
— reverse-authority symbolic identifiers; all seven published worked
examples are test cases.

## Gaps found by re-auditing against draft3

I audited in both directions — every normative requirement to code, and
every `uav:` term in code back to a spec that defines it. Three
findings, all fixed:

1. **`Wot-Con 1.02` accepted anything well-formed.** draft3 makes format
validation part of the unit, because *WoT-Con Minimal* is one unit wide
and materialization projects a *valid* document. The 1.02 upload path
only deserialized into the `ThingDescription` POCO, which happily
accepts JSON that is not a WoT document at all — every member of that
type is optional, so `{}` deserialized and was materialized. It now
parses via `WotDocument.Parse` and requires the document to identify
itself by a non-empty `name` or `title`, returning `Bad_DecodingError`
and materializing nothing otherwise. The rule lives in
`ThingDescriptionFormatValidator` and is shared with
`CreateAssetForEndpoint`, whose auto-generated Thing Descriptions §11
also treats as untrusted input.
2. **Two `WoTFile` operations were ungated.** draft3 §11 names `Write`
and `CloseAndUpdate` alongside the five management methods; both reached
the materializer without an access check. `EnforceManagementAccess` is
now wired into `WotAssetFileManager`.
3. **Two invented `uav:` terms.** `uav:NodeSet2Preservation` was
declared and never used — removed. `uav:eventFields` is read by
`OpcUaBindingPlanner` and defined in *neither* spec, while a comment
claimed it came from OPC 10101. It still works for compatibility but is
now documented as a non-standard extension at both sites rather than
misattributed.

The SSRF prohibition is satisfied by construction: there is no HTTP
client anywhere in these assemblies, and all resolution goes through the
injected `IWotThingResolver`.

## Two ambiguities fixed in the specifications themselves

The audit turned up two places where a conforming implementer had to
invent an answer. Rather than pick silently, I took both upstream — they
are pushed to [spec-drafts PR
#8](https://github.com/OPCF-Members/spec-drafts/pull/8) (all checks
green) and are now normative in *Binding* §12.6 and *Connectivity*
§7.13:

- **`ViewVersion`** had one sentence saying only *that* it changes. It
now changes iff the resolved membership changes, is updated at commit
time alongside the `NodeVersion` stamping, and is a deterministic
function of the membership alone in a canonical order — so two servers
that resolved the same membership agree, which a per-server counter
would not across a redundant pair. Explicitly **not monotonic**: compare
for inequality only.
- **"Type-level view"** was used once in §12.2 and defined nowhere,
while every other statement of materialization was universal. §12.6 now
*defines* it rather than contradicting §12.2: the NodeClass is `View` in
both cases.

Both were already what this code did, so neither changed behaviour — but
the order-insensitivity the clause added was **not** covered by any
test, and `ViewVersionIsUnchangedWhenOnlyTheOrderOfTheMembershipChanges`
now holds it. It is mutation-verified: removing the ordinal sort turns
it red.

`docs/WotBindings.md` no longer lists these as points "the specification
leaves open".

## Validation

| Suite | Result |
|---|---|
| `Opc.Ua.Types.Tests` | 8525 ✅ (8518 on net48) |
| `Opc.Ua.WotCon.Tests` | 1057 ✅ (net10.0 and net48) |
| `Opc.Ua.WotCon.Bindings.Tests` | 559 ✅ |
| `Opc.Ua.XRegistry.Tests` | 196 ✅ |
| `Opc.Ua.Server.Tests` | 4027 ✅ |
| `Opc.Ua.SourceGeneration.Core.Tests` | 3771 ✅ |
| `Opc.Ua.WotCon.Samples.Tests` | 9 ✅ |

The full `UA.slnx` builds with **0 errors and 0 warnings attributable to
this PR** on all 6 TFMs (`TreatWarningsAsErrors` is on).
`Opc.Ua.Server.Tests` and the source-generation suites are listed
because this PR now also touches `MasterNodeManager` and the WoT-Con
NodeSet.

## Compatibility

No public API is removed or broken. Everything added is new surface
using `ArrayOf<T>` / `ByteString` per the repo conventions, wired for DI
with a direct-construction fallback, and the one behavioural tightening
(`Wot-Con 1.02` format validation) rejects input that was never valid to
begin with.

One pre-existing issue is **not** addressed here because it came in with
#4128 and is out of scope: `WotAssetFileManager.OnCloseAndUpdate` uses
`.GetAwaiter().GetResult()`, which the repo rules ban. Worth a
follow-up.

## Landed since this PR was opened

- **DI upgraded to 1.05.0.** `ConnectsTo` was declared a subtype of
`HierarchicalReferences`, contradicting OPC 10000-100 §5.5 Table 48.
That is an upstream error in every official DI NodeSet up to 1.04,
corrected by the OPC Foundation in 1.05.0, so the fixture is replaced
with the verbatim 1.05.0 file rather than hand-patched. A guard test
pins the corrected form. This also closed a second gap: six DI types the
WoT model was missing are 1.05 additions.
- **The sample Pump has the type definition OPC 40223 declares for it.**
`Pump1.Identification` carried DI's `FunctionalGroupType` instead of
`PumpIdentificationType`. Every test, the sample documents and the
README agreed with each other about it, which is why it survived.
- **`WotPumpAddressSpaceComparisonTests`** compares this server against
`PumpDeviceIntegrationServer`, which builds the same OPC 40223 Pump from
generated NodeSets and hand-written wiring. An independent oracle rather
than a restatement of the same assumptions - it is what found the defect
above.
- **Error containment in `MaterializeProjectionViewsAsync`.** An
exception mid-loop discarded the applied View handles before they were
published, leaving View Nodes in the address space that nothing could
remove.
- **`HasWoTProjection`** is now created; it was documented and plumbed
but never wired.
- **The default in-memory View host** no longer deletes the View it has
just applied.
- **An undeclared NodeSet alias now says so.** It previously surfaced as
`IdentifierMissing`, naming neither the value nor the fact that an alias
was expected.
- **`ViewVersion` follows the algorithm the specification now states.**
My earlier spec text was rejected on review, correctly: it named neither
the function nor the canonical order it demanded, ignored that OPC
10000-3 §5.4 requires a value greater than zero, and its "when, and only
when" is unsatisfiable for a 32-bit value. §12.6 now specifies the
algorithm outright and this implementation follows it exactly, pinned by
an independently computed test vector. Implementing it surfaced a
further defect in the clause - joining members on U+000A is not
injective, because a NodeId string identifier may itself contain U+000A
- which is fixed in the specification by length-prefixing each member.
- **An auto-generated Thing Description is treated as untrusted input.**
§11 requires it; `CreateAssetForEndpoint` validated the endpoint but
handed whatever the discovery provider returned straight to the
materializer. The `Wot-Con 1.02` format rule now gates both paths from
one place.
- **Document bytes moved out of the registry snapshot** into a
content-addressed store, so a commit no longer rewrites the whole corpus
to edit one document. Writes stage and are promoted during the commit,
which preserves both durability-first ordering and the fail-closed rule
that a blob directory without a manifest means operator recovery.
Restoring streaming digest verification along the way closed a real gap:
content integrity had degenerated to a length comparison.
- **External references now reach a NodeManager registered later.** A
reference to a Node that does not exist yet is dropped rather than
queued, and the dynamic-registration path had no second phase to apply
them in, so registering a NodeSet before the one it points into lost the
forward edge permanently. Review of that change then found a state leak
in its rollback path, fixed with a red-to-green regression test.
- **`WOTC-Legacy` is renamed `Wot-Con 1.02`** in the specification and
here. Editorial: the NodeSet diff is `<Category>` values only.

## Status

Open for review. Two things may still move:

1. **The specifications are unpublished drafts.** Anything here can
change if the working group decides differently. The `ViewVersion`
definition has since been settled - §12.6 specifies the algorithm and
this follows it - but the same could happen to any other clause tracked
here.
2. **`uav:eventFields`** is kept as a documented non-standard extension.
That is a judgement call the WG may want to reverse, either by adopting
the term or removing it.

Two known items are recorded rather than fixed:

- `Pump1` has no hierarchical parent, so it is reachable by NodeId but
not by browsing down from `Objects`. The cross-NodeSet ordering defect
this was originally attributed to is fixed above, and cross-manager
references are now symmetric in both registration orders. The Pump
itself is still blocked on a different cause found while verifying that
fix: with the reference added, `DeviceSet` exists but the Pump has
**zero** inverse references, so the edge is dropped at *import* - before
any external-reference machinery runs. That is a separate defect in the
materialization path, not the one that was fixed, and it is left
recorded rather than guessed at.
- `WotAssetFileManager.OnCloseAndUpdate` uses
`.GetAwaiter().GetResult()`, which the repo rules ban. Pre-existing from
#4128 and out of scope here.

Review of the projection resolver and the materialization host is the
most valuable thing to look at; the vocabulary and identifier commits
are mechanical by comparison.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
 2026-08-08 09:08:23 +02:00
Marc Schier e73e71184d [WotCon] Add OPC UA WoT Connectivity 1.1 with the Types, source generation, Server and xRegistry work it builds on (#4128) 
## Summary

Adds OPC UA WoT Connectivity 1.1 to the stack, together with the Types,
source generation, Server and xRegistry work it is built on. This branch
is the merge point for the fourteen-PR stack that replaced integration
PR #4093; every constituent PR was reviewed and approved separately and
is listed below.

414 files changed, +108,936 / -3,199. Roughly 46k added lines of product
code, 57k of tests, plus samples, tools and documentation.

## Types

- Complete `ExtensionObject` raw-body decoding for binary, XML and JSON
bodies by resolving the concrete type through the message context's
encodeable factory, and simplify the `Variant` structure helpers by
delegating to it (#4128).
- Add `IAtomicFileReplace` as an optional `IFileSystem` capability so
existing external implementations keep working, with atomic publish for
`LocalFileSystem` via `File.Replace`/`File.Move` and for
`VirtualFileSystem` by re-keying the in-memory entry (#4128).
- Add lossless conversion between WoT documents and NodeSet2
(`Opc.Ua.Wot.WotNodeSetConverter`). A byte-exact `uav:nodeSet` envelope
is preserved when requested, the structured `uav:nodes` projection is
used when the readable vocabulary is incomplete, and NodeSet2 is
otherwise synthesized from readable WoT terms. Unmapped JSON members
survive a round trip as pointer-addressed residue (#4131).

## Source generation

- Improve the NodeSet to ModelDesign conversion and the generated node
state (#4132).
- Generate OPC UA models directly from WoT Thing Description files
(#4134).

## Server

- Add live NodeManager shadow and immediate reload. A reload
materializes a new generation beside the active one and switches
atomically; the superseded generation is retired gracefully so existing
MonitoredItems keep being served until they drain, or immediately when
the caller asks for it. Includes the request admission and drain
machinery that lets an orderly shutdown wait for admitted requests
instead of tearing down underneath them (#4147).
- Make subscription transfer between sessions transactional, so a failed
transfer leaves neither session holding a partially moved subscription
(#4150).

## xRegistry

- Allow clients to use explicit registry roots rather than assuming a
well-known location (#4130).

## WoT Connectivity

- Add the WoT Connectivity 1.1 information model: a registry-first
revision layered on the abstract xRegistry base model, incorporating the
complete published OPC 10100-1 v1.02 surface as deprecated nodes in the
same namespace so existing 1.02 clients keep working (#4139).
- Add the protocol binding abstractions and planners that compile WoT
forms into executable plans (#4140).
- Add the HTTP, Modbus, OPC UA and MQTT binding executors (#4144).
- Add the registry and its stores, including document versioning,
validation, dependency resolution and refresh (#4146).
- Add the registry client (#4142).
- Materialize WoT Thing Descriptions into the server address space,
deriving types from Thing Models and instances from Thing Descriptions
(#4154).
- Restore the WoT asset registry bridge so the deprecated 1.02 asset
surface is backed by the registry (#4171).
- Add the WoT Connectivity samples and complete the documentation
(#4156), and restore the remaining coverage and docs (#4172).

## New projects

`src/Opc.Ua.WotCon.Bindings`, `src/Opc.Ua.WotCon.Bindings.Mqtt`,
`tests/Opc.Ua.WotCon.Bindings.Tests`,
`tests/Opc.Ua.WotCon.Samples.Tests`, and the `samples/WotCon`
AggregationServer, AggregationClient and FlatTagServer samples.

## Constituent pull requests

Merged into this branch top-down, each independently approved:

| PR | Title |
| --- | --- |
| #4128 | [Types] Complete ExtensionObject decoding and add an atomic
file replace capability |
| #4131 | [Types] Add lossless conversion between WoT documents and
NodeSet2 |
| #4132 | [SourceGeneration] Improve NodeSet to ModelDesign conversion
and generated node state |
| #4134 | [SourceGeneration] Generate OPC UA models from WoT Thing
Description files |
| #4147 | [Server] Add live NodeManager shadow and immediate reload |
| #4150 | [Server] Make subscription transfer between sessions
transactional |
| #4130 | [XRegistry] Allow clients to use explicit registry roots |
| #4139 | [WotCon] Add the WoT Connectivity 1.1 information model |
| #4140 | [WotCon] Add the WoT protocol binding abstractions and
planners |
| #4144 | [WotCon] Add the HTTP, Modbus, OPC UA and MQTT binding
executors |
| #4146 | [WotCon] Add the WoT Connectivity registry and its stores |
| #4142 | [WotCon] Add the WoT Connectivity registry client |
| #4154 | [WotCon] Materialize WoT Thing Descriptions into the server
address space |
| #4156 | [WotCon] Add the WoT Connectivity samples and complete the
documentation |
| #4171 | Restore WoT asset registry bridge |
| #4172 | Restore WoT coverage and docs |

Supersedes #4093.

## Follow-up

The WoT Connectivity and WoT Binding drafts have moved to 1.1-draft2
since this work was authored. Aligning with that revision - the removed
group vocabulary, the new projection/View construct, the remaining model
vocabulary terms and the two new portable-identity validation rules - is
tracked separately and will follow in its own pull request.

## Validation

Built for every target framework with zero warnings and zero errors, and
validated at the tip of the stack with `Opc.Ua.WotCon.Tests` at 999
passed / 0 failed and `Opc.Ua.Server.Tests` at 4024 passed / 0 failed.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
 2026-08-05 10:14:15 +02:00
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 8a10139b8c Fix OPC UA address-space compliance in the device integration stack (#4117) 
# Description

A live OPC UA compliance audit of `samples/PumpDeviceIntegrationServer`
against the **OPC 40223 Pumps** and **OPC 10000-100 (DI)** companion
specifications found **13 address-space violations** plus a
serialization bug in the MCP tool. Every fix lands at its **root cause
in the shared stack** rather than as a workaround in the sample, so all
servers benefit.

The sample now exposes **N fully simulated pumps** (`--pumps N`) and
advertises the DI conformance facets it actually satisfies.

> This branch has been merged with `master` including **#4116** ("Add
simulation to PumpDeviceIntegrationServer and Event modelling"), which
independently reworked the same pump simulation and the same fluent
alarm builders. See [Relationship to #4116](#relationship-to-4116) below
for how the two were reconciled.

## Findings and fixes

| # | Finding | Root cause | Fix |
|---|---------|-----------|-----|
| 1 | `OverTempAlarm` created by the fluent API did not exist in the
address space | `AttachAlarm` attached the alarm with `AddChild` but
never indexed it into the node manager | Register the node; the same gap
existed in `AddObject`, `CreateInstance` and the state-machine creators
|
| 2 | Alarms were unsubscribable — `EventNotifier = 0` everywhere, no
`HasNotifier` from `Server` | No event-source wiring at all | Set
`HasCondition`, initialise
`SourceNode`/`SourceName`/`ConditionName`/`InputNode`, promote
`EventNotifier` up the ancestor chain, publish `HasNotifier` from the
`Server` object |
| 3 | Every instance NodeId was minted in the **DI standard namespace**
(`ns=4;s=5001_Pump #1`) | `DiNodeManager.New` inherited the parent's
namespace index | New `DiNodeManager.InstanceNamespaceIndex`; instances
move to the server's application namespace |
| 4 | `Server/Namespaces` described only 2 of 6 namespaces | The
NodeSet2 importer drops nodes parented to namespace 0, so companion-spec
metadata objects never reached the address space — and no route covered
the server's own namespace | New `NamespaceMetadataPublisher` walks the
`NamespaceArray` and fills version/publication date from
`ModelDependencyAttribute` |
| 5 | Vendor BrowseNames (`Diagnostics`, `LastError`, …) sat in the DI
namespace | Builders defaulted to the parent's namespace | Default to
the server namespace; **spec-defined BrowseNames are unchanged** |
| 6 | `AccessLevel="5"` from the Pumps NodeSet was reported as `1` | The
ModelDesign `AccessLevel` enum is not `[Flags]` and cannot represent
`CurrentRead \| HistoryRead` | Carry the verbatim NodeSet2 bitmask and
emit named `AccessLevels` constants |
| 7 | `Historizing = true` while `HistoryRead` returned
`BadHistoryOperationUnsupported` | Nothing reconciled the declared
history surface with the wired historians | Startup clears the
advertisement (including masking the attribute read callbacks) when no
provider resolves; the sample wires a real historian |
| 8 | Mandatory `ProductInstanceUri` unset | Sample omission | Populated
on every pump |
| 9 | Mandatory `TrueState`/`FalseState` empty on every supervision
boolean | Absent in the official NodeSet; a server must still populate
them | Populated on every pump |
| 10 | Machinery `Machines` folder empty (OPC 40001-1 §9.2 says
**shall**) | `TryAddToMachinesFolder` used `AddChild`, which
*re-parents* the device away from `DeviceSet` | Add the `Organizes`
reference instead |
| 11 | BrowseName contained a space and `#` | Sample naming | `Pump_1`
with the readable label in `DisplayName` |
| 12 | `Maintenance` group materialised but empty | Sample omission |
Populated |
| 13 | Uninitialised values reported `Good` | No initial-value status |
`BadWaitingForInitialData` until the first simulation tick |
| — | `ServerProfileArray` advertised only `StandardUA2017` |
`DiNodeManager` did not implement the existing `IConformanceContributor`
| Contribute DI 1.05 server facets computed at runtime from what is
actually wired, merged with `StandardUA2017` |
| — | MCP tool serialised boolean `false` and numeric `0` as JSON `null`
| `Variant.Null` is `default`, so `Variant.Equals` adopts the non-null
type and compares against the default value | Dispatch on `BuiltInType`
and use typed `TryGetValue` accessors instead of the prohibited
`AsBoxedObject` |

## Relationship to #4116

#4116 landed on `master` while this branch was in flight and targeted
overlapping ground. The merge is a **union of both intents**, not a
fast-forward:

- **`AlarmBuilderExtensions`** — kept both sides. #4116 contributes the
parent-must-be-an-Object guard, `SetEnableState`, and the
`HasEventSource` references; this branch contributes the `HasCondition`
reference, condition source initialisation, **registration of the alarm
with the node manager so it is browsable**, notifier promotion up the
whole ancestor chain, and root-notifier registration.
- **`SupervisionBuilderExtensions`** — took #4116's `SetAlarmActive`. It
is more spec-correct than this branch's version: it honours
`EnabledState`, resets Acked/Confirmed on activation, and computes
`Retain` per OPC 10000-9 so an unacknowledged Condition stays retained
after going inactive. This branch's null guard was kept so a malformed
alarm cannot dereference a missing `EnabledState`.
- **`PumpNodeManager.Configure.cs`** — took #4116's **push-based**
simulation (`RegisterPumpSimulation`, `CreatePumpSimulation`,
`IValueUpdater<T>`, `Initialize`/`Advance`/`Publish`, deterministic
per-pump phase offsets) as the structure, then folded in this branch's
compliance work: configurable pump count, Maintenance group,
`TrueState`/`FalseState`, historian wiring, per-pump mandatory
Identification values, and `BadWaitingForInitialData` until the first
tick.
- **README** — union, keeping #4116's address-space validation workflow
section.

Two test adjustments were needed, both because the merge legitimately
changed behaviour the tests pinned:

- `PumpHostedReferenceTests` now resolves pump NodeIds from the browse
result instead of hard-coding the DI namespace, and waits for the first
published value. `PumpCreatedAfterStartupJoinsTheLiveSimulationAsync`
was **added** to restore #4116's coverage that a pump created after
startup through `ConfigureDevicesFor` joins the live simulation.
- `FluentAlarmRegistrationIntegrationTests` no longer asserts that
`Retain` clears when an alarm goes inactive — that pinned this branch's
weaker semantics, and #4116's Part 9 behaviour is correct.

## Breaking change

Runtime-created DI device instance NodeIds move from the companion-spec
namespace to the server's application namespace, e.g.
`ns=<DI>;s=5001_Pump #1` becomes `ns=<application>;s=5001_Pump #1`. This
invalidates NodeIds cached by clients from earlier 2.0 previews. It is
**not** a model change — standard DI/Machinery/Pumps type NodeIds and
spec-defined BrowseNames are untouched.

Device integration hosting is new in 2.0, so there is nothing to migrate
from 1.5.378 and this is deliberately *not* in `docs/MigrationGuide.md`.
Anyone tracking 2.0 previews should rediscover devices by browsing
`DeviceSet` rather than persisting NodeIds, and resolve namespace
indexes from `NamespaceArray` per connection.

## Verification

Compliance is pinned by regression tests that assert **through the
server's service surface** (`BrowseAsync` / `ReadAsync` /
`HistoryReadAsync`), not by inspecting `NodeState` objects in memory.
That distinction mattered: three defects survived in-memory assertions
and were only caught through the read path — most subtly, `Historizing`
was cleared while `AccessLevel` still advertised `HistoryRead`, because
attribute read callbacks re-applied the NodeSet bits.

Post-merge on net10.0:

| Suite | Result |
|---|---|
| `Opc.Ua.Di.Tests` | 326 passed |
| `Opc.Ua.Server.Tests` | 3846 passed, 5 skipped |
| `Opc.Ua.Tools.Tests` | 288 passed |
| `Opc.Ua.History.Tests` | 506 passed, 27 skipped |
| `Opc.Ua.SourceGeneration.Core.Tests` | 3722 passed, 8 skipped |

The full solution was also built and tested on **net48** before the
merge.

Review feedback from `copilot-pull-request-reviewer` has been addressed
in commit `badad31dc`: the shared `ServerObjectState` lock was removed
(the forward `HasNotifier` edge is published through the owning node
manager instead), MCP array serialization now preserves element types,
and the pump option guards report the specific rejected option. The only
failure is
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, a
pre-existing certificate-store issue that **passes when run in
isolation** and is unrelated to this change.

## Documentation

Updated `docs/DeviceIntegration.md`, `docs/MigrationGuide.md`,
`docs/HistoricalAccess.md`, `docs/DeveloperGuide.md`,
`docs/McpServer.md`, `docs/README.md`,
`samples/PumpDeviceIntegrationServer/README.md` and
`tools/Opc.Ua.Mcp/README.md`, and consolidated the four node-manager
documents into a single [`docs/NodeManagers.md`](docs/NodeManagers.md) —
overview, built-in managers (including `MasterNodeManager`), core vs
custom, registration, server address-space metadata, and source
generation — with a TOC and all inbound links repointed.

## Related Issues

No tracking issue exists for this work yet — it originated from an
ad-hoc compliance audit of the pump sample. Given the size of the change
and the runtime-breaking NodeId move, please open (or link) a tracking
issue so the design is recorded as an ADR before merging.

- Fixes #

## 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](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: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
 2026-07-30 16:04:18 +02:00
Marc Schier 275c3800b5 [Server] Add live NodeManager lifecycle support (#4094) 
# Description

Adds supported live add, reload, and remove lifecycle operations for
NodeManager instances on a running server.

The change introduces a DI-backed `INodeManagerLifecycle` service with
direct `StandardServer` access, 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
`BadNodeIdUnknown` as 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](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:

- `INodeManagerMonitoredItemTracker` folded into
`ISubscriptionMonitoredItemLifecycle`.
- `INodeManagerMutationCoordinator` and the asynchronous MonitoredItem
recovery entry point folded into `IDynamicNodeManagerHost`.
- `ISessionClosingRegistry` folded into `IServerInternal`, then replaced
by `ISession.IsClosing`.
- `ISubscriptionDeletionRegistry` folded into `ISubscriptionManager`,
then removed entirely.
- `INodeManagerContinuationPointTracker` folded into `ISession`.
- The synchronous recovery entry point kept on its own
`ISyncNodeManagerMonitoredItemRecovery`, because `AddPredefinedNode`
cannot await; it still routes through the asynchronous
`INodeManagerMonitoredItemLifecycle` that `AsyncNodeManagerAdapter`
exposes.
- `IMonitoredItemLifecycle` renamed to `IDetachableMonitoredItem`.

Reach into the Subscription area reduced to the minimum the feature
needs:

- The lifecycle feature itself leaves `SubscriptionManager` almost
untouched: two guards that reject a request from a Session that is
closing, on `CreateSubscription` and on `TransferSubscriptions`, 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.
- Queueing is disabled again for a MonitoredItem with `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.
- What remains of the notification is one `DataValue` plus one bool in
`DataChangeQueueHandler`, mirroring the existing overflow pair, which
stops it being the value discarded when a configured queue is full.
- `MonitoredItem` keeps its original publish path, its original queue
creation and restore paths, its original triggering behaviour, and no
per-epoch bookkeeping.
- `IStoredMonitoredItem` carries `IsDeleted` and `IsDetached`, so a
restored item reports the state it was stored with rather than one
inferred from the last value's StatusCode.

Other feedback:

- Documented `HostedNodeManagerLifecycle`, `IDynamicNodeManagerHost`,
`NodeManagerRoutingTable`, and the `RequestManager` validation and
request scopes.
- Renamed `ValidateMonitoredItemAsync` to `CanAttachMonitoredItemAsync`
and `RestoreMonitoredItemAsync` to `RecoverMonitoredItemAsync`, so the
lifecycle API no longer collides with the durable Subscription
`RestoreMonitoredItem` path used at server startup.
- Removed the `CustomNodeManager2` dependency from the synchronous
MonitoredItem recovery path.
- `DataChangeQueueHandler` is back to the two constructors 1.5.378
shipped, each taking an optional `TimeProvider`.
- Session closing is a one-way latch, and the Session is removed even
when teardown throws.
- Documented what a custom NodeManager has to implement to take part in
live lifecycle operations, and which NodeManager implements
`INodeManagerReloadParticipant` today.

## Fixes found during review

- The guard that stops a NodeManager lifecycle operation being started
from inside a service handler never fired. It was backed by an
`AsyncLocal` written inside `async 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.
- `ValidateRequestAsync` is no longer virtual. An override that rejected
a request after calling `base` left it registered forever; subclasses
now use `OnRequestValidatedAsync`, whose failures complete the request.
- A MonitoredItem deleted and disposed while a reload was handing it to
the replacement NodeManager could be attached anyway. The item now
reserves itself for the hand-over and the transition takes it back if it
was disposed meanwhile.
- The Overflow bit is recorded and applied when the value is published,
as the surrounding design does, rather than written into the value at
enqueue time.
- MonitoredItem cleanup for a Subscription being deleted no longer
passes null entries to `DeleteMonitoredItemsAsync`.
- Browse continuation points of a retired NodeManager are persisted and
disposed outside the Session lock.

## 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 in
`StandardServer`, and how much of the async MonitoredItem service
implementation should stay in `Subscription`.

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
`EventQueueOverflowEventType` only, so the specification does not say
how it survives a full queue.

## Validation

- Full `Opc.Ua.Server.Tests` on net10.0: 3,768 passed, 5 skipped.
- Lifecycle, MonitoredItem, Subscription and continuation point suites:
185 passed on net10.0 and 75 on net48.
- Server builds complete with zero warnings or errors.
- Two failures are pre-existing and unrelated:
`DurableDataValueQueueVerifyReferenceBatchingAsync` (flaky
`BatchPersistor` file contention, passes in isolation on net10.0 and
net48) and
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`
(fails on a clean tree with these changes stashed).

## Related Issues

- Fixes #3993
- Follow-up tracked in #4098
 2026-07-28 16:06:28 +02:00
Marc Schier 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00