Repository navigation
Harden recovery, concurrency, and resource lifetimes across the OPC UA stack - #4516
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208
Complete C3-1 effective-chain notification handling, C3-4 custom-core flag propagation, and C4-2 private key file protection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208
Close admission before teardown and retain owned resources until admitted service, lifecycle, and async method operations return. Dispose outside the node and admission locks, and forward shared asynchronous completion through the adapter. Add deterministic regressions for late admission, active callers, sampling-worker joins, reentrant shutdown, cancellation, failure, and repeated disposal. The selected server suites pass 1037 cases on both net10.0 and net48, with 64 existing assumption-based not-executed cases per target.
romanett
left a comment
There was a problem hiding this comment.
Review at head c5186d97, covering the source diff plus a re-check of every finding that the bug audit still listed as open or partial after #4503 (59 findings: 43 open and 16 partial).
Audit status at this head
| Result | Count | Findings |
|---|---|---|
| fixed, with a regression test | 52 | C2-4, C3-1, C3-3, C3-5, C3-6, C4-6, C4-7, C5-1, C5-13, C7-2, C7-4, L1-3, L1-5, L1-6, L1-7, L1-10, L1-11, L2-9, L2-11, L2-12, L2-14, L3-2, L3-3, L5-5, L5-15, L6-6, L6-7, L6-8, L6-9, S1-6, S1-7, S1-8, S1-9, S2-5, S2-8, S2-9, S2-11, S3-3, S3-7, S3-8, S5-1, S5-5, S5-8, S6-3, S6-5, S6-6, S7-4, S7-5, S8-4, S8-10, S10-2, S12-3 |
| fixed, but the fix introduces a new issue | 5 | L2-7 (keep-alive not restarted), C3-8 (staging order), C6-5 (Rsa-typed certificate), C7-5 (PSS certificates rejected), S4-8 (1024-entry Browse failure). See inline comments. |
| partial | 2 | C3-2 (below), S5-6 (inline at AsyncCustomNodeManager.cs:985) |
Good work overall: every remaining Tier 2 item is closed or nearly closed, and nearly every fix has a focused test.
C3-2 (partial, file not in the diff): X509CertificateStore.Open now strips the X509Store: prefix, and trust lists work because the manager re-resolves their store type. Application certificate stores still fail. CertificateStoreIdentifier.DetermineStoreType (CertificateStoreIdentifier.cs:149-181) does not recognise the prefix, so an app-cert StorePath of X509Store:CurrentUser\My is typed as a Directory store and opened as a literal folder. X509StoreAcceptsPrefixedAndLegacyPaths covers only Open.
Smaller gaps, no inline comment:
- C4-6: no test covers the
Session.cschange that maps every token-decrypt failure toBad_IdentityTokenInvalid. - C3-1:
CertificateChangeEventstill does not document that its certificate fields are valid only duringOnNext. The publicApplicationCertificateChangedevent forwards the same object, so a handler that keeps it can read disposed handles. - S2-8: items restored from a durable store written by an older build keep their creation-time interval clamp.
- C2-4: a fresh reverse connection handed to an entry that is closing, or superseded, or never consumed before the cycle ends, is never closed. Each case leaves one accepted socket open.
Must fix before merge
WebApiWssTransportChannel.cs:826: one response that fails to decode tears down the whole WSS connection and every pending request (regression from master).Session.cs:3429: network-path failover that reuses the session leaves it without a keep-alive, so a silent loss of the alternate path is never detected.DirectoryBrowser.cs:167: directories with more than 1024 entries can no longer be browsed. The limit is hard-coded, and clients can push a directory over it withCreateFile.TrustListTransaction.cs:200: "add X, then remove X" in one transaction now commits X as trusted.CustomNodeManager.cs:209:DisposeAsynchangs server shutdown if a legacy subclass'sDispose(bool)does not call base.
The other inline comments are minor.
CI
The latest head is red on test-macos-net10.0 (25/30): Opc.Ua.Subscriptions.Classic.Tests.TransferSubscriptionTest.TransferSubscriptionAsync(True,False) fails at TransferSubscriptionTest.cs:522 (subscription 0 target count expected 10, was 20, i.e. the initial values were delivered twice after transfer). The previous head 3c00b4a5f was green, and the only change since then is the synchronous node-manager drain in CustomNodeManager, which the ReferenceServer uses. Please confirm whether this is a flake or a regression before merging. A rerun alone does not settle it.
Carry the actual immediate-acknowledgement partition out of the classic transfer decision, preserving advertised sequences and the public transfer API. Do not acknowledge replay-owned messages before their responses arrive. Make retained versus acknowledged origin state explicit in integration tests and verify exact replay and initial-value sequences and payloads. Cover empty, gapped, and reentrant-preference partitions with gated real-session regressions.
Preserve transport, certificate, node-manager, directory, subscription and state-machine lifetimes, with deterministic regressions and the agreed reverse-connect/shared-buffer documentation. Keep an active event source in the readiness wait when its old drain completes between activation selection and waiter collection; this prevents reporting an unready replacement as ready.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The critical concurrent lease-generation issue and moderate virtual-node error-swallowing issue remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
Keep peer certificate-chain cleanup alongside quota-scoped message-assembly scheduler unregistration. Preserve both transport lifetime fixes.
Retain the reviewed cancellation and virtual-node validation fixes alongside the upstream fluent configuration, Publish queue, and JSON corrections. Preserve both transport lifetime cleanup paths from the preceding merge.
Terminate failed participant-budget recovery through the existing Faulted handover, including failures at deadline expiry. Keep manual replacement bound to its current lease and manager, fence retired callbacks, and preserve active recovery ownership.
The PR intentionally gives event compensation an independent five-second deadline and surfaces provider enumeration errors. Correct the six stale expectations while retaining ownership and admission assertions and verifying request-cancellation independence, deadline retirement, exact exception identity, and single enumeration. Reproduced all six failures locally. The final 113-case selection passes on net48 and net10.0; independent mutations make all six corrected cases fail. No production behavior or CI gate is changed.
…ce gaps (#4571) ## Summary Fixes the findings of a bug audit of `Opc.Ua.Server`, `Opc.Ua.Gds.Server` and `Opc.Ua.Gds.Client`. Rebased on master after #4516. What went in: - **Audit:** 93 findings, each checked against the OPC UA specification. - **Code review of those fixes:** 35 further findings. - **Spec grounding of every fix commit:** 18 conformance gaps, now closed. Commits are one per finding, named `Fix <ID>`, `Fix review <ID>` or `Fix spec gap <name>`. The IDs refer to the audit report (S = Server areas, G1 = GDS server, G2 = GDS client). Validation: - Each finding was traced in code and checked against OPC 10000 (parts 3, 4, 5, 9, 11, 12, 13, 18, 20). - Every fix commit was checked against the spec text again afterwards: 0 non-conformant, remaining gaps fixed. - Regression tests were added for most fixes. - Suites run on net10.0 before the rebase: Server, GDS, History, Configuration, Features, Core.Security, DI, Redundancy, LDS, InformationModel, PubSub.Server, Client, Sessions and the three Subscriptions suites, all green. The same suites on the rebased branch are still running; results will be posted as a comment. ## Behaviour and user-facing changes ### Secure defaults - **`Historize()` / `HistorianBuilder`** grant HistoryRead only. History writes need an explicit `historyAccessLevel: HistoryRead | HistoryWrite`. - **File-system mount:** - Anonymous sessions can no longer create, delete, move or open files for writing (`Bad_UserAccessDenied`). - UserWritable and UserExecutable now reflect the user. - `AllowAnonymousWrite` restores the old behaviour; the ReferenceServer sample turns it on. - **Default PKI root** for the hosted server, hosted GDS, hosted LDS and `OpcUaApplicationConfigurationProvider`: - Moved from the shared temp directory to `{LocalApplicationData}/OPC Foundation/{ApplicationName}/pki`, and startup fails if no per-user directory exists. - When certificate stores of an earlier version still exist under temp, a warning names both paths. Existing deployments that relied on the default get a new application certificate (and the hosted GDS a new CA) unless `PkiRoot` is set to the old path. - **GDS:** - `GdsServerOptions.AutoApprove` defaults to `false`. - The default KeyCredential store grants no roles unless `AuthorizeRoles` allows them. - Certificate requests (StartSigningRequest, StartNewKeyPairRequest, FinishRequest) need an encrypted channel. - **Hosted server anonymous logins:** - Anonymous is rejected (`Bad_IdentityTokenRejected`) when the default authenticator options disable it. - A custom `AddIdentityAuthenticator<T>()` on its own no longer disables anonymous. - With anonymous disabled and no configured token policies, the endpoint advertises the authenticators' token types instead of Anonymous. - **Runtime-added roles:** their properties and methods need SecurityAdmin over an encrypted channel, like the well-known roles. - **Disabled users:** users disabled through Part 18 UserManagement can no longer log in through `RegisterDefaultAuthenticators(IUserDatabase)`. ### GDS server (Part 12) - **Certificate requests:** - FinishRequest / ReadRequest with another application's RequestId returns `Bad_InvalidArgument`. - Every Start*Request returns a new RequestId. - A CSR without the ApplicationUri is rejected (`Bad_CertificateUriInvalid`). - An abstract or unsupported certificate type is rejected at Start (`Bad_InvalidArgument`). - **Private-key password:** no longer persisted in the JSON applications database. A password saved by an earlier version is read into memory once and dropped on the next save. - **CheckRevocationStatus:** validates offline against the GDS's own trust lists and CRLs. It returns Good for good GDS-issued certificates and `Bad_CertificateInvalid` for input that is not a certificate. - **Certificate-group trust lists:** - Writes need CertificateAuthorityAdmin; SecurityAdmin and application admins are read-only. - Writes are validated with the Part 4 certificate validation. - **AuthorizationService:** - The legacy RequestAccessToken applies `AccessControl` and issues the token for the session identity. - StartRequestToken passes the session identity to every identity-aware provider. - Unknown ResourceIds return `Bad_NotFound`. - Expired token requests and refresh tokens are pruned, with at most 10 000 pending requests (`Bad_TooManyOperations`). - **KeyCredential:** requests with PublicKey + SecurityPolicyUri are rejected with `Bad_SecurityPolicyRejected` (secret encryption is not implemented); a PublicKey without SecurityPolicyUri is rejected with `Bad_InvalidArgument`. - **Audit events:** access-token audit events no longer contain the password or token data. - **RegisterTickets / UnregisterTickets:** need the RegistrarAdmin role when called from a session. - **Managed application nodes:** moved from namespace 0 to their own namespace (`ManagedApplicationsNamespaceUri`); the NodeId is the ApplicationUri, and the mandatory child properties exist. - **Hosted GDS:** - New `GdsServerOptions.CertificateGroups` (`GdsCertificateGroupOptions`), bindable from configuration. - The mandatory DefaultApplicationGroup is always present. - The default CA subject uses the resolved application name. - **JSON applications database:** - A corrupt file throws instead of being silently replaced by an empty database. - Saves are atomic and write in place on bind mounts; on net8+ builds they also follow symlinks and keep the Unix file mode. - Issued certificates and trust-list links survive a reload. ### GDS client - **Trust-list download:** - Reads until an empty chunk (Part 20 short reads). - A size-limit rejection of the final end-of-file read, as sent by released servers, counts as end of file. - **Default certificate group:** a null group means DefaultApplicationGroup, and vendor groups in other namespaces are no longer mistaken for well-known groups. - **Status codes and errors:** - Missing directory proxies report `Bad_ServerNotConnected`. - Short or wrongly typed method outputs throw `BadUnexpectedError` / `BadTypeMismatch`. - **GdsAccessTokenProvider:** no longer caches a failed client task, and one caller's cancellation no longer fails other callers. ### Trust lists, push and ConfigurationFile (Part 12 / Part 20) - **AddCertificate:** - `IsTrustedCertificate=false` returns `Bad_CertificateInvalid`. - Added certificates are validated (Part 4), and a certificate revoked by a current CRL returns `Bad_CertificateRevoked`. - **CloseAndUpdate:** validates the uploaded list against its own certificates and CRLs, not the stores it replaces. Validation failures are no longer recorded in the server's Rejected store. - **RemoveCertificate:** removing a trusted or issuer CA that other certificates still need returns `Bad_CertificateChainIncomplete`. - **Handles:** - Several read handles may be open at once. - Any open while a write handle exists returns `Bad_NotReadable` / `Bad_NotWritable`. - A write open while any handle is open returns `Bad_NotWritable`. - Handles of closed sessions are released. - Idle handles close after ActivityTimeout (default 60 s). - **Status codes:** - Read clamps to the remaining bytes and no longer enforces the size limit. - Oversized Write / AddCertificate returns `Bad_RequestTooLarge`. - Unsupported open modes return `Bad_NotSupported`. - **ConfigurationFile.CloseAndUpdate:** - Empty Targets return `Bad_InvalidArgument`. - The ConfigurationUpdatedAuditEvent is raised with the configuration owner as source. - With the new `IApplicationConfigurationFileTargetProvider`, providers receive the Targets, a bad target returns Uncertain with per-target results, and session-interrupting updates are applied after RestartDelayTime. - **Default namespace permissions:** namespaces registered after the first permission lookup now get their NamespaceMetadata default RolePermissions and AccessRestrictions (no stale negative cache). ### Sessions, identity and roles (Part 18) - **Session timeouts:** a failure closing one timed-out session no longer stops the session-timeout monitor. - **ChangePassword:** an empty old password returns `Bad_IdentityTokenInvalid`. - **Removing or disabling a user:** closes only that user's UserName-token sessions. - **Role configuration changes:** roles granted by an `ImpersonateUser` mapping are kept. - **X509Subject rules:** a certificate subject value containing a quote never matches an X509Subject rule. - **`ImpersonateUser` callback:** no longer runs under the global session lock. - **Session diagnostics:** LocaleIds are kept as the client sent them, while null LocaleIds are ignored for translation. ### Subscriptions and monitored items (Part 4) - **Acknowledgements:** a failing acknowledgement returns a per-entry Bad status instead of failing the whole Publish. - **`DiscardedMessageCount`:** counts messages dropped from the retransmission queue and unsent overflow messages. - **Durable subscriptions:** transferring a restored anonymous subscription works (no NullReferenceException). - **Transferred subscriptions:** sampling and permission checks use the item's current owner session. - **Access denied:** a monitored item whose user may no longer read the attribute gets a `Bad_UserAccessDenied` notification instead of going silent. Non-Value attributes need Browse permission (RolePermissions needs ReadRolePermissions), and User* attributes are evaluated per subscriber. - **ModifyMonitoredItems:** a SamplingInterval of -1 uses the subscription's publishing interval. - **AggregateFilter priming:** needs HistoryRead / ReadHistory permission and is bounded (100 000 values, bounded page size). - **Resource leak:** event monitored items no longer leak a consumer task per create/delete cycle. ### Node management services (Part 3 / Part 4) - **Reference types:** AddNodes, AddReferences, DeleteReferences and Browse reject ids that are not ReferenceTypes with `Bad_ReferenceTypeIdInvalid`. AddNodes, AddReferences and DeleteReferences also reject abstract ReferenceTypes; Browse still accepts them (e.g. `HierarchicalReferences` with includeSubtypes). - **Concurrent AddNodes:** duplicate BrowseNames under one parent get `Bad_BrowseNameDuplicated`. - **TranslateBrowsePathsToNodeIds:** - Requires Browse permission on intermediate nodes and final targets (RolePermissions and AccessRestrictions). - A long RelativePath no longer rejects the whole request. - **SemanticChangeEvent:** - Raised once per actual change of a semantic property, whether or not it is monitored. - Also raised for properties with the SemanticChange access-level bit. - Reports the owning node's TypeDefinition as AffectedType. - **Localized diagnostics:** operation-level text is translated when OperationLocalizedText is requested. ### History and aggregates (Part 11 / Part 13) - **DeleteRawModified:** an unspecified or reversed range returns `Bad_InvalidTimestampArgument`, and StartTime == EndTime deletes the value at StartTime for every provider. - **ReadAtTime fallback:** reads only the values around each requested time. A time whose bound is not found within the search limit returns `Bad_BoundNotSupported` instead of failing the request. - **ReadProcessed:** a ProcessingInterval shorter than one tick returns `Bad_AggregateInvalidInputs`. - **Delta / DeltaBounds:** widen to the next larger signed type when the result does not fit the source type (e.g. a decreasing unsigned counter, or an Int16 overflow). - **Range / Range2:** return the source data type. ### Alarms and fluent API (Part 9) - **Suppression:** an alarm stays suppressed while any suppression group is active, and re-evaluation without a state change does not move TransitionTime. - **`MonitorVariable(variable)`:** sets SourceNode, SourceName and InputNode to the variable and makes it a ConditionSource (HasCondition to the alarm, HasEventSource from the object). - **Fluent input/output arguments:** InputArguments / OutputArguments added through the fluent API to a registered method are readable. - **Root notifiers:** a failed root-notifier registration no longer leaves an orphaned event worker. - **State-machine handlers:** exceptions are logged at Error. ### File system (Part 20) - **Delete / MoveOrCopy:** - Only act on direct children of the directory. - A NodeId whose kind does not match the entry returns `Bad_NotFound` (a file NodeId can no longer delete a directory). - Copying a directory into its own subtree returns `Bad_InvalidArgument`. - **Locking:** - An open file, or a directory containing one, cannot be deleted or moved (`Bad_InvalidState`). - The check is atomic with opens. - While a Delete or Move runs, read opens get `Bad_NotReadable` and write opens `Bad_InvalidState`. - **Open:** - Reserved mode bits or EraseExisting without Write return `Bad_InvalidArgument`. - Read+Write returns `Bad_NotSupported`. - Read while a writer is open returns `Bad_NotReadable`. - **Client `FileSystemErrors`:** maps `Bad_NotReadable` to `IOException`. - **RuntimeNodeSet:** well-formed NodeSets with a stray `Model` element, or two models in one file where one requires the other, now load. ### Server core, hosting and lifecycle - **`SessionEstablishmentQueueLimit` > 0:** CreateSession/ActivateSession now queue for a permit. The permit is acquired before the request is registered, and a waiting handshake releases its worker through the request-parking policy. - **Node manager lifecycle:** - Add, reload and remove no longer wait for parked Publish requests. - A Migrate reload racing with CreateMonitoredItems no longer blocks later lifecycle operations. - Late items on retired or removed managers get `Bad_NodeIdUnknown`. - **HistoryUpdate:** a batch mixing data and event details is bounded by both limits. - **Server stop/start:** stopping the server disposes the configuration watcher and certificate subscription; starting recreates them. - **Monitored-item queue factory:** a factory supplied by the caller or the DI container is no longer disposed by the server. - **Audit events:** each certificate validation error raises its own AuditCertificate event. A failed OpenSecureChannel without a service result is audited as failed. ## New public API (additive) - `IQueuedSessionEstablishmentLimiter` - `StandardServer.OwnsMonitoredItemQueueFactory` (protected virtual) - `DefaultPkiRoot.Get` (Opc.Ua.Configuration) - `TrustList.SetCertificateValidation` - `NamespaceMetadataRegistry.Invalidate` - `IApplicationConfigurationFileTargetProvider` and `ApplicationConfigurationUpdatePlan` - `GdsServerOptions.CertificateGroups` / `GdsCertificateGroupOptions` - `InMemoryKeyCredentialRequestStore.AuthorizeRoles` - `DefaultManagedApplicationsNodeManager.ManagedApplicationsNamespaceUri` - `AuthorizationServiceManager.RequestAccessTokenAsync(..., IUserIdentity?, ...)` - `AllowAnonymousWrite` on the file-system node manager and its factory - `IFileSystemHost.TryBeginMutation` / `EndMutation`, replacing `HasOpenHandles` ## Not fixed / known limitations - **KeyCredential:** secret encryption with the client's PublicKey (requests needing it are rejected). - **CSR SAN restriction:** restricting subject / SAN values copied from a CSR (CA policy). - **Ticket signatures:** ticket signature verification (needs a trusted-signer configuration). - **FileType Read+Write open:** needs a read/write stream contract in `IFileSystemProvider`. - **Custom `IUserDatabase` implementations:** the disabled-user check still scans all users per login. The built-in stores do a single lookup. - **64-bit Delta precision:** limited by the calculator's double arithmetic. ## Tests with changed expectations Existing tests were updated where they asserted the old behaviour: - File-system status codes - Trust-list handle eviction - `DiscardedMessageCount` - Localized diagnostics bits - Delta on unsigned sources - SourceNode of fluent alarms - ReferenceServer transfer permission test (now expects `Bad_UserAccessDenied` notifications) - `UserDeactivationShutdownReviewTests` (the revoked session is a UserName login) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…riptions, redundancy, browse, files, WebApi) (#4572) ## Summary Fixes the findings of a bug audit of `Opc.Ua.Client` and `Opc.Ua.Client.ComplexTypes`, done on master plus #4516 (now merged). - **Audit:** 82 findings across 12 areas, each checked against the OPC UA specification. Final severities: 2 High, 37 Medium, 43 Low. - **Fixes:** 76 fixed, 4 partial, 2 skipped (see [Not fixed](#not-fixed--partial)). - **Review:** a follow-up review of the fixes found 15 more defects in them, all fixed here. - **Tests:** every fix has a regression test, except a few races that cannot be reproduced deterministically; the commit messages say which. The finding IDs (`L<area>-<n>` for the audit, `G<n>` for the review) appear in the commit titles. ## Behaviour and user-facing changes ### Security and endpoint selection - **Reconnect no longer drops to weaker security (L3-1, High).** When the server rejects the configured security policy or mode during reconnect, `SessionReconnectHandler` no longer switches to whatever endpoint the server advertises, None included. It only picks an endpoint at least as strong as the configured one, judged by a locally computed security level. The server-advertised `SecurityLevel` is not trusted. If nothing qualifies, reconnect fails with the original error (new log event `SessionReconnectHandler + 13`) and keeps retrying with the original settings. - **`CoreClientUtils.SelectEndpoint(useSecurity: true)` never returns a SecurityMode None endpoint over opc.tcp (L9-9, High).** It now returns `null` when no secure endpoint matches. - When the discovery URL is `https`/`opc.https`, an HTTPS endpoint with SecurityMode None is still accepted as a last resort, because TLS protects it (Part 6 §7.4.1) (G17). - Endpoints with SecurityMode `Invalid` no longer count as secure. - `GlobalDiscoveryServerClient` and `ServerPushConfigurationClient` now throw `BadSecurityPolicyRejected` when no secure endpoint matches, instead of passing `null` on. The ConsoleReferenceClient sample reports this with a clear error. - **No Bearer/Basic credentials over `http://` (L12-2).** `WebApiClient.Create` and WebApi channel open/reconnect reject them with `BadSecurityChecksFailed`. The public `WebApiClient(HttpClient, …)` constructor also rejects them when the client's `BaseAddress` is not https, and every send checks the address it actually goes to (covers a `BaseAddress` set later). ### Session, keep-alive and identity - **Faster detection of a silent server (L1-1).** The keep-alive Read is now bounded by its `TimeoutHint` (2 × `KeepAliveInterval`), which is what Part 4 §7.32 says the client should do. It then fails with `BadNoCommunication`, so reconnect starts after about 2 × `KeepAliveInterval` instead of `OperationTimeout` (120 s by default). - **Operations fail fast during a channel-manager recovery (L1-2, G9).** `UpdateIdentityAsync`, `TransferSubscriptionsAsync` and `ReactivateSubscriptionsAsync` fail with `BadSecureChannelClosed` (retryable) instead of deadlocking. This covers the operation holding the reconnect lock and any queued behind it. - **Closing or disposing from a handler no longer waits on itself (L1-3, G5, G6).** - `Session` and `ManagedSession` Close/Dispose from a Notification handler no longer wait 30 s for their own background drain. - Disposing a `ManagedSession` from a keep-alive `PublishStatusChanged` handler no longer deadlocks. - Tasks spawned by a handler no longer count as "the handler" (the owner marker is scoped to the work item). - **Switching to an ECC user-token policy takes two ActivateSession calls (L1-4, G4).** This follows Part 6 §6.8.2 (fetch the ephemeral key, then activate). - If the server returns no key, the switch fails with `BadSecurityPolicyRejected`. - If the second activation is rejected, the old identity stays on the override policy with the key just fetched. Restoring the old key was tried: against the reference server the next reconnect then failed to decrypt the identity token. - **`ManagedSession.ConfiguredEndpoint` follows failover (L2-6).** It now reports the inner session's endpoint, so it changes after a failover or network-path rotation. - **An explicit identity update replaces the identity provider (L2-7).** `ManagedSession.UpdateSessionAsync(identity)` stops the provider's refresh loop and drops the provider for good. If the update fails, the loop is restarted. - **Reconnect waits for in-flight calls on a healthy channel (L2-3, G8a).** Reconnect/failover on a healthy channel (certificate or trust-list change, manual reconnect) waits for in-flight service calls. The wait is bounded (max(`KeepAliveInterval`, 1 s)) only once the channel is unhealthy (keep-alive stopped, channel not Ready), so a dead channel no longer holds recovery for `OperationTimeout`. - **Smaller fixes:** - Restore reads the server certificate as a chain; this failed on macOS (L1-5). - Session recovery can no longer run twice at once (L1-6). - Identity expiries more than 49.7 days out no longer throw (L2-2). - Keep-alive reconnect suppression is recalculated on a lease swap (L2-4). ### Reconnect state machine and reverse connect - **⚠ `MaxTotalReconnectTime = 0` now means "no time limit" (L3-7). Reviewer decision needed.** Before, 0 disabled reconnect entirely. Negative values other than `Timeout.InfiniteTimeSpan` now throw `ArgumentOutOfRangeException` when the state machine is constructed; before, they crashed the worker. The XML docs of `ReconnectPolicy`/`ReconnectPolicyOptions` are updated. If 0 should keep meaning "no reconnect", this is a one-line revert. - **A close racing the end of the reconnect budget is no longer lost (L3-4).** It goes through the normal close instead of switching to failover. - **The "reconnect worker" marker covers one step only (L2-1).** It lasts for a single connect/reconnect/failover/close step. Keep-alive, publish and identity loops started in a step no longer count as the worker afterwards. - **Reverse-connect strategies `Undefined` and `Any` alone are removed after the first callback (L8-1).** This matches the documentation; before, they behaved like `Always`. - **`AddEndpoint` for a URL that is already added disposes the replaced listener (L8-2).** ### Service calls - **Batched calls clean up after a failed later batch (L3-2).** When a later batch of `Browse`/`BrowseNext`/`HistoryRead`/`CreateMonitoredItems` fails, the continuation points and monitored items created by earlier batches are released or deleted before the exception is rethrown. This adds one request. `AddNodes`/`RegisterNodes` are not undone. - **Batched diagnostic infos point at the right strings (L3-3).** The string-table indexes are rebased when diagnostics are padded. - **`ReadBytesAsync` rejects oversized responses (L3-5).** It throws `BadUnknownResponse` when the server returns more bytes than requested, and `BadEncodingLimitsExceeded` beyond `int` range. - **`ReadByteStringInChunksAsync` handles an unknown server limit (L3-6).** When the server's `MaxByteStringLength` is 0 (absent) or larger than `int`, it falls back to the client's `MessageContext.MaxByteStringLength`. It throws only when both are unknown. ### Browse and NodeCache - **Empty browse pages with a continuation point are followed (L9-1, L9-4, L9-5, G16).** Part 4 §5.9.2.1 allows a server to return an empty page with a continuation point, so browsing keeps following it. After 10 consecutive empty rounds it gives up with `BadNoData` and releases the continuation point. - `BrowseAsync(NodeId)` now throws instead of silently returning a truncated list. - `BrowseStreamAsync` yields a `BadNoData` result for that node. - The same cap, through one shared helper, now also protects `RoleManagementClient.ListRolesAsync` and the FileSystem enumerate/empty-check loops. - **Continuation points are released on failure (L9-3).** `BrowseAsync(NodeId)` releases them on any exception, and `BrowseStreamAsync` releases all points of the current page when the consumer stops early. - **NodeCache lookups propagate cancellation (L9-6, L9-8).** `FindAsync`, `IsKnownAsync`, `FetchNodeAsync`, `FindReferenceTypeNameAsync`, the reference-table fill and the type-hierarchy search now let `OperationCanceledException` through. Before, a cancelled lookup looked like "node does not exist". - **A NodeClass mismatch fails only that node (L9-7).** It returns `BadNodeClassInvalid` for that node instead of throwing `KeyNotFoundException` for the whole batch. - **Smaller fixes:** - `MaxNodesPerBrowse` ≥ 2^31 no longer breaks the managed browse (L9-2). - Null discovery URLs are skipped (L9-10). - `ModelChangeTracker` no longer cancels a joined start (L9-11). - **Log changes:** `BrowserContinuationPointExistsButBrowse` (Browser + 0) was removed, and `BrowserServerReturnedEmptyReferencesBut` now includes the round count. ### Subscriptions (default engine) - **Recovery after a transfer delivers each message exactly once (L5-1, G22).** Messages already dispatched are acknowledged instead of republished and delivered again. Messages whose earlier republish failed are now republished and delivered, with the Republish flag and after newer messages, instead of being acknowledged and lost. - **A keep-alive that proves a gap triggers republish at once (L5-5, G23).** The dedup mark moves only past messages that were actually recovered, so a failed republish is retried by the next data message. - **Item changes that fail are retried with backoff (L5-3).** They are retried on the next apply pass after the state manager's 250 ms–5 s backoff, instead of six times back to back. The six attempts now span about 13 s. - **Extreme revised intervals no longer throw (L5-4, L6-1).** Very large or NaN intervals and counts revised by the server are capped (NaN falls back to the requested value). Before, the overflow could stop every publish worker. - **`SetTriggeringAsync` from a callback no longer hangs (L5-7, G21).** While the subscription is deleted, recreated or disposed, it returns `BadOperationAbandoned`. A cancelled reset no longer makes later calls fail. - **Publish responses for unknown subscription ids (L6-3, L6-4, G20).** The client no longer re-sends acknowledgements the server already processed. An unknown id seen while a creation is pending is left alone instead of being deleted. - **Smaller fixes:** - A null and an empty `IndexRange` are treated as equal, and restored snapshots use `null` (L5-6). - A failed partition attach during restore is rolled back (L6-2). - `StreamingSubscription` throws `ObjectDisposedException` after dispose (L6-6). ### Subscriptions (classic `Subscription`) - **Republish no longer abandons other missing messages (L7-1).** A Republish response no longer overwrites `AvailableSequenceNumbers`, so other missing messages are not given up while the server still holds them. - **Late copies are no longer delivered twice (L7-2).** Copies of already-processed sequence numbers are ignored, and creating a new server subscription resets the sequence state. - **Disposing from a keep-alive handler no longer deadlocks (L7-3).** `Dispose()` from a keep-alive `PublishStatusChanged` handler can return while that callback is still finishing. - **Transfer of a restored subscription (L7-4, G27).** - It succeeds even when some saved items were never created. - The server subscription is deleted only when this session's own TransferSubscriptions just moved it and adoption failed definitively (item count mismatch or a non-transient status). Transient errors and `ReactivateSubscriptionsAsync` never delete it. - The public `Subscription.TransferAsync` never deletes. - **Smaller fixes:** - Cloned items keep their triggering links (L7-5). - `CreateAsync` clears stale server ids (L7-6). - A `Good_SubscriptionTransferred` for the client's own transfer is ignored (L7-7). - An item removed while its create is in flight is deleted afterwards (L7-8). - The publish timeout for very long keep-alives no longer overflows (L7-9). - A negative `MaxMessageCount` is handled (L7-10). - `LastMessage` tolerates a wrong notification type (L7-11). - Pending acks exactly 2^31 away no longer throw (L4-1). - **Log changes:** new log events `Subscription + 68..70`. ### Redundancy - **Warm/Hot demotion (L4-2, G1, G3).** - Without token reuse, the demoted leader closes its session and reconnects as a fresh standby. - With token reuse, it abandons the shared server session locally, without CloseSession, so the new leader can reactivate it and keep its subscriptions. This uses the new internal methods `ManagedSession.AbandonServerSessionAsync` and `Session.AbandonServerSession`. - `RoleChanged(false)` is now raised before the standby connect and no longer depends on it, so `CurrentSession` can briefly be `null`. - Exceptions from `RoleChanged` handlers are logged and do not stop the teardown. - **Promotion (L4-3, G2).** A promoted standby that adopts the leader's session first closes its own server session. A retried promotion keeps the session it already adopted. - **Client failover takeover (L4-4, partial).** Subscriptions the backup prepared in advance (not yet created, `TransferId` set to the active client's id) are moved through the session's own transfer and keep publishing. `docs/HighAvailability.md` is updated. - **`ShouldFailover` through the interface (L4-6).** Calling it through `IServerRedundancyHandler` no longer fails over away from a server that is still Healthy. ### Fluent / dependency injection - **Shared DI session (L10-5).** The shared session from `AddClient` is reconnected after a caller disposes it, and is disposed with the container. After that the connect delegate throws `ObjectDisposedException`. - **New public API: `ManagedSessionBuilder.WithServerRedundancyOptions(ServerRedundancyOptions)` (L10-6).** This is additive. The `Session.ServerRedundancy` options are now actually applied. - **`AddDiscoveryAndConnect` rewrites the advertised address (L10-7).** It replaces the host and port the server advertises with those of the discovery URL (Part 4 §5.6.2.1, NAT/containers), and prefers endpoints with the same scheme. ### State machines and role management - **`GetSubStateMachineAsync` returns the instance's own sub-state machine (L10-1).** Before, it returned the type's placeholder. - **`WaitForStateAsync` reports failures with OPC UA status codes (L10-2).** It throws `BadNotFound` when `CurrentState/Id` cannot be resolved, and `BadInvalidState` when observation ends without reaching the state. Before, both surfaced as `OperationCanceledException`. - **`RoleManagementClient.ListRolesAsync` pages correctly (L10-3).** It follows continuation points, releases them on any failure, and throws on a Bad browse status instead of returning an empty list. - **Role properties are read into the right fields (L10-4).** Properties whose namespace cannot be resolved are skipped instead of shifting the other values. ### File system, temporary files and alias names - **Path escaping (L11-8, G12).** - Path segments escape `:` and `/` with `&`, and `&` itself as `&&` before those characters or at the end of a name. - `Format` and `Parse` round-trip all names. Typed input ending in `&/` now reads the slash as part of the name. - A segment without a namespace prefix also matches a namespace-0 child when nothing matches in the parent's namespace. - **Stale path cache (L11-1).** When a cached lookup fails, `GetInfoAsync` retries once from the server. `ExistsAsync` and `GetFileAsync` report "not found" instead of throwing. - **`GenerateFileForReadAsync` waits for the file (L11-2).** It waits until the file-generation state machine reaches ReadTransfer (`Idle` counts as not started yet), and throws `IOException` if it reports Error. The wait is bounded only by the caller's token. - **Non-recursive delete refuses hidden children (L11-3).** It refuses when the directory holds children that the subtype options hide from enumeration. Before, the server deleted them all. - **Alias names (L11-5, L11-6, L11-7).** - `BadMethodInvalid` maps to `NotSupportedException` for all alias calls, so `UseVerbose` falls back to `FindAlias`. - A duplicate alias name resolves to all its targets. - If the refresh strategy fails to start, the resolver logs a warning and keeps working without automatic invalidation. New log event block `AliasNameResolver = 560`. - **Null profile entries (L11-4).** Null `ServerProfileArray` entries no longer crash `GetConformanceInfoAsync`. ### WebApi transport - **Failures come back as `ServiceResultException` (L12-1).** - A timeout becomes `BadRequestTimeout`. - HTTP 429/503 becomes `BadServerTooBusy` with the Retry-After hint, on every target framework. `WebApiClient` checks the status itself (reusing `HttpsTransportChannel.CreateServerTooBusyException`), so callers of `WebApiClient` also get a `ServiceResultException` instead of an `HttpRequestException` for these two statuses (G15). - An IO error becomes `BadNotConnected`. - **Timeouts (L12-1, G14).** - `OperationTimeout` applies to each request. When `RequestTimeout` is unset, HttpClient's 100 s default no longer cuts off long Publish requests. - The GetEndpoints call at open is bounded by `OperationTimeout`, and cancelling the open now throws. - **Core change:** `HttpsTransportChannel.MapRequestFailure` went from private to internal so the WebApi channel can reuse it. ### ComplexTypes - **Out-of-range union selector (L12-3).** A union whose SwitchField is larger than its field count now fails with `BadDecodingError` when decoded and `BadEncodingError` when encoded (Part 6 §5.2.8). - **Verbose JSON unions (L12-4).** They now decode their selected field instead of `null`. ## Not fixed / partial - **L2-5 (skipped):** an identity provider combined with a caller-supplied reverse connection still can't connect to an undiscovered endpoint. Fixing it means choosing the identity after CreateSession, a design change. - **L4-5 (skipped):** the one-time KeyCredential token needs a fresh token on each reactivation. Today reactivation fails and the client falls back to a new session. - **L4-4 (partial):** only subscriptions the backup prepared in advance survive a takeover. - **L3-2 (partial):** `AddNodes`/`RegisterNodes` batches are not rolled back. - **L2-3 (partial):** service calls still hold the session lock for their whole round trip. - **Known from the review, not fixed here:** - G7: the batch cleanup request can't be cancelled. - G11: the DI session's close at host shutdown never reaches the server, and a closed session keeps being handed out. - G13: a file renamed by another client is still found under its old path. - G25: a clone-based transfer doesn't mark the original subscriptions. - G27b: the transfer item check matches by count, not by ID. - **Outside this PR:** the reference server ignores `ECDHPolicyUri` on ActivateSession (`SessionSecurityPolicyHelper`), which breaks Part 6 §6.8.2. As a result, the L1-4 two-step ECC switch ends in `BadSecurityPolicyRejected` against our own server. ## Testing All suites on net10.0, on the branch before the master merge. Every suite had 0 failures: | Suite | Passed | Skipped | |---|---|---| | Opc.Ua.Client.Tests | 2698 | 0 | | Opc.Ua.Redundancy.Client.Tests | 134 | 0 | | Opc.Ua.Subscriptions.Classic.Tests | 57 | 0 | | Opc.Ua.Bindings.Https.WebApi.Tests | 385 | 0 | | Opc.Ua.Sessions.Tests | 782 | 379 | | Opc.Ua.Subscriptions.Tests | 640 | 6 | | Opc.Ua.Client.ComplexTypes.Tests | 2858 | 0 | After merging current master (net10.0), every suite had 0 failures except one, a flaky new test fixed in 9f52952: | Suite | Passed | Skipped | Failed | |---|---|---|---| | Opc.Ua.Client.Tests | 2719 | 0 | 1 (the flaky test) | | Opc.Ua.Redundancy.Client.Tests | 134 | 0 | 0 | | Opc.Ua.Client.ComplexTypes.Tests | 2858 | 0 | 0 | | Opc.Ua.Subscriptions.Classic.Tests | 57 | 0 | 0 | | Opc.Ua.Bindings.Https.WebApi.Tests | 397 | 0 | 0 | | Opc.Ua.Sessions.Tests | 790 | 379 | 0 | | Opc.Ua.Subscriptions.Tests | 640 | 6 | 0 | The touched test projects also build on net48 with 0 warnings. 🤖 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>
…acked 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>
…yml (#4718) ## Description `Opc.Ua.Stress.Tests` with `Category=Contract|Category=Integration` had five failures on master. The README says these categories run on every PR, but no workflow ran them: `get-ci-matrix.ps1` excludes the Stress project, and `stress-test.yml` only ran `ChaosTCP`. The old `Channels.Stress` special case in `buildandtest.yml` never matched the project name, and #4513 removed it. So the tests rotted unseen. Two of the five failures were real product bugs. ### Product fixes - **Session recreate after a server restart failed forever (regression from #4503).** User-token PolicyIds are assigned by the server and are not stable across a restart. In the failing run the client had cached `Anonymous|20` and the restarted server offered `Anonymous|2`. The CreateSession check added in #4503 requires an exact match, including PolicyId, so every `RecreateInPlace` failed with `BadSecurityChecksFailed` ("different security policy for the selected user identity token"). The managed session then retried without end. - `Session.OpenCoreAsync` now adopts the server's policy when only the PolicyId differs. TokenType, SecurityPolicyUri, IssuedTokenType and IssuerEndpointUrl must still match, so a downgrade is still rejected. The token is restamped with the new id. - **A certificate rotation during a reconnect reactivated every session twice.** `ChannelEntry.ReconnectCycleAsync` only noticed the superseded client certificate *after* it had notified participants. It then replaced the transport and notified them again. The check now also runs right after the transport reconnects, before participants are notified. ### Tests updated to the #4516 contracts - `RetryBudgetEnforcementTests`: `Session.ReconnectManagedChannelAsync` gained an optional reverse-connection parameter, which the reflection call must supply. - `GateAndBypassTests`: the participant callback token is cancelled once `OnReconnectAsync` returns. The spawned worker now runs on the test token. - `LeaseLifecycleTests`: releasing the last lease cancels the recovery in progress, as documented. The reconnect caller now expects `OperationCanceledException`. ### CI - New `Contract and Integration` job in `stress-test.yml`, with the same gate as Chaos TCP: nightly, manual, and PRs labelled `stress`. It takes about 1 minute of test time. - The README now says where these categories actually run. ### Tests - New `SessionIdentityReviewTests.CreateAdoptsRenumberedUserTokenPolicyIdAsync`, covering both a renumbered id (accepted) and a changed token security policy (rejected). It fails without the fix. - Each of the 5 fixed tests passed 3/3 runs in isolation. The full `Contract|Integration` filter passed 45/45 twice in Debug and once in Release. - Related tests also pass: Core.Tests channel tests 587/587, Sessions.Tests channel tests 119 passed (63 skipped), Client.Tests channel/session/reconnect tests 512/512. - The net48 build of `UA.slnx` is clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Marc Schier <marcschier@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Description
Bound managed-session recovery, preserve Publish workers and recovery ownership, and address the client, server, transport, and certificate audit findings and subsequent concurrency review. Also reduce the memory retained by exhaustive session-test diagnostics.
Recovery and Publish ownership
ManagedSessionOptions.ChannelReconnectTimeoutthrough direct construction, the fluent builder, and DI. The automatic duration ismax(3 * KeepAliveInterval, revised SessionTimeout, OperationTimeout), sampled at cycle start. Positive overrides are supported;Timeout.InfiniteTimeSpanexplicitly opts out. Raw sessions retain their unlimited default.CreateReconnectBudget(TimeProvider)as a requiredIReconnectParticipantmember, as agreed in review. Custom participants implement it and may return null for no participant-specific bound.Faultedevent hands control to the outer reconnect/failover policy. Preserve session recreate admission through final Publish cleanup and keepalive suppression until shared recovery is complete.Faulted, preserve the original diagnostic, and never silently replace a finite budget with unlimited recovery.Audit and review corrections
ClientBasecaller-handle sequence is distinct from the non-wrapping WSS wire allocator. Existing exhaustion/reopen, late-reply, and Cancel-translation regressions verify the connection-local contract; no allocator behavior was changed by this clarification.BadNodeIdInvalid; callback type mismatches and resolver service errors keep their exact status. Per-item boundaries preserve unaffected reads, writes, history operations, method calls, browse paths, and monitored-item creation/restoration, including permission-validation paths. Caller cancellation still propagates; stored-item validation failures are logged without aborting later restores.Latest review-feedback corrections
The expanded readiness controls also exposed a real generation-handoff race: an old source could finish draining after activation was deferred but before readiness tasks were collected, causing the wanted replacement to be omitted from the wait set. A deterministic test gates that exact interval; the fix keeps the replacement in the readiness wait. The original race passed 200 repetitions after the fix. A separate control now awaits both producer readiness and iterator entry under the same original deadline, rather than treating those independent signals as equivalent. Temporary logging and repetition are removed.
CI integration contracts
The History and Server CI failures exposed integration tests that still asserted the behavior intentionally changed by the audit fixes. A Windows WoT negative test raced its caller deadline against upstream retries. A subsequent Ubuntu Client failure exposed a real automatic-update versus explicit-restoration race:
AliasForreferences, instead of racing a startup-only snapshot expectation. Explicit refresh opt-out controls remain.Connectedevent from actual outer recovery completion. A deterministic timer-order test covers both cooperative and cancellation-ignoring resolvers, cancellation before return, rejection of late cached results, and a fresh subsequent lookup.ArrayOf.Countinto an integer-only trust-list summary. Projecting the same count throughSpan.Lengthkeeps cardinality distinct from serialized contents without allocating, dropping diagnostics, suppressing the rule, or dismissing the alert. Five log-contract cases cover null/empty/nonempty results and mixed chain-capable/fallback listeners, asserting the exact count, message, and structured properties.RepublishAfterTransfercannot change the already-selected acknowledgement ownership.No tests were disabled, assertions loosened, coverage thresholds lowered, or CI checks made optional. Existing platform exclusions and Explicit debug variants remain.
Related Issues
Validation
Lease-election review correction (
a13afa641182c70983e16bbcfebf507e666a45a5):The correction is based on the existing upstream merge
43079e2b851fe338a4082066f8d14abfdafb6e32. Three deterministic stale-verdictcases fail on the original source and pass with separate confirmation fencing.
Six new cases cover the initial foreign-owner verdict, failed-CAS revalidation
in both invocation orders, and fresh ownership loss through both read paths,
including revalidation begun after another successful confirmation.
The selection is the full
Opc.Ua.Core.Tests.Redundancynamespace, includingall 38
SharedStoreLeaseElectionTestscases and the existing expiry, clockrollback, disposal, and overlapping-success regressions. New witnesses are
StaleForeignLeaseReadDoesNotRevokeNewerAcquisitionAsync,StaleFailedRevalidationDoesNotRevokeNewerAcquisitionAsync, andFreshForeignLeaseObservationRevokesLeadershipAsync. They assert resultingleadership, exact transitions, independent progress, and the unchanged expiry
boundary. Retaining the initial generation for the fallback read independently
makes the fresh-loss-after-confirmation control fail; that mutation was removed
before the final rebuilt runs.
The successful scoped command is
dotnet test tests\Opc.Ua.Core.Tests\Opc.Ua.Core.Tests.csproj -c Debug --no-restore --filter "FullyQualifiedName~Opc.Ua.Core.Tests.Redundancy",with
CustomTestTargetand-fmatching each row and that framework's assetsrestored. Both changed C# files pass formatting and analyzers through
informational severity on both frameworks. These are scoped results, not a
full-solution test or measured coverage-delta claim. CI monitoring is paused
for this feedback-only operation; no current-head CI success is claimed.
Latest CI contract correction (
1cd1f2047bb5b50c9bf300b1ff710b0b2e935a29):All four failing Server shard 23 jobs in
run 36120946740
reported the same six cases. Two local replays reproduced all six failures.
The source changes and documentation from
cd43b831aestablish both intentionalcontracts: independent bounded event compensation and surfaced provider errors.
Only the two test files are corrected; production code and CI policy are unchanged.
The selection contains
MonitoredItemManagerLifecycleTests,DirectoryObjectStateTests,DirectoryBrowserBoundedEnumerationTests, andEventCompensationReadinessHasFiniteDeadlineWithInfiniteSourceTimeoutAsync.Both changed files pass formatting and analyzers through informational severity
on both frameworks. Independent mutations restoring a non-cancellable
compensation token and swallowing provider errors fail all four lifecycle
cases and both browser cases, respectively. Both mutations were removed before
the final passing runs and commit.
The scoped command is
dotnet test tests\Opc.Ua.Server.Tests\Opc.Ua.Server.Tests.csprojwith
CustomTestTargetand-fset to the selected framework,--no-restore, and--filter "FullyQualifiedName~MonitoredItemManagerLifecycleTests|FullyQualifiedName~DirectoryObjectStateTests|FullyQualifiedName~DirectoryBrowserBoundedEnumerationTests|FullyQualifiedName~EventCompensationReadinessHasFiniteDeadlineWithInfiniteSourceTimeoutAsync".The modern run additionally uses
-c Release /p:CollectCoverage=true --collect "XPlat Code Coverage" --settings tests\coverlet.runsettings.xml.These are scoped results, not a full-solution test or measured coverage-delta claim.
Hosted checks for that commit were pending at publication. The fail-fast CI
watch was stopped when the requested operation switched to review feedback;
no current-head CI success is claimed.
Earlier master merge (
8b15ea6ef9fc2214ac8ec886f373bb3846dd6d6f, baseb4136d8e9bab0df680f41d9ef985f519160534b1):The
TcpListenerChannelconflict preserves both lifetime responsibilities:disposal clears the retained peer-certificate chain and unregisters the
quota-scoped message-assembly scheduler. Both fields and all upstream deadline
handling remain intact. The only additional manual source adjustment adds the
parentheses required by the existing analyzer around the session-count fallback.
Master advanced during validation. The initial transport merge
80303c434ce813cf75aa7fe54a6a32fe2aeafc30was retained, and the newer base wasmerged normally in
8b15ea6ef9fc2214ac8ec886f373bb3846dd6d6f, without rewritinghistory. The additional recovery, fluent-pipeline, and test-framework overlaps
were inspected; the branch's cancellation, validation-error, and acknowledgement
ownership fixes remain alongside the upstream changes.
Full
UA.slnxbuilds passed on net10.0 and net48 with 0 warnings and 0 errors.The final scoped tests rebuilt against the committed tree
2074f8efcb61d8d9c6106e7d6f76ee941940e358:The net48 skips are existing platform exclusions: six RSA_DH_AesGcm nonce cases
and one ClientWebSocket certificate-validation case requiring .NET 7 or later.
The selection covers transport cleanup, reassembly budgets/deadlines/admission,
peer-chain ownership, server session binding, fluent lifecycles and source
generation, reconnect and keepalive, transfer acknowledgements, Publish queues,
and JSON encoding. Modern-.NET WSS transport and real-server integration cases
also pass. The resolved file passed solution-relative formatting and analyzer
verification through informational severity on both frameworks.
These are scoped test results, not unfiltered full-suite or measured-coverage
claims. Hosted CI was not monitored or repaired as part of this merge-only
operation.
Selected local results below are overlapping validation runs, not distinct-test totals.
Earlier review-feedback batch (
cd43b831a8f11de3f7d65c3681712a61e3c669a5):The final net10.0 scoped selection passed 471 tests, with no failures or skips.
The final net48 solution-root selection passed 439 tests, with no failures and
one unchanged platform skip (
ValidateServerCertificateFallsBackToTlsPolicyErrors,which requires newer ClientWebSocket certificate-validation support).
Both full nonincremental solution builds completed with 0 warnings and 0 errors,
and all 40 changed C# files passed informational-level formatting/analyzer
verification. Both frameworks validated the identical final tracked diff.
The focused matrix covers every one of the 20 current review threads, including
real mid-frame WSS cancellation, stale admission contexts, directory continuation
ownership, multi-element Browse/Translate failures, and the readiness drain race.
The net10.0 solution-root test command has a separate pre-existing runner conflict:
three unrelated AOT projects reject the VSTest target under Microsoft.Testing.Platform
2.4.0 and SDK 10. Per-project net10.0 results are recorded separately; no runner
migration, test exclusion, assertion relaxation, or CI-policy change is included.
These are bounded selections, not unfiltered full-suite or measured coverage claims.
At publication of
cd43b831a, hosted CI was not green: theOPCFoundation.UA-.NETStandardcheck reportedACTION_REQUIRED, AppVeyor reportedFAILURE, and the two signed-build jobs were in progress. No CI jobs were rerun,cancelled, approved, or repaired as part of that feedback-only operation.
Classic transfer correction (
f425376fca045794122e19aaf9cafebc4a717d91):The preceding head's
macOS net10.0 shard 25/30
failed
TransferSubscriptionTest(DisconnectedRepublish).TransferSubscriptionAsync(True,False)at the static-node count assertion: expected 10, received 20. The trace contains a successful,
explicitly requested republish of retained sequence 1 and a fresh initial-value batch at sequence 3,
not duplicate dispatch of one response.
Controlled lost origin acknowledgements reproduced that incorrect test expectation on both the
preceding PR head and exact master base
a2539cb53dd1666cef856f33812d7a8830d68909.The maintainer explicitly authorized correcting this existing issue in the PR. Retained replay
and requested initial values are independent, as specified by
OPC UA Part 4, TransferSubscriptions.
The reason that the original hosted run retained its particular origin message remains unproved.
The expanded full-suite run then exposed a real production race, not just a count assertion:
Session queued acknowledgements for all transferred available messages before the asynchronous
Republish responses arrived. Publish could delete those messages first, causing
BadMessageNotAvailable. A gated real-Session/classic-engine regression failed both replay-enabledcases on the original implementation while both no-replay controls passed. The ownership fix
passes all 24 combinations of empty/consecutive/unsorted-gapped queues, replay preference,
sequential delivery, and a reentrant preference change.
All 40 normal real-server transfer cases pass on both frameworks. Independent mutations
suppressing actual replay and duplicating observed sequence records each fail the intended
assertion; both mutations were removed. The full classic suite passes on both frameworks.
Hosted CI was pending when that correction was published; the earlier failed hosted job was not
rerun for a lucky pass.
Local net48 caveat: the full Client run timed out in the pre-existing fake-clock test
ReconnectAsyncSwapsFaultedLeaseEntryAsync, which does not exercise subscription transfer.Its unchanged isolated replay passed, but that does not resolve the full-suite timeout.
No unrelated code or assertions were changed, and a clean full Client net48 run is not claimed.
clr.dllwith native access violation0xc0000005; an unchanged-assembly full diagnostic rerun passed as recorded above. That native host crash has not been attributed or claimed fixed.c985f8bded77a60449d9cd381c8a004d8590ccf6completed with 168 passing checks, one expected release-package-publication skip, and no failures or pending checks. All test shards, signed builds, container builds, the coverage check, and the CodeQL PR check passed. Hosted CodeQL analysis1829262912completed successfully and marks count-flow alert 385 fixed. This is evidence for that head, not a claim that subsequent review follow-ups have completed hosted CI.Session-test diagnostic memory
Complete browse listings are streamed to unique NUnit attachments rather than retained in inline NUnit/VSTest output. Detached regression-test results are cleaned up without deleting runner-collected attachments.
The separate Azure build-18452 VM deprovisioning cause remains unproved without provider logs. The memory measurements do not establish its cause.
Unfiltered local
UA.slnxtests, including the long stress/soak suites, have not been run on both frameworks. A local coverage delta was not collected; builds and focused tests are not substitutes for those claims.Checklist
Tests have been added and pass as recorded above; the hosted coverage check passed on the prior head identified above. The combined tests/coverage-increase checkbox remains open because no local coverage delta was measured.