From 40635a53c40048be9095bb5243545bf9e82ff351 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 09:05:05 -0700 Subject: [PATCH 01/10] test(native): declare stdlib in the Arduino mock so ConfigSerializer builds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The real Arduino.h includes stdlib.h, so ConfigSerializer.cpp reaches atoi, atol and atof through it and compiles on device. The mock supplied only cstdint, cmath and Stream.h, leaving those undeclared — and since the native env compiles ConfigSerializer.cpp into every suite via build_src_filter, all 21 suites errored rather than just its own. Fixing the mock keeps src/ identical to upstream and covers any other source relying on the same transitive include. pio test -e native: 297 test cases, 297 succeeded. --- test/mocks/Arduino.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/mocks/Arduino.h b/test/mocks/Arduino.h index 77499fe414..a3d5b2765e 100644 --- a/test/mocks/Arduino.h +++ b/test/mocks/Arduino.h @@ -2,8 +2,15 @@ #include #include +// The real Arduino.h pulls in stdlib.h, so device code reaches atoi/atol/atof/strtoul +// without including it. Mirror that here or those sources fail only on the native build. +#include #include "Stream.h" +using std::atof; +using std::atoi; +using std::atol; + inline uint32_t g_mock_millis = 0; using std::isnan; From 4c90db2199fdc149ee9cb6293e8773edff3f57b8 Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 5 Aug 2026 22:29:51 -0700 Subject: [PATCH 02/10] fix(mqtt): renew JWT credentials without stopping the esp-mqtt client The scheduled JWT bounce called PsychicMqttClient::disconnect(), which ends with esp_mqtt_client_stop(). That ends the client task and returns its 6 KiB stack to the heap at the moment the TLS teardown vacates two 16 KiB mbedTLS record buffers, so the stack lands in that hole and the next handshake cannot reuse it. On non-PSRAM boards the largest free block then ratchets down 16 KiB at a time while total free heap stays flat. Soak evidence from a Heltec V3 on 8d1a0eb3: 43 of 60 disconnects had no preceding transport error, i.e. they were this proactive bounce rather than a broker FIN, and two of the three max_alloc steps landed within 5 s of one. Losing a whole TLS session later returned exactly 16,384 bytes of contiguity. softDisconnect() closes the transport without the stop, so the task and its stack stay put across the handshake. The bounce uses it plus reconnect(), and falls back to connect() when the client really is stopped, since reconnect() is a silent no-op in that state. Also corrects a comment claiming the mbedTLS context survives a transport close: only the esp-mqtt client object does. (cherry picked from commit 10cf5cf48fb009e751e25b37fcc1f3d1256ddbbc) --- .../src/PsychicMqttClient.cpp | 39 ++++++++++++++++++- lib/PsychicMqttClient/src/PsychicMqttClient.h | 24 ++++++++++++ src/helpers/bridges/MQTTBridge.cpp | 29 ++++++++++---- src/helpers/bridges/MQTTBridge.h | 13 +++++++ 4 files changed, 97 insertions(+), 8 deletions(-) diff --git a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp index abe49c88e3..5d037b7e30 100644 --- a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp +++ b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp @@ -433,7 +433,12 @@ void PsychicMqttClient::connect() } } - ESP_ERROR_CHECK_WITHOUT_ABORT(esp_mqtt_client_start(_client)); + esp_err_t start_result = esp_mqtt_client_start(_client); + ESP_ERROR_CHECK_WITHOUT_ABORT(start_result); + if (start_result == ESP_OK) + { + _started = true; + } ESP_LOGI(TAG, "MQTT client started."); } @@ -489,9 +494,40 @@ void PsychicMqttClient::disconnect() } esp_mqtt_client_stop(_client); + _started = false; ESP_LOGI(TAG, "MQTT client stopped."); } +void PsychicMqttClient::softDisconnect(unsigned long timeout_ms) +{ + if (_client == nullptr) + { + ESP_LOGW(TAG, "MQTT client not started."); + return; + } + + if (!_connected) + { + // Nothing to close; leaving the task alone is the whole point. + return; + } + + ESP_LOGI(TAG, "Disconnecting MQTT transport (client task retained)."); + _stopMqttClient = false; + esp_mqtt_client_disconnect(_client); + + unsigned long waited = 0; + while (!_stopMqttClient && waited < timeout_ms) + { + vTaskDelay(10 / portTICK_PERIOD_MS); + waited += 10; + } + if (!_stopMqttClient) + { + ESP_LOGW(TAG, "softDisconnect: no DISCONNECTED event in %lums", timeout_ms); + } +} + void PsychicMqttClient::forceStop() { if (_client == nullptr) @@ -506,6 +542,7 @@ void PsychicMqttClient::forceStop() } ESP_ERROR_CHECK_WITHOUT_ABORT(esp_mqtt_client_stop(_client)); _connected = false; + _started = false; ESP_LOGI(TAG, "MQTT client forcefully stopped."); } diff --git a/lib/PsychicMqttClient/src/PsychicMqttClient.h b/lib/PsychicMqttClient/src/PsychicMqttClient.h index 42cf8d5c3d..28e53fc3a1 100644 --- a/lib/PsychicMqttClient/src/PsychicMqttClient.h +++ b/lib/PsychicMqttClient/src/PsychicMqttClient.h @@ -368,6 +368,29 @@ class PsychicMqttClient */ void disconnect(); + /** + * @brief Closes the transport but leaves the client task running. + * + * disconnect() ends with esp_mqtt_client_stop(), which ends the client task + * and returns its 6 KiB stack to the heap right as a TLS handshake vacates + * two 16 KiB mbedTLS record buffers — the stack then lands in that hole and + * the largest free block ratchets down. This variant omits the stop, so the + * task and its stack stay put. Pair it with reconnect(). + * + * @param timeout_ms how long to wait for the DISCONNECTED event before + * giving up. Bounded on purpose: disconnect()'s wait is + * unbounded and a lost event would wedge the caller. + */ + void softDisconnect(unsigned long timeout_ms = 5000); + + /** + * @brief True once esp_mqtt_client_start() has succeeded and no stop has run. + * + * reconnect() silently does nothing on a stopped client, so callers that + * want to avoid stop/start must check this and fall back to connect(). + */ + bool isStarted() const { return _started; } + /** * @brief Forcefully stops the MQTT client and disconnects from the server. * This does not trigger the onDisconnect callbacks. @@ -478,6 +501,7 @@ class PsychicMqttClient bool _connected = false; bool _stopMqttClient = false; bool _config_dirty = true; + bool _started = false; // Runtime cap on the esp-mqtt outbox for QoS 0 async publishes (bytes). // 0 = disabled. Enforced in publish(); not an esp-mqtt config field. diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 735999e97e..bb2ff93e95 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -1775,9 +1775,11 @@ bool MQTTBridge::setupSlot(int index) { } // Reconfigure path: if we're re-applying (e.g. after a preset change), stop - // the existing connection cleanly first. The client object (and its mbedTLS - // context) is reused; setCredentials / setServer below overwrite the config - // fields in place before connect() restarts the ESP-IDF client. + // the existing connection cleanly first. The client object is reused, but its + // mbedTLS context is NOT — closing the transport destroys the TLS session, + // record buffers, and peer certificate, and the next connect() reallocates + // them. setCredentials / setServer below overwrite the config fields in place + // before connect() restarts the ESP-IDF client. if (slot.initial_connect_done) { if (slot.client->connected()) { slot.client->disconnect(); @@ -2140,11 +2142,24 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns // Disconnect + reconnect with fresh credentials, reusing existing client // to avoid internal heap leak/fragmentation from destroy/create cycles MQTT_DEBUG_PRINTLN("MQTT%d token renewal: reconnecting with fresh credentials", index + 1); - if (slot.client->connected()) { - slot.client->disconnect(); // stops the client internally + MQTT_TRACE_HEAP("renewal:before-bounce", index); + if (slot.client->isStarted()) { + // Keep the esp-mqtt task alive across the handshake. disconnect() + // would stop it, returning its 6 KiB stack into the hole the two + // 16 KiB mbedTLS record buffers just vacated — which is what walks + // the largest free block down 16 KiB at a time on non-PSRAM boards. + slot.client->softDisconnect(); + MQTT_TRACE_HEAP("renewal:after-disconnect", index); + slot.client->setCredentials(_jwt_username, slot.auth_token); + MQTT_TRACE_HEAP("renewal:after-credentials", index); + slot.client->reconnect(); + } else { + // Client was stopped (teardown/reconfigure). reconnect() is a no-op + // on a stopped client, so this path must start it. + slot.client->setCredentials(_jwt_username, slot.auth_token); + slot.client->connect(); } - slot.client->setCredentials(_jwt_username, slot.auth_token); - slot.client->connect(); // restart stopped client; reconnect() fails silently on a stopped client + MQTT_TRACE_HEAP("renewal:after-reconnect", index); reconnect_attempted = true; _last_slot_reconnect_ms = now_millis; MQTT_DEBUG_PRINTLN("MQTT%d int_heap=%d at token renewal reconnect", index + 1, diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index eb7ab94349..6056a46a09 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -36,6 +36,19 @@ class MeshSNMPAgent; // Forward declaration #define MQTT_DEBUG_PRINTLN(...) {} #endif +// Largest-free-block trace around the reconnect lifecycle. On non-PSRAM boards the +// mbedTLS record buffers are two 16 KiB internal-DRAM blocks, so what matters is the +// largest contiguous block, not the free total — a soak can show flat free heap while +// max_alloc walks down. Costs two heap_caps calls per reconnect, so it stays on. +#if defined(MQTT_DEBUG) && defined(ARDUINO) && defined(ESP32) + #define MQTT_TRACE_HEAP(point, idx) \ + MQTT_DEBUG_PRINTLN("HEAPTRACE slot=%d %s free=%u max=%u", (int)(idx) + 1, point, \ + (unsigned)heap_caps_get_free_size(MALLOC_CAP_INTERNAL), \ + (unsigned)heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL)) +#else + #define MQTT_TRACE_HEAP(point, idx) do {} while(0) +#endif + #ifdef WITH_MQTT_BRIDGE // Periodic neighbors publication keys off the mesh neighbor cache (sized by From 8ee21d2c0fabdebe6b0fba0e49a4440eb3b0b0ec Mon Sep 17 00:00:00 2001 From: agessaman Date: Mon, 10 Aug 2026 15:45:56 -0700 Subject: [PATCH 03/10] fix(mqtt): allocate the neighbors JSON buffer on first use, not at bridge start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit allocateRuntimeBuffers() took NEIGHBORS_JSON_BUFFER_SIZE unconditionally on every board built WITH_MQTT_NEIGHBORS, whether or not mqtt.neighbors was ever turned on. On a non-PSRAM board that is 4 KB of internal DRAM held for the bridge's lifetime by a node that may never publish a neighbours snapshot. Gating the existing allocation on the pref would not work: mqtt.neighbors is read live by the mesh loop with no bridge restart, so enabling it at runtime would find no buffer and silently publish nothing. Allocate on first use instead, in requestPublishNeighbors(), which is reached only when something actually wants to publish — periodic or a manual discovery. Publishing the pointer across cores is safe with the existing handshake: the allocation precedes the release store on _neighbors_publish_pending, and the task loop reads the pointer only after its matching acquire load, so the pointer cannot be observed half-published. A failed allocation drops that one snapshot and retries on the next, rather than disabling neighbours for the bridge's lifetime as the eager path did. (cherry picked from commit e6da052a93f8765824d0fb4bd0c704ca3ed3d294) --- src/helpers/bridges/MQTTBridge.cpp | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index bb2ff93e95..da2015969e 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -768,17 +768,10 @@ void MQTTBridge::allocateRuntimeBuffers() { _json_scratch_buffer ? "PSRAM" : "stack fallback"); #endif -#if defined(WITH_MQTT_NEIGHBORS) - // Persistent neighbors JSON buffer, heap-allocated on every board: too large to - // keep inline in the bridge object the way the non-PSRAM status/packet buffers - // are. psram_malloc() falls back to internal DRAM, so this works without PSRAM. - // Unlike status/packet there is no stack fallback — a nullptr simply disables - // publishing (requestPublishNeighbors/publishNeighbors both no-op on nullptr). - _neighbors_json_buffer = static_cast(MQTTRuntimeBufferLifecycle::allocateIfMissing( - _neighbors_json_buffer, NEIGHBORS_JSON_BUFFER_SIZE, psram_malloc)); - MQTT_DEBUG_PRINTLN("Neighbors buffer: %s", - _neighbors_json_buffer ? "ready" : "unavailable"); -#endif + // The neighbors JSON buffer is NOT allocated here — requestPublishNeighbors() + // allocates it on first use, so a node with mqtt.neighbors off never pays its + // 4 KB. mqtt.neighbors is read live with no bridge restart, so gating on the + // pref here would leave a runtime enable with no buffer. } void MQTTBridge::releaseRuntimeBuffers() { @@ -795,7 +788,7 @@ void MQTTBridge::releaseRuntimeBuffers() { _json_scratch_doc.clear(); #if defined(WITH_MQTT_NEIGHBORS) - // Paired with the unconditional allocation in allocateRuntimeBuffers(). + // Paired with the lazy allocation in requestPublishNeighbors(); no-op if never used. _neighbors_json_buffer = static_cast(MQTTRuntimeBufferLifecycle::release( _neighbors_json_buffer, psram_free)); _neighbors_publish_len = 0; @@ -3640,10 +3633,19 @@ void MQTTBridge::setNeighborsSchedule(NeighborsPhase phase, uint32_t secs_until_ } void MQTTBridge::requestPublishNeighbors(const char* json, size_t len) { - if (!_neighbors_json_buffer || !json || len == 0) return; + if (!json || len == 0) return; // Drop a new snapshot while one is still being published (Core 0 clears the // flag when done). Acquire pairs with the task loop's release store. if (_neighbors_publish_pending.load(std::memory_order_acquire)) return; + // Allocated on first use so a node with neighbors off never pays the 4 KB. + // Cross-core safe: the release store below publishes this pointer, and the task + // loop only reads it after the matching acquire load. + _neighbors_json_buffer = static_cast(MQTTRuntimeBufferLifecycle::allocateIfMissing( + _neighbors_json_buffer, NEIGHBORS_JSON_BUFFER_SIZE, psram_malloc)); + if (!_neighbors_json_buffer) { + MQTT_DEBUG_PRINTLN("Neighbors buffer unavailable, dropping snapshot"); + return; + } if (len >= NEIGHBORS_JSON_BUFFER_SIZE) { len = NEIGHBORS_JSON_BUFFER_SIZE - 1; } From 8275512964063902067c26a3215af7984fde8c2e Mon Sep 17 00:00:00 2001 From: agessaman Date: Mon, 10 Aug 2026 15:57:55 -0700 Subject: [PATCH 04/10] build(mqtt): make the reduced-TLS mbedTLS archives shippable, opt-in and verified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reduced-TLS work was validated on hardware but only reachable through PLATFORMIO_BUILD_FLAGS pointing at an absolute path in a developer's home directory, so nothing outside that machine could reproduce it. Distribute the archives as a release asset instead of committing them: ~6 MB per architecture, and they must be rebuilt for every espressif32 bump, so committing would grow history permanently and go stale without any signal. scripts/mbedtls_4k_manifest.txt per-arch sha256 of each archive scripts/fetch_mbedtls_4k.sh fetch into .mbedtls-4k//, verify scripts/mbedtls_4k.py pre-build wiring and post-link proof Off by default. The script is attached to esp32_base but returns immediately unless MESHCORE_REDUCED_TLS=1, so ordinary builds need no artifact and are byte-for-byte unaffected — confirmed by building with it absent. Both ways this can fail silently produce a firmware that looks fine and lacks the change, so the opt-in path refuses to guess: - a -L at a missing or partial directory: the linker ignores an unusable search path and resolves mbedTLS from the framework. Now a hard error. - archives left over from an earlier platform version: now a sha256 mismatch against the manifest, naming both hashes. - a -L that is present but outranked, leaving the flag inert: after the link, firmware.map must resolve every libmbed*.a into .mbedtls-4k/, or the build fails and prints the offending paths. That last check earned its place immediately — it caught its own first implementation comparing a relative map path against an absolute one, and an earlier build flag in this investigation was accepted by the compiler while no source read it. A flag reaching the compiler proves nothing about the link. Verified all four paths on Heltec_v3_repeater_observer_mqtt: default build unaffected; opted in with archives present links all four from .mbedtls-4k/ and says so; archives absent fails with a fetch hint; a single appended byte fails on sha256. Also removes platformio.local.ini.hold, which held the superseded approach of pointing platform_packages at a whole custom framework. That installs over the shared framework package and changes mbedTLS for every other ESP32 project on the machine; the -L path keeps the change scoped to one env. Note the inbound record buffer stays at 16 KiB, so this lowers per-connection footprint by ~12 KiB but does not move the contiguous allocation a handshake needs. It buys headroom, not a lower floor. (cherry picked from commit a87faff6ff170c328fdd0550f4b4dd9089aa2ea0) --- .gitignore | 4 + docs/mbedtls-tls-footprint.md | 30 +++++++ platformio.ini | 2 + platformio.local.ini.hold | 15 ---- scripts/fetch_mbedtls_4k.sh | 72 ++++++++++++++++ scripts/mbedtls_4k.py | 144 ++++++++++++++++++++++++++++++++ scripts/mbedtls_4k_manifest.txt | 12 +++ 7 files changed, 264 insertions(+), 15 deletions(-) delete mode 100644 platformio.local.ini.hold create mode 100755 scripts/fetch_mbedtls_4k.sh create mode 100644 scripts/mbedtls_4k.py create mode 100644 scripts/mbedtls_4k_manifest.txt diff --git a/.gitignore b/.gitignore index 699b28241b..40f04f8d23 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,7 @@ platformio.local.ini .build-wt-*/ .wt-*/ scripts/__pycache__/* + +# Reduced-TLS mbedTLS archives, ~6 MB per arch, fetched by scripts/fetch_mbedtls_4k.sh. +# Not committed because they must be rebuilt for every espressif32 platform bump. +.mbedtls-4k/ diff --git a/docs/mbedtls-tls-footprint.md b/docs/mbedtls-tls-footprint.md index 0985c4f655..70ea6db18d 100644 --- a/docs/mbedtls-tls-footprint.md +++ b/docs/mbedtls-tls-footprint.md @@ -136,6 +136,36 @@ fix was `rm -rf` the package and `pio pkg install` to re-download stock. A prepe search path keeps the change scoped to one env, because the linker takes each archive member from the first archive that satisfies an undefined symbol. +### How the archives are distributed + +They are not committed: ~6 MB per architecture, and they have to be rebuilt for every +`platformio/espressif32` bump, so committing them would grow history permanently and go +stale silently. Instead they are published as a release asset and fetched on demand: + +``` +scripts/fetch_mbedtls_4k.sh esp32s3 # download + verify against the manifest +MESHCORE_REDUCED_TLS=1 pio run -e Heltec_v3_repeater_observer_mqtt +``` + +- `scripts/mbedtls_4k_manifest.txt` — per-arch sha256 of each archive. **Update it on every + platform bump**, together with the published asset. +- `scripts/fetch_mbedtls_4k.sh` — downloads into `.mbedtls-4k//` (gitignored) and + verifies. `MBEDTLS_4K_LOCAL=` copies from a local build tree instead of downloading. +- `scripts/mbedtls_4k.py` — wired into `esp32_base.extra_scripts`, but a **no-op unless + `MESHCORE_REDUCED_TLS=1`**, so ordinary builds need no artifact and behave as before. + +The opt-in path is deliberately loud, because both ways this can go wrong produce a +firmware that looks correct and silently lacks the change: + +| failure | what happens without a guard | guard | +|---|---|---| +| directory missing or partial | linker ignores an unusable `-L` and resolves mbedTLS from the framework | pre-build: hard error | +| archives stale after a platform bump | links the wrong build | pre-build: sha256 vs manifest | +| `-L` present but outranked | framework archives win, flag is inert | post-link: `firmware.map` must resolve every `libmbed*.a` to `.mbedtls-4k/` | + +That last one is the reason the post-link check exists rather than trusting the flag: a +build flag reaching the compiler proves nothing about what got linked. + ## How to verify it worked 1. `strings`/`grep` the new `sdkconfig.h` for the four settings. diff --git a/platformio.ini b/platformio.ini index 6c012f1e52..73ce373b86 100644 --- a/platformio.ini +++ b/platformio.ini @@ -64,6 +64,8 @@ platform = platformio/espressif32@6.11.0 monitor_filters = esp32_exception_decoder extra_scripts = pre:scripts/generate_webconfig_html.py +; No-op unless MESHCORE_REDUCED_TLS=1; see docs/mbedtls-tls-footprint.md. + pre:scripts/mbedtls_4k.py merge-bin.py build_flags = ${arduino_base.build_flags} -D ESP32_PLATFORM diff --git a/platformio.local.ini.hold b/platformio.local.ini.hold deleted file mode 100644 index e0d89578f4..0000000000 --- a/platformio.local.ini.hold +++ /dev/null @@ -1,15 +0,0 @@ -; Local-only override (gitignored) pointing the Heltec V3 observer env at a custom -; framework whose mbedTLS archives were rebuilt with an asymmetric TLS record buffer: -; CONFIG_MBEDTLS_ASYMMETRIC_CONTENT_LEN=y -; CONFIG_MBEDTLS_SSL_IN_CONTENT_LEN=16384 (unchanged — a peer may send a 16 KiB record) -; CONFIG_MBEDTLS_SSL_OUT_CONTENT_LEN=4096 (was 16384) -; -; Expected: ~12 KiB less internal DRAM per TLS connection, ~24 KiB across two broker slots. -; Built from the shipped sdkconfig verbatim plus those three lines, so the archives differ -; only by this change. See docs/mbedtls-tls-footprint.md. -; -; Absolute path, hence local-only: not committable. - -[env:Heltec_v3_repeater_observer_mqtt] -platform_packages = - framework-arduinoespressif32 @ file:///Users/adam/framework-arduinoespressif32-tlsfix diff --git a/scripts/fetch_mbedtls_4k.sh b/scripts/fetch_mbedtls_4k.sh new file mode 100755 index 0000000000..f1e43c778d --- /dev/null +++ b/scripts/fetch_mbedtls_4k.sh @@ -0,0 +1,72 @@ +#!/usr/bin/env bash +# Fetch the reduced-TLS mbedTLS archives into .mbedtls-4k//. +# +# These archives are built from the shipped sdkconfig plus three lines +# (CONFIG_MBEDTLS_ASYMMETRIC_CONTENT_LEN=y, IN_CONTENT_LEN 16384, +# OUT_CONTENT_LEN 4096) and save ~12 KiB of internal DRAM per TLS connection. +# See docs/mbedtls-tls-footprint.md for the rationale and the build recipe. +# +# They are not committed: ~6 MB per architecture, and they must be rebuilt for +# every platform bump, so they are published as a release asset keyed on the +# espressif32 platform version instead. +# +# scripts/fetch_mbedtls_4k.sh [arch] # default: esp32s3 +# +# Set MBEDTLS_4K_LOCAL to skip the download and copy from a local build tree: +# MBEDTLS_4K_LOCAL=~/mbedtls-4k-esp32s3/staged scripts/fetch_mbedtls_4k.sh +set -euo pipefail + +ARCH="${1:-esp32s3}" +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +DEST="$REPO_ROOT/.mbedtls-4k/$ARCH" +MANIFEST="$REPO_ROOT/scripts/mbedtls_4k_manifest.txt" +BASE_URL="${MBEDTLS_4K_BASE_URL:-https://github.com/agessaman/MeshCore/releases/download/mbedtls-4k}" + +if [ ! -f "$MANIFEST" ]; then + echo "error: missing $MANIFEST" >&2 + exit 1 +fi + +# Manifest lines: . Blank lines and # comments ignored. +expected="$(awk -v a="$ARCH" '$1 == a && $0 !~ /^#/ {print $2" "$3}' "$MANIFEST")" +if [ -z "$expected" ]; then + echo "error: no manifest entries for arch '$ARCH'" >&2 + echo "known arches: $(awk '$0 !~ /^#/ && NF {print $1}' "$MANIFEST" | sort -u | tr '\n' ' ')" >&2 + exit 1 +fi + +mkdir -p "$DEST" + +if [ -n "${MBEDTLS_4K_LOCAL:-}" ]; then + echo "copying from $MBEDTLS_4K_LOCAL" + while read -r _sha name; do + cp "$MBEDTLS_4K_LOCAL/$name" "$DEST/$name" + done <<< "$expected" +else + TARBALL="mbedtls-4k-$ARCH.tar.gz" + echo "downloading $BASE_URL/$TARBALL" + tmp="$(mktemp -d)" + trap 'rm -rf "$tmp"' EXIT + curl -fsSL "$BASE_URL/$TARBALL" -o "$tmp/$TARBALL" + tar -xzf "$tmp/$TARBALL" -C "$tmp" + while read -r _sha name; do + # Accept the archive whether or not the tarball has a leading directory. + found="$(find "$tmp" -name "$name" -type f | head -1)" + if [ -z "$found" ]; then + echo "error: $name missing from $TARBALL" >&2 + exit 1 + fi + cp "$found" "$DEST/$name" + done <<< "$expected" +fi + +# Verify every archive against the manifest. A wrong or truncated archive would +# otherwise link silently and produce a firmware without the reduced buffers. +cd "$DEST" +if command -v shasum >/dev/null 2>&1; then + echo "$expected" | shasum -a 256 -c - +else + echo "$expected" | sha256sum -c - +fi + +echo "ok: $ARCH archives verified in $DEST" diff --git a/scripts/mbedtls_4k.py b/scripts/mbedtls_4k.py new file mode 100644 index 0000000000..9ea01d3b88 --- /dev/null +++ b/scripts/mbedtls_4k.py @@ -0,0 +1,144 @@ +"""Link the reduced-TLS mbedTLS archives, and prove they were actually linked. + +Opt in per build with MESHCORE_REDUCED_TLS=1. Off by default, so an ordinary build +needs no 6 MB artifact and behaves exactly as before. + + MESHCORE_REDUCED_TLS=1 pio run -e Heltec_v3_repeater_observer_mqtt + +The archives lower the mbedTLS outbound record buffer from 16 KiB to 4 KiB, saving +~12 KiB of internal DRAM per TLS connection on non-PSRAM observers. The inbound +buffer stays at 16 KiB, so the contiguous allocation a handshake needs is unchanged +— this buys headroom, it does not move that floor. See docs/mbedtls-tls-footprint.md. + +Two failure modes this guards against, both of which produce a firmware that looks +fine and silently lacks the change: + + - a -L pointing at a missing or partial directory. The linker ignores an + unusable search path and quietly resolves mbedTLS from the framework instead. + - archives that do not match the manifest, e.g. left over from an earlier + platform version. + +So the opt-in path verifies every archive by sha256 before the build, and after the +link re-reads firmware.map to confirm every libmbed*.a came from our directory. +""" +Import("env") + +import hashlib +import os +import sys + +REQUIRED = ("libmbedcrypto.a", "libmbedtls_2.a", "libmbedtls.a", "libmbedx509.a") + + +def _fail(msg): + print("\n*** reduced-TLS build failed ***", file=sys.stderr) + print(msg, file=sys.stderr) + print( + "\nFetch the archives with: scripts/fetch_mbedtls_4k.sh " + "\nOr build without them by unsetting MESHCORE_REDUCED_TLS.", + file=sys.stderr, + ) + env.Exit(1) + + +def _sha256(path): + h = hashlib.sha256() + with open(path, "rb") as fh: + for chunk in iter(lambda: fh.read(1 << 20), b""): + h.update(chunk) + return h.hexdigest() + + +def _manifest(project_dir, arch): + path = os.path.join(project_dir, "scripts", "mbedtls_4k_manifest.txt") + if not os.path.isfile(path): + _fail("missing scripts/mbedtls_4k_manifest.txt") + wanted = {} + with open(path) as fh: + for line in fh: + line = line.strip() + if not line or line.startswith("#"): + continue + parts = line.split() + if len(parts) == 3 and parts[0] == arch: + wanted[parts[2]] = parts[1] + return wanted + + +if os.environ.get("MESHCORE_REDUCED_TLS", "") not in ("1", "true", "yes"): + Return() + +project_dir = env.subst("$PROJECT_DIR") +arch = env.BoardConfig().get("build.mcu", "") +if not arch: + _fail("could not determine board MCU, so cannot pick an archive set") + +staged = os.path.join(project_dir, ".mbedtls-4k", arch) +if not os.path.isdir(staged): + _fail("no archives for %s at %s" % (arch, staged)) + +wanted = _manifest(project_dir, arch) +if not wanted: + _fail("manifest has no entries for arch '%s'" % arch) + +for name in REQUIRED: + archive = os.path.join(staged, name) + if not os.path.isfile(archive): + _fail("missing %s" % archive) + if name not in wanted: + _fail("%s is not in the manifest for %s" % (name, arch)) + actual = _sha256(archive) + if actual != wanted[name]: + _fail( + "%s does not match the manifest\n expected %s\n actual %s\n" + "Rebuild it for this platform version, or re-run the fetch script." + % (archive, wanted[name], actual) + ) + +# Prepend so these satisfy mbedTLS symbols ahead of the framework's own copies: +# the linker takes each archive member from the first archive that resolves it. +env.Prepend(LIBPATH=[staged]) +print("reduced-TLS: linking mbedTLS from %s (verified)" % staged) + + +def _verify_map(source, target, env): + """Confirm every mbedTLS archive in the link came from our directory.""" + map_path = os.path.join(env.subst("$BUILD_DIR"), "firmware.map") + if not os.path.isfile(map_path): + print("reduced-TLS: WARNING no firmware.map, cannot confirm the link", + file=sys.stderr) + return + # The map records whatever the linker was given, which for a -L hit is a path + # relative to the linker's cwd (the project dir). Resolve before comparing, or + # every one of our own archives reads as stray. + staged_real = os.path.realpath(staged) + stray = set() + seen = set() + with open(map_path, errors="replace") as fh: + for line in fh: + for token in line.split(): + if "libmbed" not in token or ".a" not in token: + continue + path = token.split("(")[0] + base = os.path.basename(path) + if not base.startswith("libmbed") or not base.endswith(".a"): + continue + seen.add(base) + resolved = os.path.realpath(os.path.join(project_dir, path)) + if os.path.dirname(resolved) != staged_real: + stray.add(path) + if stray: + print("\n*** reduced-TLS: archives linked from the WRONG place ***", + file=sys.stderr) + for path in sorted(stray): + print(" " + path, file=sys.stderr) + env.Exit(1) + if not seen: + print("reduced-TLS: WARNING firmware.map names no mbedTLS archive", + file=sys.stderr) + return + print("reduced-TLS: confirmed %d archives linked from %s" + % (len(seen), staged)) + + +env.AddPostAction("$BUILD_DIR/${PROGNAME}.elf", _verify_map) diff --git a/scripts/mbedtls_4k_manifest.txt b/scripts/mbedtls_4k_manifest.txt new file mode 100644 index 0000000000..06c0f38509 --- /dev/null +++ b/scripts/mbedtls_4k_manifest.txt @@ -0,0 +1,12 @@ +# Reduced-TLS mbedTLS archives (OUT_CONTENT_LEN 4096, IN_CONTENT_LEN 16384). +# Format: +# +# Rebuild these for every espressif32 platform bump — the archives must match the +# rest of the framework they link against. Built as of platformio/espressif32@6.11.0 +# from the recipe in docs/mbedtls-tls-footprint.md. +# +# Fetch with: scripts/fetch_mbedtls_4k.sh esp32s3 +esp32s3 01629f635b33ffa2c1fdfcd8ac52327cd3a92e8d4e7c9a32d92974e0cfdfe398 libmbedcrypto.a +esp32s3 07e7a09847589fefc35bc8d2f739d556535d535acd7185a9824b2af8edd5b05a libmbedtls_2.a +esp32s3 e12bcc8d76a368e819987f266e73c265178d6b6675f05d0e29014bb543c402af libmbedtls.a +esp32s3 c1e10324e19f6d7763737f72d9032cec832bdfce60c7a7d381a5fcfc7dcad573 libmbedx509.a From daec2e4edd38c573cca109bb8b8b8f73cc0522dd Mon Sep 17 00:00:00 2001 From: agessaman Date: Mon, 10 Aug 2026 16:13:15 -0700 Subject: [PATCH 05/10] =?UTF-8?q?fix(mqtt):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20stopped=20clients,=20late=20allocation,=20fail-open=20map=20?= =?UTF-8?q?check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found reviewing the preceding commits. 1. reconnectSlotClient() stranded a STOPPED client, reintroducing the very bug this branch fixes. It only rebuilt when isStarted() was true and otherwise fell through to reconnect(), which is a documented no-op on a stopped client — so nothing restarted it, at any rung, including the breaker probe. The WiFi-transition teardown reaches exactly this state: it calls the hard disconnect(), clearing _started while initial_connect_done stays set, so after WiFi returned the slot could never come back. Now a stopped client is started with connect() before the rebuild/reuse decision is considered. The post-NTP credential refresh had the same exposure — it called reconnect() directly — so it now goes through the helper too, still reusing the transport since its fault is stale credentials, not the transport. 2. Allocating the neighbors buffer on first use let a stopped bridge allocate. A neighbour discovery started before a stop can complete after it, and neither caller rechecks bridge state, so requestPublishNeighbors() would allocate 4 KB after releaseRuntimeBuffers() had already run and strand _neighbors_publish_pending with no task to consume it. end() then returns early on !_initialized, retaining the buffer until a later begin/end or a reboot. Guarded on isRunning(), the same flag end() checks. The release/acquire handoff itself was confirmed sound: the allocation and copy precede the release store, and the task loop reads the pointer only after its acquire load, so a half-published pointer is not observable. 3. The post-link map check failed open, contradicting the fail-closed claim in its own commit message. A missing map, an unrecognised map format, or a partial archive list each warned and passed; and it hardcoded firmware.map while the post-action target used ${PROGNAME}, so a renamed program could inspect a stale or absent file and still succeed. All four now fail the build, and it requires every one of the four archives to appear rather than at least one. Rebuilt Heltec_v3_repeater_observer_mqtt, Heltec_v3_repeater and heltec_v4_repeater_observer_mqtt; the opt-in path still reports all 4 archives linked from .mbedtls-4k/. (cherry picked from commit 5b5f076e5e165997e8050f2be061c7c67340fcf7) --- scripts/mbedtls_4k.py | 39 ++++++++++++++++++++++++------ src/helpers/bridges/MQTTBridge.cpp | 26 +++++++++++++++++++- src/helpers/bridges/MQTTBridge.h | 3 +++ 3 files changed, 60 insertions(+), 8 deletions(-) diff --git a/scripts/mbedtls_4k.py b/scripts/mbedtls_4k.py index 9ea01d3b88..6718cae425 100644 --- a/scripts/mbedtls_4k.py +++ b/scripts/mbedtls_4k.py @@ -102,11 +102,27 @@ def _manifest(project_dir, arch): def _verify_map(source, target, env): - """Confirm every mbedTLS archive in the link came from our directory.""" - map_path = os.path.join(env.subst("$BUILD_DIR"), "firmware.map") + """Confirm every mbedTLS archive in the link came from our directory. + + Fails closed. Anything that stops this from *proving* the link — no map, an + unparsable map, a short archive list — is a failure, not a warning. A warning + here would leave exactly the hole the check exists to close: an opt-in build + that succeeds while silently linking the framework's 16 KiB buffers. + """ + # Derive the map name from PROGNAME rather than hardcoding firmware.map, so a + # renamed program cannot leave us inspecting a stale or absent file. + map_path = os.path.join(env.subst("$BUILD_DIR"), + env.subst("${PROGNAME}") + ".map") if not os.path.isfile(map_path): - print("reduced-TLS: WARNING no firmware.map, cannot confirm the link", + legacy = os.path.join(env.subst("$BUILD_DIR"), "firmware.map") + map_path = legacy if os.path.isfile(legacy) else map_path + if not os.path.isfile(map_path): + print("\n*** reduced-TLS: no linker map at %s ***" % map_path, + file=sys.stderr) + print("Cannot prove the reduced-TLS archives were linked. Ensure the env " + "emits a map (-Wl,-Map), or unset MESHCORE_REDUCED_TLS.", file=sys.stderr) + env.Exit(1) return # The map records whatever the linker was given, which for a -L hit is a path # relative to the linker's cwd (the project dir). Resolve before comparing, or @@ -133,12 +149,21 @@ def _verify_map(source, target, env): for path in sorted(stray): print(" " + path, file=sys.stderr) env.Exit(1) - if not seen: - print("reduced-TLS: WARNING firmware.map names no mbedTLS archive", + return + # Every required archive must appear. Seeing only some of them means the rest + # resolved somewhere this parse did not recognise, which is not proof of anything. + missing = [name for name in REQUIRED if name not in seen] + if missing: + print("\n*** reduced-TLS: %s names only %d of %d archives ***" + % (os.path.basename(map_path), len(seen), len(REQUIRED)), file=sys.stderr) + print(" missing: " + ", ".join(missing), file=sys.stderr) + print("Either the map format changed or mbedTLS was resolved elsewhere; " + "the reduced buffers cannot be assumed.", file=sys.stderr) + env.Exit(1) return - print("reduced-TLS: confirmed %d archives linked from %s" - % (len(seen), staged)) + print("reduced-TLS: confirmed all %d archives linked from %s" + % (len(REQUIRED), staged)) env.AddPostAction("$BUILD_DIR/${PROGNAME}.elf", _verify_map) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index da2015969e..d6dcc2b4fb 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -1992,6 +1992,23 @@ void MQTTBridge::teardownSlot(int index) { slot.last_deferred_log_ms = 0; } +// A stopped client needs connect(): reconnect() is a documented no-op on one, so reaching +// it here would strand the slot. The WiFi-transition teardown stops a client while leaving +// initial_connect_done set, so the ladder does see this state. +void MQTTBridge::reconnectSlotClient(int index) { + if (index < 0 || index >= RUNTIME_MQTT_SLOTS) return; + MQTTSlot& slot = _slots[index]; + if (slot.client == nullptr) return; + + if (!slot.client->isStarted()) { + MQTT_DEBUG_PRINTLN("MQTT%d start (client was stopped)", index + 1); + slot.client->connect(); + return; + } + slot.client->reconnect(); +} + + void MQTTBridge::maintainSlotConnections() { if (!_identity) return; @@ -3637,6 +3654,11 @@ void MQTTBridge::requestPublishNeighbors(const char* json, size_t len) { // Drop a new snapshot while one is still being published (Core 0 clears the // flag when done). Acquire pairs with the task loop's release store. if (_neighbors_publish_pending.load(std::memory_order_acquire)) return; + // Allocating here means a stopped bridge must not: a discovery started before the + // stop can finish after it, and releaseRuntimeBuffers() has already run, so the + // allocation would be retained with no task left to consume it. isRunning() is the + // same flag end() guards on. + if (!isRunning()) return; // Allocated on first use so a node with neighbors off never pays the 4 KB. // Cross-core safe: the release store below publishes this pointer, and the task // loop only reads it after the matching acquire load. @@ -3959,7 +3981,9 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { if (createSlotAuthToken(i)) { _slots[i].client->setCredentials(_jwt_username, _slots[i].auth_token); } - _slots[i].client->reconnect(); + // Reuse the transport — the fault is stale credentials, not the transport — + // but via the helper, so a stopped client is started rather than no-opped. + reconnectSlotClient(i); } } } diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index 6056a46a09..cbc8248279 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -447,6 +447,9 @@ class MQTTBridge : public BridgeBase { int activatedSlotCount() const; bool canActivateSlot(int index) const; void teardownSlot(int index); // Disconnect the slot's client (keeps the object alive) + // Reconnect a slot, starting it instead when the client is stopped (reconnect() is a + // no-op on a stopped client). See the definition. + void reconnectSlotClient(int index); void maintainSlotConnections(); // Maintain all slot connections (token renewal, reconnect) void maintainSlotConnection(int index, unsigned long now_millis, unsigned long current_time, bool time_synced, bool& reconnect_attempted, bool& teardown_attempted); bool createSlotAuthToken(int index); // Create/renew JWT token for a slot From f4ba55be7a97fc8f61523b5ec8ea964d268e3e4b Mon Sep 17 00:00:00 2001 From: agessaman Date: Tue, 11 Aug 2026 08:14:39 -0700 Subject: [PATCH 06/10] fix(mqtt): stop bouncing a live waev session to renew its token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit waev's operator confirmed on 2026-08-11 that their servers do not disconnect a client when its JWT passes exp — a 60-minute token can hold a session open for hours. The renewal path assumed the opposite, in as many words: the comment at the bounce called the renewal buffer "the ONLY margin between 'device re-authenticates' and 'broker enforces exp and FIN-closes the session mid-stream' — observed on the waev preset". That premise made waev expensive, because waev is the only preset with a short token_lifetime (3300 s; every other is 0, meaning the 24 h default). It was therefore the only slot bouncing often: measured every ~47 minutes, about 30 times a day per device. And the bounce's re-handshake is where contiguous internal DRAM goes — one renewal traced on hardware took the largest free block from 27,124 to 16,372 B, below the 16,384 B mbedTLS inbound record buffer, after which that slot could not re-handshake at all. The teardown and the credential update cost nothing; the handshake costs everything. So for a broker that leaves live sessions alone, refresh the credentials in place and let the next genuine reconnect use them. That path already existed for the "token renewed but old one still valid" case; this just stops treating imminent expiry as a reason to tear down a healthy connection. mqttPresetEnforcesTokenExp() defaults to true and is keyed by preset name rather than a new struct field: adding a field would mean re-ordering a dozen positional initialisers, where a mistake is silent, and the wrong default costs an outage rather than a re-handshake. Custom and audience-only slots have no preset and are treated as enforcing. Our own logs already argued against the premise and we had not noticed: across 14 multi-device outages (10 hitting all four devices) the drops landed within ~3 s of each other, on devices whose independent boot times gave them independent token issue times. Independent expiries cannot align that tightly, so exp enforcement was never a good explanation for them. Unverified on hardware yet — the operator's statement is second-hand. Next: apply to one board only and confirm the session survives past exp, that a later reconnect still authenticates, and that the ~47-minute 27,124<->16,372 oscillation stops. (cherry picked from commit 27bd05a17b9303b158feec7dab60af2fe128f5ce) --- src/helpers/MQTTPresets.h | 18 ++++++++++++++++++ src/helpers/bridges/MQTTBridge.cpp | 11 ++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/helpers/MQTTPresets.h b/src/helpers/MQTTPresets.h index b5f342c8b5..1083026dcf 100644 --- a/src/helpers/MQTTPresets.h +++ b/src/helpers/MQTTPresets.h @@ -47,6 +47,24 @@ struct MQTTPresetDef { // Braces match topic placeholders ({device}/{iata}); never send this string to the broker. static const char MQTT_USERPASS_USERNAME_PUBKEY[] = "{pubkey}"; +// True when the broker tears down a live session once its JWT passes exp, so the +// renewal must proactively bounce the connection to present a fresh token. +// +// Default true, because getting this wrong the safe way costs a re-handshake and +// getting it wrong the unsafe way costs an outage. waev is the exception: its +// operator confirmed (2026-08-11) that their servers do not disconnect on expiry, +// so a live session there needs only its credentials refreshed for the next +// reconnect. waev is also the only preset with a short token_lifetime, so it was +// the only one bouncing often — every ~47 min, and each bounce's re-handshake can +// cost ~10 KB of contiguous internal DRAM on a non-PSRAM board. +// +// Keyed by name rather than a struct field on purpose: adding a field would mean +// re-ordering a dozen positional initialisers below, where a mistake is silent. +static inline bool mqttPresetEnforcesTokenExp(const MQTTPresetDef* preset) { + if (!preset || !preset->name) return true; // custom/audience slots: assume enforced + return strcmp(preset->name, "waev") != 0; +} + static inline bool mqttPresetUsesDevicePubkeyUsername(const MQTTPresetDef* preset) { return preset && preset->auth_type == MQTT_AUTH_USERPASS && preset->userpass_username && diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index d6dcc2b4fb..906f525aa2 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -2148,7 +2148,16 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns (time_synced && old_token_expires_at >= 1000000000 && current_time >= (old_token_expires_at - renewal_buffer)); - if (old_token_expired_or_imminent || !slot.client->connected()) { + // Only bounce for exp if this broker actually enforces it. A broker that + // leaves live sessions alone past expiry needs the fresh token at the next + // reconnect, not now, and the bounce's re-handshake is where contiguity goes. + const bool exp_forces_bounce = + old_token_expired_or_imminent && mqttPresetEnforcesTokenExp(slot.preset); + if (!exp_forces_bounce && old_token_expired_or_imminent && slot.client->connected()) { + MQTT_DEBUG_PRINTLN("MQTT%d token renewed, no bounce (broker does not enforce exp)", + index + 1); + } + if (exp_forces_bounce || !slot.client->connected()) { // Disconnect + reconnect with fresh credentials, reusing existing client // to avoid internal heap leak/fragmentation from destroy/create cycles MQTT_DEBUG_PRINTLN("MQTT%d token renewal: reconnecting with fresh credentials", index + 1); From 88c824c1755f36386ef0d107dae0a1772699f288 Mon Sep 17 00:00:00 2001 From: agessaman Date: Tue, 11 Aug 2026 18:39:58 -0700 Subject: [PATCH 07/10] docs(mqtt): correct what the WS buffer padding actually fixes The comment claimed the (WS_BUFFER_SIZE + 1) padding makes an oversized upgrade response "fail cleanly (Upgrade header not found)". Reading release/v4.4 transport_ws.c against release/v5.3 shows it does not. v4.4's response loop is } while (NULL == strstr(ws->buffer, "\r\n\r\n") && header_len < WS_BUFFER_SIZE); so it also exits when the buffer fills without the terminator, and the code then looks for "Sec-WebSocket-Accept:" and returns 0 if it is present. That header appears early in a response, so an oversized header block yields a BOGUS SUCCESS rather than a clean failure: the unread remainder stays queued on the socket and is delivered as the first post-upgrade read, where the deframer parses HTTP bytes as a WebSocket frame header. Observed on hardware 2026-08-12 on a Heltec V4: a Cloudflare Page Shield CSP report-uri header pushed the 101 response past the buffer, and the tail of that header ("csp-reporting.cloudflare.com/cdn-cgi/script_monitor/report?") reached the MQTT layer as payload, surfacing as "Invalid MSG_TYPE response: 3" (0x35 = '5', high nibble 3). The padding's real and only value is preventing the one-byte overflow of the heap canary, which is still worth having. Narrow the comment to that claim and record where the parser fix has to come from: IDF 5.2+ requires the "\r\n\r\n" delimiter, memmoves the bytes following it, and fails cleanly when the buffer fills. It cannot be patched here, since transport_ws.c ships precompiled in libtcp_transport.a on Arduino 2.x. No functional change. (cherry picked from commit 9894e65e8ad961704d430a12a065baffda353f50) --- src/helpers/ESP32WsTransportFix.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/helpers/ESP32WsTransportFix.cpp b/src/helpers/ESP32WsTransportFix.cpp index 2e14bb33e9..a69e0b0885 100644 --- a/src/helpers/ESP32WsTransportFix.cpp +++ b/src/helpers/ESP32WsTransportFix.cpp @@ -33,13 +33,24 @@ // Fix: [esp32_base] adds `-Wl,--wrap=esp_transport_ws_init`, so every // creation of a WS transport (esp-mqtt does one per wss slot) is routed // through __wrap_esp_transport_ws_init below, which replaces the freshly -// allocated 1024-byte buffer with a (WS_BUFFER_SIZE + 1)-byte one. The -// out-of-bounds index WS_BUFFER_SIZE then lands on our extra byte and the -// handshake fails cleanly ("Upgrade" header not found) instead of corrupting -// the heap. Upstream fixed this in ESP-IDF 5.x, so this file compiles to a +// allocated 1024-byte buffer with a (WS_BUFFER_SIZE + 1)-byte one, so the +// out-of-bounds index WS_BUFFER_SIZE lands on our extra byte instead of the +// heap canary. Upstream fixed this in ESP-IDF 5.x, so this file compiles to a // pass-through there and can be deleted (together with the --wrap flag) when // the fork moves to Arduino core 3.x. // +// Scope: this stops the heap corruption and NOTHING else. It does not make an +// oversized response fail the handshake — v4.4's read loop also exits on +// `header_len < WS_BUFFER_SIZE` going false, and then still accepts the +// connection if "Sec-WebSocket-Accept:" was inside those first bytes (it comes +// early, so it usually is). ws_connect() therefore returns success on a partial +// header block and the unread remainder arrives as the first "payload" read, +// where the deframer parses HTTP bytes as a frame header. Observed on hardware +// 2026-08-12: a large Cloudflare CSP header produced exactly that, surfacing as +// `Invalid MSG_TYPE response: 3`. Only IDF 5.2+ fixes it (it requires the +// "\r\n\r\n" delimiter, preserves the bytes after it, and fails cleanly when the +// buffer fills); it cannot be patched here because transport_ws.c is precompiled. +// // transport_ws_t below is copied verbatim from ESP-IDF release/v4.4 // transport_ws.c (the struct is file-private, so it is not in any shipped // header). Source fidelity was verified against the shipped binary: addr2line From c0c823b6b004c1d3591376f535764c3576cfea56 Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 12 Aug 2026 10:03:59 -0700 Subject: [PATCH 08/10] fix(mqtt): reuse a still-valid JWT on ordinary reconnects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every ordinary backoff reconnect and every circuit-breaker probe minted a fresh JWT and re-applied credentials, with no check of whether the existing token was still valid. setCredentials() always dirties the esp-mqtt config, so reconnect() then called esp_mqtt_set_config() as well. On a flapping broker that is a signing plus a configuration-copy cycle on every retry, and these observers see ~38 genuine reconnects/day per slot. The no-bounce renewal change (27bd05a1) only stopped the proactive renewal from tearing down a live session; it left this retry path untouched, which is why a soak shows renewals neither firing nor failing for hours while drops continue — each reconnect silently re-mints and pushes the expiry out. Reuse the credentials when their validity is provable and refresh them otherwise. canReuseJwtForReconnect() lives with the other policy predicates so it is host-testable, and it establishes current_time < token_expires_at before subtracting: token_expires_at is unsigned, so an already-expired token would otherwise wrap to ~4e9 seconds and read as valid for decades. The >= kMinimumValidEpoch term also rejects the 0 that a failed renewal writes. Minting stays the default for every uncertain case — unsynced clock, missing or insane expiry, empty token, or an expiry inside kJwtReconnectSafetyMarginSecs (60 s), which covers the handshake itself. Two paths still always mint, deliberately: - The circuit-breaker probe. It is the recovery of last resort for a slot that has already failed repeatedly, quite possibly on auth, and it runs once per 30 minutes — so a fresh token there costs nothing worth counting against keeping that path guaranteed-clean. - Any slot whose last error was a broker refusal. Before this change, minting on every retry accidentally recovered from server-side credential invalidation: key rotation, revocation, broker clock skew, or an audience change after a reconfigure. Reuse would have retried a rejected credential until it neared expiry — up to 24 h for every preset that leaves token_lifetime at the default. onError already detects MQTT_ERROR_TYPE_CONNECTION_REFUSED and only logged it; it now also sets a per-slot force-mint flag, cleared on a successful connect and wherever the credentials it referred to are blanked. The flag is volatile because the esp-mqtt callback sets it and the bridge loop consumes it. The reconnect log line reports the decision and its outcome — REUSE, MINT with a reason, and OK/FAILED for the mint — because a silently failed mint is the case most likely to end in an auth refusal. It never prints the token. Host tests cover the reuse boundary: exact margin, already-expired, expiry 0, sub-epoch expiry, empty token, unsynced clock, and the force-mint override. --- src/helpers/MQTTConnectionPolicy.h | 15 ++++ src/helpers/bridges/MQTTBridge.cpp | 79 +++++++++++++++---- src/helpers/bridges/MQTTBridge.h | 4 + .../test_mqtt_connection_policy.cpp | 16 ++++ 4 files changed, 97 insertions(+), 17 deletions(-) diff --git a/src/helpers/MQTTConnectionPolicy.h b/src/helpers/MQTTConnectionPolicy.h index 0ab4be12b1..ebbf497566 100644 --- a/src/helpers/MQTTConnectionPolicy.h +++ b/src/helpers/MQTTConnectionPolicy.h @@ -17,6 +17,7 @@ static const uint8_t kMaxFailuresAtMaxBackoff = 3; static const uint32_t kDefaultJwtLifetimeSecs = 86400UL; static const uint32_t kMaxJwtStaggerSecs = 300UL; static const uint32_t kMinimumValidEpoch = 1000000000UL; +static const uint32_t kJwtReconnectSafetyMarginSecs = 60UL; static const uint32_t kJwtClockThreshold = 1735689600UL; // 2025-01-01 UTC // A wall clock at or past this instant was set from a real source (NTP or an // admin); anything earlier is the firmware's unset-clock default (1715770351, @@ -157,6 +158,20 @@ static inline bool tokenNeedsRenewal(bool time_synced, uint32_t current_time, return current_time >= token_expires_at - renewal_buffer_secs; } +// A reconnect may keep its credentials only when their validity is known to +// outlast the next handshake; uncertainty refreshes them before reconnecting. +static inline bool canReuseJwtForReconnect(bool time_synced, bool has_token, + bool force_mint, + uint32_t current_time, + uint32_t token_expires_at) { + return time_synced && + has_token && + !force_mint && + token_expires_at >= kMinimumValidEpoch && + current_time < token_expires_at && + (token_expires_at - current_time) > kJwtReconnectSafetyMarginSecs; +} + static inline bool renewalAttemptAllowed(uint32_t now, uint32_t last_attempt) { return elapsedMs(now, last_attempt) >= kRenewalThrottleMs; } diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 906f525aa2..5a4cc917c6 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -704,6 +704,7 @@ MQTTBridge::MQTTBridge(NodePrefs *prefs, MQTTPrefs *obs, mesh::PacketManager *mg _slots[i].last_log_time = 0; _slots[i].port = 1883; _slot_reconfigure_pending[i] = false; + _slot_force_jwt_mint[i] = false; _status_publish_pending[i] = false; } @@ -1601,6 +1602,7 @@ bool MQTTBridge::ensureSlotClient(int index) { slot.client->onConnect([this, index](bool sessionPresent) { MQTT_DEBUG_PRINTLN("MQTT%d connected", index + 1); _slots[index].connected = true; + _slot_force_jwt_mint[index] = false; // NOTE: reconnect_backoff / max_backoff_failures are NOT reset here. // A CONNACK alone doesn't prove the link is healthy — a broker that // accepts and then drops within seconds would reset the ladder every @@ -1647,6 +1649,7 @@ bool MQTTBridge::ensureSlotClient(int index) { _slots[index].last_sock_errno = error.esp_transport_sock_errno; _slots[index].last_error_time = millis(); if (error.error_type == MQTT_ERROR_TYPE_CONNECTION_REFUSED) { + _slot_force_jwt_mint[index] = true; // Broker rejected the MQTT CONNECT itself — not a transport failure. // return code: 1=protocol, 2=client-id rejected, 3=server unavailable, // 4=bad username/password, 5=not authorized. Codes 3/4/5 point at a @@ -1801,6 +1804,8 @@ bool MQTTBridge::setupSlot(int index) { slot.max_backoff_failures = 0; slot.circuit_breaker_tripped = false; slot.last_reconnect_attempt = 0; + // The refusal that set this belonged to the credentials being cleared here. + _slot_force_jwt_mint[index] = false; } bool uses_jwt = (slot.preset && slot.preset->auth_type == MQTT_AUTH_JWT) || slot.audience[0] != '\0'; @@ -1990,6 +1995,8 @@ void MQTTBridge::teardownSlot(int index) { slot.last_reconnect_attempt = 0; slot.last_log_time = 0; slot.last_deferred_log_ms = 0; + // The refusal that set this belonged to the credentials being cleared here. + _slot_force_jwt_mint[index] = false; } // A stopped client needs connect(): reconnect() is a documented no-op on one, so reaching @@ -2203,6 +2210,59 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns // persistent clients (Phase 1), the mbedTLS context is allocated once at // startup and the preflight is no longer necessary. + const auto prepareJwtReconnect = [&](bool force_mint, int backoff_level) { + const bool has_token = slot.auth_token && slot.auth_token[0] != '\0'; + const unsigned long expires_at = slot.token_expires_at; + const bool remaining_known = time_synced && + expires_at >= MQTTConnectionPolicy::kMinimumValidEpoch; + const unsigned long remaining_secs = current_time < expires_at + ? expires_at - current_time + : 0; + const bool force_mint_after_refusal = _slot_force_jwt_mint[index]; + force_mint = force_mint || force_mint_after_refusal; + const bool reuse_token = MQTTConnectionPolicy::canReuseJwtForReconnect( + time_synced, has_token, force_mint, static_cast(current_time), + static_cast(expires_at)); + const char* mint_reason = "none"; + if (!reuse_token) { + if (backoff_level < 0) { + mint_reason = "circuit-breaker-probe"; + } else if (force_mint_after_refusal) { + mint_reason = "connection-refused"; + } else if (!time_synced) { + mint_reason = "clock-unsynced"; + } else if (!has_token) { + mint_reason = "empty-token"; + } else if (expires_at < MQTTConnectionPolicy::kMinimumValidEpoch) { + mint_reason = "invalid-expiry"; + } else if (current_time >= expires_at) { + mint_reason = "expired"; + } else { + mint_reason = "safety-margin"; + } + } + const char* mint_result = reuse_token ? "REUSED" : "FAILED"; + if (!reuse_token && createSlotAuthToken(index)) { + slot.client->setCredentials(_jwt_username, slot.auth_token); + mint_result = "OK"; + } + char remaining_text[24]; + if (remaining_known) { + snprintf(remaining_text, sizeof(remaining_text), "%lus", remaining_secs); + } else { + strncpy(remaining_text, "unknown", sizeof(remaining_text)); + } + if (backoff_level >= 0) { + MQTT_DEBUG_PRINTLN("MQTT%d JWT reconnect backoff=%d token=%s mint_reason=%s result=%s remaining=%s", + index + 1, backoff_level, reuse_token ? "REUSE" : "MINT", mint_reason, + mint_result, remaining_text); + } else { + MQTT_DEBUG_PRINTLN("MQTT%d JWT circuit-breaker probe token=%s mint_reason=%s result=%s remaining=%s", + index + 1, reuse_token ? "REUSE" : "MINT", mint_reason, mint_result, + remaining_text); + } + }; + // Periodic probe for circuit-breaker-tripped slots (recovery from transient outages) // Attempts a single reconnect every 30 minutes to see if the server has come back if (slot.circuit_breaker_tripped && !reconnect_attempted) { @@ -2219,13 +2279,7 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns _radio ? _radio->getRadioState() : -1, (_radio && _radio->getLastRecvMillis() > 0) ? (_ms->getMillis() - _radio->getLastRecvMillis()) : 0); if (slot_uses_jwt) { - // Regenerate or refresh token, then reconnect the persistent client. - // Reaching the ladder at all means setupSlot() ran, so the client object - // and its mbedTLS context are live and no full setup is needed here. - if (createSlotAuthToken(index)) { - slot.client->setCredentials(_jwt_username, slot.auth_token); - MQTT_DEBUG_PRINTLN("MQTT%d circuit breaker probe (fresh token)", index + 1); - } + prepareJwtReconnect(true, -1); slot.client->reconnect(); } else { slot.client->reconnect(); @@ -2259,16 +2313,7 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns reconnect_attempted = true; _last_slot_reconnect_ms = now_millis; if (slot_uses_jwt) { - // Always lightweight reconnect on the persistent client. A stale/expired - // token is handled by regenerating it in place and updating credentials - // — no teardown is needed because the client and its mbedTLS context - // persist for the bridge lifetime. - if (createSlotAuthToken(index)) { - slot.client->setCredentials(_jwt_username, slot.auth_token); - MQTT_DEBUG_PRINTLN("MQTT%d reconnect (fresh token, backoff %d)", index + 1, slot.reconnect_backoff); - } else { - MQTT_DEBUG_PRINTLN("MQTT%d reconnect (token refresh failed, backoff %d)", index + 1, slot.reconnect_backoff); - } + prepareJwtReconnect(false, slot.reconnect_backoff); slot.client->reconnect(); } else { // Non-JWT slots — lightweight reconnect on existing client. diff --git a/src/helpers/bridges/MQTTBridge.h b/src/helpers/bridges/MQTTBridge.h index cbc8248279..bda752a6fe 100644 --- a/src/helpers/bridges/MQTTBridge.h +++ b/src/helpers/bridges/MQTTBridge.h @@ -228,6 +228,10 @@ class MQTTBridge : public BridgeBase { // Pending slot reconfigure: set from CLI (Core 1), processed by MQTT task (Core 0) volatile bool _slot_reconfigure_pending[RUNTIME_MQTT_SLOTS]; + // A broker refusal can invalidate an otherwise clock-valid JWT. The esp-mqtt + // callback sets this and the bridge loop consumes it; byte access is atomic. + volatile bool _slot_force_jwt_mint[RUNTIME_MQTT_SLOTS]; + // Pending on-connect status publish: set from the onConnect callback (which // runs on the esp-mqtt event task, NOT this bridge task), consumed by the MQTT // task (Core 0). publishStatusToSlot() touches the shared status doc/buffer/ diff --git a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp index 496a1c434e..55c58793d7 100644 --- a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp +++ b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp @@ -137,6 +137,22 @@ TEST(MQTTConnectionPolicy, SyncedClockRenewsInvalidExpiredOrImminentTokens) { EXPECT_TRUE(Policy::tokenNeedsRenewal(true, expires + 1U, expires, 300U)); } +TEST(MQTTConnectionPolicy, JwtReconnectReusesOnlyProvenValidCredentials) { + const uint32_t now = 1735689600U; + const uint32_t usable_expiry = now + Policy::kJwtReconnectSafetyMarginSecs + 1U; + + EXPECT_TRUE(Policy::canReuseJwtForReconnect(true, true, false, now, usable_expiry)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect( + true, true, false, now, Policy::kMinimumValidEpoch - 1U)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, true, false, now, 0U)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect( + true, true, false, now, now + Policy::kJwtReconnectSafetyMarginSecs)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, true, false, now, now - 1U)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, false, false, now, usable_expiry)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(false, true, false, now, usable_expiry)); + EXPECT_FALSE(Policy::canReuseJwtForReconnect(true, true, true, now, usable_expiry)); +} + TEST(MQTTConnectionPolicy, RenewalThrottleHasExactBoundaryAndHandlesRollover) { EXPECT_FALSE(Policy::renewalAttemptAllowed(59999U, 0U)); EXPECT_TRUE(Policy::renewalAttemptAllowed(60000U, 0U)); From 6ffadd6e727794e5024d98147136e8b22068359f Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 09:49:16 -0700 Subject: [PATCH 09/10] fix(mqtt): route both reconnect ladders through the stopped-client guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reconnectSlotClient() checks isStarted() and calls connect() instead of reconnect() when the client was stopped, but only the post-NTP stale-token path used it. The ordinary backoff ladder and the circuit-breaker probe called slot.client->reconnect() directly, and esp_mqtt_client_reconnect() is a no-op on a client that is not started. Two ways in. connect() sets _started only when esp_mqtt_client_start() returns ESP_OK while setupSlot() sets initial_connect_done unconditionally, so a start failure under heap pressure stranded the slot. More routinely, the WiFi-drop handler calls disconnect() on every connected slot, which clears _started — after that the ladder issued no-ops forever and the slot never came back. Not caught by the soaks: the log line the guard prints can only come from the NTP path, so a stranded slot and a slot that never entered the state produce identical logs. Observed reconnects were broker-side drops with WiFi up, which leave the client started. The renewal-bounce path keeps its own isStarted() branch — it needs softDisconnect(), which the helper does not do. --- src/helpers/bridges/MQTTBridge.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 5a4cc917c6..7983f1617e 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -2280,10 +2280,10 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns (_radio && _radio->getLastRecvMillis() > 0) ? (_ms->getMillis() - _radio->getLastRecvMillis()) : 0); if (slot_uses_jwt) { prepareJwtReconnect(true, -1); - slot.client->reconnect(); - } else { - slot.client->reconnect(); } + // Via the helper: reconnect() is a no-op on a client the WiFi-drop path + // stopped, which would probe forever without ever starting it. + reconnectSlotClient(index); // If the connect callback fires and sets slot.connected = true, // it will clear circuit_breaker_tripped via the onConnect handler } @@ -2314,12 +2314,13 @@ void MQTTBridge::maintainSlotConnection(int index, unsigned long now_millis, uns _last_slot_reconnect_ms = now_millis; if (slot_uses_jwt) { prepareJwtReconnect(false, slot.reconnect_backoff); - slot.client->reconnect(); } else { // Non-JWT slots — lightweight reconnect on existing client. MQTT_DEBUG_PRINTLN("MQTT%d reconnect (non-JWT, backoff %d)", index + 1, slot.reconnect_backoff); - slot.client->reconnect(); } + // Via the helper: reconnect() is a no-op on a client the WiFi-drop path + // stopped, which would back off forever without ever starting it. + reconnectSlotClient(index); } } } From f64852e223296b36cb75c825af6d1aecdc1636e2 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 09:53:38 -0700 Subject: [PATCH 10/10] fix(mqtt): log a failed client start instead of reporting success connect() logged "MQTT client started." unconditionally, so a failing esp_mqtt_client_start() looked identical to a successful one. That is the one state a later reconnect() cannot recover from, which made it the worst possible line to be wrong. --- lib/PsychicMqttClient/src/PsychicMqttClient.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp index 5d037b7e30..ba1dc5f19d 100644 --- a/lib/PsychicMqttClient/src/PsychicMqttClient.cpp +++ b/lib/PsychicMqttClient/src/PsychicMqttClient.cpp @@ -438,8 +438,13 @@ void PsychicMqttClient::connect() if (start_result == ESP_OK) { _started = true; + ESP_LOGI(TAG, "MQTT client started."); + } + else + { + // Reporting success here hides the one state reconnect() cannot recover from. + ESP_LOGE(TAG, "MQTT client failed to start: %s", esp_err_to_name(start_result)); } - ESP_LOGI(TAG, "MQTT client started."); } void PsychicMqttClient::reconnect()