From 1e09c5ea98d1b1be9db97b1aadd9da64b839d7df Mon Sep 17 00:00:00 2001 From: "torlando-agent[bot]" <281092095+torlando-agent[bot]@users.noreply.github.com> Date: Wed, 29 Jul 2026 09:30:27 +0000 Subject: [PATCH] fix: fail closed on SD recovery errors --- lib/tdeck_ui/Hardware/TDeck/MapTileStore.cpp | 61 +++++++++--------- .../Hardware/TDeck/MapTileStoreSD.cpp | 62 +++++++++++++++---- .../test_map_tile_store_contract.py | 11 +++- tests/native/test_map_tile_store.cpp | 12 +++- tests/native/test_map_tile_store.py | 2 +- 5 files changed, 104 insertions(+), 44 deletions(-) diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileStore.cpp b/lib/tdeck_ui/Hardware/TDeck/MapTileStore.cpp index 90610f27..f261703d 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileStore.cpp +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileStore.cpp @@ -330,38 +330,41 @@ TileStoreResult MapTileStore::recoverEvictionTransaction() { TileStoreResult MapTileStore::recoverIndex() { TileStoreResult result = recoverEvictionTransaction(); if (result != TileStoreResult::OK) return result; - // With no manifest, every .evict file belongs to a committed transaction. - // Remove these in a separate list pass so they cannot consume recovery - // index slots, including when max_entries == HARD_MAX_ENTRIES. - TileKey committed_evictions[HARD_MAX_ENTRIES] = {}; - std::uint16_t committed_count = 0U; - result = storage_.beginList(); - if (result != TileStoreResult::OK) return result; + // With no manifest, every .evict file belongs to a committed transaction + // and every .tmp tile belongs to an uncommitted put. Remove cleanup artifacts + // in bounded batches before collecting live generations, so any number of + // stale tombstones and HARD_MAX live keys plus one crash temp remain recoverable. while (true) { - char name[PATH_CAPACITY] = {}; - bool done = false; - result = storage_.nextList(name, sizeof(name), done); - if (result != TileStoreResult::OK) { storage_.endList(); return result; } - if (done) break; - TileKey key = {0U, 0U, 0U}; - std::uint8_t flag = 0U; - result = parseOwnedPath(name, key, flag); - if (result != TileStoreResult::OK) { storage_.endList(); return result; } - if (flag == HAS_EVICT) { - if (committed_count >= HARD_MAX_ENTRIES) { - storage_.endList(); - return TileStoreResult::INDEX_FULL; + TileKey cleanup_keys[HARD_MAX_ENTRIES] = {}; + std::uint8_t cleanup_flags[HARD_MAX_ENTRIES] = {}; + std::uint16_t cleanup_count = 0U; + result = storage_.beginList(); + if (result != TileStoreResult::OK) return result; + while (cleanup_count < HARD_MAX_ENTRIES) { + char name[PATH_CAPACITY] = {}; + bool done = false; + result = storage_.nextList(name, sizeof(name), done); + if (result != TileStoreResult::OK) { storage_.endList(); return result; } + if (done) break; + TileKey key = {0U, 0U, 0U}; + std::uint8_t flag = 0U; + result = parseOwnedPath(name, key, flag); + if (result != TileStoreResult::OK) { storage_.endList(); return result; } + if ((flag == HAS_EVICT) || (flag == HAS_TEMP)) { + cleanup_keys[cleanup_count] = key; + cleanup_flags[cleanup_count++] = flag; } - committed_evictions[committed_count++] = key; } - } - storage_.endList(); - for (std::uint16_t i = 0U; i < committed_count; ++i) { - char live[PATH_CAPACITY] = {}, evicted[PATH_CAPACITY] = {}; - canonicalPath(committed_evictions[i], live, sizeof(live)); - appendSuffix(live, ".evict", evicted, sizeof(evicted)); - result = storage_.remove(evicted); - if ((result != TileStoreResult::OK) && (result != TileStoreResult::MISS)) return result; + storage_.endList(); + if (cleanup_count == 0U) break; + for (std::uint16_t i = 0U; i < cleanup_count; ++i) { + char live[PATH_CAPACITY] = {}, artifact[PATH_CAPACITY] = {}; + canonicalPath(cleanup_keys[i], live, sizeof(live)); + appendSuffix(live, (cleanup_flags[i] == HAS_EVICT) ? ".evict" : ".tmp", + artifact, sizeof(artifact)); + result = storage_.remove(artifact); + if ((result != TileStoreResult::OK) && (result != TileStoreResult::MISS)) return result; + } } result = storage_.beginList(); diff --git a/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp b/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp index b0fb4b80..1e15490e 100644 --- a/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp +++ b/lib/tdeck_ui/Hardware/TDeck/MapTileStoreSD.cpp @@ -4,11 +4,27 @@ #include "MapTileStoreSD.h" #ifdef ARDUINO +#include +#include #include +#include namespace Hardware { namespace TDeck { +namespace { +TileStoreResult statMountedPathLocked(const char* name, struct stat& info) { + char mounted[MapTileStore::PATH_CAPACITY + 4U] = {}; + const int written = std::snprintf(mounted, sizeof(mounted), "/sd%s", name); + if ((written < 0) || (static_cast(written) >= sizeof(mounted))) { + return TileStoreResult::INVALID_ARGUMENT; + } + errno = 0; + if (::stat(mounted, &info) == 0) return TileStoreResult::OK; + return (errno == ENOENT) ? TileStoreResult::MISS : TileStoreResult::IO_ERROR; +} +} + MapTileStoreSD::MapTileStoreSD() : stream_(), list_root_(), list_zoom_(), list_x_(), writing_(false), healthy_(true) {} MapTileStoreSD::~MapTileStoreSD() { abortWrite(); endRead(); endList(); } @@ -49,7 +65,9 @@ bool MapTileStoreSD::makeParentDirectoriesLocked(const char* name) { TileStoreResult MapTileStoreSD::beginRead(const char* name, std::uint32_t& size) { if (!healthy_ || !SDAccess::is_ready() || !SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } - if (!SD.exists(name)) { SDAccess::release_bus(); return TileStoreResult::MISS; } + struct stat info = {}; + const TileStoreResult present = statMountedPathLocked(name, info); + if (present != TileStoreResult::OK) { SDAccess::release_bus(); return present; } stream_ = SD.open(name, FILE_READ); if (!stream_) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } const std::size_t file_size = stream_.size(); @@ -124,17 +142,25 @@ void MapTileStoreSD::abortWrite() { TileStoreResult MapTileStoreSD::remove(const char* name) { if (!healthy_ || !SDAccess::is_ready() || !SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } - const bool existed = SD.exists(name); - const bool removed = !existed || SD.remove(name); + struct stat info = {}; + const TileStoreResult present = statMountedPathLocked(name, info); + if (present != TileStoreResult::OK) { SDAccess::release_bus(); return present; } + const bool removed = SD.remove(name); SDAccess::release_bus(); - return !existed ? TileStoreResult::MISS : (removed ? TileStoreResult::OK : TileStoreResult::IO_ERROR); + return removed ? TileStoreResult::OK : TileStoreResult::IO_ERROR; } TileStoreResult MapTileStoreSD::rename(const char* from, const char* to) { if (!healthy_ || !SDAccess::is_ready() || !SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } - if (!SD.exists(from)) { SDAccess::release_bus(); return TileStoreResult::MISS; } - if (SD.exists(to) && !SD.remove(to)) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } + struct stat source_info = {}, destination_info = {}; + const TileStoreResult source = statMountedPathLocked(from, source_info); + if (source != TileStoreResult::OK) { SDAccess::release_bus(); return source; } + const TileStoreResult destination = statMountedPathLocked(to, destination_info); + if ((destination != TileStoreResult::MISS) && (destination != TileStoreResult::OK)) { + SDAccess::release_bus(); return destination; + } + if ((destination == TileStoreResult::OK) && !SD.remove(to)) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } const bool renamed = SD.rename(from, to); SDAccess::release_bus(); return renamed ? TileStoreResult::OK : TileStoreResult::IO_ERROR; @@ -143,13 +169,12 @@ TileStoreResult MapTileStoreSD::rename(const char* from, const char* to) { TileStoreResult MapTileStoreSD::stat(const char* name, std::uint32_t& size) { if (!healthy_ || !SDAccess::is_ready() || !SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } - fs::File file = SD.open(name, FILE_READ); - if (!file) { SDAccess::release_bus(); return TileStoreResult::MISS; } - const std::size_t file_size = file.size(); - file.close(); + struct stat info = {}; + const TileStoreResult present = statMountedPathLocked(name, info); SDAccess::release_bus(); - if (file_size > UINT32_MAX) return TileStoreResult::TOO_LARGE; - size = static_cast(file_size); + if (present != TileStoreResult::OK) return present; + if ((info.st_size < 0) || (static_cast(info.st_size) > UINT32_MAX)) return TileStoreResult::TOO_LARGE; + size = static_cast(info.st_size); return TileStoreResult::OK; } @@ -157,7 +182,14 @@ TileStoreResult MapTileStoreSD::beginList() { endList(); if (!healthy_ || !SDAccess::is_ready() || !SDAccess::acquire_bus(500U)) return TileStoreResult::STORAGE_UNAVAILABLE; if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } + struct stat info = {}; + const TileStoreResult present = statMountedPathLocked("/pyxis-map/tiles", info); + if (present == TileStoreResult::MISS) { SDAccess::release_bus(); return TileStoreResult::OK; } + if ((present != TileStoreResult::OK) || !S_ISDIR(info.st_mode)) { + SDAccess::release_bus(); return TileStoreResult::IO_ERROR; + } list_root_ = SD.open("/pyxis-map/tiles", FILE_READ); + if (!list_root_) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } SDAccess::release_bus(); return TileStoreResult::OK; // An absent cache directory is an empty cache. } @@ -170,28 +202,34 @@ TileStoreResult MapTileStoreSD::nextList(char* name, std::size_t capacity, bool& if (!cardPresentLocked()) { SDAccess::release_bus(); return TileStoreResult::STORAGE_UNAVAILABLE; } while (true) { if (list_x_) { + errno = 0; fs::File item = list_x_.openNextFile(); if (item) { const TileStoreResult copied = copyName(item.path(), name, capacity); item.close(); SDAccess::release_bus(); return copied; } + if (errno != 0) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } list_x_.close(); } if (list_zoom_) { + errno = 0; fs::File item = list_zoom_.openNextFile(); if (item) { if (item.isDirectory()) { list_x_ = item; continue; } const TileStoreResult copied = copyName(item.path(), name, capacity); item.close(); SDAccess::release_bus(); return copied; } + if (errno != 0) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } list_zoom_.close(); } + errno = 0; fs::File item = list_root_.openNextFile(); if (item) { if (item.isDirectory()) { list_zoom_ = item; continue; } const TileStoreResult copied = copyName(item.path(), name, capacity); item.close(); SDAccess::release_bus(); return copied; } + if (errno != 0) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; } done = true; SDAccess::release_bus(); return TileStoreResult::OK; diff --git a/tests/build_scripts/test_map_tile_store_contract.py b/tests/build_scripts/test_map_tile_store_contract.py index a5620195..90c5c25e 100644 --- a/tests/build_scripts/test_map_tile_store_contract.py +++ b/tests/build_scripts/test_map_tile_store_contract.py @@ -55,5 +55,14 @@ def test_sd_adapter_distinguishes_missing_files_from_open_failures(): source = SD_CPP.read_text() body = source[source.index("TileStoreResult MapTileStoreSD::beginRead"): source.index("TileStoreResult MapTileStoreSD::readChunk")] - assert body.index("SD.exists(name)") < body.index("SD.open(name, FILE_READ)") + assert "statMountedPathLocked(name, info)" in body + assert body.index("statMountedPathLocked(name, info)") < body.index("SD.open(name, FILE_READ)") assert "if (!stream_) { SDAccess::release_bus(); return TileStoreResult::IO_ERROR; }" in body + stat_body = source[source.index("TileStoreResult MapTileStoreSD::stat"): + source.index("TileStoreResult MapTileStoreSD::beginList")] + assert "SD.open" not in stat_body + assert "statMountedPathLocked(name, info)" in stat_body + list_body = source[source.index("TileStoreResult MapTileStoreSD::beginList"): + source.index("TileStoreResult MapTileStoreSD::nextList")] + assert "present == TileStoreResult::MISS" in list_body + assert "if (!list_root_)" in list_body and "TileStoreResult::IO_ERROR" in list_body diff --git a/tests/native/test_map_tile_store.cpp b/tests/native/test_map_tile_store.cpp index c522aabf..1aac9cdc 100644 --- a/tests/native/test_map_tile_store.cpp +++ b/tests/native/test_map_tile_store.cpp @@ -266,8 +266,18 @@ void testPostCommitCleanupResidueKeepsWholeNewGeneration() { beginTest(); FakeSt std::uint32_t size=0U; CHECK(s.beginGet(a,size)==TileStoreResult::MISS); CHECK(fs.find("/pyxis-map/tiles/1/0/0.png.evict")<0); } +void testHardMaxLivePlusCrashTempRecovers() { beginTest(); FakeStorage fs; + for(std::uint32_t i=0U;i<128U;++i) { const std::string path="/pyxis-map/tiles/8/"+std::to_string(i)+"/0.png"; fs.add(path.c_str(),png()); } + fs.add("/pyxis-map/tiles/8/128/0.png.tmp",png()); + MapTileStore s(fs,config(128U,5120U,80U)); CHECK(s.initialize()==TileStoreResult::OK); CHECK(s.entryCount()==128U); CHECK(fs.find("/pyxis-map/tiles/8/128/0.png.tmp")<0); +} +void testUnboundedCommittedEvictionResidueCleansInBatches() { beginTest(); FakeStorage fs; + for(std::uint32_t i=0U;i<129U;++i) { const std::string path="/pyxis-map/tiles/8/"+std::to_string(i)+"/0.png.evict"; fs.add(path.c_str(),png()); } + MapTileStore s(fs,config()); CHECK(s.initialize()==TileStoreResult::OK); CHECK(s.entryCount()==0U); + for(std::uint32_t i=0U;i<129U;++i) { const std::string path="/pyxis-map/tiles/8/"+std::to_string(i)+"/0.png.evict"; CHECK(fs.find(path.c_str())<0); } +} void testDeterministicStress() { beginTest(); FakeStorage fs; MapTileStore s(fs,config(3U,120U,80U)); CHECK(s.initialize()==TileStoreResult::OK); CHECK(put(s,TileKey{2U,0U,0U},png())==TileStoreResult::OK); std::uint32_t size=0U; for(std::uint32_t i=0U;i<100000U;++i) { const TileKey k={2U,i&3U,(i>>2)&3U}; TileStoreResult r=s.beginGet(k,size); CHECK(r==TileStoreResult::OK||r==TileStoreResult::MISS); if(r==TileStoreResult::OK)s.endGet(); } } } -int main() { testKeyAndCanonicalPath(); testMissHitAndRemoval(); testMalformedPngs(); testShortWriteAbortsTemp(); testExactQuotaAndLruEviction(); testDuplicateAtomicReplacement(); testInterruptedFilesRecover(); testLiveWinsRecovery(); testCorruptLiveRecoversValidBackup(); testCorruptLiveWithoutBackupIsRemoved(); testStaleTempRemovalFailureAbortsPut(); testRecoveryRejectsMalformedAndExhaustion(); testRecoveryQuotaFailsClosed(); testRenameFailureRestoresDuplicate(); testDuplicateRollbackFailureInvalidatesStore(); testPromotionFailureDoesNotEvictVictims(); testEvictionPreflightFailurePreservesAllVictims(); testEvictionStageFailureRollsBackAllVictims(); testEvictionPowerCutsRestoreWholeOldGeneration(); testDuplicateEvictionPowerCutsRestoreOldCandidateAndVictim(); testStaleDuplicateBackupMustClearBeforeManifest(); testSemanticManifestValidationPrecedesMutation(); testMalformedEvictionManifestFailsClosed(); testPostCommitCleanupResidueKeepsWholeNewGeneration(); testDeterministicStress(); std::cout<<"map tile store: "< None: env["UBSAN_OPTIONS"] = "halt_on_error=1:print_stacktrace=1" ran = subprocess.run([str(binary)], capture_output=True, text=True, timeout=60, env=env) assert ran.returncode == 0, ran.stdout + ran.stderr - assert ran.stdout == "map tile store: 25 tests passed\n" + assert ran.stdout == "map tile store: 27 tests passed\n"