incorporate thread-safe plugin management (#103)

* incorporate thread-safe plugin management

* copilot comments

* docs

CMakeLists.txt

@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.14)
 project(SimplePluginExample LANGUAGES CXX)
 
-set(CMAKE_CXX_STANDARD 11)
+set(CMAKE_CXX_STANDARD 17)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 
 # Find Qt packages

README.md

@@ -134,6 +134,12 @@ void logos_core_process_events();
 
 See `src/logos_core/logos_core.h` for the full API.
 
+### Thread safety
+
+All C API functions that mutate plugin state (`logos_core_load_plugin`, `logos_core_load_plugin_with_dependencies`, `logos_core_unload_plugin`, `logos_core_refresh_plugins`) are serialised internally by a single mutex. It is safe to call them concurrently from multiple threads, including rapid and repeated load/unload cycles on the same module — each call waits for its turn and the process management layer handles teardown cleanly before the next launch.
+
+Read-only accessors (`logos_core_get_known_plugins`, `logos_core_get_loaded_plugins`) use a shared reader-writer lock and are safe to call concurrently with each other and with the mutating functions above.
+
 ## Dev vs Portable Builds
 
 The library supports two build modes controlled by the `LOGOS_PORTABLE_BUILD` CMake flag:

docs/project.md

@@ -98,6 +98,8 @@ logos-liblogos/
 
 **Purpose:** Thin facade that orchestrates `PluginRegistry`, `PluginLauncher`, and `DependencyResolver`. Provides the C++-level API for module lifecycle management. Each module runs in a separate `logos_host` process for isolation.
 
+**Thread safety:** `loadPlugin`, `loadPluginWithDependencies`, and `unloadPlugin` are serialised by a static `loadMutex()` (one load/unload at a time). `discoverInstalledModules` delegates to `PluginRegistry` which has its own reader-writer lock.
+
 **API (namespace `PluginManager`):**
 
 | Method | Description |
@@ -123,12 +125,13 @@ logos-liblogos/
 
 **Files:** `src/logos_core/plugin_registry.h`, `src/logos_core/plugin_registry.cpp`
 
-**Purpose:** In-memory registry of discovered and loaded modules. Stores plugin paths, dependencies, and load state.
+**Purpose:** In-memory registry of discovered and loaded modules. Stores plugin paths, dependencies, and load state. All public methods are thread-safe: mutating methods acquire a `std::unique_lock` on an internal `std::shared_mutex`; read-only methods acquire a `std::shared_lock`, allowing concurrent reads.
 
 **Data:**
 - `PluginInfo` struct — holds `path`, `dependencies` (QStringList), `loaded` flag
 - `QHash<QString, PluginInfo> m_plugins` — plugin database keyed by name
 - `QStringList m_pluginsDirs` — configured plugin directories
+- `std::shared_mutex m_mutex` — reader-writer lock protecting all fields
 
 **API (class `PluginRegistry`):**
 
@@ -189,7 +192,7 @@ Takes callback functions (`IsKnownFn`, `GetDependenciesFn`) so it has no couplin
 **Purpose:** Isolates all Qt-specific code behind internal interfaces.
 
 - **QtAppContext** — Creates/manages `QCoreApplication`, runs event loop, processes events
-- **QtProcessManager** — Manages `QProcess` instances for module subprocesses, handles exit/error signals, sends tokens via stdin
+- **QtProcessManager** — Manages `QProcess` instances for module subprocesses, handles exit/error signals, sends tokens via local socket. A `std::mutex` (`s_processesMutex`) protects the `s_processes` map against concurrent access from calling threads and async signal deliveries. All teardown paths use a shared `destroyProcess` helper that disconnects signals before waiting, preventing double-free from deferred `deleteLater` calls.
 
 ### ProcessStats (external dependency)
 
@@ -245,6 +248,15 @@ The public C API (`logos_core.h`) is the only exported interface. All functions
 | `logos_core_get_module_stats() → char*` | JSON array of CPU/memory stats (caller frees) |
 | `logos_core_get_token(key) → char*` | Get auth token by key (caller frees) |
 
+### Thread Safety
+
+| Category | Guarantee |
+|----------|-----------|
+| `logos_core_load_plugin`, `logos_core_load_plugin_with_dependencies`, `logos_core_unload_plugin` | Serialised by a single internal mutex — safe to call concurrently from multiple threads |
+| `logos_core_get_known_plugins`, `logos_core_get_loaded_plugins` | Protected by a shared reader-writer lock — safe to call concurrently with each other and with the mutating functions above |
+| `logos_core_refresh_plugins` | Protected by `PluginRegistry`'s reader-writer lock (write side) — safe for concurrent registry access but not serialised against load/unload |
+| `logos_core_init`, `logos_core_start`, `logos_core_cleanup` | Not thread-safe — must be called from a single thread during startup/shutdown |
+
 ## Build Artifacts
 
 | Artifact | Description |

docs/spec.md

@@ -137,6 +137,15 @@ Every module ships a `metadata.json` referenced by Qt's `Q_PLUGIN_METADATA` macr
 - Core Manager process is excluded from stats
 - Not available on iOS
 
+### Thread Safety
+
+The C API is designed to be safe for use from multi-threaded host applications:
+
+- **Load/unload operations** (`load_plugin`, `load_plugin_with_dependencies`, `unload_plugin`) are serialised — only one runs at a time, so rapid concurrent load/unload cycles on the same or different modules do not produce data races.
+- **Read-only queries** (`get_known_plugins`, `get_loaded_plugins`) use a shared reader-writer lock and may execute concurrently with each other and with load/unload operations.
+- **Plugin discovery** (`refresh_plugins`) is protected by the registry's own write lock.
+- **Lifecycle functions** (`init`, `start`, `cleanup`) are not thread-safe and must be called from a single thread.
+
 ### Dev vs Portable Builds
 
 The platform supports two build variants:

src/logos_core/app_lifecycle.cpp

@@ -21,7 +21,7 @@ namespace AppLifecycle {
     }
 
     void cleanup() {
-        PluginManager::terminateAll();
+        PluginManager::clear();
         QtAppContext::cleanup();
     }
 

src/logos_core/plugin_manager.cpp

@@ -4,6 +4,7 @@
 #include "plugin_launcher.h"
 #include <QDebug>
 #include <QUuid>
+#include <mutex>
 #include <cassert>
 #include <cstring>
 #include "logos_api.h"
@@ -16,6 +17,11 @@ namespace {
         return instance;
     }
 
+    std::mutex& loadMutex() {
+        static std::mutex mutex;
+        return mutex;
+    }
+
     char** toNullTerminatedArray(const QStringList& list) {
         int count = list.size();
         if (count == 0) {
@@ -50,6 +56,44 @@ namespace {
             qWarning() << "Failed to register token with capability module for:" << name;
         }
     }
+
+    bool loadPluginInternal(const char* pluginName) {
+        QString name = QString::fromUtf8(pluginName);
+
+        if (!registryInstance().isKnown(name)) {
+            qWarning() << "Cannot load unknown plugin:" << name;
+            return false;
+        }
+
+        if (registryInstance().isLoaded(name)) {
+            qWarning() << "Plugin already loaded:" << name;
+            return false;
+        }
+
+        QString pluginPath = registryInstance().pluginPath(name);
+
+        auto onTerminated = [](const QString& n) {
+            registryInstance().markUnloaded(n);
+        };
+
+        if (!PluginLauncher::launch(name, pluginPath, registryInstance().pluginsDirs(), onTerminated))
+            return false;
+
+        QString authToken = QUuid::createUuid().toString(QUuid::WithoutBraces);
+
+        if (!PluginLauncher::sendToken(name, authToken))
+            return false;
+
+        registryInstance().markLoaded(name);
+
+        TokenManager::instance().saveToken(name, authToken);
+
+        notifyCapabilityModule(name, authToken);
+
+        qInfo() << "Plugin loaded:" << name;
+
+        return true;
+    }
 }
 
 namespace PluginManager {
@@ -92,44 +136,13 @@ namespace PluginManager {
     }
 
     bool loadPlugin(const char* pluginName) {
-        QString name = QString::fromUtf8(pluginName);
-
-        if (!registryInstance().isKnown(name)) {
-            qWarning() << "Cannot load unknown plugin:" << name;
-            return false;
-        }
-
-        if (registryInstance().isLoaded(name)) {
-            qWarning() << "Plugin already loaded:" << name;
-            return false;
-        }
-
-        QString pluginPath = registryInstance().pluginPath(name);
-
-        auto onTerminated = [](const QString& n) {
-            registryInstance().markUnloaded(n);
-        };
-
-        if (!PluginLauncher::launch(name, pluginPath, registryInstance().pluginsDirs(), onTerminated))
-            return false;
-
-        QString authToken = QUuid::createUuid().toString(QUuid::WithoutBraces);
-
-        if (!PluginLauncher::sendToken(name, authToken))
-            return false;
-
-        registryInstance().markLoaded(name);
-
-        TokenManager::instance().saveToken(name, authToken);
-
-        notifyCapabilityModule(name, authToken);
-
-        qInfo() << "Plugin loaded:" << name;
-
-        return true;
+        std::lock_guard lock(loadMutex());
+        return loadPluginInternal(pluginName);
     }
 
     bool loadPluginWithDependencies(const char* pluginName) {
+        std::lock_guard lock(loadMutex());
+
         QString name = QString::fromUtf8(pluginName);
 
         QStringList requested;
@@ -150,7 +163,7 @@ namespace PluginManager {
         for (const QString& moduleName : resolved) {
             if (registryInstance().isLoaded(moduleName))
                 continue;
-            if (!loadPlugin(moduleName.toUtf8().constData())) {
+            if (!loadPluginInternal(moduleName.toUtf8().constData())) {
                 qWarning() << "Failed to load plugin:" << moduleName;
                 allSucceeded = false;
             }
@@ -160,10 +173,12 @@ namespace PluginManager {
     }
 
     bool initializeCapabilityModule() {
+        std::lock_guard lock(loadMutex());
+
         if (!registryInstance().isKnown("capability_module"))
             return false;
 
-        if (!loadPlugin("capability_module")) {
+        if (!loadPluginInternal("capability_module")) {
             qWarning() << "Failed to load capability module";
             return false;
         }
@@ -172,6 +187,8 @@ namespace PluginManager {
     }
 
     bool unloadPlugin(const char* pluginName) {
+        std::lock_guard lock(loadMutex());
+
         QString name = QString::fromUtf8(pluginName);
 
         if (!registryInstance().isLoaded(name)) {
@@ -192,10 +209,17 @@ namespace PluginManager {
     }
 
     void terminateAll() {
+        std::lock_guard lock(loadMutex());
         PluginLauncher::terminateAll();
         registryInstance().clearLoaded();
     }
 
+    void clear() {
+        std::lock_guard lock(loadMutex());
+        PluginLauncher::terminateAll();
+        registryInstance().clear();
+    }
+
     char** getLoadedPluginsCStr() {
         return toNullTerminatedArray(registryInstance().loadedPluginNames());
     }

src/logos_core/plugin_manager.h

@@ -22,6 +22,7 @@ namespace PluginManager {
     bool initializeCapabilityModule();
     bool unloadPlugin(const char* pluginName);
     void terminateAll();
+    void clear();
 
     char** getLoadedPluginsCStr();
     char** getKnownPluginsCStr();

src/logos_core/plugin_registry.cpp

@@ -2,6 +2,8 @@
 #include <QDebug>
 #include <QDir>
 #include <cassert>
+#include <mutex>
+#include <shared_mutex>
 #include <nlohmann/json.hpp>
 #include <module_lib/module_lib.h>
 #include <package_manager_lib.h>
@@ -12,20 +14,25 @@ static PackageManagerLib& packageManagerInstance() {
 }
 
 void PluginRegistry::setPluginsDir(const QString& dir) {
+    std::unique_lock lock(m_mutex);
     m_pluginsDirs.clear();
     m_pluginsDirs.append(dir);
 }
 
 void PluginRegistry::addPluginsDir(const QString& dir) {
+    std::unique_lock lock(m_mutex);
     if (m_pluginsDirs.contains(dir)) return;
     m_pluginsDirs.append(dir);
 }
 
 QStringList PluginRegistry::pluginsDirs() const {
+    std::shared_lock lock(m_mutex);
     return m_pluginsDirs;
 }
 
 void PluginRegistry::discoverInstalledModules() {
+    std::unique_lock lock(m_mutex);
+
     PackageManagerLib& pm = packageManagerInstance();
     if (!m_pluginsDirs.isEmpty()) {
         pm.setEmbeddedModulesDirectory(m_pluginsDirs.first().toStdString());
@@ -45,7 +52,7 @@ void PluginRegistry::discoverInstalledModules() {
         if (name.isEmpty() || mainFilePath.isEmpty())
             continue;
 
-        QString pluginName = processPlugin(mainFilePath);
+        QString pluginName = processPluginInternal(mainFilePath);
         if (pluginName.isEmpty()) {
             qWarning() << "Failed to process plugin:" << mainFilePath;
         }
@@ -53,6 +60,11 @@ void PluginRegistry::discoverInstalledModules() {
 }
 
 QString PluginRegistry::processPlugin(const QString& pluginPath) {
+    std::unique_lock lock(m_mutex);
+    return processPluginInternal(pluginPath);
+}
+
+QString PluginRegistry::processPluginInternal(const QString& pluginPath) {
     std::string name = ModuleLib::LogosModule::getModuleName(pluginPath.toStdString());
     if (name.empty()) {
         qWarning() << "No valid metadata for plugin:" << pluginPath;
@@ -72,23 +84,28 @@ QString PluginRegistry::processPlugin(const QString& pluginPath) {
 }
 
 bool PluginRegistry::isKnown(const QString& name) const {
+    std::shared_lock lock(m_mutex);
     return m_plugins.contains(name);
 }
 
 QString PluginRegistry::pluginPath(const QString& name) const {
+    std::shared_lock lock(m_mutex);
     return m_plugins.value(name).path;
 }
 
 QStringList PluginRegistry::pluginDependencies(const QString& name) const {
+    std::shared_lock lock(m_mutex);
     return m_plugins.value(name).dependencies;
 }
 
 QStringList PluginRegistry::knownPluginNames() const {
+    std::shared_lock lock(m_mutex);
     return m_plugins.keys();
 }
 
 void PluginRegistry::registerPlugin(const QString& name, const QString& path,
                                     const QStringList& dependencies) {
+    std::unique_lock lock(m_mutex);
     PluginInfo& info = m_plugins[name];
     info.path = path;
     if (!dependencies.isEmpty())
@@ -96,23 +113,28 @@ void PluginRegistry::registerPlugin(const QString& name, const QString& path,
 }
 
 void PluginRegistry::registerDependencies(const QString& name, const QStringList& dependencies) {
+    std::unique_lock lock(m_mutex);
     m_plugins[name].dependencies = dependencies;
 }
 
 bool PluginRegistry::isLoaded(const QString& name) const {
+    std::shared_lock lock(m_mutex);
     return m_plugins.value(name).loaded;
 }
 
 void PluginRegistry::markLoaded(const QString& name) {
+    std::unique_lock lock(m_mutex);
     m_plugins[name].loaded = true;
 }
 
 void PluginRegistry::markUnloaded(const QString& name) {
+    std::unique_lock lock(m_mutex);
     if (m_plugins.contains(name))
         m_plugins[name].loaded = false;
 }
 
 QStringList PluginRegistry::loadedPluginNames() const {
+    std::shared_lock lock(m_mutex);
     QStringList result;
     for (auto it = m_plugins.constBegin(); it != m_plugins.constEnd(); ++it) {
         if (it.value().loaded)
@@ -122,11 +144,13 @@ QStringList PluginRegistry::loadedPluginNames() const {
 }
 
 void PluginRegistry::clearLoaded() {
+    std::unique_lock lock(m_mutex);
     for (auto it = m_plugins.begin(); it != m_plugins.end(); ++it)
         it.value().loaded = false;
 }
 
 void PluginRegistry::clear() {
+    std::unique_lock lock(m_mutex);
     m_pluginsDirs.clear();
     m_plugins.clear();
 }

src/logos_core/plugin_registry.h

@@ -4,6 +4,7 @@
 #include <QString>
 #include <QStringList>
 #include <QHash>
+#include <shared_mutex>
 
 struct PluginInfo {
     QString path;
@@ -37,6 +38,9 @@ public:
     void clear();
 
 private:
+    QString processPluginInternal(const QString& pluginPath);
+
+    mutable std::shared_mutex m_mutex;
     QStringList m_pluginsDirs;
     QHash<QString, PluginInfo> m_plugins;
 };

src/logos_core/qt/qt_process_manager.cpp

@@ -4,12 +4,31 @@
 #include <QLocalSocket>
 #include <QThread>
 #include <QHash>
+#include <mutex>
 
 namespace {
     QHash<QString, QProcess*> s_processes;
+    std::mutex s_processesMutex;
 
     QString toQ(const std::string& s) { return QString::fromStdString(s); }
     std::string fromQ(const QString& s) { return s.toStdString(); }
+
+    // Tears down a process that has already been removed from s_processes.
+    // Must be called without s_processesMutex held (waitForFinished can block).
+    void destroyProcess(QProcess* process, const QString& name) {
+        // Disconnect all signal handlers before waiting. The finished-signal
+        // lambda calls process->deleteLater(), which would queue a deferred
+        // deletion on an object we are about to delete directly — causing a
+        // double-free when that deferred deletion fires later.
+        process->disconnect();
+        process->terminate();
+        if (!process->waitForFinished(5000)) {
+            qWarning() << "Process did not terminate gracefully, killing it:" << name;
+            process->kill();
+            process->waitForFinished(2000);
+        }
+        delete process;
+    }
 }
 
 namespace QtProcessManager {
@@ -33,13 +52,19 @@ namespace QtProcessManager {
             return false;
         }
 
-        s_processes.insert(qName, process);
+        {
+            std::lock_guard<std::mutex> lock(s_processesMutex);
+            s_processes.insert(qName, process);
+        }
 
         QObject::connect(process, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
                         [name, qName, process, callbacks](int exitCode, QProcess::ExitStatus exitStatus) {
                             bool crashed = (exitStatus == QProcess::CrashExit);
 
-                            s_processes.remove(qName);
+                            {
+                                std::lock_guard<std::mutex> lock(s_processesMutex);
+                                s_processes.remove(qName);
+                            }
                             process->deleteLater();
 
                             if (callbacks.onFinished) {
@@ -107,10 +132,12 @@ namespace QtProcessManager {
             qCritical() << "Failed to connect to token socket for:" << qName;
             tokenSocket->deleteLater();
 
-            if (s_processes.contains(qName)) {
-                s_processes.value(qName)->terminate();
-                delete s_processes.take(qName);
+            QProcess* p = nullptr;
+            {
+                std::lock_guard<std::mutex> lock(s_processesMutex);
+                p = s_processes.take(qName);
             }
+            if (p) destroyProcess(p, qName);
             return false;
         }
 
@@ -123,60 +150,48 @@ namespace QtProcessManager {
         return true;
     }
 
+
     void terminateProcess(const std::string& name) {
         QString qName = toQ(name);
-        if (!s_processes.contains(qName)) return;
-
-        QProcess* process = s_processes.take(qName);
-        if (!process) return;
-
-        process->terminate();
-
-        if (!process->waitForFinished(5000)) {
-            qWarning() << "Process did not terminate gracefully, killing it:" << qName;
-            process->kill();
-            process->waitForFinished(2000);
+        QProcess* process = nullptr;
+        {
+            std::lock_guard<std::mutex> lock(s_processesMutex);
+            process = s_processes.take(qName);
         }
-
-        delete process;
+        if (!process) return;
+        destroyProcess(process, qName);
     }
 
     void terminateAll() {
-        if (s_processes.isEmpty()) return;
-
-        // Copy and clear first to avoid iterator invalidation from the
-        // finished signal handler which removes entries from s_processes.
-        QHash<QString, QProcess*> snapshot = s_processes;
-        s_processes.clear();
+        QHash<QString, QProcess*> snapshot;
+        {
+            std::lock_guard<std::mutex> lock(s_processesMutex);
+            if (s_processes.isEmpty()) return;
+            // Swap out the whole map so the finished-signal lambda (which also
+            // locks s_processesMutex) cannot race with our teardown loop.
+            snapshot.swap(s_processes);
+        }
 
         for (auto it = snapshot.begin(); it != snapshot.end(); ++it) {
-            QProcess* process = it.value();
-            QString processName = it.key();
-
-            process->terminate();
-
-            if (!process->waitForFinished(3000)) {
-                qWarning() << "Process did not terminate gracefully, killing it:" << processName;
-                process->kill();
-                process->waitForFinished(1000);
-            }
-
-            delete process;
+            if (it.value())
+                destroyProcess(it.value(), it.key());
         }
     }
 
     bool hasProcess(const std::string& name) {
+        std::lock_guard<std::mutex> lock(s_processesMutex);
         return s_processes.contains(toQ(name));
     }
 
     int64_t getProcessId(const std::string& name) {
+        std::lock_guard<std::mutex> lock(s_processesMutex);
         QString qName = toQ(name);
-        if (!s_processes.contains(qName)) return -1;
         QProcess* process = s_processes.value(qName);
         return process ? process->processId() : -1;
     }
 
     std::unordered_map<std::string, int64_t> getAllProcessIds() {
+        std::lock_guard<std::mutex> lock(s_processesMutex);
         std::unordered_map<std::string, int64_t> result;
         for (auto it = s_processes.begin(); it != s_processes.end(); ++it) {
             if (it.value()) {
@@ -187,18 +202,24 @@ namespace QtProcessManager {
     }
 
     void clearAll() {
-        for (auto it = s_processes.begin(); it != s_processes.end(); ++it) {
+        QHash<QString, QProcess*> snapshot;
+        {
+            std::lock_guard<std::mutex> lock(s_processesMutex);
+            snapshot.swap(s_processes);
+        }
+        for (auto it = snapshot.begin(); it != snapshot.end(); ++it) {
             QProcess* process = it.value();
             if (process) {
+                process->disconnect();
                 process->terminate();
                 process->waitForFinished(1000);
                 delete process;
             }
         }
-        s_processes.clear();
     }
 
     void registerProcess(const std::string& name) {
+        std::lock_guard<std::mutex> lock(s_processesMutex);
         QString qName = toQ(name);
         if (!s_processes.contains(qName)) {
             s_processes.insert(qName, nullptr);