diff --git a/src/helpers/MQTTConnectionPolicy.h b/src/helpers/MQTTConnectionPolicy.h index ebbf4975..b2c57690 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 adbe0cb2..24553a5d 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 55c58793..7c7bd7de 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();