PDF presets: a rename that can't be written keeps the preset under its old name

Renaming a saved PDF preset (pdf.preset.save with newName) removed
the old file from the PDF Presets folder before it wrote the new one.
When that write failed, for example on a full disk, the folder held no
file for the preset, and the preset was gone at the next start.

The store now writes the new file first and removes the old one only
when that write succeeded. A failed write leaves the preset in the
folder under its old name. A rename that changes only the case of the
name writes the old file and then renames it, and the old file gets
its content back when the rename fails. The command still succeeds
when the folder can't be written, and the failure is logged, as
before.

crates/engine/src/store.rs

@@ -133,22 +133,46 @@ impl Session {
     }
 
     /// Write the saved preset `preset` to the store, which held it as `old_name` when that is set
-    /// (a rename). Without a store, nothing happens.
+    /// (a rename). A rename writes the new file first and removes the old one only when that write
+    /// succeeded: after a failed write the store still holds the preset under its old name. When
+    /// the two names differ only in case, the old file is written and then renamed to the new name;
+    /// when that rename fails, the old file gets its content back. Without a store, nothing happens.
     pub(crate) fn store_pdf_preset(&self, old_name: Option<&str>, preset: &PdfPreset) {
         let Some(dir) = &self.pdf_preset_dir else { return };
         let mut clean = preset.clone();
         clean.settings.clear_passwords();
         let file = format!("{}.{PRESET_FORMAT}", file_stem(&clean.name));
-        if let Some(old) = old_name.map(|o| format!("{}.{PRESET_FORMAT}", file_stem(o)))
-            && old != file
-        {
-            take(dir, &old);
-        }
-        match serde_json::to_vec_pretty(&PresetFile::new(vec![clean])) {
-            Ok(bytes) => {
+        let bytes = match serde_json::to_vec_pretty(&PresetFile::new(vec![clean])) {
+            Ok(bytes) => bytes,
+            Err(e) => {
+                log::warn!("can't save the PDF preset `{}`: {e}", preset.name);
+                return;
+            }
+        };
+        match old_name.map(|o| format!("{}.{PRESET_FORMAT}", file_stem(o))).filter(|old| *old != file) {
+            // Names equal in any case get file names that differ only in case. A file system that
+            // ignores case holds them as one file, which removing the old name would delete.
+            Some(old) if old.to_lowercase() == file.to_lowercase() => match read_capped(&dir.join(&old), MAX_PRESET_FILE) {
+                Ok(before) => {
+                    if put(dir, &old, &bytes)
+                        && let Err(e) = std::fs::rename(dir.join(&old), dir.join(&file))
+                    {
+                        log::warn!("can't rename {} to {file}: {e}", dir.join(&old).display());
+                        put(dir, &old, &before);
+                    }
+                }
+                Err(_) => {
+                    put(dir, &file, &bytes);
+                }
+            },
+            Some(old) => {
+                if put(dir, &file, &bytes) {
+                    take(dir, &old);
+                }
+            }
+            None => {
                 put(dir, &file, &bytes);
             }
-            Err(e) => log::warn!("can't save the PDF preset `{}`: {e}", preset.name),
         }
     }
 

crates/engine/src/tests_store.rs

@@ -40,6 +40,82 @@ fn saved_presets_come_back_in_the_next_session_and_renames_and_deletes_follow()
     assert!(!dir.join(format!("{}.vcpdfpresets", file_stem("Old"))).exists());
 }
 
+#[test]
+fn a_rename_that_cannot_be_written_keeps_the_preset_under_its_old_name() {
+    let dir = fresh_dir("rename-fails").join("PDF Presets");
+    let mut a = Session::new();
+    a.attach_pdf_preset_store(dir.clone());
+    a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap();
+    // The test puts a folder where the new name's file goes, so the write fails as it would on a full disk.
+    std::fs::create_dir(dir.join(format!("{}.vcpdfpresets", file_stem("Screen")))).unwrap();
+    a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "Screen"})).unwrap();
+
+    let mut b = Session::new();
+    b.attach_pdf_preset_store(dir.clone());
+    assert_eq!(saved_names(&mut b), ["Web"]);
+    assert_eq!(b.prefs.pdf_presets[0].description, "for screens");
+    assert!(dir.join(format!("{}.vcpdfpresets", file_stem("Web"))).is_file());
+}
+
+#[test]
+fn a_rename_that_only_changes_case_keeps_one_file() {
+    let dir = fresh_dir("rename-case").join("PDF Presets");
+    let mut a = Session::new();
+    a.attach_pdf_preset_store(dir.clone());
+    a.execute("pdf.preset.save", &json!({"name": "Web"})).unwrap();
+    a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB", "description": "renamed"})).unwrap();
+
+    let mut b = Session::new();
+    b.attach_pdf_preset_store(dir.clone());
+    assert_eq!(saved_names(&mut b), ["WEB"]);
+    assert_eq!(b.prefs.pdf_presets[0].description, "renamed");
+    assert_eq!(file_names(&dir), [format!("{}.vcpdfpresets", file_stem("WEB"))]);
+}
+
+fn file_names(dir: &std::path::Path) -> Vec<String> {
+    std::fs::read_dir(dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect()
+}
+
+/// Windows does not allow replacing a read-only file or writing into it, so the write fails.
+#[cfg(windows)]
+#[test]
+fn a_rename_that_only_changes_case_and_cannot_be_written_keeps_the_old_file() {
+    let dir = fresh_dir("rename-case-fails").join("PDF Presets");
+    let mut a = Session::new();
+    a.attach_pdf_preset_store(dir.clone());
+    a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap();
+    let old = dir.join(format!("{}.vcpdfpresets", file_stem("Web")));
+    let writable = std::fs::metadata(&old).unwrap().permissions();
+    let mut read_only = writable.clone();
+    read_only.set_readonly(true);
+    std::fs::set_permissions(&old, read_only).unwrap();
+    a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB"})).unwrap();
+    std::fs::set_permissions(&old, writable).unwrap();
+
+    let mut b = Session::new();
+    b.attach_pdf_preset_store(dir.clone());
+    assert_eq!(saved_names(&mut b), ["Web"]);
+    assert_eq!(b.prefs.pdf_presets[0].description, "for screens");
+    assert_eq!(file_names(&dir), [format!("{}.vcpdfpresets", file_stem("Web"))]);
+}
+
+/// Where file names keep case apart, a folder named like the new file makes the rename fail.
+#[cfg(not(any(windows, target_vendor = "apple")))]
+#[test]
+fn a_rename_that_only_changes_case_and_cannot_be_moved_keeps_the_old_preset() {
+    let dir = fresh_dir("rename-case-blocked").join("PDF Presets");
+    let mut a = Session::new();
+    a.attach_pdf_preset_store(dir.clone());
+    a.execute("pdf.preset.save", &json!({"name": "Web", "description": "for screens"})).unwrap();
+    std::fs::create_dir(dir.join(format!("{}.vcpdfpresets", file_stem("WEB")))).unwrap();
+    a.execute("pdf.preset.save", &json!({"name": "Web", "newName": "WEB", "description": "renamed"})).unwrap();
+
+    let mut b = Session::new();
+    b.attach_pdf_preset_store(dir.clone());
+    assert_eq!(saved_names(&mut b), ["Web"]);
+    assert_eq!(b.prefs.pdf_presets[0].description, "for screens");
+}
+
 #[test]
 fn a_folder_that_does_not_exist_yet_takes_the_presets_the_preferences_held() {
     let dir = fresh_dir("seed").join("PDF Presets");