GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-29 07:07:24 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,642 Commits 77 Branches 105 Tags
copilot/fix-arrayof-toarrayof-issue
Commit Graph
11 Commits
Author SHA1 Message Date
Cristi PogaceanandCopilot Autofix powered by AI 9b8a3b1188 Preserve node initialization and copied child ownership (#4544) 
# Description

Preserve populated derived node children through optional initialization
and give copied children independent ownership by their copied parent.

- Track initialization from a source in `NodeState` and guard generated
optional-default recreation without removing the optional initialization
hook.
- Reuse `CloneChild` for dynamic children, generated typed children, and
method argument properties. Preserve the clone root's existing external
parent.
- Add cross-model initialization/copy regressions, method-argument and
nested-child ownership tests, and inherited alarm-subtree checks.

This PR contains only the six files for this fix group.
Certificate-store, runtime-type-loader, publish-queue and
transport-exception fixes are separate work.

## Related Issues

Fixes #4525.
Fixes #4526.

## Validation

Fresh focused Release/net10.0 checks on the final base:

| Fixture | Passed | Failed | Skipped |
| --- | ---: | ---: | ---: |
| CrossModelInstanceTypeDefinitionGeneratorTests | 9 | 0 | 0 |
| Types NodeStateTests | 227 | 0 | 0 |
| Core StateTypesTests | 13 | 0 | 0 |

All three test commands exited 0. `git diff --check` and the
changed-file diagnostic check are clean.

Regression proof was also executed in the isolated worktree: with the
three production changes reversed, six generator cases failed with the
missing derived Temperature child while three controls passed; two copy
cases failed parent-identity assertions while the
external-parent/absent-arguments control passed. The production fix was
restored and the focused checks are green. The retained red runs
preceded the later upstream synchronization; the affected production
paths were unchanged by that synchronization.

Windows win-x64 NativeAOT publish and native execution also passed: main
178/178, Historian 2/2, MCP 3/3, no skips, every invocation exit 0.
These native runs preceded the final CTT-configuration/notes-only
fast-forward; this PR's six files were unchanged.

Local validation is intentionally change-scoped for this work, with the
full matrix and repository-wide checks left to PR CI. Repository
guidance and test assertions/timeouts were not modified. Local commands
used a NuGet v2 source override and disabled audit only on the command
line; CI feed/audit policy is unchanged.

Earlier broad attempts are not claimed as a completed local gate: an
unmodified-base durable restart timeout was reproduced before
synchronization (the later durable run passed), and the net48 runner
stalled after recording passing client tests. A direct client retry
subsequently exited 0, but the full net48 matrix was not completed.
Those results are not substituted for the focused evidence above.

## Checklist

- [x] I have signed the CLA and read the CONTRIBUTING doc.
- [x] I have added regression tests and demonstrated failure without the
production fix.
- [x] The focused tests pass; no new compiler diagnostics were reported
by those builds.
- [ ] Coverage and the full net48/net10.0 matrix are verified by PR CI.
- [ ] All CI, CodeQL, and review feedback is addressed.

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
 2026-09-25 12:23:49 +00:00
Marc SchierandCopilot d08d4faec1 Reduce NodeState allocations and pack scalar Variants (#4452) 
# Description

Consolidate the complete B01/B02 NodeState memory work into one PR.

- Replace eager cold-path NodeState locks with lazy canonical
publication and update
  the event-monitor counter atomically.
- Store six design-metadata properties and three security properties in
separate lazy
private bags while preserving public APIs, null/empty distinctions,
masks, copying
  and existing serialization behavior.
- Avoid allocating the ReportEvent scheduling closure when the
async-dispatch path is
  unused.
- Retain the logger factory's category logger directly, avoiding a
redundant typed
  wrapper on telemetry initialization.
- Keep Variant at 24 bytes while removing eligible outer boxes for
QualifiedName,
  NodeId, NET8+ ByteString and text-only LocalizedText values.
- Add reproducible allocation/layout/occupancy fixtures, real registered
and monitored
populations, reference contracts, generated-consumer coverage and
NativeAOT cases.

## Measured allocation changes

Matched .NET 10.0.11 x64 observations against the integrated master
baseline:

| Construction/assignment | Master | This PR |
|---|---:|---:|
| Bare/minimal object | 824 B | 640 B |
| Bare/minimal variable | 1160 B | 976 B |
| Variable with both optional groups populated or reset | 1160 B | 1088
B |
| Variable with a shared ByteString value | 1192 B | 976 B |

Separate paired measurements show:

- telemetry initialization: 24 B typed-logger wrapper removed;
- eligible QualifiedName, NodeId and ByteString Variant construction: 32
B to 0 B;
- eligible text-only LocalizedText Variant construction: 40 B to 0 B;
- variable attributes read into caller-provided Variant destinations:
136 B to 0 B.

These are runtime-specific per-operation observations, not additive
whole-server
savings. Optional bags retain their documented populated/high-water
premiums and are
not reclaimed on reset.

## Reference-storage decision

A complete private indexed reference backend and a sorted-traversal
alternative were
measured, but both changed the existing filtered hash-index enumeration
order. They
were rejected despite lower memory use. The production reference backend
remains
unchanged; strengthened ordering, identity, serialization and
concurrency contracts
are retained. Adaptive small-set storage remains deferred.

## Consolidation

This PR now contains all changes from #4451 and the former narrower
#4452 branch.
Every #4452 changed path was present in #4451; after merging, the #4451
head
`836e45226` is a direct ancestor of this branch. #4451 is closed as
superseded.
Draft #4425 remains unchanged and is also superseded by this
consolidated work.

## Review feedback

- Removed the standalone Variant storage document and its README entry.
- Made the raw NodeId constructor private; the documented internal
`GetRawState`/`SetRawState` bridge is explicitly Variant-only and must
never become
  public.
- Marked the two 32-thread counter tests non-parallel.
- Added 10-second timeouts to the concurrent browse benchmark's main and
worker
  barriers and completion waits.
- Migrated the registered-population fixture to the current async
`SealAsync` API.

All review threads on #4451 and #4452 were replied to with the action
and fix commit,
then resolved.

## Related issues

- Related to #4275.
- Supersedes #4425 and #4451.

## Validation

- Consolidated Types/Core/Server selection: **1241 passed** on net10.0.
- Consolidated Types/Core/Server selection: **1238 passed** on net48.
- Focused #4451 feedback selection: **189 passed** on net10.0 and **187
passed**
  on net48.
- Earlier integrated NativeAOT validation passed **163/163** cases
across the main,
MCP and Historian applications; current PR CI provides the authoritative
final
  merged-source native validation.

The first #4452 CI run was blocked by merge conflicts. Seven later jobs
completed
their tests but encountered a shared artifact-service HTTP 403 while
uploading
results; no workflow or test was weakened to mask that infrastructure
failure.

Generated `report.*.json` crash artifacts remain untracked and are not
part of this PR.

## Checklist

- [ ] 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 the changes and improve coverage.
- [x] I have added the necessary current-behavior documentation.
- [ ] I have verified that the fresh consolidated CI run introduces no
build or analyzer warnings.
- [ ] I ran the complete UA.slnx suite locally on net48 and net10.0 with
no failures.
- [ ] I fixed all failing and flaky CI tests and all CodeQL warnings.
- [x] I have addressed all PR feedback received.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: d2569e2e-7115-4969-bb44-8159e302b450
 2026-09-09 16:48:13 +02:00
Marc SchierandCopilot da7412c429 Add native xRegistry events and distinct Resource versions (#4396) 
## Summary

- support the xRegistry 0.6.0 distinct Resource/Version hierarchy:
  `Registry -> Group -> Resource -> Versions -> Version`
- instantiate stable logical Resource nodes separately from exact
Version nodes
  while preserving existing Version NodeIds
- delegate logical Resource FileType operations and Version-view
metadata to the
selected default Version with handle pinning and shared writer
reservations
- make Resource and Version delete behavior role-stable and use
`MetaEpoch` /
  `Epoch` respectively
- use stable Resource SourceNodes, exact Version SourceNodes and the
  `Server -> Registry -> Group -> Resource -> Version` notifier chain
- keep a model-metadata-gated client fallback for 0.5 flat Servers; 0.6
Servers
  expose no legacy aliases
- target `HasWoTProjection` at the real stable logical Resource node
- use `AsyncCustomNodeManager` for the registration, fast-path, and
federation managers,
  with awaited lifecycle, node mutations, event delivery, and cleanup
- handle startup rollback, reentrant deletion callbacks, undisclosed
file handles,
  and retryable cleanup without blocking unrelated file operations

## Authoritative dependencies

- companion specification: OPCF-Members/spec-drafts#32 at
  `d3399fca025025a73e5a6b599f850a81d985c060`
- OPC UA binding: xregistry/spec#568
- xRegistry event semantics: xregistry/spec#563, merged as
  `66ee31e47fbadff6ab982ae1516f7d9065e46b64`
- companion NodeSet SHA-256:
  `369f5f3f1814b333e4eeb52bfb4b6a730cdff372505a5f91a54c94f0016ac4cf`

## Review feedback

- keep the WoT Connectivity information-model designation at `1.1`;
draft revisions update the publication date without changing the model
version
- eliminate all 198 unique CA1861 diagnostics across 54 files and remove
seven obsolete project suppressions
- fix the analyzer-clean touched-path CA1307 and the hierarchy follow-up
CA1846/CA1851 diagnostics without dropping legacy-TFM compatibility

- recover the reconcile queue after callback faults/cancellation and
continue accepted pending changes
- remove migration guidance for the xRegistry managers, which are new in
2.0

## Compatibility

- public WoT Connectivity model designation remains `1.1`
- generated event-record ABI, constructors, options, NodeIds, and stored
documents remain compatible
- the generic xRegistry managers are new in 2.0 and implement
`IAsyncNodeManager`;
use `IAsyncNodeManagerFactory` and the async lifecycle hooks (see
`docs\XRegistry.md`)
- persistence schema and document blobs remain compatible unless noted
below

## Validation

- merge validation at `980422f76` with upstream `e8bec27d5`: 454
targeted WoT/materialization/model/registry cases and all 327 XRegistry
cases passed on both net10.0 and net48; all 29 net10 aggregation sample
cases passed
- feedback regressions at `308e8ba74`: 55 targeted queue and live
registry event/projection/lifecycle cases passed on each of net10.0 and
net48
- prior CI: all 266 checks passed at `6489a0908`; the unchanged Ubuntu
Core certificate-counter test and dependent jobs passed on the one-time
retry ([run 34117304035, attempt
2](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/34117304035/attempts/2))
- `Opc.Ua.XRegistry.Tests` — 327 passed on net10.0 and net48 for
migration commit `32c914aaf`
- `Opc.Ua.WotCon.Tests` net10.0 — 1348 passed
- `Opc.Ua.WotCon.Bindings.Tests` net10.0 — 683 passed
- `Opc.Ua.SourceGeneration.Core.Tests` net10.0 — 3807 passed, 8 skipped
- `Opc.Ua.SourceGeneration.Tests` net10.0 — 145 passed
- `Opc.Ua.SourceGeneration.Stack.Tests` net10.0 — 94 passed
- affected XRegistry and WoT client/server projects build successfully
- xRegistry NodeSet is byte-identical to the companion artifact
- repository-wide CA1861 inventory — 198 unique diagnostics across 54
files reduced to zero
- targeted changed-fixture validation — 1,098 passed, 3 skipped, 0
failed
- constrained full `UA.slnx` net10.0 Release build — CA1861 = 0
- `Opc.Ua.XRegistry.Client` net472 and netstandard2.1 builds — 0
warnings, 0 errors
- independent shared-array safety review — no significant issues
- `git diff --check`

The final review regressions cover concurrent reconcile/open, synthetic
logical
handle collisions across default switches, cross-session Close
isolation,
shared writer reservations, logical FileType property/change
propagation,
logical Labels delegation, role-fixed deletion, event sources,
model-version
gating, and stable `HasWoTProjection` targets.

The companion specification and protocol binding are reviewed in the
linked PRs above.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5c79a1ed-6dab-4522-a9fc-6548caa78ad5
Copilot-Session: b605797f-a97a-4cd0-8f12-016d33c2fd88
Copilot-Session: 7ef369a5-2307-4f71-8a11-15af0e72f240
Copilot-Session: e716d455-8397-4d22-9c5a-398847ab7317
 2026-09-08 04:46:40 +00:00
romanettandClaude Opus 5 3543d7cf6c Report Executable/UserExecutable for state machine causes (#4368) 
# Description

A Part 16 cause is a Method, and whether it may fire depends on the
state the machine is in. Part 3 defines `Executable` / `UserExecutable`
as exactly that question, and Part 4 has `Call` refuse a non-executable
Method with `Bad_NotExecutable`. `FiniteStateMachineState` already knows
the answer — `IsCausePermitted(context, causeId, checkUserAccessRights)`
— but nothing outside alarm shelving connected the two. Across the
library, `OnReadExecutable` was assigned in exactly three places, all in
`AlarmConditionState`. Everywhere else a cause method kept the `true`
its node was constructed with, so a client could not tell which causes
currently apply and found out only by calling one and being refused with
`Bad_NotSupported` from `DoCause`.

## What changed

**`StateMachineBuilder.WithCause`** now answers both attributes from
`IsCausePermitted` alongside installing the call handler — on the
unified builder (via the dispatcher's `InstallCauseMethod`) and on the
`Opc.Ua.Server.Fluent` surface reached through
`INodeBuilder.AsStateMachine()`, which had the identical gap.
`Executable` ignores user rights; `UserExecutable` also runs the
machine's `OnCheckUserPermission` callback.

Per the issue's note about servers that deliberately keep methods
executable and answer at call time, both overloads take
`reportExecutable: false` to opt out.

**`ProgramStateMachineState`** wires its own `Start` / `Suspend` /
`Resume` / `Halt` / `Reset` in `OnAfterCreate`, so a Program server no
longer repeats the boilerplate the class already shipped handlers for.
The Boiler quickstart's copy of it is deleted rather than left as an
example.

**Generator-emitted `*StateMachineState` subclasses** are left alone.
The generator emits no Part 16 `StateTable` / `TransitionTable` /
`CauseMappings` overrides — the same gap that makes the DI
software-update dispatcher write `CurrentState` directly instead of
going through `SetState` (see `docs/SoftwareUpdate.md`) — so there is
nothing to derive executability from. A vendor machine from a NodeSet
gets its causes through `StateMachineBuilder.For(...).WithCause(...)`,
which the first change covers. Worth revisiting if the generator gains
table emission.

## Notes for reviewers

Two details behind the `ProgramStateMachineState` change:

- The five methods are `OptionalPlaceholder` on
`ProgramStateMachineType`, so the generated class has no typed
properties for them. They are matched on browse name **without** the
namespace index, because a subtype that redeclares them puts them in its
own namespace — the Boiler model uses `1:Start`, not `0:Start`.
- `NodeState.Create` rebuilds the child list from the type template, so
a method added by hand before it is gone by the time `OnAfterCreate`
runs. `WireCauseMethods(ISystemContext)` is public for that case.

**Behaviour change worth flagging:** a `WithCause` against a machine
whose `CauseMappings` lack that cause now reports the method
non-executable where it previously reported executable. Such a call was
already failing with `Bad_NotSupported` inside `DoCause`, so it fails
earlier with the code Part 4 specifies rather than newly failing — but
it is visible to clients that read the attribute.

## Related Issues

- Fixes #4363

## 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.
- [ ] 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.

### Tests

New: `StateMachineBuilderCauseExecutableTests` (definition mode, the
opt-out, `OnCheckUserPermission` affecting `UserExecutable` only,
suspended machines) and `ProgramStateMachineStateTests`, plus two cases
added to the existing fluent-builder fixture. All read through
`MethodState.ReadAttribute`, so a handler that is installed but never
consulted fails the test.

Run locally on **net10.0**: `Opc.Ua.Server.Tests` (4797),
`Opc.Ua.Core.Tests` (4374), `Opc.Ua.Di.Tests`, `Opc.Ua.Features.Tests`,
`Opc.Ua.Robotics.Intent.Tests` and the Boiler AOT tests all pass;
`UA.slnx` builds with no new warnings. The .NET Framework legs were not
run locally — leaving that checklist box to CI.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
 2026-09-01 18:31:36 +02:00
romanettandCopilot App 8d2d363d04 Add MultiState Discrete write validation (#4255) 
## Summary

Adds hand-written partial state classes that enforce OPC UA Part 8
§5.3.3 write validation for the `DiscreteItemType` subtypes, plus tests.

## Details

- **`MultiStateDiscreteState`** — a written value is treated as an index
into `EnumStrings`. Returns `BadOutOfRange` when the index is outside
`[0, len)`.
- **`MultiStateValueDiscreteState`** — a written value is matched
against the sparse `EnumValues` set. Returns `BadOutOfRange` when no
entry matches. On a good write, `ValueAsText` is refreshed from the
matching entry's display name.
- **`DiscreteItemState`** — provides the shared `TryGetIntegerValue`
helper that normalizes any scalar integer built-in type to `long` (the
exact-type `Variant` accessors return `default` on a built-in-type
mismatch, so a `BuiltInType` switch is required).
- **`TwoStateDiscreteType`** intentionally has no hand-written class.

These 3 classes are `public partial class` extending the
source-generated DataAccess partials already present on `master`. They
read via the typed properties (`EnumStrings`, `EnumValues`,
`ValueAsText`) rather than `FindChild`, because
`BaseDataVariableState.FindChild` special-cases the `EnumStrings`
BrowseName and never falls through to the `AddChild`'d children.

## Testing

- `dotnet build src\Opc.Ua.Core.Types\Opc.Ua.Core.Types.csproj -c
Release -f net10.0` → 0 warnings, 0 errors.
- 13 tests across `MultiStateDiscreteStateTests` and
`MultiStateValueDiscreteStateTests` pass (13 passed, 0 failed).

---------

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
 2026-08-14 08:24:45 -07:00
romanettandCopilot App aef86ba9dc Add SelectionListType write validation (#4254) 
## Summary

Adds write-validation for the standard OPC UA `SelectionListType`
VariableType via the handwritten `SelectionListState` partial, plus unit
tests. Split out of #4220 into its own PR against `master`.

## The feature

`SelectionListState` overrides `WriteValueAttribute` to enforce **OPC UA
Part 5 section 7.18**: when the optional `RestrictToList` property is
present and `true`, a written value must be one of the entries in the
mandatory `Selections` property.

Behavior contract:
- Validation **gates on `RestrictToList`** — only enforced when the
property is present and true. Otherwise the base-class behavior applies
unchanged.
- **Empty `Selections` rejects all** values (nothing is a valid member).
- Membership is **data-type independent** — determined via `Variant`
equality rather than any specific built-in type.
- **Mismatched built-in types defer to the base class** for its normal
type validation.

The generated `SelectionListState` partial (base
`BaseDataVariableState`, typed
`Selections`/`SelectionDescriptions`/`RestrictToList` properties) is
produced by the source generator from the core address space; this
handwritten partial supplies only the write-validation override, using a
single clean generic helper.

## Validation

- Build `Opc.Ua.Core.Types` (net10.0, Release): **0 warnings, 0
errors**.
- `SelectionListStateTests` (net10.0, Release): **14 passed, 0 failed**.

---------

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
 2026-08-14 08:24:34 -07:00
Marc SchierandCopilot 1cca744b5c Retire the last exposed locks: node, browser, server diagnostics and application properties (#4222) 
Closes #4201. Closes #4202. Closes #4203. Closes #4215. Closes #4216.

(#4215 and #4216 are @marcschier's originals from the #4188 review;
#4202 and #4201 are my later duplicates of the same two findings. All
four describe the two changes in section 1 and are closed together.)

**This PR now carries what was #4221 as well.** The two were a stack;
they have been unstacked and combined here, and #4221 is closed.
Everything below the "Read the server diagnostics" section is the work
that was previously reviewed as #4221.

---

## 1. The browse path guards itself, and the last node locks are gone

`AsyncCustomNodeManager` kept two `lock (source)` sites around
`CreateBrowser`, carrying a NOTE that browser creation was not
internally synchronized. `NodeBrowser` still handed its lock to derived
types through `protected Lock DataLock`.

`NodeState.CreateBrowser` now holds a browse lock across
`PopulateBrowser` and `OnPopulateBrowser`, so browser construction on a
node is serialized — which is what the external `lock (source)` was
actually providing, and what a handler that mutates the node during
population (the historian's lazy config installer) depends on. Both
`lock (source)` statements and the NOTE are gone.

**What that lock does not do**, stated plainly in the XML docs and in
`docs/NodeManagers.md` rather than over-promised: it is not an atomic
snapshot across the node's children, notifiers and references. Writers
take those collections' own locks, not the browse lock. The browser is a
point-in-time copy and each collection is read consistently, but the
combination is not a transaction.

No `lock` takes a `NodeState` as its sync root any more. That was the
last thing keeping `NodeState` from being documentably self-guarding, so
the other eleven went with it: `lock (activeNode)`, `lock (target)`,
`lock (source.Node)`, `lock (target.Node)`, `lock (notifier)`, `lock
(handle.Node)` in `AsyncCustomNodeManager`, and `lock (node)` in
`CustomNodeManager2`. Most were a check-then-act pair now served by
`NodeState.AddReferenceIfMissing`, which does the check and the insert
under the node's own lock.

`NodeBrowser.DataLock` is removed. A browser is **single-consumer**: it
belongs to whoever created it, and both server browse paths already
serialize on the owning side. The exposed lock only added an
inheritance-level locking contract that a derived browser could not
reason about.

## 2. Read the server diagnostics under the lock every writer takes

`OnUpdateDiagnostics` locked the diagnostics **payload** rather than the
lock every writer takes through `UpdateServerDiagnostics`, so the
snapshot a client reads from the `ServerDiagnostics` node was never
excluded from a concurrent writer.

Both halves matter, and the second is the one that is easy to miss:

```csharp
lock (m_diagnosticsLock)                                        // was: lock (ServerDiagnostics)
{
    value = Variant.FromStructure(ServerDiagnostics, copy: true);  // was: copy defaulted to false
}
```

`copy: true` is what makes it a snapshot. `FromStructure(value, copy:
false)` does `new ExtensionObject(value, copy)` — it wraps the **live**
structure without copying it, so the caller would read the fields long
after the lock was released and see them change underneath. Swapping the
lock alone would have looked like a fix while still excluding nothing.

## 3. Analyzer numbering

The browser rule is **UA0027**, not UA0025. This branch had
independently taken UA0025 for `NodeBrowser.DataLock` while #4188
published UA0025 for `ILocalNode.DataLock` and UA0026 for
`BaseVariableValue.Lock`. Master's are shipped, so they keep the
numbers:

| | UA0025 | UA0026 | UA0027 |
|---|---|---|---|
| master (#4188) | `ILocalNode.DataLock` | `BaseVariableValue.Lock` | —
|
| **this PR** | `ILocalNode.DataLock` | `BaseVariableValue.Lock` |
**`NodeBrowser.DataLock`** |

Both sides had written a different `UA0025Tests.cs`, which arrived as an
add/add conflict. They are now split: `UA0025Tests.cs` holds master's
`ILocalNode` tests, `UA0027Tests.cs` holds the `NodeBrowser` ones.

UA0024's help text also drops its reference to `ReadServerDiagnostics`,
which #4193 removed as unused, and names `ISession.ReadDiagnostics` /
`ISubscription.ReadDiagnostics` instead — those do still exist.


## 4. Two more locks reachable from outside their owner

**`ApplicationConfiguration.PropertiesLock` is removed.** It returned
the properties dictionary itself, so `lock
(configuration.PropertiesLock)` was `lock (configuration.Properties)` —
the lock *was* the data it guarded, the same shape as #4203.

It had exactly one production consumer
(`OAuth2CredentialCollection.Load`, a get-or-add with `ParseExtension`
called inside the lock), so it could not simply be deleted. `Properties`
is now a `ConcurrentDictionary` — same public type, every operation
atomic on its own — and the one multi-operation need gets a member of
its own:

```csharp
public T GetOrAddProperty<T>(string key, Func<T> valueFactory);
```

It deliberately does **not** invoke the factory under a lock, so
`ParseExtension` now runs outside any critical section. Holding a lock
across a caller-supplied callback is exactly the shape that produced the
attribute/diagnostics deadlock earlier in this work.

The two existing tests asserted only `PropertiesLock is not null`, so
they tested nothing but its presence. They are replaced by tests of the
contract: factory runs once for a missing key, not at all for a present
one, 32 concurrent callers all observe the same instance, null arguments
throw.

`PropertiesLock` ships in 1.5.378, so this is a breaking removal —
migration guide section plus analyzer **UA0028**.

**`SharedDefinitionCache` no longer hands out its lock.** Three sites in
`SharedKeyValueSubscriptionStore` took the nested cache's `Lock` to
operate on the cache's own dictionary. Both are private now, behind
`ReplaceAll` and `CloneWhere`.

`SnapshotCommitLock` stays exposed on purpose: it is a `SemaphoreSlim`
sequencing an async commit across `await` points, which
`System.Threading.Lock` cannot span, so folding it in would change
behaviour rather than hide a lock.
## Verification

Merged `master` (`0edfe237a`, including #4234) with no conflicts.

- `dotnet build UA.slnx` — 0 warnings, 0 errors
- `Opc.Ua.Server.Tests` — 4074 passed
- `Opc.Ua.Types.Tests` — 8501 passed
- `Opc.Ua.MigrationAnalyzer.Tests` — 151 passed on all five TFMs (138 →
151, the new ones being UA0027 and UA0028)
- `Opc.Ua.Core.Tests` — 4249 passed
- `Opc.Ua.Redundancy.Server.Tests` — 573 passed

## Note for the reviewer

@romanett — your approval was given when this PR's diff was the 7
diagnostics files, because it was measured against
`marcschier/node-browse-locks`. Now that the two are combined and the
base is `master`, the diff is 23 files, and the extra 16 are the
node/browser lock work that was #4221. GitHub kept the approval across
the retarget, but it did not cover that code, so please re-review rather
than let it ride.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041
Copilot-Session: b3c0e146-5b9e-4d08-a0f6-db3f940484b1
Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63
Copilot-Session: 0ee22c1d-c319-4257-bf78-a5a077a1824a
 2026-08-11 22:14:10 +02:00
Marc SchierandCopilot e05a820c91 Removal of the exposed diagnostics and node locks (#4188) 
## What this is

Design analysis for `Opc.Ua.Server`, the two rounds of design
exploration behind it, and the first two implementation chunks it
recommends.

Draft: the analyses are ready to read, but the *conclusions* deserve
discussion before the larger phases proceed. Later chunks stack onto
this PR as they land.

| File | Kind | Lines |
|---|---|---:|
| `plans/NodeManagerAnalysis.md` | Evidence — the node-manager plugin
surface | 488 |
| `plans/ServerRuntimeAnalysis.md` | Evidence — the server runtime
cluster | 576 |
| `plans/ServerDesignExploration.md` | Design round 2 — delivery
strategy | 1,394 |
| `plans/ServerDesignExplorationAlt.md` | Design round 1 — four
interface designs | 2,119 |
| `src/Opc.Ua.Types/State/NodeState.cs` | **Code** — node self-guards
attributes | |
| `src/Opc.Ua.Types/State/BaseVariableState.cs` | **Code** — value
accessors take the lock | |
| `src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs` | **Code** —
6 external locks removed | |
| `tests/…/NodeStateAttributeConcurrencyTests.cs` | **Code** —
concurrency safety net | 449 |

`docs/` is untouched — it holds shipped feature documentation, and none
of this is shipped API.

## The analyses

Counted mechanically over `src`, `samples` and `tests`; **every figure
is reproducible** from commands printed in each document.

**Node manager surface.** `AsyncCustomNodeManager` declares **93
`virtual`/`abstract` members** (80 distinct names). Across the
repository: **112 override sites, 25 distinct names ever overridden, 55
of 80 never**. Six carry nearly all real use. For scale,
`ReferenceNodeManager` is 6,373 lines and overrides **8**.

**Server runtime.** Seven findings, **two of which recommend no
change**. Findings 1–5 are filed as #4183–#4187. The headline is an
`object`-typed sync root published on three public interfaces and taken
in **88 `lock` statements across 7 files**, including a shipped sample.

**What is deliberately not flagged.** `IStandardServer` — 9 members over
5,575 lines, 16 of 37 virtuals used — is identified as the reference
shape to preserve. `IApplicationInstance` and the already-`[Obsolete]`
`Stop()` are examined and left alone.

## The design exploration

**Round 1** explored four alternative interfaces in parallel under the
current back-compat constraint. Its value is the convergence: four
designers who never saw each other's work independently arrived at a 6–9
member `IServerContext` replacing the 57-member locator, removal of the
`object` node-manager handle, elimination of the `Processed` protocol,
and `IHistorianProvider` as the pattern to copy.

**Round 2** re-runs it with back-compat lifted and asks how to deliver:
full rewrite versus side-by-side fork. **Both option designers
independently declined to recommend their own assignment.** Three
structural facts were verified:

1. **The fork line already sits at `IAsyncNodeManager`** — old and new
node managers can coexist with zero runtime changes, so a fork
duplicates ~10% of the assembly, not all of it.
2. **`[OpcUaShim]` cannot shim an inheritance surface** — honest
auto-fix ceiling for the authoring surface is **~10%**, against ~70% for
the shipped analyzers.
3. **A second lock leak exists** — `NodeState` takes `lock (this)` under
a `CA2002` suppression whose own comment documents the contract.

Recommendation: **neither pure option**, but a staged path reaching the
rewrite end state incrementally, gated on migrating the five
framework-internal node managers that today make it impossible for the
old base to ever freeze.

## Implementation: NodeState now guards its own attributes

This removes the prose-only contract in which `NodeState` took
`lock(this)` and every external caller was required to take
`lock(source)` on the node instance. The code already carried a TODO
describing exactly this fix and noting it required updating every
external site.

- The node self-guards rather than relying on callers.
`BaseVariableState` gains a **fully private** `System.Threading.Lock`
covering its value fields; `NodeState`'s other collections keep their
existing private locks.
- All **6** `lock(this)` sites and both `CA2002`/`RCS1059` suppressions
are gone.
- `Value`, `Timestamp` and `StatusCode` on `BaseVariableState` take the
lock too — without that, a direct property write raced an attribute read
once the external lock was gone.
- **6 of 9** external sites removed. Five were already redundant
(`GetReferences`, `GetChildren`, `ReferenceExists` and
`SetAreEventsMonitored` guard themselves; `FindMethod` reaches its state
via `GetChildren`); the sixth was an attribute read now covered by the
node's own lock.
- Attribute read/write deliberately take **no** node-wide lock: both
dispatch to caller-supplied handlers (`OnReadValue` and siblings) that
may take locks of their own, and holding a lock on the node across one
gives the node no fixed place in the lock order.
- **3 deliberately retained and annotated in code**: two `CreateBrowser`
calls, which are not internally synchronized (tracked in #4216), and one
in `CustomNodeManager`, whose class is slated for deletion so
refactoring its lock would be wasted work.

### No lock is exposed anywhere, to anyone

An earlier revision put the attribute lock on `NodeState` as `private
protected`, which still published a lock-sharing contract on the
inheritance surface. Review pushed back and it is gone:
`BaseVariableState` was the only user, so the lock moved there as a
plain `private readonly Lock`. There is no member for a derived type to
call, in this assembly or any other.

`BaseVariableValue` was the last public leak — it handed out the lock
guarding its value through a `public Lock` property, so any caller could
hold it across a call back into the stack. That property is removed:

- Derived value classes (what the source generator emits for every
structure variable) synchronize through a `protected EnterLock()` /
`ExitLock()` pair. Callback helpers were the first choice, but the
generated bodies pass `ref Variant` / `ref StatusCode` / `ref
DateTimeUtc`, and `ref` arguments cannot be captured by a lambda. The
lock *object* stays private either way.
- `NodeStateTemplates` emits the new form.
- `ServerInternalData`, the one cross-assembly caller, was reading the
lock back off a value it had itself constructed with that lock. It now
takes `m_diagnosticsLock` directly.

`NodeBrowser.DataLock` remains `protected` and is tracked in #4215: a
subclass extending `Next()` legitimately needs its enumeration state
atomic with the base browser's, so the fix needs a base-owned
acquisition rather than a deletion.

### Migration coverage for the removed lock members

None of the removed lock members get an `[Obsolete]` shim, and the
migration guide now says why: a shim would have to be `object`-typed to
keep old call sites compiling, and on .NET 9+ `lock (o)` on a
statically-`object` reference is `Monitor.Enter`, which does **not**
interlock with `Lock.Enter()`. It would compile, run, and silently
synchronize nothing — strictly worse than a compile error. Re-adding a
member to a downstream-implemented interface is also a break in its own
right on `net48` / `netstandard2.0`, where there are no default
interface methods.

Instead, every removed member has a named replacement on the migration
path:

| Rule | Covers | Replacement it names |
|---|---|---|
| `UA0024` | `IServerInternal` / `ISession` / `ISubscription`
`DiagnosticsLock`, `DiagnosticsWriteLock` |
`UpdateServerDiagnostics(...)` / `UpdateDiagnostics(...)`,
`ReadServerDiagnostics` / `ReadDiagnostics` |
| `UA0025` | `ILocalNode.DataLock`, `Node.DataLock` | a lock the caller
owns; the node guards its own state |
| `UA0026` | `BaseVariableValue.Lock` | a lock passed to the
`BaseVariableValue` constructor |

Each rule fires on sources still written against 1.5.378, where the
compiler only says a member is missing. `docs/MigrationGuide.md` gains a
section per removal with before/after.

### The tests earned their keep

The four concurrency tests were landed **first**, against the unmodified
code, because removing a synchronization contract cannot be verified by
the compiler and the existing `NodeStateHandlerConcurrencyTests` covers
a different concern (racing a handler assignment against a write, not
reader/writer exclusion). They now run with **no external locking**,
which is what demonstrates the node guards itself.

They caught two real problems, and one of them was mine:

- The `Value`/`StatusCode`/`Timestamp` setters bypassed the new lock — a
genuine race.
- One test asserted an invariant the design does not offer. Two property
setters are two lock acquisitions, so that pair was **never** atomic; it
only passed originally because the test held a lock across both. It now
uses a single attribute write carrying a whole `DataValue`, which
genuinely is atomic and is the path the server uses.

A third issue was found and fixed before it shipped: the reader loops
gated on `!writer.IsCompleted`, so a reader scheduled after the writer
finished performed zero reads — failing two runs in three on `net48`.
Converted to `do…while`; now five consecutive green runs.

**Verified:** `Stack.State` 116 passed on `net10.0` *and* `net48`; the
concurrency category green 5/5 consecutive on `net48`;
`Opc.Ua.Types.Tests` 8,456 passed; the server node-manager suite 1,205
passed, including `NodeManagerLifecycleTests` and
`AsyncCustomNodeManagerTests` which exercise the changed paths.

## Two corrections to my own published figures

Both were found while starting implementation, and both are recorded on
#4183.

**`ForceDiagnosticsScan()` was overstated.** The analysis claimed the
`DiagnosticsWriteLock` getter *"triggers a full address-space
diagnostics rescan"*. It is in fact a single flag assignment
(`DiagnosticsNodeManager.cs:700-703`) consumed by the read paths and the
periodic timer. The defect stands — a side-effecting property getter
with a racy write on a synchronisation path — but its cost was
misstated.

**The contract was 15 sites, not 19.** A naive search matched ten
comment and `#pragma` lines that *mention* `lock(source)` while
explaining it. The documents now carry a counting command that excludes
comment matches.

A third finding expanded scope the other way: the `object`-typed lock
leak is **repo-wide**, with ten such members across `Opc.Ua.Core`,
`Opc.Ua.Types` and `Opc.Ua.Server` — including `Node.DataLock => this`
and `ApplicationConfiguration.PropertiesLock => m_properties`, the same
defect class as `Subscription.DiagnosticsLock`.

## Caveats stated in the documents

- **Nothing in the designs is implemented or benchmarked.** Every
performance claim across all seven designs is a design intention, not a
measurement.
- The larger phases are a **major undertaking against a live plugin
API**; the documents say so rather than letting the neatness of the
proposals imply otherwise.
- The recommended sequencing puts low-risk, self-contained steps first
and notes they are **worth doing regardless** of which strategy is
eventually chosen.
- Round 2 notes the two option designers' cost estimates are **not
independent of their assignments**.

## Review guidance

The two analyses make factual claims checkable by running the included
commands. The design explorations are exploratory and need not be agreed
with to merge. The code change is small, test-backed, and independent of
which delivery strategy is eventually chosen.

Reasonable outcomes include merging as-is, merging only the analyses and
the lock fix, or disagreeing with the recommendation while keeping the
evidence.

---

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041
Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63
 2026-08-10 06:47:50 +02:00
Marc SchierandCopilot 416d619c41 Split the CI matrix across both systems, split the required test gate from the advisory coverage check, and give background work an owner (#4158) 
# Description

Two changes to how CI is routed and how it reports.

## 1. Split the CI matrix back across both systems

Flips both halves of the CI backend switch from `ado` to `actions`, so
the workload is shared instead of concentrated on the managed pool. The
switch is checked into source in **two places that must always be
flipped together**:

| File | Setting | Before | After |
| --- | --- | --- | --- |
| `azure-pipelines.yml` | `parameters.ciBuildBackend` | `ado` |
`actions` |
| `.github/workflows/buildandtest.yml` | `env.CI_BUILD_BACKEND` | `ado`
| `actions` |

| Work | GitHub Actions | Azure Pipelines (managed pool) |
| --- | --- | --- |
| All-TFM solution builds | `build-all-tfm-windows` + `-linux` | two-TFM
PR gate (`net10.0`, `net48`) |
| Test matrix | ubuntu, every test project | Windows `net48` + `net10.0`
fast PR legs |
| Native AoT | ubuntu | Schedule/Manual only |
| macOS | always (no managed-pool macOS image) | — |

## 2. Split the required gate from the advisory coverage check

The coverage gate job was appended to *every* test stage, so `Fast PR
test` emitted three sibling checks whose display names collided —
`Coverage Release (net10.0)` appeared twice, once for the Windows leg
and once for macOS, with nothing to tell them apart:

```text
✓ Coverage Release (net48)      55s
● Coverage Release (net10.0)
○ Coverage Release (net10.0)
```

That job also fused two unrelated concerns: whether the tests passed
(which must block a merge) and whether coverage met its thresholds
(which should not — the `net10.0` leg had `enforceCoverage: true`, so a
coverage dip blocked merges).

Both concerns now get a stage of their own, mirrored in both CI systems:

| Concern | Azure Pipelines | GitHub Actions | In the branch ruleset? |
| --- | --- | --- | --- |
| Every test passed | **`Tests passed`** stage | **`build-and-test
summary`** job | **Yes — required** |
| Coverage meets thresholds | **`Code coverage`** stage | **`code
coverage`** job | **No — advisory** |

The coverage check reports a clean failure when the thresholds are
missed, so the miss is visible, but it never blocks the merge.

Coverage is now merged **once** across every leg of the run instead of
once per leg, which also removes the `enforceCoverage` /
`publishCoverage` "exactly one invocation may set this" parameters.
Neither check re-runs any tests — the previous design re-ran the whole
suite in one job with the collector attached, which serialised a suite
that is deliberately fanned out and always exceeded the stage timeout.

### Coverage checks now exist in GitHub Actions

`build-and-test` already ran with `--collect:"XPlat Code Coverage"` and
uploaded the fragments, but nothing ever merged or evaluated them. It
now runs the same `check-coverage.ps1` against the same
`coverage-thresholds.json` as Azure. Since `CI_BUILD_BACKEND` defaults
to `actions`, that ubuntu matrix — which runs *every* test project — is
the fullest coverage signal in the repository.

### Reporting the numbers

`check-coverage.ps1` gained `-SummaryPath`, rendering a markdown report
that both systems surface:

- **GitHub Actions** — appended to the job summary and upserted as a
single **sticky pull-request comment**, updated in place on each run.
Threshold misses also appear as `::error::` annotations.
- **Azure Pipelines** — attached to the build summary via
`##vso[task.uploadsummary]`.

The report is a pass/fail table of the project floor, patch coverage and
baseline delta, with the uncovered changed lines listed by file in a
collapsible block. Rendered example:

> ## Code coverage
>
> ❌ **Coverage gate failed.** This check is advisory and does not
block the merge.
>
> | Check | Result | Threshold |
> | --- | --- | --- |
> | ✅ Project line rate | **72.10%** (40 731/56 496
lines) | >= 70.00% |
> | ❌ Patch coverage | **0.00%** (0/1 changed lines) | >= 75.00% |

The comment is fork-safe: a `pull_request` from a fork gets a read-only
token regardless of the `permissions:` block, so it degrades to the job
summary alone. The sticky comment uses `actions/github-script` rather
than a community action, keeping this workflow on first-party actions
only.

## Notable correctness details

These were found and fixed during review, and are the parts most worth a
second pair of eyes:

- **`dependencies.<stage>.result` resolves at *stage* scope only.** At
job scope `dependencies` refers to other jobs of the same stage, so
reading stage results from a jobs template would have silently returned
empty and the gate would have passed everything. `gate.yml` and
`coverage.yml` are therefore **stage** templates that derive their own
`dependsOn` from the same stage list they evaluate, so the two can never
drift apart. An unresolved result is a hard error.
- **Both rollups run on `not(canceled())` / `always()`**, not on
success. A check that reports **skipped** is treated by GitHub as
**satisfied**, so a required check that skips when its dependency fails
would wave a red build straight through.
- **`get-matrix.ps1` used to emit an empty matrix on a discovery miss.**
The downstream job is then skipped, a skipped job rolls up as a
*succeeded* stage, and the gate would have approved a run that executed
no tests. It now fails, with an `-AllowEmpty` opt-out.
- **Zero coverage reports used to be a green advisory check**, which
would hide a broken collector indefinitely. Both systems now distinguish
"the tests never ran" (warning) from "the tests ran and published
nothing" (error).
- The gate reads stage-level results rather than enumerating
matrix-generated job names, which change whenever a test project or
agent is added.

## Test-host hang and the reconnect defect behind it — both fixed

Flipping `CI_BUILD_BACKEND` to `actions` started exercising the ubuntu
matrix, which
`master` never runs, and it immediately surfaced two real defects.

**1. The test host hung for ten minutes.** `Assert.ThrowsAsync` blocks
the calling thread
with no timeout, and these tests drive a *fake clock* from that same
thread — so once the
reconnect parked on a fake-clock timer, nothing could ever advance it.
NUnit sat in
`WaitForCompletion` until `--blame-hang-timeout` killed the host,
aborting the run at 3902
of 4238 tests and taking every other test in the assembly down with it.
A hang dump named
the culprit exactly. Replaced with an awaited, bounded
`AssertThrowsAsync` helper (the
repo's own rules forbid sync over async). The run now completes in
**~1m29s** with all
tests executed.

**2. A budget-aware reconnect never returned.** With the hang gone, two
tests failed with
`Expected ServiceResultException but got TimeoutException`. `dumpasync`
on a hang dump gave
the whole chain:

```
ReconnectAsyncWithBudgetShrinksDelayToFitRemainingAsync
  → AssertThrowsAsync
  → ClientChannelManager.ReconnectAsync
  → ClientChannelManager.ReconnectLeaseAsync
  → ClientChannelManager.SwapFaultedEntryAsync   ← parked here
```

`master` (via #4128) added `IsTerminalReconnectRace` so a reconnect that
loses a race
against a concurrent close recovers on a freshly swapped entry. The
predicate only asks
whether the entry ended up `Closed`/`Faulted` with
`BadSecureChannelClosed` — which is
*also* exactly how a reconnect ends when the caller's retry budget or
the reconnect policy
is deliberately exhausted. So a budget-exhausted reconnect was misread
as a race: the
manager swapped the entry and started a **second, unbudgeted** cycle,
waiting out the swap
back-off first. `ReconnectAsync(channel, budget)` therefore could not
return within its
budget, and under a fake clock it never returned at all.

The fix requires the budget to have room left before treating a terminal
failure as a race:

```csharp
return !ct.IsCancellationRequested &&
    !budget.IsExhausted &&
    sre.StatusCode == StatusCodes.BadSecureChannelClosed &&
    entry.State is ChannelState.Closed or ChannelState.Faulted;
```

The parameterless `ReconnectAsync(channel, ct)` overload passes an
unlimited budget, so
genuine race recovery is unchanged. Verified locally: Core 4152, Client
2122 and Sessions
776 tests all pass, and the full GitHub Actions PR run is green across
56 jobs.

Two smaller fixes came along the way, neither weakening an assertion:

- `ObservableFakeTimeProvider` waiters made **relative** ("N more timers
from now") rather
than absolute ("the Nth timer of the run"). Absolute numbering let an
earlier reconnect
consume the slots, so a waiter completed before the awaited timer
existed and `Advance()`
  fired nothing.
- The raw worker thread in `NodeStateHandlerConcurrencyTests` was a
**foreground** thread
whose stop flag was read without a barrier and set only on the success
path — a failed
assertion leaked a spinning thread that pins the process at exit. Now
background,
  `Volatile`, stopped in a `finally`, joined with a bound.
## Background work now has an owner

The reconnect hang above was one symptom of a pattern the codebase had
**26 times** across seven assemblies: a bare `_ = Task.Run(...)`. Each
one handed work to the thread pool and forgot it — nothing observed the
exception if it threw, nothing bounded how many ran at once, and
disposal raced them, so a component could finish tearing itself down
while work it started was still touching its fields. Two of those sites
already carried a standing `// TODO: Await the task completion in
shutdown`.

`BackgroundTaskScope` (in `Opc.Ua.Types`) gives them an owner: schedule
with `Run`, drain with `DisposeAsync`, or signal-only with `Dispose`
where the owner's teardown is synchronous and awaiting would be sync
over async. `Run` never blocks and never throws — which matters, because
most of these call sites went to the thread pool precisely to escape a
lock they were holding. 11 unit tests cover drain, cancellation,
exception observation, the concurrency cap, post-shutdown rejection and
idempotent disposal.

Two of the fixes were more than re-wiring:

- **`SessionManager` / `SubscriptionManager` loops were structurally
unawaitable.** Both return `ValueTask`, so `Task.Factory.StartNew(() =>
...)` handed back a `Task<ValueTask>` that completes at the first yield.
Storing it would not have helped — awaiting it would only have awaited
the *scheduling*. Both now go through `AsTask().Unwrap()`, matching the
neighbouring `StartConditionRefreshWorker`.
- **`ChannelAsyncOperation` was swallowing subscriber exceptions.** Its
`doNotBlock` branch had no try/catch while the inline branch beside it
did. It is per-operation with no owner to hang a drain on, so it gained
the missing handler rather than a scope.

The Kubernetes readiness endpoint keeps a purpose-built semaphore
instead: an unauthenticated endpoint needs a hard cap on concurrent
handlers, not just an owner.

### ⚠️ Breaking: `ISessionManager.Shutdown()` is removed

Replaced by `ShutdownAsync(CancellationToken)`. There is no correct
synchronous way to wait for the session monitor loop, so leaving the
overload would only have preserved the race. This mirrors
`ISubscriptionManager`, which already had `ShutdownAsync`. Migration
notes are in `docs/MigrationGuide.md`; only one in-repo implementer
existed.

Also converted the injected `object partitionLock` shared by
`LogicalSubscription` and `CompositeMonitoredItemCollection` to
`System.Threading.Lock`, per the repo rule.

## Patch coverage now scales with the size of the patch

A flat floor over changed lines punishes small changes for arithmetic:
two uncovered lines in a four-line fix reads as 50 % and failed an 80/5
floor — which is exactly how this branch earned a red coverage check for
a four-line predicate change. A check that fires on changes nobody
considers undertested is one people learn to ignore.

| Coverable changed lines | Floor | Below it |
| --- | --- | --- |
| 1 – 10 | 50 % | warning, still passes |
| 11 – 100 | 60 % | warning, still passes |
| more than 100 | 75 % | **failure** |

Only changes past the last band can fail. Below it the author still gets
a warning naming the uncovered lines. Bands live in `patch.bands` in
`coverage-thresholds.json` and each carries its own `enforced` flag.
Verified end-to-end against synthetic repositories at every boundary.

## Codecov is back, as reporting

Dropped in #4122 when the in-pipeline gate replaced it. Keeping the gate
was right, but dropping Codecov also lost the PR comment, the
file-by-file diff view and the trend. It returns with both statuses
`informational: true` — two gates with two sets of thresholds would
eventually disagree, and `coverage-thresholds.json` stays the single
source of truth.

The upload is switchable and can never fail a build: `enableCodecov`
(Azure) and `ENABLE_CODECOV` (Actions), both defaulting on, both skipped
when `CODECOV_TOKEN` is absent as on fork PRs. Each system uploads the
single merged report it already builds, under its own flag. The old
`codecov.io/bash` uploader was sunset, so Azure uses the current CLI.

## Follow-up actions for maintainers

1. **Update the branch ruleset.** The per-leg `Coverage Release (…)`
checks are gone. Azure reports checks as `<pipeline> (<stage> <job>)`,
so the exact names are:

   | Check | Require? |
   | --- | --- |
| `OPCFoundation.UA-.NETStandard (Tests passed Verify stage results)` |
**yes** |
   | `build-and-test summary` | **yes** (already required) |
| `OPCFoundation.UA-.NETStandard (Code coverage Merge and evaluate)` |
no |
   | `code coverage` | no |
2. **Re-calibrate the thresholds.** `coverage-thresholds.json` is still
seeded from the last Codecov reading (73.61 %). The two systems now
report *different* numbers by design — Actions merges every project on
ubuntu, Azure only the Windows fast-PR legs, and scheduled runs read
higher still because the Debug / .NET 8 / .NET 9 / netstandard stages
also contribute. Re-seed from the GitHub Actions figure.
3. **Note:** a failed advisory coverage stage still turns the whole
Azure run red, which the README build badge reflects on `master`. That
follows from wanting a visible red X on a threshold miss; say the word
if you would rather it were a warning.
4. **The Azure agent pool is the slowest thing here.** Several runs on
this PR sat queued for hours, and one build failed with `Insufficient
system resources` while writing an XML doc file — an agent exhaustion
error, not a code error. It passed on re-run. Worth raising against the
`netstandard` Managed DevOps Pool capacity separately.
## Related Issues

_No tracking issue; this is CI infrastructure work requested directly.
It builds on the switch introduced in #4122._

## 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.
- No product code changed, so there are no unit tests to add. The new
logic was verified with purpose-built harnesses instead:
- `check-coverage.ps1` run for real against a merged Cobertura report on
all three paths — pass, threshold failure, and patch coverage with
uncovered changed lines (a scratch commit adding an uncovered method was
correctly resolved to `DataTypeException.cs:193`, reported 0 % patch
coverage and exited 1).
- The gate's accept/reject logic dry-run over `Succeeded` /
`SucceededWithIssues` / `Skipped` / empty / `Failed` / `Canceled` — 6/6
correct.
- `get-matrix.ps1` exit codes verified in clean processes: empty → 1,
normal → 0, `-AllowEmpty` → 0.
- All five pipeline/workflow files parse; both PowerShell scripts parse.
- Action versions checked against the GitHub API and pinned to current
majors.
- [x] I have added all necessary documentation.
- Rewrote the coverage section of `docs/DeveloperGuide.md` as *Required
checks and coverage*, covering which checks belong in the ruleset and
which must not, how coverage is measured, where the numbers appear, and
why the two systems disagree. Corrected the stale "BLOCKS" wording in
`coverage-thresholds.json`.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- CI YAML, two PowerShell scripts and documentation only; no compilation
involved.
- [x] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- Not applicable — no product code changed. `Opc.Ua.Core.Schema.Tests`
was run repeatedly on net10.0 (119 passed) to generate the real
Cobertura fragments used to validate the gate.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [x] I have addressed **all** PR feedback received.
- Feedback on the path-relevance diff (now a merge-base diff) and the
target-framework numbering in `docs/DeveloperGuide.md` is fixed in
`15b8e38df`.
- A review pass over the gate/coverage split produced the four
correctness fixes listed above, in `863eefc4d`.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
 2026-08-07 06:56:05 +02:00
Marc SchierandCopilot 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 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00