From 4846191fff10f12f79676c4217618220254dc8f0 Mon Sep 17 00:00:00 2001 From: mikecarper Date: Fri, 16 Oct 2026 00:10:57 -0700 Subject: [PATCH] Never roll beacon IDs back to an unrelated stale backup --- src/helpers/BeaconManager.cpp | 7 +++- test/fixtures/beacons/manager_test.cpp | 52 ++++++++++++++++++++++++++ test/test_beacon_manager.py | 11 ++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/helpers/BeaconManager.cpp b/src/helpers/BeaconManager.cpp index 484d4d327..3a58e76b0 100644 --- a/src/helpers/BeaconManager.cpp +++ b/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 diff --git a/test/fixtures/beacons/manager_test.cpp b/test/fixtures/beacons/manager_test.cpp index 3a264d872..ab8472504 100644 --- a/test/fixtures/beacons/manager_test.cpp +++ b/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]); } diff --git a/test/test_beacon_manager.py b/test/test_beacon_manager.py index b7b4ca4c5..f06b8d7b9 100644 --- a/test/test_beacon_manager.py +++ b/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()