fix(mqtt): a broker-refused CONNECT no longer reports a Healthy driver
MQTTnet 5 does not throw on an unsuccessful CONNACK — it returns the reason in MqttClientConnectResult.ResultCode and v5 removed v4's ThrowOnNonSuccessfulConnectResponse, so ConnectCoreAsync's unconditional SetState(Connected) sealed a wrong-password deployment green: DriverState.Healthy / HostState.Running on a session that never authenticated. Caught by the Task-13 Mosquitto fixture; 240 offline tests missed it. ConnectCoreAsync now inspects the CONNACK and raises MqttConnectRejectedException. Credentials/identity/protocol/Last-Will refusals are unrecoverable and set Faulted, which stops the reconnect supervisor; transient refusals (ServerUnavailable, ServerBusy, QuotaExceeded, UseAnotherServer, ConnectionRateExceeded, and anything unrecognised) stay Reconnecting under backoff. MqttDriverProbe now shares the rejection wording, so the AdminUI Test-connect button and the running driver can no longer disagree. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
Joseph Doherty
@@ -927,6 +927,168 @@ public sealed class MqttConnectionTests
|
||||
.ShouldBeTrue();
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------------
|
||||
// Refused CONNACK — MQTTnet 5 reports it as a RESULT, never an exception
|
||||
// ---------------------------------------------------------------------------------
|
||||
|
||||
/// <summary>
|
||||
/// The Task-13 live-gate defect, reproduced offline: Mosquitto genuinely rejected a wrong
|
||||
/// password (<c>Client … disconnected, not authorised.</c>) and <c>ConnectAsync</c> returned
|
||||
/// normally, because MQTTnet 5 hands a refused CONNACK back in
|
||||
/// <c>MqttClientConnectResult.ResultCode</c> and v5 removed v4's
|
||||
/// <c>ThrowOnNonSuccessfulConnectResponse</c> option.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task ConnectAsync_BrokerRefusesTheCredentials_Throws_AndFaults()
|
||||
{
|
||||
using var broker = new MiniBroker { ConnAckReturnCode = MiniBroker.ReturnCodeNotAuthorized };
|
||||
await using var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null);
|
||||
|
||||
var ex = await Should.ThrowAsync<MqttConnectRejectedException>(
|
||||
async () => await conn.ConnectAsync(CancellationToken.None));
|
||||
|
||||
ex.ResultCode.ShouldBe(MqttClientConnectResultCode.NotAuthorized);
|
||||
ex.IsUnrecoverable.ShouldBeTrue();
|
||||
ex.Message.ShouldContain("rejected the credentials");
|
||||
conn.IsConnected.ShouldBeFalse();
|
||||
conn.State.ShouldBe(MqttConnectionState.Faulted);
|
||||
}
|
||||
|
||||
/// <summary>Credentials must never reach the message an operator (or a log sink) will see.</summary>
|
||||
[Fact]
|
||||
public async Task ConnectAsync_RefusedCredentials_DoesNotLeakThePasswordIntoTheRejection()
|
||||
{
|
||||
const string secret = "sup3r-s3cret-broker-pw";
|
||||
using var broker = new MiniBroker { ConnAckReturnCode = MiniBroker.ReturnCodeBadUserNameOrPassword };
|
||||
var opts = BrokerOpts(broker.Port) with { Username = "svc", Password = secret };
|
||||
await using var conn = new MqttConnection(opts, driverId: "t", logger: null);
|
||||
|
||||
var ex = await Should.ThrowAsync<MqttConnectRejectedException>(
|
||||
async () => await conn.ConnectAsync(CancellationToken.None));
|
||||
|
||||
ex.ToString().ShouldNotContain(secret);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// An unrecoverable rejection must not leave a supervisor grinding away every backoff period
|
||||
/// against a broker that will never accept these credentials.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task ConnectAsync_RefusedCredentials_StartsNoReconnectSupervisor()
|
||||
{
|
||||
using var broker = new MiniBroker { ConnAckReturnCode = MiniBroker.ReturnCodeNotAuthorized };
|
||||
var opts = BrokerOpts(broker.Port) with { ReconnectMinBackoffSeconds = 1, ReconnectMaxBackoffSeconds = 1 };
|
||||
await using var conn = new MqttConnection(opts, driverId: "t", logger: null);
|
||||
|
||||
await Should.ThrowAsync<MqttConnectRejectedException>(
|
||||
async () => await conn.ConnectAsync(CancellationToken.None));
|
||||
var acceptedAfterTheAttempt = broker.AcceptedCount;
|
||||
|
||||
await Task.Delay(TimeSpan.FromSeconds(4), TestContext.Current.CancellationToken);
|
||||
|
||||
broker.AcceptedCount.ShouldBe(acceptedAfterTheAttempt, "a hopeless rejection is being retried");
|
||||
conn.State.ShouldBe(MqttConnectionState.Faulted);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A genuinely transient refusal is NOT a fault. Faulting on <c>ServerUnavailable</c> would take
|
||||
/// a recoverable driver down until someone redeployed it.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task ConnectAsync_TransientRefusal_Throws_ButStaysRetryable()
|
||||
{
|
||||
using var broker = new MiniBroker { ConnAckReturnCode = MiniBroker.ReturnCodeServerUnavailable };
|
||||
await using var conn = new MqttConnection(BrokerOpts(broker.Port), driverId: "t", logger: null);
|
||||
|
||||
var ex = await Should.ThrowAsync<MqttConnectRejectedException>(
|
||||
async () => await conn.ConnectAsync(CancellationToken.None));
|
||||
|
||||
ex.ResultCode.ShouldBe(MqttClientConnectResultCode.ServerUnavailable);
|
||||
ex.IsUnrecoverable.ShouldBeFalse();
|
||||
conn.State.ShouldNotBe(MqttConnectionState.Faulted);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(MqttClientConnectResultCode.NotAuthorized, true)]
|
||||
[InlineData(MqttClientConnectResultCode.BadUserNameOrPassword, true)]
|
||||
[InlineData(MqttClientConnectResultCode.ClientIdentifierNotValid, true)]
|
||||
[InlineData(MqttClientConnectResultCode.UnsupportedProtocolVersion, true)]
|
||||
[InlineData(MqttClientConnectResultCode.Banned, true)]
|
||||
[InlineData(MqttClientConnectResultCode.BadAuthenticationMethod, true)]
|
||||
[InlineData(MqttClientConnectResultCode.TopicNameInvalid, true)]
|
||||
[InlineData(MqttClientConnectResultCode.QoSNotSupported, true)]
|
||||
// MQTT 5: 0x9D "permanently use another server" vs 0x9C "temporarily use another server".
|
||||
[InlineData(MqttClientConnectResultCode.ServerMoved, true)]
|
||||
[InlineData(MqttClientConnectResultCode.UseAnotherServer, false)]
|
||||
[InlineData(MqttClientConnectResultCode.ServerUnavailable, false)]
|
||||
[InlineData(MqttClientConnectResultCode.ServerBusy, false)]
|
||||
[InlineData(MqttClientConnectResultCode.QuotaExceeded, false)]
|
||||
[InlineData(MqttClientConnectResultCode.ConnectionRateExceeded, false)]
|
||||
// Carries no actionable information, so it must not be allowed to permanently fault a driver.
|
||||
[InlineData(MqttClientConnectResultCode.UnspecifiedError, false)]
|
||||
[InlineData(MqttClientConnectResultCode.ImplementationSpecificError, false)]
|
||||
public void IsUnrecoverableConnackRejection_ClassifiesByWhetherRetryingCouldEverHelp(
|
||||
MqttClientConnectResultCode code,
|
||||
bool expected)
|
||||
=> MqttConnection.IsUnrecoverableConnackRejection(code).ShouldBe(expected);
|
||||
|
||||
/// <summary>
|
||||
/// A reason code this build has never seen must default to retryable — see the classification
|
||||
/// remarks for why the ambiguous case fails toward availability, not toward Faulted.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void IsUnrecoverableConnackRejection_UnknownCode_DefaultsToRetryable()
|
||||
=> MqttConnection.IsUnrecoverableConnackRejection((MqttClientConnectResultCode)250).ShouldBeFalse();
|
||||
|
||||
/// <summary>
|
||||
/// The supervisor's own attempts go through the same CONNACK check. A broker that starts
|
||||
/// refusing mid-life (credentials revoked, client banned) must stop the loop, not feed it.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task Reconnect_BrokerStartsRefusing_FaultsAndStopsTheSupervisor()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
var opts = BrokerOpts(broker.Port) with { ReconnectMinBackoffSeconds = 1, ReconnectMaxBackoffSeconds = 1 };
|
||||
await using var conn = new MqttConnection(opts, driverId: "t", logger: null);
|
||||
var resubscribes = 0;
|
||||
conn.Reconnected += _ => { Interlocked.Increment(ref resubscribes); return Task.CompletedTask; };
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
broker.ConnAckReturnCode = MiniBroker.ReturnCodeNotAuthorized; // credentials revoked
|
||||
broker.DropAll();
|
||||
|
||||
(await WaitUntilAsync(() => conn.State == MqttConnectionState.Faulted, TimeSpan.FromSeconds(20)))
|
||||
.ShouldBeTrue("a revoked-credentials reconnect must fault, not retry forever");
|
||||
var acceptedAtFault = broker.AcceptedCount;
|
||||
await Task.Delay(TimeSpan.FromSeconds(4), TestContext.Current.CancellationToken);
|
||||
|
||||
broker.AcceptedCount.ShouldBe(acceptedAtFault, "the supervisor kept retrying a hopeless rejection");
|
||||
Volatile.Read(ref resubscribes).ShouldBe(0, "nothing was ever re-subscribed on a refused session");
|
||||
}
|
||||
|
||||
/// <summary>The mirror case: a transient refusal keeps the supervisor retrying under backoff.</summary>
|
||||
[Fact]
|
||||
public async Task Reconnect_TransientRefusal_KeepsRetryingUnderBackoff()
|
||||
{
|
||||
using var broker = new MiniBroker();
|
||||
var opts = BrokerOpts(broker.Port) with { ReconnectMinBackoffSeconds = 1, ReconnectMaxBackoffSeconds = 2 };
|
||||
await using var conn = new MqttConnection(opts, driverId: "t", logger: null);
|
||||
|
||||
await conn.ConnectAsync(CancellationToken.None);
|
||||
var acceptedWhileHealthy = broker.AcceptedCount;
|
||||
broker.ConnAckReturnCode = MiniBroker.ReturnCodeServerUnavailable;
|
||||
broker.DropAll();
|
||||
|
||||
(await WaitUntilAsync(() => broker.AcceptedCount >= acceptedWhileHealthy + 2, TimeSpan.FromSeconds(20)))
|
||||
.ShouldBeTrue("the supervisor gave up on a transiently-unavailable broker");
|
||||
conn.State.ShouldBe(MqttConnectionState.Reconnecting, "a busy broker is retryable, not Faulted");
|
||||
|
||||
// And it recovers on its own once the broker starts accepting again.
|
||||
broker.ConnAckReturnCode = MiniBroker.ReturnCodeAccepted;
|
||||
(await WaitUntilAsync(() => conn.State == MqttConnectionState.Connected, TimeSpan.FromSeconds(20)))
|
||||
.ShouldBeTrue();
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------------
|
||||
// helpers
|
||||
// ---------------------------------------------------------------------------------
|
||||
@@ -1150,156 +1312,4 @@ public sealed class MqttConnectionTests
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The smallest broker that can complete an MQTT 3.1.1 handshake: accept TCP, answer CONNECT
|
||||
/// with a success CONNACK, answer PINGREQ with PINGRESP, and nothing else. That is enough for
|
||||
/// MQTTnet to report a real connection — which is what makes a real drop
|
||||
/// (<see cref="DropAll"/>) drive a real <c>DisconnectedAsync</c> and a real reconnect, rather
|
||||
/// than a test seam standing in for one.
|
||||
/// </summary>
|
||||
private sealed class MiniBroker : IDisposable
|
||||
{
|
||||
private static readonly byte[] ConnAckAccepted = [0x20, 0x02, 0x00, 0x00];
|
||||
private static readonly byte[] PingResp = [0xD0, 0x00];
|
||||
|
||||
private readonly CancellationTokenSource _cts = new();
|
||||
private readonly HashSet<TcpClient> _live = [];
|
||||
private readonly TcpListener _listener;
|
||||
private int _acceptedCount;
|
||||
|
||||
public MiniBroker()
|
||||
{
|
||||
_listener = new TcpListener(IPAddress.Loopback, 0);
|
||||
_listener.Start();
|
||||
Port = ((IPEndPoint)_listener.LocalEndpoint).Port;
|
||||
_ = Task.Run(AcceptLoopAsync);
|
||||
}
|
||||
|
||||
public int Port { get; }
|
||||
|
||||
/// <summary>When set, TCP is still accepted but CONNECT is never answered (frozen peer).</summary>
|
||||
public bool Blackhole { get; set; }
|
||||
|
||||
/// <summary>Total TCP connections accepted since construction — counts reconnect attempts.</summary>
|
||||
public int AcceptedCount => Volatile.Read(ref _acceptedCount);
|
||||
|
||||
/// <summary>
|
||||
/// Sockets the broker still has open. Drops to zero only once the peer actually closes,
|
||||
/// so a leaked-but-live client keeps this above zero.
|
||||
/// </summary>
|
||||
public int LiveConnections
|
||||
{
|
||||
get
|
||||
{
|
||||
lock (_live)
|
||||
{
|
||||
return _live.Count;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Kills every established session — the "broker went away" event.</summary>
|
||||
public void DropAll()
|
||||
{
|
||||
lock (_live)
|
||||
{
|
||||
foreach (var client in _live)
|
||||
{
|
||||
try
|
||||
{
|
||||
client.Close();
|
||||
}
|
||||
catch (Exception)
|
||||
{
|
||||
// Already gone.
|
||||
}
|
||||
}
|
||||
|
||||
_live.Clear();
|
||||
}
|
||||
}
|
||||
|
||||
public void Dispose()
|
||||
{
|
||||
_cts.Cancel();
|
||||
_listener.Stop();
|
||||
DropAll();
|
||||
_cts.Dispose();
|
||||
}
|
||||
|
||||
private async Task AcceptLoopAsync()
|
||||
{
|
||||
try
|
||||
{
|
||||
while (!_cts.IsCancellationRequested)
|
||||
{
|
||||
var client = await _listener.AcceptTcpClientAsync(_cts.Token);
|
||||
Interlocked.Increment(ref _acceptedCount);
|
||||
lock (_live)
|
||||
{
|
||||
_live.Add(client);
|
||||
}
|
||||
|
||||
_ = Task.Run(() => ServeAsync(client));
|
||||
}
|
||||
}
|
||||
catch (Exception)
|
||||
{
|
||||
// Listener stopped / token cancelled — the fixture is going away.
|
||||
}
|
||||
}
|
||||
|
||||
private async Task ServeAsync(TcpClient client)
|
||||
{
|
||||
var buffer = new byte[1024];
|
||||
try
|
||||
{
|
||||
var stream = client.GetStream();
|
||||
while (!_cts.IsCancellationRequested)
|
||||
{
|
||||
var read = await stream.ReadAsync(buffer, _cts.Token);
|
||||
if (read == 0)
|
||||
{
|
||||
break; // peer closed — this is how a disposed client stops counting as live
|
||||
}
|
||||
|
||||
if (Blackhole)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
var packetType = buffer[0] & 0xF0;
|
||||
if (packetType == 0x10)
|
||||
{
|
||||
await stream.WriteAsync(ConnAckAccepted, _cts.Token);
|
||||
}
|
||||
else if (packetType == 0xC0)
|
||||
{
|
||||
await stream.WriteAsync(PingResp, _cts.Token);
|
||||
}
|
||||
}
|
||||
}
|
||||
catch (Exception)
|
||||
{
|
||||
// Socket torn down by either side.
|
||||
}
|
||||
finally
|
||||
{
|
||||
lock (_live)
|
||||
{
|
||||
_live.Remove(client);
|
||||
}
|
||||
|
||||
try
|
||||
{
|
||||
client.Dispose();
|
||||
}
|
||||
catch (Exception)
|
||||
{
|
||||
// Already gone.
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user