fix(mqtt): release nothing after a stop the MQTT task never acknowledged (F01)
The timeout fallback did the two things that cannot be done safely to a task that may still be inside mbedTLS or holding esp-mqtt's API mutex: it deleted that task from the loop task, then force-stopped and deleted the clients it owned, then freed the queue and buffers it can reach. `esp_mqtt_client_stop()` waits on the API mutex and the task's stopped event with no bound, so killing its caller strands the next one; a second stop sees the run flag already clear and returns failure without joining. Nothing in that path established quiescence, and `canFlashAfterStop()` could only withhold OTA afterwards. Changes, all of them about who owns what: - `MQTTLifecycle` gains a `StopUnproven` state (appended, so existing values keep their numbering). `StopTimedOut` now leads there and releases NOTHING; it still fires `ota_release` so the OTA barrier aborts. `acceptsNewWork`, `mayRestart` and `isStopInProgress` are false there, `mayTouchOwnedState` is true, and `end()`'s wait loop still terminates. - `begin()` refuses to start while a stop is unproven. A new start was never proof that the previous clients stopped, and it used to clear the dirty latch that withheld OTA. Because a start is now impossible from `StopUnproven`, clearing that latch on an accepted start is sound. - A late acknowledgement is honoured. The task publishes it only after tearing its clients down, so it proves the same thing whenever it arrives; the deadline is an availability policy, not a statement about what the ack means. `pollLateStopAck()` releases the withheld resources and makes the bridge restartable — a slow stop (a blackholed WSS broker can hold the SDK well past the budget) no longer costs a reboot. - The handshake flags become `std::atomic<bool>` with release/acquire, and the ack moves into the task trampoline immediately before `vTaskDelete(nullptr)`, gated on a `_teardown_complete` flag the loop sets. Published from inside the loop it proved teardown returned, not that the task had stopped executing — and `volatile` ordered none of it. This is the soak campaign's blocker #20, the one item on its list that can corrupt memory. - The force-stop paths are gone with their only caller: `destroySlotClients()` and `teardownSlot()` no longer take a `force` flag. - `get mqtt.status` reports "previous stop unproven, reboot to recover" instead of a bare "not running", latched so it survives end() clearing the singleton. Two lifecycle tests asserted the old contract and are re-encoded, not deleted: a timed-out stop is no longer `Stopped`-and-released, and "repeated failed stops leave the bridge restartable" becomes "repeated *slow* stops recover on their late ack" plus a new case pinning that a never-acknowledged stop stays unusable for the boot. That availability trade is the point of the change.
This commit is contained in:
agessaman committed
1 parent
31d9925b05
commit
415c5d6580
4 files changed
+285
-90
No files matched your search
@@ -174,9 +174,12 @@ TEST(MQTTLifecycle, RestartAfterStop) {
|
||||
|
||||
// --- Timeout / fallback ----------------------------------------------------
|
||||
|
||||
// Handoff: "Timeout/fallback behavior when the MQTT task or client does not
|
||||
// acknowledge." Models the reviewed fallback replacing the abrupt vTaskDelete.
|
||||
TEST(MQTTLifecycle, StopTimeoutFiresReviewedFallback) {
|
||||
// F01: a stop that passes its deadline unacknowledged releases NOTHING. The
|
||||
// MQTT task may still be inside mbedTLS or holding the SDK's API mutex, so the
|
||||
// queue, buffers, clients and the task itself all stay owned. The OTA barrier
|
||||
// is still unblocked — so it can abort — but nothing is freed and the bridge
|
||||
// may not restart.
|
||||
TEST(MQTTLifecycle, StopTimeoutReleasesNothingAndBlocksRestart) {
|
||||
FakeOps ops;
|
||||
L::Coordinator c(ops, kStopTimeoutMs);
|
||||
bringUpToRunning(c);
|
||||
@@ -191,18 +194,70 @@ TEST(MQTTLifecycle, StopTimeoutFiresReviewedFallback) {
|
||||
EXPECT_EQ(L::State::StopRequested, c.state());
|
||||
EXPECT_EQ(0, ops.release_calls);
|
||||
|
||||
// At the deadline the fallback fires: forced release, dirty OTA signal.
|
||||
ops.now = 1000 + kStopTimeoutMs;
|
||||
c.tick();
|
||||
EXPECT_EQ(L::State::Stopped, c.state());
|
||||
|
||||
EXPECT_EQ(L::State::StopUnproven, c.state());
|
||||
EXPECT_TRUE(c.isStopUnproven());
|
||||
EXPECT_TRUE(c.stopTimedOut());
|
||||
EXPECT_EQ(1, ops.release_calls);
|
||||
EXPECT_EQ(0, ops.release_calls) << "an unproven stop must not release";
|
||||
EXPECT_EQ(1, ops.stop_complete_calls);
|
||||
EXPECT_FALSE(ops.last_stop_clean);
|
||||
|
||||
// A late ack after the fallback does not double-release.
|
||||
// Everything the task can reach is still owned, and neither a restart nor a
|
||||
// flash is permitted. A repeated tick does not change that.
|
||||
EXPECT_TRUE(c.mayTouchOwnedState());
|
||||
EXPECT_FALSE(c.mayRestart());
|
||||
EXPECT_FALSE(c.mayBeginFlash());
|
||||
EXPECT_FALSE(c.acceptsNewWork());
|
||||
EXPECT_FALSE(c.isStopInProgress()) << "end()'s wait loop must terminate";
|
||||
|
||||
ops.now += kStopTimeoutMs * 10;
|
||||
c.tick();
|
||||
EXPECT_EQ(L::State::StopUnproven, c.state());
|
||||
EXPECT_EQ(0, ops.release_calls);
|
||||
|
||||
// A start attempt is refused outright: a new start is not proof that the
|
||||
// previous clients stopped.
|
||||
EXPECT_FALSE(c.requestStart());
|
||||
EXPECT_EQ(L::State::StopUnproven, c.state());
|
||||
EXPECT_EQ(1, ops.start_task_calls) << "no second task while unproven";
|
||||
EXPECT_TRUE(c.stopTimedOut()) << "a refused start cannot clear the dirty latch";
|
||||
}
|
||||
|
||||
// The deadline is an availability policy, not a statement about what an ack
|
||||
// means: the task publishes it only after tearing its clients down, so a late
|
||||
// ack is exactly as trustworthy as a timely one. Honouring it releases the
|
||||
// resources and makes the bridge restartable without a reboot.
|
||||
TEST(MQTTLifecycle, LateAckRecoversFromAnUnprovenStop) {
|
||||
FakeOps ops;
|
||||
L::Coordinator c(ops, kStopTimeoutMs);
|
||||
bringUpToRunning(c);
|
||||
|
||||
ops.now = 1000;
|
||||
ASSERT_TRUE(c.requestStop());
|
||||
ops.now = 1000 + kStopTimeoutMs;
|
||||
c.tick();
|
||||
ASSERT_EQ(L::State::StopUnproven, c.state());
|
||||
ASSERT_EQ(0, ops.release_calls);
|
||||
|
||||
ops.now = 1000 + kStopTimeoutMs * 4;
|
||||
ASSERT_TRUE(c.onTaskStopped());
|
||||
|
||||
EXPECT_EQ(L::State::Stopped, c.state());
|
||||
EXPECT_EQ(1, ops.release_calls) << "the proof arrived; release now";
|
||||
EXPECT_TRUE(c.mayRestart());
|
||||
// The OTA barrier already reported dirty and its caller acted on that, so the
|
||||
// flash gate stays shut until a clean start/stop cycle.
|
||||
EXPECT_EQ(1, ops.stop_complete_calls);
|
||||
EXPECT_FALSE(c.mayBeginFlash());
|
||||
EXPECT_TRUE(c.stopTimedOut());
|
||||
|
||||
// Restarting from the proven stop clears the latch, and a second ack is inert.
|
||||
EXPECT_FALSE(c.onTaskStopped());
|
||||
EXPECT_EQ(1, ops.release_calls);
|
||||
ASSERT_TRUE(c.requestStart());
|
||||
EXPECT_FALSE(c.stopTimedOut());
|
||||
}
|
||||
|
||||
TEST(MQTTLifecycle, TickWithoutPendingStopIsNoop) {
|
||||
@@ -371,7 +426,9 @@ TEST(MQTTLifecycle, OtaFlashBlockedUntilCleanStopAcknowledged) {
|
||||
}
|
||||
|
||||
// Handoff OTA barrier: "MQTT stop times out: OTA aborts safely rather than
|
||||
// writing under uncertain ownership."
|
||||
// writing under uncertain ownership." The barrier is still released so the
|
||||
// caller aborts, but flashing stays shut and — unlike the pre-F01 contract —
|
||||
// the bridge cannot be restarted to paper over it.
|
||||
TEST(MQTTLifecycle, OtaAbortsWhenStopTimesOut) {
|
||||
FakeOps ops;
|
||||
L::Coordinator c(ops, kStopTimeoutMs);
|
||||
@@ -382,11 +439,16 @@ TEST(MQTTLifecycle, OtaAbortsWhenStopTimesOut) {
|
||||
ops.now = 500 + kStopTimeoutMs;
|
||||
c.tick();
|
||||
|
||||
EXPECT_EQ(L::State::Stopped, c.state());
|
||||
EXPECT_FALSE(c.mayBeginFlash()); // dirty stop => flashing withheld
|
||||
EXPECT_FALSE(ops.last_stop_clean);
|
||||
EXPECT_EQ(L::State::StopUnproven, c.state());
|
||||
EXPECT_EQ(1, ops.stop_complete_calls); // barrier released...
|
||||
EXPECT_FALSE(ops.last_stop_clean); // ...with "do not flash"
|
||||
EXPECT_FALSE(c.mayBeginFlash());
|
||||
EXPECT_FALSE(c.requestStart()); // and no restart to hide it
|
||||
|
||||
// A fresh clean start/stop cycle clears the latch and re-enables flashing.
|
||||
// Only the task's late acknowledgement makes the bridge usable again, and
|
||||
// only a clean cycle after that re-enables flashing.
|
||||
ASSERT_TRUE(c.onTaskStopped());
|
||||
EXPECT_FALSE(c.mayBeginFlash());
|
||||
ASSERT_TRUE(c.requestStart());
|
||||
EXPECT_FALSE(c.stopTimedOut());
|
||||
ASSERT_TRUE(c.onTaskStarted());
|
||||
@@ -415,8 +477,13 @@ TEST(MQTTLifecycle, NoRestartWhileStopInProgress) {
|
||||
}
|
||||
|
||||
// Handoff OTA barrier: "Repeated failed OTA attempts do not ... leave the
|
||||
// bridge permanently stopped."
|
||||
TEST(MQTTLifecycle, RepeatedFailedStopsLeaveBridgeRestartable) {
|
||||
// bridge permanently stopped." Re-encoded for the F01 contract: what keeps the
|
||||
// bridge usable across repeated slow stops is the task's acknowledgement, not
|
||||
// the deadline. A stop that is merely slow recovers every time; a stop that is
|
||||
// never acknowledged deliberately does NOT, because restarting on top of
|
||||
// clients that may still be running is the failure this state exists to
|
||||
// prevent. That is an availability trade, made knowingly.
|
||||
TEST(MQTTLifecycle, RepeatedSlowStopsRecoverOnTheirLateAck) {
|
||||
FakeOps ops;
|
||||
L::Coordinator c(ops, kStopTimeoutMs);
|
||||
|
||||
@@ -427,19 +494,51 @@ TEST(MQTTLifecycle, RepeatedFailedStopsLeaveBridgeRestartable) {
|
||||
ops.now += 1000;
|
||||
ASSERT_TRUE(c.requestStop());
|
||||
ops.now += kStopTimeoutMs;
|
||||
c.tick(); // times out (dirty)
|
||||
c.tick(); // deadline passes with no ack
|
||||
EXPECT_EQ(L::State::StopUnproven, c.state());
|
||||
EXPECT_FALSE(c.mayRestart()); // not until the task proves it finished
|
||||
EXPECT_EQ(attempt, ops.release_calls);
|
||||
|
||||
ops.now += 1000;
|
||||
ASSERT_TRUE(c.onTaskStopped()); // the slow teardown completes
|
||||
EXPECT_EQ(L::State::Stopped, c.state());
|
||||
EXPECT_TRUE(c.mayRestart()); // never permanently stuck
|
||||
EXPECT_TRUE(c.mayRestart());
|
||||
EXPECT_EQ(attempt + 1, ops.release_calls);
|
||||
}
|
||||
EXPECT_EQ(3, ops.start_task_calls);
|
||||
}
|
||||
|
||||
// A never-acknowledged stop stays unusable for the rest of the boot: no
|
||||
// release, no restart, no flash, however long the owner waits or however many
|
||||
// times it asks.
|
||||
TEST(MQTTLifecycle, NeverAcknowledgedStopStaysUnproven) {
|
||||
FakeOps ops;
|
||||
L::Coordinator c(ops, kStopTimeoutMs);
|
||||
bringUpToRunning(c);
|
||||
|
||||
ASSERT_TRUE(c.requestStop());
|
||||
ops.now += kStopTimeoutMs;
|
||||
c.tick();
|
||||
|
||||
for (int i = 0; i < 5; ++i) {
|
||||
ops.now += 60'000;
|
||||
c.tick();
|
||||
EXPECT_FALSE(c.requestStart());
|
||||
EXPECT_FALSE(c.requestStop());
|
||||
EXPECT_EQ(L::State::StopUnproven, c.state());
|
||||
}
|
||||
EXPECT_EQ(0, ops.release_calls);
|
||||
EXPECT_FALSE(c.mayBeginFlash());
|
||||
EXPECT_TRUE(c.mayTouchOwnedState());
|
||||
}
|
||||
|
||||
// --- Diagnostics -----------------------------------------------------------
|
||||
|
||||
TEST(MQTTLifecycle, StateAndEventNamesAreStable) {
|
||||
EXPECT_STREQ("Stopped", L::stateName(L::State::Stopped));
|
||||
EXPECT_STREQ("Running", L::stateName(L::State::Running));
|
||||
EXPECT_STREQ("StopRequested", L::stateName(L::State::StopRequested));
|
||||
EXPECT_STREQ("StopUnproven", L::stateName(L::State::StopUnproven));
|
||||
EXPECT_STREQ("StartRequested", L::eventName(L::Event::StartRequested));
|
||||
EXPECT_STREQ("StopTimedOut", L::eventName(L::Event::StopTimedOut));
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user