fix(mqtt/sparkplug): initial push on subscribe + BadNoCommunication on connection loss

This commit is contained in:
Joseph Doherty
2026-08-20 09:52:15 -04:00
parent ac2f8dc801
commit b7d20d14a8
4 changed files with 385 additions and 5 deletions
@@ -618,3 +618,20 @@ test could only pass once the production change landed.
- The event fires inside the existing `State == Connected` guard in `OnDisconnectedAsync`, which is
what makes "not on an initial-connect failure" and "not on dispose" true by construction rather
than by a second condition — `DisposeAsync` sets `Disposed` before tearing the client down.
### T4 — Sparkplug ingestor: initial push + connection-loss stale
- RED: `--filter "…SparkplugIngestorRecoveryTests"` ⇒ **build failure**, `CS1061: 'SparkplugIngestor'
does not contain a definition for 'OnConnectionLost'` (×2).
- GREEN: 6/6. Full `Driver.Mqtt.Tests`: 803/803.
- ⚠️ **Deviation from the plan's test rationale (recorded, not silently accepted):** the plan asks for
the identical-DBIRTH-replay assertion as this seam's vacuity guard. It is present, but **it is not a
falsifier here** — Sparkplug ingestion has no unchanged-value suppression at all (births/data
publish directly, never through `Deliver`), so that replay was already raised Good before the
change. That is now asserted explicitly by an added control,
`An_identical_birth_replay_is_raised_even_without_a_preceding_stale`, and the suite header says so.
The load-bearing assertion on this seam is the **stale** itself.
- The stale reports `BadNoCommunication` while a DDEATH keeps `BadCommunicationError`: the death is the
source announcing its own offline state, the stale is our transport losing the broker (and matches
what the host's S15 fan-out writes). The existing const's "no driver emits BadNoCommunication at all"
remark was corrected rather than left to rot.
@@ -25,7 +25,7 @@
{
"id": "T4",
"subject": "SparkplugIngestor: initial push on subscribe + connection-loss stale; rewrite the 'values are deliberately not staled' remark",
"status": "pending",
"status": "done",
"blockedBy": [
"T3"
]
@@ -101,9 +101,28 @@ public sealed class SparkplugIngestor
/// OpcUaClient and this driver's own plain-mode path all emit it, while no driver emits
/// BadNoCommunication at all (it appears once, in a CLI formatter's label map). Following the
/// precedent keeps one code meaning "not communicating" across the whole fleet.
/// <para><b>Superseded in part (archreview R3-02):</b> <see cref="StatusBadNoCommunication"/> now
/// has a distinct, fleet-wide meaning — "the link to the source is down" — written by the host's
/// comms-loss fan-out and by <see cref="OnConnectionLost"/>. This code keeps the narrower one it
/// always had here: the <i>source itself</i> announced it is offline, or a birth voided the
/// scope.</para>
/// </summary>
private const uint StatusBadCommunicationError = 0x80050000u;
/// <summary>
/// The broker session this driver was observing is gone — see <see cref="OnConnectionLost"/>.
/// Distinct from <see cref="StatusBadCommunicationError"/>, which means the source announced its
/// own death.
/// </summary>
private const uint StatusBadNoCommunication = 0x80310000u;
/// <summary>
/// No value has ever been observed for this reference — <c>LastValueCache.Read</c>'s answer for
/// a RawPath nothing has published under yet. Neither the initial push nor the connection-loss
/// stale touches such a reference: both would claim an observation that never happened.
/// </summary>
private const uint StatusBadWaitingForInitialData = 0x80320000u;
/// <summary>The metric's value does not fit the tag's effective data type — including an explicit Sparkplug null.</summary>
private const uint StatusBadTypeMismatch = 0x80740000u;
@@ -317,6 +336,47 @@ public sealed class SparkplugIngestor
_publishTransport = connection;
connection.MessageReceived += HandleMessage;
connection.Reconnected += OnReconnectedAsync;
connection.ConnectionLost += OnConnectionLost;
}
/// <summary>
/// Marks every already-observed metric <c>BadNoCommunication</c>, <b>keeping its last value</b>,
/// and publishes the change — this driver's answer to a lost broker session (archreview
/// 05/STAB-18 shape a). Wired to <see cref="MqttConnection.ConnectionLost"/> by
/// <see cref="AttachTo"/>; public so tests can drive the edge directly.
/// </summary>
/// <remarks>
/// <para>
/// <b>A death is a different event and keeps its own code.</b> NDEATH/DDEATH is the source
/// announcing it is offline and stales with <see cref="StatusBadCommunicationError"/> as it
/// always has; this is <i>our</i> transport losing the broker, and it reports
/// <see cref="StatusBadNoCommunication"/> — the same code the host's comms-loss fan-out
/// writes onto the value nodes, so an operator sees one story from both ends.
/// </para>
/// <para>
/// <b>It is what makes the reconnect recover.</b> A reconnect replays NBIRTH/DBIRTH carrying
/// values that are routinely unchanged; the birth path publishes them, but every node the
/// host latched Bad needs a change to un-latch. Staling first guarantees the replay differs.
/// </para>
/// <para>
/// Runs on MQTTnet's dispatcher thread: dictionary reads and synchronous
/// <see cref="OnDataChange"/> invokes only, never an await. A never-observed metric and an
/// already-Bad one are both left alone.
/// </para>
/// </remarks>
public void OnConnectionLost()
{
var now = _utcNow();
foreach (var rawPath in _authored.ByRawPath.Keys)
{
var prior = Values.Read(rawPath);
if (prior.StatusCode == StatusBadWaitingForInitialData) continue; // never observed
if ((prior.StatusCode >> 30) >= 2) continue; // already Bad
var staled = prior with { StatusCode = StatusBadNoCommunication, ServerTimestampUtc = now };
Values.Update(rawPath, staled);
Raise(rawPath, staled);
}
}
/// <summary>
@@ -502,6 +562,13 @@ public sealed class SparkplugIngestor
_handleByRawPath[def.Name] = handle;
}
// Initial push (05/STAB-18 shape b): every metric this driver has already observed is
// republished onto the new handle. A poll driver gets this free from PollGroupEngine's
// forceRaise on the first poll of a new subscription; Sparkplug pushed nothing, so an
// address-space rebuild — which reseeds every node BadWaitingForInitialData and triggers no
// rebirth — left a quiet metric waiting for the plant indefinitely.
PushObservedSnapshots(defs);
return Task.FromResult<ISubscriptionHandle>(handle);
}
@@ -580,10 +647,16 @@ public sealed class SparkplugIngestor
/// already empty, so calling it three times during one connect costs three no-ops.
/// </para>
/// <para>
/// Values are deliberately <b>not</b> staled here. A session change is a driver-side event,
/// not a statement about the plant; last observed values remain the best available answer
/// until a birth lands, and the connection's own health surface already reports the outage.
/// Only a death — the source saying it is offline — stales a tag.
/// <b>Values are not staled here — but they ARE staled on a connection loss</b> (archreview
/// R3-02; this remark used to claim the opposite and that was the stale-Good bug). What is
/// still true is the narrow part: a session reset for a <i>config rebuild</i> on a live
/// connection says nothing about the plant, so last observed values remain the best answer
/// until a birth lands. What was wrong was the reason given for the general case — "the
/// connection's own health surface already reports the outage". Health is not quality: an
/// OPC UA client subscribed to the metric's node sees neither the health page nor the
/// outage, and reported Good over a dead broker. The transport loss is now handled by
/// <see cref="OnConnectionLost"/> (<see cref="StatusBadNoCommunication"/>, value retained);
/// a death still stales the way it always did (<see cref="StatusBadCommunicationError"/>).
/// </para>
/// </remarks>
/// <param name="reason">Why the reset is happening, for the diagnostic log.</param>
@@ -1376,6 +1449,28 @@ public sealed class SparkplugIngestor
return new DataValueSnapshot(null, StatusBadTypeMismatch, stamp, serverUtc);
}
/// <summary>
/// Republishes the cached snapshot of every already-observed reference in
/// <paramref name="defs"/> — the push-model equivalent of <c>PollGroupEngine</c>'s
/// <c>forceRaise: true</c> on the first poll of a new subscription (05/STAB-18 shape b).
/// </summary>
/// <remarks>
/// A never-observed reference is skipped: its cached status is
/// <see cref="StatusBadWaitingForInitialData"/>, which is a more precise statement than any
/// value this could republish. The cache is not re-<c>Update</c>d — nothing new was observed,
/// only re-delivered — so the stored timestamps stay honest.
/// </remarks>
/// <param name="defs">The definitions the new handle covers.</param>
private void PushObservedSnapshots(IReadOnlyList<MqttTagDefinition> defs)
{
for (var i = 0; i < defs.Count; i++)
{
var snapshot = Values.Read(defs[i].Name);
if (snapshot.StatusCode == StatusBadWaitingForInitialData) continue;
Raise(defs[i].Name, snapshot);
}
}
/// <summary>Marks every authored tag in <paramref name="scope"/> as no longer communicating.</summary>
private void FanStale(SparkplugScope scope, DateTime? sourceUtc, string reason)
{
@@ -0,0 +1,268 @@
using Google.Protobuf;
using Org.Eclipse.Tahu.Protobuf;
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Sparkplug;
using TahuDataType = Org.Eclipse.Tahu.Protobuf.DataType;
namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests;
/// <summary>
/// The two push-model edge behaviours <see cref="SparkplugIngestor"/> lacked (archreview
/// 05/STAB-18), matching <see cref="MqttSubscriptionManagerRecoveryTests"/> on the plain path: an
/// <b>initial push</b> of the cached snapshot on every (re)subscribe, and a
/// <b>connection-loss stale</b> to <c>BadNoCommunication</c> with the value retained.
///
/// <para><b>Where the falsifier actually is on this seam — measured, not assumed.</b> Recovery is
/// proved with an <b>identical</b> DBIRTH value, as on the plain path. But Sparkplug ingestion
/// carries <em>no</em> unchanged-value suppression (it publishes births and data directly rather
/// than through <c>MqttSubscriptionManager.Deliver</c>), so that replay was already raised Good
/// before this change — pinned below by
/// <see cref="An_identical_birth_replay_is_raised_even_without_a_preceding_stale"/>. The
/// load-bearing assertion here is therefore the <b>stale</b>: a Sparkplug tag now reports Bad on
/// the driver's own disconnect edge instead of Good over a dead broker.</para>
///
/// <para><b>A death is not a connection loss.</b> DDEATH/NDEATH is the source announcing it is
/// offline and keeps its own <c>BadCommunicationError</c> stale, unchanged; <c>ConnectionLost</c>
/// is this driver's transport losing the broker and reports <c>BadNoCommunication</c> — the same
/// code the host's comms-loss fan-out writes. The DDEATH regression case below pins that the two
/// paths stayed distinct.</para>
/// </summary>
public sealed class SparkplugIngestorRecoveryTests
{
// OPC UA status codes, verified against Opc.Ua.StatusCodes rather than recalled.
private const uint Good = 0x00000000u;
private const uint BadCommunicationError = 0x80050000u;
private const uint BadNoCommunication = 0x80310000u;
private const uint BadWaitingForInitialData = 0x80320000u;
private const string Group = "Plant1";
private const string Node = "EdgeA";
private const string Device = "Filler1";
private const string TempPath = "Plant/Mqtt/spb/Filler1Temp";
private const string PressPath = "Plant/Mqtt/spb/Filler1Press";
private const ulong TempAlias = 5UL;
private const ulong PressAlias = 6UL;
private sealed record Raised(string Reference, object? Value, uint StatusCode);
private static List<Raised> Record(SparkplugIngestor ing)
{
var raised = new List<Raised>();
ing.OnDataChange += (_, e) => raised.Add(new Raised(e.FullReference, e.Snapshot.Value, e.Snapshot.StatusCode));
return raised;
}
// ---------------------------------------------------------------------------------
// (A) Initial push on (re)subscribe
// ---------------------------------------------------------------------------------
/// <summary>
/// A metric observed before this subscription existed is pushed once from the cache. This is the
/// address-space-rebuild shape: a rebuild reseeds every node <c>BadWaitingForInitialData</c> and
/// triggers no rebirth, so without the push a quiet metric never refills.
/// </summary>
[Fact]
public async Task SubscribeAsync_pushes_the_cached_snapshot_for_an_observed_metric()
{
var ing = await NewSubscribedIngestorAsync();
FeedBirths(ing);
ing.Dispatch(Topic(SparkplugMessageType.DDATA, Device), Ddata(seq: 2, (TempAlias, TahuDataType.Float, 7f)));
var raised = Record(ing);
await ing.SubscribeAsync([TempPath], TimeSpan.Zero, TestContext.Current.CancellationToken);
raised.Count.ShouldBe(1);
raised[0].Reference.ShouldBe(TempPath);
raised[0].Value.ShouldBe(7f);
raised[0].StatusCode.ShouldBe(Good);
}
/// <summary>
/// A metric that has never been observed carries only the cache's
/// <c>BadWaitingForInitialData</c> placeholder, and pushing that would claim an observation the
/// driver has not made.
/// </summary>
[Fact]
public async Task SubscribeAsync_does_not_push_a_never_observed_metric()
{
var ing = await NewSubscribedIngestorAsync();
var raised = Record(ing);
await ing.SubscribeAsync([TempPath, PressPath], TimeSpan.Zero, TestContext.Current.CancellationToken);
raised.ShouldBeEmpty();
ing.Values.Read(TempPath).StatusCode.ShouldBe(BadWaitingForInitialData);
}
// ---------------------------------------------------------------------------------
// (B) Connection-loss stale + the identical-value recovery
// ---------------------------------------------------------------------------------
/// <summary>
/// A lost broker session marks every observed metric <c>BadNoCommunication</c> with its last
/// value retained. <c>ResetSessionState</c>'s previous remark argued the opposite — that the
/// connection's health surface already reports the outage — but health is not quality, and an
/// OPC UA client subscribed to the metric's node sees neither the health page nor the outage.
/// </summary>
[Fact]
public async Task OnConnectionLost_stales_every_observed_metric_keeping_its_value()
{
var ing = await NewSubscribedIngestorAsync();
FeedBirths(ing);
ing.Dispatch(Topic(SparkplugMessageType.DDATA, Device), Ddata(seq: 2, (TempAlias, TahuDataType.Float, 7f)));
var raised = Record(ing);
ing.OnConnectionLost();
raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == BadNoCommunication && Equals(r.Value, 7f));
ing.Values.Read(TempPath).StatusCode.ShouldBe(BadNoCommunication);
ing.Values.Read(TempPath).Value.ShouldBe(7f);
}
/// <summary>
/// <b>The contract test for this seam.</b> The reconnect replays NBIRTH/DBIRTH carrying the
/// <b>identical</b> metric value, and it must be raised Good. Before the stale, the birth's
/// value equalled the cached one and the whole tag stayed Bad until the plant moved.
/// </summary>
[Fact]
public async Task After_the_stale_an_identical_birth_replay_is_raised_Good()
{
var ing = await NewSubscribedIngestorAsync();
FeedBirths(ing, tempBirthValue: 7f);
ing.OnConnectionLost();
ing.Values.Read(TempPath).StatusCode.ShouldBe(BadNoCommunication);
var raised = Record(ing);
FeedBirths(ing, tempBirthValue: 7f, bdSeq: 2); // the reconnect's replayed births, value unchanged
raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == Good && Equals(r.Value, 7f));
}
/// <summary>
/// <b>Vacuity control, and it changes what the test above proves.</b> Unlike the plain path,
/// Sparkplug ingestion has no unchanged-value suppression at all — a birth republishes its
/// metrics regardless — so an identical birth replay would have been raised Good even before
/// the stale existed. This asserts that directly. The consequence: on THIS seam the
/// load-bearing assertion is the stale itself (a Sparkplug tag now reports Bad on the driver's
/// own disconnect edge instead of Good over a dead broker), and the replay test guards only
/// that the stale is not sticky.
/// </summary>
[Fact]
public async Task An_identical_birth_replay_is_raised_even_without_a_preceding_stale()
{
var ing = await NewSubscribedIngestorAsync();
FeedBirths(ing, tempBirthValue: 7f);
var raised = Record(ing);
FeedBirths(ing, tempBirthValue: 7f, bdSeq: 2);
raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == Good && Equals(r.Value, 7f));
}
/// <summary>
/// Regression: a DDEATH still stales with <c>BadCommunicationError</c>, the source-announced
/// code. The connection-loss stale is a second, distinct path — it did not replace this one.
/// </summary>
[Fact]
public async Task DDeath_still_stales_with_BadCommunicationError()
{
var ing = await NewSubscribedIngestorAsync();
FeedBirths(ing, tempBirthValue: 7f);
var raised = Record(ing);
ing.Dispatch(Topic(SparkplugMessageType.DDEATH, Device), Ddeath(seq: 2));
raised.ShouldContain(r => r.Reference == TempPath && r.StatusCode == BadCommunicationError);
ing.Values.Read(TempPath).StatusCode.ShouldBe(BadCommunicationError);
}
// =================================================================================
// Fixtures — deliberately a local copy of SparkplugIngestorTests' helpers (both are
// private to their own class; sharing them would make one suite's edits ripple into
// the other's fixtures).
// =================================================================================
private static RawTagEntry Tag(string rawPath, string tagConfig) => new(rawPath, tagConfig, WriteIdempotent: false);
private static string Blob(string group, string node, string? device, string metric, string dataType)
{
var deviceKey = device is null ? "" : $"""{'"'}deviceId{'"'}:"{device}",""";
return $$"""{"groupId":"{{group}}","edgeNodeId":"{{node}}",{{deviceKey}}"metricName":"{{metric}}","dataType":"{{dataType}}"}""";
}
private static async Task<SparkplugIngestor> NewSubscribedIngestorAsync()
{
var options = new MqttDriverOptions
{
Mode = MqttMode.SparkplugB,
Sparkplug = new MqttSparkplugOptions { GroupId = Group, HostId = "otopcua-host-1" },
};
var ingestor = new SparkplugIngestor(options, "mqtt-spb-1", rebirthDebounce: TimeSpan.Zero);
ingestor.Register(
[
Tag(TempPath, Blob(Group, Node, Device, "Temperature", "Float32")),
Tag(PressPath, Blob(Group, Node, Device, "Pressure", "Float32")),
]);
await ingestor.SubscribeAsync([.. ingestor.AuthoredRawPaths], TimeSpan.Zero, TestContext.Current.CancellationToken);
return ingestor;
}
private static void FeedBirths(SparkplugIngestor ing, float tempBirthValue = 0f, ulong bdSeq = 1)
{
ing.Dispatch(Topic(SparkplugMessageType.NBIRTH), Nbirth(seq: 0, bdSeq));
ing.Dispatch(Topic(SparkplugMessageType.DBIRTH, Device), Dbirth(seq: 1, tempBirthValue));
}
private static SparkplugTopic Topic(SparkplugMessageType type, string? device = null) =>
new(type, Group, Node, device, null);
private static SparkplugPayload Decode(Payload payload) => SparkplugCodec.Decode(payload.ToByteArray());
private static SparkplugPayload Nbirth(ulong seq, ulong bdSeq)
{
var payload = new Payload { Seq = seq, Timestamp = 1721822400000UL };
payload.Metrics.Add(new Payload.Types.Metric
{
Name = "bdSeq",
Datatype = (uint)TahuDataType.Uint64,
LongValue = bdSeq,
});
return Decode(payload);
}
private static SparkplugPayload Dbirth(ulong seq, float tempValue)
{
var payload = new Payload { Seq = seq, Timestamp = 1721822400000UL };
payload.Metrics.Add(BirthMetric("Temperature", TempAlias, tempValue));
payload.Metrics.Add(BirthMetric("Pressure", PressAlias, 0f));
return Decode(payload);
}
private static SparkplugPayload Ddata(ulong seq, params (ulong Alias, TahuDataType Type, float Value)[] metrics)
{
var payload = new Payload { Seq = seq, Timestamp = 1721822405000UL };
foreach (var (alias, _, value) in metrics)
{
// Deliberately NO Name and NO Datatype: the real post-birth wire shape.
payload.Metrics.Add(new Payload.Types.Metric { Alias = alias, FloatValue = value });
}
return Decode(payload);
}
private static SparkplugPayload Ddeath(ulong seq) =>
Decode(new Payload { Seq = seq, Timestamp = 1721822405000UL });
private static Payload.Types.Metric BirthMetric(string name, ulong alias, float value) => new()
{
Name = name,
Alias = alias,
Datatype = (uint)TahuDataType.Float,
FloatValue = value,
};
}