mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-27 04:59:55 +08:00
fix/encoder-stack-depth-json-flake
37
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Marc SchierandCopilot
|
e80d0ca9ac |
Use a 4-byte KeyNonce in the reference PubSub sample (#4710)
# Description
The first PubSub exchange in
[docs/PubSub.md](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/docs/PubSub.md#run-a-first-publisher-and-subscriber)
delivers no DataSets on master or in the 2.0.0 source. The subscriber
only logs `DataSetReader Reader 1 faulted on MessageReceiveTimeout`,
while the publisher logs `UADP security wrap failed; dropping message`
with `ArgumentException: AES-CTR nonce must be exactly 12 bytes` for
every NetworkMessage.
Cause: the reference PubSub sample's demo keys use a 12-byte KeyNonce.
Since #4573, the AES-CTR counter block follows OPC 10000-14 Table 157
(`KeyNonce[4] || MessageNonce[8] || BlockCounter[4]`). With the sample's
keys, the publisher builds a 20-byte nonce, and `AesCtrTransform`
rejects it. The 2.0.0 release (release/2.0 at
|
||
|
romanettandClaude Opus 5.5
|
9b034f8291 |
Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700)
# Description Nullable reference types are now enabled once, in `common.props`, and every project in the repository builds warning-clean with them. **Configuration** - `<Nullable>enable</Nullable>` lives in `common.props`. All per-project `<Nullable>` settings are removed: 178 project files (enable, annotations, and the `disable` in the three Aot test projects). - 419 per-file `#nullable enable`/`restore` directives and the 4 `#nullable disable` directives are removed. - Kept on purpose: - The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`, `PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the project setting in generated code, so the directive is required there. - `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against the 1.5.378 packages and does not import the repository props. - `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now describe the central setting. **Migration** (the 43 unset and 13 annotations-only projects, roughly 10k diagnostics) - **Production code: annotated at the declarations.** Covers `tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the LDS sample. - Members, parameters and returns are nullable where null actually flows. - Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`, `IsMethodTypeDesign`). - `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take `params object?[]`. `Template.AddReplacement` takes `string?` (rendering already skips null replacements). - `IRegisteredServerStore.RegisterAsync` returns `Task<RegistrationEntry?>`: it returns null when a registration is removed. - Dereferences backed by an invariant (for example, the validated model) use `!`, so runtime behaviour is unchanged. - **Behavioural changes in production code** (small, deliberate): - `NodeSetToModelDesign` throws `InvalidOperationException` when the file is not a NodeSet. Before, it threw a NullReferenceException on the next line. - `ModelDesignValidator` starts its lookup tables empty instead of null. A redundant `if (m_nodes == null) return;` guard is gone. - **Test code: minimal annotations.** - `?` on values that can be null, `!` where the test asserts non-null. - `= null!` for fixture members assigned in SetUp. - `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the `IEncodeable` constraint. - A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`) folded into the adjacent `IsNull`/`Count` checks. **Notes for reviewers** - The diff is large (~1,500 files) but almost entirely mechanical: `?`, `!` and directive removal. The non-annotation changes are listed above. - Known follow-up, not done here: `IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay non-nullable even though the validator returns null before it loads a dictionary. Making them nullable would ripple through every generator call site. ## Related Issues - Fixes #4439 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [ ] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - [ ] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. The "no new warnings" box covers net10.0 and net48: full non-incremental builds of `UA.slnx`, `tools/SourceGeneration.slnx`, `tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0 warnings and 0 errors under the repository's warnings-as-errors settings. net9.0 and net8.0 also had no nullable diagnostics, but they were not rebuilt after the final fixes. Tests have not been run locally yet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Klaus Landsdorf
|
cc7908eaa1 |
WebApi: serve a generated OpenAPI document of the REST binding (opt-in) and map the session-less service set (#4657)
Reworked after the review: the OpenAPI document is generated from the code, not embedded; the route is opt-in; `StatusCode` is not accepted. The branch is a new history on top of master (the first version conflicted); master with #4674 is merged in. # Description The REST binding implements the routes of the OPC Foundation OpenAPI mapping (OPC 10000-6 §G.3), but it does not describe them: `GET /openapi.json` is answered with HTTP 405, and a client generator has nothing to read from the server. `docs/WebApi.md` listed a generated OpenAPI document as a deferred follow-up. The coded API is the source of truth, so this PR **generates** the document from the route table and from the structure definitions of the request and response types, and compares it with the document the model compiler publishes in tests. Nothing from `UA-Nodeset/OpenApi` ships in the package. ## Changes ### Service sets (`Opc.Ua.Core`) - `WebApiServiceSet` (`AllServices`, `Sessionless`) and `WebApiServiceRoutes.GetRoutes(serviceSet)`. `Sessionless` is the eight services of `opc.ua.openapi.sessionless.json` (OPC 10000-4 §6.3): Read, Write, HistoryRead, HistoryUpdate, Call, Browse, BrowseNext, TranslateBrowsePathsToNodeIds. ### Generator (`Opc.Ua.Core.Schema`, `Opc.Ua.Schema.OpenApi`) - `WebApiOpenApiGenerator.Generate(serviceSet, includeSchemas, serverUrl)` returns the OpenAPI 3.0 document as a `JsonObject`. It is built at runtime from `WebApiServiceRoutes` and the `StructureDefinition`/`EnumDefinition` of the message types (the same definitions the schema generation uses, resolved through `IDataTypeDefinitionResolver`). No reflection, so it is NativeAOT compatible; the AOT test project covers it. - **Two levels of detail**, because full schemas make big documents: | | Content | All 28 services | | --- | --- | --- | | default | paths, `operationId`s, every body a JSON object | about 12 KB | | `includeSchemas: true` | one component schema per message, structure and enumeration (100), for typed client generators | about 46 KB | The two published files are about 300 KB together. - The schemas describe the Compact JSON encoding, with the property names, types, formats and integer ranges of the published documents. DataValue is the one structure that differs: its status property is `Status`, as in Part 6 Table 42. - `OpenApiBuiltInSchemas` holds the schemas of the built-in types (StatusCode, LocalizedText, Variant, DataValue, DiagnosticInfo, ExtensionObject and the scalar types) as the encoder writes them. Abstract `Structure` fields are ExtensionObjects, abstract number fields are Variants. ### Serving the document (`Opc.Ua.Bindings.Https`) - `WebApiTransportOptions`: `ServiceSet`, `OpenApiDocumentPath` (default `null`: **off**) and `OpenApiIncludeSchemas` (default `false`). - `MapWebApiEndpoints(WebApiTransportOptions)` maps the routes of the service set and, when a path is set, the `GET` route of the document **in the same route group**. The route has no `AllowAnonymous` metadata. When an authentication scheme is registered, the contributor's `group.RequireAuthorization()` covers the document like `POST /read`; only `/findservers` and `/getendpoints` stay anonymous. The existing `MapWebApiEndpoints()` maps all routes as before. - The document is generated once per path base (listed as a relative `servers` URL; without a path base the list is omitted) by the `WebApiOpenApiGenerator` that `AddWebApiTransport` registers, or by a default one. - `Opc.Ua.Bindings.Https` references `Opc.Ua.Core.Schema` on net8.0 and later. ### Documentation - `docs/OpenApi.md` (new, linked from `docs/README.md`): serving the document, the two levels, generating clients, the JSON generated clients send, known issues, how the document is kept correct, troubleshooting. - `docs/WebApi.md`: new section "Service sets and the OpenAPI document"; the deferred follow-up is removed. `docs/SchemaGeneration.md` and both `NugetREADME.md` files describe the generator and the options. ## Comparison with the model compiler documents The two published documents are test resources of `Opc.Ua.Core.Schema.Tests` only (README next to them: source commit and update procedure). `WebApiOpenApiConformanceTests` compares the generated document with them for both service sets: - paths, `operationId`s, and the request and response schema of every operation; - the set of component schemas the operations reach: equal (100 for all services); - every enumeration (members and values); - for every structure: property names, types, formats, integer ranges, and inheritance. Everything both describe is equal. The test lists the differences and fails when a listed one disappears or a new one appears: | Difference | Generated | Published | Reason | | --- | --- | --- | --- | | DataValue status property | `Status` | `StatusCode` | Part 6 Table 42; UA-Nodeset#146 is open. The stack only knows `Status` (encoder and decoder). | | `description`, `default` keywords | not written | spec links, .NET defaults | documentation and defaults, not contract; Compact omits default values | `WebApiOpenApiWireFormatTests` validates what the binding really writes (the default request and response of all 28 routes, plus populated requests and responses with variants, status codes, references, identity tokens and notifications) against the generated component schemas. ## Tests - `Opc.Ua.Core.Schema.Tests` `OpenApi/`: `WebApiOpenApiGeneratorTests` (shape of the document, both levels, both service sets, servers list, sorting and determinism, concurrency, argument and resolver errors, every built-in type, arrays and ranks, enumerations, option sets, unions, base types, self-references, unknown definitions), `WebApiOpenApiConformanceTests`, `WebApiOpenApiWireFormatTests`, `OpenApiBuiltInSchemasTests`: 81 tests. - `Opc.Ua.Bindings.Https.WebApi.Tests`: `WebApiOpenApiDocumentEndpointTests` (TestServer with the startup contributor: off by default, configured path, both levels, service set maps and describes only the sessionless routes, path base, repeated requests, registered generator is used, **a request without credentials gets 401 when an authentication scheme is registered while one with credentials gets 200, the route carries `IAuthorizeData` and no `IAllowAnonymous`**, argument checks), options defaults and DI registration of the generator. - `Opc.Ua.Core.Tests`: `GetRoutes` for both sets and an undefined value. - `Opc.Ua.Aot.Tests`: the generator and a served document under the AOT fixture. - Results (macOS arm64): `Opc.Ua.Core.Schema.Tests` 260/260, `Opc.Ua.Bindings.Https.WebApi.Tests` 457/457, `Opc.Ua.Core.Tests` WebApi tests 110/110, each on net10.0 and net8.0 (Release). `Opc.Ua.Aot.Tests` (JIT run) SchemaAotTests 5/5, WebApiAotTests 8/8. - `Opc.Ua.Core`, `Opc.Ua.Core.Schema` and `Opc.Ua.Bindings.Https` and the three test projects build for net48, net8.0, net9.0 and net10.0 (Release) with 0 warnings and 0 errors (net48 builds only; the Windows legs of the fork CI run its tests). `dotnet format` whitespace, style and analyzers (`--severity info`) are clean on every changed file; the two `RCS1090` hits in `SchemaAotTests.cs` are on lines that master already has. - Patch coverage with `.azurepipelines/check-coverage.ps1` against master (`Opc.Ua.Core.Schema.Tests`, `Opc.Ua.Bindings.Https.WebApi.Tests` and the WebApi tests of `Opc.Ua.Core.Tests`, net10.0): 362/362 changed lines covered. - Fork CI (full matrix incl. macOS) and CodeQL on `f990af5db` (master with #4674 merged in): [CI run 37310622902](https://github.com/biancode/UA-.NETStandard/actions/runs/37310622902) green, [CodeQL run 37310626786](https://github.com/biancode/UA-.NETStandard/actions/runs/37310626786) green. The run on the earlier head `abda0e491` had one failure outside this PR: `McpServerProcessTests.HttpTransportAliasesExposeOnlyMcpRouteAsync("http")` timed out after 30 s on Windows net10.0; this PR does not touch `tools/`. ## Not in this PR - Authentication: the fix discussed in the review is @romanett's #4674. The document route uses whatever the REST routes use; this PR does not change authentication. - `StatusCode` tolerance in the decoder: dropped, the spec text is the reference. The first commit of the previous version is gone. - Samples. - Session-less invocation on the server side (OPC 10000-4 §6.3) is a separate draft PR. With `ServiceSet = Sessionless` this PR only limits the mapped routes and the document. ## Questions 1. `Opc.Ua.Bindings.Https` now references `Opc.Ua.Core.Schema` (net8.0 and later) because the generator lives next to the other schema generators and is trimmed away when unused. Fine, or should it move into `Opc.Ua.Core`? 2. Should `OpenApiIncludeSchemas` default to `false` as here, or should a host get typed clients out of the box? ## Related Issues - Fixes #4656 ## 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. (Affected suites on net10.0 and net8.0 locally on macOS; .NET Framework is covered by the fork CI's Windows net48 legs.) - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [ ] I have addressed **all** PR feedback received. |
||
|
|
2f5aeb3da4 |
Fix Medium encoder/decoder defects found by the second codec review (#4625)
## Summary This is the follow-up to #4599. A second read-only review of `src/Opc.Ua.Types/Encoders` was run on master after #4599 merged. It found about 40 defects: none Critical or High, 10 Medium. This PR fixes the 10 Medium ones. Each fix follows the OPC 10000-6 clause named in the table, and each has regression tests. | Finding | Fix | Part 6 | |---|---|---| | **Regression from #4599:** a nested JSON array (`[[1,2]]`) decoded as an *empty* Byte/SByte array or ByteString, even with ParseStrict. Debug builds hit a `Debug.Assert` fail-fast instead. | Return false when `TryGetArrayElements` rejects the input | 5.4.5 | | A malformed `UaTypeId` was accepted as a raw JSON body with no type | Decoding error when ParseStrict is on; the raw-body fallback is kept when it is off | 5.4.2.16 | | With namespace mappings active, the Binary/XML decoders and `XmlParser` turned `nsu=…` ExpandedNodeIds into `ns=0` | Skip the index mapping when a NamespaceUri was decoded | 5.2.2.10 | | An ExtensionObject with no `Body` failed to decode when its field is in a companion namespace | `PopNamespace` before `EndField` (XmlDecoder and XmlParser) | 5.3.1.16 | | Null DiagnosticInfo/DataValue array elements were left out of the XML output, so array positions shifted | Pass `isArrayElement` so they are written as nil elements | 5.3.4 | | CR characters were lost in XML strings, and a string NodeId containing one changed | Write CR as `
`; this also works with caller-supplied writers | 5.3.1.5, XML 1.0 §2.11 | | A null element in a structure array/matrix caused a NullReferenceException in `JsonEncoder` | Write JSON `null` | 5.4.5 | | An ExtensionObject with a null ByteString/XmlElement body was written as encoding 0x01 with Length -1 | Write encoding 0x00 (no body) | 5.2.2.15 | | Default Number/Integer/UInteger structure fields threw BadEncodingError in Binary/XML. Integer arrays with values did not round-trip through XML. | `Variant.CreateDefault` returns a Variant default for these types; the XML decoders read such array fields as Variant arrays | 5.1.6 | ### Behaviour changes - Encoding a null-body ExtensionObject in binary now produces encoding `0x00`. `WriteExtensionObject_NullByteString_WritesTypeIdAndNoneEncoding` had pinned `Binary`, which contradicted its own name; it now expects `None`. - With ParseStrict, the JSON decoder rejects a present but unparsable `UaTypeId`. ### Review follow-up ( |
||
|
|
20c2d77d57 |
Fix Core/Types audit findings and conform them to the OPC UA spec (stacked on #4569) (#4573)
> **Stacked on #4569.** The base branch is `romanett/practical-murdock-fd9c56`, so this diff shows only the commits of this PR. Retarget to `master` after #4569 merges. ## Summary Bug audit of `Opc.Ua.Core`, `Opc.Ua.Core.Types`, `Opc.Ua.Types` and `Opc.Ua.Core.Schema`, with the findings fixed and the fixes grounded against the OPC UA specification (reference.opcfoundation.org). 133 commits in four groups: 1. **Audit fixes** (`Fix <ID>: ...`): 116 findings from a read-only audit, each with a regression test. IDs follow the audit scopes: T1-T6 `Opc.Ua.Types`, C1-C4 `Opc.Ua.Core`, S1 `Opc.Ua.Core.Types` + `Opc.Ua.Core.Schema`. 2. **Review fixes** (`Fix review <ID>: ...`): issues a code review of group 1 found, plus lower-severity leftovers. 3. **Spec fixes** (`Spec: #<n> ...`): the 18 places where an earlier fix only partly matched the spec text. 4. Test and analyzer follow-ups. Where `master` (#4516, #4522, #4540, #4564) or #4569 already fixed the same issue, their version was kept and this PR only adds what they didn't cover. ## Behaviour and user-facing changes ### Wire format and interoperability (breaking) - **PubSub AES-CTR (Aes128-CTR / Aes256-CTR)** now follows Part 14 Tables 156/157: - The counter block is `KeyNonce[4] | MessageNonce[8] | BlockCounter[4]`, and the block counter starts at 1. - The SecurityHeader nonce is 8 bytes: `Random[4] | UInt32 SequenceNumber`, with the sequence number starting at 1. - SKS key data is split with a 4-byte KeyNonce, giving 52 / 68 bytes. - **Not interoperable with earlier releases of this stack**, but matches the spec. There is a test against the independent RFC 3686 vector. - **User token signature** (Part 4 6.1.8): - The signed data is now built from the user token policy's SecurityPolicy on both client and server. - It also supports the SecurityMode None layout `ServerNonce | HASH(ServerCertificate) | ClientNonce`. - New overload `SecurityPolicyInfo.GetUserTokenSignatureData(..., MessageSecurityMode)`. - **Secure channel:** - The ECC AEAD `LastSequenceNumber` is correct across the 32-bit sequence wrap (Part 6 6.8.1). - The incoming chunk counts against MaxChunkCount/MaxMessageSize. - The sender no longer lets a message through that is 24 bytes over MaxMessageSize. - The OPN response signature is verified without overrunning the buffer (RSA_DH with differing key sizes). ### Sessions (server) - **CreateSession nonce:** - The client nonce is kept on None channels, because the None-mode user token signature covers it. - A nonce over 128 bytes returns `Bad_NonceInvalid` on every channel (Part 4 5.7.2.3). - The duplicate-nonce check runs only on secured channels. - A non-empty nonce under 32 bytes is still accepted on None, for compatibility. - **ActivateSession:** an embedded `EccEncryptedSecret` signing certificate is validated with the server's peer certificate validator (Part 6 6.8.3). Byte equality with the client certificate is only the fallback when no validator is available. New opt-in interface `ICertificateValidatorProvider`. - **FindServers / GetEndpoints:** `GatewayServerUri` and `DiscoveryProfileUri` are no longer swapped. ### Write, Read and Call (NodeState / node managers) - **UserWriteMask:** - Enforced for non-value attributes and for VariableType Value writes (`ValueForVariableType`), returning `Bad_UserAccessDenied`. - An unconfigured UserWriteMask (0, no handler) is not enforced, and reads back as the WriteMask so Read and Write agree (Part 3 8.60). - A throwing or Bad `OnReadUserWriteMask` handler fails only that item. - **Index-range writes:** - Slices are merged into the cached value under the attribute lock, also for `OnWriteValueAsync` handlers. - A data-change notification is still produced when the cache cannot absorb the slice. - The slice must match the unclipped range, otherwise `Bad_IndexRangeDataMismatch` (new status code in `Opc.Ua.Types`). - A range past the target returns `Bad_IndexRangeNoData`. - A handler without index-range support returns `Bad_WriteNotSupported` (Part 4 7.27, 5.11.4.4). - **Index-range reads on matrices** return partial results clipped to the source, instead of allocating the requested size (a Read with `"0:40000,0:40000"` no longer allocates about 1.6e9 elements). - **DataTypeDefinition writes:** - A value that isn't a decoded DataTypeDefinition, or whose kind doesn't match the DataType category, returns `Bad_TypeMismatch`. - Null is rejected for concrete Structure/Enumeration/OptionSet types. - WriteMask is checked first (Part 3 5.8.3). - **AnalogItem:** a null write skips the InstrumentRange check again. - **Method call:** the async fallback passes the ObjectId to the method handlers. - **Redundancy:** `DistributedValueParticipation` merges index-range writes and applies the index range on reads. - **Change masks:** `ClearChangeMasks` takes the bits atomically, so concurrent changes are no longer lost. - **`AccessLevelEx`:** preserved by Clone/Export. ### Alarms & Conditions (Part 9) - **Acknowledge / Confirm / AddComment:** - Accept any EventId the condition or its branches reported while that state is still pending (5.7.3). - An unknown id returns `Bad_EventIdUnknown`. - An already handled state returns `Bad_ConditionBranchAlreadyAcked` / `...AlreadyConfirmed`. - AddComment on a branch no longer changes the trunk, and the branch table is re-keyed when a branch's EventId changes. - **Shelving:** - OneShotShelve without MaxTimeShelved no longer overflows the timer. - A timed shelve is measured on the monotonic clock, so wall-clock steps don't delay the unshelve. - The state is named `OneShotShelved`. - **Audit events:** - They report failure for rejected method calls. - `SelectedResponse` is a UInt32. - A rejected negative Respond leaves SelectedResponse unset. - **`ContentFilterElement.Validate`:** an unknown FilterOperator is reported per element as `Bad_FilterOperatorInvalid`, instead of failing the whole service call. ### ContentFilter evaluation (Part 4 7.7.3, on top of #4564) - **Bitwise operators:** - The result takes the size of the larger operand. - Boolean, numeric String and one-element array operands are converted to integers implicitly. - Types without an implicit integer conversion give NULL. - **Strings:** ordering is ordinal, no longer culture dependent. - **Cast:** restricted to the Table 121 conversions, and QualifiedName to LocalizedText is added. The general `Variant.ConvertTo` keeps its wider set. - **Null and empty operands:** - `Like` with a NULL operand is NULL. - Empty arrays of the same DataType compare equal. - Null-string operands are NULL everywhere. ### Encoders and built-in types - **XML:** - Strings keep leading, trailing and whitespace-only content (Part 6 5.3.1.5). NodeSet import opts back into treating whitespace-only strings as empty with the new `XmlDecoder.TreatWhitespaceOnlyStringsAsEmpty`. - Layout whitespace around `<Identifier>` values is ignored, and trailing control characters are stripped from `s=` ids. - `XmlElement` ignores DTDs without expanding entities. - **JSON:** - A DateTime without an offset is read as UTC. - An unmappable `nsu=` NodeId and an unknown `svu=` ExpandedNodeId decode to a namespace-0 String NodeId (5.4.2.10 / 5.4.2.11). - `svr=` is rejected in a NodeId field. - Bad matrix dimensions return `BadDecodingError`. - The non-strict body stack stays balanced after an error. - The nesting limits match between encoder and decoder. - **Binary:** - `MaxByteStringLength` no longer applies to strings. - ExtensionObject bodies count toward the nesting limit on seekable streams (and in the XML encoder). - **Parsing:** - The NodeId parser requires `i=`/`s=`/`g=`/`b=`. - `ExpandedNodeId.Parse` rejects an empty `nsu=`/`svu=`. - NumericRange rejects a trailing `,` with `Bad_IndexRangeInvalid`. - RelativePath escapes `#`. - **Status codes:** `StatusCode.WithSubCode`/`WithCodeBits` keep the severity bits and look up the symbolic id for the new code. - **Variant / VariantHelper:** - `TryCastTo` returns false on a type mismatch. So `DataValue.GetValue<T>(default)` returns the default, and `GetValueOrDefault` / `CastTo` throw `BadTypeMismatch` as documented. - `System.DateTime` casts are supported. - Concrete encodeable arrays (e.g. `Argument[]`) no longer throw. - `ConvertTo` keeps one-element and empty arrays as arrays and checks the conversion is legal for empty arrays. - `CastFrom(object[])`, `CastFrom(Enum[])` and the obsolete `Variant(object)` constructors keep the array shape (previously a one-element array collapsed to a scalar and an empty one to Null). - `TypeInfo.Unknown` / `Variant.Null.TypeInfo` report `ValueRanks.Any`. - **Equality and hashing contracts fixed:** - Types: Matrix/MatrixOf, typed-null Variants, NodeId (the namespace index is now hashed), ExpandedNodeId, LocalizedText, DiagnosticInfo, StructureDefinition/EnumDefinition, CertificateIdentifier, UserIdentity and DateTimeUtc. - `MatrixOf.Dimensions` returns a copy. - Decimal equality and hashing now run in bounded time. - **Localized text:** a LocalizedText with locale `"mul"` and no usable translations, or null client locale ids, no longer throw. ### NodeSet, address space and utilities - **NodeSet export/import:** - Server index 0 is the local server on import and export, so local ids are no longer written as `svr=65535`. - `StructureDefinition.BaseDataType` is filled from the supertype on import. - **`NodeTable.Import`:** replaces duplicates, keeping the references neighbouring nodes hold. `TypeTable.Add` detaches a re-parented type from its old supertype, and `Node.Clone` gets its own ReferenceTable. - **`StringTable` / `NamespaceTable`:** capped at UInt16 indexes (`BadEncodingLimitsExceeded`) for Append and Update, instead of silently wrapping. - **Small fixes:** - `UnsecureRandom.Shared` is seeded randomly. - The peer's RetryAfterMs is clamped to one day. - The `CancelAsync` and `string.GetHashCode(StringComparison)` polyfills match BCL behaviour. - `CoreUtils.IsEqual` compares strings ordinally. ### Schema generation and ComplexTypeSystem - **JSON schema:** describes the 1.05 layout with the compact/verbose null rules, and now validates real `JsonEncoder` output. - **XSD:** - `EncodingMask` (`xs:unsignedLong`) only for StructureWithOptionalFields. - Guid and StatusCode are complex types. - Standard types are in the `Types.xsd` namespace. - **Structure-backed OptionSets** are described as `{Value, ValidBits}`. New `UaTypeDescription.IsStructureOptionSet` property and constructor overload. - **ComplexTypeSystem:** - A malformed type in a server dictionary is skipped per type instead of discarding the whole dictionary. - Cyclic supertype walks are bounded. - Declared definitions are no longer changed. - A dictionary validator is kept only if the dictionary parsed. ### Certificates and trust lists (Part 4 6.1.3, Part 12) - **Revocation:** - Checked for issuers supplied in the peer's chain and for inline-trusted issuers, against the CRLs in the trusted and issuer stores. - A store without CRL support no longer hides "revocation unknown". - `RejectUnknownRevocationStatus` is honoured for inline-trusted issuers unless the entry sets `SuppressRevocationStatusUnknown`. This changes the strict case of `ExplicitIssuerChainIsHonoredWithoutStorePathAsync`. - **Basic constraints / path length** violations fail with `Bad_CertificateInvalid`. - **`TreatAsInvalid`:** - Now yields `Bad_CertificateUntrusted`, which can't be suppressed or auto-accepted. - It is honoured on individual trust-list entries even when the certificate is also in a store. - **`EccEncryptedSecret`:** enforces the result of validating the sender certificate. - **`TrustListTransaction`:** - Routes each CRL to the store holding its issuer (TrustedCrls/IssuerCrls). - Validates staged CRLs, including the store's CRL support, before changing anything. - Applies certificates before CRLs. - **Other:** - One failing `CertificateChanges` observer no longer stops the others. - `InMemory:` stores share their contents per path, and `InMemoryStoreProvider` is `IDisposable`. ### WoT (`Opc.Ua.Types/Wot`) - **Projection security:** projection security schemes whose qualified names collide are refused. New diagnostic `WotDiagnosticCode.ProjectionSecurityConflict = 6063`. - **Input handling:** - Invalid Unicode (lone surrogates, bad UTF-8) is rejected as `JsonException`. - External-schema results are keyed per affordance. - A duplicate security-floor member is rejected. - `nsu=` ids are percent-encoded (Part 6 5.1.12). - The configured JSON depth is honoured on re-parse. - An oversized document is reported before it is parsed back. - **Performance and limits:** - Subtype-graph validation, `componentOf` linking and the namespace table are now linear. - Namespaces are capped at UInt16. ### New or changed public API - `ICertificateValidatorProvider` (server) - `SecurityPolicyInfo.GetUserTokenSignatureData(..., MessageSecurityMode)` overload - `XmlDecoder.TreatWhitespaceOnlyStringsAsEmpty` - `UaTypeDescription.IsStructureOptionSet` plus a constructor overload - `WotDiagnosticCode.ProjectionSecurityConflict` - `StatusCodes.BadIndexRangeDataMismatch` in the `Opc.Ua.Types` list - `InMemoryStoreProvider : IDisposable` ## Not changed / open - **Deferred:** - S1-14 (BSD OptionSet size needs the base type in `UaTypeDescription`, and BSD is deprecated). - C2-7 (restoring the own-certificate trust exemption would accept certificates the spec says to reject; removing the dead code is a separate decision). - A1-4 (config values keep surrounding whitespace, as the pre-April-2026 loader did). - H-2 (JsonDecoder has no real nesting counter). - A2-3 (the StringTable cap also applies to diagnostics tables; default `MaxArrayLength` already bounds them). - **Needs a product decision:** - The general `ConvertToString` writes a QualifiedName as `ns:Name`, while Table 121 drops the namespace. - Short (< 32 byte) client nonces are accepted on None channels. ## Verification - Every commit added a regression test that fails without its fix, where practical. - Before the rebases, with all audit and review fixes (before the `Spec: #…` commits): 0 build warnings and 16 test suites green on net10.0. The suites: Types, Core, Core.Encoders, Core.Security, Core.Schema, Client, Client.ComplexTypes, Server, Sessions, Gds, History, Features, Redundancy.Server, PubSub, WotCon and WotCon.Bindings. Core hit its known intermittent end-of-run hang once, and reruns were clean. - The `Spec: #…` fixes were verified per group on their own suites. Those include PubSub.Udp, Redundancy.PubSub, PubSub.Server, Configuration and SourceGeneration.Core. - **Not yet built or tested on this base.** It was rebased onto master (resolving overlaps with #4522, #4540 and #4564) and then onto #4569. This PR stays a draft until the full run on this base is green. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> |
||
|
|
f9892020a3 |
Add configurable server resource isolation (#4541)
# Description Add configurable server resource isolation on top of the merged #4522 foundation. - Coordinate connection, handshake, retained-message, session-establishment, queue, execution, request-cost and parked-request limits through one server-scoped provider. - Keep configured startup, continuity/reconnect, control and trusted-owner reservations inside existing totals. Shared capacity remains usable up to explicit owner ceilings. - Use authoritative Session membership for reassembly continuity capacity without granting unrelated resource or rate privileges. Preserve explicit owner mappings and bounded tracking. - Cache immutable classification evidence while retaining current Session and classifier checks. - Support weighted request ordering or FIFO independently of resource accounting. Installed providers enforce decoded-request limits in both modes. - Preserve fail-fast reassembly, valid Abort handling, fixed deadlines, active-work protection, reverse-connect handoff and bounded OpenAPI processing. - Support custom-handler parking through `IRequestParkingPolicy`, `WithRequestParking(...)`, and `RequestLifetime.ParkSink`. Parking releases execution capacity only, keeping retained-data and parked charges until completion. Built-in Publish and the global opt-out remain unchanged. - Provide direct, fluent and DI configuration, public incoming-request parking support, effective startup limit logs and user-facing documentation. Balanced remains the default. SharedOnly without an explicit provider retains shared behavior for intentionally incompatible/unlimited configurations. Reservations are finite and are not one full-message allowance per Session. ## Related Issues Builds on merged #4522. Fixes #4554. ## Review and CI corrections All three previously missed findings in [review 5326734718](https://github.com/OPCFoundation/UA-.NETStandard/pull/4541#pullrequestreview-5326734718) are addressed in `9e25e4ecc`: 1. A custom FIFO provider receives RequestQueue, RequestQueueBytes, RequestExecution and ParkedRequest accounting. A blocked head cannot be overtaken. 2. `IResourceIsolationReassemblyProvider` replaces the sessionless threshold independently of `UseFairScheduling`. The aggregate byte ceiling remains enforced. 3. Runtime `ReservedReassemblyBytes` includes all configured protected stage floors, including ControlReserved. The review's two already-resolved changes remain intact. The CI collection race was fixed at its source in `e1a994a4b`: `ArrayOf.ToArrayOf` treats a separately read collection count as an initial capacity hint and grows/trims for the actual enumeration. Deterministic tests cover growth, shrinkage and stale-zero counts. The subsequent recovery-test race was fixed in `05753ce77`: fake time advances only after a gated keepalive response has been processed and reset its timer, not merely after request arrival. Recovery-order and timeout assertions remain intact, with additional response-state assertions. Instrumentation reproduced the original failure twice; both fixed variants passed 400 executions per framework. CodeQL alert 388 was verified and dismissed as a false positive. Startup logs contain aggregate numeric capacities, not credentials or caller identities. ## Validation | Scope | Result | | --- | --- | | Latest automated checks at `05753ce7775a58801b4ba9d8f6d7781b709d0b89` | 167 successful, one package-publication check skipped, no failures | | Remaining external prerequisite | `license/cla` pending: Contributor License Agreement is not signed yet | | FIFO/weighted queues, custom parking and server-channel selection | 175 passed on each net10.0/net48 | | Isolation, configuration and parking-hosting selection | 259 passed net10.0; 257 passed net48 | | ArrayOf conversion tests | 73 passed on each net10.0/net48 | | Recovery-audit and keepalive fixtures | 40 passed on each net10.0/net48 | | NativeAOT smoke from preceding updates | Published and executed successfully | No tests, assertions, coverage thresholds, or required checks were weakened. The remaining CLA prerequisite must be completed through [CLA Assistant](https://cla-assistant.io/OPCFoundation/UA-.NETStandard?pullRequest=4541). This is not CTT certification or an unconditional network-DDoS, whole-process-memory or fleet-wide guarantee. ## 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. - [x] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> |
||
|
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. |
||
|
|
5362ec5e5f |
Fix the Opc.Ua.Types defects found by the read-only audit (#4474)
# Description
Fixes the defects a read-only audit reported against `Opc.Ua.Types` at
|
||
|
Marc Schier
|
2d9088119b |
Fix alias startup ordering and retained translation fallbacks (#4469)
# Description Complete the two P2 follow-up fixes identified during review of #4456. - Copy alias stores after application `IServerPreStartupTask` registrations finish, but before address-space creation. This preserves stores initialized asynchronously during startup, for both the default DI server and compatible custom servers. Task ordering, once-only execution, failure/cancellation behavior, and opt-in snapshot materialization remain intact. - Preserve the original `TranslationInfo` fallback locale, template, key, and arguments while rendering the selected translation. Retained values and service results can be translated for another session without losing their original fallback. - Keep selected templates unformatted until rendering, use the selected locale for formatting, retain metadata through locale filtering, and avoid double-formatting escaped braces during single-language conversion. - Add deterministic alias initialization and translation regressions, extend native hosting coverage, and update the feature documentation. Public signatures and `LocalizedText` struct layout are unchanged. No dependencies, migration-guide changes, CI settings, or coverage-threshold changes are introduced. ## Related Issues - Related to #4410. - Follow-up to the two P2 review findings in #4456. ## Validation The new regressions failed before the relevant fixes and pass afterward. | Scope | net10.0 | net48 | | --- | --- | --- | | Complete Types test project | 10,265 passed; 1 existing skip | 10,255 passed; 1 existing skip | | Affected hosting, aliases, resources, and configuration tests | 561 passed | 561 passed | | Existing real-session alias integration tests | 11 passed | 11 passed | - Built the affected libraries across all configured target frameworks with **0 warnings and 0 errors**. - Published and ran the actual Windows NativeAOT test executable: **7 HostingAotTests passed**, including initialized alias registries and English-to-German-to-English fallback preservation. Local compilation used `IlcSingleThreaded=true` after a parallel compiler attempt exhausted host memory; repository settings are unchanged. - Focused coverage: **28/29 changed executable production lines (96.55%)**. - Changed-file formatting and diff checks pass. The results above are the stated local project/filtered runs, not a complete local `UA.slnx` test run. Remote CI and CodeQL results are pending. ## 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. - [ ] I have addressed **all** PR feedback received. |
||
|
|
0e600c0f0b |
Fix hosted-server DI composition gaps (#4456)
# Description Complete the remaining hosted-server DI hooks so applications can compose the regular server without subclassing `StandardServer` solely for these extension points. - Add `ConfigureServerProperties` and ordered, per-server `ConfigureResources` callbacks. BuildInfo defaults come from the effective application configuration; explicit property registrations take precedence without mutating shared options. - Add authenticator instance/validator-aware factory overloads and generic/async-delegate startup tasks. Startup tasks are awaited in registration order, and failures or cancellation clean up the server. Host shutdown waits for execution cleanup before disposing injected dependencies, including when its shutdown deadline expires. - Add asynchronous and legacy node-manager factory instances plus deferred, configuration-aware `ArrayOf<IAsyncNodeManagerFactory>` registrations. Resolve factories after configuration and certificate setup while preserving ownership and feature isolation. - Register alias stores before address-space creation and add opt-in standard `TagVariables`/`Topics` materialization. Existing custom-category behavior is retained; browse nodes are startup snapshots while queries and supported LastChange values remain live. - Derive the default DI server from `ReverseConnectServer`, retaining DI roles/session hooks and authoritative XML/stream configuration. Ignore queued callbacks from stopped or superseded timers to prevent reconnection or rearming after shutdown. - Fix resource translation lookup/formatting exposed by the new callbacks, retain existing constructor signatures and direct/custom-server paths, and document the optional hosting APIs. Adopting these hooks is optional; existing construction and subclass-based customization remain supported. The migration guide is unchanged. No changes to UA-.NETStandard-Samples and no new dependencies. ## Related Issues - Fixes #4410 ## Validation - Focused server hosting, aliases, reverse-connect, configuration, and resource suites: **571 passed on net10.0 and 571 passed on net48**. - Existing affected real-session alias integration fixtures: **11 passed on each framework**. - Real reverse-connect discovery, session creation, and BuildInfo reads verified for options, XML-file, and stream configuration. - Published and executed the Windows NativeAOT test binary: **7 HostingAotTests passed**. - Final server-library build passed across all configured target frameworks with **0 warnings and 0 errors**; no new diagnostics in changed test files. - Focused changed-line coverage: **336/338 executable production lines (99.41%)**. - Deterministic timer regressions failed before the shutdown fix and pass afterward. - CI shutdown follow-up: the unchanged `UnavailableUpstreamEndpointFailsMappedReadAsync` scenario passes locally, and **245 hosting tests pass on each of net10.0 and net48**, including regressions for an expired host shutdown deadline and already-canceled startup. The local net48 run uses a short NUnit work directory to avoid Windows path-length limits. ## Azure build 18136 follow-up - Await WoT browse reconciliation before upload, default-version, and enable/disable handlers return. Three barrier-controlled regressions reproduce the exact missing version metadata before the fix. - Make the reconciliation test observe the actual corrective state frame rather than require a new acknowledgement after a no-op reseed. The corrected tests also verify value, status, timestamp, and quiescence; production gossip behavior is unchanged. - Drain redirected stdout/stderr when sample processes exit, replacing a fixed grace delay that could lose the final success line. A controlled real-process regression fails before the fix. - Fix CA1508, CA1823, CA1850, CA2012, and CA1861 at their sources, and correct unsupported legacy AOT declarations and the existing stress-placeholder target. Modern test targets and AOT analysis remain enabled. Follow-up validation: - Full **Release `UA.slnx` builds pass on net10.0 and net48 with 0 warnings and 0 errors**. - Complete previously failing WoT net48 job: **1496 passed**. The identical binary was run from a short local assembly path to avoid the nested repair worktree exceeding Windows path-length limits. - Complete previously failing Redundancy.Server net10.0 job: **783 passed**. - All short-haul HA/process cases: **12 passed**, including both original identity-failover topologies. - Affected WoT lifecycle selection: **113 passed on Release net10.0**; controlled replication selection: **15 passed on each framework**. - No test assertions, timeouts, coverage thresholds, collector settings, or CI failure gates were weakened. Existing legacy empty-project result notices and auto-property coverage exclusions remain unchanged. These are full solution builds plus the reported test scopes, not a full local `UA.slnx` test run. Remote validation at `77c1055f1adb1557c5bf99b70e76f4a0cd95ee94`: **all 265 PR checks passed**, including Azure build **18172**, GitHub Build and Test, CodeQL, and Docker image checks. ## 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. - [x] 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: romanett <romanett98@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Marc SchierandCopilot Autofix powered by AI
|
7d5f7a7076 |
Add shared stack APIs and fixes used by UaLens (#4464)
# Description Extract shared stack, regression-test, sample and documentation changes from draft #4462 into a separate prerequisite PR. The PR now contains **19 shared paths**: the original eighteen extracted files plus a focused user-management regression file added during review. It does not include UaLens source, desktop dependencies, solution registrations or application documentation. ## Changes - Correct the V2 ConditionRefresh target and response validation. Bound disposal-time remote subscription deletion with an independent five-second TimeProvider-backed cancellation deadline, allowing local cleanup after a server outage. This is a per-delete limit, not a global application shutdown deadline. - Preserve configured PubSub writer message/header settings, canonical numeric JSON publisher matching, and flattened DataValue quality/timestamps. - Add asynchronous user-management client wrappers over the generated standard proxy, and set file/directory method declaration IDs needed by typed client workflows. - Enable AI client NativeAOT compatibility and add focused AI client regressions. - Enable the ConsoleReferenceServer file-system node manager and configure 1 ms publishing/sampling demonstrations. These sample defaults do not promise hard real-time delivery. - Document the subscription and PubSub behavior. Make the new subscription/PubSub regression helpers portable to .NET Framework without weakening their assertions. ## Review follow-up Commit `812297ce1` aligns both public `ListUsersAsync` signatures with `ValueTask<ArrayOf<UserManagementUser>>`. The implementation constructs its result directly from an owned snapshot array, without an intermediate list. Five focused cases pass on **net10.0 and net48**: `ListUsersReturnsArrayOfSnapshotsAsync` checks both interface and concrete entry points and preserves snapshot order, values and flags; `ListUsersReturnsEmptyArrayAsync` covers typed-empty and missing values; `ListUsersPreservesBadReadStatusAsync` checks the exact server failure status. All **eight single-line XML summaries** were converted to multi-line form. A complete scan of the fourteen changed C# files, including the new test, finds zero remaining single-line summaries. Scoped whitespace/line-ending verification passes. The sole UaLens caller was adapted separately in draft #4462 (`4caf69f33`). A fresh combined dependency-graph build and all **924 UaLens net10.0 tests** pass with both changes. Each of the three review threads received an action-taken reply and was resolved only after the fixes were pushed and verified. ## Split boundary The branch originated at `a45105375`; the subsequent master merge `f209f3804` incorporates upstream `4633aa7a3`. The original shared changes and review regression remain separate from the app. Draft #4462 retains `tools\Opc.Ua.Lens`, `tests\Opc.Ua.Lens.Tests`, and its six solution/package/documentation integration files. **Merge this prerequisite before the UaLens draft.** The draft consumes the extracted APIs. Published history is preserved; neither PR has been merged automatically. ## Initial split validation The following results describe the initial split validation at `fa7199afa`; current review-specific validation is recorded above. | Scope | net10.0 | net48 | |---|---:|---:| | Client subscription/file-system tests | 168 passed | 168 passed | | Server file-system/user-management tests | 244 passed | 244 passed | | PubSub writer/reader regressions | 34 passed | 34 passed | | ConsoleReferenceServer build | Passed | Passed | The corrected portable regression helpers were rechecked on net10.0 after the net48 fixes. The combined UaLens application also passed its 924-case net10.0 suite before extraction. **Focused NativeAOT: 16 passed, zero failed/skipped.** A session-local TUnit runner linked the exact committed `AIClientAotTests.cs` and referenced the real AI client project. It published without warnings or suppressions and executed as a genuine Windows native binary with no CLR header. The canonical `Opc.Ua.Aot.Tests` project was blocked by pre-existing CS8632 nullable-annotation errors in unchanged upstream `SchemaAotTests.cs` (lines 82, 127, 129 and 132). Its existing nullable configuration and all upstream tests were preserved; the focused result is not a claim that the full AOT test project builds. The net48 hosting selection uses a short NUnit WorkDirectory: the existing fixture builds long PKI paths that otherwise exceed the platform's path limit. The original full selection passed without test exclusions, source changes or increased timeouts. Scoped builds reported inherited warnings in unchanged upstream files: CA1508 in `ReferenceBuilderExtensions.cs`, CA1861 in `NodeSetImportIntegrationTests.cs`, and net48 CA1823 in `DefaultNodeIdFactory.cs`. They were not suppressed or folded into this split. Full-solution, coverage and CI/CodeQL gates remain unasserted. ## Related Issues - Tracking/design review: #4461. - Prerequisite extracted from draft #4462. ## 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. - [x] I have addressed **all** PR feedback received. Tests and scoped results are documented above; measured coverage, inherited-warning qualification, CLA status, full-solution results and CI completion are not asserted by these checkboxes. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Marc SchierandCopilot
|
d08d4faec1 |
Reduce NodeState allocations and pack scalar Variants (#4452)
# Description Consolidate the complete B01/B02 NodeState memory work into one PR. - Replace eager cold-path NodeState locks with lazy canonical publication and update the event-monitor counter atomically. - Store six design-metadata properties and three security properties in separate lazy private bags while preserving public APIs, null/empty distinctions, masks, copying and existing serialization behavior. - Avoid allocating the ReportEvent scheduling closure when the async-dispatch path is unused. - Retain the logger factory's category logger directly, avoiding a redundant typed wrapper on telemetry initialization. - Keep Variant at 24 bytes while removing eligible outer boxes for QualifiedName, NodeId, NET8+ ByteString and text-only LocalizedText values. - Add reproducible allocation/layout/occupancy fixtures, real registered and monitored populations, reference contracts, generated-consumer coverage and NativeAOT cases. ## Measured allocation changes Matched .NET 10.0.11 x64 observations against the integrated master baseline: | Construction/assignment | Master | This PR | |---|---:|---:| | Bare/minimal object | 824 B | 640 B | | Bare/minimal variable | 1160 B | 976 B | | Variable with both optional groups populated or reset | 1160 B | 1088 B | | Variable with a shared ByteString value | 1192 B | 976 B | Separate paired measurements show: - telemetry initialization: 24 B typed-logger wrapper removed; - eligible QualifiedName, NodeId and ByteString Variant construction: 32 B to 0 B; - eligible text-only LocalizedText Variant construction: 40 B to 0 B; - variable attributes read into caller-provided Variant destinations: 136 B to 0 B. These are runtime-specific per-operation observations, not additive whole-server savings. Optional bags retain their documented populated/high-water premiums and are not reclaimed on reset. ## Reference-storage decision A complete private indexed reference backend and a sorted-traversal alternative were measured, but both changed the existing filtered hash-index enumeration order. They were rejected despite lower memory use. The production reference backend remains unchanged; strengthened ordering, identity, serialization and concurrency contracts are retained. Adaptive small-set storage remains deferred. ## Consolidation This PR now contains all changes from #4451 and the former narrower #4452 branch. Every #4452 changed path was present in #4451; after merging, the #4451 head `836e45226` is a direct ancestor of this branch. #4451 is closed as superseded. Draft #4425 remains unchanged and is also superseded by this consolidated work. ## Review feedback - Removed the standalone Variant storage document and its README entry. - Made the raw NodeId constructor private; the documented internal `GetRawState`/`SetRawState` bridge is explicitly Variant-only and must never become public. - Marked the two 32-thread counter tests non-parallel. - Added 10-second timeouts to the concurrent browse benchmark's main and worker barriers and completion waits. - Migrated the registered-population fixture to the current async `SealAsync` API. All review threads on #4451 and #4452 were replied to with the action and fix commit, then resolved. ## Related issues - Related to #4275. - Supersedes #4425 and #4451. ## Validation - Consolidated Types/Core/Server selection: **1241 passed** on net10.0. - Consolidated Types/Core/Server selection: **1238 passed** on net48. - Focused #4451 feedback selection: **189 passed** on net10.0 and **187 passed** on net48. - Earlier integrated NativeAOT validation passed **163/163** cases across the main, MCP and Historian applications; current PR CI provides the authoritative final merged-source native validation. The first #4452 CI run was blocked by merge conflicts. Seven later jobs completed their tests but encountered a shared artifact-service HTTP 403 while uploading results; no workflow or test was weakened to mask that infrastructure failure. Generated `report.*.json` crash artifacts remain untracked and are not part of this PR. ## 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 the changes and improve coverage. - [x] I have added the necessary current-behavior documentation. - [ ] I have verified that the fresh consolidated CI run introduces no build or analyzer warnings. - [ ] I ran the complete UA.slnx suite locally on net48 and net10.0 with no failures. - [ ] I fixed all failing and flaky CI tests and all CodeQL warnings. - [x] I have addressed all PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d2569e2e-7115-4969-bb44-8159e302b450 |
||
|
Marc SchierandCopilot
|
7411654c8d |
Complete OPC UA Part 11 Historical Access (#4390)
# Description Completes the OPC UA Part 11 Historical Access work requested by #4387 and the historian framework integration requested by #4400. The conformance/evidence catalog covers all 37 released UACore 1.05 Historical Access profiles: 15 Server facets and 22 Client facets, together with the standard aggregate functions and their conformance units. Server facets are advertised only when the resolved providers expose their required interfaces and capabilities. ## Server and provider support - Completes raw, modified, at-time, processed, annotation, structured, and event history. - Dispatches best-effort, bulk, and transactional updates with aligned statuses, old values, diagnostics, and rollback state. - Supports mixed history-update detail types and batched structured/annotation operations. - Uses composite identities and exclusive cursors for same-timestamp structured, modified, and event values. - Captures live data and events, installs historical configuration objects, and emits typed audit payloads. - Derives aggregate monitoring filters from per-node historian capabilities and rejects unsupported processed aggregates before provider dispatch. - Supports tri-state `Historizing`: set, clear, or preserve provider-owned state. ## Client support - Exposes modified values together with `ModificationInfo`. - Adds event-history reads and event insert/replace/update/delete operations. - Adds structured and annotation batching with strict response validation. - Shares continuation handling and release-on-abandon across read families. - Accepts inherited standard event fields rooted at event subtypes. ## Review hardening - Registers DI-selected historians before node-manager reconciliation and capability publication, while preserving provider ownership. - Uses committed Raft read barriers for linearizable `TryGetAsync` and `ScanAsync`. - Keeps claimed continuation state immutable until a fresh successor is saved, restoring the original after transient failures. - Uses exact-incarnation compare-and-delete so delayed cleanup cannot remove a restored same-ID continuation. - Cleans up indeterminate initial and replacement saves, and propagates shutdown cancellation through cleanup resolution. - Preserves legacy continuation envelopes/cursors and distinct annotation parent/property identities. - Deletes every structured entry at a requested timestamp and uses the complete modified-history ordering tuple. - Shares native/fallback at-time and AnnotationCount calculations, including reverse endpoints and cross-framework fractional intervals. - Primes modified aggregate items before buffered live delivery and deduplicates history/live overlap using provider-specific identities. - Keeps committed Modify results successful while reporting priming failures through the monitored item's notification queue. - Keeps protected-notification state live-only for queues larger than one. Queue size one retains the newest notification without pinning errors or transferring priority during resizing. Durable restore retains ordinary definitions, last values/errors, and raw queues without recreating priority. - Isolates aggregate calculation, modification preparation, and historical/live handoff in `AggregationFilterHandler`, retaining existing provider/factory wiring and supported past-start behavior. The larger-queue protection policy is unchanged pending the open review discussion. - Uses one `IMonitoringFilter` owner for protocol filters or the aggregation handler, eliminating parallel effective-filter storage and retaining the current definition during preparation. The original client filter stays distinct. - Integrates current master, retaining custom monitored-item creation decisions and per-node history callbacks. ## Redundant-server alignment and samples - Adds a protected shared historian with immutable segments, manifest CAS publication, writer fencing, and restart recovery. - Persists portable continuation state before exposing it to clients and transfers ownership during mirrored-session takeover. - Rejects eventual-consistency and active/active multi-writer historian configurations rather than silently degrading. - ReferenceServer and ConsoleReferenceClient demonstrate discovery, paging, raw/modified/at-time/processed reads, annotation/structured CRUD, event capture, and event CRUD. - RedundantServer and RedundantClient demonstrate active/passive history, continuation recovery after replica termination, and post-promotion writes. - Updates Historical Access, High Availability, profile, migration, and sample documentation. ## Compatibility - New 2.0 collection boundaries use `ArrayOf<T>` and public byte payloads use `ByteString`. - Removes the legacy public virtual `SamplingGroupManager.CreateMonitoredItem` / `ModifyMonitoredItem` entry points and protected creation factory, as agreed in review. Migration guidance points to the monitored-item manager and construction-decision APIs. - Retains established persisted formats and current write layouts. Retired interim notification-state formats (sample v2 / shared v4) are rejected, as agreed in review. - Keeps the historian integration compatible with .NET Framework and NativeAOT. ## Validation ### Latest CI source repair `73c619435` forwards the existing cancellation token at both predefined-node creation calls in the PR-added History event fixture. The merged lifecycle overload exposed these two CA2016 errors, which blocked History jobs, the solution build matrix, and CodeQL on GitHub and Azure. The exact compiler failure was reproduced locally; the repaired fixture passes 4/4 integration cases on net10.0 and 4/4 on net48 with clean scoped diagnostics. No assertions, tests, coverage thresholds, or CI configuration were weakened. The repaired History jobs on Linux/macOS and all four Azure solution builds now pass. The Windows all-TFM GitHub job instead lost its hosted runner's connection; no build log was recoverable, and GitHub recorded a runner-loss annotation rather than a compiler/test diagnostic. One unchanged retry was requested. Remaining checks are monitored with `gh pr checks --watch --fail-fast --interval 30`; CI is not yet reported as green. ### Latest base integration `bd52e5fea` merges upstream `master` at `da7412c42`. The lifecycle conflict retains upstream's shared cancellation-aware registration helper and the historian behavior. The hosted-historian fixture forwards its existing token to the new lifecycle overload. GDS project renames and all three AOT test hosts are retained, with their project references resolving after the merge. | Integrated scope | net10.0 | net48 | | --- | --- | --- | | Server lifecycle, fluent authoring/import, historian, monitored-item and hosting regressions | 732 passed | 732 passed | | NodeState lifecycle regressions | 216 passed | 216 passed | | Automatically merged Raft client fixtures | 28 passed | 28 passed | No selected tests failed or were skipped. The manually resolved files pass scoped formatting/analyzer checks. One CA1861 warning remains in the incoming `NodeSetImportIntegrationTests.cs`; that file is byte-identical to upstream master and was not rewritten or suppressed during integration. ### Latest feedback follow-up `102954eaa` replaces split filter references with the shared `IMonitoringFilter` contract implemented by protocol filters and `AggregationFilterHandler`. Targeted server regressions pass **103/103 on net10.0 and 103/103 on net48**; scoped formatting/analyzers are clean, and the updated XML-documentation inventory has zero gaps across 188 non-generated PR C# files. `93c5b5db8` renames `AggregationFilterHandler`, makes it the owner of the server-revised aggregate filter while preserving the original client filter, and removes the retired interim notification-state readers. Server regressions pass **101/101** and shared-store regressions **64/64**, on both net10.0 and net48, with no failures or skips. Runtime-file formatting/analyzers are clean. `5ccf45630` completes the public/internal XML-documentation sweep. The Roslyn inventory finds zero undocumented eligible declarations across all 186 non-generated PR C# files; the 110-file documentation slice changes only XML comments and spacing. `f17765475` applies the three requested Markdown edits. ### Earlier feedback follow-ups `a53950737` removes single-slot notification pinning and extracts aggregate implementation from MonitoredItem. Targeted sampling, aggregate modification, history/live handoff, queue/lifecycle, restore, and serialization cases pass **101/101 on net10.0 and 101/101 on net48**, without failures or skips. Scoped formatting/analyzers are clean; the focused review found no significant issues. The final formatted monitored-item fixtures also pass 45/45 on net48. The preceding `b21555568` removes the legacy sampling API bridge and durable notification-priority state. Its targeted Server cases passed 97/97 and shared subscription-store cases passed 64/64 on each framework. The broader evidence below remains pinned to its original tested commit, not attributed to these follow-ups. ### Earlier integrated-implementation validation Validated implementation commit: **`3efae597f`**. The following commit, `f6dbefb1c`, only adds two explanatory comment lines for the final standards review; it changes no executable code. | Scope | net10.0 | net48 | | --- | --- | --- | | Server | 5,187 passed; 56 skipped | Affected surface: 2,160 passed; 1 reproduced baseline failure; 56 skipped | | Client | 2,202 passed; 2 skipped | 2,204 passed; 5 skipped | | History integration | 519 passed; 21 skipped | 519 passed; 21 skipped | | Redundancy server | 641 passed | 641 passed | | Redundancy client | 124 passed | 124 passed | | Core shared-store regression suite | 23 passed | 23 passed | All six affected samples passed non-incremental Release/net10.0 builds with zero warnings and errors: ConsoleReferenceClient, ConsoleReferenceServer, RedundantServer, RedundantClient, MinimalBoilerServer, and PumpDeviceIntegrationServer. The published self-contained win-x64 NativeAOT executable is 116,924,928 bytes and has no CLR header or managed runtime dependency. Direct execution passed **HistoryAotTests 3/3** and **RaftAotTests 2/2**. Final standards and specification reviews found no confirmed outstanding high/medium functional gaps or hard standards violations. The remaining constructor-purpose clarification is addressed in `f6dbefb1c`. ### Reproduced baseline exceptions The net48 hosted-startup timeout also reproduces in a pristine worktree at the previous PR head, `24cf1416d`, in `RegisterPostStartRegistriesWiresHistorianAndAliasStoresOnPlainServerAsync`, with the same subsequent certificate-leak assertion. The corresponding renamed case fails before startup in the reviewed branch as well. This is reported separately from the net48 affected-surface results, not counted as a passing full Server suite. The net48 matrix selected the historian, aggregate, monitored-item, fluent, and continuation surfaces and excluded the Hosting namespace. `DurableDataValueQueueVerifyReferenceBatchingAsync` failed at item 1000 in the net48 affected-surface run and passed its single isolated rerun. The same failure at the same assertion reproduces on pristine `849e16acc`; the test, sample queue, queue factory, and batch persistor are unchanged. The original failure remains in the table rather than being erased by the rerun. ### CI source repairs - `4be7eb8f7` keeps diagnostics/configuration managers out of distributed address-space replication. Those managers inherit the source interface, but their live server state and redundancy metadata must remain replica-local. The ownership regression passes on net10.0 and net48, and all nine redundancy sample CI tests pass locally. - `d17f467b3` completes the sample startup repair: core managers are also replica-local; the startup banner no longer advertises readiness before initialization; the producer starts after distributed services; and native Raft startup no longer forces disruptive extra elections. The full sample suite passed 9/9, with initialized `RedundancySupport=Hot`, and focused Raft/address-space regressions passed 8/8 and 3/3. - `81b0b72be` moves the unchanged distributed-historian NativeAOT scenario into a smaller, always-run companion executable to bound Apple's linker input. All 137 AOT tests remain present; the moved test and helpers are unchanged. GitHub and Azure publish/run both native executables, with separate result directories where they share a job. The companion passed actual Windows NativeAOT execution. Linux and Intel macOS CI passed both executables, but this split alone did not fix ARM64 macOS linking. The remaining historian graph is rooted by ReferenceServer startup, not just the relocated test. No test exclusion or speculative linker setting was added to conceal it. - `8a5842913` isolates the independent MCP test graph into an always-run native host. The existing MCP source and assembly leak hooks are linked unchanged, and evaluated compiler inputs prove exactly one owner for every test source: 133 main + 3 MCP + 1 historian = 137 declarations. All three hosts run on every AOT platform. The main Windows native object is 9.35% smaller; both changed publishes had zero diagnostics. MCP passed 3/3; the identical main executable passed 133/133 on one unfiltered followup after two initial endpoint-discovery connection failures, which remain recorded in the local evidence. - The previous Intel macOS job successfully published and executed its native tests, then failed on artifact-upload DNS (`ENOTFOUND`). No upload gate, shared environment setting, or cache configuration was disabled. The first repair round also encountered the unrelated `MetricsAreEmittedForChannelLifetimeAsync` timing failure on macOS Core. Neither that test nor channel-manager code is changed by this PR, and its assertions remain intact. It passed unchanged on the final repair head. On `d17f467b3`, the Linux and macOS redundancy sample jobs pass; the remaining failure was ARM64 Apple's `too many large addends` assertion. The verified upstream relocation-anchor fix (dotnet/runtime#124721, backport dotnet/runtime#132171) is not present in the shipping ILCompiler 10.0.11. An experimental multi-module probe failed and was rejected, not added to CI. On `8a5842913`, [`aot-macos-latest`](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/34102158680/job/101678934057) successfully publishes and executes all three native hosts on ARM64: 136 main runtime cases, 1 historian case, and 3 MCP cases, with no skips. The previous Apple linker blocker is resolved. Linux NativeAOT also passes. No assertion, test, shared environment setting, cache, or required gate was weakened. The same CI run reports a separate macOS Sessions teardown failure: all 662 executed tests pass, but the global leak assertion detects one undisposed certificate. The MCP partition changes no runtime, Sessions, or test-fixture code. A single unchanged diagnostic rerun is queued once GitHub permits rerunning that job; the failure is not suppressed or counted as green. Other CI checks are still pending. ## Related issues - Fixes #4387 - Fixes #4400 ## Checklist - [ ] I have signed the CLA and read the contributing guide. - [x] I have added regression tests and directly related documentation. - [x] I have retained working reference and redundant-server sample wiring. - [x] Final validation and review evidence is complete, including the baseline exceptions above. - [ ] All required CI checks pass; the fail-fast rerun for `73c619435` is pending. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9dbd9527-d818-47a0-984c-fbb1472d3d6b |
||
|
romanettandClaude Opus 5
|
30ed2390a8 |
Import NodeSet2 documents into a fluent node manager (#4432)
## What
`INodeManagerBuilder.Import(UANodeSet, INodeSetImportFactoryProvider?)`
lets a source-generated (or hand-written fluent) node manager overlay a
NodeSet2 document that is only known at runtime onto the model it was
generated from.
Every document imported during one `Configure` pass forms a single batch
that is linked **exactly once** after the pass. A node may therefore
declare a `ParentNodeId` that lives in another document of the batch, or
a node the manager already owns — which is what makes an overlay
possible. Imported nodes resolve through
`Node(...)`/`NodeFromTypeId(...)` immediately, so they can be wired in
the same pass; they are registered with the manager when
`CompleteConfigureAsync` runs.
### Public surface
- **+1 method**: `INodeManagerBuilder.Import(UANodeSet nodeSet,
INodeSetImportFactoryProvider? factoryProvider = null)`.
- **+3 contracts** in `Opc.Ua.Server.Nodes`:
`INodeSetImportFactoryProvider`, `INodeSetImportFactory`,
`NodeSetImportDiscriminator` (TypeDefinition / MethodDeclaration /
NodeId).
- In `Opc.Ua.Export`: a public `UANodeSet.Import` overload taking a
`NodeSetImportStateFactory`, and a public batch
`LinkParentChildRelationships(context, nodes,
NodeSetImportLinkOptions)`. These replace what would otherwise have been
an `InternalsVisibleTo("Opc.Ua.Server")` on `Opc.Ua.Types`.
- `NodeState.IsExplicitlyDefinedChild(context, child)` and `protected
DetachExplicitlyDefinedChild(child)`, used to tell a generated slot
apart from an ordinary child.
### Typed states, no reflection
The generator emits `{Model}NodeSetImportFactoryProvider` — one factory
per model type/declaration, each calling a concrete constructor for an
*empty* state — and makes the generated node manager implement
`INodeSetImportFactoryProvider` by delegating to it. So the builder
resolves the provider from the manager (`NodeManager as
INodeSetImportFactoryProvider`); `Import` also accepts one directly, and
the generated manager exposes `partial void
AddNodeSetImportFactories(List<INodeSetImportFactory>)` for dependency
models. An imported node of a generated type materializes as its
generated `*State` subclass with no type lookup, which keeps the path
NativeAOT-safe.
Because the factories create empty states, an imported instance carries
exactly the children its document declares.
### Reconciliation
When an imported child lands in a slot the generated parent declares
(same BrowseName), it replaces the generated placeholder: the displaced
node and every descendant the document does not carry over are removed
from the manager, and references to them are retargeted at the
replacement. Replacing a node the same `Configure` pass already wired is
rejected with `BadInvalidState` rather than silently dropping the
wiring, and sealing a builder with an unregistered import batch is
rejected too.
### Also
- `RuntimeNodeSetNodeManager` now uses the same importer: its documents
are one batch, so a node may declare a parent in another document of the
group regardless of parse order, and duplicate NodeIds across documents
are detected while parsing.
- Linking resolves an imported child's reference type from its own
inverse reference (previously always `HasComponent`/`HasProperty`).
- Method import keeps the document's `UserExecutable`; it previously
copied `Executable` into it. View import keeps `EventNotifier`.
## Tests
- `NodeSetImporterTests` (15) — discriminator selection, generic
fallbacks, field/value/reference fidelity against a generic import,
cross-document linking, typed replacement of inherited and argument
slots, deferred linking preserving an application `Handle`, duplicate
NodeIds, factory misuse.
- `NodeSetImportBuilderTests` (15) — staging and lookup before
registration, batch linking, attaching to an existing node, provider
resolution, placeholder replacement with reference retargeting,
configured-placeholder rejection, seal/completion guards.
- `NodeSetImportIntegrationTests` (5) — a generated `[NodeManager]`
overlaying its own model on a running server: typed states through the
generated provider, cross-document linking, placeholder replacement, and
browse through a session.
- `NodeManagerGeneratorTests` (+4) — the emitted provider and the
manager's delegation, including that no factory uses
`Activator.CreateInstance`/reflection.
- `NodeSetAotTests` (+1) — the public state-factory import under
NativeAOT.
Full `Opc.Ua.Server.Tests` (4946), `Opc.Ua.SourceGeneration.Core.Tests`
(3821), `Opc.Ua.SourceGeneration.Tests` (165) and `Opc.Ua.Types.Tests`
pass locally on net10.0.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
romanettandClaude Opus 5
|
1af191ec26 |
Drop the .Common suffix from the three GDS projects (#4438)
# Description Every other companion specification in the stack ships as `<Family>`, `<Family>.Client` and `<Family>.Server` — `Opc.Ua.Di`, `Opc.Ua.ISA95`, `Opc.Ua.Positioning`, `Opc.Ua.Robotics`, `Opc.Ua.Vision`, `Opc.Ua.WotCon`, `Opc.Ua.XRegistry`, and `Opc.Ua.Lds.Server` without a family package. GDS was the one exception, carrying a `.Common` suffix that named nothing. | Before | After | | --- | --- | | `src/Opc.Ua.Gds.Common` | `src/Opc.Ua.Gds` | | `src/Opc.Ua.Gds.Client.Common` | `src/Opc.Ua.Gds.Client` | | `src/Opc.Ua.Gds.Server.Common` | `src/Opc.Ua.Gds.Server` | ## No source changes were needed `RootNamespace` was already the final name in all three projects — the namespaces have always been `Opc.Ua.Gds`, `Opc.Ua.Gds.Client` and `Opc.Ua.Gds.Server`, and only `AssemblyName` and `PackageId` disagreed. Nothing that compiles against these assemblies changes a `using` directive; the diff outside the moves is entirely ids, paths and prose. Directories were moved with `git mv`, and every one of the 73 file moves is detected as a rename at 100% similarity (bar the handful that also had a name in their text), so `git log --follow` and blame survive. ## Consumer impact The package ids change, which is visible to anyone consuming the NuGet packages: | 1.5.378 package | 2.0 package | | --- | --- | | `OPCFoundation.NetStandard.Opc.Ua.Gds.Common` | `OPCFoundation.NetStandard.Opc.Ua.Gds` | | `OPCFoundation.NetStandard.Opc.Ua.Gds.Client.Common` | `OPCFoundation.NetStandard.Opc.Ua.Gds.Client` | | `OPCFoundation.NetStandard.Opc.Ua.Gds.Server.Common` | `OPCFoundation.NetStandard.Opc.Ua.Gds.Server` | `docs/migrate/2.0.x/packages.md` gains a renamed-packages section — it is the document that undertakes to cover NuGet renames, and had no GDS entry — and the migration skill's package table gains the two rows a 1.5.378 consumer needs. That is the only hand-written content in this PR; everything else is mechanical. Assembly names follow the package ids, so the section also notes that binding redirects, trimming descriptors and signing manifests naming the files directly have to be updated. ## Build infrastructure Everything naming the projects or their output moves with them: - `UA.slnx` and `tools/MigrationAnalyzer.slnx` (both re-sorted — the project lists are alphabetical, and `Gds` now sorts before `Gds.Client`). - `.azurepipelines/expected-packages.txt` — 3 ids, also re-sorted for the same reason. - `.azurepipelines/signlistDebug.txt` and `signlistRelease.txt` — 18 paths each. Their internal ordering is left alone: that group was already unsorted, and reordering a signing manifest is diff noise. - `samples/Reference/ConsoleReferenceServer/Dockerfile`, the two metapackage nuspecs, the analyzer nuspec's package dependencies, and 8 `.csproj` references across tests, samples and tools. `preview-pack.slnx` is generated from `UA.slnx` by `generate-slnx.ps1`, so there is nothing static to edit. Nothing under `.github/workflows`, and no `.ps1`/`.yml` in `.azurepipelines`, names these projects. The migration analyzer's shim tree mirrors the source project layout by its own documented convention, so `Gds.Client.Common/` becomes `Gds.Client/` and its readme table follows. ## Deliberately not changed Prose naming past work: `plans/23` still records that its deferral happened during the "GDS Client.Common modernization", which is the name that effort had. File *paths* in the same document were updated. ## Validation `net10.0`: - `UA.slnx`: 0 errors. 14 warnings, all pre-existing CA1861/CA1850/CA1307 in `XRegistry`, `Types.Tests` and `WotCon.Tests` — none in anything this PR touches. - Assemblies build as `Opc.Ua.Gds.dll`, `Opc.Ua.Gds.Client.dll`, `Opc.Ua.Gds.Server.dll`, with no `*.Common.dll` produced anywhere. - `Opc.Ua.Gds.Tests`: 1,116 passed, 0 failed, 54 skipped. - `Opc.Ua.MigrationAnalyzer.Core.Tests`: 10 passed, 3 skipped. - A repo-wide grep for the old names returns only the intentional old → new mappings in the migration docs. ## Checklist - [x] Existing tests pass - [x] No new tests — this is a rename with no behaviour change - [x] Consumer-visible package rename documented in the migration guide 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Marc SchierandCopilot
|
62658d1a44 |
Fix runtime schema resolution and direct construction (#4426)
# Description Fix runtime schema resolution and provide a supported DI-free construction path for generated data types. - Resolve registered data types by local/index-form and namespace-URI identifiers without using BrowseName as a namespace mapping. - Preserve identifier kind in URI registry keys, so numeric zero and empty GUID/string/ByteString identifiers remain distinct in nonzero namespaces, including during replacement. - Reconstruct typed identifiers during factory URI-to-index fallback while retaining exact lookup precedence and unknown-namespace/remote-server guards. - Make the built-in JSON, XSD, and BSD generators public, stabilize `EncodeableFactoryDefinitionSource`, and document generated activator/factory usage. - Keep the stable-API assertion effective across the netstandard2.1/net8.0 polyfill boundary by inspecting attribute metadata without referencing the ambiguous CLR type. - Merge the latest `master` without dropping upstream changes. ## Related Issues - Fixes #4423 - Fixes #4424 ## Validation - `Opc.Ua.Core.Schema.Tests`: 157/157 on `net10.0` - `Opc.Ua.Core.Schema.Tests`: 157/157 on `net48` - `Opc.Ua.Core.Schema.Tests`: 157/157 with `CustomTestTarget=netstandard2.1` and its `net8.0` consumer - Managed `SchemaAotTests`: 4/4 - NativeAOT Release publish and filtered `SchemaAotTests`: 4/4 - All three review reproductions pass against rebuilt assemblies. ## 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. - [ ] I have addressed **all** PR feedback received. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b17fd166-fdef-4660-a9d5-97c0e69f1bd5 |
||
|
Marc SchierandCopilot
|
4eccafbe2f |
Remove AAS projects and samples (#4359)
# Description Remove the Asset Administration Shell projects and samples from the mainline repository while preserving the complete implementation and history on the `aas` branch. This change: - removes the `Opc.Ua.Aas`, `Opc.Ua.Aas.Client`, `Opc.Ua.Aas.Server`, and `Opc.Ua.Aas.WoT` projects; - removes the AAS test project, AOT coverage, sample, and documentation; - removes AAS entries from `UA.slnx`, the expected package list, and related build configuration; - removes the now-unused `System.IO.Packaging` package version; and - updates remaining documentation and comments so they no longer advertise removed AAS functionality. The preservation branch `aas` points to the original `master` commit before this removal. ## Related Issues - None. ## Validation - `dotnet restore UA.slnx` - `dotnet build UA.slnx -c Release --no-restore` - Repository-wide sweep for dangling AAS project, package, file, and documentation references The full Release build completed with zero errors. It reported 31 existing analyzer warnings in files unrelated to this change. ## 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. (Not applicable: this removes a feature and its tests.) - [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. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Marc SchierandCopilot
|
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 |
||
|
Copilotandromanett
|
8ea319d910 |
Fix macOS ARM64 NativeAOT linker failure in aot-macos-latest (#4298)
# Description
The `aot-macos-latest` job was failing while linking
`tests/Opc.Ua.Aot.Tests` on `macos-latest` / `osx-arm64`, before managed
tests started. The failure was an Apple linker assertion (`too many
large addends`) caused by the size/fixup profile of the NativeAOT test
binary.
- **Root cause**
- `Opc.Ua.Aot.Tests` publishes a large NativeAOT executable.
- On Apple Silicon CI, the final native link hit the linker’s
addend/fixup limit.
- **Change**
- Keep the fix scoped to
`/tests/Opc.Ua.Aot.Tests/Opc.Ua.Aot.Tests.csproj`.
- For macOS only, make the AOT publish size-focused and drop native
debug / debugger / stack-trace payload:
- `OptimizationPreference=Size`
- `DebugType=none`
- `DebuggerSupport=false`
- `StackTraceSupport=false`
- Retain identical-method-body folding.
- **Impact**
- Lowers linker pressure on macOS ARM64 CI.
- Leaves production assemblies unchanged.
- Keeps the managed test surface intact; only the CI-only NativeAOT test
executable shape changes on macOS.
```xml
<PropertyGroup Condition="$([MSBuild]::IsOSPlatform('OSX'))">
<IlcFoldIdenticalMethodBodies>true</IlcFoldIdenticalMethodBodies>
<OptimizationPreference>Size</OptimizationPreference>
<DebugType>none</DebugType>
<DebuggerSupport>false</DebuggerSupport>
<StackTraceSupport>false</StackTraceSupport>
</PropertyGroup>
```
## Related Issues
_Reference all GitHub issues this PR addresses. If there is no issue
yet, open one and link it here._
_If this is a relatively large or complex change, a design must have
been discussed in the related tracking issue and signed off (which
becomes the Architectural Decision Record (ADR))._
## Checklist
_Put an `x` in the boxes that apply. You can complete these step by step
after opening the PR._
- [ ] 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.
- [ ] 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.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>
|
||
|
|
d525bc3f28 |
Bound reconnect cleanup and organize samples by domain (#4276)
# Description This PR fixes bounded ManagedSession reconnect cleanup and reorganizes the sample applications into domain-focused folders. ## Reconnect cleanup - Bounds reconnect integration cleanup with independent 30-second close and dispose limits. - Sends `CloseSession` through the awaited transport request path during `Session.DisposeAsync`. - Preserves explicit `Session.CloseAsync` behavior that waits for or cancels outstanding publish requests. - Covers the disposal request path with a regression test. ## Sample organization - `samples/OpenUsd`: `GeneratorServer`, `SiteCompositionServer` - `samples/Redundancy`: `RedundantClient`, `RedundantServer`, `RedundantPubSub` - `samples/Isa95`: `MinimalIsa95Server` - `samples/MinimalApi`: `MinimalBoilerServer`, `MinimalCalcServer`, `MinimalClient` - `samples/DI`: `PumpDeviceIntegrationServer` - `samples/Lds`: `ConsoleLdsServer` - `samples/Reference`: `ConsoleReferenceClient`, `ConsoleReferenceServer` - `samples/PubSub`: `ConsoleReferencePubSubClient` `UA.slnx` now exposes the same requested domain groups. Project references, tests, runtime sample probes, CI workflows, Dockerfiles, Compose files, helper scripts, and documentation were updated to the new paths. `samples/Robotics/MinimalRobotServer` remains under `Robotics`. ## Validation - `dotnet build UA.slnx` - 3 Redundancy short-haul sample tests - 70 DI sample tests - 914 OpenUSD tests - 1 WotCon model-materialization test - 137 AOT tests - `docker compose config` for all 6 moved Redundancy Compose files ## Related Issues - Fixes #4273 ## Checklist - [ ] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - [x] I have added all necessary documentation. - [x] I have verified that my changes do not introduce new build or analyzer warnings. - [x] I built the complete `UA.slnx` solution locally. - [ ] I ran all tests locally against at least .NET Framework and .NET 10. - [ ] I fixed all failing and flaky tests in the CI pipelines and all CodeQL warnings. - [x] I have addressed all PR feedback received. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Co-authored-by: Marc Schier <mschier@microsoft.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Copilot-Session: a2c2838f-74ae-4997-a1d4-50574d9e0636 |
||
|
Copilotandromanett
|
ef667a1082 |
fix(aot): remove deprecated -ld_classic linker flag causing macOS NativeAOT build crash (#4291)
The `aot-macos-latest` CI job was crashing during NativeAOT link with an assertion in `ld_classic` itself (`Assertion failed: (it != _dylibToOrdinal.end())`). The `-ld_classic` flag was originally added as a workaround for a "too many large addends" linker issue, but recent Xcode toolchains on `macos-latest` runners have a bug in `ld_classic` that causes it to crash when linking large NativeAOT binaries. The classic linker is also deprecated and being removed. ## Changes - **`tests/Opc.Ua.Aot.Tests/Opc.Ua.Aot.Tests.csproj`**: Removed the `<LinkerArg Include="-ld_classic" />` item for macOS builds. The modern default linker handles the binary correctly on current toolchains. `IlcFoldIdenticalMethodBodies` is retained as it still reduces binary size and fixup count. ## Related Issues ## 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. - [ ] 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. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: romanett <7413710+romanett@users.noreply.github.com> |
||
|
|
4ac6084fe5 |
Fix aot-macos-latest CI: resolve "too many large addends" linker assertion on macOS ARM64 (#4278)
The `aot-macos-latest` CI job was crashing during the NativeAOT link
step on macOS ARM64 with an Apple linker assertion failure (`ld:
Assertion failed: (_addend == uniqueIndex && "too many large
addends")`). This occurs when the linked binary exceeds the new Apple
`ld`'s fixup/relocation capacity — `Opc.Ua.Aot.Tests` references nearly
the entire SDK surface and has crossed that threshold.
## Changes
- **`tests/Opc.Ua.Aot.Tests/Opc.Ua.Aot.Tests.csproj`** — two
complementary macOS-only workarounds:
- `IlcFoldIdenticalMethodBodies=true`: reduces total fixups by
deduplicating identical method bodies at the ILC stage, lowering the
relocation count below the Apple `ld` threshold.
- `<LinkerArg Include="-ld_classic" />`: falls back to the classic Apple
linker which does not have this assertion, as a secondary safety net.
```xml
<PropertyGroup Condition="$([MSBuild]::IsOSPlatform('OSX'))">
<IlcFoldIdenticalMethodBodies>true</IlcFoldIdenticalMethodBodies>
</PropertyGroup>
<ItemGroup Condition="$([MSBuild]::IsOSPlatform('OSX'))">
<LinkerArg Include="-ld_classic" />
</ItemGroup>
```
Both properties are conditioned on `OSX` and have no effect on Windows
or Linux builds.
## Related Issues
## 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.
- [ ] 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.
- [x] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [x] I have addressed **all** PR feedback received.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
||
|
Marc SchierandCopilot
|
397db603db |
Add OPC UA for Asset Administration Shell: the published OPC 30270 and v3 draft (#4244)
# Description Implements the **Asset Administration Shell** companion specifications on top of `marcschier/wot-type-binding`, in two halves that a Server chooses between: * **AAS V3** — the [draft](https://github.com/marcschier/opcua-drafts/tree/main/companion-specs/AAS) (`http://opcfoundation.org/UA/I4AAS/v3/`), all sixteen conformance units: the metamodel, the registry built on the abstract xRegistry base, generational materialization, federation, packages with digest-bound integrity, the Digital Product Passport vocabulary, and the Annex F bridge to WoT Thing Descriptions. * **AAS V2** — the published [OPC 30270](https://reference.opcfoundation.org/specs/OPC-30270/full) (`http://opcfoundation.org/UA/I4AAS/`), which maps the AAS V2.0.1 metamodel: object model, ingestion of XML, JSON and AASX, materialization, server and client. The two are different metamodel generations rather than two revisions of one model — V2 keeps `Asset` as a first-class Identifiable, `identification.idType`, Views and ordered collections, none of which survive into V3 — so they are **alternatives**. A Server hosts one or the other; this is documented rather than enforced at startup. Each is named for the generation it maps (`Opc.Ua.Aas.V2`, `Opc.Ua.Aas.V3`, `AddAasV2Server`, `AddAasV3Server`), and what genuinely serves both carries no marker and sits at the root of `Opc.Ua.Aas`: the deterministic NodeId encoding and idShortPath conventions, `AasOptional<T>`, the digest rule, the materialization diagnostics, the AASX container walk, and the value and operation providers, which are keyed on `NodeId` and `Variant` and so carry no version at all. ## What is in here **New libraries**: `Opc.Ua.Aas`, `Opc.Ua.Aas.Server`, `Opc.Ua.Aas.Client`, `Opc.Ua.Aas.WoT`, plus `samples/Aas` and `docs/Aas.md`. **Enabling changes to existing code**, each needed by the above: * The OPC UA `Decimal` DataType added to `Opc.Ua.Types`, with its Part 6 §5.1.10 wire encoding. `BinaryDecoder` gained one internal member, `TryReadRemainingBodyBytes`, because a trailing field with no self-describing length cannot otherwise be read; `Decimal.Decode` is its only caller. Part 6 Table 3 types the unscaled `Value` as an **OctetString whose byte count is `Length` − 2**, not a length-prefixed `ByteString`, so it carries no length of its own — `DecimalEncodingTests` pins the body byte-for-byte against that mistake, and against the `Length ≤ 2` values the same table calls invalid. * The xRegistry projection engine extracted into `Opc.Ua.XRegistry.Server` and `Opc.Ua.WotCon.Server` migrated onto it (−848/+387 lines), with the WoT Connectivity tests passing **unedited**. * Two source-generator fixes, both of which made a model impossible to build (see below). ## Defects found and fixed along the way Running the stack end to end, rather than trusting per-area test suites, surfaced defects that no unit test caught. The ones worth a maintainer's attention: * **The generator emitted non-terminating constructors.** A structure field with no declared default was initialized with `new T()`, so a self-referential DataType (`AASReferenceDataType.referredSemanticId`) recursed until the stack overflowed and `AddOpcUaAas` could not load the model at all. The test host *died* rather than failing. Only the field that closes a cycle is now left null. * **The generator omitted `new` on a redeclared placeholder.** `AASOrderedSubmodelElementCollectionType` redeclares the `SubmodelElement` slot of its base type, and the generated `Add` method hid the base one without saying so, so the V2 model would not compile. No existing model could have hit either defect — both are build failures, so anything that builds today is unaffected. * **The metamodel half had never worked outside its unit tests**: the emitted NodeSet declared no reference-type aliases and was rejected by the loader; the Mandatory `Invoke` Method was never emitted, because a NodeSet is imported as written rather than instantiated from its type; the runtime bound at the authored namespace index instead of the one the Server assigns; and `MethodDeclarationId` was addressed in the wrong namespace. Every materializer test inspected the in-memory object graph, which cannot see whether the graph is loadable — two tests now import it exactly as `RuntimeNodeSetNodeManager` does. * **The client could not see any Property member.** It browsed `HasComponent` with subtypes, but `HasProperty` is a sibling under `Aggregates`, so reading a value failed and invoking failed argument validation. * **Every repeated registry write forked a duplicate group and resource** instead of adding a version, because the identifier was re-derived against a sibling set that already contained the entity itself. This also made the persistence bounds unreachable. * `.gitignore`'s `**/packages/*` silently swallowed the whole package-integrity implementation and its tests, which would have vanished on a fresh clone. ## Specification findings Nine findings against the V3 draft are fixed in [marcschier/opcua-drafts#75](https://github.com/marcschier/opcua-drafts/pull/75). The substantive one: clause 6.1.5's absent-versus-empty rule was **unsatisfiable** for five container fields that materialize as parent components with no node of their own, which made `AAS-LosslessRoundTrip` unachievable. OPC 30270 contradicts itself twice. Both are recorded in `AasV2ConformanceUnits.cs` and the documentation rather than smoothed over: Table 83 spells the unit `I4AAS MultiLangaugeProperty` where Table 85 spells it correctly, and Table 85 lists an `I4AAS Security` unit that Table 83 never defines. Table 84, which would assign profile URIs, is empty, so `ServerProfileArray` gains no I4AAS entry. ## Known limitation Calling an `Operation` **with arguments** answers `BadTooManyArguments`. Method resolution works from both the instance NodeId and the `MethodDeclarationId`, and the argument definitions are published, but `NodeState.AddChild` does not route through `MethodState.FindChild`, so a Method imported from a NodeSet leaves its typed `InputArguments` null. Closing this means changing how the shared NodeSet import claims typed children, which affects every consumer of runtime NodeSets and is out of scope here. It is documented in `docs/Aas.md` §11, the conformance matrix marks `AAS-OperationInvoke` partial rather than claiming it, and `samples/Aas` exercises the path and reports the status rather than working around it. V2 is unaffected — its `Operation` is an OptionalPlaceholder with no declared arguments. ## Related Issues - No tracking issue yet. Opening as a **draft** for design discussion first, per the ADR guidance below; happy to open one and link it once the shape is agreed. ## 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. - [ ] I have addressed **all** PR feedback received. ### Verification | Check | Result | |---|---| | AAS tests | **670 passed, 0 failed** on net472, net48, net8.0, net9.0, net10.0 | | Coverage | Client 80.8 %, Server 81.2 %, `Opc.Ua.Aas` 82.0 %, WoT 82.4 % — all above the 80 % gate | | Libraries | `Opc.Ua.Aas`, `.Server`, `.Client`, `.WoT` build clean on every target framework, zero warnings | | Regression | `Opc.Ua.Types` 8677, `Opc.Ua.WotCon` 1094, `Opc.Ua.XRegistry` 198, `Opc.Ua.Server` 4050 — all green | | Sample | `samples/Aas` builds warning-free and runs end to end | | Merge validation | Solution builds clean on net10.0 and net48; WoT Types 504, WotCon 1111, AAS 670, XRegistry 198 tests passed | | CI | All 99 checks green on `ff9298e44`; only the CLA signature status is outstanding | ### Review pass A full review against both specifications, and for API consistency and correctness, produced twelve fixes. Each one has a test that fails without it. | Defect | Where | Why it mattered | |---|---|---| | V2 projection aborted on **every** `File` read from a document | `AasV2EnvironmentRuntime` | The runtime bound `File/Value` unconditionally, the materializer emitted it only with content, and the builder throws `BadNotFound` for an unknown NodeId — aborting the whole registration. No reader ever populates `AasFile.File`, so no AAS V2 document could be hosted at all. | | `aas-suppl` anchored on the wrong part | `AasxPackageReaderCore`, `AasxPackageWriter` | IDTA and the Package Explorer anchor it on the environment part; we used the origin part. Every real `.aasx` returned zero supplementary files, silently. Affects both halves. | | Federation failed closed on pooled connections | `AasFederation` | `ConnectCallback` fires only for new connections, so the second fetch to an origin — and all of net472/net48/netstandard2.1 — failed with a security-flavoured error. The peer is now validated *inside* the callback, which makes reuse safe by construction. | | Malformed `Decimal` escaped as `ArgumentOutOfRangeException` | `BinaryDecoder` | A declared body shorter than `Scale` produced a negative read length. Two of the three test cases had been passing for the wrong reason. | | V2 and V3 cross-wired their value providers | `OpcUaAasServerBuilderExtensions` | Both registered `IAasValueProvider`; whichever was added first served both AddressSpaces. V2 now has its own contract, so co-hosting is harmless rather than silently wrong. `EnvironmentFolder` was also bound and ignored on the V2 path. | | Export filter skipped three nesting kinds | `AasEnvironmentExporter` | Content under `Entity`, `AnnotatedRelationshipElement` and `Operation` was exported unevaluated *and* stamped with a Digest presenting it as complete (clause 6.5.10). List authorization paths also shifted after a denial. | | Concealment held at the Method but not in the projection | `AasRegistryModel` | `GetSubmodel` answered `BadNotFound` while an anonymous Browse revealed the node, its identifier, semanticId and digest (clause 6.5.7). | | File handles were unsynchronized and unowned | `AasElementFileManager` | The async node manager holds no lock, so concurrent Calls raced a bare `Dictionary`; sequential handles let any Session read, seek or commit another's. Writes were also unbounded. | | Projection host was unimplementable outside the stack | `AasEnvironmentProjection` | The handle demanded a type with an internal constructor — our own tests used `FormatterServices.GetUninitializedObject` to get round it. Now mirrors WotCon's interface-typed registration. | | Embedded `FileType` omitted 7 of 10 Mandatory members | `AasEnvironmentMaterializer` (V2) | OPC 10000-3 6.4.4 requires them all; without `Size` and `OpenCount` a Client cannot size a read or detect an abandoned handle. | | Round-trip oracle ignored most of the model | `AasRoundTripComparer` | `Blob`, `File`, `ReferenceElement` and `BasicEventElement` had no case arm, so their fields compared equal by falling off the switch; `valueType`, `semanticId`, `entityType` and `Key.type` were never read. The clause 6.4 losslessness claim rested on it. | Known, unchanged: calling an `Operation` with arguments answers `BadTooManyArguments`, because `NodeState.AddChild` does not route through `MethodState.FindChild`. Documented in `docs/Aas.md` §11. Not addressed in this pass, and worth a maintainer decision: OPC 30270 §9.2 says all NodeIds of Nodes not defined in the companion specification **shall not** use the standard namespaces, and Table 87 marks a vendor-specific instance namespace Mandatory. Both halves currently mint instances in the companion namespace. Separating them touches NodeId minting, the runtime and the client together, so it is better done deliberately than folded into a review pass. This is a large change and touches shared code (`Opc.Ua.Types`, the source generator, the xRegistry projection). It is deliberately a draft: the enabling changes are separable if maintainers would prefer them split into their own PRs. The latest `master` is merged into this branch in `0dd0ff0e1`. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8 Copilot-Session: c3d88950-4a65-4ff6-87d2-1f545a875c9b Copilot-Session: b8ab3763-b9bd-46ae-b0ef-045db26e5782 |
||
|
|
aad8350bc1 |
Pluggable symmetric cryptography, an awaitable asymmetric path, registrable security policies, and PubSub crypto offboarding (#4206, #4207, #4208, #4210) (#4232)
# Description Closes the three crypto-offboarding follow-ups split out of #4192. Each was recorded there as knowingly *not* done, with the reason and what would justify revisiting it. Those conditions are met, and where one of them turned out to be unachievable that is written down rather than worked around. The safety property throughout: **every seam is opt-in and null-checked, and in the default software configuration the added code is observably inert.** No interface dispatch on the per-message path, and the asynchronous paths complete synchronously — so channel locking order, buffer ownership and error sequencing are unchanged for a deployment that configures nothing. ## #4207 — symmetric, key-derivation and RNG provider seams `ISymmetricCryptoProvider`, `IKeyDerivationProvider` and `ISecureRandomSource`, added on the terms #4207 set: only once a concrete requirement arrived (a validated module that must perform *every* operation, not just the asymmetric ones), behind a fast path, with `SymmetricChannelCryptoBenchmarks` as the gate. They are **facets discovered by type test**, not members of `ICryptoProvider`, so providers written against the shipped interface still compile. `PlatformCryptoProvider` implements all three, which answers the "an interface nothing implements" objection recorded in the issue. `CryptoProviderFacets` returns `null` both when no registry is configured *and* when resolution lands on the platform, because the platform facets are the inline code. Resolution happens once, in `CalculateSymmetricKeySizes`, and is held for the life of the channel. `CryptoCompliance.GetUnservedOperationPurposes` closes the failure the seam would otherwise introduce: a provider bound to `ChannelSymmetric` without implementing the facet is silently replaced by the platform, so a deployment could believe its validated module performed the per-message cryptography while the platform did. Under `FipsOnly` that now refuses to start. ## #4208 — the asymmetric path is awaitable `RSA` and `ECDsa` are .NET's contracts and cannot be replaced without making every ready-made hardware and cloud implementation unusable. Instead an implementation may **also** declare `IAsyncRsaKey` / `IAsyncEcdsaKey`, which the stack finds by type test. Now asynchronous: the secure channel open and renew path, user identity token signing and decryption, and session activation. A software key declares neither facet, so those paths complete synchronously and nothing about their ordering changes. `ReadAsymmetricMessage` is refactored into shared helpers rather than duplicated — adding an `await` was not worth two hundred cloned lines of signature, padding and endpoint-selection logic. `AsymmetricMessage` / `AsymmetricWriteResult` exist because an asynchronous method cannot have `out` parameters. The channel no longer serialises its state on a monitor. A monitor cannot be held across an `await`, and a plain `SemaphoreSlim` was not an option at the time: the code relied on re-entrancy in eight places (`HandleIncomingMessage` holds the lock and calls `ForceChannelFault`, which takes it again; `ForceReconnect` and `Shutdown` do the same). `ChannelGate` replaces it at 32 sites and `DataLock` is `[Obsolete]`. That reliance has since been removed as well — see [Removing the gate's re-entrancy](#removing-the-gates-re-entrancy) below. ### Defects this surfaced, all fixed and regression-tested **Detached work started inline.** Writes were started with `_ = WriteBuffersAsync(...)`. An async method runs its prologue on the caller's stack, so `LeaveInheritedContext` — which exists to stop *detached* work inheriting the right to re-enter the gate — was stripping the **caller's own** entitlement; when the send then completed synchronously, the completion blocked on a gate that very thread was holding. The client sent its OpenSecureChannel request and never processed the response. Writes are now queued. **A synchronous gate handle held across an `await`.** `Enter()` records the acquiring thread so an inline completion callback can re-enter — `ChannelAsyncOperation` invokes its callback both inline and detached. That record is only sound while the holder is synchronously on that thread; the token renewal path held it across an await, so a reused pool thread was recognised as the holder. **`SignAsync` rejected the policy that signs nothing.** It validated the certificate before the algorithm, so `SecurityPolicies.None` — which carries no certificate — threw where the synchronous `Sign` returns `null`. **`EnterAsync` lost the holder when contended** *(found in review)*. The `AsyncLocal` recording ownership was written inside the awaiting method, and .NET restores the caller's execution context when an async method completes, so the write was discarded. The caller returned owning the semaphore while `IsHeldByCurrentContext` reported `false` — and any nested `Enter()` then blocked forever on a gate its own context held. Reachable remotely: `HandleIncomingMessageAsync` → error path → `ForceChannelFault`. The holder is now published from the caller's frame before the wait, and only its depth is raised in the continuation. **Queued writes lost their order** *(found in review)*. Sequence numbers are assigned under the gate, but independently queued work items reach the transport's FIFO send lock in arbitrary order, so a peer could reject a chunk with `BadSequenceNumberInvalid`. Writes are now appended to a chain: still off the caller's stack, but a write cannot start before its predecessor finishes. ### Four more this surfaced in CI, on paths only the integration suites reach Queuing the writes to fix the deadlock above turned out to have moved three implicit guarantees, and the gate had one more liveness bug. Each was reproduced locally against a clean baseline before it was changed. **A faulting channel's error message never reached the peer.** A listener channel sends a UASC `Error` describing why it faulted and closes the transport as its next statement. That worked only because the write started on the caller's stack, so the bytes reached the socket first; queued, the close discarded them and the client saw `BadConnectionClosed` instead of the certificate status it was told to expect. Nine `SecurityCertValidationTests` cases assert exactly that status. The terminal error is written inline again, on a path that touches the gate only to disclaim what it inherited and reports nothing — a channel that is already faulted is owed nothing beyond its buffer. **The certificate of a rejected sender leaked.** An asynchronous method cannot have `out` parameters, so the asymmetric read returns a record instead. That silently dropped an ownership guarantee: an `out` parameter reaches the caller even when the method goes on to throw, so the caller's `catch` disposed the sender certificate and reported it to the audit. A return value does not — nine leaked certificates, one per rejection case, which the leak detector fails the run on. The parsed certificate is now handed to the caller before anything that can reject the message, restoring both the disposal and the audit. **The gate could strand its own permit.** It released only when the holder's depth returned to zero, but that depth lives in an `AsyncLocal` and is therefore shared with every context forked from the owner's — a timer callback whose timer was created inside the region, for instance. Such a context entering the gate raised the shared depth, so the owner's own exit saw a non-zero depth and returned without releasing, and nothing afterwards could: the nested exit is not the owner. Every later entry on that channel blocked forever. `Opc.Ua.Features.Tests` hung after its tests had finished, with a receive loop waiting on the gate and the server unable to stop. The owner now releases when it leaves; depth still gates re-entrancy, but liveness no longer depends on a count another context can move. **Write completion throttled the writer.** Reporting a completed write sat inside the ordering chain, and the client channel implements that callback by entering the gate — so every write queued behind a gate the receive loop contends for on each incoming chunk. A session with fifty subscriptions publishing continuously had its publish requests throttled enough that the client kept topping them up past its own limit, which `Subscriptions.Classic` asserts against. Completion is now reported off the chain, which covers only what ordering actually requires. ### Still synchronous, and why Certificate, certificate request and revocation list signing cannot be made asynchronous at all: `X509SignatureGenerator.SignData` is invoked by .NET's own builders. Service faults and the `Reconnect` override are reached from synchronous call sites (`SendServiceFault` alone has eight) and are left as they are behind scoped, commented suppressions. All three are documented rather than left to be rediscovered. ## #4210 — PubSub This one produced a finding rather than the API the issue asked for, and the finding is the point: > **With a standard Security Key Service the key necessarily exists in process memory.** `GetSecurityKeys` (Part 14 §8.3.2) returns raw key bytes over the wire, so the "key never leaves the device" property #4192 achieved for client and server **cannot** be achieved for PubSub through the SKS pull profile. That is a property of the specification, not of this stack. A wrapped-key envelope would change what is on the wire and break interoperability with third-party key services and publishers, so it was rejected rather than built. What is achievable is delivered: the per-message AES-CTR and HMAC route through `ISymmetricCryptoProvider`, so a validated module performs them. `IPubSubSecurityPolicy` is **not** modified — new members would break every implementation of it; the provider is supplied through the policy constructor. `PlatformSymmetricCryptoProvider` gained AES counter mode, which the seam should have covered anyway since the algorithm enum names it. Key lifetime is also tightened: unpacking an SKS response copied key material into intermediates that `ByteString.Create` then copied again, leaving plain keys in the heap for the lifetime of the process. Those are now zeroed. ### Two wiring gaps this depended on, both found in review The registrations above were **inert**, so this section originally claimed more than the code did: - `CryptoProviderConfiguration` was applied nowhere outside tests, so `AddCryptoProvider(configure)` never reached the registry in a real host. The registry now applies every registered configuration when it is first resolved — which also affects #4207's DI story. - `PubSubSecurityWrapperResolver` selected `PubSubAes256CtrPolicy.Instance`, the provider-less static singleton, ignoring the provider-backed bundles registered for it. A `FipsOnly` deployment therefore certified while the platform did the work. The resolver now selects from the registered policies; the container and `PubSubApplicationBuilder` both supply theirs. A third, smaller one: `CryptoProviderBuilder.Use` published a **policy-scoped** random source as the process-wide nonce source, so `.For(purpose, policyUri).Use(hsm)` redirected nonce generation for every other policy too. Only an unscoped binding does that now. ## #4206 — a provider can contribute a security policy Added in review: the policy set was fixed at compile time, so a provider could contribute key custody but not a *profile*. It is now table-driven and open, and the blockers #4206 listed are gone. ```csharp using IDisposable registration = SecurityPolicies.Register( new SecurityPolicyInfo("urn:vendor:SecurityPolicy#CustomProfile", "CustomProfile") { PlatformSupport = () => true, SupportedCertificateTypes = [ObjectTypeIds.RsaSha256ApplicationCertificateType], IsDefault = true }); services.AddOpcUa() .AddCryptoProvider(crypto => crypto .For(CryptoPurpose.ChannelSymmetric, "urn:vendor:SecurityPolicy#CustomProfile").Use(module)) .AddSecurityPolicy(customPolicy); ``` - **The reflection is gone.** Four lookup tables were built by reflecting over the fields of `SecurityPolicies` and `SecurityPolicyInfo`. They are one explicit table now, which removes the **last reflection in `Security/Constants`** — so this is a trimming and Native AOT improvement as much as a feature. - **`SecurityPolicyInfo` is constructible from another assembly** and carries the metadata the helpers need, so `IsPlatformSupportedName`, `GetDefaultUris`, `MapSecurityPolicyToCertificateTypes` and `GetCurveFromCertificateTypeId` read that table instead of each being hand-written. Adding a policy is one entry rather than five edits. - **The dead `#if CURVE25519` conditional is deleted** from `Security/Constants`. - Registration is copy-on-write, so the read path stays lock-free. Shadowing a built-in policy requires `replaceExisting: true` and is reversible by disposing the registration. `RegisterLightsUpCurvePoliciesFromOutsideCore` is the acceptance test the issue asked for: it registers `ECC_curve25519` and `ECC_curve448` from the test project and asserts they become discoverable through the public API. **Registering a policy makes it advertised and resolvable; it does not supply the cryptography behind it.** For those two curve profiles the in-tree key agreement is still behind a compile symbol no project defines plus a BouncyCastle dependency, so a deployment lighting them up provides the operations through a provider. That division is the point: the policy set says *what* is offered, the provider says *who* performs it. ## Removing the gate's re-entrancy Folded in from #4242, which was merged into this branch. `ChannelGate` was re-entrant, and that forced the design behind most of the defects above. A holder that awaits releases its thread, so ownership could not be tracked by thread identity — it had to live in an `AsyncLocal<Holder?>`. A logical context is *inherited*, so anything started while the gate was held inherited the right to re-enter and ran inside the guarded region alongside whatever started it, unless it opted out by hand via `LeaveInheritedContext()` at each of eight sites, enforced by review alone. Five of the defects listed above trace to that mechanism, including both CI hangs. So the *requirement* was removed rather than the mechanism, and the `AsyncLocal` then deleted itself. Every path that re-entered now calls a lock-free `Core` method (`ForceChannelFaultCore`, `ShutdownCore`, `ForceReconnectCore`, `CompleteConnectCore`), and three regions stopped running foreign code under the lock: the `RequestReceived` dispatch is deferred until the gate is released, `OnTransportError` no longer wraps the `HandleSocketError` virtual, and the callback setters and `EndpointDescription` publish a single reference with a volatile write instead of locking — the last is load-bearing, because `AsyncResultBase.OperationCompleted` invokes the reverse-hello callback inline while the gate is held. `ChannelGate` is now a `SemaphoreSlim(1,1)` behind a `Releaser`; `Holder`, the thread-identity fallback, `LeaveInheritedContext`, `IsHeldByCurrentContext` and all eight disclaimer calls are gone, with the call shape at all 32 sites unchanged. `SaveIntermediateChunk`, `GetSavedChunks` and `DoMessageLimitsExceeded` gained a `gateHeld` argument, because the client reaches them both with and without the gate and must pick the matching teardown — without it an oversized `OpenSecureChannel` response deadlocked the receive loop against itself. The remaining paths were not found by reading the code, since that is what missed them originally. A temporary strict mode made every re-entrant acquisition throw with its stack, the suite was run under it until clean, and it was deleted with the re-entrancy it measured. It caught two sites review had missed: the `EndpointDescription` **property getter** taking the gate, and the `RequestReceived` dispatch to application code happening inside the guarded region. Two deliberate residuals are documented for reviewers: `Dispose` no longer takes the gate (unavoidable — it is reached from `ChannelFaulted` with the gate held, where the old re-entrant acquisition excluded nothing anyway), and the deferred request dispatch builds `SecureChannelContext` just after gate release rather than under it. ## Testing New: the three seams and their platform implementations, facet discovery and resolution precedence, `FipsOnly` refusing a provider that cannot serve what it was bound to, AES counter mode (including a non-block-aligned round trip and a check that the counter carries, since one that did not would silently reuse key stream), `ChannelGate` (exclusion, re-entrancy across an await, re-entrancy on a reused thread, and the deadlocks above), the asynchronous channel paths, and the PubSub policy provider. Each review finding got a test that fails without its fix: - `ContendedEntryStillRecordsTheHolderAsync` — established contention deterministically, then re-entered; failed on the pre-fix gate. Removed with the re-entrancy it covered; `ChannelGateTests` now asserts the non-re-entrant contract instead. - `SecureClientAndTcpListenerKeepChunkOrderUnderConcurrentRequestsAsync` — 60 concurrent multi-chunk `SignAndEncrypt` requests over a real loopback listener; failed 3 runs out of 3 with the previous dispatch, passes 3 out of 3 with the chain. - `DependencyInjectionRoutesPubSubCryptoThroughTheRegisteredProviderAsync` — builds a container, registers a counting provider, wraps a message and asserts the provider actually performed the encryption and the signature. This is the assertion the PubSub claim above now rests on. - `PolicyScopedRandomBindingDoesNotEscapeItsPolicy`. Two further assertions are the ones that would catch a silent defect: a provider and the platform must produce **byte-identical** output — a validated module that disagreed could not interoperate — and with a software key the returned `ValueTask` must report `IsCompletedSuccessfully`, which is what makes "ordering is unchanged" checkable rather than merely asserted. `SymmetricChannelCryptoBenchmarks` gains a provider-registered variant beside the existing baseline, so the cost of the indirection is measured rather than assumed. Verified locally, every suite at or above its recorded baseline: `dotnet build UA.slnx -c Release` **0 warnings, 0 errors on all six target frameworks**; Core **4413** (net10.0); Core.Security **617**, with its certificate-leak detector clean; PubSub **1350** plus PubSub.Diagnostics **54** and PubSub.Udp **233**; Features **156**; Subscriptions.Classic **32**; Server integration **4053**; Client integration **2123**; MigrationAnalyzer **151** and MigrationAnalyzer.Core **13**. Zero failures in each. Core moved from 4388 to 4381 when the gate change landed — eight re-entrancy tests were removed along with the behaviour they covered, and one deadlock regression test added — and has since grown to 4413 with the registry isolation, provider-output and security-review tests. `ChannelGateTests` was rewritten around the new contract — exclusion across both entry modes, an uncontended asynchronous entry not suspending, re-entry blocking by contract, and the property that used to be the hazard: work started while the gate is held is excluded without having to opt out. Every CI failure this PR produced was reproduced locally, diagnosed against a clean `origin/master` baseline in a second worktree, and fixed at the source. Two hangs were found from `--blame-hang-timeout` dumps read with `dotnet-dump`. One failure that looked like a candidate was confirmed **not** to be: an intermittent `Opc.Ua.Aot.Tests` connect failure, pre-existing on `origin/master` (5 of 8 baseline runs failed, versus 3 of 14 here). ### The publish request overshoot `Subscriptions.Classic.PublishRequestCountAsync` failed on the Windows net10.0 leg with **207** outstanding publish requests against a cap of `max(maxServerPublishRequest, subscriptions)` = 50. It failed in 2 of 6 pipeline builds on this branch, never on `master` in 13 sampled builds, and never locally in 12 attempts including under deliberate CPU contention. The mechanism is a genuine race. `ClassicSubscriptionEngine.QueueBeginPublish` was a **check-then-act**: it read `GoodPublishRequestCount`, compared it against `GetDesiredPublishRequestCount(false)`, and issued a publish if it was below. It runs on every publish completion, so *n* completions that overlap each read the same value, each conclude they are below the cap, and each issue — overshooting by up to *n* per round. The read also *lags*: a request only becomes visible to `GoodPublishRequestCount` once `AsyncRequestStarted` has recorded it, which happens after the request has already been sent. That code is pre-existing and was untouched by this PR. What this PR changed is how hard the race is hit: the server used to hold the channel gate across request processing, which serialised every request on a channel, so publish completions arrived one at a time. Dispatching `RequestReceived` outside the gate — an intended improvement, since a channel should not serialise on application request-processing time — lets them complete concurrently. Rather than change the publish pipeline blind, the race was first made reproducible. `ConcurrentPublishReEvaluationDoesNotExceedDesiredRequestCount` drives 64 barrier-released completions against a mocked context whose count lags the send exactly the way the session's does. It issued **64 requests where 5 were desired** — the same defect the CI leg hit, now deterministic and running in milliseconds. The engine now tracks the requests it has in flight and reserves a slot with a compare-exchange before sending, so the decision and the send are one atomic step. The reservation is released when the request completes, when the send is declined, or when it throws. The repro test then issues at most the desired count. `StartPublishing` is deliberately left **uncapped**. It is the recovery valve that refills a pipeline whose requests are outstanding but no longer expected to return, so gating it on the in-flight count would remove the only path out of a stalled pipeline. An earlier revision of this fix did gate it, and that is the one change here that could not be justified on the evidence — it was reverted. The assertion was **not** loosened, retried away, or marked flaky: it caught a real throttling regression earlier in this PR and is worth keeping sharp. Validated: the repro test red before the fix and green after; `Subscriptions.Classic` 32/32 **five** consecutive runs on net10.0 and once on net48; `Opc.Ua.Client.Tests` 2124 on net10.0 across four consecutive runs and 2127 on net48; `Opc.Ua.Subscriptions.Tests` 604 passed with this fix in place. ### The SSH.NET advisory that turned CI red repo-wide Partway through this work every build job began failing with `NU1903: Package 'SSH.NET' 2025.1.0 has a known high severity vulnerability` ([GHSA-q939-rpr3-3284](https://github.com/advisories/GHSA-q939-rpr3-3284), `<= 2025.1.0` affected, first patched `2026.0.0`). Nothing in the dependency graph changed — the advisory was published. Build 17026 failed on it where build 17000, two hours earlier on an identical graph, had passed every build job. SSH.NET is not referenced anywhere in the repository; it arrives transitively through `Testcontainers.Kafka`. `master` landed the remediation independently while this was being investigated (a central transitive pin to `2026.0.0`, which `CentralPackageTransitivePinningEnabled` exists for). This branch briefly carried its own identical pin, which made the merge commit hold **two** `PackageVersion` entries for SSH.NET and fail restore with `NU1506`. The duplicate is removed and `master` is merged, so the pin now comes from one place only. The audit was **not** suppressed: `NuGetAudit` stays enabled and `NU1903` stays an error. Verified after the merge — `dotnet restore UA.slnx` reports neither NU1903 nor NU1506, and `dotnet build UA.slnx -c Release` is **0 warnings, 0 errors** with the audit live. The remaining red check in build 17026 was not from this change either. `aot-ubuntu-latest` is the intermittent `Opc.Ua.Aot.Tests` failure already noted above — `ReconnectSessionAsync` timed out at 30s with 135 of 136 passing. It remains below the `origin/master` baseline rate (4 of 15 sampled here against 5 of 8 on the baseline), and the managed reconnect paths are green throughout: `Opc.Ua.Client.Tests` 2124 across four consecutive runs and `Opc.Ua.Subscriptions.Tests` 604. ## Documentation `docs/CryptoProvider.md` gains sections on substituting the symmetric primitives, using a key served over a network, and PubSub — including what cannot be claimed and why, the scope rule for a random-source binding, and how to supply a provider-backed policy outside the container. `docs/MigrationGuide.md` covers the `[Obsolete]` channel virtuals and `DataLock`, and the non-re-entrant gate's contract for channel subclasses. `docs/WhatsNewIn2.0.md` is updated. `plans/cryptooffboard.md` is deleted now that the work it planned has landed. ## Note for reviewers This touches the secure channel handshake, which is the most security-critical code in the stack. Nine distinct concurrency and lifetime defects were found and fixed in it during this work — three during development, two in review, four from CI — so `ChannelGate`, the write path and the certificate ownership across the new suspension points deserve the closest reading. Five of those nine traced to the gate's `AsyncLocal`, which is why the re-entrancy was subsequently removed and the `AsyncLocal` deleted. The wrapper/`Core` split and the two documented residuals in that section are the parts most worth a second reading. The #4206 work removes the last reflection from `Security/Constants` and rewrites the policy lookup tables, so `SecurityConfiguration.SupportedSecurityPolicies` and the endpoint set it feeds are worth a second look: an earlier revision of it changed that set and broke six server tests, which is now covered by `SecurityConfigurationBuildsExpectedSupportedPolicySet`. ## Related Issues - Fixes #4206 - Fixes #4207 - Fixes #4208 - Fixes #4210 - Follow-up #4250 — this PR makes the security policy set injectable, but nothing in the stack resolves `ISecurityPolicyRegistry` from DI yet, so every production path still lands on the `Default` fallback. #4250 tracks threading it through `Session` and `UaSCBinaryChannel`, which is what makes the injection points already added here load-bearing. ## 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> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> Copilot-Session: 7a68ca72-83e8-496c-b2f5-3df4c718320b Copilot-Session: 3f5ab76a-c0ba-4f47-87ac-54d1f4182c6b |
||
|
Marc SchierandCopilot
|
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 |
||
|
|
466c5cb982 |
Robot intent follow-ups: multi-controller executors, spec fixes, and an MCP tool library (#4195)
Follow-ups to #4165, plus the work that followed from re-checking this branch against the specification. Everything started as a defect found by review or by probing the running code; the specification is unchanged except where noted. The TLS certificate lifetime fix (#4196) and the OpenUSD work (#4198) were both merged into this branch. #4196 has since landed on master, so it no longer appears in this diff. #4198 was closed in favour of this PR and its content is here. The MCP trim/AOT work (#4228), the `DataGenerator` null `NodeId` fix (#4227) and the NodeSet external-parent import fix (#4229) have since been merged in as well. All three are closed in favour of this PR and their content is here, described under *Making the MCP libraries trim and AOT clean*, *A generator that returned the value you opted out of* and *A NodeSet parent that lived outside the import batch* below. ## A two-arm cell could not be built `IntentExecution` carried no controller identity, and there was effectively one `IIntentExecutor` per server. An executor handed an execution therefore could not tell **which arm** it was for, so a cell with two `IntentControllerType` instances — an ordinary configuration — was not expressible through the builder or DI. Both halves are addressed: - `IntentExecution` now carries `ControllerId` (`NodeId`, the OPC UA identity) and `ControllerName`, so one shared executor can discriminate; - DI gained per-controller registration — `AddRobotIntentExecutor<TExecutor>(controllerBrowseName)` and an instance overload — so two genuinely different executors can coexist. Resolution order is `WithExecutor` → per-controller DI registration → the single DI executor → a build-time throw. There is still **no silent fallback** to a rejecting executor; that was a Critical defect fixed in #4165 and it stays fixed. `IIntentExecutor` is unchanged, so this is purely additive. ## Declarations that were quietly untrue - **`Accepts<T>` silently discarded a redeclaration.** Declaring an intent type twice — say to narrow `cancelSupported` on the second call — kept the first, wider declaration and said nothing. That is the same class of defect as the capability-honesty problems #4165 fixed (`PauseSupported` declaring a pause the host could not perform; `Aborting` degrading silently to `Buffered`). Identical redeclarations remain idempotent; conflicting ones now throw `BadInvalidArgument`. - **`RI-Interop-40010` assumed Annex B item 3 rather than checking it.** It now requires exact set equality between the OPC 40010 `TaskProgramName` values and the Robot Intent `ProgramType.ProgramId` values. Both sides are readable from the address space without commanding the robot, which is what makes the requirement structural under §12.1. ## Two things the specification requires that this did not do Re-checking the implementation against the merged draft revision turned up two genuine violations. Both are fixed, and the NodeSet is untouched by either. - **Every graphed mission reported `Failed` on success.** `MissionRules.SelectTransition` returned `null` for two different situations — a graph *terminus*, which is the normal success path, and a branch point whose conditions all evaluated false — and the caller collapsed both into `Failed`/`NoTransition`. A three-way result (`Terminus` / `Selected` / `NoConditionMatched`) separates them, so a mission that runs to its end now completes. The failure reason surfaces through the inherited Part 10 result container rather than a new member. - **Facet URIs were never published.** An upstream `may` became a `shall` in the revision this branch adopts. `RobotIntentConformanceUris.Facets` held 31 URIs with **zero** production references — the set existed but nothing emitted it. The published set is now derived from `RobotIntentFacetCalculator`, the same authority that already computed `SupportedFacets`, rather than from a second hand-maintained list that could drift; a reflection test pins the two together. ## An easy client mistake `RequestAuthorityAsync` returns a lease the caller must remember to inspect. Forgetting makes every later submission fail with `ControlNotOwned` for reasons that are invisible at the call site. `RequireAuthorityAsync` throws instead, and names the current owner. `RequestAuthorityAsync` is untouched — inspecting and branching is legitimate. ## Letting an agent drive the robot New `tools/Opc.Ua.Mcp.Robotics` gives a language model the intent API as MCP tools: 36 across discovery, monitoring, direct control and missions, behind a `Robotics` tool profile, composable into any MCP host exactly as the existing OPC UA tool packages are. The `IntentViewerClient` sample hosts them behind `--mcp` (stdio by default, HTTP when `--view`, since MCP stdio and the in-process viewer would otherwise share stdout), and the sample gained a pallet scene so an agent can pick and stack parts and watch the result in the OpenUSD viewer. All the scene logic is server-side — no viewer or connector changes. Four gaps in the client API had to be filled first, since the tools are a thin layer over it and could not have been written honestly without them: `ReadStateAsync`, `ListOperationsAsync`/`ListMissionsAsync`, controller-level `CancelIntent`/`CancelAll`/`Pause`/`Resume`/`Retry`/`ReleaseControl`, and `WaitForCompletionAsync`. The tools refuse rather than paper over: a refusal from the server is returned with its `IntentFailureEnum` and never retried, and command authority is never acquired as a side effect of a tool call. **One defect this surfaced:** `--profile robotics` shipped 36 tools that could not do anything. Every robotics tool resolves a named OPC UA session and only the connection tools can open one, so with no `Connect` in the catalogue the whole profile failed. A test had pinned the defect in place, asserting the profile did *not* contain `Connect` on the line below the `diagnostics` assertion requiring it. Each package now declares its own requirement in its registration extension rather than the host special-casing it, so an application embedding `Opc.Ua.Mcp.Robotics` directly gets a usable catalogue without reproducing the host's pairing. `Full` still takes the connection tools from the core package, so they are never registered twice. The replacement test states the invariant — a profile exposing session-scoped tools must expose the tools that open a session — rather than the instance. ## Making the MCP libraries trim and AOT clean *Merged from #4228.* `Opc.Ua.Mcp.Core` is now `IsAotCompatible` on `net10.0`, and the reflection the analysers then reported is gone. Turning the analysers on is the point: until now they were not running on these libraries at all — including on the tool library this branch adds. - **`OpcUaJsonHelper.Serialize`** went through `JsonSerializer` (IL2026 / IL3050). The conversion helpers in that class already reduce every OPC UA value to a closed set — `null`, the primitive scalars, `string`, `Dictionary<string, object?>` and lists thereof — so the JSON is now written directly with a `Utf8JsonWriter`. Output is unchanged, and that is asserted rather than assumed: `OpcUaJsonHelperTests.SerializeMatchesTheReflectionBasedSerializer` compares the new writer byte for byte against `JsonSerializer` for every shape the helpers emit, including all numeric widths, the custom-formatted scalars, nested sequences and strings that force the encoder to escape. One deliberate contract change: arbitrary object graphs are no longer accepted. Serializing them needs reflection, so `Serialize` now throws `NotSupportedException` naming the type instead of silently stringifying it. No call site in the repository passes anything outside the closed set. - **`McpSchemaFilters`** used `JsonSerializer.SerializeToElement` on a `JsonObject`; it now parses the node's own text, which needs no reflection. `Opc.Ua.Mcp.Diagnostics` is deliberately **not** marked. It depends on SharpPcap and on the reflective service-call dissection in `Opc.Ua.Core.Diagnostics`, neither of which is trim safe — which is why that package does not opt in either. Two improvements are kept regardless: `PacketDecodeTools` serializes `DecodedServiceCall` through a source-generated `JsonSerializerContext`, and `PacketCaptureTools.ListInterfaces` now carries the `RequiresUnreferencedCode` / `RequiresDynamicCode` annotations that `NetworkInterfaceEnumerator.ListLocalInterfaces` already declares, so the constraint is visible to callers instead of implicit. Nothing is suppressed. Marking an assembly `IsAotCompatible` only proves its own IL is clean; it says nothing about whether the library still functions once reflection is gone. So `Opc.Ua.Aot.Tests` references `Opc.Ua.Mcp.Core` and exercises it with JSON reflection disabled, stating what is and is not true today: - tool **results** serialize without reflection — this would fail outright if the `Utf8JsonWriter` change regressed; - tool **registration** still requires reflection, because the MCP SDK builds each tool's input schema by asking `JsonSchemaExporter` for a `JsonTypeInfo` of every parameter type. Without a source-generated context covering the tool signatures — and with the injected `OpcUaSessionManager` being described rather than treated as a service — that throws `NotSupportedException`. The second test asserts the throw. It documents the remaining gap and turns red the moment someone closes it, rather than leaving it to be found in a published binary. The package README says the same thing in prose. **Two pre-existing issues this surfaced, both outside this PR:** pulling `Opc.Ua.Mcp.Diagnostics` into an AOT publish reports `IL2046` / `IL3051` on `NicCaptureSource.StartAsync`, which is annotated `RequiresUnreferencedCode` / `RequiresDynamicCode` while the `ICaptureSource.StartAsync` it implements is not, and `IL2075` on `ServiceCallReassembler.GetCollectionCount` / `GetPropertyString`, which call `Type.GetProperty` on the result of `GetType()`. Happy to file these separately if useful. ## A generator that returned the value you opted out of *Merged from #4227. Fixes #4226.* `GetRandomNodeId(useBoundaryValues: false)` could still return `NodeId.Null` — one of the boundary values the same method offers when boundary values *are* requested. A caller that opted out should not receive one, and `GetRandomExpandedNodeId` builds on it and inherited the same behaviour. The `IdType.Opaque` branch is the practical path: `GetRandomByteString()` draws its length with `NextInt32(MaxStringLength)`, which is inclusive of zero, and a zero-length opaque identifier in namespace 0 satisfies `NodeId.IsNull`. With three namespaces and `MaxStringLength` at 10 that is roughly **one call in 130**. A numeric identifier of `0` in namespace 0 is null too, at around one in four billion. The draw is now repeated while the result is null and boundary values were not requested. Guarding the result rather than the individual branches covers every identifier type, including the numeric case, rather than only the one that happens to be reachable today. The `useBoundaryValues: true` path is untouched, so boundary values remain available when they are asked for. Four tests in `tests/Opc.Ua.Core.Tests/Types/DataGeneratorTests.cs` loop 20,000 draws, because at one call in 130 a single draw proves nothing. Against the **unmodified** generator they fail on draw 93 and draw 1346. Two of the four exist to keep the fix honest rather than to test the bug: `GetRandomNodeIdStillProducesEveryIdentifierType` catches a "fix" that simply stopped emitting opaque identifiers, which would satisfy the null assertions while silently narrowing the generator, and `GetRandomNodeIdWithBoundaryValuesStillYieldsThem` confirms the leak was closed rather than the feature removed. **How it was found:** an intermittent downstream test failure. A test server initialising a static `NodeId`-valued variable via `TestDataSystem.ReadValue` occasionally stored `NodeId.Null`; the encoder correctly writes that as JSON `null`, and a consumer asserting a value was present failed about one run in five — never reproducibly in isolation. It only surfaced under load because `UnsecureRandom.Shared` is a fixed-seed, process-global generator shared between `DataGenerator` and OPC UA transport jitter, so the number of draws preceding address-space initialisation depends on what else is running in the process. ## A NodeSet parent that lived outside the import batch *Merged from #4229.* `UANodeSetHelpers.LinkParentChildRelationships` builds its lookup table from **only the nodes in the current import batch**, links a child when the parent is found — and then cleared `instance.Handle` **unconditionally**, which is exactly where `Import` had stashed the authored `ParentNodeId`. A node whose parent lives in another NodeSet, or is owned by another NodeManager, therefore lost that parent **silently** at import, and nothing downstream could recover it. `RuntimeNodeSetNodeManager.CreateAddressSpaceAsync` already has a step that wires references to external node managers; the parent link never reached it. Such a parent is now recorded rather than discarded, and exposed through `UANodeSet.TryGetUnresolvedParentNodeId`. A parent that resolved normally inside the batch is **not** recorded, so a caller does not re-wire references that already exist. The record lives in a `ConditionalWeakTable`, so it does not keep an imported node alive and does not widen `NodeState` — whose `Handle` is a general-purpose slot a caller may legitimately use for its own purposes, and so is not a safe place to leave the value. `RuntimeNodeSetNodeManager` consumes it to report the one case that cannot be repaired automatically. In NodeSet2 the `ParentNodeId` attribute is a *hint*; the authoritative edge is the entry under `<References>`, which the reverse-reference pass already turns into an external reference. A node that names an out-of-manager parent but carries **no such Reference** materializes with no path from that parent, and no ReferenceType can be inferred to synthesise one — so it is reported as an authoring defect rather than guessed at or silently accepted. This is deliberately **not** a fix for a specific address-space symptom. It removes a silent loss of authored information and gives callers the means to act on it. Whether any particular orphaned-node report is caused by this path, or by namespace-index translation elsewhere, still needs separate diagnosis. **Changed while merging.** As authored in #4229 the accessor was `GetUnresolvedParentNodeId`, returning `NodeId?`. `NodeId` is a readonly struct implementing `INullable`, so that was a `System.Nullable<NodeId>` wrapped around a type that already carries its own null representation — which this repository excludes in favour of an `IsNull` check, and it asks for `TryGet` over casting on struct types. The tests correspondingly asserted `Is.Null` against a struct, which the testing rules also exclude. It is now `bool TryGetUnresolvedParentNodeId(NodeState, out NodeId)`, yielding `NodeId.Null` when there is no unresolved parent. The API is unshipped — it arrives with this branch — so nothing depended on the previous shape. Both tests were re-verified by mutation after the rewrite: disabling the record in `LinkParentChildRelationships` still fails `ImportKeepsAParentThatIsNotInTheSameBatch`. ## Dependencies - **OpenUSD to `0.7.0-alpha`.** All 11 issues we filed upstream are fixed, so the workarounds they justified are gone and the parts of the pump and generator samples that were blocked on them are implemented. - **ModelContextProtocol to `2.1.0`** across all five MCP tool libraries and the executable. No code changes were needed — the surface we use is unchanged in 2.x — but central package management required a cascade of pins, each commented where it sits. Both transports were verified against a running server rather than assumed. MCP 2.x is stateless (no `initialize`, no `Mcp-Session-Id`); an OPC UA session is application state addressed by a caller-supplied name, which fits that model but does assume process affinity, and that assumption is now written down rather than implied. ## Infrastructure The `code coverage` gate was measuring nothing on this branch and reporting success. It is fixed and now enforced, which immediately exposed real gaps — most usefully `RoboticsIntentJson`, which parses agent-supplied JSON (untrusted input from a language model) and had no tests at all. It is now at full line coverage in CI. Patch coverage is **90.20%** against a 75% floor. The `netstandard2.1` build leg was also failing. The MCP projects build as empty shells under a legacy `CustomTestTarget`, and the viewer sample referenced them on the target framework alone, so it tried to resolve a net10.0 shell from net8.0. The sample now derives MCP support from whether those projects are actually being built. ## From review A review pass over the branch found two real defects, both fixed. `RoboticsIntentJson` parses JSON written by a language model, so every malformed shape it can be handed is reachable input; it indexed pose arrays without checking their length and enumerated properties without checking their kind, so a short or wrongly-typed array escaped as `IndexOutOfRangeException` or `InvalidOperationException` rather than the argument error the tool descriptions promise the agent. Separately, `IntentOperationHandle` treated a zero `QueuePosition` as "not observed" and discarded it, so a handle kept reporting the last queued position after the server had said the operation left the queue. The pose validation immediately caught a latent test defect: the pallet scenario interpolated doubles into JSON with the current culture, so under a comma-decimal culture `[0.12, 0.16, 0.28]` became a six-element array. It passed only because that submission is expected to be refused for want of authority, so the wrong coordinates were never used. A third review finding — that a default-constructed `RobotIntentControllerState` hands out a null `NodeId` — was checked and rejected: `NodeId`, `QualifiedName` and `LocalizedText` are readonly structs here, so `default` is the null value and `IsNull` is `true`. The tests asserting that invariant are kept. A second review pass over the merged branch found two more, both fixed and both about an error surfacing as the wrong kind of failure rather than about a wrong result: - **Malformed agent JSON did not arrive as an argument error.** `RoboticsIntentJson` turns every *validation* failure into an `ArgumentException` — a wrong root kind, a missing property, a short pose array — but three `JsonDocument.Parse` calls and the NodeId parse were unguarded. A syntax error escaped as `JsonException` and an unparseable NodeId as `ServiceResultException`, which are the two shapes a language model is most likely to produce. That is the same defect class as the pose/kind checking above: a caller separating bad input from a genuine fault had to know to catch three unrelated exception types. Parsing now goes through one helper that reports a syntax error as an `ArgumentException` naming the argument, keeping the `JsonException` as the inner exception so the position is not lost, and the NodeId goes through `NodeId.TryParse` rather than `Parse`, collapsing its two failure modes into one. `MalformedIntentJsonIsRejected` had asserted `InstanceOf<JsonException>`, pinning the behaviour rather than the intended contract; it now asserts `ArgumentException` like every neighbouring test, and three tests cover the newly guarded paths. - **`ConditionalWeakTable.Add` throws on a duplicate key.** Not reachable today, but `TryGetUnresolvedParentNodeId` documents `Handle` as a general-purpose slot a caller may use for its own purposes — and a caller doing exactly that before re-importing into the same collection would take a hard throw from inside the import pipeline. Removing first makes the most recent import win. `AddOrUpdate` would say this more directly but does not exist on `netstandard2.0`. ## Deliberately not fixed here `FastenIntentDataType.Joint` is refused when the Server exposes no OPC 40450/40451 model, and a client cannot discover that before calling. That is **not fixable in this repository**: §5.4.2 makes `Joint` the defining mechanism while §11.3 says the parameters otherwise stand alone, and `IntentCapabilityDataType` declares capabilities at intent-type granularity with no member-level mechanism anywhere in the model. It is a specification gap, raised as S4 on [marcschier/opcua-drafts#60](https://github.com/marcschier/opcua-drafts/pull/60) rather than worked around with a bespoke mechanism the spec does not define. ## Validation `dotnet build UA.slnx -c Release` on the merged branch — **0 errors, 0 warnings**, with the AOT analysers now running over `Opc.Ua.Mcp.Core`. | Suite (net10.0) | Tests | |---|---| | `Opc.Ua.Core.Tests` | 4247 (86 skipped) | | `Opc.Ua.Robotics.Tests` | 540 | | `Opc.Ua.OpenUsd.Tests` | 909 | | `Opc.Ua.Tools.Tests` | 419 | | `Opc.Ua.Server.Tests` — `RuntimeNodeSet` | 94 | | `Opc.Ua.Robotics.Intent.Tests` | 22 | | `Opc.Ua.Aot.Tests` — `McpAotTests` | 3 | 0 failures. `Opc.Ua.Tools.Tests` rose from 409 to 419 with the serializer-equivalence tests from #4228 and the malformed-JSON tests from the second review pass, and `Opc.Ua.Core.Tests` covers the `DataGenerator` tests from #4227 and the import tests from #4229. `UANodeSetHelpers` also passes on **net48** (5/5). The MCP agent-usability grader scores Core 100 / Full 100. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Copilot-Session: a248a589-9a20-4372-868e-5d347e57001b Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8 Copilot-Session: a0718e8a-a158-45c5-929d-e1d6c0982431 |
||
|
Marc Schier
|
0612a43fe4 |
Pluggable cryptography and hardware-held private keys (#4192)
Makes the stack's cryptographic operations replaceable - by another library, an offboard service, or hardware (TPM 2.0 / HSM / PKCS#11 token / cloud KMS) - with no performance cost in the default configuration, and extends this to certificates so a private key need never exist in process memory. Also fixes three pre-existing defects found while validating: STACKGEN001 erroring for every compilation the stack generator was merely loaded into, certificates failing to load from paths past MAX_PATH on Windows, and the MonitoredNode2 tests starving the thread pool. Fixes #4190 |
||
|
Copilotandmarcschier
|
d8aca69444 |
Remove unused MinMetadataSamplingInterval configuration option (#4194)
## Description Server-side `MonitoredItem` sampling interval revision is based on the per-node `BaseVariableState.MinimumSamplingInterval` attribute, not on any global server setting — `ServerConfiguration.MinMetadataSamplingInterval` was dead configuration that was never read by any code path. Per maintainer feedback, this PR removes it instead of wiring it into revision logic that was never meant to consume it. Separately, the concern that `Subscription.MaxNotificationsPerPublish` isn't revised by the server was verified against existing coverage: `CreateSubscriptionWithNotificationLimitsUsesEffectiveLimitAsync` / `ModifySubscriptionWithNotificationLimitsUsesEffectiveLimitAsync` in `SubscriptionTests.cs` already assert the server clamps a client-requested value down to the configured server limit (e.g. requested `25` vs. server limit `10` → revised `10`). ### Removed `MinMetadataSamplingInterval` - Dropped the `ServerConfiguration.MinMetadataSamplingInterval` property (`ApplicationConfiguration.cs`) and its XSD element - Dropped `SetMinMetadataSamplingInterval` from `IApplicationConfigurationBuilder` / `ApplicationConfigurationBuilder` - Removed the element from sample and test server configuration XML files (`Ctt.ReferenceServer.Config.xml`, `Quickstarts.ReferenceServer.Config.xml`, GDS test configs, `test-server-full.xml`) - Updated `ApplicationConfigurationBuilderTests` and `ConfigurationRoundTripTests` to drop assertions on the removed property ## Related Issues ## 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. - [ ] 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. <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #4191 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: marcschier <11168470+marcschier@users.noreply.github.com> |
||
|
Marc SchierandCopilot
|
2b44fcad22 |
Robot intent: a task-level command API with a Part 10 lifecycle (#4165)
# Description Implement the draft **OPC UA — Robot Intent** companion model end to end: task-level motion verbs, a Part 10 lifecycle, a server hosting surface, a client, two samples and a live integration suite. It replaces the opt-in convention API merged in #4127 with an implementation generated from a companion information model. ## Why OPC 40010-1 *Robotics* describes robot **topology** in detail and defines **no motion verbs at all** — its entire actuation surface is `Start`, `Stop` and loading a named program. A conformant client can discover everything about a robot's construction and cannot ask it to move anywhere. #4127 filled that gap with ten verbs in an application-owned namespace, resolved by BrowseName, and the file said what it was: *"opt-in, explicitly non-normative industrial operation conventions"*. That contribution established a **vocabulary**. It established no **lifecycle**, and the lifecycle is the harder half. A motion takes seconds; a pick takes a minute. OPC 10000-4 §5.12.2 discards a method result when the Session ends *"independent of the task actually performed at the Server"* — so a synchronous motion method does not merely time out, it **loses the outcome of work that has already physically happened**. OPC 10000-10 §4.1 gives the OPC Foundation's own resolution: a Method performs a calculation, a **Program** runs a batch process or a machine tool part program. So `SubmitIntent` returns as soon as the intent is *admitted*, and what it returns is a NodeId — an `IntentOperationState` (a Part 10 program instance) the client subscribes to for progress and reads for the result. ## What changed **Model and shared code** — `Opc.Ua.RobotIntent.NodeSet2.xml` is source-generated in `Opc.Ua.Robotics`, so the enums, the polymorphic intent structures, `IntentOperationState : ProgramStateMachineState`, the method states and the typed clients derive from the model rather than being hand-written. Its only `RequiredModel` is the base UA namespace, so a server can adopt it without OPC 40010 or DI. `PoseMath` implements the specification's Annex C conversion between its unit quaternion and the core `ThreeDFrame` — including the `asin` clamp that stops a pole orientation becoming a domain error, and the non-negative-`w` representative that makes two servers agree on four numbers. `FrameTree` composes transforms along the frame tree. **Verbs are a DataType hierarchy, not one Method each.** A single submission and a mission step are then the same shape, and a new intent is a subtype rather than a new method — which is what `AddOperation<TRequest,TResponse>` was working around. **Server** — `IntentControllerHost` owns admission in the specification's order, the queue with PLCopen buffer modes, blending, cancellation *with the server's right to refuse*, missions with a committed base and a revisable horizon plus an IEC 61131-3 step graph, safety-aware refusals, real-time channel brokerage, and the capability declaration. All thirteen Methods are wired; command authority and channel leases are released when the holding Session closes; every client-supplied NodeId is validated against the controller being commanded (clause 11.3). `RobotIntentNodeManager`, the fluent `IIntentControllerBuilder` and `AddRobotIntent`/`ConfigureRobotIntent` make it reachable, and `RobotIntentFacetCalculator` computes which of the specification's facets an instance actually satisfies. **Client** — `RobotIntentClient` discovers `Server/RobotIntent/Controllers` and reads what a robot accepts; `IntentOperationHandle` subscribes and completes on a terminal state, recovering the result after a reconnect because clause 6.7 promises it survives; `CommandAuthorityLease` and `RealTimeChannelLease` manage the two exclusive resources; fluent builders cover every intent and mission. **Samples** — all robotics samples move under `samples/Robotics/`. `IntentEnabledRobot` is one UR5e-style offset-wrist arm on a bench, declared completely enough to show what a conformant server publishes, with a simulated safety source so the clause 10.4 refusals are demonstrable. `IntentViewerClient` turns a **click on a target prim in the OpenUSD viewport** into an intent and watches the arm execute it; it also runs headless, which is how CI exercises it. **Intents execute serially**, which satisfies every `BlockingMode` constraint by construction: the specification forbids beginning a `Single` or `Hard` intent while another executes and merely *permits* `None` and `Soft` to overlap. The convention API (`RoboticsOperationConventions`, `RoboticsOperationsBuilders`, `RoboticsOperationsClient`) is removed. ### On safety — what this deliberately does *not* claim The interface is **non-safety-rated**, and that is a property of the technology rather than a scoping choice. OPC 10000-15 carries cyclic safety data from a SafetyProvider to a SafetyConsumer; the consumer's `RequestSPDU` holds an identifier, a monitoring number and one octet of explicitly **non-safety** flags, so a caller has no channel through which to supply safety-rated arguments. Every safety fieldbus (PROFIsafe, CIP Safety, FSoE, openSAFETY) expresses a safety command as a **continuously asserted cyclic signal**, because the integrity argument rests on the fail-safe state that follows when assertion *stops* — and a Method call has no defined behaviour when it stops being called. What the host does instead is **observe and refuse**: `UpdateSafetyState` is how the application reports what the safety system is enforcing, and admission then refuses on the same values a client can read, so a refusal is explainable from the address space rather than from Server-internal state. It may never *instruct* the safety system, and no Method here commands a safe motion function, changes an operational mode or clears a stop. ## Defects this found in the specification, fixed upstream Implementing a specification is the only way to find out whether it can be implemented. Seven defects went back to [marcschier/opcua-drafts#47](https://github.com/marcschier/opcua-drafts/pull/47) rather than being worked around here: - **Refusal was unobservable.** Clause 6.2 orders six distinct refusals and clause 5.8 calls the failure set "small and diagnosable on purpose" — but `SubmitIntent` returned only `IntentId` and `Operation`, so a client could see *that* something was refused and never *which*. It now returns `Accepted`, `Failure` and `Message`, and clause 6.2 states that a refusal returns `Good` rather than substituting a Bad `StatusCode`. - **The Part 10 promotion was illegal.** `IntentOperationType.ProgramDiagnostic` was declared as a Property of `PropertyType`; OPC 10000-10 declares it a Variable of `ProgramDiagnostic2Type` reached by `HasComponent`. The declaration added a *second* member beside the inherited one instead of promoting it — which is precisely why an earlier version of this branch had deleted it. Correcting the TypeDefinition made it generate cleanly with **no change to the curated Core**. - **Clause 9 said nothing about the Method surface.** A server could declare `MissionsSupported` true and omit `SubmitMission` entirely, because the mission and channel Methods are Optional. This implementation did exactly that until an integration test caught it. A fourth honesty rule and a table now fix which Methods each declaration implies. - **Conformance was not machine-readable, and two implementations disagreed.** Clause 12 defined conformance in terms of facets and gave a Server nowhere to publish which ones it satisfied, so every client had to re-derive Table 12.2 from the address space. This repository proved why that fails: the server''s `RobotIntentFacetCalculator` and the client''s `RobotIntentRules.DeriveFacets` each implemented clause 12.2 independently and **disagreed on essentially every row** — the client''s `Blending` was `BlendingSupported` alone where the server also requires all four blending buffer modes, its `QueuedIntents` was `MaxQueueDepth > 0` where the server also requires `Buffered`. A client and server from the same release answered "does this controller conform?" differently, and both were reading the specification correctly, because several rows are behavioural and no amount of browsing settles them. `IntentCapabilitiesType` now carries **`SupportedFacets`**, exactly as OPC 10000-5 does with `ServerCapabilitiesType.ServerProfileArray`, RI-Base requires it, and clause 12.2 separates structural requirements a Server shall meet before listing a facet from attested ones governed by clause 9. - `WaitIntentDataType.Signal` was unbounded where clause 11.3 has to check it; §11.3 gained a table fixing the expected type of every NodeId-valued member; §5.7.0 gave `Ready`/`ActiveIntent`/`ActiveMission` normative meaning; §6.9 bounded `RequestedLease`; §6.5 and §6.4 now say what a server that cannot differentiate `StopMode` must do, and which stop a superseded intent gets. ## Bugs the tests and reviews caught Each of these would have been silent in the field: - **The execution engine was never wired into the public API.** `IntentControllerBuilder.RegisterAsync` built the address space and never started an `IntentControllerHost`, so through the documented `AddRobotIntent` → `ConfigureRobotIntent` path **every Method returned `BadNotImplemented`** and the registered `IIntentExecutor` was never resolved. A browsable address space is not a working robot interface, and only a live integration test showed the difference. - **NodeId scoping failed open on an empty index.** A controller advertising `CallProgramIntentDataType` with no published `ProgramType` accepted an *arbitrary* NodeId as `CallProgram.Program` — and `CallProgram` runs code the server holds. - **Blending completed the predecessor at submission time** and abandoned its motion, telling the client it succeeded and publishing an all-zero pose at the origin as a genuine `AchievedPose`. - **Mission base immutability compared only identifiers**, so a client could keep the ids and swap the *motion* of an already-committed step. - **`FinalResultData/Result` was never registered** with the node manager: Browse showed it, Read returned `BadNodeIdUnknown` — so clause 6.7's promise to a Part 10 client did not hold. - `DisposeAsync` disposed primitives out from under in-flight executor code; re-using a terminated `IntentId` destroyed the retained result and duplicated a NodeId; neither side registered the Robot Intent encodeables, so `Pose3DDataType` did not decode; host startup was hooked to a task that ran in only one hosting shape. - The result was published **after** the state went terminal; capabilities were resolved against a stale namespace table; an empty `ContentFilter` arrives as an empty element array rather than a null filter, so testing only for null made every unconditional mission transition silently untaken — a mission would run its first step and stop. ### Found by a later test-quality audit, after the above was written An audit of the test suite itself found five tests whose **names** asserted a normative requirement while their **code** asserted something weaker or nothing at all. The worst **fabricated the answer it checked**: a helper caught a decode failure and invented "5 supported intents, 6 axes, all facets true", so six capability-honesty assertions across four tests could pass while reading nothing from the server. The fallback is gone and the suite passes with honest reads. Chasing the facet gaps it reported led to the clause 12 defect above. Two further defects followed from completing that work: - **`RI-Interop-40010` was never computed**, and the code documented the omission as impossible to fix "because the calculator receives only a Robot Intent controller, not the linked OPC 40010 model". That was wrong: Annex B's structural requirement is an **inverse** `HasIntentController` reference, reachable from `IntentControllerState` — and the integration suite was already browsing exactly that. A server implementing Annex B could not claim Annex B. - **The published claim could go stale.** `SupportedFacets` was a snapshot taken during `RegisterAsync`, so a reference attached afterwards left it wrong. It is now bound through `BindRead` and recomputed on read, so the claim tracks the address space by construction rather than by discipline — which matters for anyone bolting Robot Intent onto an existing OPC 40010 node manager, where the attach ordering is often not theirs to choose. Two tests that **could not fail** were also replaced: one asserted `AccessLevel` equals `CurrentRead` to pin a read-only marking, but `BaseVariableState` initialises every variable to exactly that, so deleting the marking left it green; it is now a write expecting `BadNotWritable`, including a real OPC UA `Write` service call over a live session. Finally, a build break that a plain build hides: `RoboticsAotTests` used `out string?` in a project declaring `<Nullable>disable</Nullable>`. Building the project **without** `-f` suppresses `CS8632` through `NoWarn=nullable` and succeeds; building it **with** `-f net10.0` resolves `NoWarn` differently, `CS8632` fires, and `TreatWarningsAsErrors` makes it an error. CI builds per-TFM, so this would have failed there while passing every local build to that point. ## Testing and coverage | | | |---|---| | `dotnet build UA.slnx -t:Rebuild` | **0 errors, 2 warnings** — both pre-existing `CA1861` in `tests/Opc.Ua.Tools.Tests`, a file this PR does not touch | | **Whole `UA.slnx` test suite, net10.0** | **50 of 52 assemblies fully green** (measured at `40d9cf213`, before the `master` merge; the other two are explained below) | | `Opc.Ua.Robotics.Tests` | **467** net10.0, **447** net48 | | `Opc.Ua.Robotics.Intent.Tests` (live client <-> server) | **20** on net10.0 and net48, run repeatedly | | `Opc.Ua.OpenUsd.Tests` | **904** after merging `master` (664 before; the rest arrive with #4160 / #4162 / #4180) | | `Opc.Ua.Aot.Tests` | **121/121** | | `Opc.Ua.Di.Tests` (regression from the sample move) | **364/365**, one pre-existing skip | | **Changed-line coverage** | **90.7 %**, no file below 80 % — comfortably above the enforced band, which after #4158 applies only to patches larger than 100 changed lines | The two non-green assemblies are **not** caused by this branch. Rather than assert that, I ran the same selections at the merge-base `ae241b662` on the same machine: - `ServiceCallReassemblerTests.PrivateRequestSummaryFormatsKnownRequestKinds` expects `12.5` and gets `12,5` — a culture-sensitive assertion failing on a European locale. Fails 4/4 on **both** commits. - `CertRotationLiveTests.L2Cert2RotateCertificateDuringServerRestartRecoversSharedChannelAsync` fails on a teardown count. Fails 4/4 on **both** commits. - A third failure seen once, AOT `FindServersAsync` with `BadSecureChannelClosed`, did not reproduce: 4/4 on retest and 121/121 in the final run. Environmental. Two things are worth calling out because they would otherwise be invisible: - `tools/**` is **not** in the `coverage-thresholds.json` ignore list, and no test project referenced the OpenUSD tools assemblies — so their changed lines counted as uncovered and the real patch figure was 75 %, exactly the blocking threshold. Rather than widening the ignore list, the pure decisions in the viewport host (pointer-to-physical-pixel conversion, `RenderPickRequest` construction, stale-retry, backend discovery, pick-mode selection) were separated from the Avalonia/native shell and tested headless against a fake picking backend. - `dotnet build UA.slnx` **hides warnings on unchanged projects**; only `-t:Rebuild` surfaces them. That is how a `CA2263` introduced by a net48 portability fix survived several "zero warning" builds. The multi-target requirement earned its keep repeatedly: it caught a default interface implementation, `ArgumentNullException.ThrowIfNull`, `string.Create` with an interpolated handler, `ValueTask.FromResult`/`CompletedTask`, `Task.WaitAsync`, generic `Enum.GetValues` and generic `Enum.IsDefined`, and a non-generic `TaskCompletionSource` — none of which the net10.0 build complained about. A net10.0-**only** hang in the simulated executor and two load-sensitive flaky tests were also found and fixed at the cause. ### Merged with `master` `master` (through `e73e71184`) is merged in as `08205d81d`. Four conflicts, all where the two branches touched the same robot and OpenUSD surfaces, and none resolved by dropping a side: - #4162 added six files to `samples/MinimalRobotServer/` while this branch moved that directory to `samples/Robotics/MinimalRobotServer/`. They are kept at the moved path, and `UA.slnx` takes the union of both sides' new projects. - #4160 replaced `object` with a `UsdValue` union and rewrote `Flatten`; this branch had reformatted the same method and fixed a path in its doc comment. Git produced a hybrid that did not compile. The resolution keeps `master`'s `UsdValue` arm and `Flatten(UsdValue)` overload in this branch's block-bodied form, with the path fix retained. - `master` inserted `FindStageCamera` directly before two methods this branch had relocated, so the merge duplicated `WriteStageUsda` and `GetPrivateStateRoot`. Only `FindStageCamera` is genuinely new — `master`'s diff shows the other two as unmodified context — so the duplicates are dropped and this branch's `internal` versions kept, including the `GetPrivateStateRoot(string?)` overload the connector tests use to stay out of the real user profile. - `master` moved the OpenUSD stack to `0.4.0-alpha`; this branch pins `OpenUsd.Rendering` in the test project so the renderer-pick tests can reach the picking backend. Left at `0.1.0-alpha` that pin is below what the viewer now resolves transitively, which is `CS1705`. The pin follows `master`. CI was green on `baaf35e9` with **190 checks passing and none failing**, across every build leg (net472 / net48 / netstandard2.0 / netstandard2.1 / net8.0 / net9.0 / net10.0, Debug and Release) and the full test matrix on Windows and Linux. Since then `master` has been merged twice more and two CI-stability fixes landed (see below), so the current head is re-running. Locally, `dotnet build UA.slnx -t:Rebuild` gives **0 errors** and the 2 pre-existing `CA1861` warnings; Robotics **467** net10.0 / **447** net48, Robot Intent **20** on both, OpenUSD **904**, AOT **121/121**, source generation **3771**. ### Review round Five reviewers over disjoint areas found defects worth calling out, all fixed in `3ffec2c1`: - **A refused mission step was reported `Succeeded`.** The advance path returned the same `false` for "steps exhausted" and "step refused", so a protective stop between steps told a supervisory system the mission had completed while the robot stood halted with work unexecuted. - **Three attacker-controlled NodeIds reached the executor unvalidated** — `Pick.Tool`, `Place.Tool`, `ToolChange.DockStation`. `DockStation` drives a physical tool-exchange motion to a location the server never authorised. The replacement test enumerates NodeId members by reflection so the next one added cannot be missed. - **A lock-order inversion that loses a stop.** `CanCancel` and the mission condition evaluator ran under the host lock while the executor's own thread re-entered it via `ReportBlendBegin`. - **A non-terminal executor outcome killed the pump** from outside the `try/catch` that exists because the executor is untrusted — `Ready` stayed true and submissions kept being accepted while nothing moved. - **`Pause` published `Suspended` while the robot kept moving.** Now queue-only and honest about it; the specification gained the matching rule ([opcua-drafts `357a558`](https://github.com/marcschier/opcua-drafts/pull/47)). - **`RI-RealTimeChannel` and `RI-Safety` were false claims** — granted on an empty folder with no methods, and on the mere presence of a Mandatory node. - **The Part 10 promotions never promoted.** Namespace-1 BrowseNames declared second members beside the inherited ones; the conformance test could not see it because its lookup stripped the prefix. Fixed upstream and regenerated. - **Renderer picking never worked.** The reflective probe bound a renderer that is not a picking backend and throws off its owner thread. It now binds only `IRenderPickingBackend`, falls back immediately and loudly, and the docs say plainly that renderer picking is unavailable with this package version. - **The client lost command authority the instant it gained it** — monitored items report the current value first, and the lease treated every notification as a loss. ### CI stability fixes for master-side failures The CI split in #4158 turned on legs that had never run, and two of the failures they exposed were real bugs on `master` rather than anything this branch introduced. Both are fixed here because they were blocking every branch: - **ECC key generation was broken on macOS.** `ExportExplicitParameters` throws `PlatformNotSupportedException` there — Apple's SecurityTransforms supports named curves only — and it was called for exactly one value, the curve order `n`. The four curves OPC 10000-12 admits have published orders, so those are carried directly, with `CurveOrderTableMatchesThePlatform` checking each against the platform's own exported order wherever export works, so the constants cannot silently drift. This accounts for the two ECC failures in `test-macOS-latest-Server`. - **The certificate-leak detector raced asynchronous disposal.** It polled a fixed budget and reported whatever it saw, which cannot distinguish a genuine leak (count steady) from a suite still draining fire-and-forget disposals (count falling) — on a loaded agent that produced `Certificate leak detected: 2 instance(s)` with every test passing. It now waits for the count to **settle**, so a real leak is reported sooner than before while a slow drain gets as long as it keeps progressing. Shared by every suite that asserts no certificate leaks. The remaining macOS failures on `master` (`Gds`, `Tools`, `Redundancy.Samples`, `aot-macos`) have different root causes, are not reproducible without a macOS host, and are **not** addressed here. ### Merged with `master` again `master` through `416d619c4` (#4158, the CI matrix split and graduated coverage bands) is merged in as `08ae4528`. One conflict, in `docs/DeveloperGuide.md`, where both sides had deliberately rewritten the coverage-gate section: - `master`'s rewrite is kept in full — graduated `patch.bands`, Codecov being informational-only, and where the numbers surface per CI system. - This branch's paragraph about the `ignore` globs is kept too, re-sited ahead of those subsections. I re-checked it against the merged `coverage-thresholds.json` rather than assuming it still held: the list is still `tests/**`, `samples/**`, `obj`, `bin`, `*.g.cs`, with **`tools/**` absent** — so the warning that anything under `tools/` is measured like product code, and that an assembly no test project references contributes changed lines counted as *uncovered*, is still accurate and worth keeping. - `master`'s "reproduce a coverage failure locally" line supersedes this branch's near-identical one, so only `master`'s survives. Post-merge: `dotnet build UA.slnx -t:Rebuild` **0 errors**, 2 pre-existing `CA1861`; Robotics **467** net10.0 / **447** net48; Robot Intent **20** on both. ### Getting CI green The pipeline had never actually run on this branch until now (it was waiting on the fork-PR approval gate), so the first run surfaced four real problems, each fixed at source: - **Seven build legs failed** because `IntentEnabledRobot` pins `net10.0` for the OpenUSD payload but never opted into `RestrictForLegacyTfm`. On the legacy legs the libraries build as net472/net48, where `Opc.Ua.Types` exposes the `System.Threading.Lock` polyfill publicly, and that collides with the sample's own net10.0 BCL (`CS0433`). `Opc.Ua.OpenUsd.Connector.Viewer` was already using the opt-out for the same reason. - **A Linux-only test failure** turned out to be a real defect, not a platform quirk: `SetPose` published the *commanded* interpolated pose beside joint frames derived from the *solved* joints, so the two disagreed by the inverse-kinematics residual — about 8 mm, constant after any real motion. That is the lying-digital-twin defect one level deeper than the per-step IK fix reached. `SetPose` now derives both from the joint angles. - **Three source-generator tests** asserted the old emitted shape. The clause 6.2 Null-Variant fix was implemented in `ObjectTypeProxyGenerator` so every generated proxy benefits, which changed that shape; the tests are updated and strengthened to pin the `IsNull` branch as well. - **A CI-only timeout** in the simulated arm drain: the executor runs on a manual clock so the tests do not depend on machine speed, but the drain guarded itself with a wall-clock deadline, which put that dependence back. CI runs with coverage instrumentation, which is enough to take a per-step IK probe from ~0.6 s to over 5 s. The guard is now a simulated step budget. One failure was **not** from this PR and did not recur: a Roslyn `AccessViolationException` compiling `Opc.Ua.ISA95` on one leg, while the Debug leg of the same target passed on identical code and the Release build succeeds locally. ## Related Issues - Relates to #3827 — the request that prompted the robotics intent work. - Builds on #4127. The companion specification is drafted in the open at [marcschier/opcua-drafts#47](https://github.com/marcschier/opcua-drafts/pull/47) (`metaverse-specs/robot-intent/`), with the prior art and the reasoning behind each decision in the research document beside it. **Nothing in it is normative or endorsed by the OPC Foundation, and its NodeIds and namespace URI are provisional** — which is the main reason this is a draft. ## 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. - [ ] I have addressed **all** PR feedback received. On the test checkbox, precisely: the whole `UA.slnx` suite now runs on net10.0 with **50 of 52 assemblies fully green**, and `Opc.Ua.Robotics.Tests` passes on **all five** target frameworks including net472, net8.0 and net9.0 which had never previously been exercised. The two remaining failures are reproduced identically at the merge-base and are detailed above, so I have ticked the box; if you would rather it stayed unticked until those two are green, say so and I will revert it. ## Points I would most like feedback on 1. **The namespace and NodeIds are provisional.** They come from a draft specification, not from the OPC Foundation. If this lands before the model is registered, the identifiers will move. 2. **The model lives in `Opc.Ua.Robotics`.** It is standalone on the base UA namespace and takes no dependency on OPC 40010, so a separate `Opc.Ua.RobotIntent` package would arguably be cleaner. Keeping it here was a deliberate call — this is where the robot-facing API already is — but it is the easiest thing to change now and the hardest later. 3. **Removing the convention API from #4127 is a breaking change** for anyone who adopted it. It was explicitly marked non-normative, so I have assumed that is acceptable — please say if a deprecation period is wanted instead. 4. **Serial execution** is a deliberate simplification. Concurrent `None`/`Soft` intents would be an optimisation, not a correction, but it is worth agreeing that reading. 5. **`samples/Robotics/` reorganisation** moves `MinimalRobotServer`. Happy to split that into its own commit if it makes review easier. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1dab1302-19c5-4a9b-a50c-d97d389713aa Copilot-Session: a248a589-9a20-4372-868e-5d347e57001b |
||
|
Marc SchierandCopilot
|
ae241b6625 |
Robotics: the OPC 40010 companion SDK, its client and the robot sample (#4127)
**Stack 5/5** — base #4126. Part of the #4096 split; see #4096 for the overall picture. Closes the API work for #3827. > Review after #4123, #4124, #4125 and #4126. This branch is content-identical to the original #4096 branch, so merging the stack lands exactly what #4096 contained. ## The problem this solves OPC 40010 1.02 defines a rich robot **topology** but **no motion verbs at all** — its actuation surface is the SystemOperation and TaskControl state machines plus a Controller `Programs` directory. Meanwhile [URML](https://github.com/URML-MARS/URML) names OPC UA Robotics as its canonical non-ROS substrate and asks for `MoveTo` / `Grasp` / `Release` methods the companion spec does not define, so today it needs a hand-maintained per-deployment NodeId mapping file. This closes that gap in two clearly separated tiers, and never presents the second as standard. ## Contents **Server** — stock `RoboticsNodeManager`, ordered `IRoboticsModelProvider` composition, `IRoboticsConfigurator` code-behind, and validated fluent topology builders for systems, controllers, motion devices, axes, power trains, motors, gears, drives, safety states, software, task controls and task modules, with the correct OPC 40010 semantic references. **Standard operations** — `AddSystemOperation` and `AddTaskControlOperation` drive the Part 16 state machines. An illegal cause is rejected with `BadInvalidState` before the handler runs; a handler returning a bad `ServiceResult` does not move the machine; and a transition already in flight rejects a concurrent one rather than letting both commit. **Programs** — binds the optional `FileDirectoryType` through the reusable `IFileDirectoryBinder` added in 1/5. **Operation conventions** — opt-in and explicitly non-normative `MoveTo` / `MoveJ` / `MoveL` / `Grasp` / `Release` / `PickFrom` / `PlaceAt` / `SwapTool` / `SetOutput` / `CallProgram`, created **only** in an application-owned namespace — the OPC UA, DI, IA and Robotics namespaces are rejected — plus a generic `AddOperation<TRequest, TResponse>` extension point. **Client** — `RoboticsClient` extends the DI client, is subtype aware throughout, and provides typed topology snapshots, the standard operation and task-control clients, `ProgramsAsync`, streaming observation and a DI factory. Verbs resolve by BrowseName, so no NodeId mapping file is needed. **Sample** — `MinimalRobotServer` composes five models (Robotics, IA, DI, OpenUSD, RSL/GPOS) purely through `IRoboticsModelProvider` contributions, which is the real proof that the stock hosting is sufficient. It follows the published KUKA KR 16-2 kinematics and axis limits with an eased pick-and-place cycle. Also restores the vendor-subtype scanner test held back from 2/5, which uses the Robotics NodeSet as its fixture. ## Defects found and fixed along the way - **`ControllerType.CurrentUser`** is Mandatory in OPC 40010 but was neither built nor validated. Now has a builder and a validation error. - **`MotionDeviceType.TaskControlReference`** was absent because it must target a `TaskControlOperationType` instance that did not exist. Now wired. ## Validation `dotnet build UA.slnx` — 0 errors, 0 warnings. | Suite | Result | | --- | --- | | Opc.Ua.Robotics.Tests | 103 / 0 | | Opc.Ua.OpenUsd.Tests | 635 / 0 | | Opc.Ua.Di.Tests | 342 / 0 | | Opc.Ua.SourceGeneration.Tests | 94 / 0 | | Opc.Ua.Aot.Tests (NativeAOT) | 120 / 0 | --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8cbb8cd0-f0cb-4ab0-bea2-6202fbf69485 |
||
|
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 |
||
|
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 |
||
|
Copilot
|
a03bca97cb |
Remove TraceConfiguration and related APIs (#4092)
# Description
This change removes the remaining `TraceConfiguration`-driven
apply/setter API path and aligns configuration startup with the 2.0
telemetry model (`ITelemetryContext`/`ILoggerFactory`). It also updates
migration docs to make the removal explicit and actionable.
- **Core API removals**
- Deleted `TraceConfiguration.ApplySettings()` implementation file.
- Removed runtime apply calls from configuration/server startup paths.
- Removed fluent builder trace setters:
- `SetOutputFilePath(...)`
- `SetDeleteOnLoad(...)`
- `SetTraceMasks(...)`
- Simplified builder interface inheritance accordingly.
- **Call-site and test fixture updates**
- Removed usage of deleted trace fluent APIs in affected test
infrastructure and fixtures.
- Updated GDS log readers to tolerate missing `TraceConfiguration`
output path safely.
- **Migration guidance**
- Updated migration docs to explicitly state removal of
`TraceConfiguration` apply APIs and direct users to telemetry-based
logging configuration.
```csharp
// Removed API pattern
app.Build(uri, product)
.AsClient()
.SetOutputFilePath("trace.log")
.SetTraceMasks(Utils.TraceMasks.Error);
// New direction
// Configure logging via ITelemetryContext / ILoggerFactory at composition/startup.
```
## Related Issues
- OPCFoundation/UA-.NETStandard#4091
## 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.
<!-- START COPILOT CODING AGENT SUFFIX -->
- Fixes #4091
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
|
||
|
Marc Schier
|
2a4bc0fe24 |
Make reverse connect lifecycle fully asynchronous (#4059)
## Summary - add cancellation-aware asynchronous start, lazy initialization, stop, reload, and disposal for `ReverseConnectManager` - make listener replacement transactional with explicit ownership, rollback/restoration, stale watcher protection, and deterministic concurrent start/stop/dispose behavior - add injectable async configuration providers, remove the former protected configuration hooks, and retain obsolete synchronous lifecycle compatibility wrappers - integrate eager Generic Host startup plus non-host lazy startup, including custom application-configuration providers - serialize `ReverseConnectHost` lifecycle and extend raw TCP, Kestrel TCP, WSS, hosting, concurrency, rollback, and NativeAOT coverage - document the async lifecycle, hosting behavior, cancellation guarantees, provider model, and migration path ## Validation - `Opc.Ua.Client` Release build across all supported target frameworks - reverse-connect client and core tests on net10.0 and net48 - raw TCP, Kestrel TCP, and WSS integration tests on net10.0; raw TCP on net48 - NativeAOT publish and 115-test executable run Closes #4013. Related: #3985, #4009. |
||
|
Marc Schier
|
3490d9cd05 |
Use Dekaf by default for Kafka on all target frameworks (#4038)
# Description Upgrade the Kafka transport from Dekaf 1.2.0 to 1.7.0 and use the pure-managed Dekaf adapter by default on every supported target framework. - Enable the Dekaf adapter and its guard tests on `net472`, `net48`, `netstandard2.1`, `net8.0`, `net9.0`, and `net10.0`. - Retain the `Confluent.Kafka` adapter and factory as an opt-in DI backend through `WithConfluentKafkaClient()`. - Update the central dependency pins required by Dekaf 1.7.0. - Validate NativeAOT/trimming compatibility on `net10.0` for the default Dekaf backend; the opt-in Confluent backend remains JIT-only. - Avoid per-header allocation on .NET Framework when Dekaf header memory is array-backed. Validation: - Release builds passed with zero warnings for all six library target frameworks. - Kafka adapter and DI selection tests passed on `net48` and `net10.0`. - `dotnet publish tests\Opc.Ua.Aot.Tests\Opc.Ua.Aot.Tests.csproj -c Release -r win-x64 --self-contained true` - The generated NuGet package includes Dekaf 1.7.0 and Confluent.Kafka 2.15.0 for every target framework. ## 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. - [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. |
||
|
Marc Schier
|
8538f531e7 | Update paths for the consolidated repository layout | ||
|
Marc Schier
|
7cec2d614d | Move repository files to the consolidated layout |