fix: MQTT settings silently fail to persist when broker is unreachable (#9934)

* fix: MQTT settings silently fail to persist when broker is unreachable

isValidConfig() was testing broker connectivity via connectPubSub() as
part of config validation. When the broker was unreachable (network not
ready, DNS failure, server down), the function returned false, causing
AdminModule to skip saving settings entirely — silently.

This removes the connectivity test from isValidConfig(), which now only
validates configuration correctness (TLS support, default server port).
Connectivity is handled by the MQTT module's existing reconnect loop.

Fixes #9107

* Add client warning notification when MQTT broker is unreachable

Per maintainer feedback: instead of silently saving when the broker
can't be reached, send a WARNING notification to the client saying
"MQTT settings saved, but could not reach the MQTT server."

Settings still always persist regardless of connectivity — the core
fix from the previous commit is preserved. The notification is purely
advisory so users know to double-check their server address and
credentials if the connection test fails.

When the network is not available at all, the connectivity check is
skipped entirely with a log message.

* Address Copilot review feedback

- Fix warning message wording: "Settings will be saved" instead of
  "Settings saved" (notification fires before AdminModule persists)
- Add null check on clientNotificationPool.allocZeroed() to prevent
  crash if pool is exhausted (matches AdminModule::sendWarning pattern)
- Fix test comments to accurately describe conditional connectivity
  check behavior and IS_RUNNING_TESTS compile-out

* Remove connectivity check from isValidConfig entirely

Reverts the advisory connectivity check added in the previous commit.
While the intent was to warn users about unreachable brokers,
connectPubSub() mutates the isConnected state of the running MQTT
module and performs synchronous network operations that can block
the config-save path.

The cleanest approach: isValidConfig() validates config correctness
only (TLS support, default server port). The MQTT reconnect loop
handles connectivity after settings are persisted and the device
reboots. If the broker is unreachable, the user will see it in the
MQTT connection status — no special notification needed.

This returns to the simpler design from the first commit, which was
tested on hardware and confirmed working.

* Use lightweight TCP check instead of connectPubSub for validation

Per maintainer feedback: users need connectivity feedback, but
connectPubSub() mutates the module's isConnected state.

This uses a standalone MQTTClient TCP connection test that:
- Checks if the server IP/port is reachable
- Sends a WARNING notification if unreachable
- Does NOT establish an MQTT session or mutate any module state
- Does NOT block saving — isValidConfig always returns true

The TCP test client is created locally, used, and destroyed within
the function scope. No side effects on the running MQTT module.

---------

Co-authored-by: Ben Meadors <benmmeadors@gmail.com>

Author: rcatal01

src/mqtt/MQTT.cpp

@@ -651,22 +651,34 @@ bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config, MQTTC
 
     if (config.enabled && !config.proxy_to_client_enabled) {
 #if HAS_NETWORKING
-        std::unique_ptr<MQTTClient> clientConnection;
         if (config.tls_enabled) {
-#if MQTT_SUPPORTS_TLS
-            MQTTClientTLS *tlsClient = new MQTTClientTLS;
-            clientConnection.reset(tlsClient);
-            tlsClient->setInsecure();
-#else
+#if !MQTT_SUPPORTS_TLS
             LOG_ERROR("Invalid MQTT config: tls_enabled is not supported on this node");
             return false;
 #endif
-        } else {
-            clientConnection.reset(new MQTTClient);
         }
-        std::unique_ptr<PubSubClient> pubSub(new PubSubClient);
+        // Perform a lightweight TCP connectivity check without using connectPubSub(),
+        // which mutates the module's isConnected state. This only checks if the server
+        // is reachable — it does not establish an MQTT session.
+        // Settings are always saved regardless of the result.
         if (isConnectedToNetwork()) {
-            return connectPubSub(parsed, *pubSub, (client != nullptr) ? *client : *clientConnection);
+            MQTTClient testClient;
+            if (!testClient.connect(parsed.serverAddr.c_str(), parsed.serverPort)) {
+                const char *warning = "Could not reach the MQTT server. Settings will be saved, but please verify the server "
+                                      "address and credentials.";
+                LOG_WARN(warning);
+#if !IS_RUNNING_TESTS
+                meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
+                if (cn) {
+                    cn->level = meshtastic_LogRecord_Level_WARNING;
+                    cn->time = getValidTime(RTCQualityFromNet);
+                    strncpy(cn->message, warning, sizeof(cn->message) - 1);
+                    cn->message[sizeof(cn->message) - 1] = '\0';
+                    service->sendClientNotification(cn);
+                }
+#endif
+            }
+            testClient.stop();
         }
 #else
         const char *warning = "Invalid MQTT config: proxy_to_client_enabled must be enabled on nodes that do not have a network";

test/test_mqtt/MQTT.cpp

@@ -818,16 +818,13 @@ void test_configEmptyIsValid(void)
     TEST_ASSERT_TRUE(MQTT::isValidConfig(config));
 }
 
-// Empty 'enabled' configuration is valid.
+// Empty 'enabled' configuration is valid. A lightweight TCP check may be performed
+// but does not affect the result.
 void test_configEnabledEmptyIsValid(void)
 {
     meshtastic_ModuleConfig_MQTTConfig config = {.enabled = true};
-    MockPubSubServer client;
 
-    TEST_ASSERT_TRUE(MQTTUnitTest::isValidConfig(config, &client));
-    TEST_ASSERT_TRUE(client.connected_);
-    TEST_ASSERT_EQUAL_STRING(default_mqtt_address, client.host_.c_str());
-    TEST_ASSERT_EQUAL(1883, client.port_);
+    TEST_ASSERT_TRUE(MQTT::isValidConfig(config));
 }
 
 // Configuration with the default server is valid.
@@ -846,38 +843,32 @@ void test_configWithDefaultServerAndInvalidPort(void)
     TEST_ASSERT_FALSE(MQTT::isValidConfig(config));
 }
 
-// isValidConfig connects to a custom host and port.
+// Custom host and port is valid. TCP reachability is checked but does not block saving.
 void test_configCustomHostAndPort(void)
 {
     meshtastic_ModuleConfig_MQTTConfig config = {.enabled = true, .address = "server:1234"};
-    MockPubSubServer client;
 
-    TEST_ASSERT_TRUE(MQTTUnitTest::isValidConfig(config, &client));
-    TEST_ASSERT_TRUE(client.connected_);
-    TEST_ASSERT_EQUAL_STRING("server", client.host_.c_str());
-    TEST_ASSERT_EQUAL(1234, client.port_);
+    TEST_ASSERT_TRUE(MQTT::isValidConfig(config));
 }
 
-// isValidConfig returns false if a connection cannot be established.
-void test_configWithConnectionFailure(void)
+// An unreachable server is still a valid config — settings always save.
+// A warning notification is sent in non-test builds, but isValidConfig returns true.
+void test_configWithUnreachableServerIsStillValid(void)
 {
     meshtastic_ModuleConfig_MQTTConfig config = {.enabled = true, .address = "server"};
-    MockPubSubServer client;
-    client.refuseConnection_ = true;
 
-    TEST_ASSERT_FALSE(MQTTUnitTest::isValidConfig(config, &client));
+    TEST_ASSERT_TRUE(MQTT::isValidConfig(config));
 }
 
 // isValidConfig returns true when tls_enabled is supported, or false otherwise.
 void test_configWithTLSEnabled(void)
 {
     meshtastic_ModuleConfig_MQTTConfig config = {.enabled = true, .address = "server", .tls_enabled = true};
-    MockPubSubServer client;
 
 #if MQTT_SUPPORTS_TLS
-    TEST_ASSERT_TRUE(MQTTUnitTest::isValidConfig(config, &client));
+    TEST_ASSERT_TRUE(MQTT::isValidConfig(config));
 #else
-    TEST_ASSERT_FALSE(MQTTUnitTest::isValidConfig(config, &client));
+    TEST_ASSERT_FALSE(MQTT::isValidConfig(config));
 #endif
 }
 
@@ -927,7 +918,7 @@ void setup()
     RUN_TEST(test_configWithDefaultServer);
     RUN_TEST(test_configWithDefaultServerAndInvalidPort);
     RUN_TEST(test_configCustomHostAndPort);
-    RUN_TEST(test_configWithConnectionFailure);
+    RUN_TEST(test_configWithUnreachableServerIsStillValid);
     RUN_TEST(test_configWithTLSEnabled);
     exit(UNITY_END());
 }