From 618c18d385a3f7e7e9aa9a5ad79bde2e26dd0459 Mon Sep 17 00:00:00 2001 From: nullrouten0 <201262769+nullrouten0@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:37:28 +0000 Subject: [PATCH] Keep whole contact records ahead of a partial trailing record Upstream MeshCore, and this fork up to v1.17.1.6 except ESP32 Full builds, rewrote /contacts3 in place, one record after another. A reset or a full volume during that save leaves whole records followed by part of the next one. Upstream reads such a file up to the short read. Here readableContactFile() treats any size that is not a multiple of 152 bytes as unreadable, and since 2311160a9 the bounded recovery then deletes /contacts3. On the first boot after an upgrade, three whole records plus one stray byte became zero contacts. Invariant: on ESP32, RP2040 and STM32, when /contacts3 holds at least one whole record followed by part of one, and /contacts3.bak does not exist, the whole records load and nothing is written or deleted at boot. The next ordinary save publishes only whole records through the existing transaction. Unchanged: whole-record images; a backup still wins over an incomplete primary, including a backup that is unreadable or of unknown presence at boot, which keeps its retry at the recovery boundary; read errors keep the bounded recovery; a file with no whole record is still discarded; nRF52 is not touched. selectContactFile() makes this check only after the strict checks of both names fail, through a new partial_tail mode of readableContactFile(). loadContacts() drops its own size check; its whole-record loop bound already stops before the tail. Add test_companion_contact_partial_tail.py, which runs the real load, recovery and save paths on the uncached ESP32, RP2040 and STM32 harness and on the ESP32 Full cache harness under ASan/UBSan. On the old code all four fail with zero contacts and /contacts3 deleted. It catches 14 of 15 mutations of this change, including a full revert; the other drops a guard whose value is read only when the file handle is valid. Two cases in the uncached storage fixture used a one-byte truncation as their unreadable image; they now use a file shorter than one record, so they still exercise the discard path. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/run-unit-tests.yml | 1 + examples/companion_radio/DataStore.cpp | 34 +-- .../companion_uncached_storage/test.cpp | 5 +- test/test_companion_contact_partial_tail.py | 235 ++++++++++++++++++ 4 files changed, 258 insertions(+), 17 deletions(-) create mode 100644 test/test_companion_contact_partial_tail.py diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index 78ca72b2b..24584ff16 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -411,6 +411,7 @@ jobs: python3 -B test/test_companion_identity_cli.py -v python3 -B test/test_companion_packet_stream.py -v python3 -B test/test_companion_uncached_storage.py -v + python3 -B test/test_companion_contact_partial_tail.py -v python3 -B test/test_companion_channel_storage.py -v python3 -B test/test_nrf52_extrafs_contract.py -v python3 -B test/test_companion_ota_config.py -v diff --git a/examples/companion_radio/DataStore.cpp b/examples/companion_radio/DataStore.cpp index c8ce18f29..4fce620d7 100644 --- a/examples/companion_radio/DataStore.cpp +++ b/examples/companion_radio/DataStore.cpp @@ -2111,12 +2111,15 @@ void DataStore::clearContactLoadFailure() { #if !defined(NRF52_PLATFORM) // Validate a complete image before retiring its alternative. Legacy contacts // have no file CRC; retain their existing record/route repair semantics. -static bool readableContactFile(FILESYSTEM* fs, const char* path) { +// partial_tail instead requires whole records followed by part of one. +static bool readableContactFile(FILESYSTEM* fs, const char* path, + bool partial_tail = false) { File file = mesh::openFileRead(fs, path); - bool valid = file && !file.isDirectory() - && file.size() % mesh::storage::CONTACT_RECORD_SIZE == 0; + bool valid = file && !file.isDirectory(); + const size_t tail = valid ? file.size() % mesh::storage::CONTACT_RECORD_SIZE : 0; + valid = valid && (partial_tail ? tail != 0 && file.size() > tail : tail == 0); uint8_t record[mesh::storage::CONTACT_RECORD_SIZE]; - size_t remaining = valid ? file.size() : 0; + size_t remaining = valid ? file.size() - tail : 0; while (valid && remaining != 0) { valid = file.read(record, sizeof(record)) == sizeof(record); remaining -= sizeof(record); @@ -2154,6 +2157,17 @@ const char* DataStore::selectContactFile(bool& present) { } } if (failed) { + // Older firmware rewrote this file in place; a reset or full volume + // during that save leaves part of a record after whole ones. Keep those + // records while no backup exists for this incomplete image to retire. + bool backup = false; +#if defined(ESP32_PLATFORM) || defined(RP2040_PLATFORM) + if (!companionPathPresence(fs, "/contacts3.bak", backup)) backup = true; +#endif + if (!backup && readableContactFile(fs, "/contacts3", true)) { + present = true; + return "/contacts3"; + } queueContactRecovery(true); return nullptr; } @@ -2500,17 +2514,6 @@ void DataStore::loadContacts(DataStoreHost* host) { if (file) { bool full = false; uint16_t record_index = 0; -#if !defined(NRF52_PLATFORM) - if (file.size() % mesh::storage::CONTACT_RECORD_SIZE != 0) { -#if MESH_CONTACT_CACHE - _cache_load_incomplete = true; -#else - _uncached_contact_load_incomplete = true; -#endif - file.close(); - return; - } -#endif #if defined(NRF52_PLATFORM) _legacy_contact_count = mesh::storage::legacyContactCountForSize(legacy_size); @@ -2519,6 +2522,7 @@ void DataStore::loadContacts(DataStoreHost* host) { #endif while (!full #if !defined(NRF52_PLATFORM) + // A partial trailing record admitted by selectContactFile() stays unread. && record_index < file.size() / mesh::storage::CONTACT_RECORD_SIZE #endif #if defined(NRF52_PLATFORM) diff --git a/test/fixtures/companion_uncached_storage/test.cpp b/test/fixtures/companion_uncached_storage/test.cpp index 9d525e4fd..f8186cac7 100644 --- a/test/fixtures/companion_uncached_storage/test.cpp +++ b/test/fixtures/companion_uncached_storage/test.cpp @@ -321,7 +321,8 @@ static void contact_checks() { filesystem.files["/contacts3"] = original; if (fault == 0) filesystem.fail_open = "/contacts3"; if (fault == 1) filesystem.max_read = 16; - if (fault == 2) filesystem.files["/contacts3"].pop_back(); + // No whole record; whole records before a partial one load instead. + if (fault == 2) filesystem.files["/contacts3"].resize(mesh::storage::CONTACT_RECORD_SIZE - 1); DataStore rebooted; Host loaded; if (fault == 3) loaded.capacity = 1; @@ -355,7 +356,7 @@ static void recovery_checks() { for (unsigned fault = 0; fault < 3; ++fault) { filesystem = FakeFilesystem(); filesystem.files["/contacts3"] = original; - filesystem.files["/contacts3"].pop_back(); + filesystem.files["/contacts3"].resize(mesh::storage::CONTACT_RECORD_SIZE - 1); filesystem.files[identity_path] = std::vector(96, 7); filesystem.files["/channels2"] = {3, 4, 5}; filesystem.files["/prefs"] = {6, 7}; diff --git a/test/test_companion_contact_partial_tail.py b/test/test_companion_contact_partial_tail.py new file mode 100644 index 000000000..6a9385d80 --- /dev/null +++ b/test/test_companion_contact_partial_tail.py @@ -0,0 +1,235 @@ +#!/usr/bin/env python3 +"""Keep whole legacy contact records ahead of an interrupted in-place save.""" +from pathlib import Path +import unittest + +import test_contact_cache as cached +import test_companion_uncached_storage as uncached +from test_t096_full_memory import method + +ROOT = Path(__file__).resolve().parents[1] +MARKER = "PASS: partial contact tail" + +# Older firmware rewrote /contacts3 in place. A reset or full volume during +# that save leaves whole records followed by part of the next one. +CHECKS = r''' +static constexpr size_t RS = mesh::storage::CONTACT_RECORD_SIZE; +#if MESH_CONTACT_CACHE +static FakeFilesystem& tailFs() { return SPIFFS; } +struct TailBoot { Host host; DataStore store{host}; }; +static bool tailSave(TailBoot& b) { return b.store.saveContacts(&b.host, cachedContactFilter); } +static void failOpens(const char* path, unsigned count) { + SPIFFS.read_open_failures.clear(); + SPIFFS.read_open_failures[path] = count; +} +static void failMetadata(bool fail) { SPIFFS.metadata_error = fail; } +static std::vector tailImage() { + Fixture f(4); + return SPIFFS.files.at("/contacts3"); +} +#else +static FakeFilesystem& tailFs() { return filesystem; } +struct TailBoot { Host host; DataStore store; }; +static bool tailSave(TailBoot& b) { return b.store.saveContacts(&b.host); } +static void failOpens(const char* path, unsigned count) { + filesystem.fail_open = path; + filesystem.fail_open_remaining = count; +} +[[maybe_unused]] static void failMetadata(bool fail) { filesystem.stat_error = fail ? EIO : 0; } +static std::vector tailImage() { + filesystem = FakeFilesystem(); + DataStore store; + Host host; + host.contacts = {contact(1), contact(2), contact(3), contact(4)}; + assert(store.saveContacts(&host)); + return filesystem.files.at("/contacts3"); +} +#endif + +static std::vector records(const std::vector& image, size_t count) { + return std::vector(image.begin(), image.begin() + count * RS); +} + +static std::vector partial(const std::vector& image, size_t tail) { + auto file = records(image, 3); + file.insert(file.end(), image.begin() + 3 * RS, image.begin() + 3 * RS + tail); + return file; +} + +static void boot(TailBoot& b) { // MyMesh::begin() order. + b.store.loadContacts(&b.host); + b.store.recoverContacts(&b.host); +} + +static void checkLoaded(Host& host, const std::vector& image, size_t count) { + assert(host.contacts.size() == count); + for (size_t i = 0; i < count; ++i) { + const uint8_t* record = image.data() + i * RS; + uint8_t path[64]; + assert(!memcmp(host.contacts[i].id.pub_key, record, 32)); + assert(!memcmp(host.contacts[i].name, record + 32, 32)); + assert(host.contacts[i].copyPathTo(path) && !memcmp(path, record + 76, 64)); + } +} + +static void partial_tail_keeps_whole_records() { + const auto image = tailImage(); + assert(image.size() == 4 * RS); + for (size_t tail : {size_t(1), RS / 2, RS - 1}) { + tailFs() = FakeFilesystem(); + tailFs().files["/contacts3"] = partial(image, tail); + const auto durable = tailFs().files; + { + TailBoot b; + boot(b); + assert(!b.store.hasIncompleteContactLoad()); + assert(!b.store.contactRecoveryPending() && !b.store.contactsAreVolatile()); + checkLoaded(b.host, image, 3); + assert(tailFs().files == durable); // Loading neither rewrites nor discards. + assert(tailSave(b)); + assert(tailFs().files.at("/contacts3") == records(image, 3)); + } + TailBoot rebooted; + boot(rebooted); + assert(!rebooted.store.hasIncompleteContactLoad()); + checkLoaded(rebooted.host, image, 3); + } +} + +static void without_a_whole_record_recovery_is_unchanged() { + const auto image = tailImage(); + tailFs() = FakeFilesystem(); + tailFs().files["/contacts3"] = records(image, 1); + tailFs().files["/contacts3"].pop_back(); + TailBoot b; + boot(b); + assert(!b.store.hasIncompleteContactLoad() && !b.store.contactRecoveryPending()); + assert(b.host.contacts.empty() && !tailFs().files.count("/contacts3")); +} + +static void whole_record_read_errors_keep_bounded_recovery() { + const auto image = tailImage(); + tailFs() = FakeFilesystem(); + tailFs().files["/contacts3"] = records(image, 3); + failOpens("/contacts3", 2); // Both boot-time reads. + TailBoot b; + b.store.loadContacts(&b.host); + assert(b.store.contactRecoveryPending() && b.host.contacts.empty()); + failOpens("", 0); + b.store.recoverContacts(&b.host); + assert(!b.store.hasIncompleteContactLoad() && !b.store.contactRecoveryPending()); + checkLoaded(b.host, image, 3); +} + +#if !defined(STM32_PLATFORM) +static void a_complete_backup_still_wins() { + const auto image = tailImage(); + for (unsigned fault = 0; fault < 3; ++fault) { + tailFs() = FakeFilesystem(); + tailFs().files["/contacts3"] = partial(image, 1); + tailFs().files["/contacts3.bak"] = records(image, 2); + // A backup unreadable or of unknown presence at boot keeps its retry at + // the recovery boundary; the incomplete primary must not retire it first. + if (fault == 1) failOpens("/contacts3.bak", 2); + if (fault == 2) failMetadata(true); + TailBoot b; + b.store.loadContacts(&b.host); + failOpens("", 0); + failMetadata(false); + b.store.recoverContacts(&b.host); + assert(!b.store.hasIncompleteContactLoad() && !b.store.contactRecoveryPending()); + checkLoaded(b.host, image, 2); + assert(tailFs().files.at("/contacts3") == records(image, 2)); + assert(!tailFs().files.count("/contacts3.bak")); + } +} +#endif + +#if !MESH_CONTACT_CACHE +static void salvaged_primary_open_failure_is_not_absence() { + const auto image = tailImage(); + filesystem = FakeFilesystem(); + filesystem.files["/contacts3"] = partial(image, 1); + { + TailBoot dry; // The last read open of a healthy load opens the image to load. + dry.store.loadContacts(&dry.host); + assert(dry.host.contacts.size() == 3); + } + const unsigned load_open = filesystem.read_opens; + filesystem = FakeFilesystem(); + filesystem.files["/contacts3"] = partial(image, 1); + filesystem.fail_read_open = load_open; + TailBoot b; + b.store.loadContacts(&b.host); + assert(b.store.hasIncompleteContactLoad() && b.store.contactRecoveryPending()); + const auto unchanged = filesystem.files; + assert(!tailSave(b) && filesystem.files == unchanged); + b.store.recoverContacts(&b.host); + assert(!b.store.hasIncompleteContactLoad() && !b.store.contactRecoveryPending()); + checkLoaded(b.host, image, 3); +} +#endif + +int main() { + partial_tail_keeps_whole_records(); + without_a_whole_record_recovery_is_unchanged(); + whole_record_read_errors_keep_bounded_recovery(); +#if !defined(STM32_PLATFORM) + a_complete_backup_still_wins(); +#endif +#if !MESH_CONTACT_CACHE + salvaged_primary_open_failure_is_not_absence(); +#endif + printf("''' + MARKER + r'''\n"); + return 0; +} +''' + + +def uncached_fixture(work): + source = (ROOT / "test/fixtures/companion_uncached_storage/test.cpp").read_text() + for signature in ("static std::vector make_original()", + "static void identity_checks()", "static void contact_checks()", + "static void recovery_checks()"): + source = source.replace(method(source, signature), "") + # Fail one chosen read open, so a later open can fail after earlier ones succeed. + for old, new in ((" unsigned read_opens = 0;\n", + " unsigned read_opens = 0, fail_read_open = 0;\n"), + (" if (*mode == 'r') ++read_opens;\n", + " if (*mode == 'r' && ++read_opens == fail_read_open) return File();\n")): + assert source.count(old) == 1, old + source = source.replace(old, new) + generated = work / "partial_tail.cpp" + generated.write_text("#include \n" + source[:source.index("int main() {")] + CHECKS) + return generated + + +def cached_fixture(work): + fixtures = ROOT / "test/fixtures/contact_cache" + (work / "FakeFilesystem.h").write_text((fixtures / "FakeFilesystem.h").read_text()) + source = (fixtures / "test_contact_cache.cpp").read_text() + generated = work / "partial_tail.cpp" + generated.write_text("#include \n" + source[:source.index("int main() {")] + CHECKS) + return generated + + +class CompanionContactPartialTailTest(unittest.TestCase): + def test_esp32_uncached_partial_tail(self): + uncached.CompanionUncachedStorageTest().run_platform( + "ESP32_PLATFORM", fixture_transform=uncached_fixture) + + def test_rp2040_partial_tail(self): + uncached.CompanionUncachedStorageTest().run_platform( + "RP2040_PLATFORM", fixture_transform=uncached_fixture) + + def test_stm32_partial_tail(self): + uncached.CompanionUncachedStorageTest().run_platform( + "STM32_PLATFORM", fixture_transform=uncached_fixture) + + def test_esp32_cached_partial_tail(self): + cached.ContactCacheTest().run_cache_test( + "ESP32_PLATFORM", fixture_transform=cached_fixture, expected_marker=MARKER) + + +if __name__ == "__main__": + unittest.main()