Merge #570/#579: native AbCip per-tag config errors, no mistype churn, STRING 82, LWORD type checks; non-blocking lease release (AbCip + AbLegacy)

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe

# Conflicts:
#	src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport/Cip/CipPath.cs
This commit is contained in:
Joseph Doherty
2026-10-07 23:06:52 -04:00
16 changed files with 788 additions and 45 deletions
@@ -0,0 +1,135 @@
using Microsoft.Extensions.Logging;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport;
using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes;
using CapturingLogger = ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests.AbCipLoggingTests.CapturingLogger<ZB.MOM.WW.OtOpcUa.Driver.AbCip.AbCipDriver>;
namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests;
/// <summary>
/// Gitea #570: a tag whose own name cannot become a request is a configuration error on THAT tag. It
/// reports <c>BadConfigurationError</c>, never opens the device's connect-backoff window and never
/// degrades driver health, so healthy tags on the same device keep reading. Before the fix the native
/// encoder's raw exception was counted as a failed Forward Open: the window opened and every cold sibling
/// was throttled to <c>BadCommunicationError</c> — one typo blacked out the device.
/// </summary>
[Trait("Category", "Unit")]
public sealed class AbCipMalformedTagNameTests
{
private const string Host = "ab://10.0.0.5/1,0";
private const int Polls = 3;
#pragma warning disable OTOPCUA0001 // the driver's own status handling is under test, not resilience wrapping
[Fact]
public async Task A_name_the_encoder_refuses_is_BadConfigurationError_and_does_not_throttle_a_cold_sibling()
{
var factory = new FakeAbCipTagFactory
{
Customise = p => p.TagName == "Bad"
? new FakeAbCipTag(p)
{
ThrowOnInitialize = true,
Exception = new AbCipTagConfigurationException("'Bad' cannot be encoded"),
}
: new FakeAbCipTag(p) { Value = 7 },
};
var log = new CapturingLogger();
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Host)],
RawTags = AbCipRawTags.From(
new AbCipTagDefinition("Bad", Host, "Bad", AbCipDataType.DInt, Writable: true),
new AbCipTagDefinition("Good", Host, "Good", AbCipDataType.DInt)),
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-570", factory, logger: log);
await drv.InitializeAsync("{}", CancellationToken.None);
var health = drv.GetHealth();
for (var n = 0; n < Polls; n++)
(await drv.ReadAsync(["Bad"], CancellationToken.None)).Single().StatusCode
.ShouldBe(AbCipStatusMapper.BadConfigurationError);
(await drv.WriteAsync([new WriteRequest("Bad", 1)], CancellationToken.None)).Single().StatusCode
.ShouldBe(AbCipStatusMapper.BadConfigurationError);
// The sibling is still COLD here, so it goes through the backoff check — a window opened by the
// malformed tag would throttle it to BadCommunicationError without ever creating it.
var good = (await drv.ReadAsync(["Good"], CancellationToken.None)).Single();
good.StatusCode.ShouldBe(AbCipStatusMapper.Good);
good.Value.ShouldBe(7);
factory.Tags["Good"].InitializeCount.ShouldBe(1);
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
drv.GetHealth().State.ShouldBe(health.State);
log.Entries.Count(e => e.Level == LogLevel.Warning && e.Message.Contains("misconfigured")).ShouldBe(1);
}
[Fact]
public async Task A_malformed_TagPath_is_BadConfigurationError_without_degrading_health()
{
var factory = new FakeAbCipTagFactory { Customise = p => new FakeAbCipTag(p) { Value = 7 } };
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Host)],
RawTags = AbCipRawTags.From(
new AbCipTagDefinition("Bad", Host, "1nvalid[", AbCipDataType.DInt, Writable: true),
new AbCipTagDefinition("Good", Host, "Good", AbCipDataType.DInt)),
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-570-path", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
var results = await drv.ReadAsync(["Bad", "Good"], CancellationToken.None);
results[0].StatusCode.ShouldBe(AbCipStatusMapper.BadConfigurationError);
results[1].StatusCode.ShouldBe(AbCipStatusMapper.Good); // positive control: the batch did read
(await drv.WriteAsync([new WriteRequest("Bad", 1)], CancellationToken.None)).Single().StatusCode
.ShouldBe(AbCipStatusMapper.BadConfigurationError);
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
factory.Tags.ContainsKey("1nvalid[").ShouldBeFalse(); // never created
}
/// <summary>
/// End to end over the REAL native runtime + a private pool + a fake CIP target: a non-ASCII name (which
/// <see cref="AbCipTagPath"/> accepts — <c>char.IsLetter</c> — but the CIP symbolic encoder refuses) is
/// BadConfigurationError, the sibling on the same device reads Good, and the device sees ONE Forward Open.
/// </summary>
[Fact]
public async Task Over_the_native_runtime_a_non_ASCII_name_leaves_the_device_and_its_siblings_alone()
{
await using var target = new FakeCipTarget();
// Read Tag Fragmented (0x52) → DINT 42.
target.ConnectedHandler = req => Task.FromResult<byte[]?>(
req[0] == 0x52 ? [0xD2, 0x00, 0x00, 0x00, 0xC4, 0x00, 0x2A, 0x00, 0x00, 0x00] : target.ServiceReply(req));
var pool = new DeviceSessionPool();
var host = $"ab://127.0.0.1:{target.Port}/1,0";
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)],
RawTags = AbCipRawTags.From(
new AbCipTagDefinition("Bad", host, "Tëmp", AbCipDataType.DInt),
new AbCipTagDefinition("Good", host, "Good", AbCipDataType.DInt)),
Probe = new AbCipProbeOptions { Enabled = false },
Timeout = TimeSpan.FromSeconds(5),
}, "drv-570-native", new NativeAbCipTagFactory(pool));
await drv.InitializeAsync("{}", CancellationToken.None);
try
{
for (var n = 0; n < Polls; n++)
(await drv.ReadAsync(["Bad"], CancellationToken.None)).Single().StatusCode
.ShouldBe(AbCipStatusMapper.BadConfigurationError);
target.ForwardOpens.ShouldBe(0); // nothing reached the device
var good = (await drv.ReadAsync(["Good"], CancellationToken.None)).Single();
good.StatusCode.ShouldBe(AbCipStatusMapper.Good);
good.Value.ShouldBe(42);
target.ForwardOpens.ShouldBe(1);
drv.GetHealth().State.ShouldBe(DriverState.Healthy);
}
finally
{
await drv.ShutdownAsync(CancellationToken.None);
}
}
#pragma warning restore OTOPCUA0001
}
@@ -187,5 +187,92 @@ public sealed class AbCipTypeMismatchTests
await drv.ShutdownAsync(CancellationToken.None);
}
}
/// <summary>
/// Gitea #570 mistype churn — the path the I4 fix above did not reach. A controller that refuses the
/// EXISTENCE read itself with a type error (CIP 0xFF / 0x2107) made <c>InitializeAsync</c> throw, so the
/// driver disposed the runtime (the sole lease: Forward Close), opened the device backoff window, and
/// re-created the runtime (Forward Open) on the next poll — forever, throttled. One create and one
/// Forward Open across N polls, every poll BadTypeMismatch, health unchanged.
/// </summary>
[Fact]
public async Task A_native_type_refusal_on_the_existence_read_causes_no_runtime_or_connection_churn()
{
await using var target = new FakeCipTarget();
// Read Tag Fragmented (0x52) → general status 0xFF, extended 0x2107 (type mismatch).
target.ConnectedHandler = req => Task.FromResult<byte[]?>(
req[0] == 0x52 ? [0xD2, 0x00, 0xFF, 0x01, 0x07, 0x21] : target.ServiceReply(req));
var pool = new DeviceSessionPool();
var host = $"ab://127.0.0.1:{target.Port}/1,0";
var creates = 0;
var native = new NativeAbCipTagFactory(pool);
var counting = new CountingFactory(p => { Interlocked.Increment(ref creates); return native.Create(p); });
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(host, AbCipPlcFamily.ControlLogix)],
RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", host, "T", AbCipDataType.DInt)),
Probe = new AbCipProbeOptions { Enabled = false },
Timeout = TimeSpan.FromSeconds(5),
}, "drv-570-churn", counting);
await drv.InitializeAsync("{}", CancellationToken.None);
var health = drv.GetHealth();
try
{
for (var n = 0; n < Polls; n++)
(await drv.ReadAsync(["T"], CancellationToken.None)).Single().StatusCode
.ShouldBe(AbCipStatusMapper.BadTypeMismatch);
Volatile.Read(ref creates).ShouldBe(1);
target.ForwardOpens.ShouldBe(1);
target.ForwardCloses.ShouldBe(0);
target.ConnectedRequests.ShouldBeGreaterThanOrEqualTo(Polls); // positive control: polls hit the wire
drv.GetHealth().ShouldBe(health);
}
finally
{
await drv.ShutdownAsync(CancellationToken.None);
}
}
/// <summary>
/// The driver-side half of the same guarantee, independent of the native runtime: a runtime whose
/// <c>InitializeAsync</c> throws a type mismatch is kept, not disposed and re-created per poll.
/// </summary>
[Fact]
public async Task A_runtime_that_throws_a_type_mismatch_from_initialize_is_kept_not_recreated()
{
var creates = 0;
var mismatch = new AbTransportException(AbStatus.TypeMismatch, "REAL on the controller, DINT authored");
var factory = new FakeAbCipTagFactory
{
Customise = p =>
{
Interlocked.Increment(ref creates);
return new FakeAbCipTag(p) { ThrowOnInitialize = true, ThrowOnRead = true, Exception = mismatch };
},
};
var drv = new AbCipDriver(new AbCipDriverOptions
{
Devices = [new AbCipDeviceOptions(Host)],
RawTags = AbCipRawTags.From(new AbCipTagDefinition("T", Host, "T", AbCipDataType.DInt)),
Probe = new AbCipProbeOptions { Enabled = false },
}, "drv-570-fake", factory);
await drv.InitializeAsync("{}", CancellationToken.None);
var health = drv.GetHealth();
for (var n = 0; n < Polls; n++)
(await drv.ReadAsync(["T"], CancellationToken.None)).Single().StatusCode
.ShouldBe(AbCipStatusMapper.BadTypeMismatch);
Volatile.Read(ref creates).ShouldBe(1);
factory.Tags["T"].Disposed.ShouldBeFalse();
factory.Tags["T"].ReadCount.ShouldBe(Polls - 1); // positive control: later polls reached the kept runtime
drv.GetHealth().ShouldBe(health);
}
private sealed class CountingFactory(Func<AbCipTagCreateParams, IAbCipTagRuntime> create) : IAbCipTagFactory
{
public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams);
}
#pragma warning restore OTOPCUA0001
}
@@ -0,0 +1,139 @@
using System.Diagnostics;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport;
using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes;
namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests;
/// <summary>
/// Gitea #579: the native runtime's <c>Dispose</c> — which the driver calls under the tag's lock — used to
/// block for up to 2 s waiting for the last lease's Forward Close. It now releases in the background
/// (<see cref="ConnectionLease.ReleaseInBackground"/>, the same shape as the native AbLegacy runtime), and
/// these tests pin both halves of why that is safe: the caller is not held, and a re-create still cannot
/// overlap the close or leak a connection.
/// </summary>
[Trait("Category", "Unit")]
public sealed class NativeAbCipLeaseReleaseTests
{
private static readonly TimeSpan Guard = TimeSpan.FromSeconds(15);
private static CancellationToken Ct => TestContext.Current.CancellationToken;
private static AbCipTagCreateParams Params(int port) => new(
Gateway: "127.0.0.1",
Port: port,
CipPath: "1,0",
LibplctagPlcAttribute: "controllogix",
TagName: "T",
Timeout: TimeSpan.FromSeconds(5),
AllowPacking: false,
ElementCount: 1,
IsArray: false,
AuthoredType: AbCipDataType.DInt,
UseConnectedMessaging: true);
/// <summary>A target whose Read Tag Fragmented (0x52) answers DINT 42.</summary>
private static FakeCipTarget NewTarget()
{
var target = new FakeCipTarget();
target.ConnectedHandler = req => Task.FromResult<byte[]?>(
req[0] == 0x52 ? [0xD2, 0x00, 0x00, 0x00, 0xC4, 0x00, 0x2A, 0x00, 0x00, 0x00] : target.ServiceReply(req));
return target;
}
[Fact]
public async Task Release_under_the_tag_lock_returns_promptly_while_the_Forward_Close_is_still_pending()
{
await using var target = NewTarget();
var closeHeld = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
target.ForwardCloseGate = () => closeHeld.Task;
var pool = new DeviceSessionPool();
var factory = new NativeAbCipTagFactory(pool);
var first = (NativeAbCipTagRuntime)factory.Create(Params(target.Port));
await first.InitializeAsync(Ct);
target.ForwardOpens.ShouldBe(1);
// The driver disposes under the per-tag lock; hold one and time the release inside it.
using var tagLock = new SemaphoreSlim(1, 1);
await tagLock.WaitAsync(Ct);
var sw = Stopwatch.StartNew();
first.Dispose();
sw.Stop();
tagLock.Release();
// The old Dispose waited up to 2 s for exactly this close; it must not wait at all now.
sw.Elapsed.ShouldBeLessThan(TimeSpan.FromMilliseconds(500));
await FakeCipTarget.Until(() => target.ForwardCloses == 1, Guard, "the Forward Close");
(first.TeardownTask is null).ShouldBeFalse();
first.TeardownTask!.IsCompleted.ShouldBeFalse(); // the close really is still pending
// An immediate re-create (what the driver does after an evict) must NOT open a second session while
// the first one's Forward Close is in flight (review finding 22) — the pool holds it behind the close.
var second = (NativeAbCipTagRuntime)factory.Create(Params(target.Port));
var init = second.InitializeAsync(Ct);
await Task.Delay(300, Ct);
target.ForwardOpens.ShouldBe(1);
init.IsCompleted.ShouldBeFalse();
closeHeld.SetResult();
await first.TeardownTask.WaitAsync(Guard, Ct);
await init.WaitAsync(Guard, Ct);
target.ForwardOpens.ShouldBe(2);
second.GetStatus().ShouldBe((int)AbStatus.Ok);
second.DecodeValue(AbCipDataType.DInt, null).ShouldBe(42);
second.Dispose();
await second.TeardownTask!.WaitAsync(Guard, Ct);
target.ForwardCloses.ShouldBe(2);
}
/// <summary>
/// The offline soak: N create → read → release cycles, each release fire-and-forget and each create
/// immediately after it — the evict/re-create rhythm at its tightest. Every connection opened is closed
/// (one Forward Open and one Forward Close per cycle, one TCP connection per cycle), no TCP connection is
/// left open on the target, and the pool ends empty. A clock that runs fast lets the 1 s Forward Open
/// throttle (press safety) pass every cycle without the test taking N seconds; the throttle is still
/// exercised.
/// </summary>
[Fact]
public async Task Repeated_create_and_release_cycles_leak_no_connection()
{
const int cycles = 25;
await using var target = NewTarget();
var pool = new DeviceSessionPool(new FastForwardClock());
var factory = new NativeAbCipTagFactory(pool);
NativeAbCipTagRuntime? last = null;
for (var i = 0; i < cycles; i++)
{
var rt = (NativeAbCipTagRuntime)factory.Create(Params(target.Port));
await rt.InitializeAsync(Ct).WaitAsync(Guard, Ct);
rt.DecodeValue(AbCipDataType.DInt, null).ShouldBe(42);
rt.Dispose(); // not awaited — the next cycle starts at once
last = rt;
}
await last!.TeardownTask!.WaitAsync(Guard, Ct);
await FakeCipTarget.Until(() => target.Server.OpenConnections == 0, Guard, "every TCP connection to close");
target.ForwardOpens.ShouldBe(cycles);
target.ForwardCloses.ShouldBe(cycles);
target.Server.Connections.ShouldBe(cycles);
pool.EntryCount.ShouldBe(0);
}
/// <summary>
/// Real timers, but a monotonic clock that jumps 10 s on every read — so an interval measured between
/// two reads has always "elapsed". The Forward Open throttle reads elapsed time this way; the only other
/// reader, the keep-alive idle check, runs on a real-time timer and at worst sends a harmless keep-alive.
/// </summary>
private sealed class FastForwardClock : TimeProvider
{
private long _skew;
public override long GetTimestamp() =>
System.GetTimestamp() + Interlocked.Add(ref _skew, 10 * System.TimestampFrequency);
public override long TimestampFrequency => System.TimestampFrequency;
}
}
@@ -327,6 +327,12 @@ public sealed class NativeAbCipTagRuntimeTests
[InlineData(AbCipDataType.Int, "C400", "2A000000")] // different width
[InlineData(AbCipDataType.Real, "D300", "2A000000")] // integer ↔ real, same width
[InlineData(AbCipDataType.UDInt, "CA00", "0000803F")] // real ↔ integer, same width
// Gitea #570: the Micro800 bit-string codes are integers of a known width, not "unknown, exempt".
[InlineData(AbCipDataType.Real, "D400", "0000803F00000000")] // REAL ← Micro800 LWORD
[InlineData(AbCipDataType.LReal, "D400", "000000000000F03F")] // LREAL ← Micro800 LWORD (same width)
[InlineData(AbCipDataType.DInt, "D400", "2A00000000000000")] // DINT ← Micro800 LWORD (narrower)
[InlineData(AbCipDataType.Real, "D200", "2A00")] // REAL ← Micro800 WORD
[InlineData(AbCipDataType.Bool, "D100", "01")] // BOOL ← Micro800 BYTE
public async Task Different_widths_and_integer_real_pairs_still_mismatch(
AbCipDataType authored, string typeHex, string dataHex)
{
@@ -334,17 +340,93 @@ public sealed class NativeAbCipTagRuntimeTests
rt.GetStatus().ShouldBe((int)AbStatus.TypeMismatch);
}
[Fact]
public async Task A_malformed_tag_name_never_throws_from_Create_and_surfaces_on_Initialize_without_IO()
/// <summary>
/// Gitea #570: a name the encoder refuses is a CONFIGURATION error on this tag
/// (<see cref="AbCipTagConfigurationException"/>, which the driver reports as BadConfigurationError
/// without touching the device's backoff), raised before any I/O — not a raw encoder exception the
/// driver would count as a failed connect.
/// </summary>
[Theory]
[InlineData("Arr[abc]")] // non-numeric subscript
[InlineData("Arr[1")] // unterminated subscript
[InlineData("Arr[99999999999]")] // subscript past uint
[InlineData("Tëmp")] // not ASCII — used to go out as "T?mp"
[InlineData("A..B")] // empty member
public async Task A_malformed_tag_name_never_throws_from_Create_and_is_a_configuration_error_on_Initialize_without_IO(
string name)
{
var ch = new FakeCipChannel();
var p = Params("Arr[abc]", AbCipDataType.DInt);
var p = Params(name, AbCipDataType.DInt);
using var created = new NativeAbCipTagFactory().Create(p); // must not throw
using var rt = NativeAbCipTagRuntime.ForTest(ch, p);
await Should.ThrowAsync<Exception>(() => rt.InitializeAsync(CancellationToken.None));
await Should.ThrowAsync<AbCipTagConfigurationException>(() => rt.InitializeAsync(CancellationToken.None));
ch.SentCount.ShouldBe(0);
}
[Fact]
public async Task An_over_long_member_name_is_a_configuration_error_not_a_wrapped_length_byte()
{
var ch = new FakeCipChannel();
using var rt = NativeAbCipTagRuntime.ForTest(ch, Params(new string('A', 256), AbCipDataType.DInt));
await Should.ThrowAsync<AbCipTagConfigurationException>(() => rt.InitializeAsync(CancellationToken.None));
ch.SentCount.ShouldBe(0);
}
/// <summary>
/// Gitea #570 mistype churn: a controller that answers the existence read with a type refusal (CIP
/// 0xFF / 0x2107) has proved the tag exists. Initialize completes with the status instead of throwing
/// — a throw makes the driver dispose and re-create the runtime on every poll — and the next read
/// reports the refusal as before.
/// </summary>
[Fact]
public async Task A_type_refusal_on_the_existence_read_completes_initialize_with_the_status()
{
var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString("D200FF010721")) };
using var rt = NativeAbCipTagRuntime.ForTest(ch, Params("T", AbCipDataType.DInt));
await rt.InitializeAsync(CancellationToken.None);
rt.GetStatus().ShouldBe((int)AbStatus.TypeMismatch);
(await Should.ThrowAsync<AbTransportException>(() => rt.ReadAsync(CancellationToken.None)))
.Status.ShouldBe(AbStatus.TypeMismatch);
ch.SentCount.ShouldBe(2); // positive control: both reads reached the wire
}
/// <summary>
/// Gitea #570: the built-in Logix STRING is 88 bytes on the wire, but 2 of them are alignment padding —
/// its DATA holds 82 characters, not 84. 83 characters must be refused, 82 accepted, and the write
/// still carries the whole 88-byte structure (padding zeroed) as libplctag sends it.
/// </summary>
[Fact]
public async Task A_built_in_STRING_holds_82_characters_not_the_84_its_padding_would_allow()
{
var ch = Replying("A002CE0F", "00000000" + new string('0', 2 * 84));
using var rt = await ReadAsync(ch, Params("S", AbCipDataType.String));
ch.Respond = req => Task.FromResult(FakeCipChannel.OkReply(req));
var sentBefore = ch.SentCount;
rt.EncodeValue(AbCipDataType.String, null, new string('x', 83));
(await Should.ThrowAsync<AbTransportException>(() => rt.WriteAsync(CancellationToken.None)))
.Status.ShouldBe(AbStatus.TooLarge);
ch.SentCount.ShouldBe(sentBefore);
var full = new string('x', 82);
rt.EncodeValue(AbCipDataType.String, null, full);
await rt.WriteAsync(CancellationToken.None);
var expected = "4D02" + "91015300" + "A002" + "CE0F" + "0100"
+ "52000000" + Convert.ToHexString(Encoding.ASCII.GetBytes(full)) + "0000";
ch.Sent[^1].ShouldBe(Convert.FromHexString(expected));
}
[Fact]
public async Task A_built_in_STRING_never_decodes_its_alignment_padding_as_characters()
{
// LEN claims 84; DATA is 82 'A's then the 2 pad bytes (here non-zero, as garbage would be).
var data = "54000000" + Convert.ToHexString(Encoding.ASCII.GetBytes(new string('A', 82))) + "4242";
using var rt = await ReadAsync(Replying("A002CE0F", data), Params("S", AbCipDataType.String));
rt.DecodeValue(AbCipDataType.String, null).ShouldBe(new string('A', 82));
}
[Fact]
public async Task A_refused_write_stores_the_status_and_rethrows()
{