diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index c901fb453..bf0155277 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -542,6 +542,13 @@ static MqttJsonRecovery recoverMqttJsonFiles(FILESYSTEM* fs) { MESH_DEBUG_PRINTLN("MQTT: unresolved /mqtt.json candidate; running the backup in place and holding writes"); return {true, true}; } + if (action == MQTTPrefsRecovery::Action::RunDefaultsHeld) { + // Same reasoning, with no image this firmware can run: renaming either file + // would spend transaction state that a later boot still needs. + MESH_DEBUG_PRINTLN("MQTT: unresolved /mqtt.json candidate and no runnable backup; " + "using defaults (files preserved)"); + return {true, false}; + } if (action == MQTTPrefsRecovery::Action::PromoteBackup) { if (fs->rename("/mqtt.json.bak", "/mqtt.json")) { if (backup == MQTTPrefsRecovery::FileState::Usable && @@ -618,7 +625,8 @@ static bool recoverMqttPrefsFiles(FILESYSTEM* fs) { MESH_DEBUG_PRINTLN("MQTT: could not recover /mqtt_prefs temp; files preserved"); return true; } - if (action == MQTTPrefsRecovery::Action::UseBackupHeld) { + if (action == MQTTPrefsRecovery::Action::UseBackupHeld || + action == MQTTPrefsRecovery::Action::RunDefaultsHeld) { // 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. @@ -766,26 +774,36 @@ public: return true; } - void discardFinishedTemp() { + // Drop a temp that was written and byte-verified but then rejected. Returns + // false when it is still on flash and no primary outranks it: recovery + // promotes a lone temp, so the caller cannot claim the change was undone. + bool discardFinishedTemp() { if (_open) _file.close(); _open = false; - if (_owns_temp && _fs->exists("/mqtt.json.tmp")) _fs->remove("/mqtt.json.tmp"); + bool removed = true; + if (_owns_temp && _fs->exists("/mqtt.json.tmp")) { + removed = _fs->remove("/mqtt.json.tmp"); + } _finished = false; _owns_temp = false; + return removed || _fs->exists("/mqtt.json"); } - void abort() { + // Same disposition contract as discardFinishedTemp(). Only unfinished staging + // is disposable here: once finish() has verified the temp, + // rollbackFailedCommit() has already decided its fate, and a temp that + // survived that is one recovery must resolve after reset. + bool abort() { if (_open) _file.close(); _open = false; - // Only unfinished staging is disposable here. Once finish() has verified - // the temp, rollbackFailedCommit() has already decided its fate, and a temp - // that survived that is one recovery must resolve after reset. + bool removed = true; if (_owns_temp && !_finished && _fs->exists("/mqtt.json.tmp")) { - _fs->remove("/mqtt.json.tmp"); + removed = _fs->remove("/mqtt.json.tmp"); } _finished = false; _owns_temp = false; _owns_backup = false; + return removed || _fs->exists("/mqtt.json"); } private: @@ -1130,6 +1148,12 @@ bool CommonCLI::saveMQTTPrefs(FILESYSTEM* fs) { return verify_result == JsonPrefsLoadResult::Loaded; }); + // A recovery file this firmware may still promote is holding the new image, + // so the change cannot be reported as rolled back. No further save can start + // until it is resolved: begin() refuses while either the temp or the backup + // exists, which is exactly the state that gets us here. + if (MQTTPrefsAtomicStore::saveOutcomeUnresolved(result)) _observer_save_indeterminate = true; + switch (result) { case MQTTPrefsAtomicStore::VerifiedImageResult::Committed: return true; @@ -1149,15 +1173,14 @@ bool CommonCLI::saveMQTTPrefs(FILESYSTEM* fs) { MESH_DEBUG_PRINTLN("MQTT: generated /mqtt.json temp failed schema validation; source preserved"); } break; + case MQTTPrefsAtomicStore::VerifiedImageResult::CleanupIndeterminate: + MESH_DEBUG_PRINTLN("MQTT: rejected /mqtt.json temp could not be removed and no primary outranks it; " + "recovery files preserved"); + break; case MQTTPrefsAtomicStore::VerifiedImageResult::CommitFailed: MESH_DEBUG_PRINTLN("MQTT: atomic /mqtt.json save failed during rename; transaction rolled back"); break; case MQTTPrefsAtomicStore::VerifiedImageResult::CommitIndeterminate: - // The failed publish could not be undone, so a recovery file this - // firmware may still promote is holding the new image. No further save - // can start until it is resolved: begin() refuses while either the temp - // or the backup exists, which is exactly the state that gets us here. - _observer_save_indeterminate = true; MESH_DEBUG_PRINTLN("MQTT: atomic /mqtt.json save failed during rename and could not be rolled back; " "recovery files preserved"); break; diff --git a/src/helpers/MQTTPrefsAtomicStore.h b/src/helpers/MQTTPrefsAtomicStore.h index 91b7ae39e..373dfa3b5 100644 --- a/src/helpers/MQTTPrefsAtomicStore.h +++ b/src/helpers/MQTTPrefsAtomicStore.h @@ -13,22 +13,32 @@ namespace MQTTPrefsAtomicStore { // commit() allowed to publish it. A schema-verification failure discards the // finished temp; a commit failure is undone by rollbackFailedCommit(). // -// CommitFailed and CommitIndeterminate are distinct outcomes, not shades of the -// same one. Publishing moves the old primary aside before the verified temp -// takes its name, so a half-done commit leaves an image that boot recovery -// would promote. CommitFailed means that was undone and the change is really -// gone; CommitIndeterminate means it could not be, and the next boot may still -// come up with the new value. Callers must not report the two the same way. +// A failed save that is really gone and one that may still surface at the next +// boot are distinct outcomes, not shades of the same one. Publishing moves the +// old primary aside before the verified temp takes its name, so a half-done +// commit leaves an image that boot recovery would promote; CommitFailed means +// that was undone, CommitIndeterminate means it could not be. The same applies +// before the commit: a rejected temp that cleanup could not delete is still an +// image recovery may promote when no primary outranks it, which is +// CleanupIndeterminate. Callers must not report these as a rollback. enum class VerifiedImageResult : uint8_t { Committed, BeginFailed, WriteFailed, FinishFailed, VerifyFailed, + CleanupIndeterminate, CommitFailed, CommitIndeterminate, }; +// The change may still be on flash at the next boot, so the caller must not +// tell the operator it was rolled back. +inline bool saveOutcomeUnresolved(VerifiedImageResult result) { + return result == VerifiedImageResult::CleanupIndeterminate || + result == VerifiedImageResult::CommitIndeterminate; +} + template inline VerifiedImageResult writeVerifiedImage(Store& store, ImageWriter write_image, @@ -38,16 +48,24 @@ inline VerifiedImageResult writeVerifiedImage(Store& store, return VerifiedImageResult::BeginFailed; } if (!write_image()) { + // A short write leaves a structurally incomplete image that recovery + // classifies as invalid, so a temp that survives cleanup here cannot be + // promoted and the change really is gone. store.abort(); return VerifiedImageResult::WriteFailed; } if (!store.finish()) { - store.abort(); - return VerifiedImageResult::FinishFailed; + // Unlike a short write, this does not prove the bytes on disk are + // unusable: finish() also fails when a complete temp cannot be read back. + return store.abort() ? VerifiedImageResult::FinishFailed + : VerifiedImageResult::CleanupIndeterminate; } if (!verify_image()) { - store.discardFinishedTemp(); - return VerifiedImageResult::VerifyFailed; + // The temp is complete and byte-verified, only rejected by the schema. If + // it cannot be deleted and no primary outranks it, boot recovery promotes + // the very image this call is about to report as not saved. + return store.discardFinishedTemp() ? VerifiedImageResult::VerifyFailed + : VerifiedImageResult::CleanupIndeterminate; } if (!store.commit()) { // Undo the partial publish before answering. Only the store knows whether diff --git a/src/helpers/MQTTPrefsRecovery.h b/src/helpers/MQTTPrefsRecovery.h index 4f6b15ac2..15eb014e4 100644 --- a/src/helpers/MQTTPrefsRecovery.h +++ b/src/helpers/MQTTPrefsRecovery.h @@ -16,7 +16,9 @@ // 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. +// which would delete it exactly when it finally became readable. When neither +// file can run this boot, RunDefaultsHeld keeps both names untouched for the +// same reason. namespace MQTTPrefsRecovery { enum class FileState : uint8_t { @@ -38,8 +40,15 @@ enum class Action : uint8_t { // Every later boot re-runs this policy against the same three names until one // of them can classify the candidate. UseBackupHeld, + // Neither the candidate nor the backup can run this boot. Change nothing, + // come up on defaults, and hold writes until a boot that can classify them. + RunDefaultsHeld, }; +inline bool uncertain(FileState state) { + return state == FileState::FutureClaimed || state == FileState::Indeterminate; +} + inline Action select(FileState primary, FileState temp, FileState backup) { // A primary of any kind owns the name. In particular, do not roll a newer // or corrupt primary back to an older backup just because it cannot be read @@ -63,10 +72,20 @@ 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 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::UseBackupHeld : Action::PromoteTemp; + // marks the candidate as mid-commit. + // + // A backup this firmware cannot run is still the previous committed image, so + // the same argument applies to it: promoting the candidate would give it the + // authoritative name, and the "any primary owns the name" rule above would + // then keep it even on a boot that proves it corrupt. Spend the transaction + // state recorded in the filenames only once the backup is definitively no + // longer a usable fallback. + if (uncertain(temp)) { + if (backup == FileState::Usable) return Action::UseBackupHeld; + if (backup == FileState::FutureUsable || uncertain(backup)) { + return Action::RunDefaultsHeld; + } + return Action::PromoteTemp; } // No temp survived. The backup is the only recoverable image, even when it @@ -75,8 +94,4 @@ inline Action select(FileState primary, FileState temp, FileState backup) { return Action::None; } -inline bool uncertain(FileState state) { - return state == FileState::FutureClaimed || state == FileState::Indeterminate; -} - } // namespace MQTTPrefsRecovery 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 a5b28be15..197948396 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 @@ -126,6 +126,8 @@ public: bool rollback_rename_fails = false; bool rollback_remove_fails = false; bool has_primary = true; + bool discard_remove_fails = false; + bool abort_remove_fails = false; }; explicit InMemoryJsonStore(FailurePoint failure) : _opts{failure, false, false, true} { @@ -157,8 +159,11 @@ public: bool finish() { ++finish_calls; _open = false; - if (_opts.failure == FailurePoint::Finish) return false; + // Production writes straight to /mqtt.json.tmp, so the bytes are on flash + // before finish() reads them back. A finish() failure can therefore be a + // failed read-back of an otherwise complete image, not a torn one. _files["/mqtt.json.tmp"] = _staging; + if (_opts.failure == FailurePoint::Finish) return false; _finished = true; return true; } @@ -196,18 +201,28 @@ public: return true; } - void discardFinishedTemp() { + // Both cleanups report the same disposition as production: false means a temp + // survived with no primary to outrank it, so boot recovery may still promote + // the image the caller is about to report as not saved. + bool discardFinishedTemp() { ++discard_calls; - _files.erase("/mqtt.json.tmp"); + const bool removed = !_opts.discard_remove_fails; + if (removed) _files.erase("/mqtt.json.tmp"); _finished = false; + return removed || has("/mqtt.json"); } - void abort() { + bool abort() { ++abort_calls; _open = false; _staging.clear(); - if (!_finished) _files.erase("/mqtt.json.tmp"); + bool removed = true; + if (!_finished && has("/mqtt.json.tmp")) { + removed = !_opts.abort_remove_fails; + if (removed) _files.erase("/mqtt.json.tmp"); + } _finished = false; + return removed || has("/mqtt.json"); } // Apply the boot-recovery policy to whatever the transaction left behind and @@ -467,8 +482,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::UseBackupHeld || + action == Recovery::Action::RunDefaultsHeld) { + return; // production renames nothing, so the transaction state survives } if (action == Recovery::Action::PromoteBackup) { rename("/mqtt_prefs.bak", "/mqtt_prefs"); @@ -605,6 +621,49 @@ TEST(MQTTPrefsAtomicStore, UnrestorablePublishFailureIsReportedAsIndeterminate) EXPECT_EQ(InMemoryJsonStore::newImage(), first_save.imageAfterReboot()); } +TEST(MQTTPrefsAtomicStore, RejectedTempThatCannotBeDeletedIsReportedAsIndeterminate) { + // The temp is a complete, byte-verified, perfectly valid image: only the + // scratch allocation that reparses it failed. With no primary on a fresh + // install, a temp that cleanup cannot delete is exactly what boot recovery + // promotes, so this must not be answered as a rollback. + InMemoryJsonStore first_save(InMemoryJsonStore::Options{ + FailurePoint::Verify, false, false, /*has_primary=*/false, + /*discard_remove_fails=*/true}); + ASSERT_EQ(AtomicStore::VerifiedImageResult::CleanupIndeterminate, + runVerifiedJson(&first_save)); + EXPECT_TRUE(AtomicStore::saveOutcomeUnresolved( + AtomicStore::VerifiedImageResult::CleanupIndeterminate)); + EXPECT_TRUE(first_save.has("/mqtt.json.tmp")); + EXPECT_FALSE(first_save.canStartSave()); // retries fail until this resolves + EXPECT_EQ(InMemoryJsonStore::newImage(), first_save.imageAfterReboot()); + + // With a primary published, the same stuck temp cannot win recovery, so the + // change really is gone and the ordinary rejection verdict still holds. + InMemoryJsonStore with_primary(InMemoryJsonStore::Options{ + FailurePoint::Verify, false, false, /*has_primary=*/true, + /*discard_remove_fails=*/true}); + ASSERT_EQ(AtomicStore::VerifiedImageResult::VerifyFailed, + runVerifiedJson(&with_primary)); + EXPECT_FALSE(AtomicStore::saveOutcomeUnresolved( + AtomicStore::VerifiedImageResult::VerifyFailed)); + EXPECT_EQ(InMemoryJsonStore::oldImage(), with_primary.imageAfterReboot()); +} + +TEST(MQTTPrefsAtomicStore, UnverifiedTempThatCannotBeDeletedIsReportedAsIndeterminate) { + // finish() failing does not prove the bytes are torn — a complete image whose + // read-back failed still parses at the next boot. If abort() cannot remove it + // and no primary outranks it, the outcome is unresolved for the same reason. + InMemoryJsonStore store(InMemoryJsonStore::Options{ + FailurePoint::Finish, false, false, /*has_primary=*/false, + /*discard_remove_fails=*/false, /*abort_remove_fails=*/true}); + + ASSERT_EQ(AtomicStore::VerifiedImageResult::CleanupIndeterminate, + runVerifiedJson(&store)); + EXPECT_TRUE(store.has("/mqtt.json.tmp")); + EXPECT_FALSE(store.canStartSave()); + EXPECT_EQ(InMemoryJsonStore::newImage(), store.imageAfterReboot()); +} + TEST(MQTTPrefsAtomicStore, FirstSavePublishFailureLeavesNoImageToPromote) { InMemoryJsonStore store(InMemoryJsonStore::Options{ FailurePoint::Commit, false, false, /*has_primary=*/false}); @@ -1010,6 +1069,49 @@ TEST(MQTTPrefsAtomicStore, UncertainTempWithNoUsableBackupStillOwnsThePrimaryNam Recovery::FileState::Preserve)); } +TEST(MQTTPrefsAtomicStore, UncertainTempDoesNotSpendAnOpaqueBackup) { + // Newer firmware was replacing a future-version primary when power was lost. + // This boot can prove the backup is a syntactically valid future image but + // cannot run it, and cannot classify the candidate at all. + EXPECT_EQ(Recovery::Action::RunDefaultsHeld, + Recovery::select(Recovery::FileState::Missing, + Recovery::FileState::FutureClaimed, + Recovery::FileState::FutureUsable)); + EXPECT_EQ(Recovery::Action::RunDefaultsHeld, + Recovery::select(Recovery::FileState::Missing, + Recovery::FileState::Indeterminate, + Recovery::FileState::Indeterminate)); + // A backup that is definitively invalid has ceased to be a fallback, so the + // candidate may take the authoritative name. + EXPECT_EQ(Recovery::Action::PromoteTemp, + Recovery::select(Recovery::FileState::Missing, + Recovery::FileState::Indeterminate, + Recovery::FileState::Preserve)); +} + +TEST(MQTTPrefsAtomicStore, HeldOpaqueBackupSurvivesACandidateThatProvesInvalid) { + SpiffsMqttTransaction store; + store.cutAt(SpiffsMqttTransaction::Boundary::AfterBackupRename); + + // Nothing is renamed, so the empty primary name still records the interrupted + // commit and the previous committed image keeps its backup name. + store.recover(Recovery::FileState::Missing, Recovery::FileState::FutureClaimed, + Recovery::FileState::FutureUsable); + EXPECT_FALSE(store.has("/mqtt_prefs")); + EXPECT_TRUE(store.has("/mqtt_prefs.tmp")); + EXPECT_TRUE(store.has("/mqtt_prefs.bak")); + EXPECT_FALSE(store.canStartSave()); + + // A later boot understands both and finds the candidate corrupt. Because the + // first boot did not spend the primary name on it, the last committed image + // is still there to publish instead of being stranded behind a bad primary. + 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, UsablePrimaryDoesNotCleanIndeterminateArtifact) { SpiffsMqttTransaction store; store.cutDuringTempWrite();