From 6e3efa39292ffbc78e475bcf5020fccf3512e507 Mon Sep 17 00:00:00 2001 From: nabil-Tounarti Date: Fri, 7 Nov 2025 10:20:23 +0100 Subject: [PATCH 1/5] fix: delete file on write failure and preserve parent directory --- pre-compute/src/compute/utils/file_utils.rs | 59 ++++++++++++++++++--- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/pre-compute/src/compute/utils/file_utils.rs b/pre-compute/src/compute/utils/file_utils.rs index 65a8b87..1b2b83e 100644 --- a/pre-compute/src/compute/utils/file_utils.rs +++ b/pre-compute/src/compute/utils/file_utils.rs @@ -118,15 +118,16 @@ pub fn download_file(url: &str, parent_dir: &str, filename: &str) -> Option { - info!("Folder deleted [path:{}]", parent_path.display()); + info!("File deleted [path:{}]", file_path.display()); } - Err(_) => { + Err(e) => { error!( - "Folder does not exist, nothing to delete [path:{}]", - parent_path.display() + "Failed to delete file [path:{}, error:{}]", + file_path.display(), + e ); } } @@ -311,6 +312,52 @@ mod tests { let result = download_from_url("not-a-valid-url"); assert!(result.is_none()); } + + #[test] + #[cfg(unix)] + fn test_download_fails_on_file_write_error_preserves_parent() { + use std::os::unix::fs::PermissionsExt; + let (_container, container_url) = start_container(); + + let temp_dir = TempDir::new().unwrap(); + let nested_path = temp_dir.path().join("new_dir"); + + // Create the directory and make it read-only to prevent file creation + fs::create_dir_all(&nested_path).unwrap(); + let mut perms = fs::metadata(&nested_path).unwrap().permissions(); + perms.set_mode(0o555); + fs::set_permissions(&nested_path, perms).unwrap(); + + let result = download_file(&container_url, nested_path.to_str().unwrap(), "test.json"); + + // Download should fail due to inability to write file and Parent directory should still exist + assert!(result.is_none()); + assert!(nested_path.exists()); + } + + #[test] + #[cfg(unix)] + fn test_file_deleted_when_write_fails_with_existing_parent() { + use std::os::unix::fs::PermissionsExt; + let (_container, container_url) = start_container(); + + let temp_dir = TempDir::new().unwrap(); + let parent_path = temp_dir.path(); + let file_path = parent_path.join("test.json"); + + // Pre-create a read-only file to force write failure + fs::write(&file_path, b"old content").unwrap(); + let mut perms = fs::metadata(&file_path).unwrap().permissions(); + perms.set_mode(0o444); + fs::set_permissions(&file_path, perms).unwrap(); + + let result = download_file(&container_url, parent_path.to_str().unwrap(), "test.json"); + assert!(result.is_none()); + + // Parent directory should still exist File should be deleted on cleanup + assert!(parent_path.exists()); + assert!(!file_path.exists(), "File should have been cleaned up after write failure"); + } #[test] fn test_download_from_url_with_server_error() { From ab4de8ce991c210cf959c33584a2df0954d97557 Mon Sep 17 00:00:00 2001 From: nabil-Tounarti Date: Fri, 7 Nov 2025 10:28:52 +0100 Subject: [PATCH 2/5] style: format the code --- pre-compute/src/compute/utils/file_utils.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pre-compute/src/compute/utils/file_utils.rs b/pre-compute/src/compute/utils/file_utils.rs index 1b2b83e..7d0875c 100644 --- a/pre-compute/src/compute/utils/file_utils.rs +++ b/pre-compute/src/compute/utils/file_utils.rs @@ -312,7 +312,7 @@ mod tests { let result = download_from_url("not-a-valid-url"); assert!(result.is_none()); } - + #[test] #[cfg(unix)] fn test_download_fails_on_file_write_error_preserves_parent() { @@ -321,7 +321,7 @@ mod tests { let temp_dir = TempDir::new().unwrap(); let nested_path = temp_dir.path().join("new_dir"); - + // Create the directory and make it read-only to prevent file creation fs::create_dir_all(&nested_path).unwrap(); let mut perms = fs::metadata(&nested_path).unwrap().permissions(); @@ -330,7 +330,7 @@ mod tests { let result = download_file(&container_url, nested_path.to_str().unwrap(), "test.json"); - // Download should fail due to inability to write file and Parent directory should still exist + // Download should fail due to inability to write file. Parent directory should still exist assert!(result.is_none()); assert!(nested_path.exists()); } @@ -344,7 +344,7 @@ mod tests { let temp_dir = TempDir::new().unwrap(); let parent_path = temp_dir.path(); let file_path = parent_path.join("test.json"); - + // Pre-create a read-only file to force write failure fs::write(&file_path, b"old content").unwrap(); let mut perms = fs::metadata(&file_path).unwrap().permissions(); @@ -353,10 +353,13 @@ mod tests { let result = download_file(&container_url, parent_path.to_str().unwrap(), "test.json"); assert!(result.is_none()); - - // Parent directory should still exist File should be deleted on cleanup + + // Parent directory should still exist. File should be deleted on cleanup assert!(parent_path.exists()); - assert!(!file_path.exists(), "File should have been cleaned up after write failure"); + assert!( + !file_path.exists(), + "File should have been cleaned up after write failure" + ); } #[test] From f891e5c28e8850acd7ab3151a65f17fb1898db73 Mon Sep 17 00:00:00 2001 From: nabil-Tounarti Date: Fri, 7 Nov 2025 12:04:04 +0100 Subject: [PATCH 3/5] docs: clarify file deletion behavior on write failure --- pre-compute/src/compute/utils/file_utils.rs | 36 ++++++++++----------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/pre-compute/src/compute/utils/file_utils.rs b/pre-compute/src/compute/utils/file_utils.rs index 7d0875c..b269822 100644 --- a/pre-compute/src/compute/utils/file_utils.rs +++ b/pre-compute/src/compute/utils/file_utils.rs @@ -54,7 +54,7 @@ pub fn write_file(content: &[u8], file_path: &Path, context: &str) -> Result<(), /// /// If the download or any file operation fails, the function logs an appropriate error /// and returns `None`. It also ensures the parent directory exists, creating it if necessary. -/// If the directory is newly created but the file write fails, it is cleaned up (deleted). +/// If the file write fails and a partial file was created, it will be cleaned up (deleted). /// /// # Arguments /// @@ -115,24 +115,24 @@ pub fn download_file(url: &str, parent_dir: &str, filename: &str) -> Option { - info!("File deleted [path:{}]", file_path.display()); - } - Err(e) => { - error!( - "Failed to delete file [path:{}, error:{}]", - file_path.display(), - e - ); + match write_file(&bytes, &file_path, &format!("url:{url}")) { + Ok(_) => Some(file_path), + Err(_) => { + if file_path.exists() { + match fs::remove_file(&file_path) { + Ok(_) => { + info!("File deleted [path:{}]", file_path.display()); + } + Err(e) => { + error!( + "Failed to delete file [path:{}, error:{e}]", + file_path.display() + ); + } } } + None } - None } } @@ -315,7 +315,7 @@ mod tests { #[test] #[cfg(unix)] - fn test_download_fails_on_file_write_error_preserves_parent() { + fn download_from_url_fails_and_preserves_parent_dir_when_file_write_error() { use std::os::unix::fs::PermissionsExt; let (_container, container_url) = start_container(); @@ -337,7 +337,7 @@ mod tests { #[test] #[cfg(unix)] - fn test_file_deleted_when_write_fails_with_existing_parent() { + fn download_from_url_fails_and_file_deleted_when_write_partialy_fails() { use std::os::unix::fs::PermissionsExt; let (_container, container_url) = start_container(); From fac3d9b7dd655c5101f3b92aa39591628325614c Mon Sep 17 00:00:00 2001 From: nabil-Tounarti Date: Fri, 7 Nov 2025 16:57:53 +0100 Subject: [PATCH 4/5] feat: delete file on write failure in write_file function --- pre-compute/src/compute/utils/file_utils.rs | 26 ++++++++++----------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/pre-compute/src/compute/utils/file_utils.rs b/pre-compute/src/compute/utils/file_utils.rs index b269822..c559aca 100644 --- a/pre-compute/src/compute/utils/file_utils.rs +++ b/pre-compute/src/compute/utils/file_utils.rs @@ -45,6 +45,19 @@ pub fn write_file(content: &[u8], file_path: &Path, context: &str) -> Result<(), "Failed to write file [{context}, path:{}]", file_path.display() ); + if file_path.exists() { + match fs::remove_file(file_path) { + Ok(_) => { + info!("File deleted [path:{}]", file_path.display()); + } + Err(e) => { + error!( + "Failed to delete file [path:{}, error:{e}]", + file_path.display() + ); + } + } + } Err(e) } } @@ -118,19 +131,6 @@ pub fn download_file(url: &str, parent_dir: &str, filename: &str) -> Option Some(file_path), Err(_) => { - if file_path.exists() { - match fs::remove_file(&file_path) { - Ok(_) => { - info!("File deleted [path:{}]", file_path.display()); - } - Err(e) => { - error!( - "Failed to delete file [path:{}, error:{e}]", - file_path.display() - ); - } - } - } None } } From d20525833b52a713f3c2c46e0cd6723b6f897072 Mon Sep 17 00:00:00 2001 From: nabil-Tounarti Date: Fri, 7 Nov 2025 17:08:13 +0100 Subject: [PATCH 5/5] style: format the code --- pre-compute/src/compute/utils/file_utils.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pre-compute/src/compute/utils/file_utils.rs b/pre-compute/src/compute/utils/file_utils.rs index c559aca..7dfe53d 100644 --- a/pre-compute/src/compute/utils/file_utils.rs +++ b/pre-compute/src/compute/utils/file_utils.rs @@ -130,9 +130,7 @@ pub fn download_file(url: &str, parent_dir: &str, filename: &str) -> Option Some(file_path), - Err(_) => { - None - } + Err(_) => None, } }