Files
UA-.NETStandard/tests/Opc.Ua.Subscriptions.Tests/ConcurrentSubscriptionChurnTests.cs
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
2026-07-30 12:17:00 +02:00

184 lines
7.6 KiB
C#

/* ========================================================================
* Copyright (c) 2005-2025 The OPC Foundation, Inc. All rights reserved.
*
* OPC Foundation MIT License 1.00
*
* Permission is hereby granted, free of charge, to any person
* obtaining a copy of this software and associated documentation
* files (the "Software"), to deal in the Software without
* restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the
* Software is furnished to do so, subject to the following
* conditions:
*
* The above copyright notice and this permission notice shall be
* included in all copies or substantial portions of the Software.
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
* OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
* NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
* HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
* WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
* FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
* OTHER DEALINGS IN THE SOFTWARE.
*
* The complete license agreement can be found here:
* http://opcfoundation.org/License/MIT/1.00/
* ======================================================================*/
// CA2016: integration tests intentionally call cleanup in finally without forwarding the test
// cancellation token. The test CT may already be cancelled (the [CancelAfter] timeout), which
// would prevent cleanup from running. CloseAsync/DisposeAsync must complete regardless.
#pragma warning disable CA2016
using System;
using System.Collections.Concurrent;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using NUnit.Framework;
using Opc.Ua.Client;
using Opc.Ua.Client.Subscriptions;
using Opc.Ua.Client.TestFramework;
using ManagedSessionType = Opc.Ua.Client.ManagedSession;
namespace Opc.Ua.Subscriptions.Tests
{
/// <summary>
/// Regression coverage for
/// <see href="https://github.com/OPCFoundation/UA-.NETStandard/issues/4113"/>:
/// adding and disposing subscriptions concurrently on a single
/// <see cref="ManagedSessionType"/> silently starved a fraction of them.
/// A subscription resets its server side id before its message processor
/// completes, so retiring it by id evicted an arbitrary other subscription
/// that had been added but not created yet. The evicted subscription was
/// then missing from the publish dispatch registry and never received a
/// notification.
/// </summary>
[TestFixture]
[Category("Client")]
[Category("ManagedSession")]
[Category("SubscriptionManager")]
[SetCulture("en-us")]
[SetUICulture("en-us")]
public class ConcurrentSubscriptionChurnTests : ClientTestFramework
{
[OneTimeSetUp]
public override Task OneTimeSetUpAsync()
{
SupportsExternalServerUrl = true;
SingleSession = false;
return OneTimeSetUpCoreAsync(securityNone: true);
}
[OneTimeTearDown]
public override Task OneTimeTearDownAsync()
{
return base.OneTimeTearDownAsync();
}
[SetUp]
public override Task SetUpAsync()
{
return base.SetUpAsync();
}
[TearDown]
public override Task TearDownAsync()
{
return base.TearDownAsync();
}
[Test]
[CancelAfter(180_000)]
public async Task ConcurrentSubscriptionChurnNeverStarvesASubscription(
CancellationToken ct)
{
const int kConcurrency = 6;
const int kIterationsPerWorker = 8;
ConfiguredEndpoint endpoint = await ClientFixture
.GetEndpointAsync(ServerUrl, SecurityPolicies.None)
.ConfigureAwait(false);
ManagedSessionType session = await new ManagedSessionBuilder(
ClientFixture.Config, Telemetry)
.UseEndpoint(endpoint)
.WithSessionName(nameof(ConcurrentSubscriptionChurnNeverStarvesASubscription))
.WithSessionTimeout(TimeSpan.FromSeconds(120))
.ConnectAsync(ct)
.ConfigureAwait(false);
var starved = new ConcurrentBag<int>();
try
{
Assert.That(
session.TryGetSubscriptionManager(out ISubscriptionManager? manager),
Is.True);
Assert.That(manager, Is.Not.Null);
await Task.WhenAll(Enumerable.Range(0, kConcurrency)
.Select(worker => Task.Run(async () =>
{
for (int i = 0; i < kIterationsPerWorker; i++)
{
var handler = new RecordingSubscriptionHandler();
ISubscription subscription = session.AddSubscription(
handler,
new Client.Subscriptions.SubscriptionOptions
{
PublishingInterval = TimeSpan.FromMilliseconds(200),
PublishingEnabled = true,
KeepAliveCount = 10,
LifetimeCount = 1000
});
try
{
Assert.That(
subscription.TryAddMonitoredItem(
"CurrentTime",
VariableIds.Server_ServerStatus_CurrentTime,
o => o with
{
SamplingInterval =
TimeSpan.FromMilliseconds(100),
QueueSize = 1
},
out _),
Is.True);
bool gotData = await handler
.WaitForFirstDataAsync(
TimeSpan.FromSeconds(20), ct)
.ConfigureAwait(false);
if (!gotData)
{
starved.Add((worker * kIterationsPerWorker) + i);
}
}
finally
{
await subscription.DisposeAsync().ConfigureAwait(false);
}
}
}))).ConfigureAwait(false);
Assert.That(starved, Is.Empty,
$"{starved.Count} of {kConcurrency * kIterationsPerWorker} " +
"subscriptions never received a data change notification.");
// Every subscription was disposed, so the manager registry
// must be empty again — an evicted subscription would leave
// the counts inconsistent.
Assert.That(manager!.Count, Is.Zero);
}
finally
{
await session.CloseAsync().ConfigureAwait(false);
await session.DisposeAsync().ConfigureAwait(false);
}
}
}
}