From dad6d39c458323b9f60df60fdb566ebcdd449cb3 Mon Sep 17 00:00:00 2001 From: agessaman Date: Sat, 15 Aug 2026 15:22:22 -0700 Subject: [PATCH] fix(mqtt): keep an unclassifiable /mqtt.json candidate across boots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recovery preserved a FutureClaimed or Indeterminate temp, but published the backup into the primary name to run that boot. That spent the one piece of state saying the candidate had already passed the backup rename: the next boot saw an ordinary usable primary beside a stray temp, and deleted the temp precisely when more heap or newer firmware finally made it readable. The OOM path needed no future firmware to hit it — power cut after the backup rename, one boot short of classification scratch, and a verified new image was gone. Answer an uncertain temp with UseBackupHeld instead: rename nothing, read the last committed image straight out of /mqtt.json.bak, and hold writes. The filenames then still describe the interrupted transaction, so a later boot promotes the candidate through the ordinary temp rule, or falls back to the backup once the candidate proves definitively corrupt. Tests: two-boot sequences for Indeterminate and FutureClaimed candidates that later classify as Usable or FutureUsable, the invalid-candidate fallback, and the no-usable-backup case where the candidate still takes the authoritative name. --- MQTT_INTERNALS.md | 10 +- src/helpers/CommonCLI.cpp | 112 +++++++++++++----- src/helpers/MQTTPrefsRecovery.h | 21 +++- .../test_mqtt_prefs_atomic_store.cpp | 79 +++++++++++- 4 files changed, 184 insertions(+), 38 deletions(-) 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();