File locking refactored (#631)

Remove FileMutex and wire locking into driver subsystems.

Modules/app-module/private/app/private/fs.h

@@ -1,12 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 #pragma once
 
-// Minimal filesystem helpers shared by app-module internals that need to look at on-disk app
-// directories (app_install.cpp, manager.cpp's install-path scan) - app-module may not depend
-// upward on Tactility::file, so this is a small local re-implementation (see
-// app_metadata_parsing.cpp for the same constraint applied to properties-file loading).
-
-#include <tactility/filesystem/file_mutex.h>
+// Minimal filesystem helpers shared by app-module internals.
 
 #include <cstring>
 #include <dirent.h>
@@ -21,22 +16,12 @@
 
 inline bool app_fs_is_directory(const std::string& path) {
     struct stat result {};
-    FileMutex file_mutex;
-    file_mutex_get(&file_mutex, path.c_str());
-    file_mutex_lock(&file_mutex);
-    auto is_dir = stat(path.c_str(), &result) == 0 && S_ISDIR(result.st_mode);
-    file_mutex_unlock(&file_mutex);
-    return is_dir;
+    return stat(path.c_str(), &result) == 0 && S_ISDIR(result.st_mode);
 }
 
 inline bool app_fs_is_file(const std::string& path) {
-    FileMutex file_mutex;
-    file_mutex_get(&file_mutex, path.c_str());
-    file_mutex_lock(&file_mutex);
     struct stat result {};
-    auto retval = stat(path.c_str(), &result) == 0 && S_ISREG(result.st_mode);
-    file_mutex_unlock(&file_mutex);
-    return retval;
+    return stat(path.c_str(), &result) == 0 && S_ISREG(result.st_mode);
 }
 
 // Appends the full path of every direct subdirectory of @a path to @a out.
@@ -53,15 +38,11 @@ inline bool app_fs_delete_recursively(const std::string& path) {
     // ESP-IDF newlib has no lstat(); ESP32 filesystems (FAT/SPIFFS) don't
     // support symlinks, so stat() is equivalent there.
     struct stat st {};
-    FileMutex file_mutex;
-    file_mutex_get(&file_mutex, path.c_str());
-    file_mutex_lock(&file_mutex);
 #ifdef ESP_PLATFORM
     int rc = stat(path.c_str(), &st);
 #else
     int rc = lstat(path.c_str(), &st);
 #endif
-    file_mutex_unlock(&file_mutex);
 
     if (rc != 0) {
         return false;
@@ -70,24 +51,15 @@ inline bool app_fs_delete_recursively(const std::string& path) {
 #ifndef ESP_PLATFORM
     if (S_ISLNK(st.st_mode)) {
         // Symlink — remove as a leaf regardless of its target.
-        file_mutex_lock(&file_mutex);
-        bool result = unlink(path.c_str()) == 0;
-        file_mutex_unlock(&file_mutex);
-        return result;
+        return unlink(path.c_str()) == 0;
     }
 #endif
 
     if (S_ISDIR(st.st_mode)) {
-        // Collect child names while locked, then release before recursing —
-        // child paths can resolve to the same mount mutex (see
-        // app_fs_list_direct_subdirectories comment), so holding the parent
-        // lock across the recursive call would self-deadlock.
         std::vector<std::string> children;
 
-        file_mutex_lock(&file_mutex);
         DIR* dir = opendir(path.c_str());
         if (dir == nullptr) {
-            file_mutex_unlock(&file_mutex);
             return false;
         }
 
@@ -99,7 +71,6 @@ inline bool app_fs_delete_recursively(const std::string& path) {
             children.push_back(path + "/" + entry->d_name);
         }
         closedir(dir);
-        file_mutex_unlock(&file_mutex);
 
         bool success = true;
         for (const auto& child : children) {
@@ -109,33 +80,18 @@ inline bool app_fs_delete_recursively(const std::string& path) {
             }
         }
 
-        file_mutex_lock(&file_mutex);
-        bool result = rmdir(path.c_str()) == 0;
-        file_mutex_unlock(&file_mutex);
-        return result;
+        return rmdir(path.c_str()) == 0;
     }
 
     // Regular file or other — unlink.
-    file_mutex_lock(&file_mutex);
-    bool result = unlink(path.c_str()) == 0;
-    file_mutex_unlock(&file_mutex);
-    return result;
+    return unlink(path.c_str()) == 0;
 }
 
 inline void app_fs_list_direct_subdirectories(const std::string& path, std::vector<std::string>& out) {
-    // Collect child names while the directory lock is held, then release it before classifying
-    // each one with app_fs_is_directory() - that function looks up and locks a FileMutex too,
-    // and file_mutex_get() resolves a child path to the same registered mutex as its parent
-    // mount. Calling it while still holding the directory's own lock would be a nested
-    // acquisition of that same (possibly non-recursive) mutex, and could self-deadlock.
     std::vector<std::string> children;
 
-    FileMutex file_mutex;
-    file_mutex_get(&file_mutex, path.c_str());
-    file_mutex_lock(&file_mutex);
     DIR* dir = opendir(path.c_str());
     if (dir == nullptr) {
-        file_mutex_unlock(&file_mutex);
         return;
     }
 
@@ -148,7 +104,6 @@ inline void app_fs_list_direct_subdirectories(const std::string& path, std::vect
     }
 
     closedir(dir);
-    file_mutex_unlock(&file_mutex);
 
     for (const auto& child_path : children) {
         if (app_fs_is_directory(child_path)) {

Modules/app-module/source/app_install.cpp

@@ -8,7 +8,6 @@
 #include <app/private/ledger.h>
 
 #include <tactility/concurrent/mutex.h>
-#include <tactility/filesystem/file_mutex.h>
 #include <tactility/log.h>
 #include <tactility/paths.h>
 
@@ -44,11 +43,7 @@ bool ensure_directory(const std::string& path) {
         return true;
     }
 
-    FileMutex mutex {};
-    file_mutex_get(&mutex, path.c_str());
-    file_mutex_lock(&mutex);
     bool created = mkdir(path.c_str(), 0777) == 0 || errno == EEXIST;
-    file_mutex_unlock(&mutex);
     if (!created) {
         return false;
     }
@@ -140,6 +135,66 @@ bool untar(const std::string& tar_path, const std::string& destination_path) {
 
 // endregion
 
+// region Staging-path lock: at most one caller may clean up/populate a given staging_path at a
+// time, keyed by source basename. The HTTP server and app tasks can call app_install()
+// concurrently, e.g. two uploads sharing a source basename - without this, one call's cleanup
+// can delete or overwrite the staging directory another call is still extracting into.
+
+struct StagingLock {
+    Mutex mutex {};
+    int refcount = 0;
+};
+
+struct StagingLockTable {
+    std::unordered_map<std::string, std::unique_ptr<StagingLock>> locks;
+    Mutex table_mutex {};
+
+    StagingLockTable() { mutex_construct(&table_mutex); }
+};
+
+StagingLockTable& staging_lock_table() {
+    static StagingLockTable table;
+    return table;
+}
+
+// Blocks until any other caller staging @a path has released it, then locks it for this caller.
+// Must be paired with exactly one release_staging_lock(path) call.
+void acquire_staging_lock(const std::string& path) {
+    auto& table = staging_lock_table();
+    mutex_lock(&table.table_mutex);
+    auto iterator = table.locks.find(path);
+    if (iterator == table.locks.end()) {
+        auto lock = std::make_unique<StagingLock>();
+        mutex_construct(&lock->mutex);
+        iterator = table.locks.emplace(path, std::move(lock)).first;
+    }
+    StagingLock* lock = iterator->second.get();
+    lock->refcount++;
+    mutex_unlock(&table.table_mutex);
+
+    mutex_lock(&lock->mutex);
+}
+
+// Erases the table entry once nothing references it anymore, so the table doesn't grow forever
+// across installs with distinct basenames (e.g. unique upload temp names).
+void release_staging_lock(const std::string& path) {
+    auto& table = staging_lock_table();
+    mutex_lock(&table.table_mutex);
+    auto iterator = table.locks.find(path);
+    if (iterator == table.locks.end()) {
+        mutex_unlock(&table.table_mutex);
+        return;
+    }
+    StagingLock* lock = iterator->second.get();
+    mutex_unlock(&lock->mutex);
+    if (--lock->refcount == 0) {
+        table.locks.erase(iterator);
+    }
+    mutex_unlock(&table.table_mutex);
+}
+
+// endregion
+
 // region Installed-app registry: owns the AppManifest (and its id/name/path strings) that
 // app_manager's ledger only keeps a non-owning pointer to (see app_manager_add()'s contract).
 
@@ -282,22 +337,14 @@ error_t app_install(const char* source_path) {
     }
 
     auto staging_path = app_parent_path + "/" + last_path_segment(source_path);
+    acquire_staging_lock(staging_path);
+
     delete_recursively(staging_path);
 
-    FileMutex target_mutex {};
-    file_mutex_get(&target_mutex, app_parent_path.c_str());
-    FileMutex source_mutex {};
-    file_mutex_get(&source_mutex, source_path);
-
-    file_mutex_lock(&target_mutex);
-    file_mutex_lock(&source_mutex);
-    bool untar_success = untar(source_path, staging_path);
-    file_mutex_unlock(&source_mutex);
-    file_mutex_unlock(&target_mutex);
-
-    if (!untar_success) {
+    if (!untar(source_path, staging_path)) {
         LOG_E(TAG, "Failed to extract %s", source_path);
         delete_recursively(staging_path);
+        release_staging_lock(staging_path);
         return ERROR_NOT_FOUND;
     }
 
@@ -305,6 +352,7 @@ error_t app_install(const char* source_path) {
     if (!app_fs_is_file(manifest_path)) {
         LOG_E(TAG, "Manifest not found at %s", manifest_path.c_str());
         delete_recursively(staging_path);
+        release_staging_lock(staging_path);
         return ERROR_INVALID_ARGUMENT;
     }
 
@@ -312,6 +360,7 @@ error_t app_install(const char* source_path) {
     if (app_metadata_parse(manifest_path.c_str(), &metadata) != ERROR_NONE) {
         LOG_E(TAG, "Install failed: invalid manifest");
         delete_recursively(staging_path);
+        release_staging_lock(staging_path);
         return ERROR_INVALID_ARGUMENT;
     }
 
@@ -331,22 +380,21 @@ error_t app_install(const char* source_path) {
         LOG_E(TAG, "Install failed: failed to remove existing installation");
         mutex_unlock(&registry.mutex);
         delete_recursively(staging_path);
+        release_staging_lock(staging_path);
         return ERROR_RESOURCE;
     }
 
     auto final_path = app_parent_path + "/" + metadata.app_id;
     delete_recursively(final_path);
 
-    file_mutex_lock(&target_mutex);
-    bool rename_success = rename(staging_path.c_str(), final_path.c_str()) == 0;
-    file_mutex_unlock(&target_mutex);
-
-    if (!rename_success) {
+    if (rename(staging_path.c_str(), final_path.c_str()) != 0) {
         LOG_E(TAG, "Failed to rename \"%s\" to \"%s\"", staging_path.c_str(), final_path.c_str());
         delete_recursively(staging_path);
+        release_staging_lock(staging_path);
         mutex_unlock(&registry.mutex);
         return ERROR_NOT_FOUND;
     }
+    release_staging_lock(staging_path);
 
     // Only remaining failure mode is a duplicate id - can't happen, uninstall_locked() above
     // already removed any previous registration for this exact id.

Modules/app-module/source/app_metadata_parsing.cpp

@@ -1,6 +1,4 @@
 // SPDX-License-Identifier: Apache-2.0
-#include <tactility/filesystem/file_mutex.h>
-
 #include <app/metadata.h>
 #include <app/private/metadata_parsing_internal.h>
 
@@ -62,13 +60,8 @@ bool validate_csv_list(const std::string& value, bool (*is_valid_item)(const std
  * minimal re-implementation rather than depending on Tactility's file::loadPropertiesFile() -
  * app-module (like every other kernel module) may not depend upward on the Tactility layer. */
 bool load_properties(const std::string& path, std::map<std::string, std::string>& out_properties, std::string& out_first_line) {
-    FileMutex mutex;
-    file_mutex_get(&mutex, path.c_str());
-    file_mutex_lock(&mutex);
-
     std::ifstream file(path);
     if (!file.is_open()) {
-        file_mutex_unlock(&mutex);
         return false;
     }
 
@@ -103,7 +96,6 @@ bool load_properties(const std::string& path, std::map<std::string, std::string>
         out_properties[key] = value;
     }
 
-    file_mutex_unlock(&mutex);
     return true;
 }