From 8e6e369793fa7dff688bffa0602eb0779a80dea3 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Tue, 3 Nov 2026 12:13:04 -0800 Subject: [PATCH] font-store: verify removal receipts through the recovery handle --- crates/font-store/src/language_packs.rs | 27 ++++++++++--- crates/font-store/src/language_packs/tests.rs | 40 +++++++++++++++++-- 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/crates/font-store/src/language_packs.rs b/crates/font-store/src/language_packs.rs index fccf5df24..1e3fbd882 100644 --- a/crates/font-store/src/language_packs.rs +++ b/crates/font-store/src/language_packs.rs @@ -476,7 +476,11 @@ impl FontStore { installed.committed_removals.push(id); } installed.packs.retain(|p| p.locale != locale); - finish_pack_removal(stage, &backup, &dest, has_folder, self.write_installed(&installed)) + let saved = stage + .recovery_root() + .map_err(io("using the Language pack recovery stage")) + .and_then(|root| self.write_installed_with_recovery(&installed, Some(root))); + finish_pack_removal(stage, &backup, &dest, has_folder, saved) } /// Retain the move, rollback, and cleanup capabilities; preserve the stage if opening fails. @@ -609,19 +613,22 @@ impl FontStore { Installed { version: 1, packs, committed_removals: Vec::new() } } + /// Prune receipts only through an available recovery capability; otherwise retain them. fn write_installed(&self, installed: &Installed) -> Result<(), Error> { + let recovery = if installed.committed_removals.is_empty() { None } else { PackRemovalRoot::open(self.dir()).ok() }; + self.write_installed_with_recovery(installed, recovery.as_ref()) + } + + /// Commit removal receipts against the same recovery directory used for the pack move. + fn write_installed_with_recovery(&self, installed: &Installed, recovery: Option<&PackRemovalRoot>) -> 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. 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) - }) + .filter(|id| Stage::valid_name(id) && recovery.is_none_or(|root| root.retains_receipt(id))) .cloned() .collect(); let bytes = serde_json::to_vec_pretty(&Installed { version: 1, packs: installed.packs.clone(), committed_removals }) @@ -668,6 +675,14 @@ impl PackRemovalRoot { cap_primitives::fs::rename(stage, Path::new(locale), &self.packs, Path::new(locale)) } + /// Absence must be observed relative to this directory, never through its display path. + fn retains_receipt(&self, id: &str) -> bool { + !matches!( + cap_primitives::fs::stat(&self.recovery, Path::new(id), cap_primitives::fs::FollowSymlinks::No), + Err(error) if error.kind() == std::io::ErrorKind::NotFound + ) + } + /// Release the child lock before deletion while retaining every recovery ancestor. pub(crate) fn remove_stage(mut self, id: &str, display_parent: &Path) -> std::io::Result<()> { drop(self.stage.take()); diff --git a/crates/font-store/src/language_packs/tests.rs b/crates/font-store/src/language_packs/tests.rs index 458ad5735..5e468bc4b 100644 --- a/crates/font-store/src/language_packs/tests.rs +++ b/crates/font-store/src/language_packs/tests.rs @@ -24,7 +24,7 @@ thread_local! { #[cfg(unix)] static REPLACE_CLEANUP_PARENT: RefCell> = const { RefCell::new(None) }; #[cfg(unix)] - static REPLACE_MOVE_PARENT: RefCell> = const { RefCell::new(None) }; + static REPLACE_MOVE_PARENT: RefCell> = const { RefCell::new(None) }; } struct InjectFailure(Option); @@ -67,10 +67,12 @@ fn take_cleanup_attempts() -> Vec { /// Replace the recovery parent immediately before moving a pack, keeping a matching outside ID. pub(super) fn before_pack_move(stage: &Path) -> std::io::Result<()> { #[cfg(unix)] - if let Some(outside) = REPLACE_MOVE_PARENT.take() { + if let Some((outside, matching_id)) = REPLACE_MOVE_PARENT.take() { let parent = stage.parent().ok_or_else(|| std::io::Error::other("missing recovery parent"))?; let id = stage.file_name().ok_or_else(|| std::io::Error::other("missing recovery ID"))?; - std::fs::create_dir(outside.join(id))?; + if matching_id { + std::fs::create_dir(outside.join(id))?; + } std::fs::rename(parent, parent.with_extension("original"))?; std::os::unix::fs::symlink(outside, parent)?; } @@ -547,7 +549,7 @@ fn removal_move_stays_with_its_cleanup_anchor_after_parent_replacement() { store.install_pack_from(false, &index, index.pack("nl").unwrap(), &Cancel::new(), &mut |_| {}).unwrap(); let outside = temp("remove-move-parent-race-outside"); std::fs::write(outside.join("keep"), b"untouched").unwrap(); - REPLACE_MOVE_PARENT.set(Some(outside.clone())); + REPLACE_MOVE_PARENT.set(Some((outside.clone(), true))); store.remove_language_pack("nl").unwrap(); assert_eq!(std::fs::read(outside.join("keep")).unwrap(), b"untouched"); for entry in std::fs::read_dir(&outside).unwrap().flatten() { @@ -559,6 +561,36 @@ fn removal_move_stays_with_its_cleanup_anchor_after_parent_replacement() { std::fs::remove_dir_all(store.dir()).unwrap(); } +/// An unavailable display path must not prune a receipt for a backup held by its capability. +#[cfg(unix)] +#[test] +fn committed_receipt_survives_parent_replacement_and_failed_immediate_cleanup() { + let (store, _server, pin, _) = setup("remove-receipt-parent-race"); + 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 dictionary = std::fs::read(store.language_packs_dir().join("nl/spelling.dic")).unwrap(); + let outside = temp("remove-receipt-parent-race-outside"); + REPLACE_MOVE_PARENT.set(Some((outside.clone(), false))); + let injected = InjectFailure::new(IoFailure::Cleanup); + store.remove_language_pack("nl").unwrap(); + drop(injected); + let record = store.read_record().unwrap(); + assert_eq!(record.committed_removals.len(), 1, "the committed receipt must survive a missing path under the replacement parent"); + let id = &record.committed_removals[0]; + let parent = store.language_packs_dir().join(".removing"); + let original = parent.with_extension("original"); + assert_eq!(std::fs::read(original.join(id).join("nl/spelling.dic")).unwrap(), dictionary); + assert!(std::fs::read_dir(&outside).unwrap().next().is_none()); + std::fs::remove_file(&parent).unwrap(); + std::fs::rename(original, &parent).unwrap(); + drop(store.lock().unwrap()); + assert!(!parent.join(id).exists(), "the persisted receipt drives a later anchored retry"); + store.write_installed(&record).unwrap(); + assert!(store.read_record().unwrap().committed_removals.is_empty()); + std::fs::remove_dir_all(outside).unwrap(); + std::fs::remove_dir_all(store.dir()).unwrap(); +} + /// Removing a stale record needs no recovery directory when there is no folder to move. #[test] fn removing_a_missing_pack_folder_only_updates_the_record() {