fix(ablegacy): deterministic NotSupported refusals are BadNotSupported with no health write

Review 07 H16: an array of String/Timer/Counter/Control passed the mapper, so every poll created a handle (one init read), the decode threw NotSupportedException, the generic catch evicted the handle and latched Degraded. FromTagConfig now rejects that combination (the skip carries the reason, Inspect/deploy gate and the AdminUI model state it), and a runtime NotSupportedException on read or write is BadNotSupported: handle kept, no health write, one Warning per tag and operation. Write refusals (native PLC-5 and status-bit writes, libplctag translated addresses) now warn once per tag too. Refs #738 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
2026-10-22 14:20:17 -04:00
parent 7377daa9b0
commit 8b50b43048
5 changed files with 215 additions and 5 deletions
@@ -77,6 +77,10 @@ public static class AbLegacyTagDefinitionFactory
                if (rawLength >= 1)
                    arrayLength = Math.Min(rawLength, AbLegacyArray.MaxElements);
            }
+            // A deterministic refusal (review 07 H16): String / Timer / Counter / Control elements are structured
+            // or variable-width and do not lay out as a flat array, so both runtimes refuse to decode them. Accepting
+            // the tag only bought a handle create + a NotSupportedException on every poll.
+            if (arrayLength is not null && !IsArrayElementTypeSupported(dataType)) return false;
            // "writable" defaults to true when absent (today's value) — makes read-only equipment tags
            // authorable.
            var writable = TagConfigJson.ReadWritable(root);
@@ -224,8 +228,30 @@ public static class AbLegacyTagDefinitionFactory
        }

        if (TagConfigJson.DescribeInvalidEnum<AbLegacyDataType>(root, "dataType") is { } w) warnings.Add(w);
+
+        if (IsArrayFlag(root) && ReadInt(root, "arrayLength") >= 1
+            && TagConfigJson.TryReadEnumStrict(root, "dataType", AbLegacyDataType.Int, out var dt)
+            && !IsArrayElementTypeSupported(dt))
+            warnings.Add(ArrayElementTypeError(dt));
    }

+    /// <summary>
+    ///     Whether <paramref name="type"/> can be the element type of an array tag. Only the flat word/long/float/bit
+    ///     files lay out as a contiguous array; String, Timer, Counter and Control elements are structured.
+    /// </summary>
+    /// <param name="type">The authored element data type.</param>
+    /// <returns><see langword="true"/> for Bit, Int, AnalogInt, Long and Float.</returns>
+    public static bool IsArrayElementTypeSupported(AbLegacyDataType type) => type is
+        AbLegacyDataType.Bit or AbLegacyDataType.Int or AbLegacyDataType.AnalogInt
+        or AbLegacyDataType.Long or AbLegacyDataType.Float;
+
+    /// <summary>The authoring / deploy-gate message for an array of an unsupported element type.</summary>
+    /// <param name="type">The authored element data type.</param>
+    /// <returns>The message.</returns>
+    public static string ArrayElementTypeError(AbLegacyDataType type) =>
+        $"AbLegacy {type} is not supported as an array element (String, Timer, Counter and Control elements are "
+        + "structured); author a scalar tag per element instead. The driver skips this tag and it reads BadNodeIdUnknown.";
+
    private static int ReadInt(JsonElement o, string name)
        => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.Number
           && e.TryGetInt32(out var v) ? v : 0;
@@ -221,10 +221,13 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            {
                if (!AbLegacyTagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def))
                {
-                    _skippedTags.Skip(entry.RawPath, "TagConfig did not map to a definition");
+                    // The deploy gate's own explanation (same authority) — e.g. an array of a structured type.
+                    var reason = AbLegacyTagDefinitionFactory.Inspect(entry.TagConfig).FirstOrDefault()
+                                 ?? "TagConfig did not map to a definition";
+                    _skippedTags.Skip(entry.RawPath, reason);
                    _logger.LogWarning(
-                        "AbLegacy tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}",
-                        _driverInstanceId, entry.RawPath);
+                        "AbLegacy tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath} Reason={Reason}",
+                        _driverInstanceId, entry.RawPath, reason);
                    continue;
                }
                tags[entry.RawPath] = def with
@@ -523,6 +526,14 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            {
                // The runtime refused the operation before the wire (e.g. a native PLC-5 non-N read).
                // Not a comms fault: BadNotSupported, keep the handle, leave health alone.
+                WarnRefusalOnce(device, def, "read", ex.Message);
+                results[i] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now);
+            }
+            catch (NotSupportedException nse)
+            {
+                // A deterministic decode refusal (review 07 H16) — the same answer every poll. BadNotSupported, keep
+                // the handle (re-creating it only re-paid the init read), warn once, and NO health write.
+                WarnRefusalOnce(device, def, "read", nse.Message);
                results[i] = new DataValueSnapshot(null, AbLegacyStatusMapper.BadNotSupported, null, now);
            }
            catch (Exception ex)
@@ -601,8 +612,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
                // READ, and against a device lacking that file the read failure surfaced as a comms fault —
                // BadCommunicationError, evicted handle, Degraded health — instead of the refusal.
                // Ahead of the bit-RMW branch too, which would otherwise read the parent word first.
-                if (parsed is not null && _tagFactory.WriteRefusal(CreateParamsFor(device, def, parsed)) is not null)
+                if (parsed is not null && _tagFactory.WriteRefusal(CreateParamsFor(device, def, parsed)) is { } refusal)
                {
+                    WarnRefusalOnce(device, def, "write", refusal);
                    results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported);
                    continue;
                }
@@ -654,8 +666,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            catch (OperationCanceledException) { throw; }
            catch (NotSupportedException nse)
            {
+                // Deterministic (review 07 H16): nothing reached the wire, so it says nothing about the device.
+                WarnRefusalOnce(device, def, "write", nse.Message);
                results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported);
-                SetHealthIfCurrent(epoch, new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message));
            }
            catch (Exception ex) when (ex is FormatException or InvalidCastException)
            {
@@ -669,6 +682,7 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            {
                // The runtime refused the write before the wire — a native status-bit sub-element write or a
                // PLC-5 write. Not a comms fault: BadNotSupported, keep the handle, leave health alone.
+                WarnRefusalOnce(device, def, "write", ex.Message);
                results[i] = new WriteResult(AbLegacyStatusMapper.BadNotSupported);
            }
            catch (Exception ex)
@@ -1157,6 +1171,18 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
            _driverInstanceId, def.DeviceHostAddress, def.Name, def.Address, device.Options.PlcFamily);
    }

+    /// <summary>
+    ///     A deterministic refusal (BadNotSupported) is answered the same way every time, so it is logged ONCE per
+    ///     tag and operation — a polled tag must not flood the log, and the refusal never touches driver health.
+    /// </summary>
+    private void WarnRefusalOnce(DeviceState device, AbLegacyTagDefinition def, string operation, string reason)
+    {
+        if (!device.RefusalLogged.TryAdd($"{operation}:{def.Name}", 0)) return;
+        _logger.LogWarning(
+            "AbLegacy {Operation} refused (BadNotSupported). Driver={DriverInstanceId} Device={DeviceHostAddress} Reference={Reference} Address={Address} Reason={Reason}",
+            operation, _driverInstanceId, def.DeviceHostAddress, def.Name, def.Address, reason);
+    }
+
    /// <summary>The create parameters for a tag's own runtime — shared by creation and the write-refusal check, so
    /// the refusal is asked about exactly the runtime a write would otherwise create.</summary>
    private AbLegacyTagCreateParams CreateParamsFor(DeviceState device, AbLegacyTagDefinition def, AbLegacyAddress parsed) =>
@@ -1387,6 +1413,9 @@ public sealed class AbLegacyDriver : IDriver, IReadable, IWritable, ITagDiscover
        /// </summary>
        public bool FirstNonZeroStatusLogged { get; set; }

+        /// <summary>Tag operations already warned about for a deterministic refusal (once-per-tag-and-operation log).</summary>
+        public System.Collections.Concurrent.ConcurrentDictionary<string, byte> RefusalLogged { get; } = new(StringComparer.Ordinal);
+
        /// <summary>Tags already warned about for a malformed address (once-per-tag log).</summary>
        public System.Collections.Concurrent.ConcurrentDictionary<string, byte> MalformedAddressLogged { get; } = new();

@@ -87,6 +87,11 @@ public sealed class AbLegacyTagConfigModel
        if (isArray && arrayLength > AbLegacyArray.MaxElements)
            return $"AbLegacy array length cannot exceed {AbLegacyArray.MaxElements} (PCCC data-file element limit).";

+        // Same rule the reader enforces (review 07 H16): structured elements never form an array. Stated here so
+        // the CSV-import / API paths get the specific reason instead of the parse gate's generic one.
+        if (isArray && arrayLength >= 1 && !AbLegacyTagDefinitionFactory.IsArrayElementTypeSupported(DataType))
+            return AbLegacyTagDefinitionFactory.ArrayElementTypeError(DataType);
+
        // Parse-gate backstop: the checks above mirror the reader's whole rejection surface today, so this is
        // expected to be silent for AbLegacy. It is here so a future reader rule cannot become a gap.
        return TagConfigParseGate.Describe(
@@ -0,0 +1,136 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests;
+
+/// <summary>
+///     Review 07 H16: an array of String/Timer/Counter/Control was accepted by the mapper, so every poll created a
+///     handle (one init read), threw <see cref="NotSupportedException"/> from the decode, disposed the handle and
+///     latched Degraded. A deterministic refusal is now rejected at the mapper; a runtime
+///     <see cref="NotSupportedException"/> that still occurs is per-tag <c>BadNotSupported</c>, keeps its handle,
+///     warns once and leaves driver health alone.
+/// </summary>
+[Trait("Category", "Unit")]
+public sealed class AbLegacyDeterministicRefusalTests
+{
+    private const string Host = "ab://10.0.0.5/1,0";
+
+    [Theory]
+    [InlineData("String", "ST9:0")]
+    [InlineData("TimerElement", "T4:0")]
+    [InlineData("CounterElement", "C5:0")]
+    [InlineData("ControlElement", "R6:0")]
+    public void Mapper_rejects_arrays_of_structured_element_types(string dataType, string address)
+    {
+        var json = $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}""";
+
+        AbLegacyTagDefinitionFactory.FromTagConfig(json, "p", out _).ShouldBeFalse();
+        AbLegacyTagDefinitionFactory.Inspect(json).ShouldContain(w => w.Contains("not supported as an array"));
+        // Positive control: the same type as a SCALAR is still accepted.
+        AbLegacyTagDefinitionFactory.FromTagConfig(
+            $$"""{"address":"{{address}}","dataType":"{{dataType}}"}""", "p", out _).ShouldBeTrue();
+    }
+
+    [Theory]
+    [InlineData("Int", "N7:0")]
+    [InlineData("Float", "F8:0")]
+    [InlineData("Long", "L9:0")]
+    [InlineData("Bit", "B3:0")]
+    public void Mapper_keeps_arrays_of_flat_element_types(string dataType, string address)
+    {
+        var json = $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}""";
+        AbLegacyTagDefinitionFactory.FromTagConfig(json, "p", out var def).ShouldBeTrue();
+        def.ArrayLength.ShouldBe(4);
+        AbLegacyTagDefinitionFactory.Inspect(json).ShouldBeEmpty();
+    }
+
+    [Fact]
+    public async Task Rejected_array_tag_is_skipped_with_its_reason_and_creates_no_runtime()
+    {
+        var factory = new FakeAbLegacyTagFactory { Customise = p => new FakeAbLegacyTag(p) { Value = 1 } };
+        var drv = new AbLegacyDriver(new AbLegacyDriverOptions
+        {
+            Devices = [new AbLegacyDeviceOptions(Host)],
+            RawTags =
+            [
+                new RawTagEntry("Strs", """{"address":"ST9:0","dataType":"String","isArray":true,"arrayLength":3}""", false, Host),
+                new RawTagEntry("Good", """{"address":"N7:0","dataType":"Int"}""", false, Host),
+            ],
+            Probe = new AbLegacyProbeOptions { Enabled = false },
+        }, "drv-h16", factory);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+
+        var r = await drv.ReadAsync(["Strs", "Good"], CancellationToken.None);
+
+        r[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNodeIdUnknown);
+        r[1].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); // positive control
+        factory.Tags.Keys.ShouldNotContain("ST9:0");
+        var skipped = drv.GetSkippedTags().ShouldNotBeNull();
+        skipped.Sample.ShouldContain(s => s.RawPath == "Strs" && s.Reason.Contains("not supported as an array"));
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+        await drv.ShutdownAsync(CancellationToken.None);
+    }
+
+    private sealed class RefusingDecodeTag(AbLegacyTagCreateParams p) : FakeAbLegacyTag(p)
+    {
+        public override object? DecodeArray(AbLegacyDataType type, int count) =>
+            throw new NotSupportedException("decode refused");
+
+        public override void EncodeValue(AbLegacyDataType type, int? bitIndex, object? value) =>
+            throw new NotSupportedException("encode refused");
+    }
+
+    [Fact]
+    public async Task Runtime_NotSupported_on_read_is_BadNotSupported_keeps_handle_no_health_warns_once()
+    {
+        var logger = new AbLegacyTestLogger();
+        var factory = new FakeAbLegacyTagFactory
+        {
+            Customise = p => p.TagName == "N7:0" ? new RefusingDecodeTag(p) : new FakeAbLegacyTag(p) { Value = 7 },
+        };
+        var drv = new AbLegacyDriver(new AbLegacyDriverOptions
+        {
+            Devices = [new AbLegacyDeviceOptions(Host)],
+            RawTags = AbLegacyRawTags.Entries(
+                new AbLegacyTagDefinition("Arr", Host, "N7:0", AbLegacyDataType.Int, ArrayLength: 4),
+                new AbLegacyTagDefinition("Good", Host, "N9:0", AbLegacyDataType.Int)),
+            Probe = new AbLegacyProbeOptions { Enabled = false },
+        }, "drv-h16", factory, logger);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+
+        var first = await drv.ReadAsync(["Arr", "Good"], CancellationToken.None);
+        var second = await drv.ReadAsync(["Arr", "Good"], CancellationToken.None);
+
+        first[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported);
+        second[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported);
+        first[1].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); // positive control: the refusal is per tag
+        var tag = factory.Tags["N7:0"];
+        tag.InitializeCount.ShouldBe(1); // handle kept, not churned
+        tag.Disposed.ShouldBeFalse();
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+        logger.Warnings.Count(w => w.Contains("Arr") && w.Contains("decode refused")).ShouldBe(1);
+        await drv.ShutdownAsync(CancellationToken.None);
+    }
+
+    [Fact]
+    public async Task Runtime_NotSupported_on_write_is_BadNotSupported_with_no_health_write()
+    {
+        var factory = new FakeAbLegacyTagFactory { Customise = p => new RefusingDecodeTag(p) { Value = 1 } };
+        var drv = new AbLegacyDriver(new AbLegacyDriverOptions
+        {
+            Devices = [new AbLegacyDeviceOptions(Host)],
+            RawTags = AbLegacyRawTags.Entries(new AbLegacyTagDefinition("X", Host, "N7:0", AbLegacyDataType.Int)),
+            Probe = new AbLegacyProbeOptions { Enabled = false },
+        }, "drv-h16", factory);
+        await drv.InitializeAsync("{}", CancellationToken.None);
+        (await drv.ReadAsync(["X"], CancellationToken.None))[0].StatusCode.ShouldBe(AbLegacyStatusMapper.Good);
+
+        var w = await drv.WriteAsync([new WriteRequest("X", 5)], CancellationToken.None);
+
+        w[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadNotSupported);
+        drv.GetHealth().State.ShouldBe(DriverState.Healthy);
+        factory.Tags["N7:0"].Disposed.ShouldBeFalse();
+        await drv.ShutdownAsync(CancellationToken.None);
+    }
+}
@@ -122,6 +122,20 @@ public sealed class AbLegacyTagConfigModelTests
        m.Validate().ShouldBeNull();
    }

+    /// <summary>Review 07 H16: an array of a structured element type is refused with the reader's own reason.</summary>
+    [Theory]
+    [InlineData("String", "ST9:0")]
+    [InlineData("TimerElement", "T4:0")]
+    public void Validate_refuses_an_array_of_a_structured_element_type(string dataType, string address)
+    {
+        var m = AbLegacyTagConfigModel.FromJson(
+            $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}""");
+
+        m.Validate().ShouldNotBeNull().ShouldContain("not supported as an array");
+        // Positive control: the same type as a scalar validates.
+        AbLegacyTagConfigModel.FromJson($$"""{"address":"{{address}}","dataType":"{{dataType}}"}""").Validate().ShouldBeNull();
+    }
+
    [Fact]
    public void Validate_returns_null_for_scalar_tag_regardless_of_no_array_length()
    {