From cece565e11da1bd8b29abc4794a939bd5bd7a727 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sun, 19 Jul 2026 12:35:52 -0700 Subject: [PATCH] refactor(webconfig): wire WebConfigBatch spec into WebConfigServer (Phase 6) The pure batch/reboot/stop state machine in WebConfigBatch.h was host-tested but not referenced by production, so the real logic in WebConfigServer.cpp was untested and the two could drift silently. Repoint the production decision points at the spec: POST classification and replay-state naming, drain pacing/all-ok/finish, reboot scheduling and firing, result classification, confirm-reboot arming, and stop gating. MAX_BATCH and STOP_WARN_MS now alias kMaxBatch/kStopWarnMs so the constants cannot drift. Behavior-preserving. Two asymmetries are deliberate and documented in the header: finishRebootAt()'s 0 return must not be assigned unconditionally (the manual /api/reboot route also owns _reboot_at and could be cancelled), and classifyPost() is consulted in two phases because the change count is only known after parsing, which must not precede the Replay/Busy answer. Native suite (14 suites) and both MQTT smoke builds green. --- src/helpers/WebConfigBatch.h | 26 ++++++--- src/helpers/esp32/WebConfigServer.cpp | 83 ++++++++++++++++++--------- src/helpers/esp32/WebConfigServer.h | 19 +++++- 3 files changed, 90 insertions(+), 38 deletions(-) diff --git a/src/helpers/WebConfigBatch.h b/src/helpers/WebConfigBatch.h index d7040571..2f93f56d 100644 --- a/src/helpers/WebConfigBatch.h +++ b/src/helpers/WebConfigBatch.h @@ -11,14 +11,24 @@ // 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. +// WIRED: WebConfigServer.cpp calls these functions directly, so they are +// load-bearing and the host tests cover production behavior rather than a +// parallel copy of it. MAX_BATCH and STOP_WARN_MS in WebConfigServer.h alias +// kMaxBatch/kStopWarnMs here, so the constants cannot drift either. +// +// Two deliberate asymmetries remain between this spec and its caller: +// +// 1. finishRebootAt() returns 0 for "no reboot scheduled", but the caller must +// only ASSIGN _reboot_at when the result is non-zero. _reboot_at is not +// solely batch-owned — the manual /api/reboot route arms it from the +// async_tcp task, possibly while a batch is still draining — so an +// unconditional assign would silently cancel a manual reboot. +// 2. classifyPost() is consulted in two phases by handleConfigPost, because the +// change count is only known after the `set` map is parsed, and parsing must +// not precede the Replay/Busy answer (a replayed POST carrying a bad key +// must still receive its 202). +// +// 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 diff --git a/src/helpers/esp32/WebConfigServer.cpp b/src/helpers/esp32/WebConfigServer.cpp index 04aaaeba..5603a55d 100644 --- a/src/helpers/esp32/WebConfigServer.cpp +++ b/src/helpers/esp32/WebConfigServer.cpp @@ -69,8 +69,9 @@ WebConfigServer::~WebConfigServer() { bool WebConfigServer::isRebootPending() { WebConfigServer* w = _active; - return w != NULL && w->_reboot_at != 0 && w->_batch_reboot && - w->_batch_state == BATCH_DONE; + return w != NULL && WebConfigBatch::isConfigRebootPending( + w->_reboot_at, w->_batch_reboot, + toSpecState(w->_batch_state)); } bool WebConfigServer::getSetupInfo(char* ssid, size_t ssid_len, char* ip, size_t ip_len) { @@ -191,8 +192,7 @@ void WebConfigServer::requestStop() { if (_dns) _dns->stop(); _mode = MODE_OFF; _stopping = true; - uint32_t warn_at = millis() + STOP_WARN_MS; - _stop_warn_at = warn_at ? warn_at : 1; + _stop_warn_at = WebConfigBatch::scheduleAt(millis(), STOP_WARN_MS); _stop_warned = false; } @@ -244,12 +244,17 @@ void WebConfigServer::finalizeTeardown() { void WebConfigServer::tick(uint32_t now) { if (_stopping) { uint32_t refs = handlerRefCount(); - if (refs == 0) { - finalizeTeardown(); - } else if (!_stop_warned && _stop_warn_at && (int32_t)(now - _stop_warn_at) >= 0) { - _stop_warned = true; - Serial.printf("WC: stop waiting for %lu handler(s); retaining session safely\n", - (unsigned long)refs); + switch (WebConfigBatch::stopStep(refs, _stop_warned, _stop_warn_at, now)) { + case WebConfigBatch::StopAction::Finalize: + finalizeTeardown(); + break; + case WebConfigBatch::StopAction::Warn: + _stop_warned = true; + Serial.printf("WC: stop waiting for %lu handler(s); retaining session safely\n", + (unsigned long)refs); + break; + case WebConfigBatch::StopAction::Wait: + break; } return; } @@ -259,7 +264,7 @@ void WebConfigServer::tick(uint32_t now) { if (_batch_state == BATCH_PENDING) drainBatch(now); - if (_reboot_at && (int32_t)(now - _reboot_at) >= 0) { + if (WebConfigBatch::rebootDue(_reboot_at, now)) { Serial.printf("WC: rebooting now (%s)\n", _batch_reboot_armed ? "confirmed" : "fallback"); _cb->rebootNow(); // does not return } @@ -293,7 +298,7 @@ void WebConfigServer::drainBatch(uint32_t now) { // clients (iPhones especially) to drop off mid-save. if (_batch_next == 0) { _cb->onConfigBatchStart(); - } else if (_batch_next < _batch_count && (int32_t)(now - _batch_last_cmd) < 25) { + } else if (WebConfigBatch::drainMustWait(_batch_next, _batch_count, now, _batch_last_cmd)) { return; // let the WiFi task breathe between flash writes } if (_batch_next < _batch_count) { @@ -311,25 +316,31 @@ void WebConfigServer::drainBatch(uint32_t now) { if (e.reply[0] == 0) strcpy(e.reply, "OK"); // Success convention across every allowlisted setter is an "OK" prefix // (the UI relies on the same test); anything else is a rejection. - if (strncmp(e.reply, "OK", 2) != 0) _batch_all_ok = false; + _batch_all_ok = WebConfigBatch::nextAllOk(_batch_all_ok, + strncmp(e.reply, "OK", 2) == 0); } _batch_last_cmd = millis(); Serial.printf("WC: cmd %d/%d '%s' took %lums\n", (int)_batch_next, (int)_batch_count, e.key, (unsigned long)(_batch_last_cmd - t0)); - if (_batch_next < _batch_count) return; // more commands next tick + if (!WebConfigBatch::drainFinished(_batch_next, _batch_count)) { + return; // more commands next tick + } } _cb->onConfigBatchEnd(); WCLock lock(_mux); _batch_state = BATCH_DONE; - if (_batch_reboot && _batch_all_ok) { + // Assign only when a reboot is actually scheduled. finishRebootAt() returns 0 + // for "not scheduled", but _reboot_at is NOT solely batch-owned: the manual + // /api/reboot route can arm it from the async_tcp task while a batch is still + // draining, and an unconditional assign here would silently cancel it. + if (WebConfigBatch::finishRebootAt(_batch_reboot, _batch_all_ok, now) != 0) { // Fallback only, and only when every command succeeded: rebooting into a // partially-applied config would strand the node. The real 3 s reboot timer // is armed when the client reads /api/config/result (handleConfigResult), so // the browser gets its confirmation before the AP/WiFi drops. This covers a // client that disconnected and never polls — generous enough for a phone // that got bounced off the AP mid-save to rejoin and fetch its confirmation. - _reboot_at = now + 30000; - if (_reboot_at == 0) _reboot_at = 1; + _reboot_at = WebConfigBatch::finishRebootAt(_batch_reboot, _batch_all_ok, now); } } @@ -634,9 +645,20 @@ void WebConfigServer::handleConfigPost(AsyncWebServerRequest* req) { // Repeating a POST with the same request ID is also idempotent: acknowledge // the batch already occupying the slot instead of applying its commands a // second time after an ambiguous network failure. - if (_batch_state != BATCH_IDLE && strcmp(reqid, _batch_reqid) == 0) { + // Classification lives in WebConfigBatch::classifyPost. It is consulted in two + // phases because the change count is only known after the `set` map is parsed + // below, and parsing must not run before Replay/Busy are answered (a replayed + // POST carrying a bad key must still get its 202, not a 400). kCountUnknown is + // a non-zero placeholder that keeps the count-dependent arms unreachable here. + const WebConfigBatch::State bstate = toSpecState(_batch_state); + const bool reqid_matches = (strcmp(reqid, _batch_reqid) == 0); + const int kCountUnknown = 1; + const WebConfigBatch::PostOutcome pre = + WebConfigBatch::classifyPost(bstate, reqid_matches, kCountUnknown, reboot_after); + + if (pre == WebConfigBatch::PostOutcome::Replay) { StaticJsonDocument<96> ack; - ack["state"] = _batch_state == BATCH_DONE ? "done" : "pending"; + ack["state"] = WebConfigBatch::replayStateName(bstate); ack["count"] = _batch_count; ack["reqid"] = (const char*)_batch_reqid; String out; @@ -644,7 +666,7 @@ void WebConfigServer::handleConfigPost(AsyncWebServerRequest* req) { req->send(202, "application/json", out); return; } - if (_batch_state == BATCH_PENDING) { + if (pre == WebConfigBatch::PostOutcome::Busy) { // Echo the in-flight batch's reqid so the caller can tell its own retry // (same reqid — landed, keep polling) from another client's save. StaticJsonDocument<96> bd; @@ -692,7 +714,10 @@ void WebConfigServer::handleConfigPost(AsyncWebServerRequest* req) { e.cmd[pos] = 0; count++; } - if (count == 0 && !reboot_after) { + // Phase 2: the count is now known, so the remaining NoChanges/Accept arms + // resolve. Replay/Busy were already answered above. + if (WebConfigBatch::classifyPost(bstate, reqid_matches, count, reboot_after) == + WebConfigBatch::PostOutcome::NoChanges) { req->send(400, "application/json", "{\"error\":\"no changes\"}"); return; } @@ -735,7 +760,9 @@ void WebConfigServer::handleConfigResult(AsyncWebServerRequest* req) { // fires but no branch print follows, the handler is blocked on _mux. Serial.printf("WC: result entry mode=%d state=%d\n", (int)_mode, (int)_batch_state); WCLock lock(_mux); - if (_batch_state == BATCH_IDLE) { + const WebConfigBatch::ResultOutcome outcome = WebConfigBatch::classifyResult( + toSpecState(_batch_state), strcmp(requested_reqid.c_str(), _batch_reqid) == 0); + if (outcome == WebConfigBatch::ResultOutcome::Idle) { Serial.println("WC: result read -> idle"); StaticJsonDocument<64> idle; idle["state"] = "idle"; @@ -745,11 +772,11 @@ void WebConfigServer::handleConfigResult(AsyncWebServerRequest* req) { req->send(200, "application/json", out); return; } - if (strcmp(requested_reqid.c_str(), _batch_reqid) != 0) { + if (outcome == WebConfigBatch::ResultOutcome::Unknown) { req->send(404, "application/json", "{\"error\":\"unknown request\"}"); return; } - if (_batch_state == BATCH_PENDING) { + if (outcome == WebConfigBatch::ResultOutcome::Pending) { StaticJsonDocument<96> pd; pd["state"] = "pending"; pd["reqid"] = (const char*)_batch_reqid; @@ -764,7 +791,7 @@ void WebConfigServer::handleConfigResult(AsyncWebServerRequest* req) { doc["state"] = "done"; // Only advertise a reboot when it will actually happen: a partially-failed // batch is not rebooted (see below), so the UI must not show a reboot screen. - doc["reboot"] = _batch_reboot && _batch_all_ok; + doc["reboot"] = WebConfigBatch::doneReportsReboot(_batch_reboot, _batch_all_ok); doc["all_ok"] = _batch_all_ok; doc["reqid"] = (const char*)_batch_reqid; JsonArray results = doc.createNestedArray("results"); @@ -774,15 +801,15 @@ void WebConfigServer::handleConfigResult(AsyncWebServerRequest* req) { r["reply"] = (const char*)_batch[i].reply; } // State stays DONE (re-readable) until the next POST claims the slot. - if (_batch_reboot && _batch_all_ok && !_batch_reboot_armed) { + if (WebConfigBatch::shouldArmConfirmReboot(toSpecState(_batch_state), _batch_reboot, + _batch_all_ok, _batch_reboot_armed)) { // Confirmation delivered and every command succeeded — reboot 3 s from now // (replaces the 30 s drain-time fallback) so the UI can show its countdown // first. Armed once; re-reads must not keep pushing the deadline out. A // partially-failed batch is deliberately left running so the operator can // correct and retry instead of rebooting into a broken config. _batch_reboot_armed = true; - _reboot_at = millis() + 3000; - if (_reboot_at == 0) _reboot_at = 1; + _reboot_at = WebConfigBatch::confirmRebootAt(millis()); } AsyncResponseStream* res = req->beginResponseStream("application/json"); diff --git a/src/helpers/esp32/WebConfigServer.h b/src/helpers/esp32/WebConfigServer.h index da261a11..2772057b 100644 --- a/src/helpers/esp32/WebConfigServer.h +++ b/src/helpers/esp32/WebConfigServer.h @@ -29,6 +29,7 @@ #include #include #include +#include class AsyncWebServer; class AsyncWebServerRequest; @@ -87,13 +88,27 @@ public: bool isStopping() const { return _stopping; } private: - static const int MAX_BATCH = 24; + // Batch/reboot/stop decisions and timings live in the fork-owned pure spec + // WebConfigBatch.h (host-tested by test/test_webconfig_batch). These aliases + // keep a single source of truth so the spec and this server cannot drift. + static const int MAX_BATCH = WebConfigBatch::kMaxBatch; static const size_t MAX_BODY = 4096; // A detached session normally drains immediately because handlers are short. // If one does not, keep the session alive (safe) and emit a diagnostic rather // than freeing memory still referenced by the async task. - static const uint32_t STOP_WARN_MS = 10000; + static const uint32_t STOP_WARN_MS = WebConfigBatch::kStopWarnMs; enum BatchState : uint8_t { BATCH_IDLE = 0, BATCH_PENDING, BATCH_DONE }; + + // BatchState and WebConfigBatch::State are deliberately kept as separate + // types (the enum is stored in a volatile member and used in prints); this + // is the one conversion point. + static WebConfigBatch::State toSpecState(BatchState s) { + switch (s) { + case BATCH_PENDING: return WebConfigBatch::State::Pending; + case BATCH_DONE: return WebConfigBatch::State::Done; + default: return WebConfigBatch::State::Idle; + } + } struct BatchEntry { char key[24]; // allowlisted `set` key (echoed back to the UI) char cmd[160]; // full CLI command (may contain secrets - never echoed)