Never roll beacon IDs back to an unrelated stale backup

src/helpers/BeaconManager.cpp

@@ -254,14 +254,19 @@ BeaconManager::ReadResult BeaconManager::readIds(const char* path, uint32_t& las
 bool BeaconManager::commit(const char* path, const char* backup,
                            const char* temporary, bool& hold) {
   bool active_present = false, backup_present = false;
+  bool active_backed_up = false;
   if (!filePresence(fs_, path, active_present) || !filePresence(fs_, backup, backup_present)) return false;
   if (active_present) {
     if (backup_present && !fs_->remove(backup)) return false;
     if (!fs_->rename(path, backup)) return false;
+    active_backed_up = true;
     backup_present = true;
   }
   if (!fs_->rename(temporary, path)) {
-    if (backup_present && !fs_->rename(backup, path)) hold = true;
+    // A pre-existing backup can be older than an already transmitted ID.
+    // Roll back only the active image moved by this transaction; promoting
+    // an unrelated backup could make that ID reusable after the next boot.
+    if (active_backed_up && !fs_->rename(backup, path)) hold = true;
     return false;
   }
   // A leftover backup after a successful commit is harmless. Never choose it

test/fixtures/beacons/manager_test.cpp

@@ -350,6 +350,57 @@ void staleBackup() {
       && uninitialized.fs.files.at("/beacon_ids.bak") == stale);
 }
 
+void liveBackupRollback() {
+  {
+    Fixture f;
+    f.mt();
+    const auto stale = f.fs.files.at("/beacon_ids");
+    f.tick(2 * H);
+    assert(f.mesh.sends.size() == 1 && f.mesh.sends[0].id == 0x1234567a);
+    // A failed backup cleanup can leave an older image. If the active entry
+    // disappears during this boot, a later failed promotion must not turn
+    // that unrelated image into authoritative metadata.
+    f.fs.files["/beacon_ids.bak"] = stale;
+    f.fs.files.erase("/beacon_ids");
+    f.fs.fail_rename_from = {"/beacon_ids.tmp"};
+    f.tick(24 * H);
+    assert(f.mesh.sends.size() == 1);
+    assert(!f.fs.files.count("/beacon_ids"));
+    assert(f.fs.files.at("/beacon_ids.bak") == stale);
+    f.fs.fail_rename_from.clear();
+    f.tick(24 * H); // The failed counter write holds this boot too.
+    assert(f.mesh.sends.size() == 1);
+    f.manager.begin(&f.fs, &f.mesh, &f.clock, f.name);
+    f.tick(2 * H);
+    assert(f.mesh.sends.size() == 1); // Reboot must not reuse 0x1234567a.
+    f.cmd("set beacon 1 meshtext US LongFast 14 Still held", false);
+    f.mc(2);
+    f.tick(2 * H);
+    assert(f.mesh.sends.size() == 2
+        && f.mesh.sends.back().slot.radio.protocol == mesh::RadioProfileProtocol::MeshCore);
+  }
+  {
+    Fixture f;
+    f.mc(1, "Old");
+    const auto stale = f.fs.files.at("/beacons");
+    f.mc(1, "Current");
+    f.fs.files["/beacons.bak"] = stale;
+    f.fs.files.erase("/beacons");
+    f.fs.fail_rename_from = {"/beacons.tmp"};
+    f.cmd("set beacon 1 meshcore 907.5,125,8,6,10 New", false);
+    assert(!f.fs.files.count("/beacons"));
+    assert(f.fs.files.at("/beacons.bak") == stale);
+    assert(std::strstr(f.cmd("get beacon 1"), "Current"));
+    // Configuration can still commit a complete replacement on a later
+    // successful command, removing the stale backup after that commit.
+    f.fs.fail_rename_from.clear();
+    f.cmd("set beacon 1 meshcore 907.5,125,8,6,10 New");
+    assert(!f.fs.files.count("/beacons.bak"));
+    f.manager.begin(&f.fs, &f.mesh, &f.clock, f.name);
+    assert(std::strstr(f.cmd("get beacon 1"), "New"));
+  }
+}
+
 int main(int argc, char** argv) {
   assert(argc == 2);
   if (!std::strcmp(argv[1], "commands")) commands();
@@ -359,6 +410,7 @@ int main(int argc, char** argv) {
   else if (!std::strcmp(argv[1], "ids")) ids();
   else if (!std::strcmp(argv[1], "mutations")) mutations();
   else if (!std::strcmp(argv[1], "stale_backup")) staleBackup();
+  else if (!std::strcmp(argv[1], "live_backup")) liveBackupRollback();
   else assert(false);
   std::printf("beacon manager %s passed\n", argv[1]);
 }

test/test_beacon_manager.py

@@ -66,6 +66,9 @@ class BeaconManagerTest(unittest.TestCase):
     def test_missing_active_counter_never_rewinds_to_backup(self):
         self.run_scenario("stale_backup")
 
+    def test_failed_live_promotion_never_restores_an_unrelated_backup(self):
+        self.run_scenario("live_backup")
+
     def negative_control(self, name, source, scenario):
         generated = Path(self.work.name) / (name + ".cpp")
         generated.write_text(source, encoding="ascii")
@@ -97,6 +100,14 @@ class BeaconManagerTest(unittest.TestCase):
         self.assertEqual(source.count(held), 1)
         self.negative_control("rewound", source.replace(held, restored), "stale_backup")
 
+    def test_negative_control_live_counter_backup_rewind_is_detected(self):
+        source = (ROOT / "src/helpers/BeaconManager.cpp").read_text(encoding="ascii")
+        guard = "if (active_backed_up && !fs_->rename(backup, path))"
+        self.assertEqual(source.count(guard), 1)
+        self.negative_control("live_rewound", source.replace(
+            guard, "if ((active_backed_up || backup_present) && !fs_->rename(backup, path))"),
+            "live_backup")
+
 
 if __name__ == "__main__":
     unittest.main()