fix(mqtt): report an undeletable rejected /mqtt.json temp as unresolved

Cleanup before the commit phase ignored whether the temp was actually
removed. On a fresh install a complete, byte-verified temp that failed
schema verification (or whose read-back failed in finish()) could survive
a failed remove() with no primary to outrank it, and boot recovery then
promoted the very value the CLI had just reported as rolled back.

Both cleanups now return a disposition: success means the temp is gone or
an existing primary is authoritative. A false disposition maps to the new
CleanupIndeterminate, which latches the same indeterminate reply as a
commit that could not be rolled back. A short write is excluded, since it
leaves structurally incomplete JSON that recovery classifies as invalid.

Recovery also stops spending transaction state on an opaque backup: an
uncertain temp beside a FutureUsable or uncertain backup now holds both
names and runs defaults instead of promoting the candidate into the
authoritative name, where the "any primary owns the name" rule would keep
it even after a later boot proved it corrupt.

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;

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 <typename Store, typename ImageWriter, typename ImageVerifier>
 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

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

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();