fix: MQTT settings silently fail to persist when broker is unreachable#9934
Conversation
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 meshtastic#9107
|
|
@rcatal01, Welcome to Meshtastic!Thanks for opening your first pull request. We really appreciate it. We discuss work as a team in discord, please join us in the #firmware channel. Welcome to the team 😄 |
|
This may have a positive impact on #9876 |
There was a problem hiding this comment.
Pull request overview
Fixes a configuration persistence bug where MQTT settings were not saved if the broker was unreachable during validation, by making MQTT::isValidConfig() validate configuration correctness only (not connectivity). This aligns validation with the intended behavior that the MQTT module’s reconnect loop handles connectivity after settings are persisted.
Changes:
- Removed broker connectivity attempts from
MQTT::isValidConfig()so unreachable brokers no longer block saving MQTT settings. - Updated MQTT unit tests to reflect the new validation semantics (connectivity not tested) and renamed the relevant test to match behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/mqtt/MQTT.cpp |
Stops treating broker connectivity as a config validity requirement; keeps TLS support and default-server port validation. |
test/test_mqtt/MQTT.cpp |
Updates/renames unit tests to assert config validity without requiring broker connectivity. |
You can also share your feedback on Copilot code review. Take the survey.
| 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); | ||
| if (isConnectedToNetwork()) { | ||
| return connectPubSub(parsed, *pubSub, (client != nullptr) ? *client : *clientConnection); | ||
| } | ||
| // Note: connectivity is intentionally NOT validated here. | ||
| // Settings must be saved even when the network is temporarily unavailable. | ||
| // The MQTT module's reconnect loop will establish the connection when possible. |
|
@rcatal01 I think I'm in favor of this change because its currently confusing users, but I'm not sure silently failing a different way is better. I think perhaps we should throw a client notification up to the client letting them know that we could not reach the configured MQTT broker and to double check the settings. |
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.
|
@thebentern Good point — silently succeeding isn't much better than silently failing. I've pushed an update that:
This way users get clear feedback without losing their settings. |
There was a problem hiding this comment.
Pull request overview
Fixes MQTT configuration persistence when the broker is unreachable by changing how MQTT config validation behaves, so settings aren’t rejected (and therefore not saved) due to transient connectivity failures.
Changes:
- Adjusts
MQTT::isValidConfig()to avoid failing validation when an MQTT connection attempt fails, and emits a warning instead. - Updates MQTT unit tests to expect
isValidConfig()to returntrueeven when the broker is unreachable. - Renames/updates related test cases and comments to reflect the new validation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/mqtt/MQTT.cpp |
Changes MQTT config validation to no longer fail on unreachable broker; adds warning/notification path. |
test/test_mqtt/MQTT.cpp |
Updates unit tests and naming to align with new isValidConfig() behavior. |
You can also share your feedback on Copilot code review. Take the survey.
| std::unique_ptr<PubSubClient> pubSub(new PubSubClient); | ||
| if (!connectPubSub(parsed, *pubSub, (client != nullptr) ? *client : *clientConnection)) { | ||
| const char *warning = "MQTT settings saved, but could not reach the MQTT server. Please verify the server address and credentials."; |
| } | ||
| std::unique_ptr<PubSubClient> pubSub(new PubSubClient); | ||
| if (!connectPubSub(parsed, *pubSub, (client != nullptr) ? *client : *clientConnection)) { | ||
| const char *warning = "MQTT settings saved, but could not reach the MQTT server. Please verify the server address and credentials."; |
| meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed(); | ||
| 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'; |
| } | ||
|
|
||
| // Empty 'enabled' configuration is valid. | ||
| // Empty 'enabled' configuration is valid. Connectivity is verified but does not block saving. |
| // isValidConfig verifies connectivity but always returns true (settings always save). | ||
| void test_configCustomHostAndPort(void) |
- 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
|
Pushed a follow-up commit addressing the Copilot review feedback:
|
There was a problem hiding this comment.
Pull request overview
Fixes MQTT settings not persisting when the broker/network is unreachable by ensuring MQTT::isValidConfig() no longer fails validation solely due to connection failures, allowing AdminModule to persist the new configuration and letting the MQTT reconnect loop handle connectivity later.
Changes:
- Adjusted
MQTT::isValidConfig()to not reject configs when broker connectivity checks fail (adds warning/log/notification instead). - Updated MQTT unit tests to reflect the new “unreachable broker is still valid” behavior and renamed the affected test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/mqtt/MQTT.cpp | Changes config validation behavior so unreachable broker doesn’t prevent saving; adds a warning path. |
| test/test_mqtt/MQTT.cpp | Updates/renames unit tests to match the revised validation behavior. |
Comments suppressed due to low confidence (1)
test/test_mqtt/MQTT.cpp:831
- These unit tests still assert that
isValidConfig()establishes a connection (client.connected_), which bakes connectivity side effects into config validation and will make future refactors (e.g., truly removing connectivity checks fromisValidConfig) harder. Since the new intended behavior is “settings always save”, consider narrowing these tests to assert parsing/normalization (host/port) and return value, and reserve connection assertions for reconnect-loop tests.
// Empty 'enabled' configuration is valid. Connectivity is checked when network is available but does not block saving.
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_);
}
You can also share your feedback on Copilot code review. Take the survey.
| // Attempt to verify broker connectivity, but do NOT block saving settings if unreachable. | ||
| // The MQTT module's reconnect loop will establish the connection when possible. | ||
| if (isConnectedToNetwork()) { |
| if (isConnectedToNetwork()) { | ||
| return connectPubSub(parsed, *pubSub, (client != nullptr) ? *client : *clientConnection); | ||
| std::unique_ptr<MQTTClient> clientConnection; | ||
| if (config.tls_enabled) { | ||
| #if MQTT_SUPPORTS_TLS | ||
| MQTTClientTLS *tlsClient = new MQTTClientTLS; | ||
| clientConnection.reset(tlsClient); | ||
| tlsClient->setInsecure(); | ||
| #endif | ||
| } else { | ||
| clientConnection.reset(new MQTTClient); | ||
| } | ||
| std::unique_ptr<PubSubClient> pubSub(new PubSubClient); | ||
| if (!connectPubSub(parsed, *pubSub, (client != nullptr) ? *client : *clientConnection)) { | ||
| const char *warning = "Could not reach the MQTT server. Settings will be saved, but please verify the server address and credentials."; |
| std::unique_ptr<MQTTClient> clientConnection; | ||
| if (config.tls_enabled) { | ||
| #if MQTT_SUPPORTS_TLS | ||
| MQTTClientTLS *tlsClient = new MQTTClientTLS; | ||
| clientConnection.reset(tlsClient); | ||
| tlsClient->setInsecure(); | ||
| #endif | ||
| } else { | ||
| clientConnection.reset(new MQTTClient); | ||
| } | ||
| std::unique_ptr<PubSubClient> pubSub(new PubSubClient); | ||
| if (!connectPubSub(parsed, *pubSub, (client != nullptr) ? *client : *clientConnection)) { |
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.
|
After reviewing the Copilot feedback on the advisory connectivity check, I've removed it entirely and returned to the simpler approach from the first commit. The issue: The cleaner solution: This is the version that was tested on hardware (Heltec Wireless Tracker) and confirmed working: MQTT settings now persist regardless of broker reachability. |
|
@rcatal01 I'm not in favor of this as-is. Removing the connectivity check diagnostic with client notification strips that whole process of any value it had before and people will end up being confused the other direction when their mqtt connection doesn't work, with no feedback. |
Xaositek
left a comment
There was a problem hiding this comment.
Removing my approval at this time - this PR has changed quite a bit
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.
|
@thebentern @Xaositek Understood — I agree users need connectivity feedback. The issue Copilot raised is that Would a lightweight TCP-only connection test be acceptable? The idea: use a standalone Want to confirm this direction before I push more code. |
I still believe there are bugs in the code because it will claim to not be able to reach a server that it was connecting to without issue just fine for days until I disable MQTT, and when re-enabling it with the same settings it will log that the server is unreachable and refuse to enable it. Network captures at the time showed no attempt to connect so I think it's logging does not provide enough information about the failure to determine what is actually failing. The original pubsub codebase has not been updated since 2020, and was absolutely full of unresolved git issues. They are using a fork maintained by one man which received some updates but has not been updated in a year. It may simply be that the pubsub codebase is buggy and we are victim to using that implementation. 🤷 Looks like hmueller01 is the only person actively maintaining a fork at this point, with many updates. |
to be fair, the 'feedback' it provides now is not helpful. It provides no details about the failure, simply stating that it could not connect. Was it a failure because of temporarily DNS resolution? Was it actually unable to reach the target? did it take too long to get a response from the target? did it actually try to connect at all? (I've seen packet captures where it did not even try to connect over the fire) or maybe it received an unexpected reply from the MQTT server. No clues are given. We don't even know if it would self-recover after the first failure because it never gives the opportunity to try. On another note, anyone who might want to pre-configure MQTT ahead of time for a device so that it's already ready to transmit when deployed is actively blocked from doing so because the MQTT server simply has to be reachable at the time it is turned on. 🤷 IMHO it should turn MQTT on or off, and not care if it can connect or not. It will already log each time it cannot reach the MQTT server so it's not as if the user has no way to know if there is a problem. |
There was a problem hiding this comment.
Pull request overview
Fixes an MQTT configuration persistence bug where AdminModule would refuse to save settings if MQTT::isValidConfig() failed due to broker/network reachability, causing silent reversion when the broker was unreachable.
Changes:
- Removed the blocking “must-connect” behavior from
MQTT::isValidConfig()so config validity is no longer tied to broker reachability. - Added a non-blocking, best-effort TCP reachability warning (without mutating MQTT module connection state).
- Updated MQTT unit tests to reflect that unreachable brokers no longer invalidate configuration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/mqtt/MQTT.cpp |
Stops treating broker connectivity as a validation failure; adds a warning-only reachability check. |
test/test_mqtt/MQTT.cpp |
Updates unit tests to assert configs remain valid even when the broker is unreachable. |
You can also share your feedback on Copilot code review. Take the survey.
| bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config, MQTTClient *client) | ||
| { | ||
| const PubSubConfig parsed(config); | ||
|
|
||
| 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)) { |
#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>
meshtastic#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 meshtastic#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>
Summary
isValidConfig()was testing broker connectivity viaconnectPubSub()during config validationfalse, causingAdminModule::handleSetModuleConfig()to skip saving settings entirely — silentlyisValidConfig(), which now only validates configuration correctness (TLS support check, default server port check)Fixes #9107
Root Cause
In
MQTT::isValidConfig()(MQTT.cpp line 668-670),connectPubSub()was called to test broker connectivity as part of validation. When this failed,isValidConfig()returnedfalse, which causedAdminModule(line 924-926) toreturn falsebefore reaching line 930 wheremoduleConfig.mqtt = c.payload_variant.mqttactually persists the settings.The result: settings appeared to save but silently reverted. No error was shown to the user.
Changes
src/mqtt/MQTT.cpp: RemovedconnectPubSub()call and associatedMQTTClient/PubSubClientobject creation fromisValidConfig(). The function now only validates config correctness, not connectivity.test/test_mqtt/MQTT.cpp: Updated 4 unit tests to reflect thatisValidConfig()no longer tests connectivity.test_configWithConnectionFailurerenamed totest_configWithUnreachableServerIsStillValidand now assertstrue.Test plan