Plug-ins: a put-back that fails after writing the old module keeps a disabled plug-in off

When plugin.install can't write or remove a grant and the write that puts
the old module back reports a failure, install now reads the stored module
again and turns the plug-in on only when the store holds the new module. A
put-back that wrote the old bytes and then failed (for example at its last
flush) leaves a disabled plug-in disabled. This applies to modules that
list permissions and to modules that don't.

crates/engine/src/cmd/plugin.rs

@@ -742,15 +742,17 @@ 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: the store then holds the new module,
-    // which the grant doesn't cover. A stored module that can't be read back isn't replaced by one
-    // that needs a grant.
+    // 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)) {
-            stored = !s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref());
+            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.
@@ -763,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) };
 }
 
 /// Remove `dir/file` → whether it is gone; a file that isn't there is what was wanted.
@@ -498,16 +501,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

@@ -2243,6 +2243,91 @@ fn an_update_left_in_the_store_by_a_failed_put_back_turns_a_disabled_plugin_on()
     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.