Repository navigation
Fix AddNodes scaling and OpenSecureChannel revocation errors found by CTT Monitored Item, Node Management and security runs - #4486
Merged
Conversation
…ays linear The CTT Node Management Delete Node Err-002.js adds 15,000 variables below one folder and warned that AddNodes responses took 400-800 ms. Every added node searched all children of the parent twice (the BrowseName duplicate check, which also copied the children list, and the NodeVersion lookup in the model change filter), so adding N nodes below one parent was O(N^2): 5,000 nodes took 0.8 s, the next 5,000 2.1 s and the next 4.0 s. NodeState.FindChild now searches children lists with 64 or more entries through a BrowseName index held in a ConditionalWeakTable, so nodes with few children carry no extra field. The index keeps first-match semantics, is maintained by AddChild/RemoveChild, dropped on replacement, and invalidated when a child is renamed or moves to another parent. Lists whose children point at another parent (clones) are searched linearly. The AddNodes duplicate check uses FindChild instead of copying the list. The same three 5,000-node batches now take 141, 166 and 172 ms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Monitor Value Change V2 020.js skips the configured ByteString array when any element is shorter than four bytes; its message only names elements 0-2, but the script takes the minimum length of all elements. The static Scalar_Static_Arrays_ByteString value had a 1-byte and a 3-byte element, and the simulation padded only the first three elements of dynamic arrays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ailed The CTT Security Certificate Validation 002.js connects with a certificate whose issuer has no revocation list and warns that the server returned BadCertificateIssuerRevocationUnknown. Part 4 6.1.3 (Table 106) requires Bad_SecurityChecksFailed to be reported to the client for the revocation check (Bad_CertificateRevoked, Bad_CertificateIssuerRevoked) and for a missing revocation list (Bad_CertificateRevocationUnknown, Bad_CertificateIssuerRevocationUnknown). TcpServerChannel masked only Bad_CertificateRevoked and passed the other three through, while the CreateSession path already masked all of them. Only validity period, host name, URI and usage errors are now reported with their own code. The audit events still carry the specific result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Monitor Value Change V2 042.js monitors element [1,1] of every configured multi-dimensional array, adds one to each element and expects a data change for all 19 items. Element [1,1] of Scalar_Static_Arrays2D_Float was -1.03179664E+33, where adding one is below the Float precision, so the written value was unchanged and the server correctly sent no notification. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
plans/ctt-testing.md (as introduced by #4482) gains how to run Monitored Item and Node Management Services and how to run Security General and Security User Token without -a: an isolated server PKI built from the CTT project's copyToServer folders, restored before every part, and waiting for the server's "Server started" line. plans/ctt-issues.md records the evidence for the 042.js item (C9) and the 015.js PolicyId check (issue 17), the new CTT defects C32-C36, the server findings fixed on this branch, and configuration and coverage notes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
12 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Certificate-error handling and child lookup/indexing issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Addresses CTT-identified AddNodes scaling and OpenSecureChannel certificate-error handling, with sample-data, tests, and documentation updates.
Changes:
- Adds indexed child lookup and invalidation.
- Updates certificate validation error mapping.
- Refreshes reference data, regression tests, and CTT guidance.
File summaries
| File | Summary |
|---|---|
tests/Opc.Ua.Types.Tests/State/NodeStateChildNameIndexTests.cs |
Tests child-name index behavior. |
tests/Opc.Ua.Server.Tests/NodeManager/AsyncCustomNodeManagerNodeManagementTests.cs |
Tests AddNodes duplicate detection and scaling. |
tests/Opc.Ua.Core.Tests/Stack/Transport/TcpServerChannelCertificateErrorTests.cs |
Tests certificate-error mapping. |
src/Opc.Ua.Types/State/NodeState.cs |
Adds indexed child lookup; rename/index atomicity remains unresolved. |
src/Opc.Ua.Types/State/BaseInstanceState.cs |
Tracks parent changes for index invalidation. |
src/Opc.Ua.Server/NodeManager/AsyncCustomNodeManager.cs |
Uses indexed duplicate detection; full QualifiedName equality remains unresolved. |
src/Opc.Ua.Core/Stack/Tcp/TcpServerChannel.cs |
Maps certificate errors; nested results and detailed validation errors require correction. |
samples/Quickstarts.Servers/ReferenceServer/ReferenceNodeManager.cs |
Updates ByteString array padding. |
samples/Quickstarts.Servers/ReferenceServer/Model/ReferenceServer.NodeSet2.xml |
Updates sample seed values. |
plans/ctt-testing.md |
Documents CTT procedures and results. |
plans/ctt-issues.md |
Records CTT defects and findings. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code coverage✅ Coverage gate passed.
Uncovered changed lines
Coverage is above the recorded baseline - consider ratcheting Thresholds live in |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4486 +/- ##
==========================================
+ Coverage 81.04% 81.05% +0.01%
==========================================
Files 2055 2056 +1
Lines 303285 303365 +80
Branches 52552 52579 +27
==========================================
+ Hits 245794 245907 +113
+ Misses 39268 39240 -28
+ Partials 18223 18218 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…security # Conflicts: # plans/ctt-issues.md # plans/ctt-testing.md
marcschier
approved these changes
Sep 14, 2026
… names - UaSCBinaryChannel throws the certificate validation result with ThrowIfInvalid, so the nested errors of the validator reach TcpServerChannel. TryGetReportableCertificateError reports a validity, host name, URI or usage error only when every error in the chain is one of those; any other nested error is reported as BadSecurityChecksFailed. Tested with synthetic chains and with results of CertificateValidationCore for a trusted and an untrusted expired certificate. - NodeState.FindChildWithQualifiedName compares the full browse name of generated child slots, which FindChild overrides resolve by Name only, and searches the child list otherwise. The AddNodes duplicate check uses it, matching the previous GetChildren comparison. - A rename updates the browse name and drops the parent's index under the parent's children lock, so a concurrent lookup cannot see the new name with an index that still maps the old one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
added a commit
that referenced
this pull request
Sep 14, 2026
#4486 already registers C32-C36 for the MonitoredItem, NodeManagement and Security findings. Its C35 is the same opc.wss endpoint selection as the Session Base entry, so the entries now reference each other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…reateSession StandardServer.OnApplicationCertificateError (CreateSession) and TcpServerChannel (OpenSecureChannel) each carried their own list of the certificate errors Part 4 6.1.3 Table 106 hides behind BadSecurityChecksFailed, and they had diverged: the CreateSession list missed BadCertificatePolicyCheckFailed and looked at the outer status only. Both now use CertificateErrorReporting in Opc.Ua.Core, which hides the result when any nested error is a structure, chain, policy, trust list or revocation error. CreateSession throws the validation result with ThrowIfInvalid so the nested errors reach the mapping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@copilot resolve conflicts |
…security # Conflicts: # plans/ctt-issues.md # plans/ctt-testing.md Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>
|
|
Contributor
marcschier
enabled auto-merge (squash)
September 15, 2026 09:33
romanett
added a commit
that referenced
this pull request
Sep 15, 2026
Master now carries the Monitored Item/Node Management/Security CTT triage (#4486), whose plans docs include an earlier copy of this branch's A&C text. The conflicting A&C paragraphs keep this branch's newer wording (C42/C43 counts and the server-side evidence for the Enable event stall); master's C32–C36 entries and its ctt-testing.md sections 7–9 are kept unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 of 7 tasks
marcschier
added a commit
to marcschier/UA-.NETStandard
that referenced
this pull request
Sep 15, 2026
Integrate master 162fc5e (OPCFoundation#4486). Preserve the branch CreateSession certificate ownership, application URI and SecureChannel checks while retaining nested validation errors for upstream shared client-status masking and audit reporting. Keep both indexed BrowseName reuse and stable NodeId admission: the regression verifies a colliding derived ID leaves the renamed node intact before reusing its old name with a fresh explicit ID. Use the existing seeded UnsecureRandom wrapper for non-security model-based testing. Validation: 6602 passing executions across net10 and net48 selections; final net10 solution build completed with zero warnings and zero errors.
marcschier
added a commit
that referenced
this pull request
Sep 16, 2026
# 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](#4477 (review)) 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.
1 of 7 tasks
romanett
added a commit
that referenced
this pull request
Sep 22, 2026
Full CTT run (23 parts, 3,752 test cases) against origin/master + #4477, - C48/C49: aggregate oracle lacks the Part 13 Uncertain status and Calculated bit rules that #4477 implements for Min/Max(ActualTime) - C14 withdrawn: Auditing Connections pass since the event queue size fix (#4480) - AddNodes BadNodeIdExists after a child rename (#4477 x #4486) - Security None 007.js / Basic256Sha256 005.js fail on master too (CloseSession latency outlasts the idle channel lifetime) - C10 also covers A & C Confirm Test_001.js depending on alarm phase - coverage notes for UAFX, PubSub Publisher UADP and UserDefinedCU - ctt-testing.md section 10: how to run every group in parts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
pushed a commit
that referenced
this pull request
Sep 24, 2026
…l CTT run of the merged CTT PRs (#4502) # Description Findings of a full CTT run (23 parts, 3,752 test cases, CTT 1.05.06 / scripts 1.05.513) against the merge of #4477, #4482, #4485 and #4486, which are all on master now, plus the CTT defects filed in Mantis. No server code changes: the PR changes only the CTT configuration of the reference server sample and the `plans/` documentation. ## CTT configuration: raise `ChannelLifetime` to 120 s CTT Security None `007.js` and Security Basic256Sha256 `005.js` failed (also on master): they keep SecureChannels without a Session idle for about 51 s (60 s token, stretched by the CTT's CloseSession delay, C50) and expect the newest one to close with Good. `TcpTransportListener.DetectInactiveChannels` had already closed it after `ChannelLifetime` (30 s) without traffic (`CloseSecureChannel().Result received BadInvalidState`). `samples/Reference/ConsoleReferenceServer/Ctt.ReferenceServer.Config.xml` now sets `ChannelLifetime` to 120000. The default of 30000 and the server's inactivity cleanup are unchanged. An earlier revision of this PR changed the inactivity cleanup instead, keeping idle Session-less channels open until their token expired (Part 4 §5.6.2.1). That change was withdrawn because it lets an unauthenticated client hold a channel for up to the `SecurityTokenLifetime` (one hour by default) instead of 30 s. The difference to Part 4 §5.6.2.1 is recorded as an open server finding in `plans/ctt-issues.md`. Validated on 2026-09-24 with the reverted code and the new configuration (full run, compared with the run of 2026-09-22 at test-case level): - Security None `007.js` passes and Security Basic256Sha256 `005.js` has only the C50 CloseSession delay warnings (both failed with 30 s). - All other parts have the same error signatures as before; the remaining differences are *"Response Timestamp shows a delay"* warnings from a loaded machine (a back-to-back Session Services run with 30 s and 120 s on an idle machine is identical). - A & C Refresh2 `Err_003.js`/`Err_004.js` and Shelving `initialize.js` report `BadSubscriptionIdInvalid` in today's runs with both 30 s and 120 s on the same build, so they are unrelated to the configuration (both passed again in a run on the idle machine with 120 s). ## Mantis 32 CTT defects filed on 2026-09-24 (11420–11454, script issues in *CTT UA Scripts*, native aggregate oracle issues in *CTT UA Binary*); 12 drafts were already covered by existing reports (including GDS issues 11406–11413 by node-opcua and 11102 for the `opc.https`/`opc.wss` endpoint selection); 5 are held back for further investigation. `plans/ctt-issues.md` links every entry. ## CTT documentation (`plans/ctt-issues.md`, `plans/ctt-testing.md`) - `ctt-issues.md` lists every failing CTT test case with a short abstract and its Mantis issue; the details live in Mantis. It keeps the defects not filed yet, the open questions (U1–U4), the open server findings and the CTT project configuration notes, and no longer lists findings the server has fixed. - Notable CTT findings of this run: the aggregate oracle lacks the Part 13 Uncertain status and Calculated bit rules that #4477 implements (C48/C49, 24 test cases); the CloseSession "delay in excess of 600ms" warnings are a CTT client artifact (C50: the server answers in 2–5 ms, the CTT stops the Session's SessionThread between stamping and sending); the CTT's `FindTokenType` rewrites its cached endpoints, so Security User Name Password 2 `015.js` reports duplicate PolicyIds (C36); A & C Comment passes or skips depending on the date (C39). - Open server finding: AddNodes after a child rename returns `BadNodeIdExists` (#4477 × #4486 interaction, not covered by the CTT). - `ctt-testing.md` is a runbook for agents: setup, headless runs, selection generation, reading and comparing results, triage, recipes per conformance group (A & C, Session/Subscription, Monitored Items, security groups with an isolated PKI) and a full partitioned run of every group (about 80 minutes). It points to `ctt-issues.md` for the failures to expect. ## Related Issues - Follow-up to #4477, #4482, #4485, #4486 ## 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. Validated locally with a full partitioned CTT run of the Release net10.0 reference server (reverted code, `ChannelLifetime` 120000). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Findings from running the CTT conformance groups Monitored Item Services, Node Management Services, Security General and Security User Token against
ConsoleReferenceServer --ctt(CTT 1.05.06, scripts 1.05.513). The security groups ran without-a, against an isolated server PKI built from the CTT'scopyToServerfolders, so the negative certificate tests are meaningful.Server fixes
AddNodes below one parent was quadratic (Node Management Delete Node
Err-002.js)The CTT adds 15,000 variables below
ns=2;s=CTTin batches of 5,000 and warned that AddNodes responses took 400–800 ms. For every added node the server searched all children of the parent twice: once in the BrowseName duplicate check inAsyncCustomNodeManager.AddNodeCoreAsync, which also copied the children list, and once in the NodeVersion lookup ofFilterChangesByNodeVersion.NodeState.FindChildnow searches children lists with ≥ 64 entries through a BrowseName index kept in aConditionalWeakTable, so nodes with few children get no extra field.AddChild/RemoveChild.FindChildinstead of copying the list.In the CTT, AddNodes of 5,000 items now answers in 51–72 ms and the warning is gone.
OpenSecureChannel leaked revocation errors (Security Certificate Validation
002.js)Part 4 §6.1.3, Table 106:
Bad_CertificateRevoked,Bad_CertificateIssuerRevoked),Bad_SecurityChecksFailedshall be reported to the client.Bad_CertificateRevocationUnknown,Bad_CertificateIssuerRevocationUnknown), it should be.TcpServerChannelmasked onlyBad_CertificateRevoked, andStandardServer.OnApplicationCertificateError(CreateSession) had its own list that missedBad_CertificatePolicyCheckFailed. Both now use one helper,CertificateErrorReportingin Opc.Ua.Core, which also inspects the nested errors of the certificate validator (both paths now throw the validation result withThrowIfInvalid). Only validity period, host name, URI and usage errors keep their own code, and audit events still carry the specific result. The CTT warning is gone.Sample data
020.jsskipped becauseScalar_Static_Arrays_ByteStringhad a 1-byte and a 3-byte element. The script checks every element, and the simulation only padded elements 0–2. The seed value now has no element shorter than four bytes, and the simulation pads every element.042.jsgot 18 of 19 notifications. The missing item is the Float matrix: its element [1,1] was-1.03179664E+33, where adding 1 is below Float precision, so the value really did not change. The server did not drop a notification. The element is now-1031.79664.CTT results after the fixes
039.js(C17, missing include),038.jswarning (C32), and 1–2 sporadic timestamp-tolerance warnings that move between test cases from run to run.Err-008.js(Mantis 11266).002.js(C35) and User Name Password 2015.js(C36) fail because of CTT defects. X509: 18/18 automated cases pass.CTT defects documented (
plans/ctt-issues.md)038.jscompares a RevisedSamplingInterval of 0 with a project setting instead of the node's MinimumSamplingInterval or the server's MinSupportedSampleRate.020.jsrequires every ByteString element to be ≥ 4 bytes, but its message says elements 0–2.RequestedNewNodeId()always sendsns=1, regardless ofRequestedNodeId_Namespace, so client NodeIds can't be enabled.opc.wssendpoint, which the CTT cannot open (BadNotSupported).UaEndpointDescription.FindTokenTyperewrites the cached endpoints' token policies, so015.jsreports PolicyIds as reused. The server's GetEndpoints output was verified compliant (PolicyIds unique per configuration, Mantis 11258).plans/ctt-testing.mdgains how to run the Monitored Item/Node Management groups and a recommended setup for the security groups: isolated PKI fromcopyToServer, no-a, PKI restore per part, and waiting forServer started. Other findings recorded there:LoginNameAccessDeniedis not a real user;samples\UAReferenceServer.ctt.xmlneeds no change (enablingRequestedNodeIdbreaks six Add Node cases because of C34).Note:
plans/ctt-testing.mdand the refreshedplans/ctt-issues.mdare introduced by #4482. The plans files here are based on #4482's versions, and new entries start at C32 because #4481 uses C19–C31. Please merge after #4482 (or rebase the plans files onto it).Tests
NodeStateChildNameIndexTests(Opc.Ua.Types.Tests): index semantics (duplicates, remove, rename, replace, re-parent, clone, shrink/grow, 2,000 random operations against a linear search). Also a scan guard: 5,000 misses against 50,000 children take 97 ms with the index and 7,083 ms with it disabled.AsyncCustomNodeManagerNodeManagementTests.AddNodeAsync_DuplicateBrowseNameUnderParentWithManyChildren…TcpServerChannelCertificateErrorTests(Opc.Ua.Core.Tests): masked vs reported certificate errors.WotSpecFixtureManifestTests.EveryEmbeddedExampleMatchesItsRecordedHash. That test hashes WoT assets which areeol=lfin the index but CRLF in this worktree's checkout; no WoT file is touched by this PR.Checklist
🤖 Generated with Claude Code