From c7deaf76fd39ef5713927e241d14650786c61f22 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 16 Oct 2024 13:20:48 -0500 Subject: [PATCH 01/11] Test loading cache does not overwrite files When copying from the cache back into the app, it should not replace files that already exist. This test asserts already passing behavior. --- commons/src/cache/app_cache.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index bf1434b..3b40017 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -387,6 +387,31 @@ mod tests { assert_eq!(layer_name!("cache_my_input"), layer); } + #[test] + fn test_load_does_not_clobber_files() { + let tmpdir = tempfile::tempdir().unwrap(); + let cache_path = tmpdir.path().join("cache"); + let app_path = tmpdir.path().join("app"); + fs_err::create_dir_all(&cache_path).unwrap(); + fs_err::create_dir_all(&app_path).unwrap(); + + fs_err::write(app_path.join("a.txt"), "app").unwrap(); + fs_err::write(cache_path.join("a.txt"), "cache").unwrap(); + + let store = AppCache { + path: app_path.clone(), + cache: cache_path, + limit: Byte::from_u64(512), + keep_path: KeepPath::Runtime, + cache_state: CacheState::NewEmpty, + }; + + store.load().unwrap(); + + let contents = fs_err::read_to_string(app_path.join("a.txt")).unwrap(); + assert_eq!("app", contents); + } + #[test] fn test_copying_back_to_cache() { let tmpdir = tempfile::tempdir().unwrap(); From 6f20a75d230c2c2d2651bd9198a0375fda5e43ec Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 16 Oct 2024 13:51:58 -0500 Subject: [PATCH 02/11] Add test for code coverage --- commons/src/cache/app_cache.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index 3b40017..993e404 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -387,6 +387,27 @@ mod tests { assert_eq!(layer_name!("cache_my_input"), layer); } + #[test] + fn test_layer_name_cache_state() { + let layer_name = layer_name!("name"); + let tempdir = tempfile::tempdir().unwrap(); + let path = tempdir.path(); + assert_eq!( + CacheState::NewEmpty, + layer_name_cache_state(path, &layer_name) + ); + fs_err::create_dir_all(path.join(layer_name.as_str())).unwrap(); + assert_eq!( + CacheState::ExistsEmpty, + layer_name_cache_state(path, &layer_name) + ); + fs_err::write(path.join(layer_name.as_str()).join("file"), "data").unwrap(); + assert_eq!( + CacheState::ExistsWithContents, + layer_name_cache_state(path, &layer_name) + ); + } + #[test] fn test_load_does_not_clobber_files() { let tmpdir = tempfile::tempdir().unwrap(); From 9903c58bc0cb4f9b067412750bfd656aecda3f41 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 16 Oct 2024 14:41:35 -0500 Subject: [PATCH 03/11] Group import lines --- commons/src/cache/app_cache.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index 993e404..c9ae022 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -374,11 +374,9 @@ fn is_empty_dir(path: &Path) -> bool { #[cfg(test)] mod tests { - use std::str::FromStr; - - use libcnb::data::layer_name; - use super::*; + use libcnb::data::layer_name; + use std::str::FromStr; #[test] fn test_to_layer_name() { From b15d7453ac049f76b051c58748fd299de06866b2 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 16 Oct 2024 14:49:07 -0500 Subject: [PATCH 04/11] Test mtime save/load behavior Adds tests for ensuring FIFO behavior in the cache works based on mime types. This comment https://github.com/heroku/buildpacks-ruby/issues/302#issuecomment-2415128173. Currently it works locally but fails on CI, that tells me the mtime-preserving behavior is platform dependent. --- commons/src/cache/app_cache.rs | 86 ++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index c9ae022..7825456 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -375,6 +375,7 @@ fn is_empty_dir(path: &Path) -> bool { #[cfg(test)] mod tests { use super::*; + use filetime::FileTime; use libcnb::data::layer_name; use std::str::FromStr; @@ -492,4 +493,89 @@ mod tests { assert!(store.cache.join("lol.txt").exists()); assert!(!store.path.join("lol.txt").exists()); } + + #[test] + fn mtime_preserved_keep_path_build_only() { + let mtime = FileTime::from_unix_time(1000, 0); + let tmpdir = tempfile::tempdir().unwrap(); + let filename = "totoro.txt"; + let app_path = tmpdir.path().join("app"); + let cache_path = tmpdir.path().join("cache"); + + fs_err::create_dir_all(&cache_path).unwrap(); + fs_err::create_dir_all(&app_path).unwrap(); + + let store = AppCache { + path: app_path.clone(), + cache: cache_path.clone(), + limit: Byte::from_u64(512), + keep_path: KeepPath::BuildOnly, + cache_state: CacheState::NewEmpty, + }; + + fs_err::write(app_path.join(filename), "catbus").unwrap(); + filetime::set_file_mtime(app_path.join(filename), mtime).unwrap(); + + store.save().unwrap(); + + // Cache file mtime should match app file mtime + let actual = fs_err::metadata(cache_path.join(filename)) + .map(|metadata| FileTime::from_last_modification_time(&metadata)) + .unwrap(); + assert_eq!(mtime, actual); + + // File was removed already after save + assert!(!store.path.join(filename).exists()); + + store.load().unwrap(); + + // App path mtime should match cache file mtime + let actual = fs_err::metadata(app_path.join(filename)) + .map(|metadata| FileTime::from_last_modification_time(&metadata)) + .unwrap(); + assert_eq!(mtime, actual); + } + + #[test] + fn mtime_preserved_keep_path_runtime() { + let mtime = FileTime::from_unix_time(1000, 0); + let tmpdir = tempfile::tempdir().unwrap(); + let filename = "totoro.txt"; + let app_path = tmpdir.path().join("app"); + let cache_path = tmpdir.path().join("cache"); + + fs_err::create_dir_all(&cache_path).unwrap(); + fs_err::create_dir_all(&app_path).unwrap(); + + let store = AppCache { + path: app_path.clone(), + cache: cache_path.clone(), + limit: Byte::from_u64(512), + keep_path: KeepPath::Runtime, + cache_state: CacheState::NewEmpty, + }; + + fs_err::write(app_path.join(filename), "catbus").unwrap(); + filetime::set_file_mtime(app_path.join(filename), mtime).unwrap(); + + store.save().unwrap(); + + // Cache file mtime should match app file mtime + let actual = fs_err::metadata(cache_path.join(filename)) + .map(|metadata| FileTime::from_last_modification_time(&metadata)) + .unwrap(); + assert_eq!(mtime, actual); + + // Remove app path to test loading + fs_err::remove_dir_all(&app_path).unwrap(); + assert!(!store.path.join(filename).exists()); + + store.load().unwrap(); + + // App path mtime should match cache file mtime + let actual = fs_err::metadata(app_path.join(filename)) + .map(|metadata| FileTime::from_last_modification_time(&metadata)) + .unwrap(); + assert_eq!(mtime, actual); + } } From e3781e94bf8a9ee4358ec07f893fcb6a2df9ba84 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 16 Oct 2024 16:05:02 -0500 Subject: [PATCH 05/11] Commons changelog --- commons/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/commons/CHANGELOG.md b/commons/CHANGELOG.md index 5889f3c..e2f0618 100644 --- a/commons/CHANGELOG.md +++ b/commons/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog for commons features +## 2024-08-16 + +### Fixed + +- `AppCache` will now preserve mtime of files when copying them to/from the cache (https://github.com/heroku/buildpacks-ruby/pull/336) + ## 2024-08-15 ### Changed From 1c9128007cf13499e04a75ce13ec70e1aa7249e7 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 17 Oct 2024 08:27:16 -0500 Subject: [PATCH 06/11] Copy and delete instead of moving files --- commons/src/cache/app_cache.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index 7825456..1d25978 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -306,7 +306,7 @@ fn preserve_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { /// /// - If the move command fails an `IoExtraError` will be raised. fn remove_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { - fs_extra::dir::move_dir( + fs_extra::dir::copy( &store.path, &store.cache, &CopyOptions { @@ -322,6 +322,8 @@ fn remove_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { error, })?; + fs_err::remove_dir_all(&store.path).map_err(CacheError::IoError)?; + Ok(store) } From 072b0bfefd51d0d48de4fb6b19c8202dc74750ef Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 17 Oct 2024 08:39:28 -0500 Subject: [PATCH 07/11] Manually copy mtime after copying directory contents --- commons/Cargo.toml | 1 + commons/src/cache/app_cache.rs | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/commons/Cargo.toml b/commons/Cargo.toml index c1b1fd1..579826c 100644 --- a/commons/Cargo.toml +++ b/commons/Cargo.toml @@ -32,6 +32,7 @@ sha2 = "0.10" tempfile = "3" thiserror = "1" walkdir = "2" +filetime = "0.2" [dev-dependencies] filetime = "0.2" diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index 1d25978..247148b 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -5,6 +5,7 @@ use byte_unit::{AdjustedByte, Byte, UnitType}; use fs_extra::dir::CopyOptions; use libcnb::build::BuildContext; use libcnb::data::layer::LayerName; +use std::os::unix::fs::MetadataExt; use std::path::Path; use std::path::PathBuf; @@ -164,6 +165,7 @@ impl AppCache { cache: self.cache.clone(), error, })?; + copy_mtime_r(&self.cache, &self.path)?; Ok(self) } @@ -292,6 +294,8 @@ fn preserve_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { error, })?; + copy_mtime_r(&store.path, &store.cache)?; + Ok(store) } @@ -321,12 +325,30 @@ fn remove_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { cache: store.cache.clone(), error, })?; + copy_mtime_r(&store.path, &store.cache)?; fs_err::remove_dir_all(&store.path).map_err(CacheError::IoError)?; Ok(store) } +fn copy_mtime_r(from: &Path, to_path: &Path) -> Result<(), CacheError> { + for entry in walkdir::WalkDir::new(from) + .into_iter() + .filter_map(Result::ok) + { + let mtime = entry + .metadata() + .map(|metadata| filetime::FileTime::from_last_modification_time(&metadata)) + .expect("TODO Error handling"); + + let relative = entry.path().strip_prefix(from); + let target = to_path.join(relative.expect("TODO Error handling")); + filetime::set_file_mtime(target, mtime).expect("TODO error handling"); + } + Ok(()) +} + /// Converts a path inside of an app to a valid layer name for libcnb. fn create_layer_name(app_root: &Path, path: &Path) -> Result { let name = path From b6113627ec9d862fadb79cf63ffcf7108babe5c3 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 17 Oct 2024 08:53:48 -0500 Subject: [PATCH 08/11] Proper error handling --- commons/src/cache/app_cache.rs | 30 +++++++++++++++++++++--------- commons/src/cache/error.rs | 7 +++++++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index 247148b..2ad8408 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -8,6 +8,7 @@ use libcnb::data::layer::LayerName; use std::os::unix::fs::MetadataExt; use std::path::Path; use std::path::PathBuf; +use walkdir::WalkDir; use tempfile as _; @@ -333,18 +334,29 @@ fn remove_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { } fn copy_mtime_r(from: &Path, to_path: &Path) -> Result<(), CacheError> { - for entry in walkdir::WalkDir::new(from) - .into_iter() - .filter_map(Result::ok) - { + for entry in WalkDir::new(from).into_iter().filter_map(Result::ok) { + let relative = entry + .path() + .strip_prefix(from) + .expect("Walkdir path should return path with prefix of called root"); + let mtime = entry .metadata() + .map_err(|error| std::io::Error::new(std::io::ErrorKind::Other, error)) .map(|metadata| filetime::FileTime::from_last_modification_time(&metadata)) - .expect("TODO Error handling"); - - let relative = entry.path().strip_prefix(from); - let target = to_path.join(relative.expect("TODO Error handling")); - filetime::set_file_mtime(target, mtime).expect("TODO error handling"); + .map_err(|error| CacheError::Mtime { + from: entry.path().to_path_buf(), + to_path: to_path.join(relative), + error, + })?; + + filetime::set_file_mtime(to_path.join(relative), mtime).map_err(|error| { + CacheError::Mtime { + from: entry.path().to_path_buf(), + to_path: to_path.join(relative), + error, + } + })?; } Ok(()) } diff --git a/commons/src/cache/error.rs b/commons/src/cache/error.rs index 4a5f4e7..8d18cf1 100644 --- a/commons/src/cache/error.rs +++ b/commons/src/cache/error.rs @@ -31,6 +31,13 @@ pub enum CacheError { error: fs_extra::error::Error, }, + #[error("Cannot copy mtime. From: {from} To: {to_path}\nError: {error}")] + Mtime { + from: PathBuf, + to_path: PathBuf, + error: std::io::Error, + }, + #[error("IO error: {0}")] IoError(std::io::Error), From c30c412345d0ea6d8ad07178d11df0cb873ca586 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 17 Oct 2024 08:59:29 -0500 Subject: [PATCH 09/11] Remove nearly identical function --- commons/src/cache/app_cache.rs | 43 +++++----------------------------- 1 file changed, 6 insertions(+), 37 deletions(-) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index 2ad8408..c3b4949 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -5,7 +5,6 @@ use byte_unit::{AdjustedByte, Byte, UnitType}; use fs_extra::dir::CopyOptions; use libcnb::build::BuildContext; use libcnb::data::layer::LayerName; -use std::os::unix::fs::MetadataExt; use std::path::Path; use std::path::PathBuf; use walkdir::WalkDir; @@ -128,9 +127,12 @@ impl AppCache { /// - If the files cannot be moved/coppied into the cache /// then then an error will be raised. pub fn save(&self) -> Result<&AppCache, CacheError> { + save(self)?; match self.keep_path { - KeepPath::Runtime => preserve_path_save(self)?, - KeepPath::BuildOnly => remove_path_save(self)?, + KeepPath::Runtime => {} + KeepPath::BuildOnly => { + fs_err::remove_dir_all(&self.path).map_err(CacheError::IoError)?; + } }; Ok(self) @@ -278,7 +280,7 @@ pub fn build( /// # Errors /// /// - If the copy command fails an `IoExtraError` will be raised. -fn preserve_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { +fn save(store: &AppCache) -> Result<&AppCache, CacheError> { fs_extra::dir::copy( &store.path, &store.cache, @@ -300,39 +302,6 @@ fn preserve_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { Ok(store) } -/// Move contents of application path into the cache -/// -/// This action is destructive, after execution the application path -/// will be empty. Files from the application path are considered -/// cannonical and will overwrite files with the same name in the -/// cache. -/// -/// # Errors -/// -/// - If the move command fails an `IoExtraError` will be raised. -fn remove_path_save(store: &AppCache) -> Result<&AppCache, CacheError> { - fs_extra::dir::copy( - &store.path, - &store.cache, - &CopyOptions { - overwrite: true, - copy_inside: true, // Recursive - content_only: true, // Don't copy top level directory name - ..CopyOptions::default() - }, - ) - .map_err(|error| CacheError::DestructiveMoveAppToCacheError { - path: store.path.clone(), - cache: store.cache.clone(), - error, - })?; - copy_mtime_r(&store.path, &store.cache)?; - - fs_err::remove_dir_all(&store.path).map_err(CacheError::IoError)?; - - Ok(store) -} - fn copy_mtime_r(from: &Path, to_path: &Path) -> Result<(), CacheError> { for entry in WalkDir::new(from).into_iter().filter_map(Result::ok) { let relative = entry From 92f12628e6e8996af3e7126a1ca0ddb514e6d418 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 17 Oct 2024 09:00:17 -0500 Subject: [PATCH 10/11] Add docs --- commons/src/cache/app_cache.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index c3b4949..d32859b 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -302,6 +302,9 @@ fn save(store: &AppCache) -> Result<&AppCache, CacheError> { Ok(store) } +/// Copies the mtime information from a path to another path +/// +/// This is information used for the LRU cleaner so that older files are removed first. fn copy_mtime_r(from: &Path, to_path: &Path) -> Result<(), CacheError> { for entry in WalkDir::new(from).into_iter().filter_map(Result::ok) { let relative = entry From e104dda98e9101e5e09a2baa52f4f55fac250115 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 17 Oct 2024 09:16:23 -0500 Subject: [PATCH 11/11] Copy cache and delete instead of moving --- commons/src/cache/app_cache.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index d32859b..cf09961 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -152,7 +152,7 @@ impl AppCache { fs_err::create_dir_all(&self.path).map_err(CacheError::IoError)?; fs_err::create_dir_all(&self.cache).map_err(CacheError::IoError)?; - fs_extra::dir::move_dir( + fs_extra::dir::copy( &self.cache, &self.path, &CopyOptions { @@ -170,6 +170,8 @@ impl AppCache { })?; copy_mtime_r(&self.cache, &self.path)?; + fs_err::remove_dir_all(&self.cache).map_err(CacheError::IoError)?; + Ok(self) }