From a96900d7ee856d08c52e035cb43a6853cb4ea413 Mon Sep 17 00:00:00 2001 From: Kaj Schittecat Date: Thu, 13 Aug 2026 16:36:37 +0200 Subject: [PATCH] core: patch changed contact records in place instead of rewriting the table (#222) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Yoss101 reported the beta_62/63 fix helped but did not cure it: the freezes got rarer AND longer. Rarer is the blob-delete queue working; longer is what was left, and it was always the bigger half. MAX_CONTACTS is 2000 and a contact record is 152 bytes, so saveContacts was rewriting a 304 KB file on every change, with a same-sized .tmp alongside it for the atomic swap. On a card-less V4 that is ~608 KB of peak churn on a 3.375 MB SPIFFS volume that is already carrying one blob file per contact. SPIFFS GC cost scales with how full the volume is, and GC suspends the flash cache, which stalls BOTH cores — so the fuller the table, the longer the device is simply gone. The table never actually changes in bulk: eviction replaces contacts[oldest] in place (the array is unsorted and never shifts) and an advert refresh touches one entry. So compare each record against what is already on disk and write back only the ones that differ. An eviction now writes 152 bytes instead of 304 KB, an unchanged table writes nothing, and there is no .tmp and no free-space spike. Reads never trigger GC, so the compare scan is cheap. Falls back to the full atomic rewrite whenever the mapping is not provably safe (no live file, ragged size, or a table that shrank — there is no truncate here), and never truncates or renames, so a fallback always leaves a valid list on disk. Verified against the real function source with an in-memory FS harness: steady state, eviction, growth, shrink, filtered anon slots, missing file, ragged file, and 167 scattered changes all match a full rewrite byte-for-byte. Also: the blob-delete queue silently ORPHANED a blob on overflow — nothing else ever deletes it, so it leaked flash permanently on the one metric that drives GC cost. Depth 8 -> 32, and overflows are now counted and surfaced. New About > diagnostics block "Contact store": contact count, approximate on-disk size, internal-flash used percentage, and orphaned blobs. The used percentage is the number that predicts these freezes, so a reporter can photograph it. Co-Authored-By: Claude Opus 5 --- src/DataStore.cpp | 117 ++++++++++++++++++++++++++++++++++++++++ src/DataStore.h | 5 ++ src/MyMesh.cpp | 7 ++- src/MyMesh.h | 8 ++- src/ui-touch/UITask.cpp | 20 +++++++ 5 files changed, 155 insertions(+), 2 deletions(-) diff --git a/src/DataStore.cpp b/src/DataStore.cpp index f39136d..8d3494d 100644 --- a/src/DataStore.cpp +++ b/src/DataStore.cpp @@ -496,6 +496,118 @@ File file = openRead(_getContactsChannelsFS(), "/contacts3"); } } +#if defined(ESP32) +// A full contacts write REWRITES THE WHOLE TABLE — 152 bytes per contact, so 304 KB at +// MAX_CONTACTS=2000 — and the atomic swap below keeps a same-sized .tmp alongside it, so +// the peak cost is ~608 KB of a card-less V4's 3.375 MB SPIFFS volume that is already +// carrying one blob file per contact. SPIFFS garbage collection suspends the flash cache, +// which stalls BOTH cores, and it gets dramatically worse the fuller the volume — that is +// the #222 freeze, and taking the eviction blob delete off the packet path only removed +// the smaller half of it (the reporter still saw stalls, and LONGER ones, because this +// rewrite is what was left). +// +// But the table only ever CHANGES one slot at a time: eviction replaces contacts[oldest] +// in place (the array is unsorted and never shifts), and an advert refresh touches a +// single entry. So compare each record we would write against the record already on disk +// and patch only the ones that actually differ — 152 bytes instead of 304 KB, with no +// .tmp and no free-space spike. Reads never trigger GC, so the scan itself is cheap. +// +// Returns true only if the live file is now fully up to date. False means "not +// applicable" — the caller must fall back to the full atomic rewrite. This never +// truncates or renames, and the fallback rewrite repairs a partial patch, so a false +// return always ends with a valid list on disk. +bool DataStore::saveContactsInPlace(DataStoreHost* host, bool (*filter)(const ContactInfo& c)) { + static const size_t REC = 152; // MUST match the record packing in saveContacts + static const size_t CHUNK_RECS = 16; + + FILESYSTEM* fs = _getContactsChannelsFS(); + if (!fs->exists(_rp("/contacts3"))) return false; // no live file yet -> full write + + // Count first, with no I/O at all. A table that SHRANK needs records dropped off the + // end and this FS API has no truncate, so bail BEFORE touching the file — that keeps + // the fallback a clean rewrite instead of a half-patched one. + uint32_t nrec = 0; + { + uint32_t idx = 0; + ContactInfo c; + while (host->getContactForSave(idx, c)) { + if (!filter || filter(c)) nrec++; + idx++; + } + } + + File f = fs->open(_rp("/contacts3"), "r+"); + if (!f) return false; + const size_t fsz = f.size(); + if (fsz == 0 || (fsz % REC) != 0 || (fsz / REC) > nrec) { f.close(); return false; } + const uint32_t nfile = fsz / REC; // records currently on disk + + uint8_t* wbuf = (uint8_t*)malloc(REC * CHUNK_RECS); + uint8_t* rbuf = (uint8_t*)malloc(REC * CHUNK_RECS); + if (!wbuf || !rbuf) { free(wbuf); free(rbuf); f.close(); return false; } + + bool ok = true; + uint32_t base = 0; // record number of wbuf[0] + size_t fill = 0; // records packed in wbuf + + // Reconcile one slab: read what is there, and write back only the records that differ. + // Records at or past the old end of file are appends (the table grew), written in + // ascending order so each one extends the file contiguously. + auto reconcile = [&](uint32_t at, size_t count) -> bool { + const size_t off = (size_t)at * REC; + const size_t bytes = count * REC; + size_t in_file = (at < nfile) ? ((size_t)(nfile - at) * REC) : 0; + if (in_file > bytes) in_file = bytes; + if (in_file) { + if (!f.seek(off)) return false; + if ((size_t)f.read(rbuf, in_file) != in_file) return false; + if (in_file == bytes && memcmp(rbuf, wbuf, bytes) == 0) return true; // slab unchanged + } + for (size_t k = 0; k < count; k++) { + const size_t ro = k * REC; + if (ro + REC <= in_file && memcmp(rbuf + ro, wbuf + ro, REC) == 0) continue; + if (!f.seek(off + ro)) return false; + if (f.write(wbuf + ro, REC) != REC) return false; + } + return true; + }; + + uint32_t idx = 0; + ContactInfo c; + uint8_t unused = 0; + while (ok) { + if (!host->getContactForSave(idx, c)) break; + idx++; + if (filter && !filter(c)) continue; + + uint8_t* p = wbuf + fill * REC; + memcpy(p, c.id.pub_key, 32); p += 32; + memcpy(p, (uint8_t *)&c.name, 32); p += 32; + *p++ = c.type; + *p++ = c.flags; + *p++ = unused; + memcpy(p, (uint8_t *)&c.sync_since, 4); p += 4; + memcpy(p, (uint8_t *)&c.out_path_len, 1); p += 1; + memcpy(p, (uint8_t *)&c.last_advert_timestamp, 4); p += 4; + memcpy(p, c.out_path, 64); p += 64; + memcpy(p, (uint8_t *)&c.lastmod, 4); p += 4; + memcpy(p, (uint8_t *)&c.gps_lat, 4); p += 4; + memcpy(p, (uint8_t *)&c.gps_lon, 4); p += 4; + fill++; + + if (fill == CHUNK_RECS) { ok = reconcile(base, fill); base += fill; fill = 0; } + } + if (ok && fill > 0) { ok = reconcile(base, fill); base += fill; fill = 0; } + + f.close(); + free(wbuf); + free(rbuf); + // base is the number of records reconciled; if it disagrees with the pre-count the + // table changed underneath us — let the caller rewrite rather than trust the file. + return ok && base == nrec; +} +#endif + void DataStore::saveContacts(DataStoreHost* host, bool (*filter)(const ContactInfo& c)) { #if defined(HAS_TDISPLAY_P4) if (!p4OnStorageTask()) { @@ -514,6 +626,11 @@ void DataStore::saveContacts(DataStoreHost* host, bool (*filter)(const ContactIn // that calls saveContacts stays balanced. On SD-routed devices this is cheap // (FAT has no such GC); it matters most for card-less SPIFFS devices. WdtHeavyGuard _wdt; + + // Patch the live file in place when only individual records changed — the normal case + // by far, and the one that was freezing card-less V4s (#222). Falls through to the full + // atomic rewrite below whenever that is not provably safe. + if (saveContactsInPlace(host, filter)) return; #endif #if defined(ESP32) // Write to a TEMP file and swap it in only after it is FULLY written, so a partial diff --git a/src/DataStore.h b/src/DataStore.h index 884d12f..3637d85 100644 --- a/src/DataStore.h +++ b/src/DataStore.h @@ -64,6 +64,11 @@ public: bool savePrefs(const NodePrefs& prefs, double node_lat, double node_lon); void loadContacts(DataStoreHost* host); void saveContacts(DataStoreHost* host, bool (*filter)(const ContactInfo& c) = NULL); +#if defined(ESP32) + // Patch only the contact records that changed, in place. True = live file is up to + // date; false = caller must do the full atomic rewrite. See the definition for why. + bool saveContactsInPlace(DataStoreHost* host, bool (*filter)(const ContactInfo& c)); +#endif void loadChannels(DataStoreHost* host); void saveChannels(DataStoreHost* host); void migrateToSecondaryFS(); diff --git a/src/MyMesh.cpp b/src/MyMesh.cpp index 1ff8815..de142d0 100644 --- a/src/MyMesh.cpp +++ b/src/MyMesh.cpp @@ -2329,7 +2329,12 @@ void MyMesh::onContactOverwrite(const uint8_t* pub_key) { // it, so a burst of evictions cannot chain GC passes back to back. if (_pending_del_n < PENDING_DEL_MAX) { memcpy(_pending_del[_pending_del_n++], pub_key, PUB_KEY_SIZE); - } // queue full: the blob is orphaned, harmless — reclaimed on the next wipe + } else { + // Overflow orphans the blob for good — no other path deletes it — so it is not the + // "harmless" case the first cut of this assumed. Count it so a device that is + // actually hitting the wall can say so instead of silently leaking flash. + if (_orphaned_blobs < 0xFFFF) _orphaned_blobs++; + } if (_serial->isConnected()) { out_frame[0] = PUSH_CODE_CONTACT_DELETED; memcpy(&out_frame[1], pub_key, PUB_KEY_SIZE); diff --git a/src/MyMesh.h b/src/MyMesh.h index 733f36d..807e6b7 100644 --- a/src/MyMesh.h +++ b/src/MyMesh.h @@ -145,6 +145,7 @@ public: NodePrefs *getNodePrefs(); uint32_t getBLEPin(); bool setBLEPin(uint32_t pin); // user-chosen 6-digit pairing code (persisted; applies next boot) + uint32_t getOrphanedBlobs() const { return _orphaned_blobs; } // see PENDING_DEL_MAX (#222) // Live device info accessors (used by the touch Settings → Device modal to // mirror the web client's "Device (live)" panel — public key prefix, channel @@ -265,10 +266,15 @@ protected: void onContactOverwrite(const uint8_t* pub_key) override; // Blob deletes queued by onContactOverwrite and drained from loop() — never // from packet handling, see #222 (SPIFFS GC stalls both cores). - static const uint8_t PENDING_DEL_MAX = 8; + // 8 was too shallow: a drop here does not just skip work, it ORPHANS the blob file + // permanently (nothing else ever deletes it), and orphans accumulate on the one + // resource that drives GC cost — how full the volume is. 32 costs 1 KB of RAM and + // needs a 16-second eviction burst to overflow. + static const uint8_t PENDING_DEL_MAX = 32; uint8_t _pending_del[PENDING_DEL_MAX][PUB_KEY_SIZE]; uint8_t _pending_del_n = 0; uint32_t _next_pending_del_at = 0; + uint16_t _orphaned_blobs = 0; // queue overflows — each one leaks a blob file for good void drainPendingBlobDeletes(); bool onContactPathRecv(ContactInfo& from, uint8_t* in_path, uint8_t in_path_len, uint8_t* out_path, uint8_t out_path_len, uint8_t extra_type, uint8_t* extra, uint8_t extra_len) override; void onDiscoveredContact(ContactInfo &contact, bool is_new, uint8_t path_len, const uint8_t* path) override; diff --git a/src/ui-touch/UITask.cpp b/src/ui-touch/UITask.cpp index 546cbe5..495b6d1 100644 --- a/src/ui-touch/UITask.cpp +++ b/src/ui-touch/UITask.cpp @@ -10995,6 +10995,26 @@ static void sysInfoTextRest(char* buf, size_t cap) { (unsigned)nvs.free_entries, (unsigned)nvs.namespace_count); } + // Contact store pressure — the #222 diagnostic. The table is a flat file of 152-byte + // records, so its size follows the contact count directly, and on a card-less board it + // shares the internal volume with one blob file per contact. SPIFFS GC cost is driven by + // how FULL that volume is, and GC suspends the flash cache, which stalls both cores. So + // "used %" here is the number that predicts freezes; a reporter can photograph it. + { + const int nc = the_mesh.getNumContacts(); + p += snprintf(buf + p, cap - p, "Contact store\n %d / %d contacts (~%u KB)\n", + nc, (int)MAX_CONTACTS, (unsigned)(((uint32_t)nc * 152u) / 1024u)); + const size_t sp_tot = SPIFFS.totalBytes(), sp_used = SPIFFS.usedBytes(); + if (sp_tot) { + p += snprintf(buf + p, cap - p, " internal flash: %u / %u KB (%u%%)\n", + (unsigned)(sp_used / 1024u), (unsigned)(sp_tot / 1024u), + (unsigned)((uint64_t)sp_used * 100ull / sp_tot)); + } + const uint32_t orph = the_mesh.getOrphanedBlobs(); + if (orph) p += snprintf(buf + p, cap - p, " orphaned blobs: %lu\n", (unsigned long)orph); + p += snprintf(buf + p, cap - p, "\n"); + } + p += snprintf(buf + p, cap - p, "Last reset\n %s\n\n", resetReasonString(esp_reset_reason())); #endif