diff --git a/docs/drivers/Galaxy.md b/docs/drivers/Galaxy.md index 4168d57e..da6525b1 100644 --- a/docs/drivers/Galaxy.md +++ b/docs/drivers/Galaxy.md @@ -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. diff --git a/docs/v2/Galaxy.Performance.md b/docs/v2/Galaxy.Performance.md index 0e046cfa..d83e60a8 100644 --- a/docs/v2/Galaxy.Performance.md +++ b/docs/v2/Galaxy.Performance.md @@ -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 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs index 983add93..4c535360 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/EventPump.cs @@ -67,7 +67,13 @@ internal sealed class EventPump : IAsyncDisposable private Task? _loop; private Task? _dispatchLoop; private bool _disposed; - private int _undecodableWarned; + + /// + /// Value kinds already warned about. Bounded by the MxValue 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. + /// + private readonly HashSet _undecodableKindsWarned = []; /// Occurs when a data change event is received from the Galaxy subscriber. public event EventHandler? OnDataChange; @@ -202,6 +208,23 @@ internal sealed class EventPump : IAsyncDisposable } } + /// + /// 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. + /// + 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 diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EventPumpReplayGapTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EventPumpReplayGapTests.cs index a6f74c2c..df76fcc8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EventPumpReplayGapTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/EventPumpReplayGapTests.cs @@ -222,6 +222,75 @@ public sealed class EventPumpReplayGapTests (await faultObserved.Task).Message.ShouldContain("closed or evicted"); } + /// + /// Verifies a value kind this driver cannot represent is published as Bad rather + /// than as a null carrying the event's own quality. + /// + /// + /// The event below carries Quality = 192 (OPC DA Good), so a decoder that gave up + /// silently would publish null under a Good 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. + /// + [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(); + 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"); + } + } + /// /// fake whose stream can be fed arbitrary events and /// completed cleanly, so both control-signal paths can be driven directly.