Commit Graph

3 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
Marc Schier be61d598a9 Fix startup NodeManager lifecycle and imported method arguments (#4427)
# Description

Make startup-composed application NodeManagers participate in the live
lifecycle and bind imported NodeSet2 Method argument properties to their
typed `MethodState` children.

This change:

- adopts application NodeManagers created before startup into
`INodeManagerLifecycle.Registrations` after server startup succeeds,
while keeping diagnostics, configuration, and core managers protected;
- retains per-manager startup external-reference ownership and exact
routing positions so reload, removal, rollback, overlapping namespaces,
synchronous adapters, and server restart preserve existing behavior;
counted immutable reference snapshots avoid quadratic startup matching;
- rejects duplicate factory output across active, unpublished, and
retired lifecycle generations without deleting or disposing the live
manager; callers can catch the public
`NodeManagerAlreadyRegisteredException`;
- binds valid namespace-zero `Argument[]` `PropertyType`
`InputArguments` and `OutputArguments` Variables to
`MethodState.InputArguments` and `MethodState.OutputArguments` from
either `HasProperty` direction, without requiring `ParentNodeId`,
including local namespace-URI reference targets;
- preserves authored metadata during NodeSet typed-child promotion
without changing the initialization behavior of unrelated generated
nodes;
- retains generic-import guards for custom and malformed signatures,
duplicate standard-signature validation, and upstream factory-driven
generated-slot replacement and reconciliation callbacks;
- updates lifecycle and Runtime NodeSet documentation and clarifies
`OnServerStartedAsync` timing for both `StartAsync` overloads, including
the unopened default host returned to the caller.

This is a focused follow-up to #4418. That PR preserves registrations
from multiple fluent builders, but does not change lifecycle ownership
or NodeSet child typing. The branch includes upstream `master` through
`da7412c42`, including #4396, #4432, #4437, #4438, and #4440.

The completed review fixes in `686d0561f` are included. The latest base
merge (`a29fc6ca8`) combines them with #4432's shared batch/fluent
importer and generated state factories. One iterative linker supports
available parent nodes, deferred parent side tables, application-owned
handles, and placeholder reconciliation while retaining reference-driven
Method argument binding and URI handling. Upstream cancellation, GDS
assembly renames, certificate leak scoping, and xRegistry changes are
preserved.

## Related Issues

- Fixes #4421
- Fixes #4422

## CI regression fix

`e1a4436f0` fixes the GDS failure reported on Ubuntu and macOS. Copying
permission metadata in general `NodeState` initialization applied
`SecurityAdmin`-only defaults to generated GDS custom certificate groups
and denied their existing GDS-admin Browse requests. Metadata
preservation is now confined to NodeSet typed-child promotion; imported
Method argument permissions remain intact, while unrelated generated
nodes retain their existing initialization behavior.

Local Release validation also exposed a 260-character certificate path
in the startup-failure test added by this PR. Its isolated PKI now uses
a short temporary path. No assertions, authorization checks, or CI gates
were weakened, and no pre-existing tests were changed.

## macOS ARM64 NativeAOT suspension

Per maintainer request, `a15971f32` suspends only the failing
`macos-latest` / `osx-arm64` NativeAOT matrix entry. Re-enabling it once
the compiler contains the runtime fix is tracked in #4443. Intel macOS,
Linux, and Windows NativeAOT coverage, managed macOS tests, and all
other CI gates remain enabled. This is an explicit coverage suspension,
not a claim that the underlying compiler/linker defect is fixed.

The macOS ARM64 NativeAOT job on `a29fc6ca8` fails during native linking
with
`ld: Assertion failed: (_addend == uniqueIndex && "too many large
addends")`.
Both attempts of [run
34191755950](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/34191755950)
hit the same assertion before the native test executable can run; the
build/test summary failure is downstream.

This matches the NativeAOT Mach-O relocation issue in
dotnet/runtime#119380, fixed by relocation-anchor emission in
dotnet/runtime#124721 and its .NET 10 backport dotnet/runtime#132171.
The failing `Microsoft.DotNet.ILCompiler` 10.0.11 package's nuspec
points to `dotnet/dotnet` commit
`e2f47b0110ed922f21a1522da67279133ce28f32` (July 23); its object writer
does not contain the anchor fix merged on August 12. Version 10.0.11 is
currently the latest stable package.

The base commit passed on the same runner image and SDK, so this is not
being dismissed as a generally broken runner or a test flake. The
precise PR-specific object-layout trigger has not been isolated. A
compiler containing the relocation fix is needed before restoring the
lane. No test code, assertions, coverage thresholds, or shared tooling
settings were changed.

## Validation

Latest base-merge validation on both `net10.0` and `net48`, Release:

- Import helpers and argument-binding regressions: 51 passed per
framework.
- Batch/fluent import, generated-state overlays, runtime/startup
lifecycle, cross-source references, routing, reference snapshots, and
node-authoring selection: 344 passed per framework.
- Merge-specific cases cover available Method parents with
forward/inverse and URI/index references while retaining an
application-owned `NodeId` handle.
- The unchanged upstream `NodeSetImportIntegrationTests.cs:210` CA1861
warning remains; no diagnostic suppression or unrelated cleanup was
added.

Release CI-fix validation on both `net10.0` and `net48`:

- `CustomCertificateGroupIntegrationTest`: 4 passed, including the
unchanged CI failure.
- `UANodeSetHelpersTests`: 8 passed, including imported security
metadata.
- `StartupRuntimeNodeSetLifecycleTests`: 5 passed.

Earlier Debug merge and review validation on both frameworks:

- Lifecycle, RuntimeNodeSet, fluent, and OperationContext selection
after merging `978e6d1af`: 638 passed per framework.
- Startup/transport-binding and RuntimeNodeSet integration selection
after merging `017856f4a`: 28 passed per framework.
- Public exception contract and duplicate-registration selection in
`fbf08428b`: 6 passed per framework.

Targeted test invocations rebuilt the affected projects successfully.
The full `UA.slnx` suite was not rerun locally.

## 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 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.
- [x] I have addressed **all** PR feedback received.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 2bb8a834-7695-4ae6-84cb-2471c76d1b12
2026-09-08 11:11:53 +00: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