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

11 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 d98eb6b28c Fix server session defects found by a spec-checked review of CreateSession, ActivateSession, session lifecycle and continuation points (#4632) 
# Description

Fixes defects found by a spec-checked review of the server session
implementation: CreateSession, ActivateSession, the session lifecycle
(per-request validation, CloseSession, Cancel, timeout, shutdown) and
session continuation points. Each finding was checked against OPC
10000-4 (§5.7, §7.9, §7.32), OPC 10000-5 (session diagnostics and audit
events) and OPC 10000-6 §6.8.2.

There is one commit per finding, and each commit message cites the spec
section. A max-effort code review of the fixes then found 15 more
issues, mostly regressions the first round introduced; they are fixed in
the `revfix` merges.

## What changed

**CreateSession**
- At MaxSessionCount the oldest session that was never activated is
closed to make room (§5.7.2.1). Sessions that are activated, already
closing, or have an activation in flight are never evicted. The victim
is claimed atomically, so a late activation commit on it is rejected.
- `requestedSessionTimeout` NaN is revised. A malformed `endpointUrl` no
longer fails the call with Bad_UnexpectedError. `serverUri` is ignored,
as §5.7.2.2 requires.
- Unnamed sessions get distinct names.
- Session diagnostics now report `MaxResponseMessageSize` and
`ServerUri`, and `SessionSecurityDiagnostics.Encoding` uses the
specified names ("UA Binary", "XML", "JSON").
- AuditCreateSessionEvent carries SecureChannelId and the request
certificate, also on failure.
- Every rejected CreateSession is counted, and only once. A rejection
during server startup no longer turns into Bad_UnexpectedError.
- If an `OnApplicationCertificateError` override accepts an error, the
certificate is now kept and the remaining checks still run. A
certificate accepted this way does not grant TrustedApplication or
application-based roles.

**ActivateSession**
- EphemeralKey handling follows OPC 10000-6 §6.8.2:
- A key used to decrypt a UserName or IssuedToken secret is retired and
replaced in the same commit.
  - A key that was not used is kept, and nothing is returned.
- ECDHPolicyUri is honoured; an unsupported policy gets
Bad_SecurityPolicyRejected.
  - Request parameters are no longer echoed back.
- After the activation commits, a failure or cancellation no longer
faults the response, which would hide the new server nonce. Post-commit
callbacks run uncancellably.
- An activation that overlaps a session close or timeout fails with
Bad_SessionClosed instead of returning Good.
- The PolicyId of an AnonymousIdentityToken is validated.
- Status codes now follow §5.7.3.3:
- Bad_ApplicationSignatureInvalid for a mismatched client-signature
algorithm
- Bad_UserSignatureInvalid for a mismatched X509 user-token signature
algorithm
- Bad_IdentityTokenInvalid for an undecodable token or an unknown policy
- `ClientUserIdOfSession` keeps the user of the first activation.

**Lifecycle and requests**
- Requests on an expired or closing session are rejected with
Bad_SessionClosed (§5.7.2.1). A CloseSession on an expired session that
is not yet closing is still processed as the client's close and honours
`deleteSubscriptions`.
- Closing a session aborts its outstanding requests with
Bad_SessionClosed. A request that registers after that abort pass is
rejected too.
- Cancel completes the matching requests with
Bad_RequestCancelledByClient (§5.7.5.1). It also reaches requests still
waiting in the request queue, but only when both timestamps prove the
request was sent earlier.
- There is one AuditCancelEvent per Cancel call, carrying the client's
audit entry id and user.
- Every close path goes through a single close claim, so
SessionAbortCount, SessionTimeoutCount and the "Session/Terminated" and
"Session/Timeout" audits are each counted once.
- Teardown that started cannot be cancelled. Requests rejected by
session validation are counted.

**Continuation points**
- HistoryRead returns Bad_NoContinuationPoints instead of evicting
points of the same request (§7.9). Only the request that saved a point
pins it, so points from prior requests can still be freed.
- Points saved by a HistoryRead that faults are released.
- Session-less Browse and BrowseNext no longer dereference a null
session.

## Behaviour changes reviewers should know about
- At the session cap, CreateSession evicts a non-activated session
instead of returning Bad_TooManySessions. The eviction is counted in
SessionAbortCount.
- Clients see Bad_SessionClosed for requests on expired or closing
sessions, and Bad_RequestCancelledByClient for cancelled requests.
- `ActivateSessionProcessAdditionalParameters` is now called on every
successful activation, with or without an AdditionalHeader.
- If an `OnApplicationCertificateError` override returns instead of
throwing, CreateSession now accepts the certificate.
- Timeout and termination audits are now raised after the session
teardown.
- The only public API changes are additive overloads in `AuditEvents`
and `RequestManager`.

**Known partial fixes:**
- The session's `maxResponseMessageSize` is reported but not enforced;
only the channel limit applies.
- With overlapping ActivateSession calls on one session, an earlier
response can still carry a key that a later activation has already
replaced. Closing that fully needs a new `ISession` member.

## Related Issues

- The Redundancy.Server session-mirroring findings from the same review
are tracked separately in #4627.

## Tests
- Regression tests cover each finding. The new files are:
  - `CreateSessionSpecRegressionTests`
  - `ActivateSessionValidationRegressionTests`
  - `StandardServerActivateSessionRegressionTests`
  - `SessionLifecycleRegressionTests`
  - `SessionRequestAdmissionRegressionTests`
  - `HistoryReadContinuationDispatchTests`
  
  Existing test classes were extended too.
- Local results on net10.0:
  - `Opc.Ua.Server.Tests`: 7883 passed / 0 failed / 5 skipped
- `Opc.Ua.Sessions.Tests`: 790 passed / 0 failed / 379 skipped (same as
master)
- net48 Release build of `Opc.Ua.Server.Tests`: 0 errors / 0 warnings.

## 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.
- [ ] 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-03 05:42:01 +02:00
romanett a0d569d756 Fix Server and GDS audit findings, review findings and spec-conformance gaps (#4571) 
## Summary

Fixes the findings of a bug audit of `Opc.Ua.Server`,
`Opc.Ua.Gds.Server` and `Opc.Ua.Gds.Client`. Rebased on master after
#4516.

What went in:
- **Audit:** 93 findings, each checked against the OPC UA specification.
- **Code review of those fixes:** 35 further findings.
- **Spec grounding of every fix commit:** 18 conformance gaps, now
closed.

Commits are one per finding, named `Fix <ID>`, `Fix review <ID>` or `Fix
spec gap <name>`. The IDs refer to the audit report (S = Server areas,
G1 = GDS server, G2 = GDS client).

Validation:
- Each finding was traced in code and checked against OPC 10000 (parts
3, 4, 5, 9, 11, 12, 13, 18, 20).
- Every fix commit was checked against the spec text again afterwards: 0
non-conformant, remaining gaps fixed.
- Regression tests were added for most fixes.
- Suites run on net10.0 before the rebase: Server, GDS, History,
Configuration, Features, Core.Security, DI, Redundancy, LDS,
InformationModel, PubSub.Server, Client, Sessions and the three
Subscriptions suites, all green. The same suites on the rebased branch
are still running; results will be posted as a comment.

## Behaviour and user-facing changes

### Secure defaults
- **`Historize()` / `HistorianBuilder`** grant HistoryRead only. History
writes need an explicit `historyAccessLevel: HistoryRead |
HistoryWrite`.
- **File-system mount:**
- Anonymous sessions can no longer create, delete, move or open files
for writing (`Bad_UserAccessDenied`).
  - UserWritable and UserExecutable now reflect the user.
- `AllowAnonymousWrite` restores the old behaviour; the ReferenceServer
sample turns it on.
- **Default PKI root** for the hosted server, hosted GDS, hosted LDS and
`OpcUaApplicationConfigurationProvider`:
- Moved from the shared temp directory to `{LocalApplicationData}/OPC
Foundation/{ApplicationName}/pki`, and startup fails if no per-user
directory exists.
- When certificate stores of an earlier version still exist under temp,
a warning names both paths. Existing deployments that relied on the
default get a new application certificate (and the hosted GDS a new CA)
unless `PkiRoot` is set to the old path.
- **GDS:**
  - `GdsServerOptions.AutoApprove` defaults to `false`.
- The default KeyCredential store grants no roles unless
`AuthorizeRoles` allows them.
- Certificate requests (StartSigningRequest, StartNewKeyPairRequest,
FinishRequest) need an encrypted channel.
- **Hosted server anonymous logins:**
- Anonymous is rejected (`Bad_IdentityTokenRejected`) when the default
authenticator options disable it.
- A custom `AddIdentityAuthenticator<T>()` on its own no longer disables
anonymous.
- With anonymous disabled and no configured token policies, the endpoint
advertises the authenticators' token types instead of Anonymous.
- **Runtime-added roles:** their properties and methods need
SecurityAdmin over an encrypted channel, like the well-known roles.
- **Disabled users:** users disabled through Part 18 UserManagement can
no longer log in through `RegisterDefaultAuthenticators(IUserDatabase)`.

### GDS server (Part 12)
- **Certificate requests:**
- FinishRequest / ReadRequest with another application's RequestId
returns `Bad_InvalidArgument`.
  - Every Start*Request returns a new RequestId.
- A CSR without the ApplicationUri is rejected
(`Bad_CertificateUriInvalid`).
- An abstract or unsupported certificate type is rejected at Start
(`Bad_InvalidArgument`).
- **Private-key password:** no longer persisted in the JSON applications
database. A password saved by an earlier version is read into memory
once and dropped on the next save.
- **CheckRevocationStatus:** validates offline against the GDS's own
trust lists and CRLs. It returns Good for good GDS-issued certificates
and `Bad_CertificateInvalid` for input that is not a certificate.
- **Certificate-group trust lists:**
- Writes need CertificateAuthorityAdmin; SecurityAdmin and application
admins are read-only.
  - Writes are validated with the Part 4 certificate validation.
- **AuthorizationService:**
- The legacy RequestAccessToken applies `AccessControl` and issues the
token for the session identity.
- StartRequestToken passes the session identity to every identity-aware
provider.
  - Unknown ResourceIds return `Bad_NotFound`.
- Expired token requests and refresh tokens are pruned, with at most 10
000 pending requests (`Bad_TooManyOperations`).
- **KeyCredential:** requests with PublicKey + SecurityPolicyUri are
rejected with `Bad_SecurityPolicyRejected` (secret encryption is not
implemented); a PublicKey without SecurityPolicyUri is rejected with
`Bad_InvalidArgument`.
- **Audit events:** access-token audit events no longer contain the
password or token data.
- **RegisterTickets / UnregisterTickets:** need the RegistrarAdmin role
when called from a session.
- **Managed application nodes:** moved from namespace 0 to their own
namespace (`ManagedApplicationsNamespaceUri`); the NodeId is the
ApplicationUri, and the mandatory child properties exist.
- **Hosted GDS:**
- New `GdsServerOptions.CertificateGroups`
(`GdsCertificateGroupOptions`), bindable from configuration.
  - The mandatory DefaultApplicationGroup is always present.
  - The default CA subject uses the resolved application name.
- **JSON applications database:**
- A corrupt file throws instead of being silently replaced by an empty
database.
- Saves are atomic and write in place on bind mounts; on net8+ builds
they also follow symlinks and keep the Unix file mode.
  - Issued certificates and trust-list links survive a reload.

### GDS client
- **Trust-list download:**
  - Reads until an empty chunk (Part 20 short reads).
- A size-limit rejection of the final end-of-file read, as sent by
released servers, counts as end of file.
- **Default certificate group:** a null group means
DefaultApplicationGroup, and vendor groups in other namespaces are no
longer mistaken for well-known groups.
- **Status codes and errors:**
  - Missing directory proxies report `Bad_ServerNotConnected`.
- Short or wrongly typed method outputs throw `BadUnexpectedError` /
`BadTypeMismatch`.
- **GdsAccessTokenProvider:** no longer caches a failed client task, and
one caller's cancellation no longer fails other callers.

### Trust lists, push and ConfigurationFile (Part 12 / Part 20)
- **AddCertificate:**
  - `IsTrustedCertificate=false` returns `Bad_CertificateInvalid`.
- Added certificates are validated (Part 4), and a certificate revoked
by a current CRL returns `Bad_CertificateRevoked`.
- **CloseAndUpdate:** validates the uploaded list against its own
certificates and CRLs, not the stores it replaces. Validation failures
are no longer recorded in the server's Rejected store.
- **RemoveCertificate:** removing a trusted or issuer CA that other
certificates still need returns `Bad_CertificateChainIncomplete`.
- **Handles:**
  - Several read handles may be open at once.
- Any open while a write handle exists returns `Bad_NotReadable` /
`Bad_NotWritable`.
  - A write open while any handle is open returns `Bad_NotWritable`.
  - Handles of closed sessions are released.
  - Idle handles close after ActivityTimeout (default 60 s).
- **Status codes:**
- Read clamps to the remaining bytes and no longer enforces the size
limit.
  - Oversized Write / AddCertificate returns `Bad_RequestTooLarge`.
  - Unsupported open modes return `Bad_NotSupported`.
- **ConfigurationFile.CloseAndUpdate:**
  - Empty Targets return `Bad_InvalidArgument`.
- The ConfigurationUpdatedAuditEvent is raised with the configuration
owner as source.
- With the new `IApplicationConfigurationFileTargetProvider`, providers
receive the Targets, a bad target returns Uncertain with per-target
results, and session-interrupting updates are applied after
RestartDelayTime.
- **Default namespace permissions:** namespaces registered after the
first permission lookup now get their NamespaceMetadata default
RolePermissions and AccessRestrictions (no stale negative cache).

### Sessions, identity and roles (Part 18)
- **Session timeouts:** a failure closing one timed-out session no
longer stops the session-timeout monitor.
- **ChangePassword:** an empty old password returns
`Bad_IdentityTokenInvalid`.
- **Removing or disabling a user:** closes only that user's
UserName-token sessions.
- **Role configuration changes:** roles granted by an `ImpersonateUser`
mapping are kept.
- **X509Subject rules:** a certificate subject value containing a quote
never matches an X509Subject rule.
- **`ImpersonateUser` callback:** no longer runs under the global
session lock.
- **Session diagnostics:** LocaleIds are kept as the client sent them,
while null LocaleIds are ignored for translation.

### Subscriptions and monitored items (Part 4)
- **Acknowledgements:** a failing acknowledgement returns a per-entry
Bad status instead of failing the whole Publish.
- **`DiscardedMessageCount`:** counts messages dropped from the
retransmission queue and unsent overflow messages.
- **Durable subscriptions:** transferring a restored anonymous
subscription works (no NullReferenceException).
- **Transferred subscriptions:** sampling and permission checks use the
item's current owner session.
- **Access denied:** a monitored item whose user may no longer read the
attribute gets a `Bad_UserAccessDenied` notification instead of going
silent. Non-Value attributes need Browse permission (RolePermissions
needs ReadRolePermissions), and User* attributes are evaluated per
subscriber.
- **ModifyMonitoredItems:** a SamplingInterval of -1 uses the
subscription's publishing interval.
- **AggregateFilter priming:** needs HistoryRead / ReadHistory
permission and is bounded (100 000 values, bounded page size).
- **Resource leak:** event monitored items no longer leak a consumer
task per create/delete cycle.

### Node management services (Part 3 / Part 4)
- **Reference types:** AddNodes, AddReferences, DeleteReferences and
Browse reject ids that are not ReferenceTypes with
`Bad_ReferenceTypeIdInvalid`. AddNodes, AddReferences and
DeleteReferences also reject abstract ReferenceTypes; Browse still
accepts them (e.g. `HierarchicalReferences` with includeSubtypes).
- **Concurrent AddNodes:** duplicate BrowseNames under one parent get
`Bad_BrowseNameDuplicated`.
- **TranslateBrowsePathsToNodeIds:**
- Requires Browse permission on intermediate nodes and final targets
(RolePermissions and AccessRestrictions).
  - A long RelativePath no longer rejects the whole request.
- **SemanticChangeEvent:**
- Raised once per actual change of a semantic property, whether or not
it is monitored.
  - Also raised for properties with the SemanticChange access-level bit.
  - Reports the owning node's TypeDefinition as AffectedType.
- **Localized diagnostics:** operation-level text is translated when
OperationLocalizedText is requested.

### History and aggregates (Part 11 / Part 13)
- **DeleteRawModified:** an unspecified or reversed range returns
`Bad_InvalidTimestampArgument`, and StartTime == EndTime deletes the
value at StartTime for every provider.
- **ReadAtTime fallback:** reads only the values around each requested
time. A time whose bound is not found within the search limit returns
`Bad_BoundNotSupported` instead of failing the request.
- **ReadProcessed:** a ProcessingInterval shorter than one tick returns
`Bad_AggregateInvalidInputs`.
- **Delta / DeltaBounds:** widen to the next larger signed type when the
result does not fit the source type (e.g. a decreasing unsigned counter,
or an Int16 overflow).
- **Range / Range2:** return the source data type.

### Alarms and fluent API (Part 9)
- **Suppression:** an alarm stays suppressed while any suppression group
is active, and re-evaluation without a state change does not move
TransitionTime.
- **`MonitorVariable(variable)`:** sets SourceNode, SourceName and
InputNode to the variable and makes it a ConditionSource (HasCondition
to the alarm, HasEventSource from the object).
- **Fluent input/output arguments:** InputArguments / OutputArguments
added through the fluent API to a registered method are readable.
- **Root notifiers:** a failed root-notifier registration no longer
leaves an orphaned event worker.
- **State-machine handlers:** exceptions are logged at Error.

### File system (Part 20)
- **Delete / MoveOrCopy:**
  - Only act on direct children of the directory.
- A NodeId whose kind does not match the entry returns `Bad_NotFound` (a
file NodeId can no longer delete a directory).
- Copying a directory into its own subtree returns
`Bad_InvalidArgument`.
- **Locking:**
- An open file, or a directory containing one, cannot be deleted or
moved (`Bad_InvalidState`).
  - The check is atomic with opens.
- While a Delete or Move runs, read opens get `Bad_NotReadable` and
write opens `Bad_InvalidState`.
- **Open:**
- Reserved mode bits or EraseExisting without Write return
`Bad_InvalidArgument`.
  - Read+Write returns `Bad_NotSupported`.
  - Read while a writer is open returns `Bad_NotReadable`.
- **Client `FileSystemErrors`:** maps `Bad_NotReadable` to
`IOException`.
- **RuntimeNodeSet:** well-formed NodeSets with a stray `Model` element,
or two models in one file where one requires the other, now load.

### Server core, hosting and lifecycle
- **`SessionEstablishmentQueueLimit` > 0:**
CreateSession/ActivateSession now queue for a permit. The permit is
acquired before the request is registered, and a waiting handshake
releases its worker through the request-parking policy.
- **Node manager lifecycle:**
  - Add, reload and remove no longer wait for parked Publish requests.
- A Migrate reload racing with CreateMonitoredItems no longer blocks
later lifecycle operations.
  - Late items on retired or removed managers get `Bad_NodeIdUnknown`.
- **HistoryUpdate:** a batch mixing data and event details is bounded by
both limits.
- **Server stop/start:** stopping the server disposes the configuration
watcher and certificate subscription; starting recreates them.
- **Monitored-item queue factory:** a factory supplied by the caller or
the DI container is no longer disposed by the server.
- **Audit events:** each certificate validation error raises its own
AuditCertificate event. A failed OpenSecureChannel without a service
result is audited as failed.

## New public API (additive)
- `IQueuedSessionEstablishmentLimiter`
- `StandardServer.OwnsMonitoredItemQueueFactory` (protected virtual)
- `DefaultPkiRoot.Get` (Opc.Ua.Configuration)
- `TrustList.SetCertificateValidation`
- `NamespaceMetadataRegistry.Invalidate`
- `IApplicationConfigurationFileTargetProvider` and
`ApplicationConfigurationUpdatePlan`
- `GdsServerOptions.CertificateGroups` / `GdsCertificateGroupOptions`
- `InMemoryKeyCredentialRequestStore.AuthorizeRoles`
-
`DefaultManagedApplicationsNodeManager.ManagedApplicationsNamespaceUri`
- `AuthorizationServiceManager.RequestAccessTokenAsync(...,
IUserIdentity?, ...)`
- `AllowAnonymousWrite` on the file-system node manager and its factory
- `IFileSystemHost.TryBeginMutation` / `EndMutation`, replacing
`HasOpenHandles`

## Not fixed / known limitations
- **KeyCredential:** secret encryption with the client's PublicKey
(requests needing it are rejected).
- **CSR SAN restriction:** restricting subject / SAN values copied from
a CSR (CA policy).
- **Ticket signatures:** ticket signature verification (needs a
trusted-signer configuration).
- **FileType Read+Write open:** needs a read/write stream contract in
`IFileSystemProvider`.
- **Custom `IUserDatabase` implementations:** the disabled-user check
still scans all users per login. The built-in stores do a single lookup.
- **64-bit Delta precision:** limited by the calculator's double
arithmetic.

## Tests with changed expectations
Existing tests were updated where they asserted the old behaviour:
- File-system status codes
- Trust-list handle eviction
- `DiscardedMessageCount`
- Localized diagnostics bits
- Delta on unsigned sources
- SourceNode of fluent alarms
- ReferenceServer transfer permission test (now expects
`Bad_UserAccessDenied` notifications)
- `UserDeactivationShutdownReviewTests` (the revoked session is a
UserName login)

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-09-29 07:26:05 +02:00
Copilot ca9cfcc933 Inject ISecurityPolicyRegistry into Session and secure channel (#4314) 
- [x] `ChannelQuotas` + `UaSCUaBinaryChannel` carry an
`ISecurityPolicyRegistry` (falling back to `SecurityPolicies.Default`)
- [x] Transport channel/listener settings carry the registry and feed
every `ChannelQuotas` construction site
- [x] `ClientChannelManager` and `ServerBase` source the registry
- [x] `Session`, `DefaultSessionFactory`, `ChannelManagerSessionFactory`
and `ManagedSession` carry it
- [x] Identity token handler injection points made load-bearing
(`UserIdentity`, `AsTokenHandler`, default identity providers,
`IdentitySelectionContext`)
- [x] `SessionSecurityPolicyHelper`, `ClientIdentityProviderExtensions`,
`CoreClientUtils`, `OfflineSecureChannel`,
`OpcUaSecurityKeyServiceClient`
- [x] Client and server DI wiring resolve `ISecurityPolicyRegistry`
- [x] `IPubSubSecurityPolicyRegistry` registered in DI and threaded
through the PubSub application/SKS paths
- [x] Channel asymmetric crypto (padding, sign, verify, ECC
classification) resolves from the negotiated policy instead of a
hard-coded policy URI switch
- [x] Unit tests for the registry carriers plus an integration test that
negotiates a policy registered through `AddSecurityPolicy` over a real
loopback handshake
- [x] `docs/CryptoProvider.md` documents how the registry reaches the
channel and the session
- [x] Full solution build (`UA.slnx`, net10.0) and full
`Opc.Ua.Core.Tests` run (4351 passed) are green
- [ ] Broader integration suites (`Opc.Ua.Sessions.Tests`,
`Opc.Ua.Server.Tests`, `Opc.Ua.PubSub.Tests`) were still running when
time ran out

- [x] Review feedback addressed: preserved original CLR signatures,
routed factory channel creation through public registry-aware overloads,
failed closed for unsupported asymmetric encryption, and added a
provider contract for custom session factories
- [x] Focused review-feedback tests pass (28 tests across Client, Core,
and Core.Diagnostics)

- [x] Security review: hardened SKS custom-policy admission against weak
algorithms and incoherent RSA/ECC key-exchange metadata; final
specialist review found no remaining vulnerabilities

<!-- START COPILOT CODING AGENT SUFFIX -->

- Fixes #4250

---------

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 <mschier@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: romanett <romanett98@gmail.com>
 2026-08-27 05:57:17 +02: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 9375a2431b Retire the IServerInternal bind mutators and freeze the bind phase (#4197) 
## Stacked on #4193

Base is `marcschier/server-context`, which is itself based on #4188.
**Review #4188 and
#4193 first.** GitHub retargets each base automatically as the stack
merges.

Closes #4184.

## What this does

`IServerInternal` published twelve `Set*` binding methods plus
`CreateServerObjectAsync`,
so any holder of the interface could rewire a running server. This takes
them off the
interface and makes a late bind fail loudly.

## The audit changed the shape of the work

The plan this came from said "design a frozen bind phase". **One already
exists.**
`StandardServer` has a seventeen-method `Create*` factory seam, and
`DependencyInjectionStandardServer` overrides seven of those to resolve
from the container:

```csharp
protected override IRoleManager CreateRoleManager(IServerInternal server, ApplicationConfiguration configuration)
    => m_services.GetService<IRoleManager>() ?? base.CreateRoleManager(server, configuration);
```

`StandardServer` then carries each factory result into the datastore:

```csharp
ServerInternal.SetRoleManager(CreateRoleManager(m_serverInternal, configuration));
```

So an integrator already had two supported routes — override the
virtual, or register in
DI. `Set*` on the interface was a **third route that bypassed the
ordering**, and the only
one that let a caller rebind a running server.

The job was therefore to stop publishing plumbing, not to build a phase.

## Measurement

Every call site of the twelve mutators plus `CreateServerObjectAsync`:

| Area | Count | Where |
|---|---:|---|
| `Opc.Ua.Server` | **11** | `StandardServer.cs:3729–3844`, one
contiguous block |
| samples | **1** | `ReferenceServer.cs:219`, inside a
`CreateMasterNodeManager` override |
| tests | 20 | `ServerInternalDataTests` (18), `SubscriptionTests` (2) —
**all on the concrete `ServerInternalData`** |

`SetIdentityRegistry` had **zero** call sites anywhere.

The earlier estimate for this work was 268 files. That number counted
files *referencing*
`IServerInternal`, which this change never touches: the interface
survives and only the
mutators move off it.

## Commits

### `e719a88a` Add the missing `CreateUserManagement` factory seam

The one gap, and the reason this had to land first. User management was
the only subsystem
with **no factory seam** — the only way to populate
`IServerInternal.UserManagement` was
`SetUserManagement`, which is what `ReferenceServer` called from inside
a
`CreateMasterNodeManager` override. Removing the mutator without a
replacement would have
broken that scenario with no migration path.

The DI path at `OpcUaServerBuilderExtensions.cs:1178` builds a
`UserNamePasswordAuthenticator` from a registered `IUserManagement` — a
different thing
entirely that never reaches the Part 18 §5 model on the address space.

Purely additive. `ReferenceServer` moves onto the override.

The test covers the **override** seam rather than the DI seam
deliberately: registering an
`IUserManagement` in the container also switches on the
username/password authenticator, so
a DI-based test exercises two unrelated paths and fails for reasons
unrelated to binding.

### `cbcfda47` Remove `SetIdentityRegistry`

Zero call sites, despite a doc comment advertising it as the integrator
seam. The supported
route has always been
`ServerIdentityRegistryExtensions.RegisterDefaultAuthenticators`,
which *adds* authenticators to the default registry rather than
replacing it.
`IdentityRegistry` is read five times outside `Opc.Ua.Server`, so the
default is what every
consumer already uses.

**The analyzer confirmed the reading**: with the setter gone, `RCS1170`
fired on the
property — it is provably never reassigned after construction, so it is
now read-only.

### `ea0937a2` Take the bind mutators off `IServerInternal`

The mutators stay `public` on `ServerInternalData`, which owns the
state. Only the interface
loses them. The eleven call sites switch from the `ServerInternal`
property to the
`m_serverInternal` field, already typed `ServerInternalData?` — the same
one-token change
`GetStatus` and `EnterState` took in #4193.

**No test changed.** All twenty test call sites already used the
concrete type, which is the
clearest evidence these were never interface-level API.

`CS1574` found six doc comments across `IRoleManager`, `RoleManager`,
`IUserManagement`,
`UserManagement`, `IServerInternal` and `SessionManager` that pointed
integrators at the
mutators. They now point at the factory seam.

### `f3c4b9d3` Refuse to bind after the server has started

Each bind goes through a guard; `StandardServer` closes the phase once
the session manager
is in place. A late `Set*` throws
`ServiceResultException(BadInvalidState)` **naming the
operation** and pointing at the factory seam — otherwise the failure is
indistinguishable
from a configuration problem.

Rebinding *during* startup stays legal, which is what let both audit
hazards resolve with no
behaviour change:

- **`SetNodeManager` also binds three more managers**
(`DiagnosticsNodeManager`,
`ConfigurationNodeManager`, `CoreNodeManager`). Intentional — they are
reached through the
master node manager and have no independent lifetime — and now
documented rather than
  silent.
- **`SetSessionManager` unhooks the previous manager's handlers**, which
reads as
written-for-repeat-calls. Nothing rebinds it, but the freeze does not
make the path dead
either, since binding twice during startup is still allowed. Left as-is;
removing it is a
  separate question.

## Tests

Four new tests on the freeze: the refusal itself across four
representative mutators, that
the message names the operation, that rebinding stays legal until the
phase closes, and that
closing twice is harmless. Plus one hosted-server test for the
`CreateUserManagement` seam.

| Suite | net10.0 | net48 |
|---|---|---|
| `Opc.Ua.Server.Tests` | 4033 passed, 1 failed | 230 passed, 1 failed
(filtered) |
| `Opc.Ua.Redundancy.Server.Tests` | 573 passed | — |
| `Opc.Ua.Redundancy.Kubernetes.Tests` | 97 passed | n/a (net8/net10
only) |

The single failure is
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`, a
local certificate-store problem verified to reproduce unchanged at HEAD
with these changes
stashed. Not caused by this PR.

## Docs

`docs/MigrationGuide.md` gains a table mapping every removed mutator to
the `Create*`
override and the DI registration that replace it, plus the
`SetIdentityRegistry` removal
with its rationale.

## Net effect across #4193 and this PR

`IServerInternal` goes from ~460 lines to **~200**. It no longer
publishes locks, dead
members, or the ability to rewire a running server.

## Reviewer questions

- `CompleteBindPhase()` is public on `ServerInternalData` but
deliberately **not** on
`IServerInternal` — only the server owning the startup sequence should
decide when binding
  is over. Reasonable, or should it be `internal`?
- `SetSessionManager`'s unhook path is now unreachable in practice but
still legal during
startup. Leave it, or remove it and assert single-bind for that one
subsystem?

---------

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
 2026-08-11 00:27:45 +02:00
Marc Schier 4e0b82af5f Add IServerContext and move server startup tasks onto it (#4193) 
## Stacked on #4188

Base is `marcschier/server-seam-analysis`, not `master`.
`IServerInternal.cs` and
`ServerInternalData.cs` both diverge from master on that branch (chunk
4c removed the two
`object` locks), so branching off master would conflict on both files
*and* design
`IServerContext` against a stale interface. GitHub retargets the base to
`master`
automatically when #4188 merges. **Review #4188 first.**

Closes part of #4184.

## What this does

`IServerInternal` is a service locator: 56 members, 30-odd of which hand
out one of 20+
subsystems. A component that only needs to observe the server and act on
it has to take the
whole thing, and no signature tells a reader which parts a component
actually touches.

This adds `IServerContext` — the ambient view of a running server — and
moves
`IServerStartupTask` onto it.

```csharp
public interface IServerContext
{
    ServerState CurrentState { get; }
    ServerObjectState ServerObject { get; }
    IServiceMessageContext MessageContext { get; }
    ServerSystemContext DefaultSystemContext { get; }

    ServerSystemContext CreateSystemContext(ISession session);
    T? FindPredefinedNode<T>(NodeId nodeId) where T : NodeState;
    IEnumerable<T> FindNodeManagers<T>() where T : class;

    ValueTask ReportEventAsync(IFilterTarget e, CancellationToken ct = default);
    ValueTask CloseSessionAsync(OperationContext context, NodeId sessionId,
        bool deleteSubscriptions, CancellationToken ct = default);
    ValueTask DeleteSubscriptionAsync(uint subscriptionId, CancellationToken ct = default);

    void UpdateServerDiagnostics(Action<ServerDiagnosticsSummaryDataType> update);
}
```

Nothing on it returns a subsystem. A component that needs one takes it
as a constructor
dependency, which states the requirement instead of hiding it behind an
ambient handle.

`IServerInternal : IServerContext`, so every existing handle already
satisfies the new seam
and consumers migrate one at a time.

## The member set was measured, not guessed

Every member of `IServerInternal` was counted by read site across `src`,
`samples` and
`tests`, split by area. `Ext` is non-test production consumers, which is
what decides
whether something belongs on an ambient interface.

| Member | InServer | OtherSrc | Sample | Test | **Ext** | On
`ISystemContext`? |
|---|---:|---:|---:|---:|---:|---|
| `NamespaceUris` | 63 | 50 | 34 | 131 | **84** | yes |
| `Telemetry` | 58 | 23 | 35 | 5 | **58** | yes |
| `MessageContext` | 2 | 8 | 7 | 11 | **15** | no |
| `TypeTree` | 42 | 3 | 11 | 10 | **14** | yes, as `TypeTable` |
| `NodeManager` | 66 | 7 | 6 | 74 | **13** | no |
| `DefaultSystemContext` | 21 | 4 | 1 | 7 | **5** | no |
| `IdentityRegistry` | 1 | 4 | 1 | 1 | **5** | no |
| `SubscriptionManager` | 23 | 0 | 0 | 17 | **0** | no |
| `SessionManager` | 21 | 0 | 0 | 15 | **0** | no |
| `RequestManager` | 10 | 0 | 0 | 23 | **0** | no |
| `AggregateManager` | 12 | 0 | 0 | 2 | **0** | no |
| `ModellingRulesManager`, `ConformanceUnitsManager`,
`DiagnosticsEnabled` | 0 | 0 | 0 | 0 | **0** | no |

Two things fall out. The highest-traffic members are already carried by
`ISystemContext`,
which `ServerSystemContext` copies off the server at construction — so
most of those 278
`NamespaceUris` reads are callers reaching through the server for
something they already
hold. And the subsystems with zero external demand are exactly the ones
that should be
injected rather than published.

The check that matters: the eight `IServerStartupTask` implementations
in
`Opc.Ua.Redundancy.Server` are the heaviest real consumer of the ambient
handle, and
between them they touch **eight distinct members**. That is the shape
the interface is cut
to.

## Commits

### `e4a912eb` Remove seven unused members from `IServerInternal`

The deletion test, applied before anything is designed against the
interface.

| Removed | Evidence |
|---|---|
| `CloseSession(OperationContext, NodeId, bool)` | zero call sites;
sync-over-async; already obsolete |
| `ReadServerDiagnostics<TResult>` | zero production sites — **added one
commit earlier in #4188 for symmetry and never called** |
| `ModellingRulesManager` | property never read; `ServerInternalData`
owns the lifetime |
| `ConformanceUnitsManager` | same |
| `DiagnosticsEnabled` | façade over
`IDiagnosticsNodeManager.DiagnosticsEnabled`, which is what callers use
|
| `Status` | one site, itself inside an obsolete method |
| `ServerDiagnostics` | only reachable through the diagnostics methods
that replaced it |

`EndpointAddresses` was kept — `Opc.Ua.Di.Tests` uses it to resolve an
endpoint URL. Thin,
but real.

The only build fallout was three lines in one test assembly.
`Opc.Ua.Redundancy.Server`,
`Opc.Ua.Redundancy.Kubernetes`, `Opc.Ua.Gds.Server.Common`,
`Opc.Ua.WotCon.Server`,
`Opc.Ua.PubSub.Server`, `Opc.Ua.Di.Server`, `ConsoleReferenceServer` and
`Quickstarts.Servers` all built untouched. That is the evidence the
members were dead.

### `ffcc80b9` Add `IServerContext`

Additive. `ServerInternalData` implements it; no caller changes.

### `e8c4e15f` Hand startup tasks `IServerContext`

Nine implementations, `OpcUaServerHostedService`, and the test call
sites.

### `1b4de97a` Drop the `IServerStartupTask` migration-guide section

Review feedback. `IServerStartupTask` does not exist in `master378`, so
it is a 2.0-only
concept and nobody migrating from 1.5.378 can own an implementation of
it. The **Removed
members on `IServerInternal`** section stays - those members *are* in
master378 - and is now
self-contained.

### `2ab7d689` Merge `marcschier/server-seam-analysis`

Picks up the base branch including its merge of master. Textually clean,
no conflicts.

### `6a650f12` Migrate `RobotIntentHostStartupTask` to `IServerContext`

The merge was textually clean but **semantically broken**: master's
Robot Intent feature
(#4165) added a tenth `IServerStartupTask` implementation written
against the old
`IServerInternal` signature, so it no longer satisfied the interface.
Its body walked
`server.NodeManager.AsyncNodeManagers` and type-tested each entry for
`RobotIntentNodeManager` - exactly the pattern `FindNodeManagers<T>()`
replaces - so it
migrated the same way as the other nine. `FindNodeManagers` also
searches the synchronous
registry and de-dupes by reference, so a manager registered on both
faces still starts its
intent controller hosts exactly once.

## Three findings from migrating real consumers

**`MessageContext` is not derivable.** The design had callers use
`DefaultSystemContext.AsMessageContext()`, on the reasoning that a
message context can be
built from a system context. It can't: that conversion carries the
namespace tables and the
factory but **not** `MaxStringLength`, `MaxArrayLength` or
`MaxByteStringLength`. Four
replication paths in `Opc.Ua.Redundancy.Server` would have silently
fallen back to default
decoding limits — a component quietly accepting more than the server is
configured to. It
is a real member, and the interface documents why.

**`new`-shadowed members split the mock surface.** `IServerInternal`
first redeclared
`DefaultSystemContext` with a narrower type and `CurrentState` with a
setter. That
compiles, but a shadowed member is a *second interface slot*: production
code calling
through `IServerContext` read a slot that every existing
`Mock<IServerInternal>` leaves
unstubbed. Seventeen tests went from passing to `NullReferenceException`
**while still
compiling**. The narrower typing bought no encapsulation anyway — the
object really is a
`ServerSystemContext` and any caller can cast — so both shadows are
gone. `CurrentState` is
read-only on the ambient interface and the one site that assigns it uses
the concrete type,
as `GetStatus` already does.

**`IServiceProvider` is deliberately *not* on `OnServerStartedAsync`.**
The plan sketched
it. All nine implementations already receive their dependencies through
their constructors
from the builder extensions, so a container on the callback would have
replaced one service
locator with another.

## `FindNodeManagers<T>`

Two tasks walked `server.NodeManager.NodeManagers` and type-tested each
entry for
`ILocalAddressSpaceSource`; two more reached for a specific manager to
cast it to a
capability. Asking for the capability says what the caller wants; asking
for the registry
says how to find it.

```csharp
// before
foreach (INodeManager nodeManager in server.NodeManager.NodeManagers)
{
    if (nodeManager is not ILocalAddressSpaceSource source) { continue; }
    ...
}

// after
foreach (ILocalAddressSpaceSource source in server.FindNodeManagers<ILocalAddressSpaceSource>())
{
    ...
}
```

It searches both the asynchronous and synchronous registries:
`ToSyncNodeManager()` returns
`this` when a manager already implements `INodeManager`, so the two
lists overlap and a
capability may sit on either face.

## Tests

Mock harnesses are **wired, not stubbed** — they return the node
managers and contexts the
tests already build, so the tests keep asserting against real behaviour
rather than against
a mock that returns whatever is convenient. Four new tests cover
`CreateSystemContext`
(identity/locale propagation, null guard), the explicit
`DefaultSystemContext` identity,
and `FindPredefinedNode` before the address space is bound.

| Suite | net10.0 | net48 |
|---|---|---|
| `Opc.Ua.Redundancy.Server.Tests` | 573 passed | 573 passed |
| `Opc.Ua.Redundancy.Kubernetes.Tests` | 97 passed | n/a (net8/net10
only) |
| `Opc.Ua.Server.Tests` | 4028 passed, 1 failed | 225 passed, 1 failed
(filtered) |

The single failure is
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`,
a local certificate-store problem. Verified it reproduces unchanged at
HEAD with these
changes stashed — it is not caused by this PR.

All twelve dependent assemblies and samples build clean on both TFMs,
zero warnings.

### Re-verified after the base merge (`6a650f12`)

`Opc.Ua.Server`, `Redundancy.Server`, `Redundancy.Kubernetes`,
`Robotics.Server`,
`WotCon.Server`, `Di.Server`, `PubSub.Server`, `Gds.Server.Common` and
`XRegistry.Server` all build clean on net10.0. `Opc.Ua.Robotics.Tests`
491 passed,
`Opc.Ua.Redundancy.Server.Tests` 573 passed,
`Opc.Ua.Redundancy.Kubernetes.Tests` 97 passed.

### CI status: 119 passed, 3 failed - none caused by this PR

| Failing job | Why it is not this PR |
|---|---|
| `SourceGeneration_Core_Tests_windows` (net48) |
`GenerateAndCompileDesignFileAsync(..., CSharp11, ...)` fails with
`CS9058: Feature 'Lock object' is not available in C# 11.0` because
generated `Opc.Ua.NodeStates.g.cs` emits `System.Threading.Lock`.
**Identical failure on base #4188 (build 16737)**, which contains none
of these commits. This PR touches no source-generation file. |
| `SourceGeneration_Core_Tests_windows` (net10.0) | Same root cause;
also failing on base #4188. |
| `PubSub_Eth_Tests_windows` (net48) | Agent DNS outage:
`NuGetToolInstaller` died with `ERR:getaddrinfo ENOTFOUND
dist.nuget.org`. The job aborted before restore/build, so **the test
step never executed**. Infra flake; needs a re-run, not a code change. |

Both `SourceGeneration.Core` failures belong to the base branch and
should be fixed there
rather than papered over here.

## Correction to the recorded plan

Chunk 7 (the frozen bind phase) was recorded as the largest mechanical
footprint remaining
and the one most likely to stall. It isn't: **each of the 12 `Set*`
mutators has exactly one
call site**, in the `StandardServer` wiring, plus one sample. It is a
~13 call-site change.
The 268-file figure that produced that assessment counted files
*referencing*
`IServerInternal`, which chunk 7 never has to touch, because the
interface survives and only
the mutators move off it.

## Docs

`docs/MigrationGuide.md` gains one section: the removed
`IServerInternal` members with their
replacements, plus the note that `MessageContext`,
`DefaultSystemContext`, `CurrentState`,
`ServerObject`, `ReportEventAsync`, `CloseSessionAsync`,
`DeleteSubscriptionAsync` and
`UpdateServerDiagnostics` moved *down* to `IServerContext` and stay
reachable through
`IServerInternal` unchanged, so no call site has to move.

An earlier revision also documented the `IServerStartupTask` signature
change there. That
was dropped after review (1b4de97a4): `IServerStartupTask` does not
exist in `master378`, so
it is a 2.0-only concept and nobody migrating from 1.5.378 can own an
implementation of it.
The rationale for the ambient view - including the `AsMessageContext()`
decoding-limit trap -
lives on the `IServerContext` and `IServerStartupTask` XML docs instead,
which is where a
2.0-only concept belongs.

## Reviewer questions

- `IServerContext` is 11 members, not the 9 the design targeted.
`MessageContext` and
`FindNodeManagers<T>` were added because migrating real consumers showed
the alternatives
were wrong (silent decoding-limit loss) or a pass-through
(`NodeManager.NodeManagers` plus
  a type test at every call site). Reasonable?
- `IServerInternal : IServerContext` keeps the fat interface alive on
purpose — it is what
makes this a signature change rather than a 268-file rewrite. It only
shrinks once the DI
migration lands. The 6a deletions are unconditional, so even a full
stall leaves it seven
  members smaller.

---------

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
 2026-08-10 21:14:09 +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 e73e71184d [WotCon] Add OPC UA WoT Connectivity 1.1 with the Types, source generation, Server and xRegistry work it builds on (#4128) 
## Summary

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

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

## Types

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

## Source generation

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

## Server

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

## xRegistry

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

## WoT Connectivity

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

## New projects

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

## Constituent pull requests

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

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

Supersedes #4093.

## Follow-up

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

## Validation

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

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8
 2026-08-05 10:14:15 +02:00
Marc Schier 275c3800b5 [Server] Add live NodeManager lifecycle support (#4094) 
# Description

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

The change introduces a DI-backed `INodeManagerLifecycle` service with
direct `StandardServer` access, generation-aware registration handles,
copy-on-write routing, transactional prepare/commit/rollback, runtime
NodeSet convenience APIs, append-only namespace updates, model and
semantic change notifications, additive-compatible complex type priming,
and complete teardown behavior.

Lifecycle publication is coordinated with requests, sessions,
subscriptions, monitored items, continuation points, and shutdown so
retired managers cannot remain reachable or be disposed while still in
use. Compatible monitored items transfer to replacement generations
without a transient bad status. Removed or incompatible Nodes publish
`BadNodeIdUnknown` as required by OPC UA Part 4 section 5.8.4.1, retain
their MonitoredItem identity, and automatically recover when a
compatible Node with the same NodeId returns.

Registration is documented end to end in the new
[docs/NodeManagerRegistration.md](docs/NodeManagerRegistration.md),
which covers compile-time, startup, and runtime registration together
with the guarantees for MonitoredItems, Browse continuation points,
namespace indexes, DataTypes, and change notifications.
## Review feedback addressed

Interface surface reduced, as requested:

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

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

- The lifecycle feature itself leaves `SubscriptionManager` almost
untouched: two guards that reject a request from a Session that is
closing, on `CreateSubscription` and on `TransferSubscriptions`, because
it would create state that is about to be torn down. The deletion claim
registry, the session closing gates, the closed-session bookkeeping and
the deletion restore paths introduced by earlier revisions are gone.
Transferring a subscription away from a session that is closing stays
allowed, since that is the recovery path the service exists for; the
race that exposed is fixed by serialising source-queue closure with the
transfer rather than by rejecting it.
- Queueing is disabled again for a MonitoredItem with `QueueSize <= 1`,
as before this PR. Without a queue the Client is served the last sampled
value, so the missing-Node notification simply becomes that value;
losing values is expected for a MonitoredItem without queuing.
- What remains of the notification is one `DataValue` plus one bool in
`DataChangeQueueHandler`, mirroring the existing overflow pair, which
stops it being the value discarded when a configured queue is full.
- `MonitoredItem` keeps its original publish path, its original queue
creation and restore paths, its original triggering behaviour, and no
per-epoch bookkeeping.
- `IStoredMonitoredItem` carries `IsDeleted` and `IsDetached`, so a
restored item reports the state it was stored with rather than one
inferred from the last value's StatusCode.

Other feedback:

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

## Fixes found during review

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

## Open review discussions

Three threads are open, replied to and awaiting reviewer decisions:
separating service-call dispatch from NodeManager handling in
`MasterNodeManager` (tracked as #4098), the request-scoping shape in
`StandardServer`, and how much of the async MonitoredItem service
implementation should stay in `Subscription`.

Issue #4102 records a specification ambiguity found while implementing
the missing-Node notification: Part 4 §5.8.4.1 makes it mandatory, but
the protected over-capacity queue entry §5.13.1.5 defines applies to
`EventQueueOverflowEventType` only, so the specification does not say
how it survives a full queue.

## Validation

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

## Related Issues

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