Skip to content

Commit c8bc1bb

Browse files
authored
Fix nightly Stress Tests (Chaos TCP): transparent reconnect + robust chaos assertions (#3986)
# Description Fixes the nightly **Stress Tests / Chaos TCP** job (e.g. run [29068846476](https://github.com/OPCFoundation/UA-.NETStandard/actions/runs/29068846476)), which had failed on **every run since it was introduced** — the workflow runs on a schedule and never gated PRs, so the failures went unnoticed. ## Root cause The Chaos TCP tests point a `ConfiguredEndpoint` at a local `TcpChaosProxy` and set `UpdateBeforeConnect = false` so the session connects *through* the proxy. Two problems prevented this from working: 1. `ManagedSession.HandleConnectAsync` **ignored** `ConfiguredEndpoint.UpdateBeforeConnect` and hard-coded a pre-connect re-discovery for all non-OpenAPI endpoints. Re-discovery adopted the server's advertised `EndpointUrl`, so the session connected **directly to the server, bypassing the proxy** — no chaos ever reached the connection. Two tests "passed" vacuously. 2. Once the proxy was genuinely engaged, the transparent-reconnect feature had a gap: a request that hit the just-dropped channel (before the manager detected the drop) failed with `BadConnectionClosed` instead of transparently recovering. ## Changes **Product** - **`ManagedSession`** now honours `ConfiguredEndpoint.UpdateBeforeConnect` (default `true`). Setting it `false` opens the channel against exactly the supplied URL (proxy / gateway / NAT / pinned endpoint) without re-discovering. - **`ManagedTransportChannelLease.SendRequestAsync`** transparently resends **idempotent** requests (`Read` / `Browse` / `TranslateBrowsePaths` / `GetEndpoints` / `FindServers`) across a channel-manager reconnect. On a transient transport-drop error it forces a *coalesced* reconnect and resends once the shared channel recovers. - A new **`ChannelEntry.ReconnectGeneration`** counter guards this so a reconnect is only forced for a genuinely undetected drop (same generation, still `Ready`), never for a stale in-flight failure that arrives after recovery — which previously spawned spurious extra reconnect cycles that tore down healthy channels. - **Non-idempotent** requests (`Create*`, `Write`, `Call`, `Publish`, …) surface the error so higher-level recovery (e.g. the subscription engine's own re-create loop) stays in control; requests are never double-applied. **Tests / harness** - **`TcpChaosProxy`** no longer rethrows the expected `OperationAborted` raised by an in-flight upstream `ConnectAsync` during `DropAllConnections` (it was faulting the awaited connection task and failing the test). - Chaos reconnect assertions (L3-A1/A2/A5) are made robust to the inherent non-determinism of reconnect **counts** under concurrent chaos (coalescing merges drops landing in one recovery window; a stale keep-alive `Bad` can add one cycle). Survival is asserted via `FailureRate`, `ReconnectFailed == 0`, and the coalescing fan-out relationship rather than exact `ReconnectStarted/Completed == dropCount`. - Subscription post-drop recovery window widened (`2s → 8s`) to allow re-creating all sessions' subscriptions and monitored items on a loaded CI agent. - **New unit tests** in `ClientChannelManagerManagedTests` cover the new product paths (fast-PR / codecov, since the nightly ChaosTCP job does not run in the fast pipeline): idempotent transient-drop retry, non-idempotent no-retry, non-transient no-retry, and `ChannelEntry.ReconnectGeneration` increment. ## Validation (local, net10.0) - **Chaos TCP**: 6/6 pass across many random seeds (incl. post-merge re-runs of `338475986` and `1999999999`). - `Opc.Ua.Client.Tests` ChannelManager/ManagedSession: **256/256** (incl. the 4 new tests). - `Opc.Ua.Sessions.Tests` ChannelManager/Reconnect: **139/139**. - Core / Client / Stress build with **0 warnings**. ## Notes - This branch has been merged up to date with `master` (redundancy PR #3918). - The two failing `Opc.Ua.PubSub.Tests` in CI — `UdpLoopbackActionResponderAnswersRequesterAsync` and `UdpLoopbackDiscoveryPublisherAnswersSubscriberRequests` — are **unrelated** to this PR (UDP-loopback request/response tests in the PubSub library, associated with #3918) and are timing/environment-flaky. They are out of scope for this change. ## Related Issues - Fixes the recurring nightly Chaos TCP failure (workflow: `.github/workflows/stress-test.yml`). ## 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. - [ ] 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.
1 parent 28a1779 commit c8bc1bb

9 files changed

Lines changed: 435 additions & 28 deletions

File tree

.github/workflows/preview-publish.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ jobs:
191191
# Scoped to nuget/ - the MigrationAnalyzer nuspec at Tools/Opc.Ua.MigrationAnalyzer/
192192
# uses MSBuild $(...) substitutions that only `dotnet pack` resolves; standalone
193193
# `nuget.exe pack` would crash on it. See PR #3872.
194-
continueOnError: true
194+
continue-on-error: true
195195
run: |
196196
$nuspecs = Get-ChildItem -Path 'nuget' -Filter 'Opc.*.nuspec' -File
197197
if (-not $nuspecs) {
@@ -209,7 +209,7 @@ jobs:
209209
- name: Sign NuGet packages (nupkg + snupkg)
210210
if: env.SIGNING_CLIENT_SECRET != ''
211211
shell: pwsh
212-
continueOnError: true
212+
continue-on-error: true
213213
run: |
214214
$nupkgs = Get-ChildItem -Path "$env:ARTIFACTS_DIR" -Filter 'OPCFoundation.*.nupkg' -File
215215
$snupkgs = Get-ChildItem -Path "$env:ARTIFACTS_DIR" -Filter 'OPCFoundation.*.snupkg' -File

Libraries/Opc.Ua.Client/Session/ManagedSession.cs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -990,9 +990,17 @@ private async Task<ServiceResult> HandleConnectAsync(
990990
// channel manager would fail to resolve a factory for
991991
// the bare "https" scheme. Skip the refresh for the
992992
// OpenAPI profiles to preserve the caller-supplied URL.
993+
//
994+
// Otherwise honour the caller's ConfiguredEndpoint.UpdateBeforeConnect
995+
// choice (default true). A caller that pins a specific URL - for
996+
// example connecting through a proxy / gateway / NAT whose host and
997+
// port differ from the server's advertised EndpointUrl - sets the
998+
// flag to false so the channel is opened against exactly that URL
999+
// instead of re-discovering and adopting the server-advertised URL.
9931000
string? profile = ConfiguredEndpoint.Description.TransportProfileUri;
994-
bool updateBeforeConnect = !Profiles.IsHttpsOpenApi(profile) &&
995-
!Profiles.IsWssOpenApi(profile);
1001+
bool updateBeforeConnect = ConfiguredEndpoint.UpdateBeforeConnect
1002+
&& !Profiles.IsHttpsOpenApi(profile)
1003+
&& !Profiles.IsWssOpenApi(profile);
9961004

9971005
IDisposable? connectLease = null;
9981006
Session session;

Stack/Opc.Ua.Core/Stack/Client/Channels/Internal/ChannelEntry.cs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,16 @@ public ChannelState State
9999
}
100100
}
101101

102+
/// <summary>
103+
/// Monotonic counter incremented each time the entry enters
104+
/// <see cref="ChannelState.TransportReconnecting"/>. Callers capture
105+
/// it before sending a request and compare afterwards to tell a
106+
/// genuinely undetected transport drop (generation unchanged) apart
107+
/// from a stale in-flight failure that arrives after the shared
108+
/// channel has already begun (or finished) reconnecting.
109+
/// </summary>
110+
internal long ReconnectGeneration => Interlocked.Read(ref m_reconnectGeneration);
111+
102112
public ITransportChannel? Underlying
103113
{
104114
get
@@ -1215,6 +1225,10 @@ private void TransitionTo(ChannelState next, ServiceResult? error, int attempt)
12151225
return;
12161226
}
12171227
m_state = next;
1228+
if (next == ChannelState.TransportReconnecting)
1229+
{
1230+
Interlocked.Increment(ref m_reconnectGeneration);
1231+
}
12181232
m_lastStateChange = OwnerManager.TimeProvider.GetUtcNow();
12191233
m_lastReconnectAttempt = attempt;
12201234
m_lastError = error;
@@ -1291,6 +1305,7 @@ private struct AggregatedReactivationOutcome
12911305
private ServiceResult? m_lastError;
12921306
private ChannelState m_state = ChannelState.Disconnected;
12931307
private ITransportChannel? m_underlying;
1308+
private long m_reconnectGeneration;
12941309
private long m_clientCertificateVersion;
12951310
private TaskCompletionSource<bool> m_readyGate;
12961311
private TaskCompletionSource<bool>? m_reconnectCoalescer;

Stack/Opc.Ua.Core/Stack/Client/Channels/Internal/ManagedTransportChannelLease.cs

Lines changed: 103 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -242,13 +242,109 @@ public async ValueTask<IServiceResponse> SendRequestAsync(
242242
return await bypass.SendRequestAsync(request, ct).ConfigureAwait(false);
243243
}
244244

245-
await Entry.WaitForReadyAsync(ct).ConfigureAwait(false);
245+
int attempt = 0;
246+
while (true)
247+
{
248+
await Entry.WaitForReadyAsync(ct).ConfigureAwait(false);
249+
250+
ChannelEntry entry = Entry;
251+
long generation = entry.ReconnectGeneration;
252+
ITransportChannel? underlying = entry.Underlying
253+
?? throw ServiceResultException.Create(
254+
StatusCodes.BadSecureChannelClosed,
255+
"Channel has no underlying transport.");
256+
try
257+
{
258+
return await underlying.SendRequestAsync(request, ct).ConfigureAwait(false);
259+
}
260+
catch (ServiceResultException sre) when (
261+
IsActive &&
262+
attempt < kMaxTransientSendRetries &&
263+
!ct.IsCancellationRequested &&
264+
IsIdempotentRequest(request) &&
265+
IsTransientChannelError(sre.StatusCode))
266+
{
267+
// The shared channel broke while the request was in
268+
// flight. If no reconnect has begun since we sent (the
269+
// generation is unchanged) and the entry still reports
270+
// Ready, this is a transport drop the manager has not
271+
// detected yet - force a (coalesced) reconnect so the
272+
// shared channel recovers. Otherwise a reconnect is
273+
// already in progress or completed (a stale in-flight
274+
// failure): loop back, wait for the recovered channel and
275+
// resend without starting an extra reconnect cycle. Either
276+
// way the drop is transparent for the caller. Retries are
277+
// bounded and the reconnect itself is governed by the
278+
// reconnect policy / budget; if the channel terminally
279+
// faults the original transport error is surfaced.
280+
attempt++;
281+
ChannelState state = entry.State;
282+
if (state is ChannelState.Closed or ChannelState.Faulted)
283+
{
284+
// The entry is already terminal (e.g. the reconnect
285+
// policy / budget was exhausted by a concurrent cycle).
286+
// Surface the ORIGINAL transport error rather than
287+
// looping back into WaitForReadyAsync, which would throw
288+
// a generic BadSecureChannelClosed and mask the real
289+
// failure signal.
290+
throw;
291+
}
292+
if (entry.ReconnectGeneration == generation &&
293+
state == ChannelState.Ready)
294+
{
295+
bool recovered;
296+
try
297+
{
298+
recovered = await entry.RequestReconnectAsync(ct).ConfigureAwait(false);
299+
}
300+
catch (ServiceResultException)
301+
{
302+
throw sre;
303+
}
304+
305+
if (!recovered)
306+
{
307+
throw;
308+
}
309+
}
310+
}
311+
}
312+
}
246313

247-
ITransportChannel? underlying = Entry.Underlying
248-
?? throw ServiceResultException.Create(
249-
StatusCodes.BadSecureChannelClosed,
250-
"Channel has no underlying transport.");
251-
return await underlying.SendRequestAsync(request, ct).ConfigureAwait(false);
314+
private static bool IsIdempotentRequest(IServiceRequest request)
315+
{
316+
// Only requests with no server-side side effects are safe to
317+
// transparently re-send after a transport drop: the request may
318+
// already have been processed on the server before the
319+
// connection died, so re-sending a non-idempotent request
320+
// (Create*, Write, Call, AddNodes, HistoryUpdate, Publish,
321+
// RegisterNodes, BrowseNext, Query*, ...) could double-apply it
322+
// or corrupt continuation state. Non-idempotent requests surface
323+
// the transport error instead, letting the caller or a
324+
// higher-level recovery loop (for example the subscription
325+
// engine's own re-create) decide how to retry.
326+
return request is ReadRequest
327+
or BrowseRequest
328+
or TranslateBrowsePathsToNodeIdsRequest
329+
or GetEndpointsRequest
330+
or FindServersRequest
331+
or FindServersOnNetworkRequest;
332+
}
333+
334+
private static bool IsTransientChannelError(StatusCode statusCode)
335+
{
336+
uint code = statusCode.CodeBits;
337+
return code == StatusCodes.BadConnectionClosed
338+
|| code == StatusCodes.BadSecureChannelClosed
339+
|| code == StatusCodes.BadSecureChannelIdInvalid
340+
|| code == StatusCodes.BadNotConnected
341+
|| code == StatusCodes.BadConnectionRejected
342+
|| code == StatusCodes.BadServerNotConnected
343+
|| code == StatusCodes.BadServerHalted
344+
|| code == StatusCodes.BadNoCommunication
345+
|| code == StatusCodes.BadCommunicationError
346+
|| code == StatusCodes.BadTcpInternalError
347+
|| code == StatusCodes.BadRequestInterrupted;
252348
}
253349

254350
/// <inheritdoc/>
@@ -310,6 +406,7 @@ private async ValueTask DisposeAsyncCore()
310406
private ChannelEntry m_entry;
311407
private int m_active;
312408
private int m_swapCount;
409+
private const int kMaxTransientSendRetries = 3;
313410
private readonly Lock m_participantLock = new();
314411
private IReconnectParticipant m_participant;
315412
}

0 commit comments

Comments
 (0)