Fixes

Firmware/CMakeLists.txt

@@ -96,6 +96,8 @@ else ()
         Tactility
         TactilityFreeRtos
         lvgl-module
+        lvgl-window-manager-module
+        app-module
         crypt-module
         gps-module
         gps-generic-module

Modules/app-esp32-module/source/app_esp32_loader_service.cpp

@@ -151,7 +151,7 @@ void destroy_service(const ServiceManifest*, void*) {
 
 } // namespace
 
-extern ServiceManifest loader_service_manifest = {
+ServiceManifest loader_service_manifest = {
     .id = APP_LOADER_PATH_SERVICE_ID,
     .create_service = create_service,
     .destroy_service = destroy_service,

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

@@ -6,12 +6,32 @@
 
 #include <tactility/concurrent/mutex.h>
 #include <tactility/freertos/freertos.h>
+#include <tactility/freertos/semphr.h>
 #include <tactility/freertos/task.h>
 
 #include <stdint.h>
 #include <string>
 #include <unordered_map>
 
+/**
+ * A dedicated (not the task's shared default FreeRTOS notification, which app_event.cpp's
+ * AppEventSubscription also uses - an unrelated event delivered to the same task could
+ * otherwise unblock a waiter early) completion signal for one app instance's task, given as the
+ * literal last action app_task_main() takes before vTaskDelete(). Heap-allocated with its own
+ * refcount (protected by app_ledger().mutex, not atomic) rather than owned by the ledger
+ * entry, since app_task_main() always erases that entry - and may run its exit path entirely -
+ * before app_scheduler_stop() ever looks for it: whichever side (the exiting task, or a
+ * concurrent app_scheduler_stop() that found the entry in time and is waiting on `semaphore`)
+ * finishes with it last is the one that deletes `semaphore` and frees this struct.
+ */
+struct AppCompletionSignal {
+    SemaphoreHandle_t semaphore;
+    /** Starts at 1, owned by app_task_main() until its own exit. app_scheduler_stop() takes an
+     * additional reference for as long as it's waiting on `semaphore`, if it finds the instance
+     * still running. Reaching 0 means deletion. */
+    int refcount = 1;
+};
+
 /** A registered/running app instance, as tracked internally by app-module. */
 struct AppInstanceRecord {
     uint32_t id;
@@ -25,11 +45,9 @@ struct AppInstanceRecord {
      * app_manager_start_for_result() - the instance that receives this child's APP_EVENT_RESULT. */
     uint32_t parent_id = 0;
 
-    /** The task currently blocked in app_scheduler_stop() for this instance, if any - notified
+    /** This instance's completion signal - see AppCompletionSignal. Set once by
-     * (via xTaskNotifyGive()) as the literal last action app_task_main() takes before
+     * app_scheduler_start(), never reassigned. */
-     * vTaskDelete(), so app_scheduler_stop() can't observe completion before the task has
+    AppCompletionSignal* completion = nullptr;
-     * actually finished running. See app_scheduler.cpp. */
-    TaskHandle_t stop_waiter = nullptr;
 };
 
 struct AppLedger {

Modules/app-module/source/app_scheduler.cpp

@@ -35,6 +35,7 @@ struct TaskContext {
     AppInstanceId app_instance_id;
     int argc;
     char** argv;
+    AppCompletionSignal* completion;
 };
 
 void set_state(AppInstanceId app_instance_id, AppInstanceState state) {
@@ -57,21 +58,44 @@ void set_task(AppInstanceId app_instance_id, TaskHandle_t task) {
     mutex_unlock(&ledger.mutex);
 }
 
-// Registers the calling task to be notified when app_instance_id's task actually finishes
+void set_completion(AppInstanceId app_instance_id, AppCompletionSignal* completion) {
-// running (see app_task_main()'s exit path), and reports whether there's anything to wait for.
-// @return true if the instance's ledger entry still exists (a wait was registered); false if
-// the task has already fully finished (and already given any notification it would have) -
-// there is nothing left to wait for.
-bool register_stop_waiter(AppInstanceId app_instance_id) {
     auto& ledger = app_ledger();
     mutex_lock(&ledger.mutex);
     auto iterator = ledger.instances.find(app_instance_id);
-    bool exists = iterator != ledger.instances.end();
+    if (iterator != ledger.instances.end()) {
-    if (exists) {
+        iterator->second.completion = completion;
-        iterator->second.stop_waiter = xTaskGetCurrentTaskHandle();
     }
     mutex_unlock(&ledger.mutex);
-    return exists;
+}
+
+// Takes a reference on app_instance_id's completion signal (see AppCompletionSignal), for the
+// caller to wait on. @return the signal to wait on, or NULL if the instance has already fully
+// finished (its ledger entry - and so its reference to the signal - is already gone) and so
+// there's nothing left to wait for.
+AppCompletionSignal* acquire_completion_signal(AppInstanceId app_instance_id) {
+    auto& ledger = app_ledger();
+    mutex_lock(&ledger.mutex);
+    auto iterator = ledger.instances.find(app_instance_id);
+    AppCompletionSignal* completion = nullptr;
+    if (iterator != ledger.instances.end()) {
+        completion = iterator->second.completion;
+        completion->refcount++;
+    }
+    mutex_unlock(&ledger.mutex);
+    return completion;
+}
+
+// Releases a reference taken by acquire_completion_signal(), deleting the signal (and its
+// semaphore) if this was the last one.
+void release_completion_signal(AppCompletionSignal* completion) {
+    auto& ledger = app_ledger();
+    mutex_lock(&ledger.mutex);
+    bool should_delete = (--completion->refcount == 0);
+    mutex_unlock(&ledger.mutex);
+    if (should_delete) {
+        vSemaphoreDelete(completion->semaphore);
+        delete completion;
+    }
 }
 
 const char* loader_service_id_for(AppLocationType type) {
@@ -137,26 +161,28 @@ void app_task_main(void* context) {
     app_ledger_free_arguments(ctx->argc, ctx->argv);
 
     AppInstanceId app_instance_id = ctx->app_instance_id;
+    AppCompletionSignal* completion = ctx->completion;
     delete ctx;
 
     LOG_I(TAG, "Thread for %d finished", app_instance_id);
 
-    // Erase the ledger entry before self-deleting, capturing whoever's blocked in
+    // Erase the ledger entry before self-deleting - see "Reap self-terminated app tasks":
-    // app_scheduler_stop() for this instance (if anyone) so they can be notified afterward.
+    // nothing else is guaranteed to ever call app_scheduler_stop() for this instance (the
+    // common case is the app just closing itself), so this can't wait for that to happen.
     auto& ledger = app_ledger();
     mutex_lock(&ledger.mutex);
-    auto iterator = ledger.instances.find(app_instance_id);
-    TaskHandle_t stop_waiter = (iterator != ledger.instances.end()) ? iterator->second.stop_waiter : nullptr;
     ledger.instances.erase(app_instance_id);
     mutex_unlock(&ledger.mutex);
 
     // Signal completion as the literal last action before this task ceases to exist, so
-    // app_scheduler_stop() can't observe "stopped" one step early (see its own comment) -
+    // app_scheduler_stop() can't observe "stopped" one step early - unlike watching the ledger
-    // unlike watching the ledger entry disappear, this can only happen once the task is truly
+    // entry disappear, this can only happen once the task is truly done running. A dedicated
-    // done running.
+    // semaphore rather than this task's default FreeRTOS notification, since app_event.cpp's
-    if (stop_waiter != nullptr) {
+    // AppEventSubscription also uses that shared slot - an unrelated event (e.g. a child's
-        xTaskNotifyGive(stop_waiter);
+    // APP_EVENT_RESULT) delivered to this same task could otherwise unblock a concurrent
-    }
+    // app_scheduler_stop() early.
+    xSemaphoreGive(completion->semaphore);
+    release_completion_signal(completion); // releases app_task_main()'s own reference
 
     vTaskDelete(nullptr);
 }
@@ -181,9 +207,27 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
         return load_result;
     }
 
-    auto* context = new (std::nothrow) TaskContext { loader, runtime, app_instance_id, argc, argv };
+    auto* completion = new (std::nothrow) AppCompletionSignal();
+    if (completion == nullptr) {
+        LOG_E(TAG, "Failed to allocate app");
+        loader->unload(runtime);
+        app_ledger_free_arguments(argc, argv);
+        return ERROR_OUT_OF_MEMORY;
+    }
+    completion->semaphore = xSemaphoreCreateBinary();
+    if (completion->semaphore == nullptr) {
+        LOG_E(TAG, "Failed to allocate app");
+        delete completion;
+        loader->unload(runtime);
+        app_ledger_free_arguments(argc, argv);
+        return ERROR_OUT_OF_MEMORY;
+    }
+
+    auto* context = new (std::nothrow) TaskContext { loader, runtime, app_instance_id, argc, argv, completion };
     if (context == nullptr) {
         LOG_E(TAG, "Failed to allocate app");
+        vSemaphoreDelete(completion->semaphore);
+        delete completion;
         loader->unload(runtime);
         app_ledger_free_arguments(argc, argv);
         return ERROR_OUT_OF_MEMORY;
@@ -200,6 +244,8 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
     BaseType_t create_result = xTaskCreate(app_task_main, task_name, 8192 / sizeof(StackType_t), context, tskIDLE_PRIORITY, &task_handle);
     if (create_result != pdPASS) {
         delete context;
+        vSemaphoreDelete(completion->semaphore);
+        delete completion;
         loader->unload(runtime);
         app_ledger_free_arguments(argc, argv);
         return ERROR_OUT_OF_MEMORY;
@@ -207,6 +253,7 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
     vTaskSuspend(task_handle);
 
     set_task(app_instance_id, task_handle);
+    set_completion(app_instance_id, completion);
     vTaskPrioritySet(task_handle, APP_TASK_PRIORITY);
     vTaskResume(task_handle);
 
@@ -214,22 +261,22 @@ error_t app_scheduler_start(AppInstanceId app_instance_id, AppLocation location,
 }
 
 error_t app_scheduler_stop(AppInstanceId app_instance_id, TickType_t join_timeout) {
-    // Drain any stale notification credit before registering as the waiter - otherwise a
+    AppCompletionSignal* completion = acquire_completion_signal(app_instance_id);
-    // leftover give from an unrelated earlier wait on this same task (e.g. a previous
+    if (completion != nullptr) {
-    // app_scheduler_stop() call that timed out and only got notified afterward) could make the
-    // take below return immediately for the wrong event. Mirrors app_event_await()'s same
-    // defensive drain.
-    ulTaskNotifyTake(pdTRUE, 0);
-
-    if (register_stop_waiter(app_instance_id)) {
         AppEvent event { .type = APP_EVENT_CLOSE, .timestamp = 0, .result = {} };
         app_event_emit(app_instance_id, &event);
 
-        // Blocks until app_task_main() gives this notification as the literal last thing it
+        // Blocks until app_task_main() gives this dedicated semaphore as the literal last
-        // does before vTaskDelete() - unlike polling the ledger for the task handle to clear,
+        // thing it does before vTaskDelete() - unlike polling the ledger for the task handle to
-        // this can't observe "stopped" while the task is still mid-exit (still running its own
+        // clear, this can't observe "stopped" while the task is still mid-exit (still running
-        // cleanup/vTaskDelete()).
+        // its own cleanup/vTaskDelete()). A dedicated semaphore rather than this task's default
-        if (ulTaskNotifyTake(pdTRUE, join_timeout) == 0) {
+        // FreeRTOS notification, since app_event.cpp's AppEventSubscription also uses that
+        // shared slot - an unrelated event (e.g. a different child's APP_EVENT_RESULT)
+        // delivered to this same task could otherwise unblock this early.
+        BaseType_t taken = xSemaphoreTake(completion->semaphore, join_timeout);
+        release_completion_signal(completion);
+
+        if (taken == pdFALSE) {
             LOG_W(TAG, "App instance %u did not stop in time", app_instance_id);
             return ERROR_TIMEOUT;
         }

Modules/app-module/source/manager.cpp

@@ -259,8 +259,16 @@ void app_manager_install_path_scan(void) {
         app_fs_list_direct_subdirectories(root, found_app_dirs);
     }
 
+    // Snapshot of what's already registered, taken once so the rest of this scan can run without holding registry.mutex
     mutex_lock(&registry.mutex);
+    std::unordered_map<std::string, std::string> known_paths; // id -> path
+    for (const auto& [id, record] : registry.scanned) {
+        known_paths.emplace(id, record->path);
+    }
+    mutex_unlock(&registry.mutex);
 
+    // Stat each manifest and parse it entirely without registry.mutex held (due to filesystem IO being slow)
+    std::vector<std::unique_ptr<ScannedAppManifest>> new_records;
     for (const auto& app_dir : found_app_dirs) {
         auto manifest_path = app_dir + "/manifest.properties";
         if (!app_fs_is_file(manifest_path)) {
@@ -273,7 +281,7 @@ void app_manager_install_path_scan(void) {
             continue;
         }
 
-        if (registry.scanned.contains(metadata.app_id)) {
+        if (known_paths.contains(metadata.app_id)) {
             continue; // already registered by an earlier scan
         }
 
@@ -288,29 +296,41 @@ void app_manager_install_path_scan(void) {
             .location = { APP_LOCATION_PATH, const_cast<char*>(record->path.c_str()) },
             .flags = 0,
         };
-
+        new_records.push_back(std::move(record));
-        if (app_manager_add(&record->manifest) != ERROR_NONE) {
-            LOG_E(TAG, "Failed to register app %s (duplicate id?)", record->id.c_str());
-            continue;
-        }
-
-        registry.scanned[record->id] = std::move(record);
     }
 
-    // Anything a previous scan registered whose directory has since disappeared (e.g. an SD
+    // Anything a previous scan registered whose directory has since disappeared  gets unregistered below.
-    // card was removed) just gets unregistered - no file deletion, no touching running
-    // instances, that's app_install()/app_uninstall()'s job, not scanning's.
     std::vector<std::string> missing_ids;
-    for (const auto& [id, record] : registry.scanned) {
+    for (const auto& [id, path] : known_paths) {
-        if (!app_fs_is_directory(record->path)) {
+        if (!app_fs_is_directory(path)) {
             missing_ids.push_back(id);
         }
     }
+
+    // app_manager_add()/app_manager_remove() take app-module's own ledger mutex internally -
+    // calling them while holding registry.mutex would establish a registry.mutex -> ledger-
+    // mutex lock order that any future opposite-order path would deadlock against, so these
+    // also run with registry.mutex released. registry.mutex is taken only afterward, briefly,
+    // to publish the results (plain in-memory map updates, no I/O or other locks involved).
     for (const auto& id : missing_ids) {
         app_manager_remove(id.c_str());
-        registry.scanned.erase(id);
+    }
+    std::vector<std::unique_ptr<ScannedAppManifest>> added_records;
+    for (auto& record : new_records) {
+        if (app_manager_add(&record->manifest) == ERROR_NONE) {
+            added_records.push_back(std::move(record));
+        } else {
+            LOG_E(TAG, "Failed to register app %s (duplicate id?)", record->id.c_str());
+        }
     }
 
+    mutex_lock(&registry.mutex);
+    for (const auto& id : missing_ids) {
+        registry.scanned.erase(id);
+    }
+    for (auto& record : added_records) {
+        registry.scanned[record->id] = std::move(record);
+    }
     mutex_unlock(&registry.mutex);
 }
 

Modules/lvgl-window-manager-module/include/lvgl_window_manager/window_manager.h

@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: Apache-2.0
 #pragma once
 
+#include "../../../app-module/include/app/instance.h"
+
+
 #include <lvgl.h>
 
 #include <tactility/error.h>
@@ -80,12 +83,13 @@ typedef void (*WindowCreateWidgetsFn)(lv_obj_t* root, void* user_data);
  * Creates a new window on top of the stack (last created = topmost). Deletes the previously
  * topmost window's widgets (if any) and builds this window's widgets immediately via
  * @a create_widgets - only the topmost window ever has live widgets.
+ * @param[in] app_instance_id the application instance this window belongs to, should not be 0
  * @param[in] user_data opaque; passed back to @a create_widgets on every call, including a
  * later rebuild triggered by window_manager_remove() - see its @warning about which thread that
  * can run on. Typically the calling app's own Context*.
  * @return the new window's id, or 0 if window_manager_start() hasn't been called
  */
-WindowId window_manager_create(uint32_t app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data);
+WindowId window_manager_create(AppInstanceId app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data);
 
 /**
  * Removes a window, wherever it is in the stack - not necessarily the topmost one. If it was

Modules/lvgl-window-manager-module/source/window_manager.cpp

@@ -1,42 +1,66 @@
 // SPDX-License-Identifier: Apache-2.0
 #include <lvgl_window_manager/window_manager.h>
 
+#include "../../app-module/include/app/instance.h"
+
 #include <lvgl/lvgl.h>
 
 #include <tactility/check.h>
 #include <tactility/concurrent/mutex.h>
+#include <tactility/freertos/semphr.h>
 
 #include <algorithm>
+#include <new>
 #include <vector>
 
 constexpr auto* TAG = "window_manager";
 
 namespace {
 
+/**
+ * A dedicated (not the waiting task's shared default FreeRTOS notification, which other
+ * subsystems - e.g. app_event.cpp's AppEventSubscription - also use; an unrelated notification
+ * delivered to the same task could otherwise unblock a wait early) completion signal for one
+ * window_manager_await_state_change() call. Heap-allocated with its own refcount (protected by
+ * WindowManagerState::mutex, not atomic) rather than owned solely by the WindowRecord, since
+ * window_manager_create()/remove() claim (read + clear) a window's signal under the lock but
+ * give it after releasing that lock - refcounting lets whichever side (the waiting task waking
+ * up, or the claimer after it gives) finishes last safely delete it.
+ */
+struct WindowWaitSignal {
+    SemaphoreHandle_t semaphore;
+    /** Starts at 1, owned by window_manager_await_state_change() until it's done waiting.
+     * Whoever claims this signal from a WindowRecord (see claim_waiter_locked()) takes an
+     * additional reference for as long as it takes to give the semaphore. Reaching 0 means
+     * deletion. */
+    int refcount = 1;
+};
+
 struct WindowRecord {
     WindowId id;
     uint32_t app_instance_id;
     WindowCreateWidgetsFn create_widgets;
     void* user_data;
 
-    /** Task blocked in window_manager_await_state_change() for this specific window, if any -
+    /** Set by window_manager_await_state_change() for this specific window, if a task is
-     * see that function's @warning on at most one concurrent awaiter per window. Per-window
+     * currently blocked there - see that function's @warning on at most one concurrent awaiter
-     * rather than a single manager-wide slot, since a stacked window manager serving several
+     * per window. Per-window rather than a single manager-wide slot, since a stacked window
-     * app tasks can have more than one window (though only ever one of them topmost/GRANTED at
+     * manager serving several app tasks can have more than one window (though only ever one of
-     * a time) with a live await() call outstanding. */
+     * them topmost/GRANTED at a time) with a live await() call outstanding. */
-    TaskHandle_t waiting_task = nullptr;
+    WindowWaitSignal* waiting_signal = nullptr;
 };
 
 struct WindowManagerState {
     /** Mutex for read/write operations. Shortly held. */
     Mutex mutex {};
 
-    /** Serializes the full start()/stop() transition (including the LVGL work done with
+    /** Serializes the full start()/stop()/create()/remove() transitions against each other,
-     * `mutex` released) so two concurrent starts can't both pass the `started` check and each
+     * including the LVGL work done with `mutex` released (and any create_widgets()/
-     * create their own root widget, and a concurrent stop can't run while a start is still
+     * screen_init() callback invoked as part of that work). Without this, e.g.
-     * mid-flight. Never held across a create_widgets()/screen_init() callback - those only
+     * window_manager_stop() could delete real_root_widget/content_root_widget/top_widget
-     * reach window_manager_create()/remove(), not start()/stop() - so there's no lock-order
+     * between a concurrent create()/remove() capturing one of those pointers under `mutex` and
-     * risk with `mutex` or the LVGL lock. */
+     * actually using it via build_window_widget()/delete_widget() after releasing `mutex` -
+     * touching an LVGL object it no longer holds a valid reference to. */
     Mutex lifecycle_mutex {};
 
     bool started = false;
@@ -91,6 +115,38 @@ void delete_widget(lv_obj_t* widget) {
     lvgl_unlock();
 }
 
+// Call while holding WindowManagerState::mutex. Transfers ownership of `window`'s waiting
+// signal (if any) to the caller, taking an additional reference on the caller's behalf - the
+// caller must eventually pass the result to give_and_release() exactly once, outside the lock.
+WindowWaitSignal* claim_waiter_locked(WindowRecord& window) {
+    WindowWaitSignal* signal = window.waiting_signal;
+    window.waiting_signal = nullptr;
+    if (signal != nullptr) {
+        signal->refcount++;
+    }
+    return signal;
+}
+
+// Gives `signal`'s semaphore (waking window_manager_await_state_change() if it's still
+// waiting) and releases the caller's reference (see claim_waiter_locked()), deleting the
+// signal if that was the last one. No-op if `signal` is NULL.
+void give_and_release(WindowWaitSignal* signal) {
+    if (signal == nullptr) {
+        return;
+    }
+
+    xSemaphoreGive(signal->semaphore);
+
+    auto& s = state();
+    mutex_lock(&s.mutex);
+    bool should_delete = (--signal->refcount == 0);
+    mutex_unlock(&s.mutex);
+    if (should_delete) {
+        vSemaphoreDelete(signal->semaphore);
+        delete signal;
+    }
+}
+
 } // namespace
 
 extern "C" {
@@ -178,12 +234,12 @@ error_t window_manager_stop(void) {
         return ERROR_NONE;
     }
     lv_obj_t* widget = s.real_root_widget;
-    // Collect every window's waiter before clearing - normally at most the topmost window's is
+    // Claim every window's waiter before clearing - normally at most the topmost window's is
     // ever set, but every window is being torn down here, so every one is checked.
-    std::vector<TaskHandle_t> waiters;
+    std::vector<WindowWaitSignal*> waiters;
-    for (const auto& window : s.windows) {
+    for (auto& window : s.windows) {
-        if (window.waiting_task != nullptr) {
+        if (auto* signal = claim_waiter_locked(window); signal != nullptr) {
-            waiters.push_back(window.waiting_task);
+            waiters.push_back(signal);
         }
     }
     s.real_root_widget = nullptr;
@@ -193,8 +249,8 @@ error_t window_manager_stop(void) {
     s.started = false;
     mutex_unlock(&s.mutex);
 
-    for (TaskHandle_t waiter : waiters) {
+    for (WindowWaitSignal* waiter : waiters) {
-        xTaskNotifyGive(waiter);
+        give_and_release(waiter);
     }
 
     // Deleting the real widget cascades to everything under it - chrome and top_widget alike.
@@ -204,31 +260,35 @@ error_t window_manager_stop(void) {
     return ERROR_NONE;
 }
 
-WindowId window_manager_create(uint32_t app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data) {
+WindowId window_manager_create(AppInstanceId app_instance_id, WindowCreateWidgetsFn create_widgets, void* user_data) {
+    if (app_instance_id == 0) {
+        return 0;
+    }
+
     auto& s = state();
 
+    // See lifecycle_mutex's comment - blocks a concurrent window_manager_stop() (or another
+    // create()/remove()) from touching real_root_widget/content_root_widget/top_widget while
+    // this call still holds pointers to them.
+    mutex_lock(&s.lifecycle_mutex);
+
     mutex_lock(&s.mutex);
     if (!s.started) {
         mutex_unlock(&s.mutex);
+        mutex_unlock(&s.lifecycle_mutex);
         return 0;
     }
     lv_obj_t* content = s.content_root_widget;
     lv_obj_t* old_top_widget = s.top_widget;
-    // The current topmost window (if any) is about to be superseded - transfer its waiter (if
+    // The current topmost window (if any) is about to be superseded - claim its waiter (if
     // any) here so it gets notified below, since it's no longer topmost after this.
-    TaskHandle_t waiter = nullptr;
+    WindowWaitSignal* waiter = !s.windows.empty() ? claim_waiter_locked(s.windows.back()) : nullptr;
-    if (!s.windows.empty()) {
-        waiter = s.windows.back().waiting_task;
-        s.windows.back().waiting_task = nullptr;
-    }
     s.top_widget = nullptr;
     WindowId new_id = s.next_id++;
     s.windows.push_back(WindowRecord { new_id, app_instance_id, create_widgets, user_data });
     mutex_unlock(&s.mutex);
 
-    if (waiter != nullptr) {
+    give_and_release(waiter);
-        xTaskNotifyGive(waiter);
-    }
 
     delete_widget(old_top_widget);
     lv_obj_t* new_widget = build_window_widget(content, create_widgets, user_data);
@@ -245,25 +305,32 @@ WindowId window_manager_create(uint32_t app_instance_id, WindowCreateWidgetsFn c
     // another app thread) - discard what we just made.
     delete_widget(new_widget);
 
+    mutex_unlock(&s.lifecycle_mutex);
     return new_id;
 }
 
 void window_manager_remove(WindowId id) {
     auto& s = state();
 
+    // See lifecycle_mutex's comment - blocks a concurrent window_manager_stop() (or another
+    // create()/remove()) from touching real_root_widget/content_root_widget/top_widget while
+    // this call still holds pointers to them.
+    mutex_lock(&s.lifecycle_mutex);
+
     mutex_lock(&s.mutex);
     auto iterator = std::find_if(s.windows.begin(), s.windows.end(),
         [id](const WindowRecord& window) { return window.id == id; });
     if (iterator == s.windows.end()) {
         mutex_unlock(&s.mutex);
+        mutex_unlock(&s.lifecycle_mutex);
         return;
     }
     bool was_topmost = (iterator + 1 == s.windows.end());
     // The window being removed owns its own waiter (if any) - a waiter is only ever registered
     // while its window is topmost (see window_manager_await_state_change()), and if this window
     // later stopped being topmost without being removed, window_manager_create() would already
-    // have transferred/cleared it - so a buried window's waiting_task is always already null.
+    // have claimed/cleared it - so a buried window's waiting_signal is always already null.
-    TaskHandle_t waiter = iterator->waiting_task;
+    WindowWaitSignal* waiter = claim_waiter_locked(*iterator);
     s.windows.erase(iterator);
 
     lv_obj_t* content = s.content_root_widget;
@@ -285,12 +352,11 @@ void window_manager_remove(WindowId id) {
     }
     mutex_unlock(&s.mutex);
 
-    if (waiter != nullptr) {
+    give_and_release(waiter);
-        xTaskNotifyGive(waiter);
-    }
 
     if (!was_topmost) {
         // A buried window was removed - the topmost window's widgets are unaffected.
+        mutex_unlock(&s.lifecycle_mutex);
         return;
     }
 
@@ -306,6 +372,8 @@ void window_manager_remove(WindowId id) {
     mutex_unlock(&s.mutex);
 
     delete_widget(new_widget);
+
+    mutex_unlock(&s.lifecycle_mutex);
 }
 
 WindowState window_manager_get_state(WindowId id) {
@@ -319,36 +387,51 @@ WindowState window_manager_get_state(WindowId id) {
 WindowState window_manager_await_state_change(WindowId id, TickType_t timeout) {
     auto& s = state();
 
+    // Dedicated semaphore rather than this task's default FreeRTOS notification - other
+    // subsystems (e.g. app_event.cpp's AppEventSubscription) use that same shared slot, so an
+    // unrelated notification delivered to this task could otherwise wake this wait early.
+    auto* signal = new (std::nothrow) WindowWaitSignal();
+    if (signal == nullptr) {
+        return window_manager_get_state(id);
+    }
+    signal->semaphore = xSemaphoreCreateBinary();
+    if (signal->semaphore == nullptr) {
+        delete signal;
+        return window_manager_get_state(id);
+    }
+
     mutex_lock(&s.mutex);
     bool is_top = !s.windows.empty() && s.windows.back().id == id;
     if (!is_top) {
         mutex_unlock(&s.mutex);
+        vSemaphoreDelete(signal->semaphore);
+        delete signal;
         return WINDOW_STATE_REVOKED;
     }
     // At most one concurrent awaiter per window - see the @warning on this function.
-    check(s.windows.back().waiting_task == nullptr);
+    check(s.windows.back().waiting_signal == nullptr);
-    s.windows.back().waiting_task = xTaskGetCurrentTaskHandle();
+    s.windows.back().waiting_signal = signal;
     mutex_unlock(&s.mutex);
 
-    ulTaskNotifyTake(pdTRUE, timeout);
+    xSemaphoreTake(signal->semaphore, timeout);
 
-    /* Deregister ourselves if a create()/remove() hasn't already claimed us (the ordinary, intended wakeup)
+    // Deregister ourselves if a create()/remove() hasn't already claimed us (the ordinary,
-     * Otherwise a later create()/remove() could notify a task that's no longer waiting here:
+    // intended wakeup) - otherwise a later create()/remove() could read a signal that's already
-     * a use-after-exit on the handle if this task is gone, or a stale wakeup the next time it waits.
+    // been given away here. Re-locate the record by id - it may have been erased
-     * Re-locate the record by id - it may have been erased (window_manager_remove()) while we waited. */
+    // (window_manager_remove()) while we waited. Either way, release our own reference:
+    // whichever side (us or a claimer) does this last is the one that actually deletes it.
     mutex_lock(&s.mutex);
     auto iterator = std::find_if(s.windows.begin(), s.windows.end(),
         [id](const WindowRecord& window) { return window.id == id; });
-    if (iterator != s.windows.end() && iterator->waiting_task == xTaskGetCurrentTaskHandle()) {
+    if (iterator != s.windows.end() && iterator->waiting_signal == signal) {
-        iterator->waiting_task = nullptr;
+        iterator->waiting_signal = nullptr;
     }
+    bool should_delete = (--signal->refcount == 0);
     mutex_unlock(&s.mutex);
-
+    if (should_delete) {
-    /* create()/remove() read+clear `waiting_task` under the lock but call xTaskNotifyGive()
+        vSemaphoreDelete(signal->semaphore);
-     * after releasing it, so a notification can still land on us right around the timeout
+        delete signal;
-     * boundary regardless of which branch above ran. Drain it now (non-blocking) so it doesn't
+    }
-     * linger and cause a spurious immediate return the next time this task awaits. */
-    ulTaskNotifyTake(pdTRUE, 0);
 
     return window_manager_get_state(id);
 }

Tactility/CMakeLists.txt

@@ -8,6 +8,9 @@ list(APPEND REQUIRES_LIST
     TactilityKernel
     TactilityFreeRtos
     lvgl-module
+    lvgl-window-manager-module
+    app-module
+    app-esp32-module
     crypt-module
     gps-module
     gps-generic-module

TactilityC/Include/tt_app_fileselection.h

@@ -8,13 +8,13 @@ extern "C" {
 
 /**
  * Show a file selection dialog that allows the user to select an existing file.
- * @return the launch ID of the dialog, which can be compared in onResult to identify the source
+ * @return the launch ID of the dialog
  */
 AppInstanceId tt_app_fileselection_start_for_existing_file(AppInstanceId app_id);
 
 /**
  * Show a file selection dialog that allows the user to select a new or existing file.
- * @return the launch ID of the dialog, which can be compared in onResult to identify the source
+ * @return the launch ID of the dialog
  */
 AppInstanceId tt_app_fileselection_start_for_existing_or_new_file(AppInstanceId app_id);
 

TactilityKernel/include/tactility/preferences.h

@@ -21,12 +21,13 @@ extern "C" {
 typedef struct Preferences Preferences;
 
 /**
- * Open (or create) a preferences store backed by the properties file at @a path. The file is
+ * Open (or create) a preferences store backed by the properties file at @a path. The parent
+ * directory is created (recursively, like mkdir -p) if it doesn't already exist. The file is
  * read into memory now; changes made with preferences_put_*() are only written back to disk by
  * preferences_close().
- * @param[in] path absolute or relative file path (e.g. "/data/settings.properties") - the
+ * @param[in] path absolute or relative file path (e.g. "/data/settings.properties")
- * parent directory must already exist
+ * @return the new instance, or NULL if the parent directory couldn't be created, or on
- * @return the new instance, or NULL on allocation failure
+ * allocation failure
  */
 Preferences* preferences_open(const char* path);
 

TactilityKernel/include/tactility/properties_file.h

@@ -26,13 +26,22 @@ typedef struct PropertiesFile PropertiesFile;
  * made with properties_file_set() are only written back to disk by properties_file_close().
  * @param[in] path absolute or relative file path (e.g. "/data/settings.properties") - the
  * parent directory must already exist
- * @return the new instance, or NULL on allocation failure
+ * @return the new instance, or NULL on allocation failure, or NULL if @a path exists but a
+ * genuine I/O error interrupted reading it (a missing file is not an error - the instance
+ * starts out empty in that case)
  */
 PropertiesFile* properties_file_open(const char* path);
 
-/** Writes any pending properties_file_set() changes to the backing file, then releases the
+/**
- * instance. */
+ * Writes any pending properties_file_set() changes to the backing file (atomically - via a
-void properties_file_close(PropertiesFile* file);
+ * temporary file in the same directory, renamed over the real path - so a write failure leaves
+ * the previous on-disk content untouched rather than a truncated/partial file), then releases
+ * the instance either way.
+ * @retval ERROR_NONE the backing file was fully updated
+ * @retval ERROR_RESOURCE writing failed (full filesystem, I/O error, ...) - the previous
+ * on-disk content, if any, is unchanged; the in-memory changes are lost along with the instance
+ */
+error_t properties_file_close(PropertiesFile* file);
 
 bool properties_file_has(const PropertiesFile* file, const char* key);
 

TactilityKernel/include/tactility/system_event.h

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 #pragma once
 
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ -161,6 +162,15 @@ struct SystemEventSubscription {
         uint32_t sequence;
         uint32_t consumed_sequence;
 
+        /** Number of tasks currently blocked in system_event_await() on `semaphore` -
+         * system_event_unsubscribe() waits for this to reach 0 before deleting it, since
+         * FreeRTOS requires no task be blocked on a semaphore when it's deleted. */
+        int waiter_count;
+        /** Set by system_event_unsubscribe() before it gives `semaphore` and waits, so a task
+         * already blocked in system_event_await() bails out (ERROR_INVALID_STATE) instead of
+         * waiting out its full timeout. Reset on the next system_event_subscribe(). */
+        bool cancelled;
+
         struct SystemEventSubscription* next;
     } internal;
 };
@@ -180,6 +190,10 @@ error_t system_event_subscribe(struct SystemEventSubscription* sub);
 /**
  * Remove a previously registered poll subscription.
  * @warning Does not work in ISR context.
+ * @warning Blocks (briefly - not for the full duration of anyone's timeout) until any task
+ * currently blocked in system_event_await() on @a sub has woken up and left, so it's safe to
+ * delete the subscription's semaphore before this call returns. A blocked awaiter is woken
+ * (with ERROR_INVALID_STATE) as part of this call rather than left to time out on its own.
  * @param[in] sub subscription to remove, as passed to system_event_subscribe()
  * @return ERROR_NONE on success, ERROR_NOT_FOUND if no matching subscription exists
  */
@@ -187,9 +201,18 @@ error_t system_event_unsubscribe(struct SystemEventSubscription* sub);
 
 /**
  * Blocks the calling task until a new event arrives for @a sub, or timeout elapses.
+ * @warning Poll subscriptions coalesce to the latest event, they are not a queue: if
+ * system_event_emit() is called more than once for @a sub->event.type between two
+ * system_event_await() calls, only the most recent event's data/timestamp is visible via
+ * system_event_get_data()/system_event_get_timestamp() afterward - intermediate events are
+ * silently overwritten, never delivered. Use system_event_callback_add() instead if every
+ * individual event matters.
  * @param[in,out] sub subscription to wait on, as passed to system_event_subscribe()
  * @param[in] timeout max ticks to wait
- * @return ERROR_NONE if an event arrived, ERROR_TIMEOUT if the timeout elapsed
+ * @retval ERROR_NONE an event arrived
+ * @retval ERROR_TIMEOUT @a timeout elapsed first
+ * @retval ERROR_INVALID_STATE another task called system_event_unsubscribe() on @a sub while
+ * this call was blocked
  */
 error_t system_event_await(struct SystemEventSubscription* sub, TickType_t timeout);
 

TactilityKernel/source/bundle.cpp

@@ -41,8 +41,19 @@ Bundle* bundle_alloc(void) {
 
 Bundle* bundle_clone(const Bundle* bundle) {
     auto* clone = new (std::nothrow) Bundle();
-    if (clone != nullptr) {
+    if (clone == nullptr) {
+        return nullptr;
+    }
+    // The Bundle allocation above is nothrow, but copy-assigning `entries` (allocating a node
+    // and copying the key/value_string for every entry) is not - std::bad_alloc could still
+    // escape mid-copy. Callers only ever check for a NULL return, so convert that into the
+    // documented nullptr-on-failure contract instead of letting it propagate out of this
+    // extern "C" function (which would be undefined behavior).
+    try {
         clone->entries = bundle->entries;
+    } catch (...) {
+        delete clone;
+        return nullptr;
     }
     return clone;
 }

TactilityKernel/source/preferences.cpp

@@ -3,6 +3,7 @@
 #include <tactility/properties_file.h>
 #include <tactility/paths.h>
 
+#include <cerrno>
 #include <cinttypes>
 #include <cstdio>
 #include <cstdlib>
@@ -57,6 +58,60 @@ bool split_tag(const std::string& tagged_value, std::string& tag, std::string& r
     return true;
 }
 
+// Rejects anything but an exact "0" or "1" - a manually edited or corrupted properties file
+// could otherwise have e.g. "b:garbage" silently read back as false.
+bool parse_bool(const std::string& raw_value, bool& out) {
+    if (raw_value == "0") {
+        out = false;
+        return true;
+    }
+    if (raw_value == "1") {
+        out = true;
+        return true;
+    }
+    return false;
+}
+
+// strtol()'s own error signaling (errno/end pointer) is easy to get wrong by omission: called
+// naively, it silently accepts trailing garbage ("42abc"), out-of-range input (clamped to
+// LONG_MIN/LONG_MAX instead of failing), and - since `long` can be wider than int32_t (e.g. on
+// the posix simulator, where `long` is 64-bit) - a value that overflows int32_t but not `long`
+// would silently truncate on the narrowing cast instead of being rejected.
+bool parse_int32(const std::string& raw_value, int32_t& out) {
+    if (raw_value.empty()) {
+        return false;
+    }
+    errno = 0;
+    char* end = nullptr;
+    long parsed = std::strtol(raw_value.c_str(), &end, 10);
+    if (errno == ERANGE || end != raw_value.c_str() + raw_value.size()) {
+        return false;
+    }
+    if (parsed < INT32_MIN || parsed > INT32_MAX) {
+        return false;
+    }
+    out = static_cast<int32_t>(parsed);
+    return true;
+}
+
+// See parse_int32() - same reasoning, with strtoll()/`long long`/int64_t.
+bool parse_int64(const std::string& raw_value, int64_t& out) {
+    if (raw_value.empty()) {
+        return false;
+    }
+    errno = 0;
+    char* end = nullptr;
+    long long parsed = std::strtoll(raw_value.c_str(), &end, 10);
+    if (errno == ERANGE || end != raw_value.c_str() + raw_value.size()) {
+        return false;
+    }
+    if (parsed < INT64_MIN || parsed > INT64_MAX) {
+        return false;
+    }
+    out = static_cast<int64_t>(parsed);
+    return true;
+}
+
 bool ensure_directory(const std::string& path) {
     struct stat info {};
     if (stat(path.c_str(), &info) == 0) {
@@ -75,6 +130,16 @@ bool ensure_directory_recursive(const std::string& path) {
     return ensure_directory(path);
 }
 
+// "" if @a path has no directory component (e.g. a bare filename) - nothing to create in that
+// case, the current/root directory already exists.
+std::string parent_directory(const std::string& path) {
+    size_t slash = path.find_last_of('/');
+    if (slash == std::string::npos) {
+        return "";
+    }
+    return path.substr(0, slash);
+}
+
 } // namespace
 
 // Definition of the opaque handle declared in tactility/preferences.h - C callers only ever
@@ -109,6 +174,11 @@ bool try_get_tagged(const PropertiesFile* file, const char* key, std::string& ta
 extern "C" {
 
 Preferences* preferences_open(const char* path) {
+    std::string directory = parent_directory(path);
+    if (!directory.empty() && !ensure_directory_recursive(directory)) {
+        return nullptr;
+    }
+
     PropertiesFile* file = properties_file_open(path);
     if (file == nullptr) {
         return nullptr;
@@ -129,17 +199,20 @@ void preferences_close(Preferences* preferences) {
 
 bool preferences_has_bool(const Preferences* preferences, const char* key) {
     std::string tag, raw_value;
-    return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "b";
+    bool value;
+    return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "b" && parse_bool(raw_value, value);
 }
 
 bool preferences_has_int32(const Preferences* preferences, const char* key) {
     std::string tag, raw_value;
-    return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i32";
+    int32_t value;
+    return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i32" && parse_int32(raw_value, value);
 }
 
 bool preferences_has_int64(const Preferences* preferences, const char* key) {
     std::string tag, raw_value;
-    return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i64";
+    int64_t value;
+    return try_get_tagged(preferences->file, key, tag, raw_value) && tag == "i64" && parse_int64(raw_value, value);
 }
 
 bool preferences_has_string(const Preferences* preferences, const char* key) {
@@ -152,8 +225,7 @@ bool preferences_opt_bool(const Preferences* preferences, const char* key, bool*
     if (!try_get_tagged(preferences->file, key, tag, raw_value) || tag != "b") {
         return false;
     }
-    *out_value = (raw_value == "1");
+    return parse_bool(raw_value, *out_value);
-    return true;
 }
 
 bool preferences_opt_int32(const Preferences* preferences, const char* key, int32_t* out_value) {
@@ -161,8 +233,7 @@ bool preferences_opt_int32(const Preferences* preferences, const char* key, int3
     if (!try_get_tagged(preferences->file, key, tag, raw_value) || tag != "i32") {
         return false;
     }
-    *out_value = static_cast<int32_t>(std::strtol(raw_value.c_str(), nullptr, 10));
+    return parse_int32(raw_value, *out_value);
-    return true;
 }
 
 bool preferences_opt_int64(const Preferences* preferences, const char* key, int64_t* out_value) {
@@ -170,8 +241,7 @@ bool preferences_opt_int64(const Preferences* preferences, const char* key, int6
     if (!try_get_tagged(preferences->file, key, tag, raw_value) || tag != "i64") {
         return false;
     }
-    *out_value = static_cast<int64_t>(std::strtoll(raw_value.c_str(), nullptr, 10));
+    return parse_int64(raw_value, *out_value);
-    return true;
 }
 
 error_t preferences_opt_string(const Preferences* preferences, const char* key, char* out_value, size_t out_value_size) {

TactilityKernel/source/properties_file.cpp

@@ -48,7 +48,10 @@ namespace {
 // close(). Mirrors Tactility's loadPropertiesFile(): "#"-prefixed and blank lines are skipped;
 // a "[section]" line becomes a literal prefix (verbatim, brackets included) prepended to every
 // subsequent key, until the next "[section]" line replaces it.
-void load_from_file(PropertiesFile* file) {
+// @return false if the file exists but a genuine I/O error interrupted reading it (fgetc()'s
+// EOF return doesn't by itself distinguish clean end-of-file from a read error - ferror() after
+// the loop does); true otherwise, including for a missing file.
+bool load_from_file(PropertiesFile* file) {
     FileMutex mutex {};
     file_mutex_get(&mutex, file->path.c_str());
     file_mutex_lock(&mutex);
@@ -56,7 +59,7 @@ void load_from_file(PropertiesFile* file) {
     FILE* handle = std::fopen(file->path.c_str(), "r");
     if (handle == nullptr) {
         file_mutex_unlock(&mutex);
-        return;
+        return true;
     }
 
     std::string key_prefix;
@@ -94,28 +97,62 @@ void load_from_file(PropertiesFile* file) {
     }
     flush_line();
 
+    bool read_ok = std::ferror(handle) == 0;
     std::fclose(handle);
     file_mutex_unlock(&mutex);
+
+    if (!read_ok) {
+        LOG_E(TAG, "Failed to read %s", file->path.c_str());
+    }
+    return read_ok;
 }
 
-void save_to_file(const PropertiesFile* file) {
+// Writes to a temporary file in the same directory, then atomically replaces the real path -
+// opening the real path directly with "w" would truncate it immediately, so any failure
+// partway through (full filesystem, I/O error, a reset before close) would discard the
+// previously-good content instead of leaving it intact. Same directory so rename() stays on one
+// filesystem, which is what makes it atomic.
+// @return true if the backing file was fully replaced with the current entries; false (leaving
+// the previous on-disk content untouched) if any step failed.
+bool save_to_file(const PropertiesFile* file) {
     FileMutex mutex {};
     file_mutex_get(&mutex, file->path.c_str());
     file_mutex_lock(&mutex);
 
-    FILE* handle = std::fopen(file->path.c_str(), "w");
+    std::string temp_path = file->path + ".tmp";
+
+    FILE* handle = std::fopen(temp_path.c_str(), "w");
     if (handle == nullptr) {
-        LOG_E(TAG, "Failed to open %s", file->path.c_str());
+        LOG_E(TAG, "Failed to open %s", temp_path.c_str());
         file_mutex_unlock(&mutex);
-        return;
+        return false;
     }
 
     for (const auto& [key, value] : file->entries) {
         std::fprintf(handle, "%s=%s\n", key.c_str(), value.c_str());
     }
 
-    std::fclose(handle);
+    // Order matters: ferror()/fflush() need the still-open handle, fclose() consumes it.
+    bool write_ok = std::ferror(handle) == 0;
+    bool flush_ok = std::fflush(handle) == 0;
+    bool close_ok = std::fclose(handle) == 0;
+
+    if (!write_ok || !flush_ok || !close_ok) {
+        LOG_E(TAG, "Failed to write %s", temp_path.c_str());
+        std::remove(temp_path.c_str());
+        file_mutex_unlock(&mutex);
+        return false;
+    }
+
+    if (std::rename(temp_path.c_str(), file->path.c_str()) != 0) {
+        LOG_E(TAG, "Failed to replace %s", file->path.c_str());
+        std::remove(temp_path.c_str());
+        file_mutex_unlock(&mutex);
+        return false;
+    }
+
     file_mutex_unlock(&mutex);
+    return true;
 }
 
 } // namespace
@@ -128,13 +165,17 @@ PropertiesFile* properties_file_open(const char* path) {
         return nullptr;
     }
     file->path = path;
-    load_from_file(file);
+    if (!load_from_file(file)) {
+        delete file;
+        return nullptr;
+    }
     return file;
 }
 
-void properties_file_close(PropertiesFile* file) {
+error_t properties_file_close(PropertiesFile* file) {
-    save_to_file(file);
+    bool saved = save_to_file(file);
     delete file;
+    return saved ? ERROR_NONE : ERROR_RESOURCE;
 }
 
 bool properties_file_has(const PropertiesFile* file, const char* key) {

TactilityKernel/source/system_event.cpp

@@ -1,6 +1,7 @@
 #include <tactility/system_event.h>
 
 #include <tactility/concurrent/mutex.h>
+#include <tactility/delay.h>
 #include <tactility/error.h>
 #include <tactility/time.h>
 
@@ -186,6 +187,8 @@ error_t system_event_subscribe(SystemEventSubscription* sub) {
     sub->internal.semaphore = semaphore;
     sub->internal.sequence = 0;
     sub->internal.consumed_sequence = 0;
+    sub->internal.waiter_count = 0;
+    sub->internal.cancelled = false;
     sub->event.data_len = 0;
     sub->internal.next = poll_subscriptions;
     poll_subscriptions = sub;
@@ -197,6 +200,7 @@ error_t system_event_subscribe(SystemEventSubscription* sub) {
 
 error_t system_event_unsubscribe(SystemEventSubscription* sub) {
     error_t result = ERROR_NOT_FOUND;
+    SemaphoreHandle_t semaphore_to_delete = nullptr;
 
     mutex_lock(&poll_subscriptions_mutex.handle);
     for (SystemEventSubscription** link = &poll_subscriptions; *link != nullptr; link = &(*link)->internal.next) {
@@ -206,37 +210,109 @@ error_t system_event_unsubscribe(SystemEventSubscription* sub) {
             break;
         }
     }
-    mutex_unlock(&poll_subscriptions_mutex.handle);
-
     if (result == ERROR_NONE) {
-        // Unlinked first, so notify_poll_subscribers() can no longer reach this semaphore
+        // Unlinked first, so notify_poll_subscribers() can no longer reach this subscription.
-        // before it's deleted.
+        // Mark it cancelled (checked by system_event_await()'s loop) and capture the semaphore
-        vSemaphoreDelete(sub->internal.semaphore);
+        // handle into a local variable rather than deleting it via sub->internal.semaphore
+        // directly - a concurrent system_event_subscribe() re-registering this same `sub` after
+        // this point would overwrite that field with a freshly created semaphore, and we must
+        // not delete the wrong (newly active) one.
+        sub->internal.cancelled = true;
+        semaphore_to_delete = sub->internal.semaphore;
         sub->internal.semaphore = nullptr;
     }
+    mutex_unlock(&poll_subscriptions_mutex.handle);
+
+    if (result != ERROR_NONE) {
+        return result;
+    }
+
+    // Nudge any task already blocked in system_event_await() (it captured its own local copy
+    // of this same semaphore handle before this point, so it's unaffected by the field having
+    // just been cleared above) so it re-checks `cancelled` and bails out now instead of waiting
+    // out its full timeout, then wait for it to actually leave the semaphore before deleting it
+    // - FreeRTOS requires no task be blocked on a semaphore when it's deleted.
+    xSemaphoreGive(semaphore_to_delete);
+    while (true) {
+        mutex_lock(&poll_subscriptions_mutex.handle);
+        bool still_waiting = sub->internal.waiter_count > 0;
+        mutex_unlock(&poll_subscriptions_mutex.handle);
+
+        if (!still_waiting) {
+            break;
+        }
+        delay_ticks(pdMS_TO_TICKS(10));
+    }
+
+    vSemaphoreDelete(semaphore_to_delete);
+
+    // Reset so a future system_event_subscribe() re-registering this same `sub` isn't left
+    // pre-cancelled (subscribe() also resets this itself, defensively).
+    sub->internal.cancelled = false;
+
+    return ERROR_NONE;
+}
+
+error_t system_event_await(SystemEventSubscription* sub, TickType_t timeout) {
+    mutex_lock(&poll_subscriptions_mutex.handle);
+    SemaphoreHandle_t semaphore = sub->internal.semaphore;
+    sub->internal.waiter_count++;
+    mutex_unlock(&poll_subscriptions_mutex.handle);
+
+    error_t result = ERROR_NONE;
+
+    // sequence/consumed_sequence are written by notify_poll_subscribers() under
+    // poll_subscriptions_mutex - read (and, on a match, updated) under the same lock each
+    // iteration, rather than compared lock-free, so a concurrent emit can't land between an
+    // unlocked read and this loop acting on it.
+    //
+    // Compare against consumed_sequence, not a sequence snapshot taken now - an emit that
+    // landed between system_event_subscribe() and this call already incremented sequence and
+    // gave the semaphore, so that event is pending but unconsumed. Snapshotting "now" would
+    // make the loop wait for yet another event instead of returning this already-pending one.
+    while (true) {
+        mutex_lock(&poll_subscriptions_mutex.handle);
+        bool pending = sub->internal.sequence != sub->internal.consumed_sequence;
+        bool cancelled = sub->internal.cancelled;
+        if (pending) {
+            sub->internal.consumed_sequence = sub->internal.sequence;
+        }
+        mutex_unlock(&poll_subscriptions_mutex.handle);
+
+        if (pending) {
+            break;
+        }
+        if (cancelled) {
+            result = ERROR_INVALID_STATE;
+            break;
+        }
+        if (xSemaphoreTake(semaphore, timeout) == pdFALSE) {
+            result = ERROR_TIMEOUT;
+            break;
+        }
+    }
+
+    mutex_lock(&poll_subscriptions_mutex.handle);
+    sub->internal.waiter_count--;
+    mutex_unlock(&poll_subscriptions_mutex.handle);
 
     return result;
 }
 
-error_t system_event_await(SystemEventSubscription* sub, TickType_t timeout) {
-    uint32_t old_sequence = sub->internal.sequence;
-
-    while (sub->internal.sequence == old_sequence) {
-        if (xSemaphoreTake(sub->internal.semaphore, timeout) == pdFALSE) {
-            return ERROR_TIMEOUT;
-        }
-    }
-
-    sub->internal.consumed_sequence = sub->internal.sequence;
-    return ERROR_NONE;
-}
-
 error_t system_event_get_data(SystemEventSubscription* sub, uint8_t* data, size_t data_len) {
+    // sub->event.* is written by notify_poll_subscribers() under poll_subscriptions_mutex -
+    // the length check and the copy must happen as one snapshot under the same lock, otherwise
+    // a concurrent emit could grow data_len (or overwrite data) between the check and the
+    // memcpy below.
+    mutex_lock(&poll_subscriptions_mutex.handle);
+    error_t result = ERROR_NONE;
     if (data_len < sub->event.data_len) {
-        return ERROR_BUFFER_OVERFLOW;
+        result = ERROR_BUFFER_OVERFLOW;
+    } else {
+        std::memcpy(data, sub->event.data, sub->event.data_len);
     }
-    std::memcpy(data, sub->event.data, sub->event.data_len);
+    mutex_unlock(&poll_subscriptions_mutex.handle);
-    return ERROR_NONE;
+    return result;
 }
 
 } // extern "C"

Tests/TactilityKernel/Source/PreferencesTest.cpp

@@ -3,6 +3,8 @@
 
 #include <cstdio>
 #include <cstring>
+#include <sys/stat.h>
+#include <unistd.h>
 
 namespace {
 
@@ -22,6 +24,19 @@ bool file_exists(const char* path) {
     return true;
 }
 
+bool is_directory(const char* path) {
+    struct stat info {};
+    return stat(path, &info) == 0 && (info.st_mode & S_IFMT) == S_IFDIR;
+}
+
+// Writes a raw properties file directly (bypassing preferences_put_*()) so a test can exercise
+// a hand-crafted/corrupted payload that preferences_put_*() itself would never produce.
+void write_raw(const char* path, const char* content) {
+    FILE* file = std::fopen(path, "w");
+    std::fputs(content, file);
+    std::fclose(file);
+}
+
 } // namespace
 
 TEST_CASE("preferences_open_path on a missing file starts out empty, without creating it") {
@@ -98,6 +113,47 @@ TEST_CASE("has_*/opt_* reject a key stored with a different type") {
     preferences_close(preferences);
 }
 
+TEST_CASE("has_*/opt_* reject malformed scalar payloads instead of misparsing them") {
+    ScratchFile scratch;
+    write_raw(TEST_PATH,
+        "bad_bool=b:garbage\n"
+        "bad_bool_2=b:2\n"
+        "trailing_junk=i32:42abc\n"
+        "int32_overflow=i32:5000000000\n"
+        "int64_overflow=i64:99999999999999999999\n"
+        "empty_int=i32:\n");
+
+    Preferences* preferences = preferences_open(TEST_PATH);
+
+    // "b:garbage" must not silently read back as false - has_bool()/opt_bool() must agree it's
+    // not a valid bool at all.
+    CHECK_FALSE(preferences_has_bool(preferences, "bad_bool"));
+    bool bool_out = true;
+    CHECK_FALSE(preferences_opt_bool(preferences, "bad_bool", &bool_out));
+    CHECK_FALSE(preferences_has_bool(preferences, "bad_bool_2"));
+    CHECK_FALSE(preferences_opt_bool(preferences, "bad_bool_2", &bool_out));
+
+    // "42abc" must not silently parse as 42 - the full payload must be consumed.
+    CHECK_FALSE(preferences_has_int32(preferences, "trailing_junk"));
+    int32_t int32_out = 0;
+    CHECK_FALSE(preferences_opt_int32(preferences, "trailing_junk", &int32_out));
+
+    // Fits in a (64-bit, on this platform) `long` but overflows int32_t - must not silently
+    // truncate on the narrowing cast.
+    CHECK_FALSE(preferences_has_int32(preferences, "int32_overflow"));
+    CHECK_FALSE(preferences_opt_int32(preferences, "int32_overflow", &int32_out));
+
+    // Overflows even a 64-bit integer - strtoll() itself reports ERANGE.
+    CHECK_FALSE(preferences_has_int64(preferences, "int64_overflow"));
+    int64_t int64_out = 0;
+    CHECK_FALSE(preferences_opt_int64(preferences, "int64_overflow", &int64_out));
+
+    CHECK_FALSE(preferences_has_int32(preferences, "empty_int"));
+    CHECK_FALSE(preferences_opt_int32(preferences, "empty_int", &int32_out));
+
+    preferences_close(preferences);
+}
+
 TEST_CASE("a string value with embedded newlines and backslashes survives a reopen") {
     ScratchFile scratch;
 
@@ -151,3 +207,37 @@ TEST_CASE("put_* on an already-closed value is visible without reopening") {
     CHECK_EQ(out, 2);
     preferences_close(final_instance);
 }
+
+TEST_CASE("preferences_open creates missing parent directories (recursively) and persists into them") {
+    const char* nested_dir_a = "/tmp/tactility_kernel_preferences_test_nested";
+    const char* nested_dir_b = "/tmp/tactility_kernel_preferences_test_nested/a";
+    const char* nested_dir_c = "/tmp/tactility_kernel_preferences_test_nested/a/b";
+    const char* nested_path = "/tmp/tactility_kernel_preferences_test_nested/a/b/settings.properties";
+
+    std::remove(nested_path);
+    rmdir(nested_dir_c);
+    rmdir(nested_dir_b);
+    rmdir(nested_dir_a);
+    REQUIRE_FALSE(is_directory(nested_dir_a));
+
+    Preferences* preferences = preferences_open(nested_path);
+    REQUIRE_NE(preferences, nullptr);
+    CHECK(is_directory(nested_dir_a));
+    CHECK(is_directory(nested_dir_b));
+    CHECK(is_directory(nested_dir_c));
+
+    preferences_put_int32(preferences, "count", 7);
+    preferences_close(preferences);
+    CHECK(file_exists(nested_path));
+
+    Preferences* reopened = preferences_open(nested_path);
+    int32_t out = 0;
+    CHECK(preferences_opt_int32(reopened, "count", &out));
+    CHECK_EQ(out, 7);
+    preferences_close(reopened);
+
+    std::remove(nested_path);
+    rmdir(nested_dir_c);
+    rmdir(nested_dir_b);
+    rmdir(nested_dir_a);
+}

Tests/TactilityKernel/Source/PropertiesFileTest.cpp

@@ -4,6 +4,8 @@
 #include <cstdio>
 #include <cstring>
 #include <string>
+#include <sys/stat.h>
+#include <unistd.h>
 #include <utility>
 #include <vector>
 
@@ -44,6 +46,19 @@ TEST_CASE("properties_file_open on a missing file starts out empty, without crea
     properties_file_close(file);
 }
 
+TEST_CASE("properties_file_open returns NULL when a genuine I/O error interrupts reading") {
+    // fopen() on a directory succeeds on Linux, but the first read fails with EISDIR and sets
+    // the stream's error indicator - a deterministic way to exercise load_from_file()'s
+    // ferror() check without needing real storage-hardware fault injection.
+    const char* dir_path = "/tmp/tactility_kernel_properties_file_test_is_a_directory";
+    rmdir(dir_path);
+    REQUIRE_EQ(mkdir(dir_path, 0777), 0);
+
+    CHECK_EQ(properties_file_open(dir_path), nullptr);
+
+    rmdir(dir_path);
+}
+
 TEST_CASE("set/has/get round-trip, and close persists while unclosed changes don't") {
     ScratchFile scratch;
 
@@ -154,6 +169,63 @@ TEST_CASE("a [section] line prefixes every following key until the next section"
     properties_file_close(file);
 }
 
+TEST_CASE("properties_file_close reports ERROR_NONE on success") {
+    ScratchFile scratch;
+
+    PropertiesFile* file = properties_file_open(TEST_PATH);
+    properties_file_set(file, "key", "value");
+
+    CHECK_EQ(properties_file_close(file), ERROR_NONE);
+}
+
+TEST_CASE("properties_file_close reports ERROR_RESOURCE when the parent directory doesn't exist") {
+    const char* path = "/tmp/tactility_kernel_properties_file_test_missing_dir/settings.properties";
+    std::remove(path); // no-op if the directory doesn't exist, which is the point of this test
+
+    // Missing directory is not an error for open() - it starts out empty, same as a missing
+    // file (see the "starts out empty" test above).
+    PropertiesFile* file = properties_file_open(path);
+    REQUIRE_NE(file, nullptr);
+    properties_file_set(file, "key", "value");
+
+    // close()'s save can't create its temp file in a directory that doesn't exist.
+    CHECK_EQ(properties_file_close(file), ERROR_RESOURCE);
+}
+
+TEST_CASE("a failed close leaves previously-saved content on disk untouched") {
+    const char* dir = "/tmp/tactility_kernel_properties_file_readonly_test";
+    const char* path = "/tmp/tactility_kernel_properties_file_readonly_test/settings.properties";
+
+    mkdir(dir, 0777);
+    chmod(dir, 0777);
+    std::remove(path);
+
+    {
+        PropertiesFile* file = properties_file_open(path);
+        properties_file_set(file, "key", "original");
+        REQUIRE_EQ(properties_file_close(file), ERROR_NONE);
+    }
+
+    // Read-only directory - save_to_file()'s temp file can't be created there, so the close
+    // below must fail without disturbing the "original" content already on disk.
+    REQUIRE_EQ(chmod(dir, 0555), 0);
+
+    PropertiesFile* file = properties_file_open(path);
+    properties_file_set(file, "key", "corrupted");
+    CHECK_EQ(properties_file_close(file), ERROR_RESOURCE);
+
+    chmod(dir, 0777); // restore write access for the check below and for cleanup
+
+    PropertiesFile* reloaded = properties_file_open(path);
+    char buffer[32];
+    CHECK_EQ(properties_file_get(reloaded, "key", buffer, sizeof(buffer)), ERROR_NONE);
+    CHECK_EQ(std::strcmp(buffer, "original"), 0);
+    properties_file_close(reloaded);
+
+    std::remove(path);
+    rmdir(dir);
+}
+
 TEST_CASE("properties_file_for_each visits every key exactly once") {
     ScratchFile scratch;
 

Tests/TactilityKernel/Source/SystemEventTest.cpp

@@ -257,6 +257,31 @@ TEST_CASE("system_event_subscribe/_await deliver the event payload by value") {
     CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NOT_FOUND);
 }
 
+TEST_CASE("system_event_await returns a matching event that arrived before it started waiting") {
+    SystemEventSubscription sub {};
+    sub.event.type = KERNEL_EVENT_NETWORK_CONNECTED;
+    CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE);
+
+    // Same-thread emit, no background thread needed: unlike the "detects a change after it
+    // starts waiting" tests above, this is exactly the case system_event_await() must handle -
+    // sequence already moved ahead of consumed_sequence before await() is even called.
+    NetworkConnectedEvent connected { .device = nullptr, .ipv4_addr = 0x0A000001, .gateway = 0x0A0000FE };
+    CHECK_EQ(system_event_emit(KERNEL_EVENT_NETWORK_CONNECTED, &connected, sizeof(connected)), ERROR_NONE);
+
+    CHECK_EQ(system_event_await(&sub, 0), ERROR_NONE);
+
+    NetworkConnectedEvent received {};
+    CHECK_EQ(system_event_get_data(&sub, reinterpret_cast<uint8_t*>(&received), sizeof(received)), ERROR_NONE);
+    CHECK_EQ(received.ipv4_addr, connected.ipv4_addr);
+    CHECK_EQ(received.gateway, connected.gateway);
+
+    // The pending event was consumed by the call above - a second await() with no further
+    // emit must time out rather than returning the same event again.
+    CHECK_EQ(system_event_await(&sub, 0), ERROR_TIMEOUT);
+
+    system_event_unsubscribe(&sub);
+}
+
 TEST_CASE("system_event_await times out when no matching event has arrived") {
     SystemEventSubscription sub {};
     sub.event.type = KERNEL_EVENT_TIME_CHANGED;
@@ -341,3 +366,74 @@ TEST_CASE("system_event_get_data on a subscription with no payload copies nothin
 
     system_event_unsubscribe(&sub);
 }
+
+// Regression coverage for system_event_unsubscribe() racing a task blocked in
+// system_event_await() on the same subscription, and for reusing a subscription node after
+// unsubscribing it - see the @warning on system_event_unsubscribe() in system_event.h.
+
+TEST_CASE("system_event_unsubscribe wakes a task blocked in system_event_await with ERROR_INVALID_STATE") {
+    SystemEventSubscription sub {};
+    sub.event.type = KERNEL_EVENT_SERVICE_STARTED;
+    CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE);
+
+    auto* thread = thread_alloc_full(
+        "system-event-awaiter",
+        4096,
+        [](void* context) {
+            auto* awaited_sub = static_cast<SystemEventSubscription*>(context);
+            // Long timeout - the point is that unsubscribe() wakes this early, not that it
+            // eventually times out on its own.
+            return static_cast<int32_t>(system_event_await(awaited_sub, pdMS_TO_TICKS(5000)));
+        },
+        &sub,
+        -1
+    );
+    CHECK_EQ(thread_start(thread), ERROR_NONE);
+
+    // Give the awaiter task a moment to actually reach xSemaphoreTake() before unsubscribing -
+    // otherwise this test wouldn't exercise the "already blocked" race at all.
+    delay_millis(20);
+
+    // Must return promptly (nudging the blocked awaiter awake), not by waiting out its timeout.
+    TickType_t before = get_ticks();
+    CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE);
+    CHECK_LT(get_ticks() - before, pdMS_TO_TICKS(1000));
+
+    CHECK_EQ(thread_join(thread, 2, 1), ERROR_NONE);
+    CHECK_EQ(thread_get_return_code(thread), ERROR_INVALID_STATE);
+    thread_free(thread);
+
+    // A second unsubscribe() has nothing left to do.
+    CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NOT_FOUND);
+}
+
+TEST_CASE("a subscription node can be re-subscribed after system_event_unsubscribe") {
+    SystemEventSubscription sub {};
+    sub.event.type = KERNEL_EVENT_SERVICE_STOPPED;
+
+    CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE);
+    CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE);
+
+    // Re-registering the same node (same storage, not a fresh SystemEventSubscription) must
+    // work as if it were new - a fresh semaphore, and no leftover `cancelled` state from the
+    // unsubscribe() above causing an immediate spurious ERROR_INVALID_STATE below.
+    CHECK_EQ(system_event_subscribe(&sub), ERROR_NONE);
+
+    auto* thread = thread_alloc_full(
+        "system-event-emitter",
+        4096,
+        [](void*) {
+            delay_millis(20);
+            system_event_emit(KERNEL_EVENT_SERVICE_STOPPED, nullptr, 0);
+            return 0;
+        },
+        nullptr,
+        -1
+    );
+    CHECK_EQ(thread_start(thread), ERROR_NONE);
+    CHECK_EQ(system_event_await(&sub, pdMS_TO_TICKS(2000)), ERROR_NONE);
+    CHECK_EQ(thread_join(thread, 2, 1), ERROR_NONE);
+    thread_free(thread);
+
+    CHECK_EQ(system_event_unsubscribe(&sub), ERROR_NONE);
+}