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

5 Commits

Author SHA1 Message Date
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 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
Marc Schier 15e795bbed Fix #4061: invalidate component cache on runtime node add/remove (#4068) 
## Summary

Fixes #4061.

`AsyncCustomNodeManager` kept its component cache (`m_componentCache`,
populated when monitored items / registered nodes resolve through the
manager) out of sync with runtime address-space changes:

- `RemovePredefinedNodeAsync` (the delete path) never evicted the
removed node, so a deleted node stayed resolvable through the cache.
- A re-registered / replaced `NodeId` kept pointing at the stale
instance.

For a dynamic-composition consumer (recompose = delete + re-add) this
surfaced as a Browse/Read/Call served a stale, pre-change view — the
runtime-added child intermittently missing while the pre-add set (or a
removed node) lingered.

## Fix

- `RemovePredefinedNodeAsync` force-evicts the removed node from the
component cache.
- `IndexPredefinedNode` / `ReplacePredefinedNode` refresh the cached
instance on re-registration (reference count preserved).
- `CreateNodeAsync` / `AddNodeAsync` / `DeleteNodeAsync` (both
convenience and service overloads) refresh the parent's cached view
after a runtime child add/remove — covering both the runtime and
predefined-node registration paths.
- `NodeState.AddChild` now sets the `Children` change mask inside the
children lock so the mask is published together with the mutation.

The component-cache helpers are additive and NativeAOT-safe. The runtime
async paths use async waits — `RefreshParentComponentCacheAsync` /
`InvalidateComponentCacheAsync` (`await
m_componentCacheSemaphore.WaitAsync(cancellationToken)`) — while the
genuinely synchronous registration/upgrade paths (`IndexPredefinedNode`
via `AddPredefinedNodeSynchronously`, `ReplacePredefinedNode`) keep a
synchronous `RefreshComponentCache` that matches the existing
synchronous cache helpers on the type. All are `private`.

## Tests

- Manager-level regression tests (`AsyncCustomNodeManagerTests.cs`):
delete-eviction, replace-refresh, and
browse-of-a-runtime-added-child-from-an-already-cached-parent (the
reporter's explicitly requested framework coverage).
- End-to-end test (`MasterNodeManagerNodeManagementTests.cs`): registers
a real `AsyncCustomNodeManager` in a `MasterNodeManager` over the
`StandardServer` fixture, caches a parent, runtime-adds a child, and
browses through the full master dispatch (Forward /
HierarchicalReferences / IncludeSubtypes), asserting the child is
visible and then gone after delete.

## Validation

- `net10.0` and `net48`: full `AsyncCustomNodeManager` suite +
`MasterNodeManagerNodeManagement` suite green, no regressions.

## Docs

- Browse-consistency note added to `docs/ModelChangeTracking.md`.
 2026-07-22 11:57:07 +02:00
Marc Schier 8538f531e7 Update paths for the consolidated repository layout 2026-07-16 06:26:05 +02:00
Marc Schier 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00