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.
This commit is contained in:
Joseph Doherty
2026-08-05 06:48:49 -04:00
parent b29623cf99
commit b833dda673
2 changed files with 91 additions and 2 deletions
@@ -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.
/// </para>
/// <para>
/// <b>Every formatting throw is caught, not just the documented conversion ones.</b> The
/// rendering runs the value's own <see cref="object.ToString"/> /
/// <see cref="IFormattable"/>, which is arbitrary code — and this runs OUTSIDE
/// <see cref="WriteAsync"/>'s per-slot <c>try</c>, so anything that escapes here escapes
/// the whole batch and breaks its never-throws contract. It costs its own slot a
/// <c>BadTypeMismatch</c> and is logged.
/// </para>
/// </remarks>
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;
}
@@ -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");
}
/// <summary>A value whose <see cref="ToString"/> throws something no conversion filter would name.</summary>
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");
}
/// <summary>Cancels the batch from inside its own rendering, then renders normally.</summary>
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()
{