fix(galaxy): publish an unrepresentable value as BadTypeMismatch, not a good null
v2-ci / build (push) Successful in 6m23s
v2-ci / unit-tests (push) Failing after 20m2s

Reverses the conservative call from the previous commit at the user's direction,
and they were right: "surface it on a counter first" left the wire lying. An
MXAccess event can carry Good quality while its payload is a kind MxValueDecoder
has no mapping for, so the old behaviour published null-under-Good — which a
client cannot tell apart from a genuine null reading. A value the gateway DID
send would be consumed as a value it did not, with the only evidence in a log
nobody is tailing. Bad data is the honest wire representation of lost data.

BadTypeMismatch is an existing StatusCodeMap constant, deliberately reused rather
than written inline: StatusCodeParityTests finds status constants by reflecting
over Good/Uncertain/Bad-prefixed uint fields and checking them against the pinned
SDK, so an inline literal would be invisible to that guard (Gitea #497, where 16
wrong constants had accumulated across 6 drivers exactly this way).

Logging is now once per DISTINCT value kind rather than once per pump. The set is
bounded by the MxValue oneof enum so a broken producer still cannot flood, but a
second unrepresentable kind appearing later gets its own line — warn-once-per-pump
would have silently swallowed it, which is the same class of bug as the null this
commit fixes.

The test's fixture matters more than its assertion: the undecodable event carries
Quality = 192 (OPC DA Good), so a decoder that gave up silently would publish Good
and fail. Paired with a positive control — a decodable value at the same quality
must still publish Good — so it cannot be satisfied by a pump that marks
everything Bad. Falsified by reverting the status to Good: the test reds, and only
that test.

Verified: 331 Galaxy tests green, Core.Abstractions 298 (the parity guard),
solution builds 0 errors. Docs corrected in the two places that recorded the old
decision as deliberate.
This commit is contained in:
Joseph Doherty
2026-08-12 03:03:51 -04:00
parent 4769044f88
commit c51c59987b
4 changed files with 110 additions and 19 deletions
+1 -1
View File
@@ -112,4 +112,4 @@ The session-less alarm feed (`GatewayGalaxyAlarmFeed`) and alarm acknowledger (`
- **A stream the gateway ends cleanly is treated as a fault.** A closed or evicted gateway session completes the `StreamEvents` RPC *without an error*, so nothing throws. `EventPump` reports that to the `ReconnectSupervisor` exactly as it reports a transport fault. Before this it exited silently: the supervisor was never told, `GetHealth()` kept reporting `Healthy`, and because the [comms-loss watchdog](../DriverLifecycle.md) requires a **non-Healthy** state to trip, it never fired either — the driver went permanently deaf with no signal anywhere. Nothing else in the system can observe this, so do not remove the notification on the grounds that "a clean end isn't an error".
- **Replay gaps** (`galaxy.events.replay_gaps`): the gateway signals that events were evicted from its replay ring before we could read them. The driver responds by re-issuing `SubscribeBulk` for every tracked subscription — MXAccess delivers an initial value per item on subscribe, so re-subscribing *is* the re-snapshot the contract asks for. It does **not** restart the event pump (the stream carrying the sentinel is healthy) and it honours `Reconnect:ReplayOnSessionLost=false` by logging and standing down, on the grounds that an operator who delegated recovery to the gateway did not ask for a re-subscribe on a different trigger.
⚠️ This cannot currently fire: the sentinel is only emitted on a stream opened with `after_worker_sequence > 0`, and the driver opens with `0`, which the gateway treats as "fresh stream, not a resume". Enabling resume is a bigger change than it looks — the sequence is only meaningful **within one gateway session**, and the reconnect path recreates the session, so a remembered sequence replayed against a fresh session would set the server's live watermark *above* every event it is about to send, silencing the stream rather than recovering it.
- **Undecodable values** (`galaxy.events.undecodable`): a value kind this driver has no CLR representation for is published as a **null under the event's own quality**, not as Bad. The status is deliberately left alone — nothing can reach this branch today, so the counter exists to produce evidence before anyone changes what an OPC UA client sees.
- **Undecodable values** (`galaxy.events.undecodable`): a value kind this driver has no CLR representation for is published as **`BadTypeMismatch` with a null value**, and the first occurrence of each distinct kind is logged. It is *not* published as a null carrying the event's own quality: an MXAccess event can be Good-quality while the payload is one we cannot map, and a good null is indistinguishable from a genuine null reading — a client would consume a value it never received as a real one, with the failure visible only in a log. Non-zero here means the gateway is sending a wire type `MxValueDecoder` has no mapping for and those readings are being lost; the fix is a decoder mapping, not a quality change.
+4 -3
View File
@@ -63,9 +63,10 @@ observation.
`galaxy.events.undecodable` should likewise be flat at zero: every
`MxValue` kind the contract declares decodes today. Non-zero means the
gateway shipped a new wire type and those readings are being published as
**nulls under the event's own quality** — the status is deliberately not
downgraded, so this counter is the only signal that it is happening.
gateway shipped a new wire type, and those readings are being published
as **`BadTypeMismatch` with a null value** — visible to clients as bad
data rather than as a plausible null, and logged once per distinct kind.
The remedy is a mapping in `MxValueDecoder`, not a quality change.
### Bounded channel design
@@ -67,7 +67,13 @@ internal sealed class EventPump : IAsyncDisposable
private Task? _loop;
private Task? _dispatchLoop;
private bool _disposed;
private int _undecodableWarned;
/// <summary>
/// Value kinds already warned about. Bounded by the <c>MxValue</c> oneof enum, so a
/// broken producer cannot flood the log, while a SECOND unrepresentable kind appearing
/// later still gets its own line — warn-once-per-pump would have swallowed it.
/// </summary>
private readonly HashSet<MxValue.KindOneofCase> _undecodableKindsWarned = [];
/// <summary>Occurs when a data change event is received from the Galaxy subscriber.</summary>
public event EventHandler<DataChangeEventArgs>? OnDataChange;
@@ -202,6 +208,23 @@ internal sealed class EventPump : IAsyncDisposable
}
}
/// <summary>
/// Logs the first occurrence of each unrepresentable value kind. The dispatch loop is
/// single-reader, so the set needs no lock — but it is only ever touched from there.
/// </summary>
private void WarnUndecodableOnce(MxEvent ev)
{
var kind = ev.Value?.KindCase ?? MxValue.KindOneofCase.None;
if (!_undecodableKindsWarned.Add(kind)) return;
_logger.LogWarning(
"Galaxy event for item handle {ItemHandle} carried a value kind this driver cannot " +
"represent ({KindCase}) — publishing BadTypeMismatch. This means the gateway is sending " +
"a wire type MxValueDecoder has no mapping for; readings of this kind are being lost. " +
"Further occurrences of {KindCase} are counted on galaxy.events.undecodable, not logged.",
ev.ItemHandle, kind, kind);
}
private void NotifyStreamFault(Exception cause)
{
if (_onStreamFault is null) return;
@@ -314,21 +337,19 @@ internal sealed class EventPump : IAsyncDisposable
{
if (!MxValueDecoder.TryDecode(ev.Value, out var value))
{
// The status is deliberately NOT downgraded here. Publishing null-under-Good is
// dishonest, but changing what an OPC UA client sees for a live shape is not a
// change to make blind: today nothing can reach this branch (every declared
// MxValue kind decodes), so it only fires on a wire type a future gateway adds.
// Surfacing it on a counter first means the next person has evidence to act on
// rather than a guess. Warn once per pump — a broken producer would flood.
// A value we cannot represent is published as Bad, NOT as a null carrying the
// event's own (typically Good) quality. A good null is indistinguishable from a
// real null reading, so a client would treat a value it never received as a
// genuine one — the failure would be invisible on the wire and survive only in a
// log nobody is reading. BadTypeMismatch says what actually happened: the payload
// arrived, and this driver has no CLR type to carry it in.
ValuesUndecodable.Add(1, _clientTag);
if (Interlocked.Exchange(ref _undecodableWarned, 1) == 0)
{
_logger.LogWarning(
"Galaxy event for item handle {ItemHandle} carried a value kind this driver cannot " +
"represent ({KindCase}); publishing null under the event's own quality. Further " +
"occurrences are counted on galaxy.events.undecodable but not logged.",
ev.ItemHandle, ev.Value?.KindCase);
}
WarnUndecodableOnce(ev);
return new DataValueSnapshot(
Value: null,
StatusCode: StatusCodeMap.BadTypeMismatch,
SourceTimestampUtc: ev.SourceTimestamp is { } undecodableTs ? undecodableTs.ToDateTime() : null,
ServerTimestampUtc: DateTime.UtcNow);
}
var statusCode = ev.Statuses.Count > 0
@@ -222,6 +222,75 @@ public sealed class EventPumpReplayGapTests
(await faultObserved.Task).Message.ShouldContain("closed or evicted");
}
/// <summary>
/// Verifies a value kind this driver cannot represent is published as <c>Bad</c> rather
/// than as a null carrying the event's own quality.
/// </summary>
/// <remarks>
/// The event below carries <c>Quality = 192</c> (OPC DA Good), so a decoder that gave up
/// silently would publish <c>null</c> under a <b>Good</b> status — indistinguishable from
/// a genuine null reading, which is how a value the gateway did send would be consumed as
/// a value it didn't. The Good quality is the point of the fixture: with a Bad quality on
/// the event the assertion would hold no matter what the decoder did.
/// </remarks>
[Fact]
public async Task An_unrepresentable_value_is_published_as_bad_not_as_a_good_null()
{
var subscriber = new ControllableSubscriber();
var registry = new SubscriptionRegistry();
registry.Register(1, [new TagBinding("Tank.Level", ItemHandle: 7)]);
var received = new List<DataChangeEventArgs>();
var bothSeen = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
await using var pump = new EventPump(
subscriber, registry, channelCapacity: 8, clientName: "GapTest");
pump.OnDataChange += (_, args) =>
{
lock (received)
{
received.Add(args);
if (received.Count == 2) bothSeen.TrySetResult();
}
};
pump.Start();
// An MxValue with no kind set at all — a payload we cannot interpret — under Good quality.
await subscriber.EmitAsync(new MxEvent
{
Family = MxEventFamily.OnDataChange,
ItemHandle = 7,
Value = new MxValue(),
Quality = 192,
SourceTimestamp = Timestamp.FromDateTime(DateTime.UtcNow),
});
// Positive control: a decodable value on the same handle and the same Good quality must
// still publish Good, so the assertion above cannot be satisfied by a pump that simply
// marks everything Bad.
await subscriber.EmitAsync(new MxEvent
{
Family = MxEventFamily.OnDataChange,
ItemHandle = 7,
Value = new MxValue { DoubleValue = 42.0 },
Quality = 192,
SourceTimestamp = Timestamp.FromDateTime(DateTime.UtcNow),
});
(await Task.WhenAny(bothSeen.Task, Task.Delay(WaitMs))).ShouldBe(bothSeen.Task);
lock (received)
{
received[0].Snapshot.Value.ShouldBeNull();
received[0].Snapshot.StatusCode.ShouldBe(StatusCodeMap.BadTypeMismatch,
"an undecodable payload must not reach a client wearing the event's Good quality");
received[1].Snapshot.Value.ShouldBe(42.0);
received[1].Snapshot.StatusCode.ShouldBe(StatusCodeMap.Good,
"a decodable value on the same quality must still publish Good");
}
}
/// <summary>
/// <see cref="IGalaxySubscriber"/> fake whose stream can be fed arbitrary events and
/// completed cleanly, so both control-signal paths can be driven directly.