21 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	efad0ddeff	Server subscriptions: fix spec defects in Subscription/MonitoredItem handling found by audit (#4629) ... # Description Fixes defects found in a spec-grounded audit of the server **Subscription / MonitoredItem** implementation (`src/Opc.Ua.Server/Subscription/**`, `NodeManager/MonitoredItem/**`, `EventManager`, monitored-item service dispatch and filter validation). Each behaviour change was checked against OPC 10000-4 (§5.13, §5.14, §7.21, §7.22), Part 5 (subscription diagnostics) and Part 9 (ConditionRefresh). Every fix has a regression test. The audit found 56 defects (1 Critical, 3 High, 20 Medium, 32 Low). A follow-up max-effort code review of the fix branch found 27 more issues, mostly introduced by the first round of fixes. All but one are fixed here. ### Most important fixes - **Critical — remote hang:** an `AggregateFilter` with `processingInterval = NaN` was not revised, and `AggregateCalculator` looped forever while holding the node-manager lock, so one CreateMonitoredItems request blocked the node manager. Non-finite, ≤0 and sub-tick intervals are now revised (Part 4 §5.13.2.1), and the calculator rejects them as a backstop. - **Monitored-item limits:** new `ServerConfiguration.MaxMonitoredItemCount` / `MaxMonitoredItemsPerSubscription`; Bad_TooManyMonitoredItems is returned when a limit is hit, and ServerCapabilities now report the limits. Both default to `0` (no limit), so existing behaviour is unchanged. - **Sampling groups:** a client could get one dedicated thread per distinct sampling interval above 1 h; intervals above the last rate group are now rounded, and the loop is async. An item could also end up in two sampling groups and keep being sampled after it was deleted. - **Subscription state machine** (Part 4 §5.14.1.2): - the lifetime counter resets while a Publish is queued and counts otherwise; - the lifetime and keep-alive stay within `MaxSubscriptionLifetime`; - LatePublishRequestCount is counted once per entry into LATE; - messages held while publishing is disabled are no longer sent, and are not offered by Transfer/Republish either; - ConditionRefresh returns Bad_RefreshInProgress for any refresh already queued or running on the subscription. - **Triggering** (Part 4 §5.13.1.6): - linked items are triggered by an item still draining a backlog, by the first value after re-enable, and by an item created Disabled; - links are persisted for durable subscriptions; - Disabled/Sampling items follow the publish rules in the shared pipeline, so custom `IMonitoredItem` implementations follow them too. - **Queues** (Part 4 §5.13.1.5, §7.21): - no duplicate value after a queue-size increase; - EventQueueOverflow events are placed where the events were lost, including on queue shrink; - the duplicate-event check scans the newest entries; - EventQueueOverflowCount and MonitoringQueueOverflowCount are maintained correctly; - changing select clauses rebuilds queued events in the new layout, and is safe on durable queues and on failure. - **Filters:** - per-clause EventFilterResult instead of rejecting the whole item (Part 4 §7.22.3); - corrected status codes: Bad_DeadbandFilterInvalid, Bad_MonitoredItemFilterInvalid, Bad_EventFilterInvalid, Bad_MonitoringModeInvalid, Bad_TimestampsToReturnInvalid; - aggregate filter in subscriptions: processing interval at least 2× the revised sampling interval, revised StartTime on the requested grid, ServerTimestamp = end of interval (Part 4 §7.22.4). - **Lifecycle and races:** - expired subscriptions are always cleaned up, even when the session closes during cleanup; - Create/Transfer racing CloseSession returns Bad_SessionClosed; - session diagnostics counts no longer drift; - a large but valid Publish TimeoutHint no longer fails the request; - a ready subscription is served by queued Publish requests instead of going Late; - restored persisted state is validated. - **Access:** a value change notification is checked against each subscriber's UserAccessLevel, not the writer's. - **Redundancy / durable store:** - the first-unsent sequence number is mirrored; - sequence numbers are no longer reused after a second failover; - triggering links are stored (definition format 6, written only when links exist, so older replicas can still read link-less subscriptions). ### Behaviour changes reviewers should know - With the default config, publishing intervals above `MaxSubscriptionLifetime / 3` (1,200,000 ms) are revised down, so lifetime ≥ 3×keep-alive fits the configured maximum. - The revised keep-alive is capped at a third of the max lifetime. - Idle subscriptions expire after exactly MaxLifetimeCount cycles without a queued Publish. - A partly invalid EventFilter select list is accepted with per-clause errors. The public `EventFilter.Validate` keeps Status Good if at least one clause is valid; callers that cannot return per-clause results must also check the new `EventFilter.Result.HasSelectClauseErrors`. There is a migration note in `docs/migrate/2.0.x/sessions-subscriptions.md`. - Aggregate processing intervals may be revised higher (≥ 2× sampling interval); aggregate ServerTimestamp is now the interval end. - Sampling intervals above 3,600,000 ms are rounded to multiples of 900,000 ms (default rate groups). ### Public API Additive only: - `ServerConfiguration.MaxMonitoredItemCount` and `MaxMonitoredItemsPerSubscription`; - `EventFilter.Result.HasSelectClauseErrors`; - `IStoredSubscriptionTriggering`, `ISubscriptionRetransmissionSendStateStore`, `SubscriptionRetransmissionState.FirstUnsentSequenceNumber` (optional store capabilities, detected with `as`); - new `EventQueueHandler` constructor overloads. ### Known open items (not in this PR) - **Part 4 §5.13.2.1:** CreateMonitoredItems should create an item even when the user lacks Read permission and report the error in Publish. This is not done: the create-time RolePermissions check is the only role check on the initial value, so following the spec would leak values until every node manager re-checks per sample. - **Sampling loop:** it runs on thread-pool threads; node managers with blocking synchronous reads should not use sampling groups heavily. The alternative, a dedicated thread per group, was the thread-exhaustion issue fixed here. - **Restarts:** a pending QueueSize-1 value and a pending overflow flag are not kept across a restart; that needs persistence interface extensions. - **Limit defaults:** decide whether the new limits should get finite defaults (currently 0 = unlimited). ## Related Issues - No tracking issue; the findings come from an internal audit of the server subscription code. ## 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. - [ ] 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. ### Local verification (net10.0, all green) | Suite | Passed | Failed | |---|---|---| | Opc.Ua.Server.Tests | 7963 | 0 | | Opc.Ua.Subscriptions.Tests | 643 | 0 | | Opc.Ua.Subscriptions.Classic.Tests | 57 | 0 | | Opc.Ua.Subscriptions.Durable.Tests | 36 | 0 | | Opc.Ua.Redundancy.Server.Tests | 847 | 0 | | Opc.Ua.Core.Tests (filter tests) | 283 | 0 | net48 build of `Opc.Ua.Server.Tests`: 0 errors. net48 tests and the full UA.slnx were not run locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>	2026-10-03 05:43:16 +02:00
romanett	61edaa1ed0	Remove netstandard2.1 and net472 target frameworks (#4624) ... # Description Drops support for **netstandard2.1** and **.NET Framework 4.7.2**. Libraries now target `net48`, `net8.0`, `net9.0` and `net10.0`. `Opc.Ua.Types` and `Opc.Ua.SourceGeneration.Core` still also target `netstandard2.0`, but only because the Roslyn source generators load them. There is no general netstandard2.0 support. **Preprocessor directives** (112 files) - Every `#if`/`#elif` mentioning `NETSTANDARD2_1*`, `NET472*`, `NET_STANDARD_TESTS`, `SKIP_ECC_CERTIFICATE_REQUEST_SIGNING` or `NETSTANDARD*` was evaluated against the remaining TFM set (net48, net8/9/10, plus netstandard2.0 inside `Opc.Ua.Types`/`tools`) and simplified. Dead branches were removed. - Example: `NETSTANDARD2_1 || NET8_0_OR_GREATER` → `NET8_0_OR_GREATER`. - Example: `NET8_0_OR_GREATER || NET472_OR_GREATER` → removed (always true). - Netstandard-only code was removed: `UnixFileModeShim`, the CRDT reflection path in `ReplicatedGossipOptions.CreateReaderOptions`, and the netstandard2.1-only test variants. **Custom test logic** - `targets.props` drops the `net472`, `netstandard2.0` and `netstandard2.1` `CustomTestTarget` profiles, `NetStandardTests` and `SKIP_ECC_CERTIFICATE_REQUEST_SIGNING`. Supported values are now `net48`, `net8.0`, `net9.0`, `net10.0`. - The `NET_STANDARD_TESTS` define is removed from 29 test projects. - The "build netstandard shells as net10.0" overrides are removed from 18 projects. - `RestrictForLegacyTfm` now only applies to `net48`. - `tests/customtest.bat` is updated to the new values. **Build / packaging / CI** - MSBuild TFM conditions are simplified: Kestrel package choice, MQTTnet v4 (net48 only), BouncyCastle, Di/OpenUsd/Vision/Stress test excludes, and `System.IO.FileSystem.AccessControl` (dropped from Core). - The MigrationAnalyzer nuspec drops its net472/netstandard2.1 lib groups. The Azure signing lists are updated. - `get-ci-matrix.ps1` removes the net472/netstandard test profiles (including the PR-gating `linux-netstandard2.1`) and the matching build legs. **Breaking API change (2.0 preview):** removed `HttpsTransportListener.IsWssTransportSupported` and `ReplicatedGossipOptions.IsGossipStateDecodingSupported`. They only returned `false` on netstandard2.1 and are now always `true`. The test guards that used them were removed as well. **Docs** - NuGet READMEs, `README.md`, `DeveloperGuide.md`, `PubSub.md`, the certificate/ECC/identity docs and the 2.0 migration guide (plus its `.agents` skill mirror) are updated. - The migration guide notes that 4.7.2 consumers must move to 4.8 and netstandard2.1 consumers to net8.0+. ### Reviewer notes - The `System.IO.FileSystem.AccessControl` `PackageVersion` pin is left in `Directory.Packages.props`. It has no direct reference any more, but transitive pinning is on, so it was kept to avoid shifting resolved versions. - The `#if` rewrite was done mechanically by a truth-table evaluator. The diff in the `.cs` files is limited to directive lines, removed dead branches, and stale-comment updates. ### Verified locally - `dotnet build UA.slnx -p:CustomTestTarget=net48`: 0 errors. - `dotnet build UA.slnx -p:CustomTestTarget=net10.0`: 0 errors. - Default multi-target builds (no `CustomTestTarget`) of Opc.Ua.Types, Bindings.Https, PubSub.Mqtt and Redundancy.Server succeeded. - net10.0 tests: - Opc.Ua.Redundancy.Server.Tests: 840/840 passed. - Opc.Ua.Types.Tests: 11363 passed, 1 skipped. - ModelDependencyScanner tests: 20/20 passed. - The full test suite was **not** run locally. ## Related Issues - None ## 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. - [ ] 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-02 17:09:49 +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
Marc Schier	46fa7d364a	Harden recovery, concurrency, and resource lifetimes across the OPC UA stack (#4516) ... # Description Bound managed-session recovery, preserve Publish workers and recovery ownership, and address the client, server, transport, and certificate audit findings and subsequent concurrency review. Also reduce the memory retained by exhaustive session-test diagnostics. ## Recovery and Publish ownership - Add `ManagedSessionOptions.ChannelReconnectTimeout` through direct construction, the fluent builder, and DI. The automatic duration is `max(3 * KeepAliveInterval, revised SessionTimeout, OperationTimeout)`, sampled at cycle start. Positive overrides are supported; `Timeout.InfiniteTimeSpan` explicitly opts out. Raw sessions retain their unlimited default. - Add `CreateReconnectBudget(TimeProvider)` as a required `IReconnectParticipant` member, as agreed in review. Custom participants implement it and may return null for no participant-specific bound. - Enforce the earliest participant/caller deadline through transport reconnect, activation/recreation, and subscription restoration. Late callers can shorten an active cycle but cannot extend it. - Cancel and unwind recovery, fence late responses, and release channel recovery ownership before the existing `Faulted` event hands control to the outer reconnect/failover policy. Preserve session recreate admission through final Publish cleanup and keepalive suppression until shared recovery is complete. - Treat participant budget creation/evaluation failures, including exceptions at the exact deadline, as terminal shared-recovery failures. Notify participants before exposing `Faulted`, preserve the original diagnostic, and never silently replace a finite budget with unlimited recovery. - Keep caller-supplied replacement channels, managed bindings, wrapper event subscriptions, and recovery-owner identities consistent across success, failure, and cancellation. Reject replacement during active recovery and fence retired callbacks so they cannot clear newer ownership or suppress later keepalive recovery. - Retain eligible Publish workers through temporary subscription-identifier clearing without retaining workers for never-created or removed subscriptions. Preserve cancellation, acknowledgement rollback, and drain-before-replacement ordering. - Coordinate automatic subscription updates with session recovery: cancel admitted updates, keep them paused across a failed-restoration handoff, and retry only after explicit restoration completes. This prevents competing CreateSubscription requests and updates against a retiring session, while preserving manual recreation, transfer fallback, and pending option changes. - Exercise the real ManagedSession, V2 subscription engine, and channel ready gate with scripted transports and an injected clock. Cover transfer fallback, deadline handoff, late completions, and resumed notification delivery. ## Audit and review corrections - Correct certificate/trust-list transactions, change-pump lifetimes, validation, peer revalidation, and reset/resource ownership. - Harden node admission, attribute/range/dimension validation, monitored-item sampling and lifecycle transitions, event fanout, and server shutdown. Run lifecycle readiness and compensation outside shared monitored-item admission, with stale-completion ownership checks. - Close synchronous node-manager admission before teardown and retain resources until admitted service, lifecycle, and asynchronous method work returns. Dispose the monitored-item manager outside both locks, reject late calls before they can reach it, and forward asynchronous disposal through the adapter so the master awaits the same completion. Preserve reentrant shutdown initiation, cancellation/failure unwinding, and shared cleanup-failure reporting. - Bound WSS pending-request retention using connection-local wire handles while preserving caller-visible request handles and translating Cancel targets. Safely ignore retired replies and drain admitted send/close users before resource disposal. - Clarify that the shared `ClientBase` caller-handle sequence is distinct from the non-wrapping WSS wire allocator. Existing exhaustion/reopen, late-reply, and Cancel-translation regressions verify the connection-local contract; no allocator behavior was changed by this clarification. - Preserve virtual-node validation and resolver service errors instead of converting them to cached misses. A mismatched NodeId reports `BadNodeIdInvalid`; callback type mismatches and resolver service errors keep their exact status. Per-item boundaries preserve unaffected reads, writes, history operations, method calls, browse paths, and monitored-item creation/restoration, including permission-validation paths. Caller cancellation still propagates; stored-item validation failures are logged without aborting later restores. - Correct client file/role/state-machine/streaming APIs, node-cache behavior, redundancy/failover ownership, and session configuration/identity handling. - Cancel each owned peer-discovery scope before disposing it, including when the bounded wait wins a cancellation-delivery race. Keep late results out of the endpoint cache and preserve single-flight lookup ownership. - Keep materialized alias Browse results synchronized with the live store by default, preserving explicit startup-only opt-out. Apply HistoryUpdate service results per value while retaining explicit all-or-nothing provider APIs. - Update the directly related feature documentation and add focused positive, negative, concurrency, and ownership regressions. ## Latest review-feedback corrections - Fence stale failed lease observations with a successful-confirmation generation distinct from the revocation epoch. Snapshot it before each store read, including failed-CAS revalidation, so an old failure cannot revoke or reschedule a newer lease while fresh ownership loss still revokes leadership. Preserve independent overlapping acquisitions, expiry, disposal, and late-success protections. - Isolate attributable WSS response-decoding errors to their request, including retired handles. Keep admitted socket sends connection-owned so caller timeout/cancellation cannot abort sibling requests; shutdown still cancels and drains those sends. - Restart keepalive and classic Publish replenishment after successful session reuse. Retire V2 worker demand after intentional deletion/disable without confusing that state with temporary recovery resets. - Browse virtual directories lazily across continuation pages without the 1,024-entry snapshot cap. Preserve active-page cancellation, bounded metadata retention, pushback, and exactly-once asynchronous cursor cleanup. - Apply certificate/CRL staging in last-operation order, preflight required issuer storage before writes, select actual policy-compatible RSA keys for None endpoints, frame certificate chains independently of signature OIDs, and preserve admitted validation results during rejected-writer shutdown. - Preserve base asynchronous disposal when a legacy override omits its base call; avoid double derived disposal in the adapter. Let genuinely admitted nested work finish while rejecting stale captured contexts and returning null from closed lookup APIs. - Re-park recovered monitored items if AddNodes rolls back. Give event compensation a fresh finite deadline, and preserve healthy Browse/Translate siblings at every reference/path boundary while retaining starting-node errors and caller cancellation. - Give final reconnect notifications a live, bounded shutdown-linked lifetime. Synchronize child state before parent lifecycle handlers outside the parent transition lock. - Document the accepted existing policies: direct reverse connections are single-use without a ReverseConnectManager, and multi-node stream capacity is shared rather than reserved per node. The expanded readiness controls also exposed a real generation-handoff race: an old source could finish draining after activation was deferred but before readiness tasks were collected, causing the wanted replacement to be omitted from the wait set. A deterministic test gates that exact interval; the fix keeps the replacement in the readiness wait. The original race passed 200 repetitions after the fix. A separate control now awaits both producer readiness and iterator entry under the same original deadline, rather than treating those independent signals as equivalent. Temporary logging and repetition are removed. ## CI integration contracts The History and Server CI failures exposed integration tests that still asserted the behavior intentionally changed by the audit fixes. A Windows WoT negative test raced its caller deadline against upstream retries. A subsequent Ubuntu Client failure exposed a real automatic-update versus explicit-restoration race: - History insertion collisions now verify exact per-value statuses and all persisted timestamps/values, including preservation of the existing conflicting entry. Explicit atomic-provider rollback controls remain. - Alias materialization tests wait for actual publication and verify the added node plus forward/inverse `AliasFor` references, instead of racing a startup-only snapshot expectation. Explicit refresh opt-out controls remain. - The unavailable-upstream WoT test uses an explicit finite retry policy through existing DI configuration, preserving its original four-minute guard and requiring a mapped-value service error before caller cancellation. Production retry defaults remain unchanged. - The Client fix retains the exact one-create and late-response assertions. Three deterministic real-subscription/real-manager cases cover paused creation, disposal while paused, and cancellation of an admitted create followed by explicit restoration and a pending option update. Both composed restoration-deadline cases also prove automatic updates stay blocked at the outer handoff and resume after restoration. - The merged-head net48 correction prevents lost peer-resolver cancellation and distinguishes the harness's delayed initial `Connected` event from actual outer recovery completion. A deterministic timer-order test covers both cooperative and cancellation-ignoring resolvers, cancellation before return, rejection of late cached results, and a fresh subsequent lookup. - The remaining CodeQL alert was a false content flow through `ArrayOf.Count` into an integer-only trust-list summary. Projecting the same count through `Span.Length` keeps cardinality distinct from serialized contents without allocating, dropping diagnostics, suppressing the rule, or dismissing the alert. Five log-contract cases cover null/empty/nonempty results and mixed chain-capable/fallback listeners, asserting the exact count, message, and structured properties. - Classic transfer acknowledges messages selected for asynchronous Republish only after their responses arrive. The existing locked replay-selection decision returns the immediate-acknowledgement subset to Session, preserving the public transfer API, advertised availability, and immediate acknowledgement of older gapped messages not selected for replay. A callback changing `RepublishAfterTransfer` cannot change the already-selected acknowledgement ownership. - Transfer integration tests explicitly control acknowledged versus retained origin messages. Exact assertions distinguish retained replay from requested fresh initial values by sequence number, client handles, values, status, and source timestamp. Atomic counters, observed delivery/keepalive progress, and failure-safe session/file cleanup replace timing assumptions and racy resets. - Server lifecycle tests verify the documented independent five-second compensation deadline rather than requiring a non-cancellable token. They cancel the actual request, prove compensation and unrelated monitored-item creation still proceed one tick before the deadline, preserve the original failure and ownership assertions, and verify the completed deadline is retired. - Synchronous and asynchronous directory-browser tests require the exact provider exception instead of an empty-directory success, then verify the failed cursor is not enumerated again. These correct expectations left behind by the intentional, documented provider-error propagation change; production behavior is unchanged. No tests were disabled, assertions loosened, coverage thresholds lowered, or CI checks made optional. Existing platform exclusions and Explicit debug variants remain. ## Related Issues - Fixes #4514 - Fixes #4515 - Builds on merged #4503 and retains the #4508 publishing-drain behavior. ## Validation **Lease-election review correction (`a13afa641182c70983e16bbcfebf507e666a45a5`):** The correction is based on the existing upstream merge `43079e2b851fe338a4082066f8d14abfdafb6e32`. Three deterministic stale-verdict cases fail on the original source and pass with separate confirmation fencing. Six new cases cover the initial foreign-owner verdict, failed-CAS revalidation in both invocation orders, and fresh ownership loss through both read paths, including revalidation begun after another successful confirmation. | Target and configuration | Passed | Failed | Skipped | | --- | ---: | ---: | ---: | | net10.0 Debug | 120 | 0 | 0 | | net48 Debug | 120 | 0 | 0 | The selection is the full `Opc.Ua.Core.Tests.Redundancy` namespace, including all 38 `SharedStoreLeaseElectionTests` cases and the existing expiry, clock rollback, disposal, and overlapping-success regressions. New witnesses are `StaleForeignLeaseReadDoesNotRevokeNewerAcquisitionAsync`, `StaleFailedRevalidationDoesNotRevokeNewerAcquisitionAsync`, and `FreshForeignLeaseObservationRevokesLeadershipAsync`. They assert resulting leadership, exact transitions, independent progress, and the unchanged expiry boundary. Retaining the initial generation for the fallback read independently makes the fresh-loss-after-confirmation control fail; that mutation was removed before the final rebuilt runs. The successful scoped command is `dotnet test tests\Opc.Ua.Core.Tests\Opc.Ua.Core.Tests.csproj -c Debug --no-restore --filter "FullyQualifiedName~Opc.Ua.Core.Tests.Redundancy"`, with `CustomTestTarget` and `-f` matching each row and that framework's assets restored. Both changed C# files pass formatting and analyzers through informational severity on both frameworks. These are scoped results, not a full-solution test or measured coverage-delta claim. CI monitoring is paused for this feedback-only operation; no current-head CI success is claimed. **Latest CI contract correction (`1cd1f2047bb5b50c9bf300b1ff710b0b2e935a29`):** All four failing Server shard 23 jobs in [run 36120946740](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/36120946740) reported the same six cases. Two local replays reproduced all six failures. The source changes and documentation from `cd43b831a` establish both intentional contracts: independent bounded event compensation and surfaced provider errors. Only the two test files are corrected; production code and CI policy are unchanged. | Target and configuration | Passed | Failed | Skipped | | --- | ---: | ---: | ---: | | net48 Debug | 113 | 0 | 0 | | net10.0 Release, CI Coverlet settings | 113 | 0 | 0 | The selection contains `MonitoredItemManagerLifecycleTests`, `DirectoryObjectStateTests`, `DirectoryBrowserBoundedEnumerationTests`, and `EventCompensationReadinessHasFiniteDeadlineWithInfiniteSourceTimeoutAsync`. Both changed files pass formatting and analyzers through informational severity on both frameworks. Independent mutations restoring a non-cancellable compensation token and swallowing provider errors fail all four lifecycle cases and both browser cases, respectively. Both mutations were removed before the final passing runs and commit. The scoped command is `dotnet test tests\Opc.Ua.Server.Tests\Opc.Ua.Server.Tests.csproj` with `CustomTestTarget` and `-f` set to the selected framework, `--no-restore`, and `--filter "FullyQualifiedName~MonitoredItemManagerLifecycleTests|FullyQualifiedName~DirectoryObjectStateTests|FullyQualifiedName~DirectoryBrowserBoundedEnumerationTests|FullyQualifiedName~EventCompensationReadinessHasFiniteDeadlineWithInfiniteSourceTimeoutAsync"`. The modern run additionally uses `-c Release /p:CollectCoverage=true --collect "XPlat Code Coverage" --settings tests\coverlet.runsettings.xml`. These are scoped results, not a full-solution test or measured coverage-delta claim. Hosted checks for that commit were pending at publication. The fail-fast CI watch was stopped when the requested operation switched to review feedback; no current-head CI success is claimed. **Earlier master merge (`8b15ea6ef9fc2214ac8ec886f373bb3846dd6d6f`, base `b4136d8e9bab0df680f41d9ef985f519160534b1`):** The `TcpListenerChannel` conflict preserves both lifetime responsibilities: disposal clears the retained peer-certificate chain and unregisters the quota-scoped message-assembly scheduler. Both fields and all upstream deadline handling remain intact. The only additional manual source adjustment adds the parentheses required by the existing analyzer around the session-count fallback. Master advanced during validation. The initial transport merge `80303c434ce813cf75aa7fe54a6a32fe2aeafc30` was retained, and the newer base was merged normally in `8b15ea6ef9fc2214ac8ec886f373bb3846dd6d6f`, without rewriting history. The additional recovery, fluent-pipeline, and test-framework overlaps were inspected; the branch's cancellation, validation-error, and acknowledgement ownership fixes remain alongside the upstream changes. Full `UA.slnx` builds passed on net10.0 and net48 with **0 warnings and 0 errors**. The final scoped tests rebuilt against the committed tree `2074f8efcb61d8d9c6106e7d6f76ee941940e358`: | Target | Passed | Failed | Skipped | | --- | ---: | ---: | ---: | | net10.0 | 2,653 | 0 | 0 | | net48 | 2,529 | 0 | 7 | The net48 skips are existing platform exclusions: six RSA_DH_AesGcm nonce cases and one ClientWebSocket certificate-validation case requiring .NET 7 or later. The selection covers transport cleanup, reassembly budgets/deadlines/admission, peer-chain ownership, server session binding, fluent lifecycles and source generation, reconnect and keepalive, transfer acknowledgements, Publish queues, and JSON encoding. Modern-.NET WSS transport and real-server integration cases also pass. The resolved file passed solution-relative formatting and analyzer verification through informational severity on both frameworks. These are scoped test results, not unfiltered full-suite or measured-coverage claims. Hosted CI was not monitored or repaired as part of this merge-only operation. Selected local results below are overlapping validation runs, not distinct-test totals. **Earlier review-feedback batch (`cd43b831a8f11de3f7d65c3681712a61e3c669a5`):** The final net10.0 scoped selection passed **471 tests, with no failures or skips**. The final net48 solution-root selection passed **439 tests, with no failures and one unchanged platform skip** (`ValidateServerCertificateFallsBackToTlsPolicyErrors`, which requires newer ClientWebSocket certificate-validation support). Both full nonincremental solution builds completed with **0 warnings and 0 errors**, and all **40 changed C# files** passed informational-level formatting/analyzer verification. Both frameworks validated the identical final tracked diff. The focused matrix covers every one of the 20 current review threads, including real mid-frame WSS cancellation, stale admission contexts, directory continuation ownership, multi-element Browse/Translate failures, and the readiness drain race. The net10.0 solution-root test command has a separate pre-existing runner conflict: three unrelated AOT projects reject the VSTest target under Microsoft.Testing.Platform 2.4.0 and SDK 10. Per-project net10.0 results are recorded separately; no runner migration, test exclusion, assertion relaxation, or CI-policy change is included. These are bounded selections, not unfiltered full-suite or measured coverage claims. At publication of `cd43b831a`, hosted CI was not green: the `OPCFoundation.UA-.NETStandard` check reported `ACTION_REQUIRED`, AppVeyor reported `FAILURE`, and the two signed-build jobs were in progress. No CI jobs were rerun, cancelled, approved, or repaired as part of that feedback-only operation. **Classic transfer correction (`f425376fca045794122e19aaf9cafebc4a717d91`):** The preceding head's [macOS net10.0 shard 25/30](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/35980326299/job/107570512458) failed `TransferSubscriptionTest(DisconnectedRepublish).TransferSubscriptionAsync(True,False)` at the static-node count assertion: expected 10, received 20. The trace contains a successful, explicitly requested republish of retained sequence 1 and a fresh initial-value batch at sequence 3, not duplicate dispatch of one response. Controlled lost origin acknowledgements reproduced that incorrect test expectation on both the preceding PR head and exact master base `a2539cb53dd1666cef856f33812d7a8830d68909`. The maintainer explicitly authorized correcting this existing issue in the PR. Retained replay and requested initial values are independent, as specified by [OPC UA Part 4, TransferSubscriptions](https://reference.opcfoundation.org/specs/OPC-10000-4/5.14.7). The reason that the original hosted run retained its particular origin message remains unproved. The expanded full-suite run then exposed a **real production race**, not just a count assertion: Session queued acknowledgements for all transferred available messages before the asynchronous Republish responses arrived. Publish could delete those messages first, causing `BadMessageNotAvailable`. A gated real-Session/classic-engine regression failed both replay-enabled cases on the original implementation while both no-replay controls passed. The ownership fix passes all 24 combinations of empty/consecutive/unsorted-gapped queues, replay preference, sequential delivery, and a reentrant preference change. All 40 normal real-server transfer cases pass on both frameworks. Independent mutations suppressing actual replay and duplicating observed sequence records each fail the intended assertion; both mutations were removed. The full classic suite passes on both frameworks. Hosted CI was pending when that correction was published; the earlier failed hosted job was not rerun for a lucky pass. **Local net48 caveat:** the full Client run timed out in the pre-existing fake-clock test `ReconnectAsyncSwapsFaultedLeaseEntryAsync`, which does not exercise subscription transfer. Its unchanged isolated replay passed, but that does not resolve the full-suite timeout. No unrelated code or assertions were changed, and a clean full Client net48 run is not claimed. | Scope | net10.0 | net48 | | --- | --- | --- | | Full Client.Tests after transfer acknowledgement correction | 2,549 passed | 2,552 passed, 1 timeout failure noted above, 1 existing platform skip | | Full classic-subscription project after transfer correction | 57 passed, including all 40 normal transfer cases | 57 passed, including all 40 normal transfer cases | | Gated transferred-message acknowledgement ownership | 24 passed, included above | 24 passed, included above | | Saved-session consumer of the shared subscription helper | 40 passed | 16 passed, 24 existing unsupported-security-policy skips | | Audit regression matrix | 2,339 passed | 1,934 passed, 34 existing skips | | Full Client.Tests after review follow-ups | 2,520 passed | Focused selections only | | Full Client.Tests after CI recovery correction | 2,523 passed, CI coverage collector | 2,527 passed, 1 existing platform skip | | Latest merged-head recovery/redundancy selection | 139 passed | 139 passed | | Full Client.Tests after merged-head net48 correction | Focused selection above | 2,529 passed, 1 existing platform skip | | Lifecycle and actual-dimension regressions | 146 passed | 146 passed | | Focused recovery follow-ups | 7 passed | 7 passed | | WSS-related fixtures | 93 passed | Not applicable | | CI History integration corrections | 28 passed | 28 passed | | CI alias, dispatcher, and explicit-atomic controls | 63 passed | 63 passed | | Full WoT sample project with CI coverage collector | 29 passed | Not applicable | | Trust-list effects and count-only summary contract | 22 passed | 22 passed | | Virtual-node feedback, node-manager, dispatcher, and lifecycle selections | 661 passed, 63 existing not-executed cases; all 48 virtual-node cases passed | 661 passed, 63 existing not-executed cases; all 48 virtual-node cases passed | | WSS wire-handle exhaustion, late replies, and Cancel translation | 9 passed | Not applicable | | Synchronous node-manager disposal and related service/lifecycle suites | 1,037 passed, 64 not-executed cases from existing fixture assumptions; all 93 sampling/disposal cases passed | 1,037 passed, 64 not-executed cases from existing fixture assumptions; all 93 sampling/disposal cases passed | - Full solution builds passed on net10.0 and net48 during the audit work. The subsequent full non-incremental net10.0 build also completed with zero warnings/errors. - Modified source/test formatting and analyzer checks passed through informational severity. - Original composed recovery regressions passed repeated runs on both frameworks. Independent mutations removing Publish cancellation, bypassing the recreate drain call, or ignoring ready-gate cancellation each failed the intended assertion; all mutations were restored before passing runs. - The latest recovery correction passed 100 executions of each original restoration-deadline scenario with unchanged assertions. Restoring premature update admission independently made both strengthened handoff cases fail; the correct retention behavior was restored afterward. - Startup-event and cancellation-delivery timing probes reproduced the subsequent net48 failures, then passed 100 executions of each of the three scenarios with the fixes. All timing probes and repetition were removed. The permanent timeout-order regression also went RED on the original cancellation cleanup without sleeps or scheduling amplification. - Final post-format recovery/subscription/engine checks passed all 151 selected cases on net10.0. The first local full net48 attempt aborted in `clr.dll` with native access violation `0xc0000005`; an unchanged-assembly full diagnostic rerun passed as recorded above. That native host crash has not been attributed or claimed fixed. - A minimal replay with the same CodeQL CLI and query-pack versions reproduced the custom-count alert and confirmed that the standard span-length projection is clean while direct sensitive-value and raw-identifier logging controls remain flagged. The source was already logging only a count; no identifier or certificate disclosure was established at the reported location. - The virtual-node feedback regressions first reproduced five error-preservation/cache failures; expanded service-dispatch cases exposed two additional whole-batch failures before the operation-boundary fixes. The final selections pass on both frameworks. Genuine misses remain cached, successful results remain reusable, failed validation allows same-cache retry, and request cancellation still stops the batch. - Three deterministic synchronous-manager disposal regressions reproduced early resource release and lifecycle calls reaching a manager during disposal. The fix passes direct and adapted calls, a sampling-worker join after the last operation exits, reentrant shutdown, cancellation and failure paths, repeated disposal, and exact rejection across 39 service/lifecycle entry points both during drain and after cleanup. The original outside-node-lock disposal regression remains and now requires the worker to observe closed admission. - Prior-head CI on `c985f8bded77a60449d9cd381c8a004d8590ccf6` completed with **168 passing checks, one expected release-package-publication skip, and no failures or pending checks**. All test shards, signed builds, container builds, the coverage check, and the CodeQL PR check passed. Hosted CodeQL analysis `1829262912` completed successfully and marks count-flow alert 385 fixed. This is evidence for that head, not a claim that subsequent review follow-ups have completed hosted CI. ### Session-test diagnostic memory Complete browse listings are streamed to unique NUnit attachments rather than retained in inline NUnit/VSTest output. Detached regression-test results are cleaned up without deleting runner-collected attachments. - Two covered session-suite runs each completed with 782 passed and 379 existing skips. Peak working set fell from 4.86 GiB to 2.44-2.78 GiB. - TRX output shrank from 303.1 MiB to approximately 40.7 MiB, retaining all 46 complete browse attachments. - Seven output regressions cover exact content, empty/single/large listings, disabled/enabled output, bounded inline summaries, and unique filenames. The separate Azure build-18452 VM deprovisioning cause remains unproved without provider logs. The memory measurements do not establish its cause. Unfiltered local `UA.slnx` tests, including the long stress/soak suites, have not been run on both frameworks. A local coverage delta was not collected; builds and focused tests are not substitutes for those claims. ## 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. - [ ] All checks on the latest review-follow-up head have completed successfully; prior-head CI passed as recorded above. - [x] I have addressed **all** PR feedback received. Tests have been added and pass as recorded above; the hosted coverage check passed on the prior head identified above. The combined tests/coverage-increase checkbox remains open because no local coverage delta was measured. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208	2026-09-25 14:59:34 +00:00
Marc Schier	138086863b	Fix audit findings and review regressions across the OPC UA stack (#4503) ... ## Scope Audit fixes and review follow-ups across the OPC UA Core, Client, Server, transport, and redundancy components. This includes implemented Tier 1, Tier 2, and Tier 3 work; it does not claim that every item in the 244-finding audit backlog is complete. The review batch fixes mandatory full-key record protection and owned plaintext buffers; transactional pending-certificate claims; shared-channel nonce and certificate ownership; discovery and token-policy validation; reconnect/callback ordering without changing unbounded buffering defaults; server permissions, filters, monitored-item lifetime, and alarm topology; historian paging, retention, and annotations; atomic user/password metadata; and canonical file identities with bounded alias and role refresh workers. ## Review-batch verification at `ab686ccae` The following results come from fresh test builds using `dotnet test`, `--no-restore`, `-m:1`, and an explicit `CustomTestTarget`. They replace the earlier description's library-build/`--no-build` totals, which did not establish the state of the merged test sources. | Scope | Target | Passed | Skipped | |---|---|---:|---:| | Core review regressions | net10.0 | 827 | 0 | | Client and lifecycle regressions | net10.0 | 630 | 0 | | Server review regressions | net10.0 | 1970 | 0 | | Redundancy Server / Client / PubSub | net10.0 | 276 / 10 / 11 | 0 | | Sessions / file-and-alias / streaming / HTTPS-WSS integration | net10.0 | 8 / 21 / 30 / 20 | 0 | | Core review regressions | net48 | 786 | 39 | | Client recovery and policy regressions | net48 | 54 | 0 | | Server ownership, aliases, queues, and user-state regressions | net48 | 137 | 0 | Every listed run exited successfully. The 39 net48 skips are existing platform/capability cases. New key-agreement tests assert unavailable-policy behavior on unsupported targets rather than adding skips or pretending those targets performed an unsupported handshake. Additional exact node/permission/filter/alarm checks passed 13 Core and 46 Server cases. The PR-scoped formatter verification covers 26 projects and 389 non-generated C# files. A Roslyn syntax audit covers 53 changed interfaces and 263 members, with no missing parameter/type-parameter/return contracts and a 120-column limit for their declarations and documentation. No full-solution, all-target-framework, or NativeAOT matrix is claimed from these local runs; the new CI run must establish its own result. ## CI follow-up at `e1bc3ac3b` The follow-up fixes the failures reported by Azure build 18400. TLS certificates retain configured DNS names, including mixed-case URI schemes, without relaxing hostname validation. Reverse-WSS fixtures use the DNS identity in the client certificate. Reconnect completion is published only after cycle cleanup, so a subsequent reconnect cannot join an old completed cycle and retain old certificates. Certificate-alarm roots now support event subscriptions. The pump sample has one continuous notifier path, with no duplicate shortcut. Historian and aggregate fixtures share the provider's retention clock. Alarm collection lets the server expire idle Publish requests rather than canceling only the client wait. The corrected server, WoT, and managed-session fixtures exercise valid requests and initialized lifecycle contracts. Downlevel crypto tests preserve real credential and key-ownership assertions without enabling unsupported raw-ECDH policies or adding skips. Fresh local evidence after integration includes: | Scope | Target | Passed | Skipped | |---|---|---:|---:| | Client lifecycle / channel ownership / builder fixtures | net10.0 and net48, each | 96 / 103 / 123 | 0 | | Combined server lifecycle, events, aggregates, crypto, and contracts | net48 | 358 | 0 | | Full lifecycle plus certificate-alarm and aggregate fixtures | net10.0 | 116 | 0 | | Final server-contract and history-routing delta | net10.0 and net48, each | 84 | 0 | | Subscription / XRegistry / WoT event regressions | net10.0 | 7 / 2 / 3 | 0 | | Full Sessions project | net10.0 | 780 | 379 | | Full ComplexTypes project / downlevel resolver fixtures | net10.0 / net48 | 2854 / 15 | 0 | | Full WoT project / downlevel structured-write fixtures | net10.0 / net48 | 1496 / 27 | 0 | | TLS domain-name regressions | net10.0 and net48, each | 21 | 0 | All listed runs exited 0. The Sessions skips come from existing endpoint and policy capability guards; they are not counted as passes. No skip was added. Worker validation also covered full DI (426 passes), full History (540 passes, 21 existing skips), 38 exact downlevel event/history failures, and 60 repeated certificate-rotation race cases. The final crypto fixtures passed 73 cases on each target with the unchanged leak detector; a temporary premature-disposal mutation made all three borrower tests fail and was then removed. All 29 C# files in this follow-up pass included-file formatting at info severity across 12 projects. The complete PR interface audit now covers 54 interfaces and 271 members with zero missing contracts or line-width findings. No full-solution or NativeAOT matrix is claimed. New-head CI has started and remains authoritative for the remote result. ## Test-result gate follow-up at `4b6e1203a` Azure build 18401 reported the net48 Client job as successful despite a test-host crash after only 264 passing tests. Its old evaluator counted per-test failures but ignored the failed run verdict. This was a partial run, not a benign exit after the suite had completed. The shared result evaluator now rejects aborted, errored, incomplete, malformed, or inconsistent TRX reports. Each file must describe a completed run, so a passing file cannot mask another file's failure. It retains support for completed runs followed by a benign nonzero host exit, existing skipped-result conventions, and no-TRX MTP jobs. All 78 regression cases pass after integration using Windows PowerShell 5.1. Real completed Sessions and Client reports also pass. The process-test fixture follows the existing NET10_0-only tools-test pattern; its net48 build is not counted as a test run. Formatting passes for the added test file. The Client host crash itself has not reproduced locally. The isolated fixture passed 59 tests, and two fresh full fast-CI selections each completed with 2,392 passes and three existing skips, including a run with crash collection enabled. The CI merge tree is identical to the tested source tree. No speculative runtime change was made, and the original partial CI run is not claimed as a full-suite pass. Azure build 18402 has now completed the full net48 Client run: 2,392 passed, three existing console skips, and a successful test-host exit. The corrected evaluator independently validated outcome=Completed. Across the matrix, 119 test-project jobs succeeded; Sessions net10.0 was canceled when the remote provider deprovisioned its runner, so the overall Azure pipeline was canceled. That cancellation is not claimed as a code failure or a successful run. CodeQL and three Docker image jobs were still running at the latest check. The migration discovery failure remains separate. ## Documentation follow-up at `57ed02bea` The environment blocker has been removed by the maintainer. The three bundled migration references now exactly match the canonical documents after deterministic link rewriting; all 15 bundled documents pass `validate-migration-plugin.ps1`. The canonical migration prose was not rewritten. `FileSystemClient.md` now documents that extended Windows roots preserve literal trailing-dot names, completing the filesystem review follow-up. The filesystem thread has been replied to and resolved after publication. The review pass now has 108 of 110 threads resolved. The two explanation-only threads remain open as requested; no unfinished change request is being represented as resolved. ## Cross-platform follow-up at `0d9d936a8` With migration discovery restored, the Linux and macOS test matrix exposed two stale durable-subscription expectations. The expected value 83,442 encoded the old 32-bit overflow when converting 3,600 hours to milliseconds. The existing overflow-safe server correctly returns 14,400,000 lifetime intervals at a 900 ms publishing interval. Only those two expected constants changed; production behavior, negotiated-hour checks, and all other assertions remain unchanged. The exact local lifetime selection reproduced both failures before the correction. The full durable suite now passes 36 cases with 11 existing skips on each of net10.0 and net48. All four lifetime cases executed and passed on both targets, and included-file info-severity formatting passes. ## Session shutdown follow-up at `8daf7f091` A late net48 Sessions failure exposed a shutdown race. Canceling an in-flight keepalive read can produce a transport `ServiceResultException`. The retiring worker treated its own cancellation as proof that the peer was unavailable, so `CloseAsync` or `DisposeAsync` could skip the server-side `CloseSession` request. The worker now ignores that service error only when its own token is canceled; genuine peer errors still update liveness. The full integration fixture reproduced the original error-code mismatch. Four deterministic mock-transport cases then produced three failures and one passing genuine-error control before the fix. They verify successful close, returned server error, asynchronous disposal, and unchanged behavior for a real keepalive failure. Fresh related runs pass 75 client and 101 integration cases on each of net48 and net10.0, with no skips. Ten repetitions of the formerly flaky full net48 fixture pass all 1,010 cases. All three changed files pass info-severity formatting, and temporary diagnostic instrumentation was removed. ## Atomic listener snapshot follow-up at `82dc22b5d` Ubuntu Core CI exposed an unsafe formatter rewrite in TCP listener disposal: `ConcurrentDictionary.ToArray()` had become a collection spread. A concurrent channel removal between the spread's count and copy operations could leave a default entry, causing a null dereference during disposal. The listener now uses the dictionary's atomic `Values` snapshot and disposes those channels outside the listener lock. Deterministic regressions close zero, one, or all channels at the copy boundary. Before the fix, the one/all-removal cases reproduced the exact null dereference; the unchanged-count control passed. All 23 admission/listener cases now pass on net10.0 and net48, and the full fast Core net10.0 suite passes 5,267 cases with 88 existing platform skips. Both changed files pass info-severity formatting. A bounded audit of the formatter's other snapshot rewrites found no further verified unsafe concurrent-registry copies. ## Shutdown-test observation follow-up at `58403ee8b` The net48 shutdown-order test raced its own observation: it read `m_publishWorkerTask` after starting shutdown, which can legitimately drain the worker and clear that field immediately. The test now captures the worker before shutdown starts and confirms it is initially running. It still requires the worker to finish while shutdown remains blocked by the held manager semaphore. Production behavior is unchanged. A temporary scheduling perturbation reproduced the exact null-field failure. The corrected capture passed 100 perturbed iterations; both the temporary repeat and yield were then removed. The final 126 subscription/lifecycle cases pass on each of net48 and net10.0, and formatting passes. A complete local Sessions net10.0 run also passed 780 cases with 379 existing capability skips in 7m20s after the earlier keepalive correction. ## Retirement and alias-startup follow-up at `1a048ff39` The shadow-generation test now observes its documented asynchronous cleanup through the existing bounded helper. Removing the replacement does not join a background drain that already claimed the retired generation. The test still checks live-registration removal immediately and requires the old node to disappear; no timeout or production lifecycle behavior changed. All 81 lifecycle cases pass on each of net10.0 and net48. Initial alias-node materialization now retries one timed-out, read-only snapshot query before creating nodes for that root. The retry is logged through source-generated logging. Each query retains the original shared 100 ms deadline; repeated timeouts, other errors, and cancellation still abort startup. Client searches and live refresh do not gain this immediate retry. Fault-injection tests cover success after one timeout, a persistent timeout, a different store failure, and cancellation. All 187 related alias/hosting cases and the original certificate-rotation integration case pass on each target; the positive test records exactly one retry warning. The alias documentation now accurately describes the shared request budget. All changed C# files pass formatter checks, and the complete 54-interface/271-member documentation audit passes. Fresh CI is running on `1a048ff39`; the previous completed matrix exposed only the two failures corrected in this batch and their dependent summaries. ## Sample endpoint allocation follow-up at `d08a94925` The strong-cluster test harness allocated OPC UA and Raft endpoints in separate released-port batches. Uniqueness was enforced only inside each batch, so the same OS-assigned port could appear in both protocol groups. The harness now requests one distinct batch for both groups and splits it before launching any replicas. Deterministic one- and three-replica allocation cases reproduced overlapping groups with the old plan and pass with the corrected plan. The complete 17-case short-haul sample suite passes without skips; all failover, historian-continuation, and service assertions remain unchanged. Both changed files pass info-severity formatting. This prevents intra-cluster self-collisions but does not claim to reserve released ports against unrelated external processes. ## Code-review follow-up at `085fd9636` A review of the complete branch diff found five defects this PR introduced. All are now fixed, each in its own commit. `RelatedTo`'s recursive-to-iterative rewrite defaulted the optional `includeTypeDefinitionSubtypes` and `includeReferenceTypeSubtypes` operands to `false`. Part 4 §7.7.4 specifies both default to `true`, so a filter that omitted them silently stopped matching subtypes. Dynamically created key-credential nodes derived their namespace from the containing folder, which is the standard `i=18155` node in namespace 0 — the namespace reserved for the OPC UA standard address space; they now resolve the server-owned namespace and fail with `BadInternalError` rather than guessing index 1. The retained audience-less `CreateTokenData` overload produced tokens that verification always rejects, so it is now `[Obsolete]` and throws instead. A failed reference to a remote parent left the new node registered with a one-sided inverse reference, and is now rolled back. An existing `.pfx` is restricted before it is rewritten, rather than after its new key bytes are on disk. One further reported finding was rejected after investigation. `JwksIssuerKeyResolver` deliberately does not dispose retired key snapshots: `RetiredKeysAreCollectibleWhileResolverRemainsAliveAsync` proves the resolver retains no retired snapshot, and `ReturnedKeysSurviveRepeatedRefreshAndResolverDisposalAsync` requires already-returned keys to keep verifying across refreshes and resolver disposal. Adding disposal would have broken that tested contract, so the exploratory change was reverted. Validated on net10.0 and net48: 256 Core and 633 Server cases pass, with one pre-existing net48 skip. The build reports zero compiler and analyzer diagnostics, all eight included-file info-format checks pass, the contract audit covers 58 interfaces and 310 members with no findings, and the 15 bundled migration documents validate. All 122 review threads are resolved; the key-credential namespace thread was reopened to retract an earlier incorrect answer and then resolved against the real fix. ## Review-body follow-up at `da0d59cfb` The re-review at `8daf7f091 + 82dc22b5d` listed four items in its summary that had no inline thread of their own, so they had not been picked up. All four are now fixed. Both event-dispatch permission gates had been tightened from not-Bad to `IsGood`, which silently dropped events whose permission verdict is Uncertain; the data-change path in the same file still treated Uncertain as deliverable, so the two had diverged. `EventSourceRegistry` only cleared `ConsecutiveFailures` when a source was deactivated, so a source that had backed off to the 30-second cap kept that delay for the rest of the process even after starting successfully. `UserNameIdentityTokenHandler` built a temporary issuer chain whose `Add` calls take independent certificate references, and never released them. The fourth was the receiver-side sequence rollover, which the reviewer had correctly re-flagged as still open despite its thread being resolved. `m_sequenceRollover` was set permanently, so the second legal wrap on a long-lived channel was rejected and faulted the channel along with the transport-reconnect OPN. OPC 10000-6 does not cap a SecureChannel at one wrap. The flag is now cleared once an accepted number has traversed the normal range, which keeps rejecting a replayed low number that never left the rollover window. Validated on net10.0 and net48: 636 / 593 Core cases and 135 / 134 Server cases pass, including the new `UaSCBinaryChannelSequenceRolloverTests` and the Uncertain-permission delivery cases. The build reports zero compiler and analyzer diagnostics and the changed files pass included-file info-format checks on both targets. The previous head `085fd9636` was fully green: 279 checks passed and Azure build 18414 succeeded with 137 of 137 jobs, after the platform automatically retried a job whose NuGet tool download had hit a transient TLS error. ## Reported issues #4508 and #4509 at `6ad1ecf16` **#4508 (V2 subscription recreate deadlocks against a faulted transport) was not yet fixed and is now closed by `bac260a06`.** Both drain sites the report identifies were still unbounded. A publish worker that started its request before the quiesce parks inside the service call on the channel ready gate while holding an active publish request; the recreate that would make the channel ready is the very operation awaiting the drain, so the wait never ends. The drain now cancels in-flight attempts through a per-attempt token linked to the worker token. An aborted worker rolls its acknowledgements back, releases the request and returns to the paused park instead of terminating, so it resumes when the caller leaves the quiesce. Publish ingress stays quiesced for the caller's operation, so `DropPendingForSubscription` keeps the guarantee the report asks to preserve and no blind timeout is added. The new regression parks a worker on a cancellable gate that never completes on its own: it reproduces the hang for the full 30-second budget before the change and passes after it. **#4509 (identity provider fails the initial connect) was already fixed for its primary symptom.** `HandleConnectAsync` materializes the provider's identity through `CreateIdentitySelectionContext` before the first `CreateAsync`, so Anonymous is no longer substituted against a UserName-only endpoint. `6ad1ecf16` adds the two remaining details from the report: the enabled-policy set now includes the negotiated channel policy, so a client cannot reject the policy it already accepted as `NotEnabledByClient`; and when discovery has not populated the endpoint's token policies the connect keeps the configured identity so the attempt fails at the transport layer for the reconnect policy to retry, instead of failing at the identity layer. Validated on net10.0 and net48: 477 / 473 client session and subscription cases pass, the build is free of compiler and analyzer diagnostics, and the changed files pass included-file info-format checks on both targets. The full `Opc.Ua.Sessions.Tests` suite also passes locally on net10.0 with 782 passed and 379 pre-existing capability skips. GitHub checks are green on this head: Build and Test .NET 10.0, Docker Sample Images CI, and NuGet Publish all succeeded. Azure build 18417 completed 134 of its 135 jobs successfully; its net10.0 Sessions job was cancelled by the hosted-agent provider ("Received request to deprovision"), the same infrastructure flake that hit build 18409 earlier on a head predating these changes. That job is not claimed as passing, and the local full-suite run above covers the same project. The previous head `da0d59cfb` completed Azure build 18415 with every job succeeded. ## Review follow-up at `a08f0c12b` Eleven new review threads arrived, nearly all documentation. Before acting on each "this is new in 2.0" remark I checked the API against `master378` rather than assuming, because a genuine 1.5 API still needs its migration entry. `ManagedSession`'s server-redundancy refresh had two hard-coded two-second bounds. The new `ServerRedundancyOptions` exposes `RefreshTimeout` for the metadata read and `PeerDiscoveryTimeout` for each peer lookup, defaults both to the previous two seconds, validates that a bound is positive or `Timeout.InfiniteTimeSpan`, and flows through `ManagedSessionOptions`, `ManagedSession.CreateAsync`, the fluent builder, and `DefaultServerRedundancyHandler`. `AsyncCustomNodeManager` and `KeyCredentialBridgeAuthenticator` are both absent from `master378`, so their obsolete members had no compatibility constraint and are removed rather than carried: `AsyncCustomNodeManager.ReadInitialValue`, the audience-less `CreateTokenData` that only threw, and the audience-less `CreateProof` that produced a proof no server accepts. `CustomNodeManager2.ReadInitialValue` and the `MonitoredNode2` migration section are genuine 1.5 APIs and are untouched. The documentation now states master's behaviour instead of the delta from 1.5.378, with each moved item already covered under `docs/migrate/2.0.x/`. `IHistorianAnnotationProvider` is described by its annotation-time key rather than as "legacy" — it is new in 2.0, so the label implied a compatibility constraint that does not exist. The KeyCredential bridge payload's `"version"` field is explained as a vendor-extension format version with no OPC UA counterpart, and its version-1 upgrade narrative is gone. `CertificateManager.md`'s migration walkthrough is replaced by the resolver API it targets. `NodeManagers.md`, `Sessions.md`, `AsyncServerSupport.md`, `Certificates.md`, and `Profiles.md` no longer announce features relative to 1.5. `Benchmarks.md`, `WhatsNewIn2.0.md`, `DeveloperGuide.md`, and `docs/migrate/` keep their 1.5.378 references, which are their subject. CI then exposed a defect this batch introduced: five fixtures resolve `ManagedSession`'s private constructor by its exact positional parameter types, so the added parameter compiled cleanly but made `GetConstructor` return `null` at run time. `7df1d5c40` updates every site; `Redundancy.Client` passes 126 of 126 and the affected client fixtures 112 of 112. The BouncyCastle question is answered and tracked in #4510: `Opc.Ua.Core` already project-references `Opc.Ua.Security.Certificates`, which already carries the conditional BouncyCastle reference, so the raw-ECDH polyfill belongs there rather than as a new Core dependency. The `IUserDatabase` metadata question is addressed to another maintainer and is deliberately left open. ## Reported issue #4509 follow-up at `5d4d13d63` The reporter verified `6ad1ecf16` and found one case still failing, correctly. The empty-token-policies branch kept the configured identity so a server that is down surfaces a transport failure rather than an identity error. A provider-only configuration has no configured identity, so it kept `null`: `OpenCoreAsync` turned that into Anonymous and `OpenValidateIdentity` rejected it with `BadIdentityTokenInvalid`, silently ignoring the configured provider. The branch now requires a configured identity to defer to. Without one the provider is consulted, so its own `BadIdentityTokenRejected` names the offered and rejected policy set instead. Two regression tests cover both configurations; the first fails against the previous behaviour by reaching the session factory with a `null` identity, which is the exact step that produced the reported error. That branch had shipped without a test, which is why the case was missed. ## Review and CI status at `5d4d13d63` **All actionable review threads are resolved: 132 of 133.** The single open thread is a question the reviewer addressed to another maintainer about moving user metadata into `IUserDatabase`; it is left open for them rather than resolved silently. Every resolved thread carries a reply naming its fix commit. **Latest-head CI is green on `5d4d13d63`: 279 checks passed, zero failed, zero pending**, with one conditional signed-package publishing job skipped. Local validation for this batch: the full `Opc.Ua.Client.Tests` suite passes 2,430 of 2,430 on net10.0, `Opc.Ua.Redundancy.Client.Tests` 126 of 126, and `Opc.Ua.Server.Tests` 590 of 590 for the affected selections. Both net10.0 and net48 build clean for `Opc.Ua.Client`, `Opc.Ua.Server`, and the changed test projects, and all changed files pass included-file info-severity formatting on both targets. The PR remains open; no merge was performed. As stated at the top, this does not claim that every item in the 244-finding audit backlog is complete. ## Latest CI follow-up at `b6f2dfa43` Azure build 18442 exposed a net48 compile regression in the Kafka adapter: `RuntimeHelpers.GetUninitializedObject` is unavailable on .NET Framework. Commit `2cf1e0eed` now uses normal `ProducerMessage` construction on `NETFRAMEWORK` while retaining reflection-based construction for modern targets that can load a different Dekaf runtime asset. The exact net48 CI selection passes 92 of 92 tests, and the netstandard2.1 adapter guard selection passes 11 of 11. After merging the latest `master`, GitHub Actions run 35643592139 is green. The initial Ubuntu Core.Schema job built successfully and passed 160 of 160 tests, then hit an external 403 while finalizing its test-result artifact; its failed-job rerun completed successfully. Azure build 18451 was canceled when its remote provider deprovisioned the net10.0 Sessions runner (`Received request to deprovision: The request was cancelled by the remote provider`). Azure attached no test task log or source error, and the canceled parent left that child check stale as in-progress. No source workaround, test skip, or check relaxation was made for either infrastructure failure. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208	2026-09-22 09:10:58 +02:00
Marc Schier	fa8d45f2cf	Fix server/core validation, lifecycle and protocol regressions (#4477) ... # Description **Server/core audit fixes with regression tests. Latest review-fix validation, merge validation and earlier acceptance results are distinguished below.** This addresses a 95-finding server/core report. Every finding was reviewed with the OPC UA specification knowledge base or, where the standard does not prescribe a C# implementation, the corresponding ownership/configuration contract. Incorrect or withdrawn subclaims remain qualified rather than being presented as additional fixes. The original checkpoint covers session identity/certificate validation and revocation; certificate/key ownership and transactional updates; TCP reconnect, receive/admission and buffer lifetime; HTTP quotas; subscription admission, durability and pooled payloads; file-operation lifecycle; historian aggregates; fluent/state-machine behavior; and related utilities. It includes targeted regressions and directly related documentation. The approved acceptance cleanup is separate in the commit history: | Cleanup | Verified result | | --- | --- | | Invalid identity-policy and KeyCredential fixture inputs | Fixtures explicitly advertise Username and use its generated policy ID. Credential persistence uses valid clear mode inside the existing encrypted admin context. Original permission, lockout and negative encryption assertions remain. | | Native GDS lifetime and concurrent connection failures | Retain ApplicationInstance until client teardown. A deterministic native reproducer proved the test-only channel cap of 10 was exhausted; removing that override restores the existing bounded default of 1000. No global serialization or production quota change. | | Stress certificate ownership and close accounting | Balance two helper references and the fake transport's server-certificate ownership. Replace the invalid event-count bound with exactly-once physical disposal, opens = closes, zero active entries and one logical entry lifetime. | | Participant deadlines | Use the injected TimeProvider with the existing cross-framework WaitAsync overload. Tests pin one tick before, exact expiry, terminal notification, recovery and timely completion. | | Reproducibility and platform assertions | Preserve canonical WoT bytes/hashes, explicit display-culture contracts and exact numeric-vector assertions. Accept Apple's CryptographicException subtype without weakening disposal balance. | | Missing user-signature proof | Reject null/empty proofs before OS crypto. Return BadUserSignatureInvalid per Part 4, 5.7.3.3. None-policy and valid-signature controls remain. A CI-only test nullable-context error was corrected without changing production behavior. | | Durable sample restore and shared test storage | Dequeue waits for actual batch residency instead of treating failed restore completion as success, and restored files close before deletion. Five deterministic regressions failed before the fix. The two fixtures that clear the shared batch directory are isolated; all original 3,000-item assertions remain unchanged. | ## Latest review follow-up Commit `828856850219518970e8e2f580b8f94b3c1fc748` fixes the deferred robotics disposal reported in the latest review. `RobotIntentNodeManager.DisposeAsync` now joins a shared controller-host drain and then explicitly awaits base node-manager disposal. A controller's bounded shutdown wait may defer its cleanup, but no longer lets the node-manager override finish early. Concurrent callers, including an asynchronous call after synchronous `Dispose`, cannot bypass the drain after the host list has been detached. Synchronous disposal remains nonblocking for deferred work. Deferred initialization failures are logged and retained, and base cleanup failures propagate to asynchronous callers. The existing controller-level bounded disposal contract is unchanged; the node-manager contract is documented in `docs/Robotics.md`. | Regression requirement | Evidence | | --- | --- | | Wait for deferred hosts; preserve synchronous compatibility and concurrent joins | `NodeManagerAsyncDisposalWaitsForDeferredHostsAsync`: four cases covering both entry-point orders and zero/positive host shutdown waits. All four failed before the fix. | | Wait through base cleanup and observe failures | `NodeManagerDisposalWaitsForBaseCleanupAndReportsItsFailureAsync`: four cases hold the actual base-resource semaphore, verify pending callers after the host finishes, then check success or the same propagated exception and exactly-once cleanup. | The focused `IntentHostingCoverageTests` and `IntentControllerHostTests.Dispose` selection passed **30/30 on net10.0 and 30/30 on net48**, with no failures or skips. The final net10 solution build passed with zero errors and only the four previously documented CA2249 warnings in unchanged fuzz-test code. No new warnings, analyzer suppressions, dependencies, or weakened assertions were added. The review thread received a commit-specific reply and was explicitly resolved. This verification is scoped to the robotics feedback; the earlier full Subscriptions-run limitation remains recorded in the merge section below. ## Documentation review follow-up (2026-09-14) Commit `f9a9686fcc5d3cfd19cbbc8c2d826e20f5c7e72f` addresses the six follow-up threads on documentation and implementation rationale. Each has an in-thread reply citing the commit and an explicit resolution. Multiline XML documentation now covers the declarations introduced or modified throughout the entire PR, including internal contracts, samples, tests, and public members of related private helpers. A syntax-aware inventory checked **2,031 declarations across all 256 PR C# paths with zero documentation gaps**. A supplementary check also covers **75 previously unchanged public members** of private helpers involved in the changed code. Token comparison across the conditional-compilation branches proves this follow-up changes **no executable C# or test assertions**. Project-file changes are rationale comments only. The implementation questions are documented at their source: | Question | Resolution | | --- | --- | | Certificate assembly friendship | Server Push configuration reuses the internal explicit-subject CSR helper with the existing key; no public crypto API was added. | | Core.Types assembly friendship | Server builders use internal transition callbacks/revision guards; Server.Tests checks the revision deterministically without creating a public compatibility contract. | | Always-single-reader notifications | One dequeuer preserves event order; optional concurrency fans out within one event and joins before the next event. | | Asynchronous disposal completion | `DisposeAsync` already joins the completion published after admitted operations and all cleanup attempts. The compatibility `Dispose` remains nonblocking to avoid deadlocking an admitted callback against its own drain. | The requested detailed review disposition follows. It distinguishes actual fixes from confirmed policies and incorrect subclaims rather than counting every comment as a new runtime change. ### Detailed disposition of review 5191795284 The [SDK review](https://github.com/OPCFoundation/UA-.NETStandard/pull/4477#pullrequestreview-5191795284) was submitted against `1c4a9df6c`, before the subsequent merge and feedback fixes. Its **15 inline threads** have each received an action-specific reply and an explicit resolution. A fresh GraphQL query verified those replies and resolution states; the fixes in `b8a59990f` remain ancestors of the current branch. | Review concern | Action and disposition | Concrete regression evidence | | --- | --- | --- | | Injected certificate-store providers bypassed | **Fixed.** Registration, configuration snapshots, and validation use the injected providers. Explicit custom types remain authoritative; the built-in Directory catch-all is deferred so native X509 paths and later custom providers are not captured. Failed store opens dispose the created store. | `InjectedProviderResolvesRegisteredAndConfiguredTrustStores` covers default-provider ordering and both registration paths; provider validation/open-failure cases and the unchanged `TrustedCertificateInjectedViaIndependentX509StoreIsObservedAsync` cover real use and independent-store visibility. | | Unguarded timer-thread leadership callbacks | **Fixed.** Each subscriber's exception is logged independently and cannot suppress subsequent subscribers or fail lease operations. `IsLeader` reads the deadline without invoking application callbacks. | `ThrowingLeadershipSubscriberDoesNotFailAcquireOrDisposeAsync`, `ExpiryNotifiesOtherSubscribersWhenOneThrowsAsync`, and `IsLeaderDoesNotDispatchExpiryNotificationsAsync` were all demonstrated failing before the fix. | | Legacy pending-key provider accepts an unfinishable regenerated CSR | **Fixed by explicit capability validation.** A provider implementing only `IPendingCertificateKeyStore` receives `BadNotSupported` before key generation or persistence when regeneration is requested. Existing-key CSR/update remains supported. Regeneration requires matching-only consumption; no unsafe destructive probing was introduced. | `LegacyPendingProviderRejectsRegenerationBeforeCreatingOrSavingAKeyAsync` checks no key or store mutation; `LegacyPendingProviderCompletesExistingKeySigningAndUpdateAsync` proves the supported flow completes. | | Shutdown waits behind user deactivation | **Fixed.** Accepted deactivations drain before the master's lifecycle gate is taken. Configuration retains its binding until the shared drain completes, including concurrent shutdown callers and direct master disposal. | Four cases of `ShutdownDrainsDeactivationOutsideTheSessionClosingGateAsync` exercise real configuration/binding/ServerInternalData/master reentry, retained address space, exact session/subscription closure, and no production cancellation shortcut. | | Subclass cleanup runs under the operation-lifetime lock | **Fixed.** Cleanup starts outside that lock. A pre-published completion joins all disposers, and only the active asynchronous teardown context may use guarded helpers. Captured contexts lose that privilege when the callback exits. | `SynchronousOwnedCleanupLetsSamplingWorkerReachClosedAdmissionAsync`, `DisposeCoreCanUseGuardedMembersWithoutReopeningCallerAdmissionAsync`, captured-context rejection, and exact cleanup-failure/ownership assertions cover both synchronous and suspended cleanup. | | Discovery requestId-zero chunk read after pool return | **Fixed by the earlier merge.** Discovery validation precedes `SaveIntermediateChunk`; no read occurs after that path can return the buffer. The ordering is retained. | `DiscoveryChannelReadsTheFirstChunkBeforeItIsReturnedAsync` uses a poisoning pool and the requestId-zero path. | | T1.8 OPN cleanup misses post-decryption throws | **Fixed by the earlier merge and pinned with new regressions.** Both sequence rejection and certificate mismatch release the decrypted body and input ownership. | `OpenSecureChannelRejectionAfterDecryptionReturnsEveryRentalAsync` checks the exact audit status and pool conservation. Omitting either corresponding release in an isolated snapshot made its case fail; no mutation remains. | | T1.2 mount-root protection depends on the provider | **Fixed at the host boundary.** Separator-only aliases resolve to the protected root. Traversal, empty/whitespace segments, backslashes, and drive/stream qualifiers are rejected before arbitrary providers are invoked. Valid nested paths and root destinations remain supported. | A deliberately permissive provider is used by `NonCanonicalObjectPathsCannotDeleteMoveOrCopyAsync` and invalid-target cases; exact zero-call checks are paired with legitimate nested-path/root-destination controls. | | Inline trusted/issuer entries now face CRL checks | **Intentional strict behavior, documented and tested.** A configured backing store with strict unknown-status rejection requires current revocation information. Explicit-only lists without a store retain their no-store behavior. Actual revocation cannot be suppressed; a matching store entry's policy takes precedence. | `InlineIssuerWithStoreHonorsStrictUnknownRevocationPolicyAsync`, `InlineIssuerRevocationIsNeverSuppressedAsync`, store-precedence cases, and explicit-only controls preserve the missing-information versus actual-revocation distinction in Part 4 6.1.3. | | RSA minimum applied to unspecified ECC identifiers | **Fixed at the application-certificate check.** The RSA minimum is compared only with an actual RSA public key. An unspecified ECC identity is not replaced simply because its key size is below an RSA threshold. | `SilentCertificateCheckPreservesUnspecifiedEccIdentityAsync` checks DER, thumbprint, store contents, and private-key retention; explicit RSA and HTTPS minimum controls remain. | | Failed directory refresh wedges corrective mutations | **Fixed.** Recoverable automatic pre-refresh failures are logged and leave reconciliation pending, but do not block corrective provider mutations. Explicit refresh errors, capacity admission, caller cancellation, and actual provider denials remain enforced. | `CorrectiveDeleteRecoversAfterPersistentOverLimitRefreshFailureAsync` converges an externally over-limit directory back to its exact allowed contents. Capacity, cancellation/token, and six refresh-error controls check negative paths. | | Timed-transition rejection logs at 10 Hz | **Fixed.** Repeated `(state, revision, status)` warnings are deduplicated without changing the retry cadence. A different status or state entry permits another warning. | `RepeatedTimedRejectionLogsOnceAndRecoversAsync` checks one warning over eleven rejections and success on the next attempt. Status-change and same/different-state reentry controls pin reset behavior. | | Bad unsubscribe strands retired-generation bookkeeping | **Fixed.** Retired all-event bookkeeping completes even when unsubscribe reports Bad; the first Bad is still returned to the caller. | Good and BadCommunicationError cases of `RetiredAllEventsDeletionCompletesBookkeepingAndReleasesGenerationAsync` check event/snapshot removal, generation release, and exactly one unsubscribe without finalization replay. | | CSR subject parsing and fallback | **Fixed for existing and regenerated keys.** Omitted subjects fall back to the actual stored certificate. Regeneration resolves the public certificate's subject before creating a key. Malformed or unresolvable subjects return `BadInvalidArgument` before pending-key mutation. | Store-only fallback, real-generator null/empty cases, and missing-subject/malformed-DN controls verify the signed CSR, matching fresh pending key, unchanged active identity, and no destructive side effects. | | Committed RemoveUser change reported as failed | **Fixed for RemoveUser and ModifyUser.** Accepted cleanup is still awaited and close failures remain logged, but cannot replace the committed mutation result. Precommit failures retain their original result and side effects. | `CommittedUserChangeKeepsResultAndPublishedUsersWhenCloseFailsAsync` checks published Users, Good result, logging, and continued closes. `FailedUserChangeKeepsPublishedUsersAndDoesNotCloseSessionsAsync` checks BadNotFound with no publication or close. | | Alleged pooled decoded-request aliasing | **No runtime redesign needed; reviewer rebuttal confirmed.** The decoder copies the relevant input data. Misleading ownership comments were corrected rather than introducing a new dispatch model. | `DecodedRequestRetainsValuesAfterInputBuffersAreReturnedAsync` checks the exact decoded NodeId and ByteString after returned arrays are poisoned. | The current documentation-only follow-up rechecked **104 Core, 473 Server, and 4 Configuration cases** covering these concerns: **581 passed, zero failed or skipped**. The final net10 solution build also passed with **zero warnings and errors**. The complete earlier Core/Server/Configuration and net48 results remain listed separately rather than being presented as rerun current-head acceptance. ## Earlier review follow-up Commit `b8a59990fe2e8e967a12ffd5d9158beed7831143` addresses all 19 review threads. Each thread has an in-thread reply naming this commit and was explicitly resolved after verification; the final paginated query found 19 resolved and zero unresolved threads. The follow-up fixes injected certificate-store selection (including default provider ordering and native X509 paths), actual-RSA minimum enforcement, and CSR capability/subject validation before key mutation. Node-manager teardown uses simpler scoped ownership and drains accepted user revocation outside the master gate; committed user results and retired-event cleanup remain correct when cleanup reports failure. File mutations can recover after failed refresh without bypassing capacity or cancellation, provider paths protect mount roots, and repeated timed rejections no longer flood logs. Leadership subscriber failures are isolated without callbacks from `IsLeader`. Regressions also pin the already-merged OPN/discovery buffer fixes and prove decoded request values do not borrow returned input buffers. The strict inline issuer revocation policy is documented and tested, not weakened. Existing tests were changed only for the reviewed behavior: the incorrect committed-user result was corrected, and inline-issuer/root-alias controls were expanded. ### Review-fix validation | Suite | Scope | Framework | Passed | Existing TRX NotExecuted | | --- | --- | --- | ---: | ---: | | Core | Complete project | net10.0 | 4,970 | 88 | | Server | Complete project | net10.0 | 6,414 | 69 | | Configuration | Complete project | net10.0 | 231 | 0 | | Core | Certificate, lease and buffer compatibility slice | net48 | 153 | 0 | | Server | Reviewed lifecycle, user, file, timer and CSR compatibility slice | net48 | 692 | 0 | | Configuration | Complete project | net48 | 231 | 0 | | CSR provider regressions | Final portable-mock follow-up | net10.0 | 11 | 0 | All runs exited successfully with normal concurrency and leak detection. The Server console reports 5 skips; its TRX additionally records existing assumption/explicit cases, hence the 69-record count above. The Framework build caught two modern-only `ValueTask.CompletedTask` usages in new mock setup; only those expressions changed to the equivalent portable `default(ValueTask)`. The complete modern suites precede that test-only correction; all production files remained byte-identical, and the corrected fixture passed on both targets. The final net10 solution build succeeded with 0 errors and the same 4 CA2249 warnings in unchanged fuzz-test code. Diff checks and changed-line whitespace checks are clean; whole-file formatting still identifies 10 unchanged baseline lines, which were not reformatted outside the feedback scope. The committed 35-file tree was hash-verified against the validation inputs. This is the review-feedback verification scope, not a rerun or sign-off of the historical complete platform/native/soak acceptance campaign below. ## Latest upstream merge Merge `9d9bf679f317164c5b2b90924cd3cdc27a86d451` integrates `master` through `162fc5ebb` (#4486), including the AddNodes child-name index, shared certificate-error reporting, CTT sample-data corrections, and the accompanying regressions and documentation. The CreateSession conflict preserves this branch's certificate ownership, required certificate, application-URI, and channel-certificate checks. The validation result now retains nested errors through `ThrowIfInvalid`, allowing the upstream shared reporting helper to mask trust, policy, and revocation failures consistently without losing audit detail. The new indexed AddNodes regression initially attempted to reuse a deterministic NodeId after renaming its existing node. Renaming frees the BrowseName, not that NodeId. `IndexedBrowseNameReusePreservesExistingNodeIdentityAsync` now proves both contracts: the derived-ID collision leaves the original node intact, then a fresh explicitly requested ID can reuse the old name while the new name remains protected against duplicates. No production identity rule was relaxed. The incoming seeded random-index test uses the existing `UnsecureRandom` wrapper with the same seed and sequence; no new analyzer suppression was added. | Local merge validation | Framework | Passed | Existing exclusions | | --- | --- | ---: | ---: | | Core, complete project | net10.0 | 5,081 | 88 | | NodeState / BaseInstanceState | net10.0 | 579 | 0 | | Server node management, certificate and session-security selection | net10.0 | 112 | 0 | | InformationModel node-management services | net10.0 | 29 | 1 | | NodeState / BaseInstanceState | net48 | 578 | 0 | | Core certificate validation, error reporting and TCP buffers | net48 | 111 | 0 | | Server node management, certificate and session-security selection | net48 | 112 | 0 | All selected runs exited successfully at normal concurrency: **6,602 passing test executions**. Modified test assemblies were explicitly rebuilt, and the combined name/identity regression was verified by name in both net10 and net48 results. No skips or weakened assertions were introduced. The final `dotnet build UA.slnx -c Release --no-restore -m:1` with `CustomTestTarget=net10.0` succeeded with **zero warnings and zero errors**. The resolved source and adjusted tests pass whitespace verification, and their source hashes match the validation inputs. This is merge-specific verification. Earlier review results and the previously reported incomplete full Subscriptions runs below remain scoped to their own commits; they were not reopened or relabeled as passing by this operation. ## Previous upstream merge (2026-09-15 morning) Merge `c4694e20ab089c555af233642042133ff3a03ac8` integrates `master` through `484d6c887`, retaining #4481's shared Like parser/GDS query fixes, #4488's RequestHandle recovery in ServiceFaults, and #4487's expired-session and durable-transfer corrections. Three conflict resolutions preserve both sides: - Alias searches now use the shared `LikePattern` implementation, including upstream malformed-list validation. A timeout overload retains this branch's 100 ms per-alias deadline and `BadTimeout` reporting without keeping a second wildcard translator. The shared matcher's original unlimited overload remains unchanged for existing GDS/ContentFilter callers. New regressions cover bounded prefix/list scans, reuse after timeout, and the alias service result. - Expired activation closes outside the session-manager gate using upstream's once-only timeout claim, audit, and diagnostic accounting. This branch's original-channel and security-policy checks remain in place. - TCP buffer tests retain this branch's ownership, poisoning-pool and post-decryption checks alongside the upstream RequestHandle/fault overload tests; neither set was discarded. The upstream HTTPS binary behavior is intentionally retained: a decoder `ServiceResultException` produces HTTP 200 with an OPC UA ServiceFault carrying the decoder status, response timestamp, and recoverable RequestHandle, rather than the previous HTTP 500 text response. Other exceptions retain their existing HTTP error handling. | Local validation | Framework | Passed | TRX NotExecuted | | --- | --- | ---: | ---: | | Core, complete project | net10.0 | 5,054 | 88 | | Sessions, complete project | net10.0 | 780 | 610 | | Server aliases, expiry/security and subscriptions | net10.0 | 288 | 0 | | GDS database, paging and live directory calls | net10.0 | 263 | 0 | | Shared Like patterns and bounded evaluation | net10.0 | 109 | 0 | | Subscriptions transfer/durability/service selection | net10.0 | 23 | 1 | | HTTPS WebApi, complete project | net10.0 | 346 | 0 | | InformationModel aliases | net10.0 | 54 | 5 | | Shared Like patterns and bounded evaluation | net48 | 109 | 0 | | Core fault handles, TCP/HTTPS and filters | net48 | 268 | 0 | | Server aliases, expiry/security and subscriptions | net48 | 288 | 0 | These selected runs exited successfully with normal test concurrency: **7,582 passing test executions**. Sessions' console reports 379 skips; the TRX also contains conditional/assumption exclusions, so the table uses the actual 610 NotExecuted records. No skips or weakened assertions were added. The new `EvaluationTimeoutBoundsWorkAndPreservesPatternReuse` and `SharedPatternTimeoutRemainsAnAliasServiceError` tests passed on both targets. **Validation limitation:** two unfiltered Subscriptions runs did not produce a final result and were stopped, including a retry with a hang guard. They are not counted as passing. The isolated failover fixture passed 2/2, and the transfer/durability/service selection above passed. The stall's cause was not established; no unrelated code or test was changed to conceal it. The final `dotnet build UA.slnx -c Release --no-restore -m:1` with `CustomTestTarget=net10.0` succeeded with **0 warnings and 0 errors**. Whitespace verification passed for all hand-reconciled files, and staged source hashes match the validation inputs. These are merge-specific checks, not a new full platform/native/soak acceptance sign-off. Earlier review and acceptance evidence below remains dated to its own commit. ## Previous upstream merge (2026-09-14) Merge `c3e09553e4d1509502d4ba751960d58491bea2cf` integrates `master` through `6313e38a0` (#4478), including the CTT-driven Part 13 aggregate corrections and FindAlias argument validation. The upstream tests and CTT documentation are retained. The single conflict in `AliasNameWildcardMatcher` is reconciled through the branch's shared regex compiler: upstream malformed-pattern rejection and `IsValidPattern` coexist with OPC UA character escaping, whole-string/newline matching, the 100 ms timeout, and service-level error reporting. The upstream ReferenceTypeFilter checks and chronological aggregate behavior are preserved. Local merge verification: | Suite / selection | Framework | Passed | Existing skips | | --- | --- | ---: | ---: | | Server aliases, aggregates and historian | net10.0 | 1,011 | 0 | | Server aliases, aggregates and historian | net48 | 1,011 | 0 | | InformationModel aliases | net10.0 | 54 | 5 | | Features aliases | net10.0 | 11 | 0 | | Client aliases | net10.0 | 28 | 0 | | History aggregate/historical-access consumers | net10.0 | 265 | 0 | All selected tests passed at normal concurrency. The final net10 solution build passed with 0 errors and 0 warnings. The resolved file's whitespace check and the staged diff check passed. These are merge-specific checks, not a new full platform/native/soak acceptance claim. ## Previous upstream merges Integrates `master` through `8e874d073`, including the Client, Types, Core and source-generator audit fixes in #4471, #4473, #4474 and #4475, plus the event queue-size revision from #4480. The merge commits are `150ea0439` (26 conflict resolutions) and `a0e17c07c` (the base update that arrived during validation). The overlapping fixes are reconciled rather than replacing either branch: - **Rejected-certificate retention follows this branch, as explicitly requested:** zero means unlimited history; negative values disable new storage while preserving existing backend trimming behavior (Directory clears prior history; shared key/value leaves it intact). The processor and manager policy are not redefined, and documentation/tests describe the distinction. - Preserve owned certificate snapshots and borrowed validation-core lifetimes alongside upstream cache invalidation, store-first CRL checks and `BadCertificateRevocationUnknown` for unsupported revocation lookup. - Combine exactly-once chunk ownership, receive-loop cancellation, stale-epoch rejection and detach-before-adoption with upstream signature-before-padding verification and secret cleanup. - Preserve pooled `HttpClient` per-request configuration together with bounded response reads and deadlines that remain active through body consumption. Regression cases exercise already-used clients and both timeout precedence cases. - Retain the binary-compatible channel-key constructor and normalize omitted profiles; preserve exact profile filtering and upstream binary-first HTTPS/WSS endpoint ordering. - Preserve monotonic lease expiry and stale-confirmation fencing while adding ordered, non-reentrant leadership notifications outside the state lock. ## Validation Local validation of the conflict-resolution merge: | Suite | Framework | Passed | Existing skips | | --- | --- | ---: | ---: | | Core | net10.0 | 4,940 | 88 | | Client | net10.0 | 2,278 | 0 | | Server | net10.0 | 6,088 | 5 | | Types | net10.0 | 10,394 | 1 | | SourceGeneration.Core | net10.0 | 3,940 | 8 | | SourceGeneration | net10.0 | 185 | 0 | | Core | net48 | 4,855 | 104 | After the subsequent four-file #4480 update, the related Server slice passed 811 tests with 5 existing skips, the real durable queue-size cases passed 6/6, and EventManager's 21 cases passed again after analyzer-only expected-array cleanup. The final `dotnet build UA.slnx -c Release` with `CustomTestTarget=net10.0` succeeded: 0 errors and 4 pre-existing CA2249 warnings in unchanged fuzz-test code. Whitespace verification passed for the resolved files and the follow-up. Merge-specific proof includes `RequestTimeoutRemainsActiveWhileReadingTheOpenApiBodyAsync` on already-used shared clients with either the request or HttpClient timeout shorter, the existing ownership/expiry regressions from both branches, and both branches' retention tests adjusted only to the explicitly chosen zero/negative contract. Earlier acceptance at `1c4a9df6c` passed hosted Linux/macOS CI, CodeQL, image builds and the canonical coverage gate (87.15% line, 77.27% branch, 92.14% patch). Both Framework project matrices, all three Windows native suites (183 cases), and the unfiltered net10 stress project (230 cases, including full 60-minute chaos and 30-minute memory soaks) also passed. Those are **pre-merge results**, not a claim that the full framework/platform/native/soak matrix has been rerun on this merge. The previous acceptance workflow remains separate from this review-feedback operation. Security checks, meaningful assertions, leak detection, default native concurrency, full soak durations and coverage thresholds remain intact. The user-set ready-for-review state is unchanged. No architectural or complete merged-head acceptance sign-off is claimed. ## Related Issues - Fixes #4476 ## 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.	2026-09-16 04:42:09 +00:00
Marc Schier	da7412c429	Add native xRegistry events and distinct Resource versions (#4396) ... ## Summary - support the xRegistry 0.6.0 distinct Resource/Version hierarchy: `Registry -> Group -> Resource -> Versions -> Version` - instantiate stable logical Resource nodes separately from exact Version nodes while preserving existing Version NodeIds - delegate logical Resource FileType operations and Version-view metadata to the selected default Version with handle pinning and shared writer reservations - make Resource and Version delete behavior role-stable and use `MetaEpoch` / `Epoch` respectively - use stable Resource SourceNodes, exact Version SourceNodes and the `Server -> Registry -> Group -> Resource -> Version` notifier chain - keep a model-metadata-gated client fallback for 0.5 flat Servers; 0.6 Servers expose no legacy aliases - target `HasWoTProjection` at the real stable logical Resource node - use `AsyncCustomNodeManager` for the registration, fast-path, and federation managers, with awaited lifecycle, node mutations, event delivery, and cleanup - handle startup rollback, reentrant deletion callbacks, undisclosed file handles, and retryable cleanup without blocking unrelated file operations ## Authoritative dependencies - companion specification: OPCF-Members/spec-drafts#32 at `d3399fca025025a73e5a6b599f850a81d985c060` - OPC UA binding: xregistry/spec#568 - xRegistry event semantics: xregistry/spec#563, merged as `66ee31e47fbadff6ab982ae1516f7d9065e46b64` - companion NodeSet SHA-256: `369f5f3f1814b333e4eeb52bfb4b6a730cdff372505a5f91a54c94f0016ac4cf` ## Review feedback - keep the WoT Connectivity information-model designation at `1.1`; draft revisions update the publication date without changing the model version - eliminate all 198 unique CA1861 diagnostics across 54 files and remove seven obsolete project suppressions - fix the analyzer-clean touched-path CA1307 and the hierarchy follow-up CA1846/CA1851 diagnostics without dropping legacy-TFM compatibility - recover the reconcile queue after callback faults/cancellation and continue accepted pending changes - remove migration guidance for the xRegistry managers, which are new in 2.0 ## Compatibility - public WoT Connectivity model designation remains `1.1` - generated event-record ABI, constructors, options, NodeIds, and stored documents remain compatible - the generic xRegistry managers are new in 2.0 and implement `IAsyncNodeManager`; use `IAsyncNodeManagerFactory` and the async lifecycle hooks (see `docs\XRegistry.md`) - persistence schema and document blobs remain compatible unless noted below ## Validation - merge validation at `980422f76` with upstream `e8bec27d5`: 454 targeted WoT/materialization/model/registry cases and all 327 XRegistry cases passed on both net10.0 and net48; all 29 net10 aggregation sample cases passed - feedback regressions at `308e8ba74`: 55 targeted queue and live registry event/projection/lifecycle cases passed on each of net10.0 and net48 - prior CI: all 266 checks passed at `6489a0908`; the unchanged Ubuntu Core certificate-counter test and dependent jobs passed on the one-time retry ([run 34117304035, attempt 2](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/34117304035/attempts/2)) - `Opc.Ua.XRegistry.Tests` — 327 passed on net10.0 and net48 for migration commit `32c914aaf` - `Opc.Ua.WotCon.Tests` net10.0 — 1348 passed - `Opc.Ua.WotCon.Bindings.Tests` net10.0 — 683 passed - `Opc.Ua.SourceGeneration.Core.Tests` net10.0 — 3807 passed, 8 skipped - `Opc.Ua.SourceGeneration.Tests` net10.0 — 145 passed - `Opc.Ua.SourceGeneration.Stack.Tests` net10.0 — 94 passed - affected XRegistry and WoT client/server projects build successfully - xRegistry NodeSet is byte-identical to the companion artifact - repository-wide CA1861 inventory — 198 unique diagnostics across 54 files reduced to zero - targeted changed-fixture validation — 1,098 passed, 3 skipped, 0 failed - constrained full `UA.slnx` net10.0 Release build — CA1861 = 0 - `Opc.Ua.XRegistry.Client` net472 and netstandard2.1 builds — 0 warnings, 0 errors - independent shared-array safety review — no significant issues - `git diff --check` The final review regressions cover concurrent reconcile/open, synthetic logical handle collisions across default switches, cross-session Close isolation, shared writer reservations, logical FileType property/change propagation, logical Labels delegation, role-fixed deletion, event sources, model-version gating, and stable `HasWoTProjection` targets. The companion specification and protocol binding are reviewed in the linked PRs above. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5c79a1ed-6dab-4522-a9fc-6548caa78ad5 Copilot-Session: b605797f-a97a-4cd0-8f12-016d33c2fd88 Copilot-Session: 7ef369a5-2307-4f71-8a11-15af0e72f240 Copilot-Session: e716d455-8397-4d22-9c5a-398847ab7317	2026-09-08 04:46:40 +00:00
romanett	414957c88a	Reduce ISubscription: internalize the publish pipeline, delete the dead members (#4349) ... # Description Reduces the server subscription API surface per the measurements from issue #4185. ## ISubscription: 42 → 30 members (#4185) A call-site census of the 11 "publish-pipeline protocol" members plus `Publish` (all 68 `src/` projects, samples, tools, fuzzing, tests) found **zero callers outside `Opc.Ua.Server`** and split the members three ways: - **Deleted (4):** `ItemReadyToPublish` and `ItemNotificationsAvailable` were dead — zero callers repo-wide and implementation bodies commented out since before 1.5.378 — the obsolete parameterless `SessionClosed()` had zero callers, and `TransferSessionAsync` (the one-shot direct transfer, which bypassed the reservation protecting a transfer against a concurrently closing source session) lost its last caller once the manager always uses the claim/prepare/commit protocol. - **Internalized (8):** `PublishTimerExpired`, `Acknowledge`, `PublishTimeout`, `SubscriptionTransferred`, `AvailableSequenceNumbersForRetransmission`, `QueueOverflowHandler`, `SessionClosed(ISession)` and `Publish` move to `internal interface ISubscriptionPublishPipeline : ISubscription`, implemented **explicitly** by `Subscription` (the `ISubscriptionMonitoredItemLifecycle` shape) so they are unreachable through the public concrete class too. `Publish` goes with them deliberately — it is the same hazard class (consumes notifications, advances sequence numbers) with the same single production caller. - **Kept public (1):** `ResendData` drops out of the issue's list — its only caller is the `Server_ResendData` UA-method handler in `DiagnosticsNodeManager`, resolving via `TryGetSubscription` like `GetMonitoredItems` does. It is service-facing, not pipeline. ## The pipeline contract is complete: SessionPublishQueue traffics only in it `SessionPublishQueue` becomes `internal sealed` (zero consumers outside the assembly; `SentMessageQueue` already was) and works **solely with `ISubscriptionPublishPipeline`** — no `ISubscription` and no concrete `Subscription` anywhere in the file. To get there, the internal transfer-claim protocol (`TryBeginTransfer`, `PrepareSessionTransferAsync`, `CompleteTransfer`, `AbortTransfer`, `TryRestoreSessionAfterFailedTransfer`) moved from `internal` methods on the concrete class onto the internal interface, implemented explicitly. That completes the contract, so `TransferSubscriptionsAsync` loses its two-tier shape: the ~6 `subscription as Subscription` null-guard branches and the degraded `TransferSessionAsync` fallback for non-`Subscription` implementations are gone — one code path, always the claim/prepare/commit/rollback protocol, and `ISubscription.TransferSessionAsync` is deleted with it. Subscription creation fails with `Bad_InternalError` when a `CreateSubscription` override returns a type not derived from `Subscription`, instead of publishing partially — such implementations were already second-class (transfer required the concrete type before this change). This is the headline behavior change for exotic extenders and is documented prominently in `docs/migrate/2.0.x/sessions-subscriptions.md`. The durable-subscription open question in the issue is answered **no**: `Subscription/Persistence/` never references `ISubscription` (the store deals in `IStoredSubscription` DTOs; restore constructs the concrete type via `Subscription.RestoreAsync` inside the manager), so the internalization cannot break a store implementer. ## #4186: measured, prototyped, and deliberately kept as-is Body inspection showed only 4 of the 10 `ISubscriptionManager` wrappers are pure lookup-and-delegate (`Republish`, `SetTriggering`, `ModifyMonitoredItemsAsync`, `SetMonitoringModeAsync` — one caller each); the other 6 carry logic with no `ISubscription` substitute. A collapse of the four was implemented and then reverted on this branch: it traded ten uniform id-addressed members for a mixed dispatch shape in `StandardServer` without reducing what a reader must understand. **Every subscription service keeps going through `ISubscriptionManager`**; the wrapper bodies now resolve against the pipeline-typed registry. The decision is recorded in `plans/ServerRuntimeAnalysis.md` (open question 5). ## Compatibility treatment Removals are outright, per the documented no-`[Obsolete]`-shim policy for these interfaces (`docs/MigrationGuide.md` — re-adding an interface member would break every implementer; net48/netstandard2.0 have no default interface methods). In its place: - Migration analyzer **UA0030** (pipeline members + `SessionPublishQueue`, distinguishing deleted no-ops from internalized protocol), with tests, following the UA0024 syntax-based pattern. The transfer-claim members were never public in 1.5.378, so they need no migration story. - `docs/MigrationGuide.md#ua0030` section with before/after code; the `docs/migrate/2.0.x/sessions-subscriptions.md` section that told implementers to *add* `SessionClosed(ISession)` is replaced with the derive-from-`Subscription` story; the 2.0.x README symptom table routes the rule. - All 11 removed/internalized members existed in 1.5.378 (verified against `master378`), so each is documented as a 2.0 break. ## Tests Converted, not deleted. `Mock<ISubscription>` setups of pipeline members become `Mock<ISubscriptionPublishPipeline>` — Moq proxies the internal interface via a new `InternalsVisibleTo("DynamicProxyGenAssembly2")` (keyless; the SDK appends `$(PublicKey)` under `SignAssembly`, and signed pipelines do not run tests, matching the existing `Opc.Ua.Redundancy.Kubernetes` precedent). A test-side extension shim forwards the original member names (pipeline + transfer protocol) so the direct call sites on concrete `Subscription` instances compile unchanged. The transfer-race tests' typed reflection probes into `m_abandonedSubscriptions` follow the field's new type. - `Opc.Ua.Server.Tests` (net10.0): 4795 tests, 0 failures - `Opc.Ua.Subscriptions.Tests`, `Opc.Ua.Subscriptions.Durable.Tests`, `Opc.Ua.Subscriptions.Classic.Tests` (net10.0): green - `Opc.Ua.MigrationAnalyzer.Tests`: green incl. the new UA0030 tests - net48 gate: solution build green; subscription-area fixtures green (the known pre-existing local hosting-test environment failures reproduce identically on unmodified master) ## Related Issues - Fixes #4185 - #4186 was measured and prototyped on this branch and deliberately **not** taken (see the section above); the decision is recorded in `plans/ServerRuntimeAnalysis.md`. ## 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. _(net10 suites fully green locally; net48 build green and subscription-area fixtures green — the remaining net48 hosting-test failures on my machine reproduce identically on unmodified master.)_ - [ ] 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 Fable 5 <noreply@anthropic.com>	2026-08-31 10:53:51 +00:00
Marc Schier	cef83550cb	OPC UA Vision: a vision-guided bin-picking demo an LLM agent can see and drive (#4235) ... # Description Implements **OPC UA — Vision** and uses it to build the demo the companion specifications were drafted for: an eye-in-hand camera watches a bin of parts, a language model *looks at the frame*, decides which part matches the instruction, and commands the robot through Robot Intent — with the OpenUSD viewer showing the arm move and the bin empty out. > **Draft, and stacked.** [#4195](https://github.com/OPCFoundation/UA-.NETStandard/pull/4195) has merged and master is merged in, so what is left on top of `master` is the Vision work plus the unmerged `marcschier/ai-model-management` branch, which the demo needs for the model/dataset/deployment provenance. Review the Vision commits; the AI Model Management work lands with its own PR. ## What is here **The specification, implemented.** `Opc.Ua.Vision` is generated from the spec's own `Opc.Ua.Vision.NodeSet2.xml`, vendored byte-identically (SHA-256 `79C592C6…`, verified against a fresh download and stored LF-preserved like the Robot Intent NodeSet). `Opc.Ua.Vision.Server` builds the §4.2 discovery object, sensors, the coordinate-frame tree, calibrations, media endpoints, inference pipelines and the §9 feedback methods, with rendering and inference behind injectable providers. `Opc.Ua.Vision.Client` is built on the generated ObjectType proxies and composes transforms across the frame tree, which is what turns a detection in camera coordinates into a pose a robot can act on. **Two perception paths behind one contract**, which is exactly what `InferenceLocationEnum` is for. `OnServer` is a deterministic detector derived from the stage's ground truth — no model, no network, no GPU, so CI can exercise the whole loop. `EdgeOffServer` is the agent: it sees the frame over MCP and calls `SubmitDetections`, and the Server publishes results it did not compute. A client reads `DetectionResultType` identically either way. **The agent's eyes.** `tools/Opc.Ua.Mcp.Vision` adds 22 `vision_*` tools, which with the four shared connection tools makes the `vision` profile 26; `vision_get_frame` returns the camera image as an MCP `ImageContentBlock`, so the model looks at pixels rather than reading a description of them. Tool profiles now compose — `--profile vision,robotics` yields 62 tools — so one agent can both see and act without dragging in the full 136-tool catalogue. **The cell.** `samples/Robotics/BinPickingCell` follows the spec's own Robotics-Vision Addendum: the `world → robot_base → flange → gripper_tcp` frame tree with `camera_eih` on the flange, the `HandEye` extrinsics and the `Intrinsics612x512` intrinsics — the addendum's 2448×2048 calibration scaled to the 4×4-binned grid the camera actually delivers. `samples/Robotics/BinPickingClient` hosts the composed MCP catalogue and optionally the viewer. **Documentation.** `docs/Vision.md` (~800 lines) covers the model, hosting, the build context, every topology builder, both perception paths, feedback, the MCP profile and the limitations; linked from `docs/README.md`, with the profile table in `docs/McpServer.md` and READMEs for both samples. ## Evidence, not assertions Each of these was measured on a running system rather than assumed: | Claim | Evidence | |---|---| | The camera sees the bin | 612×512 frame with the bin and all five parts in view — red `(212,47,47)`, green `(61,193,71)`, blue `(54,85,218)`, yellow `(215,196,58)`, orange `(209,119,55)` | | An agent can act on what it sees | Over MCP, `vision_get_frame` returns a 612×512 PNG that decodes from base64 to 1,254,051 bytes, the descriptor beside the detections says 612×512, and all five `BoundingBox2D` centres land **inside** the frame on the pixel colour their own detection claims | | The geometry is right | A detection composed `camera_eih → world` lands on the authored position, residual **0.0000 m** — on both the on-server and off-server paths | | The detector tracks the world | Placing a part drops the next scan from 5 detections to 4, with that part absent | | The robot obeys the agent | Scripted run took command authority and completed a pick and a place, both `Succeeded` | | Tool catalogues are honest | Counted from a running server: vision 26, robotics 40, `vision,robotics` 62, full 136, **zero duplicates** | | Every node is browsable by NodeId | Eight representative NodeIds — both folders, a pipeline, a frame, the sensor, its Calibrations folder, the HandEye calibration, the Media folder — all `Good` | That tool-count check matters because the profile tests collapse names into a `HashSet` and *cannot see* a duplicate registration — so the new tests count registrations, not names. ## Things that were wrong, and are not now Writing the tests found three defects of one kind — code that answered instead of refusing. A zero-norm quaternion was silently rewritten to identity, so a caller with an all-zero orientation got a confident wrong grasp pose. `SubmitCorrection` rewrote a null `ResultId` to empty and called the sink anyway. A bad `StageIdentifier` **killed the host process** inside native `UsdStage.Open`, via an exception .NET cannot catch. All three now refuse loudly and are pinned by tests. **Nodes that existed but could not be reached.** The node manager indexes the Vision root before configurators run, so everything the fluent builder grafted on afterwards — both folders, the pipelines with their Results and Feedback, the frames, the sensors with their optics, calibrations and media endpoints — was never added to the index. Browsing *forward from a parent* worked, because that walks `NodeState.Children` in memory; browsing any of those nodes *by its own NodeId* returned `BadNodeIdUnknown`, which is how an ordinary client and the MCP discovery tools navigate. Registration is now deferred and flushed after each configurator. The first fix covered only the DI path and left the documented non-DI fallback broken in exactly the same way, so `ConfigureVisionAsync` now exists and registers what it builds. That defect survived a green suite because every test browsed children. The new tests assert by NodeId, and were checked against the broken code — with the flush removed, they fail. **Every Method was uncallable.** The Server passed 426 unit tests while not one of its Methods could be invoked over a real session — `RunInference` returned `Bad_TooManyArguments`, and so did every other Method that takes arguments, which is all of them but `StartContinuous` and `Stop`. Three independent faults, each sufficient on its own: the Method nodes were never created (they are Optional children, and the dispatcher guards every attachment with a null check, so it silently attached nothing); nothing declared the arguments, so the stack concluded none were expected; and `MethodDeclarationId` pointed at a synthesised NodeId rather than the NodeSet declaration a client calls with, so the lookup missed the instance Method entirely. The builder now creates the Methods, the `Results` folder and the `Feedback` object when a provider or sink is configured, declares every signature, and names the right declaration. Unit tests could not have caught this: they invoke the handler delegates directly and never go through `MethodState.Call`. It took an end-to-end test over a real session. Relatedly, children created by the generated `CreateOrReplace` helpers carry no `ReferenceTypeId`, so nothing could reach them — fixed once in the build context rather than at the several dozen call sites. **And one where I was wrong.** Earlier in this branch I reversed my own agents' decision and made `SubmitDetections` accept an empty detection set, reasoning that "I looked and the bin is empty" is a correct observation and refusing it forces a correct agent to invent a detection. The reasoning is sound; it is also an argument *against* the specification rather than an implementation of it. §9.5 is normative and explicit — its StatusCode table lists `Bad_InvalidArgument` for "`Detections` empty; or `SubmitCorrection` supplies both or neither corrected array". Worse, **none of §9.5's argument rules were enforced anywhere** — the dispatcher forwarded every submission straight to the feedback sink, so conformance depended on whichever sink a host installed. They are Server obligations, so they now live in the dispatcher, ahead of the sink, proven by strict mocks that would throw if the sink were consulted. The usability problem is real and does not go away by conforming, so it is raised where it can be fixed: [opcua-drafts#70](https://github.com/marcschier/opcua-drafts/issues/70). ## Tracking the updated drafts The Vision, Robot Intent and AI Model Management NodeSets on `opcua-drafts` `main` have moved, partly because of feedback this branch raised. All three are re-vendored byte-identically — verified by comparing git blob hashes against the upstream files rather than by eye — and the implementations follow. **Two statements that were previously inexpressible now have a wire representation.** `SubmitDetections` takes `SceneIsEmpty` and `SubmitCorrection` takes `RetractAll`, so an agent can report "I examined this frame and there is nothing in it" — the terminating condition of the bin-picking task — and can retract a false positive by correcting a result down to nothing. Both are checked in both directions, because the flag is exactly what separates a deliberate empty observation from a lost payload: an empty array without the flag is refused, and the flag with an array attached is refused as two contradictory claims about one frame. Implemented in the dispatcher, the client, the MCP tools the agent drives, and the sample cell. Also absorbed: `LampType` and `LightingMode` became enums (§66), `ProfileName` became `DefaultProfileName` on the endpoint (distinct from the `ProfileName` a caller passes to `GetStreamEndpoint`, which keeps its name), AI `Invoke`/`InvokeAsync` gained `PayloadUri` under the same exactly-one rule, and AI `ListModels` gained a `ContinuationPoint` — because `MaxResults` alone puts everything past the bound permanently out of reach. **OpenUSD is on `0.9.0-alpha`**, verified on the running cell rather than by restore alone: D3D12 backend, 612×512 frame, the bin and all five parts in view and detected — the same result `0.8.0-alpha` produced. The one local workaround that survives is `VisionMethodArguments`, and its reason has changed rather than gone away: the builder materialises these Methods as Optional children through `CreateOrReplace`, which constructs the state object directly and never runs the generated factory that now carries the signatures. Checked rather than assumed — removing the declarations fails five of the eight `VisionMethodSurfaceTests`. ## Review feedback Three threads, all addressed in `0fe2803a3`: - **The AI Model Management work is now a library family, not a sample.** `src/Opc.Ua.AI` (model), `.Inference` (`IInferenceBackend` and backends), `.Server` (node manager) and `.Client` (discovery, reads, calls, artefact transfer), in the same shape as the Robotics and Vision families; the samples become `samples/AI/ModelManagementServer` and `ModelManagementClient`. The client library needed real work rather than relocation: its browsing surface was `private` inside the sample's scenario runner, so moving the file alone would have produced a library nobody could call. - **No cloud-vendor SDK is referenced anywhere.** Inference goes through `Microsoft.Extensions.AI` — an `IChatClient` is implemented by hosted services and on-device runtimes alike, which is exactly the property clause 8.1 asserts. `Azure.Identity` is gone: workload identity is not an Azure feature, every platform projects a token into a file and each SDK reads that file behind its own API, so the resolver reads it directly. The comment claiming `Microsoft.Extensions.AI` would force a `System.Text.Json` bump was stale — it pins 10.0.10, which this repository already centralises on. - **`BinPickingCell` and `BinPickingClient` moved into `samples/Robotics/`**, which is where they belonged: the cell was already linking the arm, its kinematics and both USD assets out of `IntentEnabledRobot`. ## Upstream Work that belonged elsewhere went elsewhere rather than being worked around locally: - **`openusd-dotnet` [#13](https://github.com/marcschier/openusd-dotnet/pull/13)** — merged and shipped in `0.8.0-alpha`. `SilkFrameCapture.Capture` rendered only the first capture of a session and silently returned a blank frame for every one after it. Also filed [#14](https://github.com/marcschier/openusd-dotnet/issues/14) (no camera-by-prim API) and [#15](https://github.com/marcschier/openusd-dotnet/issues/15) (skipped tests reported as passed — which fooled me for several runs). Both are fixed; this branch takes `0.8.0-alpha` and deletes two workarounds, including 143 lines of hand-rolled projection maths. - **`opcua-drafts` [#66](https://github.com/marcschier/opcua-drafts/issues/66)–[#69](https://github.com/marcschier/opcua-drafts/issues/69)** — a close read of the spec produced 22 candidate ambiguities; 12 were discarded as misreadings with clause citations and the remaining 10 filed. One is concrete: five members exist in the NodeId CSV that no clause documents. - **`opcua-drafts` [#70](https://github.com/marcschier/opcua-drafts/issues/70)** — §9.5 could not express an empty observation or a false-positive retraction. **Adopted**, with the explicit-flag resolution argued for: `SubmitDetections` gained `SceneIsEmpty`, `SubmitCorrection` gained `RetractAll`, and the corrected-array rule relaxed from *exactly one* to *at most one*. - **`opcua-drafts` [#71](https://github.com/marcschier/opcua-drafts/issues/71)** — the Vision NodeSet browse-named all 13 `InputArguments` / `OutputArguments` Properties in the Vision namespace rather than the base namespace, so a stack could not find them and every Method in the specification was uncallable. **Fixed**; this branch now tracks the corrected artefact. ## The defect underneath it all The end-to-end suite was removed earlier in this branch because nine of its fourteen tests failed on one symptom: a client could not open a pipeline's `Feedback` object or enumerate its `Results`, even though browsing the same parent listed both. Restoring it meant explaining that, and the explanation was not in Vision. `RelativePathElement` initialised **`IsInverse` to `true`**. A caller that sets a `ReferenceTypeId` and a `TargetName` — which reads as *"the child reached by this Reference"* — was therefore asking the Server for the **inverse** Reference, and got `Bad_NoMatch` with nothing in the request to suggest why. `ObjectTypeClient.ResolveChildNodeIdAsync` is such a caller, and it is what **every source-generated Optional-child accessor in the stack** is built on — the accessor whose own documentation cites `AlarmConditionTypeClient.GetShelvingStateAsync` as an example. Vision is merely where it surfaced, because Vision puts two Optional children on the path an off-Server agent must take. The sibling helper in `StateMachineTypeClientExtensions` already set `IsInverse = false` by hand, which is the shape of a trap rather than a default. The default is now forward, `ResolveChildNodeIdAsync` says so explicitly, and the existing test that had recorded `true` as *expected* now pins `false` with the reasoning. Of the forty `RelativePathElement` construction sites in the tree, that accessor was **the only one relying on the default at all** — every deliberate caller already set it. Two further defects were found underneath it, both real and both fixed: - The generated `CreateOrReplace` helpers construct a child state object directly and leave **`TypeDefinitionId`** unset as well as `ReferenceTypeId`. A child referenced by nothing cannot be browsed; one with no type definition is a malformed Object that a client filtering by type silently skips — which is exactly what `EnumerateResultsAsync` does. Normalisation moved to `VisionNodeManager`, so it also covers the case the builder cannot see: a result published **at runtime** by an inference provider, long after the address space was built. That path produced results a client could list but not read. - `Opc.Ua.AI` shipped without an `AssemblyInfo`, so the solution built with six `CA1014` warnings. I had initially "fixed" this in the Vision client with Browse-based fallbacks, on the strength of a probe that appeared to show `TranslateBrowsePathsToNodeIds` failing even for `Server → ServerStatus`. That probe was wrong: it used an unqualified `ReferenceTypeIds`, which inside a `Opc.Ua.Vision.*` namespace binds to *Vision's* `ReferenceTypeIds` and yields a nonsense NodeId. Chasing that to the end is what turned up the real cause. **The fallbacks are gone** — they would have hidden a stack-wide bug behind a Vision-shaped workaround, and one of them matched on browse name while ignoring the namespace index. `tests/Opc.Ua.Vision.Intent.Tests` is restored and back in `UA.slnx`, passing **14/14** on its own merit rather than through a client-side workaround. A registration test now pins the pipeline's `Feedback` and `Results` children, which nothing covered before. ## Running it, and what that found The demo's premise is that a language model **looks at the bin and reasons about what it sees**. Running it end to end showed it could not, for reasons no test covered, because every one of them lived between components. **The sample did not compile.** Two `CS8604` nullable errors with `TreatWarningsAsErrors`, so neither the cell server nor the demo could be started at all. Worth recording that my first explanation was wrong: CI *does* build this project - the Linux leg's `net10.0` pass compiles it, log-confirmed - so this is not a coverage gap in the workflow, and no workflow change was made. **`Pick` and `Place` never moved the arm.** They were a delay plus a gripper action, so the operations reported `Succeeded` while nothing travelled. They now solve IK once and interpolate in joint space. **And the parts never moved**: the arm swung but the bin stayed full, because nothing tied the held object to the world state. The connector now creates 11 monitored items where it created 6, and a scripted run delivers `SeqNo=151` across 10 messages where it delivered `SeqNo=3` across 1. **The frame was smuggled through a string field.** The media provider put a base64 *data URI* of the PNG into `VisionImageReferenceDataType.Uri` - a field meant to be a *reference*, while the bytes were already returned in the inline `ByteString`. Every response therefore shipped the image twice, and a 1,311,400-byte PNG became a 1,748,536-character string against a `MaxStringLength` of 65,535: **26.7x over**. That is why raising `MaxByteStringLength`, `MaxArrayLength` and `MaxMessageSize` never helped - none of them govern a `String`. Separately the MCP tool assigned raw bytes where `ImageContentBlock` requires base64, so the wire carried `"data":"�PNG..."` - every non-UTF-8 byte replaced by U+FFFD, destroying the image and inflating the response to 6.8 MB. **The camera was aimed at nothing.** The arm's home pose, the flange scan pose the Vision model declared and the joint angles in the USD stage were three independent claims about where the camera was, and they disagreed - the declared flange orientation pointed the view 86 degrees away from the projection camera. Solving them as one thing exposed two further constraints that are easy to miss: the camera has to sit off the tool axis or it photographs its own gripper, and the IK branch has to be elbow-back or a link parks under the camera and fills the frame with the arm's own upper arm. A third was a regression this branch introduced and `Opc.Ua.Robotics.Tests` caught - aiming a straight-down camera from a point on the base's own X-Z plane puts the wrist on the J4/J6 singularity, so every motion away from home fails to solve. A 15-degree camera roll, free because the camera still looks straight down, gets 25 degrees clear, which is the margin the poses before this branch had. **The frame and the detections were different images.** The sensor declared 2448x2048, the clip endpoint 1280x1024, and the renderer produced 640x512 - not even the same aspect ratio. The ground-truth detector projects through the declared intrinsics, so its boxes lived in 2448x2048 space while an agent was handed a 640x512 picture: *"pick the red cube you can see"* arrived with coordinates that pointed off the image. Everything now agrees at 612x512, an exact 4x4 bin of the native sensor with the intrinsics scaled to match; the native size stays in the model and serial number, where it describes the hardware rather than the image. **`LatestClip` was created and never written**, so it reported `Bad_NoDataAvailable` for the life of the Server and a consumer following the model - read the published frame, call the method only if there is none - never got a frame. The dispatcher now publishes a clip it has just encoded, for every Vision server rather than this sample alone. One genuine API gap turned up on the way: `OpcUaServerOptions` exposes `MaxByteStringLength`, `MaxArrayLength` and `MaxMessageSize` and **no `MaxStringLength`**, so a Server that legitimately needs a long string could not raise the limit through the hosting API at all. Added, with tests. It is not the fix for anything above - that was a design bug - but the missing knob was real. ## Not finished - **Two projects sit below the repository's 80% line coverage.** `Opc.Ua.Vision.OpenUsd` is at 71.1% — what is left only executes with a real OpenUSD stage and plugin tree, which is not on every build agent. `Opc.Ua.AI.Inference` is now at **86.7%**, up from 12.9%: the code this branch *wrote* is covered by 25 tests, and `RestChatCompletionsBackend` — previously the whole of the shortfall — is covered by 18 more against a stubbed `HttpMessageHandler`, exercising success, refusal, throttling, retry-after, timeout and cancellation without a network. Everything else clears the bar — `Opc.Ua.Vision.Server` 92.1%, `Opc.Ua.AI` 91.4%, `.Vision.Client` 88.1%, `Opc.Ua.Vision` 85.7%, `Opc.Ua.AI.Server` 80.1%, all measured with coverlet. The CI coverage gate reports itself as advisory and non-blocking. - **`SamplesCollected` is not counted here, and `docs/Vision.md` now says so.** Section 9.4 requires a Server to count a negative example (`SceneIsEmpty` / `RetractAll` carrying a `GroundTruthLabel`) exactly as it counts one carrying geometry. `VisionNodeManager` does not, and structurally cannot: `SamplesCollected` is a property of `LearningJobType`, which the *AI Model Management* companion defines, and Vision reaches the job through a `NodeId` value rather than a Reference **precisely so this model takes no dependency on the model that defines it**. What the Server does guarantee is that the negative example survives the hop intact — both flags are carried verbatim to `IVisionFeedbackSink` — so a host that binds a learning job has everything it needs to satisfy the clause on the counter it owns. Stated in the limitations rather than left implied. ## Related Issues Was stacked on #4195, now merged. No tracking issue yet for the Vision implementation itself — happy to open one if you would like the design recorded as an ADR before this goes further. ## 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. — *Types 8562, Server 4072, Client 2123, InformationModel 1063, Robotics 540, Tools 466, Vision 442, AI 74 and the restored Vision+Intent end-to-end suite 14/14, all green on net10.0. Only `.OpenUsd` (71.1%) is below the 80% bar — see Not finished.* - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. — *`dotnet build UA.slnx -c Release`: 0 errors, 0 warnings.* - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. — *net48: Types 8555, Client 2126, Vision 385 green. Two net48 Server tests fail (`CreateUserManagementSeamBindsTheModelToTheServerAsync`, and `DurableDataValueQueueVerifyReferenceBatchingAsync` only under parallel load); both were confirmed **pre-existing** by re-running them with the `IsInverse` change reverted, which fails identically.* - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. — *The net48 build legs failed because the restored end-to-end project used `ValueTask.FromResult` / `string.Create`, which do not exist on .NET Framework; fixed in `fb57522da` and verified building on every TFM. `DisposeDrainsHeldConnectionCallbackBeforeListenerDisposal` failed once on the Ubuntu Client leg — a 13 ms timing-sensitive reverse-connect disposal test that passes locally 2123/2123 and touches no browse paths.* - [ ] I have addressed **all** PR feedback received. ## Typed Vision-guided Robotics MCP workflows The branch now also makes the demo workflow first-class in the Full MCP profile: - replaces stringified Robotics intent and mission inputs with typed request schemas and exact name-or-NodeId selectors; - adds bounded operation/mission paging, stable step-to-operation identity, retained mission discovery, and `robotics_wait_mission`; - makes `vision_run_inference` return bounded typed detection, inspection, or segmentation summaries in one call; - adds same-session `robotics_vision_pick` to select a detection and submit Pick or Pick/Place work without implicitly taking authority, retrying, cancelling, or waiting; and - starts Publish workers for classic subscriptions so an independently launched OpenUSD viewer receives live robot and part updates. The migration guide documents the intentionally breaking MCP request-schema changes. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dab1302-19c5-4a9b-a50c-d97d389713aa Copilot-Session: a248a589-9a20-4372-868e-5d347e57001b Copilot-Session: 99d1ab42-f83a-41ab-bf3e-85bf81fadace Copilot-Session: bcbc8ac9-5abd-42a8-8bac-e10533790e2e	2026-08-29 19:34:15 +02:00
Marc Schier	fac30c45e6	Add MinSupportedSamplingInterval configuration and honor it when revising sampling intervals (#4243) ... # Description PR #4194 removed the unused `ServerConfiguration.MinMetadataSamplingInterval` and left nothing in its place. As a result there was no way to populate `Server.ServerCapabilities.MinSupportedSampleRate` (`i=2272`) — `ServerInternalData.CreateServerObjectAsync` hardcoded it to `0`. Separately, the only clamp applied when revising a monitored item's sampling interval was the per-node `BaseVariableState.MinimumSamplingInterval`, which defaults to `MinimumSamplingIntervals.Continuous` (`0`), so reference-server nodes accepted any requested interval (e.g. 10 ms) unrevised — exactly what was reported. This adds a configuration entry **in the exact schema slot #4194 vacated** and enforces it as a lower bound. ## Configuration - **`ServerConfiguration.MinSupportedSamplingInterval`** — `double` (milliseconds), default `0`, `[DataTypeField(Order = 17)]`, i.e. between `MaxNotificationsPerPublish` (16) and `AvailableSamplingRates` (18). - `ApplicationConfiguration.xsd` — `<xs:element name="MinSupportedSamplingInterval" type="xs:double" minOccurs="0" />` in the same position the removed element occupied. - Fluent API **`SetMinSupportedSamplingInterval(double)`** on `IApplicationConfigurationBuilderServerOptions`, in the place `SetMinMetadataSamplingInterval` was removed from. - The element is added to the six config XML files #4194 touched (reference / CTT / AOT / GDS use `0`; `test-server-full.xml` uses a distinctive non-zero value for the round-trip test). The address-space Property keeps its OPC UA spec name, so `Server.ServerCapabilities.MinSupportedSampleRate` and `VariableIds.Server_ServerCapabilities_MinSupportedSampleRate` are unchanged. ```xml <MaxNotificationsPerPublish>1000</MaxNotificationsPerPublish> <MinSupportedSamplingInterval>2000</MinSupportedSamplingInterval> <AvailableSamplingRates> ``` ```csharp application.Build(applicationUri, productUri) .AsServer([endpointUrl]) .SetMinSupportedSamplingInterval(2000); ``` ## Behavior - `ServerInternalData` now publishes the configured value to `Server.ServerCapabilities.MinSupportedSampleRate`. - New `SubscriptionManager.CalculateRevisedSamplingInterval(...)` (two overloads, alongside the existing `CalculateRevisedQueueSize`) centralizes the revision rule and replaces the logic that was duplicated across four call sites — the create and modify paths of `AsyncCustomNodeManager` and `CustomNodeManager` — which now expose a `MinSupportedSamplingInterval` property seeded from the configuration (mirroring how `MaxQueueSize` is initialized). The rule: 1. A requested interval below zero resolves to the *default sampling interval*: the **publishing interval of the subscription** on create, and the item's **current sampling interval** on modify. The modify case preserves 1.5.378 behavior, so a `ModifyMonitoredItems` call that leaves the sampling interval unspecified does not silently retune the item. 2. A node declaring `MinimumSamplingIntervals.Continuous` (`0`) for the `Value` Attribute reports **by exception** and is **not** bound by the server-wide minimum — the requested interval is returned unchanged. 3. Every other item is raised to `max(nodeMinimumSamplingInterval, MinSupportedSamplingInterval)`. Attributes other than `Value`, nodes declaring `MinimumSamplingIntervals.Indeterminate` (`-1`), and non-Variable nodes are bound only by `MinSupportedSamplingInterval`. 4. `double.MaxValue` is capped to one year (pre-existing behavior, now in one place). Event monitored items do not sample and are unaffected. **Backward compatible:** with the default of `0` the computation is identical to the previous code for every input, including `Continuous`, `Indeterminate`, positive node minimums, non-`Value` attributes and `NaN`. No existing configuration or deployment changes behavior. Node managers that implement `INodeManager` directly can call the new public helper to apply the same rule. ## Related Issues - Fixes #4191 The issue thread also asked whether `Subscription.MaxNotificationsPerPublish` is revised by the server. It is (`SubscriptionManager.CalculateMaxNotificationsPerPublish`), but it was untested and is not observable from the `CreateSubscription` response, which carries no revised field for that parameter. Four tests were added that assert it through `SubscriptionDiagnostics.MaxNotificationsPerPublish` on a privileged session, covering create and modify, the above-limit, below-limit and `0` (server default) cases. ## Tests - **`SamplingIntervalRevisionTests`** (17 unit tests, `Opc.Ua.Server.Tests`) — the full revision matrix: negative request resolving to the default interval, the `Continuous` bypass, `Indeterminate`, node minimum above *and* below the floor, a floor of `0` being a no-op, non-`Value` attributes, non-Variable and null nodes, and the `double.MaxValue` cap. - **`MinSupportedSamplingIntervalTests`** (10 integration tests, `Opc.Ua.Subscriptions.Tests`) — against a reference server configured with a 500 ms floor: the capabilities node reflects the configuration; a node declaring 100 ms is lifted to 500 ms; `Scalar_Static_Arrays2D_LocalizedText` (1000 ms) wins over the floor; `CurrentTime` (250 ms) is lifted; a `Continuous` node keeps its 10 ms; a non-`Value` attribute is floored; `ModifyMonitoredItems` behaves the same in both directions; event items are unaffected. - Restored the configuration builder and round-trip test coverage that #4194 deleted. - `TestFixture` gained a `protected virtual void ConfigureServer(ApplicationConfiguration)` hook so a derived fixture can vary the server configuration before start. Default implementation is a no-op, so no existing fixture changes behavior. ## Documentation - `docs/Subscriptions.md` — new **Sampling interval revision** section with the input table, the four-step rule (including both negative-interval fallbacks), XML and fluent configuration examples, and guidance for custom node managers. - `docs/migrate/2.0.x/configuration.md` — new section documenting that `MinMetadataSamplingInterval` was removed and `MinSupportedSamplingInterval` added. Verified against `master378`: the old setting defaulted to `1000` but was never read, and `MinSupportedSampleRate` was hardcoded to `0` with no server-wide floor. The guide therefore stresses that these are **two different settings, not a rename** — consumers should drop the old element (a default of `0` reproduces 1.5.378 exactly) and only opt into the new one deliberately, since reusing the old value would silently introduce a sampling floor. ## Validation - `dotnet build UA.slnx` — **0 warnings, 0 errors** across all TFMs. - .NET 10: `Opc.Ua.Server.Tests` 4091 passed, `Opc.Ua.Subscriptions.Tests` 619 passed (full suite including the new fixture), `Opc.Ua.InformationModel.Tests` 1063 passed, `Opc.Ua.Configuration.Tests` 221 passed, `Opc.Ua.Gds.Tests` 119 passed. - .NET Framework 4.8: new unit tests (200 incl. sampling groups), new integration tests (17) and configuration round-trip (135) all passed. - `ConsoleReferenceServer` starts successfully with the updated configuration file. ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79f8090c-b2a3-409c-a5bf-56b6d2a35e70	2026-08-12 17:41:29 +00:00
Marc Schier	5bc8a9d705	Reduce ISession: type the continuation points, delete the obsolete validate overload (#4199) ... ## Stacked on #4197 Base is `marcschier/server-bind-phase`. The full stack is **#4188 → #4193 → #4197 → this**. GitHub retargets each base automatically as the stack merges. **Review the earlier three first.** **#4217 has been merged into this branch** (squash `53dd4b618`), so the last remaining item from the issue — `ISession.SessionDiagnostics` — is now part of this PR. Closes #4187. Refs #4203 — **does not close it.** #4217 swapped `OnUpdateDiagnostics` onto `m_diagnosticsLock`, but it still calls `Variant.FromStructure(ServerDiagnostics)` with the default `copy: false`, which wraps the *live* structure in the `ExtensionObject`. The caller reads its fields after the lock is released, so the lock currently excludes nothing. #4222 adds `copy: true` and is what actually closes #4203. ## The finding that changed the approach `ISession` has **exactly one production adapter**: `src/Opc.Ua.Server/Session/Session.cs`. The other `ISession` implementations in the tree are the unrelated **client** interface (`src/Opc.Ua.Client/Session/Session.cs`, `RedundantClientSession`) and a migration-analyzer test stub. One adapter means the seam is hypothetical; two means it is real. The issue proposed splitting `ISession` into five concern-interfaces — that would turn one hypothetical seam into five. The interface earns its keep for testability and because node managers receive it through `OperationContext`, but that argues for making it **smaller**, not for multiplying it. **So: reduce, don't split.** Reasoning posted to the issue as [comment 5226712936](https://github.com/OPCFoundation/UA-.NETStandard/issues/4187#issuecomment-5226712936) so the five-way framing does not get re-proposed later. ## Measurement 38 members, 368 references. External production consumers are **4 files using 5 members**: | Consumer | Members used | |---|---| | `Opc.Ua.Redundancy.Server/Sessions/DistributedSessionManager.cs` | `Id`, `InitializeAsync`, `Dispose` | | `Opc.Ua.Di.Server/Locking/DefaultLockService.cs` | `Id` | | `samples/ConsoleReferenceServer/UAServer.cs` | `Id`, `Identity`, `ReadDiagnostics` | | `samples/Quickstarts.Servers/.../DataChangeMonitoredItem.cs` | type reference only | Every production caller of the five continuation-point members is inside `Opc.Ua.Server` (`MasterNodeManager`, `NodeManagerLifecycle`, `HistorianDispatcher`). **`ISession` is 38 → 32 members.** ## Commits ### `882b2937` Type the history continuation point `ISession` leaked `object` twice, against the repository rule: ```csharp void SaveHistoryContinuationPoint(Guid id, object continuationPoint); object? RestoreHistoryContinuationPoint(ByteString continuationPoint); ``` The erasure was **not** hiding a genuinely opaque plugin token. `HistorianDispatcher` is the only production caller, it always passes a `HistorianContinuationState`, and it casts straight back to that type on restore. The `object` was a round trip through the type system that bought nothing. `HistorianContinuationState` is `internal`, so it cannot appear on a public interface. `IHistoryContinuationPoint` names the concept instead: it carries an `Id` and extends `IDisposable`. Two latent bugs fell out of typing it: - The session disposed only those points that **happened** to implement `IDisposable` and **silently leaked the rest**. Every point is now disposable by construction, and three `as IDisposable` probes are gone. - Carrying the `Id` on the point removes the separate `Guid` parameter, which could disagree with the payload it was filed under. The mock stores in the historian tests had the same latent inconsistency. ### `0d020b77` Delete the obsolete synchronous `ValidateBeforeActivate` **Zero production callers** — the nine references were all tests written for it. `[Obsolete]` since 1.5.378, which the repository rules allow removing in master. It is also the divergent-signature problem the runtime analysis flagged: the async form returns the validated handler and policy as a tuple, while the sync one took the same two values as trailing `out` parameters, so a caller had to learn two shapes for one operation. The overload could not verify a user token requiring decryption, so on a secure endpoint it failed closed and told the caller to use the async path anyway. The one test asserting that fail-closed behaviour goes with the method it described; the path it pointed at is already covered by `ActivationValidationDecodesBinaryEncodedUserIdentityToken`. The other eight tests are **converted, not deleted** — the channel, signature, security-policy and token-policy validation they cover is still asserted, now against the path the server actually uses. ### `821e8302` Route the continuation points behind one member Five `ISession` members were one-line pass-throughs to `SessionContinuationPoints`, a 419-line module that already owns the behaviour and already has its own `IContinuationPointStore` seam. The interface restated the module's surface without adding anything. ```csharp // was session.SaveContinuationPoint(cp); session.RestoreHistoryContinuationPoint(bytes); // now session.ContinuationPoints.SaveBrowse(cp); session.ContinuationPoints.RestoreHistory(bytes); ``` The holder is `internal sealed`, so `ISessionContinuationPoints` is what makes it addressable. **This is composition, not the service locator `IServerContext` refused to be.** A subsystem with an independent lifetime should be injected; continuation points belong to the session and cannot exist without it. `InvalidateContinuationPoints` called `RemoveBrowseForManager` and `RemoveHistoryForManager` in sequence, so the pair becomes `RemoveForManager` — the caller's intent is that a node manager is going away, not that two collections need visiting. ## Tests Three harnesses had hand-rolled a `Dictionary<Guid, object>` to stand in for the holder. They now use the **real** one, so the dispatcher and `BrowseNext` meet the eviction and disposal rules a live session actually applies. **One hazard worth recording for future interface work.** A Moq member that is never stubbed returns `null`. Where the old code called a method straight on `ISession`, that `null` was harmless. Reaching *through* a property means an unstubbed `ContinuationPoints` null-derefs instead. Three tests failed exactly that way; they are wired to a real holder, not restubbed. | Suite | net10.0 | net48 | |---|---|---| | `Opc.Ua.Server.Tests` | 4032 passed, 1 failed | builds clean | | Touched areas (continuation points, historian, master node manager, session) | 898 passed | — | 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. `Opc.Ua.Redundancy.Server`, `Opc.Ua.Di.Server`, `Quickstarts.Servers` and `ConsoleReferenceServer` all build clean. ## Docs `docs/MigrationGuide.md` gains a "Removed members on ISession" section covering the `ValidateBeforeActivate` removal with the async replacement, and the `IHistoryContinuationPoint` change with before/after. ## The SessionDiagnostics removal (folded in from #4217) `ISession.SessionDiagnostics` handed out the whole mutable `SessionDiagnosticsDataType` — the structure the session's diagnostics lock protects — so a caller could read a field while the owner was writing it. Every server-side reader wanted exactly one value out of it, so the interface now exposes those two values (`SessionName`, `ClientApplicationUri`) directly and the structure stays behind the lock. That work landed here as #4217 rather than as a separate PR, and it also fixes #4203: `OnUpdateDiagnostics` now takes `m_diagnosticsLock` instead of locking the diagnostics payload it was handed. ## Reviewer questions - `ISessionContinuationPoints` exposes `MaxBrowse` as read-only while the concrete holder keeps a setter (the test framework adjusts it to exercise limits). Should the setter be on the interface, or is test-only mutation better left to the concrete type? - `RemoveForManager` collapses two removal methods. The concrete `RemoveBrowseForManager`/`RemoveHistoryForManager` remain public on the holder — worth making them private now that nothing outside calls them individually? --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041	2026-08-11 06:57:40 +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	636912bd4c	Harden subscription retirement after #4114 and refresh the client namespace table on model change (#4115) ... # Description Two things, both surfaced by the live NodeManager registration work in #4094. > **Rebased on #4114.** Upstream landed #4114 (`cc4667d0b`) while this PR was open; it fixes the > same #4113 root cause. `origin/master` is merged in and the conflicts were resolved **in favour of > the upstream API shape** — `CompleteAsync(IMessageProcessor, uint subscriptionId, ct)` with the > last known server id passed as an argument. The `IMessageProcessor.LastServerId` member this > branch had added is dropped again. What remains here is the orthogonal hardening #4114 does not > cover, plus all of the #4100 work. ## 1. Remaining subscription-retirement hardening (on top of #4114) #4114 fixed the id-collision itself. Three related holes in the same paths are still open: - **Retirement was not atomic with removal.** `RetireSubscriptionId` is now called *under* `m_subscriptionLock` from both `CompleteAsync` and `RemoveSecondaryPartitionAsync`. `GetById` resolves incoming publish responses under the same lock, so a worker could otherwise observe the subscription as removed while its id had not yet reached the retired-id ring — and send a redundant `DeleteSubscriptions` for an already-retired id. `RemoveSecondaryPartitionAsync` did not record the retired id at all. - **The retired-id ring was far too small.** `kMaxSubscriptionHistory` 10 → 256, so subscription churn cannot evict live history. - **The orphan delete was unguarded.** The publish worker no longer sends `DeleteSubscriptions` for an unresolved id while any subscription is still awaiting its id. A publish response can overtake the `CreateSubscription` continuation that assigns the id, and deleting in that window kills a healthy subscription. Leaving a genuine orphan alive until its lifetime expires is the safe trade-off. - **`PublishControllerAsync` lost resize signals.** It re-created its `AsyncAutoResetEvent` waiter on every loop iteration; the abandoned waiter stayed enqueued and swallowed a later `Set()`, so the publish worker pool stopped resizing. This is a second, latent starvation path independent of the id collision. The wait is now created once and only re-armed after it completes. ## 2. Managed client does not refresh the namespace table on model change (#4100) `NodeManagerLifecycle.NotifyCommittedChangeAsync` updates `NamespaceArray`, bumps `UrisVersion`, and then reports a **`BaseModelChangeEvent`** — an event shape that carries no `Changes` payload. The client tracker *did* observe it and cleared the `INodeCache`, but it held only an `IStreamingSubscription`, an `INodeCache` and an `ILogger`, so nothing could re-read the namespace table. A tracking client therefore resolved NodeIds from a namespace added by a live NodeManager registration against a stale `NamespaceUris` table. - **New `INamespaceTableRefresher`** (`NamespaceUris` + `FetchNamespaceTablesAsync`). The member signatures match what `Session` and `ManagedSession` already expose, so both satisfy the interface without any new members. `ISession` is deliberately left untouched — moving its members onto a base interface would be a binary break for 1.5.378 consumers. - **`ModelChangeTracker`** takes the refresher as an optional *trailing* constructor argument, which keeps every existing call site source-compatible and makes `null` the opt-out. A refresh runs when the event carries no per-node detail, when an affected node is `Server_NamespaceArray`, or when an affected namespace index is beyond the client's table. It runs **before** the cache is invalidated and before `ModelChanged` is raised, so subscribers that re-browse from the handler already see the new uris. A failing refresh is logged and swallowed. - **`ModelChangedEventArgs.NamespaceTableRefreshed`** reports the refresh to applications. - **`ManagedSession.EnableModelChangeTrackingAsync`** wires the session in, so opting into tracking is all an application has to do — no new option, no change to the positional `CreateAsync` signature. - **`ModelChange.TypeDefinition`** changes from `NodeId?` to `NodeId`. `NodeId` is a readonly struct implementing `INullable` and must not be wrapped in `System.Nullable<T>`; use `NodeId.Null` and `IsNull` instead. The type is 2.0-preview only (absent from 1.5.378), and the only producer is the tracker itself, which already assigns a non-null `AffectedType`. Per review feedback the type is also now a `readonly record struct`, matching `DataValueChange` / `EventNotification`. ## Tests Every new regression test was run against a temporarily re-introduced defect and **fails** there: | Test | Covers | |---|---| | `CompleteAsyncRetiresOnlyTheCompletedSubscriptionWhenOthersArePendingCreationAsync` | retire-by-identity (also guards #4114's fix) | | `CompleteAsyncRemembersRetiredServerIdAfterIdWasResetAsync` | retired-id history | | `PublishWorkerDefersDeletingUnknownSubscriptionWhileCreationIsPendingAsync` | orphan-delete guard | | `PublishControllerKeepsResizingAfterWorkersExitAsync` | publish-controller signal loss | | `ConcurrentAddAndCompleteRetireOnlyCompletedSubscriptionsAsync` | parallel add/retire registry integrity | | `ConcurrentSubscriptionChurnNeverStarvesASubscription` (integration) | #4113 repro against the reference server | | 7 × `ModelChangeTrackerTests` namespace-refresh cases | each trigger, no-trigger, throwing refresher, `null` refresher, ordering | | `LiveNodeManagerAddRefreshesTheClientNamespaceTable` (integration) | #4100 repro — times out without the wiring | `FakeMessageAckQueue` records both the completed id (upstream's assertions) and the completing instance (the identity assertion in `MessageProcessorTests`), so neither side's coverage was lost in the merge. Results after merging `origin/master`: - `Opc.Ua.Client.Tests` — **2119/2119** pass on net10.0; the new `ModelChange` and `SubscriptionManager` fixtures also pass on **net48**. - `Opc.Ua.Subscriptions.Tests` churn + coverage fixtures — **28/28** (net10.0); churn fixture also passes on net48. - `Opc.Ua.Server.Tests` `NodeManagerLifecycle*` — **49/49** on net10.0; the new fixture also passes on net48. - No new analyzer warnings; only the three pre-existing `CA1873` hits in `NodeCacheResolver.cs` and `ClassicSubscriptionEngine.cs` (untouched files) remain. ## Documentation - `docs/ModelChangeTracking.md` — new **Namespace table refresh** section with the trigger table, updated `ModelChangedEventArgs` / `ModelChange` snippets, updated manual-construction snippet, and a quick-reference row. - `docs/NodeManagerRegistration.md` — client-side half of the **Namespaces** section. No `docs/migrationguide.md` entry: every changed type is either `internal` or 2.0-preview-only. ## Related Issues - Fixes #4100 - Follow-up hardening for #4113 (closed by #4114) - Related to #4094, #3993 ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e22f9b3c-1682-40bd-8b72-31a11352fa2c	2026-07-30 12:17:00 +02:00
dpse	cc4667d0b5	Complete client subscriptions by identity instead of id (#4114) ... # Description Completes subscriptions by passing `IMessageProcessor` and last known server subscription id to prevent incorrect subscription removal when multiple subscriptions with id 0 exist. ## Issue During calls to `SubscriptionManager.CompleteAsync`, the subscription manager looked up the subscription entry to remove by the passed subscription id. But newly created subscriptions not yet having received an id from the server have their id set to 0, meaning that disposing an unrelated subscription also with id set to 0 could remove the wrong one (as there can exist multiple subscriptions with the same zero-id), which in turn could lead to the id no longer being resolved, and the publish worker treating it as unknown. Deleted subscriptions also have their id reset to zero, which could potentially clash as well. ## Fix - Internal interface `IMessageAckQueue` is changed so that `CompleteAsync` also takes `IMessageProcessor` as an argument. - `MessageProcessor` stores the known server id (if set) and passes itself along with that id to `CompleteAsync`. - With the `IMessageProcessor` argument, `SubscriptionManager` no longer needs to look up subscription by id and therefore removes the correct subscription. - Additionally, `SubscriptionManager` receives the non-zeroed server id and no longer enqueues zero-value ids to the subscription history. ## Related Issues - Fixes #4113 ## 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. --------- Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>	2026-07-30 09:33:50 +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	5b9bcedc62	Add OPC UA ISA-95 Common Model and Job Control support (#4089) ... # Description Adds first-class support for the OPC UA ISA-95 companion specifications: - OPC-10030 Common Model. - OPC-10031-4 Job Control V1. - OPC-10031-4 Job Control V2. The change introduces `Opc.Ua.ISA95`, `Opc.Ua.ISA95.Client`, and `Opc.Ua.ISA95.Server`, together with a NativeAOT-compatible minimal server sample and comprehensive tests. ## Highlights - Compiles the three canonical NodeSets and identifier mapping CSV files through the source generator. - Adds NodeSet identifier mapping validation, typed method arguments/results, companion event records, and generated method argument nodes. - Provides typed Common Model builders with normative relationships and an injectable `GeoSpatialLocationType` provider. - Implements lossless V1/V2 Job Control providers over a shared version-neutral store, including state paths, status events, comments, response queries, catalog changes, and cohesive DI registration. - Adds subtype-aware typed clients, standalone encodeable registration, `IIsa95ClientFactory`, and V2 status-event streaming. - Adds the `MinimalIsa95Server` sample, package documentation, and an ISA-95 guide with conformance and HA guidance. - Tracks typed RSL/GPOS integration as follow-up issue #4095. ## Validation - `Opc.Ua.ISA95.Tests`: 137/137 passed on `net10.0` and `net48`. - Source-generator suites: 81/81 model, 90/90 stack, 3718 passed / 8 skipped core. - `EventRecordFieldReaders` feedback tests: 6/6 on `net10.0` and `net48`. - `EventRecordFieldReaders` changed-method patch coverage: 100% line coverage (36/36) and 100% branch coverage (22/22). - `Opc.Ua.ISA95`, `.Client`, and `.Server` build across all configured target frameworks without new diagnostics. - Cobertura line coverage: 88.08% model, 90.04% client, 91.19% server. - NativeAOT sample publishes and passes a live typed V2 Store/status-event validation. - The ISA-95 NativeAOT smoke test passes in the source-generated native test executable. - OPC UA KB NodeSet validation reports zero errors for Common, V1, and V2. The broader NativeAOT executable still reports unrelated existing GDS and complex-type test failures; the ISA-95-specific native test passes. ## Node state generator fixes Generating method arguments inline for imported nodesets regressed every model that already emits `InputArguments` / `OutputArguments` child node factories: - The inline block ran before the child factories. `MethodState.CreateOrReplaceInputArguments` keeps the property that was assigned first, so the child carrying the well-known NodeId was dropped and a NodeId-less placeholder remained in the address space. This broke browsing (reference descriptions without a target) and made method arguments unreachable at their well-known NodeIds. - The `MethodDeclarationId` fallback keyed on an autogenerated display name redirected every declared method of the standard address space to its own NodeId, which made method calls fail. The inline block is now emitted after the child factories and reduced to a value assignment when a child factory exists. The value is only re-emitted when an argument references a namespace other than OPC UA, because the value of an imported property is the verbatim XML of the source nodeset whose namespace indices do not survive the import. The `MethodDeclarationId` fallback is restricted to declarations without a resolvable NodeId. The generated OPC UA address space is byte-identical to `master` again. ## Related Issues - Fixes #4088 - Follow-up: #4095	2026-07-28 12:01:57 +02:00
Marc Schier	39991bfea1	Harden managed subscription recreation and reapply (#4065) ... ## Summary - Makes managed subscription recreation generation-safe by quiescing publish ingress, draining in-flight requests, retiring queued messages and acknowledgements from the old server-side generation, then recreating monitored items as one serialized operation. - Exposes `RecreateAsync` on `ISubscription`, rejects recreate/dispose reentrancy from notification callbacks, preserves the active generation when server-side deletion fails, and coordinates disposal with an in-flight recreate. - Aggregates logical subscription recovery state across partitions and adds pending/reapply APIs so monitored items can be deterministically re-created after model or configuration changes. - Initializes the Quickstarts Boiler logger with `NullLogger` so simulation updates remain safe before a logger is assigned. - Keeps the new logical recovery test compatible with net48. ## Review follow-up - Documents the `RecreateAsync` callback-reentrancy and disposal exceptions on the public interface. - Extends generation-retirement coverage with a 1,024-message stale burst and verifies that only ordered current-generation callbacks are delivered. - Proves manager disposal cancels a cancellation-aware operation holding publish quiescence and waits for the gate to release safely. - No runtime timeout or blanket channel drain was added: the quiesced production delegate is internal and cancellation-aware, while unconditional draining could discard or reorder current-generation messages arriving through direct ingress. ## Validation Current head: `edb4a75145c8b03bd03da15eb71327e3bffda091`. Review follow-up validation on the current head: - `Opc.Ua.Client` and `Quickstarts.Servers` build successfully for net472, net48, netstandard2.1, net8.0, net9.0, and net10.0. - focused `SubscriptionTests` and `SubscriptionManagerTests`: 72 passed on net10.0 and 72 passed on net48. The runtime implementation is unchanged from `7b944ac68616e2d6c0595957c65179f86e78a637`, which passed: - net10.0 `Opc.Ua.Client.Tests`: 1,974 passed. - net10.0 `Opc.Ua.Sessions.Tests`: 738 passed, 379 skipped. - net10.0 `Opc.Ua.Subscriptions.Tests`: 601 passed, 6 skipped. - focused net48 managed-subscription tests: 122 passed. - win-x64 Native-AOT publish succeeded; subscription, monitored-item, and Boiler AOT cohorts: 14 passed.	2026-07-23 09:54:26 +02:00
Marc Schier	84fa8e8906	CTT follow-up: monitored items, alarms, codecs, history, and model validation (#4002) ... # Description Follow-up to #3978 based on the latest OPC UA CTT aggregate, matrix, Historical Access, Attribute, and Node Management runs. ## Aggregate run 16 - Ignore synthetic `BadBoundNotFound` raw-bound markers when feeding aggregate calculators. - Add direct and live historian regression coverage. ## Multi-dimensional Variant matrices - Fix `Variant.Copy()` so rank-2 Variant matrices preserve their payload and dimensions. - Enforce valid Variant matrix dimensions consistently across Binary, JSON, XML stream, and XML parser codecs. - Encoders return `BadEncodingError`; decoders return `BadDecodingError` for malformed matrix dimensions. - Add cross-codec valid/null/empty/malformed tests and a batched ReferenceServer Read regression. - Correct the previous CTT issue attribution: the invalid `Scalar_Static_Arrays2D_Variant` server encoding poisoned the batched CTT Read. ## Node Management AddNodes - Return `BadNodeIdRejected` for unsupported requested-NodeId namespaces. - Validate hierarchical ReferenceType source/target NodeClass constraints. - Document run-18 confirmation that all remaining 15 AddNodes errors are CTT project/script configuration issues. ## Historical Access and Attribute Services run 18 - Reject whitespace in `NumericRange` syntax with `BadIndexRangeInvalid`. - Add HistoryRead tests for operation-level invalid syntax and per-DataValue `BadIndexRangeNoData`. - Document CTT defects: - `012.js` expects `BadIndexRangeNoData` at the operation level instead of on returned DataValues; - `Err-012.js` uses a non-historizing node for an access-denied test; - generic Attribute array helpers omit NodeId[] and StatusCode[] conversion paths. ## Base Information model validation run 19 - Run the CTT main session as `sysadmin` over Basic256Sha256 SignAndEncrypt so SecurityAdmin RolePermissions and EncryptionRequired restrictions are both satisfied without weakening the server model. - Preserve predefined well-known NodeIds while making generated dynamic type and optional-child factories allocate compatible per-instance NodeIds, remap internal references, and update references from the owning subtree. - Materialize dynamic RoleType properties through generated helpers and keep role-child string NodeIds disjoint from numeric RoleManager root IDs. - Add net48/net10 regressions for generator output, NodeId allocation/reference remapping, DI mandatory children, dynamic roles, and low-privilege versus SecurityAdmin Browse visibility. - Document CTT defects for permission-unaware mandatory-child validation, the UA 1.04 reference model used against a UA 1.05 server, stale ServerDiagnostics cache, and the `ConformanceUnits` scalar/array mismatch. ## Related Issues - Follow-up to #3960 and #3978. ## Checklist - [x] I have signed the CLA and read the CONTRIBUTING document. - [x] Tests prove the fixes and improve coverage. - [x] Required CTT documentation is updated. - [x] Focused builds/tests introduce no warnings. - [x] Affected tests pass on .NET Framework and .NET 10. - [ ] CI and CodeQL are green. - [ ] All PR feedback is addressed. ## Latest CTT full run and A&C run 20 - Fix Monitor Value Change V2 `036.js`: an ambiguous `DataValue(StatusCode)` call encoded `BadIndexRangeNoData` as a Good value. - Prove all 19 configured matrix monitored items notify when the selected value actually changes; document the independent `042.js` script defects. - Rebase dynamic `CertificateExpired` / `TrustListOutOfDate` alarm roots and descendants, restore standard type declarations and ModellingRules, and expose the cross-node-manager references. - Deliver method-triggered events to every authorized Session, fixing the two-Session Confirm scenario. - Preserve locale-only `LocalizedText` values consistently across Binary, JSON, and XML and align Acknowledge/Confirm comment behavior and tests. - Document the remaining proven CTT defects for TransactionDiagnostics, SemanticChange, History `013`, PolicyId scope, A&C, durable subscriptions, and Subscription Minimum events. ## Base synchronization Merged `master` through `007cfaf90` (`#4010`, repository layout consolidation) and retained the matrix regression tests at their new `tests/` locations. ## Latest validation - Focused merged-tree tests pass on net10 and net48. - `Opc.Ua.Server` and `Quickstarts.Servers` Release builds complete with 0 warnings and 0 errors on net10 and net48. - The existing `DisableEnableViaTypeAndInstanceMethodNoEventsDuringDisabledAsync` timing test took its documented inconclusive timeout path after the layout merge; no test failure was reported. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <romanett98@gmail.com>	2026-07-18 05:01:03 +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