font-store: preserve Language packs when removal cannot commit

crates/font-store/README.md

@@ -138,6 +138,14 @@ let installed = store.installed_language_packs();
 | `installed_language_packs()` | The recorded packs whose folder is there. |
 | `language_packs_dir()` | `<store>/language-packs`, for `craft_proof::packs::set_packs_dir`. |
 
+Removal restores the original folder if the installed-record update fails. If restoration also
+fails, the error names a preserved backup under `language-packs/.removing/`. That recovery
+folder is outside the ordinary download staging sweep; cleanup does not delete the backup.
+Committed removals record their backup IDs in the same atomic record update. The next locked
+store operation retries cleanup of those backups; failed rollback backups have no receipt and
+remain available for recovery. Receipts stay until cleanup succeeds and cannot be reused for a
+new removal while they remain recorded.
+
 **Unpacking is hostile-input safe:** pure-Rust zstd (`ruzstd`, MIT; every frame is read, and nothing but tar padding may follow the archive) and `tar` (MIT OR Apache-2.0); regular files and the `LICENCES` folder only (no links or devices). Only the contract's names are allowed: `pack.toml`, `spelling.aff`, `spelling.dic`, `hyphenation.pat` at the root and `LICENCES/<name>` (`[A-Za-z0-9._-]`, no leading or trailing dot, no Windows device names such as `CON` or `nul.txt`). Each file at most 64 MiB, 256 MiB and 256 entries per pack. `pack.toml` is required and must name the entry's locale; the files the index promises (`spelling`, `hyphenation`) must be there. Index entries need a lowercase BCP 47 locale (listed once, not a Windows device name), a sha256 and an `https://` URL; names, licences, versions, classes and the release lose control and bidi/format characters before they are stored or listed; an optional `source` must be an `https://` URL. At most 4096 zstd frames per archive. An unreadable `language-packs.json` is copied to `language-packs.json.bad` and rebuilt from the pack folders (each `pack.toml`) atomically in place, as is a missing one when pack folders are there, so no installed pack drops out of the list. Only an install or removal (under the store lock) rewrites it; listing reads the folders meanwhile and writes nothing.
 
 **Developer override (testing only — bypasses the pin):** `CRAFT_LANGUAGE_PACK_INDEX=<url or path>` replaces the pinned index URL with an `http://` URL on a loopback host (`127.0.0.1`, `localhost`, `[::1]`), an `https://` URL or a local file path (`file://` URLs are not taken), and **skips the index's sha256 pin**. Archives are still checked against the sha256 and size in that index, and may then also be loopback `http://` URLs or local paths. Off unless the variable is set; it works in release builds too (for end-to-end tests of a real build), a warning is logged whenever it is used, and `language_pack_index_overridden() -> Option<String>` returns the value so apps can show that the pin is bypassed. Never set it for users.

crates/font-store/src/language_packs.rs

@@ -125,6 +125,9 @@ pub struct InstalledLanguagePack {
 struct Installed {
     version: u32,
     packs: Vec<InstalledLanguagePack>,
+    /// Committed backup IDs, recorded atomically with removing their pack entries.
+    #[serde(default, skip_serializing_if = "Vec::is_empty")]
+    committed_removals: Vec<String>,
 }
 
 impl PackIndex {
@@ -442,20 +445,63 @@ impl FontStore {
         if !is_locale(locale) {
             return Err(Error::NotInstalled(locale.chars().take(40).collect()));
         }
-        let stage = Stage::new(self.dir())?;
         let _lock = self.lock()?;
         let dest = self.language_packs_dir().join(locale);
         let mut installed = self.read_installed()?;
         let known = installed.packs.iter().any(|p| p.locale == locale);
-        if !known && std::fs::symlink_metadata(&dest).is_err() {
+        let has_folder = std::fs::symlink_metadata(&dest).is_ok();
+        if !known && !has_folder {
             return Err(Error::NotInstalled(locale.to_string()));
         }
-        if std::fs::symlink_metadata(&dest).is_ok() {
-            std::fs::rename(&dest, stage.dir.join(locale)).map_err(io(format!("removing the {locale} pack")))?;
+        let stage = self.language_pack_removal_stage(&installed.committed_removals)?;
+        let backup = stage.dir.join(locale);
+        if has_folder {
+            let id = stage
+                .dir
+                .file_name()
+                .and_then(|name| name.to_str())
+                .filter(|name| Stage::valid_name(name))
+                .ok_or_else(|| io("recording a Language pack removal")(std::io::Error::other("invalid backup name")))?
+                .to_string();
+            std::fs::rename(&dest, &backup).map_err(io(format!("removing the {locale} pack")))?;
+            installed.committed_removals.push(id);
         }
         installed.packs.retain(|p| p.locale != locale);
-        self.write_installed(&installed)
-        // `stage` deletes the old folder when dropped.
+        finish_pack_removal(stage, &backup, &dest, has_folder, self.write_installed(&installed))
+    }
+
+    fn language_pack_removal_stage(&self, excluded: &[String]) -> Result<Stage, Error> {
+        // A failed rollback or interrupted removal must not lose its backup to the download
+        // sweep. Locale scans ignore this hidden directory.
+        Stage::in_dir_excluding(&self.pack_removal_dir()?, excluded)
+    }
+
+    fn pack_removal_dir(&self) -> Result<PathBuf, Error> {
+        let parent = self.language_packs_dir().join(".removing");
+        match std::fs::symlink_metadata(&parent) {
+            Ok(metadata) if !metadata.is_dir() || metadata.file_type().is_symlink() => {
+                return Err(io("using the Language pack recovery folder")(std::io::Error::other("not a plain directory")));
+            }
+            Err(error) if error.kind() != std::io::ErrorKind::NotFound => return Err(io("reading the Language pack recovery folder")(error)),
+            _ => {}
+        }
+        Ok(parent)
+    }
+
+    /// Called only after acquiring the store lock: a snapshot must not delete a newly reused ID.
+    pub(crate) fn sweep_committed_pack_removals(&self) {
+        let Some(installed) = self.read_record() else { return };
+        if installed.version != 1 {
+            return;
+        }
+        let Ok(parent) = self.pack_removal_dir() else { return };
+        for id in installed.committed_removals {
+            if let Err(error) = std::fs::remove_dir_all(parent.join(&id))
+                && error.kind() != std::io::ErrorKind::NotFound
+            {
+                log::warn!("cannot clean up committed Language pack removal {id}: {error}");
+            }
+        }
     }
 
     /// The record of installed packs. One that can't be read is copied aside as
@@ -482,7 +528,8 @@ impl FontStore {
 
     /// The record as written, if there is one that can be read.
     fn read_record(&self) -> Option<Installed> {
-        read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice::<Installed>(&b).ok())
+        let installed: Installed = read_limited(&self.dir().join(INSTALLED), MAX_PACK_INDEX_BYTES).and_then(|b| serde_json::from_slice(&b).ok())?;
+        installed.committed_removals.iter().all(|name| Stage::valid_name(name)).then_some(installed)
     }
 
     /// The installed packs as their folders say: each folder named by a locale, with the name
@@ -512,16 +559,42 @@ impl FontStore {
             })
             .collect();
         packs.sort_by(|a, b| a.locale.cmp(&b.locale));
-        Installed { version: 1, packs }
+        Installed { version: 1, packs, committed_removals: Vec::new() }
     }
 
     fn write_installed(&self, installed: &Installed) -> Result<(), Error> {
-        let bytes = serde_json::to_vec_pretty(&Installed { version: 1, packs: installed.packs.clone() })
+        let parent = self.language_packs_dir().join(".removing");
+        // Keep receipts until their backup has actually disappeared. A future locked sweep
+        // retries failed cleanup, and stage creation excludes IDs whose receipts remain.
+        let committed_removals = installed
+            .committed_removals
+            .iter()
+            .filter(|id| {
+                Stage::valid_name(id)
+                    && !matches!(std::fs::symlink_metadata(parent.join(id)), Err(error) if error.kind() == std::io::ErrorKind::NotFound)
+            })
+            .cloned()
+            .collect();
+        let bytes = serde_json::to_vec_pretty(&Installed { version: 1, packs: installed.packs.clone(), committed_removals })
             .map_err(|e| io("saving the Language pack list")(std::io::Error::other(e)))?;
         self.write_file(INSTALLED, &bytes)
     }
 }
 
+fn finish_pack_removal(stage: Stage, backup: &Path, dest: &Path, moved: bool, saved: Result<(), Error>) -> Result<(), Error> {
+    if let Err(error) = saved {
+        if moved && let Err(restore) = std::fs::rename(backup, dest) {
+            stage.keep_for_recovery();
+            return Err(io(format!("saving the Language pack list failed ({error}); the original pack is preserved at {}", backup.display()))(
+                restore,
+            ));
+        }
+        return Err(error);
+    }
+    // The record committed the removal; dropping the stage cleans up its old files.
+    Ok(())
+}
+
 /// The zstd frames of `archive` one after another, as one stream (`zstd` writes one frame, but
 /// a concatenation of frames is a valid `.zst` too, and must not stop after the first).
 struct Frames<'a> {

crates/font-store/src/language_packs/tests.rs

@@ -171,6 +171,148 @@ fn install_list_replace_and_remove_a_pack() {
     assert!(matches!(store.remove_language_pack("../x"), Err(Error::NotInstalled(_))));
 }
 
+#[cfg(unix)]
+#[test]
+fn a_failed_pack_removal_preserves_the_files_and_installed_record() {
+    use std::os::unix::fs::PermissionsExt;
+    for record_failure in [true, false] {
+        let (store, _server, pin, _) = setup(if record_failure { "remove-record-failure" } else { "remove-rename-failure" });
+        let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap();
+        let got = store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap();
+        let dictionary = store.language_packs_dir().join("nl/spelling.dic");
+        let before_dictionary = std::fs::read(&dictionary).unwrap();
+        let before_record = std::fs::read(store.dir().join(INSTALLED)).unwrap();
+        drop(store.language_pack_removal_stage(&[]).unwrap());
+        // The staging and pack subdirectories remain writable when only the store root
+        // rejects the atomic record replacement. The other case rejects the pack rename.
+        let protected = if record_failure { store.dir().to_path_buf() } else { store.language_packs_dir() };
+        let permissions = std::fs::metadata(&protected).unwrap().permissions();
+        std::fs::set_permissions(&protected, std::fs::Permissions::from_mode(0o555)).unwrap();
+        let probe = protected.join("write-probe");
+        if std::fs::File::create(&probe).is_ok() {
+            std::fs::set_permissions(&protected, permissions).unwrap();
+            std::fs::remove_dir_all(store.dir()).unwrap();
+            continue; // root ignores permissions
+        }
+        let result = store.remove_language_pack("nl");
+        std::fs::set_permissions(&protected, permissions).unwrap();
+        assert!(matches!(result, Err(Error::Io { .. })), "{result:?}");
+        assert_eq!(std::fs::read(&dictionary).unwrap(), before_dictionary, "record failure: {record_failure}");
+        assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before_record);
+        assert_eq!(store.installed_language_packs(), vec![got]);
+        assert!(std::fs::read_dir(store.dir().join(".staging")).unwrap().next().is_none());
+        assert!(std::fs::read_dir(store.language_packs_dir().join(".removing")).unwrap().next().is_none());
+        std::fs::remove_dir_all(store.dir()).unwrap();
+    }
+}
+
+#[test]
+fn a_failed_removal_rollback_keeps_a_recoverable_pack_outside_download_cleanup() {
+    let (store, _server, pin, _) = setup("remove-rollback-failure");
+    let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap();
+    let got = store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap();
+    let before_record = std::fs::read(store.dir().join(INSTALLED)).unwrap();
+    let dest = store.language_packs_dir().join("nl");
+    let before_dictionary = std::fs::read(dest.join("spelling.dic")).unwrap();
+    let stage = store.language_pack_removal_stage(&[]).unwrap();
+    let stage_dir = stage.dir.clone();
+    let backup = stage_dir.join("nl");
+    std::fs::rename(&dest, &backup).unwrap();
+    // A replacement appearing at the original path makes restoration fail. Keep it too.
+    std::fs::write(&dest, b"replacement").unwrap();
+    let failed = io("saving the Language pack list")(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "injected record failure"));
+    let result = finish_pack_removal(stage, &backup, &dest, true, Err(failed));
+    assert!(matches!(result, Err(Error::Io { .. })));
+    assert!(result.unwrap_err().to_string().contains(&backup.display().to_string()));
+    #[cfg(unix)]
+    {
+        let old = std::time::SystemTime::now() - Duration::from_secs(7200);
+        std::fs::File::open(&stage_dir).unwrap().set_times(std::fs::FileTimes::new().set_modified(old)).unwrap();
+    }
+    store.sweep_staging();
+    drop(store.lock().unwrap());
+    assert_eq!(std::fs::read(backup.join("spelling.dic")).unwrap(), before_dictionary);
+    assert_eq!(std::fs::read(&dest).unwrap(), b"replacement");
+    assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before_record);
+    std::fs::remove_file(&dest).unwrap();
+    std::fs::rename(&backup, &dest).unwrap();
+    assert_eq!(store.installed_language_packs(), vec![got]);
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
+#[test]
+fn committed_removal_cleanup_survives_interruption_and_does_not_reuse_a_recorded_id() {
+    let (store, _server, pin, _) = setup("remove-committed-cleanup");
+    let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap();
+    store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap();
+    let mut installed = store.read_installed().unwrap();
+    let stage = store.language_pack_removal_stage(&installed.committed_removals).unwrap();
+    let id = stage.dir.file_name().unwrap().to_str().unwrap().to_string();
+    let backup = stage.dir.join("nl");
+    std::fs::rename(store.language_packs_dir().join("nl"), &backup).unwrap();
+    installed.packs.clear();
+    installed.committed_removals.push(id.clone());
+    store.write_installed(&installed).unwrap();
+    stage.keep_for_recovery(); // Simulate exiting after the atomic record commit, before Drop.
+    assert!(backup.join("spelling.dic").is_file());
+    let _lock = store.lock().unwrap();
+    assert!(!backup.exists(), "a committed leftover is cleaned at the next locked store operation");
+    let installed = store.read_installed().unwrap();
+    assert_eq!(installed.committed_removals.as_slice(), std::slice::from_ref(&id));
+    // A restarted PID/counter could propose the old name after cleanup removed its directory.
+    // Its still-recorded receipt must exclude it until the next record update prunes it.
+    let parent = store.pack_removal_dir().unwrap();
+    let fresh = Stage::create_in(&parent, &installed.committed_removals, [id.clone(), "0-0".into()].into_iter()).unwrap();
+    assert_eq!(fresh.dir.file_name().unwrap(), "0-0");
+    drop(fresh);
+    store.write_installed(&installed).unwrap();
+    assert!(store.read_record().unwrap().committed_removals.is_empty());
+    assert!(store.installed_language_packs().is_empty());
+    drop(_lock);
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
+#[test]
+fn removal_receipts_cannot_name_paths_outside_the_recovery_folder() {
+    let (store, _server, pin, _) = setup("remove-invalid-receipts");
+    let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap();
+    store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap();
+    let outside = temp("remove-outside");
+    std::fs::write(outside.join("keep"), b"untouched").unwrap();
+    let before = std::fs::read(store.dir().join(INSTALLED)).unwrap();
+    for bad in ["../outside".to_string(), outside.display().to_string(), "1-2/../../outside".to_string()] {
+        let mut record: serde_json::Value = serde_json::from_slice(&before).unwrap();
+        record["committed_removals"] = serde_json::json!([bad]);
+        std::fs::write(store.dir().join(INSTALLED), serde_json::to_vec(&record).unwrap()).unwrap();
+        drop(store.lock().unwrap());
+        assert_eq!(std::fs::read(outside.join("keep")).unwrap(), b"untouched");
+    }
+    std::fs::remove_dir_all(outside).unwrap();
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
+#[cfg(unix)]
+#[test]
+fn recovery_cleanup_does_not_follow_a_replaced_parent_symlink() {
+    let (store, _server, pin, _) = setup("remove-symlink-parent");
+    let index = store.fetch_pack_index_from(&IndexSource::Pinned(pin), &Cancel::new()).unwrap();
+    store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap();
+    let outside = temp("remove-symlink-outside");
+    std::fs::create_dir(outside.join("11-12")).unwrap();
+    std::fs::write(outside.join("11-12/keep"), b"untouched").unwrap();
+    std::os::unix::fs::symlink(&outside, store.language_packs_dir().join(".removing")).unwrap();
+    let mut record = store.read_installed().unwrap();
+    record.committed_removals.push("11-12".into());
+    store.write_installed(&record).unwrap();
+    drop(store.lock().unwrap());
+    assert!(matches!(store.remove_language_pack("nl"), Err(Error::Io { .. })));
+    assert_eq!(std::fs::read(outside.join("11-12/keep")).unwrap(), b"untouched");
+    assert!(store.language_packs_dir().join("nl/spelling.dic").is_file());
+    std::fs::remove_file(store.language_packs_dir().join(".removing")).unwrap();
+    std::fs::remove_dir_all(outside).unwrap();
+    std::fs::remove_dir_all(store.dir()).unwrap();
+}
+
 #[test]
 fn network_off_pins_and_checksums_are_enforced() {
     let (store, server, pin, archive) = setup("pins");

crates/font-store/src/store.rs

@@ -718,6 +718,7 @@ impl FontStore {
         std::fs::create_dir_all(&self.dir).map_err(io("creating the font store folder"))?;
         let file = File::options().create(true).truncate(false).write(true).open(self.dir.join(LOCK)).map_err(io("locking the font store"))?;
         file.lock().map_err(io("locking the font store"))?;
+        self.sweep_committed_pack_removals();
         Ok(file)
     }
 
@@ -831,29 +832,54 @@ fn host(url: &str) -> &str {
     url.split("://").nth(1).and_then(|r| r.split('/').next()).unwrap_or(url)
 }
 
-/// A folder of downloads under `.staging`, deleted when dropped.
+/// A temporary folder, deleted when dropped unless kept for recovery.
 pub(crate) struct Stage {
     pub(crate) dir: PathBuf,
+    remove_on_drop: bool,
 }
 
 impl Stage {
     pub(crate) fn new(store: &Path) -> Result<Self, Error> {
+        Self::in_dir(&store.join(STAGING))
+    }
+
+    pub(crate) fn in_dir(staging: &Path) -> Result<Self, Error> {
+        Self::in_dir_excluding(staging, &[])
+    }
+
+    pub(crate) fn in_dir_excluding(staging: &Path, excluded: &[String]) -> Result<Self, Error> {
         static N: AtomicU64 = AtomicU64::new(0);
-        let staging = store.join(STAGING);
-        std::fs::create_dir_all(&staging).map_err(io("creating a download folder"))?;
+        Self::create_in(staging, excluded, std::iter::repeat_with(|| format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed))))
+    }
+
+    pub(crate) fn valid_name(name: &str) -> bool {
+        name.split_once('-').is_some_and(|(pid, counter)| pid.parse::<u32>().is_ok() && counter.parse::<u64>().is_ok())
+    }
+
+    /// The candidate iterator also lets tests reproduce a restarted PID/counter sequence.
+    pub(crate) fn create_in(staging: &Path, excluded: &[String], names: impl Iterator<Item = String>) -> Result<Self, Error> {
+        std::fs::create_dir_all(staging).map_err(io("creating a temporary folder"))?;
         // A folder of that name may be another process's (PID namespaces in Flatpak or
         // containers repeat ids) or a stale one: never touched here, the next name is taken
-        // (stale ones go in `sweep_staging`).
+        // (`sweep_staging` handles ordinary download staging, not recovery folders).
         let mut last = None;
-        for _ in 0..1000 {
-            let dir = staging.join(format!("{}-{}", std::process::id(), N.fetch_add(1, Ordering::Relaxed)));
+        for name in names.take(1000) {
+            if !Self::valid_name(&name) || excluded.contains(&name) {
+                continue;
+            }
+            let dir = staging.join(name);
             match std::fs::create_dir(&dir) {
-                Ok(()) => return Ok(Self { dir }),
+                Ok(()) => return Ok(Self { dir, remove_on_drop: true }),
                 Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => last = Some(e),
-                Err(e) => return Err(io("creating a download folder")(e)),
+                Err(e) => return Err(io("creating a temporary folder")(e)),
             }
         }
-        Err(io("creating a download folder")(last.unwrap_or_else(|| std::io::Error::other("no free name"))))
+        Err(io("creating a temporary folder")(last.unwrap_or_else(|| std::io::Error::other("no free name"))))
+    }
+
+    /// The caller places recovery data outside the ordinary download staging sweep.
+    pub(crate) fn keep_for_recovery(mut self) {
+        self.remove_on_drop = false;
     }
 
     /// Writes a verified download as `<name>.part`, which no font scan picks up.
@@ -871,7 +897,9 @@ impl Stage {
 
 impl Drop for Stage {
     fn drop(&mut self) {
-        let _ = std::fs::remove_dir_all(&self.dir);
+        if self.remove_on_drop {
+            let _ = std::fs::remove_dir_all(&self.dir);
+        }
     }
 }