5 Commits

Author	SHA1	Message	Date
romanett	ea93c86da2	Fix the Opc.Ua.Client defects reported by the client stack audit (#4471) ... # Description Fixes the 78 `Opc.Ua.Client` findings of a read-only bug audit of `Opc.Ua.Client` / `Opc.Ua.Types` / `Opc.Ua.SourceGeneration` taken at a05a36055, plus the regressions a subsequent review of those fixes turned up. Every behavioural change was checked against the OPC UA specification, and the citations are in the code comments. ## Session, reconnect and failover - The reconnect `finally` re-acquired `m_reconnectLock` with the caller's token, so a cancelled reconnect left the session stuck in `Reconnecting` forever and every later reconnect failed. It now uses `CancellationToken.None`. - An in-place session recreate tore down subscriptions it should not have: `transferred` is all-or-nothing, so a single failed transfer reset and recreated the ones the server had just taken over, orphaning them on the new session. `TransferSubscriptions` now reports the per-subscription outcome and only the subscriptions the server did not take over are recreated. A token-reuse failover keeps the same server session, so nothing is transferred or reset there at all. - In-place recreate only tears down the channel it created itself; a caller-supplied channel or a channel-manager lease is left to its owner. - `Snapshot(out SessionState)` went through the `SessionOptions` record copy constructor, which drops every member declared on `SessionState`, so the paired `Snapshot`/`Restore` API could not round-trip. - The user-token policy override restored the policy URI on failure but had already disposed the ECC ephemeral key the previous identity needs, so the identity the rollback claims to preserve could no longer be reactivated. The key is now parked and restored. - `SetTriggering` batching sliced the remove list past its end when a call had more adds than removes (Part 5 §6.3.11 bounds the *sum* of both arrays, so an individual list may be shorter than the limit). - The certificate-less `None` attempt dropped the request header, and with it the ECDH ephemeral key that travels in `additionalHeader` (Part 4 §7.1/§7.15). - `Session.Equals` compared a `ConfiguredEndpoint` against an `EndpointDescription`, so a session did not equal a session on the same endpoint. ## Managed session, pool and connection state machine - `ManagedSession.Dispose()` blocked on the connection state machine's worker, which raises `ConnectionStateChanged` synchronously — disposing from that handler deadlocked permanently. The machine now recognises its own flow and completes the teardown as the worker's continuation. - `ManagedSessionFactory` dropped `updateBeforeConnect`, `connection` and `reverseConnectManager` on all four `CreateAsync` overloads. - The session pool ran its shared connect on no token at all, so once the only caller gave up nothing could abort it and `RemoveAsync`/`DisposeAsync` waited out the whole reconnect budget. Each entry now owns the token that aborts its connect, and removal closes *and* disposes the session. - An explicit `engineFactory` was silently ignored for `DefaultSessionFactory` subclasses, which quietly downgraded the session to the classic subscription engine. ## Subscriptions - Fixed a lock-order inversion between `m_subscriptionLock` and the partition locks in `CompleteAsync` and `DisposeAsync`. - `OwnsSubscriptionId` stopped at the first registered subscription carrying the id — but in the very case it guards, the asker still holds its own retired id, so the answer depended on `HashSet` enumeration order and the delete could still tear down a sibling that had been handed the recycled id. - `MessageProcessor`'s republish gap walk was unbounded; it is now bounded by the server's retransmission queue (Part 4 §5.14.5). - A publish acknowledgement is sent even when a notification handler throws — the ack means "the server may delete the message" (Part 4 §5.14.5.2), not "I processed it". - Loaded monitored items did not raise the process-wide client-handle counter, so a fresh process minted handles that collided with the loaded ones; a transfer-restored item was also stamped `BadOperationAbandoned` and left at the default monitoring mode. - `AutoSetQueueSize` was not idempotent, so the queue size ratcheted up on every subscription state change. - The snapshot dropped the partition and recovery options entirely, and clamped the sampling interval and the secondary-partition idle timeout to zero — turning the Part 4 §7.21 "use the publishing interval" sentinel and "never delete an idle partition" into something else. - The state stream is positional OPC UA binary and the reader cannot detect an absent field, so appending the partition options would have mis-decoded every previously saved file. The stream now carries a format version and the first format is decoded through a frozen copy of its shape. ## Browse, node cache and the remaining clients - Managed browse wrote a retry pass's status into whatever node happened to sit at that offset in the caller's list, and retried continuation-point errors without any bound. It now keeps the caller's slot and gives up on passes that make no progress — a fixed pass cap would cut short a large browse against a conforming server with a small continuation-point quota. - Continuation points are released when a consumer stops enumerating early, as Part 4 §5.9.3.2 requires. - The node cache reordered its batched results relative to the inputs, which mispairs them for every consumer that matches positionally; its supertype walks could also spin forever on a server whose `HasSubtype` chain loops back on itself. - A browse error was cached as "this node has no references"; it is now surfaced to the direct caller, while the bulk and hierarchy walks treat the node as having no references for that one operation without caching it, so a single un-browsable node cannot fail a whole batch. - `WebApiClient` mutated a caller-supplied (typically `IHttpClientFactory`-pooled) `HttpClient`, which throws once that client has sent its first request; those instances are configured per request instead. - `PathCache` was not thread-safe. ## Review follow-ups The last two commits are the result of reviewing the audit fixes themselves. Worth calling out: the `Task.WaitAsync(CancellationToken)` polyfill in `Opc.Ua.Types` never checked `IsCompleted`, so an already-finished task plus an already-cancelled token reported cancellation on every pre-net8.0 target where the BCL overload returns the result. It is now aligned with the BCL and pinned by a test. ## Related Issues - No tracking issue; this PR is the result of a read-only audit of the client stack. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. ### Verification 35 regression tests were added under `tests/Opc.Ua.Client.Tests/AuditRegressions/` (category `AuditRegression`), one per fixed defect where the defect is observable at unit level; most were mutation-checked by reintroducing the bug and confirming the test fails. Locally on **net10.0**: `Opc.Ua.Client.Tests` 2252/2252, `Opc.Ua.Client.ComplexTypes.Tests` 2773/2773, `Opc.Ua.Bindings.Https.WebApi.Tests` 346/346, and all 23 library and test projects that reference `Opc.Ua.Client` build clean. `Opc.Ua.Client` also builds clean for **netstandard2.1**, the target that actually compiles the `WaitAsync` polyfill. The full **UA.slnx** solution and the .NET Framework targets have not been run locally — left to CI. One pre-existing failure in `Opc.Ua.Types.Tests` is unrelated to this branch: `WotSpecFixtureManifestTests.EveryEmbeddedExampleMatchesItsRecordedHash` (embedded WoT fixture hashes) fails identically with these changes reverted. ### Notes for reviewers - Two review findings were deliberately left unfixed and are documented in the code: `ManagedSession.CreateAsync` has to write the engine choice onto a caller-supplied `DefaultSessionFactory` subclass (the only alternative is changing the `ISessionFactory.Create` contract), and the connection state machine's "am I on my own worker" flag is an `AsyncLocal`, so it also flows into work a `StateChanged` handler spawns. The previous behaviour in that second case was a hard deadlock. - `Session.Dispose()`'s single `GetAwaiter().GetResult()` bridge to `CloseAsync` is left as is: `Dispose(bool)` must be synchronous and `DisposeAsync` already exists. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-14 08:22:28 +02:00
Copilot	d525bc3f28	Bound reconnect cleanup and organize samples by domain (#4276) ... # Description This PR fixes bounded ManagedSession reconnect cleanup and reorganizes the sample applications into domain-focused folders. ## Reconnect cleanup - Bounds reconnect integration cleanup with independent 30-second close and dispose limits. - Sends `CloseSession` through the awaited transport request path during `Session.DisposeAsync`. - Preserves explicit `Session.CloseAsync` behavior that waits for or cancels outstanding publish requests. - Covers the disposal request path with a regression test. ## Sample organization - `samples/OpenUsd`: `GeneratorServer`, `SiteCompositionServer` - `samples/Redundancy`: `RedundantClient`, `RedundantServer`, `RedundantPubSub` - `samples/Isa95`: `MinimalIsa95Server` - `samples/MinimalApi`: `MinimalBoilerServer`, `MinimalCalcServer`, `MinimalClient` - `samples/DI`: `PumpDeviceIntegrationServer` - `samples/Lds`: `ConsoleLdsServer` - `samples/Reference`: `ConsoleReferenceClient`, `ConsoleReferenceServer` - `samples/PubSub`: `ConsoleReferencePubSubClient` `UA.slnx` now exposes the same requested domain groups. Project references, tests, runtime sample probes, CI workflows, Dockerfiles, Compose files, helper scripts, and documentation were updated to the new paths. `samples/Robotics/MinimalRobotServer` remains under `Robotics`. ## Validation - `dotnet build UA.slnx` - 3 Redundancy short-haul sample tests - 70 DI sample tests - 914 OpenUSD tests - 1 WotCon model-materialization test - 137 AOT tests - `docker compose config` for all 6 moved Redundancy Compose files ## Related Issues - Fixes #4273 ## 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. - [x] I built the complete `UA.slnx` solution locally. - [ ] I ran all tests locally against at least .NET Framework and .NET 10. - [ ] 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-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: Marc Schier <marcschier@users.noreply.github.com> Copilot-Session: a2c2838f-74ae-4997-a1d4-50574d9e0636	2026-08-21 04:35:47 +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
Marc Schier	8538f531e7	Update paths for the consolidated repository layout	2026-07-16 06:26:05 +02:00
Marc Schier	7cec2d614d	Move repository files to the consolidated layout	2026-07-16 06:25:54 +02:00