fix(mqtt): stop reporting transport failures as credential rejections

[jamesarich]

Fixes #6505

MQTT stopped working for users on self-hosted and event brokers, and the app blamed their credentials for it. The reporter in #6505 (DEF CON mqtt.defcon.run:4433, correct credentials, working on iOS and on the 2.7.13 Android client) saw MQTT: connection rejected (check credentials) and an MQTT-proxy toggle that flicked green then straight back to grey.

Nothing was wrong with their credentials. Three separate defects stack up to produce that experience, and this PR fixes the app's share of them.

🐛 Transport failures were reported as credential rejections

MqttClient.connect wraps every connect failure — TCP timeout, TLS error, socket EOF, a broker that accepts the socket then stalls the CONNECT — as MqttException.ConnectionRejected carrying UNSPECIFIED_ERROR. MQTTRepositoryImpl treated any ConnectionRejected as unrecoverable: it closed the flow, permanently stopped the proxy, and MqttManagerImpl printed the credentials dialog. The Paho-era client retried these, which is why this only appeared from 2.7.14 onward (#5165).

The fatal stop is now gated on isCredentialRejection() — the CONNACK reason codes where retrying genuinely cannot help (BAD_USER_NAME_OR_PASSWORD, NOT_AUTHORIZED, BAD_AUTHENTICATION_METHOD, CLIENT_IDENTIFIER_NOT_VALID, BANNED). Everything else goes back into the existing exponential-backoff retry loop, and the user-facing message is phrased from the same classification instead of asserting a credentials problem.

This is the dominant failure mode fleet-wide, not just at DEF CON: Datadog RUM shows ~26k of these events across ~2,500 users on the 2.7.14 production build, with underlying messages like Connection timed out, Chain validation failed, and Not enough data available.

🐛 Empty-address configs connected anonymously and were refused

Firmware's PubSubConfig treats an empty address as "the public broker with its well-known credentials", substituting server and credentials together. The app substituted only the address and passed the stored (empty) credentials through, so the proxy connected anonymously and the public broker refused it with a real BAD_USER_NAME_OR_PASSWORD — a genuine rejection triggered by a config the firmware itself connects with happily. Empty-address configs do occur in the wild: lockdown-enabled firmware hands an unauthenticated client a zeroed MQTTConfig.

effectiveCredentials() now mirrors the firmware rule exactly.

🐛 The connection probe didn't mirror the live client

The library's probe defaults to keepAliveSeconds = 0, and some brokers refuse a keepalive-0 CONNECT — reporting it, misleadingly, as CLIENT_IDENTIFIER_NOT_VALID. The probe now sets the same keepalive as the live client. (Same class of bug as the transport/TLS mirroring already documented in this file.)

Upstream

The remaining cause is in the MQTT client library and is fixed by meshtastic/MQTTastic-Client-KMP#113: this broker silently closes MQTT 5 CONNECTs without any CONNACK, and the library's 3.1.1 fallback only fired on an explicit UNSUPPORTED_PROTOCOL_VERSION CONNACK — so the connection died with EOFException instead of retrying. iOS works because it speaks 3.1.1. That PR also stops classifying transport failures as ConnectionRejected at the source. Once it ships in 0.8.0 and Renovate bumps us, the reason-code gate here becomes belt-and-braces; it stays correct either way, so it should not be removed while we are on ≤0.7.0.

Testing Performed

Unit — 8 new tests in MQTTRepositoryImplTest (transport-failure retry, credential-rejection stop, the full reason-code classification table, and the five firmware-parity credential cases). Full local gate green: spotlessApply spotlessCheck detekt, plus :core:network:allTests :core:data:allTests (1,316 tests).

On-device, with the reporter's actual broker and real credentials from run.defcon.run — fdroid debug build on an emulator, connected to a simulated node, MQTT config set to mqtt.defcon.run:4433 + TLS, then "Test connection". Identical app code, only the library version differing:

MQTT client library	Result
Released 0.7.0	❌ Timed out after 5000 ms
meshtastic/MQTTastic-Client-KMP#113 build (0.7.1-SNAPSHOT via mavenLocal)	✅ Reachable. Broker accepted credentials.

Also verified on the public broker (ssl://mqtt.meshtastic.org:8883): meshdev succeeds; empty credentials reproduce the genuine BAD_USER_NAME_OR_PASSWORD this PR's second fix prevents.

Summary by CodeRabbit

• Bug Fixes
  • Improved MQTT connection reliability by retrying temporary connection rejections instead of stopping permanently.
  • Credential-related failures are now identified clearly and handled without unnecessary retries.
  • Fixed MQTT probe connections that could be rejected due to incorrect keep-alive settings.
  • Connections using the public broker now automatically use the appropriate default credentials when no address is configured.
  • Explicit broker configurations continue to use their saved credentials.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 60f44367-01d1-4d0f-a10f-edaf9acdf6ba

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

MQTT connection rejection handling now distinguishes credential failures from retryable failures. Client creation selects public broker credentials for missing addresses, and probe connections use the shared keepalive setting. Tests cover credential selection, rejection classification, retrying, and permanent failures.

Changes

MQTT connection handling

Layer / File(s)	Summary
Rejection classification and retry flow core/network/.../MQTTRepositoryImpl.kt, core/data/.../MqttManagerImpl.kt, core/network/.../MQTTRepositoryImplTest.kt	Credential-related ConnectionRejected reason codes stop retries, while other rejection reasons retry; manager messages distinguish credential failures from other rejections.
Credential selection and keepalive configuration core/network/.../MQTTRepositoryImpl.kt, core/data/.../MqttManagerImpl.kt, core/network/.../MQTTRepositoryImplTest.kt	Missing MQTT addresses use public broker credentials, explicit addresses preserve stored credentials, and probe connections use MQTT_KEEPALIVE_SECONDS.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MqttManagerImpl
  participant MQTTRepositoryImpl
  participant MQTTClient
  MqttManagerImpl->>MQTTRepositoryImpl: start proxy connection
  MQTTRepositoryImpl->>MQTTClient: connect
  MQTTClient-->>MQTTRepositoryImpl: ConnectionRejected
  MQTTRepositoryImpl->>MQTTRepositoryImpl: classify reason code
  MQTTRepositoryImpl-->>MqttManagerImpl: retry or permanent rejection

Loading

Suggested labels: bugfix

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: transport failures are no longer treated as credential rejections.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Sibling Call Sites And Presence Semantics	✅ Passed	No unfixed sibling call sites found: keepAliveSeconds, effectiveCredentials, and isCredentialRejection each have the expected updated usages only.
Tests Prove The Path, Not The End State	✅ Passed	The new tests assert call counts, retry behavior, and direct classification; none just echo fake state or depend on unstable collection ordering.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MqttManagerImpl.kt`:
- Around line 86-94: Update the MQTT error-message handling in MqttManagerImpl’s
serviceStateWriter flow to load user-facing strings from core:resources via
asynchronous getStringSuspend(...). Add or reuse resource templates for generic
rejection, credential rejection, and connection-lost messages, using a %1$s
placeholder for rejection details instead of hardcoded English text, then pass
the localized results to serviceStateWriter.

In
`@core/network/src/commonTest/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImplTest.kt`:
- Line 404: Replace runCatching with safeCatching in the backgroundScope.async
block collecting harness.repository.proxyMessageFlow, preserving observable
collection failures while allowing coroutine cancellation to propagate.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 7896e92b-654d-47c3-bef2-922c13b1cbaf

📥 Commits

Reviewing files that changed from the base of the PR and between dce2d89 and bdae765.

📒 Files selected for processing (3)

• core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/MqttManagerImpl.kt
• core/network/src/commonMain/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImpl.kt
• core/network/src/commonTest/kotlin/org/meshtastic/core/network/repository/MQTTRepositoryImplTest.kt