Plug-ins: a failed store write keeps the stored module and its grant together

plugin.install wrote the grant and the module as two separate writes,
and either could fail with only a log record. With a grant tied to the
module's hash, the stored pair then no longer matched and the plug-in
was skipped at the next start.

The module is now written first and its grant only once it is there.
When the grant can't be written, the store gets its old module back, or
loses the new one on a first install, and a disabled plug-in stays
disabled. A stored module that can't be read back is not replaced by
one that needs a grant.

crates/engine/src/cmd/plugin.rs

@@ -379,13 +379,20 @@ fn install(s: &mut Session, p: &Value) -> Result<Value> {
         let asked = permissions::names(&wanted).join(", ");
         return Err(bad(C, format!("“{name}” asks for the permissions {asked}: install it with `grant` listing them once the user agrees")));
     }
+    // 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, so the module and grant kept there always belong together. 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);
     if m.asks_permissions() {
         let g = Grant { id: m.id.clone(), hash: plugin.hash().to_string(), permissions: wanted };
         registry::grant(g.clone());
-        s.store_grant(&m.id, Some(g));
+        if stored && !s.store_grant(&m.id, Some(g)) {
+            s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref());
+            stored = false;
+        }
     }
-    // Installed plug-ins stay installed after a restart (#2541).
-    let stored = s.store_plugin(plugin.id(), &bytes);
     // 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.

crates/engine/src/store.rs

@@ -372,6 +372,29 @@ impl Session {
         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
+    /// an error, with a log record, when the file can't be read or is larger than the module limit.
+    pub(crate) fn stored_plugin_bytes(&self, id: &str) -> Result<Option<Vec<u8>>, String> {
+        let Some(dir) = self.plugin_dir.as_deref() else { return Ok(None) };
+        let path = dir.join(format!("{}.{PLUGIN_EXT}", file_stem(id)));
+        if !path.exists() {
+            return Ok(None);
+        }
+        let max = vectorcraft_plugins::Limits::default().max_module_bytes as u64;
+        read_capped(&path, max).map(Some).inspect_err(|e| log::warn!("can't read the stored plug-in {}: {e}", path.display()))
+    }
+
+    /// 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.
+    pub(crate) fn put_back_plugin(&self, id: &str, previous: Option<&[u8]>) {
+        match previous {
+            Some(bytes) => {
+                self.store_plugin(id, bytes);
+            }
+            None => self.unstore_plugin(id),
+        }
+    }
+
     /// The file the plug-in `id` is stored in, spelled as [`Session::load_stored_plugins`] names it
     /// (`None` without a plug-in folder).
     pub(crate) fn stored_plugin_file(&self, id: &str) -> Option<String> {
@@ -382,14 +405,15 @@ impl Session {
     /// 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`.
-    pub(crate) fn store_grant(&self, id: &str, grant: Option<vectorcraft_plugins::permissions::Grant>) {
-        let Some(dir) = &self.plugin_dir else { return };
+    /// 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).
+    pub(crate) fn store_grant(&self, id: &str, grant: Option<vectorcraft_plugins::permissions::Grant>) -> bool {
+        let Some(dir) = &self.plugin_dir else { return false };
         let mut file = match read_grants(dir) {
             Ok(file) => file,
             Err(e) => {
                 log::warn!("not updating the stored plug-in grants {}: {e}", dir.join(GRANTS_FILE).display());
-                return;
+                return false;
             }
         };
         let others = file.other.len();
@@ -399,22 +423,23 @@ impl Session {
             None => file.grants.remove(id).is_some(),
         } || file.other.len() != others;
         if !changed {
-            return;
+            return true;
         }
         let mut entries: Vec<serde_json::Value> = file.grants.into_values().filter_map(|g| serde_json::to_value(g).ok()).collect();
         entries.extend(file.other);
         if entries.is_empty() {
             take(dir, GRANTS_FILE);
-            return;
+            return true;
         }
         // The file is read back only up to its cap: written compactly when pretty-printed is larger,
         // and not at all when that is too.
         let fits = |bytes: &Vec<u8>| bytes.len() as u64 <= MAX_GRANTS_FILE;
         match serde_json::to_vec_pretty(&entries).ok().filter(fits).or_else(|| serde_json::to_vec(&entries).ok().filter(fits)) {
-            Some(bytes) => {
-                put(dir, GRANTS_FILE, &bytes);
+            Some(bytes) => put(dir, GRANTS_FILE, &bytes),
+            None => {
+                log::warn!("not updating the stored plug-in grants {}: {}", dir.join(GRANTS_FILE).display(), too_large(MAX_GRANTS_FILE));
+                false
             }
-            None => log::warn!("not updating the stored plug-in grants {}: {}", dir.join(GRANTS_FILE).display(), too_large(MAX_GRANTS_FILE)),
         }
     }
 

crates/engine/src/tests/plugins.rs

@@ -690,6 +690,87 @@ fn a_stored_grant_covers_only_the_stored_module_it_recorded() {
     run(&mut attached, "plugin.remove", json!({"id": id}));
 }
 
+/// A write to the plug-in store that fails leaves the store's module beside the grant that approved
+/// it, so the old module loads at the next start: a module that can't be written doesn't update the
+/// grant, and a grant that can't be written puts the old module back. On a first install, it takes
+/// the new module out again.
+#[test]
+fn a_failed_store_write_keeps_the_stored_module_with_its_grant() {
+    use vectorcraft_plugins::permissions::sha256_hex;
+    let folder = crate::tests_links::Folder::new("plugin-store-failed-write");
+    let id = "test.engine.permissions.failedwrite";
+    let first = asking(id, r#"["doc.read"]"#, &json!({"objects": []}));
+    let changed = asking(id, r#"["doc.read","doc.write"]"#, &json!({"objects": []}));
+    let first_data = vectorcraft_format::base64_encode(&first);
+    let update = json!({"dataBase64": vectorcraft_format::base64_encode(&changed), "grant": ["doc.read", "doc.write"]});
+    let mut s = session();
+    s.attach_plugin_store(folder.0.clone());
+    run(&mut s, "plugin.install", json!({"dataBase64": first_data, "grant": ["doc.read"]}));
+    let grants = folder.0.join("grants.json");
+    let recorded = std::fs::read(&grants).unwrap();
+    let module = std::path::PathBuf::from(s.stored_plugin_file(id).unwrap());
+    let starts = || {
+        vectorcraft_plugins::registry::remove(id);
+        vectorcraft_plugins::registry::revoke(id);
+        let mut next = session();
+        next.attach_plugin_store(folder.0.clone());
+        vectorcraft_plugins::registry::get(id).map(|p| p.hash().to_string())
+    };
+    let block = |path: &std::path::Path| {
+        std::fs::remove_file(path).unwrap();
+        std::fs::create_dir_all(path.join("block")).unwrap();
+    };
+
+    // The module can't be written: the grant stays the old module's.
+    block(&module);
+    run(&mut s, "plugin.install", update.clone());
+    assert_eq!(std::fs::read(&grants).unwrap(), recorded, "the grant isn't updated for a module that wasn't stored");
+    std::fs::remove_dir_all(&module).unwrap();
+    std::fs::write(&module, &first).unwrap();
+    assert_eq!(starts(), Some(sha256_hex(&first)), "the old module loads after a failed module write");
+
+    // The grants file is full: the new grant, which is longer, doesn't fit.
+    let cap = 1 << 20;
+    let mut entries: Vec<Value> = serde_json::from_slice(&recorded).unwrap();
+    entries.push(json!({"id": "", "pad": ""}));
+    let room = cap - serde_json::to_vec(&entries).unwrap().len();
+    *entries.last_mut().unwrap() = json!({"id": "", "pad": "x".repeat(room)});
+    let full = serde_json::to_vec(&entries).unwrap();
+    assert_eq!(full.len(), cap);
+    std::fs::write(&grants, &full).unwrap();
+    run(&mut s, "plugin.install", update.clone());
+    assert_eq!(std::fs::read(&grants).unwrap(), full, "not written past the cap");
+    assert_eq!(std::fs::read(&module).unwrap(), first, "the old module is put back");
+    assert_eq!(starts(), Some(sha256_hex(&first)), "the old module loads after a failed grant write");
+
+    // The grants file can't be read, so nothing can be recorded in it.
+    block(&grants);
+    run(&mut s, "plugin.install", update.clone());
+    assert_eq!(std::fs::read(&module).unwrap(), first, "the old module is put back");
+    std::fs::remove_dir_all(&grants).unwrap();
+    std::fs::write(&grants, &recorded).unwrap();
+    assert_eq!(starts(), Some(sha256_hex(&first)), "the old module loads once the grants are readable");
+
+    // A stored module that can't be read back isn't replaced, so a failed grant write can't lose it.
+    let oversized = vectorcraft_plugins::Limits::default().max_module_bytes as u64 + 1;
+    std::fs::File::options().write(true).open(&module).unwrap().set_len(oversized).unwrap();
+    block(&grants);
+    run(&mut s, "plugin.install", update);
+    assert_eq!(std::fs::metadata(&module).unwrap().len(), oversized, "the stored module stays");
+    std::fs::remove_dir_all(&grants).unwrap();
+    std::fs::write(&grants, &recorded).unwrap();
+    std::fs::write(&module, &first).unwrap();
+
+    // A first install whose grant can't be written leaves no module without a grant in the store.
+    run(&mut s, "plugin.remove", json!({"id": id}));
+    assert!(!module.exists() && !grants.exists());
+    std::fs::create_dir_all(grants.join("block")).unwrap();
+    run(&mut s, "plugin.install", json!({"dataBase64": first_data, "grant": ["doc.read"]}));
+    assert!(!module.exists(), "the store holds no module its grants don't cover");
+    vectorcraft_plugins::registry::remove(id);
+    vectorcraft_plugins::registry::revoke(id);
+}
+
 /// 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.

crates/engine/tests/plugin_safe_mode.rs

@@ -198,6 +198,25 @@ fn installing_a_disabled_plug_in_again_turns_it_on() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
+/// When the grant of an update can't be stored, the store keeps the old module, and it stays disabled.
+#[test]
+fn a_disabled_plug_in_whose_new_grant_cant_be_stored_stays_disabled() {
+    let _f = fresh();
+    let dir = tmp_dir("reinstall-no-grant");
+    let mut s = launch(&dir);
+    let manifest = r#"{"id":"test.sm.g","name":"Test","version":"1","kind":"filter","permissions":["doc.read"]}"#;
+    let module = |out: &str| vectorcraft_format::base64_encode(&wat::parse_str(tpl::returning(manifest, out)).unwrap());
+    s.execute("plugin.install", &json!({"dataBase64": module(r#"{"objects":[]}"#), "grant": ["doc.read"]})).unwrap();
+    s.execute("plugin.setEnabled", &json!({"id": "test.sm.g", "enabled": false})).unwrap();
+    let grants = dir.join("grants.json");
+    std::fs::remove_file(&grants).unwrap();
+    std::fs::create_dir_all(grants.join("block")).unwrap();
+    s.execute("plugin.install", &json!({"dataBase64": module(r#"{"objects":[],"error":null}"#), "grant": ["doc.read"]})).unwrap();
+    assert_eq!(safe_mode::disabled().len(), 1, "the module the update could not replace stays disabled");
+    safe_mode::set_disabled(vec![]);
+    let _ = std::fs::remove_dir_all(&dir);
+}
+
 /// A launch where the stored plug-in `id` stopped while loading: it is installed, then disabled by
 /// its stored file alone (as Disable and Continue saves it), and the next launch skips it.
 fn stored_plug_in_disabled_by_file(dir: &std::path::Path, id: &str) -> Session {

docs/plugins.md

@@ -136,7 +136,9 @@ capability model proposed for PhotoCraft plug-ins
 - The grant is recorded with the plug-in's id, the module's hash and the permissions, in `grants.json` in the
   `Plug-ins` folder, and covers only that module. Installing the same bytes again needs no new approval. A module with
   other bytes asks again, even when it lists the same permissions. Until it is approved, the installed module and its
-  grant stay as they are; once it is, the grant records the new hash. A grant whose hash is not 64 lower-case
+  grant stay as they are; once it is, the grant 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. A grant whose hash is not 64 lower-case
   hexadecimal digits covers nothing. `plugin.remove` forgets the grant. An entry of `grants.json` that this version can't use (a
   permission it doesn't know, an invalid id) is skipped and stays in the file until its plug-in is granted again or
   removed. A `grants.json` that is not a JSON list is never overwritten, and the file is never written larger than