Skip to content

Commit f7e43df

Browse files
jamesarichclaude
andauthored
fix(client): classify connect failures by origin, plus 3.1.1 silent-close fallback (#113)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 0a7b940 commit f7e43df

10 files changed

Lines changed: 990 additions & 75 deletions

File tree

CHANGELOG.md

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,74 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Added
11+
12+
- `MqttException.ConnectionFailed` — a new subtype for a connect attempt that failed before the
13+
broker accepted or refused it: DNS, TCP connect, TLS handshake, a socket that closed
14+
mid-handshake, or a CONNACK that never arrived.
15+
16+
### Changed
17+
18+
- **`connect()` no longer reports every failure as `MqttException.ConnectionRejected`.** It now
19+
classifies by which side of the handshake failed, because a consumer cannot write a correct retry
20+
policy without that distinction: a rejection means the broker read the CONNECT and said no, so
21+
retrying the same configuration is pointless, whereas a network failure is exactly what a retry
22+
is for. Previously both arrived as `ConnectionRejected`, so a TCP timeout or a TLS chain failure
23+
was indistinguishable from a bad password — a caller that stopped on `ConnectionRejected` gave up
24+
permanently on a transient network blip, and one that retried hammered a broker that would never
25+
accept it. Downstream consumers had to reach past the type and re-derive the answer from reason
26+
codes.
27+
28+
`connect()` now throws:
29+
30+
- `ConnectionRejected` only for a genuine CONNACK carrying an error reason code — including an
31+
unsupported protocol version that could not be renegotiated, and including a CONNACK whose
32+
reason code is `PROTOCOL_ERROR`, since the broker still answered. `serverReference` is
33+
preserved.
34+
- `ConnectionFailed` for transport and I/O failures, with the original platform exception kept as
35+
`cause`.
36+
- `ProtocolError` when the broker answered but violated the spec (a malformed CONNACK, an
37+
unexpected packet type mid-handshake).
38+
39+
Reason codes alone could not express this — a broker may legitimately refuse a CONNECT with
40+
`PROTOCOL_ERROR`, and a transport failure has no broker reason code at all and synthesises
41+
`UNSPECIFIED_ERROR` — so the connection layer now records the failure's origin explicitly.
42+
43+
**Migration:** callers matching `MqttException.ConnectionRejected` to stop retrying keep working
44+
and get more accurate: only real refusals land there now. Code that relied on `ConnectionRejected`
45+
as the catch-all for *any* connect failure should add a `ConnectionFailed` branch, or catch
46+
`MqttException`. Auto-reconnect behaviour is unchanged — it already classified by reason code and
47+
never stopped on a transport failure.
48+
49+
### Fixed
50+
51+
- MQTT 5.0 → 3.1.1 version negotiation now also triggers when a broker **silently closes** the
52+
connection on an MQTT 5.0 CONNECT instead of answering with an `UNSUPPORTED_PROTOCOL_VERSION`
53+
CONNACK. Many older brokers and gateways (observed live on `mqtt.defcon.run:4433`) drop the
54+
connection without any reply, which previously surfaced as a bare transport error
55+
(`EOFException: Not enough data available`) — the Meshtastic Android app misreported it as a
56+
credentials problem, while iOS clients, which speak 3.1.1 natively, connected fine. The client
57+
and `MqttClient.probe` now retry once with MQTT 3.1.1 when `negotiateVersion` is enabled (the
58+
default). The fallback is phase-gated: it fires only when the connection failed *after* the
59+
CONNECT packet was written and *before* a CONNACK arrived. DNS, TCP, and TLS failures — and
60+
CONNACK timeouts, where the broker kept the connection open — do not trigger it, so real network
61+
errors are never masked and dead hosts don't pay a doubled connect latency. Those failures are
62+
still reported accurately, as `ConnectionFailed` per the classification change above. The probe's
63+
retry runs within the remaining `timeoutMs` budget, preserving its total wall-clock contract.
64+
`probe` additionally gains the explicit `UNSUPPORTED_PROTOCOL_VERSION` fallback the client
65+
already had, so probing a 3.1.1-only broker now reports `Success` instead of `Rejected`.
66+
- A failed handshake no longer leaks the socket. Rejections raised while awaiting CONNACK — an
67+
unexpected packet type, an AUTH packet during an MQTT 3.1.1 handshake — took a rethrow path that
68+
skipped transport cleanup, as did an invalid Maximum QoS value in the CONNACK. Handshake cleanup
69+
is now centralised and covers every rejection path.
70+
- Cancelling `connect()` mid-handshake no longer leaks the socket either. The cancellation branch
71+
rethrows to preserve structured concurrency and so bypassed the failure paths' cleanup; it now
72+
closes the transport on a `NonCancellable` context, because the coroutine is already cancelled
73+
and a plain suspending close would abort immediately.
74+
- `probe()` now reports a transport-factory failure as a `ProbeResult` instead of throwing. A
75+
composite factory with no delegate for the endpoint throws synchronously from `create()`, which
76+
was evaluated outside the classified path and escaped as a raw `IllegalArgumentException`.
77+
1078
## [0.7.0] - 2026-07-26
1179

1280
### Added

core/api/core.klib.api

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -723,6 +723,10 @@ sealed class org.meshtastic.mqtt/MqttException : kotlin/Exception { // org.mesht
723723
final val reasonCode // org.meshtastic.mqtt/MqttException.reasonCode|{}reasonCode[0]
724724
final fun <get-reasonCode>(): org.meshtastic.mqtt/ReasonCode // org.meshtastic.mqtt/MqttException.reasonCode.<get-reasonCode>|<get-reasonCode>(){}[0]
725725

726+
final class ConnectionFailed : org.meshtastic.mqtt/MqttException { // org.meshtastic.mqtt/MqttException.ConnectionFailed|null[0]
727+
constructor <init>(org.meshtastic.mqtt/ReasonCode, kotlin/String, kotlin/Throwable? = ...) // org.meshtastic.mqtt/MqttException.ConnectionFailed.<init>|<init>(org.meshtastic.mqtt.ReasonCode;kotlin.String;kotlin.Throwable?){}[0]
728+
}
729+
726730
final class ConnectionLost : org.meshtastic.mqtt/MqttException { // org.meshtastic.mqtt/MqttException.ConnectionLost|null[0]
727731
constructor <init>(org.meshtastic.mqtt/ReasonCode, kotlin/String, kotlin/Throwable? = ...) // org.meshtastic.mqtt/MqttException.ConnectionLost.<init>|<init>(org.meshtastic.mqtt.ReasonCode;kotlin.String;kotlin.Throwable?){}[0]
728732
}

core/api/jvm/core.api

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,11 @@ public abstract class org/meshtastic/mqtt/MqttException : java/lang/Exception {
269269
public final fun getReasonCode ()Lorg/meshtastic/mqtt/ReasonCode;
270270
}
271271

272+
public final class org/meshtastic/mqtt/MqttException$ConnectionFailed : org/meshtastic/mqtt/MqttException {
273+
public fun <init> (Lorg/meshtastic/mqtt/ReasonCode;Ljava/lang/String;Ljava/lang/Throwable;)V
274+
public synthetic fun <init> (Lorg/meshtastic/mqtt/ReasonCode;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
275+
}
276+
272277
public final class org/meshtastic/mqtt/MqttException$ConnectionLost : org/meshtastic/mqtt/MqttException {
273278
public fun <init> (Lorg/meshtastic/mqtt/ReasonCode;Ljava/lang/String;Ljava/lang/Throwable;)V
274279
public synthetic fun <init> (Lorg/meshtastic/mqtt/ReasonCode;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V

core/src/commonMain/kotlin/org/meshtastic/mqtt/MqttClient.kt

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -363,9 +363,23 @@ public class MqttClient
363363
* `SERVER_MOVED` with a Server Reference property), the client automatically follows
364364
* the redirect up to [MAX_REDIRECTS] times (§4.13).
365365
*
366+
* Failures are classified by which side of the handshake produced them, so a caller can
367+
* tell an unusable configuration from a bad network and pick a retry policy accordingly.
368+
*
366369
* @param endpoint Broker endpoint (TCP or WebSocket).
367-
* @throws MqttException.ConnectionRejected if the broker rejects the connection.
370+
* @throws MqttException.ConnectionRejected if the broker answered with a CONNACK carrying
371+
* an error reason code — including an unsupported protocol version that could not be
372+
* renegotiated. Retrying the same configuration will fail the same way.
373+
* @throws MqttException.ConnectionFailed if the transport failed before any CONNACK
374+
* arrived (DNS, TCP, TLS, a socket closed mid-handshake, or a CONNACK timeout). The
375+
* broker never expressed an opinion, so a retry may succeed.
376+
* @throws MqttException.ProtocolError if the broker answered but violated the spec.
377+
*
378+
* `SwallowedException` is suppressed deliberately: the internal exception is not chained as
379+
* the cause, because [toConnectException] carries over its reason code, message, server
380+
* reference and its own cause, and the internal type is not part of the public API.
368381
*/
382+
@Suppress("SwallowedException")
369383
@Throws(MqttException::class, kotlin.coroutines.cancellation.CancellationException::class)
370384
public suspend fun connect(endpoint: MqttEndpoint) {
371385
try {
@@ -395,16 +409,10 @@ public class MqttClient
395409
connectWithRedirect(endpoint, redirectsRemaining = MAX_REDIRECTS)
396410
}
397411
} catch (e: MqttConnectionException) {
398-
val rejected =
399-
MqttException.ConnectionRejected(
400-
reasonCode = e.reasonCode,
401-
message = e.message ?: "Connection failed",
402-
cause = e.cause,
403-
serverReference = e.serverReference,
404-
)
412+
val failure = e.toConnectException()
405413
// Forwarding hasn't started yet (connection==null on failure), so we own the state.
406-
_connectionState.value = ConnectionState.Disconnected(reason = rejected)
407-
throw rejected
414+
_connectionState.value = ConnectionState.Disconnected(reason = failure)
415+
throw failure
408416
}
409417
}
410418

@@ -729,14 +737,20 @@ public class MqttClient
729737
try {
730738
conn.connect(endpoint)
731739
} catch (e: MqttConnectionException) {
732-
// Version fallback: if broker rejected V5_0, try V3_1_1 on a fresh transport
733-
if (
734-
e.reasonCode == ReasonCode.UNSUPPORTED_PROTOCOL_VERSION &&
735-
effectiveVersion == MqttProtocolVersion.V5_0 &&
736-
config.negotiateVersion
737-
) {
740+
// Version fallback: if broker rejected V5_0 — either explicitly with an
741+
// UNSUPPORTED_PROTOCOL_VERSION CONNACK, or silently by closing the connection
742+
// after the CONNECT was written without ever answering (many older brokers and
743+
// gateways do this) — try V3_1_1 on a fresh transport.
744+
val silentClose = e.closedBeforeConnAck
745+
val v5Refused = e.reasonCode == ReasonCode.UNSUPPORTED_PROTOCOL_VERSION || silentClose
746+
if (v5Refused && effectiveVersion == MqttProtocolVersion.V5_0 && config.negotiateVersion) {
738747
log.info(TAG) {
739-
"Broker rejected MQTT 5.0 — falling back to MQTT 3.1.1"
748+
if (silentClose) {
749+
"Broker closed the connection without answering the MQTT 5.0 CONNECT — " +
750+
"falling back to MQTT 3.1.1"
751+
} else {
752+
"Broker rejected MQTT 5.0 — falling back to MQTT 3.1.1"
753+
}
740754
}
741755
// Validate the config is compatible with 3.1.1 before retrying
742756
try {

0 commit comments

Comments
 (0)