fix(plugins): stop watcher from clobbering loader state during install - #87
Conversation
A store install (install or update) could leave the user with a "success" toast followed by the plugin silently absent from memory — `plugin_list` shows it as not installed, `resolve_url` for a matching URL returns "No media plugin installed", and the plugin only comes back after an app restart (which re-runs the startup scan). Root cause: the plugin watcher runs in recursive mode on `plugins_dir` and receives events for the install's own filesystem writes — `remove_dir_all` on the destination fires REMOVE events, the copy loop fires CREATE/MODIFY events, and the staging directory itself used to live at `plugins_dir/.staging/`, adding another wave of spurious events per install. Those events race the handler's final `self.load()` call: the handler returns `Ok` before the watcher has drained its queue, and when the watcher eventually processes a late REMOVE event (or a CREATE that fires before a `.wasm` is fully on disk and fails `parse_manifest`), it ends by unloading what the install just loaded. Three changes, one PR: 1. **Move staging outside `plugins_dir`** `store_staging_dir` is now `app_data_dir/plugin-staging/` — a sibling of `plugins_dir` rather than a subdirectory. The watcher no longer sees any events for the download half of an install. A one-shot cleanup removes the legacy `plugins_dir/.staging/` on startup so users upgrading don't keep the cruft around. 2. **Suppress watcher events for plugins under install** `ExtismPluginLoader` now tracks a `DashSet<String>` of plugins whose `load_from_dir` is in flight. The watcher consults `is_install_in_progress(name)` and skips the event if true. An RAII guard clears the flag even if the body returns `Err` or panics, so a failed install can't leave the watcher permanently deaf for that plugin. 3. **Clean up staging after a successful install** Best-effort `remove_dir_all(staging_for_cleanup)` once `load_from_dir` has returned `Ok`. Prevents the staging root from accumulating one subdir per install for the lifetime of the app data directory. Tests: two new watcher tests cover the suppression for both CREATE and REMOVE events (the latter is the exact case that motivated the fix — destination `remove_dir_all` firing after the handler's load). All 635 lib tests pass, clippy clean.
📝 WalkthroughWalkthroughAdds per-plugin install coordination (refcount + serializer) to the loader, suppresses watcher reactions during installs, moves plugin staging directory, updates store-install to rely on loader for unload and perform best-effort cleanup, and adds unit tests for suppression and watcher behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Installer as Installer (store_install)
participant Loader as ExtismPluginLoader
participant FS as Filesystem
participant Watcher as Filesystem Watcher
Installer->>Loader: load_from_dir(plugin_name)
Loader->>Loader: mark_install_in_progress(plugin_name) %% increment refcount + acquire serializer
Loader->>FS: stage/extract files -> dest
FS->>Watcher: emit create/modify/remove events
Watcher->>Loader: is_install_in_progress(plugin_name)?
alt true
Watcher->>Watcher: suppress reload/unload (skip)
else false
Watcher->>Loader: trigger load/reload/unload
end
Loader->>Loader: complete install (drop guard -> decrement refcount / release serializer)
Installer->>FS: cleanup staging dir (best-effort)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs`:
- Around line 23-27: The installs_in_progress DashSet<String> only records
presence and allows two overlapping installs for the same plugin to share
staging/destination and race when one InstallInFlight is dropped; replace this
boolean-style set with a per-plugin serialization mechanism (e.g., a map from
plugin name to a mutex or a ref-counted guard) so that functions that start
installs (where installs_in_progress is currently used, and the InstallInFlight
guard type) acquire a per-name lock (or increment a refcounted entry) before
staging and release it only when that specific install completes; update uses in
load_from_dir, InstallInFlight creation/drop and watcher skip checks to consult
the per-plugin guard so same-name installs cannot interleave.
In `@src-tauri/src/adapters/driven/plugin/watcher.rs`:
- Around line 89-93: The tracing::warn! invocation in watcher.rs is failing
rustfmt because its format string uses "{e}" but the call supplies positional
args; change the macro to use positional placeholders and pass e as an argument
so the call is a single, fmt-friendly expression: replace tracing::warn!("failed
to parse manifest at {}: {e}", plugin_dir.display()); with a single-line call
like tracing::warn!("failed to parse manifest at {}: {}", plugin_dir.display(),
e); ensuring the warning uses plugin_dir.display() and the error variable e as
positional arguments.
- Around line 48-57: The current loop filters paths by
is_plugin_toml/is_wasm_file before checking parent directories, so
RemoveKind::Folder events (directory removals) can be skipped and plugins remain
loaded; move or add logic to detect directory removals before the file-type
filter: for each path in event.paths, compute plugin_dir = path.parent() and
dir_name = plugin_dir.file_name().and_then(|n| n.to_str()) first, then if
event.kind matches EventKind::Remove(RemoveKind::Folder) call the existing
unload branch using dir_name, otherwise proceed to the
is_plugin_toml/is_wasm_file checks for file-level handling; update or add tests
to include folder-deletion events to cover this case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21f3740e-4cf3-43e6-b8ba-9988938913ac
📒 Files selected for processing (4)
src-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/watcher.rssrc-tauri/src/application/commands/store_install.rssrc-tauri/src/lib.rs
| Err(e) => { | ||
| tracing::warn!( | ||
| "failed to parse manifest at {}: {e}", | ||
| plugin_dir.display() | ||
| ); |
There was a problem hiding this comment.
Rustfmt is still failing on this tracing::warn! call.
CI already flags this block; matching rustfmt’s single-line output here will unblock cargo fmt --check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/adapters/driven/plugin/watcher.rs` around lines 89 - 93, The
tracing::warn! invocation in watcher.rs is failing rustfmt because its format
string uses "{e}" but the call supplies positional args; change the macro to use
positional placeholders and pass e as an argument so the call is a single,
fmt-friendly expression: replace tracing::warn!("failed to parse manifest at {}:
{e}", plugin_dir.display()); with a single-line call like tracing::warn!("failed
to parse manifest at {}: {}", plugin_dir.display(), e); ensuring the warning
uses plugin_dir.display() and the error variable e as positional arguments.
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/adapters/driven/plugin/extism_loader.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/extism_loader.rs:83">
P2: Using a `DashSet` for in-flight installs is not safe for overlapping installs of the same plugin name; one completion can clear suppression while another install is still running.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR fixes a real race condition that left plugins unloaded in memory after a successful store install/update. The three-pronged fix — moving staging outside Confidence Score: 5/5Safe to merge — the race condition is correctly fixed and all remaining findings are P2 style suggestions. All P1 concerns (timing gap between unload and suppression flag) from the prior review are resolved by moving both operations inside No files require special attention.
|
| Filename | Overview |
|---|---|
| src-tauri/src/adapters/driven/plugin/extism_loader.rs | Adds per-plugin InstallState (serializer + AtomicUsize refcount) with RAII InstallInFlight guard; moves unload() inside the suppression window inside load_from_dir; adds test helpers and a new refcount-overlap test. |
| src-tauri/src/adapters/driven/plugin/watcher.rs | Refactors event handling into handle_upsert_event/handle_remove_event helpers; adds plugin_name_from_plugin_file and plugin_name_from_plugin_dir path-validation helpers; suppresses events via is_install_in_progress; adds four new watcher unit tests. |
| src-tauri/src/application/commands/store_install.rs | Removes explicit pre-unload call (now handled inside load_from_dir within the suppression window); adds best-effort staging cleanup after successful install. |
| src-tauri/src/lib.rs | Moves store_staging_dir to app_data_dir/plugin-staging/ (outside plugins_dir) and adds one-shot cleanup of the legacy plugins_dir/.staging/ on startup. |
Sequence Diagram
sequenceDiagram
participant Handler as store_install handler
participant BlockingThread as spawn_blocking thread
participant Loader as ExtismPluginLoader
participant Registry as PluginRegistry
participant FS as Filesystem
participant Watcher as PluginWatcher task
Handler->>BlockingThread: spawn_blocking(load_from_dir)
BlockingThread->>Loader: load_from_dir(staging_dir)
Loader->>Loader: parse_manifest(staging_dir)
Loader->>Loader: fetch_add(refcount, 1) — watcher suppression ON
Loader->>Registry: unload(name) — inside suppression window
Loader->>FS: remove_dir_all(plugins_dir/name)
FS-->>Watcher: REMOVE events queued in channel
Watcher->>Loader: is_install_in_progress(name) → true → skip
Loader->>FS: copy files staging_dir → plugins_dir/name
FS-->>Watcher: CREATE events queued in channel
Watcher->>Loader: is_install_in_progress(name) → true → skip
Loader->>Registry: load(manifest) — plugin inserted
Loader-->>BlockingThread: Ok(())
Note over Loader: InstallInFlight drops → fetch_sub(refcount, 1) — suppression OFF
BlockingThread-->>Handler: Ok(())
Handler->>FS: remove_dir_all(staging_dir) — outside plugins_dir, no watcher events
Handler-->>Handler: Ok — toast shown
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src-tauri/src/adapters/driven/plugin/extism_loader.rs
Line: 89-107
Comment:
**`installs` map entries accumulate indefinitely**
`get_or_create_install_state` inserts entries into the `HashMap` but nothing ever removes them. Once an `InstallState` is created for a plugin name, it stays in the map with `count == 0` for the lifetime of the process. In practice the map is bounded by the number of distinct plugin names ever installed in a session, so this isn't a memory emergency — but a simple `retain` after the refcount drops to zero inside `InstallInFlight::drop` (or a periodic cleanup of zero-count entries) would keep the map from growing without bound in long-running sessions.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src-tauri/src/adapters/driven/plugin/extism_loader.rs
Line: 132-145
Comment:
**`unmark` helper can silently underflow the refcount**
`unmark_install_in_progress_for_testing` decrements the `AtomicUsize` without checking that it is greater than zero. If a test calls it more times than the paired `mark_…` helper, the counter wraps to `usize::MAX` and `is_install_in_progress` returns `true` permanently for that plugin name, making any future suppression-check assertions in the same test suite meaningless. A `debug_assert!(count > 0)` before the `fetch_sub`, or a `saturating_sub`, would catch this class of test-helper misuse early.
```suggestion
#[cfg(test)]
pub fn unmark_install_in_progress_for_testing(&self, name: &str) {
let map = self
.installs
.lock()
.unwrap_or_else(|poisoned| poisoned.into_inner());
if let Some(state) = map.get(name) {
let prev = state.count.fetch_sub(1, Ordering::SeqCst);
debug_assert!(prev > 0, "unmark called more times than mark for '{name}'");
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (4): Last reviewed commit: "fix: address PR review comments" | Re-trigger Greptile
- Switch `installs_in_progress` from `DashSet<String>` to a `Mutex<HashMap<String, Arc<InstallState>>>` where each entry carries a per-plugin serializer mutex and a refcount. Two concurrent installs of the same plugin name now serialize on the mutex (no more racing on staging/destination writes) and the watcher's suppression stays active until the last install drops (the first guard's drop no longer prematurely clears the flag while the second install is still running). CodeRabbit + cubic. - Move the idempotent `unload()` call from `handle_store_install` into `load_from_dir` so it runs *inside* the suppression window. Closes the timing gap Greptile flagged where a delayed watcher event fired between the handler's `unload()` and `load_from_dir`'s flag insertion could re-insert the plugin and make the final `self.load()` fail with `AlreadyExists`. - Handle `EventKind::Remove(RemoveKind::Folder)` in the plugin watcher. macOS FSEvents and occasionally Windows ReadDirectoryChangesW emit a single Remove event for the plugin directory itself rather than per-file removes; the previous file-extension filter silently dropped those and left the plugin loaded until restart. The remove handler now tries both candidate plugin names (parent dir for file removes, path's own last segment for folder removes) and `unload()`'s NotFound is logged at debug level for false positives. - Fix the rustfmt failure on `watcher.rs` by splitting `handle_fs_event` into `handle_upsert_event` + `handle_remove_event` — the format re-emerges naturally from the smaller scopes, and the split makes the branch logic easier to read. Two new unit tests: one asserts folder-level remove unloads the plugin, one asserts two overlapping installs of the same plugin both hold the suppression active until the last drop.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/plugin/extism_loader.rs (1)
117-137: Test helpers look reasonable.Minor note:
unmark_install_in_progress_for_testingdoesn't guard against underflow if called more times thanmark. This could wrap tousize::MAXand break test assertions. Current tests use these correctly, so not a blocker—but consider adding a debug assertion if this helper sees wider use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs` around lines 117 - 137, unmark_install_in_progress_for_testing can underflow the atomic refcount if called too many times; update its implementation (the unmark_install_in_progress_for_testing function that accesses state.count) to guard against underflow by checking the current count before decrementing (e.g., use fetch_update to only decrement when > 0, or read with load and conditionally call fetch_sub), or add a debug_assert that the count is > 0 prior to fetch_sub; ensure you reference the same atomic (state.count) so tests keep deterministic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/extism_loader.rs`:
- Around line 117-137: unmark_install_in_progress_for_testing can underflow the
atomic refcount if called too many times; update its implementation (the
unmark_install_in_progress_for_testing function that accesses state.count) to
guard against underflow by checking the current count before decrementing (e.g.,
use fetch_update to only decrement when > 0, or read with load and conditionally
call fetch_sub), or add a debug_assert that the count is > 0 prior to fetch_sub;
ensure you reference the same atomic (state.count) so tests keep deterministic
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9502ab55-c1cf-4ec1-885e-1d1e471f8381
📒 Files selected for processing (3)
src-tauri/src/adapters/driven/plugin/extism_loader.rssrc-tauri/src/adapters/driven/plugin/watcher.rssrc-tauri/src/application/commands/store_install.rs
✅ Files skipped from review due to trivial changes (1)
- src-tauri/src/application/commands/store_install.rs
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/adapters/driven/plugin/watcher.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/watcher.rs:130">
P1: Remove handling can unload the wrong plugin because folder fallback uses any removed path basename in recursive watch events.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Restrict the watcher's plugin-name extraction to paths that are direct children of `plugins_dir`. A recursive watch can otherwise fire a `Remove(Folder)` event for a nested subdirectory whose leaf name coincidentally matches a loaded plugin, and the previous folder fallback — which just took `path.file_name()` without checking depth — would unload that plugin by mistake (cubic P1). Both `handle_upsert_event` and `handle_remove_event` now route through a shared `plugin_name_from_event_path` helper that only accepts: - plugins_dir/<name>/plugin.toml | plugins_dir/<name>/*.wasm - plugins_dir/<name>/ (folder, for FSEvents etc.) Anything deeper or outside `plugins_dir` returns `None` and the handler bails early. Added `test_handle_fs_event_remove_nested_path_does_not_unload_plugin` to lock the invariant down.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/plugin/watcher.rs (1)
100-128: Minor: Directory name vs manifest name mismatch.The install-in-progress check (line 106) uses the directory-derived
name, but unload (line 123) usesmanifest_namefrom the parsed manifest. If the manifest declares a different name than the directory, the suppression check may not match what's actually being unloaded.This is likely fine if the convention is that directory name always equals manifest name, but worth documenting or asserting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/adapters/driven/plugin/watcher.rs` around lines 100 - 128, In handle_upsert_event, the install-in-progress check uses the directory-derived name from plugin_name_from_event_path but unload/load use manifest.info().name(), which can diverge; to fix, parse the manifest first (call parse_manifest(&plugin_dir)) and derive manifest_name via manifest.info().name(), then use that manifest_name for loader.is_install_in_progress, loader.unload, and loader.load (or else assert directory name == manifest_name and log if not); update the control flow in handle_upsert_event to prefer manifest_name consistently when interacting with loader methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/adapters/driven/plugin/watcher.rs`:
- Around line 100-128: In handle_upsert_event, the install-in-progress check
uses the directory-derived name from plugin_name_from_event_path but unload/load
use manifest.info().name(), which can diverge; to fix, parse the manifest first
(call parse_manifest(&plugin_dir)) and derive manifest_name via
manifest.info().name(), then use that manifest_name for
loader.is_install_in_progress, loader.unload, and loader.load (or else assert
directory name == manifest_name and log if not); update the control flow in
handle_upsert_event to prefer manifest_name consistently when interacting with
loader methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 760c9ce5-c830-4c38-a3ae-5a8042a6cc44
📒 Files selected for processing (1)
src-tauri/src/adapters/driven/plugin/watcher.rs
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/adapters/driven/plugin/watcher.rs">
<violation number="1" location="src-tauri/src/adapters/driven/plugin/watcher.rs:102">
P2: `handle_upsert_event` now accepts folder/direct-child events, so non-plugin create/modify events can incorrectly trigger plugin reload logic. Restrict upsert handling back to plugin file paths only.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Restrict upsert handling to plugin file paths only. After the previous
refactor routed both branches through the same
`plugin_name_from_event_path` helper, a CREATE or MODIFY event on any
direct child of `plugins_dir` (e.g. a stray `.txt` the user dropped in)
was getting forwarded to `parse_manifest`, which would predictably fail
and log a warning (cubic P2).
Split the plumbing into two helpers:
- `plugin_name_from_plugin_file` — matches only
`plugins_dir/<name>/plugin.toml` or `…/<name>.wasm`. Used by
`handle_upsert_event` (file events are the only ones that carry
loadable content).
- `plugin_name_from_plugin_dir` — matches `plugins_dir/<name>`
itself. Combined with the file helper for `handle_remove_event`,
which must still respond to folder-level removes (macOS FSEvents
may coalesce a plugin deletion into a single `Remove(Folder)`).
Added `test_handle_fs_event_upsert_ignores_non_plugin_file_events` to
cover the stray-file case.
Summary
Store installs (install and update) could leave users with a "success" toast followed by the plugin silently absent from memory:
plugin_listreports the plugin as not installed.resolve_urlreturnsNotFound, so any matching URL surfaces the "No media plugin installed for this URL" error.This was observed and reproduced immediately after merging #86: the vimeo v1.1.1 release got installed correctly, the success toast fired, then trying to download a vimeo URL produced the "install the plugin" error. Disk state was correct; in-memory state was empty.
Root cause
The plugin watcher uses
RecursiveMode::Recursiveonplugins_dir, so it receives events for the install's own filesystem writes:download_pluginused to write toplugins_dir/.staging/<name>/— the watcher fires CREATE/MODIFY events for every staged file.load_from_dircallsremove_dir_allon the destination (fires REMOVE events), then copies the staged files back (fires CREATE/MODIFY events).self.load()inserts the new version and returnsOkto the UI..wasmfile is fully written failsparse_manifestand logs a warning without recovering. End state: registry empty, UI says "not installed."Fix (three changes, one PR)
1. Move staging outside
plugins_dirstore_staging_diris nowapp_data_dir/plugin-staging/— a sibling ofplugins_dir, not a child. The watcher no longer sees any of the download-half events.A one-shot cleanup removes the legacy
plugins_dir/.staging/on startup, so upgrading users don't leave the cruft around.2. Suppress watcher events for plugins under install
ExtismPluginLoadernow tracks aDashSet<String>of plugins whoseload_from_diris in flight. The watcher consultsis_install_in_progress(name)and skips the event when true. An RAII guard (InstallInFlightstruct withDropimpl) clears the flag even if the body returnsError panics — so a failed install can't leave the watcher permanently deaf for that plugin.3. Clean up staging after a successful install
Best-effort
remove_dir_all(staging_for_cleanup)afterload_from_dirreturnsOk. Prevents the staging root from growing by one subdirectory per install forever.Why the suppression belongs in the loader, not the watcher
The watcher doesn't know why a burst of events happened. The loader is the component that initiated them (via
remove_dir_all+ copy). Owning the suppression flag there keeps the ordering guarantees colocated with the code that creates the race, and the RAII guard makes the invariant impossible to leak.Test plan
cargo test --lib— 634 passed, 4 ignored; 2 new watcher tests cover CREATE and REMOVE suppression (the REMOVE test is the exact case that motivated the fix).cargo clippy --all-targets -- -D warnings— clean~/.local/share/dev.vortex.app/plugins/.staging/is gone after next app launch (legacy cleanup) and thatplugin-staging/stays empty between installs.Trace of the original bug, for the record
The memory state vs. disk state divergence is exactly what the watcher-suppression flag prevents.
Summary by cubic
Fixes a race where the plugin watcher unloaded a just-installed plugin and tightens watcher filtering to avoid spurious reloads. Installs and updates now keep the plugin loaded immediately, and deleting a plugin folder correctly unloads it.
app_data_dir/plugin-staging/and cleaned legacyplugins_dir/.staging/on startup; staging is removed after a successful install.ExtismPluginLoader; the watcher checksis_install_in_progressand skips events until the last install finishes.load_from_dirnow unloads any prior instance inside the suppression window; removed the pre-unload from the store install handler to close a timing gap.Remove(Folder)and reacts only toplugins_dir/<name>/plugin.tomlor*.wasmfor upserts, and toplugins_dir/<name>for folder removes; ignores nested paths and non-plugin files to prevent false unloads and noisy parse failures.Written for commit 14c0834. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Bug Fixes
Tests