diff --git a/STABILITY_TESTABILITY_HANDOFF.md b/STABILITY_TESTABILITY_HANDOFF.md index 09a0b214..0128c0e6 100644 --- a/STABILITY_TESTABILITY_HANDOFF.md +++ b/STABILITY_TESTABILITY_HANDOFF.md @@ -30,7 +30,7 @@ index. | 4 | Ownership and teardown test seams | Seams + ownership doc + teardown tests done; production rewiring deferred to Phase 5 | | 5 | Cooperative MQTT shutdown | Minimal cooperative `end()` + `begin()` guard + OTA barrier implemented on branch `phase5/cooperative-mqtt-shutdown` (native green, firmware smoke build green); NOT hardware-validated. Volatile-handshake replacement + snapshot-consumer repointing deferred | | — | OTA teardown barrier | Implemented — flash gated on a clean MQTT stop in `simple_repeater`; not hardware-validated | -| 6 | Request/queue/connection/publication integration tests | Not started | +| 6 | Request/queue/connection/publication integration tests | Partial: WiFi-backoff + publish-outcome + enum-alignment gaps extracted and host-tested on branch `phase6/integration-tests`; WebConfig batch/reboot/stop state machine and queue-orchestration coverage still open | | 7 | Uptime, memory, and fault-injection gates | Not started | Forward plan, in execution order: **Phase 0 → Phase 4 → Phase 5 (with the OTA @@ -454,7 +454,26 @@ Acceptance criteria: ### Phase 6: Expand request, queue, connection, and publication integration tests -**Status: Not started.** Depends on Phase 4/5 lifecycle ownership being stable. +**Status: Partial — branch `phase6/integration-tests` (draft PR, base `phase5`).** +The remaining *inline* decision points that were host-testable have been extracted +into the pure policy seams and covered: + +- WiFi STA reconnect backoff moved out of `handleWiFiConnection()` into + `MQTTConnectionPolicy::{wifiReconnectBackoffMs,wifiReconnectDue, + nextWifiBackoffAttempt}` (behavior-preserving; adversarially reviewed for + rollover/boundary equivalence) with `test_mqtt_connection_policy` cases. +- The (packet, raw) publication-outcome pairing named as + `MQTTPacketQueuePolicy::queuedPacketPublished()` and wired at both queue-drain + sites, with `test_mqtt_packet_queue_policy` cases (partial success = completed). +- `MQTTPublicationType` values frozen in `test_mqtt_topic_router`; the + bridge-side `MQTTMessageType` alignment was already a compile-time `static_assert`. + +Still open (each a good follow-up PR): the **WebConfig POST/result/reboot/stop +state machine** (largest gap — all inline in `WebConfigServer.cpp`; natural to +extract a pure `WebConfigBatch` seam mirroring `MQTTLifecycle.h`) and the +**queue-orchestration** behaviors (FIFO ordering, evict/requeue-failure interplay, +the two adapters' drop-vs-keep-head divergence), which need a fake-queue harness. +The original scope list follows. After lifecycle ownership is stable, broaden deterministic integration coverage: diff --git a/src/helpers/MQTTConnectionPolicy.h b/src/helpers/MQTTConnectionPolicy.h index 42e263a9..72dfae64 100644 --- a/src/helpers/MQTTConnectionPolicy.h +++ b/src/helpers/MQTTConnectionPolicy.h @@ -86,6 +86,35 @@ static inline bool circuitBreakerProbeDue(uint32_t now, uint32_t last_attempt) { return elapsedMs(now, last_attempt) >= kCircuitBreakerProbeMs; } +// WiFi station reconnect backoff. The bridge drives its own STA reconnect loop +// separate from the per-slot MQTT reconnects, with a slightly longer first rung +// (15 s vs the slot ladder's 10 s). Extracted from handleWiFiConnection() so the +// ladder and its wrap-safe timing are exercised by host tests instead of a +// second inline copy of the backoff math. +static inline uint32_t wifiReconnectBackoffMs(uint8_t attempt) { + static const uint32_t kBackoffMs[] = { + 15000UL, 30000UL, 60000UL, 120000UL, 300000UL + }; + const uint8_t index = attempt < 5 ? attempt : 4; + return kBackoffMs[index]; +} + +// A reconnect is due only once the link has been down for the current rung AND +// no attempt has been made within that rung (both measured wrap-safely). This +// mirrors the two-part guard the bridge applied inline. +static inline bool wifiReconnectDue(uint32_t now, uint32_t disconnected_since, + uint32_t last_attempt, uint8_t attempt) { + const uint32_t delay = wifiReconnectBackoffMs(attempt); + return elapsedMs(now, disconnected_since) >= delay && + elapsedMs(now, last_attempt) >= delay; +} + +// The attempt counter climbs to 5 and then saturates; the index clamp in +// wifiReconnectBackoffMs() holds it at the 300 s rung. +static inline uint8_t nextWifiBackoffAttempt(uint8_t attempt) { + return attempt < 5 ? static_cast(attempt + 1) : attempt; +} + // Each later slot expires up to five percent of the base lifetime earlier, // capped at five minutes per slot. Runtime slot indexes are bounded by the // persisted MQTT slot count; the final clamp also prevents underflow if this diff --git a/src/helpers/MQTTPacketQueuePolicy.h b/src/helpers/MQTTPacketQueuePolicy.h index 74eabf47..4272d4c8 100644 --- a/src/helpers/MQTTPacketQueuePolicy.h +++ b/src/helpers/MQTTPacketQueuePolicy.h @@ -87,6 +87,17 @@ struct RetryDecision { uint32_t next_retry_ms; }; +// A queued packet counts as delivered if EITHER its structured-packet publish +// or its raw-frame publish reached at least one slot. Partial success (one +// succeeds while the other fails or was not attempted) is still success — the +// packet completes and is not retried. This is the (packet, raw) outcome pairing +// fed to retryDecision(); naming it keeps the "partial publish = done" contract +// explicit and host-tested rather than inline in the bridge's queue drain. +static inline bool queuedPacketPublished(bool packet_published, + bool raw_published) { + return packet_published || raw_published; +} + static inline RetryDecision retryDecision(bool any_published, uint8_t retry_attempts, uint32_t now) { diff --git a/src/helpers/bridges/MQTTBridge.cpp b/src/helpers/bridges/MQTTBridge.cpp index af241ee5..5016505a 100644 --- a/src/helpers/bridges/MQTTBridge.cpp +++ b/src/helpers/bridges/MQTTBridge.cpp @@ -2334,18 +2334,17 @@ bool MQTTBridge::handleWiFiConnection(unsigned long now) { } } } else if (_wifi_disconnected_time > 0) { - unsigned long disconnected_duration = now - _wifi_disconnected_time; - static const unsigned long WIFI_BACKOFF_MS[] = { 15000, 30000, 60000, 120000, 300000 }; - unsigned int idx = (_wifi_reconnect_backoff_attempt < 5) ? _wifi_reconnect_backoff_attempt : 4; - unsigned long delay_ms = WIFI_BACKOFF_MS[idx]; - unsigned long elapsed_since_attempt = (now >= _last_wifi_reconnect_attempt) - ? (now - _last_wifi_reconnect_attempt) - : (ULONG_MAX - _last_wifi_reconnect_attempt + now + 1); - if (disconnected_duration >= delay_ms && elapsed_since_attempt >= delay_ms) { + // Backoff ladder + wrap-safe timing live in MQTTConnectionPolicy (Phase 6), + // exercised by host tests. Behavior is unchanged: both the link-down + // duration and the since-last-attempt interval must clear the current rung + // (elapsedMs is the wrap-safe form of the old ULONG_MAX branch). + if (MQTTConnectionPolicy::wifiReconnectDue( + (uint32_t)now, (uint32_t)_wifi_disconnected_time, + (uint32_t)_last_wifi_reconnect_attempt, + _wifi_reconnect_backoff_attempt)) { _last_wifi_reconnect_attempt = now; - if (_wifi_reconnect_backoff_attempt < 5) { - _wifi_reconnect_backoff_attempt++; - } + _wifi_reconnect_backoff_attempt = + MQTTConnectionPolicy::nextWifiBackoffAttempt(_wifi_reconnect_backoff_attempt); WiFi.disconnect(); WiFi.begin(_obs->wifi_ssid, _obs->wifi_password); } @@ -2664,7 +2663,7 @@ void MQTTBridge::processPacketQueue() { raw_published = publishRaw(&queued.packet_copy); } - bool any_published = packet_published || raw_published; + bool any_published = MQTTPacketQueuePolicy::queuedPacketPublished(packet_published, raw_published); const MQTTPacketQueuePolicy::RetryDecision retry = MQTTPacketQueuePolicy::retryDecision( any_published, queued.retry_attempts, @@ -2790,7 +2789,7 @@ void MQTTBridge::processPacketQueue() { raw_published = publishRaw(&queued.packet_copy); } - bool any_published = packet_published || raw_published; + bool any_published = MQTTPacketQueuePolicy::queuedPacketPublished(packet_published, raw_published); const MQTTPacketQueuePolicy::RetryDecision retry = MQTTPacketQueuePolicy::retryDecision( any_published, queued.retry_attempts, 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 fd6fbe0b..d09f5e30 100644 --- a/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp +++ b/test/test_mqtt_connection_policy/test_mqtt_connection_policy.cpp @@ -152,6 +152,49 @@ TEST(MQTTConnectionPolicy, JwtClockNeedsNtpOrAReasonableWallClock) { EXPECT_TRUE(Policy::jwtClockAvailable(true, 0U)); } +TEST(MQTTConnectionPolicy, WifiBackoffLadderStartsAtFifteenSecondsAndSaturates) { + EXPECT_EQ(15000U, Policy::wifiReconnectBackoffMs(0)); + EXPECT_EQ(30000U, Policy::wifiReconnectBackoffMs(1)); + EXPECT_EQ(60000U, Policy::wifiReconnectBackoffMs(2)); + EXPECT_EQ(120000U, Policy::wifiReconnectBackoffMs(3)); + EXPECT_EQ(300000U, Policy::wifiReconnectBackoffMs(4)); + // Clamps at the 300 s rung for the saturated attempt count and beyond. + EXPECT_EQ(300000U, Policy::wifiReconnectBackoffMs(5)); + EXPECT_EQ(300000U, Policy::wifiReconnectBackoffMs(200)); +} + +TEST(MQTTConnectionPolicy, WifiBackoffAttemptClimbsThenSaturatesAtFive) { + uint8_t attempt = 0; + for (uint8_t expected = 1; expected <= 5; ++expected) { + attempt = Policy::nextWifiBackoffAttempt(attempt); + EXPECT_EQ(expected, attempt); + } + // Saturated: never advances past 5 (index stays clamped at the 300 s rung). + EXPECT_EQ(5U, Policy::nextWifiBackoffAttempt(attempt)); + EXPECT_EQ(5U, Policy::nextWifiBackoffAttempt(5)); +} + +TEST(MQTTConnectionPolicy, WifiReconnectRequiresBothDownAndSinceAttemptToClearRung) { + const uint32_t down_since = 1000U; + const uint32_t last_attempt = 1000U; + const uint8_t attempt = 0; // 15 s rung + // Neither interval has elapsed yet. + EXPECT_FALSE(Policy::wifiReconnectDue(1000U + 14999U, down_since, last_attempt, attempt)); + // Down long enough, but an attempt was made only 5 s ago (since-attempt short). + EXPECT_FALSE(Policy::wifiReconnectDue(1000U + 15000U, down_since, 1000U + 10000U, attempt)); + // Both cleared at the exact boundary: due. + EXPECT_TRUE(Policy::wifiReconnectDue(1000U + 15000U, down_since, last_attempt, attempt)); +} + +TEST(MQTTConnectionPolicy, WifiReconnectDueSurvivesMillisRollover) { + const uint32_t down_since = std::numeric_limits::max() - 100U; + const uint32_t last_attempt = down_since; + const uint8_t attempt = 0; // 15 s rung + const uint32_t now = down_since + 15000U; // wraps past zero + EXPECT_TRUE(Policy::wifiReconnectDue(now, down_since, last_attempt, attempt)); + EXPECT_FALSE(Policy::wifiReconnectDue(down_since + 14999U, down_since, last_attempt, attempt)); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/test/test_mqtt_packet_queue_policy/test_mqtt_packet_queue_policy.cpp b/test/test_mqtt_packet_queue_policy/test_mqtt_packet_queue_policy.cpp index 80e4ba26..ed282415 100644 --- a/test/test_mqtt_packet_queue_policy/test_mqtt_packet_queue_policy.cpp +++ b/test/test_mqtt_packet_queue_policy/test_mqtt_packet_queue_policy.cpp @@ -151,6 +151,29 @@ TEST(MQTTPacketQueuePolicy, RetrySchedulingDeadlineMayWrapToZero) { decision.retry_attempts)); } +TEST(MQTTPacketQueuePolicy, PartialPublishCountsAsDeliveredEitherWay) { + EXPECT_TRUE(QueuePolicy::queuedPacketPublished(true, true)); + EXPECT_TRUE(QueuePolicy::queuedPacketPublished(true, false)); // packet ok, raw failed + EXPECT_TRUE(QueuePolicy::queuedPacketPublished(false, true)); // raw ok, packet failed + EXPECT_FALSE(QueuePolicy::queuedPacketPublished(false, false)); // neither reached a slot +} + +TEST(MQTTPacketQueuePolicy, PublishOutcomePairingDrivesRetryDecision) { + // packet succeeds / raw fails -> completed, no retry. + QueuePolicy::RetryDecision d = + QueuePolicy::retryDecision(QueuePolicy::queuedPacketPublished(true, false), 0, 1234U); + EXPECT_EQ(QueuePolicy::RetryAction::Complete, d.action); + + // raw succeeds / packet fails -> also completed. + d = QueuePolicy::retryDecision(QueuePolicy::queuedPacketPublished(false, true), 0, 1234U); + EXPECT_EQ(QueuePolicy::RetryAction::Complete, d.action); + + // both fail on a fresh packet -> scheduled for a bounded retry. + d = QueuePolicy::retryDecision(QueuePolicy::queuedPacketPublished(false, false), 0, 1234U); + EXPECT_EQ(QueuePolicy::RetryAction::Schedule, d.action); + EXPECT_EQ(1U, d.retry_attempts); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/test/test_mqtt_topic_router/test_mqtt_topic_router.cpp b/test/test_mqtt_topic_router/test_mqtt_topic_router.cpp index 48881573..93e270c7 100644 --- a/test/test_mqtt_topic_router/test_mqtt_topic_router.cpp +++ b/test/test_mqtt_topic_router/test_mqtt_topic_router.cpp @@ -171,6 +171,18 @@ TEST(MQTTTopicRouter, RejectsInvalidStyleTypeSlotAndOutput) { EXPECT_FALSE(mqttTopicSlotIndexValid(0, 0)); } +TEST(MQTTTopicRouter, PublicationTypeEnumValuesAreFrozen) { + // The bridge passes MQTTBridge::MQTTMessageType to mqttBuildPublicationTopic + // as an int; a compile-time static_assert in the bridge ties the two enums + // together. Freeze the router side here so its values can't drift on their own. + EXPECT_EQ(0, MQTT_PUBLICATION_STATUS); + EXPECT_EQ(1, MQTT_PUBLICATION_PACKETS); + EXPECT_EQ(2, MQTT_PUBLICATION_RAW); + EXPECT_STREQ("status", mqttPublicationTypeName(MQTT_PUBLICATION_STATUS)); + EXPECT_STREQ("packets", mqttPublicationTypeName(MQTT_PUBLICATION_PACKETS)); + EXPECT_STREQ("raw", mqttPublicationTypeName(MQTT_PUBLICATION_RAW)); +} + } // namespace int main(int argc, char** argv) {