GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-21 17:16:16 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,739 Commits 110 Branches 105 Tags
fix/encoder-stack-depth-json-flake
Commit Graph

14 Commits

Author SHA1 Message Date
romanett 9b034f8291 Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) 
# Description

Nullable reference types are now enabled once, in `common.props`, and
every project in the repository builds warning-clean with them.

**Configuration**
- `<Nullable>enable</Nullable>` lives in `common.props`. All per-project
`<Nullable>` settings are removed: 178 project files (enable,
annotations, and the `disable` in the three Aot test projects).
- 419 per-file `#nullable enable`/`restore` directives and the 4
`#nullable disable` directives are removed.
- Kept on purpose:
- The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type
Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`,
`PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the
project setting in generated code, so the directive is required there.
- `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against
the 1.5.378 packages and does not import the repository props.
- `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now
describe the central setting.

**Migration** (the 43 unset and 13 annotations-only projects, roughly
10k diagnostics)
- **Production code: annotated at the declarations.** Covers
`tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the
LDS sample.
- Members, parameters and returns are nullable where null actually
flows.
- Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`,
`IsMethodTypeDesign`).
- `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take
`params object?[]`. `Template.AddReplacement` takes `string?` (rendering
already skips null replacements).
- `IRegisteredServerStore.RegisterAsync` returns
`Task<RegistrationEntry?>`: it returns null when a registration is
removed.
- Dereferences backed by an invariant (for example, the validated model)
use `!`, so runtime behaviour is unchanged.
- **Behavioural changes in production code** (small, deliberate):
- `NodeSetToModelDesign` throws `InvalidOperationException` when the
file is not a NodeSet. Before, it threw a NullReferenceException on the
next line.
- `ModelDesignValidator` starts its lookup tables empty instead of null.
A redundant `if (m_nodes == null) return;` guard is gone.
- **Test code: minimal annotations.**
  - `?` on values that can be null, `!` where the test asserts non-null.
  - `= null!` for fixture members assigned in SetUp.
- `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the
`IEncodeable` constraint.
- A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`)
folded into the adjacent `IsNull`/`Count` checks.

**Notes for reviewers**
- The diff is large (~1,500 files) but almost entirely mechanical: `?`,
`!` and directive removal. The non-annotation changes are listed above.
- Known follow-up, not done here:
`IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay
non-nullable even though the validator returns null before it loads a
dictionary. Making them nullable would ripple through every generator
call site.

## Related Issues

- Fixes #4439

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

The "no new warnings" box covers net10.0 and net48: full non-incremental
builds of `UA.slnx`, `tools/SourceGeneration.slnx`,
`tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0
warnings and 0 errors under the repository's warnings-as-errors
settings. net9.0 and net8.0 also had no nullable diagnostics, but they
were not rebuilt after the final fixes. Tests have not been run locally
yet.

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-08 08:48:13 +02:00
romanett 7212607f16 OPC UA Part 3 compliance: permissions, Write semantics, NodeManagement, NodeSet DataTypes, locales (#4675) 
# Description

Fixes the High and Medium findings of an OPC UA Part 3 (OPC 10000-3,
Address Space Model) compliance audit. Each finding was checked against
the full spec text (Parts 3, 4, 5, 6, 14 and 18) before it was fixed.
Every fix comes with regression tests that fail without it.

### Role permissions (Part 3 §8.55, Part 4 §5.12.2.2)
- The type-hierarchy exemption in
`MasterNodeManager.ValidateRolePermissions` /
`ValidateAccessRestrictions` now covers only Browse, Read and
ReadRolePermissions. Type nodes stay visible to generic clients, which
was the intent of #3705; every other permission bit is now enforced on
them.
- `ReceiveEvents` is enforced on the EventType node. The standard
NodeSet grants it on `AuditEventType` and its subtypes only to
SecurityAdmin, so only SecurityAdmin sessions now receive audit events.
- Call checks the `Call` permission and AccessRestrictions on the Object
as well as on the Method.
- When a Call's `methodId` is the Method declaration of the ObjectType,
the Object's Method with the same BrowseName is permission-checked and
invoked. The declaration is used only if the Object has no such Method.
This applies to `AsyncCustomNodeManager`, `CustomNodeManager2` and the
sample node manager.

### Write service (Part 3 §5.6.2, §8.40, §8.57; Part 4 §5.11.4)
- The synchronous `OnWriteValue` path (fluent `OnWrite`) checks DataType
and ValueRank first (`Bad_TypeMismatch`). After an IndexRange write it
merges the slice into the cached value instead of replacing it.
- StatusWrite / TimestampWrite are enforced (`Bad_WriteNotSupported`).
- Undefined enumeration values and invalid OptionSet writes return
`Bad_OutOfRange`. OptionSet writes merge only the valid bits into the
stored value.
- New `ReportPropertyValueChanged` / `ReportSemanticChange` let server
code raise SemanticChangeEvents for changes it makes itself.
- Required follow-up (Part 14 §6.2.11.1): if an external target rejects
a timestamp, the PubSub subscriber's TargetVariables writer retries
without it.

### NodeManagement and Call arguments (Part 3 §5.5.2, §5.7.3, §6.4.2;
Part 4 §5.8, §5.12.2)
- AddNodes creates the Mandatory InstanceDeclarations of the
TypeDefinition and rejects abstract or Interface types
(`Bad_TypeDefinitionInvalid`).
- AddReferences, DeleteReferences and DeleteNodes bump NodeVersion and
raise GeneralModelChangeEvents.
- Clients may omit trailing optional input arguments
(`HasOptionalInputArgumentDescription`).

### NodeSet DataTypes (Part 3 §5.8.3, §8.48; Part 6 Annex F.12)
- `UANodeSet.Import` completes structure definitions with the base
type's fields (base fields first). It creates definitions for structures
without fields and sets `DefaultEncodingId`.
`StructureDefinition.FirstExplicitFieldIndex` marks the subtype's own
fields.
- `DataTypeState` no longer mutates the stored definition on read.

### Locales (Part 3 §5.2.5, §8.5; Part 4 §5.4)
- Read, Browse and data change notifications return the translation that
best matches the session's LocaleIds, or all translations for `mul`. The
selection lives in the server dispatcher and is built on
`ResourceManager`: the new `TranslateValue` calls the overridable
`Translate`.
- Locale ids are matched case-insensitively, and fallback translations
carry their real locale.

### Client complex types (Part 6 §5.1.7)
- The Reflection.Emit builder encodes only `IsOptional` subtyped fields
as ExtensionObject. It also supports OptionSet subtypes: an unsupported
type is skipped instead of aborting the whole load.
- `TypeInfo.GetXmlName` uses the data type's XML name for dynamic
structures and OptionSets (the XML body element was `<Structure>`). Both
builders now produce identical XML.

## Behaviour changes
All are documented in `docs/MigrationGuide.md`.
- **Audit events:** only SecurityAdmin receives them. The CTT "Auditing
Connections" unit needs a SecurityAdmin user (e.g. sysadmin);
`AuditingOperationTests` was updated.
- **Write timestamps and status:** clients writing a SourceTimestamp or
a non-Good StatusCode get `Bad_WriteNotSupported` unless the Variable
has StatusWrite/TimestampWrite. No variable in `src/` sets these bits.
- **AddNodes:** it rejects abstract types (including `BaseVariableType`
and `BaseEventType`), and new instances now come with child nodes.
- **Call:** the Object needs the Call permission. The Object's own
Method runs even when the call names the type declaration.

## Not included
- Low-severity findings: missing validation on optional services and
author-side NodeSet rules.
- LocalizedText nested inside structures or Variant arrays, and in
HistoryRead or Call outputs.

## Testing
net10.0, Debug, one suite at a time on the merged branch. All suites
below pass:

| Suite | Passed |
|---|---|
| Types | 11430 |
| Core.Encoders | 2947 |
| Core.Schema | 179 |
| Core.Security | 558 |
| Client | 2761 |
| Server | 8210 |
| InformationModel | 1080 |
| Features | 145 |
| History | 536 |
| Di | 455 |
| Machinery | 167 |
| Robotics | 661 |
| Robotics.Intent | 22 |
| Pumps | 75 |
| Scales | 122 |
| Positioning | 69 |
| ISA95 | 144 |
| Vision | 484 |
| PubSub | 1490 |
| PubSub.Adapter | 151 |
| PubSub.Server | 177 |
| Redundancy.Server | 856 |
| Lds | 173 |
| Configuration | 241 |
| WotCon | 1496 |

- **Client.ComplexTypes:** one test wrote a SourceTimestamp to a
variable without TimestampWrite. It is fixed in this PR, and the suite
has not been re-run since.
- **Core:** 6068 passed, 1 failure in
`ActivityTraceFlags_Metrics_ShouldRecordMetricsOnly`. That telemetry
test passed when re-run alone, so it looks flaky.
- **Still running:** Sessions, Subscriptions (Classic, Durable,
standard) and Interop. Results will follow on this PR.
- `UA.slnx` builds with 0 errors for net10.0 and net48, with no new
analyzer warnings in changed files.

## Related Issues

- Part 3 compliance audit (no tracking issue)

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

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-05 12:43:59 +00:00
romanett 20c2d77d57 Fix Core/Types audit findings and conform them to the OPC UA spec (stacked on #4569) (#4573) 
> **Stacked on #4569.** The base branch is
`romanett/practical-murdock-fd9c56`, so this diff shows only the commits
of this PR. Retarget to `master` after #4569 merges.

## Summary

Bug audit of `Opc.Ua.Core`, `Opc.Ua.Core.Types`, `Opc.Ua.Types` and
`Opc.Ua.Core.Schema`, with the findings fixed and the fixes grounded
against the OPC UA specification (reference.opcfoundation.org). 133
commits in four groups:

1. **Audit fixes** (`Fix <ID>: ...`): 116 findings from a read-only
audit, each with a regression test. IDs follow the audit scopes: T1-T6
`Opc.Ua.Types`, C1-C4 `Opc.Ua.Core`, S1 `Opc.Ua.Core.Types` +
`Opc.Ua.Core.Schema`.
2. **Review fixes** (`Fix review <ID>: ...`): issues a code review of
group 1 found, plus lower-severity leftovers.
3. **Spec fixes** (`Spec: #<n> ...`): the 18 places where an earlier fix
only partly matched the spec text.
4. Test and analyzer follow-ups.

Where `master` (#4516, #4522, #4540, #4564) or #4569 already fixed the
same issue, their version was kept and this PR only adds what they
didn't cover.

## Behaviour and user-facing changes

### Wire format and interoperability (breaking)
- **PubSub AES-CTR (Aes128-CTR / Aes256-CTR)** now follows Part 14
Tables 156/157:
- The counter block is `KeyNonce[4] | MessageNonce[8] |
BlockCounter[4]`, and the block counter starts at 1.
- The SecurityHeader nonce is 8 bytes: `Random[4] | UInt32
SequenceNumber`, with the sequence number starting at 1.
  - SKS key data is split with a 4-byte KeyNonce, giving 52 / 68 bytes.
- **Not interoperable with earlier releases of this stack**, but matches
the spec. There is a test against the independent RFC 3686 vector.
- **User token signature** (Part 4 6.1.8):
- The signed data is now built from the user token policy's
SecurityPolicy on both client and server.
- It also supports the SecurityMode None layout `ServerNonce |
HASH(ServerCertificate) | ClientNonce`.
- New overload `SecurityPolicyInfo.GetUserTokenSignatureData(...,
MessageSecurityMode)`.
- **Secure channel:**
- The ECC AEAD `LastSequenceNumber` is correct across the 32-bit
sequence wrap (Part 6 6.8.1).
  - The incoming chunk counts against MaxChunkCount/MaxMessageSize.
- The sender no longer lets a message through that is 24 bytes over
MaxMessageSize.
- The OPN response signature is verified without overrunning the buffer
(RSA_DH with differing key sizes).

### Sessions (server)
- **CreateSession nonce:**
- The client nonce is kept on None channels, because the None-mode user
token signature covers it.
- A nonce over 128 bytes returns `Bad_NonceInvalid` on every channel
(Part 4 5.7.2.3).
  - The duplicate-nonce check runs only on secured channels.
- A non-empty nonce under 32 bytes is still accepted on None, for
compatibility.
- **ActivateSession:** an embedded `EccEncryptedSecret` signing
certificate is validated with the server's peer certificate validator
(Part 6 6.8.3). Byte equality with the client certificate is only the
fallback when no validator is available. New opt-in interface
`ICertificateValidatorProvider`.
- **FindServers / GetEndpoints:** `GatewayServerUri` and
`DiscoveryProfileUri` are no longer swapped.

### Write, Read and Call (NodeState / node managers)
- **UserWriteMask:**
- Enforced for non-value attributes and for VariableType Value writes
(`ValueForVariableType`), returning `Bad_UserAccessDenied`.
- An unconfigured UserWriteMask (0, no handler) is not enforced, and
reads back as the WriteMask so Read and Write agree (Part 3 8.60).
- A throwing or Bad `OnReadUserWriteMask` handler fails only that item.
- **Index-range writes:**
- Slices are merged into the cached value under the attribute lock, also
for `OnWriteValueAsync` handlers.
- A data-change notification is still produced when the cache cannot
absorb the slice.
- The slice must match the unclipped range, otherwise
`Bad_IndexRangeDataMismatch` (new status code in `Opc.Ua.Types`).
  - A range past the target returns `Bad_IndexRangeNoData`.
- A handler without index-range support returns `Bad_WriteNotSupported`
(Part 4 7.27, 5.11.4.4).
- **Index-range reads on matrices** return partial results clipped to
the source, instead of allocating the requested size (a Read with
`"0:40000,0:40000"` no longer allocates about 1.6e9 elements).
- **DataTypeDefinition writes:**
- A value that isn't a decoded DataTypeDefinition, or whose kind doesn't
match the DataType category, returns `Bad_TypeMismatch`.
  - Null is rejected for concrete Structure/Enumeration/OptionSet types.
  - WriteMask is checked first (Part 3 5.8.3).
- **AnalogItem:** a null write skips the InstrumentRange check again.
- **Method call:** the async fallback passes the ObjectId to the method
handlers.
- **Redundancy:** `DistributedValueParticipation` merges index-range
writes and applies the index range on reads.
- **Change masks:** `ClearChangeMasks` takes the bits atomically, so
concurrent changes are no longer lost.
- **`AccessLevelEx`:** preserved by Clone/Export.

### Alarms & Conditions (Part 9)
- **Acknowledge / Confirm / AddComment:**
- Accept any EventId the condition or its branches reported while that
state is still pending (5.7.3).
  - An unknown id returns `Bad_EventIdUnknown`.
- An already handled state returns `Bad_ConditionBranchAlreadyAcked` /
`...AlreadyConfirmed`.
- AddComment on a branch no longer changes the trunk, and the branch
table is re-keyed when a branch's EventId changes.
- **Shelving:**
  - OneShotShelve without MaxTimeShelved no longer overflows the timer.
- A timed shelve is measured on the monotonic clock, so wall-clock steps
don't delay the unshelve.
  - The state is named `OneShotShelved`.
- **Audit events:**
  - They report failure for rejected method calls.
  - `SelectedResponse` is a UInt32.
  - A rejected negative Respond leaves SelectedResponse unset.
- **`ContentFilterElement.Validate`:** an unknown FilterOperator is
reported per element as `Bad_FilterOperatorInvalid`, instead of failing
the whole service call.

### ContentFilter evaluation (Part 4 7.7.3, on top of #4564)
- **Bitwise operators:**
  - The result takes the size of the larger operand.
- Boolean, numeric String and one-element array operands are converted
to integers implicitly.
  - Types without an implicit integer conversion give NULL.
- **Strings:** ordering is ordinal, no longer culture dependent.
- **Cast:** restricted to the Table 121 conversions, and QualifiedName
to LocalizedText is added. The general `Variant.ConvertTo` keeps its
wider set.
- **Null and empty operands:**
  - `Like` with a NULL operand is NULL.
  - Empty arrays of the same DataType compare equal.
  - Null-string operands are NULL everywhere.

### Encoders and built-in types
- **XML:**
- Strings keep leading, trailing and whitespace-only content (Part 6
5.3.1.5). NodeSet import opts back into treating whitespace-only strings
as empty with the new `XmlDecoder.TreatWhitespaceOnlyStringsAsEmpty`.
- Layout whitespace around `<Identifier>` values is ignored, and
trailing control characters are stripped from `s=` ids.
  - `XmlElement` ignores DTDs without expanding entities.
- **JSON:**
  - A DateTime without an offset is read as UTC.
- An unmappable `nsu=` NodeId and an unknown `svu=` ExpandedNodeId
decode to a namespace-0 String NodeId (5.4.2.10 / 5.4.2.11).
  - `svr=` is rejected in a NodeId field.
  - Bad matrix dimensions return `BadDecodingError`.
  - The non-strict body stack stays balanced after an error.
  - The nesting limits match between encoder and decoder.
- **Binary:**
  - `MaxByteStringLength` no longer applies to strings.
- ExtensionObject bodies count toward the nesting limit on seekable
streams (and in the XML encoder).
- **Parsing:**
  - The NodeId parser requires `i=`/`s=`/`g=`/`b=`.
  - `ExpandedNodeId.Parse` rejects an empty `nsu=`/`svu=`.
  - NumericRange rejects a trailing `,` with `Bad_IndexRangeInvalid`.
  - RelativePath escapes `#`.
- **Status codes:** `StatusCode.WithSubCode`/`WithCodeBits` keep the
severity bits and look up the symbolic id for the new code.
- **Variant / VariantHelper:**
- `TryCastTo` returns false on a type mismatch. So
`DataValue.GetValue<T>(default)` returns the default, and
`GetValueOrDefault` / `CastTo` throw `BadTypeMismatch` as documented.
  - `System.DateTime` casts are supported.
  - Concrete encodeable arrays (e.g. `Argument[]`) no longer throw.
- `ConvertTo` keeps one-element and empty arrays as arrays and checks
the conversion is legal for empty arrays.
- `CastFrom(object[])`, `CastFrom(Enum[])` and the obsolete
`Variant(object)` constructors keep the array shape (previously a
one-element array collapsed to a scalar and an empty one to Null).
- `TypeInfo.Unknown` / `Variant.Null.TypeInfo` report `ValueRanks.Any`.
- **Equality and hashing contracts fixed:**
- Types: Matrix/MatrixOf, typed-null Variants, NodeId (the namespace
index is now hashed), ExpandedNodeId, LocalizedText, DiagnosticInfo,
StructureDefinition/EnumDefinition, CertificateIdentifier, UserIdentity
and DateTimeUtc.
  - `MatrixOf.Dimensions` returns a copy.
  - Decimal equality and hashing now run in bounded time.
- **Localized text:** a LocalizedText with locale `"mul"` and no usable
translations, or null client locale ids, no longer throw.

### NodeSet, address space and utilities
- **NodeSet export/import:**
- Server index 0 is the local server on import and export, so local ids
are no longer written as `svr=65535`.
- `StructureDefinition.BaseDataType` is filled from the supertype on
import.
- **`NodeTable.Import`:** replaces duplicates, keeping the references
neighbouring nodes hold. `TypeTable.Add` detaches a re-parented type
from its old supertype, and `Node.Clone` gets its own ReferenceTable.
- **`StringTable` / `NamespaceTable`:** capped at UInt16 indexes
(`BadEncodingLimitsExceeded`) for Append and Update, instead of silently
wrapping.
- **Small fixes:**
  - `UnsecureRandom.Shared` is seeded randomly.
  - The peer's RetryAfterMs is clamped to one day.
- The `CancelAsync` and `string.GetHashCode(StringComparison)` polyfills
match BCL behaviour.
  - `CoreUtils.IsEqual` compares strings ordinally.

### Schema generation and ComplexTypeSystem
- **JSON schema:** describes the 1.05 layout with the compact/verbose
null rules, and now validates real `JsonEncoder` output.
- **XSD:**
- `EncodingMask` (`xs:unsignedLong`) only for
StructureWithOptionalFields.
  - Guid and StatusCode are complex types.
  - Standard types are in the `Types.xsd` namespace.
- **Structure-backed OptionSets** are described as `{Value, ValidBits}`.
New `UaTypeDescription.IsStructureOptionSet` property and constructor
overload.
- **ComplexTypeSystem:**
- A malformed type in a server dictionary is skipped per type instead of
discarding the whole dictionary.
  - Cyclic supertype walks are bounded.
  - Declared definitions are no longer changed.
  - A dictionary validator is kept only if the dictionary parsed.

### Certificates and trust lists (Part 4 6.1.3, Part 12)
- **Revocation:**
- Checked for issuers supplied in the peer's chain and for
inline-trusted issuers, against the CRLs in the trusted and issuer
stores.
  - A store without CRL support no longer hides "revocation unknown".
- `RejectUnknownRevocationStatus` is honoured for inline-trusted issuers
unless the entry sets `SuppressRevocationStatusUnknown`. This changes
the strict case of `ExplicitIssuerChainIsHonoredWithoutStorePathAsync`.
- **Basic constraints / path length** violations fail with
`Bad_CertificateInvalid`.
- **`TreatAsInvalid`:**
- Now yields `Bad_CertificateUntrusted`, which can't be suppressed or
auto-accepted.
- It is honoured on individual trust-list entries even when the
certificate is also in a store.
- **`EccEncryptedSecret`:** enforces the result of validating the sender
certificate.
- **`TrustListTransaction`:**
- Routes each CRL to the store holding its issuer
(TrustedCrls/IssuerCrls).
- Validates staged CRLs, including the store's CRL support, before
changing anything.
  - Applies certificates before CRLs.
- **Other:**
- One failing `CertificateChanges` observer no longer stops the others.
- `InMemory:` stores share their contents per path, and
`InMemoryStoreProvider` is `IDisposable`.

### WoT (`Opc.Ua.Types/Wot`)
- **Projection security:** projection security schemes whose qualified
names collide are refused. New diagnostic
`WotDiagnosticCode.ProjectionSecurityConflict = 6063`.
- **Input handling:**
- Invalid Unicode (lone surrogates, bad UTF-8) is rejected as
`JsonException`.
  - External-schema results are keyed per affordance.
  - A duplicate security-floor member is rejected.
  - `nsu=` ids are percent-encoded (Part 6 5.1.12).
  - The configured JSON depth is honoured on re-parse.
  - An oversized document is reported before it is parsed back.
- **Performance and limits:**
- Subtype-graph validation, `componentOf` linking and the namespace
table are now linear.
  - Namespaces are capped at UInt16.

### New or changed public API
- `ICertificateValidatorProvider` (server)
- `SecurityPolicyInfo.GetUserTokenSignatureData(...,
MessageSecurityMode)` overload
- `XmlDecoder.TreatWhitespaceOnlyStringsAsEmpty`
- `UaTypeDescription.IsStructureOptionSet` plus a constructor overload
- `WotDiagnosticCode.ProjectionSecurityConflict`
- `StatusCodes.BadIndexRangeDataMismatch` in the `Opc.Ua.Types` list
- `InMemoryStoreProvider : IDisposable`

## Not changed / open
- **Deferred:**
- S1-14 (BSD OptionSet size needs the base type in `UaTypeDescription`,
and BSD is deprecated).
- C2-7 (restoring the own-certificate trust exemption would accept
certificates the spec says to reject; removing the dead code is a
separate decision).
- A1-4 (config values keep surrounding whitespace, as the pre-April-2026
loader did).
  - H-2 (JsonDecoder has no real nesting counter).
- A2-3 (the StringTable cap also applies to diagnostics tables; default
`MaxArrayLength` already bounds them).
- **Needs a product decision:**
- The general `ConvertToString` writes a QualifiedName as `ns:Name`,
while Table 121 drops the namespace.
  - Short (< 32 byte) client nonces are accepted on None channels.

## Verification
- Every commit added a regression test that fails without its fix, where
practical.
- Before the rebases, with all audit and review fixes (before the `Spec:
#…` commits): 0 build warnings and 16 test suites green on net10.0. The
suites: Types, Core, Core.Encoders, Core.Security, Core.Schema, Client,
Client.ComplexTypes, Server, Sessions, Gds, History, Features,
Redundancy.Server, PubSub, WotCon and WotCon.Bindings. Core hit its
known intermittent end-of-run hang once, and reruns were clean.
- The `Spec: #…` fixes were verified per group on their own suites.
Those include PubSub.Udp, Redundancy.PubSub, PubSub.Server,
Configuration and SourceGeneration.Core.
- **Not yet built or tested on this base.** It was rebased onto master
(resolving overlaps with #4522, #4540 and #4564) and then onto #4569.
This PR stays a draft until the full run on this base is green.

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
 2026-09-29 11:14:15 +00:00
Cristi Pogacean 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 Schier 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 Schier 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
romanett 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
romanett 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
romanett 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 Schier 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 Schier 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 Schier 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 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 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00