fix(mqtt): slot-count-aware cooperative-stop timeout
Phase 0 hardware characterization showed real mbedTLS/wss client teardown takes ~5-6s per connected slot, sequentially, so the flat 8s MQTT_STOP_TIMEOUT_MS force-timed-out healthy multi-slot stops (2-slot non-PSRAM ~11-12s, 3-slot PSRAM ~16s), which sets the dirty latch and makes the OTA barrier withhold flashing -- multi-slot nodes could never ota update. Replace the flat constant with a slot-scaled budget computed per stop in end(): 5s base + 8s per enabled slot (~1.5-2x headroom over measured), applied via new MQTTLifecycle::Coordinator::setStopTimeoutMs() before requestStop(). Headroom is nearly free: end() returns as soon as the task acks (checked before the timeout ticks), so a larger bound only lengthens the wait before force-killing a genuinely wedged task. Hardware-verified on V3: a 2-slot stop that force-timed-out at 8s pre-fix now logs "timeout 21000 ms" and acks clean in ~11.7s. Native suite + both observer firmware builds green.
This commit is contained in:
1 parent
29a7016e03
commit
7767760fae
3 files changed
+86
-33
No files matched your search
@@ -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:
|
||||
|
||||
@@ -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); }
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in new issue
Block a user