From 4c90db2199fdc149ee9cb6293e8773edff3f57b8 Mon Sep 17 00:00:00 2001 From: agessaman Date: Wed, 5 Aug 2026 22:29:51 -0700 Subject: [PATCH 01/18] 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 02/18] 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 03/18] 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 04/18] =?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 05/18] 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 06/18] 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 07/18] 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 08/18] 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 09/18] 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() From 2173794966f02977b62d4f85c71cef3a9c7931a3 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 18:41:20 -0700 Subject: [PATCH 10/18] fix(mqtt): reconnect the NTP-corrected slot instead of no-opping on a live client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit esp_mqtt_client_reconnect() is honoured only from MQTT_STATE_WAIT_RECONNECT, so the post-correction path minted a fresh token, staged it, and then asked a connected client to reconnect — a request esp-mqtt refuses. The slot kept running on the token the clock correction had just proven stale, and recovery became broker-driven rather than the clean reconnect this code intends. Split the three states the path can find: a stopped or waiting client goes through reconnectSlotClient() as before, and a live one has its transport closed first. Only where the broker enforces exp, though — waev leaves live sessions alone past expiry, so bouncing it would spend the 16 KiB contiguous handshake that the rest of this branch exists to avoid. Not a regression: the base branch called client->reconnect() directly at the same site. On ESP32 the block is reachable from the WiFi-reconnect resync and the CLI forced sync; the hourly refresh uses refreshNTP(), which does not carry it. --- src/helpers/bridges/MQTTBridge.cpp | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 7983f1617e..a325d0ab38 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -4015,8 +4015,8 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { MQTT_DEBUG_PRINTLN("Time synced: %lu (via %s)", epochTime, ntp_server_used); // If slots are already set up and the time jumped significantly (e.g., SNTP - // initially returned stale RTC time, then a later sync corrected it), tear down - // and re-setup all JWT-authenticated slots so they get fresh tokens. + // initially returned stale RTC time, then a later sync corrected it), re-issue + // credentials for every JWT slot the correction left holding an expired token. if (_slots_setup_done && was_ntp_synced) { unsigned long current_time = (unsigned long)time(nullptr); // Every slot, not _max_active_slots: that is a count of positions, never an @@ -4033,12 +4033,27 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { // in place and reconnect the persistent client. No teardown needed. if (_slots[i].token_expires_at > 0 && current_time > _slots[i].token_expires_at) { MQTT_DEBUG_PRINTLN("MQTT%d token stale after time correction, re-creating", i + 1); - if (createSlotAuthToken(i)) { + const bool minted = createSlotAuthToken(i); + if (minted) { + // Staged only; the config is applied by connect()/reconnect() below. _slots[i].client->setCredentials(_jwt_username, _slots[i].auth_token); } - // 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); + if (!_slots[i].client->connected()) { + // 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); + } else if (minted && mqttPresetEnforcesTokenExp(_slots[i].preset)) { + // esp-mqtt honours reconnect() only from WAIT_RECONNECT, so on a live + // session it is refused and the slot keeps running on the stale token. + // Close the transport first — and only here, where the broker enforces + // exp: elsewhere that handshake buys nothing. + MQTT_DEBUG_PRINTLN("MQTT%d bouncing for the corrected-clock token", i + 1); + _slots[i].client->softDisconnect(); + _slots[i].client->reconnect(); + } else if (minted) { + MQTT_DEBUG_PRINTLN("MQTT%d token re-created, no bounce (broker does not enforce exp)", + i + 1); + } } } } From 74a3df320627d3def10d2913309d5e006484d8a1 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 18:41:29 -0700 Subject: [PATCH 11/18] build(tls): bind the reduced-TLS archives to the framework they were built against MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The manifest said "rebuild these for every espressif32 platform bump" and nothing enforced it. mbedtls_4k.py verified the staged archives against the manifest's own hashes, which proves the pair agrees with itself and nothing more: bump the platform without rebuilding and every check still passes while the link takes mbedTLS built against a different IDF. That fails at runtime on struct-layout drift, not at the link, which is the failure the mechanism claimed to prevent. Fingerprint the framework's own mbedTLS archives — the ones ours displace — as stock: lines in the manifest and check them before the build. If the framework moves, the staged pair is stale by construction and the build stops with the replacement hashes printed ready to paste. Stronger than comparing a version string: framework-arduinoespressif32 versions independently of the platform, and its archives are what actually has to match. The lib directory is resolved by trying the layouts espressif32 has used rather than hardcoding one, and failing closed if none holds all four archives. The fetch script ignores the new lines; its known-arches hint skips them so they cannot be reported as architectures. --- scripts/fetch_mbedtls_4k.sh | 5 +- scripts/mbedtls_4k.py | 91 ++++++++++++++++++++++++++++++--- scripts/mbedtls_4k_manifest.txt | 11 ++++ 3 files changed, 98 insertions(+), 9 deletions(-) diff --git a/scripts/fetch_mbedtls_4k.sh b/scripts/fetch_mbedtls_4k.sh index f1e43c778d..462773e6e1 100755 --- a/scripts/fetch_mbedtls_4k.sh +++ b/scripts/fetch_mbedtls_4k.sh @@ -28,10 +28,13 @@ if [ ! -f "$MANIFEST" ]; then fi # Manifest lines: . Blank lines and # comments ignored. +# The "platform" and "stock:" lines bind the archives to a framework version; +# they are the build check's business, not ours, and are not architectures. 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 + echo "known arches: $(awk '$0 !~ /^#/ && NF && $1 != "platform" && $1 !~ /^stock:/ {print $1}' \ + "$MANIFEST" | sort -u | tr '\n' ' ')" >&2 exit 1 fi diff --git a/scripts/mbedtls_4k.py b/scripts/mbedtls_4k.py index 6718cae425..29cf0e3b8b 100644 --- a/scripts/mbedtls_4k.py +++ b/scripts/mbedtls_4k.py @@ -10,16 +10,21 @@ 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: +Three failure modes this guards against, each of which produces a firmware that looks +fine and is silently wrong: - 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. + - a manifest and archives that agree with each other but not with the installed + framework, which is what a platform bump without a rebuild leaves behind. That + one links cleanly and drifts on struct layout at runtime. + +So the opt-in path verifies every archive by sha256 before the build, checks the +framework's own mbedTLS archives still fingerprint as the ones these were built +against, and after the link re-reads firmware.map to confirm every libmbed*.a came +from our directory. """ Import("env") @@ -54,15 +59,44 @@ def _manifest(project_dir, arch): if not os.path.isfile(path): _fail("missing scripts/mbedtls_4k_manifest.txt") wanted = {} + stock = {} + platform_id = "" 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: + if len(parts) == 2 and parts[0] == "platform": + platform_id = parts[1] + elif len(parts) == 3 and parts[0] == "stock:" + arch: + stock[parts[2]] = parts[1] + elif len(parts) == 3 and parts[0] == arch: wanted[parts[2]] = parts[1] - return wanted + return wanted, stock, platform_id + + +# Where each espressif32 generation keeps the archives we displace. First directory +# holding all four wins, so this resolves without knowing which platform is in play. +FRAMEWORK_LIB_DIRS = ( + ("framework-arduinoespressif32", "tools/sdk/%s/lib"), + ("framework-arduinoespressif32-libs", "%s/lib"), + ("framework-arduinoespressif32", "tools/esp32-arduino-libs/%s/lib"), +) + + +def _framework_lib_dir(platform, arch): + for package, layout in FRAMEWORK_LIB_DIRS: + try: + base = platform.get_package_dir(package) + except Exception: + base = None + if not base: + continue + path = os.path.join(base, *(layout % arch).split("/")) + if all(os.path.isfile(os.path.join(path, name)) for name in REQUIRED): + return path + return None if os.environ.get("MESHCORE_REDUCED_TLS", "") not in ("1", "true", "yes"): @@ -77,7 +111,7 @@ def _manifest(project_dir, arch): if not os.path.isdir(staged): _fail("no archives for %s at %s" % (arch, staged)) -wanted = _manifest(project_dir, arch) +wanted, stock, manifest_platform = _manifest(project_dir, arch) if not wanted: _fail("manifest has no entries for arch '%s'" % arch) @@ -95,6 +129,47 @@ def _manifest(project_dir, arch): % (archive, wanted[name], actual) ) +# Bind the staged archives to the framework they were built against. The hashes above +# only prove the staged files are the ones the manifest names; they say nothing about +# whether the manifest is still current. Bump the platform without rebuilding, and +# every check above still passes while the link takes mbedTLS built against a +# different IDF — a struct-layout drift that corrupts silently at runtime. So +# fingerprint the framework's own archives, the ones being displaced: if those moved, +# the staged pair is stale by construction. +platform = env.PioPlatform() +platform_id = "%s@%s" % (platform.name, platform.version) +stock_dir = _framework_lib_dir(platform, arch) +if stock_dir is None: + _fail("cannot locate the framework's own mbedTLS archives for %s, so the staged " + "ones cannot be tied to a framework version" % arch) +if not stock: + _fail("manifest has no stock:%s fingerprints — it predates the framework binding.\n" + "Add these lines for the framework now installed (%s):\n%s" + % (arch, platform_id, + "\n".join("stock:%s %s %s" % (arch, _sha256(os.path.join(stock_dir, n)), n) + for n in REQUIRED))) + +for name in REQUIRED: + actual = _sha256(os.path.join(stock_dir, name)) + if name not in stock: + _fail("manifest has no stock:%s entry for %s" % (arch, name)) + if actual != stock[name]: + _fail( + "the framework's mbedTLS archives are not the ones these were built against.\n" + " %s\n manifest %s\n installed %s\n" + "Manifest records %s; installed is %s.\n" + "Rebuild the reduced-TLS archives against this framework " + "(docs/mbedtls-tls-footprint.md), then update every hash in the manifest." + % (os.path.join(stock_dir, name), stock[name], actual, + manifest_platform or "no platform", platform_id) + ) + +if manifest_platform and manifest_platform != platform_id: + # Hashes are the check; the version string is orientation. Identical archives + # under a renamed platform are not a compatibility problem. + print("reduced-TLS: manifest says %s, installed is %s — archives match, so this is " + "only a stale label" % (manifest_platform, platform_id)) + # 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]) diff --git a/scripts/mbedtls_4k_manifest.txt b/scripts/mbedtls_4k_manifest.txt index 06c0f38509..5ac608a64f 100644 --- a/scripts/mbedtls_4k_manifest.txt +++ b/scripts/mbedtls_4k_manifest.txt @@ -10,3 +10,14 @@ esp32s3 01629f635b33ffa2c1fdfcd8ac52327cd3a92e8d4e7c9a32d92974e0cfdfe398 libmbed esp32s3 07e7a09847589fefc35bc8d2f739d556535d535acd7185a9824b2af8edd5b05a libmbedtls_2.a esp32s3 e12bcc8d76a368e819987f266e73c265178d6b6675f05d0e29014bb543c402af libmbedtls.a esp32s3 c1e10324e19f6d7763737f72d9032cec832bdfce60c7a7d381a5fcfc7dcad573 libmbedx509.a + +# Framework binding, read by scripts/mbedtls_4k.py and ignored by the fetch script. +# "stock:" lines fingerprint the framework's own archives — the ones the entries above +# displace. They are what makes "rebuild on every platform bump" enforceable instead of +# advisory: if the framework's copies move, the entries above are stale by construction +# and the build stops. Regenerate both sets together, never one alone. +platform espressif32@6.11.0 +stock:esp32s3 abdaf759ee17aa697468427b55a86c6c0082ac4cdeb643a63d8b3ac2df324633 libmbedcrypto.a +stock:esp32s3 9a580d2ff7c885e1bf59479a14422e3d10a485ae9083acb5db943df714c0ac35 libmbedtls_2.a +stock:esp32s3 a0f331245feb5cf4fe8d9f99b03d34d64f10e38d8a8d680e35b34ec82aa3cf9e libmbedtls.a +stock:esp32s3 3d56277224b066118b6c48a973ae18fdf8078448bf54b482ac5557699e52f223 libmbedx509.a From b1ceaf01a82ac7884200bb1bb8393d789b400694 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 19:40:48 -0700 Subject: [PATCH 12/18] fix(mqtt): require real SNTP completion before crediting a fallback server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fallback configured a server, waited 500 ms, and accepted any plausible system clock as proof that server had answered. It usually has not answered that fast — and the device usually already holds valid time, from an earlier sync or the RTC — so the first server in the list was credited unconditionally, the walk stopped there, _last_ntp_sync was refreshed, and an unreachable host was logged as the source. On the `set mqtt.ntp` validation path, where the single-server walk exists so a typo fails fast, that reported a bad server as OK. Poll sntp_get_sync_status() for SNTP_SYNC_STATUS_COMPLETED instead, which is the layer's own statement that a packet arrived. The status is one-shot — reading COMPLETED clears it — so a result left by an earlier sync would latch on the first poll; clear it before the loop. An implausible epoch after a completed sync now moves to the next server rather than spinning out the remaining attempts against a server that has answered. --- src/helpers/bridges/MQTTBridge.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index a325d0ab38..b73ab34bb1 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -21,6 +21,7 @@ #ifdef ESP_PLATFORM #include +#include #include #include #include @@ -3986,15 +3987,26 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { const char* server = servers[s]; MQTT_DEBUG_PRINTLN("SNTP fallback trying %s...", server); configTime(0, 0, server); + // A plausible clock is not evidence this server answered. The device usually + // already holds valid time here — from an earlier sync, or the RTC — so polling + // time(nullptr) declared the very first server successful without a packet ever + // arriving, stopped the fallback walk there, and refreshed _last_ntp_sync. Worse + // on the `set mqtt.ntp` validation path, where a typo is supposed to fail fast. + // Wait for SNTP itself to report completion. The status is one-shot — reading + // COMPLETED clears it — so drop any result an earlier sync left behind. + sntp_set_sync_status(SNTP_SYNC_STATUS_RESET); for (int i = 0; i < 20; i++) { delay(500); + if (sntp_get_sync_status() != SNTP_SYNC_STATUS_COMPLETED) continue; epochTime = (unsigned long)time(nullptr); if (epochTime >= kMinValidEpoch) { ntp_ok = true; ntp_server_used = server; MQTT_DEBUG_PRINTLN("SNTP fallback succeeded on %s: %lu", server, epochTime); - break; + } else { + MQTT_DEBUG_PRINTLN("SNTP fallback: %s synced an implausible epoch %lu", server, epochTime); } + break; } } } From 168d4a0a8a779c2a5099236331d8df352f5c6443 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 19:41:04 -0700 Subject: [PATCH 13/18] fix(mqtt): make the accepted NTP epoch authoritative before any JWT work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit syncTimeWithNTP() read an epoch over UDP, called configTime(), set _ntp_synced, and then had the stale-token test and createSlotAuthToken() read time(nullptr) — without anything having put the accepted epoch there. configTime() restarts SNTP and returns; the clock lands whenever a packet does. _rtc->setCurrentTime() looks like it covers this and does not. AutoDiscoverRTCClock::setCurrentTime() writes a detected DS3231/RV3028/PCF8563/ RX8130CE *instead of* delegating to its fallback, and only that fallback (ESP32RTCClock) calls settimeofday(). So on every board carrying an RTC chip — T-Beam Supreme and Station G3 both compile this bridge and both instantiate AutoDiscoverRTCClock — libc kept the pre-correction time, and the correction path tested staleness and minted iat claims against exactly the clock it had just proven wrong. Boards without a chip take the fallback and were unaffected, which is why the soak rig (Heltec V3/V4, no RTC) never showed it. settimeofday() with the accepted epoch first, so the invariant downstream code already assumes actually holds: once _ntp_synced is true, time(nullptr) returns the epoch we accepted. configTime() still follows, to keep future syncs running. --- src/helpers/bridges/MQTTBridge.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index b73ab34bb1..adbe0cb2ac 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -4013,6 +4013,18 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { #endif if (ntp_ok && ntp_server_used) { + // Take ownership of the system clock here, before anything reads it. configTime() + // only restarts SNTP and returns, and _rtc reaches settimeofday() on exactly one + // path: AutoDiscoverRTCClock writes a detected DS3231/RV3028/PCF8563/RX8130CE chip + // *instead of* its fallback, so on any board carrying one, libc keeps the pre-sync + // time. Everything downstream reads time(nullptr) — the stale-token test below, and + // the iat of every JWT minted from here on — so once _ntp_synced is true that call + // has to already return the epoch we accepted. + struct timeval accepted; + accepted.tv_sec = (time_t)epochTime; + accepted.tv_usec = 0; + settimeofday(&accepted, nullptr); + configTime(0, 0, ntp_server_used); if (_rtc) { From 0d12ec7d796289835090843ce485b9dc7dfedb74 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 19:41:14 -0700 Subject: [PATCH 14/18] fix(mqtt): defer the stale-token reconnect when the mint fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The corrected-clock path reconnected a disconnected slot whether or not createSlotAuthToken() had produced anything, which re-presented the credentials the correction had just invalidated. Minting fails for recoverable reasons — allocation pressure is treated as recoverable elsewhere in this file — so the path is reachable, and the reconnect it spends is one that cannot succeed. Move the decision into MQTTConnectionPolicy as classifyStaleToken(), where the four outcomes are named and host-tested rather than spelled out in nested conditions: Defer on a failed mint, Reconnect a client that is down, Bounce a live session whose broker enforces exp, KeepAlive one whose broker does not. Deferring leaves the slot to the backoff ladder, which mints again on its next attempt. Covers the reviewer's first four cases. The other two — that a completed SNTP sync is required, and that time(nullptr) reflects the accepted epoch before _ntp_synced flips — are inside MQTTBridge.cpp, which the native env does not compile; locking those down needs a seam around the IDF calls that does not exist yet. --- src/helpers/MQTTConnectionPolicy.h | 23 +++++++++++++ src/helpers/bridges/MQTTBridge.cpp | 24 +++++++------- .../test_mqtt_connection_policy.cpp | 32 +++++++++++++++++++ 3 files changed, 68 insertions(+), 11 deletions(-) diff --git a/src/helpers/MQTTConnectionPolicy.h b/src/helpers/MQTTConnectionPolicy.h index ebbf497566..b2c57690a9 100644 --- a/src/helpers/MQTTConnectionPolicy.h +++ b/src/helpers/MQTTConnectionPolicy.h @@ -211,4 +211,27 @@ static inline SlotActivation classifySlotActivation(int slot, const bool* enable : SlotActivation::OverActiveCap; } +// What to do with a slot whose token a clock correction just proved stale. Four +// outcomes, because esp-mqtt accepts a reconnect request only from +// MQTT_STATE_WAIT_RECONNECT: asking a live client to reconnect is refused and +// leaves it running on the stale token, so a live session has to have its +// transport closed first — and that costs a handshake, which is only worth +// spending where the broker actually enforces exp. +enum class StaleTokenAction : uint8_t { + Defer, // no fresh credentials — leave the slot to the backoff ladder + Reconnect, // client is down: start it, or wake one that is waiting + Bounce, // live session the broker will reject: close the transport, then reconnect + KeepAlive, // live session the broker tolerates: stage credentials, keep the handshake +}; + +// A failed mint yields Defer even when the slot is down: reconnecting then would +// re-present the credentials the correction just invalidated. Minting fails for +// recoverable reasons (allocation pressure), and the ladder retries. +static inline StaleTokenAction classifyStaleToken(bool minted, bool connected, + bool broker_enforces_exp) { + if (!minted) return StaleTokenAction::Defer; + if (!connected) return StaleTokenAction::Reconnect; + return broker_enforces_exp ? StaleTokenAction::Bounce : StaleTokenAction::KeepAlive; +} + } // namespace MQTTConnectionPolicy diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index adbe0cb2ac..24553a5d70 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -4057,24 +4057,26 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { // in place and reconnect the persistent client. No teardown needed. if (_slots[i].token_expires_at > 0 && current_time > _slots[i].token_expires_at) { MQTT_DEBUG_PRINTLN("MQTT%d token stale after time correction, re-creating", i + 1); - const bool minted = createSlotAuthToken(i); - if (minted) { - // Staged only; the config is applied by connect()/reconnect() below. - _slots[i].client->setCredentials(_jwt_username, _slots[i].auth_token); + const MQTTConnectionPolicy::StaleTokenAction action = + MQTTConnectionPolicy::classifyStaleToken( + createSlotAuthToken(i), _slots[i].client->connected(), + mqttPresetEnforcesTokenExp(_slots[i].preset)); + if (action == MQTTConnectionPolicy::StaleTokenAction::Defer) { + MQTT_DEBUG_PRINTLN("MQTT%d token refresh failed after time correction, " + "deferring to the reconnect ladder", i + 1); + continue; } - if (!_slots[i].client->connected()) { + // Staged only; the config is applied by connect()/reconnect() below. + _slots[i].client->setCredentials(_jwt_username, _slots[i].auth_token); + if (action == MQTTConnectionPolicy::StaleTokenAction::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); - } else if (minted && mqttPresetEnforcesTokenExp(_slots[i].preset)) { - // esp-mqtt honours reconnect() only from WAIT_RECONNECT, so on a live - // session it is refused and the slot keeps running on the stale token. - // Close the transport first — and only here, where the broker enforces - // exp: elsewhere that handshake buys nothing. + } else if (action == MQTTConnectionPolicy::StaleTokenAction::Bounce) { MQTT_DEBUG_PRINTLN("MQTT%d bouncing for the corrected-clock token", i + 1); _slots[i].client->softDisconnect(); _slots[i].client->reconnect(); - } else if (minted) { + } else { MQTT_DEBUG_PRINTLN("MQTT%d token re-created, no bounce (broker does not enforce exp)", i + 1); } 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 55c58793d7..7c7bd7dee6 100644 --- a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp +++ b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp @@ -258,6 +258,38 @@ TEST(SlotActivation, DisabledAndOutOfRangeSlots) { EXPECT_EQ(SlotActivation::Disabled, Policy::classifySlotActivation(0, nullptr, 3, 2)); } +using Policy::StaleTokenAction; + +TEST(StaleToken, ConnectedSlotOnAnExpEnforcingBrokerBounces) { + // The broker will reject the stale token, and esp-mqtt refuses reconnect() from + // CONNECTED, so the transport has to close first. + EXPECT_EQ(StaleTokenAction::Bounce, + Policy::classifyStaleToken(/*minted=*/true, /*connected=*/true, + /*broker_enforces_exp=*/true)); +} + +TEST(StaleToken, ConnectedSlotOnATolerantBrokerKeepsItsSession) { + // waev leaves live sessions alone past exp. Bouncing would spend a 16 KiB + // contiguous handshake to replace a session the broker was not going to drop. + EXPECT_EQ(StaleTokenAction::KeepAlive, + Policy::classifyStaleToken(true, true, /*broker_enforces_exp=*/false)); +} + +TEST(StaleToken, DisconnectedSlotReconnectsRegardlessOfBrokerPolicy) { + EXPECT_EQ(StaleTokenAction::Reconnect, Policy::classifyStaleToken(true, false, true)); + EXPECT_EQ(StaleTokenAction::Reconnect, Policy::classifyStaleToken(true, false, false)); +} + +TEST(StaleToken, FailedMintNeverReconnects) { + // Reconnecting here would re-present the credentials the correction invalidated. + // Every combination defers — including the disconnected one, which is the case + // that previously reconnected on the stale token. + EXPECT_EQ(StaleTokenAction::Defer, Policy::classifyStaleToken(false, false, true)); + EXPECT_EQ(StaleTokenAction::Defer, Policy::classifyStaleToken(false, false, false)); + EXPECT_EQ(StaleTokenAction::Defer, Policy::classifyStaleToken(false, true, true)); + EXPECT_EQ(StaleTokenAction::Defer, Policy::classifyStaleToken(false, true, false)); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); From 1a01344e717b79b90c278b316616fb9c67d32f24 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 19:43:18 -0700 Subject: [PATCH 15/18] fix(mqtt): keep the usable-clock fallback the SNTP strictness removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring a real SNTP completion took away something the plausible-clock test was doing by accident. _ntp_synced gates slot setup outright (:1386, :2894), so a device that cannot reach NTP now brings up no slots at all — and a network that blocks UDP/123 while allowing 443 is an ordinary firewall configuration, not a corner case. An RTC-backed observer there used to stay synced and keep minting JWTs against a perfectly good clock. Accept the existing clock explicitly when every server has failed, logged as what it is rather than as a claim about a server that never replied. Excluded from the `set mqtt.ntp` validation path, where the question is whether that server works and the clock cannot answer it. configTime() is now called only when a server did answer, since otherwise there is nothing new to point SNTP at. --- src/helpers/bridges/MQTTBridge.cpp | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 24553a5d70..1a3c2d1550 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -4012,7 +4012,24 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { } #endif - if (ntp_ok && ntp_server_used) { + // No server answered, but the clock itself may still be usable. Requiring a real + // SNTP completion above removed something the plausible-clock test was doing by + // accident: an RTC-backed device on a network that blocks NTP (UDP/123) while + // allowing the broker (443) stayed synced and kept minting JWTs. _ntp_synced gates + // slot setup outright, so losing that strands those deployments with no slots at + // all. Keep the behaviour, but as its own decision rather than as a claim about a + // server that never replied. Not on the validation path — `set mqtt.ntp` asks + // whether that server works, and the clock cannot answer for it. + if (!ntp_ok && !primary_only) { + unsigned long existing = (unsigned long)time(nullptr); + if (existing >= kMinValidEpoch) { + epochTime = existing; + ntp_ok = true; + MQTT_DEBUG_PRINTLN("No NTP server answered; continuing on the existing clock: %lu", existing); + } + } + + if (ntp_ok) { // Take ownership of the system clock here, before anything reads it. configTime() // only restarts SNTP and returns, and _rtc reaches settimeofday() on exactly one // path: AutoDiscoverRTCClock writes a detected DS3231/RV3028/PCF8563/RX8130CE chip @@ -4025,7 +4042,11 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { accepted.tv_usec = 0; settimeofday(&accepted, nullptr); - configTime(0, 0, ntp_server_used); + // Only when a server actually answered: there is nothing to point SNTP at + // otherwise, and the existing configuration is the best guess available. + if (ntp_server_used) { + configTime(0, 0, ntp_server_used); + } if (_rtc) { _rtc->setCurrentTime(epochTime); @@ -4036,7 +4057,8 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { _last_ntp_sync = millis(); sync_in_progress = false; - MQTT_DEBUG_PRINTLN("Time synced: %lu (via %s)", epochTime, ntp_server_used); + MQTT_DEBUG_PRINTLN("Time synced: %lu (via %s)", epochTime, + ntp_server_used ? ntp_server_used : "existing clock"); // If slots are already set up and the time jumped significantly (e.g., SNTP // initially returned stale RTC time, then a later sync corrected it), re-issue From ee2f866b104bcf19d61facd4a36c3f9fa3f7ceb6 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 19:58:39 -0700 Subject: [PATCH 16/18] fix(mqtt): clear the stale SNTP status before starting the new request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reset was on the wrong side of configTime(). configTime() configures the server, calls sntp_init(), and returns — the new request is live before it comes back — so a fast reply could set SNTP_SYNC_STATUS_COMPLETED inside that call, and the reset immediately after would erase it. The following ten seconds of polling would then see nothing and reject a server that had in fact answered. On the `set mqtt.ntp` path that surfaces as a good server failing validation. Stop any running session first, discard its status, then start the new one, so the only completion observable is the one being waited for. --- src/helpers/bridges/MQTTBridge.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 1a3c2d1550..4da8714971 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -3986,15 +3986,21 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { for (int s = 0; s < server_count && !ntp_ok; s++) { const char* server = servers[s]; MQTT_DEBUG_PRINTLN("SNTP fallback trying %s...", server); - configTime(0, 0, server); // A plausible clock is not evidence this server answered. The device usually // already holds valid time here — from an earlier sync, or the RTC — so polling // time(nullptr) declared the very first server successful without a packet ever // arriving, stopped the fallback walk there, and refreshed _last_ntp_sync. Worse // on the `set mqtt.ntp` validation path, where a typo is supposed to fail fast. // Wait for SNTP itself to report completion. The status is one-shot — reading - // COMPLETED clears it — so drop any result an earlier sync left behind. + // COMPLETED clears it — so drop any result an earlier sync left behind, and do + // that *before* starting this one: configTime() returns after sntp_init(), so a + // fast reply can complete inside it, and clearing afterwards would erase the + // very result being waited for. + if (sntp_enabled()) { + sntp_stop(); + } sntp_set_sync_status(SNTP_SYNC_STATUS_RESET); + configTime(0, 0, server); for (int i = 0; i < 20; i++) { delay(500); if (sntp_get_sync_status() != SNTP_SYNC_STATUS_COMPLETED) continue; From 436bb65ae611a5fb62ba14412dfd5e6afa978591 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 19:58:50 -0700 Subject: [PATCH 17/18] fix(mqtt): consult the RTC when libc cannot vouch for the fallback clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The usable-clock fallback asked libc only, which does not answer for the case it was written to cover. On a cold boot ESP32RTCClock::begin() stamps libc with a 2024 placeholder on power-on; AutoDiscoverRTCClock::begin() probes the chip but never copies its time across, and getCurrentTime() reads the chip directly. So a Station G3 or T-Beam Supreme that knows exactly what time it is, on a network with UDP/123 blocked, still failed the plausibility test, left _ntp_synced false, and brought up no slots — precisely the deployment the fallback exists for. Ask the RTC when libc is below the floor. libc still wins when it is usable: a clock SNTP set recently outranks a chip that may have drifted. Accepting the RTC value then flows through the same block, so settimeofday() repairs libc and the epoch is written back to the chip. The choice is chooseFallbackClock() in MQTTConnectionPolicy, host-tested across the four states including the power-on placeholder and the exact floor. Also corrects the previous commit's claim that configTime() is called only when a server replied — the fallback necessarily points it at each server before knowing that; it is the post-acceptance call that is now conditional. --- src/helpers/MQTTConnectionPolicy.h | 21 ++++++++++ src/helpers/bridges/MQTTBridge.cpp | 25 +++++++---- .../test_mqtt_connection_policy.cpp | 42 +++++++++++++++++++ 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/src/helpers/MQTTConnectionPolicy.h b/src/helpers/MQTTConnectionPolicy.h index b2c57690a9..096c5d38e5 100644 --- a/src/helpers/MQTTConnectionPolicy.h +++ b/src/helpers/MQTTConnectionPolicy.h @@ -234,4 +234,25 @@ static inline StaleTokenAction classifyStaleToken(bool minted, bool connected, return broker_enforces_exp ? StaleTokenAction::Bounce : StaleTokenAction::KeepAlive; } +// Which clock to fall back on when no NTP server answered. +enum class ClockSource : uint8_t { + None, // nothing plausible to work from — stay unsynced + System, // libc already holds a usable time + Rtc, // libc does not, but the RTC does +}; + +// System first: a clock SNTP set recently outranks an RTC that may have drifted. +// The RTC matters on a cold boot, where ESP32RTCClock::begin() seeds libc with a 2024 +// placeholder on power-on while a detected chip already holds real time and +// AutoDiscoverRTCClock::begin() never copies one into the other. Never while +// validating a server: that asks whether a specific host answers, and no clock can +// answer it. Pass rtc_time 0 when the board has no clock to consult. +static inline ClockSource chooseFallbackClock(bool validating_server, uint32_t system_time, + uint32_t rtc_time, uint32_t min_valid_epoch) { + if (validating_server) return ClockSource::None; + if (system_time >= min_valid_epoch) return ClockSource::System; + if (rtc_time >= min_valid_epoch) return ClockSource::Rtc; + return ClockSource::None; +} + } // namespace MQTTConnectionPolicy diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 4da8714971..7afb2e28d0 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -4026,12 +4026,21 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { // all. Keep the behaviour, but as its own decision rather than as a claim about a // server that never replied. Not on the validation path — `set mqtt.ntp` asks // whether that server works, and the clock cannot answer for it. - if (!ntp_ok && !primary_only) { - unsigned long existing = (unsigned long)time(nullptr); - if (existing >= kMinValidEpoch) { - epochTime = existing; + if (!ntp_ok) { + const unsigned long system_time = (unsigned long)time(nullptr); + // On a cold boot with a detected RTC chip these disagree: ESP32RTCClock::begin() + // stamps libc with a 2024 placeholder on power-on, AutoDiscoverRTCClock::begin() + // never copies the chip into it, and getCurrentTime() reads the chip. Asking libc + // alone would reject a board that knows exactly what time it is. + const unsigned long rtc_time = _rtc ? (unsigned long)_rtc->getCurrentTime() : 0; + const MQTTConnectionPolicy::ClockSource source = MQTTConnectionPolicy::chooseFallbackClock( + primary_only, (uint32_t)system_time, (uint32_t)rtc_time, (uint32_t)kMinValidEpoch); + if (source != MQTTConnectionPolicy::ClockSource::None) { + const bool from_rtc = (source == MQTTConnectionPolicy::ClockSource::Rtc); + epochTime = from_rtc ? rtc_time : system_time; ntp_ok = true; - MQTT_DEBUG_PRINTLN("No NTP server answered; continuing on the existing clock: %lu", existing); + MQTT_DEBUG_PRINTLN("No NTP server answered; continuing on the existing %s: %lu", + from_rtc ? "RTC" : "system clock", epochTime); } } @@ -4048,8 +4057,10 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { accepted.tv_usec = 0; settimeofday(&accepted, nullptr); - // Only when a server actually answered: there is nothing to point SNTP at - // otherwise, and the existing configuration is the best guess available. + // Only when a server supplied the accepted epoch. The fallback above necessarily + // points configTime() at each server before knowing whether it replies; this is + // the post-acceptance call, and there is nothing to re-point it at when the epoch + // came from a local clock. if (ntp_server_used) { configTime(0, 0, ntp_server_used); } 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 7c7bd7dee6..c3cd469e4c 100644 --- a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp +++ b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp @@ -290,6 +290,48 @@ TEST(StaleToken, FailedMintNeverReconnects) { EXPECT_EQ(StaleTokenAction::Defer, Policy::classifyStaleToken(false, true, false)); } +using Policy::ClockSource; + +// 2026-01-01, the bridge's plausibility floor, and the 2024 placeholder +// ESP32RTCClock::begin() stamps into libc on a power-on reset. +static const uint32_t kFloor = 1767225600; +static const uint32_t kPowerOnPlaceholder = 1715770351; +static const uint32_t kPlausibleNow = 1786000000; + +TEST(FallbackClock, PrefersTheSystemClockWhenItIsUsable) { + // A clock SNTP set recently outranks an RTC that may have drifted. + EXPECT_EQ(ClockSource::System, + Policy::chooseFallbackClock(false, kPlausibleNow, kPlausibleNow - 900, kFloor)); +} + +TEST(FallbackClock, FallsBackToTheRtcOnAColdBoot) { + // The case the system-clock-only check missed: libc holds the power-on + // placeholder while a detected chip holds real time. + EXPECT_EQ(ClockSource::Rtc, + Policy::chooseFallbackClock(false, kPowerOnPlaceholder, kPlausibleNow, kFloor)); +} + +TEST(FallbackClock, NothingUsableStaysUnsynced) { + EXPECT_EQ(ClockSource::None, + Policy::chooseFallbackClock(false, kPowerOnPlaceholder, kPowerOnPlaceholder, kFloor)); + // rtc_time 0 is how a board with no clock to consult is passed in. + EXPECT_EQ(ClockSource::None, + Policy::chooseFallbackClock(false, kPowerOnPlaceholder, 0, kFloor)); +} + +TEST(FallbackClock, ServerValidationNeverAcceptsALocalClock) { + // `set mqtt.ntp` asks whether that host answers. No clock can answer for it, + // however plausible — this is the path where a typo has to fail. + EXPECT_EQ(ClockSource::None, + Policy::chooseFallbackClock(true, kPlausibleNow, kPlausibleNow, kFloor)); +} + +TEST(FallbackClock, TheFloorItselfIsAccepted) { + EXPECT_EQ(ClockSource::System, Policy::chooseFallbackClock(false, kFloor, 0, kFloor)); + EXPECT_EQ(ClockSource::Rtc, Policy::chooseFallbackClock(false, kFloor - 1, kFloor, kFloor)); + EXPECT_EQ(ClockSource::None, Policy::chooseFallbackClock(false, kFloor - 1, kFloor - 1, kFloor)); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); From 9597d9ca5253c444518f726762fda0ce57ea4c9d Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 14 Aug 2026 20:55:23 -0700 Subject: [PATCH 18/18] fix(mqtt): do not send an NTP request to a name that failed to resolve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found on hardware while validating the SNTP fix. `set mqtt.ntp bogus.invalid` reported SUCCESS with a correct epoch, in 4 s, with no retry and without ever reaching the SNTP fallback: [E] hostByName(): DNS Failed for bogus.invalid [E] beginPacket(): could not get host from dns: 11 MQTT: Time synced: 1786764354 (via bogus.invalid) Three pieces compose it. WiFiUDP::beginPacket(const char*, port) returns 0 on a DNS failure and leaves remote_ip/remote_port at their previous values. NTPClient::sendNTPPacket() discards that return and calls endPacket() regardless. endPacket() sends to whatever remote_ip still holds. So the request went to the pool address resolved at boot, that server answered with a genuine timestamp, and the loop recorded ntp_server_used as the name that had never been contacted. This sits one layer above the fallback that b1ceaf01 made honest — control never reaches it — so `set mqtt.ntp `, whose whole purpose is to fail fast, still reported OK and the fleet kept a server name it had never spoken to. The DNS pre-check was already here and only logged a warning. Make it decide: skip a name that does not resolve rather than attempt a send that cannot go where it claims. IP literals are unaffected — hostByName() returns them via fromString() without a lookup — and the lookup already ran, so no latency is added. Moved setPoolServerName() below it so the client is never pointed at a server being skipped. Residual, narrower window: our lookup succeeds and NTPClient's own gethostbyname() then fails, which needs the entry to leave the lwIP cache between two calls microseconds apart. Closing it properly needs the resolved IP handed to NTPClient, and this version exposes no setPoolServerIP(); the constructor is the only way in. Not host-testable — NTPClient and WiFiUDP both. Verified by inspection of both library sources plus the captured hardware trace above. --- src/helpers/bridges/MQTTBridge.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index 7afb2e28d0..1ce0f1bbaf 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -3954,15 +3954,25 @@ bool MQTTBridge::syncTimeWithNTP(bool force, bool primary_only) { const int kMaxNtpRetriesPerServer = 2; for (int s = 0; s < server_count && !ntp_ok; s++) { const char* server = servers[s]; - _ntp_client.setPoolServerName(server); #ifdef ESP_PLATFORM + // Authoritative, not advisory. NTPClient::sendNTPPacket() ignores what + // beginPacket() returns, and WiFiUDP leaves remote_ip/remote_port at the previous + // destination when a name fails to resolve — so asking an unresolvable host sends + // the request to whichever server resolved last, and that server's genuine reply + // gets credited to this name. Observed on d4: `set mqtt.ntp bogus.invalid` reported + // success with a correct epoch, answered by the pool address left over from boot. + // Skipping is what keeps the credit honest; the name that answered is the name + // recorded. IPAddress resolved_ip; if (!WiFi.hostByName(server, resolved_ip)) { - MQTT_DEBUG_PRINTLN("WARNING: DNS resolution failed for %s - NTP sync may fail", server); + MQTT_DEBUG_PRINTLN("NTP: %s does not resolve — skipping, not attempting a send", server); + continue; } #endif + _ntp_client.setPoolServerName(server); + for (int attempt = 1; attempt <= kMaxNtpRetriesPerServer && !ntp_ok; attempt++) { if (attempt > 1) { MQTT_DEBUG_PRINTLN("NTP retry %d/%d on %s...", attempt, kMaxNtpRetriesPerServer, server);