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

fix(s7): worst-severity error message + OCE flushes recorded pass evidence (review round)

Browse Source
This commit is contained in:
Joseph Doherty
2026-08-18 07:10:51 -04:00
parent f4b91dbbab
commit 1765e30063
2 changed files with 146 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs
+52 -18
View File
@@ -666,25 +666,33 @@ public sealed class S7Driver
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.
}
finally
{
// THE single health write of the pass — worst-state-wins over every outcome recorded
// above. In a FINALLY, so evidence the pass already gathered survives an abandoned
// batch: the three OCE arms rethrow mid-pass, and on a partially-working device a
// repeatedly-cancelled poll would otherwise freeze LastSuccessfulRead while tags were
// demonstrably still being read — tripping the comms-loss watchdog into a reconnect
// loop, the exact false positive the AnySuccess design exists to prevent.
//
// A pass that observed NOTHING — an empty batch, or one whose every reference was
// unresolvable — reports nothing: it writes no health at all, preserving a prior
// Faulted (e.g. the access-denied config alert) that a Healthy write would have erased.
if (outcome.AnySuccess || outcome.WorstFailure != DriverState.Healthy)
// A pass that observed NOTHING reports nothing (HasObservations), which is also what
// makes flushing here safe on the two paths that reach the finally with no outcomes:
// the connect-failure early return, whose own Degraded write must NOT be overwritten,
// and an OCE that fired before any tag was stamped. Both no-op.
//
// LastSuccessfulRead advances whenever ANYTHING read, even into a Degraded/Faulted
// state — see ReadPassOutcome for why that is load-bearing.
if (outcome.HasObservations)
{
_health = new DriverHealth(
outcome.WorstFailure,
outcome.AnySuccess ? now : _health.LastSuccessfulRead,
outcome.WorstFailure == DriverState.Healthy ? null : outcome.FirstError);
outcome.WorstFailure == DriverState.Healthy ? null : outcome.WorstError);
}
_gate.Release();
}
finally { _gate.Release(); }
return results;
}
@@ -726,21 +734,47 @@ public sealed class S7Driver
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.
/// The first message recorded <b>at the worst severity</b> — the one that explains
/// <see cref="WorstFailure"/>.
/// </summary>
public string? FirstError { get; private set; }
/// <remarks>
/// It must move WITH the state, not independently of it. Keeping the pass's
/// chronologically-first message instead desynced the pair whenever a Degraded fault
/// preceded a Faulted one: the driver reported the access-denied config alert while its
/// message talked about some unrelated device fault and never mentioned PUT/GET, which
/// is the one thing the operator has to act on. Within a severity the FIRST message
/// still wins — a later peer failure is as likely to be a consequence of it as an
/// independent fault.
/// </remarks>
public string? WorstError { get; private set; }
/// <summary>
/// True once the pass observed anything at all — a success or a failure. False for a
/// degenerate pass (empty batch, or every reference unresolvable), which must report
/// nothing rather than invent a verdict.
/// </summary>
public bool HasObservations => AnySuccess || WorstFailure != DriverState.Healthy;
/// <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>
/// <summary>
/// Records one failed tag, keeping the worst state and the message that explains it.
/// </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;
if (Rank(state) > Rank(WorstFailure))
{
// Escalating the severity MUST re-point the message at the new cause.
WorstFailure = state;
WorstError = message;
}
else
{
WorstError ??= message;
}
}
/// <summary>Severity order the pass ranks by: Healthy (none) &lt; Degraded &lt; Faulted.</summary>
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7SpanReadParityTests.cs
+94
View File
@@ -489,6 +489,16 @@ public sealed class S7SpanReadParityTests
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());
// The poison is the FILLER, not a fake knob: Images() writes (i * 37 + 11) & 0xFF, which
// steps +37 per byte and so runs ASCENDING except in the ~15% of positions where it wraps
// past 256. Byte 100 is not one of those, so the STRING header at 100/101 reads
// maxLen 127 / curLen 164 — a current length larger than the capacity, which
// S7String.FromByteArray rejects. Asserted rather than assumed, so a change to the filler
// pattern fails HERE, loudly, instead of silently turning this into a passing no-op test.
var db1Image = factory.Images[(S7NetDataType.DataBlock, 1)];
db1Image[101].ShouldBeGreaterThan(
db1Image[100], "the S7 STRING at DB1.DBB100 must declare curLen > maxLen to be undecodable");
using var driver = new S7Driver(Options(tags, maxGap: 16), "s7-span-decode-fault", factory);
await driver.InitializeAsync("{}", ct);
@@ -555,6 +565,90 @@ public sealed class S7SpanReadParityTests
before!.Value, "one tag read fine, so the watchdog's evidence must still advance");
}
/// <summary>
/// The pass's message must explain the state it reports. When a merely-Degraded fault is
/// observed BEFORE the access-denied one, escalating to <c>Faulted</c> has to re-point
/// <c>LastError</c> at the access-denied cause — reporting the config alert under the
/// earlier device-fault message tells the operator to chase the wrong thing.
/// </summary>
/// <remarks>
/// This is the reversed ordering of
/// <see cref="An_access_denied_tag_keeps_the_pass_Faulted_even_after_a_later_tag_succeeds"/>,
/// and the pair is the point: keeping the pass's chronologically-first message satisfies
/// that test and fails this one, so only a message that moves WITH the severity passes both.
/// </remarks>
[Fact]
public async Task Escalating_to_Faulted_re_points_the_error_message_at_the_access_denied_cause()
{
var ct = TestContext.Current.CancellationToken;
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.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);
var before = driver.GetHealth().LastSuccessfulRead;
var results = await driver.ReadAsync(["A", "B", "C"], ct);
results[0].StatusCode.ShouldBe(StatusBadDeviceFailure);
results[1].StatusCode.ShouldNotBe(0u);
results[2].StatusCode.ShouldBe(0u);
var health = driver.GetHealth();
health.State.ShouldBe(DriverState.Faulted, "the access-denied fault must still outrank the earlier one");
health.LastError.ShouldNotBeNull();
health.LastError!.ShouldContain(
"PUT/GET",
customMessage: "the message must explain the FAULTED state, not the earlier Degraded one");
health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(before!.Value);
}
/// <summary>
/// Cancellation mid-pass abandons the batch but must NOT discard the evidence the pass had
/// already gathered: a tag that read successfully before the cancellation still advances
/// <c>LastSuccessfulRead</c>.
/// </summary>
/// <remarks>
/// Deciding health once at the end of the pass makes this a real risk, which per-item
/// writes did not have. On a partially-working device a repeatedly-cancelled poll would
/// freeze <c>LastSuccessfulRead</c> while tags were demonstrably still being read, and the
/// comms-loss watchdog — which trips on non-Healthy state AND a frozen timestamp — would
/// reconnect a CPU that is answering. Flushing from a <c>finally</c> is what prevents it.
/// </remarks>
[Fact]
public async Task A_cancelled_pass_still_flushes_the_evidence_it_had_already_gathered()
{
var ct = TestContext.Current.CancellationToken;
// Narrow first (a per-address read, which the hang knob does NOT intercept), then a wide
// Int64 (a BLOCK read, which it does) — so the pass banks one success and is then cancelled
// mid-flight on the very next tag.
var tags = S7RawTags.Entries(
new S7TagDefinition("Narrow", "DB1.DBW0", S7DataType.UInt16, Writable: false),
new S7TagDefinition("Wide", "DB1.DBB8", S7DataType.Int64, Writable: false));
var factory = new ImageS7PlcFactory(Images()) { HangReadBytes = true };
// maxGap -1 keeps both tags on the per-tag path, in reference order.
using var driver = new S7Driver(Options(tags, maxGap: -1), "s7-cancel-flush", factory);
await driver.InitializeAsync("{}", ct);
var before = driver.GetHealth().LastSuccessfulRead;
using var cts = new CancellationTokenSource();
var reading = driver.ReadAsync(["Narrow", "Wide"], cts.Token);
await factory.ReadBytesEntered.Task.WaitAsync(TimeSpan.FromSeconds(10), ct);
await cts.CancelAsync();
await Should.ThrowAsync<OperationCanceledException>(async () => await reading);
// The first tag really was served before the cancellation — one per-address read happened.
factory.StringReadCalls.ShouldBe(1);
var health = driver.GetHealth();
health.LastSuccessfulRead!.Value.ShouldBeGreaterThan(
before!.Value, "the banked success must survive the abandoned batch");
}
/// <summary>
/// A pass that observes NOTHING — every reference unresolvable, so not one tag was read and
/// not one failed — reports nothing: it must leave the driver's prior <c>Faulted</c> config