From b2c064bec3b23ca617cdc06a01503a4f088a3bd3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 16:31:26 -0400 Subject: [PATCH] fix(mtconnect): an unanswered write degrades; one success writer for reads and writes (review I-3, m7) Coordinator ruling on F3: a write with no reply (transport failure / deadline) sets Degraded with the stamp preserved and a type-only LastError, through a new _writeDegraded flag folded into PublishHealthy. One success writer is shared: RecordReadAnswered (a /current answer, an Agent refusal, a live-index read) clears the read AND write degradations; RecordWriteAnswered clears the write one; neither clears the other path's stream degradation. Tests both directions (answered write clears it, good read clears it) plus the converse (an answered write does not clear a failed read); red with each half removed. m7: a good /current resets the refusal-dedup latch, so a refusal recurring after a recovery is logged again (tested). Refs #743 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- docs/drivers/MTConnect.md | 8 ++- .../MTConnectDriver.cs | 61 ++++++++++++++----- .../MTConnectBatchedWriteTests.cs | 61 +++++++++++++++++++ .../MTConnectReadPathTests.cs | 30 +++++++++ 4 files changed, 144 insertions(+), 16 deletions(-) diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index 0edc52f3..9ff329d1 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -429,8 +429,12 @@ different `level|text` wire format, and are out of scope for v1. `DriverHealth.LastError`, which is type-only by repo-wide discipline. A refused write is not evidence the read path degraded, but it **is** the Agent answering: any answer — accepted or refused — advances `LastSuccessfulRead`, with the state following the read / `/sample` flags -(a write cannot clear either path's degradation). A write that got **no** answer leaves health -to those two paths, which own liveness. +(a write cannot clear either of those paths' degradation). A write that got **no** answer +(transport failure, deadline) sets **Degraded** with the stamp preserved and a type-only +`LastError` (review 2026-10-21 I-3, coordinator ruling). One success writer is shared: that +degradation is cleared by the next answered **write** or the next good **read** (a `/current` +answer or a live-index read), so it cannot stick on a driver still talking to the Agent; a +`/sample` chunk alone does not clear it. **One request per device (review 2026-10-21 T-6).** The slots that survive the refusals above go out as one `POST {agent}/{device}?id1=v1&id2=v2…` per device (the Agent's PUT surface takes diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs index ed5a4c2e..4c5164de 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -369,6 +369,13 @@ public sealed partial class MTConnectDriver /// Whether the /current read path is currently failing. See . private volatile bool _readDegraded; + /// + /// Whether the last Agent write got no reply (review I-3 / coordinator ruling: an unanswered write degrades). + /// Cleared by the next answered read OR write ( / ), + /// so it cannot latch on a driver that is still talking to the Agent. + /// + private volatile bool _writeDegraded; + /// The last /current refusal text logged, so a persistent refusal warns once, not per read. private string? _lastReadRefusal; @@ -761,8 +768,9 @@ public sealed partial class MTConnectDriver results[i] = snapshot.Get(binding.ToDataItemId(fullReferences[i])); } - _readDegraded = false; - PublishHealthy(DateTime.UtcNow); + // m7: the next refusal after a recovery is news again. + Volatile.Write(ref _lastReadRefusal, null); + RecordReadAnswered(DateTime.UtcNow); return results; } @@ -779,8 +787,7 @@ public sealed partial class MTConnectDriver // scope, an unsupported request) — it answered. The batch is Bad, but this is not a comms fault: the // read path is not degraded by it (that would reconnect-loop a driver over a config problem), the stamp // advances, and the Agent's text is logged once per distinct refusal. - _readDegraded = false; - PublishHealthy(DateTime.UtcNow); + RecordReadAnswered(DateTime.UtcNow); if (!string.Equals(Interlocked.Exchange(ref _lastReadRefusal, ex.Message), ex.Message, StringComparison.Ordinal)) { _logger.LogWarning( @@ -830,10 +837,9 @@ public sealed partial class MTConnectDriver // The read path is answering (from a stream that is delivering), so a degradation a failed /current left // behind is no longer true of it. The stamp stays the pump's — this read heard nothing from the Agent itself. - if (_readDegraded) + if (_readDegraded || _writeDegraded) { - _readDegraded = false; - PublishHealthy(ReadHealth().LastSuccessfulRead ?? DateTime.UtcNow); + RecordReadAnswered(ReadHealth().LastSuccessfulRead ?? DateTime.UtcNow); } return results; @@ -868,7 +874,7 @@ public sealed partial class MTConnectDriver // nowhere near LastError, which is type-only by repo-wide discipline. A refused write is // not evidence the read path degraded — but it IS the Agent answering, so any answer // (accepted or refused) advances LastSuccessfulRead with the state following the read / - // stream flags; a write that got no answer leaves health to those paths, which own liveness. + // stream flags; a write that got no answer degrades (stamp preserved) until the next answered read or write. // Slots that survive the local refusals go out as ONE request per device (R06:T-6). // is null — a caller bug. public async Task> WriteAsync( @@ -1017,8 +1023,8 @@ public sealed partial class MTConnectDriver catch (Exception ex) { // A transport failure, a blown per-call deadline, or a disposed client — no answer arrived. Logged with - // the exception (its TYPE is the diagnosis) and reported per slot; health is left to the read and - // stream paths, which own liveness. + // the exception (its TYPE is the diagnosis), reported per slot, and Degraded with the stamp preserved + // (RecordWriteNoReply) until the next answered read or write. _logger.LogWarning( ex, "MTConnect driver {DriverInstanceId} could not write {Count} DataItem(s) ({DataItemIds}) on device '{DeviceName}' to {AgentUri}.", @@ -1029,6 +1035,8 @@ public sealed partial class MTConnectDriver results[write.Slot] = new WriteResult(BadCommunicationError); } + RecordWriteNoReply(options, ex); + return; } @@ -1063,18 +1071,42 @@ public sealed partial class MTConnectDriver } /// - /// A write got an answer from the Agent: advances, and the state - /// follows the read / stream flags exactly as their own successes do ( — a write - /// cannot clear either path's degradation, but it is evidence the Agent is reachable). + /// A write got an answer from the Agent: advances, a prior + /// unanswered-write degradation clears, and the state follows the read / stream flags (a write cannot clear + /// either of THOSE paths' degradation, but it is evidence the Agent is reachable). /// private void RecordWriteAnswered() { + _writeDegraded = false; if (ReadHealth().State is DriverState.Healthy or DriverState.Degraded) { PublishHealthy(DateTime.UtcNow); } } + /// + /// A write got NO answer (transport failure, deadline): Degraded with the stamp preserved — the health contract + /// for a comms fault. Type-only text. Cleared by the next answered read or write. + /// + private void RecordWriteNoReply(MTConnectDriverOptions options, Exception ex) + { + _writeDegraded = true; + Degrade($"MTConnect write to {AgentDisplay(options)} got no reply ({ex.GetType().Name})."); + } + + /// + /// The ONE success writer the read path and the write path share (review I-3): an answered read clears the + /// read AND write degradations (the Agent is answering requests), then publishes Healthy unless the + /// /sample stream is still failing. + /// + /// The stamp to publish. + private void RecordReadAnswered(DateTime at) + { + _readDegraded = false; + _writeDegraded = false; + PublishHealthy(at); + } + /// /// Renders a value to the invariant string a PUT query carries, or reports that it has no such /// form. @@ -3162,6 +3194,7 @@ public sealed partial class MTConnectDriver // re-initialize is precisely the event that means an operator changed something. _streamDegraded = false; _readDegraded = false; + _writeDegraded = false; WriteHealth(new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); @@ -3619,7 +3652,7 @@ public sealed partial class MTConnectDriver { var current = ReadHealth(); - if (_streamDegraded || _readDegraded) + if (_streamDegraded || _readDegraded || _writeDegraded) { if (current.State != DriverState.Faulted) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBatchedWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBatchedWriteTests.cs index 5ea2268b..cb9c96c3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBatchedWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectBatchedWriteTests.cs @@ -104,4 +104,65 @@ public sealed class MTConnectBatchedWriteTests driver.GetHealth().LastSuccessfulRead.ShouldNotBeNull().ShouldBeGreaterThan(before); driver.GetHealth().State.ShouldBe(DriverState.Healthy); } + + /// Review I-3 (coordinator ruling): a write that got NO reply degrades health, with the stamp preserved. + [Fact] + public async Task A_write_with_no_reply_degrades_and_preserves_the_stamp() + { + var (driver, client) = await WritableAsync(); + var before = driver.GetHealth(); + before.State.ShouldBe(DriverState.Healthy, "positive control: Healthy before the failed write"); + client.WriteFailure = new HttpRequestException("connection refused"); + + var results = await driver.WriteAsync([new WriteRequest(PosRaw, 1.5)], Ct); + + results.Single().StatusCode.ShouldBe(BadCommunicationError); + var h = driver.GetHealth(); + h.State.ShouldBe(DriverState.Degraded); + h.LastSuccessfulRead.ShouldBe(before.LastSuccessfulRead); + h.LastError.ShouldNotBeNull().ShouldContain(nameof(HttpRequestException)); + } + + /// The unanswered-write degradation is cleared by the next ANSWERED write (it cannot latch). + [Fact] + public async Task An_answered_write_clears_an_unanswered_write_degradation() + { + var (driver, client) = await WritableAsync(); + client.WriteFailure = new HttpRequestException("connection refused"); + await driver.WriteAsync([new WriteRequest(PosRaw, 1.5)], Ct); + driver.GetHealth().State.ShouldBe(DriverState.Degraded); + + client.WriteFailure = null; + await driver.WriteAsync([new WriteRequest(PosRaw, 2.5)], Ct); + + driver.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// …and by the next good READ (the other direction of the shared health writer). + [Fact] + public async Task A_good_read_clears_an_unanswered_write_degradation() + { + var (driver, client) = await WritableAsync(); + client.WriteFailure = new HttpRequestException("connection refused"); + await driver.WriteAsync([new WriteRequest(PosRaw, 1.5)], Ct); + driver.GetHealth().State.ShouldBe(DriverState.Degraded); + + (await driver.ReadAsync([PosRaw], Ct)).Single().StatusCode.ShouldBe(Good); + + driver.GetHealth().State.ShouldBe(DriverState.Healthy); + } + + /// The converse: a failed READ is not cleared by an answered write (each path clears only what it proves). + [Fact] + public async Task An_answered_write_does_not_clear_a_failed_read() + { + var (driver, client) = await WritableAsync(); + client.CurrentFailure = new HttpRequestException("connection refused"); + await driver.ReadAsync([PosRaw], Ct); + driver.GetHealth().State.ShouldBe(DriverState.Degraded); + + await driver.WriteAsync([new WriteRequest(PosRaw, 2.5)], Ct); + + driver.GetHealth().State.ShouldBe(DriverState.Degraded); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectReadPathTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectReadPathTests.cs index 8ec06742..265e6906 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectReadPathTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectReadPathTests.cs @@ -228,4 +228,34 @@ public sealed class MTConnectReadPathTests [Fact] public void A_garbage_answer_is_not_an_agent_refusal() => MTConnectXml.IsAgentRefusal(ParseFailure("proxy error")).ShouldBeFalse(); + + /// Review m7: a refusal that recurs AFTER a good read is news again and is logged again. + [Fact] + public async Task A_refusal_recurring_after_a_good_read_is_logged_again() + { + var logger = new RecordingDriverLogger(); + var client = CannedAgentClient.FromFixtures(); + var driver = new MTConnectDriver(Opts(), "mt-refusal-again", _ => client, logger); + await driver.InitializeAsync("{}", Ct); + try + { + var refusal = ParseFailure(NoDeviceError); + client.CurrentFailure = refusal; + await driver.ReadAsync(["dev1_pos"], Ct); + await driver.ReadAsync(["dev1_pos"], Ct); + logger.WarningsSnapshot().Count(w => w.Contains("NO_DEVICE")).ShouldBe(1, "positive control: deduplicated while it persists"); + + client.CurrentFailure = null; + await driver.ReadAsync(["dev1_pos"], Ct); + client.CurrentFailure = refusal; + await driver.ReadAsync(["dev1_pos"], Ct); + + logger.WarningsSnapshot().Count(w => w.Contains("NO_DEVICE")).ShouldBe(2); + } + finally + { + client.CurrentFailure = null; + await driver.ShutdownAsync(CancellationToken.None); + } + } }