From d64c77b62d4d9809c2e6a0eade83cca82636c730 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 17:08:44 -0400 Subject: [PATCH] fix(opcuaclient): shutdown cancels session work before the gate wait; dispose skips a redundant nested shutdown (review r2 R2-1, R2-2) - R2-1: the ConditionRefresh sweeps (RefreshRequired, post-outage recovery) run under the session's work token, and ShutdownAsync cancels session work first thing - so a sweep holding the subscription gate against a stalling upstream no longer eats the whole teardown budget the deletes and close need. - R2-2: DisposeAsync skips its nested ShutdownAsync when a shutdown already started for the current lifecycle generation (nothing initialised since), so PostStop's dispose does not re-wait the gate past its budget. Test: with the gate held, the shutdown waits (positive control) and the dispose does not wait again; red with the skip removed. Refs #743 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../OpcUaClientDriver.cs | 28 ++++++++++++++++--- .../OpcUaClientLifecycleGenerationTests.cs | 21 ++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index df7599ed..7cb2ce33 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -1234,6 +1234,14 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit using var budget = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); budget.CancelAfter(TeardownBudget); + // r2 R2-1: cancel the session's background work FIRST — a ConditionRefresh sweep holding the subscription gate + // against a stalling upstream would otherwise make the gate wait below spend the whole wire budget, leaving the + // deletes and the close none. (ReleaseSessionAsync cancels it again; that is idempotent.) + CancelSessionWork(); + + // r2 R2-2: a later DisposeAsync for this same lifecycle skips its nested shutdown. + Interlocked.Exchange(ref _shutdownStartedGeneration, generation); + // r1 N-2: subscription management is held off for the whole teardown. A subscribe already in flight finishes // first (bounded by the budget) and is enumerated below; one that arrives during the delete phase waits and // then finds the session released, instead of registering a subscription on the closing session after the @@ -2281,6 +2289,9 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit /// private long _lifecycleGeneration; + /// The lifecycle generation the last started under (r2 R2-2). + private long _shutdownStartedGeneration = -1; + /// Test seam: the current lifecycle generation. internal long LifecycleGenerationForTest => Volatile.Read(ref _lifecycleGeneration); @@ -2700,7 +2711,9 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit if (IsRefreshRequired(fields)) { // Part 9 §5.11.7: the server lost events (queue overflow / restart) and asks for a refresh. - _ = Task.Run(() => RefreshAllAlarmSubscriptionsAsync(CancellationToken.None)); + // r2 R2-1: bound to the session's work token, which a shutdown cancels first thing. + var sessionWork = _sessionWorkCts.Token; + _ = Task.Run(() => RefreshAllAlarmSubscriptionsAsync(sessionWork)); return; } @@ -2821,6 +2834,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit await _subscriptionGate.WaitAsync(cancellationToken).ConfigureAwait(false); } catch (ObjectDisposedException) { return; } + catch (OperationCanceledException) { return; /* the session's work was cancelled (shutdown / re-init) */ } try { var session = Session; @@ -3418,7 +3432,7 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit if (!Current()) return; AnnotateAlarmQuality(StatusGood, Clock.GetUtcNow().UtcDateTime); if (!_alarmSubscriptions.IsEmpty) - await RefreshAllAlarmSubscriptionsAsync(CancellationToken.None).ConfigureAwait(false); + await RefreshAllAlarmSubscriptionsAsync(_sessionWorkCts.Token).ConfigureAwait(false); } catch (Exception ex) { @@ -3645,8 +3659,14 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit { if (_disposed) return; _disposed = true; - try { await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); } - catch { /* disposal is best-effort */ } + // r2 R2-2: the host's PostStop runs ShutdownAsync then DisposeAsync under one budget. When nothing has happened + // since that shutdown started (no init since), a nested shutdown would only re-wait the gate the first one may + // still hold and overrun the dispose bound — the running / finished shutdown already covers the teardown. + if (Volatile.Read(ref _shutdownStartedGeneration) != Volatile.Read(ref _lifecycleGeneration)) + { + try { await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); } + catch { /* disposal is best-effort */ } + } // Detach the cert-validation handler. if (_certValidationHandler is not null && _certValidatorRef is not null) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLifecycleGenerationTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLifecycleGenerationTests.cs index db12dbd6..ae26ca54 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLifecycleGenerationTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientLifecycleGenerationTests.cs @@ -257,4 +257,25 @@ public sealed class OpcUaClientLifecycleGenerationTests session.Verify(s => s.AddSubscription(It.IsAny()), Times.Once, "the late subscribe must not have created anything on the closing session"); } + + /// + /// r2 R2-2: PostStop runs ShutdownAsync then DisposeAsync. With subscription management contended (a subscribe + /// still holding the gate), the shutdown spends its budget waiting on the gate; the dispose that follows must not + /// re-run a nested shutdown and wait all over again. Positive control: the shutdown itself did wait. + /// + [Fact] + public async Task Dispose_after_a_shutdown_of_the_same_lifecycle_does_not_rewait_the_gate() + { + var drv = new OpcUaClientDriver(Options(), "opcua-r22") { TeardownBudget = TimeSpan.FromSeconds(3) }; + drv.SetSessionForTest(NewSession().Object); + await drv.SubscriptionGate.WaitAsync(TestContext.Current.CancellationToken); // a "subscribe" that never finishes + + var sw = Stopwatch.StartNew(); + await drv.ShutdownAsync(TestContext.Current.CancellationToken); + sw.Elapsed.ShouldBeGreaterThan(TimeSpan.FromSeconds(2), "positive control: the shutdown waited on the held gate"); + + sw.Restart(); + await drv.DisposeAsync(); + sw.Elapsed.ShouldBeLessThan(TimeSpan.FromSeconds(2), "the dispose must not wait the 3 s gate budget a second time"); + } }