fix(mqtt): defer the stale-token reconnect when the mint fails

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.
This commit is contained in:
agessaman committed 2026-08-14 19:41:14 -07:00
1 parent 168d4a0a8a
commit 0d12ec7d79
3 files changed
+68 -11

No files matched your search

+23
View File
@@ -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
+13 -11
View File
@@ -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);
}
@@ -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();