diff --git a/STABILITY_TESTABILITY_HANDOFF.md b/STABILITY_TESTABILITY_HANDOFF.md index 0128c0e6..a2a018d5 100644 --- a/STABILITY_TESTABILITY_HANDOFF.md +++ b/STABILITY_TESTABILITY_HANDOFF.md @@ -468,12 +468,19 @@ into the pure policy seams and covered: - `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. +The **WebConfig POST/result/reboot/stop state machine** (the largest gap) now +has a pure, host-tested SPEC — `src/helpers/WebConfigBatch.h` + +`test/test_webconfig_batch/` on branch `phase6/webconfig-batch-seam` — that +faithfully characterizes the accept/drain/result/reboot/stop decisions currently +inline in `WebConfigServer.cpp`. Like Phase 4's `MQTTLifecycle.h`, it is +**spec-first and NOT yet wired**: rewiring the hardware-tuned server to consume it +(so it becomes load-bearing and non-drifting) is a deliberately separate, +hardware-validated follow-up. + +Still open (each a good follow-up PR): wiring `WebConfigServer.cpp` to the +`WebConfigBatch` spec above, 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/WebConfigBatch.h b/src/helpers/WebConfigBatch.h new file mode 100644 index 00000000..d7040571 --- /dev/null +++ b/src/helpers/WebConfigBatch.h @@ -0,0 +1,195 @@ +#pragma once + +#include + +// Fork-owned, dependency-free spec for the WebConfig "config batch / reboot / +// stop" decision + timing core, plus its host tests (test/test_webconfig_batch/). +// +// This is the Phase 6 counterpart of MQTTLifecycle.h: a PURE state machine that +// captures exactly what src/helpers/esp32/WebConfigServer.cpp decides today, so +// the POST-accept / drain / result-read / reboot / stop transitions can be +// exercised deterministically without AsyncWebServer, ArduinoJson, WiFi, or the +// FreeRTOS mutex/refcount. +// +// Scope boundary (spec-first, exactly like Phase 4's MQTTLifecycle.h): this +// header is the SPEC and the test seam. It is NOT yet wired into +// WebConfigServer.cpp. The batch/reboot/stop state machine there is +// hardware-tuned (it was debugged against real iOS captive-portal behavior, +// HTTP caching, and route ordering), so the production rewiring that makes these +// functions load-bearing is a deliberately separate, hardware-validated +// follow-up. Until then, keep this in sync with WebConfigServer.cpp by hand; the +// file:line references below point at the behavior each function mirrors. +// +// Behavior source (all line refs against WebConfigServer.{h,cpp} at the time of +// writing): constants at .h:90-96,155-161; POST accept at .cpp:610-719; drain at +// .cpp:289-334; result read at .cpp:721-791; reboot fire at .cpp:262-265; +// isRebootPending at .cpp:70-74; stop gating at .cpp:185-255. +namespace WebConfigBatch { + +// Constants, verbatim from WebConfigServer. +static const int kMaxBatch = 24; // .h:90 MAX_BATCH +static const uint32_t kDrainPacingMs = 25; // .cpp:296 inter-command gap +static const uint32_t kRebootFallbackMs = 30000; // .cpp:331 drain-finish fallback +static const uint32_t kRebootConfirmMs = 3000; // .cpp:784 first result-read arm +static const uint32_t kStopWarnMs = 10000; // .h:95 STOP_WARN_MS + +// The batch lifecycle. A fresh POST moves Idle->Pending; the drainer moves +// Pending->Done; Done stays re-readable until the next POST claims the slot; +// finalizeTeardown() resets to Idle. +enum class State : uint8_t { + Idle = 0, + Pending, + Done, +}; + +// millis() idioms. elapsedMs uses unsigned wraparound (correct across one 32-bit +// rollover). scheduleAt mirrors the production wrap-guard: every _reboot_at / +// _stop_warn_at assignment does `if (t == 0) t = 1;` so 0 keeps meaning +// "unscheduled" even when the deadline lands exactly on the rollover boundary. +static inline uint32_t elapsedMs(uint32_t now, uint32_t then) { return now - then; } +static inline uint32_t scheduleAt(uint32_t now, uint32_t delay) { + const uint32_t t = now + delay; + return t == 0 ? 1u : t; +} +// Signed wrap-safe "deadline reached", matching the production +// `(int32_t)(now - deadline) >= 0` comparisons. +static inline bool deadlineReached(uint32_t now, uint32_t deadline) { + return (int32_t)(now - deadline) >= 0; +} + +// -------------------------------------------------------------------------- +// POST accept classification (.cpp:637-718). Precedence, verbatim from the +// source: an in-flight/finished batch with the SAME reqid is an idempotent +// replay (commands are NOT re-applied); a DIFFERENT reqid while a batch is still +// PENDING is rejected as busy; otherwise a batch with no changes and no reboot +// is a no-op, and anything else is accepted. Note the asymmetry: a different +// reqid while DONE is NOT busy — the new batch overwrites the DONE slot. +// +// Assumes the request already passed reqid grammar (WebConfigKeys::wcIsValidReqId) +// and per-key allowlist/secret validation, which are covered by test_webconfig_keys. +// -------------------------------------------------------------------------- +enum class PostOutcome : uint8_t { + Replay, // 202; reqid matches the current batch, commands not re-applied + Busy, // 409; a different batch is still PENDING + Accept, // 202; a new batch is accepted (from Idle, or overwriting a Done slot) + NoChanges, // 400; nothing to do (no changes and no reboot requested) +}; + +static inline PostOutcome classifyPost(State state, bool reqid_matches_current, + int change_count, bool reboot_after) { + if (state != State::Idle && reqid_matches_current) return PostOutcome::Replay; + if (state == State::Pending) return PostOutcome::Busy; // reqid differs (match handled above) + if (change_count <= 0 && !reboot_after) return PostOutcome::NoChanges; + return PostOutcome::Accept; +} + +// The state string a Replay body reports mirrors the batch state (.cpp:640): +// "done" when the matched batch already finished, else "pending". +static inline const char* replayStateName(State state) { + return state == State::Done ? "done" : "pending"; +} + +// -------------------------------------------------------------------------- +// Drain (.cpp:289-333). One command per tick. +// -------------------------------------------------------------------------- + +// The drainer waits only BETWEEN commands: never before the first (batch_next +// == 0 runs immediately and fires onConfigBatchStart), never after the last, and +// otherwise until the 25 ms pacing gap elapses. The pacing compare is SIGNED to +// mirror the source verbatim (.cpp:296 `(int32_t)(now - _batch_last_cmd) < 25`), +// matching deadlineReached()'s signedness; for all reachable inputs (elapsed +// 0..25 ms) it is identical to the unsigned form. +static inline bool drainMustWait(int batch_next, int batch_count, + uint32_t now, uint32_t last_cmd_ms) { + return batch_next > 0 && batch_next < batch_count && + (int32_t)(now - last_cmd_ms) < (int32_t)kDrainPacingMs; +} + +// all_ok is a sticky AND across command replies; a reply counts as ok iff it +// begins with "OK" (.cpp:314). Once false it stays false. +static inline bool nextAllOk(bool prev_all_ok, bool reply_is_ok) { + return prev_all_ok && reply_is_ok; +} + +// The batch is finished once the post-increment drain index reaches the count +// (.cpp:319-321). +static inline bool drainFinished(int batch_next_after_increment, int batch_count) { + return batch_next_after_increment >= batch_count; +} + +// On finish, a reboot-requested + all-ok batch arms the 30 s fallback deadline; +// a partially-failed batch (all_ok == false) never reboots (.cpp:324-333). +// Returns the reboot_at deadline, or 0 for "no reboot scheduled". +static inline uint32_t finishRebootAt(bool batch_reboot, bool batch_all_ok, uint32_t now) { + return (batch_reboot && batch_all_ok) ? scheduleAt(now, kRebootFallbackMs) : 0u; +} + +// -------------------------------------------------------------------------- +// Result read (.cpp:738-786). +// -------------------------------------------------------------------------- +enum class ResultOutcome : uint8_t { + Idle, // 200 "idle" — no batch; any valid reqid is echoed + Unknown, // 404 — a batch exists but the reqid does not match it + Pending, // 200 "pending" + Done, // 200 "done" (+ per-command results) +}; + +static inline ResultOutcome classifyResult(State state, bool reqid_matches_current) { + if (state == State::Idle) return ResultOutcome::Idle; // no reqid check while idle + if (!reqid_matches_current) return ResultOutcome::Unknown; + return state == State::Pending ? ResultOutcome::Pending : ResultOutcome::Done; +} + +// The "reboot" flag reported in a Done body (.cpp:767): only a fully-OK, +// reboot-requested batch advertises a pending reboot. +static inline bool doneReportsReboot(bool batch_reboot, bool batch_all_ok) { + return batch_reboot && batch_all_ok; +} + +// The first Done read arms the confirmed (3 s) reboot exactly once (.cpp:777-786): +// the !already_armed guard makes later reads idempotent, so polling cannot push +// the deadline out. When this returns true the caller sets armed = true and +// reboot_at = confirmRebootAt(now). +static inline bool shouldArmConfirmReboot(State state, bool batch_reboot, + bool batch_all_ok, bool already_armed) { + return state == State::Done && batch_reboot && batch_all_ok && !already_armed; +} +static inline uint32_t confirmRebootAt(uint32_t now) { + return scheduleAt(now, kRebootConfirmMs); +} + +// -------------------------------------------------------------------------- +// Reboot fire (.cpp:262-265) and isRebootPending (.cpp:70-74). +// -------------------------------------------------------------------------- +static inline bool rebootDue(uint32_t reboot_at, uint32_t now) { + return reboot_at != 0 && deadlineReached(now, reboot_at); +} + +// isRebootPending() reports true only for a config-save reboot in the Done +// state, so the manual /api/reboot path (batch_reboot == false) is deliberately +// NOT reported as pending even though _reboot_at is set. +static inline bool isConfigRebootPending(uint32_t reboot_at, bool batch_reboot, State state) { + return reboot_at != 0 && batch_reboot && state == State::Done; +} + +// -------------------------------------------------------------------------- +// Stop gating (.cpp:244-255). Teardown waits indefinitely for in-flight async +// handlers to drain (refs == 0); the STOP_WARN_MS timer only triggers a one-time +// diagnostic — it never forces teardown. +// -------------------------------------------------------------------------- +enum class StopAction : uint8_t { + Finalize, // refs == 0: finalizeTeardown() may run now + Warn, // refs > 0 and the warn deadline passed, not yet warned: log once + Wait, // refs > 0: keep the session alive and wait +}; + +static inline StopAction stopStep(uint32_t handler_refs, bool already_warned, + uint32_t stop_warn_at, uint32_t now) { + if (handler_refs == 0) return StopAction::Finalize; + if (!already_warned && stop_warn_at != 0 && deadlineReached(now, stop_warn_at)) { + return StopAction::Warn; + } + return StopAction::Wait; +} + +} // namespace WebConfigBatch diff --git a/test/test_webconfig_batch/test_webconfig_batch.cpp b/test/test_webconfig_batch/test_webconfig_batch.cpp new file mode 100644 index 00000000..9faddc4f --- /dev/null +++ b/test/test_webconfig_batch/test_webconfig_batch.cpp @@ -0,0 +1,206 @@ +// Host contract tests for the pure WebConfig config-batch / reboot / stop state +// machine. This spec mirrors src/helpers/esp32/WebConfigServer.cpp; it is not +// yet wired into the bridge (see WebConfigBatch.h scope note). +#include + +#include +#include + +#include "helpers/WebConfigBatch.h" + +namespace Batch = WebConfigBatch; +using State = WebConfigBatch::State; + +// -------------------------------------------------------------------------- +// POST accept classification +// -------------------------------------------------------------------------- +TEST(WebConfigBatch, IdleAcceptsANewBatchWithChangesOrRebootOnly) { + // A normal save (changes present) is accepted. + EXPECT_EQ(Batch::PostOutcome::Accept, + Batch::classifyPost(State::Idle, /*reqid_matches=*/false, /*count=*/3, false)); + // A reboot-only request (no changes) is still accepted. + EXPECT_EQ(Batch::PostOutcome::Accept, + Batch::classifyPost(State::Idle, false, 0, /*reboot_after=*/true)); +} + +TEST(WebConfigBatch, IdleWithNothingToDoIsNoChanges) { + EXPECT_EQ(Batch::PostOutcome::NoChanges, + Batch::classifyPost(State::Idle, false, 0, false)); +} + +TEST(WebConfigBatch, SameReqidReplaysWithoutReapplyingWhilePendingOrDone) { + // The idempotent-replay path fires for BOTH in-flight and finished batches + // when the reqid matches; commands are never re-applied. + EXPECT_EQ(Batch::PostOutcome::Replay, + Batch::classifyPost(State::Pending, /*reqid_matches=*/true, 3, false)); + EXPECT_EQ(Batch::PostOutcome::Replay, + Batch::classifyPost(State::Done, /*reqid_matches=*/true, 3, false)); + // Replay wins even over a would-be no-changes request. + EXPECT_EQ(Batch::PostOutcome::Replay, + Batch::classifyPost(State::Pending, true, 0, false)); + // The replay body reports the batch's own state. + EXPECT_STREQ("pending", Batch::replayStateName(State::Pending)); + EXPECT_STREQ("done", Batch::replayStateName(State::Done)); +} + +TEST(WebConfigBatch, DifferentReqidIsBusyOnlyWhilePending) { + // A second client (different reqid) while a batch is still draining => busy. + EXPECT_EQ(Batch::PostOutcome::Busy, + Batch::classifyPost(State::Pending, /*reqid_matches=*/false, 2, false)); + // But once the previous batch is DONE, a different reqid is NOT busy: the new + // batch overwrites the finished slot (asymmetry with the Pending case). + EXPECT_EQ(Batch::PostOutcome::Accept, + Batch::classifyPost(State::Done, false, 2, false)); + EXPECT_EQ(Batch::PostOutcome::NoChanges, + Batch::classifyPost(State::Done, false, 0, false)); +} + +// -------------------------------------------------------------------------- +// Drain pacing / all_ok / finish +// -------------------------------------------------------------------------- +TEST(WebConfigBatch, DrainNeverWaitsBeforeTheFirstOrAfterTheLastCommand) { + // batch_next == 0: the first command runs immediately (fires onConfigBatchStart). + EXPECT_FALSE(Batch::drainMustWait(0, 5, 1000, 1000)); + // batch_next >= batch_count: nothing left to pace. + EXPECT_FALSE(Batch::drainMustWait(5, 5, 1000, 1000)); + // A single-command (or reboot-only) batch never paces. + EXPECT_FALSE(Batch::drainMustWait(0, 1, 1000, 1000)); + EXPECT_FALSE(Batch::drainMustWait(0, 0, 1000, 1000)); +} + +TEST(WebConfigBatch, DrainPacesTwentyFiveMillisBetweenCommandsWithInclusiveRelease) { + const uint32_t last = 1000; + // 24 ms after the previous command: still waiting. + EXPECT_TRUE(Batch::drainMustWait(2, 5, last + 24, last)); + // Exactly 25 ms: the gate releases (production uses `< 25`). + EXPECT_FALSE(Batch::drainMustWait(2, 5, last + 25, last)); + EXPECT_FALSE(Batch::drainMustWait(2, 5, last + 26, last)); +} + +TEST(WebConfigBatch, DrainPacingSurvivesMillisRollover) { + const uint32_t last = std::numeric_limits::max() - 10; + EXPECT_TRUE(Batch::drainMustWait(2, 5, last + 24, last)); // 24 ms elapsed, wrapped + EXPECT_FALSE(Batch::drainMustWait(2, 5, last + 25, last)); // 25 ms elapsed, wrapped +} + +TEST(WebConfigBatch, AllOkIsAStickyAndAcrossCommandReplies) { + bool all_ok = true; + all_ok = Batch::nextAllOk(all_ok, true); + EXPECT_TRUE(all_ok); + all_ok = Batch::nextAllOk(all_ok, false); // one command failed + EXPECT_FALSE(all_ok); + all_ok = Batch::nextAllOk(all_ok, true); // stays false forever after + EXPECT_FALSE(all_ok); +} + +TEST(WebConfigBatch, DrainFinishesWhenTheIndexReachesTheCount) { + EXPECT_FALSE(Batch::drainFinished(4, 5)); + EXPECT_TRUE(Batch::drainFinished(5, 5)); + EXPECT_TRUE(Batch::drainFinished(0, 0)); // reboot-only / empty batch +} + +TEST(WebConfigBatch, FinishArmsThirtySecondFallbackOnlyForAFullyOkRebootBatch) { + const uint32_t now = 100000; + EXPECT_EQ(now + Batch::kRebootFallbackMs, + Batch::finishRebootAt(/*reboot=*/true, /*all_ok=*/true, now)); + // A partially-failed batch never reboots. + EXPECT_EQ(0u, Batch::finishRebootAt(true, false, now)); + // No reboot requested. + EXPECT_EQ(0u, Batch::finishRebootAt(false, true, now)); +} + +// -------------------------------------------------------------------------- +// Result read +// -------------------------------------------------------------------------- +TEST(WebConfigBatch, ResultReadClassifiesIdlePendingDoneAndUnknownReqid) { + // Idle: any valid reqid gets "idle" (no reqid check while idle). + EXPECT_EQ(Batch::ResultOutcome::Idle, Batch::classifyResult(State::Idle, false)); + EXPECT_EQ(Batch::ResultOutcome::Idle, Batch::classifyResult(State::Idle, true)); + // Matching reqid reflects the batch state. + EXPECT_EQ(Batch::ResultOutcome::Pending, Batch::classifyResult(State::Pending, true)); + EXPECT_EQ(Batch::ResultOutcome::Done, Batch::classifyResult(State::Done, true)); + // A live/finished batch with a mismatched reqid is unknown (404). + EXPECT_EQ(Batch::ResultOutcome::Unknown, Batch::classifyResult(State::Pending, false)); + EXPECT_EQ(Batch::ResultOutcome::Unknown, Batch::classifyResult(State::Done, false)); +} + +TEST(WebConfigBatch, DoneBodyAdvertisesRebootOnlyWhenFullyOk) { + EXPECT_TRUE(Batch::doneReportsReboot(true, true)); + EXPECT_FALSE(Batch::doneReportsReboot(true, false)); + EXPECT_FALSE(Batch::doneReportsReboot(false, true)); +} + +TEST(WebConfigBatch, FirstDoneReadArmsTheThreeSecondRebootExactlyOnce) { + const uint32_t now = 500000; + // First read of a fully-OK reboot batch arms. + EXPECT_TRUE(Batch::shouldArmConfirmReboot(State::Done, true, true, /*already_armed=*/false)); + EXPECT_EQ(now + Batch::kRebootConfirmMs, Batch::confirmRebootAt(now)); + // Already armed => never re-arms (polling can't push the deadline out). + EXPECT_FALSE(Batch::shouldArmConfirmReboot(State::Done, true, true, /*already_armed=*/true)); + // Not applicable while pending, without reboot, or after a partial failure. + EXPECT_FALSE(Batch::shouldArmConfirmReboot(State::Pending, true, true, false)); + EXPECT_FALSE(Batch::shouldArmConfirmReboot(State::Done, false, true, false)); + EXPECT_FALSE(Batch::shouldArmConfirmReboot(State::Done, true, false, false)); +} + +// -------------------------------------------------------------------------- +// Reboot fire / pending +// -------------------------------------------------------------------------- +TEST(WebConfigBatch, RebootFiresAtOrAfterTheDeadlineAndNeverWhenUnscheduled) { + EXPECT_FALSE(Batch::rebootDue(0, 1234567)); // 0 == unscheduled + const uint32_t at = 100000; + EXPECT_FALSE(Batch::rebootDue(at, at - 1)); + EXPECT_TRUE(Batch::rebootDue(at, at)); // inclusive boundary + EXPECT_TRUE(Batch::rebootDue(at, at + 1)); +} + +TEST(WebConfigBatch, RebootDueSurvivesMillisRollover) { + const uint32_t at = std::numeric_limits::max() - 5; // near the top + EXPECT_FALSE(Batch::rebootDue(at, at - 1)); + EXPECT_TRUE(Batch::rebootDue(at, at)); + EXPECT_TRUE(Batch::rebootDue(at, at + 10)); // now has wrapped past zero +} + +TEST(WebConfigBatch, OnlyAConfigSaveRebootInDoneStateReportsPending) { + EXPECT_TRUE(Batch::isConfigRebootPending(/*reboot_at=*/123, /*batch_reboot=*/true, State::Done)); + // Manual /api/reboot: reboot_at set but batch_reboot false => not "pending". + EXPECT_FALSE(Batch::isConfigRebootPending(123, false, State::Done)); + // Not yet done, or nothing scheduled. + EXPECT_FALSE(Batch::isConfigRebootPending(123, true, State::Pending)); + EXPECT_FALSE(Batch::isConfigRebootPending(0, true, State::Done)); +} + +// -------------------------------------------------------------------------- +// Stop gating +// -------------------------------------------------------------------------- +TEST(WebConfigBatch, StopFinalizesOnlyWhenNoHandlersAreInFlight) { + EXPECT_EQ(Batch::StopAction::Finalize, + Batch::stopStep(/*refs=*/0, /*warned=*/false, /*warn_at=*/50000, /*now=*/60000)); +} + +TEST(WebConfigBatch, StopWarnsOnceAfterTheDeadlineThenKeepsWaiting) { + const uint32_t warn_at = 50000; + // Before the warn deadline with handlers in flight: just wait. + EXPECT_EQ(Batch::StopAction::Wait, Batch::stopStep(2, false, warn_at, warn_at - 1)); + // At the deadline, not yet warned: warn once. + EXPECT_EQ(Batch::StopAction::Warn, Batch::stopStep(2, false, warn_at, warn_at)); + // Already warned: keep waiting (never warns again, never forces teardown). + EXPECT_EQ(Batch::StopAction::Wait, Batch::stopStep(2, true, warn_at, warn_at + 100000)); + // An unscheduled warn timer never warns. + EXPECT_EQ(Batch::StopAction::Wait, Batch::stopStep(2, false, 0, 999999)); +} + +// -------------------------------------------------------------------------- +// Wrap-around guard shared with the production _reboot_at assignments +// -------------------------------------------------------------------------- +TEST(WebConfigBatch, ScheduleAtNeverReturnsTheUnscheduledSentinel) { + EXPECT_EQ(1000u + 3000u, Batch::scheduleAt(1000, 3000)); + // A deadline that lands exactly on 0 is bumped to 1 so it still means "set". + const uint32_t just_below_wrap = std::numeric_limits::max(); // +1 wraps to 0 + EXPECT_EQ(1u, Batch::scheduleAt(just_below_wrap, 1)); +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}