diff --git a/examples/companion_radio/DataStore.cpp b/examples/companion_radio/DataStore.cpp index 803bd843c..42940eca7 100644 --- a/examples/companion_radio/DataStore.cpp +++ b/examples/companion_radio/DataStore.cpp @@ -2052,7 +2052,9 @@ bool DataStore::writeContactPage(DataStoreHost* host, uint8_t page, } if (!writer.commit(wrote)) { #if MESH_CONTACT_CACHE - paths.endCommit(false); + // Publication may have overwritten the old backing page. Keep preserved + // snapshots detached, but leave dirty handles and page generation pending. + paths.endCommit(writer.outcome() == mesh::AtomicFileWriter::Outcome::Indeterminate); #endif MESH_DEBUG_PRINTLN("DataStore: atomic contact page %u write failed", page); return false; @@ -3371,14 +3373,16 @@ bool DataStore::migrateToSecondaryFS() { return MigrationJournalState::Invalid; }; - auto writeJournal = [this](uint8_t state) -> bool { + using WriteOutcome = mesh::AtomicFileWriter::Outcome; + auto writeJournal = [this](uint8_t state) -> WriteOutcome { uint8_t payload[sizeof(journal_magic) + 1]; memcpy(payload, journal_magic, sizeof(journal_magic)); payload[sizeof(journal_magic)] = state; mesh::AtomicFileWriter journal(_fsExtra, SECONDARY_MIGRATION_JOURNAL); - return journal - && journal.write(payload, sizeof(payload)) == sizeof(payload) - && journal.commit(); + const bool wrote = journal + && journal.write(payload, sizeof(payload)) == sizeof(payload); + journal.commit(wrote); + return journal.outcome(); }; auto hasSecondarySources = [this](bool& present) -> bool { @@ -3532,20 +3536,27 @@ bool DataStore::migrateToSecondaryFS() { && (journal_state != MigrationJournalState::None || secondary_sources_present)) { if (journal_state == MigrationJournalState::None - && !writeJournal(SECONDARY_MIGRATION_PENDING)) { + && writeJournal(SECONDARY_MIGRATION_PENDING) != WriteOutcome::Committed) { MESH_DEBUG_PRINTLN("DataStore: could not start ExtraFS migration transaction"); _fsExtra = nullptr; return false; } - if (!copySecondarySources() - || !writeJournal(SECONDARY_MIGRATION_COMMITTED)) { - // Pending means no source has been retired, so primary remains a complete - // fallback even if the destination contains harmless partial copies. - MESH_DEBUG_PRINTLN("DataStore: ExtraFS migration copy incomplete; using primary storage"); - _fsExtra = nullptr; + const auto outcome = copySecondarySources() + ? writeJournal(SECONDARY_MIGRATION_COMMITTED) : WriteOutcome::NotCommitted; + // A final rename may have published COMMITTED despite a failed readback. + // Only a verified Pending journal or no publication attempt permits + // primary fallback; unknown authority must preserve both stores. + journal_state = outcome == WriteOutcome::Committed ? MigrationJournalState::Committed + : outcome == WriteOutcome::NotCommitted ? MigrationJournalState::Pending : readJournal(); + if (journal_state != MigrationJournalState::Committed) { + if (journal_state == MigrationJournalState::Pending) { + MESH_DEBUG_PRINTLN("DataStore: ExtraFS migration copy incomplete; using primary storage"); + _fsExtra = nullptr; + } else { + _contact_load_incomplete = true; + } return false; } - journal_state = MigrationJournalState::Committed; } if (journal_state == MigrationJournalState::Committed) { diff --git a/src/helpers/AtomicFileWriter.h b/src/helpers/AtomicFileWriter.h index 1d777350d..64fba52ad 100644 --- a/src/helpers/AtomicFileWriter.h +++ b/src/helpers/AtomicFileWriter.h @@ -12,9 +12,11 @@ namespace mesh { * Power-loss-safe file replacement for the Adafruit LittleFS backends. * * Data is written to a sibling .tmp file. commit() flushes and closes that - * file, reads it back while checking its size and CRC32, and only then uses - * LittleFS rename() to atomically replace the live file. A failed or abandoned - * write removes the temporary file and leaves the previous live file intact. + * file, reads it back while checking its size and CRC32C, and only then uses + * LittleFS rename() to atomically replace the live file. Failures before that + * publication leave the previous live file intact. Once rename is attempted, + * a failure may leave the old or new image live; inspect outcome() before + * assuming that the previous image survived. */ class AtomicFileWriter { static const size_t TEMP_PATH_CAPACITY = 96; @@ -28,9 +30,13 @@ class AtomicFileWriter { uint32_t _crc; bool _opened; bool _write_ok; - bool _finished; +public: + enum class Outcome : uint8_t { Pending, NotCommitted, Committed, Indeterminate }; - static uint32_t updateCRC32(uint32_t crc, const uint8_t* data, size_t len); +private: + Outcome _outcome; + + static uint32_t updateCRC32C(uint32_t crc, const uint8_t* data, size_t len); void removeTempFile(); bool validateFile(const char* path); @@ -47,9 +53,12 @@ public: /** * Commit the staged file. content_valid lets callers include format-specific - * checks in the same decision; false always preserves the previous file. + * checks in the same decision; content_valid=false prevents publication. + * Returns true only after verifying the public image. A false return can be + * Indeterminate, not proof that publication did not happen. */ bool commit(bool content_valid = true); + Outcome outcome() const { return _outcome; } size_t bytesWritten() const { return _bytes_written; } const char* tempPath() const { return _temp_path; } @@ -61,11 +70,16 @@ public: namespace mesh { -uint32_t AtomicFileWriter::updateCRC32(uint32_t crc, const uint8_t* data, size_t len) { +// Share this loop between write and readback instead of duplicating it under LTO. +__attribute__((noinline)) uint32_t AtomicFileWriter::updateCRC32C( + uint32_t crc, const uint8_t* data, size_t len) { + // Persistent images can end in their own IEEE CRC32. Reusing that polynomial + // here would give all valid same-sized images a fixed whole-image residue. + // This independent, nonpersistent fingerprint does not alter file formats. while (len-- > 0) { crc ^= *data++; - for (uint8_t bit = 0; bit < 8; bit++) { - crc = (crc >> 1) ^ ((crc & 1) ? 0xEDB88320UL : 0); + for (unsigned bit = 0; bit < 8; bit++) { + crc = (crc >> 1) ^ (0x82F63B78U & (0U - (crc & 1U))); } } return crc; @@ -82,7 +96,7 @@ bool AtomicFileWriter::validateFile(const char* path) { File verify(*_fs); if (!verify.open(path, FILE_O_READ)) continue; - bool valid = verify.size() == _bytes_written; + bool valid = !verify.isDirectory() && verify.size() == _bytes_written; uint32_t read_crc = 0xFFFFFFFFUL; size_t remaining = _bytes_written; uint8_t buf[64]; @@ -94,7 +108,7 @@ bool AtomicFileWriter::validateFile(const char* path) { valid = false; break; } - read_crc = updateCRC32(read_crc, buf, chunk); + read_crc = updateCRC32C(read_crc, buf, chunk); remaining -= chunk; } @@ -106,7 +120,7 @@ bool AtomicFileWriter::validateFile(const char* path) { AtomicFileWriter::AtomicFileWriter(FILESYSTEM* fs, const char* target_path) : _fs(fs), _target_path(target_path), _file(*fs), _bytes_written(0), - _crc(0xFFFFFFFFUL), _opened(false), _write_ok(true), _finished(false) { + _crc(0xFFFFFFFFUL), _opened(false), _write_ok(true), _outcome(Outcome::Pending) { _temp_path[0] = 0; if (target_path == NULL) { _write_ok = false; @@ -130,7 +144,7 @@ AtomicFileWriter::AtomicFileWriter(FILESYSTEM* fs, const char* target_path) } AtomicFileWriter::~AtomicFileWriter() { - if (_finished) return; + if (_outcome != Outcome::Pending) return; if (_opened) { _file.close(); _opened = false; @@ -155,12 +169,12 @@ size_t AtomicFileWriter::write(const uint8_t* data, size_t len) { } _bytes_written += written; - _crc = updateCRC32(_crc, data, written); + _crc = updateCRC32C(_crc, data, written); return written; } bool AtomicFileWriter::commit(bool content_valid) { - if (_finished) return false; + if (_outcome != Outcome::Pending) return false; bool success = _opened && _write_ok && content_valid; if (_opened) { @@ -171,14 +185,19 @@ bool AtomicFileWriter::commit(bool content_valid) { } if (success) success = validateFile(_temp_path); - if (success) success = _fs->rename(_temp_path, _target_path); - // A lower layer can acknowledge a metadata update before the physical page - // is durable. Validate the public filename too; callers must never clear a - // dirty flag or announce success while the old/corrupt file is still live. - if (success) success = validateFile(_target_path); - if (!success) removeTempFile(); - - _finished = true; + if (success) { + // Rename can report an error after publishing, or acknowledge a change + // without replacing the public image. Its return value cannot establish + // either outcome: always verify the public bytes against the staged CRC. + _fs->rename(_temp_path, _target_path); + success = validateFile(_target_path); + _outcome = success ? Outcome::Committed : Outcome::Indeterminate; + } else { + _outcome = Outcome::NotCommitted; + } + // A failed rename may leave scratch even when the public image already + // matches. It is never an authority or a rollback image. + removeTempFile(); return success; } diff --git a/src/helpers/PersistentStoreFormat.h b/src/helpers/PersistentStoreFormat.h index 9089f37db..cbc788a68 100644 --- a/src/helpers/PersistentStoreFormat.h +++ b/src/helpers/PersistentStoreFormat.h @@ -125,8 +125,8 @@ inline void writeLE32(uint8_t* dest, uint32_t value) { dest[3] = (uint8_t)(value >> 24); } -// Same CRC convention as AtomicFileWriter: reflected CRC-32, initial value -// 0xFFFFFFFF, with no final xor. Keeping this incremental makes it usable on +// Persistent images use reflected IEEE CRC-32, initial value 0xFFFFFFFF, +// with no final xor. Keeping this incremental makes it usable on // small targets without buffering an entire file. inline uint32_t updateCRC32(uint32_t crc, const uint8_t* data, size_t len) { while (len-- > 0) { diff --git a/test/fixtures/contact_cache/FakeFilesystem.h b/test/fixtures/contact_cache/FakeFilesystem.h index 3d6f71773..19d3349b7 100644 --- a/test/fixtures/contact_cache/FakeFilesystem.h +++ b/test/fixtures/contact_cache/FakeFilesystem.h @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -30,6 +31,7 @@ public: File(FakeFilesystem* fs, const char* path, bool write); bool open(const char* path, uint8_t mode); explicit operator bool() const { return _fs != nullptr; } + bool isDirectory() const { return false; } // This fixture models regular files only. size_t read(uint8_t* bytes, size_t length); size_t write(const uint8_t* bytes, size_t length); size_t size() const; @@ -61,6 +63,9 @@ public: unsigned metadata_probes = 0, fail_metadata_probe = 0; unsigned removes = 0, missing_remove_logs = 0; unsigned fail_rename = 0, renames = 0, writes = 0, reads = 0, opens = 0; + unsigned false_success_rename = 0, late_rename_error = 0; + std::map read_open_failures; + std::function after_rename; std::vector rename_snapshots; std::map generations; bool exists(const char* path) const { return files.count(path) != 0; } @@ -82,6 +87,11 @@ public: files[path].clear(); return File(this, path, true); } + auto failure = read_open_failures.find(path); + if (failure != read_open_failures.end() && failure->second != 0) { + --failure->second; + return File(); + } if (fail_read == path || !exists(path)) return File(); return File(this, path, false); } @@ -91,6 +101,7 @@ public: bool rename(const char* from, const char* to) { ++renames; if (renames == fail_rename || !exists(from)) return false; + if (renames == false_success_rename) return true; #if defined(ESP32_PLATFORM) if (exists(to)) return false; #endif @@ -98,7 +109,8 @@ public: files.erase(from); ++generations[from]; ++generations[to]; rename_snapshots.push_back(files); - return true; + if (after_rename) after_rename(*this, to); + return renames != late_rename_error; } size_t totalBytes() const { return capacity; } void setBlockCount(uint32_t count) { _config.block_count = count; } diff --git a/test/fixtures/contact_cache/test_contact_cache.cpp b/test/fixtures/contact_cache/test_contact_cache.cpp index 5d72f9461..be163fa26 100644 --- a/test/fixtures/contact_cache/test_contact_cache.cpp +++ b/test/fixtures/contact_cache/test_contact_cache.cpp @@ -116,6 +116,7 @@ public: } #endif #if defined(NRF52_PLATFORM) + uint32_t pageGeneration(uint8_t page) const { return _contact_page_generations[page]; } bool loadContactPages(DataStoreHost*, uint16_t, uint32_t); bool writeContactPage(DataStoreHost*, uint8_t, bool (*filter)(const ContactInfo&)); bool markContactDirty(const ContactInfo& c) { @@ -924,6 +925,43 @@ static void nrf52_migration_snapshots_and_failed_page_replacement() { assert(!f.host.contacts[3].getPath()); assert(!f.store.writeContactPage(&f.host, 0, cachedContactFilter)); } + +static void nrf52_uncertain_page_publication_preserves_snapshots() { + for (unsigned fault = 0; fault < 3; ++fault) { + Fixture f; + for (uint8_t page = 0; page < 14; ++page) + assert(f.store.writeContactPage(&f.host, page, cachedContactFilter)); + SPIFFS.remove("/contacts3"); + const auto original = SPIFFS.files.at("/contacts4_00"); + ContactInfo snapshot = f.host.contacts[0]; + assert(f.host.contacts[0].setRawPath(route(1100).data())); + assert(f.store.markContactDirty(f.host.contacts[0])); + const auto generation = f.store.pageGeneration(0); + if (fault == 2) SPIFFS.false_success_rename = SPIFFS.renames + 1; + SPIFFS.after_rename = [fault](FakeFilesystem& fs, const char* path) { + if (strcmp(path, "/contacts4_00") != 0) return; + if (fault == 0) fs.late_rename_error = fs.renames; + else if (fault == 1) fs.read_open_failures[path] = 3; + }; + assert(f.store.serviceContactWrites(&f.host, cachedContactFilter) == (fault == 0)); + assert(f.store.hasPendingContactWrites() == (fault != 0)); + assert(f.store.pageGeneration(0) == generation + (fault == 0)); + assert((SPIFFS.files.at("/contacts4_00") == original) == (fault == 2)); + // Make the retained old path cold if it still has an evictable disk binding. + for (unsigned i = 1; i < 350; ++i) f.check(i, route(i)); + Path path; + assert(snapshot.copyPathTo(path.data()) && path == route(0)); + f.check(0, route(1100)); + SPIFFS.after_rename = {}; + SPIFFS.false_success_rename = SPIFFS.late_rename_error = 0; + assert(f.store.serviceContactWrites(&f.host, cachedContactFilter)); + assert(!f.store.hasPendingContactWrites()); + assert(f.store.pageGeneration(0) == generation + 1); + for (unsigned i = 1; i < 350; ++i) f.check(i, route(i)); + assert(snapshot.copyPathTo(path.data()) && path == route(0)); + f.check(0, route(1100)); + } +} #endif static void contact_tx_policy_round_trips_without_growing_records() { @@ -991,6 +1029,7 @@ int main() { #else nrf52_secret_cache_leaves_contact_storage_reserve(); nrf52_migration_snapshots_and_failed_page_replacement(); + nrf52_uncertain_page_publication_preserves_snapshots(); encoded_paths_and_copy_assignment(); failed_path_reads_fail_sends_without_leaking_packets(); contact_reload_preserves_disk_on_cache_exhaustion(); diff --git a/test/fixtures/contact_cache/test_secondary_migration.cpp b/test/fixtures/contact_cache/test_secondary_migration.cpp new file mode 100644 index 000000000..a4ffb0af3 --- /dev/null +++ b/test/fixtures/contact_cache/test_secondary_migration.cpp @@ -0,0 +1,147 @@ +#include "FakeFilesystem.h" +#include +#include +#define FILESYSTEM FakeFilesystem +#define ATOMIC_FILE_WRITER_IMPLEMENTATION +#include +#define MESH_DEBUG_PRINTLN(...) ((void)0) + +static const char* SECONDARY_MIGRATION_JOURNAL = "/.extrafs.mig"; +static const uint8_t SECONDARY_MIGRATION_PENDING = 1, SECONDARY_MIGRATION_COMMITTED = 2; +bool contactPathPresence(FakeFilesystem* fs, const char* path, bool& present) { + if (fs->metadata_error) return false; + present = fs->exists(path); + return true; +} +struct ChannelDetails { + struct { uint8_t tx_radio = mesh::RADIO_TX_AUTO, secret[32] = {}; } channel; + char name[32] = {}; +}; +struct DataStoreHost { + bool getChannelForSave(uint8_t, ChannelDetails&) { return false; } +}; +class DataStore { + public: + FakeFilesystem* _fs; + FakeFilesystem* _fsExtra; + bool _contact_load_incomplete = false, _identity_creation_blocked = false; + bool _prefs_load_incomplete = false; + DataStore(FakeFilesystem& primary, FakeFilesystem& secondary) : _fs(&primary), _fsExtra(&secondary) {} + File openRead(FakeFilesystem* fs, const char* path) { return fs->open(path); } + FakeFilesystem* _getContactsChannelsFS() { return _fsExtra ? _fsExtra : _fs; } + bool migrateToSecondaryFS(); + bool saveChannels(DataStoreHost*); +}; +#include "migration_under_test.h" + +static std::vector journal(uint8_t state) { + return {'M', 'C', 'X', 'F', 1, state}; +} +static void seed(FakeFilesystem& primary, FakeFilesystem& secondary) { + primary.files["/contacts3"] = {1, 2, 3}; + primary.files["/channels2"] = {4, 5, 6}; + secondary.files[SECONDARY_MIGRATION_JOURNAL] = journal(SECONDARY_MIGRATION_PENDING); +} + +int main() { + unsigned scenarios = 0; + // Final journal faults: late error, exhausted verification, unreadable + // reconciliation, missing/invalid journal, metadata error, retained Pending, + // and proven pre-publication failure with an unrelated metadata fault. + for (unsigned fault = 0; fault < 8; ++fault) { + FakeFilesystem primary, secondary; + seed(primary, secondary); + const auto originals = primary.files; + DataStore store(primary, secondary); + if (fault == 6) secondary.false_success_rename = 3; + secondary.after_rename = [fault](FakeFilesystem& fs, const char* path) { + if (fault == 7 && strcmp(path, "/channels2") == 0) { + fs.max_write = 0; + fs.metadata_error = true; + } + if (strcmp(path, SECONDARY_MIGRATION_JOURNAL) != 0) return; + if (fault == 0) fs.late_rename_error = fs.renames; + if (fault == 1) fs.read_open_failures[path] = 3; + if (fault == 2) fs.read_open_failures[path] = 4; + if (fault == 3) fs.files.erase(path); + if (fault == 4) fs.files[path][0] ^= 1; + if (fault == 5) { + fs.read_open_failures[path] = 3; + fs.metadata_error = true; + } + }; + const bool migrated = store.migrateToSecondaryFS(); + const bool committed = fault < 2; + const bool primary_retained = fault >= 6; + assert(migrated == committed); + assert(store._contact_load_incomplete == (!committed && !primary_retained)); + assert(store._fs == &primary); + assert(store._fsExtra == (primary_retained ? nullptr : &secondary)); + if (committed) { + assert(primary.files.empty()); + assert(!secondary.exists(SECONDARY_MIGRATION_JOURNAL)); + } else { + assert(primary.files == originals); + // Reproduce the authority leak: a quarantined store must refuse writes. + if (!primary_retained) { + DataStoreHost host; + const auto primary_before = primary.files, secondary_before = secondary.files; + assert(!store.saveChannels(&host)); + assert(primary.files == primary_before && secondary.files == secondary_before); + } + } + for (const auto& source : originals) assert(secondary.files.at(source.first) == source.second); + + // A subsequent boot honors the journal that actually reached public storage. + secondary.after_rename = {}; + secondary.metadata_error = false; + secondary.max_write = std::numeric_limits::max(); + secondary.false_success_rename = secondary.late_rename_error = 0; + secondary.read_open_failures.clear(); + DataStore reboot(primary, secondary); + if (fault == 4) { + assert(!reboot.migrateToSecondaryFS() && reboot._contact_load_incomplete); + assert(primary.files == originals); + } else { + if (secondary.exists(SECONDARY_MIGRATION_JOURNAL) + && secondary.files.at(SECONDARY_MIGRATION_JOURNAL) == journal(SECONDARY_MIGRATION_COMMITTED)) { + secondary.files["/contacts3"] = {9, 8, 7}; + } + assert(reboot.migrateToSecondaryFS()); + assert(primary.files.empty() && reboot._fsExtra == &secondary); + assert(!secondary.exists(SECONDARY_MIGRATION_JOURNAL)); + assert(secondary.files.at("/contacts3") == (fault == 2 || fault == 5 + ? std::vector{9, 8, 7} : originals.at("/contacts3"))); + } + ++scenarios; + } + + // A copy failure cannot publish the final authority switch or retire sources. + { + FakeFilesystem primary, secondary; + seed(primary, secondary); + const auto originals = primary.files; + secondary.max_write = 0; + DataStore store(primary, secondary); + assert(!store.migrateToSecondaryFS()); + assert(store._fsExtra == nullptr && !store._contact_load_incomplete); + assert(primary.files == originals); + assert(secondary.files.at(SECONDARY_MIGRATION_JOURNAL) == journal(SECONDARY_MIGRATION_PENDING)); + ++scenarios; + } + // An uncertain initial Pending publication still leaves complete primary authority. + { + FakeFilesystem primary, secondary; + primary.files["/contacts3"] = {1, 2, 3}; + const auto originals = primary.files; + secondary.after_rename = [](FakeFilesystem& fs, const char* path) { + if (strcmp(path, SECONDARY_MIGRATION_JOURNAL) == 0) fs.read_open_failures[path] = 3; + }; + DataStore store(primary, secondary); + assert(!store.migrateToSecondaryFS()); + assert(store._fsExtra == nullptr && !store._contact_load_incomplete); + assert(primary.files == originals && !secondary.exists("/contacts3")); + ++scenarios; + } + std::printf("PASS: %u secondary migration publication scenarios\n", scenarios); +} diff --git a/test/test_atomic_file_writer/test_atomic_file_writer.cpp b/test/test_atomic_file_writer/test_atomic_file_writer.cpp index 25c87e2de..6f865a5c9 100644 --- a/test/test_atomic_file_writer/test_atomic_file_writer.cpp +++ b/test/test_atomic_file_writer/test_atomic_file_writer.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include class FakeFilesystem; @@ -25,6 +26,7 @@ public: size_t write(const uint8_t* data, size_t len); int read(void* data, uint16_t len); uint32_t size() const; + bool isDirectory() const; void flush() {} void close() { _open = false; } explicit operator bool() const { return _open; } @@ -33,15 +35,19 @@ public: class FakeFilesystem { public: std::unordered_map> files; + std::unordered_set directories; size_t max_write = std::numeric_limits::max(); bool fail_read_open = false; bool fail_rename = false; bool false_success_rename = false; + bool error_after_publish = false; + bool fail_public_reads_after_publish = false; + bool corrupt_after_publish = false; unsigned read_open_failures_remaining = 0; unsigned rename_calls = 0; bool exists(const char* path) const { - return files.find(path) != files.end(); + return files.find(path) != files.end() || directories.count(path); } bool remove(const char* path) { @@ -52,11 +58,14 @@ public: rename_calls++; if (fail_rename) return false; if (false_success_rename) return true; + if (directories.count(to)) return false; auto source = files.find(from); if (source == files.end()) return false; files[to] = source->second; files.erase(source); - return true; + if (fail_public_reads_after_publish) fail_read_open = true; + if (corrupt_after_publish && !files[to].empty()) files[to][0] ^= 1; + return !error_after_publish; } }; @@ -110,6 +119,10 @@ uint32_t FakeFile::size() const { return found == _fs->files.end() ? 0 : (uint32_t)found->second.size(); } +bool FakeFile::isDirectory() const { + return _fs->directories.count(_path) != 0; +} + #define NRF52_PLATFORM 1 #define FILESYSTEM FakeFilesystem using File = FakeFile; @@ -120,17 +133,31 @@ static std::vector bytes(const char* text) { return std::vector(text, text + strlen(text)); } +static std::vector ieeeCrcImage(const char* text) { + auto result = bytes(text); + uint32_t crc = 0xffffffffU; + for (uint8_t value : result) { + crc ^= value; + for (unsigned bit = 0; bit < 8; ++bit) + crc = (crc >> 1) ^ ((crc & 1) ? 0xedb88320U : 0); + } + for (unsigned byte = 0; byte < 4; ++byte) result.push_back(uint8_t(crc >> (8 * byte))); + return result; +} + TEST(AtomicFileWriter, ReplacesLiveFileOnlyAfterVerifiedCommit) { FakeFilesystem fs; fs.files["/prefs"] = bytes("old"); mesh::AtomicFileWriter writer(&fs, "/prefs"); ASSERT_TRUE(writer); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Pending); const uint8_t replacement[] = {'n', 'e', 'w'}; EXPECT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); EXPECT_EQ(fs.files["/prefs"], bytes("old")); EXPECT_TRUE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Committed); EXPECT_EQ(fs.files["/prefs"], bytes("new")); EXPECT_FALSE(fs.exists("/prefs.tmp")); EXPECT_EQ(fs.rename_calls, 1u); @@ -145,6 +172,7 @@ TEST(AtomicFileWriter, PartialWritePreservesLiveFile) { const uint8_t replacement[] = {'n', 'e', 'w'}; EXPECT_EQ(writer.write(replacement, sizeof(replacement)), 2u); EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::NotCommitted); EXPECT_EQ(fs.files["/prefs"], bytes("old")); EXPECT_FALSE(fs.exists("/prefs.tmp")); @@ -161,6 +189,7 @@ TEST(AtomicFileWriter, ReadbackMismatchPreservesLiveFile) { fs.files["/prefs.tmp"][1] ^= 0x01; EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::NotCommitted); EXPECT_EQ(fs.files["/prefs"], bytes("old")); EXPECT_FALSE(fs.exists("/prefs.tmp")); EXPECT_EQ(fs.rename_calls, 0u); @@ -175,6 +204,7 @@ TEST(AtomicFileWriter, CallerValidationFailurePreservesLiveFile) { ASSERT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); EXPECT_FALSE(writer.commit(false)); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::NotCommitted); EXPECT_EQ(fs.files["/prefs"], bytes("old")); EXPECT_FALSE(fs.exists("/prefs.tmp")); EXPECT_EQ(fs.rename_calls, 0u); @@ -190,6 +220,7 @@ TEST(AtomicFileWriter, RenameFailurePreservesLiveFileAndCleansTemp) { ASSERT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Indeterminate); EXPECT_EQ(fs.files["/prefs"], bytes("old")); EXPECT_FALSE(fs.exists("/prefs.tmp")); EXPECT_EQ(fs.rename_calls, 1u); @@ -205,6 +236,7 @@ TEST(AtomicFileWriter, FalseSuccessRenameIsDetectedByLiveReadback) { ASSERT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Indeterminate); EXPECT_EQ(fs.files["/prefs"], bytes("old")); EXPECT_FALSE(fs.exists("/prefs.tmp")); EXPECT_EQ(fs.rename_calls, 1u); @@ -223,6 +255,112 @@ TEST(AtomicFileWriter, ValidationRetriesTransientReadOpenFailures) { EXPECT_EQ(fs.files["/prefs"], bytes("new")); } +TEST(AtomicFileWriter, PublicImageConfirmsRenameDespiteLateError) { + FakeFilesystem fs; + fs.files["/prefs"] = bytes("old"); + fs.error_after_publish = true; + + mesh::AtomicFileWriter writer(&fs, "/prefs"); + const uint8_t replacement[] = {'n', 'e', 'w'}; + ASSERT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); + EXPECT_TRUE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Committed); + EXPECT_EQ(fs.files["/prefs"], bytes("new")); + EXPECT_FALSE(fs.exists("/prefs.tmp")); +} + +TEST(AtomicFileWriter, ApplicationCrcTailCannotMaskRejectedPublication) { + // Equal-sized valid IEEE-CRC images have the same whole-image IEEE residue. + // The writer must use an independent check to distinguish old from new. + for (bool false_success : {false, true}) { + FakeFilesystem fs; + const auto previous = ieeeCrcImage("old"); + const auto replacement = ieeeCrcImage("new"); + fs.files["/prefs"] = previous; + fs.fail_rename = !false_success; + fs.false_success_rename = false_success; + + mesh::AtomicFileWriter writer(&fs, "/prefs"); + ASSERT_EQ(writer.write(replacement.data(), replacement.size()), replacement.size()); + EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Indeterminate); + EXPECT_EQ(fs.files["/prefs"], previous); + EXPECT_FALSE(fs.exists("/prefs.tmp")); + } +} + +TEST(AtomicFileWriter, UnreadablePublishedImageIsNotAConfirmedRollback) { + for (bool late_error : {false, true}) { + FakeFilesystem fs; + fs.files["/prefs"] = bytes("old"); + fs.error_after_publish = late_error; + fs.fail_public_reads_after_publish = true; + + mesh::AtomicFileWriter writer(&fs, "/prefs"); + const uint8_t replacement[] = {'n', 'e', 'w'}; + ASSERT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); + EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Indeterminate); + EXPECT_EQ(fs.files["/prefs"], bytes("new")); + EXPECT_FALSE(fs.exists("/prefs.tmp")); + EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Indeterminate); + EXPECT_EQ(fs.rename_calls, 1u); + } +} + +TEST(AtomicFileWriter, CorruptPublicImageIsIndeterminate) { + FakeFilesystem fs; + fs.files["/prefs"] = bytes("old"); + fs.corrupt_after_publish = true; + + mesh::AtomicFileWriter writer(&fs, "/prefs"); + const uint8_t replacement[] = {'n', 'e', 'w'}; + ASSERT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); + EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Indeterminate); + EXPECT_NE(fs.files["/prefs"], bytes("old")); + EXPECT_NE(fs.files["/prefs"], bytes("new")); +} + +TEST(AtomicFileWriter, AlreadyMatchingPublicImageCanConfirmFailedRename) { + FakeFilesystem fs; + fs.files["/prefs"] = bytes("new"); + fs.fail_rename = true; + + mesh::AtomicFileWriter writer(&fs, "/prefs"); + const uint8_t replacement[] = {'n', 'e', 'w'}; + ASSERT_EQ(writer.write(replacement, sizeof(replacement)), sizeof(replacement)); + EXPECT_TRUE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Committed); + EXPECT_EQ(fs.files["/prefs"], bytes("new")); + EXPECT_FALSE(fs.exists("/prefs.tmp")); +} + +TEST(AtomicFileWriter, EmptyImageMustNotAcceptDirectoryAsCommittedFile) { + FakeFilesystem fs; + fs.directories.insert("/prefs"); + + mesh::AtomicFileWriter writer(&fs, "/prefs"); + ASSERT_TRUE(writer); + EXPECT_FALSE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Indeterminate); + EXPECT_TRUE(fs.directories.count("/prefs")); + EXPECT_FALSE(fs.files.count("/prefs")); + EXPECT_FALSE(fs.exists("/prefs.tmp")); +} + +TEST(AtomicFileWriter, EmptyRegularFileCanBeCommitted) { + FakeFilesystem fs; + mesh::AtomicFileWriter writer(&fs, "/marker"); + ASSERT_TRUE(writer); + EXPECT_TRUE(writer.commit()); + EXPECT_EQ(writer.outcome(), mesh::AtomicFileWriter::Outcome::Committed); + EXPECT_TRUE(fs.files.count("/marker")); + EXPECT_TRUE(fs.files["/marker"].empty()); + EXPECT_FALSE(fs.exists("/marker.tmp")); +} + TEST(AtomicFileWriter, AbandonedWriteAndStaleTempAreCleaned) { FakeFilesystem fs; fs.files["/prefs"] = bytes("old"); diff --git a/test/test_nrf52_extrafs_contract.py b/test/test_nrf52_extrafs_contract.py index cfd2ab6a4..6e2d4f868 100644 --- a/test/test_nrf52_extrafs_contract.py +++ b/test/test_nrf52_extrafs_contract.py @@ -4,6 +4,7 @@ from pathlib import Path import re import subprocess +import tempfile import unittest @@ -37,6 +38,34 @@ def function_body(source: str, signature: str) -> str: class Nrf52ExtraFsContractTest(unittest.TestCase): + def test_uncertain_journal_publication_preserves_storage_authority(self): + store = STORE.read_text(encoding="utf-8") + # Compile the complete production methods, including their platform branches. + def method(signature): + start = store.index(signature) + body = function_body(store, signature) + return store[start:store.index("{", start) + 1] + body + "}" + + implementation = "\n".join(method(signature) for signature in ( + "static void makeContactPagePath(", "bool DataStore::migrateToSecondaryFS()", + "bool DataStore::saveChannels(")) + with tempfile.TemporaryDirectory(prefix="mesh-secondary-publication-") as directory: + directory = Path(directory) + (directory / "migration_under_test.h").write_text(implementation, encoding="ascii") + binary = directory / "migration" + compiled = subprocess.run([ + "c++", "-std=c++17", "-O1", "-g", "-Wall", "-Wextra", + "-fsanitize=address,undefined", "-fno-omit-frame-pointer", "-fno-pie", "-no-pie", + "-DNRF52_PLATFORM=1", "-I", str(ROOT / "test/fixtures/contact_cache/mocks"), + "-I", str(ROOT / "test/fixtures/contact_cache"), "-I", str(ROOT / "src"), + "-I", str(directory), str(ROOT / "test/fixtures/contact_cache/test_secondary_migration.cpp"), + "-o", str(binary), + ], capture_output=True, text=True) + self.assertEqual(compiled.returncode, 0, compiled.stderr) + result = subprocess.run([str(binary)], capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn("PASS: 10 secondary migration publication scenarios", result.stdout) + def test_hil_stat_failure_is_consumed_before_source_discovery(self): store = STORE.read_text(encoding="utf-8") load_contacts = function_body(store, "void DataStore::loadContacts(") @@ -552,7 +581,7 @@ class Nrf52ExtraFsContractTest(unittest.TestCase): self.assertLess(committed_at, retire_at) self.assertLess(retire_at, journal_remove_at) copy_failure_end = migration.index( - "journal_state = MigrationJournalState::Committed", committed_at + "if (journal_state == MigrationJournalState::Committed)", committed_at ) self.assertIn( "_fsExtra = nullptr;", migration[copy_at:copy_failure_end]