Preserve contacts when source discovery is unknown

Do not classify legacy or page-name stat errors as unread payloads. Keep candidate contact files unchanged, retry source selection, and use bounded volatile operation if authority remains unknown. Cover legacy absence, downgrade leftovers, cleanup-time metadata recovery and persistent faults with actual-source sanitizer tests and restored-bug negative controls.

docs/research/companion_contact_cache.md

@@ -44,7 +44,10 @@ If damaged contacts cannot be removed, the node remains usable with volatile
 contacts; explicit saves still report failure, while reboot/shutdown remain
 available. Identity, credentials, channels and unrelated settings are not erased.
 Allocation/capacity failures and shared filesystem-migration authority errors
-are not corrupt-contact evidence and do not authorize this reset. The path checksum also rejects
+are not corrupt-contact evidence and do not authorize this reset. Failed source
+discovery also preserves contact files; if authority stays unknown, use volatile
+contacts instead of deleting readable pages or selecting a potentially stale view.
+The path checksum also rejects
 a cold path changed underneath a live handle; it does not add a new checksum
 to the legacy ESP32 on-disk format.
 

examples/companion_radio/DataStore.cpp

@@ -2369,14 +2369,14 @@ void DataStore::loadContacts(DataStoreHost* host) {
       MESH_DEBUG_PRINTLN(
           "DataStore: HIL injecting contact page %u stat failure", page);
       _contact_load_incomplete = true;
-      _unread_contact_pages.mark(page);
       queueContactRecovery();
       return;
     }
 #endif
     if (!contactPathPresence(contacts_fs, path, page_exists)) {
       _contact_load_incomplete = true;
-      _unread_contact_pages.mark(page);
+      // Discovery failed before the contact source was selected. This is not
+      // evidence that the page payload is unreadable or safe to discard.
       queueContactRecovery();
       return;
     }
@@ -2388,7 +2388,9 @@ void DataStore::loadContacts(DataStoreHost* host) {
   bool migration_marker_exists = false;
   if (!contactPathPresence(contacts_fs, "/contacts3", legacy_exists)) {
     _contact_load_incomplete = true;
-    queueContactRecovery(true);
+    // Unknown legacy presence does not make readable pages expendable: they
+    // may be the only authoritative store. Retry without authorizing erasure.
+    queueContactRecovery();
     return;
   }
   if (!contactPathPresence(contacts_fs, CONTACT_MIGRATION_MARKER,

test/fixtures/contact_cache/test_contact_cache.cpp

@@ -47,7 +47,14 @@ using DataStoreHost = Host;
 #if defined(NRF52_PLATFORM)
 static const char* CONTACT_MIGRATION_MARKER = "/contacts4.mig";
 static bool fail_contact_migration_marker = false;
+static bool fail_contact_legacy_stat = false;
+static int fail_contact_page_stats = 0;
 bool contactPathPresence(FakeFilesystem* fs, const char* path, bool& present) {
+  if (fail_contact_page_stats && !strcmp(path, "/contacts4_00")) {
+    if (fail_contact_page_stats > 0) --fail_contact_page_stats;
+    return false;
+  }
+  if (fail_contact_legacy_stat && !strcmp(path, "/contacts3")) return false;
   if (fail_contact_migration_marker && !strcmp(path, CONTACT_MIGRATION_MARKER)) return false;
   present = fs->exists(path);
   return true;
@@ -1090,6 +1097,108 @@ static void nrf52_marker_error_keeps_a_usable_contact_view() {
   assert(SPIFFS.files == durable && SPIFFS.writes == writes && SPIFFS.removes == removes);
 }
 
+static void nrf52_unknown_legacy_stat_preserves_contact_authority() {
+  for (bool legacy_present : {false, true}) {
+    for (bool persistent_failure : {false, true}) {
+      Fixture f(30);
+      assert(f.store.writeContactPage(&f.host, 0, cachedContactFilter));
+      assert(f.store.writeContactPage(&f.host, 1, cachedContactFilter));
+      if (legacy_present) {
+        // A downgrade may leave a shorter authoritative legacy view beside
+        // stale, otherwise completely readable pages. No migration marker
+        // permits treating those extra records as a committed page tail.
+        SPIFFS.files["/contacts3"].resize(5 * mesh::storage::CONTACT_RECORD_SIZE);
+      } else {
+        SPIFFS.remove("/contacts3");
+      }
+      const FakeFilesystem::Files unrelated = {
+          {"/_main.id", {1, 3}}, {"/new_prefs", {2, 4}}, {"/channels2", {5, 7}},
+          {"/.extrafs.mig", {'M', 'C', 'X', 'F', 1, 2}}};
+      for (const auto& file : unrelated) SPIFFS.files[file.first] = file.second;
+      const auto durable = SPIFFS.files;
+      const unsigned writes = SPIFFS.writes, removes = SPIFFS.removes;
+      f.host.contacts.clear();
+      fail_contact_legacy_stat = true;
+      f.store.loadContacts(&f.host);
+      assert(f.store.contactRecoveryPending() && f.store.hasIncompleteContactLoad());
+      assert(f.host.contacts.empty() && SPIFFS.files == durable);
+      if (!persistent_failure) fail_contact_legacy_stat = false;
+      f.store.recoverContacts(&f.host);
+      assert(!f.store.contactRecoveryPending() && !f.store.hasIncompleteContactLoad());
+      assert(f.store.contactsAreVolatile() == persistent_failure);
+      const unsigned expected_contacts = legacy_present ? 5 : 30;
+      assert(f.host.contacts.size() == (persistent_failure ? 0 : expected_contacts));
+      if (persistent_failure) {
+        // Unknown source authority is not a corrupt legacy image. Keep both
+        // candidate namespaces unchanged and permit usable volatile hardware.
+        assert(SPIFFS.files == durable);
+        assert(SPIFFS.writes == writes && SPIFFS.removes == removes);
+        assert(!f.store.saveContacts(&f.host, cachedContactFilter));
+        f.store.recoverContacts(&f.host);
+        assert(SPIFFS.files == durable && f.host.contacts.empty());
+      }
+      for (const auto& file : unrelated) assert(SPIFFS.files.at(file.first) == file.second);
+      fail_contact_legacy_stat = false;
+      f.host.contacts.clear();
+      DataStore rebooted(f.host);
+      rebooted.loadContacts(&f.host);
+      assert(!rebooted.hasIncompleteContactLoad() && !rebooted.contactRecoveryPending());
+      assert(!rebooted.contactsAreVolatile() && f.host.contacts.size() == expected_contacts);
+      for (unsigned i = 0; i < expected_contacts; ++i) f.check(i, route(i));
+    }
+  }
+}
+
+static void nrf52_unknown_page_stat_preserves_contact_authority() {
+  for (bool legacy_present : {false, true}) {
+    for (int failures : {1, 2, -1}) {
+      Fixture f(30);
+      assert(f.store.writeContactPage(&f.host, 0, cachedContactFilter));
+      assert(f.store.writeContactPage(&f.host, 1, cachedContactFilter));
+      if (legacy_present) {
+        SPIFFS.files["/contacts3"].resize(5 * mesh::storage::CONTACT_RECORD_SIZE);
+      } else {
+        SPIFFS.remove("/contacts3");
+      }
+      const FakeFilesystem::Files unrelated = {
+          {"/_main.id", {1, 3}}, {"/new_prefs", {2, 4}}, {"/channels2", {5, 7}},
+          {"/.extrafs.mig", {'M', 'C', 'X', 'F', 1, 2}}};
+      for (const auto& file : unrelated) SPIFFS.files[file.first] = file.second;
+      const auto durable = SPIFFS.files;
+      const unsigned writes = SPIFFS.writes, removes = SPIFFS.removes;
+      f.host.contacts.clear();
+      fail_contact_page_stats = failures;
+      f.store.loadContacts(&f.host);
+      assert(f.store.contactRecoveryPending() && f.store.hasIncompleteContactLoad());
+      assert(f.host.contacts.empty() && SPIFFS.files == durable);
+      f.store.recoverContacts(&f.host);
+      const bool persistent_failure = failures < 0;
+      assert(!f.store.contactRecoveryPending() && !f.store.hasIncompleteContactLoad());
+      assert(f.store.contactsAreVolatile() == persistent_failure);
+      const unsigned expected_contacts = legacy_present ? 5 : 30;
+      assert(f.host.contacts.size() == (persistent_failure ? 0 : expected_contacts));
+      // In particular, two metadata failures clear just before the cleanup
+      // probe. That later successful stat is not permission to erase a page
+      // whose complete bytes never failed validation or even a read attempt.
+      assert(SPIFFS.files == durable);
+      assert(SPIFFS.writes == writes && SPIFFS.removes == removes);
+      for (const auto& file : unrelated) assert(SPIFFS.files.at(file.first) == file.second);
+      if (persistent_failure) {
+        assert(!f.store.saveContacts(&f.host, cachedContactFilter));
+        f.store.recoverContacts(&f.host);
+        assert(SPIFFS.files == durable && f.host.contacts.empty());
+      }
+      fail_contact_page_stats = 0;
+      f.host.contacts.clear();
+      DataStore rebooted(f.host);
+      rebooted.loadContacts(&f.host);
+      assert(!rebooted.hasIncompleteContactLoad() && !rebooted.contactRecoveryPending());
+      assert(!rebooted.contactsAreVolatile() && f.host.contacts.size() == expected_contacts);
+      for (unsigned i = 0; i < expected_contacts; ++i) f.check(i, route(i));
+    }
+  }
+}
+
 static void nrf52_unread_legacy_and_unknown_marker_cannot_resurrect_stale_pages() {
   Fixture f(30);
   assert(f.store.writeContactPage(&f.host, 0, cachedContactFilter));
@@ -1225,6 +1334,8 @@ int main() {
   nrf52_migration_snapshots_and_failed_page_replacement();
   nrf52_resource_refusals_never_discard_contact_pages();
   nrf52_marker_error_keeps_a_usable_contact_view();
+  nrf52_unknown_legacy_stat_preserves_contact_authority();
+  nrf52_unknown_page_stat_preserves_contact_authority();
   nrf52_unread_legacy_and_unknown_marker_cannot_resurrect_stale_pages();
   nrf52_uncertain_page_publication_preserves_snapshots();
   encoded_paths_and_copy_assignment();

test/test_contact_cache.py

@@ -50,7 +50,14 @@ class ContactCacheTest(unittest.TestCase):
     def test_esp32_disabled_contact_recovery_is_detected(self):
         self.run_cache_test("ESP32_PLATFORM", disable_recovery=True)
 
-    def run_cache_test(self, platform, disable_crc_batch=False, disable_recovery=False):
+    def test_nrf52_unsafe_legacy_discovery_recovery_is_detected(self):
+        self.run_cache_test("NRF52_PLATFORM", unsafe_legacy_discovery=True)
+
+    def test_nrf52_unsafe_page_discovery_recovery_is_detected(self):
+        self.run_cache_test("NRF52_PLATFORM", unsafe_page_discovery=True)
+
+    def run_cache_test(self, platform, disable_crc_batch=False, disable_recovery=False,
+                       unsafe_legacy_discovery=False, unsafe_page_discovery=False):
         with tempfile.TemporaryDirectory(prefix="mesh-contact-cache-") as temp:
             temp = Path(temp)
             store = (ROOT / "examples/companion_radio/DataStore.cpp").read_text()
@@ -112,6 +119,22 @@ class ContactCacheTest(unittest.TestCase):
                 self.assertIn(recovery, implementation)
                 implementation = implementation.replace(
                     recovery, "void DataStore::recoverContacts(DataStoreHost*) {}")
+            if unsafe_legacy_discovery:
+                discovery = method(store,
+                    'if (!contactPathPresence(contacts_fs, "/contacts3", legacy_exists))')
+                self.assertEqual(discovery.count("queueContactRecovery();"), 1)
+                self.assertEqual(implementation.count(discovery), 1)
+                implementation = implementation.replace(discovery, discovery.replace(
+                    "queueContactRecovery();", "queueContactRecovery(true);"))
+            if unsafe_page_discovery:
+                discovery = method(store,
+                    "if (!contactPathPresence(contacts_fs, path, page_exists))")
+                self.assertNotIn("_unread_contact_pages.mark(page);", discovery)
+                self.assertEqual(discovery.count("queueContactRecovery();"), 1)
+                self.assertEqual(implementation.count(discovery), 1)
+                implementation = implementation.replace(discovery, discovery.replace(
+                    "queueContactRecovery();",
+                    "_unread_contact_pages.mark(page);\n      queueContactRecovery();"))
             if platform == "NRF52_PLATFORM":
                 # Fault only the page snapshot allocation; keep production
                 # loading/recovery logic and unrelated crypto allocations.
@@ -159,7 +182,15 @@ class ContactCacheTest(unittest.TestCase):
             ], capture_output=True, text=True)
             self.assertEqual(result.returncode, 0, result.stderr)
             result = subprocess.run([str(binary)], capture_output=True, text=True)
-            if disable_recovery:
+            if unsafe_page_discovery:
+                self.assertNotEqual(result.returncode, 0)
+                self.assertIn("nrf52_unknown_page_stat_preserves_contact_authority", result.stderr)
+                self.assertIn("f.host.contacts.size()", result.stderr)
+            elif unsafe_legacy_discovery:
+                self.assertNotEqual(result.returncode, 0)
+                self.assertIn("nrf52_unknown_legacy_stat_preserves_contact_authority", result.stderr)
+                self.assertIn("SPIFFS.files == durable", result.stderr)
+            elif disable_recovery:
                 self.assertNotEqual(result.returncode, 0)
                 self.assertIn("!f.store.contactRecoveryPending()", result.stderr)
             elif disable_crc_batch: