From d9f27c2ef2782f6d736bc04a73e85e680d907ebe Mon Sep 17 00:00:00 2001 From: Brad Kollmyer Date: Mon, 28 Sep 2026 13:28:52 -0700 Subject: [PATCH] fix(cache): drop cached packs that are no longer in the index Clear pack cache entries missing from the index whenever it is loaded, including backup. Packs another host has pruned no longer stay on a backup node. --- crates/core/src/backend/cache.rs | 56 ++++++++++++++- crates/core/src/index.rs | 5 ++ crates/core/src/index/binarysorted.rs | 8 +++ crates/core/src/repository.rs | 26 ++++++- crates/core/tests/integration.rs | 100 ++++++++++++++++++++++++-- 5 files changed, 189 insertions(+), 6 deletions(-) diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index 7cacd6539..a395a3dc5 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, fs::{self, File}, io::{self, Read, Seek, SeekFrom}, path::{Path, PathBuf}, @@ -448,6 +448,26 @@ impl Cache { Ok(()) } + /// Removes cached files of `tpe` whose ids are not in `present`. + /// + /// File size is not compared. Restic's `cache.Clear` does the same after + /// loading the index: packs deleted from the repository are dropped, and + /// packs that are still present are kept. + /// + /// # Errors + /// + /// * If the cache directory could not be read. + /// * If a cache file could not be removed. + pub fn remove_ids_not_in(&self, tpe: FileType, present: &HashSet) -> RusticResult<()> { + let list_cache = self.list_with_size(tpe)?; + for id in list_cache.keys() { + if !present.contains(id) { + self.remove(tpe, id)?; + } + } + Ok(()) + } + /// Reads full data of the given file. /// /// # Arguments @@ -658,3 +678,37 @@ impl Cache { Ok(()) } } + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use super::*; + + fn new_cache() -> (tempfile::TempDir, Cache) { + let dir = tempfile::tempdir().unwrap(); + let cache = Cache::new(RepositoryId::default(), Some(dir.path().to_path_buf())).unwrap(); + (dir, cache) + } + + #[test] + fn remove_ids_not_in_drops_only_absent_packs() { + let (_dir, cache) = new_cache(); + let keep = Id::random(); + let drop_a = Id::random(); + let drop_b = Id::random(); + for id in [&keep, &drop_a, &drop_b] { + cache + .write_bytes(FileType::Pack, id, &vec![1_u8; 8].into()) + .unwrap(); + } + + let mut present = HashSet::new(); + assert!(present.insert(keep)); + cache.remove_ids_not_in(FileType::Pack, &present).unwrap(); + + assert!(cache.read_full(FileType::Pack, &keep).unwrap().is_some()); + assert!(cache.read_full(FileType::Pack, &drop_a).unwrap().is_none()); + assert!(cache.read_full(FileType::Pack, &drop_b).unwrap().is_none()); + } +} diff --git a/crates/core/src/index.rs b/crates/core/src/index.rs index 95f981c89..409add032 100644 --- a/crates/core/src/index.rs +++ b/crates/core/src/index.rs @@ -251,6 +251,11 @@ impl GlobalIndex { } } + /// Pack ids of both blob types still recorded by this index. + pub(crate) fn pack_ids(&self) -> impl Iterator + '_ { + self.index.pack_ids() + } + /// Create a new [`GlobalIndex`] from an [`IndexCollector`] /// /// # Arguments diff --git a/crates/core/src/index/binarysorted.rs b/crates/core/src/index/binarysorted.rs index f399da24c..bfe2f5dd0 100644 --- a/crates/core/src/index/binarysorted.rs +++ b/crates/core/src/index/binarysorted.rs @@ -82,6 +82,11 @@ impl Index { } })) } + + /// Pack ids of both blob types still recorded by this index. + pub(crate) fn pack_ids(&self) -> impl Iterator + '_ { + self.0.values().flat_map(|ty| ty.packs.iter().copied()) + } } impl IndexCollector { @@ -332,6 +337,9 @@ mod tests { fn all_index_types() -> RusticResult<()> { for it in [IndexType::OnlyTrees, IndexType::DataIds, IndexType::Full] { let index = index(it); + // DataIds and OnlyTrees still record every pack id. Cache cleanup + // depends on that, including data packs that are not cached today. + assert_eq!(index.pack_ids().count(), 3); let id = "0000000000000000000000000000000000000000000000000000000000000000".parse()?; assert!(!index.has(BlobType::Data, &id)); diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index 66a09ffe9..6aaf4c84c 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -7,6 +7,7 @@ pub use status::*; use std::{ cmp::Ordering, + collections::HashSet, io::Write, path::{Path, PathBuf}, sync::Arc, @@ -15,7 +16,7 @@ use std::{ use bytes::Bytes; use derive_setters::Setters; use jiff::SignedDuration; -use log::info; +use log::{info, warn}; use serde_with::{DisplayFromStr, serde_as}; use crate::{ @@ -64,6 +65,7 @@ use crate::{ ConfigFile, KeyId, PathList, RepoFile, RepoId, SnapshotFile, SnapshotSummary, Tree, configfile::ConfigId, keyfile::{MasterKey, find_key_in_backend}, + packfile::PackId, snapshotfile::SnapshotId, }, repository::{ @@ -1126,8 +1128,29 @@ impl Repository { Ok(self.into_indexed_with_index(index)) } + /// Drop cached packs whose ids are not in `index`. + /// + /// Runs on every index load, as restic's `prepareCache` does, so a backup + /// node drops packs another host has already pruned. + fn clear_cached_packs_not_in_index(&self, index: &GlobalIndex) { + let Some(cache) = self.cache() else { + return; + }; + let present = index + .pack_ids() + .map(PackId::into_inner) + .collect::>(); + if let Err(err) = cache.remove_ids_not_in(FileType::Pack, &present) { + warn!( + "failed to remove cached packs that are no longer in the index: {}", + err.display_log() + ); + } + } + // helper function to deduplicate code fn into_indexed_with_index(self, index: GlobalIndex) -> Repository { + self.clear_cached_packs_not_in_index(&index); let status = IndexedFullStatus { open: self.status.into_open_status(), index, @@ -1193,6 +1216,7 @@ impl Repository { // helper function to deduplicate code fn into_indexed_ids_with_index(self, index: GlobalIndex) -> Repository { + self.clear_cached_packs_not_in_index(&index); let status = IndexedIdsStatus { open: self.status.into_open_status(), index, diff --git a/crates/core/tests/integration.rs b/crates/core/tests/integration.rs index 43840ec7b..fd667f16b 100644 --- a/crates/core/tests/integration.rs +++ b/crates/core/tests/integration.rs @@ -42,7 +42,7 @@ mod integration { use super::*; } -use std::{env, fs::File, path::Path, sync::Arc}; +use std::{env, fs, fs::File, path::Path, sync::Arc}; use anyhow::Result; use flate2::read::GzDecoder; @@ -59,9 +59,10 @@ use tempfile::{TempDir, tempdir}; // use simplelog::{Config, SimpleLogger}; use rustic_core::{ - CommandInput, ConfigOptions, CredentialOptions, Credentials, IndexedFull, IndexedFullStatus, - KeyOptions, OpenStatus, PathList, Repository, RepositoryBackends, RepositoryOptions, - repofile::MasterKey, + BackupOptions, CommandInput, ConfigOptions, CredentialOptions, Credentials, IndexedFull, + IndexedFullStatus, KeyOptions, OpenStatus, PathList, Repository, RepositoryBackends, + RepositoryOptions, + repofile::{MasterKey, SnapshotFile}, }; use rustic_testing::backend::in_memory_backend::InMemoryBackend; @@ -244,3 +245,94 @@ fn test_wrapping_in_new_type() -> Result<()> { Ok(()) } + +const STALE_PACK_IDS_LOAD: &str = + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const STALE_PACK_FULL_LOAD: &str = + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + +/// Names of cached pack files. The sweep only considers 64-character hex names. +fn cached_pack_names(repo_cache: &Path) -> Result> { + let mut names = Vec::new(); + let data = repo_cache.join("data"); + if !data.is_dir() { + return Ok(names); + } + for prefix in fs::read_dir(data)? { + let prefix = prefix?.path(); + if !prefix.is_dir() { + continue; + } + for file in fs::read_dir(prefix)? { + let file = file?; + if !file.file_type()?.is_file() { + continue; + } + let name = file.file_name(); + let name = name.to_string_lossy(); + if name.len() == 64 { + names.push(name.into_owned()); + } + } + } + names.sort(); + Ok(names) +} + +fn plant_cached_pack(repo_cache: &Path, id_hex: &str) -> Result<()> { + let path = repo_cache.join("data").join(&id_hex[..2]).join(id_hex); + fs::create_dir_all(path.parent().expect("pack path has a parent"))?; + fs::write(path, b"not-a-pack")?; + Ok(()) +} + +/// Loading the index drops cached packs that are no longer in it. +/// +/// `to_indexed_ids` is the backup path. `to_indexed` is the full index path. +#[test] +fn index_load_drops_cached_packs_missing_from_the_index() -> Result<()> { + let cache_dir = tempdir()?; + let source = tempdir()?; + fs::write(source.path().join("file.txt"), b"hello")?; + + let backends = RepositoryBackends::new(Arc::new(InMemoryBackend::new()), None); + let options = RepositoryOptions::default().cache_dir(cache_dir.path().to_path_buf()); + let repo = Repository::new(&options, &backends)?.init( + &Credentials::Masterkey(MasterKey::new()), + &KeyOptions::default(), + &ConfigOptions::default(), + )?; + let repo_cache = cache_dir.path().join(repo.config().id.to_hex().as_str()); + + let repo = repo.to_indexed_ids()?; + let _snapshot = repo.backup( + &BackupOptions::default(), + &PathList::from_iter(Some(source.path().to_path_buf())), + SnapshotFile::default(), + )?; + + let real_packs = cached_pack_names(&repo_cache)?; + assert!( + !real_packs.is_empty(), + "backup should cache the tree pack it writes" + ); + + plant_cached_pack(&repo_cache, STALE_PACK_IDS_LOAD)?; + let with_stale = cached_pack_names(&repo_cache)?; + assert!(with_stale.iter().any(|name| name == STALE_PACK_IDS_LOAD)); + + let repo = repo.to_indexed_ids()?; + assert_eq!(cached_pack_names(&repo_cache)?, real_packs); + + plant_cached_pack(&repo_cache, STALE_PACK_FULL_LOAD)?; + assert!( + cached_pack_names(&repo_cache)? + .iter() + .any(|name| name == STALE_PACK_FULL_LOAD) + ); + + let _repo = repo.to_indexed()?; + assert_eq!(cached_pack_names(&repo_cache)?, real_packs); + + Ok(()) +}