From 5a5c80ee57e5d3962a48a275e4a9d7a985d61629 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 06:59:38 -0400 Subject: [PATCH] fix(drivers): join FOCAS and AbLegacy background loops on shutdown (#602 STAB-23/6) Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- .../AbLegacyDriver.cs | 49 ++++++++++++++-- .../FocasDriver.cs | 44 +++++++++++--- .../AbLegacyShutdownJoinTests.cs | 54 ++++++++++++++++++ .../FocasShutdownJoinTests.cs | 57 +++++++++++++++++++ 4 files changed, 193 insertions(+), 11 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyShutdownJoinTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasShutdownJoinTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 0a11cdb6..72196c08 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -229,7 +229,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover { state.ProbeCts = new CancellationTokenSource(); var ct = state.ProbeCts.Token; - _ = Task.Run(() => ProbeLoopAsync(state, ct), ct); + state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); } } _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); @@ -245,10 +245,13 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover // handles alive. Mirrors the body of ShutdownAsync without awaiting the poll engine // (nothing has been subscribed yet at init time). foreach (var state in _devices.Values) - { try { state.ProbeCts?.Cancel(); } catch { } + JoinProbeLoopsSync(); + foreach (var state in _devices.Values) + { state.ProbeCts?.Dispose(); state.ProbeCts = null; + state.ProbeTask = null; state.DisposeRuntimes(); } _devices.Clear(); @@ -266,15 +269,45 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover await InitializeAsync(driverConfigJson, cancellationToken).ConfigureAwait(false); } + /// Upper bound on how long shutdown waits for the probe loops to observe cancellation. + internal static readonly TimeSpan LoopJoinTimeout = TimeSpan.FromSeconds(5); + + /// + /// Synchronous bounded join of the (already-cancelled) probe loops for the sync teardown paths. + /// The loop body uses ConfigureAwait(false) throughout, so blocking here cannot deadlock. + /// + private void JoinProbeLoopsSync() + { + var loops = _devices.Values.Select(d => d.ProbeTask).OfType().ToArray(); + if (loops.Length == 0) return; + try { Task.WaitAll(loops, LoopJoinTimeout); } + catch (AggregateException) { /* loops observe cancellation; a fault is not actionable at teardown */ } + } + /// public async Task ShutdownAsync(CancellationToken cancellationToken) { await _poll.DisposeAsync().ConfigureAwait(false); + // STAB-6: cancel the probe loops, JOIN them (bounded), then dispose the CTSs and runtimes. + // The loop used to be fire-and-forget, so one parked in a probe-runtime create/read could + // outlive shutdown and keep a libplctag handle alive past DisposeRuntimes. + foreach (var state in _devices.Values) + try { state.ProbeCts?.Cancel(); } catch { } + var loops = _devices.Values.Select(d => d.ProbeTask).OfType().ToList(); + if (loops.Count > 0) + { + var all = Task.WhenAll(loops); + var finished = await Task.WhenAny(all, Task.Delay(LoopJoinTimeout, CancellationToken.None)).ConfigureAwait(false); + if (finished != all) + _logger.LogWarning( + "AbLegacy shutdown: {Count} probe loop(s) did not stop within {Timeout}; proceeding with teardown. Driver={DriverInstanceId}", + loops.Count, LoopJoinTimeout, _driverInstanceId); + } foreach (var state in _devices.Values) { - try { state.ProbeCts?.Cancel(); } catch { } state.ProbeCts?.Dispose(); state.ProbeCts = null; + state.ProbeTask = null; state.DisposeRuntimes(); } _devices.Clear(); @@ -892,10 +925,13 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover // can't poison the rest of the disposal chain. try { _poll.DisposeAsync().ConfigureAwait(false).GetAwaiter().GetResult(); } catch { } foreach (var state in _devices.Values) - { try { state.ProbeCts?.Cancel(); } catch { } + JoinProbeLoopsSync(); + foreach (var state in _devices.Values) + { state.ProbeCts?.Dispose(); state.ProbeCts = null; + state.ProbeTask = null; state.DisposeRuntimes(); } _devices.Clear(); @@ -1021,6 +1057,11 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover /// public CancellationTokenSource? ProbeCts { get; set; } + /// + /// Gets or sets the probe loop task, retained so shutdown can join it (STAB-6). + /// + public Task? ProbeTask { get; set; } + /// /// Gets or sets a value indicating whether the probe has been initialized. /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index 9fb93432..07fc913c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -277,7 +277,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { state.ProbeCts = new CancellationTokenSource(); var ct = state.ProbeCts.Token; - _ = Task.Run(() => ProbeLoopAsync(state, ct), ct); + state.ProbeTask = Task.Run(() => ProbeLoopAsync(state, ct), ct); } } @@ -287,7 +287,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { state.RecycleCts = new CancellationTokenSource(); var ct = state.RecycleCts.Token; - _ = Task.Run(() => RecycleLoopAsync(state, ct), ct); + state.RecycleTask = Task.Run(() => RecycleLoopAsync(state, ct), ct); } } @@ -305,7 +305,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, { state.FixedTreeCts = new CancellationTokenSource(); var ct = state.FixedTreeCts.Token; - _ = Task.Run(() => FixedTreeLoopAsync(state, ct), ct); + state.FixedTreeTask = Task.Run(() => FixedTreeLoopAsync(state, ct), ct); } } @@ -326,6 +326,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, await InitializeAsync(driverConfigJson, cancellationToken).ConfigureAwait(false); } + /// Upper bound on how long shutdown waits for the background loops to observe cancellation. + internal static readonly TimeSpan LoopJoinTimeout = TimeSpan.FromSeconds(5); + /// public async Task ShutdownAsync(CancellationToken cancellationToken) { @@ -335,22 +338,43 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, await proj.DisposeAsync().ConfigureAwait(false); _alarmProjection = null; } + // STAB-23: cancel every loop first, JOIN them (bounded), and only then dispose the CTSs and the + // client. Fire-and-forget loops used to outlive shutdown: one parked in EnsureConnectedAsync could + // finish its connect AFTER DisposeClient and leave a live socket on a shut-down driver. + var loops = new List(); foreach (var state in _devices.Values) { // Cancel-then-dispose can race in tight shutdown loops; swallowing is intentional // but we now log the cause so a noisy shutdown leaves a Debug trace. try { state.ProbeCts?.Cancel(); } catch (Exception ex) { _logger.LogDebug(ex, "Cancelling probe CTS for {Host} failed", state.Options.HostAddress); } - state.ProbeCts?.Dispose(); - state.ProbeCts = null; try { state.RecycleCts?.Cancel(); } catch (Exception ex) { _logger.LogDebug(ex, "Cancelling recycle CTS for {Host} failed", state.Options.HostAddress); } - state.RecycleCts?.Dispose(); - state.RecycleCts = null; try { state.FixedTreeCts?.Cancel(); } catch (Exception ex) { _logger.LogDebug(ex, "Cancelling fixed-tree CTS for {Host} failed", state.Options.HostAddress); } + if (state.ProbeTask is { } pt) loops.Add(pt); + if (state.RecycleTask is { } rt) loops.Add(rt); + if (state.FixedTreeTask is { } ft) loops.Add(ft); + } + if (loops.Count > 0) + { + var all = Task.WhenAll(loops); + var finished = await Task.WhenAny(all, Task.Delay(LoopJoinTimeout, CancellationToken.None)).ConfigureAwait(false); + if (finished != all) + _logger.LogWarning("FOCAS shutdown: {Count} background loop(s) did not stop within {Timeout}; proceeding with teardown", + loops.Count, LoopJoinTimeout); + } + foreach (var state in _devices.Values) + { + state.ProbeCts?.Dispose(); + state.ProbeCts = null; + state.ProbeTask = null; + state.RecycleCts?.Dispose(); + state.RecycleCts = null; + state.RecycleTask = null; state.FixedTreeCts?.Dispose(); state.FixedTreeCts = null; + state.FixedTreeTask = null; state.DisposeClient(); } _devices.Clear(); @@ -1575,6 +1599,12 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, public CancellationTokenSource? RecycleCts { get; set; } /// Gets or sets the cancellation token source for the fixed-tree loop. public CancellationTokenSource? FixedTreeCts { get; set; } + /// The probe loop task, retained so shutdown can join it (STAB-23). + public Task? ProbeTask { get; set; } + /// The recycle loop task, retained so shutdown can join it (STAB-23). + public Task? RecycleTask { get; set; } + /// The fixed-tree loop task, retained so shutdown can join it (STAB-23). + public Task? FixedTreeTask { get; set; } /// Gets or sets the fixed-tree cache for this device. public FocasFixedTreeCache? FixedTreeCache { get; set; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyShutdownJoinTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyShutdownJoinTests.cs new file mode 100644 index 00000000..7b610135 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyShutdownJoinTests.cs @@ -0,0 +1,54 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// STAB-6 / #602 — the probe loop was fire-and-forget, so shutdown cancelled and disposed while the +/// loop could still be parked in a probe-runtime call that ignores cancellation, leaving its +/// libplctag handle alive past teardown. Shutdown now joins the loop (bounded) first. +/// +[Trait("Category", "Unit")] +public sealed class AbLegacyShutdownJoinTests +{ + private sealed class ParkedInitTag( + AbLegacyTagCreateParams p, TaskCompletionSource entered, TaskCompletionSource release) : FakeAbLegacyTag(p) + { + public override async Task InitializeAsync(CancellationToken ct) + { + entered.TrySetResult(); + await release.Task.ConfigureAwait(false); // deliberately ignores ct + } + } + + /// Shutdown blocks on a probe loop parked in runtime init, then the loop disposes its probe runtime. + [Fact] + public async Task Shutdown_WaitsForProbeLoopParkedInInit_ThenProbeRuntimeIsDisposed() + { + var entered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeAbLegacyTagFactory + { + Customise = p => new ParkedInitTag(p, entered, release), + }; + var drv = new AbLegacyDriver(new AbLegacyDriverOptions + { + Devices = [new AbLegacyDeviceOptions("ab://10.0.0.5/1,0", AbLegacyPlcFamily.Slc500)], + Probe = new AbLegacyProbeOptions { Enabled = true, Interval = TimeSpan.FromMilliseconds(50), ProbeAddress = "S:0" }, + }, "drv-join", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + await entered.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + var shutdown = drv.ShutdownAsync(CancellationToken.None); + + await Task.Delay(300); + shutdown.IsCompleted.ShouldBeFalse("shutdown must join the probe loop parked in runtime init"); + + // Positive control: releasing the parked call lets shutdown finish, with the probe runtime released. + release.SetResult(); + await shutdown.WaitAsync(TimeSpan.FromSeconds(5)); + + factory.Tags["S:0"].Disposed.ShouldBeTrue(); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasShutdownJoinTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasShutdownJoinTests.cs new file mode 100644 index 00000000..6e07ae92 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasShutdownJoinTests.cs @@ -0,0 +1,57 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// STAB-23 / #602 — the probe / recycle / fixed-tree loops were fire-and-forget, so shutdown +/// cancelled and disposed while a loop could still be parked inside a connect that ignores +/// cancellation, and that connect then completed against a shut-down driver. Shutdown now joins +/// the loops (bounded) before disposing the client. +/// +[Trait("Category", "Unit")] +public sealed class FocasShutdownJoinTests +{ + /// A client whose connect parks until released and deliberately ignores the cancellation token. + private sealed class ParkedConnectClient(TaskCompletionSource entered, TaskCompletionSource release) : FakeFocasClient + { + public override async Task ConnectAsync(FocasHostAddress address, TimeSpan timeout, CancellationToken ct) + { + entered.TrySetResult(); + await release.Task.ConfigureAwait(false); + await base.ConnectAsync(address, timeout, CancellationToken.None).ConfigureAwait(false); + } + } + + /// Shutdown blocks on a probe loop parked in connect, then disposes the client that connect produced. + [Fact] + public async Task Shutdown_WaitsForLoopParkedInConnect_ThenDisposesItsClient() + { + var entered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var release = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var factory = new FakeFocasClientFactory { Customise = () => new ParkedConnectClient(entered, release) }; + var options = new FocasDriverOptions + { + Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")], + RawTags = [FocasRawTags.Entry(new FocasTagDefinition("R", "focas://10.0.0.5:8193", "R100", FocasDataType.Byte))], + Probe = new FocasProbeOptions { Enabled = true, Interval = TimeSpan.FromMilliseconds(50), Timeout = TimeSpan.FromSeconds(10) }, + }; + var drv = new FocasDriver(options, "focas-1", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + await entered.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + var shutdown = drv.ShutdownAsync(CancellationToken.None); + + await Task.Delay(300); + shutdown.IsCompleted.ShouldBeFalse("shutdown must join the probe loop parked in connect"); + + // Positive control: releasing the parked connect lets shutdown finish. + release.SetResult(); + await shutdown.WaitAsync(TimeSpan.FromSeconds(5)); + + factory.Clients.ShouldNotBeEmpty(); + factory.Clients.ShouldAllBe(c => c.DisposeCount >= 1); + factory.Clients.ShouldAllBe(c => !c.IsConnected); + } +}