Fixes and improvements (#617)

Modules/app-module/include/app/event.h

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: Apache-2.0
 #pragma once
 
+#include "instance.h"
+
 #include <stddef.h>
 #include <stdint.h>
 
@@ -54,7 +56,7 @@ struct AppEvent {
  */
 struct AppEventSubscription {
     /** The app instance this subscription receives events for; set by the caller before app_event_subscribe(). */
-    uint32_t app_instance_id;
+    AppInstanceId app_instance_id;
 
     TaskHandle_t task;
 
@@ -89,7 +91,7 @@ error_t app_event_unsubscribe(struct AppEventSubscription* sub);
  * @retval ERROR_RESOURCE at least one matching subscription's queue was full; the event was
  * dropped for that subscription (still delivered to any other matching subscription)
  */
-error_t app_event_emit(uint32_t app_instance_id, const struct AppEvent* event);
+error_t app_event_emit(AppInstanceId app_instance_id, const struct AppEvent* event);
 
 /**
  * Pop the next event for @a sub, blocking up to @a timeout if the queue is currently empty.

Modules/app-module/include/app/manager.h

@@ -152,6 +152,14 @@ error_t app_manager_install_path_add(const char* path);
  */
 void app_manager_install_path_scan(void);
 
+/**
+ * Uninstalls an app that was registered via app_manager_install_path_scan() (i.e. discovered on
+ * disk, not installed via app_install()). Stops running instances, removes the manifest
+ * registration, and deletes the app directory. Returns ERROR_NOT_FOUND if the app id is not in
+ * the scan registry.
+ */
+error_t app_manager_install_path_uninstall(const char* app_id);
+
 #ifdef __cplusplus
 }
 #endif

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

@@ -12,6 +12,11 @@
 #include <dirent.h>
 #include <string>
 #include <sys/stat.h>
+#ifdef ESP_PLATFORM
+#include <sys/unistd.h>
+#else
+#include <unistd.h>
+#endif
 #include <vector>
 
 inline bool app_fs_is_directory(const std::string& path) {
@@ -36,6 +41,87 @@ inline bool app_fs_is_file(const std::string& path) {
 
 // Appends the full path of every direct subdirectory of @a path to @a out.
 // No-op (not an error) if @a path can't be opened.
+inline bool app_fs_delete_recursively(const std::string& path) {
+    if (path.empty() || path == "/" || path == "." || path == "..") {
+        return true;
+    }
+
+    // Use lstat() so symbolic links are not followed: a symlink that points at
+    // an external directory must be removed as a leaf entry (unlink), not
+    // recursed into.  app_fs_is_directory() uses stat() and would follow the
+    // link, potentially deleting files outside the target tree.
+    // 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;
+    }
+
+#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;
+    }
+#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;
+        }
+
+        struct dirent* entry;
+        while ((entry = readdir(dir)) != nullptr) {
+            if (std::strcmp(entry->d_name, ".") == 0 || std::strcmp(entry->d_name, "..") == 0) {
+                continue;
+            }
+            children.push_back(path + "/" + entry->d_name);
+        }
+        closedir(dir);
+        file_mutex_unlock(&file_mutex);
+
+        bool success = true;
+        for (const auto& child : children) {
+            success = app_fs_delete_recursively(child);
+            if (!success) {
+                return false;
+            }
+        }
+
+        file_mutex_lock(&file_mutex);
+        bool result = rmdir(path.c_str()) == 0;
+        file_mutex_unlock(&file_mutex);
+        return result;
+    }
+
+    // Regular file or other — unlink.
+    file_mutex_lock(&file_mutex);
+    bool result = unlink(path.c_str()) == 0;
+    file_mutex_unlock(&file_mutex);
+    return result;
+}
+
 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,

Modules/app-module/source/app_install.cpp

@@ -66,62 +66,13 @@ bool ensure_directory_recursive(const std::string& path) {
 }
 
 bool delete_recursively(const std::string& path) {
-    LOG_D(TAG, "Deleting %s...", path.c_str());
-    if (path.empty() || path == "/" || path == "." || path == "..") {
-        return true;
-    }
-
-    if (app_fs_is_directory(path)) {
-        LOG_D(TAG, "Deleting dir %s", path.c_str());
-
-        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) {
-            LOG_E(TAG, "Failed to scan directory %s", path.c_str());
-            file_mutex_unlock(&file_mutex);
-            return false;
-        }
-
-        bool success = true;
-        dirent* entry;
-        while (success && (entry = readdir(dir)) != nullptr) {
-            if (std::strcmp(entry->d_name, ".") == 0 || std::strcmp(entry->d_name, "..") == 0) {
-                continue;
-            }
-            success = delete_recursively(path + "/" + entry->d_name);
-        }
-        closedir(dir);
-
-        if (!success) {
-            file_mutex_unlock(&file_mutex);
-            return false;
-        }
-
-        bool result = rmdir(path.c_str()) == 0;
-        file_mutex_unlock(&file_mutex);
-        return result;
-    }
-
-    if (app_fs_is_file(path)) {
-        LOG_D(TAG, "Deleting file %s", path.c_str());
-        FileMutex mutex {};
-        file_mutex_get(&mutex, path.c_str());
-        file_mutex_lock(&mutex);
-        bool result = remove(path.c_str()) == 0;
-        file_mutex_unlock(&mutex);
-        return result;
-    }
-
-    LOG_D(TAG, "Deleting done");
-    return true;
+    LOG_I(TAG, "Deleting %s...", path.c_str());
+    return app_fs_delete_recursively(path);
 }
 
 bool get_app_install_directory(std::string& out_path) {
     char root[192];
-    if (paths_get_user_data_path(root, sizeof(root)) != ERROR_NONE) {
+    if (paths_get_data_path(root, sizeof(root)) != ERROR_NONE) {
         return false;
     }
     out_path = std::string(root) + "/app";
@@ -277,6 +228,11 @@ error_t uninstall_locked(const std::string& app_id) {
         return ERROR_NOT_FOUND;
     }
 
+    // Can't uninstall in-memory apps
+    if (iterator->second->manifest.location.type != APP_LOCATION_PATH) {
+        return ERROR_NOT_SUPPORTED;
+    }
+
     stop_all_instances_of(&iterator->second->manifest);
     app_manager_remove(app_id.c_str());
     delete_recursively(iterator->second->path);
@@ -404,10 +360,20 @@ error_t app_uninstall(const char* app_id) {
 
     auto& registry = install_registry();
     mutex_lock(&registry.mutex);
-    error_t result = uninstall_locked(app_id);
+    error_t error = uninstall_locked(app_id);
     mutex_unlock(&registry.mutex);
 
-    return result;
+    if (error == ERROR_NOT_FOUND) {
+        error = app_manager_install_path_uninstall(app_id);
+    }
+
+    if (error == ERROR_NONE) {
+        LOG_I(TAG, "Uninstalled %s", app_id);
+    } else {
+        LOG_I(TAG, "Uninstalling %s failed: %s", app_id, error_to_string(error));
+    }
+
+    return error;
 }
 
 } // extern "C"

Modules/app-module/source/app_paths.cpp

@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
+#include <app/location.h>
+#include <app/manager.h>
 #include <app/paths.h>
 #include <tactility/paths.h>
 
@@ -9,7 +11,7 @@ extern "C" {
 
 error_t app_paths_get_user_data_directory(const char* app_id, char* out_path, size_t out_path_size) {
     char root[192];
-    error_t error = paths_get_user_data_path(root, sizeof(root));
+    error_t error = paths_get_data_path(root, sizeof(root));
     if (error != ERROR_NONE) {
         return error;
     }
@@ -34,12 +36,17 @@ error_t app_paths_get_user_data_path(const char* app_id, const char* child_path,
 }
 
 error_t app_paths_get_assets_directory(const char* app_id, char* out_path, size_t out_path_size) {
-    char directory[224];
-    error_t error = app_paths_get_user_data_directory(app_id, directory, sizeof(directory));
+    AppManifest manifest;
+    error_t error = app_manager_find_manifest(app_id, &manifest);
     if (error != ERROR_NONE) {
         return error;
     }
-    int written = std::snprintf(out_path, out_path_size, "%s/assets", directory);
+
+    if (manifest.location.type != APP_LOCATION_PATH) {
+        return ERROR_NOT_FOUND;
+    }
+
+    int written = std::snprintf(out_path, out_path_size, "%s/assets", static_cast<const char*>(manifest.location.location));
     if (written < 0 || (size_t)written >= out_path_size) {
         return ERROR_BUFFER_OVERFLOW;
     }

Modules/app-module/source/event.cpp

@@ -50,7 +50,7 @@ error_t app_event_unsubscribe(AppEventSubscription* sub) {
     return result;
 }
 
-error_t app_event_emit(uint32_t app_instance_id, const AppEvent* event) {
+error_t app_event_emit(AppInstanceId app_instance_id, const AppEvent* event) {
     AppEvent stamped_event = *event;
     stamped_event.timestamp = get_micros_since_boot();
 

Modules/app-module/source/manager.cpp

@@ -1,13 +1,12 @@
 // SPDX-License-Identifier: Apache-2.0
 #include <app/manager.h>
-
 #include <app/metadata.h>
-
 #include <app/private/app_fs.h>
 #include <app/private/app_ledger.h>
 #include <app/private/app_scheduler.h>
 
 #include <tactility/concurrent/mutex.h>
+#include <tactility/error.h>
 #include <tactility/log.h>
 
 #include <algorithm>
@@ -340,4 +339,55 @@ void app_manager_install_path_scan(void) {
     mutex_unlock(&registry.mutex);
 }
 
+error_t app_manager_install_path_uninstall(const char* app_id) {
+    auto& registry = install_path_registry();
+
+    mutex_lock(&registry.mutex);
+    auto iterator = registry.scanned.find(app_id);
+    if (iterator == registry.scanned.end()) {
+        mutex_unlock(&registry.mutex);
+        return ERROR_NOT_FOUND;
+    }
+
+    const AppManifest* manifest = &iterator->second->manifest;
+    auto path = iterator->second->path;
+    mutex_unlock(&registry.mutex);
+
+    // Stop every running instance that retains this manifest pointer, mirroring
+    // stop_all_instances_of() in app_install.cpp.  Collect under ledger.mutex,
+    // then call app_manager_stop() outside it (that call bound-joins the
+    // instance's thread, which itself takes ledger.mutex in its thread_main).
+    std::vector<uint32_t> instance_ids;
+    auto& ledger = app_ledger();
+    mutex_lock(&ledger.mutex);
+    for (const auto& [id, record] : ledger.instances) {
+        if (record.manifest == manifest) {
+            instance_ids.push_back(id);
+        }
+    }
+    mutex_unlock(&ledger.mutex);
+
+    for (uint32_t id : instance_ids) {
+        app_manager_stop(id);
+    }
+
+    // app_manager_remove takes ledger.mutex internally - call outside both
+    // registry.mutex and ledger.mutex to match the lock ordering in
+    // app_manager_install_path_scan().
+    app_manager_remove(app_id);
+
+    // Every instance has stopped and the manifest is unregistered — safe to
+    // delete the on-disk directory.  Delete before erasing the scan record so
+    // that a failed deletion leaves the entry discoverable for a retry.
+    if (!app_fs_delete_recursively(path)) {
+        return ERROR_RESOURCE;
+    }
+
+    mutex_lock(&registry.mutex);
+    registry.scanned.erase(app_id);
+    mutex_unlock(&registry.mutex);
+
+    return ERROR_NONE;
+}
+
 } // extern "C"

Modules/app-module/source/symbols.cpp

@@ -40,6 +40,7 @@ const ModuleSymbol app_module_symbols[] = {
     DEFINE_MODULE_SYMBOL(app_manager_get_topmost_app_id),
     DEFINE_MODULE_SYMBOL(app_manager_install_path_add),
     DEFINE_MODULE_SYMBOL(app_manager_install_path_scan),
+    DEFINE_MODULE_SYMBOL(app_manager_install_path_uninstall),
     // app/metadata
     DEFINE_MODULE_SYMBOL(app_metadata_parse),
     // app/paths

Modules/service-module/source/paths.cpp

@@ -9,7 +9,7 @@ extern "C" {
 
 error_t service_paths_get_user_data_directory(const char* service_id, char* out_path, size_t out_path_size) {
     char root[192];
-    error_t error = paths_get_user_data_path(root, sizeof(root));
+    error_t error = paths_get_data_path(root, sizeof(root));
     if (error != ERROR_NONE) {
         return error;
     }

Modules/service-module/tests/source/service_paths_test.cpp

@@ -7,20 +7,20 @@
 #include <cstring>
 #include <string>
 
-TEST_CASE("paths_get_user_data_path returns a non-empty path") {
+TEST_CASE("paths_get_data_path returns a non-empty path") {
     char buffer[192];
-    CHECK_EQ(paths_get_user_data_path(buffer, sizeof(buffer)), ERROR_NONE);
+    REQUIRE_EQ(paths_get_data_path(buffer, sizeof(buffer)), ERROR_NONE);
     CHECK_GT(std::strlen(buffer), 0);
 }
 
-TEST_CASE("paths_get_user_data_path reports overflow for a too-small buffer") {
+TEST_CASE("paths_get_data_path reports overflow for a too-small buffer") {
     char buffer[1];
-    CHECK_EQ(paths_get_user_data_path(buffer, sizeof(buffer)), ERROR_BUFFER_OVERFLOW);
+    CHECK_EQ(paths_get_data_path(buffer, sizeof(buffer)), ERROR_BUFFER_OVERFLOW);
 }
 
 TEST_CASE("service_paths_get_user_data_directory includes the service id") {
     char root[192];
-    REQUIRE_EQ(paths_get_user_data_path(root, sizeof(root)), ERROR_NONE);
+    REQUIRE_EQ(paths_get_data_path(root, sizeof(root)), ERROR_NONE);
 
     char buffer[224];
     CHECK_EQ(service_paths_get_user_data_directory("my-service", buffer, sizeof(buffer)), ERROR_NONE);