From 1765e30063488f4983019386b9bb7267c444f3ce Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 18 Aug 2026 07:10:51 -0400 Subject: [PATCH] fix(s7): worst-severity error message + OCE flushes recorded pass evidence (review round) --- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 70 ++++++++++---- .../S7SpanReadParityTests.cs | 94 +++++++++++++++++++ 2 files changed, 146 insertions(+), 18 deletions(-) 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 60926e7c..bed5e1bc 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -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; /// - /// 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 at the worst severity — the one that explains + /// . /// - public string? FirstError { get; private set; } + /// + /// 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. + /// + public string? WorstError { get; private set; } + + /// + /// 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. + /// + public bool HasObservations => AnySuccess || WorstFailure != DriverState.Healthy; /// Records that one tag read successfully. public void RecordSuccess() => AnySuccess = true; - /// Records one failed tag, keeping the worst state and the first message. + /// + /// Records one failed tag, keeping the worst state and the message that explains it. + /// /// The state this failure would have stamped on its own. /// The failure's operator-facing message. 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; + } } /// Severity order the pass ranks by: Healthy (none) < Degraded < Faulted. 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 454a2798..d6d752c8 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 @@ -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"); } + /// + /// The pass's message must explain the state it reports. When a merely-Degraded fault is + /// observed BEFORE the access-denied one, escalating to Faulted has to re-point + /// LastError at the access-denied cause — reporting the config alert under the + /// earlier device-fault message tells the operator to chase the wrong thing. + /// + /// + /// This is the reversed ordering of + /// , + /// 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. + /// + [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); + } + + /// + /// 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 + /// LastSuccessfulRead. + /// + /// + /// 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 LastSuccessfulRead 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 finally is what prevents it. + /// + [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(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"); + } + /// /// 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 Faulted config