dohertj2/lmxopcua
1
0
Fork 0
Code Issues 43 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(mqtt): retry failed SUBSCRIBEs via a pending set; throw on no-verdict subscribe (R06:M-1, M-11)

Browse Source 
A failed SUBSCRIBE (refusal or no broker verdict) used to drop its topic from every
retry set while SubscribeAsync returned a live handle, leaving tags dark behind a
Healthy connection until a redeploy. Failed topics now join a pending set retried by
the next SubscribeAsync and by every reconnect (not counted toward the reconnect's
total-failure verdict, so an ACL-denied topic cannot reconnect-loop). A no-verdict
subscribe degrades its refs then throws so the actor's reconcile re-subscribes. A
subscribe with no transport attached is recorded pending, never established (M-11).
MiniBroker now answers SUBSCRIBE.

Refs #744
Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
2026-10-22 15:22:17 -04:00
parent 0d49eaf77b
commit 01efd39177
5 changed files with 374 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MiniBroker.cs
+52
View File
@@ -59,6 +59,54 @@ internal sealed class MiniBroker : IDisposable
/// </summary>
public byte ConnAckReturnCode { get; set; } = ReturnCodeAccepted;
/// <summary>
/// When true (the default) a SUBSCRIBE is answered with a SUBACK granting each filter's requested
/// QoS. False is a broker that accepts SUBSCRIBE and never answers — the subscribe-deadline shape.
/// </summary>
public bool AnswerSubscribe { get; set; } = true;
/// <summary>
/// Builds the SUBACK for one SUBSCRIBE packet read whole into <paramref name="buffer"/>, or null when
/// the packet is not parseable from one read (the fake handles one packet per read, like CONNECT).
/// </summary>
private static byte[]? TryBuildSubAck(byte[] buffer, int read)
{
// Fixed header: type byte, then the variable-length "remaining length".
var at = 1;
var remaining = 0;
var multiplier = 1;
byte digit;
do
{
if (at >= read) return null;
digit = buffer[at++];
remaining += (digit & 0x7F) * multiplier;
multiplier *= 128;
}
while ((digit & 0x80) != 0);
var end = at + remaining;
if (end > read || remaining < 2) return null;
var packetIdHigh = buffer[at];
var packetIdLow = buffer[at + 1];
at += 2;
var grants = new List<byte>();
while (at + 2 <= end)
{
var length = (buffer[at] << 8) | buffer[at + 1];
at += 2 + length;
if (at >= end) return null;
grants.Add((byte)(buffer[at] & 0x03));
at++;
}
var ack = new List<byte> { 0x90, (byte)(2 + grants.Count), packetIdHigh, packetIdLow };
ack.AddRange(grants);
return [.. ack];
}
/// <summary>Total TCP connections accepted since construction — counts reconnect attempts.</summary>
public int AcceptedCount => Volatile.Read(ref _acceptedCount);
@@ -163,6 +211,10 @@ internal sealed class MiniBroker : IDisposable
{
await stream.WriteAsync(PingResp, _cts.Token);
}
else if (packetType == 0x80 && AnswerSubscribe && TryBuildSubAck(buffer, read) is { } subAck)
{
await stream.WriteAsync(subAck, _cts.Token);
}
}
}
catch (Exception)
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttDriverSessionLifecycleTests.cs
+49 -4
View File
@@ -13,9 +13,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests;
/// (<see cref="MqttDriver.ShutdownAsync"/> / <see cref="MqttDriver.DisposeAsync"/>).
/// </summary>
/// <remarks>
/// Plain mode deliberately: it is the only mode whose initialize needs no SUBSCRIBE, and
/// <see cref="MiniBroker"/> answers CONNECT and PINGREQ and nothing else — so a Sparkplug driver
/// would park on its group SUBSCRIBE until the deadline and prove nothing about connections.
/// Plain mode deliberately: it is the only mode whose initialize needs no SUBSCRIBE.
/// <see cref="MiniBroker"/> answers CONNECT, SUBSCRIBE (unless told not to) and PINGREQ, and never
/// PUBLISHes, so ingest is driven through <c>Subscriptions.HandleMessage</c>.
/// </remarks>
[Trait("Category", "Unit")]
public sealed class MqttDriverSessionLifecycleTests
@@ -122,7 +122,7 @@ public sealed class MqttDriverSessionLifecycleTests
/// </para>
/// <para>
/// Ingest is fed through <c>Subscriptions.HandleMessage</c>, the method MQTTnet's dispatcher
/// calls, because <see cref="MiniBroker"/> answers CONNECT and PINGREQ and nothing else: it
/// calls, because <see cref="MiniBroker"/> answers CONNECT, SUBSCRIBE and PINGREQ only: it
/// can drop a session (which is what this test needs from it) but cannot PUBLISH.
/// </para>
/// </remarks>
@@ -174,6 +174,51 @@ public sealed class MqttDriverSessionLifecycleTests
recovered.Value.ShouldBe(21.5);
}
/// <summary>
/// <b>R06:M-1, end to end.</b> A SUBSCRIBE the broker never answers used to come back from
/// <see cref="MqttDriver.SubscribeAsync"/> as a live handle while the topic left every retry set, so
/// <c>DriverInstanceActor</c> never re-subscribed and the tag stayed dark behind a Healthy connection.
/// It must throw (the actor's reconcile then retries), and the retry against a broker that answers
/// must establish and deliver — the positive control.
/// </summary>
[Trait("Category", "SlowUnit")]
[Fact]
public async Task ASubscribeTheBrokerNeverAnswers_Throws_AndTheRetryEstablishes()
{
const string rawPath = "Plant/Mqtt/dev1/Temp";
const string topic = "factory/oven/temp";
using var broker = new MiniBroker { AnswerSubscribe = false };
var options = BrokerOptions(broker.Port) with
{
RawTags =
[
new RawTagEntry(
rawPath,
$$"""{"topic":"{{topic}}","payloadFormat":"Scalar","dataType":"Float64"}""",
WriteIdempotent: false),
],
};
await using var driver = new MqttDriver(options, "d", null);
await driver.InitializeAsync(Json(options), TestContext.Current.CancellationToken);
await Should.ThrowAsync<InvalidOperationException>(() =>
driver.SubscribeAsync([rawPath], TimeSpan.Zero, TestContext.Current.CancellationToken));
driver.GetHealth().State.ShouldBe(DriverState.Healthy, "the connection itself is fine — that was the trap");
broker.AnswerSubscribe = true;
var handle = await driver.SubscribeAsync([rawPath], TimeSpan.Zero, TestContext.Current.CancellationToken);
var raised = new List<DataChangeEventArgs>();
driver.OnDataChange += (_, e) => { lock (raised) raised.Add(e); };
driver.Subscriptions.HandleMessage(topic, "7"u8.ToArray(), retained: false);
lock (raised)
{
raised.ShouldHaveSingleItem().SubscriptionHandle.ShouldBe(handle);
raised[0].Snapshot.StatusCode.ShouldBe(0u);
}
}
private static async Task WaitForAsync(Func<bool> condition, string what)
{
var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(10);
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerTests.cs
+160 -3
View File
@@ -538,17 +538,171 @@ public sealed class MqttSubscriptionManagerTests
mgr.Values.Read(OvenTempPath).StatusCode.ShouldBe(BadCommunicationError);
}
/// <summary>
/// A SUBSCRIBE that never got a verdict (transport throw / deadline) degrades its refs AND throws,
/// so <c>DriverInstanceActor</c> holds no handle and its health-tick reconcile re-subscribes. It
/// used to return a live-looking handle with the topic dropped from every retry set — dark behind a
/// Healthy connection until a redeploy.
/// </summary>
[Fact]
public async Task SubscribeAsync_TransportThrows_DegradesRefsToBad_DoesNotThrow()
public async Task SubscribeAsync_TransportThrows_DegradesRefsToBad_AndThrows()
{
var transport = new FakeTransport { Throw = new InvalidOperationException("broker gone") };
var mgr = NewManager(transport);
mgr.Register([Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}""")]);
var raised = new List<DataChangeEventArgs>();
mgr.OnDataChange += (_, e) => raised.Add(e);
await Should.NotThrowAsync(() =>
await Should.ThrowAsync<Exception>(() =>
mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken));
mgr.Values.Read(OvenTempPath).StatusCode.ShouldBe(BadCommunicationError);
// The Bad verdict still reached the subscriber before the throw.
raised.ShouldContain(e => e.FullReference == OvenTempPath && e.Snapshot.StatusCode == BadCommunicationError);
}
/// <summary>
/// The reconcile path: once the actor re-subscribes after the throw, the topic IS re-SUBSCRIBEd and
/// delivery resumes on the new handle — the positive control for the throw above.
/// </summary>
[Fact]
public async Task SubscribeAsync_AfterATransportThrow_TheRetrySubscribesAndDelivers()
{
var transport = new FakeTransport { Throw = new InvalidOperationException("broker gone") };
var mgr = NewManager(transport);
mgr.Register([Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}""")]);
await Should.ThrowAsync<Exception>(() =>
mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken));
transport.Throw = null;
var handle = await mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
var raised = new List<DataChangeEventArgs>();
mgr.OnDataChange += (_, e) => raised.Add(e);
mgr.HandleMessage(OvenTopic, "21.5"u8.ToArray(), retained: false);
transport.Calls.Count.ShouldBe(2);
transport.Calls[1].Select(f => f.Topic).ShouldBe([OvenTopic]);
raised.ShouldHaveSingleItem().SubscriptionHandle.ShouldBe(handle);
raised[0].Snapshot.Value.ShouldBe(21.5);
}
/// <summary>
/// A topic whose first SUBSCRIBE was refused stays in a pending set the reconnect re-subscribe
/// covers. The reconnect used to re-SUBSCRIBE only the granted set, so it was never retried.
/// </summary>
[Fact]
public async Task OnReconnectedAsync_RetriesATopicWhoseFirstSubscribeWasRefused()
{
var transport = new FakeTransport { Reject = _ => true };
var mgr = NewManager(transport);
mgr.Register([Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}""")]);
var handle = await mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
transport.Reject = _ => false;
await mgr.OnReconnectedAsync(TestContext.Current.CancellationToken);
transport.Calls.Count.ShouldBe(2);
transport.Calls[1].Select(f => f.Topic).ShouldBe([OvenTopic]);
var raised = new List<DataChangeEventArgs>();
mgr.OnDataChange += (_, e) => raised.Add(e);
mgr.HandleMessage(OvenTopic, "3"u8.ToArray(), retained: false);
raised.ShouldHaveSingleItem().SubscriptionHandle.ShouldBe(handle);
raised[0].Snapshot.StatusCode.ShouldBe(Good);
}
/// <summary>
/// A persistently ACL-denied topic must not turn every reconnect into a teardown loop: a pending
/// topic refused again is not "re-established nothing" — nothing that was live was lost. Both a
/// mixed set (the granted topic still re-subscribes) and a pending-only set are covered.
/// </summary>
[Fact]
public async Task OnReconnectedAsync_PendingTopicsRefusedAgain_DoesNotThrow()
{
var transport = new FakeTransport { Reject = t => t == "f/oven/press" };
var mgr = NewManager(transport);
mgr.Register(
[
Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}"""),
Tag(OvenPressPath, """{"topic":"f/oven/press","payloadFormat":"Scalar","dataType":"Float64"}"""),
]);
await mgr.SubscribeAsync([OvenTempPath, OvenPressPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
await Should.NotThrowAsync(() => mgr.OnReconnectedAsync(TestContext.Current.CancellationToken));
transport.Calls[1].Select(f => f.Topic).OrderBy(t => t, StringComparer.Ordinal)
.ShouldBe(["f/oven/press", OvenTopic]);
var lone = new FakeTransport { Reject = _ => true };
var mgr2 = NewManager(lone);
mgr2.Register([Tag(OvenPressPath, """{"topic":"f/oven/press","payloadFormat":"Scalar","dataType":"Float64"}""")]);
await mgr2.SubscribeAsync([OvenPressPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
await Should.NotThrowAsync(() => mgr2.OnReconnectedAsync(TestContext.Current.CancellationToken));
lone.Calls.Count.ShouldBe(2); // it WAS retried
}
/// <summary>
/// A later subscribe on the same session (a redeploy's bulk re-subscribe) also retries the pending
/// set, so a broker ACL fixed while connected recovers without waiting for a reconnect.
/// </summary>
[Fact]
public async Task SubscribeAsync_RetriesPendingTopicsFromAnEarlierRefusal()
{
var transport = new FakeTransport { Reject = t => t == "f/oven/press" };
var mgr = NewManager(transport);
mgr.Register(
[
Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}"""),
Tag(OvenPressPath, """{"topic":"f/oven/press","payloadFormat":"Scalar","dataType":"Float64"}"""),
]);
await mgr.SubscribeAsync([OvenPressPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
transport.Reject = _ => false;
await mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
transport.Calls[1].Select(f => f.Topic).OrderBy(t => t, StringComparer.Ordinal)
.ShouldBe(["f/oven/press", OvenTopic]);
}
/// <summary>
/// <b>R06:M-11.</b> A subscribe issued before any transport is attached reaches no broker, so it must
/// not be recorded as established. It used to be — "subscribed, receives nothing" — and the first
/// reconnect then had nothing to re-establish. Now it stays pending, and the first reconnect on the
/// attached session attempts it: the Bad verdict from the unconnected session is the proof.
/// </summary>
[Fact]
public async Task SubscribeAsync_WithNoTransport_StaysPending_AndTheFirstReconnectAttemptsIt()
{
var mgr = NewManager(transport: null);
mgr.Register([Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}""")]);
await mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
mgr.Values.Read(OvenTempPath).StatusCode.ShouldBe(0x80320000u); // BadWaitingForInitialData
await using var attached = new MqttConnection(
new MqttDriverOptions { Host = "127.0.0.1", Port = 1, UseTls = false }, "mqtt-1");
mgr.AttachTo(attached);
// The unconnected session gives no verdict, so the reconnect throws — and it ATTEMPTED the topic.
await Should.ThrowAsync<Exception>(() => mgr.OnReconnectedAsync(TestContext.Current.CancellationToken));
mgr.Values.Read(OvenTempPath).StatusCode.ShouldBe(BadCommunicationError);
}
/// <summary>
/// Register drops a pending topic whose tags the redeploy removed — the same housekeeping the
/// granted set gets — so a deleted tag's denied topic is not retried forever.
/// </summary>
[Fact]
public async Task Register_DroppingATag_RemovesItsTopicFromThePendingSet()
{
var transport = new FakeTransport { Reject = _ => true };
var mgr = NewManager(transport);
mgr.Register([Tag(OvenPressPath, """{"topic":"f/oven/press","payloadFormat":"Scalar","dataType":"Float64"}""")]);
await mgr.SubscribeAsync([OvenPressPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
mgr.Register([Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Scalar","dataType":"Float64"}""")]);
await mgr.OnReconnectedAsync(TestContext.Current.CancellationToken);
transport.Calls.Count.ShouldBe(1); // nothing left to retry
}
// ---------------------------------------------------------------------------------
@@ -939,7 +1093,10 @@ public sealed class MqttSubscriptionManagerTests
new MqttDriverOptions { Host = "127.0.0.1", Port = 1, UseTls = false }, "mqtt-1");
mgr.AttachTo(rebound);
await mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken);
// The unconnected session gives no verdict, so the subscribe now THROWS (M-1) — the throw is a
// second proof the SUBSCRIBE was attempted against the new session.
await Should.ThrowAsync<Exception>(() =>
mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken));
mgr.Values.Read(OvenTempPath).StatusCode.ShouldBe(
BadCommunicationError,