GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-26 04:19:53 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,739 Commits 126 Branches 105 Tags
fix/encoder-stack-depth-json-flake
Commit Graph
6 Commits
Author SHA1 Message Date
romanettClaude Opus 5.5Marc Schier
 		efad0ddeff Server subscriptions: fix spec defects in Subscription/MonitoredItem handling found by audit (#4629) 
# Description

Fixes defects found in a spec-grounded audit of the server
**Subscription / MonitoredItem** implementation
(`src/Opc.Ua.Server/Subscription/**`, `NodeManager/MonitoredItem/**`,
`EventManager`, monitored-item service dispatch and filter validation).
Each behaviour change was checked against OPC 10000-4 (§5.13, §5.14,
§7.21, §7.22), Part 5 (subscription diagnostics) and Part 9
(ConditionRefresh). Every fix has a regression test.

The audit found 56 defects (1 Critical, 3 High, 20 Medium, 32 Low). A
follow-up max-effort code review of the fix branch found 27 more issues,
mostly introduced by the first round of fixes. All but one are fixed
here.

### Most important fixes
- **Critical — remote hang:** an `AggregateFilter` with
`processingInterval = NaN` was not revised, and `AggregateCalculator`
looped forever while holding the node-manager lock, so one
CreateMonitoredItems request blocked the node manager. Non-finite, ≤0
and sub-tick intervals are now revised (Part 4 §5.13.2.1), and the
calculator rejects them as a backstop.
- **Monitored-item limits:** new
`ServerConfiguration.MaxMonitoredItemCount` /
`MaxMonitoredItemsPerSubscription`; Bad_TooManyMonitoredItems is
returned when a limit is hit, and ServerCapabilities now report the
limits. Both default to `0` (no limit), so existing behaviour is
unchanged.
- **Sampling groups:** a client could get one dedicated thread per
distinct sampling interval above 1 h; intervals above the last rate
group are now rounded, and the loop is async. An item could also end up
in two sampling groups and keep being sampled after it was deleted.
- **Subscription state machine** (Part 4 §5.14.1.2):
- the lifetime counter resets while a Publish is queued and counts
otherwise;
  - the lifetime and keep-alive stay within `MaxSubscriptionLifetime`;
  - LatePublishRequestCount is counted once per entry into LATE;
- messages held while publishing is disabled are no longer sent, and are
not offered by Transfer/Republish either;
- ConditionRefresh returns Bad_RefreshInProgress for any refresh already
queued or running on the subscription.
- **Triggering** (Part 4 §5.13.1.6):
- linked items are triggered by an item still draining a backlog, by the
first value after re-enable, and by an item created Disabled;
  - links are persisted for durable subscriptions;
- Disabled/Sampling items follow the publish rules in the shared
pipeline, so custom `IMonitoredItem` implementations follow them too.
- **Queues** (Part 4 §5.13.1.5, §7.21):
  - no duplicate value after a queue-size increase;
- EventQueueOverflow events are placed where the events were lost,
including on queue shrink;
  - the duplicate-event check scans the newest entries;
- EventQueueOverflowCount and MonitoringQueueOverflowCount are
maintained correctly;
- changing select clauses rebuilds queued events in the new layout, and
is safe on durable queues and on failure.
- **Filters:**
- per-clause EventFilterResult instead of rejecting the whole item (Part
4 §7.22.3);
- corrected status codes: Bad_DeadbandFilterInvalid,
Bad_MonitoredItemFilterInvalid, Bad_EventFilterInvalid,
Bad_MonitoringModeInvalid, Bad_TimestampsToReturnInvalid;
- aggregate filter in subscriptions: processing interval at least 2× the
revised sampling interval, revised StartTime on the requested grid,
ServerTimestamp = end of interval (Part 4 §7.22.4).
- **Lifecycle and races:**
- expired subscriptions are always cleaned up, even when the session
closes during cleanup;
  - Create/Transfer racing CloseSession returns Bad_SessionClosed;
  - session diagnostics counts no longer drift;
  - a large but valid Publish TimeoutHint no longer fails the request;
- a ready subscription is served by queued Publish requests instead of
going Late;
  - restored persisted state is validated.
- **Access:** a value change notification is checked against each
subscriber's UserAccessLevel, not the writer's.
- **Redundancy / durable store:**
  - the first-unsent sequence number is mirrored;
  - sequence numbers are no longer reused after a second failover;
- triggering links are stored (definition format 6, written only when
links exist, so older replicas can still read link-less subscriptions).

### Behaviour changes reviewers should know
- With the default config, publishing intervals above
`MaxSubscriptionLifetime / 3` (1,200,000 ms) are revised down, so
lifetime ≥ 3×keep-alive fits the configured maximum.
- The revised keep-alive is capped at a third of the max lifetime.
- Idle subscriptions expire after exactly MaxLifetimeCount cycles
without a queued Publish.
- A partly invalid EventFilter select list is accepted with per-clause
errors. The public `EventFilter.Validate` keeps Status Good if at least
one clause is valid; callers that cannot return per-clause results must
also check the new `EventFilter.Result.HasSelectClauseErrors`. There is
a migration note in `docs/migrate/2.0.x/sessions-subscriptions.md`.
- Aggregate processing intervals may be revised higher (≥ 2× sampling
interval); aggregate ServerTimestamp is now the interval end.
- Sampling intervals above 3,600,000 ms are rounded to multiples of
900,000 ms (default rate groups).

### Public API
Additive only:
- `ServerConfiguration.MaxMonitoredItemCount` and
`MaxMonitoredItemsPerSubscription`;
- `EventFilter.Result.HasSelectClauseErrors`;
- `IStoredSubscriptionTriggering`,
`ISubscriptionRetransmissionSendStateStore`,
`SubscriptionRetransmissionState.FirstUnsentSequenceNumber` (optional
store capabilities, detected with `as`);
- new `EventQueueHandler` constructor overloads.

### Known open items (not in this PR)
- **Part 4 §5.13.2.1:** CreateMonitoredItems should create an item even
when the user lacks Read permission and report the error in Publish.
This is not done: the create-time RolePermissions check is the only role
check on the initial value, so following the spec would leak values
until every node manager re-checks per sample.
- **Sampling loop:** it runs on thread-pool threads; node managers with
blocking synchronous reads should not use sampling groups heavily. The
alternative, a dedicated thread per group, was the thread-exhaustion
issue fixed here.
- **Restarts:** a pending QueueSize-1 value and a pending overflow flag
are not kept across a restart; that needs persistence interface
extensions.
- **Limit defaults:** decide whether the new limits should get finite
defaults (currently 0 = unlimited).

## Related Issues

- No tracking issue; the findings come from an internal audit of the
server subscription code.

## Checklist

- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [ ] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.

### Local verification (net10.0, all green)
| Suite | Passed | Failed |
|---|---|---|
| Opc.Ua.Server.Tests | 7963 | 0 |
| Opc.Ua.Subscriptions.Tests | 643 | 0 |
| Opc.Ua.Subscriptions.Classic.Tests | 57 | 0 |
| Opc.Ua.Subscriptions.Durable.Tests | 36 | 0 |
| Opc.Ua.Redundancy.Server.Tests | 847 | 0 |
| Opc.Ua.Core.Tests (filter tests) | 283 | 0 |

net48 build of `Opc.Ua.Server.Tests`: 0 errors. net48 tests and the full
UA.slnx were not run locally.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Marc Schier <marcschier@users.noreply.github.com>
 2026-10-03 05:43:16 +02:00
Marc SchierandCopilot 46fa7d364a Harden recovery, concurrency, and resource lifetimes across the OPC UA stack (#4516) 
# Description

Bound managed-session recovery, preserve Publish workers and recovery
ownership, and address the client, server, transport, and certificate
audit findings and subsequent concurrency review. Also reduce the memory
retained by exhaustive session-test diagnostics.

## Recovery and Publish ownership

- Add `ManagedSessionOptions.ChannelReconnectTimeout` through direct
construction, the fluent builder, and DI. The automatic duration is
`max(3 * KeepAliveInterval, revised SessionTimeout, OperationTimeout)`,
sampled at cycle start. Positive overrides are supported;
`Timeout.InfiniteTimeSpan` explicitly opts out. Raw sessions retain
their unlimited default.
- Add `CreateReconnectBudget(TimeProvider)` as a required
`IReconnectParticipant` member, as agreed in review. Custom participants
implement it and may return null for no participant-specific bound.
- Enforce the earliest participant/caller deadline through transport
reconnect, activation/recreation, and subscription restoration. Late
callers can shorten an active cycle but cannot extend it.
- Cancel and unwind recovery, fence late responses, and release channel
recovery ownership before the existing `Faulted` event hands control to
the outer reconnect/failover policy. Preserve session recreate admission
through final Publish cleanup and keepalive suppression until shared
recovery is complete.
- Treat participant budget creation/evaluation failures, including
exceptions at the exact deadline, as terminal shared-recovery failures.
Notify participants before exposing `Faulted`, preserve the original
diagnostic, and never silently replace a finite budget with unlimited
recovery.
- Keep caller-supplied replacement channels, managed bindings, wrapper
event subscriptions, and recovery-owner identities consistent across
success, failure, and cancellation. Reject replacement during active
recovery and fence retired callbacks so they cannot clear newer
ownership or suppress later keepalive recovery.
- Retain eligible Publish workers through temporary
subscription-identifier clearing without retaining workers for
never-created or removed subscriptions. Preserve cancellation,
acknowledgement rollback, and drain-before-replacement ordering.
- Coordinate automatic subscription updates with session recovery:
cancel admitted updates, keep them paused across a failed-restoration
handoff, and retry only after explicit restoration completes. This
prevents competing CreateSubscription requests and updates against a
retiring session, while preserving manual recreation, transfer fallback,
and pending option changes.
- Exercise the real ManagedSession, V2 subscription engine, and channel
ready gate with scripted transports and an injected clock. Cover
transfer fallback, deadline handoff, late completions, and resumed
notification delivery.

## Audit and review corrections

- Correct certificate/trust-list transactions, change-pump lifetimes,
validation, peer revalidation, and reset/resource ownership.
- Harden node admission, attribute/range/dimension validation,
monitored-item sampling and lifecycle transitions, event fanout, and
server shutdown. Run lifecycle readiness and compensation outside shared
monitored-item admission, with stale-completion ownership checks.
- Close synchronous node-manager admission before teardown and retain
resources until admitted service, lifecycle, and asynchronous method
work returns. Dispose the monitored-item manager outside both locks,
reject late calls before they can reach it, and forward asynchronous
disposal through the adapter so the master awaits the same completion.
Preserve reentrant shutdown initiation, cancellation/failure unwinding,
and shared cleanup-failure reporting.
- Bound WSS pending-request retention using connection-local wire
handles while preserving caller-visible request handles and translating
Cancel targets. Safely ignore retired replies and drain admitted
send/close users before resource disposal.
- Clarify that the shared `ClientBase` caller-handle sequence is
distinct from the non-wrapping WSS wire allocator. Existing
exhaustion/reopen, late-reply, and Cancel-translation regressions verify
the connection-local contract; no allocator behavior was changed by this
clarification.
- Preserve virtual-node validation and resolver service errors instead
of converting them to cached misses. A mismatched NodeId reports
`BadNodeIdInvalid`; callback type mismatches and resolver service errors
keep their exact status. Per-item boundaries preserve unaffected reads,
writes, history operations, method calls, browse paths, and
monitored-item creation/restoration, including permission-validation
paths. Caller cancellation still propagates; stored-item validation
failures are logged without aborting later restores.
- Correct client file/role/state-machine/streaming APIs, node-cache
behavior, redundancy/failover ownership, and session
configuration/identity handling.
- Cancel each owned peer-discovery scope before disposing it, including
when the bounded wait wins a cancellation-delivery race. Keep late
results out of the endpoint cache and preserve single-flight lookup
ownership.
- Keep materialized alias Browse results synchronized with the live
store by default, preserving explicit startup-only opt-out. Apply
HistoryUpdate service results per value while retaining explicit
all-or-nothing provider APIs.
- Update the directly related feature documentation and add focused
positive, negative, concurrency, and ownership regressions.

## Latest review-feedback corrections

- Fence stale failed lease observations with a successful-confirmation
generation distinct from the revocation epoch. Snapshot it before each
store read, including failed-CAS revalidation, so an old failure cannot
revoke or reschedule a newer lease while fresh ownership loss still
revokes leadership. Preserve independent overlapping acquisitions,
expiry, disposal, and late-success protections.
- Isolate attributable WSS response-decoding errors to their request,
including retired handles. Keep admitted socket sends connection-owned
so caller timeout/cancellation cannot abort sibling requests; shutdown
still cancels and drains those sends.
- Restart keepalive and classic Publish replenishment after successful
session reuse. Retire V2 worker demand after intentional
deletion/disable without confusing that state with temporary recovery
resets.
- Browse virtual directories lazily across continuation pages without
the 1,024-entry snapshot cap. Preserve active-page cancellation, bounded
metadata retention, pushback, and exactly-once asynchronous cursor
cleanup.
- Apply certificate/CRL staging in last-operation order, preflight
required issuer storage before writes, select actual policy-compatible
RSA keys for None endpoints, frame certificate chains independently of
signature OIDs, and preserve admitted validation results during
rejected-writer shutdown.
- Preserve base asynchronous disposal when a legacy override omits its
base call; avoid double derived disposal in the adapter. Let genuinely
admitted nested work finish while rejecting stale captured contexts and
returning null from closed lookup APIs.
- Re-park recovered monitored items if AddNodes rolls back. Give event
compensation a fresh finite deadline, and preserve healthy
Browse/Translate siblings at every reference/path boundary while
retaining starting-node errors and caller cancellation.
- Give final reconnect notifications a live, bounded shutdown-linked
lifetime. Synchronize child state before parent lifecycle handlers
outside the parent transition lock.
- Document the accepted existing policies: direct reverse connections
are single-use without a ReverseConnectManager, and multi-node stream
capacity is shared rather than reserved per node.

The expanded readiness controls also exposed a real generation-handoff
race: an old source could finish draining after activation was deferred
but before readiness tasks were collected, causing the wanted
replacement to be omitted from the wait set. A deterministic test gates
that exact interval; the fix keeps the replacement in the readiness
wait. The original race passed 200 repetitions after the fix. A separate
control now awaits both producer readiness and iterator entry under the
same original deadline, rather than treating those independent signals
as equivalent. Temporary logging and repetition are removed.

## CI integration contracts

The History and Server CI failures exposed integration tests that still
asserted the behavior intentionally changed by the audit fixes. A
Windows WoT negative test raced its caller deadline against upstream
retries. A subsequent Ubuntu Client failure exposed a real
automatic-update versus explicit-restoration race:

- History insertion collisions now verify exact per-value statuses and
all persisted timestamps/values, including preservation of the existing
conflicting entry. Explicit atomic-provider rollback controls remain.
- Alias materialization tests wait for actual publication and verify the
added node plus forward/inverse `AliasFor` references, instead of racing
a startup-only snapshot expectation. Explicit refresh opt-out controls
remain.
- The unavailable-upstream WoT test uses an explicit finite retry policy
through existing DI configuration, preserving its original four-minute
guard and requiring a mapped-value service error before caller
cancellation. Production retry defaults remain unchanged.
- The Client fix retains the exact one-create and late-response
assertions. Three deterministic real-subscription/real-manager cases
cover paused creation, disposal while paused, and cancellation of an
admitted create followed by explicit restoration and a pending option
update. Both composed restoration-deadline cases also prove automatic
updates stay blocked at the outer handoff and resume after restoration.
- The merged-head net48 correction prevents lost peer-resolver
cancellation and distinguishes the harness's delayed initial `Connected`
event from actual outer recovery completion. A deterministic timer-order
test covers both cooperative and cancellation-ignoring resolvers,
cancellation before return, rejection of late cached results, and a
fresh subsequent lookup.
- The remaining CodeQL alert was a false content flow through
`ArrayOf.Count` into an integer-only trust-list summary. Projecting the
same count through `Span.Length` keeps cardinality distinct from
serialized contents without allocating, dropping diagnostics,
suppressing the rule, or dismissing the alert. Five log-contract cases
cover null/empty/nonempty results and mixed chain-capable/fallback
listeners, asserting the exact count, message, and structured
properties.
- Classic transfer acknowledges messages selected for asynchronous
Republish only after their responses arrive. The existing locked
replay-selection decision returns the immediate-acknowledgement subset
to Session, preserving the public transfer API, advertised availability,
and immediate acknowledgement of older gapped messages not selected for
replay. A callback changing `RepublishAfterTransfer` cannot change the
already-selected acknowledgement ownership.
- Transfer integration tests explicitly control acknowledged versus
retained origin messages. Exact assertions distinguish retained replay
from requested fresh initial values by sequence number, client handles,
values, status, and source timestamp. Atomic counters, observed
delivery/keepalive progress, and failure-safe session/file cleanup
replace timing assumptions and racy resets.
- Server lifecycle tests verify the documented independent five-second
compensation deadline rather than requiring a non-cancellable token.
They cancel the actual request, prove compensation and unrelated
monitored-item creation still proceed one tick before the deadline,
preserve the original failure and ownership assertions, and verify the
completed deadline is retired.
- Synchronous and asynchronous directory-browser tests require the exact
provider exception instead of an empty-directory success, then verify
the failed cursor is not enumerated again. These correct expectations
left behind by the intentional, documented provider-error propagation
change; production behavior is unchanged.

No tests were disabled, assertions loosened, coverage thresholds
lowered, or CI checks made optional. Existing platform exclusions and
Explicit debug variants remain.

## Related Issues

- Fixes #4514
- Fixes #4515
- Builds on merged #4503 and retains the #4508 publishing-drain
behavior.

## Validation

**Lease-election review correction
(`a13afa641182c70983e16bbcfebf507e666a45a5`):**

The correction is based on the existing upstream merge
`43079e2b851fe338a4082066f8d14abfdafb6e32`. Three deterministic
stale-verdict
cases fail on the original source and pass with separate confirmation
fencing.
Six new cases cover the initial foreign-owner verdict, failed-CAS
revalidation
in both invocation orders, and fresh ownership loss through both read
paths,
including revalidation begun after another successful confirmation.

| Target and configuration | Passed | Failed | Skipped |
| --- | ---: | ---: | ---: |
| net10.0 Debug | 120 | 0 | 0 |
| net48 Debug | 120 | 0 | 0 |

The selection is the full `Opc.Ua.Core.Tests.Redundancy` namespace,
including
all 38 `SharedStoreLeaseElectionTests` cases and the existing expiry,
clock
rollback, disposal, and overlapping-success regressions. New witnesses
are
`StaleForeignLeaseReadDoesNotRevokeNewerAcquisitionAsync`,
`StaleFailedRevalidationDoesNotRevokeNewerAcquisitionAsync`, and
`FreshForeignLeaseObservationRevokesLeadershipAsync`. They assert
resulting
leadership, exact transitions, independent progress, and the unchanged
expiry
boundary. Retaining the initial generation for the fallback read
independently
makes the fresh-loss-after-confirmation control fail; that mutation was
removed
before the final rebuilt runs.

The successful scoped command is
`dotnet test tests\Opc.Ua.Core.Tests\Opc.Ua.Core.Tests.csproj -c Debug
--no-restore --filter
"FullyQualifiedName~Opc.Ua.Core.Tests.Redundancy"`,
with `CustomTestTarget` and `-f` matching each row and that framework's
assets
restored. Both changed C# files pass formatting and analyzers through
informational severity on both frameworks. These are scoped results, not
a
full-solution test or measured coverage-delta claim. CI monitoring is
paused
for this feedback-only operation; no current-head CI success is claimed.

**Latest CI contract correction
(`1cd1f2047bb5b50c9bf300b1ff710b0b2e935a29`):**

All four failing Server shard 23 jobs in
[run
36120946740](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/36120946740)
reported the same six cases. Two local replays reproduced all six
failures.
The source changes and documentation from `cd43b831a` establish both
intentional
contracts: independent bounded event compensation and surfaced provider
errors.
Only the two test files are corrected; production code and CI policy are
unchanged.

| Target and configuration | Passed | Failed | Skipped |
| --- | ---: | ---: | ---: |
| net48 Debug | 113 | 0 | 0 |
| net10.0 Release, CI Coverlet settings | 113 | 0 | 0 |

The selection contains `MonitoredItemManagerLifecycleTests`,
`DirectoryObjectStateTests`, `DirectoryBrowserBoundedEnumerationTests`,
and

`EventCompensationReadinessHasFiniteDeadlineWithInfiniteSourceTimeoutAsync`.
Both changed files pass formatting and analyzers through informational
severity
on both frameworks. Independent mutations restoring a non-cancellable
compensation token and swallowing provider errors fail all four
lifecycle
cases and both browser cases, respectively. Both mutations were removed
before
the final passing runs and commit.

The scoped command is `dotnet test
tests\Opc.Ua.Server.Tests\Opc.Ua.Server.Tests.csproj`
with `CustomTestTarget` and `-f` set to the selected framework,
`--no-restore`, and
`--filter
"FullyQualifiedName~MonitoredItemManagerLifecycleTests|FullyQualifiedName~DirectoryObjectStateTests|FullyQualifiedName~DirectoryBrowserBoundedEnumerationTests|FullyQualifiedName~EventCompensationReadinessHasFiniteDeadlineWithInfiniteSourceTimeoutAsync"`.
The modern run additionally uses `-c Release /p:CollectCoverage=true
--collect "XPlat Code Coverage" --settings
tests\coverlet.runsettings.xml`.
These are scoped results, not a full-solution test or measured
coverage-delta claim.
Hosted checks for that commit were pending at publication. The fail-fast
CI
watch was stopped when the requested operation switched to review
feedback;
no current-head CI success is claimed.

**Earlier master merge (`8b15ea6ef9fc2214ac8ec886f373bb3846dd6d6f`, base
`b4136d8e9bab0df680f41d9ef985f519160534b1`):**

The `TcpListenerChannel` conflict preserves both lifetime
responsibilities:
disposal clears the retained peer-certificate chain and unregisters the
quota-scoped message-assembly scheduler. Both fields and all upstream
deadline
handling remain intact. The only additional manual source adjustment
adds the
parentheses required by the existing analyzer around the session-count
fallback.

Master advanced during validation. The initial transport merge
`80303c434ce813cf75aa7fe54a6a32fe2aeafc30` was retained, and the newer
base was
merged normally in `8b15ea6ef9fc2214ac8ec886f373bb3846dd6d6f`, without
rewriting
history. The additional recovery, fluent-pipeline, and test-framework
overlaps
were inspected; the branch's cancellation, validation-error, and
acknowledgement
ownership fixes remain alongside the upstream changes.

Full `UA.slnx` builds passed on net10.0 and net48 with **0 warnings and
0 errors**.
The final scoped tests rebuilt against the committed tree
`2074f8efcb61d8d9c6106e7d6f76ee941940e358`:

| Target | Passed | Failed | Skipped |
| --- | ---: | ---: | ---: |
| net10.0 | 2,653 | 0 | 0 |
| net48 | 2,529 | 0 | 7 |

The net48 skips are existing platform exclusions: six RSA_DH_AesGcm
nonce cases
and one ClientWebSocket certificate-validation case requiring .NET 7 or
later.
The selection covers transport cleanup, reassembly
budgets/deadlines/admission,
peer-chain ownership, server session binding, fluent lifecycles and
source
generation, reconnect and keepalive, transfer acknowledgements, Publish
queues,
and JSON encoding. Modern-.NET WSS transport and real-server integration
cases
also pass. The resolved file passed solution-relative formatting and
analyzer
verification through informational severity on both frameworks.

These are scoped test results, not unfiltered full-suite or
measured-coverage
claims. Hosted CI was not monitored or repaired as part of this
merge-only
operation.

Selected local results below are overlapping validation runs, not
distinct-test totals.

**Earlier review-feedback batch
(`cd43b831a8f11de3f7d65c3681712a61e3c669a5`):**
The final net10.0 scoped selection passed **471 tests, with no failures
or skips**.
The final net48 solution-root selection passed **439 tests, with no
failures and
one unchanged platform skip**
(`ValidateServerCertificateFallsBackToTlsPolicyErrors`,
which requires newer ClientWebSocket certificate-validation support).
Both full nonincremental solution builds completed with **0 warnings and
0 errors**,
and all **40 changed C# files** passed informational-level
formatting/analyzer
verification. Both frameworks validated the identical final tracked
diff.
The focused matrix covers every one of the 20 current review threads,
including
real mid-frame WSS cancellation, stale admission contexts, directory
continuation
ownership, multi-element Browse/Translate failures, and the readiness
drain race.
The net10.0 solution-root test command has a separate pre-existing
runner conflict:
three unrelated AOT projects reject the VSTest target under
Microsoft.Testing.Platform
2.4.0 and SDK 10. Per-project net10.0 results are recorded separately;
no runner
migration, test exclusion, assertion relaxation, or CI-policy change is
included.
These are bounded selections, not unfiltered full-suite or measured
coverage claims.
At publication of `cd43b831a`, hosted CI was not green: the
`OPCFoundation.UA-.NETStandard` check reported `ACTION_REQUIRED`,
AppVeyor reported
`FAILURE`, and the two signed-build jobs were in progress. No CI jobs
were rerun,
cancelled, approved, or repaired as part of that feedback-only
operation.

**Classic transfer correction
(`f425376fca045794122e19aaf9cafebc4a717d91`):**
The preceding head's
[macOS net10.0 shard
25/30](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/35980326299/job/107570512458)
failed
`TransferSubscriptionTest(DisconnectedRepublish).TransferSubscriptionAsync(True,False)`
at the static-node count assertion: expected 10, received 20. The trace
contains a successful,
explicitly requested republish of retained sequence 1 and a fresh
initial-value batch at sequence 3,
not duplicate dispatch of one response.

Controlled lost origin acknowledgements reproduced that incorrect test
expectation on both the
preceding PR head and exact master base
`a2539cb53dd1666cef856f33812d7a8830d68909`.
The maintainer explicitly authorized correcting this existing issue in
the PR. Retained replay
and requested initial values are independent, as specified by
[OPC UA Part 4,
TransferSubscriptions](https://reference.opcfoundation.org/specs/OPC-10000-4/5.14.7).
The reason that the original hosted run retained its particular origin
message remains unproved.

The expanded full-suite run then exposed a **real production race**, not
just a count assertion:
Session queued acknowledgements for all transferred available messages
before the asynchronous
Republish responses arrived. Publish could delete those messages first,
causing
`BadMessageNotAvailable`. A gated real-Session/classic-engine regression
failed both replay-enabled
cases on the original implementation while both no-replay controls
passed. The ownership fix
passes all 24 combinations of empty/consecutive/unsorted-gapped queues,
replay preference,
sequential delivery, and a reentrant preference change.

All 40 normal real-server transfer cases pass on both frameworks.
Independent mutations
suppressing actual replay and duplicating observed sequence records each
fail the intended
assertion; both mutations were removed. The full classic suite passes on
both frameworks.
Hosted CI was pending when that correction was published; the earlier
failed hosted job was not
rerun for a lucky pass.

**Local net48 caveat:** the full Client run timed out in the
pre-existing fake-clock test
`ReconnectAsyncSwapsFaultedLeaseEntryAsync`, which does not exercise
subscription transfer.
Its unchanged isolated replay passed, but that does not resolve the
full-suite timeout.
No unrelated code or assertions were changed, and a clean full Client
net48 run is not claimed.

| Scope | net10.0 | net48 |
| --- | --- | --- |
| Full Client.Tests after transfer acknowledgement correction | 2,549
passed | 2,552 passed, 1 timeout failure noted above, 1 existing
platform skip |
| Full classic-subscription project after transfer correction | 57
passed, including all 40 normal transfer cases | 57 passed, including
all 40 normal transfer cases |
| Gated transferred-message acknowledgement ownership | 24 passed,
included above | 24 passed, included above |
| Saved-session consumer of the shared subscription helper | 40 passed |
16 passed, 24 existing unsupported-security-policy skips |
| Audit regression matrix | 2,339 passed | 1,934 passed, 34 existing
skips |
| Full Client.Tests after review follow-ups | 2,520 passed | Focused
selections only |
| Full Client.Tests after CI recovery correction | 2,523 passed, CI
coverage collector | 2,527 passed, 1 existing platform skip |
| Latest merged-head recovery/redundancy selection | 139 passed | 139
passed |
| Full Client.Tests after merged-head net48 correction | Focused
selection above | 2,529 passed, 1 existing platform skip |
| Lifecycle and actual-dimension regressions | 146 passed | 146 passed |
| Focused recovery follow-ups | 7 passed | 7 passed |
| WSS-related fixtures | 93 passed | Not applicable |
| CI History integration corrections | 28 passed | 28 passed |
| CI alias, dispatcher, and explicit-atomic controls | 63 passed | 63
passed |
| Full WoT sample project with CI coverage collector | 29 passed | Not
applicable |
| Trust-list effects and count-only summary contract | 22 passed | 22
passed |
| Virtual-node feedback, node-manager, dispatcher, and lifecycle
selections | 661 passed, 63 existing not-executed cases; all 48
virtual-node cases passed | 661 passed, 63 existing not-executed cases;
all 48 virtual-node cases passed |
| WSS wire-handle exhaustion, late replies, and Cancel translation | 9
passed | Not applicable |
| Synchronous node-manager disposal and related service/lifecycle suites
| 1,037 passed, 64 not-executed cases from existing fixture assumptions;
all 93 sampling/disposal cases passed | 1,037 passed, 64 not-executed
cases from existing fixture assumptions; all 93 sampling/disposal cases
passed |

- Full solution builds passed on net10.0 and net48 during the audit
work. The subsequent full non-incremental net10.0 build also completed
with zero warnings/errors.
- Modified source/test formatting and analyzer checks passed through
informational severity.
- Original composed recovery regressions passed repeated runs on both
frameworks. Independent mutations removing Publish cancellation,
bypassing the recreate drain call, or ignoring ready-gate cancellation
each failed the intended assertion; all mutations were restored before
passing runs.
- The latest recovery correction passed 100 executions of each original
restoration-deadline scenario with unchanged assertions. Restoring
premature update admission independently made both strengthened handoff
cases fail; the correct retention behavior was restored afterward.
- Startup-event and cancellation-delivery timing probes reproduced the
subsequent net48 failures, then passed 100 executions of each of the
three scenarios with the fixes. All timing probes and repetition were
removed. The permanent timeout-order regression also went RED on the
original cancellation cleanup without sleeps or scheduling
amplification.
- Final post-format recovery/subscription/engine checks passed all 151
selected cases on net10.0. The first local full net48 attempt aborted in
`clr.dll` with native access violation `0xc0000005`; an
unchanged-assembly full diagnostic rerun passed as recorded above. That
native host crash has not been attributed or claimed fixed.
- A minimal replay with the same CodeQL CLI and query-pack versions
reproduced the custom-count alert and confirmed that the standard
span-length projection is clean while direct sensitive-value and
raw-identifier logging controls remain flagged. The source was already
logging only a count; no identifier or certificate disclosure was
established at the reported location.
- The virtual-node feedback regressions first reproduced five
error-preservation/cache failures; expanded service-dispatch cases
exposed two additional whole-batch failures before the
operation-boundary fixes. The final selections pass on both frameworks.
Genuine misses remain cached, successful results remain reusable, failed
validation allows same-cache retry, and request cancellation still stops
the batch.
- Three deterministic synchronous-manager disposal regressions
reproduced early resource release and lifecycle calls reaching a manager
during disposal. The fix passes direct and adapted calls, a
sampling-worker join after the last operation exits, reentrant shutdown,
cancellation and failure paths, repeated disposal, and exact rejection
across 39 service/lifecycle entry points both during drain and after
cleanup. The original outside-node-lock disposal regression remains and
now requires the worker to observe closed admission.
- Prior-head CI on `c985f8bded77a60449d9cd381c8a004d8590ccf6` completed
with **168 passing checks, one expected release-package-publication
skip, and no failures or pending checks**. All test shards, signed
builds, container builds, the coverage check, and the CodeQL PR check
passed. Hosted CodeQL analysis `1829262912` completed successfully and
marks count-flow alert 385 fixed. This is evidence for that head, not a
claim that subsequent review follow-ups have completed hosted CI.

### Session-test diagnostic memory

Complete browse listings are streamed to unique NUnit attachments rather
than retained in inline NUnit/VSTest output. Detached regression-test
results are cleaned up without deleting runner-collected attachments.

- Two covered session-suite runs each completed with 782 passed and 379
existing skips. Peak working set fell from 4.86 GiB to 2.44-2.78 GiB.
- TRX output shrank from 303.1 MiB to approximately 40.7 MiB, retaining
all 46 complete browse attachments.
- Seven output regressions cover exact content, empty/single/large
listings, disabled/enabled output, bounded inline summaries, and unique
filenames.

The separate Azure build-18452 VM deprovisioning cause remains unproved
without provider logs. The memory measurements do not establish its
cause.

Unfiltered local `UA.slnx` tests, including the long stress/soak suites,
have not been run on both frameworks. A local coverage delta was not
collected; builds and focused tests are not substitutes for those
claims.

## Checklist

- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [ ] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] All checks on the latest review-follow-up head have completed
successfully; prior-head CI passed as recorded above.
- [x] I have addressed **all** PR feedback received.

Tests have been added and pass as recorded above; the hosted coverage
check passed on the prior head identified above. The combined
tests/coverage-increase checkbox remains open because no local coverage
delta was measured.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208
 2026-09-25 14:59:34 +00:00
Marc Schier fa8d45f2cf Fix server/core validation, lifecycle and protocol regressions (#4477) 
# Description

**Server/core audit fixes with regression tests. Latest review-fix
validation, merge validation and earlier acceptance results are
distinguished below.**

This addresses a 95-finding server/core report. Every finding was
reviewed with
the OPC UA specification knowledge base or, where the standard does not
prescribe a C# implementation, the corresponding ownership/configuration
contract. Incorrect or withdrawn subclaims remain qualified rather than
being
presented as additional fixes.

The original checkpoint covers session identity/certificate validation
and
revocation; certificate/key ownership and transactional updates; TCP
reconnect,
receive/admission and buffer lifetime; HTTP quotas; subscription
admission,
durability and pooled payloads; file-operation lifecycle; historian
aggregates;
fluent/state-machine behavior; and related utilities. It includes
targeted
regressions and directly related documentation.

The approved acceptance cleanup is separate in the commit history:

| Cleanup | Verified result |
| --- | --- |
| Invalid identity-policy and KeyCredential fixture inputs | Fixtures
explicitly advertise Username and use its generated policy ID.
Credential persistence uses valid clear mode inside the existing
encrypted admin context. Original permission, lockout and negative
encryption assertions remain. |
| Native GDS lifetime and concurrent connection failures | Retain
ApplicationInstance until client teardown. A deterministic native
reproducer proved the test-only channel cap of 10 was exhausted;
removing that override restores the existing bounded default of 1000. No
global serialization or production quota change. |
| Stress certificate ownership and close accounting | Balance two helper
references and the fake transport's server-certificate ownership.
Replace the invalid event-count bound with exactly-once physical
disposal, opens = closes, zero active entries and one logical entry
lifetime. |
| Participant deadlines | Use the injected TimeProvider with the
existing cross-framework WaitAsync overload. Tests pin one tick before,
exact expiry, terminal notification, recovery and timely completion. |
| Reproducibility and platform assertions | Preserve canonical WoT
bytes/hashes, explicit display-culture contracts and exact
numeric-vector assertions. Accept Apple's CryptographicException subtype
without weakening disposal balance. |
| Missing user-signature proof | Reject null/empty proofs before OS
crypto. Return BadUserSignatureInvalid per Part 4, 5.7.3.3. None-policy
and valid-signature controls remain. A CI-only test nullable-context
error was corrected without changing production behavior. |
| Durable sample restore and shared test storage | Dequeue waits for
actual batch residency instead of treating failed restore completion as
success, and restored files close before deletion. Five deterministic
regressions failed before the fix. The two fixtures that clear the
shared batch directory are isolated; all original 3,000-item assertions
remain unchanged. |

## Latest review follow-up

Commit `828856850219518970e8e2f580b8f94b3c1fc748` fixes the deferred
robotics
disposal reported in the latest review.
`RobotIntentNodeManager.DisposeAsync`
now joins a shared controller-host drain and then explicitly awaits base
node-manager disposal. A controller's bounded shutdown wait may defer
its
cleanup, but no longer lets the node-manager override finish early.
Concurrent
callers, including an asynchronous call after synchronous `Dispose`,
cannot
bypass the drain after the host list has been detached.

Synchronous disposal remains nonblocking for deferred work. Deferred
initialization failures are logged and retained, and base cleanup
failures
propagate to asynchronous callers. The existing controller-level bounded
disposal contract is unchanged; the node-manager contract is documented
in
`docs/Robotics.md`.

| Regression requirement | Evidence |
| --- | --- |
| Wait for deferred hosts; preserve synchronous compatibility and
concurrent joins | `NodeManagerAsyncDisposalWaitsForDeferredHostsAsync`:
four cases covering both entry-point orders and zero/positive host
shutdown waits. All four failed before the fix. |
| Wait through base cleanup and observe failures |
`NodeManagerDisposalWaitsForBaseCleanupAndReportsItsFailureAsync`: four
cases hold the actual base-resource semaphore, verify pending callers
after the host finishes, then check success or the same propagated
exception and exactly-once cleanup. |

The focused `IntentHostingCoverageTests` and
`IntentControllerHostTests.Dispose`
selection passed **30/30 on net10.0 and 30/30 on net48**, with no
failures or
skips. The final net10 solution build passed with zero errors and only
the four
previously documented CA2249 warnings in unchanged fuzz-test code. No
new
warnings, analyzer suppressions, dependencies, or weakened assertions
were added.
The review thread received a commit-specific reply and was explicitly
resolved.

This verification is scoped to the robotics feedback; the earlier full
Subscriptions-run limitation remains recorded in the merge section
below.


## Documentation review follow-up (2026-09-14)

Commit `f9a9686fcc5d3cfd19cbbc8c2d826e20f5c7e72f` addresses the six
follow-up
threads on documentation and implementation rationale. Each has an
in-thread
reply citing the commit and an explicit resolution.

Multiline XML documentation now covers the declarations introduced or
modified
throughout the entire PR, including internal contracts, samples, tests,
and
public members of related private helpers. A syntax-aware inventory
checked
**2,031 declarations across all 256 PR C# paths with zero documentation
gaps**.
A supplementary check also covers **75 previously unchanged public
members**
of private helpers involved in the changed code. Token comparison across
the
conditional-compilation branches proves this follow-up changes **no
executable
C# or test assertions**. Project-file changes are rationale comments
only.

The implementation questions are documented at their source:

| Question | Resolution |
| --- | --- |
| Certificate assembly friendship | Server Push configuration reuses the
internal explicit-subject CSR helper with the existing key; no public
crypto API was added. |
| Core.Types assembly friendship | Server builders use internal
transition callbacks/revision guards; Server.Tests checks the revision
deterministically without creating a public compatibility contract. |
| Always-single-reader notifications | One dequeuer preserves event
order; optional concurrency fans out within one event and joins before
the next event. |
| Asynchronous disposal completion | `DisposeAsync` already joins the
completion published after admitted operations and all cleanup attempts.
The compatibility `Dispose` remains nonblocking to avoid deadlocking an
admitted callback against its own drain. |

The requested detailed review disposition follows. It distinguishes
actual
fixes from confirmed policies and incorrect subclaims rather than
counting every
comment as a new runtime change.


### Detailed disposition of review 5191795284

The [SDK
review](https://github.com/OPCFoundation/UA-.NETStandard/pull/4477#pullrequestreview-5191795284)
was submitted against `1c4a9df6c`, before the subsequent merge and
feedback fixes.
Its **15 inline threads** have each received an action-specific reply
and an
explicit resolution. A fresh GraphQL query verified those replies and
resolution
states; the fixes in `b8a59990f` remain ancestors of the current branch.

| Review concern | Action and disposition | Concrete regression evidence
|
| --- | --- | --- |
| Injected certificate-store providers bypassed | **Fixed.**
Registration, configuration snapshots, and validation use the injected
providers. Explicit custom types remain authoritative; the built-in
Directory catch-all is deferred so native X509 paths and later custom
providers are not captured. Failed store opens dispose the created
store. | `InjectedProviderResolvesRegisteredAndConfiguredTrustStores`
covers default-provider ordering and both registration paths; provider
validation/open-failure cases and the unchanged
`TrustedCertificateInjectedViaIndependentX509StoreIsObservedAsync` cover
real use and independent-store visibility. |
| Unguarded timer-thread leadership callbacks | **Fixed.** Each
subscriber's exception is logged independently and cannot suppress
subsequent subscribers or fail lease operations. `IsLeader` reads the
deadline without invoking application callbacks. |
`ThrowingLeadershipSubscriberDoesNotFailAcquireOrDisposeAsync`,
`ExpiryNotifiesOtherSubscribersWhenOneThrowsAsync`, and
`IsLeaderDoesNotDispatchExpiryNotificationsAsync` were all demonstrated
failing before the fix. |
| Legacy pending-key provider accepts an unfinishable regenerated CSR |
**Fixed by explicit capability validation.** A provider implementing
only `IPendingCertificateKeyStore` receives `BadNotSupported` before key
generation or persistence when regeneration is requested. Existing-key
CSR/update remains supported. Regeneration requires matching-only
consumption; no unsafe destructive probing was introduced. |
`LegacyPendingProviderRejectsRegenerationBeforeCreatingOrSavingAKeyAsync`
checks no key or store mutation;
`LegacyPendingProviderCompletesExistingKeySigningAndUpdateAsync` proves
the supported flow completes. |
| Shutdown waits behind user deactivation | **Fixed.** Accepted
deactivations drain before the master's lifecycle gate is taken.
Configuration retains its binding until the shared drain completes,
including concurrent shutdown callers and direct master disposal. | Four
cases of `ShutdownDrainsDeactivationOutsideTheSessionClosingGateAsync`
exercise real configuration/binding/ServerInternalData/master reentry,
retained address space, exact session/subscription closure, and no
production cancellation shortcut. |
| Subclass cleanup runs under the operation-lifetime lock | **Fixed.**
Cleanup starts outside that lock. A pre-published completion joins all
disposers, and only the active asynchronous teardown context may use
guarded helpers. Captured contexts lose that privilege when the callback
exits. |
`SynchronousOwnedCleanupLetsSamplingWorkerReachClosedAdmissionAsync`,
`DisposeCoreCanUseGuardedMembersWithoutReopeningCallerAdmissionAsync`,
captured-context rejection, and exact cleanup-failure/ownership
assertions cover both synchronous and suspended cleanup. |
| Discovery requestId-zero chunk read after pool return | **Fixed by the
earlier merge.** Discovery validation precedes `SaveIntermediateChunk`;
no read occurs after that path can return the buffer. The ordering is
retained. | `DiscoveryChannelReadsTheFirstChunkBeforeItIsReturnedAsync`
uses a poisoning pool and the requestId-zero path. |
| T1.8 OPN cleanup misses post-decryption throws | **Fixed by the
earlier merge and pinned with new regressions.** Both sequence rejection
and certificate mismatch release the decrypted body and input ownership.
| `OpenSecureChannelRejectionAfterDecryptionReturnsEveryRentalAsync`
checks the exact audit status and pool conservation. Omitting either
corresponding release in an isolated snapshot made its case fail; no
mutation remains. |
| T1.2 mount-root protection depends on the provider | **Fixed at the
host boundary.** Separator-only aliases resolve to the protected root.
Traversal, empty/whitespace segments, backslashes, and drive/stream
qualifiers are rejected before arbitrary providers are invoked. Valid
nested paths and root destinations remain supported. | A deliberately
permissive provider is used by
`NonCanonicalObjectPathsCannotDeleteMoveOrCopyAsync` and invalid-target
cases; exact zero-call checks are paired with legitimate
nested-path/root-destination controls. |
| Inline trusted/issuer entries now face CRL checks | **Intentional
strict behavior, documented and tested.** A configured backing store
with strict unknown-status rejection requires current revocation
information. Explicit-only lists without a store retain their no-store
behavior. Actual revocation cannot be suppressed; a matching store
entry's policy takes precedence. |
`InlineIssuerWithStoreHonorsStrictUnknownRevocationPolicyAsync`,
`InlineIssuerRevocationIsNeverSuppressedAsync`, store-precedence cases,
and explicit-only controls preserve the missing-information versus
actual-revocation distinction in Part 4 6.1.3. |
| RSA minimum applied to unspecified ECC identifiers | **Fixed at the
application-certificate check.** The RSA minimum is compared only with
an actual RSA public key. An unspecified ECC identity is not replaced
simply because its key size is below an RSA threshold. |
`SilentCertificateCheckPreservesUnspecifiedEccIdentityAsync` checks DER,
thumbprint, store contents, and private-key retention; explicit RSA and
HTTPS minimum controls remain. |
| Failed directory refresh wedges corrective mutations | **Fixed.**
Recoverable automatic pre-refresh failures are logged and leave
reconciliation pending, but do not block corrective provider mutations.
Explicit refresh errors, capacity admission, caller cancellation, and
actual provider denials remain enforced. |
`CorrectiveDeleteRecoversAfterPersistentOverLimitRefreshFailureAsync`
converges an externally over-limit directory back to its exact allowed
contents. Capacity, cancellation/token, and six refresh-error controls
check negative paths. |
| Timed-transition rejection logs at 10 Hz | **Fixed.** Repeated
`(state, revision, status)` warnings are deduplicated without changing
the retry cadence. A different status or state entry permits another
warning. | `RepeatedTimedRejectionLogsOnceAndRecoversAsync` checks one
warning over eleven rejections and success on the next attempt.
Status-change and same/different-state reentry controls pin reset
behavior. |
| Bad unsubscribe strands retired-generation bookkeeping | **Fixed.**
Retired all-event bookkeeping completes even when unsubscribe reports
Bad; the first Bad is still returned to the caller. | Good and
BadCommunicationError cases of
`RetiredAllEventsDeletionCompletesBookkeepingAndReleasesGenerationAsync`
check event/snapshot removal, generation release, and exactly one
unsubscribe without finalization replay. |
| CSR subject parsing and fallback | **Fixed for existing and
regenerated keys.** Omitted subjects fall back to the actual stored
certificate. Regeneration resolves the public certificate's subject
before creating a key. Malformed or unresolvable subjects return
`BadInvalidArgument` before pending-key mutation. | Store-only fallback,
real-generator null/empty cases, and missing-subject/malformed-DN
controls verify the signed CSR, matching fresh pending key, unchanged
active identity, and no destructive side effects. |
| Committed RemoveUser change reported as failed | **Fixed for
RemoveUser and ModifyUser.** Accepted cleanup is still awaited and close
failures remain logged, but cannot replace the committed mutation
result. Precommit failures retain their original result and side
effects. |
`CommittedUserChangeKeepsResultAndPublishedUsersWhenCloseFailsAsync`
checks published Users, Good result, logging, and continued closes.
`FailedUserChangeKeepsPublishedUsersAndDoesNotCloseSessionsAsync` checks
BadNotFound with no publication or close. |
| Alleged pooled decoded-request aliasing | **No runtime redesign
needed; reviewer rebuttal confirmed.** The decoder copies the relevant
input data. Misleading ownership comments were corrected rather than
introducing a new dispatch model. |
`DecodedRequestRetainsValuesAfterInputBuffersAreReturnedAsync` checks
the exact decoded NodeId and ByteString after returned arrays are
poisoned. |

The current documentation-only follow-up rechecked **104 Core, 473
Server, and
4 Configuration cases** covering these concerns: **581 passed, zero
failed or
skipped**. The final net10 solution build also passed with **zero
warnings and
errors**. The complete earlier Core/Server/Configuration and net48
results remain
listed separately rather than being presented as rerun current-head
acceptance.

## Earlier review follow-up

Commit `b8a59990fe2e8e967a12ffd5d9158beed7831143` addresses all 19
review
threads. Each thread has an in-thread reply naming this commit and was
explicitly
resolved after verification; the final paginated query found 19 resolved
and
zero unresolved threads.

The follow-up fixes injected certificate-store selection (including
default
provider ordering and native X509 paths), actual-RSA minimum
enforcement, and
CSR capability/subject validation before key mutation. Node-manager
teardown
uses simpler scoped ownership and drains accepted user revocation
outside the
master gate; committed user results and retired-event cleanup remain
correct
when cleanup reports failure. File mutations can recover after failed
refresh
without bypassing capacity or cancellation, provider paths protect mount
roots,
and repeated timed rejections no longer flood logs. Leadership
subscriber
failures are isolated without callbacks from `IsLeader`.

Regressions also pin the already-merged OPN/discovery buffer fixes and
prove
decoded request values do not borrow returned input buffers. The strict
inline
issuer revocation policy is documented and tested, not weakened.
Existing tests
were changed only for the reviewed behavior: the incorrect
committed-user result
was corrected, and inline-issuer/root-alias controls were expanded.

### Review-fix validation

| Suite | Scope | Framework | Passed | Existing TRX NotExecuted |
| --- | --- | --- | ---: | ---: |
| Core | Complete project | net10.0 | 4,970 | 88 |
| Server | Complete project | net10.0 | 6,414 | 69 |
| Configuration | Complete project | net10.0 | 231 | 0 |
| Core | Certificate, lease and buffer compatibility slice | net48 | 153
| 0 |
| Server | Reviewed lifecycle, user, file, timer and CSR compatibility
slice | net48 | 692 | 0 |
| Configuration | Complete project | net48 | 231 | 0 |
| CSR provider regressions | Final portable-mock follow-up | net10.0 |
11 | 0 |

All runs exited successfully with normal concurrency and leak detection.
The Server console reports 5 skips; its TRX additionally records
existing
assumption/explicit cases, hence the 69-record count above. The
Framework build
caught two modern-only `ValueTask.CompletedTask` usages in new mock
setup; only
those expressions changed to the equivalent portable
`default(ValueTask)`.
The complete modern suites precede that test-only correction; all
production
files remained byte-identical, and the corrected fixture passed on both
targets.

The final net10 solution build succeeded with 0 errors and the same 4
CA2249
warnings in unchanged fuzz-test code. Diff checks and changed-line
whitespace
checks are clean; whole-file formatting still identifies 10 unchanged
baseline
lines, which were not reformatted outside the feedback scope. The
committed
35-file tree was hash-verified against the validation inputs.

This is the review-feedback verification scope, not a rerun or sign-off
of the
historical complete platform/native/soak acceptance campaign below.


## Latest upstream merge

Merge `9d9bf679f317164c5b2b90924cd3cdc27a86d451` integrates `master`
through `162fc5ebb` (#4486),
including the AddNodes child-name index, shared certificate-error
reporting,
CTT sample-data corrections, and the accompanying regressions and
documentation.

The CreateSession conflict preserves this branch's certificate
ownership,
required certificate, application-URI, and channel-certificate checks.
The
validation result now retains nested errors through `ThrowIfInvalid`,
allowing
the upstream shared reporting helper to mask trust, policy, and
revocation
failures consistently without losing audit detail.

The new indexed AddNodes regression initially attempted to reuse a
deterministic
NodeId after renaming its existing node. Renaming frees the BrowseName,
not that
NodeId. `IndexedBrowseNameReusePreservesExistingNodeIdentityAsync` now
proves
both contracts: the derived-ID collision leaves the original node
intact, then
a fresh explicitly requested ID can reuse the old name while the new
name
remains protected against duplicates. No production identity rule was
relaxed.
The incoming seeded random-index test uses the existing `UnsecureRandom`
wrapper
with the same seed and sequence; no new analyzer suppression was added.

| Local merge validation | Framework | Passed | Existing exclusions |
| --- | --- | ---: | ---: |
| Core, complete project | net10.0 | 5,081 | 88 |
| NodeState / BaseInstanceState | net10.0 | 579 | 0 |
| Server node management, certificate and session-security selection |
net10.0 | 112 | 0 |
| InformationModel node-management services | net10.0 | 29 | 1 |
| NodeState / BaseInstanceState | net48 | 578 | 0 |
| Core certificate validation, error reporting and TCP buffers | net48 |
111 | 0 |
| Server node management, certificate and session-security selection |
net48 | 112 | 0 |

All selected runs exited successfully at normal concurrency: **6,602
passing
test executions**. Modified test assemblies were explicitly rebuilt, and
the
combined name/identity regression was verified by name in both net10 and
net48
results. No skips or weakened assertions were introduced.

The final `dotnet build UA.slnx -c Release --no-restore -m:1` with
`CustomTestTarget=net10.0` succeeded with **zero warnings and zero
errors**.
The resolved source and adjusted tests pass whitespace verification, and
their
source hashes match the validation inputs.

This is merge-specific verification. Earlier review results and the
previously
reported incomplete full Subscriptions runs below remain scoped to their
own
commits; they were not reopened or relabeled as passing by this
operation.

## Previous upstream merge (2026-09-15 morning)

Merge `c4694e20ab089c555af233642042133ff3a03ac8` integrates `master`
through `484d6c887`, retaining
#4481's shared Like parser/GDS query fixes, #4488's RequestHandle
recovery in
ServiceFaults, and #4487's expired-session and durable-transfer
corrections.

Three conflict resolutions preserve both sides:

- Alias searches now use the shared `LikePattern` implementation,
including
upstream malformed-list validation. A timeout overload retains this
branch's
100 ms per-alias deadline and `BadTimeout` reporting without keeping a
second
wildcard translator. The shared matcher's original unlimited overload
remains
unchanged for existing GDS/ContentFilter callers. New regressions cover
bounded prefix/list scans, reuse after timeout, and the alias service
result.
- Expired activation closes outside the session-manager gate using
upstream's
once-only timeout claim, audit, and diagnostic accounting. This branch's
  original-channel and security-policy checks remain in place.
- TCP buffer tests retain this branch's ownership, poisoning-pool and
post-decryption checks alongside the upstream RequestHandle/fault
overload
  tests; neither set was discarded.

The upstream HTTPS binary behavior is intentionally retained: a decoder
`ServiceResultException` produces HTTP 200 with an OPC UA ServiceFault
carrying
the decoder status, response timestamp, and recoverable RequestHandle,
rather
than the previous HTTP 500 text response. Other exceptions retain their
existing HTTP error handling.

| Local validation | Framework | Passed | TRX NotExecuted |
| --- | --- | ---: | ---: |
| Core, complete project | net10.0 | 5,054 | 88 |
| Sessions, complete project | net10.0 | 780 | 610 |
| Server aliases, expiry/security and subscriptions | net10.0 | 288 | 0
|
| GDS database, paging and live directory calls | net10.0 | 263 | 0 |
| Shared Like patterns and bounded evaluation | net10.0 | 109 | 0 |
| Subscriptions transfer/durability/service selection | net10.0 | 23 | 1
|
| HTTPS WebApi, complete project | net10.0 | 346 | 0 |
| InformationModel aliases | net10.0 | 54 | 5 |
| Shared Like patterns and bounded evaluation | net48 | 109 | 0 |
| Core fault handles, TCP/HTTPS and filters | net48 | 268 | 0 |
| Server aliases, expiry/security and subscriptions | net48 | 288 | 0 |

These selected runs exited successfully with normal test concurrency:
**7,582 passing test executions**. Sessions' console reports 379 skips;
the
TRX also contains conditional/assumption exclusions, so the table uses
the
actual 610 NotExecuted records. No skips or weakened assertions were
added.
The new `EvaluationTimeoutBoundsWorkAndPreservesPatternReuse` and
`SharedPatternTimeoutRemainsAnAliasServiceError` tests passed on both
targets.

**Validation limitation:** two unfiltered Subscriptions runs did not
produce
a final result and were stopped, including a retry with a hang guard.
They
are not counted as passing. The isolated failover fixture passed 2/2,
and the
transfer/durability/service selection above passed. The stall's cause
was not
established; no unrelated code or test was changed to conceal it.

The final `dotnet build UA.slnx -c Release --no-restore -m:1` with
`CustomTestTarget=net10.0` succeeded with **0 warnings and 0 errors**.
Whitespace verification passed for all hand-reconciled files, and staged
source hashes match the validation inputs.

These are merge-specific checks, not a new full platform/native/soak
acceptance
sign-off. Earlier review and acceptance evidence below remains dated to
its
own commit.

## Previous upstream merge (2026-09-14)

Merge `c3e09553e4d1509502d4ba751960d58491bea2cf` integrates `master`
through `6313e38a0` (#4478),
including the CTT-driven Part 13 aggregate corrections and FindAlias
argument
validation. The upstream tests and CTT documentation are retained.

The single conflict in `AliasNameWildcardMatcher` is reconciled through
the
branch's shared regex compiler: upstream malformed-pattern rejection and
`IsValidPattern` coexist with OPC UA character escaping,
whole-string/newline
matching, the 100 ms timeout, and service-level error reporting. The
upstream
ReferenceTypeFilter checks and chronological aggregate behavior are
preserved.

Local merge verification:

| Suite / selection | Framework | Passed | Existing skips |
| --- | --- | ---: | ---: |
| Server aliases, aggregates and historian | net10.0 | 1,011 | 0 |
| Server aliases, aggregates and historian | net48 | 1,011 | 0 |
| InformationModel aliases | net10.0 | 54 | 5 |
| Features aliases | net10.0 | 11 | 0 |
| Client aliases | net10.0 | 28 | 0 |
| History aggregate/historical-access consumers | net10.0 | 265 | 0 |

All selected tests passed at normal concurrency. The final net10
solution
build passed with 0 errors and 0 warnings.
The resolved file's whitespace check and the staged diff check passed.
These
are merge-specific checks, not a new full platform/native/soak
acceptance claim.

## Previous upstream merges

Integrates `master` through `8e874d073`, including the Client, Types,
Core and
source-generator audit fixes in #4471, #4473, #4474 and #4475, plus the
event queue-size revision from #4480. The merge commits are `150ea0439`
(26 conflict resolutions) and `a0e17c07c` (the base update that arrived
during
validation).

The overlapping fixes are reconciled rather than replacing either
branch:

- **Rejected-certificate retention follows this branch, as explicitly
requested:**
zero means unlimited history; negative values disable new storage while
preserving existing backend trimming behavior (Directory clears prior
history;
shared key/value leaves it intact). The processor and manager policy are
not
  redefined, and documentation/tests describe the distinction.
- Preserve owned certificate snapshots and borrowed validation-core
lifetimes
  alongside upstream cache invalidation, store-first CRL checks and
  `BadCertificateRevocationUnknown` for unsupported revocation lookup.
- Combine exactly-once chunk ownership, receive-loop cancellation,
stale-epoch
rejection and detach-before-adoption with upstream
signature-before-padding
  verification and secret cleanup.
- Preserve pooled `HttpClient` per-request configuration together with
bounded
response reads and deadlines that remain active through body
consumption.
Regression cases exercise already-used clients and both timeout
precedence cases.
- Retain the binary-compatible channel-key constructor and normalize
omitted
profiles; preserve exact profile filtering and upstream binary-first
HTTPS/WSS
  endpoint ordering.
- Preserve monotonic lease expiry and stale-confirmation fencing while
adding
ordered, non-reentrant leadership notifications outside the state lock.

## Validation

Local validation of the conflict-resolution merge:

| Suite | Framework | Passed | Existing skips |
| --- | --- | ---: | ---: |
| Core | net10.0 | 4,940 | 88 |
| Client | net10.0 | 2,278 | 0 |
| Server | net10.0 | 6,088 | 5 |
| Types | net10.0 | 10,394 | 1 |
| SourceGeneration.Core | net10.0 | 3,940 | 8 |
| SourceGeneration | net10.0 | 185 | 0 |
| Core | net48 | 4,855 | 104 |

After the subsequent four-file #4480 update, the related Server slice
passed
811 tests with 5 existing skips, the real durable queue-size cases
passed 6/6,
and EventManager's 21 cases passed again after analyzer-only
expected-array
cleanup. The final `dotnet build UA.slnx -c Release` with
`CustomTestTarget=net10.0`
succeeded: 0 errors and 4 pre-existing CA2249 warnings in unchanged
fuzz-test
code. Whitespace verification passed for the resolved files and the
follow-up.

Merge-specific proof includes
`RequestTimeoutRemainsActiveWhileReadingTheOpenApiBodyAsync` on
already-used
shared clients with either the request or HttpClient timeout shorter,
the
existing ownership/expiry regressions from both branches, and both
branches'
retention tests adjusted only to the explicitly chosen zero/negative
contract.

Earlier acceptance at `1c4a9df6c` passed hosted Linux/macOS CI, CodeQL,
image
builds and the canonical coverage gate (87.15% line, 77.27% branch,
92.14% patch).
Both Framework project matrices, all three Windows native suites (183
cases),
and the unfiltered net10 stress project (230 cases, including full
60-minute
chaos and 30-minute memory soaks) also passed. Those are **pre-merge
results**,
not a claim that the full framework/platform/native/soak matrix has been
rerun
on this merge. The previous acceptance workflow remains separate from
this
review-feedback operation.

Security checks, meaningful assertions, leak detection, default native
concurrency, full soak durations and coverage thresholds remain intact.
The user-set ready-for-review state is unchanged. No architectural or
complete
merged-head acceptance sign-off is claimed.

## Related Issues

- Fixes #4476

## Checklist

- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [x] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [x] I have addressed **all** PR feedback received.
 2026-09-16 04:42:09 +00:00
romanettClaude Opus 5copilot-swe-agent[bot]romanett
 		93559d06d3 Fix sample A&C refresh and alarm timing found by slow CTT Alarms and Conditions runs (#4482) 
## Why the CTT *Alarms and Conditions* group was so slow

I measured every A&C CU with a fresh `ConsoleReferenceServer --ctt` (CTT
1.05.06, scripts 1.05.513), read the per-test `ResultNode` timestamps,
and added `addLog` timing/counter instrumentation to a copy of the CTT
project.

| Cause | Effect |
| --- | --- |
| **CertificateExpiration** opens a modal dialog in `initialize.js`
("adjust the clock on the server"), and it also appears in `--hidden`
runs | The CTT never exits. This is why the whole group "did not finish
in 40 minutes". |
| **Limit/Level CUs:** `initialize.js` idles the CU session for one
Alarm Cycle Time (60 s) during the initial capture.
`RequestedSessionTimeout` is also 60 s. | The server correctly closes
the session. `LimitHelper` then gets `BadSessionIdInvalid` for its 5
filter subscriptions, and Test_003–006 each run 180 s, about 14 min per
CU. |
| Alarm `Test_002` never gives the AcknowledgeableCondition type a
result | Always runs 3 × cycle (180 s). |
| Enable `Test_003` needs every alarm type to go active within cycle/10
| Timed out in 6 of 7 runs, because boolean and analog alarms changed
state on different ticks. |
| Enable `Err_004` answers each event with Disable/Disable/Enable |
Feedback burst, after which the CTT alarm thread sometimes returns no
events. Details below. |
| Comment `Test_001`–`004` compare against
`CallHelper.ServerTimeOfCall`, which the CTT never sets (`0001-01-01`) |
Every comment event counts as unrelated, and the tests skip after 3
retries. The server's events carry the expected comment. |

Measured CU by CU, the group took **about 87 min plus the hang**. With
the fixes below, running the whole group in one CTT process without
CertificateExpiration takes **18:45** at Alarm Cycle Time 30. The per-CU
table is in `plans/ctt-testing.md` §6.

## Server changes

- **Refresh one subscription per request, not one per Call**
(`AlarmNodeManager.CallAsync`). The sample answered every
ConditionRefresh/ConditionRefresh2 after the first one in a Call with
`Bad_RefreshInProgress`, even for other subscriptions, and never
forwarded them. So CTT Refresh `Err_004` (10 subscriptions per Call)
only ever refreshed one of them. The check is now keyed by
SubscriptionId and MonitoredItemId.
- Regression tests:
`AlarmsAndConditionsRefreshTests.ConditionRefresh[2]OfDifferentSubscriptionsInOneCallSucceedsAsync`.
Both fail without the fix.
- **Boolean alarm source now uses the analog source's steps and limits**
(`AlarmController`, `ConditionTypeHolder.GetSeverity`). OffNormal,
Discrete and SystemOffNormal alarms now change state on the same tick as
the limit alarms (5 per second, 70/90/30/10), and the shortest state
still lasts 4 s.
- With this, Enable `Test_003` passed in 4 s instead of a timeout.
Confirm `Test_001` for the boolean types also stopped failing in both
group runs.
- Test:
`AlarmControllerTests.BooleanAndAnalogSourcesChangeStateOnTheSameStep`
(fails without the change).
- **Alarm controllers step on shared interval boundaries**
(`AlarmController.SetNextInterval`). Scheduling relative to the last
step let the two controllers drift up to one 100 ms simulation tick
apart, so the boolean events sometimes landed in a later publish and
Enable `Test_003` still timed out in 3 of 5 runs. With shared boundaries
it passed in 6 of 6 runs.
- Test:
`AlarmControllerTests.ControllersScheduleTheirStepsOnSharedIntervalBoundaries`.

I did not make the simulation faster. Acknowledge, Confirm and Comment
tests call methods with the EventId of the event they just received, and
a shorter state would make them race the next transition.

## Not reproduced / not a server defect

- **Refresh `Err_004` `BadSubscriptionIdInvalid` cascade.** In the
original run the alarm-thread session's subscriptions (15.5 s lifetime)
were gone after the script's 30 s wait. Nine further CTT runs passed all
28 Refresh/Refresh2 cases. An in-process replay with a sequential
publisher kept every subscription; that replay is what exposed the
refresh defect above.
- **Enable event stall (C43) is inside the CTT.** A server build that
logs every returned notification message showed that during a stall the
server sent 435 and 420 events to the two CTT event items in 173
messages, and the CTT acknowledged all of them, while its alarm-thread
buffers returned nothing. It only happens after `Err_004`'s
Disable/Enable feedback burst, in 8 of 28 runs.

## Docs

- `plans/ctt-testing.md`: new §6 *Alarms and Conditions*, covering the
recommended run, where the time goes, how to instrument a run and the
expected result. Also a note that a hidden CTT run can block on dialogs,
so always use a timeout.
- `plans/ctt-issues.md`: the A&C "open server findings" are replaced
with these results, and there are new CTT script defects C39–C44 plus
configuration notes (Shelving chattering alarms, Alarm Cycle Time vs
session timeout).

## Tests

- `Opc.Ua.History.Tests` A&C + AlarmClient: 155 passed.
- `Opc.Ua.Server.Tests` `AlarmControllerTests` /
`QuickstartsServerUtilitiesTests`: 4 passed.
- CTT whole-group runs (27 CUs, 128 cases): 18:45 at cycle 30 and 29:23
at cycle 60 on this branch; 23:43 and 24:21 with only the refresh fix.
The spread comes from the Enable stall (C43).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: romanett <7413710+romanett@users.noreply.github.com>
 2026-09-15 14:44:36 +00:00
romanettandClaude Opus 5 8e874d073b Revise event monitored item queueSize 0/1 per Part 4 §7.21 (fixes CTT Auditing Connections) (#4480) 
## Problem

CTT **Auditing Connections** (CTT 1.05.06, scripts 1.05.513,
`ConsoleReferenceServer --ctt -a`) fails nondeterministically:

- `TestFindActualFromExpected - Unable to Find Entry for
ClientAuditEntryId <id>` (011.js, 012.js, 007.js, 020.js, varying by
run)
- `TestFindActualFromExpected - Unable to Find an Entry for
AuditEventType: i=2060` (and i=2069/i=2071), reported as Not Supported

## Root cause

The CTT audit subscription (`library/Base/auditThread.js`) monitors
`Server.EventNotifier` with the CTT's default `QueueSize = 1` and a 250
ms publishing interval. For **event** monitored items Part 4 §7.21
defines queueSize 0 as "the Server default" and 1 as "the minimum queue
size the Server requires for Event Notifications". `EventManager` only
clamped the maximum and `MonitoredItem.InitializeQueue` mapped only 0 to
1000, so 1 became a literal one-entry discard-oldest queue
(`RevisedQueueSize = 1`).

A single connect raises OpenSecureChannel, CreateSession,
ActivateSession and CloseSession audit events within milliseconds, and
in `--ctt` mode alarm events share the queue. Every event except the
last one raised between two publishes was dropped and replaced by an
`EventQueueOverflowEventType`.

The audit event content itself was correct. The events that survived
carried exactly the `ClientAuditEntryId` the CTT put in
`RequestHeader.AuditEntryId`.

## Evidence (instrumented CTT copy logging `Test.Audit.GetBuffer()`)

| Server | RevisedQueueSize | CTT buffer | Auditing group result |
| --- | --- | --- | --- |
| master | 1 | 16 events, **7 × i=3035 EventQueueOverflow**, 0 ×
OpenSecureChannel (2060), 0 × CloseSession (2069) | 18 pass, 2 Error + 1
NotSupported (007/020/001) |
| this PR | 1000 | 42 events, no overflow, every expected audit type
present | 21 pass, 0 errors (official project, uninstrumented) |

Each gap in the master buffer is followed by an overflow event. For
example, OpenSecureChannel at .553 is overwritten by CreateSession at
.617, and CloseSession at 08.307 by CloseSecureChannel at 08.312.

## Fix

- `EventManager.CalculateRevisedQueueSize`: requested queueSize 0 and 1
are revised to the new `EventManager.DefaultEventQueueSize` (1000), then
limited by `MaxEventQueueSize`/`MaxDurableEventQueueSize`. This applies
on create, modify and restore, so `RevisedQueueSize` reports the real
size.
- `MonitoredItem.InitializeQueue`/`RestoreQueue`: event items with
queueSize ≤ 1 use the same default. Previously only 0 did; data change
behaviour is unchanged.

## Tests

- New `EventManagerTests`: revision of 0/1, clamping to max, modify with
1, and a behavioural test that a 3-event burst on a `QueueSize = 1` item
publishes all 3. Before the fix it published 2: the last event plus the
overflow event.
- `Opc.Ua.Server.Tests`: 5558 passed / 0 failed (net10.0)
- `Opc.Ua.Subscriptions.Tests`: 619 passed / 0 failed / 6 skipped
(net10.0)
- `Opc.Ua.Core.Security.Tests` (Audit filter): 88 passed / 0 failed / 3
skipped

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
 2026-09-14 10:15:21 +02:00
Marc Schier 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00