From 99a4b6fad0ff94817b68354e29bd56db697fefa1 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 26 Sep 2026 12:25:53 -0700 Subject: [PATCH] fix(mqtt): count failed starts and ignore bounced sessions in cf connect()/reconnect() returning an error started no attempt, so no event arrived and the failure went uncounted -- the low-heap case the counter exists for. Counted now in a bridge-task-only start_failures, summed into cf: and mqtt_connect_failures. A renewal or corrected-clock bounce whose softDisconnect() times out gets its old session's DISCONNECTED after the new attempt raised the flag. That session is still marked connected until the handler runs, so the gate now also requires !connected. An ignored late CONNECTED clears the flag too. cf: no longer depends on dc being non-zero. Comments corrected for the PSRAM-fallback stack cost and heap_largest's sampling point; the worst-case test now models 6 slots. --- src/helpers/MQTTPayloadBuilder.h | 2 ++ src/helpers/bridges/MQTTBridge.cpp | 22 +++++++++++-------- src/helpers/bridges/MQTTBridge.h | 11 +++++++--- .../test_mqtt_payload_builder.cpp | 4 ++-- 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/helpers/MQTTPayloadBuilder.h b/src/helpers/MQTTPayloadBuilder.h index 42dd3ff86..7f0cd85b0 100644 --- a/src/helpers/MQTTPayloadBuilder.h +++ b/src/helpers/MQTTPayloadBuilder.h @@ -17,6 +17,8 @@ // heap alone does not — an 85 KB-free board sat unable to connect for hours. // A low value with all slots up is normal, not a fault: established sessions // already hold their buffers. It means only that the next slot to ask will fail. +// Sampled at status-publish time, not at the attempt, so it is a trend, not a +// 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 diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 134b12e7c..2bee5bb25 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -584,17 +584,18 @@ void MQTTBridge::formatSlotDiagReply(char* buf, size_t bufsize, int slot_index) replyAppendf(buf, bufsize, &pos, "> mqtt%d: %s", slot_index + 1, state); if (slot.disconnect_count > 0) { replyAppendf(buf, bufsize, &pos, ", dc:%lu", (unsigned long)slot.disconnect_count); - // cf: attempts that never reached onConnect. A slot retrying into a heap that - // cannot satisfy the TLS allocation moves only this counter — dc rises with it, - // but the publish counters cannot move at all while the slot is down. - if (slot.connect_failures > 0) { - replyAppendf(buf, bufsize, &pos, ", cf:%lu", (unsigned long)slot.connect_failures); - } if (slot.first_disconnect_time > 0) { unsigned long first_disc_age_sec = (millis() - slot.first_disconnect_time) / 1000; 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) { @@ -1728,6 +1729,7 @@ bool MQTTBridge::ensureSlotClient(int index) { MQTT_DEBUG_PRINTLN("MQTT%d ignoring late CONNECTED (state=%s, gen=%lu, enabled=%d)", index + 1, clientStateName(st), (unsigned long)_slots[index].generation, (int)_slots[index].enabled); + _slot_attempt_pending[index] = false; // still the end of that attempt return; } MQTT_DEBUG_PRINTLN("MQTT%d connected", index + 1); @@ -1766,8 +1768,9 @@ bool MQTTBridge::ensureSlotClient(int index) { slot.client->onDisconnect([this, index](bool sessionPresent) { MQTT_DEBUG_PRINTLN("MQTT%d disconnected", index + 1); // 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]) { + // first; a bounced live session is still marked connected until this handler runs, + // so its late DISCONNECTED after a timed-out softDisconnect() is not counted either. + if (_slot_attempt_pending[index] && !_slots[index].connected) { _slot_attempt_pending[index] = false; _slots[index].connect_failures++; } @@ -2524,6 +2527,7 @@ esp_err_t MQTTBridge::reconnectSlotClient(int index) { slot.applied_token_expires_at = slot.token_expires_at; } else { _slot_attempt_pending[index] = false; + slot.start_failures++; } return r; } @@ -3199,7 +3203,7 @@ MQTTConnHealth MQTTBridge::collectConnHealth() const { const MQTTSlot& slot = _slots[i]; if (!slot.enabled) continue; // "none" slots are not an outage, they are unconfigured health.slots_total++; - connect_failures += slot.connect_failures; + connect_failures += slot.connect_failures + slot.start_failures; if (slot.circuit_breaker_tripped) breaker_slots++; if (slot.connected) { health.slots_up++; diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index 87b75b050..87e585b10 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -187,6 +187,10 @@ private: // disconnect_count (which also counts dropped live sessions) and from the // publish error counter, which cannot move at all while a slot is down. uint32_t connect_failures; + // connect()/reconnect() calls that returned an error, so no attempt started and + // no event will arrive. Written only by the bridge task; reported summed with + // connect_failures. + uint32_t start_failures; unsigned long first_disconnect_time; // millis() of first disconnect after boot // Current-outage timer (used by AlertReporter to fire faults after a sustained @@ -288,7 +292,7 @@ private: // 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. + // (including an ignored late CONNECTED) or a deliberate stop. volatile bool _slot_attempt_pending[RUNTIME_MQTT_SLOTS]; // CLI-requested forced NTP sync, marshalled onto the MQTT task (Core 0). @@ -381,8 +385,9 @@ private: // _device_id[65], _board_model[64], _firmware_version[64] and a 63-byte client version): // measured 762/768 before the connection-health fields existed. Overflow is silent — // serializeComplete() returns 0 and the bridge publishes only len>0, so status stops - // entirely. Raised to 1024, which costs no extra RAM here because status serializes into - // the shared scratch buffer. See the worst-case test in test_mqtt_payload_builder. + // entirely. Raised to 1024: no extra RAM while status serializes into the shared scratch + // buffer, but +256 B of bridge-task stack in the fallback used if the PSRAM allocation + // failed. See the worst-case test in test_mqtt_payload_builder. static const size_t STATUS_JSON_BUFFER_SIZE = 1024; static_assert(STATUS_JSON_BUFFER_SIZE <= PUBLISH_JSON_BUFFER_SIZE, "status payloads serialize into the shared publish buffer"); 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 e8ae51c87..eedf69795 100644 --- a/test/test_mqtt_payload_builder/test_mqtt_payload_builder.cpp +++ b/test/test_mqtt_payload_builder/test_mqtt_payload_builder.cpp @@ -222,11 +222,11 @@ TEST(MQTTPayloadBuilder, WorstCaseStatusWithConnectionHealthFitsProductionBuffer char buffer[kStatusJsonBufferSize]; MQTTConnHealth health; health.slots_up = 0; - health.slots_total = 5; // PSRAM slot cap, the widest configuration + health.slots_total = 6; // RUNTIME_MQTT_SLOTS on PSRAM, the widest health.worst_outage_secs = 4294967; // millis() rollover / 1000, the widest value health.heap_largest = 8388608; health.connect_failures = 2147483647; - health.slots_breaker = 5; + health.slots_breaker = 6; int len = MQTTPayloadBuilder::buildStatusMessage( scratch, origin.c_str(), origin_id.c_str(), model.c_str(), firmware.c_str(),