fix(focas): recycle the device handle under ConnectGate and drain it first (R07:L6)

The handle-recycle loop disposed the device client with no gate. It could null a client a concurrent EnsureConnectedAsync had just installed (orphaning a live two-socket session), or dispose one a poll was mid-read on (ObjectDisposedException -> BadCommunicationError + Degraded on every recycle interval). The recycle now takes the device's ConnectGate, detaches the client, and disposes it via SynchronizedFocasClient.DisposeWhenIdleAsync, which waits (bounded by the call timeout plus slack) for ops that already hold the client. The gate stays held through the drain, so no fresh session opens beside the old one. SynchronizedFocasClient no longer disposes its call gate. Releasing a disposed semaphore threw out of reads that had otherwise completed. Ops that reach a disposed client now get ObjectDisposedException and are never run on the dead socket. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 14:21:02 -04:00
parent 256915b7e4
commit 4c4dfaadcc
3 changed files with 158 additions and 18 deletions
@@ -1779,6 +1779,13 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,
        return null;
    }

+    /// <summary>
+    ///     How long a handle recycle waits for in-flight ops on the detached client: one call timeout (each op is
+    ///     bounded by it) plus slack, so a recycle never kills a read that would have completed.
+    /// </summary>
+    private TimeSpan RecycleDrainBudget =>
+        (_options.Timeout > TimeSpan.Zero ? _options.Timeout : TimeSpan.FromSeconds(2)) + TimeSpan.FromMilliseconds(500);
+
    private async Task RecycleLoopAsync(DeviceState state, CancellationToken ct)
    {
        while (!ct.IsCancellationRequested)
@@ -1788,14 +1795,31 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery,

            // Close the current handle — the next Read / Write / Probe call triggers
            // EnsureConnectedAsync, which reopens a fresh one. We don't block here on
-            // reconnect because the goal is just to release the FWLIB handle slot; a
-            // readable tick one probe cycle later is an acceptable cost.
-            try { state.DisposeClient(); }
+            // reconnect because the goal is just to release the FWLIB handle slot.
+            //
+            // Under the device's ConnectGate, exactly like EnsureConnectedAsync's install: unserialised, the
+            // recycle could null a client a concurrent connect had just installed (orphaning a live two-socket
+            // session) or dispose one a poll was mid-read on (BadCommunicationError + Degraded every interval).
+            // The gate is held through the drain so no fresh session opens beside the old one — the CNC's
+            // handle pool is small — and the detached client is disposed only once the ops already holding it
+            // have finished (bounded by the call timeout).
+            try { await state.ConnectGate.WaitAsync(ct).ConfigureAwait(false); }
+            catch (OperationCanceledException) { break; }
+            try
+            {
+                var old = state.Client;
+                state.Client = null;
+                if (old is SynchronizedFocasClient sync)
+                    await sync.DisposeWhenIdleAsync(RecycleDrainBudget).ConfigureAwait(false);
+                else
+                    old?.Dispose();
+            }
            catch (Exception ex)
            {
                /* already disposed or race — next EnsureConnected recovers */
                _logger.LogDebug(ex, "FOCAS handle-recycle dispose failed for {Host}", state.Options.HostAddress);
            }
+            finally { state.ConnectGate.Release(); }
        }
    }

@@ -32,6 +32,9 @@ public sealed class SynchronizedFocasClient : IFocasClient
    private readonly IFocasClient _inner;
    private readonly TimeSpan _callTimeout;
    private readonly SemaphoreSlim _gate = new(1, 1);
+    /// <summary>Ops holding or queued for <see cref="_gate"/> — what <see cref="DisposeWhenIdleAsync"/> drains.</summary>
+    private int _pending;
+    private int _disposed;

    /// <summary>Wraps <paramref name="inner"/> with per-device serialization + a per-call timeout.</summary>
    /// <param name="inner">The underlying FOCAS client to serialize access to.</param>
@@ -115,26 +118,66 @@ public sealed class SynchronizedFocasClient : IFocasClient
    public Task<IReadOnlyList<double?>> GetSpindleMotorSpeedsAsync(CancellationToken cancellationToken) =>
        RunBoundedAsync(ct => _inner.GetSpindleMotorSpeedsAsync(ct), cancellationToken);

-    /// <summary>Disposes the inner client and the call gate.</summary>
+    /// <summary>
+    ///     Disposes the inner client. Ops still queued on the call gate then fail with
+    ///     <see cref="ObjectDisposedException"/> instead of running on a disposed socket. The gate itself is NOT
+    ///     disposed: an op that already holds it releases it on the way out, and releasing a disposed
+    ///     <see cref="SemaphoreSlim"/> threw out of a read that had otherwise completed.
+    /// </summary>
    public void Dispose()
    {
+        if (Interlocked.Exchange(ref _disposed, 1) != 0) return;
        _inner.Dispose();
-        _gate.Dispose();
+    }
+
+    /// <summary>
+    ///     Disposes once every op that already holds (or is queued for) this client has finished, waiting at most
+    ///     <paramref name="maxWait"/>. Used by the handle-recycle loop, which detaches the client under the device's
+    ///     connect gate first — so nothing new can reach it — and then must not kill a read that is mid-flight on
+    ///     it (that read used to surface BadCommunicationError + Degraded on every recycle tick).
+    /// </summary>
+    /// <param name="maxWait">Upper bound on the wait; the client is disposed regardless once it passes.</param>
+    /// <returns>A task that completes when the client is disposed.</returns>
+    public async Task DisposeWhenIdleAsync(TimeSpan maxWait)
+    {
+        var deadline = DateTime.UtcNow + maxWait;
+        while (Volatile.Read(ref _pending) > 0 && DateTime.UtcNow < deadline)
+            await Task.Delay(5, CancellationToken.None).ConfigureAwait(false);
+        Dispose();
+    }
+
+    /// <summary>Gets the number of ops currently holding or queued for the call gate. Test seam.</summary>
+    internal int PendingOperations => Volatile.Read(ref _pending);
+
+    private void ThrowIfDisposed()
+    {
+        if (Volatile.Read(ref _disposed) != 0)
+            throw new ObjectDisposedException(nameof(SynchronizedFocasClient), "The FOCAS client was recycled or disposed.");
    }

    // Gate only — the caller already governs the budget (connect timeout arg / probe linked token).
    private async Task<T> RunGatedAsync<T>(Func<CancellationToken, Task<T>> op, CancellationToken ct)
    {
-        await _gate.WaitAsync(ct).ConfigureAwait(false);
-        try { return await op(ct).ConfigureAwait(false); }
-        finally { _gate.Release(); }
+        Interlocked.Increment(ref _pending);
+        try
+        {
+            await _gate.WaitAsync(ct).ConfigureAwait(false);
+            try { ThrowIfDisposed(); return await op(ct).ConfigureAwait(false); }
+            finally { _gate.Release(); }
+        }
+        finally { Interlocked.Decrement(ref _pending); }
    }

    private async Task RunGatedAsync(Func<CancellationToken, Task> op, CancellationToken ct)
    {
-        await _gate.WaitAsync(ct).ConfigureAwait(false);
-        try { await op(ct).ConfigureAwait(false); }
-        finally { _gate.Release(); }
+        Interlocked.Increment(ref _pending);
+        try
+        {
+            await _gate.WaitAsync(ct).ConfigureAwait(false);
+            try { ThrowIfDisposed(); await op(ct).ConfigureAwait(false); }
+            finally { _gate.Release(); }
+        }
+        finally { Interlocked.Decrement(ref _pending); }
    }

    // Gate + per-call timeout. A fired timeout surfaces as OperationCanceledException whose token is
@@ -142,16 +185,22 @@ public sealed class SynchronizedFocasClient : IFocasClient
    // their own token's IsCancellationRequested.
    private async Task<T> RunBoundedAsync<T>(Func<CancellationToken, Task<T>> op, CancellationToken ct)
    {
-        await _gate.WaitAsync(ct).ConfigureAwait(false);
+        Interlocked.Increment(ref _pending);
        try
        {
-            if (_callTimeout <= TimeSpan.Zero)
-                return await op(ct).ConfigureAwait(false);
+            await _gate.WaitAsync(ct).ConfigureAwait(false);
+            try
+            {
+                ThrowIfDisposed();
+                if (_callTimeout <= TimeSpan.Zero)
+                    return await op(ct).ConfigureAwait(false);

-            using var linked = CancellationTokenSource.CreateLinkedTokenSource(ct);
-            linked.CancelAfter(_callTimeout);
-            return await op(linked.Token).ConfigureAwait(false);
+                using var linked = CancellationTokenSource.CreateLinkedTokenSource(ct);
+                linked.CancelAfter(_callTimeout);
+                return await op(linked.Token).ConfigureAwait(false);
+            }
+            finally { _gate.Release(); }
        }
-        finally { _gate.Release(); }
+        finally { Interlocked.Decrement(ref _pending); }
    }
 }
@@ -62,6 +62,73 @@ public sealed class FocasHandleRecycleTests
        await drv.ShutdownAsync(CancellationToken.None);
    }

+    /// <summary>A client whose reads take a while — long enough for a recycle tick to land mid-read.</summary>
+    private sealed class SlowReadClient : FakeFocasClient
+    {
+        public override async Task<(object? value, uint status)> ReadAsync(
+            FocasAddress address, FocasDataType type, CancellationToken ct)
+        {
+            await Task.Delay(200, ct).ConfigureAwait(false);
+            if (!IsConnected) throw new ObjectDisposedException(nameof(SlowReadClient), "disposed mid-read");
+            return ((sbyte)5, FocasStatusMapper.Good);
+        }
+    }
+
+    /// <summary>
+    ///     #740 / R07:L6 — the recycle loop disposed the device client with no gate, so a recycle tick landing
+    ///     mid-read killed that read (BadCommunicationError + Degraded, every interval). The recycle now takes the
+    ///     device's connect gate and lets the in-flight read finish before disposing the client it detached.
+    /// </summary>
+    [Fact]
+    public async Task Recycle_tick_landing_mid_read_lets_the_read_finish()
+    {
+        var factory = new FakeFocasClientFactory { Customise = () => new SlowReadClient() };
+        var drv = new FocasDriver(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 = false },
+            HandleRecycle = new FocasHandleRecycleOptions { Enabled = true, Interval = TimeSpan.FromMilliseconds(50) },
+        }, "drv-1", factory);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+
+        // Each 200 ms read spans at least three recycle ticks.
+        for (var i = 0; i < 3; i++)
+        {
+            var snap = (await drv.ReadAsync(["R"], CancellationToken.None))[0];
+            snap.StatusCode.ShouldBe(FocasStatusMapper.Good, $"read {i} was killed by a recycle tick");
+            snap.Value.ShouldBe((sbyte)5);
+        }
+        drv.GetHealth().State.ShouldBe(Core.Abstractions.DriverState.Healthy);
+
+        // Positive control: the recycle really ran — the first client was disposed and a fresh one opened.
+        await WaitFor(() => factory.Clients[0].DisposeCount > 0, TimeSpan.FromSeconds(3));
+        factory.Clients[0].DisposeCount.ShouldBeGreaterThan(0);
+        factory.Clients.Count.ShouldBeGreaterThan(1);
+        await drv.ShutdownAsync(CancellationToken.None);
+        factory.Clients.ShouldAllBe(c => !c.IsConnected, "no client opened around a recycle may be orphaned");
+    }
+
+    /// <summary>DisposeWhenIdleAsync drains the in-flight op, then disposes; a later op is refused, not run on a dead socket.</summary>
+    [Fact]
+    public async Task DisposeWhenIdle_drains_the_in_flight_op_then_refuses_new_ones()
+    {
+        var inner = new SlowReadClient();
+        await inner.ConnectAsync(new FocasHostAddress("10.0.0.5", 8193), TimeSpan.FromSeconds(1), CancellationToken.None);
+        var sync = new SynchronizedFocasClient(inner, TimeSpan.FromSeconds(5));
+        var addr = FocasAddress.TryParse("R100")!;
+
+        var read = sync.ReadAsync(addr, FocasDataType.Byte, CancellationToken.None);
+        await Task.Delay(20);
+        sync.PendingOperations.ShouldBe(1);
+        var dispose = sync.DisposeWhenIdleAsync(TimeSpan.FromSeconds(5));
+
+        (await read).status.ShouldBe(FocasStatusMapper.Good);
+        await dispose;
+        inner.DisposeCount.ShouldBe(1);
+        await Should.ThrowAsync<ObjectDisposedException>(() => sync.ReadAsync(addr, FocasDataType.Byte, CancellationToken.None));
+    }
+
    private static async Task WaitFor(Func<bool> pred, TimeSpan timeout)
    {
        var deadline = DateTime.UtcNow + timeout;