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

fix(mqtt): session-rebind subscriptions, re-init teardown, timeout clamp

Browse Source 
Findings from the per-module review of Driver.Mqtt at b990adae
(code-reviews/Driver.Mqtt/findings.md records all ten; these are the fixed
ones).

Driver.Mqtt-001 (High) — MqttSubscriptionManager._subscribedTopics is a claim
about the broker session it established filters on, and AttachTo never cleared
it, but the manager OUTLIVES a session rebuild (AdoptOptions keeps it whenever
the ingest identity is unchanged). After a driver-host reconnect
(ShutdownAsync + InitializeAsync, the #527 watchdog's retry) or a
ReinitializeAsync that only changed the endpoint/credentials, the host's
re-subscribe resolved an empty pending set, sent no SUBSCRIBE at all, and left
the driver Connected/Healthy on a session carrying ZERO subscriptions — no
error, no Bad status, values frozen until the new session happened to drop.
AttachTo now forgets them, mirroring SparkplugIngestor.AttachTo's reset.

Driver.Mqtt-002 (High) — InitializeCoreAsync published _connection and
_primaryHost over whatever the driver already held, so a second initialize
stranded a connected, self-supervising broker session (plus a second Sparkplug
primary-host claim) that no later dispose could reach. Reachable without any
teardown of ours via DriverInstanceActor.AdoptConfigDuringInit. Now tears down
first, under the gate both callers already hold.

Driver.Mqtt-003 (Medium) — connectTimeoutSeconds is [Range]-annotated but
unvalidated on the runtime path; 0 makes every deadline fire immediately and a
negative value throws ArgumentOutOfRangeException out of ShutdownAsync and
DisposeAsync. Normalized once, at the seam every options instance passes.

Also: ShutdownAsync keeps the live last-message instant (-004), three usings
left behind by #512 removed (-005), stale remarks corrected (-009, -010), and
docs/drivers/Mqtt.md now distinguishes the two kinds of reconnect.

Tests 708 -> 713. All three regression tests were confirmed to fail against
the un-fixed code, with two controls that pass in either state.
This commit is contained in:
Joseph Doherty
2026-08-10 06:59:18 -04:00
parent b990adae55
commit 2b4b3fcbbf
7 changed files with 333 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttConnection.cs
+10 -10
View File
@@ -1,7 +1,6 @@
// System.Buffers is load-bearing despite no type here naming it: OnApplicationMessageReceivedAsync
// calls ReadOnlySequence<byte>.ToArray(), which is BuffersExtensions', not a member.
using System.Buffers;
using System.Net.Security;
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using Microsoft.Extensions.Logging;
using MQTTnet;
using MQTTnet.Protocol;
@@ -238,13 +237,14 @@ public sealed class MqttConnectRejectedException : Exception
/// client it created and reports <see cref="ObjectDisposedException"/>.
/// </para>
/// <para>
/// <b>The publish leg is deliberately narrow.</b> <see cref="PublishAsync"/> exists to satisfy
/// <see cref="Sparkplug.IMqttPublishTransport"/> — whose entire production caller is
/// <see cref="Sparkplug.RebirthRequester"/> — not to make this a general MQTT publisher. This
/// driver has no <c>IWritable</c> leg in v1, so a Sparkplug rebirth NCMD is the <i>only</i>
/// outbound application message it ever sends. Sparkplug rebirth-on-reconnect itself is decided
/// by <see cref="Sparkplug.SparkplugIngestor"/> hanging off <see cref="Reconnected"/>; this type
/// only carries the bytes.
/// <b>The publish leg is deliberately narrow.</b> <see cref="PublishAsync"/> exists to carry
/// outbound application messages for callers that decide <i>what</i> to send, not to make this a
/// general MQTT publisher: <see cref="Sparkplug.IMqttPublishTransport"/> (implemented for
/// <see cref="Sparkplug.RebirthRequester"/>'s NCMDs), <c>MqttDriver.WriteAsync</c>'s encoded
/// write (Gitea #508 — the statement that this driver has no <c>IWritable</c> leg was true only
/// before it) and <see cref="Sparkplug.SparkplugPrimaryHost"/>'s retained STATE. Sparkplug
/// rebirth-on-reconnect itself is decided by <see cref="Sparkplug.SparkplugIngestor"/> hanging
/// off <see cref="Reconnected"/>; this type only carries the bytes.
/// </para>
/// </remarks>
public sealed class MqttConnection : IAsyncDisposable, IMqttSubscribeTransport, Sparkplug.IMqttPublishTransport
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttDriver.cs
+83 -6
View File
@@ -104,6 +104,18 @@ public sealed class MqttDriver
/// <summary>How often the host-connectivity poll samples <see cref="MqttConnection.State"/>.</summary>
private static readonly TimeSpan HostProbeInterval = TimeSpan.FromSeconds(1);
/// <summary>
/// Stand-in for a non-positive authored <c>connectTimeoutSeconds</c> — see <see cref="Normalize"/>.
/// Matches <see cref="MqttDriverOptions.ConnectTimeoutSeconds"/>'s own default.
/// </summary>
private const int DefaultConnectTimeoutSeconds = 15;
/// <summary>
/// Stand-in for a negative authored <c>keepAliveSeconds</c> — see <see cref="Normalize"/>.
/// Matches <see cref="MqttDriverOptions.KeepAliveSeconds"/>'s own default.
/// </summary>
private const int DefaultKeepAliveSeconds = 30;
private readonly string _driverInstanceId;
private readonly ILogger? _logger;
@@ -221,13 +233,13 @@ public sealed class MqttDriver
ArgumentNullException.ThrowIfNull(options);
ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId);
_options = options;
_options = Normalize(options);
_driverInstanceId = driverInstanceId;
_logger = logger;
_primaryRoleSource = primaryRoleSource ?? UnmanagedDriverPrimaryRoleSource.Instance;
_subscriptions = CreateSubscriptionManager(options);
_sparkplug = CreateSparkplugIngestor(options);
RegisterAuthoredTags(options);
_subscriptions = CreateSubscriptionManager(_options);
_sparkplug = CreateSparkplugIngestor(_options);
RegisterAuthoredTags(_options);
}
/// <inheritdoc />
@@ -372,7 +384,12 @@ public sealed class MqttDriver
/// <inheritdoc />
public async Task ShutdownAsync(CancellationToken cancellationToken)
{
var lastMessage = ReadHealth().LastSuccessfulRead;
// GetHealth(), not ReadHealth(): the live connection's LastMessageUtc is the real last-message
// instant, and the stored snapshot only ever carries what was true at the last initialize. Taking
// the stored one here throws away the evidence a shutdown-then-initialize cycle (the #527
// watchdog's reconnect) is measured against, reporting a driver that was delivering a moment ago
// as one that has never delivered.
var lastMessage = GetHealth().LastSuccessfulRead;
if (await _lifecycleGate.WaitAsync(TimeSpan.FromSeconds(_options.ConnectTimeoutSeconds), cancellationToken)
.ConfigureAwait(false))
{
@@ -932,6 +949,17 @@ public sealed class MqttDriver
WriteHealth(new DriverHealth(DriverState.Initializing, ReadHealth().LastSuccessfulRead, null));
try
{
// Tear down whatever this driver already holds BEFORE building a new session. Both fields
// published below (_connection, _primaryHost) are IAsyncDisposable and were previously
// overwritten, so a second initialize on a live driver stranded a connected, supervised
// broker session — plus a whole second primary-host connection — that no later dispose
// could reach. Reachable without any teardown of ours: DriverInstanceActor re-runs
// InitializeAsync with no ShutdownAsync when an ApplyDelta lands while the driver is in
// Connecting/Reconnecting (AdoptConfigDuringInit). Idempotent, so the ReinitializeAsync
// rebuild branch calling TeardownAsync first costs a no-op, and a first initialize sees
// three nulls.
await TeardownAsync().ConfigureAwait(false);
// A blank / unusable blob keeps the constructor's options rather than degrading the
// driver to defaults pointing at localhost.
var parsed = ParseOptions(driverConfigJson);
@@ -1030,7 +1058,8 @@ public sealed class MqttDriver
// deliberately: enums must round-trip by NAME. A second, divergent
// JsonSerializerOptions is this repo's documented systemic enum bug, where an
// AdminUI-authored config with a string enum field faults the driver.
return JsonSerializer.Deserialize<MqttDriverOptions>(driverConfigJson, MqttJson.Options);
var parsed = JsonSerializer.Deserialize<MqttDriverOptions>(driverConfigJson, MqttJson.Options);
return parsed is null ? null : Normalize(parsed);
}
catch (JsonException ex)
{
@@ -1050,6 +1079,54 @@ public sealed class MqttDriver
}
}
/// <summary>
/// Replaces out-of-range duration knobs with their documented defaults, once, at the single seam
/// every options instance this driver holds passes through (the constructor and
/// <see cref="ParseOptions"/>).
/// </summary>
/// <remarks>
/// <para>
/// <see cref="MqttDriverOptions"/> carries <c>[Range]</c> annotations, but nothing validates
/// them on the runtime path: the blob is deserialized straight out of the deploy artifact, so
/// a config authored (or hand-edited) with <c>connectTimeoutSeconds: 0</c> or a negative value
/// reaches the driver unchecked. Both are load-bearing, and neither fails where an operator
/// would look:
/// </para>
/// <list type="bullet">
/// <item>
/// zero makes every <c>new CancellationTokenSource(TimeSpan.Zero)</c> in the connect,
/// subscribe and publish paths fire immediately, so the driver can never connect and
/// reports a timeout against a perfectly healthy broker;
/// </item>
/// <item>
/// a negative value is outside <see cref="SemaphoreSlim.WaitAsync(TimeSpan)"/>'s and
/// <see cref="CancellationTokenSource"/>'s legal range, so it throws
/// <see cref="ArgumentOutOfRangeException"/> out of <see cref="ShutdownAsync"/> and
/// <see cref="DisposeAsync"/> — the two paths whose whole job is to not fail.
/// </item>
/// </list>
/// <para>
/// Normalising here rather than at each use site keeps one answer for the whole driver: the
/// value also rides into <see cref="MqttConnection"/>, <see cref="SparkplugPrimaryHost"/> and
/// <c>MqttClientOptionsFactory</c> through <see cref="_options"/>. <c>MaxPayloadBytes</c> and
/// the reconnect backoffs are deliberately left alone — the manager's constructor and
/// <see cref="MqttConnection.NextBackoff"/> already clamp those at their own use site.
/// </para>
/// </remarks>
private static MqttDriverOptions Normalize(MqttDriverOptions options)
{
var connectTimeout = options.ConnectTimeoutSeconds > 0
? options.ConnectTimeoutSeconds
: DefaultConnectTimeoutSeconds;
// Zero keep-alive is legal MQTT ("no keep-alive"); only a negative one is nonsense.
var keepAlive = options.KeepAliveSeconds >= 0 ? options.KeepAliveSeconds : DefaultKeepAliveSeconds;
return connectTimeout == options.ConnectTimeoutSeconds && keepAlive == options.KeepAliveSeconds
? options
: options with { ConnectTimeoutSeconds = connectTimeout, KeepAliveSeconds = keepAlive };
}
/// <summary>
/// Adopts new options, rebuilding the subscription manager only when a setting it captures at
/// construction actually changed. The rebuilt manager inherits the driver-owned
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs
+23 -5
View File
@@ -183,17 +183,35 @@ public sealed class MqttSubscriptionManager
/// transport.
/// </summary>
/// <remarks>
/// The manager does not own the connection's lifetime and never detaches — the driver disposes
/// both together. Call once per connection, and <b>before</b> the first
/// <see cref="SubscribeAsync"/>: a subscribe issued with no transport attached records its
/// intent (and warns) but sends nothing to a broker, and would only be established if the
/// connection later dropped and recovered.
/// <para>
/// The manager does not own the connection's lifetime and never detaches — the driver disposes
/// both together. Call once per connection, and <b>before</b> the first
/// <see cref="SubscribeAsync"/>: a subscribe issued with no transport attached records its
/// intent (and warns) but sends nothing to a broker, and would only be established if the
/// connection later dropped and recovered.
/// </para>
/// <para>
/// <b>Binding to a session forgets the previous session's established filters</b> — the same
/// reset <see cref="Sparkplug.SparkplugIngestor.AttachTo"/> performs, and for the same reason:
/// this manager <i>outlives</i> a session rebuild. <c>MqttDriver.AdoptOptions</c> only
/// replaces it when the ingest identity changed, so a driver-host reconnect (ShutdownAsync +
/// InitializeAsync, #527's watchdog path) and a <c>ReinitializeAsync</c> that changes only the
/// endpoint/credentials both re-attach <i>this</i> instance to a brand-new broker session.
/// <c>_subscribedTopics</c> is a claim about the session that has just gone: carried across,
/// the host's post-reconnect re-subscribe resolves an empty pending set, issues no SUBSCRIBE
/// at all, and leaves a driver that reports Connected/Healthy and receives nothing until the
/// <i>new</i> session happens to drop — the silent-death outcome every other path in this
/// type is written to prevent.
/// </para>
/// </remarks>
/// <param name="connection">The connection to observe.</param>
public void AttachTo(MqttConnection connection)
{
ArgumentNullException.ThrowIfNull(connection);
// FIRST, before delivery is wired: a new session holds none of the old one's subscriptions.
_subscribedTopics.Clear();
_transport = connection;
connection.MessageReceived += HandleMessage;
connection.Reconnected += OnReconnectedAsync;
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/Sparkplug/SequenceTracker.cs
+13 -5
View File
@@ -285,11 +285,19 @@ public sealed class SequenceTracker
/// session token.
/// </summary>
/// <remarks>
/// The reconnect seam (§3.6 invariant #5 — late join). After a dropped connection the driver
/// has missed an unknown number of messages, so the baseline it was holding is no longer
/// evidence of anything: carrying it across would let the stream look contiguous purely
/// because the edge node happened to publish the right number of messages while the driver was
/// away.
/// <para>
/// The rationale is §3.6 invariant #5 (late join): after a dropped connection the driver has
/// missed an unknown number of messages, so the baseline it was holding is no longer evidence
/// of anything — carrying it across would let the stream look contiguous purely because the
/// edge node happened to publish the right number of messages while the driver was away.
/// </para>
/// <para>
/// <b>The reconnect path does not call this</b>, despite the reasoning being its.
/// <c>SparkplugIngestor.ResetSessionState</c> drops the whole per-node tracker <i>map</i>
/// instead, which additionally releases the entry for a node the plant has stopped
/// publishing. This member is the same reset for a single tracker, kept for a caller holding
/// one directly (the unit suite is currently the only one).
/// </para>
/// </remarks>
public void Reset()
{