From 73098a44b6d958a54851334cbc02bf7b1f85df8a Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 26 Sep 2026 11:53:49 -0700 Subject: [PATCH] fix(mqtt): count connect failures from a flag set before the attempt The Starting-state gate raced the event task: client_state becomes Starting only after connect()/reconnect() returns, so a fast failure could deliver DISCONNECTED first and go uncounted, while a deliberate stop of an attempt in flight was counted as a failure. A per-slot flag is now raised before the call and cleared by onConnect, a failed start, or stopSlotClient(). Also stops zero-valued connect_failures/slots_breaker from creating an empty stats object. --- src/helpers/MQTTPayloadBuilder.cpp | 2 +- src/helpers/bridges/MQTTBridge.cpp | 13 ++++++++++--- src/helpers/bridges/MQTTBridge.h | 5 +++++ .../test_mqtt_payload_builder.cpp | 17 +++++++++++++++++ 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/helpers/MQTTPayloadBuilder.cpp b/src/helpers/MQTTPayloadBuilder.cpp index 9811b98f..8cbb03f4 100644 --- a/src/helpers/MQTTPayloadBuilder.cpp +++ b/src/helpers/MQTTPayloadBuilder.cpp @@ -65,7 +65,7 @@ int MQTTPayloadBuilder::buildStatusMessage( const bool has_conn_health = conn_health.slots_up >= 0 || conn_health.slots_total >= 0 || conn_health.worst_outage_secs >= 0 || conn_health.heap_largest >= 0 || - conn_health.connect_failures >= 0 || conn_health.slots_breaker >= 0; + conn_health.connect_failures > 0 || conn_health.slots_breaker > 0; if (battery_mv >= 0 || uptime_secs >= 0 || errors >= 0 || queue_len >= 0 || noise_floor > -999 || tx_air_secs >= 0 || rx_air_secs >= 0 || recv_errors >= 0 || diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index bcff6917..134b12e7 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -772,6 +772,7 @@ MQTTBridge::MQTTBridge(NodePrefs *prefs, MQTTPrefs *obs, mesh::PacketManager *mg _slot_reconfigure_pending[i] = false; _slot_force_jwt_mint[i] = false; _status_publish_pending[i] = false; + _slot_attempt_pending[i] = false; } // Reset CLI-requested forced NTP sync handshake (bridge object is reused across restarts) @@ -1730,6 +1731,7 @@ bool MQTTBridge::ensureSlotClient(int index) { return; } MQTT_DEBUG_PRINTLN("MQTT%d connected", index + 1); + _slot_attempt_pending[index] = false; _slots[index].client_state = ClientState::Connected; _slots[index].connected = true; _slot_force_jwt_mint[index] = false; @@ -1763,9 +1765,10 @@ bool MQTTBridge::ensureSlotClient(int index) { }); slot.client->onDisconnect([this, index](bool sessionPresent) { MQTT_DEBUG_PRINTLN("MQTT%d disconnected", index + 1); - // An event while an attempt is in flight ended one that never reached onConnect. - // Stopped/quarantined late events and soft-disconnects are not Starting. - if (mqttClientStateHasAttemptInFlight(_slots[index].client_state)) { + // Ended an attempt that never reached onConnect. A deliberate stop clears the flag + // first, so cancelling an attempt is not counted as a failure. + if (_slot_attempt_pending[index]) { + _slot_attempt_pending[index] = false; _slots[index].connect_failures++; } // Only a live client's disconnect is news. One arriving for a client we @@ -2411,6 +2414,7 @@ void MQTTBridge::closeLiveClientForLinkTransition(int index) { // the rest of the boot. void MQTTBridge::stopSlotClient(int index) { MQTTSlot& slot = _slots[index]; + _slot_attempt_pending[index] = false; const esp_err_t r = slot.client->disconnect(); // What the SDK's results actually mean here (mqtt_client.h documents @@ -2505,6 +2509,7 @@ esp_err_t MQTTBridge::reconnectSlotClient(int index) { } esp_err_t r; + _slot_attempt_pending[index] = true; if (!slot.client->isStarted()) { MQTT_DEBUG_PRINTLN("MQTT%d start (client was stopped)", index + 1); r = slot.client->connect(); @@ -2517,6 +2522,8 @@ esp_err_t MQTTBridge::reconnectSlotClient(int index) { // The attempt carries whatever credential is configured right now, so this // is the point at which a freshly minted token becomes the one in use. slot.applied_token_expires_at = slot.token_expires_at; + } else { + _slot_attempt_pending[index] = false; } return r; } diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index bbf04472..87b75b05 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -286,6 +286,11 @@ private: // _slot_reconfigure_pending; a single-byte volatile store/load is atomic. volatile bool _status_publish_pending[RUNTIME_MQTT_SLOTS]; + // Set by the bridge task before connect()/reconnect(), so a failure the event task + // delivers before that call returns is still seen; cleared by the attempt's outcome + // or a deliberate stop. A disconnect that finds it set is a connect failure. + volatile bool _slot_attempt_pending[RUNTIME_MQTT_SLOTS]; + // CLI-requested forced NTP sync, marshalled onto the MQTT task (Core 0). // All NTP I/O must run on Core 0; the CLI thread // (Core 1) sets _ntp_force_requested and blocks in requestForcedNtpSync() diff --git a/test/test_mqtt_payload_builder/test_mqtt_payload_builder.cpp b/test/test_mqtt_payload_builder/test_mqtt_payload_builder.cpp index f539ee8d..e8ae51c8 100644 --- a/test/test_mqtt_payload_builder/test_mqtt_payload_builder.cpp +++ b/test/test_mqtt_payload_builder/test_mqtt_payload_builder.cpp @@ -158,6 +158,23 @@ TEST(MQTTPayloadBuilder, StatusReportsConnectFailuresAndBreakerOnlyWhenNonZero) EXPECT_FALSE(healthy_stats["mqtt_slots_breaker"].is()); } +// Zero counters with nothing else supplied must not create an empty stats object. +TEST(MQTTPayloadBuilder, StatusOmitsStatsWhenOnlyZeroCountersSupplied) { + JsonDocument scratch; + char buffer[768]; + MQTTConnHealth health; + health.connect_failures = 0; + health.slots_breaker = 0; + + ASSERT_GT(MQTTPayloadBuilder::buildStatusMessage( + scratch, "node", "id", "model", "firmware", "radio", "client", "online", + kTimestamp, buffer, sizeof(buffer), -1, -1, -1, -1, -999, + -1, -1, -1, -1, -1, -1, nullptr, health), 0); + JsonDocument parsed; + ASSERT_FALSE(deserializeJson(parsed, buffer)); + EXPECT_FALSE(parsed["stats"].is()); +} + // The outage key doubles as the "something is down" flag, so its absence has to be // meaningful: a fully healthy board must not emit it. TEST(MQTTPayloadBuilder, StatusOmitsOutageKeyWhenEverySlotIsUp) {