fix(twincat): review — a connect failure no longer burns the sum-chunk warning; doc precision

- ReadBucketAsync: the once-per-device "sum-read chunk failed" Warning fires only after a successful connect; a connect/backoff failure is the connect path's to report. TwinCATSumChunkWarningTests pins it (no warning across connect failures, exactly one across two later failing sum polls; red without the guard) plus a source guard that ReadGroupAsync builds its SumRead only via CreateSumRead. - Docs: diagnostics live-gated via the driver harness (/hosts not exercised); TC2 costs 8 reads on the first tick and after each reconnect, then <= 4; read counts attributed to the unit tests. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 21:26:26 -04:00
parent 664ff75ee2
commit a92fb81f00
4 changed files with 118 additions and 9 deletions
@@ -0,0 +1,99 @@
+using System.Text.RegularExpressions;
+using Microsoft.Extensions.Logging;
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests;
+
+/// <summary>
+///     hw2 review of the live-gate fix: the once-per-device "sum-read chunk failed" Warning exists to surface a
+///     sum-read path that fails on every poll (the gate's silent failure). A device that is merely unreachable must
+///     not consume it — the connect path reports that itself — or the later real defect is logged nowhere.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class TwinCATSumChunkWarningTests
+{
+    private const string Host = "ads://5.23.91.23.1.1:851";
+    private const string ChunkWarning = "an ADS sum-read chunk";
+
+    [Fact]
+    public async Task A_connect_failure_does_not_burn_the_warning_and_a_later_real_chunk_failure_warns_once()
+    {
+        var connectFails = true;
+        var factory = new FakeTwinCATClientFactory
+        {
+            Customise = () => new FakeTwinCATClient
+            {
+                ThrowOnConnect = connectFails,
+                Exception = new InvalidOperationException("device down"),
+                SumReadException = new InvalidOperationException("sum path broken"),
+            },
+        };
+        var logger = new CapturingLogger();
+        var drv = new TwinCATDriver(new TwinCATDriverOptions
+        {
+            Devices = [new TwinCATDeviceOptions(Host)],
+            RawTags = TwinCATRawTags.Entries([
+                new TwinCATTagDefinition("A", Host, "MAIN.A", TwinCATDataType.DInt),
+                new TwinCATTagDefinition("B", Host, "MAIN.B", TwinCATDataType.DInt),
+            ]),
+            Probe = new TwinCATProbeOptions { Enabled = false },
+            UseSumRead = true,
+            SumReadMaxItems = 10,
+        }, "drv-warn", factory, logger);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+
+        // Unreachable: the connect attempt fails, then the backoff window rejects further attempts.
+        for (var i = 0; i < 3; i++)
+            (await drv.ReadAsync(["A", "B"], CancellationToken.None))
+                .ShouldAllBe(s => s.StatusCode == TwinCATStatusMapper.BadCommunicationError);
+        logger.Warnings.Count(w => w.Contains(ChunkWarning)).ShouldBe(0);
+
+        // Device comes back, but the sum path itself fails: wait out the 1 s connect backoff, then two polls.
+        connectFails = false;
+        var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(5);
+        while (!factory.Clients.Any(c => c.IsConnected) && DateTime.UtcNow < deadline)
+        {
+            await drv.ReadAsync(["A", "B"], CancellationToken.None);
+            await Task.Delay(50);
+        }
+        factory.Clients.Any(c => c.IsConnected).ShouldBeTrue("the device must reconnect for the second half to mean anything");
+        await drv.ReadAsync(["A", "B"], CancellationToken.None);
+        await drv.ReadAsync(["A", "B"], CancellationToken.None);
+
+        logger.Warnings.Count(w => w.Contains(ChunkWarning)).ShouldBe(1);
+    }
+
+    /// <summary>Source guard: the driver's only Beckhoff <c>SumRead</c> construction is inside
+    ///     <c>CreateSumRead</c>, so re-inlining a <c>List</c>-based construction in <c>ReadGroupAsync</c> goes red.</summary>
+    [Fact]
+    public void ReadGroupAsync_builds_its_sum_command_through_CreateSumRead_only()
+    {
+        var dir = new DirectoryInfo(AppContext.BaseDirectory);
+        while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) dir = dir.Parent;
+        dir.ShouldNotBeNull("repo root not found");
+        var src = File.ReadAllText(Path.Combine(dir!.FullName,
+            "src", "Drivers", "ZB.MOM.WW.OtOpcUa.Driver.TwinCAT", "AdsTwinCATClient.cs"));
+
+        Regex.Matches(src, @"new\s+SumRead\s*\(").Count.ShouldBe(1);
+        var create = src.IndexOf("internal static SumRead CreateSumRead(", StringComparison.Ordinal);
+        create.ShouldBeGreaterThan(0);
+        var ctor = Regex.Match(src, @"new\s+SumRead\s*\(").Index;
+        ctor.ShouldBeGreaterThan(create);
+        src.IndexOf("private const uint SymbolValueByHandle", StringComparison.Ordinal).ShouldBeGreaterThan(ctor);
+        Regex.IsMatch(src, @"await\s+CreateSumRead\(_client").ShouldBeTrue();
+    }
+
+    private sealed class CapturingLogger : ILogger<TwinCATDriver>
+    {
+        public List<string> Warnings { get; } = [];
+        public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
+        public bool IsEnabled(LogLevel logLevel) => true;
+        public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception? exception,
+            Func<TState, Exception?, string> formatter)
+        {
+            if (logLevel == LogLevel.Warning) lock (Warnings) Warnings.Add(formatter(state, exception));
+        }
+    }
+}