Skip to content

Commit 6e3efa3

Browse files
fix: delete file on write failure and preserve parent directory
1 parent 2d55149 commit 6e3efa3

File tree

1 file changed

+53
-6
lines changed

1 file changed

+53
-6
lines changed

pre-compute/src/compute/utils/file_utils.rs

Lines changed: 53 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,15 +118,16 @@ pub fn download_file(url: &str, parent_dir: &str, filename: &str) -> Option<Path
118118
if write_file(&bytes, &file_path, &format!("url:{url}")).is_ok() {
119119
Some(file_path)
120120
} else {
121-
if !parent_existed {
122-
match fs::remove_dir_all(parent_path) {
121+
if file_path.exists() {
122+
match fs::remove_file(&file_path) {
123123
Ok(_) => {
124-
info!("Folder deleted [path:{}]", parent_path.display());
124+
info!("File deleted [path:{}]", file_path.display());
125125
}
126-
Err(_) => {
126+
Err(e) => {
127127
error!(
128-
"Folder does not exist, nothing to delete [path:{}]",
129-
parent_path.display()
128+
"Failed to delete file [path:{}, error:{}]",
129+
file_path.display(),
130+
e
130131
);
131132
}
132133
}
@@ -311,6 +312,52 @@ mod tests {
311312
let result = download_from_url("not-a-valid-url");
312313
assert!(result.is_none());
313314
}
315+
316+
#[test]
317+
#[cfg(unix)]
318+
fn test_download_fails_on_file_write_error_preserves_parent() {
319+
use std::os::unix::fs::PermissionsExt;
320+
let (_container, container_url) = start_container();
321+
322+
let temp_dir = TempDir::new().unwrap();
323+
let nested_path = temp_dir.path().join("new_dir");
324+
325+
// Create the directory and make it read-only to prevent file creation
326+
fs::create_dir_all(&nested_path).unwrap();
327+
let mut perms = fs::metadata(&nested_path).unwrap().permissions();
328+
perms.set_mode(0o555);
329+
fs::set_permissions(&nested_path, perms).unwrap();
330+
331+
let result = download_file(&container_url, nested_path.to_str().unwrap(), "test.json");
332+
333+
// Download should fail due to inability to write file and Parent directory should still exist
334+
assert!(result.is_none());
335+
assert!(nested_path.exists());
336+
}
337+
338+
#[test]
339+
#[cfg(unix)]
340+
fn test_file_deleted_when_write_fails_with_existing_parent() {
341+
use std::os::unix::fs::PermissionsExt;
342+
let (_container, container_url) = start_container();
343+
344+
let temp_dir = TempDir::new().unwrap();
345+
let parent_path = temp_dir.path();
346+
let file_path = parent_path.join("test.json");
347+
348+
// Pre-create a read-only file to force write failure
349+
fs::write(&file_path, b"old content").unwrap();
350+
let mut perms = fs::metadata(&file_path).unwrap().permissions();
351+
perms.set_mode(0o444);
352+
fs::set_permissions(&file_path, perms).unwrap();
353+
354+
let result = download_file(&container_url, parent_path.to_str().unwrap(), "test.json");
355+
assert!(result.is_none());
356+
357+
// Parent directory should still exist File should be deleted on cleanup
358+
assert!(parent_path.exists());
359+
assert!(!file_path.exists(), "File should have been cleaned up after write failure");
360+
}
314361

315362
#[test]
316363
fn test_download_from_url_with_server_error() {

0 commit comments

Comments
 (0)