GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-21 09:06:37 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,739 Commits 110 Branches 105 Tags
fix/encoder-stack-depth-json-flake
Commit Graph

9 Commits

Author SHA1 Message Date
romanett 9b034f8291 Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) 
# Description

Nullable reference types are now enabled once, in `common.props`, and
every project in the repository builds warning-clean with them.

**Configuration**
- `<Nullable>enable</Nullable>` lives in `common.props`. All per-project
`<Nullable>` settings are removed: 178 project files (enable,
annotations, and the `disable` in the three Aot test projects).
- 419 per-file `#nullable enable`/`restore` directives and the 4
`#nullable disable` directives are removed.
- Kept on purpose:
- The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type
Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`,
`PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the
project setting in generated code, so the directive is required there.
- `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against
the 1.5.378 packages and does not import the repository props.
- `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now
describe the central setting.

**Migration** (the 43 unset and 13 annotations-only projects, roughly
10k diagnostics)
- **Production code: annotated at the declarations.** Covers
`tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the
LDS sample.
- Members, parameters and returns are nullable where null actually
flows.
- Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`,
`IsMethodTypeDesign`).
- `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take
`params object?[]`. `Template.AddReplacement` takes `string?` (rendering
already skips null replacements).
- `IRegisteredServerStore.RegisterAsync` returns
`Task<RegistrationEntry?>`: it returns null when a registration is
removed.
- Dereferences backed by an invariant (for example, the validated model)
use `!`, so runtime behaviour is unchanged.
- **Behavioural changes in production code** (small, deliberate):
- `NodeSetToModelDesign` throws `InvalidOperationException` when the
file is not a NodeSet. Before, it threw a NullReferenceException on the
next line.
- `ModelDesignValidator` starts its lookup tables empty instead of null.
A redundant `if (m_nodes == null) return;` guard is gone.
- **Test code: minimal annotations.**
  - `?` on values that can be null, `!` where the test asserts non-null.
  - `= null!` for fixture members assigned in SetUp.
- `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the
`IEncodeable` constraint.
- A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`)
folded into the adjacent `IsNull`/`Count` checks.

**Notes for reviewers**
- The diff is large (~1,500 files) but almost entirely mechanical: `?`,
`!` and directive removal. The non-annotation changes are listed above.
- Known follow-up, not done here:
`IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay
non-nullable even though the validator returns null before it loads a
dictionary. Making them nullable would ripple through every generator
call site.

## Related Issues

- Fixes #4439

## Checklist

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

The "no new warnings" box covers net10.0 and net48: full non-incremental
builds of `UA.slnx`, `tools/SourceGeneration.slnx`,
`tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0
warnings and 0 errors under the repository's warnings-as-errors
settings. net9.0 and net8.0 also had no nullable diagnostics, but they
were not rebuilt after the final fixes. Tests have not been run locally
yet.

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

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-08 08:48:13 +02:00
romanett 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 Schier 138086863b Fix audit findings and review regressions across the OPC UA stack (#4503) 
## Scope

Audit fixes and review follow-ups across the OPC UA Core, Client,
Server, transport, and redundancy components. This includes implemented
Tier 1, Tier 2, and Tier 3 work; it does not claim that every item in
the 244-finding audit backlog is complete.

The review batch fixes mandatory full-key record protection and owned
plaintext buffers; transactional pending-certificate claims;
shared-channel nonce and certificate ownership; discovery and
token-policy validation; reconnect/callback ordering without changing
unbounded buffering defaults; server permissions, filters,
monitored-item lifetime, and alarm topology; historian paging,
retention, and annotations; atomic user/password metadata; and canonical
file identities with bounded alias and role refresh workers.

## Review-batch verification at `ab686ccae`

The following results come from fresh test builds using `dotnet test`,
`--no-restore`, `-m:1`, and an explicit `CustomTestTarget`. They replace
the earlier description's library-build/`--no-build` totals, which did
not establish the state of the merged test sources.

| Scope | Target | Passed | Skipped |
|---|---|---:|---:|
| Core review regressions | net10.0 | 827 | 0 |
| Client and lifecycle regressions | net10.0 | 630 | 0 |
| Server review regressions | net10.0 | 1970 | 0 |
| Redundancy Server / Client / PubSub | net10.0 | 276 / 10 / 11 | 0 |
| Sessions / file-and-alias / streaming / HTTPS-WSS integration |
net10.0 | 8 / 21 / 30 / 20 | 0 |
| Core review regressions | net48 | 786 | 39 |
| Client recovery and policy regressions | net48 | 54 | 0 |
| Server ownership, aliases, queues, and user-state regressions | net48
| 137 | 0 |

Every listed run exited successfully. The 39 net48 skips are existing
platform/capability cases. New key-agreement tests assert
unavailable-policy behavior on unsupported targets rather than adding
skips or pretending those targets performed an unsupported handshake.
Additional exact node/permission/filter/alarm checks passed 13 Core and
46 Server cases.

The PR-scoped formatter verification covers 26 projects and 389
non-generated C# files. A Roslyn syntax audit covers 53 changed
interfaces and 263 members, with no missing
parameter/type-parameter/return contracts and a 120-column limit for
their declarations and documentation. No full-solution,
all-target-framework, or NativeAOT matrix is claimed from these local
runs; the new CI run must establish its own result.

## CI follow-up at `e1bc3ac3b`

The follow-up fixes the failures reported by Azure build 18400. TLS
certificates retain configured DNS names, including mixed-case URI
schemes, without relaxing hostname validation. Reverse-WSS fixtures use
the DNS identity in the client certificate. Reconnect completion is
published only after cycle cleanup, so a subsequent reconnect cannot
join an old completed cycle and retain old certificates.

Certificate-alarm roots now support event subscriptions. The pump sample
has one continuous notifier path, with no duplicate shortcut. Historian
and aggregate fixtures share the provider's retention clock. Alarm
collection lets the server expire idle Publish requests rather than
canceling only the client wait. The corrected server, WoT, and
managed-session fixtures exercise valid requests and initialized
lifecycle contracts. Downlevel crypto tests preserve real credential and
key-ownership assertions without enabling unsupported raw-ECDH policies
or adding skips.

Fresh local evidence after integration includes:

| Scope | Target | Passed | Skipped |
|---|---|---:|---:|
| Client lifecycle / channel ownership / builder fixtures | net10.0 and
net48, each | 96 / 103 / 123 | 0 |
| Combined server lifecycle, events, aggregates, crypto, and contracts |
net48 | 358 | 0 |
| Full lifecycle plus certificate-alarm and aggregate fixtures | net10.0
| 116 | 0 |
| Final server-contract and history-routing delta | net10.0 and net48,
each | 84 | 0 |
| Subscription / XRegistry / WoT event regressions | net10.0 | 7 / 2 / 3
| 0 |
| Full Sessions project | net10.0 | 780 | 379 |
| Full ComplexTypes project / downlevel resolver fixtures | net10.0 /
net48 | 2854 / 15 | 0 |
| Full WoT project / downlevel structured-write fixtures | net10.0 /
net48 | 1496 / 27 | 0 |
| TLS domain-name regressions | net10.0 and net48, each | 21 | 0 |

All listed runs exited 0. The Sessions skips come from existing endpoint
and policy capability guards; they are not counted as passes. No skip
was added. Worker validation also covered full DI (426 passes), full
History (540 passes, 21 existing skips), 38 exact downlevel
event/history failures, and 60 repeated certificate-rotation race cases.
The final crypto fixtures passed 73 cases on each target with the
unchanged leak detector; a temporary premature-disposal mutation made
all three borrower tests fail and was then removed.

All 29 C# files in this follow-up pass included-file formatting at info
severity across 12 projects. The complete PR interface audit now covers
54 interfaces and 271 members with zero missing contracts or line-width
findings. No full-solution or NativeAOT matrix is claimed. New-head CI
has started and remains authoritative for the remote result.

## Test-result gate follow-up at `4b6e1203a`

Azure build 18401 reported the net48 Client job as successful despite a
test-host crash after only 264 passing tests. Its old evaluator counted
per-test failures but ignored the failed run verdict. This was a partial
run, not a benign exit after the suite had completed.

The shared result evaluator now rejects aborted, errored, incomplete,
malformed, or inconsistent TRX reports. Each file must describe a
completed run, so a passing file cannot mask another file's failure. It
retains support for completed runs followed by a benign nonzero host
exit, existing skipped-result conventions, and no-TRX MTP jobs.

All 78 regression cases pass after integration using Windows PowerShell
5.1. Real completed Sessions and Client reports also pass. The
process-test fixture follows the existing NET10_0-only tools-test
pattern; its net48 build is not counted as a test run. Formatting passes
for the added test file.

The Client host crash itself has not reproduced locally. The isolated
fixture passed 59 tests, and two fresh full fast-CI selections each
completed with 2,392 passes and three existing skips, including a run
with crash collection enabled. The CI merge tree is identical to the
tested source tree. No speculative runtime change was made, and the
original partial CI run is not claimed as a full-suite pass. Azure build
18402 has now completed the full net48 Client run: 2,392 passed, three
existing console skips, and a successful test-host exit. The corrected
evaluator independently validated outcome=Completed. Across the matrix,
119 test-project jobs succeeded; Sessions net10.0 was canceled when the
remote provider deprovisioned its runner, so the overall Azure pipeline
was canceled. That cancellation is not claimed as a code failure or a
successful run. CodeQL and three Docker image jobs were still running at
the latest check. The migration discovery failure remains separate.

## Documentation follow-up at `57ed02bea`

The environment blocker has been removed by the maintainer. The three
bundled migration references now exactly match the canonical documents
after deterministic link rewriting; all 15 bundled documents pass
`validate-migration-plugin.ps1`. The canonical migration prose was not
rewritten. `FileSystemClient.md` now documents that extended Windows
roots preserve literal trailing-dot names, completing the filesystem
review follow-up.

The filesystem thread has been replied to and resolved after
publication. The review pass now has 108 of 110 threads resolved. The
two explanation-only threads remain open as requested; no unfinished
change request is being represented as resolved.

## Cross-platform follow-up at `0d9d936a8`

With migration discovery restored, the Linux and macOS test matrix
exposed two stale durable-subscription expectations. The expected value
83,442 encoded the old 32-bit overflow when converting 3,600 hours to
milliseconds. The existing overflow-safe server correctly returns
14,400,000 lifetime intervals at a 900 ms publishing interval. Only
those two expected constants changed; production behavior,
negotiated-hour checks, and all other assertions remain unchanged.

The exact local lifetime selection reproduced both failures before the
correction. The full durable suite now passes 36 cases with 11 existing
skips on each of net10.0 and net48. All four lifetime cases executed and
passed on both targets, and included-file info-severity formatting
passes.

## Session shutdown follow-up at `8daf7f091`

A late net48 Sessions failure exposed a shutdown race. Canceling an
in-flight keepalive read can produce a transport
`ServiceResultException`. The retiring worker treated its own
cancellation as proof that the peer was unavailable, so `CloseAsync` or
`DisposeAsync` could skip the server-side `CloseSession` request. The
worker now ignores that service error only when its own token is
canceled; genuine peer errors still update liveness.

The full integration fixture reproduced the original error-code
mismatch. Four deterministic mock-transport cases then produced three
failures and one passing genuine-error control before the fix. They
verify successful close, returned server error, asynchronous disposal,
and unchanged behavior for a real keepalive failure. Fresh related runs
pass 75 client and 101 integration cases on each of net48 and net10.0,
with no skips. Ten repetitions of the formerly flaky full net48 fixture
pass all 1,010 cases. All three changed files pass info-severity
formatting, and temporary diagnostic instrumentation was removed.

## Atomic listener snapshot follow-up at `82dc22b5d`

Ubuntu Core CI exposed an unsafe formatter rewrite in TCP listener
disposal: `ConcurrentDictionary.ToArray()` had become a collection
spread. A concurrent channel removal between the spread's count and copy
operations could leave a default entry, causing a null dereference
during disposal. The listener now uses the dictionary's atomic `Values`
snapshot and disposes those channels outside the listener lock.

Deterministic regressions close zero, one, or all channels at the copy
boundary. Before the fix, the one/all-removal cases reproduced the exact
null dereference; the unchanged-count control passed. All 23
admission/listener cases now pass on net10.0 and net48, and the full
fast Core net10.0 suite passes 5,267 cases with 88 existing platform
skips. Both changed files pass info-severity formatting. A bounded audit
of the formatter's other snapshot rewrites found no further verified
unsafe concurrent-registry copies.

## Shutdown-test observation follow-up at `58403ee8b`

The net48 shutdown-order test raced its own observation: it read
`m_publishWorkerTask` after starting shutdown, which can legitimately
drain the worker and clear that field immediately. The test now captures
the worker before shutdown starts and confirms it is initially running.
It still requires the worker to finish while shutdown remains blocked by
the held manager semaphore. Production behavior is unchanged.

A temporary scheduling perturbation reproduced the exact null-field
failure. The corrected capture passed 100 perturbed iterations; both the
temporary repeat and yield were then removed. The final 126
subscription/lifecycle cases pass on each of net48 and net10.0, and
formatting passes. A complete local Sessions net10.0 run also passed 780
cases with 379 existing capability skips in 7m20s after the earlier
keepalive correction.

## Retirement and alias-startup follow-up at `1a048ff39`

The shadow-generation test now observes its documented asynchronous
cleanup through the existing bounded helper. Removing the replacement
does not join a background drain that already claimed the retired
generation. The test still checks live-registration removal immediately
and requires the old node to disappear; no timeout or production
lifecycle behavior changed. All 81 lifecycle cases pass on each of
net10.0 and net48.

Initial alias-node materialization now retries one timed-out, read-only
snapshot query before creating nodes for that root. The retry is logged
through source-generated logging. Each query retains the original shared
100 ms deadline; repeated timeouts, other errors, and cancellation still
abort startup. Client searches and live refresh do not gain this
immediate retry. Fault-injection tests cover success after one timeout,
a persistent timeout, a different store failure, and cancellation. All
187 related alias/hosting cases and the original certificate-rotation
integration case pass on each target; the positive test records exactly
one retry warning. The alias documentation now accurately describes the
shared request budget.

All changed C# files pass formatter checks, and the complete
54-interface/271-member documentation audit passes. Fresh CI is running
on `1a048ff39`; the previous completed matrix exposed only the two
failures corrected in this batch and their dependent summaries.

## Sample endpoint allocation follow-up at `d08a94925`

The strong-cluster test harness allocated OPC UA and Raft endpoints in
separate released-port batches. Uniqueness was enforced only inside each
batch, so the same OS-assigned port could appear in both protocol
groups. The harness now requests one distinct batch for both groups and
splits it before launching any replicas.

Deterministic one- and three-replica allocation cases reproduced
overlapping groups with the old plan and pass with the corrected plan.
The complete 17-case short-haul sample suite passes without skips; all
failover, historian-continuation, and service assertions remain
unchanged. Both changed files pass info-severity formatting. This
prevents intra-cluster self-collisions but does not claim to reserve
released ports against unrelated external processes.

## Code-review follow-up at `085fd9636`

A review of the complete branch diff found five defects this PR
introduced. All are now fixed, each in its own commit.

`RelatedTo`'s recursive-to-iterative rewrite defaulted the optional
`includeTypeDefinitionSubtypes` and `includeReferenceTypeSubtypes`
operands to `false`. Part 4 §7.7.4 specifies both default to `true`, so
a filter that omitted them silently stopped matching subtypes.
Dynamically created key-credential nodes derived their namespace from
the containing folder, which is the standard `i=18155` node in namespace
0 — the namespace reserved for the OPC UA standard address space; they
now resolve the server-owned namespace and fail with `BadInternalError`
rather than guessing index 1. The retained audience-less
`CreateTokenData` overload produced tokens that verification always
rejects, so it is now `[Obsolete]` and throws instead. A failed
reference to a remote parent left the new node registered with a
one-sided inverse reference, and is now rolled back. An existing `.pfx`
is restricted before it is rewritten, rather than after its new key
bytes are on disk.

One further reported finding was rejected after investigation.
`JwksIssuerKeyResolver` deliberately does not dispose retired key
snapshots: `RetiredKeysAreCollectibleWhileResolverRemainsAliveAsync`
proves the resolver retains no retired snapshot, and
`ReturnedKeysSurviveRepeatedRefreshAndResolverDisposalAsync` requires
already-returned keys to keep verifying across refreshes and resolver
disposal. Adding disposal would have broken that tested contract, so the
exploratory change was reverted.

Validated on net10.0 and net48: 256 Core and 633 Server cases pass, with
one pre-existing net48 skip. The build reports zero compiler and
analyzer diagnostics, all eight included-file info-format checks pass,
the contract audit covers 58 interfaces and 310 members with no
findings, and the 15 bundled migration documents validate. All 122
review threads are resolved; the key-credential namespace thread was
reopened to retract an earlier incorrect answer and then resolved
against the real fix.
## Review-body follow-up at `da0d59cfb`

The re-review at `8daf7f091 + 82dc22b5d` listed four items in its
summary that had no inline thread of their own, so they had not been
picked up. All four are now fixed.

Both event-dispatch permission gates had been tightened from not-Bad to
`IsGood`, which silently dropped events whose permission verdict is
Uncertain; the data-change path in the same file still treated Uncertain
as deliverable, so the two had diverged. `EventSourceRegistry` only
cleared `ConsecutiveFailures` when a source was deactivated, so a source
that had backed off to the 30-second cap kept that delay for the rest of
the process even after starting successfully.
`UserNameIdentityTokenHandler` built a temporary issuer chain whose
`Add` calls take independent certificate references, and never released
them.

The fourth was the receiver-side sequence rollover, which the reviewer
had correctly re-flagged as still open despite its thread being
resolved. `m_sequenceRollover` was set permanently, so the second legal
wrap on a long-lived channel was rejected and faulted the channel along
with the transport-reconnect OPN. OPC 10000-6 does not cap a
SecureChannel at one wrap. The flag is now cleared once an accepted
number has traversed the normal range, which keeps rejecting a replayed
low number that never left the rollover window.

Validated on net10.0 and net48: 636 / 593 Core cases and 135 / 134
Server cases pass, including the new
`UaSCBinaryChannelSequenceRolloverTests` and the Uncertain-permission
delivery cases. The build reports zero compiler and analyzer diagnostics
and the changed files pass included-file info-format checks on both
targets.

The previous head `085fd9636` was fully green: 279 checks passed and
Azure build 18414 succeeded with 137 of 137 jobs, after the platform
automatically retried a job whose NuGet tool download had hit a
transient TLS error.
## Reported issues #4508 and #4509 at `6ad1ecf16`

**#4508 (V2 subscription recreate deadlocks against a faulted transport)
was not yet fixed and is now closed by `bac260a06`.** Both drain sites
the report identifies were still unbounded. A publish worker that
started its request before the quiesce parks inside the service call on
the channel ready gate while holding an active publish request; the
recreate that would make the channel ready is the very operation
awaiting the drain, so the wait never ends.

The drain now cancels in-flight attempts through a per-attempt token
linked to the worker token. An aborted worker rolls its acknowledgements
back, releases the request and returns to the paused park instead of
terminating, so it resumes when the caller leaves the quiesce. Publish
ingress stays quiesced for the caller's operation, so
`DropPendingForSubscription` keeps the guarantee the report asks to
preserve and no blind timeout is added. The new regression parks a
worker on a cancellable gate that never completes on its own: it
reproduces the hang for the full 30-second budget before the change and
passes after it.

**#4509 (identity provider fails the initial connect) was already fixed
for its primary symptom.** `HandleConnectAsync` materializes the
provider's identity through `CreateIdentitySelectionContext` before the
first `CreateAsync`, so Anonymous is no longer substituted against a
UserName-only endpoint. `6ad1ecf16` adds the two remaining details from
the report: the enabled-policy set now includes the negotiated channel
policy, so a client cannot reject the policy it already accepted as
`NotEnabledByClient`; and when discovery has not populated the
endpoint's token policies the connect keeps the configured identity so
the attempt fails at the transport layer for the reconnect policy to
retry, instead of failing at the identity layer.

Validated on net10.0 and net48: 477 / 473 client session and
subscription cases pass, the build is free of compiler and analyzer
diagnostics, and the changed files pass included-file info-format checks
on both targets. The full `Opc.Ua.Sessions.Tests` suite also passes
locally on net10.0 with 782 passed and 379 pre-existing capability
skips.

GitHub checks are green on this head: Build and Test .NET 10.0, Docker
Sample Images CI, and NuGet Publish all succeeded. Azure build 18417
completed 134 of its 135 jobs successfully; its net10.0 Sessions job was
cancelled by the hosted-agent provider ("Received request to
deprovision"), the same infrastructure flake that hit build 18409
earlier on a head predating these changes. That job is not claimed as
passing, and the local full-suite run above covers the same project. The
previous head `da0d59cfb` completed Azure build 18415 with every job
succeeded.
## Review follow-up at `a08f0c12b`

Eleven new review threads arrived, nearly all documentation. Before
acting on each "this is new in 2.0" remark I checked the API against
`master378` rather than assuming, because a genuine 1.5 API still needs
its migration entry.

`ManagedSession`'s server-redundancy refresh had two hard-coded
two-second bounds. The new `ServerRedundancyOptions` exposes
`RefreshTimeout` for the metadata read and `PeerDiscoveryTimeout` for
each peer lookup, defaults both to the previous two seconds, validates
that a bound is positive or `Timeout.InfiniteTimeSpan`, and flows
through `ManagedSessionOptions`, `ManagedSession.CreateAsync`, the
fluent builder, and `DefaultServerRedundancyHandler`.

`AsyncCustomNodeManager` and `KeyCredentialBridgeAuthenticator` are both
absent from `master378`, so their obsolete members had no compatibility
constraint and are removed rather than carried:
`AsyncCustomNodeManager.ReadInitialValue`, the audience-less
`CreateTokenData` that only threw, and the audience-less `CreateProof`
that produced a proof no server accepts.
`CustomNodeManager2.ReadInitialValue` and the `MonitoredNode2` migration
section are genuine 1.5 APIs and are untouched.

The documentation now states master's behaviour instead of the delta
from 1.5.378, with each moved item already covered under
`docs/migrate/2.0.x/`. `IHistorianAnnotationProvider` is described by
its annotation-time key rather than as "legacy" — it is new in 2.0, so
the label implied a compatibility constraint that does not exist. The
KeyCredential bridge payload's `"version"` field is explained as a
vendor-extension format version with no OPC UA counterpart, and its
version-1 upgrade narrative is gone. `CertificateManager.md`'s migration
walkthrough is replaced by the resolver API it targets.
`NodeManagers.md`, `Sessions.md`, `AsyncServerSupport.md`,
`Certificates.md`, and `Profiles.md` no longer announce features
relative to 1.5. `Benchmarks.md`, `WhatsNewIn2.0.md`,
`DeveloperGuide.md`, and `docs/migrate/` keep their 1.5.378 references,
which are their subject.

CI then exposed a defect this batch introduced: five fixtures resolve
`ManagedSession`'s private constructor by its exact positional parameter
types, so the added parameter compiled cleanly but made `GetConstructor`
return `null` at run time. `7df1d5c40` updates every site;
`Redundancy.Client` passes 126 of 126 and the affected client fixtures
112 of 112.

The BouncyCastle question is answered and tracked in #4510:
`Opc.Ua.Core` already project-references `Opc.Ua.Security.Certificates`,
which already carries the conditional BouncyCastle reference, so the
raw-ECDH polyfill belongs there rather than as a new Core dependency.
The `IUserDatabase` metadata question is addressed to another maintainer
and is deliberately left open.

## Reported issue #4509 follow-up at `5d4d13d63`

The reporter verified `6ad1ecf16` and found one case still failing,
correctly. The empty-token-policies branch kept the configured identity
so a server that is down surfaces a transport failure rather than an
identity error. A provider-only configuration has no configured
identity, so it kept `null`: `OpenCoreAsync` turned that into Anonymous
and `OpenValidateIdentity` rejected it with `BadIdentityTokenInvalid`,
silently ignoring the configured provider.

The branch now requires a configured identity to defer to. Without one
the provider is consulted, so its own `BadIdentityTokenRejected` names
the offered and rejected policy set instead. Two regression tests cover
both configurations; the first fails against the previous behaviour by
reaching the session factory with a `null` identity, which is the exact
step that produced the reported error. That branch had shipped without a
test, which is why the case was missed.

## Review and CI status at `5d4d13d63`

**All actionable review threads are resolved: 132 of 133.** The single
open thread is a question the reviewer addressed to another maintainer
about moving user metadata into `IUserDatabase`; it is left open for
them rather than resolved silently. Every resolved thread carries a
reply naming its fix commit.

**Latest-head CI is green on `5d4d13d63`: 279 checks passed, zero
failed, zero pending**, with one conditional signed-package publishing
job skipped. Local validation for this batch: the full
`Opc.Ua.Client.Tests` suite passes 2,430 of 2,430 on net10.0,
`Opc.Ua.Redundancy.Client.Tests` 126 of 126, and `Opc.Ua.Server.Tests`
590 of 590 for the affected selections. Both net10.0 and net48 build
clean for `Opc.Ua.Client`, `Opc.Ua.Server`, and the changed test
projects, and all changed files pass included-file info-severity
formatting on both targets.

The PR remains open; no merge was performed. As stated at the top, this
does not claim that every item in the 244-finding audit backlog is
complete.
## Latest CI follow-up at `b6f2dfa43`

Azure build 18442 exposed a net48 compile regression in the Kafka
adapter: `RuntimeHelpers.GetUninitializedObject` is unavailable on .NET
Framework. Commit `2cf1e0eed` now uses normal `ProducerMessage`
construction on `NETFRAMEWORK` while retaining reflection-based
construction for modern targets that can load a different Dekaf runtime
asset. The exact net48 CI selection passes 92 of 92 tests, and the
netstandard2.1 adapter guard selection passes 11 of 11.

After merging the latest `master`, GitHub Actions run 35643592139 is
green. The initial Ubuntu Core.Schema job built successfully and passed
160 of 160 tests, then hit an external 403 while finalizing its
test-result artifact; its failed-job rerun completed successfully. Azure
build 18451 was canceled when its remote provider deprovisioned the
net10.0 Sessions runner (`Received request to deprovision: The request
was cancelled by the remote provider`). Azure attached no test task log
or source error, and the canceled parent left that child check stale as
in-progress. No source workaround, test skip, or check relaxation was
made for either infrastructure failure.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 68f26810-22b8-476f-abcd-0a8c82d6e208
 2026-09-22 09:10:58 +02:00
Marc Schier 7411654c8d Complete OPC UA Part 11 Historical Access (#4390) 
# Description

Completes the OPC UA Part 11 Historical Access work requested by #4387
and
the historian framework integration requested by #4400.

The conformance/evidence catalog covers all 37 released UACore 1.05
Historical Access profiles: 15 Server facets and 22 Client facets,
together
with the standard aggregate functions and their conformance units.
Server
facets are advertised only when the resolved providers expose their
required
interfaces and capabilities.

## Server and provider support

- Completes raw, modified, at-time, processed, annotation, structured,
and event history.
- Dispatches best-effort, bulk, and transactional updates with aligned
statuses, old values, diagnostics, and rollback state.
- Supports mixed history-update detail types and batched
structured/annotation operations.
- Uses composite identities and exclusive cursors for same-timestamp
structured, modified, and event values.
- Captures live data and events, installs historical configuration
objects, and emits typed audit payloads.
- Derives aggregate monitoring filters from per-node historian
capabilities and rejects unsupported processed aggregates before
provider dispatch.
- Supports tri-state `Historizing`: set, clear, or preserve
provider-owned state.

## Client support

- Exposes modified values together with `ModificationInfo`.
- Adds event-history reads and event insert/replace/update/delete
operations.
- Adds structured and annotation batching with strict response
validation.
- Shares continuation handling and release-on-abandon across read
families.
- Accepts inherited standard event fields rooted at event subtypes.

## Review hardening

- Registers DI-selected historians before node-manager reconciliation
and capability publication, while preserving provider ownership.
- Uses committed Raft read barriers for linearizable `TryGetAsync` and
`ScanAsync`.
- Keeps claimed continuation state immutable until a fresh successor is
saved, restoring the original after transient failures.
- Uses exact-incarnation compare-and-delete so delayed cleanup cannot
remove a restored same-ID continuation.
- Cleans up indeterminate initial and replacement saves, and propagates
shutdown cancellation through cleanup resolution.
- Preserves legacy continuation envelopes/cursors and distinct
annotation parent/property identities.
- Deletes every structured entry at a requested timestamp and uses the
complete modified-history ordering tuple.
- Shares native/fallback at-time and AnnotationCount calculations,
including reverse endpoints and cross-framework fractional intervals.
- Primes modified aggregate items before buffered live delivery and
deduplicates history/live overlap using provider-specific identities.
- Keeps committed Modify results successful while reporting priming
failures
  through the monitored item's notification queue.
- Keeps protected-notification state live-only for queues larger than
one.
Queue size one retains the newest notification without pinning errors or
transferring priority during resizing. Durable restore retains ordinary
definitions, last values/errors, and raw queues without recreating
priority.
- Isolates aggregate calculation, modification preparation, and
historical/live
handoff in `AggregationFilterHandler`, retaining existing
provider/factory
wiring and supported past-start behavior. The larger-queue protection
policy
  is unchanged pending the open review discussion.
- Uses one `IMonitoringFilter` owner for protocol filters or the
aggregation
handler, eliminating parallel effective-filter storage and retaining the
current definition during preparation. The original client filter stays
distinct.
- Integrates current master, retaining custom monitored-item creation
decisions and per-node history callbacks.

## Redundant-server alignment and samples

- Adds a protected shared historian with immutable segments, manifest
CAS publication, writer fencing, and restart recovery.
- Persists portable continuation state before exposing it to clients and
transfers ownership during mirrored-session takeover.
- Rejects eventual-consistency and active/active multi-writer historian
configurations rather than silently degrading.
- ReferenceServer and ConsoleReferenceClient demonstrate discovery,
paging, raw/modified/at-time/processed reads, annotation/structured
CRUD, event capture, and event CRUD.
- RedundantServer and RedundantClient demonstrate active/passive
history, continuation recovery after replica termination, and
post-promotion writes.
- Updates Historical Access, High Availability, profile, migration, and
sample documentation.

## Compatibility

- New 2.0 collection boundaries use `ArrayOf<T>` and public byte
payloads use `ByteString`.
- Removes the legacy public virtual
`SamplingGroupManager.CreateMonitoredItem` /
`ModifyMonitoredItem` entry points and protected creation factory, as
agreed
  in review. Migration guidance points to the monitored-item manager and
  construction-decision APIs.
- Retains established persisted formats and current write layouts.
Retired interim
notification-state formats (sample v2 / shared v4) are rejected, as
agreed in review.
- Keeps the historian integration compatible with .NET Framework and
NativeAOT.

## Validation

### Latest CI source repair

`73c619435` forwards the existing cancellation token at both
predefined-node
creation calls in the PR-added History event fixture. The merged
lifecycle
overload exposed these two CA2016 errors, which blocked History jobs,
the
solution build matrix, and CodeQL on GitHub and Azure. The exact
compiler
failure was reproduced locally; the repaired fixture passes 4/4
integration
cases on net10.0 and 4/4 on net48 with clean scoped diagnostics. No
assertions,
tests, coverage thresholds, or CI configuration were weakened.

The repaired History jobs on Linux/macOS and all four Azure solution
builds
now pass. The Windows all-TFM GitHub job instead lost its hosted
runner's
connection; no build log was recoverable, and GitHub recorded a
runner-loss
annotation rather than a compiler/test diagnostic. One unchanged retry
was
requested. Remaining checks are monitored with
`gh pr checks --watch --fail-fast --interval 30`; CI is not yet reported
as green.

### Latest base integration

`bd52e5fea` merges upstream `master` at `da7412c42`. The lifecycle
conflict
retains upstream's shared cancellation-aware registration helper and the
historian behavior. The hosted-historian fixture forwards its existing
token
to the new lifecycle overload. GDS project renames and all three AOT
test
hosts are retained, with their project references resolving after the
merge.

| Integrated scope | net10.0 | net48 |
| --- | --- | --- |
| Server lifecycle, fluent authoring/import, historian, monitored-item
and hosting regressions | 732 passed | 732 passed |
| NodeState lifecycle regressions | 216 passed | 216 passed |
| Automatically merged Raft client fixtures | 28 passed | 28 passed |

No selected tests failed or were skipped. The manually resolved files
pass
scoped formatting/analyzer checks. One CA1861 warning remains in the
incoming
`NodeSetImportIntegrationTests.cs`; that file is byte-identical to
upstream
master and was not rewritten or suppressed during integration.

### Latest feedback follow-up

`102954eaa` replaces split filter references with the shared
`IMonitoringFilter`
contract implemented by protocol filters and `AggregationFilterHandler`.
Targeted server regressions pass **103/103 on net10.0 and 103/103 on
net48**;
scoped formatting/analyzers are clean, and the updated XML-documentation
inventory has zero gaps across 188 non-generated PR C# files.

`93c5b5db8` renames `AggregationFilterHandler`, makes it the owner of
the
server-revised aggregate filter while preserving the original client
filter,
and removes the retired interim notification-state readers. Server
regressions
pass **101/101** and shared-store regressions **64/64**, on both net10.0
and
net48, with no failures or skips. Runtime-file formatting/analyzers are
clean.

`5ccf45630` completes the public/internal XML-documentation sweep. The
Roslyn
inventory finds zero undocumented eligible declarations across all 186
non-generated PR C# files; the 110-file documentation slice changes only
XML
comments and spacing. `f17765475` applies the three requested Markdown
edits.

### Earlier feedback follow-ups

`a53950737` removes single-slot notification pinning and extracts
aggregate
implementation from MonitoredItem. Targeted sampling, aggregate
modification,
history/live handoff, queue/lifecycle, restore, and serialization cases
pass
**101/101 on net10.0 and 101/101 on net48**, without failures or skips.
Scoped formatting/analyzers are clean; the focused review found no
significant
issues. The final formatted monitored-item fixtures also pass 45/45 on
net48.

The preceding `b21555568` removes the legacy sampling API bridge and
durable
notification-priority state. Its targeted Server cases passed 97/97 and
shared
subscription-store cases passed 64/64 on each framework. The broader
evidence
below remains pinned to its original tested commit, not attributed to
these
follow-ups.

### Earlier integrated-implementation validation

Validated implementation commit: **`3efae597f`**. The following commit,
`f6dbefb1c`, only adds two explanatory comment lines for the final
standards
review; it changes no executable code.

| Scope | net10.0 | net48 |
| --- | --- | --- |
| Server | 5,187 passed; 56 skipped | Affected surface: 2,160 passed; 1
reproduced baseline failure; 56 skipped |
| Client | 2,202 passed; 2 skipped | 2,204 passed; 5 skipped |
| History integration | 519 passed; 21 skipped | 519 passed; 21 skipped
|
| Redundancy server | 641 passed | 641 passed |
| Redundancy client | 124 passed | 124 passed |
| Core shared-store regression suite | 23 passed | 23 passed |

All six affected samples passed non-incremental Release/net10.0 builds
with
zero warnings and errors: ConsoleReferenceClient,
ConsoleReferenceServer,
RedundantServer, RedundantClient, MinimalBoilerServer, and
PumpDeviceIntegrationServer.

The published self-contained win-x64 NativeAOT executable is 116,924,928
bytes and has no CLR header or managed runtime dependency. Direct
execution
passed **HistoryAotTests 3/3** and **RaftAotTests 2/2**.

Final standards and specification reviews found no confirmed outstanding
high/medium functional gaps or hard standards violations. The remaining
constructor-purpose clarification is addressed in `f6dbefb1c`.

### Reproduced baseline exceptions

The net48 hosted-startup timeout also reproduces in a pristine worktree
at
the previous PR head, `24cf1416d`, in

`RegisterPostStartRegistriesWiresHistorianAndAliasStoresOnPlainServerAsync`,
with the same subsequent certificate-leak assertion. The corresponding
renamed case fails before startup in the reviewed branch as well. This
is
reported separately from the net48 affected-surface results, not counted
as
a passing full Server suite. The net48 matrix selected the historian,
aggregate, monitored-item, fluent, and continuation surfaces and
excluded
the Hosting namespace.

`DurableDataValueQueueVerifyReferenceBatchingAsync` failed at item 1000
in
the net48 affected-surface run and passed its single isolated rerun. The
same
failure at the same assertion reproduces on pristine `849e16acc`; the
test,
sample queue, queue factory, and batch persistor are unchanged. The
original
failure remains in the table rather than being erased by the rerun.

### CI source repairs

- `4be7eb8f7` keeps diagnostics/configuration managers out of
distributed
address-space replication. Those managers inherit the source interface,
but their live server state and redundancy metadata must remain
replica-local.
  The ownership regression passes on net10.0 and net48, and all nine
  redundancy sample CI tests pass locally.
- `d17f467b3` completes the sample startup repair: core managers are
also
replica-local; the startup banner no longer advertises readiness before
initialization; the producer starts after distributed services; and
native
Raft startup no longer forces disruptive extra elections. The full
sample
suite passed 9/9, with initialized `RedundancySupport=Hot`, and focused
  Raft/address-space regressions passed 8/8 and 3/3.
- `81b0b72be` moves the unchanged distributed-historian NativeAOT
scenario
into a smaller, always-run companion executable to bound Apple's linker
input. All 137 AOT tests remain present; the moved test and helpers are
  unchanged. GitHub and Azure publish/run both native executables, with
separate result directories where they share a job. The companion passed
actual Windows NativeAOT execution. Linux and Intel macOS CI passed both
  executables, but this split alone did not fix ARM64 macOS linking.
The remaining historian graph is rooted by ReferenceServer startup, not
just the relocated test. No test exclusion or speculative linker setting
  was added to conceal it.
- `8a5842913` isolates the independent MCP test graph into an always-run
native host. The existing MCP source and assembly leak hooks are linked
  unchanged, and evaluated compiler inputs prove exactly one owner for
  every test source: 133 main + 3 MCP + 1 historian = 137 declarations.
All three hosts run on every AOT platform. The main Windows native
object
is 9.35% smaller; both changed publishes had zero diagnostics. MCP
passed
  3/3; the identical main executable passed 133/133 on one unfiltered
followup after two initial endpoint-discovery connection failures, which
  remain recorded in the local evidence.
- The previous Intel macOS job successfully published and executed its
native
tests, then failed on artifact-upload DNS (`ENOTFOUND`). No upload gate,
  shared environment setting, or cache configuration was disabled.

The first repair round also encountered the unrelated
`MetricsAreEmittedForChannelLifetimeAsync` timing failure on macOS Core.
Neither that test nor channel-manager code is changed by this PR, and
its
assertions remain intact. It passed unchanged on the final repair head.

On `d17f467b3`, the Linux and macOS redundancy sample jobs pass; the
remaining
failure was ARM64 Apple's `too many large addends` assertion. The
verified
upstream relocation-anchor fix (dotnet/runtime#124721, backport
dotnet/runtime#132171) is not present in the shipping ILCompiler
10.0.11.
An experimental multi-module probe failed and was rejected, not added to
CI.
On `8a5842913`,

[`aot-macos-latest`](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/34102158680/job/101678934057)
successfully publishes and executes all three native hosts on ARM64:
136 main runtime cases, 1 historian case, and 3 MCP cases, with no
skips.
The previous Apple linker blocker is resolved. Linux NativeAOT also
passes.
No assertion, test, shared environment setting, cache, or required gate
was
weakened.

The same CI run reports a separate macOS Sessions teardown failure: all
662 executed tests pass, but the global leak assertion detects one
undisposed certificate. The MCP partition changes no runtime, Sessions,
or
test-fixture code. A single unchanged diagnostic rerun is queued once
GitHub
permits rerunning that job; the failure is not suppressed or counted as
green.
Other CI checks are still pending.

## Related issues

- Fixes #4387
- Fixes #4400

## Checklist

- [ ] I have signed the CLA and read the contributing guide.
- [x] I have added regression tests and directly related documentation.
- [x] I have retained working reference and redundant-server sample
wiring.
- [x] Final validation and review evidence is complete, including the
baseline exceptions above.
- [ ] All required CI checks pass; the fail-fast rerun for `73c619435`
is pending.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9dbd9527-d818-47a0-984c-fbb1472d3d6b
 2026-09-08 11:32:23 +02:00
romanett ff68296fda Close the SupportsFilteredRetain gaps in the server implementation (#4377) 
Fixes #4370.

`SupportsFilteredRetain` worked for the exact shape the unit tests
exercised, but was unreachable for other filter targets, wrong for
branches, and kept state that outlived the filter it was derived from.
This addresses all seven gaps from the issue.

### 1 — reachable only via `InstanceStateSnapshot`

`CanSendFilteredAlarm` now resolves the condition through a helper that
accepts either a snapshot's `Handle` or a `ConditionState` passed
directly. Anything else still degrades to plain `WhereClause.Evaluate`,
which is now stated in the XML docs instead of left implicit.

### 2 — the commented-out `Retain` guard

Behaviour is unchanged — that is a spec call — but the dead `if` is gone
and the open question is written up in place with a link to the issue,
so it is tracked somewhere more durable than a bare source comment.

### 3 — branches collide on the key

The key is now composed of ConditionId **and** BranchId, so a condition
and its branches no longer contend for one entry.

### 4 — branches never inherit the flag

`CreateBranch` copies `SupportsFilteredRetain` into the branch
explicitly.

### 5 — address-space wiring

I checked the spec before touching this. Part 9 §5.5.2 says the Property
*"is only provided on the ConditionType"*, and `i=32060` in the standard
nodeset accordingly carries no `HasModellingRule` — so the generator and
nodeset are right, and making it an instance child would be the
deviation.

Confirmed experimentally: flipping `StandardTypes.xml` to
`ModellingRule="Optional"` does generate the full property /
`GetChildren` / `FindChild` / `CopyTo` wiring, and its **only**
address-space effect is stamping `ModellingRule_Optional` onto `i=32060`
— which the standard nodeset does not have. That change is therefore not
made here.

The two halves are reconciled the other way instead: the C# property is
documented as the server-side per-condition switch that is deliberately
*not* an address-space child (clients read the flag from the type node,
which the generated address space already builds), with a test locking
that in. Since nothing walks it as a child, #4 is fixed by the explicit
copy rather than by `Initialize`.

### 6 — `ConditionRefresh` bypassed the bookkeeping

Fixed for free by 1. Refresh queues the raw `ConditionState`, which now
resolves, so refreshed conditions are evaluated against the where clause
and re-prime their entries. No new API was needed — the concern in the
issue about refresh mutating the set turns out to be benign: an entry
means "passed the where clause at the last evaluation", so a refresh of
an in-scope condition removes and re-adds the same key.

### 7 — never cleared, never persisted

`ModifyAttributes` discards the state when the where clause actually
changes (select clauses are ignored — they only shape the fields of an
event that is being sent anyway). Persistence added via
`IStoredMonitoredItem.FilteredRetainConditionIds` →
`StoredMonitoredItem` → `ToStorableMonitoredItem` / restore constructor,
plus encode/decode in `SharedKeyValueSubscriptionStore` behind a new
definition format version 3.

## Judgment calls worth a look

- **`SetMonitoringMode` deliberately keeps the state.** A disabled item
never reaches `CanSendFilteredAlarm` at all, so the entries still
describe what the client was last told, and the trailing event is owed
to it once reporting resumes. Documented rather than changed.
- **`IStoredMonitoredItem` gained a member**, which is source-breaking
for anyone with a custom subscription store. `TestStoredMonitoredItem`
in the test suite needed the same one-line addition.

## Tests

`tests/Opc.Ua.Server.Tests/FilterRetainTests.cs` grows from 16 to 26
tests, covering branches, `ConditionState` targets, filter modification
(both changed and unchanged where clause) and a durable round trip. Each
new test was verified to fail against the unfixed code by reverting the
corresponding change individually.

The suite also drops the reflection hazard the issue flagged: a
`TestableMonitoredItem` subclass exposes the protected method, so a
signature change now fails the build instead of the run. Its queue
factory is owned by the fixture rather than disposed inside the call
that hands it to the item.

## Verification

Full solution builds clean. `Opc.Ua.Server.Tests` (4801),
`Opc.Ua.Core.Tests` (4371), `Opc.Ua.Redundancy.Server.Tests` (573) and
`Opc.Ua.History.Tests` (506) all pass on net10.0.

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

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
 2026-09-02 07:02:43 +02:00
Marc Schier 1cca744b5c Retire the last exposed locks: node, browser, server diagnostics and application properties (#4222) 
Closes #4201. Closes #4202. Closes #4203. Closes #4215. Closes #4216.

(#4215 and #4216 are @marcschier's originals from the #4188 review;
#4202 and #4201 are my later duplicates of the same two findings. All
four describe the two changes in section 1 and are closed together.)

**This PR now carries what was #4221 as well.** The two were a stack;
they have been unstacked and combined here, and #4221 is closed.
Everything below the "Read the server diagnostics" section is the work
that was previously reviewed as #4221.

---

## 1. The browse path guards itself, and the last node locks are gone

`AsyncCustomNodeManager` kept two `lock (source)` sites around
`CreateBrowser`, carrying a NOTE that browser creation was not
internally synchronized. `NodeBrowser` still handed its lock to derived
types through `protected Lock DataLock`.

`NodeState.CreateBrowser` now holds a browse lock across
`PopulateBrowser` and `OnPopulateBrowser`, so browser construction on a
node is serialized — which is what the external `lock (source)` was
actually providing, and what a handler that mutates the node during
population (the historian's lazy config installer) depends on. Both
`lock (source)` statements and the NOTE are gone.

**What that lock does not do**, stated plainly in the XML docs and in
`docs/NodeManagers.md` rather than over-promised: it is not an atomic
snapshot across the node's children, notifiers and references. Writers
take those collections' own locks, not the browse lock. The browser is a
point-in-time copy and each collection is read consistently, but the
combination is not a transaction.

No `lock` takes a `NodeState` as its sync root any more. That was the
last thing keeping `NodeState` from being documentably self-guarding, so
the other eleven went with it: `lock (activeNode)`, `lock (target)`,
`lock (source.Node)`, `lock (target.Node)`, `lock (notifier)`, `lock
(handle.Node)` in `AsyncCustomNodeManager`, and `lock (node)` in
`CustomNodeManager2`. Most were a check-then-act pair now served by
`NodeState.AddReferenceIfMissing`, which does the check and the insert
under the node's own lock.

`NodeBrowser.DataLock` is removed. A browser is **single-consumer**: it
belongs to whoever created it, and both server browse paths already
serialize on the owning side. The exposed lock only added an
inheritance-level locking contract that a derived browser could not
reason about.

## 2. Read the server diagnostics under the lock every writer takes

`OnUpdateDiagnostics` locked the diagnostics **payload** rather than the
lock every writer takes through `UpdateServerDiagnostics`, so the
snapshot a client reads from the `ServerDiagnostics` node was never
excluded from a concurrent writer.

Both halves matter, and the second is the one that is easy to miss:

```csharp
lock (m_diagnosticsLock)                                        // was: lock (ServerDiagnostics)
{
    value = Variant.FromStructure(ServerDiagnostics, copy: true);  // was: copy defaulted to false
}
```

`copy: true` is what makes it a snapshot. `FromStructure(value, copy:
false)` does `new ExtensionObject(value, copy)` — it wraps the **live**
structure without copying it, so the caller would read the fields long
after the lock was released and see them change underneath. Swapping the
lock alone would have looked like a fix while still excluding nothing.

## 3. Analyzer numbering

The browser rule is **UA0027**, not UA0025. This branch had
independently taken UA0025 for `NodeBrowser.DataLock` while #4188
published UA0025 for `ILocalNode.DataLock` and UA0026 for
`BaseVariableValue.Lock`. Master's are shipped, so they keep the
numbers:

| | UA0025 | UA0026 | UA0027 |
|---|---|---|---|
| master (#4188) | `ILocalNode.DataLock` | `BaseVariableValue.Lock` | —
|
| **this PR** | `ILocalNode.DataLock` | `BaseVariableValue.Lock` |
**`NodeBrowser.DataLock`** |

Both sides had written a different `UA0025Tests.cs`, which arrived as an
add/add conflict. They are now split: `UA0025Tests.cs` holds master's
`ILocalNode` tests, `UA0027Tests.cs` holds the `NodeBrowser` ones.

UA0024's help text also drops its reference to `ReadServerDiagnostics`,
which #4193 removed as unused, and names `ISession.ReadDiagnostics` /
`ISubscription.ReadDiagnostics` instead — those do still exist.


## 4. Two more locks reachable from outside their owner

**`ApplicationConfiguration.PropertiesLock` is removed.** It returned
the properties dictionary itself, so `lock
(configuration.PropertiesLock)` was `lock (configuration.Properties)` —
the lock *was* the data it guarded, the same shape as #4203.

It had exactly one production consumer
(`OAuth2CredentialCollection.Load`, a get-or-add with `ParseExtension`
called inside the lock), so it could not simply be deleted. `Properties`
is now a `ConcurrentDictionary` — same public type, every operation
atomic on its own — and the one multi-operation need gets a member of
its own:

```csharp
public T GetOrAddProperty<T>(string key, Func<T> valueFactory);
```

It deliberately does **not** invoke the factory under a lock, so
`ParseExtension` now runs outside any critical section. Holding a lock
across a caller-supplied callback is exactly the shape that produced the
attribute/diagnostics deadlock earlier in this work.

The two existing tests asserted only `PropertiesLock is not null`, so
they tested nothing but its presence. They are replaced by tests of the
contract: factory runs once for a missing key, not at all for a present
one, 32 concurrent callers all observe the same instance, null arguments
throw.

`PropertiesLock` ships in 1.5.378, so this is a breaking removal —
migration guide section plus analyzer **UA0028**.

**`SharedDefinitionCache` no longer hands out its lock.** Three sites in
`SharedKeyValueSubscriptionStore` took the nested cache's `Lock` to
operate on the cache's own dictionary. Both are private now, behind
`ReplaceAll` and `CloneWhere`.

`SnapshotCommitLock` stays exposed on purpose: it is a `SemaphoreSlim`
sequencing an async commit across `await` points, which
`System.Threading.Lock` cannot span, so folding it in would change
behaviour rather than hide a lock.
## Verification

Merged `master` (`0edfe237a`, including #4234) with no conflicts.

- `dotnet build UA.slnx` — 0 warnings, 0 errors
- `Opc.Ua.Server.Tests` — 4074 passed
- `Opc.Ua.Types.Tests` — 8501 passed
- `Opc.Ua.MigrationAnalyzer.Tests` — 151 passed on all five TFMs (138 →
151, the new ones being UA0027 and UA0028)
- `Opc.Ua.Core.Tests` — 4249 passed
- `Opc.Ua.Redundancy.Server.Tests` — 573 passed

## Note for the reviewer

@romanett — your approval was given when this PR's diff was the 7
diagnostics files, because it was measured against
`marcschier/node-browse-locks`. Now that the two are combined and the
base is `master`, the diff is 23 files, and the extra 16 are the
node/browser lock work that was #4221. GitHub kept the approval across
the retarget, but it did not cover that code, so please re-review rather
than let it ride.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e9c13dd0-a6e3-4ad2-8ad6-1c3fe6323041
Copilot-Session: b3c0e146-5b9e-4d08-a0f6-db3f940484b1
Copilot-Session: d18f87e9-2484-44ce-98ce-463ba6cf5f63
Copilot-Session: 0ee22c1d-c319-4257-bf78-a5a077a1824a
 2026-08-11 22:14:10 +02:00
Marc Schier 275c3800b5 [Server] Add live NodeManager lifecycle support (#4094) 
# Description

Adds supported live add, reload, and remove lifecycle operations for
NodeManager instances on a running server.

The change introduces a DI-backed `INodeManagerLifecycle` service with
direct `StandardServer` access, generation-aware registration handles,
copy-on-write routing, transactional prepare/commit/rollback, runtime
NodeSet convenience APIs, append-only namespace updates, model and
semantic change notifications, additive-compatible complex type priming,
and complete teardown behavior.

Lifecycle publication is coordinated with requests, sessions,
subscriptions, monitored items, continuation points, and shutdown so
retired managers cannot remain reachable or be disposed while still in
use. Compatible monitored items transfer to replacement generations
without a transient bad status. Removed or incompatible Nodes publish
`BadNodeIdUnknown` as required by OPC UA Part 4 section 5.8.4.1, retain
their MonitoredItem identity, and automatically recover when a
compatible Node with the same NodeId returns.

Registration is documented end to end in the new
[docs/NodeManagerRegistration.md](docs/NodeManagerRegistration.md),
which covers compile-time, startup, and runtime registration together
with the guarantees for MonitoredItems, Browse continuation points,
namespace indexes, DataTypes, and change notifications.
## Review feedback addressed

Interface surface reduced, as requested:

- `INodeManagerMonitoredItemTracker` folded into
`ISubscriptionMonitoredItemLifecycle`.
- `INodeManagerMutationCoordinator` and the asynchronous MonitoredItem
recovery entry point folded into `IDynamicNodeManagerHost`.
- `ISessionClosingRegistry` folded into `IServerInternal`, then replaced
by `ISession.IsClosing`.
- `ISubscriptionDeletionRegistry` folded into `ISubscriptionManager`,
then removed entirely.
- `INodeManagerContinuationPointTracker` folded into `ISession`.
- The synchronous recovery entry point kept on its own
`ISyncNodeManagerMonitoredItemRecovery`, because `AddPredefinedNode`
cannot await; it still routes through the asynchronous
`INodeManagerMonitoredItemLifecycle` that `AsyncNodeManagerAdapter`
exposes.
- `IMonitoredItemLifecycle` renamed to `IDetachableMonitoredItem`.

Reach into the Subscription area reduced to the minimum the feature
needs:

- The lifecycle feature itself leaves `SubscriptionManager` almost
untouched: two guards that reject a request from a Session that is
closing, on `CreateSubscription` and on `TransferSubscriptions`, because
it would create state that is about to be torn down. The deletion claim
registry, the session closing gates, the closed-session bookkeeping and
the deletion restore paths introduced by earlier revisions are gone.
Transferring a subscription away from a session that is closing stays
allowed, since that is the recovery path the service exists for; the
race that exposed is fixed by serialising source-queue closure with the
transfer rather than by rejecting it.
- Queueing is disabled again for a MonitoredItem with `QueueSize <= 1`,
as before this PR. Without a queue the Client is served the last sampled
value, so the missing-Node notification simply becomes that value;
losing values is expected for a MonitoredItem without queuing.
- What remains of the notification is one `DataValue` plus one bool in
`DataChangeQueueHandler`, mirroring the existing overflow pair, which
stops it being the value discarded when a configured queue is full.
- `MonitoredItem` keeps its original publish path, its original queue
creation and restore paths, its original triggering behaviour, and no
per-epoch bookkeeping.
- `IStoredMonitoredItem` carries `IsDeleted` and `IsDetached`, so a
restored item reports the state it was stored with rather than one
inferred from the last value's StatusCode.

Other feedback:

- Documented `HostedNodeManagerLifecycle`, `IDynamicNodeManagerHost`,
`NodeManagerRoutingTable`, and the `RequestManager` validation and
request scopes.
- Renamed `ValidateMonitoredItemAsync` to `CanAttachMonitoredItemAsync`
and `RestoreMonitoredItemAsync` to `RecoverMonitoredItemAsync`, so the
lifecycle API no longer collides with the durable Subscription
`RestoreMonitoredItem` path used at server startup.
- Removed the `CustomNodeManager2` dependency from the synchronous
MonitoredItem recovery path.
- `DataChangeQueueHandler` is back to the two constructors 1.5.378
shipped, each taking an optional `TimeProvider`.
- Session closing is a one-way latch, and the Session is removed even
when teardown throws.
- Documented what a custom NodeManager has to implement to take part in
live lifecycle operations, and which NodeManager implements
`INodeManagerReloadParticipant` today.

## Fixes found during review

- The guard that stops a NodeManager lifecycle operation being started
from inside a service handler never fired. It was backed by an
`AsyncLocal` written inside `async ValidateRequestAsync`, which is
invisible to the caller that awaited it, so a lifecycle call from a
NodeManager callback waited for its own request and held the lifecycle
semaphore forever. The mark is now applied by the service dispatcher,
and the drain is bounded so no path can wedge the lifecycle permanently.
- `ValidateRequestAsync` is no longer virtual. An override that rejected
a request after calling `base` left it registered forever; subclasses
now use `OnRequestValidatedAsync`, whose failures complete the request.
- A MonitoredItem deleted and disposed while a reload was handing it to
the replacement NodeManager could be attached anyway. The item now
reserves itself for the hand-over and the transition takes it back if it
was disposed meanwhile.
- The Overflow bit is recorded and applied when the value is published,
as the surrounding design does, rather than written into the value at
enqueue time.
- MonitoredItem cleanup for a Subscription being deleted no longer
passes null entries to `DeleteMonitoredItemsAsync`.
- Browse continuation points of a retired NodeManager are persisted and
disposed outside the Session lock.

## Open review discussions

Three threads are open, replied to and awaiting reviewer decisions:
separating service-call dispatch from NodeManager handling in
`MasterNodeManager` (tracked as #4098), the request-scoping shape in
`StandardServer`, and how much of the async MonitoredItem service
implementation should stay in `Subscription`.

Issue #4102 records a specification ambiguity found while implementing
the missing-Node notification: Part 4 §5.8.4.1 makes it mandatory, but
the protected over-capacity queue entry §5.13.1.5 defines applies to
`EventQueueOverflowEventType` only, so the specification does not say
how it survives a full queue.

## Validation

- Full `Opc.Ua.Server.Tests` on net10.0: 3,768 passed, 5 skipped.
- Lifecycle, MonitoredItem, Subscription and continuation point suites:
185 passed on net10.0 and 75 on net48.
- Server builds complete with zero warnings or errors.
- Two failures are pre-existing and unrelated:
`DurableDataValueQueueVerifyReferenceBatchingAsync` (flaky
`BatchPersistor` file contention, passes in isolation on net10.0 and
net48) and
`ConfigureApplicationBuildsSharedClientAndServerConfigurationAsync`
(fails on a clean tree with these changes stashed).

## Related Issues

- Fixes #3993
- Follow-up tracked in #4098
 2026-07-28 16:06:28 +02:00
Marc Schier 6b4208a882 Restore coherent durable Subscription snapshots (OPC 10000-4) (#4045) 
## Failure

Durable Subscription restore depended on a process-local cache, while
full snapshots were written record-by-record under shared mutable keys.
A restart or concurrent writer could therefore expose stale, mixed, or
partially committed Subscription state.

## Fix

- Write each snapshot to an immutable generation and atomically publish
a protected manifest.
- Restore only the committed generation, validate record identities and
completeness, and retain legacy-key fallback.
- Replace the cache only after the full snapshot is authenticated and
decoded, including explicit empty snapshots.

## Standard

OPC 10000-4 §6.6.2.4.4 and §6.6.2.4.5.5 require durable Subscription and
retransmission state to remain coherent across redundant Server failover
and restart.

## Tests

- `dotnet test
tests\Opc.Ua.Redundancy.Server.Tests\Opc.Ua.Redundancy.Server.Tests.csproj
-f net10.0 --no-restore --nologo --verbosity minimal` — 542 passed
- `CustomTestTarget=net48 dotnet test
tests\Opc.Ua.Redundancy.Server.Tests\Opc.Ua.Redundancy.Server.Tests.csproj
--no-restore --nologo --verbosity minimal -m:1` — 542 passed

- Review follow-up: `SharedKeyValueSubscriptionStoreTests` — 39 passed
on net10.0 and 39 passed on net48.

## Split

Split from #4021.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 2026-07-21 20:46:27 +02:00
Marc Schier 7cec2d614d Move repository files to the consolidated layout 2026-07-16 06:25:54 +02:00