Merge pull request #6670 from dotCooCoo/fix/plugin-put-back-result

Plug-ins: an update left in the store by a failed put-back turns a disabled plug-in on

crates/engine/src/cmd/plugin.rs

@@ -741,17 +741,18 @@ fn install(s: &mut Session, p: &Value) -> Result<Value> {
     }
     // 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, and the grant then doesn't cover
-    // the new module. A stored module that can't be read back isn't replaced by one that needs a
-    // grant.
+    // 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(plugin.id());
     let mut stored = (previous.is_ok() || !m.asks_permissions()) && s.store_plugin(plugin.id(), &bytes);
+    let holds_new = |s: &Session| s.stored_plugin_bytes(plugin.id()).is_ok_and(|held| held.as_deref() == Some(&bytes[..]));
     if m.asks_permissions() {
         let g = Grant { id: m.id.clone(), hash: plugin.hash().to_string(), permissions: wanted };
         registry::grant(g.clone());
         if stored && !s.store_grant(&m.id, Some(g)) {
-            s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref());
-            stored = false;
+            stored = !s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref()) && holds_new(s);
         }
     } else {
         // This module takes the id over, so nothing the user approved for the id applies any more.
@@ -764,7 +765,7 @@ fn install(s: &mut Session, p: &Value) -> Result<Value> {
         // stays; one that can't be opened or read may approve the old bytes once it reads again.
         if stored && !s.store_grant(&m.id, None) && s.holds_plugin_grant(&m.id) {
             let back = previous.as_ref().is_ok_and(|held| s.put_back_plugin(plugin.id(), held.as_deref()));
-            stored = !back;
+            stored = !back && holds_new(s);
             if !back || matches!(previous, Ok(None)) {
                 s.ignore_stored_grant(&m.id);
             }

crates/engine/src/store.rs

@@ -190,6 +190,9 @@ fn lock_disabled(_: &Path) -> Option<std::fs::File> {
 thread_local! {
     /// Tests: [`Session::put_back_plugin`] fails on this thread, as on a full volume.
     pub(crate) static PUT_BACK_FAILS: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
+    /// Tests: [`Session::put_back_plugin`] writes the old module back on this thread and then
+    /// reports a failure, as when the last flush of a copy fails.
+    pub(crate) static PUT_BACK_FAILS_AFTER_WRITE: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
     /// Tests: moving a plug-in's settings and cache back from `<folder>.removed` ([`move_back`])
     /// fails on this thread, as when another program holds a file in the folder.
     pub(crate) static MOVE_BACK_FAILS: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
@@ -504,16 +507,21 @@ 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
-    /// store now holds `previous`.
+    /// 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 {
         #[cfg(test)]
         if PUT_BACK_FAILS.get() {
             return false;
         }
-        match previous {
+        let back = match previous {
             Some(bytes) => self.store_plugin(id, bytes),
             None => self.unstore_plugin(id),
+        };
+        #[cfg(test)]
+        if PUT_BACK_FAILS_AFTER_WRITE.get() {
+            return false;
         }
+        back
     }
 
     /// The file the plug-in `id` is stored in, spelled as [`Session::load_stored_plugins`] names it

crates/engine/src/tests/plugins.rs

@@ -2297,6 +2297,194 @@ fn a_failed_store_write_keeps_the_stored_module_with_its_grant() {
     vectorcraft_plugins::registry::revoke(id);
 }
 
+/// 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.
+fn in_own_process(name: &str) -> bool {
+    const CHILD: &str = "VECTORCRAFT_TEST_PLUGIN_DISABLED_LIST";
+    if std::env::var_os(CHILD).is_some() {
+        return true;
+    }
+    let output = std::process::Command::new(std::env::current_exe().unwrap())
+        .args(["--exact", &format!("tests::plugins::{name}"), "--nocapture"])
+        .env(CHILD, "1")
+        .output()
+        .unwrap();
+    let stdout = String::from_utf8_lossy(&output.stdout);
+    assert!(
+        output.status.success() && stdout.contains("test result: ok. 1 passed; 0 failed;"),
+        "{name} failed in its own process:\n{stdout}\n{}",
+        String::from_utf8_lossy(&output.stderr),
+    );
+    false
+}
+
+/// An update whose grant can't be written, and whose old module can't be put back either, stays in
+/// the store: a disabled plug-in is turned on, as for any update the store holds. The old grant
+/// doesn't cover the new module, so the next start skips it until it is approved.
+#[test]
+fn an_update_left_in_the_store_by_a_failed_put_back_turns_a_disabled_plugin_on() {
+    if !in_own_process("an_update_left_in_the_store_by_a_failed_put_back_turns_a_disabled_plugin_on") {
+        return;
+    }
+    use vectorcraft_plugins::{permissions::sha256_hex, registry, safe_mode};
+    let folder = crate::tests_links::Folder::new("plugin-store-failed-put-back-update");
+    let id = "test.engine.permissions.putbackupdate";
+    let first = asking(id, r#"["doc.read"]"#, &json!({"objects": []}));
+    let changed = asking(id, r#"["doc.read","doc.write"]"#, &json!({"objects": []}));
+    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"]}));
+    run(&mut s, "plugin.setEnabled", json!({"id": id, "enabled": false}));
+    let grants = folder.0.join("grants.json");
+    let recorded = std::fs::read(&grants).unwrap();
+    std::fs::remove_file(&grants).unwrap();
+    std::fs::create_dir_all(grants.join("block")).unwrap();
+
+    crate::store::PUT_BACK_FAILS.set(true);
+    let update = json!({"dataBase64": vectorcraft_format::base64_encode(&changed), "grant": ["doc.read", "doc.write"], "replace": true});
+    let installed = s.execute("plugin.install", &update);
+    crate::store::PUT_BACK_FAILS.set(false);
+    assert_eq!(installed.unwrap()["hash"], sha256_hex(&changed), "the new module runs in this session");
+    assert_eq!(registry::get(id).map(|p| p.hash().to_string()), Some(sha256_hex(&changed)));
+    assert_eq!(std::fs::read(s.stored_plugin_file(id).unwrap()).unwrap(), changed, "the store holds the new module");
+    assert!(safe_mode::disabled().is_empty(), "the module the store holds is turned on");
+    let saved = std::fs::read(folder.0.join(crate::store::DISABLED_FILE)).unwrap();
+    assert!(safe_mode::parse_disabled_file(&saved).is_empty(), "nothing disabled is saved");
+
+    std::fs::remove_dir_all(&grants).unwrap();
+    std::fs::write(&grants, &recorded).unwrap();
+    registry::remove(id);
+    registry::revoke(id);
+    let mut next = session();
+    next.attach_plugin_store(folder.0.clone());
+    assert!(registry::get(id).is_none(), "the old grant doesn't cover the new module");
+    safe_mode::set_disabled(vec![]);
+}
+
+/// An update whose grant can't be written, and whose put-back writes the old module and then
+/// reports a failure, leaves the old module in the store: a disabled plug-in stays disabled, and the
+/// next start doesn't load it.
+#[test]
+fn an_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off() {
+    if !in_own_process("an_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off") {
+        return;
+    }
+    use vectorcraft_plugins::{registry, safe_mode};
+    let folder = crate::tests_links::Folder::new("plugin-store-put-back-fails-after-write");
+    let id = "test.engine.permissions.putbacklate";
+    let first = asking(id, r#"["doc.read"]"#, &json!({"objects": []}));
+    let changed = asking(id, r#"["doc.read","doc.write"]"#, &json!({"objects": []}));
+    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"]}));
+    run(&mut s, "plugin.setEnabled", json!({"id": id, "enabled": false}));
+    let disabled = safe_mode::disabled();
+    assert!(!disabled.is_empty());
+    let grants = folder.0.join("grants.json");
+    let recorded = std::fs::read(&grants).unwrap();
+    std::fs::remove_file(&grants).unwrap();
+    std::fs::create_dir_all(grants.join("block")).unwrap();
+
+    crate::store::PUT_BACK_FAILS_AFTER_WRITE.set(true);
+    let update = json!({"dataBase64": vectorcraft_format::base64_encode(&changed), "grant": ["doc.read", "doc.write"], "replace": true});
+    let installed = s.execute("plugin.install", &update);
+    crate::store::PUT_BACK_FAILS_AFTER_WRITE.set(false);
+    installed.unwrap();
+    assert_eq!(std::fs::read(s.stored_plugin_file(id).unwrap()).unwrap(), first, "the store holds the old module");
+    assert_eq!(safe_mode::disabled(), disabled, "the plug-in stays disabled");
+    let saved = std::fs::read(folder.0.join(crate::store::DISABLED_FILE)).unwrap();
+    assert!(!safe_mode::parse_disabled_file(&saved).is_empty(), "the disabled list on disk is kept");
+
+    std::fs::remove_dir_all(&grants).unwrap();
+    std::fs::write(&grants, &recorded).unwrap();
+    registry::remove(id);
+    registry::revoke(id);
+    let mut next = session();
+    next.attach_plugin_store(folder.0.clone());
+    assert!(registry::get(id).is_none(), "the disabled old module doesn't load");
+    safe_mode::set_disabled(vec![]);
+}
+
+/// A module without permissions whose install can't remove the old grant, and whose put-back writes
+/// the old module and then reports a failure, leaves the old module in the store: a disabled plug-in
+/// stays disabled.
+#[test]
+fn a_plain_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off() {
+    if !in_own_process("a_plain_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off") {
+        return;
+    }
+    use vectorcraft_plugins::safe_mode;
+    let folder = crate::tests_links::Folder::new("plugin-store-plain-put-back-fails-after-write");
+    let id = "test.engine.permissions.plainputbacklate";
+    let empty = json!({"objects": []});
+    let approved = asking(id, r#"["doc.read"]"#, &empty);
+    let plain = asking(id, "null", &empty);
+    let grants = folder.0.join("grants.json");
+    let mut s = session();
+    s.attach_plugin_store(folder.0.clone());
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&approved), "grant": ["doc.read"]}));
+    run(&mut s, "plugin.setEnabled", json!({"id": id, "enabled": false}));
+    let disabled = safe_mode::disabled();
+    assert!(!disabled.is_empty());
+    let file = s.stored_plugin_file(id).unwrap();
+    // `1e9` is written back as `1000000000.0`, which makes the rewritten file larger than the 1 MiB that is read.
+    let entry = serde_json::from_slice::<Vec<Value>>(&std::fs::read(&grants).unwrap()).unwrap().remove(0);
+    let full = format!(r#"[{entry},{{"n":[{}]}}]"#, vec!["1e9"; 200_000].join(",")).into_bytes();
+    std::fs::write(&grants, &full).unwrap();
+
+    crate::store::PUT_BACK_FAILS_AFTER_WRITE.set(true);
+    let installed = s.execute("plugin.install", &json!({"dataBase64": vectorcraft_format::base64_encode(&plain), "replace": true}));
+    crate::store::PUT_BACK_FAILS_AFTER_WRITE.set(false);
+    installed.unwrap();
+    assert_eq!(std::fs::read(&file).unwrap(), approved, "the store holds the old module");
+    assert_eq!(safe_mode::disabled(), disabled, "the plug-in stays disabled");
+    let saved = std::fs::read(folder.0.join(crate::store::DISABLED_FILE)).unwrap();
+    assert!(!safe_mode::parse_disabled_file(&saved).is_empty(), "the disabled list on disk is kept");
+
+    std::fs::remove_file(&grants).unwrap();
+    run(&mut s, "plugin.remove", json!({"id": id}));
+    safe_mode::set_disabled(vec![]);
+}
+
+/// A first install whose grant can't be written, and whose module can't be taken out of the store
+/// again, stays in the store: the plug-in's file is turned on. No grant covers the module, so the
+/// next start skips it until it is approved.
+#[test]
+fn a_first_install_left_in_the_store_by_a_failed_put_back_turns_its_file_on() {
+    if !in_own_process("a_first_install_left_in_the_store_by_a_failed_put_back_turns_its_file_on") {
+        return;
+    }
+    use vectorcraft_plugins::{registry, safe_mode};
+    let folder = crate::tests_links::Folder::new("plugin-store-failed-put-back-first");
+    let id = "test.engine.permissions.putbackfirst";
+    let first = asking(id, r#"["doc.read"]"#, &json!({"objects": []}));
+    let mut s = session();
+    s.attach_plugin_store(folder.0.clone());
+    let module = s.stored_plugin_file(id).unwrap();
+    run(&mut s, "plugin.setEnabled", json!({"file": module, "enabled": false}));
+    let grants = folder.0.join("grants.json");
+    std::fs::create_dir_all(grants.join("block")).unwrap();
+
+    crate::store::PUT_BACK_FAILS.set(true);
+    let installed = s.execute("plugin.install", &json!({"dataBase64": vectorcraft_format::base64_encode(&first), "grant": ["doc.read"]}));
+    crate::store::PUT_BACK_FAILS.set(false);
+    installed.unwrap();
+    assert!(registry::get(id).is_some(), "the module runs in this session");
+    assert_eq!(std::fs::read(&module).unwrap(), first, "the store holds the module");
+    assert!(safe_mode::disabled().is_empty(), "the file the store holds is turned on");
+    let saved = std::fs::read(folder.0.join(crate::store::DISABLED_FILE)).unwrap();
+    assert!(safe_mode::parse_disabled_file(&saved).is_empty(), "nothing disabled is saved");
+
+    std::fs::remove_dir_all(&grants).unwrap();
+    registry::remove(id);
+    registry::revoke(id);
+    let mut next = session();
+    next.attach_plugin_store(folder.0.clone());
+    assert!(registry::get(id).is_none(), "no grant covers the module");
+    safe_mode::set_disabled(vec![]);
+}
+
 /// A plug-in folder reports a changed module as needing approval and still loads the other files;
 /// granted on its own, the changed module loads from the folder. A session without a store uses the
 /// grants held by the process.

docs/mcp.md

@@ -7953,7 +7953,9 @@ pendingItem}`; a command does not turn Safe Mode on or off. `plugin.setEnabled {
 off for the next launch, as Help › Safe Mode's Enable on Re-launch does; it does not load, unload or uninstall anything
 now, and `plugin.install` of a disabled plug-in turns it on. Turning a plug-in on by `id` or by installing it also turns
 on the file it is stored in, which names it when it stopped a launch while loading; when the new version can't be saved
-over that file, the plug-in stays disabled. `plugin.list` reports `safeMode` and `disabled`, and
+over that file, the plug-in stays disabled. When the new version is saved but its grant can't be, and the file can't be
+returned to what it held before (the old module, or no file), the plug-in is turned on, and the next launch skips it
+until it is approved. `plugin.list` reports `safeMode` and `disabled`, and
 `plugin.reload` skips disabled plug-ins. A change to the disabled list is saved on top of the list the file holds at
 that moment, so a change another window saved to the same preferences meanwhile is kept. When the list file can't be
 read (it is too large) or holds another format or version, it is left as it is; a file that isn't JSON is replaced.

docs/plugins.md

@@ -245,7 +245,9 @@ capability model proposed for PhotoCraft plug-ins
   records the new hash. When the new module or its grant can't be written to the `Plug-ins` folder, the folder keeps
   the old module and its grant, and the old module loads at the next start. If the write that puts the old module back
   fails as well, the folder holds the new module with the old grant, the grant does not cover that module, and the
-  plug-in is skipped at the next start until it is approved again. A grant whose hash is not 64 lower-case
+  plug-in is skipped at the next start until it is approved again. When the folder held no module for that id, that
+  write removes the new module; when the removal fails, the folder holds the new module and no grant covers it. In
+  both cases a disabled plug-in is turned on, as for any install the folder holds. A grant whose hash is not 64 lower-case
   hexadecimal digits covers nothing. `plugin.remove` forgets the grant, and so does a successful `plugin.install` of a
   module that lists no permissions under that id, so the earlier bytes need a new approval. When `plugin.remove` can't
   remove the grant from `grants.json`, it returns an error, the plug-in stays installed, and a second `plugin.remove`

docs/target-app-parity.md

@@ -1674,6 +1674,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur
 - **Plug-in data kept with the document (G17):** a filter writes JSON of at most 1 MiB with `documentData` (one undo step with its objects; `null` removes it) and reads it back in `_context.documentData`, which no other filter is given. It is the document's foreign data (`unknown.pluginData`, by plug-in id), as the perspective grid is, so the native file keeps it with no section of its own; an exporter is given the whole document, so it can read every plug-in's. Regressions: `crates/engine/src/tests/plugins.rs`, `crates/engine/tests/plugin_export.rs` (the exporter), `crates/doc/src/plugin_data.rs`, `crates/plugins/src/objects/tests.rs`. Scores and hours unchanged.
 - **Heavy filter plug-ins (G17):** a filter's manifest may ask for `"budget": "heavy"`: 1 G instructions (20 times a light run's) and 300 s a run, for routing, packing or simulation. Install Plug-in… and File › Open ask the user first, `plugin.install` needs `allowHeavy: true` (`plugin.inspect` tells beforehand), the Additional Plug-ins Folder skips such filters, and the plug-in store keeps the answer. Its dialog has no Preview; the app runs it on a thread of its own with a progress window (Cancel, Continue in Background) and applies the result as `plugin.run` with the same parameters, one undo step, unless the document or the plug-in changed meanwhile. Regressions: `crates/plugins/tests/abi.rs`, `crates/plugins/src/manifest.rs`, `crates/engine/src/tests/plugins.rs`, `crates/ui-egui/src/tests/plugin_runs.rs`. Scores and hours unchanged.
 - **Safe Mode for plug-ins (G17):** `vectorcraft --safe-mode` and `vectorcraft-cli --safe-mode <subcommand>` start without plug-ins. When a launch stopped while a plug-in was loading or drawing its first live effect, the next launch names that plug-in, without saying it caused the stop, and offers Disable and Continue, Start Normally or Start in Safe Mode. Help › Safe Mode lists the disabled plug-ins with Enable on Re-launch, which takes effect at the next launch, and the app bar and the window title show Safe Mode. The help page (https://web.archive.org/web/20210601162814/https://helpx.adobe.com/pdf/illustrator_reference.pdf, pages 65 to 69) describes the same choice between diagnosing and starting normally, a Safe Mode with the problem item off and the rest of the app working, and turning the item back on at the next launch. In Safe Mode no plug-in loads or runs, and a live-effect plug-in draws its object's geometry unchanged while the object keeps the effect, which the Appearance panel marks "(Safe Mode)" with the plug-in's id in its tooltip and `effect.list` marks `skipped`; Expand Appearance refuses such an object. The web build keeps no plug-ins between page loads and has no marker. Each desktop launch with preferences keeps a marker in the `Launch` folder next to them; `vectorcraft-cli` and a desktop launch without preferences keep none, so a stop there leads to no choice at the next launch. Disabled plug-ins are listed in `Plug-ins/disabled.json`. `plugin.safeMode` reports Safe Mode and the disabled plug-ins, and `plugin.setEnabled` changes the list. Regressions: `crates/plugins/src/safe_mode.rs`, `crates/plugins/tests/safe_mode.rs`, `crates/engine/tests/plugin_safe_mode.rs`, `crates/ui-egui/tests/safe_mode.rs`. Scores and hours unchanged.
+- **A disabled plug-in whose update stays in the store is turned on:** when `plugin.install` writes a module that lists permissions to the `Plug-ins` folder, its grant can't be written, and the folder can't be returned to the old module (or, on a first install, the new module can't be removed), the folder holds the new module, so a disabled plug-in is turned on, by id and by stored file, as for any install the folder holds. The old grant, or none, doesn't cover the new module, so the next start skips it until it is approved. Regressions: `crates/engine/src/tests/plugins.rs`. Scores and hours unchanged.
 - **Plug-in instruction budget at a bulk memory instruction:** a run that pauses where one slice of its instruction budget ends keeps the instructions the slice still held. A `memory.fill` or `memory.copy` that the slice can't pay for in full runs on the next slice, so a run whose budget covers its work finishes; a run ends with `Limit("instruction budget")` only when the instructions it has left can't pay for the next instruction. Regressions: `crates/plugins/tests/abi.rs`. Scores and hours unchanged.
 - **Bulk memory fuzz cases use the host's fuel constants:** `crates/engine/tests/plugin_fuzz.rs` samples runs around one slice of the instruction budget and lengths around the bytes one unit of fuel pays for with `vectorcraft_plugins`' own `FUEL_SLICE` and `BYTES_COPIED_PER_FUEL`, so the sampled cases move when either value changes. Regressions: `crates/engine/tests/plugin_fuzz.rs`. Scores and hours unchanged.
 - **Plug-in permissions (G17):** a manifest may list `permissions` (`doc.read`, `doc.write`, `state`, `effect.render`, `export.write`; the first three follow storytold/photocraft#662) and an `api` level; without the list a plug-in gets what its kind implies and installs as before. Install Plug-in… and File › Open show the name, id, version, file SHA-256 and each permission in plain words, with control and bidirectional characters escaped, and install nothing on Cancel; for a plug-in that lists permissions, `plugin.install` needs `grant` and folders install it only when it is granted. The grant is kept with the module's hash, a module asking for more asks again, and a filter run refuses output its permissions don't allow. Regressions: `crates/plugins/src/permissions.rs`, `crates/plugins/src/manifest.rs`, `crates/engine/src/tests/plugins.rs`, `crates/engine/tests/plugin_fuzz.rs`, `crates/ui-egui/src/tests/plugins.rs`. Scores and hours unchanged.