From 5fc0943f591cc3ac3a7565ab330d4ac48eb96a4d Mon Sep 17 00:00:00 2001 From: mikecarper Date: Fri, 16 Oct 2026 13:45:48 -0700 Subject: [PATCH] Report uncertain preference saves without promising rollback --- .github/workflows/run-unit-tests.yml | 2 + src/helpers/CommonCLI.cpp | 61 ++-- src/helpers/CommonCLI_Observer.cpp | 2 +- src/helpers/PeripheralModePrefs.h | 2 +- src/helpers/PrefsSaveReplyGuard.h | 2 +- .../fixtures/common_cli_save_results/main.cpp | 7 +- test/fixtures/common_prefs_apply_reply.cpp | 320 ++++++++++++++++++ .../common_prefs_uncertain_reply/main.cpp | 209 ++++++++++++ test/fixtures/rs232_mqtt_runtime.cpp | 12 +- test/test_common_cli_logging_transaction.py | 3 +- test/test_common_prefs_apply_reply.py | 85 +++++ test/test_common_prefs_uncertain_reply.py | 133 ++++++++ test/test_common_usb_debug.py | 2 +- test/test_gps_sync_preferences.py | 2 +- test/test_observer_ota_channel.py | 2 +- test/test_peripheral_mode_prefs.py | 6 +- test/test_repeater_trace_repeat.py | 2 +- 17 files changed, 807 insertions(+), 45 deletions(-) create mode 100644 test/fixtures/common_prefs_apply_reply.cpp create mode 100644 test/fixtures/common_prefs_uncertain_reply/main.cpp create mode 100644 test/test_common_prefs_apply_reply.py create mode 100644 test/test_common_prefs_uncertain_reply.py diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index ba095513e..a63f28a86 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -360,6 +360,8 @@ jobs: python3 -B test/test_companion_radio_gain_restore.py -v python3 -B test/test_common_radio_persistence.py -v python3 -B test/test_common_cli_save_results.py -v + python3 -B test/test_common_prefs_uncertain_reply.py -v + python3 -B test/test_common_prefs_apply_reply.py -v python3 -B test/test_common_prefs_commit.py -v python3 -B test/test_common_prefs_legacy_rxgain.py -v python3 -B test/test_flood_policy_capacity.py -v diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 47f3ce4e4..b3d1890e5 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -2852,6 +2852,9 @@ void CommonCLI::savePrefs(PrefsSaveRouting::Scope scope) { } } +// Failure does not prove the previous image is still on disk: publication can +// succeed before readback fails. Callers can restore runtime values, but must +// describe the durable save as unconfirmed rather than promise disk rollback. bool CommonCLI::trySavePrefs(PrefsSaveRouting::Scope scope) { const PrefsSaveRouting::Plan plan = PrefsSaveRouting::planFor(scope); const uint8_t old_advert_interval = _prefs->advert_interval; @@ -3242,7 +3245,7 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re StrHelper::strncpy(_prefs->password, &command[9], sizeof(_prefs->password)); if (!trySavePrefs()) { memcpy(_prefs->password, previous, sizeof(previous)); - strcpy(reply, "Error: password not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } sprintf(reply, "password now: %s", _prefs->password); // echo back just to let admin know for sure!! @@ -3827,7 +3830,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->trace_when_repeat_off = strcmp(value, "on") == 0 ? 1 : 0; if (!trySavePrefs()) { _prefs->trace_when_repeat_off = previous; - strcpy(reply, "Error: Repeat trace not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } _callbacks->onRetryConfigChanged(); @@ -3865,7 +3868,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->gps_sync_interval_hours = hours; if (!trySavePrefs()) { _prefs->gps_sync_interval_hours = previous; - strcpy(reply, "Error: GPS sync interval could not be saved"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); } else { _sensors->applyGpsTimeSyncInterval(hours); snprintf(reply, 160, "OK - GPS sync interval %u hours (saved)", (unsigned)hours); @@ -3923,7 +3926,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->usb_logging_enabled = enabled ? 1 : 0; if (!trySavePrefs()) { _prefs->usb_logging_enabled = previous; - strcpy(reply, "Error: USB logging not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } // Commit before changing the stream's live protocol ownership. @@ -3945,7 +3948,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->usb_debug_enabled = enabled ? 1 : 0; if (!trySavePrefs()) { _prefs->usb_debug_enabled = previous; - strcpy(reply, "Error: USB debug not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } mesh::setUsbDebugEnabled(enabled); @@ -3978,7 +3981,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep if (!trySavePrefs()) { _prefs->usb_logging_enabled = previous_usb; _prefs->bridge_enabled = previous_bridge; - strcpy(reply, "Error: logging output not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } mesh::setUsbLoggingEnabled(usb_enabled); @@ -4053,7 +4056,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep // intent first so a failed write leaves the running service unchanged. if (!trySavePrefs()) { _prefs->bridge_enabled = previous; - strcpy(reply, "Error: MQTT setting not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } const bool applied = enable ? _callbacks->setMqttBridgeState(true) @@ -4075,7 +4078,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->rs232_heartbeat = enable ? 1 : 0; if (!trySavePrefs()) { _prefs->rs232_heartbeat = previous; - strcpy(reply, "Error: RS232 heartbeat not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); } else { strcpy(reply, enable ? "OK - RS232 heartbeat enabled" : "OK - heartbeat and standby disabled"); } @@ -4092,7 +4095,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->rs232_autosleep = force ? 2 : enable ? 1 : 0; if (!trySavePrefs()) { _prefs->rs232_autosleep = previous; - strcpy(reply, "Error: RS232 autosleep not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); } else { strcpy(reply, force ? "OK - forced standby; stock/unknown peers can miss packets" : enable ? "OK - standby after peer heartbeat confirmed; stock stays active" @@ -4112,7 +4115,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->rs232_idle_seconds = static_cast(seconds); if (!trySavePrefs()) { _prefs->rs232_idle_seconds = previous; - strcpy(reply, "Error: RS232 idle not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); } else { sprintf(reply, "OK - RS232 idle %us", static_cast(seconds)); } @@ -4147,9 +4150,11 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep // Saved-on may currently be in soft standby. Restore an owner as well // as its intent, otherwise no object remains to execute the wake timer. const bool restored = _callbacks->setRs232BridgeState(previous_enabled != 0); - strcpy(reply, restored - ? "Error: RS232 change failed; setting unchanged" - : "Error: RS232 change failed; previous runtime could not be restored"); + strcpy(reply, applied + ? (restored ? "Error: save unconfirmed; reboot may differ" + : "Error: save unconfirmed; runtime restore failed; reboot may differ") + : (restored ? "Error: RS232 change failed; setting unchanged" + : "Error: RS232 change failed; previous runtime could not be restored")); } else { strcpy(reply, enable && _sensors->gpsUsesSerialUart(_prefs->bridge_uart) ? "OK - UART GPS paused" : "OK"); @@ -4313,7 +4318,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->flood_advert_interval = (uint8_t)(hours); if (!trySavePrefs()) { _prefs->flood_advert_interval = previous; - strcpy(reply, "Error: interval not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } _callbacks->updateFloodAdvertTimer(); @@ -4329,7 +4334,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep _prefs->advert_interval = (uint8_t)(mins / 2); if (!trySavePrefs()) { _prefs->advert_interval = previous; - strcpy(reply, "Error: interval not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } _callbacks->updateAdvertTimer(); @@ -4341,7 +4346,7 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep StrHelper::strncpy(_prefs->guest_password, &config[15], sizeof(_prefs->guest_password)); if (!trySavePrefs()) { memcpy(_prefs->guest_password, previous, sizeof(previous)); - strcpy(reply, "Error: guest password not saved; unchanged"); + strcpy(reply, "Error: save unconfirmed; reboot may differ"); return; } strcpy(reply, "OK"); @@ -5103,9 +5108,11 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep // non-running heap-backed bridge instance. restored = _callbacks->setRs232BridgeState(previous_running); } - strcpy(reply, restored - ? "Error: bridge state change failed; setting unchanged" - : "Error: bridge state change failed; previous runtime state could not be restored"); + strcpy(reply, applied + ? (restored ? "Error: save unconfirmed; reboot may differ" + : "Error: save unconfirmed; runtime restore failed; reboot may differ") + : (restored ? "Error: bridge state change failed; setting unchanged" + : "Error: bridge state change failed; previous runtime state could not be restored")); } #else // MQTT and ESP-NOW preserve configured intent across missing WiFi @@ -5176,9 +5183,11 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep const bool restored = previous_running ? _callbacks->restartRs232Bridge() : _callbacks->setRs232BridgeState(false); - strcpy(reply, restored - ? "Error: bridge failed to restart; baud unchanged" - : "Error: bridge failed to restart; previous baud could not be restored"); + strcpy(reply, applied + ? (restored ? "Error: save unconfirmed; reboot may differ" + : "Error: save unconfirmed; runtime restore failed; reboot may differ") + : (restored ? "Error: bridge failed to restart; baud unchanged" + : "Error: bridge failed to restart; previous baud could not be restored")); } } else { sprintf(reply, "Error: baud rate must be between 9600-%d",BRIDGE_MAX_BAUD); @@ -5231,9 +5240,11 @@ void CommonCLI::handleSetCmd(uint32_t sender_timestamp, char* command, char* rep const bool restored = previous_running ? _callbacks->restartRs232Bridge() : _callbacks->setRs232BridgeState(false); - strcpy(reply, restored - ? "Error: bridge failed to restart; UART unchanged" - : "Error: bridge failed to restart; previous UART could not be restored"); + strcpy(reply, applied + ? (restored ? "Error: save unconfirmed; reboot may differ" + : "Error: save unconfirmed; runtime restore failed; reboot may differ") + : (restored ? "Error: bridge failed to restart; UART unchanged" + : "Error: bridge failed to restart; previous UART could not be restored")); } else if ( #ifdef WITH_MQTT_BRIDGE _prefs->rs232_bridge_enabled diff --git a/src/helpers/CommonCLI_Observer.cpp b/src/helpers/CommonCLI_Observer.cpp index 68347c597..811943c4f 100644 --- a/src/helpers/CommonCLI_Observer.cpp +++ b/src/helpers/CommonCLI_Observer.cpp @@ -1326,7 +1326,7 @@ bool CommonCLI::handleObserverCommand(uint32_t sender_timestamp, char* command, _prefs->ota_channel = ch; if (!saveCommonPrefs()) { _prefs->ota_channel = previous; - strcpy(reply, "ERR: OTA channel save failed; setting unchanged"); + strcpy(reply, "ERR: save unconfirmed; reboot may differ"); } else { snprintf(reply, 160, "channel set to %s, base %s; run ota update to switch", ota_channel_name(ch), ota_resolve_base(ch)); diff --git a/src/helpers/PeripheralModePrefs.h b/src/helpers/PeripheralModePrefs.h index 07ee5fff7..868a9d128 100644 --- a/src/helpers/PeripheralModePrefs.h +++ b/src/helpers/PeripheralModePrefs.h @@ -92,7 +92,7 @@ bool handleCommand(uint8_t& shared, const char* command, char* reply, shared = strcmp(value, "shared") == 0 ? 1 : 0; if (!save()) { shared = previous; - snprintf(reply, capacity, "Error: peripheral mode could not be saved"); + snprintf(reply, capacity, "Error: save unconfirmed; reboot may differ"); } else { snprintf(reply, capacity, "OK - %s saved; reboot to apply", value); } diff --git a/src/helpers/PrefsSaveReplyGuard.h b/src/helpers/PrefsSaveReplyGuard.h index 0619421c2..c290c5bcc 100644 --- a/src/helpers/PrefsSaveReplyGuard.h +++ b/src/helpers/PrefsSaveReplyGuard.h @@ -15,7 +15,7 @@ public: : _failures(failures), _start(failures), _reply(reply) {} ~PrefsSaveReplyGuard() { if (_failures != _start) { - strcpy(_reply, "Error: settings not saved; unsaved changes may remain active until reboot"); + strcpy(_reply, "Error: save unconfirmed; changes may be active; reboot may differ"); } } }; diff --git a/test/fixtures/common_cli_save_results/main.cpp b/test/fixtures/common_cli_save_results/main.cpp index 2ba8697c6..c4b2b2421 100644 --- a/test/fixtures/common_cli_save_results/main.cpp +++ b/test/fixtures/common_cli_save_results/main.cpp @@ -112,7 +112,7 @@ int main() { cli.callbacks.common_ok = false; if (guest) cli.set("guest.password new-guest", reply); else cli.password("password new-admin", reply); - assert(error(reply) && strstr(reply, "unchanged")); + assert(error(reply) && strstr(reply, "unconfirmed") && strstr(reply, "reboot")); assert(strcmp(cli.prefs.password, "old-admin") == 0); assert(strcmp(cli.prefs.guest_password, "old-guest") == 0); assert(cli.prefs.advert_interval == 2 && cli.callbacks.local_updates == 0); @@ -143,7 +143,7 @@ int main() { assert(error(reply) && cli.callbacks.saves == 0); cli.callbacks.common_ok = false; cli.set((std::string(key) + (flood ? "3" : "120")).c_str(), reply); - assert(error(reply) && strstr(reply, "unchanged")); + assert(error(reply) && strstr(reply, "unconfirmed") && strstr(reply, "reboot")); assert(cli.prefs.advert_interval == 30 && cli.prefs.flood_advert_interval == 12); assert(cli.callbacks.local_updates == 0 && cli.callbacks.flood_updates == 0); cli.callbacks.common_ok = true; @@ -188,7 +188,8 @@ int main() { cli.prefs.dirty = false; cli.callbacks.common_ok = false; cli.set("multi.acks 2", reply); - assert(error(reply) && strstr(reply, "unsaved changes")); + assert(error(reply) && strstr(reply, "unconfirmed") && strstr(reply, "may be active") + && strstr(reply, "reboot may differ")); assert(cli.prefs.multi_acks == 2 && cli.callbacks.persisted.multi_acks == 0); assert(cli.prefs.dirty && cli._prefs_save_failures == 1); cli.callbacks.common_ok = true; diff --git a/test/fixtures/common_prefs_apply_reply.cpp b/test/fixtures/common_prefs_apply_reply.cpp new file mode 100644 index 000000000..8e0c4d38b --- /dev/null +++ b/test/fixtures/common_prefs_apply_reply.cpp @@ -0,0 +1,320 @@ +// Insert only actual production parsing and the four bounded CLI branches. +#include +#include +#include +#include +#include +#include +#include + +#define BRIDGE_MAX_BAUD 115200 + +struct NodePrefs { + uint8_t bridge_enabled = 0; + uint8_t rs232_bridge_enabled = 0; + uint32_t bridge_baud = 9600; + uint8_t bridge_uart = 1; + uint8_t espnow_bridge_enabled = 1; + bool operator==(const NodePrefs& other) const { + return bridge_enabled == other.bridge_enabled + && rs232_bridge_enabled == other.rs232_bridge_enabled + && bridge_baud == other.bridge_baud && bridge_uart == other.bridge_uart + && espnow_bridge_enabled == other.espnow_bridge_enabled; + } +}; + +struct Event { + std::string kind; + bool enabled; + NodePrefs prefs; +}; + +struct Sensors { + bool gps = false; + bool conflict = false; + bool yield = true; + bool gpsUsesSerialUart(uint8_t) const { return gps; } + bool gpsSerialTransportMayConflict(uint8_t) const { return conflict; } + bool gpsSerialTransportCanYield(uint8_t) const { return yield; } +}; + +struct Callbacks { + NodePrefs* prefs; + std::vector events; + std::vector responses; + size_t response_index = 0; + bool running = false; + bool save_result = true; + unsigned save_calls = 0; + unsigned running_queries = 0; + + bool result(const char* kind, bool enabled) { + events.push_back({kind, enabled, *prefs}); + assert(response_index < responses.size()); + const bool accepted = responses[response_index++]; + // A failed apply can have torn down its previous runtime owner. This + // forces rollback rather than accidentally hiding that branch in tests. + running = accepted && enabled; + return accepted; + } + bool setRs232BridgeState(bool enabled) { return result("state", enabled); } + bool restartRs232Bridge() { return result("restart", true); } + bool isRs232BridgeRunning() { ++running_queries; return running; } + bool save() { + events.push_back({"save", false, *prefs}); + ++save_calls; + return save_result; + } +}; + +struct CommonCLI { + NodePrefs* _prefs; + Callbacks* _callbacks; + Sensors* _sensors; + bool trySavePrefs() { return _callbacks->save(); } + void rs232(const char*, char*); + void bridge(const char*, char*); + void baud(const char*, char*); + void uart(const char*, char*); +}; + +@PARSE_ON_OFF@ +@PRODUCTION_BRANCHES@ + +enum class Branch { Rs232, Bridge, Baud, Uart }; + +static bool isEnable(Branch branch) { + return branch == Branch::Rs232 || branch == Branch::Bridge; +} + +static uint8_t& intent(NodePrefs& prefs) { +#ifdef WITH_MQTT_BRIDGE + return prefs.rs232_bridge_enabled; +#else + return prefs.bridge_enabled; +#endif +} + +static NodePrefs initial(bool enabled) { + NodePrefs prefs; + intent(prefs) = enabled; +#ifdef WITH_MQTT_BRIDGE + prefs.bridge_enabled = !enabled; // Independent MQTT intent must survive. +#else + prefs.rs232_bridge_enabled = enabled; +#endif + return prefs; +} + +static void run(CommonCLI& cli, Branch branch, bool enabled, char* reply, + bool unchanged = false) { + switch (branch) { + case Branch::Rs232: + cli.rs232(enabled ? "rs232.enabled on" : "rs232.enabled off", reply); + break; + case Branch::Bridge: +#ifndef WITH_MQTT_BRIDGE + cli.bridge(enabled ? "bridge.enabled on" : "bridge.enabled off", reply); +#else + assert(false && "standalone bridge branch is not an MQTT command"); +#endif + break; + case Branch::Baud: + cli.baud(unchanged ? "bridge.baud 9600" : "bridge.baud 19200", reply); + break; + case Branch::Uart: + cli.uart(unchanged ? "bridge.uart 1" : "bridge.uart 2", reply); + break; + } +} + +static NodePrefs candidateFor(NodePrefs before, Branch branch, bool enabled, + bool unchanged = false) { + if (isEnable(branch)) intent(before) = enabled; + else if (branch == Branch::Baud) before.bridge_baud = unchanged ? 9600 : 19200; + else before.bridge_uart = unchanged ? 1 : 2; + return before; +} + +static const char* failedReply(Branch branch, bool applied, bool restored) { + if (applied) { + return restored ? "Error: save unconfirmed; reboot may differ" + : "Error: save unconfirmed; runtime restore failed; reboot may differ"; + } + switch (branch) { + case Branch::Rs232: + return restored ? "Error: RS232 change failed; setting unchanged" + : "Error: RS232 change failed; previous runtime could not be restored"; + case Branch::Bridge: + return restored ? "Error: bridge state change failed; setting unchanged" + : "Error: bridge state change failed; previous runtime state could not be restored"; + case Branch::Baud: + return restored ? "Error: bridge failed to restart; baud unchanged" + : "Error: bridge failed to restart; previous baud could not be restored"; + case Branch::Uart: + return restored ? "Error: bridge failed to restart; UART unchanged" + : "Error: bridge failed to restart; previous UART could not be restored"; + } + assert(false); + return ""; +} + +static unsigned cases = 0; + +static void matrixCase(Branch branch, bool previous, bool applied, bool saved, + bool restored) { + NodePrefs prefs = initial(previous); + const NodePrefs before = prefs; + const bool requested = !previous; + const NodePrefs candidate = candidateFor(before, branch, requested); + Callbacks callbacks{&prefs, {}, {applied, restored}}; + callbacks.running = previous; + callbacks.save_result = saved; + Sensors sensors; + CommonCLI cli{&prefs, &callbacks, &sensors}; + char reply[160] = {}; + run(cli, branch, requested, reply); + assert(callbacks.save_calls == (applied ? 1u : 0u)); + assert(callbacks.events[0].kind == (isEnable(branch) ? "state" : "restart")); + assert(callbacks.events[0].prefs == candidate); + if (isEnable(branch)) assert(callbacks.events[0].enabled == requested); + if (applied) { + assert(callbacks.events[1].kind == "save"); + assert(callbacks.events[1].prefs == candidate); + } + if (applied && saved) { + assert(!strcmp(reply, "OK")); + assert(prefs == candidate); + assert(callbacks.events.size() == 2); + assert(callbacks.response_index == 1); + } else { + assert(!strcmp(reply, failedReply(branch, applied, restored))); + assert(prefs == before); + assert(callbacks.events.size() == (applied ? 3u : 2u)); + const Event& rollback = callbacks.events.back(); + assert(rollback.kind == (isEnable(branch) ? "state" : "restart")); + assert(rollback.prefs == before); + assert(rollback.enabled == (isEnable(branch) ? previous : true)); + assert(callbacks.response_index == 2); + if (applied) { + assert(strstr(reply, "unconfirmed")); + assert(strstr(reply, "reboot may differ")); + assert(!strstr(reply, "unchanged")); + } else { + assert(!strstr(reply, "unconfirmed")); + } + } + ++cases; +} + +static void disabledSettingCase(Branch branch, bool saved, bool restored) { + NodePrefs prefs = initial(false); + const NodePrefs before = prefs; + const NodePrefs candidate = candidateFor(before, branch, false); + Callbacks callbacks{&prefs, {}, {restored}}; + callbacks.save_result = saved; + Sensors sensors; + CommonCLI cli{&prefs, &callbacks, &sensors}; + char reply[160] = {}; + run(cli, branch, false, reply); + // A disabled bridge needs no runtime restart to apply UART/baud changes. + assert(callbacks.save_calls == 1); + assert(callbacks.events[0].kind == "save"); + assert(callbacks.events[0].prefs == candidate); + if (saved) { + assert(!strcmp(reply, "OK")); + assert(prefs == candidate); + assert(callbacks.events.size() == 1); + assert(callbacks.response_index == 0); + } else { + assert(!strcmp(reply, failedReply(branch, true, restored))); + assert(prefs == before); + assert(callbacks.events.size() == 2); + assert(callbacks.events[1].kind == "state"); + assert(!callbacks.events[1].enabled); + assert(callbacks.events[1].prefs == before); + assert(callbacks.response_index == 1); + } + ++cases; +} + +static void healthyCase(Branch branch, bool previous, bool gps, bool unchanged) { + NodePrefs prefs = initial(previous); + const NodePrefs before = prefs; + const bool requested = unchanged ? previous : !previous; + const NodePrefs candidate = candidateFor(before, branch, requested, unchanged); + const bool applies = isEnable(branch) || previous; + Callbacks callbacks{&prefs, {}, {true}}; + callbacks.running = previous; + Sensors sensors; + sensors.gps = gps; + CommonCLI cli{&prefs, &callbacks, &sensors}; + char reply[160] = {}; + run(cli, branch, requested, reply, unchanged); + const bool paused = gps && ((isEnable(branch) && requested) + || (branch == Branch::Uart && previous)); + assert(!strcmp(reply, paused ? "OK - UART GPS paused" : "OK")); + assert(prefs == candidate); + assert(callbacks.save_calls == 1); + assert(callbacks.events.size() == (applies ? 2u : 1u)); + assert(callbacks.response_index == (applies ? 1u : 0u)); + for (const Event& event : callbacks.events) assert(event.prefs == candidate); + ++cases; +} + +#ifndef WITH_MQTT_BRIDGE +static void alreadyRestoredCase() { + NodePrefs prefs = initial(true); + const NodePrefs before = prefs; + Callbacks callbacks{&prefs, {}, {true}}; + callbacks.running = true; + callbacks.save_result = false; + Sensors sensors; + CommonCLI cli{&prefs, &callbacks, &sensors}; + char reply[160] = {}; + // A successfully applied same-value ON leaves the old runtime running. + // A refused disable also leaves it stopped in this adapter and must call + // the restoration callback instead, as covered by matrixCase above. + run(cli, Branch::Bridge, true, reply, true); + assert(!strcmp(reply, "Error: save unconfirmed; reboot may differ")); + assert(prefs == before); + assert(callbacks.events.size() == 2); + assert(callbacks.response_index == 1); + assert(callbacks.running_queries == 1); + ++cases; +} +#endif + +int main() { + const Branch branches[] = {Branch::Rs232, +#ifndef WITH_MQTT_BRIDGE + Branch::Bridge, +#endif + Branch::Baud, Branch::Uart}; + for (Branch branch : branches) { + for (bool applied : {false, true}) { + for (bool saved : {false, true}) { + for (bool restored : {false, true}) { + matrixCase(branch, true, applied, saved, restored); + if (isEnable(branch)) matrixCase(branch, false, applied, saved, restored); + } + } + } + for (bool previous : {false, true}) { + for (bool gps : {false, true}) { + for (bool unchanged : {false, true}) { + healthyCase(branch, previous, gps, unchanged); + } + } + } + } + for (Branch branch : {Branch::Baud, Branch::Uart}) { + for (bool saved : {false, true}) { + for (bool restored : {false, true}) disabledSettingCase(branch, saved, restored); + } + } +#ifndef WITH_MQTT_BRIDGE + alreadyRestoredCase(); +#endif + printf("%u actual CommonCLI apply/save/restore cases passed\n", cases); +} diff --git a/test/fixtures/common_prefs_uncertain_reply/main.cpp b/test/fixtures/common_prefs_uncertain_reply/main.cpp new file mode 100644 index 000000000..b48b66207 --- /dev/null +++ b/test/fixtures/common_prefs_uncertain_reply/main.cpp @@ -0,0 +1,209 @@ + +// Included after the extracted production CommonCLI methods and serializer. +#include + +static const char* oldSecret(bool guest) { return guest ? "old-guest" : "old-admin"; } +static const char* newSecret(bool guest) { return guest ? "new-guest" : "new-admin"; } +static const char* retrySecret(bool guest) { return guest ? "retry-guest" : "retry-admin"; } +static const char* currentSecret(const CommonCLI& cli, bool guest) { + return guest ? cli.prefs.guest_password : cli.prefs.password; +} + +static void seed(CommonCLI& cli) { + strcpy(cli.prefs.password, oldSecret(false)); + strcpy(cli.prefs.guest_password, oldSecret(true)); + cli.savePrefs(&cli.fs, PrefsSaveRouting::Scope::Common); + assert(cli._common_save_result_known && cli._common_save_succeeded); +} + +static void change(CommonCLI& cli, bool guest, const char* secret, char* reply) { + char command[80] = {}; + snprintf(command, sizeof(command), "%s%s", guest ? "guest.password " : "password ", secret); + memset(reply, 0, 160); + if (guest) cli.guestPassword(command, reply); + else cli.password(command, reply); +} + +static void assertError(const char* reply, bool guest, const char* attempted) { + assert(strncmp(reply, "Error:", 6) == 0); + assert(strstr(reply, "unconfirmed") != nullptr); + assert(strstr(reply, "reboot") != nullptr); + assert(strstr(reply, "unchanged") == nullptr); + assert(strstr(reply, "OK") == nullptr); + // Neither an old credential nor any attempted credential belongs in an + // error, even though the successful admin command intentionally echoes it. + for (const char* secret : {oldSecret(false), oldSecret(true), + newSecret(false), newSecret(true), attempted}) { + assert(strstr(reply, secret) == nullptr); + } + (void)guest; +} + +static void assertSuccess(const CommonCLI& cli, bool guest, + const char* secret, const char* reply) { + assert(cli._common_save_result_known && cli._common_save_succeeded); + assert(strcmp(currentSecret(cli, guest), secret) == 0); + if (guest) assert(strcmp(reply, "OK") == 0); + else { + char expected[80]; + snprintf(expected, sizeof(expected), "password now: %s", secret); + assert(strcmp(reply, expected) == 0); + } +} + +static void assertReboot(const CommonCLI& cli, bool guest, const char* expected) { + CommonCLI reboot; + reboot.fs.files = cli.fs.files; + reboot.loadPrefsInt(&reboot.fs, "/com_prefs"); + assert(strcmp(currentSecret(reboot, guest), expected) == 0); + assert(strcmp(currentSecret(reboot, !guest), oldSecret(!guest)) == 0); +} + +static void clearFaults(CommonCLI& cli) { + cli.fs.fail_write = false; + cli.fs.fail_write_after = -1; + cli.fs.fail_read_open = false; + cli.fs.fail_after_publication = false; + cli.fs.fail_rename = 0; +} + +static void assertLegacyError(const CommonCLI& cli, const char* reply) { + assert(!cli._common_save_succeeded && cli._prefs_save_failures == 1); + assert(cli.prefs.dirty); + assert(strncmp(reply, "Error:", 6) == 0); + assert(strstr(reply, "unconfirmed") != nullptr); + assert(strstr(reply, "active") != nullptr); + assert(strstr(reply, "reboot") != nullptr); + assert(strstr(reply, "unchanged") == nullptr && strstr(reply, "OK") == nullptr); + assert(strstr(reply, "unsaved") == nullptr); +} + +static void assertLegacyReboot(const CommonCLI& cli, uint8_t expected) { + CommonCLI reboot; + reboot.fs.files = cli.fs.files; + reboot.loadPrefsInt(&reboot.fs, "/com_prefs"); + assert(reboot.prefs.multi_acks == expected); + assert(strcmp(reboot.prefs.password, oldSecret(false)) == 0); + assert(strcmp(reboot.prefs.guest_password, oldSecret(true)) == 0); +} + +int main() { + unsigned scenarios = 0; + for (bool guest : {false, true}) { + char reply[160] = {}; + CommonCLI healthy; + seed(healthy); + const auto previous = healthy.fs.files.at("/com_prefs"); + change(healthy, guest, newSecret(guest), reply); + assertSuccess(healthy, guest, newSecret(guest), reply); + assertReboot(healthy, guest, newSecret(guest)); + const auto candidate = healthy.fs.files.at("/com_prefs"); + assert(candidate != previous && candidate.size() == previous.size()); + ++scenarios; + + // Each ordinary rejection leaves the old image on disk. The bool API + // cannot promise that for every false result, so its reply stays cautious. + for (unsigned fault = 0; fault != 4; ++fault) { + CommonCLI cli; + seed(cli); + if (fault == 0) cli.fs.fail_write = true; + if (fault == 1) cli.fs.fail_write_after = 64; + if (fault == 2) cli.fs.fail_read_open = true; + if (fault == 3) cli.fs.fail_rename = 1; + change(cli, guest, newSecret(guest), reply); + assert(!cli._common_save_succeeded); + assertError(reply, guest, newSecret(guest)); + assert(strcmp(currentSecret(cli, guest), oldSecret(guest)) == 0); + assert(cli.fs.files.at("/com_prefs") == previous); + clearFaults(cli); + assertReboot(cli, guest, oldSecret(guest)); + change(cli, guest, retrySecret(guest), reply); + assertSuccess(cli, guest, retrySecret(guest), reply); + assertReboot(cli, guest, retrySecret(guest)); + ++scenarios; + } + +#if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) + // Atomic publication succeeded, then every public-image readback failed. + // The real writer returns false/Indeterminate, runtime credentials roll + // back, but fresh boot must read the new durable credential. An "unchanged" + // promise here is both false and dangerous for remote administration. + { + CommonCLI cli; + seed(cli); + cli.fs.fail_after_publication = true; + change(cli, guest, newSecret(guest), reply); + assert(!cli._common_save_succeeded); + assertError(reply, guest, newSecret(guest)); + assert(strcmp(currentSecret(cli, guest), oldSecret(guest)) == 0); + assert(cli.fs.files.at("/com_prefs") == candidate); + clearFaults(cli); + assertReboot(cli, guest, newSecret(guest)); + change(cli, guest, retrySecret(guest), reply); + assertSuccess(cli, guest, retrySecret(guest), reply); + assertReboot(cli, guest, retrySecret(guest)); + ++scenarios; + } +#endif + } + + // Legacy setters retain their live changes after save failure. Exercise the + // actual outer reply guard so the branch's final OK cannot hide either + // known rejection or post-publication uncertainty. + { + char reply[160] = {}; + CommonCLI healthy; + seed(healthy); + assert(healthy.prefs.multi_acks == 0); + const auto previous = healthy.fs.files.at("/com_prefs"); + healthy.legacySet("multi.acks 1", reply); + assert(strcmp(reply, "OK") == 0 && healthy.prefs.multi_acks == 1); + assert(healthy._common_save_succeeded && healthy._prefs_save_failures == 0); + assert(!healthy.prefs.dirty); + assertLegacyReboot(healthy, 1); + const auto candidate = healthy.fs.files.at("/com_prefs"); + assert(candidate != previous && candidate.size() == previous.size()); + ++scenarios; + + for (unsigned fault = 0; fault != 4; ++fault) { + CommonCLI cli; + seed(cli); + if (fault == 0) cli.fs.fail_write = true; + if (fault == 1) cli.fs.fail_write_after = 64; + if (fault == 2) cli.fs.fail_read_open = true; + if (fault == 3) cli.fs.fail_rename = 1; + cli.legacySet("multi.acks 1", reply); + assertLegacyError(cli, reply); + assert(cli.prefs.multi_acks == 1); + assert(cli.fs.files.at("/com_prefs") == previous); + clearFaults(cli); + assertLegacyReboot(cli, 0); + // Retry the same live value: an unchanged assignment must still save. + cli.legacySet("multi.acks 1", reply); + assert(strcmp(reply, "OK") == 0 && cli._common_save_succeeded); + assert(cli._prefs_save_failures == 1 && !cli.prefs.dirty); + assertLegacyReboot(cli, 1); + ++scenarios; + } + +#if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) + { + CommonCLI cli; + seed(cli); + cli.fs.fail_after_publication = true; + cli.legacySet("multi.acks 1", reply); + assertLegacyError(cli, reply); + assert(cli.prefs.multi_acks == 1); + assert(cli.fs.files.at("/com_prefs") == candidate); + clearFaults(cli); + assertLegacyReboot(cli, 1); + cli.legacySet("multi.acks 0", reply); + assert(strcmp(reply, "OK") == 0 && cli._common_save_succeeded); + assert(cli._prefs_save_failures == 1 && !cli.prefs.dirty); + assertLegacyReboot(cli, 0); + ++scenarios; + } +#endif + } + printf("%u actual CommonCLI save scenarios passed\n", scenarios); +} diff --git a/test/fixtures/rs232_mqtt_runtime.cpp b/test/fixtures/rs232_mqtt_runtime.cpp index f6fae3b13..5f495cddb 100644 --- a/test/fixtures/rs232_mqtt_runtime.cpp +++ b/test/fixtures/rs232_mqtt_runtime.cpp @@ -373,18 +373,18 @@ int main() { assert(cli.saves == disabled_saves && mesh.isMqttBridgeRunning() && mesh.isEspNowBridgeRunning()); cli.fail_save = true; cli.set("rs232.enabled on", reply); - assert(strstr(reply, "setting unchanged") && !mesh.bridge && !mesh._prefs.rs232_bridge_enabled); + assert(strstr(reply, "unconfirmed") && strstr(reply, "reboot") && !mesh.bridge && !mesh._prefs.rs232_bridge_enabled); assert(cli.saves == disabled_saves && mesh.isMqttBridgeRunning() && mesh.isEspNowBridgeRunning()); cli.fail_save = false; cli.set("rs232.enabled on", reply); assert(strcmp(reply, "OK") == 0); const unsigned saved_after_enable = cli.saves; cli.fail_save = true; cli.set("rs232.enabled off", reply); - assert(strstr(reply, "setting unchanged") && mesh.isRs232BridgeRunning() && mesh._prefs.rs232_bridge_enabled); + assert(strstr(reply, "unconfirmed") && strstr(reply, "reboot") && mesh.isRs232BridgeRunning() && mesh._prefs.rs232_bridge_enabled); cli.set("bridge.baud 19200", reply); - assert(strstr(reply, "baud unchanged") && mesh._prefs.bridge_baud == 57600 && Serial2.baud == 57600); + assert(strstr(reply, "unconfirmed") && strstr(reply, "reboot") && mesh._prefs.bridge_baud == 57600 && Serial2.baud == 57600); cli.set("bridge.uart 2", reply); - assert(strstr(reply, "UART unchanged") && mesh._prefs.bridge_uart == 2 && mesh.isRs232BridgeRunning()); + assert(strstr(reply, "unconfirmed") && strstr(reply, "reboot") && mesh._prefs.bridge_uart == 2 && mesh.isRs232BridgeRunning()); assert(cli.saves == saved_after_enable && mesh.mqtt_bridge->begins == wifi_starts); assert(mesh.espnow_bridge.begins == wireless_starts); cli.fail_save = false; @@ -405,7 +405,7 @@ int main() { // A failed intent save must not signal an uncancellable worker stop. cli.fail_save = true; cli.set("mqtt.enabled off", reply); - assert(strstr(reply, "not saved; unchanged") && mesh._prefs.bridge_enabled); + assert(strstr(reply, "unconfirmed") && strstr(reply, "reboot") && mesh._prefs.bridge_enabled); assert(mesh.isMqttBridgeRunning() && !mesh.isMqttBridgeStopping()); cli.fail_save = false; cli.set("mqtt.enabled off", reply); @@ -427,7 +427,7 @@ int main() { expect_get(cli, "mqtt.stopping", "> off"); cli.fail_save = true; cli.set("mqtt.enabled on", reply); - assert(strstr(reply, "not saved; unchanged") && !mesh._prefs.bridge_enabled); + assert(strstr(reply, "unconfirmed") && strstr(reply, "reboot") && !mesh._prefs.bridge_enabled); assert(!mesh.isMqttBridgeRunning()); cli.fail_save = false; cli.set("espnow.enabled on", reply); assert(strcmp(reply, "OK") == 0); diff --git a/test/test_common_cli_logging_transaction.py b/test/test_common_cli_logging_transaction.py index e69937966..757fdf539 100644 --- a/test/test_common_cli_logging_transaction.py +++ b/test/test_common_cli_logging_transaction.py @@ -57,7 +57,8 @@ static void check(const char* command, bool usb, bool wifi, bool failed) { assert(cli.persisted.usb_logging_enabled == (failed ? !usb : usb)); assert(live_usb == (failed ? !usb : usb)); assert(usb_calls == (failed ? 0u : 1u)); - assert(strstr(reply, failed ? "unchanged" : "(saved)")); + assert(strstr(reply, failed ? "unconfirmed" : "(saved)")); + if (failed) assert(strstr(reply, "reboot may differ")); if (strncmp(command, "logging.output", 14) == 0) { assert(cli.prefs.bridge_enabled == (failed ? !wifi : wifi)); assert(cli.persisted.bridge_enabled == (failed ? !wifi : wifi)); diff --git a/test/test_common_prefs_apply_reply.py b/test/test_common_prefs_apply_reply.py new file mode 100644 index 000000000..47cf9bb1f --- /dev/null +++ b/test/test_common_prefs_apply_reply.py @@ -0,0 +1,85 @@ +"""Run real RS-232 apply/save branches with controlled callback outcomes. + +The callbacks model application and save results, not filesystem transactions. +Actual post-publication storage failures are exercised separately by +test_common_prefs_uncertain_reply.py. +""" +from pathlib import Path +import os +import shutil +import subprocess +import sys +import tempfile +import unittest + +from test_replay_reset_integration import extract_braced + +ROOT = Path(__file__).resolve().parents[1] + + +def actual_harness(compiler, flags): + source = (ROOT / "src/helpers/CommonCLI.cpp").read_text(encoding="ascii") + # Preprocess the whole real method first: platform conditionals can + # straddle the closing brace between adjacent else-if commands. + selected = subprocess.run([ + compiler, "-E", "-P", "-x", "c++", *flags, "-", + ], input=extract_braced(source, "void CommonCLI::handleSetCmd("), + capture_output=True, text=True, check=True, timeout=20).stdout + branches = [] + for method, signature in ( + ("rs232", 'if (strncmp(config, "rs232.enabled ", 14) == 0)'), + ("bridge", 'if (memcmp(config, "bridge.enabled ", 15) == 0)'), + ("baud", 'if (memcmp(config, "bridge.baud ", 12) == 0)'), + ("uart", 'if (memcmp(config, "bridge.uart ", 12) == 0)'), + ): + definition = ("void CommonCLI::" + method + + "(const char* config, char* reply) {\n" + + extract_braced(selected, signature) + "\n}\n") + if method == "bridge": + # The strict apply/save bridge.enabled branch is standalone UART + # only. WITH_MQTT_BRIDGE keeps its independent transport policy. + definition = "#ifndef WITH_MQTT_BRIDGE\n" + definition + "#endif\n" + branches.append(definition) + fixture = (ROOT / "test/fixtures/common_prefs_apply_reply.cpp").read_text( + encoding="ascii") + return fixture.replace("@PARSE_ON_OFF@", extract_braced( + source, "static bool parseOnOffStrict(")).replace( + "@PRODUCTION_BRANCHES@", "\n".join(branches)) + + +class CommonPrefsApplyReplyTests(unittest.TestCase): + def test_actual_apply_save_rollback_combinations(self): + compiler = os.environ.get("CXX") or shutil.which("g++") or shutil.which("clang++") + self.assertIsNotNone(compiler, "a host C++17 compiler is required") + sanitizer_flags = (["-fsanitize=address,undefined", "-fno-sanitize-recover=all", + "-fno-pie", "-no-pie"] + if sys.platform.startswith("linux") else []) + with tempfile.TemporaryDirectory(prefix="common-prefs-apply-reply-") as directory: + work = Path(directory) + for mqtt in (False, True): + with self.subTest(mqtt=mqtt): + flags = ["-DWITH_BRIDGE=1", "-DWITH_RS232_BRIDGE=1", + "-DWITH_RS232_BRIDGE_UART=1", "-DWITH_RS232_BRIDGE_ALT=1", + "-DWITH_RS232_BRIDGE_ALT_UART=2"] + if mqtt: + flags.append("-DWITH_MQTT_BRIDGE=1") + (work / "test.cpp").write_text(actual_harness(compiler, flags), + encoding="ascii") + binary = work / ("test.exe" if os.name == "nt" else "test") + built = subprocess.run([ + compiler, "-std=c++17", "-Wall", "-Wextra", "-Werror", + *flags, *sanitizer_flags, "-I" + str(ROOT / "src"), + str(work / "test.cpp"), "-o", str(binary), + ], capture_output=True, text=True, timeout=60) + self.assertEqual(built.returncode, 0, built.stdout + built.stderr) + tested = subprocess.run([str(binary)], capture_output=True, + text=True, timeout=20) + self.assertEqual(tested.returncode, 0, tested.stdout + tested.stderr) + cases = 64 if mqtt else 89 + self.assertIn( + f"{cases} actual CommonCLI apply/save/restore cases passed", + tested.stdout) + + +if __name__ == "__main__": + unittest.main() diff --git a/test/test_common_prefs_uncertain_reply.py b/test/test_common_prefs_uncertain_reply.py new file mode 100644 index 000000000..ab99ac690 --- /dev/null +++ b/test/test_common_prefs_uncertain_reply.py @@ -0,0 +1,133 @@ +"""Exercise real CommonCLI password and legacy save uncertainty replies.""" +from pathlib import Path +import os +import re +import subprocess +import tempfile +import unittest + +from test_common_usb_debug import HARNESS +from test_replay_reset_integration import extract_braced + +ROOT = Path(__file__).resolve().parents[1] + + +def actual_harness(): + source = (ROOT / 'src/helpers/CommonCLI.cpp').read_text(encoding='ascii') + header = (ROOT / 'src/helpers/CommonCLI.h').read_text(encoding='ascii') + fields = header.split('class NodePrefs :', 1)[1].split('private:', 1)[0] + fields = '\n'.join(re.findall( + r'^\s*(?:float|double|char|u?int(?:8|16|32)_t)\s+[^;]+;', fields, re.M)) + constants = '\n'.join(re.findall( + r'^\s*#define\s+\w+\s+(?:0x[\dA-Fa-f]+|\d+)\s*$', header, re.M)) + constants += '\n' + '\n'.join(re.findall( + r'^static const size_t LEGACY_\w+\s*=[^;]+;', source, re.M)) + constants += '\n' + re.search( + r'^static constexpr uint8_t DEFAULT_ADVERT_LOC_POLICY\s*=[^;]+;', + header, re.M).group() + helpers = '\n'.join(extract_braced(source, signature) for signature in ( + 'static bool bwMatches(', 'static bool isValidLoRaBandwidth(', + 'static float defaultLoRaBandwidth(', 'static void markDirectRetryPrefsValid(', + 'static void applyFloodRetryPreset(', 'static void applyDirectRetryPreset(', + 'static void setDefaultDirectRetryPrefs(', 'static bool directRetryPrefsValid(')) + methods = '\n'.join(extract_braced(source, signature) for signature in ( + 'static const char* commonPrefsSaveResultName(', + 'bool CommonCLI::recoverCommonPrefsFiles(', + 'bool CommonCLI::saveCommonPrefsImageAtomically(', + 'void CommonCLI::savePrefs(FILESYSTEM*', + 'void CommonCLI::savePrefs(PrefsSaveRouting::Scope scope)', + 'bool CommonCLI::trySavePrefs(', 'void CommonCLI::loadPrefsInt(')) + + # Reuse the established adapter, but not its USB-specific main or dynamic + # schema tests. Production serializers and transaction code remain real. + code = HARNESS[:HARNESS.index('// Execute the production dynamic schema')] + code = code.replace(' void set(const char* config, char* reply) {', + ' void password(const char*, char*);\n' + ' void guestPassword(const char*, char*);\n' + ' void legacySet(const char*, char*);\n' + ' void set(const char* config, char* reply) {') + code = '#include \n#include \n' + code + text_helpers = (ROOT / 'src/helpers/TxtDataHelpers.cpp').read_text(encoding='ascii') + code = code.replace('struct CommonCLI;\n', + 'struct StrHelper { static void strncpy(char*, const char*, size_t); };\n' + + extract_braced(text_helpers, 'void StrHelper::strncpy(') + + '\nstruct CommonCLI;\n', 1) + for key, value in { + 'FIELDS': fields, 'CONSTANTS': constants, 'HELPERS': helpers, + 'LEGACY': extract_braced(header, 'struct LegacyObserverTail') + ';', + 'SERIALIZER': 'template\n' + extract_braced( + source, 'static bool writeCommonPrefsImage('), + 'STORE': extract_braced(source, 'class CommonPrefsFileStore'), + 'METHODS': methods, + 'SET_DEBUG': extract_braced(source, 'if (strncmp(config, "usb.debug", 9)'), + 'SET_MASTER': extract_braced(source, 'if (strncmp(config, "usb.logging", 11)'), + 'GET_DEBUG': extract_braced(source, 'if (strcmp(config, "usb.debug") == 0)'), + 'GET_MASTER': extract_braced(source, 'if (strcmp(config, "usb.logging") == 0)'), + }.items(): + code = code.replace('@' + key + '@', value) + code += '\nvoid CommonCLI::password(const char* command, char* reply) {\n' + code += extract_braced(source, 'if (memcmp(command, "password ", 9) == 0)') + '\n}\n' + code += '\nvoid CommonCLI::guestPassword(const char* config, char* reply) {\n' + code += extract_braced(source, 'if (memcmp(config, "guest.password ", 15) == 0)') + '\n}\n' + handle = extract_braced(source, 'void CommonCLI::handleCommand(') + guard = re.search(r'PrefsSaveReplyGuard save_reply\([^;]+;', handle).group() + code += '\nvoid CommonCLI::legacySet(const char* config, char* reply) {\n' + code += guard + '\n' + code += extract_braced(source, 'if (memcmp(config, "multi.acks ", 11) == 0)') + '\n}\n' + return code + (ROOT / 'test/fixtures/common_prefs_uncertain_reply/main.cpp').read_text( + encoding='ascii') + + +class CommonPrefsUncertainReplyTest(unittest.TestCase): + def test_actual_password_commands_and_durable_images(self): + code = actual_harness() + mock = (ROOT / 'test/fixtures/radio_profiles/mocks/helpers/IdentityStore.h').read_text( + encoding='ascii') + # Fail only after a real rename publishes the complete candidate. The + # real AtomicFileWriter still performs all its normal verification. + old_field = ' bool fail_read_open = false;' + self.assertEqual(mock.count(old_field), 1) + mock = mock.replace(old_field, old_field + '\n bool fail_after_publication = false;') + old_rename = ' files[to] = files[from]; files.erase(from); return true;' + self.assertEqual(mock.count(old_rename), 1) + mock = mock.replace(old_rename, + ' files[to] = files[from]; files.erase(from);\n' + ' if (fail_after_publication && !strcmp(to, "/com_prefs"))\n' + ' fail_read_open = true;\n' + ' return true;') + sanitizer_flags = [] if os.name == 'nt' else [ + '-fsanitize=address,undefined', '-fno-sanitize-recover=all', + '-fno-pie', '-no-pie'] + with tempfile.TemporaryDirectory(prefix='common-prefs-uncertain-') as directory: + work = Path(directory) + (work / 'helpers').mkdir() + (work / 'helpers/IdentityStore.h').write_text(mock, encoding='ascii') + # Preserve the production transaction body; redirect only its + # quoted filesystem include to the same injected adapter. + transaction = (ROOT / 'src/helpers/ContactFileTransaction.h').read_text( + encoding='ascii') + self.assertEqual(transaction.count('#include "IdentityStore.h"'), 1) + (work / 'ContactFileTransaction.h').write_text(transaction.replace( + '#include "IdentityStore.h"', '#include '), + encoding='ascii') + (work / 'test.cpp').write_text(code, encoding='ascii') + for platform in ('NRF52_PLATFORM', 'STM32_PLATFORM', + 'ESP32_PLATFORM', 'RP2040_PLATFORM'): + with self.subTest(platform=platform): + binary = work / ('test.exe' if os.name == 'nt' else 'test') + built = subprocess.run([ + os.environ.get('CXX', 'g++'), '-std=c++17', '-DENABLE_OTA=1', + '-D' + platform + '=1', *sanitizer_flags, + '-I' + str(work), '-I' + str(ROOT / 'src'), + '-I' + str(ROOT / 'src/helpers'), str(work / 'test.cpp'), + '-o', str(binary)], capture_output=True, text=True, timeout=60) + self.assertEqual(built.returncode, 0, built.stdout + built.stderr) + tested = subprocess.run([str(binary)], capture_output=True, + text=True, timeout=20) + self.assertEqual(tested.returncode, 0, tested.stdout + tested.stderr) + scenarios = 18 if platform in ('NRF52_PLATFORM', 'STM32_PLATFORM') else 15 + self.assertIn(f'{scenarios} actual CommonCLI save scenarios passed', tested.stdout) + + +if __name__ == '__main__': + unittest.main() diff --git a/test/test_common_usb_debug.py b/test/test_common_usb_debug.py index dd7677406..bfd5caa97 100644 --- a/test/test_common_usb_debug.py +++ b/test/test_common_usb_debug.py @@ -213,7 +213,7 @@ int main() { if (fault == 3) cli.fs.fail_rename = 1; const unsigned updates = mesh::debug_updates; cli.set("usb.debug on", reply); - assert(strcmp(reply, "Error: USB debug not saved; unchanged") == 0); + assert(strcmp(reply, "Error: save unconfirmed; reboot may differ") == 0); assert(cli.prefs.usb_debug_enabled == 0 && !mesh::isUsbDebugEnabled()); assert(mesh::debug_updates == updates && !mesh::isUsbLoggingEnabled()); assert(cli.fs.files["/com_prefs"] == debug_off); diff --git a/test/test_gps_sync_preferences.py b/test/test_gps_sync_preferences.py index 8952f8d5c..d1c6f81ec 100644 --- a/test/test_gps_sync_preferences.py +++ b/test/test_gps_sync_preferences.py @@ -86,7 +86,7 @@ int main() { } common.save_ok=companion.save_ok=false; const unsigned common_applied=common.sensors.applications, companion_applied=companion.sensors.applications; - common.set("gps.sync.interval 7",reply);assert(strstr(reply,"could not be saved")); + common.set("gps.sync.interval 7",reply);assert(strstr(reply,"unconfirmed")&&strstr(reply,"reboot")); assert(companion.command("set gps.sync.interval 7",reply,sizeof(reply)) && strstr(reply,"could not be saved")); assert(common.prefs.gps_sync_interval_hours==336 && common.persisted.gps_sync_interval_hours==336); assert(companion._prefs.gps_sync_interval_hours==336 && companion.persisted.gps_sync_interval_hours==336); diff --git a/test/test_observer_ota_channel.py b/test/test_observer_ota_channel.py index cfc4ed4a6..5349e8dd9 100644 --- a/test/test_observer_ota_channel.py +++ b/test/test_observer_ota_channel.py @@ -183,7 +183,7 @@ int main() { cli.save_ok=false; assert(cli.run("ota branch prod", reply)); assert(cli.prefs.ota_channel==OTA_CH_DEV && cli.persisted==OTA_CH_DEV); - assert(strstr(reply, "ERR: OTA channel save failed")); + assert(strstr(reply, "ERR: save unconfirmed") && strstr(reply, "reboot may differ")); const int saves=cli.saves; assert(cli.run("ota branch Beta", reply) && cli.saves==saves); assert(!cli.run("ota branches", reply)); diff --git a/test/test_peripheral_mode_prefs.py b/test/test_peripheral_mode_prefs.py index a290c359f..ffa339524 100644 --- a/test/test_peripheral_mode_prefs.py +++ b/test/test_peripheral_mode_prefs.py @@ -127,8 +127,8 @@ int main(){Common common;Companion companion;char reply[160]; assert(companion._prefs.peripherals_shared==1&&companion.durable.peripherals_shared==1&&strstr(reply,"reboot")); assert(common.sensor.applications==0&&companion.sensors.applications==0); common.ok=companion.ok=false; - common.set("set peripherals.mode gps-only",reply);assert(strstr(reply,"could not be saved")); - assert(companion.command("set peripherals.mode gps-only",reply,sizeof(reply))&&strstr(reply,"could not be saved")); + common.set("set peripherals.mode gps-only",reply);assert(strstr(reply,"unconfirmed")&&strstr(reply,"reboot")); + assert(companion.command("set peripherals.mode gps-only",reply,sizeof(reply))&&strstr(reply,"unconfirmed")&&strstr(reply,"reboot")); assert(common.prefs.peripherals_shared==1&&companion._prefs.peripherals_shared==1); common.get("get peripherals.mode",reply);assert(strstr(reply,"shared")); assert(companion.command("get peripherals.mode",reply,sizeof(reply))&&strstr(reply,"shared")); @@ -164,7 +164,7 @@ int main(){ assert(strstr(reply,"Error")&&saves==before&&mode==durable); } ok=false;assert(mesh::peripherals::handleCommand(mode,"set peripherals.mode shared",reply,sizeof(reply),true,save)); - assert(mode==0&&durable==0&&strstr(reply,"could not be saved")); + assert(mode==0&&durable==0&&strstr(reply,"unconfirmed")&&strstr(reply,"reboot")); unsigned before=saves; assert(mesh::peripherals::handleCommand(mode,"set peripherals.mode shared",reply,sizeof(reply),false,save)); assert(strstr(reply,"unsupported")&&saves==before&&mode==0); diff --git a/test/test_repeater_trace_repeat.py b/test/test_repeater_trace_repeat.py index a185dbfee..6a81dcba7 100644 --- a/test/test_repeater_trace_repeat.py +++ b/test/test_repeater_trace_repeat.py @@ -81,7 +81,7 @@ int main() { if (fault == 2) cli.fs.fail_read_open = true; if (fault == 3) cli.fs.fail_rename = 1; cli.set(desired ? "repeat.trace on" : "repeat.trace off", reply); - assert(!strcmp(reply, "Error: Repeat trace not saved; unchanged")); + assert(!strcmp(reply, "Error: save unconfirmed; reboot may differ")); assert(cli.prefs.trace_when_repeat_off == uint8_t(!desired)); assert(cli.fs.files["/com_prefs"] == committed); assert(cli.callbacks.retry_updates == callbacks_before);