font-store: bound removal cleanup and cover portable failure retries

crates/font-store/README.md

@@ -141,10 +141,15 @@ let installed = store.installed_language_packs();
 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.
+Committed removals record their backup IDs in the same atomic record update. Each locked store
+operation attempts cleanup of at most four recorded backups, completing finite rounds through
+the receipts within a live store so failed entries are revisited even as new receipts arrive.
+This bounds the number of
+deletion attempts, not the work to delete one backup. Recreating the store resets the cursor;
+backups and receipts remain available for later retries, and the next lock need not finish all
+pending cleanup. Failed rollback backups have no receipt and remain available for recovery.
+Receipts stay until cleanup succeeds and a later record write prunes them; their IDs 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.
 

crates/font-store/src/language_packs.rs

@@ -463,6 +463,8 @@ impl FontStore {
                 .filter(|name| Stage::valid_name(name))
                 .ok_or_else(|| io("recording a Language pack removal")(std::io::Error::other("invalid backup name")))?
                 .to_string();
+            #[cfg(test)]
+            tests::fail_io(tests::IoFailure::Rename).map_err(io(format!("removing the {locale} pack")))?;
             std::fs::rename(&dest, &backup).map_err(io(format!("removing the {locale} pack")))?;
             installed.committed_removals.push(id);
         }
@@ -495,13 +497,34 @@ impl FontStore {
             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))
+        let ids = &installed.committed_removals;
+        if ids.is_empty() {
+            return;
+        }
+        // All scheduler state is read and updated under the store lock. Complete finite
+        // rounds so appended receipts cannot keep old failures waiting behind a growing tail.
+        const ATTEMPTS: usize = 4;
+        let mut end = self.pack_removal_round_end.load(std::sync::atomic::Ordering::Relaxed).min(ids.len());
+        let mut next = self.pack_removal_cursor.load(std::sync::atomic::Ordering::Relaxed);
+        if next >= end {
+            next = 0;
+            end = ids.len();
+        }
+        for _ in 0..ATTEMPTS.min(ids.len()) {
+            if next == end {
+                next = 0;
+                end = ids.len();
+            }
+            let id = &ids[next];
+            next += 1;
+            if let Err(error) = remove_committed_backup(&parent.join(id))
                 && error.kind() != std::io::ErrorKind::NotFound
             {
                 log::warn!("cannot clean up committed Language pack removal {id}: {error}");
             }
         }
+        self.pack_removal_cursor.store(next, std::sync::atomic::Ordering::Relaxed);
+        self.pack_removal_round_end.store(end, std::sync::atomic::Ordering::Relaxed);
     }
 
     /// The record of installed packs. One that can't be read is copied aside as
@@ -563,6 +586,8 @@ impl FontStore {
     }
 
     fn write_installed(&self, installed: &Installed) -> Result<(), Error> {
+        #[cfg(test)]
+        tests::fail_io(tests::IoFailure::RecordWrite).map_err(io("saving the Language pack list"))?;
         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.
@@ -581,6 +606,12 @@ impl FontStore {
     }
 }
 
+fn remove_committed_backup(path: &Path) -> std::io::Result<()> {
+    #[cfg(test)]
+    tests::before_cleanup(path)?;
+    std::fs::remove_dir_all(path)
+}
+
 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) {

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

@@ -1,3 +1,4 @@
+use std::cell::{Cell, RefCell};
 use std::collections::HashMap;
 use std::io::{Read, Write};
 use std::net::{TcpListener, TcpStream};
@@ -10,6 +11,49 @@ use crate::store::Caller;
 
 const RELEASE: &str = "https://github.com/storytold/craft-i18n/releases/download/language-packs-1/";
 
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub(super) enum IoFailure {
+    Rename,
+    RecordWrite,
+    Cleanup,
+}
+
+thread_local! {
+    static IO_FAILURE: Cell<Option<IoFailure>> = const { Cell::new(None) };
+    static CLEANUP_ATTEMPTS: RefCell<Vec<PathBuf>> = const { RefCell::new(Vec::new()) };
+}
+
+struct InjectFailure(Option<IoFailure>);
+
+impl InjectFailure {
+    fn new(failure: IoFailure) -> Self {
+        Self(IO_FAILURE.replace(Some(failure)))
+    }
+}
+
+impl Drop for InjectFailure {
+    fn drop(&mut self) {
+        IO_FAILURE.set(self.0);
+    }
+}
+
+pub(super) fn fail_io(failure: IoFailure) -> std::io::Result<()> {
+    if IO_FAILURE.get() == Some(failure) {
+        Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, format!("injected {failure:?} failure")))
+    } else {
+        Ok(())
+    }
+}
+
+pub(super) fn before_cleanup(path: &Path) -> std::io::Result<()> {
+    CLEANUP_ATTEMPTS.with_borrow_mut(|attempts| attempts.push(path.to_path_buf()));
+    fail_io(IoFailure::Cleanup)
+}
+
+fn take_cleanup_attempts() -> Vec<PathBuf> {
+    CLEANUP_ATTEMPTS.with_borrow_mut(std::mem::take)
+}
+
 /// A local HTTP server answering 200 with the body set for a path, else 404.
 struct Server {
     base: String,
@@ -206,6 +250,27 @@ fn a_failed_pack_removal_preserves_the_files_and_installed_record() {
     }
 }
 
+#[test]
+fn removal_io_failures_preserve_the_pack_on_every_platform() {
+    for failure in [IoFailure::Rename, IoFailure::RecordWrite] {
+        let (store, _server, pin, _) = setup(&format!("remove-injected-{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();
+        let injected = InjectFailure::new(failure);
+        let result = store.remove_language_pack("nl");
+        drop(injected);
+        assert!(matches!(result, Err(Error::Io { .. })), "{failure:?}: {result:?}");
+        assert_eq!(std::fs::read(&dictionary).unwrap(), before_dictionary, "{failure:?}");
+        assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before_record, "{failure:?}");
+        assert_eq!(store.installed_language_packs(), vec![got]);
+        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");
@@ -258,7 +323,18 @@ fn committed_removal_cleanup_survives_interruption_and_does_not_reuse_a_recorded
     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 before_dictionary = std::fs::read(backup.join("spelling.dic")).unwrap();
+    let before_record = std::fs::read(store.dir().join(INSTALLED)).unwrap();
+    {
+        let _injected = InjectFailure::new(IoFailure::Cleanup);
+        let _lock = store.lock().unwrap();
+        assert_eq!(std::fs::read(backup.join("spelling.dic")).unwrap(), before_dictionary);
+        assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before_record);
+        assert_eq!(store.read_record().unwrap().committed_removals.as_slice(), std::slice::from_ref(&id));
+        store.write_installed(&installed).unwrap();
+        assert_eq!(store.read_record().unwrap().committed_removals.as_slice(), std::slice::from_ref(&id));
+        assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before_record);
+    }
     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();
@@ -276,6 +352,81 @@ fn committed_removal_cleanup_survives_interruption_and_does_not_reuse_a_recorded
     std::fs::remove_dir_all(store.dir()).unwrap();
 }
 
+#[test]
+fn committed_cleanup_attempts_are_bounded_and_rotate_past_failures() {
+    let (store, _server, _, _) = setup("remove-bounded-cleanup");
+    let parent = store.language_packs_dir().join(".removing");
+    let ids: Vec<_> = (0..9).map(|n| format!("0-{n}")).collect();
+    for id in &ids {
+        std::fs::create_dir_all(parent.join(id)).unwrap();
+        std::fs::write(parent.join(id).join("keep"), id).unwrap();
+    }
+    let mut installed = Installed { committed_removals: ids.clone(), ..Installed::default() };
+    store.write_installed(&installed).unwrap();
+    let before = std::fs::read(store.dir().join(INSTALLED)).unwrap();
+    take_cleanup_attempts();
+    {
+        let _injected = InjectFailure::new(IoFailure::Cleanup);
+        for expected in [&ids[..4], &ids[4..8], &ids[8..9]] {
+            drop(store.lock().unwrap());
+            let attempts = take_cleanup_attempts();
+            assert_eq!(attempts.len(), 4, "cleanup work is bounded even when every deletion fails");
+            for id in expected {
+                assert!(attempts.contains(&parent.join(id)), "receipt {id} gets a turn despite failed earlier receipts");
+            }
+        }
+        let reopened = FontStore::new(store.dir().to_path_buf(), Caller::User);
+        drop(reopened.lock().unwrap());
+        assert_eq!(std::fs::read(store.dir().join(INSTALLED)).unwrap(), before, "recreating the store never loses receipts");
+        for id in &ids {
+            assert_eq!(std::fs::read(parent.join(id).join("keep")).unwrap(), id.as_bytes());
+        }
+        // Adding one failed receipt per lock must not strand the oldest entries at a moving
+        // tail. An ever-increasing counter modulo the growing length eventually does that.
+        take_cleanup_attempts();
+        for arrivals in [1, 4] {
+            let store = FontStore::new(store.dir().to_path_buf(), Caller::User);
+            let mut revisited_oldest = false;
+            for n in 0..64 {
+                for offset in 0..arrivals {
+                    let id = format!("{arrivals}-{}", n * arrivals + offset);
+                    std::fs::create_dir(parent.join(&id)).unwrap();
+                    std::fs::write(parent.join(&id).join("keep"), &id).unwrap();
+                    installed.committed_removals.push(id);
+                }
+                store.write_installed(&installed).unwrap();
+                drop(store.lock().unwrap());
+                let attempts = take_cleanup_attempts();
+                assert_eq!(attempts.len(), 4);
+                if n >= 40 && attempts.contains(&parent.join(&ids[0])) {
+                    revisited_oldest = true;
+                }
+            }
+            assert!(revisited_oldest, "old receipts keep getting turns with {arrivals} new failures per lock");
+        }
+        // A later record write can prune many already-deleted backups between rounds.
+        for id in &installed.committed_removals[..installed.committed_removals.len() - 2] {
+            std::fs::remove_dir_all(parent.join(id)).unwrap();
+        }
+        store.write_installed(&installed).unwrap();
+        installed = store.read_record().unwrap();
+        assert_eq!(installed.committed_removals.len(), 2);
+        drop(store.lock().unwrap());
+        let attempts = take_cleanup_attempts();
+        assert_eq!(attempts.len(), 2);
+        for id in &installed.committed_removals {
+            assert!(attempts.contains(&parent.join(id)), "shrinking the list does not skip retained receipts");
+        }
+    }
+    for _ in 0..installed.committed_removals.len() {
+        drop(store.lock().unwrap());
+    }
+    assert!(std::fs::read_dir(parent).unwrap().next().is_none());
+    store.write_installed(&installed).unwrap();
+    assert!(store.read_record().unwrap().committed_removals.is_empty());
+    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");

crates/font-store/src/store.rs

@@ -17,7 +17,7 @@ use std::fs::File;
 use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
-use std::sync::atomic::{AtomicU64, Ordering};
+use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::time::{Duration, SystemTime};
 
 use craft_font_catalog::{Catalog, FontEnv, font_store_dir};
@@ -218,6 +218,8 @@ pub struct FontStore {
     caller: Caller,
     client: Client,
     catalog: Option<Arc<Catalog>>,
+    pub(crate) pack_removal_cursor: AtomicUsize,
+    pub(crate) pack_removal_round_end: AtomicUsize,
 }
 
 impl std::fmt::Debug for FontStore {
@@ -240,7 +242,14 @@ impl FontStore {
 
     /// A store in `dir`.
     pub fn new(dir: PathBuf, caller: Caller) -> Self {
-        Self { dir, caller, client: Client::default(), catalog: None }
+        Self {
+            dir,
+            caller,
+            client: Client::default(),
+            catalog: None,
+            pack_removal_cursor: AtomicUsize::new(0),
+            pack_removal_round_end: AtomicUsize::new(0),
+        }
     }
 
     /// Uses `client` for downloads (other timeouts).