Fix module logic, updated tests, add tests (#584)

TactilityKernel/include/tactility/module.h

@@ -124,7 +124,7 @@ error_t module_construct_add_start(struct Module* module);
 
 /**
  * @brief Tries to ensure the module is in a started state.
- * Calls module_construct if needed, calls module_start if needed.
+ * Calls module_construct if needed, calls module_add if needed, calls module_start if needed.
  * @param module the module
  * @return ERROR_NONE if module is in a started state
  */
@@ -132,7 +132,7 @@ error_t module_ensure_started(struct Module* module);
 
 /**
  * @brief Tries to ensure the module is in a started state.
- * Calls module_stop if needed, calls module_destruct if needed.
+ * Calls module_stop if needed, calls module_remove, calls module_destruct if needed.
  * @param module the module
  * @return ERROR_NONE if module is in a destructed state
  */

TactilityKernel/source/module.cpp

@@ -146,6 +146,9 @@ error_t module_ensure_started(Module* module) {
         if (result != ERROR_NONE) { return result; }
     }
 
+    error_t add_result = module_add(module);
+    if (add_result != ERROR_NONE && add_result != ERROR_INVALID_STATE) { return add_result; }
+
     if (!module->internal->started) {
         error_t result = module_start(module);
         if (result != ERROR_NONE) { return result; }
@@ -162,6 +165,9 @@ error_t module_ensure_destructed(Module* module) {
             if (result != ERROR_NONE) { return result; }
         }
 
+        result = module_remove(module);
+        if (result != ERROR_NONE) { return result; }
+
         result = module_destruct(module);
         if (result != ERROR_NONE) { return result; }
     }

Tests/TactilityKernel/Source/FileMutexTest.cpp

@@ -0,0 +1,122 @@
+#include "doctest.h"
+#include <tactility/filesystem/file_mutex.h>
+
+namespace {
+
+int lock_calls = 0;
+int unlock_calls = 0;
+int try_lock_calls = 0;
+bool try_lock_result = true;
+uint32_t try_lock_timeout_seen = 0;
+
+void mock_lock() { lock_calls++; }
+void mock_unlock() { unlock_calls++; }
+bool mock_try_lock(uint32_t timeout) {
+    try_lock_calls++;
+    try_lock_timeout_seen = timeout;
+    return try_lock_result;
+}
+
+int lock_a_calls = 0;
+int lock_b_calls = 0;
+void mock_lock_a() { lock_a_calls++; }
+void mock_lock_b() { lock_b_calls++; }
+
+void reset_mocks() {
+    lock_calls = 0;
+    unlock_calls = 0;
+    try_lock_calls = 0;
+    try_lock_result = true;
+    try_lock_timeout_seen = 0;
+    lock_a_calls = 0;
+    lock_b_calls = 0;
+}
+
+} // namespace
+
+TEST_CASE("file_mutex_get with zero registrations returns a no-op mutex") {
+    FileMutex mutex;
+    file_mutex_get(&mutex, "/nowhere/file.txt");
+
+    CHECK_EQ(mutex.lock, nullptr);
+    CHECK_EQ(mutex.try_lock, nullptr);
+    CHECK_EQ(mutex.unlock, nullptr);
+
+    // Calling through a no-op mutex must be safe, and try_lock must report success.
+    file_mutex_lock(&mutex);
+    CHECK_EQ(file_mutex_try_lock(&mutex, 123), true);
+    file_mutex_unlock(&mutex);
+}
+
+TEST_CASE("file_mutex_register/get with a single registration") {
+    reset_mocks();
+    FileMutex registered = { .lock = mock_lock, .try_lock = mock_try_lock, .unlock = mock_unlock };
+    file_mutex_register(&registered, "/mock1");
+
+    FileMutex mutex;
+
+    // Exact mount path match.
+    file_mutex_get(&mutex, "/mock1");
+    CHECK_EQ(mutex.lock, mock_lock);
+    CHECK_EQ(mutex.try_lock, mock_try_lock);
+    CHECK_EQ(mutex.unlock, mock_unlock);
+
+    // Descendant path match.
+    file_mutex_get(&mutex, "/mock1/nested/file.txt");
+    CHECK_EQ(mutex.lock, mock_lock);
+
+    // Unrelated path falls back to no-op.
+    FileMutex unrelated;
+    file_mutex_get(&unrelated, "/other/file.txt");
+    CHECK_EQ(unrelated.lock, nullptr);
+
+    // Prefix-but-not-descendant path (e.g. "/mock1x") must not match "/mock1".
+    FileMutex prefix_only;
+    file_mutex_get(&prefix_only, "/mock1x/file.txt");
+    CHECK_EQ(prefix_only.lock, nullptr);
+
+    // Exercise the resolved callbacks.
+    file_mutex_get(&mutex, "/mock1");
+    file_mutex_lock(&mutex);
+    CHECK_EQ(lock_calls, 1);
+    CHECK_EQ(file_mutex_try_lock(&mutex, 42), true);
+    CHECK_EQ(try_lock_calls, 1);
+    CHECK_EQ(try_lock_timeout_seen, 42);
+    file_mutex_unlock(&mutex);
+    CHECK_EQ(unlock_calls, 1);
+
+    // Re-registering the same path is a no-op: original callbacks remain in place.
+    FileMutex replacement = { .lock = nullptr, .try_lock = nullptr, .unlock = nullptr };
+    file_mutex_register(&replacement, "/mock1");
+    file_mutex_get(&mutex, "/mock1");
+    CHECK_EQ(mutex.lock, mock_lock);
+}
+
+TEST_CASE("file_mutex_register/get with two registrations resolves to the matching path") {
+    reset_mocks();
+
+    FileMutex mutex_a = { .lock = mock_lock_a, .try_lock = nullptr, .unlock = nullptr };
+    FileMutex mutex_b = { .lock = mock_lock_b, .try_lock = nullptr, .unlock = nullptr };
+
+    file_mutex_register(&mutex_a, "/mock2a");
+    file_mutex_register(&mutex_b, "/mock2b");
+
+    FileMutex resolved;
+
+    file_mutex_get(&resolved, "/mock2a/file.txt");
+    CHECK_EQ(resolved.lock, mock_lock_a);
+
+    file_mutex_get(&resolved, "/mock2b/file.txt");
+    CHECK_EQ(resolved.lock, mock_lock_b);
+
+    // Path matching neither registration falls back to no-op.
+    file_mutex_get(&resolved, "/mock2c/file.txt");
+    CHECK_EQ(resolved.lock, nullptr);
+
+    // Registration order matters: the first matching entry wins, not the longest
+    // prefix. A mount nested under an earlier one is shadowed by it.
+    FileMutex mutex_nested = { .lock = nullptr, .try_lock = nullptr, .unlock = nullptr };
+    file_mutex_register(&mutex_nested, "/mock2a/nested");
+    file_mutex_get(&resolved, "/mock2a/nested/file.txt");
+    CHECK_EQ(resolved.lock, mock_lock_a); // still /mock2a, registered first
+}

Tests/TactilityKernel/Source/ModuleTest.cpp

@@ -5,8 +5,14 @@ static void symbol_test_function() { /* NO-OP */ }
 
 static error_t test_start_result = ERROR_NONE;
 static bool start_called = false;
+static struct Module* start_add_order_check_module = nullptr;
 static error_t test_start() {
     start_called = true;
+    if (start_add_order_check_module != nullptr) {
+        // If the module was already added to the ledger before start() runs,
+        // a duplicate module_add() must report that it already exists.
+        CHECK_EQ(module_add(start_add_order_check_module), ERROR_INVALID_STATE);
+    }
     return test_start_result;
 }
 
@@ -139,3 +145,91 @@ TEST_CASE("Global symbol resolution") {
 
     CHECK_EQ(module_destruct(&module), ERROR_NONE);
 }
+
+TEST_CASE("module_ensure_started adds module to global ledger") {
+    start_called = false;
+    stop_called = false;
+    test_start_result = ERROR_NONE;
+    test_stop_result = ERROR_NONE;
+
+    static const struct ModuleSymbol test_symbols[] = {
+        DEFINE_MODULE_SYMBOL(symbol_test_function),
+        MODULE_SYMBOL_TERMINATOR
+    };
+
+    struct Module module = {
+        .name = "test_ensure_started",
+        .start = test_start,
+        .stop = test_stop,
+        .symbols = test_symbols,
+        .internal = nullptr
+    };
+
+    uintptr_t addr;
+    // Not resolvable before module_ensure_started is called
+    CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), false);
+
+    // test_start() asserts module_add(&module) is already ERROR_INVALID_STATE by the
+    // time start() runs, proving module_add happens before module_start (not after).
+    start_add_order_check_module = &module;
+    CHECK_EQ(module_ensure_started(&module), ERROR_NONE);
+    start_add_order_check_module = nullptr;
+    CHECK_EQ(module_is_started(&module), true);
+    CHECK_EQ(start_called, true);
+
+    // Module must be both added to the ledger and started to be resolvable
+    CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true);
+
+    // Calling again should be idempotent: no duplicate start, still resolvable
+    start_called = false;
+    CHECK_EQ(module_ensure_started(&module), ERROR_NONE);
+    CHECK_EQ(start_called, false);
+    CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true);
+
+    // Cleanup
+    CHECK_EQ(module_stop(&module), ERROR_NONE);
+    CHECK_EQ(module_remove(&module), ERROR_NONE);
+    CHECK_EQ(module_destruct(&module), ERROR_NONE);
+}
+
+TEST_CASE("module_ensure_destructed removes module from global ledger") {
+    start_called = false;
+    stop_called = false;
+    test_start_result = ERROR_NONE;
+    test_stop_result = ERROR_NONE;
+
+    static const struct ModuleSymbol test_symbols[] = {
+        DEFINE_MODULE_SYMBOL(symbol_test_function),
+        MODULE_SYMBOL_TERMINATOR
+    };
+
+    struct Module module = {
+        .name = "test_ensure_destructed",
+        .start = test_start,
+        .stop = test_stop,
+        .symbols = test_symbols,
+        .internal = nullptr
+    };
+
+    CHECK_EQ(module_ensure_started(&module), ERROR_NONE);
+
+    uintptr_t addr;
+    CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), true);
+
+    CHECK_EQ(module_ensure_destructed(&module), ERROR_NONE);
+    CHECK_EQ(module_is_started(&module), false);
+    CHECK_EQ(stop_called, true);
+
+    // Module must no longer be resolvable once destructed. Note: this alone doesn't
+    // prove removal from the ledger, since module_resolve_symbol_global() also skips
+    // non-started modules — a leaked-but-stopped ledger entry would look the same.
+    CHECK_EQ(module_resolve_symbol_global("symbol_test_function", &addr), false);
+
+    // Directly prove detachment from the ledger: if module_ensure_destructed had left
+    // the module in place, this module_add() would return ERROR_INVALID_STATE.
+    CHECK_EQ(module_add(&module), ERROR_NONE);
+    CHECK_EQ(module_remove(&module), ERROR_NONE);
+
+    // Calling again on an already-destructed module should be a no-op
+    CHECK_EQ(module_ensure_destructed(&module), ERROR_NONE);
+}