fix: serialize SdFat FsFile close through HalStorage mutex (#2135)

SdFat's SdSpiCard tracks SPI bus state with an unsynchronized
m_spiActive bool. When two tasks call into SdFat concurrently they can
confuse that state machine, ending with one task calling
SPIClass::endTransaction() against a paramLock the other task holds.
That trips FreeRTOS's xTaskPriorityDisinherit assert (tasks.c:5156,
pxTCB == pxCurrentTCBs[0]) and panics the system.

HalStorage already serialized every explicit method call via
storageMutex, but HalFile's destructor was `= default`, which let the
underlying SdFat FsFile destructor run close() outside any lock
(DESTRUCTOR_CLOSES_FILE=1). Any task that destructed a HalFile while
another task was mid-SD-op would race the unsynchronized state.

Move the locking discipline into HalFile::Impl::~Impl: an explicit
close() under StorageLock, then the FsFile member destructor's redundant
close() is a no-op. HalFile's special members can stay = default.

Switch storageMutex to xSemaphoreCreateRecursiveMutex so openFileForRead
and openFileForWrite can hold the lock while assigning to a HalFile&
out-param whose prior Impl needs locked teardown. Priority inheritance
still applies to recursive mutexes.

Also documented the no-bypass rule in CLAUDE.md: never call SdFat /
SdSpiCard / FsBaseFile / SDCardManager directly, never define
HAL_STORAGE_IMPL outside HalStorage.cpp.

Addresses my admittedly synthetic repro for #2047

Did you use AI tools to help write this code? partial

lib/hal/HalStorage.cpp

@@ -12,7 +12,13 @@
 HalStorage HalStorage::instance;
 
 HalStorage::HalStorage() {
-  storageMutex = xSemaphoreCreateMutex();
+  // Recursive so the same task can re-enter StorageLock without self-deadlock.
+  // openFileForRead/Write take the lock and then assign to a HalFile&
+  // out-param; if that out-param already held an Impl, its destructor takes
+  // the lock again to close the prior FsFile under serialization (see
+  // HalFile::Impl::~Impl below). Priority inheritance still applies to
+  // recursive mutexes.
+  storageMutex = xSemaphoreCreateRecursiveMutex();
   assert(storageMutex != nullptr);
 }
 
@@ -26,8 +32,8 @@ bool HalStorage::ready() const { return SDCard.ready(); }
 
 class HalStorage::StorageLock {
  public:
-  StorageLock() { xSemaphoreTake(HalStorage::getInstance().storageMutex, portMAX_DELAY); }
-  ~StorageLock() { xSemaphoreGive(HalStorage::getInstance().storageMutex); }
+  StorageLock() { xSemaphoreTakeRecursive(HalStorage::getInstance().storageMutex, portMAX_DELAY); }
+  ~StorageLock() { xSemaphoreGiveRecursive(HalStorage::getInstance().storageMutex); }
 };
 
 #define HAL_STORAGE_WRAPPED_CALL(method, ...) \
@@ -57,17 +63,23 @@ bool HalStorage::ensureDirectoryExists(const char* path) { HAL_STORAGE_WRAPPED_C
 class HalFile::Impl {
  public:
   Impl(FsFile&& fsFile) : file(std::move(fsFile)) {}
+  // SdFat is not thread-safe; FsFile::close() touches SD/SPI and must run
+  // under StorageLock or it races SdSpiCard::m_spiActive across tasks and
+  // trips FreeRTOS's xTaskPriorityDisinherit assert. The FsFile member
+  // destructor (DESTRUCTOR_CLOSES_FILE=1) will close() again after the lock
+  // releases, but close() on an already-closed FsFile is a no-op. See SdFat
+  // issue #518 and the HAL note in CLAUDE.md.
+  ~Impl() {
+    HalStorage::StorageLock lock;
+    file.close();
+  }
   FsFile file;
 };
 
 HalFile::HalFile() = default;
-
 HalFile::HalFile(std::unique_ptr<Impl> impl) : impl(std::move(impl)) {}
-
 HalFile::~HalFile() = default;
-
 HalFile::HalFile(HalFile&&) = default;
-
 HalFile& HalFile::operator=(HalFile&&) = default;
 
 HalFile HalStorage::open(const char* path, const oflag_t oflag) {