From 895cc28886993feb421268f792d7c85948dc3fea Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:24:06 -0400 Subject: [PATCH] fix(abcip): write-side health neither latches nor overwrites H19: Format/InvalidCast/Overflow on a client value never touched the wire but latched Degraded; a good write set Healthy and so undid a Degraded the poll loop had just recorded for other tags (the batch read path is worst-wins for exactly this reason). - The three conversion failures map to BadTypeMismatch/BadOutOfRange with a once-per-tag Warning and no health write (AbLegacy's rule). - A good write (scalar or bit RMW) advances LastSuccessfulRead only - the device answered - and leaves the state alone. - A write comms fault (thrown, or a non-refusal status word, which used to write no health at all) is Degraded with LastSuccessfulRead preserved and never downgrades Faulted. Refs #737 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbCipDriver.cs | 45 ++++--- .../AbCipWriteHealthTests.cs | 113 ++++++++++++++++++ 2 files changed, 142 insertions(+), 16 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 89f06793..f675234e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -1390,7 +1390,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, await WriteBitInDIntAsync(device, parsedPath, bit, w.Value, cancellationToken) .ConfigureAwait(false)); if (results[i].StatusCode == AbCipStatusMapper.Good) - _health = new DriverHealth(DriverState.Healthy, now, null); + RecordWriteAnswered(now); continue; } @@ -1455,6 +1455,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, { EvictRuntime(device, def.Name); results[i] = new WriteResult(AbCipStatusMapper.MapStatus(status)); + RecordWriteCommsFault($"transport status {status} writing {w.FullReference}"); _logger.LogWarning( "AbCip write returned non-zero transport status {Status} for tag {Tag} on device {Device}; " + "evicting cached runtime so next call re-creates it", @@ -1463,7 +1464,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, else { results[i] = new WriteResult(AbCipStatusMapper.Good); - _health = new DriverHealth(DriverState.Healthy, now, null); + RecordWriteAnswered(now); } } catch (OperationCanceledException) @@ -1481,30 +1482,29 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, "AbCip write not supported for tag {Tag} on device {Device}: {Reason} (logged once per tag)", w.FullReference, def.DeviceHostAddress, nse.Message); } + // H19: the three client-value conversion failures below never touched the wire — they say nothing about + // the device, so they write no health (a Degraded here latched until the next good read, and AbLegacy + // already maps the same three with no health write). Not a transport fault either: no evict. catch (FormatException fe) { - // Value conversion error — not a transport fault; don't evict. results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, fe.Message); - _logger.LogWarning(fe, - "AbCip write value-conversion error for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + WarnWriteRefusedOnce(def.Name, + "AbCip write to {Tag} on device {Device}: the value cannot be converted to {DataType}: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, def.DataType, fe.Message); } catch (InvalidCastException ice) { results[i] = new WriteResult(AbCipStatusMapper.BadTypeMismatch); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ice.Message); - _logger.LogWarning(ice, - "AbCip write type-cast error for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + WarnWriteRefusedOnce(def.Name, + "AbCip write to {Tag} on device {Device}: the value cannot be converted to {DataType}: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, def.DataType, ice.Message); } catch (OverflowException oe) { results[i] = new WriteResult(AbCipStatusMapper.BadOutOfRange); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, oe.Message); - _logger.LogWarning(oe, - "AbCip write value out of range for tag {Tag} on device {Device}", - w.FullReference, def.DeviceHostAddress); + WarnWriteRefusedOnce(def.Name, + "AbCip write to {Tag} on device {Device}: the value is out of range for {DataType}: {Reason} (logged once per tag)", + w.FullReference, def.DeviceHostAddress, def.DataType, oe.Message); } catch (AbTransportException ate) when (ate.Status == AbStatus.Unsupported) { @@ -1549,7 +1549,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, if (rmwParent is not null) EvictParentRuntime(device, rmwParent); else EvictRuntime(device, def.Name); results[i] = new WriteResult(AbCipStatusMapper.BadCommunicationError); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + RecordWriteCommsFault(ex.Message); _logger.LogWarning(ex, "AbCip write transport exception for tag {Tag} on device {Device}", w.FullReference, def.DeviceHostAddress); @@ -1559,6 +1559,19 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, return results; } + /// + /// A good write proves the device answered: advance LastSuccessfulRead but leave the STATE alone. Writing + /// Healthy here (H19) overwrote a Degraded the poll loop had just recorded for other tags. + /// + private void RecordWriteAnswered(DateTime now) => + UpdateHealth(h => h with { LastSuccessfulRead = now }); + + /// A write-side comms fault: Degraded (never downgrading Faulted), LastSuccessfulRead preserved. + private void RecordWriteCommsFault(string error) => + UpdateHealth(h => h.State == DriverState.Faulted + ? h + : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, error)); + /// /// Read-modify-write one bit within an integer parent (SINT / INT / DINT / LINT — Logix allows .n on /// each). Creates / reuses a parallel parent runtime (distinct from the bit-selector handle) + serialises diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs new file mode 100644 index 00000000..4821a43a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipWriteHealthTests.cs @@ -0,0 +1,113 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Review 07 H19: write-side health. A client value that cannot be converted never touched the wire, so it must +/// not latch Degraded; and a good write must not overwrite a Degraded the read path just recorded +/// with Healthy (the batch read path is worst-wins for exactly this reason). A good write proves the device +/// answered, so it advances LastSuccessfulRead without touching the state. +/// +[Trait("Category", "Unit")] +public sealed class AbCipWriteHealthTests +{ + private const string Device = "ab://10.0.0.5/1,0"; + + private sealed class ThrowingWriteTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public override Task WriteAsync(CancellationToken cancellationToken) => + throw new InvalidOperationException("socket reset"); + } + + private static async Task<(AbCipDriver Driver, FakeAbCipTagFactory Factory)> BuildAsync( + Func? make = null) + { + var factory = new FakeAbCipTagFactory + { + Customise = make ?? (p => p.TagName == "Broken" + ? new FakeAbCipTag(p) { ThrowOnRead = true } + : new RealEncodingTag(p) { Value = 0 }), + }; + var drv = new AbCipDriver(new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions(Device)], + RawTags = AbCipRawTags.From( + new AbCipTagDefinition("D", Device, "D", AbCipDataType.DInt), + new AbCipTagDefinition("I", Device, "I", AbCipDataType.Int), + new AbCipTagDefinition("Broken", Device, "Broken", AbCipDataType.DInt)), + Probe = new AbCipProbeOptions { Enabled = false }, + }, "abcip-write-health", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + return (drv, factory); + } + + /// A fake whose encode converts like the real runtimes do (Convert.ToInt32 / ToInt16). + private sealed class RealEncodingTag(AbCipTagCreateParams p) : FakeAbCipTag(p) + { + public override void EncodeValue(AbCipDataType type, int? bitIndex, object? value) => + Value = type switch + { + AbCipDataType.Int => Convert.ToInt16(value), + _ => Convert.ToInt32(value), + }; + } + + /// Unconvertible / out-of-range client values: refused per tag, health untouched, nothing sent. + [Fact] + public async Task A_client_conversion_error_writes_no_health() + { + var (drv, factory) = await BuildAsync(); + var before = drv.GetHealth(); + + var r = await drv.WriteAsync( + [ + new WriteRequest("D", "not a number"), // FormatException + new WriteRequest("D", new object()), // InvalidCastException + new WriteRequest("I", 1_000_000), // OverflowException + ], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + r[1].StatusCode.ShouldBe(AbCipStatusMapper.BadTypeMismatch); + r[2].StatusCode.ShouldBe(AbCipStatusMapper.BadOutOfRange); + drv.GetHealth().ShouldBe(before); + factory.Tags["D"].WriteCount.ShouldBe(0); + factory.Tags["I"].WriteCount.ShouldBe(0); + } + + /// A good write keeps a Degraded the read path recorded, and advances LastSuccessfulRead. + [Fact] + public async Task A_good_write_does_not_overwrite_Degraded_with_Healthy() + { + var (drv, _) = await BuildAsync(); + await drv.ReadAsync(["Broken"], CancellationToken.None); + var degraded = drv.GetHealth(); + degraded.State.ShouldBe(DriverState.Degraded); + await Task.Delay(20); + + var r = await drv.WriteAsync([new WriteRequest("D", 5)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.Good); + var after = drv.GetHealth(); + after.State.ShouldBe(DriverState.Degraded); + after.LastError.ShouldBe(degraded.LastError); + after.LastSuccessfulRead!.Value.ShouldBeGreaterThan(degraded.LastSuccessfulRead!.Value); // the device answered + } + + /// Positive control: a genuine write transport fault still degrades and preserves LastSuccessfulRead. + [Fact] + public async Task A_write_transport_fault_still_degrades() + { + var (drv, factory) = await BuildAsync(p => new ThrowingWriteTag(p)); + var before = drv.GetHealth(); + + var r = await drv.WriteAsync([new WriteRequest("D", 5)], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbCipStatusMapper.BadCommunicationError); + var after = drv.GetHealth(); + after.State.ShouldBe(DriverState.Degraded); + after.LastSuccessfulRead.ShouldBe(before.LastSuccessfulRead); + factory.Tags["D"].Disposed.ShouldBeTrue(); + } +}