dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(s7): read-pass health is worst-state-wins — a mixed pass can no longer report Healthy

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-17 07:14:32 -04:00
parent e9ba174156
commit 4c16118105
2 changed files with 261 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs
+115 -19
View File
@@ -544,6 +544,9 @@ public sealed class S7Driver
RequireInitialized();
var now = DateTime.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.
var outcome = new ReadPassOutcome();
await _gate.WaitAsync(cancellationToken).ConfigureAwait(false);
try
@@ -612,7 +615,7 @@ public sealed class S7Driver
perTag.Add(span.Members[0].OriginalIndex);
continue;
}
await ReadSpanAsync(plc, span, results, now, cancellationToken).ConfigureAwait(false);
await ReadSpanAsync(plc, span, results, now, outcome, cancellationToken).ConfigureAwait(false);
}
perTag.AddRange(plan.LeftoverIndexes);
}
@@ -635,7 +638,7 @@ public sealed class S7Driver
if (_options.SpanReadMaxGapBytes >= 0)
{
perTag = await ReadLeftoversByMultiVarAsync(
plc, resolved, perTag, results, now, cancellationToken).ConfigureAwait(false);
plc, resolved, perTag, results, now, outcome, cancellationToken).ConfigureAwait(false);
}
foreach (var i in perTag)
@@ -644,7 +647,7 @@ public sealed class S7Driver
{
var value = await ReadOneAsync(plc, resolved[i]!, cancellationToken).ConfigureAwait(false);
results[i] = new DataValueSnapshot(value, 0u, now, now);
_health = new DriverHealth(DriverState.Healthy, now, null);
outcome.RecordSuccess();
}
catch (OperationCanceledException)
{
@@ -660,14 +663,90 @@ public sealed class S7Driver
}
catch (Exception ex)
{
results[i] = new DataValueSnapshot(null, RecordReadFailure(ex), null, now);
results[i] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now);
}
}
// THE single health write of the pass — worst-state-wins over every outcome above.
// Deliberately the last statement of the try body rather than a finally: the three OCE
// arms abandon the batch mid-pass, and a partially-observed pass must not stamp a
// verdict. LastSuccessfulRead advances whenever ANYTHING read, even into a
// Degraded/Faulted state — see ReadPassOutcome for why that is load-bearing.
_health = new DriverHealth(
outcome.WorstFailure,
outcome.AnySuccess ? now : _health.LastSuccessfulRead,
outcome.WorstFailure == DriverState.Healthy ? null : outcome.FirstError);
}
finally { _gate.Release(); }
return results;
}
/// <summary>
/// The outcomes ONE <see cref="ReadAsync"/> pass observed, accumulated so the pass writes
/// <see cref="_health"/> exactly once — <b>worst state wins</b>.
/// </summary>
/// <remarks>
/// <para>
/// Health used to be stamped per item, in three separate read loops, so the pass
/// reported whatever its LAST item happened to be: a mixed pass read <c>Healthy</c>, and
/// the access-denied <c>Faulted</c> config alert — usually a minority of the batch — was
/// erased by any later good tag. Ranking (Healthy &lt; Degraded &lt; Faulted) and
/// deciding once at the end makes the verdict a function of the whole pass rather than
/// of its item ordering.
/// </para>
/// <para>
/// <b><see cref="AnySuccess"/> is tracked separately from the state on purpose.</b> The
/// comms-loss watchdog trips only on a non-Healthy state AND a frozen
/// <c>LastSuccessfulRead</c>; several drivers latch Degraded off a single unreadable
/// address, so a pass that lands Degraded must still advance the timestamp when
/// anything at all was read, or an entirely reachable CPU would be reconnected every
/// 90 s forever.
/// </para>
/// <para>
/// Not thread-safe, and does not need to be: a pass runs wholly under <see cref="_gate"/>
/// and every read loop inside it is sequential.
/// </para>
/// </remarks>
private sealed class ReadPassOutcome
{
/// <summary>True once any tag in the pass read successfully.</summary>
public bool AnySuccess { get; private set; }
/// <summary>
/// The worst failure state recorded so far; <see cref="DriverState.Healthy"/> means no
/// failure has been recorded at all.
/// </summary>
public DriverState WorstFailure { get; private set; } = DriverState.Healthy;
/// <summary>
/// The FIRST failure's message — the causal one. A later failure is as likely to be a
/// consequence of the first as an independent fault, so it does not overwrite it.
/// </summary>
public string? FirstError { get; private set; }
/// <summary>Records that one tag read successfully.</summary>
public void RecordSuccess() => AnySuccess = true;
/// <summary>Records one failed tag, keeping the worst state and the first message.</summary>
/// <param name="state">The state this failure would have stamped on its own.</param>
/// <param name="message">The failure's operator-facing message.</param>
public void RecordFailure(DriverState state, string message)
{
if (Rank(state) > Rank(WorstFailure)) WorstFailure = state;
FirstError ??= message;
}
/// <summary>Severity order the pass ranks by: Healthy (none) &lt; Degraded &lt; Faulted.</summary>
/// <param name="state">The state to rank.</param>
/// <returns>Its severity, higher being worse.</returns>
private static int Rank(DriverState state) => state switch
{
DriverState.Faulted => 2,
DriverState.Degraded => 1,
_ => 0,
};
}
/// <summary>
/// Items one multi-var request may carry. <b>The caller owns chunking — S7.Net does NOT
/// split a multi-var read</b>, it throws when the request or the projected response exceeds
@@ -743,6 +822,7 @@ public sealed class S7Driver
/// <param name="leftovers">Indexes the span pass left for the per-tag path, ascending.</param>
/// <param name="results">The batch's result array, written in place at each tag's original index.</param>
/// <param name="now">The batch timestamp, shared by every tag in the batch.</param>
/// <param name="outcome">The pass's outcome accumulator; every decoded item records into it.</param>
/// <param name="ct">Cancellation token for the reads.</param>
/// <returns>The indexes still owed an individual read, ascending.</returns>
private async Task<List<int>> ReadLeftoversByMultiVarAsync(
@@ -751,6 +831,7 @@ public sealed class S7Driver
List<int> leftovers,
DataValueSnapshot[] results,
DateTime now,
ReadPassOutcome outcome,
CancellationToken ct)
{
var stillPerTag = new List<int>();
@@ -825,13 +906,13 @@ public sealed class S7Driver
?? throw new System.IO.InvalidDataException(
$"S7.Net returned null multi-var value for '{tag.Address}'");
results[index] = new DataValueSnapshot(ReinterpretRawValue(tag, addr, raw), 0u, now, now);
_health = new DriverHealth(DriverState.Healthy, now, null);
outcome.RecordSuccess();
}
catch (Exception ex)
{
// 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), null, now);
results[index] = new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now);
}
}
}
@@ -902,10 +983,12 @@ public sealed class S7Driver
/// <param name="span">The planned block read.</param>
/// <param name="results">The batch's result array, written in place at each member's original index.</param>
/// <param name="now">The batch timestamp, shared by every tag in the batch.</param>
/// <param name="outcome">The pass's outcome accumulator; every member records into it.</param>
/// <param name="ct">Cancellation token for the read.</param>
/// <returns>A task that completes once every member of the span has been stamped.</returns>
private async Task ReadSpanAsync(
IS7Plc plc, S7ReadSpanPlanner.Span span, DataValueSnapshot[] results, DateTime now, CancellationToken ct)
IS7Plc plc, S7ReadSpanPlanner.Span span, DataValueSnapshot[] results, DateTime now,
ReadPassOutcome outcome, CancellationToken ct)
{
byte[] block;
try
@@ -926,7 +1009,7 @@ public sealed class S7Driver
}
catch (Exception ex)
{
var status = RecordReadFailure(ex);
var status = RecordReadFailure(ex, outcome);
foreach (var member in span.Members)
results[member.OriginalIndex] = new DataValueSnapshot(null, status, null, now);
return;
@@ -938,13 +1021,14 @@ public sealed class S7Driver
{
var value = DecodeMember(member, block);
results[member.OriginalIndex] = new DataValueSnapshot(value, 0u, now, now);
_health = new DriverHealth(DriverState.Healthy, now, null);
outcome.RecordSuccess();
}
catch (Exception ex)
{
// 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), null, now);
results[member.OriginalIndex] =
new DataValueSnapshot(null, RecordReadFailure(ex, outcome), null, now);
}
}
}
@@ -998,17 +1082,29 @@ public sealed class S7Driver
/// <summary>
/// The read-failure classification ladder, shared by the per-tag path and the span path so
/// a failed span stamps every member with exactly the status the same exception would have
/// produced on a single tag. Applies the health / dead-handle side effects and returns the
/// status code the caller must stamp.
/// produced on a single tag. Records the failure into the pass's outcome, applies the
/// dead-handle side effect, and returns the status code the caller must stamp.
/// </summary>
/// <remarks>
/// <see cref="OperationCanceledException"/> is deliberately NOT handled here: it is control
/// flow rather than a per-tag status (it marks the handle dead and propagates), and
/// rethrowing it from a helper would rewrite the stack. Both call sites catch it first.
/// <para>
/// <b>The health state is recorded, not applied.</b> It goes into
/// <paramref name="outcome"/> so the pass can write <see cref="_health"/> once at the
/// end, worst-state-wins — writing it here is what let a later good tag erase this
/// failure. The <c>_plcDead</c> / <see cref="MarkConnectionDeadIfFatal"/> side effects
/// stay IMMEDIATE: they govern whether the very next read reopens the handle, which
/// cannot wait for the pass to finish.
/// </para>
/// <para>
/// <see cref="OperationCanceledException"/> is deliberately NOT handled here: it is
/// control flow rather than a per-tag status (it marks the handle dead and propagates),
/// and rethrowing it from a helper would rewrite the stack. Both call sites catch it
/// first.
/// </para>
/// </remarks>
/// <param name="ex">The exception observed on a read.</param>
/// <param name="outcome">The pass's outcome accumulator this failure is recorded into.</param>
/// <returns>The OPC UA status code every affected tag must be stamped with.</returns>
private uint RecordReadFailure(Exception ex)
private uint RecordReadFailure(Exception ex, ReadPassOutcome outcome)
{
switch (ex)
{
@@ -1020,7 +1116,7 @@ public sealed class S7Driver
// 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.
_health = new DriverHealth(DriverState.Faulted, _health.LastSuccessfulRead,
outcome.RecordFailure(DriverState.Faulted,
"S7 access denied — enable PUT/GET communication in TIA Portal " +
$"(Protection & Security) for this CPU. PLC reported: {pex.Message}");
return StatusBadNotSupported;
@@ -1031,14 +1127,14 @@ public sealed class S7Driver
// PlcException (socket drop / ConnectionError) additionally marks the handle dead
// so the next read reopens it.
MarkConnectionDeadIfFatal(pex);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, pex.Message);
outcome.RecordFailure(DriverState.Degraded, pex.Message);
return StatusBadDeviceFailure;
default:
// Socket / timeout / transport fault — mark the handle dead when it's a connection
// loss so EnsureConnectedAsync reopens on the next call.
MarkConnectionDeadIfFatal(ex);
_health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message);
outcome.RecordFailure(DriverState.Degraded, ex.Message);
return StatusBadCommunicationError;
}
}
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs
+146 -5
View File
@@ -387,10 +387,14 @@ public sealed class S7SpanReadParityTests
/// failed REQUEST, and firing it here would re-read tags that already have good values.
/// </summary>
/// <remarks>
/// Health ends <c>Healthy</c> rather than <c>Degraded</c> because the decode loop stamps
/// health per item in chunk order, so the good item after the bad one re-stamps it. That is
/// the same last-writer-wins behaviour the per-tag loop has always had; it is pinned here so
/// a change to it is a deliberate one.
/// Health ends <c>Degraded</c>, NOT <c>Healthy</c>. This pin used to assert the opposite:
/// the decode loop stamped health per item in chunk order, so the good item after the bad
/// one re-stamped it and a pass with failures reported <c>Healthy</c> — last-writer-wins.
/// That was deliberately replaced (perf follow-ups round 3) by <b>worst-state-wins</b>: the
/// whole read pass accumulates its outcomes and writes health ONCE at the end, so a mixed
/// pass can no longer report Healthy. Only the driver-level health claim changed — the
/// per-tag statuses below are exactly as they were, because a per-ITEM decode fault still
/// spoils only its own tag.
/// </remarks>
[Fact]
public async Task One_undecodable_multi_var_item_stamps_only_its_own_tag_Bad()
@@ -420,7 +424,135 @@ public sealed class S7SpanReadParityTests
factory.MultiVarCalls.ShouldBe(1);
factory.StringReadCalls.ShouldBe(0);
factory.ReadBytesCalls.ShouldBe(0);
driver.GetHealth().State.ShouldBe(DriverState.Healthy);
driver.GetHealth().State.ShouldBe(DriverState.Degraded);
}
// ── Worst-state-wins read-pass health ────────────────────────────────────────────────
/// <summary>
/// A pass with BOTH failures and successes reports <c>Degraded</c> — the failure is not
/// erased by the later success — AND still advances <c>LastSuccessfulRead</c>, because
/// something genuinely was read.
/// </summary>
/// <remarks>
/// Advancing the timestamp on a Degraded pass is load-bearing, not cosmetic: the
/// comms-loss watchdog (CLAUDE.md §"Comms-loss detection") trips on non-Healthy state
/// <b>AND</b> a frozen <c>LastSuccessfulRead</c>. Several drivers latch Degraded off one
/// unreadable address, so freezing the timestamp on any non-Healthy pass would reconnect a
/// perfectly reachable PLC every 90 s forever.
/// </remarks>
[Fact]
public async Task A_mixed_read_pass_reports_Degraded_and_still_advances_LastSuccessfulRead()
{
var ct = TestContext.Current.CancellationToken;
var factory = new ImageS7PlcFactory(Images()) { NullItemIndex = 1 };
using var driver = new S7Driver(Options(ScatteredWords(3), maxGap: 2), "s7-mixed-pass", factory);
await driver.InitializeAsync("{}", ct);
// The connect stamped a LastSuccessfulRead; the pass must move it PAST that value, so a
// frozen timestamp is distinguishable from a merely non-null one.
var before = driver.GetHealth().LastSuccessfulRead;
before.ShouldNotBeNull();
var results = await driver.ReadAsync(ScatteredRefs(3), ct);
results[0].StatusCode.ShouldBe(0u);
results[1].StatusCode.ShouldBe(StatusBadCommunicationError);
results[2].StatusCode.ShouldBe(0u);
var health = driver.GetHealth();
health.State.ShouldBe(DriverState.Degraded, "a pass that had a failure must not report Healthy");
health.LastSuccessfulRead.ShouldNotBeNull();
health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(
before!.Value, "two of the three tags read fine — the watchdog's evidence must advance");
}
/// <summary>
/// The SPAN path obeys the same rule: one member that fails to decode leaves the pass
/// <c>Degraded</c> even though a later member of the same block decoded fine.
/// </summary>
/// <remarks>
/// The block read itself succeeds — this is the per-member decode guard, the span-path
/// sibling of the multi-var per-item guard above. The poisoned member is an S7 STRING laid
/// over the image's filler pattern, whose two-byte header declares a current length larger
/// than its capacity; <c>S7String.FromByteArray</c> rejects that. It is deliberately the
/// FIRST member (lower offset) so the good member decodes after it — the exact ordering
/// last-writer-wins used to erase.
/// </remarks>
[Fact]
public async Task One_undecodable_span_member_leaves_the_pass_Degraded()
{
var ct = TestContext.Current.CancellationToken;
// The STRING occupies DB1 bytes 100..111 (StringLength + 2), so the word at 112 is
// adjacent and lands in the SAME span.
var tags = S7RawTags.Entries(
new S7TagDefinition("BadStr", "DB1.DBB100", S7DataType.String, Writable: false, StringLength: 10),
new S7TagDefinition("GoodWord", "DB1.DBW112", S7DataType.UInt16, Writable: false));
var factory = new ImageS7PlcFactory(Images());
using var driver = new S7Driver(Options(tags, maxGap: 16), "s7-span-decode-fault", factory);
await driver.InitializeAsync("{}", ct);
var before = driver.GetHealth().LastSuccessfulRead;
var results = await driver.ReadAsync(["BadStr", "GoodWord"], ct);
// ONE block read served both members — this really is the span path, not two per-tag reads.
factory.ReadBytesCalls.ShouldBe(1);
factory.StringReadCalls.ShouldBe(0);
// Only the poisoned member is Bad; its block-mate keeps its value.
results[0].StatusCode.ShouldNotBe(0u);
results[0].Value.ShouldBeNull();
results[1].StatusCode.ShouldBe(0u);
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.LastSuccessfulRead!.Value.ShouldBeGreaterThan(before!.Value);
}
/// <summary>
/// Faulted outranks BOTH Degraded and Healthy: an access-denied read (the permanent PUT/GET
/// configuration fault) on one tag survives a later device-fault tag AND a later tag that
/// read successfully, all in the same pass.
/// </summary>
/// <remarks>
/// The ordering is Healthy &lt; Degraded &lt; Faulted, so the config alert — the one state
/// an operator must act on — is precisely the one last-writer-wins was most likely to
/// erase, since the denied tag is usually a minority of the batch. Both later outcomes are
/// present deliberately: the success falsifies "any success clears the failure", the
/// Degraded falsifies "the last failure wins". <c>LastSuccessfulRead</c> still advances,
/// because one tag really was read.
/// </remarks>
[Fact]
public async Task An_access_denied_tag_keeps_the_pass_Faulted_even_after_a_later_tag_succeeds()
{
var ct = TestContext.Current.CancellationToken;
var factory = new ImageS7PlcFactory(Images());
// A: "not allowed" is what IsAccessDenied matches — PUT/GET disabled on an S7-1200/1500,
// 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");
// 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);
await driver.InitializeAsync("{}", ct);
var before = driver.GetHealth().LastSuccessfulRead;
var results = await driver.ReadAsync(["A", "B", "C"], ct);
results[0].StatusCode.ShouldNotBe(0u);
results[1].StatusCode.ShouldBe(StatusBadDeviceFailure);
results[2].StatusCode.ShouldBe(0u);
var health = driver.GetHealth();
health.State.ShouldBe(
DriverState.Faulted, "a config alert must outrank a later device fault AND a later success");
health.LastError.ShouldNotBeNull();
// The FIRST failure is the causal one — the later device fault does not overwrite it.
health.LastError!.ShouldContain("PUT/GET");
health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(
before!.Value, "one tag read fine, so the watchdog's evidence must still advance");
}
/// <summary>
@@ -611,6 +743,14 @@ public sealed class S7SpanReadParityTests
/// <summary>When set, only the MULTI-VAR read throws this — the per-tag fallback still serves the image.</summary>
public Exception? MultiVarFault { get; init; }
/// <summary>
/// Per-address read faults, keyed by the address string (e.g. <c>DB1.DBW0</c>): only
/// those addresses throw, every other one still serves the image. Lets a test put
/// faults of DIFFERENT severities and successes in ONE pass, which
/// <see cref="ReadFault"/> (all-or-nothing) cannot express.
/// </summary>
public Dictionary<string, Exception> PerTagFaults { get; } = new(StringComparer.OrdinalIgnoreCase);
/// <summary>
/// When true, <c>ReadBytesAsync</c> hangs until its token is cancelled, so cancellation is
/// the only way out of a block read (STAB-15a). Settable so a test can turn it back off
@@ -745,6 +885,7 @@ public sealed class S7SpanReadParityTests
public Task<object?> ReadAsync(string address, CancellationToken ct)
{
Interlocked.Increment(ref owner.StringReadCalls);
if (owner.PerTagFaults.TryGetValue(address, out var aimed)) throw aimed;
if (owner.ReadFault is { } fault) throw fault;
var addr = S7AddressParser.Parse(address);
var image = owner.Images[(ToS7NetArea(addr.Area), addr.DbNumber)];