Skip to content

Fix the Opc.Ua.Client defects reported by the client stack audit - #4471

Merged
marcschier merged 16 commits into
masterfrom
romanett/client-bugs-audit-fb6ef5
Sep 14, 2026
Merged

Fix the Opc.Ua.Client defects reported by the client stack audit#4471
marcschier merged 16 commits into
masterfrom
romanett/client-bugs-audit-fb6ef5

Conversation

@romanett

@romanett romanett commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes the 78 Opc.Ua.Client findings of a read-only bug audit of Opc.Ua.Client / Opc.Ua.Types / Opc.Ua.SourceGeneration taken at a05a360, plus the regressions a subsequent review of those fixes turned up. Every behavioural change was checked against the OPC UA specification, and the citations are in the code comments.

Session, reconnect and failover

  • The reconnect finally re-acquired m_reconnectLock with the caller's token, so a cancelled reconnect left the session stuck in Reconnecting forever and every later reconnect failed. It now uses CancellationToken.None.
  • An in-place session recreate tore down subscriptions it should not have: transferred is all-or-nothing, so a single failed transfer reset and recreated the ones the server had just taken over, orphaning them on the new session. TransferSubscriptions now reports the per-subscription outcome and only the subscriptions the server did not take over are recreated. A token-reuse failover keeps the same server session, so nothing is transferred or reset there at all.
  • In-place recreate only tears down the channel it created itself; a caller-supplied channel or a channel-manager lease is left to its owner.
  • Snapshot(out SessionState) went through the SessionOptions record copy constructor, which drops every member declared on SessionState, so the paired Snapshot/Restore API could not round-trip.
  • The user-token policy override restored the policy URI on failure but had already disposed the ECC ephemeral key the previous identity needs, so the identity the rollback claims to preserve could no longer be reactivated. The key is now parked and restored.
  • SetTriggering batching sliced the remove list past its end when a call had more adds than removes (Part 5 §6.3.11 bounds the sum of both arrays, so an individual list may be shorter than the limit).
  • The certificate-less None attempt dropped the request header, and with it the ECDH ephemeral key that travels in additionalHeader (Part 4 §7.1/§7.15).
  • Session.Equals compared a ConfiguredEndpoint against an EndpointDescription, so a session did not equal a session on the same endpoint.

Managed session, pool and connection state machine

  • ManagedSession.Dispose() blocked on the connection state machine's worker, which raises ConnectionStateChanged synchronously — disposing from that handler deadlocked permanently. The machine now recognises its own flow and completes the teardown as the worker's continuation.
  • ManagedSessionFactory dropped updateBeforeConnect, connection and reverseConnectManager on all four CreateAsync overloads.
  • The session pool ran its shared connect on no token at all, so once the only caller gave up nothing could abort it and RemoveAsync/DisposeAsync waited out the whole reconnect budget. Each entry now owns the token that aborts its connect, and removal closes and disposes the session.
  • An explicit engineFactory was silently ignored for DefaultSessionFactory subclasses, which quietly downgraded the session to the classic subscription engine.

Subscriptions

  • Fixed a lock-order inversion between m_subscriptionLock and the partition locks in CompleteAsync and DisposeAsync.
  • OwnsSubscriptionId stopped at the first registered subscription carrying the id — but in the very case it guards, the asker still holds its own retired id, so the answer depended on HashSet enumeration order and the delete could still tear down a sibling that had been handed the recycled id.
  • MessageProcessor's republish gap walk was unbounded; it is now bounded by the server's retransmission queue (Part 4 §5.14.5).
  • A publish acknowledgement is sent even when a notification handler throws — the ack means "the server may delete the message" (Part 4 §5.14.5.2), not "I processed it".
  • Loaded monitored items did not raise the process-wide client-handle counter, so a fresh process minted handles that collided with the loaded ones; a transfer-restored item was also stamped BadOperationAbandoned and left at the default monitoring mode.
  • AutoSetQueueSize was not idempotent, so the queue size ratcheted up on every subscription state change.
  • The snapshot dropped the partition and recovery options entirely, and clamped the sampling interval and the secondary-partition idle timeout to zero — turning the Part 4 §7.21 "use the publishing interval" sentinel and "never delete an idle partition" into something else.
  • The state stream is positional OPC UA binary and the reader cannot detect an absent field, so appending the partition options would have mis-decoded every previously saved file. The stream now carries a format version and the first format is decoded through a frozen copy of its shape.

Browse, node cache and the remaining clients

  • Managed browse wrote a retry pass's status into whatever node happened to sit at that offset in the caller's list, and retried continuation-point errors without any bound. It now keeps the caller's slot and gives up on passes that make no progress — a fixed pass cap would cut short a large browse against a conforming server with a small continuation-point quota.
  • Continuation points are released when a consumer stops enumerating early, as Part 4 §5.9.3.2 requires.
  • The node cache reordered its batched results relative to the inputs, which mispairs them for every consumer that matches positionally; its supertype walks could also spin forever on a server whose HasSubtype chain loops back on itself.
  • A browse error was cached as "this node has no references"; it is now surfaced to the direct caller, while the bulk and hierarchy walks treat the node as having no references for that one operation without caching it, so a single un-browsable node cannot fail a whole batch.
  • WebApiClient mutated a caller-supplied (typically IHttpClientFactory-pooled) HttpClient, which throws once that client has sent its first request; those instances are configured per request instead.
  • PathCache was not thread-safe.

Review follow-ups

The last two commits are the result of reviewing the audit fixes themselves. Worth calling out: the Task.WaitAsync(CancellationToken) polyfill in Opc.Ua.Types never checked IsCompleted, so an already-finished task plus an already-cancelled token reported cancellation on every pre-net8.0 target where the BCL overload returns the result. It is now aligned with the BCL and pinned by a test.

Related Issues

  • No tracking issue; this PR is the result of a read-only audit of the client stack.

Checklist

  • I have signed the CLA and read the CONTRIBUTING doc.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added all necessary documentation.
  • I have verified that my changes do not introduce (new) build or analyzer warnings.
  • I ran all tests locally using the UA.slnx solution against at least .net framework and .net 10, and all passed.
  • I fixed all failing and flaky tests in the CI pipelines and all CodeQL warnings.
  • I have addressed all PR feedback received.

Verification

35 regression tests were added under tests/Opc.Ua.Client.Tests/AuditRegressions/ (category AuditRegression), one per fixed defect where the defect is observable at unit level; most were mutation-checked by reintroducing the bug and confirming the test fails.

Locally on net10.0: Opc.Ua.Client.Tests 2252/2252, Opc.Ua.Client.ComplexTypes.Tests 2773/2773, Opc.Ua.Bindings.Https.WebApi.Tests 346/346, and all 23 library and test projects that reference Opc.Ua.Client build clean. Opc.Ua.Client also builds clean for netstandard2.1, the target that actually compiles the WaitAsync polyfill. The full UA.slnx solution and the .NET Framework targets have not been run locally — left to CI.

One pre-existing failure in Opc.Ua.Types.Tests is unrelated to this branch: WotSpecFixtureManifestTests.EveryEmbeddedExampleMatchesItsRecordedHash (embedded WoT fixture hashes) fails identically with these changes reverted.

Notes for reviewers

  • Two review findings were deliberately left unfixed and are documented in the code: ManagedSession.CreateAsync has to write the engine choice onto a caller-supplied DefaultSessionFactory subclass (the only alternative is changing the ISessionFactory.Create contract), and the connection state machine's "am I on my own worker" flag is an AsyncLocal, so it also flows into work a StateChanged handler spawns. The previous behaviour in that second case was a hard deadlock.
  • Session.Dispose()'s single GetAwaiter().GetResult() bridge to CloseAsync is left as is: Dispose(bool) must be synchronous and DisposeAsync already exists.

🤖 Generated with Claude Code

romanett and others added 6 commits September 11, 2026 06:16
@romanett @claude
Fix client session, managed session and subscription defects from the…
2a7aba5 
… audit

Session: keep the reconnect finally on CancellationToken.None so a cancelled
caller no longer leaves the session permanently Reconnecting; clamp the
batched SetTriggering remove slice; only close a self-created channel on an
in-place recreate; transfer/recreate V1 subscriptions after an in-place
recreate; round-trip Snapshot/Restore(SessionState); send the ECDH request
header on the certificate-less SecurityPolicy None attempt; restore the user
token policy when an identity update is rejected; compare configured
endpoints in Equals; dispose replaced ECC ephemeral keys; drop the duplicate
keep-alive timer and the shared default subscription in the template ctor;
clear m_inKeepAliveCallback in a finally; report the real per-node error from
ReadValuesAsync(expectedTypes).

ManagedSession: retract a published inner session whose setup failed;
forward reverseConnectManager/connection/updateBeforeConnect through
ManagedSessionFactory; rotate network endpoints from the endpoint in use;
re-subscribe to channel state after an in-place recreate; close and dispose
pooled sessions and stop binding pool callers to the first caller's token;
give the clean close a grace period before the worker token is cancelled;
stop replacing DefaultSessionFactory subclasses and route the channel
manager path through the factory; follow the provider a caller installed with
UpdateIdentityAsync; run the full teardown from the sync Dispose.

Subscription: break the partition/registry lock-order inversion that could
deadlock the publish pipeline; raise the global client handle counter for
loaded and transferred items; treat queued triggering ops as pending changes;
make AutoSetQueueSize idempotent; guard m_deletedItems with the manager lock;
bound the sequence-number gap walk by the retransmission queue; forward the
publish response string table to the notification handlers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett @claude
Fix subscription manager, node cache and browser defects from the audit
928f74a 
Subscription manager/classic subscription: catch the TransferSubscriptions
fault so a server without the service no longer aborts every session
recreate; recreate secondary partitions restored without transfer; consume
the BadTooManyPublishRequests flag so the worker cap stops collapsing to 1;
unregister a zombie subscription when a restore transfer throws; do not block
on the message worker when Dispose runs inside a notification callback;
complete queued SetTriggering awaiters on dispose; recreate on
BadSubscriptionIdInvalid instead of retrying the dead id; skip deleting a
stale id a sibling already owns; round-trip the partition and recovery
options through the snapshot; stop back-filling placeholders before the
sequence number is resynchronised; roll back collected acks on cancellation;
always signal the initial create pass; replay the Opened callback through the
logical wrapper; observe faults from fire-and-forget state callbacks and
guard the forwarding semaphore; clamp a sub-millisecond publishing interval.

Node cache/browser/complex types: keep retry-pass browse errors in the
caller's slot and bound the retry passes; throw instead of caching "no
references" for a failed browse; propagate cancellation out of the single
node browse; preserve input order in the batched GetReferencesAsync and pair
dictionary namespaces by owner; warm super-type chains so the filter never
blocks on the network; add cycle protection to every type-hierarchy walk;
forward the request header in NodeCacheContext reads; dispose the NodeCache a
NodeCacheResolver creates; keep the model change pump alive past the start
token and clear IsTracking when it dies; use the configuration the discovery
provider returns.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett @claude
Fix the file system, alias name, user management and web API client d…
c245332 
…efects

WebApi: stop disposing a caller-supplied HttpMessageHandler by default, so a
handler shared between channels survives the first channel closing; keep the
factory-pooled HttpClient untouched and carry the base address, auth header
and timeout per request instead (writing them on a client that already sent a
request throws); size the WSS response limit by MaxMessageSize rather than
the 64 KiB buffer size.

FileSystem: only mark a temporary write file terminated once the server
accepted the commit, so a failed commit can still be retried or closed;
release the directory-enumeration continuation point when the consumer stops
early; reject a server read larger than the requested window; make PathCache
thread-safe, since FileSystemClient mutates it concurrently.

AliasNames: keep an Invalidate that races an in-flight refresh; release the
sub-category continuation point on early exit; tear down the subscription the
monitored-item refresh strategy created when its start fails.

UserManagement: treat BadNoMatch/BadNodeIdUnknown as an absent optional
PasswordRestrictions property, which is what the browse path actually returns.

Adds 26 regression tests under tests/Opc.Ua.Client.Tests/AuditRegressions
covering the fixes across all six client areas; 19 of them were confirmed to
fail when the corresponding defect is reintroduced.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett @claude
Correct and cite the spec basis for the client audit fixes
34f3201 
Checked the behavioural changes against the OPC UA specification. One comment
was wrong: the classic in-place recreate claimed the server discards the
previous session's subscriptions. Part 4 section 5.7.2.1 says the opposite -
a server that terminates a session for any reason other than
CloseSession(deleteSubscriptions) keeps them alive until their lifetime
expires so they can be reassigned. The code is unchanged (the client can no
longer reach them either way, section 5.14.1.3), but the reasoning now says
so honestly: the orphans expire on their own.

The remaining comments gain the reference that backs them: the negative
sampling interval sentinel (4/7.21), the SetTriggering operation limit
bounding the sum of both link arrays (5/6.3.11), releasing continuation
points a client stops following (4/5.9.3.2 and 4/7.9), acknowledgements
being a "may drop" signal rather than a processing claim (4/5.14.5.2),
server-wide subscription id uniqueness behind the id-recycling guard
(4/5.14.2.2), Bad_NoMatch for an unresolvable browse path (4/5.9.4.4),
PasswordRestrictions being Optional (18/5.2.1), and the ECDH key travelling
in the request additionalHeader which a non-ECC server ignores
(4/7.1, 4/7.15, 4/7.32).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett @claude
Fix the regressions the review of the client audit fixes found
bbc1e4f 
The in-place session recreate reset every classic subscription whenever
a single transfer failed and also on a reused server session; the
transfer now reports which subscriptions the server did not take over
and only those are recreated. The identity override rollback restored
the token policy but had already disposed the ECC ephemeral key the
previous identity needs; the key is parked and restored instead.

ManagedSession.Dispose() blocked on the connection state machine's
worker, which raises ConnectionStateChanged synchronously, so disposing
from that handler deadlocked; the machine now detects its own flow and
finishes the teardown as the worker's continuation. The session pool
connect runs on a per-entry token again so RemoveAsync and DisposeAsync
can abort it. An explicit engine factory is honoured for
DefaultSessionFactory subclasses.

The node cache bulk reads and type-chain walks tolerate a node the
server refuses to browse (logged, not cached) instead of failing the
whole operation; the browse path supertype climb got the cycle guard
the other walkers received; and the managed browse gives up on passes
that make no progress rather than after a fixed count, which cut short
large browses against a conforming server.

OwnsSubscriptionId now walks every registered subscription so the
asker's own stale id cannot mask a sibling. The snapshot keeps the
infinite secondary partition idle timeout, and the subscription state
stream carries a format version with the first format decoded through
a frozen copy of its shape. The forwarding handler's replay observes
handler faults and holds the serialisation gate.

Cleanups: the hand-rolled cancellation awaits use Task.WaitAsync, the
three recreate-and-rebind blocks share a helper, the dead per-pass error
lists are gone, the continuation point release lives in one extension,
the classic publish worker sets its dispatch flag once, and the web API
client parses its header values once.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@romanett @claude
Harden the pool teardown and the WaitAsync polyfill
a62006c 
Removing a pooled entry waited on the connect with the caller's token, so
a cancelled token abandoned the wait after the key was already gone from
the pool and left a connected session with nobody to close it. The wait
now runs on the entry's own abort instead, and an entry whose connect was
never started is torn down without starting one.

The Opc.Ua.Types WaitAsync polyfill, used on every pre-net8.0 target,
never checked IsCompleted, so an already finished task plus an already
cancelled token reported cancellation where the BCL overload returns the
result. Three call sites added by the previous commit go through it, so
align it with the BCL and pin the semantics with a test.

The node cache's lenient reference fetch no longer reports "no
references" for a failure caused by the caller's own cancellation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Code coverage

✅ Coverage gate passed.

Check Result Threshold
✅ Project line rate 86.93% (262658/302162 lines) >= 70.00%
✅ Project branch rate 76.96% >= 60.00%
✅ Patch coverage 77.97% (938/1203 changed lines) >= 75.00% (> 100 changed lines)
ℹ️ Baseline delta (advisory) +13.33 pp 73.60% recorded
Uncovered changed lines
  • src/Opc.Ua.Client/Session/Session.cs: 1821, 1825, 1841, 1842, 1849, 1850, 1851, 1852, 1853, 2332, 2370, 2379, 2403, 2405, 2407, 2409, 2433, 2435, 2437, 2439, 2441, 3940, 3941, 3944, 3945, 3948, 3973, 3975, 3980, 3993, 3994, 3997
  • src/Opc.Ua.Client/WebApi/WebApiWssTransportChannel.cs: 539, 540
  • src/Opc.Ua.Client/Session/ManagedSession.cs: 1011, 1012, 1014, 1015, 1085, 1086, 1087, 1088, 1089, 1090, 1091, 1092, 1093, 1094, 1095, 1099, 1100, 1193, 1537, 1539, 1540
  • src/Opc.Ua.Client/Fluent/OpcUaClientBuilderExtensions.cs: 1018
  • src/Opc.Ua.Client/Subscription/CompositeMonitoredItemCollection.cs: 681, 683, 699, 701, 703, 706
  • src/Opc.Ua.Client/Subscription/Subscription.cs: 473, 474, 475, 934, 935, 936, 937, 946, 947, 948
  • src/Opc.Ua.Client/WebApi/WebApiTransportChannel.cs: 509
  • src/Opc.Ua.Client/Session/Reconnect/ConnectionStateMachine.cs: 364, 366, 367, 368, 369
  • src/Opc.Ua.Client.ComplexTypes/ComplexTypeSystemFactory.cs: 78, 79
  • src/Opc.Ua.Client/ModelChange/ModelChangeTracker.cs: 154, 155, 182
  • src/Opc.Ua.Client/ReverseConnectManager.cs: 1998, 2004, 2005, 2006, 2007
  • src/Opc.Ua.Client/ComplexTypes/DefaultComplexTypeSystemFactory.cs: 57, 58, 59, 60, 61
  • src/Opc.Ua.Client/NodeCache/NodeCache.cs: 404, 540, 922, 923
  • src/Opc.Ua.Client/UserManagement/UserManagementClient.cs: 257, 258, 259, 260, 261, 262, 263, 264
  • src/Opc.Ua.Client/Session/SessionClientExtensions.cs: 167
  • src/Opc.Ua.Client/Subscription/Classic/Subscription.cs: 407, 408, 409, 410, 1086, 1087, 1088, 1089
  • src/Opc.Ua.Client/Subscription/SubscriptionManager.cs: 296, 391, 980, 981, 982, 983, 984, 991, 993, 997, 998, 999, 1019, 1021, 1022, 1025, 1033, 1034, 1035, 1038, 1041, 1045, 1176, 1189, 1192, 1193, 1194, 1195, 1196, 1197, 1198, 1199, 1200, 1202, 1203, 1204, 1205, 1206, 1209, 1211, 1212, 1213, 1215, 1435, 1443, 1445, 1446, 1450, 1452, 1453, 1605, 1606, 1611
  • src/Opc.Ua.Client/Browser.cs: 365, 366, 368
  • src/Opc.Ua.Client/Session/ManagedSessionFactory.cs: 115, 116, 117, 141, 142, 143, 168, 169, 170, 195, 196, 197, 288, 289, 290
  • src/Opc.Ua.Client/Subscription/PartitionForwardingHandler.cs: 115, 116, 165, 166, 169, 174, 175, 349, 355, 358
  • src/Opc.Ua.Client/Subscription/SubscriptionManagerSerializer.cs: 188, 189, 190, 191, 192, 193
  • src/Opc.Ua.Client/ComplexTypes/NodeCacheResolver.cs: 70, 87, 88, 93, 94, 95, 106, 108, 168, 172
  • src/Opc.Ua.Client/Session/ManagedSessionPool.cs: 125, 126, 140, 141, 142, 143, 144, 145, 146, 147, 148, 149, 150, 151, 152, 153, 154, 155, 156, 157, 187, 285
  • src/Opc.Ua.Client/FileSystem/FileSystemClient.cs: 564, 591, 592, 593, 594, 595
  • src/Opc.Ua.Client/WebApi/WebApiClient.cs: 111, 113, 114, 312, 314, 316
  • src/Opc.Ua.Client/Fluent/ManagedSessionBuilder.cs: 825, 826
  • src/Opc.Ua.Client/AliasNames/Refresh/MonitoredItemAliasNameRefreshStrategy.cs: 135, 140, 142, 145, 146, 148, 151, 153, 159, 160, 161, 162, 163, 164, 166, 169, 172, 175

Coverage is above the recorded baseline - consider ratcheting coverage-thresholds.json.

Thresholds live in coverage-thresholds.json. Whole report before exclusions: line 86.07%, branch 76.15%.

@codecov

codecov Bot commented Sep 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 73.56608% with 318 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.97%. Comparing base (a05a360) to head (69b1c93).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
.../Opc.Ua.Client/Subscription/SubscriptionManager.cs 50.80% 53 Missing and 8 partials ⚠️
src/Opc.Ua.Client/Session/Session.cs 60.00% 30 Missing and 8 partials ⚠️
src/Opc.Ua.Client/Session/ManagedSession.cs 72.64% 21 Missing and 8 partials ⚠️
src/Opc.Ua.Client/Session/ManagedSessionPool.cs 65.78% 22 Missing and 4 partials ⚠️
...s/Refresh/MonitoredItemAliasNameRefreshStrategy.cs 57.14% 18 Missing ⚠️
src/Opc.Ua.Client/Session/ManagedSessionFactory.cs 25.00% 15 Missing ⚠️
....Client/Subscription/PartitionForwardingHandler.cs 74.13% 10 Missing and 5 partials ⚠️
src/Opc.Ua.Client/Subscription/Subscription.cs 76.92% 10 Missing and 2 partials ⚠️
...rc/Opc.Ua.Client/ComplexTypes/NodeCacheResolver.cs 73.68% 10 Missing ⚠️
src/Opc.Ua.Client/WebApi/WebApiClient.cs 73.68% 5 Missing and 5 partials ⚠️
... and 20 more

❌ Your patch check has failed because the patch coverage (73.56%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4471      +/-   ##
==========================================
- Coverage   80.98%   80.97%   -0.01%     
==========================================
  Files        2050     2052       +2     
  Lines      300775   302162    +1387     
  Branches    52050    52337     +287     
==========================================
+ Hits       243592   244690    +1098     
- Misses      39204    39346     +142     
- Partials    17979    18126     +147
Flag Coverage Δ
actions 80.97% <73.56%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/Opc.Ua.Client/AliasNames/AliasNameClient.cs 84.25% <100.00%> (+0.22%) ⬆️
src/Opc.Ua.Client/AliasNames/AliasNameResolver.cs 83.94% <100.00%> (+0.35%) ⬆️
.../ComplexTypes/ComplexTypeSystemClientExtensions.cs 100.00% <100.00%> (ø)
...c/Opc.Ua.Client/Discovery/OpcUaDiscoveryService.cs 100.00% <100.00%> (ø)
src/Opc.Ua.Client/FileSystem/UaFileStream.cs 73.84% <100.00%> (+0.83%) ⬆️
...c/Opc.Ua.Client/FileSystem/UaTemporaryWriteFile.cs 56.79% <100.00%> (-0.53%) ⬇️
src/Opc.Ua.Client/NodeCache/NodeCacheContext.cs 82.60% <100.00%> (+0.09%) ⬆️
src/Opc.Ua.Client/NodeCache/NodeCacheExtensions.cs 95.16% <100.00%> (+0.20%) ⬆️
src/Opc.Ua.Client/Session/DefaultSessionFactory.cs 92.11% <100.00%> (+0.37%) ⬆️
src/Opc.Ua.Client/Session/SessionClientBatched.cs 99.04% <100.00%> (+<0.01%) ⬆️
... and 38 more

... and 59 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@romanett
romanett requested a balanced review from Copilot September 11, 2026 13:21
Copilot AI reviewed Sep 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Several remaining lifecycle and concurrency races can lose pool entries, recreate live subscriptions, expose stale aliases, or leak resources.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes audited client-stack defects across sessions, subscriptions, browsing, caching, Web API, and file handling.

Changes:

  • Corrects reconnect, failover, subscription transfer, and disposal behavior.
  • Hardens browsing, node caching, continuation-point handling, and client utilities.
  • Adds regression coverage and versioned subscription snapshots.
File summaries
File Description
tests/Opc.Ua.Types.Tests/Utils/TaskWaitAsyncTests.cs Tests polyfill cancellation semantics.
tests/Opc.Ua.Client.Tests/Subscription/Fakes/FakeMessageAckQueue.cs Adds ownership-test hooks.
tests/Opc.Ua.Client.Tests/Subscription/Classic/ClassicMonitoredItemCoverageTests.cs Updates clone-handle assertions.
tests/Opc.Ua.Client.Tests/AuditRegressions/SessionAuditRegressionTests.cs Covers session and pool regressions.
tests/Opc.Ua.Client.Tests/AuditRegressions/ClientServicesAuditRegressionTests.cs Covers cache, file, HTTP, and reverse-connect fixes.
tests/Opc.Ua.Client.Tests/AuditRegressions/BrowserAuditRegressionTests.cs Covers browse retry defects.
src/Opc.Ua.Types/Polyfills/System.Threading.Tasks.cs Aligns WaitAsync with BCL behavior.
src/Opc.Ua.Client/WebApi/WebApiWssTransportChannel.cs Uses message-size response limits.
src/Opc.Ua.Client/WebApi/WebApiTransportChannel.cs Avoids mutating factory clients.
src/Opc.Ua.Client/WebApi/WebApiClientOptions.cs Clarifies handler ownership.
src/Opc.Ua.Client/WebApi/WebApiClient.cs Applies shared-client settings per request.
src/Opc.Ua.Client/UserManagement/UserManagementClient.cs Handles optional-property lookup failures.
src/Opc.Ua.Client/Subscription/SubscriptionStateSnapshotV0.cs Defines the legacy snapshot shape.
src/Opc.Ua.Client/Subscription/SubscriptionStateSnapshot.cs Persists partition options.
src/Opc.Ua.Client/Subscription/SubscriptionManagerSerializer.cs Versions snapshot streams.
src/Opc.Ua.Client/Subscription/SubscriptionManager.cs Fixes locking, transfer, restore, and acknowledgements.
src/Opc.Ua.Client/Subscription/Subscription.cs Improves recreation and callback handling.
src/Opc.Ua.Client/Subscription/PartitionForwardingHandler.cs Buffers and serializes state callbacks.
src/Opc.Ua.Client/Subscription/MonitoredItemStateSnapshot.cs Preserves negative sampling intervals.
src/Opc.Ua.Client/Subscription/MonitoredItemManager.cs Completes pending operations and protects state.
src/Opc.Ua.Client/Subscription/MonitoredItem.cs Restores handles and loaded state correctly.
src/Opc.Ua.Client/Subscription/MessageProcessor.cs Bounds republish and preserves response string tables.
src/Opc.Ua.Client/Subscription/IMessageAckQueue.cs Adds subscription-ID ownership checks.
src/Opc.Ua.Client/Subscription/CompositeMonitoredItemCollection.cs Rebinds and removes restored items.
src/Opc.Ua.Client/Subscription/Classic/Subscription.cs Adjusts callback disposal and recreate state.
src/Opc.Ua.Client/Subscription/Classic/MonitoredItem.cs Fixes clone display handles.
src/Opc.Ua.Client/Session/SessionClientExtensions.cs Adds continuation-point release support.
src/Opc.Ua.Client/Session/SessionClientBatched.cs Corrects triggering batch slicing.
src/Opc.Ua.Client/Session/Session.cs Fixes snapshots, identity updates, transfer, and reconnect.
src/Opc.Ua.Client/Session/Reconnect/ConnectionStateMachine.cs Prevents disposal deadlocks.
src/Opc.Ua.Client/Session/ManagedSessionPool.cs Adds pool-owned connect cancellation.
src/Opc.Ua.Client/Session/ManagedSessionFactory.cs Forwards connection parameters.
src/Opc.Ua.Client/Session/ManagedSession.cs Repairs creation, failover, identity, and disposal paths.
src/Opc.Ua.Client/Session/DefaultSessionFactory.cs Allows engine-factory assignment.
src/Opc.Ua.Client/Session/ChannelManagerSessionFactory.cs Propagates the subscription engine.
src/Opc.Ua.Client/ReverseConnectManager.cs Adds unique registration IDs and cancellable waits.
src/Opc.Ua.Client/NodeCache/NodeCacheExtensions.cs Terminates cyclic hierarchy walks.
src/Opc.Ua.Client/NodeCache/NodeCacheContext.cs Propagates headers and browse errors.
src/Opc.Ua.Client/NodeCache/NodeCache.cs Preserves ordering and hardens hierarchy traversal.
src/Opc.Ua.Client/ModelChange/ModelChangeTracker.cs Separates startup cancellation from pump lifetime.
src/Opc.Ua.Client/Fluent/ManagedSessionBuilder.cs Disposes one-shot cache resolvers.
src/Opc.Ua.Client/FileSystem/UaTemporaryWriteFile.cs Keeps failed commits retryable.
src/Opc.Ua.Client/FileSystem/UaFileStream.cs Rejects oversized read responses.
src/Opc.Ua.Client/FileSystem/PathCache.cs Makes path caching thread-safe.
src/Opc.Ua.Client/FileSystem/FileSystemClient.cs Releases abandoned continuation points.
src/Opc.Ua.Client/Discovery/OpcUaDiscoveryService.cs Uses provider-returned configuration.
src/Opc.Ua.Client/ComplexTypes/NodeCacheResolver.cs Adds ownership disposal and correct namespace mapping.
src/Opc.Ua.Client/Browser.cs Corrects retry indexing and termination.
src/Opc.Ua.Client/AliasNames/Refresh/MonitoredItemAliasNameRefreshStrategy.cs Cleans up failed starts.
src/Opc.Ua.Client/AliasNames/AliasNameResolver.cs Tracks concurrent invalidation generations.
src/Opc.Ua.Client/AliasNames/AliasNameClient.cs Releases abandoned browse points.
Review details
  • Files reviewed: 52/52 changed files
  • Comments generated: 7
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Opc.Ua.Client/Session/ManagedSessionPool.cs
Comment thread src/Opc.Ua.Client/AliasNames/AliasNameResolver.cs Outdated
Comment thread src/Opc.Ua.Client/FileSystem/UaTemporaryWriteFile.cs Outdated
Comment thread src/Opc.Ua.Client/ModelChange/ModelChangeTracker.cs
Comment thread src/Opc.Ua.Client/NodeCache/NodeCache.cs Outdated
Comment thread src/Opc.Ua.Client/Session/Session.cs Outdated
Comment thread src/Opc.Ua.Client/WebApi/WebApiWssTransportChannel.cs Outdated
romanett and others added 2 commits September 11, 2026 15:37
@romanett @claude
Let the managed browse ride out a continuation point starvation run
d296397 
The no-progress give-up bound was three consecutive passes, which is a
throttle rather than the last-resort breaker it is meant to be: two
browses competing on one session for a small continuation point quota
legitimately complete nothing for several passes in a row while the
other one holds the quota, and cutting the browse short there returns a
truncated reference list. That is what
ParallelManagedBrowseWithManyContinuationPointsAsync(2:10, Default) saw
on the macOS CI leg - 10 references instead of 100.

Raise the bound to 100 consecutive passes without a completed node and
say in the comment why it must not be tightened again. A pass that
completes anything still resets the count, so a server that answers
every single retry with another continuation point error is still
bounded.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett @claude
Address the lifecycle and concurrency findings from the PR review
813384d 
A pooled connect that fails evicted its entry by key alone, so a
replacement installed under the same key while the old connect was still
unwinding was thrown away with it. It now removes the exact key/value
pair.

The alias name cache marked itself loaded through a separate flag that an
Invalidate landing between the generation check and the write could no
longer clear, pinning pre-invalidation data as current. The loaded
generation is now the flag, so the publish is a single atomic write and
Invalidate a single increment.

UaTemporaryWriteFile disposed its gate as part of terminating, which made
a concurrent commit or dispose that was already waiting release a
disposed semaphore. The gate now lives as long as the object.

ModelChangeTracker.StopTrackingAsync returned early once IsTracking was
false, so a pump that ended on its own left its token source and task
behind for the next start to overwrite. It now cleans up whenever those
fields are present.

The synchronous type-of walk bounded itself by depth alone and reported
any hierarchy deeper than 64 levels as a cycle. Past that depth it now
tracks visited nodes, so only a repeated NodeId ends the walk while the
common case stays allocation free.

A subscription the server reports as already belonging to this session is
added to the transferred set, so a throw later in the result loop cannot
sweep it into notTransferred and have it reset and recreated.

The WebApi WSS channel treated MaxMessageSize <= 0 as "fall back to
MaxBufferSize" where the Web API codec's convention is "no cap", which
kept rejecting fragmented responses larger than one transport buffer.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett
romanett requested a balanced review from Copilot September 11, 2026 14:45
Copilot AI reviewed Sep 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Several lifecycle, concurrency, error-propagation, and caller-owned HttpClient defects remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

Previously missed (4) — in code that hasn't changed since the last review.

src/Opc.Ua.Client/AliasNames/Refresh/MonitoredItemAliasNameRefreshStrategy.cs:129

  • The cleanup scope starts after the owned subscription has already been added to the session and CreateAsync has completed. If CreateAsync throws, this catch never runs, leaving the half-built subscription registered; repeated StartAsync calls then accumulate exactly the subscriptions this cleanup intends to prevent. Include construction, registration, and creation in the protected scope, and mark ownership before the first failing await.
    src/Opc.Ua.Client/Browser.cs:367
  • The release call can fail when cancellation was caused by a broken session/channel, and that cleanup exception then replaces the original OperationCanceledException before this throw is reached. Use the new best-effort ReleaseContinuationPointAsync helper here so cancellation remains observable even when releasing the point is impossible.
    src/Opc.Ua.Client/Session/ManagedSession.cs:1032
  • The old refresh loop is cancelled only by StartIdentityRefreshLoop, after this method has released m_serviceLock. In that gap the old loop can acquire the writer lock and reactivate the previous identity after the explicit update succeeded; replacing the provider afterward does not undo that stale activation. Rotate/cancel the old loop while the writer lock still excludes it, then observe/dispose the old loop before completing the switch.
    src/Opc.Ua.Client/WebApi/WebApiClient.cs:93
  • The public caller-owned HttpClient path still opts into constructor mutation. If that client has already sent a request and RequestTimeout is supplied, assigning HttpClient.Timeout below throws InvalidOperationException, so direct users of this public constructor retain the audited defect. Apply authorization, Accept, and timeout per request for every caller-owned client, not only the internal factory overload.

This issue also appears on line 111 of the same file.

src/Opc.Ua.Client/ModelChange/ModelChangeTracker.cs:130

  • A pump that ends or faults sets IsTracking to false but leaves m_cts and m_pumpTask populated. A subsequent StartTrackingAsync takes this branch and overwrites both fields without disposing the old token source, so every restart after an independently terminated pump leaks its previous state. Clean up the completed epoch before installing the replacement, or have PumpAsync clear and dispose its own epoch's fields in finally.
                    m_cts = new CancellationTokenSource();
                    // Capture the token before launching the pump task so a racing
                    // StopTrackingAsync (which nulls m_cts) cannot NRE the lambda.
                    CancellationToken pumpToken = m_cts.Token;
                    long epoch = ++m_trackingEpoch;
                    m_startReadyTask = ready.Task;
                    m_pumpTask = Task.Run(
                        () => PumpAsync(ready, epoch, pumpToken),
                        CancellationToken.None);

src/Opc.Ua.Client/WebApi/WebApiClient.cs:113

  • This constructor stores the explicit base URI in m_baseAddress, but the public BaseAddress property still reads only m_httpClient.BaseAddress. Factory clients are not required to have that property configured, so the WebApiTransportChannel path can send requests successfully while IWebApiClient.BaseAddress throws, violating its contract. Make the property return m_baseAddress first.
  • Files reviewed: 52/52 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/Opc.Ua.Client/NodeCache/NodeCache.cs Outdated
Comment thread src/Opc.Ua.Client/Subscription/PartitionForwardingHandler.cs Outdated
@romanett @claude
Narrow the absorbed browse failures and order the bind replay properly
735bb74 
The node cache's lenient reference fetch caught every
ServiceResultException, so a dead session or a timeout became "this node
has no references" and the caller got a silently incomplete result for
the whole batch. It now absorbs only the statuses Browse reports per
node and lets service-wide failures out.

Publishing the logical subscription before taking the serialisation gate
did not order the bind replay: a live state change could observe the new
wrapper the moment the bind lock was released and reach the user handler
first. The gate is now taken before the publish and both happen under
the bind lock; Wait(0) never blocks, so holding the lock across it is
safe.

The browse cancellation path released its continuation point through the
validating BrowseNext, so a release that failed - likely, since the
cancellation is often the session or channel dying - replaced the
OperationCanceledException the caller was waiting for. It uses the
best-effort release helper now.

The alias refresh strategy registered and created its own subscription
outside the cleanup scope, so a throw from CreateAsync left it on the
session and every retried StartAsync added another. Construction,
registration and creation are inside the scope, with ownership claimed
before the first await that can fail.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett
romanett requested a balanced review from Copilot September 11, 2026 15:25
Copilot AI reviewed Sep 11, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved concurrency races and Web API contract regressions can still produce stale state, incorrect identities, or failed endpoint access.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

src/Opc.Ua.Client/ModelChange/ModelChangeTracker.cs:147

  • The cleanup after this wait is not tied to the pump generation it started. A failing pump completes ready, clears IsTracking in finally, and a concurrent caller can install a replacement before this owner's catch runs; StopTrackingAsync then cancels and clears the replacement pump rather than the failed one. Carry the epoch into owner cleanup and only detach/cancel fields that still belong to that epoch.
                // The caller's token bounds this wait; the catch below stops
                // the pump it started when the wait is abandoned.
                await readyTask.WaitAsync(ct).ConfigureAwait(false);
            }
            catch
            {
                if (ownsStart)
                {
                    await StopTrackingAsync(CancellationToken.None).ConfigureAwait(false);

src/Opc.Ua.Client/AliasNames/AliasNameResolver.cs:195

  • An older refresh can still overwrite a newer generation's dictionaries. For example, refresh A captures generation 0, invalidation starts refresh B at generation 1, B publishes first, and then A acquires this semaphore and replaces the cache with generation-0 data. A reader that already observed B as loaded can then consume A's stale dictionaries. Skip publication when the captured generation no longer equals the current invalidation generation.
    src/Opc.Ua.Client/ReverseConnectManager.cs:2016
  • Cancellation now loses the caller's token because TrySetCanceled() creates a canceled task with the default token; the previous path used TrySetCanceled(ct). Code that catches OperationCanceledException and checks CancellationToken can no longer identify which operation was canceled. Preserve ct when completing the source.
    src/Opc.Ua.Client/Session/ManagedSession.cs:1032
  • Switching the provider after releasing m_serviceLock leaves a race with the old refresh loop. If that loop is already waiting for the writer lock, it can acquire it in this window and reactivate the previous identity once more before StartIdentityRefreshLoop cancels it, undoing the update this method just completed. Publish the provider change while still holding the writer lock and have refresh attempts verify their captured provider after acquiring that lock before updating the session.
    src/Opc.Ua.Client/WebApi/WebApiClient.cs:113
  • This constructor keeps the effective endpoint only in m_baseAddress, but the IWebApiClient.BaseAddress getter still reads only m_httpClient.BaseAddress. DefaultOpcUaHttpClientFactory.Shared returns an unconfigured cached client (OpcUaHttpClientFactory.cs:116-120), so a channel-created client can send requests successfully yet throw when consumers read its required BaseAddress property. Make the getter return m_baseAddress ?? m_httpClient.BaseAddress (with the existing error fallback).
  • Files reviewed: 52/52 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/Opc.Ua.Client/Browser.cs Outdated
@romanett @claude
Back off between fruitless browse passes and scope the tracker cleanup
0d5b9c4 
The no-progress bound on the managed browse had no delay behind it, so a
hundred passes could elapse in milliseconds - long before a browse that
holds the session's continuation point quota gets to release it, which
is exactly the contention the bound is supposed to survive. A pass that
completes nothing now backs off (5 ms doubling to 50 ms) before the
next one, making the bound worth seconds of real waiting instead of a
tight spin, and any completed node resets both the count and the delay.

ModelChangeTracker.StartTrackingAsync cleaned up after a failed start by
stopping tracking outright. A pump that faults clears IsTracking from
its own finally, so a concurrent caller can install a replacement before
that cleanup runs and the owner would then cancel the replacement. The
stop is now epoch scoped and only applies while the generation it
started is still current.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
marcschier
marcschier approved these changes Sep 12, 2026
View reviewed changes

@marcschier marcschier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix all 350+ warnings and alerts

@marcschier
marcschier marked this pull request as ready for review September 12, 2026 09:09
@marcschier
marcschier requested a balanced review from Copilot September 12, 2026 09:09
@marcschier marcschier added the ready Ready to merge once CI Passes label Sep 12, 2026
Copilot AI reviewed Sep 12, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved lifecycle, sequence-wrap, synchronization, reverse-connect, and sync-over-async defects remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/Opc.Ua.Client/NodeCache/NodeCache.cs:487

  • This changed fallback still performs sync-over-async network/cache work with GetAwaiter().GetResult(). If the new warming path misses a type (for example after a lenient per-node browse failure), a synchronous IsTypeOf call can block a request thread and potentially deadlock under a synchronization context. Keep this path cache-only and make the caller warm/resolve asynchronously instead.
    src/Opc.Ua.Client/Subscription/MessageProcessor.cs:405
  • The new gap arithmetic counts sequence number 0 as missing when the UInt32 sequence wraps, even though OPC UA wraps from uint.MaxValue directly to 1. For example, prevDataSeq == uint.MaxValue and curSeqNum == 1 produces delta == 2 and increments MissingMessageCount by 1 although no message is missing. Subtract the skipped zero slot for an accepted wraparound.

src/Opc.Ua.Client/ModelChange/ModelChangeTracker.cs:123

  • A pump that ends by itself leaves m_cts and m_pumpTask populated while setting IsTracking to false. This branch then overwrites both fields when tracking is restarted, so the previous token source is never disposed; the old start owner's epoch-guarded cleanup can also no longer reach it. Clean up the completed generation before installing its replacement, or atomically clear/dispose that generation from the pump's finally.
                    // Do NOT link to the caller's token: it bounds the wait for
                    // the tracker to become ready, not the lifetime of the
                    // pump. Linking it makes a per-request token silently kill
                    // tracking as soon as that request completes.
                    m_cts = new CancellationTokenSource();
  • Files reviewed: 52/52 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/Opc.Ua.Client/Session/ManagedSession.cs Outdated
Comment thread src/Opc.Ua.Client/Subscription/Subscription.cs
@romanett @claude
Keep the reverse connection until the gate grants, and fix the wrap gap
6bf5572 
ManagedSession consumed the caller-supplied waiting connection before it
asked the connect gate for an attempt, so a gate rejection burned the
single-use connection and the retry fell back to an outbound path that
cannot reach a server which only ever connects in reverse. It is taken
after the gate grants now.

Sequence numbers roll over from uint.MaxValue straight to 1 (Part 4
§5.14.5.1), so the arithmetic gap across a wrap includes zero, which is
never sent. MessageProcessor counted it and reported one missing message
on every wrap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
romanett commented Sep 13, 2026
View reviewed changes
Comment thread src/Opc.Ua.Client/Session/Reconnect/ConnectionStateMachine.cs
romanett
romanett commented Sep 13, 2026
View reviewed changes
Comment thread src/Opc.Ua.Client/Session/ManagedSession.cs Outdated
@romanett @claude
Fix the CA1816 and CA2000 analyzer warnings in the managed session
4d94f5b 
ManagedSession called GC.SuppressFinalize from the private
DisposeAsyncCoreAsync instead of the public DisposeAsync, which raised
CA1816 on both methods and ran the call twice on the synchronous path,
where Dispose() already makes it. It now sits in DisposeAsync.

ManagedSessionPool.DisposeAsync repeated RemoveAsync's per-key teardown
inside a loop, and CA2000 could not follow the entry obtained through
TryRemove across the await to CloseAndDisposeAsync, which disposes it.
It now calls RemoveAsync per key, removing the duplicate and the
warning; the identical shape in RemoveAsync is not flagged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
romanett commented Sep 13, 2026
View reviewed changes
Comment thread src/Opc.Ua.Client/Session/SessionClientExtensions.cs Outdated
romanett
romanett commented Sep 13, 2026
View reviewed changes
Comment thread src/Opc.Ua.Client/Subscription/Subscription.cs Outdated
romanett and others added 2 commits September 13, 2026 20:06
@romanett @claude
Stop the node cache subtype filter from blocking the caller
935f5d8 
The ubuntu Sessions job intermittently hung until the 10-minute blame
timeout killed the test host, leaving ~200 tests unrun and no dump.
The in-flight tests normally take seconds, so this was a genuine hang,
and it only ever appeared on this branch.

Root cause: GetReferencesAsync with includeSubtypes classified every
reference through a synchronous supertype walk. When a reference's own
type was not cached - which the lenient per-node fetch now leaves
behind, since a failed fetch is not cached but its type was already
counted as warmed - that walk parked the calling thread on
GetAwaiter().GetResult() until the type was fetched. Against the atomic
reference cache that wait can hang on a fetch already in flight for the
same type, whose continuations (including the managed browse's new
retry delays) need a free thread-pool thread. Four fixtures in parallel
on a two-core runner, several of them also blocking pool threads, starve
the pool and the browse never completes.

Subtype membership is now resolved asynchronously before filtering,
with the same lenient fetch and cycle guard, and the filter is pure
lookups. The synchronous fast path is limited to exact reference types,
which never walk the hierarchy. The blocking IsTypeOf and the
chain-warming helper that existed only to feed it are gone.

A regression test holds a reference type's fetch open and requires the
call to return rather than park on it; against the previous code it
fails after 10 s.

The four tests that were in flight also stop feeding the starvation and
stop taking the host down with them: ConnectAndReconnectAsync awaits the
reconnect instead of blocking a pool thread in WaitOne, the saved-secrets
test awaits its delays instead of Thread.Sleep, and all four carry
CancelAfter with the token forwarded to their reconnect, read, close and
browse calls, so a future hang fails one named test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett @claude
Address review: session factory engine, release logging, dispose path
ed3f062 
- ISessionFactory gains SubscriptionEngineFactory and
  WithSubscriptionEngine, which returns a configured copy.
  ManagedSession.CreateAsync specialises any factory that has no engine
  through it, replacing the DefaultSessionFactory type test and the
  in-place mutation of subclasses. DefaultSessionFactory's property is
  init-only again. Breaking for custom ISessionFactory implementations;
  noted in the 2.0 migration guide.
- ReleaseContinuationPointAsync takes a logger and logs a failed
  release as a warning instead of discarding it.
- ConnectionStateMachine disposed from its own flow now just cancels
  the worker and returns; the worker's finally already settles the
  waiters, so the continuation that disposed the token sources is gone.
- Clarify that only the local lifetime-count divisor is clamped, so a
  requested publishing interval of 0 still reaches the server.
- Fix CA2012 in the node cache subtype regression test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
romanett
romanett commented Sep 13, 2026
View reviewed changes

@romanett romanett left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

romanett and others added 2 commits September 13, 2026 21:09
@romanett @claude
Let ComplexTypeSystem own the resolver the client factories create
ef26d08 
ComplexTypeSystem.Create(session, ...), DefaultComplexTypeSystemFactory
and ComplexTypeSystemFactory each create a NodeCacheResolver with its
own NodeCache and hand it to a ComplexTypeSystem that outlives the call.
Nothing could dispose that resolver, so the cache's Meter stayed in the
static meter list with the caches its callbacks capture - the CA2000
warnings were real.

ComplexTypeSystem is now IDisposable with a new constructor taking
ownsResolver; Dispose disposes the resolver only when it is owned, so
resolvers passed to the existing constructors stay with the caller. The
three factories transfer ownership, and the one-shot loads in the DI
client builder, ManagedSessionBuilder, the server complex type loader
and the reference client sample dispose the system.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett @claude
Sync the bundled migration guide copy of sessions-subscriptions.md
69b1c93 
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@romanett romanett mentioned this pull request Sep 13, 2026
Closed
12 tasks
@marcschier
marcschier merged commit ea93c86 into master Sep 14, 2026
279 checks passed
@marcschier marcschier mentioned this pull request Sep 14, 2026
Merged
4 of 7 tasks
marcschier added a commit that referenced this pull request Sep 16, 2026
@marcschier
Fix server/core validation, lifecycle and protocol regressions (#4477)
fa8d45f 
# 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.
@marcschier
marcschier deleted the romanett/client-bugs-audit-fb6ef5 branch September 29, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcschier marcschier marcschier approved these changes
Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

ready Ready to merge once CI Passes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@romanett @marcschier