4 Commits

Include renames

Author	SHA1	Message	Date
romanettandClaude Opus 5.5	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
romanettandClaude Opus 5	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
4 people	ede0b68154	Fix reverse-connect certificate leak and stale-connection retry (#4386) ... Two related robustness fixes on the reverse-connect / UA-SC handshake path, both surfaced by the Sessions suite. ## 1. Certificate handle leak in the asymmetric header parse `UaSCBinaryChannel.ReadAsymmetricMessageHeader` parses the sender certificate chain (`Utils.ParseCertificateChainBlob`, allocating fresh `Certificate` handles) and then runs several validations that can throw — invalid/missing sender thumbprint, no matching receiver certificate, receiver thumbprint mismatch, missing receiver thumbprint. The chain is returned through an `out` parameter, so a throw never reaches the caller's `using (senderCertificateChain)` in `ReadAsymmetricMessageAsync`, and the freshly parsed chain is abandoned. For a self-signed leaf that is exactly the intermittent `created == disposed + 1` seen on the Release leak gate under coverage instrumentation. This is the third sibling of the two concurrent-reconnect ownership leaks closed in #4356. **Fix:** wrap the post-allocation region in a `try/catch` that disposes the chain before rethrowing (validation logic unchanged). **Test:** `ReadAsymmetricMessageHeaderDisposesSenderChainWhenReceiverThumbprintMismatches` crafts a header with a valid sender certificate and a mismatched receiver thumbprint; without the fix it reproduces `created=3, disposed=2` and trips the assembly leak gate, with the fix it passes. ## 2. Stale reverse-connection retry A reverse connection the `ReverseConnectManager` delivers can already have been closed by the server before the secure-channel handshake runs: a server rotates and times out the outbound connections it offers, so the one matched here — especially after an `updateBeforeConnect` discovery round consumed an earlier one — may be stale. That surfaced as an intermittent `BadConnectionClosed` ("Remote side closed the connection") failing the whole connect (e.g. `ReverseConnect2Async(true,true,…)`). **Fix:** wrap reverse-connect session creation in `DefaultSessionFactory` in a bounded retry (3 attempts) that requests a freshly delivered connection when an attempt fails with a connection-drop status (`BadConnectionClosed` / `BadNotConnected` / `BadSecureChannelClosed`). Non-transient failures and cancellation still surface immediately; the bound prevents an unbounded loop against a genuinely unreachable server. ## 3. Test-host crash: NULL channel map in the inactivity timer `TcpTransportListener.DetectInactiveChannels` (a fire-and-forget inactivity-timer callback) iterated `m_channels` without holding `m_lock`, while `CloseAsync` disposes the timer and nulls `m_channels` under the lock. Disposing an `ITimer` does not join an already-dispatched callback, so a late callback dereferenced the now-null map and threw an unhandled `NullReferenceException` on a thread-pool thread, crashing the process. This intermittently aborted the `test-macOS-latest-Client` host. **Fix:** snapshot `m_channels` and return early when null (mirroring the existing guarded read on the accept path). Deterministic regression test invokes the callback on a listener whose channel map is null (its state before `OpenAsync` / after `CloseAsync`) and asserts it does not throw. ## Verification - `Opc.Ua.Core.Tests` Stack.Transport/Client/Server: 2178 passed, leak gate green. - Coverage-instrumented `Opc.Ua.Sessions.Tests` (the CI leg): 775 passed, 0 failed, leak gate green. - Reverse-connect suite (`ReverseConnect*`): 52 passed with the retry. - `Opc.Ua.Core` and `Opc.Ua.Client` build warning-free (net10.0, net48, netstandard2.1, net472). Note: the reverse-connect failure is a rare timing flake (0 reproductions in 88 local attempts), so fix #2 addresses the root cause identified from the ADO log and is validated by construction plus regression rather than by a captured local repro. ## Related - Follows #4356 (concurrent channel reconnect certificate leaks). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Marc <marcschier@hotmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>	2026-09-02 12:24:42 +02:00
Marc Schier	7cec2d614d	Move repository files to the consolidated layout	2026-07-16 06:25:54 +02:00