Complete client subscriptions by identity instead of id - #4114
Merged
Conversation
|
|
7 tasks
marcschier
approved these changes
Jul 29, 2026
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4114 +/- ##
===========================================
+ Coverage 64.02% 79.89% +15.87%
===========================================
Files 1478 1514 +36
Lines 206272 209413 +3141
Branches 35532 36037 +505
===========================================
+ Hits 132057 167310 +35253
+ Misses 61686 29559 -32127
- Partials 12529 12544 +15
🚀 New features to boost your workflow:
|
7 tasks
Copilot AI
added a commit
that referenced
this pull request
Jul 30, 2026
…d failure Update IMessageAckQueue.CompleteAsync signature to include IMessageProcessor as first parameter, matching the PR #4114 intent. Update all callers: - MessageProcessor.cs: store last server id, pass self to CompleteAsync - SubscriptionManager.cs: use completing IMessageProcessor for O(1) lookup, skip zero-id history entries - FakeMessageAckQueue.cs: update fake impl to match new interface - SubscriptionManagerTests.cs: update existing tests + add new test for zero-id - SubscriptionManagerCoverageTests.cs: fix the two calls causing CI failure
Collaborator
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
marcschier
added a commit
to marcschier/UA-.NETStandard
that referenced
this pull request
Jul 30, 2026
…ce-refresh Upstream landed OPCFoundation#4114 ("Complete client subscriptions by identity instead of id", cc4667d), which fixes the same OPCFoundation#4113 root cause this branch did. Conflicts resolved in favour of the merged upstream API shape, keeping this branch's orthogonal fixes on top. Taken from upstream: - IMessageAckQueue.CompleteAsync(IMessageProcessor, uint subscriptionId, CancellationToken) - the last known server id is passed as an argument rather than exposed as IMessageProcessor.LastServerId. The interface member added by this branch is dropped again, and MessageProcessor keeps upstream's private m_id / m_lastServerId backing fields. - SubscriptionManager.CompleteAsync resolves the subscription from the passed instance and logs the passed id. - FakeMessageAckQueue records the completed subscription id and SubscriptionManagerCoverageTests / SubscriptionManagerTests use the three-argument call shape. Kept from this branch (orthogonal to OPCFoundation#4114): - RetireSubscriptionId helper, called under m_subscriptionLock from both CompleteAsync and RemoveSecondaryPartitionAsync so a publish worker can never observe a subscription as removed before its id is retired. The retired-id ring is also raised from 10 to 256 entries. - The publish worker no longer deletes an unresolved subscription id while any subscription is still awaiting its id (HasSubscriptionsPendingCreation). - PublishControllerAsync reuses its AsyncAutoResetEvent waiter instead of re-creating it every iteration, so a Set() can no longer be delivered to an abandoned waiter and lost. - The additional unit tests and the ConcurrentSubscriptionChurnTests integration fixture. - All of the OPCFoundation#4100 namespace-refresh work, which upstream does not touch. FakeMessageAckQueue now records both the completed id (upstream's assertions) and the completing instance (this branch's identity assertion in MessageProcessorTests), so neither side's coverage is lost. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e22f9b3c-1682-40bd-8b72-31a11352fa2c
marcschier
added a commit
that referenced
this pull request
Jul 30, 2026
…espace 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
marcschier
added a commit
that referenced
this pull request
Jul 30, 2026
Brings in #4114 (complete client subscriptions by identity instead of id) and #4115 (harden subscription retirement and refresh the client namespace table on model change). One conflict, of the modify/delete kind: #4115 added a paragraph to docs/NodeManagerRegistration.md, which this branch deleted when the four node-manager documents were consolidated into docs/NodeManagers.md. Resolved by keeping the deletion and carrying the upstream content over: - The new paragraph about a Client with model change tracking re-reading its namespace table is added to the Namespaces subsection of docs/NodeManagers.md, which is where that content now lives. - docs/ModelChangeTracking.md, also touched by #4115, linked to NodeManagerRegistration.md#namespaces. Repointed at NodeManagers.md#namespaces so the new cross-reference resolves. No code conflicts: the upstream changes are confined to the client subscription and model-change stack, which this branch does not touch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0d50c3fc-58b1-4c99-85a4-1e32c88666d9
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Completes subscriptions by passing
IMessageProcessorand last known server subscription id to prevent incorrect subscription removal when multiple subscriptions with id 0 exist.Issue
During calls to
SubscriptionManager.CompleteAsync, the subscription manager looked up the subscription entry to remove by the passed subscription id. But newly created subscriptions not yet having received an id from the server have their id set to 0, meaning that disposing an unrelated subscription also with id set to 0 could remove the wrong one (as there can exist multiple subscriptions with the same zero-id), which in turn could lead to the id no longer being resolved, and the publish worker treating it as unknown. Deleted subscriptions also have their id reset to zero, which could potentially clash as well.Fix
IMessageAckQueueis changed so thatCompleteAsyncalso takesIMessageProcessoras an argument.MessageProcessorstores the known server id (if set) and passes itself along with that id toCompleteAsync.IMessageProcessorargument,SubscriptionManagerno longer needs to look up subscription by id and therefore removes the correct subscription.SubscriptionManagerreceives the non-zeroed server id and no longer enqueues zero-value ids to the subscription history.Related Issues
ManagedSession#4113Checklist