From b833dda673a8c0040ecefba3655b0ae9473d89b1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 5 Aug 2026 06:48:49 -0400 Subject: [PATCH] fix(mtconnect): a throwing value formatter cannot escape the write batch TryFormatValue's catch filter named only FormatException / InvalidCastException / OverflowException, and the call sits OUTSIDE WriteAsync's per-slot try. But Convert.ToString falls back to the value's OWN ToString/IFormattable - arbitrary caller code that may throw anything - so any other exception escaped WriteAsync uncaught and failed the entire batch, breaking the documented never-throws per-slot contract. Broadened to a catch-all; the slot gets BadTypeMismatch and the exception is logged at Warning (TryFormatValue became an instance method to reach _logger). One unformattable value now costs its own slot and nothing more. Falsification checked, not assumed: restoring the narrow filter makes the new test fail with a raw "System.InvalidOperationException : this formatter is broken" propagating out of WriteAsync - i.e. it fails for the contract-breach reason, not on an assertion. Two tests added: - a value whose ToString throws InvalidOperationException is refused BadTypeMismatch while the *following* good slot still writes (the load-bearing half - it proves the batch survived). - mid-batch cancellation: cancellation injected from inside slot 1's own formatting, so slot 0 has provably already reached the Agent. It keeps its real Good outcome while slots 1-2 fill BadCommunicationError. Reporting a landed write as failed is worse than reporting a failure, and a fill that started at 0 instead of i would do exactly that. Tests: 534 passed / 0 failed (was 532). Solution build: 0 errors, no warnings in any MTConnect project. --- .../MTConnectDriver.cs | 24 ++++++- .../MTConnectWriteTests.cs | 69 +++++++++++++++++++ 2 files changed, 91 insertions(+), 2 deletions(-) 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 d8324306..1a2e8285 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -895,8 +895,16 @@ public sealed class MTConnectDriver /// happily, making the corruption invisible until someone reads the observation back. /// TIME_SERIES / DATA_SET writes are out of scope for v1. /// + /// + /// Every formatting throw is caught, not just the documented conversion ones. The + /// rendering runs the value's own / + /// , which is arbitrary code — and this runs OUTSIDE + /// 's per-slot try, so anything that escapes here escapes + /// the whole batch and breaks its never-throws contract. It costs its own slot a + /// BadTypeMismatch and is logged. + /// /// - private static bool TryFormatValue(object? value, out string text) + private bool TryFormatValue(object? value, out string text) { text = string.Empty; @@ -912,8 +920,20 @@ public sealed class MTConnectDriver { rendered = Convert.ToString(value, CultureInfo.InvariantCulture); } - catch (Exception ex) when (ex is FormatException or InvalidCastException or OverflowException) + catch (Exception ex) { + // Catch-all, and deliberately so. Convert.ToString falls back to the value's OWN + // ToString/IFormattable, which is arbitrary caller code that may throw ANYTHING — and this + // call sits outside WriteAsync's per-slot try, so a narrower filter (FormatException / + // InvalidCastException / OverflowException) let every other exception escape WriteAsync + // uncaught and break the documented never-throws contract for the whole batch. One + // unformattable value must cost its own slot, nothing more. + _logger.LogWarning( + ex, + "MTConnect driver {DriverInstanceId} could not render a write value of type {ValueType} to its invariant string form; the write is refused as a type mismatch.", + _driverInstanceId, + value.GetType()); + return false; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs index 89b30f14..936f3b6b 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectWriteTests.cs @@ -191,6 +191,36 @@ public sealed class MTConnectWriteTests client.Writes.ShouldBeEmpty(); } + [Fact] + public async Task A_value_whose_own_formatter_throws_costs_its_slot_and_nothing_more() + { + // Convert.ToString runs the value's OWN ToString — arbitrary code that may throw anything — + // and it runs OUTSIDE WriteAsync's per-slot try. A narrow catch filter let every exception + // other than Format/InvalidCast/Overflow escape WriteAsync and fail the WHOLE batch, breaking + // the never-throws contract. The load-bearing half of this test is the second slot: the good + // write still happens. + var (driver, client) = await WritableDriverAsync(); + + var results = await driver.WriteAsync( + [ + new WriteRequest(PositionRawPath, new ThrowingFormatter()), + new WriteRequest(PositionRawPath, 42.5), + ], + Ct); + + results.Count.ShouldBe(2); + results[0].StatusCode.ShouldBe(BadTypeMismatch); + results[1].StatusCode.ShouldBe(Good); + client.Writes.ShouldHaveSingleItem().Value.ShouldBe("42.5"); + } + + /// A value whose throws something no conversion filter would name. + private sealed class ThrowingFormatter + { + public override string ToString() => + throw new InvalidOperationException("this formatter is broken"); + } + // ---- what actually reaches the seam ---- [Fact] @@ -322,6 +352,45 @@ public sealed class MTConnectWriteTests client.Writes.ShouldBeEmpty(); } + [Fact] + public async Task Cancellation_mid_batch_leaves_the_slots_already_written_with_their_real_outcomes() + { + // The fill must start at the cancelled slot, not overwrite the batch. Slot 0 completed against + // the Agent before the token was cancelled, and reporting it BadCommunicationError would tell + // an operator a write failed that actually landed — the one thing worse than reporting a + // failure. Cancellation is injected from inside slot 1's own formatting, so the ordering is a + // fact of the run rather than a timing hope. + var (driver, client) = await WritableDriverAsync(); + using var cts = new CancellationTokenSource(); + + var results = await driver.WriteAsync( + [ + new WriteRequest(PositionRawPath, 42.5), + new WriteRequest(PositionRawPath, new CancellingFormatter(cts)), + new WriteRequest(PositionRawPath, 7.5), + ], + cts.Token); + + results.Count.ShouldBe(3); + results[0].StatusCode.ShouldBe(Good); + results[1].StatusCode.ShouldBe(BadCommunicationError); + results[2].StatusCode.ShouldBe(BadCommunicationError); + + // Exactly the pre-cancellation slot reached the Agent. + client.Writes.ShouldHaveSingleItem().Value.ShouldBe("42.5"); + } + + /// Cancels the batch from inside its own rendering, then renders normally. + private sealed class CancellingFormatter(CancellationTokenSource cts) + { + public override string ToString() + { + cts.Cancel(); + + return "99"; + } + } + [Fact] public async Task One_bad_request_does_not_fail_the_others_in_the_batch() {