diff --git a/STABILITY_TESTABILITY_HANDOFF.md b/STABILITY_TESTABILITY_HANDOFF.md index 7f61bc27..4fbdeeb1 100644 --- a/STABILITY_TESTABILITY_HANDOFF.md +++ b/STABILITY_TESTABILITY_HANDOFF.md @@ -26,7 +26,7 @@ index. | 1 | PR CI smoke builds + ArduinoJson pin enforcement | Done (build-size gate and ASan/UBSan still pending) | | 2 | PSRAM restart resource symmetry | Done | | 3 | MQTT preference migration fixtures | Done (filesystem adapter still lives in `CommonCLI`) | -| 0 | Pre-change lifecycle characterization | Hardware run 2026-07-19 (see "Hardware Characterization Results"): teardown timing measured on V3+V4. **Finding: `MQTT_STOP_TIMEOUT_MS=8000` is too small — trips dirty/OTA-withheld on healthy multi-slot nodes; fix pending review** | +| 0 | Pre-change lifecycle characterization | Hardware run 2026-07-19 (see "Hardware Characterization Results"): teardown timing measured on V3+V4. Finding: flat `MQTT_STOP_TIMEOUT_MS=8000` too small → **FIXED** with slot-scaled timeout (`5s + 8s×slots`), hardware-verified (2-slot stop now clean) | | 4 | Ownership and teardown test seams | Seams + ownership doc + teardown tests done; production rewiring deferred to Phase 5 | | 5 | Cooperative MQTT shutdown | Minimal cooperative `end()` + `begin()` guard + OTA barrier implemented on branch `phase5/cooperative-mqtt-shutdown` (native green, firmware smoke build green); NOT hardware-validated. Volatile-handshake replacement + snapshot-consumer repointing deferred | | — | OTA teardown barrier | Implemented — flash gated on a clean MQTT stop in `simple_repeater`; not hardware-validated | @@ -89,18 +89,29 @@ permanently withheld** with the current timeout — the barrier misclassifies healthy stops as dirty. This inverts the barrier's intent and must be fixed before Phase 5 ships. -**Recommendation (needs review — a Phase 5 code change, not applied here):** +**FIX APPLIED (slot-count-aware timeout) — verified on hardware.** -- Raise `MQTT_STOP_TIMEOUT_MS` so healthy multi-slot teardowns complete cleanly. - Either a single constant sized for the 5-slot PSRAM worst case (**≥ ~35 s**, - with headroom), or — preferred — **make it slot-count-aware**, e.g. - `base 4 s + 6 s × active_slots` (2→16 s, 3→22 s, 5→34 s). -- A larger fixed timeout lengthens the loop-task stall on every stop - (`ota update`, reconfigure-restart, `set bridge.enabled off`); the slot-aware - form avoids penalizing 1-slot nodes. -- Alternative/complementary (deeper Phase 5 work): shorten the per-slot - `esp_mqtt_client_destroy()` wait so total teardown drops under a smaller - timeout. Out of scope for this characterization; flagged for decision. +The flat `MQTT_STOP_TIMEOUT_MS = 8000` is replaced by a slot-scaled budget +computed per stop in `MQTTBridge::end()`: + +``` +timeout = MQTT_STOP_TIMEOUT_BASE_MS (5000) + MQTT_STOP_TIMEOUT_PER_SLOT_MS (8000) × enabled_slots +``` + +→ 1 slot 13 s, 2 slots 21 s, 3 slots 29 s, 5 slots 45 s — ~1.5–2× headroom over +the measured ~5–6 s/slot teardown. `end()` counts enabled slots and calls +`Coordinator::setStopTimeoutMs()` before `requestStop()`. Headroom is nearly free +because `end()` returns as soon as the task acks (`_stop_acked` is checked before +the timeout ticks), so a larger bound does not slow a healthy stop — it only +lengthens the wait before force-killing a genuinely wedged task. (Files: +`MQTTBridge.cpp` constants + `end()`; `MQTTLifecycle.h` `setStopTimeoutMs()`.) + +**Hardware verification (V3, this fix):** the same 2-slot config that force-timed- +out at 8 s pre-fix now logs `MQTT stop: 2 enabled slot(s), timeout 21000 ms` and +acks in ~11.7 s → **`MQTT Bridge stopped (clean)`** (OTA no longer withheld). +Native suite + both firmware builds green. Complementary future option (not done): +shorten the per-slot `esp_mqtt_client_destroy()` wait to reduce absolute teardown +time. ### Phase 0 — cooperative lifecycle (V3, non-PSRAM, this branch) @@ -555,16 +566,16 @@ publishing a plain-data status snapshot to repoint the §1/§2 consumers in that can still touch a torn-down bridge). Those are not required to fix the OTA panic and would enlarge the merge-sensitive diff. -**Phase 0 dependency — CHARACTERIZED 2026-07-19, ACTION REQUIRED:** -`MQTT_STOP_TIMEOUT_MS` = 8 s placeholder is **too small**. Measured per-wss-slot -teardown is ~5–6 s sequential, so a healthy stop needs ~11–12 s at 2 slots -(non-PSRAM max) and ~16 s at 3 slots / ~27–30 s at 5 slots (PSRAM). At 8 s these -healthy stops trip the dirty/timeout fallback and the OTA barrier withholds -flashing — i.e. multi-slot nodes could never `ota update`. Raise it (single -constant ≥ ~35 s for the 5-slot worst case, or preferably slot-count-aware, -e.g. `4 s + 6 s × active_slots`) before this ships. See "Hardware -Characterization Results (Phase 0 & Phase 7)". It is a single named constant so -the change is trivial. +**Phase 0 dependency — CHARACTERIZED + FIXED 2026-07-19:** the flat +`MQTT_STOP_TIMEOUT_MS` = 8 s placeholder was **too small** (measured per-wss-slot +teardown ~5–6 s sequential → ~11–12 s at 2 slots, ~16 s at 3, ~27–30 s at 5; at +8 s healthy multi-slot stops tripped the dirty fallback and the OTA barrier +withheld flashing). Replaced with a **slot-scaled timeout** set per stop in +`end()`: `5 s + 8 s × enabled_slots` (`MQTT_STOP_TIMEOUT_BASE_MS` / +`MQTT_STOP_TIMEOUT_PER_SLOT_MS`, applied via `Coordinator::setStopTimeoutMs()`). +Hardware-verified: a 2-slot stop that force-timed-out pre-fix now acks clean in +~11.7 s within the 21 s budget. See "Hardware Characterization Results (Phase 0 & +Phase 7)". The original plan of record follows. Replace direct task deletion with an explicit lifecycle such as: diff --git a/src/helpers/MQTTLifecycle.h b/src/helpers/MQTTLifecycle.h index f7b4a5c4..d85aafa9 100644 --- a/src/helpers/MQTTLifecycle.h +++ b/src/helpers/MQTTLifecycle.h @@ -245,9 +245,12 @@ struct Ops { // timeout. Idempotent start/stop; safe restart after a completed stop. class Coordinator { public: - // Phase 0 TODO: stop_timeout_ms must be characterized against real mbedTLS - // teardown timing over wss with a down broker before Phase 5 ships. It is an - // injected parameter precisely so it is not a guessed constant here. + // stop_timeout_ms is the bound on how long a requested stop may run before the + // reviewed force-kill fallback fires. It is injected (not a constant here) and + // may be updated per stop via setStopTimeoutMs(): Phase 0 hardware + // characterization (2026-07-19) showed real mbedTLS/wss teardown scales with + // the number of connected slots (~5-6 s each, sequential), so the owner sizes + // it to the current slot count before each stop. See MQTTBridge::end(). Coordinator(Ops& ops, uint32_t stop_timeout_ms) : _ops(ops), _stop_timeout_ms(stop_timeout_ms) {} @@ -272,6 +275,12 @@ class Coordinator { return _state == State::Stopped && !_stop_timed_out; } + // Update the stop-timeout bound. Call before requestStop() to size the window + // to the current slot count (see MQTTBridge::end()); the value is read by + // tick() against _stop_request_ms, which requestStop() arms afterwards. + void setStopTimeoutMs(uint32_t ms) { _stop_timeout_ms = ms; } + uint32_t stopTimeoutMs() const { return _stop_timeout_ms; } + bool requestStart() { return dispatch(Event::StartRequested); } bool requestStop() { return dispatch(Event::StopRequested); } bool onTaskStarted() { return dispatch(Event::StartCompleted); } diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index af241ee5..4bc30664 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -474,13 +474,30 @@ void MQTTBridge::formatSlotDiagReply(char* buf, size_t bufsize, int slot_index) } // Bounded cooperative-stop timeout for end() (see MQTTLifecycle::Coordinator). -// Phase 0 TODO: characterize on hardware — mbedTLS teardown over wss with a -// DOWN broker — before this ships. The placeholder is chosen conservatively: -// long enough for a real cooperative client teardown (per-slot disconnect + the -// existing 50 ms settle delays across RUNTIME_MQTT_SLOTS), short enough to bound -// the loop-task stall and the OTA/restart wait. A stop that exceeds this is -// treated as dirty: the task is force-killed and OTA flashing is withheld. -static const uint32_t MQTT_STOP_TIMEOUT_MS = 8000; +// Phase 0 hardware characterization (2026-07-19, Heltec V3 non-PSRAM + V4 PSRAM, +// see STABILITY_TESTABILITY_HANDOFF.md): a real mbedTLS/wss client teardown +// (disconnect + esp_mqtt_client_destroy) takes ~5-6 s per CONNECTED slot, applied +// SEQUENTIALLY in destroySlotClients(). So the safe timeout scales with the +// number of slots being torn down, not a single constant: a flat 8 s tripped the +// dirty/force-kill fallback on a healthy 2-slot non-PSRAM node (~11-12 s) and a +// normal 3-slot PSRAM node (~16 s), which withholds OTA on healthy devices. +// +// The budget below gives generous headroom (~8 s/slot vs the ~5-6 s measured) +// plus a fixed base for WiFi/queue/buffer teardown. Headroom is nearly free: +// end() returns as soon as the task acks (it checks _stop_acked before ticking +// the timeout), so a larger bound does NOT slow a healthy stop — it only length- +// ens the wait before force-killing a genuinely wedged task. The timeout is set +// per stop in end() via computeStopTimeoutMs() based on the enabled-slot count. +static const uint32_t MQTT_STOP_TIMEOUT_BASE_MS = 5000; // fixed teardown overhead +static const uint32_t MQTT_STOP_TIMEOUT_PER_SLOT_MS = 8000; // ~5-6 s measured + headroom + +// Slot-scaled cooperative-stop timeout. `slots` is the number of MQTT slots that +// will be torn down (enabled/connected); clamped to >=1 so a zero-slot bridge +// still budgets for the base teardown. +static inline uint32_t mqttStopTimeoutForSlots(int slots) { + if (slots < 1) slots = 1; + return MQTT_STOP_TIMEOUT_BASE_MS + MQTT_STOP_TIMEOUT_PER_SLOT_MS * (uint32_t)slots; +} // --------------------------------------------------------------------------- // Constructor @@ -525,7 +542,9 @@ MQTTBridge::MQTTBridge(NodePrefs *prefs, MQTTPrefs *obs, mesh::PacketManager *mg #endif // Cooperative lifecycle: _lifecycle_ops must be constructed before // _lifecycle (declaration order guarantees this) so the reference binds. - , _lifecycle_ops(this), _lifecycle(_lifecycle_ops, MQTT_STOP_TIMEOUT_MS) + // Seed with the worst-case (max runtime slots) budget; end() recomputes the + // slot-scaled timeout before each stop via setStopTimeoutMs(). + , _lifecycle_ops(this), _lifecycle(_lifecycle_ops, mqttStopTimeoutForSlots(RUNTIME_MQTT_SLOTS)) { // Initialize default values strncpy(_origin, "MeshCore-Repeater", sizeof(_origin) - 1); @@ -898,6 +917,20 @@ void MQTTBridge::end() { // Stop new diagnostic reads through the singleton before teardown begins. s_mqtt_bridge_instance = nullptr; + // Size the stop timeout to the work about to happen: each enabled slot's + // mbedTLS/wss client takes ~5-6 s to disconnect + destroy, sequentially (Phase + // 0 hardware characterization). A flat bound force-killed healthy multi-slot + // nodes and withheld OTA; the slot-scaled budget lets a normal teardown ack + // cleanly. Count enabled slots (the ones that connect); a disabled slot's + // client destroys quickly. Must run BEFORE requestStop() arms the window. + int stop_slots = 0; + for (int i = 0; i < RUNTIME_MQTT_SLOTS; i++) { + if (_slots[i].enabled) stop_slots++; + } + _lifecycle.setStopTimeoutMs(mqttStopTimeoutForSlots(stop_slots)); + MQTT_DEBUG_PRINTLN("MQTT stop: %d enabled slot(s), timeout %lu ms", + stop_slots, (unsigned long)_lifecycle.stopTimeoutMs()); + // Cooperative shutdown (Phase 5). Request the stop, then let the lifecycle // Coordinator drive it. On ESP32 the MQTT task (Core 0) tears down its own // clients where the mbedTLS contexts live and acknowledges via _stop_acked;