agessaman
b4daf8ca43
Review of the branch found one merge-blocking lifecycle hole and three correctness gaps where the implementation stopped short of contracts the design had already written down. All four are real; each was confirmed against the source (two of them against hardware) before anything changed. **P1 — a quarantined client could still produce a clean bridge stop.** The cooperative teardown set `_teardown_complete` unconditionally, so a slot whose `esp_mqtt_client_stop()` had not completed — deliberately skipped by `destroySlotClients()` and marked Quarantined — still let the trampoline publish the acknowledgement. The owner then freed the queue and buffers and allowed a restart while that SDK task might still be running: exactly the ownership ambiguity StopUnproven exists to remove. The ack is now withheld unless EVERY client is proven stopped, so one unproven client leaves the whole bridge unproven. The rule lives in MQTTClientState.h (`mqttStopMayBeAcknowledged`) with host tests, alongside the state predicates moved out of the bridge. **P2 — the F04 protection did not cover a client that was still connecting.** `softDisconnect()` returns ESP_OK immediately when the client is not connected, so for a slot mid-DNS/TLS/CONNECT it cancelled nothing: the attempt ran on and its CONNECTED event arrived after the new configuration was applied, and with callbacks registered once per client and esp-mqtt events carrying no generation, nothing could tell it from the new attempt's. A reconfigure that lands on a `Starting` client now stops it, joining its SDK task, before applying the new configuration. A Connected client still takes the cheap softDisconnect path, which is where the fragmentation argument applies. One helper (`closeLiveClientForReconfigure`) so the two call sites cannot drift. **P2 — a failed renewal bounce still advanced the effective expiry.** Minting updates `token_expires_at` immediately and the renewal decision read it, so a bounce that failed looked complete: the next pass saw a fresh future expiry and never retried, and clearing `last_token_renewal` re-armed nothing. Slots now carry `applied_token_expires_at` — the expiry of the credential the CONNECTION is using — which only advances when a connect or reconnect has carried it. A failed bounce leaves it on the old credential, so the renewal stays due. **P2 — config-committed and start-accepted were conflated.** `connect()` returned one result for both, so a start that failed after the configuration had committed left `applied_config` describing the previous configuration, and the next recreate-or-reuse decision could reuse a client whose trust policy was not the one it believed. `applyConfig()` is now its own wrapper operation; `applied_config` records the commit, activation records the start. The reconnect ladder resets there too rather than in `teardownSlot()` — the old endpoint's history still applies until a replacement configuration actually commits. Two of my own bugs surfaced on hardware while testing this, both fixed here: - `recreateSlotClient()` called the full `teardownSlot()`, which cleared `broker_uri`, the just-minted token and both expiries out from under a configuration that had already been decided, so a recreate handed the SDK an empty URI and an empty token. It now stops the client and swaps the object, touching nothing else, and the apply step refuses to configure a URI that changed under it rather than passing it on. - `stopSlotClient()` quarantined on any non-OK result, but `ESP_FAIL` from `esp_mqtt_client_stop()` means "client is in invalid state", i.e. not started: there was no task to join, the safest state there is. It was observed quarantining healthy clients on hardware. The case that genuinely cannot be proven is a stop that never RETURNS, which cannot surface here at all — it hangs the task, which is what the bridge-level timeout contains. Hardware (Heltec V4, 5 live slots): a reconfigure landing on a connecting client logs `reconfigure during connect - stopping to cancel the attempt`; a broker holding the CONNACK sees the client close the socket and the disabled slot never connects; `wss`→`mqtt`→`wss` recreate cycles reconnect each way; a blackholed endpoint recovers. 499/499 native tests, four envs clean.
83 lines
3.7 KiB
C
83 lines
3.7 KiB
C
#pragma once
|
|
|
|
#include <stdint.h>
|
|
|
|
// What a slot's MQTT client is doing, as opposed to whether its network is up,
|
|
// and the two decisions that hang off it.
|
|
//
|
|
// Pure logic: no Arduino, esp-mqtt or bridge dependency, so the shutdown
|
|
// contract below is a host test rather than a hardware run.
|
|
//
|
|
// `client->connected()` answers "is the network up" and was being used for "is
|
|
// there anything to stop": a client resolving DNS, negotiating TLS or waiting
|
|
// after a failed CONNECT reports not-connected, so teardown skipped it and left
|
|
// its task running (F04).
|
|
|
|
enum class MqttClientState : uint8_t {
|
|
Absent = 0, // no client object
|
|
Configured, // client allocated, never started
|
|
Starting, // start/reconnect requested, awaiting CONNECTED
|
|
Connected, // CONNECTED received
|
|
Disconnected, // started, no session (our reconnect ladder governs it)
|
|
Stopped, // stop completed; the SDK task is joined and gone
|
|
Quarantined, // stop failed: the SDK task was NOT joined. Never destroy,
|
|
// never reuse, never free anything it still points at.
|
|
};
|
|
|
|
static inline const char* mqttClientStateName(MqttClientState s) {
|
|
switch (s) {
|
|
case MqttClientState::Absent: return "absent";
|
|
case MqttClientState::Configured: return "configured";
|
|
case MqttClientState::Starting: return "starting";
|
|
case MqttClientState::Connected: return "connected";
|
|
case MqttClientState::Disconnected: return "disconnected";
|
|
case MqttClientState::Stopped: return "stopped";
|
|
case MqttClientState::Quarantined: return "quarantined";
|
|
}
|
|
return "?";
|
|
}
|
|
|
|
// The client has been started and not proven stopped: it may still own a task,
|
|
// a socket and a TLS context. Quarantined is deliberately NOT live — it is
|
|
// worse: it may own them and we can never find out.
|
|
static inline bool mqttClientStateIsLive(MqttClientState s) {
|
|
return s == MqttClientState::Starting || s == MqttClientState::Connected ||
|
|
s == MqttClientState::Disconnected;
|
|
}
|
|
|
|
// A connection attempt is in flight, so an event for it can still arrive.
|
|
// Closing the transport is not enough here: PsychicMqttClient::softDisconnect()
|
|
// returns immediately when the client is not yet connected, which means an
|
|
// in-progress DNS/TLS/CONNECT is left to complete on its own. Cancelling one
|
|
// requires a real stop.
|
|
static inline bool mqttClientStateHasAttemptInFlight(MqttClientState s) {
|
|
return s == MqttClientState::Starting;
|
|
}
|
|
|
|
// The SDK task is known to be gone (or never existed), so the object may be
|
|
// destroyed and anything it pointed at may be freed.
|
|
static inline bool mqttClientStateIsProvenStopped(MqttClientState s) {
|
|
return s == MqttClientState::Absent || s == MqttClientState::Configured ||
|
|
s == MqttClientState::Stopped;
|
|
}
|
|
|
|
// The bridge's cooperative stop may only be acknowledged when EVERY client is
|
|
// proven stopped.
|
|
//
|
|
// This is the invariant the whole shutdown contract rests on: the owner treats
|
|
// the acknowledgement as proof that the task destroyed its clients, and only
|
|
// then frees the queue and buffers and allows a restart. One client whose
|
|
// esp_mqtt_client_stop() never completed makes that false — its SDK task may
|
|
// still be running — so the acknowledgement must be withheld and the bridge
|
|
// left in its unproven state, however many other slots stopped cleanly.
|
|
static inline bool mqttStopMayBeAcknowledged(const MqttClientState* states, int count) {
|
|
// An empty set is trivially proven (no slots, nothing to stop); a null array
|
|
// with a nonzero count is a caller bug and must not read as proof.
|
|
if (count <= 0) return true;
|
|
if (states == nullptr) return false;
|
|
for (int i = 0; i < count; i++) {
|
|
if (!mqttClientStateIsProvenStopped(states[i])) return false;
|
|
}
|
|
return true;
|
|
}
|