mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-20 00:31:32 +08:00
fix/encoder-stack-depth-json-flake
4 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 romanett
|
87c35b84b6 |
Fix Opc.Ua.Client audit findings (security fallback, reconnect, subscriptions, redundancy, browse, files, WebApi) (#4572)
## Summary
Fixes the findings of a bug audit of `Opc.Ua.Client` and
`Opc.Ua.Client.ComplexTypes`, done on master plus #4516 (now merged).
- **Audit:** 82 findings across 12 areas, each checked against the OPC
UA specification. Final severities: 2 High, 37 Medium, 43 Low.
- **Fixes:** 76 fixed, 4 partial, 2 skipped (see [Not
fixed](#not-fixed--partial)).
- **Review:** a follow-up review of the fixes found 15 more defects in
them, all fixed here.
- **Tests:** every fix has a regression test, except a few races that
cannot be reproduced deterministically; the commit messages say which.
The finding IDs (`L<area>-<n>` for the audit, `G<n>` for the review)
appear in the commit titles.
## Behaviour and user-facing changes
### Security and endpoint selection
- **Reconnect no longer drops to weaker security (L3-1, High).** When
the server rejects the configured security policy or mode during
reconnect, `SessionReconnectHandler` no longer switches to whatever
endpoint the server advertises, None included. It only picks an endpoint
at least as strong as the configured one, judged by a locally computed
security level. The server-advertised `SecurityLevel` is not trusted. If
nothing qualifies, reconnect fails with the original error (new log
event `SessionReconnectHandler + 13`) and keeps retrying with the
original settings.
- **`CoreClientUtils.SelectEndpoint(useSecurity: true)` never returns a
SecurityMode None endpoint over opc.tcp (L9-9, High).** It now returns
`null` when no secure endpoint matches.
- When the discovery URL is `https`/`opc.https`, an HTTPS endpoint with
SecurityMode None is still accepted as a last resort, because TLS
protects it (Part 6 §7.4.1) (G17).
- Endpoints with SecurityMode `Invalid` no longer count as secure.
- `GlobalDiscoveryServerClient` and `ServerPushConfigurationClient` now
throw `BadSecurityPolicyRejected` when no secure endpoint matches,
instead of passing `null` on. The ConsoleReferenceClient sample reports
this with a clear error.
- **No Bearer/Basic credentials over `http://` (L12-2).**
`WebApiClient.Create` and WebApi channel open/reconnect reject them with
`BadSecurityChecksFailed`. The public `WebApiClient(HttpClient, …)`
constructor also rejects them when the client's `BaseAddress` is not
https, and every send checks the address it actually goes to (covers a
`BaseAddress` set later).
### Session, keep-alive and identity
- **Faster detection of a silent server (L1-1).** The keep-alive Read is
now bounded by its `TimeoutHint` (2 × `KeepAliveInterval`), which is
what Part 4 §7.32 says the client should do. It then fails with
`BadNoCommunication`, so reconnect starts after about 2 ×
`KeepAliveInterval` instead of `OperationTimeout` (120 s by default).
- **Operations fail fast during a channel-manager recovery (L1-2, G9).**
`UpdateIdentityAsync`, `TransferSubscriptionsAsync` and
`ReactivateSubscriptionsAsync` fail with `BadSecureChannelClosed`
(retryable) instead of deadlocking. This covers the operation holding
the reconnect lock and any queued behind it.
- **Closing or disposing from a handler no longer waits on itself (L1-3,
G5, G6).**
- `Session` and `ManagedSession` Close/Dispose from a Notification
handler no longer wait 30 s for their own background drain.
- Disposing a `ManagedSession` from a keep-alive `PublishStatusChanged`
handler no longer deadlocks.
- Tasks spawned by a handler no longer count as "the handler" (the owner
marker is scoped to the work item).
- **Switching to an ECC user-token policy takes two ActivateSession
calls (L1-4, G4).** This follows Part 6 §6.8.2 (fetch the ephemeral key,
then activate).
- If the server returns no key, the switch fails with
`BadSecurityPolicyRejected`.
- If the second activation is rejected, the old identity stays on the
override policy with the key just fetched. Restoring the old key was
tried: against the reference server the next reconnect then failed to
decrypt the identity token.
- **`ManagedSession.ConfiguredEndpoint` follows failover (L2-6).** It
now reports the inner session's endpoint, so it changes after a failover
or network-path rotation.
- **An explicit identity update replaces the identity provider (L2-7).**
`ManagedSession.UpdateSessionAsync(identity)` stops the provider's
refresh loop and drops the provider for good. If the update fails, the
loop is restarted.
- **Reconnect waits for in-flight calls on a healthy channel (L2-3,
G8a).** Reconnect/failover on a healthy channel (certificate or
trust-list change, manual reconnect) waits for in-flight service calls.
The wait is bounded (max(`KeepAliveInterval`, 1 s)) only once the
channel is unhealthy (keep-alive stopped, channel not Ready), so a dead
channel no longer holds recovery for `OperationTimeout`.
- **Smaller fixes:**
- Restore reads the server certificate as a chain; this failed on macOS
(L1-5).
- Session recovery can no longer run twice at once (L1-6).
- Identity expiries more than 49.7 days out no longer throw (L2-2).
- Keep-alive reconnect suppression is recalculated on a lease swap
(L2-4).
### Reconnect state machine and reverse connect
- **⚠ `MaxTotalReconnectTime = 0` now means "no time limit" (L3-7).
Reviewer decision needed.** Before, 0 disabled reconnect entirely.
Negative values other than `Timeout.InfiniteTimeSpan` now throw
`ArgumentOutOfRangeException` when the state machine is constructed;
before, they crashed the worker. The XML docs of
`ReconnectPolicy`/`ReconnectPolicyOptions` are updated. If 0 should keep
meaning "no reconnect", this is a one-line revert.
- **A close racing the end of the reconnect budget is no longer lost
(L3-4).** It goes through the normal close instead of switching to
failover.
- **The "reconnect worker" marker covers one step only (L2-1).** It
lasts for a single connect/reconnect/failover/close step. Keep-alive,
publish and identity loops started in a step no longer count as the
worker afterwards.
- **Reverse-connect strategies `Undefined` and `Any` alone are removed
after the first callback (L8-1).** This matches the documentation;
before, they behaved like `Always`.
- **`AddEndpoint` for a URL that is already added disposes the replaced
listener (L8-2).**
### Service calls
- **Batched calls clean up after a failed later batch (L3-2).** When a
later batch of
`Browse`/`BrowseNext`/`HistoryRead`/`CreateMonitoredItems` fails, the
continuation points and monitored items created by earlier batches are
released or deleted before the exception is rethrown. This adds one
request. `AddNodes`/`RegisterNodes` are not undone.
- **Batched diagnostic infos point at the right strings (L3-3).** The
string-table indexes are rebased when diagnostics are padded.
- **`ReadBytesAsync` rejects oversized responses (L3-5).** It throws
`BadUnknownResponse` when the server returns more bytes than requested,
and `BadEncodingLimitsExceeded` beyond `int` range.
- **`ReadByteStringInChunksAsync` handles an unknown server limit
(L3-6).** When the server's `MaxByteStringLength` is 0 (absent) or
larger than `int`, it falls back to the client's
`MessageContext.MaxByteStringLength`. It throws only when both are
unknown.
### Browse and NodeCache
- **Empty browse pages with a continuation point are followed (L9-1,
L9-4, L9-5, G16).** Part 4 §5.9.2.1 allows a server to return an empty
page with a continuation point, so browsing keeps following it. After 10
consecutive empty rounds it gives up with `BadNoData` and releases the
continuation point.
- `BrowseAsync(NodeId)` now throws instead of silently returning a
truncated list.
- `BrowseStreamAsync` yields a `BadNoData` result for that node.
- The same cap, through one shared helper, now also protects
`RoleManagementClient.ListRolesAsync` and the FileSystem
enumerate/empty-check loops.
- **Continuation points are released on failure (L9-3).**
`BrowseAsync(NodeId)` releases them on any exception, and
`BrowseStreamAsync` releases all points of the current page when the
consumer stops early.
- **NodeCache lookups propagate cancellation (L9-6, L9-8).**
`FindAsync`, `IsKnownAsync`, `FetchNodeAsync`,
`FindReferenceTypeNameAsync`, the reference-table fill and the
type-hierarchy search now let `OperationCanceledException` through.
Before, a cancelled lookup looked like "node does not exist".
- **A NodeClass mismatch fails only that node (L9-7).** It returns
`BadNodeClassInvalid` for that node instead of throwing
`KeyNotFoundException` for the whole batch.
- **Smaller fixes:**
- `MaxNodesPerBrowse` ≥ 2^31 no longer breaks the managed browse (L9-2).
- Null discovery URLs are skipped (L9-10).
- `ModelChangeTracker` no longer cancels a joined start (L9-11).
- **Log changes:** `BrowserContinuationPointExistsButBrowse` (Browser +
0) was removed, and `BrowserServerReturnedEmptyReferencesBut` now
includes the round count.
### Subscriptions (default engine)
- **Recovery after a transfer delivers each message exactly once (L5-1,
G22).** Messages already dispatched are acknowledged instead of
republished and delivered again. Messages whose earlier republish failed
are now republished and delivered, with the Republish flag and after
newer messages, instead of being acknowledged and lost.
- **A keep-alive that proves a gap triggers republish at once (L5-5,
G23).** The dedup mark moves only past messages that were actually
recovered, so a failed republish is retried by the next data message.
- **Item changes that fail are retried with backoff (L5-3).** They are
retried on the next apply pass after the state manager's 250 ms–5 s
backoff, instead of six times back to back. The six attempts now span
about 13 s.
- **Extreme revised intervals no longer throw (L5-4, L6-1).** Very large
or NaN intervals and counts revised by the server are capped (NaN falls
back to the requested value). Before, the overflow could stop every
publish worker.
- **`SetTriggeringAsync` from a callback no longer hangs (L5-7, G21).**
While the subscription is deleted, recreated or disposed, it returns
`BadOperationAbandoned`. A cancelled reset no longer makes later calls
fail.
- **Publish responses for unknown subscription ids (L6-3, L6-4, G20).**
The client no longer re-sends acknowledgements the server already
processed. An unknown id seen while a creation is pending is left alone
instead of being deleted.
- **Smaller fixes:**
- A null and an empty `IndexRange` are treated as equal, and restored
snapshots use `null` (L5-6).
- A failed partition attach during restore is rolled back (L6-2).
- `StreamingSubscription` throws `ObjectDisposedException` after dispose
(L6-6).
### Subscriptions (classic `Subscription`)
- **Republish no longer abandons other missing messages (L7-1).** A
Republish response no longer overwrites `AvailableSequenceNumbers`, so
other missing messages are not given up while the server still holds
them.
- **Late copies are no longer delivered twice (L7-2).** Copies of
already-processed sequence numbers are ignored, and creating a new
server subscription resets the sequence state.
- **Disposing from a keep-alive handler no longer deadlocks (L7-3).**
`Dispose()` from a keep-alive `PublishStatusChanged` handler can return
while that callback is still finishing.
- **Transfer of a restored subscription (L7-4, G27).**
- It succeeds even when some saved items were never created.
- The server subscription is deleted only when this session's own
TransferSubscriptions just moved it and adoption failed definitively
(item count mismatch or a non-transient status). Transient errors and
`ReactivateSubscriptionsAsync` never delete it.
- The public `Subscription.TransferAsync` never deletes.
- **Smaller fixes:**
- Cloned items keep their triggering links (L7-5).
- `CreateAsync` clears stale server ids (L7-6).
- A `Good_SubscriptionTransferred` for the client's own transfer is
ignored (L7-7).
- An item removed while its create is in flight is deleted afterwards
(L7-8).
- The publish timeout for very long keep-alives no longer overflows
(L7-9).
- A negative `MaxMessageCount` is handled (L7-10).
- `LastMessage` tolerates a wrong notification type (L7-11).
- Pending acks exactly 2^31 away no longer throw (L4-1).
- **Log changes:** new log events `Subscription + 68..70`.
### Redundancy
- **Warm/Hot demotion (L4-2, G1, G3).**
- Without token reuse, the demoted leader closes its session and
reconnects as a fresh standby.
- With token reuse, it abandons the shared server session locally,
without CloseSession, so the new leader can reactivate it and keep its
subscriptions. This uses the new internal methods
`ManagedSession.AbandonServerSessionAsync` and
`Session.AbandonServerSession`.
- `RoleChanged(false)` is now raised before the standby connect and no
longer depends on it, so `CurrentSession` can briefly be `null`.
- Exceptions from `RoleChanged` handlers are logged and do not stop the
teardown.
- **Promotion (L4-3, G2).** A promoted standby that adopts the leader's
session first closes its own server session. A retried promotion keeps
the session it already adopted.
- **Client failover takeover (L4-4, partial).** Subscriptions the backup
prepared in advance (not yet created, `TransferId` set to the active
client's id) are moved through the session's own transfer and keep
publishing. `docs/HighAvailability.md` is updated.
- **`ShouldFailover` through the interface (L4-6).** Calling it through
`IServerRedundancyHandler` no longer fails over away from a server that
is still Healthy.
### Fluent / dependency injection
- **Shared DI session (L10-5).** The shared session from `AddClient` is
reconnected after a caller disposes it, and is disposed with the
container. After that the connect delegate throws
`ObjectDisposedException`.
- **New public API:
`ManagedSessionBuilder.WithServerRedundancyOptions(ServerRedundancyOptions)`
(L10-6).** This is additive. The `Session.ServerRedundancy` options are
now actually applied.
- **`AddDiscoveryAndConnect` rewrites the advertised address (L10-7).**
It replaces the host and port the server advertises with those of the
discovery URL (Part 4 §5.6.2.1, NAT/containers), and prefers endpoints
with the same scheme.
### State machines and role management
- **`GetSubStateMachineAsync` returns the instance's own sub-state
machine (L10-1).** Before, it returned the type's placeholder.
- **`WaitForStateAsync` reports failures with OPC UA status codes
(L10-2).** It throws `BadNotFound` when `CurrentState/Id` cannot be
resolved, and `BadInvalidState` when observation ends without reaching
the state. Before, both surfaced as `OperationCanceledException`.
- **`RoleManagementClient.ListRolesAsync` pages correctly (L10-3).** It
follows continuation points, releases them on any failure, and throws on
a Bad browse status instead of returning an empty list.
- **Role properties are read into the right fields (L10-4).** Properties
whose namespace cannot be resolved are skipped instead of shifting the
other values.
### File system, temporary files and alias names
- **Path escaping (L11-8, G12).**
- Path segments escape `:` and `/` with `&`, and `&` itself as `&&`
before those characters or at the end of a name.
- `Format` and `Parse` round-trip all names. Typed input ending in `&/`
now reads the slash as part of the name.
- A segment without a namespace prefix also matches a namespace-0 child
when nothing matches in the parent's namespace.
- **Stale path cache (L11-1).** When a cached lookup fails,
`GetInfoAsync` retries once from the server. `ExistsAsync` and
`GetFileAsync` report "not found" instead of throwing.
- **`GenerateFileForReadAsync` waits for the file (L11-2).** It waits
until the file-generation state machine reaches ReadTransfer (`Idle`
counts as not started yet), and throws `IOException` if it reports
Error. The wait is bounded only by the caller's token.
- **Non-recursive delete refuses hidden children (L11-3).** It refuses
when the directory holds children that the subtype options hide from
enumeration. Before, the server deleted them all.
- **Alias names (L11-5, L11-6, L11-7).**
- `BadMethodInvalid` maps to `NotSupportedException` for all alias
calls, so `UseVerbose` falls back to `FindAlias`.
- A duplicate alias name resolves to all its targets.
- If the refresh strategy fails to start, the resolver logs a warning
and keeps working without automatic invalidation. New log event block
`AliasNameResolver = 560`.
- **Null profile entries (L11-4).** Null `ServerProfileArray` entries no
longer crash `GetConformanceInfoAsync`.
### WebApi transport
- **Failures come back as `ServiceResultException` (L12-1).**
- A timeout becomes `BadRequestTimeout`.
- HTTP 429/503 becomes `BadServerTooBusy` with the Retry-After hint, on
every target framework. `WebApiClient` checks the status itself (reusing
`HttpsTransportChannel.CreateServerTooBusyException`), so callers of
`WebApiClient` also get a `ServiceResultException` instead of an
`HttpRequestException` for these two statuses (G15).
- An IO error becomes `BadNotConnected`.
- **Timeouts (L12-1, G14).**
- `OperationTimeout` applies to each request. When `RequestTimeout` is
unset, HttpClient's 100 s default no longer cuts off long Publish
requests.
- The GetEndpoints call at open is bounded by `OperationTimeout`, and
cancelling the open now throws.
- **Core change:** `HttpsTransportChannel.MapRequestFailure` went from
private to internal so the WebApi channel can reuse it.
### ComplexTypes
- **Out-of-range union selector (L12-3).** A union whose SwitchField is
larger than its field count now fails with `BadDecodingError` when
decoded and `BadEncodingError` when encoded (Part 6 §5.2.8).
- **Verbose JSON unions (L12-4).** They now decode their selected field
instead of `null`.
## Not fixed / partial
- **L2-5 (skipped):** an identity provider combined with a
caller-supplied reverse connection still can't connect to an
undiscovered endpoint. Fixing it means choosing the identity after
CreateSession, a design change.
- **L4-5 (skipped):** the one-time KeyCredential token needs a fresh
token on each reactivation. Today reactivation fails and the client
falls back to a new session.
- **L4-4 (partial):** only subscriptions the backup prepared in advance
survive a takeover.
- **L3-2 (partial):** `AddNodes`/`RegisterNodes` batches are not rolled
back.
- **L2-3 (partial):** service calls still hold the session lock for
their whole round trip.
- **Known from the review, not fixed here:**
- G7: the batch cleanup request can't be cancelled.
- G11: the DI session's close at host shutdown never reaches the server,
and a closed session keeps being handed out.
- G13: a file renamed by another client is still found under its old
path.
- G25: a clone-based transfer doesn't mark the original subscriptions.
- G27b: the transfer item check matches by count, not by ID.
- **Outside this PR:** the reference server ignores `ECDHPolicyUri` on
ActivateSession (`SessionSecurityPolicyHelper`), which breaks Part 6
§6.8.2. As a result, the L1-4 two-step ECC switch ends in
`BadSecurityPolicyRejected` against our own server.
## Testing
All suites on net10.0, on the branch before the master merge. Every
suite had 0 failures:
| Suite | Passed | Skipped |
|---|---|---|
| Opc.Ua.Client.Tests | 2698 | 0 |
| Opc.Ua.Redundancy.Client.Tests | 134 | 0 |
| Opc.Ua.Subscriptions.Classic.Tests | 57 | 0 |
| Opc.Ua.Bindings.Https.WebApi.Tests | 385 | 0 |
| Opc.Ua.Sessions.Tests | 782 | 379 |
| Opc.Ua.Subscriptions.Tests | 640 | 6 |
| Opc.Ua.Client.ComplexTypes.Tests | 2858 | 0 |
After merging current master (net10.0), every suite had 0 failures
except one, a flaky new test fixed in
|
||
|
Marc Schier
|
e73e71184d |
[WotCon] Add OPC UA WoT Connectivity 1.1 with the Types, source generation, Server and xRegistry work it builds on (#4128)
## Summary Adds OPC UA WoT Connectivity 1.1 to the stack, together with the Types, source generation, Server and xRegistry work it is built on. This branch is the merge point for the fourteen-PR stack that replaced integration PR #4093; every constituent PR was reviewed and approved separately and is listed below. 414 files changed, +108,936 / -3,199. Roughly 46k added lines of product code, 57k of tests, plus samples, tools and documentation. ## Types - Complete `ExtensionObject` raw-body decoding for binary, XML and JSON bodies by resolving the concrete type through the message context's encodeable factory, and simplify the `Variant` structure helpers by delegating to it (#4128). - Add `IAtomicFileReplace` as an optional `IFileSystem` capability so existing external implementations keep working, with atomic publish for `LocalFileSystem` via `File.Replace`/`File.Move` and for `VirtualFileSystem` by re-keying the in-memory entry (#4128). - Add lossless conversion between WoT documents and NodeSet2 (`Opc.Ua.Wot.WotNodeSetConverter`). A byte-exact `uav:nodeSet` envelope is preserved when requested, the structured `uav:nodes` projection is used when the readable vocabulary is incomplete, and NodeSet2 is otherwise synthesized from readable WoT terms. Unmapped JSON members survive a round trip as pointer-addressed residue (#4131). ## Source generation - Improve the NodeSet to ModelDesign conversion and the generated node state (#4132). - Generate OPC UA models directly from WoT Thing Description files (#4134). ## Server - Add live NodeManager shadow and immediate reload. A reload materializes a new generation beside the active one and switches atomically; the superseded generation is retired gracefully so existing MonitoredItems keep being served until they drain, or immediately when the caller asks for it. Includes the request admission and drain machinery that lets an orderly shutdown wait for admitted requests instead of tearing down underneath them (#4147). - Make subscription transfer between sessions transactional, so a failed transfer leaves neither session holding a partially moved subscription (#4150). ## xRegistry - Allow clients to use explicit registry roots rather than assuming a well-known location (#4130). ## WoT Connectivity - Add the WoT Connectivity 1.1 information model: a registry-first revision layered on the abstract xRegistry base model, incorporating the complete published OPC 10100-1 v1.02 surface as deprecated nodes in the same namespace so existing 1.02 clients keep working (#4139). - Add the protocol binding abstractions and planners that compile WoT forms into executable plans (#4140). - Add the HTTP, Modbus, OPC UA and MQTT binding executors (#4144). - Add the registry and its stores, including document versioning, validation, dependency resolution and refresh (#4146). - Add the registry client (#4142). - Materialize WoT Thing Descriptions into the server address space, deriving types from Thing Models and instances from Thing Descriptions (#4154). - Restore the WoT asset registry bridge so the deprecated 1.02 asset surface is backed by the registry (#4171). - Add the WoT Connectivity samples and complete the documentation (#4156), and restore the remaining coverage and docs (#4172). ## New projects `src/Opc.Ua.WotCon.Bindings`, `src/Opc.Ua.WotCon.Bindings.Mqtt`, `tests/Opc.Ua.WotCon.Bindings.Tests`, `tests/Opc.Ua.WotCon.Samples.Tests`, and the `samples/WotCon` AggregationServer, AggregationClient and FlatTagServer samples. ## Constituent pull requests Merged into this branch top-down, each independently approved: | PR | Title | | --- | --- | | #4128 | [Types] Complete ExtensionObject decoding and add an atomic file replace capability | | #4131 | [Types] Add lossless conversion between WoT documents and NodeSet2 | | #4132 | [SourceGeneration] Improve NodeSet to ModelDesign conversion and generated node state | | #4134 | [SourceGeneration] Generate OPC UA models from WoT Thing Description files | | #4147 | [Server] Add live NodeManager shadow and immediate reload | | #4150 | [Server] Make subscription transfer between sessions transactional | | #4130 | [XRegistry] Allow clients to use explicit registry roots | | #4139 | [WotCon] Add the WoT Connectivity 1.1 information model | | #4140 | [WotCon] Add the WoT protocol binding abstractions and planners | | #4144 | [WotCon] Add the HTTP, Modbus, OPC UA and MQTT binding executors | | #4146 | [WotCon] Add the WoT Connectivity registry and its stores | | #4142 | [WotCon] Add the WoT Connectivity registry client | | #4154 | [WotCon] Materialize WoT Thing Descriptions into the server address space | | #4156 | [WotCon] Add the WoT Connectivity samples and complete the documentation | | #4171 | Restore WoT asset registry bridge | | #4172 | Restore WoT coverage and docs | Supersedes #4093. ## Follow-up The WoT Connectivity and WoT Binding drafts have moved to 1.1-draft2 since this work was authored. Aligning with that revision - the removed group vocabulary, the new projection/View construct, the remaining model vocabulary terms and the two new portable-identity validation rules - is tracked separately and will follow in its own pull request. ## Validation Built for every target framework with zero warnings and zero errors, and validated at the tip of the stack with `Opc.Ua.WotCon.Tests` at 999 passed / 0 failed and `Opc.Ua.Server.Tests` at 4024 passed / 0 failed. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9e6a5abf-3299-4cd1-9855-010fedbf0ad8 |
||
|
Marc Schier
|
636912bd4c |
Harden subscription retirement after #4114 and refresh the client namespace table on model change (#4115)
# Description Two things, both surfaced by the live NodeManager registration work in #4094. > **Rebased on #4114.** Upstream landed #4114 (`cc4667d0b`) while this PR was open; it fixes the > same #4113 root cause. `origin/master` is merged in and the conflicts were resolved **in favour of > the upstream API shape** — `CompleteAsync(IMessageProcessor, uint subscriptionId, ct)` with the > last known server id passed as an argument. The `IMessageProcessor.LastServerId` member this > branch had added is dropped again. What remains here is the orthogonal hardening #4114 does not > cover, plus all of the #4100 work. ## 1. Remaining subscription-retirement hardening (on top of #4114) #4114 fixed the id-collision itself. Three related holes in the same paths are still open: - **Retirement was not atomic with removal.** `RetireSubscriptionId` is now called *under* `m_subscriptionLock` from both `CompleteAsync` and `RemoveSecondaryPartitionAsync`. `GetById` resolves incoming publish responses under the same lock, so a worker could otherwise observe the subscription as removed while its id had not yet reached the retired-id ring — and send a redundant `DeleteSubscriptions` for an already-retired id. `RemoveSecondaryPartitionAsync` did not record the retired id at all. - **The retired-id ring was far too small.** `kMaxSubscriptionHistory` 10 → 256, so subscription churn cannot evict live history. - **The orphan delete was unguarded.** The publish worker no longer sends `DeleteSubscriptions` for an unresolved id while any subscription is still awaiting its id. A publish response can overtake the `CreateSubscription` continuation that assigns the id, and deleting in that window kills a healthy subscription. Leaving a genuine orphan alive until its lifetime expires is the safe trade-off. - **`PublishControllerAsync` lost resize signals.** It re-created its `AsyncAutoResetEvent` waiter on every loop iteration; the abandoned waiter stayed enqueued and swallowed a later `Set()`, so the publish worker pool stopped resizing. This is a second, latent starvation path independent of the id collision. The wait is now created once and only re-armed after it completes. ## 2. Managed client does not refresh the namespace table on model change (#4100) `NodeManagerLifecycle.NotifyCommittedChangeAsync` updates `NamespaceArray`, bumps `UrisVersion`, and then reports a **`BaseModelChangeEvent`** — an event shape that carries no `Changes` payload. The client tracker *did* observe it and cleared the `INodeCache`, but it held only an `IStreamingSubscription`, an `INodeCache` and an `ILogger`, so nothing could re-read the namespace table. A tracking client therefore resolved NodeIds from a namespace added by a live NodeManager registration against a stale `NamespaceUris` table. - **New `INamespaceTableRefresher`** (`NamespaceUris` + `FetchNamespaceTablesAsync`). The member signatures match what `Session` and `ManagedSession` already expose, so both satisfy the interface without any new members. `ISession` is deliberately left untouched — moving its members onto a base interface would be a binary break for 1.5.378 consumers. - **`ModelChangeTracker`** takes the refresher as an optional *trailing* constructor argument, which keeps every existing call site source-compatible and makes `null` the opt-out. A refresh runs when the event carries no per-node detail, when an affected node is `Server_NamespaceArray`, or when an affected namespace index is beyond the client's table. It runs **before** the cache is invalidated and before `ModelChanged` is raised, so subscribers that re-browse from the handler already see the new uris. A failing refresh is logged and swallowed. - **`ModelChangedEventArgs.NamespaceTableRefreshed`** reports the refresh to applications. - **`ManagedSession.EnableModelChangeTrackingAsync`** wires the session in, so opting into tracking is all an application has to do — no new option, no change to the positional `CreateAsync` signature. - **`ModelChange.TypeDefinition`** changes from `NodeId?` to `NodeId`. `NodeId` is a readonly struct implementing `INullable` and must not be wrapped in `System.Nullable<T>`; use `NodeId.Null` and `IsNull` instead. The type is 2.0-preview only (absent from 1.5.378), and the only producer is the tracker itself, which already assigns a non-null `AffectedType`. Per review feedback the type is also now a `readonly record struct`, matching `DataValueChange` / `EventNotification`. ## Tests Every new regression test was run against a temporarily re-introduced defect and **fails** there: | Test | Covers | |---|---| | `CompleteAsyncRetiresOnlyTheCompletedSubscriptionWhenOthersArePendingCreationAsync` | retire-by-identity (also guards #4114's fix) | | `CompleteAsyncRemembersRetiredServerIdAfterIdWasResetAsync` | retired-id history | | `PublishWorkerDefersDeletingUnknownSubscriptionWhileCreationIsPendingAsync` | orphan-delete guard | | `PublishControllerKeepsResizingAfterWorkersExitAsync` | publish-controller signal loss | | `ConcurrentAddAndCompleteRetireOnlyCompletedSubscriptionsAsync` | parallel add/retire registry integrity | | `ConcurrentSubscriptionChurnNeverStarvesASubscription` (integration) | #4113 repro against the reference server | | 7 × `ModelChangeTrackerTests` namespace-refresh cases | each trigger, no-trigger, throwing refresher, `null` refresher, ordering | | `LiveNodeManagerAddRefreshesTheClientNamespaceTable` (integration) | #4100 repro — times out without the wiring | `FakeMessageAckQueue` records both the completed id (upstream's assertions) and the completing instance (the identity assertion in `MessageProcessorTests`), so neither side's coverage was lost in the merge. Results after merging `origin/master`: - `Opc.Ua.Client.Tests` — **2119/2119** pass on net10.0; the new `ModelChange` and `SubscriptionManager` fixtures also pass on **net48**. - `Opc.Ua.Subscriptions.Tests` churn + coverage fixtures — **28/28** (net10.0); churn fixture also passes on net48. - `Opc.Ua.Server.Tests` `NodeManagerLifecycle*` — **49/49** on net10.0; the new fixture also passes on net48. - No new analyzer warnings; only the three pre-existing `CA1873` hits in `NodeCacheResolver.cs` and `ClassicSubscriptionEngine.cs` (untouched files) remain. ## Documentation - `docs/ModelChangeTracking.md` — new **Namespace table refresh** section with the trigger table, updated `ModelChangedEventArgs` / `ModelChange` snippets, updated manual-construction snippet, and a quick-reference row. - `docs/NodeManagerRegistration.md` — client-side half of the **Namespaces** section. No `docs/migrationguide.md` entry: every changed type is either `internal` or 2.0-preview-only. ## Related Issues - Fixes #4100 - Follow-up hardening for #4113 (closed by #4114) - Related to #4094, #3993 ## Checklist - [x] 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. - [x] 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. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e22f9b3c-1682-40bd-8b72-31a11352fa2c |
||
|
Marc Schier
|
7cec2d614d | Move repository files to the consolidated layout |