diff --git a/MQTT_INTERNALS.md b/MQTT_INTERNALS.md index eb5c7944..bc1e13b2 100644 --- a/MQTT_INTERNALS.md +++ b/MQTT_INTERNALS.md @@ -228,8 +228,14 @@ exists. Boot recovery selects the usable primary/temp/backup without overwriting an opaque future or corrupt primary. A valid future-version temp that reached the rename phase wins over the stale backup and is held for newer firmware. If a temp claims a future version but uses grammar this firmware -cannot parse, or cannot be classified because scratch allocation fails, recovery may run -the last usable backup but retains the uncertain temp and holds all observer writes. A +cannot parse, or cannot be classified because scratch allocation fails, recovery renames +nothing at all: it reads the last usable backup straight out of `/mqtt.json.bak`, leaves +the primary name empty, and holds all observer writes. The empty primary name is the only +record that the candidate had already passed the backup rename, so publishing the backup +would make the candidate indistinguishable from a stale artifact — and the next boot, the +one with enough heap or new enough firmware to finally read it, would delete it. Leaving +the names alone means that boot promotes the candidate through the ordinary temp rule, or +falls back to the backup if it turns out to be definitively corrupt. A definitively corrupt/incomplete current-version temp may be discarded for that backup. If power fails during the very first migration, there is no JSON primary or backup yet; recovery discards a definitively invalid temp so the intact `/mqtt_prefs` source can be diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 930676a5..c901fb45 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -409,8 +409,14 @@ enum class JsonPrefsLoadResult : uint8_t { UnsupportedVersion, FutureClaimed, Invalid, + NoMemory, }; +static bool jsonPrefsLoaded(JsonPrefsLoadResult result) { + return result == JsonPrefsLoadResult::Loaded || + result == JsonPrefsLoadResult::LoadedWithRepairs; +} + static JsonPrefsLoadResult loadMqttJsonFile(FILESYSTEM* fs, const char* path, MQTTPrefs* output) { if (output == nullptr) return JsonPrefsLoadResult::Invalid; @@ -441,6 +447,19 @@ static JsonPrefsLoadResult loadMqttJsonFile(FILESYSTEM* fs, const char* path, return repaired ? JsonPrefsLoadResult::LoadedWithRepairs : JsonPrefsLoadResult::Loaded; } +// Parse `path` through a heap scratch object and publish it over `dest` only +// once the whole file is known good, so a late failure cannot leave the live +// preferences half-loaded. +static JsonPrefsLoadResult adoptMqttJsonFile(FILESYSTEM* fs, const char* path, + MQTTPrefs* dest) { + MQTTPrefs* scratch = new (std::nothrow) MQTTPrefs; + if (scratch == nullptr) return JsonPrefsLoadResult::NoMemory; + const JsonPrefsLoadResult result = loadMqttJsonFile(fs, path, scratch); + if (jsonPrefsLoaded(result)) memcpy(dest, scratch, sizeof(*dest)); + delete scratch; + return result; +} + static MQTTPrefsRecovery::FileState mqttJsonFileState(FILESYSTEM* fs, const char* path) { if (!fs->exists(path)) return MQTTPrefsRecovery::FileState::Missing; MQTTPrefs* scratch = new (std::nothrow) MQTTPrefs; @@ -462,7 +481,15 @@ static MQTTPrefsRecovery::FileState mqttJsonFileState(FILESYSTEM* fs, const char : MQTTPrefsRecovery::FileState::Preserve; } -static bool recoverMqttJsonFiles(FILESYSTEM* fs) { +// hold: observer writes must not replace whatever was left on disk. +// run_from_backup: nothing was renamed and /mqtt.json does not exist; this boot +// reads the last committed image straight out of /mqtt.json.bak. +struct MqttJsonRecovery { + bool hold; + bool run_from_backup; +}; + +static MqttJsonRecovery recoverMqttJsonFiles(FILESYSTEM* fs) { const MQTTPrefsRecovery::FileState primary = mqttJsonFileState(fs, "/mqtt.json"); const MQTTPrefsRecovery::FileState temp = mqttJsonFileState(fs, "/mqtt.json.tmp"); const MQTTPrefsRecovery::FileState backup = mqttJsonFileState(fs, "/mqtt.json.bak"); @@ -479,17 +506,18 @@ static bool recoverMqttJsonFiles(FILESYSTEM* fs) { fs->remove("/mqtt.json.bak"); } } - return MQTTPrefsRecovery::uncertain(primary) || - MQTTPrefsRecovery::uncertain(temp) || - MQTTPrefsRecovery::uncertain(backup); + return {MQTTPrefsRecovery::uncertain(primary) || + MQTTPrefsRecovery::uncertain(temp) || + MQTTPrefsRecovery::uncertain(backup), + false}; } if (action == MQTTPrefsRecovery::Action::DiscardTemp) { if (fs->remove("/mqtt.json.tmp")) { MESH_DEBUG_PRINTLN("MQTT: discarded incomplete first-migration JSON temp"); - return false; + return {false, false}; } MESH_DEBUG_PRINTLN("MQTT: could not discard incomplete /mqtt.json temp; source held"); - return true; + return {true, false}; } if (action == MQTTPrefsRecovery::Action::PromoteTemp) { if (fs->rename("/mqtt.json.tmp", "/mqtt.json")) { @@ -498,11 +526,21 @@ static bool recoverMqttJsonFiles(FILESYSTEM* fs) { fs->remove("/mqtt.json.bak"); } MESH_DEBUG_PRINTLN("MQTT: recovered /mqtt.json from transaction temp"); - return MQTTPrefsRecovery::uncertain(temp) || - MQTTPrefsRecovery::uncertain(backup); + return {MQTTPrefsRecovery::uncertain(temp) || + MQTTPrefsRecovery::uncertain(backup), + false}; } MESH_DEBUG_PRINTLN("MQTT: could not recover /mqtt.json temp; files preserved"); - return true; + return {true, false}; + } + if (action == MQTTPrefsRecovery::Action::UseBackupHeld) { + // Deliberately rename nothing. The empty primary name is what records that + // the interrupted commit had already moved the old image aside, and a boot + // that cannot classify the candidate must not spend that record: a later + // boot with more heap, or firmware that understands the candidate, promotes + // it through the ordinary rule instead of deleting it as a stale artifact. + MESH_DEBUG_PRINTLN("MQTT: unresolved /mqtt.json candidate; running the backup in place and holding writes"); + return {true, true}; } if (action == MQTTPrefsRecovery::Action::PromoteBackup) { if (fs->rename("/mqtt.json.bak", "/mqtt.json")) { @@ -512,13 +550,14 @@ static bool recoverMqttJsonFiles(FILESYSTEM* fs) { fs->remove("/mqtt.json.tmp"); } MESH_DEBUG_PRINTLN("MQTT: recovered /mqtt.json from transaction backup"); - return MQTTPrefsRecovery::uncertain(temp) || - MQTTPrefsRecovery::uncertain(backup); + return {MQTTPrefsRecovery::uncertain(temp) || + MQTTPrefsRecovery::uncertain(backup), + false}; } MESH_DEBUG_PRINTLN("MQTT: could not recover /mqtt.json backup; files preserved"); - return true; + return {true, false}; } - return false; + return {false, false}; } static MQTTPrefsRecovery::FileState mqttPrefsFileState(FILESYSTEM* fs, const char* path) { @@ -579,6 +618,12 @@ static bool recoverMqttPrefsFiles(FILESYSTEM* fs) { MESH_DEBUG_PRINTLN("MQTT: could not recover /mqtt_prefs temp; files preserved"); return true; } + if (action == MQTTPrefsRecovery::Action::UseBackupHeld) { + // Unreachable for the binary format, whose classifier never reports an + // uncertain state. Hold rather than fall through to "nothing to do" if a + // later classifier change makes it reachable. + return true; + } if (action == MQTTPrefsRecovery::Action::PromoteBackup) { if (fs->rename("/mqtt_prefs.bak", "/mqtt_prefs")) { // Symmetric case: a usable backup is now primary, so any interrupted @@ -784,7 +829,26 @@ private: void CommonCLI::loadMQTTPrefs( FILESYSTEM* fs, MQTTPrefsAtomicStore::LegacyUpgradeGate* legacy_upgrade) { setMQTTPrefsDefaults(&_mqtt_prefs); - _mqtt_prefs_hold = recoverMqttJsonFiles(fs); + const MqttJsonRecovery recovery = recoverMqttJsonFiles(fs); + _mqtt_prefs_hold = recovery.hold; + + // An interrupted commit left a candidate this boot cannot classify, so + // recovery renamed nothing. Read the last committed image out of the backup + // rather than publishing it: the transaction filenames must survive this boot + // intact for a later one to promote the candidate. + if (recovery.run_from_backup) { + _legacy_tail.valid = false; + const JsonPrefsLoadResult backup_result = + adoptMqttJsonFile(fs, "/mqtt.json.bak", &_mqtt_prefs); + if (jsonPrefsLoaded(backup_result)) { + MESH_DEBUG_PRINTLN("MQTT: running the /mqtt.json backup; candidate preserved and writes held"); + } else { + setMQTTPrefsDefaults(&_mqtt_prefs); + MESH_DEBUG_PRINTLN("MQTT: /mqtt.json backup became unreadable; using defaults (files preserved)"); + } + return; + } + if (_mqtt_prefs_hold && !fs->exists("/mqtt.json") && (fs->exists("/mqtt.json.tmp") || fs->exists("/mqtt.json.bak"))) { _legacy_tail.valid = false; @@ -796,18 +860,9 @@ void CommonCLI::loadMQTTPrefs( // stale binary snapshot when JSON is corrupt, unreadable, or from a future // schema: preserve it and run defaults until an operator resolves it. if (fs->exists("/mqtt.json")) { - MQTTPrefs* scratch = new (std::nothrow) MQTTPrefs; - if (scratch == nullptr) { - _mqtt_prefs_hold = true; - _legacy_tail.valid = false; - MESH_DEBUG_PRINTLN("MQTT: no memory to validate /mqtt.json; source preserved"); - return; - } - const JsonPrefsLoadResult json_result = loadMqttJsonFile(fs, "/mqtt.json", scratch); - if (json_result == JsonPrefsLoadResult::Loaded || - json_result == JsonPrefsLoadResult::LoadedWithRepairs) { - memcpy(&_mqtt_prefs, scratch, sizeof(_mqtt_prefs)); - delete scratch; + const JsonPrefsLoadResult json_result = + adoptMqttJsonFile(fs, "/mqtt.json", &_mqtt_prefs); + if (jsonPrefsLoaded(json_result)) { _legacy_tail.valid = false; if (json_result == JsonPrefsLoadResult::LoadedWithRepairs) { MESH_DEBUG_PRINTLN("MQTT: repaired out-of-range values in /mqtt.json"); @@ -818,10 +873,11 @@ void CommonCLI::loadMQTTPrefs( } return; } - delete scratch; _mqtt_prefs_hold = true; _legacy_tail.valid = false; - if (json_result == JsonPrefsLoadResult::UnsupportedVersion) { + if (json_result == JsonPrefsLoadResult::NoMemory) { + MESH_DEBUG_PRINTLN("MQTT: no memory to validate /mqtt.json; source preserved"); + } else if (json_result == JsonPrefsLoadResult::UnsupportedVersion) { MESH_DEBUG_PRINTLN("MQTT: /mqtt.json uses a future version; using defaults (file preserved)"); } else if (json_result == JsonPrefsLoadResult::FutureClaimed) { MESH_DEBUG_PRINTLN( diff --git a/src/helpers/MQTTPrefsRecovery.h b/src/helpers/MQTTPrefsRecovery.h index 37073209..4f6b15ac 100644 --- a/src/helpers/MQTTPrefsRecovery.h +++ b/src/helpers/MQTTPrefsRecovery.h @@ -7,9 +7,16 @@ // the primary name. On a reset, the loader uses this policy before decoding // the primary. FutureUsable is syntactically valid but belongs to newer // firmware. FutureClaimed and Indeterminate cannot be safely classified by -// this firmware, so recovery may use a known-good backup but must retain the +// this firmware, so recovery may run a known-good backup but must retain the // uncertain image and hold further writes. Preserve is definitively invalid or // unsupported and may be discarded only where the transaction policy permits. +// +// The filenames are the transaction state. A temp that exists while the primary +// name is empty says the commit had already passed the backup rename, and that +// is the only record of it — so an uncertain temp is answered with +// UseBackupHeld, which renames nothing. Publishing the backup instead would +// make the candidate look like an ordinary stale artifact to the next boot, +// which would delete it exactly when it finally became readable. namespace MQTTPrefsRecovery { enum class FileState : uint8_t { @@ -27,6 +34,10 @@ enum class Action : uint8_t { DiscardTemp, PromoteTemp, PromoteBackup, + // Run the backup where it lies, changing nothing on disk, and hold writes. + // Every later boot re-runs this policy against the same three names until one + // of them can classify the candidate. + UseBackupHeld, }; inline Action select(FileState primary, FileState temp, FileState backup) { @@ -50,10 +61,12 @@ inline Action select(FileState primary, FileState temp, FileState backup) { } // FutureClaimed and Indeterminate may be a completed image this firmware - // cannot classify. A known-good backup can run this boot, but without one - // preserve the only candidate under the authoritative name and hold writes. + // cannot classify. A known-good backup can run this boot, but it must run + // from its own name: promoting it would spend the empty primary name that + // marks the candidate as mid-commit. Without such a backup, preserve the only + // candidate under the authoritative name and hold writes. if (temp == FileState::FutureClaimed || temp == FileState::Indeterminate) { - return backup == FileState::Usable ? Action::PromoteBackup : Action::PromoteTemp; + return backup == FileState::Usable ? Action::UseBackupHeld : Action::PromoteTemp; } // No temp survived. The backup is the only recoverable image, even when it diff --git a/test/test_mqtt_prefs_atomic_store/test_mqtt_prefs_atomic_store.cpp b/test/test_mqtt_prefs_atomic_store/test_mqtt_prefs_atomic_store.cpp index ddd6e184..a5b28be1 100644 --- a/test/test_mqtt_prefs_atomic_store/test_mqtt_prefs_atomic_store.cpp +++ b/test/test_mqtt_prefs_atomic_store/test_mqtt_prefs_atomic_store.cpp @@ -467,6 +467,9 @@ public: _files.erase("/mqtt_prefs.tmp"); return; } + if (action == Recovery::Action::UseBackupHeld) { + return; // production renames nothing and runs the backup where it lies + } if (action == Recovery::Action::PromoteBackup) { rename("/mqtt_prefs.bak", "/mqtt_prefs"); if (backup == Recovery::FileState::Usable && !Recovery::uncertain(temp) && @@ -930,15 +933,83 @@ TEST(MQTTPrefsAtomicStore, AmbiguousFutureOrOomTempIsNeverDeleted) { store.recover(Recovery::FileState::Missing, uncertain, Recovery::FileState::Usable); - // The supported backup can run this boot, but the candidate that this - // firmware could not classify remains available to newer firmware or a - // later boot with more heap. Its presence also blocks a new transaction. - EXPECT_EQ(SpiffsMqttTransaction::oldImage(), store.primary()); + // The supported backup runs this boot from its own name. Nothing is + // renamed, so the empty primary name still records that the candidate had + // reached the publish phase. Its presence also blocks a new transaction. + EXPECT_FALSE(store.has("/mqtt_prefs")); EXPECT_TRUE(store.has("/mqtt_prefs.tmp")); + EXPECT_TRUE(store.has("/mqtt_prefs.bak")); EXPECT_FALSE(store.canStartSave()); } } +TEST(MQTTPrefsAtomicStore, PreservedCandidateIsPromotedByTheBootThatCanReadIt) { + // The whole point of keeping an uncertain temp is that a later boot can act + // on it. Publishing the backup on the first boot would defeat that: the + // candidate would then look like a stale artifact next to a usable primary, + // and get deleted exactly when it finally became readable. + const struct { + Recovery::FileState first_boot; + Recovery::FileState second_boot; + // A promoted current-format image ends the transaction, so its backup goes + // too. A future-format one keeps the last readable image and stays held. + bool clears_backup; + } cases[] = { + // Classification scratch could not be allocated, then heap recovered. + {Recovery::FileState::Indeterminate, Recovery::FileState::Usable, true}, + // Downgraded firmware could not parse it, then the node rolled forward. + {Recovery::FileState::FutureClaimed, Recovery::FileState::Usable, true}, + // Still a newer schema on the second boot, but now classifiable. + {Recovery::FileState::FutureClaimed, Recovery::FileState::FutureUsable, false}, + }; + + for (const auto& test_case : cases) { + SpiffsMqttTransaction store; + store.cutAt(SpiffsMqttTransaction::Boundary::AfterBackupRename); + + store.recover(Recovery::FileState::Missing, test_case.first_boot, + Recovery::FileState::Usable); + ASSERT_TRUE(store.has("/mqtt_prefs.tmp")); + ASSERT_FALSE(store.has("/mqtt_prefs")); + + store.recover(Recovery::FileState::Missing, test_case.second_boot, + Recovery::FileState::Usable); + EXPECT_EQ(SpiffsMqttTransaction::newImage(), store.primary()); + EXPECT_FALSE(store.has("/mqtt_prefs.tmp")); + EXPECT_EQ(test_case.clears_backup, !store.has("/mqtt_prefs.bak")); + EXPECT_EQ(test_case.clears_backup, store.canStartSave()); + } +} + +TEST(MQTTPrefsAtomicStore, CandidateThatProvesInvalidYieldsToTheHeldBackup) { + SpiffsMqttTransaction store; + store.cutAt(SpiffsMqttTransaction::Boundary::AfterBackupRename); + store.recover(Recovery::FileState::Missing, Recovery::FileState::Indeterminate, + Recovery::FileState::Usable); + + // The second boot can classify it and finds it definitively corrupt, so the + // last committed image is published and the candidate is dropped. The node + // leaves the held state without ever having discarded an unread candidate. + store.recover(Recovery::FileState::Missing, Recovery::FileState::Preserve, + Recovery::FileState::Usable); + EXPECT_EQ(SpiffsMqttTransaction::oldImage(), store.primary()); + EXPECT_FALSE(store.has("/mqtt_prefs.tmp")); + EXPECT_TRUE(store.canStartSave()); +} + +TEST(MQTTPrefsAtomicStore, UncertainTempWithNoUsableBackupStillOwnsThePrimaryName) { + // With no image that can run this boot, the candidate is the only thing left + // to protect, so it takes the authoritative name and CommonCLI holds it. + EXPECT_EQ(Recovery::Action::PromoteTemp, + Recovery::select(Recovery::FileState::Missing, + Recovery::FileState::Indeterminate, + Recovery::FileState::Missing)); + EXPECT_EQ(Recovery::Action::PromoteTemp, + Recovery::select(Recovery::FileState::Missing, + Recovery::FileState::FutureClaimed, + Recovery::FileState::Preserve)); +} + TEST(MQTTPrefsAtomicStore, UsablePrimaryDoesNotCleanIndeterminateArtifact) { SpiffsMqttTransaction store; store.cutDuringTempWrite();