From 908bd7ba4ae3b5973640f24be0be6e87753ecb9a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 24 Jul 2026 15:09:42 -0400 Subject: [PATCH] fix(mtconnect): deterministic concurrency test + DATA_SET -> BadNotSupported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The concurrency test asserted a post-Apply post-condition from inside the race window: dev1_pos is authored, so a reader beating the first Apply correctly observes BadWaitingForInitialData, which the test's two-member "legal" set omitted. Consistently red (10/10) rather than intermittent. Rewritten to assert only properties that hold at every instant — the whole (status, value, source timestamp) triple against the three legal snapshots, which is the real torn-snapshot check — plus a read counter guarding a vacuous pass and one deterministic closing Apply for an exact end state. The index is unchanged here; BadWaitingForInitialData is a designed state. Also codes structured (DATA_SET / TABLE) observations BadNotSupported now that the parser supplies MTConnectObservation.IsStructured, so a two-entry data set no longer surfaces as the Good string "12". --- .../MTConnectObservationIndex.cs | 37 +++-- .../MTConnectObservationIndexTests.cs | 149 ++++++++++++++++-- 2 files changed, 156 insertions(+), 30 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectObservationIndex.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectObservationIndex.cs index 29703f0f..c68a870c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectObservationIndex.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectObservationIndex.cs @@ -48,20 +48,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.MTConnect; /// would serialize the pump against every read for no semantic gain. /// /// -/// KNOWN LIMITATION — DATA_SET / TABLE observations. Their real content -/// is <Entry key=…> child elements, but the parser reads an observation's value -/// with XElement.Value, which concatenates all descendant text. Such an observation -/// therefore reaches this index as a run-together string with the keys discarded, and — -/// because the type inference correctly demotes DATA_SET/TABLE to -/// — it coerces "successfully" into a Good-coded -/// snapshot carrying noise. This index cannot detect the shape: neither -/// nor carries the -/// DataItem's representation, and no value-shape heuristic can separate a -/// concatenated entry list from a legitimate space-bearing EVENT (a Message reads -/// "Coolant level low"). The discriminating signal — the observation element having child -/// elements — exists only in , so the fix belongs there -/// (flag structured observations at parse time; this index then codes them -/// BadNotSupported alongside the TIME_SERIES deferral below). +/// Shapes this build does not represent are coded BadNotSupported with a null +/// value rather than approximated: a TIME_SERIES vector (array materialization is +/// deferred to P1.5) and a DATA_SET / TABLE observation. The latter carries its +/// real content in <Entry key=…> child elements, which reach a text-valued +/// observation concatenated with the keys discarded — a two-entry set reads as the single +/// token "12" — and because the type inference correctly demotes those +/// representations to , coercion would otherwise +/// "succeed" into a Good-coded snapshot carrying noise. This index cannot detect that shape +/// itself (no value heuristic can separate concatenated entries from a legitimate +/// space-bearing EVENT such as a Message reading "Coolant level low"), so it relies on +/// , which +/// sets while the XML is still XML. /// /// internal sealed class MTConnectObservationIndex @@ -257,6 +255,17 @@ internal sealed class MTConnectObservationIndex return new DataValueSnapshot(null, BadNoCommunication, sourceTimestamp, serverTimestamp); } + // DATA_SET / TABLE: the Agent carried the content in child elements, so + // Value is their concatenated text with the keys discarded (a two-entry set reads "12"). + // Publishing that as a Good string would be noise an operator would trust. Checked before + // the tag lookup because it is a fact about the wire shape, true whether or not the data + // item is authored — and after the sentinel, so an unavailable data set still reports the + // truthful no-comms. + if (observation.IsStructured) + { + return new DataValueSnapshot(null, BadNotSupported, sourceTimestamp, serverTimestamp); + } + _tags.TryGetValue(observation.DataItemId, out var tag); // TIME_SERIES vectors arrive as one space-separated string. P1 does not materialize OPC UA diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectObservationIndexTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectObservationIndexTests.cs index 205dec2a..253a491f 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectObservationIndexTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Tests/MTConnectObservationIndexTests.cs @@ -47,7 +47,8 @@ public sealed class MTConnectObservationIndexTests MTConnectStreamsParser.Parse(File.ReadAllText(path)); /// Wraps one synthetic observation in the minimal legal streams result. - private static MTConnectStreamsResult Synthetic(string dataItemId, string value, DateTime? timestampUtc = null) => + private static MTConnectStreamsResult Synthetic( + string dataItemId, string value, DateTime? timestampUtc = null, bool isStructured = false) => new( InstanceId: 1L, NextSequence: 2L, @@ -55,7 +56,8 @@ public sealed class MTConnectObservationIndexTests Observations: [new MTConnectObservation( dataItemId, value, - timestampUtc ?? new DateTime(2026, 7, 24, 12, 0, 0, DateTimeKind.Utc))]); + timestampUtc ?? new DateTime(2026, 7, 24, 12, 0, 0, DateTimeKind.Utc), + isStructured)]); private static MTConnectTagDefinition Tag(string id, DriverDataType type) => new(id, type); @@ -475,6 +477,92 @@ public sealed class MTConnectObservationIndexTests snap.Value.ShouldBeOfType().ShouldBe("1.1 1.2 1.3 1.4 1.5 1.6 1.7 1.8 1.9 2.0"); } + // --------------------------------------------------------------------- DATA_SET / TABLE + + /// + /// A structured (DATA_SET / TABLE) observation's real content is its <Entry> + /// children, which reach the index concatenated with the keys discarded. It is coded + /// BadNotSupported rather than published as that noise — regardless of whether the data item + /// is authored, because the shape is a fact about the wire, not about the tag. + /// + [Fact] + public void Structured_observation_reports_BadNotSupported_when_unauthored() + { + var idx = new MTConnectObservationIndex(); + idx.Apply(Synthetic("ds1", "12", isStructured: true)); + + var snap = idx.Get("ds1"); + + snap.StatusCode.ShouldBe(BadNotSupported); + snap.Value.ShouldBeNull(); + } + + /// The String authoring the type inference gives a DATA_SET must not smuggle the noise through. + [Fact] + public void Structured_observation_is_never_published_as_concatenated_text() + { + var idx = new MTConnectObservationIndex([Tag("ds1", DriverDataType.String)]); + idx.Apply(Synthetic("ds1", "12", isStructured: true)); + + var snap = idx.Get("ds1"); + + snap.Value.ShouldNotBe("12"); + snap.Value.ShouldBeNull(); + snap.StatusCode.ShouldBe(BadNotSupported); + } + + /// No-comms outranks the unsupported shape, exactly as it does for a TIME_SERIES array. + [Fact] + public void Unavailable_wins_over_the_structured_shape() + { + var idx = new MTConnectObservationIndex([Tag("ds1", DriverDataType.String)]); + idx.Apply(Synthetic("ds1", "UNAVAILABLE", isStructured: true)); + + idx.Get("ds1").StatusCode.ShouldBe(BadNoCommunication); + } + + /// + /// End-to-end through the real parser: the entry list that concatenates to the single token + /// "12" is caught by the parser's structured flag and never surfaces as a Good value. This is + /// the leg that would regress silently if the parser stopped setting the flag. + /// + [Fact] + public void A_parsed_data_set_observation_never_surfaces_as_a_good_value() + { + const string xml = """ + +
+ + + 1 + 2 + + + + """; + + var idx = new MTConnectObservationIndex([Tag("ds1", DriverDataType.String)]); + idx.Apply(MTConnectStreamsParser.Parse(xml)); + + var snap = idx.Get("ds1"); + + snap.StatusCode.ShouldBe(BadNotSupported); + snap.Value.ShouldBeNull(); + } + + /// An ordinary text observation is not structured — the flag must not be over-applied. + [Fact] + public void An_ordinary_observation_is_not_treated_as_structured() + { + var idx = new MTConnectObservationIndex([Tag("dev1_program", DriverDataType.String)]); + idx.Apply(ParseFixture(CurrentFixture)); + + var snap = idx.Get("dev1_program"); + + snap.StatusCode.ShouldBe(Good); + snap.Value.ShouldBe("O1234"); + } + // -------------------------------------------------------------------------------- ordering /// Within one Apply, the later observation for a dataItemId wins (Agent order is ascending sequence). @@ -652,8 +740,23 @@ public sealed class MTConnectObservationIndexTests /// /// /current (Task 10) and the /sample pump (Task 11) write concurrently while OPC UA reads - /// snapshot. Per-key atomicity is the contract: no throw, no torn snapshot (a Good value - /// paired with a Bad code), and every read observes one whole snapshot. + /// snapshot. Three properties, each stated so it holds at every instant rather than + /// only once the race settles: no call throws; no read observes a torn snapshot; and the + /// state after a deterministic closing Apply is exact. + /// + /// Torn-snapshot check. The whole (status, value, source timestamp) triple + /// is matched against the closed set of snapshots dev1_pos may legally present — + /// never a field in isolation. A Good code beside the unavailable document's timestamp, + /// or beside a null value, matches no member and fails. + /// + /// + /// The legal set has THREE members, not two. dev1_pos is authored, so a + /// reader that beats the first Apply correctly observes BadWaitingForInitialData — + /// the designed "authored, not yet reported" state. An earlier version of this test + /// asserted a post-Apply post-condition from inside the race window and failed whenever + /// a reader got there first: it was asserting something the index never promised, and it + /// verified nothing its name claimed. + /// /// [Fact] public async Task Concurrent_applies_and_reads_stay_consistent_and_never_throw() @@ -662,14 +765,24 @@ public sealed class MTConnectObservationIndexTests var current = ParseFixture(CurrentFixture); var unavailable = ParseFixture(UnavailableFixture); + var goodAt = new DateTime(2026, 7, 24, 12, 0, 0, 200, DateTimeKind.Utc); + (uint Status, object? Value, DateTime? Source)[] legalSnapshots = + [ + (BadWaitingForInitialData, null, null), + (Good, 123.4567, goodAt), + (BadNoCommunication, null, new DateTime(2026, 7, 24, 12, 10, 0, 100, DateTimeKind.Utc)), + ]; + + var reads = 0L; using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + var writers = Enumerable.Range(0, 4).Select(i => Task.Run(() => { while (!cts.IsCancellationRequested) { idx.Apply(i % 2 == 0 ? current : unavailable); } - })); + })).ToArray(); var readers = Enumerable.Range(0, 4).Select(_ => Task.Run(() => { @@ -677,21 +790,25 @@ public sealed class MTConnectObservationIndexTests { var snap = idx.Get("dev1_pos"); - // Exactly two snapshots are legal for this tag; anything else is a tear. - if (snap.StatusCode == 0u) - { - snap.Value.ShouldBe(123.4567); - } - else - { - snap.StatusCode.ShouldBe(BadNoCommunication); - snap.Value.ShouldBeNull(); - } + legalSnapshots.ShouldContain((snap.StatusCode, snap.Value, snap.SourceTimestampUtc)); + Interlocked.Increment(ref reads); } - })); + })).ToArray(); await Task.WhenAll(writers.Concat(readers)); + // Guards a vacuous pass: a reader loop that never ran would have asserted nothing. + Interlocked.Read(ref reads).ShouldBeGreaterThan(0L); + + // The racing writers alternate documents, so the state DURING the race is unknowable by + // construction — pinning a specific one there is exactly the defect this test used to have. + // One deterministic apply, after every writer has stopped, gives an exact end state. + idx.Apply(current); + + var final = idx.Get("dev1_pos"); + final.StatusCode.ShouldBe(Good); + final.Value.ShouldBe(123.4567); + final.SourceTimestampUtc.ShouldBe(goodAt); idx.Count.ShouldBe(AllFixtureDataItemIds.Length); }