From 5e990b3991fd033db8411cfd279eadd601c354a8 Mon Sep 17 00:00:00 2001 From: Pedro Cardoso Date: Thu, 18 Jun 2026 19:01:56 -0300 Subject: [PATCH] fix: prevent progress.bin corruption from interrupted writes (#2275) (#2305) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary * **What is the goal of this PR?** Fixes #2275. A book could get stuck reopening on an old page, with progress no longer saving and neither "Delete Book Cache" nor "Clear Reading Cache" able to fix it. Root cause: `progress.bin` was written truncate-in-place, so an interrupted write (power loss, or a crash mid-SPI during sleep) left it with a broken FAT cluster chain that the firmware could neither rewrite nor delete — recovery required `fsck`/manual deletion on a host PC. Confirmed in the SDK: `SDCardManager::openFileForWrite` opens with `O_RDWR | O_CREAT | O_TRUNC`, so the canonical file is zeroed before the few progress bytes are rewritten — exactly the window that corrupts the FAT chain. * **What changes are included?** * New shared helper `ProgressFile::writeAtomic()` (`src/activities/reader/ProgressFile.h`): writes progress to `progress.bin.tmp`, flushes and closes it, then `remove`s the old `progress.bin` and `rename`s the temp into place. An interrupted write now only ever damages the throwaway temp; the canonical file is never torn. * All three readers route their progress saves through the helper: EPUB (`EpubReaderUtils.h`), `TxtReaderActivity`, `XtcReaderActivity` — they all shared the identical vulnerable pattern. * Minor: `EpubReaderUtils::saveProgress` now takes `const Epub&` (clears a cppcheck `constParameterReference` finding). ## Additional Context * **Crash-safe, not metadata-atomic.** On FAT the replace is `remove` + `rename` (two directory ops; SdFat's `rename` won't overwrite, hence remove-first). A crash between them leaves *neither* file, which reads as "no saved progress" on next launch — a harmless reset to an old page, never a corrupt/unclearable file. The guarantee is that `progress.bin` is never half-written. * **Prevents, does not repair.** This stops new corruption on healthy cards. It cannot fix an already-corrupted `progress.bin` (removing it may itself fail at the FAT level) — those still need `fsck`/manual deletion, as in the issue's workaround. * **Known follow-up (out of scope here):** a crash *while writing the temp* can leave an orphan `progress.bin.tmp`. It's harmless and self-healing (the next save overwrites it, and it never blocks reading progress), but a boot-time orphan-`.tmp` cleanup would be a tidy follow-up. * **Focus areas for review:** the close-before-rename ordering in `ProgressFile.h` and the remove-before-rename rationale. ## Verification * `./bin/clang-format-fix` — clean * `pio check --fail-on-defect low --fail-on-defect medium --fail-on-defect high` — no defects * `pio run` — SUCCESS (RAM 30.9%, Flash 78.8%; footprint essentially unchanged) * Tested on a **Xteink X4** device: open book, turn pages, sleep/exit, reopen — progress now restores to the navigated page across all three readers (EPUB / TXT / XTC). --- ### AI Usage Did you use AI tools to help write this code? _**YES**_ --------- Co-authored-by: Claude Opus 4.8 (1M context) --- src/activities/reader/EpubReaderUtils.h | 14 ++--- src/activities/reader/ProgressFile.h | 64 +++++++++++++++++++++ src/activities/reader/TxtReaderActivity.cpp | 16 +++--- src/activities/reader/XtcReaderActivity.cpp | 17 +++--- 4 files changed, 84 insertions(+), 27 deletions(-) create mode 100644 src/activities/reader/ProgressFile.h diff --git a/src/activities/reader/EpubReaderUtils.h b/src/activities/reader/EpubReaderUtils.h index 7dc0fb63..e9ac05b8 100644 --- a/src/activities/reader/EpubReaderUtils.h +++ b/src/activities/reader/EpubReaderUtils.h @@ -1,23 +1,19 @@ #pragma once #include -#include #include +#include "ProgressFile.h" + namespace EpubReaderUtils { // Persists reader progress for an EPUB to its cache directory. Returns true on success. -inline bool saveProgress(Epub& epub, int spineIndex, int pageNumber, int pageCount) { +inline bool saveProgress(const Epub& epub, int spineIndex, int pageNumber, int pageCount) { if (spineIndex < 0 || spineIndex > 0xFFFF || pageNumber < 0 || pageNumber > 0xFFFF || pageCount < 0 || pageCount > 0xFFFF) { LOG_ERR("ERS", "Progress values out of range: spine=%d page=%d count=%d", spineIndex, pageNumber, pageCount); return false; } - HalFile f; - if (!Storage.openFileForWrite("ERS", epub.getCachePath() + "/progress.bin", f)) { - LOG_ERR("ERS", "Could not open progress file for write!"); - return false; - } uint8_t data[6]; data[0] = spineIndex & 0xFF; data[1] = (spineIndex >> 8) & 0xFF; @@ -25,9 +21,7 @@ inline bool saveProgress(Epub& epub, int spineIndex, int pageNumber, int pageCou data[3] = (pageNumber >> 8) & 0xFF; data[4] = pageCount & 0xFF; data[5] = (pageCount >> 8) & 0xFF; - const size_t written = f.write(data, sizeof(data)); - if (written != sizeof(data)) { - LOG_ERR("ERS", "Short write saving progress: %u/%u bytes", (unsigned)written, (unsigned)sizeof(data)); + if (!ProgressFile::writeAtomic(epub.getCachePath(), data, sizeof(data))) { return false; } LOG_DBG("ERS", "Progress saved: spine=%d page=%d", spineIndex, pageNumber); diff --git a/src/activities/reader/ProgressFile.h b/src/activities/reader/ProgressFile.h new file mode 100644 index 00000000..5238a157 --- /dev/null +++ b/src/activities/reader/ProgressFile.h @@ -0,0 +1,64 @@ +#pragma once + +#include +#include + +#include +#include +#include + +namespace ProgressFile { + +// Writes `len` bytes of reader progress to `/progress.bin` without +// ever leaving the canonical file half-written. +// +// The bytes go to a temporary `progress.bin.tmp` first; only once that is fully +// written and closed is it renamed over progress.bin. An interrupted write +// (power loss or a crash mid-SPI) therefore damages only the throwaway temp file. +// Previously a truncate-in-place write that was cut short left progress.bin with +// a broken FAT cluster chain that the firmware could neither rewrite nor clear, +// stranding the book on an old page (issue #2275). +// +// This is crash-safe, not metadata-atomic: on FAT the replace is remove + rename, +// two separate directory operations, so a crash between them can leave neither +// file -- which simply reads as "no saved progress" on next launch, never a +// corrupt or unclearable file. The point is that progress.bin is never torn. +// +// Note: this prevents corruption on a healthy card going forward. It cannot +// repair an already-corrupted progress.bin -- removing the stale file may itself +// fail at the FAT level, in which case recovery still requires fsck on a host. +// +// Returns true only if the new progress.bin is fully in place. +inline bool writeAtomic(const std::string& cachePath, const uint8_t* data, size_t len) { + const std::string finalPath = cachePath + "/progress.bin"; + const std::string tmpPath = cachePath + "/progress.bin.tmp"; + + { + HalFile f; + if (!Storage.openFileForWrite("PRG", tmpPath, f)) { + LOG_ERR("PRG", "Could not open temp progress file for write: %s", tmpPath.c_str()); + return false; + } + const size_t written = f.write(data, len); + if (written != len) { + LOG_ERR("PRG", "Short write saving progress to %s: %u/%u bytes", tmpPath.c_str(), (unsigned)written, + (unsigned)len); + return false; + } + f.flush(); + // f (the temp file) is closed at scope exit (DESTRUCTOR_CLOSES_FILE=1) before + // the rename below -- SdFat must not rename a path that still has an open FsFile. + } + + // SdFat's rename does not overwrite an existing destination, so drop the old + // canonical file first. The brief window where neither file exists reads as + // "no saved progress" on next launch -- never a corrupt, unclearable file. + Storage.remove(finalPath.c_str()); + if (!Storage.rename(tmpPath.c_str(), finalPath.c_str())) { + LOG_ERR("PRG", "Failed to rename temp progress into place: %s", finalPath.c_str()); + return false; + } + return true; +} + +} // namespace ProgressFile diff --git a/src/activities/reader/TxtReaderActivity.cpp b/src/activities/reader/TxtReaderActivity.cpp index 0f3a7992..0464c614 100644 --- a/src/activities/reader/TxtReaderActivity.cpp +++ b/src/activities/reader/TxtReaderActivity.cpp @@ -11,6 +11,7 @@ #include "CrossPointSettings.h" #include "CrossPointState.h" #include "MappedInputManager.h" +#include "ProgressFile.h" #include "ReaderUtils.h" #include "RecentBooksStore.h" #include "components/UITheme.h" @@ -421,14 +422,13 @@ void TxtReaderActivity::renderStatusBar() const { } void TxtReaderActivity::saveProgress() const { - HalFile f; - if (Storage.openFileForWrite("TRS", txt->getCachePath() + "/progress.bin", f)) { - uint8_t data[4]; - data[0] = currentPage & 0xFF; - data[1] = (currentPage >> 8) & 0xFF; - data[2] = 0; - data[3] = 0; - f.write(data, 4); + uint8_t data[4]; + data[0] = currentPage & 0xFF; + data[1] = (currentPage >> 8) & 0xFF; + data[2] = 0; + data[3] = 0; + if (!ProgressFile::writeAtomic(txt->getCachePath(), data, sizeof(data))) { + LOG_ERR("TRS", "Failed to save progress: page %d", currentPage); } } diff --git a/src/activities/reader/XtcReaderActivity.cpp b/src/activities/reader/XtcReaderActivity.cpp index c78aeea1..c2306aa8 100644 --- a/src/activities/reader/XtcReaderActivity.cpp +++ b/src/activities/reader/XtcReaderActivity.cpp @@ -17,6 +17,7 @@ #include "CrossPointSettings.h" #include "CrossPointState.h" #include "MappedInputManager.h" +#include "ProgressFile.h" #include "ReaderUtils.h" #include "RecentBooksStore.h" #include "XtcReaderChapterSelectionActivity.h" @@ -375,15 +376,13 @@ void XtcReaderActivity::renderPage() { } void XtcReaderActivity::saveProgress() const { - HalFile f; - if (Storage.openFileForWrite("XTR", xtc->getCachePath() + "/progress.bin", f)) { - uint8_t data[4]; - data[0] = currentPage & 0xFF; - data[1] = (currentPage >> 8) & 0xFF; - data[2] = (currentPage >> 16) & 0xFF; - data[3] = (currentPage >> 24) & 0xFF; - f.write(data, 4); - f.close(); + uint8_t data[4]; + data[0] = currentPage & 0xFF; + data[1] = (currentPage >> 8) & 0xFF; + data[2] = (currentPage >> 16) & 0xFF; + data[3] = (currentPage >> 24) & 0xFF; + if (!ProgressFile::writeAtomic(xtc->getCachePath(), data, sizeof(data))) { + LOG_ERR("XTR", "Failed to save progress: page %lu", currentPage); } }