fix(mqtt): reduce reconnect and renewal handshakes on non-PSRAM observers - #45
Merged
Merged
Conversation
…builds The real Arduino.h includes stdlib.h, so ConfigSerializer.cpp reaches atoi, atol and atof through it and compiles on device. The mock supplied only cstdint, cmath and Stream.h, leaving those undeclared — and since the native env compiles ConfigSerializer.cpp into every suite via build_src_filter, all 21 suites errored rather than just its own. Fixing the mock keeps src/ identical to upstream and covers any other source relying on the same transitive include. pio test -e native: 297 test cases, 297 succeeded.
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 8d1a0eb: 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)
…idge start 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)
…and verified
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/<arch>/, 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)
…en map check
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)
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)
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)
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.
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.
agessaman
force-pushed
the
fix/mqtt-resilience-trimmed
branch
from
August 14, 2026 16:49
e5ddc2f to
6ffadd6
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MQTT reconnect and credential-renewal fixes for the non-PSRAM observer, plus an
opt-in reduced-TLS build. Nine commits; the first seven are soaked on hardware.
What this changes
Route both reconnect ladders through the stopped-client guard (
6ffadd6e) — adefensive fix found by review after the soaks. No observed failure is attributed to it;
see the honest severity note below.
reconnectSlotClient()checksisStarted()and callsconnect()rather thanreconnect()when a 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, andesp_mqtt_client_reconnect()is a no-op on a client that is not started, so a slot in thatstate could never come back.
Two entries, of unequal likelihood.
The live one:
connect()sets_startedonly whenesp_mqtt_client_start()returnsESP_OK, whilesetupSlot()setsinitial_connect_doneunconditionally. A start failure —task allocation under heap pressure, which is not hypothetical on non-PSRAM boards — leaves
_startedfalse andinitial_connect_donetrue, and the ladder then no-ops forever.The narrow one: the WiFi-drop handler calls
disconnect(), which clears_started. Butit only does so for slots still marked
connected:In practice the sockets fault first and the slots self-mark disconnected before the WiFi
handler notices, so
disconnect()is not reached. The one real WiFi outage on the soak rigconfirms this: slots dropped at 02:11:46 on
Poll read error: 119,WiFi disconnected: reason 201was logged 2.5 min later, the AP stayed away 2h44m, and when it returned theladder recovered all three slots unaided — 21 s before an unrelated commanded reboot.
Reaching this entry requires the WiFi handler to win that race, which has not been observed
in 124 h across four boards.
An earlier revision of this description called the WiFi path routine. That was wrong, and
the correction is why this commit is presented as hardening rather than as a bug fix with a
victim.
f64852e2addresses the diagnostic gap that would hide the live entry:connect()logged"MQTT client started."unconditionally, so a failedesp_mqtt_client_start()read as asuccess. It now logs the error name. Worth noting that this lib's
[I]output never reachesserial on these builds while
[E]does, so a start failure was previously invisible ratherthan merely mislabelled.
The renewal-bounce path keeps its own
isStarted()branch — it needssoftDisconnect(),which the helper does not do.
Renew JWT credentials without stopping the esp-mqtt client (
2cf73a62) — the oldpath stopped and restarted the client to pick up fresh credentials, which tore down and
rebuilt the TLS session on every renewal.
Do not bounce a live session at all when the broker ignores token
exp(0f2c0a6e) —mqttPresetEnforcesTokenExp()returns false only forwaev, whose broker does not enforceexpiry. Those slots now renew in place and keep the session; every other preset still
bounces, because their brokers do enforce it.
Reuse a still-valid JWT on ordinary reconnects (
e5ddc2fd) — a reconnect only mints anew token when the current one cannot be proven to outlast the handshake. The predicate is
in
MQTTConnectionPolicy.hwith host tests:The
current_time < token_expires_atterm must stay ahead of the subtraction — these areunsigned, and an expired token would otherwise wrap to a huge remaining lifetime.
Allocate the neighbors JSON buffer on first use (
9c218c4b) — 4 KB was allocated atbridge start whether or not
mqtt.neighborswas enabled.Reconnect robustness (
bd688352) — guards areconnect()on an already-stopped client(a documented no-op, and the WiFi-transition teardown really does produce that state), moves
a late allocation off the hot path, and makes a map lookup fail open.
Opt-in reduced-TLS build (
e75e7ff9) —MESHCORE_REDUCED_TLS=1prepends-Lat astaged mbedTLS with
CONFIG_MBEDTLS_SSL_OUT_CONTENT_LEN=4096, saving ~12 KB of internalDRAM per TLS connection. Off by default; the post-link check verifies every archive resolves
from the staged directory. This also deletes
platformio.local.ini.hold, which arrived onthis branch with the doc commit that has already landed as
c5bf3e99: it hardcodes anabsolute local path, its own header says "not committable", and it uses the
platform_packagesoverride this commit replaces.Inbound stays at 16,384. That is the binding fragmentation constraint and nothing here
addresses it.
Hardware evidence
Soaked on two non-PSRAM Heltec V3 boards, 3 slots each, reduced TLS, MQTT over WSS. The
soaks predate
6ffadd6eandf64852e2; both are compile-verified, and the head is nowrunning on a third board (Heltec V4, 5 slots, PSRAM) as a regression soak.
6ffadd6eis not hardware-provable on this rig. Its pass signal —MQTT<n> start (client was stopped)reaching the log from a ladder — needs the stopped state, and neither entry canbe induced: there is no CLI lever that drops WiFi (
wifi.ssid/wifi.pwdonly write prefs,and nothing calls
WiFi.disconnect()), and forcing anesp_mqtt_client_start()failure needsheap exhaustion at an exact moment. Taking the AP away would most likely reproduce the
observed ordering above and hit neither entry, so a negative result would be uninformative
rather than a failure. Reviewed by inspection instead.
following any of them, and 2 bounces, both on meshmapper — correct, that preset
enforces
exp. 54 reconnects, every one recovered. Contiguity probe 0 FAIL in ~570reads, zero
tls_stack=32512, zero reboots, free-heap drift −128 B.failures, lowest observed remaining lifetime 511 s — well clear of the 60 s margin.
Not yet exercised on hardware: the safety-margin mint (needs a reconnect inside the last
60 s of a token's life) and the force-mint-after-a-refusal guard (
connection refusedhasnever occurred on any board). Both are covered by host tests only.
Scope
This reduces how often a handshake happens. It is not a fragmentation fix — a
non-PSRAM board still reaches the 16,372 B contiguous floor under multi-slot WSS load.
Reaching that floor does not strand a slot: a reconnecting slot frees its own record
buffers first, confirmed by 18 instrumented sub-floor reconnects with no failures.
Also unaffected: the ESP-IDF 4.4 WebSocket header-parser defect that produces
Invalid MSG_TYPE response: 8.transport_ws.cships precompiled, so it is not fixablefrom this shim, and a slot can retry for hours before one attempt succeeds. That work is
tracked separately against an IDF 5.x canary.
Verification
test_mqtt_connection_policycovers the reuse predicate: usable token, sub-epoch, zero,exact margin, already expired, empty token, unsynced clock, forced mint.
The
nativeenvironment could not build when this PR was opened — the Arduino mock did notdeclare
atoi/atol/atof, andConfigSerializer.cppis compiled into every suite, soall 21 errored on the base and on this branch alike. Fixed separately on
observer-firmware-dev(40635a53, test scaffolding only); this branch is rebased ontothat, and the suite runs: 297 test cases, 297 succeeded.
MQTTBridge.cppis not in the native env'sbuild_src_filter, so no host suite covers6ffadd6e. It was verified by buildingHeltec_v3_repeater_observer_mqtt(SUCCESS, flash48.1%, RAM 22.8%). Locking the stranded-slot case down in a host test would need a seam
around the client that does not exist yet.
The predicate was additionally compiled and asserted standalone against this branch's
header — 9/9, including exact-margin rejection and the expired-token unsigned-underflow
case.
Excluded deliberately: the
mqtt.testallocprobe and the non-PSRAM active-slot-capoverride. Both are soak instrumentation and neither belongs in dev.