fix: prevent progress.bin corruption from interrupted writes (#2275) (#2305)

## 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) <noreply@anthropic.com>

src/activities/reader/EpubReaderUtils.h

@@ -1,23 +1,19 @@
 #pragma once
 
 #include <Epub.h>
-#include <HalStorage.h>
 #include <Logging.h>
 
+#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);

src/activities/reader/ProgressFile.h

@@ -0,0 +1,64 @@
+#pragma once
+
+#include <HalStorage.h>
+#include <Logging.h>
+
+#include <cstddef>
+#include <cstdint>
+#include <string>
+
+namespace ProgressFile {
+
+// Writes `len` bytes of reader progress to `<cachePath>/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

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

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