fix(ota): recover corrupt companion settings stores

src/helpers/ota/CompanionOtaConfig.cpp

@@ -11,6 +11,7 @@ namespace mesh { namespace ota {
 namespace {
 FILESYSTEM* settings_fs = nullptr;
 bool held = false;
+bool recoverable_corrupt_store = false;
 constexpr size_t ImageSize = 16 + MAX_OTA_SIGNERS * 32;
 constexpr size_t CrcOffset = ImageSize - 4;
 const char* const Path = "/ota_config";
@@ -69,45 +70,74 @@ bool decode(const uint8_t* image, OtaConfigState& state) {
   return true;
 }
 
-bool readImage(const char* path, uint8_t* image, OtaConfigState& state) {
+enum class ImageReadResult : uint8_t { Missing, Valid, Invalid, Unreadable };
+
+ImageReadResult readImage(const char* path, uint8_t* image, OtaConfigState& state) {
+  bool present = false;
+  if (!settings_fs || !pathPresence(path, present)) return ImageReadResult::Unreadable;
+  if (!present) return ImageReadResult::Missing;
 #if defined(NRF52_PLATFORM)
   File file(*settings_fs);
-  if (!file.open(path, FILE_O_READ)) return false;
+  if (!file.open(path, FILE_O_READ)) return ImageReadResult::Unreadable;
 #elif defined(STM32_PLATFORM)
   File file = settings_fs->open(path, FILE_O_READ);
 #else
   File file = settings_fs->open(path, "r");
 #endif
-  if (!file) return false;
-  const bool complete = file.size() == ImageSize
-      && file.read(image, ImageSize) == (int)ImageSize;
+  if (!file) return ImageReadResult::Unreadable;
+  const bool right_size = file.size() == ImageSize;
+  const bool complete = right_size && file.read(image, ImageSize) == (int)ImageSize;
   file.close();
-  return complete && decode(image, state);
+  if (!right_size) return ImageReadResult::Invalid;
+  if (!complete) return ImageReadResult::Unreadable;
+  return decode(image, state) ? ImageReadResult::Valid : ImageReadResult::Invalid;
+}
+
+bool discardCorruptSavedImagesForWrite() {
+  if (!held) return true;
+  if (!settings_fs || !recoverable_corrupt_store) return false;
+  // This is a user-triggered recovery for the separate OTA policy/allowlist;
+  // it never touches node identity, private keys, contacts, or common prefs.
+  if (!removeIfPresent(Path) || !removeIfPresent(Backup)) return false;
+  held = false;
+  recoverable_corrupt_store = false;
+  return true;
 }
 }
 
 void beginCompanionOtaConfig(FILESYSTEM* fs) {
   settings_fs = fs;
   held = false;
+  recoverable_corrupt_store = false;
 }
 
 bool loadCompanionOtaConfig(OtaConfigState& state) {
   if (!settings_fs) return false;
   uint8_t image[ImageSize];
-  bool primary = false, backup = false;
-  if (!pathPresence(Path, primary)) { held = true; return false; }
-  if (primary && readImage(Path, image, state)) return true;
-  if (primary) held = true;
-  if (!pathPresence(Backup, backup)) { held = true; return false; }
-  if (backup && readImage(Backup, image, state)) {
+  const ImageReadResult primary = readImage(Path, image, state);
+  bool loaded = primary == ImageReadResult::Valid;
+  ImageReadResult backup = ImageReadResult::Missing;
+  if (!loaded) backup = readImage(Backup, image, state);
+  if (!loaded && backup == ImageReadResult::Valid) {
     // Keep the verified previous settings even if recovery cannot rename them.
     // An unreadable primary may only have suffered a transient read failure;
     // do not remove it to promote the fallback.
-    if (primary || !settings_fs->rename(Backup, Path)) held = true;
+    if (primary == ImageReadResult::Unreadable
+        || (primary != ImageReadResult::Missing && !removeIfPresent(Path))
+        || !settings_fs->rename(Backup, Path)) held = true;
     return true;
   }
-  if (primary || backup) {
-    held = true;  // unreadable is not permission to overwrite with defaults
+  if (loaded) return true;
+  if (!loaded) {
+    held = primary != ImageReadResult::Missing || backup != ImageReadResult::Missing;
+    // A later explicit setting can recreate only files that were fully read
+    // and decoded as invalid. I/O failures and a valid backup stay protected.
+    recoverable_corrupt_store = held
+        && primary != ImageReadResult::Unreadable
+        && backup != ImageReadResult::Unreadable
+        && (primary == ImageReadResult::Invalid || backup == ImageReadResult::Invalid);
+  }
+  if (held) {
     return false;
   }
   state = OtaConfigState();
@@ -115,15 +145,21 @@ bool loadCompanionOtaConfig(OtaConfigState& state) {
 }
 
 bool saveCompanionOtaConfig(const OtaConfigState& state) {
-  if (!settings_fs || held || !validState(state)) return false;
+  if (!settings_fs || !validState(state) || !discardCorruptSavedImagesForWrite()) return false;
   uint8_t image[ImageSize] = {'O', 'C', 1, 0}, verify[ImageSize];
   OtaConfigState prior;
-  bool had_primary = false;
-  if (!pathPresence(Path, had_primary)) { held = true; return false; }
-  if (had_primary && !readImage(Path, verify, prior)) {
+  const ImageReadResult primary = readImage(Path, verify, prior);
+  if (primary == ImageReadResult::Unreadable) {
     held = true;
     return false;
   }
+  if (primary == ImageReadResult::Invalid) {
+    // The store changed after boot. Do not erase it on an implicit retry.
+    held = true;
+    recoverable_corrupt_store = true;
+    return false;
+  }
+  const bool had_primary = primary == ImageReadResult::Valid;
   image[4] = state.autofetch; image[5] = state.autoinstall; image[6] = state.hops;
   image[7] = state.allow.count();
   storage::writeLE16(image + 8, state.checkpoint);
@@ -144,7 +180,7 @@ bool saveCompanionOtaConfig(const OtaConfigState& state) {
   if (!file) return false;
   const bool wrote = file.write(image, sizeof(image)) == sizeof(image);
   file.flush(); file.close();
-  if (!wrote || !readImage(Temp, verify, prior)
+  if (!wrote || readImage(Temp, verify, prior) != ImageReadResult::Valid
       || memcmp(image, verify, sizeof(image))) return false;
   if (!removeIfPresent(Backup)) return false;
   if (had_primary && !settings_fs->rename(Path, Backup)) return false;

src/helpers/ota/OtaSpeedConfig.cpp

@@ -4,38 +4,91 @@
 #include <helpers/PersistentStoreFormat.h>
 #include <stdio.h>
 #include <string.h>
+#if defined(ESP32_PLATFORM)
+#include <errno.h>
+#include <sys/stat.h>
+#endif
 
 namespace mesh { namespace ota {
 namespace {
 FILESYSTEM* settings_fs = nullptr;
 float factor = OTA_SPEED_DEFAULT;
 bool held = false;
+bool recoverable_corrupt_store = false;
 constexpr size_t ImageSize = 12;
 const char* const Path = "/ota_speed";
 const char* const Temp = "/ota_speed.tmp";
 const char* const Backup = "/ota_speed.bak";
 
-bool readImage(const char* path, uint8_t* image) {
+enum class ImageReadResult : uint8_t { Missing, Valid, Invalid, Unreadable };
+
+// A failed open is not enough to prove a path is absent: some filesystems
+// report an I/O error as a failed exists() call.  Keep those images protected.
+bool pathPresence(const char* path, bool& present) {
+#if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM)
+  struct lfs_info info;
+  settings_fs->_lockFS();
+  const int result = lfs_stat(settings_fs->_getFS(), path, &info);
+  settings_fs->_unlockFS();
+  if (result != 0 && result != LFS_ERR_NOENT) return false;
+  present = result == 0;
+#elif defined(ESP32_PLATFORM)
+  char vfs_path[48];
+  const int length = snprintf(vfs_path, sizeof(vfs_path), "/spiffs%s", path);
+  if (length < 0 || static_cast<size_t>(length) >= sizeof(vfs_path)) return false;
+  struct stat info;
+  const int result = ::stat(vfs_path, &info);
+  if (result != 0 && errno != ENOENT) return false;
+  present = result == 0;
+#else
+  present = settings_fs->exists(path);
+#endif
+  return true;
+}
+
+bool removeIfPresent(const char* path) {
+  bool present = false;
+  return pathPresence(path, present) && (!present || settings_fs->remove(path));
+}
+
+ImageReadResult readImage(const char* path, uint8_t* image) {
+  bool present = false;
+  if (!settings_fs || !pathPresence(path, present)) return ImageReadResult::Unreadable;
+  if (!present) return ImageReadResult::Missing;
 #if defined(NRF52_PLATFORM)
   File file(*settings_fs);
-  if (!file.open(path, FILE_O_READ)) return false;
+  if (!file.open(path, FILE_O_READ)) return ImageReadResult::Unreadable;
 #elif defined(STM32_PLATFORM)
   File file = settings_fs->open(path, FILE_O_READ);
 #else
   File file = settings_fs->open(path, "r");
 #endif
-  if (!file) return false;
-  const bool complete = file.size() == ImageSize && file.read(image, ImageSize) == (int)ImageSize;
+  if (!file) return ImageReadResult::Unreadable;
+  const bool right_size = file.size() == ImageSize;
+  const bool complete = right_size && file.read(image, ImageSize) == (int)ImageSize;
   file.close();
-  if (!complete) return false;
+  if (!right_size) return ImageReadResult::Invalid;
+  if (!complete) return ImageReadResult::Unreadable;
   float value;
   memcpy(&value, image + 4, sizeof(value));
   return !memcmp(image, "OS\1\0", 4) && validSpeed(value)
-      && storage::readLE32(image + 8) == storage::updateCRC32(0xffffffffU, image, 8);
+      && storage::readLE32(image + 8) == storage::updateCRC32(0xffffffffU, image, 8)
+      ? ImageReadResult::Valid : ImageReadResult::Invalid;
+}
+
+bool discardCorruptSavedImagesForWrite() {
+  if (!held) return true;
+  if (!settings_fs || !recoverable_corrupt_store) return false;
+  // This is only reached from an explicit setter after both images were read
+  // completely and rejected. OTA policy is independent of identity/key data.
+  if (!removeIfPresent(Path) || !removeIfPresent(Backup)) return false;
+  held = false;
+  recoverable_corrupt_store = false;
+  return true;
 }
 
 bool saveSpeed(float value) {
-  if (!settings_fs || held) return false;
+  if (!settings_fs || !validSpeed(value) || !discardCorruptSavedImagesForWrite()) return false;
   uint8_t image[ImageSize] = {'O', 'S', 1, 0};
   memcpy(image + 4, &value, sizeof(value));
   storage::writeLE32(image + 8, storage::updateCRC32(0xffffffffU, image, 8));
@@ -52,9 +105,11 @@ bool saveSpeed(float value) {
   bool ok = file.write(image, sizeof(image)) == sizeof(image);
   file.flush(); file.close();
   uint8_t verify[ImageSize];
-  if (!ok || !readImage(Temp, verify) || memcmp(image, verify, sizeof(image))) return false;
-  if (settings_fs->exists(Backup) && !settings_fs->remove(Backup)) return false;
-  const bool had_primary = settings_fs->exists(Path);
+  if (!ok || readImage(Temp, verify) != ImageReadResult::Valid
+      || memcmp(image, verify, sizeof(image))) return false;
+  if (!removeIfPresent(Backup)) return false;
+  bool had_primary = false;
+  if (!pathPresence(Path, had_primary)) { held = true; return false; }
   if (had_primary && !settings_fs->rename(Path, Backup)) return false;
   if (!settings_fs->rename(Temp, Path)) {
     if (had_primary && !settings_fs->rename(Backup, Path)) held = true;
@@ -68,19 +123,35 @@ bool saveSpeed(float value) {
 
 void beginSpeedConfig(FILESYSTEM* fs) {
   settings_fs = fs; factor = OTA_SPEED_DEFAULT; held = false;
+  recoverable_corrupt_store = false;
   if (!fs) return;
   uint8_t image[ImageSize];
-  bool loaded = readImage(Path, image);
-  if (!loaded && readImage(Backup, image)) {
+  const ImageReadResult primary = readImage(Path, image);
+  bool loaded = primary == ImageReadResult::Valid;
+  ImageReadResult backup = ImageReadResult::Missing;
+  if (!loaded) backup = readImage(Backup, image);
+  if (!loaded && backup == ImageReadResult::Valid) {
     // A verified backup is still authoritative when storage cannot repair the
     // rename gap. Keep its pace instead of silently reverting to the faster 1x.
     memcpy(&factor, image + 4, sizeof(factor));
-    if (fs->exists(Path) && !fs->remove(Path)) { held = true; return; }
+    if (primary == ImageReadResult::Unreadable
+        || (primary != ImageReadResult::Missing && !removeIfPresent(Path))) {
+      held = true;
+      return;
+    }
     if (!fs->rename(Backup, Path)) { held = true; return; }
     loaded = true;
   }
   if (loaded) memcpy(&factor, image + 4, sizeof(factor));
-  else held = fs->exists(Path) || fs->exists(Backup);
+  else {
+    held = primary != ImageReadResult::Missing || backup != ImageReadResult::Missing;
+    // Only an explicit later setter may recreate conclusively corrupt OTA
+    // policy files. An unreadable file stays untouched for a clean reboot.
+    recoverable_corrupt_store = held
+        && primary != ImageReadResult::Unreadable
+        && backup != ImageReadResult::Unreadable
+        && (primary == ImageReadResult::Invalid || backup == ImageReadResult::Invalid);
+  }
 }
 
 float speedFactor() { return factor; }

test/test_companion_ota_config.py

@@ -204,8 +204,10 @@ int main() {
   fs.files["/ota_config"][4]^=1;
   const auto damaged=fs.files["/ota_config"];
   reboot(fs);
-  command("ota config hops 7", "ERR");
-  assert(fs.files["/ota_config"]==damaged);
+  command("ota config hops 7", "OK");
+  assert(fs.files["/ota_config"]!=damaged && context.manager.hops==7);
+  reboot(fs);
+  assert(context.manager.hops==7);
   fs.files.clear(); reboot(fs);
   acquire_ok=false;
   command("ota config hops 7", "ERR unavailable");

test/test_ota_speed.py

@@ -61,8 +61,10 @@ int main() {
   fs.files["/ota_speed"][4] ^= 1; // damaged image is not partly adopted
   beginSpeedConfig(&fs);
   assert(speedFactor() == 1.0f);
-  command("set ota.speed 2", "ERR");
-  assert(speedFactor() == 1.0f);
+  command("set ota.speed 2", "OK ota.speed=2x (saved)");
+  assert(speedFactor() == 2.0f);
+  beginSpeedConfig(&fs);
+  assert(speedFactor() == 2.0f);
   fs.files.clear();
   beginSpeedConfig(&fs);
   command("ota set speed 1.125", "OK ota.speed=1.125x");