From 172ef36843d6395b69be0250fff2a0bd01e141ef Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 19 Sep 2026 14:22:00 -0700 Subject: [PATCH] fix(mqtt): gate room-server OTA on an unproven stop; reap late acks - The room server checked canFlashAfterStop() only when the bridge was running at OTA time. After a timed-out restart the bridge reads as stopped while its unacknowledged task may still own TLS/client state, so an OTA could erase and write flash under it. It now refuses while the stop is unproven, after first reaping any late ack. The repeater already gated unconditionally. - MyMesh::loop() now reaps a late stop acknowledgement whenever it lands, releasing the withheld queue and buffers, and restarts only when a resume is pending and the bridge is enabled. Before, a bridge disabled during StopUnproven kept those resources until re-enabled or rebooted. pollLateStopAck() is public for this. - The wrapper's destructor no longer stops an already-stopped client: destroySlotClients() had just stopped it, so every shutdown logged five spurious "esp_mqtt_client_stop failed: ESP_FAIL" errors. Hardware (Heltec V4, 1 s test stop deadline): restart -> StopUnproven -> `set bridge.enabled off`; the late ack was reaped ("releasing withheld resources"), status read "not running", and `set bridge.enabled on` started cleanly without a second release. --- examples/simple_repeater/MyMesh.cpp | 15 ++++++----- examples/simple_room_server/MyMesh.cpp | 25 ++++++++++++------- .../src/PsychicMqttClient.cpp | 3 ++- src/helpers/bridges/MQTTBridge.h | 6 ++--- 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/examples/simple_repeater/MyMesh.cpp b/examples/simple_repeater/MyMesh.cpp index f9bdf7a18..a8a0b2018 100644 --- a/examples/simple_repeater/MyMesh.cpp +++ b/examples/simple_repeater/MyMesh.cpp @@ -1700,12 +1700,15 @@ void MyMesh::loop() { mesh::Mesh::loop(); #ifdef WITH_MQTT_BRIDGE - // A timed-out stop keeps the bridge down until the MQTT task acknowledges it; - // once that late ack lands, restart the bridge that was meant to be running. - if (_bridge_resume_pending && _prefs.bridge_enabled && bridge && - bridge->stopAcknowledgedLate()) { - Serial.println("MQTT: stop acknowledged late - resuming bridge"); - setBridgeState(true); + // A timed-out stop keeps the bridge down until the MQTT task acknowledges it. + // Release the withheld resources whenever that late ack lands, and restart + // only a bridge that is still meant to be running. + if (bridge && bridge->stopAcknowledgedLate()) { + bridge->pollLateStopAck(); + if (_bridge_resume_pending && _prefs.bridge_enabled) { + Serial.println("MQTT: stop acknowledged late - resuming bridge"); + setBridgeState(true); + } } #endif diff --git a/examples/simple_room_server/MyMesh.cpp b/examples/simple_room_server/MyMesh.cpp index 62fe821b2..7c13018c8 100644 --- a/examples/simple_room_server/MyMesh.cpp +++ b/examples/simple_room_server/MyMesh.cpp @@ -1516,12 +1516,15 @@ void MyMesh::loop() { mesh::Mesh::loop(); #ifdef WITH_MQTT_BRIDGE - // A timed-out stop keeps the bridge down until the MQTT task acknowledges it; - // once that late ack lands, restart the bridge that was meant to be running. - if (_bridge_resume_pending && _prefs.bridge_enabled && bridge && - bridge->stopAcknowledgedLate()) { - Serial.println("MQTT: stop acknowledged late - resuming bridge"); - setBridgeState(true); + // A timed-out stop keeps the bridge down until the MQTT task acknowledges it. + // Release the withheld resources whenever that late ack lands, and restart + // only a bridge that is still meant to be running. + if (bridge && bridge->stopAcknowledgedLate()) { + bridge->pollLateStopAck(); + if (_bridge_resume_pending && _prefs.bridge_enabled) { + Serial.println("MQTT: stop acknowledged late - resuming bridge"); + setBridgeState(true); + } } #endif #ifdef WITH_MQTT_BRIDGE @@ -1619,14 +1622,18 @@ void MyMesh::loop() { drainOutbound(OTA_TX_DRAIN_TIMEOUT_MS); bool may_flash = true; + if (bridge) bridge->pollLateStopAck(); if (bridge_was_running) { setBridgeState(false); // OTA must not write after a forced/timed-out MQTT shutdown: its TLS/heap // ownership is uncertain until a subsequent clean start/stop cycle. may_flash = bridge && bridge->canFlashAfterStop(); - if (!may_flash) { - Serial.println("OTA: aborted, MQTT stop did not complete cleanly"); - } + } else if (bridge && bridge->isStopUnproven()) { + // Reads as stopped, but an unacknowledged MQTT task may still own TLS/client state. + may_flash = false; + } + if (!may_flash) { + Serial.println("OTA: aborted, MQTT stop did not complete cleanly"); } char ota_reply[160]; diff --git a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp index ccd11341d..01efb194b 100644 --- a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp +++ b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp @@ -20,7 +20,8 @@ PsychicMqttClient::PsychicMqttClient() : _mqtt_cfg() PsychicMqttClient::~PsychicMqttClient() { - disconnect(); + // Owners stop the client before deleting it; stopping it again only logs ESP_FAIL. + if (_started) disconnect(); if (_client != nullptr) { esp_mqtt_client_destroy(_client); diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index e733044ff..b88b35a63 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -602,9 +602,6 @@ private: void logMemoryStatus(); void refreshOriginFromPrefs(); void applyWifiPowerSave(); // one mapping, applied on every association - // Honours a stop acknowledgement that arrived after the deadline: releases the - // withheld resources and makes the bridge restartable. Loop task only. - void pollLateStopAck(); // begin()/end()-scoped PSRAM buffers. Each allocation is independent so a // transient heap shortage degrades to the existing stack fallback instead // of making the bridge unusable. @@ -746,6 +743,9 @@ public: // The unproven stop has since been acknowledged, so begin() will release the // withheld resources and start. Loop task only. bool stopAcknowledgedLate() const; + // Honours a stop acknowledgement that arrived after the deadline: releases the + // withheld resources and makes the bridge restartable. Loop task only. + void pollLateStopAck(); // Survives end() clearing the diagnostic singleton, so `get mqtt.status` can // still explain why a stopped bridge will not come back without a reboot. static bool stopUnprovenLatched();