From 7d0a602b20cd61419005566bce5b5da7cb644160 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 26 Sep 2026 13:54:43 -0700 Subject: [PATCH] fix(mqtt): exclude unready slots from slot health and keep cf monotonic A slot waiting on a token/IATA/credential stays enabled but never attempts a connection, so it held mqtt_slots_up below mqtt_slots_total (and down= at 1) permanently -- a standing false alert on a node that is merely unconfigured. slots_total and down= now count only ready slots, matching the CLI's "wait". mqtt_connect_failures summed only enabled slots, so disabling a failing slot made it drop. It now sums every slot, monotonic within a boot. cf: moves after the error tail in mqttN.diag so a clamped 160-byte reply loses the count rather than the refusal/TLS detail. A failed start now clears the attempt flag instead of restoring a possibly stale one. Outage comments corrected: the clock starts at a slot's first DISCONNECTED, not first connect. --- src/helpers/MQTTPayloadBuilder.cpp | 2 +- src/helpers/MQTTPayloadBuilder.h | 10 ++++----- src/helpers/bridges/MQTTBridge.cpp | 36 +++++++++++++++++------------- 3 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/helpers/MQTTPayloadBuilder.cpp b/src/helpers/MQTTPayloadBuilder.cpp index 8cbb03f4..56e2a259 100644 --- a/src/helpers/MQTTPayloadBuilder.cpp +++ b/src/helpers/MQTTPayloadBuilder.cpp @@ -88,7 +88,7 @@ int MQTTPayloadBuilder::buildStatusMessage( if (conn_health.slots_total >= 0) stats["mqtt_slots_total"] = conn_health.slots_total; // Emitted only during an outage of known duration, so the steady-state payload // does not grow. Presence implies a slot is down, but absence does NOT imply - // health: a slot that has never connected since boot has no outage start time. + // health: a slot has no outage start time until its first attempt ends. // `mqtt_slots_up < mqtt_slots_total` is the signal to alert on; this is detail. if (conn_health.worst_outage_secs >= 0) { stats["mqtt_outage_secs"] = conn_health.worst_outage_secs; diff --git a/src/helpers/MQTTPayloadBuilder.h b/src/helpers/MQTTPayloadBuilder.h index 7f0cd85b..fa46e741 100644 --- a/src/helpers/MQTTPayloadBuilder.h +++ b/src/helpers/MQTTPayloadBuilder.h @@ -21,12 +21,12 @@ // per-attempt reading. struct MQTTConnHealth { int slots_up = -1; // -1 = not supplied, field omitted - int slots_total = -1; // enabled slots, whether or not they connect - int worst_outage_secs = -1; // longest current outage; -1 = nothing is down + int slots_total = -1; // enabled, fully configured slots, connected or not + int worst_outage_secs = -1; // longest timed outage; -1 = none timed (see below) int heap_largest = -1; // largest free internal block, bytes - // Attempts that ended without reaching onConnect, summed across slots. A slot - // stuck retrying moves this while its publish counters stay frozen, which is - // what separates "actively failing" from "idle and quiet". + // Attempts that ended without reaching onConnect, summed across all slots since + // boot. A slot stuck retrying moves this while its publish counters stay frozen, + // which is what separates "actively failing" from "idle and quiet". int connect_failures = -1; // Slots parked by the circuit breaker. Materially worse than a retrying slot: // the breaker only probes every 30 minutes, so recovery is far slower. diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 8e5eec70..8c24fbf2 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -373,13 +373,13 @@ void MQTTBridge::formatMqttStatsReply(char* buf, size_t bufsize) { if (b->_filtered_packets > 0) { replyAppendf(buf, bufsize, &pos, " filt=%lu", b->_filtered_packets); } - // down=: enabled slots that are not connected. Without this the reply cannot + // down=: ready slots that are not connected. Without this the reply cannot // show an outage at all — sN= counts publishes, and a slot that never connects // never publishes, so its ok freezes and its err stays 0 for the whole outage. // Omitted while zero, like filt=, so a healthy node's reply keeps its length. int down = 0; for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) { - if (b->_slots[i].enabled && !b->_slots[i].connected) down++; + if (b->_slots[i].enabled && !b->_slots[i].connected && b->isSlotReady(i)) down++; } if (down > 0) { replyAppendf(buf, bufsize, &pos, " down=%d", down); @@ -589,13 +589,6 @@ void MQTTBridge::formatSlotDiagReply(char* buf, size_t bufsize, int slot_index) replyAppendf(buf, bufsize, &pos, ", first_disc:%lus", first_disc_age_sec); } } - // cf: attempts that never reached onConnect, plus starts that failed outright. A - // slot retrying into a heap that cannot satisfy the TLS allocation moves only this; - // the publish counters cannot move at all while the slot is down. - const uint32_t cf = slot.connect_failures + slot.start_failures; - if (cf > 0) { - replyAppendf(buf, bufsize, &pos, ", cf:%lu", (unsigned long)cf); - } // Connected with no errors: nothing more to say about the connection. if (slot.connected && slot.last_error_time == 0) { @@ -642,6 +635,14 @@ void MQTTBridge::formatSlotDiagReply(char* buf, size_t bufsize, int slot_index) } else if (!slot.connected) { replyAppendf(buf, bufsize, &pos, ", no error info"); } + // cf: attempts that never reached onConnect, plus starts that failed outright. A + // slot retrying into a heap that cannot satisfy the TLS allocation moves only this; + // the publish counters cannot move at all while the slot is down. + // After the error tail so a clamped reply loses this count, not the error detail. + const uint32_t cf = slot.connect_failures + slot.start_failures; + if (cf > 0) { + replyAppendf(buf, bufsize, &pos, ", cf:%lu", (unsigned long)cf); + } // Appended last so it never displaces connection diagnostics. replyAppendf // clamps rather than overflows, but a clipped type list is worse than no @@ -2528,7 +2529,8 @@ esp_err_t MQTTBridge::reconnectSlotClient(int index) { // 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] = was_pending; + // A failed start leaves nothing in flight; a rejected reconnect leaves the prior one. + _slot_attempt_pending[index] = starting ? false : was_pending; // A failed start (buffer/task allocation, config) means no attempt could begin. // reconnect() on a started client returns ESP_FAIL while one is already in // progress, which is not a failure (seen on hardware after a reconfigure). @@ -3206,19 +3208,21 @@ MQTTConnHealth MQTTBridge::collectConnHealth() const { for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) { const MQTTSlot& slot = _slots[i]; - if (!slot.enabled) continue; // "none" slots are not an outage, they are unconfigured - health.slots_total++; + // Summed before the skip so disabling a slot never makes the total go backwards. connect_failures += slot.connect_failures + slot.start_failures; + // Unconfigured slots and slots waiting on a token/IATA/credential are not outages. + if (!slot.enabled || !isSlotReady(i)) continue; + health.slots_total++; if (slot.circuit_breaker_tripped) breaker_slots++; if (slot.connected) { health.slots_up++; continue; } - // A slot that has never connected since boot has no outage start time. It still - // counts against slots_up, which is why that pair — not this duration — is the - // signal to alert on. + // The outage clock starts at a slot's first DISCONNECTED, so a slot that has not yet + // finished its first attempt has no start time. It still counts against slots_up, + // which is why that pair — not this duration — is the signal to alert on. if (slot.current_outage_started_ms != 0) { - const unsigned long outage_ms = now - slot.current_outage_started_ms; // wrap-safe + const unsigned long outage_ms = now - slot.current_outage_started_ms; // wrap-safe under 49.7 days if (!any_outage_timed || outage_ms > worst_outage_ms) { worst_outage_ms = outage_ms; any_outage_timed = true;