fix(twincat): review round 2 - teardown confined to its generation; write health; re-arm epoch

- I1: the poll engine belongs to a teardown generation. ShutdownAsync/Dispose retire the current engine and install a fresh one in the same swap as the device/tag tables, then dispose the retired one (possibly in the background after a cancelled or over-budget shutdown). A subscription made after the next InitializeAsync can no longer land in an engine whose late dispose tail would dispose its CTS without cancelling it (a loop no UnsubscribeAsync could find). Poll handles are generation-qualified (TwinCATPollHandle) because each engine numbers handles from 1; SubscribeAsync re-homes a subscription that raced a retirement. - M1: a write's status feeds per-device health - Good clears that device's comms fault and advances LastSuccessfulRead, a comms status degrades, a refusal keeps the state. - M2: a symbol-version re-arm bumps a per-registration epoch; an in-flight retry that fails afterwards no longer re-imposes the backoff. - M3: a re-arm restarts the refusal backoff at 30 s. - M4: documented that PLC-not-in-RUN detection needs the probe. - M5: WSTRING and multi-dimensional array encode-plan tests; the cancelled- shutdown test asserts Unknown health and awaits its background teardown. - M6: an infinity spelled as a string ('Infinity', '-inf') passes like a float/double infinity; a finite overflow stays BadOutOfRange. Refs #739 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 15:00:23 -04:00
parent 991d277c03
commit a8d313ed5a
5 changed files with 417 additions and 22 deletions
@@ -83,16 +83,31 @@ internal class FakeTwinCATClient : ITwinCATClient
    public virtual Task<(object? value, uint status)> ReadValueAsync(
        string symbolPath, TwinCATDataType type, int? bitIndex, int? arrayCount, CancellationToken ct)
    {
+        if (BlockRead is { } block)
+        {
+            // A read wedged in the (uncancellable) wire call; completes with the normal answer once released.
+            return block.Task.ContinueWith(_ =>
+            {
+                BlockRead = null;
+                return ReadValueAsync(symbolPath, type, bitIndex, arrayCount, CancellationToken.None).Result;
+            }, TaskScheduler.Default);
+        }
        if (ThrowOnRead) throw Exception ?? new InvalidOperationException();
-        SingleReadCount++;
+        Interlocked.Increment(ref _singleReadCount);
        LastReadArrayCount = arrayCount;
        var status = ReadStatuses.TryGetValue(symbolPath, out var s) ? s : TwinCATStatusMapper.Good;
        var value = Values.TryGetValue(symbolPath, out var v) ? v : null;
        return Task.FromResult((value, status));
    }

+    /// <summary>When set, <see cref="ReadValueAsync"/> does not complete (and ignores cancellation) until this is
+    ///     completed — a poll read wedged in the wire call.</summary>
+    public TaskCompletionSource? BlockRead { get; set; }
+
+    private int _singleReadCount;
+
    /// <summary>Gets the number of individual <see cref="ReadValueAsync"/> calls served.</summary>
-    public int SingleReadCount { get; private set; }
+    public int SingleReadCount { get => Volatile.Read(ref _singleReadCount); private set => Volatile.Write(ref _singleReadCount, value); }
    /// <summary>Gets the size of each <see cref="ReadValuesAsync"/> batch the driver issued, in order (#645).</summary>
    public List<int> SumReadBatches { get; } = new();
    /// <summary>Gets or sets an exception <see cref="ReadValuesAsync"/> throws instead of answering — a
@@ -264,6 +279,9 @@ internal class FakeTwinCATClient : ITwinCATClient
    public int AttemptsFor(string symbolPath) => _addAttempts.TryGetValue(symbolPath, out var n) ? n : 0;
    /// <summary>Records the most recently-supplied <c>maxDelayMs</c> for Driver.TwinCAT-014 tests.</summary>
    public int LastMaxDelayMs { get; private set; }
+    /// <summary>When set, <see cref="AddNotificationAsync"/> awaits this gate BEFORE applying
+    ///     <see cref="AddNotificationErrors"/> — wedges an attempt in flight whose outcome is a failure.</summary>
+    public TaskCompletionSource? PreAttemptGate { get; set; }
    /// <summary>When set, <see cref="AddNotificationAsync"/> awaits this gate before creating the handle —
    ///     lets a test wedge a replay/register mid-flight (STAB-17 orphan race).</summary>
    public TaskCompletionSource? AddNotificationGate { get; set; }
@@ -283,6 +301,7 @@ internal class FakeTwinCATClient : ITwinCATClient
        int maxDelayMs, Action<string, object?> onChange, CancellationToken cancellationToken)
    {
        _addAttempts.AddOrUpdate(symbolPath, 1, (_, n) => n + 1);
+        if (PreAttemptGate is { } pre) await pre.Task.ConfigureAwait(false);
        if (ThrowOnAddNotification)
            throw Exception ?? new InvalidOperationException("fake AddNotification failure");
        if (AddNotificationErrors.TryGetValue(symbolPath, out var perSymbol))
@@ -210,7 +210,15 @@ public sealed class TwinCATReviewFixRound1Tests
        await Should.ThrowAsync<OperationCanceledException>(() => drv.ShutdownAsync(cts.Token));
        sw.Elapsed.ShouldBeLessThan(TwinCATDriver.ProbeJoinTimeout, "the wait ended on the token, not the join bound");
        drv.DeviceCount.ShouldBe(0);
+        drv.GetHealth().State.ShouldBe(DriverState.Unknown);
+
+        // The background teardown finishes once the wedge releases, and disposes the client the wedged
+        // connect produced (no dangling teardown left behind the test).
+        var teardown = drv.LastTeardownForTest.ShouldNotBeNull();
        wedge.SetResult();
+        await teardown.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken);
+        await WaitForAsync(() => factory.Clients[0].DisposeCount > 0, TimeSpan.FromSeconds(3));
+        factory.Clients[0].DisposeCount.ShouldBeGreaterThan(0);
    }

    // ---- MINOR 7: the real STRING / array encode path through the client planning helpers ----
@@ -0,0 +1,262 @@
+using System.Collections.Concurrent;
+using Shouldly;
+using TwinCAT.TypeSystem;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests;
+
+/// <summary>
+///     #739 review fix round 2: a cancelled shutdown's background teardown is confined to its own generation
+///     (I1); write outcomes feed per-device health (M1); a symbol-version re-arm survives an in-flight retry
+///     and restarts the backoff (M2/M3); WSTRING / multi-dimensional encode plans (M5); infinity strings (M6).
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class TwinCATReviewFixRound2Tests
+{
+    private const string Host = "ads://5.23.91.23.1.1:851";
+
+    private static TwinCATProbeOptions FastProbe => new()
+    {
+        Enabled = true,
+        Interval = TimeSpan.FromMilliseconds(30),
+        Timeout = TimeSpan.FromMilliseconds(100),
+    };
+
+    private static TwinCATDriver NewDriver(FakeTwinCATClientFactory factory, TwinCATProbeOptions probe, bool native,
+        params TwinCATTagDefinition[] tags) =>
+        new(new TwinCATDriverOptions
+        {
+            Devices = [new TwinCATDeviceOptions(Host)],
+            RawTags = TwinCATRawTags.Entries(tags),
+            Probe = probe,
+            UseNativeNotifications = native,
+        }, "drv-1", factory);
+
+    private static async Task WaitForAsync(Func<bool> condition, TimeSpan timeout)
+    {
+        var deadline = DateTime.UtcNow + timeout;
+        while (!condition() && DateTime.UtcNow < deadline)
+            await Task.Delay(10);
+    }
+
+    // ---- I1 ----
+
+    /// <summary>
+    ///     The exact reviewed sequence: a poll read is wedged; ShutdownAsync is cancelled (its teardown
+    ///     continues in the background); the same instance is re-initialised and subscribed; the wedge is
+    ///     released and the OLD teardown completes. The NEW subscription keeps polling and delivering (positive
+    ///     control), UnsubscribeAsync finds and stops it, and a final shutdown leaves nothing polling.
+    /// </summary>
+    [Fact]
+    public async Task A_cancelled_shutdowns_background_teardown_never_touches_the_next_generations_subscription()
+    {
+        var wedge = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var counter = 0;
+        var clients = new ConcurrentQueue<FakeTwinCATClient>();
+        var factory = new FakeTwinCATClientFactory
+        {
+            Customise = () =>
+            {
+                // Client 1 (generation 1) wedges its first read; later clients answer an ever-changing value
+                // so every poll publishes.
+                var c = new ChangingValueFake(() => Interlocked.Increment(ref counter))
+                {
+                    BlockRead = clients.IsEmpty ? wedge : null,
+                };
+                clients.Enqueue(c);
+                return c;
+            },
+        };
+        var tag = new TwinCATTagDefinition("A", Host, "MAIN.A", TwinCATDataType.DInt);
+        var drv = NewDriver(factory, new TwinCATProbeOptions { Enabled = false }, native: false, tag);
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        var events = new ConcurrentQueue<DataChangeEventArgs>();
+        drv.OnDataChange += (_, e) => events.Enqueue(e);
+
+        _ = await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken);
+        await WaitForAsync(() => !clients.IsEmpty, TimeSpan.FromSeconds(2)); // gen-1 poll read is wedged
+
+        using (var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(100)))
+            await Should.ThrowAsync<OperationCanceledException>(() => drv.ShutdownAsync(cts.Token));
+        var oldTeardown = drv.LastTeardownForTest.ShouldNotBeNull();
+        oldTeardown.IsCompleted.ShouldBeFalse("the old teardown must still be running in the background");
+
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        var handle = await drv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken);
+
+        wedge.SetResult();
+        await oldTeardown.WaitAsync(TimeSpan.FromSeconds(10), TestContext.Current.CancellationToken);
+
+        // Positive control: the new subscription delivers AFTER the old teardown finished.
+        var afterTeardown = events.Count(e => e.SubscriptionHandle.Equals(handle));
+        await WaitForAsync(() => events.Count(e => e.SubscriptionHandle.Equals(handle)) >= afterTeardown + 3, TimeSpan.FromSeconds(3));
+        events.Count(e => e.SubscriptionHandle.Equals(handle)).ShouldBeGreaterThanOrEqualTo(afterTeardown + 3);
+
+        // UnsubscribeAsync can still find and stop it.
+        await drv.UnsubscribeAsync(handle, TestContext.Current.CancellationToken);
+        var stoppedAt = events.Count(e => e.SubscriptionHandle.Equals(handle));
+        // Measured window: a sibling driver's poll publishes 5 times; the stopped loop must publish nothing.
+        var witnessFactory = new FakeTwinCATClientFactory { Customise = () => new ChangingValueFake(() => Environment.TickCount) };
+        var witnessDrv = NewDriver(witnessFactory, new TwinCATProbeOptions { Enabled = false }, native: false, tag);
+        await witnessDrv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        var witnessEvents = 0;
+        witnessDrv.OnDataChange += (_, _) => Interlocked.Increment(ref witnessEvents);
+        _ = await witnessDrv.SubscribeAsync(["A"], TimeSpan.FromMilliseconds(50), TestContext.Current.CancellationToken);
+        await WaitForAsync(() => Volatile.Read(ref witnessEvents) >= 5, TimeSpan.FromSeconds(3));
+        Volatile.Read(ref witnessEvents).ShouldBeGreaterThanOrEqualTo(5);
+        await witnessDrv.ShutdownAsync(TestContext.Current.CancellationToken);
+
+        events.Count(e => e.SubscriptionHandle.Equals(handle)).ShouldBe(stoppedAt, "the unsubscribed loop must have stopped");
+        await drv.ShutdownAsync(TestContext.Current.CancellationToken);
+    }
+
+    /// <summary>Answers each read with a new value so every poll cycle publishes.</summary>
+    private sealed class ChangingValueFake(Func<int> next) : FakeTwinCATClient
+    {
+        public override Task<(object? value, uint status)> ReadValueAsync(
+            string symbolPath, TwinCATDataType type, int? bitIndex, int? arrayCount, CancellationToken ct)
+        {
+            if (BlockRead is not null) return base.ReadValueAsync(symbolPath, type, bitIndex, arrayCount, ct);
+            Values[symbolPath] = next();
+            return base.ReadValueAsync(symbolPath, type, bitIndex, arrayCount, ct);
+        }
+    }
+
+    // ---- M1 ----
+
+    /// <summary>
+    ///     A write-only device with the probe off: a wire-failed write degrades; a later Good write is the device
+    ///     answering — Healthy again with the stamp advanced. A refused write (BadNotWritable) changes nothing.
+    /// </summary>
+    [Fact]
+    public async Task A_good_write_clears_that_devices_comms_fault()
+    {
+        var fake = new FakeTwinCATClient();
+        var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake },
+            new TwinCATProbeOptions { Enabled = false }, native: false,
+            new TwinCATTagDefinition("W", Host, "MAIN.W", TwinCATDataType.DInt));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        var initStamp = drv.GetHealth().LastSuccessfulRead!.Value;
+
+        fake.ThrowOnWrite = true;
+        _ = await drv.WriteAsync([new WriteRequest("W", 1)], TestContext.Current.CancellationToken);
+        drv.GetHealth().State.ShouldBe(DriverState.Degraded);
+
+        fake.ThrowOnWrite = false;
+        fake.WriteStatuses["MAIN.W"] = TwinCATStatusMapper.BadNotWritable;
+        _ = await drv.WriteAsync([new WriteRequest("W", 1)], TestContext.Current.CancellationToken);
+        drv.GetHealth().State.ShouldBe(DriverState.Degraded, "a refusal says nothing about the comms fault");
+
+        await WaitForAsync(() => DateTime.UtcNow > initStamp, TimeSpan.FromSeconds(1));
+        fake.WriteStatuses.Remove("MAIN.W");
+        _ = await drv.WriteAsync([new WriteRequest("W", 1)], TestContext.Current.CancellationToken);
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+        drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThan(initStamp);
+    }
+
+    // ---- M2 + M3 ----
+
+    /// <summary>
+    ///     A symbol-version re-arm that lands while a retry of the refused registration is in flight is not
+    ///     overwritten by that retry's failure: the next tick re-attempts at once. (Without the epoch the
+    ///     failure re-imposed a 30 s+ backoff.)
+    /// </summary>
+    [Fact]
+    public async Task A_rearm_during_an_in_flight_retry_is_not_lost()
+    {
+        var fake = new FakeTwinCATClient();
+        fake.AddNotificationErrors["MAIN.T"] = new TwinCATTypeMismatchException("MAIN.T", "PLC STRING");
+        var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, FastProbe, native: true,
+            new TwinCATTagDefinition("T", Host, "MAIN.T", TwinCATDataType.DInt));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        _ = await drv.SubscribeAsync(["T"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken);
+        fake.AttemptsFor("MAIN.T").ShouldBe(1);
+
+        var inFlight = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        fake.PreAttemptGate = inFlight;
+        fake.FireSymbolVersionChanged(); // re-arm: the next tick starts a retry, which wedges on the gate
+        await WaitForAsync(() => fake.AttemptsFor("MAIN.T") >= 2, TimeSpan.FromSeconds(3));
+        fake.FireSymbolVersionChanged(); // a second program change while that retry is in flight
+        fake.PreAttemptGate = null;
+        inFlight.SetResult();           // the in-flight retry now fails (still refused)
+
+        await WaitForAsync(() => fake.AttemptsFor("MAIN.T") >= 3, TimeSpan.FromSeconds(3));
+        fake.AttemptsFor("MAIN.T").ShouldBeGreaterThanOrEqualTo(3);
+        await drv.ShutdownAsync(TestContext.Current.CancellationToken);
+    }
+
+    /// <summary>A re-arm restarts the refusal backoff from the initial delay instead of doubling the old one.</summary>
+    [Fact]
+    public async Task A_rearm_restarts_the_refusal_backoff()
+    {
+        var fake = new FakeTwinCATClient();
+        fake.AddNotificationErrors["MAIN.T"] = new TwinCATTypeMismatchException("MAIN.T", "PLC STRING");
+        var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake }, FastProbe, native: true,
+            new TwinCATTagDefinition("T", Host, "MAIN.T", TwinCATDataType.DInt));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+        _ = await drv.SubscribeAsync(["T"], TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken);
+        var reg = drv.GetDeviceState(Host)!.NativeRegistrations.Values.Single();
+        reg.RefusalBackoff.ShouldBe(TwinCATDriver.RefusedRetryInitial);
+
+        fake.FireSymbolVersionChanged();
+        await WaitForAsync(() => fake.AttemptsFor("MAIN.T") >= 2, TimeSpan.FromSeconds(3));
+        await WaitForAsync(() => reg.RefusalBackoff != TimeSpan.Zero, TimeSpan.FromSeconds(1));
+        reg.RefusalBackoff.ShouldBe(TwinCATDriver.RefusedRetryInitial);
+        await drv.ShutdownAsync(TestContext.Current.CancellationToken);
+    }
+
+    // ---- M5 ----
+
+    /// <summary>A WSTRING tag over a WSTRING symbol writes UTF-16LE with an aligned 2-byte terminator, zero-padded.</summary>
+    [Fact]
+    public async Task A_WSTRING_write_goes_through_the_client_encode_plan()
+    {
+        var fake = new FakeTwinCATClient();
+        fake.SymbolShapes["MAIN.W"] = TwinCATPlcSymbolShape.Classify("WSTRING(3)", DataTypeCategory.String, 8);
+        var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake },
+            new TwinCATProbeOptions { Enabled = false }, native: false,
+            new TwinCATTagDefinition("W", Host, "MAIN.W", TwinCATDataType.WString));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+
+        var results = await drv.WriteAsync(
+            [new WriteRequest("W", "中x"), new WriteRequest("W", "abcd")], TestContext.Current.CancellationToken);
+
+        results[0].StatusCode.ShouldBe(TwinCATStatusMapper.Good);
+        results[1].StatusCode.ShouldBe(TwinCATStatusMapper.BadOutOfRange); // 4 chars do not fit WSTRING(3)
+        fake.RawWrites["MAIN.W"].ShouldBe(new byte[] { 0x2D, 0x4E, (byte)'x', 0, 0, 0, 0, 0 });
+    }
+
+    /// <summary>A multi-dimensional array is written flattened in PLC memory order (last index fastest).</summary>
+    [Fact]
+    public async Task A_multi_dimensional_array_write_is_flattened_row_major()
+    {
+        var fake = new FakeTwinCATClient();
+        var element = TwinCATPlcSymbolShape.Classify("INT", DataTypeCategory.Primitive, 2);
+        fake.SymbolShapes["MAIN.M"] = TwinCATPlcSymbolShape.Classify("ARRAY [0..1,0..2] OF INT", DataTypeCategory.Array, 12);
+        fake.ArrayShapes["MAIN.M"] = new TwinCATPlcArrayShape([2, 3], 12, element);
+        var drv = NewDriver(new FakeTwinCATClientFactory { Customise = () => fake },
+            new TwinCATProbeOptions { Enabled = false }, native: false,
+            new TwinCATTagDefinition("M", Host, "MAIN.M", TwinCATDataType.Int, ArrayLength: 6));
+        await drv.InitializeAsync("{}", TestContext.Current.CancellationToken);
+
+        var result = await drv.WriteAsync([new WriteRequest("M", new[] { 1, 2, 3, 4, 5, 6 })], TestContext.Current.CancellationToken);
+
+        result.Single().StatusCode.ShouldBe(TwinCATStatusMapper.Good);
+        fake.RawWrites["MAIN.M"].ShouldBe(new byte[] { 1, 0, 2, 0, 3, 0, 4, 0, 5, 0, 6, 0 });
+        TwinCATArrayCodec.Decode(TwinCATDataType.Int, fake.RawWrites["MAIN.M"], new TwinCATArrayLayout(6, 2), false)
+            .ShouldBe(new short[] { 1, 2, 3, 4, 5, 6 });
+    }
+
+    // ---- M6 ----
+
+    /// <summary>An infinity spelled as a string passes like a float/double infinity; a finite overflow does not.</summary>
+    [Fact]
+    public void Infinity_strings_are_explicit_infinities()
+    {
+        AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, "Infinity").ShouldBe(float.PositiveInfinity);
+        AdsTwinCATClient.ConvertForWrite(TwinCATDataType.LReal, "-Infinity").ShouldBe(double.NegativeInfinity);
+        Should.Throw<OverflowException>(() => AdsTwinCATClient.ConvertForWrite(TwinCATDataType.Real, "1e300"));
+    }
+}