From 7abd11d19db2dae884b05e69ad8a9398b2fb603a Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Fri, 13 Nov 2026 21:07:28 -0800 Subject: [PATCH] Plug-ins: windows and processes take turns on the Plug-ins folder Two windows or processes using one Plug-ins folder could interleave their writes: an install from one could leave its module with the other's grant, a grant written for one plug-in could drop one written for another, and plug-in settings and cache changes could lose each other's keys or pass the cache caps together. Each change to the folder now runs in one turn on disabled.lock, the file the disabled list was already locked with: - plugin.install takes the turn before it reads the stored grants and holds it through the module, grant, put-back and disabled-list writes; plugin.remove holds it through grant removal and storage deletion. - Grant writes, the disabled list, loading the folder, and the plug-ins' settings and cache changes read and write in a turn. - When disabled.lock can't be locked, plugin.install and plugin.remove fail and change nothing, plugin.setEnabled keeps its change in the process, and loading the folder writes nothing to it. Docs: docs/plugins.md, docs/mcp.md, the plugin.install and plugin.remove params text, docs/target-app-parity.md. --- crates/engine/src/cmd/plugin.rs | 127 ++++++++++------ crates/engine/src/store.rs | 233 ++++++++++++++++++++++------- crates/engine/src/tests/plugins.rs | 214 +++++++++++++++++++++++++- crates/engine/tests/plugin_fuzz.rs | 5 +- docs/mcp.md | 4 +- docs/plugins.md | 8 +- docs/target-app-parity.md | 1 + 7 files changed, 482 insertions(+), 110 deletions(-) diff --git a/crates/engine/src/cmd/plugin.rs b/crates/engine/src/cmd/plugin.rs index 3fa082bdb0..867f256b4c 100644 --- a/crates/engine/src/cmd/plugin.rs +++ b/crates/engine/src/cmd/plugin.rs @@ -51,6 +51,7 @@ use vectorcraft_doc::{Effect, Node}; use vectorcraft_plugins::{Budget, Command, Kind, Plugin, effect, export, objects, registry, safe_mode}; use super::{CommandSpec, EngineError, NodeId, Result, Session, always, bad, cmd, has_doc, ids_param, str_param, switch_or}; +use crate::store::StoreTurn; use vectorcraft_plugins::permissions::{self, Grant, GrantToken, Permission}; @@ -683,6 +684,9 @@ thread_local! { /// Tests: runs once in the next `plugin.install` on this thread after it installs and before it /// writes the store. pub(crate) static BEFORE_STORE: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; + /// Tests: runs once on this thread when [`keep_installed`] has written the module to the store + /// and before it writes the grant. + pub(crate) static BEFORE_GRANT_STORED: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; /// Tests: runs once on this thread when [`hold_plugin_changes`] finds the lock held and waits. pub(crate) static WHILE_WAITING: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; } @@ -718,7 +722,9 @@ pub(crate) fn hold_plugin_changes() -> std::sync::MutexGuard<'static, ()> { /// the next start → its `plugin.list` entry. A refused install changes neither the plug-in store /// nor the plug-ins and grants of the process. A module that lists no permissions ends any grant /// held for its id. A `plugin.remove` in another session waits until the install has written the -/// store. +/// store. With a plug-in store, all of it happens in a turn on the store ([`StoreTurn`]), so another +/// process using the same folder changes it before or after; when the store can't be locked, nothing +/// is installed. fn install(s: &mut Session, p: &Value) -> Result { const C: &str = "plugin.install"; // One use: spent whatever this install does. @@ -736,8 +742,11 @@ fn install(s: &mut Session, p: &Value) -> Result { { return Err(bad(C, "the module doesn't have the SHA-256 `hash` gives: nothing was installed")); } - // A removal in another session waits until the store holds what this install wrote. + // A removal in another session waits until the store holds what this install wrote, and so + // does a change of the store by another process. let _held = hold_plugin_changes(); + let turn = + s.plugin_store_turn().map_err(|e| EngineError::Other(format!("{C}: {}: {e}: nothing was installed", crate::store::STORE_NOT_LOCKED)))?; // The grant the checks below count on, the approval included. The install checks it again, // with the plug-in installed under the id, before anything is stored: another session may // change either in between. @@ -844,41 +853,12 @@ fn install(s: &mut Session, p: &Value) -> Result { } // The plug-in gets the settings and cache kept for its id. A module without permissions takes // the id over, so nothing the user approved for the id applies any more. - let installed = registry::install_checked(plugin, update, check, || s.attach_plugin_storage(&id), err)?; + let installed = registry::install_checked(plugin, update, check, || s.attach_plugin_storage(&turn, &id), err)?; #[cfg(test)] if let Some(between) = BEFORE_STORE.take() { between(); } - // Installed plug-ins stay installed after a restart (#2541). The module is written first and its - // grant only once it is there; when the grant can't be written, the store gets the module it - // held back. A failed write of that module is only logged: the store then holds the new module, - // which the grant doesn't cover. A write that reports a failure may still have put the old - // module back, so the store is read again to tell which module it holds. A stored module that - // can't be read back isn't replaced by one that needs a grant. - let previous = s.stored_plugin_bytes(&id); - let mut stored = (previous.is_ok() || !asks) && s.store_plugin(&id, &bytes); - let holds_new = |s: &Session| s.stored_plugin_bytes(&id).is_ok_and(|held| held.as_deref() == Some(&bytes[..])); - if asks { - if stored && !s.store_grant(&id, Some(g)) { - stored = !s.put_back_plugin(&id, previous.ok().flatten().as_deref()) && holds_new(s); - } - } else { - // When the write failed, the old stored module stays in the folder and needs its grant at - // the next launch. A grant that can't be removed still approves the old bytes, so the store - // gets the module it held back. When that module can't be read or written back, or there - // was none, no module goes with the grant, and the grant stops counting for every session in - // the process attached to the folder. A grants file that is too large or doesn't hold a list - // approves nothing, and the new module stays; one that can't be opened or read may approve - // the old bytes once it reads again. - let writes = s.grant_writes(); - if stored && !s.store_grant(&id, None) && s.holds_plugin_grant(&id) { - let back = previous.as_ref().is_ok_and(|held| s.put_back_plugin(&id, held.as_deref())); - stored = !back && holds_new(s); - if !back || matches!(previous, Ok(None)) { - s.ignore_stored_grant(&id, writes); - } - } - } + let stored = keep_installed(s, &turn, &id, &bytes, asks.then_some(g)); // Installing a disabled plug-in again (an updated version) turns it on for the next launch too, // also when it is disabled by its stored file alone (it stopped a launch while loading). When // the new version could not replace the stored file, the old one stays disabled. @@ -891,8 +871,9 @@ fn install(s: &mut Session, p: &Value) -> Result { } } if changed { - s.store_disabled_plugins(); + s.store_disabled_plugins_in(&turn); } + drop(turn); let mut v = describe_installed(&installed); if !unknown.is_empty() { v["unknownCommands"] = json!(unknown); @@ -900,6 +881,53 @@ fn install(s: &mut Session, p: &Value) -> Result { Ok(v) } +/// Keeps the plug-in `id` that `plugin.install` installed, its module `bytes`, in the plug-in store, +/// in `turn`, with `grant` when its module lists permissions (`None`: it lists none, and the grant +/// held for the id is removed) → whether the store now holds the new module (`false` without a +/// store). +/// +/// Installed plug-ins stay installed after a restart (#2541). The module is written first and its +/// grant only once it is there; when the grant can't be written, the store gets the module it held +/// back. A failed write of that module is only logged: the store then holds the new module, which +/// the grant doesn't cover. A write that reports a failure may still have put the old module back, +/// so the store is read again to tell which module it holds. A stored module that can't be read back +/// isn't replaced by one that needs a grant. +pub(crate) fn keep_installed(s: &Session, turn: &StoreTurn, id: &str, bytes: &[u8], grant: Option) -> bool { + let asks = grant.is_some(); + let previous = s.stored_plugin_bytes(id); + let mut stored = (previous.is_ok() || !asks) && s.store_plugin(turn, id, bytes); + #[cfg(test)] + if let Some(between) = BEFORE_GRANT_STORED.take() { + between(); + } + let holds_new = |s: &Session| s.stored_plugin_bytes(id).is_ok_and(|held| held.as_deref() == Some(bytes)); + match grant { + Some(g) => { + if stored && !s.store_grant_in(turn, id, Some(g)) { + stored = !s.put_back_plugin(turn, id, previous.ok().flatten().as_deref()) && holds_new(s); + } + } + None => { + // When the write failed, the old stored module stays in the folder and needs its grant + // at the next launch. A grant that can't be removed still approves the old bytes, so the + // store gets the module it held back. When that module can't be read or written back, or + // there was none, no module goes with the grant, and the grant stops counting for every + // session in the process attached to the folder. A grants file that is too large or + // doesn't hold a list approves nothing, and the new module stays; one that can't be + // opened or read may approve the old bytes once it reads again. + let writes = s.grant_writes(); + if stored && !s.store_grant_in(turn, id, None) && s.holds_plugin_grant(id) { + let back = previous.as_ref().is_ok_and(|held| s.put_back_plugin(turn, id, held.as_deref())); + stored = !back && holds_new(s); + if !back || matches!(previous, Ok(None)) { + s.ignore_stored_grant(id, writes); + } + } + } + } + stored +} + /// `plugin.inspect`: what installing the module would install, without installing it, whether that /// needs the user to grant its permissions first (`needsApproval`) and whether it would take /// another module's place (`needsReplace`). @@ -992,15 +1020,17 @@ fn load_folder(s: &Session, folder: &str) -> Value { None if crate::file_access::current().is_none() => Ok(()), None => Err(format!("automation path rejected: {} isn't valid text", f.display())), }; - // A `plugin.install` in another session finishes writing the store first. + // A `plugin.install` in another session or process finishes writing the store first. When the + // store can't be locked, the folder still loads, and the store isn't written. let _held = hold_plugin_changes(); + let turn = s.plugin_store_turn().unwrap_or_else(|_| StoreTurn::unheld()); match registry::load_folder_where(std::path::Path::new(folder), allow, s.plugin_folder_approver()) { Ok(r) => { for id in &r.loaded { - s.attach_plugin_storage(id); + s.attach_plugin_storage(&turn, id); } if r.disabled_changed { - s.store_disabled_plugins(); + s.store_disabled_plugins_in(&turn); } json!({"folder": folder, "loaded": r.loaded, "failed": r.failed.iter().map(|(f, e)| json!({"file": f, "error": e})).collect::>()}) } @@ -1140,6 +1170,9 @@ fn set_enabled(s: &mut Session, p: &Value) -> Result { (None, None) => return Err(bad(C, "give `id` or `file` (see plugin.safeMode)")), }; let items: Vec<&safe_mode::Item> = std::iter::once(&item).chain(stored.as_ref()).collect(); + // The list is changed and saved in one turn on the plug-in store, so that an install in another + // process doesn't change the store in between. + let turn = s.plugin_store_turn().ok(); if enabled && !safe_mode::disabled().iter().any(|i| items.iter().any(|item| i.same(item))) { return Err(bad(C, "that plug-in isn't disabled (see plugin.safeMode)")); } @@ -1149,7 +1182,7 @@ fn set_enabled(s: &mut Session, p: &Value) -> Result { } // Without a plug-in folder (a session confined to automation roots), or when the list there // can't take the change, the change stays in this process and goes with the next change that saves. - let saved = !changed || s.store_disabled_plugins(); + let saved = !changed || turn.as_ref().is_some_and(|turn| s.store_disabled_plugins_in(turn)); Ok(json!({"changed": changed, "saved": saved, "disabled": safe_mode::disabled().iter().map(shown_item).collect::>()})) } @@ -1164,31 +1197,35 @@ fn info(_: &mut Session, p: &Value) -> Result { fn remove(s: &mut Session, p: &Value) -> Result { let id = str_param(p, "id").ok_or_else(|| bad("plugin.remove", "missing `id`"))?; - // An install in another session finishes its store writes first. + // An install in another session or process finishes its store writes first. When the store + // can't be locked, nothing is removed. let _held = hold_plugin_changes(); + let turn = s + .plugin_store_turn() + .map_err(|e| EngineError::Other(format!("plugin.remove: {}: {e}: nothing was removed", crate::store::STORE_NOT_LOCKED)))?; let installed = registry::get(id).is_some(); // Settings and cache the store keeps for an id that isn't installed are deleted too. let stored = !installed && vectorcraft_plugins::manifest::valid_id(id) && s.has_plugin_storage(id); // Installed again, it asks again. A grant that can't be removed still approves the module, and // so may a grants file that can't be opened or read. One that is too large or doesn't hold a // list approves nothing and doesn't stop the removal. - let forget = || s.store_grant(id, None) || !s.holds_plugin_grant(id); + let forget = || s.store_grant_in(&turn, id, None) || !s.holds_plugin_grant(id); // Its settings and cache are moved aside and its grant removed before anything is deleted: when // either can't be done, the plug-in stays installed, as it was, and a second `plugin.remove` // tries again. let failed = if installed || stored { - s.remove_plugin_storage(id, forget).err().map(|e| e.to_string()) + s.remove_plugin_storage(&turn, id, forget).err().map(|e| e.to_string()) } else { (!forget()).then(|| crate::store::GRANT_NOT_REMOVED.to_string()) }; if let Some(e) = failed { if installed { - s.attach_plugin_storage(id); + s.attach_plugin_storage(&turn, id); } return Err(EngineError::Other(e)); } registry::revoke(id); - s.unstore_plugin(id); + s.unstore_plugin(&turn, id); if !stored && (!installed || !registry::remove(id)) { return Err(err(vectorcraft_plugins::Error::NotFound(id.into()))); } @@ -1248,7 +1285,7 @@ pub fn specs() -> Vec { "Install Plug-in", [], None, - "{path: a .wasm file (desktop) | dataBase64: the module (+ name?: shown as its source), replace?: bool (true once the user agreed: a module with other bytes than the plug-in installed with its id, or with none installed, one whose id has settings or a cache in the plug-in store, installs only with it, takes that plug-in's place and gets its settings and cache; false: one whose id is installed is refused; left out, the same module installs again), name?: file name (with dataBase64), allowHeavy?: bool (a filter whose manifest asks for a heavy budget, budget \"heavy\" in plugin.inspect, installs only with it), grant?: [permission] (a plug-in whose manifest lists permissions, needsApproval in plugin.inspect, installs only when grant holds every one of them; the grant is recorded with the module's hash and covers only that module, so a module with other bytes needs a new grant even when it lists the same permissions; installing a module that lists no permissions forgets the grant held for its id; a refused install keeps the grant, a failed write of the module to the plug-in store leaves the grant in the store, and a stored module that is disabled or not read in Safe Mode keeps the grant for its id; with a plug-in store attached, only the grants kept there count; a request's allowHeavy and grant are refused unless the session was started with vectorcraft-cli --allow-plugin-grants: in the desktop app a person approves the install, see ui.installPlugin, and a module whose hash is already recorded, by a person's approval or by a grant accepted under that flag, needs no grant), hash?: string (the module's SHA-256 as plugin.inspect gave it: a module with other bytes is refused)} validate and install a WebAssembly plug-in for this session (a module whose commandAllowlist names a command no plug-in may call, such as one that reads or writes files, is refused) → its plugin.list entry, plus unknownCommands?: [id] (commandAllowlist ids no command has; a call of one fails)", + "{path: a .wasm file (desktop) | dataBase64: the module (+ name?: shown as its source), replace?: bool (true once the user agreed: a module with other bytes than the plug-in installed with its id, or with none installed, one whose id has settings or a cache in the plug-in store, installs only with it, takes that plug-in's place and gets its settings and cache; false: one whose id is installed is refused; left out, the same module installs again), name?: file name (with dataBase64), allowHeavy?: bool (a filter whose manifest asks for a heavy budget, budget \"heavy\" in plugin.inspect, installs only with it), grant?: [permission] (a plug-in whose manifest lists permissions, needsApproval in plugin.inspect, installs only when grant holds every one of them; the grant is recorded with the module's hash and covers only that module, so a module with other bytes needs a new grant even when it lists the same permissions; installing a module that lists no permissions forgets the grant held for its id; a refused install keeps the grant, a failed write of the module to the plug-in store leaves the grant in the store, and a stored module that is disabled or not read in Safe Mode keeps the grant for its id; with a plug-in store attached, only the grants kept there count; a request's allowHeavy and grant are refused unless the session was started with vectorcraft-cli --allow-plugin-grants: in the desktop app a person approves the install, see ui.installPlugin, and a module whose hash is already recorded, by a person's approval or by a grant accepted under that flag, needs no grant), hash?: string (the module's SHA-256 as plugin.inspect gave it: a module with other bytes is refused)} validate and install a WebAssembly plug-in for this session (with a plug-in store, the install waits while another window or process changes the store and writes the module and its grant before that window or process changes it again; when the store's disabled.lock can't be locked, nothing is installed) (a module whose commandAllowlist names a command no plug-in may call, such as one that reads or writes files, is refused) → its plugin.list entry, plus unknownCommands?: [id] (commandAllowlist ids no command has; a call of one fails)", always, install ), @@ -1266,7 +1303,7 @@ pub fn specs() -> Vec { "Remove Plug-in", [], None, - "{id} uninstall a plug-in (its live effects then leave the geometry as it is) and delete the settings and cache it kept (when their folder can't be moved aside, or its grant can't be removed from the plug-in store, the command fails and the plug-in stays installed; when the settings can't be moved back either, the error names the folder they stay in, and the next load, install or removal of the plug-in moves them back); for an id that isn't installed, deletes the settings and cache the plug-in store still keeps for it → {removed}", + "{id} uninstall a plug-in (its live effects then leave the geometry as it is) and delete the settings and cache it kept (when their folder can't be moved aside, or its grant can't be removed from the plug-in store, the command fails and the plug-in stays installed; when the settings can't be moved back either, the error names the folder they stay in, and the next load, install or removal of the plug-in moves them back); for an id that isn't installed, deletes the settings and cache the plug-in store still keeps for it; it waits while another window or process changes the plug-in store, and when the store's disabled.lock can't be locked, the command fails and nothing is removed → {removed}", always, remove ), diff --git a/crates/engine/src/store.rs b/crates/engine/src/store.rs index 8a18ce9824..9c555d82f7 100644 --- a/crates/engine/src/store.rs +++ b/crates/engine/src/store.rs @@ -17,9 +17,8 @@ //! * **Plug-ins**: the module of each plug-in `plugin.install` installs, as a `.wasm` file named //! after its id (as Illustrator keeps plug-ins in its Plug-ins folder); `plugin.remove` deletes it. //! Attaching the folder installs them again, except in Safe Mode and except the ones the user -//! disabled, which `disabled.json` in the same folder lists (`plugin.setEnabled`; a window locks -//! `disabled.lock` while it writes the list). The Additional Plug-ins Folder preference is not -//! copied: it is loaded from where it is. `grants.json` holds what the user granted the plug-ins +//! disabled, which `disabled.json` in the same folder lists (`plugin.setEnabled`). The Additional +//! Plug-ins Folder preference is not copied: it is loaded from where it is. `grants.json` holds what the user granted the plug-ins //! stored there that list permissions (id, module hash, permissions); a stored plug-in whose //! permissions it doesn't grant is skipped with a log record, and stays in the folder. //! * **Plug-in settings and caches**: `storage/id-/` in the plug-in store, the id written as @@ -27,6 +26,11 @@ //! keeps with the `settings` and `cache` permissions ([`PluginStorage`]): `settings.json` and a //! `cache` folder with one file per entry. The host names every file; a plug-in only names keys. //! Replacing a plug-in keeps them, and `plugin.remove` deletes them. +//! * **Turns on the plug-in store**: a window or process locks `disabled.lock` in the plug-in folder +//! ([`StoreTurn`]) while it reads and changes the stored plug-ins, `grants.json`, `disabled.json` +//! and the plug-ins' settings and caches, so that two of them using one folder change it one after +//! the other: an install's module and grant are written in one turn, and a grant written for one +//! plug-in doesn't undo another one's. //! //! The files are untrusted input like any other: sizes and counts are capped, a file that does not //! parse is skipped with a log record, and every write is atomic. @@ -62,12 +66,15 @@ pub const MAX_PLUGIN_STORE: u64 = 512 << 20; const PLUGIN_EXT: &str = "wasm"; /// The file in the plug-in folder that lists the plug-ins the user disabled. pub const DISABLED_FILE: &str = "disabled.json"; -/// The file in the plug-in folder that a window locks while it reads and writes [`DISABLED_FILE`]. +/// The file in the plug-in folder that a window locks while it reads and changes the plug-in store +/// ([`StoreTurn`]), [`DISABLED_FILE`] included. pub const DISABLED_LOCK: &str = "disabled.lock"; /// The file in the plug-in store that holds the permissions the user granted. const GRANTS_FILE: &str = "grants.json"; /// The error of a `plugin.remove` that can't remove the plug-in's grant from [`GRANTS_FILE`]. pub(crate) const GRANT_NOT_REMOVED: &str = "the plug-in's grant can't be removed from the plug-in store"; +/// The error of a change to the plug-in store that can't take its turn ([`StoreTurn`]). +pub(crate) const STORE_NOT_LOCKED: &str = "the plug-in store can't be locked"; /// The largest grants file read. const MAX_GRANTS_FILE: u64 = 1 << 20; /// The folder in the plug-in store that holds each plug-in's settings and cache. @@ -84,7 +91,7 @@ const CACHE_EXT: &str = "bin"; /// A write to a folder that holds more is refused as full. pub(crate) const MAX_CACHE_SCAN: usize = storage::MAX_CACHE_ENTRIES * 2; /// Held while a plug-in's settings or cache change, so two runs can't pass a cap together or lose -/// each other's settings. +/// each other's settings. Taken after the plug-in store's turn ([`StoreTurn`]), never before it. static PLUGIN_DATA_LOCK: Mutex<()> = Mutex::new(()); /// The state of each plug-in folder's storage, shared by every storage made for the folder until /// the plug-in is removed, which closes it and drops it. @@ -99,7 +106,7 @@ const MAX_GRANT_WRITES_KEPT: usize = 256; struct IgnoredGrants { /// The grants that should be gone but could not be removed: no session in the process counts /// them ([`Session::ignore_stored_grant`]) until one writes the store's grants for the id - /// ([`Session::store_grant`]). + /// ([`Session::store_grant_in`]). ignored: BTreeSet<(PathBuf, String)>, /// The latest grant writes of the process, at most [`MAX_GRANT_WRITES_KEPT`], each with the /// count of writes it made ([`IgnoredGrants::writes`]). @@ -217,23 +224,53 @@ fn write_atomic(_: &Path, _: &[u8]) -> std::io::Result<()> { Err(std::io::Error::new(std::io::ErrorKind::Unsupported, "no file system")) } -/// [`DISABLED_LOCK`] in `dir`, locked until the file is dropped, waiting while another window holds -/// it; `None` when it can't be opened or locked. +/// [`DISABLED_LOCK`] in the plug-in store `dir`, locked until the file is dropped, waiting while +/// another window or process holds it, or another handle in this process. The file is opened for +/// reading and writing (a handle that only appends can't be locked on Windows) and is never +/// replaced or removed, so that every window locks the same file. #[cfg(not(target_arch = "wasm32"))] -fn lock_disabled(dir: &Path) -> Option { - let opened = std::fs::create_dir_all(dir) - .and_then(|()| std::fs::File::options().create(true).truncate(false).read(true).write(true).open(dir.join(DISABLED_LOCK))); - let file = opened.and_then(|f| f.lock().map(|()| f)); - if let Err(e) = &file { - log::warn!("can't lock {}: {e}", dir.join(DISABLED_LOCK).display()); +fn lock_store(dir: &Path) -> std::io::Result { + std::fs::create_dir_all(dir)?; + let file = std::fs::File::options().create(true).truncate(false).read(true).write(true).open(dir.join(DISABLED_LOCK))?; + match file.try_lock() { + Ok(()) => return Ok(file), + Err(std::fs::TryLockError::WouldBlock) => {} + Err(std::fs::TryLockError::Error(e)) => return Err(e), } - file.ok() + #[cfg(test)] + if let Some(waiting) = WHILE_STORE_LOCKED.take() { + waiting(); + } + file.lock()?; + Ok(file) } /// The browser has no folders to keep anything in. #[cfg(target_arch = "wasm32")] -fn lock_disabled(_: &Path) -> Option { - None +fn lock_store(_: &Path) -> std::io::Result { + Err(std::io::Error::new(std::io::ErrorKind::Unsupported, "no file system")) +} + +/// A turn on a plug-in store ([`Session::plugin_store_turn`]): while it is held, no other window or +/// process that takes one reads and changes the store's plug-ins, grants, disabled list, settings +/// or caches. Dropping it ends the turn. A turn is taken once per operation and handed to the +/// functions it calls, since a second lock of the same file by the same thread waits for ever. +pub(crate) struct StoreTurn { + /// [`DISABLED_LOCK`], locked; `None` without a store, and for a load that goes ahead without + /// a turn ([`Session::plugin_store_turn_for_load`]), which then writes nothing to the store. + lock: Option, +} + +impl StoreTurn { + /// A turn that isn't held, for a load that goes ahead without one: it writes nothing. + pub(crate) fn unheld() -> Self { + StoreTurn { lock: None } + } + + /// Whether the store may be written in this turn. + fn held(&self) -> bool { + self.lock.is_some() + } } #[cfg(test)] @@ -252,6 +289,15 @@ thread_local! { /// Tests: writing the `.delete` file beside a folder about to be deleted ([`delete_aside`]) /// fails on this thread, as on a full volume. pub(crate) static MARKER_WRITE_FAILS: std::cell::Cell = const { std::cell::Cell::new(false) }; + /// Tests: runs once on this thread when locking the plug-in store ([`lock_store`]) finds it + /// locked and waits. + pub(crate) static WHILE_STORE_LOCKED: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; + /// Tests: runs once on this thread when a grant write has read `grants.json` and before it + /// writes the file. + pub(crate) static AFTER_GRANTS_READ: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; + /// Tests: runs once on this thread when a change of a plug-in's settings has read them and + /// before it writes them. + pub(crate) static AFTER_SETTINGS_READ: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; } /// Remove `dir/file` → whether it is gone; a file that isn't there is what was wanted. @@ -401,11 +447,14 @@ impl Session { /// Keep the plug-ins this session installs in `dir`, take the disabled plug-ins saved there /// ([`DISABLED_FILE`]), and install the stored ones ([`Session::load_stored_plugins`]). pub fn attach_plugin_store(&mut self, dir: PathBuf) { - let disabled = - read_capped(&dir.join(DISABLED_FILE), safe_mode::MAX_DISABLED_FILE_BYTES).map(|b| safe_mode::parse_disabled_file(&b)).unwrap_or_default(); - safe_mode::set_disabled(disabled); + let path = dir.join(DISABLED_FILE); self.plugin_dir = Some(dir); - self.load_stored_plugins(); + // A `plugin.install` in another session or process writes the store before this reads it. + let _held = crate::cmd::plugin::hold_plugin_changes(); + let turn = self.plugin_store_turn_for_load(); + let disabled = read_capped(&path, safe_mode::MAX_DISABLED_FILE_BYTES).map(|b| safe_mode::parse_disabled_file(&b)).unwrap_or_default(); + safe_mode::set_disabled(disabled); + self.load_stored_plugins_in(&turn); } /// Install the plug-ins kept in the plug-in folder: none in Safe Mode, and none the user @@ -415,14 +464,25 @@ impl Session { /// stays in the folder with its grant. A grant for an id whose stored module lists no /// permissions is forgotten; one that can't be removed from the grants file doesn't count for /// any session in the process attached to the folder until the file can be written for the id - /// ([`Session::ignore_stored_grant`]). + /// ([`Session::ignore_stored_grant`]). The folder is read and changed in one turn on it + /// ([`Session::plugin_store_turn_for_load`]). pub fn load_stored_plugins(&mut self) { + if self.plugin_dir.is_none() || safe_mode::active() { + return; + } + // A `plugin.install` in another session or process writes the store before this reads it. + let _held = crate::cmd::plugin::hold_plugin_changes(); + let turn = self.plugin_store_turn_for_load(); + self.load_stored_plugins_in(&turn); + } + + /// [`Session::load_stored_plugins`] in `turn`, while [`crate::cmd::plugin::hold_plugin_changes`] + /// is held. + fn load_stored_plugins_in(&mut self, turn: &StoreTurn) { let Some(dir) = self.plugin_dir.clone() else { return }; if safe_mode::active() { return; } - // A `plugin.install` in another session writes the store before this reads it. - let _held = crate::cmd::plugin::hold_plugin_changes(); let grants = self.counted_grants(&dir); let limits = vectorcraft_plugins::Limits::default(); let mut left = MAX_PLUGIN_STORE; @@ -454,7 +514,7 @@ impl Session { Some(g) => { asks_nothing.remove(&m.id); vectorcraft_plugins::registry::grant(GrantToken::from_engine_grants_file(g.clone())); - vectorcraft_plugins::registry::set_storage(&m.id, plugin_storage(Some(&dir), &m.id)); + vectorcraft_plugins::registry::set_storage(&m.id, plugin_storage(Some(&dir), &m.id, turn)); if let Err(e) = vectorcraft_plugins::registry::install(plugin.with_source(path.display().to_string())) { log::warn!("skipping the stored plug-in {}: {e}", path.display()); } @@ -466,7 +526,7 @@ impl Session { } Ok(plugin) => { asks_nothing.insert(plugin.id().to_string()); - vectorcraft_plugins::registry::set_storage(plugin.id(), plugin_storage(Some(&dir), plugin.id())); + vectorcraft_plugins::registry::set_storage(plugin.id(), plugin_storage(Some(&dir), plugin.id(), turn)); if let Err(e) = vectorcraft_plugins::registry::install(plugin.with_source(path.display().to_string())) { log::warn!("skipping the stored plug-in {}: {e}", path.display()); } @@ -483,13 +543,34 @@ impl Session { }; if named { let writes = self.grant_writes(); - if !self.store_grant(id, None) && self.holds_plugin_grant(id) { + if !self.store_grant_in(turn, id, None) && self.holds_plugin_grant(id) { self.ignore_stored_grant(id, writes); } } } } + /// Take a turn on the attached plug-in store ([`StoreTurn`]), waiting while another window or + /// process holds one; without a store, a turn that writes nothing. An error, with a log record, + /// when the lock file can't be made, opened or locked. Taken after + /// [`crate::cmd::plugin::hold_plugin_changes`] when both are needed, and never by a thread that + /// holds a turn already. + pub(crate) fn plugin_store_turn(&self) -> std::io::Result { + let Some(dir) = self.plugin_dir.as_deref() else { return Ok(StoreTurn::unheld()) }; + lock_store(dir) + .map(|file| StoreTurn { lock: Some(file) }) + .inspect_err(|e| log::warn!("can't lock {}: {e}", dir.join(DISABLED_LOCK).display())) + } + + /// [`Session::plugin_store_turn`] for a load, which goes ahead without the turn when the store + /// folder isn't there (nothing is stored) or can't be locked, and then writes nothing to it. + pub(crate) fn plugin_store_turn_for_load(&self) -> StoreTurn { + match self.plugin_dir.as_deref() { + Some(dir) if dir.is_dir() => self.plugin_store_turn().unwrap_or_else(|_| StoreTurn::unheld()), + _ => StoreTurn::unheld(), + } + } + /// Write the disabled plug-ins saved for the next launch to the plug-in folder → whether they /// went there, every change included (`false` without a folder, and when a plug-in turned off /// found the file's list full: [`safe_mode::Merged::complete`]). The changes this launch made are applied to the list @@ -499,8 +580,15 @@ impl Session { /// a file that can't be read (too large, say) or that a later version wrote /// ([`safe_mode::read_disabled_file`]): it is kept as it is. pub fn store_disabled_plugins(&self) -> bool { + self.plugin_store_turn().is_ok_and(|turn| self.store_disabled_plugins_in(&turn)) + } + + /// [`Session::store_disabled_plugins`] in `turn`; `false` when it isn't held. + pub(crate) fn store_disabled_plugins_in(&self, turn: &StoreTurn) -> bool { let Some(dir) = self.plugin_dir.as_deref() else { return false }; - let Some(_turn) = lock_disabled(dir) else { return false }; + if !turn.held() { + return false; + } let path = dir.join(DISABLED_FILE); let theirs = match read_capped(&path, safe_mode::MAX_DISABLED_FILE_BYTES) { Ok(bytes) => match safe_mode::read_disabled_file(&bytes) { @@ -580,10 +668,10 @@ impl Session { grants } - /// Write the module `bytes` of the plug-in `id` to the store → whether they went there (`false` - /// without a folder). - pub(crate) fn store_plugin(&self, id: &str, bytes: &[u8]) -> bool { - self.plugin_dir.as_deref().is_some_and(|dir| put(dir, &format!("{}.{PLUGIN_EXT}", file_stem(id)), bytes)) + /// Write the module `bytes` of the plug-in `id` to the store in `turn` → whether they went there + /// (`false` without a folder, and when `turn` isn't held). + pub(crate) fn store_plugin(&self, turn: &StoreTurn, id: &str, bytes: &[u8]) -> bool { + turn.held() && self.plugin_dir.as_deref().is_some_and(|dir| put(dir, &format!("{}.{PLUGIN_EXT}", file_stem(id)), bytes)) } /// The module the store holds for the plug-in `id`: `None` without a folder or such a file, and @@ -601,14 +689,15 @@ impl Session { /// Return the store's module for the plug-in `id` to `previous`, the bytes it held before a /// write ([`Session::stored_plugin_bytes`]): with `None`, the file is removed. → whether the /// write reported success; a write that reports a failure may still have put `previous` back. - pub(crate) fn put_back_plugin(&self, id: &str, previous: Option<&[u8]>) -> bool { + /// In `turn`, the one the write was made in. + pub(crate) fn put_back_plugin(&self, turn: &StoreTurn, id: &str, previous: Option<&[u8]>) -> bool { #[cfg(test)] if PUT_BACK_FAILS.get() { return false; } let back = match previous { - Some(bytes) => self.store_plugin(id, bytes), - None => self.unstore_plugin(id), + Some(bytes) => self.store_plugin(turn, id, bytes), + None => self.unstore_plugin(turn, id), }; #[cfg(test)] if PUT_BACK_FAILS_AFTER_WRITE.get() { @@ -623,15 +712,23 @@ impl Session { self.plugin_dir.as_ref().map(|dir| dir.join(format!("{}.{PLUGIN_EXT}", file_stem(id))).display().to_string()) } + /// Tests: [`Session::store_grant_in`] in a turn of its own ([`Session::plugin_store_turn`]); + /// `false` when it can't be taken. + #[cfg(test)] + pub(crate) fn store_grant(&self, id: &str, grant: Option) -> bool { + self.plugin_store_turn().is_ok_and(|turn| self.store_grant_in(&turn, id, grant)) + } + /// Record in the store what the user granted plug-in `id` (`None`: nothing), leaving the store's /// other grants as they are; with none left, remove the file. Entries this version can't use /// are written back as they were, except one for `id`. A grants file that is too large, can't /// be read or doesn't hold a list is left as it is, with a log record, and nothing is recorded; - /// so is one whose new content would be larger than `MAX_GRANTS_FILE`. → whether the store now - /// holds what was asked (`false` without a folder). When it does, the grant kept for `id` counts - /// again, for every session in the process attached to the store ([`Session::ignore_stored_grant`]). - pub(crate) fn store_grant(&self, id: &str, grant: Option) -> bool { - let done = self.write_grant(id, grant); + /// so is one whose new content would be larger than `MAX_GRANTS_FILE`. The file is read and + /// written in `turn`. → whether the store now holds what was asked (`false` without a folder, + /// and when `turn` isn't held). When it does, the grant kept for `id` counts again, for every + /// session in the process attached to the store ([`Session::ignore_stored_grant`]). + pub(crate) fn store_grant_in(&self, turn: &StoreTurn, id: &str, grant: Option) -> bool { + let done = turn.held() && self.write_grant(id, grant); if done && let Some(key) = self.plugin_dir.as_deref().map(plugin_store_key) { let mut shared = IGNORED_PLUGIN_GRANTS.lock().unwrap_or_else(|e| e.into_inner()); let entry = (key, id.to_string()); @@ -648,7 +745,7 @@ impl Session { done } - /// How many grant writes the sessions of the process have made ([`Session::store_grant`]). + /// How many grant writes the sessions of the process have made ([`Session::store_grant_in`]). /// Taken before a write that may fail, for [`Session::ignore_stored_grant`]. pub(crate) fn grant_writes(&self) -> u64 { IGNORED_PLUGIN_GRANTS.lock().unwrap_or_else(|e| e.into_inner()).writes @@ -661,7 +758,7 @@ impl Session { } /// Stop counting the attached store's grant for the plug-in `id`, in every session in the - /// process attached to the same folder, until [`Session::store_grant`] next succeeds for it in + /// process attached to the same folder, until [`Session::store_grant_in`] next succeeds for it in /// one of them: for a grant that should be gone and could not be removed, which would otherwise /// approve the bytes it covers. `writes` is [`Session::grant_writes`] from before the failed /// write: when a session has written the store's grants for `id` since, the file holds what that @@ -687,6 +784,10 @@ impl Session { return false; } }; + #[cfg(test)] + if let Some(between) = AFTER_GRANTS_READ.take() { + between(); + } let others = file.other.len(); file.other.retain(|entry| entry.get("id").and_then(serde_json::Value::as_str) != Some(id)); let changed = match grant { @@ -724,16 +825,17 @@ impl Session { }) } - /// Remove the plug-in `id` from the store → whether its file is gone (`false` without a folder). - pub(crate) fn unstore_plugin(&self, id: &str) -> bool { - self.plugin_dir.as_deref().is_some_and(|dir| take(dir, &format!("{}.{PLUGIN_EXT}", file_stem(id)))) + /// Remove the plug-in `id` from the store in `turn` → whether its file is gone (`false` without a + /// folder, and when `turn` isn't held). + pub(crate) fn unstore_plugin(&self, turn: &StoreTurn, id: &str) -> bool { + turn.held() && self.plugin_dir.as_deref().is_some_and(|dir| take(dir, &format!("{}.{PLUGIN_EXT}", file_stem(id)))) } /// Keep the settings and cache of plug-in `id` in the attached plug-in store from now on; without /// one, nowhere (its storage calls then fail as unavailable). Settings and cache that a removal - /// kept are moved back first ([`plugin_storage`]). - pub(crate) fn attach_plugin_storage(&self, id: &str) { - vectorcraft_plugins::registry::set_storage(id, plugin_storage(self.plugin_dir.as_deref(), id)); + /// kept are moved back first, in `turn` ([`plugin_storage`]). + pub(crate) fn attach_plugin_storage(&self, turn: &StoreTurn, id: &str) { + vectorcraft_plugins::registry::set_storage(id, plugin_storage(self.plugin_dir.as_deref(), id, turn)); } /// Does the attached plug-in store have a folder of settings and cache for plug-in `id`, or one @@ -757,9 +859,13 @@ impl Session { /// that rename fails too, the error also names both paths and the reason, and the next attach or /// removal for the id moves the folder back. Once `forget` returns `true`, the folder and the /// kept settings are deleted ([`delete_aside`]); what that deletion leaves stays, with a log - /// record, and a later call for the id tries to delete it again. - pub(crate) fn remove_plugin_storage(&self, id: &str, forget: impl FnOnce() -> bool) -> Result<(), StorageError> { + /// record, and a later call for the id tries to delete it again. All of it happens in `turn`; + /// with a store and a turn that isn't held, nothing changes and this fails. + pub(crate) fn remove_plugin_storage(&self, turn: &StoreTurn, id: &str, forget: impl FnOnce() -> bool) -> Result<(), StorageError> { let not_forgotten = || StorageError::Io(GRANT_NOT_REMOVED.to_string()); + if self.plugin_dir.is_some() && !turn.held() { + return Err(StorageError::Io(STORE_NOT_LOCKED.to_string())); + } vectorcraft_plugins::registry::set_storage(id, None); let Some(dir) = &self.plugin_dir else { return if forget() { Ok(()) } else { Err(not_forgotten()) }; @@ -943,13 +1049,16 @@ fn restore_removed(path: &Path) -> std::io::Result<()> { /// Where plug-in `id` keeps its settings and cache in the plug-in store `dir`. Settings and cache /// that a removal kept are moved back first ([`restore_removed`]); when that fails, the failure is /// logged and there is no storage (its calls fail as unavailable), so that nothing the plug-in -/// writes takes their place. A later attach or load tries again. -fn plugin_storage(dir: Option<&Path>, id: &str) -> Option> { +/// writes takes their place. A later attach or load tries again. They are moved in `turn`; when it +/// isn't held, they stay where they are, as when moving them fails. +fn plugin_storage(dir: Option<&Path>, id: &str, turn: &StoreTurn) -> Option> { let dir = dir?; let data = plugin_data_dir(dir, id); if !is_link(&dir.join(PLUGIN_DATA_DIR)) { let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - if let Err(e) = restore_removed(&data) { + let kept_aside = || std::fs::symlink_metadata(&data).is_err() && aside_paths(&data).iter().any(|a| is_kept(a)); + let restored = if turn.held() || !kept_aside() { restore_removed(&data) } else { Err(std::io::Error::other(STORE_NOT_LOCKED)) }; + if let Err(e) = restored { log::warn!("can't move the settings and cache of {} back: {e}", data.display()); return None; } @@ -1088,6 +1197,12 @@ impl PluginStorage { Ok(()) } + /// A turn on the plug-in store ([`lock_store`]), taken before [`PLUGIN_DATA_LOCK`], so that a + /// window or process changing the same settings or cache waits for this change. + fn turn(&self) -> Result { + lock_store(&self.root).map_err(|e| StorageError::Io(format!("{STORE_NOT_LOCKED}: {e}"))) + } + fn kept(&self) -> MutexGuard<'_, Kept> { self.state.kept.lock().unwrap_or_else(|e| e.into_inner()) } @@ -1212,6 +1327,8 @@ impl PluginStorage { impl Storage for PluginStorage { fn setting(&self, key: &str, meter: &mut Meter) -> Result>, StorageError> { + // Reading may move a settings file that doesn't read aside. + let _turn = self.turn()?; let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()); self.open()?; let mut kept = self.kept(); @@ -1219,10 +1336,15 @@ impl Storage for PluginStorage { } fn set_setting(&self, key: &str, value: &[u8], meter: &mut Meter) -> Result<(), StorageError> { + let _turn = self.turn()?; let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()); self.open()?; let mut kept = self.kept(); let settings = &mut self.read_settings(&mut kept.settings, meter)?.values; + #[cfg(test)] + if let Some(between) = AFTER_SETTINGS_READ.take() { + between(); + } if value.len() > storage::MAX_SETTING_BYTES || (!settings.contains_key(key) && settings.len() >= storage::MAX_SETTINGS) { return Err(StorageError::Full); } @@ -1234,6 +1356,7 @@ impl Storage for PluginStorage { } fn delete_setting(&self, key: &str, meter: &mut Meter) -> Result { + let _turn = self.turn()?; let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()); self.open()?; let mut kept = self.kept(); @@ -1258,6 +1381,8 @@ impl Storage for PluginStorage { } fn set_cache_entry(&self, name: &str, value: &[u8], meter: &mut Meter) -> Result<(), StorageError> { + // The cache's size and count are added up and the entry written in one turn. + let _turn = self.turn()?; let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()); self.open()?; let path = self.entry_path(name); @@ -1281,6 +1406,7 @@ impl Storage for PluginStorage { } fn remove_cache_entry(&self, name: &str) -> Result { + let _turn = self.turn()?; let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()); self.open()?; let path = self.entry_path(name); @@ -1296,6 +1422,7 @@ impl Storage for PluginStorage { } fn clear_cache(&self) -> Result { + let _turn = self.turn()?; let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner()); self.open()?; let dir = self.cache_dir(); diff --git a/crates/engine/src/tests/plugins.rs b/crates/engine/src/tests/plugins.rs index 499f2d15c6..a294cc9036 100644 --- a/crates/engine/src/tests/plugins.rs +++ b/crates/engine/src/tests/plugins.rs @@ -1921,6 +1921,12 @@ fn disabled_plugins_name_files_outside_the_read_root_by_their_names() { run(&mut s, "plugin.remove", json!({"id": id})); } +/// The files and folders in the plug-in store `dir`, without the file windows lock while they +/// change it. +fn store_files(dir: &std::path::Path) -> usize { + std::fs::read_dir(dir).unwrap().filter(|e| e.as_ref().is_ok_and(|e| e.file_name() != crate::store::DISABLED_LOCK)).count() +} + /// #2541: a plug-in installed in a session with a plug-in store is installed again in the next /// session that attaches the store, and Remove Plug-in takes it out of the store. #[test] @@ -1931,14 +1937,14 @@ fn installed_plugins_are_kept_in_the_store() { let mut s = session(); s.attach_plugin_store(folder.0.clone()); install_wat(&mut s, &src); - assert_eq!(std::fs::read_dir(&folder.0).unwrap().count(), 1, "one file in the store"); + assert_eq!(store_files(&folder.0), 1, "one file in the store"); vectorcraft_plugins::registry::remove(id); let mut next = session(); next.attach_plugin_store(folder.0.clone()); assert!(vectorcraft_plugins::registry::get(id).is_some(), "installed again from the store"); run(&mut next, "plugin.remove", json!({"id": id})); - assert_eq!(std::fs::read_dir(&folder.0).unwrap().count(), 0, "removed from the store"); + assert_eq!(store_files(&folder.0), 0, "removed from the store"); // A file that isn't a plug-in is skipped; the store still attaches. std::fs::write(folder.0.join("broken.wasm"), b"\0asm?").unwrap(); @@ -3274,6 +3280,198 @@ fn a_store_loaded_while_installing_loads_the_new_version() { run(&mut s, "plugin.remove", json!({"id": id})); } +/// A grant of `doc.read` to plug-in `id`, the module with the SHA-256 `hash`. +fn doc_read_grant(id: &str, hash: &str) -> vectorcraft_plugins::permissions::Grant { + serde_json::from_value(json!({"id": id, "hash": hash, "permissions": ["doc.read"]})).unwrap() +} + +/// A session that uses the plug-in store `dir` without loading it, as another process using the +/// same folder does: it doesn't wait for the installs and removals of this process's sessions. +fn other_process(dir: std::path::PathBuf) -> Session { + let mut s = Session::new(); + s.plugin_dir = Some(dir); + s +} + +/// Waits on this thread for a turn on a plug-in store, telling `waiting` before it waits. +fn tell_when_waiting(waiting: std::sync::mpsc::Sender<&'static str>) { + crate::store::WHILE_STORE_LOCKED.set(Some(Box::new(move || { + let _ = waiting.send("waiting"); + }))); +} + +/// The ids `grants.json` in `dir` holds grants for, and the hash of each. +fn stored_grant_hashes(dir: &std::path::Path) -> std::collections::BTreeMap { + let entries: Vec = serde_json::from_slice(&std::fs::read(dir.join("grants.json")).unwrap()).unwrap(); + entries.iter().map(|e| (e["id"].as_str().unwrap().to_string(), e["hash"].as_str().unwrap().to_string())).collect() +} + +/// An install into a `Plug-ins` folder that another process also installs the same id into keeps a +/// module with its own grant: the other process's install waits for this one's turn on the folder, +/// between this install's module and its grant, and then writes both of its own. +#[test] +fn installs_from_two_processes_into_one_store_keep_each_module_with_its_grant() { + use vectorcraft_plugins::permissions::sha256_hex; + let folder = crate::tests_links::Folder::new("plugin-store-installs-take-turns"); + let id = "test.engine.permissions.installsturns"; + let ours = asking(id, r#"["doc.read"]"#, &json!({"objects": []})); + let theirs = asking(id, r#"["doc.read"]"#, &json!({"objects": [], "documentData": null})); + let mut s = session(); + s.attach_plugin_store(folder.0.clone()); + + let (said, heard) = std::sync::mpsc::channel::<&'static str>(); + let (handed, other) = std::sync::mpsc::channel(); + let (dir, module) = (folder.0.clone(), theirs.clone()); + crate::cmd::plugin::BEFORE_GRANT_STORED.set(Some(Box::new(move || { + let thread = std::thread::spawn(move || { + tell_when_waiting(said.clone()); + let other = other_process(dir); + let turn = other.plugin_store_turn().unwrap(); + let stored = crate::cmd::plugin::keep_installed(&other, &turn, id, &module, Some(doc_read_grant(id, &sha256_hex(&module)))); + let _ = said.send(if stored { "stored" } else { "not stored" }); + }); + // The other install has written the store, or waits for this one. + let first = heard.recv().unwrap_or("gone"); + let _ = handed.send((first, thread)); + }))); + run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&ours), "grant": ["doc.read"]})); + let (first, thread) = other.recv().unwrap(); + thread.join().unwrap(); + + let module = std::fs::read(s.stored_plugin_file(id).unwrap()).unwrap(); + assert_eq!(stored_grant_hashes(&folder.0).get(id), Some(&sha256_hex(&module)), "the stored grant covers the stored module"); + assert_eq!(module, theirs, "the install that waited wrote last"); + assert_eq!(first, "waiting", "the other install waits for this one"); + run(&mut s, "plugin.remove", json!({"id": id})); +} + +/// Grants written for two plug-ins into one `Plug-ins` folder by two processes are both kept: the +/// second write reads `grants.json` only once the first has written it. +#[test] +fn grants_written_by_two_processes_into_one_store_keep_both() { + let folder = crate::tests_links::Folder::new("plugin-store-grants-take-turns"); + let (ours, theirs) = ("test.engine.permissions.grantturnsa", "test.engine.permissions.grantturnsb"); + let hash = vectorcraft_plugins::permissions::sha256_hex(b"module"); + let s = other_process(folder.0.clone()); + + let (said, heard) = std::sync::mpsc::channel::<&'static str>(); + let (handed, other) = std::sync::mpsc::channel(); + let (dir, other_hash) = (folder.0.clone(), hash.clone()); + crate::store::AFTER_GRANTS_READ.set(Some(Box::new(move || { + let thread = std::thread::spawn(move || { + tell_when_waiting(said.clone()); + let written = other_process(dir).store_grant(theirs, Some(doc_read_grant(theirs, &other_hash))); + let _ = said.send(if written { "written" } else { "not written" }); + }); + // The other write has written the file, or waits for this one. + let first = heard.recv().unwrap_or("gone"); + let _ = handed.send((first, thread)); + }))); + assert!(s.store_grant(ours, Some(doc_read_grant(ours, &hash))), "the grant is written"); + let (first, thread) = other.recv().unwrap(); + thread.join().unwrap(); + + let kept = stored_grant_hashes(&folder.0); + assert_eq!(kept.keys().map(String::as_str).collect::>(), [ours, theirs], "both grants are kept"); + assert_eq!(first, "waiting", "the other write waits for this one"); +} + +/// A setting a plug-in changes in one process and another setting it changes in a second process +/// using the same `Plug-ins` folder are both kept: the second process waits for the first's turn on +/// the folder and reads what it wrote. The second process is this test run again with +/// `VECTORCRAFT_TEST_PLUGIN_SETTINGS_STORE` naming the folder; it prints when it waits and when it +/// is done. +#[test] +fn settings_changed_by_two_processes_keep_both() { + use std::io::{BufRead, Write}; + use vectorcraft_plugins::storage::Meter; + const CHILD: &str = "VECTORCRAFT_TEST_PLUGIN_SETTINGS_STORE"; + const NAME: &str = "tests::plugins::settings_changed_by_two_processes_keep_both"; + let id = "test.engine.storagetwoprocesses"; + // Nothing a removal kept is moved back, so the storage is made without a turn on the folder. + let attached = |s: &Session| { + s.attach_plugin_storage(&crate::store::StoreTurn::unheld(), id); + vectorcraft_plugins::registry::storage(id).unwrap() + }; + if let Some(dir) = std::env::var_os(CHILD) { + let st = attached(&other_process(std::path::PathBuf::from(dir))); + crate::store::WHILE_STORE_LOCKED.set(Some(Box::new(|| { + println!("settings-child-waiting"); + let _ = std::io::stdout().flush(); + }))); + st.set_setting("theirs", b"2", &mut Meter::unlimited()).unwrap(); + println!("settings-child-done"); + let _ = std::io::stdout().flush(); + return; + } + let folder = crate::tests_links::Folder::new("plugin-settings-two-processes"); + let st = attached(&other_process(folder.0.clone())); + + let (handed, child) = std::sync::mpsc::channel(); + let dir = folder.0.clone(); + crate::store::AFTER_SETTINGS_READ.set(Some(Box::new(move || { + let mut process = std::process::Command::new(std::env::current_exe().unwrap()) + .args(["--exact", NAME, "--nocapture", "--test-threads=1"]) + .env(CHILD, &dir) + .stdout(std::process::Stdio::piped()) + .spawn() + .unwrap(); + let mut lines = std::io::BufReader::new(process.stdout.take().unwrap()).lines(); + // The other process has written the settings, or waits for this change. + let first = lines + .by_ref() + .map_while(std::io::Result::ok) + .find_map(|l| ["waiting", "done"].into_iter().find(|said| l.contains(&format!("settings-child-{said}")))); + let _ = handed.send((first, process, lines)); + }))); + st.set_setting("ours", b"1", &mut Meter::unlimited()).unwrap(); + let (first, mut process, lines) = child.recv().unwrap(); + let rest: Vec = lines.map_while(std::io::Result::ok).collect(); + assert!(process.wait().unwrap().success(), "the other process failed: {rest:?}"); + + let file: std::collections::BTreeMap = + serde_json::from_slice(&std::fs::read(crate::store::plugin_data_dir(&folder.0, id).join("settings.json")).unwrap()).unwrap(); + assert_eq!(file.keys().map(String::as_str).collect::>(), ["ours", "theirs"], "both settings are kept"); + assert_eq!(first, Some("waiting"), "the other process waits for this change"); + vectorcraft_plugins::registry::set_storage(id, None); +} + +/// When the `Plug-ins` folder can't be locked (here `disabled.lock` is a folder), `plugin.install` +/// and `plugin.remove` change nothing and say why: the plug-in installed with the id, its grant and +/// the stored module stay as they were. +#[test] +fn a_store_that_cant_be_locked_refuses_installs_and_removals() { + use vectorcraft_plugins::{permissions::sha256_hex, registry}; + let folder = crate::tests_links::Folder::new("plugin-store-not-locked"); + let id = "test.engine.permissions.notlocked"; + let first = asking(id, r#"["doc.read"]"#, &json!({"objects": []})); + let second = asking(id, r#"["doc.read"]"#, &json!({"objects": [], "documentData": null})); + let mut s = session(); + s.attach_plugin_store(folder.0.clone()); + run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&first), "grant": ["doc.read"]})); + let lock = folder.0.join(crate::store::DISABLED_LOCK); + std::fs::remove_file(&lock).unwrap(); + std::fs::create_dir(&lock).unwrap(); + let grants = std::fs::read(folder.0.join("grants.json")).unwrap(); + + let update = json!({"dataBase64": vectorcraft_format::base64_encode(&second), "grant": ["doc.read"], "replace": true}); + let refused = s.execute("plugin.install", &update).unwrap_err().to_string(); + assert!(refused.contains(crate::store::STORE_NOT_LOCKED) && refused.contains("nothing was installed"), "{refused}"); + assert_eq!(registry::get(id).map(|p| p.hash().to_string()), Some(sha256_hex(&first)), "the installed plug-in stays"); + assert_eq!(registry::granted(id).map(|g| g.hash), Some(sha256_hex(&first)), "its grant stays"); + assert_eq!(std::fs::read(s.stored_plugin_file(id).unwrap()).unwrap(), first, "the store keeps its module"); + assert_eq!(std::fs::read(folder.0.join("grants.json")).unwrap(), grants, "and its grant"); + + let refused = s.execute("plugin.remove", &json!({"id": id})).unwrap_err().to_string(); + assert!(refused.contains(crate::store::STORE_NOT_LOCKED) && refused.contains("nothing was removed"), "{refused}"); + assert!(registry::get(id).is_some(), "the plug-in stays installed"); + assert!(std::path::Path::new(&s.stored_plugin_file(id).unwrap()).exists(), "and stored"); + + std::fs::remove_dir(&lock).unwrap(); + run(&mut s, "plugin.remove", json!({"id": id})); + assert!(registry::get(id).is_none()); +} + /// Runs the test `name` of this module again in a process of its own → whether this is that /// process. The list of disabled plug-ins is kept for the whole process, and the other tests here /// read it again each time they attach a plug-in store. @@ -3740,7 +3938,7 @@ fn granted_permissions_are_kept_and_folders_skip_the_rest() { let mut third = session(); third.attach_plugin_store(folder.0.clone()); assert!(vectorcraft_plugins::registry::get(id).is_none(), "nobody granted it"); - assert_eq!(std::fs::read_dir(&folder.0).unwrap().count(), 1, "the module stays in the store"); + assert_eq!(store_files(&folder.0), 1, "the module stays in the store"); // The Additional Plug-ins Folder skips it with a note until it is granted. let dir = crate::tests_links::Folder::new("plugin-folder-grants"); @@ -4678,7 +4876,7 @@ fn plugin_storage_is_capped_and_survives_a_damaged_settings_file() { // An id that differs only in case has storage of its own, which removing this one leaves. let upper_id = "test.engine.StorageCaps"; - s.attach_plugin_storage(upper_id); + s.attach_plugin_storage(&s.plugin_store_turn().unwrap(), upper_id); let upper = vectorcraft_plugins::registry::storage(upper_id).unwrap(); assert_eq!(upper.setting("k0", m).unwrap(), None); upper.set_setting("k0", b"upper", m).unwrap(); @@ -4690,13 +4888,13 @@ fn plugin_storage_is_capped_and_survives_a_damaged_settings_file() { // Removing the plug-in closes the storage a run still holds, even one from before the plug-in // was replaced, so nothing is written back. - s.attach_plugin_storage(id); + s.attach_plugin_storage(&s.plugin_store_turn().unwrap(), id); run(&mut s, "plugin.remove", json!({"id": id})); assert_eq!(st.set_setting("zoom", b"again", m), Err(StorageError::Unavailable)); assert_eq!(st.set_cache_entry("tile", b"again", m), Err(StorageError::Unavailable)); assert!(!dir.exists()); assert_eq!(upper.setting("k0", m).unwrap(), Some(b"upper".to_vec())); - s.remove_plugin_storage(upper_id, || true).unwrap(); + s.remove_plugin_storage(&s.plugin_store_turn().unwrap(), upper_id, || true).unwrap(); } /// Files larger than the storage reads are not read: a settings file larger than 1 MiB, although @@ -4874,12 +5072,12 @@ fn removing_a_plugin_drops_its_settings_from_memory() { std::fs::create_dir_all(file.parent().unwrap()).unwrap(); std::fs::write(&file, &bytes).unwrap(); set_modified(&file, written); - s.attach_plugin_storage(id); + s.attach_plugin_storage(&s.plugin_store_turn().unwrap(), id); let st = vectorcraft_plugins::registry::storage(id).unwrap(); let mut m = Meter::unlimited(); assert_eq!(st.setting("zoom", &mut m).unwrap(), Some(b"hello".to_vec())); assert_eq!(m.used(), bytes.len() as u64, "read from the file"); - s.remove_plugin_storage(id, || true).unwrap(); + s.remove_plugin_storage(&s.plugin_store_turn().unwrap(), id, || true).unwrap(); } /// The cache's total stays in memory while the cache folder keeps its time; a write and a clear diff --git a/crates/engine/tests/plugin_fuzz.rs b/crates/engine/tests/plugin_fuzz.rs index 58cc302279..20546835c1 100644 --- a/crates/engine/tests/plugin_fuzz.rs +++ b/crates/engine/tests/plugin_fuzz.rs @@ -950,9 +950,10 @@ proptest! { prop_assert_eq!(theirs.setting("mine", m).unwrap(), Some(b"kept".to_vec())); prop_assert_eq!(theirs.cache_entry("mine", m).unwrap(), Some(b"kept".to_vec())); prop_assert_eq!(theirs.cache_bytes(), 4); - // The host names every file: the store holds the two modules, their grants and their folders. + // The host names every file: the store holds the two modules, their grants, their folders and + // the file windows lock while they change the store. let names = |dir: &std::path::Path| std::fs::read_dir(dir).unwrap().filter_map(Result::ok).map(|e| e.file_name()).collect::>(); - prop_assert!(names(&folder).len() <= 4, "{:?}", names(&folder)); + prop_assert!(names(&folder).len() <= 5, "{:?}", names(&folder)); prop_assert!(names(&folder.join("storage")).len() <= 2, "{:?}", names(&folder.join("storage"))); let _ = s.execute("plugin.remove", &json!({"id": "fuzz.storage"})); let _ = s.execute("plugin.remove", &json!({"id": "fuzz.storage.other"})); diff --git a/docs/mcp.md b/docs/mcp.md index f756bc8c97..935a57f28e 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -9587,7 +9587,9 @@ and turn, shear and flip about it (`origin`). WebAssembly plug-ins ([plugins.md](plugins.md)) are reached through commands; the desktop app and an unconfined `vectorcraft-cli` keep the ones `plugin.install` installs in a `Plug-ins` folder next to the preferences and install -them again at startup (`plugin.remove` deletes them there): +them again at startup (`plugin.remove` deletes them there). A window or process locks `disabled.lock` in that folder +while it changes it, so installs, removals, grants and plug-in settings from two of them do not mix; when the file can't +be locked, `plugin.install` and `plugin.remove` fail and change nothing: `plugin.install {path | dataBase64}` installs one, `plugin.list` / `plugin.info {id}` describe them (parameters, schema, defaults). An object filter runs on the selected paths and compound paths with `plugin.run {id, params}` (one undo step; it may change, delete or add objects, and the result is selected). A filter whose manifest lists `inputs` diff --git a/docs/plugins.md b/docs/plugins.md index 5f1479748e..c2c7617cea 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -269,7 +269,13 @@ capability model proposed for PhotoCraft plug-ins while `plugin.install` checks a module, the install is refused, before anything is written to the `Plug-ins` folder, if the other plug-in would need `replace: true` or the changed grant no longer approves the module. A `plugin.remove` in another session waits until the install has written the `Plug-ins` folder, and then removes the - module and its grant. Loading the `Plug-ins` folder or a plug-ins folder in another session waits for it too. When + module and its grant. Loading the `Plug-ins` folder or a plug-ins folder in another session waits for it too. + Another process using the same `Plug-ins` folder, for example `vectorcraft-cli` beside the app, waits as well: each + window or process locks `disabled.lock` in the folder while it reads and changes the stored modules, `grants.json`, + `disabled.json` and the plug-ins' settings and caches. An install therefore keeps a module with its own grant, and a + grant or setting written in one process does not undo one written in another. When `disabled.lock` can't be + locked, `plugin.install` and `plugin.remove` fail and change nothing, `plugin.setEnabled` keeps its change in the + process until a later change can be saved, and loading the folder writes nothing to it. When an install of a module that lists no permissions can't write the module to the `Plug-ins` folder, or can't remove the grant from `grants.json`, the folder keeps the module it held and `grants.json` keeps the grant. The new module runs until the session ends. When the grant can't be diff --git a/docs/target-app-parity.md b/docs/target-app-parity.md index 296bd84b2b..1f2409904a 100644 --- a/docs/target-app-parity.md +++ b/docs/target-app-parity.md @@ -1943,6 +1943,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur - **A dialog's OK runs what it was opened for:** `ui.dialog.set`, and MCP's `answer_dialog` through it, can set a `__` field only when it is a control of that kind of dialog (a section or tab shown, a link between two fields, a name or search being typed). Every other `__` field, such as the `__command` and `__params` of a confirmation or the generic parameter dialog, a plug-in command dialog's `__command` and `__plugin`, or the paste a swatch conflict waits to finish, is refused with "the `` dialog cannot set protected field ``"; the request sets no field, and with `action: "confirm"` OK does not run. Regressions: `crates/ui-egui/src/tests/automation.rs`, `crates/ui-egui/src/control/tests.rs`. Scores and hours unchanged. - **Only an approval records a plug-in grant (G17):** `vectorcraft_plugins::registry::grant` takes a `GrantToken`, which the engine makes after an approval and when it loads `grants.json`, and `registry::install`, `install_bytes` and folder loads refuse a module that lists permissions no recorded grant gives this very module (`Error::NeedsApproval`), leaving the plug-in under its id and the revision as they were. A module without `permissions`, or with an empty list, installs as before. `docs/plugins.md` says what the approval does not protect against (native code linked into the app, another program running as the same user, the unsigned `grants.json`). Regressions: `crates/plugins/src/registry.rs`, `crates/plugins/tests/grant_sites.rs`, `crates/engine/src/tests/plugins.rs`. Scores and hours unchanged. - **A refused plug-in install writes nothing (G17):** `plugin.install` checks the plug-in installed under the module's id and its grant again while it records the grant and installs the module (`registry::install_checked`), and only then writes the module and its grant to the `Plug-ins` folder, attaches its settings and cache and turns a disabled plug-in on. When another session installed a plug-in with the id that would need `replace: true`, or changed the grant the checks relied on, the install is refused and the folder, `grants.json`, the disabled list and the installed plug-in stay as they were. Without a plug-in store, the approval comes from the grant the install checks again. A `plugin.remove`, or a load of the folder or of a plug-ins folder, in another session waits until the install has written the folder. VectorCraft's own rule. Regressions: `crates/engine/src/tests/plugins.rs`. Scores and hours unchanged. +- **Windows and processes take turns on the Plug-ins folder:** each one locks `disabled.lock` in the `Plug-ins` folder while it reads and changes the stored modules, `grants.json`, `disabled.json` and the plug-ins' settings and caches, so two installs of one id keep each module with its own grant, and grants or settings written by two processes are all kept. When the file can't be locked, `plugin.install` and `plugin.remove` fail and change nothing. VectorCraft's own rule. Regressions: `crates/engine/src/tests/plugins.rs`. Scores and hours unchanged. - **Plug-in approval waits before OK (G17):** the approval dialog's OK stays disabled until its question has been on screen in the focused window for half a second, VectorCraft's own value; a click, Enter, Space, or a press begun before then installs nothing, and the dialog stays open. The wait starts over for every new question, even one that reads the same, and when the window gets the focus back, so a question swapped in just before a click is not approved by it. Cancel works at once. Regressions: `crates/ui-egui/src/tests/plugins.rs`. Scores and hours unchanged. - **An install automation asked for needs the person to start it (G17):** an approval question that a control request, MCP, injected input or a file dialog one of them opened leads to keeps OK disabled and refuses every confirm, a person's click included; it says to choose Object › Plug-ins › Install Plug-in…, and `ui.installPlugin` answers `approval: "personMustStartInstall"`. The person's own Install Plug-in…, File › Open or drop inspects the module again and asks a new question. Automation's `replace: true` no longer skips the question about taking another module's place, and that question now opens in the approval dialog for everyone. VectorCraft's own rule. Regressions: `crates/ui-egui/src/tests/automation.rs`, `crates/ui-egui/src/tests/plugins.rs`. Scores and hours unchanged. - **A plug-in with no permissions ends its id's grant (G17):** installing a module that lists no permissions under a granted id forgets the grant, in the process and in `grants.json`, so the formerly approved bytes need a new approval. Attaching a `Plug-ins` folder whose module for that id lists no permissions forgets it too, and plug-in folders forget the process's grant. A refused install leaves the grant. Regressions: `crates/engine/src/tests/plugins.rs`, `crates/engine/tests/plugin_safe_mode.rs`. Scores and hours unchanged.