From 3b396a57f142143be96a7ecd1decd6dd39f80062 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 12:02:52 +0000 Subject: [PATCH 01/14] feat(plugins): directory-per-plugin layout and unified native loader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update `just copy-plugins-native` to output a directory-per-plugin structure under `.plugins/native//` instead of the flat layout. Each plugin now gets its own subdirectory containing the library and `plugin.yml` manifest. Also adds aac-encoder to the copy loop. Merge `load_active_plugins_from_dir` and `load_native_plugins_from_dir` into a single `load_all_native_plugins` method that discovers plugins from three sources in priority order: 1. Active records (.plugins/active/*.json) — marketplace bundles 2. Directory bundles (.plugins/native//) — local directory layout 3. Bare .so files (.plugins/native/*.so) — legacy flat fallback Plugins already loaded by a higher-priority source are skipped. Update bundle convention docs to reflect implemented changes. Closes #254 Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugin_assets.rs | 7 +- apps/skit/src/plugins.rs | 171 ++++++++++++------ .../architecture/plugin-bundle-convention.md | 117 ++++++------ justfile | 57 +++--- 4 files changed, 210 insertions(+), 142 deletions(-) diff --git a/apps/skit/src/plugin_assets.rs b/apps/skit/src/plugin_assets.rs index 58c2f832..ab2e0e0a 100644 --- a/apps/skit/src/plugin_assets.rs +++ b/apps/skit/src/plugin_assets.rs @@ -898,9 +898,10 @@ pub fn plugin_assets_router() -> Router> { /// /// Searches for the manifest in two locations: /// 1. `plugin.yml` / `plugin.yaml` in the same directory as the `.so` file -/// (works when the plugin is in its source tree, e.g. `plugins/native/slint/`). -/// 2. `{stem}.plugin.yml` next to the `.so` file (works with the flat layout -/// produced by `just copy-plugins-native`, e.g. `.plugins/native/slint.plugin.yml`). +/// (primary — works with the directory-per-plugin layout produced by +/// `just copy-plugins-native`, e.g. `.plugins/native/slint/plugin.yml`). +/// 2. `{stem}.plugin.yml` next to the `.so` file (legacy flat layout fallback, +/// e.g. `.plugins/native/slint.plugin.yml`). pub fn read_local_plugin_manifest( library_path: &std::path::Path, ) -> Option { diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index 8c3c4f04..f7d0ed4b 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -192,46 +192,128 @@ impl UnifiedPluginManager { }) } - /// Load all native plugins from the native directory. + /// Unified native plugin loader. /// - /// Scans the top-level directory for `.so`/`.dylib`/`.dll` files (flat - /// layout) and also one level of subdirectories (bundle layout, e.g. - /// `native/slint/libslint.so`). - fn load_native_plugins_from_dir(&mut self) -> Result> { + /// Discovers native plugins from three sources, loaded in priority order: + /// + /// 1. **Active records** (`.plugins/active/*.json`) — marketplace-installed + /// bundles whose entrypoints live under `.plugins/bundles/`. + /// 2. **Directory bundles** (`.plugins/native//`) — local directory + /// layout where each subdirectory contains a `plugin.yml` manifest and + /// the plugin library. + /// 3. **Bare library files** (`.plugins/native/*.so`) — legacy flat layout. + /// + /// A plugin kind that was already loaded by an earlier source is skipped so + /// that marketplace versions always take precedence, followed by directory + /// bundles, followed by bare files. + fn load_all_native_plugins(&mut self) -> Vec { let mut summaries = Vec::new(); - info!("Loading native plugins..."); + info!("Loading native plugins (unified)..."); - let mut lib_paths: Vec = Vec::new(); + // Phase 1: active records (marketplace-installed bundles). + summaries.extend(self.load_active_plugin_records()); - for entry in std::fs::read_dir(&self.native_directory).with_context(|| { - format!("failed to read native plugin directory {}", self.native_directory.display()) - })? { - let entry = entry?; + // Phase 2 & 3: directory bundles + bare library files from the + // native directory. Plugins already loaded in phase 1 are + // automatically skipped (load_native_plugin checks the map). + summaries.extend(self.load_native_dir_plugins()); + + summaries + } + + /// Phase 1: load marketplace-managed plugins from active records. + fn load_active_plugin_records(&mut self) -> Vec { + let active_dir = plugin_active_dir(&self.plugin_base_dir); + if !active_dir.exists() { + return Vec::new(); + } + + let base_real = std::fs::canonicalize(&self.plugin_base_dir).ok(); + let entries = match std::fs::read_dir(&active_dir) { + Ok(entries) => entries, + Err(err) => { + warn!( + error = %err, + dir = %active_dir.display(), + "Failed to read active plugins directory" + ); + return Vec::new(); + }, + }; + + let mut summaries = Vec::new(); + for entry in entries { + let entry = match entry { + Ok(e) => e, + Err(err) => { + warn!(error = %err, "Failed to read active plugin entry"); + continue; + }, + }; let path = entry.path(); + if path.extension().and_then(|ext| ext.to_str()) != Some("json") { + continue; + } + if let Some(summary) = self.load_active_plugin_record(&path, base_real.as_deref()) { + info!( + plugin = %summary.kind, + source = "active-record", + "Loaded marketplace plugin" + ); + summaries.push(summary); + } + } + summaries + } - if path.is_dir() { - // Scan one level of subdirectories for plugin bundles. - if let Ok(sub_entries) = std::fs::read_dir(&path) { - for sub_entry in sub_entries.flatten() { - let sub_path = sub_entry.path(); - if Self::is_native_lib(&sub_path) { - lib_paths.push(sub_path); + /// Phases 2 & 3: scan the native directory for directory bundles and bare + /// library files, loading directory bundles first so they take precedence. + fn load_native_dir_plugins(&mut self) -> Vec { + let mut dir_bundle_libs: Vec = Vec::new(); + let mut bare_libs: Vec = Vec::new(); + + if let Ok(entries) = std::fs::read_dir(&self.native_directory) { + for entry in entries.flatten() { + let path = entry.path(); + + if path.is_dir() { + // Scan one level of subdirectories for plugin libraries. + if let Ok(sub_entries) = std::fs::read_dir(&path) { + for sub_entry in sub_entries.flatten() { + let sub_path = sub_entry.path(); + if Self::is_native_lib(&sub_path) { + dir_bundle_libs.push(sub_path); + } } } + continue; } - continue; - } - if Self::is_native_lib(&path) { - lib_paths.push(path); + if Self::is_native_lib(&path) { + bare_libs.push(path); + } } } - for path in lib_paths { + // Load directory-bundle plugins first, then bare files. Skip any + // plugin whose kind is already loaded (from active records or an + // earlier library in this phase). + let mut summaries = Vec::new(); + for path in dir_bundle_libs.into_iter().chain(bare_libs) { match self.load_native_plugin(&path) { Ok(summary) => { - info!(plugin = %summary.kind, file = ?path, plugin_type = ?summary.plugin_type, "Loaded plugin from disk"); + let source = if path.parent() == Some(self.native_directory.as_path()) { + "bare-file" + } else { + "directory-bundle" + }; + info!( + plugin = %summary.kind, + file = ?path, + source, + "Loaded native plugin from disk" + ); summaries.push(summary); }, Err(err) => { @@ -239,8 +321,7 @@ impl UnifiedPluginManager { }, } } - - Ok(summaries) + summaries } /// Returns `true` if the path looks like a native plugin library. @@ -278,33 +359,6 @@ impl UnifiedPluginManager { Ok(summaries) } - /// Load marketplace-managed plugins using active records. - fn load_active_plugins_from_dir(&mut self) -> Result> { - let active_dir = plugin_active_dir(&self.plugin_base_dir); - if !active_dir.exists() { - return Ok(Vec::new()); - } - - let base_real = std::fs::canonicalize(&self.plugin_base_dir).ok(); - let mut summaries = Vec::new(); - - for entry in std::fs::read_dir(&active_dir).with_context(|| { - format!("failed to read active plugins dir {}", active_dir.display()) - })? { - let entry = entry?; - let path = entry.path(); - if path.extension().and_then(|ext| ext.to_str()) != Some("json") { - continue; - } - - if let Some(summary) = self.load_active_plugin_record(&path, base_real.as_deref()) { - summaries.push(summary); - } - } - - Ok(summaries) - } - fn load_active_plugin_record( &mut self, record_path: &Path, @@ -407,16 +461,17 @@ impl UnifiedPluginManager { Some(entrypoint_path) } - /// Loads all existing plugins from both WASM and native directories. - /// Native plugins are loaded first as they are faster to initialize. + /// Loads all existing plugins from both native and WASM directories. + /// + /// Native plugins (including marketplace-installed bundles) are loaded + /// first via the unified loader, then WASM plugins. /// /// # Errors /// - /// Returns an error if the plugin directories cannot be read. + /// Returns an error if the WASM plugin directory cannot be read. /// Individual plugin load failures are logged but do not prevent other plugins from loading. pub fn load_existing(&mut self) -> Result> { - let mut summaries = self.load_active_plugins_from_dir()?; - summaries.extend(self.load_native_plugins_from_dir()?); + let mut summaries = self.load_all_native_plugins(); summaries.extend(self.load_wasm_plugins_from_dir()?); Ok(summaries) } diff --git a/docs/src/content/docs/architecture/plugin-bundle-convention.md b/docs/src/content/docs/architecture/plugin-bundle-convention.md index c7154e86..f3fbd580 100644 --- a/docs/src/content/docs/architecture/plugin-bundle-convention.md +++ b/docs/src/content/docs/architecture/plugin-bundle-convention.md @@ -2,39 +2,20 @@ # SPDX-FileCopyrightText: © 2025 StreamKit Contributors # SPDX-License-Identifier: MPL-2.0 title: Plugin Bundle Convention -description: Proposed directory layout for plugin bundles +description: Directory layout for plugin bundles --- -> [!NOTE] -> This is a **design proposal** for unifying the plugin directory layout. -> The migration is incremental — existing bare `.so` plugins continue to work. -> See [Issue #254](https://github.com/streamer45/streamkit/issues/254) for -> discussion. - ## Motivation -StreamKit currently has two plugin layouts: - -| Source | Layout | Example path | -|--------|--------|-------------| -| Local / dev | Bare `.so` in a flat directory | `.plugins/native/libslint.so` | -| Marketplace | Extracted bundle directory | `.plugins/bundles/slint/0.1.0/libslint.so` | - -The discrepancy causes problems: - -1. **Asset types lost on restart** — marketplace bundles ship `manifest.json` - but the local loader expects `plugin.yml`. Without a YAML manifest next to - the library, `collect_plugin_asset_specs` cannot rediscover asset - declarations after a server restart. -2. **No room for extras** — a bare `.so` cannot carry bundled assets, custom - UI files, or license metadata. -3. **Two code paths** — `load_native_plugins_from_dir` scans for `.so` files - while `load_active_plugins_from_dir` reads JSON records. Both should - converge on the same convention. +StreamKit plugins can come from multiple sources (local builds, marketplace +installs, manual uploads). A consistent directory layout ensures that the +server can always discover a plugin's manifest — and therefore its asset +type declarations, model requirements, and other metadata — regardless of +how the plugin was installed. -## Proposed convention +## Convention -Every plugin — whether local or marketplace — should be a **directory** +Every plugin — whether local or marketplace — is a **directory** (a.k.a. "bundle") with a well-known internal layout: ``` @@ -65,45 +46,71 @@ additional `bundle` block for download metadata. ### Discovery rules -`read_local_plugin_manifest` already searches for: +`read_local_plugin_manifest` searches for: 1. `plugin.yml` / `plugin.yaml` in the same directory as the `.so` 2. `{stem}.plugin.yml` next to the `.so` (flat layout fallback) -No changes to the discovery logic are needed. The key requirement is that -a `plugin.yml` file exists next to the entrypoint library. +The key requirement is that a `plugin.yml` file exists next to the +entrypoint library. + +## Unified loader + +`load_all_native_plugins` discovers native plugins from three sources, +loaded in priority order: + +1. **Active records** (`.plugins/active/*.json`) — marketplace-installed + bundles whose entrypoints live under `.plugins/bundles/`. +2. **Directory bundles** (`.plugins/native//`) — local directory + layout where each subdirectory contains the plugin library and a + `plugin.yml` manifest. +3. **Bare library files** (`.plugins/native/*.so`) — legacy flat layout + for backward compatibility. -## Current state & incremental steps +A plugin kind that was already loaded by an earlier source is skipped so +that marketplace versions always take precedence, followed by directory +bundles, followed by bare files. -### Already done +## Build tooling + +`just copy-plugins-native` copies built plugins into the directory layout: + +``` +.plugins/native/slint/ +├── libslint.so +└── plugin.yml +``` + +Each plugin gets its own subdirectory under `.plugins/native/`. The +`plugin.yml` is copied from the plugin's source tree +(`plugins/native//plugin.yml`). + +## Implemented - **Marketplace bundles write `plugin.yml` on install** — during - `handle_bundle_install`, the marketplace installer now serializes the - verified manifest as `plugin.yml` next to the entrypoint. This ensures + `handle_bundle_install`, the marketplace installer serializes the + verified manifest as `plugin.yml` next to the entrypoint. This ensures `collect_plugin_asset_specs` finds asset declarations on restart. -- **`just copy-plugins-native` copies manifests** — when copying built - plugins from `target/` to `.plugins/native/`, the recipe also copies - `plugin.yml` as `{stem}.plugin.yml` alongside the `.so` file. - -### Future steps (not yet implemented) +- **`just copy-plugins-native` uses directory layout** — when copying + built plugins from `target/` to `.plugins/native/`, the recipe creates + a directory per plugin (e.g. `.plugins/native/slint/`) containing both + the library and `plugin.yml`. -1. **Local plugin directory layout** — allow placing local plugins as - directories under `.plugins/native//` instead of bare `.so` - files. `load_native_plugins_from_dir` would check for directories - containing a `plugin.yml` before falling back to bare `.so` scanning. +- **Unified loader** — `load_all_native_plugins` merges the former + `load_active_plugins_from_dir` and `load_native_plugins_from_dir` into + a single pass that handles active records, directory bundles, and bare + library files with correct priority ordering. -2. **Unified loader** — merge `load_native_plugins_from_dir` and - `load_active_plugins_from_dir` into a single pass that handles both - directory-based plugins (with `plugin.yml`) and legacy bare `.so` files. +## Future steps -3. **Bundled system assets** — when a plugin bundle includes a `samples/` +1. **Bundled system assets** — when a plugin bundle includes a `samples/` directory, the asset registry should resolve `system_dir` relative to the bundle root. This allows marketplace plugins to ship default assets (e.g. example `.slint` files) without requiring separate downloads. -4. **Custom UI** — plugins could ship frontend components (e.g. +2. **Custom UI** — plugins could ship frontend components (e.g. `ui/config-panel.jsx`) that the dashboard loads at runtime for node-specific configuration UIs. @@ -111,12 +118,12 @@ a `plugin.yml` file exists next to the entrypoint library. The migration is **backward-compatible**: -- Bare `.so` files continue to work. `load_native_plugins_from_dir` - keeps its current scanning logic as a fallback. +- Bare `.so` files continue to work. The unified loader scans for them + as a fallback after directory bundles. - Marketplace bundles already ARE directories — they just need a - `plugin.yml` written during installation (now done). -- The `{stem}.plugin.yml` naming convention bridges the flat layout used - by `just copy-plugins-native`. + `plugin.yml` written during installation (done). +- The `{stem}.plugin.yml` naming convention is still supported by + `read_local_plugin_manifest` for any remaining flat layouts. -No breaking changes are required. New features (directory-based local -plugins, bundled assets) are additive. +No breaking changes are required. New features (bundled assets, custom +UI) are additive. diff --git a/justfile b/justfile index 71854446..019548cc 100644 --- a/justfile +++ b/justfile @@ -1011,41 +1011,46 @@ copy-plugins-native: PLUGINS_TARGET="{{plugins_target_dir}}" - # Examples (gain-native has its own target dir, not shared) - cp examples/plugins/gain-native/target/release/libgain_plugin_native.* .plugins/native/ 2>/dev/null || true + # Helper: copy a built plugin into the directory-per-plugin layout. + # copy_plugin + # Creates .plugins/native// with the .so and plugin.yml. + copy_plugin() { + local id="$1" stem="$2" src_dir="$3" + local dest=".plugins/native/$id" + mkdir -p "$dest" - # Official native plugins (shared target dir) - for name in whisper kokoro piper matcha vad sensevoice nllb helsinki supertonic slint; do for f in \ - "$PLUGINS_TARGET"/release/lib"$name".so \ - "$PLUGINS_TARGET"/release/lib"$name".so.* \ - "$PLUGINS_TARGET"/release/lib"$name".dylib \ - "$PLUGINS_TARGET"/release/"$name".dll + "$src_dir"/release/lib"$stem".so \ + "$src_dir"/release/lib"$stem".so.* \ + "$src_dir"/release/lib"$stem".dylib \ + "$src_dir"/release/"$stem".dll do if [[ -f "$f" ]]; then - cp -f "$f" .plugins/native/ + cp -f "$f" "$dest/" fi done - # Copy plugin.yml manifest alongside the .so so asset types can be + + # Copy plugin.yml into the bundle directory so asset types can be # discovered at runtime (see plugin_assets::read_local_plugin_manifest). - if [[ -f "plugins/native/$name/plugin.yml" ]]; then - cp -f "plugins/native/$name/plugin.yml" ".plugins/native/${name}.plugin.yml" - fi - done - for f in \ - "$PLUGINS_TARGET"/release/libpocket_tts.so \ - "$PLUGINS_TARGET"/release/libpocket_tts.so.* \ - "$PLUGINS_TARGET"/release/libpocket_tts.dylib \ - "$PLUGINS_TARGET"/release/pocket_tts.dll - do - if [[ -f "$f" ]]; then - cp -f "$f" .plugins/native/ + if [[ -f "plugins/native/$id/plugin.yml" ]]; then + cp -f "plugins/native/$id/plugin.yml" "$dest/plugin.yml" fi + } + + # Examples (gain-native has its own target dir, not shared) + mkdir -p .plugins/native/gain-native + cp examples/plugins/gain-native/target/release/libgain_plugin_native.* .plugins/native/gain-native/ 2>/dev/null || true + + # Official native plugins (shared target dir). + # For most plugins the lib stem matches the plugin id. + for name in whisper kokoro piper matcha vad sensevoice nllb helsinki supertonic slint; do + copy_plugin "$name" "$name" "$PLUGINS_TARGET" done - # Copy pocket-tts plugin.yml for consistency with the main loop above. - if [[ -f "plugins/native/pocket-tts/plugin.yml" ]]; then - cp -f "plugins/native/pocket-tts/plugin.yml" ".plugins/native/pocket-tts.plugin.yml" - fi + + # Plugins whose lib stem differs from the plugin id. + copy_plugin "pocket-tts" "pocket_tts" "$PLUGINS_TARGET" + copy_plugin "aac-encoder" "aac_encoder" "$PLUGINS_TARGET" + echo "✓ Native plugins copied to .plugins/native/" # --- License Headers (REUSE) --- From d49d545ec034a678bc613b987fb0e066695df018 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 13:03:13 +0000 Subject: [PATCH 02/14] fix(plugins): address review feedback on unified loader - Use copy_plugin helper for gain-native instead of raw cp + glob, ensuring consistent handling of versioned .so.* and plugin.yml. - Log a warning when the native plugins directory cannot be read, matching the behavior of load_active_plugin_records. - Distinguish 'already loaded' skips (debug level) from genuine load failures (warn level) to avoid misleading log output. - Sort discovered library paths for deterministic load order. - Add comment explaining the source-label heuristic. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 57 ++++++++++++++++++++++++++++------------ justfile | 3 +-- 2 files changed, 41 insertions(+), 19 deletions(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index f7d0ed4b..4c3294d8 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -273,29 +273,43 @@ impl UnifiedPluginManager { let mut dir_bundle_libs: Vec = Vec::new(); let mut bare_libs: Vec = Vec::new(); - if let Ok(entries) = std::fs::read_dir(&self.native_directory) { - for entry in entries.flatten() { - let path = entry.path(); - - if path.is_dir() { - // Scan one level of subdirectories for plugin libraries. - if let Ok(sub_entries) = std::fs::read_dir(&path) { - for sub_entry in sub_entries.flatten() { - let sub_path = sub_entry.path(); - if Self::is_native_lib(&sub_path) { - dir_bundle_libs.push(sub_path); + match std::fs::read_dir(&self.native_directory) { + Ok(entries) => { + for entry in entries.flatten() { + let path = entry.path(); + + if path.is_dir() { + // Scan one level of subdirectories for plugin libraries. + if let Ok(sub_entries) = std::fs::read_dir(&path) { + for sub_entry in sub_entries.flatten() { + let sub_path = sub_entry.path(); + if Self::is_native_lib(&sub_path) { + dir_bundle_libs.push(sub_path); + } } } + continue; } - continue; - } - if Self::is_native_lib(&path) { - bare_libs.push(path); + if Self::is_native_lib(&path) { + bare_libs.push(path); + } } - } + }, + Err(err) => { + warn!( + error = %err, + dir = %self.native_directory.display(), + "Failed to read native plugins directory" + ); + }, } + // Sort for deterministic load order when multiple bundles or bare + // files provide the same plugin kind. + dir_bundle_libs.sort(); + bare_libs.sort(); + // Load directory-bundle plugins first, then bare files. Skip any // plugin whose kind is already loaded (from active records or an // earlier library in this phase). @@ -303,6 +317,10 @@ impl UnifiedPluginManager { for path in dir_bundle_libs.into_iter().chain(bare_libs) { match self.load_native_plugin(&path) { Ok(summary) => { + // Classify the source based on the library's parent + // directory: if it is directly inside the native dir it + // came from a bare flat file; otherwise it is nested + // inside a subdirectory (directory-bundle layout). let source = if path.parent() == Some(self.native_directory.as_path()) { "bare-file" } else { @@ -317,7 +335,12 @@ impl UnifiedPluginManager { summaries.push(summary); }, Err(err) => { - warn!(error = %err, file = ?path, "Failed to load native plugin from disk"); + let msg = err.to_string(); + if msg.contains("already loaded") || msg.contains("already registered") { + debug!(file = ?path, "Skipping plugin (already loaded by higher-priority source)"); + } else { + warn!(error = %err, file = ?path, "Failed to load native plugin from disk"); + } }, } } diff --git a/justfile b/justfile index 019548cc..71cddf27 100644 --- a/justfile +++ b/justfile @@ -1038,8 +1038,7 @@ copy-plugins-native: } # Examples (gain-native has its own target dir, not shared) - mkdir -p .plugins/native/gain-native - cp examples/plugins/gain-native/target/release/libgain_plugin_native.* .plugins/native/gain-native/ 2>/dev/null || true + copy_plugin "gain-native" "gain_plugin_native" "examples/plugins/gain-native/target" # Official native plugins (shared target dir). # For most plugins the lib stem matches the plugin id. From 7dc833a83121546015b6d805a307a97fa2ed8ac8 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 13:35:38 +0000 Subject: [PATCH 03/14] refactor(plugins): drop bare .so fallback, require directory layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we are pre-1.0 (v0.x), enforce the directory-per-plugin layout everywhere instead of maintaining a legacy flat-file fallback: - load_native_dir_plugins now only scans subdirectories of .plugins/native/, ignoring top-level .so files. - validate_plugin_upload_target places HTTP-uploaded native plugins into a subdirectory derived from the library stem (e.g. libgain.so → .plugins/native/gain/libgain.so). - unload_plugin cleans up the parent directory if empty after removing the .so file. - Updated docs and code comments to reflect the two-source model (active records + directory bundles). Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugin_assets.rs | 6 +- apps/skit/src/plugins.rs | 92 ++++++++++--------- .../architecture/plugin-bundle-convention.md | 39 ++++---- 3 files changed, 70 insertions(+), 67 deletions(-) diff --git a/apps/skit/src/plugin_assets.rs b/apps/skit/src/plugin_assets.rs index ab2e0e0a..c749b259 100644 --- a/apps/skit/src/plugin_assets.rs +++ b/apps/skit/src/plugin_assets.rs @@ -898,10 +898,10 @@ pub fn plugin_assets_router() -> Router> { /// /// Searches for the manifest in two locations: /// 1. `plugin.yml` / `plugin.yaml` in the same directory as the `.so` file -/// (primary — works with the directory-per-plugin layout produced by +/// (works with the directory-per-plugin layout produced by /// `just copy-plugins-native`, e.g. `.plugins/native/slint/plugin.yml`). -/// 2. `{stem}.plugin.yml` next to the `.so` file (legacy flat layout fallback, -/// e.g. `.plugins/native/slint.plugin.yml`). +/// 2. `{stem}.plugin.yml` / `{stem}.plugin.yaml` next to the `.so` file +/// (fallback for any non-standard layouts). pub fn read_local_plugin_manifest( library_path: &std::path::Path, ) -> Option { diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index 4c3294d8..da9ba65b 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -194,18 +194,20 @@ impl UnifiedPluginManager { /// Unified native plugin loader. /// - /// Discovers native plugins from three sources, loaded in priority order: + /// Discovers native plugins from two sources, loaded in priority order: /// /// 1. **Active records** (`.plugins/active/*.json`) — marketplace-installed /// bundles whose entrypoints live under `.plugins/bundles/`. /// 2. **Directory bundles** (`.plugins/native//`) — local directory /// layout where each subdirectory contains a `plugin.yml` manifest and /// the plugin library. - /// 3. **Bare library files** (`.plugins/native/*.so`) — legacy flat layout. /// /// A plugin kind that was already loaded by an earlier source is skipped so - /// that marketplace versions always take precedence, followed by directory - /// bundles, followed by bare files. + /// that marketplace versions always take precedence. + /// + /// Errors reading plugin directories are logged but do not propagate; + /// callers cannot distinguish "no plugins found" from "directory unreadable" + /// except via the `warn!` log output. fn load_all_native_plugins(&mut self) -> Vec { let mut summaries = Vec::new(); @@ -267,32 +269,26 @@ impl UnifiedPluginManager { summaries } - /// Phases 2 & 3: scan the native directory for directory bundles and bare - /// library files, loading directory bundles first so they take precedence. + /// Phases 2 & 3: scan the native directory for directory bundles, loading + /// plugins from subdirectories of `.plugins/native//`. fn load_native_dir_plugins(&mut self) -> Vec { - let mut dir_bundle_libs: Vec = Vec::new(); - let mut bare_libs: Vec = Vec::new(); + let mut lib_paths: Vec = Vec::new(); match std::fs::read_dir(&self.native_directory) { Ok(entries) => { for entry in entries.flatten() { let path = entry.path(); - - if path.is_dir() { - // Scan one level of subdirectories for plugin libraries. - if let Ok(sub_entries) = std::fs::read_dir(&path) { - for sub_entry in sub_entries.flatten() { - let sub_path = sub_entry.path(); - if Self::is_native_lib(&sub_path) { - dir_bundle_libs.push(sub_path); - } - } - } + if !path.is_dir() { continue; } - - if Self::is_native_lib(&path) { - bare_libs.push(path); + // Scan one level of subdirectories for plugin libraries. + if let Ok(sub_entries) = std::fs::read_dir(&path) { + for sub_entry in sub_entries.flatten() { + let sub_path = sub_entry.path(); + if Self::is_native_lib(&sub_path) { + lib_paths.push(sub_path); + } + } } } }, @@ -305,32 +301,20 @@ impl UnifiedPluginManager { }, } - // Sort for deterministic load order when multiple bundles or bare - // files provide the same plugin kind. - dir_bundle_libs.sort(); - bare_libs.sort(); + // Sort for deterministic load order when multiple bundles provide + // the same plugin kind. + lib_paths.sort(); - // Load directory-bundle plugins first, then bare files. Skip any - // plugin whose kind is already loaded (from active records or an - // earlier library in this phase). + // Skip any plugin whose kind is already loaded (from active records + // or an earlier library in this phase). let mut summaries = Vec::new(); - for path in dir_bundle_libs.into_iter().chain(bare_libs) { + for path in lib_paths { match self.load_native_plugin(&path) { Ok(summary) => { - // Classify the source based on the library's parent - // directory: if it is directly inside the native dir it - // came from a bare flat file; otherwise it is nested - // inside a subdirectory (directory-bundle layout). - let source = if path.parent() == Some(self.native_directory.as_path()) { - "bare-file" - } else { - "directory-bundle" - }; info!( plugin = %summary.kind, file = ?path, - source, - "Loaded native plugin from disk" + "Loaded native plugin from directory bundle" ); summaries.push(summary); }, @@ -855,6 +839,20 @@ impl UnifiedPluginManager { "Failed to remove plugin file during unload" ); } + + // Clean up the parent directory if it is a now-empty subdirectory + // inside the native plugins dir (directory-bundle layout). + if let Some(parent) = file_path.parent() { + if parent != self.native_directory && parent.starts_with(&self.native_directory) { + let is_empty = parent + .read_dir() + .map(|mut entries| entries.next().is_none()) + .unwrap_or(false); + if is_empty { + let _ = std::fs::remove_dir(parent); + } + } + } } Ok(summary) @@ -987,7 +985,17 @@ impl UnifiedPluginManager { let (target_path, plugin_type) = match extension { Some("wasm") => (self.wasm_directory.join(sanitized), PluginType::Wasm), Some("so" | "dylib" | "dll") => { - (self.native_directory.join(sanitized), PluginType::Native) + // Place native plugins inside a subdirectory derived from the + // library stem (e.g. `libgain.so` → `native/gain/libgain.so`). + let stem = Path::new(sanitized) + .file_stem() + .and_then(|s| s.to_str()) + .map_or(sanitized, |s| s.strip_prefix("lib").unwrap_or(s)); + let subdir = self.native_directory.join(stem); + std::fs::create_dir_all(&subdir).with_context(|| { + format!("failed to create native plugin directory {}", subdir.display()) + })?; + (subdir.join(sanitized), PluginType::Native) }, _ => { return Err(anyhow!( diff --git a/docs/src/content/docs/architecture/plugin-bundle-convention.md b/docs/src/content/docs/architecture/plugin-bundle-convention.md index f3fbd580..f5fd82d7 100644 --- a/docs/src/content/docs/architecture/plugin-bundle-convention.md +++ b/docs/src/content/docs/architecture/plugin-bundle-convention.md @@ -49,14 +49,14 @@ additional `bundle` block for download metadata. `read_local_plugin_manifest` searches for: 1. `plugin.yml` / `plugin.yaml` in the same directory as the `.so` -2. `{stem}.plugin.yml` next to the `.so` (flat layout fallback) +2. `{stem}.plugin.yml` / `{stem}.plugin.yaml` next to the `.so` (fallback) The key requirement is that a `plugin.yml` file exists next to the entrypoint library. ## Unified loader -`load_all_native_plugins` discovers native plugins from three sources, +`load_all_native_plugins` discovers native plugins from two sources, loaded in priority order: 1. **Active records** (`.plugins/active/*.json`) — marketplace-installed @@ -64,12 +64,10 @@ loaded in priority order: 2. **Directory bundles** (`.plugins/native//`) — local directory layout where each subdirectory contains the plugin library and a `plugin.yml` manifest. -3. **Bare library files** (`.plugins/native/*.so`) — legacy flat layout - for backward compatibility. A plugin kind that was already loaded by an earlier source is skipped so -that marketplace versions always take precedence, followed by directory -bundles, followed by bare files. +that marketplace versions always take precedence. Duplicate-kind skips +are logged at `debug` level; genuine load failures are logged at `warn`. ## Build tooling @@ -85,6 +83,13 @@ Each plugin gets its own subdirectory under `.plugins/native/`. The `plugin.yml` is copied from the plugin's source tree (`plugins/native//plugin.yml`). +## HTTP uploads + +Plugins uploaded via `POST /api/v1/plugins` are also placed into the +directory layout. The subdirectory name is derived from the library stem +(e.g. `libgain.so` → `.plugins/native/gain/libgain.so`). On unload the +subdirectory is cleaned up if empty. + ## Implemented - **Marketplace bundles write `plugin.yml` on install** — during @@ -99,8 +104,12 @@ Each plugin gets its own subdirectory under `.plugins/native/`. The - **Unified loader** — `load_all_native_plugins` merges the former `load_active_plugins_from_dir` and `load_native_plugins_from_dir` into - a single pass that handles active records, directory bundles, and bare - library files with correct priority ordering. + a single pass that handles active records and directory bundles with + correct priority ordering. + +- **HTTP uploads use directory layout** — uploaded native plugins are + placed into a subdirectory of `.plugins/native/` matching the library + stem, consistent with the build-time layout. ## Future steps @@ -113,17 +122,3 @@ Each plugin gets its own subdirectory under `.plugins/native/`. The 2. **Custom UI** — plugins could ship frontend components (e.g. `ui/config-panel.jsx`) that the dashboard loads at runtime for node-specific configuration UIs. - -## Migration path - -The migration is **backward-compatible**: - -- Bare `.so` files continue to work. The unified loader scans for them - as a fallback after directory bundles. -- Marketplace bundles already ARE directories — they just need a - `plugin.yml` written during installation (done). -- The `{stem}.plugin.yml` naming convention is still supported by - `read_local_plugin_manifest` for any remaining flat layouts. - -No breaking changes are required. New features (bundled assets, custom -UI) are additive. From c103a07415b5d86b50ad79fe182aa1a8e68d4d35 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 13:41:46 +0000 Subject: [PATCH 04/14] fix(plugins): handle empty stem edge case and warn on bare .so files - Guard against lib.so/lib.dylib/lib.dll uploads where stripping the 'lib' prefix yields an empty stem, which would place the file flat in the native directory. Now falls back to the full filename. - Log a warning when bare .so files are found directly in the native directory, guiding users to move them into subdirectories. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index da9ba65b..b67f34ed 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -279,6 +279,14 @@ impl UnifiedPluginManager { for entry in entries.flatten() { let path = entry.path(); if !path.is_dir() { + // Warn about bare library files that will be ignored. + if Self::is_native_lib(&path) { + warn!( + file = ?path, + "Bare plugin file found in native directory; \ + move it into a subdirectory (e.g. .plugins/native//)" + ); + } continue; } // Scan one level of subdirectories for plugin libraries. @@ -990,7 +998,9 @@ impl UnifiedPluginManager { let stem = Path::new(sanitized) .file_stem() .and_then(|s| s.to_str()) - .map_or(sanitized, |s| s.strip_prefix("lib").unwrap_or(s)); + .map(|s| s.strip_prefix("lib").unwrap_or(s)) + .filter(|s| !s.is_empty()) + .unwrap_or(sanitized); let subdir = self.native_directory.join(stem); std::fs::create_dir_all(&subdir).with_context(|| { format!("failed to create native plugin directory {}", subdir.display()) From a232bc9a5cbf98fc55b3f4f038353ff014d96b54 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 13:48:12 +0000 Subject: [PATCH 05/14] fix(plugins): fix stale comment and clean up orphaned dirs on failed upload - Update comment from 'Phase 2 & 3' to 'Phase 2' since bare .so files are no longer loaded. - Clean up the subdirectory when a native plugin upload fails to load, mirroring the cleanup logic in unload_plugin. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index b67f34ed..d4b1b49e 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -269,7 +269,7 @@ impl UnifiedPluginManager { summaries } - /// Phases 2 & 3: scan the native directory for directory bundles, loading + /// Phase 2: scan the native directory for directory bundles, loading /// plugins from subdirectories of `.plugins/native//`. fn load_native_dir_plugins(&mut self) -> Vec { let mut lib_paths: Vec = Vec::new(); @@ -957,6 +957,20 @@ impl UnifiedPluginManager { Ok(summary) => Ok(summary), Err(e) => { let _ = std::fs::remove_file(&target_path); + // Clean up the subdirectory if it is now empty (failed + // native upload should not leave orphaned directories). + if let Some(parent) = target_path.parent() { + if parent != self.native_directory && parent.starts_with(&self.native_directory) + { + let is_empty = parent + .read_dir() + .map(|mut entries| entries.next().is_none()) + .unwrap_or(false); + if is_empty { + let _ = std::fs::remove_dir(parent); + } + } + } Err(e) }, } From 8eac95b94de69feb173e77a8557be13e0bccb758 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 13:55:22 +0000 Subject: [PATCH 06/14] refactor(plugins): extract try_remove_empty_plugin_dir helper Deduplicate the directory cleanup logic used by unload_plugin, load_from_bytes, and load_from_temp_file into a shared helper method. Also fixes load_from_bytes which was missing cleanup on failure. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 52 +++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index d4b1b49e..ab76c65f 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -848,19 +848,7 @@ impl UnifiedPluginManager { ); } - // Clean up the parent directory if it is a now-empty subdirectory - // inside the native plugins dir (directory-bundle layout). - if let Some(parent) = file_path.parent() { - if parent != self.native_directory && parent.starts_with(&self.native_directory) { - let is_empty = parent - .read_dir() - .map(|mut entries| entries.next().is_none()) - .unwrap_or(false); - if is_empty { - let _ = std::fs::remove_dir(parent); - } - } - } + self.try_remove_empty_plugin_dir(&file_path); } Ok(summary) @@ -890,6 +878,20 @@ impl UnifiedPluginManager { self.plugins_loaded_gauge.record(native_count, &[KeyValue::new("plugin_type", "native")]); } + /// Remove the parent directory of a plugin file if it is a now-empty + /// subdirectory inside the native plugins dir (directory-bundle layout). + fn try_remove_empty_plugin_dir(&self, file_path: &Path) { + if let Some(parent) = file_path.parent() { + if parent != self.native_directory && parent.starts_with(&self.native_directory) { + let is_empty = + parent.read_dir().map(|mut entries| entries.next().is_none()).unwrap_or(false); + if is_empty { + let _ = std::fs::remove_dir(parent); + } + } + } + } + /// Saves raw plugin bytes into the managed directory and loads the resulting plugin. /// Automatically detects plugin type based on file extension. /// @@ -907,7 +909,14 @@ impl UnifiedPluginManager { std::fs::write(&target_path, bytes) .with_context(|| format!("failed to write plugin file {}", target_path.display()))?; - self.load_from_written_path(plugin_type, target_path) + match self.load_from_written_path(plugin_type, target_path.clone()) { + Ok(summary) => Ok(summary), + Err(e) => { + let _ = std::fs::remove_file(&target_path); + self.try_remove_empty_plugin_dir(&target_path); + Err(e) + }, + } } /// Moves an already-written plugin file into the managed directory and loads it. @@ -957,20 +966,7 @@ impl UnifiedPluginManager { Ok(summary) => Ok(summary), Err(e) => { let _ = std::fs::remove_file(&target_path); - // Clean up the subdirectory if it is now empty (failed - // native upload should not leave orphaned directories). - if let Some(parent) = target_path.parent() { - if parent != self.native_directory && parent.starts_with(&self.native_directory) - { - let is_empty = parent - .read_dir() - .map(|mut entries| entries.next().is_none()) - .unwrap_or(false); - if is_empty { - let _ = std::fs::remove_dir(parent); - } - } - } + self.try_remove_empty_plugin_dir(&target_path); Err(e) }, } From 892a1b172219f1bc8a1bb6bdb9f44423c2ae7b97 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 14:04:09 +0000 Subject: [PATCH 07/14] fix(plugins): validate stem edge cases and document error-string coupling - Reject stems that resolve to '.' or '..' (e.g. lib..so) which would create problematic directories. - Document the coupling between load_native_plugin's error messages and load_native_dir_plugins' string-matching control flow, at both the producer and consumer sites. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index ab76c65f..49325af1 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -327,6 +327,11 @@ impl UnifiedPluginManager { summaries.push(summary); }, Err(err) => { + // NOTE: This relies on the exact error wording produced by + // load_native_plugin ("already loaded" / "already registered"). + // A pre-check is not feasible here because the plugin kind + // is only known after dlopen (LoadedNativePlugin::load). + // If those messages change, update this check too. let msg = err.to_string(); if msg.contains("already loaded") || msg.contains("already registered") { debug!(file = ?path, "Skipping plugin (already loaded by higher-priority source)"); @@ -752,6 +757,9 @@ impl UnifiedPluginManager { let categories = metadata.categories.clone(); if self.plugins.contains_key(&kind) { + // NOTE: load_native_dir_plugins pattern-matches on "already loaded" + // in this message to distinguish expected dedup skips from genuine + // failures. Keep the wording in sync with the check there. return Err(anyhow!( "A plugin providing node '{original_kind}' (registered as '{kind}') is already loaded" )); @@ -1011,6 +1019,11 @@ impl UnifiedPluginManager { .map(|s| s.strip_prefix("lib").unwrap_or(s)) .filter(|s| !s.is_empty()) .unwrap_or(sanitized); + if stem == "." || stem == ".." { + return Err(anyhow!( + "Cannot derive a valid plugin directory name from '{sanitized}'" + )); + } let subdir = self.native_directory.join(stem); std::fs::create_dir_all(&subdir).with_context(|| { format!("failed to create native plugin directory {}", subdir.display()) From 75cadcd842702818a8f8973a54ba1c09b790d363 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 14:13:37 +0000 Subject: [PATCH 08/14] fix(plugins): ensure directory cleanup on all upload error paths Use closure-based error handling in load_from_bytes and load_from_temp_file so that try_remove_empty_plugin_dir runs on every error path after validate_plugin_upload_target creates the subdirectory, not just when load_from_written_path fails. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 81 +++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 39 deletions(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index 49325af1..bf8ac0b8 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -914,17 +914,18 @@ impl UnifiedPluginManager { pub fn load_from_bytes(&mut self, file_name: &str, bytes: &[u8]) -> Result { let (target_path, plugin_type) = self.validate_plugin_upload_target(file_name)?; - std::fs::write(&target_path, bytes) - .with_context(|| format!("failed to write plugin file {}", target_path.display()))?; - - match self.load_from_written_path(plugin_type, target_path.clone()) { - Ok(summary) => Ok(summary), - Err(e) => { - let _ = std::fs::remove_file(&target_path); - self.try_remove_empty_plugin_dir(&target_path); - Err(e) - }, + let result = (|| { + std::fs::write(&target_path, bytes).with_context(|| { + format!("failed to write plugin file {}", target_path.display()) + })?; + self.load_from_written_path(plugin_type, target_path.clone()) + })(); + + if result.is_err() { + let _ = std::fs::remove_file(&target_path); + self.try_remove_empty_plugin_dir(&target_path); } + result } /// Moves an already-written plugin file into the managed directory and loads it. @@ -946,38 +947,40 @@ impl UnifiedPluginManager { ) -> Result { let (target_path, plugin_type) = self.validate_plugin_upload_target(file_name)?; - let meta = std::fs::metadata(temp_path) - .with_context(|| format!("failed to stat temp plugin file {}", temp_path.display()))?; - if !meta.is_file() { - return Err(anyhow!("temp plugin path is not a file: {}", temp_path.display())); - } - - // Prefer atomic move; fall back to copy+remove for cross-device temp dirs. - if let Err(e) = std::fs::rename(temp_path, &target_path) { - debug!( - error = %e, - from = %temp_path.display(), - to = %target_path.display(), - "rename failed; falling back to copy+remove" - ); - std::fs::copy(temp_path, &target_path).with_context(|| { - format!( - "failed to copy temp plugin file from {} to {}", - temp_path.display(), - target_path.display() - ) + let result = (|| { + let meta = std::fs::metadata(temp_path).with_context(|| { + format!("failed to stat temp plugin file {}", temp_path.display()) })?; - let _ = std::fs::remove_file(temp_path); - } + if !meta.is_file() { + return Err(anyhow!("temp plugin path is not a file: {}", temp_path.display())); + } - match self.load_from_written_path(plugin_type, target_path.clone()) { - Ok(summary) => Ok(summary), - Err(e) => { - let _ = std::fs::remove_file(&target_path); - self.try_remove_empty_plugin_dir(&target_path); - Err(e) - }, + // Prefer atomic move; fall back to copy+remove for cross-device temp dirs. + if let Err(e) = std::fs::rename(temp_path, &target_path) { + debug!( + error = %e, + from = %temp_path.display(), + to = %target_path.display(), + "rename failed; falling back to copy+remove" + ); + std::fs::copy(temp_path, &target_path).with_context(|| { + format!( + "failed to copy temp plugin file from {} to {}", + temp_path.display(), + target_path.display() + ) + })?; + let _ = std::fs::remove_file(temp_path); + } + + self.load_from_written_path(plugin_type, target_path.clone()) + })(); + + if result.is_err() { + let _ = std::fs::remove_file(&target_path); + self.try_remove_empty_plugin_dir(&target_path); } + result } fn validate_plugin_upload_target(&self, file_name: &str) -> Result<(PathBuf, PluginType)> { From 5661bcb1c1a2235e9caffe35ec0176daa6767483 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 14:46:47 +0000 Subject: [PATCH 09/14] fix(plugins): probe native plugin kind before overwriting on upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When uploading a native plugin whose kind is already loaded, the old code would move the new file to the target path (overwriting the existing library), then discover the conflict and delete the file — destroying the original plugin's .so on disk. Now load_from_temp_file probes the plugin kind from the temp file (via check_native_upload_conflict) before touching the final location. If the kind conflicts, the upload is rejected immediately and the existing library file is never overwritten. Also adds an integration test (test_duplicate_upload_preserves_existing_plugin) that verifies: duplicate upload is rejected with 422, the original .so file is preserved on disk, and the plugin remains loaded. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 62 ++++++++++++++ apps/skit/tests/plugin_integration_test.rs | 95 ++++++++++++++++++++++ 2 files changed, 157 insertions(+) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index bf8ac0b8..aa1f2f1a 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -900,6 +900,50 @@ impl UnifiedPluginManager { } } + /// Probes a native plugin library to discover its kind and checks for + /// conflicts with already-loaded plugins, without registering anything. + /// + /// Used by the upload path to detect kind conflicts *before* moving the + /// uploaded file to its final location, preventing accidental overwriting + /// of an existing plugin's library. + fn check_native_upload_conflict(&self, probe_path: &Path) -> Result<()> { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mut perms = std::fs::metadata(probe_path)?.permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(probe_path, perms)?; + } + + let plugin = LoadedNativePlugin::load(probe_path) + .with_context(|| format!("failed to probe native plugin {}", probe_path.display()))?; + let metadata = plugin.metadata(); + let original_kind = metadata.kind.clone(); + let kind = streamkit_plugin_native::namespaced_kind(&original_kind) + .with_context(|| format!("invalid plugin kind '{original_kind}'"))?; + // Close the library before the file is moved and re-opened from the + // final path. + drop(plugin); + + if self.plugins.contains_key(&kind) { + return Err(anyhow!( + "A plugin providing node '{original_kind}' (registered as '{kind}') is already loaded" + )); + } + + { + let registry = + self.engine.registry.read().map_err(|e| anyhow!("Registry lock poisoned: {e}"))?; + if registry.contains(&kind) { + return Err(anyhow!( + "Node kind '{kind}' is already registered; refusing to overwrite it with a plugin" + )); + } + } + + Ok(()) + } + /// Saves raw plugin bytes into the managed directory and loads the resulting plugin. /// Automatically detects plugin type based on file extension. /// @@ -918,6 +962,16 @@ impl UnifiedPluginManager { std::fs::write(&target_path, bytes).with_context(|| { format!("failed to write plugin file {}", target_path.display()) })?; + + // For native plugins, probe the kind to detect conflicts before + // registering. Note: if target_path already existed, the write + // above has already overwritten it. The primary upload path + // (load_from_temp_file) avoids this by probing from the temp file + // before moving. + if plugin_type == PluginType::Native { + self.check_native_upload_conflict(&target_path)?; + } + self.load_from_written_path(plugin_type, target_path.clone()) })(); @@ -955,6 +1009,14 @@ impl UnifiedPluginManager { return Err(anyhow!("temp plugin path is not a file: {}", temp_path.display())); } + // For native plugins, probe the kind from the temp file to detect + // conflicts before moving to the final location. This prevents + // overwriting an existing plugin's library file when the uploaded + // plugin's kind is already loaded. + if plugin_type == PluginType::Native { + self.check_native_upload_conflict(temp_path)?; + } + // Prefer atomic move; fall back to copy+remove for cross-device temp dirs. if let Err(e) = std::fs::rename(temp_path, &target_path) { debug!( diff --git a/apps/skit/tests/plugin_integration_test.rs b/apps/skit/tests/plugin_integration_test.rs index c0e7a87e..1082c924 100644 --- a/apps/skit/tests/plugin_integration_test.rs +++ b/apps/skit/tests/plugin_integration_test.rs @@ -773,3 +773,98 @@ async fn test_unload_plugin_after_pipeline_use() { println!("✅ Server healthy after unload"); server.shutdown().await; } + +/// Uploading a plugin whose kind is already loaded must be rejected without +/// destroying the existing plugin's library file on disk. +#[tokio::test] +async fn test_duplicate_upload_preserves_existing_plugin() { + let _ = tracing_subscriber::fmt::try_init(); + let _permit = acquire_test_permit().await; + + let Some(server) = TestServer::start().await else { + eprintln!("Skipping plugin integration tests: local TCP bind not permitted"); + return; + }; + let plugin_path = ensure_gain_plugin_built().await; + let plugin_bytes = fs::read(&plugin_path).await.expect("Failed to read plugin file"); + let file_name = plugin_path.file_name().unwrap().to_string_lossy().to_string(); + let client = reqwest::Client::new(); + let url = format!("http://{}/api/v1/plugins", server.addr); + + // First upload — should succeed. + let form = multipart::Form::new() + .part("plugin", multipart::Part::bytes(plugin_bytes.clone()).file_name(file_name.clone())); + let response = timeout(Duration::from_secs(10), client.post(&url).multipart(form).send()) + .await + .expect("Upload timed out") + .expect("Failed to upload plugin"); + assert_eq!(response.status(), StatusCode::CREATED, "First upload should succeed"); + println!("✅ First upload succeeded"); + + // Locate the installed plugin file on disk so we can verify it survives. + #[allow(clippy::used_underscore_binding)] + let plugins_dir = server._temp_dir.path().join("plugins/native"); + let installed_file = find_plugin_file(&plugins_dir).await; + let original_metadata = + fs::metadata(&installed_file).await.expect("Failed to stat installed plugin"); + let original_len = original_metadata.len(); + assert!(original_len > 0, "Installed plugin file should not be empty"); + println!("✅ Plugin installed at {}", installed_file.display()); + + // Second upload of the same plugin — should be rejected. + let form2 = multipart::Form::new() + .part("plugin", multipart::Part::bytes(plugin_bytes).file_name(file_name)); + let response2 = timeout(Duration::from_secs(10), client.post(&url).multipart(form2).send()) + .await + .expect("Second upload timed out") + .expect("Failed to send second upload"); + assert_eq!( + response2.status(), + StatusCode::UNPROCESSABLE_ENTITY, + "Second upload of same plugin kind should be rejected" + ); + println!("✅ Second upload correctly rejected"); + + // Verify the original plugin file is still on disk and intact. + let after_metadata = fs::metadata(&installed_file) + .await + .expect("Plugin file should still exist after rejected duplicate upload"); + assert_eq!( + after_metadata.len(), + original_len, + "Plugin file size should be unchanged after rejected duplicate" + ); + println!("✅ Original plugin file preserved"); + + // Verify the plugin is still loaded and functional. + let list_url = format!("http://{}/api/v1/plugins", server.addr); + let list_response = client.get(&list_url).send().await.expect("Failed to list plugins"); + assert_eq!(list_response.status(), StatusCode::OK); + let plugins: Vec = + list_response.json().await.expect("Failed to parse plugins list"); + assert_eq!(plugins.len(), 1, "Plugin should still be loaded"); + assert_eq!(plugins[0]["kind"], "plugin::native::gain"); + println!("✅ Plugin still loaded and listed"); + + server.shutdown().await; +} + +/// Find the first native plugin library file under the given directory tree. +async fn find_plugin_file(dir: &std::path::Path) -> std::path::PathBuf { + let mut stack = vec![dir.to_path_buf()]; + while let Some(current) = stack.pop() { + let mut entries = fs::read_dir(¤t).await.expect("Failed to read dir"); + while let Some(entry) = entries.next_entry().await.expect("Failed to read entry") { + let path = entry.path(); + if path.is_dir() { + stack.push(path); + } else if path.extension().and_then(|e| e.to_str()) == Some("so") + || path.extension().and_then(|e| e.to_str()) == Some("dylib") + || path.extension().and_then(|e| e.to_str()) == Some("dll") + { + return path; + } + } + } + panic!("No plugin library file found under {}", dir.display()); +} From 05cf2c8da069bc2ee3372bc6a8d85bb543c3ccbb Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 14:53:24 +0000 Subject: [PATCH 10/14] fix(plugins): only clean up target_path if file was actually placed there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error handler in load_from_temp_file was unconditionally deleting target_path on any error. When the conflict probe (check_native_upload_conflict) fails *before* the move/copy, target_path points to the pre-existing plugin's library — deleting it destroys the original. Track placement with a file_placed flag so we only remove files we created. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index aa1f2f1a..1e522e1c 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -1001,6 +1001,10 @@ impl UnifiedPluginManager { ) -> Result { let (target_path, plugin_type) = self.validate_plugin_upload_target(file_name)?; + // Track whether we actually placed a file at target_path so the error + // handler only cleans up files *we* created, not a pre-existing plugin. + let mut file_placed = false; + let result = (|| { let meta = std::fs::metadata(temp_path).with_context(|| { format!("failed to stat temp plugin file {}", temp_path.display()) @@ -1034,11 +1038,12 @@ impl UnifiedPluginManager { })?; let _ = std::fs::remove_file(temp_path); } + file_placed = true; self.load_from_written_path(plugin_type, target_path.clone()) })(); - if result.is_err() { + if result.is_err() && file_placed { let _ = std::fs::remove_file(&target_path); self.try_remove_empty_plugin_dir(&target_path); } From 84a2d3d044dbe38955cf8be36f5aecb4f44d2db5 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 15:03:04 +0000 Subject: [PATCH 11/14] fix(plugins): load_from_bytes writes to temp file before probing conflicts Mirrors the load_from_temp_file pattern: write bytes to a .tmp file, run check_native_upload_conflict on it, then rename to the final path. This prevents overwriting an existing plugin's library file when the uploaded kind is already loaded. Also adds the file_placed guard so the error handler only cleans up files that were actually placed at target_path. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 42 +++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index 1e522e1c..fa56107f 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -958,24 +958,44 @@ impl UnifiedPluginManager { pub fn load_from_bytes(&mut self, file_name: &str, bytes: &[u8]) -> Result { let (target_path, plugin_type) = self.validate_plugin_upload_target(file_name)?; - let result = (|| { - std::fs::write(&target_path, bytes).with_context(|| { - format!("failed to write plugin file {}", target_path.display()) - })?; + // Track whether we actually placed a file at target_path so the error + // handler only cleans up files *we* created, not a pre-existing plugin. + let mut file_placed = false; - // For native plugins, probe the kind to detect conflicts before - // registering. Note: if target_path already existed, the write - // above has already overwritten it. The primary upload path - // (load_from_temp_file) avoids this by probing from the temp file - // before moving. + let result = (|| { if plugin_type == PluginType::Native { - self.check_native_upload_conflict(&target_path)?; + // Write to a temporary file first, probe for kind conflicts, + // then move to the final location. This mirrors the pattern + // used by load_from_temp_file and prevents overwriting an + // existing plugin's library when the kind is already loaded. + let tmp_path = target_path.with_extension("tmp"); + std::fs::write(&tmp_path, bytes).with_context(|| { + format!("failed to write temp plugin file {}", tmp_path.display()) + })?; + + if let Err(e) = self.check_native_upload_conflict(&tmp_path) { + let _ = std::fs::remove_file(&tmp_path); + return Err(e); + } + + std::fs::rename(&tmp_path, &target_path).with_context(|| { + format!( + "failed to move temp plugin file from {} to {}", + tmp_path.display(), + target_path.display() + ) + })?; + } else { + std::fs::write(&target_path, bytes).with_context(|| { + format!("failed to write plugin file {}", target_path.display()) + })?; } + file_placed = true; self.load_from_written_path(plugin_type, target_path.clone()) })(); - if result.is_err() { + if result.is_err() && file_placed { let _ = std::fs::remove_file(&target_path); self.try_remove_empty_plugin_dir(&target_path); } From e70fe115edad3353a06f4880d623780029a5a48a Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 17:25:26 +0000 Subject: [PATCH 12/14] refactor(plugins): address review feedback on conflict detection and cleanup - Extract shared check_kind_conflict() helper used by both load_native_plugin and check_native_upload_conflict, eliminating duplicated conflict-check logic. - Extract ERR_ALREADY_LOADED / ERR_ALREADY_REGISTERED as shared constants, replacing fragile string literals in both the producer (check_kind_conflict) and consumer (load_native_dir_plugins). - Fix temp file leak in load_from_bytes: clean up .tmp file on rename failure instead of letting it accumulate on disk. - Fix empty subdirectory leak: call try_remove_empty_plugin_dir unconditionally on error (not only when file_placed is true), so subdirectories created by validate_plugin_upload_target are cleaned up even when the conflict check fails before placement. - Fix stale 'Phase 2 & 3' comment (bare .so fallback was removed). - Clarify redundant-looking permissions comment (probe file vs final file are different). - Improve dead_code annotation rationale on load_from_bytes. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 114 +++++++++++++++++++++------------------ 1 file changed, 61 insertions(+), 53 deletions(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index fa56107f..a97cfe5e 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -25,6 +25,14 @@ use crate::{ plugin_paths, plugin_records::{active_dir as plugin_active_dir, namespaced_kind as active_namespaced_kind}, }; + +/// Sentinel substring used in conflict-detection errors to distinguish +/// expected dedup skips from genuine failures. Referenced by both the +/// producer (`load_native_plugin` / `check_kind_conflict`) and the +/// consumer (`load_native_dir_plugins`). +const ERR_ALREADY_LOADED: &str = "already loaded"; +const ERR_ALREADY_REGISTERED: &str = "already registered"; + /// The type of plugin #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] #[serde(rename_all = "lowercase")] @@ -216,9 +224,10 @@ impl UnifiedPluginManager { // Phase 1: active records (marketplace-installed bundles). summaries.extend(self.load_active_plugin_records()); - // Phase 2 & 3: directory bundles + bare library files from the - // native directory. Plugins already loaded in phase 1 are - // automatically skipped (load_native_plugin checks the map). + // Phase 2: directory bundles from the native directory. + // Bare library files are warned about and skipped. + // Plugins already loaded in phase 1 are automatically skipped + // (load_native_plugin checks the map). summaries.extend(self.load_native_dir_plugins()); summaries @@ -327,13 +336,12 @@ impl UnifiedPluginManager { summaries.push(summary); }, Err(err) => { - // NOTE: This relies on the exact error wording produced by - // load_native_plugin ("already loaded" / "already registered"). - // A pre-check is not feasible here because the plugin kind - // is only known after dlopen (LoadedNativePlugin::load). - // If those messages change, update this check too. + // Uses the shared sentinel constants ERR_ALREADY_LOADED / + // ERR_ALREADY_REGISTERED produced by check_kind_conflict. + // A pre-check is not feasible because the plugin kind is + // only known after dlopen (LoadedNativePlugin::load). let msg = err.to_string(); - if msg.contains("already loaded") || msg.contains("already registered") { + if msg.contains(ERR_ALREADY_LOADED) || msg.contains(ERR_ALREADY_REGISTERED) { debug!(file = ?path, "Skipping plugin (already loaded by higher-priority source)"); } else { warn!(error = %err, file = ?path, "Failed to load native plugin from disk"); @@ -756,25 +764,7 @@ impl UnifiedPluginManager { .with_context(|| format!("invalid plugin kind '{original_kind}'"))?; let categories = metadata.categories.clone(); - if self.plugins.contains_key(&kind) { - // NOTE: load_native_dir_plugins pattern-matches on "already loaded" - // in this message to distinguish expected dedup skips from genuine - // failures. Keep the wording in sync with the check there. - return Err(anyhow!( - "A plugin providing node '{original_kind}' (registered as '{kind}') is already loaded" - )); - } - - // Ensure we don't override an existing node definition - { - let registry = - self.engine.registry.read().map_err(|e| anyhow!("Registry lock poisoned: {e}"))?; - if registry.contains(&kind) { - return Err(anyhow!( - "Node kind '{kind}' is already registered; refusing to overwrite it with a plugin" - )); - } - } + self.check_kind_conflict(&kind, &original_kind)?; // Register with the engine's node registry { @@ -900,6 +890,32 @@ impl UnifiedPluginManager { } } + /// Checks whether the given plugin kind conflicts with an already-loaded + /// plugin or an already-registered node kind. + /// + /// Error messages intentionally contain `ERR_ALREADY_LOADED` / + /// `ERR_ALREADY_REGISTERED` so that `load_native_dir_plugins` can + /// distinguish expected dedup skips from genuine failures. + fn check_kind_conflict(&self, kind: &str, original_kind: &str) -> Result<()> { + if self.plugins.contains_key(kind) { + return Err(anyhow!( + "A plugin providing node '{original_kind}' (registered as '{kind}') is {ERR_ALREADY_LOADED}" + )); + } + + { + let registry = + self.engine.registry.read().map_err(|e| anyhow!("Registry lock poisoned: {e}"))?; + if registry.contains(kind) { + return Err(anyhow!( + "Node kind '{kind}' is {ERR_ALREADY_REGISTERED}; refusing to overwrite it with a plugin" + )); + } + } + + Ok(()) + } + /// Probes a native plugin library to discover its kind and checks for /// conflicts with already-loaded plugins, without registering anything. /// @@ -907,6 +923,9 @@ impl UnifiedPluginManager { /// uploaded file to its final location, preventing accidental overwriting /// of an existing plugin's library. fn check_native_upload_conflict(&self, probe_path: &Path) -> Result<()> { + // Set executable permissions so dlopen can load the probe file. + // load_from_written_path sets permissions again on the final path + // after the move — the two calls operate on different files. #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -925,23 +944,7 @@ impl UnifiedPluginManager { // final path. drop(plugin); - if self.plugins.contains_key(&kind) { - return Err(anyhow!( - "A plugin providing node '{original_kind}' (registered as '{kind}') is already loaded" - )); - } - - { - let registry = - self.engine.registry.read().map_err(|e| anyhow!("Registry lock poisoned: {e}"))?; - if registry.contains(&kind) { - return Err(anyhow!( - "Node kind '{kind}' is already registered; refusing to overwrite it with a plugin" - )); - } - } - - Ok(()) + self.check_kind_conflict(&kind, &original_kind) } /// Saves raw plugin bytes into the managed directory and loads the resulting plugin. @@ -954,7 +957,7 @@ impl UnifiedPluginManager { /// - The plugin file cannot be written to disk /// - The plugin fails to load after being written /// - On Unix systems, setting executable permissions fails - #[allow(dead_code)] // Useful for non-streaming callers; HTTP uses `load_from_temp_file`. + #[allow(dead_code)] // Public API for non-streaming callers; HTTP handler uses load_from_temp_file. pub fn load_from_bytes(&mut self, file_name: &str, bytes: &[u8]) -> Result { let (target_path, plugin_type) = self.validate_plugin_upload_target(file_name)?; @@ -978,9 +981,10 @@ impl UnifiedPluginManager { return Err(e); } - std::fs::rename(&tmp_path, &target_path).with_context(|| { - format!( - "failed to move temp plugin file from {} to {}", + std::fs::rename(&tmp_path, &target_path).map_err(|e| { + let _ = std::fs::remove_file(&tmp_path); + anyhow!( + "failed to move temp plugin file from {} to {}: {e}", tmp_path.display(), target_path.display() ) @@ -995,8 +999,10 @@ impl UnifiedPluginManager { self.load_from_written_path(plugin_type, target_path.clone()) })(); - if result.is_err() && file_placed { - let _ = std::fs::remove_file(&target_path); + if result.is_err() { + if file_placed { + let _ = std::fs::remove_file(&target_path); + } self.try_remove_empty_plugin_dir(&target_path); } result @@ -1063,8 +1069,10 @@ impl UnifiedPluginManager { self.load_from_written_path(plugin_type, target_path.clone()) })(); - if result.is_err() && file_placed { - let _ = std::fs::remove_file(&target_path); + if result.is_err() { + if file_placed { + let _ = std::fs::remove_file(&target_path); + } self.try_remove_empty_plugin_dir(&target_path); } result From c6dd132bc96dcc58f279fd19cb6cb41e3866a07a Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 18:08:52 +0000 Subject: [PATCH 13/14] feat(plugins): log both versions when skipping duplicate plugin Bump the dedup-skip log from debug to warn and include both the loaded (marketplace) version and the skipped (local) version read from plugin.yml manifests. This helps operators spot cases where a marketplace install is outdated relative to a locally patched plugin. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index a97cfe5e..9c1840fb 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -22,6 +22,7 @@ use tracing::{debug, info, warn}; use crate::{ marketplace::PluginKind, + plugin_assets::read_local_plugin_manifest, plugin_paths, plugin_records::{active_dir as plugin_active_dir, namespaced_kind as active_namespaced_kind}, }; @@ -342,7 +343,31 @@ impl UnifiedPluginManager { // only known after dlopen (LoadedNativePlugin::load). let msg = err.to_string(); if msg.contains(ERR_ALREADY_LOADED) || msg.contains(ERR_ALREADY_REGISTERED) { - debug!(file = ?path, "Skipping plugin (already loaded by higher-priority source)"); + // Read both versions from manifests so operators can + // spot outdated marketplace installs vs patched locals. + let local_manifest = read_local_plugin_manifest(&path); + let local_version = + local_manifest.as_ref().map_or("unknown", |m| m.version.as_str()); + let (loaded_kind, loaded_version) = local_manifest + .as_ref() + .and_then(|m| { + let kind = + streamkit_plugin_native::namespaced_kind(&m.node_kind).ok()?; + let ver = self + .plugins + .get(&kind) + .and_then(|p| p.version.as_deref()) + .unwrap_or("unknown"); + Some((kind, ver.to_owned())) + }) + .unwrap_or_else(|| ("unknown".into(), "unknown".into())); + warn!( + file = ?path, + plugin = %loaded_kind, + loaded_version = %loaded_version, + skipped_version = %local_version, + "Skipping local plugin (already loaded by higher-priority source)" + ); } else { warn!(error = %err, file = ?path, "Failed to load native plugin from disk"); } From 1ba0a4135d9997804ed361b8df29795cbf2b24e6 Mon Sep 17 00:00:00 2001 From: StreamKit Devin Date: Wed, 8 Apr 2026 20:06:15 +0000 Subject: [PATCH 14/14] fix(plugins): avoid probing native uploads from noexec temp mounts Move the uploaded temp file into the plugin subdirectory (.tmp extension) before dlopen-probing it. The original temp file may reside on a noexec mount (e.g. /tmp on hardened hosts), causing dlopen to fail even though .plugins/native/ would work fine. The probe file is cleaned up on conflict or rename failure, and an existing plugin's library is never overwritten until the probe passes. Signed-off-by: StreamKit Devin Co-Authored-By: Claudio Costa --- apps/skit/src/plugins.rs | 70 +++++++++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 19 deletions(-) diff --git a/apps/skit/src/plugins.rs b/apps/skit/src/plugins.rs index 9c1840fb..8495d0a9 100644 --- a/apps/skit/src/plugins.rs +++ b/apps/skit/src/plugins.rs @@ -1064,30 +1064,62 @@ impl UnifiedPluginManager { return Err(anyhow!("temp plugin path is not a file: {}", temp_path.display())); } - // For native plugins, probe the kind from the temp file to detect - // conflicts before moving to the final location. This prevents - // overwriting an existing plugin's library file when the uploaded - // plugin's kind is already loaded. if plugin_type == PluginType::Native { - self.check_native_upload_conflict(temp_path)?; - } + // Move the temp file into the plugin subdirectory *before* + // probing. The original temp file may live on a noexec mount + // (e.g. /tmp), which would cause dlopen to fail even though + // .plugins/native/ is fine. Using a .tmp extension keeps it + // separate from the final target so a pre-existing plugin is + // never overwritten until the probe passes. + let probe_path = target_path.with_extension("tmp"); + if let Err(e) = std::fs::rename(temp_path, &probe_path) { + debug!( + error = %e, + from = %temp_path.display(), + to = %probe_path.display(), + "rename to probe path failed; falling back to copy+remove" + ); + std::fs::copy(temp_path, &probe_path).with_context(|| { + format!( + "failed to copy temp plugin file from {} to {}", + temp_path.display(), + probe_path.display() + ) + })?; + let _ = std::fs::remove_file(temp_path); + } - // Prefer atomic move; fall back to copy+remove for cross-device temp dirs. - if let Err(e) = std::fs::rename(temp_path, &target_path) { - debug!( - error = %e, - from = %temp_path.display(), - to = %target_path.display(), - "rename failed; falling back to copy+remove" - ); - std::fs::copy(temp_path, &target_path).with_context(|| { - format!( - "failed to copy temp plugin file from {} to {}", - temp_path.display(), + if let Err(e) = self.check_native_upload_conflict(&probe_path) { + let _ = std::fs::remove_file(&probe_path); + return Err(e); + } + + std::fs::rename(&probe_path, &target_path).map_err(|e| { + let _ = std::fs::remove_file(&probe_path); + anyhow!( + "failed to move probe file from {} to {}: {e}", + probe_path.display(), target_path.display() ) })?; - let _ = std::fs::remove_file(temp_path); + } else { + // WASM plugins don't need dlopen probing; move directly. + if let Err(e) = std::fs::rename(temp_path, &target_path) { + debug!( + error = %e, + from = %temp_path.display(), + to = %target_path.display(), + "rename failed; falling back to copy+remove" + ); + std::fs::copy(temp_path, &target_path).with_context(|| { + format!( + "failed to copy temp plugin file from {} to {}", + temp_path.display(), + target_path.display() + ) + })?; + let _ = std::fs::remove_file(temp_path); + } } file_placed = true;