fix(mqtt): gate DisposeAsync against in-flight lifecycle ops (C1) + rebuild-branch tests (I1)
C1 (Critical) — DisposeAsync went straight to TeardownAsync with no lifecycle-gate
wait, reopening the orphaned-connection class. Interleaving: ReinitializeAsync's
rebuild branch holds the gate mid-InitializeCoreAsync, past its own teardown but
before `_connection = connection`; an ungated dispose sees a null connection, does
nothing, and sets `_disposed`; the rebuild then publishes a live connection plus a
host-probe loop that every later dispose short-circuits past. Not proven reachable
today (DriverInstanceActor.PostStop calls the gated ShutdownAsync), but MqttDriver
publicly implements IAsyncDisposable, which invites `await using`.
- DisposeAsync now takes the gate with the same bounded wait + fallback teardown
ShutdownAsync uses, and sets `_disposed` BEFORE the wait.
- InitializeCoreAsync re-checks `_disposed` after the connect and disposes the
connection it just built rather than publishing it — this closes the residual
window on the bounded-timeout fallback path.
- The doc comment no longer claims parity with ShutdownAsync it did not have, and
stops conflating "don't Dispose() the semaphore object" with "don't WaitAsync".
I1 (Important) — the SameSession == false rebuild branch had no test driving it.
Adds three: an endpoint-changing delta rebuilds and Faults on a refused connect;
an ingest-only delta (MaxPayloadBytes) ALSO rebuilds, pinning that IngestIdentity
is nested inside SessionIdentity; and DisposeAsync serializes behind a lifecycle
operation parked at a new internal BeforeConnectHookForTests seam (mirroring
MqttConnection's AfterConnectHookForTests, which exists for the same race class).
Minors: comments recording that IngestIdentity names no Sparkplug field and must
grow one in P2 (tasks 21/22), and why _options/_subscriptions/_authoredRawPaths
get looser memory discipline than _health/_hostState.
Falsifiability: reverting DisposeAsync to the ungated form reddens exactly the new
serialization test ("Shouldly.ShouldAssertException : raced"); forcing SameSession
to always-true reddens exactly the two new rebuild tests. 240/240 MQTT tests pass;
forced rebuild of the driver project is 0 warnings under TreatWarningsAsErrors.
Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
@@ -72,6 +72,14 @@ public sealed class MqttDriver
|
||||
/// <summary>Guards <see cref="InitializeAsync"/> / <see cref="ReinitializeAsync"/> / <see cref="ShutdownAsync"/> against each other.</summary>
|
||||
private readonly SemaphoreSlim _lifecycleGate = new(1, 1);
|
||||
|
||||
// _options / _subscriptions / _authoredRawPaths are written only under _lifecycleGate (or in the
|
||||
// ctor) and read freely elsewhere, WITHOUT the Volatile discipline _health / _hostState get.
|
||||
// That is deliberate, not an oversight: all three are reference/interface fields, so a reader
|
||||
// sees either the whole old object or the whole new one — never a torn one. The looser fields
|
||||
// are the ones a reader may legitimately observe one publish stale (a read served from the
|
||||
// previous authored table is a correct read of a value that was correct a microsecond ago);
|
||||
// _health and _hostState feed ServiceLevel and the status dashboard, where a stale per-core copy
|
||||
// is an operator-visible lie about whether the driver is up.
|
||||
private MqttDriverOptions _options;
|
||||
private MqttSubscriptionManager _subscriptions;
|
||||
|
||||
@@ -133,6 +141,16 @@ public sealed class MqttDriver
|
||||
/// </summary>
|
||||
internal MqttSubscriptionManager Subscriptions => _subscriptions;
|
||||
|
||||
/// <summary>
|
||||
/// Test seam: awaited inside <see cref="InitializeCoreAsync"/> <b>while the lifecycle gate is
|
||||
/// held</b>, immediately before the broker connect. Lets a test park a lifecycle operation
|
||||
/// mid-flight and prove <see cref="DisposeAsync"/> serializes behind it rather than racing
|
||||
/// past a half-built session. Mirrors <see cref="MqttConnection"/>'s own
|
||||
/// <c>AfterConnectHookForTests</c>, which exists for the same class of race. Always
|
||||
/// <see langword="null"/> in production.
|
||||
/// </summary>
|
||||
internal Func<CancellationToken, Task>? BeforeConnectHookForTests { get; set; }
|
||||
|
||||
// ---- IDriver: lifecycle ----
|
||||
|
||||
/// <summary>
|
||||
@@ -437,9 +455,37 @@ public sealed class MqttDriver
|
||||
public void Dispose() => DisposeAsync().AsTask().GetAwaiter().GetResult();
|
||||
|
||||
/// <summary>
|
||||
/// Performs the same teardown as <see cref="ShutdownAsync"/> so a caller that uses
|
||||
/// <c>await using</c> without an explicit shutdown does not leak a live broker session.
|
||||
/// Performs the same teardown as <see cref="ShutdownAsync"/>, <b>under the same lifecycle
|
||||
/// gate</b>, so a caller that uses <c>await using</c> without an explicit shutdown does not
|
||||
/// leak a live broker session.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// <b>The gate wait is the whole point, not ceremony.</b> An ungated dispose that ran
|
||||
/// while <see cref="ReinitializeAsync"/>'s rebuild branch held the gate would observe
|
||||
/// <c>_connection == null</c> — the rebuild has torn the old session down but not yet
|
||||
/// assigned the new one — do nothing, and set <c>_disposed</c>. The rebuild would then
|
||||
/// complete, assign a <i>live, connected</i> connection and start a host-probe loop that
|
||||
/// no later dispose can ever reach, because the disposed guard short-circuits them all.
|
||||
/// That is the orphaned-connection class <see cref="MqttConnection"/>'s own remarks
|
||||
/// describe having already fixed once. Waiting for the gate makes this dispose tear down
|
||||
/// whatever the in-flight operation ends up assigning.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <c>_disposed</c> is set <b>before</b> the wait, so an in-flight
|
||||
/// <see cref="InitializeCoreAsync"/> sees it at its post-connect re-check and refuses to
|
||||
/// publish the connection at all — which is what closes the residual window on the
|
||||
/// bounded-timeout fallback path below.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Separately (and for an unrelated reason): the gate object itself is deliberately never
|
||||
/// <c>Dispose()</c>d — same call as <see cref="MqttConnection"/>'s semaphores. A caller
|
||||
/// that disposes before its last <see cref="ShutdownAsync"/> would otherwise get an
|
||||
/// <see cref="ObjectDisposedException"/> naming <see cref="SemaphoreSlim"/>, which says
|
||||
/// nothing useful; the gate holds no timer and no surviving registration, so leaving it
|
||||
/// undisposed costs nothing.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <returns>A task that represents the asynchronous dispose.</returns>
|
||||
public async ValueTask DisposeAsync()
|
||||
{
|
||||
@@ -448,10 +494,27 @@ public sealed class MqttDriver
|
||||
return;
|
||||
}
|
||||
|
||||
// The lifecycle gate is deliberately NOT disposed — same call as MqttConnection's semaphores.
|
||||
// A caller that disposes before its last ShutdownAsync would otherwise get an
|
||||
// ObjectDisposedException naming SemaphoreSlim, which says nothing useful; the gate holds no
|
||||
// timer and no surviving registration, so leaving it undisposed costs nothing.
|
||||
if (await _lifecycleGate.WaitAsync(TimeSpan.FromSeconds(_options.ConnectTimeoutSeconds))
|
||||
.ConfigureAwait(false))
|
||||
{
|
||||
try
|
||||
{
|
||||
await TeardownAsync().ConfigureAwait(false);
|
||||
}
|
||||
finally
|
||||
{
|
||||
_lifecycleGate.Release();
|
||||
}
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
// Bounded even here: a wedged initialize must not turn dispose into a hang. The post-connect
|
||||
// disposed re-check in InitializeCoreAsync is what stops that operation from publishing a
|
||||
// connection behind this teardown's back.
|
||||
_logger?.LogWarning(
|
||||
"MQTT driver '{DriverId}': lifecycle gate still held at dispose; tearing the session down anyway.",
|
||||
_driverInstanceId);
|
||||
await TeardownAsync().ConfigureAwait(false);
|
||||
}
|
||||
|
||||
@@ -480,7 +543,24 @@ public sealed class MqttDriver
|
||||
// throw-on-total-failure is what tears a deaf session down and retries it.
|
||||
_subscriptions.AttachTo(connection);
|
||||
|
||||
if (BeforeConnectHookForTests is { } hook)
|
||||
{
|
||||
await hook(cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
|
||||
await connection.ConnectAsync(cancellationToken).ConfigureAwait(false);
|
||||
|
||||
// Re-check disposal AFTER the connect, before publishing the connection. DisposeAsync
|
||||
// normally waits for this gate, but its wait is bounded — on the wedged-gate fallback
|
||||
// path it can run concurrently with this method, and assigning a live connection behind
|
||||
// a completed teardown is exactly the orphaned-session bug. Losing the race means
|
||||
// disposing what we just built, never leaking it.
|
||||
if (Volatile.Read(ref _disposed) != 0)
|
||||
{
|
||||
await connection.DisposeAsync().ConfigureAwait(false);
|
||||
throw new ObjectDisposedException(nameof(MqttDriver));
|
||||
}
|
||||
|
||||
_connection = connection;
|
||||
|
||||
WriteHealth(new DriverHealth(DriverState.Healthy, connection.LastMessageUtc, null));
|
||||
@@ -597,6 +677,16 @@ public sealed class MqttDriver
|
||||
=> SessionIdentity(a).Equals(SessionIdentity(b));
|
||||
|
||||
/// <summary>Whether two option sets produce an identical <see cref="MqttSubscriptionManager"/>.</summary>
|
||||
/// <remarks>
|
||||
/// ⚠️ <b>P2 (Sparkplug, tasks 21/22) must extend <see cref="IngestIdentity"/>.</b> It names
|
||||
/// only the settings the <b>P1</b> manager captures at construction — <c>Mode</c>,
|
||||
/// <c>Plain.DefaultQos</c>, <c>MaxPayloadBytes</c> — and deliberately no
|
||||
/// <see cref="MqttSparkplugOptions"/> field, because nothing reads one yet. The moment a
|
||||
/// Sparkplug handler captures its own options (group id, host id, rebirth policy, birth
|
||||
/// window) at construction, a delta changing one of them would be judged "same ingest",
|
||||
/// applied in place, and silently never reach the component that needed rebuilding — the
|
||||
/// driver would keep subscribing to the OLD group id and report perfect health.
|
||||
/// </remarks>
|
||||
private static bool SameIngest(MqttDriverOptions a, MqttDriverOptions b)
|
||||
=> IngestIdentity(a).Equals(IngestIdentity(b));
|
||||
|
||||
|
||||
Reference in New Issue
Block a user