From 87a7414df7b8446e38bb30662f80c1a0b4ff1609 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:04:19 -0400 Subject: [PATCH 1/7] fix(s7): refuse unservable tag shapes per tag at every gate (R07:H3, R07:U4) The init-time tag guards THREW: one mis-authored tag (DB1.DBZ0, DB1.DBW0 typed Float64, T5 typed Int32, a writable Bool array) faulted the whole driver and the actor retried the same init every 10 s forever, darkening every tag on the PLC. Neither the editor nor the deploy gate checked any of it. - S7AddressParser moves to S7.Contracts (same namespace) and a new S7TagShapeRules there is the single authority on servable shapes. - S7TagDefinitionFactory.FromTagConfig applies it, so the AdminUI editor, the deploy gate (Inspect, with the specific reason) and the driver init refuse the same tags. Init now skips a refused tag (ISkippedTagReporter) instead of throwing; the throwing RejectUnsupportedTagConfigs is gone. - New rules on top of the old guards: a narrow type at the wrong access width (every read failed) and a Bool array not starting at bit 0 (U4: DB1.DBX0.3 silently read bit 0 of bytes 0..n). - AdminUI test fixtures that used addresses the driver could never serve (DB1.DBS0, a browse "SomeAddress", Int32/Float32 on DBW) now use real ones. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 17 +- .../S7AddressParser.cs | 0 .../S7TagDefinitionFactory.cs | 38 +++- .../S7TagShapeRules.cs | 149 ++++++++++++++ .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 192 ++---------------- .../Uns/TagEditors/S7TagConfigModel.cs | 5 +- .../S7_1500/S7_1500TagShapeSkipTests.cs | 57 ++++++ .../S7DriverCodeReviewFixTests.cs | 40 ++-- .../S7DriverCodeReviewFixTests2.cs | 20 +- .../S7DriverReadWriteTests.cs | 21 +- .../S7DriverScaffoldTests.cs | 114 ++--------- .../S7TagShapeGateTests.cs | 66 ++++++ .../S7TagSkipAssert.cs | 60 ++++++ .../S7ValueCodecTests.cs | 13 +- .../ScriptedS7Plc.cs | 142 +++++++++++++ .../Uns/CsvMergeEvidenceTests.cs | 2 +- .../Uns/RawTagConfigGateTests.cs | 6 +- .../Uns/S7TagConfigModelTests.cs | 6 +- .../Uns/TagConfigNonEnumParityTests.cs | 18 +- 19 files changed, 598 insertions(+), 368 deletions(-) rename src/Drivers/{ZB.MOM.WW.OtOpcUa.Driver.S7 => ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts}/S7AddressParser.cs (100%) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 02d27885..18e07821 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -103,9 +103,18 @@ Addresses use Siemens TIA-Portal / STEP 7 Classic syntax, parsed by | Timer | `T0` / `T15` | Timer area (read-only — see Wide types section) | | Counter | `C0` / `C10` | Counter area (read-only — see Wide types section) | -Parsing is strict and runs once at `InitializeAsync` so a config typo fails fast -at load instead of surfacing as `BadInternalError` on every read. Bit offsets -must be 0-7, byte offsets non-negative, DB numbers >= 1. +Parsing is strict. Bit offsets must be 0-7, byte offsets non-negative, DB numbers >= 1. + +**Tag shape rules are judged per tag, at every gate** (`S7TagShapeRules`, called by +`S7TagDefinitionFactory.FromTagConfig`): the AdminUI editor refuses the tag, the deploy gate +(`Inspect`) warns, and the driver's init **skips** it (reported through `ISkippedTagReporter`; it reads +`BadNodeIdUnknown`) while every other tag keeps working. Refused: an address that does not parse, a +wide/structured type that is an array or is not byte-addressed (`DBB`/`MB`/`IB`/`QB`), a narrow type at the +wrong access width (`Int32` on `DBW0`, `Bool` on `DBB0`...), a Timer not typed `Float64` or a Counter not typed +`Int32`, a writable or array Timer/Counter, a writable `Bool` array, and a `Bool` array that does not start +at bit 0 (a Bool array reads bit 0 of each byte, so `DB1.DBX0.3` used to silently return bit 0 of bytes +0..n). These used to be init guards that THREW, so one typo faulted every tag on the PLC and the actor +retried the same init every 10 s forever (R07:H3). ## Data types @@ -359,7 +368,7 @@ normal dev (the S7 sim is on the shared Docker host but currently offline). **Array writes (S7-016)** — a writable `Byte` / `Int16` / `UInt16` / `Int32` / `UInt32` / `Float32` array is written as ONE contiguous `WriteBytesAsync` of `count x element-bytes` (the inverse of the read decode), and the value must be an array of exactly `arrayLength` elements (else `BadTypeMismatch`, no -health impact). **Writable `Bool` arrays are refused at init** (`NotSupportedException`): the array +health impact). **Writable `Bool` arrays are refused per tag** (see the tag shape rules above): the array read is byte-granular, so a matching write would overwrite the 7 neighbouring bits of every byte it touches, and a safe bit-packed write needs read-modify-write. Mark Bool arrays `writable: false`. A `null` array element is rejected (`BadTypeMismatch`, nothing written) rather than coerced to 0. An array write larger than one PDU is split by S7.Net into several writes and is diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7AddressParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7AddressParser.cs similarity index 100% rename from src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7AddressParser.cs rename to src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7AddressParser.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs index a10c2c7d..94457fa5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs @@ -48,8 +48,23 @@ public static class S7TagDefinitionFactory /// The mapped definition when this returns . /// when is a valid S7 address object. public static bool FromTagConfig(string tagConfig, string rawPath, out S7TagDefinition def) + => FromTagConfig(tagConfig, rawPath, out def, out _); + + /// + /// , also reporting WHY a structurally + /// valid blob was refused by (an unparseable address, or a shape the + /// driver cannot serve). stays for a JSON-level + /// refusal (missing keys, bad enum), which explains on its own. + /// + /// The authored equipment-tag TagConfig JSON. + /// The tag's RawPath — becomes the definition's identity (Name). + /// The mapped definition when this returns . + /// The shape/address rejection reason, when that is why it refused. + /// when the driver can serve the tag. + public static bool FromTagConfig(string tagConfig, string rawPath, out S7TagDefinition def, out string? shapeRejection) { def = null!; + shapeRejection = null; if (string.IsNullOrWhiteSpace(tagConfig)) return false; try { @@ -78,7 +93,7 @@ public static class S7TagDefinitionFactory // "writable" defaults to true when absent (today's value); node-level authz still governs // writes. Honouring the key makes read-only equipment tags authorable. var writable = TagConfigJson.ReadWritable(root); - def = new S7TagDefinition( + var mapped = new S7TagDefinition( Name: rawPath, Address: address, DataType: dataType, @@ -86,6 +101,15 @@ public static class S7TagDefinitionFactory StringLength: stringLength == 0 ? MaxStringLength : stringLength, WriteIdempotent: false, ArrayCount: arrayCount); + // The address must parse and the shape must be one the driver can serve. These used to be + // init-time guards that THREW and faulted every tag on the PLC; refusing here skips only + // this tag, and the editor + deploy gate (which ask this method) refuse it too. + if (S7TagShapeRules.Validate(mapped, out _) is { } reason) + { + shapeRejection = reason; + return false; + } + def = mapped; return true; } catch (JsonException) { return false; } @@ -162,7 +186,9 @@ public static class S7TagDefinitionFactory /// reject — a structurally unparseable or non-object document, an absent / non-string / /// blank address, an absent dataType (see ), a /// present-but-invalid dataType (the runtime enum read is STRICT, so a - /// typo rejects the tag), or an out-of-range stringLength on a String tag. Such a tag + /// typo rejects the tag), an out-of-range stringLength on a String tag, or a shape + /// refuses (an unparseable address, a wrong access width, a mis-typed + /// Timer/Counter, an unsupported array). Such a tag /// is skipped by the driver and reads BadNodeIdUnknown forever. Empty when the mapper accepts /// the blob, when it is blank, or when it is not an equipment-tag object. Never throws. /// @@ -184,7 +210,13 @@ public static class S7TagDefinitionFactory // The accept gate: whatever the driver reads draws nothing. The RAW reference is handed to the // mapper — the driver reads the stored blob, not a trimmed copy of it. - if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings; + if (FromTagConfig(reference, InspectionRawPath, out _, out var shapeRejection)) return warnings; + + if (shapeRejection is not null) + { + warnings.Add(shapeRejection + " The driver will skip this tag and it will read BadNodeIdUnknown."); + return warnings; + } try { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs new file mode 100644 index 00000000..17a9bf48 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagShapeRules.cs @@ -0,0 +1,149 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.S7; + +/// +/// The single authority on which S7 tag shapes the driver can serve. Consulted by +/// , so the AdminUI editor, the deploy gate +/// () and the driver's own init all refuse the same +/// tags with the same words. +/// +/// +/// +/// These rules used to be init-time guards in S7Driver that THREW. One mis-authored tag +/// (DB1.DBZ0, DB1.DBW0 typed Float64, T5 typed Int32) faulted the +/// whole driver, and the actor retried that same init every 10 s forever, darkening every tag on +/// the PLC. The editor and the deploy gate checked none of it, and a CSV import or API blob +/// bypasses the editor entirely. A rejected tag is now skipped on its own (it reads +/// BadNodeIdUnknown) and the deploy gate warns about it. +/// +/// +/// Every rule rejects a tag that could never be served correctly. None of them can newly break +/// a tag that used to work: each one either faulted init, or failed (or silently mis-decoded) +/// every read of that tag. +/// +/// +public static class S7TagShapeRules +{ + /// + /// S7 data types addressed by START BYTE (the B suffix) rather than by an access-width + /// suffix. The driver decodes them from a contiguous byte buffer, so a wide tag authored against a + /// non-byte suffix (DBW/DBD/...) would mis-frame its value. + /// + private static readonly HashSet ByteAnchoredDataTypes = + [ + S7DataType.Int64, + S7DataType.UInt64, + S7DataType.Float64, + S7DataType.String, + S7DataType.DateTime, + S7DataType.WString, + S7DataType.Char, + S7DataType.WChar, + S7DataType.Dtl, + S7DataType.Date, + S7DataType.Tod, + S7DataType.Time, + S7DataType.S5Time, + ]; + + /// True when is a wide/structured type read from a byte-anchored block. + /// The declared data type. + /// for the byte-anchored types. + public static bool IsByteAnchored(S7DataType dataType) => ByteAnchoredDataTypes.Contains(dataType); + + /// + /// Judges one tag definition. Returns when the driver can serve it, or + /// the operator-facing reason it cannot. + /// + /// The mapped tag definition. + /// The parsed address when this returns . + /// The rejection reason, or when the tag is servable. + public static string? Validate(S7TagDefinition def, out S7ParsedAddress parsed) + { + ArgumentNullException.ThrowIfNull(def); + parsed = default; + + S7ParsedAddress addr; + try + { + addr = S7AddressParser.Parse(def.Address); + } + catch (FormatException ex) + { + return $"S7 address '{def.Address}' does not parse: {ex.Message}"; + } + + var isWide = IsByteAnchored(def.DataType); + var isArray = def.ArrayCount is >= 1; + + // Wide-type arrays have no decoder. + if (isWide && isArray) + return $"S7 {def.DataType} arrays are not supported. Use a scalar {def.DataType} tag, or an " + + "Int16/UInt16/Int32/UInt32/Float32/Byte/Bool element type for an array."; + + // Timer / Counter type constraints: a timer decodes to Float64 seconds, a counter to an Int32 count. + if (addr.Area is S7Area.Timer && def.DataType != S7DataType.Float64) + return $"S7 Timer address '{def.Address}' is typed {def.DataType}. Timer tags must be " + + "DataType=Float64 (decoded to seconds, read-only)."; + if (addr.Area is S7Area.Counter && def.DataType != S7DataType.Int32) + return $"S7 Counter address '{def.Address}' is typed {def.DataType}. Counter tags must be " + + "DataType=Int32 (decoded to a count, read-only)."; + if (addr.Area is S7Area.Timer or S7Area.Counter) + { + if (isArray) + return $"S7 Timer/Counter address '{def.Address}' cannot be an array."; + if (def.Writable) + return $"S7 Timer/Counter address '{def.Address}' is declared writable. Timer/Counter tags " + + "are read-only; set Writable=false."; + parsed = addr; + return null; + } + + // Wide/structured scalars on DB/M/I/Q must be byte-anchored (DBB/MB/IB/QB). + if (isWide) + { + if (addr.Size != S7Size.Byte) + return $"S7 {def.DataType} at '{def.Address}' is not byte-addressed. Wide/structured types " + + "must be byte-addressed (DBB/MB/IB/QB), e.g. 'DB1.DBB0', 'MB4'."; + parsed = addr; + return null; + } + + // Narrow types: the declared type must match the address's access width, or every read fails + // (or decodes garbage) with a type mismatch. + var required = def.DataType switch + { + S7DataType.Bool => S7Size.Bit, + S7DataType.Byte => S7Size.Byte, + S7DataType.Int16 or S7DataType.UInt16 => S7Size.Word, + _ => S7Size.DWord, // Int32 / UInt32 / Float32 + }; + if (addr.Size != required) + return $"S7 {def.DataType} at '{def.Address}' has the wrong access width ({addr.Size}). " + + $"A {def.DataType} tag needs a {SuffixHint(required)} address."; + + if (isArray && def.DataType == S7DataType.Bool) + { + // Element i of a Bool array is bit 0 of byte (start + i): the array is byte-granular. A + // non-zero bit offset used to be silently ignored (DB1.DBX0.3 returned bit 0 of bytes 0..n). + if (addr.BitOffset != 0) + return $"S7 Bool array at '{def.Address}' starts at bit {addr.BitOffset}. A Bool array reads " + + "one byte per element (bit 0 of each byte), so it must start at bit 0 (e.g. 'DB1.DBX0.0')."; + // A byte-granular write would overwrite the 7 neighbouring bits of every byte it touches. + if (def.Writable) + return $"S7 Bool array at '{def.Address}' is declared writable. Bool array writes are not " + + "supported (a byte-granular write would clobber neighbouring bits); set Writable=false " + + "or write the bits as individual scalar Bool tags."; + } + + parsed = addr; + return null; + } + + private static string SuffixHint(S7Size size) => size switch + { + S7Size.Bit => "bit (DBX/M/I/Q n.b)", + S7Size.Byte => "byte (DBB/MB/IB/QB)", + S7Size.Word => "word (DBW/MW/IW/QW)", + _ => "double-word (DBD/MD/ID/QD)", + }; +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 8659565a..347a0b19 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -239,41 +239,36 @@ public sealed class S7Driver if (HasConfigBody(driverConfigJson)) _options = S7DriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); - // v3: build the RawPath → definition table from the authored raw tags BEFORE the config - // guards run (they now iterate the mapped table). Each entry's TagConfig blob is mapped by - // the pure factory; the entry's WriteIdempotent flag is threaded onto the def (it lives on - // the RawTagEntry, not inside the blob). A mapper miss is skipped (logged), never thrown — - // one bad tag must not fail the whole driver init. + // v3: build the RawPath → definition table from the authored raw tags. Each entry's TagConfig + // blob is mapped by the pure factory, which also applies S7TagShapeRules (the address must + // parse; wide arrays, mis-typed Timer/Counter, wrong access widths, writable Timer/Counter or + // Bool arrays and offset Bool arrays are refused). A refused tag is SKIPPED (logged, reported + // through ISkippedTagReporter, reads BadNodeIdUnknown), never thrown: these used to be init + // guards that faulted every tag on the PLC and made the actor retry the same init forever. + // The entry's WriteIdempotent flag is threaded onto the def (it lives on the RawTagEntry). _tagsByRawPath.Clear(); _parsedByRawPath.Clear(); _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry _skippedTags.BeginRebuild(_options.RawTags.Count); foreach (var entry in _options.RawTags) { - if (S7TagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def)) + if (S7TagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def, out var shapeRejection) + && S7AddressParser.TryParse(def.Address, out var parsedAddress)) + { _tagsByRawPath[entry.RawPath] = def with { WriteIdempotent = entry.WriteIdempotent }; + _parsedByRawPath[entry.RawPath] = parsedAddress; + } else { - _skippedTags.Skip(entry.RawPath, "TagConfig did not map to a definition"); + var reason = shapeRejection ?? "TagConfig did not map to a definition"; + _skippedTags.Skip(entry.RawPath, reason); _logger.LogWarning( - "S7 tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}", - _driverInstanceId, entry.RawPath); + "S7 tag skipped: {Reason} Driver={DriverInstanceId} RawPath={RawPath}", + reason, _driverInstanceId, entry.RawPath); } } _skippedTags.Commit(); - // Phase 4d combined config guard. Timer/Counter addresses and the wide/structured - // types (Int64/UInt64/Float64/String/DateTime) are now SUPPORTED, but only in - // specific shapes — this rejects the genuinely-unsupported configurations - // (wide-type arrays, non-byte-addressed wide scalars, wrong Timer/Counter DataType) - // so a config typo fails fast at init instead of as a misleading per-read fault. - RejectUnsupportedTagConfigs(); - - // Legacy data-type seam. The UnimplementedDataTypes set is now - // empty (Phase 4d unblocked all five wide types), so this rejects nothing — kept as - // the single grep target / future seam should a type ever need re-gating at init. - RejectUnsupportedTagDataTypes(); - // Build the connection through the factory (the same seam EnsureConnectedAsync's reopen // uses) so the read/write timeouts are baked in identically on connect and reconnect. var plc = _plcFactory.Create(_options); @@ -306,15 +301,6 @@ public sealed class S7Driver _connectBackoff.RecordSuccess(); Volatile.Write(ref _negotiatedPduSize, plc.MaxPduSize); - // Parse every tag's address once at init so config typos fail fast here instead - // of surfacing as BadInternalError on every Read against the bad tag. The parser - // also rejects bit-offset > 7, DB 0, unknown area letters, etc. Keyed by RawPath - // (== def.Name) so Read/Write can pick up the pre-parsed address. - foreach (var t in _tagsByRawPath.Values) - { - _parsedByRawPath[t.Name] = S7AddressParser.Parse(t.Address); // throws FormatException - } - _initialized = true; Health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); _logger.LogInformation("S7Driver connected. Driver={DriverInstanceId} Host={Host} CPU={CpuType} PduSize={PduSize} Tags={TagCount}", @@ -437,13 +423,6 @@ public sealed class S7Driver return trimmed is not "{}" and not "[]"; } - /// - /// S7 data types addressed by START BYTE (the B suffix) rather than by an - /// access-width suffix. Phase 4d decodes these from a contiguous byte buffer - /// (DB{n}.DBB{offset} / MB / IB / QB{offset}), so a wide tag - /// authored against a non-byte suffix (DBW/DBD/…) is a config error — see - /// . - /// /// /// PDU size the CPU negotiated on the most recent successful connect, in bytes; 0 before the first /// connect and after shutdown. Snapshotted at connect so it stays readable while the handle is @@ -474,143 +453,6 @@ public sealed class S7Driver : new Dictionary(); } - private static readonly HashSet ByteAnchoredDataTypes = new() - { - S7DataType.Int64, - S7DataType.UInt64, - S7DataType.Float64, - S7DataType.String, - S7DataType.DateTime, - S7DataType.WString, - S7DataType.Char, - S7DataType.WChar, - S7DataType.Dtl, - S7DataType.Date, - S7DataType.Tod, - S7DataType.Time, - S7DataType.S5Time, - }; - - /// - /// Combined init-time config guard (Phase 4d, supersedes the old Timer/Counter-only - /// RejectUnsupportedTagAddresses). Timer/Counter tags and the wide/structured - /// types (Int64/UInt64/Float64/String/DateTime) are now supported — but only in - /// specific shapes. This guard fails fast on the genuinely-unsupported configurations so - /// a config typo throws here at init rather than as a misleading per-read fault: - /// - /// (a) a wide/structured type declared as an array — wide-type arrays are out of scope this phase. - /// (b) a wide/structured type on a DB/M/I/Q area whose address isn't byte-anchored (DBB/MB/IB/QB). - /// (c) a Timer tag not typed Float64, or a Counter tag not typed Int32. - /// - /// Addresses that don't parse are left to the existing - /// pass in so the parse error isn't double-handled. - /// - private void RejectUnsupportedTagConfigs() - { - foreach (var t in _tagsByRawPath.Values) - { - var isWide = ByteAnchoredDataTypes.Contains(t.DataType); - - // (a) wide-type arrays — out of scope this phase. - if (isWide && t.ArrayCount is >= 1) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a {t.DataType} array, which the S7 driver does not yet " + - "support. Use a scalar wide tag, or an Int16/UInt16/Int32/UInt32/Float32/Byte/Bool " + - "element type for arrays."); - } - - // Parse the address; if it doesn't parse, leave it for the InitializeAsync - // S7AddressParser.Parse pass to throw the FormatException (no double-handling). - if (!S7AddressParser.TryParse(t.Address, out var parsed)) - continue; - - // (b) wide/structured scalar on DB/M/I/Q must be byte-anchored (Size == Byte). The - // codec decodes from a byte buffer at the start byte; a non-byte suffix (DBW/DBD/…) - // would mis-frame the value. - if (isWide - && parsed.Area is S7Area.DataBlock or S7Area.Memory or S7Area.Input or S7Area.Output - && parsed.Size != S7Size.Byte) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a {t.DataType} at '{t.Address}', which is not byte-addressed. " + - "Wide/structured types must be byte-addressed (DBB/MB/IB/QB) — e.g. 'DB1.DBB0', 'MB4'."); - } - - // (c) Timer/Counter type constraints — Timer decodes to a Float64 (seconds), Counter - // to an Int32 (count); both are read-only. - if (parsed.Area is S7Area.Timer && t.DataType != S7DataType.Float64) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Timer address ('{t.Address}') but is typed {t.DataType}. " + - "Timer tags must be DataType=Float64 (decoded to seconds, read-only)."); - } - if (parsed.Area is S7Area.Counter && t.DataType != S7DataType.Int32) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Counter address ('{t.Address}') but is typed {t.DataType}. " + - "Counter tags must be DataType=Int32 (decoded to count, read-only)."); - } - - // (d) Timer/Counter declared Writable — writes are read-only this phase; without this - // a node is discovered as Operate-writable but every write returns BadNotSupported. - if (parsed.Area is S7Area.Timer or S7Area.Counter && t.Writable) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Timer/Counter ('{t.Address}') declared Writable — " + - "Timer/Counter are read-only this phase; set Writable=false."); - } - - // (e) Bool array declared Writable — element writes are byte-granular (one byte per - // element, mirroring the read), so a write would overwrite the 7 neighbouring bits of - // every byte it touches; a safe bit-packed write needs read-modify-write and is not - // implemented. Every other supported element type (Byte/Int16/UInt16/Int32/UInt32/ - // Float32) writes as ONE contiguous block (EncodeArrayBlock). Wide-type arrays - // are already rejected above (guard-a). - if (t.ArrayCount is >= 1 && t.Writable && t.DataType == S7DataType.Bool) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' is a Bool array (ArrayCount={t.ArrayCount}) declared Writable — " + - "Bool array writes are not supported (a byte-granular write would clobber the neighbouring " + - "bits). Set Writable=false, or write the bits as individual scalar Bool tags."); - } - } - } - - /// - /// Rejects tags configured with an still on the - /// list — types / - /// throw for, which - /// would otherwise create live OPC UA nodes via that return - /// BadNotSupported on every access. - /// The set is now empty (Phase 4d unblocked all five wide types), so this rejects - /// nothing — kept as the seam / grep target should a type ever need re-gating at init. - /// - private void RejectUnsupportedTagDataTypes() - { - foreach (var t in _tagsByRawPath.Values) - { - if (UnimplementedDataTypes.Contains(t.DataType)) - { - throw new NotSupportedException( - $"S7 tag '{t.Name}' uses data type '{t.DataType}' which is not yet " + - "supported by the S7 driver — Read/Write would return BadNotSupported. " + - "Remove the tag or use Bool/Byte/Int16/UInt16/Int32/UInt32/Float32 until " + - $"{t.DataType} is wired through."); - } - } - } - - /// - /// S7DataType members that the read/write helpers throw NotSupportedException for. - /// Now empty — Phase 4d (S7 wide types + Timer/Counter) wired Int64, UInt64, - /// Float64, String, and DateTime through the byte-buffer codec, and Timer/Counter read - /// through it on AREA. Kept here (rather than reflecting over - /// ) as the single grep target / future seam should a - /// data type ever need re-gating at init. - /// - private static readonly HashSet UnimplementedDataTypes = new(); - // ---- IReadable ---- // The batch is served in two passes under ONE gate acquisition. First S7ReadSpanPlanner groups @@ -1378,7 +1220,7 @@ public sealed class S7Driver /// true if the tag routes through the byte-buffer codec. internal static bool IsBufferType(S7TagDefinition tag, S7ParsedAddress addr) => addr.Area is S7Area.Timer or S7Area.Counter - || ByteAnchoredDataTypes.Contains(tag.DataType); + || S7TagShapeRules.IsByteAnchored(tag.DataType); /// /// Reads a wide/structured scalar as ONE contiguous byte block via S7.Net's diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs index 49b4e199..723cd369 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs @@ -112,8 +112,9 @@ public sealed class S7TagConfigModel + string.Join(", ", Enum.GetNames()) + "."; } - // Array shape is judged against the driver's INIT guards, which THROW (failing the whole - // driver's init, not just this tag) for a wide-type array and for a writable Bool array. The + // Array shape: the friendlier editor wording for two of the S7TagShapeRules refusals (a wide-type + // array, a writable Bool array). The reader below refuses them too (and the rest of the shape + // rules), so this is wording, not a second authority. The // isArray / arrayLength keys themselves belong to the TagModal's generic array controls // (TagArrayConfig) and are intentionally not fields of this model, so read them off the key bag. if (TagConfigJson.GetBool(_bag, "isArray")) diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs new file mode 100644 index 00000000..17bc9a61 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500TagShapeSkipTests.cs @@ -0,0 +1,57 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// Live gate for R07:H3 — one mis-authored tag must not darken the PLC. Before the fix each of +/// these shapes threw at init, so the driver Faulted and served nothing (and the actor retried the +/// same init every 10 s forever). Now each is skipped on its own and the good tags keep reading +/// their seeded values over real S7comm. +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500TagShapeSkipTests(Snap7ServerFixture sim) +{ + /// Unservable tags are skipped per tag; the seeded neighbours still read Good. + [Fact] + public async Task Mis_authored_tags_are_skipped_and_the_rest_of_the_PLC_still_reads() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var ct = TestContext.Current.CancellationToken; + + var good = S7_1500Profile.BuildOptions(sim.Host, sim.Port); + var bad = new[] + { + new S7TagDefinition("Bad/Typo", "DB1.DBZ0", S7DataType.Int16), + new S7TagDefinition("Bad/WideOnWord", "DB1.DBW0", S7DataType.Float64), + new S7TagDefinition("Bad/TimerAsInt", "T5", S7DataType.Int32, Writable: false), + new S7TagDefinition("Bad/OffsetBoolArray", "DB1.DBX50.3", S7DataType.Bool, Writable: false, ArrayCount: 8), + }; + var options = new S7DriverOptions + { + Host = good.Host, Port = good.Port, CpuType = good.CpuType, Rack = good.Rack, Slot = good.Slot, + Timeout = good.Timeout, Probe = good.Probe, + RawTags = [.. good.RawTags, .. S7RawTags.Entries(bad)], + }; + + await using var drv = new S7Driver(options, driverInstanceId: "s7-shape-skip-live"); + await drv.InitializeAsync("{}", ct); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + drv.GetSkippedTags().ShouldNotBeNull().Sample.Select(s => s.RawPath) + .ShouldBe(bad.Select(b => b.Name), ignoreOrder: true); + + var r = await drv.ReadAsync( + [S7_1500Profile.ProbeTag, S7_1500Profile.SmokeI16Tag, S7_1500Profile.SmokeBoolTag, bad[0].Name], ct); + r[0].StatusCode.ShouldBe(0u); + Convert.ToInt32(r[0].Value).ShouldBe(S7_1500Profile.ProbeSeedValue); + r[1].StatusCode.ShouldBe(0u); + Convert.ToInt32(r[1].Value).ShouldBe((int)S7_1500Profile.SmokeI16SeedValue); + r[2].StatusCode.ShouldBe(0u); + Convert.ToBoolean(r[2].Value).ShouldBeTrue(); + r[3].StatusCode.ShouldBe(0x80340000u, "a skipped tag reads BadNodeIdUnknown"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs index 9c1c3a8a..6f0f12b8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests.cs @@ -25,26 +25,10 @@ public sealed class S7DriverCodeReviewFixTests [InlineData("C10")] public async Task Initialize_rejects_timer_or_counter_tag_with_NotSupportedException(string address) { - // A Timer/Counter address parses cleanly but the read path has no decode case for it, - // so it must fail fast at init rather than throw a misleading type-mismatch on every - // read. The host is reserved-for-documentation so the TCP connect can never succeed — - // the unsupported-address guard runs before the connect, so the NotSupportedException - // is what surfaces. - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Quirk", address, S7DataType.Int16)), - }; - using var drv = new S7Driver(opts, "s7-tc"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain(address); - - var health = drv.GetHealth(); - health.State.ShouldBe(DriverState.Faulted, "an unsupported-address config error must Fault the driver"); - health.LastError.ShouldNotBeNull(); + // A Timer/Counter address typed anything but Float64 (Timer) / Int32 (Counter) cannot be decoded. + // It is refused PER TAG at every gate; it used to throw at init and fault every tag on the PLC. + await S7TagSkipAssert.RefusedPerTagAsync( + new S7TagDefinition("Quirk", address, S7DataType.Int16, Writable: false), address); } /// Verifies that initialize accepts DB and MIQ addresses without triggering the unsupported guard. @@ -102,19 +86,21 @@ public sealed class S7DriverCodeReviewFixTests [Fact] public async Task Initialize_rejects_a_timer_tag_supplied_only_through_driverConfigJson() { - // The Timer/Counter guard must run against the *re-parsed* config, not just the - // constructor options — proves Driver.S7-001 and Driver.S7-011 compose correctly. + // The shape gate must run against the *re-parsed* config, not just the constructor options — + // proves the per-tag skip and Driver.S7-011 compose correctly. var ctorOpts = new S7DriverOptions { Host = "192.0.2.1", Timeout = TimeSpan.FromMilliseconds(250) }; - using var drv = new S7Driver(ctorOpts, "s7-cfg-tc"); + using var drv = new S7Driver(ctorOpts, "s7-cfg-tc", new ScriptedS7PlcFactory()); const string json = """ - { "Host": "192.0.2.1", "TimeoutMs": 250, + { "Host": "192.0.2.1", "TimeoutMs": 250, "Probe": { "Enabled": false }, "RawTags": [ { "RawPath": "TimerTag", "TagConfig": "{\"address\":\"T5\",\"dataType\":\"Int16\"}", "WriteIdempotent": false } ] } """; - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync(json, TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("T5"); + await drv.InitializeAsync(json, TestContext.Current.CancellationToken); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + var skipped = drv.GetSkippedTags().ShouldNotBeNull().Sample.ShouldHaveSingleItem(); + skipped.RawPath.ShouldBe("TimerTag"); + skipped.Reason.ShouldContain("T5"); } /// Verifies that reinitialize applies a changed driverConfigJson. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs index 75731de5..59bb0922 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverCodeReviewFixTests2.cs @@ -158,23 +158,9 @@ public sealed class S7DriverCodeReviewFixTests2 [InlineData(S7DataType.DateTime)] public async Task Initialize_rejects_wide_type_at_non_byte_address_with_NotSupportedException(S7DataType dt) { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - // DB1.DBD0 is a DWord (non-byte) address — guard-(b) must fault on the address shape - // for a wide type. (At a byte address like DB1.DBB0 the 8-byte numerics round-trip; - // see S7ScalarBlockTests.) - RawTags = S7RawTags.Entries(new S7TagDefinition("X", "DB1.DBD0", dt)), - }; - using var drv = new S7Driver(opts, $"s7-wide-nonbyte-{dt}"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain(dt.ToString()); - - var health = drv.GetHealth(); - health.State.ShouldBe(DriverState.Faulted); + // DB1.DBD0 is a DWord (non-byte) address — a wide type there would mis-frame its value. (At a + // byte address like DB1.DBB0 the 8-byte numerics round-trip; see S7ScalarBlockTests.) + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("X", "DB1.DBD0", dt), dt.ToString(), "byte-addressed"); } /// Verifies that Initialize accepts implemented data types. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs index 9f59d91c..d5bca500 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverReadWriteTests.cs @@ -17,22 +17,11 @@ public sealed class S7DriverReadWriteTests [Fact] public async Task Initialize_rejects_invalid_tag_address_and_fails_fast() { - // Bad address at init time must throw; the alternative (deferring the parse to the - // first read) would surface the config bug as BadInternalError on every subsequent - // Read which is impossible for an operator to diagnose from the OPC UA client. - var opts = new S7DriverOptions - { - Host = "192.0.2.1", // reserved — will never complete TCP handshake - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("BadTag", "NOT-AN-S7-ADDRESS", S7DataType.Int16)), - }; - using var drv = new S7Driver(opts, "s7-bad-tag"); - - // Either the TCP connect fails first (Exception) or the parser fails (FormatException) - // — both are acceptable since both are init-time fail-fast. What matters is that we - // don't return a "healthy" driver with a latent bad tag. - await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + // A bad address is refused PER TAG at every gate (mapper/editor, deploy gate, driver init): + // it reads BadNodeIdUnknown while its neighbours keep working. It used to throw at init and + // fault every tag on the PLC, with the actor retrying the same init every 10 s forever. + await S7TagSkipAssert.RefusedPerTagAsync( + new S7TagDefinition("BadTag", "NOT-AN-S7-ADDRESS", S7DataType.Int16), "does not parse"); } /// Verifies that ReadAsync without initialize throws InvalidOperationException. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs index 6c1c6561..a8e4e52e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs @@ -101,80 +101,32 @@ public sealed class S7DriverScaffoldTests [Fact] public async Task Initialize_rejects_wide_type_array_with_clear_message() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Batch64", "DB1.DBB0", S7DataType.Int64, ArrayCount: 4)), - }; - using var drv = new S7Driver(opts, "s7-wide-array"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Batch64"); - ex.Message.ShouldContain("array", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Batch64", "DB1.DBB0", S7DataType.Int64, ArrayCount: 4), "Int64", "array"); } /// Verifies that a wide scalar with a non-byte address (Float64 at DB1.DBW0) is rejected with a byte-address message. [Fact] public async Task Initialize_rejects_wide_scalar_with_non_byte_address() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("WideWord", "DB1.DBW0", S7DataType.Float64)), - }; - using var drv = new S7Driver(opts, "s7-wide-nonbyte"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("WideWord"); - ex.Message.ShouldContain("byte", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("WideWord", "DB1.DBW0", S7DataType.Float64), "Float64", "byte-addressed"); } /// Verifies that a Timer tag with the wrong DataType (Int32, not Float64) is rejected. [Fact] public async Task Initialize_rejects_Timer_tag_with_wrong_data_type() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Timer5", "T5", S7DataType.Int32)), - }; - using var drv = new S7Driver(opts, "s7-timer-bad"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Timer5"); - ex.Message.ShouldContain("Float64"); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Timer5", "T5", S7DataType.Int32, Writable: false), "T5", "Float64"); } /// Verifies that a Counter tag with the wrong DataType (Float32, not Int32) is rejected. [Fact] public async Task Initialize_rejects_Counter_tag_with_wrong_data_type() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Counter3", "C3", S7DataType.Float32)), - }; - using var drv = new S7Driver(opts, "s7-counter-bad"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Counter3"); - ex.Message.ShouldContain("Int32"); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Counter3", "C3", S7DataType.Float32, Writable: false), "C3", "Int32"); } // ── Phase 4d bundle-review fix — Writable Timer/Counter guard ─────────────────────── @@ -188,42 +140,16 @@ public sealed class S7DriverScaffoldTests [Fact] public async Task Initialize_rejects_Timer_tag_declared_Writable() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Timer5", "T5", S7DataType.Float64, Writable: true)), - }; - using var drv = new S7Driver(opts, "s7-timer-writable"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Timer5"); - ex.Message.ShouldContain("Writable", Case.Insensitive); - ex.Message.ShouldContain("read-only", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Timer5", "T5", S7DataType.Float64, Writable: true), "T5", "writable", "read-only"); } /// Verifies that a Counter tag declared Writable=true is rejected at init with a Writable message. [Fact] public async Task Initialize_rejects_Counter_tag_declared_Writable() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("Counter3", "C3", S7DataType.Int32, Writable: true)), - }; - using var drv = new S7Driver(opts, "s7-counter-writable"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("Counter3"); - ex.Message.ShouldContain("Writable", Case.Insensitive); - ex.Message.ShouldContain("read-only", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Counter3", "C3", S7DataType.Int32, Writable: true), "C3", "writable", "read-only"); } // ── Driver.S7-015 / S7-016 — writable Bool array must fail fast at init ───────────── @@ -236,20 +162,8 @@ public sealed class S7DriverScaffoldTests [Fact] public async Task Initialize_rejects_writable_bool_array_tag_with_NotSupportedException() { - var opts = new S7DriverOptions - { - Host = "192.0.2.1", - Timeout = TimeSpan.FromMilliseconds(250), - RawTags = S7RawTags.Entries(new S7TagDefinition("ArrTag", "DB1.DBX0.0", S7DataType.Bool, Writable: true, ArrayCount: 4)), - }; - using var drv = new S7Driver(opts, "s7-writable-bool-arr"); - - var ex = await Should.ThrowAsync(async () => - await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); - ex.Message.ShouldContain("ArrTag"); - ex.Message.ShouldContain("Bool array", Case.Insensitive); - - drv.GetHealth().State.ShouldBe(DriverState.Faulted); + // Refused PER TAG at every gate (it used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("ArrTag", "DB1.DBX0.0", S7DataType.Bool, Writable: true, ArrayCount: 4), "Bool array", "writable"); } /// Verifies that writable non-bool arrays pass the init guard — the failure diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs new file mode 100644 index 00000000..3bc47dca --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagShapeGateTests.cs @@ -0,0 +1,66 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:H3 / R07:U4 — the tag shapes the driver cannot serve are refused per tag at the mapper, +/// the deploy gate and the driver's init (see ), instead of faulting +/// the whole driver at init or being silently mis-read. +/// +[Trait("Category", "Unit")] +public sealed class S7TagShapeGateTests +{ + /// A typo'd area/size letter is refused per tag (it used to throw FormatException at init). + [Fact] + public Task Unparseable_address_is_refused_per_tag() + => S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Typo", "DB1.DBZ0", S7DataType.Int16), "DB1.DBZ0", "does not parse"); + + /// + /// R07:U4 — a Bool array reads bit 0 of each byte, so a non-zero start bit was silently ignored + /// (DB1.DBX0.3 returned bit 0 of bytes 0..7). It is refused at authoring now. + /// + [Fact] + public Task Bool_array_starting_at_a_nonzero_bit_is_refused() + => S7TagSkipAssert.RefusedPerTagAsync( + new S7TagDefinition("OffsetBits", "DB1.DBX0.3", S7DataType.Bool, Writable: false, ArrayCount: 8), + "Bool array", "bit 3"); + + /// A narrow type at the wrong access width fails every read with a type mismatch; refused up front. + /// The address. + /// The declared type, which does not match the address's access width. + [Theory] + [InlineData("DB1.DBW0", S7DataType.Int32)] + [InlineData("DB1.DBD0", S7DataType.Int16)] + [InlineData("DB1.DBB0", S7DataType.Bool)] + [InlineData("M0.0", S7DataType.Byte)] + [InlineData("MW0", S7DataType.Float32)] + public Task Narrow_type_at_the_wrong_access_width_is_refused(string address, S7DataType dt) + => S7TagSkipAssert.RefusedPerTagAsync(new S7TagDefinition("Mismatch", address, dt), "access width"); + + /// Positive control: the shapes the driver serves are all still accepted by the mapper and the gate. + /// The address. + /// The declared type. + /// The array count, or 0 for a scalar. + /// Whether the tag is writable. + [Theory] + [InlineData("DB1.DBX0.0", S7DataType.Bool, 8, false)] + [InlineData("DB1.DBX50.3", S7DataType.Bool, 0, true)] + [InlineData("DB1.DBB0", S7DataType.Byte, 4, true)] + [InlineData("DB1.DBW0", S7DataType.Int16, 0, true)] + [InlineData("MW2", S7DataType.UInt16, 3, true)] + [InlineData("DB1.DBD0", S7DataType.Float32, 0, true)] + [InlineData("QD4", S7DataType.Int32, 2, true)] + [InlineData("DB1.DBB0", S7DataType.Float64, 0, true)] + [InlineData("DB1.DBB0", S7DataType.String, 0, true)] + [InlineData("T5", S7DataType.Float64, 0, false)] + [InlineData("C3", S7DataType.Int32, 0, false)] + public void Servable_shapes_are_accepted(string address, S7DataType dt, int arrayCount, bool writable) + { + var def = new S7TagDefinition("Ok", address, dt, Writable: writable, ArrayCount: arrayCount == 0 ? null : arrayCount); + var blob = S7TagDefinitionFactory.ToTagConfig(def); + S7TagDefinitionFactory.FromTagConfig(blob, "Ok", out var mapped, out var reason).ShouldBeTrue(reason); + mapped.Address.ShouldBe(address); + S7TagDefinitionFactory.Inspect(blob).ShouldBeEmpty(); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs new file mode 100644 index 00000000..47f96de4 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TagSkipAssert.cs @@ -0,0 +1,60 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// Shared assertion for an S7 tag shape the driver cannot serve. It must be refused PER TAG at +/// every gate, never fault the whole driver: the mapper (and so the AdminUI editor) refuses it with +/// the reason, the deploy gate () warns, and a driver +/// carrying it next to a good tag initializes Healthy, reports the bad one through +/// , and still serves the good one (the positive control). +/// +internal static class S7TagSkipAssert +{ + private const string GoodName = "Good"; + + /// Asserts is refused per tag at the mapper, the deploy gate and the driver. + /// The unservable tag. + /// Fragments the operator-facing rejection reason must contain. + /// A task that completes when every gate has been checked. + public static async Task RefusedPerTagAsync(S7TagDefinition bad, params string[] reasonFragments) + { + var ct = TestContext.Current.CancellationToken; + var blob = S7TagDefinitionFactory.ToTagConfig(bad); + + // Mapper (and therefore the AdminUI editor, which asks it) — refused, with the reason. + S7TagDefinitionFactory.FromTagConfig(blob, bad.Name, out _, out var reason).ShouldBeFalse(); + reason.ShouldNotBeNull(); + foreach (var f in reasonFragments) reason.ShouldContain(f, Case.Insensitive); + + // Deploy gate — warns with the same reason. + var warnings = S7TagDefinitionFactory.Inspect(blob); + warnings.ShouldHaveSingleItem().ShouldContain(reason); + + // Driver — initializes Healthy with the bad tag skipped and the good tag served. + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(bad, new S7TagDefinition(GoodName, "DB1.DBW0", S7DataType.UInt16, Writable: false)), + }; + using var drv = new S7Driver(opts, "s7-skip-" + bad.Name, new ScriptedS7PlcFactory()); + await drv.InitializeAsync("{}", ct); + + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "one unservable tag must not fault the driver"); + var skipped = drv.GetSkippedTags().ShouldNotBeNull(); + skipped.Sample.ShouldHaveSingleItem().RawPath.ShouldBe(bad.Name); + skipped.Sample[0].Reason.ShouldBe(reason); + + var builder = new RecordingAddressSpaceBuilder(); + await drv.DiscoverAsync(builder, ct); + builder.Variables.Select(v => v.Name).ShouldBe([GoodName]); + + var results = await drv.ReadAsync([bad.Name, GoodName], ct); + results[0].StatusCode.ShouldBe(0x80340000u, "the skipped tag reads BadNodeIdUnknown"); + results[1].StatusCode.ShouldBe(0u, "the good neighbour is still served"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs index 506d5d67..95bba0b3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ValueCodecTests.cs @@ -318,16 +318,9 @@ public sealed class S7ValueCodecTests [Fact] public async Task New_types_declared_as_arrays_or_off_byte_addresses_fail_init() { - S7DriverOptions With(S7TagDefinition t) => new() - { - Host = "192.0.2.1", - Probe = new S7ProbeOptions { Enabled = false }, - RawTags = S7RawTags.Entries(t), - }; - using var arr = new S7Driver(With(Tag(S7DataType.Dtl) with { ArrayCount = 2 }), "a", new ConnectingFakeS7PlcFactory()); - await Should.ThrowAsync(() => arr.InitializeAsync("{}", TestContext.Current.CancellationToken)); - using var word = new S7Driver(With(Tag(S7DataType.Tod, address: "DB1.DBW0")), "b", new ConnectingFakeS7PlcFactory()); - await Should.ThrowAsync(() => word.InitializeAsync("{}", TestContext.Current.CancellationToken)); + // Refused PER TAG at every gate (they used to throw at init and fault the whole driver). + await S7TagSkipAssert.RefusedPerTagAsync(Tag(S7DataType.Dtl) with { Name = "DtlArr", ArrayCount = 2 }, "Dtl", "array"); + await S7TagSkipAssert.RefusedPerTagAsync(Tag(S7DataType.Tod, address: "DB1.DBW0") with { Name = "TodWord" }, "Tod", "byte-addressed"); } // ---- TagConfig ---- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs new file mode 100644 index 00000000..25d74961 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/ScriptedS7Plc.cs @@ -0,0 +1,142 @@ +using S7DataItem = global::S7.Net.Types.DataItem; +using S7NetDataType = global::S7.Net.DataType; +using VarType = global::S7.Net.VarType; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// A scriptable : every connection it creates delegates its wire +/// operations to the factory's handlers, so a test scripts the device once and observes every +/// connection (including reopened ones). Counts wire operations so a test can prove what did +/// NOT go to the wire without timing anything. +/// +internal sealed class ScriptedS7PlcFactory : IS7PlcFactory +{ + private int _wireOps; + + /// Every connection created, in order. + public List Created { get; } = []; + + /// Wire operations issued (reads, multi-var reads, writes; not opens or status probes). + public int WireOps => Volatile.Read(ref _wireOps); + + /// Hook run inside every ; the default opens immediately. + public Func Open { get; set; } = (_, _) => Task.CompletedTask; + + /// Per-address string read; the default returns (ushort)42. + public Func> Read { get; set; } = (_, _) => Task.FromResult((ushort)42); + + /// Block read; the default returns count zero bytes. + public Func> ReadBytes { get; set; } + = (_, _, _, count, _) => Task.FromResult(new byte[count]); + + /// Multi-var read; the default answers every item with a zero of its var type. + public Func, CancellationToken, Task>> ReadMulti { get; set; } + = (items, _) => + { + foreach (var i in items) + i.Value = i.VarType switch + { + VarType.Bit => false, + VarType.Byte => (byte)0, + VarType.Word => (ushort)0, + _ => (uint)0, + }; + return Task.FromResult>(items); + }; + + /// Per-address string write; the default succeeds. + public Func Write { get; set; } = (_, _, _) => Task.CompletedTask; + + /// Block write; the default succeeds. + public Func WriteBytes { get; set; } + = (_, _, _, _, _) => Task.CompletedTask; + + /// CPU-status probe; the default answers. + public Func ReadStatus { get; set; } = _ => Task.CompletedTask; + + /// + public IS7Plc Create(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout) + { + lock (Created) + { + var plc = new ScriptedS7Plc(this, Created.Count); + Created.Add(plc); + return plc; + } + } + + /// Counts one wire operation. + internal void CountWireOp() => Interlocked.Increment(ref _wireOps); +} + +/// One connection created by . +internal sealed class ScriptedS7Plc(ScriptedS7PlcFactory factory, int index) : IS7Plc +{ + /// Zero-based creation index. + public int Index { get; } = index; + + /// + public bool IsConnected { get; private set; } + + /// True once disposed. + public bool Disposed { get; private set; } + + /// + public int MaxPduSize => 240; + + /// + public async Task OpenAsync(CancellationToken cancellationToken) + { + await factory.Open(Index, cancellationToken).ConfigureAwait(false); + IsConnected = true; + } + + /// + public Task ReadAsync(string address, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.Read(address, cancellationToken); + } + + /// + public Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.ReadBytes(area, db, startByteAdr, count, cancellationToken); + } + + /// + public Task> ReadMultipleVarsAsync(List items, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.ReadMulti(items, cancellationToken); + } + + /// + public Task WriteAsync(string address, object value, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.Write(address, value, cancellationToken); + } + + /// + public Task WriteBytesAsync(S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken) + { + factory.CountWireOp(); + return factory.WriteBytes(area, db, startByteAdr, value, cancellationToken); + } + + /// + public Task ReadStatusAsync(CancellationToken cancellationToken) => factory.ReadStatus(cancellationToken); + + /// + public void Close() => IsConnected = false; + + /// + public void Dispose() + { + Disposed = true; + IsConnected = false; + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs index 337c6a34..112f1287 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/CsvMergeEvidenceTests.cs @@ -142,7 +142,7 @@ public sealed class CsvMergeEvidenceTests var row = SingleRow( DriverTypeNames.S7, "Name,DataType,Address,S7DataType,TagConfigJson", - "t1,Int16,DB1.DBW0,Int32,\"{\"\"pollHint\"\":\"\"fast\"\"}\""); + "t1,Int16,DB1.DBD0,Int32,\"{\"\"pollHint\"\":\"\"fast\"\"}\""); var blob = Blob(row); blob["dataType"]!.GetValue().ShouldBe("Int32"); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs index 65930cf0..17703c6b 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs @@ -346,7 +346,11 @@ public sealed class RawTagConfigGateTests public void A_browse_committed_config_passes_the_gate(string driverType) { var tagConfig = RawBrowseCommitMapper.BuildTagConfig( - driverType, driverType == DriverTypeNames.AbLegacy ? "N7:0" : "SomeAddress", BrowseFieldsFor(driverType)); + driverType, + // Drivers whose address grammar is validated by their reader need a real address: AbLegacy's + // file grammar, and S7's (S7TagShapeRules — an unparseable or wrong-width address is refused). + driverType == DriverTypeNames.AbLegacy ? "N7:0" : driverType == DriverTypeNames.S7 ? "DB1.DBW0" : "SomeAddress", + BrowseFieldsFor(driverType)); TagConfigValidator.Validate(driverType, tagConfig).ShouldBeNull( $"browse-commit for {driverType} must not author a config the import refuses"); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs index dbd8e61b..12cb121c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/S7TagConfigModelTests.cs @@ -98,13 +98,13 @@ public sealed class S7TagConfigModelTests /// driver serves. (An absent key is deliberately not asserted: S7 alone hard-requires the key, an /// asymmetry that predates this change and that its own Inspect does not report either.) [Theory] - [InlineData("""{"address":"DB1.DBW0","dataType":"float32"}""")] - [InlineData("""{"address":"DB1.DBW0","dataType":"FLOAT32"}""")] + [InlineData("""{"address":"DB1.DBD0","dataType":"float32"}""")] + [InlineData("""{"address":"DB1.DBD0","dataType":"FLOAT32"}""")] [InlineData("""{"address":"DB1.DBW0","dataType":7}""")] public void Validate_accepts_every_dataType_the_driver_accepts(string json) => S7TagConfigModel.FromJson(json).Validate().ShouldBeNull(); - // ---- S7-016: array shape vs the driver's init guards (which fail the WHOLE driver init) ---- + // ---- S7-016: array shape vs S7TagShapeRules (refused per tag at every gate) ---- [Theory] [InlineData("Int64")] diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs index 019498e4..d71a00e0 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs @@ -160,9 +160,9 @@ public sealed class TagConfigNonEnumParityTests /// S7's remaining non-enum refusal: a String tag whose stringLength falls outside /// 0–254. The reader REJECTS it (it does not clamp), so the tag would never resolve. [Theory] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":255}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":300}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":-1}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":255}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":300}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":-1}""")] public void S7_refuses_a_string_length_its_reader_refuses(string blob) { RuntimeAccepts(DriverTypeNames.S7, blob).ShouldBeFalse( @@ -174,9 +174,9 @@ public sealed class TagConfigNonEnumParityTests /// stops this becoming an over-rejection — an out-of-range stringLength on a tag that is not a /// String at all, which the reader ignores entirely. [Theory] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":10}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":254}""")] - [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":0}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":10}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":254}""")] + [InlineData("""{"address":"DB1.DBB0","dataType":"String","stringLength":0}""")] [InlineData("""{"address":"DB1.DBW0","dataType":"Int16","stringLength":300}""")] public void S7_accepts_a_string_length_its_reader_accepts(string blob) { @@ -360,8 +360,8 @@ public sealed class TagConfigNonEnumParityTests (DriverTypeNames.S7, """{"address":"DB1.DBW0","dataType":"Int16"}"""), (DriverTypeNames.S7, """{"address":"DB1.DBW0"}"""), (DriverTypeNames.S7, """{"address":" ","dataType":"Int16"}"""), - (DriverTypeNames.S7, """{"address":"DB1.DBS0","dataType":"String","stringLength":300}"""), - (DriverTypeNames.S7, """{"address":"DB1.DBS0","dataType":"String","stringLength":24}"""), + (DriverTypeNames.S7, """{"address":"DB1.DBB0","dataType":"String","stringLength":300}"""), + (DriverTypeNames.S7, """{"address":"DB1.DBB0","dataType":"String","stringLength":24}"""), (DriverTypeNames.AbCip, "{}"), (DriverTypeNames.AbCip, """{"tagPath":"Program:Main.Speed"}"""), @@ -576,7 +576,7 @@ public sealed class TagConfigNonEnumParityTests data.Add(DriverTypeNames.Modbus, new ModbusTagConfigModel { Address = 1 }.ToJson()); data.Add(DriverTypeNames.S7, new S7TagConfigModel { Address = "DB1.DBW0" }.ToJson()); data.Add(DriverTypeNames.S7, - new S7TagConfigModel { Address = "DB1.DBS0", DataType = S7DataType.String, StringLength = 24 }.ToJson()); + new S7TagConfigModel { Address = "DB1.DBB0", DataType = S7DataType.String, StringLength = 24 }.ToJson()); data.Add(DriverTypeNames.AbCip, new AbCipTagConfigModel { TagPath = "Program:Main.Speed" }.ToJson()); data.Add(DriverTypeNames.AbLegacy, new AbLegacyTagConfigModel { Address = "N7:0" }.ToJson()); data.Add(DriverTypeNames.TwinCAT, new TwinCATTagConfigModel { SymbolPath = "MAIN.bStart" }.ToJson()); From ecf1e864641a24f89b6230ae1f72bb862ec01e97 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:10:21 -0400 Subject: [PATCH 2/7] fix(s7): lifecycle gate + generation-stamped health and publishes (R07:H5) Overlapping InitializeAsync calls (the actor's fire-and-forget init plus a retry tick) could leak an opened PLC connection and orphan a probe loop: no gate guarded the lifecycle, only _gate for I/O. - _lifecycleGate (the Modbus pattern) serialises Initialize / Reinitialize / Shutdown / Dispose; teardown waits a bounded time, then tears down anyway. - Every initialize and teardown starts a lifecycle generation. Health writes (init, read/write passes, poll errors, shutdown) and the init publish carry it and are dropped once it has moved on, so an abandoned init never publishes over a later shutdown and a shutdown's final write never clobbers a newer init. A superseded init disposes what it opened and throws. - The init publish (options, tag tables, Plc, probe loop) happens under _gate; the tag tables are built fresh and swapped by one reference write instead of being cleared in place under live poll passes. - Read/write passes re-check initialization under _gate, so a pass that waited out a teardown cannot reopen a connection on a shut-down driver. - The semaphores are no longer disposed: Dispose can race a running shutdown. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 439 +++++++++++++----- .../S7LifecycleGateTests.cs | 143 ++++++ 2 files changed, 467 insertions(+), 115 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 347a0b19..acade74e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -59,7 +59,7 @@ public sealed class S7Driver _plcFactory = plcFactory ?? new S7PlcFactory(); _logger = logger ?? NullLogger.Instance; _resolver = new EquipmentTagRefResolver( - r => _tagsByRawPath.TryGetValue(r, out var t) ? t : null); + r => TagsByRawPath.TryGetValue(r, out var t) ? t : null); // The polling overlay runs on the shared PollGroupEngine (structural array diff + // drain-before-dispose + capped-exponential failure backoff + caller-token-filtered OCE), // retiring the bespoke S7 poll fork. minInterval 100 ms matches the S7 scan-mailbox floor; @@ -128,13 +128,6 @@ public sealed class S7Driver /// OPC UA StatusCode used for a genuine device fault (CPU error, hardware fault). private const uint StatusBadDeviceFailure = 0x808B0000u; - // v3: the RawPath → definition table. Keyed by RawPath (the v3 tag identity, == def.Name), - // case-sensitive ordinal. Built in InitializeAsync from _options.RawTags via the pure factory. - private readonly Dictionary _tagsByRawPath = new(StringComparer.Ordinal); - // Pre-parsed S7 address per tag, keyed by RawPath (== def.Name). Populated alongside - // _tagsByRawPath at init so a config typo fails fast here rather than as a per-read fault. - private readonly Dictionary _parsedByRawPath = new(StringComparer.Ordinal); - // Resolves a read/write/subscribe fullReference (a RawPath in v3) to a tag definition by a single // authored-table hit; a miss is a miss (the driver surfaces BadNodeIdUnknown, never a parse attempt). private readonly EquipmentTagRefResolver _resolver; @@ -225,19 +218,50 @@ public sealed class S7Driver /// public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { - // InitializeAsync is reachable on a live driver with no teardown first (the IDriver contract - // allows it, and a reconnect retry can overlap a running attempt). Stop the previous probe loop and drop the previous connection - // before they are overwritten below. Deliberately NOT ShutdownAsync: that also drops _poll and the - // subscriptions, which are owned by the shutdown path (the actor re-subscribes after every successful init). - await ReleaseConnectionAsync().ConfigureAwait(false); - Health = new DriverHealth(DriverState.Initializing, null, null); + // Serialised against every other lifecycle operation: DriverInstanceActor dispatches initialize + // on a fire-and-forget Task.Run and a reconnect retry can start a second attempt while the + // first is still inside OpenAsync. Without the gate attempt A's freshly opened Plc and probe + // loop were overwritten by attempt B and never released (R07:H5) — the Modbus pattern. + await _lifecycleGate.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + await InitializeCoreAsync(driverConfigJson, cancellationToken).ConfigureAwait(false); + } + finally + { + _lifecycleGate.Release(); + } + } + + /// The lifecycle-gated body of . + /// The driver config. + /// Caller cancellation. + /// A task that completes when the attempt has published, been superseded, or failed. + private async Task InitializeCoreAsync(string driverConfigJson, CancellationToken cancellationToken) + { + // This attempt's generation. Every health write and table/connection publish below is stamped + // with it and dropped once the generation has moved on: an attempt abandoned by its caller can + // still be running when a ShutdownAsync (which only waits a bounded time for the lifecycle + // gate) starts, and it must never publish over it. + var gen = BeginGeneration(); + + // InitializeAsync is reachable on a live driver with no teardown first (the IDriver contract + // allows it). Stop the previous probe loop and drop the previous connection before they are + // replaced below. Deliberately NOT a full teardown: that also drops _poll and the subscriptions, + // which are owned by the shutdown path (the actor re-subscribes after every successful init). + await ReleaseConnectionAsync(gen).ConfigureAwait(false); + TryWriteHealth(gen, new DriverHealth(DriverState.Initializing, null, null)); + + // Re-parse the supplied DriverConfig JSON so a config change delivered through the IDriver + // contract is honoured. An empty / placeholder document (e.g. the "{}" some unit tests pass) + // keeps the constructor-supplied options. Local until published: a superseded attempt must + // not leave its options behind for the live connection to reopen against. + var options = _options; + IS7Plc? plc = null; try { - // Re-parse the supplied DriverConfig JSON so a config change delivered through the - // IDriver contract is honoured. An empty / placeholder document - // (e.g. the "{}" some unit tests pass) keeps the constructor-supplied options. if (HasConfigBody(driverConfigJson)) - _options = S7DriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); + options = S7DriverFactoryExtensions.ParseOptions(_driverInstanceId, driverConfigJson); // v3: build the RawPath → definition table from the authored raw tags. Each entry's TagConfig // blob is mapped by the pure factory, which also applies S7TagShapeRules (the address must @@ -246,36 +270,32 @@ public sealed class S7Driver // through ISkippedTagReporter, reads BadNodeIdUnknown), never thrown: these used to be init // guards that faulted every tag on the PLC and made the actor retry the same init forever. // The entry's WriteIdempotent flag is threaded onto the def (it lives on the RawTagEntry). - _tagsByRawPath.Clear(); - _parsedByRawPath.Clear(); - _resolver.Clear(); // no-op in v3 (the resolver reads the live table by closure); kept for symmetry - _skippedTags.BeginRebuild(_options.RawTags.Count); - foreach (var entry in _options.RawTags) + // Built into fresh dictionaries and published by ONE reference swap — a retained poll pass + // reading the old table never sees it half-cleared. + var tags = new Dictionary(StringComparer.Ordinal); + var parsed = new Dictionary(StringComparer.Ordinal); + var skipped = new List<(string RawPath, string Reason)>(); + foreach (var entry in options.RawTags) { if (S7TagDefinitionFactory.FromTagConfig(entry.TagConfig, entry.RawPath, out var def, out var shapeRejection) && S7AddressParser.TryParse(def.Address, out var parsedAddress)) { - _tagsByRawPath[entry.RawPath] = def with { WriteIdempotent = entry.WriteIdempotent }; - _parsedByRawPath[entry.RawPath] = parsedAddress; + tags[entry.RawPath] = def with { WriteIdempotent = entry.WriteIdempotent }; + parsed[entry.RawPath] = parsedAddress; } else { - var reason = shapeRejection ?? "TagConfig did not map to a definition"; - _skippedTags.Skip(entry.RawPath, reason); - _logger.LogWarning( - "S7 tag skipped: {Reason} Driver={DriverInstanceId} RawPath={RawPath}", - reason, _driverInstanceId, entry.RawPath); + skipped.Add((entry.RawPath, shapeRejection ?? "TagConfig did not map to a definition")); } } - _skippedTags.Commit(); // Build the connection through the factory (the same seam EnsureConnectedAsync's reopen // uses) so the read/write timeouts are baked in identically on connect and reconnect. - var plc = _plcFactory.Create(_options); + plc = _plcFactory.Create(options); try { using var cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - cts.CancelAfter(_options.Timeout); + cts.CancelAfter(options.Timeout); await plc.OpenAsync(cts.Token).ConfigureAwait(false); } catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) @@ -283,88 +303,157 @@ public sealed class S7Driver // The linked CancelAfter fired — a connect TIMEOUT, not a caller cancellation. // Surface it as a TimeoutException so init-time and reconnect-time timeouts report // the same exception type (for consistency); the outer catch still faults health. - try { plc.Dispose(); } catch { } throw new TimeoutException( - $"S7 connect to {_options.Host}:{_options.Port} timed out after " + - $"{(int)_options.Timeout.TotalMilliseconds} ms."); + $"S7 connect to {options.Host}:{options.Port} timed out after " + + $"{(int)options.Timeout.TotalMilliseconds} ms."); } - catch + + // Publish everything this attempt built under _gate (no pass can be mid-flight against a + // half-published state) and only while this attempt is still the current generation. + await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); + try { - // Dispose the partially-constructed connection so a failed init doesn't leak it - // before the outer catch nulls Plc (Plc isn't assigned yet, so it can't). - try { plc.Dispose(); } catch { } - throw; + if (!IsCurrentGeneration(gen)) + { + // The outer finally disposes the unpublished plc; the catch writes no health (the + // generation moved on). Thrown, not returned: a caller must never read "initialized" + // into an attempt that published nothing. + throw new InvalidOperationException( + "S7 initialize was superseded by a newer lifecycle operation before it published; " + + "its connection was discarded."); + } + + _options = options; + Volatile.Write(ref _tables, new TagTables(tags, parsed)); + _skippedTags.BeginRebuild(options.RawTags.Count); + foreach (var (rawPath, reason) in skipped) + { + _skippedTags.Skip(rawPath, reason); + _logger.LogWarning( + "S7 tag skipped: {Reason} Driver={DriverInstanceId} RawPath={RawPath}", + reason, _driverInstanceId, rawPath); + } + _skippedTags.Commit(); + + Plc = plc; + plc = null; // published — no longer this method's to dispose + _plcDead = false; + _connectBackoff.RecordSuccess(); + Volatile.Write(ref _negotiatedPduSize, Plc.MaxPduSize); + _initialized = true; + TryWriteHealth(gen, new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); + + // Kick off the probe loop once the connection is up. Initial HostState stays Unknown + // until the first probe tick succeeds — avoids broadcasting a premature Running + // transition before any PDU round-trip has happened. Tracked (not fire-and-forget) so + // a teardown can await it before disposing its CTS; Task.Run gets None so the handle + // is always awaitable — the loop's own token check handles cancellation. + if (options.Probe.Enabled) + { + var probeCts = new CancellationTokenSource(); + _probeCts = probeCts; + _probeTask = Task.Run(() => ProbeLoopAsync(probeCts.Token), CancellationToken.None); + } + } + finally + { + _gate.Release(); } - Plc = plc; - _plcDead = false; - _connectBackoff.RecordSuccess(); - Volatile.Write(ref _negotiatedPduSize, plc.MaxPduSize); - - _initialized = true; - Health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); _logger.LogInformation("S7Driver connected. Driver={DriverInstanceId} Host={Host} CPU={CpuType} PduSize={PduSize} Tags={TagCount}", - _driverInstanceId, _options.Host, _options.CpuType, _negotiatedPduSize, _tagsByRawPath.Count); - - // Kick off the probe loop once the connection is up. Initial HostState stays - // Unknown until the first probe tick succeeds — avoids broadcasting a premature - // Running transition before any PDU round-trip has happened. - if (_options.Probe.Enabled) - { - _probeCts = new CancellationTokenSource(); - // Track the probe Task (not fire-and-forget) so ShutdownAsync can await it - // before disposing _gate / _probeCts. Pass None to Task.Run so - // the delegate always runs and the handle is always awaitable; the loop's own - // token check handles cancellation. - _probeTask = Task.Run(() => ProbeLoopAsync(_probeCts.Token), CancellationToken.None); - } + _driverInstanceId, options.Host, options.CpuType, _negotiatedPduSize, tags.Count); } catch (Exception ex) { - // Clean up a partially-constructed Plc so a retry from the caller doesn't leak - // the TcpClient. Dispose is best-effort and idempotent. - try { Plc?.Dispose(); } catch { } - Plc = null; - Volatile.Write(ref _negotiatedPduSize, 0); - _initialized = false; - _plcDead = false; - Health = new DriverHealth(DriverState.Faulted, null, ex.Message); - _logger.LogError(ex, "S7Driver connect failed. Driver={DriverInstanceId} Host={Host}", _driverInstanceId, _options.Host); + // A superseded attempt reports nothing: the generation that replaced it owns health. + if (TryWriteHealth(gen, new DriverHealth(DriverState.Faulted, null, ex.Message))) + { + _options = options; + _logger.LogError(ex, "S7Driver connect failed. Driver={DriverInstanceId} Host={Host}", _driverInstanceId, options.Host); + } throw; } + finally + { + // A connection this attempt opened but did not publish (failed or superseded) is released + // here, so a retry from the caller never leaks the TcpClient. + if (plc is not null) { try { plc.Dispose(); } catch { /* best-effort */ } } + } } /// public async Task ReinitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { - // InitializeAsync re-parses driverConfigJson, so a config change delivered here is + // Holds the lifecycle gate across BOTH halves so no other lifecycle operation slips between + // them. InitializeCoreAsync re-parses driverConfigJson, so a config change delivered here is // applied in place rather than silently discarded. - await ShutdownAsync(cancellationToken).ConfigureAwait(false); - await InitializeAsync(driverConfigJson, cancellationToken).ConfigureAwait(false); + await _lifecycleGate.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + await ShutdownCoreAsync().ConfigureAwait(false); + await InitializeCoreAsync(driverConfigJson, cancellationToken).ConfigureAwait(false); + } + finally + { + _lifecycleGate.Release(); + } } /// public async Task ShutdownAsync(CancellationToken cancellationToken) { - // Signal cancellation to the probe + poll loops first, collect their Task handles, - // then await all of them with a bounded timeout BEFORE disposing the shared semaphore - // and CTS objects. Without the drain, a loop iteration mid-_gate would call Release() - // on (or WaitAsync against) a disposed semaphore. + // Bounded wait for the lifecycle gate: a wedged initialize (a connect that never returns, or an + // attempt its caller abandoned) must not turn shutdown into a hang. Past the bound we tear down + // anyway — the generation bump makes the still-running attempt discard instead of publish. + var gated = false; + try { gated = await _lifecycleGate.WaitAsync(LifecycleGateTimeout, cancellationToken).ConfigureAwait(false); } + catch (OperationCanceledException) { /* the caller stopped waiting for the gate — still tear down */ } + if (!gated) + _logger.LogWarning( + "S7 lifecycle gate still held at shutdown; tearing down anyway. Driver={DriverInstanceId}", + _driverInstanceId); + try + { + await ShutdownCoreAsync().ConfigureAwait(false); + } + finally + { + if (gated) _lifecycleGate.Release(); + } + } + + /// + /// The complete teardown: the poll engine (every subscription), the probe loop, the connection, + /// the sticky write fault and the per-generation memos. It is its own generation, so its final + /// health write can never clobber an initialize that started after it. + /// + /// A task that completes once everything is released. + private async Task ShutdownCoreAsync() + { + var gen = BeginGeneration(); // Drain the polling overlay first — the engine cancels each loop and awaits it before - // disposing its CTS (drain-before-dispose), so no poll iteration touches _gate - // after this returns. - await _poll.DisposeAsync().ConfigureAwait(false); - await ReleaseConnectionAsync().ConfigureAwait(false); - _writeFault = null; - _connectBackoff.RecordSuccess(); - Health = new DriverHealth(DriverState.Unknown, Health.LastSuccessfulRead, null); + // disposing its CTS (drain-before-dispose), so no poll iteration touches _gate after this. + try { await _poll.DisposeAsync().ConfigureAwait(false); } + catch (Exception ex) { _logger.LogWarning(ex, "S7 poll engine drain failed. Driver={DriverInstanceId}", _driverInstanceId); } + await ReleaseConnectionAsync(gen).ConfigureAwait(false); + lock (_healthLock) + { + if (_generation != gen) return; // a newer initialize owns the state now + _writeFault = null; + _connectBackoff.RecordSuccess(); + Health = new DriverHealth(DriverState.Unknown, Health.LastSuccessfulRead, null); + } } /// /// Cancels + bounded-joins the probe loop, then disposes its CTS and the PLC connection. The - /// connection part of (everything but the poll engine and health). + /// connection part of a teardown (everything but the poll engine and health). Releases only + /// while is still current — a teardown that ran past the lifecycle gate + /// must not dispose a connection a NEWER initialize already published. /// - private async Task ReleaseConnectionAsync() + /// The generation of the lifecycle operation doing the release. + /// A task that completes once the prior state is released. + private async Task ReleaseConnectionAsync(long gen) { var probeCts = _probeCts; var probeTask = _probeTask; @@ -379,11 +468,6 @@ public sealed class S7Driver catch { /* loop faults are already surfaced via health; teardown continues */ } } - // The probe loop has now observed cancellation and released _gate — safe to dispose the CTS. - probeCts?.Dispose(); - _probeCts = null; - _probeTask = null; - // Swap the connection out under _gate so a Read/Write/poll pass that already holds the gate (they // re-read Plc inside it) can never see a disposed Plc mid-swap. Bounded: a wedged holder must not hang // teardown, in which case we proceed exactly as the un-gated path always did. @@ -395,6 +479,15 @@ public sealed class S7Driver _driverInstanceId, DrainTimeout); try { + if (!IsCurrentGeneration(gen)) return; // superseded: the newer generation released (or owns) this state + + // The probe loop has now observed cancellation and released _gate — safe to dispose the CTS. + if (ReferenceEquals(_probeCts, probeCts)) + { + probeCts?.Dispose(); + _probeCts = null; + _probeTask = null; + } _initialized = false; _plcDead = false; try { Plc?.Dispose(); } catch { /* best-effort — tearing down anyway */ } @@ -407,6 +500,101 @@ public sealed class S7Driver } } + // ---- lifecycle generation ---- + + /// + /// Guards / / + /// / against each other (R07:H5, the + /// Modbus _lifecycleGate pattern). Distinct from , which serialises I/O. + /// Never disposed: a Dispose racing a still-running ShutdownAsync must not make it throw. + /// + private readonly SemaphoreSlim _lifecycleGate = new(1, 1); + + /// + /// Bounded wait the teardown paths take on before tearing down anyway. + /// Internal so a test can shorten it instead of waiting the production 10 s. + /// + internal TimeSpan LifecycleGateTimeout { get; set; } = TimeSpan.FromSeconds(10); + + /// Serialises generation bumps against generation-stamped health writes. + private readonly object _healthLock = new(); + + /// + /// Lifecycle generation: bumped by every initialize and teardown. Health writes and + /// table/connection publishes carry the generation they were computed under and are dropped + /// once it has moved on. Written under . + /// + private long _generation; + + /// Starts a new lifecycle generation. + /// The new generation. + private long BeginGeneration() + { + lock (_healthLock) return ++_generation; + } + + /// The current lifecycle generation. + private long CurrentGeneration => Interlocked.Read(ref _generation); + + /// True while is still the current lifecycle generation. + /// The generation to test. + /// when nothing has superseded it. + private bool IsCurrentGeneration(long gen) => CurrentGeneration == gen; + + /// Writes only while is current. + /// The generation the health was computed under. + /// The health to publish. + /// when written. + private bool TryWriteHealth(long gen, DriverHealth health) + { + lock (_healthLock) + { + if (_generation != gen) return false; + Health = health; + return true; + } + } + + /// + /// Derives and writes a new health from the current one, atomically, only while + /// is current — for the passes that preserve LastSuccessfulRead. + /// + /// The generation the update was computed under. + /// Maps the current health to the new one; returning null writes nothing. + /// when written. + private bool TryUpdateHealth(long gen, Func update) + { + lock (_healthLock) + { + if (_generation != gen) return false; + if (update(Health) is not { } next) return false; + Health = next; + return true; + } + } + + /// The published RawPath → definition and RawPath → parsed-address tables, swapped as one. + /// RawPath → definition. + /// RawPath → pre-parsed address. + private sealed record TagTables( + IReadOnlyDictionary Tags, + IReadOnlyDictionary Parsed) + { + /// The empty tables a driver starts with. + public static readonly TagTables Empty = new( + new Dictionary(StringComparer.Ordinal), + new Dictionary(StringComparer.Ordinal)); + } + + /// The live tag tables; replaced by reference at init, never mutated in place. + private TagTables _tables = TagTables.Empty; + + /// The live RawPath → definition table. + private IReadOnlyDictionary TagsByRawPath => Volatile.Read(ref _tables).Tags; + + /// The live RawPath → pre-parsed address table. + private IReadOnlyDictionary ParsedByRawPath => Volatile.Read(ref _tables).Parsed; + /// public DriverHealth GetHealth() => Health; @@ -488,8 +676,15 @@ public sealed class S7Driver var outcome = new ReadPassOutcome(); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); + // The lifecycle generation this pass runs in — its health write is dropped if a teardown or + // re-initialize has moved on by the time it lands. Captured under _gate, where the init + // publish happens, and re-checked against _initialized: a pass that passed RequireInitialized + // and then waited out a teardown must not reopen a connection on a shut-down driver. + var gen = CurrentGeneration; try { + RequireInitialized(); + // (Re)open the connection under the gate. A failed (re)connect degrades the whole // batch to BadCommunicationError rather than throwing — the poll loop / next read // retries, and EnsureConnectedAsync repairs on the following call. @@ -506,7 +701,7 @@ public sealed class S7Driver { for (var i = 0; i < fullReferences.Count; i++) results[i] = new DataValueSnapshot(null, StatusBadCommunicationError, null, now); - Health = new DriverHealth(DriverState.Degraded, Health.LastSuccessfulRead, ex.Message); + TryUpdateHealth(gen, h => new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, ex.Message)); return results; } @@ -635,10 +830,10 @@ public sealed class S7Driver state = wf.State; error = wf.Message; } - Health = new DriverHealth( + TryUpdateHealth(gen, h => new DriverHealth( state, - outcome.AnySuccess ? now : Health.LastSuccessfulRead, - error); + outcome.AnySuccess ? now : h.LastSuccessfulRead, + error)); } _gate.Release(); @@ -1068,7 +1263,7 @@ public sealed class S7Driver /// The parsed address, valid only when this returns true. /// true when the address is known or parses. private bool TryGetParsedAddress(S7TagDefinition tag, out S7ParsedAddress addr) - => _parsedByRawPath.TryGetValue(tag.Name, out addr) || S7AddressParser.TryParse(tag.Address, out addr); + => ParsedByRawPath.TryGetValue(tag.Name, out addr) || S7AddressParser.TryParse(tag.Address, out addr); /// /// The read-failure classification ladder, shared by the per-tag path and the span path so @@ -1137,10 +1332,10 @@ public sealed class S7Driver private async Task ReadOneAsync(IS7Plc plc, S7TagDefinition tag, CancellationToken ct) { - // Every tag pre-parses its address at init (_parsedByRawPath); the on-demand + // Every tag pre-parses its address at init (ParsedByRawPath); the on-demand // S7AddressParser.Parse is a defensive fallback for a tag not in that table. Parse throws // FormatException on a bad address, which the caller's catch maps to BadCommunicationError. - var addr = _parsedByRawPath.TryGetValue(tag.Name, out var parsed) + var addr = ParsedByRawPath.TryGetValue(tag.Name, out var parsed) ? parsed : S7AddressParser.Parse(tag.Address); @@ -1691,8 +1886,11 @@ public sealed class S7Driver var outcome = new WritePassOutcome(); await _gate.WaitAsync(cancellationToken).ConfigureAwait(false); + var gen = CurrentGeneration; // see ReadAsync try { + RequireInitialized(); + // (Re)open under the gate; a failed (re)connect degrades every write to // BadCommunicationError rather than throwing. IS7Plc plc; @@ -1733,7 +1931,7 @@ public sealed class S7Driver // Timer/Counter, but the mapper doesn't run that guard). // TryParse (not Parse) so a malformed address falls through to the existing // error path rather than throwing here. - if (_parsedByRawPath.TryGetValue(tag.Name, out var tcParsed) + if (ParsedByRawPath.TryGetValue(tag.Name, out var tcParsed) ? tcParsed.Area is S7Area.Timer or S7Area.Counter : (S7AddressParser.TryParse(tag.Address, out tcParsed) && tcParsed.Area is S7Area.Timer or S7Area.Counter)) { @@ -1806,7 +2004,7 @@ public sealed class S7Driver { // THE single health write of the batch (a finally, so an OCE mid-batch still flushes the // evidence gathered so far — a pass that observed nothing writes nothing). - ApplyWritePassHealth(outcome); + ApplyWritePassHealth(gen, outcome); _gate.Release(); } return results; @@ -1818,10 +2016,11 @@ public sealed class S7Driver /// successful sibling. Maintains , the sticky Faulted a read pass /// must not erase. Caller holds . /// + /// The lifecycle generation the batch ran in; a superseded batch writes nothing. /// The batch's accumulated outcomes. - private void ApplyWritePassHealth(WritePassOutcome outcome) + private void ApplyWritePassHealth(long gen, WritePassOutcome outcome) { - if (!outcome.HasObservations) return; + if (!outcome.HasObservations || !IsCurrentGeneration(gen)) return; var worst = outcome.WorstFailure; var priorWriteFault = _writeFault; @@ -1836,9 +2035,10 @@ public sealed class S7Driver // side) were holding — a clean write is the only evidence the alert has cleared. // Only a Faulted the write side owns: a read-side Faulted (e.g. access denied on another DB) is the // read pass's to clear, and Health still carries the write fault's own message iff it is ours. - if (outcome.AnySuccess && _writeFault is null && priorWriteFault is { } owned - && Health.State == DriverState.Faulted && Health.LastError == owned.Message) - Health = new DriverHealth(DriverState.Healthy, Health.LastSuccessfulRead, null); + if (outcome.AnySuccess && _writeFault is null && priorWriteFault is { } owned) + TryUpdateHealth(gen, h => h.State == DriverState.Faulted && h.LastError == owned.Message + ? new DriverHealth(DriverState.Healthy, h.LastSuccessfulRead, null) + : null); return; } @@ -1850,7 +2050,7 @@ public sealed class S7Driver state = wf.State; error = wf.Message; } - Health = new DriverHealth(state, Health.LastSuccessfulRead, error); + TryUpdateHealth(gen, h => new DriverHealth(state, h.LastSuccessfulRead, error)); } /// @@ -1881,10 +2081,10 @@ public sealed class S7Driver private async Task WriteOneAsync(IS7Plc plc, S7TagDefinition tag, object? value, CancellationToken ct) { // Parse the address the same way ReadOneAsync does: every tag pre-parses at init - // (_parsedByRawPath), with an on-demand parse as a defensive fallback. Needed here so the + // (ParsedByRawPath), with an on-demand parse as a defensive fallback. Needed here so the // wide-type write can byte-address the block (the narrow path below addresses by the raw // address string instead). - var addr = _parsedByRawPath.TryGetValue(tag.Name, out var parsed) + var addr = ParsedByRawPath.TryGetValue(tag.Name, out var parsed) ? parsed : S7AddressParser.Parse(tag.Address); @@ -2243,7 +2443,7 @@ public sealed class S7Driver { ArgumentNullException.ThrowIfNull(builder); var folder = builder.Folder("S7", "S7"); - foreach (var t in _tagsByRawPath.Values) + foreach (var t in TagsByRawPath.Values) { // A tag carrying a non-null array count (>= 1) surfaces as a 1-D OPC UA array node. // A null count stays scalar. A count of 1 IS a valid 1-element array: the foundation @@ -2308,8 +2508,9 @@ public sealed class S7Driver private void HandlePollError(Exception ex) { _logger.LogWarning(ex, "S7 poll reader failed. Driver={DriverInstanceId}", _driverInstanceId); - if (Health.State != DriverState.Faulted) - Health = new DriverHealth(DriverState.Degraded, Health.LastSuccessfulRead, ex.Message); + TryUpdateHealth(CurrentGeneration, h => h.State == DriverState.Faulted + ? null + : new DriverHealth(DriverState.Degraded, h.LastSuccessfulRead, ex.Message)); } // ---- IHostConnectivityProbe ---- @@ -2418,7 +2619,10 @@ public sealed class S7Driver if (_disposed) return; _disposed = true; SynchronousTeardown(); - _gate.Dispose(); + // _gate / _lifecycleGate are deliberately NOT disposed: a Dispose can race a still-running + // ShutdownAsync (the runtime disposes right after a bounded shutdown), which would then throw + // ObjectDisposedException mid-teardown. A SemaphoreSlim whose wait handle was never requested + // holds no unmanaged resource. } /// Asynchronously disposes the driver and releases resources. @@ -2427,9 +2631,10 @@ public sealed class S7Driver { if (_disposed) return; _disposed = true; + // Safe while a ShutdownAsync is still running: it waits (bounded) for the lifecycle gate, and + // the teardown is idempotent. The semaphores are not disposed (see Dispose). try { await ShutdownAsync(CancellationToken.None).ConfigureAwait(false); } catch { /* disposal is best-effort */ } - _gate.Dispose(); } /// @@ -2439,6 +2644,10 @@ public sealed class S7Driver /// private void SynchronousTeardown() { + // Its own lifecycle generation, like ShutdownCoreAsync: an initialize still running past this + // point discards what it built instead of publishing it over the torn-down driver. + var gen = BeginGeneration(); + // Drain the polling overlay (drain-before-dispose). try { _poll.DisposeAsync().AsTask().Wait(DrainTimeout); } catch { /* timeouts/loop faults are tolerated — teardown continues */ } @@ -2463,6 +2672,6 @@ public sealed class S7Driver try { Plc?.Dispose(); } catch { /* best-effort — tearing down anyway */ } Plc = null; Volatile.Write(ref _negotiatedPduSize, 0); // a stale post-teardown reading would read as "online" - Health = new DriverHealth(DriverState.Unknown, Health.LastSuccessfulRead, null); + TryUpdateHealth(gen, h => new DriverHealth(DriverState.Unknown, h.LastSuccessfulRead, null)); } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs new file mode 100644 index 00000000..d856e307 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs @@ -0,0 +1,143 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:H5 — overlapping lifecycle calls. DriverInstanceActor dispatches initialize on a +/// fire-and-forget task and a reconnect retry can start a second attempt while the first is still in +/// OpenAsync; an abandoned initialize can also still be running when the next shutdown starts. +/// Every wait here is a condition wait on the fake's own signals, never a sleep. +/// +[Trait("Category", "Unit")] +public sealed class S7LifecycleGateTests +{ + private static readonly TimeSpan Bound = TimeSpan.FromSeconds(10); + + private static S7DriverOptions Options(bool probe = false) => new() + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromSeconds(30), // the fake's gated open is released by the test, never timed out + Probe = new S7ProbeOptions { Enabled = probe, Interval = TimeSpan.FromSeconds(30) }, + RawTags = S7RawTags.Entries(new S7TagDefinition("W0", "DB1.DBW0", S7DataType.UInt16, Writable: false)), + }; + + /// A factory whose FIRST open blocks until released; later opens complete immediately. + private sealed class FirstOpenBlocks + { + public readonly ScriptedS7PlcFactory Factory = new(); + public readonly TaskCompletionSource FirstOpenEntered = new(TaskCreationOptions.RunContinuationsAsynchronously); + public readonly TaskCompletionSource ReleaseFirstOpen = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public FirstOpenBlocks() + { + Factory.Open = async (index, ct) => + { + if (index != 0) return; + FirstOpenEntered.TrySetResult(); + await ReleaseFirstOpen.Task.WaitAsync(ct).ConfigureAwait(false); + }; + } + } + + /// + /// A second initialize waits for the first instead of running beside it, then releases the first + /// attempt's connection: exactly one live connection remains. Before the gate, attempt B opened + /// its own connection while A was still in OpenAsync, and A's published connection (and + /// probe loop) was overwritten and never released. + /// + [Fact] + public async Task Overlapping_initializes_are_serialised_and_leave_one_live_connection() + { + var ct = TestContext.Current.CancellationToken; + var f = new FirstOpenBlocks(); + using var drv = new S7Driver(Options(), "s7-overlap", f.Factory); + + var a = drv.InitializeAsync("{}", ct); + await f.FirstOpenEntered.Task.WaitAsync(Bound, ct); + var b = drv.InitializeAsync("{}", ct); + + // Positive control first: B is genuinely pending (not finished, not faulted)... + b.IsCompleted.ShouldBeFalse(); + // ...and while A holds the gate B has not created a connection of its own. B can only reach + // its open after A completes, so this is not a race the test can lose by being slow. + f.Factory.Created.Count.ShouldBe(1, "a second initialize must not open beside the first"); + + f.ReleaseFirstOpen.SetResult(); + await a.WaitAsync(Bound, ct); + await b.WaitAsync(Bound, ct); + + f.Factory.Created.Count.ShouldBe(2); + f.Factory.Created[0].Disposed.ShouldBeTrue("B releases A's published connection before replacing it"); + f.Factory.Created[1].Disposed.ShouldBeFalse(); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + (await drv.ReadAsync(["W0"], ct))[0].StatusCode.ShouldBe(0u); + } + + /// + /// An initialize still running when a shutdown gives up waiting for it must not publish its + /// connection, its health or its probe loop over the shut-down driver; it fails and disposes what + /// it opened. Before the fix the late attempt published Healthy + a live connection after the + /// shutdown returned. + /// + [Fact] + public async Task An_initialize_superseded_by_shutdown_publishes_nothing() + { + var ct = TestContext.Current.CancellationToken; + var f = new FirstOpenBlocks(); + using var drv = new S7Driver(Options(probe: true), "s7-superseded", f.Factory) + { + LifecycleGateTimeout = TimeSpan.FromSeconds(1), + }; + + var init = drv.InitializeAsync("{}", ct); + await f.FirstOpenEntered.Task.WaitAsync(Bound, ct); + + // The shutdown cannot get the lifecycle gate (the attempt holds it), gives up after its bound + // and tears down anyway. + await drv.ShutdownAsync(ct).WaitAsync(Bound, ct); + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + + // The abandoned attempt now completes its open... + f.ReleaseFirstOpen.SetResult(); + var ex = await Should.ThrowAsync(() => init.WaitAsync(Bound, ct)); + ex.Message.ShouldContain("superseded"); + + // ...and published nothing: no connection kept, health untouched, the driver still shut down. + f.Factory.Created.ShouldHaveSingleItem().Disposed.ShouldBeTrue(); + drv.GetHealth().State.ShouldBe(DriverState.Unknown, "a superseded attempt must not write Healthy or Faulted"); + await Should.ThrowAsync(() => drv.ReadAsync(["W0"], ct)); + + // Positive control: the driver is not wedged — a fresh initialize publishes normally. + await drv.InitializeAsync("{}", ct).WaitAsync(Bound, ct); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + (await drv.ReadAsync(["W0"], ct))[0].StatusCode.ShouldBe(0u); + } + + /// + /// Dispose racing a shutdown that is still running is safe: neither throws, and the driver ends + /// torn down. The semaphores are no longer disposed, so the running shutdown cannot hit an + /// ObjectDisposedException. + /// + [Fact] + public async Task Dispose_while_a_shutdown_is_still_running_is_safe() + { + var ct = TestContext.Current.CancellationToken; + var f = new FirstOpenBlocks(); + var drv = new S7Driver(Options(), "s7-dispose-race", f.Factory) { LifecycleGateTimeout = TimeSpan.FromSeconds(1) }; + + var init = drv.InitializeAsync("{}", ct); + await f.FirstOpenEntered.Task.WaitAsync(Bound, ct); + + var shutdown = drv.ShutdownAsync(ct); // waits on the gate the blocked init holds + var dispose = drv.DisposeAsync().AsTask(); + await shutdown.WaitAsync(Bound, ct); + await dispose.WaitAsync(Bound, ct); + + f.ReleaseFirstOpen.SetResult(); + await Should.ThrowAsync(() => init.WaitAsync(Bound, ct)); + f.Factory.Created.ShouldHaveSingleItem().Disposed.ShouldBeTrue(); + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + } +} From a1f20e78f905bd0b5411847e8c71b508366967d8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:20:51 -0400 Subject: [PATCH 3/7] fix(s7): a CPU refusal is per-tag Bad and counts as answered (R07:H4) A deterministic PLC refusal (object does not exist, address out of range, type inconsistent...) degraded driver health, and an all-refused driver (a wrong DB number) sat Degraded with LastSuccessfulRead frozen, so the 90 s comms-loss watchdog reconnected it forever against a reachable PLC. - IsRefusal / IsAccessDenied classify over the whole exception chain. Live probe on the snap7 fixture: S7.Net's READ paths raise the item error as a BARE Exception ("Received error from PLC: ..."), its write paths as PlcException(WriteData). The old PlcException-only match missed the read shape entirely, so a real read access-denied was BadCommunicationError + Degraded instead of the Faulted PUT/GET alert; the Test Connect probe had the same blind spot. - A refusal (read or write) is BadDeviceFailure with a once-per-tag Warning, no health write and no reopen; it counts as answered, so it advances LastSuccessfulRead and restores Healthy after a comms fault. An undecodable buffer (S7ValueRangeException) also counts as answered. - A write batch the CPU answered without a failure advances LastSuccessfulRead and retires a Degraded the write side itself published. - Tests that used ReadData as their generic "device fault" now use SendData so their Degraded leg is still exercised. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 15 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 192 ++++++++++++++--- .../S7DriverProbe.cs | 10 +- .../S7_1500/S7_1500RefusalTests.cs | 83 ++++++++ .../CapturingS7Logger.cs | 32 +++ .../S7RefusalHealthTests.cs | 193 ++++++++++++++++++ .../S7SpanReadParityTests.cs | 22 +- .../S7TsapAndPduTests.cs | 17 ++ 8 files changed, 515 insertions(+), 49 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/CapturingS7Logger.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 18e07821..448c7689 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -312,7 +312,8 @@ Portal under *Protection & Security* for the CPU. | Read-only tag written | `BadNotWritable` | unchanged | | Unimplemented data type | `BadNotSupported` | unchanged | | PUT/GET denied | `BadNotSupported` | `Faulted` (config alert) | -| CPU / hardware fault | `BadDeviceFailure` | `Degraded` | +| CPU refusal of the item (object does not exist, address out of range, type inconsistent, hardware fault) | `BadDeviceFailure` | unchanged — counts as *answered*, advances `LastSuccessfulRead`; one Warning per tag | +| Other device-layer fault (`PlcException` that is neither a refusal nor connection-fatal) | `BadDeviceFailure` | `Degraded` | | Socket / timeout | `BadCommunicationError` | `Degraded` | | Reconnect suppressed (backoff window) | `BadCommunicationError` | `Degraded` | | Array write of the wrong shape / length / element type | `BadTypeMismatch` | unchanged | @@ -323,7 +324,17 @@ accumulate their outcomes and write health once (`Healthy < Degraded < Faulted`) `Faulted` item is not erased by a later `Degraded` or successful sibling. The write-side `Faulted` is **sticky against reads**: a clean read pass (reads often work against a write-protected CPU) leaves `Faulted` in place and only advances `LastSuccessfulRead`; the next write that succeeds clears it. A -transient `Degraded` write failure is still cleared by a clean read, as before. +transient `Degraded` write failure is cleared by a clean read, or by the next write batch the CPU +answers without a failure. + +**A refusal is an answer (R07:H4).** S7.Net's read paths raise the CPU's item error as a bare +`Exception` ("Received error from PLC: Object does not exist." — live-probed on the snap7 fixture), its +write paths as a `PlcException(WriteData)`; both are classified over the whole exception chain, and the +access-denied text inside either is the `Faulted` alert. A refusal is deterministic, so it neither +degrades health nor reopens the connection, and because it proves the CPU is reachable it advances +`LastSuccessfulRead` and restores `Healthy` after an earlier comms fault. Before this, a driver whose +every tag was refused (a wrong DB number) sat `Degraded` with a frozen timestamp and the comms-loss +watchdog reconnected it every 90 s against a perfectly reachable PLC. **Connect backoff.** A dead PLC costs one connect attempt per backoff window (1 s doubling to a 30 s cap), not one per read / poll tick / probe tick: inside the window `EnsureConnectedAsync` fails fast diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index acade74e..f1136908 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -208,6 +208,19 @@ public sealed class S7Driver /// Guarded by . /// private (DriverState State, string Message)? _writeFault; + + /// + /// The message of the last Degraded the WRITE side published, or null. A clean write batch + /// retires a Degraded carrying exactly this message (a good write proves the comms fault it + /// reported is gone) and leaves any other Degraded to the read pass. Guarded by . + /// + private string? _writeOwnedError; + + /// + /// Tags whose refusal has already been logged this generation — the once-per-tag Warning. Cleared + /// by every initialize publish. + /// + private readonly ConcurrentDictionary _refusalWarned = new(StringComparer.Ordinal); private bool _disposed; /// @@ -797,7 +810,7 @@ public sealed class S7Driver } catch (Exception ex) { - results[i] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now); + results[i] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [resolved[i]!.Name]), null, now); } } } @@ -903,6 +916,13 @@ public sealed class S7Driver /// Records that one tag read successfully. public void RecordSuccess() => AnySuccess = true; + /// + /// Records that the CPU answered for one tag without serving it (a refusal, an undecodable + /// buffer). Not a failure — it ranks nothing — but it IS proof of a live device, so it + /// advances LastSuccessfulRead exactly like a success. + /// + public void RecordAnswered() => AnySuccess = true; + /// /// Records one failed tag, keeping the worst state and the message that explains it. /// @@ -1098,7 +1118,7 @@ public sealed class S7Driver { // Decoding is pure and synchronous — no cancellation can surface here, so there // is deliberately no OperationCanceledException arm to shadow the request one. - results[index] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now); + results[index] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [tag.Name]), null, now); } } } @@ -1195,7 +1215,7 @@ public sealed class S7Driver } catch (Exception ex) { - var status = RecordReadFailure(ex, outcome); + var status = RecordReadFailure(ex, outcome, span.Members.Select(m => m.Tag.Name)); foreach (var member in span.Members) results[member.OriginalIndex] = new DataValueSnapshot(null, status, null, now); return; @@ -1214,7 +1234,7 @@ public sealed class S7Driver // Decoding is pure and synchronous — no cancellation can surface here, so there is // deliberately no OperationCanceledException arm to shadow the block-read one above. results[member.OriginalIndex] = - new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now); + new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [member.Tag.Name]), null, now); } } } @@ -1289,8 +1309,10 @@ public sealed class S7Driver /// /// The exception observed on a read. /// The pass's outcome accumulator this failure is recorded into. + /// The tags the failure belongs to (every member of a failed span), for the + /// once-per-tag refusal Warning. /// The OPC UA status code every affected tag must be stamped with. - private uint RecordReadFailure(Exception ex, ReadPassOutcome outcome) + private uint RecordReadFailure(Exception ex, ReadPassOutcome outcome, IEnumerable tagNames) { switch (ex) { @@ -1299,19 +1321,33 @@ public sealed class S7Driver case S7ValueRangeException: // A PLC buffer the type codec refuses (uninitialised DTL, non-BCD S5TIME…) is a data - // problem, not a transport fault — no health change, no connection teardown. + // problem, not a transport fault — no health change, no connection teardown. The + // bytes came off the wire, so the CPU answered. + outcome.RecordAnswered(); return StatusBadOutOfRange; - case PlcException pex when IsAccessDenied(pex): + case var _ when IsAccessDenied(ex): // PUT/GET-disabled (S7-1200/1500) / access-protection — a permanent configuration // fault, NOT a transient one. Blind retry is wasted effort, so map it to // BadNotSupported and flag the driver as a config alert (Faulted) rather than - // Degraded — per driver-specs.md §5. + // Degraded — per driver-specs.md §5. Matched over the whole chain: S7.Net's READ + // paths raise the CPU's item error as a bare Exception, not a PlcException. outcome.RecordFailure(DriverState.Faulted, "S7 access denied — enable PUT/GET communication in TIA Portal " + - $"(Protection & Security) for this CPU. PLC reported: {pex.Message}"); + $"(Protection & Security) for this CPU. PLC reported: {ex.Message}"); return StatusBadNotSupported; + case var _ when IsRefusal(ex): + // The CPU answered and refused the item (object does not exist, address out of + // range, type inconsistent...) — deterministic, so a retry, a reopen or a Degraded + // would all be wrong: per-tag Bad, a once-per-tag Warning, no health write, and it + // counts as answered so LastSuccessfulRead advances (R07:H4). An all-refused driver + // (wrong DB number) used to sit Degraded with a frozen timestamp and trip the + // comms-loss watchdog every 90 s against a reachable PLC. + outcome.RecordAnswered(); + foreach (var name in tagNames) WarnRefusalOnce(name, "read", ex); + return StatusBadDeviceFailure; + case PlcException pex: // A genuine device-layer fault (CPU error, hardware fault) — transient enough to // keep retrying; report BadDeviceFailure and degrade health. A connection-fatal @@ -1969,7 +2005,7 @@ public sealed class S7Driver // Client-supplied value of the wrong shape — not a device or comms fault. results[i] = new WriteResult(StatusBadTypeMismatch); } - catch (PlcException pex) when (IsAccessDenied(pex)) + catch (Exception ex) when (IsAccessDenied(ex)) { // PUT/GET-disabled / access-protection on write — same permanent // configuration fault as on read. BadNotSupported + @@ -1977,7 +2013,15 @@ public sealed class S7Driver results[i] = new WriteResult(StatusBadNotSupported); outcome.RecordFailure(DriverState.Faulted, "S7 access denied — enable PUT/GET communication in TIA Portal " + - $"(Protection & Security) for this CPU. PLC reported: {pex.Message}"); + $"(Protection & Security) for this CPU. PLC reported: {ex.Message}"); + } + catch (Exception ex) when (IsRefusal(ex)) + { + // The CPU answered and refused the item — per-tag Bad, once-per-tag Warning, no + // Degraded, and it counts as answered (R07:H4). + results[i] = new WriteResult(StatusBadDeviceFailure); + outcome.RecordAnswered(); + WarnRefusalOnce(tag.Name, "write", ex); } catch (PlcException pex) { @@ -2029,16 +2073,24 @@ public sealed class S7Driver else if (outcome.AnySuccess) _writeFault = null; // a write got through — the protection fault is gone + var answeredAt = outcome.AnyAnswered ? DateTime.UtcNow : (DateTime?)null; if (worst == DriverState.Healthy) { - // Success-only batch: no health write, except to retire a Faulted that WE (the write - // side) were holding — a clean write is the only evidence the alert has cleared. - // Only a Faulted the write side owns: a read-side Faulted (e.g. access denied on another DB) is the - // read pass's to clear, and Health still carries the write fault's own message iff it is ours. - if (outcome.AnySuccess && _writeFault is null && priorWriteFault is { } owned) - TryUpdateHealth(gen, h => h.State == DriverState.Faulted && h.LastError == owned.Message - ? new DriverHealth(DriverState.Healthy, h.LastSuccessfulRead, null) - : null); + // Clean batch (successes and/or refusals): the CPU answered, so LastSuccessfulRead + // advances, and a non-Healthy state the WRITE side owns is retired — a Faulted only by a + // real success (a refusal does not prove PUT/GET is enabled), a write-side comms Degraded + // by any answer. A state the read side owns (its own message) is the read pass's to clear. + var ownedFault = outcome.AnySuccess && _writeFault is null ? priorWriteFault?.Message : null; + var ownedDegraded = _writeOwnedError; + _writeOwnedError = null; + TryUpdateHealth(gen, h => + { + var lsr = answeredAt ?? h.LastSuccessfulRead; + if ((h.State == DriverState.Faulted && ownedFault is not null && h.LastError == ownedFault) + || (h.State == DriverState.Degraded && ownedDegraded is not null && h.LastError == ownedDegraded)) + return new DriverHealth(DriverState.Healthy, lsr, null); + return lsr == h.LastSuccessfulRead ? null : h with { LastSuccessfulRead = lsr }; + }); return; } @@ -2050,7 +2102,8 @@ public sealed class S7Driver state = wf.State; error = wf.Message; } - TryUpdateHealth(gen, h => new DriverHealth(state, h.LastSuccessfulRead, error)); + if (state == DriverState.Degraded) _writeOwnedError = error; + TryUpdateHealth(gen, h => new DriverHealth(state, answeredAt ?? h.LastSuccessfulRead, error)); } /// @@ -2068,10 +2121,18 @@ public sealed class S7Driver public DriverState WorstFailure => _inner.WorstFailure; /// The message explaining . public string? WorstError => _inner.WorstError; - /// True once the batch observed a success or a failure. - public bool HasObservations => _inner.HasObservations; + /// True once the batch observed a success, a refusal or a failure. + public bool HasObservations => _inner.HasObservations || AnyAnswered; + /// True once the CPU answered any item (a success or a refusal) — proof of a live device. + public bool AnyAnswered { get; private set; } /// Records one successful write. - public void RecordSuccess() => _inner.RecordSuccess(); + public void RecordSuccess() + { + _inner.RecordSuccess(); + AnyAnswered = true; + } + /// Records a write the CPU answered by refusing it: not a success, not a failure. + public void RecordAnswered() => AnyAnswered = true; /// Records one failed write. /// The state the failure would have stamped alone. /// The operator-facing message. @@ -2407,19 +2468,82 @@ public sealed class S7Driver } /// - /// Detects an S7 PUT/GET-disabled / access-protection fault inside an S7.Net - /// . S7.Net's read/write paths wrap every PLC-side error in a - /// PlcException with / ; - /// the response-code validator throws a plain for the S7 - /// AccessingObjectNotAllowed status, which lands as the inner exception. There is - /// no typed error code for it, so the inner message is the only discriminator - /// S7.Net exposes. + /// True when is a deterministic PLC REFUSAL of the item: the CPU answered + /// with an S7 item error (object does not exist, address out of range, data type inconsistent / + /// not supported, hardware fault, or an unknown status), or S7.Net raised a + /// / over a response it could + /// not use. Excludes the access-denied refusal (its own sticky Faulted config alert) and + /// anything carrying a socket / timeout / framing / cancellation fault in its chain — those + /// wrap a transport failure, not an answer. /// - /// The exception raised by S7.Net. - /// when the PLC refused access to the object. - internal static bool IsAccessDenied(PlcException pex) + /// + /// Live-probed against the snap7 fixture (2026-10-22): S7.Net's READ paths + /// (ReadAsync(string), ReadBytesAsync) raise the item error as a BARE + /// — "Received error from PLC: Object does not exist." — not wrapped in + /// a ; its write paths wrap whatever they hit in a + /// PlcException(WriteData). Both shapes are matched. + /// + /// The exception raised by S7.Net. + /// when the CPU answered and refused. + internal static bool IsRefusal(Exception ex) { - for (Exception? e = pex; e is not null; e = e.InnerException) + if (IsAccessDenied(ex) || IsS7ConnectionFatal(ex) || HasCancellationInChain(ex)) return false; + if (ex is PlcException { ErrorCode: ErrorCode.ReadData or ErrorCode.WriteData }) return true; + for (Exception? e = ex; e is not null; e = e.InnerException) + if (IsS7ItemError(e)) return true; + return false; + } + + /// + /// True for the bare S7.Net's response-code validator raises for an S7 + /// item error ("Received error from PLC: …" / "Invalid response from PLC: statusCode=…"). + /// Exact type only: a derived exception carrying similar words is not S7.Net's validator. + /// + /// One exception of a chain. + /// for the validator's item-error shape. + private static bool IsS7ItemError(Exception e) + => e.GetType() == typeof(Exception) + && (e.Message.StartsWith("Received error from PLC", StringComparison.Ordinal) + || e.Message.StartsWith("Invalid response from PLC", StringComparison.Ordinal)); + + /// True when a cancellation is buried in the exception chain (a half-read PDU, not a refusal). + /// The exception to inspect. + /// when any inner exception is an . + private static bool HasCancellationInChain(Exception ex) + { + for (var e = ex.InnerException; e is not null; e = e.InnerException) + if (e is OperationCanceledException) return true; + return false; + } + + /// Logs a refusal once per tag per generation. + /// The refused tag. + /// "read" or "write". + /// The refusal. + private void WarnRefusalOnce(string tagName, string operation, Exception ex) + { + if (!_refusalWarned.TryAdd(tagName, 0)) return; + _logger.LogWarning( + "S7 CPU refused the {Operation} of tag {Tag} ({Reason}); it reads Bad until the tag or the PLC program is fixed. " + + "Driver health is unaffected. Further refusals of this tag are not logged. Driver={DriverInstanceId}", + operation, tagName, ex.InnerException?.Message ?? ex.Message, _driverInstanceId); + } + + /// + /// Detects an S7 PUT/GET-disabled / access-protection fault. S7.Net's response-code validator + /// throws a plain for the S7 AccessingObjectNotAllowed status — + /// bare on the read paths, wrapped in a () + /// on the write paths. There is no typed error code for it, so the message is the only + /// discriminator S7.Net exposes. The top-level exception must be S7.Net-shaped (a + /// or the validator's bare item error), so an unrelated exception that + /// happens to say "not allowed" is not mistaken for it. + /// + /// The exception raised by S7.Net. + /// when the PLC refused access to the object. + internal static bool IsAccessDenied(Exception ex) + { + if (ex is not PlcException && !IsS7ItemError(ex)) return false; + for (Exception? e = ex; e is not null; e = e.InnerException) { if (e.Message.Contains("Accessing object not allowed", StringComparison.OrdinalIgnoreCase) || e.Message.Contains("not allowed", StringComparison.OrdinalIgnoreCase)) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs index d936b98a..13925989 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs @@ -128,15 +128,17 @@ public sealed class S7DriverProbe : IDriverProbe return null; } catch (OperationCanceledException) { throw; } - catch (PlcException pex) when (S7Driver.IsAccessDenied(pex)) + // S7.Net's read paths raise the CPU's item error as a BARE Exception ("Received error from + // PLC: …"), not a PlcException, so both classifiers walk the whole chain (the driver's own). + catch (Exception ex) when (S7Driver.IsAccessDenied(ex)) { return new(false, "S7 connected but data access is denied — enable PUT/GET communication in TIA Portal " + - $"(Protection & Security) for this CPU. PLC reported: {pex.Message}", null); + $"(Protection & Security) for this CPU. PLC reported: {ex.Message}", null); } - catch (PlcException pex) when (pex.ErrorCode == ErrorCode.ReadData) + catch (Exception ex) when (S7Driver.IsRefusal(ex)) { - return null; // address not available on this CPU — not a connection problem + return null; // address not available on this CPU — the CPU answered; not a connection problem } catch (Exception ex) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs new file mode 100644 index 00000000..709be4c2 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs @@ -0,0 +1,83 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// Live gate for R07:H4 — a real CPU refusal over real S7comm. The snap7 server has no DB99, so it +/// answers a read of DB99.DBW0 with an S7 item error. That is a per-tag Bad with no health +/// impact (and LastSuccessfulRead advances), not a Degraded that would trip the comms-loss watchdog +/// against a reachable PLC. +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500RefusalTests(Snap7ServerFixture sim) +{ + /// A refused DB read is per-tag Bad, keeps the driver Healthy, and its neighbour reads Good. + [Fact] + public async Task A_read_of_a_missing_DB_is_refused_per_tag_without_degrading() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var ct = TestContext.Current.CancellationToken; + + var baseOptions = S7_1500Profile.BuildOptions(sim.Host, sim.Port); + var options = new S7DriverOptions + { + Host = baseOptions.Host, Port = baseOptions.Port, CpuType = baseOptions.CpuType, + Rack = baseOptions.Rack, Slot = baseOptions.Slot, Timeout = baseOptions.Timeout, + Probe = baseOptions.Probe, + RawTags = [.. baseOptions.RawTags, .. S7RawTags.Entries( + new S7TagDefinition("Missing/Db99", "DB99.DBW0", S7DataType.UInt16, Writable: false))], + }; + await using var drv = new S7Driver(options, driverInstanceId: "s7-refusal-live"); + await drv.InitializeAsync("{}", ct); + var before = drv.GetHealth().LastSuccessfulRead!.Value; + + // Read the refused tag ALONE first: an all-refused pass is exactly the case that used to + // freeze LastSuccessfulRead and latch Degraded. + var refused = await drv.ReadAsync(["Missing/Db99"], ct); + refused[0].StatusCode.ShouldNotBe(0u); + refused[0].Value.ShouldBeNull(); + var health = drv.GetHealth(); + health.State.ShouldBe(DriverState.Healthy, "a CPU refusal is not a comms fault"); + health.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(before); + + // Positive control: the same connection still serves the seeded neighbour. + var good = await drv.ReadAsync([S7_1500Profile.ProbeTag], ct); + good[0].StatusCode.ShouldBe(0u); + Convert.ToInt32(good[0].Value).ShouldBe(S7_1500Profile.ProbeSeedValue); + } + + /// A refused write is per-tag Bad and keeps the driver Healthy; a good write still lands. + [Fact] + public async Task A_write_to_a_missing_DB_is_refused_per_tag_without_degrading() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var ct = TestContext.Current.CancellationToken; + + var baseOptions = S7_1500Profile.BuildOptions(sim.Host, sim.Port); + var options = new S7DriverOptions + { + Host = baseOptions.Host, Port = baseOptions.Port, CpuType = baseOptions.CpuType, + Rack = baseOptions.Rack, Slot = baseOptions.Slot, Timeout = baseOptions.Timeout, + Probe = baseOptions.Probe, + RawTags = [.. baseOptions.RawTags, .. S7RawTags.Entries( + new S7TagDefinition("Missing/Db99W", "DB99.DBW0", S7DataType.UInt16, Writable: true))], + }; + await using var drv = new S7Driver(options, driverInstanceId: "s7-refusal-write-live"); + await drv.InitializeAsync("{}", ct); + + var refused = await drv.WriteAsync([new WriteRequest("Missing/Db99W", (ushort)5)], ct); + refused[0].StatusCode.ShouldNotBe(0u); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a CPU write refusal is not a comms fault"); + + // Positive control: a write the CPU accepts still lands (and reads back). + var ok = await drv.WriteAsync([new WriteRequest(S7_1500Profile.WriteScratchTag, (ushort)4711)], ct); + ok[0].StatusCode.ShouldBe(0u); + var back = await drv.ReadAsync([S7_1500Profile.WriteScratchTag], ct); + Convert.ToInt32(back[0].Value).ShouldBe(4711); + await drv.WriteAsync([new WriteRequest(S7_1500Profile.WriteScratchTag, (ushort)0)], ct); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/CapturingS7Logger.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/CapturingS7Logger.cs new file mode 100644 index 00000000..c2664116 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/CapturingS7Logger.cs @@ -0,0 +1,32 @@ +using Microsoft.Extensions.Logging; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// An that records (level, message) pairs. +internal sealed class CapturingS7Logger : ILogger +{ + private readonly List<(LogLevel Level, string Message)> _entries = []; + + /// A snapshot of every entry logged so far. + public IReadOnlyList<(LogLevel Level, string Message)> Entries + { + get { lock (_entries) return [.. _entries]; } + } + + /// The Warning messages logged so far. + public IReadOnlyList Warnings + => [.. Entries.Where(e => e.Level == LogLevel.Warning).Select(e => e.Message)]; + + /// + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + /// + public bool IsEnabled(LogLevel logLevel) => true; + + /// + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) + { + lock (_entries) _entries.Add((logLevel, formatter(state, exception))); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs new file mode 100644 index 00000000..06aa3277 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs @@ -0,0 +1,193 @@ +using S7.Net; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:H4 — a deterministic PLC refusal (S7.Net ReadData / WriteData carrying an S7 +/// item error such as "object does not exist", not a socket/timeout/framing fault) proves the CPU +/// answered. It is per-tag Bad with a once-per-tag Warning and NO Degraded; it advances +/// LastSuccessfulRead, so a driver whose every tag is refused (wrong DB number) no longer +/// trips the 90 s comms-loss watchdog against a reachable PLC. +/// +[Trait("Category", "Unit")] +public sealed class S7RefusalHealthTests +{ + private const uint BadDeviceFailure = 0x808B0000u; + + /// + /// The shape S7.Net's READ paths really raise for a CPU item error — a BARE + /// , live-probed against the snap7 fixture (not a PlcException). + /// + private static Exception ReadRefusal() => new("Received error from PLC: Object does not exist."); + + private static PlcException WriteRefusal() => + new(ErrorCode.WriteData, new Exception("Received error from PLC: Address out of range.")); + + private static S7DriverOptions Options() => new() + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromSeconds(5), + Probe = new S7ProbeOptions { Enabled = false }, + // Grouping off: one wire request per tag keeps each tag's outcome its own. + SpanReadMaxGapBytes = -1, + RawTags = S7RawTags.Entries( + new S7TagDefinition("Missing", "DB99.DBW0", S7DataType.UInt16), + new S7TagDefinition("Good", "DB1.DBW0", S7DataType.UInt16)), + }; + + private static async Task<(S7Driver Driver, ScriptedS7PlcFactory Factory, CapturingS7Logger Log)> StartAsync() + { + var factory = new ScriptedS7PlcFactory(); + var log = new CapturingS7Logger(); + var drv = new S7Driver(Options(), "s7-refusal", factory, log); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + return (drv, factory, log); + } + + /// A refused tag is Bad, health stays Healthy, LastSuccessfulRead advances, and it warns once. + [Fact] + public async Task A_read_refusal_is_per_tag_bad_with_no_health_impact_and_one_warning() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, log) = await StartAsync(); + using var _ = drv; + factory.Read = (addr, _) => addr == "DB99.DBW0" ? throw ReadRefusal() : Task.FromResult((ushort)7); + + var before = drv.GetHealth().LastSuccessfulRead!.Value; + for (var pass = 0; pass < 3; pass++) + { + var r = await drv.ReadAsync(["Missing"], ct); + r[0].StatusCode.ShouldBe(BadDeviceFailure); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a refusal is not a comms fault"); + } + + drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(before, + "the CPU answered, so the all-refused pass still advances LastSuccessfulRead"); + log.Warnings.Count(w => w.Contains("Missing")).ShouldBe(1, "one Warning per refused tag, not one per poll"); + + // Positive control: the good tag still reads, and nothing reopened the connection. + (await drv.ReadAsync(["Good"], ct))[0].StatusCode.ShouldBe(0u); + factory.Created.Count.ShouldBe(1); + } + + /// + /// A refusal after a comms fault restores Healthy — the CPU is back and answering. Before the + /// fix the refusal latched Degraded with LastSuccessfulRead frozen, which is exactly the + /// watchdog's trip condition. + /// + [Fact] + public async Task A_refusal_after_a_comms_fault_restores_healthy() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _) = await StartAsync(); + using var __ = drv; + + // Positive control: a genuine comms fault DOES degrade. + factory.Read = (_, _) => throw new TimeoutException("frozen peer"); + await drv.ReadAsync(["Missing"], ct); + var degraded = drv.GetHealth(); + degraded.State.ShouldBe(DriverState.Degraded); + + factory.Read = (addr, _) => addr == "DB99.DBW0" ? throw ReadRefusal() : Task.FromResult((ushort)7); + var r = await drv.ReadAsync(["Missing"], ct); + r[0].StatusCode.ShouldBe(BadDeviceFailure); + var after = drv.GetHealth(); + after.State.ShouldBe(DriverState.Healthy); + after.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(degraded.LastSuccessfulRead!.Value); + } + + /// The ReadData-wrapped shape of a refusal is classified the same way. + [Fact] + public async Task A_read_data_wrapped_refusal_is_also_a_refusal() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _) = await StartAsync(); + using var __ = drv; + factory.Read = (_, _) => throw new PlcException(ErrorCode.ReadData, ReadRefusal()); + + (await drv.ReadAsync(["Missing"], ct))[0].StatusCode.ShouldBe(BadDeviceFailure); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// The bare read-path shape of access denied (PUT/GET disabled) is the Faulted config alert. + /// Before, only a PlcException-wrapped "not allowed" was recognised, so a real CPU's read + /// refusal fell through to BadCommunicationError + Degraded. + /// + [Fact] + public async Task A_bare_access_denied_read_is_the_faulted_config_alert() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _) = await StartAsync(); + using var __ = drv; + factory.Read = (_, _) => throw new Exception("Received error from PLC: Accessing object not allowed."); + + (await drv.ReadAsync(["Good"], ct))[0].StatusCode.ShouldBe(0x803D0000u); // BadNotSupported + drv.GetHealth().State.ShouldBe(DriverState.Faulted); + drv.GetHealth().LastError!.ShouldContain("PUT/GET"); + } + + /// A ReadData wrapping a socket fault is still a comms fault (Degraded), not a refusal. + [Fact] + public async Task A_read_data_wrapping_a_socket_fault_still_degrades() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _) = await StartAsync(); + using var __ = drv; + factory.Read = (_, _) => throw new PlcException(ErrorCode.ReadData, new System.IO.IOException("connection reset")); + + await drv.ReadAsync(["Good"], ct); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + /// A write refusal is per-tag Bad, counts as answered and leaves health alone. + [Fact] + public async Task A_write_refusal_is_per_tag_bad_and_counts_as_answered() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, log) = await StartAsync(); + using var _ = drv; + factory.Write = (addr, _, _) => addr == "DB99.DBW0" ? throw WriteRefusal() : Task.CompletedTask; + + // Put the driver into a comms-fault Degraded first; the refusal must lift it. + factory.Read = (_, _) => throw new TimeoutException("frozen peer"); + await drv.ReadAsync(["Good"], ct); + var degraded = drv.GetHealth(); + degraded.State.ShouldBe(DriverState.Degraded); + + for (var i = 0; i < 2; i++) + { + var w = await drv.WriteAsync([new WriteRequest("Missing", (ushort)1)], ct); + w[0].StatusCode.ShouldBe(BadDeviceFailure); + } + + var after = drv.GetHealth(); + after.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(degraded.LastSuccessfulRead!.Value); + log.Warnings.Count(m => m.Contains("Missing")).ShouldBe(1); + } + + /// + /// A good write clears a Degraded the write side wrote. Before the fix a success-only batch + /// never wrote health, so a transient write fault latched Degraded until some read pass ran. + /// + [Fact] + public async Task A_good_write_clears_a_write_side_degraded() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _) = await StartAsync(); + using var _ = drv; + + factory.Write = (_, _, _) => throw new System.IO.IOException("connection reset"); + var bad = await drv.WriteAsync([new WriteRequest("Good", (ushort)1)], ct); + bad[0].StatusCode.ShouldNotBe(0u); + drv.GetHealth().State.ShouldBe(DriverState.Degraded, "positive control: a comms fault on write degrades"); + + factory.Write = (_, _, _) => Task.CompletedTask; + var good = await drv.WriteAsync([new WriteRequest("Good", (ushort)2)], ct); + good[0].StatusCode.ShouldBe(0u); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs index 2dc2152f..ad23e466 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs @@ -202,9 +202,10 @@ public sealed class S7SpanReadParityTests public async Task A_span_read_failure_degrades_every_member_like_the_per_tag_path() { var ct = TestContext.Current.CancellationToken; - // ErrorCode.ReadData with a message that is NOT access-denied ("not allowed") — a genuine - // device-layer fault, which the driver maps to BadDeviceFailure + Degraded. - var fault = new PlcException(ErrorCode.ReadData, "S7 CPU reported a device fault"); + // A device-layer fault that is neither a refusal (a ReadData item error is per-tag Bad with + // no health impact since R07:H4, and a refused span falls back per member since R07:P2) nor + // connection-fatal — the driver maps it to BadDeviceFailure + Degraded on every member. + var fault = new PlcException(ErrorCode.SendData, "S7 CPU reported a device fault"); var groupingFactory = new ImageS7PlcFactory(Images()) { ReadFault = fault }; var perTagFactory = new ImageS7PlcFactory(Images()) { ReadFault = fault }; @@ -468,8 +469,11 @@ public sealed class S7SpanReadParityTests } /// - /// The SPAN path obeys the same rule: one member that fails to decode leaves the pass - /// Degraded even though a later member of the same block decoded fine. + /// The SPAN path guards each member's decode: one member that fails to decode is Bad on its + /// own while a later member of the same block decodes fine. S7.Net rejects the malformed + /// STRING with a ReadData — the CPU answered, the buffer is + /// what is wrong — so since R07:H4 it is a per-tag refusal with no Degraded (like the + /// codec's own BadOutOfRange), and LastSuccessfulRead still advances. /// /// /// The block read itself succeeds — this is the per-member decode guard, the span-path @@ -480,7 +484,7 @@ public sealed class S7SpanReadParityTests /// last-writer-wins used to erase. /// [Fact] - public async Task One_undecodable_span_member_leaves_the_pass_Degraded() + public async Task One_undecodable_span_member_is_bad_alone_without_degrading() { var ct = TestContext.Current.CancellationToken; // The STRING occupies DB1 bytes 100..111 (StringLength + 2), so the word at 112 is @@ -516,7 +520,7 @@ public sealed class S7SpanReadParityTests results[1].Value.ShouldNotBeNull(); var health = driver.GetHealth(); - health.State.ShouldBe(DriverState.Degraded, "a span member that failed to decode must not be erased"); + health.State.ShouldBe(DriverState.Healthy, "an undecodable buffer is a data problem, not a comms fault"); health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(before!.Value); } @@ -542,7 +546,7 @@ public sealed class S7SpanReadParityTests // which is the Faulted config alert. B: an ordinary device-layer fault, merely Degraded, // observed AFTER it. C: no fault at all. factory.PerTagFaults["DB1.DBW0"] = new PlcException(ErrorCode.ReadData, "Accessing object not allowed"); - factory.PerTagFaults["DB1.DBW2"] = new PlcException(ErrorCode.ReadData, "S7 CPU reported a device fault"); + factory.PerTagFaults["DB1.DBW2"] = new PlcException(ErrorCode.SendData, "S7 CPU reported a device fault"); // maxGap -1 turns grouping off entirely, so every tag takes the per-tag read path and the // faults can be aimed at individual addresses. using var driver = new S7Driver(Options(NarrowTags(), maxGap: -1), "s7-access-denied", factory); @@ -584,7 +588,7 @@ public sealed class S7SpanReadParityTests var factory = new ImageS7PlcFactory(Images()); // A: an ordinary device-layer fault (Degraded) FIRST. B: the access-denied config alert // (Faulted) after it. C: no fault at all. - factory.PerTagFaults["DB1.DBW0"] = new PlcException(ErrorCode.ReadData, "S7 CPU reported a device fault"); + factory.PerTagFaults["DB1.DBW0"] = new PlcException(ErrorCode.SendData, "S7 CPU reported a device fault"); factory.PerTagFaults["DB1.DBW2"] = new PlcException(ErrorCode.ReadData, "Accessing object not allowed"); using var driver = new S7Driver(Options(NarrowTags(), maxGap: -1), "s7-escalation-message", factory); await driver.InitializeAsync("{}", ct); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs index 4f9bd37f..a69cd4a1 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7TsapAndPduTests.cs @@ -224,6 +224,23 @@ public sealed class S7TsapAndPduTests (await S7DriverProbe.CheckDataAccessAsync(_ => throw missing, TestContext.Current.CancellationToken)).ShouldBeNull(); } + /// + /// The shapes a real read raises — S7.Net's read paths throw the CPU's item error as a BARE + /// Exception (live-probed on the snap7 fixture). Before, the probe only matched PlcException, so a + /// PUT/GET-disabled CPU got the generic "test read failed" text and a missing address failed the probe. + /// + [Fact] + public async Task Probe_classifies_the_bare_read_path_shapes() + { + var ct = TestContext.Current.CancellationToken; + var denied = await S7DriverProbe.CheckDataAccessAsync( + _ => throw new Exception("Received error from PLC: Accessing object not allowed."), ct); + denied.ShouldNotBeNull().Message!.ShouldContain("PUT/GET"); + + (await S7DriverProbe.CheckDataAccessAsync( + _ => throw new Exception("Received error from PLC: Object does not exist."), ct)).ShouldBeNull(); + } + [Fact] public async Task Probe_reports_any_other_read_fault_as_a_failure() { From 1d83cde6bc9e189f0d8e6826fa532b0da2051679 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:26:14 -0400 Subject: [PATCH 4/7] perf(s7): fail the rest of a pass fast once the handle is dead (R07:P1) No read or write loop checked _plcDead between items: on a frozen-but- established peer the first op blew its deadline and marked the handle dead, and every remaining span / tag / write still went to the wire and could wait its own deadline under _gate (a 20-span pass ~100 s), with Degraded and the comms-loss watchdog's clock deferred until the pass ended. - The span loop, the multi-var chunk loop, the per-tag loop and the write loop stamp BadCommunicationError without I/O once _plcDead is set. - A multi-var chunk that dies on a transport loss (socket / IO / timeout / ConnectionError) marks the handle dead instead of falling back to one doomed per-tag read per member. The framing class is deliberately excluded there: snap7/S7.Net report a multi-var with one refused item as ReadData(WrongNumberReceivedBytes) over a healthy stream. - Live gate via an in-test freezable TCP proxy (counts request frames, not seconds). Mutation shows S7.Net 0.20 already self-closes on a cancelled op, so the gate pins the end-to-end property; the driver guard itself is proven by the unit tests (an op that ignores its token keeps the socket open). Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 8 + .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 69 ++++++++ .../FreezableTcpProxy.cs | 104 +++++++++++ .../S7_1500/S7_1500FrozenPeerTests.cs | 71 ++++++++ .../S7ArrayWriteAndHealthTests.cs | 4 +- .../S7DeadHandleShortCircuitTests.cs | 167 ++++++++++++++++++ 6 files changed, 422 insertions(+), 1 deletion(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/FreezableTcpProxy.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500FrozenPeerTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DeadHandleShortCircuitTests.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 448c7689..9491c93c 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -336,6 +336,14 @@ degrades health nor reopens the connection, and because it proves the CPU is rea every tag was refused (a wrong DB number) sat `Degraded` with a frozen timestamp and the comms-loss watchdog reconnected it every 90 s against a perfectly reachable PLC. +**A dead handle ends the pass's wire traffic (R07:P1).** Once a read pass or write batch marks the +connection dead (a wire op blew its deadline on a frozen-but-established peer, a socket dropped), the +remaining spans / multi-var chunks / tags / writes of that pass are stamped `BadCommunicationError` +without I/O; the next call reopens. A multi-var chunk that dies on a transport loss no longer falls back +to one doomed per-tag read per member. Without this an op that ignores its cancellation token would cost +one full deadline per remaining item under the gate, and `Degraded` (and so the comms-loss watchdog's +clock) only became visible after the whole pass. + **Connect backoff.** A dead PLC costs one connect attempt per backoff window (1 s doubling to a 30 s cap), not one per read / poll tick / probe tick: inside the window `EnsureConnectedAsync` fails fast (`BadCommunicationError`, `Degraded`) without a connect. A successful connect clears the window, so a diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index f1136908..e413125c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -762,6 +762,13 @@ public sealed class S7Driver perTag.Add(span.Members[0].OriginalIndex); continue; } + // Fail fast once an earlier span marked the handle dead (R07:P1): every + // further wire op on it would pay the full deadline under the gate. + if (_plcDead) + { + foreach (var member in span.Members) results[member.OriginalIndex] = DeadHandleSnapshot(now); + continue; + } await ReadSpanAsync(plc, span, results, now, outcome, cancellationToken).ConfigureAwait(false); } perTag.AddRange(plan.LeftoverIndexes); @@ -790,6 +797,12 @@ public sealed class S7Driver foreach (var i in perTag) { + // Fail fast once the handle is dead (R07:P1) — see the span loop. + if (_plcDead) + { + results[i] = DeadHandleSnapshot(now); + continue; + } try { var value = await ReadOneAsync(plc, resolved[i]!, cancellationToken).ConfigureAwait(false); @@ -1069,6 +1082,13 @@ public sealed class S7Driver { var chunk = batchable.GetRange(start, Math.Min(MultiVarReadMaxItems, batchable.Count - start)); + // Fail fast once the handle is dead (R07:P1) — see ReadAsync's span loop. + if (_plcDead) + { + foreach (var (index, _, _) in chunk) results[index] = DeadHandleSnapshot(now); + continue; + } + IReadOnlyList answered; try { @@ -1094,6 +1114,17 @@ public sealed class S7Driver _plcDead = true; throw; } + catch (Exception ex) when (IsS7TransportLoss(ex)) + { + // A transport loss is the connection's fault, not the batch's: falling back would + // send one more doomed per-tag read per member, each paying the full deadline + // (R07:P1). Mark the handle dead and fail the chunk (and, via the guards, the rest + // of the pass) without I/O. + _plcDead = true; + outcome.RecordFailure(DriverState.Degraded, ex.Message); + foreach (var (index, _, _) in chunk) results[index] = DeadHandleSnapshot(now); + continue; + } catch (Exception ex) { _logger.LogDebug(ex, @@ -1974,6 +2005,13 @@ public sealed class S7Driver results[i] = new WriteResult(StatusBadNotWritable); continue; } + // Fail the rest of the batch fast once a write marked the handle dead (R07:P1): the + // next call reopens; this batch must not serialise one deadline per remaining write. + if (_plcDead) + { + results[i] = new WriteResult(StatusBadCommunicationError); + continue; + } try { await WriteOneAsync(plc, tag, w.Value, cancellationToken).ConfigureAwait(false); @@ -2415,6 +2453,37 @@ public sealed class S7Driver if (IsS7ConnectionFatal(ex)) _plcDead = true; } + /// + /// The result stamped, without any I/O, on an item a pass skipped because an earlier item had + /// already marked the connection dead (R07:P1). The pass's health already carries the failure + /// that marked it. + /// + /// The pass timestamp. + /// A BadCommunicationError snapshot. + private static DataValueSnapshot DeadHandleSnapshot(DateTime now) => new(null, StatusBadCommunicationError, null, now); + + /// + /// The TRANSPORT subset of : a socket / IO / disposed handle / + /// blown deadline / anywhere in the chain. Excludes the + /// framing class, because S7.Net reports a multi-var request with one refused item as + /// PlcException(ReadData, PlcException(WrongNumberReceivedBytes)) (live-probed on the snap7 + /// fixture) over a perfectly healthy stream — the multi-var path falls back per tag for that + /// shape, and a genuine desync still surfaces on the per-tag reads. + /// + /// The exception to classify. + /// for a transport loss. + private static bool IsS7TransportLoss(Exception ex) + { + for (Exception? e = ex; e is not null; e = e.InnerException) + { + if (e is System.Net.Sockets.SocketException or System.IO.IOException or ObjectDisposedException or TimeoutException) + return true; + if (e is PlcException { ErrorCode: ErrorCode.ConnectionError }) + return true; + } + return false; + } + /// /// True when (or any inner exception) is a socket-level / connection /// loss OR an ISO-on-TCP framing/desync fault that a reopen can repair — a diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/FreezableTcpProxy.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/FreezableTcpProxy.cs new file mode 100644 index 00000000..6c5e48ce --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/FreezableTcpProxy.cs @@ -0,0 +1,104 @@ +using System.Net; +using System.Net.Sockets; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests; + +/// +/// An in-test TCP forwarder in front of the S7 fixture that can be FROZEN: while frozen it keeps +/// every socket open but forwards nothing in either direction — the frozen-but-established peer +/// (cable pull, firewall DROP) that only a wire-op deadline can detect, produced without touching +/// the shared fixture. It counts the ISO-on-TCP (TPKT) request frames the client sends while frozen, +/// so a test can assert how many wire operations went out without timing anything. +/// +internal sealed class FreezableTcpProxy : IAsyncDisposable +{ + private readonly string _targetHost; + private readonly int _targetPort; + private readonly TcpListener _listener = new(IPAddress.Loopback, 0); + private readonly CancellationTokenSource _cts = new(); + private readonly List _sockets = []; + private volatile bool _frozen; + private int _framesWhileFrozen; + + /// Starts a proxy forwarding to :. + /// The fixture host. + /// The fixture port. + public FreezableTcpProxy(string targetHost, int targetPort) + { + _targetHost = targetHost; + _targetPort = targetPort; + _listener.Start(); + _ = AcceptLoopAsync(); + } + + /// The local port clients connect to. + public int Port => ((IPEndPoint)_listener.LocalEndpoint).Port; + + /// TPKT request frames the client sent while the proxy was frozen. + public int FramesWhileFrozen => Volatile.Read(ref _framesWhileFrozen); + + /// Stops forwarding (sockets stay open). + public void Freeze() => _frozen = true; + + /// Resumes forwarding for connections made from now on. + public void Unfreeze() => _frozen = false; + + private async Task AcceptLoopAsync() + { + try + { + while (!_cts.IsCancellationRequested) + { + var client = await _listener.AcceptTcpClientAsync(_cts.Token).ConfigureAwait(false); + var upstream = new TcpClient(); + await upstream.ConnectAsync(_targetHost, _targetPort, _cts.Token).ConfigureAwait(false); + lock (_sockets) { _sockets.Add(client); _sockets.Add(upstream); } + // A connection accepted while frozen stays frozen for its whole life (its peer is "gone"). + var frozenAtBirth = _frozen; + _ = PumpAsync(client.GetStream(), upstream.GetStream(), countFrames: true, frozenAtBirth); + _ = PumpAsync(upstream.GetStream(), client.GetStream(), countFrames: false, frozenAtBirth); + } + } + catch { /* disposed */ } + } + + private async Task PumpAsync(NetworkStream from, NetworkStream to, bool countFrames, bool frozenAtBirth) + { + var buffer = new byte[8192]; + var pending = new List(); + try + { + while (!_cts.IsCancellationRequested) + { + var n = await from.ReadAsync(buffer, _cts.Token).ConfigureAwait(false); + if (n == 0) return; + var frozen = _frozen || frozenAtBirth; + if (countFrames && frozen) + { + // Count complete TPKT frames: 0x03 0x00 then a big-endian total length. + pending.AddRange(buffer.AsSpan(0, n).ToArray()); + while (pending.Count >= 4) + { + var len = (pending[2] << 8) | pending[3]; + if (len < 4 || pending.Count < len) break; + pending.RemoveRange(0, len); + Interlocked.Increment(ref _framesWhileFrozen); + } + } + if (frozen) continue; // swallow — the peer never answers + await to.WriteAsync(buffer.AsMemory(0, n), _cts.Token).ConfigureAwait(false); + } + } + catch { /* socket closed / disposed */ } + } + + /// + public ValueTask DisposeAsync() + { + _cts.Cancel(); + _listener.Stop(); + lock (_sockets) foreach (var s in _sockets) s.Dispose(); + _cts.Dispose(); + return ValueTask.CompletedTask; + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500FrozenPeerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500FrozenPeerTests.cs new file mode 100644 index 00000000..088931de --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500FrozenPeerTests.cs @@ -0,0 +1,71 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// Live gate for R07:P1 over real S7comm: a frozen-but-established peer (produced by an in-test +/// , so the shared fixture is never touched). The first wire op of +/// the pass blows its deadline and marks the connection dead; the rest of the pass must NOT go to +/// the wire. The assertion counts request frames, not seconds. +/// +/// +/// What this gate does and does not prove. Verified by mutation (2026-10-22): on the real +/// stack this test also passes with the driver's dead-handle guard removed, because S7.Net 0.20 +/// honours the deadline's cancellation token and closes its own socket, so the next op fails at once +/// with ConnectionError and sends nothing. The driver guard (proven by +/// S7DeadHandleShortCircuitTests against a fake that does not self-close) is what covers an +/// op that ignores its token — the WaitAsync leg of S7OperationDeadline, where the +/// abandoned socket stays open and every further op would pay the full deadline. This test pins the +/// end-to-end property (one request per frozen pass, then recovery) against S7.Net upgrades. +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500FrozenPeerTests(Snap7ServerFixture sim) +{ + /// A pass against a frozen peer issues ONE request, then fails the rest fast; it recovers after. + [Fact] + public async Task A_frozen_peer_costs_one_wire_op_per_pass_not_one_per_tag() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var ct = TestContext.Current.CancellationToken; + + await using var proxy = new FreezableTcpProxy(sim.Host, sim.Port); + var baseOptions = S7_1500Profile.BuildOptions("127.0.0.1", proxy.Port); + var options = new S7DriverOptions + { + Host = baseOptions.Host, Port = baseOptions.Port, CpuType = baseOptions.CpuType, + Rack = baseOptions.Rack, Slot = baseOptions.Slot, + Timeout = TimeSpan.FromSeconds(1), // the per-op deadline that detects the frozen peer + Probe = baseOptions.Probe, + SpanReadMaxGapBytes = -1, // one wire request per tag, so "one per tag" is countable + RawTags = baseOptions.RawTags, + }; + string[] refs = + [ + S7_1500Profile.ProbeTag, S7_1500Profile.SmokeI16Tag, S7_1500Profile.SmokeI32Tag, + S7_1500Profile.SmokeF32Tag, S7_1500Profile.SmokeBoolTag, S7_1500Profile.WriteScratchTag, + ]; + + await using var drv = new S7Driver(options, driverInstanceId: "s7-frozen-peer-live"); + await drv.InitializeAsync("{}", ct); + + // Positive control: through the proxy, every tag reads Good. + (await drv.ReadAsync(refs, ct)).ShouldAllBe(s => s.StatusCode == 0u); + + proxy.Freeze(); + var frozen = await drv.ReadAsync(refs, ct); + frozen.ShouldAllBe(s => s.StatusCode != 0u); + proxy.FramesWhileFrozen.ShouldBe(1, "only the request that discovered the dead connection may go out"); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + // Recovery: the next pass reopens (a fresh proxy connection) and reads Good again. + proxy.Unfreeze(); + var recovered = await drv.ReadAsync(refs, ct); + recovered.ShouldAllBe(s => s.StatusCode == 0u); + Convert.ToInt32(recovered[0].Value).ShouldBe(S7_1500Profile.ProbeSeedValue); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs index fab2b496..27c5e20a 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ArrayWriteAndHealthTests.cs @@ -243,7 +243,9 @@ public sealed class S7ArrayWriteAndHealthTests var (drv, factory) = await TwoTagDriver(); using var _ = drv; factory.Created[0].WriteFaults["DB1.DBW0"] = new PlcException(ErrorCode.WriteData, "Accessing object not allowed"); - factory.Created[0].WriteFaults["DB1.DBW2"] = new IOException("socket glitch"); + // A Degraded failure that is NOT connection-fatal: since R07:P1 a fatal one (an IOException) + // would fail the rest of the batch without I/O, and C must reach the wire to be the success. + factory.Created[0].WriteFaults["DB1.DBW2"] = new PlcException(ErrorCode.SendData, "device glitch"); var res = await drv.WriteAsync( [new WriteRequest("A", (ushort)1), new WriteRequest("B", (ushort)2), new WriteRequest("C", (ushort)3)], diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DeadHandleShortCircuitTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DeadHandleShortCircuitTests.cs new file mode 100644 index 00000000..852e9d47 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DeadHandleShortCircuitTests.cs @@ -0,0 +1,167 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:P1 — once a pass has marked the connection dead (a wire op blew its deadline on a frozen +/// peer), the rest of that pass must not go to the wire: each further op would pay the full +/// timeout under the gate, so a 20-span pass took ~100 s and the comms-loss watchdog's clock only +/// started after it. The remaining items are stamped BadCommunicationError without I/O. +/// Every assertion counts wire operations; nothing is timed. +/// +[Trait("Category", "Unit")] +public sealed class S7DeadHandleShortCircuitTests +{ + private const uint BadCommunicationError = 0x80050000u; + + private static S7DriverOptions Options(int maxGap, params S7TagDefinition[] tags) => new() + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromSeconds(5), + Probe = new S7ProbeOptions { Enabled = false }, + SpanReadMaxGapBytes = maxGap, + RawTags = S7RawTags.Entries(tags), + }; + + /// Five isolated words — far enough apart that each is its own span. + private static S7TagDefinition[] FarApartWords() => + [.. Enumerable.Range(0, 5).Select(i => new S7TagDefinition($"W{i}", $"DB{i + 1}.DBW0", S7DataType.UInt16, Writable: true))]; + + private static string[] Refs(int n) => [.. Enumerable.Range(0, n).Select(i => $"W{i}")]; + + private static async Task<(S7Driver, ScriptedS7PlcFactory)> StartAsync(S7DriverOptions options) + { + var factory = new ScriptedS7PlcFactory(); + var drv = new S7Driver(options, "s7-dead-short-circuit", factory); + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); + return (drv, factory); + } + + /// Per-tag path: the first timeout ends the pass's wire traffic. + [Fact] + public async Task Per_tag_pass_stops_issuing_wire_ops_once_the_handle_is_dead() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory) = await StartAsync(Options(maxGap: -1, FarApartWords())); + using var _ = drv; + factory.Read = (_, _) => throw new TimeoutException("frozen peer"); + + var before = factory.WireOps; + var r = await drv.ReadAsync(Refs(5), ct); + + (factory.WireOps - before).ShouldBe(1, "only the op that discovered the dead handle may go to the wire"); + r.ShouldAllBe(s => s.StatusCode == BadCommunicationError); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + // Positive control: the next pass reopens and serves every tag. + factory.Read = (_, _) => Task.FromResult((ushort)1); + var recovered = await drv.ReadAsync(Refs(5), ct); + recovered.ShouldAllBe(s => s.StatusCode == 0u); + factory.Created.Count.ShouldBe(2); + } + + /// Span path: arrays are block reads even alone, so five arrays are five spans. + [Fact] + public async Task Span_pass_stops_issuing_wire_ops_once_the_handle_is_dead() + { + var ct = TestContext.Current.CancellationToken; + S7TagDefinition[] arrays = + [.. Enumerable.Range(0, 5).Select(i => new S7TagDefinition($"W{i}", $"DB{i + 1}.DBW0", S7DataType.UInt16, Writable: false, ArrayCount: 2))]; + var (drv, factory) = await StartAsync(Options(maxGap: 16, arrays)); + using var _ = drv; + factory.ReadBytes = (_, _, _, _, _) => throw new TimeoutException("frozen peer"); + + var before = factory.WireOps; + var r = await drv.ReadAsync(Refs(5), ct); + + (factory.WireOps - before).ShouldBe(1); + r.ShouldAllBe(s => s.StatusCode == BadCommunicationError); + } + + /// + /// Multi-var path: a chunk that dies on a transport loss used to fall back to one per-tag read + /// per member — each paying the timeout again. Now it marks the handle dead and the fallback is + /// short-circuited. + /// + [Fact] + public async Task Multi_var_transport_loss_does_not_fall_back_to_per_tag_wire_reads() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory) = await StartAsync(Options(maxGap: 0, FarApartWords())); + using var _ = drv; + factory.ReadMulti = (_, _) => throw new TimeoutException("frozen peer"); + factory.Read = (_, _) => throw new TimeoutException("frozen peer"); + + var before = factory.WireOps; + var r = await drv.ReadAsync(Refs(5), ct); + + (factory.WireOps - before).ShouldBe(1); + r.ShouldAllBe(s => s.StatusCode == BadCommunicationError); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + /// + /// Positive control for the classification: a multi-var REFUSAL (not a transport loss) still + /// falls back per tag, so the members the CPU will serve are served. + /// + [Fact] + public async Task Multi_var_refusal_still_falls_back_per_tag() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory) = await StartAsync(Options(maxGap: 0, FarApartWords())); + using var _ = drv; + // The live snap7 shape of a multi-var with one refused item. + factory.ReadMulti = (_, _) => throw new global::S7.Net.PlcException(global::S7.Net.ErrorCode.ReadData, + new global::S7.Net.PlcException(global::S7.Net.ErrorCode.WrongNumberReceivedBytes)); + factory.Read = (_, _) => Task.FromResult((ushort)9); + + var before = factory.WireOps; + var r = await drv.ReadAsync(Refs(5), ct); + + (factory.WireOps - before).ShouldBe(6, "one multi-var request, then one per-tag read each"); + r.ShouldAllBe(s => s.StatusCode == 0u); + factory.Created.Count.ShouldBe(1, "a refusal is not a reason to reopen"); + } + + /// A refusal does not short-circuit the pass: the CPU answered, the handle is fine. + [Fact] + public async Task A_refusal_does_not_short_circuit_the_pass() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory) = await StartAsync(Options(maxGap: -1, FarApartWords())); + using var _ = drv; + factory.Read = (addr, _) => addr == "DB1.DBW0" + ? throw new Exception("Received error from PLC: Object does not exist.") + : Task.FromResult((ushort)3); + + var before = factory.WireOps; + var r = await drv.ReadAsync(Refs(5), ct); + + (factory.WireOps - before).ShouldBe(5); + r.Skip(1).ShouldAllBe(s => s.StatusCode == 0u); + } + + /// Write batch: the first transport loss fails the rest of the batch without I/O. + [Fact] + public async Task Write_batch_stops_issuing_wire_ops_once_the_handle_is_dead() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory) = await StartAsync(Options(maxGap: -1, FarApartWords())); + using var _ = drv; + factory.Write = (_, _, _) => throw new System.IO.IOException("connection reset"); + + var before = factory.WireOps; + var w = await drv.WriteAsync([.. Refs(5).Select(r => new WriteRequest(r, (ushort)1))], ct); + + (factory.WireOps - before).ShouldBe(1); + w.ShouldAllBe(x => x.StatusCode == BadCommunicationError); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + + // Positive control: the next batch reopens and writes. + factory.Write = (_, _, _) => Task.CompletedTask; + (await drv.WriteAsync([.. Refs(5).Select(r => new WriteRequest(r, (ushort)1))], ct)) + .ShouldAllBe(x => x.StatusCode == 0u); + } +} From 1f7ab80b814402a9bf93b6999b5848bd294da5c7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:30:12 -0400 Subject: [PATCH 5/7] fix(s7): a refused span falls back per member and is not regrouped (R07:P2) A span's block is the union of its members' ranges, so one member that runs past the DB's real end (or over-declares a STRING) made the CPU refuse the whole block, and every neighbour within it was Bad on every poll. The planner's stated invariant (a grouped read cannot fault where per-tag reads would succeed) was false; the multi-var path already fell back, spans did not. - A refused multi-member span hands its members back; they are read one at a time for that pass, bypassing the multi-var pass. - _noGroup remembers the members refused on their own (or every member when none was) and keeps them out of later plans, so the rest group again. It is cleared by every initialize publish. Non-refusal span failures are unchanged (every member stamped). - The once-per-tag refusal Warning memo (R07:H4) is now also cleared by the publish, and shutdown resets the write-side Degraded owner, as documented. - Live gate: MW10 + MD254 (2 bytes past the 256-byte M area) merged into one refused block; MW10 keeps reading 4097 on every pass. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 7 + .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 74 +++++++- .../S7ReadSpanPlanner.cs | 10 +- .../S7_1500/S7_1500SpanRefusalTests.cs | 49 ++++++ .../S7SpanRefusalFallbackTests.cs | 161 ++++++++++++++++++ 5 files changed, 295 insertions(+), 6 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 9491c93c..207d26b1 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -344,6 +344,13 @@ to one doomed per-tag read per member. Without this an op that ignores its cance one full deadline per remaining item under the gate, and `Degraded` (and so the comms-loss watchdog's clock) only became visible after the whole pass. +**A refused span falls back per member (R07:P2).** A grouped block read covers the union of its members' +ranges, so one member the CPU refuses (running past the DB's real end, an over-declared `stringLength`) +used to make every neighbour in the block Bad on every poll. When the CPU refuses a multi-member span the +driver re-reads its members one at a time for that pass and remembers which tags must not be grouped — +the members refused on their own, or every member when none was — so later passes group only the rest. +The memo lasts until the next initialize. A span failure that is not a refusal still stamps every member. + **Connect backoff.** A dead PLC costs one connect attempt per backoff window (1 s doubling to a 30 s cap), not one per read / poll tick / probe tick: inside the window `EnsureConnectedAsync` fails fast (`BadCommunicationError`, `Degraded`) without a connect. A successful connect clears the window, so a diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index e413125c..9b0ce86b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -221,6 +221,14 @@ public sealed class S7Driver /// by every initialize publish. /// private readonly ConcurrentDictionary _refusalWarned = new(StringComparer.Ordinal); + + /// + /// Tags the span planner must not group (R07:P2): members of a span the CPU refused as a block — + /// the ones that were refused on their own, or every member when none was. They are read + /// individually for the rest of the generation; a re-initialize (a corrected DB, a fixed tag) + /// groups everything again. Guarded by . + /// + private readonly HashSet _noGroup = new(StringComparer.Ordinal); private bool _disposed; /// @@ -338,6 +346,8 @@ public sealed class S7Driver _options = options; Volatile.Write(ref _tables, new TagTables(tags, parsed)); + _refusalWarned.Clear(); + _noGroup.Clear(); _skippedTags.BeginRebuild(options.RawTags.Count); foreach (var (rawPath, reason) in skipped) { @@ -453,6 +463,7 @@ public sealed class S7Driver { if (_generation != gen) return; // a newer initialize owns the state now _writeFault = null; + _writeOwnedError = null; _connectBackoff.RecordSuccess(); Health = new DriverHealth(DriverState.Unknown, Health.LastSuccessfulRead, null); } @@ -732,6 +743,13 @@ public sealed class S7Driver // Indexes the per-tag path must serve: everything, when grouping is off; otherwise the // planner's leftovers (Timer/Counter) plus any tag whose address won't parse. var perTag = new List(); + // Indexes that must take the per-tag path WITHOUT the multi-var pass: tags the planner may + // not group (R07:P2) and the members of a span refused this pass. Batching them into a + // multi-var request would just refuse the whole chunk again. + List? forcedPerTag = null; + // Members of each span refused this pass, and which of them were then refused on their own. + List>? refusedSpans = null; + HashSet? refusedAlone = null; if (_options.SpanReadMaxGapBytes >= 0) { @@ -739,6 +757,11 @@ public sealed class S7Driver for (var i = 0; i < fullReferences.Count; i++) { if (resolved[i] is not { } tag) continue; + if (_noGroup.Contains(tag.Name)) + { + (forcedPerTag ??= []).Add(i); + continue; + } if (!TryGetParsedAddress(tag, out var addr)) { // Defensive: every tag pre-parses at init. A tag that somehow didn't goes @@ -769,7 +792,12 @@ public sealed class S7Driver foreach (var member in span.Members) results[member.OriginalIndex] = DeadHandleSnapshot(now); continue; } - await ReadSpanAsync(plc, span, results, now, outcome, cancellationToken).ConfigureAwait(false); + if (await ReadSpanAsync(plc, span, results, now, outcome, cancellationToken).ConfigureAwait(false) + is { } refusedMembers) + { + (refusedSpans ??= []).Add(refusedMembers); + (forcedPerTag ??= []).AddRange(refusedMembers); + } } perTag.AddRange(plan.LeftoverIndexes); } @@ -794,6 +822,11 @@ public sealed class S7Driver perTag = await ReadLeftoversByMultiVarAsync( plc, resolved, perTag, results, now, outcome, cancellationToken).ConfigureAwait(false); } + if (forcedPerTag is not null) + { + perTag.AddRange(forcedPerTag); + perTag.Sort(); + } foreach (var i in perTag) { @@ -823,9 +856,25 @@ public sealed class S7Driver } catch (Exception ex) { + if (refusedSpans is not null && IsRefusal(ex)) (refusedAlone ??= []).Add(i); results[i] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [resolved[i]!.Name]), null, now); } } + + // Remember what must not be grouped again (R07:P2). Skipped when the handle died mid-pass: + // the per-member reads then proved nothing about which member the CPU refuses. + if (refusedSpans is not null && !_plcDead) + { + foreach (var members in refusedSpans) + { + var bad = members.Where(m => refusedAlone?.Contains(m) == true).ToList(); + var memo = bad.Count > 0 ? bad : [.. members]; + foreach (var m in memo) _noGroup.Add(resolved[m]!.Name); + _logger.LogInformation( + "S7 CPU refused a grouped read of {MemberCount} tag(s); reading {Tags} individually from now on. Driver={DriverInstanceId}", + members.Count, string.Join(", ", memo.Select(m => resolved[m]!.Name)), _driverInstanceId); + } + } } finally { @@ -1222,8 +1271,12 @@ public sealed class S7Driver /// The batch timestamp, shared by every tag in the batch. /// The pass's outcome accumulator; every member records into it. /// Cancellation token for the read. - /// A task that completes once every member of the span has been stamped. - private async Task ReadSpanAsync( + /// + /// once every member has been stamped; or, when the CPU REFUSED the block + /// of a multi-member span (R07:P2 — one member past the DB's end refuses them all), the members' + /// original indexes, unstamped, for the caller to read one at a time. + /// + private async Task?> ReadSpanAsync( IS7Plc plc, S7ReadSpanPlanner.Span span, DataValueSnapshot[] results, DateTime now, ReadPassOutcome outcome, CancellationToken ct) { @@ -1244,12 +1297,24 @@ public sealed class S7Driver _plcDead = true; throw; } + catch (Exception ex) when (span.Members.Count > 1 && IsRefusal(ex)) + { + // The planner's old invariant ("a grouped read cannot fault where the per-tag reads would + // have succeeded") is false: the block is the UNION of the members' ranges, so one member + // running past the DB's real end, or over-declaring a STRING, makes the CPU refuse every + // neighbour with it. Hand the members back for individual reads. + _logger.LogDebug(ex, + "S7 driver {DriverInstanceId}: CPU refused the block read of {Area} db{Db} @{Start}+{Length}; " + + "re-reading its {Count} member(s) individually", _driverInstanceId, span.Area, span.DbNumber, + span.StartByte, span.LengthBytes, span.Members.Count); + return [.. span.Members.Select(m => m.OriginalIndex)]; + } catch (Exception ex) { var status = RecordReadFailure(ex, outcome, span.Members.Select(m => m.Tag.Name)); foreach (var member in span.Members) results[member.OriginalIndex] = new DataValueSnapshot(null, status, null, now); - return; + return null; } foreach (var member in span.Members) @@ -1268,6 +1333,7 @@ public sealed class S7Driver new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [member.Tag.Name]), null, now); } } + return null; } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs index 96f7770a..dfe05b77 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7ReadSpanPlanner.cs @@ -11,8 +11,14 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.S7; /// The safety invariant is that a span never extends past its last member's end. A /// span may deliberately read bytes nobody asked for BETWEEN its members (that is what the /// gap budget buys), but every byte it requests lies inside a range some member was going -/// to read anyway. A grouped read therefore cannot fault (area overrun, DB overrun) where -/// the individual per-tag reads would have succeeded. +/// to read anyway. +/// +/// +/// That does NOT make a grouped read as safe as the per-tag reads (R07:P2): the block is +/// the union of its members' ranges, so ONE member the CPU refuses (running past the DB's real +/// end, an over-declared STRING) makes it refuse the whole block, neighbours included. +/// owns that case: a refused multi-member span is re-read one +/// member at a time, and the members that must not be grouped are left out of later plans. /// /// /// Timer and Counter tags are never grouped. Their diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs new file mode 100644 index 00000000..96dde491 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500SpanRefusalTests.cs @@ -0,0 +1,49 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests.S7_1500; + +/// +/// Live gate for R07:P2 over real S7comm. The fixture's M area is 256 bytes. MW10 is seeded +/// (4097); MD254 runs two bytes past the end of the area. With the gap budget opened wide +/// the planner merges them into ONE block MB10..258, which the CPU refuses — before the fix +/// that made the perfectly readable MW10 Bad (and the driver Degraded) on every poll. +/// +[Collection(Snap7ServerCollection.Name)] +[Trait("Category", "Integration")] +[Trait("Device", "S7_1500")] +public sealed class S7_1500SpanRefusalTests(Snap7ServerFixture sim) +{ + /// The good neighbour of an over-running member keeps reading its seeded value, every pass. + [Fact] + public async Task A_member_past_the_area_end_does_not_spoil_its_span_neighbour() + { + if (sim.SkipReason is not null) Assert.Skip(sim.SkipReason); + var ct = TestContext.Current.CancellationToken; + + var options = new S7DriverOptions + { + Host = sim.Host, Port = sim.Port, CpuType = S7CpuType.S71500, + Timeout = TimeSpan.FromSeconds(5), + Probe = new S7ProbeOptions { Enabled = false }, + // Wide enough that MW10 and MD254 land in one planned span. + SpanReadMaxGapBytes = 256, + SpanReadMaxSpanBytes = 256, + RawTags = S7RawTags.Entries( + new S7TagDefinition("Mk/Word10", "MW10", S7DataType.UInt16, Writable: false), + new S7TagDefinition("Mk/PastEnd", "MD254", S7DataType.Int32, Writable: false)), + }; + await using var drv = new S7Driver(options, driverInstanceId: "s7-span-refusal-live"); + await drv.InitializeAsync("{}", ct); + + for (var pass = 0; pass < 3; pass++) + { + var r = await drv.ReadAsync(["Mk/Word10", "Mk/PastEnd"], ct); + r[0].StatusCode.ShouldBe(0u, $"pass {pass}: the in-range neighbour must be served"); + Convert.ToInt32(r[0].Value).ShouldBe(4097); + r[1].StatusCode.ShouldNotBe(0u, "the over-running member is refused on its own"); + drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a refusal is not a comms fault"); + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs new file mode 100644 index 00000000..ce6ff0ca --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs @@ -0,0 +1,161 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:P2 — a span's block is the union of its members' ranges, so ONE member that runs past the +/// DB's real end (or over-declares a STRING) made the CPU refuse the whole block and every +/// neighbour inside it went Bad on every poll. A refused span now re-reads its members one at a +/// time for that pass and remembers which tags must not be grouped, so later passes group only the +/// members that can be grouped. +/// +[Trait("Category", "Unit")] +public sealed class S7SpanRefusalFallbackTests +{ + private const uint BadDeviceFailure = 0x808B0000u; + + /// DB1 is 4 bytes long on this fake CPU: anything reaching past byte 4 is refused. + private const int DbLength = 4; + + private static Exception OutOfRange() => new("Received error from PLC: Address out of range."); + + private static (S7Driver Driver, ScriptedS7PlcFactory Factory, Func BlockReads, Func TagReads) Start( + params S7TagDefinition[] tags) + { + var factory = new ScriptedS7PlcFactory(); + var blockReads = 0; + var tagReads = 0; + factory.ReadBytes = (_, _, start, count, _) => + { + Interlocked.Increment(ref blockReads); + if (start + count > DbLength) throw OutOfRange(); + var b = new byte[count]; + for (var i = 0; i < count; i++) b[i] = (byte)(start + i + 1); + return Task.FromResult(b); + }; + factory.Read = (addr, _) => + { + Interlocked.Increment(ref tagReads); + var offset = int.Parse(addr[(addr.LastIndexOf('W') + 1)..], System.Globalization.CultureInfo.InvariantCulture); + if (offset + 2 > DbLength) throw OutOfRange(); + return Task.FromResult((ushort)(((offset + 1) << 8) | (offset + 2))); + }; + var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(tags), + }, "s7-span-fallback", factory); + return (drv, factory, () => Volatile.Read(ref blockReads), () => Volatile.Read(ref tagReads)); + } + + private static S7TagDefinition Word(string name, int offset) => + new(name, $"DB1.DBW{offset}", S7DataType.UInt16, Writable: false); + + /// + /// The over-running member is refused on its own; its neighbours are served from the very first + /// pass, and from the second pass on the neighbours are grouped again WITHOUT the bad member. + /// + [Fact] + public async Task A_refused_span_falls_back_per_member_and_stops_grouping_the_bad_member() + { + var ct = TestContext.Current.CancellationToken; + var (drv, _, blockReads, tagReads) = Start(Word("A", 0), Word("B", 2), Word("C", 4)); + using var _ = drv; + await drv.InitializeAsync("{}", ct); + string[] refs = ["A", "B", "C"]; + + // Pass 1: the 6-byte span is refused, then each member is read on its own. + var r1 = await drv.ReadAsync(refs, ct); + r1[0].StatusCode.ShouldBe(0u); + r1[0].Value.ShouldBe((ushort)0x0102); + r1[1].StatusCode.ShouldBe(0u); + r1[1].Value.ShouldBe((ushort)0x0304); + r1[2].StatusCode.ShouldBe(BadDeviceFailure); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + blockReads().ShouldBe(1); + tagReads().ShouldBe(3); + + // Pass 2: A+B ride ONE block read again (the planner no longer sees C); C reads alone. + var r2 = await drv.ReadAsync(refs, ct); + r2[0].Value.ShouldBe((ushort)0x0102); + r2[1].Value.ShouldBe((ushort)0x0304); + r2[2].StatusCode.ShouldBe(BadDeviceFailure); + blockReads().ShouldBe(2, "A+B grouped again, without the refused member"); + tagReads().ShouldBe(4, "only C took the per-tag path on pass 2"); + } + + /// + /// When the block is refused but every member reads fine alone, the span itself is the problem: + /// all of its members stop being grouped (and stop re-trying the refused block every pass). + /// + [Fact] + public async Task A_refused_span_whose_members_all_read_alone_is_not_grouped_again() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ScriptedS7PlcFactory(); + var blockReads = 0; + factory.ReadBytes = (_, _, _, _, _) => { Interlocked.Increment(ref blockReads); throw OutOfRange(); }; + factory.Read = (_, _) => Task.FromResult((ushort)7); + using var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(Word("A", 0), Word("B", 2)), + }, "s7-span-fallback-all", factory); + await drv.InitializeAsync("{}", ct); + + (await drv.ReadAsync(["A", "B"], ct)).ShouldAllBe(s => s.StatusCode == 0u); + (await drv.ReadAsync(["A", "B"], ct)).ShouldAllBe(s => s.StatusCode == 0u); + Volatile.Read(ref blockReads).ShouldBe(1, "the refused block is not re-issued on later passes"); + } + + /// + /// Positive control: a span failure that is NOT a refusal (a device-layer fault) does not fall + /// back — re-reading N members one by one against a failing device would only multiply the fault. + /// + [Fact] + public async Task A_non_refusal_span_failure_does_not_fall_back() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ScriptedS7PlcFactory + { + ReadBytes = (_, _, _, _, _) => throw new global::S7.Net.PlcException(global::S7.Net.ErrorCode.SendData, "device fault"), + }; + using var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries(Word("A", 0), Word("B", 2)), + }, "s7-span-nofallback", factory); + await drv.InitializeAsync("{}", ct); + + var before = factory.WireOps; + var r = await drv.ReadAsync(["A", "B"], ct); + r.ShouldAllBe(s => s.StatusCode == BadDeviceFailure); + (factory.WireOps - before).ShouldBe(1); + drv.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + /// The memo is per generation: a re-initialize (e.g. a corrected DB) groups everything again. + [Fact] + public async Task Reinitialize_clears_the_do_not_group_memo() + { + var ct = TestContext.Current.CancellationToken; + var (drv, _, blockReads, tagReads) = Start(Word("A", 0), Word("B", 2), Word("C", 4)); + using var _ = drv; + await drv.InitializeAsync("{}", ct); + await drv.ReadAsync(["A", "B", "C"], ct); + blockReads().ShouldBe(1); + tagReads().ShouldBe(3); + + await drv.InitializeAsync("{}", ct); + await drv.ReadAsync(["A", "B", "C"], ct); + // With the memo still in force pass 2 would be one A+B block read plus ONE per-tag read (C). + // Cleared, the full A..C span is tried again, refused again, and all three re-read alone. + blockReads().ShouldBe(2); + tagReads().ShouldBe(6, "after re-init the full span is tried again (and refused again)"); + } +} From a0b635f67383e5c21532deeede1633082980cb34 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:33:38 -0400 Subject: [PATCH 6/7] fix(s7): probe gate contention is no verdict; Test Connect honours timeoutMs (R07:H7) - The background probe reported the host Stopped whenever a read pass held _gate past the probe deadline: contention is the connection being used, not lost. A contended tick now gives no verdict (counted for tests); a real probe failure still reports Stopped. A tick that races a teardown no longer reopens a connection on an uninitialised driver. - S7DriverProbe deserialised S7DriverOptions directly, whose Timeout is a TimeSpan no config names, so "timeoutMs" was ignored and every handshake waited the whole Test Connect budget. The handshake and test read are now bounded by timeoutMs, capped by the caller's timeout. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 5 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 112 ++++++++++------ .../S7DriverProbe.cs | 39 +++++- .../S7ProbeContentionAndTimeoutTests.cs | 121 ++++++++++++++++++ 4 files changed, 233 insertions(+), 44 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ProbeContentionAndTimeoutTests.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 207d26b1..889d08be 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -88,6 +88,9 @@ After the handshake, Test Connect reads 2 bytes from `MB0`. A CPU with PUT/GET d default) completes the handshake but denies every read, so the probe fails with an actionable "enable PUT/GET communication" message instead of a false green tick. A CPU that merely rejects that address (no `MB0`) still passes — the connection is fine; any other read fault fails the probe with its message. +Both are recognised in the bare-`Exception` shape S7.Net's read path really raises. The handshake and +test read are bounded by the config's `timeoutMs` (the driver's own per-operation timeout), capped by the +Test Connect timeout — before R07:H7 the config value was ignored. ## Address forms @@ -286,7 +289,7 @@ AdminUI `/hosts` page (Gitea #640). See `docs/Telemetry.md` ("Driver diagnostics | `IWritable` | `WriteAsync` → `S7.Net.Plc.WriteAsync` | Read-only tags (`Writable=false`) return `BadNotWritable` | | `ITagDiscovery` | `DiscoverAsync` | Emits a flat `S7/` folder of the configured tags — no live browse | | `ISubscribable` | per-tag poll loop with capped exponential backoff | S7 has no push model; floor is 100 ms (the CPU services the comms mailbox once per scan) | -| `IHostConnectivityProbe` | periodic `S7.Net.Plc.ReadStatusAsync` (CPU-status PDU) | `host:port` host key; `Running`/`Stopped` transitions raise `OnHostStatusChanged` | +| `IHostConnectivityProbe` | periodic `S7.Net.Plc.ReadStatusAsync` (CPU-status PDU) | `host:port` host key; `Running`/`Stopped` transitions raise `OnHostStatusChanged`. A tick that finds the gate held past the probe deadline (a long read pass) gives **no verdict** — it no longer reports `Stopped` for a connection that is busy, not lost (R07:H7) | ### Single-connection policy diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 9b0ce86b..9f0e98ae 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -2792,66 +2792,96 @@ public sealed class S7Driver { while (!ct.IsCancellationRequested) { - var success = false; + // Running / Stopped, or null for "no verdict this tick" — which leaves the host state alone. + HostState? verdict = null; try { // Probe via S7.Net's low-cost GetCpuStatus — returns the CPU state (Run/Stop) // and is intentionally light on the comms mailbox. Single-word Plc.ReadAsync // would also work but GetCpuStatus doubles as a "PLC actually up" check. - using var gateCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - gateCts.CancelAfter(_options.Probe.Timeout); - - await _gate.WaitAsync(gateCts.Token).ConfigureAwait(false); - try + // + // Gate CONTENTION is not a verdict (R07:H7): a read pass or write batch holding the + // gate past the probe deadline is the connection being USED, and that pass reports + // its own outcome through health. Reporting the host Stopped for it flapped /hosts on + // every long pass against a perfectly healthy PLC. + if (!await _gate.WaitAsync(_options.Probe.Timeout, ct).ConfigureAwait(false)) + { + Interlocked.Increment(ref _probeGateContentions); + } + else { try { - // EnsureConnectedAsync doubles as the reconnect backstop: if a read/write - // marked the handle dead (or the socket dropped between ticks), the probe - // reopens it here so recovery doesn't wait for the next data call. - // It runs on the LOOP token, not the (shorter) probe deadline: the connect is bounded by - // _options.Timeout inside, and a timeout there must count as a failure so the - // backoff window opens — cutting it with the probe deadline looked like a caller cancel, - // recorded nothing, and let a black-holed host cost a connect attempt every probe tick. - var plc = await EnsureConnectedAsync(ct).ConfigureAwait(false); - // The probe deadline bounds only the CPU-status round trip. - using var probeCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - probeCts.CancelAfter(_options.Probe.Timeout); - await plc.ReadStatusAsync(probeCts.Token).ConfigureAwait(false); - success = true; - } - catch (OperationCanceledException) when (ct.IsCancellationRequested) - { - // Loop teardown — rethrow to the outer filter (after finally releases the gate). - throw; - } - catch (OperationCanceledException) - { - // Probe deadline fired mid-PDU: the CPU-status response may still be in - // flight, so the handle can't be reused (half-read ⇒ desync). We hold _gate - // here, keeping _plcDead gate-guarded. - _plcDead = true; - } - catch (Exception ex) - { - // Classify + mark while holding _gate so a wire-dead/desynced - // handle whose TcpClient still claims Connected is reopened by the NEXT - // tick's EnsureConnectedAsync instead of being re-probed forever. - MarkConnectionDeadIfFatal(ex); + // A tick that raced a teardown / re-initialize has nothing to probe and must + // not reopen a connection on a driver that is not initialized. + if (_initialized) + verdict = await ProbeOnceAsync(ct).ConfigureAwait(false); } + finally { _gate.Release(); } } - finally { _gate.Release(); } } catch (OperationCanceledException) when (ct.IsCancellationRequested) { return; } - catch { /* gate-wait timeout / other — treated as Stopped below (gate contention is not a connection fault) */ } + catch { verdict = HostState.Stopped; /* any other probe fault */ } - TransitionTo(success ? HostState.Running : HostState.Stopped); + if (verdict is { } state) TransitionTo(state); try { await Task.Delay(_options.Probe.Interval, ct).ConfigureAwait(false); } catch (OperationCanceledException) { return; } } } + /// + /// One probe round trip. Caller holds . Returns the verdict; rethrows only + /// loop teardown. + /// + /// The probe loop's token. + /// or . + private async Task ProbeOnceAsync(CancellationToken ct) + { + try + { + // EnsureConnectedAsync doubles as the reconnect backstop: if a read/write marked the handle + // dead (or the socket dropped between ticks), the probe reopens it here so recovery doesn't + // wait for the next data call. It runs on the LOOP token, not the (shorter) probe deadline: + // the connect is bounded by _options.Timeout inside, and a timeout there must count as a + // failure so the backoff window opens — cutting it with the probe deadline looked like a + // caller cancel, recorded nothing, and let a black-holed host cost a connect attempt every + // probe tick. + var plc = await EnsureConnectedAsync(ct).ConfigureAwait(false); + // The probe deadline bounds only the CPU-status round trip. + using var probeCts = CancellationTokenSource.CreateLinkedTokenSource(ct); + probeCts.CancelAfter(_options.Probe.Timeout); + await plc.ReadStatusAsync(probeCts.Token).ConfigureAwait(false); + return HostState.Running; + } + catch (OperationCanceledException) when (ct.IsCancellationRequested) + { + throw; // loop teardown — the caller's finally releases the gate + } + catch (OperationCanceledException) + { + // Probe deadline fired mid-PDU: the CPU-status response may still be in flight, so the + // handle can't be reused (half-read ⇒ desync). The caller holds _gate, keeping _plcDead + // gate-guarded. + _plcDead = true; + return HostState.Stopped; + } + catch (Exception ex) + { + // Classify + mark while holding _gate so a wire-dead/desynced handle whose TcpClient still + // claims Connected is reopened by the NEXT tick's EnsureConnectedAsync instead of being + // re-probed forever. + MarkConnectionDeadIfFatal(ex); + return HostState.Stopped; + } + } + + /// Probe ticks that found the gate held past the probe deadline and gave no verdict. + private int _probeGateContentions; + + /// Count of probe ticks skipped for gate contention. Internal so a test can wait on it. + internal int ProbeGateContentions => Volatile.Read(ref _probeGateContentions); + private void TransitionTo(HostState newState) { HostState old; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs index 13925989..ad53b8b7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7DriverProbe.cs @@ -42,6 +42,14 @@ public sealed class S7DriverProbe : IDriverProbe if (string.IsNullOrWhiteSpace(host) || port <= 0) return new(false, "Config has no host/port to probe.", null); + // The S7 handshake + test read are bounded by the driver's own per-operation timeout + // (config "timeoutMs", what the deployed driver will wait), capped by the caller's overall + // Test Connect timeout. The options type carries it as a TimeSpan the JSON never names, so it + // used to be ignored and every handshake waited the full Test Connect timeout (R07:H7). + var opTimeout = ReadTimeoutMs(configJson) is { } ms && TimeSpan.FromMilliseconds(ms) < timeout + ? TimeSpan.FromMilliseconds(ms) + : timeout; + // Phase 1: bare TCP preflight — fast rejection for unreachable hosts. var sw = Stopwatch.StartNew(); try @@ -71,9 +79,9 @@ public sealed class S7DriverProbe : IDriverProbe var plc = S7Tsap.Resolve(opts) is { } tsap ? new Plc(host, port, tsap) : new Plc(S7CpuTypeMap.ToS7Net(opts.CpuType), host, port, opts.Rack, opts.Slot); - plc.ReadTimeout = (int)timeout.TotalMilliseconds; + plc.ReadTimeout = (int)opTimeout.TotalMilliseconds; using var handshakeCts = CancellationTokenSource.CreateLinkedTokenSource(ct); - handshakeCts.CancelAfter(timeout); + handshakeCts.CancelAfter(opTimeout); try { await plc.OpenAsync(handshakeCts.Token); @@ -148,4 +156,31 @@ public sealed class S7DriverProbe : IDriverProbe private static (string host, int port) ExtractTarget(S7DriverOptions opts) => (opts.Host, opts.Port); + + /// + /// The config's timeoutMs (any casing, as the driver factory binds it), or null when absent + /// or not a positive integer. + /// + /// The driver config. + /// The configured per-operation timeout in milliseconds, or null. + internal static int? ReadTimeoutMs(string configJson) + { + try + { + using var doc = JsonDocument.Parse(configJson); + if (doc.RootElement.ValueKind != JsonValueKind.Object) return null; + foreach (var prop in doc.RootElement.EnumerateObject()) + { + if (!string.Equals(prop.Name, "timeoutMs", StringComparison.OrdinalIgnoreCase)) continue; + return prop.Value.ValueKind == JsonValueKind.Number && prop.Value.TryGetInt32(out var ms) && ms > 0 + ? ms + : null; + } + return null; + } + catch (JsonException) + { + return null; + } + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ProbeContentionAndTimeoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ProbeContentionAndTimeoutTests.cs new file mode 100644 index 00000000..6289a9ea --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7ProbeContentionAndTimeoutTests.cs @@ -0,0 +1,121 @@ +using System.Net; +using System.Net.Sockets; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// R07:H7 — (a) the background probe used to report the host Stopped whenever a long read +/// pass held the gate past the probe deadline: contention is the connection being USED, not lost; +/// (b) Test Connect ignored the config's timeoutMs. Waits are condition waits on counters and +/// events; bounds are generous. +/// +[Trait("Category", "Unit")] +public sealed class S7ProbeContentionAndTimeoutTests +{ + private static readonly TimeSpan Bound = TimeSpan.FromSeconds(15); + + private static async Task WaitUntilAsync(Func condition, CancellationToken ct) + { + using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); + cts.CancelAfter(Bound); + while (!condition()) + await Task.Delay(20, cts.Token); // polling a condition, not asserting on elapsed time + } + + /// A read pass holding the gate past the probe deadline does not make the host Stopped. + [Fact] + public async Task Gate_contention_is_no_verdict_but_a_real_probe_failure_still_is() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ScriptedS7PlcFactory(); + var readEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseRead = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + using var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromSeconds(30), + Probe = new S7ProbeOptions { Enabled = true, Interval = TimeSpan.FromMilliseconds(50), Timeout = TimeSpan.FromSeconds(1) }, + RawTags = S7RawTags.Entries(new S7TagDefinition("W0", "DB1.DBW0", S7DataType.UInt16, Writable: false)), + }, "s7-probe-contention", factory); + var transitions = new List(); + drv.OnHostStatusChanged += (_, e) => { lock (transitions) transitions.Add(e.NewState); }; + await drv.InitializeAsync("{}", ct); + + await WaitUntilAsync(() => drv.GetHostStatuses()[0].State == HostState.Running, ct); + + // A read that holds the gate until released. + factory.Read = async (_, token) => + { + readEntered.TrySetResult(); + await releaseRead.Task.WaitAsync(token); + return (ushort)1; + }; + var read = drv.ReadAsync(["W0"], ct); + await readEntered.Task.WaitAsync(Bound, ct); + + // At least two probe ticks give up on the gate... + var contentionsBefore = drv.ProbeGateContentions; + await WaitUntilAsync(() => drv.ProbeGateContentions >= contentionsBefore + 2, ct); + // ...and the host is still Running — no Stopped transition was published for them. + drv.GetHostStatuses()[0].State.ShouldBe(HostState.Running); + lock (transitions) transitions.ShouldNotContain(HostState.Stopped); + + releaseRead.SetResult(); + (await read.WaitAsync(Bound, ct))[0].StatusCode.ShouldBe(0u); + + // Positive control: a genuine probe failure still reports Stopped. + factory.ReadStatus = _ => throw new System.IO.IOException("connection reset"); + await WaitUntilAsync(() => drv.GetHostStatuses()[0].State == HostState.Stopped, ct); + } + + /// Test Connect's handshake is bounded by the config's timeoutMs, not the caller's whole budget. + [Fact] + public async Task Test_connect_honours_the_config_timeoutMs() + { + var ct = TestContext.Current.CancellationToken; + // A TCP endpoint that accepts and then never answers the ISO-on-TCP handshake. + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var held = new List(); + _ = Task.Run(async () => + { + try { while (true) { var c = await listener.AcceptTcpClientAsync(ct); lock (held) held.Add(c); } } + catch { /* stopped */ } + }, ct); + try + { + var port = ((IPEndPoint)listener.LocalEndpoint).Port; + var json = $$"""{"host":"127.0.0.1","port":{{port}},"timeoutMs":1000}"""; + + // The caller's budget (60 s) is far beyond the safety token (20 s): only a probe that honours + // timeoutMs returns its own "handshake failed: timed out"; one that waits the caller's budget + // is cut by the safety token and reports the caller-timeout message instead. + using var safety = CancellationTokenSource.CreateLinkedTokenSource(ct); + safety.CancelAfter(TimeSpan.FromSeconds(20)); + var r = await new S7DriverProbe().ProbeAsync(json, TimeSpan.FromSeconds(60), safety.Token); + + r.Ok.ShouldBeFalse(); + r.Message!.ShouldContain("handshake failed: timed out"); + safety.IsCancellationRequested.ShouldBeFalse(); + } + finally + { + listener.Stop(); + lock (held) foreach (var c in held) c.Dispose(); + } + } + + /// The config reader: any casing, positive integers only. + [Fact] + public void ReadTimeoutMs_reads_the_driver_key() + { + S7DriverProbe.ReadTimeoutMs("""{"TimeoutMs":2500}""").ShouldBe(2500); + S7DriverProbe.ReadTimeoutMs("""{"timeoutms":10}""").ShouldBe(10); + S7DriverProbe.ReadTimeoutMs("""{"timeoutMs":0}""").ShouldBeNull(); + S7DriverProbe.ReadTimeoutMs("""{"host":"x"}""").ShouldBeNull(); + S7DriverProbe.ReadTimeoutMs("not json").ShouldBeNull(); + } +} From 081fa693af80ae7ae57d1b586f81dcc194beaf56 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:57:53 -0400 Subject: [PATCH 7/7] fix(s7): access-denied is an answer; review round-1 minors (R07:H4 follow-through) I-1: a PUT/GET-disabled CPU (every read denied) stayed Faulted with LastSuccessfulRead frozen, so the comms-loss watchdog reconnected and re-subscribed it every 90 s forever. Both access-denied arms now also record an answer: Faulted is kept (worst state wins), the stamp advances, and the sticky write fault still clears only on a real success. Pinned end to end in Runtime.Tests: the real S7Driver under the real DriverInstanceActor watchdog is not reconnected across 2x the threshold; the control (every read timing out) is. Minors: - m-1: injectable pass clock (UtcNow); "LastSuccessfulRead advanced" is now asserted strictly, including the write-refusal case. - m-2: the editor uses S7TagShapeRules.IsByteAnchored instead of its own list. - m-3: a release that races an init publish also cancels the probe loop the publish started after the release took its snapshot. - m-4: a retained poll failing during a lifecycle operation writes no health. - m-5: a multi-var chunk that fell back memoises its members refused alone. - m-7: access-denied needs S7.Net's exact wording and no transport fault in the chain. - m-8: sync teardown clears the write-side Degraded owner; a sync Dispose racing a running shutdown is tested. Refs #742 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/S7.md | 8 +- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 80 +++++++++-- .../Uns/TagEditors/S7TagConfigModel.cs | 5 +- .../S7_1500/S7_1500RefusalTests.cs | 4 +- .../S7LifecycleGateTests.cs | 63 ++++++++ .../S7RefusalHealthTests.cs | 134 +++++++++++++++--- .../S7SpanRefusalFallbackTests.cs | 52 +++++++ .../Drivers/S7AccessDeniedWatchdogTests.cs | 123 ++++++++++++++++ .../ZB.MOM.WW.OtOpcUa.Runtime.Tests.csproj | 3 + 9 files changed, 438 insertions(+), 34 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/S7AccessDeniedWatchdogTests.cs diff --git a/docs/drivers/S7.md b/docs/drivers/S7.md index 889d08be..39788f8c 100644 --- a/docs/drivers/S7.md +++ b/docs/drivers/S7.md @@ -314,7 +314,7 @@ Portal under *Protection & Security* for the CPU. | Tag not in config | `BadNodeIdUnknown` | unchanged | | Read-only tag written | `BadNotWritable` | unchanged | | Unimplemented data type | `BadNotSupported` | unchanged | -| PUT/GET denied | `BadNotSupported` | `Faulted` (config alert) | +| PUT/GET denied | `BadNotSupported` | `Faulted` (config alert) — also counts as *answered*, advances `LastSuccessfulRead` (no watchdog reconnect loop) | | CPU refusal of the item (object does not exist, address out of range, type inconsistent, hardware fault) | `BadDeviceFailure` | unchanged — counts as *answered*, advances `LastSuccessfulRead`; one Warning per tag | | Other device-layer fault (`PlcException` that is neither a refusal nor connection-fatal) | `BadDeviceFailure` | `Degraded` | | Socket / timeout | `BadCommunicationError` | `Degraded` | @@ -333,7 +333,11 @@ answers without a failure. **A refusal is an answer (R07:H4).** S7.Net's read paths raise the CPU's item error as a bare `Exception` ("Received error from PLC: Object does not exist." — live-probed on the snap7 fixture), its write paths as a `PlcException(WriteData)`; both are classified over the whole exception chain, and the -access-denied text inside either is the `Faulted` alert. A refusal is deterministic, so it neither +access-denied text inside either (S7.Net's exact "Accessing object not allowed", and never when the chain +also carries a transport fault) is the `Faulted` alert — which is ALSO an answer: it advances +`LastSuccessfulRead`, so a factory-default CPU with PUT/GET disabled stays `Faulted` instead of being +reconnected by the comms-loss watchdog every 90 s. A multi-var chunk that falls back per tag memoises the +members refused on their own, like a refused span. A refusal is deterministic, so it neither degrades health nor reopens the connection, and because it proves the CPU is reachable it advances `LastSuccessfulRead` and restores `Healthy` after an earlier comms fault. Before this, a driver whose every tag was refused (a wrong DB number) sat `Degraded` with a frozen timestamp and the comms-loss diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 9f0e98ae..cbf94e6b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -201,6 +201,12 @@ public sealed class S7Driver /// Clock the connect backoff is measured on. Internal so tests step it deterministically. internal Func BackoffClock { get; set; } = static () => DateTime.UtcNow; + /// + /// Clock the read/write passes and the init publish stamp timestamps and + /// LastSuccessfulRead with. Internal so a test can step it and assert a STRICT advance. + /// + internal Func UtcNow { get; set; } = static () => DateTime.UtcNow; + /// /// The sticky write-side config fault (access denied / PUT-GET disabled), or null. A read /// pass that finds everything readable must NOT erase it (reads can succeed while writes are @@ -364,7 +370,7 @@ public sealed class S7Driver _connectBackoff.RecordSuccess(); Volatile.Write(ref _negotiatedPduSize, Plc.MaxPduSize); _initialized = true; - TryWriteHealth(gen, new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); + TryWriteHealth(gen, new DriverHealth(DriverState.Healthy, UtcNow(), null)); // Kick off the probe loop once the connection is up. Initial HostState stays Unknown // until the first probe tick succeeds — avoids broadcasting a premature Running @@ -509,9 +515,18 @@ public sealed class S7Driver if (ReferenceEquals(_probeCts, probeCts)) { probeCts?.Dispose(); - _probeCts = null; - _probeTask = null; } + else + { + // An initialize published (and started a probe loop) after this release took its + // snapshot — a teardown whose generation bump raced the publish block. Cancel that + // loop too; it exits on its next gate wait / delay. Not joined: it needs _gate, which + // we hold. Its CTS is left to the GC rather than disposed under a running loop. + try { _probeCts?.Cancel(); } catch { /* best-effort */ } + probeCts?.Dispose(); + } + _probeCts = null; + _probeTask = null; _initialized = false; _plcDead = false; try { Plc?.Dispose(); } catch { /* best-effort — tearing down anyway */ } @@ -693,7 +708,7 @@ public sealed class S7Driver // InvalidOperationException from the not-initialized check. ArgumentNullException.ThrowIfNull(fullReferences); RequireInitialized(); - var now = DateTime.UtcNow; + var now = UtcNow(); var results = new DataValueSnapshot[fullReferences.Count]; // Health is decided ONCE, at the end of the pass, from every outcome the pass observed — // never per item. See ReadPassOutcome. @@ -749,6 +764,8 @@ public sealed class S7Driver List? forcedPerTag = null; // Members of each span refused this pass, and which of them were then refused on their own. List>? refusedSpans = null; + // Members of each multi-var chunk that fell back per tag this pass (R07:P2's multi-var sibling). + List>? multiVarFallbacks = null; HashSet? refusedAlone = null; if (_options.SpanReadMaxGapBytes >= 0) @@ -820,7 +837,8 @@ public sealed class S7Driver if (_options.SpanReadMaxGapBytes >= 0) { perTag = await ReadLeftoversByMultiVarAsync( - plc, resolved, perTag, results, now, outcome, cancellationToken).ConfigureAwait(false); + plc, resolved, perTag, results, now, outcome, cancellationToken, + fallback => (multiVarFallbacks ??= []).Add(fallback)).ConfigureAwait(false); } if (forcedPerTag is not null) { @@ -856,13 +874,28 @@ public sealed class S7Driver } catch (Exception ex) { - if (refusedSpans is not null && IsRefusal(ex)) (refusedAlone ??= []).Add(i); + if ((refusedSpans is not null || multiVarFallbacks is not null) && IsRefusal(ex)) (refusedAlone ??= []).Add(i); results[i] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome, [resolved[i]!.Name]), null, now); } } // Remember what must not be grouped again (R07:P2). Skipped when the handle died mid-pass: // the per-member reads then proved nothing about which member the CPU refuses. + // A multi-var chunk that fell back: memo only the members refused ON THEIR OWN — without one, + // the chunk's failure explains nothing about its members, and they keep batching. + if (multiVarFallbacks is not null && refusedAlone is not null && !_plcDead) + { + foreach (var members in multiVarFallbacks) + { + var bad = members.Where(refusedAlone.Contains).ToList(); + if (bad.Count == 0) continue; + foreach (var m in bad) _noGroup.Add(resolved[m]!.Name); + _logger.LogInformation( + "S7 CPU refused a multi-var read of {MemberCount} tag(s); reading {Tags} individually from now on. Driver={DriverInstanceId}", + members.Count, string.Join(", ", bad.Select(m => resolved[m]!.Name)), _driverInstanceId); + } + } + if (refusedSpans is not null && !_plcDead) { foreach (var members in refusedSpans) @@ -1092,6 +1125,8 @@ public sealed class S7Driver /// The batch timestamp, shared by every tag in the batch. /// The pass's outcome accumulator; every decoded item records into it. /// Cancellation token for the reads. + /// Receives the member indexes of each chunk that fell back per tag, so + /// the caller can memo the members then refused on their own (the multi-var sibling of R07:P2). /// The indexes still owed an individual read, ascending. private async Task> ReadLeftoversByMultiVarAsync( IS7Plc plc, @@ -1100,7 +1135,8 @@ public sealed class S7Driver DataValueSnapshot[] results, DateTime now, ReadPassOutcome outcome, - CancellationToken ct) + CancellationToken ct, + Action> onChunkFallback) { var stillPerTag = new List(); List<(int Index, S7TagDefinition Tag, S7ParsedAddress Addr)>? batchable = null; @@ -1180,6 +1216,7 @@ public sealed class S7Driver "S7 driver {DriverInstanceId}: multi-var read of {ItemCount} leftover tag(s) failed; " + "falling back to per-tag reads for that chunk", _driverInstanceId, chunk.Count); foreach (var (index, _, _) in chunk) stillPerTag.Add(index); + onChunkFallback([.. chunk.Select(c => c.Index)]); continue; } @@ -1429,6 +1466,11 @@ public sealed class S7Driver // BadNotSupported and flag the driver as a config alert (Faulted) rather than // Degraded — per driver-specs.md §5. Matched over the whole chain: S7.Net's READ // paths raise the CPU's item error as a bare Exception, not a PlcException. + // It is ALSO an answer from a reachable CPU: LastSuccessfulRead must advance, or a + // factory-default CPU (every read denied) trips the comms-loss watchdog into a + // reconnect + re-subscribe every 90 s that can never fix a PUT/GET setting. + // Faulted still wins the pass (worst state), so the config alert is kept. + outcome.RecordAnswered(); outcome.RecordFailure(DriverState.Faulted, "S7 access denied — enable PUT/GET communication in TIA Portal " + $"(Protection & Security) for this CPU. PLC reported: {ex.Message}"); @@ -2115,6 +2157,9 @@ public sealed class S7Driver // configuration fault as on read. BadNotSupported + // a config-alert health state, not a transient device failure. results[i] = new WriteResult(StatusBadNotSupported); + // An answer (see the read ladder): advances LastSuccessfulRead. RecordAnswered sets + // AnyAnswered only, so the sticky _writeFault still clears only on a real success. + outcome.RecordAnswered(); outcome.RecordFailure(DriverState.Faulted, "S7 access denied — enable PUT/GET communication in TIA Portal " + $"(Protection & Security) for this CPU. PLC reported: {ex.Message}"); @@ -2177,7 +2222,7 @@ public sealed class S7Driver else if (outcome.AnySuccess) _writeFault = null; // a write got through — the protection fault is gone - var answeredAt = outcome.AnyAnswered ? DateTime.UtcNow : (DateTime?)null; + var answeredAt = outcome.AnyAnswered ? UtcNow() : (DateTime?)null; if (worst == DriverState.Healthy) { // Clean batch (successes and/or refusals): the CPU answered, so LastSuccessfulRead @@ -2678,10 +2723,14 @@ public sealed class S7Driver internal static bool IsAccessDenied(Exception ex) { if (ex is not PlcException && !IsS7ItemError(ex)) return false; + // A transport fault wins over any wording in the chain: a socket error whose text happens to + // contain the phrase must reopen (Degraded), not latch a sticky config alert. + if (IsS7ConnectionFatal(ex) || HasCancellationInChain(ex)) return false; for (Exception? e = ex; e is not null; e = e.InnerException) { - if (e.Message.Contains("Accessing object not allowed", StringComparison.OrdinalIgnoreCase) - || e.Message.Contains("not allowed", StringComparison.OrdinalIgnoreCase)) + // S7.Net's exact wording for the S7 AccessingObjectNotAllowed status — not the loose "not + // allowed", which any unrelated exception text can contain. + if (e.Message.Contains("Accessing object not allowed", StringComparison.OrdinalIgnoreCase)) { return true; } @@ -2766,6 +2815,16 @@ public sealed class S7Driver /// The exception caught by the poll engine. private void HandlePollError(Exception ex) { + // A retained poll that ran into a teardown / re-initialize IN PROGRESS ("not initialized" while a + // lifecycle operation holds _lifecycleGate) belongs to no live generation: that operation owns + // health, and writing Degraded over its Initializing (or its final Unknown) would break the "old + // work never overwrites newer health" rule. A poll against a driver nobody is initializing (a + // subscribe before init, a failed init) still surfaces Degraded (Driver.S7-009). + if (!_initialized && _lifecycleGate.CurrentCount == 0) + { + _logger.LogDebug(ex, "S7 poll reader failed during a lifecycle operation; no health write. Driver={DriverInstanceId}", _driverInstanceId); + return; + } _logger.LogWarning(ex, "S7 poll reader failed. Driver={DriverInstanceId}", _driverInstanceId); TryUpdateHealth(CurrentGeneration, h => h.State == DriverState.Faulted ? null @@ -2957,6 +3016,7 @@ public sealed class S7Driver _initialized = false; _plcDead = false; _writeFault = null; + _writeOwnedError = null; _connectBackoff.RecordSuccess(); try { Plc?.Dispose(); } catch { /* best-effort — tearing down anyway */ } Plc = null; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs index 723cd369..d02db4d9 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs @@ -120,10 +120,7 @@ public sealed class S7TagConfigModel if (TagConfigJson.GetBool(_bag, "isArray")) { var dt = DataType; - if (dt is S7DataType.Int64 or S7DataType.UInt64 or S7DataType.Float64 - or S7DataType.String or S7DataType.DateTime or S7DataType.WString or S7DataType.Char - or S7DataType.WChar or S7DataType.Dtl or S7DataType.Date or S7DataType.Tod - or S7DataType.Time or S7DataType.S5Time) + if (S7TagShapeRules.IsByteAnchored(dt)) { return $"S7 does not support {dt} arrays — use Bool (read-only), Byte, Int16, UInt16, Int32, " + "UInt32 or Float32 for an array tag, or a scalar tag."; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs index 709be4c2..79015983 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.IntegrationTests/S7_1500/S7_1500RefusalTests.cs @@ -34,6 +34,8 @@ public sealed class S7_1500RefusalTests(Snap7ServerFixture sim) await using var drv = new S7Driver(options, driverInstanceId: "s7-refusal-live"); await drv.InitializeAsync("{}", ct); var before = drv.GetHealth().LastSuccessfulRead!.Value; + // Let the wall clock move past the init stamp so "advanced" is asserted strictly (review m-1). + while (DateTime.UtcNow <= before) await Task.Yield(); // Read the refused tag ALONE first: an all-refused pass is exactly the case that used to // freeze LastSuccessfulRead and latch Degraded. @@ -42,7 +44,7 @@ public sealed class S7_1500RefusalTests(Snap7ServerFixture sim) refused[0].Value.ShouldBeNull(); var health = drv.GetHealth(); health.State.ShouldBe(DriverState.Healthy, "a CPU refusal is not a comms fault"); - health.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(before); + health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(before, "the refusal is an answer: the stamp advances"); // Positive control: the same connection still serves the seeded neighbour. var good = await drv.ReadAsync([S7_1500Profile.ProbeTag], ct); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs index d856e307..1c99a61e 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7LifecycleGateTests.cs @@ -115,6 +115,69 @@ public sealed class S7LifecycleGateTests (await drv.ReadAsync(["W0"], ct))[0].StatusCode.ShouldBe(0u); } + /// + /// Review m-4: a retained poll that runs into a re-initialize in progress ("not initialized") + /// must not write Degraded over the new attempt's Initializing. Waits on the poll's own log line + /// (a counted signal), then asserts; the control is that a successful init then reports Healthy. + /// + [Fact] + public async Task A_retained_poll_does_not_overwrite_a_reinitialize_in_progress() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ScriptedS7PlcFactory(); + var secondOpenEntered = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var releaseSecondOpen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + factory.Open = async (index, token) => + { + if (index != 1) return; + secondOpenEntered.TrySetResult(); + await releaseSecondOpen.Task.WaitAsync(token); + }; + var log = new CapturingS7Logger(); + using var drv = new S7Driver(Options(), "s7-retained-poll", factory, log); + await drv.InitializeAsync("{}", ct); + await drv.SubscribeAsync(["W0"], TimeSpan.FromMilliseconds(100), ct); + + var reinit = drv.InitializeAsync("{}", ct); + await secondOpenEntered.Task.WaitAsync(Bound, ct); + + using (var cts = CancellationTokenSource.CreateLinkedTokenSource(ct)) + { + cts.CancelAfter(Bound); + while (log.Entries.Count(e => e.Message.Contains("during a lifecycle operation")) < 2) + await Task.Delay(20, cts.Token); // condition wait on the poll's counted log line + } + drv.GetHealth().State.ShouldBe(DriverState.Initializing, "a retained poll must not write over the new attempt"); + + releaseSecondOpen.SetResult(); + await reinit.WaitAsync(Bound, ct); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// Review m-8: the SYNCHRONOUS Dispose() racing a still-running ShutdownAsync is safe too + /// — neither throws, the superseded init publishes nothing, and the driver ends torn down. + /// + [Fact] + public async Task Sync_dispose_while_a_shutdown_is_still_running_is_safe() + { + var ct = TestContext.Current.CancellationToken; + var f = new FirstOpenBlocks(); + var drv = new S7Driver(Options(), "s7-sync-dispose-race", f.Factory) { LifecycleGateTimeout = TimeSpan.FromSeconds(1) }; + + var init = drv.InitializeAsync("{}", ct); + await f.FirstOpenEntered.Task.WaitAsync(Bound, ct); + + var shutdown = drv.ShutdownAsync(ct); // waits on the gate the blocked init holds + Should.NotThrow(() => drv.Dispose()); + await shutdown.WaitAsync(Bound, ct); + + f.ReleaseFirstOpen.SetResult(); + await Should.ThrowAsync(() => init.WaitAsync(Bound, ct)); + f.Factory.Created.ShouldHaveSingleItem().Disposed.ShouldBeTrue(); + drv.GetHealth().State.ShouldBe(DriverState.Unknown); + } + /// /// Dispose racing a shutdown that is still running is safe: neither throws, and the driver ends /// torn down. The semaphores are no longer disposed, so the running shutdown cannot hit an diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs index 06aa3277..8c7c5182 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7RefusalHealthTests.cs @@ -38,13 +38,24 @@ public sealed class S7RefusalHealthTests new S7TagDefinition("Good", "DB1.DBW0", S7DataType.UInt16)), }; - private static async Task<(S7Driver Driver, ScriptedS7PlcFactory Factory, CapturingS7Logger Log)> StartAsync() + /// A stepped pass clock, so "LastSuccessfulRead advanced" is asserted STRICTLY. + private sealed class PassClock + { + public DateTime Now { get; private set; } = new(2026, 10, 22, 12, 0, 0, DateTimeKind.Utc); + + /// Moves the clock one second on and returns the new time. + public DateTime Step() => Now = Now.AddSeconds(1); + } + + private static async Task<(S7Driver Driver, ScriptedS7PlcFactory Factory, CapturingS7Logger Log, PassClock Clock)> StartAsync() { var factory = new ScriptedS7PlcFactory(); var log = new CapturingS7Logger(); - var drv = new S7Driver(Options(), "s7-refusal", factory, log); + var clock = new PassClock(); + var drv = new S7Driver(Options(), "s7-refusal", factory, log) { UtcNow = () => clock.Now }; await drv.InitializeAsync("{}", TestContext.Current.CancellationToken); - return (drv, factory, log); + drv.GetHealth().LastSuccessfulRead.ShouldBe(clock.Now, "init stamps the pass clock"); + return (drv, factory, log, clock); } /// A refused tag is Bad, health stays Healthy, LastSuccessfulRead advances, and it warns once. @@ -52,20 +63,19 @@ public sealed class S7RefusalHealthTests public async Task A_read_refusal_is_per_tag_bad_with_no_health_impact_and_one_warning() { var ct = TestContext.Current.CancellationToken; - var (drv, factory, log) = await StartAsync(); + var (drv, factory, log, clock) = await StartAsync(); using var _ = drv; factory.Read = (addr, _) => addr == "DB99.DBW0" ? throw ReadRefusal() : Task.FromResult((ushort)7); - var before = drv.GetHealth().LastSuccessfulRead!.Value; for (var pass = 0; pass < 3; pass++) { + var at = clock.Step(); var r = await drv.ReadAsync(["Missing"], ct); r[0].StatusCode.ShouldBe(BadDeviceFailure); drv.GetHealth().State.ShouldBe(DriverState.Healthy, "a refusal is not a comms fault"); + drv.GetHealth().LastSuccessfulRead.ShouldBe(at, + "the CPU answered, so the all-refused pass advances LastSuccessfulRead to the pass time"); } - - drv.GetHealth().LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(before, - "the CPU answered, so the all-refused pass still advances LastSuccessfulRead"); log.Warnings.Count(w => w.Contains("Missing")).ShouldBe(1, "one Warning per refused tag, not one per poll"); // Positive control: the good tag still reads, and nothing reopened the connection. @@ -82,21 +92,25 @@ public sealed class S7RefusalHealthTests public async Task A_refusal_after_a_comms_fault_restores_healthy() { var ct = TestContext.Current.CancellationToken; - var (drv, factory, _) = await StartAsync(); + var (drv, factory, _, clock) = await StartAsync(); using var __ = drv; - // Positive control: a genuine comms fault DOES degrade. + // Positive control: a genuine comms fault DOES degrade, and leaves LastSuccessfulRead frozen. + var initStamp = clock.Now; + clock.Step(); factory.Read = (_, _) => throw new TimeoutException("frozen peer"); await drv.ReadAsync(["Missing"], ct); var degraded = drv.GetHealth(); degraded.State.ShouldBe(DriverState.Degraded); + degraded.LastSuccessfulRead.ShouldBe(initStamp); + var at = clock.Step(); factory.Read = (addr, _) => addr == "DB99.DBW0" ? throw ReadRefusal() : Task.FromResult((ushort)7); var r = await drv.ReadAsync(["Missing"], ct); r[0].StatusCode.ShouldBe(BadDeviceFailure); var after = drv.GetHealth(); after.State.ShouldBe(DriverState.Healthy); - after.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(degraded.LastSuccessfulRead!.Value); + after.LastSuccessfulRead.ShouldBe(at); } /// The ReadData-wrapped shape of a refusal is classified the same way. @@ -104,7 +118,7 @@ public sealed class S7RefusalHealthTests public async Task A_read_data_wrapped_refusal_is_also_a_refusal() { var ct = TestContext.Current.CancellationToken; - var (drv, factory, _) = await StartAsync(); + var (drv, factory, _, clock) = await StartAsync(); using var __ = drv; factory.Read = (_, _) => throw new PlcException(ErrorCode.ReadData, ReadRefusal()); @@ -121,7 +135,7 @@ public sealed class S7RefusalHealthTests public async Task A_bare_access_denied_read_is_the_faulted_config_alert() { var ct = TestContext.Current.CancellationToken; - var (drv, factory, _) = await StartAsync(); + var (drv, factory, _, clock) = await StartAsync(); using var __ = drv; factory.Read = (_, _) => throw new Exception("Received error from PLC: Accessing object not allowed."); @@ -130,12 +144,93 @@ public sealed class S7RefusalHealthTests drv.GetHealth().LastError!.ShouldContain("PUT/GET"); } + /// + /// Review I-1: an access-denied CPU (PUT/GET left at its factory default — EVERY read denied) is + /// reachable and answering, so each pass advances LastSuccessfulRead while staying Faulted. + /// Before, the pass stayed Faulted with the stamp frozen — the comms-loss watchdog's trip + /// condition, so the actor reconnected + re-subscribed every 90 s, forever, which can never fix + /// a PUT/GET setting. The control: a comms fault afterwards leaves the stamp frozen. + /// + [Fact] + public async Task An_all_access_denied_pass_stays_faulted_but_advances_last_successful_read() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _, clock) = await StartAsync(); + using var __ = drv; + factory.Read = (_, _) => throw new Exception("Received error from PLC: Accessing object not allowed."); + + DateTime last = default; + for (var pass = 0; pass < 2; pass++) + { + last = clock.Step(); + await drv.ReadAsync(["Good", "Missing"], ct); + var h = drv.GetHealth(); + h.State.ShouldBe(DriverState.Faulted, "the config alert is kept"); + h.LastSuccessfulRead.ShouldBe(last, "the CPU answered, so the stamp advances every pass"); + } + + // Positive control: a real comms loss does NOT advance it (and the sticky Faulted outranks it). + clock.Step(); + factory.Read = (_, _) => throw new TimeoutException("frozen peer"); + await drv.ReadAsync(["Good"], ct); + drv.GetHealth().LastSuccessfulRead.ShouldBe(last); + } + + /// The write-side access-denied is an answer too, and its sticky Faulted still clears only on a real success. + [Fact] + public async Task A_write_access_denied_advances_last_successful_read_and_stays_sticky() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _, clock) = await StartAsync(); + using var __ = drv; + factory.Write = (_, _, _) => throw new PlcException(ErrorCode.WriteData, + new Exception("Received error from PLC: Accessing object not allowed.")); + + var at = clock.Step(); + (await drv.WriteAsync([new WriteRequest("Good", (ushort)1)], ct))[0].StatusCode.ShouldBe(0x803D0000u); + drv.GetHealth().State.ShouldBe(DriverState.Faulted); + drv.GetHealth().LastSuccessfulRead.ShouldBe(at); + + // A clean read leaves the write-side Faulted in place (sticky) … + clock.Step(); + await drv.ReadAsync(["Good"], ct); + drv.GetHealth().State.ShouldBe(DriverState.Faulted); + + // … and a real write success clears it. + factory.Write = (_, _, _) => Task.CompletedTask; + clock.Step(); + await drv.WriteAsync([new WriteRequest("Good", (ushort)1)], ct); + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// + /// Review m-7: a transport fault whose text happens to contain "not allowed" reopens (Degraded), + /// and only S7.Net's exact "Accessing object not allowed" is the access-denied alert. + /// + [Fact] + public async Task Only_the_exact_access_denied_wording_without_a_transport_fault_is_access_denied() + { + var ct = TestContext.Current.CancellationToken; + var (drv, factory, _, clock) = await StartAsync(); + using var __ = drv; + + clock.Step(); + factory.Write = (_, _, _) => throw new PlcException(ErrorCode.WriteData, + new System.IO.IOException("Accessing object not allowed: socket closed by peer")); + (await drv.WriteAsync([new WriteRequest("Good", (ushort)1)], ct))[0].StatusCode + .ShouldNotBe(0x803D0000u, "not the access-denied BadNotSupported"); + drv.GetHealth().State.ShouldBe(DriverState.Degraded, "a transport fault is not a config alert"); + + S7Driver.IsAccessDenied(new PlcException(ErrorCode.WriteData, new Exception("operation not allowed"))).ShouldBeFalse(); + S7Driver.IsAccessDenied(new Exception("Received error from PLC: Accessing object not allowed.")).ShouldBeTrue(); + } + /// A ReadData wrapping a socket fault is still a comms fault (Degraded), not a refusal. [Fact] public async Task A_read_data_wrapping_a_socket_fault_still_degrades() { var ct = TestContext.Current.CancellationToken; - var (drv, factory, _) = await StartAsync(); + var (drv, factory, _, clock) = await StartAsync(); using var __ = drv; factory.Read = (_, _) => throw new PlcException(ErrorCode.ReadData, new System.IO.IOException("connection reset")); @@ -148,7 +243,7 @@ public sealed class S7RefusalHealthTests public async Task A_write_refusal_is_per_tag_bad_and_counts_as_answered() { var ct = TestContext.Current.CancellationToken; - var (drv, factory, log) = await StartAsync(); + var (drv, factory, log, clock) = await StartAsync(); using var _ = drv; factory.Write = (addr, _, _) => addr == "DB99.DBW0" ? throw WriteRefusal() : Task.CompletedTask; @@ -160,12 +255,17 @@ public sealed class S7RefusalHealthTests for (var i = 0; i < 2; i++) { + var at = clock.Step(); var w = await drv.WriteAsync([new WriteRequest("Missing", (ushort)1)], ct); w[0].StatusCode.ShouldBe(BadDeviceFailure); + var h = drv.GetHealth(); + h.LastSuccessfulRead.ShouldBe(at, "a write refusal is an answer: the stamp advances strictly"); + // The Degraded was written by the READ side (its own message), so it is the read pass's to clear. + h.State.ShouldBe(DriverState.Degraded); } var after = drv.GetHealth(); - after.LastSuccessfulRead!.Value.ShouldBeGreaterThanOrEqualTo(degraded.LastSuccessfulRead!.Value); + after.LastSuccessfulRead!.Value.ShouldBeGreaterThan(degraded.LastSuccessfulRead!.Value); log.Warnings.Count(m => m.Contains("Missing")).ShouldBe(1); } @@ -177,7 +277,7 @@ public sealed class S7RefusalHealthTests public async Task A_good_write_clears_a_write_side_degraded() { var ct = TestContext.Current.CancellationToken; - var (drv, factory, _) = await StartAsync(); + var (drv, factory, _, clock) = await StartAsync(); using var _ = drv; factory.Write = (_, _, _) => throw new System.IO.IOException("connection reset"); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs index ce6ff0ca..11b7358d 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanRefusalFallbackTests.cs @@ -139,6 +139,58 @@ public sealed class S7SpanRefusalFallbackTests drv.GetHealth().State.ShouldBe(DriverState.Degraded); } + /// + /// Review m-5, the multi-var sibling: a leftover chunk holding one refused scalar used to fall back + /// per tag on EVERY poll (1 + N requests). The member refused on its own is now memoised, so the + /// next pass batches the rest and reads only the bad one alone. + /// + [Fact] + public async Task A_refused_multi_var_chunk_memoises_only_the_member_refused_alone() + { + var ct = TestContext.Current.CancellationToken; + var factory = new ScriptedS7PlcFactory(); + var multiCalls = 0; + factory.ReadMulti = (items, _) => + { + Interlocked.Increment(ref multiCalls); + if (items.Any(i => i.DB == 3)) + // The live snap7 shape of a multi-var request with one refused item. + throw new global::S7.Net.PlcException(global::S7.Net.ErrorCode.ReadData, + new global::S7.Net.PlcException(global::S7.Net.ErrorCode.WrongNumberReceivedBytes)); + foreach (var i in items) i.Value = (ushort)5; + return Task.FromResult>(items); + }; + factory.Read = (addr, _) => addr.StartsWith("DB3.", StringComparison.Ordinal) + ? throw new Exception("Received error from PLC: Object does not exist.") + : Task.FromResult((ushort)5); + using var drv = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + RawTags = S7RawTags.Entries( + new S7TagDefinition("A", "DB1.DBW0", S7DataType.UInt16, Writable: false), + new S7TagDefinition("B", "DB2.DBW0", S7DataType.UInt16, Writable: false), + new S7TagDefinition("Bad", "DB3.DBW0", S7DataType.UInt16, Writable: false)), + }, "s7-mv-memo", factory); + await drv.InitializeAsync("{}", ct); + string[] refs = ["A", "B", "Bad"]; + + var before = factory.WireOps; + var r1 = await drv.ReadAsync(refs, ct); + r1[0].StatusCode.ShouldBe(0u); + r1[1].StatusCode.ShouldBe(0u); + r1[2].StatusCode.ShouldBe(BadDeviceFailure); + (factory.WireOps - before).ShouldBe(4, "pass 1: one refused multi-var, then one per-tag read each"); + + before = factory.WireOps; + var r2 = await drv.ReadAsync(refs, ct); + r2[0].StatusCode.ShouldBe(0u); + r2[1].StatusCode.ShouldBe(0u); + r2[2].StatusCode.ShouldBe(BadDeviceFailure); + (factory.WireOps - before).ShouldBe(2, "pass 2: A+B ride one multi-var, Bad reads alone"); + Volatile.Read(ref multiCalls).ShouldBe(2); + } + /// The memo is per generation: a re-initialize (e.g. a corrected DB) groups everything again. [Fact] public async Task Reinitialize_clears_the_do_not_group_memo() diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/S7AccessDeniedWatchdogTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/S7AccessDeniedWatchdogTests.cs new file mode 100644 index 00000000..b7ace6d4 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/Drivers/S7AccessDeniedWatchdogTests.cs @@ -0,0 +1,123 @@ +using Akka.Actor; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.S7; +using ZB.MOM.WW.OtOpcUa.Runtime.Drivers; +using ZB.MOM.WW.OtOpcUa.Runtime.Tests.Harness; +using S7DataItem = global::S7.Net.Types.DataItem; +using S7NetDataType = global::S7.Net.DataType; + +namespace ZB.MOM.WW.OtOpcUa.Runtime.Tests.Drivers; + +/// +/// Lane S7 review I-1, end to end: the REAL under the REAL +/// comms-loss watchdog. A CPU with PUT/GET disabled denies every +/// read but is reachable and answering; the driver reports Faulted (the config alert) and must +/// still advance LastSuccessfulRead, so the watchdog's "non-Healthy AND silent for the +/// threshold" conjunction is never met and the actor does NOT reconnect + re-subscribe. The +/// positive control is the same rig with a real comms loss, which DOES trip it. +/// +public sealed class S7AccessDeniedWatchdogTests : RuntimeActorTestBase +{ + private static readonly TimeSpan FastPoll = TimeSpan.FromMilliseconds(50); + + /// The watchdog threshold — an order of magnitude above the poll so the window is a decision, not a race. + private static readonly TimeSpan Threshold = TimeSpan.FromMilliseconds(1500); + + private sealed class ScriptedPlcFactory(Func readFault) : IS7PlcFactory + { + public IS7Plc Create(S7CpuType cpuType, string host, int port, short rack, short slot, TimeSpan timeout) + => new ScriptedPlc(readFault); + } + + private sealed class ScriptedPlc(Func readFault) : IS7Plc + { + public bool IsConnected { get; private set; } + public Task OpenAsync(CancellationToken cancellationToken) { IsConnected = true; return Task.CompletedTask; } + public void Close() => IsConnected = false; + public Task ReadAsync(string address, CancellationToken cancellationToken) => Task.FromException(readFault()); + public Task ReadBytesAsync(S7NetDataType area, int db, int startByteAdr, int count, CancellationToken cancellationToken) + => Task.FromException(readFault()); + public Task> ReadMultipleVarsAsync(List items, CancellationToken cancellationToken) + => Task.FromException>(readFault()); + public Task WriteAsync(string address, object value, CancellationToken cancellationToken) => Task.CompletedTask; + public Task WriteBytesAsync(S7NetDataType area, int db, int startByteAdr, byte[] value, CancellationToken cancellationToken) => Task.CompletedTask; + public Task ReadStatusAsync(CancellationToken cancellationToken) => Task.CompletedTask; + public void Dispose() => IsConnected = false; + } + + private (S7Driver Driver, Akka.TestKit.TestProbe Parent) Connect(Func readFault) + { + var driver = new S7Driver(new S7DriverOptions + { + Host = "192.0.2.1", + Probe = new S7ProbeOptions { Enabled = false }, + SpanReadMaxGapBytes = -1, + RawTags = + [ + new RawTagEntry("W0", S7TagDefinitionFactory.ToTagConfig(new S7TagDefinition("W0", "DB1.DBW0", S7DataType.UInt16, Writable: false)), false), + new RawTagEntry("W2", S7TagDefinitionFactory.ToTagConfig(new S7TagDefinition("W2", "DB1.DBW2", S7DataType.UInt16, Writable: false)), false), + ], + }, "s7-watchdog", new ScriptedPlcFactory(readFault)); + + var parent = CreateTestProbe(); + var actor = parent.ChildActorOf(DriverInstanceActor.Props( + driver, + reconnectInterval: TimeSpan.FromSeconds(30), + healthPollInterval: FastPoll, + commsLossTimeout: Threshold)); + actor.Tell(new DriverInstanceActor.InitializeRequested("{}")); + parent.ExpectMsg(TimeSpan.FromSeconds(5)).Connected.ShouldBeTrue(); + return (driver, parent); + } + + /// Drives read passes (the poll the actor would schedule) until . + private static async Task<(int Passes, int FaultedPasses)> PollUntilAsync(S7Driver driver, DateTime until) + { + var passes = 0; + var faulted = 0; + while (DateTime.UtcNow < until) + { + try + { +#pragma warning disable OTOPCUA0001 // the watchdog's reading of driver health is under test, not resilience wrapping + await driver.ReadAsync(["W0", "W2"], CancellationToken.None); +#pragma warning restore OTOPCUA0001 + passes++; + if (driver.GetHealth().State == DriverState.Faulted) faulted++; + } + catch (InvalidOperationException) { /* the actor tore the driver down (control case) */ } + await Task.Delay(100); // pacing the simulated poll, not an assertion + } + return (passes, faulted); + } + + /// An all-access-denied CPU is left connected across more than twice the watchdog threshold. + [Fact] + public async Task An_access_denied_cpu_is_not_reconnected_by_the_watchdog() + { + var (driver, parent) = Connect(() => new Exception("Received error from PLC: Accessing object not allowed.")); + + var (passes, faulted) = await PollUntilAsync(driver, DateTime.UtcNow + Threshold + Threshold + TimeSpan.FromMilliseconds(500)); + + // Positive controls: the window really ran past the threshold with the driver Faulted throughout, + // so "no reconnect" below cannot pass merely because nothing happened. + passes.ShouldBeGreaterThanOrEqualTo(10); + faulted.ShouldBe(passes, "every pass reports the PUT/GET config alert"); + + parent.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); + driver.GetHealth().State.ShouldBe(DriverState.Faulted); + } + + /// The control: a real comms loss (every read times out) on the same rig IS reconnected. + [Fact] + public async Task A_real_comms_loss_is_reconnected_by_the_watchdog() + { + var (driver, parent) = Connect(() => new TimeoutException("frozen peer")); + + var poll = PollUntilAsync(driver, DateTime.UtcNow + Threshold + Threshold + TimeSpan.FromMilliseconds(500)); + parent.ExpectMsg(PresenceBudget).Connected.ShouldBeFalse(); + await poll; + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ZB.MOM.WW.OtOpcUa.Runtime.Tests.csproj b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ZB.MOM.WW.OtOpcUa.Runtime.Tests.csproj index 554d3e15..bc5bdb83 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ZB.MOM.WW.OtOpcUa.Runtime.Tests.csproj +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests/ZB.MOM.WW.OtOpcUa.Runtime.Tests.csproj @@ -39,6 +39,9 @@ re-statement end-to-end tests prove what reaches the host's /alerts rows and Part 9 conditions. --> + +