diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e1b8afe7..9cfb5d7d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -40,3 +40,18 @@ jobs: - name: Build and Test run: | cargo test --verbose --no-fail-fast --features "strict" + build_and_test-win: + runs-on: windows-latest + steps: + - name: Checkout repository + uses: actions/checkout@v4 + - name: Install Rust 1.86 + uses: dtolnay/rust-toolchain@1.86.0 + with: + components: clippy + - name: Lint + run: | + cargo clippy -r --verbose -- -D warnings # elevates warnings to errors + - name: Build and Test + run: | + cargo test --verbose --no-fail-fast --features "strict" diff --git a/cas_client/src/local_client.rs b/cas_client/src/local_client.rs index bbbc3798..500599c8 100644 --- a/cas_client/src/local_client.rs +++ b/cas_client/src/local_client.rs @@ -154,6 +154,7 @@ impl LocalClient { { if let Ok(metadata) = std::fs::metadata(&file_path) { let mut permissions = metadata.permissions(); + #[allow(clippy::permissions_set_readonly_false)] permissions.set_readonly(false); let _ = std::fs::set_permissions(&file_path, permissions); } @@ -276,9 +277,12 @@ impl UploadClient for LocalClient { tempfile.persist(&file_path).map_err(|e| e.error)?; - // attempt to set to readonly - // its ok to fail. - if let Ok(metadata) = std::fs::metadata(&file_path) { + // attempt to set to readonly on unix. + // On windows, this may pose issues if a xorb has recently + // been deleted and `exists` returns false, but the FS + // still has the metadata (and previous xorb was read-only). + #[cfg(unix)] + if let Ok(metadata) = metadata(&file_path) { let mut permissions = metadata.permissions(); permissions.set_readonly(true); let _ = std::fs::set_permissions(&file_path, permissions); diff --git a/chunk_cache/src/disk.rs b/chunk_cache/src/disk.rs index ab1c6d41..752b2520 100644 --- a/chunk_cache/src/disk.rs +++ b/chunk_cache/src/disk.rs @@ -372,8 +372,13 @@ impl DiskCache { // removing by index in reverse to guarantee lower-index items aren't shifted/moved for item_idx in to_remove.into_iter().rev() { let item = items.swap_remove(item_idx); - overlapping_item_paths.insert(self.item_path(key, &item)?); - total_bytes_rm += item.len; + // We only remove from the disk if the item found is not equal to the cache_item + // we just wrote. This can happen when multiple put calls are made for the same + // item simultaneously. + if item != cache_item { + overlapping_item_paths.insert(self.item_path(key, &item)?); + total_bytes_rm += item.len; + } } state.num_items -= num_items_rm; state.total_bytes -= total_bytes_rm; @@ -1300,4 +1305,44 @@ mod concurrency_tests { handle.await.expect("join should not error"); } } + + #[tokio::test(flavor = "multi_thread")] + async fn test_run_concurrently_thundering_herd() { + let cache_root = TempDir::new("run_concurrently_thundering_herd").unwrap(); + let config = CacheConfig { + cache_directory: cache_root.path().to_path_buf(), + cache_size: RANGE_LEN as u64 * NUM_ITEMS_PER_TASK as u64, + }; + let cache = DiskCache::initialize(&config).unwrap(); + + // data inserted is the same + let mut it = RandomEntryIterator::std_from_seed(RANDOM_SEED); + let (key, range, chunk_byte_indices, data) = it.next().unwrap(); + + // Spawn tasks to simultaneously insert into cache + let num_tasks = 64; + let mut handles = Vec::with_capacity(num_tasks as usize); + for _ in 0..num_tasks { + let cache_clone = cache.clone(); + let key = key.clone(); + let range = range.clone(); + let chunk_byte_indices = chunk_byte_indices.clone(); + let data_clone = data.clone(); + handles.push(tokio::spawn(async move { + let res = cache_clone.put(&key, &range, &chunk_byte_indices, &data_clone); + assert!(res.is_ok(), "err: {res:?}"); + })) + } + + for handle in handles { + handle.await.expect("join should not error"); + } + + // check that there is only 1 term in the cache for this data + let state = cache.state.lock().unwrap(); + let items = state.inner.get(&key).unwrap(); + + let num = items.iter().filter(|item| item.range == range).count(); + assert_eq!(num, 1); + } } diff --git a/chunk_cache/src/disk/cache_item.rs b/chunk_cache/src/disk/cache_item.rs index d406549b..ad3735ca 100644 --- a/chunk_cache/src/disk/cache_item.rs +++ b/chunk_cache/src/disk/cache_item.rs @@ -86,6 +86,12 @@ impl VerificationCell { } } +impl PartialEq for VerificationCell { + fn eq(&self, other: &T) -> bool { + self.inner.eq(other) + } +} + // range start, range end, length, and checksum const CACHE_ITEM_FILE_NAME_BUF_SIZE: usize = size_of::() * 2 + size_of::() + size_of::(); @@ -101,7 +107,7 @@ pub(crate) struct CacheItem { } impl std::fmt::Display for CacheItem { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!(f, "CacheItem {{ range: {:?}, len: {}, checksum: {} }}", self.range, self.len, self.checksum,) } } diff --git a/data/tests/test_clean_smudge.rs b/data/tests/test_clean_smudge.rs index a6c8f252..e36625b1 100644 --- a/data/tests/test_clean_smudge.rs +++ b/data/tests/test_clean_smudge.rs @@ -105,7 +105,7 @@ pub fn check_directories_match(dir1: &Path, dir2: &Path) { async fn dehydrate_directory(cas_dir: &Path, src_dir: &Path, ptr_dir: &Path) { let config = TranslatorConfig::local_config(cas_dir).unwrap(); - std::fs::create_dir_all(&ptr_dir).unwrap(); + create_dir_all(ptr_dir).unwrap(); let upload_session = FileUploadSession::new(config.clone(), ThreadPool::from_current_runtime(), None) .await @@ -132,7 +132,7 @@ async fn dehydrate_directory(cas_dir: &Path, src_dir: &Path, ptr_dir: &Path) { async fn hydrate_directory(cas_dir: &Path, ptr_dir: &Path, out_dir: &Path) { let config = TranslatorConfig::local_config(cas_dir).unwrap(); - std::fs::create_dir_all(&out_dir).unwrap(); + create_dir_all(out_dir).unwrap(); let downloader = FileDownloader::new(config, ThreadPool::from_current_runtime()).await.unwrap(); diff --git a/error_printer/tests/test_error.rs b/error_printer/tests/test_error.rs index 3228d42d..1cb36bce 100644 --- a/error_printer/tests/test_error.rs +++ b/error_printer/tests/test_error.rs @@ -60,9 +60,12 @@ fn test_ok() { } fn check_logs bool>(logs_contain: F, log_level: &str, line_num: i32) { - let expected_line = format!("{}:{}", file!(), line_num); - assert!(logs_contain(log_level)); assert!(logs_contain("test, error: \"some error\"")); + #[cfg(not(windows))] + let expected_line = format!("{}:{}", file!(), line_num); + #[cfg(windows)] + let expected_line = format!("{}:{}", file!(), line_num).replace("\\", "\\\\"); + assert!(logs_contain(&expected_line)); } diff --git a/error_printer/tests/test_option.rs b/error_printer/tests/test_option.rs index b3b0757b..49a439e9 100644 --- a/error_printer/tests/test_option.rs +++ b/error_printer/tests/test_option.rs @@ -60,9 +60,11 @@ fn test_some() { } fn check_logs bool>(logs_contain: F, log_level: &str, line_num: i32) { - let expected_line = format!("{}:{}", file!(), line_num); - assert!(logs_contain(log_level)); assert!(logs_contain("opt is None")); + #[cfg(not(windows))] + let expected_line = format!("{}:{}", file!(), line_num); + #[cfg(windows)] + let expected_line = format!("{}:{}", file!(), line_num).replace("\\", "\\\\"); assert!(logs_contain(&expected_line)); } diff --git a/file_utils/src/file_metadata.rs b/file_utils/src/file_metadata.rs index 2e137520..26e2f80f 100644 --- a/file_utils/src/file_metadata.rs +++ b/file_utils/src/file_metadata.rs @@ -2,9 +2,11 @@ use std::fs::Metadata; #[cfg(unix)] use std::os::unix::fs::MetadataExt; use std::path::Path; +#[cfg(unix)] use std::time::SystemTime; /// Matches the metadata of a file to another file's metadata +#[cfg(unix)] pub fn set_file_metadata>(path: P, metadata: &Metadata, match_owner: bool) -> std::io::Result<()> { let path = path.as_ref(); @@ -28,7 +30,6 @@ pub fn set_file_metadata>(path: P, metadata: &Metadata, match_own }, ]; - #[cfg(unix)] if let Some(path_s) = path.to_str() { if match_owner { // Set ownership @@ -46,7 +47,19 @@ pub fn set_file_metadata>(path: P, metadata: &Metadata, match_own Ok(()) } +/// Matches the metadata of a file to another file's metadata +#[cfg(not(unix))] +pub fn set_file_metadata>(path: P, metadata: &Metadata, _match_owner: bool) -> std::io::Result<()> { + let path = path.as_ref(); + + // Set permissions + let permissions = metadata.permissions(); + std::fs::set_permissions(path, permissions.clone())?; + Ok(()) +} + #[cfg(test)] +#[cfg(unix)] mod tests { use std::fs::{self, File}; use std::os::unix::fs::PermissionsExt; @@ -90,7 +103,6 @@ mod tests { } #[test] - #[cfg(unix)] fn test_set_metadata_timestamps() { let dir = tempdir().unwrap(); let file_path = dir.path().join("test_file"); @@ -135,7 +147,6 @@ mod tests { } #[test] - #[cfg(unix)] fn test_set_metadata_owner() { let dir = tempdir().unwrap(); let file_path = dir.path().join("test_file"); @@ -163,3 +174,45 @@ mod tests { assert_eq!(updated_metadata.gid(), src_metadata.gid()); } } + +#[cfg(test)] +#[cfg(not(unix))] +mod tests { + use std::fs::{self, File}; + + use tempfile::tempdir; + + use super::*; + + #[test] + fn test_set_metadata_permissions() { + let dir = tempdir().unwrap(); + let file_path = dir.path().join("test_file"); + File::create(&file_path).unwrap(); + + // Set some initial permissions + let mut perms = File::open(&file_path).unwrap().metadata().unwrap().permissions(); + perms.set_readonly(false); + + fs::set_permissions(&file_path, perms.clone()).unwrap(); + + // Create a file with different permissions to copy from + let src_file_path = dir.path().join("src_file"); + let src_file = File::create(&src_file_path).unwrap(); + let mut src_perms = src_file.metadata().unwrap().permissions(); + src_perms.set_readonly(true); + fs::set_permissions(&src_file_path, src_perms.clone()).unwrap(); + + let src_metadata = src_file.metadata().unwrap(); + + // Apply set_metadata + set_file_metadata(&file_path, &src_metadata, false).unwrap(); + + // Check that permissions have been updated. But we need to re-read some of the things + // as Unix adds an extra bit indicating a regular file. + let updated_metadata = File::open(file_path).unwrap().metadata().unwrap(); + let src_metadata = File::open(src_file_path).unwrap().metadata().unwrap(); + + assert_eq!(updated_metadata.permissions().readonly(), src_metadata.permissions().readonly()); + } +} diff --git a/file_utils/src/safe_file_creator.rs b/file_utils/src/safe_file_creator.rs index 6b61ce7b..199ab9e4 100644 --- a/file_utils/src/safe_file_creator.rs +++ b/file_utils/src/safe_file_creator.rs @@ -59,7 +59,7 @@ impl SafeFileCreator { /// and additionally the metadata of the old one will match the new metadata. pub fn replace_existing>(dest_path: P) -> io::Result { let mut s = Self::new(&dest_path)?; - s.original_metadata = std::fs::metadata(dest_path).ok(); + s.original_metadata = fs::metadata(dest_path).ok(); Ok(s) } @@ -67,9 +67,9 @@ impl SafeFileCreator { fn temp_file_path(dest_path: Option<&Path>) -> io::Result { let (parent, file_name) = match dest_path { Some(p) => { - let parent = (p.parent().ok_or_else(|| { + let parent = p.parent().ok_or_else(|| { io::Error::new(io::ErrorKind::InvalidInput, "path doesn't have a valid parent directory") - }))?; + })?; let file_name = p.file_name().ok_or_else(|| { io::Error::new(io::ErrorKind::InvalidInput, "path doesn't have a valid file name") })?; @@ -125,7 +125,7 @@ impl SafeFileCreator { fn writer(&mut self) -> io::Result<&mut BufWriter> { match &mut self.writer { Some(wr) => Ok(wr), - None => Err(std::io::Error::new( + None => Err(io::Error::new( io::ErrorKind::BrokenPipe, format!("Writing to {:?} already completed.", &self.dest_path), )), @@ -160,7 +160,7 @@ pub fn write_all_safe(path: &Path, bytes: &[u8]) -> io::Result<()> { // Make sure dir exists. if !dir.exists() { - std::fs::create_dir_all(dir)?; + fs::create_dir_all(dir)?; } let mut tempfile = create_temp_file(dir, "")?; @@ -184,6 +184,7 @@ pub fn create_temp_file(dir: &Path, suffix: &str) -> io::Result { mod tests { use std::fs::{self, File}; use std::io::Read; + #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use tempfile::tempdir; @@ -207,6 +208,7 @@ mod tests { // Verify file permissions let metadata = fs::metadata(&dest_path).unwrap(); let permissions = metadata.permissions(); + #[cfg(unix)] assert_eq!(permissions.mode() & 0o777, 0o644); // Assuming default creation mode } @@ -232,6 +234,7 @@ mod tests { // Verify file permissions let metadata = fs::metadata(&dest_path).unwrap(); let permissions = metadata.permissions(); + #[cfg(unix)] assert_eq!(permissions.mode() & 0o777, 0o644); // Assuming default creation mode } @@ -245,6 +248,7 @@ mod tests { let mut file = File::create(&dest_path).unwrap(); file.write_all(b"Old content").unwrap(); let mut perms = file.metadata().unwrap().permissions(); + #[cfg(unix)] perms.set_mode(0o600); fs::set_permissions(&dest_path, perms).unwrap(); } @@ -261,6 +265,7 @@ mod tests { // Verify file permissions let metadata = fs::metadata(&dest_path).unwrap(); let permissions = metadata.permissions(); + #[cfg(unix)] assert_eq!(permissions.mode() & 0o777, 0o600); // Original file mode } @@ -324,6 +329,7 @@ mod tests { // Verify file permissions let metadata = fs::metadata(&dest_path).unwrap(); let permissions = metadata.permissions(); + #[cfg(unix)] assert_eq!(permissions.mode() & 0o777, 0o600); // Original file mode } }