diff --git a/crates/font-store/README.md b/crates/font-store/README.md index 7751eefd..c53f198c 100644 --- a/crates/font-store/README.md +++ b/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/` (`[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. diff --git a/crates/font-store/src/language_packs.rs b/crates/font-store/src/language_packs.rs index b8cc3d03..dfdb67af 100644 --- a/crates/font-store/src/language_packs.rs +++ b/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) { diff --git a/crates/font-store/src/language_packs/tests.rs b/crates/font-store/src/language_packs/tests.rs index 1c03da67..10ff3c7f 100644 --- a/crates/font-store/src/language_packs/tests.rs +++ b/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> = const { Cell::new(None) }; + static CLEANUP_ATTEMPTS: RefCell> = const { RefCell::new(Vec::new()) }; +} + +struct InjectFailure(Option); + +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 { + 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"); diff --git a/crates/font-store/src/store.rs b/crates/font-store/src/store.rs index 4e9506bb..2f5aeccb 100644 --- a/crates/font-store/src/store.rs +++ b/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>, + 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).